[Intel-gfx] [PATCH 3/8] drm/i915: Verify GT workaround state at runtime

[Chris Wilson]

Quoting Tvrtko Ursulin (2018-11-30 12:02:56)
> 
> On 30/11/2018 11:38, Chris Wilson wrote:
> > Quoting Tvrtko Ursulin (2018-11-30 11:31:56)
> >> From: Tvrtko Ursulin <tvrtko.ursulin at intel.com>
> >>
> >> 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 <tvrtko.ursulin at intel.com>
> >> ---
> >>   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(+)
> >>
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 2f3dc1cf83a6..14d019c9455b 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -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);
> > 
> > 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.

Adding the I915_READ isn't the end-of-the-world, I am more wary of the
slipperly slope and suddenly finding ourselves with a per-engine reset
that is no longer suitable for fast resets, possibly from hardirq
context. (Although that's more likely to be softirq really.)

In this case, I think we can satisfy ourselves with a comprehensive yet
fast test suite. And there's no harm in having the checks on idle for
runtime consistency checking (we can even inject SRM before idling for
completeness).
-Chris