Re: [PATCH v5 13/15] drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Source/sink protocol mismatch for PSR2 panels with AUX-less capability.
- [High] State leakage of the `has_alpm` flag forces ALPM on unsupported PSR1 fallbacks.
- [High] Inconsistent ALPM wake line validation causes unsafe late atomic state downgrades.
--

--- Patch [13]: [PATCH v5 13/15] drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf ---
commit dd0c0bc3be20db88d8488b6083be9663dc32cc6d
Author: Animesh Manna <animesh.manna@intel.com>
Subject: drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf

For DP2.1, ALPM is optional and there can be scenario where panel replay
will be supported without ALPM. So decouple ALPM feature by has_alpm flag
from related display features.

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

[ ... ]

> @@ -575,8 +569,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp,
>  	enum transcoder cpu_transcoder = crtc_state->cpu_transcoder;
>  	u32 alpm_ctl, alpm_ctl2, lttpr_count;
>  
> -	if (DISPLAY_VER(display) < 20 || (!intel_psr_needs_alpm(intel_dp, crtc_state) &&
> -					  !crtc_state->has_lobf))
> +	if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm)
>  		return;
>  
>  	mutex_lock(&intel_dp->alpm.lock);
> @@ -584,7 +577,7 @@ static void lnl_alpm_configure(struct intel_dp *intel_dp,
>  	 * Panel Replay on eDP is always using ALPM aux less. I.e. no need to
>  	 * check panel support at this point.
>  	 */
> -	if (intel_alpm_is_alpm_aux_less(intel_dp, crtc_state)) {
> +	if (intel_alpm_aux_less_wake_supported(intel_dp)) {
>  		alpm_ctl = ALPM_CTL_ALPM_ENABLE |
>  			ALPM_CTL_ALPM_AUX_LESS_ENABLE |
>  			ALPM_CTL_AUX_LESS_SLEEP_HOLD_TIME_50_SYMBOLS;

[Severity: High]
Does this create a protocol mismatch between the source and the sink?

By changing this to rely on intel_alpm_aux_less_wake_supported(), the source
is now programmed for AUX-less ALPM (ALPM_CTL_ALPM_AUX_LESS_ENABLE) for PSR2
panels that advertise DP_ALPM_AUX_LESS_CAP.

However, intel_alpm_enable_sink() still requires Panel Replay or LOBF to
enable AUX-less mode on the sink. Because neither is true for PSR2, the sink
will be expecting fast-wake ALPM, which conflicts with how the source is
now programmed here.

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 40e3d70959965..8c6aa4e948894 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c

[ ... ]

> @@ -1729,11 +1731,17 @@ static bool _panel_replay_compute_config(struct intel_crtc_state *crtc_state,
>  	crtc_state->link_off_after_as_sdp_when_pr_active = compute_link_off_after_as_sdp_when_pr_active(connector);
>  	crtc_state->disable_as_sdp_when_pr_active = compute_disable_as_sdp_when_pr_active(connector);
>  
> +	if (intel_alpm_is_possible(intel_dp))
> +		crtc_state->has_alpm = alpm_config_valid(intel_dp, crtc_state, true, true, false);
> +
>  	if (!intel_dp_is_edp(intel_dp))
>  		return true;
>  
>  	/* Remaining checks are for eDP only */
>  
> +	if (!crtc_state->has_alpm)
> +		return false;
> +
>  	if (to_intel_crtc(crtc_state->uapi.crtc)->pipe != PIPE_A &&
>  	    to_intel_crtc(crtc_state->uapi.crtc)->pipe != PIPE_B)
>  		return false;

[Severity: High]
Can this leak the has_alpm state if the panel replay config is aborted?

If alpm_config_valid() succeeds, crtc_state->has_alpm is set to true.
However, if a subsequent check in _panel_replay_compute_config() returns
false (such as the PIPE_A/B check or a later HDCP check), has_alpm remains
set to true.

When the driver then falls back to computing PSR1, has_alpm stays true and
bypasses the previous protections that prevented ALPM from being enabled
on PSR1. This forces ALPM on for PSR1 fallbacks where it is not supported.

[ ... ]

> @@ -4547,15 +4542,17 @@ void intel_psr_compute_config_late(struct intel_dp *intel_dp,
>  	int vblank = intel_crtc_vblank_length(crtc_state);
>  	int wake_lines;
>  
> -	if (intel_psr_needs_alpm_aux_less(intel_dp, crtc_state))
> -		wake_lines = crtc_state->alpm_state.aux_less_wake_lines;
> -	else if (intel_psr_needs_alpm(intel_dp, crtc_state))
> -		wake_lines = DISPLAY_VER(display) < 20 ?
> -			     psr2_block_count_lines(crtc_state->alpm_state.io_wake_lines,
> -						    crtc_state->alpm_state.fast_wake_lines) :
> -			     crtc_state->alpm_state.io_wake_lines;
> -	else
> +	if (crtc_state->has_alpm) {
> +		if (intel_alpm_aux_less_wake_supported(intel_dp))
> +			wake_lines = crtc_state->alpm_state.aux_less_wake_lines;
> +		else
> +			wake_lines = DISPLAY_VER(display) < 20 ?
> +				     psr2_block_count_lines(crtc_state->alpm_state.io_wake_lines,
> +							    crtc_state->alpm_state.fast_wake_lines) :
> +				     crtc_state->alpm_state.io_wake_lines;
> +	} else {
>  		wake_lines = 0;
> +	}
>  
>  	/*
>  	 * Disable the PSR features if wake lines exceed the available vblank.

[Severity: High]
Does this late selection of aux_less_wake_lines conflict with the early
validation performed for PSR2?

During early validation, intel_psr2_config_valid() hardcodes the aux_less
parameter to false when calling alpm_config_valid(), so it validates
against the smaller io_wake_lines.

If intel_alpm_aux_less_wake_supported() evaluates to true here in late
compute, it will dynamically select the larger aux_less_wake_lines instead. 
This mismatch can cause the subsequent _wake_lines_fit_into_vblank() check
to fail, triggering a late teardown of has_sel_update after watermarks and
DDB allocations have already been computed assuming PSR/SU was active.

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