[PATCH 1/1] drm/amdgpu: add helper function for vm pasid
Christian König
christian.koenig at amd.com
Mon Jun 21 10:26:00 UTC 2021
Well you completely break the handling in amdgpu_vm_handle_fault() with
this.
Christian.
Am 21.06.21 um 11:47 schrieb Das, Nirmoy:
> ping.
>
> On 6/17/2021 3:03 PM, Nirmoy Das wrote:
>> Cleanup code related to vm pasid by adding helper function.
>> This reduces lots code duplication.
>>
>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>> ---
>> drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c | 17 +--
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 176 ++++++++++++------------
>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 2 +-
>> 3 files changed, 96 insertions(+), 99 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> index cbb932f97355..27851fb0e25b 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
>> @@ -1149,7 +1149,7 @@ int amdgpu_driver_open_kms(struct drm_device
>> *dev, struct drm_file *file_priv)
>> {
>> struct amdgpu_device *adev = drm_to_adev(dev);
>> struct amdgpu_fpriv *fpriv;
>> - int r, pasid;
>> + int r;
>> /* Ensure IB tests are run on ring */
>> flush_delayed_work(&adev->delayed_init_work);
>> @@ -1172,15 +1172,9 @@ int amdgpu_driver_open_kms(struct drm_device
>> *dev, struct drm_file *file_priv)
>> goto out_suspend;
>> }
>> - pasid = amdgpu_pasid_alloc(16);
>> - if (pasid < 0) {
>> - dev_warn(adev->dev, "No more PASIDs available!");
>> - pasid = 0;
>> - }
>> -
>> - r = amdgpu_vm_init(adev, &fpriv->vm, pasid);
>> + r = amdgpu_vm_init(adev, &fpriv->vm);
>> if (r)
>> - goto error_pasid;
>> + goto free_fpriv;
>> fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
>> if (!fpriv->prt_va) {
>> @@ -1208,10 +1202,7 @@ int amdgpu_driver_open_kms(struct drm_device
>> *dev, struct drm_file *file_priv)
>> error_vm:
>> amdgpu_vm_fini(adev, &fpriv->vm);
>> -error_pasid:
>> - if (pasid)
>> - amdgpu_pasid_free(pasid);
>> -
>> +free_fpriv:
>> kfree(fpriv);
>> out_suspend:
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> index 63975bda8e76..562c2c48a3a3 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>> @@ -87,6 +87,69 @@ struct amdgpu_prt_cb {
>> struct dma_fence_cb cb;
>> };
>> +static int amdgpu_vm_pasid_alloc(struct amdgpu_device *adev,
>> + struct amdgpu_vm *vm, unsigned int pasid)
>> +{
>> + unsigned long flags;
>> + int r;
>> +
>> + if (!pasid)
>> + return 0;
>> +
>> + 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)
>> + return r;
>> +
>> + vm->pasid = pasid;
>> + return 0;
>> +}
>> +static void amdgpu_vm_pasid_remove_id(struct amdgpu_device *adev,
>> + unsigned int pasid)
>> +{
>> + unsigned long flags;
>> +
>> + if (!pasid)
>> + return;
>> +
>> + 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);
>> +
>> +}
>> +
>> +static void amdgpu_vm_pasid_remove(struct amdgpu_device *adev,
>> + struct amdgpu_vm *vm)
>> +{
>> + amdgpu_vm_pasid_remove_id(adev, vm->pasid);
>> + vm->pasid = 0;
>> +}
>> +
>> +static void amdgpu_vm_pasid_free(struct amdgpu_device *adev,
>> + struct amdgpu_vm *vm)
>> +{
>> + if (!vm->pasid)
>> + return;
>> +
>> + amdgpu_pasid_free(vm->pasid);
>> + amdgpu_vm_pasid_remove(adev, vm);
>> +}
>> +
>> +static struct amdgpu_vm *amdgpu_vm_pasid_find(struct amdgpu_device
>> *adev,
>> + unsigned int pasid)
>> +{
>> + struct amdgpu_vm *vm;
>> + unsigned long flags;
>> +
>> + spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> + vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>> + spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> +
>> + return vm;
>> +}
>> +
>> /*
>> * vm eviction_lock can be taken in MMU notifiers. Make sure no
>> reclaim-FS
>> * happens while holding this lock anywhere to prevent deadlocks when
>> @@ -2859,17 +2922,17 @@ 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;
>> + unsigned int pasid;
>> int r, i;
>> vm->va = RB_ROOT_CACHED;
>> @@ -2940,19 +3003,15 @@ 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;
>> + pasid = amdgpu_pasid_alloc(16);
>> + if (pasid < 0) {
>> + dev_warn(adev->dev, "No more PASIDs available!");
>> + pasid = 0;
>> }
>> + if (amdgpu_vm_pasid_alloc(adev, vm, pasid))
>> + goto error_free_pasid;
>> +
>> INIT_KFIFO(vm->faults);
>> return 0;
>> @@ -2960,6 +3019,10 @@ int amdgpu_vm_init(struct amdgpu_device *adev,
>> struct amdgpu_vm *vm, u32 pasid)
>> error_unreserve:
>> amdgpu_bo_unreserve(vm->root.bo);
>> +error_free_pasid:
>> + if (pasid)
>> + amdgpu_pasid_free(pasid);
>> +
>> error_free_root:
>> amdgpu_bo_unref(&root->shadow);
>> amdgpu_bo_unref(&root_bo);
>> @@ -3039,18 +3102,11 @@ int amdgpu_vm_make_compute(struct
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>> if (r)
>> goto unreserve_bo;
>> - if (pasid) {
>> - unsigned long flags;
>> -
>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> - r = idr_alloc(&adev->vm_manager.pasid_idr, vm, pasid, pasid
>> + 1,
>> - GFP_ATOMIC);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> + r = amdgpu_vm_pasid_alloc(adev, vm, pasid);
>> + if (r == -ENOSPC)
>> + goto unreserve_bo;
>> - if (r == -ENOSPC)
>> - goto unreserve_bo;
>> - r = 0;
>> - }
>> + r = 0;
>> /* Check if PD needs to be reinitialized and do it before
>> * changing any other state, in case it fails.
>> @@ -3088,19 +3144,7 @@ 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;
>> - }
>> + amdgpu_vm_pasid_free(adev, vm);
>> /* Free the shadow bo for compute VM */
>> amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
>> @@ -3111,13 +3155,8 @@ int amdgpu_vm_make_compute(struct
>> amdgpu_device *adev, struct amdgpu_vm *vm,
>> goto unreserve_bo;
>> free_idr:
>> - if (pasid) {
>> - unsigned long flags;
>> + amdgpu_vm_pasid_remove_id(adev, pasid);
>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> - idr_remove(&adev->vm_manager.pasid_idr, pasid);
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> - }
>> unreserve_bo:
>> amdgpu_bo_unreserve(vm->root.bo);
>> return r;
>> @@ -3133,14 +3172,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_pasid_remove(adev, vm);
>> vm->is_compute_context = false;
>> }
>> @@ -3164,15 +3196,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_pasid_remove(adev, vm);
>> dma_fence_wait(vm->last_unlocked, false);
>> dma_fence_put(vm->last_unlocked);
>> @@ -3334,16 +3358,10 @@ int amdgpu_vm_ioctl(struct drm_device *dev,
>> void *data, struct drm_file *filp)
>> void amdgpu_vm_get_task_info(struct amdgpu_device *adev, u32 pasid,
>> struct amdgpu_task_info *task_info)
>> {
>> - struct amdgpu_vm *vm;
>> - unsigned long flags;
>> + struct amdgpu_vm *vm = amdgpu_vm_pasid_find(adev, pasid);
>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
>> -
>> - vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
>> if (vm)
>> *task_info = vm->task_info;
>> -
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
>> }
>> /**
>> @@ -3380,24 +3398,16 @@ bool amdgpu_vm_handle_fault(struct
>> amdgpu_device *adev, u32 pasid,
>> {
>> bool is_compute_context = false;
>> struct amdgpu_bo *root;
>> - unsigned long irqflags;
>> uint64_t value, flags;
>> struct amdgpu_vm *vm;
>> int r;
>> - spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
>> - vm = idr_find(&adev->vm_manager.pasid_idr, 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);
>> -
>> - if (!root)
>> + vm = amdgpu_vm_pasid_find(adev, pasid);
>> + if (!vm)
>> return false;
>> + root = amdgpu_bo_ref(vm->root.bo);
>> + is_compute_context = vm->is_compute_context;
>> addr /= AMDGPU_GPU_PAGE_SIZE;
>> if (is_compute_context &&
>> @@ -3411,12 +3421,8 @@ 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);
>> - if (vm && vm->root.bo != root)
>> - vm = NULL;
>> - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
>> - if (!vm)
>> + vm = amdgpu_vm_pasid_find(adev, pasid);
>> + if (!vm || (vm && vm->root.bo != root))
>> goto error_unlock;
>> flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED |
>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> index ddb85a85cbba..ee994e21dffa 100644
>> --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
>> @@ -376,7 +376,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device
>> *adev);
>> void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
>> 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_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
>> int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct
>> amdgpu_vm *vm, u32 pasid);
>> 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);
More information about the amd-gfx
mailing list