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 <[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.
>
>> 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,
>
>
>
>
