Re: [PATCH 1/3] drm/amdgpu: re-work VM syncing

[Friedrich Vock <friedrich.vock@xxxxxx>]

On 21.08.24 22:46, Felix Kuehling wrote:
> On 2024-08-21 08:03, Christian König wrote:
> > Rework how VM operations synchronize to submissions. Provide an
> > amdgpu_sync container to the backends instead of an reservation
> > object and fill in the amdgpu_sync object in the higher layers
> > of the code.
> >
> > No intended functional change, just prepares for upcomming changes.
> >
> > Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> > ---
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c      | 84 +++++++++++++--------
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h      | 11 +--
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c  |  7 +-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c   |  2 +-
> >   drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c | 16 +---
>
> There are two calls to amdgpu_vm_update_range in amdkfd/kfd_svm.c that 
> would need to be updated as well.

I don't think any change should be needed there? Both calls pass NULL 
for the resv. All this patch changes is that we're now passing NULL for 
the amdgpu_sync - but the behavior with a NULL amdgpu_sync with this 
patch is the same as with a NULL dma_resv without this patch, so nothing 
needs to change.

Regards,
Friedrich

> Regards,
>    Felix
>
>
> >   5 files changed, 65 insertions(+), 55 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index bcb729094521..ba99d428610a 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -838,7 +838,7 @@ int amdgpu_vm_update_pdes(struct amdgpu_device *adev,
> >       params.vm = vm;
> >       params.immediate = immediate;
> > -    r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
> > +    r = vm->update_funcs->prepare(&params, NULL);
> >       if (r)
> >           goto error;
> > @@ -933,7 +933,7 @@ amdgpu_vm_tlb_flush(struct amdgpu_vm_update_params 
> > *params,
> >    * @unlocked: unlocked invalidation during MM callback
> >    * @flush_tlb: trigger tlb invalidation after update completed
> >    * @allow_override: change MTYPE for local NUMA nodes
> > - * @resv: fences we need to sync to
> > + * @sync: fences we need to sync to
> >    * @start: start of mapped range
> >    * @last: last mapped entry
> >    * @flags: flags for the entries
> > @@ -949,16 +949,16 @@ amdgpu_vm_tlb_flush(struct 
> > amdgpu_vm_update_params *params,
> >    * 0 for success, negative erro code for failure.
> >    */
> >   int amdgpu_vm_update_range(struct amdgpu_device *adev, struct 
> > amdgpu_vm *vm,
> > -               bool immediate, bool unlocked, bool flush_tlb, bool 
> > allow_override,
> > -               struct dma_resv *resv, uint64_t start, uint64_t last,
> > -               uint64_t flags, uint64_t offset, uint64_t vram_base,
> > +               bool immediate, bool unlocked, bool flush_tlb,
> > +               bool allow_override, struct amdgpu_sync *sync,
> > +               uint64_t start, uint64_t last, uint64_t flags,
> > +               uint64_t offset, uint64_t vram_base,
> >                  struct ttm_resource *res, dma_addr_t *pages_addr,
> >                  struct dma_fence **fence)
> >   {
> >       struct amdgpu_vm_tlb_seq_struct *tlb_cb;
> >       struct amdgpu_vm_update_params params;
> >       struct amdgpu_res_cursor cursor;
> > -    enum amdgpu_sync_mode sync_mode;
> >       int r, idx;
> >       if (!drm_dev_enter(adev_to_drm(adev), &idx))
> > @@ -991,14 +991,6 @@ int amdgpu_vm_update_range(struct amdgpu_device 
> > *adev, struct amdgpu_vm *vm,
> >       params.allow_override = allow_override;
> >       INIT_LIST_HEAD(&params.tlb_flush_waitlist);
> > -    /* Implicitly sync to command submissions in the same VM before
> > -     * unmapping. Sync to moving fences before mapping.
> > -     */
> > -    if (!(flags & AMDGPU_PTE_VALID))
> > -        sync_mode = AMDGPU_SYNC_EQ_OWNER;
> > -    else
> > -        sync_mode = AMDGPU_SYNC_EXPLICIT;
> > -
> >       amdgpu_vm_eviction_lock(vm);
> >       if (vm->evicting) {
> >           r = -EBUSY;
> > @@ -1013,7 +1005,7 @@ int amdgpu_vm_update_range(struct amdgpu_device 
> > *adev, struct amdgpu_vm *vm,
> >           dma_fence_put(tmp);
> >       }
> > -    r = vm->update_funcs->prepare(&params, resv, sync_mode);
> > +    r = vm->update_funcs->prepare(&params, sync);
> >       if (r)
> >           goto error_free;
> > @@ -1155,23 +1147,30 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
> > *adev, struct amdgpu_bo_va *bo_va,
> >       struct amdgpu_bo *bo = bo_va->base.bo;
> >       struct amdgpu_vm *vm = bo_va->base.vm;
> >       struct amdgpu_bo_va_mapping *mapping;
> > +    struct dma_fence **last_update;
> >       dma_addr_t *pages_addr = NULL;
> >       struct ttm_resource *mem;
> > -    struct dma_fence **last_update;
> > +    struct amdgpu_sync sync;
> >       bool flush_tlb = clear;
> > -    bool uncached;
> > -    struct dma_resv *resv;
> >       uint64_t vram_base;
> >       uint64_t flags;
> > +    bool uncached;
> >       int r;
> > +    amdgpu_sync_create(&sync);
> >       if (clear || !bo) {
> >           mem = NULL;
> > -        resv = vm->root.bo->tbo.base.resv;
> > +
> > +        /* Implicitly sync to command submissions in the same VM before
> > +         * unmapping.
> > +         */
> > +        r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> > +                     AMDGPU_SYNC_EQ_OWNER, vm);
> > +        if (r)
> > +            goto error_free;
> >       } else {
> >           struct drm_gem_object *obj = &bo->tbo.base;
> > -        resv = bo->tbo.base.resv;
> >           if (obj->import_attach && bo_va->is_xgmi) {
> >               struct dma_buf *dma_buf = obj->import_attach->dmabuf;
> >               struct drm_gem_object *gobj = dma_buf->priv;
> > @@ -1185,6 +1184,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
> > *adev, struct amdgpu_bo_va *bo_va,
> >           if (mem && (mem->mem_type == TTM_PL_TT ||
> >                   mem->mem_type == AMDGPU_PL_PREEMPT))
> >               pages_addr = bo->tbo.ttm->dma_address;
> > +
> > +        /* Implicitly sync to moving fences before mapping anything */
> > +        r = amdgpu_sync_resv(adev, &sync, bo->tbo.base.resv,
> > +                     AMDGPU_SYNC_EXPLICIT, vm);
> > +        if (r)
> > +            goto error_free;
> >       }
> >       if (bo) {
> > @@ -1234,12 +1239,12 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
> > *adev, struct amdgpu_bo_va *bo_va,
> >           trace_amdgpu_vm_bo_update(mapping);
> >           r = amdgpu_vm_update_range(adev, vm, false, false, flush_tlb,
> > -                       !uncached, resv, mapping->start, mapping->last,
> > -                       update_flags, mapping->offset,
> > -                       vram_base, mem, pages_addr,
> > -                       last_update);
> > +                       !uncached, &sync, mapping->start,
> > +                       mapping->last, update_flags,
> > +                       mapping->offset, vram_base, mem,
> > +                       pages_addr, last_update);
> >           if (r)
> > -            return r;
> > +            goto error_free;
> >       }
> >       /* If the BO is not in its preferred location add it back to
> > @@ -1267,7 +1272,9 @@ int amdgpu_vm_bo_update(struct amdgpu_device 
> > *adev, struct amdgpu_bo_va *bo_va,
> >               trace_amdgpu_vm_bo_mapping(mapping);
> >       }
> > -    return 0;
> > +error_free:
> > +    amdgpu_sync_free(&sync);
> > +    return r;
> >   }
> >   /**
> > @@ -1414,25 +1421,34 @@ int amdgpu_vm_clear_freed(struct amdgpu_device 
> > *adev,
> >                 struct amdgpu_vm *vm,
> >                 struct dma_fence **fence)
> >   {
> > -    struct dma_resv *resv = vm->root.bo->tbo.base.resv;
> >       struct amdgpu_bo_va_mapping *mapping;
> > -    uint64_t init_pte_value = 0;
> >       struct dma_fence *f = NULL;
> > +    struct amdgpu_sync sync;
> >       int r;
> > +
> > +    /*
> > +     * Implicitly sync to command submissions in the same VM before
> > +     * unmapping.
> > +     */
> > +    amdgpu_sync_create(&sync);
> > +    r = amdgpu_sync_resv(adev, &sync, vm->root.bo->tbo.base.resv,
> > +                 AMDGPU_SYNC_EQ_OWNER, vm);
> > +    if (r)
> > +        goto error_free;
> > +
> >       while (!list_empty(&vm->freed)) {
> >           mapping = list_first_entry(&vm->freed,
> >               struct amdgpu_bo_va_mapping, list);
> >           list_del(&mapping->list);
> >           r = amdgpu_vm_update_range(adev, vm, false, false, true, false,
> > -                       resv, mapping->start, mapping->last,
> > -                       init_pte_value, 0, 0, NULL, NULL,
> > -                       &f);
> > +                       &sync, mapping->start, mapping->last,
> > +                       0, 0, 0, NULL, NULL, &f);
> >           amdgpu_vm_free_mapping(adev, vm, mapping, f);
> >           if (r) {
> >               dma_fence_put(f);
> > -            return r;
> > +            goto error_free;
> >           }
> >       }
> > @@ -1443,7 +1459,9 @@ int amdgpu_vm_clear_freed(struct amdgpu_device 
> > *adev,
> >           dma_fence_put(f);
> >       }
> > -    return 0;
> > +error_free:
> > +    amdgpu_sync_free(&sync);
> > +    return r;
> >   }
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index 046949c4b695..1a759012ce93 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -304,8 +304,8 @@ struct amdgpu_vm_update_params {
> >   struct amdgpu_vm_update_funcs {
> >       int (*map_table)(struct amdgpu_bo_vm *bo);
> > -    int (*prepare)(struct amdgpu_vm_update_params *p, struct dma_resv 
> > *resv,
> > -               enum amdgpu_sync_mode sync_mode);
> > +    int (*prepare)(struct amdgpu_vm_update_params *p,
> > +               struct amdgpu_sync *sync);
> >       int (*update)(struct amdgpu_vm_update_params *p,
> >                 struct amdgpu_bo_vm *bo, uint64_t pe, uint64_t addr,
> >                 unsigned count, uint32_t incr, uint64_t flags);
> > @@ -505,9 +505,10 @@ int amdgpu_vm_flush_compute_tlb(struct 
> > amdgpu_device *adev,
> >   void amdgpu_vm_bo_base_init(struct amdgpu_vm_bo_base *base,
> >                   struct amdgpu_vm *vm, struct amdgpu_bo *bo);
> >   int amdgpu_vm_update_range(struct amdgpu_device *adev, struct 
> > amdgpu_vm *vm,
> > -               bool immediate, bool unlocked, bool flush_tlb, bool 
> > allow_override,
> > -               struct dma_resv *resv, uint64_t start, uint64_t last,
> > -               uint64_t flags, uint64_t offset, uint64_t vram_base,
> > +               bool immediate, bool unlocked, bool flush_tlb,
> > +               bool allow_override, struct amdgpu_sync *sync,
> > +               uint64_t start, uint64_t last, uint64_t flags,
> > +               uint64_t offset, uint64_t vram_base,
> >                  struct ttm_resource *res, dma_addr_t *pages_addr,
> >                  struct dma_fence **fence);
> >   int amdgpu_vm_bo_update(struct amdgpu_device *adev,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> > index 3895bd7d176a..9ff59a4e6f15 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_cpu.c
> > @@ -46,13 +46,12 @@ static int amdgpu_vm_cpu_map_table(struct 
> > amdgpu_bo_vm *table)
> >    * Negativ errno, 0 for success.
> >    */
> >   static int amdgpu_vm_cpu_prepare(struct amdgpu_vm_update_params *p,
> > -                 struct dma_resv *resv,
> > -                 enum amdgpu_sync_mode sync_mode)
> > +                 struct amdgpu_sync *sync)
> >   {
> > -    if (!resv)
> > +    if (!sync)
> >           return 0;
> > -    return amdgpu_bo_sync_wait_resv(p->adev, resv, sync_mode, p->vm, 
> > true);
> > +    return amdgpu_sync_wait(sync, true);
> >   }
> >   /**
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > index e39d6e7643bf..a076f43097e4 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_pt.c
> > @@ -403,7 +403,7 @@ int amdgpu_vm_pt_clear(struct amdgpu_device *adev, 
> > struct amdgpu_vm *vm,
> >       params.vm = vm;
> >       params.immediate = immediate;
> > -    r = vm->update_funcs->prepare(&params, NULL, AMDGPU_SYNC_EXPLICIT);
> > +    r = vm->update_funcs->prepare(&params, NULL);
> >       if (r)
> >           goto exit;
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c 
> > b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > index 9b748d7058b5..4772fba33285 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm_sdma.c
> > @@ -77,32 +77,24 @@ static int amdgpu_vm_sdma_alloc_job(struct 
> > amdgpu_vm_update_params *p,
> >    * amdgpu_vm_sdma_prepare - prepare SDMA command submission
> >    *
> >    * @p: see amdgpu_vm_update_params definition
> > - * @resv: reservation object with embedded fence
> > - * @sync_mode: synchronization mode
> > + * @sync: amdgpu_sync object with fences to wait for
> >    *
> >    * Returns:
> >    * Negativ errno, 0 for success.
> >    */
> >   static int amdgpu_vm_sdma_prepare(struct amdgpu_vm_update_params *p,
> > -                  struct dma_resv *resv,
> > -                  enum amdgpu_sync_mode sync_mode)
> > +                  struct amdgpu_sync *sync)
> >   {
> > -    struct amdgpu_sync sync;
> >       int r;
> >       r = amdgpu_vm_sdma_alloc_job(p, 0);
> >       if (r)
> >           return r;
> > -    if (!resv)
> > +    if (!sync)
> >           return 0;
> > -    amdgpu_sync_create(&sync);
> > -    r = amdgpu_sync_resv(p->adev, &sync, resv, sync_mode, p->vm);
> > -    if (!r)
> > -        r = amdgpu_sync_push_to_job(&sync, p->job);
> > -    amdgpu_sync_free(&sync);
> > -
> > +    r = amdgpu_sync_push_to_job(sync, p->job);
> >       if (r) {
> >           p->num_dw_left = 0;
> >           amdgpu_job_free(p->job);