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

[PATCH 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====
...

Signed-off-by: Ryan McCann <quic_rmccann@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 suffix in name of sub blocks
      drm/msm/disp: Remove redundant prefix 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           | 194 +++++++++++++++++++---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |   2 +-
 3 files changed, 214 insertions(+), 72 deletions(-)
---
base-commit: 710025fdedb3767655823c3a12d27d404d209f75
change-id: 20230622-devcoredump_patch-df7e8f6fd632

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

[PATCH 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")
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 1/6] drm/msm: Update dev core dump to not print backwards

[Dmitry Baryshkov]

On 23/06/2023 02:48, 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")
> 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: Dmitry Baryshkov <dmitry.baryshkov@linaro.org>


-- 
With best wishes
Dmitry

[PATCH 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.

Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 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..69200b4cf210 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) \
@@ -365,11 +365,11 @@ static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
 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);
+				_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);
 
-#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 2/6] drm/msm/dpu: Drop unused num argument from relevant macros

[Dmitry Baryshkov]

On 23/06/2023 02:48, Ryan McCann wrote:
> Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub block
> macros. Update calls to relevant macros to reflect change.

This is not relevant for this patchset.

With https://patchwork.freedesktop.org/patch/534745/?series=116851&rev=2 
in place, VIG_SBLK and VIG_SBLK_ROT should also stop using num. But 
let's probably take another step forward and drop both arguments and use 
just a single instance of sblk per platform. I'll send a patch for it.

> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 22 +++++++++++-----------
>   1 file changed, 11 insertions(+), 11 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..69200b4cf210 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) \
> @@ -365,11 +365,11 @@ static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
>   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);
> +				_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);
>   
> -#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
> 

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 23/06/2023 02:57, Dmitry Baryshkov wrote:
> On 23/06/2023 02:48, Ryan McCann wrote:
>> Drop unused parameter "num" from VIG_SBLK_NOSCALE and DMA sub block
>> macros. Update calls to relevant macros to reflect change.
> 
> This is not relevant for this patchset.

Ok, after reviewing the rest of the patches, I see that it is indeed 
relevant. However could you please follow another path (outlined in the 
review for patch 6).

Note, that for the sake of subblock dumping we can ignore the issue with 
the names completely and drop them in the separate patchset.

> 
> With https://patchwork.freedesktop.org/patch/534745/?series=116851&rev=2 
> in place, VIG_SBLK and VIG_SBLK_ROT should also stop using num. But 
> let's probably take another step forward and drop both arguments and use 
> just a single instance of sblk per platform. I'll send a patch for it.
> 
>>
>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 22 
>> +++++++++++-----------
>>   1 file changed, 11 insertions(+), 11 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..69200b4cf210 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) \
>> @@ -365,11 +365,11 @@ static const struct dpu_sspp_sub_blks 
>> sm8550_vig_sblk_1 =
>>   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);
>> +                _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);
>> -#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
>>
> 

-- 
With best wishes
Dmitry

[PATCH 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.

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 69200b4cf210..8349ecda1f3c 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 4/6] drm/msm/dpu: Remove redundant suffix in name of sub blocks

[Ryan McCann]

For a device core dump, the registers of sub blocks are printed under the
title <mainBlkName_sblkName>. For example, the csc sub block for an SSPP
block "sspp_0" would be printed "sspp_0_csc0". There is a redundant 0 in
the title due to a concatention done in the definition of the VIG_SBLK
macro, the macro used to define the sub blocks of "sspp_0". Remove this
concatenation to eliminate redundancy and remove the num parameter of
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 | 48 +++++++++++++-------------
 1 file changed, 24 insertions(+), 24 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 8349ecda1f3c..c624b2cf0b35 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 = "sspp_scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = STRCAT("sspp_csc", num), \
+	.csc_blk = {.name = "sspp_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 = "sspp_scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = STRCAT("sspp_csc", num), \
+	.csc_blk = {.name = "sspp_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,29 +341,29 @@ 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(10, DPU_SSPP_SCALER_QSEED4);
 static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK(5);

-- 
2.25.1

[PATCH 5/6] drm/msm/disp: Remove redundant prefix in name of sub blocks

[Ryan McCann]

When the registers of the csc and scaler sub blocks are printed during a
device core dump, the sub block title is printed: <mainBlkName_sblkName>.
Currently this appears as "sspp_0_sspp_csc" for a csc sub block to an
SSPP main block named "sspp_0". Because the name of the sub block defined
in the VIG_SBLK macro is "sspp_csc", the result is a redundant name. To
avoid this redundancy, remove the duplicate prefix "sspp".

Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
---
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 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 c624b2cf0b35..836efa074a35 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -257,10 +257,10 @@ static const uint32_t wb2_formats[] = {
 	.maxdwnscale = MAX_DOWNSCALE_RATIO, \
 	.maxupscale = MAX_UPSCALE_RATIO, \
 	.smart_dma_priority = sdma_pri, \
-	.scaler_blk = {.name = "sspp_scaler", \
+	.scaler_blk = {.name = "scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = "sspp_csc", \
+	.csc_blk = {.name = "csc", \
 		.id = DPU_SSPP_CSC_10BIT, \
 		.base = 0x1a00, .len = 0x100,}, \
 	.format_list = plane_formats_yuv, \
@@ -275,10 +275,10 @@ static const uint32_t wb2_formats[] = {
 	.maxdwnscale = MAX_DOWNSCALE_RATIO, \
 	.maxupscale = MAX_UPSCALE_RATIO, \
 	.smart_dma_priority = sdma_pri, \
-	.scaler_blk = {.name = "sspp_scaler", \
+	.scaler_blk = {.name = "scaler", \
 		.id = qseed_ver, \
 		.base = 0xa00, .len = 0xa0,}, \
-	.csc_blk = {.name = "sspp_csc", \
+	.csc_blk = {.name = "csc", \
 		.id = DPU_SSPP_CSC_10BIT, \
 		.base = 0x1a00, .len = 0x100,}, \
 	.format_list = plane_formats_yuv, \

-- 
2.25.1

[PATCH 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. Add wrapper
function to dump hardware blocks that contain sub blocks.

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

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..9b1b1c382269 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
 	return 0;
 }
 
+static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state *disp_state,
+					   void __iomem *mmio, void *blk,
+					   enum dpu_hw_blk_type blk_type)
+{
+	u32 base;
+
+	switch (blk_type) {
+	case DPU_HW_BLK_TOP:
+	{
+		struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
+
+		if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
+			msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
+						    mmio + top->base, "top");
+			msm_disp_snapshot_add_block(disp_state, top->len - MDP_PERIPH_TOP0_END,
+						    mmio + top->base + MDP_PERIPH_TOP0_END,
+						    "top_2");
+		} else {
+			msm_disp_snapshot_add_block(disp_state, top->len, mmio + top->base, "top");
+		}
+		break;
+	}
+	case DPU_HW_BLK_LM:
+	{
+		struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
+
+		msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + mixer->base, "%s",
+					    mixer->name);
+		break;
+	}
+	case DPU_HW_BLK_CTL:
+	{
+		struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
+
+		msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + ctl->base, "%s",
+					    ctl->name);
+		break;
+	}
+	case DPU_HW_BLK_INTF:
+	{
+		struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
+
+		msm_disp_snapshot_add_block(disp_state, intf->len, mmio + intf->base, "%s",
+					    intf->name);
+		break;
+	}
+	case DPU_HW_BLK_WB:
+	{
+		struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
+
+		msm_disp_snapshot_add_block(disp_state, wb->len, mmio + wb->base, "%s",
+					    wb->name);
+		break;
+	}
+	case DPU_HW_BLK_SSPP:
+	{
+		struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
+		const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
+
+		base = sspp_block->base;
+
+		msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, "%s",
+					    sspp_block->name);
+
+		if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
+		    sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
+		    sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
+			msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
+						    mmio + base + sblk->scaler_blk.base, "%s_%s",
+						    sspp_block->name, sblk->scaler_blk.name);
+
+		if (sspp_block->features & BIT(DPU_SSPP_CSC) || sspp_block->features
+					& BIT(DPU_SSPP_CSC_10BIT))
+			msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
+						    mmio + base + sblk->csc_blk.base, "%s_%s",
+						    sspp_block->name, sblk->csc_blk.name);
+		break;
+	}
+	case DPU_HW_BLK_DSPP:
+	{
+		struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
+
+		base = dspp_block->base;
+
+		msm_disp_snapshot_add_block(disp_state, dspp_block->len, mmio + base, "%s",
+					    dspp_block->name);
+
+		if (dspp_block->features & BIT(DPU_DSPP_PCC))
+			msm_disp_snapshot_add_block(disp_state, dspp_block->sblk->pcc.len,
+						    mmio + base + dspp_block->sblk->pcc.base,
+						    "%s_%s", dspp_block->name,
+						    dspp_block->sblk->pcc.name);
+		break;
+	}
+	case DPU_HW_BLK_PINGPONG:
+	{
+		struct dpu_pingpong_cfg *pingpong_block = (struct dpu_pingpong_cfg *)blk;
+		const struct dpu_pingpong_sub_blks *sblk = pingpong_block->sblk;
+
+		base = pingpong_block->base;
+
+		msm_disp_snapshot_add_block(disp_state, pingpong_block->len, mmio + base, "%s",
+					    pingpong_block->name);
+
+		if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
+			msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
+						    mmio + base + sblk->te2.base, "%s_%s",
+						    pingpong_block->name, sblk->te2.name);
+
+		if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
+			msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
+						    mmio + base + sblk->dither.base, "%s_%s",
+						    pingpong_block->name, sblk->dither.name);
+		break;
+	}
+	case DPU_HW_BLK_DSC:
+	{
+		struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
+
+		base = dsc_block->base;
+
+		if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
+			struct dpu_dsc_blk enc = dsc_block->sblk->enc;
+			struct dpu_dsc_blk ctl = dsc_block->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",
+						    dsc_block->name);
+			msm_disp_snapshot_add_block(disp_state, enc.len, mmio + base + enc.base,
+						    "%s_%s", dsc_block->name, enc.name);
+			msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + base + ctl.base,
+						    "%s_%s", dsc_block->name, ctl.name);
+		} else {
+			msm_disp_snapshot_add_block(disp_state, dsc_block->len, mmio + base, "%s",
+						    dsc_block->name);
+		}
+		break;
+	}
+	default:
+		DPU_ERROR("Block type not supported.");
+	}
+}
+
 static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms)
 {
 	int i;
@@ -899,53 +1047,47 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
 
 	/* dump CTL sub-blocks HW regs info */
 	for (i = 0; i < cat->ctl_count; i++)
-		msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
-				dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->ctl[i],
+					       DPU_HW_BLK_CTL);
 
 	/* 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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->dspp[i],
+					       DPU_HW_BLK_DSPP);
 
 	/* 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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->intf[i],
+					       DPU_HW_BLK_INTF);
 
 	/* 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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->pingpong[i],
+					       DPU_HW_BLK_PINGPONG);
 
 	/* 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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->sspp[i],
+					       DPU_HW_BLK_SSPP);
 
 	/* 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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->mixer[i],
+					       DPU_HW_BLK_LM);
 
 	/* 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);
-
-	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, cat->mdp[0].len - MDP_PERIPH_TOP0_END,
-				dpu_kms->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");
-	}
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->wb[i],
+					       DPU_HW_BLK_WB);
+
+	/* dump top block */
+	dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->mdp[0],
+				       DPU_HW_BLK_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);
+		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->dsc[i],
+					       DPU_HW_BLK_DSC);
 
 	pm_runtime_put_sync(&dpu_kms->pdev->dev);
 }

-- 
2.25.1

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

[Dmitry Baryshkov]

On 23/06/2023 02:48, Ryan McCann wrote:
> Currently, the device core dump mechanism does not dump registers of sub
> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
> function to dump hardware blocks that contain sub blocks.
> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 +++++++++++++++++++++++++++-----
>   1 file changed, 168 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa8499de1b9f..9b1b1c382269 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
>   	return 0;
>   }
>   
> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state *disp_state,
> +					   void __iomem *mmio, void *blk,
> +					   enum dpu_hw_blk_type blk_type)

No. Such multiplexers add no value to the code. Please inline it.

Not to mention that this patch is hard to review. You both move existing 
code and add new features. If it were to go, it should have been split 
into two patches: one introducing the multiplexer and another one adding 
subblocks.

> +{
> +	u32 base;
> +
> +	switch (blk_type) {
> +	case DPU_HW_BLK_TOP:
> +	{
> +		struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
> +
> +		if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
> +			msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
> +						    mmio + top->base, "top");
> +			msm_disp_snapshot_add_block(disp_state, top->len - MDP_PERIPH_TOP0_END,
> +						    mmio + top->base + MDP_PERIPH_TOP0_END,
> +						    "top_2");
> +		} else {
> +			msm_disp_snapshot_add_block(disp_state, top->len, mmio + top->base, "top");
> +		}
> +		break;
> +	}
> +	case DPU_HW_BLK_LM:
> +	{
> +		struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
> +
> +		msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + mixer->base, "%s",
> +					    mixer->name);
> +		break;
> +	}
> +	case DPU_HW_BLK_CTL:
> +	{
> +		struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
> +
> +		msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + ctl->base, "%s",
> +					    ctl->name);
> +		break;
> +	}
> +	case DPU_HW_BLK_INTF:
> +	{
> +		struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
> +
> +		msm_disp_snapshot_add_block(disp_state, intf->len, mmio + intf->base, "%s",
> +					    intf->name);
> +		break;
> +	}
> +	case DPU_HW_BLK_WB:
> +	{
> +		struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
> +
> +		msm_disp_snapshot_add_block(disp_state, wb->len, mmio + wb->base, "%s",
> +					    wb->name);
> +		break;
> +	}
> +	case DPU_HW_BLK_SSPP:
> +	{
> +		struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
> +		const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
> +
> +		base = sspp_block->base;
> +
> +		msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, "%s",
> +					    sspp_block->name);
> +
> +		if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
> +		    sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
> +		    sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
> +			msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
> +						    mmio + base + sblk->scaler_blk.base, "%s_%s",
> +						    sspp_block->name, sblk->scaler_blk.name);

Actually, it would be better to:
- drop name from all sblk instances (and use known string instead of the 
sblk name here)
- Use sblk->foo_blk.len to check if it should be printed or not.

> +
> +		if (sspp_block->features & BIT(DPU_SSPP_CSC) || sspp_block->features
> +					& BIT(DPU_SSPP_CSC_10BIT))

A very bad use of indentation. In future please split logically rather 
than just filling the line up to the line width.

> +			msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
> +						    mmio + base + sblk->csc_blk.base, "%s_%s",
> +						    sspp_block->name, sblk->csc_blk.name);
> +		break;
> +	}
> +	case DPU_HW_BLK_DSPP:
> +	{
> +		struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
> +
> +		base = dspp_block->base;
> +
> +		msm_disp_snapshot_add_block(disp_state, dspp_block->len, mmio + base, "%s",
> +					    dspp_block->name);
> +
> +		if (dspp_block->features & BIT(DPU_DSPP_PCC))
> +			msm_disp_snapshot_add_block(disp_state, dspp_block->sblk->pcc.len,
> +						    mmio + base + dspp_block->sblk->pcc.base,
> +						    "%s_%s", dspp_block->name,
> +						    dspp_block->sblk->pcc.name);
> +		break;
> +	}
> +	case DPU_HW_BLK_PINGPONG:
> +	{
> +		struct dpu_pingpong_cfg *pingpong_block = (struct dpu_pingpong_cfg *)blk;
> +		const struct dpu_pingpong_sub_blks *sblk = pingpong_block->sblk;
> +
> +		base = pingpong_block->base;
> +
> +		msm_disp_snapshot_add_block(disp_state, pingpong_block->len, mmio + base, "%s",
> +					    pingpong_block->name);
> +
> +		if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
> +			msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
> +						    mmio + base + sblk->te2.base, "%s_%s",
> +						    pingpong_block->name, sblk->te2.name);
> +
> +		if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
> +			msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
> +						    mmio + base + sblk->dither.base, "%s_%s",
> +						    pingpong_block->name, sblk->dither.name);
> +		break;
> +	}
> +	case DPU_HW_BLK_DSC:
> +	{
> +		struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
> +
> +		base = dsc_block->base;
> +
> +		if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
> +			struct dpu_dsc_blk enc = dsc_block->sblk->enc;
> +			struct dpu_dsc_blk ctl = dsc_block->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.

Nice catch, thank you. We should fix that.

> +			 */
> +			msm_disp_snapshot_add_block(disp_state, 0, mmio + base, "%s",
> +						    dsc_block->name);



> +			msm_disp_snapshot_add_block(disp_state, enc.len, mmio + base + enc.base,
> +						    "%s_%s", dsc_block->name, enc.name);
> +			msm_disp_snapshot_add_block(disp_state, ctl.len, mmio + base + ctl.base,
> +						    "%s_%s", dsc_block->name, ctl.name);
> +		} else {
> +			msm_disp_snapshot_add_block(disp_state, dsc_block->len, mmio + base, "%s",
> +						    dsc_block->name);
> +		}
> +		break;
> +	}
> +	default:
> +		DPU_ERROR("Block type not supported.");
> +	}
> +}
> +
>   static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms)
>   {
>   	int i;
> @@ -899,53 +1047,47 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   
>   	/* dump CTL sub-blocks HW regs info */
>   	for (i = 0; i < cat->ctl_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
> -				dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->ctl[i],
> +					       DPU_HW_BLK_CTL);
>   
>   	/* 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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->dspp[i],
> +					       DPU_HW_BLK_DSPP);
>   
>   	/* 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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->intf[i],
> +					       DPU_HW_BLK_INTF);
>   
>   	/* 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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->pingpong[i],
> +					       DPU_HW_BLK_PINGPONG);
>   
>   	/* 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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->sspp[i],
> +					       DPU_HW_BLK_SSPP);
>   
>   	/* 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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->mixer[i],
> +					       DPU_HW_BLK_LM);
>   
>   	/* 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);
> -
> -	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, cat->mdp[0].len - MDP_PERIPH_TOP0_END,
> -				dpu_kms->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");
> -	}
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->wb[i],
> +					       DPU_HW_BLK_WB);
> +
> +	/* dump top block */
> +	dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->mdp[0],
> +				       DPU_HW_BLK_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);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->dsc[i],
> +					       DPU_HW_BLK_DSC);
>   
>   	pm_runtime_put_sync(&dpu_kms->pdev->dev);
>   }
> 

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
> On 23/06/2023 02:48, Ryan McCann wrote:
>> Currently, the device core dump mechanism does not dump registers of sub
>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>> function to dump hardware blocks that contain sub blocks.
>>
>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>> ---
>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>> +++++++++++++++++++++++++++-----
>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index aa8499de1b9f..9b1b1c382269 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
>>       return 0;
>>   }
>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>> *disp_state,
>> +                       void __iomem *mmio, void *blk,
>> +                       enum dpu_hw_blk_type blk_type)
> 
> No. Such multiplexers add no value to the code. Please inline it.
> 
> Not to mention that this patch is hard to review. You both move existing 
> code and add new features. If it were to go, it should have been split 
> into two patches: one introducing the multiplexer and another one adding 
> subblocks.
> 

Ok. we can split this into:

1) adding the multiplexer
2) adding sub-blk parsing support inside the multiplexer

>> +{
>> +    u32 base;
>> +
>> +    switch (blk_type) {
>> +    case DPU_HW_BLK_TOP:
>> +    {
>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>> +
>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>> +                            mmio + top->base, "top");
>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>> MDP_PERIPH_TOP0_END,
>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>> +                            "top_2");
>> +        } else {
>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio + 
>> top->base, "top");
>> +        }
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_LM:
>> +    {
>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>> mixer->base, "%s",
>> +                        mixer->name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_CTL:
>> +    {
>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>> ctl->base, "%s",
>> +                        ctl->name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_INTF:
>> +    {
>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>> intf->base, "%s",
>> +                        intf->name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_WB:
>> +    {
>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>> wb->base, "%s",
>> +                        wb->name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_SSPP:
>> +    {
>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>> +
>> +        base = sspp_block->base;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio 
>> + base, "%s",
>> +                        sspp_block->name);
>> +
>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>> +            msm_disp_snapshot_add_block(disp_state, 
>> sblk->scaler_blk.len,
>> +                            mmio + base + sblk->scaler_blk.base, 
>> "%s_%s",
>> +                            sspp_block->name, sblk->scaler_blk.name);
> 
> Actually, it would be better to:
> - drop name from all sblk instances (and use known string instead of the 
> sblk name here)
> - Use sblk->foo_blk.len to check if it should be printed or not.
> 

No, I dont agree. If we drop the names from the sub_blk in the catalog, 
we will end up using "sub_blk_name" string here in the code to indicate 
which blk that is in the dump.

If we add more sub_blks in the catalog in the future we need to keep 
changing the code over here. Thats not how it should be.

Leaving the names in the catalog ensures that this code wont change and 
only catalog changes when we add a new sub_blk either for an existing or 
new chipset.

catalog is indicating the new blk, and dumping code just prints it.

with your approach, dumping code will or can keep changing with chipsets 
or sub_blks. Thats not how it should be.

>> +
>> +        if (sspp_block->features & BIT(DPU_SSPP_CSC) || 
>> sspp_block->features
>> +                    & BIT(DPU_SSPP_CSC_10BIT))
> 
> A very bad use of indentation. In future please split logically rather 
> than just filling the line up to the line width.
> 
>> +            msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>> +                            mmio + base + sblk->csc_blk.base, "%s_%s",
>> +                            sspp_block->name, sblk->csc_blk.name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_DSPP:
>> +    {
>> +        struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
>> +
>> +        base = dspp_block->base;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, dspp_block->len, mmio 
>> + base, "%s",
>> +                        dspp_block->name);
>> +
>> +        if (dspp_block->features & BIT(DPU_DSPP_PCC))
>> +            msm_disp_snapshot_add_block(disp_state, 
>> dspp_block->sblk->pcc.len,
>> +                            mmio + base + dspp_block->sblk->pcc.base,
>> +                            "%s_%s", dspp_block->name,
>> +                            dspp_block->sblk->pcc.name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_PINGPONG:
>> +    {
>> +        struct dpu_pingpong_cfg *pingpong_block = (struct 
>> dpu_pingpong_cfg *)blk;
>> +        const struct dpu_pingpong_sub_blks *sblk = pingpong_block->sblk;
>> +
>> +        base = pingpong_block->base;
>> +
>> +        msm_disp_snapshot_add_block(disp_state, pingpong_block->len, 
>> mmio + base, "%s",
>> +                        pingpong_block->name);
>> +
>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
>> +            msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
>> +                            mmio + base + sblk->te2.base, "%s_%s",
>> +                            pingpong_block->name, sblk->te2.name);
>> +
>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
>> +            msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
>> +                            mmio + base + sblk->dither.base, "%s_%s",
>> +                            pingpong_block->name, sblk->dither.name);
>> +        break;
>> +    }
>> +    case DPU_HW_BLK_DSC:
>> +    {
>> +        struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
>> +
>> +        base = dsc_block->base;
>> +
>> +        if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
>> +            struct dpu_dsc_blk enc = dsc_block->sblk->enc;
>> +            struct dpu_dsc_blk ctl = dsc_block->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.
> 
> Nice catch, thank you. We should fix that.
> 

Yes and we would have fixed that ourself if you wanted that with this 
series as another patch.

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

[Jessica Zhang]

On 6/23/2023 5:09 PM, Abhinav Kumar wrote:
> 
> 
> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>> On 23/06/2023 02:48, Ryan McCann wrote:
>>> Currently, the device core dump mechanism does not dump registers of sub
>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>> function to dump hardware blocks that contain sub blocks.
>>>
>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>> +++++++++++++++++++++++++++-----
>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index aa8499de1b9f..9b1b1c382269 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>> *kms)
>>>       return 0;
>>>   }
>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>> *disp_state,
>>> +                       void __iomem *mmio, void *blk,
>>> +                       enum dpu_hw_blk_type blk_type)
>>
>> No. Such multiplexers add no value to the code. Please inline it.
>>
>> Not to mention that this patch is hard to review. You both move 
>> existing code and add new features. If it were to go, it should have 
>> been split into two patches: one introducing the multiplexer and 
>> another one adding subblocks.
>>
> 
> Ok. we can split this into:
> 
> 1) adding the multiplexer
> 2) adding sub-blk parsing support inside the multiplexer
> 
>>> +{
>>> +    u32 base;
>>> +
>>> +    switch (blk_type) {
>>> +    case DPU_HW_BLK_TOP:
>>> +    {
>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>> +
>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>> +                            mmio + top->base, "top");
>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>> MDP_PERIPH_TOP0_END,
>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>> +                            "top_2");
>>> +        } else {
>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio + 
>>> top->base, "top");
>>> +        }
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_LM:
>>> +    {
>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>> mixer->base, "%s",
>>> +                        mixer->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_CTL:
>>> +    {
>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>> ctl->base, "%s",
>>> +                        ctl->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_INTF:
>>> +    {
>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>> intf->base, "%s",
>>> +                        intf->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_WB:
>>> +    {
>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>> wb->base, "%s",
>>> +                        wb->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_SSPP:
>>> +    {
>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>> +
>>> +        base = sspp_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>> mmio + base, "%s",
>>> +                        sspp_block->name);
>>> +
>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>> +            msm_disp_snapshot_add_block(disp_state, 
>>> sblk->scaler_blk.len,
>>> +                            mmio + base + sblk->scaler_blk.base, 
>>> "%s_%s",
>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>
>> Actually, it would be better to:
>> - drop name from all sblk instances (and use known string instead of 
>> the sblk name here)

Hey Dmitry,

FWIW, I second Abhinav's points about the sblk names. For example, if in 
the future we want to add a "_rot" suffix specifically to the 
VIG_SBLK_ROT.scaler name, it would be easier to just make that change in 
the HW catalog.

>> - Use sblk->foo_blk.len to check if it should be printed or not.
 From my understanding, your suggestion is to replace the feature flag 
checks with a sblk.len > 0 check.

I don't think that would be good because it wouldn't be correct to 
assume that the sblk will always be present. For example, for 
DPU_HW_BLK_DSC, the sblks will only be present for DSC_BLK_1_2.

In addition, it is possible for sblks, like pp_sblk_te.te2, to have a 
len of 0. While the register space of that specific sblk will not be 
printed, I'd prefer the devcore dump to reflect what is present within 
the HW catalog so that the user knows which pingpong blks have the TE2 sblk.

Thanks,

Jessica Zhang

>>
> 
> No, I dont agree. If we drop the names from the sub_blk in the catalog, 
> we will end up using "sub_blk_name" string here in the code to indicate 
> which blk that is in the dump.
> 
> If we add more sub_blks in the catalog in the future we need to keep 
> changing the code over here. Thats not how it should be.
> 
> Leaving the names in the catalog ensures that this code wont change and 
> only catalog changes when we add a new sub_blk either for an existing or 
> new chipset.
> 
> catalog is indicating the new blk, and dumping code just prints it.
> 
> with your approach, dumping code will or can keep changing with chipsets 
> or sub_blks. Thats not how it should be.
> 
>>> +
>>> +        if (sspp_block->features & BIT(DPU_SSPP_CSC) || 
>>> sspp_block->features
>>> +                    & BIT(DPU_SSPP_CSC_10BIT))
>>
>> A very bad use of indentation. In future please split logically rather 
>> than just filling the line up to the line width.
>>
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>> +                            mmio + base + sblk->csc_blk.base, "%s_%s",
>>> +                            sspp_block->name, sblk->csc_blk.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_DSPP:
>>> +    {
>>> +        struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
>>> +
>>> +        base = dspp_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, dspp_block->len, 
>>> mmio + base, "%s",
>>> +                        dspp_block->name);
>>> +
>>> +        if (dspp_block->features & BIT(DPU_DSPP_PCC))
>>> +            msm_disp_snapshot_add_block(disp_state, 
>>> dspp_block->sblk->pcc.len,
>>> +                            mmio + base + dspp_block->sblk->pcc.base,
>>> +                            "%s_%s", dspp_block->name,
>>> +                            dspp_block->sblk->pcc.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_PINGPONG:
>>> +    {
>>> +        struct dpu_pingpong_cfg *pingpong_block = (struct 
>>> dpu_pingpong_cfg *)blk;
>>> +        const struct dpu_pingpong_sub_blks *sblk = 
>>> pingpong_block->sblk;
>>> +
>>> +        base = pingpong_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, pingpong_block->len, 
>>> mmio + base, "%s",
>>> +                        pingpong_block->name);
>>> +
>>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
>>> +                            mmio + base + sblk->te2.base, "%s_%s",
>>> +                            pingpong_block->name, sblk->te2.name);
>>> +
>>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
>>> +                            mmio + base + sblk->dither.base, "%s_%s",
>>> +                            pingpong_block->name, sblk->dither.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_DSC:
>>> +    {
>>> +        struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
>>> +
>>> +        base = dsc_block->base;
>>> +
>>> +        if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
>>> +            struct dpu_dsc_blk enc = dsc_block->sblk->enc;
>>> +            struct dpu_dsc_blk ctl = dsc_block->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.
>>
>> Nice catch, thank you. We should fix that.
>>
> 
> Yes and we would have fixed that ourself if you wanted that with this 
> series as another patch.
> 

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

[Dmitry Baryshkov]

On 24/06/2023 04:23, Jessica Zhang wrote:
> 
> 
> On 6/23/2023 5:09 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>> Currently, the device core dump mechanism does not dump registers of 
>>>> sub
>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>> function to dump hardware blocks that contain sub blocks.
>>>>
>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>> +++++++++++++++++++++++++++-----
>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>>> *kms)
>>>>       return 0;
>>>>   }
>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>> *disp_state,
>>>> +                       void __iomem *mmio, void *blk,
>>>> +                       enum dpu_hw_blk_type blk_type)
>>>
>>> No. Such multiplexers add no value to the code. Please inline it.
>>>
>>> Not to mention that this patch is hard to review. You both move 
>>> existing code and add new features. If it were to go, it should have 
>>> been split into two patches: one introducing the multiplexer and 
>>> another one adding subblocks.
>>>
>>
>> Ok. we can split this into:
>>
>> 1) adding the multiplexer
>> 2) adding sub-blk parsing support inside the multiplexer
>>
>>>> +{
>>>> +    u32 base;
>>>> +
>>>> +    switch (blk_type) {
>>>> +    case DPU_HW_BLK_TOP:
>>>> +    {
>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>> +
>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>> +                            mmio + top->base, "top");
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>> MDP_PERIPH_TOP0_END,
>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>> +                            "top_2");
>>>> +        } else {
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio 
>>>> + top->base, "top");
>>>> +        }
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_LM:
>>>> +    {
>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>>> mixer->base, "%s",
>>>> +                        mixer->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_CTL:
>>>> +    {
>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>> ctl->base, "%s",
>>>> +                        ctl->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_INTF:
>>>> +    {
>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>>> intf->base, "%s",
>>>> +                        intf->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_WB:
>>>> +    {
>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>> wb->base, "%s",
>>>> +                        wb->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_SSPP:
>>>> +    {
>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>> +
>>>> +        base = sspp_block->base;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>> mmio + base, "%s",
>>>> +                        sspp_block->name);
>>>> +
>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>> sblk->scaler_blk.len,
>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>> "%s_%s",
>>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>>
>>> Actually, it would be better to:
>>> - drop name from all sblk instances (and use known string instead of 
>>> the sblk name here)
> 
> Hey Dmitry,
> 
> FWIW, I second Abhinav's points about the sblk names. For example, if in 
> the future we want to add a "_rot" suffix specifically to the 
> VIG_SBLK_ROT.scaler name, it would be easier to just make that change in 
> the HW catalog.

But why? The scaler is the same qseed3 scaler. We do not dump features, 
they are constant for the platform in question.

> 
>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>  From my understanding, your suggestion is to replace the feature flag 
> checks with a sblk.len > 0 check.
> 
> I don't think that would be good because it wouldn't be correct to 
> assume that the sblk will always be present. For example, for 
> DPU_HW_BLK_DSC, the sblks will only be present for DSC_BLK_1_2.

I don't consider sub-block as being always present. But if it present, 
it has non-zero length. If its length is zero, we have nothing to dump 
for it.

> In addition, it is possible for sblks, like pp_sblk_te.te2, to have a 
> len of 0. While the register space of that specific sblk will not be 
> printed, I'd prefer the devcore dump to reflect what is present within 
> the HW catalog so that the user knows which pingpong blks have the TE2 
> sblk.

I'd consider this as dumping the feature instead of dumping the 
registers. If you think it is necessary to ease decoding of the dump, 
consider adding block.features to the dump instead.

> 
> Thanks,
> 
> Jessica Zhang
> 
>>>
>>
>> No, I dont agree. If we drop the names from the sub_blk in the 
>> catalog, we will end up using "sub_blk_name" string here in the code 
>> to indicate which blk that is in the dump.
>>
>> If we add more sub_blks in the catalog in the future we need to keep 
>> changing the code over here. Thats not how it should be.
>>
>> Leaving the names in the catalog ensures that this code wont change 
>> and only catalog changes when we add a new sub_blk either for an 
>> existing or new chipset.
>>
>> catalog is indicating the new blk, and dumping code just prints it.
>>
>> with your approach, dumping code will or can keep changing with 
>> chipsets or sub_blks. Thats not how it should be.
>>

-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 24/06/2023 03:09, Abhinav Kumar wrote:
> 
> 
> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>> On 23/06/2023 02:48, Ryan McCann wrote:
>>> Currently, the device core dump mechanism does not dump registers of sub
>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>> function to dump hardware blocks that contain sub blocks.
>>>
>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>> ---
>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>> +++++++++++++++++++++++++++-----
>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index aa8499de1b9f..9b1b1c382269 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>> *kms)
>>>       return 0;
>>>   }
>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>> *disp_state,
>>> +                       void __iomem *mmio, void *blk,
>>> +                       enum dpu_hw_blk_type blk_type)
>>
>> No. Such multiplexers add no value to the code. Please inline it.
>>
>> Not to mention that this patch is hard to review. You both move 
>> existing code and add new features. If it were to go, it should have 
>> been split into two patches: one introducing the multiplexer and 
>> another one adding subblocks.
>>
> 
> Ok. we can split this into:
> 
> 1) adding the multiplexer
> 2) adding sub-blk parsing support inside the multiplexer

I'd say, drop the multiplexer completely. It adds no value here. It is 
only used from dpu_kms_mdp_snapshot(). If the code there was complex 
enough, it would have made sense to _split_ the function. But even in 
such case there would be no point in having multiplexer. We do not 
enumerate block by type.

> 
>>> +{
>>> +    u32 base;
>>> +
>>> +    switch (blk_type) {
>>> +    case DPU_HW_BLK_TOP:
>>> +    {
>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>> +
>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>> +                            mmio + top->base, "top");
>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>> MDP_PERIPH_TOP0_END,
>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>> +                            "top_2");
>>> +        } else {
>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio + 
>>> top->base, "top");
>>> +        }
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_LM:
>>> +    {
>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>> mixer->base, "%s",
>>> +                        mixer->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_CTL:
>>> +    {
>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>> ctl->base, "%s",
>>> +                        ctl->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_INTF:
>>> +    {
>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>> intf->base, "%s",
>>> +                        intf->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_WB:
>>> +    {
>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>> wb->base, "%s",
>>> +                        wb->name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_SSPP:
>>> +    {
>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>> +
>>> +        base = sspp_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>> mmio + base, "%s",
>>> +                        sspp_block->name);
>>> +
>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>> +            msm_disp_snapshot_add_block(disp_state, 
>>> sblk->scaler_blk.len,
>>> +                            mmio + base + sblk->scaler_blk.base, 
>>> "%s_%s",
>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>
>> Actually, it would be better to:
>> - drop name from all sblk instances (and use known string instead of 
>> the sblk name here)
>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>
> 
> No, I dont agree. If we drop the names from the sub_blk in the catalog, 
> we will end up using "sub_blk_name" string here in the code to indicate 
> which blk that is in the dump.
> 
> If we add more sub_blks in the catalog in the future we need to keep 
> changing the code over here. Thats not how it should be.
> 
> Leaving the names in the catalog ensures that this code wont change and 
> only catalog changes when we add a new sub_blk either for an existing or 
> new chipset.
> 
> catalog is indicating the new blk, and dumping code just prints it.
> 
> with your approach, dumping code will or can keep changing with chipsets 
> or sub_blks. Thats not how it should be.

Well, we do not enumerate sub-blocks in any way, they are not indexed. 
So even with sblk->blk.name in place, adding new sub-block would require 
adding new code here. That's why I wrote that the calling code knows 
which sub-block it refers to.

Let me extract the relevant code (skipping all the conditions for now):

msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, "%s",
			    sspp_block->name);

if (have_scaler)
	msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
				    mmio + base + sblk->scaler_blk.base, "%s_%s",
				    sspp_block->name, sblk->scaler_blk.name);

if (have_csc)
	msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
				    mmio + base + sblk->csc_blk.base, "%s_%s",
				    sspp_block->name, sblk->csc_blk.name);

Consider adding new sub-block, "baz". We would still require manual 
addition of the following code:

	msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
				    mmio + base + sblk->baz_blk.base, "%s_%s",
				    sspp_block->name, sblk->baz_blk.name);


Compare this with:

	msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
				    mmio + base + sblk->baz_blk.base, "%s_baz",
				    sspp_block->name);

Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which 
doesn't use name), it should be:

	msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
				    mmio + base + sblk->baz_blk.base, "sspp%d_baz", idx);



> 
>>> +
>>> +        if (sspp_block->features & BIT(DPU_SSPP_CSC) || 
>>> sspp_block->features
>>> +                    & BIT(DPU_SSPP_CSC_10BIT))
>>
>> A very bad use of indentation. In future please split logically rather 
>> than just filling the line up to the line width.
>>
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>> +                            mmio + base + sblk->csc_blk.base, "%s_%s",
>>> +                            sspp_block->name, sblk->csc_blk.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_DSPP:
>>> +    {
>>> +        struct dpu_dspp_cfg *dspp_block = (struct dpu_dspp_cfg *)blk;
>>> +
>>> +        base = dspp_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, dspp_block->len, 
>>> mmio + base, "%s",
>>> +                        dspp_block->name);
>>> +
>>> +        if (dspp_block->features & BIT(DPU_DSPP_PCC))
>>> +            msm_disp_snapshot_add_block(disp_state, 
>>> dspp_block->sblk->pcc.len,
>>> +                            mmio + base + dspp_block->sblk->pcc.base,
>>> +                            "%s_%s", dspp_block->name,
>>> +                            dspp_block->sblk->pcc.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_PINGPONG:
>>> +    {
>>> +        struct dpu_pingpong_cfg *pingpong_block = (struct 
>>> dpu_pingpong_cfg *)blk;
>>> +        const struct dpu_pingpong_sub_blks *sblk = 
>>> pingpong_block->sblk;
>>> +
>>> +        base = pingpong_block->base;
>>> +
>>> +        msm_disp_snapshot_add_block(disp_state, pingpong_block->len, 
>>> mmio + base, "%s",
>>> +                        pingpong_block->name);
>>> +
>>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_TE2))
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->te2.len,
>>> +                            mmio + base + sblk->te2.base, "%s_%s",
>>> +                            pingpong_block->name, sblk->te2.name);
>>> +
>>> +        if (pingpong_block->features & BIT(DPU_PINGPONG_DITHER))
>>> +            msm_disp_snapshot_add_block(disp_state, sblk->dither.len,
>>> +                            mmio + base + sblk->dither.base, "%s_%s",
>>> +                            pingpong_block->name, sblk->dither.name);
>>> +        break;
>>> +    }
>>> +    case DPU_HW_BLK_DSC:
>>> +    {
>>> +        struct dpu_dsc_cfg *dsc_block = (struct dpu_dsc_cfg *)blk;
>>> +
>>> +        base = dsc_block->base;
>>> +
>>> +        if (dsc_block->features & BIT(DPU_DSC_HW_REV_1_2)) {
>>> +            struct dpu_dsc_blk enc = dsc_block->sblk->enc;
>>> +            struct dpu_dsc_blk ctl = dsc_block->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.
>>
>> Nice catch, thank you. We should fix that.
>>
> 
> Yes and we would have fixed that ourself if you wanted that with this 
> series as another patch.

This is not relevant to this series, so it should be fixed in a separate 
series.

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
> On 24/06/2023 03:09, Abhinav Kumar wrote:
>>
>>
>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>> Currently, the device core dump mechanism does not dump registers of 
>>>> sub
>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>> function to dump hardware blocks that contain sub blocks.
>>>>
>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>> ---
>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>> +++++++++++++++++++++++++++-----
>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>>> *kms)
>>>>       return 0;
>>>>   }
>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>> *disp_state,
>>>> +                       void __iomem *mmio, void *blk,
>>>> +                       enum dpu_hw_blk_type blk_type)
>>>
>>> No. Such multiplexers add no value to the code. Please inline it.
>>>
>>> Not to mention that this patch is hard to review. You both move 
>>> existing code and add new features. If it were to go, it should have 
>>> been split into two patches: one introducing the multiplexer and 
>>> another one adding subblocks.
>>>
>>
>> Ok. we can split this into:
>>
>> 1) adding the multiplexer
>> 2) adding sub-blk parsing support inside the multiplexer
> 
> I'd say, drop the multiplexer completely. It adds no value here. It is 
> only used from dpu_kms_mdp_snapshot(). If the code there was complex 
> enough, it would have made sense to _split_ the function. But even in 
> such case there would be no point in having multiplexer. We do not 
> enumerate block by type.
> 

Can you pls elaborate what you mean by enumerate blk by type?

We do have DPU_HW_BLK_***

Did you mean sub-blk?

>>
>>>> +{
>>>> +    u32 base;
>>>> +
>>>> +    switch (blk_type) {
>>>> +    case DPU_HW_BLK_TOP:
>>>> +    {
>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>> +
>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>> +                            mmio + top->base, "top");
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>> MDP_PERIPH_TOP0_END,
>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>> +                            "top_2");
>>>> +        } else {
>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio 
>>>> + top->base, "top");
>>>> +        }
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_LM:
>>>> +    {
>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>>> mixer->base, "%s",
>>>> +                        mixer->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_CTL:
>>>> +    {
>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>> ctl->base, "%s",
>>>> +                        ctl->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_INTF:
>>>> +    {
>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>>> intf->base, "%s",
>>>> +                        intf->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_WB:
>>>> +    {
>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>> wb->base, "%s",
>>>> +                        wb->name);
>>>> +        break;
>>>> +    }
>>>> +    case DPU_HW_BLK_SSPP:
>>>> +    {
>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>> +
>>>> +        base = sspp_block->base;
>>>> +
>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>> mmio + base, "%s",
>>>> +                        sspp_block->name);
>>>> +
>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>> sblk->scaler_blk.len,
>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>> "%s_%s",
>>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>>
>>> Actually, it would be better to:
>>> - drop name from all sblk instances (and use known string instead of 
>>> the sblk name here)
>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>
>>
>> No, I dont agree. If we drop the names from the sub_blk in the 
>> catalog, we will end up using "sub_blk_name" string here in the code 
>> to indicate which blk that is in the dump.
>>
>> If we add more sub_blks in the catalog in the future we need to keep 
>> changing the code over here. Thats not how it should be.
>>
>> Leaving the names in the catalog ensures that this code wont change 
>> and only catalog changes when we add a new sub_blk either for an 
>> existing or new chipset.
>>
>> catalog is indicating the new blk, and dumping code just prints it.
>>
>> with your approach, dumping code will or can keep changing with 
>> chipsets or sub_blks. Thats not how it should be.
> 
> Well, we do not enumerate sub-blocks in any way, they are not indexed. 
> So even with sblk->blk.name in place, adding new sub-block would require 
> adding new code here. That's why I wrote that the calling code knows 
> which sub-block it refers to.
> 

Today, unfortunately each sub_blk type is different so we have to do 
this case by case.

Ideally, this should have just been

-> print main blk
-> print all sub-blks of the main blk

Without having to handle each main blk's sub-blks separately.

That way the dumping code would have remained generic without having to 
do even the multiplexer in the first place.

Need to explore if somehow we can come up with a generic sub-blk struct 
and make this possible. Then this code will become much easier and what 
I am saying will make total sense.

Even without that, conceptually these sub-blk names are reflecting whats 
in our software document. So its not a random name but reflects the 
actual sub-blk name from the hardware. So this belongs in the catalog.

Dumping code should not change or know whats the name of each block. It 
should just use whats in the catalog. thats why even conceptually I am 
not okay with your idea.

> Let me extract the relevant code (skipping all the conditions for now):
> 
> msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, "%s",
>                  sspp_block->name);
> 
> if (have_scaler)
>      msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
>                      mmio + base + sblk->scaler_blk.base, "%s_%s",
>                      sspp_block->name, sblk->scaler_blk.name);
> 
> if (have_csc)
>      msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>                      mmio + base + sblk->csc_blk.base, "%s_%s",
>                      sspp_block->name, sblk->csc_blk.name);
> 
> Consider adding new sub-block, "baz". We would still require manual 
> addition of the following code:
> 
>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>                      mmio + base + sblk->baz_blk.base, "%s_%s",
>                      sspp_block->name, sblk->baz_blk.name);
> 
> 
> Compare this with:
> 
>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>                      mmio + base + sblk->baz_blk.base, "%s_baz",
>                      sspp_block->name);
> 

Basically you are saying why not make the one line change here instead 
of using the name from the catalog.

I think it will be better to use from the catalog for the reason I wrote 
above that dumping code should just "use" the catalog's information and 
not become a catalog itself.

You are not saving much by dropping the sub-blk name from catalog anyway.

> Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which 
> doesn't use name), it should be:
> 
>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>                      mmio + base + sblk->baz_blk.base, "sspp%d_baz", idx);
> 
> 
tbh, after looking at this series, it made me think why I didnt use the 
name from the catalog even for the dpu_kms_mdp_snapshot()
> 
>>

<snipped>

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

[Dmitry Baryshkov]

On 24/06/2023 17:17, Abhinav Kumar wrote:
> 
> 
> On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
>> On 24/06/2023 03:09, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>>> Currently, the device core dump mechanism does not dump registers 
>>>>> of sub
>>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>>> function to dump hardware blocks that contain sub blocks.
>>>>>
>>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>>> ---
>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>>> +++++++++++++++++++++++++++-----
>>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms 
>>>>> *kms)
>>>>>       return 0;
>>>>>   }
>>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>>> *disp_state,
>>>>> +                       void __iomem *mmio, void *blk,
>>>>> +                       enum dpu_hw_blk_type blk_type)
>>>>
>>>> No. Such multiplexers add no value to the code. Please inline it.
>>>>
>>>> Not to mention that this patch is hard to review. You both move 
>>>> existing code and add new features. If it were to go, it should have 
>>>> been split into two patches: one introducing the multiplexer and 
>>>> another one adding subblocks.
>>>>
>>>
>>> Ok. we can split this into:
>>>
>>> 1) adding the multiplexer
>>> 2) adding sub-blk parsing support inside the multiplexer
>>
>> I'd say, drop the multiplexer completely. It adds no value here. It is 
>> only used from dpu_kms_mdp_snapshot(). If the code there was complex 
>> enough, it would have made sense to _split_ the function. But even in 
>> such case there would be no point in having multiplexer. We do not 
>> enumerate block by type.
>>
> 
> Can you pls elaborate what you mean by enumerate blk by type?
> 
> We do have DPU_HW_BLK_***
> 
> Did you mean sub-blk?
> 
>>>
>>>>> +{
>>>>> +    u32 base;
>>>>> +
>>>>> +    switch (blk_type) {
>>>>> +    case DPU_HW_BLK_TOP:
>>>>> +    {
>>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>>> +
>>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>>> +                            mmio + top->base, "top");
>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>>> MDP_PERIPH_TOP0_END,
>>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>>> +                            "top_2");
>>>>> +        } else {
>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, mmio 
>>>>> + top->base, "top");
>>>>> +        }
>>>>> +        break;
>>>>> +    }
>>>>> +    case DPU_HW_BLK_LM:
>>>>> +    {
>>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>>> +
>>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio + 
>>>>> mixer->base, "%s",
>>>>> +                        mixer->name);
>>>>> +        break;
>>>>> +    }
>>>>> +    case DPU_HW_BLK_CTL:
>>>>> +    {
>>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>>> +
>>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>>> ctl->base, "%s",
>>>>> +                        ctl->name);
>>>>> +        break;
>>>>> +    }
>>>>> +    case DPU_HW_BLK_INTF:
>>>>> +    {
>>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>>> +
>>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>>>> intf->base, "%s",
>>>>> +                        intf->name);
>>>>> +        break;
>>>>> +    }
>>>>> +    case DPU_HW_BLK_WB:
>>>>> +    {
>>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>>> +
>>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>>> wb->base, "%s",
>>>>> +                        wb->name);
>>>>> +        break;
>>>>> +    }
>>>>> +    case DPU_HW_BLK_SSPP:
>>>>> +    {
>>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg *)blk;
>>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>>> +
>>>>> +        base = sspp_block->base;
>>>>> +
>>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>>> mmio + base, "%s",
>>>>> +                        sspp_block->name);
>>>>> +
>>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>> sblk->scaler_blk.len,
>>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>>> "%s_%s",
>>>>> +                            sspp_block->name, sblk->scaler_blk.name);
>>>>
>>>> Actually, it would be better to:
>>>> - drop name from all sblk instances (and use known string instead of 
>>>> the sblk name here)
>>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>>
>>>
>>> No, I dont agree. If we drop the names from the sub_blk in the 
>>> catalog, we will end up using "sub_blk_name" string here in the code 
>>> to indicate which blk that is in the dump.
>>>
>>> If we add more sub_blks in the catalog in the future we need to keep 
>>> changing the code over here. Thats not how it should be.
>>>
>>> Leaving the names in the catalog ensures that this code wont change 
>>> and only catalog changes when we add a new sub_blk either for an 
>>> existing or new chipset.
>>>
>>> catalog is indicating the new blk, and dumping code just prints it.
>>>
>>> with your approach, dumping code will or can keep changing with 
>>> chipsets or sub_blks. Thats not how it should be.
>>
>> Well, we do not enumerate sub-blocks in any way, they are not indexed. 
>> So even with sblk->blk.name in place, adding new sub-block would 
>> require adding new code here. That's why I wrote that the calling code 
>> knows which sub-block it refers to.
>>
> 
> Today, unfortunately each sub_blk type is different so we have to do 
> this case by case.
> 
> Ideally, this should have just been
> 
> -> print main blk
> -> print all sub-blks of the main blk
> 
> Without having to handle each main blk's sub-blks separately.
> 
> That way the dumping code would have remained generic without having to 
> do even the multiplexer in the first place.
> 
> Need to explore if somehow we can come up with a generic sub-blk struct 
> and make this possible. Then this code will become much easier and what 
> I am saying will make total sense.

In such case, yes. However I'd warn about having a generic array of 
subblocks. Having named subblock entries might complicate snapshotting, 
but it makes the rest of the DPU driver smaller.

> 
> Even without that, conceptually these sub-blk names are reflecting whats 
> in our software document. So its not a random name but reflects the 
> actual sub-blk name from the hardware.

Yes

> So this belongs in the catalog.

But the sub-block field already has a correct name: scaler_blk, csc_blk, 
etc. Having both sub-block field name and the .name inside results in 
kind of duplication, which seems unnecessary to me.

> Dumping code should not change or know whats the name of each block. It 
> should just use whats in the catalog. thats why even conceptually I am 
> not okay with your idea.

Dumping code itself (msm_disp_snapshot_*) doesn't. But the caller code 
knows what is the subblock.

Let me pick a definition from the patch:

static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
	.pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
		.len = 0x90, .version = 0x10007},
};

the "pcc" is repeated three times. When the code looks at this block, it 
already knows that it is a PCC block.

Compare this with:

static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
	.pcc = {
		.base = 0x1700,
		.len = 0x90,
		.version = 0x10007,
	},
};

Nothing is repeated, but we still know that this is the DSPPn_PCC 
sub-block description.

Calling code does:

u32 base;
base = ctx->cap->sblk->pcc.base;


> 
>> Let me extract the relevant code (skipping all the conditions for now):
>>
>> msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, 
>> "%s",
>>                  sspp_block->name);
>>
>> if (have_scaler)
>>      msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
>>                      mmio + base + sblk->scaler_blk.base, "%s_%s",
>>                      sspp_block->name, sblk->scaler_blk.name);
>>
>> if (have_csc)
>>      msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>                      mmio + base + sblk->csc_blk.base, "%s_%s",
>>                      sspp_block->name, sblk->csc_blk.name);
>>
>> Consider adding new sub-block, "baz". We would still require manual 
>> addition of the following code:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "%s_%s",
>>                      sspp_block->name, sblk->baz_blk.name);
>>
>>
>> Compare this with:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "%s_baz",
>>                      sspp_block->name);
>>
> 
> Basically you are saying why not make the one line change here instead 
> of using the name from the catalog.
> 
> I think it will be better to use from the catalog for the reason I wrote 
> above that dumping code should just "use" the catalog's information and 
> not become a catalog itself.
> 
> You are not saving much by dropping the sub-blk name from catalog anyway.
> 
>> Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which 
>> doesn't use name), it should be:
>>
>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>                      mmio + base + sblk->baz_blk.base, "sspp%d_baz", 
>> idx);
>>
>>
> tbh, after looking at this series, it made me think why I didnt use the 
> name from the catalog even for the dpu_kms_mdp_snapshot()
>>
>>>
> 
> <snipped>

-- 
With best wishes
Dmitry

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

[Abhinav Kumar]

On 6/24/2023 8:03 AM, Dmitry Baryshkov wrote:
> On 24/06/2023 17:17, Abhinav Kumar wrote:
>>
>>
>> On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
>>> On 24/06/2023 03:09, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>>>> Currently, the device core dump mechanism does not dump registers 
>>>>>> of sub
>>>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>>>> function to dump hardware blocks that contain sub blocks.
>>>>>>
>>>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>>>> ---
>>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>>>> +++++++++++++++++++++++++++-----
>>>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>>>
>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct 
>>>>>> msm_kms *kms)
>>>>>>       return 0;
>>>>>>   }
>>>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>>>> *disp_state,
>>>>>> +                       void __iomem *mmio, void *blk,
>>>>>> +                       enum dpu_hw_blk_type blk_type)
>>>>>
>>>>> No. Such multiplexers add no value to the code. Please inline it.
>>>>>
>>>>> Not to mention that this patch is hard to review. You both move 
>>>>> existing code and add new features. If it were to go, it should 
>>>>> have been split into two patches: one introducing the multiplexer 
>>>>> and another one adding subblocks.
>>>>>
>>>>
>>>> Ok. we can split this into:
>>>>
>>>> 1) adding the multiplexer
>>>> 2) adding sub-blk parsing support inside the multiplexer
>>>
>>> I'd say, drop the multiplexer completely. It adds no value here. It 
>>> is only used from dpu_kms_mdp_snapshot(). If the code there was 
>>> complex enough, it would have made sense to _split_ the function. But 
>>> even in such case there would be no point in having multiplexer. We 
>>> do not enumerate block by type.
>>>
>>
>> Can you pls elaborate what you mean by enumerate blk by type?
>>
>> We do have DPU_HW_BLK_***
>>
>> Did you mean sub-blk?
>>
>>>>
>>>>>> +{
>>>>>> +    u32 base;
>>>>>> +
>>>>>> +    switch (blk_type) {
>>>>>> +    case DPU_HW_BLK_TOP:
>>>>>> +    {
>>>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>>>> +
>>>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>>> +            msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>>>> +                            mmio + top->base, "top");
>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>>>> MDP_PERIPH_TOP0_END,
>>>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>>>> +                            "top_2");
>>>>>> +        } else {
>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, 
>>>>>> mmio + top->base, "top");
>>>>>> +        }
>>>>>> +        break;
>>>>>> +    }
>>>>>> +    case DPU_HW_BLK_LM:
>>>>>> +    {
>>>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>>>> +
>>>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio 
>>>>>> + mixer->base, "%s",
>>>>>> +                        mixer->name);
>>>>>> +        break;
>>>>>> +    }
>>>>>> +    case DPU_HW_BLK_CTL:
>>>>>> +    {
>>>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>>>> +
>>>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>>>> ctl->base, "%s",
>>>>>> +                        ctl->name);
>>>>>> +        break;
>>>>>> +    }
>>>>>> +    case DPU_HW_BLK_INTF:
>>>>>> +    {
>>>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>>>> +
>>>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio + 
>>>>>> intf->base, "%s",
>>>>>> +                        intf->name);
>>>>>> +        break;
>>>>>> +    }
>>>>>> +    case DPU_HW_BLK_WB:
>>>>>> +    {
>>>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>>>> +
>>>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>>>> wb->base, "%s",
>>>>>> +                        wb->name);
>>>>>> +        break;
>>>>>> +    }
>>>>>> +    case DPU_HW_BLK_SSPP:
>>>>>> +    {
>>>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg 
>>>>>> *)blk;
>>>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>>>> +
>>>>>> +        base = sspp_block->base;
>>>>>> +
>>>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>>>> mmio + base, "%s",
>>>>>> +                        sspp_block->name);
>>>>>> +
>>>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>>>> +            sspp_block->features & 
>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>>> sblk->scaler_blk.len,
>>>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>>>> "%s_%s",
>>>>>> +                            sspp_block->name, 
>>>>>> sblk->scaler_blk.name);
>>>>>
>>>>> Actually, it would be better to:
>>>>> - drop name from all sblk instances (and use known string instead 
>>>>> of the sblk name here)
>>>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>>>
>>>>
>>>> No, I dont agree. If we drop the names from the sub_blk in the 
>>>> catalog, we will end up using "sub_blk_name" string here in the code 
>>>> to indicate which blk that is in the dump.
>>>>
>>>> If we add more sub_blks in the catalog in the future we need to keep 
>>>> changing the code over here. Thats not how it should be.
>>>>
>>>> Leaving the names in the catalog ensures that this code wont change 
>>>> and only catalog changes when we add a new sub_blk either for an 
>>>> existing or new chipset.
>>>>
>>>> catalog is indicating the new blk, and dumping code just prints it.
>>>>
>>>> with your approach, dumping code will or can keep changing with 
>>>> chipsets or sub_blks. Thats not how it should be.
>>>
>>> Well, we do not enumerate sub-blocks in any way, they are not 
>>> indexed. So even with sblk->blk.name in place, adding new sub-block 
>>> would require adding new code here. That's why I wrote that the 
>>> calling code knows which sub-block it refers to.
>>>
>>
>> Today, unfortunately each sub_blk type is different so we have to do 
>> this case by case.
>>
>> Ideally, this should have just been
>>
>> -> print main blk
>> -> print all sub-blks of the main blk
>>
>> Without having to handle each main blk's sub-blks separately.
>>
>> That way the dumping code would have remained generic without having 
>> to do even the multiplexer in the first place.
>>
>> Need to explore if somehow we can come up with a generic sub-blk 
>> struct and make this possible. Then this code will become much easier 
>> and what I am saying will make total sense.
> 
> In such case, yes. However I'd warn about having a generic array of 
> subblocks. Having named subblock entries might complicate snapshotting, 
> but it makes the rest of the DPU driver smaller.
> 

Need to explore this. But not immediately.

>>
>> Even without that, conceptually these sub-blk names are reflecting 
>> whats in our software document. So its not a random name but reflects 
>> the actual sub-blk name from the hardware.
> 
> Yes
> 
>> So this belongs in the catalog.
> 
> But the sub-block field already has a correct name: scaler_blk, csc_blk, 
> etc. Having both sub-block field name and the .name inside results in 
> kind of duplication, which seems unnecessary to me.
> 

No, there is a difference and not duplicated. One is the name of the 
struct so it can really be anything and doesnt need to match the hw doc 
name. But the other is the string name which we can give exactly to 
match software interface doc and makes parsing such a dump much much easier.

One point I dont see you have considered is the block index of the sub_blk.

Today, yes I see only a "pcc" or a "dither" etc

What if there are two PCCs or two dithers.

Then their names can just be "pcc_0" and "pcc_1" or "dither_0" and 
"dither_1".

Having name gives us the ability to easily incorporate even unsequential 
indices.

For example, every sspp's name today is not sequential. it can be 
"sspp_3" then "sspp_8" etc

By having names reflect the correct indices, dumping code becomes less 
complex as the catalog will still have the right names as dumping code 
will just use that.


>> Dumping code should not change or know whats the name of each block. 
>> It should just use whats in the catalog. thats why even conceptually I 
>> am not okay with your idea.
> 
> Dumping code itself (msm_disp_snapshot_*) doesn't. But the caller code 
> knows what is the subblock.
> 

But this is the dumping code because we are adding which blocks to dump.

> Let me pick a definition from the patch:
> 
> static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
>      .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
>          .len = 0x90, .version = 0x10007},
> };
> 
> the "pcc" is repeated three times. When the code looks at this block, it 
> already knows that it is a PCC block.
> 

I agree with you about the "id" though. From what I can see in this 
patch, "id" is not used and we can drop that. So that change from your 
series is fine with me.

But the pcc is the struct name which doesnt really need to reflect the 
hw name but the name field of the PCC should IMO.

> Compare this with:
> 
> static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
>      .pcc = {
>          .base = 0x1700,
>          .len = 0x90,
>          .version = 0x10007,
>      },
> };
> 
> Nothing is repeated, but we still know that this is the DSPPn_PCC 
> sub-block description.
> 
> Calling code does:
> 
> u32 base;
> base = ctx->cap->sblk->pcc.base;
> 
> 
>>
>>> Let me extract the relevant code (skipping all the conditions for now):
>>>
>>> msm_disp_snapshot_add_block(disp_state, sspp_block->len, mmio + base, 
>>> "%s",
>>>                  sspp_block->name);
>>>
>>> if (have_scaler)
>>>      msm_disp_snapshot_add_block(disp_state, sblk->scaler_blk.len,
>>>                      mmio + base + sblk->scaler_blk.base, "%s_%s",
>>>                      sspp_block->name, sblk->scaler_blk.name);
>>>
>>> if (have_csc)
>>>      msm_disp_snapshot_add_block(disp_state, sblk->csc_blk.len,
>>>                      mmio + base + sblk->csc_blk.base, "%s_%s",
>>>                      sspp_block->name, sblk->csc_blk.name);
>>>
>>> Consider adding new sub-block, "baz". We would still require manual 
>>> addition of the following code:
>>>
>>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>>                      mmio + base + sblk->baz_blk.base, "%s_%s",
>>>                      sspp_block->name, sblk->baz_blk.name);
>>>
>>>
>>> Compare this with:
>>>
>>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>>                      mmio + base + sblk->baz_blk.base, "%s_baz",
>>>                      sspp_block->name);
>>>
>>
>> Basically you are saying why not make the one line change here instead 
>> of using the name from the catalog.
>>
>> I think it will be better to use from the catalog for the reason I 
>> wrote above that dumping code should just "use" the catalog's 
>> information and not become a catalog itself.
>>
>> You are not saving much by dropping the sub-blk name from catalog anyway.
>>
>>> Moreover, if we follow the style of dpu_kms_mdp_snapshot() (which 
>>> doesn't use name), it should be:
>>>
>>>      msm_disp_snapshot_add_block(disp_state, sblk->baz_blk.len,
>>>                      mmio + base + sblk->baz_blk.base, "sspp%d_baz", 
>>> idx);
>>>
>>>
>> tbh, after looking at this series, it made me think why I didnt use 
>> the name from the catalog even for the dpu_kms_mdp_snapshot()
>>>
>>>>
>>
>> <snipped>
> 

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

[Abhinav Kumar]

On 6/24/2023 7:44 PM, Abhinav Kumar wrote:
> 
> 
> On 6/24/2023 8:03 AM, Dmitry Baryshkov wrote:
>> On 24/06/2023 17:17, Abhinav Kumar wrote:
>>>
>>>
>>> On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
>>>> On 24/06/2023 03:09, Abhinav Kumar wrote:
>>>>>
>>>>>
>>>>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>>>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>>>>> Currently, the device core dump mechanism does not dump registers 
>>>>>>> of sub
>>>>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>>>>> function to dump hardware blocks that contain sub blocks.
>>>>>>>
>>>>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>>>>> ---
>>>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>>>>> +++++++++++++++++++++++++++-----
>>>>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>>>>
>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct 
>>>>>>> msm_kms *kms)
>>>>>>>       return 0;
>>>>>>>   }
>>>>>>> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state 
>>>>>>> *disp_state,
>>>>>>> +                       void __iomem *mmio, void *blk,
>>>>>>> +                       enum dpu_hw_blk_type blk_type)
>>>>>>
>>>>>> No. Such multiplexers add no value to the code. Please inline it.
>>>>>>
>>>>>> Not to mention that this patch is hard to review. You both move 
>>>>>> existing code and add new features. If it were to go, it should 
>>>>>> have been split into two patches: one introducing the multiplexer 
>>>>>> and another one adding subblocks.
>>>>>>
>>>>>
>>>>> Ok. we can split this into:
>>>>>
>>>>> 1) adding the multiplexer
>>>>> 2) adding sub-blk parsing support inside the multiplexer
>>>>
>>>> I'd say, drop the multiplexer completely. It adds no value here. It 
>>>> is only used from dpu_kms_mdp_snapshot(). If the code there was 
>>>> complex enough, it would have made sense to _split_ the function. 
>>>> But even in such case there would be no point in having multiplexer. 
>>>> We do not enumerate block by type.
>>>>
>>>
>>> Can you pls elaborate what you mean by enumerate blk by type?
>>>
>>> We do have DPU_HW_BLK_***
>>>
>>> Did you mean sub-blk?
>>>
>>>>>
>>>>>>> +{
>>>>>>> +    u32 base;
>>>>>>> +
>>>>>>> +    switch (blk_type) {
>>>>>>> +    case DPU_HW_BLK_TOP:
>>>>>>> +    {
>>>>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>>>>> +
>>>>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>>>> MDP_PERIPH_TOP0,
>>>>>>> +                            mmio + top->base, "top");
>>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>>>>> MDP_PERIPH_TOP0_END,
>>>>>>> +                            mmio + top->base + MDP_PERIPH_TOP0_END,
>>>>>>> +                            "top_2");
>>>>>>> +        } else {
>>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, 
>>>>>>> mmio + top->base, "top");
>>>>>>> +        }
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +    case DPU_HW_BLK_LM:
>>>>>>> +    {
>>>>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>>>>> +
>>>>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, mmio 
>>>>>>> + mixer->base, "%s",
>>>>>>> +                        mixer->name);
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +    case DPU_HW_BLK_CTL:
>>>>>>> +    {
>>>>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>>>>> +
>>>>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio + 
>>>>>>> ctl->base, "%s",
>>>>>>> +                        ctl->name);
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +    case DPU_HW_BLK_INTF:
>>>>>>> +    {
>>>>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>>>>> +
>>>>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio 
>>>>>>> + intf->base, "%s",
>>>>>>> +                        intf->name);
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +    case DPU_HW_BLK_WB:
>>>>>>> +    {
>>>>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>>>>> +
>>>>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>>>>> wb->base, "%s",
>>>>>>> +                        wb->name);
>>>>>>> +        break;
>>>>>>> +    }
>>>>>>> +    case DPU_HW_BLK_SSPP:
>>>>>>> +    {
>>>>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg 
>>>>>>> *)blk;
>>>>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>>>>> +
>>>>>>> +        base = sspp_block->base;
>>>>>>> +
>>>>>>> +        msm_disp_snapshot_add_block(disp_state, sspp_block->len, 
>>>>>>> mmio + base, "%s",
>>>>>>> +                        sspp_block->name);
>>>>>>> +
>>>>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>>>>> +            sspp_block->features & 
>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>>>> sblk->scaler_blk.len,
>>>>>>> +                            mmio + base + sblk->scaler_blk.base, 
>>>>>>> "%s_%s",
>>>>>>> +                            sspp_block->name, 
>>>>>>> sblk->scaler_blk.name);
>>>>>>
>>>>>> Actually, it would be better to:
>>>>>> - drop name from all sblk instances (and use known string instead 
>>>>>> of the sblk name here)
>>>>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>>>>
>>>>>
>>>>> No, I dont agree. If we drop the names from the sub_blk in the 
>>>>> catalog, we will end up using "sub_blk_name" string here in the 
>>>>> code to indicate which blk that is in the dump.
>>>>>
>>>>> If we add more sub_blks in the catalog in the future we need to 
>>>>> keep changing the code over here. Thats not how it should be.
>>>>>
>>>>> Leaving the names in the catalog ensures that this code wont change 
>>>>> and only catalog changes when we add a new sub_blk either for an 
>>>>> existing or new chipset.
>>>>>
>>>>> catalog is indicating the new blk, and dumping code just prints it.
>>>>>
>>>>> with your approach, dumping code will or can keep changing with 
>>>>> chipsets or sub_blks. Thats not how it should be.
>>>>
>>>> Well, we do not enumerate sub-blocks in any way, they are not 
>>>> indexed. So even with sblk->blk.name in place, adding new sub-block 
>>>> would require adding new code here. That's why I wrote that the 
>>>> calling code knows which sub-block it refers to.
>>>>
>>>
>>> Today, unfortunately each sub_blk type is different so we have to do 
>>> this case by case.
>>>
>>> Ideally, this should have just been
>>>
>>> -> print main blk
>>> -> print all sub-blks of the main blk
>>>
>>> Without having to handle each main blk's sub-blks separately.
>>>
>>> That way the dumping code would have remained generic without having 
>>> to do even the multiplexer in the first place.
>>>
>>> Need to explore if somehow we can come up with a generic sub-blk 
>>> struct and make this possible. Then this code will become much easier 
>>> and what I am saying will make total sense.
>>
>> In such case, yes. However I'd warn about having a generic array of 
>> subblocks. Having named subblock entries might complicate 
>> snapshotting, but it makes the rest of the DPU driver smaller.
>>
> 
> Need to explore this. But not immediately.
> 
>>>
>>> Even without that, conceptually these sub-blk names are reflecting 
>>> whats in our software document. So its not a random name but reflects 
>>> the actual sub-blk name from the hardware.
>>
>> Yes
>>
>>> So this belongs in the catalog.
>>
>> But the sub-block field already has a correct name: scaler_blk, 
>> csc_blk, etc. Having both sub-block field name and the .name inside 
>> results in kind of duplication, which seems unnecessary to me.
>>
> 
> No, there is a difference and not duplicated. One is the name of the 
> struct so it can really be anything and doesnt need to match the hw doc 
> name. But the other is the string name which we can give exactly to 
> match software interface doc and makes parsing such a dump much much 
> easier.
> 
> One point I dont see you have considered is the block index of the sub_blk.
> 
> Today, yes I see only a "pcc" or a "dither" etc
> 
> What if there are two PCCs or two dithers.
> 
> Then their names can just be "pcc_0" and "pcc_1" or "dither_0" and 
> "dither_1".
> 
> Having name gives us the ability to easily incorporate even unsequential 
> indices.
> 
> For example, every sspp's name today is not sequential. it can be 
> "sspp_3" then "sspp_8" etc
> 
> By having names reflect the correct indices, dumping code becomes less 
> complex as the catalog will still have the right names as dumping code 
> will just use that.
> 

The QC team is in agreement that we would like to go ahead with the 
names from the catalog and not drop them.

Hence we will post the next revision with the name still from the 
catalog and drop the multiplexer completely.

Since the intern has a short period of time to finish development on 
this task, we would like to go ahead with this approach and post the 
next rev.

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

[Dmitry Baryshkov]

On 30/06/2023 02:29, Abhinav Kumar wrote:
> 
> 
> On 6/24/2023 7:44 PM, Abhinav Kumar wrote:
>>
>>
>> On 6/24/2023 8:03 AM, Dmitry Baryshkov wrote:
>>> On 24/06/2023 17:17, Abhinav Kumar wrote:
>>>>
>>>>
>>>> On 6/24/2023 5:07 AM, Dmitry Baryshkov wrote:
>>>>> On 24/06/2023 03:09, Abhinav Kumar wrote:
>>>>>>
>>>>>>
>>>>>> On 6/22/2023 5:13 PM, Dmitry Baryshkov wrote:
>>>>>>> On 23/06/2023 02:48, Ryan McCann wrote:
>>>>>>>> Currently, the device core dump mechanism does not dump 
>>>>>>>> registers of sub
>>>>>>>> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
>>>>>>>> function to dump hardware blocks that contain sub blocks.
>>>>>>>>
>>>>>>>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
>>>>>>>> ---
>>>>>>>>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 
>>>>>>>> +++++++++++++++++++++++++++-----
>>>>>>>>   1 file changed, 168 insertions(+), 26 deletions(-)
>>>>>>>>
>>>>>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c 
>>>>>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>>> index aa8499de1b9f..9b1b1c382269 100644
>>>>>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>>>>>> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct 
>>>>>>>> msm_kms *kms)
>>>>>>>>       return 0;
>>>>>>>>   }
>>>>>>>> +static void dpu_kms_mdp_snapshot_add_block(struct 
>>>>>>>> msm_disp_state *disp_state,
>>>>>>>> +                       void __iomem *mmio, void *blk,
>>>>>>>> +                       enum dpu_hw_blk_type blk_type)
>>>>>>>
>>>>>>> No. Such multiplexers add no value to the code. Please inline it.
>>>>>>>
>>>>>>> Not to mention that this patch is hard to review. You both move 
>>>>>>> existing code and add new features. If it were to go, it should 
>>>>>>> have been split into two patches: one introducing the multiplexer 
>>>>>>> and another one adding subblocks.
>>>>>>>
>>>>>>
>>>>>> Ok. we can split this into:
>>>>>>
>>>>>> 1) adding the multiplexer
>>>>>> 2) adding sub-blk parsing support inside the multiplexer
>>>>>
>>>>> I'd say, drop the multiplexer completely. It adds no value here. It 
>>>>> is only used from dpu_kms_mdp_snapshot(). If the code there was 
>>>>> complex enough, it would have made sense to _split_ the function. 
>>>>> But even in such case there would be no point in having 
>>>>> multiplexer. We do not enumerate block by type.
>>>>>
>>>>
>>>> Can you pls elaborate what you mean by enumerate blk by type?
>>>>
>>>> We do have DPU_HW_BLK_***
>>>>
>>>> Did you mean sub-blk?
>>>>
>>>>>>
>>>>>>>> +{
>>>>>>>> +    u32 base;
>>>>>>>> +
>>>>>>>> +    switch (blk_type) {
>>>>>>>> +    case DPU_HW_BLK_TOP:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;
>>>>>>>> +
>>>>>>>> +        if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>>>>> MDP_PERIPH_TOP0,
>>>>>>>> +                            mmio + top->base, "top");
>>>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len - 
>>>>>>>> MDP_PERIPH_TOP0_END,
>>>>>>>> +                            mmio + top->base + 
>>>>>>>> MDP_PERIPH_TOP0_END,
>>>>>>>> +                            "top_2");
>>>>>>>> +        } else {
>>>>>>>> +            msm_disp_snapshot_add_block(disp_state, top->len, 
>>>>>>>> mmio + top->base, "top");
>>>>>>>> +        }
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +    case DPU_HW_BLK_LM:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_lm_cfg *mixer = (struct dpu_lm_cfg *)blk;
>>>>>>>> +
>>>>>>>> +        msm_disp_snapshot_add_block(disp_state, mixer->len, 
>>>>>>>> mmio + mixer->base, "%s",
>>>>>>>> +                        mixer->name);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +    case DPU_HW_BLK_CTL:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_ctl_cfg *ctl = (struct dpu_ctl_cfg *)blk;
>>>>>>>> +
>>>>>>>> +        msm_disp_snapshot_add_block(disp_state, ctl->len, mmio 
>>>>>>>> + ctl->base, "%s",
>>>>>>>> +                        ctl->name);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +    case DPU_HW_BLK_INTF:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_intf_cfg *intf = (struct dpu_intf_cfg *)blk;
>>>>>>>> +
>>>>>>>> +        msm_disp_snapshot_add_block(disp_state, intf->len, mmio 
>>>>>>>> + intf->base, "%s",
>>>>>>>> +                        intf->name);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +    case DPU_HW_BLK_WB:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_wb_cfg *wb = (struct dpu_wb_cfg *)blk;
>>>>>>>> +
>>>>>>>> +        msm_disp_snapshot_add_block(disp_state, wb->len, mmio + 
>>>>>>>> wb->base, "%s",
>>>>>>>> +                        wb->name);
>>>>>>>> +        break;
>>>>>>>> +    }
>>>>>>>> +    case DPU_HW_BLK_SSPP:
>>>>>>>> +    {
>>>>>>>> +        struct dpu_sspp_cfg *sspp_block = (struct dpu_sspp_cfg 
>>>>>>>> *)blk;
>>>>>>>> +        const struct dpu_sspp_sub_blks *sblk = sspp_block->sblk;
>>>>>>>> +
>>>>>>>> +        base = sspp_block->base;
>>>>>>>> +
>>>>>>>> +        msm_disp_snapshot_add_block(disp_state, 
>>>>>>>> sspp_block->len, mmio + base, "%s",
>>>>>>>> +                        sspp_block->name);
>>>>>>>> +
>>>>>>>> +        if (sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED3) ||
>>>>>>>> +            sspp_block->features & 
>>>>>>>> BIT(DPU_SSPP_SCALER_QSEED3LITE) ||
>>>>>>>> +            sspp_block->features & BIT(DPU_SSPP_SCALER_QSEED4))
>>>>>>>> +            msm_disp_snapshot_add_block(disp_state, 
>>>>>>>> sblk->scaler_blk.len,
>>>>>>>> +                            mmio + base + 
>>>>>>>> sblk->scaler_blk.base, "%s_%s",
>>>>>>>> +                            sspp_block->name, 
>>>>>>>> sblk->scaler_blk.name);
>>>>>>>
>>>>>>> Actually, it would be better to:
>>>>>>> - drop name from all sblk instances (and use known string instead 
>>>>>>> of the sblk name here)
>>>>>>> - Use sblk->foo_blk.len to check if it should be printed or not.
>>>>>>>
>>>>>>
>>>>>> No, I dont agree. If we drop the names from the sub_blk in the 
>>>>>> catalog, we will end up using "sub_blk_name" string here in the 
>>>>>> code to indicate which blk that is in the dump.
>>>>>>
>>>>>> If we add more sub_blks in the catalog in the future we need to 
>>>>>> keep changing the code over here. Thats not how it should be.
>>>>>>
>>>>>> Leaving the names in the catalog ensures that this code wont 
>>>>>> change and only catalog changes when we add a new sub_blk either 
>>>>>> for an existing or new chipset.
>>>>>>
>>>>>> catalog is indicating the new blk, and dumping code just prints it.
>>>>>>
>>>>>> with your approach, dumping code will or can keep changing with 
>>>>>> chipsets or sub_blks. Thats not how it should be.
>>>>>
>>>>> Well, we do not enumerate sub-blocks in any way, they are not 
>>>>> indexed. So even with sblk->blk.name in place, adding new sub-block 
>>>>> would require adding new code here. That's why I wrote that the 
>>>>> calling code knows which sub-block it refers to.
>>>>>
>>>>
>>>> Today, unfortunately each sub_blk type is different so we have to do 
>>>> this case by case.
>>>>
>>>> Ideally, this should have just been
>>>>
>>>> -> print main blk
>>>> -> print all sub-blks of the main blk
>>>>
>>>> Without having to handle each main blk's sub-blks separately.
>>>>
>>>> That way the dumping code would have remained generic without having 
>>>> to do even the multiplexer in the first place.
>>>>
>>>> Need to explore if somehow we can come up with a generic sub-blk 
>>>> struct and make this possible. Then this code will become much 
>>>> easier and what I am saying will make total sense.
>>>
>>> In such case, yes. However I'd warn about having a generic array of 
>>> subblocks. Having named subblock entries might complicate 
>>> snapshotting, but it makes the rest of the DPU driver smaller.
>>>
>>
>> Need to explore this. But not immediately.
>>
>>>>
>>>> Even without that, conceptually these sub-blk names are reflecting 
>>>> whats in our software document. So its not a random name but 
>>>> reflects the actual sub-blk name from the hardware.
>>>
>>> Yes
>>>
>>>> So this belongs in the catalog.
>>>
>>> But the sub-block field already has a correct name: scaler_blk, 
>>> csc_blk, etc. Having both sub-block field name and the .name inside 
>>> results in kind of duplication, which seems unnecessary to me.
>>>
>>
>> No, there is a difference and not duplicated. One is the name of the 
>> struct so it can really be anything and doesnt need to match the hw 
>> doc name. But the other is the string name which we can give exactly 
>> to match software interface doc and makes parsing such a dump much 
>> much easier.
>>
>> One point I dont see you have considered is the block index of the 
>> sub_blk.
>>
>> Today, yes I see only a "pcc" or a "dither" etc
>>
>> What if there are two PCCs or two dithers.
>>
>> Then their names can just be "pcc_0" and "pcc_1" or "dither_0" and 
>> "dither_1".
>>
>> Having name gives us the ability to easily incorporate even 
>> unsequential indices.
>>
>> For example, every sspp's name today is not sequential. it can be 
>> "sspp_3" then "sspp_8" etc
>>
>> By having names reflect the correct indices, dumping code becomes less 
>> complex as the catalog will still have the right names as dumping code 
>> will just use that.
>>
> 
> The QC team is in agreement that we would like to go ahead with the 
> names from the catalog and not drop them.
> 
> Hence we will post the next revision with the name still from the 
> catalog and drop the multiplexer completely.

Ack, let's see how it goes.

> 
> Since the intern has a short period of time to finish development on 
> this task, we would like to go ahead with this approach and post the 
> next rev.

This is a bad argument.


-- 
With best wishes
Dmitry

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

[Dmitry Baryshkov]

On 23/06/2023 02:48, Ryan McCann wrote:
> Currently, the device core dump mechanism does not dump registers of sub
> blocks within the DSPP, SSPP, DSC, and PINGPONG blocks. Add wrapper
> function to dump hardware blocks that contain sub blocks.
> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> ---
>   drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 194 +++++++++++++++++++++++++++-----
>   1 file changed, 168 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa8499de1b9f..9b1b1c382269 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -885,6 +885,154 @@ static int dpu_irq_postinstall(struct msm_kms *kms)
>   	return 0;
>   }
>   
> +static void dpu_kms_mdp_snapshot_add_block(struct msm_disp_state *disp_state,
> +					   void __iomem *mmio, void *blk,
> +					   enum dpu_hw_blk_type blk_type)
> +{
> +	u32 base;
> +
> +	switch (blk_type) {
> +	case DPU_HW_BLK_TOP:
> +	{
> +		struct dpu_mdp_cfg *top = (struct dpu_mdp_cfg *)blk;

Block configuration is constant, so these should be constant pointers. 
Not to mention that such type conversions are usually considered to be a 
bad idea. We loose type information, so compiler can not catch any 
errors here.

> +
> +		if (top->features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
> +			msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
> +						    mmio + top->base, "top");
> +			msm_disp_snapshot_add_block(disp_state, top->len - MDP_PERIPH_TOP0_END,
> +						    mmio + top->base + MDP_PERIPH_TOP0_END,
> +						    "top_2");
> +		} else {
> +			msm_disp_snapshot_add_block(disp_state, top->len, mmio + top->base, "top");
> +		}
> +		break;

[skipped]

	default:
> +		DPU_ERROR("Block type not supported.");

Which block type?

> +	}
> +}
> +
>   static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_kms *kms)
>   {
>   	int i;
> @@ -899,53 +1047,47 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>   
>   	/* dump CTL sub-blocks HW regs info */
>   	for (i = 0; i < cat->ctl_count; i++)
> -		msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
> -				dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
> +		dpu_kms_mdp_snapshot_add_block(disp_state, dpu_kms->mmio, (void *)&cat->ctl[i],

Here we are loosing the const property, which is a bad idea.

> +					       DPU_HW_BLK_CTL);

-- 
With best wishes
Dmitry

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

[Marijn Suijten]

It is nice if cover letters also include the subsystem:

drm/msm: Add support to print DPU sub-block registers

On 2023-06-22 16:48:52, Ryan McCann wrote:
> 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

Global nit: I think we previously settled on "sblk" or "sub-block(s)",
not "sub blocks".

And capitalize DPU.

> 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.

Dmitry has been doing that in one of his DPU catalog reworks.

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

Let's see what this means, because the code logic should already be able
to figure this out (and in some places we can perhaps delete the name
entirely).

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

Thought this could be rather automated, but let me see what it means in
the patches.

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

My suggestion would be to put less emphasis on this header with:

    ----sspp_0_scaler----

So that it becomes obvious that this is a sblk of the ====sspp_0====
above.

> ...
> ...
> ====sspp_0_csc====
> ...
> ...
> ====next_block====
> ...
> 
> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>

No need for sign-off in cover letters.

- Marijn

> ---
> 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 suffix in name of sub blocks
>       drm/msm/disp: Remove redundant prefix 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           | 194 +++++++++++++++++++---
>  drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |   2 +-
>  3 files changed, 214 insertions(+), 72 deletions(-)
> ---
> base-commit: 710025fdedb3767655823c3a12d27d404d209f75
> change-id: 20230622-devcoredump_patch-df7e8f6fd632
> 
> Best regards,
> -- 
> Ryan McCann <quic_rmccann@quicinc.com>
> 

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

[Jessica Zhang]

On 6/23/2023 12:10 AM, Marijn Suijten wrote:
> It is nice if cover letters also include the subsystem:
> 
> drm/msm: Add support to print DPU sub-block registers
> 
> On 2023-06-22 16:48:52, Ryan McCann wrote:
>> 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

Hey Marijn,

Sorry for the late response -- had to shift focus onto another feature 
for a bit.

> 
> Global nit: I think we previously settled on "sblk" or "sub-block(s)",
> not "sub blocks".
> 
> And capitalize DPU.

Acked.

> 
>> 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.
> 
> Dmitry has been doing that in one of his DPU catalog reworks.

Got it. Is there a specific one that's making similar changes? IIRC, I 
didn't see one that was changing the *_SBLK macro.

> 
>> 5. Defining names for sub blocks that have not yet been defined.
> 
> Let's see what this means, because the code logic should already be able
> to figure this out (and in some places we can perhaps delete the name
> entirely).
> 
>> 6. Implementing wrapper function that prints the registers of sub blocks
>> when there is a need.
> 
> Thought this could be rather automated, but let me see what it means in
> the patches.
> 
>> Sample Output of the sspp_0 block and its sub blocks for devcore dump:
>> ======sspp_0======
>> ...registers
>> ...
>> ====sspp_0_scaler====
> 
> My suggestion would be to put less emphasis on this header with:
> 
>      ----sspp_0_scaler----
> 
> So that it becomes obvious that this is a sblk of the ====sspp_0====
> above.

FWIW, I think having the main block name prefix in the sblk header makes 
it clear which blocks are main block and which ones are sblks.

In addition, I'd like to keep this change within DPU (aside from the fix 
in the first patch) since implementing this change would require 
changing the *_add_block() parameters, and DSI/DP don't seem to have a 
need for conditional header formatting.

Thanks,

Jessica Zhang

> 
>> ...
>> ...
>> ====sspp_0_csc====
>> ...
>> ...
>> ====next_block====
>> ...
>>
>> Signed-off-by: Ryan McCann <quic_rmccann@quicinc.com>
> 
> No need for sign-off in cover letters.
> 
> - Marijn
> 
>> ---
>> 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 suffix in name of sub blocks
>>        drm/msm/disp: Remove redundant prefix 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           | 194 +++++++++++++++++++---
>>   drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c |   2 +-
>>   3 files changed, 214 insertions(+), 72 deletions(-)
>> ---
>> base-commit: 710025fdedb3767655823c3a12d27d404d209f75
>> change-id: 20230622-devcoredump_patch-df7e8f6fd632
>>
>> Best regards,
>> -- 
>> Ryan McCann <quic_rmccann@quicinc.com>
>>