On 2026-10-08 08:42, Christian König wrote:
> After each unmap it might be necessary to invalidate the TLB to flush out
> VM changes to the HW and make it visible to executing work.
>
> We tracked that as part of the vm->last_update fence, but as pointed out
> by David it can be that this is overwritten by PDE updates when we have a
> combined unmap/map operation.
>
> In that case userspace doesn't correctly wait for the TLB invalidation to
> finish leading to all kind of problems (sq_int, page faults etc...).
>
> Signed-off-by: Christian König <[email protected]>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 20 ++++++----
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 39 +++++++++----------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 13 ++++---
> .../gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c | 21 +++++++---
> 4 files changed, 55 insertions(+), 38 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 374f1322c3ee4..606725d0eb155 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -843,18 +843,24 @@ amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
> /*
> * For MAP/REPLACE of non per-VM BOs we need to sync to both the
> * bo_va->last_pt_update and vm->last_update or otherwise we
> - * potentially miss the PDE updates.
> + * potentially miss the PDE updates or TLB invalidation.
> */
> fence = dma_fence_unwrap_merge(vm->last_update,
> + vm->last_tlb_inv,
> bo_va->last_pt_update);
> - if (!fence) {
> - /* As fallback in OOM situations */
> - dma_fence_wait(vm->last_update, false);
> + if (!fence)
> dma_fence_wait(bo_va->last_pt_update, false);
> - fence = dma_fence_get_stub();
> - }
> +
> } else {
> - fence = dma_fence_get(vm->last_update);
> + fence = dma_fence_unwrap_merge(vm->last_update,
> + vm->last_tlb_inv);
> + }
> +
> + if (!fence) {
> + /* As fallback in OOM situations */
> + dma_fence_wait(vm->last_update, false);
> + dma_fence_wait(vm->last_tlb_inv, false);
> + fence = dma_fence_get_stub();
> }
>
> return fence;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7d3659ef11e53..f124320345ec4 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1071,21 +1071,16 @@ static void amdgpu_vm_tlb_seq_cb(struct dma_fence
> *fence,
> */
> static void
> amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
> - struct dma_fence **fence,
> + struct dma_fence *fence,
> struct amdgpu_vm_tlb_seq_struct *tlb_cb)
> {
> struct amdgpu_vm *vm = params->vm;
>
> tlb_cb->vm = vm;
> - if (!fence || !*fence) {
> - amdgpu_vm_tlb_seq_cb(NULL, &tlb_cb->cb);
> - return;
> - }
> -
> - if (!dma_fence_add_callback(*fence, &tlb_cb->cb,
> - amdgpu_vm_tlb_seq_cb)) {
> - dma_fence_put(vm->last_tlb_flush);
> - vm->last_tlb_flush = dma_fence_get(*fence);
> + if (!fence || !dma_fence_add_callback(fence, &tlb_cb->cb,
Shouldn't this be if (fence || !dma_fence_add_callback(...))? Otherwise the
tlb_seq never gets incremented.
> + amdgpu_vm_tlb_seq_cb)) {
> + dma_fence_put(vm->last_seq_flush);
> + vm->last_seq_flush = dma_fence_get(fence);
> } else {
> amdgpu_vm_tlb_seq_cb(NULL, &tlb_cb->cb);
> }
> @@ -1095,11 +1090,12 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params
> *params,
> * sort out the issues with KIQ/MES TLB invalidation timeouts.
> */
> if (!params->unlocked && vm->need_tlb_fence) {
> - amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
> + amdgpu_vm_tlb_fence_create(params->adev, vm, fence,
> + &vm->last_tlb_inv);
>
> /* Makes sure no PD/PT is freed before the flush */
> - dma_resv_add_fence(vm->root.bo->tbo.base.resv, *fence,
> - DMA_RESV_USAGE_BOOKKEEP);
> + dma_resv_add_fence(vm->root.bo->tbo.base.resv,
> + vm->last_tlb_inv, DMA_RESV_USAGE_BOOKKEEP);
> }
> }
>
> @@ -1248,7 +1244,7 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev,
> struct amdgpu_vm *vm,
> goto error_free;
>
> if (params.needs_flush) {
> - amdgpu_vm_tlb_flush(¶ms, fence, tlb_cb);
> + amdgpu_vm_tlb_flush(¶ms, *fence, tlb_cb);
> tlb_cb = NULL;
> }
>
> @@ -2647,7 +2643,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
>
> vm->last_update = dma_fence_get_stub();
> vm->last_unlocked = dma_fence_get_stub();
> - vm->last_tlb_flush = dma_fence_get_stub();
> + vm->last_seq_flush = dma_fence_get_stub();
> + vm->last_tlb_inv = dma_fence_get_stub();
> vm->generation = amdgpu_vm_generation(adev, NULL);
>
> mutex_init(&vm->eviction_lock);
> @@ -2691,7 +2688,8 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct
> amdgpu_vm *vm,
>
> error_free_delayed:
> dma_fence_put(vm->last_update);
> - dma_fence_put(vm->last_tlb_flush);
> + dma_fence_put(vm->last_seq_flush);
> + dma_fence_put(vm->last_tlb_inv);
> dma_fence_put(vm->last_unlocked);
> ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
> amdgpu_vm_fini_entities(vm);
> @@ -2834,11 +2832,12 @@ void amdgpu_vm_fini(struct amdgpu_device *adev,
> struct amdgpu_vm *vm)
> amdgpu_bo_reserve(root, true);
> dma_fence_wait(vm->last_unlocked, false);
> dma_fence_put(vm->last_unlocked);
> - dma_fence_wait(vm->last_tlb_flush, false);
> + dma_fence_wait(vm->last_seq_flush, false);
> /* Make sure that all fence callbacks have completed */
> - dma_fence_lock_irqsave(vm->last_tlb_flush, flags);
> - dma_fence_unlock_irqrestore(vm->last_tlb_flush, flags);
> - dma_fence_put(vm->last_tlb_flush);
> + dma_fence_lock_irqsave(vm->last_seq_flush, flags);
> + dma_fence_unlock_irqrestore(vm->last_seq_flush, flags);
> + dma_fence_put(vm->last_seq_flush);
> + dma_fence_put(vm->last_tlb_inv);
>
> list_for_each_entry_safe(mapping, tmp, &vm->freed, list) {
> if (mapping->flags & AMDGPU_VM_PAGE_PRT && prt_fini_needed) {
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 8e478d3bda1ac..713a597027bbe 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -328,11 +328,12 @@ struct amdgpu_vm {
> struct drm_sched_entity immediate;
> struct drm_sched_entity delayed;
>
> - /* Last finished delayed update */
> + /* Last TLB seq/fence */
> atomic64_t tlb_seq;
> - struct dma_fence *last_tlb_flush;
> + struct dma_fence *last_seq_flush;
> atomic64_t kfd_last_flushed_seq;
> uint64_t tlb_fence_context;
> + struct dma_fence *last_tlb_inv;
>
> /* How many times we had to re-generate the page tables */
> uint64_t generation;
> @@ -561,7 +562,7 @@ static inline uint64_t amdgpu_vm_tlb_seq(struct amdgpu_vm
> *vm)
> * sure that the dma_fence structure isn't freed up.
> */
> rcu_read_lock();
> - lock = dma_fence_spinlock(vm->last_tlb_flush);
> + lock = dma_fence_spinlock(vm->last_seq_flush);
> rcu_read_unlock();
>
> spin_lock_irqsave(lock, flags);
> @@ -576,9 +577,9 @@ void amdgpu_vm_update_fault_cache(struct amdgpu_device
> *adev,
> uint32_t status,
> unsigned int vmhub);
> void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
> - struct amdgpu_vm *vm,
> - struct dma_fence **fence);
> -
> + struct amdgpu_vm *vm,
> + struct dma_fence *dep,
> + struct dma_fence **fence);
> void amdgpu_vm_print_task_info(struct amdgpu_device *adev,
> struct amdgpu_task_info *task_info);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> index b0002c8983861..c91417e4df4ed 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_tlb_fence.c
> @@ -75,7 +75,19 @@ static const struct dma_fence_ops amdgpu_tlb_fence_ops = {
> .get_timeline_name = amdgpu_tlb_fence_get_timeline_name
> };
>
> -void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev, struct amdgpu_vm
> *vm,
> +/**
> + * amdgpu_vm_tlb_fence_create - create a TLB invalidation fence
> + * @adev: the amdgpu device
> + * @vm: the vm to use
> + * @dep: the dependency to wait for
> + * @fence: the resulting fence
> + *
> + * Creates a TLB invalidation fence which signals when all the VMIDs
> assigned to
> + * the PASID of the VM are invalidated.
> + */
> +void amdgpu_vm_tlb_fence_create(struct amdgpu_device *adev,
> + struct amdgpu_vm *vm,
> + struct dma_fence *dep,
> struct dma_fence **fence)
> {
> struct amdgpu_tlb_fence *f;
> @@ -86,16 +98,15 @@ void amdgpu_vm_tlb_fence_create(struct amdgpu_device
> *adev, struct amdgpu_vm *vm
> * We can't fail since the PDEs and PTEs are already updated, so
> * just block for the dependency and execute the TLB flush
> */
> - if (*fence)
> - dma_fence_wait(*fence, false);
> + if (dep)
> + dma_fence_wait(dep, false);
>
> amdgpu_gmc_flush_gpu_tlb_pasid(adev, vm->pasid, 2, true, 0);
> - *fence = dma_fence_get_stub();
> return;
> }
>
> f->adev = adev;
> - f->dependency = *fence;
> + f->dependency = dma_fence_get(dep);
> f->pasid = vm->pasid;
> INIT_WORK(&f->work, amdgpu_tlb_fence_work);
> spin_lock_init(&f->lock);
