[PATCH] drm/msm/snapshot: fix dumping of the unaligned regions

[PATCH] drm/msm/snapshot: fix dumping of the unaligned regions

[Dmitry Baryshkov]

The snapshotting code internally aligns data segment to 16 bytes. This
works fine for DPU code (where most of the regions are aligned), but
fails for snapshotting of the DSI data (because DSI data region is
shifted by 4 bytes). Fix the code by removing length alignment and by
accurately printing last registers in the region. While reworking the
code also fix the 16x memory overallocation in
msm_disp_state_dump_regs().

Fixes: 98659487b845 ("drm/msm: add support to take dpu snapshot")
Reported-by: Salendarsingh Gaud <sgaud@qti.qualcomm.com>
Signed-off-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
---
 drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c | 27 ++++++++++++++++++-----
 1 file changed, 21 insertions(+), 6 deletions(-)

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 5e151952dea8..a86c7fc46f68 100644
--- a/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
+++ b/drivers/gpu/drm/msm/disp/msm_disp_snapshot_util.c
@@ -9,7 +9,7 @@
 
 #include "msm_disp_snapshot.h"
 
-static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *base_addr)
+static void msm_disp_state_dump_regs(u32 **reg, u32 len, void __iomem *base_addr)
 {
 	u32 len_padded;
 	u32 num_rows;
@@ -19,11 +19,11 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 	void __iomem *end_addr;
 	int i;
 
-	len_padded = aligned_len * REG_DUMP_ALIGN;
-	num_rows = aligned_len / REG_DUMP_ALIGN;
+	len_padded = round_up(len, REG_DUMP_ALIGN);
+	num_rows = DIV_ROUND_UP(len, REG_DUMP_ALIGN);
 
 	addr = base_addr;
-	end_addr = base_addr + aligned_len;
+	end_addr = base_addr + len;
 
 	*reg = kvzalloc(len_padded, GFP_KERNEL);
 	if (!*reg)
@@ -48,8 +48,8 @@ static void msm_disp_state_dump_regs(u32 **reg, u32 aligned_len, void __iomem *b
 static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 		void __iomem *base_addr, struct drm_printer *p)
 {
+	void __iomem *addr, *end_addr;
 	int i;
-	void __iomem *addr;
 	u32 num_rows;
 
 	if (!dump_addr) {
@@ -58,6 +58,7 @@ static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 	}
 
 	addr = base_addr;
+	end_addr = base_addr + len;
 	num_rows = len / REG_DUMP_ALIGN;
 
 	for (i = 0; i < num_rows; i++) {
@@ -67,6 +68,17 @@ static void msm_disp_state_print_regs(const u32 *dump_addr, u32 len,
 				dump_addr[i * 4 + 2], dump_addr[i * 4 + 3]);
 		addr += REG_DUMP_ALIGN;
 	}
+
+	if (addr != end_addr) {
+		drm_printf(p, "0x%lx : %08x",
+			   (unsigned long)(addr - base_addr),
+			   dump_addr[i * 4]);
+		if (addr + 0x4 < end_addr)
+			drm_printf(p, " %08x", dump_addr[i * 4 + 1]);
+		if (addr + 0x8 < end_addr)
+			drm_printf(p, " %08x", dump_addr[i * 4 + 2]);
+		drm_printf(p, "\n");
+	}
 }
 
 void msm_disp_state_print(struct msm_disp_state *state, struct drm_printer *p)
@@ -172,6 +184,9 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
 	struct va_format vaf;
 	va_list va;
 
+	if (strcmp(fmt, "dsi%d_ctrl"))
+		return;
+
 	new_blk = kzalloc_obj(struct msm_disp_state_block);
 	if (!new_blk)
 		return;
@@ -185,7 +200,7 @@ void msm_disp_snapshot_add_block(struct msm_disp_state *disp_state, u32 len,
 	va_end(va);
 
 	INIT_LIST_HEAD(&new_blk->node);
-	new_blk->size = ALIGN(len, REG_DUMP_ALIGN);
+	new_blk->size = len;
 	new_blk->base_addr = base_addr;
 
 	msm_disp_state_dump_regs(&new_blk->state, new_blk->size, base_addr);

---
base-commit: 11ff30385c8ad7de9862f4f1cec424fca15a4f13
change-id: 20260515-msm-fix-dsi-dump-2-64a3bc160da6

Best regards,
--  
With best wishes
Dmitry

Claude review: drm/msm/snapshot: fix dumping of the unaligned regions

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/snapshot: fix dumping of the unaligned regions
Author: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>
Patches: 1
Reviewed: 2026-05-16T08:44:40.796210

---

This is a single patch fixing the MSM display snapshot code for unaligned register regions (specifically DSI, which has a 4-byte offset). The core fix is correct and well-reasoned: it removes premature alignment in the caller, fixes a 16x memory overallocation bug (`aligned_len * REG_DUMP_ALIGN` where multiplication was used instead of rounding), and adds proper handling for partial trailing rows during both register dumping and printing.

**However, there is a critical debug filter accidentally left in the patch** that makes it unsuitable for merging as-is. The `strcmp(fmt, "dsi%d_ctrl")` check at line 187 will silently skip ALL non-DSI snapshot blocks (DPU, DP, etc.), completely breaking devcoredump for anything other than DSI controllers.

**Verdict: Needs revision** — the debug filter must be removed before merging. The rest of the logic is correct.

---

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/snapshot: fix dumping of the unaligned regions

[Claude Code Review Bot]

Patch Review

**Debug filter left in — NAK as-is**

At line 187-188 of the patched file:
```c
if (strcmp(fmt, "dsi%d_ctrl"))
    return;
```
This is clearly debug scaffolding used during development to limit snapshot capture to only DSI controller blocks. `strcmp` returns non-zero (true) for any string that does NOT match `"dsi%d_ctrl"`, so this causes `msm_disp_snapshot_add_block()` to silently return without capturing any DPU, DP, or other display hardware blocks. This is a serious regression that must be removed.

**16x overallocation fix — correct**

The original code had:
```c
len_padded = aligned_len * REG_DUMP_ALIGN;
```
where `aligned_len` was already aligned to 16 bytes. This multiplied by 16 again, allocating 16x the needed memory. The fix correctly uses:
```c
len_padded = round_up(len, REG_DUMP_ALIGN);
```

**`DIV_ROUND_UP` for num_rows — correct**

Changing from `aligned_len / REG_DUMP_ALIGN` (which relied on pre-alignment) to `DIV_ROUND_UP(len, REG_DUMP_ALIGN)` correctly handles unaligned lengths by producing enough rows to cover the full region.

**`end_addr` tracking — correct**

Setting `end_addr = base_addr + len` (using the raw, unaligned length) ensures the per-register bounds checks in the dump loop:
```c
x0 = (addr < end_addr) ? readl_relaxed(addr + 0x0) : 0;
x4 = (addr + 0x4 < end_addr) ? readl_relaxed(addr + 0x4) : 0;
```
correctly avoid MMIO reads past the valid register region. This was already present in the pre-patch code but only works correctly now that `end_addr` uses the true length.

**Print tail handling — correct but minor style nit**

The new tail section in `msm_disp_state_print_regs` (lines 72-81):
```c
if (addr != end_addr) {
    drm_printf(p, "0x%lx : %08x",
               (unsigned long)(addr - base_addr),
               dump_addr[i * 4]);
    if (addr + 0x4 < end_addr)
        drm_printf(p, " %08x", dump_addr[i * 4 + 1]);
    if (addr + 0x8 < end_addr)
        drm_printf(p, " %08x", dump_addr[i * 4 + 2]);
    drm_printf(p, "\n");
}
```
The logic is correct: after printing full 4-register rows via `num_rows = len / REG_DUMP_ALIGN` (floor division), the tail handles the 1-3 remaining registers. The buffer accesses are safe because the dump function allocated `round_up(len, REG_DUMP_ALIGN)` bytes. One minor observation: the first register in the tail (`dump_addr[i * 4]`) is printed unconditionally — this is safe because the `addr != end_addr` guard ensures at least one register remains.

**Removing alignment from the caller — correct**

Changing `new_blk->size = ALIGN(len, REG_DUMP_ALIGN)` to `new_blk->size = len` moves alignment responsibility into the dump/print functions where it belongs. The raw length is needed to correctly bound MMIO reads and partial-row printing.

**Summary**: Remove the `strcmp(fmt, "dsi%d_ctrl")` debug filter and this patch is ready. The alignment fixes and tail-printing logic are sound.

---
Generated by Claude Code Patch Reviewer