[PATCH 06/13] drm/xe: Simplify VM bind IOCTL error handling and cleanup

[Zeng, Oak]

It is a nice clean up. See one question inline

> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Wednesday, April 10, 2024 1:41 AM
> To: intel-xe at lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost at intel.com>; Zeng, Oak
> <oak.zeng at intel.com>
> Subject: [PATCH 06/13] drm/xe: Simplify VM bind IOCTL error handling and
> cleanup
> 
> Clean up everything in VM bind IOCTL in 1 path for both errors and
> non-errors. Also move VM bind IOCTL cleanup from ops (also used by
> non-IOCTL binds) to the VM bind IOCTL.
> 
> v2:
>  - Break ops_execute on error (Oak)
> 
> Cc: Oak Zeng <oak.zeng at intel.com>
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c       | 67 ++++++--------------------------
>  drivers/gpu/drm/xe/xe_vm_types.h |  5 ---
>  2 files changed, 12 insertions(+), 60 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 9d82396cf5d5..8f5b24c8f6cd 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -842,8 +842,7 @@ static int xe_vm_ops_add_rebind(struct xe_vma_ops
> *vops, struct xe_vma *vma,
>  }
> 
>  static struct dma_fence *ops_execute(struct xe_vm *vm,
> -				     struct xe_vma_ops *vops,
> -				     bool cleanup);
> +				     struct xe_vma_ops *vops);
>  static void xe_vma_ops_init(struct xe_vma_ops *vops);
> 
>  int xe_vm_rebind(struct xe_vm *vm, bool rebind_worker)
> @@ -876,7 +875,7 @@ int xe_vm_rebind(struct xe_vm *vm, bool
> rebind_worker)
>  			goto free_ops;
>  	}
> 
> -	fence = ops_execute(vm, &vops, false);
> +	fence = ops_execute(vm, &vops);
>  	if (IS_ERR(fence)) {
>  		err = PTR_ERR(fence);
>  	} else {
> @@ -2551,7 +2550,6 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> *vm, struct xe_exec_queue *q,
>  	if (!last_op)
>  		return 0;
> 
> -	last_op->ops = ops;
>  	if (last) {
>  		last_op->flags |= XE_VMA_OP_LAST;
>  		last_op->num_syncs = num_syncs;
> @@ -2721,25 +2719,6 @@ xe_vma_op_execute(struct xe_vm *vm, struct
> xe_vma_op *op)
>  	return fence;
>  }
> 
> -static void xe_vma_op_cleanup(struct xe_vm *vm, struct xe_vma_op *op)
> -{
> -	bool last = op->flags & XE_VMA_OP_LAST;
> -
> -	if (last) {
> -		while (op->num_syncs--)
> -			xe_sync_entry_cleanup(&op->syncs[op-
> >num_syncs]);

I understand all the other part of this function is not needed anymore. But I didn't figure out why sync_entry_cleanup is not needed? You still have syncs, do you? They are allocated in bind_ioctl function and seems you didn't touch it in this patch....Can you explain?

Oak


> -		kfree(op->syncs);
> -		if (op->q)
> -			xe_exec_queue_put(op->q);
> -	}
> -	if (!list_empty(&op->link))
> -		list_del(&op->link);
> -	if (op->ops)
> -		drm_gpuva_ops_free(&vm->gpuvm, op->ops);
> -	if (last)
> -		xe_vm_put(vm);
> -}
> -
>  static void xe_vma_op_unwind(struct xe_vm *vm, struct xe_vma_op *op,
>  			     bool post_commit, bool prev_post_commit,
>  			     bool next_post_commit)
> @@ -2816,8 +2795,6 @@ static void vm_bind_ioctl_ops_unwind(struct
> xe_vm *vm,
>  					 op->flags &
> XE_VMA_OP_PREV_COMMITTED,
>  					 op->flags &
> XE_VMA_OP_NEXT_COMMITTED);
>  		}
> -
> -		drm_gpuva_ops_free(&vm->gpuvm, __ops);
>  	}
>  }
> 
> @@ -2904,24 +2881,20 @@ static int
> vm_bind_ioctl_ops_lock_and_prep(struct drm_exec *exec,
>  }
> 
>  static struct dma_fence *ops_execute(struct xe_vm *vm,
> -				     struct xe_vma_ops *vops,
> -				     bool cleanup)
> +				     struct xe_vma_ops *vops)
>  {
>  	struct xe_vma_op *op, *next;
>  	struct dma_fence *fence = NULL;
> 
>  	list_for_each_entry_safe(op, next, &vops->list, link) {
> -		if (!IS_ERR(fence)) {
> -			dma_fence_put(fence);
> -			fence = xe_vma_op_execute(vm, op);
> -		}
> +		dma_fence_put(fence);
> +		fence = xe_vma_op_execute(vm, op);
>  		if (IS_ERR(fence)) {
>  			drm_warn(&vm->xe->drm, "VM op(%d) failed
> with %ld",
>  				 op->base.op, PTR_ERR(fence));
>  			fence = ERR_PTR(-ENOSPC);
> +			break;
>  		}
> -		if (cleanup)
> -			xe_vma_op_cleanup(vm, op);
>  	}
> 
>  	return fence;
> @@ -2944,7 +2917,7 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm
> *vm,
>  		if (err)
>  			goto unlock;
> 
> -		fence = ops_execute(vm, vops, true);
> +		fence = ops_execute(vm, vops);
>  		if (IS_ERR(fence)) {
>  			err = PTR_ERR(fence);
>  			/* FIXME: Killing VM rather than proper error
> handling */
> @@ -3305,30 +3278,14 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> void *data, struct drm_file *file)
>  		goto unwind_ops;
>  	}
> 
> -	xe_vm_get(vm);
> -	if (q)
> -		xe_exec_queue_get(q);
> -
>  	err = vm_bind_ioctl_ops_execute(vm, &vops);
> 
> -	up_write(&vm->lock);
> -
> -	if (q)
> -		xe_exec_queue_put(q);
> -	xe_vm_put(vm);
> -
> -	for (i = 0; bos && i < args->num_binds; ++i)
> -		xe_bo_put(bos[i]);
> -
> -	kvfree(bos);
> -	kvfree(ops);
> -	if (args->num_binds > 1)
> -		kvfree(bind_ops);
> -
> -	return err;
> -
>  unwind_ops:
> -	vm_bind_ioctl_ops_unwind(vm, ops, args->num_binds);
> +	if (err && err != -ENODATA)
> +		vm_bind_ioctl_ops_unwind(vm, ops, args->num_binds);
> +	for (i = args->num_binds - 1; i >= 0; --i)
> +		if (ops[i])
> +			drm_gpuva_ops_free(&vm->gpuvm, ops[i]);
>  free_syncs:
>  	if (err == -ENODATA)
>  		err = vm_bind_ioctl_signal_fences(vm, q, syncs, num_syncs);
> diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> b/drivers/gpu/drm/xe/xe_vm_types.h
> index 466b6c62d1f9..149ab892967e 100644
> --- a/drivers/gpu/drm/xe/xe_vm_types.h
> +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> @@ -330,11 +330,6 @@ enum xe_vma_op_flags {
>  struct xe_vma_op {
>  	/** @base: GPUVA base operation */
>  	struct drm_gpuva_op base;
> -	/**
> -	 * @ops: GPUVA ops, when set call drm_gpuva_ops_free after this
> -	 * operations is processed
> -	 */
> -	struct drm_gpuva_ops *ops;
>  	/** @q: exec queue for this operation */
>  	struct xe_exec_queue *q;
>  	/**
> --
> 2.34.1