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

[Abhinav Kumar]

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.