Re: [PATCH 1/1] drm/amdgpu: add helper function for vm pasid

["Das, Nirmoy" <nirmoy.das@amd.com>]

[-- Attachment #1.1: Type: text/plain, Size: 8975 bytes --]


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@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. 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)

}

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 ?


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@lists.freedesktop.org
>> https://nam11.safelinks.protection.outlook.com/?url=https%3A%2F%2Flists.freedesktop.org%2Fmailman%2Flistinfo%2Famd-gfx&amp;data=04%7C01%7Cnirmoy.das%40amd.com%7C3285a973b5a4498f3b0608d93558d909%7C3dd8961fe4884e608e11a82d994e183d%7C0%7C0%7C637599478002028860%7CUnknown%7CTWFpbGZsb3d8eyJWIjoiMC4wLjAwMDAiLCJQIjoiV2luMzIiLCJBTiI6Ik1haWwiLCJXVCI6Mn0%3D%7C1000&amp;sdata=QLv4hbUpPF9H%2BVL4eOQlTeROWQA%2FG1LrPGFBzCQRt7o%3D&amp;reserved=0 
>>
>

[-- Attachment #1.2: Type: text/html, Size: 48863 bytes --]

[-- Attachment #2: Type: text/plain, Size: 154 bytes --]

_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx