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.crtc));
The problem with adding these type of things is that they're almost impossible to remove afterwards. 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. On another note, what about the stale values? Related perhaps? [1] BR, Jani. [1] https://lore.kernel.org/r/[email protected] > > intel_hdcp_enable(state, encoder, crtc_state, conn_state); -- Jani Nikula, Intel
