[PATCH v2] drm/msm/dpu: calculate data_width like downstream

[PATCH v2] drm/msm/dpu: calculate data_width like downstream

[Alexander Koskovich]

Derive INTF data_width from dce_bytes_per_line rather than
timing->width when DSC is enabled. Use DIV_ROUND_UP to avoid
rounding errors.

Signed-off-by: Alexander Koskovich <akoskovich@pm.me>
---
Changes in v2:
- Added back comment about DSC & DP
- Link to v1: https://lore.kernel.org/r/20260322-fix-data-width-calc-v1-1-128880f5a067@pm.me
---
 .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  2 ++
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 26 +++++++++++++++++-----
 drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  1 +
 3 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
index 0ba777bda253..ba810f26ea30 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c
@@ -10,6 +10,7 @@
 #include "dpu_formats.h"
 #include "dpu_trace.h"
 #include "disp/msm_disp_snapshot.h"
+#include "msm_dsc_helper.h"
 
 #include <drm/display/drm_dsc_helper.h>
 #include <drm/drm_managed.h>
@@ -136,6 +137,7 @@ static void drm_mode_to_intf_timing_params(
 		timing->width = timing->width * drm_dsc_get_bpp_int(dsc) /
 				(dsc->bits_per_component * 3);
 		timing->xres = timing->width;
+		timing->dce_bytes_per_line = msm_dsc_get_bytes_per_line(dsc);
 	}
 }
 
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
index 7e620f590984..ac82b69aedf6 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c
@@ -173,13 +173,29 @@ static void dpu_hw_intf_setup_timing_engine(struct dpu_hw_intf *intf,
 	data_width = p->width;
 
 	/*
-	 * If widebus is enabled, data is valid for only half the active window
-	 * since the data rate is doubled in this mode. But for the compression
-	 * mode in DP case, the p->width is already adjusted in
-	 * drm_mode_to_intf_timing_params()
+	 * If widebus is disabled:
+	 * For uncompressed stream, the data is valid for the entire active
+	 * window period.
+	 * For compressed stream, data is valid for a shorter time period
+	 * inside the active window depending on the compression ratio.
+	 *
+	 * If widebus is enabled:
+	 * For uncompressed stream, data is valid for only half the active
+	 * window, since the data rate is doubled in this mode.
+	 * For compressed stream, data validity window needs to be adjusted for
+	 * compression ratio and then further halved.
+	 *
+	 * For the compression mode in DP case, the p->width is already
+	 * adjusted in drm_mode_to_intf_timing_params().
 	 */
-	if (p->wide_bus_en && !dp_intf)
+	if (p->compression_en && !dp_intf) {
+		if (p->wide_bus_en)
+			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
+		else
+			data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
+	} else if (p->wide_bus_en && !dp_intf) {
 		data_width = p->width >> 1;
+	}
 
 	/* TODO: handle DSC+DP case, we only handle DSC+DSI case so far */
 	if (p->compression_en && !dp_intf &&
diff --git a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
index f6ef2c21b66d..badd26305fc9 100644
--- a/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
+++ b/drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h
@@ -35,6 +35,7 @@ struct dpu_hw_intf_timing_params {
 
 	bool wide_bus_en;
 	bool compression_en;
+	u32 dce_bytes_per_line;
 };
 
 struct dpu_hw_intf_prog_fetch {

---
base-commit: f338e77383789c0cae23ca3d48adcc5e9e137e3c
change-id: 20260322-fix-data-width-calc-c44287df08b8

Best regards,
-- 
Alexander Koskovich <akoskovich@pm.me>

Re: [PATCH v2] drm/msm/dpu: calculate data_width like downstream

[Dmitry Baryshkov]

On Sun, Mar 22, 2026 at 06:48:09PM +0000, Alexander Koskovich wrote:
> Derive INTF data_width from dce_bytes_per_line rather than
> timing->width when DSC is enabled. Use DIV_ROUND_UP to avoid
> rounding errors.
> 
> Signed-off-by: Alexander Koskovich <akoskovich@pm.me>
> ---
> Changes in v2:
> - Added back comment about DSC & DP
> - Link to v1: https://lore.kernel.org/r/20260322-fix-data-width-calc-v1-1-128880f5a067@pm.me
> ---
>  .../gpu/drm/msm/disp/dpu1/dpu_encoder_phys_vid.c   |  2 ++
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.c        | 26 +++++++++++++++++-----
>  drivers/gpu/drm/msm/disp/dpu1/dpu_hw_intf.h        |  1 +
>  3 files changed, 24 insertions(+), 5 deletions(-)
> 

Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@oss.qualcomm.com>


-- 
With best wishes
Dmitry

Re: [PATCH v2] drm/msm/dpu: calculate data_width like downstream

[Pengyu Luo]

On Mon, Mar 23, 2026 at 2:48 AM Alexander Koskovich <akoskovich@pm.me> wrote:
>
> Derive INTF data_width from dce_bytes_per_line rather than
> timing->width when DSC is enabled. Use DIV_ROUND_UP to avoid
> rounding errors.
>
> Signed-off-by: Alexander Koskovich <akoskovich@pm.me>
> ---

Tested-by: Pengyu Luo <mitltlatltl@gmail.com> # Lenovo Legion Y700 Gen4 (SM8750)

Best wishes,
Pengyu

Claude review: drm/msm/dpu: calculate data_width like downstream

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dpu: calculate data_width like downstream
Author: Alexander Koskovich <akoskovich@pm.me>
Patches: 3
Reviewed: 2026-03-25T08:12:58.528087

---

This is a single-patch fix to how the DPU INTF timing engine calculates the `data_width` (the data validity window) when DSC compression is enabled on DSI interfaces. The current upstream code derives `data_width` from `p->width`, which was already adjusted by a simplified compression ratio formula (`width * bpp_int / (bpc * 3)`). The patch replaces this with a calculation based on `msm_dsc_get_bytes_per_line()` (i.e., `slice_count * slice_chunk_size`), matching downstream/vendor behavior. It uses `DIV_ROUND_UP` to avoid truncation errors.

The approach is reasonable: using the actual DSC slice geometry (`slice_count * slice_chunk_size`) is more accurate than the existing approximation, which uses `drm_dsc_get_bpp_int()` and loses the fractional BPP. The pattern also mirrors how `dsi_host.c` already computes `hdisplay` for DSC.

**Overall: The patch looks correct in concept but has one notable concern.**

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dpu: calculate data_width like downstream

[Claude Code Review Bot]

Patch Review

**Positive aspects:**
- Uses the established `msm_dsc_get_bytes_per_line()` helper already used in `dsi_host.c`, maintaining consistency.
- `DIV_ROUND_UP` is the right choice to avoid off-by-one truncation in the data width.
- The `dce_bytes_per_line` field is properly zero-initialized (struct uses `= { 0 }`) and only read when `compression_en` is set.
- The expanded comment block is a welcome improvement over the previous terse comment.

**Concern — widebus video mode divisor:**

In `dsi_host.c:1028-1035`, the widebus divisor for DSC depends on the mode:
```c
if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
    bytes_per_pclk = 6;
else
    bytes_per_pclk = 3;
```

For **video mode** with widebus, DSI host uses `bytes_per_pclk = 3` (not 6). But this patch (which is in `dpu_encoder_phys_vid.c`, i.e., video mode only) divides by 6 when widebus is enabled:

```c
if (p->wide_bus_en)
    data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
```

These are admittedly computing different things (DSI packet timing vs. INTF data validity window), so this may be intentional and correct for the INTF hardware — but the reviewer should verify that the INTF timing engine truly expects the `data_width` to be halved for widebus even in video mode with DSC. It would be worth having a comment explaining why the INTF widebus behavior differs from the DSI host here, or citing the downstream source/hardware documentation that confirms the divide-by-6.

**Minor — commit message could be more specific:**

The commit message says "calculate data_width like downstream" but doesn't explain *what was wrong* with the existing calculation. A sentence like "The existing formula using `drm_dsc_get_bpp_int()` loses fractional BPP precision and doesn't account for actual slice geometry" would help reviewers understand the motivation.

**Nit — the "TODO: handle DSC+DP" comment is retained but could be updated:**

The comment at line 184 (`/* TODO: handle DSC+DP case... */`) still refers to the old behavior. Since this patch explicitly skips DP (`!dp_intf`) in the new compression branch, the TODO is still valid, but it might be worth noting in the commit message that DSC+DP remains unhandled.

**Summary:** The patch is a logical improvement, aligning the INTF data width calculation with the downstream approach using actual DSC byte counts. The main point to verify is whether the divide-by-6 for widebus video mode is correct for the INTF timing engine, given that the DSI host uses divide-by-3 for the same scenario.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dpu: calculate data_width like downstream

[Claude Code Review Bot]

Overall Series Review

Subject: drm/msm/dpu: calculate data_width like downstream
Author: Alexander Koskovich <akoskovich@pm.me>
Patches: 2
Reviewed: 2026-03-25T08:16:31.541245

---

This is a single RFC patch that changes how `data_width` is calculated for the INTF timing engine when DSC compression is enabled on DSI. Instead of deriving `data_width` from the already-compressed `p->width` (which was divided by the compression ratio), it uses `dce_bytes_per_line` (= `slice_count * slice_chunk_size`) divided by 3 or 6 depending on widebus.

The approach is reasonable and aligns with downstream, but the patch has some issues worth noting.

---
Generated by Claude Code Patch Reviewer

Claude review: drm/msm/dpu: calculate data_width like downstream

[Claude Code Review Bot]

Patch Review

**Concept is sound but needs refinement:**

The core idea — using `dce_bytes_per_line` to compute `data_width` independently from the compressed `timing->width` — is a valid approach. The existing code relies on `p->width` which was already integer-divided by the compression ratio (potentially losing precision), then optionally halved for widebus. Using the byte-level calculation with `DIV_ROUND_UP` avoids that compound rounding error.

**Issue 1: Missing `dsc` pointer in `drm_mode_to_intf_timing_params`**

The patch adds:
```c
timing->dce_bytes_per_line = msm_dsc_get_bytes_per_line(dsc);
```

But looking at the existing code at `dpu_encoder_phys_vid.c:130-131`, the `dsc` pointer is a local variable obtained via `dpu_encoder_get_dsc_config()` inside the `if (compression_en)` block. The patch places the new line at line 139 (after `timing->xres = timing->width`), which is within scope, so this is fine. However, the diff context shows it placed correctly inside the existing `if` block.

**Issue 2: Magic divisors 3 and 6 need explanation**

```c
if (p->wide_bus_en)
    data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 6);
else
    data_width = DIV_ROUND_UP(p->dce_bytes_per_line, 3);
```

The divisor `3` represents 3 bytes per pixel (24bpp), and `6` is `3 * 2` (widebus doubles the data rate). These magic numbers should have a brief comment or use named constants. Compare with `dsi_host.c:1033` which uses `bytes_per_pclk = 3` / `bytes_per_pclk = 6` — the same pattern but at least assigned to a named variable.

**Issue 3: Interaction with the existing `timing->width` compression adjustment**

The existing code at `dpu_encoder_phys_vid.c:136-138` still compresses `timing->width` using integer division:
```c
timing->width = timing->width * drm_dsc_get_bpp_int(dsc) /
                (dsc->bits_per_component * 3);
```

This compressed `p->width` is still used to initialize `data_width` at `dpu_hw_intf.c:173`:
```c
data_width = p->width;
```

...before being overridden by the new `dce_bytes_per_line` calculation. This works but is a bit misleading — the initial assignment is immediately dead for the compression case. A comment noting this would help readability.

**Issue 4: DP case handling inconsistency**

The new code checks `!dp_intf` for the compression case:
```c
if (p->compression_en && !dp_intf) {
```

This is consistent with the existing comment `/* TODO: handle DSC+DP case */` and matches the guard on the existing `wide_bus_en` check. Good.

**Issue 5: The `dce_bytes_per_line` field is only set for video mode**

The field is set in `drm_mode_to_intf_timing_params()` in `dpu_encoder_phys_vid.c`, but command mode uses a different path (`dpu_encoder_phys_cmd.c`). If command mode ever needs this calculation, the field won't be populated. This is acceptable for an RFC since the existing compression path is also only in the vid encoder, but worth noting.

**Issue 6: Values sanity check**

From the cover letter: for a panel with `dce_bytes_per_line`, the values shift from 288→360 (no widebus) and 144→180 (widebus). The ratio 360/288 = 1.25 and 180/144 = 1.25. This means the old integer-division formula was under-counting by 20%, which is a significant error that could cause display artifacts or underflow. The fix seems directionally correct.

**Minor: RFC status**

The author explicitly marks this as RFC and asks for Qualcomm feedback, which is appropriate given the hardware-specific nature of the timing calculation. The patch depends on other unmerged DSI fixes listed in the cover letter, which should be noted as dependencies.

---
Generated by Claude Code Patch Reviewer