Message-ID: <0f988169-58cb-eccf-9590-f5097e085f22@quicinc.com>
Date:   Thu, 22 Jun 2023 16:37:09 -0700
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Jessica Zhang <quic_jesszhan@...cinc.com>
CC:     Sean Paul <sean@...rly.run>, <freedreno@...ts.freedesktop.org>,
        <linux-kernel@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        Rob Clark <robdclark@...il.com>,
        Daniel Vetter <daniel@...ll.ch>,
        <linux-arm-msm@...r.kernel.org>, David Airlie <airlied@...il.com>
Subject: Re: [Freedreno] [PATCH 2/3] drm/msm/dpu: Set DATABUS_WIDEN on command
 mode encoders



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.

>>
>>> Not to mention that it allows us to program required registers 
>>> directly (by setting values) rather than using RMW cycles and thus 
>>> depending on the value being previously programmed to these registers.
>>>
>>
>> This will not change. We will still have to use RMW cycles to preserve 
>> the reset values of some of the fields as those are the right values 
>> for the registers and shouldnt be touched.
> 
> I'd like to point to the dpu_hw_intf_setup_timing_engine(), a close 
> rival callback, setting up the INTF for video mode. It does not do RMW 
> cycles, it just writes all the registers.
> 

Yes as it ends up ORing pretty much all the usable bits. So it works fine.

> In the worst case, there will be a single RMW instead of having multiple 
> of them.
> 
> 
>>
>>>>
>>>>>>  Similar to how dpu_hw_intf_setup_timing_engine() programs the
>>>>>> hw_intf - including widebus! - for video-mode.
>>>>>>
>>>>>> Or even more generically, have a struct similar to intf_timing_params
>>>>>> that says how the intf needs to be configured - without the caller
>>>>>> knowing about INTF_CONFIG2.
>>>>>>
>>>>>> struct dpu_hw_intf_cfg is a very good example of how we can use a 
>>>>>> single
>>>>>> struct and a single callback to configure multiple registers at once
>>>>>> based on some input parameters.
>>>>>>
>>>>>>> In addition, it seems that video mode does all its INTF_CONFIG2
>>>>>>> configuration separately in dpu_hw_intf_setup_timing_engine(). If we
>>>>>>> have a generic set_intf_config2() op, it might be good to have it as
>>>>>>> part of a larger cleanup where we have both video and command 
>>>>>>> mode use
>>>>>>> the generic op. What are your thoughts on this?
>>>>>>
>>>>>> Not in that way, but if there is a generic enable_compression() or
>>>>>> configure_compression() callback (or even more generic, similar to
>>>>>> setup_intf_cfg in dpu_hw_ctl) that would work for both video-mode and
>>>>>> command-mode, maybe that is beneficial.
>>>>>
>>>>> I'd rather not do this. Let's just 'setup timing enging' vs 'setup 
>>>>> CMD'. For example, it might also include setting up other INTF 
>>>>> parameters for CMD mode (if anything is required later on).
>>>>>
>>>>
>>>> Agreed on setup CMD but I dont know whether we need a setup CMD at all.
>>>> Seems like an overkill.
>>>>
>>>>>>
>>>>>> - Marijn
>>>>>
>>>
> 

Powered by blists - more mailing lists