Re: [PATCH 1/2] drm/amdgpu: use xarray for storing pasid in vm

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

On 6/29/2021 1:10 PM, Christian König wrote:
> Am 29.06.21 um 09:49 schrieb Nirmoy Das:
> > Replace idr with xarray as we actually need hash functionality.
> > Cleanup code related to vm pasid by adding helper function.
> >
> > Signed-off-by: Nirmoy Das <nirmoy.das@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 136 ++++++++++---------------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h |   6 +-
> >   2 files changed, 60 insertions(+), 82 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index 63975bda8e76..9b0e8a9d7f86 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -87,6 +87,33 @@ struct amdgpu_prt_cb {
> >       struct dma_fence_cb cb;
> >   };
> >   +int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct 
> > amdgpu_vm *vm,
> > +            unsigned long pasid)
>
> Proper kerneldoc please when the function is now exported.


I had one, missed it this revision :/


> > +{
> > +    unsigned long flags;
> > +    int r;
> > +
> > +    if (vm->pasid == pasid)
> > +        return 0;
> > +
> > +    if (vm->pasid) {
> > +        r = xa_err(xa_erase_irq(&adev->vm_manager.pasids, vm->pasid));
> > +        if (r < 0)
> > +            return r;
>
> I think we should have a "vm->pasid = 0;" here so that we have a 
> consistent state when inserting the new pasid below failed.
>
>
> > +    }
> > +
> > +    if (pasid) {
> > +        r = xa_err(xa_store_irq(&adev->vm_manager.pasids, pasid, vm,
> > +                    GFP_KERNEL));
> > +        if (r < 0)
> > +            return r;
> > +    }
> > +
> > +    vm->pasid = pasid;
>
> This assignment can then be moved into the "if" as well.
>
> Apart from that looks like a really nice cleanup to me.


Thanks, I will resend.

> Regards,
> Christian.
>
> > +
> > +    return 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 +2967,9 @@ 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;
> > -    }
> > +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
> > +    if (r)
> > +        goto error_free_root;
> >         INIT_KFIFO(vm->faults);
> >   @@ -3039,18 +3057,15 @@ 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);
> > +    /* Free the original amdgpu allocated pasid,
> > +     * will be replaced with kfd allocated pasid.
> > +     */
> > +    if (vm->pasid)
> > +        amdgpu_pasid_free(vm->pasid);
> >   -        if (r == -ENOSPC)
> > -            goto unreserve_bo;
> > -        r = 0;
> > -    }
> > +    r = amdgpu_vm_set_pasid(adev, vm, pasid);
> > +    if (r)
> > +        goto unreserve_bo;
> >         /* Check if PD needs to be reinitialized and do it before
> >        * changing any other state, in case it fails.
> > @@ -3061,7 +3076,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device 
> > *adev, struct amdgpu_vm *vm,
> >                          to_amdgpu_bo_vm(vm->root.bo),
> >                          false);
> >           if (r)
> > -            goto free_idr;
> > +            goto free_pasid_entry;
> >       }
> >         /* Update VM state */
> > @@ -3078,7 +3093,7 @@ int amdgpu_vm_make_compute(struct amdgpu_device 
> > *adev, struct amdgpu_vm *vm,
> >           r = amdgpu_bo_sync_wait(vm->root.bo,
> >                       AMDGPU_FENCE_OWNER_UNDEFINED, true);
> >           if (r)
> > -            goto free_idr;
> > +            goto free_pasid_entry;
> >             vm->update_funcs = &amdgpu_vm_cpu_funcs;
> >       } else {
> > @@ -3088,36 +3103,13 @@ 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;
> > -    }
> > -
> >       /* 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;
> > -
> > -        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);
> > -    }
> > +free_pasid_entry:
> > +    amdgpu_vm_set_pasid(adev, vm, 0);
> >   unreserve_bo:
> >       amdgpu_bo_unreserve(vm->root.bo);
> >       return r;
> > @@ -3133,14 +3125,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_set_pasid(adev, vm, 0);
> >       vm->is_compute_context = false;
> >   }
> >   @@ -3164,15 +3149,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_set_pasid(adev, vm, 0);
> >       dma_fence_wait(vm->last_unlocked, false);
> >       dma_fence_put(vm->last_unlocked);
> >   @@ -3254,8 +3231,7 @@ void amdgpu_vm_manager_init(struct 
> > amdgpu_device *adev)
> >       adev->vm_manager.vm_update_mode = 0;
> >   #endif
> >   -    idr_init(&adev->vm_manager.pasid_idr);
> > -    spin_lock_init(&adev->vm_manager.pasid_lock);
> > +    xa_init_flags(&adev->vm_manager.pasids, XA_FLAGS_LOCK_IRQ);
> >   }
> >     /**
> > @@ -3267,8 +3243,8 @@ void amdgpu_vm_manager_init(struct 
> > amdgpu_device *adev)
> >    */
> >   void amdgpu_vm_manager_fini(struct amdgpu_device *adev)
> >   {
> > -    WARN_ON(!idr_is_empty(&adev->vm_manager.pasid_idr));
> > -    idr_destroy(&adev->vm_manager.pasid_idr);
> > +    WARN_ON(!xa_empty(&adev->vm_manager.pasids));
> > +    xa_destroy(&adev->vm_manager.pasids);
> >         amdgpu_vmid_mgr_fini(adev);
> >   }
> > @@ -3337,13 +3313,13 @@ void amdgpu_vm_get_task_info(struct 
> > amdgpu_device *adev, u32 pasid,
> >       struct amdgpu_vm *vm;
> >       unsigned long flags;
> >   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, flags);
> > +    xa_lock_irqsave(&adev->vm_manager.pasids, flags);
> >   -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > +    vm = xa_load(&adev->vm_manager.pasids, pasid);
> >       if (vm)
> >           *task_info = vm->task_info;
> >   - spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, flags);
> > +    xa_unlock_irqrestore(&adev->vm_manager.pasids, flags);
> >   }
> >     /**
> > @@ -3385,15 +3361,15 @@ bool amdgpu_vm_handle_fault(struct 
> > amdgpu_device *adev, u32 pasid,
> >       struct amdgpu_vm *vm;
> >       int r;
> >   -    spin_lock_irqsave(&adev->vm_manager.pasid_lock, irqflags);
> > -    vm = idr_find(&adev->vm_manager.pasid_idr, pasid);
> > +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> > +    vm = xa_load(&adev->vm_manager.pasids, 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);
> > +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> >         if (!root)
> >           return false;
> > @@ -3411,11 +3387,11 @@ 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);
> > +    xa_lock_irqsave(&adev->vm_manager.pasids, irqflags);
> > +    vm = xa_load(&adev->vm_manager.pasids, pasid);
> >       if (vm && vm->root.bo != root)
> >           vm = NULL;
> > -    spin_unlock_irqrestore(&adev->vm_manager.pasid_lock, irqflags);
> > +    xa_unlock_irqrestore(&adev->vm_manager.pasids, irqflags);
> >       if (!vm)
> >           goto error_unlock;
> >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index ddb85a85cbba..8e8bc132e590 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -359,8 +359,7 @@ struct amdgpu_vm_manager {
> >       /* PASID to VM mapping, will be used in interrupt context to
> >        * look up VM of a page fault
> >        */
> > -    struct idr                pasid_idr;
> > -    spinlock_t                pasid_lock;
> > +    struct xarray                pasids;
> >   };
> >     struct amdgpu_bo_va_mapping;
> > @@ -375,6 +374,9 @@ extern const struct amdgpu_vm_update_funcs 
> > amdgpu_vm_sdma_funcs;
> >   void amdgpu_vm_manager_init(struct amdgpu_device *adev);
> >   void amdgpu_vm_manager_fini(struct amdgpu_device *adev);
> >   +int amdgpu_vm_set_pasid(struct amdgpu_device *adev, struct 
> > amdgpu_vm *vm,
> > +            unsigned long pasid);
> > +
> >   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_make_compute(struct amdgpu_device *adev, struct 
> > amdgpu_vm *vm, u32 pasid);
_______________________________________________
amd-gfx mailing list
amd-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/amd-gfx