[PATCH 0/9] drm/amdgpu/gfx7: Use GFX IP block soft reset on GFX7

[PATCH 0/9] drm/amdgpu/gfx7: Use GFX IP block soft reset on GFX7

[Timur Kristóf]

GFX IP block soft reset has been implemented as recovery
method so that we can have a way to reset just the GFX
block without resetting the whole GPU or losing the
contents of VRAM.

Fix up various things in the GFX7 code to prepare it
to work with IP block soft reset. The main challenge
was fixing up the MQD/HQD handling to ensure that it
doesn't hang after the soft reset. There are also
other minor fixups.

Rework the pre-existing GFX7 soft reset implementation
like the reworked version on GFX8 to make it more robust.
In practice, this means that it will now reset everything
in the GFX IP block (instead of taking a guess at which
parts need to be reset) to make it consistent, and it now
also handles clock and power gating to make sure not to
degrade GPU functionality after a GFX IP block soft reset.

This improves current user experience on all GFX7 chips:

On Kaveri and Kabini there is currently no working
GPU recovery method so those chips currently require
the user to manually reset the computer when there
was a hang.

On Hawaii and Bonaire, the current GPU recovery method
always clears the contents of VRAM, which means that
a buggy (hanging) app can crash the whole graphical
session, which is less than ideal.

Using GFX IP block soft reset means that we can now
have a working recovery on GFX7 APUs and we can also
move on from GFX hangs on dGPUs without crashing the
whole system.

Tested on the following chips:

Bonaire (Radeon HD 7790)
Hawaii (Radeon R9 390X)
Kaveri (A10-7850K)

Timur Kristóf (9):
  drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7
  drm/amdgpu/gfx7: Refactor MQD initialization and finalization
  drm/amdgpu/gfx7: Return error code when compute ring tests fail
  drm/amdgpu/gfx7: Return error code when failing to start GFX ring
  drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets
  drm/amdgpu/gfx7: Clean up gfx ring during reset
  drm/amdgpu/gfx7: Use COND_EXEC
  drm/amdgpu/gfx7: Fixup IP block soft reset
  drm/amdgpu/gfx7: Enable IP block soft reset as a GPU recovery method

 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c |   2 +-
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c   | 333 +++++++++++++-----------
 2 files changed, 181 insertions(+), 154 deletions(-)

-- 
2.55.0

[PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Timur Kristóf]

We don't use KIQ on GFX7 but otherwise MQD works the
same way as GFX8 and newer.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
index 96c9d4f00b27..0f142c156afa 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
@@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
 #endif
 
 	/* create MQD for KIQ */
-	if (!adev->enable_mes_kiq && !ring->mqd_obj) {
+	if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq && !ring->mqd_obj) {
 		/* originaly the KIQ MQD is put in GTT domain, but for SRIOV VRAM domain is a must
 		 * otherwise hypervisor trigger SAVE_VF fail after driver unloaded which mean MQD
 		 * deallocated and gart_unbind, to strict diverage we decide to use VRAM domain for
-- 
2.55.0

Re: [PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> We don't use KIQ on GFX7 but otherwise MQD works the
> same way as GFX8 and newer.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> index 96c9d4f00b27..0f142c156afa 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> @@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>   #endif
>   
>   	/* create MQD for KIQ */
> -	if (!adev->enable_mes_kiq && !ring->mqd_obj) {
> +	if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq && !ring->mqd_obj) {

CHIP_TOPAZ is gfx7? Hm if it is then the branch would already run there. 
So the change is limiting the branch to a subset of platforms, while the 
patch title made me think it is enabling something on gfx7. Perhaps 
somehow indirectly or what am I not understanding?

Regards,

Tvrtko

>   		/* originaly the KIQ MQD is put in GTT domain, but for SRIOV VRAM domain is a must
>   		 * otherwise hypervisor trigger SAVE_VF fail after driver unloaded which mean MQD
>   		 * deallocated and gart_unbind, to strict diverage we decide to use VRAM domain for

Re: [PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Alex Deucher]

On Tue, Jul 14, 2026 at 10:59 AM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>
>
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > We don't use KIQ on GFX7 but otherwise MQD works the
> > same way as GFX8 and newer.
> >
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > index 96c9d4f00b27..0f142c156afa 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> > @@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
> >   #endif
> >
> >       /* create MQD for KIQ */
> > -     if (!adev->enable_mes_kiq && !ring->mqd_obj) {
> > +     if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq && !ring->mqd_obj) {
>
> CHIP_TOPAZ is gfx7? Hm if it is then the branch would already run there.
> So the change is limiting the branch to a subset of platforms, while the
> patch title made me think it is enabling something on gfx7. Perhaps
> somehow indirectly or what am I not understanding?

TOPAZ is gfx8.

Alex

>
> Regards,
>
> Tvrtko
>
> >               /* originaly the KIQ MQD is put in GTT domain, but for SRIOV VRAM domain is a must
> >                * otherwise hypervisor trigger SAVE_VF fail after driver unloaded which mean MQD
> >                * deallocated and gart_unbind, to strict diverage we decide to use VRAM domain for
>

Re: [PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Tvrtko Ursulin]

On 14/07/2026 16:05, Alex Deucher wrote:
> On Tue, Jul 14, 2026 at 10:59 AM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>
>>
>> On 13/07/2026 13:58, Timur Kristóf wrote:
>>> We don't use KIQ on GFX7 but otherwise MQD works the
>>> same way as GFX8 and newer.
>>>
>>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>>> ---
>>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
>>>    1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> index 96c9d4f00b27..0f142c156afa 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>> @@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device *adev,
>>>    #endif
>>>
>>>        /* create MQD for KIQ */
>>> -     if (!adev->enable_mes_kiq && !ring->mqd_obj) {
>>> +     if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq && !ring->mqd_obj) {
>>
>> CHIP_TOPAZ is gfx7? Hm if it is then the branch would already run there.
>> So the change is limiting the branch to a subset of platforms, while the
>> patch title made me think it is enabling something on gfx7. Perhaps
>> somehow indirectly or what am I not understanding?
> 
> TOPAZ is gfx8.

Ah now I get it, thank you! Could maybe adev->gfx[0].kiq.something or be 
used to make it a bit self-documenting?
Regards,

Tvrtko

>>
>>>                /* originaly the KIQ MQD is put in GTT domain, but for SRIOV VRAM domain is a must
>>>                 * otherwise hypervisor trigger SAVE_VF fail after driver unloaded which mean MQD
>>>                 * deallocated and gart_unbind, to strict diverage we decide to use VRAM domain for
>>

Re: [PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Timur Kristóf]

On Tuesday, July 14, 2026 5:19:57 PM Central European Summer Time Tvrtko 
Ursulin wrote:
> On 14/07/2026 16:05, Alex Deucher wrote:
> > On Tue, Jul 14, 2026 at 10:59 AM Tvrtko Ursulin <tursulin@ursulin.net> 
wrote:
> >> On 13/07/2026 13:58, Timur Kristóf wrote:
> >>> We don't use KIQ on GFX7 but otherwise MQD works the
> >>> same way as GFX8 and newer.
> >>> 
> >>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> >>> ---
> >>> 
> >>>    drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
> >>>    1 file changed, 1 insertion(+), 1 deletion(-)
> >>> 
> >>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> >>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c index
> >>> 96c9d4f00b27..0f142c156afa 100644
> >>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> >>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
> >>> @@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device
> >>> *adev,
> >>> 
> >>>    #endif
> >>>    
> >>>        /* create MQD for KIQ */
> >>> 
> >>> -     if (!adev->enable_mes_kiq && !ring->mqd_obj) {
> >>> +     if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq &&
> >>> !ring->mqd_obj) {>> 
> >> CHIP_TOPAZ is gfx7? Hm if it is then the branch would already run there.
> >> So the change is limiting the branch to a subset of platforms, while the
> >> patch title made me think it is enabling something on gfx7. Perhaps
> >> somehow indirectly or what am I not understanding?
> > 
> > TOPAZ is gfx8.

We currently don't use KIQ on GFX7 so the patch changes the code to allocate 
the BO only on GFX8 and newer. Topaz is the first GFX8 chip in the enum, so 
that's why the code checks >= TOPAZ here.

> Ah now I get it, thank you! Could maybe adev->gfx[0].kiq.something or be
> used to make it a bit self-documenting?

Technically, GFX7 supports the KIQ, amdgpu just doesn't use it. So, I fear 
that adding a field would mislead the reader into thinking that the HW support 
is missing when it really is just the kernel doesn't use it.

How would you feel about just updating the comment above the changed line?
Maybe like this?

/* create MQD for KIQ - only on GFX8+ GPUs where we use the KIQ */

I think that would make it self-explanatory.

> 
> >>>                /* originaly the KIQ MQD is put in GTT domain, but for
> >>>                SRIOV VRAM domain is a must>>>                
> >>>                 * otherwise hypervisor trigger SAVE_VF fail after driver
> >>>                 unloaded which mean MQD * deallocated and gart_unbind,
> >>>                 to strict diverage we decide to use VRAM domain for

Re: [PATCH 1/9] drm/amdgpu/gfx7: Make amdgpu_gfx_mqd_sw_init() usable on GFX7

[Tvrtko Ursulin]

On 14/07/2026 16:39, Timur Kristóf wrote:
> On Tuesday, July 14, 2026 5:19:57 PM Central European Summer Time Tvrtko
> Ursulin wrote:
>> On 14/07/2026 16:05, Alex Deucher wrote:
>>> On Tue, Jul 14, 2026 at 10:59 AM Tvrtko Ursulin <tursulin@ursulin.net>
> wrote:
>>>> On 13/07/2026 13:58, Timur Kristóf wrote:
>>>>> We don't use KIQ on GFX7 but otherwise MQD works the
>>>>> same way as GFX8 and newer.
>>>>>
>>>>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>>>>> ---
>>>>>
>>>>>     drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c | 2 +-
>>>>>     1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c index
>>>>> 96c9d4f00b27..0f142c156afa 100644
>>>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gfx.c
>>>>> @@ -420,7 +420,7 @@ int amdgpu_gfx_mqd_sw_init(struct amdgpu_device
>>>>> *adev,
>>>>>
>>>>>     #endif
>>>>>     
>>>>>         /* create MQD for KIQ */
>>>>>
>>>>> -     if (!adev->enable_mes_kiq && !ring->mqd_obj) {
>>>>> +     if (adev->asic_type >= CHIP_TOPAZ && !adev->enable_mes_kiq &&
>>>>> !ring->mqd_obj) {>>
>>>> CHIP_TOPAZ is gfx7? Hm if it is then the branch would already run there.
>>>> So the change is limiting the branch to a subset of platforms, while the
>>>> patch title made me think it is enabling something on gfx7. Perhaps
>>>> somehow indirectly or what am I not understanding?
>>>
>>> TOPAZ is gfx8.
> 
> We currently don't use KIQ on GFX7 so the patch changes the code to allocate
> the BO only on GFX8 and newer. Topaz is the first GFX8 chip in the enum, so
> that's why the code checks >= TOPAZ here.
> 
>> Ah now I get it, thank you! Could maybe adev->gfx[0].kiq.something or be
>> used to make it a bit self-documenting?
> 
> Technically, GFX7 supports the KIQ, amdgpu just doesn't use it. So, I fear
> that adding a field would mislead the reader into thinking that the HW support
> is missing when it really is just the kernel doesn't use it.
> 
> How would you feel about just updating the comment above the changed line?
> Maybe like this?
> 
> /* create MQD for KIQ - only on GFX8+ GPUs where we use the KIQ */
> 
> I think that would make it self-explanatory.

Your call if you think that adds value or not much. I was simply 
wondering if there is some sort of a "central" source of knowledge, like 
a function or variable under gfx or adev, which is colloquially used as 
"do we use kiq". If there isn't that's fine. The patch LGTM.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

> 
>>
>>>>>                 /* originaly the KIQ MQD is put in GTT domain, but for
>>>>>                 SRIOV VRAM domain is a must>>>
>>>>>                  * otherwise hypervisor trigger SAVE_VF fail after driver
>>>>>                  unloaded which mean MQD * deallocated and gart_unbind,
>>>>>                  to strict diverage we decide to use VRAM domain for
> 
> 
> 
> 

[PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Timur Kristóf]

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 <timur.kristof@gmail.com>
---
 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;
 }
 
 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;
 }
 
@@ -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,
-- 
2.55.0

Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Tvrtko Ursulin]

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 <timur.kristof@gmail.com>
> ---
>   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?

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.

>   }
>   
>   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?

Anyway, it looks plausible to me so assuming you were able to exercise 
both paths:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Timur Kristóf]

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 <timur.kristof@gmail.com>
> > ---
> > 
> >   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

> 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"?

Thanks,
Timur

Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Tvrtko Ursulin]

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 <timur.kristof@gmail.com>
>>> ---
>>>
>>>    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 <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Timur Kristóf]

On 2026. július 15., szerda 10:26:53 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> > 
> >> 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.

Yes, I tested both paths. I wrote this patch because I noticed that all 
compute queues hang after a soft reset without this.

Timur

Re: [PATCH 2/9] drm/amdgpu/gfx7: Refactor MQD initialization and finalization

[Alex Deucher]

On Wed, Jul 15, 2026 at 11:54 AM Timur Kristóf <timur.kristof@gmail.com> 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 <timur.kristof@gmail.com>
> > > ---
> > >
> > >   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
>
> > 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.

Technically, the MQD (Memory Queue Descriptor) and HQD (Hardware Queue
Descriptor) are separate things.  The MQD is a structure stored in
memory which stores the state of a queue.  The HQD is the hardware
state which stores the queue when it's mapped to hardware.  The way
it's supposed to work is that the driver sets up MQDs in memory and
then asks KIQ to map those queues to an HQD or put them on a runlist
(for user queues) if there are more MQDs than HQDs.  For gfx8 and
newer the driver sets up the MQD and then uses the KIQ to map the
queues.  gfx7 could work that way as well, but we never got around to
porting the changes back to gfx7.  Instead the driver maps the queues
to the HQD directly via MMIO rather than having KIQ do it on gfx7.

Alex

>
> >
> > >   }
> > >
> > >   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"?
>
> Thanks,
> Timur
>
>

[PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Timur Kristóf]

The gfx_v7_0_cp_compute_resume() function should only return
success when all compute rings are actually functional.
This will be especially important for soft reset which needs
this to know whether the reset was successful.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++--
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 9c4b3ac27e1f..a1a9f3fc4567 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -3019,12 +3019,14 @@ static int gfx_v7_0_cp_compute_resume(struct amdgpu_device *adev)
 
 	gfx_v7_0_cp_compute_enable(adev, true);
 
+	r = 0;
+
 	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
 		ring = &adev->gfx.compute_ring[i];
-		amdgpu_ring_test_helper(ring);
+		r |= amdgpu_ring_test_helper(ring);
 	}
 
-	return 0;
+	return r;
 }
 
 static void gfx_v7_0_cp_enable(struct amdgpu_device *adev, bool enable)
-- 
2.55.0

Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> The gfx_v7_0_cp_compute_resume() function should only return
> success when all compute rings are actually functional.
> This will be especially important for soft reset which needs
> this to know whether the reset was successful.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++--
>   1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 9c4b3ac27e1f..a1a9f3fc4567 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -3019,12 +3019,14 @@ static int gfx_v7_0_cp_compute_resume(struct amdgpu_device *adev)
>   
>   	gfx_v7_0_cp_compute_enable(adev, true);
>   
> +	r = 0;
> +
>   	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>   		ring = &adev->gfx.compute_ring[i];
> -		amdgpu_ring_test_helper(ring);
> +		r |= amdgpu_ring_test_helper(ring);
>   	}
>   
> -	return 0;
> +	return r;
>   }
>   
>   static void gfx_v7_0_cp_enable(struct amdgpu_device *adev, bool enable)

Gfx8 and 9 (did not look further) do not do it like that. Should they? 
Or there is more work there to be done first?

I actually might like this because maybe it gets us closer to removing 
the ring->sched.ready hack but what I am just not sure if the idea was 
to allow driver to function with some non-functional rings after resume. 
Under the premise that if they initialized during init, then after 
resume they must too, or if they don't, it is a transient glitch. I 
don't know.. I am being imaginative here thinking about silly driver 
workarounds for weird hardware glitches. It is much more likely this was 
just an oversight and it is completely fine to to error out.

I have to defer to Alex and Christian on this one.

Regards,

Tvrtko

Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Alex Deucher]

On Tue, Jul 14, 2026 at 4:14 PM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>
>
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > The gfx_v7_0_cp_compute_resume() function should only return
> > success when all compute rings are actually functional.
> > This will be especially important for soft reset which needs
> > this to know whether the reset was successful.
> >
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++--
> >   1 file changed, 4 insertions(+), 2 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > index 9c4b3ac27e1f..a1a9f3fc4567 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > @@ -3019,12 +3019,14 @@ static int gfx_v7_0_cp_compute_resume(struct amdgpu_device *adev)
> >
> >       gfx_v7_0_cp_compute_enable(adev, true);
> >
> > +     r = 0;
> > +
> >       for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> >               ring = &adev->gfx.compute_ring[i];
> > -             amdgpu_ring_test_helper(ring);
> > +             r |= amdgpu_ring_test_helper(ring);
> >       }
> >
> > -     return 0;
> > +     return r;
> >   }
> >
> >   static void gfx_v7_0_cp_enable(struct amdgpu_device *adev, bool enable)
>
> Gfx8 and 9 (did not look further) do not do it like that. Should they?
> Or there is more work there to be done first?
>
> I actually might like this because maybe it gets us closer to removing
> the ring->sched.ready hack but what I am just not sure if the idea was
> to allow driver to function with some non-functional rings after resume.
> Under the premise that if they initialized during init, then after
> resume they must too, or if they don't, it is a transient glitch. I
> don't know.. I am being imaginative here thinking about silly driver
> workarounds for weird hardware glitches. It is much more likely this was
> just an oversight and it is completely fine to to error out.
>
> I have to defer to Alex and Christian on this one.

The reason for not checking the errors was because compute queue
failure was not seen as fatal.  There are a lot of compute queues
(relative to other engines), so if something happened, it seemed
better to just continue in a degraded mode with fewer compute queues
than to fail to resume in general.

Alex

>
> Regards,
>
> Tvrtko
>

Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Timur Kristóf]

On 2026. július 14., kedd 22:41:33 közép-európai nyári idő Alex Deucher wrote:
> On Tue, Jul 14, 2026 at 4:14 PM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
> > On 13/07/2026 13:58, Timur Kristóf wrote:
> > > The gfx_v7_0_cp_compute_resume() function should only return
> > > success when all compute rings are actually functional.
> > > This will be especially important for soft reset which needs
> > > this to know whether the reset was successful.
> > > 
> > > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > > ---
> > > 
> > >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 6 ++++--
> > >   1 file changed, 4 insertions(+), 2 deletions(-)
> > > 
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index
> > > 9c4b3ac27e1f..a1a9f3fc4567 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > > @@ -3019,12 +3019,14 @@ static int gfx_v7_0_cp_compute_resume(struct
> > > amdgpu_device *adev)> > 
> > >       gfx_v7_0_cp_compute_enable(adev, true);
> > > 
> > > +     r = 0;
> > > +
> > > 
> > >       for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > >       
> > >               ring = &adev->gfx.compute_ring[i];
> > > 
> > > -             amdgpu_ring_test_helper(ring);
> > > +             r |= amdgpu_ring_test_helper(ring);
> > > 
> > >       }
> > > 
> > > -     return 0;
> > > +     return r;
> > > 
> > >   }
> > >   
> > >   static void gfx_v7_0_cp_enable(struct amdgpu_device *adev, bool
> > >   enable)
> > 
> > Gfx8 and 9 (did not look further) do not do it like that. Should they?
> > Or there is more work there to be done first?

I actually added the same code for GFX8 in the previous series.
In gfx_v8_0_cp_test_all_rings() it checks that all rings are functional, and 
it calls that from gfx_v8_0_cp_resume().

I think it would be a good idea to do this for newer GPUs as well, but I 
haven't yet touched the code for those.

> > 
> > I actually might like this because maybe it gets us closer to removing
> > the ring->sched.ready hack but what I am just not sure if the idea was
> > to allow driver to function with some non-functional rings after resume.
> > Under the premise that if they initialized during init, then after
> > resume they must too, or if they don't, it is a transient glitch. I
> > don't know.. I am being imaginative here thinking about silly driver
> > workarounds for weird hardware glitches. It is much more likely this was
> > just an oversight and it is completely fine to to error out.
> > 
> > I have to defer to Alex and Christian on this one.
> 
> The reason for not checking the errors was because compute queue
> failure was not seen as fatal.  There are a lot of compute queues
> (relative to other engines), so if something happened, it seemed
> better to just continue in a degraded mode with fewer compute queues
> than to fail to resume in general.

We had a conversation about a similar topic (it was about UVD), where Alex 
said that in general we should prefer not to handle degraded functionality in 
amdgpu. I think the same principle should apply here.

1. My main problem with handling degraded functionality here is that I have 
never seen any issue where just some compute queues fail to initialize after 
boot or after suspend/resume. That means we can't meaningfully test that 
scenario, so we can't trust any code we write to handle that either.
2. It would be very tedious to keep track of which queues didn't work in the 
first place vs. which are those that don't work because of a bug in the soft 
reset code. I consider the soft reset as failed if not all queues work 
correctly.

Considering the above, I vote that we should just expect all queues to work 
correctly at initialization and after a recovery. What do you guys think?

Thanks & best regards,
Timur

Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Christian König]

On 7/15/26 10:02, Timur Kristóf wrote:
> On 2026. július 14., kedd 22:41:33 közép-európai nyári idő Alex Deucher wrote:
>> On Tue, Jul 14, 2026 at 4:14 PM Tvrtko Ursulin <tursulin@ursulin.net> wrote:
>>> On 13/07/2026 13:58, Timur Kristóf wrote:
...
>>>
>>> I actually might like this because maybe it gets us closer to removing
>>> the ring->sched.ready hack but what I am just not sure if the idea was
>>> to allow driver to function with some non-functional rings after resume.
>>> Under the premise that if they initialized during init, then after
>>> resume they must too, or if they don't, it is a transient glitch. I
>>> don't know.. I am being imaginative here thinking about silly driver
>>> workarounds for weird hardware glitches. It is much more likely this was
>>> just an oversight and it is completely fine to to error out.
>>>
>>> I have to defer to Alex and Christian on this one.
>>
>> The reason for not checking the errors was because compute queue
>> failure was not seen as fatal.  There are a lot of compute queues
>> (relative to other engines), so if something happened, it seemed
>> better to just continue in a degraded mode with fewer compute queues
>> than to fail to resume in general.
> 
> We had a conversation about a similar topic (it was about UVD), where Alex 
> said that in general we should prefer not to handle degraded functionality in 
> amdgpu. I think the same principle should apply here.

Yeah, well that's a rather problematic topic.

Ignore such errors leads to never fix them. But on the other hand if we handle that as fatal and UVD doesn't comes up after resume you end up with a black screen while otherwise UVD is just not working.

> 1. My main problem with handling degraded functionality here is that I have 
> never seen any issue where just some compute queues fail to initialize after 
> boot or after suspend/resume. That means we can't meaningfully test that 
> scenario, so we can't trust any code we write to handle that either.

We had a bunch of cases where some engine didn't worked in the past (UVD, VCE, Compute) either on initial load or after suspend/resume.

Especially there was a Compute issues that was really annoying because it only caused trouble after Mesa switched to using compute engines for VA-API. Before that we didn't even know that Compute rings sometimes don't properly come up again after a resume.

> 2. It would be very tedious to keep track of which queues didn't work in the 
> first place vs. which are those that don't work because of a bug in the soft 
> reset code. I consider the soft reset as failed if not all queues work 
> correctly.
> 
> Considering the above, I vote that we should just expect all queues to work 
> correctly at initialization and after a recovery. What do you guys think?

Well it's basically choosing what is the lesser evil.

Regards,
Christian.

> 
> Thanks & best regards,
> Timur
> 
> 
> 
> 
> 

Re: [PATCH 3/9] drm/amdgpu/gfx7: Return error code when compute ring tests fail

[Timur Kristóf]

On 2026. július 15., szerda 11:50:30 közép-európai nyári idő Christian König 
wrote:
> On 7/15/26 10:02, Timur Kristóf wrote:
> > On 2026. július 14., kedd 22:41:33 közép-európai nyári idő Alex Deucher 
wrote:
> >> On Tue, Jul 14, 2026 at 4:14 PM Tvrtko Ursulin <tursulin@ursulin.net> 
wrote:
> >>> On 13/07/2026 13:58, Timur Kristóf wrote:
> ...
> 
> >>> I actually might like this because maybe it gets us closer to removing
> >>> the ring->sched.ready hack but what I am just not sure if the idea was
> >>> to allow driver to function with some non-functional rings after resume.
> >>> Under the premise that if they initialized during init, then after
> >>> resume they must too, or if they don't, it is a transient glitch. I
> >>> don't know.. I am being imaginative here thinking about silly driver
> >>> workarounds for weird hardware glitches. It is much more likely this was
> >>> just an oversight and it is completely fine to to error out.
> >>> 
> >>> I have to defer to Alex and Christian on this one.
> >> 
> >> The reason for not checking the errors was because compute queue
> >> failure was not seen as fatal.  There are a lot of compute queues
> >> (relative to other engines), so if something happened, it seemed
> >> better to just continue in a degraded mode with fewer compute queues
> >> than to fail to resume in general.
> > 
> > We had a conversation about a similar topic (it was about UVD), where Alex
> > said that in general we should prefer not to handle degraded functionality
> > in amdgpu. I think the same principle should apply here.
> 
> Yeah, well that's a rather problematic topic.
> 
> Ignore such errors leads to never fix them. But on the other hand if we
> handle that as fatal and UVD doesn't comes up after resume you end up with
> a black screen while otherwise UVD is just not working.

Yeah, that was exactly my point when we talked about it.

However, Alex convinced me that we shouldn't bother supporting these degraded 
use cases. After spending more time working on amdgpu, I think it would raise 
more problems than it solves if we allowed that.

> > 1. My main problem with handling degraded functionality here is that I
> > have
> > never seen any issue where just some compute queues fail to initialize
> > after boot or after suspend/resume. That means we can't meaningfully test
> > that scenario, so we can't trust any code we write to handle that either.
> We had a bunch of cases where some engine didn't worked in the past (UVD,
> VCE, Compute) either on initial load or after suspend/resume.
> 
> Especially there was a Compute issues that was really annoying because it
> only caused trouble after Mesa switched to using compute engines for
> VA-API. Before that we didn't even know that Compute rings sometimes don't
> properly come up again after a resume.

Fortunately now we have ring tests and ring IB tests which we execute on 
initialization to make sure the rings work.

> > 2. It would be very tedious to keep track of which queues didn't work in
> > the first place vs. which are those that don't work because of a bug in
> > the soft reset code. I consider the soft reset as failed if not all
> > queues work correctly.
> > 
> > Considering the above, I vote that we should just expect all queues to
> > work
> > correctly at initialization and after a recovery. What do you guys think?
> 
> Well it's basically choosing what is the lesser evil.

Agreed.

[PATCH 4/9] drm/amdgpu/gfx7: Return error code when failing to start GFX ring

[Timur Kristóf]

Return an error code instead of silently failing.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index a1a9f3fc4567..0ceadb107d26 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -2576,7 +2576,10 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
 	WREG32(mmCP_RB0_BASE_HI, upper_32_bits(rb_addr));
 
 	/* start the ring */
-	gfx_v7_0_cp_gfx_start(adev);
+	r = gfx_v7_0_cp_gfx_start(adev);
+	if (r)
+		return r;
+
 	r = amdgpu_ring_test_helper(ring);
 	if (r)
 		return r;
-- 
2.55.0

Re: [PATCH 4/9] drm/amdgpu/gfx7: Return error code when failing to start GFX ring

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> Return an error code instead of silently failing.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 5 ++++-
>   1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index a1a9f3fc4567..0ceadb107d26 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -2576,7 +2576,10 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
>   	WREG32(mmCP_RB0_BASE_HI, upper_32_bits(rb_addr));
>   
>   	/* start the ring */
> -	gfx_v7_0_cp_gfx_start(adev);
> +	r = gfx_v7_0_cp_gfx_start(adev);
> +	if (r)
> +		return r;
> +
>   	r = amdgpu_ring_test_helper(ring);
>   	if (r)
>   		return r;

Hm this one looks less "controversial" than the previous one since it 
already can error out. And surely test ring helper would error out if 
cp_gfx_start failed..

I think it also means the previous one is likely fine.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

[PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets

[Timur Kristóf]

Implement the emit_switch_buffer() function instead of emitting
them duing emit_ib, emit_pipeline_sync and emit_vm_flush.

Note that it isn't necessary to emit these in both
emit_pipeline_sync() and emit_vm_flush() because
amdgpu_vm_flush() already calls these when calling
either of those functions.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 32 +++++++++------------------
 1 file changed, 10 insertions(+), 22 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 0ceadb107d26..a93cc02c3400 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -2201,12 +2201,6 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
 	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
 	u32 header, control = 0;
 
-	/* insert SWITCH_BUFFER packet before first IB in the ring frame */
-	if (flags & AMDGPU_HAVE_CTX_SWITCH) {
-		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
-		amdgpu_ring_write(ring, 0);
-	}
-
 	if (ib->flags & AMDGPU_IB_FLAG_CE)
 		header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
 	else
@@ -2258,6 +2252,12 @@ static void gfx_v7_0_ring_emit_ib_compute(struct amdgpu_ring *ring,
 	amdgpu_ring_write(ring, control);
 }
 
+static void gfx_v7_0_ring_emit_sb(struct amdgpu_ring *ring)
+{
+	amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
+	amdgpu_ring_write(ring, 0);
+}
+
 static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
 {
 	uint32_t dw2 = 0;
@@ -3111,14 +3111,6 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct amdgpu_ring *ring)
 	amdgpu_ring_write(ring, seq);
 	amdgpu_ring_write(ring, 0xffffffff);
 	amdgpu_ring_write(ring, 4); /* poll interval */
-
-	if (usepfp) {
-		/* sync CE with ME to prevent CE fetch CEIB before context switch done */
-		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
-		amdgpu_ring_write(ring, 0);
-		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
-		amdgpu_ring_write(ring, 0);
-	}
 }
 
 /*
@@ -3160,12 +3152,6 @@ static void gfx_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
 		/* sync PFP to ME, otherwise we might get invalid PFP reads */
 		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME, 0));
 		amdgpu_ring_write(ring, 0x0);
-
-		/* synce CE with ME to prevent CE fetch CEIB before context switch done */
-		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
-		amdgpu_ring_write(ring, 0);
-		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
-		amdgpu_ring_write(ring, 0);
 	}
 }
 
@@ -4954,8 +4940,9 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
 		7 + /* gfx_v7_0_ring_emit_hdp_flush */
 		5 + /* hdp invalidate */
 		12 + 12 + 12 + /* gfx_v7_0_ring_emit_fence_gfx x3 for user fence, vm fence */
-		7 + 4 + /* gfx_v7_0_ring_emit_pipeline_sync */
-		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 6 + /* gfx_v7_0_ring_emit_vm_flush */
+		7 + /* gfx_v7_0_ring_emit_pipeline_sync */
+		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 2 + /* gfx_v7_0_ring_emit_vm_flush */
+		3 * 2 + /* gfx_v7_0_ring_emit_sb x3 (from amdgpu_vm_flush, amdgpu_ib_schedule) */
 		3 + 4 + /* gfx_v7_ring_emit_cntxcntl including vgt flush*/
 		5, /* SURFACE_SYNC */
 	.emit_ib_size = 4, /* gfx_v7_0_ring_emit_ib_gfx */
@@ -4969,6 +4956,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
 	.test_ib = gfx_v7_0_ring_test_ib,
 	.insert_nop = amdgpu_ring_insert_nop,
 	.pad_ib = amdgpu_ring_generic_pad_ib,
+	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
 	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
 	.emit_wreg = gfx_v7_0_ring_emit_wreg,
 	.soft_recovery = gfx_v7_0_ring_soft_recovery,
-- 
2.55.0

Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> Implement the emit_switch_buffer() function instead of emitting
> them duing emit_ib, emit_pipeline_sync and emit_vm_flush.

during

> 
> Note that it isn't necessary to emit these in both
> emit_pipeline_sync() and emit_vm_flush() because
> amdgpu_vm_flush() already calls these when calling
> either of those functions.

The amdgpu_vm_flush indeed does emit two switch buffers:

	/* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC */
	if (ring->funcs->emit_switch_buffer) {
		amdgpu_ring_emit_switch_buffer(ring);
		amdgpu_ring_emit_switch_buffer(ring);
	}

Comments are different though:

/* sync CE with ME to prevent CE fetch CEIB before context switch done */

Are you confident the two emissions are about the same thing?

> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 32 +++++++++------------------
>   1 file changed, 10 insertions(+), 22 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 0ceadb107d26..a93cc02c3400 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -2201,12 +2201,6 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct amdgpu_ring *ring,
>   	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>   	u32 header, control = 0;
>   
> -	/* insert SWITCH_BUFFER packet before first IB in the ring frame */
> -	if (flags & AMDGPU_HAVE_CTX_SWITCH) {
> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> -		amdgpu_ring_write(ring, 0);
> -	}

Commit message does not explain why the change of ring buffer command 
this creates is okay. Current flow is:

amdgpu_ib_schedule()
{
...
   amdgpu_ring_emit_ib
     amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));


New flow is:

...
   amdgpu_ring_emit_ib
... other ring commands ...
   amdgpu_ring_emit_switch_buffer
     amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));

Is this okay? Specifically due the above comment saying "insert 
SWITCH_BUFFER packet before first IB in the ring frame" - is the "first" 
part not important?

Also, amdgpu_ib_schedule only emits amdgpu_ring_emit_switch_buffer if 
there is a job. Currently it is always emitted.

Final interesting part is how amdgpu_ib_schedule clears 
AMDGPU_HAVE_CTX_SWITCH after having called amdgpu_ring_emit_ib.

After this change only gfx6 remains the user of that flag in 
gfx_v6_0_ring_emit_ib. Everyone else only use it in emit_cntxcntl. If 
gfx6 was adjusted too (later), amdgpu_ib_schedule could reduce the scope 
of that flag to just the scope where it calls amdgpu_ring_emit_frame_cntl.

Regards,

Tvrtko
> -
>   	if (ib->flags & AMDGPU_IB_FLAG_CE)
>   		header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
>   	else
> @@ -2258,6 +2252,12 @@ static void gfx_v7_0_ring_emit_ib_compute(struct amdgpu_ring *ring,
>   	amdgpu_ring_write(ring, control);
>   }
>   
> +static void gfx_v7_0_ring_emit_sb(struct amdgpu_ring *ring)
> +{
> +	amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> +	amdgpu_ring_write(ring, 0);
> +}
> +
>   static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t flags)
>   {
>   	uint32_t dw2 = 0;
> @@ -3111,14 +3111,6 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct amdgpu_ring *ring)
>   	amdgpu_ring_write(ring, seq);
>   	amdgpu_ring_write(ring, 0xffffffff);
>   	amdgpu_ring_write(ring, 4); /* poll interval */
> -
> -	if (usepfp) {
> -		/* sync CE with ME to prevent CE fetch CEIB before context switch done */
> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> -		amdgpu_ring_write(ring, 0);
> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> -		amdgpu_ring_write(ring, 0);
> -	}
>   }
>   
>   /*
> @@ -3160,12 +3152,6 @@ static void gfx_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   		/* sync PFP to ME, otherwise we might get invalid PFP reads */
>   		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME, 0));
>   		amdgpu_ring_write(ring, 0x0);
> -
> -		/* synce CE with ME to prevent CE fetch CEIB before context switch done */
> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> -		amdgpu_ring_write(ring, 0);
> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> -		amdgpu_ring_write(ring, 0);
>   	}
>   }
>   
> @@ -4954,8 +4940,9 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
>   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
>   		5 + /* hdp invalidate */
>   		12 + 12 + 12 + /* gfx_v7_0_ring_emit_fence_gfx x3 for user fence, vm fence */
> -		7 + 4 + /* gfx_v7_0_ring_emit_pipeline_sync */
> -		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 6 + /* gfx_v7_0_ring_emit_vm_flush */
> +		7 + /* gfx_v7_0_ring_emit_pipeline_sync */
> +		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 2 + /* gfx_v7_0_ring_emit_vm_flush */
> +		3 * 2 + /* gfx_v7_0_ring_emit_sb x3 (from amdgpu_vm_flush, amdgpu_ib_schedule) */
>   		3 + 4 + /* gfx_v7_ring_emit_cntxcntl including vgt flush*/
>   		5, /* SURFACE_SYNC */
>   	.emit_ib_size = 4, /* gfx_v7_0_ring_emit_ib_gfx */
> @@ -4969,6 +4956,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
>   	.test_ib = gfx_v7_0_ring_test_ib,
>   	.insert_nop = amdgpu_ring_insert_nop,
>   	.pad_ib = amdgpu_ring_generic_pad_ib,
> +	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
>   	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
>   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
>   	.soft_recovery = gfx_v7_0_ring_soft_recovery,

Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets

[Timur Kristóf]

On 2026. július 15., szerda 10:56:38 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > Implement the emit_switch_buffer() function instead of emitting
> > them duing emit_ib, emit_pipeline_sync and emit_vm_flush.
> 
> during
> 
> > Note that it isn't necessary to emit these in both
> > emit_pipeline_sync() and emit_vm_flush() because
> > amdgpu_vm_flush() already calls these when calling
> > either of those functions.
> 
> The amdgpu_vm_flush indeed does emit two switch buffers:
> 
> 	/* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC 
*/
> 	if (ring->funcs->emit_switch_buffer) {
> 		amdgpu_ring_emit_switch_buffer(ring);
> 		amdgpu_ring_emit_switch_buffer(ring);
> 	}
> 
> Comments are different though:
> 
> /* sync CE with ME to prevent CE fetch CEIB before context switch done */
> 
> Are you confident the two emissions are about the same thing?

Yes, I'm confident. One of the comments explains why the SWITCH_BUFFER packet 
is emitted, the other one explains why it is emitted outside COND_EXEC.

This packet is interpreted by the CE (constant engine). The reason why this 
packet is emitted is basically to make sure the CE can't start executing 
packets from the next submission until the current one is finished.

(Note that CE is not utilized by any maintained userspace driver and is 
discontinued in new GPUs. As far as I remember there were experiments to try 
to use the CE in Mesa but it didn't yield any noteworthy perf improvement so 
we just never used it. The old proprietary driver may have used it. It is now 
also deprecated in the kernel.)


> 
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> > 
> >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 32 +++++++++------------------
> >   1 file changed, 10 insertions(+), 22 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 0ceadb107d26..a93cc02c3400
> > 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > @@ -2201,12 +2201,6 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct
> > amdgpu_ring *ring,> 
> >   	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
> >   	u32 header, control = 0;
> > 
> > -	/* insert SWITCH_BUFFER packet before first IB in the ring frame */
> > -	if (flags & AMDGPU_HAVE_CTX_SWITCH) {
> > -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
0));
> > -		amdgpu_ring_write(ring, 0);
> > -	}
> 
> Commit message does not explain why the change of ring buffer command
> this creates is okay. Current flow is:
> 
> amdgpu_ib_schedule()
> {
> ...
>    amdgpu_ring_emit_ib
>      amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> 
> 
> New flow is:
> 
> ...
>    amdgpu_ring_emit_ib
> ... other ring commands ...
>    amdgpu_ring_emit_switch_buffer
>      amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));

No, that's not what the new flow is. If you check the callers of 
emit_switch_buffer() you can see that it's called from two places:

- amdgpu_vm_flush() emits it before the first IB when necessary
- amdgpu_ib_schedule() emits it after the last IB when necessary

> Is this okay? Specifically due the above comment saying "insert
> SWITCH_BUFFER packet before first IB in the ring frame" - is the "first"
> part not important?

amdgpu_vm_flush() emits it before the first IB.

> Also, amdgpu_ib_schedule only emits amdgpu_ring_emit_switch_buffer if
> there is a job. Currently it is always emitted.

I trust that the GFX8+ implementations are more precise and that it's sufficient 
to emit this packet in the cases where the emit_switch_buffer() function is 
called.

When there is "no job" that's a special case that is only used during 
initialization (specifically the IB ring tests). In that case we are not 
executing commands submitted by userspace but rather commands generated by the 
kernel. So we can be sure the CE is not used in those cases.

> 
> Final interesting part is how amdgpu_ib_schedule clears
> AMDGPU_HAVE_CTX_SWITCH after having called amdgpu_ring_emit_ib.
> 
> After this change only gfx6 remains the user of that flag in
> gfx_v6_0_ring_emit_ib. Everyone else only use it in emit_cntxcntl. If
> gfx6 was adjusted too (later), amdgpu_ib_schedule could reduce the scope
> of that flag to just the scope where it calls amdgpu_ring_emit_frame_cntl.

I also adjusted the same thing for GFX6 in the next series.
Can clean up the flag later once both series are accepted.


> 
> > -
> > 
> >   	if (ib->flags & AMDGPU_IB_FLAG_CE)
> >   	
> >   		header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
> >   	
> >   	else
> > 
> > @@ -2258,6 +2252,12 @@ static void gfx_v7_0_ring_emit_ib_compute(struct
> > amdgpu_ring *ring,> 
> >   	amdgpu_ring_write(ring, control);
> >   
> >   }
> > 
> > +static void gfx_v7_0_ring_emit_sb(struct amdgpu_ring *ring)
> > +{
> > +	amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> > +	amdgpu_ring_write(ring, 0);
> > +}
> > +
> > 
> >   static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t
> >   flags) {
> >   
> >   	uint32_t dw2 = 0;
> > 
> > @@ -3111,14 +3111,6 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct
> > amdgpu_ring *ring)> 
> >   	amdgpu_ring_write(ring, seq);
> >   	amdgpu_ring_write(ring, 0xffffffff);
> >   	amdgpu_ring_write(ring, 4); /* poll interval */
> > 
> > -
> > -	if (usepfp) {
> > -		/* sync CE with ME to prevent CE fetch CEIB before 
context switch done
> > */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
0));
> > -		amdgpu_ring_write(ring, 0);
> > -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
0));
> > -		amdgpu_ring_write(ring, 0);
> > -	}
> > 
> >   }
> >   
> >   /*
> > 
> > @@ -3160,12 +3152,6 @@ static void gfx_v7_0_ring_emit_vm_flush(struct
> > amdgpu_ring *ring,> 
> >   		/* sync PFP to ME, otherwise we might get invalid PFP 
reads */
> >   		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME, 
0));
> >   		amdgpu_ring_write(ring, 0x0);
> > 
> > -
> > -		/* synce CE with ME to prevent CE fetch CEIB before 
context switch done
> > */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
0));
> > -		amdgpu_ring_write(ring, 0);
> > -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
0));
> > -		amdgpu_ring_write(ring, 0);
> > 
> >   	}
> >   
> >   }
> > 
> > @@ -4954,8 +4940,9 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_gfx = {> 
> >   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
> >   		5 + /* hdp invalidate */
> >   		12 + 12 + 12 + /* gfx_v7_0_ring_emit_fence_gfx x3 for 
user fence, vm
> >   		fence */> 
> > -		7 + 4 + /* gfx_v7_0_ring_emit_pipeline_sync */
> > -		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 6 + /* 
gfx_v7_0_ring_emit_vm_flush
> > */ +		7 + /* gfx_v7_0_ring_emit_pipeline_sync */
> > +		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 2 + /* 
gfx_v7_0_ring_emit_vm_flush
> > */ +		3 * 2 + /* gfx_v7_0_ring_emit_sb x3 (from 
amdgpu_vm_flush,
> > amdgpu_ib_schedule) */> 
> >   		3 + 4 + /* gfx_v7_ring_emit_cntxcntl including vgt 
flush*/
> >   		5, /* SURFACE_SYNC */
> >   	
> >   	.emit_ib_size = 4, /* gfx_v7_0_ring_emit_ib_gfx */
> > 
> > @@ -4969,6 +4956,7 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_gfx = {> 
> >   	.test_ib = gfx_v7_0_ring_test_ib,
> >   	.insert_nop = amdgpu_ring_insert_nop,
> >   	.pad_ib = amdgpu_ring_generic_pad_ib,
> > 
> > +	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
> > 
> >   	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
> >   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
> >   	.soft_recovery = gfx_v7_0_ring_soft_recovery,

Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets

[Tvrtko Ursulin]

On 15/07/2026 11:36, Timur Kristóf wrote:
> On 2026. július 15., szerda 10:56:38 közép-európai nyári idő Tvrtko Ursulin
> wrote:
>> On 13/07/2026 13:58, Timur Kristóf wrote:
>>> Implement the emit_switch_buffer() function instead of emitting
>>> them duing emit_ib, emit_pipeline_sync and emit_vm_flush.
>>
>> during
>>
>>> Note that it isn't necessary to emit these in both
>>> emit_pipeline_sync() and emit_vm_flush() because
>>> amdgpu_vm_flush() already calls these when calling
>>> either of those functions.
>>
>> The amdgpu_vm_flush indeed does emit two switch buffers:
>>
>> 	/* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC
> */
>> 	if (ring->funcs->emit_switch_buffer) {
>> 		amdgpu_ring_emit_switch_buffer(ring);
>> 		amdgpu_ring_emit_switch_buffer(ring);
>> 	}
>>
>> Comments are different though:
>>
>> /* sync CE with ME to prevent CE fetch CEIB before context switch done */
>>
>> Are you confident the two emissions are about the same thing?
> 
> Yes, I'm confident. One of the comments explains why the SWITCH_BUFFER packet
> is emitted, the other one explains why it is emitted outside COND_EXEC.
> 
> This packet is interpreted by the CE (constant engine). The reason why this
> packet is emitted is basically to make sure the CE can't start executing
> packets from the next submission until the current one is finished.
> 
> (Note that CE is not utilized by any maintained userspace driver and is
> discontinued in new GPUs. As far as I remember there were experiments to try
> to use the CE in Mesa but it didn't yield any noteworthy perf improvement so
> we just never used it. The old proprietary driver may have used it. It is now
> also deprecated in the kernel.)

Sounds believable to me.

>>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>>> ---
>>>
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 32 +++++++++------------------
>>>    1 file changed, 10 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 0ceadb107d26..a93cc02c3400
>>> 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -2201,12 +2201,6 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct
>>> amdgpu_ring *ring,>
>>>    	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>    	u32 header, control = 0;
>>>
>>> -	/* insert SWITCH_BUFFER packet before first IB in the ring frame */
>>> -	if (flags & AMDGPU_HAVE_CTX_SWITCH) {
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER,
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -	}
>>
>> Commit message does not explain why the change of ring buffer command
>> this creates is okay. Current flow is:
>>
>> amdgpu_ib_schedule()
>> {
>> ...
>>     amdgpu_ring_emit_ib
>>       amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>
>>
>> New flow is:
>>
>> ...
>>     amdgpu_ring_emit_ib
>> ... other ring commands ...
>>     amdgpu_ring_emit_switch_buffer
>>       amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> 
> No, that's not what the new flow is. If you check the callers of
> emit_switch_buffer() you can see that it's called from two places:
> 
> - amdgpu_vm_flush() emits it before the first IB when necessary
> - amdgpu_ib_schedule() emits it after the last IB when necessary
> 
>> Is this okay? Specifically due the above comment saying "insert
>> SWITCH_BUFFER packet before first IB in the ring frame" - is the "first"
>> part not important?
> 
> amdgpu_vm_flush() emits it before the first IB.

True, I wrote the above before I got to the vm_flush etc and forgot to 
revisit my earlier thinking.

>> Also, amdgpu_ib_schedule only emits amdgpu_ring_emit_switch_buffer if
>> there is a job. Currently it is always emitted.
> 
> I trust that the GFX8+ implementations are more precise and that it's sufficient
> to emit this packet in the cases where the emit_switch_buffer() function is
> called.

I am not sure about gfx7 vs gfx8 because in my mind it's always possible 
different hw generations may need subtly different handling. I will have 
to defer to AMD experts here.

> When there is "no job" that's a special case that is only used during
> initialization (specifically the IB ring tests). In that case we are not
> executing commands submitted by userspace but rather commands generated by the
> kernel. So we can be sure the CE is not used in those cases.

Sounds believable. I thought for a while whether a kernel submission can 
interleave with userspace, like perhaps ring test post resume, but in 
that case I guess context switch isn't needed since GPU has been 
sufficiently re-initialized?

>> Final interesting part is how amdgpu_ib_schedule clears
>> AMDGPU_HAVE_CTX_SWITCH after having called amdgpu_ring_emit_ib.
>>
>> After this change only gfx6 remains the user of that flag in
>> gfx_v6_0_ring_emit_ib. Everyone else only use it in emit_cntxcntl. If
>> gfx6 was adjusted too (later), amdgpu_ib_schedule could reduce the scope
>> of that flag to just the scope where it calls amdgpu_ring_emit_frame_cntl.
> 
> I also adjusted the same thing for GFX6 in the next series.
> Can clean up the flag later once both series are accepted.

Yep.

Regards,

Tvrtko

> 
> 
>>
>>> -
>>>
>>>    	if (ib->flags & AMDGPU_IB_FLAG_CE)
>>>    	
>>>    		header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
>>>    	
>>>    	else
>>>
>>> @@ -2258,6 +2252,12 @@ static void gfx_v7_0_ring_emit_ib_compute(struct
>>> amdgpu_ring *ring,>
>>>    	amdgpu_ring_write(ring, control);
>>>    
>>>    }
>>>
>>> +static void gfx_v7_0_ring_emit_sb(struct amdgpu_ring *ring)
>>> +{
>>> +	amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> +	amdgpu_ring_write(ring, 0);
>>> +}
>>> +
>>>
>>>    static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t
>>>    flags) {
>>>    
>>>    	uint32_t dw2 = 0;
>>>
>>> @@ -3111,14 +3111,6 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct
>>> amdgpu_ring *ring)>
>>>    	amdgpu_ring_write(ring, seq);
>>>    	amdgpu_ring_write(ring, 0xffffffff);
>>>    	amdgpu_ring_write(ring, 4); /* poll interval */
>>>
>>> -
>>> -	if (usepfp) {
>>> -		/* sync CE with ME to prevent CE fetch CEIB before
> context switch done
>>> */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER,
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER,
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -	}
>>>
>>>    }
>>>    
>>>    /*
>>>
>>> @@ -3160,12 +3152,6 @@ static void gfx_v7_0_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,>
>>>    		/* sync PFP to ME, otherwise we might get invalid PFP
> reads */
>>>    		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME,
> 0));
>>>    		amdgpu_ring_write(ring, 0x0);
>>>
>>> -
>>> -		/* synce CE with ME to prevent CE fetch CEIB before
> context switch done
>>> */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER,
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER,
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>>
>>>    	}
>>>    
>>>    }
>>>
>>> @@ -4954,8 +4940,9 @@ static const struct amdgpu_ring_funcs
>>> gfx_v7_0_ring_funcs_gfx = {>
>>>    		7 + /* gfx_v7_0_ring_emit_hdp_flush */
>>>    		5 + /* hdp invalidate */
>>>    		12 + 12 + 12 + /* gfx_v7_0_ring_emit_fence_gfx x3 for
> user fence, vm
>>>    		fence */>
>>> -		7 + 4 + /* gfx_v7_0_ring_emit_pipeline_sync */
>>> -		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 6 + /*
> gfx_v7_0_ring_emit_vm_flush
>>> */ +		7 + /* gfx_v7_0_ring_emit_pipeline_sync */
>>> +		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 2 + /*
> gfx_v7_0_ring_emit_vm_flush
>>> */ +		3 * 2 + /* gfx_v7_0_ring_emit_sb x3 (from
> amdgpu_vm_flush,
>>> amdgpu_ib_schedule) */>
>>>    		3 + 4 + /* gfx_v7_ring_emit_cntxcntl including vgt
> flush*/
>>>    		5, /* SURFACE_SYNC */
>>>    	
>>>    	.emit_ib_size = 4, /* gfx_v7_0_ring_emit_ib_gfx */
>>>
>>> @@ -4969,6 +4956,7 @@ static const struct amdgpu_ring_funcs
>>> gfx_v7_0_ring_funcs_gfx = {>
>>>    	.test_ib = gfx_v7_0_ring_test_ib,
>>>    	.insert_nop = amdgpu_ring_insert_nop,
>>>    	.pad_ib = amdgpu_ring_generic_pad_ib,
>>>
>>> +	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
>>>
>>>    	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
>>>    	.emit_wreg = gfx_v7_0_ring_emit_wreg,
>>>    	.soft_recovery = gfx_v7_0_ring_soft_recovery,
> 
> 
> 
> 

Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets

[Christian König]

On 7/15/26 12:36, Timur Kristóf wrote:
> On 2026. július 15., szerda 10:56:38 közép-európai nyári idő Tvrtko Ursulin 
> wrote:
>> On 13/07/2026 13:58, Timur Kristóf wrote:
>>> Implement the emit_switch_buffer() function instead of emitting
>>> them duing emit_ib, emit_pipeline_sync and emit_vm_flush.
>>
>> during
>>
>>> Note that it isn't necessary to emit these in both
>>> emit_pipeline_sync() and emit_vm_flush() because
>>> amdgpu_vm_flush() already calls these when calling
>>> either of those functions.
>>
>> The amdgpu_vm_flush indeed does emit two switch buffers:
>>
>> 	/* the double SWITCH_BUFFER here *cannot* be skipped by COND_EXEC 
> */
>> 	if (ring->funcs->emit_switch_buffer) {
>> 		amdgpu_ring_emit_switch_buffer(ring);
>> 		amdgpu_ring_emit_switch_buffer(ring);
>> 	}
>>
>> Comments are different though:
>>
>> /* sync CE with ME to prevent CE fetch CEIB before context switch done */
>>
>> Are you confident the two emissions are about the same thing?
> 
> Yes, I'm confident. One of the comments explains why the SWITCH_BUFFER packet 
> is emitted, the other one explains why it is emitted outside COND_EXEC.
> 
> This packet is interpreted by the CE (constant engine). The reason why this 
> packet is emitted is basically to make sure the CE can't start executing 
> packets from the next submission until the current one is finished.
> 
> (Note that CE is not utilized by any maintained userspace driver and is 
> discontinued in new GPUs. As far as I remember there were experiments to try 
> to use the CE in Mesa but it didn't yield any noteworthy perf improvement so 
> we just never used it. The old proprietary driver may have used it. It is now 
> also deprecated in the kernel.)

Absolutely correct, yes.

We could eliminate the whole switch buffer packet if we completely deprecate any CE engine support.

There was just exactly one Mesa release which made use of the CE engine but I've disable the CE support and put it behind a debug option quite a while ago.

If nobody complains in the near term I think we can make nails with heads and just remove all the hacks we have for this.

Regards,
Christian.


> 
> 
>>
>>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>>> ---
>>>
>>>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 32 +++++++++------------------
>>>   1 file changed, 10 insertions(+), 22 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 0ceadb107d26..a93cc02c3400
>>> 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -2201,12 +2201,6 @@ static void gfx_v7_0_ring_emit_ib_gfx(struct
>>> amdgpu_ring *ring,> 
>>>   	unsigned vmid = AMDGPU_JOB_GET_VMID(job);
>>>   	u32 header, control = 0;
>>>
>>> -	/* insert SWITCH_BUFFER packet before first IB in the ring frame */
>>> -	if (flags & AMDGPU_HAVE_CTX_SWITCH) {
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -	}
>>
>> Commit message does not explain why the change of ring buffer command
>> this creates is okay. Current flow is:
>>
>> amdgpu_ib_schedule()
>> {
>> ...
>>    amdgpu_ring_emit_ib
>>      amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>
>>
>> New flow is:
>>
>> ...
>>    amdgpu_ring_emit_ib
>> ... other ring commands ...
>>    amdgpu_ring_emit_switch_buffer
>>      amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
> 
> No, that's not what the new flow is. If you check the callers of 
> emit_switch_buffer() you can see that it's called from two places:
> 
> - amdgpu_vm_flush() emits it before the first IB when necessary
> - amdgpu_ib_schedule() emits it after the last IB when necessary
> 
>> Is this okay? Specifically due the above comment saying "insert
>> SWITCH_BUFFER packet before first IB in the ring frame" - is the "first"
>> part not important?
> 
> amdgpu_vm_flush() emits it before the first IB.
> 
>> Also, amdgpu_ib_schedule only emits amdgpu_ring_emit_switch_buffer if
>> there is a job. Currently it is always emitted.
> 
> I trust that the GFX8+ implementations are more precise and that it's sufficient 
> to emit this packet in the cases where the emit_switch_buffer() function is 
> called.
> 
> When there is "no job" that's a special case that is only used during 
> initialization (specifically the IB ring tests). In that case we are not 
> executing commands submitted by userspace but rather commands generated by the 
> kernel. So we can be sure the CE is not used in those cases.
> 
>>
>> Final interesting part is how amdgpu_ib_schedule clears
>> AMDGPU_HAVE_CTX_SWITCH after having called amdgpu_ring_emit_ib.
>>
>> After this change only gfx6 remains the user of that flag in
>> gfx_v6_0_ring_emit_ib. Everyone else only use it in emit_cntxcntl. If
>> gfx6 was adjusted too (later), amdgpu_ib_schedule could reduce the scope
>> of that flag to just the scope where it calls amdgpu_ring_emit_frame_cntl.
> 
> I also adjusted the same thing for GFX6 in the next series.
> Can clean up the flag later once both series are accepted.
> 
> 
>>
>>> -
>>>
>>>   	if (ib->flags & AMDGPU_IB_FLAG_CE)
>>>   	
>>>   		header = PACKET3(PACKET3_INDIRECT_BUFFER_CONST, 2);
>>>   	
>>>   	else
>>>
>>> @@ -2258,6 +2252,12 @@ static void gfx_v7_0_ring_emit_ib_compute(struct
>>> amdgpu_ring *ring,> 
>>>   	amdgpu_ring_write(ring, control);
>>>   
>>>   }
>>>
>>> +static void gfx_v7_0_ring_emit_sb(struct amdgpu_ring *ring)
>>> +{
>>> +	amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 0));
>>> +	amdgpu_ring_write(ring, 0);
>>> +}
>>> +
>>>
>>>   static void gfx_v7_ring_emit_cntxcntl(struct amdgpu_ring *ring, uint32_t
>>>   flags) {
>>>   
>>>   	uint32_t dw2 = 0;
>>>
>>> @@ -3111,14 +3111,6 @@ static void gfx_v7_0_ring_emit_pipeline_sync(struct
>>> amdgpu_ring *ring)> 
>>>   	amdgpu_ring_write(ring, seq);
>>>   	amdgpu_ring_write(ring, 0xffffffff);
>>>   	amdgpu_ring_write(ring, 4); /* poll interval */
>>>
>>> -
>>> -	if (usepfp) {
>>> -		/* sync CE with ME to prevent CE fetch CEIB before 
> context switch done
>>> */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -	}
>>>
>>>   }
>>>   
>>>   /*
>>>
>>> @@ -3160,12 +3152,6 @@ static void gfx_v7_0_ring_emit_vm_flush(struct
>>> amdgpu_ring *ring,> 
>>>   		/* sync PFP to ME, otherwise we might get invalid PFP 
> reads */
>>>   		amdgpu_ring_write(ring, PACKET3(PACKET3_PFP_SYNC_ME, 
> 0));
>>>   		amdgpu_ring_write(ring, 0x0);
>>>
>>> -
>>> -		/* synce CE with ME to prevent CE fetch CEIB before 
> context switch done
>>> */ -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>> -		amdgpu_ring_write(ring, PACKET3(PACKET3_SWITCH_BUFFER, 
> 0));
>>> -		amdgpu_ring_write(ring, 0);
>>>
>>>   	}
>>>   
>>>   }
>>>
>>> @@ -4954,8 +4940,9 @@ static const struct amdgpu_ring_funcs
>>> gfx_v7_0_ring_funcs_gfx = {> 
>>>   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
>>>   		5 + /* hdp invalidate */
>>>   		12 + 12 + 12 + /* gfx_v7_0_ring_emit_fence_gfx x3 for 
> user fence, vm
>>>   		fence */> 
>>> -		7 + 4 + /* gfx_v7_0_ring_emit_pipeline_sync */
>>> -		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 6 + /* 
> gfx_v7_0_ring_emit_vm_flush
>>> */ +		7 + /* gfx_v7_0_ring_emit_pipeline_sync */
>>> +		CIK_FLUSH_GPU_TLB_NUM_WREG * 5 + 7 + 2 + /* 
> gfx_v7_0_ring_emit_vm_flush
>>> */ +		3 * 2 + /* gfx_v7_0_ring_emit_sb x3 (from 
> amdgpu_vm_flush,
>>> amdgpu_ib_schedule) */> 
>>>   		3 + 4 + /* gfx_v7_ring_emit_cntxcntl including vgt 
> flush*/
>>>   		5, /* SURFACE_SYNC */
>>>   	
>>>   	.emit_ib_size = 4, /* gfx_v7_0_ring_emit_ib_gfx */
>>>
>>> @@ -4969,6 +4956,7 @@ static const struct amdgpu_ring_funcs
>>> gfx_v7_0_ring_funcs_gfx = {> 
>>>   	.test_ib = gfx_v7_0_ring_test_ib,
>>>   	.insert_nop = amdgpu_ring_insert_nop,
>>>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>>>
>>> +	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
>>>
>>>   	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
>>>   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
>>>   	.soft_recovery = gfx_v7_0_ring_soft_recovery,
> 
> 
> 
> 

[PATCH 6/9] drm/amdgpu/gfx7: Clean up gfx ring during reset

[Timur Kristóf]

Clear the WPTR and RPTR at ring initialization.
Additionally clear the ring contents during reset.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index a93cc02c3400..915612628f9a 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -2546,8 +2546,14 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
 	WREG32(mmSCRATCH_ADDR, 0);
 
 	/* ring 0 - compute and gfx */
-	/* Set ring buffer size */
 	ring = &adev->gfx.gfx_ring[0];
+	atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
+	atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
+
+	if (amdgpu_in_reset(adev))
+		amdgpu_ring_clear_ring(ring);
+
+	/* Set ring buffer size */
 	rb_bufsz = order_base_2(ring->ring_size / 8);
 	tmp = (order_base_2(AMDGPU_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
 #ifdef __BIG_ENDIAN
@@ -2559,6 +2565,7 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
 	WREG32(mmCP_RB0_CNTL, tmp | CP_RB0_CNTL__RB_RPTR_WR_ENA_MASK);
 	ring->wptr = 0;
 	WREG32(mmCP_RB0_WPTR, lower_32_bits(ring->wptr));
+	WREG32(mmCP_RB0_RPTR, lower_32_bits(ring->wptr));
 
 	/* set the wb address whether it's enabled or not */
 	rptr_addr = ring->rptr_gpu_addr;
-- 
2.55.0

Re: [PATCH 6/9] drm/amdgpu/gfx7: Clean up gfx ring during reset

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> Clear the WPTR and RPTR at ring initialization.
> Additionally clear the ring contents during reset.

Please add the why part to your commit messages. ;)

Cover letter mentions the series is reworking to match gfx7 to gfx8 but 
I looked in drm-tip and amd-staging-drm-next and 
gfx_v8_0_cp_gfx_resume() does not yet have these changes.

> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 9 ++++++++-
>   1 file changed, 8 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index a93cc02c3400..915612628f9a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -2546,8 +2546,14 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
>   	WREG32(mmSCRATCH_ADDR, 0);
>   
>   	/* ring 0 - compute and gfx */
> -	/* Set ring buffer size */
>   	ring = &adev->gfx.gfx_ring[0];
> +	atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
> +	atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);

One day we need to fix this whole atomic64 situation.

Regards,

Tvrtko

> +
> +	if (amdgpu_in_reset(adev))
> +		amdgpu_ring_clear_ring(ring);
> +
> +	/* Set ring buffer size */
>   	rb_bufsz = order_base_2(ring->ring_size / 8);
>   	tmp = (order_base_2(AMDGPU_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
>   #ifdef __BIG_ENDIAN
> @@ -2559,6 +2565,7 @@ static int gfx_v7_0_cp_gfx_resume(struct amdgpu_device *adev)
>   	WREG32(mmCP_RB0_CNTL, tmp | CP_RB0_CNTL__RB_RPTR_WR_ENA_MASK);
>   	ring->wptr = 0;
>   	WREG32(mmCP_RB0_WPTR, lower_32_bits(ring->wptr));
> +	WREG32(mmCP_RB0_RPTR, lower_32_bits(ring->wptr));
>   
>   	/* set the wb address whether it's enabled or not */
>   	rptr_addr = ring->rptr_gpu_addr;

Re: [PATCH 6/9] drm/amdgpu/gfx7: Clean up gfx ring during reset

[Timur Kristóf]

On 2026. július 15., szerda 11:18:19 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > Clear the WPTR and RPTR at ring initialization.
> > Additionally clear the ring contents during reset.
> 
> Please add the why part to your commit messages. ;)

After a reset, the ring contents could be "dirty" and thus need to be cleared 
to prevent the command processor from executing packets left over in the ring 
from before the reset. I think this is obvious, but I'm happy to add some text 
to the commit message to explain it more.

> 
> Cover letter mentions the series is reworking to match gfx7 to gfx8 but
> I looked in drm-tip and amd-staging-drm-next and
> gfx_v8_0_cp_gfx_resume() does not yet have these changes.

The GFX8 code does have this:

For graphics queues:
In gfx_v8_0_cp_gfx_resume() it also sets wptr = 0; and calls 
amdgpu_ring_clear_ring() unconditionally. This is actually not needed during 
first initialization because the amdgpu_ring_init() called from 
gfx_v8_0_sw_init() already clears the ring, which is why I call the clear 
conditionally on GFX6-7. I could of course change the GFX6-7 code to also call 
it unconditionally though for the sake of simplicity and consistency.
(It is missing the atomic things though. I think it should have that too.)

For compute queues:
In gfx_v8_0_kcq_init_queue() it clears the wptr, also the wptr_cpu_addr and 
also calls amdgpu_ring_clear_ring().


> 
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> > 
> >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 9 ++++++++-
> >   1 file changed, 8 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index a93cc02c3400..915612628f9a
> > 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > @@ -2546,8 +2546,14 @@ static int gfx_v7_0_cp_gfx_resume(struct
> > amdgpu_device *adev)> 
> >   	WREG32(mmSCRATCH_ADDR, 0);
> >   	
> >   	/* ring 0 - compute and gfx */
> > 
> > -	/* Set ring buffer size */
> > 
> >   	ring = &adev->gfx.gfx_ring[0];
> > 
> > +	atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
> > +	atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
> 
> One day we need to fix this whole atomic64 situation.

What is the situation that needs to be fixed?


> 
> > +
> > +	if (amdgpu_in_reset(adev))
> > +		amdgpu_ring_clear_ring(ring);
> > +
> > +	/* Set ring buffer size */
> > 
> >   	rb_bufsz = order_base_2(ring->ring_size / 8);
> >   	tmp = (order_base_2(AMDGPU_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
> >   
> >   #ifdef __BIG_ENDIAN
> > 
> > @@ -2559,6 +2565,7 @@ static int gfx_v7_0_cp_gfx_resume(struct
> > amdgpu_device *adev)> 
> >   	WREG32(mmCP_RB0_CNTL, tmp | CP_RB0_CNTL__RB_RPTR_WR_ENA_MASK);
> >   	ring->wptr = 0;
> >   	WREG32(mmCP_RB0_WPTR, lower_32_bits(ring->wptr));
> > 
> > +	WREG32(mmCP_RB0_RPTR, lower_32_bits(ring->wptr));
> > 
> >   	/* set the wb address whether it's enabled or not */
> >   	rptr_addr = ring->rptr_gpu_addr;

Re: [PATCH 6/9] drm/amdgpu/gfx7: Clean up gfx ring during reset

[Tvrtko Ursulin]

On 15/07/2026 11:16, Timur Kristóf wrote:
> On 2026. július 15., szerda 11:18:19 közép-európai nyári idő Tvrtko Ursulin
> wrote:
>> On 13/07/2026 13:58, Timur Kristóf wrote:
>>> Clear the WPTR and RPTR at ring initialization.
>>> Additionally clear the ring contents during reset.
>>
>> Please add the why part to your commit messages. ;)
> 
> After a reset, the ring contents could be "dirty" and thus need to be cleared
> to prevent the command processor from executing packets left over in the ring
> from before the reset. I think this is obvious, but I'm happy to add some text
> to the commit message to explain it more.

Yes please, it's easier to review if commit messages always explain the 
what, how and why. It does not have to be long when it is simple like in 
this patch.

>> Cover letter mentions the series is reworking to match gfx7 to gfx8 but
>> I looked in drm-tip and amd-staging-drm-next and
>> gfx_v8_0_cp_gfx_resume() does not yet have these changes.
> 
> The GFX8 code does have this:
> 
> For graphics queues:
> In gfx_v8_0_cp_gfx_resume() it also sets wptr = 0; and calls
> amdgpu_ring_clear_ring() unconditionally. This is actually not needed during
> first initialization because the amdgpu_ring_init() called from
> gfx_v8_0_sw_init() already clears the ring, which is why I call the clear
> conditionally on GFX6-7. I could of course change the GFX6-7 code to also call
> it unconditionally though for the sake of simplicity and consistency.
> (It is missing the atomic things though. I think it should have that too.)

Ack. Then just improve the commit message please - instead "Additionally 
clear the ring contents during reset", have something like "Move the 
existing clearing of ring content to happen only during reset because it 
is otherwise already done during ring init".

Presumably this is also true during resume?

> For compute queues:
> In gfx_v8_0_kcq_init_queue() it clears the wptr, also the wptr_cpu_addr and
> also calls amdgpu_ring_clear_ring().

Specifically about wptr_cpu_addr and rptr_cpu_addr.

1)
Does gfx7 have 64-bit support or normal write would do? Ie. I don't see 
gfx7 code using the atomic64. 64-bit support seems to start in gfx9.

2)
Does rptr_cpu_addr need touching at all? Out of all other files only MES 
does it. At least if my quick grep is to be trusted.

>>
>>> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
>>> ---
>>>
>>>    drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 9 ++++++++-
>>>    1 file changed, 8 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index a93cc02c3400..915612628f9a
>>> 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
>>> @@ -2546,8 +2546,14 @@ static int gfx_v7_0_cp_gfx_resume(struct
>>> amdgpu_device *adev)>
>>>    	WREG32(mmSCRATCH_ADDR, 0);
>>>    	
>>>    	/* ring 0 - compute and gfx */
>>>
>>> -	/* Set ring buffer size */
>>>
>>>    	ring = &adev->gfx.gfx_ring[0];
>>>
>>> +	atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
>>> +	atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
>>
>> One day we need to fix this whole atomic64 situation.
> 
> What is the situation that needs to be fixed?

It is defined as normal u32 * and then, AFAIU, different ip version use 
32- or 64-bit access depending on whether the respective ip version has 
support for 64-bit ring pointers. The ugly casting should be hidden in 
wrappers as minimum. Another task for a rainy day.

Regards,

Tvrtko

>>
>>> +
>>> +	if (amdgpu_in_reset(adev))
>>> +		amdgpu_ring_clear_ring(ring);
>>> +
>>> +	/* Set ring buffer size */
>>>
>>>    	rb_bufsz = order_base_2(ring->ring_size / 8);
>>>    	tmp = (order_base_2(AMDGPU_GPU_PAGE_SIZE/8) << 8) | rb_bufsz;
>>>    
>>>    #ifdef __BIG_ENDIAN
>>>
>>> @@ -2559,6 +2565,7 @@ static int gfx_v7_0_cp_gfx_resume(struct
>>> amdgpu_device *adev)>
>>>    	WREG32(mmCP_RB0_CNTL, tmp | CP_RB0_CNTL__RB_RPTR_WR_ENA_MASK);
>>>    	ring->wptr = 0;
>>>    	WREG32(mmCP_RB0_WPTR, lower_32_bits(ring->wptr));
>>>
>>> +	WREG32(mmCP_RB0_RPTR, lower_32_bits(ring->wptr));
>>>
>>>    	/* set the wb address whether it's enabled or not */
>>>    	rptr_addr = ring->rptr_gpu_addr;
> 
> 
> 
> 

[PATCH 7/9] drm/amdgpu/gfx7: Use COND_EXEC

[Timur Kristóf]

COND_EXEC tells the CP to discard the dwords following it
when its condition is zero (false).

This is useful for GPU recovery because it can help reduce
collateral damage during GFX IP block soft reset, meaning
that it reduces the likelyhood that we fail some jobs which
are not guilty of the hang as the IP block soft reset
mechanism clears the condition before doing the reset.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 22 ++++++++++++++++++++++
 1 file changed, 22 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 915612628f9a..6d52b8710437 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -3162,6 +3162,22 @@ static void gfx_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
 	}
 }
 
+static unsigned int gfx_v7_0_ring_emit_init_cond_exec(struct amdgpu_ring *ring,
+						      uint64_t gpu_addr)
+{
+	unsigned int ret;
+
+	/* Discard following DWs after this packet when gpu_addr==0 */
+	amdgpu_ring_write(ring, PACKET3(PACKET3_COND_EXEC, 3));
+	amdgpu_ring_write(ring, lower_32_bits(gpu_addr));
+	amdgpu_ring_write(ring, upper_32_bits(gpu_addr));
+	amdgpu_ring_write(ring, 0);
+	ret = ring->wptr & ring->buf_mask;
+	/* patch dummy value later */
+	amdgpu_ring_write(ring, 0);
+	return ret;
+}
+
 static void gfx_v7_0_ring_emit_wreg(struct amdgpu_ring *ring,
 				    uint32_t reg, uint32_t val)
 {
@@ -4943,6 +4959,8 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
 	.get_wptr = gfx_v7_0_ring_get_wptr_gfx,
 	.set_wptr = gfx_v7_0_ring_set_wptr_gfx,
 	.emit_frame_size =
+		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_ib_schedule) */
+		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_vm_flush) */
 		20 + /* gfx_v7_0_ring_emit_gds_switch */
 		7 + /* gfx_v7_0_ring_emit_hdp_flush */
 		5 + /* hdp invalidate */
@@ -4965,6 +4983,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
 	.pad_ib = amdgpu_ring_generic_pad_ib,
 	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
 	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
+	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
 	.emit_wreg = gfx_v7_0_ring_emit_wreg,
 	.soft_recovery = gfx_v7_0_ring_soft_recovery,
 	.emit_mem_sync = gfx_v7_0_emit_mem_sync,
@@ -4979,6 +4998,8 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_compute = {
 	.get_wptr = gfx_v7_0_ring_get_wptr_compute,
 	.set_wptr = gfx_v7_0_ring_set_wptr_compute,
 	.emit_frame_size =
+		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_ib_schedule) */
+		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_vm_flush) */
 		20 + /* gfx_v7_0_ring_emit_gds_switch */
 		7 + /* gfx_v7_0_ring_emit_hdp_flush */
 		5 + /* hdp invalidate */
@@ -4997,6 +5018,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_compute = {
 	.test_ib = gfx_v7_0_ring_test_ib,
 	.insert_nop = amdgpu_ring_insert_nop,
 	.pad_ib = amdgpu_ring_generic_pad_ib,
+	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
 	.emit_wreg = gfx_v7_0_ring_emit_wreg,
 	.soft_recovery = gfx_v7_0_ring_soft_recovery,
 	.emit_mem_sync = gfx_v7_0_emit_mem_sync_compute,
-- 
2.55.0

Re: [PATCH 7/9] drm/amdgpu/gfx7: Use COND_EXEC

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> COND_EXEC tells the CP to discard the dwords following it
> when its condition is zero (false).
> 
> This is useful for GPU recovery because it can help reduce
> collateral damage during GFX IP block soft reset, meaning
> that it reduces the likelyhood that we fail some jobs which
> are not guilty of the hang as the IP block soft reset
> mechanism clears the condition before doing the reset.
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 22 ++++++++++++++++++++++
>   1 file changed, 22 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 915612628f9a..6d52b8710437 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -3162,6 +3162,22 @@ static void gfx_v7_0_ring_emit_vm_flush(struct amdgpu_ring *ring,
>   	}
>   }
>   
> +static unsigned int gfx_v7_0_ring_emit_init_cond_exec(struct amdgpu_ring *ring,
> +						      uint64_t gpu_addr)
> +{
> +	unsigned int ret;
> +
> +	/* Discard following DWs after this packet when gpu_addr==0 */
> +	amdgpu_ring_write(ring, PACKET3(PACKET3_COND_EXEC, 3));
> +	amdgpu_ring_write(ring, lower_32_bits(gpu_addr));
> +	amdgpu_ring_write(ring, upper_32_bits(gpu_addr));
> +	amdgpu_ring_write(ring, 0);
> +	ret = ring->wptr & ring->buf_mask;
> +	/* patch dummy value later */
> +	amdgpu_ring_write(ring, 0);
> +	return ret;
> +}
> +
>   static void gfx_v7_0_ring_emit_wreg(struct amdgpu_ring *ring,
>   				    uint32_t reg, uint32_t val)
>   {
> @@ -4943,6 +4959,8 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
>   	.get_wptr = gfx_v7_0_ring_get_wptr_gfx,
>   	.set_wptr = gfx_v7_0_ring_set_wptr_gfx,
>   	.emit_frame_size =
> +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_ib_schedule) */
> +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_vm_flush) */

gfx8 version /appears/ to make an effort to keep the additions here 
sorted in the emission order. I think at least. Not sure how much value 
it adds since the whole emit_frame_size mechanism is "fragilotedious". 
:) Another task for a rainy day - we see if it can be replaced with 
something more manageable. Anyway, digressions aside:

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

>   		20 + /* gfx_v7_0_ring_emit_gds_switch */
>   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
>   		5 + /* hdp invalidate */
> @@ -4965,6 +4983,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_gfx = {
>   	.pad_ib = amdgpu_ring_generic_pad_ib,
>   	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
>   	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
> +	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
>   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
>   	.soft_recovery = gfx_v7_0_ring_soft_recovery,
>   	.emit_mem_sync = gfx_v7_0_emit_mem_sync,
> @@ -4979,6 +4998,8 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_compute = {
>   	.get_wptr = gfx_v7_0_ring_get_wptr_compute,
>   	.set_wptr = gfx_v7_0_ring_set_wptr_compute,
>   	.emit_frame_size =
> +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_ib_schedule) */
> +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from amdgpu_vm_flush) */
>   		20 + /* gfx_v7_0_ring_emit_gds_switch */
>   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
>   		5 + /* hdp invalidate */
> @@ -4997,6 +5018,7 @@ static const struct amdgpu_ring_funcs gfx_v7_0_ring_funcs_compute = {
>   	.test_ib = gfx_v7_0_ring_test_ib,
>   	.insert_nop = amdgpu_ring_insert_nop,
>   	.pad_ib = amdgpu_ring_generic_pad_ib,
> +	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
>   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
>   	.soft_recovery = gfx_v7_0_ring_soft_recovery,
>   	.emit_mem_sync = gfx_v7_0_emit_mem_sync_compute,

Re: [PATCH 7/9] drm/amdgpu/gfx7: Use COND_EXEC

[Timur Kristóf]

On 2026. július 15., szerda 11:38:40 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > COND_EXEC tells the CP to discard the dwords following it
> > when its condition is zero (false).
> > 
> > This is useful for GPU recovery because it can help reduce
> > collateral damage during GFX IP block soft reset, meaning
> > that it reduces the likelyhood that we fail some jobs which
> > are not guilty of the hang as the IP block soft reset
> > mechanism clears the condition before doing the reset.
> > 
> > Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> > ---
> > 
> >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 22 ++++++++++++++++++++++
> >   1 file changed, 22 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 915612628f9a..6d52b8710437
> > 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > @@ -3162,6 +3162,22 @@ static void gfx_v7_0_ring_emit_vm_flush(struct
> > amdgpu_ring *ring,> 
> >   	}
> >   
> >   }
> > 
> > +static unsigned int gfx_v7_0_ring_emit_init_cond_exec(struct amdgpu_ring
> > *ring, +						      
uint64_t gpu_addr)
> > +{
> > +	unsigned int ret;
> > +
> > +	/* Discard following DWs after this packet when gpu_addr==0 */
> > +	amdgpu_ring_write(ring, PACKET3(PACKET3_COND_EXEC, 3));
> > +	amdgpu_ring_write(ring, lower_32_bits(gpu_addr));
> > +	amdgpu_ring_write(ring, upper_32_bits(gpu_addr));
> > +	amdgpu_ring_write(ring, 0);
> > +	ret = ring->wptr & ring->buf_mask;
> > +	/* patch dummy value later */
> > +	amdgpu_ring_write(ring, 0);
> > +	return ret;
> > +}
> > +
> > 
> >   static void gfx_v7_0_ring_emit_wreg(struct amdgpu_ring *ring,
> >   
> >   				    uint32_t reg, uint32_t val)
> >   
> >   {
> > 
> > @@ -4943,6 +4959,8 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_gfx = {> 
> >   	.get_wptr = gfx_v7_0_ring_get_wptr_gfx,
> >   	.set_wptr = gfx_v7_0_ring_set_wptr_gfx,
> >   	.emit_frame_size =
> > 
> > +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from 
amdgpu_ib_schedule) */
> > +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from 
amdgpu_vm_flush) */
> 
> gfx8 version /appears/ to make an effort to keep the additions here
> sorted in the emission order. I think at least.

As far as I see it was already not in emission order here.

> Not sure how much value
> it adds since the whole emit_frame_size mechanism is "fragilotedious".
> 
> :) Another task for a rainy day - we see if it can be replaced with
> 
> something more manageable.

If you have ideas how to clean it up, I'd be happy to hear it.

> Anyway, digressions aside:
> 
> Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>
> 
> Regards,
> 
> Tvrtko
> 
> >   		20 + /* gfx_v7_0_ring_emit_gds_switch */
> >   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
> >   		5 + /* hdp invalidate */
> > 
> > @@ -4965,6 +4983,7 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_gfx = {> 
> >   	.pad_ib = amdgpu_ring_generic_pad_ib,
> >   	.emit_switch_buffer = gfx_v7_0_ring_emit_sb,
> >   	.emit_cntxcntl = gfx_v7_ring_emit_cntxcntl,
> > 
> > +	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
> > 
> >   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
> >   	.soft_recovery = gfx_v7_0_ring_soft_recovery,
> >   	.emit_mem_sync = gfx_v7_0_emit_mem_sync,
> > 
> > @@ -4979,6 +4998,8 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_compute = {> 
> >   	.get_wptr = gfx_v7_0_ring_get_wptr_compute,
> >   	.set_wptr = gfx_v7_0_ring_set_wptr_compute,
> >   	.emit_frame_size =
> > 
> > +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from 
amdgpu_ib_schedule) */
> > +		5 + /* gfx_v7_0_ring_emit_init_cond_exec (from 
amdgpu_vm_flush) */
> > 
> >   		20 + /* gfx_v7_0_ring_emit_gds_switch */
> >   		7 + /* gfx_v7_0_ring_emit_hdp_flush */
> >   		5 + /* hdp invalidate */
> > 
> > @@ -4997,6 +5018,7 @@ static const struct amdgpu_ring_funcs
> > gfx_v7_0_ring_funcs_compute = {> 
> >   	.test_ib = gfx_v7_0_ring_test_ib,
> >   	.insert_nop = amdgpu_ring_insert_nop,
> >   	.pad_ib = amdgpu_ring_generic_pad_ib,
> > 
> > +	.init_cond_exec = gfx_v7_0_ring_emit_init_cond_exec,
> > 
> >   	.emit_wreg = gfx_v7_0_ring_emit_wreg,
> >   	.soft_recovery = gfx_v7_0_ring_soft_recovery,
> >   	.emit_mem_sync = gfx_v7_0_emit_mem_sync_compute,

[PATCH 8/9] drm/amdgpu/gfx7: Fixup IP block soft reset

[Timur Kristóf]

Use basically the same implementation as GFX8,
except for the GFX7 specific MQD functions.

Reset every block using the GRBM, then proceed
to reset the GRBM and SEM blocks using the SRBM.

Remove the redundant gfx_v7_0_update_cg() function.
The soft reset now calls the clock and powergating
functions of the IP block instead.

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 148 +++++++++++++-------------
 1 file changed, 76 insertions(+), 72 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 6d52b8710437..825e3d7d5f0f 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -3618,21 +3618,6 @@ static void gfx_v7_0_enable_mgcg(struct amdgpu_device *adev, bool enable)
 	}
 }
 
-static void gfx_v7_0_update_cg(struct amdgpu_device *adev,
-			       bool enable)
-{
-	gfx_v7_0_enable_gui_idle_interrupt(adev, false);
-	/* order matters! */
-	if (enable) {
-		gfx_v7_0_enable_mgcg(adev, true);
-		gfx_v7_0_enable_cgcg(adev, true);
-	} else {
-		gfx_v7_0_enable_cgcg(adev, false);
-		gfx_v7_0_enable_mgcg(adev, false);
-	}
-	gfx_v7_0_enable_gui_idle_interrupt(adev, true);
-}
-
 static void gfx_v7_0_enable_sclk_slowdown_on_pu(struct amdgpu_device *adev,
 						bool enable)
 {
@@ -4551,80 +4536,99 @@ static int gfx_v7_0_wait_for_idle(struct amdgpu_ip_block *ip_block)
 
 static int gfx_v7_0_soft_reset(struct amdgpu_ip_block *ip_block)
 {
+	struct amdgpu_device *adev = ip_block->adev;
 	u32 grbm_soft_reset = 0, srbm_soft_reset = 0;
 	u32 tmp;
-	struct amdgpu_device *adev = ip_block->adev;
+	int i;
+	int r;
 
-	/* GRBM_STATUS */
-	tmp = RREG32(mmGRBM_STATUS);
-	if (tmp & (GRBM_STATUS__PA_BUSY_MASK | GRBM_STATUS__SC_BUSY_MASK |
-		   GRBM_STATUS__BCI_BUSY_MASK | GRBM_STATUS__SX_BUSY_MASK |
-		   GRBM_STATUS__TA_BUSY_MASK | GRBM_STATUS__VGT_BUSY_MASK |
-		   GRBM_STATUS__DB_BUSY_MASK | GRBM_STATUS__CB_BUSY_MASK |
-		   GRBM_STATUS__GDS_BUSY_MASK | GRBM_STATUS__SPI_BUSY_MASK |
-		   GRBM_STATUS__IA_BUSY_MASK | GRBM_STATUS__IA_BUSY_NO_DMA_MASK))
-		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_CP_MASK |
-			GRBM_SOFT_RESET__SOFT_RESET_GFX_MASK;
-
-	if (tmp & (GRBM_STATUS__CP_BUSY_MASK | GRBM_STATUS__CP_COHERENCY_BUSY_MASK)) {
-		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_CP_MASK;
-		srbm_soft_reset |= SRBM_SOFT_RESET__SOFT_RESET_GRBM_MASK;
-	}
+	grbm_soft_reset =
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_RLC, 1) |
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_GFX, 1) |
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CP, 1) |
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPF, 1) |
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPC, 1) |
+		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPG, 1);
+
+	srbm_soft_reset =
+		REG_SET_FIELD(0, SRBM_SOFT_RESET, SOFT_RESET_GRBM, 1) |
+		REG_SET_FIELD(0, SRBM_SOFT_RESET, SOFT_RESET_SEM, 1);
+
+	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
+		struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
+
+		mutex_lock(&adev->srbm_mutex);
+		cik_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
+		gfx_v7_0_mqd_deactivate(adev, 2);
+		cik_srbm_select(adev, 0, 0, 0, 0);
+		mutex_unlock(&adev->srbm_mutex);
 
-	/* GRBM_STATUS2 */
-	tmp = RREG32(mmGRBM_STATUS2);
-	if (tmp & GRBM_STATUS2__RLC_BUSY_MASK)
-		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_RLC_MASK;
+		udelay(50);
+	}
 
-	/* SRBM_STATUS */
-	tmp = RREG32(mmSRBM_STATUS);
-	if (tmp & SRBM_STATUS__GRBM_RQ_PENDING_MASK)
-		srbm_soft_reset |= SRBM_SOFT_RESET__SOFT_RESET_GRBM_MASK;
+	ip_block->version->funcs->set_clockgating_state(ip_block, AMD_CG_STATE_UNGATE);
+	ip_block->version->funcs->set_powergating_state(ip_block, AMD_PG_STATE_UNGATE);
+	ip_block->version->funcs->suspend(ip_block);
 
 	if (grbm_soft_reset || srbm_soft_reset) {
-		/* disable CG/PG */
-		gfx_v7_0_fini_pg(adev);
-		gfx_v7_0_update_cg(adev, false);
+		tmp = RREG32(mmGMCON_DEBUG);
+		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_STALL, 1);
+		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_CLEAR, 1);
+		WREG32(mmGMCON_DEBUG, tmp);
 
-		/* stop the rlc */
-		adev->gfx.rlc.funcs->stop(adev);
+		udelay(100);
+	}
 
-		/* Disable GFX parsing/prefetching */
-		WREG32(mmCP_ME_CNTL, CP_ME_CNTL__ME_HALT_MASK | CP_ME_CNTL__PFP_HALT_MASK | CP_ME_CNTL__CE_HALT_MASK);
+	if (grbm_soft_reset) {
+		tmp = RREG32(mmGRBM_SOFT_RESET);
+		tmp |= grbm_soft_reset;
+		dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
+		WREG32(mmGRBM_SOFT_RESET, tmp);
+		tmp = RREG32(mmGRBM_SOFT_RESET);
 
-		/* Disable MEC parsing/prefetching */
-		WREG32(mmCP_MEC_CNTL, CP_MEC_CNTL__MEC_ME1_HALT_MASK | CP_MEC_CNTL__MEC_ME2_HALT_MASK);
+		udelay(100);
 
-		if (grbm_soft_reset) {
-			tmp = RREG32(mmGRBM_SOFT_RESET);
-			tmp |= grbm_soft_reset;
-			dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
-			WREG32(mmGRBM_SOFT_RESET, tmp);
-			tmp = RREG32(mmGRBM_SOFT_RESET);
+		tmp &= ~grbm_soft_reset;
+		WREG32(mmGRBM_SOFT_RESET, tmp);
+		tmp = RREG32(mmGRBM_SOFT_RESET);
 
-			udelay(50);
+		udelay(100);
+	}
 
-			tmp &= ~grbm_soft_reset;
-			WREG32(mmGRBM_SOFT_RESET, tmp);
-			tmp = RREG32(mmGRBM_SOFT_RESET);
-		}
+	if (srbm_soft_reset) {
+		tmp = RREG32(mmSRBM_SOFT_RESET);
+		tmp |= srbm_soft_reset;
+		dev_info(adev->dev, "SRBM_SOFT_RESET=0x%08X\n", tmp);
+		WREG32(mmSRBM_SOFT_RESET, tmp);
+		tmp = RREG32(mmSRBM_SOFT_RESET);
 
-		if (srbm_soft_reset) {
-			tmp = RREG32(mmSRBM_SOFT_RESET);
-			tmp |= srbm_soft_reset;
-			dev_info(adev->dev, "SRBM_SOFT_RESET=0x%08X\n", tmp);
-			WREG32(mmSRBM_SOFT_RESET, tmp);
-			tmp = RREG32(mmSRBM_SOFT_RESET);
+		udelay(100);
 
-			udelay(50);
+		tmp &= ~srbm_soft_reset;
+		WREG32(mmSRBM_SOFT_RESET, tmp);
+		tmp = RREG32(mmSRBM_SOFT_RESET);
 
-			tmp &= ~srbm_soft_reset;
-			WREG32(mmSRBM_SOFT_RESET, tmp);
-			tmp = RREG32(mmSRBM_SOFT_RESET);
-		}
-		/* Wait a little for things to settle down */
-		udelay(50);
+		udelay(100);
 	}
+
+	if (grbm_soft_reset || srbm_soft_reset) {
+		tmp = RREG32(mmGMCON_DEBUG);
+		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_STALL, 0);
+		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_CLEAR, 0);
+		WREG32(mmGMCON_DEBUG, tmp);
+	}
+
+	/* Wait a little for things to settle down */
+	udelay(100);
+
+	r = ip_block->version->funcs->resume(ip_block);
+	r |= ip_block->version->funcs->late_init(ip_block);
+	if (r)
+		return r;
+
+	ip_block->version->funcs->set_clockgating_state(ip_block, AMD_CG_STATE_GATE);
+	ip_block->version->funcs->set_powergating_state(ip_block, AMD_PG_STATE_GATE);
+
 	return 0;
 }
 
-- 
2.55.0

Re: [PATCH 8/9] drm/amdgpu/gfx7: Fixup IP block soft reset

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> Use basically the same implementation as GFX8,
> except for the GFX7 specific MQD functions.
> 
> Reset every block using the GRBM, then proceed
> to reset the GRBM and SEM blocks using the SRBM.
> 
> Remove the redundant gfx_v7_0_update_cg() function.
> The soft reset now calls the clock and powergating
> functions of the IP block instead.

Does what it says on the tin - matches the gfx8 implementation.

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko

> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 148 +++++++++++++-------------
>   1 file changed, 76 insertions(+), 72 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 6d52b8710437..825e3d7d5f0f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -3618,21 +3618,6 @@ static void gfx_v7_0_enable_mgcg(struct amdgpu_device *adev, bool enable)
>   	}
>   }
>   
> -static void gfx_v7_0_update_cg(struct amdgpu_device *adev,
> -			       bool enable)
> -{
> -	gfx_v7_0_enable_gui_idle_interrupt(adev, false);
> -	/* order matters! */
> -	if (enable) {
> -		gfx_v7_0_enable_mgcg(adev, true);
> -		gfx_v7_0_enable_cgcg(adev, true);
> -	} else {
> -		gfx_v7_0_enable_cgcg(adev, false);
> -		gfx_v7_0_enable_mgcg(adev, false);
> -	}
> -	gfx_v7_0_enable_gui_idle_interrupt(adev, true);
> -}
> -
>   static void gfx_v7_0_enable_sclk_slowdown_on_pu(struct amdgpu_device *adev,
>   						bool enable)
>   {
> @@ -4551,80 +4536,99 @@ static int gfx_v7_0_wait_for_idle(struct amdgpu_ip_block *ip_block)
>   
>   static int gfx_v7_0_soft_reset(struct amdgpu_ip_block *ip_block)
>   {
> +	struct amdgpu_device *adev = ip_block->adev;
>   	u32 grbm_soft_reset = 0, srbm_soft_reset = 0;
>   	u32 tmp;
> -	struct amdgpu_device *adev = ip_block->adev;
> +	int i;
> +	int r;
>   
> -	/* GRBM_STATUS */
> -	tmp = RREG32(mmGRBM_STATUS);
> -	if (tmp & (GRBM_STATUS__PA_BUSY_MASK | GRBM_STATUS__SC_BUSY_MASK |
> -		   GRBM_STATUS__BCI_BUSY_MASK | GRBM_STATUS__SX_BUSY_MASK |
> -		   GRBM_STATUS__TA_BUSY_MASK | GRBM_STATUS__VGT_BUSY_MASK |
> -		   GRBM_STATUS__DB_BUSY_MASK | GRBM_STATUS__CB_BUSY_MASK |
> -		   GRBM_STATUS__GDS_BUSY_MASK | GRBM_STATUS__SPI_BUSY_MASK |
> -		   GRBM_STATUS__IA_BUSY_MASK | GRBM_STATUS__IA_BUSY_NO_DMA_MASK))
> -		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_CP_MASK |
> -			GRBM_SOFT_RESET__SOFT_RESET_GFX_MASK;
> -
> -	if (tmp & (GRBM_STATUS__CP_BUSY_MASK | GRBM_STATUS__CP_COHERENCY_BUSY_MASK)) {
> -		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_CP_MASK;
> -		srbm_soft_reset |= SRBM_SOFT_RESET__SOFT_RESET_GRBM_MASK;
> -	}
> +	grbm_soft_reset =
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_RLC, 1) |
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_GFX, 1) |
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CP, 1) |
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPF, 1) |
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPC, 1) |
> +		REG_SET_FIELD(0, GRBM_SOFT_RESET, SOFT_RESET_CPG, 1);
> +
> +	srbm_soft_reset =
> +		REG_SET_FIELD(0, SRBM_SOFT_RESET, SOFT_RESET_GRBM, 1) |
> +		REG_SET_FIELD(0, SRBM_SOFT_RESET, SOFT_RESET_SEM, 1);
> +
> +	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> +		struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
> +
> +		mutex_lock(&adev->srbm_mutex);
> +		cik_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
> +		gfx_v7_0_mqd_deactivate(adev, 2);
> +		cik_srbm_select(adev, 0, 0, 0, 0);
> +		mutex_unlock(&adev->srbm_mutex);
>   
> -	/* GRBM_STATUS2 */
> -	tmp = RREG32(mmGRBM_STATUS2);
> -	if (tmp & GRBM_STATUS2__RLC_BUSY_MASK)
> -		grbm_soft_reset |= GRBM_SOFT_RESET__SOFT_RESET_RLC_MASK;
> +		udelay(50);
> +	}
>   
> -	/* SRBM_STATUS */
> -	tmp = RREG32(mmSRBM_STATUS);
> -	if (tmp & SRBM_STATUS__GRBM_RQ_PENDING_MASK)
> -		srbm_soft_reset |= SRBM_SOFT_RESET__SOFT_RESET_GRBM_MASK;
> +	ip_block->version->funcs->set_clockgating_state(ip_block, AMD_CG_STATE_UNGATE);
> +	ip_block->version->funcs->set_powergating_state(ip_block, AMD_PG_STATE_UNGATE);
> +	ip_block->version->funcs->suspend(ip_block);
>   
>   	if (grbm_soft_reset || srbm_soft_reset) {
> -		/* disable CG/PG */
> -		gfx_v7_0_fini_pg(adev);
> -		gfx_v7_0_update_cg(adev, false);
> +		tmp = RREG32(mmGMCON_DEBUG);
> +		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_STALL, 1);
> +		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_CLEAR, 1);
> +		WREG32(mmGMCON_DEBUG, tmp);
>   
> -		/* stop the rlc */
> -		adev->gfx.rlc.funcs->stop(adev);
> +		udelay(100);
> +	}
>   
> -		/* Disable GFX parsing/prefetching */
> -		WREG32(mmCP_ME_CNTL, CP_ME_CNTL__ME_HALT_MASK | CP_ME_CNTL__PFP_HALT_MASK | CP_ME_CNTL__CE_HALT_MASK);
> +	if (grbm_soft_reset) {
> +		tmp = RREG32(mmGRBM_SOFT_RESET);
> +		tmp |= grbm_soft_reset;
> +		dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> +		WREG32(mmGRBM_SOFT_RESET, tmp);
> +		tmp = RREG32(mmGRBM_SOFT_RESET);
>   
> -		/* Disable MEC parsing/prefetching */
> -		WREG32(mmCP_MEC_CNTL, CP_MEC_CNTL__MEC_ME1_HALT_MASK | CP_MEC_CNTL__MEC_ME2_HALT_MASK);
> +		udelay(100);
>   
> -		if (grbm_soft_reset) {
> -			tmp = RREG32(mmGRBM_SOFT_RESET);
> -			tmp |= grbm_soft_reset;
> -			dev_info(adev->dev, "GRBM_SOFT_RESET=0x%08X\n", tmp);
> -			WREG32(mmGRBM_SOFT_RESET, tmp);
> -			tmp = RREG32(mmGRBM_SOFT_RESET);
> +		tmp &= ~grbm_soft_reset;
> +		WREG32(mmGRBM_SOFT_RESET, tmp);
> +		tmp = RREG32(mmGRBM_SOFT_RESET);
>   
> -			udelay(50);
> +		udelay(100);
> +	}
>   
> -			tmp &= ~grbm_soft_reset;
> -			WREG32(mmGRBM_SOFT_RESET, tmp);
> -			tmp = RREG32(mmGRBM_SOFT_RESET);
> -		}
> +	if (srbm_soft_reset) {
> +		tmp = RREG32(mmSRBM_SOFT_RESET);
> +		tmp |= srbm_soft_reset;
> +		dev_info(adev->dev, "SRBM_SOFT_RESET=0x%08X\n", tmp);
> +		WREG32(mmSRBM_SOFT_RESET, tmp);
> +		tmp = RREG32(mmSRBM_SOFT_RESET);
>   
> -		if (srbm_soft_reset) {
> -			tmp = RREG32(mmSRBM_SOFT_RESET);
> -			tmp |= srbm_soft_reset;
> -			dev_info(adev->dev, "SRBM_SOFT_RESET=0x%08X\n", tmp);
> -			WREG32(mmSRBM_SOFT_RESET, tmp);
> -			tmp = RREG32(mmSRBM_SOFT_RESET);
> +		udelay(100);
>   
> -			udelay(50);
> +		tmp &= ~srbm_soft_reset;
> +		WREG32(mmSRBM_SOFT_RESET, tmp);
> +		tmp = RREG32(mmSRBM_SOFT_RESET);
>   
> -			tmp &= ~srbm_soft_reset;
> -			WREG32(mmSRBM_SOFT_RESET, tmp);
> -			tmp = RREG32(mmSRBM_SOFT_RESET);
> -		}
> -		/* Wait a little for things to settle down */
> -		udelay(50);
> +		udelay(100);
>   	}
> +
> +	if (grbm_soft_reset || srbm_soft_reset) {
> +		tmp = RREG32(mmGMCON_DEBUG);
> +		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_STALL, 0);
> +		tmp = REG_SET_FIELD(tmp, GMCON_DEBUG, GFX_CLEAR, 0);
> +		WREG32(mmGMCON_DEBUG, tmp);
> +	}
> +
> +	/* Wait a little for things to settle down */
> +	udelay(100);
> +
> +	r = ip_block->version->funcs->resume(ip_block);
> +	r |= ip_block->version->funcs->late_init(ip_block);
> +	if (r)
> +		return r;
> +
> +	ip_block->version->funcs->set_clockgating_state(ip_block, AMD_CG_STATE_GATE);
> +	ip_block->version->funcs->set_powergating_state(ip_block, AMD_PG_STATE_GATE);
> +
>   	return 0;
>   }
>   

[PATCH 9/9] drm/amdgpu/gfx7: Enable IP block soft reset as a GPU recovery method

[Timur Kristóf]

Enable IP block soft reset as a GPU recovery method for GFX7
graphics and compute rings.

This improves current user experience on all GFX7 chips:

* On Kaveri and Kabini there is currently no working
  GPU recovery method so those chips currently require
  the user to manually reset the computer when there
  was a hang.

* On Hawaii and Bonaire, the current GPU recovery method
  always clears the contents of VRAM, which means that
  a buggy (hanging) app can crash the whole graphical
  session, which is less than ideal.

Using GFX IP block soft reset means that we can now
have a working recovery on GFX7 APUs and we can also
move on from GFX hangs on dGPUs without crashing the
whole system.

Tested with the "hard_reset_cp_wait" test case from the
Hang Test Suite created by Natalie Vock and Konstantin Seurer.
This Vulkan testcase waits for an event that never occurs,
effectively a WAIT_REG_MEM packet that intentionally hangs.
IP block soft reset can resolve that hang and allow
the rest of the system to move on and keep functioning
without needing a full ASIC reset.

Tested on the following chips:

Bonaire (Radeon HD 7790)
Hawaii (Radeon R9 390X)
Kaveri (A10-7850K)

Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
---
 drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 5 +++++
 1 file changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
index 825e3d7d5f0f..1a7a6962303e 100644
--- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
+++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
@@ -4434,6 +4434,11 @@ static int gfx_v7_0_sw_init(struct amdgpu_ip_block *ip_block)
 	adev->gfx.compute_supported_reset =
 		amdgpu_get_soft_full_reset_mask(&adev->gfx.compute_ring[0]);
 
+	if (!amdgpu_sriov_vf(adev) && !adev->debug_disable_ip_block_soft_reset) {
+		adev->gfx.compute_supported_reset |= AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET;
+		adev->gfx.gfx_supported_reset |= AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET;
+	}
+
 	return r;
 }
 
-- 
2.55.0

Re: [PATCH 9/9] drm/amdgpu/gfx7: Enable IP block soft reset as a GPU recovery method

[Tvrtko Ursulin]

On 13/07/2026 13:58, Timur Kristóf wrote:
> Enable IP block soft reset as a GPU recovery method for GFX7
> graphics and compute rings.
> 
> This improves current user experience on all GFX7 chips:
> 
> * On Kaveri and Kabini there is currently no working
>    GPU recovery method so those chips currently require
>    the user to manually reset the computer when there
>    was a hang.
> 
> * On Hawaii and Bonaire, the current GPU recovery method
>    always clears the contents of VRAM, which means that
>    a buggy (hanging) app can crash the whole graphical
>    session, which is less than ideal.
> 
> Using GFX IP block soft reset means that we can now
> have a working recovery on GFX7 APUs and we can also
> move on from GFX hangs on dGPUs without crashing the
> whole system.
> 
> Tested with the "hard_reset_cp_wait" test case from the
> Hang Test Suite created by Natalie Vock and Konstantin Seurer.
> This Vulkan testcase waits for an event that never occurs,
> effectively a WAIT_REG_MEM packet that intentionally hangs.
> IP block soft reset can resolve that hang and allow
> the rest of the system to move on and keep functioning
> without needing a full ASIC reset.
> 
> Tested on the following chips:
> 
> Bonaire (Radeon HD 7790)
> Hawaii (Radeon R9 390X)
> Kaveri (A10-7850K)
> 
> Signed-off-by: Timur Kristóf <timur.kristof@gmail.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 5 +++++
>   1 file changed, 5 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> index 825e3d7d5f0f..1a7a6962303e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> @@ -4434,6 +4434,11 @@ static int gfx_v7_0_sw_init(struct amdgpu_ip_block *ip_block)
>   	adev->gfx.compute_supported_reset =
>   		amdgpu_get_soft_full_reset_mask(&adev->gfx.compute_ring[0]);
>   
> +	if (!amdgpu_sriov_vf(adev) && !adev->debug_disable_ip_block_soft_reset) {
> +		adev->gfx.compute_supported_reset |= AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET;
> +		adev->gfx.gfx_supported_reset |= AMDGPU_RESET_TYPE_IP_BLOCK_SOFT_RESET;
> +	}
> +
>   	return r;
>   }
>   

Reviewed-by: Tvrtko Ursulin <tvrtko.ursulin@igalia.com>

Regards,

Tvrtko