Message ID | 20170929161032.24394-12-matthew.auld@intel.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
On Fri, 2017-09-29 at 17:10 +0100, Matthew Auld wrote: > When SW enables the use of 2M/1G pages, it must disable the GTT cache. > > v2: don't disable for Cherryview which doesn't even support 48b PPGTT! > > v3: explicitly check that the system does support 2M/1G pages > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > Cc: Chris Wilson <chris@chris-wilson.co.uk> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> <SNIP> > @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) > > /* > * WaGttCachingOffByDefault:bdw > - * GTT cache may not work with big pages, so if those > - * are ever enabled GTT cache may need to be disabled. > + * The GTT cache must be disabled if the system is using 2M/1G pages. > */ > - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); > + I915_WRITE(HSW_GTT_CACHE_EN, > + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : > + GTT_CACHE_EN_ALL); Umm, this is mixing a known W/A with decision logic. bool can_use_gtt_cache = !HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M); /* WaGttCachingOffByDefault:bdw */ I915_WRITE(HSW_GTT_CACHE_EN, can_use_gtt_cache ? GTT_CACHE_EN_ALL : 0); The big question is that if everyone else has GTT caching enabled by default, should not we actively be disabling it on other code paths? I'm also feeling 'init_clock_gating' is not the best home for the code. We can find a new home later, but do separate the decision logic from the W/A. Also, do 1G pages imply 2M page support? It's bit on the theoritical side of science of course. Regards, Joonas
On 2 October 2017 at 13:31, Joonas Lahtinen <joonas.lahtinen@linux.intel.com> wrote: > On Fri, 2017-09-29 at 17:10 +0100, Matthew Auld wrote: >> When SW enables the use of 2M/1G pages, it must disable the GTT cache. >> >> v2: don't disable for Cherryview which doesn't even support 48b PPGTT! >> >> v3: explicitly check that the system does support 2M/1G pages >> >> Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> Cc: Chris Wilson <chris@chris-wilson.co.uk> >> Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > <SNIP> > >> @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) >> >> /* >> * WaGttCachingOffByDefault:bdw >> - * GTT cache may not work with big pages, so if those >> - * are ever enabled GTT cache may need to be disabled. >> + * The GTT cache must be disabled if the system is using 2M/1G pages. >> */ >> - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); >> + I915_WRITE(HSW_GTT_CACHE_EN, >> + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : >> + GTT_CACHE_EN_ALL); > > Umm, this is mixing a known W/A with decision logic. > > bool can_use_gtt_cache = !HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M); > > /* WaGttCachingOffByDefault:bdw */ > I915_WRITE(HSW_GTT_CACHE_EN, can_use_gtt_cache ? GTT_CACHE_EN_ALL : 0); > > The big question is that if everyone else has GTT caching enabled by > default, should not we actively be disabling it on other code paths? AFAIK it's *disabled* by default, bdw and chv are the only platforms which seek to enable it. I don't know why other platforms don't also follow suit... That WA is literally just: "WA to enable the caching if off by defaultboth at driver init and Resume". > I'm also feeling 'init_clock_gating' is not the best home for the code. > > We can find a new home later, but do separate the decision logic from > the W/A. > > Also, do 1G pages imply 2M page support? It's bit on the theoritical > side of science of course. I just assume not, since it doesn't explicitly state that anywhere. > > Regards, Joonas > -- > Joonas Lahtinen > Open Source Technology Center > Intel Corporation > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
+ Mika for more comments as the original reviewer On Mon, 2017-10-02 at 14:52 +0100, Matthew Auld wrote: > On 2 October 2017 at 13:31, Joonas Lahtinen > <joonas.lahtinen@linux.intel.com> wrote: > > On Fri, 2017-09-29 at 17:10 +0100, Matthew Auld wrote: > > > When SW enables the use of 2M/1G pages, it must disable the GTT cache. > > > > > > v2: don't disable for Cherryview which doesn't even support 48b PPGTT! > > > > > > v3: explicitly check that the system does support 2M/1G pages > > > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > > > <SNIP> > > > > > @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) > > > > > > /* > > > * WaGttCachingOffByDefault:bdw > > > - * GTT cache may not work with big pages, so if those > > > - * are ever enabled GTT cache may need to be disabled. > > > + * The GTT cache must be disabled if the system is using 2M/1G pages. > > > */ > > > - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); > > > + I915_WRITE(HSW_GTT_CACHE_EN, > > > + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : > > > + GTT_CACHE_EN_ALL); > > > > Umm, this is mixing a known W/A with decision logic. > > > > bool can_use_gtt_cache = !HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M); > > > > /* WaGttCachingOffByDefault:bdw */ > > I915_WRITE(HSW_GTT_CACHE_EN, can_use_gtt_cache ? GTT_CACHE_EN_ALL : 0); > > > > The big question is that if everyone else has GTT caching enabled by > > default, should not we actively be disabling it on other code paths? > > AFAIK it's *disabled* by default, bdw and chv are the only platforms > which seek to enable it. I don't know why other platforms don't also > follow suit... > > That WA is literally just: "WA to enable the caching if off by > defaultboth at driver init and Resume". > > > I'm also feeling 'init_clock_gating' is not the best home for the code. > > > > We can find a new home later, but do separate the decision logic from > > the W/A. > > > > Also, do 1G pages imply 2M page support? It's bit on the theoritical > > side of science of course. > > I just assume not, since it doesn't explicitly state that anywhere. > > > > > Regards, Joonas > > -- > > Joonas Lahtinen > > Open Source Technology Center > > Intel Corporation > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@lists.freedesktop.org > > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Mon, Oct 02, 2017 at 03:31:44PM +0300, Joonas Lahtinen wrote: > On Fri, 2017-09-29 at 17:10 +0100, Matthew Auld wrote: > > When SW enables the use of 2M/1G pages, it must disable the GTT cache. > > > > v2: don't disable for Cherryview which doesn't even support 48b PPGTT! > > > > v3: explicitly check that the system does support 2M/1G pages > > > > Signed-off-by: Matthew Auld <matthew.auld@intel.com> > > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> > > Cc: Chris Wilson <chris@chris-wilson.co.uk> > > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> > > <SNIP> > > > @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) > > > > /* > > * WaGttCachingOffByDefault:bdw > > - * GTT cache may not work with big pages, so if those > > - * are ever enabled GTT cache may need to be disabled. > > + * The GTT cache must be disabled if the system is using 2M/1G pages. > > */ > > - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); > > + I915_WRITE(HSW_GTT_CACHE_EN, > > + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : > > + GTT_CACHE_EN_ALL); > > Umm, this is mixing a known W/A with decision logic. > > bool can_use_gtt_cache = !HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M); > > /* WaGttCachingOffByDefault:bdw */ > I915_WRITE(HSW_GTT_CACHE_EN, can_use_gtt_cache ? GTT_CACHE_EN_ALL : 0); > > The big question is that if everyone else has GTT caching enabled by > default, should not we actively be disabling it on other code paths? HSW has it enabled by default. BDW B0 disabled it by default, even though the spec seems to suggest that the big pages vs. GTT cache issue may have been fixed properly already on BDW. Did anyone actually test whether big pages work with the GTT cache enabled? Bspec is a bit of a mess these days so it's kinda hard to see what the situation is with SKL+, but it looks like the default is still 0x0. So seems to me that someone should actually explicitly enable GTT cache on SKL+. Enabling it did prodcuce a slight performance increase on BDW/CHV IIRC.
On 9 October 2017 at 13:07, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote: > On Mon, Oct 02, 2017 at 03:31:44PM +0300, Joonas Lahtinen wrote: >> On Fri, 2017-09-29 at 17:10 +0100, Matthew Auld wrote: >> > When SW enables the use of 2M/1G pages, it must disable the GTT cache. >> > >> > v2: don't disable for Cherryview which doesn't even support 48b PPGTT! >> > >> > v3: explicitly check that the system does support 2M/1G pages >> > >> > Signed-off-by: Matthew Auld <matthew.auld@intel.com> >> > Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com> >> > Cc: Chris Wilson <chris@chris-wilson.co.uk> >> > Cc: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> > Reviewed-by: Mika Kuoppala <mika.kuoppala@linux.intel.com> >> >> <SNIP> >> >> > @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) >> > >> > /* >> > * WaGttCachingOffByDefault:bdw >> > - * GTT cache may not work with big pages, so if those >> > - * are ever enabled GTT cache may need to be disabled. >> > + * The GTT cache must be disabled if the system is using 2M/1G pages. >> > */ >> > - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); >> > + I915_WRITE(HSW_GTT_CACHE_EN, >> > + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : >> > + GTT_CACHE_EN_ALL); >> >> Umm, this is mixing a known W/A with decision logic. >> >> bool can_use_gtt_cache = !HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M); >> >> /* WaGttCachingOffByDefault:bdw */ >> I915_WRITE(HSW_GTT_CACHE_EN, can_use_gtt_cache ? GTT_CACHE_EN_ALL : 0); >> >> The big question is that if everyone else has GTT caching enabled by >> default, should not we actively be disabling it on other code paths? > > HSW has it enabled by default. BDW B0 disabled it by default, even > though the spec seems to suggest that the big pages vs. GTT cache issue > may have been fixed properly already on BDW. Did anyone actually test > whether big pages work with the GTT cache enabled? Back when I was originally testing 2M pages on my BDW machine, I was seeing lots of nasty visual corruption, and so only stumbled on disabling the GTT cache after. The bit in the spec which caught my eye at the time[1]. It might be worth making sure the same holds true for SKL+.... [1] https://gfxspecs.intel.com/Predator/Home/Index/423
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index c66af09e27a7..29c68c2a99ef 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -8483,10 +8483,11 @@ static void bdw_init_clock_gating(struct drm_i915_private *dev_priv) /* * WaGttCachingOffByDefault:bdw - * GTT cache may not work with big pages, so if those - * are ever enabled GTT cache may need to be disabled. + * The GTT cache must be disabled if the system is using 2M/1G pages. */ - I915_WRITE(HSW_GTT_CACHE_EN, GTT_CACHE_EN_ALL); + I915_WRITE(HSW_GTT_CACHE_EN, + HAS_PAGE_SIZES(dev_priv, I915_GTT_PAGE_SIZE_2M) ? 0 : + GTT_CACHE_EN_ALL); /* WaKVMNotificationOnConfigChange:bdw */ I915_WRITE(CHICKEN_PAR2_1, I915_READ(CHICKEN_PAR2_1)