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 19:08 +0300, Hogander, Jouni wrote:
> 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.

a New version sent. Please check.

BR,

Jouni Högander

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