Re: [PATCH v5 06/15] drm/i915/alpm: Auxless wake time calculation for Xe3p

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Severe 32-bit signed integer overflow in `tml_phy_lock` calculation results in negative time values.
- [Medium] Missing error check on `drm_dp_lttpr_count()` return value allows negative error codes to corrupt ALPM time calculations.
- [Medium] The bounds check for `aux_less_wake_lines` does not use the expanded Xe3LPD mask, artificially limiting the maximum wake lines and potentially disabling ALPM on newer hardware.
--

--- Patch [6]: [PATCH v5 06/15] drm/i915/alpm: Auxless wake time calculation for Xe3p ---
commit d691d0d2ce1f7d50c7d92a8c206ccf76b71ba2ce
Author: Animesh Manna <animesh.manna@intel.com>
Subject: drm/i915/alpm: Auxless wake time calculation for Xe3p

This commit adds support for auxless waketime calculation for DP2.1 ALPM,
updating dependent parameters.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 4b15d96d0ad79..af2733805505e 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -105,21 +105,50 @@ static int get_lfps_half_cycle_clocks(const struct intel_crtc_state *crtc_state)
>  		1000 / (2 * LFPS_CYCLE_COUNT);
>  }
>  
> +#define ML_PHY_LOCK_LEN		252
> +#define ML_PHY_LOCK_LEN_UHBR	396
> +
>  static int get_tphy2_p2_to_p0(const struct intel_crtc_state *crtc_state)
>  {
> -	return 12 * 1000;
> +	struct intel_display *display = to_intel_display(crtc_state);
> +
> +	return DISPLAY_VER(display) >= 35 ? (20 * 1000) : (12 * 1000);
>  }
>  
> -static int get_establishment_period(const struct intel_crtc_state *crtc_state)
> +static int get_establishment_period(struct intel_dp *intel_dp,
> +				    const struct intel_crtc_state *crtc_state)
>  {
>  	int t1 = 50 * 1000;
> -	int tps4 = 252;
> +	int tps4 = intel_dp_is_uhbr(crtc_state) ? (ML_PHY_LOCK_LEN_UHBR * 32) :
> +		   (ML_PHY_LOCK_LEN * 10);
>  	/* port_clock is link rate in 10kbit/s units */
> -	int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock;
> +	int tml_phy_lock = 1000 * 1000 * tps4 / crtc_state->port_clock / 10;

[Severity: High]
Does 1000 * 1000 * tps4 overflow the 32-bit signed integer before division?

For DP 2.1 UHBR, tps4 evaluates to 396 * 32 = 12672, yielding a product of
12,672,000,000. This exceeds the maximum 32-bit signed integer (2,147,483,647)
and results in a negative intermediate value.

This negative value will propagate through the division and subsequent time
formulas, corrupting the wake times programmed into the hardware.

> +	int lttpr_count = 0;
>  	int tcds, establishment_period;
>  
> -	tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * tml_phy_lock;
> -	establishment_period = (SILENCE_PERIOD_TIME + t1 + tcds);
> +	if (intel_crtc_has_type(crtc_state, INTEL_OUTPUT_EDP)) {
> +		tcds = (7 + DIV_ROUND_UP(6500, tml_phy_lock) + 1) * tml_phy_lock;
> +	} else {
> +		tcds = 7 * tml_phy_lock;
> +		lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps);
> +	}
> +
> +	if (lttpr_count) {

[Severity: Medium]
Can drm_dp_lttpr_count() return a negative error code here?

If drm_dp_lttpr_count() returns a negative error code (e.g., -EINVAL or
-ERANGE), lttpr_count will evaluate to true in this check.

The code will then proceed into this block and use the negative value in
arithmetic like (lttpr_count - 1), causing a massive underflow in the
calculation of tdcs, tds, and establishment_period.

Should this explicitly check if (lttpr_count > 0)?

> +		int tlw = 13000;
> +		int tcs = 10000;
> +		int tlfps_period = get_lfps_cycle_time(crtc_state);
> +		int tdcs = (SILENCE_PERIOD_TIME + t1 + tcs +
> +			    (lttpr_count - 1) * (tlw + tlfps_period));
> +		int tacds = 70000;
> +		int tds = (lttpr_count - 1) * 7 * tml_phy_lock;
> +
> +		/* tdrl is same as tcds*/
> +		establishment_period = tlw + tlfps_period + tdcs + tacds + tds + tcds;
> +	} else {
> +		/* TODO: Add a check for data realign by DPCD 0x116[3] */
> +
> +		establishment_period = (SILENCE_PERIOD_TIME + t1 + tcds);
> +	}
>  
>  	return establishment_period;
>  }

[ ... ]

> @@ -161,7 +194,7 @@ _lnl_compute_aux_less_alpm_params(struct intel_dp *intel_dp,
>  		lfps_half_cycle;
>  
>  	aux_less_wake_time =
> -		_lnl_compute_aux_less_wake_time(crtc_state);
> +		_lnl_compute_aux_less_wake_time(intel_dp, crtc_state);
>  	aux_less_wake_lines = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode,
>  						       aux_less_wake_time);
>  	silence_period = get_silence_period_symbols(crtc_state);

[Severity: Medium]
Does the bounds check further down in this function still use the legacy mask?

Looking at the rest of _lnl_compute_aux_less_alpm_params():

    if (aux_less_wake_lines > ALPM_CTL_AUX_LESS_WAKE_TIME_MASK ||
        ...
        return false;

    if (display->params.psr_safest_params)
        aux_less_wake_lines = ALPM_CTL_AUX_LESS_WAKE_TIME_MASK;

Since this patch introduces ALPM_CTL_AUX_LESS_WAKE_TIME_MASK_XE3LPD for
platforms with DISPLAY_VER >= 35, will these checks artificially limit the
maximum wake lines to the older 6-bit mask (63) and potentially disable ALPM
on newer hardware?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260825054536.3455704-1-animesh.manna@intel.com?part=6