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

Thu Mar 28 21:37:40 UTC 2024

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.

>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.

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

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
>