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 <[email protected]>
> > > Signed-off-by: Jouni Högander <[email protected]>
> > > ---
> > > 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
> >
>
