> Subject: Re: [PATCH v2] drm/i915/display: Wait for pipe start to avoid vblank > and scanline jumps > > On Thu, 29 Jan 2026, Suraj Kandpal <[email protected]> wrote: > > Check if values are updated or not in PIPE_SCANLINE register before we > > move on ahead with modeset. > > This is because we need to make sure we are not getting stale values > > from PIPE_SCANLINE register as we use theses scanline values to make a > > decision if an atomic commit can go through. Without this change we > > see Atomic update failure warning with the following > > signature: > > [drm] *ERROR* Atomic update failure on pipe B (start=457 end=458) time > > 50 us, min 2128, max 2161, scanline start 411, end 2165. > > Where the atomic commit takes less than 100us but we still see a > > vblank count jump and a big leap in scanline. > > The PIPE_SCANLINE may give stale values as internally after writing to > > TRANSCONF register it take H/w around a vblank to actually get > > enabled. > > > > Bspec: 69961 > > Signed-off-by: Suraj Kandpal <[email protected]> > > --- > > drivers/gpu/drm/i915/display/intel_ddi.c | 11 +++++++++++ > > 1 file changed, 11 insertions(+) > > > > diff --git a/drivers/gpu/drm/i915/display/intel_ddi.c > > b/drivers/gpu/drm/i915/display/intel_ddi.c > > index d8739e2bb004..4514de71cb9f 100644 > > --- a/drivers/gpu/drm/i915/display/intel_ddi.c > > +++ b/drivers/gpu/drm/i915/display/intel_ddi.c > > @@ -83,6 +83,7 @@ > > #include "intel_snps_phy.h" > > #include "intel_step.h" > > #include "intel_tc.h" > > +#include "intel_vblank.h" > > #include "intel_vdsc.h" > > #include "intel_vdsc_regs.h" > > #include "intel_vrr.h" > > @@ -3562,6 +3563,16 @@ static void intel_ddi_enable(struct > intel_atomic_state *state, > > intel_ddi_enable_hdmi(state, encoder, crtc_state, conn_state); > > else > > intel_ddi_enable_dp(state, encoder, crtc_state, conn_state); > > + /* > > + * Somtimes when pipe starts PIPEDSL/PIPE_SCANLINE reads will return > a > > + * stale value, this is because it may take 1 vblank for TRANSCONF > > + * register to enable the pipe, which causes an apparent vblank > > + * timestamp and scaline jump jump when PIPEDSL/PIPE_SCANLINE > > + * resets to its proper value. That also messes up the frame count > > + * when it's derived from the timestamps. So let's wait for the > > + * pipe to start properly, So lets wait before we proceed with modeset. > > + */ > > + > > +intel_wait_for_pipe_scanline_moving(to_intel_crtc(crtc_state->uapi.cr > > +tc)); > > The problem with adding these type of things is that they're almost impossible > to remove afterwards.
I understand but we have had this issue on and off for years and this should at least reduce one signature Of the various Atomic update failures we face. > > And on the face of it, it's kind of random placement in DDI encoder enable > where there's nothing else like this. > > But the *_crtc_enable() functions in intel_display.c do have vblank waits and > scanline moving waits right after intel_encoders_enable(). So that's kind of > where it feels like this belongs. Sure will move it there. That does sound like the right place to have this fix there. > > On another note, what about the stale values? Related perhaps? [1] The stale values happen as a result of PIPE SCANLINE hanging sporadically, which too happens at a very low frequency making it hard to reproduce unless you are on CI (to make lives worse). PIPE SCANLINE gets reset after a vblank comes in and resets it. But the problem here is if it hangs at a value which is lower than min safe scanline we do not evade vblank even though we should and then when a actual vblank comes in it resets then showing where we actually are scanline wise causing an Atomic update error As for the fix provided in [1] I think it may be possible its related to stale values, but I have not seen a case where we have a negative time diff. Maybe its best to ask for a gitlab with his logs and supporting evidence as to why he thinks this negative time diff is causing the issue. Regards, Suraj Kandpal > BR, > Jani. > > > [1] https://lore.kernel.org/r/20260108165139.1381835-1- > [email protected] > > > > > > intel_hdcp_enable(state, encoder, crtc_state, conn_state); > > -- > Jani Nikula, Intel
