Re: [PATCH] drm/i915: fix handling of the disable_power_well module option

[Imre Deak <imre.deak@intel.com>]

On ti, 2015-11-17 at 13:46 +0100, Patrik Jakobsson wrote:
> On Wed, Nov 11, 2015 at 07:03:54PM +0200, Imre Deak wrote:
> > When this option is 0 (so the power well support is disabled) we are
> > supposed to enable all power wells once and don't disable them unless we
> > system suspend the device. Currently if the option is 0, we can call the
> > power well enable handlers multiple times, whenever their refcount
> > changes from 0->1. This may not be a problem for the HW, but it's not
> > logical and may trigger some warnings in the power well code which
> > doesn't expect this. So simply keep around a reference while we are
> > not system suspended to solve this. For simplicity mark the module
> > option read only, so we don't need to deal with re-enabling the feature
> > during runtime. If someone really needs that it could be added later in
> > a more proper way.
> > 
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_params.c      |  2 +-
> >  drivers/gpu/drm/i915/intel_runtime_pm.c | 16 +++++++++++++++-
> >  2 files changed, 16 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/i915_params.c b/drivers/gpu/drm/i915/i915_params.c
> > index 7cf7474..2b36e67 100644
> > --- a/drivers/gpu/drm/i915/i915_params.c
> > +++ b/drivers/gpu/drm/i915/i915_params.c
> > @@ -131,7 +131,7 @@ module_param_named_unsafe(preliminary_hw_support, i915.preliminary_hw_support, i
> >  MODULE_PARM_DESC(preliminary_hw_support,
> >  	"Enable preliminary hardware support.");
> >  
> > -module_param_named_unsafe(disable_power_well, i915.disable_power_well, int, 0600);
> > +module_param_named_unsafe(disable_power_well, i915.disable_power_well, int, 0400);
> >  MODULE_PARM_DESC(disable_power_well,
> >  	"Disable display power wells when possible "
> >  	"(-1=auto [default], 0=power wells always on, 1=power wells disabled when possible)");
> > diff --git a/drivers/gpu/drm/i915/intel_runtime_pm.c b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > index b33969b..d167a45 100644
> > --- a/drivers/gpu/drm/i915/intel_runtime_pm.c
> > +++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
> > @@ -1427,7 +1427,7 @@ void intel_display_power_put(struct drm_i915_private *dev_priv,
> >  	for_each_power_well_rev(i, power_well, BIT(domain), power_domains) {
> >  		WARN_ON(!power_well->count);
> >  
> > -		if (!--power_well->count && i915.disable_power_well)
> > +		if (!--power_well->count)
> >  			intel_power_well_disable(dev_priv, power_well);
> >  	}
> >  
> > @@ -1899,6 +1899,10 @@ void intel_power_domains_fini(struct drm_i915_private *dev_priv)
> >  	 * the power well is not enabled, so just enable it in case
> >  	 * we're going to unload/reload. */
> >  	intel_display_set_init_power(dev_priv, true);
> > +
> > +	/* Remove the refcount we took to keep power well support disabled. */
> > +	if (!i915.disable_power_well)
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >  }
> >  
> >  static void intel_power_domains_sync_hw(struct drm_i915_private *dev_priv)
> > @@ -2099,6 +2103,9 @@ void intel_power_domains_init_hw(struct drm_i915_private *dev_priv, bool resume)
> >  
> >  	/* For now, we need the power well to be always enabled. */
> >  	intel_display_set_init_power(dev_priv, true);
> > +	/* Disable power support if the user asked so. */
> > +	if (!i915.disable_power_well)
> > +		intel_display_power_get(dev_priv, POWER_DOMAIN_INIT);
> >  	intel_power_domains_sync_hw(dev_priv);
> >  	power_domains->initializing = false;
> >  }
> > @@ -2114,6 +2121,13 @@ void intel_power_domains_suspend(struct drm_i915_private *dev_priv)
> >  {
> >  	if (IS_SKYLAKE(dev_priv) || IS_KABYLAKE(dev_priv))
> >  		skl_display_core_uninit(dev_priv);
> > +
> > +	/*
> > +	 * Even if power well support was disabled we still want to disabled
> > +	 * we want to disable the power wells while we are system suspended.
> > +	 */
> 
> This comment needs fixing. With that done:
> 
> Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.intel.com>

Pushed to dinq, thanks for the review.

> 
> > +	if (!i915.disable_power_well)
> > +		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
> >  }
> >  
> >  /**
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx