RE: [PATCH v4 12/30] drm/xe: Move ufence check to op_lock

["Zeng, Oak" <oak.zeng@intel.com>]

Patch makes sense to me. See one comment inline

> -----Original Message-----
> From: Intel-xe <intel-xe-bounces@lists.freedesktop.org> On Behalf Of Matthew
> Brost
> Sent: Friday, March 8, 2024 12:08 AM
> To: intel-xe@lists.freedesktop.org
> Cc: Brost, Matthew <matthew.brost@intel.com>
> Subject: [PATCH v4 12/30] drm/xe: Move ufence check to op_lock
> 
> Rather than checking for an unsignaled ufence ay unbind time, check for
> this during the op_lock function. This will help with the transition to
> job 1 per VM bind IOCTL.
> 
> Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_vm.c | 33 +++++++++++++++++++++++----------
>  1 file changed, 23 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> index 4432531f39fe..5767955529dd 100644
> --- a/drivers/gpu/drm/xe/xe_vm.c
> +++ b/drivers/gpu/drm/xe/xe_vm.c
> @@ -1665,16 +1665,6 @@ xe_vm_unbind_vma(struct xe_vma *vma, struct
> xe_exec_queue *q,
> 
>  	trace_xe_vma_unbind(vma);
> 
> -	if (vma->ufence) {
> -		struct xe_user_fence * const f = vma->ufence;
> -
> -		if (!xe_sync_ufence_get_status(f))
> -			return ERR_PTR(-EBUSY);
> -
> -		vma->ufence = NULL;
> -		xe_sync_ufence_put(f);
> -	}
> -
>  	if (number_tiles > 1) {
>  		fences = kmalloc_array(number_tiles, sizeof(*fences),
>  				       GFP_KERNEL);
> @@ -2721,6 +2711,21 @@ static int vma_lock(struct drm_exec *exec, struct
> xe_vma *vma, bool validate)
>  	return err;
>  }
> 
> +static int check_ufence(struct xe_vma *vma)
> +{
> +	if (vma->ufence) {
> +		struct xe_user_fence * const f = vma->ufence;
> +
> +		if (!xe_sync_ufence_get_status(f))
> +			return -EBUSY;
> +
> +		vma->ufence = NULL;
> +		xe_sync_ufence_put(f);
> +	}
> +
> +	return 0;
> +}
> +
>  static int op_lock(struct drm_exec *exec, struct xe_vm *vm,
>  		   struct xe_vma_op *op)
>  {
> @@ -2731,6 +2736,10 @@ static int op_lock(struct drm_exec *exec, struct xe_vm
> *vm,
>  		err = vma_lock(exec, op-
> >map.vma, !xe_vm_in_fault_mode(vm));
>  		break;
>  	case DRM_GPUVA_OP_REMAP:
> +		err = check_ufence(gpuva_to_vma(op->base.remap.unmap-
> >va));

This is another reason we need to rename op_lock...

Oak

> +		if (err)
> +			break;
> +
>  		err = vma_lock(exec, gpuva_to_vma(op->base.remap.unmap-
> >va),
>  			       false);
>  		if (!err && op->remap.prev)
> @@ -2739,6 +2748,10 @@ static int op_lock(struct drm_exec *exec, struct xe_vm
> *vm,
>  			err = vma_lock(exec, op->remap.next, true);
>  		break;
>  	case DRM_GPUVA_OP_UNMAP:
> +		err = check_ufence(gpuva_to_vma(op->base.unmap.va));
> +		if (err)
> +			break;
> +
>  		err = vma_lock(exec, gpuva_to_vma(op->base.unmap.va), false);
>  		break;
>  	case DRM_GPUVA_OP_PREFETCH:
> --
> 2.34.1