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

[Dmitry Baryshkov <dmitry.baryshkov@linaro.org>]

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@kernel.org>
>> Reviewed-by: Vinod Koul <vkoul@kernel.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@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.

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.

> 
> 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);
>>   }

-- 
With best wishes
Dmitry