[PATCH 1/1] drm/amdgpu: add helper function for vm pasid
Christian König
christian.koenig at amd.com
Mon Jun 21 10:59:45 UTC 2021
Am 21.06.21 um 12:56 schrieb Das, Nirmoy:
>
> On 6/21/2021 12:26 PM, Christian König wrote:
>> Well you completely break the handling in amdgpu_vm_handle_fault()
>> with this.
>
>
> I see one issue with: if (!vm || (vm && vm->root.bo != root)) . I
> will split it and resend.
>
> Am I missing something else ?
The problem is you drop and re-take the lock now at the wrong time.
I don't think what you try to do here is possible at all.
Christian.
>
>
> Regards,
>
> Nirmoy
>
>
>
>>
>> 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