Messages in this thread | | | Date | Sat, 24 Jun 2023 15:18:39 +0300 | Subject | Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks | From | Dmitry Baryshkov <> |
| |
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) > +{ > + u32 base; > + > + switch (blk_type) { > + case DPU_HW_BLK_TOP: > + { > + struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
Block configuration is constant, so these should be constant pointers. Not to mention that such type conversions are usually considered to be a bad idea. We loose type information, so compiler can not catch any errors here.
> + > + 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;
[skipped]
default: > + DPU_ERROR("Block type not supported.");
Which block type?
> + } > +} > + > static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms) > { > int i; > @@ -899,53 +1047,47 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k > > /* dump CTL sub-blocks HW regs info */ > for (i = 0; i < cat->ctl_count; i++) > - msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, > - dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i); > + dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->ctl[i],
Here we are loosing the const property, which is a bad idea.
> + DPU_HW_BLK_CTL);
-- With best wishes Dmitry
| |