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

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

On Mon, Mar 25, 2024 at 10:51:43AM -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 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@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...
> 

Yes, let me rename. How about vm_bind_ioctl_ops_execute_fini?

'destroyed the temporary vmas during vm_ioctl' - This is destroying
unmapped VMAs when the fence signals.

> 
> > +{
> > +	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)?
> 
> 

This, xe_sync_entry_signal, is a bad name. It really should be
xe_sync_entry_install_fence or something like that. It is really is
installing the fence in all out-syncs, the fence signals, then the
out-fence signals.

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

This isn't destroying the fence - it dropping a reference. This is
reference return from xe_vm_ops_execute, we install the fence anywhere
needed (this might take more refs) then drop the ref from
xe_vm_ops_execute.

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