* [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
@ 2021-06-29 15:19 Nirmoy Das
2021-06-29 15:19 ` [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment Nirmoy Das
2021-06-29 17:40 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
0 siblings, 2 replies; 6+ messages in thread
From: Nirmoy Das @ 2021-06-29 15:19 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
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.das@amd.com>
---
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(&adev->vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+
+ vm->pasid = 0;
+ }
+
+ if (pasid) {
+ r = xa_err(xa_store_irq(&adev->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(&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 +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(&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);
+ /* 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 = &amdgpu_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(&adev->vm_manager.pasid_lock, flags);
- idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
- spin_unlock_irqrestore(&adev->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(&to_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(&adev->vm_manager.pasid_lock, flags);
- idr_remove(&adev->vm_manager.pasid_idr, pasid);
- spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
- }
+free_pasid_entry:
+ amdgpu_vm_set_pasid(adev, vm, 0);
unreserve_bo:
amdgpu_bo_unreserve(vm->root.bo);
return r;
@@ -3133,14 +3138,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;
}
@@ -3164,15 +3162,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);
@@ -3254,8 +3244,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);
}
/**
@@ -3267,8 +3256,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);
}
@@ -3337,13 +3326,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);
}
/**
@@ -3385,15 +3374,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;
@@ -3411,11 +3400,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..8e8bc132e590 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;
@@ -375,6 +374,9 @@ extern const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs;
void amdgpu_vm_manager_init(struct amdgpu_device *adev);
void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
+int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
+ unsigned long pasid);
+
long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
--
2.32.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment
2021-06-29 15:19 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
@ 2021-06-29 15:19 ` Nirmoy Das
2021-06-30 21:57 ` Felix Kuehling
2021-06-29 17:40 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
1 sibling, 1 reply; 6+ messages in thread
From: Nirmoy Das @ 2021-06-29 15:19 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
Use new helper function amdgpu_vm_set_pasid() to
assign vm pasid value. This also ensures that we don't free
a pasid from vm code as pasids are allocated somewhere else.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 13 ++++++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 10 +++++--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++----------------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
4 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index f96598279593..3a2ac7f66bbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1287,11 +1287,22 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
if (avm->process_info)
return -EINVAL;
+ /* Free the original amdgpu allocated pasid,
+ * will be replaced with kfd allocated pasid.
+ */
+ if (avm->pasid) {
+ amdgpu_pasid_free(avm->pasid);
+ amdgpu_vm_set_pasid(adev, avm, 0);
+ }
+
/* Convert VM into a compute VM */
- ret = amdgpu_vm_make_compute(adev, avm, pasid);
+ ret = amdgpu_vm_make_compute(adev, avm);
if (ret)
return ret;
+ ret = amdgpu_vm_set_pasid(adev, avm, pasid);
+ if (ret)
+ return ret;
/* Initialize KFD part of the VM and process info */
ret = init_kfd_vm(avm, process_info, ef);
if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index cbb932f97355..66bf8b0c56bb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1178,10 +1178,14 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
pasid = 0;
}
- r = amdgpu_vm_init(adev, &fpriv->vm, pasid);
+ r = amdgpu_vm_init(adev, &fpriv->vm);
if (r)
goto error_pasid;
+ r = amdgpu_vm_set_pasid(adev, &fpriv->vm, pasid);
+ if (r)
+ goto error_vm;
+
fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
if (!fpriv->prt_va) {
r = -ENOMEM;
@@ -1209,8 +1213,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
amdgpu_vm_fini(adev, &fpriv->vm);
error_pasid:
- if (pasid)
+ if (pasid) {
amdgpu_pasid_free(pasid);
+ amdgpu_vm_set_pasid(adev, &fpriv->vm, 0);
+ }
kfree(fpriv);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index fd92ff27931a..1dccede6387e 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2899,14 +2899,13 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
*
* @adev: amdgpu_device pointer
* @vm: requested vm
- * @pasid: Process address space identifier
*
* Init @vm fields.
*
* Returns:
* 0 for success, error for failure.
*/
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
{
struct amdgpu_bo *root_bo;
struct amdgpu_bo_vm *root;
@@ -2980,10 +2979,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
amdgpu_bo_unreserve(vm->root.bo);
- r = amdgpu_vm_set_pasid(adev, vm, pasid);
- if (r)
- goto error_free_root;
-
INIT_KFIFO(vm->faults);
return 0;
@@ -3039,7 +3034,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
*
* @adev: amdgpu_device pointer
* @vm: requested vm
- * @pasid: pasid to use
*
* This only works on GFX VMs that don't have any BOs added and no
* page tables allocated yet.
@@ -3047,7 +3041,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
* Changes the following VM parameters:
* - use_cpu_for_update
* - pte_supports_ats
- * - pasid (old PASID is released, because compute manages its own PASIDs)
*
* Reinitializes the page directory to reflect the changed ATS
* setting.
@@ -3055,8 +3048,7 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
* Returns:
* 0 for success, -errno for errors.
*/
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
- u32 pasid)
+int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
{
bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
int r;
@@ -3070,16 +3062,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
if (r)
goto unreserve_bo;
- /* Free the original amdgpu allocated pasid,
- * will be replaced with kfd allocated pasid.
- */
- if (vm->pasid)
- amdgpu_pasid_free(vm->pasid);
-
- 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.
*/
@@ -3089,7 +3071,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_pasid_entry;
+ goto unreserve_bo;
}
/* Update VM state */
@@ -3106,7 +3088,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_pasid_entry;
+ goto unreserve_bo;
vm->update_funcs = &amdgpu_vm_cpu_funcs;
} else {
@@ -3121,8 +3103,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
goto unreserve_bo;
-free_pasid_entry:
- amdgpu_vm_set_pasid(adev, vm, 0);
unreserve_bo:
amdgpu_bo_unreserve(vm->root.bo);
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index 8e8bc132e590..a0bceccaa6b5 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -378,8 +378,8 @@ int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
unsigned long pasid);
long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
+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);
void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
--
2.32.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-29 15:19 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-29 15:19 ` [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment Nirmoy Das
@ 2021-06-29 17:40 ` Christian König
2021-06-30 8:15 ` Das, Nirmoy
1 sibling, 1 reply; 6+ messages in thread
From: Christian König @ 2021-06-29 17:40 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Felix.Kuehling
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 <nirmoy.das@amd.com>
> ---
> 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(&adev->vm_manager.pasids, vm->pasid));
> + if (r < 0)
> + return r;
> +
> + vm->pasid = 0;
> + }
> +
> + if (pasid) {
> + r = xa_err(xa_store_irq(&adev->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(&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 +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(&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);
> + /* 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 = &amdgpu_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(&adev->vm_manager.pasid_lock, flags);
> - idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
> - spin_unlock_irqrestore(&adev->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(&to_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(&adev->vm_manager.pasid_lock, flags);
> - idr_remove(&adev->vm_manager.pasid_idr, pasid);
> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> - }
> +free_pasid_entry:
> + amdgpu_vm_set_pasid(adev, vm, 0);
> unreserve_bo:
> amdgpu_bo_unreserve(vm->root.bo);
> return r;
> @@ -3133,14 +3138,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;
> }
>
> @@ -3164,15 +3162,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);
>
> @@ -3254,8 +3244,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);
> }
>
> /**
> @@ -3267,8 +3256,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);
> }
> @@ -3337,13 +3326,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);
> }
>
> /**
> @@ -3385,15 +3374,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;
> @@ -3411,11 +3400,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..8e8bc132e590 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;
> @@ -375,6 +374,9 @@ extern const struct amdgpu_vm_update_funcs amdgpu_vm_sdma_funcs;
> void amdgpu_vm_manager_init(struct amdgpu_device *adev);
> void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>
> +int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> + unsigned long pasid);
> +
> long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
> int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-29 17:40 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
@ 2021-06-30 8:15 ` Das, Nirmoy
0 siblings, 0 replies; 6+ messages in thread
From: Das, Nirmoy @ 2021-06-30 8:15 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Felix.Kuehling
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 <nirmoy.das@amd.com>
>> ---
>> 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(&adev->vm_manager.pasids, vm->pasid));
>> + if (r < 0)
>> + return r;
>> +
>> + vm->pasid = 0;
>> + }
>> +
>> + if (pasid) {
>> + r = xa_err(xa_store_irq(&adev->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(&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 +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(&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);
>> + /* 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 = &amdgpu_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(&adev->vm_manager.pasid_lock, flags);
>> - idr_remove(&adev->vm_manager.pasid_idr, vm->pasid);
>> - spin_unlock_irqrestore(&adev->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(&to_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(&adev->vm_manager.pasid_lock, flags);
>> - idr_remove(&adev->vm_manager.pasid_idr, pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> - }
>> +free_pasid_entry:
>> + amdgpu_vm_set_pasid(adev, vm, 0);
>> unreserve_bo:
>> amdgpu_bo_unreserve(vm->root.bo);
>> return r;
>> @@ -3133,14 +3138,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;
>> }
>> @@ -3164,15 +3162,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);
>> @@ -3254,8 +3244,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);
>> }
>> /**
>> @@ -3267,8 +3256,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);
>> }
>> @@ -3337,13 +3326,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);
>> }
>> /**
>> @@ -3385,15 +3374,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;
>> @@ -3411,11 +3400,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..8e8bc132e590 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;
>> @@ -375,6 +374,9 @@ extern const struct amdgpu_vm_update_funcs
>> amdgpu_vm_sdma_funcs;
>> void amdgpu_vm_manager_init(struct amdgpu_device *adev);
>> void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>> +int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct
>> amdgpu_vm *vm,
>> + unsigned long pasid);
>> +
>> long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
>> int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm
>> *vm, u32 pasid);
>> int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct
>> amdgpu_vm *vm, u32 pasid);
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment
2021-06-29 15:19 ` [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment Nirmoy Das
@ 2021-06-30 21:57 ` Felix Kuehling
0 siblings, 0 replies; 6+ messages in thread
From: Felix Kuehling @ 2021-06-30 21:57 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Christian.Koenig
On 2021-06-29 11:19 a.m., Nirmoy Das wrote:
> Use new helper function amdgpu_vm_set_pasid() to
> assign vm pasid value. This also ensures that we don't free
> a pasid from vm code as pasids are allocated somewhere else.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Christian's comments notwithstanding, the series is
Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
> ---
> .../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 13 ++++++++-
> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 10 +++++--
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++----------------
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
> 4 files changed, 26 insertions(+), 29 deletions(-)
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> index f96598279593..3a2ac7f66bbd 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1287,11 +1287,22 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
> if (avm->process_info)
> return -EINVAL;
>
> + /* Free the original amdgpu allocated pasid,
> + * will be replaced with kfd allocated pasid.
> + */
> + if (avm->pasid) {
> + amdgpu_pasid_free(avm->pasid);
> + amdgpu_vm_set_pasid(adev, avm, 0);
> + }
> +
> /* Convert VM into a compute VM */
> - ret = amdgpu_vm_make_compute(adev, avm, pasid);
> + ret = amdgpu_vm_make_compute(adev, avm);
> if (ret)
> return ret;
>
> + ret = amdgpu_vm_set_pasid(adev, avm, pasid);
> + if (ret)
> + return ret;
> /* Initialize KFD part of the VM and process info */
> ret = init_kfd_vm(avm, process_info, ef);
> if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> index cbb932f97355..66bf8b0c56bb 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> @@ -1178,10 +1178,14 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> pasid = 0;
> }
>
> - r = amdgpu_vm_init(adev, &fpriv->vm, pasid);
> + r = amdgpu_vm_init(adev, &fpriv->vm);
> if (r)
> goto error_pasid;
>
> + r = amdgpu_vm_set_pasid(adev, &fpriv->vm, pasid);
> + if (r)
> + goto error_vm;
> +
> fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
> if (!fpriv->prt_va) {
> r = -ENOMEM;
> @@ -1209,8 +1213,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
> amdgpu_vm_fini(adev, &fpriv->vm);
>
> error_pasid:
> - if (pasid)
> + if (pasid) {
> amdgpu_pasid_free(pasid);
> + amdgpu_vm_set_pasid(adev, &fpriv->vm, 0);
> + }
>
> kfree(fpriv);
>
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index fd92ff27931a..1dccede6387e 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -2899,14 +2899,13 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
> *
> * @adev: amdgpu_device pointer
> * @vm: requested vm
> - * @pasid: Process address space identifier
> *
> * Init @vm fields.
> *
> * Returns:
> * 0 for success, error for failure.
> */
> -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> {
> struct amdgpu_bo *root_bo;
> struct amdgpu_bo_vm *root;
> @@ -2980,10 +2979,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
>
> amdgpu_bo_unreserve(vm->root.bo);
>
> - r = amdgpu_vm_set_pasid(adev, vm, pasid);
> - if (r)
> - goto error_free_root;
> -
> INIT_KFIFO(vm->faults);
>
> return 0;
> @@ -3039,7 +3034,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
> *
> * @adev: amdgpu_device pointer
> * @vm: requested vm
> - * @pasid: pasid to use
> *
> * This only works on GFX VMs that don't have any BOs added and no
> * page tables allocated yet.
> @@ -3047,7 +3041,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
> * Changes the following VM parameters:
> * - use_cpu_for_update
> * - pte_supports_ats
> - * - pasid (old PASID is released, because compute manages its own PASIDs)
> *
> * Reinitializes the page directory to reflect the changed ATS
> * setting.
> @@ -3055,8 +3048,7 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
> * Returns:
> * 0 for success, -errno for errors.
> */
> -int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> - u32 pasid)
> +int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> {
> bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
> int r;
> @@ -3070,16 +3062,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> if (r)
> goto unreserve_bo;
>
> - /* Free the original amdgpu allocated pasid,
> - * will be replaced with kfd allocated pasid.
> - */
> - if (vm->pasid)
> - amdgpu_pasid_free(vm->pasid);
> -
> - 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.
> */
> @@ -3089,7 +3071,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_pasid_entry;
> + goto unreserve_bo;
> }
>
> /* Update VM state */
> @@ -3106,7 +3088,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_pasid_entry;
> + goto unreserve_bo;
>
> vm->update_funcs = &amdgpu_vm_cpu_funcs;
> } else {
> @@ -3121,8 +3103,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
>
> goto unreserve_bo;
>
> -free_pasid_entry:
> - amdgpu_vm_set_pasid(adev, vm, 0);
> unreserve_bo:
> amdgpu_bo_unreserve(vm->root.bo);
> return r;
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> index 8e8bc132e590..a0bceccaa6b5 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> @@ -378,8 +378,8 @@ int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
> unsigned long pasid);
>
> long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
> -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
> -int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
> +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> +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);
> void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 6+ messages in thread
* [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment
2021-07-02 9:25 Nirmoy Das
@ 2021-07-02 9:25 ` Nirmoy Das
0 siblings, 0 replies; 6+ messages in thread
From: Nirmoy Das @ 2021-07-02 9:25 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
Use new helper function amdgpu_vm_set_pasid() to
assign vm pasid value. This also ensures that we don't free
a pasid from vm code as pasids are allocated somewhere else.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
Acked-by: Felix Kuehling <Felix.Kuehling@amd.com>
---
.../gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 13 ++++++++-
drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 10 +++++--
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 28 +++----------------
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 4 +--
4 files changed, 26 insertions(+), 29 deletions(-)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index f96598279593..3a2ac7f66bbd 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1287,11 +1287,22 @@ int amdgpu_amdkfd_gpuvm_acquire_process_vm(struct kgd_dev *kgd,
if (avm->process_info)
return -EINVAL;
+ /* Free the original amdgpu allocated pasid,
+ * will be replaced with kfd allocated pasid.
+ */
+ if (avm->pasid) {
+ amdgpu_pasid_free(avm->pasid);
+ amdgpu_vm_set_pasid(adev, avm, 0);
+ }
+
/* Convert VM into a compute VM */
- ret = amdgpu_vm_make_compute(adev, avm, pasid);
+ ret = amdgpu_vm_make_compute(adev, avm);
if (ret)
return ret;
+ ret = amdgpu_vm_set_pasid(adev, avm, pasid);
+ if (ret)
+ return ret;
/* Initialize KFD part of the VM and process info */
ret = init_kfd_vm(avm, process_info, ef);
if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
index cbb932f97355..66bf8b0c56bb 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
@@ -1178,10 +1178,14 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
pasid = 0;
}
- r = amdgpu_vm_init(adev, &fpriv->vm, pasid);
+ r = amdgpu_vm_init(adev, &fpriv->vm);
if (r)
goto error_pasid;
+ r = amdgpu_vm_set_pasid(adev, &fpriv->vm, pasid);
+ if (r)
+ goto error_vm;
+
fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
if (!fpriv->prt_va) {
r = -ENOMEM;
@@ -1209,8 +1213,10 @@ int amdgpu_driver_open_kms(struct drm_device *dev, struct drm_file *file_priv)
amdgpu_vm_fini(adev, &fpriv->vm);
error_pasid:
- if (pasid)
+ if (pasid) {
amdgpu_pasid_free(pasid);
+ amdgpu_vm_set_pasid(adev, &fpriv->vm, 0);
+ }
kfree(fpriv);
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 0f82df5bfa7a..565c8c59c995 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -2899,14 +2899,13 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout)
*
* @adev: amdgpu_device pointer
* @vm: requested vm
- * @pasid: Process address space identifier
*
* Init @vm fields.
*
* Returns:
* 0 for success, error for failure.
*/
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
{
struct amdgpu_bo *root_bo;
struct amdgpu_bo_vm *root;
@@ -2980,10 +2979,6 @@ int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid)
amdgpu_bo_unreserve(vm->root.bo);
- r = amdgpu_vm_set_pasid(adev, vm, pasid);
- if (r)
- goto error_free_root;
-
INIT_KFIFO(vm->faults);
return 0;
@@ -3039,7 +3034,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
*
* @adev: amdgpu_device pointer
* @vm: requested vm
- * @pasid: pasid to use
*
* This only works on GFX VMs that don't have any BOs added and no
* page tables allocated yet.
@@ -3047,7 +3041,6 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
* Changes the following VM parameters:
* - use_cpu_for_update
* - pte_supports_ats
- * - pasid (old PASID is released, because compute manages its own PASIDs)
*
* Reinitializes the page directory to reflect the changed ATS
* setting.
@@ -3055,8 +3048,7 @@ static int amdgpu_vm_check_clean_reserved(struct amdgpu_device *adev,
* Returns:
* 0 for success, -errno for errors.
*/
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
- u32 pasid)
+int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm)
{
bool pte_support_ats = (adev->asic_type == CHIP_RAVEN);
int r;
@@ -3070,16 +3062,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
if (r)
goto unreserve_bo;
- /* Free the original amdgpu allocated pasid,
- * will be replaced with kfd allocated pasid.
- */
- if (vm->pasid)
- amdgpu_pasid_free(vm->pasid);
-
- 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.
*/
@@ -3089,7 +3071,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_pasid_entry;
+ goto unreserve_bo;
}
/* Update VM state */
@@ -3106,7 +3088,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_pasid_entry;
+ goto unreserve_bo;
vm->update_funcs = &amdgpu_vm_cpu_funcs;
} else {
@@ -3121,8 +3103,6 @@ int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm,
goto unreserve_bo;
-free_pasid_entry:
- amdgpu_vm_set_pasid(adev, vm, 0);
unreserve_bo:
amdgpu_bo_unreserve(vm->root.bo);
return r;
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
index bcbe362e7d5b..80cc9ab2c1d0 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
@@ -378,8 +378,8 @@ int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct amdgpu_vm *vm,
u32 pasid);
long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
-int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
-int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct amdgpu_vm *vm, u32 pasid);
+int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
+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);
void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm *vm);
void amdgpu_vm_get_pd_bo(struct amdgpu_vm *vm,
--
2.32.0
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply related [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-07-02 9:25 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 15:19 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-29 15:19 ` [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment Nirmoy Das
2021-06-30 21:57 ` Felix Kuehling
2021-06-29 17:40 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
2021-06-30 8:15 ` Das, Nirmoy
2021-07-02 9:25 Nirmoy Das
2021-07-02 9:25 ` [PATCH 2/2] drm/amdgpu: separate out vm pasid assignment Nirmoy Das
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.