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

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 <[email protected]>
>>> ---
>>>
>>>    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;
> 
> 
> 
>