Context isolation

Context isolation

[Chris Wilson]

Rebase and resend and mmio workarounds moved (see patch 5/9), and they
are very sensitive to init-ordering...
-Chris

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

[PATCH v5 1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Chris Wilson]

Now that we have a common engine state pretty printer, we can use that
instead of the adhoc information printed when we miss a breadcrumb.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Mika Kuoppala <mika.kuoppala@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
---
 drivers/gpu/drm/i915/intel_breadcrumbs.c | 11 +++++------
 drivers/gpu/drm/i915/intel_engine_cs.c   |  6 ++++++
 2 files changed, 11 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 4de054f8c1ba..1e4d2978fb86 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -64,12 +64,11 @@ static unsigned long wait_timeout(void)
 
 static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
 {
-	DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS, irq posted? %s, current seqno=%x, last=%x\n",
-			 engine->name, __builtin_return_address(0),
-			 yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
-					&engine->irq_posted)),
-			 intel_engine_get_seqno(engine),
-			 intel_engine_last_submit(engine));
+	struct drm_printer p = drm_debug_printer(__func__);
+
+	DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS\n",
+			 engine->name, __builtin_return_address(0));
+	intel_engine_dump(engine, &p);
 
 	set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
 }
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 6997306be0d2..f22dc452030f 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1835,6 +1835,12 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
 	}
 	spin_unlock_irq(&b->rb_lock);
 
+	drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s) (execlists? %s)\n",
+		   engine->irq_posted,
+		   yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
+				  &engine->irq_posted)),
+		   yesno(test_bit(ENGINE_IRQ_EXECLIST,
+				  &engine->irq_posted)));
 	drm_printf(m, "Idle? %s\n", yesno(intel_engine_is_idle(engine)));
 	drm_printf(m, "\n");
 }
-- 
2.15.0

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

[PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We want to be able to report back to userspace details about an engine's
class, and in return for userspace to be able to request actions
regarding certain classes of engines. To isolate the uABI from any
variations between hw generations, we define an abstract class for the
engines and internally map onto the hw.

v2: Remove MAX from the uABI; keep it internal if we need it, but don't
let userspace make the mistake of using it themselves.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 10 +++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++++-
 include/uapi/drm/i915_drm.h             | 15 +++++++++++++++
 3 files changed, 28 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index f22dc452030f..a91355c41d45 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -50,6 +50,8 @@ struct engine_class_info {
 	const char *name;
 	int (*init_legacy)(struct intel_engine_cs *engine);
 	int (*init_execlists)(struct intel_engine_cs *engine);
+
+	u8 uabi_class;
 };
 
 static const struct engine_class_info intel_engine_classes[] = {
@@ -57,21 +59,25 @@ static const struct engine_class_info intel_engine_classes[] = {
 		.name = "rcs",
 		.init_execlists = logical_render_ring_init,
 		.init_legacy = intel_init_render_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_RENDER,
 	},
 	[COPY_ENGINE_CLASS] = {
 		.name = "bcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_blt_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_COPY,
 	},
 	[VIDEO_DECODE_CLASS] = {
 		.name = "vcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_bsd_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO,
 	},
 	[VIDEO_ENHANCEMENT_CLASS] = {
 		.name = "vecs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_vebox_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO_ENHANCE,
 	},
 };
 
@@ -213,13 +219,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	WARN_ON(snprintf(engine->name, sizeof(engine->name), "%s%u",
 			 class_info->name, info->instance) >=
 		sizeof(engine->name));
-	engine->uabi_id = info->uabi_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
 	engine->class = info->class;
 	engine->instance = info->instance;
 
+	engine->uabi_id = info->uabi_id;
+	engine->uabi_class = class_info->uabi_class;
+
 	engine->context_size = __intel_engine_context_size(dev_priv,
 							   engine->class);
 	if (WARN_ON(engine->context_size > BIT(20)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1106904f6e31..7d3903b9fb1d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -290,11 +290,14 @@ struct intel_engine_execlists {
 struct intel_engine_cs {
 	struct drm_i915_private *i915;
 	char name[INTEL_ENGINE_CS_MAX_NAME];
+
 	enum intel_engine_id id;
-	unsigned int uabi_id;
 	unsigned int hw_id;
 	unsigned int guc_id;
 
+	u8 uabi_id;
+	u8 uabi_class;
+
 	u8 class;
 	u8 instance;
 	u32 context_size;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ac3c6503ca27..65d06da62599 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -86,6 +86,21 @@ enum i915_mocs_table_index {
 	I915_MOCS_CACHED,
 };
 
+/*
+ * Different engines serve different roles, and there may be more than one
+ * engine serving each role. enum drm_i915_gem_engine_class provides a
+ * classification of the role of the engine, which may be used when requesting
+ * operations to be performed on a certain subset of engines, or for providing
+ * information about that group.
+ */
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_OTHER = 0,
+	I915_ENGINE_CLASS_RENDER = 1,
+	I915_ENGINE_CLASS_COPY = 2,
+	I915_ENGINE_CLASS_VIDEO = 3,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
2.15.0

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

[PATCH v5 3/9] drm/i915: Force the switch to the i915->kernel_context

[Chris Wilson]

In the next few patches, we will have a hard requirement that we emit a
context-switch to the perma-pinned i915->kernel_context (so that we can
save the HW state using that context-switch). As the first context
itself may be classed as a kernel context, we want to be explicit in our
comparison. For an extra-layer of finesse, we can check the last
unretired context on the engine; as well as the last retired context
when idle.

v2: verbose verbosity
v3: Always force the switch, even when the engine is idle, and update
the assert that this happens before suspend.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v1
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 10 ++++++----
 drivers/gpu/drm/i915/intel_engine_cs.c | 26 ++++++++++++++++++++++++--
 2 files changed, 30 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 889ae8810d5f..824515556733 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4693,14 +4693,16 @@ void __i915_gem_object_release_unless_active(struct drm_i915_gem_object *obj)
 		i915_gem_object_put(obj);
 }
 
-static void assert_kernel_context_is_current(struct drm_i915_private *dev_priv)
+static void assert_kernel_context_is_current(struct drm_i915_private *i915)
 {
+	struct i915_gem_context *kernel_context = i915->kernel_context;
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
-	for_each_engine(engine, dev_priv, id)
-		GEM_BUG_ON(engine->last_retired_context &&
-			   !i915_gem_context_is_kernel(engine->last_retired_context));
+	for_each_engine(engine, i915, id) {
+		GEM_BUG_ON(__i915_gem_active_peek(&engine->timeline->last_request));
+		GEM_BUG_ON(engine->last_retired_context != kernel_context);
+	}
 }
 
 void i915_gem_sanitize(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index a91355c41d45..047fca656b4d 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1593,10 +1593,32 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv)
 	return true;
 }
 
+/**
+ * intel_engine_has_kernel_context:
+ * @engine: the engine
+ *
+ * Returns true if the last context to be executed on this engine, or has been
+ * executed if the engine is already idle, is the kernel context
+ * (#i915.kernel_context).
+ */
 bool intel_engine_has_kernel_context(const struct intel_engine_cs *engine)
 {
-	return (!engine->last_retired_context ||
-		i915_gem_context_is_kernel(engine->last_retired_context));
+	const struct i915_gem_context * const kernel_context =
+		engine->i915->kernel_context;
+	struct drm_i915_gem_request *rq;
+
+	lockdep_assert_held(&engine->i915->drm.struct_mutex);
+
+	/*
+	 * Check the last context seen by the engine. If active, it will be
+	 * the last request that remains in the timeline. When idle, it is
+	 * the last executed context as tracked by retirement.
+	 */
+	rq = __i915_gem_active_peek(&engine->timeline->last_request);
+	if (rq)
+		return rq->ctx == kernel_context;
+	else
+		return engine->last_retired_context == kernel_context;
 }
 
 void intel_engines_reset_default_submission(struct drm_i915_private *i915)
-- 
2.15.0

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

[PATCH v5 4/9] drm/i915: Move GT powersaving init to i915_gem_init()

[Chris Wilson]

GT powersaving is tightly coupled to the request infrastructure. To
avoid complications with the order of initialisation in the next patch
(where we want to send requests to hw during GEM init) move the
powersaving initialisation into the purview of i915_gem_init().

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c      | 2 ++
 drivers/gpu/drm/i915/intel_display.c | 2 --
 drivers/gpu/drm/i915/intel_pm.c      | 2 --
 3 files changed, 2 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 824515556733..37586f703c1e 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5021,6 +5021,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto out_unlock;
 
+	intel_init_gt_powersave(dev_priv);
+
 	ret = i915_gem_init_hw(dev_priv);
 	if (ret == -EIO) {
 		/* Allow engine initialisation to fail by marking the GPU as
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 84817ccc5305..bc1c6ccde7db 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15197,8 +15197,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	intel_init_gt_powersave(dev_priv);
-
 	intel_init_clock_gating(dev_priv);
 
 	intel_setup_overlay(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index e09377df590d..48a127ac6de5 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -7918,7 +7918,6 @@ void intel_init_gt_powersave(struct drm_i915_private *dev_priv)
 		intel_runtime_pm_get(dev_priv);
 	}
 
-	mutex_lock(&dev_priv->drm.struct_mutex);
 	mutex_lock(&dev_priv->pcu_lock);
 
 	/* Initialize RPS limits (for userspace) */
@@ -7960,7 +7959,6 @@ void intel_init_gt_powersave(struct drm_i915_private *dev_priv)
 	rps->boost_freq = rps->max_freq;
 
 	mutex_unlock(&dev_priv->pcu_lock);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
 
 	intel_autoenable_gt_powersave(dev_priv);
 }
-- 
2.15.0

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

[PATCH v5 5/9] drm/i915: Move intel_init_clock_gating() to i915_gem_init()

[Chris Wilson]

Despite its name intel_init_clock_gating applies both display clock gating
workarounds; GT mmio workarounds and the occasional GT power context
workaround. Worse, sometimes it includes a context register workaround
which we need to apply before we record the default HW state for all
contexts.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c      | 16 ++++++++++++++--
 drivers/gpu/drm/i915/intel_display.c |  2 --
 2 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 37586f703c1e..5c0fc78de853 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5024,6 +5024,20 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	intel_init_gt_powersave(dev_priv);
 
 	ret = i915_gem_init_hw(dev_priv);
+	if (ret)
+		goto out_unlock;
+
+	/* Despite its name intel_init_clock_gating applies both display
+	 * clock gating workarounds; GT mmio workarounds and the occasional
+	 * GT power context workaround. Worse, sometimes it includes a context
+	 * register workaround which we need to apply before we record the
+	 * default HW state for all contexts.
+	 *
+	 * FIXME: break up the workarounds and apply them at the right time!
+	 */
+	intel_init_clock_gating(dev_priv);
+
+out_unlock:
 	if (ret == -EIO) {
 		/* Allow engine initialisation to fail by marking the GPU as
 		 * wedged. But we only want to do this where the GPU is angry,
@@ -5035,8 +5049,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 		}
 		ret = 0;
 	}
-
-out_unlock:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bc1c6ccde7db..618e912fcaec 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15197,8 +15197,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
 
-	intel_init_clock_gating(dev_priv);
-
 	intel_setup_overlay(dev_priv);
 }
 
-- 
2.15.0

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

[PATCH v5 6/9] drm/i915: Inline intel_modeset_gem_init()

[Chris Wilson]

intel_modeset_gem_init() now only sets up the legacy overlay, so let's
remove the function and call the setup directly during driver load. This
should help us find a better point in the initialisation sequence for it
later.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      | 2 +-
 drivers/gpu/drm/i915/i915_drv.h      | 1 -
 drivers/gpu/drm/i915/intel_display.c | 7 -------
 3 files changed, 1 insertion(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index e7e9e061073b..1b440f2b90a5 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -676,7 +676,7 @@ static int i915_load_modeset_init(struct drm_device *dev)
 	if (ret)
 		goto cleanup_uc;
 
-	intel_modeset_gem_init(dev);
+	intel_setup_overlay(dev_priv);
 
 	if (INTEL_INFO(dev_priv)->num_pipes == 0)
 		return 0;
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 5e6938d0a903..7596dc1e1346 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -4124,7 +4124,6 @@ void intel_device_info_dump(struct drm_i915_private *dev_priv);
 /* modesetting */
 extern void intel_modeset_init_hw(struct drm_device *dev);
 extern int intel_modeset_init(struct drm_device *dev);
-extern void intel_modeset_gem_init(struct drm_device *dev);
 extern void intel_modeset_cleanup(struct drm_device *dev);
 extern int intel_connector_register(struct drm_connector *);
 extern void intel_connector_unregister(struct drm_connector *);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 618e912fcaec..ea6c6b61b996 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -15193,13 +15193,6 @@ void intel_display_resume(struct drm_device *dev)
 		drm_atomic_state_put(state);
 }
 
-void intel_modeset_gem_init(struct drm_device *dev)
-{
-	struct drm_i915_private *dev_priv = to_i915(dev);
-
-	intel_setup_overlay(dev_priv);
-}
-
 int intel_connector_register(struct drm_connector *connector)
 {
 	struct intel_connector *intel_connector = to_intel_connector(connector);
-- 
2.15.0

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

[PATCH v5 7/9] drm/i915: Mark the context state as dirty/written

[Chris Wilson]

In the next few patches, we will want to both copy out of the context
image and write a valid image into a new context. To be completely safe,
we should then couple in our domain tracking to ensure that we don't
have any issues with stale data remaining in unwanted cachelines.

Historically, we omitted the .write=true from the call to set-gtt-domain
in i915_switch_context() in order to avoid a stall between every request
as we would want to wait for the previous context write from the gpu.
Since then, we limit the set-gtt-domain to only occur when we first bind
the vma, so once in use we will never stall, and we are sure to flush
the context following a load from swap.

Equally we never applied the lessons learnt from ringbuffer submission
to execlists; so time to apply the flush of the lrc after load as well.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Acked-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_lrc.c        | 32 ++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_ringbuffer.c |  6 +++---
 2 files changed, 27 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 6840ec8db037..9b4e74151ace 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1060,12 +1060,34 @@ static void execlists_schedule(struct drm_i915_gem_request *request, int prio)
 	spin_unlock_irq(&engine->timeline->lock);
 }
 
+static int __context_pin(struct i915_gem_context *ctx, struct i915_vma *vma)
+{
+	unsigned int flags;
+	int err;
+
+	/*
+	 * Clear this page out of any CPU caches for coherent swap-in/out.
+	 * We only want to do this on the first bind so that we do not stall
+	 * on an active context (which by nature is already on the GPU).
+	 */
+	if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
+		err = i915_gem_object_set_to_gtt_domain(vma->obj, true);
+		if (err)
+			return err;
+	}
+
+	flags = PIN_GLOBAL | PIN_HIGH;
+	if (ctx->ggtt_offset_bias)
+		flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias;
+
+	return i915_vma_pin(vma, 0, GEN8_LR_CONTEXT_ALIGN, flags);
+}
+
 static struct intel_ring *
 execlists_context_pin(struct intel_engine_cs *engine,
 		      struct i915_gem_context *ctx)
 {
 	struct intel_context *ce = &ctx->engine[engine->id];
-	unsigned int flags;
 	void *vaddr;
 	int ret;
 
@@ -1082,11 +1104,7 @@ execlists_context_pin(struct intel_engine_cs *engine,
 	}
 	GEM_BUG_ON(!ce->state);
 
-	flags = PIN_GLOBAL | PIN_HIGH;
-	if (ctx->ggtt_offset_bias)
-		flags |= PIN_OFFSET_BIAS | ctx->ggtt_offset_bias;
-
-	ret = i915_vma_pin(ce->state, 0, GEN8_LR_CONTEXT_ALIGN, flags);
+	ret = __context_pin(ctx, ce->state);
 	if (ret)
 		goto err;
 
@@ -1106,9 +1124,7 @@ execlists_context_pin(struct intel_engine_cs *engine,
 	ce->lrc_reg_state[CTX_RING_BUFFER_START+1] =
 		i915_ggtt_offset(ce->ring->vma);
 
-	ce->state->obj->mm.dirty = true;
 	ce->state->obj->pin_global++;
-
 	i915_gem_context_get(ctx);
 out:
 	return ce->ring;
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 47fadf8da84e..7e2a671882fb 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1363,12 +1363,13 @@ static int context_pin(struct i915_gem_context *ctx)
 	struct i915_vma *vma = ctx->engine[RCS].state;
 	int ret;
 
-	/* Clear this page out of any CPU caches for coherent swap-in/out.
+	/*
+	 * Clear this page out of any CPU caches for coherent swap-in/out.
 	 * We only want to do this on the first bind so that we do not stall
 	 * on an active context (which by nature is already on the GPU).
 	 */
 	if (!(vma->flags & I915_VMA_GLOBAL_BIND)) {
-		ret = i915_gem_object_set_to_gtt_domain(vma->obj, false);
+		ret = i915_gem_object_set_to_gtt_domain(vma->obj, true);
 		if (ret)
 			return ret;
 	}
@@ -1445,7 +1446,6 @@ intel_ring_context_pin(struct intel_engine_cs *engine,
 		if (ret)
 			goto err;
 
-		ce->state->obj->mm.dirty = true;
 		ce->state->obj->pin_global++;
 	}
 
-- 
2.15.0

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

[PATCH v5 8/9] drm/i915: Record the default hw state after reset upon load

[Chris Wilson]

Take a copy of the HW state after a reset upon module loading by
executing a context switch from a blank context to the kernel context,
thus saving the default hw state over the blank context image.
We can then use the default hw state to initialise any future context,
ensuring that each starts with the default view of hw state.

v2: Unmap our default state from the GTT after stealing it from the
context. This should stop us from accidentally overwriting it via the
GTT (and frees up some precious GTT space).

Testcase: igt/gem_ctx_isolation
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/gvt/scheduler.c    |   2 -
 drivers/gpu/drm/i915/i915_debugfs.c     |   1 -
 drivers/gpu/drm/i915/i915_drv.c         |   3 +
 drivers/gpu/drm/i915/i915_gem.c         | 115 ++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/i915_gem_context.c |  55 ++++-----------
 drivers/gpu/drm/i915/i915_gem_context.h |   4 +-
 drivers/gpu/drm/i915/intel_engine_cs.c  |  17 +++++
 drivers/gpu/drm/i915/intel_lrc.c        |  39 +++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.c |  45 +++++++++----
 drivers/gpu/drm/i915/intel_ringbuffer.h |   2 +
 include/uapi/drm/i915_drm.h             |  15 +++++
 11 files changed, 225 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/gvt/scheduler.c b/drivers/gpu/drm/i915/gvt/scheduler.c
index f6ded475bb2c..42cc61230ca7 100644
--- a/drivers/gpu/drm/i915/gvt/scheduler.c
+++ b/drivers/gpu/drm/i915/gvt/scheduler.c
@@ -723,8 +723,6 @@ int intel_vgpu_init_gvt_context(struct intel_vgpu *vgpu)
 	if (IS_ERR(vgpu->shadow_ctx))
 		return PTR_ERR(vgpu->shadow_ctx);
 
-	vgpu->shadow_ctx->engine[RCS].initialised = true;
-
 	bitmap_zero(vgpu->shadow_ctx_desc_updated, I915_NUM_ENGINES);
 
 	return 0;
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 39883cd915db..cfcef1899da8 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1974,7 +1974,6 @@ static int i915_context_status(struct seq_file *m, void *unused)
 			struct intel_context *ce = &ctx->engine[engine->id];
 
 			seq_printf(m, "%s: ", engine->name);
-			seq_putc(m, ce->initialised ? 'I' : 'i');
 			if (ce->state)
 				describe_obj(m, ce->state->obj);
 			if (ce->ring)
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 1b440f2b90a5..d97fe9c9439a 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -406,6 +406,9 @@ static int i915_getparam(struct drm_device *dev, void *data,
 		 */
 		value = 1;
 		break;
+	case I915_PARAM_HAS_CONTEXT_ISOLATION:
+		value = intel_engines_has_context_isolation(dev_priv);
+		break;
 	case I915_PARAM_SLICE_MASK:
 		value = INTEL_INFO(dev_priv)->sseu.slice_mask;
 		if (!value)
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 5c0fc78de853..1fd2e8809683 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4972,6 +4972,120 @@ bool intel_sanitize_semaphores(struct drm_i915_private *dev_priv, int value)
 	return true;
 }
 
+static int __intel_engines_record_defaults(struct drm_i915_private *i915)
+{
+	struct i915_gem_context *ctx;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int err;
+
+	/*
+	 * As we reset the gpu during very early sanitisation, the current
+	 * register state on the GPU should reflect its defaults values.
+	 * We load a context onto the hw (with restore-inhibit), then switch
+	 * over to a second context to save that default register state. We
+	 * can then prime every new context with that state so they all start
+	 * from the same default HW values.
+	 */
+
+	ctx = i915_gem_context_create_kernel(i915, 0);
+	if (IS_ERR(ctx))
+		return PTR_ERR(ctx);
+
+	for_each_engine(engine, i915, id) {
+		struct drm_i915_gem_request *rq;
+
+		rq = i915_gem_request_alloc(engine, ctx);
+		if (IS_ERR(rq)) {
+			err = PTR_ERR(rq);
+			goto out_ctx;
+		}
+
+		err = i915_switch_context(rq);
+		if (engine->init_context)
+			err = engine->init_context(rq);
+
+		__i915_add_request(rq, true);
+		if (err)
+			goto err_active;
+	}
+
+	err = i915_gem_switch_to_kernel_context(i915);
+	if (err)
+		goto err_active;
+
+	err = i915_gem_wait_for_idle(i915, I915_WAIT_LOCKED);
+	if (err)
+		goto err_active;
+
+	assert_kernel_context_is_current(i915);
+
+	for_each_engine(engine, i915, id) {
+		struct i915_vma *state;
+
+		state = ctx->engine[id].state;
+		if (!state)
+			continue;
+
+		/*
+		 * As we will hold a reference to the logical state, it will
+		 * not be torn down with the context, and importantly the
+		 * object will hold onto its vma (making it possible for a
+		 * stray GTT write to corrupt our defaults). Unmap the vma
+		 * from the GTT to prevent such accidents and reclaim the
+		 * space.
+		 */
+		err = i915_vma_unbind(state);
+		if (err)
+			goto err_active;
+
+		err = i915_gem_object_set_to_cpu_domain(state->obj, false);
+		if (err)
+			goto err_active;
+
+		engine->default_state = i915_gem_object_get(state->obj);
+	}
+
+	if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM)) {
+		unsigned int found = intel_engines_has_context_isolation(i915);
+
+		/*
+		 * Make sure that classes with multiple engine instances all
+		 * share the same basic configuration.
+		 */
+		for_each_engine(engine, i915, id) {
+			unsigned int bit = BIT(engine->uabi_class);
+			unsigned int expected = engine->default_state ? bit : 0;
+
+			if ((found & bit) != expected) {
+				DRM_ERROR("mismatching default context state for class %d on engine %s\n",
+					  engine->uabi_class, engine->name);
+			}
+		}
+	}
+
+out_ctx:
+	i915_gem_context_set_closed(ctx);
+	i915_gem_context_put(ctx);
+	return err;
+
+err_active:
+	/*
+	 * If we have to abandon now, we expect the engines to be idle
+	 * and ready to be torn-down. First try to flush any remaining
+	 * request, ensure we are pointing at the kernel context and
+	 * then remove it.
+	 */
+	if (WARN_ON(i915_gem_switch_to_kernel_context(i915)))
+		goto out_ctx;
+
+	if (WARN_ON(i915_gem_wait_for_idle(i915, I915_WAIT_LOCKED)))
+		goto out_ctx;
+
+	i915_gem_contexts_lost(i915);
+	goto out_ctx;
+}
+
 int i915_gem_init(struct drm_i915_private *dev_priv)
 {
 	int ret;
@@ -5037,6 +5151,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	 */
 	intel_init_clock_gating(dev_priv);
 
+	ret = __intel_engines_record_defaults(dev_priv);
 out_unlock:
 	if (ret == -EIO) {
 		/* Allow engine initialisation to fail by marking the GPU as
diff --git a/drivers/gpu/drm/i915/i915_gem_context.c b/drivers/gpu/drm/i915/i915_gem_context.c
index 10affb35ac56..31ee5ec4a0a8 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.c
+++ b/drivers/gpu/drm/i915/i915_gem_context.c
@@ -418,8 +418,8 @@ i915_gem_context_create_gvt(struct drm_device *dev)
 	return ctx;
 }
 
-static struct i915_gem_context *
-create_kernel_context(struct drm_i915_private *i915, int prio)
+struct i915_gem_context *
+i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio)
 {
 	struct i915_gem_context *ctx;
 
@@ -473,7 +473,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 	ida_init(&dev_priv->contexts.hw_ida);
 
 	/* lowest priority; idle task */
-	ctx = create_kernel_context(dev_priv, I915_PRIORITY_MIN);
+	ctx = i915_gem_context_create_kernel(dev_priv, I915_PRIORITY_MIN);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default global context\n");
 		err = PTR_ERR(ctx);
@@ -487,7 +487,7 @@ int i915_gem_contexts_init(struct drm_i915_private *dev_priv)
 	dev_priv->kernel_context = ctx;
 
 	/* highest priority; preempting task */
-	ctx = create_kernel_context(dev_priv, INT_MAX);
+	ctx = i915_gem_context_create_kernel(dev_priv, INT_MAX);
 	if (IS_ERR(ctx)) {
 		DRM_ERROR("Failed to create default preempt context\n");
 		err = PTR_ERR(ctx);
@@ -522,28 +522,6 @@ void i915_gem_contexts_lost(struct drm_i915_private *dev_priv)
 		engine->context_unpin(engine, engine->last_retired_context);
 		engine->last_retired_context = NULL;
 	}
-
-	/* Force the GPU state to be restored on enabling */
-	if (!i915_modparams.enable_execlists) {
-		struct i915_gem_context *ctx;
-
-		list_for_each_entry(ctx, &dev_priv->contexts.list, link) {
-			if (!i915_gem_context_is_default(ctx))
-				continue;
-
-			for_each_engine(engine, dev_priv, id)
-				ctx->engine[engine->id].initialised = false;
-
-			ctx->remap_slice = ALL_L3_SLICES(dev_priv);
-		}
-
-		for_each_engine(engine, dev_priv, id) {
-			struct intel_context *kce =
-				&dev_priv->kernel_context->engine[engine->id];
-
-			kce->initialised = true;
-		}
-	}
 }
 
 void i915_gem_contexts_fini(struct drm_i915_private *i915)
@@ -718,9 +696,6 @@ static inline bool skip_rcs_switch(struct i915_hw_ppgtt *ppgtt,
 	if (to->remap_slice)
 		return false;
 
-	if (!to->engine[RCS].initialised)
-		return false;
-
 	if (ppgtt && (intel_engine_flag(engine) & ppgtt->pd_dirty_rings))
 		return false;
 
@@ -795,11 +770,14 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
 			return ret;
 	}
 
-	if (!to->engine[RCS].initialised || i915_gem_context_is_default(to))
-		/* NB: If we inhibit the restore, the context is not allowed to
-		 * die because future work may end up depending on valid address
-		 * space. This means we must enforce that a page table load
-		 * occur when this occurs. */
+	if (i915_gem_context_is_kernel(to))
+		/*
+		 * The kernel context(s) is treated as pure scratch and is not
+		 * expected to retain any state (as we sacrifice it during
+		 * suspend and on resume it may be corrupted). This is ok,
+		 * as nothing actually executes using the kernel context; it
+		 * is purely used for flushing user contexts.
+		 */
 		hw_flags = MI_RESTORE_INHIBIT;
 	else if (ppgtt && intel_engine_flag(engine) & ppgtt->pd_dirty_rings)
 		hw_flags = MI_FORCE_RESTORE;
@@ -843,15 +821,6 @@ static int do_rcs_switch(struct drm_i915_gem_request *req)
 		to->remap_slice &= ~(1<<i);
 	}
 
-	if (!to->engine[RCS].initialised) {
-		if (engine->init_context) {
-			ret = engine->init_context(req);
-			if (ret)
-				return ret;
-		}
-		to->engine[RCS].initialised = true;
-	}
-
 	return 0;
 }
 
diff --git a/drivers/gpu/drm/i915/i915_gem_context.h b/drivers/gpu/drm/i915/i915_gem_context.h
index 44688e22a5c2..4bfb72f8e1cb 100644
--- a/drivers/gpu/drm/i915/i915_gem_context.h
+++ b/drivers/gpu/drm/i915/i915_gem_context.h
@@ -157,7 +157,6 @@ struct i915_gem_context {
 		u32 *lrc_reg_state;
 		u64 lrc_desc;
 		int pin_count;
-		bool initialised;
 	} engine[I915_NUM_ENGINES];
 
 	/** ring_size: size for allocating the per-engine ring buffer */
@@ -292,6 +291,9 @@ int i915_gem_context_setparam_ioctl(struct drm_device *dev, void *data,
 int i915_gem_context_reset_stats_ioctl(struct drm_device *dev, void *data,
 				       struct drm_file *file);
 
+struct i915_gem_context *
+i915_gem_context_create_kernel(struct drm_i915_private *i915, int prio);
+
 static inline struct i915_gem_context *
 i915_gem_context_get(struct i915_gem_context *ctx)
 {
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 047fca656b4d..492cd9f25056 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -687,6 +687,9 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	intel_engine_cleanup_cmd_parser(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
 
+	if (engine->default_state)
+		i915_gem_object_put(engine->default_state);
+
 	if (HAS_LOGICAL_RING_PREEMPTION(engine->i915))
 		engine->context_unpin(engine, engine->i915->preempt_context);
 	engine->context_unpin(engine, engine->i915->kernel_context);
@@ -1701,6 +1704,20 @@ bool intel_engine_can_store_dword(struct intel_engine_cs *engine)
 	}
 }
 
+unsigned int intel_engines_has_context_isolation(struct drm_i915_private *i915)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	unsigned int which;
+
+	which = 0;
+	for_each_engine(engine, i915, id)
+		if (engine->default_state)
+			which |= BIT(engine->uabi_class);
+
+	return which;
+}
+
 static void print_request(struct drm_printer *m,
 			  struct drm_i915_gem_request *rq,
 			  const char *prefix)
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 9b4e74151ace..91115448b96f 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -1163,7 +1163,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	struct intel_engine_cs *engine = request->engine;
 	struct intel_context *ce = &request->ctx->engine[engine->id];
 	u32 *cs;
-	int ret;
 
 	GEM_BUG_ON(!ce->pin_count);
 
@@ -1177,14 +1176,6 @@ static int execlists_request_alloc(struct drm_i915_gem_request *request)
 	if (IS_ERR(cs))
 		return PTR_ERR(cs);
 
-	if (!ce->initialised) {
-		ret = engine->init_context(request);
-		if (ret)
-			return ret;
-
-		ce->initialised = true;
-	}
-
 	/* Note that after this point, we have committed to using
 	 * this request as it is being used to both track the
 	 * state of engine initialisation and liveness of the
@@ -2116,7 +2107,6 @@ static void execlists_init_reg_state(u32 *regs,
 
 	CTX_REG(regs, CTX_CONTEXT_CONTROL, RING_CONTEXT_CONTROL(engine),
 		_MASKED_BIT_ENABLE(CTX_CTRL_INHIBIT_SYN_CTX_SWITCH |
-				   CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT |
 				   (HAS_RESOURCE_STREAMER(dev_priv) ?
 				   CTX_CTRL_RS_CTX_ENABLE : 0)));
 	CTX_REG(regs, CTX_RING_HEAD, RING_HEAD(base), 0);
@@ -2193,6 +2183,7 @@ populate_lr_context(struct i915_gem_context *ctx,
 		    struct intel_ring *ring)
 {
 	void *vaddr;
+	u32 *regs;
 	int ret;
 
 	ret = i915_gem_object_set_to_cpu_domain(ctx_obj, true);
@@ -2209,11 +2200,32 @@ populate_lr_context(struct i915_gem_context *ctx,
 	}
 	ctx_obj->mm.dirty = true;
 
+	if (engine->default_state) {
+		/*
+		 * We only want to copy over the template context state;
+		 * skipping over the headers reserved for GuC comunication,
+		 * leaving those as zero.
+		 */
+		const unsigned long start = LRC_HEADER_PAGES * PAGE_SIZE;
+		void *defaults;
+
+		defaults = i915_gem_object_pin_map(engine->default_state,
+						   I915_MAP_WB);
+		if (IS_ERR(defaults))
+			return PTR_ERR(defaults);
+
+		memcpy(vaddr + start, defaults + start, engine->context_size);
+		i915_gem_object_unpin_map(engine->default_state);
+	}
+
 	/* The second page of the context object contains some fields which must
 	 * be set up prior to the first execution. */
-
-	execlists_init_reg_state(vaddr + LRC_STATE_PN * PAGE_SIZE,
-				 ctx, engine, ring);
+	regs = vaddr + LRC_STATE_PN * PAGE_SIZE;
+	execlists_init_reg_state(regs, ctx, engine, ring);
+	if (!engine->default_state) {
+		regs[CTX_CONTEXT_CONTROL+1] |=
+			_MASKED_BIT_ENABLE(CTX_CTRL_ENGINE_CTX_RESTORE_INHIBIT);
+	}
 
 	i915_gem_object_unpin_map(ctx_obj);
 
@@ -2266,7 +2278,6 @@ static int execlists_context_deferred_alloc(struct i915_gem_context *ctx,
 
 	ce->ring = ring;
 	ce->state = vma;
-	ce->initialised |= engine->init_context == NULL;
 
 	return 0;
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 7e2a671882fb..464dc58af27b 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -1384,11 +1384,34 @@ alloc_context_vma(struct intel_engine_cs *engine)
 	struct drm_i915_private *i915 = engine->i915;
 	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
+	int err;
 
 	obj = i915_gem_object_create(i915, engine->context_size);
 	if (IS_ERR(obj))
 		return ERR_CAST(obj);
 
+	if (engine->default_state) {
+		void *defaults, *vaddr;
+
+		vaddr = i915_gem_object_pin_map(obj, I915_MAP_WB);
+		if (IS_ERR(vaddr)) {
+			err = PTR_ERR(vaddr);
+			goto err_obj;
+		}
+
+		defaults = i915_gem_object_pin_map(engine->default_state,
+						   I915_MAP_WB);
+		if (IS_ERR(defaults)) {
+			err = PTR_ERR(defaults);
+			goto err_map;
+		}
+
+		memcpy(vaddr, defaults, engine->context_size);
+
+		i915_gem_object_unpin_map(engine->default_state);
+		i915_gem_object_unpin_map(obj);
+	}
+
 	/*
 	 * Try to make the context utilize L3 as well as LLC.
 	 *
@@ -1410,10 +1433,18 @@ alloc_context_vma(struct intel_engine_cs *engine)
 	}
 
 	vma = i915_vma_instance(obj, &i915->ggtt.base, NULL);
-	if (IS_ERR(vma))
-		i915_gem_object_put(obj);
+	if (IS_ERR(vma)) {
+		err = PTR_ERR(vma);
+		goto err_obj;
+	}
 
 	return vma;
+
+err_map:
+	i915_gem_object_unpin_map(obj);
+err_obj:
+	i915_gem_object_put(obj);
+	return ERR_PTR(err);
 }
 
 static struct intel_ring *
@@ -1449,16 +1480,6 @@ intel_ring_context_pin(struct intel_engine_cs *engine,
 		ce->state->obj->pin_global++;
 	}
 
-	/* The kernel context is only used as a placeholder for flushing the
-	 * active context. It is never used for submitting user rendering and
-	 * as such never requires the golden render context, and so we can skip
-	 * emitting it when we switch to the kernel context. This is required
-	 * as during eviction we cannot allocate and pin the renderstate in
-	 * order to initialise the context.
-	 */
-	if (i915_gem_context_is_kernel(ctx))
-		ce->initialised = true;
-
 	i915_gem_context_get(ctx);
 
 out:
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 7d3903b9fb1d..bef16d8ed03a 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -307,6 +307,7 @@ struct intel_engine_cs {
 	struct intel_ring *buffer;
 	struct intel_timeline *timeline;
 
+	struct drm_i915_gem_object *default_state;
 	struct intel_render_state *render_state;
 
 	atomic_t irq_count;
@@ -933,6 +934,7 @@ void intel_engines_park(struct drm_i915_private *i915);
 void intel_engines_unpark(struct drm_i915_private *i915);
 
 void intel_engines_reset_default_submission(struct drm_i915_private *i915);
+unsigned int intel_engines_has_context_isolation(struct drm_i915_private *i915);
 
 bool intel_engine_can_store_dword(struct intel_engine_cs *engine);
 
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index 65d06da62599..8492ebd79562 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -465,6 +465,21 @@ typedef struct drm_i915_irq_wait {
  */
 #define I915_PARAM_HAS_EXEC_FENCE_ARRAY  49
 
+/*
+ * Query whether every context (both per-file default and user created) is
+ * isolated (insofar as HW supports). If this parameter is not true, then
+ * freshly created contexts may inherit values from an existing context,
+ * rather than default HW values. If true, it also ensures (insofar as HW
+ * supports) that all state set by this context will not leak to any other
+ * context.
+ *
+ * As not every engine across every gen support contexts, the returned
+ * value reports the support of context isolation for individual engines by
+ * returning a bitmask of each engine class set to true if that class supports
+ * isolation.
+ */
+#define I915_PARAM_HAS_CONTEXT_ISOLATION 50
+
 typedef struct drm_i915_getparam {
 	__s32 param;
 	/*
-- 
2.15.0

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

[PATCH v5 9/9] drm/i915: Stop caching the "golden" renderstate

[Chris Wilson]

As we now record the default HW state and so only emit the "golden"
renderstate once to prepare the HW, there is no advantage in keeping the
renderstate batch around as it will never be used again.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h              |   1 -
 drivers/gpu/drm/i915/i915_gem_render_state.c | 141 +++++++++------------------
 drivers/gpu/drm/i915/i915_gem_render_state.h |   4 +-
 drivers/gpu/drm/i915/intel_engine_cs.c       |   9 +-
 drivers/gpu/drm/i915/intel_lrc.c             |   1 +
 drivers/gpu/drm/i915/intel_ringbuffer.c      |   5 +-
 drivers/gpu/drm/i915/intel_ringbuffer.h      |   2 -
 7 files changed, 54 insertions(+), 109 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7596dc1e1346..f8eb247ce9d1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -67,7 +67,6 @@
 #include "i915_gem_fence_reg.h"
 #include "i915_gem_object.h"
 #include "i915_gem_gtt.h"
-#include "i915_gem_render_state.h"
 #include "i915_gem_request.h"
 #include "i915_gem_timeline.h"
 
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.c b/drivers/gpu/drm/i915/i915_gem_render_state.c
index 3703dc91eeda..c2723a06fbb4 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.c
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.c
@@ -26,10 +26,12 @@
  */
 
 #include "i915_drv.h"
+#include "i915_gem_render_state.h"
 #include "intel_renderstate.h"
 
 struct intel_render_state {
 	const struct intel_renderstate_rodata *rodata;
+	struct drm_i915_gem_object *obj;
 	struct i915_vma *vma;
 	u32 batch_offset;
 	u32 batch_size;
@@ -40,6 +42,9 @@ struct intel_render_state {
 static const struct intel_renderstate_rodata *
 render_state_get_rodata(const struct intel_engine_cs *engine)
 {
+	if (engine->id != RCS)
+		return NULL;
+
 	switch (INTEL_GEN(engine->i915)) {
 	case 6:
 		return &gen6_null_state;
@@ -74,17 +79,16 @@ static int render_state_setup(struct intel_render_state *so,
 			      struct drm_i915_private *i915)
 {
 	const struct intel_renderstate_rodata *rodata = so->rodata;
-	struct drm_i915_gem_object *obj = so->vma->obj;
 	unsigned int i = 0, reloc_index = 0;
 	unsigned int needs_clflush;
 	u32 *d;
 	int ret;
 
-	ret = i915_gem_obj_prepare_shmem_write(obj, &needs_clflush);
+	ret = i915_gem_obj_prepare_shmem_write(so->obj, &needs_clflush);
 	if (ret)
 		return ret;
 
-	d = kmap_atomic(i915_gem_object_get_dirty_page(obj, 0));
+	d = kmap_atomic(i915_gem_object_get_dirty_page(so->obj, 0));
 
 	while (i < rodata->batch_items) {
 		u32 s = rodata->batch[i];
@@ -112,7 +116,7 @@ static int render_state_setup(struct intel_render_state *so,
 		goto err;
 	}
 
-	so->batch_offset = so->vma->node.start;
+	so->batch_offset = i915_ggtt_offset(so->vma);
 	so->batch_size = rodata->batch_items * sizeof(u32);
 
 	while (i % CACHELINE_DWORDS)
@@ -160,9 +164,9 @@ static int render_state_setup(struct intel_render_state *so,
 		drm_clflush_virt_range(d, i * sizeof(u32));
 	kunmap_atomic(d);
 
-	ret = i915_gem_object_set_to_gtt_domain(obj, false);
+	ret = i915_gem_object_set_to_gtt_domain(so->obj, false);
 out:
-	i915_gem_obj_finish_shmem_access(obj);
+	i915_gem_obj_finish_shmem_access(so->obj);
 	return ret;
 
 err:
@@ -173,112 +177,61 @@ static int render_state_setup(struct intel_render_state *so,
 
 #undef OUT_BATCH
 
-int i915_gem_render_state_init(struct intel_engine_cs *engine)
+int i915_gem_render_state_emit(struct drm_i915_gem_request *rq)
 {
-	struct intel_render_state *so;
-	const struct intel_renderstate_rodata *rodata;
-	struct drm_i915_gem_object *obj;
-	int ret;
+	struct intel_engine_cs *engine = rq->engine;
+	struct intel_render_state so = {}; /* keep the compiler happy */
+	int err;
 
-	if (engine->id != RCS)
+	so.rodata = render_state_get_rodata(engine);
+	if (!so.rodata)
 		return 0;
 
-	rodata = render_state_get_rodata(engine);
-	if (!rodata)
-		return 0;
-
-	if (rodata->batch_items * 4 > PAGE_SIZE)
+	if (so.rodata->batch_items * 4 > PAGE_SIZE)
 		return -EINVAL;
 
-	so = kmalloc(sizeof(*so), GFP_KERNEL);
-	if (!so)
-		return -ENOMEM;
-
-	obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
-	if (IS_ERR(obj)) {
-		ret = PTR_ERR(obj);
-		goto err_free;
-	}
+	so.obj = i915_gem_object_create_internal(engine->i915, PAGE_SIZE);
+	if (IS_ERR(so.obj))
+		return PTR_ERR(so.obj);
 
-	so->vma = i915_vma_instance(obj, &engine->i915->ggtt.base, NULL);
-	if (IS_ERR(so->vma)) {
-		ret = PTR_ERR(so->vma);
+	so.vma = i915_vma_instance(so.obj, &engine->i915->ggtt.base, NULL);
+	if (IS_ERR(so.vma)) {
+		err = PTR_ERR(so.vma);
 		goto err_obj;
 	}
 
-	so->rodata = rodata;
-	engine->render_state = so;
-	return 0;
-
-err_obj:
-	i915_gem_object_put(obj);
-err_free:
-	kfree(so);
-	return ret;
-}
-
-int i915_gem_render_state_emit(struct drm_i915_gem_request *req)
-{
-	struct intel_render_state *so;
-	int ret;
-
-	lockdep_assert_held(&req->i915->drm.struct_mutex);
-
-	so = req->engine->render_state;
-	if (!so)
-		return 0;
-
-	/* Recreate the page after shrinking */
-	if (!i915_gem_object_has_pages(so->vma->obj))
-		so->batch_offset = -1;
-
-	ret = i915_vma_pin(so->vma, 0, 0, PIN_GLOBAL | PIN_HIGH);
-	if (ret)
-		return ret;
+	err = i915_vma_pin(so.vma, 0, 0, PIN_GLOBAL | PIN_HIGH);
+	if (err)
+		goto err_vma;
 
-	if (so->vma->node.start != so->batch_offset) {
-		ret = render_state_setup(so, req->i915);
-		if (ret)
-			goto err_unpin;
-	}
+	err = render_state_setup(&so, rq->i915);
+	if (err)
+		goto err_unpin;
 
-	ret = req->engine->emit_flush(req, EMIT_INVALIDATE);
-	if (ret)
+	err = engine->emit_flush(rq, EMIT_INVALIDATE);
+	if (err)
 		goto err_unpin;
 
-	ret = req->engine->emit_bb_start(req,
-					 so->batch_offset, so->batch_size,
-					 I915_DISPATCH_SECURE);
-	if (ret)
+	err = engine->emit_bb_start(rq,
+				    so.batch_offset, so.batch_size,
+				    I915_DISPATCH_SECURE);
+	if (err)
 		goto err_unpin;
 
-	if (so->aux_size > 8) {
-		ret = req->engine->emit_bb_start(req,
-						 so->aux_offset, so->aux_size,
-						 I915_DISPATCH_SECURE);
-		if (ret)
+	if (so.aux_size > 8) {
+		err = engine->emit_bb_start(rq,
+					    so.aux_offset, so.aux_size,
+					    I915_DISPATCH_SECURE);
+		if (err)
 			goto err_unpin;
 	}
 
-	i915_vma_move_to_active(so->vma, req, 0);
+	i915_vma_move_to_active(so.vma, rq, 0);
 err_unpin:
-	i915_vma_unpin(so->vma);
-	return ret;
-}
-
-void i915_gem_render_state_fini(struct intel_engine_cs *engine)
-{
-	struct intel_render_state *so;
-	struct drm_i915_gem_object *obj;
-
-	so = fetch_and_zero(&engine->render_state);
-	if (!so)
-		return;
-
-	obj = so->vma->obj;
-
-	i915_vma_close(so->vma);
-	__i915_gem_object_release_unless_active(obj);
-
-	kfree(so);
+	i915_vma_unpin(so.vma);
+err_vma:
+	i915_vma_close(so.vma);
+err_obj:
+	__i915_gem_object_release_unless_active(so.obj);
+	return err;
 }
diff --git a/drivers/gpu/drm/i915/i915_gem_render_state.h b/drivers/gpu/drm/i915/i915_gem_render_state.h
index 87481845799d..86369520482e 100644
--- a/drivers/gpu/drm/i915/i915_gem_render_state.h
+++ b/drivers/gpu/drm/i915/i915_gem_render_state.h
@@ -26,8 +26,6 @@
 
 struct drm_i915_gem_request;
 
-int i915_gem_render_state_init(struct intel_engine_cs *engine);
-int i915_gem_render_state_emit(struct drm_i915_gem_request *req);
-void i915_gem_render_state_fini(struct intel_engine_cs *engine);
+int i915_gem_render_state_emit(struct drm_i915_gem_request *rq);
 
 #endif /* _I915_GEM_RENDER_STATE_H_ */
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 492cd9f25056..81f8f7a4121b 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -641,21 +641,15 @@ int intel_engine_init_common(struct intel_engine_cs *engine)
 	if (ret)
 		goto err_unpin_preempt;
 
-	ret = i915_gem_render_state_init(engine);
-	if (ret)
-		goto err_breadcrumbs;
-
 	if (HWS_NEEDS_PHYSICAL(engine->i915))
 		ret = init_phys_status_page(engine);
 	else
 		ret = init_status_page(engine);
 	if (ret)
-		goto err_rs_fini;
+		goto err_breadcrumbs;
 
 	return 0;
 
-err_rs_fini:
-	i915_gem_render_state_fini(engine);
 err_breadcrumbs:
 	intel_engine_fini_breadcrumbs(engine);
 err_unpin_preempt:
@@ -682,7 +676,6 @@ void intel_engine_cleanup_common(struct intel_engine_cs *engine)
 	else
 		cleanup_status_page(engine);
 
-	i915_gem_render_state_fini(engine);
 	intel_engine_fini_breadcrumbs(engine);
 	intel_engine_cleanup_cmd_parser(engine);
 	i915_gem_batch_pool_fini(&engine->batch_pool);
diff --git a/drivers/gpu/drm/i915/intel_lrc.c b/drivers/gpu/drm/i915/intel_lrc.c
index 91115448b96f..4f56ff5cb908 100644
--- a/drivers/gpu/drm/i915/intel_lrc.c
+++ b/drivers/gpu/drm/i915/intel_lrc.c
@@ -136,6 +136,7 @@
 #include <drm/drmP.h>
 #include <drm/i915_drm.h>
 #include "i915_drv.h"
+#include "i915_gem_render_state.h"
 #include "intel_mocs.h"
 
 #define RING_EXECLIST_QFULL		(1 << 0x2)
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c
index 464dc58af27b..3321b801e77d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.c
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.c
@@ -28,9 +28,12 @@
  */
 
 #include <linux/log2.h>
+
 #include <drm/drmP.h>
-#include "i915_drv.h"
 #include <drm/i915_drm.h>
+
+#include "i915_drv.h"
+#include "i915_gem_render_state.h"
 #include "i915_trace.h"
 #include "intel_drv.h"
 
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index bef16d8ed03a..2b0ae5468f48 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -166,7 +166,6 @@ struct i915_ctx_workarounds {
 };
 
 struct drm_i915_gem_request;
-struct intel_render_state;
 
 /*
  * Engine IDs definitions.
@@ -308,7 +307,6 @@ struct intel_engine_cs {
 	struct intel_timeline *timeline;
 
 	struct drm_i915_gem_object *default_state;
-	struct intel_render_state *render_state;
 
 	atomic_t irq_count;
 	unsigned long irq_posted;
-- 
2.15.0

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

Re: [PATCH v5 5/9] drm/i915: Move intel_init_clock_gating() to i915_gem_init()

[Ville Syrjälä]

On Wed, Nov 08, 2017 at 07:14:58PM +0000, Chris Wilson wrote:
> Despite its name intel_init_clock_gating applies both display clock gating
> workarounds; GT mmio workarounds and the occasional GT power context
> workaround. Worse, sometimes it includes a context register workaround
> which we need to apply before we record the default HW state for all
> contexts.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c      | 16 ++++++++++++++--
>  drivers/gpu/drm/i915/intel_display.c |  2 --
>  2 files changed, 14 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 37586f703c1e..5c0fc78de853 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5024,6 +5024,20 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>  	intel_init_gt_powersave(dev_priv);
>  
>  	ret = i915_gem_init_hw(dev_priv);
> +	if (ret)
> +		goto out_unlock;
> +
> +	/* Despite its name intel_init_clock_gating applies both display
> +	 * clock gating workarounds; GT mmio workarounds and the occasional
> +	 * GT power context workaround. Worse, sometimes it includes a context
> +	 * register workaround which we need to apply before we record the
> +	 * default HW state for all contexts.
> +	 *
> +	 * FIXME: break up the workarounds and apply them at the right time!
> +	 */
> +	intel_init_clock_gating(dev_priv);

This part lgtm.

> +
> +out_unlock:
>  	if (ret == -EIO) {
>  		/* Allow engine initialisation to fail by marking the GPU as
>  		 * wedged. But we only want to do this where the GPU is angry,
> @@ -5035,8 +5049,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>  		}
>  		ret = 0;
>  	}
> -
> -out_unlock:

Is the movement of the label going to adversely affect error handling for
the other functions which already use the label? Or none of them can
return -EIO?

>  	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
>  	mutex_unlock(&dev_priv->drm.struct_mutex);
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index bc1c6ccde7db..618e912fcaec 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -15197,8 +15197,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(dev);
>  
> -	intel_init_clock_gating(dev_priv);
> -
>  	intel_setup_overlay(dev_priv);
>  }
>  
> -- 
> 2.15.0

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 5/9] drm/i915: Move intel_init_clock_gating() to i915_gem_init()

[Chris Wilson]

Quoting Ville Syrjälä (2017-11-08 19:27:41)
> On Wed, Nov 08, 2017 at 07:14:58PM +0000, Chris Wilson wrote:
> > +out_unlock:
> >       if (ret == -EIO) {
> >               /* Allow engine initialisation to fail by marking the GPU as
> >                * wedged. But we only want to do this where the GPU is angry,
> > @@ -5035,8 +5049,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> >               }
> >               ret = 0;
> >       }
> > -
> > -out_unlock:
> 
> Is the movement of the label going to adversely affect error handling for
> the other functions which already use the label? Or none of them can
> return -EIO?

None of those can return -EIO, but if they did the same principle
applies to them. We would much prefer to disable GEM submission than
disable the driver; the theory being as always if the user can see the
display and the error messages, they can report a bug. If the driver
dies in the middle of loading, it's likely they won't be able to see
anything at all (since we've already kicked out the VGA console and it
is not coming back).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 5/9] drm/i915: Move intel_init_clock_gating() to i915_gem_init()

[Ville Syrjälä]

On Wed, Nov 08, 2017 at 07:33:05PM +0000, Chris Wilson wrote:
> Quoting Ville Syrjälä (2017-11-08 19:27:41)
> > On Wed, Nov 08, 2017 at 07:14:58PM +0000, Chris Wilson wrote:
> > > +out_unlock:
> > >       if (ret == -EIO) {
> > >               /* Allow engine initialisation to fail by marking the GPU as
> > >                * wedged. But we only want to do this where the GPU is angry,
> > > @@ -5035,8 +5049,6 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
> > >               }
> > >               ret = 0;
> > >       }
> > > -
> > > -out_unlock:
> > 
> > Is the movement of the label going to adversely affect error handling for
> > the other functions which already use the label? Or none of them can
> > return -EIO?
> 
> None of those can return -EIO, but if they did the same principle
> applies to them. We would much prefer to disable GEM submission than
> disable the driver; the theory being as always if the user can see the
> display and the error messages, they can report a bug. If the driver
> dies in the middle of loading, it's likely they won't be able to see
> anything at all (since we've already kicked out the VGA console and it
> is not coming back).

Ack.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.BAT: failure for series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno
URL   : https://patchwork.freedesktop.org/series/33452/
State : failure

== Summary ==

  CHK     include/config/kernel.release
  CHK     include/generated/uapi/linux/version.h
  CHK     include/generated/utsrelease.h
  CHK     include/generated/bounds.h
  CHK     include/generated/timeconst.h
  CHK     include/generated/asm-offsets.h
  CALL    scripts/checksyscalls.sh
  CHK     scripts/mod/devicetable-offsets.h
  CHK     include/generated/compile.h
  CHK     kernel/config_data.h
  CC [M]  drivers/gpu/drm/i915/intel_display.o
drivers/gpu/drm/i915/intel_display.c: In function ‘intel_modeset_gem_init’:
drivers/gpu/drm/i915/intel_display.c:15192:1: error: version control conflict marker in file
 <<<<<<< HEAD
 ^~~~~~~
drivers/gpu/drm/i915/intel_display.c:15194:1: error: version control conflict marker in file
 =======
 ^~~~~~~
drivers/gpu/drm/i915/intel_display.c:15196:1: error: version control conflict marker in file
 >>>>>>> drm/i915: Move GT powersaving init to i915_gem_init()
 ^~~~~~~
drivers/gpu/drm/i915/intel_display.c:15190:27: error: unused variable ‘dev_priv’ [-Werror=unused-variable]
  struct drm_i915_private *dev_priv = to_i915(dev);
                           ^~~~~~~~
cc1: all warnings being treated as errors
scripts/Makefile.build:314: recipe for target 'drivers/gpu/drm/i915/intel_display.o' failed
make[4]: *** [drivers/gpu/drm/i915/intel_display.o] Error 1
scripts/Makefile.build:573: recipe for target 'drivers/gpu/drm/i915' failed
make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:573: recipe for target 'drivers/gpu/drm' failed
make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:573: recipe for target 'drivers/gpu' failed
make[1]: *** [drivers/gpu] Error 2
Makefile:1024: recipe for target 'drivers' failed
make: *** [drivers] Error 2

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

Re: ✗ Fi.CI.BAT: failure for series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Chris Wilson]

Quoting Patchwork (2017-11-08 19:45:20)
> == Series Details ==
> 
> Series: series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno
> URL   : https://patchwork.freedesktop.org/series/33452/
> State : failure
> 
> == Summary ==
> 
>   CHK     include/config/kernel.release
>   CHK     include/generated/uapi/linux/version.h
>   CHK     include/generated/utsrelease.h
>   CHK     include/generated/bounds.h
>   CHK     include/generated/timeconst.h
>   CHK     include/generated/asm-offsets.h
>   CALL    scripts/checksyscalls.sh
>   CHK     scripts/mod/devicetable-offsets.h
>   CHK     include/generated/compile.h
>   CHK     kernel/config_data.h
>   CC [M]  drivers/gpu/drm/i915/intel_display.o
> drivers/gpu/drm/i915/intel_display.c: In function ‘intel_modeset_gem_init’:
> drivers/gpu/drm/i915/intel_display.c:15192:1: error: version control conflict marker in file
>  <<<<<<< HEAD
>  ^~~~~~~
> drivers/gpu/drm/i915/intel_display.c:15194:1: error: version control conflict marker in file
>  =======
>  ^~~~~~~
> drivers/gpu/drm/i915/intel_display.c:15196:1: error: version control conflict marker in file
>  >>>>>>> drm/i915: Move GT powersaving init to i915_gem_init()
>  ^~~~~~~
> drivers/gpu/drm/i915/intel_display.c:15190:27: error: unused variable ‘dev_priv’ [-Werror=unused-variable]
>   struct drm_i915_private *dev_priv = to_i915(dev);
>                            ^~~~~~~~

So if we are not meant to be sending patches based on drm-tip, what tree
should we be using? >:
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Lionel Landwerlin]

On 08/11/17 19:14, Chris Wilson wrote:
>   
> +/*
> + * Different engines serve different roles, and there may be more than one
> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> + * classification of the role of the engine, which may be used when requesting
> + * operations to be performed on a certain subset of engines, or for providing
> + * information about that group.
> + */
> +enum drm_i915_gem_engine_class {
> +	I915_ENGINE_CLASS_OTHER = 0,
> +	I915_ENGINE_CLASS_RENDER = 1,
> +	I915_ENGINE_CLASS_COPY = 2,
> +	I915_ENGINE_CLASS_VIDEO = 3,
> +	I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> +};
> +
I've tried to build a bit UI in GPUTop to show this.
I'm a bit skeptical about the OTHER type because if this enum is meant 
to be extended, then why do we need an OTHER class? We should create new 
classes instead.

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

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Tvrtko Ursulin]

On 09/11/2017 09:13, Lionel Landwerlin wrote:
> On 08/11/17 19:14, Chris Wilson wrote:
>> +/*
>> + * Different engines serve different roles, and there may be more 
>> than one
>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
>> + * classification of the role of the engine, which may be used when 
>> requesting
>> + * operations to be performed on a certain subset of engines, or for 
>> providing
>> + * information about that group.
>> + */
>> +enum drm_i915_gem_engine_class {
>> +    I915_ENGINE_CLASS_OTHER = 0,
>> +    I915_ENGINE_CLASS_RENDER = 1,
>> +    I915_ENGINE_CLASS_COPY = 2,
>> +    I915_ENGINE_CLASS_VIDEO = 3,
>> +    I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
>> +};
>> +
> I've tried to build a bit UI in GPUTop to show this.
> I'm a bit skeptical about the OTHER type because if this enum is meant 
> to be extended, then why do we need an OTHER class? We should create new 
> classes instead.

Good point, I agree that I cannot find a reason why we would have it in 
the uAPI. I suspect I was just doing a copy-paste-transform of the 
hardware definitions from i915_reg.h.

Regards,

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

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-09 09:27:33)
> 
> On 09/11/2017 09:13, Lionel Landwerlin wrote:
> > On 08/11/17 19:14, Chris Wilson wrote:
> >> +/*
> >> + * Different engines serve different roles, and there may be more 
> >> than one
> >> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> >> + * classification of the role of the engine, which may be used when 
> >> requesting
> >> + * operations to be performed on a certain subset of engines, or for 
> >> providing
> >> + * information about that group.
> >> + */
> >> +enum drm_i915_gem_engine_class {
> >> +    I915_ENGINE_CLASS_OTHER = 0,
> >> +    I915_ENGINE_CLASS_RENDER = 1,
> >> +    I915_ENGINE_CLASS_COPY = 2,
> >> +    I915_ENGINE_CLASS_VIDEO = 3,
> >> +    I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> >> +};
> >> +
> > I've tried to build a bit UI in GPUTop to show this.
> > I'm a bit skeptical about the OTHER type because if this enum is meant 
> > to be extended, then why do we need an OTHER class? We should create new 
> > classes instead.
> 
> Good point, I agree that I cannot find a reason why we would have it in 
> the uAPI. I suspect I was just doing a copy-paste-transform of the 
> hardware definitions from i915_reg.h.

Do we want to keep 0 as undefined? It can be both a nuisance and a
blessing...
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Tvrtko Ursulin]

On 09/11/2017 09:37, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-09 09:27:33)
>>
>> On 09/11/2017 09:13, Lionel Landwerlin wrote:
>>> On 08/11/17 19:14, Chris Wilson wrote:
>>>> +/*
>>>> + * Different engines serve different roles, and there may be more
>>>> than one
>>>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
>>>> + * classification of the role of the engine, which may be used when
>>>> requesting
>>>> + * operations to be performed on a certain subset of engines, or for
>>>> providing
>>>> + * information about that group.
>>>> + */
>>>> +enum drm_i915_gem_engine_class {
>>>> +    I915_ENGINE_CLASS_OTHER = 0,
>>>> +    I915_ENGINE_CLASS_RENDER = 1,
>>>> +    I915_ENGINE_CLASS_COPY = 2,
>>>> +    I915_ENGINE_CLASS_VIDEO = 3,
>>>> +    I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
>>>> +};
>>>> +
>>> I've tried to build a bit UI in GPUTop to show this.
>>> I'm a bit skeptical about the OTHER type because if this enum is meant
>>> to be extended, then why do we need an OTHER class? We should create new
>>> classes instead.
>>
>> Good point, I agree that I cannot find a reason why we would have it in
>> the uAPI. I suspect I was just doing a copy-paste-transform of the
>> hardware definitions from i915_reg.h.
> 
> Do we want to keep 0 as undefined? It can be both a nuisance and a
> blessing...

What can you imagine it would be useful for?

Regards,

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

✗ Fi.CI.BAT: failure for series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno
URL   : https://patchwork.freedesktop.org/series/33452/
State : failure

== Summary ==

Series 33452v1 series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno
https://patchwork.freedesktop.org/api/1.0/series/33452/revisions/1/mbox/

Test gem_ctx_basic:
                fail       -> SKIP       (fi-gdg-551)
Test gem_exec_reloc:
        Subgroup basic-cpu-active:
                fail       -> PASS       (fi-gdg-551) fdo#102582 +4
Test gem_ringfill:
        Subgroup basic-default-hang:
                pass       -> INCOMPLETE (fi-byt-n2820)
Test gem_sync:
        Subgroup basic-store-all:
                fail       -> PASS       (fi-ivb-3520m) fdo#100007
Test kms_busy:
        Subgroup basic-flip-a:
                pass       -> FAIL       (fi-gdg-551) fdo#102654 +1
Test kms_cursor_legacy:
        Subgroup basic-busy-flip-before-cursor-legacy:
                pass       -> FAIL       (fi-gdg-551) fdo#102618
        Subgroup basic-flip-after-cursor-legacy:
                pass       -> SKIP       (fi-hsw-4770r)

fdo#102582 https://bugs.freedesktop.org/show_bug.cgi?id=102582
fdo#100007 https://bugs.freedesktop.org/show_bug.cgi?id=100007
fdo#102654 https://bugs.freedesktop.org/show_bug.cgi?id=102654
fdo#102618 https://bugs.freedesktop.org/show_bug.cgi?id=102618

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:444s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:457s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:381s
fi-bsw-n3050     total:289  pass:243  dwarn:0   dfail:0   fail:0   skip:46  time:542s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:273s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:500s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:501s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:496s
fi-byt-n2820     total:156  pass:133  dwarn:0   dfail:0   fail:0   skip:22 
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:428s
fi-gdg-551       total:289  pass:175  dwarn:1   dfail:0   fail:4   skip:109 time:277s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:538s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-hsw-4770r     total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:436s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:425s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:481s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:461s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:483s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:515s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:482s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:534s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:583s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:453s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:549s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:563s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:522s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:492s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:458s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:563s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:418s
Blacklisted hosts:
fi-cnl-y         total:289  pass:260  dwarn:0   dfail:0   fail:2   skip:27  time:567s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:490s

65dc54b704d3ee0486f9f5b11f00c28973f783a2 drm-tip: 2017y-11m-09d-08h-53m-46s UTC integration manifest
0c8c4a6a3f63 drm/i915: Stop caching the "golden" renderstate
528a4ce78ed7 drm/i915: Record the default hw state after reset upon load
1106020bc009 drm/i915: Mark the context state as dirty/written
619ba0893cd9 drm/i915: Inline intel_modeset_gem_init()
50252cc19f5c drm/i915: Move intel_init_clock_gating() to i915_gem_init()
ccae92ebc55d drm/i915: Move GT powersaving init to i915_gem_init()
161777f07799 drm/i915: Force the switch to the i915->kernel_context
0695687350ee drm/i915: Define an engine class enum for the uABI
5282596b7c4c drm/i915: Include engine state on detecting a missed breadcrumb/seqno

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7029/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Mika Kuoppala]

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

> Now that we have a common engine state pretty printer, we can use that
> instead of the adhoc information printed when we miss a breadcrumb.
>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_breadcrumbs.c | 11 +++++------
>  drivers/gpu/drm/i915/intel_engine_cs.c   |  6 ++++++
>  2 files changed, 11 insertions(+), 6 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> index 4de054f8c1ba..1e4d2978fb86 100644
> --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> @@ -64,12 +64,11 @@ static unsigned long wait_timeout(void)
>  
>  static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
>  {
> -	DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS, irq posted? %s, current seqno=%x, last=%x\n",
> -			 engine->name, __builtin_return_address(0),
> -			 yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
> -					&engine->irq_posted)),
> -			 intel_engine_get_seqno(engine),
> -			 intel_engine_last_submit(engine));
> +	struct drm_printer p = drm_debug_printer(__func__);
> +
> +	DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS\n",
> +			 engine->name, __builtin_return_address(0));
> +	intel_engine_dump(engine, &p);

We are holding rb_lock while we enter and the dump will retake it.

We also should add proper asserts to intel_engine_dump that we notice
immediately if we try to dump on wrong context.

-Mika

>  
>  	set_bit(engine->id, &engine->i915->gpu_error.missed_irq_rings);
>  }
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 6997306be0d2..f22dc452030f 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -1835,6 +1835,12 @@ void intel_engine_dump(struct intel_engine_cs *engine, struct drm_printer *m)
>  	}
>  	spin_unlock_irq(&b->rb_lock);
>  
> +	drm_printf(m, "IRQ? 0x%lx (breadcrumbs? %s) (execlists? %s)\n",
> +		   engine->irq_posted,
> +		   yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
> +				  &engine->irq_posted)),
> +		   yesno(test_bit(ENGINE_IRQ_EXECLIST,
> +				  &engine->irq_posted)));
>  	drm_printf(m, "Idle? %s\n", yesno(intel_engine_is_idle(engine)));
>  	drm_printf(m, "\n");
>  }
> -- 
> 2.15.0
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno

[Chris Wilson]

Quoting Mika Kuoppala (2017-11-09 11:03:39)
> Chris Wilson <chris@chris-wilson.co.uk> writes:
> 
> > Now that we have a common engine state pretty printer, we can use that
> > instead of the adhoc information printed when we miss a breadcrumb.
> >
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Cc: Mika Kuoppala <mika.kuoppala@intel.com>
> > Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_breadcrumbs.c | 11 +++++------
> >  drivers/gpu/drm/i915/intel_engine_cs.c   |  6 ++++++
> >  2 files changed, 11 insertions(+), 6 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > index 4de054f8c1ba..1e4d2978fb86 100644
> > --- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > +++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
> > @@ -64,12 +64,11 @@ static unsigned long wait_timeout(void)
> >  
> >  static noinline void missed_breadcrumb(struct intel_engine_cs *engine)
> >  {
> > -     DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS, irq posted? %s, current seqno=%x, last=%x\n",
> > -                      engine->name, __builtin_return_address(0),
> > -                      yesno(test_bit(ENGINE_IRQ_BREADCRUMB,
> > -                                     &engine->irq_posted)),
> > -                      intel_engine_get_seqno(engine),
> > -                      intel_engine_last_submit(engine));
> > +     struct drm_printer p = drm_debug_printer(__func__);
> > +
> > +     DRM_DEBUG_DRIVER("%s missed breadcrumb at %pS\n",
> > +                      engine->name, __builtin_return_address(0));
> > +     intel_engine_dump(engine, &p);
> 
> We are holding rb_lock while we enter and the dump will retake it.

Well that's annoying. It also leads to the next point, that
intel_engine_dump() isn't taking the engine->timeline->lock enough.

Good catch, thanks.
 
> We also should add proper asserts to intel_engine_dump that we notice
> immediately if we try to dump on wrong context.

You can't just assert(!spin_locked()) as that gives false positives when
it is in use elsewhere. The answer would be to annotate all spinlocks
with lockdep.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

Quoting Tvrtko Ursulin (2017-11-09 10:04:17)
> 
> On 09/11/2017 09:37, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2017-11-09 09:27:33)
> >>
> >> On 09/11/2017 09:13, Lionel Landwerlin wrote:
> >>> On 08/11/17 19:14, Chris Wilson wrote:
> >>>> +/*
> >>>> + * Different engines serve different roles, and there may be more
> >>>> than one
> >>>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> >>>> + * classification of the role of the engine, which may be used when
> >>>> requesting
> >>>> + * operations to be performed on a certain subset of engines, or for
> >>>> providing
> >>>> + * information about that group.
> >>>> + */
> >>>> +enum drm_i915_gem_engine_class {
> >>>> +    I915_ENGINE_CLASS_OTHER = 0,
> >>>> +    I915_ENGINE_CLASS_RENDER = 1,
> >>>> +    I915_ENGINE_CLASS_COPY = 2,
> >>>> +    I915_ENGINE_CLASS_VIDEO = 3,
> >>>> +    I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> >>>> +};
> >>>> +
> >>> I've tried to build a bit UI in GPUTop to show this.
> >>> I'm a bit skeptical about the OTHER type because if this enum is meant
> >>> to be extended, then why do we need an OTHER class? We should create new
> >>> classes instead.
> >>
> >> Good point, I agree that I cannot find a reason why we would have it in
> >> the uAPI. I suspect I was just doing a copy-paste-transform of the
> >> hardware definitions from i915_reg.h.
> > 
> > Do we want to keep 0 as undefined? It can be both a nuisance and a
> > blessing...
> 
> What can you imagine it would be useful for?

I915_EXEC_DEFAULT is of I915_ENGINE_CLASS_OTHER.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Lionel Landwerlin]

On 09/11/17 11:13, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2017-11-09 10:04:17)
>> On 09/11/2017 09:37, Chris Wilson wrote:
>>> Quoting Tvrtko Ursulin (2017-11-09 09:27:33)
>>>> On 09/11/2017 09:13, Lionel Landwerlin wrote:
>>>>> On 08/11/17 19:14, Chris Wilson wrote:
>>>>>> +/*
>>>>>> + * Different engines serve different roles, and there may be more
>>>>>> than one
>>>>>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
>>>>>> + * classification of the role of the engine, which may be used when
>>>>>> requesting
>>>>>> + * operations to be performed on a certain subset of engines, or for
>>>>>> providing
>>>>>> + * information about that group.
>>>>>> + */
>>>>>> +enum drm_i915_gem_engine_class {
>>>>>> +    I915_ENGINE_CLASS_OTHER = 0,
>>>>>> +    I915_ENGINE_CLASS_RENDER = 1,
>>>>>> +    I915_ENGINE_CLASS_COPY = 2,
>>>>>> +    I915_ENGINE_CLASS_VIDEO = 3,
>>>>>> +    I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
>>>>>> +};
>>>>>> +
>>>>> I've tried to build a bit UI in GPUTop to show this.
>>>>> I'm a bit skeptical about the OTHER type because if this enum is meant
>>>>> to be extended, then why do we need an OTHER class? We should create new
>>>>> classes instead.
>>>> Good point, I agree that I cannot find a reason why we would have it in
>>>> the uAPI. I suspect I was just doing a copy-paste-transform of the
>>>> hardware definitions from i915_reg.h.
>>> Do we want to keep 0 as undefined? It can be both a nuisance and a
>>> blessing...
>> What can you imagine it would be useful for?
> I915_EXEC_DEFAULT is of I915_ENGINE_CLASS_OTHER.
> -Chris
>
But that's render, right?

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

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

Quoting Lionel Landwerlin (2017-11-09 09:13:03)
> On 08/11/17 19:14, Chris Wilson wrote:
> >   
> > +/*
> > + * Different engines serve different roles, and there may be more than one
> > + * engine serving each role. enum drm_i915_gem_engine_class provides a
> > + * classification of the role of the engine, which may be used when requesting
> > + * operations to be performed on a certain subset of engines, or for providing
> > + * information about that group.
> > + */
> > +enum drm_i915_gem_engine_class {
> > +     I915_ENGINE_CLASS_OTHER = 0,
> > +     I915_ENGINE_CLASS_RENDER = 1,
> > +     I915_ENGINE_CLASS_COPY = 2,
> > +     I915_ENGINE_CLASS_VIDEO = 3,
> > +     I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> > +};
> > +
> I've tried to build a bit UI in GPUTop to show this.
> I'm a bit skeptical about the OTHER type because if this enum is meant 
> to be extended, then why do we need an OTHER class? We should create new 
> classes instead.

Whilst OTHER is certainly a question of "why not add a define for the
new class", my 2c is that we do want an explicit INVALID enum. It
doesn't have to 0, -1 will work just fine as well, but there will be a
time either in userspace or in reporting from the kernel where we will
need to store an invalid value that is guaranteed not to map onto a real
class.

So I915_ENGINE_CLASS_INVALID = -1 ?
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Lionel Landwerlin]

On 09/11/17 21:29, Chris Wilson wrote:
> Quoting Lionel Landwerlin (2017-11-09 09:13:03)
>> On 08/11/17 19:14, Chris Wilson wrote:
>>>    
>>> +/*
>>> + * Different engines serve different roles, and there may be more than one
>>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
>>> + * classification of the role of the engine, which may be used when requesting
>>> + * operations to be performed on a certain subset of engines, or for providing
>>> + * information about that group.
>>> + */
>>> +enum drm_i915_gem_engine_class {
>>> +     I915_ENGINE_CLASS_OTHER = 0,
>>> +     I915_ENGINE_CLASS_RENDER = 1,
>>> +     I915_ENGINE_CLASS_COPY = 2,
>>> +     I915_ENGINE_CLASS_VIDEO = 3,
>>> +     I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
>>> +};
>>> +
>> I've tried to build a bit UI in GPUTop to show this.
>> I'm a bit skeptical about the OTHER type because if this enum is meant
>> to be extended, then why do we need an OTHER class? We should create new
>> classes instead.
> Whilst OTHER is certainly a question of "why not add a define for the
> new class", my 2c is that we do want an explicit INVALID enum. It
> doesn't have to 0, -1 will work just fine as well, but there will be a
> time either in userspace or in reporting from the kernel where we will
> need to store an invalid value that is guaranteed not to map onto a real
> class.
>
> So I915_ENGINE_CLASS_INVALID = -1 ?
> -Chris
>
Okay, so you want to use that value for userspace to say "execute this 
batch buffer on whatever engine".
And in that case that's fine with me, I didn't get that would be used in 
the userspace->kernel direction.

But I'm still struggling to understand why the kernel would report that 
there is an "invalid" class of engine on this device, rather than 
something more descriptive.

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

Re: [PATCH v5 2/9] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

Quoting Lionel Landwerlin (2017-11-09 22:16:26)
> On 09/11/17 21:29, Chris Wilson wrote:
> > Quoting Lionel Landwerlin (2017-11-09 09:13:03)
> >> On 08/11/17 19:14, Chris Wilson wrote:
> >>>    
> >>> +/*
> >>> + * Different engines serve different roles, and there may be more than one
> >>> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> >>> + * classification of the role of the engine, which may be used when requesting
> >>> + * operations to be performed on a certain subset of engines, or for providing
> >>> + * information about that group.
> >>> + */
> >>> +enum drm_i915_gem_engine_class {
> >>> +     I915_ENGINE_CLASS_OTHER = 0,
> >>> +     I915_ENGINE_CLASS_RENDER = 1,
> >>> +     I915_ENGINE_CLASS_COPY = 2,
> >>> +     I915_ENGINE_CLASS_VIDEO = 3,
> >>> +     I915_ENGINE_CLASS_VIDEO_ENHANCE = 4,
> >>> +};
> >>> +
> >> I've tried to build a bit UI in GPUTop to show this.
> >> I'm a bit skeptical about the OTHER type because if this enum is meant
> >> to be extended, then why do we need an OTHER class? We should create new
> >> classes instead.
> > Whilst OTHER is certainly a question of "why not add a define for the
> > new class", my 2c is that we do want an explicit INVALID enum. It
> > doesn't have to 0, -1 will work just fine as well, but there will be a
> > time either in userspace or in reporting from the kernel where we will
> > need to store an invalid value that is guaranteed not to map onto a real
> > class.
> >
> > So I915_ENGINE_CLASS_INVALID = -1 ?
> > -Chris
> >
> Okay, so you want to use that value for userspace to say "execute this 
> batch buffer on whatever engine".

No, I don't expect it to ever be a part of a request from userspace.
(Though who knows!)

> And in that case that's fine with me, I didn't get that would be used in 
> the userspace->kernel direction.
> 
> But I'm still struggling to understand why the kernel would report that 
> there is an "invalid" class of engine on this device, rather than 
> something more descriptive.

There's the discussion of exactly what class gen2-5 CS comprises; it has
dual functionality of both CLASS_COPY and CLASS_RENDER. (And the
unspoken CLASS_GPGPU.)

The usecase for invalid is in userspace in being able to say that its
struct engine doesn't map to any exact HW class. And at some point, in
some interface the same will be required from the kernel. What I am
saying is that history says that having an explicit invalid value from
the beginning is useful for futureproofing the API, as well as being
convenient for writing generic code to map between the different
interfaces.
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v3] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We want to be able to report back to userspace details about an engine's
class, and in return for userspace to be able to request actions
regarding certain classes of engines. To isolate the uABI from any
variations between hw generations, we define an abstract class for the
engines and internally map onto the hw.

v2: Remove MAX from the uABI; keep it internal if we need it, but don't
let userspace make the mistake of using it themselves.
v3: s/OTHER/INVALID/
  The use of OTHER is ill-defined, so remove it from the uABI as any
  future new type of engine can define a class to suit it. But keep a
  reserved value for an invalid class, so that we can always
  unambiguously express when something doesn't belong to the
  classification.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v2
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 10 +++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++++-
 include/uapi/drm/i915_drm.h             | 16 ++++++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 87778f03393b..bded9c40dbd5 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -50,6 +50,8 @@ struct engine_class_info {
 	const char *name;
 	int (*init_legacy)(struct intel_engine_cs *engine);
 	int (*init_execlists)(struct intel_engine_cs *engine);
+
+	u8 uabi_class;
 };
 
 static const struct engine_class_info intel_engine_classes[] = {
@@ -57,21 +59,25 @@ static const struct engine_class_info intel_engine_classes[] = {
 		.name = "rcs",
 		.init_execlists = logical_render_ring_init,
 		.init_legacy = intel_init_render_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_RENDER,
 	},
 	[COPY_ENGINE_CLASS] = {
 		.name = "bcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_blt_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_COPY,
 	},
 	[VIDEO_DECODE_CLASS] = {
 		.name = "vcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_bsd_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO,
 	},
 	[VIDEO_ENHANCEMENT_CLASS] = {
 		.name = "vecs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_vebox_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO_ENHANCE,
 	},
 };
 
@@ -213,13 +219,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	WARN_ON(snprintf(engine->name, sizeof(engine->name), "%s%u",
 			 class_info->name, info->instance) >=
 		sizeof(engine->name));
-	engine->uabi_id = info->uabi_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
 	engine->class = info->class;
 	engine->instance = info->instance;
 
+	engine->uabi_id = info->uabi_id;
+	engine->uabi_class = class_info->uabi_class;
+
 	engine->context_size = __intel_engine_context_size(dev_priv,
 							   engine->class);
 	if (WARN_ON(engine->context_size > BIT(20)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1106904f6e31..7d3903b9fb1d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -290,11 +290,14 @@ struct intel_engine_execlists {
 struct intel_engine_cs {
 	struct drm_i915_private *i915;
 	char name[INTEL_ENGINE_CS_MAX_NAME];
+
 	enum intel_engine_id id;
-	unsigned int uabi_id;
 	unsigned int hw_id;
 	unsigned int guc_id;
 
+	u8 uabi_id;
+	u8 uabi_class;
+
 	u8 class;
 	u8 instance;
 	u32 context_size;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ac3c6503ca27..1f7dfb22a7c2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -86,6 +86,22 @@ enum i915_mocs_table_index {
 	I915_MOCS_CACHED,
 };
 
+/*
+ * Different engines serve different roles, and there may be more than one
+ * engine serving each role. enum drm_i915_gem_engine_class provides a
+ * classification of the role of the engine, which may be used when requesting
+ * operations to be performed on a certain subset of engines, or for providing
+ * information about that group.
+ */
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_RENDER	= 0,
+	I915_ENGINE_CLASS_COPY		= 1,
+	I915_ENGINE_CLASS_VIDEO		= 2,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE	= 3,
+
+	I915_ENGINE_CLASS_INVALID	= -1
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
2.15.0

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

[PATCH v3] drm/i915: Define an engine class enum for the uABI

[Chris Wilson]

From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>

We want to be able to report back to userspace details about an engine's
class, and in return for userspace to be able to request actions
regarding certain classes of engines. To isolate the uABI from any
variations between hw generations, we define an abstract class for the
engines and internally map onto the hw.

v2: Remove MAX from the uABI; keep it internal if we need it, but don't
let userspace make the mistake of using it themselves.
v3: s/OTHER/INVALID/
  The use of OTHER is ill-defined, so remove it from the uABI as any
  future new type of engine can define a class to suit it. But keep a
  reserved value for an invalid class, so that we can always
  unambiguously express when something doesn't belong to the
  classification.

Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Lionel Landwerlin <lionel.g.landwerlin@intel.com>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v2
---
 drivers/gpu/drm/i915/intel_engine_cs.c  | 10 +++++++++-
 drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++++-
 include/uapi/drm/i915_drm.h             | 16 ++++++++++++++++
 3 files changed, 29 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 87778f03393b..bded9c40dbd5 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -50,6 +50,8 @@ struct engine_class_info {
 	const char *name;
 	int (*init_legacy)(struct intel_engine_cs *engine);
 	int (*init_execlists)(struct intel_engine_cs *engine);
+
+	u8 uabi_class;
 };
 
 static const struct engine_class_info intel_engine_classes[] = {
@@ -57,21 +59,25 @@ static const struct engine_class_info intel_engine_classes[] = {
 		.name = "rcs",
 		.init_execlists = logical_render_ring_init,
 		.init_legacy = intel_init_render_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_RENDER,
 	},
 	[COPY_ENGINE_CLASS] = {
 		.name = "bcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_blt_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_COPY,
 	},
 	[VIDEO_DECODE_CLASS] = {
 		.name = "vcs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_bsd_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO,
 	},
 	[VIDEO_ENHANCEMENT_CLASS] = {
 		.name = "vecs",
 		.init_execlists = logical_xcs_ring_init,
 		.init_legacy = intel_init_vebox_ring_buffer,
+		.uabi_class = I915_ENGINE_CLASS_VIDEO_ENHANCE,
 	},
 };
 
@@ -213,13 +219,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 	WARN_ON(snprintf(engine->name, sizeof(engine->name), "%s%u",
 			 class_info->name, info->instance) >=
 		sizeof(engine->name));
-	engine->uabi_id = info->uabi_id;
 	engine->hw_id = engine->guc_id = info->hw_id;
 	engine->mmio_base = info->mmio_base;
 	engine->irq_shift = info->irq_shift;
 	engine->class = info->class;
 	engine->instance = info->instance;
 
+	engine->uabi_id = info->uabi_id;
+	engine->uabi_class = class_info->uabi_class;
+
 	engine->context_size = __intel_engine_context_size(dev_priv,
 							   engine->class);
 	if (WARN_ON(engine->context_size > BIT(20)))
diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
index 1106904f6e31..7d3903b9fb1d 100644
--- a/drivers/gpu/drm/i915/intel_ringbuffer.h
+++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
@@ -290,11 +290,14 @@ struct intel_engine_execlists {
 struct intel_engine_cs {
 	struct drm_i915_private *i915;
 	char name[INTEL_ENGINE_CS_MAX_NAME];
+
 	enum intel_engine_id id;
-	unsigned int uabi_id;
 	unsigned int hw_id;
 	unsigned int guc_id;
 
+	u8 uabi_id;
+	u8 uabi_class;
+
 	u8 class;
 	u8 instance;
 	u32 context_size;
diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
index ac3c6503ca27..1f7dfb22a7c2 100644
--- a/include/uapi/drm/i915_drm.h
+++ b/include/uapi/drm/i915_drm.h
@@ -86,6 +86,22 @@ enum i915_mocs_table_index {
 	I915_MOCS_CACHED,
 };
 
+/*
+ * Different engines serve different roles, and there may be more than one
+ * engine serving each role. enum drm_i915_gem_engine_class provides a
+ * classification of the role of the engine, which may be used when requesting
+ * operations to be performed on a certain subset of engines, or for providing
+ * information about that group.
+ */
+enum drm_i915_gem_engine_class {
+	I915_ENGINE_CLASS_RENDER	= 0,
+	I915_ENGINE_CLASS_COPY		= 1,
+	I915_ENGINE_CLASS_VIDEO		= 2,
+	I915_ENGINE_CLASS_VIDEO_ENHANCE	= 3,
+
+	I915_ENGINE_CLASS_INVALID	= -1
+};
+
 /* Each region is a minimum of 16k, and there are at most 255 of them.
  */
 #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use
-- 
2.15.0

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

✗ Fi.CI.BAT: warning for series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno (rev3)

[Patchwork]

== Series Details ==

Series: series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno (rev3)
URL   : https://patchwork.freedesktop.org/series/33452/
State : warning

== Summary ==

Series 33452v3 series starting with [v5,1/9] drm/i915: Include engine state on detecting a missed breadcrumb/seqno
https://patchwork.freedesktop.org/api/1.0/series/33452/revisions/3/mbox/

Test kms_cursor_legacy:
        Subgroup basic-flip-before-cursor-atomic:
                pass       -> DMESG-WARN (fi-bsw-n3050)

fi-bdw-5557u     total:289  pass:268  dwarn:0   dfail:0   fail:0   skip:21  time:447s
fi-bdw-gvtdvm    total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:459s
fi-blb-e6850     total:289  pass:223  dwarn:1   dfail:0   fail:0   skip:65  time:382s
fi-bsw-n3050     total:289  pass:242  dwarn:1   dfail:0   fail:0   skip:46  time:540s
fi-bwr-2160      total:289  pass:183  dwarn:0   dfail:0   fail:0   skip:106 time:275s
fi-bxt-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:500s
fi-bxt-j4205     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:511s
fi-byt-j1900     total:289  pass:254  dwarn:0   dfail:0   fail:0   skip:35  time:494s
fi-byt-n2820     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:498s
fi-elk-e7500     total:289  pass:229  dwarn:0   dfail:0   fail:0   skip:60  time:430s
fi-gdg-551       total:289  pass:178  dwarn:1   dfail:0   fail:1   skip:109 time:263s
fi-glk-1         total:289  pass:261  dwarn:0   dfail:0   fail:0   skip:28  time:543s
fi-hsw-4770      total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:431s
fi-hsw-4770r     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:438s
fi-ilk-650       total:289  pass:228  dwarn:0   dfail:0   fail:0   skip:61  time:426s
fi-ivb-3520m     total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:492s
fi-ivb-3770      total:289  pass:260  dwarn:0   dfail:0   fail:0   skip:29  time:459s
fi-kbl-7500u     total:289  pass:264  dwarn:1   dfail:0   fail:0   skip:24  time:487s
fi-kbl-7560u     total:289  pass:270  dwarn:0   dfail:0   fail:0   skip:19  time:523s
fi-kbl-7567u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:476s
fi-kbl-r         total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:535s
fi-pnv-d510      total:289  pass:222  dwarn:1   dfail:0   fail:0   skip:66  time:573s
fi-skl-6260u     total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:454s
fi-skl-6600u     total:289  pass:262  dwarn:0   dfail:0   fail:0   skip:27  time:547s
fi-skl-6700hq    total:289  pass:263  dwarn:0   dfail:0   fail:0   skip:26  time:561s
fi-skl-6700k     total:289  pass:265  dwarn:0   dfail:0   fail:0   skip:24  time:517s
fi-skl-6770hq    total:289  pass:269  dwarn:0   dfail:0   fail:0   skip:20  time:493s
fi-skl-gvtdvm    total:289  pass:266  dwarn:0   dfail:0   fail:0   skip:23  time:461s
fi-snb-2520m     total:289  pass:250  dwarn:0   dfail:0   fail:0   skip:39  time:557s
fi-snb-2600      total:289  pass:249  dwarn:0   dfail:0   fail:0   skip:40  time:424s
Blacklisted hosts:
fi-cfl-s         total:289  pass:254  dwarn:3   dfail:0   fail:0   skip:32  time:540s
fi-cnl-y         total:289  pass:261  dwarn:0   dfail:0   fail:1   skip:27  time:550s
fi-glk-dsi       total:289  pass:259  dwarn:0   dfail:0   fail:0   skip:30  time:497s

c36994b067f0771eee4d3136f40c0c6040a8fa77 drm-tip: 2017y-11m-10d-11h-48m-19s UTC integration manifest
93d8c14b43b5 drm/i915: Stop caching the "golden" renderstate
a85db8f2b31f drm/i915: Record the default hw state after reset upon load
72548ae0b293 drm/i915: Mark the context state as dirty/written
f55cbe8ee5ec drm/i915: Inline intel_modeset_gem_init()
fe2ab7c41411 drm/i915: Move intel_init_clock_gating() to i915_gem_init()
6aab922466d6 drm/i915: Move GT powersaving init to i915_gem_init()
6b6c6918fabb drm/i915: Force the switch to the i915->kernel_context
fda06e94d307 drm/i915: Define an engine class enum for the uABI
f60ae93cab98 drm/i915: Include engine state on detecting a missed breadcrumb/seqno

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_7059/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH v3] drm/i915: Define an engine class enum for the uABI

[Lionel Landwerlin]

Reviewed-by: Lionel Landwerlin <lionel.g.landwerlin@intel.com>

On 10/11/17 13:19, Chris Wilson wrote:
> From: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>
> We want to be able to report back to userspace details about an engine's
> class, and in return for userspace to be able to request actions
> regarding certain classes of engines. To isolate the uABI from any
> variations between hw generations, we define an abstract class for the
> engines and internally map onto the hw.
>
> v2: Remove MAX from the uABI; keep it internal if we need it, but don't
> let userspace make the mistake of using it themselves.
> v3: s/OTHER/INVALID/
>    The use of OTHER is ill-defined, so remove it from the uABI as any
>    future new type of engine can define a class to suit it. But keep a
>    reserved value for an invalid class, so that we can always
>    unambiguously express when something doesn't belong to the
>    classification.
>
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> #v2
> ---
>   drivers/gpu/drm/i915/intel_engine_cs.c  | 10 +++++++++-
>   drivers/gpu/drm/i915/intel_ringbuffer.h |  5 ++++-
>   include/uapi/drm/i915_drm.h             | 16 ++++++++++++++++
>   3 files changed, 29 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
> index 87778f03393b..bded9c40dbd5 100644
> --- a/drivers/gpu/drm/i915/intel_engine_cs.c
> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c
> @@ -50,6 +50,8 @@ struct engine_class_info {
>   	const char *name;
>   	int (*init_legacy)(struct intel_engine_cs *engine);
>   	int (*init_execlists)(struct intel_engine_cs *engine);
> +
> +	u8 uabi_class;
>   };
>   
>   static const struct engine_class_info intel_engine_classes[] = {
> @@ -57,21 +59,25 @@ static const struct engine_class_info intel_engine_classes[] = {
>   		.name = "rcs",
>   		.init_execlists = logical_render_ring_init,
>   		.init_legacy = intel_init_render_ring_buffer,
> +		.uabi_class = I915_ENGINE_CLASS_RENDER,
>   	},
>   	[COPY_ENGINE_CLASS] = {
>   		.name = "bcs",
>   		.init_execlists = logical_xcs_ring_init,
>   		.init_legacy = intel_init_blt_ring_buffer,
> +		.uabi_class = I915_ENGINE_CLASS_COPY,
>   	},
>   	[VIDEO_DECODE_CLASS] = {
>   		.name = "vcs",
>   		.init_execlists = logical_xcs_ring_init,
>   		.init_legacy = intel_init_bsd_ring_buffer,
> +		.uabi_class = I915_ENGINE_CLASS_VIDEO,
>   	},
>   	[VIDEO_ENHANCEMENT_CLASS] = {
>   		.name = "vecs",
>   		.init_execlists = logical_xcs_ring_init,
>   		.init_legacy = intel_init_vebox_ring_buffer,
> +		.uabi_class = I915_ENGINE_CLASS_VIDEO_ENHANCE,
>   	},
>   };
>   
> @@ -213,13 +219,15 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
>   	WARN_ON(snprintf(engine->name, sizeof(engine->name), "%s%u",
>   			 class_info->name, info->instance) >=
>   		sizeof(engine->name));
> -	engine->uabi_id = info->uabi_id;
>   	engine->hw_id = engine->guc_id = info->hw_id;
>   	engine->mmio_base = info->mmio_base;
>   	engine->irq_shift = info->irq_shift;
>   	engine->class = info->class;
>   	engine->instance = info->instance;
>   
> +	engine->uabi_id = info->uabi_id;
> +	engine->uabi_class = class_info->uabi_class;
> +
>   	engine->context_size = __intel_engine_context_size(dev_priv,
>   							   engine->class);
>   	if (WARN_ON(engine->context_size > BIT(20)))
> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.h b/drivers/gpu/drm/i915/intel_ringbuffer.h
> index 1106904f6e31..7d3903b9fb1d 100644
> --- a/drivers/gpu/drm/i915/intel_ringbuffer.h
> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.h
> @@ -290,11 +290,14 @@ struct intel_engine_execlists {
>   struct intel_engine_cs {
>   	struct drm_i915_private *i915;
>   	char name[INTEL_ENGINE_CS_MAX_NAME];
> +
>   	enum intel_engine_id id;
> -	unsigned int uabi_id;
>   	unsigned int hw_id;
>   	unsigned int guc_id;
>   
> +	u8 uabi_id;
> +	u8 uabi_class;
> +
>   	u8 class;
>   	u8 instance;
>   	u32 context_size;
> diff --git a/include/uapi/drm/i915_drm.h b/include/uapi/drm/i915_drm.h
> index ac3c6503ca27..1f7dfb22a7c2 100644
> --- a/include/uapi/drm/i915_drm.h
> +++ b/include/uapi/drm/i915_drm.h
> @@ -86,6 +86,22 @@ enum i915_mocs_table_index {
>   	I915_MOCS_CACHED,
>   };
>   
> +/*
> + * Different engines serve different roles, and there may be more than one
> + * engine serving each role. enum drm_i915_gem_engine_class provides a
> + * classification of the role of the engine, which may be used when requesting
> + * operations to be performed on a certain subset of engines, or for providing
> + * information about that group.
> + */
> +enum drm_i915_gem_engine_class {
> +	I915_ENGINE_CLASS_RENDER	= 0,
> +	I915_ENGINE_CLASS_COPY		= 1,
> +	I915_ENGINE_CLASS_VIDEO		= 2,
> +	I915_ENGINE_CLASS_VIDEO_ENHANCE	= 3,
> +
> +	I915_ENGINE_CLASS_INVALID	= -1
> +};
> +
>   /* Each region is a minimum of 16k, and there are at most 255 of them.
>    */
>   #define I915_NR_TEX_REGIONS 255	/* table size 2k - maximum due to use


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