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

["Timur Kristóf" <timur.kristof@gmail.com>]

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,