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

[Matthew Brost <matthew.brost@intel.com>]

On Mon, Mar 25, 2024 at 02:37:28PM -0600, Zeng, Oak wrote:
> 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...
> 

Agree.

Matt

> 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
>