RE: [PATCH v4 15/30] drm/xe: Add xe_vm_pgtable_update_op to xe_vma_ops

["Zeng, Oak" <oak.zeng@intel.com>]

> -----Original Message-----
> From: Brost, Matthew <matthew.brost@intel.com>
> Sent: Tuesday, March 26, 2024 3:06 PM
> To: Zeng, Oak <oak.zeng@intel.com>
> Cc: intel-xe@lists.freedesktop.org
> Subject: Re: [PATCH v4 15/30] drm/xe: Add xe_vm_pgtable_update_op to
> xe_vma_ops
> 
> On Mon, Mar 25, 2024 at 03:58:10PM -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 15/30] drm/xe: Add xe_vm_pgtable_update_op to
> > > xe_vma_ops
> > >
> > > Will help with the converstion to 1 job per VM bind IOCTL. Allocation
> > > only implemented in this patch.
> >
> >
> > Can you explain why you need to introduce pt_update_op? this patch seems
> only allocate some slot for pt_update_op but they are not really used for page
> table update. I will keep looking the rest of this series.
> >
> > I also don't get why this helps 1job/bind ioctl.
> >
> 
> Each VMA operations is converted into 0-3 PT per ops tile. i.e. a MAP VMA
> operations with immediate clear generates 0 PT ops, a REMAP VMA with
> both prev & next generates 3 PT ops. PT ops work on the internal / GPU
> PT state compared to VMA ops which operate on the internal GPUVM state.
> 
> The flow roughly is:
> 
> generate VMA ops from IOCTL input
> convert all VMA ops to PT ops, prep and stage PT ops
> run PT ops as an atomic unit (1 job)
> commit PT ops
> commit VMA ops
> 
> If at any point a failure occurs before the commit step the PT ops can
> be unwound.
> 
> Likewise when a failure occurs the VMA ops are unwound too, this is just
> layer above the PT ops.
> 
> In the end if an errors at point in the IOCTL the original GPUVM and PT
> state is restored. This compared to serial execution on tip where is an
> error occurs we just kill the VM.
> 
> Does this make sense?

Hi Matt,

Thanks for explain it. Should we also document such implementation details in the xe_vm_doc.h?

Oak

> 
> Matt
> 
> >
> > Oak
> >
> > >
> > > Signed-off-by: Matthew Brost <matthew.brost@intel.com>
> > > ---
> > >  drivers/gpu/drm/xe/xe_pt_types.h | 12 ++++++
> > >  drivers/gpu/drm/xe/xe_vm.c       | 66 +++++++++++++++++++++++++++++--
> -
> > >  drivers/gpu/drm/xe/xe_vm_types.h |  8 ++++
> > >  3 files changed, 81 insertions(+), 5 deletions(-)
> > >
> > > diff --git a/drivers/gpu/drm/xe/xe_pt_types.h
> > > b/drivers/gpu/drm/xe/xe_pt_types.h
> > > index cee70cb0f014..2093150f461e 100644
> > > --- a/drivers/gpu/drm/xe/xe_pt_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_pt_types.h
> > > @@ -74,4 +74,16 @@ struct xe_vm_pgtable_update {
> > >  	u32 flags;
> > >  };
> > >
> > > +/** struct xe_vm_pgtable_update_op - Page table update operation */
> > > +struct xe_vm_pgtable_update_op {
> > > +	/** @entries: entries to update for this operation */
> > > +	struct xe_vm_pgtable_update entries[XE_VM_MAX_LEVEL * 2 + 1];
> > > +	/** @num_entries: number of entries for this update operation */
> > > +	u32 num_entries;
> > > +	/** @bind: is a bind */
> > > +	bool bind;
> > > +	/** @rebind: is a rebind */
> > > +	bool rebind;
> > > +};
> > > +
> > >  #endif
> > > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > > index 5b93c71fc5e9..72e9bdab79d5 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm.c
> > > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > > @@ -1319,6 +1319,42 @@ static void xe_vma_ops_init(struct xe_vma_ops
> *vops,
> > > struct xe_vm *vm,
> > >  	vops->num_syncs = num_syncs;
> > >  }
> > >
> > > +static int xe_vma_ops_alloc(struct xe_vma_ops *vops)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < XE_MAX_TILES_PER_DEVICE; ++i) {
> > > +		if (!vops->pt_update_ops[i].num_ops)
> > > +			continue;
> > > +
> > > +		vops->pt_update_ops[i].ops =
> > > +			kmalloc_array(vops->pt_update_ops[i].num_ops,
> > > +				      sizeof(*vops->pt_update_ops[i].ops),
> > > +				      GFP_KERNEL);
> > > +		if (!vops->pt_update_ops[i].ops)
> > > +			return -ENOMEM;
> > > +	}
> > > +
> > > +	return 0;
> > > +}
> > > +
> > > +static void xe_vma_ops_fini(struct xe_vma_ops *vops)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < XE_MAX_TILES_PER_DEVICE; ++i)
> > > +		kfree(vops->pt_update_ops[i].ops);
> > > +}
> > > +
> > > +static void xe_vma_ops_incr_pt_update_ops(struct xe_vma_ops *vops, u8
> > > tile_mask)
> > > +{
> > > +	int i;
> > > +
> > > +	for (i = 0; i < XE_MAX_TILES_PER_DEVICE; ++i)
> > > +		if (BIT(i) & tile_mask)
> > > +			++vops->pt_update_ops[i].num_ops;
> > > +}
> > > +
> > >  struct xe_vm *xe_vm_create(struct xe_device *xe, u32 flags)
> > >  {
> > >  	struct drm_gem_object *vm_resv_obj;
> > > @@ -1343,6 +1379,11 @@ struct xe_vm *xe_vm_create(struct xe_device *xe,
> > > u32 flags)
> > >  	xe_vma_ops_init(&vm->dummy_ops.vops, vm, NULL, NULL, 0);
> > >  	INIT_LIST_HEAD(&vm->dummy_ops.op.link);
> > >  	list_add(&vm->dummy_ops.op.link, &vm->dummy_ops.vops.list);
> > > +	for (id = 0; id < XE_MAX_TILES_PER_DEVICE; ++id)
> > > +		vm->dummy_ops.vops.pt_update_ops[id].num_ops = 1;
> > > +	err = xe_vma_ops_alloc(&vm->dummy_ops.vops);
> > > +	if (err)
> > > +		goto err_free;
> > >
> > >  	INIT_LIST_HEAD(&vm->rebind_list);
> > >
> > > @@ -1468,12 +1509,14 @@ struct xe_vm *xe_vm_create(struct xe_device
> *xe,
> > > u32 flags)
> > >  	return ERR_PTR(err);
> > >
> > >  err_no_resv:
> > > -	mutex_destroy(&vm->snap_mutex);
> > > +	if (!(flags & XE_VM_FLAG_MIGRATION))
> > > +		xe_device_mem_access_put(xe);
> > >  	for_each_tile(tile, xe, id)
> > >  		xe_range_fence_tree_fini(&vm->rftree[id]);
> > > +err_free:
> > > +	mutex_destroy(&vm->snap_mutex);
> > > +	xe_vma_ops_fini(&vm->dummy_ops.vops);
> > >  	kfree(vm);
> > > -	if (!(flags & XE_VM_FLAG_MIGRATION))
> > > -		xe_device_mem_access_put(xe);
> > >  	return ERR_PTR(err);
> > >  }
> > >
> > > @@ -1611,6 +1654,7 @@ static void vm_destroy_work_func(struct
> work_struct
> > > *w)
> > >
> > >  	trace_xe_vm_free(vm);
> > >  	dma_fence_put(vm->rebind_fence);
> > > +	xe_vma_ops_fini(&vm->dummy_ops.vops);
> > >  	kfree(vm);
> > >  }
> > >
> > > @@ -2284,7 +2328,6 @@ static int xe_vma_op_commit(struct xe_vm *vm,
> struct
> > > xe_vma_op *op)
> > >  	return err;
> > >  }
> > >
> > > -
> > >  static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct
> xe_exec_queue *q,
> > >  				   struct drm_gpuva_ops *ops,
> > >  				   struct xe_sync_entry *syncs, u32 num_syncs,
> > > @@ -2334,6 +2377,9 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> *vm,
> > > struct xe_exec_queue *q,
> > >  				return PTR_ERR(vma);
> > >
> > >  			op->map.vma = vma;
> > > +			if (op->map.immediate || !xe_vm_in_fault_mode(vm))
> > > +				xe_vma_ops_incr_pt_update_ops(vops,
> > > +							      op->tile_mask);
> > >  			break;
> > >  		}
> > >  		case DRM_GPUVA_OP_REMAP:
> > > @@ -2378,6 +2424,8 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> *vm,
> > > struct xe_exec_queue *q,
> > >  					vm_dbg(&xe->drm, "REMAP:SKIP_PREV:
> > > addr=0x%016llx, range=0x%016llx",
> > >  					       (ULL)op->remap.start,
> > >  					       (ULL)op->remap.range);
> > > +				} else {
> > > +					xe_vma_ops_incr_pt_update_ops(vops,
> > > op->tile_mask);
> > >  				}
> > >  			}
> > >
> > > @@ -2414,13 +2462,16 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm
> > > *vm, struct xe_exec_queue *q,
> > >  					vm_dbg(&xe->drm, "REMAP:SKIP_NEXT:
> > > addr=0x%016llx, range=0x%016llx",
> > >  					       (ULL)op->remap.start,
> > >  					       (ULL)op->remap.range);
> > > +				} else {
> > > +					xe_vma_ops_incr_pt_update_ops(vops,
> > > op->tile_mask);
> > >  				}
> > >  			}
> > > +			xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> > >  			break;
> > >  		}
> > >  		case DRM_GPUVA_OP_UNMAP:
> > >  		case DRM_GPUVA_OP_PREFETCH:
> > > -			/* Nothing to do */
> > > +			xe_vma_ops_incr_pt_update_ops(vops, op->tile_mask);
> > >  			break;
> > >  		default:
> > >  			drm_warn(&vm->xe->drm, "NOT POSSIBLE");
> > > @@ -3243,11 +3294,16 @@ int xe_vm_bind_ioctl(struct drm_device *dev,
> void
> > > *data, struct drm_file *file)
> > >  		goto unwind_ops;
> > >  	}
> > >
> > > +	err = xe_vma_ops_alloc(&vops);
> > > +	if (err)
> > > +		goto unwind_ops;
> > > +
> > >  	err = vm_bind_ioctl_ops_execute(vm, &vops);
> > >
> > >  unwind_ops:
> > >  	if (err && err != -ENODATA)
> > >  		vm_bind_ioctl_ops_unwind(vm, ops, args->num_binds);
> > > +	xe_vma_ops_fini(&vops);
> > >  	for (i = args->num_binds - 1; i >= 0; --i)
> > >  		if (ops[i])
> > >  			drm_gpuva_ops_free(&vm->gpuvm, ops[i]);
> > > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h
> > > b/drivers/gpu/drm/xe/xe_vm_types.h
> > > index f6ea4df64e79..f5d740dcbba3 100644
> > > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > > @@ -22,6 +22,7 @@ struct xe_device;
> > >  struct xe_sync_entry;
> > >  struct xe_user_fence;
> > >  struct xe_vm;
> > > +struct xe_vm_pgtable_update_op;
> > >
> > >  #define XE_VMA_READ_ONLY	DRM_GPUVA_USERBITS
> > >  #define XE_VMA_DESTROYED	(DRM_GPUVA_USERBITS << 1)
> > > @@ -219,6 +220,13 @@ struct xe_vma_ops {
> > >  	struct xe_sync_entry *syncs;
> > >  	/** @num_syncs: number of syncs */
> > >  	u32 num_syncs;
> > > +	/** @pt_update_ops: page table update operations */
> > > +	struct {
> > > +		/** @ops: operations */
> > > +		struct xe_vm_pgtable_update_op *ops;
> > > +		/** @num_ops: number of operations */
> > > +		u32 num_ops;
> > > +	} pt_update_ops[XE_MAX_TILES_PER_DEVICE];
> > >  };
> > >
> > >  struct xe_vm {
> > > --
> > > 2.34.1
> >