Re: [RFC PATCH 1/4] drm/amdkfd: Improve amdgpu_vm_handle_moved

[Christian König <christian.koenig@xxxxxxx>]

Am 17.03.22 um 20:11 schrieb Felix Kuehling:
> Am 2022-03-17 um 04:21 schrieb Christian König:
> > Am 17.03.22 um 01:20 schrieb Felix Kuehling:
> > > Let amdgpu_vm_handle_moved update all BO VA mappings of BOs reserved by
> > > the caller. This will be useful for handling extra BO VA mappings in
> > > KFD VMs that are managed through the render node API.
> >
> > Yes, that change is on my TODO list for quite a while as well.
> >
> > > TODO: This may also allow simplification of amdgpu_cs_vm_handling. See
> > > the TODO comment in the code.
> >
> > No, that won't work just yet.
> >
> > We need to change the TLB flush detection for that, but I'm already 
> > working on those as well.
>
> Your TLB flushing patch series looks good to me.
>
> There is one other issue, though. amdgpu_vm_handle_moved doesn't 
> update the sync object, so I couldn't figure out I can wait for all 
> the page table updates to finish.

Yes, and inside the CS we still need to go over all the BOs and gather 
the VM updates to wait for.

Not sure if you can do that in the KFD code as well. How exactly do you 
want to use it?

Regards,
Christian.

> Regards,
>   Felix
>
>
> > > Signed-off-by: Felix Kuehling <Felix.Kuehling@xxxxxxx>
> >
> > Please update the TODO, with that done: Reviewed-by: Christian König 
> > <christian.koenig@xxxxxxx>
> >
> > > ---
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c      |  6 +++++-
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c |  2 +-
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 18 +++++++++++++-----
> > >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      |  3 ++-
> > >   4 files changed, 21 insertions(+), 8 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > index d162243d8e78..10941f0d8dde 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_cs.c
> > > @@ -826,6 +826,10 @@ static int amdgpu_cs_vm_handling(struct 
> > > amdgpu_cs_parser *p)
> > >               return r;
> > >       }
> > >   +    /* TODO: Is this loop still needed, or could this be handled by
> > > +     * amdgpu_vm_handle_moved, now that it can handle all BOs that are
> > > +     * reserved under p->ticket?
> > > +     */
> > >       amdgpu_bo_list_for_each_entry(e, p->bo_list) {
> > >           /* ignore duplicates */
> > >           bo = ttm_to_amdgpu_bo(e->tv.bo);
> > > @@ -845,7 +849,7 @@ static int amdgpu_cs_vm_handling(struct 
> > > amdgpu_cs_parser *p)
> > >               return r;
> > >       }
> > >   -    r = amdgpu_vm_handle_moved(adev, vm);
> > > +    r = amdgpu_vm_handle_moved(adev, vm, &p->ticket);
> > >       if (r)
> > >           return r;
> > >   diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > > index 579adfafe4d0..50805613c38c 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_dma_buf.c
> > > @@ -414,7 +414,7 @@ amdgpu_dma_buf_move_notify(struct 
> > > dma_buf_attachment *attach)
> > >             r = amdgpu_vm_clear_freed(adev, vm, NULL);
> > >           if (!r)
> > > -            r = amdgpu_vm_handle_moved(adev, vm);
> > > +            r = amdgpu_vm_handle_moved(adev, vm, ticket);
> > >             if (r && r != -EBUSY)
> > >               DRM_ERROR("Failed to invalidate VM page tables (%d))\n",
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > index fc4563cf2828..726b42c6d606 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > > @@ -2190,11 +2190,12 @@ int amdgpu_vm_clear_freed(struct 
> > > amdgpu_device *adev,
> > >    * PTs have to be reserved!
> > >    */
> > >   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> > > -               struct amdgpu_vm *vm)
> > > +               struct amdgpu_vm *vm,
> > > +               struct ww_acquire_ctx *ticket)
> > >   {
> > >       struct amdgpu_bo_va *bo_va, *tmp;
> > >       struct dma_resv *resv;
> > > -    bool clear;
> > > +    bool clear, unlock;
> > >       int r;
> > >         list_for_each_entry_safe(bo_va, tmp, &vm->moved, 
> > > base.vm_status) {
> > > @@ -2212,17 +2213,24 @@ int amdgpu_vm_handle_moved(struct 
> > > amdgpu_device *adev,
> > >           spin_unlock(&vm->invalidated_lock);
> > >             /* Try to reserve the BO to avoid clearing its ptes */
> > > -        if (!amdgpu_vm_debug && dma_resv_trylock(resv))
> > > +        if (!amdgpu_vm_debug && dma_resv_trylock(resv)) {
> > >               clear = false;
> > > +            unlock = true;
> > > +        /* The caller is already holding the reservation lock */
> > > +        } else if (ticket && dma_resv_locking_ctx(resv) == ticket) {
> > > +            clear = false;
> > > +            unlock = false;
> > >           /* Somebody else is using the BO right now */
> > > -        else
> > > +        } else {
> > >               clear = true;
> > > +            unlock = false;
> > > +        }
> > >             r = amdgpu_vm_bo_update(adev, bo_va, clear, NULL);
> > >           if (r)
> > >               return r;
> > >   -        if (!clear)
> > > +        if (unlock)
> > >               dma_resv_unlock(resv);
> > >           spin_lock(&vm->invalidated_lock);
> > >       }
> > > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> > > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > index a40a6a993bb0..120a76aaae75 100644
> > > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > > @@ -396,7 +396,8 @@ int amdgpu_vm_clear_freed(struct amdgpu_device 
> > > *adev,
> > >                 struct amdgpu_vm *vm,
> > >                 struct dma_fence **fence);
> > >   int amdgpu_vm_handle_moved(struct amdgpu_device *adev,
> > > -               struct amdgpu_vm *vm);
> > > +               struct amdgpu_vm *vm,
> > > +               struct ww_acquire_ctx *ticket);
> > >   int amdgpu_vm_bo_update_mapping(struct amdgpu_device *adev,
> > >                   struct amdgpu_device *bo_adev,
> > >                   struct amdgpu_vm *vm, bool immediate,