Re: [PATCH 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub blocks
From: Dmitry Baryshkov
Date: Thu Jun 29 2023 - 20:10:25 EST
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@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
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