[PATCH v4 01/42] drm/msm/dpu: use CTL_SC7280_MASK for sm8450's ctl_0
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Wed Apr 5 00:43:36 UTC 2023
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
More information about the dri-devel
mailing list