Re: [PATCH 3/3] drm/amdgpu: handle GDS and SPM without a VM fence

On Thu, Aug 13, 2026 at 9:40 AM Christian König
<[email protected]> wrote:
>
> On 7/13/26 20:24, Alex Deucher wrote:
> > If we end up emitting a VM fence keep GDS and SPM
> > associated with that fence.  If not, emit them as
> > part of the IB fence.
>
> I've just stumbled over that this was pushed into amd-staging-drm-next.
>
> As far as I can see the patch doesn't make any sense at all. Why should we do that?

For per queue resets.  The reemit state is tied to the fence.  If we
don't emit the vm fence, and the queue gets reset, we don't reemit the
stuff done in amdgpu_vm_flush() since there is no fence to tie it to.
If there is no vm fence, then we move that state to the ib fence so it
gets properly reemitted.  Patch 1 in this series always emits the vm
fence, but you said that could have performance impacts so this patch
restores that behavior where we can emit the vm_flush state without a
vm fence.

Alex

>
> Regards,
> Christian.
>
> >
> > Signed-off-by: Alex Deucher <[email protected]>
> > ---
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c | 14 +++++++-
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 47 ++++++++++++++++++--------
> >  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |  4 ++-
> >  3 files changed, 48 insertions(+), 17 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > index f1ed4a436f5b4..d439c68eed9de 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ib.c
> > @@ -131,6 +131,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
> >       struct amdgpu_fence *af;
> >       struct amdgpu_fence *vm_af;
> >       bool need_ctx_switch;
> > +     bool emit_spm_needed = false;
> > +     bool emit_gds_needed = false;
> >       struct amdgpu_vm *vm;
> >       uint64_t fence_ctx;
> >       uint32_t status = 0, alloc_size;
> > @@ -220,7 +222,8 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
> >               vm_af = job->hw_vm_fence;
> >               /* VM sequence */
> >               vm_af->ib_wptr = ring->wptr;
> > -             amdgpu_vm_flush(ring, job, need_pipe_sync);
> > +             amdgpu_vm_flush(ring, job, need_pipe_sync, &emit_spm_needed,
> > +                             &emit_gds_needed);
> >               vm_af->ib_dw_size =
> >                       amdgpu_ring_get_dw_distance(ring, vm_af->ib_wptr, ring->wptr);
> >       }
> > @@ -232,6 +235,15 @@ int amdgpu_ib_schedule(struct amdgpu_ring *ring, unsigned int num_ibs,
> >       if (ring->funcs->insert_start)
> >               ring->funcs->insert_start(ring);
> >
> > +     if (emit_spm_needed)
> > +             adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id, ring, job->vmid);
> > +
> > +     if (emit_gds_needed)
> > +             amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> > +                                         job->gds_size, job->gws_base,
> > +                                         job->gws_size, job->oa_base,
> > +                                         job->oa_size);
> > +
> >       if ((ib->flags & AMDGPU_IB_FLAG_EMIT_MEM_SYNC) && ring->funcs->emit_mem_sync)
> >               ring->funcs->emit_mem_sync(ring);
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index aec8468634535..756810c606d80 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -766,18 +766,22 @@ bool amdgpu_vm_need_pipeline_sync(struct amdgpu_ring *ring,
> >   * @ring: ring to use for flush
> >   * @job:  related job
> >   * @need_pipe_sync: is pipe sync needed
> > + * @emit_spm_needed: does the caller need to emit spm
> > + * @emit_gds_needed: does the caller need to emit gds
> >   *
> >   * Emit a VM flush when it is necessary.
> >   */
> >  void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> > -                  bool need_pipe_sync)
> > +                  bool need_pipe_sync, bool *emit_spm_needed,
> > +                  bool *emit_gds_needed)
> >  {
> >       struct amdgpu_device *adev = ring->adev;
> >       struct amdgpu_isolation *isolation = &adev->isolation[ring->xcp_id];
> >       unsigned vmhub = ring->vm_hub;
> >       struct amdgpu_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub];
> >       struct amdgpu_vmid *id = &id_mgr->ids[job->vmid];
> > -     bool spm_update_needed = job->spm_update_needed;
> > +     bool spm_update_needed = adev->gfx.rlc.funcs->update_spm_vmid &&
> > +             job->spm_update_needed;
> >       bool gds_switch_needed = ring->funcs->emit_gds_switch &&
> >               job->gds_switch_needed;
> >       bool vm_flush_needed = job->vm_needs_flush;
> > @@ -785,6 +789,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> >       bool pasid_mapping_needed = false;
> >       struct dma_fence *fence = NULL;
> >       unsigned int patch = 0;
> > +     bool emit_fence;
> >
> >       if (amdgpu_vmid_had_gpu_reset(adev, id)) {
> >               gds_switch_needed = true;
> > @@ -800,6 +805,7 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> >       mutex_unlock(&id_mgr->lock);
> >
> >       gds_switch_needed &= !!ring->funcs->emit_gds_switch;
> > +     spm_update_needed &= !!adev->gfx.rlc.funcs->update_spm_vmid;
> >       vm_flush_needed &= !!ring->funcs->emit_vm_flush  &&
> >                       job->vm_pd_addr != AMDGPU_BO_INVALID_OFFSET;
> >       pasid_mapping_needed &= adev->gmc.gmc_funcs->emit_pasid_mapping &&
> > @@ -810,6 +816,17 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> >               ring->funcs->emit_cleaner_shader && job->base.s_fence &&
> >               &job->base.s_fence->scheduled == isolation->spearhead;
> >
> > +     emit_fence = !!(vm_flush_needed || pasid_mapping_needed ||
> > +                     cleaner_shader_needed);
> > +
> > +     *emit_spm_needed = spm_update_needed;
> > +     if (spm_update_needed && emit_fence)
> > +             *emit_spm_needed = false;
> > +
> > +     *emit_gds_needed = gds_switch_needed;
> > +     if (gds_switch_needed && emit_fence)
> > +             *emit_gds_needed = false;
> > +
> >       if (!vm_flush_needed && !gds_switch_needed && !need_pipe_sync &&
> >           !cleaner_shader_needed && !spm_update_needed)
> >               return;
> > @@ -844,21 +861,21 @@ void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> >       if (pasid_mapping_needed)
> >               amdgpu_gmc_emit_pasid_mapping(ring, job->vmid, job->pasid);
> >
> > -     if (spm_update_needed && adev->gfx.rlc.funcs->update_spm_vmid)
> > -             adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id, ring, job->vmid);
> > +     if (emit_fence) {
> > +             if (spm_update_needed)
> > +                     adev->gfx.rlc.funcs->update_spm_vmid(adev, ring->xcc_id, ring, job->vmid);
> >
> > -     if (ring->funcs->emit_gds_switch &&
> > -         gds_switch_needed) {
> > -             amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> > -                                         job->gds_size, job->gws_base,
> > -                                         job->gws_size, job->oa_base,
> > -                                         job->oa_size);
> > -     }
> > +             if (gds_switch_needed)
> > +                     amdgpu_ring_emit_gds_switch(ring, job->vmid, job->gds_base,
> > +                                                 job->gds_size, job->gws_base,
> > +                                                 job->gws_size, job->oa_base,
> > +                                                 job->oa_size);
> >
> > -     amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > -     fence = &job->hw_vm_fence->base;
> > -     /* get a ref for the job */
> > -     dma_fence_get(fence);
> > +             amdgpu_fence_emit(ring, job->hw_vm_fence, 0);
> > +             fence = &job->hw_vm_fence->base;
> > +             /* get a ref for the job */
> > +             dma_fence_get(fence);
> > +     }
> >
> >       if (vm_flush_needed) {
> >               mutex_lock(&id_mgr->lock);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 939f639cd8bfd..f82572b71c5ad 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -515,7 +515,9 @@ int amdgpu_vm_validate(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> >                      struct ww_acquire_ctx *ticket,
> >                      int (*callback)(void *p, struct amdgpu_bo *bo),
> >                      void *param);
> > -void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job, bool need_pipe_sync);
> > +void amdgpu_vm_flush(struct amdgpu_ring *ring, struct amdgpu_job *job,
> > +                  bool need_pipe_sync, bool *emit_spm_needed,
> > +                  bool *emit_gds_needed);
> >  int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> >                         struct amdgpu_vm *vm, bool immediate);
> >  int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
>