diff mbox

[11/21] drm/i915: disable GTT cache for 2M pages

Message ID 20170929161032.24394-12-matthew.auld@intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Matthew Auld Sept. 29, 2017, 4:10 p.m. UTC 
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>
---
 drivers/gpu/drm/i915/intel_pm.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

Comments

Joonas Lahtinen Oct. 2, 2017, 12:31 p.m. UTC | #1 
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
Matthew Auld Oct. 2, 2017, 1:52 p.m. UTC | #2 
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
Joonas Lahtinen Oct. 3, 2017, 12:32 p.m. UTC | #3 
+ 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
Ville Syrjälä Oct. 9, 2017, 12:07 p.m. UTC | #4 
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.
Matthew Auld Oct. 9, 2017, 1:04 p.m. UTC | #5 
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 mbox

Patch

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)