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->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,
