Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling functions

[Daniel Vetter <daniel@ffwll.ch>]

On Fri, Jun 29, 2012 at 11:51:35AM -0700, Ben Widawsky wrote:
> On Sun, 24 Jun 2012 16:42:32 +0200
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > +void intel_disable_gt_powersave(struct drm_device *dev)
> > +{
> > +	if (IS_IRONLAKE_M(dev))
> > +		ironlake_disable_drps(dev);
> > +	if (INTEL_INFO(dev)->gen >= 6 && !IS_VALLEYVIEW(dev))
> > +		gen6_disable_rps(dev);
> > +}
> 
> What happened to ironlake_disable_rc6?

Lost in refactoring. I've noticed this while playing around with a few
things on my ilk myself, but somehow forgot to send out the fixup. Shame
on me :(
> 
> > +
> > +void intel_enable_gt_powersave(struct drm_device *dev)
> > +{
> > +	struct drm_i915_private *dev_priv = dev->dev_private;
> > +
> > +	if (IS_IRONLAKE_M(dev)) {
> > +		ironlake_enable_drps(dev);
> > +		ironlake_enable_rc6(dev);
> > +		intel_init_emon(dev);
> > +	}
> > +
> > +	if ((IS_GEN6(dev) || IS_GEN7(dev)) && !IS_VALLEYVIEW(dev)) {
> > +		gen6_enable_rps(dev_priv);
> > +		gen6_update_ring_freq(dev_priv);
> > +	}
> > +}
> > +
> >  static void ironlake_init_clock_gating(struct drm_device *dev)
> >  {
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> 
> 
> It'd be nice if there was some explanation of why disable doesn't do the
> reverse of enable.

Well, the enable_rc6 and init_emon are a bit at the wrong place and might
need to be split-up and moved around (i.e. hw frobbing should be here,
object alloc and other such setup stuff moved out). It's not the only
place where we suck in this way, though ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48