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

["Hogander, Jouni" <jouni.hogander@intel.com>]

On Wed, 2025-10-22 at 18:37 +0300, Ville Syrjälä wrote:
> 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.

I actually tested without that +1 and I still saw those timeouts. So
that also supports the idea we need that.

> 
> 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() ?

Ok, I will change this.

BR,

Jouni Högander

> 
> > +	    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
>