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 <[email protected]>
---
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 <[email protected]>
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 <[email protected]>
---
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
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 <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
Reviewed-by: Dmitry Baryshkov <[email protected]>
--
With best wishes
Dmitry
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 <[email protected]>
---
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
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 <[email protected]>
---
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
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 <[email protected]>
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 <[email protected]>
>
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 <[email protected]>
>
> 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 <[email protected]>
>>