Re: [PATCH 08/13] drm/amdgpu: split amdgpu_vm_update_range

[Kuehling, Felix]

On 2026-06-05 05:21, Christian König wrote:
On 6/3/26 19:54, Kuehling, Felix wrote:
...
+    r = vm->update_funcs->prepare(&params, sync,
+                      AMDGPU_KERNEL_JOB_ID_VM_UNMAP_RANGE);
+    if (r)
+        goto error_free;
+
+    amdgpu_vm_update_leaves(&params, start, last, 0, flags);
+
+    r = vm->update_funcs->commit(&params, fence);
+    if (r)
+        goto error_free;
So this wraps amdgpu_vm_update_leaves with all the stuff that's necessary to 
actually execute the page table update.
Yes, exactly that.

I don't understand how amdgpu_vm_update_leaves works without this when you call 
it directly from amdgpu_vm_handle_fault (in patch 6).
It does mostly the same, but not 100%.

Shouldn't you use amdgpu_vm_unmap_range there instead?
No, the main difference is that calling amdgpu_vm_update_leaves() from the 
fault handler needs to bypass the normal sequencial handling of VM updates.

Background is that this is a band aid for a dma_fence based submission to avoid 
a full GPU lockup and instead just continue even when the rendering is 
incorrect.

I'm still missing something. amdgpu_vm_unmap_range calls 
vm->update_funcs->prepare, and vm->update_funcs->commit. 
amdgpu_vm_update_leaves doesn't. Neither does amdgpu_vm_handle_fault. So 
how do you e.g. create and submit an SDMA job for the page table update 
in the VM fault handler?

Or is this only meant to work with CPU page table updates? But then 
you're still missing some synchronization and HDP flushing if you never 
call prepare and commit.

Regards,
  Felix



And if that's true, then maybe you can use the name amdgpu_vm_update_leaves for 
this instead.
I'm certainly open for better naming.

Maybe we should call the function in amdgpu_vm_pt.c 
amdgpu_vm_pt_update_leaves() and the higher level function in amdgpu_vm.c just 
amdgpu_vm_update_leaves() ?

Thanks for the review,
Christian.


Regards,
   Felix


+
+    amdgpu_vm_tlb_flush(&params, fence, tlb_cb);
+    amdgpu_vm_pt_free_list(adev, &params);
+    tlb_cb = NULL;
+
+error_free:
+    kfree(tlb_cb);
+    amdgpu_vm_eviction_unlock(vm);
+    drm_dev_exit(idx);
+    return r;
+}
+
   void amdgpu_vm_get_memory(struct amdgpu_vm *vm,
                 struct amdgpu_mem_stats stats[__AMDGPU_PL_NUM])
   {
@@ -1362,11 +1434,11 @@ int amdgpu_vm_bo_update(struct amdgpu_device *adev, 
struct amdgpu_bo_va *bo_va,
             trace_amdgpu_vm_bo_update(mapping);
   -        r = amdgpu_vm_update_range(adev, vm, false, flush_tlb,
-                       !uncached, &sync, mapping->start,
-                       mapping->last, update_flags,
-                       mapping->offset, vram_base, mem,
-                       pages_addr, last_update);
+        r = amdgpu_vm_map_range(adev, vm, flush_tlb, !uncached, &sync,
+                    mapping->start, mapping->last,
+                    update_flags, mapping->offset,
+                    vram_base, mem, pages_addr,
+                    last_update);
           if (r)
               goto error_free;
       }
@@ -1565,9 +1637,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device *adev,
               struct amdgpu_bo_va_mapping, list);
           list_del(&mapping->list);
   -        r = amdgpu_vm_update_range(adev, vm, false, true, false,
-                       &sync, mapping->start, mapping->last,
-                       0, 0, 0, NULL, NULL, &f);
+        r = amdgpu_vm_map_range(adev, vm, true, false,
+                    &sync, mapping->start, mapping->last,
+                    0, 0, 0, NULL, NULL, &f);
           amdgpu_vm_free_mapping(adev, vm, mapping, f);
           if (r) {
               dma_fence_put(f);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 3e86a2a470f0..561f2873d2ec 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -529,12 +529,16 @@ int amdgpu_vm_flush_compute_tlb(struct amdgpu_device 
*adev,
                   uint32_t xcc_mask);
   void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
                   struct amdgpu_vm *vm, struct amdgpu_bo *bo);
-int amdgpu_vm_update_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
-               bool unlocked, bool flush_tlb, bool allow_override,
+int amdgpu_vm_map_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+            bool flush_tlb, bool allow_override,
+            struct amdgpu_sync *sync, uint64_t start,
+            uint64_t last, uint64_t flags, uint64_t offset,
+            uint64_t vram_base, struct ttm_resource *res,
+            dma_addr_t *pages_addr, struct dma_fence **fence);
+int amdgpu_vm_unmap_range(struct amdgpu_device *adev, struct amdgpu_vm *vm,
                  struct amdgpu_sync *sync, uint64_t start,
-               uint64_t last, uint64_t flags, uint64_t offset,
-               uint64_t vram_base, struct ttm_resource *res,
-               dma_addr_t *pages_addr, struct dma_fence **fence);
+               uint64_t last, uint64_t flags,
+               struct dma_fence **fence);
   int amdgpu_vm_bo_update(struct amdgpu_device *adev,
               struct amdgpu_bo_va *bo_va,
               bool clear);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
index 6f5415d5a1bc..ac3f3e31e2e2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
@@ -553,7 +553,6 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
                   struct amdgpu_vm_update_params *params)
   {
       struct amdgpu_vm_bo_base *entry, *next;
-    bool unlocked = params->unlocked;
         if (list_empty(&params->tlb_flush_waitlist))
           return;
@@ -561,7 +560,7 @@ void amdgpu_vm_pt_free_list(struct amdgpu_device *adev,
       /*
        * unlocked unmap clear page table leaves, warning to free the page 
entry.
        */
-    WARN_ON(unlocked);
+    WARN_ON(params->unlocked);
         list_for_each_entry_safe(entry, next, &params->tlb_flush_waitlist, 
vm_status)
           amdgpu_vm_pt_free(entry);
@@ -801,24 +800,17 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params 
*params,
           uint64_t incr, entry_end, pe_start;
           struct amdgpu_bo *pt;
   -        if (!params->unlocked) {
-            /* make sure that the page tables covering the
-             * address range are actually allocated
-             */
-            r = amdgpu_vm_pt_alloc(params->adev, params->vm,
-                           &cursor);
-            if (r)
-                return r;
-        }
+        /* make sure that the page tables covering the
+         * address range are actually allocated
+         */
+        r = amdgpu_vm_pt_alloc(params->adev, params->vm, &cursor);
+        if (r)
+            return r;
             shift = amdgpu_vm_pt_level_shift(adev, cursor.level);
           parent_shift = amdgpu_vm_pt_level_shift(adev, cursor.level - 1);
-        if (params->unlocked) {
-            /* Unlocked updates are only allowed on the leaves */
-            if (amdgpu_vm_pt_descendant(adev, &cursor))
-                continue;
-        } else if (adev->asic_type < CHIP_VEGA10 &&
-               (flags & AMDGPU_PTE_VALID)) {
+        if (adev->asic_type < CHIP_VEGA10 &&
+            (flags & AMDGPU_PTE_VALID)) {
               /* No huge page support before GMC v9 */
               if (cursor.level != AMDGPU_VM_PTB) {
                   if (!amdgpu_vm_pt_descendant(adev, &cursor))
@@ -864,14 +856,7 @@ int amdgpu_vm_ptes_update(struct amdgpu_vm_update_params 
*params,
           mask = amdgpu_vm_pt_entries_mask(adev, cursor.level);
           pe_start = ((cursor.pfn >> shift) & mask) * 8;
   -        if (cursor.level < AMDGPU_VM_PTB && params->unlocked)
-            /*
-             * MMU notifier callback unlocked unmap huge page, leave is PDE 
entry,
-             * only clear one entry. Next entry search again for PDE or PTE 
leave.
-             */
-            entry_end = 1ULL << shift;
-        else
-            entry_end = ((uint64_t)mask + 1) << shift;
+        entry_end = ((uint64_t)mask + 1) << shift;
           entry_end += cursor.pfn & ~(entry_end - 1);
           entry_end = min(entry_end, end);
   diff --git a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c 
b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
index 37b5166e9a14..d0ea20dea3e1 100644
--- a/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
+++ b/drivers/gpu/drm/amd/amdkfd/kfd_svm.c
@@ -1372,9 +1372,8 @@ svm_range_unmap_from_gpu(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
           return -EINVAL;
       }
   -    return amdgpu_vm_update_range(adev, vm, true, true, false, NULL, 
gpu_start,
-                      gpu_end, init_pte_value, 0, 0, NULL, NULL,
-                      fence);
+    return amdgpu_vm_unmap_range(adev, vm, NULL, gpu_start, gpu_end,
+                     init_pte_value, fence);
   }
     static int
@@ -1489,12 +1488,11 @@ svm_range_map_to_gpu(struct kfd_process_device *pdd, 
struct svm_range *prange,
                (last_domain == SVM_RANGE_VRAM_DOMAIN) ? 1 : 0,
                pte_flags);
   -        r = amdgpu_vm_update_range(adev, vm, false, flush_tlb, true,
-                       NULL, gpu_start, gpu_end,
-                       pte_flags,
-                       (last_start - prange->start) << PAGE_SHIFT,
-                       bo_adev ? bo_adev->vm_manager.vram_base_offset : 0,
-                       NULL, dma_addr, &vm->last_update);
+        r = amdgpu_vm_map_range(adev, vm, flush_tlb, true, NULL,
+                    gpu_start, gpu_end, pte_flags,
+                    (last_start - prange->start) << PAGE_SHIFT,
+                    bo_adev ? bo_adev->vm_manager.vram_base_offset : 0,
+                    NULL, dma_addr, &vm->last_update);
             for (j = last_start - prange->start; j <= i; j++)
               dma_addr[j] |= last_domain;