Re: [PATCH v5 14/15] drm/i915/alpm: Compute and program switch to active latency

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Severe integer arithmetic precision loss in `get_switch_to_active` calculates a latency of 0 for UHBR20 links and incorrect values for other UHBR links.
- [Medium] Missing bounds validation for the computed `switch_to_active` scanline value.
--

--- Patch [14]: [PATCH v5 14/15] drm/i915/alpm: Compute and program switch to active latency ---
commit c70c27fb353539a13b15cf08616e266e5b7146a1
Author: Animesh Manna <animesh.manna@intel.com>

drm/i915/alpm: Compute and program switch to active latency

This commit programs the ALPM switch-to-active latency into ALPM_CTL2. For
UHBR link rates, the latency is derived from the link symbol clock and
converted to scanlines. For non-UHBR rates, the latency is zero.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 8c17da9883d6d..5d2ac75615670 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c

[ ... ]

> @@ -270,6 +270,31 @@ static int get_establishment_period(struct intel_dp *intel_dp,
>  	return establishment_period;
>  }
>  
> +
> +static int get_switch_to_active(const struct intel_crtc_state *crtc_state)
> +{
> +	int port_clock = crtc_state->port_clock;
> +	int switch_to_active;
> +
> +	if (intel_dp_is_uhbr(crtc_state)) {
> +		/* symbol_clock (fLink) in MHz */
> +		int symbol_clock = port_clock / intel_dp_link_symbol_size(port_clock) / 100;
> +
> +		/*
> +		 * tSwitch to Active = 32 * (ML_PHY_LOCK Length + 3 + 64) / fLink
> +		 *
> +		 * The "+ 3" term is the trailing zero padding after the
> +		 * POST_LT_SCRAMBLER_RESET, the "+ 64" term represents the MTP
> +		 * time slots. The result is in microseconds.
> +		 */
> +		switch_to_active = 32 * ((396 + 3 + 64) / symbol_clock);

[Severity: High]
Does this calculation lose precision by performing the division before the
multiplication?

For a UHBR20 link where symbol_clock is 625 MHz, the integer division
(396 + 3 + 64) / 625 will yield 0, resulting in a switch_to_active latency
of 0.

For UHBR10 (312 MHz) and UHBR13.5 (421 MHz), the division will yield 1,
making the result 32 instead of the mathematically correct values of ~47
and ~35, respectively.

Should this multiply before dividing, perhaps using DIV_ROUND_UP()?

> +	} else {
> +		switch_to_active = 0;
> +	}
> +
> +	return switch_to_active;
> +}
> +
>  /*
>   * AUX-Less Wake Time = CEILING( ((PHY P2 to P0) + tLFPS_Period, Max+
>   * tSilence, Max+ tPHY Establishment + tCDS) / tline)

[ ... ]

> @@ -329,7 +354,8 @@ _lnl_compute_aux_less_alpm_params(struct intel_dp *intel_dp,
>  	crtc_state->alpm_state.aux_less_wake_lines = aux_less_wake_lines;
>  	crtc_state->alpm_state.silence_period_sym_clocks = silence_period;
>  	crtc_state->alpm_state.lfps_half_cycle_num_of_syms = lfps_half_cycle;
> -
> +	crtc_state->alpm_state.switch_to_active = intel_usecs_to_scanlines(&crtc_state->hw.adjusted_mode,
> +									   get_switch_to_active(crtc_state));

[Severity: Medium]
Is it possible for the computed scanline value to exceed the hardware limits?

Earlier in _lnl_compute_aux_less_alpm_params(), values like
aux_less_wake_lines and silence_period are checked against their hardware
bounds, and the function returns false if they exceed limits to safely reject
the configuration.

However, switch_to_active is programmed into a 5-bit register field
(ALPM_CTL2_SWITCH_TO_ACTIVE_LATENCY_MASK). If the converted scanline value
exceeds 31, it will be silently truncated when passed to the macro in
lnl_alpm_configure().

Should there be a bounds check for switch_to_active before applying it?

>  	return true;
>  }

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