Re: [PATCH v8 06/27] drm/i915/display: Remove locking from intel_vblank_evade critical section

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

New issues:
- [High] Bypassing `uncore->lock` during vblank evasion allows concurrent MMIO accesses, leading to hardware hangs on older platforms like Ivybridge and Haswell.

Pre-existing issues:
- [High] drm_crtc_vblank_put() can acquire the sleeping lock dev->vbl_lock while local IRQs are disabled, causing a BUG on PREEMPT_RT.
--

commit 8b19163d6f27afde254220a0f64b5572fb1ac0f0
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915/display: Remove locking from intel_vblank_evade critical section

This commit avoids calling sleeping functions with local interrupts disabled
during vblank evasion to fix PREEMPT_RT bugs. It also replaces
intel_get_crtc_scanline() with __intel_get_crtc_scanline() in unlocked
contexts to prevent taking spinlocks.

> diff --git a/drivers/gpu/drm/i915/display/intel_crtc.c b/drivers/gpu/drm/i915/display/intel_crtc.c
> index 6b71810803139..89d1091a56805 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 and in scanline_in_safe_range() risk
hardware hangs on older platforms?

By using __intel_get_crtc_scanline() inside the local_irq_disable() critical
section, the code performs raw MMIO reads that bypass the lock. Older
platforms like Ivybridge and Haswell require all register accesses to the
same cacheline to be serialized via uncore->lock.

Could this concurrent MMIO access cause those platforms to hang?

>  	u32 end_vbl_count = intel_crtc_get_vblank_counter(crtc);
>  	ktime_t end_vbl_time = ktime_get();

[ ... ]

> 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
[ ... ]
> @@ -739,24 +749,12 @@ int intel_vblank_evade(struct intel_vblank_evade_ctx *evade)
>  	struct intel_display *display = to_intel_display(crtc);
>  	long timeout = msecs_to_jiffies_timeout(1);
>  	wait_queue_head_t *wq = drm_crtc_vblank_waitqueue(&crtc->base);
> -	DEFINE_WAIT(wait);
>  	int scanline;
>  
>  	if (evade->min <= 0 || evade->max <= 0)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does the critical section surrounding
intel_vblank_evade() still have a PREEMPT_RT bug?

In intel_pipe_update_start(), local_irq_disable() is called before the
evasion loop. Immediately after the loop, drm_crtc_vblank_put() is called,
still within the IRQ-disabled section.

If drm_vblank_offdelay is set to < 0, drm_crtc_vblank_put() synchronously
calls vblank_disable_fn(), which acquires dev->vbl_lock. On PREEMPT_RT,
dev->vbl_lock maps to a sleeping rt_mutex, which triggers a sleeping in
atomic context BUG when acquired with local IRQs disabled.

Is this something that needs to be addressed for PREEMPT_RT?

>  
> -	for (;;) {
> -		/*
> -		 * prepare_to_wait() has a memory barrier, which guarantees
> -		 * other CPUs can see the task state update by the time we
> -		 * read the scanline.
> -		 */
> -		prepare_to_wait(wq, &wait, TASK_UNINTERRUPTIBLE);
> -
> -		scanline = intel_get_crtc_scanline(crtc);
> -		if (scanline < evade->min || scanline > evade->max)
> -			break;
> -
> +	while (!scanline_in_safe_range(evade, &scanline, false)) {
>  		if (!timeout) {
>  			drm_dbg_kms(display->drm,
>  				    "Potential atomic update failure on pipe %c\n",

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615201846.307297-1-dev@lankhorst.se?part=6