Re: [PATCH 5/5] drm/amdgpu: keep reading the vblank counter in IRQ context during reset

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential
issues to consider:
- [High] drm/amdgpu: missing Signed-off-by trailer in commit message
- [High] drm/amdgpu: self-exclusion of reset task in
  amdgpu_get_vblank_counter_kms()
- [Medium] drm/amdgpu: vblank->last corruption in
  amdgpu_get_vblank_counter_kms()
- [Medium] drm/amdgpu: in_task() in amdgpu_get_vblank_counter_kms() true
  in threaded IRQ
- [Low] drm/amdgpu: commit message omits vblank->last fallback change
--

commit 65f179edc9e1f1c857a1c9cda00e5004a70269a6
Author: Prike Liang <Prike.Liang@amd.com>

> drm/amdgpu: keep reading the vblank counter in IRQ context during reset
>
> amdgpu_get_vblank_counter_kms() returns the cached vblank->last instead
> of reading the hardware counter whenever it can't take
> reset_domain->sem for reading. The vblank interrupt reads the counter
> through the same callback via drm_crtc_handle_vblank(), so once GPU
> recovery holds the semaphore for writing, every vblank interrupt sees
> diff == 0 and the DRM vblank count stops advancing. The CRTCs keep
> scanning out until dm_suspend() turns vblank off and stops the streams,
> though, which only happens after the rest of the recovery preparation.
>
> Vblank waits in that window don't see a vblank anymore. They only end
> when dm_suspend() turns vblank off, or after a second with the
> "vblank wait timed out" warning if that takes longer. The fbdev damage
> worker is likely to be waiting already, because recovery prints
> "GPU reset begin!" right before it takes the semaphore.
>
> Vblank interrupts only arrive while a CRTC is scanning out, so the
> interrupt path can read the hardware counter.
> amdgpu_device_skip_hw_access() likewise only checks the reset semaphore
> in task context. Only back off in task context, so the interrupt path
> keeps the count moving until DM turns vblank off.
>
> Fixes: f2bc71d16e73 ("drm/amdgpu: serialize vblank counter reads against GPU reset")
> Cc: Vitaly Prosyak <vitaly.prosyak@amd.com>
> Cc: James Lin <pinglei.lin@amd.com>
> Cc: Leo Li <sunpeng.li@amd.com>
> Assisted-by: Copilot:claude-opus-5.5

[Severity: High]
The commit message is missing a Signed-off-by: trailer.

[Severity: Low]
Prior to this patch, f2bc71d16e73 ("drm/amdgpu: serialize vblank counter
reads against GPU reset") returned drm_crtc_vblank_count(crtc) rather than
vblank->last when the trylock failed.  Should the commit message describe
changing the fallback return value from drm_crtc_vblank_count(crtc) to
drm_crtc_vblank_crtc(crtc)->last instead of stating that it already
returned vblank->last?

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index 8e7e522a89e42..07d178031abb5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1691,6 +1691,7 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
>  	struct drm_device *dev = crtc->dev;
>  	unsigned int pipe = crtc->index;
>  	struct amdgpu_device *adev = drm_to_adev(dev);
> +	bool locked = false;
>  	int vpos, hpos, stat;
>  	u32 count;
>  
> @@ -1698,9 +1699,26 @@ u32 amdgpu_get_vblank_counter_kms(struct drm_crtc *crtc)
>  		DRM_ERROR("Invalid crtc %u\n", pipe);
>  		return -EINVAL;
>  	}
> -	if (!down_read_trylock(&adev->reset_domain->sem))
> -		return drm_crtc_vblank_count(crtc);
>  
> +	/*
> +	 * A GPU reset is in progress and the display hardware cannot be read
> +	 * safely. Return the hardware counter value DRM already cached rather
> +	 * than the software vblank count: the core feeds this callback's
> +	 * result back into vblank->last, and a software count lives in a
> +	 * different numeric domain, which later trips
> +	 * drm_WARN_ON_ONCE(cur_vblank != vblank->last) in
> +	 * drm_update_vblank_count().
> +	 *
> +	 * Only task context has to back off. Vblank interrupts only arrive
> +	 * while a CRTC is scanning out, so the interrupt path can read the
> +	 * hardware, and it has to keep counting: vblank waits that already
> +	 * sleep when a reset takes the semaphore never complete otherwise.
> +	 */
> +	if (in_task()) {
> +		if (!down_read_trylock(&adev->reset_domain->sem))
> +			return drm_crtc_vblank_crtc(crtc)->last;
> +		locked = true;
> +	}

[Severity: High]
Does down_read_trylock(&adev->reset_domain->sem) also exclude the GPU
recovery task itself during dm_resume()?

amdgpu_device_gpu_recover() holds reset_domain->sem for writing across
amdgpu_device_asic_reset().  After the ASIC reset zeroes the 24-bit
hardware frame counter, dm_resume() runs in task context while still
holding the write lock and calls dm_gpureset_toggle_vblank() ->
drm_crtc_vblank_on_config() -> drm_reset_vblank_timestamp() ->
amdgpu_get_vblank_counter_kms().  Because down_read_trylock() fails even
for the write-lock owner, drm_reset_vblank_timestamp() stores the stale
pre-reset vblank->last instead of the zeroed post-reset hardware counter.
On the first post-reset vblank interrupt or query,
drm_update_vblank_count() computes (cur_vblank - vblank->last) &
max_vblank_count with cur_vblank < vblank->last, wrapping around
0x00ffffff and jumping the DRM vblank counter forward by ~16.7 million
frames.

[Severity: Medium]
Can returning vblank->last in task context corrupt vblank->last when
vblank is enabled during the reset preparation window?

If vblank was disabled before reset and a task (such as
drm_fb_helper_damage_work) calls drm_vblank_get() -> drm_vblank_enable()
after reset_domain->sem is write-locked but before dm_suspend() runs,
amdgpu_dm_crtc_set_vblank() calls drm_crtc_vblank_restore() when IPS and
self-refresh are enabled with disable_immediate:

drivers/gpu/drm/drm_vblank.c:drm_vblank_restore() {
    ...
	get_vblank_counter_and_timestamp(dev, pipe, &cur_vblank,
					 &t_vblank, false);

	diff_ns = ktime_to_ns(ktime_sub(t_vblank, vblank->time));
	if (framedur_ns)
		diff = DIV_ROUND_CLOSEST_ULL(diff_ns, framedur_ns);
    ...
	vblank->last = (cur_vblank - diff) & max_vblank_count;
}

Because amdgpu_get_vblank_counter_kms() returns the stale vblank->last as
cur_vblank while drm_get_last_vbltimestamp() ->
amdgpu_crtc_get_scanout_position() still reads the display registers
without holding reset_domain->sem (triggering the lockdep assert in
amdgpu_device_skip_hw_access()), drm_vblank_restore() sets vblank->last to
(vblank->last - diff) & max_vblank_count.  The next vblank interrupt then
reads the real hardware counter in hardirq context and compares it against
(vblank->last - diff), double-counting diff (or wrapping around
max_vblank_count if IPS/PSR reset the hardware counter while vblank was
off).

[Severity: Medium]
Does checking in_task() still back off in the vblank interrupt handler
when forced IRQ threading (CONFIG_PREEMPT_RT or threadirqs) is enabled?

amdgpu_irq_init() requests the IRQ without IRQF_NO_THREAD, so under forced
IRQ threading amdgpu_irq_handler() runs in irq_forced_thread_fn() under
local_bh_disable().  That sets SOFTIRQ_DISABLE_OFFSET rather than
HARDIRQ_MASK or SOFTIRQ_OFFSET, so in_task() returns true and the threaded
vblank interrupt handler still takes the trylock backoff path and returns
vblank->last during reset.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261111083534.514290-1-Prike.Liang@amd.com?part=5