On Tue, Sep 01, 2026 at 11:31:19AM +0000, Kahola, Mika wrote: > > -----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.
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 <[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 -- Ville Syrjälä Intel
