Re: [PATCH v4 03/30] drm/xe: Move migrate to prefetch to op_lock function

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

On Fri, Mar 22, 2024 at 11:06:28AM -0600, Zeng, Oak wrote:
> 
> 
> > -----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 03/30] drm/xe: Move migrate to prefetch to op_lock function
> > 
> > Migrates need to be done under drm exec to make lockdep happy,
> 
> 
> Can you explain more here? By under drm exec, do you mean drm_exec_until_all_locked? I do see xe_vm_prefetch/xe_bo_migrate is called inside drm_exec_until_all_locked, in patch 1 of this series, in function vm_bind_ioctl_ops_execute
> 

This is a stale comment, will fix. Something like:

All non-binding operations in VM bind IOCTL should be in the lock and
prepare step rather than the execution step. Move prefetch to conform to
this pattern.

>  move
> > the migrate done for prefetches under the op_lock function.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_vm.c | 28 +++++++++++++---------------
> >  1 file changed, 13 insertions(+), 15 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index fb73afcab3b7..70a5ba621e4e 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1994,20 +1994,10 @@ static const u32 region_to_mem_type[] = {
> > 
> >  static struct dma_fence *
> >  xe_vm_prefetch(struct xe_vm *vm, struct xe_vma *vma,
> > -	       struct xe_exec_queue *q, u32 region,
> > -	       struct xe_sync_entry *syncs, u32 num_syncs,
> > -	       bool first_op, bool last_op)
> > +	       struct xe_exec_queue *q, struct xe_sync_entry *syncs,
> > +	       u32 num_syncs, bool first_op, bool last_op)
> >  {
> >  	struct xe_exec_queue *wait_exec_queue = to_wait_exec_queue(vm,
> > q);
> > -	int err;
> > -
> > -	xe_assert(vm->xe, region <= ARRAY_SIZE(region_to_mem_type));
> > -
> > -	if (!xe_vma_has_no_bo(vma)) {
> > -		err = xe_bo_migrate(xe_vma_bo(vma),
> > region_to_mem_type[region]);
> > -		if (err)
> > -			return ERR_PTR(err);
> > -	}
> > 
> >  	if (vma->tile_mask != (vma->tile_present & ~vma->usm.tile_invalidated))
> > {
> >  		return xe_vm_bind(vm, vma, q, xe_vma_bo(vma), syncs,
> > num_syncs,
> > @@ -2540,8 +2530,7 @@ static struct dma_fence *op_execute(struct xe_vm
> > *vm, struct xe_vma *vma,
> >  				     op->flags & XE_VMA_OP_LAST);
> >  		break;
> >  	case DRM_GPUVA_OP_PREFETCH:
> > -		fence = xe_vm_prefetch(vm, vma, op->q, op->prefetch.region,
> > -				       op->syncs, op->num_syncs,
> > +		fence = xe_vm_prefetch(vm, vma, op->q, op->syncs, op-
> > >num_syncs,
> >  				       op->flags & XE_VMA_OP_FIRST,
> >  				       op->flags & XE_VMA_OP_LAST);
> >  		break;
> > @@ -2766,8 +2755,17 @@ static int op_lock(struct drm_exec *exec, struct xe_vm
> > *vm,
> >  		err = vma_lock(exec, gpuva_to_vma(op->base.unmap.va), false);
> >  		break;
> >  	case DRM_GPUVA_OP_PREFETCH:
> > -		err = vma_lock(exec, gpuva_to_vma(op->base.prefetch.va),
> > true);
> > +	{
> > +		struct xe_vma *vma = gpuva_to_vma(op->base.prefetch.va);
> > +		u32 region = op->prefetch.region;
> > +
> > +		xe_assert(vm->xe, region <=
> > ARRAY_SIZE(region_to_mem_type));
> > +
> > +		err = vma_lock(exec, vma, false);
> > +		if (!err && !xe_vma_has_no_bo(vma))
> > +			err = xe_bo_migrate(xe_vma_bo(vma), region);
> >  		break;
> > +	}
> 
> Understand you have a reason to do this. It does introduce confusion: the function is called op_lock and now you have a migration operation inside.
> 

Yes, how about? s/op_lock/op_lock_and_prepare/

Likewise: s/vma_lock/vma_lock/vma_lock_and_validate/

Matt

> 
> Oak
> 
> 
> >  	default:
> >  		drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> >  	}
> > --
> > 2.34.1
>