[Intel-gfx] [PATCH 3/8] drm/i915: Verify GT workaround state at runtime
chris at chris-wilson.co.uk
Fri Nov 30 14:36:22 UTC 2018
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
More information about the Intel-gfx