The purpose of this patch series is to add support to print the registers
of sub-blocks in the DPU hardware catalog and fix the order in which all
hardware blocks are dumped for a device core dump. This involves:
1. Changing data structure from stack to queue to fix the printing order
of the device core dump.
2. Removing redundant suffix of sub-block names.
3. Removing redundant prefix of sub-block names.
4. Eliminating unused variable from relevant macros.
5. Defining names for sub-blocks that have not yet been defined.
6. Implementing wrapper function that prints the registers of sub-blocks
when there is a need.
Sample Output of the sspp_0 block and its sub-blocks for devcore dump:
======sspp_0======
...registers
...
====sspp_0_scaler====
...
...
====sspp_0_csc====
...
...
====next_block====
...
---
Changes in v4:
- Added review tags
- Link to v3: https://lore.kernel.org/r/[email protected]
Changes in v3:
- Split sub-block changes and main block changes into two commits
- Corrected typo in comment located in DSC for loop block
- Eliminated variables mmio and base
- Dropped unnecessary "%s"
- Link to v2: https://lore.kernel.org/r/[email protected]
Changes in v2:
- Changed spelling "sub block" to "sub-block" or "sblk".
- Capitalized DPU.
- Eliminated multiplexer/wrapper function. Inlined instead.
- Changed if statements from feature checks to length checks.
- Squashed prefix and suffix patch into one.
- Link to v1: https://lore.kernel.org/r/[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 prefix/suffix in name of sub-blocks
drm/msm/dpu: Refactor printing of main blocks in device core dump
drm/msm/dpu: Update dev core dump to dump registers of sub-blocks
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 90 +++++++++++-----------
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 94 ++++++++++++++++++-----
drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 2 +-
3 files changed, 120 insertions(+), 66 deletions(-)
---
base-commit: a0364260213c96f6817f7e85cdce293cb743460f
change-id: 20230622-devcoredump_patch-df7e8f6fd632
Best regards,
--
Ryan McCann <[email protected]>
For a device core dump, the registers of sub-blocks are printed under a
title formatted as <mainBlkName_sblkName>. For example, the csc sub-block
for an SSPP main block "sspp_0" would be printed "sspp_0_sspp_csc0". The
title is clearly redundant due to the duplicate "sspp" and "0" that exist
in both the mainBlkName and sblkName. To eliminate this redundancy, remove
the secondary "sspp" and "0" that exist in the sub-block name by
elimanting the "sspp_" prefix and the concatenation of "num" that results
in the redundant "0" suffix. Remove num parameter altogether from relevant
macros as a consequence of it no longer being used.
Reviewed-by: Abhinav Kumar <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
Signed-off-by: Ryan McCann <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c | 50 +++++++++++++-------------
1 file changed, 25 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
index 79e495dbc11d..836efa074a35 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -252,15 +252,15 @@ static const uint32_t wb2_formats[] = {
*************************************************************/
/* SSPP common configuration */
-#define _VIG_SBLK(num, sdma_pri, qseed_ver) \
+#define _VIG_SBLK(sdma_pri, qseed_ver) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
- .scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+ .scaler_blk = {.name = "scaler", \
.id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
- .csc_blk = {.name = STRCAT("sspp_csc", num), \
+ .csc_blk = {.name = "csc", \
.id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
@@ -270,15 +270,15 @@ static const uint32_t wb2_formats[] = {
.rotation_cfg = NULL, \
}
-#define _VIG_SBLK_ROT(num, sdma_pri, qseed_ver, rot_cfg) \
+#define _VIG_SBLK_ROT(sdma_pri, qseed_ver, rot_cfg) \
{ \
.maxdwnscale = MAX_DOWNSCALE_RATIO, \
.maxupscale = MAX_UPSCALE_RATIO, \
.smart_dma_priority = sdma_pri, \
- .scaler_blk = {.name = STRCAT("sspp_scaler", num), \
+ .scaler_blk = {.name = "scaler", \
.id = qseed_ver, \
.base = 0xa00, .len = 0xa0,}, \
- .csc_blk = {.name = STRCAT("sspp_csc", num), \
+ .csc_blk = {.name = "csc", \
.id = DPU_SSPP_CSC_10BIT, \
.base = 0x1a00, .len = 0x100,}, \
.format_list = plane_formats_yuv, \
@@ -300,13 +300,13 @@ static const uint32_t wb2_formats[] = {
}
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_0 =
- _VIG_SBLK("0", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_1 =
- _VIG_SBLK("1", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_2 =
- _VIG_SBLK("2", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks msm8998_vig_sblk_3 =
- _VIG_SBLK("3", 0, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(0, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
.rot_maxheight = 1088,
@@ -315,13 +315,13 @@ static const struct dpu_rotation_cfg dpu_rot_sc7280_cfg_v2 = {
};
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_0 =
- _VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(5, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_1 =
- _VIG_SBLK("1", 6, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(6, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_2 =
- _VIG_SBLK("2", 7, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(7, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_vig_sblk_3 =
- _VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED3);
+ _VIG_SBLK(8, DPU_SSPP_SCALER_QSEED3);
static const struct dpu_sspp_sub_blks sdm845_dma_sblk_0 = _DMA_SBLK(1);
static const struct dpu_sspp_sub_blks sdm845_dma_sblk_1 = _DMA_SBLK(2);
@@ -341,31 +341,31 @@ static const struct dpu_sspp_sub_blks sdm845_dma_sblk_3 = _DMA_SBLK(4);
}
static const struct dpu_sspp_sub_blks sc7180_vig_sblk_0 =
- _VIG_SBLK("0", 4, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(4, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sc7280_vig_sblk_0 =
- _VIG_SBLK_ROT("0", 4, DPU_SSPP_SCALER_QSEED4, &dpu_rot_sc7280_cfg_v2);
+ _VIG_SBLK_ROT(4, DPU_SSPP_SCALER_QSEED4, &dpu_rot_sc7280_cfg_v2);
static const struct dpu_sspp_sub_blks sm6115_vig_sblk_0 =
- _VIG_SBLK("0", 2, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(2, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8250_vig_sblk_0 =
- _VIG_SBLK("0", 5, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(5, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8250_vig_sblk_1 =
- _VIG_SBLK("1", 6, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(6, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8250_vig_sblk_2 =
- _VIG_SBLK("2", 7, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(7, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8250_vig_sblk_3 =
- _VIG_SBLK("3", 8, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(8, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_vig_sblk_0 =
- _VIG_SBLK("0", 7, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(7, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_vig_sblk_1 =
- _VIG_SBLK("1", 8, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(8, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_vig_sblk_2 =
- _VIG_SBLK("2", 9, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(9, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_vig_sblk_3 =
- _VIG_SBLK("3", 10, DPU_SSPP_SCALER_QSEED4);
+ _VIG_SBLK(10, DPU_SSPP_SCALER_QSEED4);
static const struct dpu_sspp_sub_blks sm8550_dma_sblk_4 = _DMA_SBLK(5);
static const struct dpu_sspp_sub_blks sm8550_dma_sblk_5 = _DMA_SBLK(6);
--
2.25.1
Some sub-blocks in the hw catalog have not been given a name, so when the
registers from that block are dumped, there is no name to reference.
Define names for relevant sub-blocks to fix this.
Reviewed-by: Abhinav Kumar <[email protected]>
Reviewed-by: Dmitry Baryshkov <[email protected]>
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 9f9d5ac3992f..79e495dbc11d 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_catalog.c
@@ -444,12 +444,12 @@ static const struct dpu_lm_sub_blks qcm2290_lm_sblk = {
* DSPP sub blocks config
*************************************************************/
static const struct dpu_dspp_sub_blks msm8998_dspp_sblk = {
- .pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
.len = 0x90, .version = 0x10007},
};
static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
- .pcc = {.id = DPU_DSPP_PCC, .base = 0x1700,
+ .pcc = {.name = "pcc", .id = DPU_DSPP_PCC, .base = 0x1700,
.len = 0x90, .version = 0x40000},
};
@@ -465,19 +465,19 @@ static const struct dpu_dspp_sub_blks sm8150_dspp_sblk = {
* PINGPONG sub blocks config
*************************************************************/
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk_te = {
- .te2 = {.id = DPU_PINGPONG_TE2, .base = 0x2000, .len = 0x0,
+ .te2 = {.name = "te2", .id = DPU_PINGPONG_TE2, .base = 0x2000, .len = 0x0,
.version = 0x1},
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
.len = 0x20, .version = 0x10000},
};
static const struct dpu_pingpong_sub_blks sdm845_pp_sblk = {
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0x30e0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0x30e0,
.len = 0x20, .version = 0x10000},
};
static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
- .dither = {.id = DPU_PINGPONG_DITHER, .base = 0xe0,
+ .dither = {.name = "dither", .id = DPU_PINGPONG_DITHER, .base = 0xe0,
.len = 0x20, .version = 0x20000},
};
@@ -517,13 +517,13 @@ static const struct dpu_pingpong_sub_blks sc7280_pp_sblk = {
* DSC sub blocks config
*************************************************************/
static const struct dpu_dsc_sub_blks dsc_sblk_0 = {
- .enc = {.base = 0x100, .len = 0x100},
- .ctl = {.base = 0xF00, .len = 0x10},
+ .enc = {.name = "enc", .base = 0x100, .len = 0x100},
+ .ctl = {.name = "ctl", .base = 0xF00, .len = 0x10},
};
static const struct dpu_dsc_sub_blks dsc_sblk_1 = {
- .enc = {.base = 0x200, .len = 0x100},
- .ctl = {.base = 0xF80, .len = 0x10},
+ .enc = {.name = "enc", .base = 0x200, .len = 0x100},
+ .ctl = {.name = "ctl", .base = 0xF80, .len = 0x10},
};
#define DSC_BLK(_name, _id, _base, _features) \
--
2.25.1
Device core dump add block method adds hardware blocks to dumping queue
with stack behavior which causes the hardware blocks to be printed in
reverse order. Change the addition to dumping queue data structure
from "list_add" to "list_add_tail" for FIFO queue behavior.
Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reviewed-by: Dmitry Baryshkov <[email protected]>
Reviewed-by: Abhinav Kumar <[email protected]>
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
Currently, the names of main blocks are hardcoded into the
msm_disp_snapshot_add_block function rather than using the name that
already exists in the catalog. Change this to take the name directly from
the catalog instead of hardcoding it.
Signed-off-by: Ryan McCann <[email protected]>
---
drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 ++++++++++++++++----------------
1 file changed, 16 insertions(+), 16 deletions(-)
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
index aa8499de1b9f..70dbb1204e6c 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
@@ -899,38 +899,38 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, dpu_kms->mmio +
+ cat->ctl[i].base, cat->ctl[i].name);
/* dump DSPP sub-blocks HW regs info */
for (i = 0; i < cat->dspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
- dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
+ cat->dspp[i].base, cat->dspp[i].name);
/* dump INTF sub-blocks HW regs info */
for (i = 0; i < cat->intf_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
- dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
+ cat->intf[i].base, cat->intf[i].name);
/* dump PP sub-blocks HW regs info */
for (i = 0; i < cat->pingpong_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
- dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
+ cat->pingpong[i].base, cat->pingpong[i].name);
/* dump SSPP sub-blocks HW regs info */
for (i = 0; i < cat->sspp_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
- dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
+ cat->sspp[i].base, cat->sspp[i].name);
/* dump LM sub-blocks HW regs info */
for (i = 0; i < cat->mixer_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
- dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
+ cat->mixer[i].base, cat->mixer[i].name);
/* dump WB sub-blocks HW regs info */
for (i = 0; i < cat->wb_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
- dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, dpu_kms->mmio +
+ cat->wb[i].base, cat->wb[i].name);
if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
@@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
/* dump DSC sub-blocks HW regs info */
for (i = 0; i < cat->dsc_count; i++)
- msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
- dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
+ msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
+ cat->dsc[i].base, cat->dsc[i].name);
pm_runtime_put_sync(&dpu_kms->pdev->dev);
}
--
2.25.1
On 06/07/2023 23:48, Ryan McCann wrote:
> Currently, the names of main blocks are hardcoded into the
> msm_disp_snapshot_add_block function rather than using the name that
> already exists in the catalog. Change this to take the name directly from
> the catalog instead of hardcoding it.
>
> Signed-off-by: Ryan McCann <[email protected]>
> ---
> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32 ++++++++++++++++----------------
> 1 file changed, 16 insertions(+), 16 deletions(-)
>
> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> index aa8499de1b9f..70dbb1204e6c 100644
> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
> @@ -899,38 +899,38 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>
> /* dump CTL sub-blocks HW regs info */
> for (i = 0; i < cat->ctl_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
> - dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len, dpu_kms->mmio +
> + cat->ctl[i].base, cat->ctl[i].name);
Splitting on the `+' sign is a bad idea. It makes it harder to read the
code. Please keep the first line as is, it is perfectly fine on its own,
and do just what you have stated in the commit message: change printed
block name.
>
> /* dump DSPP sub-blocks HW regs info */
> for (i = 0; i < cat->dspp_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
> - dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len, dpu_kms->mmio +
> + cat->dspp[i].base, cat->dspp[i].name);
>
> /* dump INTF sub-blocks HW regs info */
> for (i = 0; i < cat->intf_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
> - dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->intf[i].len, dpu_kms->mmio +
> + cat->intf[i].base, cat->intf[i].name);
>
> /* dump PP sub-blocks HW regs info */
> for (i = 0; i < cat->pingpong_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
> - dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len, dpu_kms->mmio +
> + cat->pingpong[i].base, cat->pingpong[i].name);
>
> /* dump SSPP sub-blocks HW regs info */
> for (i = 0; i < cat->sspp_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
> - dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len, dpu_kms->mmio +
> + cat->sspp[i].base, cat->sspp[i].name);
>
> /* dump LM sub-blocks HW regs info */
> for (i = 0; i < cat->mixer_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
> - dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len, dpu_kms->mmio +
> + cat->mixer[i].base, cat->mixer[i].name);
>
> /* dump WB sub-blocks HW regs info */
> for (i = 0; i < cat->wb_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
> - dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->wb[i].len, dpu_kms->mmio +
> + cat->wb[i].base, cat->wb[i].name);
>
> if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
> msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct msm_disp_state *disp_state, struct msm_k
>
> /* dump DSC sub-blocks HW regs info */
> for (i = 0; i < cat->dsc_count; i++)
> - msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
> - dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
> + msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len, dpu_kms->mmio +
> + cat->dsc[i].base, cat->dsc[i].name);
>
> pm_runtime_put_sync(&dpu_kms->pdev->dev);
> }
>
--
With best wishes
Dmitry
On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
> On 06/07/2023 23:48, Ryan McCann wrote:
>> Currently, the names of main blocks are hardcoded into the
>> msm_disp_snapshot_add_block function rather than using the name that
>> already exists in the catalog. Change this to take the name directly from
>> the catalog instead of hardcoding it.
>>
>> Signed-off-by: Ryan McCann <[email protected]>
>> ---
>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32
>> ++++++++++++++++----------------
>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> index aa8499de1b9f..70dbb1204e6c 100644
>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>> @@ -899,38 +899,38 @@ static void dpu_kms_mdp_snapshot(struct
>> msm_disp_state *disp_state, struct msm_k
>> /* dump CTL sub-blocks HW regs info */
>> for (i = 0; i < cat->ctl_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>> - dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>> dpu_kms->mmio +
>> + cat->ctl[i].base, cat->ctl[i].name);
>
> Splitting on the `+' sign is a bad idea. It makes it harder to read the
> code. Please keep the first line as is, it is perfectly fine on its own,
> and do just what you have stated in the commit message: change printed
> block name.
>
Actually, I asked Ryan to fix the indent here in the same patch as he
was touching this code anyway.
So you would prefer thats left untouched?
>> /* dump DSPP sub-blocks HW regs info */
>> for (i = 0; i < cat->dspp_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>> - dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>> dpu_kms->mmio +
>> + cat->dspp[i].base, cat->dspp[i].name);
>> /* dump INTF sub-blocks HW regs info */
>> for (i = 0; i < cat->intf_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>> - dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>> dpu_kms->mmio +
>> + cat->intf[i].base, cat->intf[i].name);
>> /* dump PP sub-blocks HW regs info */
>> for (i = 0; i < cat->pingpong_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>> - dpu_kms->mmio + cat->pingpong[i].base, "pingpong_%d",
>> i);
>> + msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>> dpu_kms->mmio +
>> + cat->pingpong[i].base, cat->pingpong[i].name);
>> /* dump SSPP sub-blocks HW regs info */
>> for (i = 0; i < cat->sspp_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>> - dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>> dpu_kms->mmio +
>> + cat->sspp[i].base, cat->sspp[i].name);
>> /* dump LM sub-blocks HW regs info */
>> for (i = 0; i < cat->mixer_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>> - dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>> dpu_kms->mmio +
>> + cat->mixer[i].base, cat->mixer[i].name);
>> /* dump WB sub-blocks HW regs info */
>> for (i = 0; i < cat->wb_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>> - dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>> dpu_kms->mmio +
>> + cat->wb[i].base, cat->wb[i].name);
>> if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>> msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct
>> msm_disp_state *disp_state, struct msm_k
>> /* dump DSC sub-blocks HW regs info */
>> for (i = 0; i < cat->dsc_count; i++)
>> - msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>> - dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>> + msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>> dpu_kms->mmio +
>> + cat->dsc[i].base, cat->dsc[i].name);
>> pm_runtime_put_sync(&dpu_kms->pdev->dev);
>> }
>>
>
On 7/6/2023 7:19 PM, Dmitry Baryshkov wrote:
> On 07/07/2023 03:16, Abhinav Kumar wrote:
>>
>>
>> On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
>>> On 06/07/2023 23:48, Ryan McCann wrote:
>>>> Currently, the names of main blocks are hardcoded into the
>>>> msm_disp_snapshot_add_block function rather than using the name that
>>>> already exists in the catalog. Change this to take the name directly
>>>> from
>>>> the catalog instead of hardcoding it.
>>>>
>>>> Signed-off-by: Ryan McCann <[email protected]>
>>>> ---
>>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32
>>>> ++++++++++++++++----------------
>>>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>>>
>>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> index aa8499de1b9f..70dbb1204e6c 100644
>>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>>> @@ -899,38 +899,38 @@ static void dpu_kms_mdp_snapshot(struct
>>>> msm_disp_state *disp_state, struct msm_k
>>>> /* dump CTL sub-blocks HW regs info */
>>>> for (i = 0; i < cat->ctl_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>>>> - dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->ctl[i].base, cat->ctl[i].name);
>>>
>>> Splitting on the `+' sign is a bad idea. It makes it harder to read
>>> the code. Please keep the first line as is, it is perfectly fine on
>>> its own, and do just what you have stated in the commit message:
>>> change printed block name.
>>>
>>
>> Actually, I asked Ryan to fix the indent here in the same patch as he
>> was touching this code anyway.
>>
>> So you would prefer thats left untouched?
>
> Yes. The current one was definitely better than splitting in the middle
> of a statement.
>
Certainly Yes. Splitting across '+' was the last resort. For some
reason, I thought any other option of splitting was breaking checkpatch
for ryan so we had to do that. But, for this change it seems like even
if we had done like below checkpatch didnt complain.
@@ -900,7 +900,7 @@ static void dpu_kms_mdp_snapshot(struct
msm_disp_state *disp_state, struct msm_k
/* dump CTL sub-blocks HW regs info */
for (i = 0; i < cat->ctl_count; i++)
msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
- dpu_kms->mmio + cat->ctl[i].base,
"ctl_%d", i);
+ dpu_kms->mmio +
cat->ctl[i].base, "ctl_%d", i);
But anyway, we can just take care of fixing indentation separately to
avoid the hassle.
>>
>>>> /* dump DSPP sub-blocks HW regs info */
>>>> for (i = 0; i < cat->dspp_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>>> - dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->dspp[i].base, cat->dspp[i].name);
>>>> /* dump INTF sub-blocks HW regs info */
>>>> for (i = 0; i < cat->intf_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>>> - dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->intf[i].base, cat->intf[i].name);
>>>> /* dump PP sub-blocks HW regs info */
>>>> for (i = 0; i < cat->pingpong_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>>>> - dpu_kms->mmio + cat->pingpong[i].base,
>>>> "pingpong_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state,
>>>> cat->pingpong[i].len, dpu_kms->mmio +
>>>> + cat->pingpong[i].base, cat->pingpong[i].name);
>>>> /* dump SSPP sub-blocks HW regs info */
>>>> for (i = 0; i < cat->sspp_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>>> - dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->sspp[i].base, cat->sspp[i].name);
>>>> /* dump LM sub-blocks HW regs info */
>>>> for (i = 0; i < cat->mixer_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>>> - dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->mixer[i].base, cat->mixer[i].name);
>>>> /* dump WB sub-blocks HW regs info */
>>>> for (i = 0; i < cat->wb_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>>> - dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->wb[i].base, cat->wb[i].name);
>>>> if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>>> msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct
>>>> msm_disp_state *disp_state, struct msm_k
>>>> /* dump DSC sub-blocks HW regs info */
>>>> for (i = 0; i < cat->dsc_count; i++)
>>>> - msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>>> - dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>>>> + msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>>> dpu_kms->mmio +
>>>> + cat->dsc[i].base, cat->dsc[i].name);
>>>> pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>>> }
>>>>
>>>
>
On 07/07/2023 03:16, Abhinav Kumar wrote:
>
>
> On 7/6/2023 5:07 PM, Dmitry Baryshkov wrote:
>> On 06/07/2023 23:48, Ryan McCann wrote:
>>> Currently, the names of main blocks are hardcoded into the
>>> msm_disp_snapshot_add_block function rather than using the name that
>>> already exists in the catalog. Change this to take the name directly
>>> from
>>> the catalog instead of hardcoding it.
>>>
>>> Signed-off-by: Ryan McCann <[email protected]>
>>> ---
>>> drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c | 32
>>> ++++++++++++++++----------------
>>> 1 file changed, 16 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> index aa8499de1b9f..70dbb1204e6c 100644
>>> --- a/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> +++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_kms.c
>>> @@ -899,38 +899,38 @@ static void dpu_kms_mdp_snapshot(struct
>>> msm_disp_state *disp_state, struct msm_k
>>> /* dump CTL sub-blocks HW regs info */
>>> for (i = 0; i < cat->ctl_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>>> - dpu_kms->mmio + cat->ctl[i].base, "ctl_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->ctl[i].len,
>>> dpu_kms->mmio +
>>> + cat->ctl[i].base, cat->ctl[i].name);
>>
>> Splitting on the `+' sign is a bad idea. It makes it harder to read
>> the code. Please keep the first line as is, it is perfectly fine on
>> its own, and do just what you have stated in the commit message:
>> change printed block name.
>>
>
> Actually, I asked Ryan to fix the indent here in the same patch as he
> was touching this code anyway.
>
> So you would prefer thats left untouched?
Yes. The current one was definitely better than splitting in the middle
of a statement.
>
>>> /* dump DSPP sub-blocks HW regs info */
>>> for (i = 0; i < cat->dspp_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>> - dpu_kms->mmio + cat->dspp[i].base, "dspp_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->dspp[i].len,
>>> dpu_kms->mmio +
>>> + cat->dspp[i].base, cat->dspp[i].name);
>>> /* dump INTF sub-blocks HW regs info */
>>> for (i = 0; i < cat->intf_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>> - dpu_kms->mmio + cat->intf[i].base, "intf_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->intf[i].len,
>>> dpu_kms->mmio +
>>> + cat->intf[i].base, cat->intf[i].name);
>>> /* dump PP sub-blocks HW regs info */
>>> for (i = 0; i < cat->pingpong_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->pingpong[i].len,
>>> - dpu_kms->mmio + cat->pingpong[i].base,
>>> "pingpong_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state,
>>> cat->pingpong[i].len, dpu_kms->mmio +
>>> + cat->pingpong[i].base, cat->pingpong[i].name);
>>> /* dump SSPP sub-blocks HW regs info */
>>> for (i = 0; i < cat->sspp_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>> - dpu_kms->mmio + cat->sspp[i].base, "sspp_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->sspp[i].len,
>>> dpu_kms->mmio +
>>> + cat->sspp[i].base, cat->sspp[i].name);
>>> /* dump LM sub-blocks HW regs info */
>>> for (i = 0; i < cat->mixer_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>> - dpu_kms->mmio + cat->mixer[i].base, "lm_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->mixer[i].len,
>>> dpu_kms->mmio +
>>> + cat->mixer[i].base, cat->mixer[i].name);
>>> /* dump WB sub-blocks HW regs info */
>>> for (i = 0; i < cat->wb_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>> - dpu_kms->mmio + cat->wb[i].base, "wb_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->wb[i].len,
>>> dpu_kms->mmio +
>>> + cat->wb[i].base, cat->wb[i].name);
>>> if (cat->mdp[0].features & BIT(DPU_MDP_PERIPH_0_REMOVED)) {
>>> msm_disp_snapshot_add_block(disp_state, MDP_PERIPH_TOP0,
>>> @@ -944,8 +944,8 @@ static void dpu_kms_mdp_snapshot(struct
>>> msm_disp_state *disp_state, struct msm_k
>>> /* dump DSC sub-blocks HW regs info */
>>> for (i = 0; i < cat->dsc_count; i++)
>>> - msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>> - dpu_kms->mmio + cat->dsc[i].base, "dsc_%d", i);
>>> + msm_disp_snapshot_add_block(disp_state, cat->dsc[i].len,
>>> dpu_kms->mmio +
>>> + cat->dsc[i].base, cat->dsc[i].name);
>>> pm_runtime_put_sync(&dpu_kms->pdev->dev);
>>> }
>>>
>>
--
With best wishes
Dmitry