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@xxxxxxxxx> > >> > >> 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@xxxxxxxxx> > >> --- > >> 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 _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
