[PATCH 3/6] drm/amdgpu: Flush VM updates for split bindings eagerly.
Christian König
christian.koenig at amd.com
Tue Oct 31 14:07:51 UTC 2023
Am 31.10.23 um 14:59 schrieb Bas Nieuwenhuizen:
>
>
> On Tue, Oct 31, 2023 at 2:57 PM Christian König
> <christian.koenig at amd.com> wrote:
>
> Am 31.10.23 um 14:40 schrieb Tatsuyuki Ishi:
> > The current amdgpu_gem_va_update_vm only tries to perform
> updates for the
> > BO specified in the GEM ioctl; however, when a binding is split, the
> > adjacent bindings also need to be updated. Such updates
> currently ends up
> > getting deferred until next submission which causes stalls.
>
> Yeah, that is a necessity. The hardware simply doesn't support
> what you
> try to do here in all cases.
>
>
> What can the hardware not do here? Is this just needing to wait for
> TLB flushes before we can free pagetables, can we just delay that?
On some hardware generations (especially Navi1x, but also everything
older than Polaris) you can't invalidate the TLB while it is in use.
For Polaris and older it just means that you don't have a guarantee that
the shader can't access the memory any more. So delaying the free
operation helps here.
But for Navi1x it's a workaround for a hardware bug. If you try to
invalidate the TLB while it is in use you can potentially triggering
memory accesses to random addresses.
That's why we still delay TLB invalidation's to the next CS and use a
new VMID for each submission instead of invalidating the old one.
I'm currently working on changing that for Navi2x and newer (maybe Vega
as well), but this is something you can really only do on some hw
generations after validating that it works.
Regards,
Christian.
>
>
> So this approach won't work in general.
>
> Regards,
> Christian.
>
> >
> > Introduce a new state "dirty", shared between per-VM BOs and
> traditional
> > BOs, containing all BOs that have pending updates in `invalids`.
> > amdgpu_gem_va_update_vm will now simply flush any pending
> updates for BOs
> > in the dirty state.
> >
> > Signed-off-by: Tatsuyuki Ishi <ishitatsuyuki at gmail.com>
> > ---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c | 18 ++++---
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c | 66
> ++++++++++++++++++-------
> > drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h | 3 ++
> > 3 files changed, 63 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > index a1b15d0d6c48..01d3a97248b0 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_gem.c
> > @@ -604,10 +604,9 @@ int amdgpu_gem_metadata_ioctl(struct
> drm_device *dev, void *data,
> > * vital here, so they are not reported back to userspace.
> > */
> > static void amdgpu_gem_va_update_vm(struct amdgpu_device *adev,
> > - struct amdgpu_vm *vm,
> > - struct amdgpu_bo_va *bo_va,
> > - uint32_t operation)
> > + struct amdgpu_vm *vm)
> > {
> > + struct amdgpu_bo_va *bo_va;
> > int r;
> >
> > if (!amdgpu_vm_ready(vm))
> > @@ -617,12 +616,18 @@ static void amdgpu_gem_va_update_vm(struct
> amdgpu_device *adev,
> > if (r)
> > goto error;
> >
> > - if (operation == AMDGPU_VA_OP_MAP ||
> > - operation == AMDGPU_VA_OP_REPLACE) {
> > + spin_lock(&vm->status_lock);
> > + while (!list_empty(&vm->dirty)) {
> > + bo_va = list_first_entry(&vm->dirty, struct
> amdgpu_bo_va,
> > + base.vm_status);
> > + spin_unlock(&vm->status_lock);
> > +
> > r = amdgpu_vm_bo_update(adev, bo_va, false);
> > if (r)
> > goto error;
> > + spin_lock(&vm->status_lock);
> > }
> > + spin_unlock(&vm->status_lock);
> >
> > r = amdgpu_vm_update_pdes(adev, vm, false);
> >
> > @@ -792,8 +797,7 @@ int amdgpu_gem_va_ioctl(struct drm_device
> *dev, void *data,
> > break;
> > }
> > if (!r && !(args->flags & AMDGPU_VM_DELAY_UPDATE) &&
> !amdgpu_vm_debug)
> > - amdgpu_gem_va_update_vm(adev, &fpriv->vm, bo_va,
> > - args->operation);
> > + amdgpu_gem_va_update_vm(adev, &fpriv->vm);
> >
> > error:
> > drm_exec_fini(&exec);
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > index dd6f72e2a1d6..01d31891cd05 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c
> > @@ -191,6 +191,21 @@ static void amdgpu_vm_bo_set_evicted(struct
> amdgpu_vm_bo_base *vm_bo, bool evict
> > spin_unlock(&vm_bo->vm->status_lock);
> > }
> >
> > +/**
> > + * amdgpu_vm_bo_dirty - vm_bo is dirty
> > + *
> > + * @vm_bo: vm_bo which is dirty
> > + *
> > + * State for normal and per VM BOs that are not moved, but have
> new entries in
> > + * bo_va->invalids.
> > + */
> > +static void amdgpu_vm_bo_dirty(struct amdgpu_vm_bo_base *vm_bo)
> > +{
> > + spin_lock(&vm_bo->vm->status_lock);
> > + list_move(&vm_bo->vm_status, &vm_bo->vm->dirty);
> > + spin_unlock(&vm_bo->vm->status_lock);
> > +}
> > +
> > /**
> > * amdgpu_vm_bo_moved - vm_bo is moved
> > *
> > @@ -1042,6 +1057,9 @@ void amdgpu_vm_get_memory(struct amdgpu_vm
> *vm,
> > list_for_each_entry_safe(bo_va, tmp, &vm->evicted,
> base.eviction_status)
> > amdgpu_vm_bo_get_memory(bo_va, stats);
> >
> > + list_for_each_entry_safe(bo_va, tmp, &vm->dirty,
> base.vm_status)
> > + amdgpu_vm_bo_get_memory(bo_va, stats);
> > +
> > list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
> base.vm_status)
> > amdgpu_vm_bo_get_memory(bo_va, stats);
> >
> > @@ -1411,6 +1429,17 @@ int amdgpu_vm_handle_moved(struct
> amdgpu_device *adev,
> > dma_resv_unlock(resv);
> > spin_lock(&vm->status_lock);
> > }
> > +
> > + while (!list_empty(&vm->dirty)) {
> > + bo_va = list_first_entry(&vm->dirty, struct
> amdgpu_bo_va,
> > + base.vm_status);
> > + spin_unlock(&vm->status_lock);
> > +
> > + r = amdgpu_vm_bo_update(adev, bo_va, false);
> > + if (r)
> > + return r;
> > + spin_lock(&vm->status_lock);
> > + }
> > spin_unlock(&vm->status_lock);
> >
> > return 0;
> > @@ -1476,19 +1505,16 @@ static void
> amdgpu_vm_bo_insert_map(struct amdgpu_device *adev,
> > struct amdgpu_bo_va_mapping
> *mapping)
> > {
> > struct amdgpu_vm *vm = bo_va->base.vm;
> > - struct amdgpu_bo *bo = bo_va->base.bo <http://base.bo>;
> >
> > mapping->bo_va = bo_va;
> > list_add(&mapping->list, &bo_va->invalids);
> > amdgpu_vm_it_insert(mapping, &vm->va);
> > + if (!bo_va->base.moved)
> > + amdgpu_vm_bo_dirty(&bo_va->base);
> >
> > if (mapping->flags & AMDGPU_PTE_PRT)
> > amdgpu_vm_prt_get(adev);
> >
> > - if (bo && bo->tbo.base.resv == vm->root.bo->tbo.base.resv &&
> > - !bo_va->base.moved) {
> > - amdgpu_vm_bo_moved(&bo_va->base);
> > - }
> > trace_amdgpu_vm_bo_map(bo_va, mapping);
> > }
> >
> > @@ -1725,6 +1751,8 @@ int amdgpu_vm_bo_clear_mappings(struct
> amdgpu_device *adev,
> > before->flags = tmp->flags;
> > before->bo_va = tmp->bo_va;
> > list_add(&before->list,
> &tmp->bo_va->invalids);
> > + if (!tmp->bo_va->base.moved)
> > + amdgpu_vm_bo_dirty(&tmp->bo_va->base);
> > }
> >
> > /* Remember mapping split at the end */
> > @@ -1736,6 +1764,8 @@ int amdgpu_vm_bo_clear_mappings(struct
> amdgpu_device *adev,
> > after->flags = tmp->flags;
> > after->bo_va = tmp->bo_va;
> > list_add(&after->list, &tmp->bo_va->invalids);
> > + if (!tmp->bo_va->base.moved)
> > + amdgpu_vm_bo_dirty(&tmp->bo_va->base);
> > }
> >
> > list_del(&tmp->list);
> > @@ -1761,30 +1791,18 @@ int amdgpu_vm_bo_clear_mappings(struct
> amdgpu_device *adev,
> >
> > /* Insert partial mapping before the range */
> > if (!list_empty(&before->list)) {
> > - struct amdgpu_bo *bo = before->bo_va->base.bo
> <http://base.bo>;
> > -
> > amdgpu_vm_it_insert(before, &vm->va);
> > if (before->flags & AMDGPU_PTE_PRT)
> > amdgpu_vm_prt_get(adev);
> > -
> > - if (bo && bo->tbo.base.resv ==
> vm->root.bo->tbo.base.resv &&
> > - !before->bo_va->base.moved)
> > - amdgpu_vm_bo_moved(&before->bo_va->base);
> > } else {
> > kfree(before);
> > }
> >
> > /* Insert partial mapping after the range */
> > if (!list_empty(&after->list)) {
> > - struct amdgpu_bo *bo = after->bo_va->base.bo
> <http://base.bo>;
> > -
> > amdgpu_vm_it_insert(after, &vm->va);
> > if (after->flags & AMDGPU_PTE_PRT)
> > amdgpu_vm_prt_get(adev);
> > -
> > - if (bo && bo->tbo.base.resv ==
> vm->root.bo->tbo.base.resv &&
> > - !after->bo_va->base.moved)
> > - amdgpu_vm_bo_moved(&after->bo_va->base);
> > } else {
> > kfree(after);
> > }
> > @@ -2136,6 +2154,7 @@ int amdgpu_vm_init(struct amdgpu_device
> *adev, struct amdgpu_vm *vm, int32_t xcp
> > INIT_LIST_HEAD(&vm->evicted);
> > INIT_LIST_HEAD(&vm->relocated);
> > INIT_LIST_HEAD(&vm->moved);
> > + INIT_LIST_HEAD(&vm->dirty);
> > INIT_LIST_HEAD(&vm->idle);
> > INIT_LIST_HEAD(&vm->invalidated);
> > spin_lock_init(&vm->status_lock);
> > @@ -2648,11 +2667,13 @@ void amdgpu_debugfs_vm_bo_info(struct
> amdgpu_vm *vm, struct seq_file *m)
> > {
> > struct amdgpu_bo_va *bo_va, *tmp;
> > u64 total_idle = 0;
> > + u64 total_dirty = 0;
> > u64 total_relocated = 0;
> > u64 total_moved = 0;
> > u64 total_invalidated = 0;
> > u64 total_done = 0;
> > unsigned int total_idle_objs = 0;
> > + unsigned int total_dirty_objs = 0;
> > unsigned int total_relocated_objs = 0;
> > unsigned int total_moved_objs = 0;
> > unsigned int total_invalidated_objs = 0;
> > @@ -2669,6 +2690,15 @@ void amdgpu_debugfs_vm_bo_info(struct
> amdgpu_vm *vm, struct seq_file *m)
> > total_idle_objs = id;
> > id = 0;
> >
> > + seq_puts(m, "\tDirty BOs:\n");
> > + list_for_each_entry_safe(bo_va, tmp, &vm->dirty,
> base.vm_status) {
> > + if (!bo_va->base.bo <http://base.bo>)
> > + continue;
> > + total_dirty += amdgpu_bo_print_info(id++,
> bo_va->base.bo <http://base.bo>, m);
> > + }
> > + total_dirty_objs = id;
> > + id = 0;
> > +
> > seq_puts(m, "\tRelocated BOs:\n");
> > list_for_each_entry_safe(bo_va, tmp, &vm->relocated,
> base.vm_status) {
> > if (!bo_va->base.bo <http://base.bo>)
> > @@ -2707,6 +2737,8 @@ void amdgpu_debugfs_vm_bo_info(struct
> amdgpu_vm *vm, struct seq_file *m)
> >
> > seq_printf(m, "\tTotal idle size: %12lld\tobjs:\t%d\n",
> total_idle,
> > total_idle_objs);
> > + seq_printf(m, "\tTotal dirty size: %12lld\tobjs:\t%d\n",
> total_dirty,
> > + total_dirty_objs);
> > seq_printf(m, "\tTotal relocated size:
> %12lld\tobjs:\t%d\n", total_relocated,
> > total_relocated_objs);
> > seq_printf(m, "\tTotal moved size: %12lld\tobjs:\t%d\n",
> total_moved,
> > diff --git a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > index d9ab97eabda9..f91d4fcf80b8 100644
> > --- a/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > +++ b/drivers/gpu/drm/amd/amdgpu/amdgpu_vm.h
> > @@ -276,6 +276,9 @@ struct amdgpu_vm {
> > /* per VM BOs moved, but not yet updated in the PT */
> > struct list_head moved;
> >
> > + /* normal and per VM BOs that are not moved, but have new
> PT entries */
> > + struct list_head dirty;
> > +
> > /* All BOs of this VM not currently in the state machine */
> > struct list_head idle;
> >
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <https://lists.freedesktop.org/archives/dri-devel/attachments/20231031/74275af3/attachment-0001.htm>
More information about the dri-devel
mailing list