Re: [PATCH v4 11/30] drm/xe: Move setting last fence to vm_bind_ioctl_ops_install_fences

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

On Mon, Mar 25, 2024 at 11:02:29AM -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 11/30] drm/xe: Move setting last fence to
> > vm_bind_ioctl_ops_install_fences
> > 
> > This moves setting of the last fence to a single location.
> > 
> > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > ---
> >  drivers/gpu/drm/xe/xe_vm.c | 9 ++-------
> >  1 file changed, 2 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 8c96c98cba37..4432531f39fe 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -1841,9 +1841,6 @@ xe_vm_bind(struct xe_vm *vm, struct xe_vma *vma,
> > struct xe_exec_queue *q,
> >  		fence = xe_exec_queue_last_fence_get(wait_exec_queue, vm);
> >  	}
> > 
> > -	if (last_op)
> > -		xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> > -
> >  	return fence;
> >  }
> > 
> > @@ -1853,7 +1850,6 @@ xe_vm_unbind(struct xe_vm *vm, struct xe_vma *vma,
> >  	     u32 num_syncs, bool first_op, bool last_op)
> >  {
> >  	struct dma_fence *fence;
> > -	struct xe_exec_queue *wait_exec_queue = to_wait_exec_queue(vm,
> > q);
> > 
> >  	xe_vm_assert_held(vm);
> >  	xe_bo_assert_held(xe_vma_bo(vma));
> > @@ -1862,9 +1858,6 @@ xe_vm_unbind(struct xe_vm *vm, struct xe_vma *vma,
> >  	if (IS_ERR(fence))
> >  		return fence;
> > 
> > -	if (last_op)
> > -		xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> > -
> >  	return fence;
> >  }
> > 
> > @@ -2820,6 +2813,7 @@ static void vm_bind_ioctl_ops_install_fences(struct
> > xe_vm *vm,
> >  					     struct xe_vma_ops *vops,
> >  					     struct dma_fence *fence)
> >  {
> > +	struct xe_exec_queue *wait_exec_queue = to_wait_exec_queue(vm,
> > vops->q);
> >  	struct xe_vma_op *op;
> >  	int i;
> > 
> > @@ -2832,6 +2826,7 @@ static void vm_bind_ioctl_ops_install_fences(struct
> > xe_vm *vm,
> >  	}
> >  	for (i = 0; i < vops->num_syncs; i++)
> >  		xe_sync_entry_signal(vops->syncs + i, NULL, fence);
> > +	xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> 
> 
> Oh, With this patch, I can understand the dma-fence-put below won't destroy the last fence, because this fence is set to exec queue's last fence.
> 

Yes, xe_exec_queue_last_fence_set in this case takes a ref.

> I also understand now, how the last fence will be used to signal the out-fences of vm_bind ioctl... things make sense to me now...
> 
> IMO, maybe you can squash this patch to the last patch, so people don't have the confusion as I had.
>

Will squash.

Matt

> Oak
> >  	dma_fence_put(fence);
> >  }
> > 
> > --
> > 2.34.1
>