Re: [PATCH 09/10] drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

[sashiko-bot@kernel.org]

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

New issues:
- [High] Bypassing the DMC wakelock using `intel_de_read_fw()` in `g4x_get_vblank_counter()` causes unprotected reads of powered-down registers on modern platforms (Xe2+).

Pre-existing issues:
- [High] Tracepoints calling `intel_get_crtc_scanline()` acquire the sleepable `uncore->lock` in atomic context, leading to a PREEMPT_RT lockdep splat.
- [High] Acquiring `wl->lock` in `icl_dsi_frame_update()` while hardware interrupts are disabled causes a PREEMPT_RT lockdep splat.
--

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

drm/i915/display: Make get_vblank_counter use intel_de_read_fw()

This commit modifies g4x_get_vblank_counter() to use intel_de_read_fw()
instead of intel_de_read() to avoid taking a sleeping lock in atomic
context, fixing a lockdep splat on PREEMPT_RT.

>     Fixes the following lockdep splat on PREEMPT_RT:
>     <3> BUG: sleeping function called from invalid context at kernel/locking/spinlock_rt.c:48
[ ... ]
>     <4>  #9: ffff888114f7b8b8 (&intel_dp->psr.lock){+.+.}-{3:3}, at: intel_psr_lock+0xc5/0xf0 [xe]
>     <4>  #10: ffff88812a0cbbc0 (&wl->lock){+.+.}-{2:2}, at: intel_dmc_wl_get+0x3c/0x140 [xe]
>     
>     This splat will happen otherwise on all tracepoints too, for similar reasons.

[Severity: High]
This is a pre-existing issue, but do tracepoints calling
intel_get_crtc_scanline() still acquire the sleepable uncore->lock in
atomic context, leading to a PREEMPT_RT lockdep splat?

For example, trace_intel_pipe_update_start executes with preemption disabled:

intel_display_trace.h:TRACE_EVENT() {
    ...
    __entry->scanline = intel_get_crtc_scanline(crtc);
    ...
}

Which calls the locking function:

intel_vblank.c:intel_get_crtc_scanline() {
    ...
    intel_vblank_section_enter_irqf(display, &irqflags);
    ...
}

And acquires the lock:

intel_vblank.c:intel_vblank_section_enter_irqf() {
    ...
    spin_lock_irqsave(&uncore->lock, *flags);
    ...
}

On PREEMPT_RT, uncore->lock becomes an rt_spinlock which can sleep. Acquiring
a sleeping lock with preemption disabled causes a BUG splat and system
instability.

While i915_get_vblank_counter() was fixed in a subsequent patch in this series
(commit drm/i915/display: Do not take uncore lock in i915_get_vblank_counter),
intel_get_crtc_scanline() remains unfixed at the end of the series.

[Severity: High]
This is a pre-existing issue, but does acquiring wl->lock in
icl_dsi_frame_update() while hardware interrupts are disabled cause a
PREEMPT_RT lockdep splat?

The hardware update sequence between intel_pipe_update_start() and
intel_pipe_update_end() executes with hardware interrupts disabled via
local_irq_disable().

Within this section:

intel_crtc.c:intel_pipe_update_end() {
    ...
    icl_dsi_frame_update(new_crtc_state);
    ...
}

Which calls:

icl_dsi.c:icl_dsi_frame_update() {
    ...
    intel_de_rmw(display, DSI_CMD_FRMCTL(port), 0,
                 DSI_FRAME_UPDATE_REQUEST);
    ...
}

This attempts to acquire the DMC wakelock (wl->lock), which is a sleepable
rt_spinlock on PREEMPT_RT. Taking it while IRQs are disabled triggers a
sleeping function called from invalid context bug.

> diff --git a/drivers/gpu/drm/i915/display/intel_vblank.c b/drivers/gpu/drm/i915/display/intel_vblank.c
> index 07dca05332e5b..c0cc0a4c25dbe 100644
> --- a/drivers/gpu/drm/i915/display/intel_vblank.c
> +++ b/drivers/gpu/drm/i915/display/intel_vblank.c
> @@ -132,7 +132,7 @@ u32 g4x_get_vblank_counter(struct drm_crtc *crtc)
>  	if (!vblank->max_vblank_count)
>  		return 0;
>  
> -	return intel_de_read(display, PIPE_FRMCOUNT_G4X(display, pipe));
> +	return intel_de_read_fw(display, PIPE_FRMCOUNT_G4X(display, pipe));

[Severity: High]
Does bypassing the DMC wakelock here cause unprotected reads of powered-down
registers on modern platforms?

PIPE_FRMCOUNT_G4X (0x70040) is within the powered_off_ranges
(0x60000 - 0x7FFFF) defined in intel_dmc_wl.c. On modern platforms like Xe2+,
MMIO reads do not automatically wake the hardware from DC states, and the
DMC wakelock must be explicitly held. Could using intel_de_read_fw() lead
to reading garbage data (0xFFFFFFFF) or cause MMIO timeouts?

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