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