| Message ID | 1382470214-1597-4-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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. --Imre > > DRM_DEBUG_KMS("Resuming device\n"); > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 74f2b5d..73ebb9e 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1831,6 +1831,7 @@ extern bool i915_fastboot __read_mostly; > extern int i915_enable_pc8 __read_mostly; > extern int i915_pc8_timeout __read_mostly; > extern bool i915_prefault_disable __read_mostly; > +extern int i915_enable_runtime_pm __read_mostly; > > extern int i915_suspend(struct drm_device *dev, pm_message_t state); > extern int i915_resume(struct drm_device *dev); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index a973f35..617e934 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -5785,7 +5785,7 @@ void intel_runtime_pm_get(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; > > pm_runtime_mark_last_busy(device); > @@ -5798,7 +5798,7 @@ void intel_runtime_pm_put(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; > > pm_runtime_mark_last_busy(device);
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?
2013/11/4 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? 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
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); DRM_DEBUG_KMS("Resuming device\n"); diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h index 74f2b5d..73ebb9e 100644 --- a/drivers/gpu/drm/i915/i915_drv.h +++ b/drivers/gpu/drm/i915/i915_drv.h @@ -1831,6 +1831,7 @@ extern bool i915_fastboot __read_mostly; extern int i915_enable_pc8 __read_mostly; extern int i915_pc8_timeout __read_mostly; extern bool i915_prefault_disable __read_mostly; +extern int i915_enable_runtime_pm __read_mostly; extern int i915_suspend(struct drm_device *dev, pm_message_t state); extern int i915_resume(struct drm_device *dev); diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c index a973f35..617e934 100644 --- a/drivers/gpu/drm/i915/intel_pm.c +++ b/drivers/gpu/drm/i915/intel_pm.c @@ -5785,7 +5785,7 @@ void intel_runtime_pm_get(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; pm_runtime_mark_last_busy(device); @@ -5798,7 +5798,7 @@ void intel_runtime_pm_put(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; pm_runtime_mark_last_busy(device);