Hey, Den 2025-11-26 kl. 23:57, skrev Ville Syrjälä: > On Wed, Nov 26, 2025 at 08:45:31PM +0000, Shankar, Uma wrote: >> >> >>> -----Original Message----- >>> From: Intel-xe <[email protected]> On Behalf Of Maarten >>> Lankhorst >>> Sent: Tuesday, November 4, 2025 2:07 PM >>> To: [email protected]; [email protected] >>> Cc: [email protected]; Maarten Lankhorst <[email protected]>; >>> Mario >>> Kleiner <[email protected]>; Mike Galbraith >>> <[email protected]>; Thomas Gleixner <[email protected]>; Sebastian >>> Andrzej Siewior <[email protected]>; Clark Williams >>> <[email protected]>; Steven Rostedt <[email protected]> >>> 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 <[email protected]> >>> --- >>> 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 comple te 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. > Ideally we make the time critical code even faster, and fastest is pre-obtaining the vblank locks so no race is even possible in the timing sensitive part.
If we acquire a drm_vblank_get() and dev->event_lock before we begin vblank evasion, and release them afterwards then no conversion of the lock to a raw spinlock is required. This eliminates even more jitter, and turns the support of PREEMPT_RT in something positive. :-) Kind regards, ~Maarten Lankhorst
