[PATCH 1/2] drm/i915: Split intel_engine allocation and initialisation

[PATCH 1/2] drm/i915: Split intel_engine allocation and initialisation

[Chris Wilson]

In order to reset the GPU early on in the module load sequence, we need
to allocate the basic engine structs (to populate the mmio offsets etc).
Currently, the engine initialisation allocates both the base struct and
also allocate auxiliary objects, which depend upon state setup quite
late in the load sequence. We split off the allocation callback for
later and allow ourselves to allocate the engine structs themselves
early.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c        | 19 +++++++-
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 79 +++++++++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_lrc.h       |  2 -
 4 files changed, 79 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 4ae69ebe166e..b19ec224831c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -755,6 +755,15 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)
 	return -ENOMEM;
 }
 
+static void i915_engines_cleanup(struct drm_i915_private *i915)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	for_each_engine(engine, i915, id)
+		kfree(engine);
+}
+
 static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv)
 {
 	destroy_workqueue(dev_priv->hotplug.dp_wq);
@@ -817,12 +826,15 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	mutex_init(&dev_priv->pps_mutex);
 
 	intel_uc_init_early(dev_priv);
-
 	i915_memcpy_init_early(dev_priv);
 
+	ret = intel_engines_init_early(dev_priv);
+	if (ret)
+		return ret;
+
 	ret = i915_workqueues_init(dev_priv);
 	if (ret < 0)
-		return ret;
+		goto err_engines;
 
 	ret = intel_gvt_init(dev_priv);
 	if (ret < 0)
@@ -857,6 +869,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	intel_gvt_cleanup(dev_priv);
 err_workqueues:
 	i915_workqueues_cleanup(dev_priv);
+err_engines:
+	i915_engines_cleanup(dev_priv);
 	return ret;
 }
 
@@ -869,6 +883,7 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
 	i915_perf_fini(dev_priv);
 	i915_gem_load_cleanup(dev_priv);
 	i915_workqueues_cleanup(dev_priv);
+	i915_engines_cleanup(dev_priv);
 }
 
 static int i915_mmio_setup(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 322e8c99d118..2467faa4a9e4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2941,6 +2941,9 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
+int intel_engines_init_early(struct drm_i915_private *dev_priv);
+int intel_engines_init(struct drm_i915_private *dev_priv);
+
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 			   u32 pin_mask, u32 long_mask);
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 371acf109e34..77c07e86ec62 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -110,21 +110,20 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_engines_init() - allocate, populate and init the Engine Command Streamers
+ * intel_engines_init_early() - allocate the Engine Command Streamers
  * @dev_priv: i915 device private
  *
  * Return: non-zero if the initialization failed.
  */
-int intel_engines_init(struct drm_i915_private *dev_priv)
+int intel_engines_init_early(struct drm_i915_private *dev_priv)
 {
 	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
 	unsigned int ring_mask = INTEL_INFO(dev_priv)->ring_mask;
 	unsigned int mask = 0;
-	int (*init)(struct intel_engine_cs *engine);
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 	unsigned int i;
-	int ret;
+	int err;
 
 	WARN_ON(ring_mask == 0);
 	WARN_ON(ring_mask &
@@ -134,20 +133,8 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
 		if (!HAS_ENGINE(dev_priv, i))
 			continue;
 
-		if (i915.enable_execlists)
-			init = intel_engines[i].init_execlists;
-		else
-			init = intel_engines[i].init_legacy;
-
-		if (!init)
-			continue;
-
-		ret = intel_engine_setup(dev_priv, i);
-		if (ret)
-			goto cleanup;
-
-		ret = init(dev_priv->engine[i]);
-		if (ret)
+		err = intel_engine_setup(dev_priv, i);
+		if (err)
 			goto cleanup;
 
 		mask |= ENGINE_MASK(i);
@@ -166,14 +153,66 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
 	return 0;
 
 cleanup:
+	for_each_engine(engine, dev_priv, id)
+		kfree(engine);
+	return err;
+}
+
+/**
+ * intel_engines_init() - allocate, populate and init the Engine Command Streamers
+ * @dev_priv: i915 device private
+ *
+ * Return: non-zero if the initialization failed.
+ */
+int intel_engines_init(struct drm_i915_private *dev_priv)
+{
+	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
+	unsigned int mask = 0;
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+	int err = 0;
+
+	for_each_engine(engine, dev_priv, id) {
+		int (*init)(struct intel_engine_cs *engine) = NULL;
+
+		if (!err) {
+			if (i915.enable_execlists)
+				init = intel_engines[id].init_execlists;
+			else
+				init = intel_engines[id].init_legacy;
+		}
+
+		if (!init || (err = init(engine))) {
+			kfree(engine);
+			dev_priv->engine[id] = NULL;
+			continue;
+		}
+
+		mask |= ENGINE_MASK(id);
+	}
+	if (err)
+		goto cleanup;
+
+	/*
+	 * Catch failures to update intel_engines table when the new engines
+	 * are added to the driver by a warning and disabling the forgotten
+	 * engines.
+	 */
+	if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask))
+		device_info->ring_mask = mask;
+
+	device_info->num_rings = hweight32(mask);
+
+	return 0;
+
+cleanup:
 	for_each_engine(engine, dev_priv, id) {
 		if (i915.enable_execlists)
 			intel_logical_ring_cleanup(engine);
 		else
 			intel_engine_cleanup(engine);
 	}
-
-	return ret;
+	return err;
 }
 
 void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno)
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 0c852c024227..c8009c7bfbdd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -68,8 +68,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
 int logical_render_ring_init(struct intel_engine_cs *engine);
 int logical_xcs_ring_init(struct intel_engine_cs *engine);
 
-int intel_engines_init(struct drm_i915_private *dev_priv);
-
 /* Logical Ring Contexts */
 
 /* One extra page is added before LRC for GuC as shared data */
-- 
2.11.0

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

[PATCH 2/2] drm/i915: Reset the gpu on takeover

[Chris Wilson]

The GPU may be in an unknown state following resume and module load. The
previous occupant may have left contexts loaded, or other dangerous
state, which can cause an immediate GPU hang for us. The only save
course of action is to reset the GPU prior to using it - similarly to
how we reset the GPU prior to unload (before a second user may be
affected by our leftover state).

We need to reset the GPU very early in our load/resume sequence so that
any stale HW pointers are revoked prior to any resource allocations we
make (that may conflict).

A reset should only be a couple of milliseconds on a slow device, a cost
we should easily be able to absorb into our initialisation times.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c |  2 ++
 drivers/gpu/drm/i915/i915_drv.h |  2 ++
 drivers/gpu/drm/i915/i915_gem.c | 27 +++++++++++++++++++++++----
 3 files changed, 27 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index b19ec224831c..24e9da1e4caf 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -950,6 +950,7 @@ static int i915_driver_init_mmio(struct drm_i915_private *dev_priv)
 		goto put_bridge;
 
 	intel_uncore_init(dev_priv);
+	i915_gem_init_mmio(dev_priv);
 
 	return 0;
 
@@ -1579,6 +1580,7 @@ static int i915_drm_resume(struct drm_device *dev)
 
 	disable_rpm_wakeref_asserts(dev_priv);
 	intel_sanitize_gt_powersave(dev_priv);
+	i915_gem_sanitize(dev_priv);
 
 	ret = i915_ggtt_enable_hw(dev_priv);
 	if (ret)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2467faa4a9e4..5ee4b778bf04 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3121,6 +3121,7 @@ int i915_gem_get_aperture_ioctl(struct drm_device *dev, void *data,
 				struct drm_file *file_priv);
 int i915_gem_wait_ioctl(struct drm_device *dev, void *data,
 			struct drm_file *file_priv);
+void i915_gem_sanitize(struct drm_i915_private *i915);
 int i915_gem_load_init(struct drm_i915_private *dev_priv);
 void i915_gem_load_cleanup(struct drm_i915_private *dev_priv);
 void i915_gem_load_init_fences(struct drm_i915_private *dev_priv);
@@ -3336,6 +3337,7 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv);
 void i915_gem_reset_finish(struct drm_i915_private *dev_priv);
 void i915_gem_set_wedged(struct drm_i915_private *dev_priv);
 void i915_gem_clflush_object(struct drm_i915_gem_object *obj, bool force);
+void i915_gem_init_mmio(struct drm_i915_private *i915);
 int __must_check i915_gem_init(struct drm_i915_private *dev_priv);
 int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index a07b62732923..2522d4895ff7 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4201,6 +4201,23 @@ static void assert_kernel_context_is_current(struct drm_i915_private *dev_priv)
 			   !i915_gem_context_is_kernel(engine->last_retired_context));
 }
 
+void i915_gem_sanitize(struct drm_i915_private *i915)
+{
+	/*
+	 * If we inherit context state from the BIOS or earlier occupants
+	 * of the GPU, the GPU may be in an inconsistent state when we
+	 * try to take over. The only way to remove the earlier state
+	 * is by resetting. However, resetting on earlier gen is tricky as
+	 * it may impact the display and we are uncertain about the stability
+	 * of the reset, so we only reset recent machines with logical
+	 * context support (that must be reset to remove any stray contexts).
+	 */
+	if (HAS_HW_CONTEXTS(i915)) {
+		int reset = intel_gpu_reset(i915, ALL_ENGINES);
+		WARN_ON(reset && reset != -ENODEV);
+	}
+}
+
 int i915_gem_suspend(struct drm_i915_private *dev_priv)
 {
 	struct drm_device *dev = &dev_priv->drm;
@@ -4271,10 +4288,7 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	 * machines is a good idea, we don't - just in case it leaves the
 	 * machine in an unusable condition.
 	 */
-	if (HAS_HW_CONTEXTS(dev_priv)) {
-		int reset = intel_gpu_reset(dev_priv, ALL_ENGINES);
-		WARN_ON(reset && reset != -ENODEV);
-	}
+	i915_gem_sanitize(dev_priv);
 
 	return 0;
 
@@ -4492,6 +4506,11 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+void i915_gem_init_mmio(struct drm_i915_private *i915)
+{
+	i915_gem_sanitize(i915);
+}
+
 void
 i915_gem_cleanup_engines(struct drm_i915_private *dev_priv)
 {
-- 
2.11.0

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

✓ Fi.CI.BAT: success for series starting with [1/2] drm/i915: Split intel_engine allocation and initialisation

[Patchwork]

== Series Details ==

Series: series starting with [1/2] drm/i915: Split intel_engine allocation and initialisation
URL   : https://patchwork.freedesktop.org/series/18347/
State : success

== Summary ==

Series 18347v1 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18347/revisions/1/mbox/

Test kms_force_connector_basic:
        Subgroup prune-stale-modes:
                dmesg-warn -> PASS       (fi-snb-2520m)

fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

f62d6104306f3ec835f3776434a70ca9bb6f820e drm-tip: 2017y-01m-21d-11h-28m-52s UTC integration manifest
0ad0c73 drm/i915: Reset the gpu on takeover
09119cf drm/i915: Split intel_engine allocation and initialisation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3572/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH v2] drm/i915: Split intel_engine allocation and initialisation

[Chris Wilson]

In order to reset the GPU early on in the module load sequence, we need
to allocate the basic engine structs (to populate the mmio offsets etc).
Currently, the engine initialisation allocates both the base struct and
also allocate auxiliary objects, which depend upon state setup quite
late in the load sequence. We split off the allocation callback for
later and allow ourselves to allocate the engine structs themselves
early.

v2: Different paint for the unwind following error.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c        | 19 +++++++-
 drivers/gpu/drm/i915/i915_drv.h        |  3 ++
 drivers/gpu/drm/i915/intel_engine_cs.c | 80 ++++++++++++++++++++++++++--------
 drivers/gpu/drm/i915/intel_lrc.h       |  2 -
 4 files changed, 81 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index ca168b22ee26..f8d1ffeccc69 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -755,6 +755,15 @@ static int i915_workqueues_init(struct drm_i915_private *dev_priv)
 	return -ENOMEM;
 }
 
+static void i915_engines_cleanup(struct drm_i915_private *i915)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	for_each_engine(engine, i915, id)
+		kfree(engine);
+}
+
 static void i915_workqueues_cleanup(struct drm_i915_private *dev_priv)
 {
 	destroy_workqueue(dev_priv->hotplug.dp_wq);
@@ -817,12 +826,15 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	mutex_init(&dev_priv->pps_mutex);
 
 	intel_uc_init_early(dev_priv);
-
 	i915_memcpy_init_early(dev_priv);
 
+	ret = intel_engines_init_early(dev_priv);
+	if (ret)
+		return ret;
+
 	ret = i915_workqueues_init(dev_priv);
 	if (ret < 0)
-		return ret;
+		goto err_engines;
 
 	ret = intel_gvt_init(dev_priv);
 	if (ret < 0)
@@ -857,6 +869,8 @@ static int i915_driver_init_early(struct drm_i915_private *dev_priv,
 	intel_gvt_cleanup(dev_priv);
 err_workqueues:
 	i915_workqueues_cleanup(dev_priv);
+err_engines:
+	i915_engines_cleanup(dev_priv);
 	return ret;
 }
 
@@ -869,6 +883,7 @@ static void i915_driver_cleanup_early(struct drm_i915_private *dev_priv)
 	i915_perf_fini(dev_priv);
 	i915_gem_load_cleanup(dev_priv);
 	i915_workqueues_cleanup(dev_priv);
+	i915_engines_cleanup(dev_priv);
 }
 
 static int i915_mmio_setup(struct drm_i915_private *dev_priv)
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 244628065f94..55bd2d2f8a0d 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -2941,6 +2941,9 @@ extern unsigned long i915_gfx_val(struct drm_i915_private *dev_priv);
 extern void i915_update_gfx_val(struct drm_i915_private *dev_priv);
 int vlv_force_gfx_clock(struct drm_i915_private *dev_priv, bool on);
 
+int intel_engines_init_early(struct drm_i915_private *dev_priv);
+int intel_engines_init(struct drm_i915_private *dev_priv);
+
 /* intel_hotplug.c */
 void intel_hpd_irq_handler(struct drm_i915_private *dev_priv,
 			   u32 pin_mask, u32 long_mask);
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 371acf109e34..69a6416d1223 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -110,21 +110,20 @@ intel_engine_setup(struct drm_i915_private *dev_priv,
 }
 
 /**
- * intel_engines_init() - allocate, populate and init the Engine Command Streamers
+ * intel_engines_init_early() - allocate the Engine Command Streamers
  * @dev_priv: i915 device private
  *
  * Return: non-zero if the initialization failed.
  */
-int intel_engines_init(struct drm_i915_private *dev_priv)
+int intel_engines_init_early(struct drm_i915_private *dev_priv)
 {
 	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
 	unsigned int ring_mask = INTEL_INFO(dev_priv)->ring_mask;
 	unsigned int mask = 0;
-	int (*init)(struct intel_engine_cs *engine);
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 	unsigned int i;
-	int ret;
+	int err;
 
 	WARN_ON(ring_mask == 0);
 	WARN_ON(ring_mask &
@@ -134,23 +133,65 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
 		if (!HAS_ENGINE(dev_priv, i))
 			continue;
 
+		err = intel_engine_setup(dev_priv, i);
+		if (err)
+			goto cleanup;
+
+		mask |= ENGINE_MASK(i);
+	}
+
+	/*
+	 * Catch failures to update intel_engines table when the new engines
+	 * are added to the driver by a warning and disabling the forgotten
+	 * engines.
+	 */
+	if (WARN_ON(mask != ring_mask))
+		device_info->ring_mask = mask;
+
+	device_info->num_rings = hweight32(mask);
+
+	return 0;
+
+cleanup:
+	for_each_engine(engine, dev_priv, id)
+		kfree(engine);
+	return err;
+}
+
+/**
+ * intel_engines_init() - allocate, populate and init the Engine Command Streamers
+ * @dev_priv: i915 device private
+ *
+ * Return: non-zero if the initialization failed.
+ */
+int intel_engines_init(struct drm_i915_private *dev_priv)
+{
+	struct intel_device_info *device_info = mkwrite_device_info(dev_priv);
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id, err_id;
+	unsigned int mask = 0;
+	int err = 0;
+
+	for_each_engine(engine, dev_priv, id) {
+		int (*init)(struct intel_engine_cs *engine);
+
 		if (i915.enable_execlists)
-			init = intel_engines[i].init_execlists;
+			init = intel_engines[id].init_execlists;
 		else
-			init = intel_engines[i].init_legacy;
-
-		if (!init)
+			init = intel_engines[id].init_legacy;
+		if (!init) {
+			kfree(engine);
+			dev_priv->engine[id] = NULL;
 			continue;
+		}
 
-		ret = intel_engine_setup(dev_priv, i);
-		if (ret)
-			goto cleanup;
-
-		ret = init(dev_priv->engine[i]);
-		if (ret)
+		err = init(engine);
+		if (err) {
+			err_id = id;
 			goto cleanup;
+		}
 
-		mask |= ENGINE_MASK(i);
+		mask |= ENGINE_MASK(id);
 	}
 
 	/*
@@ -158,7 +199,7 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
 	 * are added to the driver by a warning and disabling the forgotten
 	 * engines.
 	 */
-	if (WARN_ON(mask != ring_mask))
+	if (WARN_ON(mask != INTEL_INFO(dev_priv)->ring_mask))
 		device_info->ring_mask = mask;
 
 	device_info->num_rings = hweight32(mask);
@@ -167,13 +208,14 @@ int intel_engines_init(struct drm_i915_private *dev_priv)
 
 cleanup:
 	for_each_engine(engine, dev_priv, id) {
-		if (i915.enable_execlists)
+		if (id >= err_id)
+			kfree(engine);
+		else if (i915.enable_execlists)
 			intel_logical_ring_cleanup(engine);
 		else
 			intel_engine_cleanup(engine);
 	}
-
-	return ret;
+	return err;
 }
 
 void intel_engine_init_global_seqno(struct intel_engine_cs *engine, u32 seqno)
diff --git a/drivers/gpu/drm/i915/intel_lrc.h b/drivers/gpu/drm/i915/intel_lrc.h
index 0c852c024227..c8009c7bfbdd 100644
--- a/drivers/gpu/drm/i915/intel_lrc.h
+++ b/drivers/gpu/drm/i915/intel_lrc.h
@@ -68,8 +68,6 @@ void intel_logical_ring_cleanup(struct intel_engine_cs *engine);
 int logical_render_ring_init(struct intel_engine_cs *engine);
 int logical_xcs_ring_init(struct intel_engine_cs *engine);
 
-int intel_engines_init(struct drm_i915_private *dev_priv);
-
 /* Logical Ring Contexts */
 
 /* One extra page is added before LRC for GuC as shared data */
-- 
2.11.0

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

Re: [PATCH 2/2] drm/i915: Reset the gpu on takeover

[Joonas Lahtinen]

On la, 2017-01-21 at 14:50 +0000, Chris Wilson wrote:
> The GPU may be in an unknown state following resume and module load. The
> previous occupant may have left contexts loaded, or other dangerous
> state, which can cause an immediate GPU hang for us. The only save
> course of action is to reset the GPU prior to using it - similarly to
> how we reset the GPU prior to unload (before a second user may be
> affected by our leftover state).
> 
> We need to reset the GPU very early in our load/resume sequence so that
> any stale HW pointers are revoked prior to any resource allocations we
> make (that may conflict).
> 
> A reset should only be a couple of milliseconds on a slow device, a cost
> we should easily be able to absorb into our initialisation times.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Can't really suggest better names for the functions, so;

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

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

Re: [PATCH 1/2] drm/i915: Split intel_engine allocation and initialisation

[Joonas Lahtinen]

On la, 2017-01-21 at 14:50 +0000, Chris Wilson wrote:
> In order to reset the GPU early on in the module load sequence, we need
> to allocate the basic engine structs (to populate the mmio offsets etc).
> Currently, the engine initialisation allocates both the base struct and
> also allocate auxiliary objects, which depend upon state setup quite
> late in the load sequence. We split off the allocation callback for
> later and allow ourselves to allocate the engine structs themselves
> early.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> +int intel_engines_init(struct drm_i915_private *dev_priv)
> +{

<SNIP>

> +	for_each_engine(engine, dev_priv, id) {
> +		int (*init)(struct intel_engine_cs *engine) = NULL;
> +
> +		if (!err) {
> +			if (i915.enable_execlists)
> +				init = intel_engines[id].init_execlists;
> +			else
> +				init = intel_engines[id].init_legacy;
> +		}
> +
> +		if (!init || (err = init(engine))) {
> +			kfree(engine);
> +			dev_priv->engine[id] = NULL;
> +			continue;
> +		}
> +
> +		mask |= ENGINE_MASK(id);
> +	}

As discussed in IRC, this loop is broken after first erroring init.
Looking forward to v2.

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

Re: [PATCH 1/2] drm/i915: Split intel_engine allocation and initialisation

[Chris Wilson]

On Mon, Jan 23, 2017 at 11:41:12AM +0200, Joonas Lahtinen wrote:
> On la, 2017-01-21 at 14:50 +0000, Chris Wilson wrote:
> > In order to reset the GPU early on in the module load sequence, we need
> > to allocate the basic engine structs (to populate the mmio offsets etc).
> > Currently, the engine initialisation allocates both the base struct and
> > also allocate auxiliary objects, which depend upon state setup quite
> > late in the load sequence. We split off the allocation callback for
> > later and allow ourselves to allocate the engine structs themselves
> > early.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > +int intel_engines_init(struct drm_i915_private *dev_priv)
> > +{
> 
> <SNIP>
> 
> > +	for_each_engine(engine, dev_priv, id) {
> > +		int (*init)(struct intel_engine_cs *engine) = NULL;
> > +
> > +		if (!err) {
> > +			if (i915.enable_execlists)
> > +				init = intel_engines[id].init_execlists;
> > +			else
> > +				init = intel_engines[id].init_legacy;
> > +		}
> > +
> > +		if (!init || (err = init(engine))) {
> > +			kfree(engine);
> > +			dev_priv->engine[id] = NULL;
> > +			continue;
> > +		}
> > +
> > +		mask |= ENGINE_MASK(id);
> > +	}
> 
> As discussed in IRC, this loop is broken after first erroring init.

As answered, it is not.

After an err is set, it and all subsequent engines are freed, and then
all previously initialised engines are run through the cleanup.
-Chris

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

✓ Fi.CI.BAT: success for series starting with [v2] drm/i915: Split intel_engine allocation and initialisation (rev2)

[Patchwork]

== Series Details ==

Series: series starting with [v2] drm/i915: Split intel_engine allocation and initialisation (rev2)
URL   : https://patchwork.freedesktop.org/series/18347/
State : success

== Summary ==

Series 18347v2 Series without cover letter
https://patchwork.freedesktop.org/api/1.0/series/18347/revisions/2/mbox/


fi-bdw-5557u     total:246  pass:232  dwarn:0   dfail:0   fail:0   skip:14 
fi-bsw-n3050     total:246  pass:207  dwarn:0   dfail:0   fail:0   skip:39 
fi-bxt-j4205     total:246  pass:224  dwarn:0   dfail:0   fail:0   skip:22 
fi-bxt-t5700     total:79   pass:66   dwarn:0   dfail:0   fail:0   skip:12 
fi-byt-j1900     total:246  pass:219  dwarn:0   dfail:0   fail:0   skip:27 
fi-byt-n2820     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-hsw-4770      total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-hsw-4770r     total:246  pass:227  dwarn:0   dfail:0   fail:0   skip:19 
fi-ivb-3520m     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-ivb-3770      total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-kbl-7500u     total:246  pass:225  dwarn:0   dfail:0   fail:0   skip:21 
fi-skl-6260u     total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-skl-6700hq    total:246  pass:226  dwarn:0   dfail:0   fail:0   skip:20 
fi-skl-6700k     total:246  pass:222  dwarn:3   dfail:0   fail:0   skip:21 
fi-skl-6770hq    total:246  pass:233  dwarn:0   dfail:0   fail:0   skip:13 
fi-snb-2520m     total:246  pass:215  dwarn:0   dfail:0   fail:0   skip:31 
fi-snb-2600      total:246  pass:214  dwarn:0   dfail:0   fail:0   skip:32 

3599f990bcb8b72f9ae062a97140439ab9df22b7 drm-tip: 2017y-01m-23d-09h-16m-39s UTC integration manifest
45ae15c drm/i915: Reset the gpu on takeover
8c06cb6 drm/i915: Split intel_engine allocation and initialisation

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_3577/
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/2] drm/i915: Split intel_engine allocation and initialisation

[Joonas Lahtinen]

On ma, 2017-01-23 at 09:57 +0000, Chris Wilson wrote:
> On Mon, Jan 23, 2017 at 11:41:12AM +0200, Joonas Lahtinen wrote:
> > 
> > On la, 2017-01-21 at 14:50 +0000, Chris Wilson wrote:
> > > 
> > > In order to reset the GPU early on in the module load sequence, we need
> > > to allocate the basic engine structs (to populate the mmio offsets etc).
> > > Currently, the engine initialisation allocates both the base struct and
> > > also allocate auxiliary objects, which depend upon state setup quite
> > > late in the load sequence. We split off the allocation callback for
> > > later and allow ourselves to allocate the engine structs themselves
> > > early.
> > > 
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > <SNIP>
> > 
> > > 
> > > +int intel_engines_init(struct drm_i915_private *dev_priv)
> > > +{
> > 
> > <SNIP>
> > 
> > > 
> > > +	for_each_engine(engine, dev_priv, id) {
> > > +		int (*init)(struct intel_engine_cs *engine) = NULL;
> > > +
> > > +		if (!err) {
> > > +			if (i915.enable_execlists)
> > > +				init = intel_engines[id].init_execlists;
> > > +			else
> > > +				init = intel_engines[id].init_legacy;
> > > +		}
> > > +
> > > +		if (!init || (err = init(engine))) {
> > > +			kfree(engine);
> > > +			dev_priv->engine[id] = NULL;
> > > +			continue;
> > > +		}
> > > +
> > > +		mask |= ENGINE_MASK(id);
> > > +	}
> > 
> > As discussed in IRC, this loop is broken after first erroring init.
> 
> As answered, it is not.
> 
> After an err is set, it and all subsequent engines are freed, and then
> all previously initialised engines are run through the cleanup.

You're correct, I managed to ignore the declaration time initialization
a half a dozen times when going through it.

You could check the "!init" as "err" to make my next review round
easier :P

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