Re: [PATCH 1/3] drm/i915/display: Flush frontbuffer tracking on flipq completion

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Tue, Sep 01, 2026 at 11:31:19AM +0000, Kahola, Mika wrote:
> > -----Original Message-----
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Sent: Friday, 28 August 2026 21.38
> > To: Kahola, Mika <mika.kahola@intel.com>
> > Cc: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Subject: Re: [PATCH 1/3] drm/i915/display: Flush frontbuffer tracking on flipq completion
> > 
> > On Fri, Aug 07, 2026 at 12:15:38PM +0000, Mika Kahola wrote:
> > > Flipq commits execute asynchronously via the pipe DMC, but we don't
> > > notify frontbuffer tracking when they actually complete. A CRC grabbed
> > > right after can therefore observe stale state (kms_cursor_crc's
> > > cursor-alpha-opaque hits this).
> > 
> > There's nothing special about flipq vs. commit completion. The interrupt
> > is supposed to fire on the delayed vblank after the flip has executed.
> 
> Right. I think we still need to have a bookkeeping on DMC ring buffer and signal flipq completion by verifying that head and tail are equal on flipq ring buffer. As per my understanding the interrupt (flipq_event/PIPEDMC_FLIPQ_PROG_DONE) may not be enough as a sync point if we have more than one entry in the ring buffer. crtc->flipq_event is a single per-crtc slot, so a second commit queued before the first entry drains overwrites the first commit's still-pending event before its interrupt fires. Hence, I would think that we would still need this polling for flipq completion.

There is never more than one entry in the queue.

> 
> Does this sound reasonable approach?
> 
> Thanks for the review!
> 
> -Mika-
> 
> > 
> > > Stash the fb_bits on the crtc when arming the commit, and flush
> > > frontbuffer/FBC from a worker once intel_dsb_irq_handler() sees the
> > > completion interrupt. Can't call these directly from there, it's
> > > hardirq context and they may sleep.
> > >
> > > Also flush (rather than drop) any pending completion when the crtc
> > > gets disabled out from under it.
> > >
> > > Assisted-by: Copilot:claude-sonnet-5
> > > Signed-off-by: Mika Kahola <mika.kahola@intel.com>
> > > ---
> > >  drivers/gpu/drm/i915/display/intel_crtc.c     |  3 +
> > >  drivers/gpu/drm/i915/display/intel_display.c  |  3 +
> > >  .../drm/i915/display/intel_display_types.h    |  9 +++
> > >  drivers/gpu/drm/i915/display/intel_dsb.c      | 65 +++++++++++++++++++
> > >  drivers/gpu/drm/i915/display/intel_dsb.h      |  2 +
> > >  drivers/gpu/drm/i915/display/intel_fbc.c      | 21 ++++++
> > >  drivers/gpu/drm/i915/display/intel_fbc.h      |  1 +
> > >  7 files changed, 104 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > index 10ed9bdfee76..71559f86fa63 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > > @@ -25,6 +25,7 @@
> > >  #include "intel_display_trace.h"
> > >  #include "intel_display_types.h"
> > >  #include "intel_drrs.h"
> > > +#include "intel_dsb.h"
> > >  #include "intel_dsi.h"
> > >  #include "intel_fifo_underrun.h"
> > >  #include "intel_parent.h"
> > > @@ -341,6 +342,8 @@ static int __intel_crtc_init(struct intel_display *display, enum pipe pipe)
> > >  	crtc->pipe = pipe;
> > >  	crtc->num_scalers = DISPLAY_RUNTIME_INFO(display)->num_scalers[pipe];
> > >
> > > +	intel_dsb_crtc_init(crtc);
> > > +
> > >  	if (DISPLAY_VER(display) >= 9)
> > >  		primary = skl_universal_plane_create(display, pipe, PLANE_1);
> > >  	else
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> > > index 829d7a411dcc..5cdaa59f005e 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_display.c
> > > @@ -6889,6 +6889,8 @@ static void intel_update_crtc(struct intel_atomic_state *state,
> > >  	if (new_crtc_state->use_flipq) {
> > >  		intel_flipq_enable(new_crtc_state);
> > >
> > > +		crtc->flipq_fb_bits = new_crtc_state->fb_bits;
> > > +
> > >  		intel_crtc_prepare_vblank_event(new_crtc_state, &crtc->flipq_event);
> > >
> > >  		intel_flipq_add(crtc, INTEL_FLIPQ_PLANE_1, 0, INTEL_DSB_0,
> > > @@ -6969,6 +6971,7 @@ static void intel_old_crtc_state_disables(struct intel_atomic_state *state,
> > >  			intel_atomic_get_new_crtc_state(state, pipe_crtc);
> > >
> > >  		pipe_crtc->active = false;
> > > +		intel_dsb_crtc_disable(pipe_crtc);
> > >  		intel_fbc_disable(pipe_crtc);
> > >
> > >  		if (!new_pipe_crtc_state->hw.active)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_display_types.h b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > index 20a07ea06b5e..c644ca0fe9b8 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_display_types.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_display_types.h
> > > @@ -1522,6 +1522,15 @@ struct intel_crtc {
> > >  	struct drm_pending_vblank_event *dsb_event;
> > >  	/* armed event for flip queue based updates */
> > >  	struct drm_pending_vblank_event *flipq_event;
> > > +	/*
> > > +	 * Frontbuffer bits for a pending flipq/DSB-driven visible update.
> > > +	 */
> > > +	unsigned int flipq_fb_bits;
> > > +	/*
> > > +	 * intel_dsb_irq_handler() runs in hardirq context, so the
> > > +	 * frontbuffer/FBC flush for flipq_fb_bits is deferred to this worker.
> > > +	 */
> > > +	struct work_struct flipq_fb_bits_work;
> > >
> > >  	/* Access to these should be protected by display->irq.lock. */
> > >  	bool cpu_fifo_underrun_disabled;
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.c b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > index d9a270362a82..66f107493f93 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.c
> > > @@ -17,6 +17,8 @@
> > >  #include "intel_display_types.h"
> > >  #include "intel_dsb.h"
> > >  #include "intel_dsb_regs.h"
> > > +#include "intel_fbc.h"
> > > +#include "intel_frontbuffer.h"
> > >  #include "intel_psr.h"
> > >  #include "intel_vblank.h"
> > >  #include "intel_vrr.h"
> > > @@ -1106,6 +1108,59 @@ void intel_dsb_cleanup(struct intel_dsb *dsb)
> > >  	kfree(dsb);
> > >  }
> > >
> > > +/*
> > > + * Frontbuffer/FBC bookkeeping for a completed flipq commit may sleep (takes
> > > + * mutexes), but intel_dsb_irq_handler() runs in hardirq context, so do it
> > > + * here instead, on display->wq.unordered.
> > > + */
> > > +static void intel_dsb_flipq_fb_bits_work(struct work_struct *work)
> > > +{
> > > +	struct intel_crtc *crtc =
> > > +		container_of(work, typeof(*crtc), flipq_fb_bits_work);
> > > +	struct intel_display *display = to_intel_display(crtc);
> > > +	unsigned int flipq_fb_bits;
> > > +
> > > +	spin_lock_irq(&display->drm->event_lock);
> > > +	flipq_fb_bits = crtc->flipq_fb_bits;
> > > +	crtc->flipq_fb_bits = 0;
> > > +	spin_unlock_irq(&display->drm->event_lock);
> > > +
> > > +	if (!flipq_fb_bits)
> > > +		return;
> > > +
> > > +	intel_frontbuffer_flip(display, flipq_fb_bits);
> > > +	intel_fbc_flipq_post_update(display, crtc->pipe);
> > > +}
> > > +
> > > +/**
> > > + * intel_dsb_crtc_init - Initialize the per-CRTC DSB/flipq worker state.
> > > + * @crtc: the CRTC
> > > + */
> > > +void intel_dsb_crtc_init(struct intel_crtc *crtc)
> > > +{
> > > +	INIT_WORK(&crtc->flipq_fb_bits_work, intel_dsb_flipq_fb_bits_work);
> > > +}
> > > +
> > > +/**
> > > + * intel_dsb_crtc_disable - Drop any stale pending flipq FBC completion.
> > > + * @crtc: the CRTC
> > > + *
> > > + * A flipq commit's completion (and hence its deferred frontbuffer/FBC
> > > + * flush) can still be outstanding when @crtc gets disabled, eg. across a
> > > + * modeset. Cancel and discard it so it can't run later against whatever
> > > + * @crtc is reconfigured to next.
> > > + */
> > > +void intel_dsb_crtc_disable(struct intel_crtc *crtc)
> > > +{
> > > +	struct intel_display *display = to_intel_display(crtc);
> > > +
> > > +	cancel_work_sync(&crtc->flipq_fb_bits_work);
> > > +
> > > +	spin_lock_irq(&display->drm->event_lock);
> > > +	crtc->flipq_fb_bits = 0;
> > > +	spin_unlock_irq(&display->drm->event_lock);
> > > +}
> > > +
> > >  void intel_dsb_irq_handler(struct intel_display *display,
> > >  			   enum pipe pipe, enum intel_dsb_id dsb_id)
> > >  {
> > > @@ -1130,6 +1185,16 @@ void intel_dsb_irq_handler(struct intel_display *display,
> > >  		}
> > >
> > >  		spin_unlock(&display->drm->event_lock);
> > > +
> > > +		/*
> > > +		 * Flipq-driven commits complete asynchronously via DSB/DMC.
> > > +		 * Notify frontbuffer/FBC tracking only once the queued update
> > > +		 * has actually completed. The unlocked peek is just to avoid
> > > +		 * pointlessly queueing work; intel_dsb_flipq_fb_bits_work()
> > > +		 * re-checks flipq_fb_bits under event_lock.
> > > +		 */
> > > +		if (READ_ONCE(crtc->flipq_fb_bits))
> > > +			queue_work(display->wq.unordered, &crtc->flipq_fb_bits_work);
> > >  	}
> > >
> > >  	errors = tmp & dsb_error_int_status(display);
> > > diff --git a/drivers/gpu/drm/i915/display/intel_dsb.h b/drivers/gpu/drm/i915/display/intel_dsb.h
> > > index 3dcca9ed5371..86d4111c1850 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_dsb.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_dsb.h
> > > @@ -73,5 +73,7 @@ void intel_dsb_wait(struct intel_dsb *dsb);
> > >
> > >  void intel_dsb_irq_handler(struct intel_display *display,
> > >  			   enum pipe pipe, enum intel_dsb_id dsb_id);
> > > +void intel_dsb_crtc_init(struct intel_crtc *crtc);
> > > +void intel_dsb_crtc_disable(struct intel_crtc *crtc);
> > >
> > >  #endif
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.c b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > index f61b4a218d6e..05a26d991cd2 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.c
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.c
> > > @@ -1930,6 +1930,27 @@ void intel_fbc_post_update(struct intel_atomic_state *state,
> > >  	}
> > >  }
> > >
> > > +/*
> > > + * Flipq commits complete asynchronously, well after intel_fbc_post_update()
> > > + * already ran against the not-yet-latched surface. Re-run the post-update
> > > + * step for @pipe's FBC once the queued commit has actually completed, so any
> > > + * flip nuke recompresses the surface that is really being scanned out.
> > > + */
> > > +void intel_fbc_flipq_post_update(struct intel_display *display, enum pipe pipe)
> > > +{
> > > +	struct intel_fbc *fbc = intel_fbc_for_pipe(display, pipe);
> > > +
> > > +	if (!fbc)
> > > +		return;
> > > +
> > > +	mutex_lock(&fbc->lock);
> > > +
> > > +	if (fbc->state.plane && fbc->state.plane->pipe == pipe)
> > > +		__intel_fbc_post_update(fbc);
> > > +
> > > +	mutex_unlock(&fbc->lock);
> > > +}
> > > +
> > >  static unsigned int intel_fbc_get_frontbuffer_bit(struct intel_fbc *fbc)
> > >  {
> > >  	if (fbc->state.plane)
> > > diff --git a/drivers/gpu/drm/i915/display/intel_fbc.h b/drivers/gpu/drm/i915/display/intel_fbc.h
> > > index 6c96d690a2f5..ba7b93eb1424 100644
> > > --- a/drivers/gpu/drm/i915/display/intel_fbc.h
> > > +++ b/drivers/gpu/drm/i915/display/intel_fbc.h
> > > @@ -34,6 +34,7 @@ bool intel_fbc_pre_update(struct intel_atomic_state *state,
> > >  			  struct intel_crtc *crtc);
> > >  void intel_fbc_post_update(struct intel_atomic_state *state,
> > >  			   struct intel_crtc *crtc);
> > > +void intel_fbc_flipq_post_update(struct intel_display *display, enum pipe pipe);
> > >  void intel_fbc_init(struct intel_display *display);
> > >  void intel_fbc_cleanup(struct intel_display *display);
> > >  void intel_fbc_sanitize(struct intel_display *display);
> > > --
> > > 2.43.0
> > 
> > --
> > Ville Syrjälä
> > Intel

-- 
Ville Syrjälä
Intel