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

[Claude Code Review Bot <claude-review@example.com>]

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