AMD General Hi, Christian.
The lock of each fence indeed never changes. Each PT update fence has its own lock. The issue is in the fence locking process. When a fence signals, dma_fence_signal() holds the fence's lock and calls amdgpu_vm_tlb_seq_cb(), which increments vm->tlb_seq. amdgpu_vm_tlb_seq() waits on the lock of vm->last_tlb_flush until amdgpu_vm_tlb_seq_cb() completes. The lock of vm->last_tlb_flush ensures that amdgpu_vm_tlb_seq() returns the incremented vm->tlb_seq. This is the normal flow. But vm->last_tlb_flush may already point to a new fence: amdgpu_vm_tlb_flush() replaces it as soon as a new PT update is submitted, usually before the old fence signals. amdgpu_vm_tlb_seq() then takes the new fence's lock, which is not held while the old fence's callback runs, so it is not blocked and returns vm->tlb_seq before it is incremented. Finally, updates = amdgpu_vm_tlb_seq() misses the increment, the condition (*id)->flushed_updates < updates is not satisfied, and needs_flush is false, so the job reuses the VMID without a VM flush. Please let me know if anything is unclear or if I'm missing something. Thanks, Chong. -----Original Message----- From: Koenig, Christian <[email protected]> Sent: Monday, November 9, 2026 10:08 PM To: Li, Chong(Alan) <[email protected]>; [email protected] Subject: Re: [PATCH] drm/amdgpu: fix page fault by not skipping VM flush before tlb_seq update On 11/9/26 15:06, chong li wrote: > amdgpu_vm_tlb_seq() waits for the tlb_seq callback by taking the lock > of > vm->last_tlb_flush. Newer page table updates replace > vm->vm->last_tlb_flush > immediately, without waiting for older callbacks. A job scheduled > after an older fence signals but before its callback finishes takes > the newer fence's lock, while the callback runs under the older fence's own > lock. > The job misses that tlb_seq increment and can reuse a VMID whose TLB > was last flushed before the older page table update completed, without > flushing the TLB again. That's not even remotely correct. All fences use the same lock, that's why this works correctly. Regards, Christian. > > how to fix: > Count the flush-requiring page table updates whose tlb_seq callback > has not run yet, and request a VM flush when a job reuses a VMID while > this count is non-zero. > > Signed-off-by: chong li <[email protected]> > --- > drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c | 10 ++++++++-- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 12 +++++++++++- > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 10 ++++++++++ > 3 files changed, 29 insertions(+), 3 deletions(-) > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c > index 9c879ac68352..b9e058da162b 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_ids.c > @@ -393,7 +393,8 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm, > unsigned vmhub = ring->vm_hub; > uint64_t fence_context = adev->fence_context + ring->idx; > bool needs_flush = vm->use_cpu_for_update; > - uint64_t updates = amdgpu_vm_tlb_seq(vm); > + bool tlb_pending; > + uint64_t updates = amdgpu_vm_tlb_seq_pending(vm, &tlb_pending); > int r; > > *id = vm->reserved_vmid[vmhub]; > @@ -405,6 +406,8 @@ static int amdgpu_vmid_grab_reserved(struct amdgpu_vm *vm, > !dma_fence_is_signaled((*id)->last_flush))) > needs_flush = true; > > + needs_flush |= tlb_pending; > + > if ((*id)->owner != vm->immediate.fence_context || > (!adev->vm_manager.concurrent_flush && needs_flush)) { > struct dma_fence *tmp; > @@ -456,7 +459,8 @@ static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm, > unsigned vmhub = ring->vm_hub; > struct amdgpu_kq_vmid_mgr *id_mgr = &adev->vm_manager.id_mgr[vmhub]; > uint64_t fence_context = adev->fence_context + ring->idx; > - uint64_t updates = amdgpu_vm_tlb_seq(vm); > + bool tlb_pending; > + uint64_t updates = amdgpu_vm_tlb_seq_pending(vm, &tlb_pending); > int r; > > job->vm_needs_flush = vm->use_cpu_for_update; @@ -480,6 +484,8 @@ > static int amdgpu_vmid_grab_used(struct amdgpu_vm *vm, > if ((*id)->flushed_updates < updates) > needs_flush = true; > > + needs_flush |= tlb_pending; > + > if (needs_flush && !adev->vm_manager.concurrent_flush) > continue; > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > index 2ba33212d507..6d15fd001a42 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c > @@ -1095,6 +1095,9 @@ static void amdgpu_vm_tlb_seq_cb(struct > dma_fence *fence, > > tlb_cb = container_of(cb, typeof(*tlb_cb), cb); > atomic64_inc(&tlb_cb->vm->tlb_seq); > + /* Pairs with atomic_read_acquire() in amdgpu_vm_tlb_seq_pending() */ > + smp_mb__before_atomic(); > + atomic_dec(&tlb_cb->vm->tlb_pending); > kfree(tlb_cb); > } > > @@ -1281,9 +1284,16 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, > struct amdgpu_vm *vm, > start = tmp; > } > > + /* Must be visible before the update fence can signal */ > + if (params.needs_flush) > + atomic_inc(&vm->tlb_pending); > + > r = vm->update_funcs->commit(¶ms, fence); > - if (r) > + if (r) { > + if (params.needs_flush) > + atomic_dec(&vm->tlb_pending); > goto error_free; > + } > > if (params.needs_flush) { > amdgpu_vm_tlb_flush(¶ms, fence, tlb_cb); diff --git > a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > index d8e17f493f61..312fd0d268ce 100644 > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h > @@ -337,6 +337,8 @@ struct amdgpu_vm { > > /* Last finished delayed update */ > atomic64_t tlb_seq; > + /* Flush updates whose tlb_seq callback has not run yet */ > + atomic_t tlb_pending; > struct dma_fence *last_tlb_flush; > atomic64_t kfd_last_flushed_seq; > uint64_t tlb_fence_context; > @@ -578,6 +580,14 @@ static inline uint64_t amdgpu_vm_tlb_seq(struct > amdgpu_vm *vm) > return atomic64_read(&vm->tlb_seq); > } > > +/* Read tlb_pending first; pairs with the barrier in > +amdgpu_vm_tlb_seq_cb() */ static inline uint64_t > amdgpu_vm_tlb_seq_pending(struct amdgpu_vm *vm, > + bool *pending) > +{ > + *pending = atomic_read_acquire(&vm->tlb_pending) != 0; > + return amdgpu_vm_tlb_seq(vm); > +} > + > void amdgpu_vm_update_fault_cache(struct amdgpu_device *adev, > unsigned int pasid, > uint64_t addr,
