Re: [PATCH 05/19] drm/i915: add initial Runtime PM functions

[Takashi Iwai <tiwai@suse.de>]

At Fri, 6 Dec 2013 20:31:04 -0200,
Paulo Zanoni wrote:
> 
> 2013/11/29 Takashi Iwai <tiwai@suse.de>:
> > At Wed, 27 Nov 2013 18:10: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.
> >>
> >> V2: - Move some functions to intel_pm.c
> >>     - Remove useless pm_runtime_allow() call at init
> >>     - Remove useless pm_runtime_mark_last_busy() call at get
> >>     - Use pm_runtime_get_sync() instead of 2 calls
> >>     - Add a WARN to check if we're really awake
> >>
> >> V3: - Rebase.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/i915_dma.c     |  6 ++++
> >>  drivers/gpu/drm/i915/i915_drv.c     | 42 ++++++++++++++++++++++++++++
> >>  drivers/gpu/drm/i915/i915_drv.h     |  7 +++++
> >>  drivers/gpu/drm/i915/intel_drv.h    |  4 +++
> >>  drivers/gpu/drm/i915/intel_pm.c     | 56 +++++++++++++++++++++++++++++++++++++
> >>  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 89e4cf1..4cdc1ee 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])
> >>
> >> @@ -1663,6 +1665,8 @@ int i915_driver_load(struct drm_device *dev, unsigned long flags)
> >>       if (IS_GEN5(dev))
> >>               intel_gpu_ips_init(dev_priv);
> >>
> >> +     intel_init_runtime_pm(dev_priv);
> >> +
> >>       return 0;
> >>
> >>  out_power_well:
> >> @@ -1702,6 +1706,8 @@ int i915_driver_unload(struct drm_device *dev)
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       int ret;
> >>
> >> +     intel_fini_runtime_pm(dev_priv);
> >> +
> >>       intel_gpu_ips_teardown();
> >>
> >>       /* The i915.ko module is still not prepared to be loaded when
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> >> index 0ec0fb3..d5310a0 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.c
> >> +++ b/drivers/gpu/drm/i915/i915_drv.c
> >> @@ -502,6 +502,8 @@ static int i915_drm_freeze(struct drm_device *dev)
> >>       struct drm_i915_private *dev_priv = dev->dev_private;
> >>       struct drm_crtc *crtc;
> >>
> >> +     intel_runtime_pm_get(dev_priv);
> >> +
> >>       /* ignore lid events during suspend */
> >>       mutex_lock(&dev_priv->modeset_restore_lock);
> >>       dev_priv->modeset_restore = MODESET_SUSPENDED;
> >> @@ -688,6 +690,8 @@ static int __i915_drm_thaw(struct drm_device *dev, bool restore_gtt_mappings)
> >>       mutex_lock(&dev_priv->modeset_restore_lock);
> >>       dev_priv->modeset_restore = MODESET_DONE;
> >>       mutex_unlock(&dev_priv->modeset_restore_lock);
> >> +
> >> +     intel_runtime_pm_put(dev_priv);
> >>       return error;
> >>  }
> >>
> >> @@ -902,6 +906,42 @@ static int i915_pm_poweroff(struct device *dev)
> >>       return i915_drm_freeze(drm_dev);
> >>  }
> >>
> >> +static int i915_runtime_suspend(struct device *device)
> >> +{
> >> +     struct pci_dev *pdev = to_pci_dev(device);
> >> +     struct drm_device *dev = pci_get_drvdata(pdev);
> >> +     struct drm_i915_private *dev_priv = dev->dev_private;
> >> +
> >> +     WARN_ON(!HAS_RUNTIME_PM(dev));
> >
> > It'd be better to add runtime_idle callback for this kind of checks.
> > It's called always before actually doing runtime PM, and you can
> > return -EBUSY if the runtime PM isn't available.
> 
> It's a WARN we never expect to hit, so I don't think it's worth
> creating a new function just for a check like that.

In the current situation with a static condition, maybe so.  But in
general, it'd be safer not to do conditional check in
intel_runtime_pm_get()/put() but filter in runtime idle callback
instead, so that you can concentrate only on consistency of refcounts
in *_get() and *_put().  For example, if the condition changes
dynamically, you cannot apply conditional to *_get() and *_put();
otherwise it'd result in unbalance.  So the above is a recommendation
from my past experiences in others drivers.

And, even if you use WARN() for a static condition check there, it
doesn't have to be spit at each time.  It's no dynamic condition, so
WARN_ON_ONCE() should suffice.


Takashi