Re: [RFC PATCH 1/4] drm/amdkfd: Improve amdgpu_vm_handle_moved

[Felix Kuehling]

Am 2022-03-17 um 04:21 schrieb Christian König:
Am 17.03.22 um 01:20 schrieb Felix Kuehling:
Let amdgpu_vm_handle_moved update all BO VA mappings of BOs reserved by
the caller. This will be useful for handling extra BO VA mappings in
KFD VMs that are managed through the render node API.

Yes, that change is on my TODO list for quite a while as well.

TODO: This may also allow simplification of amdgpu_cs_vm_handling. See
the TODO comment in the code.

No, that won't work just yet.

We need to change the TLB flush detection for that, but I'm already 
working on those as well.

Your TLB flushing patch series looks good to me.

There is one other issue, though. amdgpu_vm_handle_moved doesn't update 
the sync object, so I couldn't figure out I can wait for all the page 
table updates to finish.

Regards,
  Felix



Signed-off-by: Felix Kuehling <felix.kuehl...@amd.com>

Please update the TODO, with that done: Reviewed-by: Christian König 
<christian.koe...@amd.com>

---
  drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 +++++-
  drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  2 +-
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 18 +++++++++++++-----
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  3 ++-
  4 files changed, 21 insertions(+), 8 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
index d162243d8e78..10941f0d8dde 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
@@ -826,6 +826,10 @@ static int amdgpu_cs_vm_handling(struct 
amdgpu_cs_parser *p)
              return r;
      }
  +    /* TODO: Is this loop still needed, or could this be handled by
+     * amdgpu_vm_handle_moved, now that it can handle all BOs that are
+     * reserved under p->ticket?
+     */
      amdgpu_bo_list_for_each_entry(e, p->bo_list) {
          /* ignore duplicates */
          bo = ttm_to_amdgpu_bo(e->tv.bo);
@@ -845,7 +849,7 @@ static int amdgpu_cs_vm_handling(struct 
amdgpu_cs_parser *p)
              return r;
      }
  -    r = amdgpu_vm_handle_moved(adev, vm);
+    r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
      if (r)
          return r;
  diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
index 579adfafe4d0..50805613c38c 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
@@ -414,7 +414,7 @@ amdgpu_dma_buf_move_notify(struct 
dma_buf_attachment *attach)
            r = amdgpu_vm_clear_freed(adev, vm, NULL);
          if (!r)
-            r = amdgpu_vm_handle_moved(adev, vm);
+            r = amdgpu_vm_handle_moved(adev, vm, ticket);
            if (r && r != -EBUSY)
              DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index fc4563cf2828..726b42c6d606 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2190,11 +2190,12 @@ int amdgpu_vm_clear_freed(struct 
amdgpu_device *adev,
   * PTs have to be reserved!
   */
  int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
-               struct amdgpu_vm *vm)
+               struct amdgpu_vm *vm,
+               struct ww_acquire_ctx *ticket)
  {
      struct amdgpu_bo_va *bo_va, *tmp;
      struct dma_resv *resv;
-    bool clear;
+    bool clear, unlock;
      int r;
        list_for_each_entry_safe(bo_va, tmp, &vm->moved, 
base.vm_status) {
@@ -2212,17 +2213,24 @@ int amdgpu_vm_handle_moved(struct 
amdgpu_device *adev,
          spin_unlock(&vm->invalidated_lock);
            /* Try to reserve the BO to avoid clearing its ptes */
-        if (!amdgpu_vm_debug && dma_resv_trylock(resv))
+        if (!amdgpu_vm_debug && dma_resv_trylock(resv)) {
              clear = false;
+            unlock = true;
+        /* The caller is already holding the reservation lock */
+        } else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
+            clear = false;
+            unlock = false;
          /* Somebody else is using the BO right now */
-        else
+        } else {
              clear = true;
+            unlock = false;
+        }
            r = amdgpu_vm_bo_update(adev, bo_va, clear, NULL);
          if (r)
              return r;
  -        if (!clear)
+        if (unlock)
              dma_resv_unlock(resv);
          spin_lock(&vm->invalidated_lock);
      }
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index a40a6a993bb0..120a76aaae75 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -396,7 +396,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device 
*adev,
                struct amdgpu_vm *vm,
                struct dma_fence **fence);
  int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
-               struct amdgpu_vm *vm);
+               struct amdgpu_vm *vm,
+               struct ww_acquire_ctx *ticket);
  int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
                  struct amdgpu_device *bo_adev,
                  struct amdgpu_vm *vm, bool immediate,