[PATCH v3 6/8] drm/msm/dpu: add support for MDP_TOP blackhole

[Abhinav Kumar]

On 11/16/2022 1:18 AM, Dmitry Baryshkov wrote:
> On 16/11/2022 11:30, Abhinav Kumar wrote:
>>
>>
>> On 11/16/2022 12:19 AM, Dmitry Baryshkov wrote:
>>> On 16/11/2022 10:50, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 11/4/2022 6:03 AM, Dmitry Baryshkov wrote:
>>>>> On sm8450 a register block was removed from MDP TOP. Accessing it 
>>>>> during
>>>>> snapshotting results in NoC errors / immediate reboot. Skip accessing
>>>>> these registers during snapshot.
>>>>>
>>>>> Tested-by: Vinod Koul <vkoul at kernel.org>
>>>>> Reviewed-by: Vinod Koul <vkoul at kernel.org>
>>>>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>>>>
>>>> I am confused with both the ordering and the split of this patch.
>>>>
>>>> You have defined DPU_MDP_PERIPH_0_REMOVED in the catalog header file 
>>>> in this patch but used it in the next.
>>>>
>>>> But you also have code in this patch which relies on setting of this 
>>>> bit.
>>>>
>>>> So if this patch is taken without the next, it will still crash.
>>>
>>> It will not crash if this patch is taken without the next one. 
>>> Without the next patch the DPU driver will not match and bind against 
>>> the qcom,sm8450-dpu device.
>>
>> Ah okay, I just now saw that you have the compatible change also in 
>> the next patch.
>>
>>>
>>> So, the ordering is quite logical from my point of view:
>>> - add support for all the features required for the device
>>> - add the device compat string & catalog entry
>>>
>>>>
>>>> Rather, you should combine the define part of this patch to the next 
>>>> patch in the series 
>>>> https://patchwork.freedesktop.org/patch/510114/?series=108883&rev=3 
>>>> , then move that one in front of this patch.
>>>
>>> No. This way we'll have a state (after adding the next patch) when 
>>> the sm8450 support is enabled, but the top-hole is not handled, 
>>> leading to a crash.
>>>
>>
>> What if you split the compatible to a separate patch like what SM8350 
>> did.
>>
>> https://patchwork.freedesktop.org/patch/511659/?series=110924&rev=1
>>
>> So, we have hw catalog changes ---> snapshot fix ---> add the compatible.
> 
> I don't see any good reason to do this. Adding a define without backing 
> implementation is a bad idea in my opinion.
> 

The define is used in two places today. First in the catalog and second 
in the snapshot (which is your change).

Even with the split i am suggesting the define and usage will be together.

In fact, in my opinion thats more coherent because you defined the 
macro, used it to show that sm8450 has this TOP_HOLE.

Then, you are using the hw->caps which will be set in the previous patch 
to avoid that region in the snapshot.

The good reason to do it this way is that, with this current ordering of 
patch, this patch is essentially a dummy patch because technically no 
chipset has set this capability.

But if you follow the order i am suggesting, it actually has more 
meaning because we know sm8450 has set it in its caps before you use it.

> Regarding splitting the hw_catalog and compat. I have always considered 
> the hw catalog entry as of_device_id.data. In other words, a devices' 
> match data, which makes a little sense without compat entry.
> 
> With the current approach each patch is atomic, it changes single point 
> or adds a single feature, etc.
> 
>>
>> That will make both of us happy?
>>
>>>>
>>>> So that its much more coherent that you defined 
>>>> DPU_MDP_PERIPH_0_REMOVED both in the catalog header and used it in 
>>>> the catalog.c file and the in the next change you used the caps to 
>>>> avoid touching that register.
>>>
>>> I'd say it's rather strange way. When I see a define/feature 
>>> addition, I'd prefer to seethe implementation too.
>>>
>>>> Regarding the TOP hole itself, I need one day to investigate this. I 
>>>> am waiting for permissions to the documentation.
>>>>
>>>> If i cannot get access by the time you have re-ordered this, I will 
>>>> ack this once the reorder is done within a day.
>>>
>>>
>>> For the reference: [1]
>>>
>>> [1] 
>>> https://git.codelinaro.org/clo/la/platform/vendor/opensource/display-drivers/-/commit/f9ff8af5b640147f3651c23551c60f81f62874b1 
>>>
>>>
>>>>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  1 +
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 11 +++++++++--
>>>>>   2 files changed, 10 insertions(+), 2 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>> index 38aa38ab1568..4730f8268f2a 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>>>>> @@ -92,6 +92,7 @@ enum {
>>>>>       DPU_MDP_UBWC_1_0,
>>>>>       DPU_MDP_UBWC_1_5,
>>>>>       DPU_MDP_AUDIO_SELECT,
>>>>> +    DPU_MDP_PERIPH_0_REMOVED,
>>>>>       DPU_MDP_MAX
>>>>>   };
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> index f3660cd14f4f..95d8765c1c53 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> @@ -927,8 +927,15 @@ static void dpu_kms_mdp_snapshot(struct 
>>>>> msm_disp_state *disp_state, struct msm_k
>>>>>           msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>>>>                   dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>>>>> -    msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
>>>>> -            dpu_kms->mmio + cat->mdp[0].base, "top");
>>>>> +    if (dpu_kms->hw_mdp->caps->features & 
>>>>> BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>> +        msm_disp_snapshot_add_block(disp_state, 0x380,
>>>>> +                dpu_kms->mmio + cat->mdp[0].base, "top");
>>>>> +        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - 
>>>>> 0x3a8,
>>>>> +                dpu_kms->mmio + cat->mdp[0].base + 0x3a8, "top_2");
>>>>> +    } else {
>>>>> +        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
>>>>> +                dpu_kms->mmio + cat->mdp[0].base, "top");
>>>>> +    }
>>>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>>>   }
>>>
>