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

[Christian König <ckoenig.leichtzumerken@xxxxxxxxx>]

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

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

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