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

["Christian König" <ckoenig.leichtzumerken@gmail.com>]

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

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@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@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: 48290 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