Restore workarounds after engine reset and unify their handling

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

At this stage these are the driver initialization and engine reset.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c          |  4 +++
 drivers/gpu/drm/i915/i915_gem.c          |  3 ++
 drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
 4 files changed, 55 insertions(+)

drivers/gpu/drm/i915/i915_drv.c +1 -0

index 2f3dc1cf83a6..14d019c9455b 100644
@@ -53,6 +53,7 @@
 #include "i915_vgpu.h"
 #include "intel_drv.h"
 #include "intel_uc.h"
+#include "intel_workarounds.h"
 
 static struct drm_driver driver;
 

Quoting Tvrtko Ursulin (2018-11-30 11:31:56)

Quoted 32 lines

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

At this stage these are the driver initialization and engine reset.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.c          |  4 +++
  drivers/gpu/drm/i915/i915_gem.c          |  3 ++
  drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
  4 files changed, 55 insertions(+)

drivers/gpu/drm/i915/i915_drv.c +1 -0

index 2f3dc1cf83a6..14d019c9455b 100644
@@ -53,6 +53,7 @@
  #include "i915_vgpu.h"
  #include "intel_drv.h"
  #include "intel_uc.h"
+#include "intel_workarounds.h"
 
  static struct drm_driver driver;
 

@@ -2362,6 +2363,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
                 goto out;
         }
 

drivers/gpu/drm/i915/i915_drv.c int i915_reset_engine(struct intel_engine_cs *engine, const char *msg) · line 2367

Quoted 2 lines

drivers/gpu/drm/i915/i915_drv.c +2 -0

+       /* Catch GT workarounds affected by engine reset. */
+       intel_gt_workarounds_verify(engine->i915, engine->name);

I'd rather, quite strongly, not have this inside i915_reset_engine() itself. i915_reset_engine() is [designed to be] called from atomic context. Adding I915_READ() here is questionable. My preference is to have all this inside selftests/intel_workarounds where we can cross check the gt/all-engines after device reset and other engine reset.

Quoted 33 lines

Quoting Tvrtko Ursulin (2018-11-30 11:31:56)

Quoted 32 lines

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

At this stage these are the driver initialization and engine reset.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.c          |  4 +++
  drivers/gpu/drm/i915/i915_gem.c          |  3 ++
  drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
  4 files changed, 55 insertions(+)

drivers/gpu/drm/i915/i915_drv.c +1 -0

index 2f3dc1cf83a6..14d019c9455b 100644
@@ -53,6 +53,7 @@
  #include "i915_vgpu.h"
  #include "intel_drv.h"
  #include "intel_uc.h"
+#include "intel_workarounds.h"
 
  static struct drm_driver driver;
 

@@ -2362,6 +2363,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
                 goto out;
         }
 

Quoted 8 lines

drivers/gpu/drm/i915/i915_drv.c int i915_reset_engine(struct intel_engine_cs *engine, const char *msg) · line 2367

Quoted 2 lines

drivers/gpu/drm/i915/i915_drv.c +2 -0

+       /* Catch GT workarounds affected by engine reset. */
+       intel_gt_workarounds_verify(engine->i915, engine->name);

I'd rather, quite strongly, not have this inside i915_reset_engine() itself. i915_reset_engine() is [designed to be] called from atomic context. Adding I915_READ() here is questionable. My preference is to have all this inside selftests/intel_workarounds where we can cross check the gt/all-engines after device reset and other engine reset.

I wasn't sure myself how much value checking it here adds, but regarding I915_READs, they are happening during engine->init_hw anyway. So I don't think a few more would harm. But yes, as said, since I was unsure myself I am happy to drop this hunk.

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

At this stage these are the driver initialization and engine reset.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
 drivers/gpu/drm/i915/i915_drv.c          |  4 +++
 drivers/gpu/drm/i915/i915_gem.c          |  3 ++
 drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
 4 files changed, 55 insertions(+)

drivers/gpu/drm/i915/i915_drv.c +4 -0

index 2f3dc1cf83a6..14d019c9455b 100644
@@ -53,6 +53,7 @@
 #include "i915_vgpu.h"
 #include "intel_drv.h"
 #include "intel_uc.h"
+#include "intel_workarounds.h"
 
 static struct drm_driver driver;
 

@@ -2362,6 +2363,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
                goto out;
        }
 
+       /* Catch GT workarounds affected by engine reset. */
+       intel_gt_workarounds_verify(engine->i915, engine->name);
+
        /*
         * The request that caused the hang is stuck on elsp, we know the
         * active request and can drop it, adjust head to skip the offending

drivers/gpu/drm/i915/i915_gem.c +3 -0

index 18adb3dd1fcd..1eff471d4366 100644
@@ -5334,7 +5334,10 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
                I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ?
                           LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED);
 
+       /* Apply the GT workarounds... */
        intel_gt_workarounds_apply(dev_priv);
+       /* ...and determine whether they are sticking. */
+       intel_gt_workarounds_verify(dev_priv, "init");
 
        i915_gem_init_swizzling(dev_priv);
 

drivers/gpu/drm/i915/intel_workarounds.c +6 -0

index be63a2af3481..a5c0d206b2a4 100644
@@ -981,6 +981,52 @@ void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv)
        wa_list_apply(dev_priv, &dev_priv->gt_wa_list);
 }
 
+static void
+wa_fail(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
+{
+       DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
+                 name, from,
+                 i915_mmio_reg_offset(wa->reg),

Quoting Tvrtko Ursulin (2018-11-30 11:31:56)

Quoted 72 lines

From: Tvrtko Ursulin <[email protected]>

Since we now have all the GT workarounds in a table, by adding a simple shared helper function we can now verify that their values are still applied after some interesting events in the lifetime of the driver.

At this stage these are the driver initialization and engine reset.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/i915_drv.c          |  4 +++
  drivers/gpu/drm/i915/i915_gem.c          |  3 ++
  drivers/gpu/drm/i915/intel_workarounds.c | 46 ++++++++++++++++++++++++
  drivers/gpu/drm/i915/intel_workarounds.h |  2 ++
  4 files changed, 55 insertions(+)

drivers/gpu/drm/i915/i915_drv.c +4 -0

index 2f3dc1cf83a6..14d019c9455b 100644
@@ -53,6 +53,7 @@
  #include "i915_vgpu.h"
  #include "intel_drv.h"
  #include "intel_uc.h"
+#include "intel_workarounds.h"
 
  static struct drm_driver driver;
 

@@ -2362,6 +2363,9 @@ int i915_reset_engine(struct intel_engine_cs *engine, const char *msg)
                 goto out;
         }
 
+       /* Catch GT workarounds affected by engine reset. */
+       intel_gt_workarounds_verify(engine->i915, engine->name);
+
         /*
          * The request that caused the hang is stuck on elsp, we know the
          * active request and can drop it, adjust head to skip the offending

drivers/gpu/drm/i915/i915_gem.c +3 -0

index 18adb3dd1fcd..1eff471d4366 100644
@@ -5334,7 +5334,10 @@ int i915_gem_init_hw(struct drm_i915_private *dev_priv)
                 I915_WRITE(MI_PREDICATE_RESULT_2, IS_HSW_GT3(dev_priv) ?
                            LOWER_SLICE_ENABLED : LOWER_SLICE_DISABLED);
 
+       /* Apply the GT workarounds... */
         intel_gt_workarounds_apply(dev_priv);
+       /* ...and determine whether they are sticking. */
+       intel_gt_workarounds_verify(dev_priv, "init");
 
         i915_gem_init_swizzling(dev_priv);
 

drivers/gpu/drm/i915/intel_workarounds.c +11 -0

index be63a2af3481..a5c0d206b2a4 100644
@@ -981,6 +981,52 @@ void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv)
         wa_list_apply(dev_priv, &dev_priv->gt_wa_list);
  }
 
+static void
+wa_fail(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
+{
+       DRM_ERROR("%s workaround lost on %s! (%x=%x/%x, expected %x, mask=%x)\n",
+                 name, from,
+                 i915_mmio_reg_offset(wa->reg),
+                 cur, cur & wa->mask, wa->val, wa->mask);
+}
+
+static void
+wa_verify_bits(const struct i915_wa *wa, u32 cur, const char *name,

drivers/gpu/drm/i915/intel_workarounds.c void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv) · line 999

Quoted 5 lines

drivers/gpu/drm/i915/intel_workarounds.c +5 -0

+              const char *from)
+{
+       u32 bits = wa->mask;
+       u32 cur_ = cur;
+       u32 val_ = wa->val;

Make sure wa->mask is set to ~0u for whole registers, which it must be for the wa_fail.

if ((cur ^ wa->val) & wa->mask) {
	wa_fail(wa, cur, name, whom);

return false; /* make it bool now to save churn later? */
}

From: Tvrtko Ursulin <[email protected]>

Two simple selftests which test that both GT and engine workarounds are not lost after either a full GPU reset, or after the per-engine ones.

(Including checks that one engine reset is not affecting workarounds not belonging to itself.)

Signed-off-by Tvrtko Ursulin <[email protected]>

---
 drivers/gpu/drm/i915/intel_workarounds.c      | 21 +++--
 drivers/gpu/drm/i915/intel_workarounds.h      |  4 +-
 .../drm/i915/selftests/intel_workarounds.c    | 90 +++++++++++++++++++
 3 files changed, 105 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_workarounds.c +13 -8

index 2d17d8a36a57..a21a21855e6a 100644
@@ -990,7 +990,7 @@ wa_fail(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
                  cur, cur & wa->mask, wa->val, wa->mask);
 }
 
-static void
+static bool
 wa_verify_bits(const struct i915_wa *wa, u32 cur, const char *name,
               const char *from)
 {

@@ -1001,30 +1001,35 @@ wa_verify_bits(const struct i915_wa *wa, u32 cur, const char *name,
        while (bits) {
                if ((bits & 1) && ((cur_ & 1) != (val_ & 1))) {
                        wa_fail(wa, cur, name, from);
-                       break;
+                       return false;
                }
 
                bits >>= 1;
                cur_ >>= 1;
                val_ >>= 1;
        }
+
+       return true;
 }
 
-static void wa_list_verify(struct drm_i915_private *dev_priv,
+static bool wa_list_verify(struct drm_i915_private *dev_priv,
                           const struct i915_wa_list *wal,
                           const char *from)
 {
        struct i915_wa *wa;
        unsigned int i;
+       bool res = true;
 
        for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
-               wa_verify_bits(wa, I915_READ(wa->reg), wal->name, from);
+               res &= wa_verify_bits(wa, I915_READ(wa->reg), wal->name, from);
+
+       return res;
 }
 
-void intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
+bool intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
                                 const char *from)
 {
-       wa_list_verify(dev_priv, &dev_priv->gt_wa_list, from);
+       return wa_list_verify(dev_priv, &dev_priv->gt_wa_list, from);
 }
 
 struct whitelist {

@@ -1313,10 +1318,10 @@ void intel_engine_workarounds_apply(struct intel_engine_cs *engine)
        wa_list_apply(engine->i915, &engine->wa_list);
 }
 
-void intel_engine_workarounds_verify(struct intel_engine_cs *engine,
+bool intel_engine_workarounds_verify(struct intel_engine_cs *engine,
                                     const char *from)
 {
-       wa_list_verify(engine->i915, &engine->wa_list, from);
+       return wa_list_verify(engine->i915, &engine->wa_list, from);
 }
 
 #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)

drivers/gpu/drm/i915/intel_workarounds.h +2 -2

index f72cfda32d68..8f664d8b9e08 100644
@@ -33,14 +33,14 @@ int intel_ctx_workarounds_emit(struct i915_request *rq);
 
 void intel_gt_workarounds_init(struct drm_i915_private *dev_priv);
 void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv);
-void intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
+bool intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
                                 const char *from);
 
 void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
 
 void intel_engine_workarounds_init(struct intel_engine_cs *engine);
 void intel_engine_workarounds_apply(struct intel_engine_cs *engine);
-void intel_engine_workarounds_verify(struct intel_engine_cs *engine,
+bool intel_engine_workarounds_verify(struct intel_engine_cs *engine,
                                     const char *from);
 
 #endif

drivers/gpu/drm/i915/selftests/intel_workarounds.c +0 -0

index 80396b3592f5..c009eb2af7fc 100644
@@ -327,10 +327,100 @@ static int live_reset_whitelist(void *arg)
        return err;
 }
 

drivers/gpu/drm/i915/selftests/intel_workarounds.c +14 -0

+
+       return ok;
+}
+
+static int
+live_gpu_reset_gt_engine_workarounds(void *arg)
+{
+       struct drm_i915_private *i915 = arg;
+       struct i915_gpu_error *error = &i915->gpu_error;
+       bool ok;
+
+       if (!intel_has_gpu_reset(i915))
+               return 0;
+

drivers/gpu/drm/i915/selftests/intel_workarounds.c +12 -0

+       i915_reset(i915, ~0, "live_workarounds");
+       intel_runtime_pm_put(i915);
+
+       ok = verify_gt_engine_wa(i915, "after reset");
+
+out:
+       clear_bit(I915_RESET_BACKOFF, &error->flags);
+
+       return ok ? 0 : -ESRCH;
+}
+
+static int

drivers/gpu/drm/i915/selftests/intel_workarounds.c +4 -0

+
+       for_each_engine(engine, i915, id) {
+               pr_info("Verifying after %s reset...\n", engine->name);
+

Quoting Tvrtko Ursulin (2018-11-30 11:31:58)

Quoted 111 lines

From: Tvrtko Ursulin <[email protected]>

Two simple selftests which test that both GT and engine workarounds are not lost after either a full GPU reset, or after the per-engine ones.

(Including checks that one engine reset is not affecting workarounds not belonging to itself.)

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/intel_workarounds.c      | 21 +++--
  drivers/gpu/drm/i915/intel_workarounds.h      |  4 +-
  .../drm/i915/selftests/intel_workarounds.c    | 90 +++++++++++++++++++
  3 files changed, 105 insertions(+), 10 deletions(-)

drivers/gpu/drm/i915/intel_workarounds.c +13 -8

index 2d17d8a36a57..a21a21855e6a 100644
@@ -990,7 +990,7 @@ wa_fail(const struct i915_wa *wa, u32 cur, const char *name, const char *from)
                   cur, cur & wa->mask, wa->val, wa->mask);
  }
 
-static void
+static bool
  wa_verify_bits(const struct i915_wa *wa, u32 cur, const char *name,
                const char *from)
  {

@@ -1001,30 +1001,35 @@ wa_verify_bits(const struct i915_wa *wa, u32 cur, const char *name,
         while (bits) {
                 if ((bits & 1) && ((cur_ & 1) != (val_ & 1))) {
                         wa_fail(wa, cur, name, from);
-                       break;
+                       return false;
                 }
 
                 bits >>= 1;
                 cur_ >>= 1;
                 val_ >>= 1;
         }
+
+       return true;
  }
 
-static void wa_list_verify(struct drm_i915_private *dev_priv,
+static bool wa_list_verify(struct drm_i915_private *dev_priv,
                            const struct i915_wa_list *wal,
                            const char *from)
  {
         struct i915_wa *wa;
         unsigned int i;
+       bool res = true;
 
         for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
-               wa_verify_bits(wa, I915_READ(wa->reg), wal->name, from);
+               res &= wa_verify_bits(wa, I915_READ(wa->reg), wal->name, from);
+
+       return res;
  }
 
-void intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
+bool intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
                                  const char *from)
  {
-       wa_list_verify(dev_priv, &dev_priv->gt_wa_list, from);
+       return wa_list_verify(dev_priv, &dev_priv->gt_wa_list, from);
  }
 
  struct whitelist {

@@ -1313,10 +1318,10 @@ void intel_engine_workarounds_apply(struct intel_engine_cs *engine)
         wa_list_apply(engine->i915, &engine->wa_list);
  }
 
-void intel_engine_workarounds_verify(struct intel_engine_cs *engine,
+bool intel_engine_workarounds_verify(struct intel_engine_cs *engine,
                                      const char *from)
  {
-       wa_list_verify(engine->i915, &engine->wa_list, from);
+       return wa_list_verify(engine->i915, &engine->wa_list, from);
  }
 
  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)

drivers/gpu/drm/i915/intel_workarounds.h +2 -2

index f72cfda32d68..8f664d8b9e08 100644
@@ -33,14 +33,14 @@ int intel_ctx_workarounds_emit(struct i915_request *rq);
 
  void intel_gt_workarounds_init(struct drm_i915_private *dev_priv);
  void intel_gt_workarounds_apply(struct drm_i915_private *dev_priv);
-void intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
+bool intel_gt_workarounds_verify(struct drm_i915_private *dev_priv,
                                  const char *from);
 
  void intel_whitelist_workarounds_apply(struct intel_engine_cs *engine);
 
  void intel_engine_workarounds_init(struct intel_engine_cs *engine);
  void intel_engine_workarounds_apply(struct intel_engine_cs *engine);
-void intel_engine_workarounds_verify(struct intel_engine_cs *engine,
+bool intel_engine_workarounds_verify(struct intel_engine_cs *engine,
                                      const char *from);
 
  #endif

drivers/gpu/drm/i915/selftests/intel_workarounds.c +0 -0

index 80396b3592f5..c009eb2af7fc 100644
@@ -327,10 +327,100 @@ static int live_reset_whitelist(void *arg)
         return err;
  }
 

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 339

Quoted 10 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +10 -0

+static bool verify_gt_engine_wa(struct drm_i915_private *i915, const char *str)
+{
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
+       bool ok = true;
+
+       ok &= intel_gt_workarounds_verify(i915, str);
+
+       for_each_engine(engine, i915, id)
+               ok &= intel_engine_workarounds_verify(engine, str);

Ok, ok.

Quoted 19 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +19 -0

+
+       return ok;
+}
+
+static int
+live_gpu_reset_gt_engine_workarounds(void *arg)
+{
+       struct drm_i915_private *i915 = arg;
+       struct i915_gpu_error *error = &i915->gpu_error;
+       bool ok;
+
+       if (!intel_has_gpu_reset(i915))
+               return 0;
+
+       pr_info("Verifying after GPU reset...\n");
+
+       set_bit(I915_RESET_BACKOFF, &error->flags);
+       set_bit(I915_RESET_HANDOFF, &error->flags);
+

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 363

Quoted 5 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +5 -0

+       ok = verify_gt_engine_wa(i915, "before reset");
+       if (!ok)
+               goto out;
+
+       intel_runtime_pm_get(i915);

Move RESET_HANDOFF here to avoid any repetition of earlier problems we had with the reset being applied too early.

Quoted 18 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +17 -0

+       i915_reset(i915, ~0, "live_workarounds");
+       intel_runtime_pm_put(i915);
+
+       ok = verify_gt_engine_wa(i915, "after reset");
+
+out:
+       clear_bit(I915_RESET_BACKOFF, &error->flags);
+
+       return ok ? 0 : -ESRCH;
+}
+
+static int
+live_engine_reset_gt_engine_workarounds(void *arg)
+{
+       struct drm_i915_private *i915 = arg;
+       struct i915_gpu_error *error = &i915->gpu_error;
+       struct intel_engine_cs *engine;

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 385

Quoted 5 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +5 -0

+       enum intel_engine_id id;
+       bool ok;
+
+       if (!intel_has_reset_engine(i915))
+               return 0;

May be easier to take global_reset_lock/unlock from selftests/intel_hanghceck.

Quoted 8 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +8 -0

+
+       for_each_engine(engine, i915, id) {
+               pr_info("Verifying after %s reset...\n", engine->name);
+
+               set_bit(I915_RESET_BACKOFF, &error->flags);
+               set_bit(I915_RESET_ENGINE + engine->id, &error->flags);
+
+               ok = verify_gt_engine_wa(i915, "before reset");

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 399

Quoted 6 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +6 -0

+               if (!ok)
+                       goto out;
+
+               intel_runtime_pm_get(i915);
+               i915_reset_engine(engine, "live_workarounds");
+               intel_runtime_pm_put(i915);

Once idle, and once with a spinner?

Quoted 9 lines

Quoting Tvrtko Ursulin (2018-11-30 11:31:58)

Quoted 8 lines

From: Tvrtko Ursulin <[email protected]>

Two simple selftests which test that both GT and engine workarounds are not lost after either a full GPU reset, or after the per-engine ones.

(Including checks that one engine reset is not affecting workarounds not belonging to itself.)

Quoted 16 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 385

Quoted 13 lines

Signed-off-by Tvrtko Ursulin <[email protected]>

---

drivers/gpu/drm/i915/selftests/intel_workarounds.c +11 -0

+static int
+live_engine_reset_gt_engine_workarounds(void *arg)
+{
+       struct drm_i915_private *i915 = arg;
+       struct i915_gpu_error *error = &i915->gpu_error;
+       struct intel_engine_cs *engine;
+       enum intel_engine_id id;
+       bool ok;
+
+       if (!intel_has_reset_engine(i915))
+               return 0;

May be easier to take global_reset_lock/unlock from selftests/intel_hanghceck.

I looked inside and did not find anything with this name so I don't know what you mean?

Quoted 8 lines

Quoted 8 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +8 -0

+
+       for_each_engine(engine, i915, id) {
+               pr_info("Verifying after %s reset...\n", engine->name);
+
+               set_bit(I915_RESET_BACKOFF, &error->flags);
+               set_bit(I915_RESET_ENGINE + engine->id, &error->flags);
+
+               ok = verify_gt_engine_wa(i915, "before reset");

Quoted 8 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c static int live_reset_whitelist(void *arg) · line 399

Quoted 6 lines

drivers/gpu/drm/i915/selftests/intel_workarounds.c +6 -0

+               if (!ok)
+                       goto out;
+
+               intel_runtime_pm_get(i915);
+               i915_reset_engine(engine, "live_workarounds");
+               intel_runtime_pm_put(i915);

Once idle, and once with a spinner?

If idle then engine reset does nothing, so maybe I am again not following you.

drivers/gpu/drm/i915/intel_workarounds.c +2 -4

-static void whitelist_reg(struct whitelist *w, i915_reg_t reg)
+static void
+whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
 {
-       if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS))
-               return;
-

Quoting Tvrtko Ursulin (2018-11-30 11:31:59)

Quoted 13 lines

drivers/gpu/drm/i915/intel_workarounds.c +5 -6

-static void whitelist_reg(struct whitelist *w, i915_reg_t reg)
+static void
+whitelist_reg(struct i915_wa_list *wal, i915_reg_t reg)
  {
-       if (GEM_DEBUG_WARN_ON(w->count >= RING_MAX_NONPRIV_SLOTS))
-               return;
-
-       w->reg[w->count++] = reg;
-}
+       struct i915_wa wa = {
+               .reg = reg
+       };

drivers/gpu/drm/i915/intel_workarounds.c bool intel_gt_workarounds_verify(struct drm_i915_private *dev_priv, · line 1042

Quoted 4 lines

drivers/gpu/drm/i915/intel_workarounds.c +1 -3

-static void bdw_whitelist_build(struct whitelist *w)
-{
-}
+       if (GEM_WARN_ON(wal->count >= RING_MAX_NONPRIV_SLOTS))

We only need this code for pre-merge testing of patches as the build list is static.

From: Tvrtko Ursulin <[email protected]>

Convert the per context workaround handling code to run against the newly introduced common workaround framework and fuse the two to use the existing smarter list add helper, the one which does the sorted insert and merges registers where possible.

This completes migration of all four classes of workarounds onto the common framework.

Existing macros are kept untouched for smaller code churn.

Signed-off-by Tvrtko Ursulin <[email protected]>

---

Quoting Tvrtko Ursulin (2018-11-30 11:32:00)

Quoted 16 lines

From: Tvrtko Ursulin <[email protected]>

Convert the per context workaround handling code to run against the newly introduced common workaround framework and fuse the two to use the existing smarter list add helper, the one which does the sorted insert and merges registers where possible.

This completes migration of all four classes of workarounds onto the common framework.

Existing macros are kept untouched for smaller code churn.

Signed-off-by Tvrtko Ursulin <[email protected]>

---

(quoted diff) +0 -0

@@ -1652,7 +1638,7 @@ struct drm_i915_private {
 

drivers/gpu/drm/i915/i915_drv.h struct drm_i915_private { · line 1642

Quoted 5 lines

drivers/gpu/drm/i915/i915_drv.h +1 -1

         int dpio_phy_iosf_port[I915_NUM_PHYS_VLV];
 
-       struct i915_workarounds workarounds;
+       struct i915_wa_list wa_list;
         struct i915_wa_list gt_wa_list;

ctx_wa_list ?

From: Tvrtko Ursulin <[email protected]>

The new workaround list allocator grows the list in chunks so will end up with some unused space. Trim it when the initialization phase is done to free up a tiny bit of slab.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
 drivers/gpu/drm/i915/intel_workarounds.c | 13 +++++++++++++
 1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/intel_workarounds.c +0 -0

index de2bddbc64b4..abfe4b530c23 100644

Quoting Tvrtko Ursulin (2018-11-30 11:32:01)

Quoted 17 lines

From: Tvrtko Ursulin <[email protected]>

The new workaround list allocator grows the list in chunks so will end up with some unused space. Trim it when the initialization phase is done to free up a tiny bit of slab.

Signed-off-by Tvrtko Ursulin <[email protected]>

---
  drivers/gpu/drm/i915/intel_workarounds.c | 13 +++++++++++++
  1 file changed, 13 insertions(+)

drivers/gpu/drm/i915/intel_workarounds.c +0 -0

index de2bddbc64b4..abfe4b530c23 100644
@@ -55,6 +55,19 @@ static void wa_init_start(struct i915_wa_list *wal, const char *name)
 

drivers/gpu/drm/i915/intel_workarounds.c static void wa_init_start(struct i915_wa_list *wal, const char *name) · line 61

Quoted 6 lines

drivers/gpu/drm/i915/intel_workarounds.c +4 -0

  static void wa_init_finish(struct i915_wa_list *wal)
  {
+       /* Trim unused entries. */
+       if (wal->count < wal->__size) {
+               struct i915_wa *wa =
+                       kcalloc(wal->count, sizeof(*wa), GFP_KERNEL);

kcalloc followed by memcpy, what have you done with Tvrtko? kmemdup ?