Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks
From: Dmitry Baryshkov
Date: Sat Jun 24 2023 - 08:14:15 EST
On 24/06/2023 04:23, Jessica Zhang wrote:
On 6/23/2023 5:09 PM, 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@xxxxxxxxxxx>
---
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
+{
+ 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)
Hey Dmitry,
FWIW, I second Abhinav's points about the sblk names. For example, if in
the future we want to add a "_rot" suffix specifically to the
VIG_SBLK_ROT.scaler name, it would be easier to just make that change in
the HW catalog.
But why? The scaler is the same qseed3 scaler. We do not dump features,
they are constant for the platform in question.
- Use sblk->foo_blk.len to check if it should be printed or not.
From my understanding, your suggestion is to replace the feature flag
checks with a sblk.len > 0 check.
I don't think that would be good because it wouldn't be correct to
assume that the sblk will always be present. For example, for
DPU_HW_BLK_DSC, the sblks will only be present for DSC_BLK_1_2.
I don't consider sub-block as being always present. But if it present,
it has non-zero length. If its length is zero, we have nothing to dump
for it.
In addition, it is possible for sblks, like pp_sblk_te.te2, to have a
len of 0. While the register space of that specific sblk will not be
printed, I'd prefer the devcore dump to reflect what is present within
the HW catalog so that the user knows which pingpong blks have the TE2
sblk.
I'd consider this as dumping the feature instead of dumping the
registers. If you think it is necessary to ease decoding of the dump,
consider adding block.features to the dump instead.
Thanks,
Jessica Zhang
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.
--
With best wishes
Dmitry