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

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

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 this doesn't cause any other problem. Again contrary to
what you'd expect reading runtime_pm.txt.

--Imre