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

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

On 6/21/2021 12:26 PM, Christian König wrote:
> Well you completely break the handling in amdgpu_vm_handle_fault() 
> with this.


I see one issue with:  if (!vm || (vm && vm->root.bo != root)) . I will 
split it and resend.

Am I missing something else ?


Regards,

Nirmoy



> Christian.
>
> Am 21.06.21 um 11:47 schrieb Das, Nirmoy:
> > ping.
> >
> > On 6/17/2021 3:03 PM, Nirmoy Das wrote:
> > > Cleanup code related to vm pasid by adding helper function.
> > > This reduces lots code duplication.
> > >
> > > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c |  17 +--
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c  | 176 
> > > ++++++++++++------------
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h  |   2 +-
> > >   3 files changed, 96 insertions(+), 99 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > index cbb932f97355..27851fb0e25b 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_kms.c
> > > @@ -1149,7 +1149,7 @@ int amdgpu_driver_open_kms(struct drm_device 
> > > *dev, struct drm_file *file_priv)
> > >   {
> > >       struct amdgpu_device *adev = drm_to_adev(dev);
> > >       struct amdgpu_fpriv *fpriv;
> > > -    int r, pasid;
> > > +    int r;
> > >         /* Ensure IB tests are run on ring */
> > >       flush_delayed_work(&adev->delayed_init_work);
> > > @@ -1172,15 +1172,9 @@ int amdgpu_driver_open_kms(struct drm_device 
> > > *dev, struct drm_file *file_priv)
> > >           goto out_suspend;
> > >       }
> > >   -    pasid = amdgpu_pasid_alloc(16);
> > > -    if (pasid < 0) {
> > > -        dev_warn(adev->dev, "No more PASIDs available!");
> > > -        pasid = 0;
> > > -    }
> > > -
> > > -    r = amdgpu_vm_init(adev, &fpriv->vm, pasid);
> > > +    r = amdgpu_vm_init(adev, &fpriv->vm);
> > >       if (r)
> > > -        goto error_pasid;
> > > +        goto free_fpriv;
> > >         fpriv->prt_va = amdgpu_vm_bo_add(adev, &fpriv->vm, NULL);
> > >       if (!fpriv->prt_va) {
> > > @@ -1208,10 +1202,7 @@ int amdgpu_driver_open_kms(struct drm_device 
> > > *dev, struct drm_file *file_priv)
> > >   error_vm:
> > >       amdgpu_vm_fini(adev, &fpriv->vm);
> > >   -error_pasid:
> > > -    if (pasid)
> > > -        amdgpu_pasid_free(pasid);
> > > -
> > > +free_fpriv:
> > >       kfree(fpriv);
> > >     out_suspend:
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > index 63975bda8e76..562c2c48a3a3 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -87,6 +87,69 @@ 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 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;
> > > +
> > > +    vm->pasid = pasid;
> > > +    return 0;
> > > +}
> > > +static void amdgpu_vm_pasid_remove_id(struct amdgpu_device *adev,
> > > +                      unsigned int 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);
> > > +
> > > +}
> > > +
> > > +static void amdgpu_vm_pasid_remove(struct amdgpu_device *adev,
> > > +                   struct amdgpu_vm *vm)
> > > +{
> > > +    amdgpu_vm_pasid_remove_id(adev, vm->pasid);
> > > +    vm->pasid = 0;
> > > +}
> > > +
> > > +static void amdgpu_vm_pasid_free(struct amdgpu_device *adev,
> > > +                 struct amdgpu_vm *vm)
> > > +{
> > > +    if (!vm->pasid)
> > > +        return;
> > > +
> > > +    amdgpu_pasid_free(vm->pasid);
> > > +    amdgpu_vm_pasid_remove(adev, vm);
> > > +}
> > > +
> > > +static struct amdgpu_vm *amdgpu_vm_pasid_find(struct amdgpu_device 
> > > *adev,
> > > +                          unsigned int pasid)
> > > +{
> > > +    struct amdgpu_vm *vm;
> > > +    unsigned long flags;
> > > +
> > > +    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> > > +    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > > + spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> > > +
> > > +    return vm;
> > > +}
> > > +
> > >   /*
> > >    * vm eviction_lock can be taken in MMU notifiers. Make sure no 
> > > reclaim-FS
> > >    * happens while holding this lock anywhere to prevent deadlocks when
> > > @@ -2859,17 +2922,17 @@ long amdgpu_vm_wait_idle(struct amdgpu_vm 
> > > *vm, long timeout)
> > >    *
> > >    * @adev: amdgpu_device pointer
> > >    * @vm: requested vm
> > > - * @pasid: Process address space identifier
> > >    *
> > >    * Init @vm fields.
> > >    *
> > >    * Returns:
> > >    * 0 for success, error for failure.
> > >    */
> > > -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm 
> > > *vm, u32 pasid)
> > > +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm)
> > >   {
> > >       struct amdgpu_bo *root_bo;
> > >       struct amdgpu_bo_vm *root;
> > > +    unsigned int pasid;
> > >       int r, i;
> > >         vm->va = RB_ROOT_CACHED;
> > > @@ -2940,19 +3003,15 @@ 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;
> > > +    pasid = amdgpu_pasid_alloc(16);
> > > +    if (pasid < 0) {
> > > +        dev_warn(adev->dev, "No more PASIDs available!");
> > > +        pasid = 0;
> > >       }
> > >   +    if (amdgpu_vm_pasid_alloc(adev, vm, pasid))
> > > +        goto error_free_pasid;
> > > +
> > >       INIT_KFIFO(vm->faults);
> > >         return 0;
> > > @@ -2960,6 +3019,10 @@ int amdgpu_vm_init(struct amdgpu_device 
> > > *adev, struct amdgpu_vm *vm, u32 pasid)
> > >   error_unreserve:
> > >       amdgpu_bo_unreserve(vm->root.bo);
> > >   +error_free_pasid:
> > > +    if (pasid)
> > > +        amdgpu_pasid_free(pasid);
> > > +
> > >   error_free_root:
> > >       amdgpu_bo_unref(&root->shadow);
> > >       amdgpu_bo_unref(&root_bo);
> > > @@ -3039,18 +3102,11 @@ int amdgpu_vm_make_compute(struct 
> > > amdgpu_device *adev, struct amdgpu_vm *vm,
> > >       if (r)
> > >           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);
> > > +    r = amdgpu_vm_pasid_alloc(adev, vm, pasid);
> > > +    if (r ==  -ENOSPC)
> > > +        goto unreserve_bo;
> > >   -        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.
> > > @@ -3088,19 +3144,7 @@ int amdgpu_vm_make_compute(struct 
> > > amdgpu_device *adev, struct amdgpu_vm *vm,
> > >       vm->last_update = NULL;
> > >       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_free(adev, vm);
> > >         /* Free the shadow bo for compute VM */
> > > amdgpu_bo_unref(&to_amdgpu_bo_vm(vm->root.bo)->shadow);
> > > @@ -3111,13 +3155,8 @@ int amdgpu_vm_make_compute(struct 
> > > amdgpu_device *adev, struct amdgpu_vm *vm,
> > >       goto unreserve_bo;
> > >     free_idr:
> > > -    if (pasid) {
> > > -        unsigned long flags;
> > > +    amdgpu_vm_pasid_remove_id(adev, pasid);
> > >   - 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 +3172,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);
> > >       vm->is_compute_context = false;
> > >   }
> > >   @@ -3164,15 +3196,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);
> > >       dma_fence_wait(vm->last_unlocked, false);
> > >       dma_fence_put(vm->last_unlocked);
> > >   @@ -3334,16 +3358,10 @@ int amdgpu_vm_ioctl(struct drm_device 
> > > *dev, void *data, struct drm_file *filp)
> > >   void amdgpu_vm_get_task_info(struct amdgpu_device *adev, u32 pasid,
> > >                struct amdgpu_task_info *task_info)
> > >   {
> > > -    struct amdgpu_vm *vm;
> > > -    unsigned long flags;
> > > +    struct amdgpu_vm *vm = amdgpu_vm_pasid_find(adev, pasid);
> > >   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> > > -
> > > -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > >       if (vm)
> > >           *task_info = vm->task_info;
> > > -
> > > - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> > >   }
> > >     /**
> > > @@ -3380,24 +3398,16 @@ bool amdgpu_vm_handle_fault(struct 
> > > amdgpu_device *adev, u32 pasid,
> > >   {
> > >       bool is_compute_context = false;
> > >       struct amdgpu_bo *root;
> > > -    unsigned long irqflags;
> > >       uint64_t value, flags;
> > >       struct amdgpu_vm *vm;
> > >       int r;
> > >   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
> > > -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > > -    if (vm) {
> > > -        root = amdgpu_bo_ref(vm->root.bo);
> > > -        is_compute_context = vm->is_compute_context;
> > > -    } else {
> > > -        root = NULL;
> > > -    }
> > > - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
> > > -
> > > -    if (!root)
> > > +    vm = amdgpu_vm_pasid_find(adev, pasid);
> > > +    if (!vm)
> > >           return false;
> > >   +    root = amdgpu_bo_ref(vm->root.bo);
> > > +        is_compute_context = vm->is_compute_context;
> > >       addr /= AMDGPU_GPU_PAGE_SIZE;
> > >         if (is_compute_context &&
> > > @@ -3411,12 +3421,8 @@ bool amdgpu_vm_handle_fault(struct 
> > > amdgpu_device *adev, u32 pasid,
> > >           goto error_unref;
> > >         /* Double check that the VM still exists */
> > > -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
> > > -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > > -    if (vm && vm->root.bo != root)
> > > -        vm = NULL;
> > > - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
> > > -    if (!vm)
> > > +    vm = amdgpu_vm_pasid_find(adev, pasid);
> > > +    if (!vm || (vm && vm->root.bo != root))
> > >           goto error_unlock;
> > >         flags = AMDGPU_PTE_VALID | AMDGPU_PTE_SNOOPED |
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > index ddb85a85cbba..ee994e21dffa 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > @@ -376,7 +376,7 @@ void amdgpu_vm_manager_init(struct amdgpu_device 
> > > *adev);
> > >   void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
> > >     long amdgpu_vm_wait_idle(struct amdgpu_vm *vm, long timeout);
> > > -int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm 
> > > *vm, u32 pasid);
> > > +int amdgpu_vm_init(struct amdgpu_device *adev, struct amdgpu_vm *vm);
> > >   int amdgpu_vm_make_compute(struct amdgpu_device *adev, struct 
> > > amdgpu_vm *vm, u32 pasid);
> > >   void amdgpu_vm_release_compute(struct amdgpu_device *adev, struct 
> > > amdgpu_vm *vm);
> > >   void amdgpu_vm_fini(struct amdgpu_device *adev, struct amdgpu_vm 
> > > *vm);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx