[PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks
Dmitry Baryshkov
dmitry.baryshkov at linaro.org
Fri Jun 30 00:10:15 UTC 2023
On 30/06/2023 02:29, Abhinav Kumar wrote:
>
>
> On 6/24/2023 7:44 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/24/2023 8:03 AM, Dmitry Baryshkov wrote:
>>> 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.
>>>
>>
>> Need to explore this. But not immediately.
>>
>>>>
>>>> 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.
>>>
>>
>> No, there is a difference and not duplicated. One is the name of the
>> struct so it can really be anything and doesnt need to match the hw
>> doc name. But the other is the string name which we can give exactly
>> to match software interface doc and makes parsing such a dump much
>> much easier.
>>
>> One point I dont see you have considered is the block index of the
>> sub_blk.
>>
>> Today, yes I see only a "pcc" or a "dither" etc
>>
>> What if there are two PCCs or two dithers.
>>
>> Then their names can just be "pcc_0" and "pcc_1" or "dither_0" and
>> "dither_1".
>>
>> Having name gives us the ability to easily incorporate even
>> unsequential indices.
>>
>> For example, every sspp's name today is not sequential. it can be
>> "sspp_3" then "sspp_8" etc
>>
>> By having names reflect the correct indices, dumping code becomes less
>> complex as the catalog will still have the right names as dumping code
>> will just use that.
>>
>
> The QC team is in agreement that we would like to go ahead with the
> names from the catalog and not drop them.
>
> Hence we will post the next revision with the name still from the
> catalog and drop the multiplexer completely.
Ack, let's see how it goes.
>
> Since the intern has a short period of time to finish development on
> this task, we would like to go ahead with this approach and post the
> next rev.
This is a bad argument.
--
With best wishes
Dmitry
More information about the dri-devel
mailing list