Re: [PATCH] drm/bridge: dw-mipi-dsi: use the lane rate for lbcc when lanes run faster

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] drm/bridge: dw-mipi-dsi: wrong lane count in dw_mipi_dsi_get_hcomponent_lbcc()
- [Medium] drm/bridge: dw-mipi-dsi: 1/8 margin check in dw_mipi_dsi_get_hcomponent_lbcc()
--

commit 6499adefdc008cafd18dfe3208439c2abd0174aa
Author: Romain TISSERAND <romain.tisserand@gmail.com>

drm/bridge: dw-mipi-dsi: use the lane rate for lbcc when lanes run faster

This commit updates dw_mipi_dsi_get_hcomponent_lbcc() to calculate horizontal
timing values from the DSI lane rate rather than the pixel clock whenever the
lane rate exceeds the pixel bandwidth by more than 1/8, fixing DPI payload
FIFO overflows on glues such as Rockchip that run the lanes faster in
non-burst video modes.

> diff --git a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> index 22d1b8cbe1d57..b026b602abc0f 100644
> --- a/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> +++ b/drivers/gpu/drm/bridge/synopsys/dw-mipi-dsi.c
> @@ -781,19 +781,28 @@ static u32 dw_mipi_dsi_get_hcomponent_lbcc(struct dw_mipi_dsi *dsi,
>  					   u32 hcomponent)
>  {
>  	u32 frac, lbcc, minimum_lbcc;
> +	u64 pixel_kbps;
>  	int bpp;
>  
> -	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST) {
> +	bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> +	if (bpp < 0) {
> +		dev_err(dsi->dev, "failed to get bpp\n");
> +		return 0;
> +	}
> +
> +	/*
> +	 * Use the lane rate when it is above the pixel bandwidth (burst, or
> +	 * glues adding margin in all modes); the 1/8 slack ignores lane_mbps
> +	 * being rounded up to the next MHz. Otherwise the pixel clock is more
> +	 * precise.
> +	 */
> +	pixel_kbps = div_u64((u64)mode->clock * bpp, dsi->lanes);
> +	if (dsi->mode_flags & MIPI_DSI_MODE_VIDEO_BURST ||
> +	    (u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8) {

[Severity: Medium]
Can the 1/8 margin check miss panels where the lane rate runs faster than the
pixel bandwidth by less than 12.5%?

In drivers/gpu/drm/rockchip/dw-mipi-dsi-rockchip.c,
dw_mipi_dsi_get_lane_mbps() computes the default lane rate using integer
division for (bpp / lanes):

	mpclk = DIV_ROUND_UP(mode->clock, MSEC_PER_SEC);
	if (mpclk) {
		/* take 1 / 0.8, since mbps must big than bandwidth of RGB */
		tmp = mpclk * (bpp / lanes) * 10 / 8;

For a 4-lane non-burst panel using MIPI_DSI_FMT_RGB666_PACKED (bpp = 18),
(bpp / lanes) truncates from 4.5 to 4, giving a lane rate of 5 * mpclk Mbps.
That is only 11.1% (1/9) above the 4.5 * mode->clock pixel bandwidth, rather
than 25%.

For any pixel clock >= 16 MHz on such a panel (or when a panel's requested
hs_rate or max_mbps clamp puts dsi->lane_mbps between +1 MHz and +12.5% above
pixel_kbps), (u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8
evaluates to false. Wouldn't that fall back to the pixel-clock formula and
program DSI_VID_HLINE_TIME, DSI_VID_HSA_TIME, and DSI_VID_HBP_TIME shorter
than the DPI line period, triggering the same DPI payload FIFO overflow?

If the slack is only meant to ignore lane_mbps being rounded up to the next
MHz, should the check compare dsi->lane_mbps against
DIV_ROUND_UP_ULL(pixel_kbps, MSEC_PER_SEC) instead of adding pixel_kbps / 8?

>  		/* lbcc based on lane_mbps */
>  		lbcc = hcomponent * dsi->lane_mbps * MSEC_PER_SEC / 8;
>  	} else {
>  		/* lbcc based on pixel clock rate */
> -		bpp = mipi_dsi_pixel_format_to_bpp(dsi->format);
> -		if (bpp < 0) {
> -			dev_err(dsi->dev, "failed to get bpp\n");
> -			return 0;
> -		}
> -
>  		lbcc = div_u64((u64)hcomponent * mode->clock * bpp, dsi->lanes * 8);

[Severity: Medium]
Should pixel_kbps and the pixel-clock lbcc calculation divide by
dw_mipi_dsi_get_lanes(dsi) instead of dsi->lanes to handle dual-DSI panels?

In dw_mipi_dsi_bridge_atomic_pre_enable() -> dw_mipi_dsi_mode_set(),
dsi->lane_mbps is obtained by passing the combined master and slave lane
count from dw_mipi_dsi_get_lanes(dsi) along with the full-panel
adjusted_mode:

	u32 lanes = dw_mipi_dsi_get_lanes(dsi);

	clk_prepare_enable(dsi->pclk);

	ret = phy_ops->get_lane_mbps(priv_data, adjusted_mode, dsi->mode_flags,
				     lanes, dsi->format, &dsi->lane_mbps);

When a dual-DSI panel is attached in non-burst video mode via
dw_mipi_dsi_set_slave() (such as innolux,p097pfg on rk3399-gru-scarlet with
4 + 4 lanes), dsi->lane_mbps is the per-lane rate across all 8 lanes, whereas
dividing mode->clock * bpp by dsi->lanes (4) makes pixel_kbps twice the
actual per-lane pixel bandwidth.

Because pixel_kbps is doubled, the condition
(u64)dsi->lane_mbps * MSEC_PER_SEC > pixel_kbps + pixel_kbps / 8 evaluates to
false, and the else branch then also divides by dsi->lanes (4) instead of
dw_mipi_dsi_get_lanes(dsi) (8), programming DSI_VID_HLINE_TIME,
DSI_VID_HSA_TIME, and DSI_VID_HBP_TIME for twice the per-lane pixel bandwidth
rather than the actual lane rate.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261025142639.3454322-1-romain.tisserand@gmail.com?part=1