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,
