Re: [PATCH v8 09/27] 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 uncore->lock serialization by using intel_de_read_fw() causes machine hangs on older architectures like Ivybridge.
- [High] intel_de_read_fw() bypasses the DMC wakelock, causing powered-off register reads during the vblank disable timer.

Pre-existing issues:
- [High] drm_crtc_vblank_put() is called with hardware interrupts disabled, leading to a sleeping-in-atomic bug on PREEMPT_RT.
--

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

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

This commit changes g4x_get_vblank_counter() to use intel_de_read_fw()
to avoid a sleeping-in-atomic lockdep splat on PREEMPT_RT.

> 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 uncore->lock serialization here risk cacheline concurrent
access bugs on older architectures like Ivybridge?

As documented in intel_de.h, unlocked MMIO accessors can cause machine hangs
on older platforms if the same cacheline is accessed concurrently by different
clients. PIPE_FRMCOUNT_G4X shares a cacheline with registers like
PIPE_FRMTMSTMP.

Since this is called holding only the vbl_lock, could an underrun interrupt
handler or other subsystem access the same cacheline concurrently without
holding vbl_lock and cause a hang?


[Severity: High]
Can using the fw variant here cause a read from a powered-off register?

By using intel_de_read_fw(), this bypasses the display microcontroller wakelock.
If g4x_get_vblank_counter() is called by drm_update_vblank_count() during the
asynchronous vblank_disable_fn() timer, the display engine pipeline might be
in a deep power-saving state like DC5/DC6.

Would reading this register while powered off return garbage and cause a
massive jump in the vblank counter?

[ ... ]
>  static u32 intel_crtc_scanlines_since_frame_timestamp(struct intel_crtc *crtc)
>  {
[ ... ]
>  	do {
>  		/*
>  		 * This field provides read back of the display
>  		 * pipe frame time stamp. The time stamp value
>  		 * is sampled at every start of vertical blank.
>  		 */
>  		scan_prev_time = intel_de_read_fw(display,
>  						  PIPE_FRMTMSTMP(crtc->pipe));

[Severity: High]
This isn't a bug introduced by this patch, but the same DC5 power state bug
seems to exist here.

Because intel_crtc_scanlines_since_frame_timestamp() also uses
intel_de_read_fw(), could it also suffer from the exact same issue and
read garbage if called during the disable timer?


While reviewing the atomic context handling for vblank locks, I noticed a
related pattern in 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);
        ...
}

[Severity: High]
This isn't a bug introduced by this patch, but is it safe to call
drm_crtc_vblank_put() here with hardware interrupts disabled?

If offdelay_ms is set to less than zero, drm_vblank_put() synchronously
calls vblank_disable_fn(), which attempts to acquire the dev->vbl_lock
spinlock.

On PREEMPT_RT, spinlock_t becomes a sleeping rt_mutex. Would attempting to
acquire this sleeping lock while hardware interrupts are disabled cause an
atomic context violation and a lockdep splat?

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