Re: [PATCH] drm/i915/display: Take into account AS SDP in intel_dp_sdp_min_guardband

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Wed, Oct 22, 2025 at 03:25:52PM +0300, Jouni Högander wrote:
> We started seeing "[drm] *ERROR* Timed out waiting PSR idle state" after
> taking optimized guardband into use. These are seen because VSC SDPs are
> sent on same line as AS SDPs when AS SDP is enabled. AS SDP is sent on line
> configured in EMP_AS_SDP_TL register. We are configuring
> crtc_state->vrr.vsync_start into that register.
> 
> Fix this by ensuring AS SDP is sent on line which is within
> guardband. From the bspec:
> 
> EMP_AS_SDP_TL < SCL + Guardband
> 
> Bspec: 71197
> 
> Fixes: 52ecd48b8d3f ("drm/i915/dp: Add helper to get min sdp guardband")
> Cc: Ankit Nautiyal <ankit.k.nautiyal@intel.com>
> Signed-off-by: Jouni Högander <jouni.hogander@intel.com>
> ---
>  drivers/gpu/drm/i915/display/intel_dp.c | 15 ++++++++++++---
>  1 file changed, 12 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
> index b0aeb6c2de86c..54b5e060be82a 100644
> --- a/drivers/gpu/drm/i915/display/intel_dp.c
> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
> @@ -7026,7 +7026,7 @@ int intel_dp_compute_config_late(struct intel_encoder *encoder,
>  }
>  
>  static
> -int intel_dp_get_lines_for_sdp(u32 type)
> +int intel_dp_get_lines_for_sdp(const struct intel_crtc_state *crtc_state, u32 type)
>  {
>  	switch (type) {
>  	case DP_SDP_VSC_EXT_VESA:
> @@ -7036,6 +7036,8 @@ int intel_dp_get_lines_for_sdp(u32 type)
>  		return 8;
>  	case DP_SDP_PPS:
>  		return 7;
> +	case DP_SDP_ADAPTIVE_SYNC:
> +		return crtc_state->vrr.vsync_start + 1;

Is the +1 actually needed? I get the impression the bspec page isn't
being very accurate with the '<' usage.

Hmm, there is an extra note in the EMP_AS_SDP_TL register:
"For DP/eDP, if there is a set context latency (SCL) window, then it
 cannot be the first line of SCL
 For DP/eDP, if there is no SCL window, then it cannot be the first line 
 of the Delayed V. Blank"
So I guess there might be a real reason for that extra line.

Though I'm pretty sure no one has even confirmed that we don't have any
off by one errors in EMP_AS_SDP_TL/etc. Should do that at some point...

>  	default:
>  		break;
>  	}
> @@ -7052,11 +7054,18 @@ int intel_dp_sdp_min_guardband(const struct intel_crtc_state *crtc_state,
>  	    crtc_state->infoframes.enable &
>  	    intel_hdmi_infoframe_enable(HDMI_PACKET_TYPE_GAMUT_METADATA))
>  		sdp_guardband = max(sdp_guardband,
> -				    intel_dp_get_lines_for_sdp(HDMI_PACKET_TYPE_GAMUT_METADATA));
> +				    intel_dp_get_lines_for_sdp(crtc_state,
> +							       HDMI_PACKET_TYPE_GAMUT_METADATA));
>  
>  	if (assume_all_enabled ||
>  	    crtc_state->dsc.compression_enable)
> -		sdp_guardband = max(sdp_guardband, intel_dp_get_lines_for_sdp(DP_SDP_PPS));
> +		sdp_guardband = max(sdp_guardband,
> +				    intel_dp_get_lines_for_sdp(crtc_state, DP_SDP_PPS));
> +
> +	if (assume_all_enabled ||

assume_all_enable && HAS_AS_SDP() ?

> +	    crtc_state->infoframes.enable & intel_hdmi_infoframe_enable(DP_SDP_ADAPTIVE_SYNC))
> +		sdp_guardband = max(sdp_guardband,
> +				    intel_dp_get_lines_for_sdp(crtc_state, DP_SDP_ADAPTIVE_SYNC));
>  
>  	return sdp_guardband;
>  }
> -- 
> 2.43.0

-- 
Ville Syrjälä
Intel