| Message ID | 20190227165850.17277-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 |
Mika Kuoppala <mika.kuoppala@linux.intel.com> writes: > We have an exported function for stopping the engine before > disabling a ringbuffer. Take it into use. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++-------------- > 2 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 4f244019560d..3feb0f74c239 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -817,6 +817,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..5363dad1208d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) > 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 (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) Hmm these could have been also converted to the _FW variants. -Mika > + return false; > } > > I915_WRITE_HEAD(engine, I915_READ_TAIL(engine)); > @@ -718,8 +714,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) { > -- > 2.17.1
Quoting Mika Kuoppala (2019-02-27 16:58:48) > We have an exported function for stopping the engine before > disabling a ringbuffer. Take it into use. > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > --- > drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++ > drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++-------------- > 2 files changed, 16 insertions(+), 18 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > index 4f244019560d..3feb0f74c239 100644 > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > @@ -817,6 +817,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..5363dad1208d 100644 > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) > 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 As you go past, gradually switch over to /* * Sometimes... style. The joys of a slow transition to uniformity and switching styles half way through. > + * set even though the ring is empty. So double > + * check before giving up. > + */ > + if (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) > + return false; > } > > I915_WRITE_HEAD(engine, I915_READ_TAIL(engine)); > @@ -718,8 +714,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) { Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk> -Chris
Quoting Mika Kuoppala (2019-02-27 17:03:38) > Mika Kuoppala <mika.kuoppala@linux.intel.com> writes: > > > We have an exported function for stopping the engine before > > disabling a ringbuffer. Take it into use. > > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > --- > > drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++ > > drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++-------------- > > 2 files changed, 16 insertions(+), 18 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c > > index 4f244019560d..3feb0f74c239 100644 > > --- a/drivers/gpu/drm/i915/intel_engine_cs.c > > +++ b/drivers/gpu/drm/i915/intel_engine_cs.c > > @@ -817,6 +817,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..5363dad1208d 100644 > > --- a/drivers/gpu/drm/i915/intel_ringbuffer.c > > +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c > > @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) > > 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 (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) > > Hmm these could have been also converted to the _FW variants. Sneak it in. -Chris
Quoting Patchwork (2019-02-27 17:41:59) > #### Possible regressions #### > > * igt@i915_selftest@live_active: > - fi-apl-guc: PASS -> DMESG-WARN Second flip due to "drm/i915: Avoid waking the engines just to check if they are idle", both skl. Suggests that its passing earlier was fluke. -Chris
diff --git a/drivers/gpu/drm/i915/intel_engine_cs.c b/drivers/gpu/drm/i915/intel_engine_cs.c index 4f244019560d..3feb0f74c239 100644 --- a/drivers/gpu/drm/i915/intel_engine_cs.c +++ b/drivers/gpu/drm/i915/intel_engine_cs.c @@ -817,6 +817,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..5363dad1208d 100644 --- a/drivers/gpu/drm/i915/intel_ringbuffer.c +++ b/drivers/gpu/drm/i915/intel_ringbuffer.c @@ -607,23 +607,19 @@ static void ring_setup_status_page(struct intel_engine_cs *engine) 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 (I915_READ_HEAD(engine) != I915_READ_TAIL(engine)) + return false; } I915_WRITE_HEAD(engine, I915_READ_TAIL(engine)); @@ -718,8 +714,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) {
We have an exported function for stopping the engine before disabling a ringbuffer. Take it into use. Cc: Chris Wilson <chris@chris-wilson.co.uk> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> --- drivers/gpu/drm/i915/intel_engine_cs.c | 3 +++ drivers/gpu/drm/i915/intel_ringbuffer.c | 31 +++++++++++-------------- 2 files changed, 16 insertions(+), 18 deletions(-)