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@xxxxxxxxxxxxxxx>
> >Signed-off-by: Chris Wilson <chris@xxxxxxxxxxxxxxxxxx>
> >Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion")
> >Cc: Tvrtko Ursulin <tvrtko.ursulin@xxxxxxxxxxxxxxx>
> >Cc: Joonas Lahtinen <joonas.lahtinen@xxxxxxxxxxxxxxx>
> >Cc: Oscar Mateo <oscar.mateo@xxxxxxxxx>
> >Cc: Daniele Ceraolo Spurio <daniele.ceraolospurio@xxxxxxxxx>
> >Cc: Michal Wajdeczko <michal.wajdeczko@xxxxxxxxx>
> >Cc: Arkadiusz Hiler <arkadiusz.hiler@xxxxxxxxx>
> >Cc: <stable@xxxxxxxxxxxxxxx> # 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@xxxxxxxxx>
>
> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/intel-gfx
