[PATCH v4 0/6] Add support to print sub-block registers in dpu hw catalog

[PATCH v4 0/6] Add support to print sub-block registers in dpu hw catalog

[Ryan McCann]

The purpose of this patch series is to add support to print the registers
of sub-blocks in the DPU hardware catalog and fix the order in which all
hardware blocks are dumped for a device core dump. This involves:

1. Changing data structure from stack to queue to fix the printing order
of the device core dump.

2. Removing redundant suffix of sub-block names.

3. Removing redundant prefix of sub-block names.

4. Eliminating unused variable from relevant macros.

5. Defining names for sub-blocks that have not yet been defined.

6. Implementing wrapper function that prints the registers of sub-blocks
when there is a need.

Sample Output of the sspp_0 block and its sub-blocks for devcore dump:
======sspp_0======
...registers
...
====sspp_0_scaler====
...
...
====sspp_0_csc====
...
...
====next_block====
...

---
Changes in v4:
- Added review tags
- Link to v3: https://lore.kernel.org/r/20230622-devcoredump_patch-v3-0-83601b72eb67@quicinc.com

Changes in v3:
- Split sub-block changes and main block changes into two commits
- Corrected typo in comment located in DSC for loop block
- Eliminated variables mmio and base
- Dropped unnecessary "%s"
- Link to v2: https://lore.kernel.org/r/20230622-devcoredump_patch-v2-0-9e90a87d393f@quicinc.com

Changes in v2:
- Changed spelling "sub block" to "sub-block" or "sblk".
- Capitalized DPU.
- Eliminated multiplexer/wrapper function. Inlined instead.
- Changed if statements from feature checks to length checks.
- Squashed prefix and suffix patch into one.
- Link to v1: https://lore.kernel.org/r/20230622-devcoredump_patch-v1-0-3b2cdcc6a576@quicinc.com

---
Ryan McCann (6):
      drm/msm: Update dev core dump to not print backwards
      drm/msm/dpu: Drop unused num argument from relevant macros
      drm/msm/dpu: Define names for unnamed sblks
      drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks
      drm/msm/dpu: Refactor printing of main blocks in device core dump
      drm/msm/dpu: Update dev core dump to dump registers of sub-blocks

 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c    | 90 +++++++++++-----------
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c           | 94 ++++++++++++++++++-----
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |  2 +-
 3 files changed, 120 insertions(+), 66 deletions(-)
---
base-commit: a0364260213c96f6817f7e85cdce293cb743460f
change-id: 20230622-devcoredump_patch-df7e8f6fd632

Best regards,
-- 
Ryan McCann <quic_rmccann@quicinc.com>

[PATCH v4 1/6] drm/msm: Update dev core dump to not print backwards

[Ryan McCann]

Device core dump add block method adds hardware blocks to dumping queue
with stack behavior which causes the hardware blocks to be printed in
reverse order. Change the addition to dumping queue data structure
from "list_add" to "list_add_tail" for FIFO queue behavior.

Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
index acfe1b31e079..add72bbc28b1 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -192,5 +192,5 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
 	new_blk->base_addr = base_addr;
 
 	msm_disp_state_dump_regs(&new_blk->state, new_blk->size, base_addr);
-	list_add(&new_blk->node, &disp_state->blocks);
+	list_add_tail(&new_blk->node, &disp_state->blocks);
 }

-- 
2.25.1

[PATCH v4 2/6] drm/msm/dpu: Drop unused num argument from relevant macros

[Ryan McCann]

Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block
macros. Update calls to relevant macros to reflect change.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 0de507d4d7b7..9f9d5ac3992f 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -288,7 +288,7 @@ static const uint32_t wb2_formats[] = {
 	.rotation_cfg = rot_cfg, \
 	}
 
-#define _DMA_SBLK(num, sdma_pri) \
+#define _DMA_SBLK(sdma_pri) \
 	{ \
 	.maxdwnscale = SSPP_UNITY_SCALE, \
 	.maxupscale = SSPP_UNITY_SCALE, \
@@ -323,10 +323,10 @@ static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
 static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
 				_VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED3);
 
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK("8", 1);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK("9", 2);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK("10", 3);
-static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK("11", 4);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_2 = _DMA_SBLK(3);
+static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK(4);
 
 #define SSPP_BLK(_name, _id, _base, _len, _features, \
 		_sblk, _xinid, _type, _clkctrl) \
@@ -366,10 +366,10 @@ static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
 				_VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
 				_VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
-static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK("12", 5);
-static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);
+static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK(5);
+static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK(6);
 
-#define _VIG_SBLK_NOSCALE(num, sdma_pri) \
+#define _VIG_SBLK_NOSCALE(sdma_pri) \
 	{ \
 	.maxdwnscale = SSPP_UNITY_SCALE, \
 	.maxupscale = SSPP_UNITY_SCALE, \
@@ -380,8 +380,8 @@ static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK("13", 6);
 	.virt_num_formats = ARRAY_SIZE(plane_formats), \
 	}
 
-static const struct dpu_sspp_sub_blks qcm2290_vig_sblk_0 = _VIG_SBLK_NOSCALE("0", 2);
-static const struct dpu_sspp_sub_blks qcm2290_dma_sblk_0 = _DMA_SBLK("8", 1);
+static const struct dpu_sspp_sub_blks qcm2290_vig_sblk_0 = _VIG_SBLK_NOSCALE(2);
+static const struct dpu_sspp_sub_blks qcm2290_dma_sblk_0 = _DMA_SBLK(1);
 
 /*************************************************************
  * MIXER sub blocks config

-- 
2.25.1

[PATCH v4 3/6] drm/msm/dpu: Define names for unnamed sblks

[Ryan McCann]

Some sub-blocks in the hw catalog have not been given a name, so when the
registers from that block are dumped, there is no name to reference.
Define names for relevant sub-blocks to fix this.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 20 ++++++++++----------
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 9f9d5ac3992f..79e495dbc11d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -444,12 +444,12 @@ static const struct dpu_lm_sub_blks qcm2290_lm_sblk = {
  * DSPP sub blocks config
  *************************************************************/
 static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
-	.pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+	.pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
 		.len = 0x90, .version = 0x10007},
 };
 
 static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
-	.pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+	.pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
 		.len = 0x90, .version = 0x40000},
 };
 
@@ -465,19 +465,19 @@ static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
  * PINGPONG sub blocks config
  *************************************************************/
 static const struct dpu_pingpong_sub_blks sdm845_pp_sblk_te = {
-	.te2 = {.id = DPU_PINGPONG_TE2, .base = 0x2000, .len = 0x0,
+	.te2 = {.name = "te2", .id = DPU_PINGPONG_TE2, .base = 0x2000, .len = 0x0,
 		.version = 0x1},
-	.dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+	.dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
 		.len = 0x20, .version = 0x10000},
 };
 
 static const struct dpu_pingpong_sub_blks sdm845_pp_sblk = {
-	.dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+	.dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
 		.len = 0x20, .version = 0x10000},
 };
 
 static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
-	.dither = {.id = DPU_PINGPONG_DITHER, .base = 0xe0,
+	.dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0xe0,
 	.len = 0x20, .version = 0x20000},
 };
 
@@ -517,13 +517,13 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
  * DSC sub blocks config
  *************************************************************/
 static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
-	.enc = {.base = 0x100, .len = 0x100},
-	.ctl = {.base = 0xF00, .len = 0x10},
+	.enc = {.name = "enc",  .base = 0x100, .len = 0x100},
+	.ctl = {.name = "ctl",	.base = 0xF00, .len = 0x10},
 };
 
 static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
-	.enc = {.base = 0x200, .len = 0x100},
-	.ctl = {.base = 0xF80, .len = 0x10},
+	.enc = {.name = "enc",	.base = 0x200, .len = 0x100},
+	.ctl = {.name = "ctl",	.base = 0xF80, .len = 0x10},
 };
 
 #define DSC_BLK(_name, _id, _base, _features) \

-- 
2.25.1

[PATCH v4 4/6] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks

[Ryan McCann]

For a device core dump, the registers of sub-blocks are printed under a
title formatted as <mainBlkName_sblkName>. For example, the csc sub-block
for an SSPP main block "sspp_0" would be printed "sspp_0_sspp_csc0". The
title is clearly redundant due to the duplicate "sspp" and "0" that exist
in both the mainBlkName and sblkName. To eliminate this redundancy, remove
the secondary "sspp" and "0" that exist in the sub-block name by
elimanting the "sspp_" prefix and the concatenation of "num" that results
in the redundant "0" suffix. Remove num parameter altogether from relevant
macros as a consequence of it no longer being used.

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>
Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>
Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 50 +++++++++++++-------------
 1 file changed, 25 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 79e495dbc11d..836efa074a35 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -252,15 +252,15 @@ static const uint32_t wb2_formats[] = {
  *************************************************************/
 
 /* SSPP common configuration */
-#define _VIG_SBLK(num, sdma_pri, qseed_ver) \
+#define _VIG_SBLK(sdma_pri, qseed_ver) \
 	{ \
 	.maxdwnscale = MAX_DOWNSCALE_RATIO, \
 	.maxupscale = MAX_UPSCALE_RATIO, \
 	.smart_dma_priority = sdma_pri, \
-	.scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+	.scaler_blk = {.name = "scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = STRCAT("sspp_csc", num), \
+	.csc_blk = {.name = "csc", \
 		.id = DPU_SSPP_CSC_10BIT, \
 		.base = 0x1a00, .len = 0x100,}, \
 	.format_list = plane_formats_yuv, \
@@ -270,15 +270,15 @@ static const uint32_t wb2_formats[] = {
 	.rotation_cfg = NULL, \
 	}
 
-#define _VIG_SBLK_ROT(num, sdma_pri, qseed_ver, rot_cfg) \
+#define _VIG_SBLK_ROT(sdma_pri, qseed_ver, rot_cfg) \
 	{ \
 	.maxdwnscale = MAX_DOWNSCALE_RATIO, \
 	.maxupscale = MAX_UPSCALE_RATIO, \
 	.smart_dma_priority = sdma_pri, \
-	.scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+	.scaler_blk = {.name = "scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = STRCAT("sspp_csc", num), \
+	.csc_blk = {.name = "csc", \
 		.id = DPU_SSPP_CSC_10BIT, \
 		.base = 0x1a00, .len = 0x100,}, \
 	.format_list = plane_formats_yuv, \
@@ -300,13 +300,13 @@ static const uint32_t wb2_formats[] = {
 	}
 
 static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
-				_VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
-				_VIG_SBLK("1", 0, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
-				_VIG_SBLK("2", 0, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
-				_VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
 
 static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
 	.rot_maxheight = 1088,
@@ -315,13 +315,13 @@ static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
 };
 
 static const struct dpu_sspp_sub_blks sdm845_vig_sblk_0 =
-				_VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(5, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks sdm845_vig_sblk_1 =
-				_VIG_SBLK("1", 6, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(6, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
-				_VIG_SBLK("2", 7, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(7, DPU_SSPP_SCALER_QSEED3);
 static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
-				_VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED3);
+				_VIG_SBLK(8, DPU_SSPP_SCALER_QSEED3);
 
 static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
 static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
@@ -341,31 +341,31 @@ static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK(4);
 	}
 
 static const struct dpu_sspp_sub_blks sc7180_vig_sblk_0 =
-				_VIG_SBLK("0", 4, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(4, DPU_SSPP_SCALER_QSEED4);
 
 static const struct dpu_sspp_sub_blks sc7280_vig_sblk_0 =
-			_VIG_SBLK_ROT("0", 4, DPU_SSPP_SCALER_QSEED4, &dpu_rot_sc7280_cfg_v2);
+			_VIG_SBLK_ROT(4, DPU_SSPP_SCALER_QSEED4, &dpu_rot_sc7280_cfg_v2);
 
 static const struct dpu_sspp_sub_blks sm6115_vig_sblk_0 =
-				_VIG_SBLK("0", 2, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(2, DPU_SSPP_SCALER_QSEED4);
 
 static const struct dpu_sspp_sub_blks sm8250_vig_sblk_0 =
-				_VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(5, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8250_vig_sblk_1 =
-				_VIG_SBLK("1", 6, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(6, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8250_vig_sblk_2 =
-				_VIG_SBLK("2", 7, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(7, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8250_vig_sblk_3 =
-				_VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(8, DPU_SSPP_SCALER_QSEED4);
 
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 =
-				_VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(7, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
-				_VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(8, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
-				_VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(9, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
-				_VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
+				_VIG_SBLK(10, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK(5);
 static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK(6);
 

-- 
2.25.1

[PATCH v4 5/6] drm/msm/dpu: Refactor printing of main blocks in device core dump

[Ryan McCann]

Currently, the names of main blocks are hardcoded into the
msm_disp_snapshot_add_block function rather than using the name that
already exists in the catalog. Change this to take the name directly from
the catalog instead of hardcoding it.

Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 ++++++++++++++++----------------
 1 file changed, 16 insertions(+), 16 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..70dbb1204e6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -899,38 +899,38 @@ 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);
+		msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, dpu_kms->mmio +
+					    cat->ctl[i].base, cat->ctl[i].name);
 
 	/* dump DSPP sub-blocks HW regs info */
 	for (i = 0; i < cat->dspp_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
-				dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
+					    cat->dspp[i].base, cat->dspp[i].name);
 
 	/* dump INTF sub-blocks HW regs info */
 	for (i = 0; i < cat->intf_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
-				dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
+					    cat->intf[i].base, cat->intf[i].name);
 
 	/* dump PP sub-blocks HW regs info */
 	for (i = 0; i < cat->pingpong_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
-				dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
+					    cat->pingpong[i].base, cat->pingpong[i].name);
 
 	/* dump SSPP sub-blocks HW regs info */
 	for (i = 0; i < cat->sspp_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
-				dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
+					    cat->sspp[i].base, cat->sspp[i].name);
 
 	/* dump LM sub-blocks HW regs info */
 	for (i = 0; i < cat->mixer_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
-				dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
+					    cat->mixer[i].base, cat->mixer[i].name);
 
 	/* dump WB sub-blocks HW regs info */
 	for (i = 0; i < cat->wb_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
-				dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, dpu_kms->mmio +
+					    cat->wb[i].base, cat->wb[i].name);
 
 	if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
 		msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
@@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 
 	/* dump DSC sub-blocks HW regs info */
 	for (i = 0; i < cat->dsc_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
-				dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
+					    cat->dsc[i].base, cat->dsc[i].name);
 
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }

-- 
2.25.1

Re: [PATCH v4 5/6] drm/msm/dpu: Refactor printing of main blocks in device core dump

[Dmitry Baryshkov]

On 06/07/2023 23:48, Ryan McCann wrote:
> Currently, the names of main blocks are hardcoded into the
> msm_disp_snapshot_add_block function rather than using the name that
> already exists in the catalog. Change this to take the name directly from
> the catalog instead of hardcoding it.
> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 ++++++++++++++++----------------
>   1 file changed, 16 insertions(+), 16 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..70dbb1204e6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -899,38 +899,38 @@ 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);
> +		msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, dpu_kms->mmio +
> +					    cat->ctl[i].base, cat->ctl[i].name);

Splitting on the `+' sign is a bad idea. It makes it harder to read the 
code. Please keep the first line as is, it is perfectly fine on its own, 
and do just what you have stated in the commit message: change printed 
block name.

>   
>   	/* dump DSPP sub-blocks HW regs info */
>   	for (i = 0; i < cat->dspp_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
> -				dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
> +					    cat->dspp[i].base, cat->dspp[i].name);
>   
>   	/* dump INTF sub-blocks HW regs info */
>   	for (i = 0; i < cat->intf_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
> -				dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
> +					    cat->intf[i].base, cat->intf[i].name);
>   
>   	/* dump PP sub-blocks HW regs info */
>   	for (i = 0; i < cat->pingpong_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
> -				dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
> +					    cat->pingpong[i].base, cat->pingpong[i].name);
>   
>   	/* dump SSPP sub-blocks HW regs info */
>   	for (i = 0; i < cat->sspp_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
> -				dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
> +					    cat->sspp[i].base, cat->sspp[i].name);
>   
>   	/* dump LM sub-blocks HW regs info */
>   	for (i = 0; i < cat->mixer_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
> -				dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
> +					    cat->mixer[i].base, cat->mixer[i].name);
>   
>   	/* dump WB sub-blocks HW regs info */
>   	for (i = 0; i < cat->wb_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
> -				dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, dpu_kms->mmio +
> +					    cat->wb[i].base, cat->wb[i].name);
>   
>   	if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>   		msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   
>   	/* dump DSC sub-blocks HW regs info */
>   	for (i = 0; i < cat->dsc_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
> -				dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
> +					    cat->dsc[i].base, cat->dsc[i].name);
>   
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/6] drm/msm/dpu: Refactor printing of main blocks in device core dump

[Abhinav Kumar]

On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
> On 06/07/2023 23:48, Ryan McCann wrote:
>> Currently, the names of main blocks are hardcoded into the
>> msm_disp_snapshot_add_block function rather than using the name that
>> already exists in the catalog. Change this to take the name directly from
>> the catalog instead of hardcoding it.
>>
>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 
>> ++++++++++++++++----------------
>>   1 file changed, 16 insertions(+), 16 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..70dbb1204e6c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -899,38 +899,38 @@ 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);
>> +        msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, 
>> dpu_kms->mmio +
>> +                        cat->ctl[i].base, cat->ctl[i].name);
> 
> Splitting on the `+' sign is a bad idea. It makes it harder to read the 
> code. Please keep the first line as is, it is perfectly fine on its own, 
> and do just what you have stated in the commit message: change printed 
> block name.
> 

Actually, I asked Ryan to fix the indent here in the same patch as he 
was touching this code anyway.

So you would prefer thats left untouched?

>>       /* dump DSPP sub-blocks HW regs info */
>>       for (i = 0; i < cat->dspp_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>> -                dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>> dpu_kms->mmio +
>> +                        cat->dspp[i].base, cat->dspp[i].name);
>>       /* dump INTF sub-blocks HW regs info */
>>       for (i = 0; i < cat->intf_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>> -                dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>> dpu_kms->mmio +
>> +                        cat->intf[i].base, cat->intf[i].name);
>>       /* dump PP sub-blocks HW regs info */
>>       for (i = 0; i < cat->pingpong_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>> -                dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d", 
>> i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, 
>> dpu_kms->mmio +
>> +                        cat->pingpong[i].base, cat->pingpong[i].name);
>>       /* dump SSPP sub-blocks HW regs info */
>>       for (i = 0; i < cat->sspp_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>> -                dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>> dpu_kms->mmio +
>> +                        cat->sspp[i].base, cat->sspp[i].name);
>>       /* dump LM sub-blocks HW regs info */
>>       for (i = 0; i < cat->mixer_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>> -                dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, 
>> dpu_kms->mmio +
>> +                        cat->mixer[i].base, cat->mixer[i].name);
>>       /* dump WB sub-blocks HW regs info */
>>       for (i = 0; i < cat->wb_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>> -                dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, 
>> dpu_kms->mmio +
>> +                        cat->wb[i].base, cat->wb[i].name);
>>       if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>           msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct 
>> msm_disp_state *disp_state, struct msm_k
>>       /* dump DSC sub-blocks HW regs info */
>>       for (i = 0; i < cat->dsc_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>> -                dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>> dpu_kms->mmio +
>> +                        cat->dsc[i].base, cat->dsc[i].name);
>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>   }
>>
> 

Re: [PATCH v4 5/6] drm/msm/dpu: Refactor printing of main blocks in device core dump

[Dmitry Baryshkov]

On 07/07/2023 03:16, Abhinav Kumar wrote:
> 
> 
> On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
>> On 06/07/2023 23:48, Ryan McCann wrote:
>>> Currently, the names of main blocks are hardcoded into the
>>> msm_disp_snapshot_add_block function rather than using the name that
>>> already exists in the catalog. Change this to take the name directly 
>>> from
>>> the catalog instead of hardcoding it.
>>>
>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 
>>> ++++++++++++++++----------------
>>>   1 file changed, 16 insertions(+), 16 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..70dbb1204e6c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -899,38 +899,38 @@ 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);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->ctl[i].base, cat->ctl[i].name);
>>
>> Splitting on the `+' sign is a bad idea. It makes it harder to read 
>> the code. Please keep the first line as is, it is perfectly fine on 
>> its own, and do just what you have stated in the commit message: 
>> change printed block name.
>>
> 
> Actually, I asked Ryan to fix the indent here in the same patch as he 
> was touching this code anyway.
> 
> So you would prefer thats left untouched?

Yes. The current one was definitely better than splitting in the middle 
of a statement.

> 
>>>       /* dump DSPP sub-blocks HW regs info */
>>>       for (i = 0; i < cat->dspp_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>> -                dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->dspp[i].base, cat->dspp[i].name);
>>>       /* dump INTF sub-blocks HW regs info */
>>>       for (i = 0; i < cat->intf_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>> -                dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->intf[i].base, cat->intf[i].name);
>>>       /* dump PP sub-blocks HW regs info */
>>>       for (i = 0; i < cat->pingpong_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>>> -                dpu_kms->mmio + cat->pingpong[i].base, 
>>> "pingpong_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, 
>>> cat->pingpong[i].len, dpu_kms->mmio +
>>> +                        cat->pingpong[i].base, cat->pingpong[i].name);
>>>       /* dump SSPP sub-blocks HW regs info */
>>>       for (i = 0; i < cat->sspp_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>> -                dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->sspp[i].base, cat->sspp[i].name);
>>>       /* dump LM sub-blocks HW regs info */
>>>       for (i = 0; i < cat->mixer_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>> -                dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->mixer[i].base, cat->mixer[i].name);
>>>       /* dump WB sub-blocks HW regs info */
>>>       for (i = 0; i < cat->wb_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>> -                dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->wb[i].base, cat->wb[i].name);
>>>       if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>           msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct 
>>> msm_disp_state *disp_state, struct msm_k
>>>       /* dump DSC sub-blocks HW regs info */
>>>       for (i = 0; i < cat->dsc_count; i++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>> -                dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>>> dpu_kms->mmio +
>>> +                        cat->dsc[i].base, cat->dsc[i].name);
>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>   }
>>>
>>

-- 
With best wishes
Dmitry

Re: [PATCH v4 5/6] drm/msm/dpu: Refactor printing of main blocks in device core dump

[Abhinav Kumar]

On 7/6/2023 7:19 PM, Dmitry Baryshkov wrote:
> On 07/07/2023 03:16, Abhinav Kumar wrote:
>>
>>
>> On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
>>> On 06/07/2023 23:48, Ryan McCann wrote:
>>>> Currently, the names of main blocks are hardcoded into the
>>>> msm_disp_snapshot_add_block function rather than using the name that
>>>> already exists in the catalog. Change this to take the name directly 
>>>> from
>>>> the catalog instead of hardcoding it.
>>>>
>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 
>>>> ++++++++++++++++----------------
>>>>   1 file changed, 16 insertions(+), 16 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..70dbb1204e6c 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -899,38 +899,38 @@ 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);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->ctl[i].base, cat->ctl[i].name);
>>>
>>> Splitting on the `+' sign is a bad idea. It makes it harder to read 
>>> the code. Please keep the first line as is, it is perfectly fine on 
>>> its own, and do just what you have stated in the commit message: 
>>> change printed block name.
>>>
>>
>> Actually, I asked Ryan to fix the indent here in the same patch as he 
>> was touching this code anyway.
>>
>> So you would prefer thats left untouched?
> 
> Yes. The current one was definitely better than splitting in the middle 
> of a statement.
> 

Certainly Yes. Splitting across '+' was the last resort. For some 
reason, I thought any other option of splitting was breaking checkpatch 
for ryan so we had to do that. But, for this change it seems like even 
if we had done like below checkpatch didnt complain.

@@ -900,7 +900,7 @@ 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->mmio + 
cat->ctl[i].base, "ctl_%d", i);

But anyway, we can just take care of fixing indentation separately to 
avoid the hassle.

>>
>>>>       /* dump DSPP sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->dspp_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>>> -                dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->dspp[i].base, cat->dspp[i].name);
>>>>       /* dump INTF sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->intf_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>>> -                dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->intf[i].base, cat->intf[i].name);
>>>>       /* dump PP sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->pingpong_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>>>> -                dpu_kms->mmio + cat->pingpong[i].base, 
>>>> "pingpong_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, 
>>>> cat->pingpong[i].len, dpu_kms->mmio +
>>>> +                        cat->pingpong[i].base, cat->pingpong[i].name);
>>>>       /* dump SSPP sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->sspp_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>>> -                dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->sspp[i].base, cat->sspp[i].name);
>>>>       /* dump LM sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->mixer_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>>> -                dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->mixer[i].base, cat->mixer[i].name);
>>>>       /* dump WB sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->wb_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>>> -                dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->wb[i].base, cat->wb[i].name);
>>>>       if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>           msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct 
>>>> msm_disp_state *disp_state, struct msm_k
>>>>       /* dump DSC sub-blocks HW regs info */
>>>>       for (i = 0; i < cat->dsc_count; i++)
>>>> -        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>>> -                dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>>>> +        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>>>> dpu_kms->mmio +
>>>> +                        cat->dsc[i].base, cat->dsc[i].name);
>>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>>   }
>>>>
>>>
> 

[PATCH v4 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks

[Ryan McCann]

Currently, the device core dump mechanism does not dump registers of
sub-blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Edit
dpu_kms_mdp_snapshot function to account for sub-blocks.

Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 ++++++++++++++++++++++++++++++---
 1 file changed, 60 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index 70dbb1204e6c..afc45d597d65 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -903,25 +903,58 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 					    cat->ctl[i].base, cat->ctl[i].name);
 
 	/* dump DSPP sub-blocks HW regs info */
-	for (i = 0; i < cat->dspp_count; i++)
+	for (i = 0; i < cat->dspp_count; i++) {
 		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
 					    cat->dspp[i].base, cat->dspp[i].name);
 
+		if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
+			msm_disp_snapshot_add_block(disp_state, cat->dspp[i].sblk->pcc.len,
+						    dpu_kms->mmio + cat->dspp[i].base +
+						    cat->dspp[i].sblk->pcc.base, "%s_%s",
+						    cat->dspp[i].name,
+						    cat->dspp[i].sblk->pcc.name);
+	}
+
 	/* dump INTF sub-blocks HW regs info */
 	for (i = 0; i < cat->intf_count; i++)
 		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
 					    cat->intf[i].base, cat->intf[i].name);
 
 	/* dump PP sub-blocks HW regs info */
-	for (i = 0; i < cat->pingpong_count; i++)
+	for (i = 0; i < cat->pingpong_count; i++) {
 		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
 					    cat->pingpong[i].base, cat->pingpong[i].name);
 
+		/* TE2 block has length of 0, so will not print it */
+
+		if (cat->pingpong[i].sblk && cat->pingpong[i].sblk->dither.len > 0)
+			msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].sblk->dither.len,
+						    dpu_kms->mmio + cat->pingpong[i].base +
+						    cat->pingpong[i].sblk->dither.base, "%s_%s",
+						    cat->pingpong[i].name,
+						    cat->pingpong[i].sblk->dither.name);
+	}
+
 	/* dump SSPP sub-blocks HW regs info */
-	for (i = 0; i < cat->sspp_count; i++)
+	for (i = 0; i < cat->sspp_count; i++) {
 		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
 					    cat->sspp[i].base, cat->sspp[i].name);
 
+		if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
+			msm_disp_snapshot_add_block(disp_state, cat->sspp[i].sblk->scaler_blk.len,
+						    dpu_kms->mmio + cat->sspp[i].base +
+						    cat->sspp[i].sblk->scaler_blk.base, "%s_%s",
+						    cat->sspp[i].name,
+						    cat->sspp[i].sblk->scaler_blk.name);
+
+		if (cat->sspp[i].sblk && cat->sspp[i].sblk->csc_blk.len > 0)
+			msm_disp_snapshot_add_block(disp_state, cat->sspp[i].sblk->csc_blk.len,
+						    dpu_kms->mmio + cat->sspp[i].base +
+						    cat->sspp[i].sblk->csc_blk.base, "%s_%s",
+						    cat->sspp[i].name,
+						    cat->sspp[i].sblk->csc_blk.name);
+	}
+
 	/* dump LM sub-blocks HW regs info */
 	for (i = 0; i < cat->mixer_count; i++)
 		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
@@ -943,9 +976,30 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 	}
 
 	/* dump DSC sub-blocks HW regs info */
-	for (i = 0; i < cat->dsc_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
-					    cat->dsc[i].base, cat->dsc[i].name);
+	for (i = 0; i < cat->dsc_count; i++) {
+		if (cat->dsc[i].features & BIT(DPU_DSC_HW_REV_1_2)) {
+			struct dpu_dsc_blk enc = cat->dsc[i].sblk->enc;
+			struct dpu_dsc_blk ctl = cat->dsc[i].sblk->ctl;
+
+			/* For now, pass in a length of 0 because the DSC_BLK register space
+			 * overlaps with the sblks' register space.
+			 *
+			 * TODO: Pass in a length of 0 to DSC_BLK_1_2 in the HW catalog where
+			 * applicable.
+			 */
+			msm_disp_snapshot_add_block(disp_state, 0, dpu_kms->mmio +
+						    cat->dsc[i].base, cat->dsc[i].name);
+			msm_disp_snapshot_add_block(disp_state, enc.len, dpu_kms->mmio +
+						    cat->dsc[i].base + enc.base, "%s_%s",
+						    cat->dsc[i].name, enc.name);
+			msm_disp_snapshot_add_block(disp_state, ctl.len, dpu_kms->mmio +
+						    cat->dsc[i].base + ctl.base, "%s_%s",
+						    cat->dsc[i].name, ctl.name);
+		} else {
+			msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
+						    cat->dsc[i].base, cat->dsc[i].name);
+		}
+	}
 
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }

-- 
2.25.1

Re: [PATCH v4 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks

[Dmitry Baryshkov]

On 06/07/2023 23: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. Edit
> dpu_kms_mdp_snapshot function to account for sub-blocks.
> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 ++++++++++++++++++++++++++++++---
>   1 file changed, 60 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index 70dbb1204e6c..afc45d597d65 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -903,25 +903,58 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   					    cat->ctl[i].base, cat->ctl[i].name);
>   
>   	/* dump DSPP sub-blocks HW regs info */
> -	for (i = 0; i < cat->dspp_count; i++)
> +	for (i = 0; i < cat->dspp_count; i++) {
>   		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
>   					    cat->dspp[i].base, cat->dspp[i].name);
>   
> +		if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
> +			msm_disp_snapshot_add_block(disp_state, cat->dspp[i].sblk->pcc.len,
> +						    dpu_kms->mmio + cat->dspp[i].base +
> +						    cat->dspp[i].sblk->pcc.base, "%s_%s",

This might look simpler in the following form. Could you please consider it?


void *base =  dpu_kms + cat->dspp[i].base;

msm_disp_snapshot_add_block(..., base, cat->dspp[i].name)

if (!cat->dspp[i].sblk)
     continue;

if (cat->dspp[i].sblk->pcc.len)
     msm_disp_snapshot_add_block(..., base + 
cat->dspp[i].sblk->pcc.base, ...);

> +						    cat->dspp[i].name,
> +						    cat->dspp[i].sblk->pcc.name);
> +	}
> +
>   	/* dump INTF sub-blocks HW regs info */
>   	for (i = 0; i < cat->intf_count; i++)
>   		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
>   					    cat->intf[i].base, cat->intf[i].name);
>   
>   	/* dump PP sub-blocks HW regs info */
> -	for (i = 0; i < cat->pingpong_count; i++)
> +	for (i = 0; i < cat->pingpong_count; i++) {
>   		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
>   					    cat->pingpong[i].base, cat->pingpong[i].name);
>   
> +		/* TE2 block has length of 0, so will not print it */
> +
> +		if (cat->pingpong[i].sblk && cat->pingpong[i].sblk->dither.len > 0)
> +			msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].sblk->dither.len,
> +						    dpu_kms->mmio + cat->pingpong[i].base +
> +						    cat->pingpong[i].sblk->dither.base, "%s_%s",
> +						    cat->pingpong[i].name,
> +						    cat->pingpong[i].sblk->dither.name);
> +	}
> +
>   	/* dump SSPP sub-blocks HW regs info */
> -	for (i = 0; i < cat->sspp_count; i++)
> +	for (i = 0; i < cat->sspp_count; i++) {
>   		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
>   					    cat->sspp[i].base, cat->sspp[i].name);
>   
> +		if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
> +			msm_disp_snapshot_add_block(disp_state, cat->sspp[i].sblk->scaler_blk.len,
> +						    dpu_kms->mmio + cat->sspp[i].base +
> +						    cat->sspp[i].sblk->scaler_blk.base, "%s_%s",
> +						    cat->sspp[i].name,
> +						    cat->sspp[i].sblk->scaler_blk.name);
> +
> +		if (cat->sspp[i].sblk && cat->sspp[i].sblk->csc_blk.len > 0)
> +			msm_disp_snapshot_add_block(disp_state, cat->sspp[i].sblk->csc_blk.len,
> +						    dpu_kms->mmio + cat->sspp[i].base +
> +						    cat->sspp[i].sblk->csc_blk.base, "%s_%s",
> +						    cat->sspp[i].name,
> +						    cat->sspp[i].sblk->csc_blk.name);
> +	}
> +
>   	/* dump LM sub-blocks HW regs info */
>   	for (i = 0; i < cat->mixer_count; i++)
>   		msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
> @@ -943,9 +976,30 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   	}
>   
>   	/* dump DSC sub-blocks HW regs info */
> -	for (i = 0; i < cat->dsc_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
> -					    cat->dsc[i].base, cat->dsc[i].name);
> +	for (i = 0; i < cat->dsc_count; i++) {
> +		if (cat->dsc[i].features & BIT(DPU_DSC_HW_REV_1_2)) {
> +			struct dpu_dsc_blk enc = cat->dsc[i].sblk->enc;
> +			struct dpu_dsc_blk ctl = cat->dsc[i].sblk->ctl;
> +
> +			/* For now, pass in a length of 0 because the DSC_BLK register space
> +			 * overlaps with the sblks' register space.
> +			 *
> +			 * TODO: Pass in a length of 0 to DSC_BLK_1_2 in the HW catalog where
> +			 * applicable.

Please assume that https://patchwork.freedesktop.org/series/119776/ and 
rebase your code on top of it.

> +			 */
> +			msm_disp_snapshot_add_block(disp_state, 0, dpu_kms->mmio +
> +						    cat->dsc[i].base, cat->dsc[i].name);
> +			msm_disp_snapshot_add_block(disp_state, enc.len, dpu_kms->mmio +
> +						    cat->dsc[i].base + enc.base, "%s_%s",
> +						    cat->dsc[i].name, enc.name);
> +			msm_disp_snapshot_add_block(disp_state, ctl.len, dpu_kms->mmio +
> +						    cat->dsc[i].base + ctl.base, "%s_%s",
> +						    cat->dsc[i].name, ctl.name);
> +		} else {
> +			msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
> +						    cat->dsc[i].base, cat->dsc[i].name);
> +		}
> +	}
>   
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
> 

-- 
With best wishes
Dmitry

Re: [PATCH v4 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks

[Ryan McCann]

My apologies for the private email, I hit reply instead of reply all by 
accident.

On 7/6/2023 5:24 PM, Dmitry Baryshkov wrote:
> On 06/07/2023 23: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. Edit
>> dpu_kms_mdp_snapshot function to account for sub-blocks.
>>
>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66 
>> ++++++++++++++++++++++++++++++---
>>   1 file changed, 60 insertions(+), 6 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index 70dbb1204e6c..afc45d597d65 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -903,25 +903,58 @@ static void dpu_kms_mdp_snapshot(struct 
>> msm_disp_state *disp_state, struct msm_k
>>                           cat->ctl[i].base, cat->ctl[i].name);
>>       /* dump DSPP sub-blocks HW regs info */
>> -    for (i = 0; i < cat->dspp_count; i++)
>> +    for (i = 0; i < cat->dspp_count; i++) {
>>           msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>> dpu_kms->mmio +
>>                           cat->dspp[i].base, cat->dspp[i].name);
>> +        if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
>> +            msm_disp_snapshot_add_block(disp_state, 
>> cat->dspp[i].sblk->pcc.len,
>> +                            dpu_kms->mmio + cat->dspp[i].base +
>> +                            cat->dspp[i].sblk->pcc.base, "%s_%s",
> 
> This might look simpler in the following form. Could you please consider 
> it?
> 
> 
> void *base =  dpu_kms + cat->dspp[i].base;
> 
> msm_disp_snapshot_add_block(..., base, cat->dspp[i].name)
> 
> if (!cat->dspp[i].sblk)
>      continue;
> 
> if (cat->dspp[i].sblk->pcc.len)
>      msm_disp_snapshot_add_block(..., base + 
> cat->dspp[i].sblk->pcc.base, ...);

Regarding what we discussed in the private email, is what I had for base 
in v2

(https://patchwork.freedesktop.org/patch/545690/?series=120249&rev=1)

essentially what you have in mind?

> 
>> +                            cat->dspp[i].name,
>> +                            cat->dspp[i].sblk->pcc.name);
>> +    }
>> +
>>       /* dump INTF sub-blocks HW regs info */
>>       for (i = 0; i < cat->intf_count; i++)
>>           msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>> dpu_kms->mmio +
>>                           cat->intf[i].base, cat->intf[i].name);
>>       /* dump PP sub-blocks HW regs info */
>> -    for (i = 0; i < cat->pingpong_count; i++)
>> +    for (i = 0; i < cat->pingpong_count; i++) {
>>           msm_disp_snapshot_add_block(disp_state, 
>> cat->pingpong[i].len, dpu_kms->mmio +
>>                           cat->pingpong[i].base, cat->pingpong[i].name);
>> +        /* TE2 block has length of 0, so will not print it */
>> +
>> +        if (cat->pingpong[i].sblk && 
>> cat->pingpong[i].sblk->dither.len > 0)
>> +            msm_disp_snapshot_add_block(disp_state, 
>> cat->pingpong[i].sblk->dither.len,
>> +                            dpu_kms->mmio + cat->pingpong[i].base +
>> +                            cat->pingpong[i].sblk->dither.base, "%s_%s",
>> +                            cat->pingpong[i].name,
>> +                            cat->pingpong[i].sblk->dither.name);
>> +    }
>> +
>>       /* dump SSPP sub-blocks HW regs info */
>> -    for (i = 0; i < cat->sspp_count; i++)
>> +    for (i = 0; i < cat->sspp_count; i++) {
>>           msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>> dpu_kms->mmio +
>>                           cat->sspp[i].base, cat->sspp[i].name);
>> +        if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
>> +            msm_disp_snapshot_add_block(disp_state, 
>> cat->sspp[i].sblk->scaler_blk.len,
>> +                            dpu_kms->mmio + cat->sspp[i].base +
>> +                            cat->sspp[i].sblk->scaler_blk.base, "%s_%s",
>> +                            cat->sspp[i].name,
>> +                            cat->sspp[i].sblk->scaler_blk.name);
>> +
>> +        if (cat->sspp[i].sblk && cat->sspp[i].sblk->csc_blk.len > 0)
>> +            msm_disp_snapshot_add_block(disp_state, 
>> cat->sspp[i].sblk->csc_blk.len,
>> +                            dpu_kms->mmio + cat->sspp[i].base +
>> +                            cat->sspp[i].sblk->csc_blk.base, "%s_%s",
>> +                            cat->sspp[i].name,
>> +                            cat->sspp[i].sblk->csc_blk.name);
>> +    }
>> +
>>       /* dump LM sub-blocks HW regs info */
>>       for (i = 0; i < cat->mixer_count; i++)
>>           msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, 
>> dpu_kms->mmio +
>> @@ -943,9 +976,30 @@ static void dpu_kms_mdp_snapshot(struct 
>> msm_disp_state *disp_state, struct msm_k
>>       }
>>       /* dump DSC sub-blocks HW regs info */
>> -    for (i = 0; i < cat->dsc_count; i++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>> dpu_kms->mmio +
>> -                        cat->dsc[i].base, cat->dsc[i].name);
>> +    for (i = 0; i < cat->dsc_count; i++) {
>> +        if (cat->dsc[i].features & BIT(DPU_DSC_HW_REV_1_2)) {
>> +            struct dpu_dsc_blk enc = cat->dsc[i].sblk->enc;
>> +            struct dpu_dsc_blk ctl = cat->dsc[i].sblk->ctl;
>> +
>> +            /* For now, pass in a length of 0 because the DSC_BLK 
>> register space
>> +             * overlaps with the sblks' register space.
>> +             *
>> +             * TODO: Pass in a length of 0 to DSC_BLK_1_2 in the HW 
>> catalog where
>> +             * applicable.
> 
> Please assume that https://patchwork.freedesktop.org/series/119776/ and 
> rebase your code on top of it.

Roger.

> 
>> +             */
>> +            msm_disp_snapshot_add_block(disp_state, 0, dpu_kms->mmio +
>> +                            cat->dsc[i].base, cat->dsc[i].name);
>> +            msm_disp_snapshot_add_block(disp_state, enc.len, 
>> dpu_kms->mmio +
>> +                            cat->dsc[i].base + enc.base, "%s_%s",
>> +                            cat->dsc[i].name, enc.name);
>> +            msm_disp_snapshot_add_block(disp_state, ctl.len, 
>> dpu_kms->mmio +
>> +                            cat->dsc[i].base + ctl.base, "%s_%s",
>> +                            cat->dsc[i].name, ctl.name);
>> +        } else {
>> +            msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>> dpu_kms->mmio +
>> +                            cat->dsc[i].base, cat->dsc[i].name);
>> +        }
>> +    }
>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>   }
>>
> 

Re: [PATCH v4 6/6] drm/msm/dpu: Update dev core dump to dump registers of sub-blocks

[Dmitry Baryshkov]

On Fri, 7 Jul 2023 at 23:49, Ryan McCann <quic_rmccann@quicinc.com> wrote:
>
> My apologies for the private email, I hit reply instead of reply all by
> accident.

No problem, it happens sometimes.

>
> On 7/6/2023 5:24 PM, Dmitry Baryshkov wrote:
> > On 06/07/2023 23: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. Edit
> >> dpu_kms_mdp_snapshot function to account for sub-blocks.
> >>
> >> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> >> ---
> >>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 66
> >> ++++++++++++++++++++++++++++++---
> >>   1 file changed, 60 insertions(+), 6 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> index 70dbb1204e6c..afc45d597d65 100644
> >> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> >> @@ -903,25 +903,58 @@ static void dpu_kms_mdp_snapshot(struct
> >> msm_disp_state *disp_state, struct msm_k
> >>                           cat->ctl[i].base, cat->ctl[i].name);
> >>       /* dump DSPP sub-blocks HW regs info */
> >> -    for (i = 0; i < cat->dspp_count; i++)
> >> +    for (i = 0; i < cat->dspp_count; i++) {
> >>           msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
> >> dpu_kms->mmio +
> >>                           cat->dspp[i].base, cat->dspp[i].name);
> >> +        if (cat->dspp[i].sblk && cat->dspp[i].sblk->pcc.len > 0)
> >> +            msm_disp_snapshot_add_block(disp_state,
> >> cat->dspp[i].sblk->pcc.len,
> >> +                            dpu_kms->mmio + cat->dspp[i].base +
> >> +                            cat->dspp[i].sblk->pcc.base, "%s_%s",
> >
> > This might look simpler in the following form. Could you please consider
> > it?
> >
> >
> > void *base =  dpu_kms + cat->dspp[i].base;
> >
> > msm_disp_snapshot_add_block(..., base, cat->dspp[i].name)
> >
> > if (!cat->dspp[i].sblk)
> >      continue;
> >
> > if (cat->dspp[i].sblk->pcc.len)
> >      msm_disp_snapshot_add_block(..., base +
> > cat->dspp[i].sblk->pcc.base, ...);
>
> Regarding what we discussed in the private email, is what I had for base
> in v2
>
> (https://patchwork.freedesktop.org/patch/545690/?series=120249&rev=1)
>
> essentially what you have in mind?
>
> >
> >> +                            cat->dspp[i].name,
> >> +                            cat->dspp[i].sblk->pcc.name);
> >> +    }
> >> +
> >>       /* dump INTF sub-blocks HW regs info */
> >>       for (i = 0; i < cat->intf_count; i++)
> >>           msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
> >> dpu_kms->mmio +
> >>                           cat->intf[i].base, cat->intf[i].name);
> >>       /* dump PP sub-blocks HW regs info */
> >> -    for (i = 0; i < cat->pingpong_count; i++)
> >> +    for (i = 0; i < cat->pingpong_count; i++) {
> >>           msm_disp_snapshot_add_block(disp_state,
> >> cat->pingpong[i].len, dpu_kms->mmio +
> >>                           cat->pingpong[i].base, cat->pingpong[i].name);
> >> +        /* TE2 block has length of 0, so will not print it */
> >> +
> >> +        if (cat->pingpong[i].sblk &&
> >> cat->pingpong[i].sblk->dither.len > 0)
> >> +            msm_disp_snapshot_add_block(disp_state,
> >> cat->pingpong[i].sblk->dither.len,
> >> +                            dpu_kms->mmio + cat->pingpong[i].base +
> >> +                            cat->pingpong[i].sblk->dither.base, "%s_%s",
> >> +                            cat->pingpong[i].name,
> >> +                            cat->pingpong[i].sblk->dither.name);
> >> +    }
> >> +
> >>       /* dump SSPP sub-blocks HW regs info */
> >> -    for (i = 0; i < cat->sspp_count; i++)
> >> +    for (i = 0; i < cat->sspp_count; i++) {
> >>           msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
> >> dpu_kms->mmio +
> >>                           cat->sspp[i].base, cat->sspp[i].name);
> >> +        if (cat->sspp[i].sblk && cat->sspp[i].sblk->scaler_blk.len > 0)
> >> +            msm_disp_snapshot_add_block(disp_state,
> >> cat->sspp[i].sblk->scaler_blk.len,
> >> +                            dpu_kms->mmio + cat->sspp[i].base +
> >> +                            cat->sspp[i].sblk->scaler_blk.base, "%s_%s",
> >> +                            cat->sspp[i].name,
> >> +                            cat->sspp[i].sblk->scaler_blk.name);
> >> +
> >> +        if (cat->sspp[i].sblk && cat->sspp[i].sblk->csc_blk.len > 0)
> >> +            msm_disp_snapshot_add_block(disp_state,
> >> cat->sspp[i].sblk->csc_blk.len,
> >> +                            dpu_kms->mmio + cat->sspp[i].base +
> >> +                            cat->sspp[i].sblk->csc_blk.base, "%s_%s",
> >> +                            cat->sspp[i].name,
> >> +                            cat->sspp[i].sblk->csc_blk.name);
> >> +    }
> >> +
> >>       /* dump LM sub-blocks HW regs info */
> >>       for (i = 0; i < cat->mixer_count; i++)
> >>           msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
> >> dpu_kms->mmio +
> >> @@ -943,9 +976,30 @@ static void dpu_kms_mdp_snapshot(struct
> >> msm_disp_state *disp_state, struct msm_k
> >>       }
> >>       /* dump DSC sub-blocks HW regs info */
> >> -    for (i = 0; i < cat->dsc_count; i++)
> >> -        msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
> >> dpu_kms->mmio +
> >> -                        cat->dsc[i].base, cat->dsc[i].name);
> >> +    for (i = 0; i < cat->dsc_count; i++) {
> >> +        if (cat->dsc[i].features & BIT(DPU_DSC_HW_REV_1_2)) {
> >> +            struct dpu_dsc_blk enc = cat->dsc[i].sblk->enc;
> >> +            struct dpu_dsc_blk ctl = cat->dsc[i].sblk->ctl;
> >> +
> >> +            /* For now, pass in a length of 0 because the DSC_BLK
> >> register space
> >> +             * overlaps with the sblks' register space.
> >> +             *
> >> +             * TODO: Pass in a length of 0 to DSC_BLK_1_2 in the HW
> >> catalog where
> >> +             * applicable.
> >
> > Please assume that https://patchwork.freedesktop.org/series/119776/ and
> > rebase your code on top of it.
>
> Roger.
>
> >
> >> +             */
> >> +            msm_disp_snapshot_add_block(disp_state, 0, dpu_kms->mmio +
> >> +                            cat->dsc[i].base, cat->dsc[i].name);
> >> +            msm_disp_snapshot_add_block(disp_state, enc.len,
> >> dpu_kms->mmio +
> >> +                            cat->dsc[i].base + enc.base, "%s_%s",
> >> +                            cat->dsc[i].name, enc.name);
> >> +            msm_disp_snapshot_add_block(disp_state, ctl.len,
> >> dpu_kms->mmio +
> >> +                            cat->dsc[i].base + ctl.base, "%s_%s",
> >> +                            cat->dsc[i].name, ctl.name);
> >> +        } else {
> >> +            msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
> >> dpu_kms->mmio +
> >> +                            cat->dsc[i].base, cat->dsc[i].name);
> >> +        }
> >> +    }
> >>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
> >>   }
> >>
> >



-- 
With best wishes
Dmitry