Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
Pre-existing issues: - [High] The fix for moving `drm_crtc_vblank_put()` out of the time-sensitive critical section was only applied to the legacy cursor update, leaving the identical anti-pattern in the main atomic update path. -- commit 43066708128be1b281ea9a6a390e979ec4569021 Author: Maarten Lankhorst <[email protected]> drm/i915/display: Move vblank put until after critical section This commit moves drm_crtc_vblank_put() after the time-sensitive critical section in intel_legacy_cursor_update() to avoid acquiring locks with interrupts disabled. It also introduces a flag to defer putting the vblank reference until programming is completed. > diff --git a/drivers/gpu/drm/i915/display/intel_cursor.c > b/drivers/gpu/drm/i915/display/intel_cursor.c > index 88384dea868b4..68b44a00567e4 100644 > --- a/drivers/gpu/drm/i915/display/intel_cursor.c > +++ b/drivers/gpu/drm/i915/display/intel_cursor.c [ ... ] > @@ -922,8 +925,6 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > local_irq_disable(); > > intel_vblank_evade(&evade); > - > - drm_crtc_vblank_put(&crtc->base); > } else { > local_irq_disable(); > } > @@ -939,6 +940,9 @@ intel_legacy_cursor_update(struct drm_plane *_plane, > > intel_psr_unlock(crtc_state); > > + if (has_vblank) > + drm_crtc_vblank_put(&crtc->base); > + [Severity: High] This isn't a bug introduced by this patch, but should the same fix be applied to the main atomic update path in intel_pipe_update_start()? If the kernel parameter drm.vblankoffdelay < 0 is set, dropping the vblank reference synchronously takes dev->vbl_lock inside vblank_disable_fn(). On PREEMPT_RT kernels, acquiring this spinlock (rt_mutex) with hardware interrupts disabled causes a "BUG: sleeping function called from invalid context" panic. Even on non-RT kernels, taking locks here causes jitter that can disrupt hardware programming. 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 drm_crtc_vblank_put() also be moved out of the IRQ-disabled section here? -- Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=5
