[PATCH v4 10/30] drm/xe: Add vm_bind_ioctl_ops_install_fences helper
Matthew Brost
matthew.brost at intel.com
Mon Mar 25 19:34:58 UTC 2024
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?
'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
>
More information about the Intel-xe
mailing list