[PATCH] drm/i915/hsw: Change default LLC age to 3

[PATCH] drm/i915/hsw: Change default LLC age to 3

[Ben Widawsky]

The default LLC age was changed:
commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Thu Jul 4 11:02:03 2013 -0700

drm/i915/hsw: Set correct Haswell PTE encodings.

This caused a regression in performance on certain benchmarks. While I
think a discussion still needs to happen about how the kernel should
default for both eLLC, and LLC - just revert this behavior for now.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e7b4204..4b1e6e3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -52,8 +52,10 @@
  */
 #define HSW_CACHEABILITY_CONTROL(bits)	((((bits) & 0x7) << 1) | \
 					 (((bits) & 0x8) << (11 - 3)))
+#define HSW_WB_LLC_AGE3			HSW_CACHEABILITY_CONTROL(0x2)
 #define HSW_WB_LLC_AGE0			HSW_CACHEABILITY_CONTROL(0x3)
 #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
+#define HSW_LLC				HSW_WB_LLC_AGE3
 
 static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
 				      enum i915_cache_level level)
@@ -105,7 +107,7 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
 	pte |= HSW_PTE_ADDR_ENCODE(addr);
 
 	if (level != I915_CACHE_NONE)
-		pte |= HSW_WB_LLC_AGE0;
+		pte |= HSW_LLC;
 
 	return pte;
 }
-- 
1.8.3.4

Re: [PATCH] drm/i915/hsw: Change default LLC age to 3

[Chad Versace]

On 07/31/2013 04:07 PM, Ben Widawsky wrote:
> The default LLC age was changed:
> commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Thu Jul 4 11:02:03 2013 -0700
>
> drm/i915/hsw: Set correct Haswell PTE encodings.
>
> This caused a regression in performance on certain benchmarks. While I
> think a discussion still needs to happen about how the kernel should
> default for both eLLC, and LLC - just revert this behavior for now.
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> ---
>   drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
> index e7b4204..4b1e6e3 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -52,8 +52,10 @@
>    */
>   #define HSW_CACHEABILITY_CONTROL(bits)	((((bits) & 0x7) << 1) | \
>   					 (((bits) & 0x8) << (11 - 3)))
> +#define HSW_WB_LLC_AGE3			HSW_CACHEABILITY_CONTROL(0x2)
>   #define HSW_WB_LLC_AGE0			HSW_CACHEABILITY_CONTROL(0x3)
>   #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
> +#define HSW_LLC				HSW_WB_LLC_AGE3
>
>   static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
>   				      enum i915_cache_level level)
> @@ -105,7 +107,7 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
>   	pte |= HSW_PTE_ADDR_ENCODE(addr);
>
>   	if (level != I915_CACHE_NONE)
> -		pte |= HSW_WB_LLC_AGE0;
> +		pte |= HSW_LLC;
>
>   	return pte;
>   }
>

I think the patch is clearer without the wrapper define HSW_LLC. But,
anyways, with or without the wrapper,
Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

[PATCH] [v2] drm/i915/hsw: Change default LLC age to 3

[Ben Widawsky]

The default LLC age was changed:
commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Thu Jul 4 11:02:03 2013 -0700

drm/i915/hsw: Set correct Haswell PTE encodings.

This caused a regression in performance on certain benchmarks. While I
think a discussion still needs to happen about how the kernel should
default for both eLLC, and LLC - just revert this behavior for now.

v2: Drop the extra #define (Chad)

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
---
 drivers/gpu/drm/i915/i915_gem_gtt.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/i915_gem_gtt.c b/drivers/gpu/drm/i915/i915_gem_gtt.c
index e7b4204..4b1e6e3 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -52,8 +52,10 @@
  */
 #define HSW_CACHEABILITY_CONTROL(bits)	((((bits) & 0x7) << 1) | \
 					 (((bits) & 0x8) << (11 - 3)))
+#define HSW_WB_LLC_AGE3			HSW_CACHEABILITY_CONTROL(0x2)
 #define HSW_WB_LLC_AGE0			HSW_CACHEABILITY_CONTROL(0x3)
 #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
+#define HSW_LLC				HSW_WB_LLC_AGE3
 
 static gen6_gtt_pte_t gen6_pte_encode(dma_addr_t addr,
 				      enum i915_cache_level level)
@@ -105,7 +107,7 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
 	pte |= HSW_PTE_ADDR_ENCODE(addr);
 
 	if (level != I915_CACHE_NONE)
-		pte |= HSW_WB_LLC_AGE0;
+		pte |= HSW_LLC;
 
 	return pte;
 }
-- 
1.8.3.4

[PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Ben Widawsky]

The default LLC age was changed:
commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
Author: Ben Widawsky <benjamin.widawsky@intel.com>
Date:   Thu Jul 4 11:02:03 2013 -0700

drm/i915/hsw: Set correct Haswell PTE encodings.

This caused a regression in performance on certain benchmarks. While I
think a discussion still needs to happen about how the kernel should
default for both eLLC, and LLC - just revert this behavior for now.

v2: Drop the extra #define (Chad)

v3: Actually git add

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
---
 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 e7b4204..3e7f124 100644
--- a/drivers/gpu/drm/i915/i915_gem_gtt.c
+++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
@@ -52,6 +52,7 @@
  */
 #define HSW_CACHEABILITY_CONTROL(bits)	((((bits) & 0x7) << 1) | \
 					 (((bits) & 0x8) << (11 - 3)))
+#define HSW_WB_LLC_AGE3			HSW_CACHEABILITY_CONTROL(0x2)
 #define HSW_WB_LLC_AGE0			HSW_CACHEABILITY_CONTROL(0x3)
 #define HSW_WB_ELLC_LLC_AGE0		HSW_CACHEABILITY_CONTROL(0xb)
 
@@ -105,7 +106,7 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
 	pte |= HSW_PTE_ADDR_ENCODE(addr);
 
 	if (level != I915_CACHE_NONE)
-		pte |= HSW_WB_LLC_AGE0;
+		pte |= HSW_WB_LLC_AGE3;
 
 	return pte;
 }
-- 
1.8.3.4

Re: [PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Daniel Vetter]

On Fri, Aug 2, 2013 at 12:56 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> The default LLC age was changed:
> commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
> Author: Ben Widawsky <benjamin.widawsky@intel.com>
> Date:   Thu Jul 4 11:02:03 2013 -0700
>
> drm/i915/hsw: Set correct Haswell PTE encodings.
>
> This caused a regression in performance on certain benchmarks. While I
> think a discussion still needs to happen about how the kernel should
> default for both eLLC, and LLC - just revert this behavior for now.
>
> v2: Drop the extra #define (Chad)
>
> v3: Actually git add
>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
> Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> Reviewed-by: Chad Versace <chad.versace@linux.intel.com>

iris_pte_encode was split up after the regressing commit in

commit 4d15c145a6234d999c0452eec0d275c1fbf0688c
Author: Ben Widawsky <ben@bwidawsk.net>
Date:   Thu Jul 4 11:02:06 2013 -0700

    drm/i915: Use eLLC/LLC by default when available

Hence I think that one should be patched, too (QA doesn't have an iris
machine afaik, so no regression report expected). And since I have
some open questions about Chris WT-for-Iris patch too I think it's
best to spin a v4 of this one here.
-Daniel

> ---
>  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 e7b4204..3e7f124 100644
> --- a/drivers/gpu/drm/i915/i915_gem_gtt.c
> +++ b/drivers/gpu/drm/i915/i915_gem_gtt.c
> @@ -52,6 +52,7 @@
>   */
>  #define HSW_CACHEABILITY_CONTROL(bits) ((((bits) & 0x7) << 1) | \
>                                          (((bits) & 0x8) << (11 - 3)))
> +#define HSW_WB_LLC_AGE3                        HSW_CACHEABILITY_CONTROL(0x2)
>  #define HSW_WB_LLC_AGE0                        HSW_CACHEABILITY_CONTROL(0x3)
>  #define HSW_WB_ELLC_LLC_AGE0           HSW_CACHEABILITY_CONTROL(0xb)
>
> @@ -105,7 +106,7 @@ static gen6_gtt_pte_t hsw_pte_encode(dma_addr_t addr,
>         pte |= HSW_PTE_ADDR_ENCODE(addr);
>
>         if (level != I915_CACHE_NONE)
> -               pte |= HSW_WB_LLC_AGE0;
> +               pte |= HSW_WB_LLC_AGE3;
>
>         return pte;
>  }
> --
> 1.8.3.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Ben Widawsky]

On Sun, Aug 04, 2013 at 09:10:25PM +0200, Daniel Vetter wrote:
> On Fri, Aug 2, 2013 at 12:56 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > The default LLC age was changed:
> > commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
> > Author: Ben Widawsky <benjamin.widawsky@intel.com>
> > Date:   Thu Jul 4 11:02:03 2013 -0700
> >
> > drm/i915/hsw: Set correct Haswell PTE encodings.
> >
> > This caused a regression in performance on certain benchmarks. While I
> > think a discussion still needs to happen about how the kernel should
> > default for both eLLC, and LLC - just revert this behavior for now.
> >
> > v2: Drop the extra #define (Chad)
> >
> > v3: Actually git add
> >
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> > Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
> 
> iris_pte_encode was split up after the regressing commit in
> 
> commit 4d15c145a6234d999c0452eec0d275c1fbf0688c
> Author: Ben Widawsky <ben@bwidawsk.net>
> Date:   Thu Jul 4 11:02:06 2013 -0700
> 
>     drm/i915: Use eLLC/LLC by default when available
> 
> Hence I think that one should be patched, too (QA doesn't have an iris
> machine afaik, so no regression report expected). And since I have
> some open questions about Chris WT-for-Iris patch too I think it's
> best to spin a v4 of this one here.
> -Daniel
> 

We've come up with a theory as to why this was a regression (which I
think nullifies your request for the eLLC version)

On the surface, one would think that aging all objects equally, whether
they be oldest, or youngest, makes no difference. The key with LLC is
that it is shared with the CPU, and presumably the CPU follows the
normal cache aging FIFO rules (ie. everything is aged 3). On this logic,
eLLC would not share the same flaw. I think it makes sense to have
someone prove we need it for eLLC, as this does allow mesa/DDX to get 2
differently aged eLLC objects, which they cannot do if we default to 3.
On the same note, even LLC age 0 is still ideal in my opinion, but was
asked to not do this so that meas 9.2 doesn't have performance
regressions.


[snip]

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Daniel Vetter]

On Sun, Aug 4, 2013 at 10:55 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Sun, Aug 04, 2013 at 09:10:25PM +0200, Daniel Vetter wrote:
>> On Fri, Aug 2, 2013 at 12:56 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
>> > The default LLC age was changed:
>> > commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
>> > Author: Ben Widawsky <benjamin.widawsky@intel.com>
>> > Date:   Thu Jul 4 11:02:03 2013 -0700
>> >
>> > drm/i915/hsw: Set correct Haswell PTE encodings.
>> >
>> > This caused a regression in performance on certain benchmarks. While I
>> > think a discussion still needs to happen about how the kernel should
>> > default for both eLLC, and LLC - just revert this behavior for now.
>> >
>> > v2: Drop the extra #define (Chad)
>> >
>> > v3: Actually git add
>> >
>> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
>> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
>> > Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
>>
>> iris_pte_encode was split up after the regressing commit in
>>
>> commit 4d15c145a6234d999c0452eec0d275c1fbf0688c
>> Author: Ben Widawsky <ben@bwidawsk.net>
>> Date:   Thu Jul 4 11:02:06 2013 -0700
>>
>>     drm/i915: Use eLLC/LLC by default when available
>>
>> Hence I think that one should be patched, too (QA doesn't have an iris
>> machine afaik, so no regression report expected). And since I have
>> some open questions about Chris WT-for-Iris patch too I think it's
>> best to spin a v4 of this one here.
>> -Daniel
>>
>
> We've come up with a theory as to why this was a regression (which I
> think nullifies your request for the eLLC version)
>
> On the surface, one would think that aging all objects equally, whether
> they be oldest, or youngest, makes no difference. The key with LLC is
> that it is shared with the CPU, and presumably the CPU follows the
> normal cache aging FIFO rules (ie. everything is aged 3). On this logic,
> eLLC would not share the same flaw. I think it makes sense to have
> someone prove we need it for eLLC, as this does allow mesa/DDX to get 2
> differently aged eLLC objects, which they cannot do if we default to 3.
> On the same note, even LLC age 0 is still ideal in my opinion, but was
> asked to not do this so that meas 9.2 doesn't have performance
> regressions.

Well we have a get/set_caching ioctl, explictly with room for stuff
like aging. So this can be added in any case. But yeah if someone can
check whether we have the same regression on iris or not would be
good. Can you please poke QA (or whoever has one of these boxen)?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Ben Widawsky]

On Mon, Aug 05, 2013 at 01:03:29AM +0200, Daniel Vetter wrote:
> On Sun, Aug 4, 2013 at 10:55 PM, Ben Widawsky <ben@bwidawsk.net> wrote:
> > On Sun, Aug 04, 2013 at 09:10:25PM +0200, Daniel Vetter wrote:
> >> On Fri, Aug 2, 2013 at 12:56 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> >> > The default LLC age was changed:
> >> > commit 0d8ff15e9a15f2b393e53337a107b7a1e5919b6d
> >> > Author: Ben Widawsky <benjamin.widawsky@intel.com>
> >> > Date:   Thu Jul 4 11:02:03 2013 -0700
> >> >
> >> > drm/i915/hsw: Set correct Haswell PTE encodings.
> >> >
> >> > This caused a regression in performance on certain benchmarks. While I
> >> > think a discussion still needs to happen about how the kernel should
> >> > default for both eLLC, and LLC - just revert this behavior for now.
> >> >
> >> > v2: Drop the extra #define (Chad)
> >> >
> >> > v3: Actually git add
> >> >
> >> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=67062
> >> > Signed-off-by: Ben Widawsky <ben@bwidawsk.net>
> >> > Reviewed-by: Chad Versace <chad.versace@linux.intel.com>
> >>
> >> iris_pte_encode was split up after the regressing commit in
> >>
> >> commit 4d15c145a6234d999c0452eec0d275c1fbf0688c
> >> Author: Ben Widawsky <ben@bwidawsk.net>
> >> Date:   Thu Jul 4 11:02:06 2013 -0700
> >>
> >>     drm/i915: Use eLLC/LLC by default when available
> >>
> >> Hence I think that one should be patched, too (QA doesn't have an iris
> >> machine afaik, so no regression report expected). And since I have
> >> some open questions about Chris WT-for-Iris patch too I think it's
> >> best to spin a v4 of this one here.
> >> -Daniel
> >>
> >
> > We've come up with a theory as to why this was a regression (which I
> > think nullifies your request for the eLLC version)
> >
> > On the surface, one would think that aging all objects equally, whether
> > they be oldest, or youngest, makes no difference. The key with LLC is
> > that it is shared with the CPU, and presumably the CPU follows the
> > normal cache aging FIFO rules (ie. everything is aged 3). On this logic,
> > eLLC would not share the same flaw. I think it makes sense to have
> > someone prove we need it for eLLC, as this does allow mesa/DDX to get 2
> > differently aged eLLC objects, which they cannot do if we default to 3.
> > On the same note, even LLC age 0 is still ideal in my opinion, but was
> > asked to not do this so that meas 9.2 doesn't have performance
> > regressions.
> 
> Well we have a get/set_caching ioctl, explictly with room for stuff
> like aging.

It's like someone wrote such patches almost a year ago, and they were
rejected as overly complicated by some maintainer.

> So this can be added in any case. But yeah if someone can
> check whether we have the same regression on iris or not would be
> good. Can you please poke QA (or whoever has one of these boxen)?
> -Daniel

Well, forgetting regressions for just one sec, what do you think makes
the most sense? If you think it should be 3, it's probably not even
worth the effort since I barely care - and I'm happy to just change it..

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] [v3] drm/i915/hsw: Change default LLC age to 3

[Daniel Vetter]

On Mon, Aug 5, 2013 at 1:22 AM, Ben Widawsky <ben@bwidawsk.net> wrote:
> On Mon, Aug 05, 2013 at 01:03:29AM +0200, Daniel Vetter wrote:
>> Well we have a get/set_caching ioctl, explictly with room for stuff
>> like aging.
>
> It's like someone wrote such patches almost a year ago, and they were
> rejected as overly complicated by some maintainer.

Rejected as "needs to be justified with performance data and a
mesa/userspace use-case", not "rejected as too complicated, period".

>> So this can be added in any case. But yeah if someone can
>> check whether we have the same regression on iris or not would be
>> good. Can you please poke QA (or whoever has one of these boxen)?
>> -Daniel
>
> Well, forgetting regressions for just one sec, what do you think makes
> the most sense? If you think it should be 3, it's probably not even
> worth the effort since I barely care - and I'm happy to just change it..

Since the iris split happened after the regression and we've just
argued that age3 seems to be the right default (as long as mesa
doesn't do anything special) I think we should argue for age3 on iris
systems, too. Imo argueing against simply propagating the copy&paste
change needs performance data.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch