[PATCH] drm/amdgpu: wait for tlb flush before freeing BO

[PATCH] drm/amdgpu: wait for tlb flush before freeing BO

[Benjamin Cheng]

On gem object close, we previously only attached the fence of its own
clear_freed() call to the BO's resv. If an unmap of this BO happened
prior to the close call, clear_freed() would be invoked, leaving
vm->freed empty, and no meaningful fence to be attached to the resv on
close. Because of that the memory may be freed before the TLB flush is
complete, and give a running userq potential access to a stale
address translation.

Fix it by tracking the last TLB fence of each VM and attaching it to the
BO on close.

Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 6 ++++++
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  | 1 +
 3 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 8dbd7bb77305..7484c52e5850 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -360,6 +360,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 
 	amdgpu_vm_bo_del(adev, bo_va);
 	amdgpu_vm_bo_update_shared(bo);
+	if (!dma_fence_is_signaled(vm->last_tlb_flush_complete))
+		amdgpu_bo_fence(bo, vm->last_tlb_flush_complete, true);
 	if (!amdgpu_vm_ready(vm))
 		goto out_unlock;
 
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 7d3659ef11e5..c5411933a76c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1097,6 +1097,9 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
 	if (!params->unlocked && vm->need_tlb_fence) {
 		amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
 
+		dma_fence_put(vm->last_tlb_flush_complete);
+		vm->last_tlb_flush_complete = dma_fence_get(*fence);
+
 		/* 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);
@@ -2648,6 +2651,7 @@ 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_tlb_flush_complete = dma_fence_get_stub();
 	vm->generation = amdgpu_vm_generation(adev, NULL);
 
 	mutex_init(&vm->eviction_lock);
@@ -2693,6 +2697,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	dma_fence_put(vm->last_update);
 	dma_fence_put(vm->last_tlb_flush);
 	dma_fence_put(vm->last_unlocked);
+	dma_fence_put(vm->last_tlb_flush_complete);
 	ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
 	amdgpu_vm_fini_entities(vm);
 
@@ -2839,6 +2844,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
 	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_put(vm->last_tlb_flush_complete);
 
 	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 ddcdc7256ef0..b9932c5e066f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -331,6 +331,7 @@ struct amdgpu_vm {
 	/* Last finished delayed update */
 	atomic64_t		tlb_seq;
 	struct dma_fence	*last_tlb_flush;
+	struct dma_fence	*last_tlb_flush_complete;
 	atomic64_t		kfd_last_flushed_seq;
 	uint64_t		tlb_fence_context;
 
-- 
2.55.0

Re: [PATCH] drm/amdgpu: wait for tlb flush before freeing BO

[Christian König]

On 10/14/26 04:21, Benjamin Cheng wrote:
> On gem object close, we previously only attached the fence of its own
> clear_freed() call to the BO's resv. If an unmap of this BO happened
> prior to the close call, clear_freed() would be invoked, leaving
> vm->freed empty, and no meaningful fence to be attached to the resv on
> close. Because of that the memory may be freed before the TLB flush is
> complete, and give a running userq potential access to a stale
> address translation.

Good catch.

> 
> Fix it by tracking the last TLB fence of each VM and attaching it to the
> BO on close.
> 
> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 6 ++++++
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  | 1 +
>  3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 8dbd7bb77305..7484c52e5850 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -360,6 +360,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  
>  	amdgpu_vm_bo_del(adev, bo_va);
>  	amdgpu_vm_bo_update_shared(bo);
> +	if (!dma_fence_is_signaled(vm->last_tlb_flush_complete))

Drop that extra check, it is actually harmfull.

You also need to increase the amount of reserved fence slots.


> +		amdgpu_bo_fence(bo, vm->last_tlb_flush_complete, true);
>  	if (!amdgpu_vm_ready(vm))
>  		goto out_unlock;
>  
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 7d3659ef11e5..c5411933a76c 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1097,6 +1097,9 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
>  	if (!params->unlocked && vm->need_tlb_fence) {
>  		amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
>  
> +		dma_fence_put(vm->last_tlb_flush_complete);
> +		vm->last_tlb_flush_complete = dma_fence_get(*fence);

I already added that in another patch, looks like amd-staging-drm-next is not up to date.

Let me double check that.

Regards,
Christian.

> +
>  		/* 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);
> @@ -2648,6 +2651,7 @@ 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_tlb_flush_complete = dma_fence_get_stub();
>  	vm->generation = amdgpu_vm_generation(adev, NULL);
>  
>  	mutex_init(&vm->eviction_lock);
> @@ -2693,6 +2697,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	dma_fence_put(vm->last_update);
>  	dma_fence_put(vm->last_tlb_flush);
>  	dma_fence_put(vm->last_unlocked);
> +	dma_fence_put(vm->last_tlb_flush_complete);
>  	ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
>  	amdgpu_vm_fini_entities(vm);
>  
> @@ -2839,6 +2844,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  	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_put(vm->last_tlb_flush_complete);
>  
>  	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 ddcdc7256ef0..b9932c5e066f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -331,6 +331,7 @@ struct amdgpu_vm {
>  	/* Last finished delayed update */
>  	atomic64_t		tlb_seq;
>  	struct dma_fence	*last_tlb_flush;
> +	struct dma_fence	*last_tlb_flush_complete;
>  	atomic64_t		kfd_last_flushed_seq;
>  	uint64_t		tlb_fence_context;
>  

Re: [PATCH] drm/amdgpu: wait for tlb flush before freeing BO

[Benjamin Cheng]

On 2026-10-14 03:17, Christian König wrote:
>
> On 10/14/26 04:21, Benjamin Cheng wrote:
>> On gem object close, we previously only attached the fence of its own
>> clear_freed() call to the BO's resv. If an unmap of this BO happened
>> prior to the close call, clear_freed() would be invoked, leaving
>> vm->freed empty, and no meaningful fence to be attached to the resv on
>> close. Because of that the memory may be freed before the TLB flush is
>> complete, and give a running userq potential access to a stale
>> address translation.
> Good catch.
>
>> Fix it by tracking the last TLB fence of each VM and attaching it to the
>> BO on close.
>>
>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 6 ++++++
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  | 1 +
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 8dbd7bb77305..7484c52e5850 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -360,6 +360,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>  
>>  	amdgpu_vm_bo_del(adev, bo_va);
>>  	amdgpu_vm_bo_update_shared(bo);
>> +	if (!dma_fence_is_signaled(vm->last_tlb_flush_complete))
> Drop that extra check, it is actually harmfull.

I will drop it in the next revision, but could you explain why it's harmful? (Sorry, still somewhat new to dma_fence semantics).


>
> You also need to increase the amount of reserved fence slots.


amdgpu_bo_fence already does that.


>
>
>> +		amdgpu_bo_fence(bo, vm->last_tlb_flush_complete, true);
>>  	if (!amdgpu_vm_ready(vm))
>>  		goto out_unlock;
>>  
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 7d3659ef11e5..c5411933a76c 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -1097,6 +1097,9 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
>>  	if (!params->unlocked && vm->need_tlb_fence) {
>>  		amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
>>  
>> +		dma_fence_put(vm->last_tlb_flush_complete);
>> +		vm->last_tlb_flush_complete = dma_fence_get(*fence);
> I already added that in another patch, looks like amd-staging-drm-next is not up to date.


I guess it's last_tlb_inv from "drm/amdgpu: fix TLB invalidation fence handling"? I'll rebase against that and use it if there are no other concerns.


>
> Let me double check that.
>
> Regards,
> Christian.
>
>> +
>>  		/* 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);
>> @@ -2648,6 +2651,7 @@ 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_tlb_flush_complete = dma_fence_get_stub();
>>  	vm->generation = amdgpu_vm_generation(adev, NULL);
>>  
>>  	mutex_init(&vm->eviction_lock);
>> @@ -2693,6 +2697,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>  	dma_fence_put(vm->last_update);
>>  	dma_fence_put(vm->last_tlb_flush);
>>  	dma_fence_put(vm->last_unlocked);
>> +	dma_fence_put(vm->last_tlb_flush_complete);
>>  	ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
>>  	amdgpu_vm_fini_entities(vm);
>>  
>> @@ -2839,6 +2844,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>>  	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_put(vm->last_tlb_flush_complete);
>>  
>>  	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 ddcdc7256ef0..b9932c5e066f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -331,6 +331,7 @@ struct amdgpu_vm {
>>  	/* Last finished delayed update */
>>  	atomic64_t		tlb_seq;
>>  	struct dma_fence	*last_tlb_flush;
>> +	struct dma_fence	*last_tlb_flush_complete;
>>  	atomic64_t		kfd_last_flushed_seq;
>>  	uint64_t		tlb_fence_context;
>>  

Re: [PATCH] drm/amdgpu: wait for tlb flush before freeing BO

[Christian König]

On 10/14/26 14:46, Benjamin Cheng wrote:
> On 2026-10-14 03:17, Christian König wrote:
>>
>> On 10/14/26 04:21, Benjamin Cheng wrote:
>>> On gem object close, we previously only attached the fence of its own
>>> clear_freed() call to the BO's resv. If an unmap of this BO happened
>>> prior to the close call, clear_freed() would be invoked, leaving
>>> vm->freed empty, and no meaningful fence to be attached to the resv on
>>> close. Because of that the memory may be freed before the TLB flush is
>>> complete, and give a running userq potential access to a stale
>>> address translation.
>> Good catch.
>>
>>> Fix it by tracking the last TLB fence of each VM and attaching it to the
>>> BO on close.
>>>
>>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 2 ++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 6 ++++++
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  | 1 +
>>>  3 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 8dbd7bb77305..7484c52e5850 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -360,6 +360,8 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>>  
>>>  	amdgpu_vm_bo_del(adev, bo_va);
>>>  	amdgpu_vm_bo_update_shared(bo);
>>> +	if (!dma_fence_is_signaled(vm->last_tlb_flush_complete))
>> Drop that extra check, it is actually harmfull.
> 
> I will drop it in the next revision, but could you explain why it's harmful? (Sorry, still somewhat new to dma_fence semantics).

The dma_resv_add_fence() function can handle signaled fences perfectly fine, but when you skip that the code path is only exercised in rare cases.

>>
>> You also need to increase the amount of reserved fence slots.

AH, good point! Yeah don't do that.

The fence slots you need must be pre-allocated by drm_exec, amdgpu_bo_fence() is just an ancient wrapper we should probably remove.

Regards,
Christian.

> 
> 
> amdgpu_bo_fence already does that.
> 
> 
>>
>>
>>> +		amdgpu_bo_fence(bo, vm->last_tlb_flush_complete, true);
>>>  	if (!amdgpu_vm_ready(vm))
>>>  		goto out_unlock;
>>>  
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> index 7d3659ef11e5..c5411933a76c 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>> @@ -1097,6 +1097,9 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params *params,
>>>  	if (!params->unlocked && vm->need_tlb_fence) {
>>>  		amdgpu_vm_tlb_fence_create(params->adev, vm, fence);
>>>  
>>> +		dma_fence_put(vm->last_tlb_flush_complete);
>>> +		vm->last_tlb_flush_complete = dma_fence_get(*fence);
>> I already added that in another patch, looks like amd-staging-drm-next is not up to date.
> 
> 
> I guess it's last_tlb_inv from "drm/amdgpu: fix TLB invalidation fence handling"? I'll rebase against that and use it if there are no other concerns.
> 
> 
>>
>> Let me double check that.
>>
>> Regards,
>> Christian.
>>
>>> +
>>>  		/* 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);
>>> @@ -2648,6 +2651,7 @@ 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_tlb_flush_complete = dma_fence_get_stub();
>>>  	vm->generation = amdgpu_vm_generation(adev, NULL);
>>>  
>>>  	mutex_init(&vm->eviction_lock);
>>> @@ -2693,6 +2697,7 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>>>  	dma_fence_put(vm->last_update);
>>>  	dma_fence_put(vm->last_tlb_flush);
>>>  	dma_fence_put(vm->last_unlocked);
>>> +	dma_fence_put(vm->last_tlb_flush_complete);
>>>  	ttm_lru_bulk_move_fini(&adev->mman.bdev, &vm->lru_bulk_move);
>>>  	amdgpu_vm_fini_entities(vm);
>>>  
>>> @@ -2839,6 +2844,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>>>  	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_put(vm->last_tlb_flush_complete);
>>>  
>>>  	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 ddcdc7256ef0..b9932c5e066f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>>> @@ -331,6 +331,7 @@ struct amdgpu_vm {
>>>  	/* Last finished delayed update */
>>>  	atomic64_t		tlb_seq;
>>>  	struct dma_fence	*last_tlb_flush;
>>> +	struct dma_fence	*last_tlb_flush_complete;
>>>  	atomic64_t		kfd_last_flushed_seq;
>>>  	uint64_t		tlb_fence_context;
>>>  

[PATCH v2] drm/amdgpu: wait for tlb flush before freeing BO

[Benjamin Cheng]

On gem object close, we previously only attached the fence of its own
clear_freed() call to the BO's resv. If an unmap of this BO happened
prior to the close call, clear_freed() would be invoked, leaving
vm->freed empty, and no meaningful fence to be attached to the resv.
Because of that the memory may be freed before the TLB flush is
complete, and give a running userq potential access to a stale
address translation.

Fix this by attaching the last_tlb_inv fence to the BO's resv.

Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
v2:
- Rebase on "drm/amdgpu: fix TLB invalidation fence handling v4" to get
  last_tlb_inv
- Move the fence after the clear_freed call to make sure we wait for the
  right invalidation.
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 6137ab4211be..36b0bd85616f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 		dev_err(adev->dev, "failed to clear page "
 			"tables on GEM object close (%ld)\n", r);
 	if (r || !fence)
-		goto out_unlock;
+		goto out_tlb;
 
 	amdgpu_bo_fence(bo, fence, true);
 	dma_fence_put(fence);
 
+out_tlb:
+	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
+
 out_unlock:
 	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
 		dev_err(adev->dev, "leaking bo va (%ld)\n", r);
-- 
2.56.0

Re: [PATCH v2] drm/amdgpu: wait for tlb flush before freeing BO

[Christian König]

On 11/6/26 16:20, Benjamin Cheng wrote:
> On gem object close, we previously only attached the fence of its own
> clear_freed() call to the BO's resv. If an unmap of this BO happened
> prior to the close call, clear_freed() would be invoked, leaving
> vm->freed empty, and no meaningful fence to be attached to the resv.
> Because of that the memory may be freed before the TLB flush is
> complete, and give a running userq potential access to a stale
> address translation.
> 
> Fix this by attaching the last_tlb_inv fence to the BO's resv.
> 
> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
> ---
> v2:
> - Rebase on "drm/amdgpu: fix TLB invalidation fence handling v4" to get
>   last_tlb_inv
> - Move the fence after the clear_freed call to make sure we wait for the
>   right invalidation.
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index 6137ab4211be..36b0bd85616f 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  		dev_err(adev->dev, "failed to clear page "
>  			"tables on GEM object close (%ld)\n", r);
>  	if (r || !fence)
> -		goto out_unlock;
> +		goto out_tlb;
>  
>  	amdgpu_bo_fence(bo, fence, true);
>  	dma_fence_put(fence);
>  
> +out_tlb:
> +	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);

You need to change the drm_exec_prepare_obj() call above from 1 to 2 since we are now attaching 2 fences.

Apart from that looks good to me.

Regards,
Christian.

> +
>  out_unlock:
>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
>  		dev_err(adev->dev, "leaking bo va (%ld)\n", r);

Re: [PATCH v2] drm/amdgpu: wait for tlb flush before freeing BO

[Benjamin Cheng]

On 2026-11-06 11:04, Christian König wrote:
>
> On 11/6/26 16:20, Benjamin Cheng wrote:
>> On gem object close, we previously only attached the fence of its own
>> clear_freed() call to the BO's resv. If an unmap of this BO happened
>> prior to the close call, clear_freed() would be invoked, leaving
>> vm->freed empty, and no meaningful fence to be attached to the resv.
>> Because of that the memory may be freed before the TLB flush is
>> complete, and give a running userq potential access to a stale
>> address translation.
>>
>> Fix this by attaching the last_tlb_inv fence to the BO's resv.
>>
>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
>> ---
>> v2:
>> - Rebase on "drm/amdgpu: fix TLB invalidation fence handling v4" to get
>>   last_tlb_inv
>> - Move the fence after the clear_freed call to make sure we wait for the
>>   right invalidation.
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 ++++-
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index 6137ab4211be..36b0bd85616f 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>  		dev_err(adev->dev, "failed to clear page "
>>  			"tables on GEM object close (%ld)\n", r);
>>  	if (r || !fence)
>> -		goto out_unlock;
>> +		goto out_tlb;
>>  
>>  	amdgpu_bo_fence(bo, fence, true);
>>  	dma_fence_put(fence);
>>  
>> +out_tlb:
>> +	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
> You need to change the drm_exec_prepare_obj() call above from 1 to 2 since we are now attaching 2 fences.


As I said in v1 of this patch, amdgpu_bo_fence calls resv itself.


> Apart from that looks good to me.
>
> Regards,
> Christian.
>
>> +
>>  out_unlock:
>>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
>>  		dev_err(adev->dev, "leaking bo va (%ld)\n", r);

Re: [PATCH v2] drm/amdgpu: wait for tlb flush before freeing BO

[Christian König]

On 11/6/26 17:07, Benjamin Cheng wrote:
> 
> On 2026-11-06 11:04, Christian König wrote:
>>
>> On 11/6/26 16:20, Benjamin Cheng wrote:
>>> On gem object close, we previously only attached the fence of its own
>>> clear_freed() call to the BO's resv. If an unmap of this BO happened
>>> prior to the close call, clear_freed() would be invoked, leaving
>>> vm->freed empty, and no meaningful fence to be attached to the resv.
>>> Because of that the memory may be freed before the TLB flush is
>>> complete, and give a running userq potential access to a stale
>>> address translation.
>>>
>>> Fix this by attaching the last_tlb_inv fence to the BO's resv.
>>>
>>> Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
>>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
>>> ---
>>> v2:
>>> - Rebase on "drm/amdgpu: fix TLB invalidation fence handling v4" to get
>>>   last_tlb_inv
>>> - Move the fence after the clear_freed call to make sure we wait for the
>>>   right invalidation.
>>> ---
>>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 5 ++++-
>>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>>
>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> index 6137ab4211be..36b0bd85616f 100644
>>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>>> @@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>>  		dev_err(adev->dev, "failed to clear page "
>>>  			"tables on GEM object close (%ld)\n", r);
>>>  	if (r || !fence)
>>> -		goto out_unlock;
>>> +		goto out_tlb;
>>>  
>>>  	amdgpu_bo_fence(bo, fence, true);
>>>  	dma_fence_put(fence);
>>>  
>>> +out_tlb:
>>> +	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
>> You need to change the drm_exec_prepare_obj() call above from 1 to 2 since we are now attaching 2 fences.
> 
> 
> As I said in v1 of this patch, amdgpu_bo_fence calls resv itself.

Mhm, looks like my response was lost.

Absolutely don't use amdgpu_bo_fence(), reserving the fence slots there is a broken approach which is why the function is deprecated.

That needs to be done once after locking.

Regards,
Christian.

> 
> 
>> Apart from that looks good to me.
>>
>> Regards,
>> Christian.
>>
>>> +
>>>  out_unlock:
>>>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
>>>  		dev_err(adev->dev, "leaking bo va (%ld)\n", r);

[PATCH v3 1/2] drm/amdgpu: wait for tlb flush before freeing BO

[Benjamin Cheng]

On gem object close, we previously only attached the fence of its own
clear_freed() call to the BO's resv. If an unmap of this BO happened
prior to the close call, clear_freed() would be invoked, leaving
vm->freed empty, and no meaningful fence to be attached to the resv.
Because of that the memory may be freed before the TLB flush is
complete, and give a running userq potential access to a stale
address translation.

Fix this by attaching the last_tlb_inv fence to the BO's resv.

Fixes: a1d201e16940 ("drm/amdgpu: enable GFX-V11 userqueue support")
Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
v3:
- reserve the extra fence under lock instead of relying on
  amdgpu_bo_fence() as Christian pointed out
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index 6137ab4211be..c9a84257ae5f 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -340,7 +340,7 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 
 	drm_exec_init(&exec, DRM_EXEC_IGNORE_DUPLICATES, 0);
 	drm_exec_until_all_locked(&exec) {
-		r = drm_exec_prepare_obj(&exec, &bo->tbo.base, 1);
+		r = drm_exec_prepare_obj(&exec, &bo->tbo.base, 2); /* clear_freed + last_tlb_inv */
 		drm_exec_retry_on_contention(&exec);
 		if (unlikely(r))
 			goto out_unlock;
@@ -371,11 +371,14 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 		dev_err(adev->dev, "failed to clear page "
 			"tables on GEM object close (%ld)\n", r);
 	if (r || !fence)
-		goto out_unlock;
+		goto out_tlb;
 
 	amdgpu_bo_fence(bo, fence, true);
 	dma_fence_put(fence);
 
+out_tlb:
+	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
+
 out_unlock:
 	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
 		dev_err(adev->dev, "leaking bo va (%ld)\n", r);
-- 
2.56.0

[PATCH v3 2/2] drm/amdgpu/gem: use dma_resv_add_fence directly

[Benjamin Cheng]

Christian pointed out that amdgpu_bo_fence() is deprecated since it
reserves fence slots there outside of the lock. Replace it with a direct
dma_resv_add_fence call here (we reserve the correct amount of fences
already). Also switch it to use the BOOKKEEP usage since these are both
kernel-internal fences.

Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
index c9a84257ae5f..eebba3bea089 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
@@ -373,11 +373,11 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
 	if (r || !fence)
 		goto out_tlb;
 
-	amdgpu_bo_fence(bo, fence, true);
+	dma_resv_add_fence(bo->tbo.base.resv, fence, DMA_RESV_USAGE_BOOKKEEP);
 	dma_fence_put(fence);
 
 out_tlb:
-	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
+	dma_resv_add_fence(bo->tbo.base.resv, vm->last_tlb_inv, DMA_RESV_USAGE_BOOKKEEP);
 
 out_unlock:
 	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))
-- 
2.56.0

Re: [PATCH v3 2/2] drm/amdgpu/gem: use dma_resv_add_fence directly

[Christian König]

On 11/9/26 15:57, Benjamin Cheng wrote:
> Christian pointed out that amdgpu_bo_fence() is deprecated since it
> reserves fence slots there outside of the lock. Replace it with a direct
> dma_resv_add_fence call here (we reserve the correct amount of fences
> already). Also switch it to use the BOOKKEEP usage since these are both
> kernel-internal fences.
> 
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>

Oh changing that to BOOKKEEP is a really good idea. Are there still users of amdgpu_bo_fence() left?

Either way Reviewed-by: Christian König <christian.koenig@amd.com> for both patches.

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> index c9a84257ae5f..eebba3bea089 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> @@ -373,11 +373,11 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>  	if (r || !fence)
>  		goto out_tlb;
>  
> -	amdgpu_bo_fence(bo, fence, true);
> +	dma_resv_add_fence(bo->tbo.base.resv, fence, DMA_RESV_USAGE_BOOKKEEP);
>  	dma_fence_put(fence);
>  
>  out_tlb:
> -	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
> +	dma_resv_add_fence(bo->tbo.base.resv, vm->last_tlb_inv, DMA_RESV_USAGE_BOOKKEEP);
>  
>  out_unlock:
>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))

Re: [PATCH v3 2/2] drm/amdgpu/gem: use dma_resv_add_fence directly

[Benjamin Cheng]

On 2026-11-09 10:00, Christian König wrote:
>
> On 11/9/26 15:57, Benjamin Cheng wrote:
>> Christian pointed out that amdgpu_bo_fence() is deprecated since it
>> reserves fence slots there outside of the lock. Replace it with a direct
>> dma_resv_add_fence call here (we reserve the correct amount of fences
>> already). Also switch it to use the BOOKKEEP usage since these are both
>> kernel-internal fences.
>>
>> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
> Oh changing that to BOOKKEEP is a really good idea. Are there still users of amdgpu_bo_fence() left?

Yeah, in amdgpu_uvd.c and amdgpu_vm.c. This patch was intended only for amdgpu_gem.

I'll get to the other usages in the next couple of days, and then we can get rid of amdgpu_bo_fence() entirely.

> Either way Reviewed-by: Christian König <christian.koenig@amd.com> for both patches.
>
>> ---
>>  drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> index c9a84257ae5f..eebba3bea089 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
>> @@ -373,11 +373,11 @@ static void amdgpu_gem_object_close(struct drm_gem_object *obj,
>>  	if (r || !fence)
>>  		goto out_tlb;
>>  
>> -	amdgpu_bo_fence(bo, fence, true);
>> +	dma_resv_add_fence(bo->tbo.base.resv, fence, DMA_RESV_USAGE_BOOKKEEP);
>>  	dma_fence_put(fence);
>>  
>>  out_tlb:
>> -	amdgpu_bo_fence(bo, vm->last_tlb_inv, true);
>> +	dma_resv_add_fence(bo->tbo.base.resv, vm->last_tlb_inv, DMA_RESV_USAGE_BOOKKEEP);
>>  
>>  out_unlock:
>>  	if (r && !drm_dev_is_unplugged(adev_to_drm(adev)))

[PATCH 1/3] drm/amdgpu/vm: stop using amdgpu_bo_fence

[Benjamin Cheng]

Switch to stop using this deprecated helper.

The original could be incorrect under certain circumstances, since the
amdgpu_bo_fence only reserves one slot, and at the time of calling the
SDMA commit and TLB flush fences were not added yet (the reserve_fences
is not additive). In addition this allows us to switch to use BOOKKEEP
usage.

Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 94b1cfe05e3e..fe6f2fcb88f6 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -1212,10 +1212,18 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
 	if (r)
 		goto error_free;
 
+	if (!unlocked) {
+		/* last_unlocked + SDMA commit + TLB flush */
+		r = dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 3);
+		if (r)
+			goto error_free;
+	}
+
 	if (!unlocked && !dma_fence_is_signaled(vm->last_unlocked)) {
 		struct dma_fence *tmp = dma_fence_get_stub();
 
-		amdgpu_bo_fence(vm->root.bo, vm->last_unlocked, true);
+		dma_resv_add_fence(vm->root.bo->tbo.base.resv, vm->last_unlocked,
+				   DMA_RESV_USAGE_BOOKKEEP);
 		swap(vm->last_unlocked, tmp);
 		dma_fence_put(tmp);
 	}
-- 
2.56.0

[PATCH 2/3] drm/amdgpu/uvd: stop using amdgpu_bo_fence

[Benjamin Cheng]

This helper is deprecated since it is incorrect in most circumstances.
This was a circumstance where it is correct but it is the only remaining
usage, so get rid of it here to remove it.

Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 17 ++++++++++++++---
 1 file changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
index d88dde859615..9570db033b18 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
@@ -1154,16 +1154,24 @@ static int amdgpu_uvd_send_msg(struct amdgpu_ring *ring, struct amdgpu_bo *bo,
 		if (r)
 			goto err_free;
 	} else {
+		r = amdgpu_bo_reserve(bo, true);
+		if (r)
+			goto err_free;
+
 		r = drm_sched_job_add_resv_dependencies(&job->base,
 							bo->tbo.base.resv,
 							DMA_RESV_USAGE_KERNEL);
 		if (r)
-			goto err_free;
+			goto err_unreserve;
 
 		f = amdgpu_job_submit(job);
 
-		amdgpu_bo_reserve(bo, true);
-		amdgpu_bo_fence(bo, f, false);
+		r = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
+		if (r) {
+			dma_fence_wait(f, false);
+			r = 0;
+		} else
+			dma_resv_add_fence(bo->tbo.base.resv, f, DMA_RESV_USAGE_BOOKKEEP);
 		amdgpu_bo_unreserve(bo);
 	}
 
@@ -1173,6 +1181,9 @@ static int amdgpu_uvd_send_msg(struct amdgpu_ring *ring, struct amdgpu_bo *bo,
 
 	return 0;
 
+err_unreserve:
+	amdgpu_bo_unreserve(bo);
+
 err_free:
 	amdgpu_job_free(job);
 	return r;
-- 
2.56.0

[PATCH 3/3] drm/amdgpu/object: get rid of amdgpu_bo_fence

[Benjamin Cheng]

All uses of this function have been removed, so we can now get rid of
this function.

Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 25 ----------------------
 drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 --
 2 files changed, 27 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
index 6aa895f63aea..7fc43363ec62 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
@@ -1441,31 +1441,6 @@ vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
 	return 0;
 }
 
-/**
- * amdgpu_bo_fence - add fence to buffer object
- *
- * @bo: buffer object in question
- * @fence: fence to add
- * @shared: true if fence should be added shared
- *
- */
-void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
-		     bool shared)
-{
-	struct dma_resv *resv = bo->tbo.base.resv;
-	int r;
-
-	r = dma_resv_reserve_fences(resv, 1);
-	if (r) {
-		/* As last resort on OOM we block for the fence */
-		dma_fence_wait(fence, false);
-		return;
-	}
-
-	dma_resv_add_fence(resv, fence, shared ? DMA_RESV_USAGE_READ :
-			   DMA_RESV_USAGE_WRITE);
-}
-
 /**
  * amdgpu_bo_sync_wait_resv - Wait for BO reservation fences
  *
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
index 2e6a8e6f577a..3a568ec56c84 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
@@ -304,8 +304,6 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
 			   struct ttm_resource *new_mem);
 void amdgpu_bo_release_notify(struct ttm_buffer_object *bo);
 vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo);
-void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
-		     bool shared);
 int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
 			     enum amdgpu_sync_mode sync_mode, void *owner,
 			     bool intr);
-- 
2.56.0

Re: [PATCH 3/3] drm/amdgpu/object: get rid of amdgpu_bo_fence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amdgpu: build failure due to remaining amdgpu_bo_fence()
  call
--

commit ad8f0288ef23c9d2bb8ccdc99b45b97d28e2518c
Author: Benjamin Cheng <benjamin.cheng@amd.com>

drm/amdgpu/object: get rid of amdgpu_bo_fence

This commit removes the amdgpu_bo_fence() helper and its declaration.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6aa895f63aeaf..7fc43363ec626 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1441,31 +1441,6 @@ vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
>  	return 0;
>  }
>  
> -/**
> - * amdgpu_bo_fence - add fence to buffer object
> - *
> - * @bo: buffer object in question
> - * @fence: fence to add
> - * @shared: true if fence should be added shared
> - *
> - */
> -void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
> -		     bool shared)
> -{
> -	struct dma_resv *resv = bo->tbo.base.resv;
> -	int r;
> -
> -	r = dma_resv_reserve_fences(resv, 1);
> -	if (r) {
> -		/* As last resort on OOM we block for the fence */
> -		dma_fence_wait(fence, false);
> -		return;
> -	}
> -
> -	dma_resv_add_fence(resv, fence, shared ? DMA_RESV_USAGE_READ :
> -			   DMA_RESV_USAGE_WRITE);
> -}

[Severity: High]
Does removing amdgpu_bo_fence() break the build in
amdgpu_gem_object_close()?

drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c:amdgpu_gem_object_close() {
    ...
	if (r || !fence)
		goto out_unlock;

	amdgpu_bo_fence(bo, fence, true);
	dma_fence_put(fence);
    ...
}

amdgpu_gem_object_close() still calls amdgpu_bo_fence(), which causes an
implicit function declaration error when compiling amdgpu_gem.c.  Since
amdgpu_gem_object_close() already reserves a fence slot via
drm_exec_prepare_obj(), that caller can use dma_resv_add_fence() directly.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112164408.1748006-1-benjamin.cheng@amd.com?part=3

Re: [PATCH 1/3] drm/amdgpu/vm: stop using amdgpu_bo_fence

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential
issue to consider:
- [High] drm/amdgpu/vm: insufficient fence slots in
  amdgpu_vm_update_range() after PT alloc
--

commit feeae3416bee7b1cf2e4fcd3fbeec98eb2fba53d
Author: Benjamin Cheng <benjamin.cheng@amd.com>

drm/amdgpu/vm: stop using amdgpu_bo_fence

This commit replaces amdgpu_bo_fence() in amdgpu_vm_update_range() with
dma_resv_reserve_fences() and dma_resv_add_fence() using
DMA_RESV_USAGE_BOOKKEEP.

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 2ba33212d5071..2da64e2cfda47 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1212,10 +1212,18 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		goto error_free;
>  
> +	if (!unlocked) {
> +		/* last_unlocked + SDMA commit + TLB flush */
> +		r = dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 3);
> +		if (r)
> +			goto error_free;
> +	}

[Severity: High]
Can reserving the SDMA commit and TLB flush fence slots before
amdgpu_vm_ptes_update() leave vm->root.bo->tbo.base.resv with no free
slot when a new page table BO is allocated?

When amdgpu_vm_ptes_update() allocates a new PT BO via
amdgpu_vm_pt_alloc(), the new BO shares vm->root.bo->tbo.base.resv.
During validation, ttm_bo_add_pipelined_eviction_fences() adds eviction
fences to that shared dma_resv and ends with:

drivers/gpu/drm/ttm/ttm_bo.c:ttm_bo_add_pipelined_eviction_fences() {
	...
	/* TODO: this call should be removed. */
	return dma_resv_reserve_fences(bo->base.resv, 1);
}

Because dma_resv_reserve_fences() is not additive, this only guarantees a
single free slot if the eviction fences filled all but one of the existing
slots.  amdgpu_vm_pt_alloc() then immediately calls amdgpu_vm_pt_clear(),
whose SDMA commit adds a fence on vm->delayed (or vm->immediate) without
reserving a slot, consuming that last free slot.

When amdgpu_vm_update_range() later calls vm->update_funcs->commit(), that
fence is replaced in-place on the same scheduler context, but
amdgpu_vm_tlb_flush() then adds a fence on vm->tlb_fence_context with zero
free slots remaining, hitting BUG_ON(fobj->num_fences >= fobj->max_fences)
in dma_resv_add_fence().

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261112164408.1748006-1-benjamin.cheng@amd.com?part=1

Re: [PATCH 1/3] drm/amdgpu/vm: stop using amdgpu_bo_fence

[Christian König]

On 11/12/26 17:44, Benjamin Cheng wrote:
> Switch to stop using this deprecated helper.
> 
> The original could be incorrect under certain circumstances, since the
> amdgpu_bo_fence only reserves one slot, and at the time of calling the
> SDMA commit and TLB flush fences were not added yet (the reserve_fences
> is not additive). In addition this allows us to switch to use BOOKKEEP
> usage.
> 
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>
> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 10 +++++++++-
>  1 file changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 94b1cfe05e3e..fe6f2fcb88f6 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -1212,10 +1212,18 @@ int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>  	if (r)
>  		goto error_free;
>  
> +	if (!unlocked) {
> +		/* last_unlocked + SDMA commit + TLB flush */
> +		r = dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 3);
> +		if (r)
> +			goto error_free;
> +	}
> +

That should have been reserved in amdgpu_vm_lock_pd(), but we currently only reserve 2 instead of 3 slots there.

Please fix that up and drop this here. Apart from that looks good to me.

Christian.

>  	if (!unlocked && !dma_fence_is_signaled(vm->last_unlocked)) {
>  		struct dma_fence *tmp = dma_fence_get_stub();
>  
> -		amdgpu_bo_fence(vm->root.bo, vm->last_unlocked, true);
> +		dma_resv_add_fence(vm->root.bo->tbo.base.resv, vm->last_unlocked,
> +				   DMA_RESV_USAGE_BOOKKEEP);
>  		swap(vm->last_unlocked, tmp);
>  		dma_fence_put(tmp);
>  	}

Re: [PATCH 2/3] drm/amdgpu/uvd: stop using amdgpu_bo_fence

[Christian König]

On 11/12/26 17:44, Benjamin Cheng wrote:
> This helper is deprecated since it is incorrect in most circumstances.
> This was a circumstance where it is correct but it is the only remaining
> usage, so get rid of it here to remove it.
> 
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c | 17 ++++++++++++++---
>  1 file changed, 14 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> index d88dde859615..9570db033b18 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_uvd.c
> @@ -1154,16 +1154,24 @@ static int amdgpu_uvd_send_msg(struct amdgpu_ring *ring, struct amdgpu_bo *bo,
>  		if (r)
>  			goto err_free;
>  	} else {
> +		r = amdgpu_bo_reserve(bo, true);
> +		if (r)
> +			goto err_free;
> +
>  		r = drm_sched_job_add_resv_dependencies(&job->base,
>  							bo->tbo.base.resv,
>  							DMA_RESV_USAGE_KERNEL);
>  		if (r)
> -			goto err_free;
> +			goto err_unreserve;
>  
>  		f = amdgpu_job_submit(job);
>  
> -		amdgpu_bo_reserve(bo, true);
> -		amdgpu_bo_fence(bo, f, false);
> +		r = dma_resv_reserve_fences(bo->tbo.base.resv, 1);
> +		if (r) {
> +			dma_fence_wait(f, false);
> +			r = 0;
> +		} else
> +			dma_resv_add_fence(bo->tbo.base.resv, f, DMA_RESV_USAGE_BOOKKEEP);
>  		amdgpu_bo_unreserve(bo);
>  	}
>  
> @@ -1173,6 +1181,9 @@ static int amdgpu_uvd_send_msg(struct amdgpu_ring *ring, struct amdgpu_bo *bo,
>  
>  	return 0;
>  
> +err_unreserve:
> +	amdgpu_bo_unreserve(bo);
> +
>  err_free:
>  	amdgpu_job_free(job);
>  	return r;

Re: [PATCH 3/3] drm/amdgpu/object: get rid of amdgpu_bo_fence

[Christian König]

On 11/12/26 17:44, Benjamin Cheng wrote:
> All uses of this function have been removed, so we can now get rid of
> this function.
> 
> Signed-off-by: Benjamin Cheng <benjamin.cheng@amd.com>

Reviewed-by: Christian König <christian.koenig@amd.com>

> ---
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.c | 25 ----------------------
>  drivers/gpu/drm/amd/amdgpu/amdgpu_object.h |  2 --
>  2 files changed, 27 deletions(-)
> 
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> index 6aa895f63aea..7fc43363ec62 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.c
> @@ -1441,31 +1441,6 @@ vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo)
>  	return 0;
>  }
>  
> -/**
> - * amdgpu_bo_fence - add fence to buffer object
> - *
> - * @bo: buffer object in question
> - * @fence: fence to add
> - * @shared: true if fence should be added shared
> - *
> - */
> -void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
> -		     bool shared)
> -{
> -	struct dma_resv *resv = bo->tbo.base.resv;
> -	int r;
> -
> -	r = dma_resv_reserve_fences(resv, 1);
> -	if (r) {
> -		/* As last resort on OOM we block for the fence */
> -		dma_fence_wait(fence, false);
> -		return;
> -	}
> -
> -	dma_resv_add_fence(resv, fence, shared ? DMA_RESV_USAGE_READ :
> -			   DMA_RESV_USAGE_WRITE);
> -}
> -
>  /**
>   * amdgpu_bo_sync_wait_resv - Wait for BO reservation fences
>   *
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> index 2e6a8e6f577a..3a568ec56c84 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_object.h
> @@ -304,8 +304,6 @@ void amdgpu_bo_move_notify(struct ttm_buffer_object *bo,
>  			   struct ttm_resource *new_mem);
>  void amdgpu_bo_release_notify(struct ttm_buffer_object *bo);
>  vm_fault_t amdgpu_bo_fault_reserve_notify(struct ttm_buffer_object *bo);
> -void amdgpu_bo_fence(struct amdgpu_bo *bo, struct dma_fence *fence,
> -		     bool shared);
>  int amdgpu_bo_sync_wait_resv(struct amdgpu_device *adev, struct dma_resv *resv,
>  			     enum amdgpu_sync_mode sync_mode, void *owner,
>  			     bool intr);