Re: [PATCH] drm/amdgpu: clear root PT after setting vm->is_npa = true

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential
issues to consider:
- [High] drm/amdgpu: missing dma_resv_reserve_fences() in
  amdgpu_vm_make_npa()
- [Low] drm/amdgpu: missing Fixes trailer on amdgpu_vm_make_npa() bug
  fix
--

commit 39cb24e53c83e097f97bd6822aa71f40ca923e2b
Author: Shahyan Soltani <shahyan.soltani@amd.com>

> drm/amdgpu: clear root PT after setting vm->is_npa = true
>
> Previously, the root pt was never cleared after setting vm->is_npa =
> true within amdgpu_vm_make_npa(). This resulted in the flag
> adev->gmc.no_retry_flags to never be applied for the NPA VMs.
>
> Call amdgpu_vm_pt_clear() after vm->is_npa = true is set and the bo has
> been pinned/validated. This allows the root PT of the NPA VM to be cleared
> with the correct value.
>
> Remove the redundant second setting of vm->is_npa = true.
>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>

[Severity: Low]
This fixes a bug in amdgpu_vm_make_npa(). Should the commit message include:

  Fixes: e05a54573df7 ("drm/amdgpu: Pin page tables in NPA VMs")

> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 2ba33212d5071..eb5d243ab463a 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2827,11 +2827,12 @@ int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>  	if (r)
>  		return r;
>  	r = amdgpu_bo_pin(vm->root.bo, AMDGPU_GEM_DOMAIN_VRAM);
> +	if (!r)
> +		r = amdgpu_vm_pt_clear(adev, vm, to_amdgpu_bo_vm(vm->root.bo), false);

[Severity: High]
Does calling amdgpu_vm_pt_clear() here without first calling
dma_resv_reserve_fences(vm->root.bo->tbo.base.resv, 1) trigger a BUG_ON() in
dma_resv_add_fence()?

Because vm->root.bo is already in compatible VRAM from amdgpu_vm_init(),
ttm_bo_validate() inside amdgpu_bo_pin() and amdgpu_vm_pt_clear() returns
early without reserving a fence slot. When SDMA updates are used,
amdgpu_vm_pt_clear() -> amdgpu_vm_sdma_commit() calls
dma_resv_add_fence(p->vm->root.bo->tbo.base.resv, f, DMA_RESV_USAGE_BOOKKEEP),
hitting BUG_ON(fobj->num_fences >= fobj->max_fences).

amdgpu_vm_init() avoids this by reserving a fence slot beforehand:

drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:amdgpu_vm_init() {
	...
	r = dma_resv_reserve_fences(root_bo->tbo.base.resv, 1);
	if (r)
		goto error_free_root;

	r = amdgpu_vm_pt_clear(adev, vm, root, false);
	...
}

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261113134017.2781893-1-shahyan.soltani@amd.com?part=1