Re: [PATCH 2/2] drm/amdgpu: use xarray for storing pasid in vm

Wed, 23 Jun 2021 05:51:02 -0700 



Am 23.06.21 um 14:25 schrieb Nirmoy Das:

Replace idr with xarray as we actually need hash functionality.
Cleanup code related to vm pasid by adding helper function.

Signed-off-by: Nirmoy Das <nirmoy....@amd.com>
---
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 134 +++++++++++--------------
  drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   3 +-
  2 files changed, 60 insertions(+), 77 deletions(-)

diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index be841d62a1d4..e047e56a4be2 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,39 @@ struct amdgpu_prt_cb {
        struct dma_fence_cb cb;
  };

  
+static int amdgpu_vm_set_pasid(struct amdgpu_device *adev,

+                              struct amdgpu_vm *vm,
+                              unsigned long pasid)


Some kerneldoc please describing why we have that function.


+{
+       unsigned long flags;
+       int r;
+
+       if (pasid) {



You should probably reorder the code so that the old pasid is first 
removed and then eventually the new one added.



+               xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+               r = xa_err(__xa_store(&adev->vm_manager.pasids, pasid, vm,
+                                     GFP_ATOMIC));
+               xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);



As far as I can see this can just use xa_store() which also drops the 
need for GFP_ATOMIC here.



+               if (r < 0)
+                       return r;
+       } else {
+               unsigned long index;
+               struct amdgpu_vm *res;
+
+               xa_lock_irqsave(&adev->vm_manager.pasids, flags);
+               xa_for_each(&adev->vm_manager.pasids, index, res) {
+                       if (res == vm) {
+                               __xa_erase(&adev->vm_manager.pasids, index);
+                               break;
+                       }
+               }
+               xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);


That is really ugly, why is that necessary?

Christian


+       }
+
+       vm->pasid = pasid;
+
+       return 0;
+}
+
  /*
   * vm eviction_lock can be taken in MMU notifiers. Make sure no reclaim-FS
   * happens while holding this lock anywhere to prevent deadlocks when
@@ -2940,18 +2973,9 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct 
amdgpu_vm *vm, u32 pasid)

  
  	amdgpu_bo_unreserve(vm->root.bo);
  
-	if (pasid) {

-               unsigned long flags;
-
-               spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-               r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-                             GFP_ATOMIC);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-               if (r < 0)
-                       goto error_free_root;
-
-               vm->pasid = pasid;
-       }
+       r = amdgpu_vm_set_pasid(adev, vm, pasid);
+       if (r)
+               goto error_free_root;

  
  	INIT_KFIFO(vm->faults);
  
@@ -3039,18 +3063,11 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,

        if (r)
                goto unreserve_bo;

  
-	if (pasid) {

-               unsigned long flags;
-
-               spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-               r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid + 1,
-                             GFP_ATOMIC);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-
-               if (r == -ENOSPC)
-                       goto unreserve_bo;
-               r = 0;
-       }
+       /* remove previous {pasid:vm} entry first */
+       r = amdgpu_vm_set_pasid(adev, vm, 0);
+       r = amdgpu_vm_set_pasid(adev, vm, pasid);
+       if (r)
+               goto unreserve_bo;

  
  	/* Check if PD needs to be reinitialized and do it before

         * changing any other state, in case it fails.
@@ -3061,7 +3078,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
                                       to_amdgpu_bo_vm(vm->root.bo),
                                       false);
                if (r)
-                       goto free_idr;
+                       goto free_pasid_entry;
        }

  
  	/* Update VM state */

@@ -3078,7 +3095,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
                r = amdgpu_bo_sync_wait(vm->root.bo,
                                        AMDGPU_FENCE_OWNER_UNDEFINED, true);
                if (r)
-                       goto free_idr;
+                       goto free_pasid_entry;

  
  		vm->update_funcs = &amdgpu_vm_cpu_funcs;

        } else {
@@ -3088,31 +3105,14 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
        vm->last_update = NULL;
        vm->is_compute_context = true;

  
-	if (vm->pasid) {

-               unsigned long flags;
-
-               spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-               idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-               vm->pasid = 0;
-       }
-
        /* Free the shadow bo for compute VM */
        amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);

  
-	if (pasid)

-               vm->pasid = pasid;
-
        goto unreserve_bo;

  
-free_idr:

-       if (pasid) {
-               unsigned long flags;
+free_pasid_entry:
+       amdgpu_vm_set_pasid(adev, vm, 0);

  
-		spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);

-               idr_remove(&adev->vm_manager.pasid_idr, pasid);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-       }
  unreserve_bo:
        amdgpu_bo_unreserve(vm->root.bo);
        return r;
@@ -3128,14 +3128,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, 
struct amdgpu_vm *vm,
   */
  void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct amdgpu_vm 
*vm)
  {
-       if (vm->pasid) {
-               unsigned long flags;
-
-               spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-               idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-       }
-       vm->pasid = 0;
+       amdgpu_vm_set_pasid(adev, vm, 0);
        vm->is_compute_context = false;
  }

  
@@ -3159,15 +3152,7 @@ void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm)
  
  	root = amdgpu_bo_ref(vm->root.bo);

        amdgpu_bo_reserve(root, true);
-       if (vm->pasid) {
-               unsigned long flags;
-
-               spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
-               idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
-               spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
-               vm->pasid = 0;
-       }
-
+       amdgpu_vm_set_pasid(adev, vm, 0);
        dma_fence_wait(vm->last_unlocked, false);
        dma_fence_put(vm->last_unlocked);

  
@@ -3249,8 +3234,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)

        adev->vm_manager.vm_update_mode = 0;
  #endif

  
-	idr_init(&adev->vm_manager.pasid_idr);

-       spin_lock_init(&adev->vm_manager.pasid_lock);
+       xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
  }

  
  /**

@@ -3262,8 +3246,8 @@ void amdgpu_vm_manager_init(struct amdgpu_device *adev)
   */
  void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
  {
-       WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
-       idr_destroy(&adev->vm_manager.pasid_idr);
+       WARN_ON(!xa_empty(&adev->vm_manager.pasids));
+       xa_destroy(&adev->vm_manager.pasids);

  
  	amdgpu_vmid_mgr_fini(adev);

  }
@@ -3332,13 +3316,13 @@ void amdgpu_vm_get_task_info(struct amdgpu_device 
*adev, u32 pasid,
        struct amdgpu_vm *vm;
        unsigned long flags;

  
-	spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);

+       xa_lock_irqsave(&adev->vm_manager.pasids, flags);

  
-	vm = idr_find(&adev->vm_manager.pasid_idr, pasid);

+       vm = xa_load(&adev->vm_manager.pasids, pasid);
        if (vm)
                *task_info = vm->task_info;

  
-	spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);

+       xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
  }

  
  /**

@@ -3380,15 +3364,15 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, 
u32 pasid,
        struct amdgpu_vm *vm;
        int r;

  
-	spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);

-       vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+       xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+       vm = xa_load(&adev->vm_manager.pasids, pasid);
        if (vm) {
                root = amdgpu_bo_ref(vm->root.bo);
                is_compute_context = vm->is_compute_context;
        } else {
                root = NULL;
        }
-       spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+       xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);

  
  	if (!root)

                return false;
@@ -3406,11 +3390,11 @@ bool amdgpu_vm_handle_fault(struct amdgpu_device *adev, 
u32 pasid,
                goto error_unref;

  
  	/* Double check that the VM still exists */

-       spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
-       vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
+       xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
+       vm = xa_load(&adev->vm_manager.pasids, pasid);
        if (vm && vm->root.bo != root)
                vm = NULL;
-       spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
+       xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
        if (!vm)
                goto error_unlock;

  
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h

index ddb85a85cbba..31c467764162 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
        /* PASID to VM mapping, will be used in interrupt context to
         * look up VM of a page fault
         */
-       struct idr                              pasid_idr;
-       spinlock_t                              pasid_lock;
+       struct xarray                           pasids;
  };

  
  struct amdgpu_bo_va_mapping;


_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx






















					Reply via email to