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 <[email protected]> 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/[email protected]?part=9
