On 16/04/2019 09:10, Chris Wilson wrote:
Read the engine workarounds back using the GPU after loading the initial
context state to verify that we are setting them correctly, and bail if
it fails.

Signed-off-by: Chris Wilson <ch...@chris-wilson.co.uk>
Cc: Tvrtko Ursulin <tvrtko.ursu...@intel.com>
---
  drivers/gpu/drm/i915/i915_gem.c               |   6 +
  drivers/gpu/drm/i915/intel_workarounds.c      | 120 ++++++++++++++++++
  drivers/gpu/drm/i915/intel_workarounds.h      |   2 +
  .../drm/i915/selftests/intel_workarounds.c    |  53 +-------
  4 files changed, 134 insertions(+), 47 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_gem.c b/drivers/gpu/drm/i915/i915_gem.c
index 0a818a60ad31..95ae69753e91 100644
--- a/drivers/gpu/drm/i915/i915_gem.c
+++ b/drivers/gpu/drm/i915/i915_gem.c
@@ -4717,6 +4717,12 @@ static int __intel_engines_record_defaults(struct 
drm_i915_private *i915)
                i915_request_add(rq);
                if (err)
                        goto err_active;
+
+               if (IS_ENABLED(CONFIG_DRM_I915_DEBUG_GEM) &&
+                   intel_engine_verify_workarounds(engine, "load")) {
+                       err = -EIO;
+                       goto err_active;

The two submission use different contexts timelines so strictly speaking should be somehow explicitly serialized.

+               }
        }
/* Flush the default context image to memory, and enable powersaving. */
diff --git a/drivers/gpu/drm/i915/intel_workarounds.c 
b/drivers/gpu/drm/i915/intel_workarounds.c
index 1c54b5030807..db99f2e676bb 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/intel_workarounds.c
@@ -1259,6 +1259,126 @@ void intel_engine_apply_workarounds(struct 
intel_engine_cs *engine)
        wa_list_apply(engine->uncore, &engine->wa_list);
  }
+static struct i915_vma *
+create_scratch(struct i915_address_space *vm, int count)
+{
+       struct drm_i915_gem_object *obj;
+       struct i915_vma *vma;
+       unsigned int size;
+       int err;
+
+       size = round_up(count * 4, PAGE_SIZE);

sizeof(u32) if you want.

+       obj = i915_gem_object_create_internal(vm->i915, size);
+       if (IS_ERR(obj))
+               return ERR_CAST(obj);
+
+       i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
+
+       vma = i915_vma_instance(obj, vm, NULL);
+       if (IS_ERR(vma)) {
+               err = PTR_ERR(vma);
+               goto err_obj;
+       }
+
+       err = i915_vma_pin(vma, 0, 0,
+                          i915_vma_is_ggtt(vma) ? PIN_GLOBAL : PIN_USER);
+       if (err)
+               goto err_obj;
+
+       return vma;
+
+err_obj:
+       i915_gem_object_put(obj);
+       return ERR_PTR(err);
+}
+
+static int
+wa_list_srm(struct i915_request *rq,
+           const struct i915_wa_list *wal,
+           struct i915_vma *vma)
+{
+       const struct i915_wa *wa;
+       u32 srm, *cs;
+       int i;

A little bit of inconsistency between type of i here and in engine_wa_list_verify. Up to you.

+
+       srm = MI_STORE_REGISTER_MEM | MI_SRM_LRM_GLOBAL_GTT;
+       if (INTEL_GEN(rq->i915) >= 8)
+               srm++;
+
+       cs = intel_ring_begin(rq, 4 * wal->count);
+       if (IS_ERR(cs))
+               return PTR_ERR(cs);
+
+       for (i = 0, wa = wal->list; i < wal->count; i++, wa++) {
+               *cs++ = srm;
+               *cs++ = i915_mmio_reg_offset(wa->reg);
+               *cs++ = i915_ggtt_offset(vma) + sizeof(u32) * i;
+               *cs++ = 0;
+       }
+       intel_ring_advance(rq, cs);
+
+       return 0;
+}
+
+static int engine_wa_list_verify(struct intel_engine_cs *engine,
+                                const struct i915_wa_list * const wal,
+                                const char *from)
+{
+       const struct i915_wa *wa;
+       struct i915_request *rq;
+       struct i915_vma *vma;
+       unsigned int i;
+       u32 *results;
+       int err;
+
+       if (!wal->count)
+               return 0;
+
+       vma = create_scratch(&engine->i915->ggtt.vm, wal->count);
+       if (IS_ERR(vma))
+               return PTR_ERR(vma);
+
+       rq = i915_request_alloc(engine, engine->kernel_context->gem_context);
+       if (IS_ERR(rq)) {
+               err = PTR_ERR(rq);
+               goto err_vma;
+       }
+
+       err = wa_list_srm(rq, wal, vma);
+       if (err)
+               goto err_vma;
+
+       i915_request_add(rq);
+       if (i915_request_wait(rq, I915_WAIT_LOCKED, HZ / 5) < 0) {

Wouldn't:

err = i915_request_wait(...
if (err < 0 || !i915_request_completed())

be more correct?

+               err = -ETIME;
+               goto err_vma;
+       }
+
+       results = i915_gem_object_pin_map(vma->obj, I915_MAP_WB);
+       if (IS_ERR(results)) {
+               err = PTR_ERR(results);
+               goto err_vma;
+       }
+
+       err = 0;
+       for (i = 0, wa = wal->list; i < wal->count; i++, wa++)
+               if (!wa_verify(wa, results[i], wal->name, from))
+                       err = -ENXIO;
+
+       i915_gem_object_unpin_map(vma->obj);
+
+err_vma:
+       i915_vma_unpin(vma);
+       i915_vma_put(vma);
+       return err;
+}
+
+int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
+                                   const char *from)
+{
+       return engine_wa_list_verify(engine, &engine->wa_list, from);
+}
+
  #if IS_ENABLED(CONFIG_DRM_I915_SELFTEST)
  #include "selftests/intel_workarounds.c"
  #endif
diff --git a/drivers/gpu/drm/i915/intel_workarounds.h 
b/drivers/gpu/drm/i915/intel_workarounds.h
index 34eee5ec511e..fdf7ebb90f28 100644
--- a/drivers/gpu/drm/i915/intel_workarounds.h
+++ b/drivers/gpu/drm/i915/intel_workarounds.h
@@ -30,5 +30,7 @@ void intel_engine_apply_whitelist(struct intel_engine_cs 
*engine);
void intel_engine_init_workarounds(struct intel_engine_cs *engine);
  void intel_engine_apply_workarounds(struct intel_engine_cs *engine);
+int intel_engine_verify_workarounds(struct intel_engine_cs *engine,
+                                   const char *from);
#endif
diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c 
b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
index 567b6f8dae86..a363748a7a4f 100644
--- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
+++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
@@ -340,49 +340,6 @@ static int check_whitelist_across_reset(struct 
intel_engine_cs *engine,
        return err;
  }
-static struct i915_vma *create_scratch(struct i915_gem_context *ctx)
-{
-       struct drm_i915_gem_object *obj;
-       struct i915_vma *vma;
-       void *ptr;
-       int err;
-
-       obj = i915_gem_object_create_internal(ctx->i915, PAGE_SIZE);
-       if (IS_ERR(obj))
-               return ERR_CAST(obj);
-
-       i915_gem_object_set_cache_coherency(obj, I915_CACHE_LLC);
-
-       ptr = i915_gem_object_pin_map(obj, I915_MAP_WB);
-       if (IS_ERR(ptr)) {
-               err = PTR_ERR(ptr);
-               goto err_obj;
-       }
-       memset(ptr, 0xc5, PAGE_SIZE);
-       i915_gem_object_flush_map(obj);
-       i915_gem_object_unpin_map(obj);
-
-       vma = i915_vma_instance(obj, &ctx->ppgtt->vm, NULL);
-       if (IS_ERR(vma)) {
-               err = PTR_ERR(vma);
-               goto err_obj;
-       }
-
-       err = i915_vma_pin(vma, 0, 0, PIN_USER);
-       if (err)
-               goto err_obj;
-
-       err = i915_gem_object_set_to_cpu_domain(obj, false);
-       if (err)
-               goto err_obj;
-
-       return vma;
-
-err_obj:
-       i915_gem_object_put(obj);
-       return ERR_PTR(err);
-}
-
  static struct i915_vma *create_batch(struct i915_gem_context *ctx)
  {
        struct drm_i915_gem_object *obj;
@@ -475,7 +432,7 @@ static int check_dirty_whitelist(struct i915_gem_context 
*ctx,
        int err = 0, i, v;
        u32 *cs, *results;
- scratch = create_scratch(ctx);
+       scratch = create_scratch(&ctx->ppgtt->vm, 2 * ARRAY_SIZE(values) + 1);
        if (IS_ERR(scratch))
                return PTR_ERR(scratch);
@@ -752,9 +709,11 @@ static bool verify_gt_engine_wa(struct drm_i915_private *i915, ok &= wa_list_verify(&i915->uncore, &lists->gt_wa_list, str); - for_each_engine(engine, i915, id)
-               ok &= wa_list_verify(engine->uncore,
-                                    &lists->engine[id].wa_list, str);
+       for_each_engine(engine, i915, id) {
+               ok &= engine_wa_list_verify(engine,
+                                           &lists->engine[id].wa_list,
+                                           str) == 0;
+       }
return ok;
  }



Okay so MMIO verification happens immediately after application and with SRM from selftests.

What about the context workarounds? With the SRM infrastructure those could be verified easily as well I think.

Regards,

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

Reply via email to