[Freedreno] [PATCH v5 08/10] drm/msm/dpu: add support for MDP_TOP blackhole

[Dmitry Baryshkov]

On 25/11/2022 08:01, Abhinav Kumar wrote:
> 
> 
> On 11/23/2022 1:04 PM, 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>
>> Reviewed-by: Konrad Dybcio <konrad.dybcio at linaro.org>
>> Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov at linaro.org>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h |  3 +++
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c        | 11 +++++++++--
>>   2 files changed, 12 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..8da4c5ba6dc3 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.h
>> @@ -82,6 +82,8 @@ enum {
>>    * @DPU_MDP_UBWC_1_0,      This chipsets supports Universal Bandwidth
>>    *                         compression initial revision
>>    * @DPU_MDP_UBWC_1_5,      Universal Bandwidth compression version 1.5
>> + * @DPU_MDP_PERIPH_0_REMOVED Indicates that access to periph top0 
>> block results
>> + *               in a failure
> shouldnt this be that "indicates that the top register block is not 
> contiguous and the two sub-blocks are separated by an offset"

Not so sure. Your suggestion is closer to the dynamic case, where all 
the sizes are dynamic in catalog. Since the patch uses fixed offsets, 
I'd mention periph0 instead (like the downstream does).

>>    * @DPU_MDP_MAX            Maximum value
>>    */
>> @@ -92,6 +94,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..4ac14de55139 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 (cat->mdp[0].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");
> 
> I recall one of the comments from konrad that this should come from the 
> catalog rather than a hard-coded offset which you wanted to keep it for 
> a later time. I am fine with that.
> 
> But instead of a hard-coded offset, do you want to have a macro so that 
> atleast we know what the value means and can fix it in the future? 
> Otherwise it would end up being one of those numbers which someone later 
> on wouldnt understand where it comes from and what it means.

Yes, I macro makes sense to me. I'll fix in v6.

> 
>> +    } 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