> On Oct 31, 2023, at 23:17, Bas Nieuwenhuizen <bas@xxxxxxxxxxxxxxxxxxx> wrote:
>
>
>
> On Tue, Oct 31, 2023 at 3:08 PM Christian König <christian.koenig@xxxxxxx> wrote:
> Am 31.10.23 um 14:59 schrieb Bas Nieuwenhuizen:
>>
>>
>> On Tue, Oct 31, 2023 at 2:57 PM Christian König <christian.koenig@xxxxxxx> 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.
>
> I think as long as we make sure all significant work gets done asynchronously, doing the TLB flushing on the next submit (/submissions, one per queue?) is fine for our purposes.
For a bit more of context, the performance / frame timing in Forza with just patch 5 wasn’t quite right. As Bas said, ideally we want to perform all the PT updates right away, and only defer the TLB flush.
For now the state machine part of this patch doesn’t seem to be going in the right direction so I’ll consider dropping this change.
Tatsuyuki.
>
> (As an aside after thinking some more I *think* we also need some work to make these maps/unmaps (VALID->PRT and PRT->VALID) atomic, as I think it is valid Vulkan to make these race. As such I'm speculating we'd need a bit more reworking there too, not just a late free of the lower level pagetables)
>
> - Bas
>
> 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@xxxxxxxxx>
>> > ---
>> > 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;
>> >
>> > 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;
>> > -
>> > 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;
>> > -
>> > 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)
>> > + continue;
>> > + total_dirty += amdgpu_bo_print_info(id++, bo_va->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)
>> > @@ -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;
>> >
