Re: [PATCH] drm/amdkfd: Move gpu_recovery location

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amdgpu: broken !amdgpu_gpu_recovery flow in
  amdgpu_gfx_reset_mes_compute()
--

commit ade3a30c574ad137adf2baa6ab425df87856ed34
Author: Amber Lin <amber.lin@amd.com>

drm/amdkfd: Move gpu_recovery location

This commit moves the amdgpu_gpu_recovery check from reset_queues_mes()
into amdgpu_gfx_reset_mes_compute() so bad queue removal can proceed when
gpu_recovery is disabled.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index c5a2dcff5058a..f8874c12c94b2 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -2602,6 +2602,10 @@ int amdgpu_gfx_reset_mes_compute(struct amdgpu_device *adev,
>  		 */
>  		goto fence_reset;
>  	}
> +
> +	if (!amdgpu_gpu_recovery)
> +		goto after_reset;
> +
>  	r = amdgpu_mes_detect_and_reset_hung_queues(adev, AMDGPU_RING_TYPE_COMPUTE,
>  						    true, &num_hung, adev->gfx.mec.mes_hung_db_array, xcc_id);
>  	if (r) {
> @@ -2662,6 +2666,7 @@ int amdgpu_gfx_reset_mes_compute(struct amdgpu_device *adev,
>  			goto out;
>  	}
>  
> +after_reset:
>  	/* MES doesn't detect any hung queue but we have a known bad queue
>  	 * and it is not KCQ
>  	 */

[Severity: High]
Does jumping to after_reset when amdgpu_gpu_recovery is 0 leave the kernel
compute ring schedulers permanently stopped after a queue hang?

amdgpu_gfx_reset_mes_compute() stops all kernel compute ring schedulers via
amdgpu_gfx_reset_stop_compute_scheds() before calling amdgpu_mes_suspend().
The new !amdgpu_gpu_recovery check is only reached when amdgpu_mes_suspend()
fails (r != 0), which happens when a queue is hung.

Jumping to after_reset with num_hung == 0 enters the (!num_hung &&
queue_input && !ring) block even though suspend_all failed:

drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c:amdgpu_gfx_reset_mes_compute() {
    ...
	if (!num_hung && queue_input && !ring) {
		/* MES suspend_all is successful means this bad queue is
		 * preempted successfuly. Remove it before resume all so it
		 * doesn't get mapped back
		 */
		...
		r = adev->mes.funcs->remove_hw_queue(&adev->mes, queue_input);
    ...
out:
	...
	ring_err = r;
	...
	if (!ring_err)
		amdgpu_gfx_reset_start_compute_scheds(adev, ring);
}

When queue_input is the hung queue (for example, when called from
remove_queue_mes_on_reset_option() or kfd_dqm_suspend_bad_queue_mes()),
remove_hw_queue() fails on the un-preempted queue and sets r to a non-zero
error.  At out:, amdgpu_gfx_reset_start_compute_scheds() is skipped because
ring_err is non-zero, and because amdgpu_gpu_recovery is 0 (or in
kfd_dqm_suspend_bad_queue_mes(), which only logs dev_dbg() on failure), no
GPU reset follows to restart the stopped compute schedulers.

Conversely, if amdgpu_mes_suspend() failed because a different queue was
hung while queue_input was already preempted, remove_hw_queue() can succeed
and overwrite the non-zero suspend error in r with 0.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113023337.21214-1-amber.lin@amd.com?part=1