Re: [PATCH RESEND 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 2 potential
issues to consider:
- [Medium] drm/amdgpu: vblank counter jump after reset in
  amdgpu_get_vblank_counter_kms()
- [Low] drm/amdgpu: commit message and Fixes SHA mismatch in
  amdgpu_get_vblank_counter_kms()
--

commit 6af07d5a30227ace169063264412acdd7272d49f
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
> Signed-off-by: Prike Liang <Prike.Liang@amd.com>

[Severity: Low]
Prior to this patch, amdgpu_get_vblank_counter_kms() returned
drm_crtc_vblank_count(crtc) rather than drm_crtc_vblank_crtc(crtc)->last
when down_read_trylock() failed.  Should the commit message mention
changing that fallback from the software counter to vblank->last?

Also, f2bc71d16e73 ("drm/amdgpu: serialize vblank counter reads against
GPU reset") is not an ancestor in this tree's history, where that commit
is 51a4359f7431 ("drm/amdgpu: serialize vblank counter reads against GPU
reset").

> 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: Medium]
Does returning vblank->last here prevent dm_resume() from re-baselining
vblank->last after an ASIC reset, causing vblank->count to jump forward by
up to ~16.7 million frames?

During GPU recovery, amdgpu_device_gpu_recover() holds reset_domain->sem
for writing on the recovery worker thread (where in_task() is true) across
amdgpu_device_asic_reset().  The ASIC reset zeroes the 24-bit hardware
frame counter, and dm_resume() then runs:

  dm_resume()
    -> dm_gpureset_toggle_vblank()
      -> drm_crtc_vblank_on_config()
        -> drm_reset_vblank_timestamp()
          -> get_vblank_counter_and_timestamp()
            -> amdgpu_get_vblank_counter_kms()

Because the recovery thread itself holds the write lock on
reset_domain->sem, down_read_trylock() fails and returns the stale
pre-reset vblank->last instead of reading the reset hardware counter (0).
drm_reset_vblank_timestamp() stores that stale pre-reset value back into
vblank->last.  On the first post-reset vblank interrupt,
drm_update_vblank_count() computes:

  diff = (cur_vblank - vblank->last) & max_vblank_count;

With cur_vblank near 0 and vblank->last still holding the pre-reset count,
this wraps modulo max_vblank_count (0x00ffffff) and advances the DRM
vblank counter by millions of bogus frames (and similarly stops
dm_suspend() -> drm_crtc_vblank_off() from sampling the final pre-reset
hardware counter).  Unlike amdgpu_device_skip_hw_access(), which allows
access when the current task holds the write lock on reset_domain->sem,
down_read_trylock() also rejects the reset thread itself.

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