[PATCH 11/13] drm/xe: Move ufence add to vm_bind_ioctl_ops_fini
Zeng, Oak
oak.zeng at intel.com
Tue Apr 23 03:36:00 UTC 2024
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Friday, April 19, 2024 3:45 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH 11/13] drm/xe: Move ufence add to
> vm_bind_ioctl_ops_fini
>
> On Fri, Apr 19, 2024 at 09:24:18AM -0600, Zeng, Oak wrote:
> >
> >
> > > -----Original Message-----
> > > From: Intel-xe <intel-xe-bounces at lists.freedesktop.org> On Behalf Of
> > > Matthew Brost
> > > Sent: Wednesday, April 10, 2024 1:41 AM
> > > To: intel-xe at lists.freedesktop.org
> > > Cc: Brost, Matthew <matthew.brost at intel.com>
> > > Subject: [PATCH 11/13] drm/xe: Move ufence add to
> vm_bind_ioctl_ops_fini
> > >
> > > Rather than adding a ufence to a VMA in the bind function, add the
> > > ufence to all VMAs in the IOCTL that require binds in
> > > vm_bind_ioctl_ops_install_fences.
> >
> > This is a typo right? From the codes, it should be vm_bind_ioctl_ops_fini
> >
>
> Yes, typo. Will fix in next rev.
>
> > I also want to make sure I understand here: so the ufence added to vma is
> *only* used to make sure last vma bind has been completed upon vma
> unbind time. So even though it is more natural to set ufence at bind function,
> it is safe to set it after all operations are submitted (vm_bind_ioctl_ops_fini
> func). No vm_bind ioctl (and the vma unbind triggered by ioctl) can go
> through *before* the last vm bind ioctl's ops finish, right?
> >
>
> The ufence is attached to all VMAs being bound in an IOCTL. It prevents
> any of those VMAs from being unbound until the attached ufence has
> signaled (binding operation complete).
>
> It is safe (and correct) to attach the ufence *after* operations
> submitted because we are past the point of failure and are under the
> VM->lock. The ufence could be signaled or unsignaled when attached to
> the VMA. Safe to attach signaled ufences due to ref counting. Future
> IOCTLs return -EBUSY if trying to unbind a VMA which an unsignaled
> ufence.
Thanks for explaining. Patch LGTM:
Reviewed-by: Oak Zeng <oak.zeng at intel.com>
>
> Matt
>
> > Oak
> >
> >
> >
> > This will help with the transition to
> > > job 1 per VM bind IOCTL.
> > >
> > > v2:
> > > - Rebase
> > >
> > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_sync.c | 15 ++++++++++++
> > > drivers/gpu/drm/xe/xe_sync.h | 1 +
> > > drivers/gpu/drm/xe/xe_vm.c | 44
> ++++++++++++++++++++++++++++++--
> > > ----
> > > 3 files changed, 53 insertions(+), 7 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_sync.c
> b/drivers/gpu/drm/xe/xe_sync.c
> > > index 65f1f1628235..2883d9aca404 100644
> > > --- a/drivers/gpu/drm/xe/xe_sync.c
> > > +++ b/drivers/gpu/drm/xe/xe_sync.c
> > > @@ -338,6 +338,21 @@ xe_sync_in_fence_get(struct xe_sync_entry
> *sync,
> > > int num_sync,
> > > return ERR_PTR(-ENOMEM);
> > > }
> > >
> > > +/**
> > > + * __xe_sync_ufence_get() - Get user fence from user fence
> > > + * @ufence: input user fence
> > > + *
> > > + * Get a user fence reference from user fence
> > > + *
> > > + * Return: xe_user_fence pointer with reference
> > > + */
> > > +struct xe_user_fence *__xe_sync_ufence_get(struct xe_user_fence
> > > *ufence)
> > > +{
> > > + user_fence_get(ufence);
> > > +
> > > + return ufence;
> > > +}
> > > +
> > > /**
> > > * xe_sync_ufence_get() - Get user fence from sync
> > > * @sync: input sync
> > > diff --git a/drivers/gpu/drm/xe/xe_sync.h
> b/drivers/gpu/drm/xe/xe_sync.h
> > > index 3e03396af2c6..006dbf780793 100644
> > > --- a/drivers/gpu/drm/xe/xe_sync.h
> > > +++ b/drivers/gpu/drm/xe/xe_sync.h
> > > @@ -37,6 +37,7 @@ static inline bool xe_sync_is_ufence(struct
> > > xe_sync_entry *sync)
> > > return !!sync->ufence;
> > > }
> > >
> > > +struct xe_user_fence *__xe_sync_ufence_get(struct xe_user_fence
> > > *ufence);
> > > struct xe_user_fence *xe_sync_ufence_get(struct xe_sync_entry
> *sync);
> > > void xe_sync_ufence_put(struct xe_user_fence *ufence);
> > > int xe_sync_ufence_get_status(struct xe_user_fence *ufence);
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 0319e70577fe..1da68a03407b 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -1900,17 +1900,10 @@ xe_vm_bind(struct xe_vm *vm, struct
> xe_vma
> > > *vma, struct xe_exec_queue *q,
> > > {
> > > struct dma_fence *fence;
> > > struct xe_exec_queue *wait_exec_queue =
> > > to_wait_exec_queue(vm, q);
> > > - struct xe_user_fence *ufence;
> > >
> > > xe_vm_assert_held(vm);
> > > xe_bo_assert_held(bo);
> > >
> > > - ufence = find_ufence_get(syncs, num_syncs);
> > > - if (vma->ufence && ufence)
> > > - xe_sync_ufence_put(vma->ufence);
> > > -
> > > - vma->ufence = ufence ?: vma->ufence;
> > > -
> > > if (immediate) {
> > > fence = xe_vm_bind_vma(vma, q, syncs, num_syncs,
> > > tile_mask,
> > > first_op, last_op);
> > > @@ -2918,20 +2911,57 @@ static struct dma_fence *ops_execute(struct
> > > xe_vm *vm,
> > > return fence;
> > > }
> > >
> > > +static void vma_add_ufence(struct xe_vma *vma, struct xe_user_fence
> > > *ufence)
> > > +{
> > > + if (vma->ufence)
> > > + xe_sync_ufence_put(vma->ufence);
> > > + vma->ufence = __xe_sync_ufence_get(ufence);
> > > +}
> > > +
> > > +static void op_add_ufence(struct xe_vm *vm, struct xe_vma_op *op,
> > > + struct xe_user_fence *ufence)
> > > +{
> > > + switch (op->base.op) {
> > > + case DRM_GPUVA_OP_MAP:
> > > + vma_add_ufence(op->map.vma, ufence);
> > > + break;
> > > + case DRM_GPUVA_OP_REMAP:
> > > + if (op->remap.prev)
> > > + vma_add_ufence(op->remap.prev, ufence);
> > > + if (op->remap.next)
> > > + vma_add_ufence(op->remap.next, ufence);
> > > + break;
> > > + case DRM_GPUVA_OP_UNMAP:
> > > + break;
> > > + case DRM_GPUVA_OP_PREFETCH:
> > > + vma_add_ufence(gpuva_to_vma(op->base.prefetch.va),
> > > ufence);
> > > + break;
> > > + default:
> > > + drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> > > + }
> > > +}
> > > +
> > > static void vm_bind_ioctl_ops_fini(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_user_fence *ufence;
> > > struct xe_vma_op *op;
> > > int i;
> > >
> > > + ufence = find_ufence_get(vops->syncs, vops->num_syncs);
> > > list_for_each_entry(op, &vops->list, link) {
> > > + if (ufence)
> > > + op_add_ufence(vm, op, ufence);
> > > +
> > > 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);
> > > }
> > > + if (ufence)
> > > + xe_sync_ufence_put(ufence);
> > > for (i = 0; i < vops->num_syncs; i++)
> > > xe_sync_entry_signal(vops->syncs + i, fence);
> > > xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> > > --
> > > 2.34.1
> >
More information about the Intel-xe
mailing list