[Intel-xe] [PATCH 0/4] Fix array of binds

[Intel-xe] [PATCH 0/4] Fix array of binds

[Matthew Brost]

Patches should explain the issue, fixes:
https://gitlab.freedesktop.org/drm/xe/kernel/-/issues/528

Matthew Brost (4):
  drm/xe: Covert xe_vma_op_flags to BIT macros
  drm/xe: Fixup unwind on VM ops errors
  drm/gpuva: Add drm_gpuva_for_each_op_reverse
  drm/xe: Fix array of binds

 drivers/gpu/drm/xe/xe_vm.c       | 422 ++++++++++++++++---------------
 drivers/gpu/drm/xe/xe_vm_types.h |  10 +-
 include/drm/drm_gpuva_mgr.h      |  10 +
 3 files changed, 236 insertions(+), 206 deletions(-)

-- 
2.34.1

[Intel-xe] [PATCH 1/4] drm/xe: Covert xe_vma_op_flags to BIT macros

[Matthew Brost]

Rather than open code the shift for values, use BIT macros.

Signed-of-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_vm_types.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 3681a5ff588b..4bcb54ccd93a 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -377,11 +377,11 @@ struct xe_vma_op_prefetch {
 /** enum xe_vma_op_flags - flags for VMA operation */
 enum xe_vma_op_flags {
 	/** @XE_VMA_OP_FIRST: first VMA operation for a set of syncs */
-	XE_VMA_OP_FIRST		= (0x1 << 0),
+	XE_VMA_OP_FIRST		= BIT(0),
 	/** @XE_VMA_OP_LAST: last VMA operation for a set of syncs */
-	XE_VMA_OP_LAST		= (0x1 << 1),
+	XE_VMA_OP_LAST		= BIT(1),
 	/** @XE_VMA_OP_COMMITTED: VMA operation committed */
-	XE_VMA_OP_COMMITTED	= (0x1 << 2),
+	XE_VMA_OP_COMMITTED	= BIT(2),
 };
 
 /** struct xe_vma_op - VMA operation */
-- 
2.34.1

Re: [Intel-xe] [PATCH 1/4] drm/xe: Covert xe_vma_op_flags to BIT macros

[Rodrigo Vivi]

On Thu, Aug 31, 2023 at 08:00:45AM -0700, Matthew Brost wrote:
> Rather than open code the shift for values, use BIT macros.
> 
> Signed-of-by: Matthew Brost <matthew.brost@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/xe/xe_vm_types.h | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> index 3681a5ff588b..4bcb54ccd93a 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -377,11 +377,11 @@ struct xe_vma_op_prefetch {
>  /** enum xe_vma_op_flags - flags for VMA operation */
>  enum xe_vma_op_flags {
>  	/** @XE_VMA_OP_FIRST: first VMA operation for a set of syncs */
> -	XE_VMA_OP_FIRST		= (0x1 << 0),
> +	XE_VMA_OP_FIRST		= BIT(0),
>  	/** @XE_VMA_OP_LAST: last VMA operation for a set of syncs */
> -	XE_VMA_OP_LAST		= (0x1 << 1),
> +	XE_VMA_OP_LAST		= BIT(1),
>  	/** @XE_VMA_OP_COMMITTED: VMA operation committed */
> -	XE_VMA_OP_COMMITTED	= (0x1 << 2),
> +	XE_VMA_OP_COMMITTED	= BIT(2),
>  };
>  
>  /** struct xe_vma_op - VMA operation */
> -- 
> 2.34.1
> 

[Intel-xe] [PATCH 2/4] drm/xe: Fixup unwind on VM ops errors

[Matthew Brost]

Remap ops have 3 parts: unmap, prev, and next. The commit step can fail
on any of these. Add a flag for each to these so the unwind is only done
the steps that have been committed.

v2: (Rodrigo) Use bit macros

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>
Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 drivers/gpu/drm/xe/xe_vm.c       | 24 +++++++++++++++++-------
 drivers/gpu/drm/xe/xe_vm_types.h | 10 +++++++---
 2 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
index 15bff0783ec9..7d64da0b3b70 100644
--- a/drivers/gpu/drm/xe/xe_vm.c
+++ b/drivers/gpu/drm/xe/xe_vm.c
@@ -2618,18 +2618,25 @@ static int xe_vma_op_commit(struct xe_vm *vm, struct xe_vma_op *op)
 	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;
 		}
@@ -2642,15 +2649,15 @@ static int xe_vma_op_commit(struct xe_vm *vm, struct xe_vma_op *op)
 		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:
-		/* Nothing to do */
+		op->flags |= XE_VMA_OP_COMMITTED;
 		break;
 	default:
 		XE_WARN_ON("NOT POSSIBLE");
 	}
 
-	op->flags |= XE_VMA_OP_COMMITTED;
 	return err;
 }
 
@@ -2855,7 +2862,8 @@ static void xe_vma_op_cleanup(struct xe_vm *vm, struct xe_vma_op *op)
 }
 
 static void xe_vma_op_unwind(struct xe_vm *vm, struct xe_vma_op *op,
-			     bool post_commit)
+			     bool post_commit, bool prev_post_commit,
+			     bool next_post_commit)
 {
 	lockdep_assert_held_write(&vm->lock);
 
@@ -2882,11 +2890,11 @@ static void xe_vma_op_unwind(struct xe_vm *vm, struct xe_vma_op *op,
 		struct xe_vma *vma = gpuva_to_vma(op->base.remap.unmap->va);
 
 		if (op->remap.prev) {
-			prep_vma_destroy(vm, op->remap.prev, post_commit);
+			prep_vma_destroy(vm, op->remap.prev, prev_post_commit);
 			xe_vma_destroy_unlocked(op->remap.prev);
 		}
 		if (op->remap.next) {
-			prep_vma_destroy(vm, op->remap.next, post_commit);
+			prep_vma_destroy(vm, op->remap.next, next_post_commit);
 			xe_vma_destroy_unlocked(op->remap.next);
 		}
 		down_read(&vm->userptr.notifier_lock);
@@ -3025,7 +3033,9 @@ static int vm_bind_ioctl_ops_commit(struct xe_vm *vm,
 
 unwind:
 	list_for_each_entry_reverse(op, ops_list, link)
-		xe_vma_op_unwind(vm, op, op->flags & XE_VMA_OP_COMMITTED);
+		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);
 	list_for_each_entry_safe(op, next, ops_list, link)
 		xe_vma_op_cleanup(vm, op);
 
@@ -3052,7 +3062,7 @@ static void vm_bind_ioctl_ops_unwind(struct xe_vm *vm,
 		drm_gpuva_for_each_op(__op, __ops) {
 			struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
 
-			xe_vma_op_unwind(vm, op, false);
+			xe_vma_op_unwind(vm, op, false, false, false);
 		}
 	}
 }
diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
index 4bcb54ccd93a..286b015b201d 100644
--- a/drivers/gpu/drm/xe/xe_vm_types.h
+++ b/drivers/gpu/drm/xe/xe_vm_types.h
@@ -377,11 +377,15 @@ struct xe_vma_op_prefetch {
 /** enum xe_vma_op_flags - flags for VMA operation */
 enum xe_vma_op_flags {
 	/** @XE_VMA_OP_FIRST: first VMA operation for a set of syncs */
-	XE_VMA_OP_FIRST		= BIT(0),
+	XE_VMA_OP_FIRST			= BIT(0),
 	/** @XE_VMA_OP_LAST: last VMA operation for a set of syncs */
-	XE_VMA_OP_LAST		= BIT(1),
+	XE_VMA_OP_LAST			= BIT(1),
 	/** @XE_VMA_OP_COMMITTED: VMA operation committed */
-	XE_VMA_OP_COMMITTED	= BIT(2),
+	XE_VMA_OP_COMMITTED		= BIT(2),
+	/** @XE_VMA_OP_PREV_COMMITTED: Previous VMA operation committed */
+	XE_VMA_OP_PREV_COMMITTED	= BIT(3),
+	/** @XE_VMA_OP_NEXT_COMMITTED: Next VMA operation committed */
+	XE_VMA_OP_NEXT_COMMITTED	= BIT(4),
 };
 
 /** struct xe_vma_op - VMA operation */
-- 
2.34.1

[Intel-xe] [PATCH 3/4] drm/gpuva: Add drm_gpuva_for_each_op_reverse

[Matthew Brost]

Xe will make use of this, add helper to walk op list in reverse when
unwinding GPUVA operations.

Signed-off-by: Matthew Brost <matthew.brost@intel.com>
---
 include/drm/drm_gpuva_mgr.h | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
index ed8d50200cc3..b2024ae19fd4 100644
--- a/include/drm/drm_gpuva_mgr.h
+++ b/include/drm/drm_gpuva_mgr.h
@@ -557,6 +557,16 @@ struct drm_gpuva_ops {
 #define drm_gpuva_for_each_op_from_reverse(op, ops) \
 	list_for_each_entry_from_reverse(op, &(ops)->list, entry)
 
+/**
+ * drm_gpuva_for_each_op_reverse - iterator to walk over &drm_gpuva_ops in reverse
+ * @op: &drm_gpuva_op to assign in each iteration step
+ * @ops: &drm_gpuva_ops to walk
+ *
+ * This iterator walks over all ops within a given list of operations in reverse
+ */
+#define drm_gpuva_for_each_op_reverse(op, ops) \
+	list_for_each_entry_reverse(op, &(ops)->list, entry)
+
 /**
  * drm_gpuva_first_op() - returns the first &drm_gpuva_op from &drm_gpuva_ops
  * @ops: the &drm_gpuva_ops to get the fist &drm_gpuva_op from
-- 
2.34.1

Re: [Intel-xe] [PATCH 3/4] drm/gpuva: Add drm_gpuva_for_each_op_reverse

[Rodrigo Vivi]

On Thu, Aug 31, 2023 at 08:00:47AM -0700, Matthew Brost wrote:
> Xe will make use of this, add helper to walk op list in reverse when
> unwinding GPUVA operations.

I'm no native, but I got confused here. Would it be better if we
change the order like:

Add a helper to walk op list in reverse. Xe will make use of this
when unwinding GPUVA operations.

with the msg changed:
Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

This is a patch that is likely better to be propagated with
Xe and not earlier with drm-misc. We will need an explicit ack
from Dave/Daniel on this.

> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  include/drm/drm_gpuva_mgr.h | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/include/drm/drm_gpuva_mgr.h b/include/drm/drm_gpuva_mgr.h
> index ed8d50200cc3..b2024ae19fd4 100644
> --- a/include/drm/drm_gpuva_mgr.h
> +++ b/include/drm/drm_gpuva_mgr.h
> @@ -557,6 +557,16 @@ struct drm_gpuva_ops {
>  #define drm_gpuva_for_each_op_from_reverse(op, ops) \
>  	list_for_each_entry_from_reverse(op, &(ops)->list, entry)
>  
> +/**
> + * drm_gpuva_for_each_op_reverse - iterator to walk over &drm_gpuva_ops in reverse
> + * @op: &drm_gpuva_op to assign in each iteration step
> + * @ops: &drm_gpuva_ops to walk
> + *
> + * This iterator walks over all ops within a given list of operations in reverse
> + */
> +#define drm_gpuva_for_each_op_reverse(op, ops) \
> +	list_for_each_entry_reverse(op, &(ops)->list, entry)
> +
>  /**
>   * drm_gpuva_first_op() - returns the first &drm_gpuva_op from &drm_gpuva_ops
>   * @ops: the &drm_gpuva_ops to get the fist &drm_gpuva_op from
> -- 
> 2.34.1
> 

[Intel-xe] [PATCH 4/4] drm/xe: Fix array of binds

[Matthew Brost]

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@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 7d64da0b3b70..b8c50ffbb623 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;
+	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;
+			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

Re: [Intel-xe] [PATCH 4/4] drm/xe: Fix array of binds

[Rodrigo Vivi]

On Thu, Aug 31, 2023 at 08:00:48AM -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@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@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 7d64da0b3b70..b8c50ffbb623 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;
> +	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;
> +			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
> 

[Intel-xe] ✗ CI.Patch_applied: failure for Fix array of binds (rev2)

[Patchwork]

== Series Details ==

Series: Fix array of binds (rev2)
URL   : https://patchwork.freedesktop.org/series/122548/
State : failure

== Summary ==

=== Applying kernel patches on branch 'drm-xe-next' with base: ===
Base commit: 62acaf965 drm/xe: fix has_llc on rkl
=== git am output follows ===
Applying: drm/xe: Covert xe_vma_op_flags to BIT macros
Applying: drm/xe: Fixup unwind on VM ops errors
Applying: drm/gpuva: Add drm_gpuva_for_each_op_reverse
Applying: drm/xe: Fix array of binds