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

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

> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Friday, March 22, 2024 1:36 PM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH v4 03/30] drm/xe: Move migrate to prefetch to op_lock
> function
> 
> 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.


Have you ever think removing the prefetch API from vm_bind?

One idea is move prefetch to the memory attributes (aka madvise) API we are building for system allocator. From the semantics of prefetch, it is more reasonable to make it part of madvise, vs vm_bind. What do you think?

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

Yes, that looks better.

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