Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues: - [High] Bypassing `uncore->lock` for scanline reads breaks the hardware serialization workaround for older platforms like Ivy Bridge and Haswell, potentially causing hardware hangs. Pre-existing issues: - [High] drm_crtc_vblank_put() is called with local interrupts disabled in intel_pipe_update_start(). -- commit a415cb59c253b3a3fa754a68afc62b743249b0f2 Author: Maarten Lankhorst <[email protected]> drm/i915/display: Remove locking from intel_vblank_evade critical section finish_wait() may take a lock, which means that it can take any amount of time. On PREEMPT-RT we should not be taking any lock after disabling preemption, so ensure that the completion is done before disabling interrupts. This also has the benefit of making vblank evasion more deterministic, by performing the final vblank check after all locking is done. > diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c > b/drivers/gpu/drm/i915/display/intel_crtc.c > index 34a159f7c9a43..8218938985b41 100644 > --- a/drivers/gpu/drm/i915/display/intel_crtc.c > +++ b/drivers/gpu/drm/i915/display/intel_crtc.c > @@ -734,7 +734,7 @@ void intel_pipe_update_end(struct intel_atomic_state > *state, > struct intel_crtc_state *new_crtc_state = > intel_atomic_get_new_crtc_state(state, crtc); > enum pipe pipe = crtc->pipe; > - int scanline_end = intel_get_crtc_scanline(crtc); > + int scanline_end = __intel_get_crtc_scanline(crtc); [Severity: High] Does bypassing uncore->lock here break the hardware serialization workaround for older platforms? Calling __intel_get_crtc_scanline() without the lock in intel_pipe_update_end() risks concurrent mmio access with other threads, such as when reading the PIPEDSL register, which could cause hardware hangs on platforms like Ivy Bridge and Haswell. [ ... ] > diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c > b/drivers/gpu/drm/i915/display/intel_vblank.c > index 28d81199792ef..ca08059e088ea 100644 > --- a/drivers/gpu/drm/i915/display/intel_vblank.c > +++ b/drivers/gpu/drm/i915/display/intel_vblank.c > @@ -732,6 +732,16 @@ void intel_vblank_evade_init(const struct > intel_crtc_state *old_crtc_state, > evade->min -= vblank_delay; > } > > +static bool scanline_in_safe_range(struct intel_vblank_evade_ctx *evade, int > *scanline, bool unlocked) > +{ > + if (unlocked) > + *scanline = intel_get_crtc_scanline(evade->crtc); > + else > + *scanline = __intel_get_crtc_scanline(evade->crtc); [Severity: High] Could this lockless scanline read cause a hardware hang during evasion? Similar to the above, calling __intel_get_crtc_scanline() here during the preempt-disabled evasion loop in scanline_in_safe_range() bypasses the required hardware serialization lock. Older platforms might experience hangs due to unsynchronized mmio cacheline access. > + > + return *scanline < evade->min || *scanline > evade->max; > +} [ ... ] [Severity: High] This is a pre-existing issue, but intel_pipe_update_start() calls drm_crtc_vblank_put() with local interrupts disabled: drivers/gpu/drm/i915/display/intel_crtc.c:intel_pipe_update_start() { ... local_irq_disable(); crtc->debug.min_vbl = evade.min; crtc->debug.max_vbl = evade.max; trace_intel_pipe_update_start(crtc); scanline = intel_vblank_evade(&evade); drm_crtc_vblank_put(&crtc->base); ... } Could this lead to scheduling while atomic on preempt-rt kernels? When the reference count drops to zero, drm_crtc_vblank_put() might acquire dev->vbl_lock, which is a spinlock_t that sleeps on preempt-rt. -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6
