Message-ID: <37c4bde0-0798-7506-ffd3-c8689ab78ba0@quicinc.com>
Date:   Sat, 24 Jun 2023 07:17:51 -0700
From:   Abhinav Kumar <quic_abhinavk@...cinc.com>
To:     Dmitry Baryshkov <dmitry.baryshkov@...aro.org>,
        Ryan McCann <quic_rmccann@...cinc.com>,
        Rob Clark <robdclark@...il.com>, Sean Paul <sean@...rly.run>,
        Marijn Suijten <marijn.suijten@...ainline.org>,
        "David Airlie" <airlied@...il.com>, Daniel Vetter <daniel@...ll.ch>
CC:     Rob Clark <robdclark@...omium.org>,
        <linux-arm-msm@...r.kernel.org>, <dri-devel@...ts.freedesktop.org>,
        <freedreno@...ts.freedesktop.org>, <linux-kernel@...r.kernel.org>,
        <quic_jesszhan@...cinc.com>
Subject: Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers
 of sub blocks



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@...cinc.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.

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. So this belongs in the catalog.

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.

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

Powered by blists - more mailing lists