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

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?

> 
> 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));

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?

Also, amdgpu_ib_schedule only emits amdgpu_ring_emit_switch_buffer if 
there is a job. Currently it is always emitted.

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.

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,