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