[PATCH 1/1] drm/amdgpu: add helper function for vm pasid
Christian König
ckoenig.leichtzumerken at gmail.com
Tue Jun 22 10:36:16 UTC 2021
Am 22.06.21 um 12:30 schrieb Das, Nirmoy:
>
> On 6/22/2021 10:36 AM, Christian König wrote:
>
>> Am 22.06.21 um 09:39 schrieb Das, Nirmoy:
>>>
>>> On 6/22/2021 9:03 AM, Christian König wrote:
>>>>
>>>>
>>>> Am 22.06.21 um 08:57 schrieb Nirmoy Das:
>>>>> Cleanup code related to vm pasid by adding helper functions.
>>>>>
>>>>> Signed-off-by: Nirmoy Das <nirmoy.das at amd.com>
>>>>> ---
>>>>> drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 105
>>>>> ++++++++++++-------------
>>>>> 1 file changed, 50 insertions(+), 55 deletions(-)
>>>>>
>>>>> diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
>>>>> index 63975bda8e76..6e476b173cbb 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;
>>>>> };
>>>>>
>>>>> +static int amdgpu_vm_pasid_alloc(struct amdgpu_device *adev,
>>>>> + struct amdgpu_vm *vm,
>>>>> + unsigned int pasid,
>>>>> + unsigned int *vm_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;
>>>>> + if (vm_pasid)
>>>>> + *vm_pasid = pasid;
>>>>> +
>>>>
>>>> Ok the more I read from this patch the less it makes sense.
>>>>
>>>> We don't allocate the passid here, we just set it up in the idr.
>>>>
>>>> What we could do is to replace the idr with an xarray, that would
>>>> certainly make more sense than this here.
>>>
>>>
>>> xarray looks great, with that we don't need pasid_lock either.
>>
>> You still need the lock to protect against VM destruction while
>> looking things up, but you could switch to RCU for this instead.
>
>
> xarray has xa_{lock|unloack}_irqsave() and adev->vm_manager.pasid_xa
> will exist till devices's lifetime.
That's just a wrapper around the lock.
> So I am thinking something like:
>
> amdgpu_vm_pasid_insert()
>
> {
>
> ...
>
> xa_lock_irqsave(adev->vm_manager.pasids, flags)
> r = xa_store(&adev->vm_manager.pasids, pasid, vm, GFP_ATOMIC);
> xa_unlock_irqsave(adev->vm_manager.pasids, flags)
It would be really nice if we could avoid the GFP_ATOMIC here, but not
much of a problem since we had that before.
> }
>
> amdgpu_vm_pasid_remove()
>
> {
>
> ....
>
> xa_lock_irqsave(adev->vm_manager.pasids, flags)
> xa_erase(&adev->vm_manager.pasids, pasid);
> xa_unlock_irqsave(adev->vm_manager.pasids, flags)
>
> }
>
>
> xa_{lock|unloack}_irqsave() can be use while looking up vm ptr for a
> pasid.
>
>
> Shouldn't this be enough ?
>
Yeah I think so.
Christian.
>
> Regards,
>
> Nirmoy
>
>>
>> Christian.
>>
>>>
>>>
>>> Thanks
>>>
>>> Nirmoy
>>>
>>>
>>>>
>>>> Christian.
>>>>
>>>>> + return 0;
>>>>> +}
>>>>> +
>>>>> +static void amdgpu_vm_pasid_remove(struct amdgpu_device *adev,
>>>>> + unsigned int pasid,
>>>>> + unsigned int *vm_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);
>>>>> +
>>>>> + if (vm_pasid)
>>>>> + *vm_pasid = 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,8 @@ 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;
>>>>> - }
>>>>> + if (amdgpu_vm_pasid_alloc(adev, vm, pasid, &vm->pasid))
>>>>> + goto error_free_root;
>>>>>
>>>>> INIT_KFIFO(vm->faults);
>>>>>
>>>>> @@ -3038,19 +3068,11 @@ int amdgpu_vm_make_compute(struct
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>> r = amdgpu_vm_check_clean_reserved(adev, vm);
>>>>> if (r)
>>>>> goto unreserve_bo;
>>>>> + r = amdgpu_vm_pasid_alloc(adev, vm, pasid, NULL);
>>>>> + if (r == -ENOSPC)
>>>>> + 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);
>>>>> -
>>>>> - 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.
>>>>> @@ -3089,35 +3111,23 @@ int amdgpu_vm_make_compute(struct
>>>>> amdgpu_device *adev, struct amdgpu_vm *vm,
>>>>> 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_remove(adev, vm->pasid, &vm->pasid);
>>>>> }
>>>>>
>>>>> /* 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;
>>>>> + amdgpu_vm_pasid_remove(adev, pasid, NULL);
>>>>>
>>>>> - 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 +3143,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->pasid, &vm->pasid);
>>>>> vm->is_compute_context = false;
>>>>> }
>>>>>
>>>>> @@ -3164,15 +3167,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->pasid, &vm->pasid);
>>>>> dma_fence_wait(vm->last_unlocked, false);
>>>>> dma_fence_put(vm->last_unlocked);
>>>>>
>>>>> --
>>>>> 2.32.0
>>>>>
>>>>
>>> _______________________________________________
>>> amd-gfx mailing list
>>> amd-gfx at 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%7C3285a973b5a4498f3b0608d93558d909%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599478002028860%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&sdata=QLv4hbUpPF9H%2BVL4eOQlTeROWQA%2FG1LrPGFBzCQRt7o%3D&reserved=0
>>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20210622/8ba00744/attachment-0001.htm>
More information about the amd-gfx
mailing list