Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets
On 7/15/26 12: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.)
Absolutely correct, yes.
We could eliminate the whole switch buffer packet if we completely deprecate
any CE engine support.
There was just exactly one Mesa release which made use of the CE engine but
I've disable the CE support and put it behind a debug option quite a while ago.
If nobody complains in the near term I think we can make nails with heads and
just remove all the hacks we have for this.
Regards,
Christian.
>
>
>>
>>> Signed-off-by: Timur Kristóf
>>> ---
>>>
>>> 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
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
---
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
Re: [PATCH 5/9] drm/amdgpu/gfx7: Fixup emitting SWITCH_BUFFER packets
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
> > ---
> >
> > 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,
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
---
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, 0x);
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 +
