> -----Original Message----- > From: Ville Syrjälä <[email protected]> > Sent: Friday, 28 August 2026 21.38 > To: Kahola, Mika <[email protected]> > Cc: [email protected]; [email protected] > 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. 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 <[email protected]> > > --- > > 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
