Re: [PATCH v5 15/15] drm/i915/alpm: Program zero-based LFPS half cycle duration

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Calculation of LFPS half cycle clocks incorrectly uses the link bit rate instead of the symbol clock, breaking UHBR configurations.
- [High] Signed 32-bit integer overflow in `get_establishment_period()` causes ALPM wake times to be calculated as negative values.
- [High] Premature integer division in `get_switch_to_active()` causes switch-to-active latency to incorrectly evaluate to 0 for UHBR20.
--

--- Patch [15]: [PATCH v5 15/15] drm/i915/alpm: Program zero-based LFPS half cycle duration ---
commit 6cd79e290a7ff8352dbda337c2c68d7456099444
Author: Animesh Manna <animesh.manna@intel.com>

drm/i915/alpm: Program zero-based LFPS half cycle duration

The LFPS Half Cycle Duration field, as well as the First and Last LFPS
Half Cycle Duration fields, in PORT_ALPM_LFPS_CTL are zero-based: per
Bspec the value programmed must be N_Symbol - 1. get_lfps_half_cycle_clocks()
returned N_Symbol and programmed it directly into all three fields, making
each half cycle one symbol clock too long.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 5d2ac75615670..9768279f94ef5 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c
> @@ -123,8 +123,16 @@ static int get_lfps_cycle_count(const struct intel_crtc_state *crtc_state)
>  
>  static int get_lfps_half_cycle_clocks(const struct intel_crtc_state *crtc_state)
>  {
> -	return get_lfps_cycle_time(crtc_state) * crtc_state->port_clock / 1000 /
> -		1000 / (2 * get_lfps_cycle_count(crtc_state));
> +	int n_symbols = get_lfps_cycle_time(crtc_state) * crtc_state->port_clock /
> +			1000 / 1000 / (2 * get_lfps_cycle_count(crtc_state));

[Severity: High]
This is a pre-existing issue, but does this correctly calculate the frequency
multiplier for UHBR configurations?

For UHBR encoding (128b/132b), the symbol clock in kHz is
port_clock * 10 / 32, not port_clock. When used for UHBR20, using the
inflated bit rate results in n_symbols - 1 = 59, which exceeds
PORT_ALPM_LFPS_CTL_LAST_LFPS_HALF_CYCLE_DURATION_MASK (31).

Could this cause validation to fail and completely disable ALPM on UHBR
displays?

> +
> +	/*
> +	 * The LFPS half cycle duration fields (including the first and last
> +	 * half cycle) are zero-based, i.e. the value programmed is N_Symbol - 1.
> +	 *
> +	 * Bspec: 71632
> +	 */
> +	return n_symbols - 1;
>  }

[ ... ]

> static int get_establishment_period(struct intel_dp *intel_dp,
> 				    const struct intel_crtc_state *crtc_state)
> {
[ ... ]
> 	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 / 10;

[Severity: High]
This isn't a bug introduced by this patch, but can this intermediate
expression overflow?

The sub-expression "1000 * 1000 * tps4" evaluates using signed 32-bit
arithmetic. For non-UHBR links, tps4 is 2520, making the result
2,520,000,000. Since this exceeds INT_MAX (2,147,483,647), it wraps around
to -1,774,967,296.

Does this negative value propagate through to the final establishment_period,
causing incorrect ALPM hardware programming?

[ ... ]

> static int get_switch_to_active(const struct intel_crtc_state *crtc_state)
> {
[ ... ]
> 	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]
This is a pre-existing issue, but does this integer division evaluate to 0
for UHBR20 links?

The expression "396 + 3 + 64" equals 463. For a UHBR20 link, symbol_clock is
625 MHz. The integer division 463 / 625 truncates to 0, resulting in an
overall latency of 0 scanlines instead of the intended ~23 microseconds.

Could this zero-latency configuration break the link state machine recovery
for UHBR20 displays?

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