Re: [Freedreno] [PATCH 2/3] drm/msm/dpu: Set DATABUS_WIDEN on command mode encoders

[Jessica Zhang]

On 6/29/2023 10:26 AM, Abhinav Kumar wrote:
> On 6/22/2023 4:37 PM, Abhinav Kumar wrote:
> > On 6/22/2023 4:14 PM, Dmitry Baryshkov wrote:
> > > On 23/06/2023 01:37, Abhinav Kumar wrote:
> > > > On 6/21/2023 4:46 PM, Dmitry Baryshkov wrote:
> > > > > On 22/06/2023 02:01, Abhinav Kumar wrote:
> > > > > > On 6/21/2023 9:36 AM, Dmitry Baryshkov wrote:
> > > > > > > On 21/06/2023 18:17, Marijn Suijten wrote:
> > > > > > > > On 2023-06-20 14:38:34, Jessica Zhang wrote:
> > > > > > > > <snip>
> > > > > > > > > > > > > > +    if (phys_enc->hw_intf->ops.enable_widebus)
> > > > > > > > > > > > > > + phys_enc->hw_intf->ops.enable_widebus(phys_enc->hw_intf);
> > > > > > > > > > > > >
> > > > > > > > > > > > > No. Please provide a single function which takes necessary
> > > > > > > > > > > > > configuration, including compression and wide_bus_enable.
> > > > > > > > > > > >
> > > > > > > > > > > > There are two ways to look at this. Your point is coming 
> > > > > > > > > > > > from the
> > > > > > > > > > > > perspective that its programming the same register but just 
> > > > > > > > > > > > a different
> > > > > > > > > > > > bit. But that will also make it a bit confusing.
> > > > > > > > > >
> > > > > > > > > > My point is to have a high-level function that configures the 
> > > > > > > > > > INTF for
> > > > > > > > > > the CMD mode. This way it can take a structure with necessary
> > > > > > > > > > configuration bits.
> > > > > > > > >
> > > > > > > > > Hi Dmitry,
> > > > > > > > >
> > > > > > > > > After discussing this approach with Abhinav, we still have a few
> > > > > > > > > questions about it:
> > > > > > > > >
> > > > > > > > > Currently, only 3 of the 32 bits for INTF_CONFIG2 are being 
> > > > > > > > > used (the
> > > > > > > > > rest are reserved with no plans of being programmed in the 
> > > > > > > > > future). Does
> > > > > > > > > this still justify the use of a struct to pass in the necessary
> > > > > > > > > configuration?
> > > > > > > >
> > > > > > > > No.  The point Dmitry is making is **not** about this concidentally
> > > > > > > > using the same register, but about adding a common codepath to 
> > > > > > > > enable
> > > > > > > > compression on this hw_intf (regardless of the registers it 
> > > > > > > > needs to
> > > > > > > > touch).
> > > > > > >
> > > > > > > Actually to setup INTF for CMD stream (which is equal to setting 
> > > > > > > up compression at this point).
> > > > > >
> > > > > > Yes it should be setup intf for cmd and not enable compression.
> > > > > >
> > > > > > Widebus and compression are different features and we should be 
> > > > > > able to control them independently.
> > > > > >
> > > > > > We just enable them together for DSI. So a separation is necessary.
> > > > > >
> > > > > > But I am still not totally convinced we even need to go down the 
> > > > > > path for having an op called setup_intf_cmd() which takes in a 
> > > > > > struct like
> > > > > >
> > > > > > struct dpu_cmd_intf_cfg {
> > > > > >      bool data_compress;
> > > > > >      bool widebus_en;
> > > > > > };
> > > > > >
> > > > > > As we have agreed that we will not touch the video mode timing 
> > > > > > engine path, it leaves us with only two bits.
> > > > > >
> > > > > > And like I said, its not that these two bits always go together. 
> > > > > > We want to be able to control them independently which means that 
> > > > > > its not necessary both bits program the same register one by one. 
> > > > > > We might just end up programming one of them if we just use widebus.
> > > > > >
> > > > > > Thats why I am still leaning on keeping this approach.
> > > > >
> > > > > I do not like the idea of having small functions being called 
> > > > > between modules. So, yes there will a config of two booleans, but 
> > > > > it is preferable (and more scalable) compared to separate callbacks.
> > > >
> > > > I definitely agree with the scalable part and I even cross checked 
> > > > that the number of usable bitfields of this register is going up 
> > > > from one chipset to the other although once again that depends on 
> > > > whether we will use those features.
> > > >
> > > > For that reason I am not opposed to the struct idea.
> > > >
> > > > But there is also another pattern i am seeing which worries me. 
> > > > Usable bitfields sometimes even reduce. For those cases, if we go 
> > > > with a pre-defined struct it ends up with redundant members as those 
> > > > bitfields go away.
> > > >
> > > > With the function op based approach, we just call the op if the 
> > > > feature bit / core revision.
> > > >
> > > > So I wanted to check once more about the fact that we should 
> > > > consider not just expansion but also reduction.
> > >
> > > As we have to support all generations, there is no actual reduction. 
> > > Newer SoCs do not have particular feature/bit, but older ones do. By 
> > > having multiple optional ops we just move this knowledge from 
> > > ops->complex_callback() to _setup_block_ops(). But more importantly 
> > > the caller gets more complicated. Instead of just calling 
> > > ops->set_me_up(), it has to check all the optional callbacks and call 
> > > the one by one.
> >
> > Alright, I am thinking that perhaps because this register is kind of 
> > unique that its actually controlling a specific setting in the 
> > datapath, downstream also has separate ops for this.
> >
> > But thats fine, we can go ahead with the struct based approach.
>
> As data_compress has already landed, let me introduced the struct along 
> with the core_revision based approach in the core_revision series and 
> this series will expand that struct to include widebus too.

Acked. Will rebase on top of the core_revision series and add widebus to 
the config struct.

Thanks,

Jessica Zhang