On 15/07/2026 09:08, Timur Kristóf wrote:
On 2026. július 14., kedd 20:47:14 közép-európai nyári idő Tvrtko Ursulin
wrote:
On 13/07/2026 13:58, Timur Kristóf wrote:
Call amdgpu_gfx_mqd_sw_init()/_fini() on GFX7 to initialize and
finalize the MQD, just like GFX8 and newer; instead of doing
an ad-hoc BO allocation. This introduces the possibility of
doing an MQD backup instead of trying to reinitialize the
MQD every time.
This solves an issue with GFX IP block soft reset where
all compute rings would hang after the reset.
Signed-off-by: Timur Kristóf <[email protected]>
---
drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 106 +++++++++++++-------------
1 file changed, 51 insertions(+), 55 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 65b8497ad5f0..9c4b3ac27e1f
100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -2698,25 +2698,6 @@ static int
gfx_v7_0_cp_compute_load_microcode(struct amdgpu_device *adev)>
return 0;
}
-/**
- * gfx_v7_0_cp_compute_fini - stop the compute queues
- *
- * @adev: amdgpu_device pointer
- *
- * Stop the compute queues and tear down the driver queue
- * info.
- */
-static void gfx_v7_0_cp_compute_fini(struct amdgpu_device *adev)
-{
- int i;
-
- for (i = 0; i < adev->gfx.num_compute_rings; i++) {
- struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
-
- amdgpu_bo_free_kernel(&ring->mqd_obj, NULL, NULL);
- }
-}
-
static void gfx_v7_0_mec_fini(struct amdgpu_device *adev)
{
amdgpu_bo_free_kernel(&adev->gfx.mec.hpd_eop_obj, NULL, NULL);
@@ -2788,28 +2769,29 @@ static void gfx_v7_0_compute_pipe_init(struct
amdgpu_device *adev,>
mutex_unlock(&adev->srbm_mutex);
}
-static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev)
+static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev, u32 req)
{
- int i;
+ int i, r = 0;
/* disable the queue if it's active */
- if (RREG32(mmCP_HQD_ACTIVE) & 1) {
- WREG32(mmCP_HQD_DEQUEUE_REQUEST, 1);
+ if (RREG32(mmCP_HQD_ACTIVE) & CP_HQD_ACTIVE__ACTIVE_MASK) {
+ WREG32_FIELD(CP_HQD_DEQUEUE_REQUEST, DEQUEUE_REQ, req);
for (i = 0; i < adev->usec_timeout; i++) {
- if (!(RREG32(mmCP_HQD_ACTIVE) & 1))
+ if (!(RREG32(mmCP_HQD_ACTIVE) &
CP_HQD_ACTIVE__ACTIVE_MASK))
break;
udelay(1);
}
if (i == adev->usec_timeout)
- return -ETIMEDOUT;
+ r = -ETIMEDOUT;
- WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
- WREG32(mmCP_HQD_PQ_RPTR, 0);
- WREG32(mmCP_HQD_PQ_WPTR, 0);
}
- return 0;
+ WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
+ WREG32(mmCP_HQD_PQ_RPTR, 0);
+ WREG32(mmCP_HQD_PQ_WPTR, 0);
+
+ return r;
I can see this matches gfx_v8_0_deactivate_hqd. If I am not missing
anything only to replace the hardcoded 1 with CP_HQD_ACTIVE__ACTIVE_MASK?
There are two changes here:
- Replacing the hardcoded "1" with the define from the register definition
- When it times out, still write the CP_HQD_ registers afterwards like gfx8
Right, thanks!
Is it okay to call the function mqd if the registers are hqd and is v7
or v8 (which calls it hqd) more correct? Not saying either way, just
observing a curiosity.
My best guess is that it's just that they used a different naming convention
and forgot to update the older code.
}
static void gfx_v7_0_mqd_init(struct amdgpu_device *adev,
@@ -2964,31 +2946,42 @@ static int gfx_v7_0_mqd_commit(struct
amdgpu_device *adev, struct cik_mqd *mqd)>
static int gfx_v7_0_compute_queue_init(struct amdgpu_device *adev, int
ring_id) {
- int r;
- u64 mqd_gpu_addr;
- struct cik_mqd *mqd;
struct amdgpu_ring *ring = &adev->gfx.compute_ring[ring_id];
-
- r = amdgpu_bo_create_reserved(adev, sizeof(struct cik_mqd),
PAGE_SIZE,
- AMDGPU_GEM_DOMAIN_GTT,
&ring->mqd_obj,
- &mqd_gpu_addr, (void
**)&mqd);
- if (r) {
- dev_warn(adev->dev, "(%d) create MQD bo failed\n", r);
- return r;
+ struct cik_mqd *mqd = ring->mqd_ptr;
+ int mqd_idx = ring - &adev->gfx.compute_ring[0];
+
+ if (!amdgpu_in_reset(adev) && !adev->in_suspend) {
+ memset((void *)mqd, 0, ring->mqd_size);
+ mutex_lock(&adev->srbm_mutex);
+ cik_srbm_select(adev, ring->me, ring->pipe, ring-
queue, 0);
+ gfx_v7_0_mqd_init(adev, mqd, ring->mqd_gpu_addr, ring);
+ gfx_v7_0_mqd_deactivate(adev, 1);
+ gfx_v7_0_mqd_commit(adev, mqd);
+ cik_srbm_select(adev, 0, 0, 0, 0);
+ mutex_unlock(&adev->srbm_mutex);
+
+ if (adev->gfx.mec.mqd_backup[mqd_idx])
+ memcpy(adev->gfx.mec.mqd_backup[mqd_idx],
mqd, ring->mqd_size);
+ } else {
+ /* restore MQD to a clean status */
+ if (adev->gfx.mec.mqd_backup[mqd_idx])
+ memcpy(mqd, adev-
gfx.mec.mqd_backup[mqd_idx], ring->mqd_size);
+
+ /* Re-commit the restored backup */
+ mutex_lock(&adev->srbm_mutex);
+ cik_srbm_select(adev, ring->me, ring->pipe, ring-
queue, 0);
+ gfx_v7_0_mqd_deactivate(adev, 2);
+ gfx_v7_0_mqd_commit(adev, mqd);
+ cik_srbm_select(adev, 0, 0, 0, 0);
+ mutex_unlock(&adev->srbm_mutex);
+
+ /* reset ring buffer */
+ ring->wptr = 0;
+ atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
+ atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
+ amdgpu_ring_clear_ring(ring);
}
- mutex_lock(&adev->srbm_mutex);
- cik_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
-
- gfx_v7_0_mqd_init(adev, mqd, mqd_gpu_addr, ring);
- gfx_v7_0_mqd_deactivate(adev);
- gfx_v7_0_mqd_commit(adev, mqd);
-
- cik_srbm_select(adev, 0, 0, 0, 0);
- mutex_unlock(&adev->srbm_mutex);
-
- amdgpu_bo_kunmap(ring->mqd_obj);
- amdgpu_bo_unreserve(ring->mqd_obj);
return 0;
}
I think I can follow this - only the wptr and rptr reset is a bit
different than what v8 does it. Any specific reason? Gfx9 then reverts
back to a single ring->wptr = 0. I guess v8 is somehow special?
@@ -3020,10 +3013,8 @@ static int gfx_v7_0_cp_compute_resume(struct
amdgpu_device *adev)>
/* init the queues */
for (i = 0; i < adev->gfx.num_compute_rings; i++) {
r = gfx_v7_0_compute_queue_init(adev, i);
- if (r) {
- gfx_v7_0_cp_compute_fini(adev);
+ if (r)
return r;
- }
}
gfx_v7_0_cp_compute_enable(adev, true);
@@ -4430,6 +4421,11 @@ static int gfx_v7_0_sw_init(struct amdgpu_ip_block
*ip_block)>
}
}
+ /* create MQD for all compute queues */
+ r = amdgpu_gfx_mqd_sw_init(adev, sizeof(struct cik_mqd), 0);
+ if (r)
+ return r;
+
adev->gfx.ce_ram_size = 0x8000;
gfx_v7_0_gpu_early_init(adev);
@@ -4452,7 +4448,7 @@ static int gfx_v7_0_sw_fini(struct amdgpu_ip_block
*ip_block)>
for (i = 0; i < adev->gfx.num_compute_rings; i++)
amdgpu_ring_fini(&adev->gfx.compute_ring[i]);
- gfx_v7_0_cp_compute_fini(adev);
+ amdgpu_gfx_mqd_sw_fini(adev, 0);
amdgpu_gfx_rlc_fini(adev);
gfx_v7_0_mec_fini(adev);
amdgpu_bo_free_kernel(&adev->gfx.rlc.clear_state_obj,
I am assuming all this applies only to compute because gfx is single
instance on v7?
It applies only to compute because only compute has HQD/MQD
on these hardware generations.
Anyway, it looks plausible to me so assuming you were able to exercise
both paths
What do you mean by "both paths"?
The two paths in gfx_v7_0_compute_queue_init. But you must of had since
one is the normal path and the other is on reset which you specifically
add and test. LGTM in this case.
Reviewed-by: Tvrtko Ursulin <[email protected]>
Regards,
Tvrtko
