On Sun, 15 Apr 2012 11:55:36 -0300 Eugeni Dodonov <[email protected]> wrote:
> On Sat, Apr 14, 2012 at 22:41, Ben Widawsky <[email protected]> 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 <[email protected]> > > Cc: stable <[email protected]> > > Reviewed-by (RFC): Kenneth Graunke <[email protected]> > > Signed-off-by: Ben Widawsky <[email protected]> > > > > 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 <[email protected]> > > +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. -- To unsubscribe from this list: send the line "unsubscribe stable" in the body of a message to [email protected] More majordomo info at http://vger.kernel.org/majordomo-info.html
