From mboxrd@z Thu Jan  1 00:00:00 1970
From: Claude Code Review Bot <claude-review@example.com>
To: dri-devel-reviews@example.com
Subject: Claude review: drm/msm/dpu: calculate data_width like downstream
Date: Wed, 25 Mar 2026 08:12:58 +1000
Message-ID: <review-patch1-20260322-fix-data-width-calc-v2-1-d5f28136bc4e@pm.me>
In-Reply-To: <20260322-fix-data-width-calc-v2-1-d5f28136bc4e@pm.me>
References: <20260322-fix-data-width-calc-v2-1-d5f28136bc4e@pm.me> <20260322-fix-data-width-calc-v2-1-d5f28136bc4e@pm.me>
X-Mailer: Claude Code Patch Reviewer
Content-Type: text/plain; charset="utf-8"
Content-Transfer-Encoding: quoted-printable
MIME-Version: 1.0

Patch Review

**Positive aspects:**
- Uses the established `msm_dsc_get_bytes_per_line()` helper already used i=
n `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 =
`=3D { 0 }`) and only read when `compression_en` is set.
- The expanded comment block is a welcome improvement over the previous ter=
se comment.

**Concern =E2=80=94 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 =3D 6;
else
    bytes_per_pclk =3D 3;
```

For **video mode** with widebus, DSI host uses `bytes_per_pclk =3D 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 =3D 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 =E2=80=94 but the reviewer should verify that the INTF timing eng=
ine truly expects the `data_width` to be halved for widebus even in video m=
ode with DSC. It would be worth having a comment explaining why the INTF wi=
debus behavior differs from the DSI host here, or citing the downstream sou=
rce/hardware documentation that confirms the divide-by-6.

**Minor =E2=80=94 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 "Th=
e existing formula using `drm_dsc_get_bpp_int()` loses fractional BPP preci=
sion and doesn't account for actual slice geometry" would help reviewers un=
derstand the motivation.

**Nit =E2=80=94 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 t=
he new compression branch, the TODO is still valid, but it might be worth n=
oting in the commit message that DSC+DP remains unhandled.

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

---
Generated by Claude Code Patch Reviewer