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

[Jessica Zhang <quic_jesszhan@quicinc.com>]

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