| Message ID | 20181012215218.5119-14-jose.souza@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [01/16] drm/i915: Properly set PCH as NOP when display is disabled | expand |
On Fri, 12 Oct 2018, José Roberto de Souza <jose.souza@intel.com> wrote: > There is just two power wells calls left after the previous changes: > - POWER_DOMAIN_INIT: used in load, unload, resume and suspend driver > paths > - POWER_DOMAIN_GT_IRQ: used by GEM to reduce interrupt latencies when > DMC is loaded > > Instead of adding several more 'if (INTEL_INFO(dev_priv)->num_pipes)' > it will be handled in intel_display_power_get/put() and if any > erroneous call is added later a error message will be printed making > easy get regressions. > > Other important point is that it will not turn power wells on or off > but it will still grab and release runtime power management > references this way kernel can power down the whole GPU when it is > not in use. > > Signed-off-by: José Roberto de Souza <jose.souza@intel.com> > --- > drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++ > 1 file changed, 15 insertions(+) > > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c > index 629091ad8337..56c65d921acd 100644 > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c > @@ -1524,6 +1524,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv, > struct i915_power_domains *power_domains = &dev_priv->power_domains; > struct i915_power_well *power_well; > > + if (!INTEL_INFO(dev_priv)->num_pipes) > + DRM_ERROR("Enabling a power well with display disabled"); What can the user do about this? It's an assert, so please make it a WARN_ON() and we'll get a backtrace as a bonus helping us fix the issue. > + > for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) > intel_power_well_get(dev_priv, power_well); > > @@ -1549,6 +1552,13 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, > > intel_runtime_pm_get(dev_priv); > > + /* With display disabled this should be the only 2 power domains > + * requested > + */ > + if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) && > + !INTEL_INFO(dev_priv)->num_pipes) > + return; I do wonder if this is the right thing to do. How bad is it? Where do we have the power get calls in areas that aren't already bypassed due to no displays? BR, Jani. > + > mutex_lock(&power_domains->lock); > > __intel_display_power_get_domain(dev_priv, domain); > @@ -1609,6 +1619,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > struct i915_power_domains *power_domains; > struct i915_power_well *power_well; > > + if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) && > + !INTEL_INFO(dev_priv)->num_pipes) > + goto end; > + > power_domains = &dev_priv->power_domains; > > mutex_lock(&power_domains->lock); > @@ -1623,6 +1637,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, > > mutex_unlock(&power_domains->lock); > > +end: > intel_runtime_pm_put(dev_priv); > }
diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c index 629091ad8337..56c65d921acd 100644 --- a/drivers/gpu/drm/i915/intel_runtime_pm.c +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c @@ -1524,6 +1524,9 @@ __intel_display_power_get_domain(struct drm_i915_private *dev_priv, struct i915_power_domains *power_domains = &dev_priv->power_domains; struct i915_power_well *power_well; + if (!INTEL_INFO(dev_priv)->num_pipes) + DRM_ERROR("Enabling a power well with display disabled"); + for_each_power_domain_well(dev_priv, power_well, BIT_ULL(domain)) intel_power_well_get(dev_priv, power_well); @@ -1549,6 +1552,13 @@ void intel_display_power_get(struct drm_i915_private *dev_priv, intel_runtime_pm_get(dev_priv); + /* With display disabled this should be the only 2 power domains + * requested + */ + if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) && + !INTEL_INFO(dev_priv)->num_pipes) + return; + mutex_lock(&power_domains->lock); __intel_display_power_get_domain(dev_priv, domain); @@ -1609,6 +1619,10 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, struct i915_power_domains *power_domains; struct i915_power_well *power_well; + if ((domain == POWER_DOMAIN_INIT || domain == POWER_DOMAIN_GT_IRQ) && + !INTEL_INFO(dev_priv)->num_pipes) + goto end; + power_domains = &dev_priv->power_domains; mutex_lock(&power_domains->lock); @@ -1623,6 +1637,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv, mutex_unlock(&power_domains->lock); +end: intel_runtime_pm_put(dev_priv); }
There is just two power wells calls left after the previous changes: - POWER_DOMAIN_INIT: used in load, unload, resume and suspend driver paths - POWER_DOMAIN_GT_IRQ: used by GEM to reduce interrupt latencies when DMC is loaded Instead of adding several more 'if (INTEL_INFO(dev_priv)->num_pipes)' it will be handled in intel_display_power_get/put() and if any erroneous call is added later a error message will be printed making easy get regressions. Other important point is that it will not turn power wells on or off but it will still grab and release runtime power management references this way kernel can power down the whole GPU when it is not in use. Signed-off-by: José Roberto de Souza <jose.souza@intel.com> --- drivers/gpu/drm/i915/intel_runtime_pm.c | 15 +++++++++++++++ 1 file changed, 15 insertions(+)