From mboxrd@z Thu Jan  1 00:00:00 1970
From: Daniel Vetter <daniel@ffwll.ch>
Subject: Re: [PATCH 1/5] drm/i915: wrap up gt powersave enabling
 functions
Date: Fri, 29 Jun 2012 23:11:31 +0200
Message-ID: <20120629211131.GD5463@phenom.ffwll.local>
References: <1340548956-4097-1-git-send-email-daniel.vetter@ffwll.ch>
	<1340548956-4097-2-git-send-email-daniel.vetter@ffwll.ch>
	<20120629115135.3566d752@bwidawsk.net>
Mime-Version: 1.0
Content-Type: text/plain; charset="us-ascii"
Content-Transfer-Encoding: 7bit
Return-path: <intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org>
Received: from mail-ee0-f49.google.com (mail-ee0-f49.google.com [74.125.83.49])
	by gabe.freedesktop.org (Postfix) with ESMTP id 386279F748
	for <intel-gfx@lists.freedesktop.org>;
	Fri, 29 Jun 2012 14:11:35 -0700 (PDT)
Received: by eekd17 with SMTP id d17so1592394eek.36
	for <intel-gfx@lists.freedesktop.org>;
	Fri, 29 Jun 2012 14:11:34 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <20120629115135.3566d752@bwidawsk.net>
List-Unsubscribe: <http://lists.freedesktop.org/mailman/options/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=unsubscribe>
List-Archive: <http://lists.freedesktop.org/archives/intel-gfx>
List-Post: <mailto:intel-gfx@lists.freedesktop.org>
List-Help: <mailto:intel-gfx-request@lists.freedesktop.org?subject=help>
List-Subscribe: <http://lists.freedesktop.org/mailman/listinfo/intel-gfx>,
	<mailto:intel-gfx-request@lists.freedesktop.org?subject=subscribe>
Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org
To: Ben Widawsky <ben@bwidawsk.net>
Cc: Daniel Vetter <daniel.vetter@ffwll.ch>, Intel Graphics Development <intel-gfx@lists.freedesktop.org>
List-Id: intel-gfx@lists.freedesktop.org

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