Re: [RFC PATCH] drm/i915/psr: Send the AS SDP in PR Active while vblank is enabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] drm/i915/psr: wakeref leak and double-put in intel_psr_notify_vblank_enable_disable()
- [Low] drm/i915: Use of undeclared function intel_alpm_pr_as_sdp_skip_frames_enabled
--

commit 83416e4f5b6b0591037f900cd3b6bc4bc168af5c
Author: Jake Steinman <j@metarealtyinc.ca>

drm/i915/psr: Send the AS SDP in PR Active while vblank is enabled

Sinks that support async video timing in PR get the AS SDP suspended in
PR Active (disable_as_sdp_when_pr_active). The LG panel in the Dell XPS
16 DA16260 (sink OUI 00:22:b9, "Balsa2") then judders while frames are
updating, with the source flipping at a steady 8.33 ms. Leaving the AS
SDP on fixes it but blocks DC5/DC6.

Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/9119
Link: https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/8930

> diff --git a/drivers/gpu/drm/i915/display/intel_psr.c b/drivers/gpu/drm/i915/display/intel_psr.c
> index f490beb66629e..f69df2147c9d4 100644
> --- a/drivers/gpu/drm/i915/display/intel_psr.c
> +++ b/drivers/gpu/drm/i915/display/intel_psr.c
[ ... ]
> @@ -2221,6 +2244,12 @@ static void intel_psr_enable_locked(struct intel_dp *intel_dp,
>  	intel_dp->psr.enabled = true;
>  	intel_dp->psr.pause_counter = 0;
>  
> +	intel_dp->psr.pr_as_sdp_follow_vblank = crtc_state->has_panel_replay &&
> +		crtc_state->disable_as_sdp_when_pr_active &&
> +		intel_dp->as_sdp_supported && intel_dp_is_edp(intel_dp) &&
> +		!intel_alpm_pr_as_sdp_skip_frames_enabled(intel_dp, crtc_state);

[Severity: Low]
Is this function call missing a declaration? The function
intel_alpm_pr_as_sdp_skip_frames_enabled() is neither defined nor declared
in the tree at this commit.

It appears to depend on an out-of-tree series, which causes a compilation
failure due to an implicit declaration.

> +	intel_psr_pr_as_sdp_update(intel_dp);
> +
>  	/*
>  	 * Link_ok is sticky and set here on PSR enable. We can assume link
>  	 * training is complete as we never continue to PSR enable with
[ ... ]
> @@ -4144,7 +4174,11 @@ void intel_psr_notify_vblank_enable_disable(struct intel_display *display,
>  				intel_dp->psr.vblank_wakeref =
>  					intel_display_power_get(display,
>  								POWER_DOMAIN_DC_OFF);
> -			else
> +
> +			intel_dp->psr.vblank_enabled = enable;
> +			intel_psr_pr_as_sdp_update(intel_dp);
> +
> +			if (!enable)
>  				intel_display_power_put(display, POWER_DOMAIN_DC_OFF,
>  							intel_dp->psr.vblank_wakeref);
>  		}

[Severity: High]
Does this loop cause unbalanced power tracking and a wakeref leak?

for_each_intel_encoder_with_psr() iterates over both the primary DP encoder
and its MST stream encoders, executing multiple times for the same
intel_dp pointer.

When enable is true, intel_dp->psr.vblank_wakeref is assigned repeatedly,
leaking all but the last tracker and leaking POWER_DOMAIN_DC_OFF references.

When enable is false, the single saved wakeref is freed multiple times,
causing a double-free or an underflow of the power domain use count.

Additionally, CAN_PANEL_REPLAY() evaluates sink capabilities which can change
dynamically across hotplugs. If the capability changes between the true and
false calls, does this break the symmetry of the wakeref lifecycle?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261006220500.3417572-1-j@metarealtyinc.ca?part=1