Message-ID: <dab90e58-c25a-9b5a-bae0-dfc50de7d363@linaro.org>
Date:   Fri, 23 Jun 2023 02:14:56 +0300
From:   Dmitry Baryshkov <dmitry.baryshkov@...aro.org>
To:     Abhinav Kumar <quic_abhinavk@...cinc.com>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        Jessica Zhang <quic_jesszhan@...cinc.com>
Cc:     Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        David Airlie <airlied@...il.com>,
        Daniel Vetter <daniel@...ll.ch>, linux-arm-msm@...r.kernel.org,
        freedreno@...ts.freedesktop.org, dri-devel@...ts.freedesktop.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH 2/3] drm/msm/dpu: Set DATABUS_WIDEN on command mode
 encoders

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.

> 
>> 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.

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
>>>>
>>

-- 
With best wishes
Dmitry

Powered by blists - more mailing lists