Re: [PATCH 18/35] drm/msm/dpu: get rid of DPU_PINGPONG_DSC

[Abhinav Kumar <quic_abhinavk@quicinc.com>]

On 1/23/2025 9:19 PM, Dmitry Baryshkov wrote:
> On Thu, Jan 23, 2025 at 01:41:14PM -0800, Abhinav Kumar wrote:
>>
>>
>> On 1/23/2025 1:32 PM, Abhinav Kumar wrote:
>>>
>>>
>>> On 12/13/2024 2:14 PM, Dmitry Baryshkov wrote:
>>>> Continue migration to the MDSS-revision based checks and replace
>>>> DPU_PINGPONG_DSC feature bit with the core_major_ver < 7 check.
>>>>
>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
>>>> ---
>>>>    drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_14_msm8937.h |  2 --
>>>>    drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_15_msm8917.h |  1 -
>>>>    drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_16_msm8953.h |  2 --
>>>>    drivers/gpu/drm/msm/disp/dpu1/catalog/dpu_1_7_msm8996.h  |  6 ++----
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c           | 10
>>>> ++--------
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h           |  2 --
>>>>    drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c          |  2 +-
>>>>    7 files changed, 5 insertions(+), 20 deletions(-)
>>>>
>>>
>>> <snip>
>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> index 36c0ec775b92036eaab26e1fa5331579651ac27c..49e03ecee9e8b567a3f809b977deb83731006ac0
>>>> 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_pingpong.c
>>>> @@ -319,7 +319,7 @@ struct dpu_hw_pingpong
>>>> *dpu_hw_pingpong_init(struct drm_device *dev,
>>>>            c->ops.disable_autorefresh = dpu_hw_pp_disable_autorefresh;
>>>>        }
>>>> -    if (test_bit(DPU_PINGPONG_DSC, &cfg->features)) {
>>>> +    if (mdss_rev->core_major_ver < 7) {
>>>>            c->ops.setup_dsc = dpu_hw_pp_setup_dsc;
>>>>            c->ops.enable_dsc = dpu_hw_pp_dsc_enable;
>>>>            c->ops.disable_dsc = dpu_hw_pp_dsc_disable;
>>>>
>>>
>>> So far in this series, we replaced the feature bits with >= checks of
>>> core_revisions. That kind of works as usually feature bits get added
>>> after a specific version.
>>>
>>> With this patch and later, whenever we use < checks it gets a bit tricky
>>> as we might also need an upper bound. Feature bits gave individual
>>> control of chipsets but generalizing that all chipsets < 7 have PP DSC
>>> is also not correct. I have to really cross-check but there could be
>>> some old chipsets which do not have DSC at all.
>>
>> This raises another question as well.
>>
>> what if some register was introduced >= X version but was then dropped in a
>> newer chipset.
>>
>> Is it not difficult for the user to go back to the files of each of the
>> sub-blocks and alter these checks rather than just fixing up the catalog.
> 
> Well, the obvious example we are going to have is the CTL_LAYER_EXT4,
> but if I understand correctly the whole block is going to be dropped, so
> maybe it's not that relevant.
> 
> Another example might be CWB, where we are going to have 5.x-7.x and
> 8.x+ DPU ranges.
> 
> Basically, yes, when adding support for a new platform we have to audit
> HW blocks. But this applied even beforehand, where new platforms could
> be drooping existing regs (8.x+ dropping a part of the TOP region).
> 

Right, I am wondering what is the trade-off here.

1) Feature bits allow selective control for every chipset. IOW, the 
specific version is already checked for. I do admit that I have seen 
errors about using the correct feature mask sometimes but even with this 
change, evey developer will need to go and check every sub-block file 
which they might not even know about.

2) core_revision certainly can generalize but we might still need 
absolute versions for some of the bits anyway. So the checks may not 
always be >= or < but it could also end up with something like

if (major_rev == xxx && minor_rev == yyy)
	ops = ops_a;
else if (major_rev == aaa &&& minor_rev == bbb)
	ops = ops_b

So the revision check will most likely end up being more complicated 
than simple range checks. With each catalog feature bit atleast we are 
guaranteed that its applied only to that revision.

I do see in the cover letter, about incorrect feature bits sometimes 
used but I am trying to assess the trade-offs even with this approach.