Re: [RFC 3/6] drm/i915: add enable_runtime_pm option

["Ville Syrjälä" <ville.syrjala@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@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@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?

-- 
Ville Syrjälä
Intel OTC