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

[PATCH v2 0/5] 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 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 (5):
      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: 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           | 106 +++++++++++++++++-----
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |   2 +-
 3 files changed, 128 insertions(+), 70 deletions(-)
---
base-commit: a0364260213c96f6817f7e85cdce293cb743460f
change-id: 20230622-devcoredump_patch-df7e8f6fd632

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

[PATCH v2 1/5] 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>
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

Re: [PATCH v2 1/5] drm/msm: Update dev core dump to not print backwards

[Abhinav Kumar]

On 7/5/2023 12:30 PM, Ryan McCann wrote:
> 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>
> 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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 2/5] 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.

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

Re: [PATCH v2 2/5] drm/msm/dpu: Drop unused num argument from relevant macros

[Dmitry Baryshkov]

On 05/07/2023 22:30, Ryan McCann wrote:
> Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block
> macros. Update calls to relevant macros to reflect change.
> 
> 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(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 2/5] drm/msm/dpu: Drop unused num argument from relevant macros

[Abhinav Kumar]

On 7/5/2023 12:30 PM, Ryan McCann wrote:
> Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub-block
> macros. Update calls to relevant macros to reflect change.
> 
> 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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 3/5] 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.

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

Re: [PATCH v2 3/5] drm/msm/dpu: Define names for unnamed sblks

[Dmitry Baryshkov]

On 05/07/2023 22:30, Ryan McCann wrote:
> 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.
> 
> 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(-)

I'm not happy with this approach, but let's see how it goes.

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 3/5] drm/msm/dpu: Define names for unnamed sblks

[Abhinav Kumar]

On 7/5/2023 12:30 PM, Ryan McCann wrote:
> 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.
> 
> 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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 4/5] 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.

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

Re: [PATCH v2 4/5] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks

[Dmitry Baryshkov]

On 05/07/2023 22:30, Ryan McCann wrote:
> 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.
> 
> 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(-)

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>

-- 
With best wishes
Dmitry

Re: [PATCH v2 4/5] drm/msm/dpu: Remove redundant prefix/suffix in name of sub-blocks

[Abhinav Kumar]

On 7/5/2023 12:30 PM, Ryan McCann wrote:
> 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.
> 
> 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(-)
> 

Reviewed-by: Abhinav Kumar <quic_abhinavk@quicinc.com>

[PATCH v2 5/5] 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 | 106 ++++++++++++++++++++++++--------
 1 file changed, 82 insertions(+), 24 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..c83f5d79e5c5 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 	int i;
 	struct dpu_kms *dpu_kms;
 	const struct dpu_mdss_cfg *cat;
+	void __iomem *mmio;
+	u32 base;
 
 	dpu_kms = to_dpu_kms(kms);
 
 	cat = dpu_kms->catalog;
+	mmio = dpu_kms->mmio;
 
 	pm_runtime_get_sync(&dpu_kms->pdev->dev);
 
 	/* 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, mmio + cat->ctl[i].base,
+					    "%s", 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);
+	for (i = 0; i < cat->dspp_count; i++) {
+		base = cat->dspp[i].base;
+		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, mmio + base, "%s",
+					    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,
+						    mmio + 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, "intf_%d", i);
+		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, mmio + cat->intf[i].base,
+					    "%s", 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);
+	for (i = 0; i < cat->pingpong_count; i++) {
+		base = cat->pingpong[i].base;
+		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, mmio + base, "%s",
+					    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,
+						    mmio + 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++)
-		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
-				dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
+	for (i = 0; i < cat->sspp_count; i++) {
+		base = cat->sspp[i].base;
+		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, mmio + cat->sspp[i].base,
+					    "%s", 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,
+						    mmio + 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,
+						    mmio + 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 + cat->mixer[i].base, "lm_%d", i);
+					    mmio + cat->mixer[i].base,
+					    "%s", 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, mmio + cat->wb[i].base,
+					    "%s", 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,
-				dpu_kms->mmio + cat->mdp[0].base, "top");
+		msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0, mmio + cat->mdp[0].base,
+					    "top");
 		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - MDP_PERIPH_TOP0_END,
-				dpu_kms->mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END, "top_2");
+					    mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END,
+					    "top_2");
 	} else {
-		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
-				dpu_kms->mmio + cat->mdp[0].base, "top");
+		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len, mmio + cat->mdp[0].base,
+					    "top");
 	}
 
 	/* 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);
+	for (i = 0; i < cat->dsc_count; i++) {
+		base = cat->dsc[i].base;
+
+		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 t0 DSC_BLK_1_2 in the HW catalog where
+			 * applicable.
+			 */
+			msm_disp_snapshot_add_block(disp_state, 0, mmio + base, "%s", cat->dsc[i].name);
+			msm_disp_snapshot_add_block(disp_state, enc.len, mmio + base + enc.base,
+						    "%s_%s", cat->dsc[i].name, enc.name);
+			msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + base + ctl.base,
+						    "%s_%s", cat->dsc[i].name, ctl.name);
+		} else {
+			msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, mmio + base, "%s",
+						    cat->dsc[i].name);
+		}
+	}
 
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }

-- 
2.25.1

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

[Dmitry Baryshkov]

On 05/07/2023 22:30, 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 | 106 ++++++++++++++++++++++++--------
>   1 file changed, 82 insertions(+), 24 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..c83f5d79e5c5 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   	int i;
>   	struct dpu_kms *dpu_kms;
>   	const struct dpu_mdss_cfg *cat;
> +	void __iomem *mmio;
> +	u32 base;
>   
>   	dpu_kms = to_dpu_kms(kms);
>   
>   	cat = dpu_kms->catalog;
> +	mmio = dpu_kms->mmio;
>   
>   	pm_runtime_get_sync(&dpu_kms->pdev->dev);
>   
>   	/* 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, mmio + cat->ctl[i].base,
> +					    "%s", cat->ctl[i].name);

This is not relevant to sub-blocks. If you wish to refactor the main 
block printing, please split it to a separate commit.

Also, please note that `msm_disp_snapshot_add_block(...., "%s", 
block->name)` is redundant. We do not expect formatting characters in 
block names. So, "%s" can be dropped.

>   
>   	/* 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);
> +	for (i = 0; i < cat->dspp_count; i++) {
> +		base = cat->dspp[i].base;
> +		msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, mmio + base, "%s",
> +					    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,
> +						    mmio + 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, "intf_%d", i);
> +		msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, mmio + cat->intf[i].base,
> +					    "%s", 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);
> +	for (i = 0; i < cat->pingpong_count; i++) {
> +		base = cat->pingpong[i].base;
> +		msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, mmio + base, "%s",
> +					    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,
> +						    mmio + 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++)
> -		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
> -				dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
> +	for (i = 0; i < cat->sspp_count; i++) {
> +		base = cat->sspp[i].base;
> +		msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, mmio + cat->sspp[i].base,
> +					    "%s", 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,
> +						    mmio + 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,
> +						    mmio + 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 + cat->mixer[i].base, "lm_%d", i);
> +					    mmio + cat->mixer[i].base,
> +					    "%s", 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, mmio + cat->wb[i].base,
> +					    "%s", 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,
> -				dpu_kms->mmio + cat->mdp[0].base, "top");
> +		msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0, mmio + cat->mdp[0].base,
> +					    "top");
>   		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - MDP_PERIPH_TOP0_END,
> -				dpu_kms->mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END, "top_2");
> +					    mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END,
> +					    "top_2");
>   	} else {
> -		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
> -				dpu_kms->mmio + cat->mdp[0].base, "top");
> +		msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len, mmio + cat->mdp[0].base,
> +					    "top");
>   	}
>   
>   	/* 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);
> +	for (i = 0; i < cat->dsc_count; i++) {
> +		base = cat->dsc[i].base;
> +
> +		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 t0 DSC_BLK_1_2 in the HW catalog where
> +			 * applicable.
> +			 */
> +			msm_disp_snapshot_add_block(disp_state, 0, mmio + base, "%s", cat->dsc[i].name);
> +			msm_disp_snapshot_add_block(disp_state, enc.len, mmio + base + enc.base,
> +						    "%s_%s", cat->dsc[i].name, enc.name);
> +			msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + base + ctl.base,
> +						    "%s_%s", cat->dsc[i].name, ctl.name);
> +		} else {
> +			msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, mmio + base, "%s",
> +						    cat->dsc[i].name);
> +		}
> +	}
>   
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
> 

-- 
With best wishes
Dmitry

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

[Ryan McCann]

On 7/5/2023 1:22 PM, Dmitry Baryshkov wrote:
> On 05/07/2023 22:30, 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 | 106 
>> ++++++++++++++++++++++++--------
>>   1 file changed, 82 insertions(+), 24 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..c83f5d79e5c5 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct 
>> msm_disp_state *disp_state, struct msm_k
>>       int i;
>>       struct dpu_kms *dpu_kms;
>>       const struct dpu_mdss_cfg *cat;
>> +    void __iomem *mmio;
>> +    u32 base;
>>       dpu_kms = to_dpu_kms(kms);
>>       cat = dpu_kms->catalog;
>> +    mmio = dpu_kms->mmio;
>>       pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>       /* 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, mmio 
>> + cat->ctl[i].base,
>> +                        "%s", cat->ctl[i].name);
> 
> This is not relevant to sub-blocks. If you wish to refactor the main 
> block printing, please split it to a separate commit.

Ok. I will split this commit into changes pertaining to sub-blocks and 
changes to how the name of main blocks are printed. I would like to 
print main block names as they appear in the catalog.
> 
> Also, please note that `msm_disp_snapshot_add_block(...., "%s", 
> block->name)` is redundant. We do not expect formatting characters in 
> block names. So, "%s" can be dropped.

Here, "%s" is used in order to print the the name of the main block from 
the catalog. As mentioned above I can implement this in another commit.
> 
>>       /* 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);
>> +    for (i = 0; i < cat->dspp_count; i++) {
>> +        base = cat->dspp[i].base;
>> +        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>> mmio + base, "%s",
>> +                        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,
>> +                            mmio + 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, "intf_%d", i);
>> +        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>> mmio + cat->intf[i].base,
>> +                        "%s", 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);
>> +    for (i = 0; i < cat->pingpong_count; i++) {
>> +        base = cat->pingpong[i].base;
>> +        msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, 
>> mmio + base, "%s",
>> +                        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,
>> +                            mmio + 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++)
>> -        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>> -                dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>> +    for (i = 0; i < cat->sspp_count; i++) {
>> +        base = cat->sspp[i].base;
>> +        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>> mmio + cat->sspp[i].base,
>> +                        "%s", 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,
>> +                            mmio + 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,
>> +                            mmio + 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 + cat->mixer[i].base, "lm_%d", i);
>> +                        mmio + cat->mixer[i].base,
>> +                        "%s", 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, mmio 
>> + cat->wb[i].base,
>> +                        "%s", 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,
>> -                dpu_kms->mmio + cat->mdp[0].base, "top");
>> +        msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0, mmio 
>> + cat->mdp[0].base,
>> +                        "top");
>>           msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - 
>> MDP_PERIPH_TOP0_END,
>> -                dpu_kms->mmio + cat->mdp[0].base + 
>> MDP_PERIPH_TOP0_END, "top_2");
>> +                        mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END,
>> +                        "top_2");
>>       } else {
>> -        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
>> -                dpu_kms->mmio + cat->mdp[0].base, "top");
>> +        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len, mmio 
>> + cat->mdp[0].base,
>> +                        "top");
>>       }
>>       /* 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);
>> +    for (i = 0; i < cat->dsc_count; i++) {
>> +        base = cat->dsc[i].base;
>> +
>> +        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 t0 DSC_BLK_1_2 in the HW 
>> catalog where
>> +             * applicable.
>> +             */
>> +            msm_disp_snapshot_add_block(disp_state, 0, mmio + base, 
>> "%s", cat->dsc[i].name);
>> +            msm_disp_snapshot_add_block(disp_state, enc.len, mmio + 
>> base + enc.base,
>> +                            "%s_%s", cat->dsc[i].name, enc.name);
>> +            msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + 
>> base + ctl.base,
>> +                            "%s_%s", cat->dsc[i].name, ctl.name);
>> +        } else {
>> +            msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>> mmio + base, "%s",
>> +                            cat->dsc[i].name);
>> +        }
>> +    }
>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>   }
>>
> 

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

[Dmitry Baryshkov]

On 05/07/2023 23:39, Ryan McCann wrote:
> 
> 
> On 7/5/2023 1:22 PM, Dmitry Baryshkov wrote:
>> On 05/07/2023 22:30, 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 | 106 
>>> ++++++++++++++++++++++++--------
>>>   1 file changed, 82 insertions(+), 24 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..c83f5d79e5c5 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -890,62 +890,120 @@ static void dpu_kms_mdp_snapshot(struct 
>>> msm_disp_state *disp_state, struct msm_k
>>>       int i;
>>>       struct dpu_kms *dpu_kms;
>>>       const struct dpu_mdss_cfg *cat;
>>> +    void __iomem *mmio;
>>> +    u32 base;
>>>       dpu_kms = to_dpu_kms(kms);
>>>       cat = dpu_kms->catalog;
>>> +    mmio = dpu_kms->mmio;
>>>       pm_runtime_get_sync(&dpu_kms->pdev->dev);
>>>       /* 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, 
>>> mmio + cat->ctl[i].base,
>>> +                        "%s", cat->ctl[i].name);
>>
>> This is not relevant to sub-blocks. If you wish to refactor the main 
>> block printing, please split it to a separate commit.
> 
> Ok. I will split this commit into changes pertaining to sub-blocks and 
> changes to how the name of main blocks are printed. I would like to 
> print main block names as they appear in the catalog.

Yes, please.

>>
>> Also, please note that `msm_disp_snapshot_add_block(...., "%s", 
>> block->name)` is redundant. We do not expect formatting characters in 
>> block names. So, "%s" can be dropped.
> 
> Here, "%s" is used in order to print the the name of the main block from 
> the catalog. As mentioned above I can implement this in another commit.

Dropping the extra "%s" will get the same result.

>>
>>>       /* 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);
>>> +    for (i = 0; i < cat->dspp_count; i++) {
>>> +        base = cat->dspp[i].base;
>>> +        msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, 
>>> mmio + base, "%s",
>>> +                        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,
>>> +                            mmio + 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, "intf_%d", i);
>>> +        msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, 
>>> mmio + cat->intf[i].base,
>>> +                        "%s", 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);
>>> +    for (i = 0; i < cat->pingpong_count; i++) {
>>> +        base = cat->pingpong[i].base;
>>> +        msm_disp_snapshot_add_block(disp_state, 
>>> cat->pingpong[i].len, mmio + base, "%s",
>>> +                        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,
>>> +                            mmio + 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++)
>>> -        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>> -                dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>>> +    for (i = 0; i < cat->sspp_count; i++) {
>>> +        base = cat->sspp[i].base;
>>> +        msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, 
>>> mmio + cat->sspp[i].base,
>>> +                        "%s", 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,
>>> +                            mmio + 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,
>>> +                            mmio + 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 + cat->mixer[i].base, "lm_%d", i);
>>> +                        mmio + cat->mixer[i].base,
>>> +                        "%s", 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, mmio 
>>> + cat->wb[i].base,
>>> +                        "%s", 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,
>>> -                dpu_kms->mmio + cat->mdp[0].base, "top");
>>> +        msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0, 
>>> mmio + cat->mdp[0].base,
>>> +                        "top");
>>>           msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len - 
>>> MDP_PERIPH_TOP0_END,
>>> -                dpu_kms->mmio + cat->mdp[0].base + 
>>> MDP_PERIPH_TOP0_END, "top_2");
>>> +                        mmio + cat->mdp[0].base + MDP_PERIPH_TOP0_END,
>>> +                        "top_2");
>>>       } else {
>>> -        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len,
>>> -                dpu_kms->mmio + cat->mdp[0].base, "top");
>>> +        msm_disp_snapshot_add_block(disp_state, cat->mdp[0].len, 
>>> mmio + cat->mdp[0].base,
>>> +                        "top");
>>>       }
>>>       /* 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);
>>> +    for (i = 0; i < cat->dsc_count; i++) {
>>> +        base = cat->dsc[i].base;
>>> +
>>> +        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 t0 DSC_BLK_1_2 in the HW 
>>> catalog where
>>> +             * applicable.
>>> +             */
>>> +            msm_disp_snapshot_add_block(disp_state, 0, mmio + base, 
>>> "%s", cat->dsc[i].name);
>>> +            msm_disp_snapshot_add_block(disp_state, enc.len, mmio + 
>>> base + enc.base,
>>> +                            "%s_%s", cat->dsc[i].name, enc.name);
>>> +            msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + 
>>> base + ctl.base,
>>> +                            "%s_%s", cat->dsc[i].name, ctl.name);
>>> +        } else {
>>> +            msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, 
>>> mmio + base, "%s",
>>> +                            cat->dsc[i].name);
>>> +        }
>>> +    }
>>>       pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>   }
>>>
>>

-- 
With best wishes
Dmitry