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 17:17, Abhinav Kumar wrote:
> 
> 
> On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
>> On 24/06/2023 03:09, 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
>>
>> I'd say, drop the multiplexer completely. It adds no value here. It is 
>> only used from dpu_kms_mdp_snapshot(). If the code there was complex 
>> enough, it would have made sense to _split_ the function. But even in 
>> such case there would be no point in having multiplexer. We do not 
>> enumerate block by type.
>>
> 
> Can you pls elaborate what you mean by enumerate blk by type?
> 
> We do have DPU_HW_BLK_***
> 
> Did you mean sub-blk?
> 
>>>
>>>>> +{
>>>>> +    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)
>>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>>
>>>
>>> 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.
>>
>> Well, we do not enumerate sub-blocks in any way, they are not indexed. 
>> So even with sblk->blk.name in place, adding new sub-block would 
>> require adding new code here. That's why I wrote that the calling code 
>> knows which sub-block it refers to.
>>
> 
> Today, unfortunately each sub_blk type is different so we have to do 
> this case by case.
> 
> Ideally, this should have just been
> 
> -> print main blk
> -> print all sub-blks of the main blk
> 
> Without having to handle each main blk's sub-blks separately.
> 
> That way the dumping code would have remained generic without having to 
> do even the multiplexer in the first place.
> 
> Need to explore if somehow we can come up with a generic sub-blk struct 
> and make this possible. Then this code will become much easier and what 
> I am saying will make total sense.

In such case, yes. However I'd warn about having a generic array of 
subblocks. Having named subblock entries might complicate snapshotting, 
but it makes the rest of the DPU driver smaller.

> 
> Even without that, conceptually these sub-blk names are reflecting whats 
> in our software document. So its not a random name but reflects the 
> actual sub-blk name from the hardware.

Yes

> So this belongs in the catalog.

But the sub-block field already has a correct name: scaler_blk, csc_blk, 
etc. Having both sub-block field name and the .name inside results in 
kind of duplication, which seems unnecessary to me.

> Dumping code should not change or know whats the name of each block. It 
> should just use whats in the catalog. thats why even conceptually I am 
> not okay with your idea.

Dumping code itself (msm_disp_snapshot_*) doesn't. But the caller code 
knows what is the subblock.

Let me pick a definition from the patch:

static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
	.pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
		.len = 0x90, .version = 0x10007},
};

the "pcc" is repeated three times. When the code looks at this block, it 
already knows that it is a PCC block.

Compare this with:

static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
	.pcc = {
		.base = 0x1700,
		.len = 0x90,
		.version = 0x10007,
	},
};

Nothing is repeated, but we still know that this is the DSPPn_PCC 
sub-block description.

Calling code does:

u32 base;
base = ctx->cap->sblk->pcc.base;


> 
>> Let me extract the relevant code (skipping all the conditions for now):
>>
>> msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, 
>> "%s",
>>                  sspp_block->name);
>>
>> if (have_scaler)
>>      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);
>>
>> if (have_csc)
>>      msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>                      mmio + base + sblk->csc_blk.base, "%s_%s",
>>                      sspp_block->name, sblk->csc_blk.name);
>>
>> Consider adding new sub-block, "baz". We would still require manual 
>> addition of the following code:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "%s_%s",
>>                      sspp_block->name, sblk->baz_blk.name);
>>
>>
>> Compare this with:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "%s_baz",
>>                      sspp_block->name);
>>
> 
> Basically you are saying why not make the one line change here instead 
> of using the name from the catalog.
> 
> I think it will be better to use from the catalog for the reason I wrote 
> above that dumping code should just "use" the catalog's information and 
> not become a catalog itself.
> 
> You are not saving much by dropping the sub-blk name from catalog anyway.
> 
>> Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which 
>> doesn't use name), it should be:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "sspp%d_baz", 
>> idx);
>>
>>
> tbh, after looking at this series, it made me think why I didnt use the 
> name from the catalog even for the dpu_kms_mdp_snapshot()
>>
>>>
> 
> <snipped>

-- 
With best wishes
Dmitry