Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks

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

On 24/06/2023 04:23, Jessica Zhang wrote:
> 
> 
> On 6/23/2023 5:09 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>> Currently, the device core dump mechanism does not dump registers of 
>>>> sub
>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>> function to dump hardware blocks that contain sub blocks.
>>>>
>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>> +++++++++++++++++++++++++++-----
>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>>> *kms)
>>>>       return 0;
>>>>   }
>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>> *disp_state,
>>>> +                       void __iomem *mmio, void *blk,
>>>> +                       enum dpu_hw_blk_type blk_type)
>>>
>>> No. Such multiplexers add no value to the code. Please inline it.
>>>
>>> Not to mention that this patch is hard to review. You both move 
>>> existing code and add new features. If it were to go, it should have 
>>> been split into two patches: one introducing the multiplexer and 
>>> another one adding subblocks.
>>>
>>
>> Ok. we can split this into:
>>
>> 1) adding the multiplexer
>> 2) adding sub-blk parsing support inside the multiplexer
>>
>>>> +{
>>>> +    u32 base;
>>>> +
>>>> +    switch (blk_type) {
>>>> +    case DPU_HW_BLK_TOP:
>>>> +    {
>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>> +
>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>> +                            mmio + top->base, "top");
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>> MDP_PERIPH_TOP0_END,
>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>> +                            "top_2");
>>>> +        } else {
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio 
>>>> + top->base, "top");
>>>> +        }
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_LM:
>>>> +    {
>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>>> mixer->base, "%s",
>>>> +                        mixer->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_CTL:
>>>> +    {
>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>> ctl->base, "%s",
>>>> +                        ctl->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_INTF:
>>>> +    {
>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>>> intf->base, "%s",
>>>> +                        intf->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_WB:
>>>> +    {
>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>> wb->base, "%s",
>>>> +                        wb->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_SSPP:
>>>> +    {
>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>> +
>>>> +        base = sspp_block->base;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>> mmio + base, "%s",
>>>> +                        sspp_block->name);
>>>> +
>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>> sblk->scaler_blk.len,
>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>> "%s_%s",
>>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>>
>>> Actually, it would be better to:
>>> - drop name from all sblk instances (and use known string instead of 
>>> the sblk name here)
> 
> Hey Dmitry,
> 
> FWIW, I second Abhinav's points about the sblk names. For example, if in 
> the future we want to add a "_rot" suffix specifically to the 
> VIG_SBLK_ROT.scaler name, it would be easier to just make that change in 
> the HW catalog.

But why? The scaler is the same qseed3 scaler. We do not dump features, 
they are constant for the platform in question.

> 
>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>  From my understanding, your suggestion is to replace the feature flag 
> checks with a sblk.len > 0 check.
> 
> I don't think that would be good because it wouldn't be correct to 
> assume that the sblk will always be present. For example, for 
> DPU_HW_BLK_DSC, the sblks will only be present for DSC_BLK_1_2.

I don't consider sub-block as being always present. But if it present, 
it has non-zero length. If its length is zero, we have nothing to dump 
for it.

> In addition, it is possible for sblks, like pp_sblk_te.te2, to have a 
> len of 0. While the register space of that specific sblk will not be 
> printed, I'd prefer the devcore dump to reflect what is present within 
> the HW catalog so that the user knows which pingpong blks have the TE2 
> sblk.

I'd consider this as dumping the feature instead of dumping the 
registers. If you think it is necessary to ease decoding of the dump, 
consider adding block.features to the dump instead.

> 
> Thanks,
> 
> Jessica Zhang
> 
>>>
>>
>> No, I dont agree. If we drop the names from the sub_blk in the 
>> catalog, we will end up using "sub_blk_name" string here in the code 
>> to indicate which blk that is in the dump.
>>
>> If we add more sub_blks in the catalog in the future we need to keep 
>> changing the code over here. Thats not how it should be.
>>
>> Leaving the names in the catalog ensures that this code wont change 
>> and only catalog changes when we add a new sub_blk either for an 
>> existing or new chipset.
>>
>> catalog is indicating the new blk, and dumping code just prints it.
>>
>> with your approach, dumping code will or can keep changing with 
>> chipsets or sub_blks. Thats not how it should be.
>>

-- 
With best wishes
Dmitry