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

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

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);

--Imre