Claude review: drm/msm/dsi: fix hdisplay calculation when programming dsi registers

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

Patch Review

The commit message states the fix is derived by "cross-comparing the calculation with the calculation in `dsi_adjust_pclk_for_compression()`." However, `dsi_adjust_pclk_for_compression` uses `dsc->bits_per_component * 3` as the uncompressed bits-per-pixel:

```c
int new_hdisplay = DIV_ROUND_UP(mode->hdisplay * drm_dsc_get_bpp_int(dsc),
		dsc->bits_per_component * 3);
```

The patch instead uses `mipi_dsi_pixel_format_to_bpp(msm_host->format)`:

> +		if (wide_bus_enabled)
> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

For a 10bpc panel (the scenario the commit message highlights as broken), the DSI pixel format is still `MIPI_DSI_FMT_RGB888`, so `mipi_dsi_pixel_format_to_bpp` returns 24. But `bits_per_component * 3` would be 30. These two functions would compute different `hdisplay` values for the same panel. If the intent is to match `dsi_adjust_pclk_for_compression`, shouldn't this use `dsc->bits_per_component * 3` instead? Could you clarify which divisor is actually correct for the timing register programming, and why it differs from the pclk adjustment function?

Regarding the removal of the video mode guard:

> -		if (wide_bus_enabled && !(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO))
> -			bytes_per_pclk = 6;
> +		if (wide_bus_enabled)
> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

The old code intentionally applied widebus only for CMD mode (not video). The commit message says "Since CMD mode does not use this, we can remove `!(msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)` safely." But the guard was about preventing widebus from affecting video mode, not about CMD mode not reaching this code. Looking at the code, CMD mode does reach this path -- the `if (msm_host->dsc)` block runs before the `if (msm_host->mode_flags & MIPI_DSI_MODE_VIDEO)` / `else` split, so both video and CMD mode execute the hdisplay calculation. For CMD mode with DSC, the `hdisplay` value computed here feeds into `DSI_CMD_MDP_STREAM0_TOTAL_H_TOTAL`. Could you explain why it is now safe to apply widebus bits-per-pclk to video mode when it was previously deliberately excluded?

One minor concern with the error return from `mipi_dsi_pixel_format_to_bpp`:

> +			bits_per_pclk = mipi_dsi_pixel_format_to_bpp(msm_host->format);

`mipi_dsi_pixel_format_to_bpp` returns `-EINVAL` for unrecognized formats. Since `bits_per_pclk` is `u32`, this would wrap to `~0U`, causing the `DIV_ROUND_UP` to yield 1 for `hdisplay`. The format should always be valid by this point (it was accepted during `dsi_dev_attach`), so this is not a practical bug, but it is worth noting that no existing caller in this function checks the return value either.

The non-widebus path is algebraically equivalent to the old code:

> +		else
> +			bits_per_pclk = 24;
>
> -		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc), bytes_per_pclk);
> +		hdisplay = DIV_ROUND_UP(msm_dsc_get_bytes_per_line(msm_host->dsc) * 8, bits_per_pclk);

`DIV_ROUND_UP(bytes * 8, 24)` simplifies to `DIV_ROUND_UP(bytes, 3)`, which matches the old `DIV_ROUND_UP(bytes, 3)`. No issue there.

The updated comment is better at describing the actual hardware behavior:

> +		 * enabled, MDP always sends out 48-bit compressed data per
> +		 * pclk and on average, DSI consumes an amount of compressed
> +		 * data equivalent to the uncompressed pixel depth per pclk.

This describes MDP sending 48 bits and DSI consuming at the uncompressed pixel depth rate. If the uncompressed pixel depth for 10bpc is indeed meant to be 24 (the DSI bus format bpp) rather than 30 (`bpc * 3`), this comment makes sense but then `dsi_adjust_pclk_for_compression` is arguably the one with the inconsistency. The commit message's claim about cross-comparing with that function then needs qualification.

In summary: the main question is whether `mipi_dsi_pixel_format_to_bpp` or `bits_per_component * 3` is the correct divisor, especially for 10bpc panels. The patch should either use the same formula as `dsi_adjust_pclk_for_compression` (for consistency) or explain why they should differ. Also, the commit message's explanation for removing the video mode guard doesn't quite track with the code structure and could use clarification.

---
Generated by Claude Code Patch Reviewer