public inbox for intel-gfx@lists.freedesktop.org
 help / color / mirror / Atom feed
* [PATCH] drm/i915: fix handling of the disable_power_well module option
@ 2015-11-11 17:03 Imre Deak
  2015-11-17 12:46 ` Patrik Jakobsson
  2015-11-17 15:44 ` [PATCH v2] " Imre Deak
  0 siblings, 2 replies; 4+ messages in thread
From: Imre Deak @ 2015-11-11 17:03 UTC (permalink / raw)
  To: intel-gfx

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.
+	 */
+	if (!i915.disable_power_well)
+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 }
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: fix handling of the disable_power_well module option
  2015-11-11 17:03 [PATCH] drm/i915: fix handling of the disable_power_well module option Imre Deak
@ 2015-11-17 12:46 ` Patrik Jakobsson
  2015-11-17 19:36   ` Imre Deak
  2015-11-17 15:44 ` [PATCH v2] " Imre Deak
  1 sibling, 1 reply; 4+ messages in thread
From: Patrik Jakobsson @ 2015-11-17 12:46 UTC (permalink / raw)
  To: Imre Deak; +Cc: intel-gfx

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>

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
* [PATCH v2] drm/i915: fix handling of the disable_power_well module option
  2015-11-11 17:03 [PATCH] drm/i915: fix handling of the disable_power_well module option Imre Deak
  2015-11-17 12:46 ` Patrik Jakobsson
@ 2015-11-17 15:44 ` Imre Deak
  1 sibling, 0 replies; 4+ messages in thread
From: Imre Deak @ 2015-11-17 15:44 UTC (permalink / raw)
  To: intel-gfx

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.

v2:
- fix typo in comment in intel_power_domains_suspend() (Patrik)

Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Patrik Jakobsson <patrik.jakobsson@linux.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 93fc50a..ade9f23 100644
--- a/drivers/gpu/drm/i915/i915_params.c
+++ b/drivers/gpu/drm/i915/i915_params.c
@@ -125,7 +125,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 36dc8e1..a53abff 100644
--- a/drivers/gpu/drm/i915/intel_runtime_pm.c
+++ b/drivers/gpu/drm/i915/intel_runtime_pm.c
@@ -1412,7 +1412,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);
 	}
 
@@ -1872,6 +1872,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)
@@ -2073,6 +2077,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;
 }
@@ -2088,6 +2095,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 disable
+	 * power wells while we are system suspended.
+	 */
+	if (!i915.disable_power_well)
+		intel_display_power_put(dev_priv, POWER_DOMAIN_INIT);
 }
 
 /**
-- 
2.5.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

^ permalink raw reply related	[flat|nested] 4+ messages in thread
* Re: [PATCH] drm/i915: fix handling of the disable_power_well module option
  2015-11-17 12:46 ` Patrik Jakobsson
@ 2015-11-17 19:36   ` Imre Deak
  0 siblings, 0 replies; 4+ messages in thread
From: Imre Deak @ 2015-11-17 19:36 UTC (permalink / raw)
  To: Patrik Jakobsson; +Cc: intel-gfx

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

^ permalink raw reply	[flat|nested] 4+ messages in thread
end of thread, other threads:[~2015-11-17 19:36 UTC | newest]

Thread overview: 4+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2015-11-11 17:03 [PATCH] drm/i915: fix handling of the disable_power_well module option Imre Deak
2015-11-17 12:46 ` Patrik Jakobsson
2015-11-17 19:36   ` Imre Deak
2015-11-17 15:44 ` [PATCH v2] " Imre Deak

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox