Re: [PATCH] drm/i915/dp: Don't promise AS SDPs in PR Active when they are disabled

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Low] drm/i915: build failure due to undefined intel_alpm_pr_as_sdp_skip_frames_enabled()
--

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

drm/i915/dp: Don't promise AS SDPs in PR Active when they are disabled

This commit conditionally disables DP_FIXED_VTOTAL_AS_SDP_EN_IN_PR_ACTIVE
during link training if AS SDP skip frames are not in use. It also attempts
to clarify documentation about when PR_ALPM_CTL[AS SDP Transmission in
Active Disable] is programmed.

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

> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index 0cd5e6b5034cf..217795e155607 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -3154,11 +3154,12 @@ static void intel_dp_compute_as_sdp(struct intel_dp *intel_dp,
>  
>  	/*
>  	 * For Panel Replay with Async Video Timing support, the source can
> -	 * disable sending the AS SDP during PR Active state. In that case,
> -	 * the sink needs the coasting vtotal value to maintain the refresh
> -	 * rate. The HW only samples this on PR_ALPM_CTL[AS SDP Transmission
> -	 * in Active Disable], which we never program, so providing the value
> -	 * unconditionally when the sink advertises the capability is safe.
> +	 * stop sending the AS SDP during PR Active (PR_ALPM_CTL[AS SDP
> +	 * Transmission in Active Disable], see intel_alpm_configure_pr_as_sdp())

[Severity: Low]
Does the referenced intel_alpm_configure_pr_as_sdp() function exist in the
codebase? A tree-wide search doesn't show it being defined here or in any
subsequent patches. Was this perhaps renamed to lnl_alpm_configure(), or
is there a missing patch dependency?

> +	 * and the sink then keeps the refresh rate using the coasting vtotal of
> +	 * the last AS SDP it received. The coasting vtotal bytes are fully
> +	 * under driver control, so fill them in whenever the sink advertises
> +	 * the capability.
>  	 *
>  	 * #TODO:
>  	 * If we ever advertise support for coasting at other refresh targets,

[ ... ]

> diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> index cb92cff906146..5729ecaba1499 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> @@ -1122,7 +1123,17 @@ static bool
>  intel_dp_pr_with_as_sdp_enabled(struct intel_dp *intel_dp,
>  				const struct intel_crtc_state *crtc_state)
>  {
> +	/*
> +	 * DP_FIXED_VTOTAL_AS_SDP_EN_IN_PR_ACTIVE tells the sink that the AS SDP
> +	 * keeps coming during PR Active. With disable_as_sdp_when_pr_active,
> +	 * intel_alpm_configure_pr_as_sdp() sets PR_ALPM_CTL[AS SDP Transmission
> +	 * in Active Disable] and the source stops sending it for as long as PR
> +	 * is active, unless AS SDP skip frames is used. The sink then keeps
> +	 * the refresh rate using the coasting vtotal of the last AS SDP.
> +	 */
>  	return intel_psr_needs_alpm_aux_less(intel_dp, crtc_state) &&
> +		(!crtc_state->disable_as_sdp_when_pr_active ||
> +		 intel_alpm_pr_as_sdp_skip_frames_enabled(intel_dp, crtc_state)) &&

[Severity: Low]
Is intel_alpm_pr_as_sdp_skip_frames_enabled() defined anywhere in this tree?
This causes a build failure due to an implicit declaration and undefined
reference.

Additionally, the commit message states that when AS SDP skip frames are in
use, it clears the PR_ALPM_CTL bit and keeps a periodic AS SDP going.
However, looking at lnl_alpm_configure() in intel_alpm.c, the bit seems to
be programmed without a skip frames check:

	if (crtc_state->disable_as_sdp_when_pr_active)
		pr_alpm_ctl |= PR_ALPM_CTL_AS_SDP_TRANSMISSION_IN_ACTIVE_DISABLE;

Are we missing the patch that introduces both these missing functions and the
updated PR_ALPM_CTL programming logic?

>  		(crtc_state->infoframes.enable &
>  		 intel_hdmi_infoframe_enable(DP_SDP_ADAPTIVE_SYNC));
>  }

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