[PATCH v4 10/30] drm/xe: Add vm_bind_ioctl_ops_install_fences helper

[Zeng, Oak]

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of Matthew
> Brost
> Sent: Friday, March 8, 2024 12:08 AM
> To: intel-xe at lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost at intel.com>
> Subject: [PATCH v4 10/30] drm/xe: Add vm_bind_ioctl_ops_install_fences helper
> 
> Simplify VM bind code by signaling out-fences / destroying VMAs in a
> single location. Will help with transition single job for many bind ops.
> 
> Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c | 55 ++++++++++++++++----------------------
>  1 file changed, 23 insertions(+), 32 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index f8b27746e5a7..8c96c98cba37 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1658,7 +1658,7 @@ xe_vm_unbind_vma(struct xe_vma *vma, struct
> xe_exec_queue *q,
>  	struct dma_fence *fence = NULL;
>  	struct dma_fence **fences = NULL;
>  	struct dma_fence_array *cf = NULL;
> -	int cur_fence = 0, i;
> +	int cur_fence = 0;
>  	int number_tiles = hweight8(vma->tile_present);
>  	int err;
>  	u8 id;
> @@ -1716,10 +1716,6 @@ xe_vm_unbind_vma(struct xe_vma *vma, struct
> xe_exec_queue *q,
> 
>  	fence = cf ? &cf->base : !fence ?
>  		xe_exec_queue_last_fence_get(wait_exec_queue, vm) : fence;
> -	if (last_op) {
> -		for (i = 0; i < num_syncs; i++)
> -			xe_sync_entry_signal(&syncs[i], NULL, fence);
> -	}
> 
>  	return fence;
> 
> @@ -1743,7 +1739,7 @@ xe_vm_bind_vma(struct xe_vma *vma, struct
> xe_exec_queue *q,
>  	struct dma_fence **fences = NULL;
>  	struct dma_fence_array *cf = NULL;
>  	struct xe_vm *vm = xe_vma_vm(vma);
> -	int cur_fence = 0, i;
> +	int cur_fence = 0;
>  	int number_tiles = hweight8(vma->tile_mask);
>  	int err;
>  	u8 id;
> @@ -1790,12 +1786,6 @@ xe_vm_bind_vma(struct xe_vma *vma, struct
> xe_exec_queue *q,
>  		}
>  	}
> 
> -	if (last_op) {
> -		for (i = 0; i < num_syncs; i++)
> -			xe_sync_entry_signal(&syncs[i], NULL,
> -					     cf ? &cf->base : fence);
> -	}
> -
>  	return cf ? &cf->base : fence;
> 
>  err_fences:
> @@ -1847,15 +1837,8 @@ xe_vm_bind(struct xe_vm *vm, struct xe_vma *vma,
> struct xe_exec_queue *q,
>  		if (IS_ERR(fence))
>  			return fence;
>  	} else {
> -		int i;
> -
>  		xe_assert(vm->xe, xe_vm_in_fault_mode(vm));
> -
>  		fence = xe_exec_queue_last_fence_get(wait_exec_queue, vm);
> -		if (last_op) {
> -			for (i = 0; i < num_syncs; i++)
> -				xe_sync_entry_signal(&syncs[i], NULL, fence);
> -		}
>  	}
> 
>  	if (last_op)
> @@ -1879,7 +1862,6 @@ xe_vm_unbind(struct xe_vm *vm, struct xe_vma *vma,
>  	if (IS_ERR(fence))
>  		return fence;
> 
> -	xe_vma_destroy(vma, fence);
>  	if (last_op)
>  		xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> 
> @@ -2037,17 +2019,7 @@ xe_vm_prefetch(struct xe_vm *vm, struct xe_vma
> *vma,
>  		return xe_vm_bind(vm, vma, q, xe_vma_bo(vma), syncs,
> num_syncs,
>  				  vma->tile_mask, true, first_op, last_op);
>  	} else {
> -		struct dma_fence *fence =
> -			xe_exec_queue_last_fence_get(wait_exec_queue, vm);
> -		int i;
> -
> -		/* Nothing to do, signal fences now */
> -		if (last_op) {
> -			for (i = 0; i < num_syncs; i++)
> -				xe_sync_entry_signal(&syncs[i], NULL, fence);
> -		}
> -
> -		return fence;
> +		return xe_exec_queue_last_fence_get(wait_exec_queue, vm);
>  	}
>  }
> 
> @@ -2844,6 +2816,25 @@ struct dma_fence *xe_vm_ops_execute(struct xe_vm
> *vm, struct xe_vma_ops *vops)
>  	return fence;
>  }
> 
> +static void vm_bind_ioctl_ops_install_fences(struct xe_vm *vm,
> +					     struct xe_vma_ops *vops,
> +					     struct dma_fence *fence)

Is this a correct function name? from the codes below, you destroyed the temporary vmas during vm_ioctl, then signaled all the sync entries, then you destroyed the fence generated from the last operation.... it is more like a cleanup stage of the vm_bind.... But I don't quite understand the code, see below questions...


> +{
> +	struct xe_vma_op *op;
> +	int i;
> +
> +	list_for_each_entry(op, &vops->list, link) {
> +		if (op->base.op == DRM_GPUVA_OP_UNMAP)
> +			xe_vma_destroy(gpuva_to_vma(op->base.unmap.va),
> fence);
> +		else if (op->base.op == DRM_GPUVA_OP_REMAP)
> +			xe_vma_destroy(gpuva_to_vma(op-
> >base.remap.unmap->va),
> +				       fence);
> +	}
> +	for (i = 0; i < vops->num_syncs; i++)
> +		xe_sync_entry_signal(vops->syncs + i, NULL, fence);

This signals the out-fence of vm_bind ioctl. Isn't this be done *after* fence is signaled (aka means the last vm bind operation is done)?


> +	dma_fence_put(fence);


I know this is also in the original code below... but I also don't understand why we can destroy fence here. As I understand it, this fence is generated during vm_bind operations. This is the last fence. Shouldn't we wait this fence somewhere so we know all the vm bind operations have been complete? I need your help to understand the picture here.

Oak

> +}
> +
>  static int vm_bind_ioctl_ops_execute(struct xe_vm *vm,
>  				     struct xe_vma_ops *vops)
>  {
> @@ -2868,7 +2859,7 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm
> *vm,
>  			xe_vm_kill(vm, false);
>  			goto unlock;
>  		} else {
> -			dma_fence_put(fence);
> +			vm_bind_ioctl_ops_install_fences(vm, vops, fence);
>  		}
>  	}
> 
> --
> 2.34.1