[Freedreno] [PATCH v4 01/42] drm/msm/dpu: use CTL_SC7280_MASK for sm8450's ctl_0

[Dmitry Baryshkov]

On 05/04/2023 03:39, Abhinav Kumar wrote:
> 
> 
> On 4/4/2023 5:33 PM, Dmitry Baryshkov wrote:
>> On 05/04/2023 01:12, Abhinav Kumar wrote:
>>>
>>>
>>> On 4/4/2023 6:05 AM, Dmitry Baryshkov wrote:
>>>> On sm8450 platform the CTL_0 doesn't differ from the rest of CTL 
>>>> blocks,
>>>> so switch it to CTL_SC7280_MASK too.
>>>>
>>>> Some background: original commit 100d7ef6995d ("drm/msm/dpu: add 
>>>> support
>>>> for SM8450") had all (relevant at that time) bit spelled individually.
>>>> Then commit 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 to hw catalog"),
>>>> despite being a mismerge, correctly changed all other CTL entries to 
>>>> use
>>>> CTL_SC7280_MASK, except CTL_0.
>>>>
>>>
>>> I think having it spelled individually is better. If we start using 
>>> one chipset's mask for another, we are again going down the same path 
>>> of this becoming one confused file.
>>>
>>> So, even though I agree that 0e91bcbb0016 ("drm/msm/dpu: Add SM8350 
>>> to hw catalog") corrected the mask to re-use sc7280, with the 
>>> individual catalog file, its better to have it separate and spelled 
>>> individually.
>>>
>>> This change is not heading in the direction of the rest of the series.
>>
>> I didn't create duplicates of all the defines. This is done well in 
>> the style of patch37. I'm not going to add all per-SoC feature masks.
>>
> 
> Yes, I was actually going to comment even on patch 37.
> 
> We are again trying to generalize a CTL's caps based on DPU version, the 
> same mistake which led us down this path.
> 
> So today you have CTL_DPU_0_MASK , CTL_DPU_5_MASK , CTL_DPU_7_MASK  and 
> CTL_DPU_9_MASK and this builds on an assumption that you can get 5 by 
> ORing ACTIVE_CFG with 0.
> 
> +#define CTL_DPU_5_MASK (CTL_DPU_0_MASK | \
> +            BIT(DPU_CTL_ACTIVE_CFG))
> +
> 
> This is again moving towards that problematic pattern.
> 
> Why dont we stick to CTL features individually spelling it then work 
> towards generalizing as we discussed.

Because adding a feature would become a nightmare of touching all the 
platforms?

We discussed not merging on major+LM. Glad, I agreed there. But I don't 
think that we should remove existing defines without good reason. We 
know that they work in the majority of cases.

>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 2 +-
>>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> index 6840b22a4159..83f8f83e2b29 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
>>>> @@ -975,7 +975,7 @@ static const struct dpu_ctl_cfg sm8450_ctl[] = {
>>>>       {
>>>>       .name = "ctl_0", .id = CTL_0,
>>>>       .base = 0x15000, .len = 0x204,
>>>> -    .features = BIT(DPU_CTL_ACTIVE_CFG) | 
>>>> BIT(DPU_CTL_SPLIT_DISPLAY) | BIT(DPU_CTL_FETCH_ACTIVE),
>>>> +    .features = BIT(DPU_CTL_SPLIT_DISPLAY) | CTL_SC7280_MASK,
>>>>       .intr_start = DPU_IRQ_IDX(MDP_SSPP_TOP0_INTR2, 9),
>>>>       },
>>>>       {
>>

-- 
With best wishes
Dmitry