[PATCH 1/6] drm/i915/guc: Update ggtt invalidate function prior to reset/suspend

[Kamble, Sagar A]

-----Original Message-----
From: Chris Wilson [mailto:chris at chris-wilson.co.uk] 
Sent: Wednesday, August 2, 2017 7:19 PM
To: Kamble, Sagar A <sagar.a.kamble at intel.com>; Intel-gfx-trybot at lists.freedesktop.org
Cc: Kamble, Sagar A <sagar.a.kamble at intel.com>; Wajdeczko, Michal <Michal.Wajdeczko at intel.com>; Ceraolo Spurio, Daniele <daniele.ceraolospurio at intel.com>; Sarvela, Tomi P <tomi.p.sarvela at intel.com>
Subject: Re: [PATCH 1/6] drm/i915/guc: Update ggtt invalidate function prior to reset/suspend

Quoting Sagar Arun Kamble (2017-08-02 14:29:22)
> drv_hangman at error-state-basic and gem_exec_suspend at basic-s3 caught 
> below kernel bug with GuC.
> 
> kernel BUG at drivers/gpu/drm/i915/i915_gem_gtt.c:3086!
> GEM_BUG_ON(i915->ggtt.invalidate != gen6_ggtt_invalidate);
> 
> This was happening since ggtt_disable_guc was not done prior to reset 
> and suspend.
> 
> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> Cc: Michal Wajdeczko <michal.wajdeczko at intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio at intel.com>
> Cc: Tomi Sarvela <tomi.p.sarvela at intel.com>
> Signed-off-by: Sagar Arun Kamble <sagar.a.kamble at intel.com>
> ---
>  drivers/gpu/drm/i915/i915_gem.c | 6 ++++++
>  1 file changed, 6 insertions(+)
> 
> diff --git a/drivers/gpu/drm/i915/i915_gem.c 
> b/drivers/gpu/drm/i915/i915_gem.c index 000a764..a1c450e 100644
> --- a/drivers/gpu/drm/i915/i915_gem.c
> +++ b/drivers/gpu/drm/i915/i915_gem.c
> @@ -2984,6 +2984,9 @@ void i915_gem_reset(struct drm_i915_private *dev_priv)
>                 if (INTEL_GEN(dev_priv) >= 6)
>                         gen6_rps_busy(dev_priv);
>         }
> +
> +       if (i915.enable_guc_loading)
> +               i915_ggtt_disable_guc(dev_priv);

Missing intel_guc_reset() ?

<Sagar> intel_guc_reset is currently managing only hw reset. Ordering of that with ggtt_enable setup is broken. Will have to create guc_interface_reset routine.

>  }
>  
>  void i915_gem_reset_finish_engine(struct intel_engine_cs *engine) @@ 
> -4557,6 +4560,9 @@ int i915_gem_suspend(struct drm_i915_private 
> *dev_priv)
>  
>         intel_guc_suspend(dev_priv);
>  
> +       if (i915.enable_guc_loading)
> +               i915_ggtt_disable_guc(dev_priv);
> +

Then why not part of intel_guc_suspend() ?
-Chris

<Sagar> intel_guc_suspend is being done from RPM path too where we need not change the ggtt_enable setup.
Latest trybot result show ggtt_enable ordering issue in driver_unload now as, unload path does gem_suspend and then touches GuC teardown.
Will need to fix that.