From mboxrd@z Thu Jan  1 00:00:00 1970
From: Chris Wilson <chris@chris-wilson.co.uk>
Subject: Re: [PATCH 3/8] drm/i915: allow package C8+ states on
 Haswell (disabled)
Date: Mon, 29 Jul 2013 22:42:05 +0100
Message-ID: <20130729214205.GA17854@cantiga.alporthouse.com>
References: <1375130907-4054-1-git-send-email-przanoni@gmail.com>
	<1375130907-4054-4-git-send-email-przanoni@gmail.com>
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 fireflyinternet.com (s16502780.onlinehome-server.info
	[87.106.93.118])
	by gabe.freedesktop.org (Postfix) with ESMTP id D5B57E5F1F
	for <intel-gfx@lists.freedesktop.org>;
	Mon, 29 Jul 2013 14:42:27 -0700 (PDT)
Content-Disposition: inline
In-Reply-To: <1375130907-4054-4-git-send-email-przanoni@gmail.com>
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: Paulo Zanoni <przanoni@gmail.com>
Cc: intel-gfx@lists.freedesktop.org, Paulo Zanoni <paulo.r.zanoni@intel.com>
List-Id: intel-gfx@lists.freedesktop.org

On Mon, Jul 29, 2013 at 05:48:22PM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This patch allows PC8+ states on Haswell. These states can only be
> reached when all the display outputs are disabled, and they allow some
> more power savings.
> 
> The fact that the graphics device is allowing PC8+ doesn't mean that
> the machine will actually enter PC8+: all the other devices also need
> to allow PC8+.
> 
> For now this option is disabled by default. You need i915.allow_pc8=1
> if you want it.

Still dislike the names. hsw_pc8 is good, so use it consistently.

> +/* Disallows PC8 so we can use the GMBUS and DP AUX interrupts. */
> +void intel_aux_display_runtime_get(struct drm_i915_private *dev_priv)
> +{
> +	mutex_lock(&dev_priv->pc8.lock);
> +	hsw_disallow_package_c8(dev_priv->dev);
> +	mutex_unlock(&dev_priv->pc8.lock);
> +}
> +
> +void intel_aux_display_runtime_put(struct drm_i915_private *dev_priv)
> +{
> +	mutex_lock(&dev_priv->pc8.lock);
> +	hsw_allow_package_c8(dev_priv->dev);
> +	mutex_unlock(&dev_priv->pc8.lock);
> +}

Move the locking from into hsw_(allow|disallow)_package_c8, and rename
the current hsw_alloc_package_c8 i.e

  void hsw_allow_package_c8(struct drm_i915_private *dev_priv)
  {
	mutex_lock(&dev_priv->pc8.lock);
	__hsw_allow_package_c8(dev_priv);
	mutex_unlock(&dev_priv->pc8.lock);
  }

then allow the lock manipulation is close together and the doesn't leak
across layers. (The locking is definitely not clear atm.) Whilst you are
at is, I do not see any reason not to call them hsw_pc8_enable() and
hsw_pc8_disable(), and hsw_set_package_c8() becomes hsw_pc8_update().

Just call forbid_refcnt, forbid_count (though I'm still liking
wake_count). And replace allowing with display_power_well_active,
verbosity is good here. s/i915_allow_pc8/i915_enable_pc8/ for
consistency.
 
So other than those minor issues, being freaked out by the locking and
the WARNs, I want at least a paragraph explaining why that is even
remotely safe, the logic at least looks sane.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre