[PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Ben Widawsky]

This originally started as a patch from Bernard as a way of simply
setting the VS scheduler. After submitting the RFC patch, we decided to
also modify the DS scheduler. To be most explicit, I've made the patch
explicitly set all scheduler modes, and included the defines for other
modes (in case someone feels frisky later).

The rest of the story gets a bit weird. The first version of the patch
showed an almost unbelievable performance improvement. Since rebasing my
branch it appears the performance improvement has gone, unfortunately.
But setting these bits seem to be the right thing to do given that the
docs describe corruption that can occur with the default settings.

In summary, I am seeing no more perf improvements (or regressions) in my
limited testing, but we believe this should be set to prevent rendering
corruption, therefore cc stable.

v1: Clear bit 4 also (Ken + Eugeni)
Do a full clear + set of the bits we want (Me).

Cc: Bernard Kilarski <bernard.r.kilarski@intel.com>
Cc: stable <stable@vger.kernel.org>
Reviewed-by (RFC): Kenneth Graunke <kenneth@whitecape.org>
Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
---
 drivers/gpu/drm/i915/i915_reg.h      |   15 +++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   14 ++++++++++++++
 2 files changed, 29 insertions(+)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 972321f..59a85c8 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -682,6 +682,21 @@
 
 #define GEN6_BSD_RNCID			0x12198
 
+#define GEN7_FF_THREAD_MODE		0x20a0
+#define   GEN7_FF_SCHED_MASK		0x0077070
+#define   GEN7_FF_TS_SCHED_HS1		(0x5<<16)
+#define   GEN7_FF_TS_SCHED_HS0		(0x3<<16)
+#define   GEN7_FF_TS_SCHED_LOAD_BALANCE	(0x1<<16)
+#define   GEN7_FF_TS_SCHED_HW		(0x0<<16) /* Default */
+#define   GEN7_FF_VS_SCHED_HS1		(0x5<<12)
+#define   GEN7_FF_VS_SCHED_HS0		(0x3<<12)
+#define   GEN7_FF_VS_SCHED_LOAD_BALANCE	(0x1<<12) /* Default */
+#define   GEN7_FF_VS_SCHED_HW		(0x0<<12)
+#define   GEN7_FF_DS_SCHED_HS1		(0x5<<4)
+#define   GEN7_FF_DS_SCHED_HS0		(0x3<<4)
+#define   GEN7_FF_DS_SCHED_LOAD_BALANCE	(0x1<<4)  /* Default */
+#define   GEN7_FF_DS_SCHED_HW		(0x0<<4)
+
 /*
  * Framebuffer compression (915+ only)
  */
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 33aaad3..4508935 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8894,6 +8894,18 @@ static void gen6_init_clock_gating(struct drm_device *dev)
 	}
 }
 
+static void gen7_setup_fixed_func_scheduler(struct drm_i915_private *dev_priv)
+{
+	uint32_t reg = I915_READ(GEN7_FF_THREAD_MODE);
+
+	reg &= ~GEN7_FF_SCHED_MASK;
+	reg |= GEN7_FF_TS_SCHED_HW;
+	reg |= GEN7_FF_VS_SCHED_HW;
+	reg |= GEN7_FF_DS_SCHED_HW;
+
+	I915_WRITE(GEN7_FF_THREAD_MODE, reg);
+}
+
 static void ivybridge_init_clock_gating(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -8938,6 +8950,8 @@ static void ivybridge_init_clock_gating(struct drm_device *dev)
 			   DISPPLANE_TRICKLE_FEED_DISABLE);
 		intel_flush_display_plane(dev_priv, pipe);
 	}
+
+	gen7_setup_fixed_func_scheduler(dev_priv);
 }
 
 static void valleyview_init_clock_gating(struct drm_device *dev)
-- 
1.7.10

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 1989 bytes --]

On Sat, Apr 14, 2012 at 22:41, Ben Widawsky <ben@bwidawsk.net> wrote:

> This originally started as a patch from Bernard as a way of simply
> setting the VS scheduler. After submitting the RFC patch, we decided to
> also modify the DS scheduler. To be most explicit, I've made the patch
> explicitly set all scheduler modes, and included the defines for other
> modes (in case someone feels frisky later).
>
> The rest of the story gets a bit weird. The first version of the patch
> showed an almost unbelievable performance improvement. Since rebasing my
> branch it appears the performance improvement has gone, unfortunately.
> But setting these bits seem to be the right thing to do given that the
> docs describe corruption that can occur with the default settings.
>
> In summary, I am seeing no more perf improvements (or regressions) in my
> limited testing, but we believe this should be set to prevent rendering
> corruption, therefore cc stable.
>
> v1: Clear bit 4 also (Ken + Eugeni)
> Do a full clear + set of the bits we want (Me).
>
> Cc: Bernard Kilarski <bernard.r.kilarski@intel.com>
> Cc: stable <stable@vger.kernel.org>
> Reviewed-by (RFC): Kenneth Graunke <kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
>

Very nice!

I also suspect that maybe the initial performance improvement you've seen
with previous testing could be related to the occasional turbo disabling
we've been seeing in other cases as well (e.g.,
https://bugs.freedesktop.org/show_bug.cgi?id=44006).

But as for this patch, I have just one comment/suggestion below, but other
than that:

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

+static void gen7_setup_fixed_func_scheduler(struct drm_i915_private
> *dev_priv)
>

Perhaps this functions should be named ivybridge_setup_fixed_func_scheduler
instead?

Even if those bits are not ivy bridge-exclusive, this specific explicit
setup applies to ivb only..

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 3080 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Ben Widawsky]

On Sun, 15 Apr 2012 11:55:36 -0300
Eugeni Dodonov <eugeni@dodonov.net> wrote:

> On Sat, Apr 14, 2012 at 22:41, Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> > This originally started as a patch from Bernard as a way of simply
> > setting the VS scheduler. After submitting the RFC patch, we
> > decided to also modify the DS scheduler. To be most explicit, I've
> > made the patch explicitly set all scheduler modes, and included the
> > defines for other modes (in case someone feels frisky later).
> >
> > The rest of the story gets a bit weird. The first version of the
> > patch showed an almost unbelievable performance improvement. Since
> > rebasing my branch it appears the performance improvement has gone,
> > unfortunately. But setting these bits seem to be the right thing to
> > do given that the docs describe corruption that can occur with the
> > default settings.
> >
> > In summary, I am seeing no more perf improvements (or regressions)
> > in my limited testing, but we believe this should be set to prevent
> > rendering corruption, therefore cc stable.
> >
> > v1: Clear bit 4 also (Ken + Eugeni)
> > Do a full clear + set of the bits we want (Me).
> >
> > Cc: Bernard Kilarski <bernard.r.kilarski@intel.com>
> > Cc: stable <stable@vger.kernel.org>
> > Reviewed-by (RFC): Kenneth Graunke <kenneth@whitecape.org>
> > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> >
> 
> Very nice!
> 
> I also suspect that maybe the initial performance improvement you've
> seen with previous testing could be related to the occasional turbo
> disabling we've been seeing in other cases as well (e.g.,
> https://bugs.freedesktop.org/show_bug.cgi?id=44006).
> 
> But as for this patch, I have just one comment/suggestion below, but
> other than that:
> 
> Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> 
> +static void gen7_setup_fixed_func_scheduler(struct drm_i915_private
> > *dev_priv)
> >
> 
> Perhaps this functions should be named
> ivybridge_setup_fixed_func_scheduler instead?
> 
> Even if those bits are not ivy bridge-exclusive, this specific
> explicit setup applies to ivb only..
> 

I wasn't sure if we wanted this for VLV or not. In fact, originally the
patch did call this in the VLV setup, but since I decided to CC stable
(per Ken's idea) I removed the VLV part.

If Jesse, or someone could confirm we don't want this for VLV, I agree
with your commen, and I'd probably just go back and inline the register
write. FWIW, it does *seem* like we don't want to set this on HSW.

Anyway, thanks for your review.

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Kenneth Graunke]

On 04/14/2012 06:41 PM, Ben Widawsky wrote:
> This originally started as a patch from Bernard as a way of simply
> setting the VS scheduler. After submitting the RFC patch, we decided to
> also modify the DS scheduler. To be most explicit, I've made the patch
> explicitly set all scheduler modes, and included the defines for other
> modes (in case someone feels frisky later).
>
> The rest of the story gets a bit weird. The first version of the patch
> showed an almost unbelievable performance improvement. Since rebasing my
> branch it appears the performance improvement has gone, unfortunately.
> But setting these bits seem to be the right thing to do given that the
> docs describe corruption that can occur with the default settings.
>
> In summary, I am seeing no more perf improvements (or regressions) in my
> limited testing, but we believe this should be set to prevent rendering
> corruption, therefore cc stable.
>
> v1: Clear bit 4 also (Ken + Eugeni)
> Do a full clear + set of the bits we want (Me).
>
> Cc: Bernard Kilarski<bernard.r.kilarski@intel.com>
> Cc: stable<stable@vger.kernel.org>
> Reviewed-by (RFC): Kenneth Graunke<kenneth@whitecape.org>
> Signed-off-by: Ben Widawsky<benjamin.widawsky@intel.com>

Looks good!  I like how you've reworked this.

It looks like we don't want to do this on Haswell.

Reviewed-by: Kenneth Graunke <kenneth@whitecape.org>

(I haven't tested it, though.)

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Daniel Vetter]

Jesse, can you also please check whether we need the same thing on vlv?
atm the the clock gating functions are almost identical safe for this wrt
touching registers in the gt core.
-Daniel

On Sun, Apr 15, 2012 at 10:14:24AM -0700, Ben Widawsky wrote:
> On Sun, 15 Apr 2012 11:55:36 -0300
> Eugeni Dodonov <eugeni@dodonov.net> wrote:
> 
> > On Sat, Apr 14, 2012 at 22:41, Ben Widawsky <ben@bwidawsk.net> wrote:
> > 
> > > This originally started as a patch from Bernard as a way of simply
> > > setting the VS scheduler. After submitting the RFC patch, we
> > > decided to also modify the DS scheduler. To be most explicit, I've
> > > made the patch explicitly set all scheduler modes, and included the
> > > defines for other modes (in case someone feels frisky later).
> > >
> > > The rest of the story gets a bit weird. The first version of the
> > > patch showed an almost unbelievable performance improvement. Since
> > > rebasing my branch it appears the performance improvement has gone,
> > > unfortunately. But setting these bits seem to be the right thing to
> > > do given that the docs describe corruption that can occur with the
> > > default settings.
> > >
> > > In summary, I am seeing no more perf improvements (or regressions)
> > > in my limited testing, but we believe this should be set to prevent
> > > rendering corruption, therefore cc stable.
> > >
> > > v1: Clear bit 4 also (Ken + Eugeni)
> > > Do a full clear + set of the bits we want (Me).
> > >
> > > Cc: Bernard Kilarski <bernard.r.kilarski@intel.com>
> > > Cc: stable <stable@vger.kernel.org>
> > > Reviewed-by (RFC): Kenneth Graunke <kenneth@whitecape.org>
> > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
> > >
> > 
> > Very nice!
> > 
> > I also suspect that maybe the initial performance improvement you've
> > seen with previous testing could be related to the occasional turbo
> > disabling we've been seeing in other cases as well (e.g.,
> > https://bugs.freedesktop.org/show_bug.cgi?id=44006).
> > 
> > But as for this patch, I have just one comment/suggestion below, but
> > other than that:
> > 
> > Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > 
> > +static void gen7_setup_fixed_func_scheduler(struct drm_i915_private
> > > *dev_priv)
> > >
> > 
> > Perhaps this functions should be named
> > ivybridge_setup_fixed_func_scheduler instead?
> > 
> > Even if those bits are not ivy bridge-exclusive, this specific
> > explicit setup applies to ivb only..
> > 
> 
> I wasn't sure if we wanted this for VLV or not. In fact, originally the
> patch did call this in the VLV setup, but since I decided to CC stable
> (per Ken's idea) I removed the VLV part.
> 
> If Jesse, or someone could confirm we don't want this for VLV, I agree
> with your commen, and I'd probably just go back and inline the register
> write. FWIW, it does *seem* like we don't want to set this on HSW.
> 
> Anyway, thanks for your review.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Daniel Vetter]

On Wed, Apr 18, 2012 at 5:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> Jesse, can you also please check whether we need the same thing on vlv?
> atm the the clock gating functions are almost identical safe for this wrt
> touching registers in the gt core.

Prod.

Now that you have a vlv, no more excuses ;-)
-Daniel

> -Daniel
>
> On Sun, Apr 15, 2012 at 10:14:24AM -0700, Ben Widawsky wrote:
>> On Sun, 15 Apr 2012 11:55:36 -0300
>> Eugeni Dodonov <eugeni@dodonov.net> wrote:
>>
>> > On Sat, Apr 14, 2012 at 22:41, Ben Widawsky <ben@bwidawsk.net> wrote:
>> >
>> > > This originally started as a patch from Bernard as a way of simply
>> > > setting the VS scheduler. After submitting the RFC patch, we
>> > > decided to also modify the DS scheduler. To be most explicit, I've
>> > > made the patch explicitly set all scheduler modes, and included the
>> > > defines for other modes (in case someone feels frisky later).
>> > >
>> > > The rest of the story gets a bit weird. The first version of the
>> > > patch showed an almost unbelievable performance improvement. Since
>> > > rebasing my branch it appears the performance improvement has gone,
>> > > unfortunately. But setting these bits seem to be the right thing to
>> > > do given that the docs describe corruption that can occur with the
>> > > default settings.
>> > >
>> > > In summary, I am seeing no more perf improvements (or regressions)
>> > > in my limited testing, but we believe this should be set to prevent
>> > > rendering corruption, therefore cc stable.
>> > >
>> > > v1: Clear bit 4 also (Ken + Eugeni)
>> > > Do a full clear + set of the bits we want (Me).
>> > >
>> > > Cc: Bernard Kilarski <bernard.r.kilarski@intel.com>
>> > > Cc: stable <stable@vger.kernel.org>
>> > > Reviewed-by (RFC): Kenneth Graunke <kenneth@whitecape.org>
>> > > Signed-off-by: Ben Widawsky <benjamin.widawsky@intel.com>
>> > >
>> >
>> > Very nice!
>> >
>> > I also suspect that maybe the initial performance improvement you've
>> > seen with previous testing could be related to the occasional turbo
>> > disabling we've been seeing in other cases as well (e.g.,
>> > https://bugs.freedesktop.org/show_bug.cgi?id=44006).
>> >
>> > But as for this patch, I have just one comment/suggestion below, but
>> > other than that:
>> >
>> > Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
>> >
>> > +static void gen7_setup_fixed_func_scheduler(struct drm_i915_private
>> > > *dev_priv)
>> > >
>> >
>> > Perhaps this functions should be named
>> > ivybridge_setup_fixed_func_scheduler instead?
>> >
>> > Even if those bits are not ivy bridge-exclusive, this specific
>> > explicit setup applies to ivb only..
>> >
>>
>> I wasn't sure if we wanted this for VLV or not. In fact, originally the
>> patch did call this in the VLV setup, but since I decided to CC stable
>> (per Ken's idea) I removed the VLV part.
>>
>> If Jesse, or someone could confirm we don't want this for VLV, I agree
>> with your commen, and I'd probably just go back and inline the register
>> write. FWIW, it does *seem* like we don't want to set this on HSW.
>>
>> Anyway, thanks for your review.
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Mail: daniel@ffwll.ch
> Mobile: +41 (0)79 365 57 48



-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders

[Jesse Barnes]

On Sun, 20 May 2012 21:24:15 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Apr 18, 2012 at 5:33 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> > Jesse, can you also please check whether we need the same thing on vlv?
> > atm the the clock gating functions are almost identical safe for this wrt
> > touching registers in the gt core.
> 
> Prod.
> 
> Now that you have a vlv, no more excuses ;-)

I've got one more: I'm waiting on a dediprog so I can upgrade the
BIOS.  The machine is stable unless I try to use gfx at the moment
because a BIOS change prevented the GPU from coming out of reset.

I was able to do some work on the VGA console side of things (adding a
couple of needed workarounds); hopefully I'll be able to refresh this
patchset this week or next and do the sprite stuff too.

Bernard's team may have one for testing by now too though; so they can
provide an answer on this thread.

-- 
Jesse Barnes, Intel Open Source Technology Center