Re: [PATCH] drm/amdgpu: fix TLB invalidation fence handling

[Benjamin Cheng <benjamin.cheng@amd.com>]

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 <christian.koenig@amd.com>
> ---
>  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(&params, fence, tlb_cb);
> +		amdgpu_vm_tlb_flush(&params, *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);