Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
Am 02.07.21 um 11: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
Acked-by: Felix Kuehling
Reviewed-by: Christian König for the series.
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 149 -
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..0f82df5bfa7a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,46 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+/**
+ * amdgpu_vm_set_pasid - manage pasid and vm ptr mapping
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm pointer
+ * @pasid: the pasid the VM is using on this GPU
+ *
+ * Set the pasid this VM is using on this GPU, can also be used to remove the
+ * pasid by passing in zero.
+ *
+ */
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ u32 pasid)
+{
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+
+ 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 +2980,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3070,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3089,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 +3106,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3116,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
-* Will be replaced with kfd allocated pasid
-*/
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
-
[PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
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
Acked-by: Felix Kuehling
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 149 -
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..0f82df5bfa7a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,46 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+/**
+ * amdgpu_vm_set_pasid - manage pasid and vm ptr mapping
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm pointer
+ * @pasid: the pasid the VM is using on this GPU
+ *
+ * Set the pasid this VM is using on this GPU, can also be used to remove the
+ * pasid by passing in zero.
+ *
+ */
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ u32 pasid)
+{
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+
+ 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 +2980,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3070,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3089,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 +3106,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3116,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
-* Will be replaced with kfd allocated pasid
-*/
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
- }
-
/* Free the shadow bo for compute VM */
Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
On 6/29/2021 7:40 PM, Christian König wrote:
Am 29.06.21 um 17:19 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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 149 -
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..fd92ff27931a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,46 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+/**
+ * amdgpu_vm_set_pasid - manage pasid and vm ptr mapping
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm pointer
+ * @pasid: requested pasid
Better write "the pasid the VM is using on this GPU".
+ *
+ * Each pasid associats with a vm pointer.
That is misleading. KFD most likely want to associate the same pasid
with multiple VMs on different GPUs.
Better write "Set the pasid this VM is using on this GPU, can also be
used to remove the pasid by passing in zero.".
OK, thanks for fixing it, I will update.
This function can be use to
+ * create a new pasid,vm association or to remove an existing one.
To remove an
+ * existing pasid,vm association, pass 0 as @pasid.
+ */
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm
*vm,
+ unsigned long pasid)
"unsigned long pasid"? The pasid is either 16 or 20 bits IIRC.
I kept it as xarray's index. I will change it to u32 then.
Thanks,
Nirmoy
Regards,
Christian.
+{
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+
+ 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 +2980,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3070,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+ * will be replaced with kfd allocated pasid.
+ */
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3089,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 +3106,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3116,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
- * Will be replaced with kfd allocated pasid
Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
Am 29.06.21 um 17:19 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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 149 -
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..fd92ff27931a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,46 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+/**
+ * amdgpu_vm_set_pasid - manage pasid and vm ptr mapping
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm pointer
+ * @pasid: requested pasid
Better write "the pasid the VM is using on this GPU".
+ *
+ * Each pasid associats with a vm pointer.
That is misleading. KFD most likely want to associate the same pasid
with multiple VMs on different GPUs.
Better write "Set the pasid this VM is using on this GPU, can also be
used to remove the pasid by passing in zero.".
This function can be use to
+ * create a new pasid,vm association or to remove an existing one. To remove an
+ * existing pasid,vm association, pass 0 as @pasid.
+ */
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ unsigned long pasid)
"unsigned long pasid"? The pasid is either 16 or 20 bits IIRC.
Regards,
Christian.
+{
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+
+ 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 +2980,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3070,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3089,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 +3106,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3116,13 @@ 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;
-
-
[PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 149 -
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 73 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..fd92ff27931a 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,46 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+/**
+ * amdgpu_vm_set_pasid - manage pasid and vm ptr mapping
+ *
+ * @adev: amdgpu_device pointer
+ * @vm: amdgpu_vm pointer
+ * @pasid: requested pasid
+ *
+ * Each pasid associats with a vm pointer. This function can be use to
+ * create a new pasid,vm association or to remove an existing one. To remove an
+ * existing pasid,vm association, pass 0 as @pasid.
+ */
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ unsigned long pasid)
+{
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+
+ 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 +2980,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3070,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3089,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 +3106,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3116,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
-* Will be replaced with kfd allocated pasid
-*/
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
- }
Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
On 6/29/2021 1:10 PM, Christian König wrote:
Am 29.06.21 um 09:49 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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 136 ++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 60 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..9b0e8a9d7f86 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,33 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct
amdgpu_vm *vm,
+ unsigned long pasid)
Proper kerneldoc please when the function is now exported.
I had one, missed it this revision :/
+{
+ unsigned long flags;
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
I think we should have a "vm->pasid = 0;" here so that we have a
consistent state when inserting the new pasid below failed.
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+ }
+
+ vm->pasid = pasid;
This assignment can then be moved into the "if" as well.
Apart from that looks like a really nice cleanup to me.
Thanks, I will resend.
Regards,
Christian.
+
+ 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 +2967,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3057,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid
+ 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+ * will be replaced with kfd allocated pasid.
+ */
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3076,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 +3093,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3103,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
- * Will be replaced with kfd allocated pasid
- */
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
- }
-
/* Free the shadow bo for compute VM */
amdgpu_bo_unref(_amdgpu_bo_vm(vm->root.bo)->shadow);
- if (pasid)
- vm->pasid = pasid;
-
goto unreserve_bo;
-free_idr:
- if (pasid) {
- unsigned long flags;
-
- spin_lock_irqsave(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock,
Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
Am 29.06.21 um 09:49 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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 136 ++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 60 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..9b0e8a9d7f86 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,33 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ unsigned long pasid)
Proper kerneldoc please when the function is now exported.
+{
+ unsigned long flags;
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
I think we should have a "vm->pasid = 0;" here so that we have a
consistent state when inserting the new pasid below failed.
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+ }
+
+ vm->pasid = pasid;
This assignment can then be moved into the "if" as well.
Apart from that looks like a really nice cleanup to me.
Regards,
Christian.
+
+ 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 +2967,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3057,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3076,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 +3093,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3103,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
-* Will be replaced with kfd allocated pasid
-*/
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
- }
-
/* Free the shadow bo for
[PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
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
---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 136 ++---
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 6 +-
2 files changed, 60 insertions(+), 82 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 63975bda8e76..9b0e8a9d7f86 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -87,6 +87,33 @@ struct amdgpu_prt_cb {
struct dma_fence_cb cb;
};
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ unsigned long pasid)
+{
+ unsigned long flags;
+ int r;
+
+ if (vm->pasid == pasid)
+ return 0;
+
+ if (vm->pasid) {
+ r = xa_err(xa_erase_irq(>vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(>vm_manager.pasids, pasid, vm,
+ GFP_KERNEL));
+ if (r < 0)
+ return r;
+ }
+
+ 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 +2967,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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>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 +3057,15 @@ 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(>vm_manager.pasid_lock, flags);
- r = idr_alloc(>vm_manager.pasid_idr, vm, pasid, pasid + 1,
- GFP_ATOMIC);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
+ /* Free the original amdgpu allocated pasid,
+* will be replaced with kfd allocated pasid.
+*/
+ if (vm->pasid)
+ amdgpu_pasid_free(vm->pasid);
- if (r == -ENOSPC)
- goto unreserve_bo;
- r = 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 +3076,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 +3093,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 = _vm_cpu_funcs;
} else {
@@ -3088,36 +3103,13 @@ 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(>vm_manager.pasid_lock, flags);
- idr_remove(>vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(>vm_manager.pasid_lock, flags);
-
- /* Free the original amdgpu allocated pasid
-* Will be replaced with kfd allocated pasid
-*/
- amdgpu_pasid_free(vm->pasid);
- vm->pasid = 0;
- }
-
/* Free the shadow bo for compute VM */
amdgpu_bo_unref(_amdgpu_bo_vm(vm->root.bo)->shadow);
- if (pasid)
- vm->pasid = pasid;
-
goto unreserve_bo;
-free_idr:
- if (pasid) {
- unsigned long flags;
-
- spin_lock_irqsave(>vm_manager.pasid_lock, flags);
-
