Re: [PATCH v3 03/15] drm/amdgpu: add an engine scope to the IP block soft reset

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

On Wed, 2026-11-04 at 03:32 +0000, Zhang, Jesse(Jie) wrote:
> AMD General
> 
> > -----Original Message-----
> > From: Timur Kristóf <timur.kristof@gmail.com>
> > Sent: Wednesday, November 4, 2026 4:16 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 03/15] drm/amdgpu: add an engine scope to
> > the IP
> > block soft reset
> > 
> > On Wed, 2026-10-28 at 16:56 +0800, Jesse Zhang wrote:
> > > Some IP blocks have engines that the hardware can reset on their
> > > own,
> > > eg. the graphics and compute engines of the GFX IP
> > > (CP_SOFT_RESET_CNTL.GFX_ONLY_SOFT_RESET / CMP_ONLY_SOFT_RESET).
> > > Resetting
> > > only the engine of a hung ring leaves the work on the other
> > > engines
> > > alone, which the IP block soft reset can't do, as it resets every
> > > ring
> > > of the IP block.
> > > 
> > > Add an optional soft_reset_engine() IP block callback, which
> > > resets
> > > one engine of the IP block, or all of them, and recovers the user
> > > queues on it, and amdgpu_device_ip_soft_reset_engine() around it.
> > > It
> > > only stops, backs up and replays the rings of that engine, and it
> > > is
> > > also reachable without a guilty kernel ring, for a hung user
> > > queue.
> > > 
> > > For a single engine, it only takes the reset domain for read: KFD
> > > only
> > > try-locks it for read before it adds or removes queues, and would
> > > fail
> > > those calls with the write lock although the other engines aren't
> > > reset.
> > > 
> > > amdgpu_device_ip_soft_reset() uses it when the IP block
> > > implements it,
> > > for the engine of the guilty ring.
> > > 
> > > v3: replaces the separate engine reset type and helper
> > >     (Timur).
> > > 
> > > Suggested-by: Timur Kristóf <timur.kristof@gmail.com>
> > > Signed-off-by: Jesse Zhang <Jesse.Zhang@amd.com>
> > > ---
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ip.c   | 116
> > > +++++++++++++++++++++++
> > >  drivers/gpu/drm/amd/amdgpu/amdgpu_ip.h   |   6 ++
> > >  drivers/gpu/drm/amd/include/amd_shared.h |  18 ++++
> > >  3 files changed, 140 insertions(+)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.c
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.c
> > > index 2eea18965fd1..e95f69dd3142 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.c
> > > @@ -418,6 +418,111 @@ static u32 amdgpu_ring_mask_from_ip(const
> > enum
> > > amd_ip_block_type ip_type)
> > >     }
> > >  }
> > > 
> > > +/**
> > > + * amdgpu_ring_mask_from_engine() - Find mask of ring types of
> > > an
> > > engine.
> > > + *
> > > + * @ip_type: The IP block type of the engine.
> > > + * @engine: The engine whose rings you are looking for.
> > > + */
> > > +static u32 amdgpu_ring_mask_from_engine(const enum
> > amd_ip_block_type
> > > ip_type,
> > > +                                   const enum amd_ip_engine
> > > engine)
> > > +{
> > > +   switch (engine) {
> > > +   case AMD_IP_ENGINE_GFX:
> > > +           return BIT(AMDGPU_RING_TYPE_GFX);
> > > +
> > > +   case AMD_IP_ENGINE_COMPUTE:
> > > +           return BIT(AMDGPU_RING_TYPE_COMPUTE);
> > > +
> > > +   default:
> > > +           return amdgpu_ring_mask_from_ip(ip_type);
> > > +   }
> > > +}
> > > +
> > > +/**
> > > + * amdgpu_engine_from_ring() - Find the engine of a ring type.
> > > + *
> > > + * @ring_type: The ring type whose engine you are looking for.
> > > + */
> > > +static enum amd_ip_engine amdgpu_engine_from_ring(const enum
> > > amdgpu_ring_type ring_type)
> > > +{
> > > +   switch (ring_type) {
> > > +   case AMDGPU_RING_TYPE_GFX:
> > > +           return AMD_IP_ENGINE_GFX;
> > > +
> > > +   case AMDGPU_RING_TYPE_COMPUTE:
> > > +           return AMD_IP_ENGINE_COMPUTE;
> > > +
> > > +   default:
> > > +           return AMD_IP_ENGINE_ALL;
> > > +   }
> > > +}
> > > +
> > > +static const char *const amdgpu_ip_engine_names[] = {
> > > +   [AMD_IP_ENGINE_ALL] = "all",
> > > +   [AMD_IP_ENGINE_GFX] = "gfx",
> > > +   [AMD_IP_ENGINE_COMPUTE] = "compute", };
> > 
> > No need to add a new enum.
> > This is redundant with amdgpu_ring_type.
> Agreed. In v4 the callback takes the ring type of the hung queue.

Sounds good, thanks!

> > 
> > 
> > > +
> > > +/**
> > > + * amdgpu_device_ip_soft_reset_engine() - Soft reset one engine
> > > of
> > > an IP block.
> > > + *
> > > + * @adev: amdgpu device
> > > + * @ip_type: The IP block type.
> > > + * @engine: The engine to reset, or AMD_IP_ENGINE_ALL for the
> > > whole
> > > IP block.
> > > + * @guilty_ring: The ring which is guilty of causing a reset, or
> > > NULL.
> > > + * @guilty_fence: The fence which didn't signal on @guilty_ring.
> > > + * @guilty_queue: The user queue which is guilty of causing a
> > > reset,
> > > or NULL.
> > > + */
> > > +int amdgpu_device_ip_soft_reset_engine(struct amdgpu_device
> > > *adev,
> > > +                                  enum amd_ip_block_type
> > > ip_type,
> > > +                                  enum amd_ip_engine engine,
> > > +                                  struct amdgpu_ring
> > > *guilty_ring,
> > > +                                  struct amdgpu_fence
> > > *guilty_fence,
> > > +                                  struct amdgpu_usermode_queue
> > > *guilty_queue)
> > 
> > Please don't add a separate function.
> > This duplicates a lot of code that shouldn't be duplicated.
> > 
> > Instead, I suggest to just add the new functionality to
> > amdgpu_device_ip_soft_reset(), for example like this:
> > 
> > if (ip_block->version->funcs->soft_reset_engine)
> >   ip_block->version->funcs->soft_reset_engine(...);
> > else if (ip_block->version->funcs->soft_reset)
> >   ip_block->version->funcs->soft_reset(...);
> 
> OK, I'll fold it into amdgpu_device_ip_soft_reset(). The separate
> entry
> point was only there for the user queue caller (patch 13): a hung gfx
> user queue has no kernel ring, and amdgpu_device_ip_soft_reset() gets
> the
> device and the IP block from guilty_ring. So the signature would
> become:
> 
>         int amdgpu_device_ip_soft_reset(struct amdgpu_device *adev,
>                                         enum amdgpu_ring_type
> ring_type,
>                                         struct amdgpu_ring
> *guilty_ring,
>                                         struct amdgpu_fence
> *guilty_fence,
>                                         struct amdgpu_usermode_queue
> *guilty_queue);
> 
> amdgpu_job_timedout() passes (ring->adev, ring->funcs->type, ring,
> job->hw_fence, NULL), and the user queue reset passes (adev,
> AMDGPU_RING_TYPE_GFX, NULL, NULL, queue). The body stays the same as
> today. When the IP block has soft_reset_engine, it uses that
> callback,
> only the rings of that type, and the reset domain for read:
> 
>         if (funcs->soft_reset_engine) {
>                 ring_type_mask = BIT(ring_type);
>                 down_read(&adev->reset_domain->sem);
>         } else {
>                 if (!guilty_ring || !funcs->soft_reset)
>                         return -EOPNOTSUPP;
>                 ring_type_mask = amdgpu_ring_mask_from_ip(ip_type);
>                 amdgpu_device_lock_reset_domain(adev->reset_domain);
>         }
> 
>         amdgpu_multi_ring_reset_helper_begin(adev, ring_type_mask,
>                                              guilty_ring,
> guilty_fence);
>         if (funcs->soft_reset_engine)
>                 r = funcs->soft_reset_engine(ip_block, ring_type,
> guilty_queue);
>         else
>                 r = funcs->soft_reset(ip_block);
>         r = amdgpu_multi_ring_reset_helper_end(adev, ring_type_mask,
>                                                guilty_ring,
> guilty_fence, r);
> 
>         if (funcs->soft_reset_engine)
>                 up_read(&adev->reset_domain->sem);
>         else
>                 amdgpu_device_unlock_reset_domain(adev-
> >reset_domain);
> 
> Does the change work for you?

Yes, that sounds good to me.
I just have one question about it:

> the user queue reset passes (adev,
> AMDGPU_RING_TYPE_GFX, NULL, NULL, queue)

Wouldn't the user queue reset still need to pass the guilty fence from
the user queue? Or is that not possible?

Thanks,
Timur