[PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Sat Jun 24 15:03:32 UTC 2023
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 at 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
More information about the dri-devel
mailing list