[PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[Chris Wilson]

Commit 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon
insertion") added the restoration of the invalidation routine after the
GuC was disabled, but missed that the GuC was unconditionally disabled
when not used. This then overwrites the invalidate routine for the older
chipsets, causing havoc and breaking resume as the most obvious victim.

We place the guard inside i915_ggtt_disable_guc() to be backport
friendly (the bug was introduced into v4.11) but it would be preferred
to be in more control over when this was guard (i.e. do not try and
teardown the data structures before we have enabled them). That should
be true with the reorganisation of the guc loaders.

Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
Cc: Oscar Mateo <oscar.mateo@intel.com>
Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
Cc: <stable@vger.kernel.org> # v4.11+
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index 4f581adf2fcf..6eb83684b97b 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -3282,7 +3282,8 @@ void i915_ggtt_enable_guc(struct drm_i915_private *i915)
 
 void i915_ggtt_disable_guc(struct drm_i915_private *i915)
 {
-	i915->ggtt.invalidate = gen6_ggtt_invalidate;
+	if (i915->ggtt.invalidate == guc_ggtt_invalidate)
+		i915->ggtt.invalidate = gen6_ggtt_invalidate;
 }
 
 void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
-- 
2.11.0

Re: [Intel-gfx] [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[Michel Thierry]

On 5/31/2017 12:05 PM, Chris Wilson wrote:
> Commit 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon
> insertion") added the restoration of the invalidation routine after the
> GuC was disabled, but missed that the GuC was unconditionally disabled
> when not used. This then overwrites the invalidate routine for the older
> chipsets, causing havoc and breaking resume as the most obvious victim.
>
> We place the guard inside i915_ggtt_disable_guc() to be backport
> friendly (the bug was introduced into v4.11) but it would be preferred
> to be in more control over when this was guard (i.e. do not try and
> teardown the data structures before we have enabled them). That should
> be true with the reorganisation of the guc loaders.
>
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: <stable@vger.kernel.org> # v4.11+
> ---
>  drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index 4f581adf2fcf..6eb83684b97b 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -3282,7 +3282,8 @@ void i915_ggtt_enable_guc(struct drm_i915_private *i915)
>
>  void i915_ggtt_disable_guc(struct drm_i915_private *i915)
>  {
> -       i915->ggtt.invalidate = gen6_ggtt_invalidate;
> +       if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> +               i915->ggtt.invalidate = gen6_ggtt_invalidate;
>  }
>
>  void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)

Reviewed-by: Michel Thierry <michel.thierry@intel.com>

btw the bug can only happen in 4.11; in 4.12+ this safeguard is already 
redundant since enable_guc_loading should be zero and ggtt_disable_guc 
is never called.

-Michel

Re: [Intel-gfx] [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[Chris Wilson]

On Wed, May 31, 2017 at 06:01:10PM -0700, Michel Thierry wrote:
> On 5/31/2017 12:05 PM, Chris Wilson wrote:
> >Commit 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon
> >insertion") added the restoration of the invalidation routine after the
> >GuC was disabled, but missed that the GuC was unconditionally disabled
> >when not used. This then overwrites the invalidate routine for the older
> >chipsets, causing havoc and breaking resume as the most obvious victim.
> >
> >We place the guard inside i915_ggtt_disable_guc() to be backport
> >friendly (the bug was introduced into v4.11) but it would be preferred
> >to be in more control over when this was guard (i.e. do not try and
> >teardown the data structures before we have enabled them). That should
> >be true with the reorganisation of the guc loaders.
> >
> >Reported-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >Cc: Oscar Mateo <oscar.mateo@intel.com>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> >Cc: <stable@vger.kernel.org> # v4.11+
> >---
> > drivers/gpu/drm/i915/i915_gem_gtt.c | 3 ++-
> > 1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >index 4f581adf2fcf..6eb83684b97b 100644
> >--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> >+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> >@@ -3282,7 +3282,8 @@ void i915_ggtt_enable_guc(struct drm_i915_private *i915)
> >
> > void i915_ggtt_disable_guc(struct drm_i915_private *i915)
> > {
> >-       i915->ggtt.invalidate = gen6_ggtt_invalidate;
> >+       if (i915->ggtt.invalidate == guc_ggtt_invalidate)
> >+               i915->ggtt.invalidate = gen6_ggtt_invalidate;
> > }
> >
> > void i915_gem_restore_gtt_mappings(struct drm_i915_private *dev_priv)
> 
> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> 
> btw the bug can only happen in 4.11; in 4.12+ this safeguard is
> already redundant since enable_guc_loading should be zero and
> ggtt_disable_guc is never called.

That's what I wanted to hear, thanks! After landing this we can then
replace the if() with a GEM_BUG_ON.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [Intel-gfx] [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[Chris Wilson]

On Wed, May 31, 2017 at 06:01:10PM -0700, Michel Thierry wrote:
> On 5/31/2017 12:05 PM, Chris Wilson wrote:
> >Commit 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon
> >insertion") added the restoration of the invalidation routine after the
> >GuC was disabled, but missed that the GuC was unconditionally disabled
> >when not used. This then overwrites the invalidate routine for the older
> >chipsets, causing havoc and breaking resume as the most obvious victim.
> >
> >We place the guard inside i915_ggtt_disable_guc() to be backport
> >friendly (the bug was introduced into v4.11) but it would be preferred
> >to be in more control over when this was guard (i.e. do not try and
> >teardown the data structures before we have enabled them). That should
> >be true with the reorganisation of the guc loaders.
> >
> >Reported-by: Ville Syrj�l� <ville.syrjala@linux.intel.com>
> >Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> >Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> >Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >Cc: Oscar Mateo <oscar.mateo@intel.com>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> >Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> >Cc: <stable@vger.kernel.org> # v4.11+

> Reviewed-by: Michel Thierry <michel.thierry@intel.com>
> 
> btw the bug can only happen in 4.11; in 4.12+ this safeguard is
> already redundant since enable_guc_loading should be zero and
> ggtt_disable_guc is never called.

Thanks for the review, pushed so we can get it back to where it needs to
go. I'll send a patch to convert this to an assert in a moment.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally

[Joonas Lahtinen]

On ke, 2017-05-31 at 20:05 +0100, Chris Wilson wrote:
> Commit 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon
> insertion") added the restoration of the invalidation routine after the
> GuC was disabled, but missed that the GuC was unconditionally disabled
> when not used. This then overwrites the invalidate routine for the older
> chipsets, causing havoc and breaking resume as the most obvious victim.
> 
> We place the guard inside i915_ggtt_disable_guc() to be backport
> friendly (the bug was introduced into v4.11) but it would be preferred
> to be in more control over when this was guard (i.e. do not try and
> teardown the data structures before we have enabled them). That should
> be true with the reorganisation of the guc loaders.
> 
> Reported-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
> Cc: Tvrtko Ursulin <tvrtko.ursulin@linux.intel.com>
> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> Cc: Oscar Mateo <oscar.mateo@intel.com>
> Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Arkadiusz Hiler <arkadiusz.hiler@intel.com>
> Cc: <stable@vger.kernel.org> # v4.11+

Reviewed-by: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>

Regards, Joonas
-- 
Joonas Lahtinen
Open Source Technology Center
Intel Corporation