Re: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on PREEMPT_RT

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

On Wed, Nov 26, 2025 at 08:45:31PM +0000, Shankar, Uma wrote:
> 
> 
> > -----Original Message-----
> > From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Maarten
> > Lankhorst
> > Sent: Tuesday, November 4, 2025 2:07 PM
> > To: intel-gfx@lists.freedesktop.org; intel-xe@lists.freedesktop.org
> > Cc: linux-rt-devel@lists.linux.dev; Maarten Lankhorst <dev@lankhorst.se>; Mario
> > Kleiner <mario.kleiner.de@gmail.com>; Mike Galbraith
> > <umgwanakikbuti@gmail.com>; Thomas Gleixner <tglx@linutronix.de>; Sebastian
> > Andrzej Siewior <bigeasy@linutronix.de>; Clark Williams
> > <clrkwllms@kernel.org>; Steven Rostedt <rostedt@goodmis.org>
> > Subject: [PATCH v2 6/7] drm/i915/display: Enable interrupts earlier on
> > PREEMPT_RT
> > 
> > The last part of the vblank evasion is about updating bookkeeping, not
> > programming hardware registers.
> > 
> > The interrupts cannot stay disabled here on PREEMPT_RT since the spinlocks get
> > converted to mutexes.
> > 
> > Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> > ---
> >  drivers/gpu/drm/i915/display/intel_crtc.c | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c
> > b/drivers/gpu/drm/i915/display/intel_crtc.c
> > index 9d2a23c96c61b..b87f6b4a4f3d7 100644
> > --- a/drivers/gpu/drm/i915/display/intel_crtc.c
> > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c
> > @@ -688,6 +688,14 @@ void intel_pipe_update_end(struct intel_atomic_state
> > *state,
> >  	    intel_crtc_has_type(new_crtc_state, INTEL_OUTPUT_DSI))
> >  		icl_dsi_frame_update(new_crtc_state);
> > 
> > +#if IS_ENABLED(CONFIG_PREEMPT_RT)
> > +	/*
> > +	 * Timing sensitive register writing completed, non-deterministic
> > +	 * locking from here on out.
> > +	 */
> > +	local_irq_enable();
> > +#endif
> 
> I think we do have VRR send push etc handled here, also arming registers are being updated.
> Not sure we can allow interrupts here. Please check once

Yeah, this doesn't seem exactly great.

Even without VRR we want the register writes and vblank event arming
to happen in the same frame. Though without VRR I suppose the worst
that could happen is that we complete the commit one frame too late.

With VRR however we need the vblank event arming and push to happen
in the same frame. Otherwise we'll complete the flip early and leave
push send assrted, which causes the next frame to terminate at vmin.
Basically that makes the next frame a mailbox flip as far as push
send is concerned.

The race is already there, but allowing the CPU to get scheduled away
will widen it. We do try to handle it in the vblank evasion, but I
think we're handling it way too early (in intel_vblank_evade_init())
so that part itself is racy. I suppose we should rather do the vmin
vs. vmax evasion decision after we've actually read out the current
scanline. That should at least make it a bit more robust.

One other thing we could maybe think about is arming the vblank
event after the push is sent (with seq = current+1), and then
immediately check if the push bit already cleared, and if so
cancel the arming and send the event directly (with seq = current).
But that's just a quick idead that popped to my head, didn't really
think it through in detail.

I'm tempted to say we should just make the vblank locks raw spinlocks
as well. But I've not looked at what other drivers do in the vblank
hooks so dunno how feasible that is.

-- 
Ville Syrjälä
Intel