[PATCH v2] drm/xe/uapi: Restore flags VM_BIND_FLAG_READONLY and VM_BIND_FLAG_IMMEDIATE

Fri Mar 29 11:19:45 UTC 2024

On Thu, Mar 28, 2024 at 04:37:40PM -0500, Lucas De Marchi wrote:
> On Thu, Mar 28, 2024 at 08:03:32PM +0000, Francois Dugast wrote:
> > The commit 84a1ed5e6756 ("drm/xe/uapi: Remove unused flags") is partially
> > reverted. At the time, flags not used by user space were removed during
> > some cleanup. There is now a need for flags DRM_XE_VM_BIND_FLAG_READONLY
> > and DRM_XE_VM_BIND_FLAG_IMMEDIATE by the compute runtime, so they are
> > brought back.
> 
> Michal/Mateusz, do you want to expand a little more here on the intended
> use for the flags?  Let us know when the userspace side is
> reviewed/ready so we can proceed with the kernel side.
> 
> > 
> > v2: Include a link to the PR in the commit message (Matthew Brost)
> > 
> > Cc: Mateusz Jablonski <mateusz.jablonski at intel.com>
> > Cc: Michal Mrozek <michal.mrozek at intel.com>
> > Cc: Matthew Brost <matthew.brost at intel.com>
> > Link: https://github.com/intel/compute-runtime/pull/717
> 
> from a quick scan on the code it seems there were already users, but the
> ioctl wrapper was not setting the flag appropriately so it was flagged by the earlier
> cleanup as "nobody is using it". Right? It seems that it was
> even already the expected behavior judging from
> 
> 	bool IoctlHelperXe::isImmediateVmBindRequired() const {
> 	    return true;
> 	}
> 
> Similar reasoning for the read-only... apparently it was already being
> used, but the interface was incomplete.

Correct, the method above was not plugged to XeKMD uAPI.

> 
> > Signed-off-by: Francois Dugast <francois.dugast at intel.com>
> > Reviewed-by: Matthew Brost <matthew.brost at intel.com>
> > ---
> > drivers/gpu/drm/xe/xe_vm.c       | 13 +++++++++++--
> > drivers/gpu/drm/xe/xe_vm_types.h |  4 ++++
> > include/uapi/drm/xe_drm.h        |  6 ++++++
> > 3 files changed, 21 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/xe/xe_vm.c b/drivers/gpu/drm/xe/xe_vm.c
> > index 8b32aa5003df..6e56b9a3985b 100644
> > --- a/drivers/gpu/drm/xe/xe_vm.c
> > +++ b/drivers/gpu/drm/xe/xe_vm.c
> > @@ -2212,6 +2212,10 @@ vm_bind_ioctl_ops_create(struct xe_vm *vm, struct xe_bo *bo,
> > 		struct xe_vma_op *op = gpuva_op_to_vma_op(__op);
> > 
> > 		if (__op->op == DRM_GPUVA_OP_MAP) {
> > +			op->map.immediate =
> > +				flags & DRM_XE_VM_BIND_FLAG_IMMEDIATE;
> > +			op->map.read_only =
> > +				flags & DRM_XE_VM_BIND_FLAG_READONLY;
> > 			op->map.is_null = flags & DRM_XE_VM_BIND_FLAG_NULL;
> > 			op->map.dumpable = flags & DRM_XE_VM_BIND_FLAG_DUMPABLE;
> > 			op->map.pat_index = pat_index;
> > @@ -2406,6 +2410,8 @@ static int vm_bind_ioctl_ops_parse(struct xe_vm *vm, struct xe_exec_queue *q,
> > 		switch (op->base.op) {
> > 		case DRM_GPUVA_OP_MAP:
> > 		{
> > +			flags |= op->map.read_only ?
> > +				VMA_CREATE_FLAG_READ_ONLY : 0;
> > 			flags |= op->map.is_null ?
> > 				VMA_CREATE_FLAG_IS_NULL : 0;
> > 			flags |= op->map.dumpable ?
> > @@ -2550,7 +2556,7 @@ static int op_execute(struct drm_exec *exec, struct xe_vm *vm,
> > 	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->map.immediate || !xe_vm_in_fault_mode(vm),
> 
> this doesn't match the doc below, but I *think* it's just a doc issue?
> 
> > 				 op->flags & XE_VMA_OP_FIRST,
> > 				 op->flags & XE_VMA_OP_LAST);
> > 		break;
> > @@ -2825,7 +2831,10 @@ static int vm_bind_ioctl_ops_execute(struct xe_vm *vm,
> > 	return 0;
> > }
> > 
> > -#define SUPPORTED_FLAGS	(DRM_XE_VM_BIND_FLAG_NULL | \
> > +#define SUPPORTED_FLAGS	\
> > +	(DRM_XE_VM_BIND_FLAG_READONLY | \
> > +	 DRM_XE_VM_BIND_FLAG_IMMEDIATE | \
> > +	 DRM_XE_VM_BIND_FLAG_NULL | \
> > 	 DRM_XE_VM_BIND_FLAG_DUMPABLE)
> > #define XE_64K_PAGE_MASK 0xffffull
> > #define ALL_DRM_XE_SYNCS_FLAGS (DRM_XE_SYNCS_FLAG_WAIT_FOR_OP)
> > diff --git a/drivers/gpu/drm/xe/xe_vm_types.h b/drivers/gpu/drm/xe/xe_vm_types.h
> > index badf3945083d..0447c79c40a2 100644
> > --- a/drivers/gpu/drm/xe/xe_vm_types.h
> > +++ b/drivers/gpu/drm/xe/xe_vm_types.h
> > @@ -276,6 +276,10 @@ struct xe_vm {
> > struct xe_vma_op_map {
> > 	/** @vma: VMA to map */
> > 	struct xe_vma *vma;
> > +	/** @immediate: Immediate bind */
> > +	bool immediate;
> > +	/** @read_only: Read only */
> > +	bool read_only;
> > 	/** @is_null: is NULL binding */
> > 	bool is_null;
> > 	/** @dumpable: whether BO is dumped on GPU hang */
> > diff --git a/include/uapi/drm/xe_drm.h b/include/uapi/drm/xe_drm.h
> > index 808ad1c308ec..94e1f3cfe705 100644
> > --- a/include/uapi/drm/xe_drm.h
> > +++ b/include/uapi/drm/xe_drm.h
> > @@ -871,6 +871,10 @@ struct drm_xe_vm_destroy {
> >  *  - %DRM_XE_VM_BIND_OP_PREFETCH
> >  *
> >  * and the @flags can be:
> > + *  - %DRM_XE_VM_BIND_FLAG_READONLY
> > + *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - Valid on a faulting VM only, do the
> 
> so we pass immediate == true to xe_vm_bind() if it's either
> op->map.immediate (set by this flag) or if !xe_vm_in_fault_mode(vm).
> Which means the flag is actually implied if it's not in fault mode, not
> that it's invalid. If it was invalid, I'd actually see us blocking the
> ioctl. I'd reword it as:
> 
>  *  - %DRM_XE_VM_BIND_FLAG_IMMEDIATE - On a faulting VM, do the
>  *    MAP operation immediately rather than deferring the MAP to the page
>  *    fault handler. This is implied on a non-faulting VM as there is no
>  *    fault handler to defer to.

The is the doc from before the removal but it was indeed incorrect, thank
you for catching this, I will update it.

> 
> also, even if DRM_XE_VM_BIND_FLAG_READONLY may be kind of obvious, let's
> add a sentence with documentation.

Will do.

Francois

> 
> thanks
> Lucas De Marchi
> 
> > + *    MAP operation immediately rather than deferring the MAP to the page
> > + *    fault handler.
> >  *  - %DRM_XE_VM_BIND_FLAG_NULL - When the NULL flag is set, the page
> >  *    tables are setup with a special bit which indicates writes are
> >  *    dropped and all reads return zero. In the future, the NULL flags
> > @@ -963,6 +967,8 @@ struct drm_xe_vm_bind_op {
> > 	/** @op: Bind operation to perform */
> > 	__u32 op;
> > 
> > +#define DRM_XE_VM_BIND_FLAG_READONLY	(1 << 0)
> > +#define DRM_XE_VM_BIND_FLAG_IMMEDIATE	(1 << 1)
> > #define DRM_XE_VM_BIND_FLAG_NULL	(1 << 2)
> > #define DRM_XE_VM_BIND_FLAG_DUMPABLE	(1 << 3)
> > 	/** @flags: Bind flags */
> > -- 
> > 2.34.1
> > 