[Intel-gfx] [PATCH 4/7] drm/i915/selftests: Add tests for GT and engine workaround verification

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-11-30 17:44:09)
> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> 
> 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.)
> 
> v2:
>  * Rebase for series refactoring.
>  * Add spinner for actual engine reset!
>  * Add idle reset test as well. (Chris Wilson)
>  * Share existing global_reset_lock. (Chris Wilson)
> 
> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> ---
>  drivers/gpu/drm/i915/Makefile                 |   1 +
>  drivers/gpu/drm/i915/intel_workarounds.c      |   6 +
>  drivers/gpu/drm/i915/selftests/igt_common.c   |  44 ++++++
>  drivers/gpu/drm/i915/selftests/igt_common.h   |  15 ++
>  .../gpu/drm/i915/selftests/intel_hangcheck.c  |  51 ++----
>  .../drm/i915/selftests/intel_workarounds.c    | 147 +++++++++++++++++-
>  6 files changed, 217 insertions(+), 47 deletions(-)
>  create mode 100644 drivers/gpu/drm/i915/selftests/igt_common.c
>  create mode 100644 drivers/gpu/drm/i915/selftests/igt_common.h
> 
> diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
> index 50a8fa8fce64..ceeb21f8aa0c 100644
> --- a/drivers/gpu/drm/i915/Makefile
> +++ b/drivers/gpu/drm/i915/Makefile
> @@ -166,6 +166,7 @@ i915-$(CONFIG_DRM_I915_SELFTEST) += \
>         selftests/i915_random.o \
>         selftests/i915_selftest.o \
>         selftests/igt_flush_test.o \
> +       selftests/igt_common.o \

I was anticipating igt_reset.c.

> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> index 80396b3592f5..194a3c30554d 100644
> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
> @@ -6,6 +6,8 @@
>  
>  #include "../i915_selftest.h"
>  
> +#include "igt_common.h"
> +#include "igt_flush_test.h"
>  #include "igt_spinner.h"
>  #include "igt_wedge_me.h"
>  #include "mock_context.h"
> @@ -290,7 +292,6 @@ static int live_reset_whitelist(void *arg)
>  {
>         struct drm_i915_private *i915 = arg;
>         struct intel_engine_cs *engine = i915->engine[RCS];
> -       struct i915_gpu_error *error = &i915->gpu_error;
>         struct whitelist w;
>         int err = 0;
>  
> @@ -302,8 +303,7 @@ static int live_reset_whitelist(void *arg)
>         if (!whitelist_build(engine, &w))
>                 return 0;
>  
> -       set_bit(I915_RESET_BACKOFF, &error->flags);
> -       set_bit(I915_RESET_ENGINE + engine->id, &error->flags);
> +       igt_global_reset_lock(i915);
>  
>         if (intel_has_reset_engine(i915)) {
>                 err = check_whitelist_across_reset(engine,
> @@ -322,15 +322,152 @@ static int live_reset_whitelist(void *arg)
>         }
>  
>  out:
> -       clear_bit(I915_RESET_ENGINE + engine->id, &error->flags);
> -       clear_bit(I915_RESET_BACKOFF, &error->flags);
> +       igt_global_reset_unlock(i915);

Yup, makes sense.

>         return err;
>  }
>  
> +bool intel_engine_workarounds_verify(struct intel_engine_cs *engine,
> +                                    const char *from);
> +
> +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);
> +
> +       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");

Such noise! ;)

> +
> +       igt_global_reset_lock(i915);
> +
> +       ok = verify_gt_engine_wa(i915, "before reset");
> +       if (!ok)
> +               goto out;
> +
> +       intel_runtime_pm_get(i915);
> +       set_bit(I915_RESET_HANDOFF, &error->flags);
> +       i915_reset(i915, ALL_ENGINES, "live_workarounds");
> +       intel_runtime_pm_put(i915);
> +
> +       ok = verify_gt_engine_wa(i915, "after reset");
> +
> +out:
> +       igt_global_reset_unlock(i915);
> +
> +       return ok ? 0 : -ESRCH;
> +}
> +
> +static int
> +live_engine_reset_gt_engine_workarounds(void *arg)
> +{
> +       struct drm_i915_private *i915 = arg;
> +       struct intel_engine_cs *engine;
> +       struct i915_gem_context *ctx;
> +       struct igt_spinner spin;
> +       enum intel_engine_id id;
> +       struct i915_request *rq;
> +       int ret = 0;
> +
> +       if (!intel_has_reset_engine(i915))
> +               return 0;
> +
> +       ctx = kernel_context(i915);
> +       if (IS_ERR(ctx))
> +               return PTR_ERR(ctx);
> +
> +       for_each_engine(engine, i915, id) {
> +               bool ok;
> +
> +               pr_info("Verifying after %s reset...\n", engine->name);
> +
> +               igt_global_reset_lock(i915);

The key thing with it being global, is that you can take it globally :)
-Chris