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

On 2026. július 14., kedd 20:47:14 közép-európai nyári idő Tvrtko Ursulin 
wrote:
> On 13/07/2026 13:58, Timur Kristóf wrote:
> > Call amdgpu_gfx_mqd_sw_init()/_fini() on GFX7 to initialize and
> > finalize the MQD, just like GFX8 and newer; instead of doing
> > an ad-hoc BO allocation. This introduces the possibility of
> > doing an MQD backup instead of trying to reinitialize the
> > MQD every time.
> > 
> > This solves an issue with GFX IP block soft reset where
> > all compute rings would hang after the reset.
> > 
> > Signed-off-by: Timur Kristóf <[email protected]>
> > ---
> > 
> >   drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c | 106 +++++++++++++-------------
> >   1 file changed, 51 insertions(+), 55 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c index 65b8497ad5f0..9c4b3ac27e1f
> > 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/gfx_v7_0.c
> > @@ -2698,25 +2698,6 @@ static int
> > gfx_v7_0_cp_compute_load_microcode(struct amdgpu_device *adev)> 
> >   	return 0;
> >   
> >   }
> > 
> > -/**
> > - * gfx_v7_0_cp_compute_fini - stop the compute queues
> > - *
> > - * @adev: amdgpu_device pointer
> > - *
> > - * Stop the compute queues and tear down the driver queue
> > - * info.
> > - */
> > -static void gfx_v7_0_cp_compute_fini(struct amdgpu_device *adev)
> > -{
> > -	int i;
> > -
> > -	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
> > -		struct amdgpu_ring *ring = &adev->gfx.compute_ring[i];
> > -
> > -		amdgpu_bo_free_kernel(&ring->mqd_obj, NULL, NULL);
> > -	}
> > -}
> > -
> > 
> >   static void gfx_v7_0_mec_fini(struct amdgpu_device *adev)
> >   {
> >   
> >   	amdgpu_bo_free_kernel(&adev->gfx.mec.hpd_eop_obj, NULL, NULL);
> > 
> > @@ -2788,28 +2769,29 @@ static void gfx_v7_0_compute_pipe_init(struct
> > amdgpu_device *adev,> 
> >   	mutex_unlock(&adev->srbm_mutex);
> >   
> >   }
> > 
> > -static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev)
> > +static int gfx_v7_0_mqd_deactivate(struct amdgpu_device *adev, u32 req)
> > 
> >   {
> > 
> > -	int i;
> > +	int i, r = 0;
> > 
> >   	/* disable the queue if it's active */
> > 
> > -	if (RREG32(mmCP_HQD_ACTIVE) & 1) {
> > -		WREG32(mmCP_HQD_DEQUEUE_REQUEST, 1);
> > +	if (RREG32(mmCP_HQD_ACTIVE) & CP_HQD_ACTIVE__ACTIVE_MASK) {
> > +		WREG32_FIELD(CP_HQD_DEQUEUE_REQUEST, DEQUEUE_REQ, req);
> > 
> >   		for (i = 0; i < adev->usec_timeout; i++) {
> > 
> > -			if (!(RREG32(mmCP_HQD_ACTIVE) & 1))
> > +			if (!(RREG32(mmCP_HQD_ACTIVE) & 
CP_HQD_ACTIVE__ACTIVE_MASK))
> > 
> >   				break;
> >   			
> >   			udelay(1);
> >   		
> >   		}
> >   		
> >   		if (i == adev->usec_timeout)
> > 
> > -			return -ETIMEDOUT;
> > +			r = -ETIMEDOUT;
> > 
> > -		WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
> > -		WREG32(mmCP_HQD_PQ_RPTR, 0);
> > -		WREG32(mmCP_HQD_PQ_WPTR, 0);
> > 
> >   	}
> > 
> > -	return 0;
> > +	WREG32(mmCP_HQD_DEQUEUE_REQUEST, 0);
> > +	WREG32(mmCP_HQD_PQ_RPTR, 0);
> > +	WREG32(mmCP_HQD_PQ_WPTR, 0);
> > +
> > +	return r;
> 
> I can see this matches gfx_v8_0_deactivate_hqd. If I am not missing
> anything only to replace the hardcoded 1 with CP_HQD_ACTIVE__ACTIVE_MASK?

There are two changes here:
- Replacing the hardcoded "1" with the define from the register definition
- When it times out, still write the CP_HQD_ registers afterwards like gfx8

> 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