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,
