| Message ID | 20190228161411.10462-1-mika.kuoppala@linux.intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/3] drm/i915: Use intel_engine_stop_cs when stopping ringbuffer | expand |
Quoting Mika Kuoppala (2019-02-28 16:14:11) > We have an exported function for stopping the engine before > disabling a ringbuffer. Take it into use. > > v2: use fw on empty check > v3: tail is tail > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 3 ++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++----------- > 2 files changed, 26 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index df8f88142f1d..e35dc0386bf6 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > > + if (INTEL_GEN(dev_priv) < 3) > + return; > + > GEM_TRACE("%s\n", engine->name); > > I915_WRITE_FW(RING_MI_MODE(engine->mmio_base), > diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > index 1b96b0960adc..5fe28d9087b7 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) > flush_cs_tlb(engine); > } > > +static bool ring_is_empty(struct intel_engine_cs *engine) > +{ > + struct drm_i915_private *dev_priv = engine->i915; > + const u32 base = engine->mmio_base; > + > + return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) == > + (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR); > +} > + > static bool stop_ring(struct intel_engine_cs *engine) > { > struct drm_i915_private *dev_priv = engine->i915; > + int ret; > > - if (INTEL_GEN(dev_priv) > 2) { > - I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING)); > - if (intel_wait_for_register(dev_priv, > - RING_MI_MODE(engine->mmio_base), > - MODE_IDLE, > - MODE_IDLE, > - 1000)) { > - DRM_ERROR("%s : timed out trying to stop ring\n", > - engine->name); > - /* Sometimes we observe that the idle flag is not > - * set even though the ring is empty. So double > - * check before giving up. > - */ > - if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) > - return false; > - } > + ret = intel_engine_stop_cs(engine); > + if (ret == -ENODEV) > + ret = 0; > + > + if (ret) { > + /* > + * Sometimes we observe that the idle flag is not > + * set even though the ring is empty. So double > + * check before giving up. > + */ > + if (!ring_is_empty(engine)) > + return false; Hmm, thinking more about this, shouldn't we push this down to stop_cs()? If that's reporting an error in a situation where we can determine that the ring is idle anyway, we can report the stop_cs succeeded. -Chris
Chris Wilson <chris@chris-wilson.co.uk> writes: > Quoting Mika Kuoppala (2019-02-28 16:14:11) >> We have an exported function for stopping the engine before >> disabling a ringbuffer. Take it into use. >> >> v2: use fw on empty check >> v3: tail is tail >> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> >> --- >> drivers/gpu/drm/i915/intel_engine_cs.c | 3 ++ >> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++----------- >> 2 files changed, 26 insertions(+), 18 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c >> index df8f88142f1d..e35dc0386bf6 100644 >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c >> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine) >> { >> struct drm_i915_private *dev_priv = engine->i915; >> >> + if (INTEL_GEN(dev_priv) < 3) >> + return; >> + >> GEM_TRACE("%s\n", engine->name); >> >> I915_WRITE_FW(RING_MI_MODE(engine->mmio_base), >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c >> index 1b96b0960adc..5fe28d9087b7 100644 >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c >> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) >> flush_cs_tlb(engine); >> } >> >> +static bool ring_is_empty(struct intel_engine_cs *engine) >> +{ >> + struct drm_i915_private *dev_priv = engine->i915; >> + const u32 base = engine->mmio_base; >> + >> + return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) == >> + (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR); >> +} >> + >> static bool stop_ring(struct intel_engine_cs *engine) >> { >> struct drm_i915_private *dev_priv = engine->i915; >> + int ret; >> >> - if (INTEL_GEN(dev_priv) > 2) { >> - I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING)); >> - if (intel_wait_for_register(dev_priv, >> - RING_MI_MODE(engine->mmio_base), >> - MODE_IDLE, >> - MODE_IDLE, >> - 1000)) { >> - DRM_ERROR("%s : timed out trying to stop ring\n", >> - engine->name); >> - /* Sometimes we observe that the idle flag is not >> - * set even though the ring is empty. So double >> - * check before giving up. >> - */ >> - if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) >> - return false; >> - } >> + ret = intel_engine_stop_cs(engine); >> + if (ret == -ENODEV) >> + ret = 0; >> + >> + if (ret) { >> + /* >> + * Sometimes we observe that the idle flag is not >> + * set even though the ring is empty. So double >> + * check before giving up. >> + */ >> + if (!ring_is_empty(engine)) >> + return false; > > Hmm, thinking more about this, shouldn't we push this down to stop_cs()? > > If that's reporting an error in a situation where we can determine that > the ring is idle anyway, we can report the stop_cs succeeded. Makes sense, I will take a look. I felt small urge to deflate the 'stop'. Would it be confusing if we just did intel_engine_start|stop instead of stop_cs and cancel_stop_cs? So hmm: intel_engine_start() intel_engine_stop() these would only toggle the STOP_RING and for replacing stop_ring with: intel_engine_empty_ring() for zeroing the heads. one 'stop' to rule the (ring)world!? -Mika
Quoting Mika Kuoppala (2019-02-28 16:53:46) > Chris Wilson <chris@chris-wilson.co.uk> writes: > > > Quoting Mika Kuoppala (2019-02-28 16:14:11) > >> We have an exported function for stopping the engine before > >> disabling a ringbuffer. Take it into use. > >> > >> v2: use fw on empty check > >> v3: tail is tail > >> > >> Cc: Chris Wilson <chris@chris-wilson.co.uk> > >> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > >> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> > >> --- > >> drivers/gpu/drm/i915/intel_engine_cs.c | 3 ++ > >> drivers/gpu/drm/i915/intel_ringbuffer.c | 41 ++++++++++++++----------- > >> 2 files changed, 26 insertions(+), 18 deletions(-) > >> > >> diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > >> index df8f88142f1d..e35dc0386bf6 100644 > >> --- a/drivers/gpu/drm/i915/intel_engine_cs.c > >> +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > >> @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine) > >> { > >> struct drm_i915_private *dev_priv = engine->i915; > >> > >> + if (INTEL_GEN(dev_priv) < 3) > >> + return; > >> + > >> GEM_TRACE("%s\n", engine->name); > >> > >> I915_WRITE_FW(RING_MI_MODE(engine->mmio_base), > >> diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> index 1b96b0960adc..5fe28d9087b7 100644 > >> --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > >> +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > >> @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) > >> flush_cs_tlb(engine); > >> } > >> > >> +static bool ring_is_empty(struct intel_engine_cs *engine) > >> +{ > >> + struct drm_i915_private *dev_priv = engine->i915; > >> + const u32 base = engine->mmio_base; > >> + > >> + return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) == > >> + (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR); > >> +} > >> + > >> static bool stop_ring(struct intel_engine_cs *engine) > >> { > >> struct drm_i915_private *dev_priv = engine->i915; > >> + int ret; > >> > >> - if (INTEL_GEN(dev_priv) > 2) { > >> - I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING)); > >> - if (intel_wait_for_register(dev_priv, > >> - RING_MI_MODE(engine->mmio_base), > >> - MODE_IDLE, > >> - MODE_IDLE, > >> - 1000)) { > >> - DRM_ERROR("%s : timed out trying to stop ring\n", > >> - engine->name); > >> - /* Sometimes we observe that the idle flag is not > >> - * set even though the ring is empty. So double > >> - * check before giving up. > >> - */ > >> - if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) > >> - return false; > >> - } > >> + ret = intel_engine_stop_cs(engine); > >> + if (ret == -ENODEV) > >> + ret = 0; > >> + > >> + if (ret) { > >> + /* > >> + * Sometimes we observe that the idle flag is not > >> + * set even though the ring is empty. So double > >> + * check before giving up. > >> + */ > >> + if (!ring_is_empty(engine)) > >> + return false; > > > > Hmm, thinking more about this, shouldn't we push this down to stop_cs()? > > > > If that's reporting an error in a situation where we can determine that > > the ring is idle anyway, we can report the stop_cs succeeded. > > Makes sense, I will take a look. > > I felt small urge to deflate the 'stop'. > > Would it be confusing if we just did > intel_engine_start|stop instead of stop_cs and > cancel_stop_cs? > > So hmm: > > intel_engine_start() > intel_engine_stop() > > these would only toggle the STOP_RING > > and for replacing stop_ring with: > intel_engine_empty_ring() intel_engine_clear_ring() / reset_ring(). Hmm, clear_ring of those two. > > for zeroing the heads. > > one 'stop' to rule the (ring)world!? The counter argument is that _start() is a little too broad. The appeal of stop_cs() is that it describing what it is doing, poking at the bit to stop the CS advancing and nothing more. It frequently doesn't succeed... So I think it's not a worthy change, but I never did feel totally satisfied with stop_cs -- cs is too short, but we do have usage with xCS. intel_engine_stop_command_streamer, intel_engine_halt_command_streamer, intel_engine_pause_command_streamer, ? But intel_engine_clear_ring() I could be sold on. -Chris
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index df8f88142f1d..e35dc0386bf6 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -856,6 +856,9 @@ void intel_engine_cancel_stop_cs(struct intel_engine_cs *engine) { struct drm_i915_private *dev_priv = engine->i915; + if (INTEL_GEN(dev_priv) < 3) + return; + GEM_TRACE("%s\n", engine->name); I915_WRITE_FW(RING_MI_MODE(engine->mmio_base), diff --git a/drivers/gpu/drm/i915/intel_ringbuffer.c b/drivers/gpu/drm/i915/intel_ringbuffer.c index 1b96b0960adc..5fe28d9087b7 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -604,26 +604,32 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) flush_cs_tlb(engine); } +static bool ring_is_empty(struct intel_engine_cs *engine) +{ + struct drm_i915_private *dev_priv = engine->i915; + const u32 base = engine->mmio_base; + + return (I915_READ_FW(RING_HEAD(base)) & HEAD_ADDR) == + (I915_READ_FW(RING_TAIL(base)) & TAIL_ADDR); +} + static bool stop_ring(struct intel_engine_cs *engine) { struct drm_i915_private *dev_priv = engine->i915; + int ret; - if (INTEL_GEN(dev_priv) > 2) { - I915_WRITE_MODE(engine, _MASKED_BIT_ENABLE(STOP_RING)); - if (intel_wait_for_register(dev_priv, - RING_MI_MODE(engine->mmio_base), - MODE_IDLE, - MODE_IDLE, - 1000)) { - DRM_ERROR("%s : timed out trying to stop ring\n", - engine->name); - /* Sometimes we observe that the idle flag is not - * set even though the ring is empty. So double - * check before giving up. - */ - if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) - return false; - } + ret = intel_engine_stop_cs(engine); + if (ret == -ENODEV) + ret = 0; + + if (ret) { + /* + * Sometimes we observe that the idle flag is not + * set even though the ring is empty. So double + * check before giving up. + */ + if (!ring_is_empty(engine)) + return false; } I915_WRITE_HEAD(engine, I915_READ_TAIL(engine)); @@ -718,8 +724,7 @@ static int init_ring_common(struct intel_engine_cs *engine) goto out; } - if (INTEL_GEN(dev_priv) > 2) - I915_WRITE_MODE(engine, _MASKED_BIT_DISABLE(STOP_RING)); + intel_engine_cancel_stop_cs(engine); /* Now awake, let it get started */ if (ring->tail != ring->head) {