[PATCH 1/6] drm/i915: Do request retirement before marking engines as wedged

[PATCH 1/6] drm/i915: Do request retirement before marking engines as wedged

[Chris Wilson]

As we declare an engine as wedged, we mark all of its active requests as
in error. However, we don't want to mark successfully completed requests
as in error, which requires us to retire those requests first.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 2709be922512..cb61cec26db5 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -2998,10 +2998,15 @@ void i915_gem_set_wedged(struct drm_i915_private *dev_priv)
 	lockdep_assert_held(&dev_priv->drm.struct_mutex);
 	set_bit(I915_WEDGED, &dev_priv->gpu_error.flags);
 
+	/* Retire completed requests first so the list of inflight/incomplete
+	 * requests is accurate and we don't try and mark successful requests
+	 * as in error during __i915_gem_set_wedged_BKL().
+	 */
+	i915_gem_retire_requests(dev_priv);
+
 	stop_machine(__i915_gem_set_wedged_BKL, dev_priv, NULL);
 
 	i915_gem_context_lost(dev_priv);
-	i915_gem_retire_requests(dev_priv);
 
 	mod_delayed_work(dev_priv->wq, &dev_priv->gt.idle_work, 0);
 }
-- 
2.11.0

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

[PATCH 2/6] drm/i915: Suppress busy status for engines if wedged

[Chris Wilson]

If the driver is wedged, HW state may be very inconsistent and
report that it is still busy, even though we have stopped using it. This
can lead to a double *ERROR* rather than a graceful cleanup after
wedging.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem.c        | 4 +---
 drivers/gpu/drm/i915/intel_engine_cs.c | 9 +++++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index cb61cec26db5..7165f314f830 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3112,9 +3112,7 @@ i915_gem_idle_work_handler(struct work_struct *work)
 	 * Wait for last execlists context complete, but bail out in case a
 	 * new request is submitted.
 	 */
-	wait_for(READ_ONCE(dev_priv->gt.active_requests) ||
-		 intel_engines_are_idle(dev_priv),
-		 10);
+	wait_for(intel_engines_are_idle(dev_priv), 10);
 	if (READ_ONCE(dev_priv->gt.active_requests))
 		return;
 
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c
index 1fb97e8401de..854e8e0c836b 100644
--- a/drivers/gpu/drm/i915/intel_engine_cs.c
+++ b/drivers/gpu/drm/i915/intel_engine_cs.c
@@ -1111,6 +1111,15 @@ bool intel_engines_are_idle(struct drm_i915_private *dev_priv)
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 
+	if (READ_ONCE(dev_priv->gt.active_requests))
+		return false;
+
+	/* If the driver is wedged, HW state may be very inconsistent and
+	 * report that it is still busy, even though we have stopped using it.
+	 */
+	if (i915_terminally_wedged(&dev_priv->gpu_error))
+		return true;
+
 	for_each_engine(engine, dev_priv, id) {
 		if (!intel_engine_is_idle(engine))
 			return false;
-- 
2.11.0

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

[PATCH 3/6] drm/i915: Move retire-requests into i915_gem_wait_for_idle()

[Chris Wilson]

As we now distinguish everywhere that can call
i915_gem_retire_requests() following a successful wait_for_idle, we can
remove the duplication by moving that call into i915_gem_wait_for_idle()
itself.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_debugfs.c               | 4 +---
 drivers/gpu/drm/i915/i915_gem.c                   | 6 +++---
 drivers/gpu/drm/i915/i915_gem_evict.c             | 2 --
 drivers/gpu/drm/i915/i915_gem_request.c           | 3 ---
 drivers/gpu/drm/i915/selftests/i915_gem_request.c | 2 --
 drivers/gpu/drm/i915/selftests/intel_hangcheck.c  | 1 -
 6 files changed, 4 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index d4904245472f..5c34757b687f 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -4180,8 +4180,6 @@ fault_irq_set(struct drm_i915_private *i915,
 		goto err_unlock;
 
 	/* Retire to kick idle work */
-	i915_gem_retire_requests(i915);
-	GEM_BUG_ON(i915->gt.active_requests);
 
 	*irq = val;
 	mutex_unlock(&i915->drm.struct_mutex);
@@ -4286,7 +4284,7 @@ i915_drop_caches_set(void *data, u64 val)
 			goto unlock;
 	}
 
-	if (val & (DROP_RETIRE | DROP_ACTIVE))
+	if (val & DROP_RETIRE)
 		i915_gem_retire_requests(dev_priv);
 
 	lockdep_set_current_reclaim_state(GFP_KERNEL);
diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 7165f314f830..70bc72634a91 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3285,6 +3285,9 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
 			if (ret)
 				return ret;
 		}
+
+		i915_gem_retire_requests(i915);
+		GEM_BUG_ON(i915->gt.active_requests);
 	} else {
 		ret = wait_for_timeline(&i915->gt.global_timeline, flags);
 		if (ret)
@@ -4426,9 +4429,6 @@ int i915_gem_suspend(struct drm_i915_private *dev_priv)
 	if (ret)
 		goto err_unlock;
 
-	i915_gem_retire_requests(dev_priv);
-	GEM_BUG_ON(dev_priv->gt.active_requests);
-
 	assert_kernel_context_is_current(dev_priv);
 	i915_gem_context_lost(dev_priv);
 	mutex_unlock(&dev->struct_mutex);
diff --git a/drivers/gpu/drm/i915/i915_gem_evict.c b/drivers/gpu/drm/i915/i915_gem_evict.c
index 2da3a94fc9f3..51e365f70464 100644
--- a/drivers/gpu/drm/i915/i915_gem_evict.c
+++ b/drivers/gpu/drm/i915/i915_gem_evict.c
@@ -196,7 +196,6 @@ i915_gem_evict_something(struct i915_address_space *vm,
 	if (ret)
 		return ret;
 
-	i915_gem_retire_requests(dev_priv);
 	goto search_again;
 
 found:
@@ -383,7 +382,6 @@ int i915_gem_evict_vm(struct i915_address_space *vm, bool do_idle)
 		if (ret)
 			return ret;
 
-		i915_gem_retire_requests(dev_priv);
 		WARN_ON(!list_empty(&vm->active_list));
 	}
 
diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index b9d8f43b31e6..f16900848650 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -203,9 +203,6 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
 	if (ret)
 		return ret;
 
-	i915_gem_retire_requests(i915);
-	GEM_BUG_ON(i915->gt.active_requests > 1);
-
 	/* If the seqno wraps around, we need to clear the breadcrumb rbtree */
 	for_each_engine(engine, i915, id) {
 		struct intel_timeline *tl = &timeline->engine[id];
diff --git a/drivers/gpu/drm/i915/selftests/i915_gem_request.c b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
index 926b24c117d6..6b788ffda822 100644
--- a/drivers/gpu/drm/i915/selftests/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/selftests/i915_gem_request.c
@@ -291,8 +291,6 @@ static int begin_live_test(struct live_test *t,
 		return err;
 	}
 
-	i915_gem_retire_requests(i915);
-
 	i915->gpu_error.missed_irq_rings = 0;
 	t->reset_count = i915_reset_count(&i915->gpu_error);
 
diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index 6ec7c731a267..aa31d6c0cdfb 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -235,7 +235,6 @@ static void hang_fini(struct hang *h)
 	i915_gem_object_put(h->hws);
 
 	i915_gem_wait_for_idle(h->i915, I915_WAIT_LOCKED);
-	i915_gem_retire_requests(h->i915);
 }
 
 static int igt_hang_sanitycheck(void *arg)
-- 
2.11.0

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

[PATCH 4/6] drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()

[Chris Wilson]

Make i915_gem_wait_for_idle() be a little heavier in order to try and
guarantee that the GPU is indeed idle (by checking each engine
individually is idle, i.e. all writes are complete and the rings
stopped) after waiting for in-flight requests to be completed.

v2: And return the final error.
v3: Break the wait_for() out from under the WARN -- the macro expansion
is hideous and unreadable in the warning message
v4: If wait_for_engine() fails the result is catastrophic, mark the
device as wedged and wait for the repair team.

References: https://bugs.freedesktop.org/show_bug.cgi?id=98836
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem.c | 29 ++++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 70bc72634a91..bbc6f1c9f175 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -3271,6 +3271,29 @@ static int wait_for_timeline(struct i915_gem_timeline *tl, unsigned int flags)
 	return 0;
 }
 
+static int wait_for_engine(struct intel_engine_cs *engine, int timeout_ms)
+{
+	return wait_for(intel_engine_is_idle(engine), timeout_ms);
+}
+
+static int wait_for_engines(struct drm_i915_private *i915)
+{
+	struct intel_engine_cs *engine;
+	enum intel_engine_id id;
+
+	for_each_engine(engine, i915, id) {
+		if (GEM_WARN_ON(wait_for_engine(engine, 50))) {
+			i915_gem_set_wedged(i915);
+			return -EIO;
+		}
+
+		GEM_BUG_ON(intel_engine_get_seqno(engine) !=
+			   intel_engine_last_submit(engine));
+	}
+
+	return 0;
+}
+
 int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
 {
 	int ret;
@@ -3288,13 +3311,13 @@ int i915_gem_wait_for_idle(struct drm_i915_private *i915, unsigned int flags)
 
 		i915_gem_retire_requests(i915);
 		GEM_BUG_ON(i915->gt.active_requests);
+
+		ret = wait_for_engines(i915);
 	} else {
 		ret = wait_for_timeline(&i915->gt.global_timeline, flags);
-		if (ret)
-			return ret;
 	}
 
-	return 0;
+	return ret;
 }
 
 /** Flushes the GTT write domain for the object if it's dirty. */
-- 
2.11.0

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

[PATCH 5/6] drm/i915: Remove redudant wait for each engine to idle from seqno wrap

[Chris Wilson]

Having added the wait upon each engine to idle into the central
i915_gem_wait_for_idle(), we can remove the now redundant wait from
reset_all_global_seqno(). This has the advantage of removing the late
detection of an error (an engine still busy) which left the seqno reset
only partially complete (though it should be safe enough!).

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index f16900848650..1bfc3e664470 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -207,9 +207,6 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
 	for_each_engine(engine, i915, id) {
 		struct intel_timeline *tl = &timeline->engine[id];
 
-		if (wait_for(intel_engine_is_idle(engine), 50))
-			return -EBUSY;
-
 		if (!i915_seqno_passed(seqno, tl->seqno)) {
 			/* spin until threads are complete */
 			while (intel_breadcrumbs_busy(engine))
-- 
2.11.0

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

[PATCH 6/6] drm/i915: Combine reset_all_global_seqno() loops into one

[Chris Wilson]

We can merge the pair of loops over the engines and their timelines into
a single loop, making it easier to read and more consistent with the
commentary.

Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/i915_gem_request.c | 14 +++++---------
 1 file changed, 5 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_request.c b/drivers/gpu/drm/i915/i915_gem_request.c
index 1bfc3e664470..6348353b91ec 100644
--- a/drivers/gpu/drm/i915/i915_gem_request.c
+++ b/drivers/gpu/drm/i915/i915_gem_request.c
@@ -191,7 +191,6 @@ i915_priotree_init(struct i915_priotree *pt)
 
 static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
 {
-	struct i915_gem_timeline *timeline = &i915->gt.global_timeline;
 	struct intel_engine_cs *engine;
 	enum intel_engine_id id;
 	int ret;
@@ -205,7 +204,8 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
 
 	/* If the seqno wraps around, we need to clear the breadcrumb rbtree */
 	for_each_engine(engine, i915, id) {
-		struct intel_timeline *tl = &timeline->engine[id];
+		struct i915_gem_timeline *timeline;
+		struct intel_timeline *tl = engine->timeline;
 
 		if (!i915_seqno_passed(seqno, tl->seqno)) {
 			/* spin until threads are complete */
@@ -216,14 +216,10 @@ static int reset_all_global_seqno(struct drm_i915_private *i915, u32 seqno)
 		/* Finally reset hw state */
 		tl->seqno = seqno;
 		intel_engine_init_global_seqno(engine, seqno);
-	}
-
-	list_for_each_entry(timeline, &i915->gt.timelines, link) {
-		for_each_engine(engine, i915, id) {
-			struct intel_timeline *tl = &timeline->engine[id];
 
-			memset(tl->sync_seqno, 0, sizeof(tl->sync_seqno));
-		}
+		list_for_each_entry(timeline, &i915->gt.timelines, link)
+			memset(timeline->engine[id].sync_seqno, 0,
+			       sizeof(timeline->engine[id].sync_seqno));
 	}
 
 	return 0;
-- 
2.11.0

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

✗ Fi.CI.BAT: failure for series starting with [1/6] drm/i915: Do request retirement before marking engines as wedged

[Patchwork]

== Series Details ==

Series: series starting with [1/6] drm/i915: Do request retirement before marking engines as wedged
URL   : https://patchwork.freedesktop.org/series/22214/
State : failure

== Summary ==

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

Test gem_exec_fence:
        Subgroup await-hang-default:
                pass       -> INCOMPLETE (fi-hsw-4770)
Test gem_exec_suspend:
        Subgroup basic-s4-devices:
                pass       -> DMESG-WARN (fi-kbl-7560u) fdo#100125

fdo#100125 https://bugs.freedesktop.org/show_bug.cgi?id=100125

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 428s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 434s
fi-bsw-n3050     total:278  pass:239  dwarn:0   dfail:0   fail:0   skip:39  time: 573s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 507s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 560s
fi-byt-j1900     total:278  pass:251  dwarn:0   dfail:0   fail:0   skip:27  time: 485s
fi-byt-n2820     total:278  pass:247  dwarn:0   dfail:0   fail:0   skip:31  time: 486s
fi-hsw-4770      total:48   pass:41   dwarn:0   dfail:0   fail:0   skip:6   time: 0s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 413s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 418s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 483s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 482s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 454s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time: 562s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 453s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 455s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 489s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 436s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 524s
fi-snb-2600      total:278  pass:248  dwarn:1   dfail:0   fail:0   skip:29  time: 403s

80e8cddb7c01d1cf1dec04d58b1c0e51d77a6050 drm-tip: 2017y-03m-30d-14h-33m-10s UTC integration manifest
00fd2d0 drm/i915: Combine reset_all_global_seqno() loops into one
4603551 drm/i915: Remove redudant wait for each engine to idle from seqno wrap
e98dad2 drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()
ef17845 drm/i915: Move retire-requests into i915_gem_wait_for_idle()
0acb5f02 drm/i915: Suppress busy status for engines if wedged
85cd515 drm/i915: Do request retirement before marking engines as wedged

== Logs ==

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

Re: [PATCH 3/6] drm/i915: Move retire-requests into i915_gem_wait_for_idle()

[Joonas Lahtinen]

On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> As we now distinguish everywhere that can call
> i915_gem_retire_requests() following a successful wait_for_idle, we can
> remove the duplication by moving that call into i915_gem_wait_for_idle()
> itself.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

<SNIP>

> @@ -4180,8 +4180,6 @@ fault_irq_set(struct drm_i915_private *i915,
>  		goto err_unlock;
>  
>  	/* Retire to kick idle work */

Stale comment, best before date was yesterday.

i915_gem_idle_gpu or something might be more descriptive now.

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 4/6] drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()

[Joonas Lahtinen]

On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> Make i915_gem_wait_for_idle() be a little heavier in order to try and
> guarantee that the GPU is indeed idle (by checking each engine
> individually is idle, i.e. all writes are complete and the rings
> stopped) after waiting for in-flight requests to be completed.
> 
> v2: And return the final error.
> v3: Break the wait_for() out from under the WARN -- the macro expansion
> is hideous and unreadable in the warning message
> v4: If wait_for_engine() fails the result is catastrophic, mark the
> device as wedged and wait for the repair team.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=98836
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

That's more accurate yep, but is our other bookkeeping broken then?

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 5/6] drm/i915: Remove redudant wait for each engine to idle from seqno wrap

[Joonas Lahtinen]

On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> Having added the wait upon each engine to idle into the central
> i915_gem_wait_for_idle(), we can remove the now redundant wait from
> reset_all_global_seqno(). This has the advantage of removing the late
> detection of an error (an engine still busy) which left the seqno reset
> only partially complete (though it should be safe enough!).
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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 6/6] drm/i915: Combine reset_all_global_seqno() loops into one

[Joonas Lahtinen]

On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> We can merge the pair of loops over the engines and their timelines into
> a single loop, making it easier to read and more consistent with the
> commentary.
> 
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

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 4/6] drm/i915: Wait for all engines to be idle as part of i915_gem_wait_for_idle()

[Chris Wilson]

On Fri, Mar 31, 2017 at 11:31:50AM +0300, Joonas Lahtinen wrote:
> On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> > Make i915_gem_wait_for_idle() be a little heavier in order to try and
> > guarantee that the GPU is indeed idle (by checking each engine
> > individually is idle, i.e. all writes are complete and the rings
> > stopped) after waiting for in-flight requests to be completed.
> > 
> > v2: And return the final error.
> > v3: Break the wait_for() out from under the WARN -- the macro expansion
> > is hideous and unreadable in the warning message
> > v4: If wait_for_engine() fails the result is catastrophic, mark the
> > device as wedged and wait for the repair team.
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=98836
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> That's more accurate yep, but is our other bookkeeping broken then?

Not as far as I can tell. It's just that the engines can be alive a
little bit longer than the breadcrumb write, and in some circumstances
that is an important distinction.

Just just wtf is going on with Baytrail elludes me. It's like the GPU is
doing a timewarp.
-Chris

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

Re: [PATCH 3/6] drm/i915: Move retire-requests into i915_gem_wait_for_idle()

[Chris Wilson]

On Fri, Mar 31, 2017 at 11:21:22AM +0300, Joonas Lahtinen wrote:
> On to, 2017-03-30 at 15:50 +0100, Chris Wilson wrote:
> > As we now distinguish everywhere that can call
> > i915_gem_retire_requests() following a successful wait_for_idle, we can
> > remove the duplication by moving that call into i915_gem_wait_for_idle()
> > itself.
> > 
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> <SNIP>
> 
> > @@ -4180,8 +4180,6 @@ fault_irq_set(struct drm_i915_private *i915,
> >  		goto err_unlock;
> >  
> >  	/* Retire to kick idle work */
> 
> Stale comment, best before date was yesterday.
> 
> i915_gem_idle_gpu or something might be more descriptive now.

In the past it was actually called i915_gpu_idle().

It evolved from that as that isn't very clear what it actually does, and
predated GEM (i.e. it also dealt with DRI1). I like having the "wait" in
the name, as that should make the might_sleep() very clear and ties in
nicely with passing around the wait-flags.

i915_gem_wait_for_idle() is still my favourite, yet.
-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