[PATCH] drm/amdkfd: Move gpu_recovery location

[PATCH] drm/amdkfd: Move gpu_recovery location

[Amber Lin]

amdgpu_gfx_reset_mes_compute combines two features into one:
1) hung queue/pipe reset - suspend all/queue reset/resume all
2) bad queue removal - suspend_all/remove_queue/resume_all
GPU recovery aims on 1)resetting hung queues/pipes. When gpu_recovery is
disabled, we need to keep 2) going. Therefore move the gpu_recovery
check from KFD's reset_queues_mes into amdgpu_gfx_reset_mes_compute
where queue reset is done.

Signed-off-by: Amber Lin <amber.lin@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c               | 5 +++++
 drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c | 9 +++------
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index c5a2dcff5058..f8874c12c94b 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
 	 */
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 86b70748617b..b77cfd8ba907 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_device_queue_manager.c
@@ -471,11 +471,6 @@ static int reset_queues_mes(struct device_queue_manager *dqm, struct queue *q)
 	if (WARN_ON(dqm->detect_hang_count > 0))
 		return 0;
 
-	if (!amdgpu_gpu_recovery) {
-		r = -ENOTRECOVERABLE;
-		goto fail;
-	}
-
 	memset(&queue_input, 0x0, sizeof(struct mes_remove_queue_input));
 	queue_input.doorbell_offset = q->properties.doorbell_off;
 	queue_input.gang_context_addr = q->gang_ctx_gpu_addr;
@@ -3609,7 +3604,9 @@ int kfd_dqm_suspend_bad_queue_mes(struct kfd_node *knode, u32 pasid, u32 doorbel
 
 		list_for_each_entry(q, &qpd->queues_list, list) {
 			if (q->doorbell_id == doorbell_id && q->properties.is_active) {
-				reset_queues_mes(dqm, q);
+				if (reset_queues_mes(dqm, q))
+					dev_dbg(knode->adev->dev,
+					"Fail to remove bad queue. doorbell_id 0x%x\n", q->doorbell_id);
 				q->properties.is_evicted = true;
 				q->properties.is_active = false;
 				decrement_queue_count(dqm, qpd, q);
-- 
2.43.0

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

[sashiko-bot]

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