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-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.


> +
> +/**
> + * 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(...);


> +{
> +	struct amdgpu_ip_block *ip_block;
> +	u32 ring_type_mask;
> +	int r;
> +
> +	ip_block = amdgpu_device_ip_get_ip_block(adev, ip_type);
> +	if (!ip_block || !ip_block->version->funcs-
> >soft_reset_engine)
> +		return -EOPNOTSUPP;
> +
> +	dev_err(adev->dev, "Starting %s %s engine soft reset\n",
> +		ip_block->version->funcs->name,
> amdgpu_ip_engine_names[engine]);
> +
> +	ring_type_mask = amdgpu_ring_mask_from_engine(ip_type,
> engine);
> +
> +	if (engine == AMD_IP_ENGINE_ALL)
> +		amdgpu_device_lock_reset_domain(adev->reset_domain);
> +	else
> +		down_read(&adev->reset_domain->sem);
> +	amdgpu_multi_ring_reset_helper_begin(adev, ring_type_mask,
> guilty_ring,
> +					     guilty_fence);
> +
> +	r = ip_block->version->funcs->soft_reset_engine(ip_block,
> engine,
> +							guilty_queue
> );
> +
> +	r = amdgpu_multi_ring_reset_helper_end(adev, ring_type_mask,
> guilty_ring,
> +					       guilty_fence, r);
> +	if (engine == AMD_IP_ENGINE_ALL)
> +		amdgpu_device_unlock_reset_domain(adev-
> >reset_domain);
> +	else
> +		up_read(&adev->reset_domain->sem);
> +
> +	if (r) {
> +		dev_err(adev->dev, "Failed %s %s engine soft reset:
> %d\n",
> +			ip_block->version->funcs->name,
> +			amdgpu_ip_engine_names[engine], r);
> +		return r;
> +	}
> +
> +	dev_err(adev->dev, "Successful %s %s engine soft reset\n",
> +		ip_block->version->funcs->name,
> amdgpu_ip_engine_names[engine]);
> +	return 0;
> +}
> +
>  /**
>   * amdgpu_device_ip_soft_reset() - Perform a graceful soft reset on
> an IP block.
>   *
> @@ -430,6 +535,9 @@ static u32 amdgpu_ring_mask_from_ip(const enum
> amd_ip_block_type ip_type)
>   * resets all rings that belong to the same device IP block
>   * and re-initializes the device IP block.
>   *
> + * IP blocks with engines that can be reset separately only reset
> the engine
> + * of @guilty_ring, see amdgpu_device_ip_soft_reset_engine().
> + *
>   * The reset is handled gracefully, meaning that we try to
>   * minimize collateral damage (ie. avoid rejecting non-guilty jobs)
>   * as well as back up and restore the contents of all rings
> @@ -441,6 +549,7 @@ int amdgpu_device_ip_soft_reset(struct
> amdgpu_ring *guilty_ring,
>  	struct amdgpu_device *adev = guilty_ring->adev;
>  	struct amdgpu_ip_block *ip_block;
>  	enum amd_ip_block_type ip_type;
> +	enum amd_ip_engine engine;
>  	u32 ring_type_mask;
>  	int r;
>  
> @@ -453,6 +562,13 @@ int amdgpu_device_ip_soft_reset(struct
> amdgpu_ring *guilty_ring,
>  		return -EOPNOTSUPP;
>  	}
>  
> +	if (ip_block->version->funcs->soft_reset_engine) {
> +		engine = amdgpu_engine_from_ring(guilty_ring->funcs-
> >type);
> +		return amdgpu_device_ip_soft_reset_engine(adev,
> ip_type, engine,
> +							 
> guilty_ring,
> +							 
> guilty_fence, NULL);
> +	}
> +
>  	if (!ip_block->version->funcs->soft_reset) {
>  		dev_warn(adev->dev, "IP block soft reset not
> supported on %s\n",
>  			 ip_block->version->funcs->name);
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.h
> index 5dd6273982c4..856297d1c8e3 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ip.h
> @@ -160,5 +160,11 @@ bool amdgpu_device_ip_is_valid(struct
> amdgpu_device *adev,
>  			       enum amd_ip_block_type block_type);
>  int amdgpu_device_ip_soft_reset(struct amdgpu_ring *guilty_ring,
>  				struct amdgpu_fence *guilty_fence);
> +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);
>  
>  #endif /* __AMDGPU_IP_H__ */
> diff --git a/drivers/gpu/drm/amd/include/amd_shared.h
> b/drivers/gpu/drm/amd/include/amd_shared.h
> index e81ce9d79009..95cdd0fc191e 100644
> --- a/drivers/gpu/drm/amd/include/amd_shared.h
> +++ b/drivers/gpu/drm/amd/include/amd_shared.h
> @@ -29,6 +29,7 @@
>  
>  #define AMD_MAX_USEC_TIMEOUT		1000000  /* 1000 ms */
>  struct amdgpu_ip_block;
> +struct amdgpu_usermode_queue;
>  
>  
>  /*
> @@ -422,6 +423,18 @@ enum DC_DEBUG_MASK {
>  
>  enum amd_dpm_forced_level;
>  
> +/**
> + * enum amd_ip_engine - the engines of an IP block that can be soft
> reset
> + * @AMD_IP_ENGINE_ALL: all the engines of the IP block
> + * @AMD_IP_ENGINE_GFX: the graphics engine of the GFX IP block
> + * @AMD_IP_ENGINE_COMPUTE: the compute engine of the GFX IP block
> + */
> +enum amd_ip_engine {
> +	AMD_IP_ENGINE_ALL,
> +	AMD_IP_ENGINE_GFX,
> +	AMD_IP_ENGINE_COMPUTE,
> +};
> +
>  /**
>   * struct amd_ip_funcs - general hooks for managing amdgpu IP Blocks
>   * @name: Name of IP block
> @@ -441,6 +454,8 @@ enum amd_dpm_forced_level;
>   * @complete: handles IP specific changes after resume
>   * @wait_for_idle: poll for idle
>   * @soft_reset: soft reset the IP block
> + * @soft_reset_engine: soft reset one engine of the IP block, or all
> of them,
> + *                     and recover the user queues on it
>   * @set_clockgating_state: enable/disable cg for the IP block
>   * @set_powergating_state: enable/disable pg for the IP block
>   * @get_clockgating_state: get current clockgating status
> @@ -469,6 +484,9 @@ struct amd_ip_funcs {
>  	void (*complete)(struct amdgpu_ip_block *ip_block);
>  	int (*wait_for_idle)(struct amdgpu_ip_block *ip_block);
>  	int (*soft_reset)(struct amdgpu_ip_block *ip_block);
> +	int (*soft_reset_engine)(struct amdgpu_ip_block *ip_block,
> +				 enum amd_ip_engine engine,
> +				 struct amdgpu_usermode_queue
> *guilty_queue);
>  	int (*set_clockgating_state)(struct amdgpu_ip_block
> *ip_block,
>  				     enum amd_clockgating_state
> state);
>  	int (*set_powergating_state)(struct amdgpu_ip_block
> *ip_block,