Re: [PATCH 7/8] drm/amdgpu/sdma: Always handle kernel queues in amdgpu_sdma_reset_engine()

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

On 04-Sep-26 12:58 PM, Timur Kristóf wrote:
> Remove the caller_handles_kernel_queues argument from
> the amdgpu_sdma_reset_engine() function and make it
> always handle kernel queues.
> 
> Now the SDMA recovery sequence is more consistent
> between callers for the KFD as follows.
> Before recovery: first the KFD is suspended,
> then the SDMA queue contents are backed up.
> After recovery: first the SDMA queue contents
> are restored, then the KFD is resumed.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c      | 68 ++++++++++---------
>   drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h      |  3 +-
>   drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c      |  2 +-
>   .../drm/amd/amdkfd/kfd_device_queue_manager.c |  2 +-
>   4 files changed, 38 insertions(+), 37 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> index 9eebd8380834..07aac5b3ea92 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.c
> @@ -542,16 +542,14 @@ static int amdgpu_sdma_soft_reset(struct amdgpu_device *adev, u32 instance_id)
>   }
>   
>   /**
> - * amdgpu_sdma_reset_engine - Reset a specific SDMA engine
> + * amdgpu_sdma_reset_engine() - Reset a specific SDMA engine instance.
> + *
>    * @adev: Pointer to the AMDGPU device
>    * @instance_id: Logical ID of the SDMA engine instance to reset
> - * @caller_handles_kernel_queues: Skip kernel queue processing. Caller
> - * will handle it.
>    *
>    * Returns: 0 on success, or a negative error code on failure.
>    */
> -int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
> -			     bool caller_handles_kernel_queues)
> +int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id)
>   {
>   	struct amdgpu_sdma_instance *sdma_instance = &adev->sdma.instance[instance_id];
>   	struct amdgpu_ring *gfx_ring = &sdma_instance->ring;
> @@ -564,20 +562,23 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
>   
>   	mutex_lock(&sdma_instance->engine_reset_mutex);
>   
> -	if (!caller_handles_kernel_queues) {
> -		/* Stop the scheduler's work queue for the GFX and page rings if they are running.
> -		 * This ensures that no new tasks are submitted to the queues while
> -		 * the reset is in progress.
> -		 */
> +	/*
> +	 * Stop the scheduler's work queue for the GFX and page rings if they are running.
> +	 * This ensures that no new tasks are submitted to the queues while
> +	 * the reset is in progress.
> +	 */
> +	if (amdgpu_ring_sched_ready(gfx_ring) && !drm_sched_is_stopped(&gfx_ring->sched))
>   		drm_sched_wqueue_stop(&gfx_ring->sched);
> -		gfx_fence = amdgpu_ring_find_guilty_fence(gfx_ring);
> -		amdgpu_ring_reset_helper_begin(gfx_ring, gfx_fence);
>   
> -		if (adev->sdma.has_page_queue) {
> +	gfx_fence = amdgpu_ring_find_guilty_fence(gfx_ring);
> +	amdgpu_ring_reset_helper_begin(gfx_ring, gfx_fence);
> +
> +	if (adev->sdma.has_page_queue) {
> +		if (amdgpu_ring_sched_ready(page_ring) && !drm_sched_is_stopped(&page_ring->sched))
>   			drm_sched_wqueue_stop(&page_ring->sched);
> -			page_fence = amdgpu_ring_find_guilty_fence(page_ring);
> -			amdgpu_ring_reset_helper_begin(page_ring, page_fence);
> -		}
> +
> +		page_fence = amdgpu_ring_find_guilty_fence(page_ring);
> +		amdgpu_ring_reset_helper_begin(page_ring, page_fence);
>   	}


Since this resets the engine, a different way may be to have something 
like below (similar to amdgpu_multi_ring_reset_helper_begin) which takes 
care of all rings in the engine instance.

amdgpu_ring_engine_reset_helper_begin(guilty_ring, guilty_fence);

amdgpu_ring_engine_reset_helper_end(guilty_ring, guilty_fence);

ring_type = guilty_ring->funcs->type;
eng_instance = guilty_ring->me

Thanks,
Lijo

>   
>   	if (sdma_instance->funcs->stop_kernel_queue) {
> @@ -612,22 +613,25 @@ int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
>   	}
>   
>   exit:
> -	if (!caller_handles_kernel_queues) {
> -		/* Restart the scheduler's work queue for the GFX and page rings
> -		 * if they were stopped by this function. This allows new tasks
> -		 * to be submitted to the queues after the reset is complete.
> -		 */
> -		if (!ret) {
> -			ret = amdgpu_ring_reset_helper_end(gfx_ring, gfx_fence);
> +	/* Restart the scheduler's work queue for the GFX and page rings
> +	 * if they were stopped by this function. This allows new tasks
> +	 * to be submitted to the queues after the reset is complete.
> +	 */
> +	if (!ret) {
> +		ret = amdgpu_ring_reset_helper_end(gfx_ring, gfx_fence);
> +		if (ret)
> +			goto unlock;
> +
> +		if (amdgpu_ring_sched_ready(gfx_ring))
> +			drm_sched_wqueue_start(&gfx_ring->sched);
> +
> +		if (adev->sdma.has_page_queue) {
> +			ret = amdgpu_ring_reset_helper_end(page_ring, page_fence);
>   			if (ret)
>   				goto unlock;
> -			drm_sched_wqueue_start(&gfx_ring->sched);
> -			if (adev->sdma.has_page_queue) {
> -				ret = amdgpu_ring_reset_helper_end(page_ring, page_fence);
> -				if (ret)
> -					goto unlock;
> +
> +			if (amdgpu_ring_sched_ready(page_ring))
>   				drm_sched_wqueue_start(&page_ring->sched);
> -			}
>   		}
>   	}
>   unlock:
> @@ -662,13 +666,11 @@ int amdgpu_sdma_reset_queue_legacy(struct amdgpu_ring *ring,
>   		return -EINVAL;
>   	}
>   
> -	amdgpu_ring_reset_helper_begin(ring, timedout_fence);
> -
>   	amdgpu_amdkfd_suspend(adev, true);
> -	r = amdgpu_sdma_reset_engine(adev, ring->me, true);
> +	r = amdgpu_sdma_reset_engine(adev, ring->me);
>   	amdgpu_amdkfd_resume(adev, true);
>   	if (r)
>   		return r;
>   
> -	return amdgpu_ring_reset_helper_end(ring, timedout_fence);
> +	return 0;
>   }
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> index cb41453c1a19..5709d438e824 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_sdma.h
> @@ -153,8 +153,7 @@ struct amdgpu_buffer_funcs {
>   				 uint32_t byte_count);
>   };
>   
> -int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id,
> -			     bool caller_handles_kernel_queues);
> +int amdgpu_sdma_reset_engine(struct amdgpu_device *adev, uint32_t instance_id);
>   
>   int amdgpu_sdma_reset_queue_legacy(struct amdgpu_ring *ring,
>   				   unsigned int vmid,
> diff --git a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> index 77f385b9ef53..796ea9f74763 100644
> --- a/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> +++ b/drivers/gpu/drm/amd/amdgpu/sdma_v4_4_2.c
> @@ -1583,7 +1583,7 @@ static int sdma_v4_4_2_reset_queue(struct amdgpu_ring *ring,
>   	int r;
>   
>   	amdgpu_amdkfd_suspend(adev, true);
> -	r = amdgpu_sdma_reset_engine(adev, id, false);
> +	r = amdgpu_sdma_reset_engine(adev, id);
>   	amdgpu_amdkfd_resume(adev, true);
>   	return r;
>   }
> diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> index a23384571193..f9fbc5628fb4 100644
> --- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> +++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
> @@ -2576,7 +2576,7 @@ static int reset_hung_queues_sdma(struct device_queue_manager *dqm)
>   				continue;
>   
>   			/* Reset engine and check. */
> -			if (amdgpu_sdma_reset_engine(dqm->dev->adev, i, false) ||
> +			if (amdgpu_sdma_reset_engine(dqm->dev->adev, i) ||
>   			    dqm->dev->kfd2kgd->hqd_sdma_get_doorbell(dqm->dev->adev, i, j) ||
>   			    !set_sdma_queue_as_reset(dqm, doorbell_off)) {
>   				r = -ENOTRECOVERABLE;