Re: [PATCH v6 15/18] 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 5 potential issue(s) to consider:
- [High] drm/i915/alpm: Unconditional overwrite of has_alpm breaks Panel Replay without ALPM
- [High] drm/i915/alpm: Retained eDP check in intel_psr_needs_alpm_aux_less prevents AS SDP for external DP
- [High] drm/i915/alpm: has_alpm state leak when PSR/PR features are downgraded late
- [Medium] drm/i915/alpm: Early return in intel_alpm_port_configure prevents clearing stale ALPM state
- [Medium] drm/i915/alpm: Missing has_alpm check causes incorrect PR_ALPM_CTL programming
--

commit 4dcfef3fd89e0ed18372dabec785892c009d57eb
Author: Animesh Manna <animesh.manna@intel.com>
Subject: drm/i915/alpm: Introduce has_alpm to decouple from pr/psr2/lobf

This commit introduces a single crtc_state->has_alpm flag to track ALPM
enablement, decoupling it from implicit feature tracking like eDP Panel
Replay, PSR2, or LOBF. It updates various ALPM configuration and port
paths to use this new flag as the single source of truth.

> diff --git a/drivers/gpu/drm/i915/display/intel_alpm.c b/drivers/gpu/drm/i915/display/intel_alpm.c
> index 29a93a252fe9b..236341aa3c8d8 100644
> --- a/drivers/gpu/drm/i915/display/intel_alpm.c
> +++ b/drivers/gpu/drm/i915/display/intel_alpm.c

[ ... ]

> @@ -719,7 +720,7 @@ void intel_alpm_pr_as_sdp_update(const struct intel_crtc_state *crtc_state)
>  		intel_dp = enc_to_intel_dp(encoder);
>  
>  		if (!intel_dp->as_sdp_supported ||
> -		    !intel_alpm_is_alpm_aux_less(intel_dp, crtc_state))
> +		    !intel_alpm_is_aux_less(intel_dp, crtc_state))
>  			continue;
>  
>  		mutex_lock(&intel_dp->alpm.lock);

[Severity: Medium]
Does this missing check for crtc_state->has_alpm cause incorrect
PR_ALPM_CTL programming?

The intel_alpm_is_aux_less() function evaluates to true for Panel Replay
regardless of the actual has_alpm state. Without an explicit check verifying
that ALPM is actually active, if Panel Replay is explicitly falling back to
a "without ALPM" configuration, this loop will incorrectly write AS SDP
parameters to the ALPM controller, which contradicts the fallback state.

[ ... ]

> @@ -792,10 +792,10 @@ void intel_alpm_port_configure(struct intel_dp *intel_dp,
>  	enum port port = dp_to_dig_port(intel_dp)->base.port;
>  	u32 alpm_ctl_val = 0, lfps_ctl_val = 0;
>  
> -	if (DISPLAY_VER(display) < 20)
> +	if (DISPLAY_VER(display) < 20 || !crtc_state->has_alpm)
>  		return;

[Severity: Medium]
Will this early return prevent clearing stale ALPM state in the hardware?

Previously, this function would proceed and write the zeroed alpm_ctl_val
and lfps_ctl_val to the hardware when ALPM was disabled. By returning
early when crtc_state->has_alpm is false, any transition from an
ALPM-enabled mode to an ALPM-disabled mode will skip the clearing writes.
This leaves the hardware with an active ALPM configuration while the driver
state considers it disabled.

> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index 872e253db1786..1f14dbe57512f 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
> @@ -1648,6 +1648,8 @@ static bool intel_sel_update_config_valid(struct intel_crtc_state *crtc_state,
>  	crtc_state->enable_psr2_su_region_et = psr2_su_region_et_valid(connector,
>  								       crtc_state->has_panel_replay);
>  
> +	crtc_state->has_alpm = intel_alpm_is_possible(intel_dp);
> +

[Severity: High]
Does this unconditional assignment overwrite the validated has_alpm state?

The Panel Replay configuration path sets has_alpm based on whether ALPM
timing actually fits into the vblank period. When Panel Replay is validated
but ALPM timing fails, has_alpm is correctly set to false to allow Panel
Replay without ALPM.

This line in intel_sel_update_config_valid() unconditionally overwrites
that calculated state with a simple capability check, forcing it back to
true. This destroys the fallback state, causing the late compute config
to evaluate ALPM wake lines again, fail, and fully disable Panel Replay
on external DP.

[ ... ]

> @@ -4555,16 +4569,6 @@ void intel_psr_connector_debugfs_add(struct intel_connector *connector)
>  				    connector, &i915_psr_status_fops);
>  }
>  
> -bool intel_psr_needs_alpm(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state)
> -{
> -	/*
> -	 * eDP Panel Replay uses always ALPM
> -	 * PSR2 uses ALPM but PSR1 doesn't
> -	 */
> -	return intel_dp_is_edp(intel_dp) && (crtc_state->has_sel_update ||
> -					     crtc_state->has_panel_replay);
> -}
> -
>  bool intel_psr_needs_alpm_aux_less(struct intel_dp *intel_dp,
>  				   const struct intel_crtc_state *crtc_state)
>  {

[Severity: High]
Does retaining the eDP-only check in intel_psr_needs_alpm_aux_less()
prevent AS SDP configuration for external DP Panel Replay?

While intel_alpm_is_aux_less() was updated to support external DP,
intel_psr_needs_alpm_aux_less() still restricts true return values to
eDP only. Any external DP configuration enabling Panel Replay will fail
this check when evaluated by intel_dp_pr_with_as_sdp_enabled() in the
link training paths.

This leads to silently missing the DP_FIXED_VTOTAL_AS_SDP_EN_IN_PR_ACTIVE
configuration during link training, causing a mismatch between source
programming and the sink DPCD configuration.

> @@ -4578,15 +4582,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 (crtc_state->has_panel_replay)
> +			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]
Can has_alpm leak as true if PSR or Panel Replay features are disabled
late in this function?

When falling back to PSR1 or fully disabling PSR due to vblank timing limits
or hardware workarounds later in intel_psr_compute_config_late():

    if (crtc_state->has_panel_replay) {
            crtc_state->has_panel_replay = false;
            crtc_state->has_psr = false;
    }
    crtc_state->has_sel_update = false;

The corresponding feature flags are cleared, but has_alpm is left as true.
Since ALPM programming paths (like intel_alpm_port_configure) now rely
directly on has_alpm, this leaked state can cause unintended ALPM hardware
programming on unsupported modes.

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