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 9:07 PM, Timur Kristóf wrote:
> On Friday, August 28, 2026 4:20:19 PM Central European Summer Time Lazar, Lijo
> wrote:
>> 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
> 
> Hi,
> 
> As far as I see, this function can be reused for all generations that don't
> use MES. If you don't think so, please explain why not.
> 

This function doesn't take care of paging ring (if enabled). Or, this 
could be made simlar to the one in sdma 4.4.2. It also makes use of the 
helpers, but not sure if the sequence reversal (save content -> kfd 
suspend vs kfd suspend -> save content) has other side effects.

Thanks,
Lijo

> Thanks,
> Timur
> 
>>
>>> 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)
> 
> 
> 
>