[Intel-gfx] [PATCH 2/3] drm/i915: Introduce intel_engine_stop_ringbuffer

[Chris Wilson]

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 <chris at chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala at 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 <chris at chris-wilson.co.uk>
-Chris