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

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

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.


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://lists.freedesktop.org/mailman/listinfo/amd-gfx