Re: [PATCH 4/4] drm/i915/display: Make set_pipeconf use the fw variants

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] vlv_atomic_update_fifo() takes a sleepable spinlock (uncore->lock) inside a hardware-interrupt-disabled critical section, leading to a 'sleeping function called from invalid context' BUG on PREEMPT_RT.
--

commit 5a85aa91d02b113ff09052cce4d68e8a6b54e2ce
Author: Maarten Lankhorst <dev@lankhorst.se>

drm/i915/display: Make set_pipeconf use the fw variants

This commit changes i9xx_set_pipeconf() and ilk_set_pipeconf() to use the
_fw variants of register read/write functions. This prevents sleepable locks
from being taken inside the interrupt-disabled critical section, which was
causing a BUG on PREEMPT_RT.

> diff --git a/drivers/gpu/drm/i915/display/intel_display.c b/drivers/gpu/drm/i915/display/intel_display.c
> index f5dac380aa7ff..deb93d26e0e94 100644
> --- a/drivers/gpu/drm/i915/display/intel_display.c
> +++ b/drivers/gpu/drm/i915/display/intel_display.c
> @@ -3030,8 +3030,9 @@ void i9xx_set_pipeconf(const struct intel_crtc_state *crtc_state)
>  
>  	val |= TRANSCONF_FRAME_START_DELAY(crtc_state->framestart_delay - 1);
>  
> -	intel_de_write(display, TRANSCONF(display, cpu_transcoder), val);
> -	intel_de_posting_read(display, TRANSCONF(display, cpu_transcoder));
> +	intel_de_write_fw(display, TRANSCONF(display, cpu_transcoder), val);
> +	/* posting read */
> +	intel_de_read_fw(display, TRANSCONF(display, cpu_transcoder));
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the explicit acquisition
of a sleepable spinlock inside an interrupt-disabled critical section in
vlv_atomic_update_fifo() violate PREEMPT_RT locking rules?

drivers/gpu/drm/i915/display/i9xx_wm.c:vlv_atomic_update_fifo() {
	...
	/*
	 * intel_pipe_update_start() has already disabled interrupts
	 * for us, so a plain spin_lock() is sufficient here.
	 */
	spin_lock(&uncore->lock);
	...
}

Taking a standard spinlock (which acts as a sleeping rt_mutex on PREEMPT_RT)
after local interrupts have been disabled causes a kernel BUG splat 'sleeping
function called from invalid context'.

The vblank evasion critical section initiated by intel_pipe_update_start()
disables local interrupts. Inside this critical section,
commit_pipe_pre_planes() calls intel_atomic_update_watermarks(), which on
VLV/CHV maps to vlv_atomic_update_fifo(). This function explicitly acquires
spin_lock(&uncore->lock).

Since this patch fixes the same locking violation in i9xx_set_pipeconf()
and ilk_set_pipeconf() by using the _fw variants, is it possible this
instance in the exact same critical section was missed?

[ ... ]

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