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

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

[Shahyan Soltani]

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>
---
 drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
 1 file changed, 2 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..bc5d4bfdd424 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2829,11 +2829,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);
 	amdgpu_bo_unreserve(vm->root.bo);
 	if (r)
 		return r;
 
-	vm->is_npa = true;
 	vm->need_tlb_fence = false;
 
 	return 0;
-- 
2.56.0

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

[sashiko-bot]

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

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

[Kuehling, Felix]

On 2026-11-13 08:40, Shahyan Soltani wrote:
> 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.

Thanks. I'm don't know why that was set twice. Must have been a mistake 
when I made that original change. See inline.


>
> Suggested-by: Christian König <christian.koenig@amd.com>
> Signed-off-by: Shahyan Soltani <shahyan.soltani@amd.com>
> ---
>   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 3 ++-
>   1 file changed, 2 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..bc5d4bfdd424 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2829,11 +2829,12 @@ int amdgpu_vm_make_npa(struct amdgpu_device *adev, struct amdgpu_vm *vm)
>   	if (r)
>   		return r;

We should unset is_npa when the function returns an error. Or maybe set 
is_npa later in the function. But it has to be set before calling 
amdgpu_vm_pt_clear below.


>   	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);

I'd recommend adding a sanity check (WARN_ON) that nothing is mapped in 
the VM yet. Otherwise this will break existing VM mappings.


>   	amdgpu_bo_unreserve(vm->root.bo);
>   	if (r)
>   		return r;

Here we need to be careful that is_npa remains set only if pinning 
succeeded. Because this flag is used to decide whether to unpin the page 
tables later when the VM is destroyed. IOW, if pinning failed, unset 
is_npa. If pinning succeeded but pt_clear failed, leave it set.

Regards,
   Felix


>   
> -	vm->is_npa = true;
>   	vm->need_tlb_fence = false;
>   
>   	return 0;