Re: [PATCH v3 02/15] drm/amdgpu: complete the guilty fence at the end of a multi ring reset

["Timur Kristóf" <timur.kristof@gmail.com>]

On Thu, 2026-11-05 at 02:09 +0000, Zhang, Jesse(Jie) wrote:
> AMD General
> 
> > -----Original Message-----
> > From: Timur Kristóf <timur.kristof@gmail.com>
> > Sent: Thursday, November 5, 2026 6:47 AM
> > To: Zhang, Jesse(Jie) <Jesse.Zhang@amd.com>; amd-
> > gfx@lists.freedesktop.org
> > Cc: Deucher, Alexander <Alexander.Deucher@amd.com>; Koenig,
> > Christian
> > <Christian.Koenig@amd.com>; Lazar, Lijo <Lijo.Lazar@amd.com>; Mario
> > Sopena-Novales <Mario.Novales@amd.com>; Rastogi, Manu
> > <Manu.Rastogi@amd.com>; Zhu, Jiongyu (Joey) <Jiongyu.Zhu@amd.com>
> > Subject: Re: [PATCH v3 02/15] drm/amdgpu: complete the guilty fence
> > at the
> > end of a multi ring reset
> > 
> > On Wed, 2026-11-04 at 02:46 +0000, Zhang, Jesse(Jie) wrote:
> > > > > +           r = amdgpu_ring_reset_helper_end(ring, ring ==
> > > > > guilty_ring ?
> > > > > +                                            guilty_fence :
> > > > > ring->guilty_fence);
> > > > 
> > > > Can you help me understand what was wrong with the line before?
> > > > I think guilty_fence == ring->guilty_fence when ring ==
> > > > guilty_ring
> > > > no?
> > > 
> > > Usually yes, but not when the soft reset follows a failed ring
> > > reset
> > > of the same job.
> > >  amdgpu_ring_reset(ring, vmid, job->hw_fence)
> > >    -> amdgpu_ring_reset_helper_begin(ring, F)
> > >    -> amdgpu_ring_backup_unprocessed_commands(ring, F)
> > >       sets ring->guilty_fence = F.
> > > 
> > > So by the time the end helper runs, ring->guilty_fence no longer
> > > equals the guilty fence: the begin helper cleared it.
> > 
> > I think I see the problem.
> > 
> > I re-read the queue reset code and the helpers. During a queue
> > reset, it makes
> > a backup and saves the guilty fence but if the queue reset fails,
> > it doesn't
> > restore the contents, and on a subsequent call it clears the guilty
> > fence and the
> > backed up contents.
> > 
> > Unfortunately I think this commit doesn't completely fix that
> > because the reset
> > begin helper will still clear the backed up contents on the second
> > call (sets
> > ring_backup_entries_to_copy = 0).
> > 
> > I think the correct fix would be to make sure we don't repeat the
> > ring reset
> > begin helper:
> > https://gitlab.freedesktop.org/Venemo/linux/-
> > /commit/86f2bf222caee0da267f9d9e12362513fe4f7f9e
> > Please take a look and if you agree feel free to take that patch in
> > your series.
> Right, the second backup also drops the commands of the other jobs on
> the
> guilty ring, and skipping the begin helper is the better fix. With
> it,
> ring->guilty_fence also stays the guilty fence, so 02/15 isn't needed
> anymore.
> 
> I think the condition in your patch is inverted though:
> 
>         if (guilty_fence && ring->guilty_fence == guilty_fence)
>                 amdgpu_ring_reset_helper_begin(ring,
> ring_guilty_fence);
> 
> calls the begin helper only for a ring that was already backed up
> with
> this guilty fence. The other rings, and the guilty ring of a soft
> reset
> without a failed ring reset before it, aren't backed up anymore. The
> latter would even replay a stale backup. The guilty ring after a
> failed
> ring reset is the one ring that still goes through the begin helper.
> I
> think you meant:
> 
>         /* A failed ring reset of the same job already backed it up.
> */
>         if (ring != guilty_ring || ring->guilty_fence !=
> guilty_fence)
>                 amdgpu_ring_reset_helper_begin(ring,
> ring_guilty_fence);
> 
> If that's what you meant, I'll take your patch with that change in
> place
> of 02 in v4.
> 

Yes, thank you for the correction.