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

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