Quoting Mika Kuoppala (2019-02-27 16:58:49) > We use identical sequence of stopping ringbuffer on reset > handing and on ring initialization. Make a function > to handle both cases. > > Cc: Chris Wilson <ch...@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuopp...@linux.intel.com> > --- > drivers/gpu/drm/i915/i915_reset.c | 18 +--------------- > drivers/gpu/drm/i915/intel_engine_cs.c | 28 +++++++++++++++++++++++++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 10 ++------- > drivers/gpu/drm/i915/intel_ringbuffer.h | 2 ++ > 4 files changed, 33 insertions(+), 25 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_reset.c > b/drivers/gpu/drm/i915/i915_reset.c > index 55d6123dbba4..4191398b5125 100644 > --- a/drivers/gpu/drm/i915/i915_reset.c > +++ b/drivers/gpu/drm/i915/i915_reset.c > @@ -113,28 +113,12 @@ void i915_reset_request(struct i915_request *rq, bool > guilty) > > static void gen3_stop_engine(struct intel_engine_cs *engine) > { > - struct drm_i915_private *dev_priv = engine->i915; > - const u32 base = engine->mmio_base; > - > GEM_TRACE("%s\n", engine->name); > > if (intel_engine_stop_cs(engine)) > GEM_TRACE("%s: timed out on STOP_RING\n", engine->name); > > - I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base))); > - POSTING_READ_FW(RING_HEAD(base)); /* paranoia */ > - > - I915_WRITE_FW(RING_HEAD(base), 0); > - I915_WRITE_FW(RING_TAIL(base), 0); > - POSTING_READ_FW(RING_TAIL(base)); > - > - /* The ring must be empty before it is disabled */ > - I915_WRITE_FW(RING_CTL(base), 0); > - > - /* Check acts as a post */ > - if (I915_READ_FW(RING_HEAD(base))) > - GEM_TRACE("%s: ring head [%x] not parked\n", > - engine->name, I915_READ_FW(RING_HEAD(base))); > + intel_engine_stop_ringbuffer(engine); > } > > static void i915_stop_engines(struct drm_i915_private *i915, > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c > b/drivers/gpu/drm/i915/intel_engine_cs.c > index 3feb0f74c239..a8e47cfa6e35 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -826,6 +826,34 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs > *engine) > _MASKED_BIT_DISABLE(STOP_RING)); > } > > +int intel_engine_stop_ringbuffer(struct intel_engine_cs *engine)
I think intel_engine_stop_ring() works better, since it is the set of RING registers we are controlling and are not concerned here with the backing buffer. > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + const u32 base = engine->mmio_base; > + > + assert_forcewakes_active(dev_priv, FORCEWAKE_ALL); > + GEM_TRACE("%s\n", engine->name); > + > + I915_WRITE_FW(RING_HEAD(base), I915_READ_FW(RING_TAIL(base))); > + POSTING_READ_FW(RING_HEAD(base)); /* paranoia */ > + > + I915_WRITE_FW(RING_HEAD(base), 0); > + I915_WRITE_FW(RING_TAIL(base), 0); > + POSTING_READ_FW(RING_TAIL(base)); > + > + /* The ring must be empty before it is disabled */ > + I915_WRITE_FW(RING_CTL(base), 0); > + > + /* Check acts as a post */ > + if (I915_READ_FW(RING_HEAD(base))) { > + GEM_TRACE("%s: ring head [%x] not parked\n", > + engine->name, I915_READ_FW(RING_HEAD(base))); > + return -EIO; > + } > + > + return 0; > +} > + > const char *i915_cache_level_str(struct drm_i915_private *i915, int type) > { > switch (type) { > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c > b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 5363dad1208d..8936a9051760 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -622,15 +622,9 @@ static bool stop_ring(struct intel_engine_cs *engine) > return false; > } > > - I915_WRITE_HEAD(engine, I915_READ_TAIL(engine)); > + ret = intel_engine_stop_ringbuffer(engine); Maybe if (intel_engine_stop_ring(engine)) return false; return false; looks tidier? Reviewed-by: Chris Wilson <ch...@chris-wilson.co.uk> -Chris _______________________________________________ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org https://lists.freedesktop.org/mailman/listinfo/intel-gfx