Re: [PATCH 05/10] drm/i915/display: Move vblank put until after critical section

[sashiko-bot@kernel.org]

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 <dev@lankhorst.se>

    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/20260702072154.171324-1-dev@lankhorst.se?part=5