Re: [Intel-gfx] [PATCH 12/34] drm/i915: Issue engine resets onto idle engines

[Chris Wilson]

Quoting John Harrison (2019-01-23 01:18:36)
> On 1/21/2019 14:20, Chris Wilson wrote:
> > @@ -479,8 +477,6 @@ static int __igt_reset_engine(struct drm_i915_private 
> > *i915, bool active)
> >   break;
> >   }
> >   
> > - GEM_BUG_ON(!rq->global_seqno);
> > - seqno = rq->global_seqno - 1;
> AFAICT this saved seqno value was never used anyway? It only exists 
> inside the loop, was only used in a pr_err earlier in the loop, and the 
> start of the loop always (re-)initialises it. Or am I missing some 
> hidden macro magic somewhere?

I can't remember what I intended it to be in the first place. History
says that we had to set engine->hangcheck.seqno correctly to force a
per-engine reset. Lost in

commit bba0869b18e44ff2f713c98575ddad8c7c5e9b10
Author: Chris Wilson 
Date:   Fri Apr 6 23:03:53 2018 +0100

drm/i915: Treat i915_reset_engine() as guilty until proven innocent

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

Re: [Intel-gfx] [PATCH 12/34] drm/i915: Issue engine resets onto idle engines

[John Harrison]

On 1/21/2019 14:20, Chris Wilson wrote:

Always perform the requested reset, even if we believe the engine is
idle. Presumably there was a reason the caller wanted the reset, and in
the near future we lose the easy tracking for whether the engine is
idle.

Signed-off-by: Chris Wilson 
---
  drivers/gpu/drm/i915/i915_reset.c |  4 
  .../gpu/drm/i915/selftests/intel_hangcheck.c  | 22 +--
  2 files changed, 6 insertions(+), 20 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reset.c 
b/drivers/gpu/drm/i915/i915_reset.c
index 064fc6da1512..d44b095e2860 100644
--- a/drivers/gpu/drm/i915/i915_reset.c
+++ b/drivers/gpu/drm/i915/i915_reset.c
@@ -1063,10 +1063,6 @@ int i915_reset_engine(struct intel_engine_cs *engine, 
const char *msg)
GEM_TRACE("%s flags=%lx\n", engine->name, error->flags);
GEM_BUG_ON(!test_bit(I915_RESET_ENGINE + engine->id, >flags));
  
-	if (i915_seqno_passed(intel_engine_get_seqno(engine),

- intel_engine_last_submit(engine)))
-   return 0;
-
reset_prepare_engine(engine);
  
  	if (msg)

diff --git a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c 
b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
index 8025c7e0bf6c..2c38ea5892d9 100644
--- a/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
+++ b/drivers/gpu/drm/i915/selftests/intel_hangcheck.c
@@ -449,8 +449,6 @@ static int __igt_reset_engine(struct drm_i915_private 
*i915, bool active)
  
  		set_bit(I915_RESET_ENGINE + id, >gpu_error.flags);

do {
-   u32 seqno = intel_engine_get_seqno(engine);
-
if (active) {
struct i915_request *rq;
  
@@ -479,8 +477,6 @@ static int __igt_reset_engine(struct drm_i915_private *i915, bool active)

break;
}
  
-GEM_BUG_ON(!rq->global_seqno);

-   seqno = rq->global_seqno - 1;
AFAICT this saved seqno value was never used anyway? It only exists 
inside the loop, was only used in a pr_err earlier in the loop, and the 
start of the loop always (re-)initialises it. Or am I missing some 
hidden macro magic somewhere?




i915_request_put(rq);
}
  
@@ -496,11 +492,10 @@ static int __igt_reset_engine(struct drm_i915_private *i915, bool active)

break;
}
  
-			reset_engine_count += active;

if (i915_reset_engine_count(>gpu_error, engine) !=
-   reset_engine_count) {
-   pr_err("%s engine reset %srecorded!\n",
-  engine->name, active ? "not " : "");
+   ++reset_engine_count) {
+   pr_err("%s engine reset not recorded!\n",
+  engine->name);
err = -EINVAL;
break;
}
@@ -728,7 +723,6 @@ static int __igt_reset_engines(struct drm_i915_private 
*i915,
  
  		set_bit(I915_RESET_ENGINE + id, >gpu_error.flags);

do {
-   u32 seqno = intel_engine_get_seqno(engine);
struct i915_request *rq = NULL;
  
  			if (flags & TEST_ACTIVE) {

@@ -756,9 +750,6 @@ static int __igt_reset_engines(struct drm_i915_private 
*i915,
err = -EIO;
break;
}
-
-   GEM_BUG_ON(!rq->global_seqno);
-   seqno = rq->global_seqno - 1;
}
  
  			err = i915_reset_engine(engine, NULL);

@@ -795,10 +786,9 @@ static int __igt_reset_engines(struct drm_i915_private 
*i915,
  
  		reported = i915_reset_engine_count(>gpu_error, engine);

reported -= threads[engine->id].resets;
-   if (reported != (flags & TEST_ACTIVE ? count : 0)) {
-   pr_err("i915_reset_engine(%s:%s): reset %lu times, but 
reported %lu, expected %lu reported\n",
-  engine->name, test_name, count, reported,
-  (flags & TEST_ACTIVE ? count : 0));
+   if (reported != count) {
+   pr_err("i915_reset_engine(%s:%s): reset %lu times, but 
reported %lu\n",
+  engine->name, test_name, count, reported);
if (!err)
err = -EINVAL;
}


Reviewed-by: John Harrison 

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