Re: [PATCH 3/5] drm/amdgpu/sdma: Move SDMA v5.x queue reset to common code

["Lazar, Lijo" <lijo.lazar@amd.com>]

On 28-Aug-26 5:37 PM, Timur Kristóf wrote:
> The code was exactly the same between SDMA v5.0 and v5.2
> furthermore the exact same implementation can be shared
> between all SDMA versions that don't use MES.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c | 37 ++++++++++++++++++++++++
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h |  4 +++
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c   | 25 +---------------
>   drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c   | 25 +---------------
>   4 files changed, 43 insertions(+), 48 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 66f278f77f71..9eebd8380834 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -635,3 +635,40 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
>   
>   	return ret;
>   }
> +
> +/**
> + * amdgpu_sdma_reset_queue_legacy() - Reset legacy SDMA queue after timeout (without MES)
> + *
> + * @ring: Pointer to the ring of the SDMA queue
> + * @vmid: VMID of the timed out job
> + * @timedout_fence: Fence of the timed out job
> + *
> + * Common implementation for resetting SDMA queues without MES (legacy).
> + * This relies on the proper amdgpu_sdma_funcs to be set up
> + * for the given ring.
> + *
> + * Applicable to SDMA versions that don't rely on the MES yet,
> + * that is all versions up to SDMA v5.x and older.
> + */
> +int amdgpu_sdma_reset_queue_legacy(struct amdgpu_ring *ring,
> +				   unsigned int vmid,
> +				   struct amdgpu_fence *timedout_fence)
> +{
> +	struct amdgpu_device *adev = ring->adev;
> +	int r;
> +
> +	if (ring->me >= adev->sdma.num_instances) {
> +		dev_err(adev->dev, "sdma instance not found\n");
> +		return -EINVAL;
> +	}
> +
> +	amdgpu_ring_reset_helper_begin(ring, timedout_fence);
> +
> +	amdgpu_amdkfd_suspend(adev, true);
> +	r = amdgpu_sdma_reset_engine(adev, ring->me, true);
> +	amdgpu_amdkfd_resume(adev, true);
> +	if (r)
> +		return r;
> +
> +	return amdgpu_ring_reset_helper_end(ring, timedout_fence);
> +}

Instead of moving it here - sdma_v5_x_reset_queue and using it for 5.2 
is better. This may not work in the same way for all legacy queues.

Thanks,
Lijo

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index 4f4e56022c97..7c4e145ca0c1 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -160,6 +160,10 @@ struct amdgpu_buffer_funcs {
>   int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
>   			     bool caller_handles_kernel_queues);
>   
> +int amdgpu_sdma_reset_queue_legacy(struct amdgpu_ring *ring,
> +				   unsigned int vmid,
> +				   struct amdgpu_fence *timedout_fence);
> +
>   #define amdgpu_emit_copy_buffer(adev, ib, s, d, b, t) (adev)->mman.buffer_funcs->emit_copy_buffer((ib),  (s), (d), (b), (t))
>   #define amdgpu_emit_fill_buffer(adev, ib, s, d, b) (adev)->mman.buffer_funcs->emit_fill_buffer((ib), (s), (d), (b))
>   
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> index 0da54c335822..76f8765fb175 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_0.c
> @@ -1518,29 +1518,6 @@ static int sdma_v5_0_wait_for_idle(struct amdgpu_ip_block *ip_block)
>   	return -ETIMEDOUT;
>   }
>   
> -static int sdma_v5_0_reset_queue(struct amdgpu_ring *ring,
> -				 unsigned int vmid,
> -				 struct amdgpu_fence *timedout_fence)
> -{
> -	struct amdgpu_device *adev = ring->adev;
> -	int r;
> -
> -	if (ring->me >= adev->sdma.num_instances) {
> -		dev_err(adev->dev, "sdma instance not found\n");
> -		return -EINVAL;
> -	}
> -
> -	amdgpu_ring_reset_helper_begin(ring, timedout_fence);
> -
> -	amdgpu_amdkfd_suspend(adev, true);
> -	r = amdgpu_sdma_reset_engine(adev, ring->me, true);
> -	amdgpu_amdkfd_resume(adev, true);
> -	if (r)
> -		return r;
> -
> -	return amdgpu_ring_reset_helper_end(ring, timedout_fence);
> -}
> -
>   static int sdma_v5_0_stop_queue(struct amdgpu_ring *ring)
>   {
>   	u32 f32_cntl, freeze, cntl, stat1_reg;
> @@ -1936,7 +1913,7 @@ static const struct amdgpu_ring_funcs sdma_v5_0_ring_funcs = {
>   	.emit_reg_write_reg_wait = sdma_v5_0_ring_emit_reg_write_reg_wait,
>   	.init_cond_exec = sdma_v5_0_ring_init_cond_exec,
>   	.preempt_ib = sdma_v5_0_ring_preempt_ib,
> -	.reset = sdma_v5_0_reset_queue,
> +	.reset = amdgpu_sdma_reset_queue_legacy,
>   };
>   
>   static void sdma_v5_0_set_ring_funcs(struct amdgpu_device *adev)
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> index 5543e381dcca..e7f4b74f27b4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v5_2.c
> @@ -1436,29 +1436,6 @@ static int sdma_v5_2_wait_for_idle(struct amdgpu_ip_block *ip_block)
>   	return -ETIMEDOUT;
>   }
>   
> -static int sdma_v5_2_reset_queue(struct amdgpu_ring *ring,
> -				 unsigned int vmid,
> -				 struct amdgpu_fence *timedout_fence)
> -{
> -	struct amdgpu_device *adev = ring->adev;
> -	int r;
> -
> -	if (ring->me >= adev->sdma.num_instances) {
> -		dev_err(adev->dev, "sdma instance not found\n");
> -		return -EINVAL;
> -	}
> -
> -	amdgpu_ring_reset_helper_begin(ring, timedout_fence);
> -
> -	amdgpu_amdkfd_suspend(adev, true);
> -	r = amdgpu_sdma_reset_engine(adev, ring->me, true);
> -	amdgpu_amdkfd_resume(adev, true);
> -	if (r)
> -		return r;
> -
> -	return amdgpu_ring_reset_helper_end(ring, timedout_fence);
> -}
> -
>   static int sdma_v5_2_stop_queue(struct amdgpu_ring *ring)
>   {
>   	u32 f32_cntl, freeze, cntl, stat1_reg;
> @@ -1951,7 +1928,7 @@ static const struct amdgpu_ring_funcs sdma_v5_2_ring_funcs = {
>   	.emit_reg_write_reg_wait = sdma_v5_2_ring_emit_reg_write_reg_wait,
>   	.init_cond_exec = sdma_v5_2_ring_init_cond_exec,
>   	.preempt_ib = sdma_v5_2_ring_preempt_ib,
> -	.reset = sdma_v5_2_reset_queue,
> +	.reset = amdgpu_sdma_reset_queue_legacy,
>   };
>   
>   static void sdma_v5_2_set_ring_funcs(struct amdgpu_device *adev)