[PATCH 1/1] drm/amdgpu: add helper function for vm pasid
Das, Nirmoy
nirmoy.das at amd.com
Tue Jun 22 10:51:24 UTC 2021
On 6/22/2021 12:36 PM, Christian König wrote:
> 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.
I think it is possible as I think only amdgpu_vm_handle_fault() runs in
interrupt context which tries to find a vm ptr not store it.
>
>> }
>>
>> 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.
Great.
Nirmoy
>
> 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/9500b0c6/attachment-0001.htm>
More information about the amd-gfx
mailing list