All of lore.kernel.org
 help / color / mirror / Atom feed
* [PATCH v8 01/12] drm/i915: Park before resetting the submission backend
@ 2018-04-09 12:23 Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 02/12] drm/i915: Correctly handle error path in i915_gem_init_hw Michal Wajdeczko
                   ` (12 more replies)
  0 siblings, 13 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

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

As different backends may have different park/unpark callbacks, we
should only ever switch backends (reset_default_submission on wedge
recovery, or on enabling the guc) while parked.

v2: Remove the assert from the guc code, as we are currently trying to
modify the engine vfuncs pointer on a live system after reset (not just
wedging). We will just have to hope that the system is balanced.
v3: Rebase onto __i915_gem_park and improve grammar.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_engine_cs.c |  3 +++
 2 files changed, 15 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 28ab0be..dd3e292 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -144,8 +144,6 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
 	if (!i915->gt.awake)
 		return I915_EPOCH_INVALID;
 
-	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
-
 	/*
 	 * Be paranoid and flush a concurrent interrupt to make sure
 	 * we don't reactivate any irq tasklets after parking.
@@ -173,6 +171,7 @@ static u32 __i915_gem_park(struct drm_i915_private *i915)
 
 	intel_runtime_pm_put(i915);
 
+	GEM_BUG_ON(i915->gt.epoch == I915_EPOCH_INVALID);
 	return i915->gt.epoch;
 }
 
@@ -3435,7 +3434,17 @@ bool i915_gem_unset_wedged(struct drm_i915_private *i915)
 		}
 	}
 	i915_retire_requests(i915);
-	GEM_BUG_ON(i915->gt.active_requests);
+
+	/*
+	 * Park before disengaging the old submit mechanism as different
+	 * backends may have different park/unpack callbacks.
+	 *
+	 * We are idle; the idle-worker will be queued, but we need to run
+	 * it now. As we already hold the struct mutex, we can park the GPU
+	 * right away, letting the lazy worker see that we are already active
+	 * again by the time it acquires the mutex.
+	 */
+	__i915_gem_park(i915);
 
 	/*
 	 * Undo nop_submit_request. We prevent all new i915 requests from
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 12486d8..b4ea77a 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1651,6 +1651,9 @@ void intel_engines_reset_default_submission(struct drm_i915_private *i915)
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
+	/* Must be parked first! */
+	GEM_BUG_ON(i915->gt.awake);
+
 	for_each_engine(engine, i915, id)
 		engine->set_default_submission(engine);
 }
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 02/12] drm/i915: Correctly handle error path in i915_gem_init_hw
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c Michal Wajdeczko
                   ` (11 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

In function gem_init_hw() we are calling uc_init_hw() but in case
of error later in function, we missed to call matching uc_fini_hw()

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index dd3e292..26294e8 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5246,9 +5246,15 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 
 	/* Only when the HW is re-initialised, can we replay the requests */
 	ret = __i915_gem_restart_engines(dev_priv);
+	if (ret)
+		goto cleanup_uc;
 out:
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	return ret;
+
+cleanup_uc:
+	intel_uc_fini_hw(dev_priv);
+	goto out;
 }
 
 static int __intel_engines_record_defaults(struct drm_i915_private *i915)
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 02/12] drm/i915: Correctly handle error path in i915_gem_init_hw Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 14:50   ` Sagar Arun Kamble
  2018-04-09 12:23 ` [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw Michal Wajdeczko
                   ` (10 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

We should keep i915_gem_init/fini functions together for easier
tracking of their symmetry.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 20 --------------------
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++++
 3 files changed, 21 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f770be1..854b26c 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -625,26 +625,6 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
 	.can_switch = i915_switcheroo_can_switch,
 };
 
-static void i915_gem_fini(struct drm_i915_private *dev_priv)
-{
-	/* Flush any outstanding unpin_work. */
-	i915_gem_drain_workqueue(dev_priv);
-
-	mutex_lock(&dev_priv->drm.struct_mutex);
-	intel_uc_fini_hw(dev_priv);
-	intel_uc_fini(dev_priv);
-	i915_gem_cleanup_engines(dev_priv);
-	i915_gem_contexts_fini(dev_priv);
-	mutex_unlock(&dev_priv->drm.struct_mutex);
-
-	intel_uc_fini_misc(dev_priv);
-	i915_gem_cleanup_userptr(dev_priv);
-
-	i915_gem_drain_freed_objects(dev_priv);
-
-	WARN_ON(!list_empty(&dev_priv->contexts.list));
-}
-
 static int i915_load_modeset_init(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = to_i915(dev);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 9bca104..f8bc276 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3143,6 +3143,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
 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);
+void i915_gem_fini(struct drm_i915_private *dev_priv);
 void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 26294e8..fb99485 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5520,6 +5520,26 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
+void i915_gem_fini(struct drm_i915_private *dev_priv)
+{
+	/* Flush any outstanding unpin_work. */
+	i915_gem_drain_workqueue(dev_priv);
+
+	mutex_lock(&dev_priv->drm.struct_mutex);
+	intel_uc_fini_hw(dev_priv);
+	intel_uc_fini(dev_priv);
+	i915_gem_cleanup_engines(dev_priv);
+	i915_gem_contexts_fini(dev_priv);
+	mutex_unlock(&dev_priv->drm.struct_mutex);
+
+	intel_uc_fini_misc(dev_priv);
+	i915_gem_cleanup_userptr(dev_priv);
+
+	i915_gem_drain_freed_objects(dev_priv);
+
+	WARN_ON(!list_empty(&dev_priv->contexts.list));
+}
+
 void i915_gem_init_mmio(struct drm_i915_private *i915)
 {
 	i915_gem_sanitize(i915);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 02/12] drm/i915: Correctly handle error path in i915_gem_init_hw Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 15:01   ` Sagar Arun Kamble
  2018-04-09 12:23 ` [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend Michal Wajdeczko
                   ` (9 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

We have i915_gem_init_hw function that on failure requires some
cleanup in uC and then in other places we were trying to do
such cleanup directly. Let's fix that by adding i915_gem_fini_hw
for nice symmetry with init_hw and call it on cleanup paths.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.h |  1 +
 drivers/gpu/drm/i915/i915_gem.c | 13 +++++++++++--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index f8bc276..dbd95a4 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -3144,6 +3144,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
 int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
 void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
 void i915_gem_fini(struct drm_i915_private *dev_priv);
+void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
 void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
 int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
 			   unsigned int flags);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index fb99485..6f71099 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5257,6 +5257,15 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
 	goto out;
 }
 
+void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
+{
+	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
+
+	intel_uc_fini_hw(dev_priv);
+
+	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
+}
+
 static int __intel_engines_record_defaults(struct drm_i915_private *i915)
 {
 	struct i915_gem_context *ctx;
@@ -5482,7 +5491,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 err_init_hw:
 	i915_gem_wait_for_idle(dev_priv, I915_WAIT_LOCKED);
 	i915_gem_contexts_lost(dev_priv);
-	intel_uc_fini_hw(dev_priv);
+	i915_gem_fini_hw(dev_priv);
 err_uc_init:
 	intel_uc_fini(dev_priv);
 err_pm:
@@ -5526,7 +5535,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
 	i915_gem_drain_workqueue(dev_priv);
 
 	mutex_lock(&dev_priv->drm.struct_mutex);
-	intel_uc_fini_hw(dev_priv);
+	i915_gem_fini_hw(dev_priv);
 	intel_uc_fini(dev_priv);
 	i915_gem_cleanup_engines(dev_priv);
 	i915_gem_contexts_fini(dev_priv);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (2 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 15:02   ` Sagar Arun Kamble
  2018-04-09 12:23 ` [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset Michal Wajdeczko
                   ` (8 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

By calling i915_gem_init_hw in i915_gem_resume and not calling
i915_gem_fini_hw in i915_gem_suspend we introduced asymmetry
in init_hw/fini_hw calls. Let's fix that.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 6f71099..ceec5a0 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5061,6 +5061,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.
 	 */
+	i915_gem_fini_hw(dev_priv);
 	intel_uc_sanitize(dev_priv);
 	i915_gem_sanitize(dev_priv);
 
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (3 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:30   ` Chris Wilson
  2018-04-09 12:23 ` [PATCH v8 07/12] drm/i915/guc: Restore symmetric doorbell cleanup Michal Wajdeczko
                   ` (7 subsequent siblings)
  12 siblings, 1 reply; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

By calling in i915_reset only i915_gem_init_hw without previous
i915_gem_fini_hw we introduced asymmetry. Let's fix that.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_drv.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 854b26c..a0a5af0 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -1902,6 +1902,8 @@ void i915_reset(struct drm_i915_private *i915,
 		goto error;
 	}
 
+	i915_gem_fini_hw(i915);
+
 	for (i = 0; i < 3; i++) {
 		ret = intel_gpu_reset(i915, ALL_ENGINES);
 		if (ret == 0)
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 07/12] drm/i915/guc: Restore symmetric doorbell cleanup
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (4 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw Michal Wajdeczko
                   ` (6 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

In commit 9192d4fb811e ("drm/i915/guc: Extract doorbell creation
from client allocation") we introduced asymmetry in doorbell cleanup
to avoid warnings due to failed communication with already reset GuC.
As we improved our reset/unload paths, we can restore symmetry in
doorbell cleanup, as GuC should be still active by now.

Suggested-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_guc_submission.c | 15 +++------------
 1 file changed, 3 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_guc_submission.c b/drivers/gpu/drm/i915/intel_guc_submission.c
index 9712123..3b6d7f5 100644
--- a/drivers/gpu/drm/i915/intel_guc_submission.c
+++ b/drivers/gpu/drm/i915/intel_guc_submission.c
@@ -848,18 +848,9 @@ static int guc_clients_doorbell_init(struct intel_guc *guc)
 
 static void guc_clients_doorbell_fini(struct intel_guc *guc)
 {
-	/*
-	 * By the time we're here, GuC has already been reset.
-	 * Instead of trying (in vain) to communicate with it, let's just
-	 * cleanup the doorbell HW and our internal state.
-	 */
-	if (guc->preempt_client) {
-		__destroy_doorbell(guc->preempt_client);
-		__update_doorbell_desc(guc->preempt_client,
-				       GUC_DOORBELL_INVALID);
-	}
-	__destroy_doorbell(guc->execbuf_client);
-	__update_doorbell_desc(guc->execbuf_client, GUC_DOORBELL_INVALID);
+	if (guc->preempt_client)
+		destroy_doorbell(guc->preempt_client);
+	destroy_doorbell(guc->execbuf_client);
 }
 
 /**
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (5 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 07/12] drm/i915/guc: Restore symmetric doorbell cleanup Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:47   ` Chris Wilson
  2018-04-09 16:01   ` Sagar Arun Kamble
  2018-04-09 12:23 ` [PATCH v8 09/12] drm/i915/uc: Use correct error code for GuC initialization failure Michal Wajdeczko
                   ` (5 subsequent siblings)
  12 siblings, 2 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

As we always call intel_uc_sanitize after every call to
intel_uc_fini_hw we may drop redundant call and sanitize
uC from the fini_hw function.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 2 --
 drivers/gpu/drm/i915/intel_uc.c | 9 +++------
 drivers/gpu/drm/i915/intel_uc.h | 1 -
 3 files changed, 3 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index ceec5a0..decda1a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3077,7 +3077,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
 	}
 
 	i915_gem_revoke_fences(dev_priv);
-	intel_uc_sanitize(dev_priv);
 
 	return err;
 }
@@ -5062,7 +5061,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	 * machine in an unusable condition.
 	 */
 	i915_gem_fini_hw(dev_priv);
-	intel_uc_sanitize(dev_priv);
 	i915_gem_sanitize(dev_priv);
 
 	intel_runtime_pm_put(dev_priv);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 1cffaf7..0439966 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -322,18 +322,13 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
 	intel_guc_fini(guc);
 }
 
-void intel_uc_sanitize(struct drm_i915_private *i915)
+static void __uc_sanitize(struct drm_i915_private *i915)
 {
 	struct intel_guc *guc = &i915->guc;
 	struct intel_huc *huc = &i915->huc;
 
-	if (!USES_GUC(i915))
-		return;
-
 	GEM_BUG_ON(!HAS_GUC(i915));
 
-	guc_disable_communication(guc);
-
 	intel_huc_sanitize(huc);
 	intel_guc_sanitize(guc);
 
@@ -445,6 +440,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
+
+	__uc_sanitize(dev_priv);
 }
 
 int intel_uc_suspend(struct drm_i915_private *i915)
diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
index 25d73ad..64aaf93 100644
--- a/drivers/gpu/drm/i915/intel_uc.h
+++ b/drivers/gpu/drm/i915/intel_uc.h
@@ -33,7 +33,6 @@
 void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
 int intel_uc_init_misc(struct drm_i915_private *dev_priv);
 void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
-void intel_uc_sanitize(struct drm_i915_private *dev_priv);
 int intel_uc_init_hw(struct drm_i915_private *dev_priv);
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
 int intel_uc_init(struct drm_i915_private *dev_priv);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 09/12] drm/i915/uc: Use correct error code for GuC initialization failure
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (6 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 10/12] drm/i915/uc: Use helper functions to detect fw load status Michal Wajdeczko
                   ` (4 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

Since commit 6ca9a2beb54a ("drm/i915: Unwind i915_gem_init() failure")
we believed that we correctly handle all errors encountered during
GuC initialization, including special one that indicates request to
run driver with disabled GPU submission (-EIO).

Unfortunately since commit 121981fafe69 ("drm/i915/guc: Combine
enable_guc_loading|submission modparams") we stopped using that
error code to avoid unwanted fallback to execlist submission mode.

In result any GuC initialization failure was treated as non-recoverable
error leading to driver load abort, so we could not even read related
GuC error log to investigate cause of the problem.

Fix that by always returning -EIO on uC hardware related failure.

v2: don't allow -EIO from uc_init
    don't call uc_fini[_misc] on -EIO
    mark guc fw as failed on hw init failure
    prepare uc_fini_hw to run after earlier -EIO

v3: update comments (Sagar)
    use sanitize functions on failure in init_hw (Michal)
    and also sanitize guc/huc fw in fini_hw (Michal)

v4: rebase
v5: rebase

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Cc: Michal Winiarski <michal.winiarski@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c    | 17 ++++++++++-------
 drivers/gpu/drm/i915/intel_guc.h   |  5 +++++
 drivers/gpu/drm/i915/intel_uc.c    | 15 +++++++++++----
 drivers/gpu/drm/i915/intel_uc_fw.h |  5 +++++
 4 files changed, 31 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index decda1a..532246a 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -5444,8 +5444,10 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	intel_init_gt_powersave(dev_priv);
 
 	ret = intel_uc_init(dev_priv);
-	if (ret)
+	if (ret) {
+		GEM_BUG_ON(ret == -EIO);
 		goto err_pm;
+	}
 
 	ret = i915_gem_init_hw(dev_priv);
 	if (ret)
@@ -5492,7 +5494,8 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	i915_gem_contexts_lost(dev_priv);
 	i915_gem_fini_hw(dev_priv);
 err_uc_init:
-	intel_uc_fini(dev_priv);
+	if (ret != -EIO)
+		intel_uc_fini(dev_priv);
 err_pm:
 	if (ret != -EIO) {
 		intel_cleanup_gt_powersave(dev_priv);
@@ -5506,15 +5509,15 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
 	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
 	mutex_unlock(&dev_priv->drm.struct_mutex);
 
-	intel_uc_fini_misc(dev_priv);
-
-	if (ret != -EIO)
+	if (ret != -EIO) {
+		intel_uc_fini_misc(dev_priv);
 		i915_gem_cleanup_userptr(dev_priv);
+	}
 
 	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,
+		 * Allow engines or uC initialization to fail by marking the GPU
+		 * as wedged. But we only want to do this when the GPU is angry,
 		 * for all other failure, such as an allocation failure, bail.
 		 */
 		if (!i915_terminally_wedged(&dev_priv->gpu_error)) {
diff --git a/drivers/gpu/drm/i915/intel_guc.h b/drivers/gpu/drm/i915/intel_guc.h
index f1265e1..c587068 100644
--- a/drivers/gpu/drm/i915/intel_guc.h
+++ b/drivers/gpu/drm/i915/intel_guc.h
@@ -176,6 +176,11 @@ static inline int intel_guc_sanitize(struct intel_guc *guc)
 	return 0;
 }
 
+static inline bool intel_guc_is_loaded(struct intel_guc *guc)
+{
+	return intel_uc_fw_is_loaded(&guc->fw);
+}
+
 static inline void intel_guc_enable_msg(struct intel_guc *guc, u32 mask)
 {
 	spin_lock_irq(&guc->irq_lock);
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 0439966..7862731 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -332,6 +332,8 @@ static void __uc_sanitize(struct drm_i915_private *i915)
 	intel_huc_sanitize(huc);
 	intel_guc_sanitize(guc);
 
+	GEM_BUG_ON(intel_guc_is_loaded(guc));
+
 	__intel_uc_reset_hw(i915);
 }
 
@@ -420,11 +422,13 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * Note that there is no fallback as either user explicitly asked for
 	 * the GuC or driver default option was to run with the GuC enabled.
 	 */
-	if (GEM_WARN_ON(ret == -EIO))
-		ret = -EINVAL;
-
 	dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret);
-	return ret;
+
+	/* Sanitize GuC/HuC to avoid clean-up on wedged */
+	__uc_sanitize(dev_priv);
+
+	/* We want to disable GPU submission but keep KMS alive */
+	return -EIO;
 }
 
 void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
@@ -436,6 +440,9 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
 
 	GEM_BUG_ON(!HAS_GUC(dev_priv));
 
+	if (!intel_guc_is_loaded(guc))
+		return;
+
 	if (USES_GUC_SUBMISSION(dev_priv))
 		intel_guc_submission_disable(guc);
 
diff --git a/drivers/gpu/drm/i915/intel_uc_fw.h b/drivers/gpu/drm/i915/intel_uc_fw.h
index dc33b12..77ad2aa 100644
--- a/drivers/gpu/drm/i915/intel_uc_fw.h
+++ b/drivers/gpu/drm/i915/intel_uc_fw.h
@@ -121,6 +121,11 @@ static inline void intel_uc_fw_sanitize(struct intel_uc_fw *uc_fw)
 		uc_fw->load_status = INTEL_UC_FIRMWARE_PENDING;
 }
 
+static inline bool intel_uc_fw_is_loaded(struct intel_uc_fw *uc_fw)
+{
+	return uc_fw->load_status == INTEL_UC_FIRMWARE_SUCCESS;
+}
+
 /**
  * intel_uc_fw_get_upload_size() - Get size of firmware needed to be uploaded.
  * @uc_fw: uC firmware.
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 10/12] drm/i915/uc: Use helper functions to detect fw load status
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (7 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 09/12] drm/i915/uc: Use correct error code for GuC initialization failure Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 11/12] drm/i915/uc: Trivial s/dev_priv/i915 in intel_uc.c Michal Wajdeczko
                   ` (3 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

We don't have to check load status values.

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_huc.c | 2 +-
 drivers/gpu/drm/i915/intel_uc.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_huc.c b/drivers/gpu/drm/i915/intel_huc.c
index 2912852..975ae61 100644
--- a/drivers/gpu/drm/i915/intel_huc.c
+++ b/drivers/gpu/drm/i915/intel_huc.c
@@ -51,7 +51,7 @@ int intel_huc_auth(struct intel_huc *huc)
 	u32 status;
 	int ret;
 
-	if (huc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_uc_fw_is_loaded(&huc->fw))
 		return -ENOEXEC;
 
 	vma = i915_gem_object_ggtt_pin(huc->fw.obj, NULL, 0, 0,
diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index 7862731..ed39273 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -459,7 +459,7 @@ int intel_uc_suspend(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return 0;
 
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_guc_is_loaded(guc))
 		return 0;
 
 	err = intel_guc_suspend(guc);
@@ -481,7 +481,7 @@ int intel_uc_resume(struct drm_i915_private *i915)
 	if (!USES_GUC(i915))
 		return 0;
 
-	if (guc->fw.load_status != INTEL_UC_FIRMWARE_SUCCESS)
+	if (!intel_guc_is_loaded(guc))
 		return 0;
 
 	gen9_enable_guc_interrupts(i915);
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 11/12] drm/i915/uc: Trivial s/dev_priv/i915 in intel_uc.c
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (8 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 10/12] drm/i915/uc: Use helper functions to detect fw load status Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:23 ` [PATCH v8 12/12] HAX: Enable GuC for CI Michal Wajdeczko
                   ` (2 subsequent siblings)
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

Some functions already use i915 name instead of dev_priv.
Let's rename this param in all remaining functions, except
those that still use legacy macros.

v2: don't forget about function descriptions (Sagar)
v3: rebased

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
---
 drivers/gpu/drm/i915/intel_uc.c | 133 ++++++++++++++++++++--------------------
 1 file changed, 66 insertions(+), 67 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
index ed39273..ccf75aa 100644
--- a/drivers/gpu/drm/i915/intel_uc.c
+++ b/drivers/gpu/drm/i915/intel_uc.c
@@ -50,10 +50,10 @@ static int __intel_uc_reset_hw(struct drm_i915_private *dev_priv)
 	return ret;
 }
 
-static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
+static int __get_platform_enable_guc(struct drm_i915_private *i915)
 {
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+	struct intel_uc_fw *guc_fw = &i915->guc.fw;
+	struct intel_uc_fw *huc_fw = &i915->huc.fw;
 	int enable_guc = 0;
 
 	/* Default is to enable GuC/HuC if we know their firmwares */
@@ -67,11 +67,11 @@ static int __get_platform_enable_guc(struct drm_i915_private *dev_priv)
 	return enable_guc;
 }
 
-static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
+static int __get_default_guc_log_level(struct drm_i915_private *i915)
 {
 	int guc_log_level;
 
-	if (!HAS_GUC(dev_priv) || !intel_uc_is_using_guc())
+	if (!HAS_GUC(i915) || !intel_uc_is_using_guc())
 		guc_log_level = GUC_LOG_LEVEL_DISABLED;
 	else if (IS_ENABLED(CONFIG_DRM_I915_DEBUG) ||
 		 IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM))
@@ -86,7 +86,7 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
 
 /**
  * sanitize_options_early - sanitize uC related modparam options
- * @dev_priv: device private
+ * @i915: device private
  *
  * In case of "enable_guc" option this function will attempt to modify
  * it only if it was initially set to "auto(-1)". Default value for this
@@ -101,14 +101,14 @@ static int __get_default_guc_log_level(struct drm_i915_private *dev_priv)
  * unless GuC is enabled on given platform and the driver is compiled with
  * debug config when this modparam will default to "enable(1..4)".
  */
-static void sanitize_options_early(struct drm_i915_private *dev_priv)
+static void sanitize_options_early(struct drm_i915_private *i915)
 {
-	struct intel_uc_fw *guc_fw = &dev_priv->guc.fw;
-	struct intel_uc_fw *huc_fw = &dev_priv->huc.fw;
+	struct intel_uc_fw *guc_fw = &i915->guc.fw;
+	struct intel_uc_fw *huc_fw = &i915->huc.fw;
 
 	/* A negative value means "use platform default" */
 	if (i915_modparams.enable_guc < 0)
-		i915_modparams.enable_guc = __get_platform_enable_guc(dev_priv);
+		i915_modparams.enable_guc = __get_platform_enable_guc(i915);
 
 	DRM_DEBUG_DRIVER("enable_guc=%d (submission:%s huc:%s)\n",
 			 i915_modparams.enable_guc,
@@ -119,28 +119,28 @@ static void sanitize_options_early(struct drm_i915_private *dev_priv)
 	if (intel_uc_is_using_guc() && !intel_uc_fw_is_selected(guc_fw)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "enable_guc", i915_modparams.enable_guc,
-			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
-					      "no GuC firmware");
+			 !HAS_GUC(i915) ? "no GuC hardware" :
+					  "no GuC firmware");
 	}
 
 	/* Verify HuC firmware availability */
 	if (intel_uc_is_using_huc() && !intel_uc_fw_is_selected(huc_fw)) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "enable_guc", i915_modparams.enable_guc,
-			 !HAS_HUC(dev_priv) ? "no HuC hardware" :
-					      "no HuC firmware");
+			 !HAS_HUC(i915) ? "no HuC hardware" :
+					  "no HuC firmware");
 	}
 
 	/* A negative value means "use platform/config default" */
 	if (i915_modparams.guc_log_level < 0)
 		i915_modparams.guc_log_level =
-			__get_default_guc_log_level(dev_priv);
+			__get_default_guc_log_level(i915);
 
 	if (i915_modparams.guc_log_level > 0 && !intel_uc_is_using_guc()) {
 		DRM_WARN("Incompatible option detected: %s=%d, %s!\n",
 			 "guc_log_level", i915_modparams.guc_log_level,
-			 !HAS_GUC(dev_priv) ? "no GuC hardware" :
-					      "GuC not enabled");
+			 !HAS_GUC(i915) ? "no GuC hardware" :
+					  "GuC not enabled");
 		i915_modparams.guc_log_level = 0;
 	}
 
@@ -195,15 +195,14 @@ void intel_uc_cleanup_early(struct drm_i915_private *i915)
 
 /**
  * intel_uc_init_mmio - setup uC MMIO access
- *
- * @dev_priv: device private
+ * @i915: device private
  *
  * Setup minimal state necessary for MMIO accesses later in the
  * initialization sequence.
  */
-void intel_uc_init_mmio(struct drm_i915_private *dev_priv)
+void intel_uc_init_mmio(struct drm_i915_private *i915)
 {
-	intel_guc_init_send_regs(&dev_priv->guc);
+	intel_guc_init_send_regs(&i915->guc);
 }
 
 static void guc_capture_load_err_log(struct intel_guc *guc)
@@ -225,11 +224,11 @@ static void guc_free_load_err_log(struct intel_guc *guc)
 
 static int guc_enable_communication(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_i915_private *i915 = guc_to_i915(guc);
 
-	gen9_enable_guc_interrupts(dev_priv);
+	gen9_enable_guc_interrupts(i915);
 
-	if (HAS_GUC_CT(dev_priv))
+	if (HAS_GUC_CT(i915))
 		return intel_guc_ct_enable(&guc->ct);
 
 	guc->send = intel_guc_send_mmio;
@@ -239,23 +238,23 @@ static int guc_enable_communication(struct intel_guc *guc)
 
 static void guc_disable_communication(struct intel_guc *guc)
 {
-	struct drm_i915_private *dev_priv = guc_to_i915(guc);
+	struct drm_i915_private *i915 = guc_to_i915(guc);
 
-	if (HAS_GUC_CT(dev_priv))
+	if (HAS_GUC_CT(i915))
 		intel_guc_ct_disable(&guc->ct);
 
-	gen9_disable_guc_interrupts(dev_priv);
+	gen9_disable_guc_interrupts(i915);
 
 	guc->send = intel_guc_send_nop;
 	guc->handler = intel_guc_to_host_event_handler_nop;
 }
 
-int intel_uc_init_misc(struct drm_i915_private *dev_priv)
+int intel_uc_init_misc(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc *guc = &i915->guc;
 	int ret;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return 0;
 
 	intel_guc_init_ggtt_pin_bias(guc);
@@ -267,32 +266,32 @@ int intel_uc_init_misc(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-void intel_uc_fini_misc(struct drm_i915_private *dev_priv)
+void intel_uc_fini_misc(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc *guc = &i915->guc;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return;
 
 	intel_guc_fini_wq(guc);
 }
 
-int intel_uc_init(struct drm_i915_private *dev_priv)
+int intel_uc_init(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc *guc = &i915->guc;
 	int ret;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return 0;
 
-	if (!HAS_GUC(dev_priv))
+	if (!HAS_GUC(i915))
 		return -ENODEV;
 
 	ret = intel_guc_init(guc);
 	if (ret)
 		return ret;
 
-	if (USES_GUC_SUBMISSION(dev_priv)) {
+	if (USES_GUC_SUBMISSION(i915)) {
 		/*
 		 * This is stuff we need to have available at fw load time
 		 * if we are planning to enable submission later
@@ -307,16 +306,16 @@ int intel_uc_init(struct drm_i915_private *dev_priv)
 	return 0;
 }
 
-void intel_uc_fini(struct drm_i915_private *dev_priv)
+void intel_uc_fini(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc *guc = &i915->guc;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
+	GEM_BUG_ON(!HAS_GUC(i915));
 
-	if (USES_GUC_SUBMISSION(dev_priv))
+	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_fini(guc);
 
 	intel_guc_fini(guc);
@@ -337,22 +336,22 @@ static void __uc_sanitize(struct drm_i915_private *i915)
 	__intel_uc_reset_hw(i915);
 }
 
-int intel_uc_init_hw(struct drm_i915_private *dev_priv)
+int intel_uc_init_hw(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
-	struct intel_huc *huc = &dev_priv->huc;
+	struct intel_guc *guc = &i915->guc;
+	struct intel_huc *huc = &i915->huc;
 	int ret, attempts;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return 0;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
+	GEM_BUG_ON(!HAS_GUC(i915));
 
-	gen9_reset_guc_interrupts(dev_priv);
+	gen9_reset_guc_interrupts(i915);
 
 	/* WaEnableuKernelHeaderValidFix:skl */
 	/* WaEnableGuCBootHashCheckNotSet:skl,bxt,kbl */
-	if (IS_GEN9(dev_priv))
+	if (IS_GEN9(i915))
 		attempts = 3;
 	else
 		attempts = 1;
@@ -362,11 +361,11 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 		 * Always reset the GuC just before (re)loading, so
 		 * that the state and timing are fairly predictable
 		 */
-		ret = __intel_uc_reset_hw(dev_priv);
+		ret = __intel_uc_reset_hw(i915);
 		if (ret)
 			goto err_out;
 
-		if (USES_HUC(dev_priv)) {
+		if (USES_HUC(i915)) {
 			ret = intel_huc_fw_upload(huc);
 			if (ret)
 				goto err_out;
@@ -389,24 +388,24 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_log_capture;
 
-	if (USES_HUC(dev_priv)) {
+	if (USES_HUC(i915)) {
 		ret = intel_huc_auth(huc);
 		if (ret)
 			goto err_communication;
 	}
 
-	if (USES_GUC_SUBMISSION(dev_priv)) {
+	if (USES_GUC_SUBMISSION(i915)) {
 		ret = intel_guc_submission_enable(guc);
 		if (ret)
 			goto err_communication;
 	}
 
-	dev_info(dev_priv->drm.dev, "GuC firmware version %u.%u\n",
+	dev_info(i915->drm.dev, "GuC firmware version %u.%u\n",
 		 guc->fw.major_ver_found, guc->fw.minor_ver_found);
-	dev_info(dev_priv->drm.dev, "GuC submission %s\n",
-		 enableddisabled(USES_GUC_SUBMISSION(dev_priv)));
-	dev_info(dev_priv->drm.dev, "HuC %s\n",
-		 enableddisabled(USES_HUC(dev_priv)));
+	dev_info(i915->drm.dev, "GuC submission %s\n",
+		 enableddisabled(USES_GUC_SUBMISSION(i915)));
+	dev_info(i915->drm.dev, "HuC %s\n",
+		 enableddisabled(USES_HUC(i915)));
 
 	return 0;
 
@@ -422,33 +421,33 @@ int intel_uc_init_hw(struct drm_i915_private *dev_priv)
 	 * Note that there is no fallback as either user explicitly asked for
 	 * the GuC or driver default option was to run with the GuC enabled.
 	 */
-	dev_err(dev_priv->drm.dev, "GuC initialization failed %d\n", ret);
+	dev_err(i915->drm.dev, "GuC initialization failed %d\n", ret);
 
 	/* Sanitize GuC/HuC to avoid clean-up on wedged */
-	__uc_sanitize(dev_priv);
+	__uc_sanitize(i915);
 
 	/* We want to disable GPU submission but keep KMS alive */
 	return -EIO;
 }
 
-void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
+void intel_uc_fini_hw(struct drm_i915_private *i915)
 {
-	struct intel_guc *guc = &dev_priv->guc;
+	struct intel_guc *guc = &i915->guc;
 
-	if (!USES_GUC(dev_priv))
+	if (!USES_GUC(i915))
 		return;
 
-	GEM_BUG_ON(!HAS_GUC(dev_priv));
+	GEM_BUG_ON(!HAS_GUC(i915));
 
 	if (!intel_guc_is_loaded(guc))
 		return;
 
-	if (USES_GUC_SUBMISSION(dev_priv))
+	if (USES_GUC_SUBMISSION(i915))
 		intel_guc_submission_disable(guc);
 
 	guc_disable_communication(guc);
 
-	__uc_sanitize(dev_priv);
+	__uc_sanitize(i915);
 }
 
 int intel_uc_suspend(struct drm_i915_private *i915)
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* [PATCH v8 12/12] HAX: Enable GuC for CI
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (9 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 11/12] drm/i915/uc: Trivial s/dev_priv/i915 in intel_uc.c Michal Wajdeczko
@ 2018-04-09 12:23 ` Michal Wajdeczko
  2018-04-09 12:51 ` ✓ Fi.CI.BAT: success for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend Patchwork
  2018-04-09 15:09 ` ✗ Fi.CI.IGT: failure " Patchwork
  12 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 12:23 UTC (permalink / raw)
  To: intel-gfx

Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
---
 drivers/gpu/drm/i915/i915_params.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_params.h b/drivers/gpu/drm/i915/i915_params.h
index c963603..53037b5 100644
--- a/drivers/gpu/drm/i915/i915_params.h
+++ b/drivers/gpu/drm/i915/i915_params.h
@@ -47,7 +47,7 @@
 	param(int, disable_power_well, -1) \
 	param(int, enable_ips, 1) \
 	param(int, invert_brightness, 0) \
-	param(int, enable_guc, 0) \
+	param(int, enable_guc, -1) \
 	param(int, guc_log_level, -1) \
 	param(char *, guc_firmware_path, NULL) \
 	param(char *, huc_firmware_path, NULL) \
-- 
1.9.1

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset
  2018-04-09 12:23 ` [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset Michal Wajdeczko
@ 2018-04-09 12:30   ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-04-09 12:30 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx

Quoting Michal Wajdeczko (2018-04-09 13:23:26)
> By calling in i915_reset only i915_gem_init_hw without previous
> i915_gem_fini_hw we introduced asymmetry. Let's fix that.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>

> ---
>  drivers/gpu/drm/i915/i915_drv.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 854b26c..a0a5af0 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -1902,6 +1902,8 @@ void i915_reset(struct drm_i915_private *i915,
>                 goto error;
>         }
>  
> +       i915_gem_fini_hw(i915);
> +
>         for (i = 0; i < 3; i++) {
>                 ret = intel_gpu_reset(i915, ALL_ENGINES);
>                 if (ret == 0)

I still have a feeling that i915_gem_reset() will cause trouble. Hmm,
the wedged -> recovery path should be triggering the submission from
inside i915_gem_reset. So it should be exploding already...

I think where we use GEM_BUG_ON(!gt.awake) in execlists, we want a
GEM_BUG_ON(!irq_pinned) in guc_submission_tasklet().
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw
  2018-04-09 12:23 ` [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw Michal Wajdeczko
@ 2018-04-09 12:47   ` Chris Wilson
  2018-04-09 14:10     ` Michal Wajdeczko
  2018-04-09 16:01   ` Sagar Arun Kamble
  1 sibling, 1 reply; 24+ messages in thread
From: Chris Wilson @ 2018-04-09 12:47 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx

Quoting Michal Wajdeczko (2018-04-09 13:23:28)
> As we always call intel_uc_sanitize after every call to
> intel_uc_fini_hw we may drop redundant call and sanitize
> uC from the fini_hw function.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>

Not that it matters, since doing it before losing control or on resume
is the same from our pov, but I've always pencilled in sanitize as being
done on takeover (i.e. before init). Why do you favour after fini?

Gut feeling prefers keeping it as a separate step rather rolling it up
into init/fini. But that's just because before we did sanitize
elsewhere, we had many strange bugs and those bugs have left their
scars (so I like seeing sanitize, it reminds me of dead bugs).
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread

* ✓ Fi.CI.BAT: success for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (10 preceding siblings ...)
  2018-04-09 12:23 ` [PATCH v8 12/12] HAX: Enable GuC for CI Michal Wajdeczko
@ 2018-04-09 12:51 ` Patchwork
  2018-04-09 15:09 ` ✗ Fi.CI.IGT: failure " Patchwork
  12 siblings, 0 replies; 24+ messages in thread
From: Patchwork @ 2018-04-09 12:51 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
URL   : https://patchwork.freedesktop.org/series/41365/
State : success

== Summary ==

Series 41365v1 series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
https://patchwork.freedesktop.org/api/1.0/series/41365/revisions/1/mbox/

---- Known issues:

Test gem_mmap_gtt:
        Subgroup basic-small-bo-tiledx:
                fail       -> PASS       (fi-gdg-551) fdo#102575
Test kms_flip:
        Subgroup basic-flip-vs-wf_vblank:
                pass       -> FAIL       (fi-cfl-s3) fdo#100368
Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

fdo#102575 https://bugs.freedesktop.org/show_bug.cgi?id=102575
fdo#100368 https://bugs.freedesktop.org/show_bug.cgi?id=100368
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:435s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:447s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:380s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:541s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:296s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:513s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:517s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:526s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:506s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:410s
fi-cfl-s3        total:285  pass:258  dwarn:0   dfail:0   fail:1   skip:26  time:554s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:511s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:583s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:426s
fi-gdg-551       total:285  pass:177  dwarn:0   dfail:0   fail:0   skip:108 time:314s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:536s
fi-glk-j4005     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:409s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:420s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:471s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:434s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:461s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:508s
fi-pnv-d510      total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:670s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:444s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:540s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:498s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:508s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:431s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:443s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:586s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:405s

6eef2aa99feb6e37e6252c0a0ddb78966a0b23dd drm-tip: 2018y-04m-09d-11h-44m-50s UTC integration manifest
058d4d58c467 HAX: Enable GuC for CI
2f9999326fb7 drm/i915/uc: Trivial s/dev_priv/i915 in intel_uc.c
38d4bebc3c54 drm/i915/uc: Use helper functions to detect fw load status
ecd487066952 drm/i915/uc: Use correct error code for GuC initialization failure
174c75f27d90 drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw
8f30a658e3ce drm/i915/guc: Restore symmetric doorbell cleanup
2298ffab92ec drm/i915: Add i915_gem_fini_hw to i915_reset
2d5745d4f14e drm/i915: Add i915_gem_fini_hw to i915_gem_suspend
d70c468442f3 drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw
2ae7ae86b3c4 drm/i915: Move i915_gem_fini to i915_gem.c
bcc6baad8bda drm/i915: Correctly handle error path in i915_gem_init_hw
7d8134ca6ba3 drm/i915: Park before resetting the submission backend

== Logs ==

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw
  2018-04-09 12:47   ` Chris Wilson
@ 2018-04-09 14:10     ` Michal Wajdeczko
  0 siblings, 0 replies; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 14:10 UTC (permalink / raw)
  To: intel-gfx, Chris Wilson

On Mon, 09 Apr 2018 14:47:24 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2018-04-09 13:23:28)
>> As we always call intel_uc_sanitize after every call to
>> intel_uc_fini_hw we may drop redundant call and sanitize
>> uC from the fini_hw function.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
>> Cc: Chris Wilson <chris@chris-wilson.co.uk>
>
> Not that it matters, since doing it before losing control or on resume
> is the same from our pov, but I've always pencilled in sanitize as being
> done on takeover (i.e. before init).

In intel_uc_init_hw we are already doing some semi-sanitization (thanks
to __intel_uc_reset_hw), but maybe to be more explicit, we should add
call to __uc_sanitize() in intel_uc_init_mmio() ?

> Why do you favour after fini?

Hmm, not at all, I would call it just more explicit

>
> Gut feeling prefers keeping it as a separate step rather rolling it up
> into init/fini. But that's just because before we did sanitize
> elsewhere, we had many strange bugs and those bugs have left their
> scars (so I like seeing sanitize, it reminds me of dead bugs).

As now we have symmetrical inits/finis that cover all critical paths,
I don't think we need separate 'sanitize' step that could be called
any time/any place.

I assume extra __sanitize in uc_init_mmio should be enough reminder.

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c
  2018-04-09 12:23 ` [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c Michal Wajdeczko
@ 2018-04-09 14:50   ` Sagar Arun Kamble
  0 siblings, 0 replies; 24+ messages in thread
From: Sagar Arun Kamble @ 2018-04-09 14:50 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 4/9/2018 5:53 PM, Michal Wajdeczko wrote:
> We should keep i915_gem_init/fini functions together for easier
> tracking of their symmetry.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.c | 20 --------------------
>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>   drivers/gpu/drm/i915/i915_gem.c | 20 ++++++++++++++++++++
>   3 files changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index f770be1..854b26c 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -625,26 +625,6 @@ static bool i915_switcheroo_can_switch(struct pci_dev *pdev)
>   	.can_switch = i915_switcheroo_can_switch,
>   };
>   
> -static void i915_gem_fini(struct drm_i915_private *dev_priv)
> -{
> -	/* Flush any outstanding unpin_work. */
> -	i915_gem_drain_workqueue(dev_priv);
> -
> -	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_uc_fini_hw(dev_priv);
> -	intel_uc_fini(dev_priv);
> -	i915_gem_cleanup_engines(dev_priv);
> -	i915_gem_contexts_fini(dev_priv);
> -	mutex_unlock(&dev_priv->drm.struct_mutex);
> -
> -	intel_uc_fini_misc(dev_priv);
> -	i915_gem_cleanup_userptr(dev_priv);
> -
> -	i915_gem_drain_freed_objects(dev_priv);
> -
> -	WARN_ON(!list_empty(&dev_priv->contexts.list));
> -}
> -
>   static int i915_load_modeset_init(struct drm_device *dev)
>   {
>   	struct drm_i915_private *dev_priv = to_i915(dev);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 9bca104..f8bc276 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3143,6 +3143,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>   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);
> +void i915_gem_fini(struct drm_i915_private *dev_priv);
>   void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>   			   unsigned int flags);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 26294e8..fb99485 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5520,6 +5520,26 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   	return ret;
>   }
>   
> +void i915_gem_fini(struct drm_i915_private *dev_priv)
> +{
> +	/* Flush any outstanding unpin_work. */
> +	i915_gem_drain_workqueue(dev_priv);
> +
> +	mutex_lock(&dev_priv->drm.struct_mutex);
> +	intel_uc_fini_hw(dev_priv);
> +	intel_uc_fini(dev_priv);
> +	i915_gem_cleanup_engines(dev_priv);
> +	i915_gem_contexts_fini(dev_priv);
> +	mutex_unlock(&dev_priv->drm.struct_mutex);
> +
> +	intel_uc_fini_misc(dev_priv);
> +	i915_gem_cleanup_userptr(dev_priv);
> +
> +	i915_gem_drain_freed_objects(dev_priv);
> +
> +	WARN_ON(!list_empty(&dev_priv->contexts.list));
> +}
> +
>   void i915_gem_init_mmio(struct drm_i915_private *i915)
>   {
>   	i915_gem_sanitize(i915);

-- 
Thanks,
Sagar

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw
  2018-04-09 12:23 ` [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw Michal Wajdeczko
@ 2018-04-09 15:01   ` Sagar Arun Kamble
  0 siblings, 0 replies; 24+ messages in thread
From: Sagar Arun Kamble @ 2018-04-09 15:01 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 4/9/2018 5:53 PM, Michal Wajdeczko wrote:
> We have i915_gem_init_hw function that on failure requires some
> cleanup in uC and then in other places we were trying to do
> such cleanup directly. Let's fix that by adding i915_gem_fini_hw
> for nice symmetry with init_hw and call it on cleanup paths.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_drv.h |  1 +
>   drivers/gpu/drm/i915/i915_gem.c | 13 +++++++++++--
>   2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index f8bc276..dbd95a4 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -3144,6 +3144,7 @@ void i915_gem_reset_engine(struct intel_engine_cs *engine,
>   int __must_check i915_gem_init_hw(struct drm_i915_private *dev_priv);
>   void i915_gem_init_swizzling(struct drm_i915_private *dev_priv);
>   void i915_gem_fini(struct drm_i915_private *dev_priv);
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv);
>   void i915_gem_cleanup_engines(struct drm_i915_private *dev_priv);
>   int i915_gem_wait_for_idle(struct drm_i915_private *dev_priv,
>   			   unsigned int flags);
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index fb99485..6f71099 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5257,6 +5257,15 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
>   	goto out;
>   }
>   
> +void i915_gem_fini_hw(struct drm_i915_private *dev_priv)
> +{
> +	intel_uncore_forcewake_get(dev_priv, FORCEWAKE_ALL);
> +
> +	intel_uc_fini_hw(dev_priv);
> +
> +	intel_uncore_forcewake_put(dev_priv, FORCEWAKE_ALL);
> +}
> +
>   static int __intel_engines_record_defaults(struct drm_i915_private *i915)
>   {
>   	struct i915_gem_context *ctx;
> @@ -5482,7 +5491,7 @@ int i915_gem_init(struct drm_i915_private *dev_priv)
>   err_init_hw:
>   	i915_gem_wait_for_idle(dev_priv, I915_WAIT_LOCKED);
>   	i915_gem_contexts_lost(dev_priv);
> -	intel_uc_fini_hw(dev_priv);
> +	i915_gem_fini_hw(dev_priv);
>   err_uc_init:
>   	intel_uc_fini(dev_priv);
>   err_pm:
> @@ -5526,7 +5535,7 @@ void i915_gem_fini(struct drm_i915_private *dev_priv)
>   	i915_gem_drain_workqueue(dev_priv);
>   
>   	mutex_lock(&dev_priv->drm.struct_mutex);
> -	intel_uc_fini_hw(dev_priv);
> +	i915_gem_fini_hw(dev_priv);
>   	intel_uc_fini(dev_priv);
>   	i915_gem_cleanup_engines(dev_priv);
>   	i915_gem_contexts_fini(dev_priv);

-- 
Thanks,
Sagar

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend
  2018-04-09 12:23 ` [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend Michal Wajdeczko
@ 2018-04-09 15:02   ` Sagar Arun Kamble
  0 siblings, 0 replies; 24+ messages in thread
From: Sagar Arun Kamble @ 2018-04-09 15:02 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 4/9/2018 5:53 PM, Michal Wajdeczko wrote:
> By calling i915_gem_init_hw in i915_gem_resume and not calling
> i915_gem_fini_hw in i915_gem_suspend we introduced asymmetry
> in init_hw/fini_hw calls. Let's fix that.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 1 +
>   1 file changed, 1 insertion(+)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index 6f71099..ceec5a0 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -5061,6 +5061,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.
>   	 */
> +	i915_gem_fini_hw(dev_priv);
>   	intel_uc_sanitize(dev_priv);
>   	i915_gem_sanitize(dev_priv);
>   

-- 
Thanks,
Sagar

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* ✗ Fi.CI.IGT: failure for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
  2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
                   ` (11 preceding siblings ...)
  2018-04-09 12:51 ` ✓ Fi.CI.BAT: success for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend Patchwork
@ 2018-04-09 15:09 ` Patchwork
  2018-04-09 15:32   ` Michal Wajdeczko
  12 siblings, 1 reply; 24+ messages in thread
From: Patchwork @ 2018-04-09 15:09 UTC (permalink / raw)
  To: Michal Wajdeczko; +Cc: intel-gfx

== Series Details ==

Series: series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
URL   : https://patchwork.freedesktop.org/series/41365/
State : failure

== Summary ==

---- Possible new issues:

Test drm_mm:
        Subgroup sanitycheck:
                pass       -> INCOMPLETE (shard-apl)
Test drv_hangman:
        Subgroup error-state-capture-blt:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup error-state-capture-bsd:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup error-state-capture-render:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup error-state-capture-vebox:
                pass       -> INCOMPLETE (shard-apl)
Test drv_selftest:
        Subgroup live_guc:
                pass       -> SKIP       (shard-apl)
        Subgroup live_hangcheck:
                pass       -> DMESG-FAIL (shard-apl)
Test gem_eio:
        Subgroup execbuf:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup in-flight-external:
                pass       -> INCOMPLETE (shard-apl)
Test gem_mocs_settings:
        Subgroup mocs-reset-dirty-render:
                pass       -> INCOMPLETE (shard-apl)
Test gem_request_retire:
        Subgroup retire-vma-not-inactive:
                pass       -> INCOMPLETE (shard-apl)
Test gem_workarounds:
        Subgroup reset-context:
                pass       -> INCOMPLETE (shard-apl)
Test kms_vblank:
        Subgroup pipe-a-query-idle-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-a-ts-continuation-idle-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-a-wait-busy-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-a-wait-forked-busy-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-a-wait-idle-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-b-query-forked-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-c-query-busy-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-c-query-forked-busy-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-c-query-forked-hang:
                pass       -> INCOMPLETE (shard-apl)
        Subgroup pipe-c-ts-continuation-idle-hang:
                pass       -> INCOMPLETE (shard-apl)
Test perf:
        Subgroup gen8-unprivileged-single-ctx-counters:
                pass       -> FAIL       (shard-apl)

---- Known issues:

Test drv_missed_irq:
                pass       -> SKIP       (shard-apl) fdo#103199
Test gem_eio:
        Subgroup in-flight-suspend:
                pass       -> INCOMPLETE (shard-apl) fdo#103375
Test kms_flip:
        Subgroup flip-vs-expired-vblank:
                fail       -> PASS       (shard-hsw) fdo#102887
        Subgroup modeset-vs-vblank-race-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#103060
Test kms_plane_multiple:
        Subgroup atomic-pipe-c-tiling-x:
                pass       -> FAIL       (shard-apl) fdo#103166
Test kms_rotation_crc:
        Subgroup sprite-rotation-90:
                fail       -> PASS       (shard-apl) fdo#103925

fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925

shard-apl        total:1541 pass:1003 dwarn:1   dfail:1   fail:9   skip:497 time:2569s
shard-hsw        total:2680 pass:1784 dwarn:1   dfail:0   fail:3   skip:891 time:11411s
Blacklisted hosts:
shard-kbl        total:1439 pass:1014 dwarn:1   dfail:1   fail:6   skip:386 time:1390s
shard-snb        total:2680 pass:1378 dwarn:1   dfail:0   fail:3   skip:1298 time:6927s

== Logs ==

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: ✗ Fi.CI.IGT: failure for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
  2018-04-09 15:09 ` ✗ Fi.CI.IGT: failure " Patchwork
@ 2018-04-09 15:32   ` Michal Wajdeczko
  2018-04-10  5:32     ` Sagar Arun Kamble
  0 siblings, 1 reply; 24+ messages in thread
From: Michal Wajdeczko @ 2018-04-09 15:32 UTC (permalink / raw)
  To: Patchwork, intel-gfx

On Mon, 09 Apr 2018 17:09:18 +0200, Patchwork  
<patchwork@emeril.freedesktop.org> wrote:

> == Series Details ==
>
> Series: series starting with [v8,01/12] drm/i915: Park before resetting  
> the submission backend
> URL   : https://patchwork.freedesktop.org/series/41365/
> State : failure
>
> == Summary ==
>
> ---- Possible new issues:

two variants:

>
> Test drm_mm:
>         Subgroup sanitycheck:
>                 pass       -> INCOMPLETE (shard-apl)

#1

<0>[  400.245461] drv_self-5775    1.... 400208508us :  
intel_guc_submission_disable: intel_guc_submission_disable:1255  
GEM_BUG_ON(dev_priv->gt.awake)

<4>[  400.245871] Call Trace:
<4>[  400.245959]  intel_uc_fini_hw+0x4b/0xe0 [i915]
<4>[  400.246047]  i915_gem_fini_hw+0x16/0x30 [i915]
<4>[  400.246129]  i915_reset+0x1e8/0x2b0 [i915]
<4>[  400.246222]  igt_global_reset+0x38/0xe0 [i915]

> Test drv_hangman:
>         Subgroup error-state-capture-blt:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup error-state-capture-bsd:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup error-state-capture-render:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup error-state-capture-vebox:
>                 pass       -> INCOMPLETE (shard-apl)
> Test drv_selftest:
>         Subgroup live_guc:
>                 pass       -> SKIP       (shard-apl)
>         Subgroup live_hangcheck:
>                 pass       -> DMESG-FAIL (shard-apl)
> Test gem_eio:
>         Subgroup execbuf:
>                 pass       -> INCOMPLETE (shard-apl)

#2:

<3>[  227.833798] intel_engine_unpin_breadcrumbs_irq:219  
GEM_BUG_ON(!b->irq_enabled)

<4>[  227.834607] Call Trace:
<4>[  227.834691]  intel_engines_park+0xef/0x180 [i915]
<4>[  227.834709]  ? synchronize_irq+0x3e/0xb0
<4>[  227.834781]  __i915_gem_park+0x3e/0x160 [i915]
<4>[  227.834850]  i915_gem_idle_work_handler+0x1cd/0x220 [i915]
<4>[  227.834868]  process_one_work+0x21a/0x640


>         Subgroup in-flight-external:
>                 pass       -> INCOMPLETE (shard-apl)
> Test gem_mocs_settings:
>         Subgroup mocs-reset-dirty-render:
>                 pass       -> INCOMPLETE (shard-apl)
> Test gem_request_retire:
>         Subgroup retire-vma-not-inactive:
>                 pass       -> INCOMPLETE (shard-apl)
> Test gem_workarounds:
>         Subgroup reset-context:
>                 pass       -> INCOMPLETE (shard-apl)
> Test kms_vblank:
>         Subgroup pipe-a-query-idle-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-a-ts-continuation-idle-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-a-wait-busy-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-a-wait-forked-busy-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-a-wait-idle-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-b-query-forked-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-c-query-busy-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-c-query-forked-busy-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-c-query-forked-hang:
>                 pass       -> INCOMPLETE (shard-apl)
>         Subgroup pipe-c-ts-continuation-idle-hang:
>                 pass       -> INCOMPLETE (shard-apl)
> Test perf:
>         Subgroup gen8-unprivileged-single-ctx-counters:
>                 pass       -> FAIL       (shard-apl)
>
> ---- Known issues:
>
> Test drv_missed_irq:
>                 pass       -> SKIP       (shard-apl) fdo#103199
> Test gem_eio:
>         Subgroup in-flight-suspend:
>                 pass       -> INCOMPLETE (shard-apl) fdo#103375
> Test kms_flip:
>         Subgroup flip-vs-expired-vblank:
>                 fail       -> PASS       (shard-hsw) fdo#102887
>         Subgroup modeset-vs-vblank-race-interruptible:
>                 pass       -> FAIL       (shard-hsw) fdo#103060
> Test kms_plane_multiple:
>         Subgroup atomic-pipe-c-tiling-x:
>                 pass       -> FAIL       (shard-apl) fdo#103166
> Test kms_rotation_crc:
>         Subgroup sprite-rotation-90:
>                 fail       -> PASS       (shard-apl) fdo#103925
>
> fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
> fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
> fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
> fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
> fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
> fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
>
> shard-apl        total:1541 pass:1003 dwarn:1   dfail:1   fail:9    
> skip:497 time:2569s
> shard-hsw        total:2680 pass:1784 dwarn:1   dfail:0   fail:3    
> skip:891 time:11411s
> Blacklisted hosts:
> shard-kbl        total:1439 pass:1014 dwarn:1   dfail:1   fail:6    
> skip:386 time:1390s
> shard-snb        total:2680 pass:1378 dwarn:1   dfail:0   fail:3    
> skip:1298 time:6927s
>
> == Logs ==
>
> For more details see:  
> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8640/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw
  2018-04-09 12:23 ` [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw Michal Wajdeczko
  2018-04-09 12:47   ` Chris Wilson
@ 2018-04-09 16:01   ` Sagar Arun Kamble
  1 sibling, 0 replies; 24+ messages in thread
From: Sagar Arun Kamble @ 2018-04-09 16:01 UTC (permalink / raw)
  To: Michal Wajdeczko, intel-gfx



On 4/9/2018 5:53 PM, Michal Wajdeczko wrote:
> As we always call intel_uc_sanitize after every call to
> intel_uc_fini_hw we may drop redundant call and sanitize
> uC from the fini_hw function.
>
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
With change to sanitize during uc_init_mmio this looks good to me.
Reviewed-by: Sagar Arun Kamble <sagar.a.kamble@intel.com>
> ---
>   drivers/gpu/drm/i915/i915_gem.c | 2 --
>   drivers/gpu/drm/i915/intel_uc.c | 9 +++------
>   drivers/gpu/drm/i915/intel_uc.h | 1 -
>   3 files changed, 3 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
> index ceec5a0..decda1a 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -3077,7 +3077,6 @@ int i915_gem_reset_prepare(struct drm_i915_private *dev_priv)
>   	}
>   
>   	i915_gem_revoke_fences(dev_priv);
> -	intel_uc_sanitize(dev_priv);
>   
>   	return err;
>   }
> @@ -5062,7 +5061,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
>   	 * machine in an unusable condition.
>   	 */
>   	i915_gem_fini_hw(dev_priv);
> -	intel_uc_sanitize(dev_priv);
>   	i915_gem_sanitize(dev_priv);
>   
>   	intel_runtime_pm_put(dev_priv);
> diff --git a/drivers/gpu/drm/i915/intel_uc.c b/drivers/gpu/drm/i915/intel_uc.c
> index 1cffaf7..0439966 100644
> --- a/drivers/gpu/drm/i915/intel_uc.c
> +++ b/drivers/gpu/drm/i915/intel_uc.c
> @@ -322,18 +322,13 @@ void intel_uc_fini(struct drm_i915_private *dev_priv)
>   	intel_guc_fini(guc);
>   }
>   
> -void intel_uc_sanitize(struct drm_i915_private *i915)
> +static void __uc_sanitize(struct drm_i915_private *i915)
>   {
>   	struct intel_guc *guc = &i915->guc;
>   	struct intel_huc *huc = &i915->huc;
>   
> -	if (!USES_GUC(i915))
> -		return;
> -
>   	GEM_BUG_ON(!HAS_GUC(i915));
>   
> -	guc_disable_communication(guc);
> -
>   	intel_huc_sanitize(huc);
>   	intel_guc_sanitize(guc);
>   
> @@ -445,6 +440,8 @@ void intel_uc_fini_hw(struct drm_i915_private *dev_priv)
>   		intel_guc_submission_disable(guc);
>   
>   	guc_disable_communication(guc);
> +
> +	__uc_sanitize(dev_priv);
>   }
>   
>   int intel_uc_suspend(struct drm_i915_private *i915)
> diff --git a/drivers/gpu/drm/i915/intel_uc.h b/drivers/gpu/drm/i915/intel_uc.h
> index 25d73ad..64aaf93 100644
> --- a/drivers/gpu/drm/i915/intel_uc.h
> +++ b/drivers/gpu/drm/i915/intel_uc.h
> @@ -33,7 +33,6 @@
>   void intel_uc_init_mmio(struct drm_i915_private *dev_priv);
>   int intel_uc_init_misc(struct drm_i915_private *dev_priv);
>   void intel_uc_fini_misc(struct drm_i915_private *dev_priv);
> -void intel_uc_sanitize(struct drm_i915_private *dev_priv);
>   int intel_uc_init_hw(struct drm_i915_private *dev_priv);
>   void intel_uc_fini_hw(struct drm_i915_private *dev_priv);
>   int intel_uc_init(struct drm_i915_private *dev_priv);

-- 
Thanks,
Sagar

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

^ permalink raw reply	[flat|nested] 24+ messages in thread

* Re: ✗ Fi.CI.IGT: failure for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
  2018-04-09 15:32   ` Michal Wajdeczko
@ 2018-04-10  5:32     ` Sagar Arun Kamble
  2018-04-10  9:21       ` Chris Wilson
  0 siblings, 1 reply; 24+ messages in thread
From: Sagar Arun Kamble @ 2018-04-10  5:32 UTC (permalink / raw)
  To: Michal Wajdeczko, Patchwork, intel-gfx



On 4/9/2018 9:02 PM, Michal Wajdeczko wrote:
> On Mon, 09 Apr 2018 17:09:18 +0200, Patchwork 
> <patchwork@emeril.freedesktop.org> wrote:
>
>> == Series Details ==
>>
>> Series: series starting with [v8,01/12] drm/i915: Park before 
>> resetting the submission backend
>> URL   : https://patchwork.freedesktop.org/series/41365/
>> State : failure
>>
>> == Summary ==
>>
>> ---- Possible new issues:
>
> two variants:
>
>>
>> Test drm_mm:
>>         Subgroup sanitycheck:
>>                 pass       -> INCOMPLETE (shard-apl)
>
> #1
>
> <0>[  400.245461] drv_self-5775    1.... 400208508us : 
> intel_guc_submission_disable: intel_guc_submission_disable:1255 
> GEM_BUG_ON(dev_priv->gt.awake)
>
> <4>[  400.245871] Call Trace:
> <4>[  400.245959]  intel_uc_fini_hw+0x4b/0xe0 [i915]
> <4>[  400.246047]  i915_gem_fini_hw+0x16/0x30 [i915]
> <4>[  400.246129]  i915_reset+0x1e8/0x2b0 [i915]
> <4>[  400.246222]  igt_global_reset+0x38/0xe0 [i915]
>
Without gem_set_wedged if i915_reset path is invoked we can face this issue.
igt_global_reset and gem_eio resets are directly invoking 
i915_handle_error/i915_reset so I think we should fix the IGTs.
>> Test drv_hangman:
>>         Subgroup error-state-capture-blt:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup error-state-capture-bsd:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup error-state-capture-render:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup error-state-capture-vebox:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test drv_selftest:
>>         Subgroup live_guc:
>>                 pass       -> SKIP       (shard-apl)
>>         Subgroup live_hangcheck:
>>                 pass       -> DMESG-FAIL (shard-apl)
>> Test gem_eio:
>>         Subgroup execbuf:
>>                 pass       -> INCOMPLETE (shard-apl)
>
> #2:
>
> <3>[  227.833798] intel_engine_unpin_breadcrumbs_irq:219 
> GEM_BUG_ON(!b->irq_enabled)
>
> <4>[  227.834607] Call Trace:
> <4>[  227.834691]  intel_engines_park+0xef/0x180 [i915]
> <4>[  227.834709]  ? synchronize_irq+0x3e/0xb0
> <4>[  227.834781]  __i915_gem_park+0x3e/0x160 [i915]
> <4>[  227.834850]  i915_gem_idle_work_handler+0x1cd/0x220 [i915]
> <4>[  227.834868]  process_one_work+0x21a/0x640
>
>
irq disabling with GuC submission is not taking into consideration if it 
was enabled by waiter.
May be we should skip disarming interrupts while parking if there was no 
waiter since we will disarm them
during engine->park. Something like below?

diff --git a/drivers/gpu/drm/i915/intel_breadcrumbs.c 
b/drivers/gpu/drm/i915/intel_breadcrumbs.c
index 671a6d6..f8c0c4d 100644
--- a/drivers/gpu/drm/i915/intel_breadcrumbs.c
+++ b/drivers/gpu/drm/i915/intel_breadcrumbs.c
@@ -231,6 +231,13 @@ void intel_engine_disarm_breadcrumbs(struct 
intel_engine_cs *engine)
                 return;

         /*
+        * In case of reset with GuC submission we disarm the interrupts
+        * while parking if there are no waiters.
+        */
+       if (USES_GUC_SUBMISSION(engine->i915) && !b->irq_wait)
+               return;
+
+       /*
          * We only disarm the irq when we are idle (all requests 
completed),
          * so if the bottom-half remains asleep, it missed the request
          * completion.
>>         Subgroup in-flight-external:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test gem_mocs_settings:
>>         Subgroup mocs-reset-dirty-render:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test gem_request_retire:
>>         Subgroup retire-vma-not-inactive:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test gem_workarounds:
>>         Subgroup reset-context:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test kms_vblank:
>>         Subgroup pipe-a-query-idle-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-a-ts-continuation-idle-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-a-wait-busy-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-a-wait-forked-busy-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-a-wait-idle-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-b-query-forked-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-c-query-busy-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-c-query-forked-busy-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-c-query-forked-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>>         Subgroup pipe-c-ts-continuation-idle-hang:
>>                 pass       -> INCOMPLETE (shard-apl)
>> Test perf:
>>         Subgroup gen8-unprivileged-single-ctx-counters:
>>                 pass       -> FAIL       (shard-apl)
>>
>> ---- Known issues:
>>
>> Test drv_missed_irq:
>>                 pass       -> SKIP       (shard-apl) fdo#103199
>> Test gem_eio:
>>         Subgroup in-flight-suspend:
>>                 pass       -> INCOMPLETE (shard-apl) fdo#103375
>> Test kms_flip:
>>         Subgroup flip-vs-expired-vblank:
>>                 fail       -> PASS       (shard-hsw) fdo#102887
>>         Subgroup modeset-vs-vblank-race-interruptible:
>>                 pass       -> FAIL       (shard-hsw) fdo#103060
>> Test kms_plane_multiple:
>>         Subgroup atomic-pipe-c-tiling-x:
>>                 pass       -> FAIL       (shard-apl) fdo#103166
>> Test kms_rotation_crc:
>>         Subgroup sprite-rotation-90:
>>                 fail       -> PASS       (shard-apl) fdo#103925
>>
>> fdo#103199 https://bugs.freedesktop.org/show_bug.cgi?id=103199
>> fdo#103375 https://bugs.freedesktop.org/show_bug.cgi?id=103375
>> fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
>> fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
>> fdo#103166 https://bugs.freedesktop.org/show_bug.cgi?id=103166
>> fdo#103925 https://bugs.freedesktop.org/show_bug.cgi?id=103925
>>
>> shard-apl        total:1541 pass:1003 dwarn:1   dfail:1 fail:9   
>> skip:497 time:2569s
>> shard-hsw        total:2680 pass:1784 dwarn:1   dfail:0 fail:3   
>> skip:891 time:11411s
>> Blacklisted hosts:
>> shard-kbl        total:1439 pass:1014 dwarn:1   dfail:1 fail:6   
>> skip:386 time:1390s
>> shard-snb        total:2680 pass:1378 dwarn:1   dfail:0 fail:3   
>> skip:1298 time:6927s
>>
>> == Logs ==
>>
>> For more details see: 
>> https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8640/shards.html
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Thanks,
Sagar

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

^ permalink raw reply related	[flat|nested] 24+ messages in thread

* Re: ✗ Fi.CI.IGT: failure for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend
  2018-04-10  5:32     ` Sagar Arun Kamble
@ 2018-04-10  9:21       ` Chris Wilson
  0 siblings, 0 replies; 24+ messages in thread
From: Chris Wilson @ 2018-04-10  9:21 UTC (permalink / raw)
  To: Sagar Arun Kamble, Michal Wajdeczko, Patchwork, intel-gfx

Quoting Sagar Arun Kamble (2018-04-10 06:32:29)
> 
> 
> On 4/9/2018 9:02 PM, Michal Wajdeczko wrote:
> > On Mon, 09 Apr 2018 17:09:18 +0200, Patchwork 
> > <patchwork@emeril.freedesktop.org> wrote:
> >
> >> == Series Details ==
> >>
> >> Series: series starting with [v8,01/12] drm/i915: Park before 
> >> resetting the submission backend
> >> URL   : https://patchwork.freedesktop.org/series/41365/
> >> State : failure
> >>
> >> == Summary ==
> >>
> >> ---- Possible new issues:
> >
> > two variants:
> >
> >>
> >> Test drm_mm:
> >>         Subgroup sanitycheck:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >
> > #1
> >
> > <0>[  400.245461] drv_self-5775    1.... 400208508us : 
> > intel_guc_submission_disable: intel_guc_submission_disable:1255 
> > GEM_BUG_ON(dev_priv->gt.awake)
> >
> > <4>[  400.245871] Call Trace:
> > <4>[  400.245959]  intel_uc_fini_hw+0x4b/0xe0 [i915]
> > <4>[  400.246047]  i915_gem_fini_hw+0x16/0x30 [i915]
> > <4>[  400.246129]  i915_reset+0x1e8/0x2b0 [i915]
> > <4>[  400.246222]  igt_global_reset+0x38/0xe0 [i915]
> >
> Without gem_set_wedged if i915_reset path is invoked we can face this issue.
> igt_global_reset and gem_eio resets are directly invoking 
> i915_handle_error/i915_reset so I think we should fix the IGTs.

No, wrong answer.

> >> Test drv_hangman:
> >>         Subgroup error-state-capture-blt:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >>         Subgroup error-state-capture-bsd:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >>         Subgroup error-state-capture-render:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >>         Subgroup error-state-capture-vebox:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >> Test drv_selftest:
> >>         Subgroup live_guc:
> >>                 pass       -> SKIP       (shard-apl)
> >>         Subgroup live_hangcheck:
> >>                 pass       -> DMESG-FAIL (shard-apl)
> >> Test gem_eio:
> >>         Subgroup execbuf:
> >>                 pass       -> INCOMPLETE (shard-apl)
> >
> > #2:
> >
> > <3>[  227.833798] intel_engine_unpin_breadcrumbs_irq:219 
> > GEM_BUG_ON(!b->irq_enabled)
> >
> > <4>[  227.834607] Call Trace:
> > <4>[  227.834691]  intel_engines_park+0xef/0x180 [i915]
> > <4>[  227.834709]  ? synchronize_irq+0x3e/0xb0
> > <4>[  227.834781]  __i915_gem_park+0x3e/0x160 [i915]
> > <4>[  227.834850]  i915_gem_idle_work_handler+0x1cd/0x220 [i915]
> > <4>[  227.834868]  process_one_work+0x21a/0x640
> >
> >
> irq disabling with GuC submission is not taking into consideration if it 
> was enabled by waiter.

irqs cannot be disabled while in guc mode. It is still the same problem
of being unbalanced across enabling. (i.e. we switch to another mode to
submit the request and then enable the guc, ergo the guc never pins the
irq for itself.)
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply	[flat|nested] 24+ messages in thread

end of thread, other threads:[~2018-04-10  9:22 UTC | newest]

Thread overview: 24+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2018-04-09 12:23 [PATCH v8 01/12] drm/i915: Park before resetting the submission backend Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 02/12] drm/i915: Correctly handle error path in i915_gem_init_hw Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 03/12] drm/i915: Move i915_gem_fini to i915_gem.c Michal Wajdeczko
2018-04-09 14:50   ` Sagar Arun Kamble
2018-04-09 12:23 ` [PATCH v8 04/12] drm/i915: Introduce i915_gem_fini_hw for symmetry with i915_gem_init_hw Michal Wajdeczko
2018-04-09 15:01   ` Sagar Arun Kamble
2018-04-09 12:23 ` [PATCH v8 05/12] drm/i915: Add i915_gem_fini_hw to i915_gem_suspend Michal Wajdeczko
2018-04-09 15:02   ` Sagar Arun Kamble
2018-04-09 12:23 ` [PATCH v8 06/12] drm/i915: Add i915_gem_fini_hw to i915_reset Michal Wajdeczko
2018-04-09 12:30   ` Chris Wilson
2018-04-09 12:23 ` [PATCH v8 07/12] drm/i915/guc: Restore symmetric doorbell cleanup Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 08/12] drm/i915/uc: Fully sanitize uC within intel_uc_fini_hw Michal Wajdeczko
2018-04-09 12:47   ` Chris Wilson
2018-04-09 14:10     ` Michal Wajdeczko
2018-04-09 16:01   ` Sagar Arun Kamble
2018-04-09 12:23 ` [PATCH v8 09/12] drm/i915/uc: Use correct error code for GuC initialization failure Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 10/12] drm/i915/uc: Use helper functions to detect fw load status Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 11/12] drm/i915/uc: Trivial s/dev_priv/i915 in intel_uc.c Michal Wajdeczko
2018-04-09 12:23 ` [PATCH v8 12/12] HAX: Enable GuC for CI Michal Wajdeczko
2018-04-09 12:51 ` ✓ Fi.CI.BAT: success for series starting with [v8,01/12] drm/i915: Park before resetting the submission backend Patchwork
2018-04-09 15:09 ` ✗ Fi.CI.IGT: failure " Patchwork
2018-04-09 15:32   ` Michal Wajdeczko
2018-04-10  5:32     ` Sagar Arun Kamble
2018-04-10  9:21       ` Chris Wilson

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.