* [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
@ 2021-06-29 7:49 Nirmoy Das
2021-06-29 7:49 ` [PATCH 2/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
2021-06-29 11:10 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
0 siblings, 2 replies; 7+ messages in thread
From: Nirmoy Das @ 2021-06-29 7:49 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 | 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(&adev->vm_manager.pasids, vm->pasid));
+ if (r < 0)
+ return r;
+ }
+
+ 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 +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(&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 +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(&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 +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 = &amdgpu_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(&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 +3125,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 +3149,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 +3231,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 +3243,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 +3313,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 +3361,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 +3387,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] 7+ messages in thread
* [PATCH 2/2] drm/amdgpu: free pasid early before converting a vm
2021-06-29 7:49 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
@ 2021-06-29 7:49 ` Nirmoy Das
2021-06-29 7:55 ` [PATCH v2 " Nirmoy Das
2021-06-29 11:10 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
1 sibling, 1 reply; 7+ messages in thread
From: Nirmoy Das @ 2021-06-29 7:49 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
VM code should not be responsible for freeing pasid as pasid
gets allocated outside of VM code, before initializing a vm.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
index f96598279593..5a9006aabec7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1287,6 +1287,14 @@ 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);
if (ret)
--
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] 7+ messages in thread
* [PATCH v2 2/2] drm/amdgpu: free pasid early before converting a vm
2021-06-29 7:49 ` [PATCH 2/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
@ 2021-06-29 7:55 ` Nirmoy Das
2021-06-29 11:12 ` Christian König
0 siblings, 1 reply; 7+ messages in thread
From: Nirmoy Das @ 2021-06-29 7:55 UTC (permalink / raw)
To: amd-gfx; +Cc: Felix.Kuehling, Nirmoy Das, Christian.Koenig
VM code should not be responsible for freeing pasid as pasid
gets allocated outside of VM code, before initializing a vm.
Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
---
drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++++++++
drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ------
2 files changed, 8 insertions(+), 6 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..5a9006aabec7 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
@@ -1287,6 +1287,14 @@ 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);
if (ret)
diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
index 9b0e8a9d7f86..0bd9575b0872 100644
--- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
+++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
@@ -3057,12 +3057,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;
--
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] 7+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-29 7:49 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-29 7:49 ` [PATCH 2/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
@ 2021-06-29 11:10 ` Christian König
2021-06-29 11:14 ` Das, Nirmoy
1 sibling, 1 reply; 7+ messages in thread
From: Christian König @ 2021-06-29 11:10 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Felix.Kuehling
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 <nirmoy.das@amd.com>
> ---
> 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(&adev->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(&adev->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(&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 +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(&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 +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 = &amdgpu_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(&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 +3125,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 +3149,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 +3231,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 +3243,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 +3313,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 +3361,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 +3387,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] 7+ messages in thread
* Re: [PATCH v2 2/2] drm/amdgpu: free pasid early before converting a vm
2021-06-29 7:55 ` [PATCH v2 " Nirmoy Das
@ 2021-06-29 11:12 ` Christian König
2021-06-29 11:15 ` Das, Nirmoy
0 siblings, 1 reply; 7+ messages in thread
From: Christian König @ 2021-06-29 11:12 UTC (permalink / raw)
To: Nirmoy Das, amd-gfx; +Cc: Felix.Kuehling, Christian.Koenig
Am 29.06.21 um 09:55 schrieb Nirmoy Das:
> VM code should not be responsible for freeing pasid as pasid
> gets allocated outside of VM code, before initializing a vm.
>
> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
> ---
> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++++++++
> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ------
> 2 files changed, 8 insertions(+), 6 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..5a9006aabec7 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
> @@ -1287,6 +1287,14 @@ 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);
I'm really wondering if amdgpu_vm_make_compute() and amdgpu_vm_init()
now need the pasid as parameter anymore.
Christian.
> if (ret)
> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> index 9b0e8a9d7f86..0bd9575b0872 100644
> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> @@ -3057,12 +3057,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;
> --
> 2.32.0
>
> _______________________________________________
> amd-gfx mailing list
> amd-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/amd-gfx
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
2021-06-29 11:10 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
@ 2021-06-29 11:14 ` Das, Nirmoy
0 siblings, 0 replies; 7+ messages in thread
From: Das, Nirmoy @ 2021-06-29 11:14 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Felix.Kuehling
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 <nirmoy.das@amd.com>
>> ---
>> 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(&adev->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(&adev->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(&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 +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(&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 +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 = &amdgpu_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(&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 +3125,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 +3149,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 +3231,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 +3243,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 +3313,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 +3361,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 +3387,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] 7+ messages in thread
* Re: [PATCH v2 2/2] drm/amdgpu: free pasid early before converting a vm
2021-06-29 11:12 ` Christian König
@ 2021-06-29 11:15 ` Das, Nirmoy
0 siblings, 0 replies; 7+ messages in thread
From: Das, Nirmoy @ 2021-06-29 11:15 UTC (permalink / raw)
To: Christian König, amd-gfx; +Cc: Felix.Kuehling, Christian.Koenig
On 6/29/2021 1:12 PM, Christian König wrote:
> Am 29.06.21 um 09:55 schrieb Nirmoy Das:
>> VM code should not be responsible for freeing pasid as pasid
>> gets allocated outside of VM code, before initializing a vm.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das@amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c | 8 ++++++++
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 6 ------
>> 2 files changed, 8 insertions(+), 6 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..5a9006aabec7 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_amdkfd_gpuvm.c
>> @@ -1287,6 +1287,14 @@ 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);
>
> I'm really wondering if amdgpu_vm_make_compute() and amdgpu_vm_init()
> now need the pasid as parameter anymore.
We could use the exported amdgpu_vm_set_pasid() and remove that pasid
from both functions.
Regards,
Nirmoy
>
> Christian.
>
>> if (ret)
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 9b0e8a9d7f86..0bd9575b0872 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -3057,12 +3057,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;
>> --
>> 2.32.0
>>
>> _______________________________________________
>> amd-gfx mailing list
>> amd-gfx@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&data=04%7C01%7Cnirmoy.das%40amd.com%7C2a379081693b490c09fd08d93aeed4b3%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637605619715690644%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C2000&sdata=6ztVdZYy8nsS6OekDFAjDc2toJYYnl9RUZEyl5Kn6c0%3D&reserved=0
>>
>
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2021-06-29 11:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-29 7:49 [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Nirmoy Das
2021-06-29 7:49 ` [PATCH 2/2] drm/amdgpu: free pasid early before converting a vm Nirmoy Das
2021-06-29 7:55 ` [PATCH v2 " Nirmoy Das
2021-06-29 11:12 ` Christian König
2021-06-29 11:15 ` Das, Nirmoy
2021-06-29 11:10 ` [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm Christian König
2021-06-29 11:14 ` Das, Nirmoy
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.