Re: [RFC 1/6] drm/i915: add initial Runtime PM functions

[Imre Deak <imre.deak@intel.com>]

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 <imre.deak@intel.com>:
> > > > On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote:
> > > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > > >>
> > > >> 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 <paulo.r.zanoni@intel.com>
> > > >> ---
> > > >>  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 <linux/vga_switcheroo.h>
> > > >>  #include <linux/slab.h>
> > > >>  #include <acpi/video.h>
> > > >> +#include <linux/pm.h>
> > > >> +#include <linux/pm_runtime.h>
> > > >>
> > > >>  #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