Re: [PATCH] drm/i915/dsi: Fix command mode line time calculation

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [Low] DIV_ROUND_UP_ULL is incorrectly used with a 64-bit divisor and a 32-bit numerator, which violates the underlying macro contract.

Pre-existing issues:
- [High] The command mode line time calculation fails to account for dual-link DSI panel bandwidth scaling, resulting in a computed line time that is exactly twice the physical line time and halving the requested vertical blanking period.
--

--- Patch [1]: [PATCH] drm/i915/dsi: Fix command mode line time calculation ---
commit edcb713539337dd734175d1e81417b69dd057925
Author: Jinman Ma <[email protected]>

drm/i915/dsi: Fix command mode line time calculation

This commit changes the line time calculation for command mode DSI panels
from microseconds to nanoseconds. This prevents divide-by-zero errors during
atomic commits at high link rates.

> diff --git a/drivers/gpu/drm/i915/display/icl_dsi.c b/drivers/gpu/drm/i915/display/icl_dsi.c
> --- a/drivers/gpu/drm/i915/display/icl_dsi.c
> +++ b/drivers/gpu/drm/i915/display/icl_dsi.c
> @@ -917,16 +917,19 @@ gen11_dsi_set_transcoder_timings(struct intel_encoder *encoder,
>  	if (is_vid_mode(intel_dsi)) {
>  		vtotal = adjusted_mode->crtc_vtotal;
>  	} else {
> -		int bpp, line_time_us, byte_clk_period_ns;
> +		u64 line_time_ns;
> +		int bpp;
>  
>  		if (crtc_state->dsc.compression_enable)
>  			bpp = fxp_q4_to_int(crtc_state->dsc.compressed_bpp_x16);
>  		else
>  			bpp = mipi_dsi_pixel_format_to_bpp(intel_dsi->pixel_format);
>  
> -		byte_clk_period_ns = 1000000 / afe_clk(encoder, crtc_state);
> -		line_time_us = (htotal * (bpp / 8) * byte_clk_period_ns) / (1000 * intel_dsi->lane_count);
> -		vtotal = vactive + DIV_ROUND_UP(400, line_time_us);
> +		line_time_ns = DIV_ROUND_UP_ULL((u64)htotal * bpp * 1000000,
> +						afe_clk(encoder, crtc_state) *
> +						intel_dsi->lane_count);

[Severity: High]
This is a pre-existing issue, but does this calculation fail to account for
dual-link DSI panel bandwidth scaling?

Because the calculation uses the full htotal against the single-port
bandwidth, and afe_clk is already halved for dual-link setups during VBT
parsing, this results in a computed line_time_ns that is exactly twice the
physical line time.

Further down in gen11_dsi_set_transcoder_timings(), htotal is properly
halved for dual-link configurations:

    if (intel_dsi->dual_link) {
        hactive /= 2;
        if (intel_dsi->dual_link == DSI_DUAL_LINK_FRONT_BACK)
            hactive += intel_dsi->pixel_overlap;
        htotal /= 2;
    }

Since this htotal halving happens after line_time_ns is calculated, could
this be requesting half the necessary blanking lines, providing ~200 us
of vertical blanking time instead of the required 400 us? This might explain
the persistent tearing and jitter mentioned in the commit message.

> +		vtotal = vactive +
> +			DIV_ROUND_UP_ULL(400 * 1000, line_time_ns);

[Severity: Low]
Does this usage of DIV_ROUND_UP_ULL() reverse the expected types for the
numerator and denominator?

The DIV_ROUND_UP_ULL() macro is designed for a 64-bit numerator and a 32-bit
denominator. When do_div() is used internally on 32-bit architectures, it
implicitly truncates the denominator to 32 bits.

While line_time_ns easily fits within 32 bits here, so there is no
functional impact, is there a risk of violating the underlying macro
contract by using a 32-bit numerator and a 64-bit divisor?

>  	}
>  	vsync_start = adjusted_mode->crtc_vsync_start;
>  	vsync_end = adjusted_mode->crtc_vsync_end;

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1