[Intel-xe] [PATCH 3/3] drm/xe: Fix array of binds
Matthew Brost
matthew.brost at intel.com
Thu Aug 31 14:43:10 UTC 2023
On Tue, Aug 22, 2023 at 07:38:46PM -0400, Rodrigo Vivi wrote:
> On Wed, Aug 16, 2023 at 09:31:48PM -0700, Matthew Brost wrote:
> > If multiple bind ops in an array of binds touch the same address range
> > invalid GPUVA operations are generated as each GPUVA operation is
> > generated based on the orignal GPUVA state. To fix this, after each
> > GPUVA operations is generated, commit the GPUVA operation updating the
> > GPUVA state so subsequent bind ops can see a current GPUVA state.
> >
> > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_vm.c | 418 +++++++++++++++++++------------------
> > 1 file changed, 212 insertions(+), 206 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index bd20840616ca..2452e24fbc81 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2426,24 +2426,73 @@ static u64 xe_vma_set_pte_size(struct xe_vma *vma, u64 size)
> > return SZ_4K;
> > }
> >
> > -/*
> > - * Parse operations list and create any resources needed for the operations
> > - * prior to fully committing to the operations. This setup can fail.
> > - */
> > +static int xe_vma_op_commit(struct xe_vm *vm, struct xe_vma_op *op)
> > +{
> > + int err = 0;
> > +
> > + lockdep_assert_held_write(&vm->lock);
> > +
> > + switch (op->base.op) {
> > + case DRM_GPUVA_OP_MAP:
> > + err |= xe_vm_insert_vma(vm, op->map.vma);
> > + if (!err)
> > + op->flags |= XE_VMA_OP_COMMITTED;
> > + break;
> > + case DRM_GPUVA_OP_REMAP:
> > + prep_vma_destroy(vm, gpuva_to_vma(op->base.remap.unmap->va),
> > + true);
> > + op->flags |= XE_VMA_OP_COMMITTED;
> > +
> > + if (op->remap.prev) {
> > + err |= xe_vm_insert_vma(vm, op->remap.prev);
> > + if (!err)
> > + op->flags |= XE_VMA_OP_PREV_COMMITTED;
> > + if (!err && op->remap.skip_prev)
> > + op->remap.prev = NULL;
> > + }
> > + if (op->remap.next) {
> > + err |= xe_vm_insert_vma(vm, op->remap.next);
> > + if (!err)
> > + op->flags |= XE_VMA_OP_NEXT_COMMITTED;
> > + if (!err && op->remap.skip_next)
> > + op->remap.next = NULL;
> > + }
> > +
> > + /* Adjust for partial unbind after removin VMA from VM */
> > + if (!err) {
> > + op->base.remap.unmap->va->va.addr = op->remap.start;
> > + op->base.remap.unmap->va->va.range = op->remap.range;
> > + }
> > + break;
> > + case DRM_GPUVA_OP_UNMAP:
> > + prep_vma_destroy(vm, gpuva_to_vma(op->base.unmap.va), true);
> > + op->flags |= XE_VMA_OP_COMMITTED;
> > + break;
> > + case DRM_GPUVA_OP_PREFETCH:
> > + op->flags |= XE_VMA_OP_COMMITTED;
> > + break;
> > + default:
> > + XE_WARN_ON("NOT POSSIBLE");
> > + }
> > +
> > + return err;
> > +}
> > +
> > +
> > static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
> > - struct drm_gpuva_ops **ops, int num_ops_list,
> > + struct drm_gpuva_ops *ops,
> > struct xe_sync_entry *syncs, u32 num_syncs,
> > - struct list_head *ops_list, bool async)
> > + struct list_head *ops_list, bool last,
> > + bool async)
> > {
> > struct xe_vma_op *last_op = NULL;
> > - struct list_head *async_list = NULL;
> > struct async_op_fence *fence = NULL;
> > - int err, i;
> > + struct drm_gpuva_op *__op;
> > + int err = 0;
> >
> > lockdep_assert_held_write(&vm->lock);
> > - XE_WARN_ON(num_ops_list > 1 && !async);
> >
> > - if (num_syncs && async) {
> > + if (last && num_syncs && async) {
> > u64 seqno;
> >
> > fence = kmalloc(sizeof(*fence), GFP_KERNEL);
> > @@ -2462,145 +2511,145 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
> > }
> > }
> >
> > - for (i = 0; i < num_ops_list; ++i) {
> > - struct drm_gpuva_ops *__ops = ops[i];
> > - struct drm_gpuva_op *__op;
>
> I got a bit confused here. why we were iterating on the op list,
> but suddenly this iteration is not needed anymore?
>
The loop is pushed to a higher level + we commit the op each iteration of the loop.
> > + drm_gpuva_for_each_op(__op, ops) {
> > + struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> > + bool first = list_empty(ops_list);
> >
> > - drm_gpuva_for_each_op(__op, __ops) {
> > - struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> > - bool first = !async_list;
> > + XE_WARN_ON(!first && !async);
> > +
> > + INIT_LIST_HEAD(&op->link);
> > + list_add_tail(&op->link, ops_list);
> >
> > - XE_WARN_ON(!first && !async);
> > + if (first) {
> > + op->flags |= XE_VMA_OP_FIRST;
> > + op->num_syncs = num_syncs;
> > + op->syncs = syncs;
> > + }
> >
> > - INIT_LIST_HEAD(&op->link);
> > - if (first)
> > - async_list = ops_list;
> > - list_add_tail(&op->link, async_list);
> > + op->q = q;
> > +
> > + switch (op->base.op) {
> > + case DRM_GPUVA_OP_MAP:
> > + {
> > + struct xe_vma *vma;
> >
> > - if (first) {
> > - op->flags |= XE_VMA_OP_FIRST;
> > - op->num_syncs = num_syncs;
> > - op->syncs = syncs;
> > + vma = new_vma(vm, &op->base.map,
> > + op->tile_mask, op->map.read_only,
> > + op->map.is_null);
> > + if (IS_ERR(vma)) {
> > + err = PTR_ERR(vma);
> > + goto free_fence;
> > }
> >
> > - op->q = q;
> > + op->map.vma = vma;
> > + break;
> > + }
> > + case DRM_GPUVA_OP_REMAP:
> > + {
> > + struct xe_vma *old =
> > + gpuva_to_vma(op->base.remap.unmap->va);
> >
> > - switch (op->base.op) {
> > - case DRM_GPUVA_OP_MAP:
> > - {
> > - struct xe_vma *vma;
> > + op->remap.start = xe_vma_start(old);
> > + op->remap.range = xe_vma_size(old);
> >
> > - vma = new_vma(vm, &op->base.map,
> > - op->tile_mask, op->map.read_only,
> > - op->map.is_null);
> > + if (op->base.remap.prev) {
> > + struct xe_vma *vma;
> > + bool read_only =
> > + op->base.remap.unmap->va->flags &
> > + XE_VMA_READ_ONLY;
> > + bool is_null =
> > + op->base.remap.unmap->va->flags &
> > + DRM_GPUVA_SPARSE;
> > +
> > + vma = new_vma(vm, op->base.remap.prev,
> > + op->tile_mask, read_only,
> > + is_null);
> > if (IS_ERR(vma)) {
> > err = PTR_ERR(vma);
> > goto free_fence;
> > }
> >
> > - op->map.vma = vma;
> > - break;
> > + op->remap.prev = vma;
> > +
> > + /*
> > + * Userptr creates a new SG mapping so
> > + * we must also rebind.
> > + */
> > + op->remap.skip_prev = !xe_vma_is_userptr(old) &&
> > + IS_ALIGNED(xe_vma_end(vma),
> > + xe_vma_max_pte_size(old));
> > + if (op->remap.skip_prev) {
> > + xe_vma_set_pte_size(vma, xe_vma_max_pte_size(old));
> > + op->remap.range -=
> > + xe_vma_end(vma) -
> > + xe_vma_start(old);
> > + op->remap.start = xe_vma_end(vma);
> > + }
> > }
> > - case DRM_GPUVA_OP_REMAP:
> > - {
> > - struct xe_vma *old =
> > - gpuva_to_vma(op->base.remap.unmap->va);
> > -
> > - op->remap.start = xe_vma_start(old);
> > - op->remap.range = xe_vma_size(old);
> > -
> > - if (op->base.remap.prev) {
> > - struct xe_vma *vma;
> > - bool read_only =
> > - op->base.remap.unmap->va->flags &
> > - XE_VMA_READ_ONLY;
> > - bool is_null =
> > - op->base.remap.unmap->va->flags &
> > - DRM_GPUVA_SPARSE;
> > -
> > - vma = new_vma(vm, op->base.remap.prev,
> > - op->tile_mask, read_only,
> > - is_null);
> > - if (IS_ERR(vma)) {
> > - err = PTR_ERR(vma);
> > - goto free_fence;
> > - }
> > -
> > - op->remap.prev = vma;
> > -
> > - /*
> > - * Userptr creates a new SG mapping so
> > - * we must also rebind.
> > - */
> > - op->remap.skip_prev = !xe_vma_is_userptr(old) &&
> > - IS_ALIGNED(xe_vma_end(vma),
> > - xe_vma_max_pte_size(old));
> > - if (op->remap.skip_prev) {
> > - xe_vma_set_pte_size(vma, xe_vma_max_pte_size(old));
> > - op->remap.range -=
> > - xe_vma_end(vma) -
> > - xe_vma_start(old);
> > - op->remap.start = xe_vma_end(vma);
> > - }
> > +
> > + if (op->base.remap.next) {
> > + struct xe_vma *vma;
> > + bool read_only =
> > + op->base.remap.unmap->va->flags &
> > + XE_VMA_READ_ONLY;
> > +
> > + bool is_null =
> > + op->base.remap.unmap->va->flags &
> > + DRM_GPUVA_SPARSE;
> > +
> > + vma = new_vma(vm, op->base.remap.next,
> > + op->tile_mask, read_only,
> > + is_null);
> > + if (IS_ERR(vma)) {
> > + err = PTR_ERR(vma);
> > + goto free_fence;
> > }
> >
> > - if (op->base.remap.next) {
> > - struct xe_vma *vma;
> > - bool read_only =
> > - op->base.remap.unmap->va->flags &
> > - XE_VMA_READ_ONLY;
> > -
> > - bool is_null =
> > - op->base.remap.unmap->va->flags &
> > - DRM_GPUVA_SPARSE;
> > -
> > - vma = new_vma(vm, op->base.remap.next,
> > - op->tile_mask, read_only,
> > - is_null);
> > - if (IS_ERR(vma)) {
> > - err = PTR_ERR(vma);
> > - goto free_fence;
> > - }
> > -
> > - op->remap.next = vma;
> > -
> > - /*
> > - * Userptr creates a new SG mapping so
> > - * we must also rebind.
> > - */
> > - op->remap.skip_next = !xe_vma_is_userptr(old) &&
> > - IS_ALIGNED(xe_vma_start(vma),
> > - xe_vma_max_pte_size(old));
> > - if (op->remap.skip_next) {
> > - xe_vma_set_pte_size(vma, xe_vma_max_pte_size(old));
> > - op->remap.range -=
> > - xe_vma_end(old) -
> > - xe_vma_start(vma);
> > - }
> > + op->remap.next = vma;
> > +
> > + /*
> > + * Userptr creates a new SG mapping so
> > + * we must also rebind.
> > + */
> > + op->remap.skip_next = !xe_vma_is_userptr(old) &&
> > + IS_ALIGNED(xe_vma_start(vma),
> > + xe_vma_max_pte_size(old));
> > + if (op->remap.skip_next) {
> > + xe_vma_set_pte_size(vma, xe_vma_max_pte_size(old));
> > + op->remap.range -=
> > + xe_vma_end(old) -
> > + xe_vma_start(vma);
> > }
> > - break;
> > - }
> > - case DRM_GPUVA_OP_UNMAP:
> > - case DRM_GPUVA_OP_PREFETCH:
> > - /* Nothing to do */
> > - break;
> > - default:
> > - XE_WARN_ON("NOT POSSIBLE");
> > }
> > -
> > - last_op = op;
> > + break;
> > + }
> > + case DRM_GPUVA_OP_UNMAP:
> > + case DRM_GPUVA_OP_PREFETCH:
> > + /* Nothing to do */
> > + break;
> > + default:
> > + XE_WARN_ON("NOT POSSIBLE");
> > }
> >
> > - last_op->ops = __ops;
> > + last_op = op;
> > +
> > + err = xe_vma_op_commit(vm, op);
> > + if (err)
> > + goto free_fence;
> > }
> >
> > - if (!last_op)
> > - return -ENODATA;
> > + /* FIXME: Unhandled corner case */
> > + XE_WARN_ON(!last_op && last && !list_empty(ops_list));
> >
> > - last_op->flags |= XE_VMA_OP_LAST;
> > - last_op->num_syncs = num_syncs;
> > - last_op->syncs = syncs;
> > - last_op->fence = fence;
> > + if (!last_op)
> > + goto free_fence;
> > + last_op->ops = ops;
> > + if (last) {
> > + last_op->flags |= XE_VMA_OP_LAST;
> > + last_op->num_syncs = num_syncs;
> > + last_op->syncs = syncs;
> > + last_op->fence = fence;
> > + }
> >
> > return 0;
> >
> > @@ -2609,58 +2658,6 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
> > return err;
> > }
> >
> > -static int xe_vma_op_commit(struct xe_vm *vm, struct xe_vma_op *op)
> > -{
> > - int err = 0;
> > -
> > - lockdep_assert_held_write(&vm->lock);
> > -
> > - switch (op->base.op) {
> > - case DRM_GPUVA_OP_MAP:
> > - err |= xe_vm_insert_vma(vm, op->map.vma);
> > - if (!err)
> > - op->flags |= XE_VMA_OP_COMMITTED;
> > - break;
> > - case DRM_GPUVA_OP_REMAP:
> > - prep_vma_destroy(vm, gpuva_to_vma(op->base.remap.unmap->va),
> > - true);
> > - op->flags |= XE_VMA_OP_COMMITTED;
> > -
> > - if (op->remap.prev) {
> > - err |= xe_vm_insert_vma(vm, op->remap.prev);
> > - if (!err)
> > - op->flags |= XE_VMA_OP_PREV_COMMITTED;
> > - if (!err && op->remap.skip_prev)
> > - op->remap.prev = NULL;
> > - }
> > - if (op->remap.next) {
> > - err |= xe_vm_insert_vma(vm, op->remap.next);
> > - if (!err)
> > - op->flags |= XE_VMA_OP_NEXT_COMMITTED;
> > - if (!err && op->remap.skip_next)
> > - op->remap.next = NULL;
> > - }
> > -
> > - /* Adjust for partial unbind after removin VMA from VM */
> > - if (!err) {
> > - op->base.remap.unmap->va->va.addr = op->remap.start;
> > - op->base.remap.unmap->va->va.range = op->remap.range;
> > - }
> > - break;
> > - case DRM_GPUVA_OP_UNMAP:
> > - prep_vma_destroy(vm, gpuva_to_vma(op->base.unmap.va), true);
> > - op->flags |= XE_VMA_OP_COMMITTED;
> > - break;
> > - case DRM_GPUVA_OP_PREFETCH:
> > - op->flags |= XE_VMA_OP_COMMITTED;
> > - break;
> > - default:
> > - XE_WARN_ON("NOT POSSIBLE");
> > - }
> > -
> > - return err;
> > -}
> > -
> > static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> > struct xe_vma_op *op)
> > {
> > @@ -2878,11 +2875,13 @@ static void xe_vma_op_unwind(struct xe_vm *vm, struct xe_vma_op *op,
> > {
> > struct xe_vma *vma = gpuva_to_vma(op->base.unmap.va);
> >
> > - down_read(&vm->userptr.notifier_lock);
> > - vma->gpuva.flags &= ~XE_VMA_DESTROYED;
> > - up_read(&vm->userptr.notifier_lock);
> > - if (post_commit)
> > - xe_vm_insert_vma(vm, vma);
> > + if (vma) {
> > + down_read(&vm->userptr.notifier_lock);
> > + vma->gpuva.flags &= ~XE_VMA_DESTROYED;
> > + up_read(&vm->userptr.notifier_lock);
> > + if (post_commit)
> > + xe_vm_insert_vma(vm, vma);
> > + }
> > break;
> > }
> > case DRM_GPUVA_OP_REMAP:
> > @@ -2897,11 +2896,13 @@ static void xe_vma_op_unwind(struct xe_vm *vm, struct xe_vma_op *op,
> > prep_vma_destroy(vm, op->remap.next, next_post_commit);
> > xe_vma_destroy_unlocked(op->remap.next);
> > }
> > - down_read(&vm->userptr.notifier_lock);
> > - vma->gpuva.flags &= ~XE_VMA_DESTROYED;
> > - up_read(&vm->userptr.notifier_lock);
> > - if (post_commit)
> > - xe_vm_insert_vma(vm, vma);
> > + if (vma) {
> > + down_read(&vm->userptr.notifier_lock);
> > + vma->gpuva.flags &= ~XE_VMA_DESTROYED;
>
> wouldn't we need to clear the other new commited flags here?
>
No sure what you mean the commit flag is per op, we unwind any ops that have been commited.
Matt
> > + up_read(&vm->userptr.notifier_lock);
> > + if (post_commit)
> > + xe_vm_insert_vma(vm, vma);
> > + }
> > break;
> > }
> > case DRM_GPUVA_OP_PREFETCH:
> > @@ -2990,20 +2991,16 @@ static void xe_vma_op_work_func(struct work_struct *w)
> > }
> > }
> >
> > -static int vm_bind_ioctl_ops_commit(struct xe_vm *vm,
> > - struct list_head *ops_list, bool async)
> > +static int vm_bind_ioctl_ops_execute(struct xe_vm *vm,
> > + struct list_head *ops_list, bool async)
> > {
> > struct xe_vma_op *op, *last_op, *next;
> > int err;
> >
> > lockdep_assert_held_write(&vm->lock);
> >
> > - list_for_each_entry(op, ops_list, link) {
> > + list_for_each_entry(op, ops_list, link)
> > last_op = op;
> > - err = xe_vma_op_commit(vm, op);
> > - if (err)
> > - goto unwind;
> > - }
> >
> > if (!async) {
> > err = xe_vma_op_execute(vm, last_op);
> > @@ -3042,28 +3039,29 @@ static int vm_bind_ioctl_ops_commit(struct xe_vm *vm,
> > return err;
> > }
> >
> > -/*
> > - * Unwind operations list, called after a failure of vm_bind_ioctl_ops_create or
> > - * vm_bind_ioctl_ops_parse.
> > - */
> > static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
> > struct drm_gpuva_ops **ops,
> > int num_ops_list)
> > {
> > int i;
> >
> > - for (i = 0; i < num_ops_list; ++i) {
> > + for (i = num_ops_list - 1; i; ++i) {
> > struct drm_gpuva_ops *__ops = ops[i];
> > struct drm_gpuva_op *__op;
> >
> > if (!__ops)
> > continue;
> >
> > - drm_gpuva_for_each_op(__op, __ops) {
> > + drm_gpuva_for_each_op_reverse(__op, __ops) {
> > struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> >
> > - xe_vma_op_unwind(vm, op, false, false, false);
> > + xe_vma_op_unwind(vm, op,
> > + op->flags & XE_VMA_OP_COMMITTED,
> > + op->flags & XE_VMA_OP_PREV_COMMITTED,
> > + op->flags & XE_VMA_OP_NEXT_COMMITTED);
> > }
> > +
> > + drm_gpuva_ops_free(&vm->mgr, __ops);
> > }
> > }
> >
> > @@ -3384,14 +3382,22 @@ int xe_vm_bind_ioctl(struct drm_device *dev, void *data, struct drm_file *file)
> > ops[i] = NULL;
> > goto unwind_ops;
> > }
> > +
> > + err = vm_bind_ioctl_ops_parse(vm, q, ops[i], syncs, num_syncs,
> > + &ops_list,
> > + i == args->num_binds - 1,
> > + async);
> > + if (err)
> > + goto unwind_ops;
> > }
> >
> > - err = vm_bind_ioctl_ops_parse(vm, q, ops, args->num_binds,
> > - syncs, num_syncs, &ops_list, async);
> > - if (err)
> > + /* Nothing to do */
> > + if (list_empty(&ops_list)) {
> > + err = -ENODATA;
> > goto unwind_ops;
> > + }
> >
> > - err = vm_bind_ioctl_ops_commit(vm, &ops_list, async);
> > + err = vm_bind_ioctl_ops_execute(vm, &ops_list, async);
> > up_write(&vm->lock);
> >
> > for (i = 0; i < args->num_binds; ++i)
> > --
> > 2.34.1
> >
More information about the Intel-xe
mailing list