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

["Christian König" <christian.koenig@amd.com>]

On 10/9/26 23:23, Felix Kuehling wrote:
> 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...).
> 
> If I understand it correctly, you fixed this by tracking two different fences in amdgpu_vm: one that updates the sequence number, and one that signals when the TLB flush completes.
> 
> 
>>
>> Signed-off-by: Christian König <christian.koenig@amd.com>
> 
> Why was cursoragent@cursor.com copied on this email?

It was CCed on Davids original mail as well.

Regards,
Christian.

> Anyway, the patch looks good to me.
> 
> Reviewed-by: Felix Kuehling <felix.kuehling@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,
>> +                          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);