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

[Ben Widawsky <ben@bwidawsk.net>]

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.