[Intel-gfx] [RFC 3/6] drm/i915: add enable_runtime_pm option

[Paulo Zanoni]

2013/11/4 Ville Syrjälä <ville.syrjala at linux.intel.com>:
> On Mon, Oct 28, 2013 at 03:27:39PM +0200, Imre Deak wrote:
>> On Tue, 2013-10-22 at 17:30 -0200, Paulo Zanoni wrote:
>> > From: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> >
>> > And leave it off by default. We have way too many driver entry points,
>> > we can't assume this will work without regressions without tons of
>> > testing first. This option allows people to test and fix the problems.
>> >
>> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni at intel.com>
>> > ---
>> >  drivers/gpu/drm/i915/i915_dma.c | 4 ++--
>> >  drivers/gpu/drm/i915/i915_drv.c | 8 ++++++--
>> >  drivers/gpu/drm/i915/i915_drv.h | 1 +
>> >  drivers/gpu/drm/i915/intel_pm.c | 4 ++--
>> >  4 files changed, 11 insertions(+), 6 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_dma.c b/drivers/gpu/drm/i915/i915_dma.c
>> > index 6aa044e..dd4f424 100644
>> > --- a/drivers/gpu/drm/i915/i915_dma.c
>> > +++ b/drivers/gpu/drm/i915/i915_dma.c
>> > @@ -1458,7 +1458,7 @@ static void i915_init_runtime_pm(struct drm_i915_private *dev_priv)
>> >
>> >     dev_priv->pm.suspended = false;
>> >
>> > -   if (!HAS_RUNTIME_PM(dev))
>> > +   if (!HAS_RUNTIME_PM(dev) || !i915_enable_runtime_pm)
>> >             return;
>> >
>> >     pm_runtime_set_active(device);
>> > @@ -1475,7 +1475,7 @@ static void i915_fini_runtime_pm(struct drm_i915_private *dev_priv)
>> >     struct drm_device *dev = dev_priv->dev;
>> >     struct device *device = &dev->pdev->dev;
>> >
>> > -   if (!HAS_RUNTIME_PM(dev))
>> > +   if (!HAS_RUNTIME_PM(dev) || !i915_enable_runtime_pm)
>> >             return;
>> >
>> >     /* Make sure we're not suspended first. */
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> > index a999a3f..c75b78f 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -154,6 +154,10 @@ module_param_named(prefault_disable, i915_prefault_disable, bool, 0600);
>> >  MODULE_PARM_DESC(prefault_disable,
>> >             "Disable page prefaulting for pread/pwrite/reloc (default:false). For developers only.");
>> >
>> > +int i915_enable_runtime_pm __read_mostly = 0;
>> > +module_param_named(enable_runtime_pm, i915_enable_runtime_pm, int, 0600);
>> > +MODULE_PARM_DESC(enable_runtime_pm, "Enable runtime PM on supported platforms (default: disabled)");
>> > +
>> >  static struct drm_driver driver;
>> >  extern int intel_agp_enabled;
>> >
>> > @@ -890,7 +894,7 @@ static int i915_runtime_suspend(struct device *device)
>> >     struct drm_device *dev = pci_get_drvdata(pdev);
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> >
>> > -   WARN_ON(!HAS_RUNTIME_PM(dev));
>> > +   WARN_ON(!HAS_RUNTIME_PM(dev) || !i915_enable_runtime_pm);
>> >
>> >     DRM_DEBUG_KMS("Suspending device\n");
>> >
>> > @@ -909,7 +913,7 @@ static int i915_runtime_resume(struct device *device)
>> >     struct drm_device *dev = pci_get_drvdata(pdev);
>> >     struct drm_i915_private *dev_priv = dev->dev_private;
>> >
>> > -   WARN_ON(!HAS_RUNTIME_PM(dev));
>> > +   WARN_ON(!HAS_RUNTIME_PM(dev) || !i915_enable_runtime_pm);
>>
>> This could a problem if someone disables runtime_pm through sysfs while
>> the device is suspended. One solution would be just to do a get/put in
>> the handler of i915_enable_runtime_pm and not check for it afterwards.
>
> There's already a standard interface for enabling/disabling runtime PM
> under sysfs. Why do we want this custom one?

Mostly to avoid those people that run powertop and switch everything
to "good". But I don't really have a strong opinion here, I'm fine
with just discarding this patch.

>
> --
> Ville Syrjälä
> Intel OTC



-- 
Paulo Zanoni