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

[sashiko-bot@kernel.org]

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