From mboxrd@z Thu Jan 1 00:00:00 1970 From: Imre Deak Subject: Re: [RFC 1/6] drm/i915: add initial Runtime PM functions Date: Thu, 07 Nov 2013 20:13:57 +0200 Message-ID: <1383848037.2445.7.camel@ideak-mobl> References: <1382470214-1597-1-git-send-email-przanoni@gmail.com> <1382470214-1597-2-git-send-email-przanoni@gmail.com> <1382966488.5775.72.camel@intelbox> <1383817117.2374.25.camel@ideak-mobl> <1383824213.3244.17.camel@ideak-mobl> Reply-To: imre.deak@intel.com Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mga09.intel.com (mga09.intel.com [134.134.136.24]) by gabe.freedesktop.org (Postfix) with ESMTP id 29082F0923 for ; Thu, 7 Nov 2013 10:14:04 -0800 (PST) In-Reply-To: <1383824213.3244.17.camel@ideak-mobl> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Thu, 2013-11-07 at 13:36 +0200, Imre Deak wrote: > On Thu, 2013-11-07 at 11:38 +0200, Imre Deak wrote: > > On Wed, 2013-11-06 at 18:32 -0200, Paulo Zanoni wrote: > > > 2013/10/28 Imre Deak : > > > > On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > > > >> > > > >> This patch adds the initial infrastructure to allow a Runtime PM > > > >> implementation that sets the device to its D3 state. The patch just > > > >> adds the necessary callbacks and the initial infrastructure. > > > >> > > > >> We still don't have any platform that actually uses this > > > >> infrastructure, we still don't call get/put in all the places we need > > > >> to, and we don't have any function to save/restore the state of the > > > >> registers. This is not a problem since no platform uses the code added > > > >> by this patch. We have a few people simultaneously working on runtime > > > >> PM, so this initial code could help everybody make their plans. > > > >> > > > >> Signed-off-by: Paulo Zanoni > > > >> --- > > > >> drivers/gpu/drm/i915/i915_dma.c | 38 +++++++++++++++++++++++++++++++++ > > > >> drivers/gpu/drm/i915/i915_drv.c | 42 +++++++++++++++++++++++++++++++++++++ > > > >> drivers/gpu/drm/i915/i915_drv.h | 7 +++++++ > > > >> drivers/gpu/drm/i915/intel_drv.h | 2 ++ > > > >> drivers/gpu/drm/i915/intel_pm.c | 26 +++++++++++++++++++++++ > > > >> drivers/gpu/drm/i915/intel_uncore.c | 9 ++++++++ > > > >> 6 files changed, 124 insertions(+) > > > >> > > > >> diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c > > > >> index fd848ef..6aa044e 100644 > > > >> --- a/drivers/gpu/drm/i915/i915_dma.c > > > >> +++ b/drivers/gpu/drm/i915/i915_dma.c > > > >> @@ -42,6 +42,8 @@ > > > >> #include > > > >> #include > > > >> #include > > > >> +#include > > > >> +#include > > > >> > > > >> #define LP_RING(d) (&((struct drm_i915_private *)(d))->ring[RCS]) > > > >> > > > >> @@ -1449,6 +1451,38 @@ static void i915_dump_device_info(struct drm_i915_private *dev_priv) > > > >> #undef SEP_COMMA > > > >> } > > > >> > > > >> +static void i915_init_runtime_pm(struct drm_i915_private *dev_priv) > > > >> +{ > > > >> + struct drm_device *dev = dev_priv->dev; > > > >> + struct device *device = &dev->pdev->dev; > > > >> + > > > >> + dev_priv->pm.suspended = false; > > > >> + > > > >> + if (!HAS_RUNTIME_PM(dev)) > > > >> + return; > > > >> + > > > >> + pm_runtime_set_active(device); > > > >> + pm_runtime_enable(device); > > > >> + > > > >> + pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */ > > > >> + pm_runtime_mark_last_busy(device); > > > >> + pm_runtime_use_autosuspend(device); > > > >> + pm_runtime_allow(device); > > > > > > > > This shouldn't be needed as we get here already with an allowed state. > > > > It's not a problem as it's just a nop here, but imo it's confusing that > > > > we don't have the corresponding pm_runtime_forbid() in > > > > i915_fini_runtime_pm(). > > > > > > If we don't call this, when we boot the machine the "power/control" > > > sysfs file will be "on", which means runtime PM is disabled. We have > > > to manually "echo auto > control" to enable runtime PM then. But I > > > guess leaving runtime PM disabled by default might be what we want, so > > > I'll remove the call here. > > > > Right, I haven't noticed that pci_pm_init() does an explicit > > pm_runtime_forbid(). Documentation/runtime_pm.txt says that drivers > > should call pm_runtime_forbid() explicitly if they want to disable user > > control. Imo the PCI subsystem doing this in the background is somewhat > > deceiving for driver authors. > > > > I noticed only now by looking at pci_pm_init() that the same goes for > > pm_runtime_set_active(), pm_runtime_enable() above. Since these are > > already called for you, atm you'll get an "unbalanced pm_runtime_enable" > > message, though that doesn't cause any other problem. Again contrary to > > what you'd expect reading runtime_pm.txt. > > Ok, Documentation/power/pci.txt explains the semantics on calling > pm_runtime_allow/forbid() for PCI devices, but still states incorrectly > that you need to call pm_runtime_enable(). > > So based on all these I think the correct init order is if you want to > leave auto suspend disabled: > > pm_runtime_set_autosuspend_delay(device, 10000); /* 10s */ > pm_runtime_mark_last_busy(device); > pm_runtime_use_autosuspend(device); > pm_runtime_put(device); Err, pm_runtime_put() is not needed since we need to keep a reference for pc8 (at least for HSW). Sorry for the noise. --Imre