[PATCH v4 02/30] drm/xe: Add ops_execute function which returns a fence
Zeng, Oak
oak.zeng at intel.com
Fri Mar 22 19:39:52 UTC 2024
> -----Original Message-----
> From: Brost, Matthew <matthew.brost at intel.com>
> Sent: Friday, March 22, 2024 1:31 PM
> To: Zeng, Oak <oak.zeng at intel.com>
> Cc: intel-xe at lists.freedesktop.org
> Subject: Re: [PATCH v4 02/30] drm/xe: Add ops_execute function which returns a
> fence
>
> On Fri, Mar 22, 2024 at 10:11:41AM -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 02/30] drm/xe: Add ops_execute function which returns a
> > > fence
> > >
> > > Add ops_execute function which returns a fence. This will be helpful to
> > > initiate all binds (VM bind IOCTL, rebinds in exec IOCTL, rebinds in
> > > preempt rebind worker, and rebinds in pagefaults) via a gpuva ops list.
> > > Returning a fence is needed in various paths.
> > >
> > > Signed-off-by: Matthew Brost <matthew.brost at intel.com>
> > > ---
> > > drivers/gpu/drm/xe/xe_vm.c | 211 +++++++++++++++++++------------------
> > > 1 file changed, 111 insertions(+), 100 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 3b5dc6de07f7..fb73afcab3b7 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -1789,16 +1789,17 @@ find_ufence_get(struct xe_sync_entry *syncs,
> u32
> > > num_syncs)
> > > return NULL;
> > > }
> > >
> > > -static int __xe_vm_bind(struct xe_vm *vm, struct xe_vma *vma,
> > > - struct xe_exec_queue *q, struct xe_sync_entry *syncs,
> > > - u32 num_syncs, bool immediate, bool first_op,
> > > - bool last_op)
> > > +static struct dma_fence *
> > > +xe_vm_bind(struct xe_vm *vm, struct xe_vma *vma, struct
> xe_exec_queue *q,
> > > + struct xe_bo *bo, struct xe_sync_entry *syncs, u32 num_syncs,
> > > + bool immediate, bool first_op, bool last_op)
> > > {
> > > 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)
> > > @@ -1810,7 +1811,7 @@ static int __xe_vm_bind(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > > fence = xe_vm_bind_vma(vma, q, syncs, num_syncs, first_op,
> > > last_op);
> > > if (IS_ERR(fence))
> > > - return PTR_ERR(fence);
> > > + return fence;
> > > } else {
> > > int i;
> > >
> > > @@ -1825,26 +1826,14 @@ static int __xe_vm_bind(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > >
> > > if (last_op)
> > > xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> > > - dma_fence_put(fence);
> > > -
> > > - return 0;
> > > -}
> > > -
> > > -static int xe_vm_bind(struct xe_vm *vm, struct xe_vma *vma, struct
> > > xe_exec_queue *q,
> > > - struct xe_bo *bo, struct xe_sync_entry *syncs,
> > > - u32 num_syncs, bool immediate, bool first_op,
> > > - bool last_op)
> > > -{
> > > - xe_vm_assert_held(vm);
> > > - xe_bo_assert_held(bo);
> > >
> > > - return __xe_vm_bind(vm, vma, q, syncs, num_syncs, immediate,
> > > first_op,
> > > - last_op);
> > > + return fence;
> > > }
> > >
> > > -static int xe_vm_unbind(struct xe_vm *vm, struct xe_vma *vma,
> > > - struct xe_exec_queue *q, struct xe_sync_entry *syncs,
> > > - u32 num_syncs, bool first_op, bool last_op)
> > > +static struct dma_fence *
> > > +xe_vm_unbind(struct xe_vm *vm, struct xe_vma *vma,
> > > + struct xe_exec_queue *q, struct xe_sync_entry *syncs,
> > > + 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);
> > > @@ -1854,14 +1843,13 @@ static int xe_vm_unbind(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > >
> > > fence = xe_vm_unbind_vma(vma, q, syncs, num_syncs, first_op,
> > > last_op);
> > > if (IS_ERR(fence))
> > > - return PTR_ERR(fence);
> > > + return fence;
> > >
> > > xe_vma_destroy(vma, fence);
> > > if (last_op)
> > > xe_exec_queue_last_fence_set(wait_exec_queue, vm, fence);
> > > - dma_fence_put(fence);
> > >
> > > - return 0;
> > > + return fence;
> > > }
> > >
> > > #define ALL_DRM_XE_VM_CREATE_FLAGS
> > > (DRM_XE_VM_CREATE_FLAG_SCRATCH_PAGE | \
> > > @@ -2004,10 +1992,11 @@ static const u32 region_to_mem_type[] = {
> > > XE_PL_VRAM1,
> > > };
> > >
> > > -static int 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)
> > > +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 *wait_exec_queue = to_wait_exec_queue(vm,
> > > q);
> > > int err;
> > > @@ -2017,27 +2006,24 @@ static int xe_vm_prefetch(struct xe_vm *vm,
> struct
> > > xe_vma *vma,
> > > if (!xe_vma_has_no_bo(vma)) {
> > > err = xe_bo_migrate(xe_vma_bo(vma),
> > > region_to_mem_type[region]);
> > > if (err)
> > > - return 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,
> > > 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++) {
> > > - struct dma_fence *fence =
> > > -
> > > xe_exec_queue_last_fence_get(wait_exec_queue, vm);
> > > -
> > > + for (i = 0; i < num_syncs; i++)
> > > xe_sync_entry_signal(&syncs[i], NULL, fence);
> > > - dma_fence_put(fence);
> > > - }
> > > }
> > >
> > > - return 0;
> > > + return fence;
> > > }
> > > }
> > >
> > > @@ -2484,10 +2470,10 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> > > *vm, struct xe_exec_queue *q,
> > > return 0;
> > > }
> > >
> > > -static int op_execute(struct xe_vm *vm, struct xe_vma *vma,
> > > - struct xe_vma_op *op)
> > > +static struct dma_fence *op_execute(struct xe_vm *vm, struct xe_vma
> *vma,
> > > + struct xe_vma_op *op)
> > > {
> > > - int err;
> > > + struct dma_fence *fence = NULL;
> > >
> > > lockdep_assert_held_write(&vm->lock);
> > > xe_vm_assert_held(vm);
> > > @@ -2495,11 +2481,11 @@ static int op_execute(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > >
> > > switch (op->base.op) {
> > > case DRM_GPUVA_OP_MAP:
> > > - err = xe_vm_bind(vm, vma, op->q, xe_vma_bo(vma),
> > > - op->syncs, op->num_syncs,
> > > - !xe_vm_in_fault_mode(vm),
> > > - op->flags & XE_VMA_OP_FIRST,
> > > - op->flags & XE_VMA_OP_LAST);
> > > + fence = xe_vm_bind(vm, vma, op->q, xe_vma_bo(vma),
> > > + op->syncs, op->num_syncs,
> > > + !xe_vm_in_fault_mode(vm),
> > > + op->flags & XE_VMA_OP_FIRST,
> > > + op->flags & XE_VMA_OP_LAST);
> > > break;
> > > case DRM_GPUVA_OP_REMAP:
> > > {
> > > @@ -2509,37 +2495,39 @@ static int op_execute(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > > if (!op->remap.unmap_done) {
> > > if (prev || next)
> > > vma->gpuva.flags |= XE_VMA_FIRST_REBIND;
> > > - err = xe_vm_unbind(vm, vma, op->q, op->syncs,
> > > - op->num_syncs,
> > > - op->flags & XE_VMA_OP_FIRST,
> > > - op->flags & XE_VMA_OP_LAST &&
> > > - !prev && !next);
> > > - if (err)
> > > + fence = xe_vm_unbind(vm, vma, op->q, op->syncs,
> > > + op->num_syncs,
> > > + op->flags & XE_VMA_OP_FIRST,
> > > + op->flags & XE_VMA_OP_LAST &&
> > > + !prev && !next);
> > > + if (IS_ERR(fence))
> > > break;
> > > op->remap.unmap_done = true;
> > > }
> > >
> > > if (prev) {
> > > op->remap.prev->gpuva.flags |= XE_VMA_LAST_REBIND;
> > > - err = xe_vm_bind(vm, op->remap.prev, op->q,
> > > - xe_vma_bo(op->remap.prev), op->syncs,
> > > - op->num_syncs, true, false,
> > > - op->flags & XE_VMA_OP_LAST
> > > && !next);
> > > + dma_fence_put(fence);
> >
> >
> > So you drop the previous fence. I assume in later operation, we will need to
> wait dma-fence for the previous operation to complete. Is it safe to only wait for
> the last fence? Shouldn't we wait all the fences, such as chain the fences in some
> way?
> >
>
> All fences on single xe_exec_queue are should ordered (e.g if the last
> completes all fences prior are also complete).
Ok, if all fences are ordered, this code make sense to me.
>
> Techincally this is broken on tip and at this point in the series too as
> we mix job fences and tlb invalidation fences on single xe_exec_queue.
> This series gets around to fix this once we convert an IOCTL into single
> bind job. So this patch while not perfect is regressing anything working
> toward 100% correctness.
>
> > > + fence = xe_vm_bind(vm, op->remap.prev, op->q,
> > > + xe_vma_bo(op->remap.prev), op-
> > > >syncs,
> > > + op->num_syncs, true, false,
> > > + op->flags & XE_VMA_OP_LAST
> > > && !next);
> > > op->remap.prev->gpuva.flags &=
> > > ~XE_VMA_LAST_REBIND;
> > > - if (err)
> > > + if (IS_ERR(fence))
> > > break;
> > > op->remap.prev = NULL;
> > > }
> > >
> > > if (next) {
> > > op->remap.next->gpuva.flags |= XE_VMA_LAST_REBIND;
> > > - err = xe_vm_bind(vm, op->remap.next, op->q,
> > > - xe_vma_bo(op->remap.next),
> > > - op->syncs, op->num_syncs,
> > > - true, false,
> > > - op->flags & XE_VMA_OP_LAST);
> > > + dma_fence_put(fence);
> >
> >
> > Same comment as above
>
> Same answer.
>
> > > + fence = xe_vm_bind(vm, op->remap.next, op->q,
> > > + xe_vma_bo(op->remap.next),
> > > + op->syncs, op->num_syncs,
> > > + true, false,
> > > + op->flags & XE_VMA_OP_LAST);
> > > op->remap.next->gpuva.flags &=
> > > ~XE_VMA_LAST_REBIND;
> > > - if (err)
> > > + if (IS_ERR(fence))
> > > break;
> > > op->remap.next = NULL;
> > > }
> > > @@ -2547,34 +2535,36 @@ static int op_execute(struct xe_vm *vm, struct
> > > xe_vma *vma,
> > > break;
> > > }
> > > case DRM_GPUVA_OP_UNMAP:
> > > - err = xe_vm_unbind(vm, vma, op->q, op->syncs,
> > > - op->num_syncs, op->flags &
> > > XE_VMA_OP_FIRST,
> > > - op->flags & XE_VMA_OP_LAST);
> > > + fence = xe_vm_unbind(vm, vma, op->q, op->syncs,
> > > + op->num_syncs, op->flags &
> > > XE_VMA_OP_FIRST,
> > > + op->flags & XE_VMA_OP_LAST);
> > > break;
> > > case DRM_GPUVA_OP_PREFETCH:
> > > - err = xe_vm_prefetch(vm, vma, op->q, op->prefetch.region,
> > > - op->syncs, op->num_syncs,
> > > - op->flags & XE_VMA_OP_FIRST,
> > > - op->flags & XE_VMA_OP_LAST);
> > > + fence = xe_vm_prefetch(vm, vma, op->q, op->prefetch.region,
> > > + op->syncs, op->num_syncs,
> > > + op->flags & XE_VMA_OP_FIRST,
> > > + op->flags & XE_VMA_OP_LAST);
> > > break;
> > > default:
> > > drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> > > }
> > >
> > > - if (err)
> > > + if (IS_ERR(fence))
> > > trace_xe_vma_fail(vma);
> > >
> > > - return err;
> > > + return fence;
> > > }
> > >
> > > -static int __xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> > > - struct xe_vma_op *op)
> > > +static struct dma_fence *
> > > +__xe_vma_op_execute(struct xe_vm *vm, struct xe_vma *vma,
> > > + struct xe_vma_op *op)
> > > {
> > > + struct dma_fence *fence;
> > > int err;
> > >
> > > retry_userptr:
> > > - err = op_execute(vm, vma, op);
> > > - if (err == -EAGAIN) {
> > > + fence = op_execute(vm, vma, op);
> > > + if (IS_ERR(fence) && PTR_ERR(fence) == -EAGAIN) {
> > > lockdep_assert_held_write(&vm->lock);
> > >
> > > if (op->base.op == DRM_GPUVA_OP_REMAP) {
> > > @@ -2591,22 +2581,24 @@ static int __xe_vma_op_execute(struct xe_vm
> *vm,
> > > struct xe_vma *vma,
> > > if (!err)
> > > goto retry_userptr;
> > >
> > > + fence = ERR_PTR(err);
> > > trace_xe_vma_fail(vma);
> > > }
> > > }
> > >
> > > - return err;
> > > + return fence;
> > > }
> > >
> > > -static int xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> > > +static struct dma_fence *
> > > +xe_vma_op_execute(struct xe_vm *vm, struct xe_vma_op *op)
> > > {
> > > - int ret = 0;
> > > + struct dma_fence *fence = ERR_PTR(-ENOMEM);
> > >
> > > lockdep_assert_held_write(&vm->lock);
> > >
> > > switch (op->base.op) {
> > > case DRM_GPUVA_OP_MAP:
> > > - ret = __xe_vma_op_execute(vm, op->map.vma, op);
> > > + fence = __xe_vma_op_execute(vm, op->map.vma, op);
> > > break;
> > > case DRM_GPUVA_OP_REMAP:
> > > {
> > > @@ -2619,23 +2611,23 @@ static int xe_vma_op_execute(struct xe_vm *vm,
> > > struct xe_vma_op *op)
> > > else
> > > vma = op->remap.next;
> > >
> > > - ret = __xe_vma_op_execute(vm, vma, op);
> > > + fence = __xe_vma_op_execute(vm, vma, op);
> > > break;
> > > }
> > > case DRM_GPUVA_OP_UNMAP:
> > > - ret = __xe_vma_op_execute(vm, gpuva_to_vma(op-
> > > >base.unmap.va),
> > > - op);
> > > + fence = __xe_vma_op_execute(vm, gpuva_to_vma(op-
> > > >base.unmap.va),
> > > + op);
> > > break;
> > > case DRM_GPUVA_OP_PREFETCH:
> > > - ret = __xe_vma_op_execute(vm,
> > > - gpuva_to_vma(op->base.prefetch.va),
> > > - op);
> > > + fence = __xe_vma_op_execute(vm,
> > > + gpuva_to_vma(op->base.prefetch.va),
> > > + op);
> > > break;
> > > default:
> > > drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> > > }
> > >
> > > - return ret;
> > > + return fence;
> > > }
> > >
> > > static void xe_vma_op_cleanup(struct xe_vm *vm, struct xe_vma_op *op)
> > > @@ -2803,11 +2795,35 @@ static int vm_bind_ioctl_ops_lock(struct
> drm_exec
> > > *exec,
> > > return 0;
> > > }
> > >
> > > +static struct dma_fence *ops_execute(struct xe_vm *vm,
> > > + struct list_head *ops_list,
> > > + bool cleanup)
> > > +{
> > > + struct xe_vma_op *op, *next;
> > > + struct dma_fence *fence = NULL;
> > > +
> > > + list_for_each_entry_safe(op, next, ops_list, link) {
> > > + if (!IS_ERR(fence)) {
> > > + dma_fence_put(fence);
> > > + fence = xe_vma_op_execute(vm, op);
> >
> > So you only return the fence of the last operation. In the later on codes, do you
> use fence to wait for *all* operations to finish?
> >
>
> Same answer as above, all fences on exec_queue should be ordered.
>
> > > + }
> > > + if (IS_ERR(fence)) {
> > > + drm_warn(&vm->xe->drm, "VM op(%d) failed with %ld",
> > > + op->base.op, PTR_ERR(fence));
> > > + fence = ERR_PTR(-ENOSPC);
> >
> > So even if there is error, you don't break the loop. Is it to perform the cleanup
> below?
> >
> > Once error happen for one operation, you seem to print the same error
> message for all the rest operations....because fence = xe_vma_op_execute(vm,
> op) is not called anymore after the first error
> >
>
> Yes.
Is this problematic though? Lets say you have 2 ops in the list and op_execute failed with op1. You will print as below:
VM op1 failed with xxx
VM op1 failed with xxx
Oak
>
> Matt
>
> >
> > Oak
> >
> > > + }
> > > + if (cleanup)
> > > + xe_vma_op_cleanup(vm, op);
> > > + }
> > > +
> > > + return fence;
> > > +}
> > > +
> > > static int vm_bind_ioctl_ops_execute(struct xe_vm *vm,
> > > struct list_head *ops_list)
> > > {
> > > struct drm_exec exec;
> > > - struct xe_vma_op *op, *next;
> > > + struct dma_fence *fence;
> > > int err;
> > >
> > > lockdep_assert_held_write(&vm->lock);
> > > @@ -2820,19 +2836,14 @@ static int vm_bind_ioctl_ops_execute(struct
> xe_vm
> > > *vm,
> > > if (err)
> > > goto unlock;
> > >
> > > - list_for_each_entry_safe(op, next, ops_list, link) {
> > > - err = xe_vma_op_execute(vm, op);
> > > - if (err) {
> > > - drm_warn(&vm->xe->drm, "VM op(%d) failed
> > > with %d",
> > > - op->base.op, err);
> > > - /*
> > > - * FIXME: Killing VM rather than proper error
> > > handling
> > > - */
> > > - xe_vm_kill(vm, false);
> > > - err = -ENOSPC;
> > > - goto unlock;
> > > - }
> > > - xe_vma_op_cleanup(vm, op);
> > > + fence = ops_execute(vm, ops_list, true);
> > > + if (IS_ERR(fence)) {
> > > + err = PTR_ERR(fence);
> > > + /* FIXME: Killing VM rather than proper error handling */
> > > + xe_vm_kill(vm, false);
> > > + goto unlock;
> > > + } else {
> > > + dma_fence_put(fence);
> > > }
> > > }
> > >
> > > --
> > > 2.34.1
> >
More information about the Intel-xe
mailing list