Re: [PATCH 5/8] drm/i915/selftests: Add tests for GT and engine workaround verification

[Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>]

On 30/11/2018 11:43, Chris Wilson wrote:
> Quoting Tvrtko Ursulin (2018-11-30 11:31:58)
>> From: Tvrtko Ursulin <tvrtko.ursulin@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.)
>>
>> Signed-off-by: Tvrtko Ursulin <tvrtko.ursulin@intel.com>
>> ---
>>   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(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_workarounds.c b/drivers/gpu/drm/i915/intel_workarounds.c
>> index 2d17d8a36a57..a21a21855e6a 100644
>> --- a/drivers/gpu/drm/i915/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/intel_workarounds.c
>> @@ -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)
>> diff --git a/drivers/gpu/drm/i915/intel_workarounds.h b/drivers/gpu/drm/i915/intel_workarounds.h
>> index f72cfda32d68..8f664d8b9e08 100644
>> --- a/drivers/gpu/drm/i915/intel_workarounds.h
>> +++ b/drivers/gpu/drm/i915/intel_workarounds.h
>> @@ -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
>> diff --git a/drivers/gpu/drm/i915/selftests/intel_workarounds.c b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
>> index 80396b3592f5..c009eb2af7fc 100644
>> --- a/drivers/gpu/drm/i915/selftests/intel_workarounds.c
>> +++ b/drivers/gpu/drm/i915/selftests/intel_workarounds.c
>> @@ -327,10 +327,100 @@ static int live_reset_whitelist(void *arg)
>>          return err;
>>   }
>>   
>> +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.
> 
>> +
>> +       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);
>> +
>> +       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.

There are not requests in this test but OK, I guess it makes it more 
future proof if one day something gets added.

> 
>> +       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;
>> +       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?

>> +
>> +       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");
>> +               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.

Regards,

Tvrtko

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