* [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders @ 2012-04-15 1:41 Ben Widawsky 2012-04-15 14:55 ` Eugeni Dodonov 2012-04-16 23:18 ` Kenneth Graunke 0 siblings, 2 replies; 7+ messages in thread From: Ben Widawsky @ 2012-04-15 1:41 UTC (permalink / raw) To: intel-gfx; +Cc: Ben Widawsky, Bernard Kilarski, stable, 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-04-15 1:41 [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders Ben Widawsky @ 2012-04-15 14:55 ` Eugeni Dodonov 2012-04-15 17:14 ` Ben Widawsky 2012-04-16 23:18 ` Kenneth Graunke 1 sibling, 1 reply; 7+ messages in thread From: Eugeni Dodonov @ 2012-04-15 14:55 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, Bernard Kilarski, stable, Ben Widawsky [-- 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-04-15 14:55 ` Eugeni Dodonov @ 2012-04-15 17:14 ` Ben Widawsky 2012-04-18 15:33 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Ben Widawsky @ 2012-04-15 17:14 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx, Bernard Kilarski, stable 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. ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-04-15 17:14 ` Ben Widawsky @ 2012-04-18 15:33 ` Daniel Vetter 2012-05-20 19:24 ` Daniel Vetter 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-04-18 15:33 UTC (permalink / raw) To: Ben Widawsky, Jesse Barnes; +Cc: intel-gfx, Bernard Kilarski, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-04-18 15:33 ` Daniel Vetter @ 2012-05-20 19:24 ` Daniel Vetter 2012-05-21 14:56 ` Jesse Barnes 0 siblings, 1 reply; 7+ messages in thread From: Daniel Vetter @ 2012-05-20 19:24 UTC (permalink / raw) To: Ben Widawsky, Jesse Barnes; +Cc: intel-gfx, Bernard Kilarski, stable 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-05-20 19:24 ` Daniel Vetter @ 2012-05-21 14:56 ` Jesse Barnes 0 siblings, 0 replies; 7+ messages in thread From: Jesse Barnes @ 2012-05-21 14:56 UTC (permalink / raw) To: Daniel Vetter; +Cc: Ben Widawsky, intel-gfx, stable, Bernard Kilarski 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 ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders 2012-04-15 1:41 [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders Ben Widawsky 2012-04-15 14:55 ` Eugeni Dodonov @ 2012-04-16 23:18 ` Kenneth Graunke 1 sibling, 0 replies; 7+ messages in thread From: Kenneth Graunke @ 2012-04-16 23:18 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, Bernard Kilarski, stable, Ben Widawsky 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.) ^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2012-05-21 14:56 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2012-04-15 1:41 [PATCH] drm/i915: [GEN7] Use HW scheduler for fixed function shaders Ben Widawsky 2012-04-15 14:55 ` Eugeni Dodonov 2012-04-15 17:14 ` Ben Widawsky 2012-04-18 15:33 ` Daniel Vetter 2012-05-20 19:24 ` Daniel Vetter 2012-05-21 14:56 ` Jesse Barnes 2012-04-16 23:18 ` Kenneth Graunke
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.