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

[Felix Kuehling <felix.kuehling@xxxxxxx>]

Am 2022-03-18 um 08:38 schrieb Christian König:
> 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?

Before resuming user mode queues after an eviction, KFD currently 
updates all the BOs and their mappings that it knows about. But it 
doesn't know about the mappings made using the render node API. So my 
plan was to use amdgpu_vm_handle_moved for that. But I don't get any 
fences for the page table operations queues by amdgpu_vm_handle_moved. I 
think amdgpu_cs has the same problem. So how do I reliably wait for 
those to finish before I resume user mode queues?

If amdgpu_vm_handle_moved were able to update the sync object, then I 
also wouldn't need explicit amdgpu_vm_bo_update calls any more, similar 
to what I suggested in the TODO comment in amdgpu_cs_vm_handling.

Regards,
  Felix


> 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,