| Message ID | 20180810071138.30138-1-chris@chris-wilson.co.uk (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RFC,1/8] drm/i915: Introduce intel_runtime_pm_disable to pair intel_runtime_pm_enable | expand |
On Fri, Aug 10, 2018 at 08:11:31AM +0100, Chris Wilson wrote: > Currently, we cancel the extra wakeref we have for !runtime-pm devices > inside power_wells_fini_hw. However, this is not strictly paired with > the acquisition of that wakeref in runtime_pm_enable (as the fini_hw may > be called on errors paths before we even call runtime_pm_enable). Make > the symmetry more explicit and include a check that we do release all of > our rpm wakerefs. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- > drivers/gpu/drm/i915/intel_drv.h | 1 + > drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++++++++++++--------- > 3 files changed, 22 insertions(+), 11 deletions(-) > > diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c > index 9dce55182c3a..62ef105a241d 100644 > --- a/drivers/gpu/drm/i915/i915_drv.c > +++ b/drivers/gpu/drm/i915/i915_drv.c > @@ -1281,6 +1281,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > */ > if (INTEL_INFO(dev_priv)->num_pipes) > drm_kms_helper_poll_init(dev); > + > + intel_runtime_pm_enable(dev_priv); > } > > /** > @@ -1289,6 +1291,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) > */ > static void i915_driver_unregister(struct drm_i915_private *dev_priv) > { > + intel_runtime_pm_disable(dev_priv); > + > intel_fbdev_unregister(dev_priv); > intel_audio_deinit(dev_priv); > > @@ -1408,8 +1412,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) > > i915_driver_register(dev_priv); > > - intel_runtime_pm_enable(dev_priv); > - > intel_init_ipc(dev_priv); > > intel_runtime_pm_put(dev_priv); > @@ -1474,6 +1476,8 @@ void i915_driver_unload(struct drm_device *dev) > i915_driver_cleanup_mmio(dev_priv); > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > + > + WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count)); This probably WARNs atm at least due to having a low-level pm_runtime_put_autosuspend() in intel_runtime_pm_enable(); but a corresponding intel_runtime_pm_get() in intel_power_domains_fini_hw() (via intel_display_set_init_power) which is i915 specific. The following would fix that: diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index e209edbc561d..1623c0d2cf57 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -3806,6 +3806,10 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) */ intel_display_set_init_power(dev_priv, true); + /* Hand over RPM reference to PM core */ + pm_runtime_get_noresume(kdev); + intel_runtime_pm_put(dev_priv); + /* Remove the refcount we took to keep power well support disabled. */ if (!i915_modparams.disable_power_well) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > } > > static void i915_driver_release(struct drm_device *dev) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0601abb8c71f..dc6c0cec9b36 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1956,6 +1956,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv); > void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume); > void bxt_display_core_uninit(struct drm_i915_private *dev_priv); > void intel_runtime_pm_enable(struct drm_i915_private *dev_priv); > +void intel_runtime_pm_disable(struct drm_i915_private *dev_priv); > const char * > intel_display_power_domain_str(enum intel_display_power_domain domain); > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index e209edbc561d..b78c3b48aa62 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -3793,8 +3793,6 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume) > */ > void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) > { > - struct device *kdev = &dev_priv->drm.pdev->dev; > - > /* > * The i915.ko module is still not prepared to be loaded when > * the power well is not enabled, so just enable it in case > @@ -3809,13 +3807,6 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) > /* Remove the refcount we took to keep power well support disabled. */ > if (!i915_modparams.disable_power_well) > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > - > - /* > - * Remove the refcount we took in intel_runtime_pm_enable() in case > - * the platform doesn't support runtime PM. > - */ > - if (!HAS_RUNTIME_PM(dev_priv)) > - pm_runtime_put(kdev); > } > > /** > @@ -4074,3 +4065,18 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv) > */ > pm_runtime_put_autosuspend(kdev); > } > + > +void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) > +{ > + struct pci_dev *pdev = dev_priv->drm.pdev; > + struct device *kdev = &pdev->dev; > + > + pm_runtime_dont_use_autosuspend(kdev); > + > + /* > + * Remove the refcount we took in intel_runtime_pm_enable() in case > + * the platform doesn't support runtime PM. > + */ > + if (!HAS_RUNTIME_PM(dev_priv)) > + pm_runtime_put(kdev); > +} > -- > 2.18.0 >
Quoting Imre Deak (2018-08-10 13:22:43) > On Fri, Aug 10, 2018 at 08:11:31AM +0100, Chris Wilson wrote: > > @@ -1474,6 +1476,8 @@ void i915_driver_unload(struct drm_device *dev) > > i915_driver_cleanup_mmio(dev_priv); > > > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > > + > > + WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count)); > > This probably WARNs atm at least due to having a low-level > pm_runtime_put_autosuspend() in intel_runtime_pm_enable(); but a > corresponding intel_runtime_pm_get() in intel_power_domains_fini_hw() (via > intel_display_set_init_power) which is i915 specific. The following > would fix that: It gets caught out by the very last display_set_init_power indeed. I mean to have a word with you about that function ;) The issue we have with intel_display_set_init_power() at the moment is that we don't always clean up it correctly as it not obvious who is responsible for it at any time. Would it be possible to make that into local POWER_DOMAIN_INIT grabs so the onion is always clear? (It wasn't obvious to me why ownership was being transferred fairly arbitrary.) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index e209edbc561d..1623c0d2cf57 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -3806,6 +3806,10 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) > */ > intel_display_set_init_power(dev_priv, true); In this case this would be much clearer as a raw intel_power_domain_get(POWER_DOMAIN_INIT) I think. -Chris
On Fri, Aug 10, 2018 at 01:33:19PM +0100, Chris Wilson wrote: > Quoting Imre Deak (2018-08-10 13:22:43) > > On Fri, Aug 10, 2018 at 08:11:31AM +0100, Chris Wilson wrote: > > > @@ -1474,6 +1476,8 @@ void i915_driver_unload(struct drm_device *dev) > > > i915_driver_cleanup_mmio(dev_priv); > > > > > > intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); > > > + > > > + WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count)); > > > > This probably WARNs atm at least due to having a low-level > > pm_runtime_put_autosuspend() in intel_runtime_pm_enable(); but a > > corresponding intel_runtime_pm_get() in intel_power_domains_fini_hw() (via > > intel_display_set_init_power) which is i915 specific. The following > > would fix that: > > It gets caught out by the very last display_set_init_power indeed. I > mean to have a word with you about that function ;) > > The issue we have with intel_display_set_init_power() at the moment is > that we don't always clean up it correctly as it not obvious who is > responsible for it at any time. Would it be possible to make that into > local POWER_DOMAIN_INIT grabs so the onion is always clear? (It wasn't > obvious to me why ownership was being transferred fairly arbitrary.) Yes, would be much clearer. One thing that depends on it is driver loading / resume expecting any power wells enabled by BIOS to stay enabled until the end of display HW readout. I can take a look at the exact dependencies there and remove the use of intel_display_set_init_power(). > > > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > > index e209edbc561d..1623c0d2cf57 100644 > > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > > @@ -3806,6 +3806,10 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) > > */ > > intel_display_set_init_power(dev_priv, true); > > In this case this would be much clearer as a raw > intel_power_domain_get(POWER_DOMAIN_INIT) > I think. Yes, but would have to change it in sync with the intel_display_set_init_power() in intel_power_domains_init_hw(). An error somewhere after that and before the end of HW readout would make the above intel_display_set_init_power() a nop. --Imre
diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c index 9dce55182c3a..62ef105a241d 100644 --- a/drivers/gpu/drm/i915/i915_drv.c +++ b/drivers/gpu/drm/i915/i915_drv.c @@ -1281,6 +1281,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) */ if (INTEL_INFO(dev_priv)->num_pipes) drm_kms_helper_poll_init(dev); + + intel_runtime_pm_enable(dev_priv); } /** @@ -1289,6 +1291,8 @@ static void i915_driver_register(struct drm_i915_private *dev_priv) */ static void i915_driver_unregister(struct drm_i915_private *dev_priv) { + intel_runtime_pm_disable(dev_priv); + intel_fbdev_unregister(dev_priv); intel_audio_deinit(dev_priv); @@ -1408,8 +1412,6 @@ int i915_driver_load(struct pci_dev *pdev, const struct pci_device_id *ent) i915_driver_register(dev_priv); - intel_runtime_pm_enable(dev_priv); - intel_init_ipc(dev_priv); intel_runtime_pm_put(dev_priv); @@ -1474,6 +1476,8 @@ void i915_driver_unload(struct drm_device *dev) i915_driver_cleanup_mmio(dev_priv); intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); + + WARN_ON(atomic_read(&dev_priv->runtime_pm.wakeref_count)); } static void i915_driver_release(struct drm_device *dev) diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0601abb8c71f..dc6c0cec9b36 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -1956,6 +1956,7 @@ void intel_power_domains_verify_state(struct drm_i915_private *dev_priv); void bxt_display_core_init(struct drm_i915_private *dev_priv, bool resume); void bxt_display_core_uninit(struct drm_i915_private *dev_priv); void intel_runtime_pm_enable(struct drm_i915_private *dev_priv); +void intel_runtime_pm_disable(struct drm_i915_private *dev_priv); const char * intel_display_power_domain_str(enum intel_display_power_domain domain); diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index e209edbc561d..b78c3b48aa62 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -3793,8 +3793,6 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume) */ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) { - struct device *kdev = &dev_priv->drm.pdev->dev; - /* * The i915.ko module is still not prepared to be loaded when * the power well is not enabled, so just enable it in case @@ -3809,13 +3807,6 @@ void intel_power_domains_fini_hw(struct drm_i915_private *dev_priv) /* Remove the refcount we took to keep power well support disabled. */ if (!i915_modparams.disable_power_well) intel_display_power_put(dev_priv, POWER_DOMAIN_INIT); - - /* - * Remove the refcount we took in intel_runtime_pm_enable() in case - * the platform doesn't support runtime PM. - */ - if (!HAS_RUNTIME_PM(dev_priv)) - pm_runtime_put(kdev); } /** @@ -4074,3 +4065,18 @@ void intel_runtime_pm_enable(struct drm_i915_private *dev_priv) */ pm_runtime_put_autosuspend(kdev); } + +void intel_runtime_pm_disable(struct drm_i915_private *dev_priv) +{ + struct pci_dev *pdev = dev_priv->drm.pdev; + struct device *kdev = &pdev->dev; + + pm_runtime_dont_use_autosuspend(kdev); + + /* + * Remove the refcount we took in intel_runtime_pm_enable() in case + * the platform doesn't support runtime PM. + */ + if (!HAS_RUNTIME_PM(dev_priv)) + pm_runtime_put(kdev); +}
Currently, we cancel the extra wakeref we have for !runtime-pm devices inside power_wells_fini_hw. However, this is not strictly paired with the acquisition of that wakeref in runtime_pm_enable (as the fini_hw may be called on errors paths before we even call runtime_pm_enable). Make the symmetry more explicit and include a check that we do release all of our rpm wakerefs. Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> --- drivers/gpu/drm/i915/i915_drv.c | 8 ++++++-- drivers/gpu/drm/i915/intel_drv.h | 1 + drivers/gpu/drm/i915/intel_runtime_pm.c | 24 +++++++++++++++--------- 3 files changed, 22 insertions(+), 11 deletions(-)