Re: [PATCH 1/2] drm/i915: Cancel reset preparations on failed resets

[Chris Wilson <chris@chris-wilson.co.uk>]

Quoting Mika Kuoppala (2018-05-30 16:02:05)
> Our reset handling has a retry layer further up in the
> chain. As we have told the engine to prepare for reset,
> and failed it, make sure to remove that preparation so
> that the next attempted reset has a clean slate by triggering
> another full prepare cycle for the engines. Note that with
> successful reset, there is nothing to cleanup.
> 
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Mika Kuoppala <mika.kuoppala@linux.intel.com>
> ---
>  drivers/gpu/drm/i915/intel_uncore.c | 20 ++++++++++++--------
>  1 file changed, 12 insertions(+), 8 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_uncore.c b/drivers/gpu/drm/i915/intel_uncore.c
> index b36a3b5736a0..68fe4c16acfb 100644
> --- a/drivers/gpu/drm/i915/intel_uncore.c
> +++ b/drivers/gpu/drm/i915/intel_uncore.c
> @@ -2092,22 +2092,26 @@ static int gen8_reset_engines(struct drm_i915_private *dev_priv,
>                               unsigned engine_mask)
>  {
>         struct intel_engine_cs *engine;
> -       unsigned int tmp;
> +       unsigned int tmp, ret;

int ret; Might as well keep the types clean.

> -       for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
> -               if (gen8_reset_engine_start(engine))
> +       for_each_engine_masked(engine, dev_priv, engine_mask, tmp) {
> +               if (gen8_reset_engine_start(engine)) {
> +                       ret = -EIO;
>                         goto not_ready;
> +               }
> +       }
>  
>         if (INTEL_GEN(dev_priv) >= 11)
> -               return gen11_reset_engines(dev_priv, engine_mask);
> +               ret = gen11_reset_engines(dev_priv, engine_mask);
>         else
> -               return gen6_reset_engines(dev_priv, engine_mask);
> +               ret = gen6_reset_engines(dev_priv, engine_mask);
>  
>  not_ready:
> -       for_each_engine_masked(engine, dev_priv, engine_mask, tmp)
> -               gen8_reset_engine_cancel(engine);
> +       if (ret)

Or we just always clear the bit here. On the success path, it will be
just a nop. On the not_ready path we're already clearing untouched
engines, so might as well go whole hog and overkill everything?

Might as well throw in a
References: https://bugs.freedesktop.org/show_bug.cgi?id=106684
-Chris
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx