Re: [PATCH 2/9] drm/amdgpu: fix cleared PDE/PTE flag generation

[Kuehling, Felix]

On 2026-09-30 10:59, Christian König wrote:
On 9/30/26 16:44, Kuehling, Felix wrote:
[+Mukul]
On 2026-09-30 05:02, Christian König wrote:
@Felix and @Philip any objections to this patch?

It is actually a bug fix for the NPA support.
As I understand it, this fixes the flags used when unmapping memory from the NPA 
VM. We passed adev->gmc.noretry_flags from amdgpu_ualink_unmap_npa_addr to 
amdgpu_vm_update_range explicitly in the flags parameter. I guess that's no longer 
needed with your fix. But I think the end result would be the same, right?
Not quite, passing adev->gmc.noretry_flags to amdgpu_vm_update_range() is 
completely broken as well and also need fixing.

The flags parameter to amdgpu_vm_update_range() can't contain the AMDGPU_PTE_TF 
nor the AMDGPU_PDE_PTE flag because those are overwritten by the PTE callbacks.

I'm pretty sure Mukul tested this and got something that worked as 
expected, at least for a time. I'm not sure if it regressed since, maybe 
during upstreaming. @Mukul, can you comment?



That's why setting the AMDGPU_PTE_IS_PTE for gfx12 through the init_pte_flags 
is completely broken as well.

We seriously need to stop doing such hacks.

Is there any documentation that tells us what flags can be used where. 
This isn't obvious at all.

Thanks,
  Felix



Regards,
Christian.

Regards,
   Felix


Regards,
Christian.

On 9/28/26 21:08, Timur Kristóf wrote:
On 2026. szeptember 28., hétfő 11:10:34 keleti államokbeli nyári idő Christian
König wrote:
That was broken since adding the NPA support.

Signed-off-by: Christian König <christian.koe...@amd.com>
Reviewed-by: Timur Kristóf <timur.kris...@gmail.com>

---
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c | 64 ++++++++++++-----------
   1 file changed, 33 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c index
c03327f1242d3..e8f441e018839 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -346,6 +346,33 @@ static void amdgpu_vm_pt_next_dfs(struct amdgpu_device
*adev, amdgpu_vm_pt_continue_dfs((start), (entry));
\
            (entry) = (cursor).entry, amdgpu_vm_pt_next_dfs((adev),
&(cursor)))
+/* Return the flags used for cleared PDEs/PTES */
+static uint64_t amdgpu_vm_pt_clear_flags(struct amdgpu_device *adev,
+                    struct amdgpu_vm *vm,
+                    unsigned int level)
+{
+    uint64_t flags;
+
+    if (adev->asic_type < CHIP_VEGA10)
+        return 0;
+
+    if (level != AMDGPU_VM_PTB) {
+        uint64_t value = 0;
+
+        flags = AMDGPU_PDE_PTE_FLAG(adev);
+        if (vm->is_npa)
+            flags |= adev->gmc.noretry_flags;
+        amdgpu_gmc_get_vm_pde(adev, level, &value, &flags);
+    } else if (vm->is_npa) {
+        flags = adev->gmc.noretry_flags;
+    } else {
+        /* Workaround for fault priority problem on GMC9 */
+        flags = AMDGPU_PTE_EXECUTABLE | adev->gmc.init_pte_flags;
+    }
+
+    return flags;
+}
+
   /**
    * amdgpu_vm_pt_clear - initially clear the PDs/PTs
    *
@@ -366,10 +393,9 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev,
struct amdgpu_vm *vm, struct ttm_operation_ctx ctx = { true, false };
       struct amdgpu_vm_update_params params;
       struct amdgpu_bo *ancestor = &vmbo->bo;
-    unsigned int entries;
       struct amdgpu_bo *bo = &vmbo->bo;
-    uint64_t value = 0, flags = 0;
-    uint64_t addr;
+    unsigned int entries;
+    uint64_t flags;
       int r, idx;

       /* Figure out our place in the hierarchy */
@@ -404,26 +430,8 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev,
struct amdgpu_vm *vm, if (r)
           goto exit;

-    addr = 0;
-
-    if (adev->asic_type >= CHIP_VEGA10) {
-        if (level != AMDGPU_VM_PTB) {
-            if (vm->is_npa)
-                flags = adev->gmc.noretry_flags;
-            /* Handle leaf PDEs as PTEs */
-            flags |= AMDGPU_PDE_PTE_FLAG(adev);
-            amdgpu_gmc_get_vm_pde(adev, level,
-                          &value, &flags);
-        } else if (vm->is_npa) {
-            flags = adev->gmc.noretry_flags;
-        } else {
-            /* Workaround for fault priority problem on
GMC9 */
-            flags = AMDGPU_PTE_EXECUTABLE | adev-
gmc.init_pte_flags;
-        }
-    }
-
-    r = vm->update_funcs->update(&params, vmbo, addr, 0, entries,
-                     value, flags);
+    flags = amdgpu_vm_pt_clear_flags(adev, vm, level);
+    r = vm->update_funcs->update(&params, vmbo, 0, 0, entries, 0,
flags);
       if (r)
           goto exit;

@@ -712,15 +720,9 @@ static void amdgpu_vm_pte_update_flags(struct
amdgpu_vm_update_params *params, flags |=
AMDGPU_PDE_PTE_FLAG(params->adev);
           amdgpu_gmc_get_vm_pde(adev, level, &addr, &flags);

-    } else if (adev->asic_type >= CHIP_VEGA10 &&
-           !(flags & AMDGPU_PTE_VALID) &&
+    } else if (!(flags & AMDGPU_PTE_VALID) &&
              !(flags & AMDGPU_PTE_PRT_FLAG(params->adev))) {
-
-        /* Workaround for fault priority problem on GMC9 and
GFX12,
-         * EXECUTABLE for GMC9 fault priority and init_pte_flags
-         * (e.g. AMDGPU_PTE_IS_PTE on GFX12)
-         */
-        flags |= AMDGPU_PTE_EXECUTABLE | adev-
gmc.init_pte_flags;
+        flags |= amdgpu_vm_pt_clear_flags(adev, params->vm,
level);
       }

       /*