Re: [PATCH 06/10] 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` 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 <dev@lankhorst.se>

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