From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail.fireflyinternet.com ([109.228.58.192]:56099 "EHLO fireflyinternet.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750926AbdFAGDn (ORCPT ); Thu, 1 Jun 2017 02:03:43 -0400 Date: Thu, 1 Jun 2017 07:03:35 +0100 From: Chris Wilson To: Michel Thierry Cc: "intel-gfx@lists.freedesktop.org" , "# v4 . 11+" Subject: Re: [Intel-gfx] [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally Message-ID: <20170601060335.GC23936@nuc-i3427.alporthouse.com> References: <20170531190514.3691-1-chris@chris-wilson.co.uk> <2a537571-cb16-2ffe-36b1-69b21ccac98b@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <2a537571-cb16-2ffe-36b1-69b21ccac98b@intel.com> Sender: stable-owner@vger.kernel.org List-ID: 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� > >Signed-off-by: Chris Wilson > >Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion") > >Cc: Tvrtko Ursulin > >Cc: Joonas Lahtinen > >Cc: Oscar Mateo > >Cc: Daniele Ceraolo Spurio > >Cc: Michal Wajdeczko > >Cc: Arkadiusz Hiler > >Cc: # 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 > > 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 From mboxrd@z Thu Jan 1 00:00:00 1970 From: Chris Wilson Subject: Re: [Intel-gfx] [PATCH] drm/i915: Guard against i915_ggtt_disable_guc() being invoked unconditionally Date: Thu, 1 Jun 2017 07:03:35 +0100 Message-ID: <20170601060335.GC23936@nuc-i3427.alporthouse.com> References: <20170531190514.3691-1-chris@chris-wilson.co.uk> <2a537571-cb16-2ffe-36b1-69b21ccac98b@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Transfer-Encoding: 8bit Return-path: Content-Disposition: inline In-Reply-To: <2a537571-cb16-2ffe-36b1-69b21ccac98b@intel.com> Sender: stable-owner@vger.kernel.org To: Michel Thierry Cc: "intel-gfx@lists.freedesktop.org" , "# v4 . 11+" List-Id: intel-gfx@lists.freedesktop.org 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ä > >Signed-off-by: Chris Wilson > >Fixes: 7c3f86b6dc51 ("drm/i915: Invalidate the guc ggtt TLB upon insertion") > >Cc: Tvrtko Ursulin > >Cc: Joonas Lahtinen > >Cc: Oscar Mateo > >Cc: Daniele Ceraolo Spurio > >Cc: Michal Wajdeczko > >Cc: Arkadiusz Hiler > >Cc: # 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 > > 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