Re: [PATCH] Revert "drm/amdgpu: Implement insert_end for VCE 3"

On Fri, Aug 14, 2026 at 11:13 AM David Rosca <[email protected]> wrote:
>
>
> On 8/14/26 17:08, Alex Deucher wrote:
> > On Fri, Aug 14, 2026 at 11:05 AM David Rosca <[email protected]> wrote:
> >> This doesn't solve the issue, VCE still hangs in some cases.
> >>
> >> 4959138b6461 ("drm/amdgpu: handle GDS and SPM without a VM fence")
> >> fixes the regression, originally introduced in
> >> 83a8dee2bdfb ("drm/amdgpu: always emit the job vm fence").
> > This doesn't make sense to me.  GDS and SPM are not supported on VCN
> > in the first place so they would never be emitted for VCN.
>
> The issue was the extra fence when only pipeline sync was needed.
> With 4959138b6461 ("drm/amdgpu: handle GDS and SPM without a VM fence")
> the fence is not emitted in that case.

If only the pipeline sync is needed that should also be moved up to
the caller as well.  I'll send a patch to fix that.

Alex

>
> David
>
> >
> > Alex
> >
> >> This reverts commit 7591335141a06d5b176510054576d9c64519f348.
> >>
> >> Signed-off-by: David Rosca <[email protected]>
> >> ---
> >>   drivers/gpu/drm/amd/amdgpu/vce_v3_0.c | 26 ++++----------------------
> >>   1 file changed, 4 insertions(+), 22 deletions(-)
> >>
> >> diff --git a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >> index a9497e2e07f7..9f4e88440c0a 100644
> >> --- a/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >> +++ b/drivers/gpu/drm/amd/amdgpu/vce_v3_0.c
> >> @@ -809,23 +809,6 @@ static void vce_v3_0_ring_emit_ib(struct amdgpu_ring *ring,
> >>          amdgpu_ring_write(ring, ib->length_dw);
> >>   }
> >>
> >> -static void vce_v3_0_ring_emit_fence(struct amdgpu_ring *ring, u64 addr,
> >> -                       u64 seq, unsigned flags)
> >> -{
> >> -       WARN_ON(flags & AMDGPU_FENCE_FLAG_64BIT);
> >> -
> >> -       amdgpu_ring_write(ring, VCE_CMD_FENCE);
> >> -       amdgpu_ring_write(ring, addr);
> >> -       amdgpu_ring_write(ring, upper_32_bits(addr));
> >> -       amdgpu_ring_write(ring, seq);
> >> -       amdgpu_ring_write(ring, VCE_CMD_TRAP);
> >> -}
> >> -
> >> -static void vce_v3_0_ring_insert_end(struct amdgpu_ring *ring)
> >> -{
> >> -       amdgpu_ring_write(ring, VCE_CMD_END);
> >> -}
> >> -
> >>   static void vce_v3_0_emit_vm_flush(struct amdgpu_ring *ring,
> >>                                     unsigned int vmid, uint64_t pd_addr)
> >>   {
> >> @@ -835,6 +818,7 @@ static void vce_v3_0_emit_vm_flush(struct amdgpu_ring *ring,
> >>
> >>          amdgpu_ring_write(ring, VCE_CMD_FLUSH_TLB);
> >>          amdgpu_ring_write(ring, vmid);
> >> +       amdgpu_ring_write(ring, VCE_CMD_END);
> >>   }
> >>
> >>   static void vce_v3_0_emit_pipeline_sync(struct amdgpu_ring *ring)
> >> @@ -900,19 +884,17 @@ static const struct amdgpu_ring_funcs vce_v3_0_ring_vm_funcs = {
> >>          .set_wptr = vce_v3_0_ring_set_wptr,
> >>          .patch_cs_in_place = amdgpu_vce_ring_parse_cs_vm,
> >>          .emit_frame_size =
> >> -               5 + /* vce_v3_0_emit_vm_flush */
> >> +               6 + /* vce_v3_0_emit_vm_flush */
> >>                  4 + /* vce_v3_0_emit_pipeline_sync */
> >> -               5 + 5 + /* vce_v3_0_ring_emit_fence x2 vm fence */
> >> -               1, /* vce_v3_0_ring_insert_end */
> >> +               6 + 6, /* amdgpu_vce_ring_emit_fence x2 vm fence */
> >>          .emit_ib_size = 5, /* vce_v3_0_ring_emit_ib */
> >>          .emit_ib = vce_v3_0_ring_emit_ib,
> >>          .emit_vm_flush = vce_v3_0_emit_vm_flush,
> >>          .emit_pipeline_sync = vce_v3_0_emit_pipeline_sync,
> >> -       .emit_fence = vce_v3_0_ring_emit_fence,
> >> +       .emit_fence = amdgpu_vce_ring_emit_fence,
> >>          .test_ring = amdgpu_vce_ring_test_ring,
> >>          .test_ib = amdgpu_vce_ring_test_ib,
> >>          .insert_nop = amdgpu_ring_insert_nop,
> >> -       .insert_end = vce_v3_0_ring_insert_end,
> >>          .pad_ib = amdgpu_ring_generic_pad_ib,
> >>          .begin_use = amdgpu_vce_ring_begin_use,
> >>          .end_use = amdgpu_vce_ring_end_use,
> >> --
> >> 2.43.0
> >>