[PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm
Das, Nirmoy
nirmoy.das at amd.com
Wed Jun 30 08:15:46 UTC 2021
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 at 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);
>
More information about the amd-gfx
mailing list