[PATCH v4 10/30] drm/xe: Add vm_bind_ioctl_ops_install_fences helper
Zeng, Oak
oak.zeng at intel.com
Mon Mar 25 19:44:46 UTC 2024
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Monday, March 25, 2024 3:35 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH v4 10/30] drm/xe: Add vm_bind_ioctl_ops_install_fences
> helper
>
> On Mon, Mar 25, 2024 at 10:51:43AM -0600, Zeng, Oak wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> Matthew
> > > Brost
> > > Sent: Friday, March 8, 2024 12:08 AM
> > > To: intel-xe at lists.freedesktop.org
> > > Cc: Brost, Matthew <matthew.brost at 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 at 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?
Yes that sounds better.
>
> 'destroyed the temporary vmas during vm_ioctl' - This is destroying
> unmapped VMAs when the fence signals.
I see.
>
> >
> > > +{
> > > + 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.
Ah! That make sense. Now it is more clear to me.
>
> > > + 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.
Ok, that make sense.
Oak
>
> 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
> >
More information about the Intel-xe
mailing list