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:16:31 +1000
Message-ID: <review-patch1-20260322-fix-data-width-calc-v1-1-128880f5a067@pm.me>
In-Reply-To: <20260322-fix-data-width-calc-v1-1-128880f5a067@pm.me>
References: <20260322-fix-data-width-calc-v1-1-128880f5a067@pm.me> <20260322-fix-data-width-calc-v1-1-128880f5a067@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

**Concept is sound but needs refinement:**

The core idea =E2=80=94 using `dce_bytes_per_line` to compute `data_width` =
independently from the compressed `timing->width` =E2=80=94 is a valid appr=
oach. The existing code relies on `p->width` which was already integer-divi=
ded by the compression ratio (potentially losing precision), then optionall=
y 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 =3D 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 =3D timing->width`), which is within scope, s=
o 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 =3D DIV_ROUND_UP(p->dce_bytes_per_line, 6);
else
    data_width =3D DIV_ROUND_UP(p->dce_bytes_per_line, 3);
```

The divisor `3` represents 3 bytes per pixel (24bpp), and `6` is `3 * 2` (w=
idebus doubles the data rate). These magic numbers should have a brief comm=
ent or use named constants. Compare with `dsi_host.c:1033` which uses `byte=
s_per_pclk =3D 3` / `bytes_per_pclk =3D 6` =E2=80=94 the same pattern but a=
t least assigned to a named variable.

**Issue 3: Interaction with the existing `timing->width` compression adjust=
ment**

The existing code at `dpu_encoder_phys_vid.c:136-138` still compresses `tim=
ing->width` using integer division:
```c
timing->width =3D 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 =3D p->width;
```

...before being overridden by the new `dce_bytes_per_line` calculation. Thi=
s works but is a bit misleading =E2=80=94 the initial assignment is immedia=
tely dead for the compression case. A comment noting this would help readab=
ility.

**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 sh=
ift from 288=E2=86=92360 (no widebus) and 144=E2=86=92180 (widebus). The ra=
tio 360/288 =3D 1.25 and 180/144 =3D 1.25. This means the old integer-divis=
ion formula was under-counting by 20%, which is a significant error that co=
uld cause display artifacts or underflow. The fix seems directionally corre=
ct.

**Minor: RFC status**

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

---
Generated by Claude Code Patch Reviewer