Messages in this thread | | | Date | Fri, 30 Jun 2023 03:10:15 +0300 | Subject | Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks | From | Dmitry Baryshkov <> |
| |
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@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
| |