[PATCH RFC 0/4] svm support

[PATCH RFC 0/4] svm support

[Mika Kuoppala]

Hi,

Now when fences got merged I reworked the series. It is now much
smaller in size. Some items are still missing like error state recording,
fault handling, documentation and in fences.

You can also find the most recent version in here:
https://cgit.freedesktop.org/~miku/drm-intel/log/?h=svm

-Mika

Jesse Barnes (4):
  drm/i915: add create_context2 ioctl
  drm/i915: IOMMU based SVM implementation v13
  drm/i915: add SVM execbuf ioctl v10
  drm/i915: Add param for SVM

 drivers/gpu/drm/i915/Kconfig               |   1 +
 drivers/gpu/drm/i915/i915_drv.c            |   5 +
 drivers/gpu/drm/i915/i915_drv.h            |  37 +++++++
 drivers/gpu/drm/i915/i915_gem.c            |   7 ++
 drivers/gpu/drm/i915/i915_gem_context.c    | 172 +++++++++++++++++++++++++----
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 157 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_reg.h            |  18 +++
 drivers/gpu/drm/i915/intel_lrc.c           |  39 +++----
 include/uapi/drm/i915_drm.h                |  55 +++++++++
 9 files changed, 446 insertions(+), 45 deletions(-)

-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Mika Kuoppala]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

Add i915_gem_context_create2_ioctl for passing flags
(e.g. SVM) when creating a context.

v2: check the pad on create_context
v3: rebase
v4: i915_dma is no more. create_gvt needs flags

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c         |  1 +
 drivers/gpu/drm/i915/i915_drv.h         |  2 +
 drivers/gpu/drm/i915/i915_gem_context.c | 70 +++++++++++++++++++++++++++------
 include/uapi/drm/i915_drm.h             | 18 +++++++++
 4 files changed, 78 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 13ae340ef1f3..9fb6de90eac0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2566,6 +2566,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
+	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE2, i915_gem_context_create2_ioctl, DRM_UNLOCKED),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 35caa9b2f36a..598e078418e3 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3399,6 +3399,8 @@ static inline bool i915_gem_context_is_default(const struct i915_gem_context *c)
 
 int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
 				  struct drm_file *file);
+int i915_gem_context_create2_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file);
 int i915_gem_context_destroy_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file);
 int i915_gem_context_getparam_ioctl(struct drm_device *dev, void *data,
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 35950ee46a1d..189a6c018b72 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -341,17 +341,21 @@ err_out:
  */
 static struct i915_gem_context *
 i915_gem_create_context(struct drm_device *dev,
-			struct drm_i915_file_private *file_priv)
+			struct drm_i915_file_private *file_priv, u32 flags)
 {
 	struct i915_gem_context *ctx;
+	bool create_vm = false;
 
 	lockdep_assert_held(&dev->struct_mutex);
 
+	if (flags & (I915_GEM_CONTEXT_FULL_PPGTT | I915_GEM_CONTEXT_ENABLE_SVM))
+		create_vm = true;
+
 	ctx = __create_hw_context(dev, file_priv);
 	if (IS_ERR(ctx))
 		return ctx;
 
-	if (USES_FULL_PPGTT(dev)) {
+	if (create_vm) {
 		struct i915_hw_ppgtt *ppgtt =
 			i915_ppgtt_create(to_i915(dev), file_priv);
 
@@ -394,7 +398,8 @@ i915_gem_context_create_gvt(struct drm_device *dev)
 	if (ret)
 		return ERR_PTR(ret);
 
-	ctx = i915_gem_create_context(dev, NULL);
+	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev) ?
+				      I915_GEM_CONTEXT_FULL_PPGTT : 0);
 	if (IS_ERR(ctx))
 		goto out;
 
@@ -440,6 +445,7 @@ int i915_gem_context_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_gem_context *ctx;
+	u32 flags = 0;
 
 	/* Init should only be called once per module load. Eventually the
 	 * restriction on the context_disabled check can be loosened. */
@@ -472,7 +478,10 @@ int i915_gem_context_init(struct drm_device *dev)
 		}
 	}
 
-	ctx = i915_gem_create_context(dev, NULL);
+	if (USES_FULL_PPGTT(dev))
+		flags |= I915_GEM_CONTEXT_FULL_PPGTT;
+
+	ctx = i915_gem_create_context(dev, NULL, flags);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context (error %ld)\n",
 			  PTR_ERR(ctx));
@@ -552,7 +561,8 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
 	idr_init(&file_priv->context_idr);
 
 	mutex_lock(&dev->struct_mutex);
-	ctx = i915_gem_create_context(dev, file_priv);
+	ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev) ?
+				      I915_GEM_CONTEXT_FULL_PPGTT : 0);
 	mutex_unlock(&dev->struct_mutex);
 
 	if (IS_ERR(ctx)) {
@@ -974,32 +984,66 @@ static bool contexts_enabled(struct drm_device *dev)
 	return i915.enable_execlists || to_i915(dev)->hw_context_size;
 }
 
-int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
-				  struct drm_file *file)
+int i915_gem_context_create2_ioctl(struct drm_device *dev, void *data,
+				   struct drm_file *file)
 {
-	struct drm_i915_gem_context_create *args = data;
+	struct drm_i915_gem_context_create2 *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_gem_context *ctx;
+	u32 flags = args->flags;
 	int ret;
 
 	if (!contexts_enabled(dev))
 		return -ENODEV;
 
-	if (args->pad != 0)
+	if (flags & ~(I915_GEM_CONTEXT_FULL_PPGTT |
+		      I915_GEM_CONTEXT_ENABLE_SVM))
 		return -EINVAL;
 
 	ret = i915_mutex_lock_interruptible(dev);
 	if (ret)
 		return ret;
 
-	ctx = i915_gem_create_context(dev, file_priv);
-	mutex_unlock(&dev->struct_mutex);
-	if (IS_ERR(ctx))
-		return PTR_ERR(ctx);
+	if (USES_FULL_PPGTT(dev))
+		flags |= I915_GEM_CONTEXT_FULL_PPGTT;
+
+	if (flags & I915_GEM_CONTEXT_ENABLE_SVM) {
+		ret = -ENODEV;
+		goto unlock;
+	}
+
+	ctx = i915_gem_create_context(dev, file_priv, flags);
+	if (IS_ERR(ctx)) {
+		ret = PTR_ERR(ctx);
+		goto unlock;
+	}
 
 	args->ctx_id = ctx->user_handle;
 	DRM_DEBUG_DRIVER("HW context %d created\n", args->ctx_id);
 
+unlock:
+	mutex_unlock(&dev->struct_mutex);
+	return ret;
+}
+
+int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
+				  struct drm_file *file)
+{
+	struct drm_i915_gem_context_create *args = data;
+	struct drm_i915_gem_context_create2 tmp;
+	int ret;
+
+	if (args->pad != 0)
+		return -EINVAL;
+
+	tmp.flags = 0;
+
+	ret = i915_gem_context_create2_ioctl(dev, &tmp, file);
+	if (ret)
+		return ret;
+
+	args->ctx_id = tmp.ctx_id;
+
 	return 0;
 }
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 452629de7a57..639ef5b0e2c9 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -258,6 +258,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_USERPTR		0x33
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
+#define DRM_I915_GEM_CONTEXT_CREATE2	0x36
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -311,6 +312,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_USERPTR			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_USERPTR, struct drm_i915_gem_userptr)
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
+#define DRM_IOCTL_I915_GEM_CONTEXT_CREATE2	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE2, struct drm_i915_gem_context_create2)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1142,6 +1144,22 @@ struct drm_i915_gem_context_create {
 	__u32 pad;
 };
 
+/*
+ * SVM handling
+ *
+ * A context can opt in to SVM support (thereby using its CPU page tables
+ * when accessing data from the GPU) by using the %I915_ENABLE_SVM flag
+ * and passing an existing context id.  This is a one way transition; SVM
+ * contexts can not be downgraded into PPGTT contexts once converted.
+ */
+#define I915_GEM_CONTEXT_ENABLE_SVM		(1<<0)
+#define I915_GEM_CONTEXT_FULL_PPGTT		(1<<1)
+struct drm_i915_gem_context_create2 {
+	/*  output: id of new context*/
+	__u32 ctx_id;
+	__u32 flags;
+};
+
 struct drm_i915_gem_context_destroy {
 	__u32 ctx_id;
 	__u32 pad;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[Mika Kuoppala]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

Use David's new IOMMU layer functions for supporting SVM in i915.

TODO:
  error record collection for failing SVM contexts
  callback handling for fatal faults
  scheduling

v2: integrate David's core IOMMU support
    make sure we don't clobber the PASID in the context reg state
v3: fixup for intel-svm.h changes (David)
v4: use fault & halt for now (Jesse)
    fix ring free in error path on context alloc (Julia)
v5: update with new callback struct (Jesse)
v6: fix init svm check per new IOMMU code (Jesse)
v7: drop debug code and obsolete i915_svm.c file (Jesse)
v8: fix !CONFIG_INTEL_IOMMU_SVM init stub (Jesse)
v9: update to new execlist and reg handling bits (Jesse)
    context teardown fix (lrc deferred alloc vs teardown race?) (Jesse)
    check for SVM availability at context create (Jesse)
v10: intel_context_svm_init/fini & rebase
v11: move context specific stuff to i915_gem_context
v12: move addressing to context descriptor
v13: strip out workqueue and mm notifiers

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: David Woodhouse <dwmw2@infradead.org>
Signed-off-by: David Woodhouse <David.Woodhouse@intel.com> (v3)
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v9)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h         |  32 ++++++++++
 drivers/gpu/drm/i915/i915_gem.c         |   7 +++
 drivers/gpu/drm/i915/i915_gem_context.c | 104 +++++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/i915_reg.h         |  18 ++++++
 drivers/gpu/drm/i915/intel_lrc.c        |  39 +++++-------
 5 files changed, 167 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 598e078418e3..64f3f0f18509 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -39,6 +39,7 @@
 #include <linux/backlight.h>
 #include <linux/hashtable.h>
 #include <linux/intel-iommu.h>
+#include <linux/intel-svm.h>
 #include <linux/kref.h>
 #include <linux/pm_qos.h>
 #include <linux/shmem_fs.h>
@@ -866,6 +867,8 @@ struct i915_ctx_hang_stats {
  * @remap_slice: l3 row remapping information.
  * @flags: context specific flags:
  *         CONTEXT_NO_ZEROMAP: do not allow mapping things to page 0.
+ *         CONTEXT_NO_ERROR_CAPTURE: do not capture gpu state on hang.
+ *         CONTEXT_SVM: context with 1:1 gpu vs cpu mapping of vm.
  * @file_priv: filp associated with this context (NULL for global default
  *	       context).
  * @hang_stats: information about the role of this context in possible GPU
@@ -891,6 +894,8 @@ struct i915_gem_context {
 	unsigned long flags;
 #define CONTEXT_NO_ZEROMAP		BIT(0)
 #define CONTEXT_NO_ERROR_CAPTURE	BIT(1)
+#define CONTEXT_SVM			BIT(2)
+
 	unsigned hw_id;
 	u32 user_handle;
 
@@ -909,6 +914,9 @@ struct i915_gem_context {
 	struct atomic_notifier_head status_notifier;
 	bool execlists_force_single_submission;
 
+	u32 pasid; /* svm, 20 bits */
+	struct task_struct *task;
+
 	struct list_head link;
 
 	u8 remap_slice;
@@ -2001,6 +2009,8 @@ struct drm_i915_private {
 
 	struct i915_runtime_pm pm;
 
+	bool svm_available;
+
 	/* Abstract the submission mechanism (legacy ringbuffer or execlists) away */
 	struct {
 		void (*cleanup_engine)(struct intel_engine_cs *engine);
@@ -3628,6 +3638,28 @@ extern void intel_set_memory_cxsr(struct drm_i915_private *dev_priv,
 int i915_reg_read_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file);
 
+/* svm */
+#ifdef CONFIG_INTEL_IOMMU_SVM
+static inline bool intel_init_svm(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	dev_priv->svm_available = USES_FULL_48BIT_PPGTT(dev_priv) &&
+		intel_svm_available(&dev->pdev->dev);
+
+	return dev_priv->svm_available;
+}
+#else
+static inline bool intel_init_svm(struct drm_device *dev)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+
+	dev_priv->svm_available = false;
+
+	return dev_priv->svm_available;
+}
+#endif
+
 /* overlay */
 extern struct intel_overlay_error_state *
 intel_overlay_capture_error_state(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7e08c774a1aa..45d67b54c018 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4304,6 +4304,13 @@ i915_gem_init_hw(struct drm_device *dev)
 		}
 	}
 
+	if (INTEL_GEN(dev) >= 8) {
+		if (intel_init_svm(dev))
+			DRM_DEBUG_DRIVER("Initialized Intel SVM support\n");
+		else
+			DRM_ERROR("Failed to enable Intel SVM support\n");
+	}
+
 	i915_gem_init_swizzling(dev);
 
 	/*
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 189a6c018b72..9ab6332f296b 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -134,6 +134,47 @@ static int get_context_size(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+static void i915_svm_fault_cb(struct device *dev, int pasid, u64 addr,
+			      u32 private, int rwxp, int response)
+{
+}
+
+static struct svm_dev_ops i915_svm_ops = {
+	.fault_cb = i915_svm_fault_cb,
+};
+
+static int i915_gem_context_svm_init(struct i915_gem_context *ctx)
+{
+	struct device *dev = &ctx->i915->drm.pdev->dev;
+	int ret;
+
+	if (WARN_ON_ONCE(!ctx->i915->svm_available))
+		return -ENODEV;
+
+	get_task_struct(current);
+
+	ret = intel_svm_bind_mm(dev, &ctx->pasid, 0, &i915_svm_ops);
+	if (ret) {
+		DRM_DEBUG_DRIVER("pasid alloc fail: %d\n", ret);
+		put_task_struct(current);
+		return ret;
+	}
+
+	ctx->task = current;
+
+	return 0;
+}
+
+static void i915_gem_context_svm_fini(struct i915_gem_context *ctx)
+{
+	struct device *dev = &ctx->i915->drm.pdev->dev;
+
+	if (ctx->task) {
+		intel_svm_unbind_mm(dev, ctx->pasid);
+		put_task_struct(ctx->task);
+	}
+}
+
 void i915_gem_context_free(struct kref *ctx_ref)
 {
 	struct i915_gem_context *ctx = container_of(ctx_ref, typeof(*ctx), ref);
@@ -143,6 +184,9 @@ void i915_gem_context_free(struct kref *ctx_ref)
 	trace_i915_context_free(ctx);
 	GEM_BUG_ON(!ctx->closed);
 
+	if (ctx->flags & CONTEXT_SVM)
+		i915_gem_context_svm_fini(ctx);
+
 	i915_ppgtt_put(ctx->ppgtt);
 
 	for (i = 0; i < I915_NUM_ENGINES; i++) {
@@ -257,9 +301,34 @@ static int assign_hw_id(struct drm_i915_private *dev_priv, unsigned *out)
 	return 0;
 }
 
+static u32 __create_ctx_desc(struct drm_i915_private *dev_priv, u32 flags)
+{
+	u32 desc = GEN8_CTX_VALID;
+
+	if (flags & I915_GEM_CONTEXT_ENABLE_SVM) {
+		desc |= INTEL_ADVANCED_CONTEXT <<
+			GEN8_CTX_ADDRESSING_MODE_SHIFT;
+		/*
+		 * Switch to stream once we have a scheduler and can
+		 * re-submit contexts.
+		 */
+		desc |= FAULT_AND_HALT << GEN8_CTX_FAULT_SHIFT;
+	} else {
+		if (IS_GEN8(dev_priv))
+			desc |= GEN8_CTX_L3LLC_COHERENT;
+
+		desc |= GEN8_CTX_PRIVILEGE;
+
+		desc |= GEN8_CTX_ADDRESSING_MODE(dev_priv) <<
+			GEN8_CTX_ADDRESSING_MODE_SHIFT;
+	}
+
+	return desc;
+}
+
 static struct i915_gem_context *
 __create_hw_context(struct drm_device *dev,
-		    struct drm_i915_file_private *file_priv)
+		    struct drm_i915_file_private *file_priv, u32 flags)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct i915_gem_context *ctx;
@@ -323,8 +392,8 @@ __create_hw_context(struct drm_device *dev,
 
 	ctx->hang_stats.ban_period_seconds = DRM_I915_CTX_BAN_PERIOD;
 	ctx->ring_size = 4 * PAGE_SIZE;
-	ctx->desc_template = GEN8_CTX_ADDRESSING_MODE(dev_priv) <<
-			     GEN8_CTX_ADDRESSING_MODE_SHIFT;
+	ctx->desc_template = __create_ctx_desc(dev_priv, flags);
+
 	ATOMIC_INIT_NOTIFIER_HEAD(&ctx->status_notifier);
 
 	return ctx;
@@ -345,13 +414,14 @@ i915_gem_create_context(struct drm_device *dev,
 {
 	struct i915_gem_context *ctx;
 	bool create_vm = false;
+	int ret;
 
 	lockdep_assert_held(&dev->struct_mutex);
 
 	if (flags & (I915_GEM_CONTEXT_FULL_PPGTT | I915_GEM_CONTEXT_ENABLE_SVM))
 		create_vm = true;
 
-	ctx = __create_hw_context(dev, file_priv);
+	ctx = __create_hw_context(dev, file_priv, flags);
 	if (IS_ERR(ctx))
 		return ctx;
 
@@ -360,19 +430,31 @@ i915_gem_create_context(struct drm_device *dev,
 			i915_ppgtt_create(to_i915(dev), file_priv);
 
 		if (IS_ERR(ppgtt)) {
-			DRM_DEBUG_DRIVER("PPGTT setup failed (%ld)\n",
-					 PTR_ERR(ppgtt));
-			idr_remove(&file_priv->context_idr, ctx->user_handle);
-			context_close(ctx);
-			return ERR_CAST(ppgtt);
+			ret = PTR_ERR(ppgtt);
+			DRM_DEBUG_DRIVER("PPGTT setup failed (%d)\n", ret);
+			goto free_ctx;
 		}
 
 		ctx->ppgtt = ppgtt;
 	}
 
+	if (flags & I915_GEM_CONTEXT_ENABLE_SVM) {
+		ret = i915_gem_context_svm_init(ctx);
+		if (ret)
+			goto free_ctx;
+
+		ctx->flags |= CONTEXT_SVM;
+	}
+
 	trace_i915_context_create(ctx);
 
 	return ctx;
+
+free_ctx:
+	idr_remove(&file_priv->context_idr, ctx->user_handle);
+	context_close(ctx);
+
+	return ERR_PTR(ret);
 }
 
 /**
@@ -987,6 +1069,7 @@ static bool contexts_enabled(struct drm_device *dev)
 int i915_gem_context_create2_ioctl(struct drm_device *dev, void *data,
 				   struct drm_file *file)
 {
+	struct drm_i915_private *dev_priv = to_i915(dev);
 	struct drm_i915_gem_context_create2 *args = data;
 	struct drm_i915_file_private *file_priv = file->driver_priv;
 	struct i915_gem_context *ctx;
@@ -1007,7 +1090,8 @@ int i915_gem_context_create2_ioctl(struct drm_device *dev, void *data,
 	if (USES_FULL_PPGTT(dev))
 		flags |= I915_GEM_CONTEXT_FULL_PPGTT;
 
-	if (flags & I915_GEM_CONTEXT_ENABLE_SVM) {
+	if ((flags & I915_GEM_CONTEXT_ENABLE_SVM) &&
+	    !dev_priv->svm_available) {
 		ret = -ENODEV;
 		goto unlock;
 	}
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index d4adf2806c50..8eebb038622b 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -3062,6 +3062,24 @@ enum {
 	INTEL_LEGACY_64B_CONTEXT
 };
 
+enum {
+	FAULT_AND_HANG = 0,
+	FAULT_AND_HALT, /* Debug only */
+	FAULT_AND_STREAM,
+	FAULT_AND_CONTINUE /* Unsupported */
+};
+
+#define GEN8_CTX_VALID (1<<0)
+#define GEN8_CTX_FORCE_PD_RESTORE (1<<1)
+#define GEN8_CTX_FORCE_RESTORE (1<<2)
+#define GEN8_CTX_L3LLC_COHERENT (1<<5)
+#define GEN8_CTX_PRIVILEGE (1<<8)
+
+#define GEN8_CTX_FAULT_SHIFT 6
+#define GEN8_CTX_ID_SHIFT 32
+#define GEN8_CTX_ID_WIDTH 21
+#define GEN8_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x17
+#define GEN9_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x26
 #define GEN8_CTX_ADDRESSING_MODE_SHIFT 3
 #define GEN8_CTX_ADDRESSING_MODE(dev_priv) (USES_FULL_48BIT_PPGTT(dev_priv) ?\
 				INTEL_LEGACY_64B_CONTEXT : \
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6b49df4316f4..6e27cc83aa43 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -185,12 +185,6 @@
 #define CTX_R_PWR_CLK_STATE		0x42
 #define CTX_GPGPU_CSR_BASE_ADDRESS	0x44
 
-#define GEN8_CTX_VALID (1<<0)
-#define GEN8_CTX_FORCE_PD_RESTORE (1<<1)
-#define GEN8_CTX_FORCE_RESTORE (1<<2)
-#define GEN8_CTX_L3LLC_COHERENT (1<<5)
-#define GEN8_CTX_PRIVILEGE (1<<8)
-
 #define ASSIGN_CTX_REG(reg_state, pos, reg, val) do { \
 	(reg_state)[(pos)+0] = i915_mmio_reg_offset(reg); \
 	(reg_state)[(pos)+1] = (val); \
@@ -207,16 +201,13 @@
 	reg_state[CTX_PDP0_LDW + 1] = lower_32_bits(px_dma(&ppgtt->pml4)); \
 } while (0)
 
-enum {
-	FAULT_AND_HANG = 0,
-	FAULT_AND_HALT, /* Debug only */
-	FAULT_AND_STREAM,
-	FAULT_AND_CONTINUE /* Unsupported */
-};
-#define GEN8_CTX_ID_SHIFT 32
-#define GEN8_CTX_ID_WIDTH 21
-#define GEN8_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x17
-#define GEN9_CTX_RCS_INDIRECT_CTX_OFFSET_DEFAULT	0x26
+#define ASSIGN_CTX_SVM(reg_state, ctx, engine) do { \
+		ASSIGN_CTX_REG(reg_state, CTX_PDP0_UDW, \
+			       GEN8_RING_PDP_UDW((engine), 0), 0); \
+		ASSIGN_CTX_REG(reg_state, CTX_PDP0_LDW, \
+			       GEN8_RING_PDP_LDW((engine), 0), (ctx)->pasid); \
+} while (0)
+
 
 /* Typical size of the average request (2 pipecontrols and a MI_BB) */
 #define EXECLISTS_REQUEST_SIZE 64 /* bytes */
@@ -270,10 +261,6 @@ logical_ring_init_platform_invariants(struct intel_engine_cs *engine)
 					IS_BXT_REVID(dev_priv, 0, BXT_REVID_A1)) &&
 					(engine->id == VCS || engine->id == VCS2);
 
-	engine->ctx_desc_template = GEN8_CTX_VALID;
-	if (IS_GEN8(dev_priv))
-		engine->ctx_desc_template |= GEN8_CTX_L3LLC_COHERENT;
-	engine->ctx_desc_template |= GEN8_CTX_PRIVILEGE;
 
 	/* TODO: WaDisableLiteRestore when we start using semaphore
 	 * signalling between Command Streamers */
@@ -380,7 +367,9 @@ static void execlists_update_context(struct drm_i915_gem_request *rq)
 	 * PML4 is allocated during ppgtt init, so this is not needed
 	 * in 48-bit mode.
 	 */
-	if (ppgtt && !USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
+	if (!(rq->ctx->flags & CONTEXT_SVM) &&
+	    ppgtt &&
+	    !USES_FULL_48BIT_PPGTT(ppgtt->base.dev))
 		execlists_update_context_pdps(ppgtt, reg_state);
 }
 
@@ -1399,7 +1388,9 @@ static int gen8_emit_bb_start(struct drm_i915_gem_request *req,
 	 * it is unsafe in case of lite-restore (because the ctx is
 	 * not idle). PML4 is allocated during ppgtt init so this is
 	 * not needed in 48-bit.*/
-	if (req->ctx->ppgtt &&
+
+	if (!(req->ctx->flags & CONTEXT_SVM) &&
+	    req->ctx->ppgtt &&
 	    (intel_engine_flag(req->engine) & req->ctx->ppgtt->pd_dirty_rings)) {
 		if (!USES_FULL_48BIT_PPGTT(req->i915) &&
 		    !intel_vgpu_active(req->i915)) {
@@ -2057,7 +2048,9 @@ populate_lr_context(struct i915_gem_context *ctx,
 	ASSIGN_CTX_REG(reg_state, CTX_PDP0_LDW, GEN8_RING_PDP_LDW(engine, 0),
 		       0);
 
-	if (USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
+	if (ctx->flags & CONTEXT_SVM) {
+		ASSIGN_CTX_SVM(reg_state, ctx, engine);
+	} else if (USES_FULL_48BIT_PPGTT(ppgtt->base.dev)) {
 		/* 64b PPGTT (48bit canonical)
 		 * PDP0_DESCRIPTOR contains the base address to PML4 and
 		 * other PDP Descriptors are ignored.
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Mika Kuoppala]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

We just need to pass in an address to execute and some flags, since we
don't have to worry about buffer relocation or any of the other usual
stuff.  Returns a fence to be used for synchronization.

v2: add a request after batch submission (Jesse)
v3: add a flag for fence creation (Chris)
v4: add CLOEXEC flag (Kristian)
    add non-RCS ring support (Jesse)
v5: update for request alloc change (Jesse)
v6: new sync file interface, error paths, request breadcrumbs
v7: always CLOEXEC for sync_file_install
v8: rebase on new sync file api
v9: rework on top of fence requests and sync_file
v10: take fence ref for sync_file (Chris)
     use correct flush (Chris)
     limit exec on rcs

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v5)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/Kconfig               |   1 +
 drivers/gpu/drm/i915/i915_drv.c            |   1 +
 drivers/gpu/drm/i915/i915_drv.h            |   3 +
 drivers/gpu/drm/i915/i915_gem_execbuffer.c | 157 +++++++++++++++++++++++++++++
 include/uapi/drm/i915_drm.h                |  36 +++++++
 5 files changed, 198 insertions(+)

diff --git a/drivers/gpu/drm/i915/Kconfig b/drivers/gpu/drm/i915/Kconfig
index 7769e469118f..6503133c3f85 100644
--- a/drivers/gpu/drm/i915/Kconfig
+++ b/drivers/gpu/drm/i915/Kconfig
@@ -8,6 +8,7 @@ config DRM_I915
 	# the shmem_readpage() which depends upon tmpfs
 	select SHMEM
 	select TMPFS
+	select SYNC_FILE
 	select DRM_KMS_HELPER
 	select DRM_PANEL
 	select DRM_MIPI_DSI
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 9fb6de90eac0..a07918d821e4 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -2567,6 +2567,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
 	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE2, i915_gem_context_create2_ioctl, DRM_UNLOCKED),
+	DRM_IOCTL_DEF_DRV(I915_EXEC_MM, intel_exec_mm_ioctl, DRM_UNLOCKED),
 };
 
 static struct drm_driver driver = {
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 64f3f0f18509..884d9844863c 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3660,6 +3660,9 @@ static inline bool intel_init_svm(struct drm_device *dev)
 }
 #endif
 
+extern int intel_exec_mm_ioctl(struct drm_device *dev, void *data,
+			       struct drm_file *file);
+
 /* overlay */
 extern struct intel_overlay_error_state *
 intel_overlay_capture_error_state(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem_execbuffer.c b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
index 699315304748..c1ba6da1fd33 100644
--- a/drivers/gpu/drm/i915/i915_gem_execbuffer.c
+++ b/drivers/gpu/drm/i915/i915_gem_execbuffer.c
@@ -29,6 +29,7 @@
 #include <linux/dma_remapping.h>
 #include <linux/reservation.h>
 #include <linux/uaccess.h>
+#include <linux/sync_file.h>
 
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
@@ -1911,3 +1912,159 @@ i915_gem_execbuffer2(struct drm_device *dev, void *data,
 	drm_free_large(exec2_list);
 	return ret;
 }
+
+static struct intel_engine_cs *
+exec_mm_select_engine(struct drm_i915_private *dev_priv,
+		      struct drm_i915_exec_mm *exec_mm)
+{
+	unsigned int user_ring_id = exec_mm->ring_id & I915_EXEC_RING_MASK;
+	struct intel_engine_cs *e;
+
+	if (user_ring_id > I915_USER_RINGS) {
+		DRM_DEBUG("exec_mm with unknown ring: %u\n", user_ring_id);
+		return NULL;
+	}
+
+	e = &dev_priv->engine[user_ring_map[user_ring_id]];
+
+	if (!intel_engine_initialized(e)) {
+		DRM_DEBUG("exec_mm with invalid ring: %u\n", user_ring_id);
+		return NULL;
+	}
+
+	return e;
+}
+
+static int do_exec_mm(struct drm_i915_exec_mm *exec_mm,
+		      struct drm_i915_gem_request *req,
+		      const u32 flags)
+{
+	const bool create_fence = flags & I915_EXEC_MM_FENCE;
+	struct sync_file *out_fence;
+	int ret;
+
+	if (create_fence) {
+		out_fence = sync_file_create(fence_get(&req->fence));
+		if (!out_fence) {
+			DRM_DEBUG("sync file creation failed\n");
+			return ret;
+		}
+
+		exec_mm->fence = get_unused_fd_flags(O_CLOEXEC);
+		fd_install(exec_mm->fence, out_fence->file);
+	}
+
+	ret = req->engine->emit_flush(req, EMIT_INVALIDATE);
+	if (ret) {
+		DRM_DEBUG_DRIVER("engine flush failed: %d\n", ret);
+		goto fput;
+	}
+
+	ret = req->engine->emit_bb_start(req, exec_mm->batch_ptr, 0, 0);
+	if (ret) {
+		DRM_DEBUG_DRIVER("engine dispatch execbuf failed: %d\n", ret);
+		goto fput;
+	}
+
+	return 0;
+fput:
+	if (create_fence) {
+		fput(out_fence->file);
+		fence_put(&req->fence);
+	}
+
+	return ret;
+}
+
+int intel_exec_mm_ioctl(struct drm_device *dev, void *data,
+			struct drm_file *file)
+{
+	struct drm_i915_private *dev_priv = to_i915(dev);
+	struct drm_i915_exec_mm *exec_mm = data;
+	struct intel_engine_cs *engine;
+	struct i915_gem_context *ctx;
+	struct drm_i915_gem_request *req;
+	const u32 ctx_id = exec_mm->ctx_id;
+	const u32 flags = exec_mm->flags;
+	int ret;
+
+	if (exec_mm->batch_ptr & 3) {
+		DRM_DEBUG_DRIVER("ptr not 4-byt aligned %llu\n",
+				 exec_mm->batch_ptr);
+		return -EINVAL;
+	}
+
+	if (flags & ~(I915_EXEC_MM_FENCE)) {
+		DRM_DEBUG_DRIVER("bad flags: 0x%08x\n", flags);
+		return -EINVAL;
+	}
+
+	if (!dev_priv->svm_available) {
+		DRM_DEBUG_DRIVER("no svm available\n");
+		return -ENODEV;
+	}
+
+	if (file == NULL)
+		return -EINVAL;
+
+	engine = exec_mm_select_engine(dev_priv, exec_mm);
+	if (!engine)
+		return -EINVAL;
+
+	if (engine->id != RCS) {
+		DRM_DEBUG_DRIVER("svm exec only allowed on RCS engine\n");
+		return -EINVAL;
+	}
+
+	intel_runtime_pm_get(dev_priv);
+
+	ret = i915_mutex_lock_interruptible(dev);
+	if (ret) {
+		DRM_ERROR("mutex interrupted\n");
+		goto pm_put;
+	}
+
+	ctx = i915_gem_validate_context(dev, file, engine, ctx_id);
+	if (IS_ERR(ctx)) {
+		ret = PTR_ERR(ctx);
+		DRM_DEBUG_DRIVER("couldn't get context\n");
+		goto unlock;
+	}
+
+	if (!(ctx->flags & CONTEXT_SVM)) {
+		ret = -EINVAL;
+		DRM_DEBUG_DRIVER("context is not SVM enabled\n");
+		goto unlock;
+	}
+
+	i915_gem_context_get(ctx);
+
+	req = i915_gem_request_alloc(engine, ctx);
+	if (IS_ERR(req)) {
+		ret = PTR_ERR(req);
+		goto ctx_put;
+	}
+
+	ret = i915_gem_request_add_to_client(req, file);
+	if (ret) {
+		DRM_DEBUG_DRIVER("failed to add request to client, %d\n", ret);
+		goto add_request;
+	}
+
+	/* If we fail here, we still need to submit the req */
+	ret = do_exec_mm(exec_mm, req, flags);
+
+add_request:
+	i915_add_request(req);
+
+ctx_put:
+	i915_gem_context_put(ctx);
+
+unlock:
+	mutex_unlock(&dev->struct_mutex);
+
+pm_put:
+	intel_runtime_pm_put(dev_priv);
+
+	return ret;
+}
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 639ef5b0e2c9..8d567744f221 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -259,6 +259,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_I915_GEM_CONTEXT_GETPARAM	0x34
 #define DRM_I915_GEM_CONTEXT_SETPARAM	0x35
 #define DRM_I915_GEM_CONTEXT_CREATE2	0x36
+#define DRM_I915_EXEC_MM		0x37
 
 #define DRM_IOCTL_I915_INIT		DRM_IOW( DRM_COMMAND_BASE + DRM_I915_INIT, drm_i915_init_t)
 #define DRM_IOCTL_I915_FLUSH		DRM_IO ( DRM_COMMAND_BASE + DRM_I915_FLUSH)
@@ -313,6 +314,7 @@ typedef struct _drm_i915_sarea {
 #define DRM_IOCTL_I915_GEM_CONTEXT_GETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_GETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_SETPARAM	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_SETPARAM, struct drm_i915_gem_context_param)
 #define DRM_IOCTL_I915_GEM_CONTEXT_CREATE2	DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_GEM_CONTEXT_CREATE2, struct drm_i915_gem_context_create2)
+#define DRM_IOCTL_I915_EXEC_MM			DRM_IOWR (DRM_COMMAND_BASE + DRM_I915_EXEC_MM, struct drm_i915_exec_mm)
 
 /* Allow drivers to submit batchbuffers directly to hardware, relying
  * on the security mechanisms provided by hardware.
@@ -1225,6 +1227,40 @@ struct drm_i915_gem_context_param {
 	__u64 value;
 };
 
+/**
+ * drm_i915_exec_mm - shared address space execbuf
+ * @batch_ptr: address of batch buffer (in context's CPU address space)
+ * @ctx_id: context to use for execution
+ * @ring_id: ring to which this context will be submitted
+ * @flags: see flags
+ * @fence: returned fence handle
+ * @fence_dep_count: number of fences this execution depends on
+ * @fence_deps: array of fence IDs (u32) this execution depends on
+ *
+ * This simplified execbuf just executes an MI_BATCH_BUFFER_START at
+ * @batch_ptr using @ctx_id as the context.  The context will indicate
+ * which address space the @batch_ptr will use.
+ *
+ * Note @batch_ptr must be dword aligned.
+ *
+ * By default, the kernel will simply execute the address given on the GPU.
+ * If the %I915_EXEC_MM_FENCE flag is passed in the @flags field however,
+ * the kernel will return a Android native sync object for the caller to
+ * use to synchronize execution (see the android-native-sync(7) man page).
+ *
+ */
+struct drm_i915_exec_mm {
+	__u64 batch_ptr;
+	__u32 ctx_id;
+	__u32 ring_id; /* see execbuffer2 flags */
+	__u32 flags;
+#define I915_EXEC_MM_FENCE (1<<0)
+	__u32 pad;
+	__u32 fence;
+	__u32 fence_dep_count;
+	__u64 fence_deps;
+};
+
 #if defined(__cplusplus)
 }
 #endif
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH RFC 4/4] drm/i915: Add param for SVM

[Mika Kuoppala]

From: Jesse Barnes <jbarnes@virtuousgeek.org>

Add possibility to query if svm is available.

v2: moved into i915_drv.c

Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c | 3 +++
 include/uapi/drm/i915_drm.h     | 1 +
 2 files changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a07918d821e4..6d9c84253412 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -354,6 +354,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 	case I915_PARAM_MIN_EU_IN_POOL:
 		value = INTEL_INFO(dev)->min_eu_in_pool;
 		break;
+	case I915_PARAM_HAS_SVM:
+		value = dev_priv->svm_available;
+		break;
 	default:
 		DRM_DEBUG("Unknown parameter %d\n", param->param);
 		return -EINVAL;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 8d567744f221..c21ba4b769c4 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -391,6 +391,7 @@ typedef struct drm_i915_irq_wait {
 #define I915_PARAM_HAS_EXEC_SOFTPIN	 37
 #define I915_PARAM_HAS_POOLED_EU	 38
 #define I915_PARAM_MIN_EU_IN_POOL	 39
+#define I915_PARAM_HAS_SVM		 40
 
 typedef struct drm_i915_getparam {
 	__s32 param;
-- 
2.7.4

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Chris Wilson]

On Mon, Aug 15, 2016 at 02:48:04PM +0300, Mika Kuoppala wrote:
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Add i915_gem_context_create2_ioctl for passing flags
> (e.g. SVM) when creating a context.
> 
> v2: check the pad on create_context
> v3: rebase
> v4: i915_dma is no more. create_gvt needs flags
> 
> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>

Considering we can use deferred ppgtt creation and have setparam do we
need a new create ioctl just to set a flag?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Joonas Lahtinen]

On ma, 2016-08-15 at 14:48 +0300, Mika Kuoppala wrote:
> @@ -2566,6 +2566,7 @@ static const struct drm_ioctl_desc i915_ioctls[] = {
>  	DRM_IOCTL_DEF_DRV(I915_GEM_USERPTR, i915_gem_userptr_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_GETPARAM, i915_gem_context_getparam_ioctl, DRM_RENDER_ALLOW),
>  	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_SETPARAM, i915_gem_context_setparam_ioctl, DRM_RENDER_ALLOW),
> +	DRM_IOCTL_DEF_DRV(I915_GEM_CONTEXT_CREATE2, i915_gem_context_create2_ioctl, DRM_UNLOCKED),

Why DRM_UNLOCKED?

> @@ -394,7 +398,8 @@ i915_gem_context_create_gvt(struct drm_device *dev)
>  	if (ret)
>  		return ERR_PTR(ret);
>  
> -	ctx = i915_gem_create_context(dev, NULL);
> +	ctx = i915_gem_create_context(dev, NULL, USES_FULL_PPGTT(dev) ?
> +				      I915_GEM_CONTEXT_FULL_PPGTT : 0);

Could use flags variable here just like below this point in code.

> @@ -552,7 +561,8 @@ int i915_gem_context_open(struct drm_device *dev, struct drm_file *file)
>  	idr_init(&file_priv->context_idr);
>  
>  	mutex_lock(&dev->struct_mutex);
> -	ctx = i915_gem_create_context(dev, file_priv);
> +	ctx = i915_gem_create_context(dev, file_priv, USES_FULL_PPGTT(dev) ?
> +				      I915_GEM_CONTEXT_FULL_PPGTT : 0);

Ditto.

> +int i915_gem_context_create_ioctl(struct drm_device *dev, void *data,
> +				  struct drm_file *file)
> +{
> +	struct drm_i915_gem_context_create *args = data;
> +	struct drm_i915_gem_context_create2 tmp;

'args2' just as we have create2?

> @@ -1142,6 +1144,22 @@ struct drm_i915_gem_context_create {
>  	__u32 pad;
>  };
>  
> +/*
> + * SVM handling
> + *
> + * A context can opt in to SVM support (thereby using its CPU page tables
> + * when accessing data from the GPU) by using the %I915_ENABLE_SVM flag

s/I915_ENABLE_SVM/I915_GEM_CONTEXT_ENABLE_SVM/ ?

> + * and passing an existing context id.  This is a one way transition; SVM
> + * contexts can not be downgraded into PPGTT contexts once converted.
> + */
> +#define I915_GEM_CONTEXT_ENABLE_SVM		(1<<0)
> +#define I915_GEM_CONTEXT_FULL_PPGTT		(1<<1)

BIT()

With the above addressed;

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[Chris Wilson]

On Mon, Aug 15, 2016 at 02:48:05PM +0300, Mika Kuoppala wrote:
> @@ -891,6 +894,8 @@ struct i915_gem_context {
>  	unsigned long flags;
>  #define CONTEXT_NO_ZEROMAP		BIT(0)
>  #define CONTEXT_NO_ERROR_CAPTURE	BIT(1)
> +#define CONTEXT_SVM			BIT(2)
> +
>  	unsigned hw_id;
>  	u32 user_handle;
>  
> @@ -909,6 +914,9 @@ struct i915_gem_context {
>  	struct atomic_notifier_head status_notifier;
>  	bool execlists_force_single_submission;
>  
> +	u32 pasid; /* svm, 20 bits */

Doesn't this conflict with hw_id for execlists.

> +	struct task_struct *task;

We don't need the task, we need the mm.

Holding the task is not sufficient.

>  	struct list_head link;
>  
>  	u8 remap_slice;
> @@ -2001,6 +2009,8 @@ struct drm_i915_private {
>  
>  	struct i915_runtime_pm pm;
>  
> +	bool svm_available;

No better home / community?

> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 7e08c774a1aa..45d67b54c018 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -4304,6 +4304,13 @@ i915_gem_init_hw(struct drm_device *dev)
>  		}
>  	}
>  
> +	if (INTEL_GEN(dev) >= 8) {
> +		if (intel_init_svm(dev))

init_hw ?

This looks more like one off early driver init.

> +			DRM_DEBUG_DRIVER("Initialized Intel SVM support\n");
> +		else
> +			DRM_ERROR("Failed to enable Intel SVM support\n");
> +	}
> +

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 560 bytes --]

On Mon, 2016-08-15 at 14:48 +0300, Mika Kuoppala wrote:
> 
>  
> +static void i915_svm_fault_cb(struct device *dev, int pasid, u64 addr,
> +                             u32 private, int rwxp, int response)
> +{
> +}
> +
> +static struct svm_dev_ops i915_svm_ops = {
> +       .fault_cb = i915_svm_fault_cb,
> +};
> +

I'd prefer that you don't hook this up unless you need it.

I'd also prefer that you don't need it.

If you need it, nail a hardware designer to a tree before you hook it
up.

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Chris Wilson]

On Mon, Aug 15, 2016 at 02:48:06PM +0300, Mika Kuoppala wrote:
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> We just need to pass in an address to execute and some flags, since we
> don't have to worry about buffer relocation or any of the other usual
> stuff.  Returns a fence to be used for synchronization.
> 
> v2: add a request after batch submission (Jesse)
> v3: add a flag for fence creation (Chris)
> v4: add CLOEXEC flag (Kristian)
>     add non-RCS ring support (Jesse)
> v5: update for request alloc change (Jesse)
> v6: new sync file interface, error paths, request breadcrumbs
> v7: always CLOEXEC for sync_file_install
> v8: rebase on new sync file api
> v9: rework on top of fence requests and sync_file
> v10: take fence ref for sync_file (Chris)
>      use correct flush (Chris)
>      limit exec on rcs

This is incomplete, so just proof of principle?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 4/4] drm/i915: Add param for SVM

[Chris Wilson]

On Mon, Aug 15, 2016 at 02:48:07PM +0300, Mika Kuoppala wrote:
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> Add possibility to query if svm is available.

When we try to enable SVM on the context we get an error. We have to do
that first anywhere, that seems like a good spot for userspace to catch
all issues.

What usecase do you have that doesn't involve creating an SVM context?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1102 bytes --]

On Mon, 2016-08-15 at 13:05 +0100, Chris Wilson wrote:
> On Mon, Aug 15, 2016 at 02:48:05PM +0300, Mika Kuoppala wrote:
> > 
> > +	struct task_struct *task;
> 
> We don't need the task, we need the mm.
> 
> Holding the task is not sufficient.

From the pure DMA point of view, you don't need the MM at all. I handle
all that from the IOMMU side so it's none of your business, darling.

However, if you want to relate a given context to the specific thread
which started it, perhaps to deliver signals or whatever else, then
perhaps you do want the task not the MM.

> 
> > --- a/drivers/gpu/drm/i915/i915_gem.c
> > +++ b/drivers/gpu/drm/i915/i915_gem.c
> > @@ -4304,6 +4304,13 @@ i915_gem_init_hw(struct drm_device *dev)
> >  		}
> >  	}
> >  
> > +	if (INTEL_GEN(dev) >= 8) {
> > +		if (intel_init_svm(dev))
> 
> init_hw ?
> 
> This looks more like one off early driver init.

It's a per-device thing. You might support SVM on one device but not
another, depending on how the IOMMU is configured. Note the 'dev'
argument in the call to intel_init_svm().


-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 4/4] drm/i915: Add param for SVM

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Aug 15, 2016 at 02:48:07PM +0300, Mika Kuoppala wrote:
>> From: Jesse Barnes <jbarnes@virtuousgeek.org>
>> 
>> Add possibility to query if svm is available.
>
> When we try to enable SVM on the context we get an error. We have to do
> that first anywhere, that seems like a good spot for userspace to catch
> all issues.
>
> What usecase do you have that doesn't involve creating an SVM context?

I can't think of any. So this patch stinks superfluous.

-Mika


> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[Chris Wilson]

On Mon, Aug 15, 2016 at 01:13:25PM +0100, David Woodhouse wrote:
> On Mon, 2016-08-15 at 13:05 +0100, Chris Wilson wrote:
> > On Mon, Aug 15, 2016 at 02:48:05PM +0300, Mika Kuoppala wrote:
> > > 
> > > +	struct task_struct *task;
> > 
> > We don't need the task, we need the mm.
> > 
> > Holding the task is not sufficient.
> 
> From the pure DMA point of view, you don't need the MM at all. I handle
> all that from the IOMMU side so it's none of your business, darling.

But you don't keep the mm alive for the duration of device activity,
right? And you don't wait for the device to finish before releasing the
mmu? (iiuc intel-svm.c)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Ro.CI.BAT: failure for svm support

[Patchwork]

== Series Details ==

Series: svm support
URL   : https://patchwork.freedesktop.org/series/11097/
State : failure

== Summary ==

Series 11097v1 svm support
http://patchwork.freedesktop.org/api/1.0/series/11097/revisions/1/mbox

Test drv_hangman:
        Subgroup error-state-basic:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
Test drv_module_reload_basic:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
Test gem_exec_suspend:
        Subgroup basic-s3:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
Test kms_cursor_legacy:
        Subgroup basic-cursor-vs-flip-varying-size:
                fail       -> PASS       (ro-ilk1-i5-650)
        Subgroup basic-flip-vs-cursor-legacy:
                fail       -> PASS       (ro-ivb2-i7-3770)
                pass       -> FAIL       (ro-bdw-i5-5250u)
        Subgroup basic-flip-vs-cursor-varying-size:
                pass       -> FAIL       (ro-skl3-i5-6260u)
                fail       -> PASS       (ro-bdw-i5-5250u)
Test kms_pipe_crc_basic:
        Subgroup hang-read-crc-pipe-a:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
        Subgroup hang-read-crc-pipe-b:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
        Subgroup hang-read-crc-pipe-c:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
                pass       -> DMESG-WARN (fi-skl-i7-6700k)
        Subgroup suspend-read-crc-pipe-a:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
        Subgroup suspend-read-crc-pipe-b:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                skip       -> DMESG-WARN (ro-bdw-i5-5250u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)
        Subgroup suspend-read-crc-pipe-c:
                pass       -> DMESG-WARN (ro-bdw-i7-5600u)
                pass       -> DMESG-WARN (ro-skl3-i5-6260u)

fi-hsw-i7-4770k  total:244  pass:222  dwarn:0   dfail:0   fail:0   skip:22 
fi-kbl-qkkr      total:244  pass:184  dwarn:31  dfail:0   fail:3   skip:26 
fi-skl-i7-6700k  total:244  pass:202  dwarn:10  dfail:2   fail:2   skip:28 
fi-snb-i7-2600   total:244  pass:202  dwarn:0   dfail:0   fail:0   skip:42 
ro-bdw-i5-5250u  total:240  pass:213  dwarn:9   dfail:0   fail:1   skip:17 
ro-bdw-i7-5600u  total:240  pass:197  dwarn:10  dfail:0   fail:1   skip:32 
ro-bsw-n3050     total:240  pass:189  dwarn:6   dfail:0   fail:3   skip:42 
ro-byt-n2820     total:240  pass:197  dwarn:0   dfail:0   fail:3   skip:40 
ro-hsw-i3-4010u  total:240  pass:214  dwarn:0   dfail:0   fail:0   skip:26 
ro-hsw-i7-4770r  total:240  pass:185  dwarn:0   dfail:0   fail:0   skip:55 
ro-ilk1-i5-650   total:235  pass:174  dwarn:0   dfail:0   fail:1   skip:60 
ro-ivb-i7-3770   total:240  pass:205  dwarn:0   dfail:0   fail:0   skip:35 
ro-ivb2-i7-3770  total:240  pass:209  dwarn:0   dfail:0   fail:0   skip:31 
ro-skl3-i5-6260u total:240  pass:212  dwarn:10  dfail:0   fail:4   skip:14 

Results at /archive/results/CI_IGT_test/RO_Patchwork_1870/

e56d79f drm-intel-nightly: 2016y-08m-15d-10h-16m-44s UTC integration manifest
6f1de3d drm/i915: Add param for SVM
ce6e32e drm/i915: add SVM execbuf ioctl v10
fd45b62 drm/i915: IOMMU based SVM implementation v13
7fa4650 drm/i915: add create_context2 ioctl

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Aug 15, 2016 at 02:48:04PM +0300, Mika Kuoppala wrote:
>> From: Jesse Barnes <jbarnes@virtuousgeek.org>
>> 
>> Add i915_gem_context_create2_ioctl for passing flags
>> (e.g. SVM) when creating a context.
>> 
>> v2: check the pad on create_context
>> v3: rebase
>> v4: i915_dma is no more. create_gvt needs flags
>> 
>> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
>> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
>
> Considering we can use deferred ppgtt creation and have setparam do we
> need a new create ioctl just to set a flag?

So like this:

- create ctx with the default create ioctl
- set cxt param it for svm capable.
- first submit deferred creates

And we use the setparam point for returning
error if svm context are not there.

?
-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1469 bytes --]

On Mon, 2016-08-15 at 13:23 +0100, Chris Wilson wrote:
> On Mon, Aug 15, 2016 at 01:13:25PM +0100, David Woodhouse wrote:
> > On Mon, 2016-08-15 at 13:05 +0100, Chris Wilson wrote:
> > > On Mon, Aug 15, 2016 at 02:48:05PM +0300, Mika Kuoppala wrote:
> > > > 
> > > > + struct task_struct *task;
> > > 
> > > We don't need the task, we need the mm.
> > > 
> > > Holding the task is not sufficient.
> > 
> > From the pure DMA point of view, you don't need the MM at all. I handle
> > all that from the IOMMU side so it's none of your business, darling.
> 
> But you don't keep the mm alive for the duration of device activity,
> right? And you don't wait for the device to finish before releasing the
> mmu? (iiuc intel-svm.c)

We don't "keep it alive" (i.e. bump mm->mm_users), no.
We *did*, but it caused problems. See commit e57e58bd390a68 for the
gory details.

Now we only bump mm->mm_count so if the process exits, the MM can still
be torn down.

Since exit_mmap() happens before exit_files(), what happens on an
unclean shutdown is that the GPU may start to take faults on the PASID
which is in the process of exiting, before the corresponding file
descriptor gets closed.

So no, we don't wait for the device to finish before releasing the MM.
That would involve calling back into device-driver code from the
mmu_notifier callback, with "interesting" locking constraints. We don't
trust device drivers that much :)

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Mika Kuoppala]

Chris Wilson <chris@chris-wilson.co.uk> writes:

> On Mon, Aug 15, 2016 at 02:48:06PM +0300, Mika Kuoppala wrote:
>> From: Jesse Barnes <jbarnes@virtuousgeek.org>
>> 
>> We just need to pass in an address to execute and some flags, since we
>> don't have to worry about buffer relocation or any of the other usual
>> stuff.  Returns a fence to be used for synchronization.
>> 
>> v2: add a request after batch submission (Jesse)
>> v3: add a flag for fence creation (Chris)
>> v4: add CLOEXEC flag (Kristian)
>>     add non-RCS ring support (Jesse)
>> v5: update for request alloc change (Jesse)
>> v6: new sync file interface, error paths, request breadcrumbs
>> v7: always CLOEXEC for sync_file_install
>> v8: rebase on new sync file api
>> v9: rework on top of fence requests and sync_file
>> v10: take fence ref for sync_file (Chris)
>>      use correct flush (Chris)
>>      limit exec on rcs
>
> This is incomplete, so just proof of principle?

At some point of rebasing I noticed that Jesse did limit
everything on rcs. So I just put it back.

No idea yet why we would need to limit for rcs only.

-Mika

> -Chris
>
> -- 
> Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[Chris Wilson]

On Mon, Aug 15, 2016 at 01:30:11PM +0100, David Woodhouse wrote:
> On Mon, 2016-08-15 at 13:23 +0100, Chris Wilson wrote:
> > On Mon, Aug 15, 2016 at 01:13:25PM +0100, David Woodhouse wrote:
> > > On Mon, 2016-08-15 at 13:05 +0100, Chris Wilson wrote:
> > > > On Mon, Aug 15, 2016 at 02:48:05PM +0300, Mika Kuoppala wrote:
> > > > > 
> > > > > + struct task_struct *task;
> > > > 
> > > > We don't need the task, we need the mm.
> > > > 
> > > > Holding the task is not sufficient.
> > > 
> > > From the pure DMA point of view, you don't need the MM at all. I handle
> > > all that from the IOMMU side so it's none of your business, darling.
> > 
> > But you don't keep the mm alive for the duration of device activity,
> > right? And you don't wait for the device to finish before releasing the
> > mmu? (iiuc intel-svm.c)
> 
> We don't "keep it alive" (i.e. bump mm->mm_users), no.
> We *did*, but it caused problems. See commit e57e58bd390a68 for the
> gory details.
> 
> Now we only bump mm->mm_count so if the process exits, the MM can still
> be torn down.
> 
> Since exit_mmap() happens before exit_files(), what happens on an
> unclean shutdown is that the GPU may start to take faults on the PASID
> which is in the process of exiting, before the corresponding file
> descriptor gets closed.
> 
> So no, we don't wait for the device to finish before releasing the MM.
> That would involve calling back into device-driver code from the
> mmu_notifier callback, with "interesting" locking constraints. We don't
> trust device drivers that much :)

With the device allocating the memory, we can keep the object alive for
as long as required for it to complete the commands and for other users.

Other uses get access to the svm pages via shared memory (mmap, memfd)
and so another process copying from the buffer should be unaffected by
termination of the original process.

So it is really just what happens to commands for this client when it
dies/exits.  The kneejerk reaction is to say the pages should be kept
alive as they are now for !svm. We could be faced with a situation where
the client copies onto a shared buffer (obtaining a fence), passes that
fence over to the server scheduling an update, and die abruptly. Given
that the fence and request arrive on the server safely (the fence will
be completed even if the command is skipped or its faults filled with
zero), the server will itself proceed to present the incomplete result
from the dead client. (Presently for !svm the output will be intact.)

The question is do we accept the change in behaviour? Or I am
completely misunderstanding how the svm faulting/mmu-notifiers will
work?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Chris Wilson]

On Mon, Aug 15, 2016 at 03:25:43PM +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > On Mon, Aug 15, 2016 at 02:48:04PM +0300, Mika Kuoppala wrote:
> >> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> 
> >> Add i915_gem_context_create2_ioctl for passing flags
> >> (e.g. SVM) when creating a context.
> >> 
> >> v2: check the pad on create_context
> >> v3: rebase
> >> v4: i915_dma is no more. create_gvt needs flags
> >> 
> >> Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> >
> > Considering we can use deferred ppgtt creation and have setparam do we
> > need a new create ioctl just to set a flag?
> 
> So like this:
> 
> - create ctx with the default create ioctl
> - set cxt param it for svm capable.
> - first submit deferred creates
> 
> And we use the setparam point for returning
> error if svm context are not there.

(and a call to set svm on a context after first use is illegal)

That's the outline I had in my head. I am not sure if the result is
cleaner - I just hope it is ;)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 2/4] drm/i915: IOMMU based SVM implementation v13

[David Woodhouse]

[-- Attachment #1.1: Type: text/plain, Size: 1454 bytes --]

On Mon, 2016-08-15 at 13:53 +0100, Chris Wilson wrote:
> 
> So it is really just what happens to commands for this client when it
> dies/exits.  The kneejerk reaction is to say the pages should be kept
> alive as they are now for !svm. We could be faced with a situation where
> the client copies onto a shared buffer (obtaining a fence), passes that
> fence over to the server scheduling an update, and die abruptly.

Which pages?

Until the moment you actually do the DMA, you don't have "pages". They
might not even exist in RAM. All you have is (a PASID and) a userspace
linear address.

When you actually the DMA, *then* we might fault in the appropriate
pages from disk. Or might not, depending on whether the address is
valid or not.

Between the time when it hands you the linear address, and the time
that you use it, the process could have done anything. we are currently
talking about the case where it exits uncleanly. But it could also
munmap() the linear address in question. Or mmap() something else over
it. Obviously those would be bugs... but so is an unclean exit.

So it doesn't seem to make much sense to ask if you accept the change
in behaviour. You don't really have much choice; it's implicit in the
SVM model of doing DMA directly to userspace addresses. You just
*don't* get to lock things down and trust that the buffers will still
be there when you finally get round to using them.

-- 
dwmw2

[-- Attachment #1.2: smime.p7s --]
[-- Type: application/x-pkcs7-signature, Size: 5760 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 0/4] svm support

[Chris Wilson]

On Mon, Aug 15, 2016 at 02:48:03PM +0300, Mika Kuoppala wrote:
> Hi,
> 
> Now when fences got merged I reworked the series. It is now much
> smaller in size. Some items are still missing like error state recording,
> fault handling, documentation and in fences.

What's the plan for KMS integration?

We didn't like using userptr for framebuffer objects for the reason the
backing pages do not perist beyond the process (or mmap).

So create regular objects for KMS and CPU mmap them for SVM use?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Jesse Barnes]

On Mon, 2016-08-15 at 13:56 +0100, Chris Wilson wrote:
> On Mon, Aug 15, 2016 at 03:25:43PM +0300, Mika Kuoppala wrote:
> > 
> > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > 
> > > 
> > > On Mon, Aug 15, 2016 at 02:48:04PM +0300, Mika Kuoppala wrote:
> > > > 
> > > > From: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > 
> > > > Add i915_gem_context_create2_ioctl for passing flags
> > > > (e.g. SVM) when creating a context.
> > > > 
> > > > v2: check the pad on create_context
> > > > v3: rebase
> > > > v4: i915_dma is no more. create_gvt needs flags
> > > > 
> > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
> > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > > 
> > > Considering we can use deferred ppgtt creation and have setparam
> > > do we
> > > need a new create ioctl just to set a flag?
> > 
> > So like this:
> > 
> > - create ctx with the default create ioctl
> > - set cxt param it for svm capable.
> > - first submit deferred creates
> > 
> > And we use the setparam point for returning
> > error if svm context are not there.
> 
> (and a call to set svm on a context after first use is illegal)
> 
> That's the outline I had in my head. I am not sure if the result is
> cleaner - I just hope it is ;)
> 


I opted against that initially because creating the tables and setup
after the fact for the process actually seemed messier.  Plus I thought
we'd want more flags at context create later anyway...

Not that my opinion matters anymore. :)

Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Jesse Barnes]

On Mon, 2016-08-15 at 15:34 +0300, Mika Kuoppala wrote:
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > 
> > On Mon, Aug 15, 2016 at 02:48:06PM +0300, Mika Kuoppala wrote:
> > > 
> > > From: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > 
> > > We just need to pass in an address to execute and some flags,
> > > since we
> > > don't have to worry about buffer relocation or any of the other
> > > usual
> > > stuff.  Returns a fence to be used for synchronization.
> > > 
> > > v2: add a request after batch submission (Jesse)
> > > v3: add a flag for fence creation (Chris)
> > > v4: add CLOEXEC flag (Kristian)
> > >     add non-RCS ring support (Jesse)
> > > v5: update for request alloc change (Jesse)
> > > v6: new sync file interface, error paths, request breadcrumbs
> > > v7: always CLOEXEC for sync_file_install
> > > v8: rebase on new sync file api
> > > v9: rework on top of fence requests and sync_file
> > > v10: take fence ref for sync_file (Chris)
> > >      use correct flush (Chris)
> > >      limit exec on rcs
> > 
> > This is incomplete, so just proof of principle?
> 
> At some point of rebasing I noticed that Jesse did limit
> everything on rcs. So I just put it back.
> 
> No idea yet why we would need to limit for rcs only.
> 

I went back and forth; I think I did test on the BLT ring and maybe one
of the video rings and things worked on at least one platform.  But I'm
still worried about bugs...

Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 1/4] drm/i915: add create_context2 ioctl

[Chris Wilson]

On Mon, Aug 15, 2016 at 09:25:15AM -0700, Jesse Barnes wrote:
> On Mon, 2016-08-15 at 13:56 +0100, Chris Wilson wrote:
> > On Mon, Aug 15, 2016 at 03:25:43PM +0300, Mika Kuoppala wrote:
> > > 
> > > Chris Wilson <chris@chris-wilson.co.uk> writes:
> > > 
> > > > 
> > > > On Mon, Aug 15, 2016 at 02:48:04PM +0300, Mika Kuoppala wrote:
> > > > > 
> > > > > From: Jesse Barnes <jbarnes@virtuousgeek.org>
> > > > > 
> > > > > Add i915_gem_context_create2_ioctl for passing flags
> > > > > (e.g. SVM) when creating a context.
> > > > > 
> > > > > v2: check the pad on create_context
> > > > > v3: rebase
> > > > > v4: i915_dma is no more. create_gvt needs flags
> > > > > 
> > > > > Cc: Daniel Vetter <daniel.vetter@ffwll.ch>
> > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk>
> > > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> > > > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org> (v1)
> > > > > Signed-off-by: Mika Kuoppala <mika.kuoppala@intel.com>
> > > > 
> > > > Considering we can use deferred ppgtt creation and have setparam
> > > > do we
> > > > need a new create ioctl just to set a flag?
> > > 
> > > So like this:
> > > 
> > > - create ctx with the default create ioctl
> > > - set cxt param it for svm capable.
> > > - first submit deferred creates
> > > 
> > > And we use the setparam point for returning
> > > error if svm context are not there.
> > 
> > (and a call to set svm on a context after first use is illegal)
> > 
> > That's the outline I had in my head. I am not sure if the result is
> > cleaner - I just hope it is ;)
> > 
> 
> 
> I opted against that initially because creating the tables and setup
> after the fact for the process actually seemed messier.  Plus I thought
> we'd want more flags at context create later anyway...

I was half thinking of some of the silly microbenchmarks might be
improved if we deferred allocation until first use (and others will no
doubt get worse). Plus we already have an interface for making
arbitrarily complex adjustments to a context, so I was trying to avoid a
second.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Joonas Lahtinen]

On ma, 2016-08-15 at 09:26 -0700, Jesse Barnes wrote:
> On Mon, 2016-08-15 at 15:34 +0300, Mika Kuoppala wrote:
> > 
> > No idea yet why we would need to limit for rcs only.
> > 
> I went back and forth; I think I did test on the BLT ring and maybe one
> of the video rings and things worked on at least one platform.  But I'm
> still worried about bugs...
> 

Any other reason for worrying other than lack of testing?

I'm pretty sure programs using OpenCL (Beignet) will want this on other
rings too for zero-copy video processing as an example.

Regards, Joonas

> Jesse
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH RFC 3/4] drm/i915: add SVM execbuf ioctl v10

[Jesse Barnes]

On Wed, 2016-08-17 at 12:37 +0300, Joonas Lahtinen wrote:
> On ma, 2016-08-15 at 09:26 -0700, Jesse Barnes wrote:
> > 
> > On Mon, 2016-08-15 at 15:34 +0300, Mika Kuoppala wrote:
> > > 
> > > 
> > > No idea yet why we would need to limit for rcs only.
> > > 
> > I went back and forth; I think I did test on the BLT ring and maybe
> > one
> > of the video rings and things worked on at least one platform.  But
> > I'm
> > still worried about bugs...
> > 
> 
> Any other reason for worrying other than lack of testing?
> 
> I'm pretty sure programs using OpenCL (Beignet) will want this on
> other
> rings too for zero-copy video processing as an example.
> 

No, not that I remember; and KBL is probably totally fine here.  Just
run some tests and turn it on, then hope for the best. :)

Jesse
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx