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

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

Right, thanks!

> 
>> Is it okay to call the function mqd if the registers are hqd and is v7
>> or v8 (which calls it hqd) more correct? Not saying either way, just
>> observing a curiosity.
> 
> My best guess is that it's just that they used a different naming convention
> and forgot to update the older code.
> 
>>
>>>    }
>>>    
>>>    static void gfx_v7_0_mqd_init(struct amdgpu_device *adev,
>>>
>>> @@ -2964,31 +2946,42 @@ static int gfx_v7_0_mqd_commit(struct
>>> amdgpu_device *adev, struct cik_mqd *mqd)>
>>>    static int gfx_v7_0_compute_queue_init(struct amdgpu_device *adev, int
>>>    ring_id) {
>>>
>>> -	int r;
>>> -	u64 mqd_gpu_addr;
>>> -	struct cik_mqd *mqd;
>>>
>>>    	struct amdgpu_ring *ring = &adev->gfx.compute_ring[ring_id];
>>>
>>> -
>>> -	r = amdgpu_bo_create_reserved(adev, sizeof(struct cik_mqd),
> PAGE_SIZE,
>>> -				      AMDGPU_GEM_DOMAIN_GTT,
> &ring->mqd_obj,
>>> -				      &mqd_gpu_addr, (void
> **)&mqd);
>>> -	if (r) {
>>> -		dev_warn(adev->dev, "(%d) create MQD bo failed\n", r);
>>> -		return r;
>>> +	struct cik_mqd *mqd = ring->mqd_ptr;
>>> +	int mqd_idx = ring - &adev->gfx.compute_ring[0];
>>> +
>>> +	if (!amdgpu_in_reset(adev) && !adev->in_suspend) {
>>> +		memset((void *)mqd, 0, ring->mqd_size);
>>> +		mutex_lock(&adev->srbm_mutex);
>>> +		cik_srbm_select(adev, ring->me, ring->pipe, ring-
>> queue, 0);
>>> +		gfx_v7_0_mqd_init(adev, mqd, ring->mqd_gpu_addr, ring);
>>> +		gfx_v7_0_mqd_deactivate(adev, 1);
>>> +		gfx_v7_0_mqd_commit(adev, mqd);
>>> +		cik_srbm_select(adev, 0, 0, 0, 0);
>>> +		mutex_unlock(&adev->srbm_mutex);
>>> +
>>> +		if (adev->gfx.mec.mqd_backup[mqd_idx])
>>> +			memcpy(adev->gfx.mec.mqd_backup[mqd_idx],
> mqd, ring->mqd_size);
>>> +	} else {
>>> +		/* restore MQD to a clean status */
>>> +		if (adev->gfx.mec.mqd_backup[mqd_idx])
>>> +			memcpy(mqd, adev-
>> gfx.mec.mqd_backup[mqd_idx], ring->mqd_size);
>>> +
>>> +		/* Re-commit the restored backup */
>>> +		mutex_lock(&adev->srbm_mutex);
>>> +		cik_srbm_select(adev, ring->me, ring->pipe, ring-
>> queue, 0);
>>> +		gfx_v7_0_mqd_deactivate(adev, 2);
>>> +		gfx_v7_0_mqd_commit(adev, mqd);
>>> +		cik_srbm_select(adev, 0, 0, 0, 0);
>>> +		mutex_unlock(&adev->srbm_mutex);
>>> +
>>> +		/* reset ring buffer */
>>> +		ring->wptr = 0;
>>> +		atomic64_set((atomic64_t *)ring->wptr_cpu_addr, 0);
>>> +		atomic64_set((atomic64_t *)ring->rptr_cpu_addr, 0);
>>> +		amdgpu_ring_clear_ring(ring);
>>>
>>>    	}
>>>
>>> -	mutex_lock(&adev->srbm_mutex);
>>> -	cik_srbm_select(adev, ring->me, ring->pipe, ring->queue, 0);
>>> -
>>> -	gfx_v7_0_mqd_init(adev, mqd, mqd_gpu_addr, ring);
>>> -	gfx_v7_0_mqd_deactivate(adev);
>>> -	gfx_v7_0_mqd_commit(adev, mqd);
>>> -
>>> -	cik_srbm_select(adev, 0, 0, 0, 0);
>>> -	mutex_unlock(&adev->srbm_mutex);
>>> -
>>> -	amdgpu_bo_kunmap(ring->mqd_obj);
>>> -	amdgpu_bo_unreserve(ring->mqd_obj);
>>>
>>>    	return 0;
>>>    
>>>    }
>>
>> I think I can follow this - only the wptr and rptr reset is a bit
>> different than what v8 does it. Any specific reason? Gfx9 then reverts
>> back to a single ring->wptr = 0. I guess v8 is somehow special?
>>
>>> @@ -3020,10 +3013,8 @@ static int gfx_v7_0_cp_compute_resume(struct
>>> amdgpu_device *adev)>
>>>    	/* init the queues */
>>>    	for (i = 0; i < adev->gfx.num_compute_rings; i++) {
>>>    	
>>>    		r = gfx_v7_0_compute_queue_init(adev, i);
>>>
>>> -		if (r) {
>>> -			gfx_v7_0_cp_compute_fini(adev);
>>> +		if (r)
>>>
>>>    			return r;
>>>
>>> -		}
>>>
>>>    	}
>>>    	
>>>    	gfx_v7_0_cp_compute_enable(adev, true);
>>>
>>> @@ -4430,6 +4421,11 @@ static int gfx_v7_0_sw_init(struct amdgpu_ip_block
>>> *ip_block)>
>>>    		}
>>>    	
>>>    	}
>>>
>>> +	/* create MQD for all compute queues */
>>> +	r = amdgpu_gfx_mqd_sw_init(adev, sizeof(struct cik_mqd), 0);
>>> +	if (r)
>>> +		return r;
>>> +
>>>
>>>    	adev->gfx.ce_ram_size = 0x8000;
>>>    	
>>>    	gfx_v7_0_gpu_early_init(adev);
>>>
>>> @@ -4452,7 +4448,7 @@ static int gfx_v7_0_sw_fini(struct amdgpu_ip_block
>>> *ip_block)>
>>>    	for (i = 0; i < adev->gfx.num_compute_rings; i++)
>>>    	
>>>    		amdgpu_ring_fini(&adev->gfx.compute_ring[i]);
>>>
>>> -	gfx_v7_0_cp_compute_fini(adev);
>>> +	amdgpu_gfx_mqd_sw_fini(adev, 0);
>>>
>>>    	amdgpu_gfx_rlc_fini(adev);
>>>    	gfx_v7_0_mec_fini(adev);
>>>    	amdgpu_bo_free_kernel(&adev->gfx.rlc.clear_state_obj,
>>
>> I am assuming all this applies only to compute because gfx is single
>> instance on v7?
> 
> It applies only to compute because only compute has HQD/MQD
> on these hardware generations.
> 
>> Anyway, it looks plausible to me so assuming you were able to exercise
>> both paths
> 
> What do you mean by "both paths"?

The two paths in gfx_v7_0_compute_queue_init. But you must of had since 
one is the normal path and the other is on reset which you specifically 
add and test. LGTM in this case.

Reviewed-by: Tvrtko Ursulin <[email protected]>

Regards,

Tvrtko