* [PATCH] drm/amdgpu: fix TLB invalidation fence handling
@ 2026-10-08 12:42 Christian König
2026-10-09 21:23 ` Felix Kuehling
0 siblings, 1 reply; 3+ messages in thread
From: Christian König @ 2026-10-08 12:42 UTC (permalink / raw)
To: David.YatSin, amd-gfx, Sunil.Khatri, natalie.vock, Bincy.Mani,
cursoragent
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,
+ 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);
--
2.43.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/amdgpu: fix TLB invalidation fence handling
2026-10-08 12:42 [PATCH] drm/amdgpu: fix TLB invalidation fence handling Christian König
@ 2026-10-09 21:23 ` Felix Kuehling
2026-10-14 7:50 ` Christian König
0 siblings, 1 reply; 3+ messages in thread
From: Felix Kuehling @ 2026-10-09 21:23 UTC (permalink / raw)
To: christian.koenig, David.YatSin, amd-gfx, Sunil.Khatri,
natalie.vock, Bincy.Mani
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? 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(¶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);
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] drm/amdgpu: fix TLB invalidation fence handling
2026-10-09 21:23 ` Felix Kuehling
@ 2026-10-14 7:50 ` Christian König
0 siblings, 0 replies; 3+ messages in thread
From: Christian König @ 2026-10-14 7:50 UTC (permalink / raw)
To: Felix Kuehling, David.YatSin, amd-gfx, Sunil.Khatri, natalie.vock,
Bincy.Mani
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(¶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);
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-14 7:50 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-10-08 12:42 [PATCH] drm/amdgpu: fix TLB invalidation fence handling Christian König
2026-10-09 21:23 ` Felix Kuehling
2026-10-14 7:50 ` Christian König
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.