[PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

[PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

[Imre Deak]

Atm we call intel_display_power_enabled() from
i915_capture_error_state() in IRQ context and then take a mutex. To fix
this add a new intel_display_power_enabled_sw() which returns the domain
state based on software tracking as opposed to reading the actual HW
state.

Since we use domain_use_count for this without locking on the reader
side make sure we increase the counter only after enabling all required
power wells and decrease it before disabling any of these power wells.

Regression introduced in
commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
Author: Imre Deak <imre.deak@intel.com>
Date:   Tue Sep 24 16:17:09 2013 +0300

    drm/i915: support for multiple power wells

Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 --
 drivers/gpu/drm/i915/intel_display.c |  4 ++--
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++++++++++++--------
 4 files changed, 34 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 47b8fd1..d17a62a 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -966,9 +966,7 @@ struct i915_power_domains {
 	int power_well_count;
 
 	struct mutex lock;
-#if IS_ENABLED(CONFIG_DEBUG_FS)
 	int domain_use_count[POWER_DOMAIN_NUM];
-#endif
 	struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index fab7d35..5b5d831 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11374,7 +11374,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
-		if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
+		if (!intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i)))
 			continue;
 
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
@@ -11410,7 +11410,7 @@ intel_display_capture_error_state(struct drm_device *dev)
 	for (i = 0; i < error->num_transcoders; i++) {
 		enum transcoder cpu_transcoder = transcoders[i];
 
-		if (!intel_display_power_enabled(dev,
+		if (!intel_display_power_enabled_sw(dev,
 				POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
 			continue;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index b2d2cc1..fb3cfc5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -830,6 +830,8 @@ int intel_power_domains_init(struct drm_device *dev);
 void intel_power_domains_remove(struct drm_device *dev);
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain);
+bool intel_display_power_enabled_sw(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index a1a54ab..b9a900d 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5633,6 +5633,21 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
+bool intel_display_power_enabled_sw(struct drm_device *dev,
+				    enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains;
+	bool enabled;
+
+	power_domains = &dev_priv->power_domains;
+
+	enabled = power_domains->domain_use_count[domain];
+	smp_rmb();
+
+	return enabled;
+}
+
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain)
 {
@@ -5753,12 +5768,16 @@ void intel_display_power_get(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 
-#if IS_ENABLED(CONFIG_DEBUG_FS)
-	power_domains->domain_use_count[domain]++;
-#endif
 	for_each_power_well(i, power_well, BIT(domain), power_domains)
 		__intel_power_well_get(dev, power_well);
 
+	/*
+	 * Don't reorder the adjustment of domain_use_count with whatever
+	 * __intel_power_well_get does.
+	 */
+	smp_wmb();
+	power_domains->domain_use_count[domain]++;
+
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5774,13 +5793,16 @@ void intel_display_power_put(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 
-	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
-		__intel_power_well_put(dev, power_well);
-
-#if IS_ENABLED(CONFIG_DEBUG_FS)
 	WARN_ON(!power_domains->domain_use_count[domain]);
 	power_domains->domain_use_count[domain]--;
-#endif
+	/*
+	 * Don't reorder the adjustment of domain_use_count with whatever
+	 * __intel_power_well_put does.
+	 */
+	smp_wmb();
+
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_put(dev, power_well);
 
 	mutex_unlock(&power_domains->lock);
 }
-- 
1.8.4

Re: [PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

[Ville Syrjälä]

On Wed, Nov 27, 2013 at 08:09:04PM +0200, Imre Deak wrote:
> Atm we call intel_display_power_enabled() from
> i915_capture_error_state() in IRQ context and then take a mutex. To fix
> this add a new intel_display_power_enabled_sw() which returns the domain
> state based on software tracking as opposed to reading the actual HW
> state.
> 
> Since we use domain_use_count for this without locking on the reader
> side make sure we increase the counter only after enabling all required
> power wells and decrease it before disabling any of these power wells.
> 
> Regression introduced in
> commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Tue Sep 24 16:17:09 2013 +0300
> 
>     drm/i915: support for multiple power wells
> 
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 --
>  drivers/gpu/drm/i915/intel_display.c |  4 ++--
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 38 ++++++++++++++++++++++++++++--------
>  4 files changed, 34 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 47b8fd1..d17a62a 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -966,9 +966,7 @@ struct i915_power_domains {
>  	int power_well_count;
>  
>  	struct mutex lock;
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>  	int domain_use_count[POWER_DOMAIN_NUM];
> -#endif
>  	struct i915_power_well *power_wells;
>  };
>  
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index fab7d35..5b5d831 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11374,7 +11374,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>  
>  	for_each_pipe(i) {
> -		if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
> +		if (!intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i)))
>  			continue;
>  
>  		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
> @@ -11410,7 +11410,7 @@ intel_display_capture_error_state(struct drm_device *dev)
>  	for (i = 0; i < error->num_transcoders; i++) {
>  		enum transcoder cpu_transcoder = transcoders[i];
>  
> -		if (!intel_display_power_enabled(dev,
> +		if (!intel_display_power_enabled_sw(dev,
>  				POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
>  			continue;
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index b2d2cc1..fb3cfc5 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -830,6 +830,8 @@ int intel_power_domains_init(struct drm_device *dev);
>  void intel_power_domains_remove(struct drm_device *dev);
>  bool intel_display_power_enabled(struct drm_device *dev,
>  				 enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +				    enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_device *dev,
>  			     enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index a1a54ab..b9a900d 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5633,6 +5633,21 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
>  		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>  
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +				    enum intel_display_power_domain domain)
> +{
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct i915_power_domains *power_domains;
> +	bool enabled;
> +
> +	power_domains = &dev_priv->power_domains;
> +
> +	enabled = power_domains->domain_use_count[domain];
> +	smp_rmb();

I'm not sure the barriers really provide extra value. Some other
thread could still come along just after this and disable the power
wells.

> +
> +	return enabled;
> +}
> +
>  bool intel_display_power_enabled(struct drm_device *dev,
>  				 enum intel_display_power_domain domain)
>  {
> @@ -5753,12 +5768,16 @@ void intel_display_power_get(struct drm_device *dev,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> -	power_domains->domain_use_count[domain]++;
> -#endif
>  	for_each_power_well(i, power_well, BIT(domain), power_domains)
>  		__intel_power_well_get(dev, power_well);
>  
> +	/*
> +	 * Don't reorder the adjustment of domain_use_count with whatever
> +	 * __intel_power_well_get does.
> +	 */
> +	smp_wmb();
> +	power_domains->domain_use_count[domain]++;
> +
>  	mutex_unlock(&power_domains->lock);
>  }
>  
> @@ -5774,13 +5793,16 @@ void intel_display_power_put(struct drm_device *dev,
>  
>  	mutex_lock(&power_domains->lock);
>  
> -	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> -		__intel_power_well_put(dev, power_well);
> -
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>  	WARN_ON(!power_domains->domain_use_count[domain]);
>  	power_domains->domain_use_count[domain]--;
> -#endif
> +	/*
> +	 * Don't reorder the adjustment of domain_use_count with whatever
> +	 * __intel_power_well_put does.
> +	 */
> +	smp_wmb();
> +
> +	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> +		__intel_power_well_put(dev, power_well);
>  
>  	mutex_unlock(&power_domains->lock);
>  }
> -- 
> 1.8.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

[Daniel Vetter]

On Wed, Nov 27, 2013 at 7:30 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> I'm not sure the barriers really provide extra value. Some other
> thread could still come along just after this and disable the power
> wells.

Yeah, the barrriers don't really add anything - the access from the
capture code is racy, but we don't really care about that. I'd still
like to see us dump this information into the error state though since
now the sw tracking could get out of sync with the hw and leave us
puzzled when trying to analyze such a dump.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: add intel_display_power_enabled_sw for use in atomic ctx

[Imre Deak]

On Wed, 2013-11-27 at 19:38 +0100, Daniel Vetter wrote:
> On Wed, Nov 27, 2013 at 7:30 PM, Ville Syrjälä
> <ville.syrjala@linux.intel.com> wrote:
> > I'm not sure the barriers really provide extra value. Some other
> > thread could still come along just after this and disable the power
> > wells.
> 
> Yeah, the barrriers don't really add anything - the access from the
> capture code is racy, but we don't really care about that. I'd still
> like to see us dump this information into the error state though since
> now the sw tracking could get out of sync with the hw and leave us
> puzzled when trying to analyze such a dump.

Right, so calling any form of intel_display_power_enabled() is
pointless. So that should be also removed and the value of
intel_display_power_enabled_sw() saved.

--Imre

> -Daniel


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

[PATCH] drm/i915: add intel_display_power_enabled_sw() for use in atomic ctx

[Imre Deak]

Atm we call intel_display_power_enabled() from
i915_capture_error_state() in IRQ context and then take a mutex. To fix
this add a new intel_display_power_enabled_sw() which returns the domain
state based on software tracking as opposed to reading the actual HW
state.

Since we use domain_use_count for this without locking on the reader
side make sure we increase the counter only after enabling all required
power wells and decrease it before disabling any of these power wells.

Regression introduced in
commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
Author: Imre Deak <imre.deak@intel.com>
Date:   Tue Sep 24 16:17:09 2013 +0300

    drm/i915: support for multiple power wells

Note that atm we depend on the value returned by
intel_display_power_enabled_sw() in i915_capture_error_state() to avoid
unclaimed register access reports. This was never guaranteed though,
since another thread can disable the power concurrently. If this is a
problem we need another explicit way to disable the reporting during
error captures.

v2:
- remove barriers as the caller can't depend on the value
  returned from i915_capture_error_state_sw() anyway (Ville)
- dump the state of pipe/transcoder power domain state (Daniel)

Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Imre Deak <imre.deak@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 --
 drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  2 ++
 drivers/gpu/drm/i915/intel_pm.c      | 24 ++++++++++++++++--------
 4 files changed, 30 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index ed7a1fc..cc06b9f 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -971,9 +971,7 @@ struct i915_power_domains {
 	int power_well_count;
 
 	struct mutex lock;
-#if IS_ENABLED(CONFIG_DEBUG_FS)
 	int domain_use_count[POWER_DOMAIN_NUM];
-#endif
 	struct i915_power_well *power_wells;
 };
 
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e4dbae2..464ae66 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -11334,6 +11334,7 @@ struct intel_display_error_state {
 	} cursor[I915_MAX_PIPES];
 
 	struct intel_pipe_error_state {
+		bool power_domain_on;
 		u32 source;
 	} pipe[I915_MAX_PIPES];
 
@@ -11348,6 +11349,7 @@ struct intel_display_error_state {
 	} plane[I915_MAX_PIPES];
 
 	struct intel_transcoder_error_state {
+		bool power_domain_on;
 		enum transcoder cpu_transcoder;
 
 		u32 conf;
@@ -11385,7 +11387,9 @@ intel_display_capture_error_state(struct drm_device *dev)
 		error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
 
 	for_each_pipe(i) {
-		if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
+		error->pipe[i].power_domain_on =
+			intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
+		if (!error->pipe[i].power_domain_on)
 			continue;
 
 		if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
@@ -11421,8 +11425,9 @@ intel_display_capture_error_state(struct drm_device *dev)
 	for (i = 0; i < error->num_transcoders; i++) {
 		enum transcoder cpu_transcoder = transcoders[i];
 
-		if (!intel_display_power_enabled(dev,
-				POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
+		error->transcoder[i].power_domain_on =
+			intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
+		if (!error->transcoder[i].power_domain_on)
 			continue;
 
 		error->transcoder[i].cpu_transcoder = cpu_transcoder;
@@ -11457,6 +11462,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 			   error->power_well_driver);
 	for_each_pipe(i) {
 		err_printf(m, "Pipe [%d]:\n", i);
+		err_printf(m, "  Power: %s\n",
+			   error->pipe[i].power_domain_on ? "on" : "off");
 		err_printf(m, "  SRC: %08x\n", error->pipe[i].source);
 
 		err_printf(m, "Plane [%d]:\n", i);
@@ -11482,6 +11489,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
 	for (i = 0; i < error->num_transcoders; i++) {
 		err_printf(m, "CPU transcoder: %c\n",
 			   transcoder_name(error->transcoder[i].cpu_transcoder));
+		err_printf(m, "  Power: %s\n",
+			   error->transcoder[i].power_domain_on ? "on" : "off");
 		err_printf(m, "  CONF: %08x\n", error->transcoder[i].conf);
 		err_printf(m, "  HTOTAL: %08x\n", error->transcoder[i].htotal);
 		err_printf(m, "  HBLANK: %08x\n", error->transcoder[i].hblank);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 0231281..5dea389 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -842,6 +842,8 @@ int intel_power_domains_init(struct drm_device *dev);
 void intel_power_domains_remove(struct drm_device *dev);
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain);
+bool intel_display_power_enabled_sw(struct drm_device *dev,
+				    enum intel_display_power_domain domain);
 void intel_display_power_get(struct drm_device *dev,
 			     enum intel_display_power_domain domain);
 void intel_display_power_put(struct drm_device *dev,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 7149f60..311199c 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -5635,6 +5635,17 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
 		     (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
 }
 
+bool intel_display_power_enabled_sw(struct drm_device *dev,
+				    enum intel_display_power_domain domain)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct i915_power_domains *power_domains;
+
+	power_domains = &dev_priv->power_domains;
+
+	return power_domains->domain_use_count[domain];
+}
+
 bool intel_display_power_enabled(struct drm_device *dev,
 				 enum intel_display_power_domain domain)
 {
@@ -5755,12 +5766,11 @@ void intel_display_power_get(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 
-#if IS_ENABLED(CONFIG_DEBUG_FS)
-	power_domains->domain_use_count[domain]++;
-#endif
 	for_each_power_well(i, power_well, BIT(domain), power_domains)
 		__intel_power_well_get(dev, power_well);
 
+	power_domains->domain_use_count[domain]++;
+
 	mutex_unlock(&power_domains->lock);
 }
 
@@ -5776,13 +5786,11 @@ void intel_display_power_put(struct drm_device *dev,
 
 	mutex_lock(&power_domains->lock);
 
-	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
-		__intel_power_well_put(dev, power_well);
-
-#if IS_ENABLED(CONFIG_DEBUG_FS)
 	WARN_ON(!power_domains->domain_use_count[domain]);
 	power_domains->domain_use_count[domain]--;
-#endif
+
+	for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
+		__intel_power_well_put(dev, power_well);
 
 	mutex_unlock(&power_domains->lock);
 }
-- 
1.8.4

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

Re: [PATCH] drm/i915: add intel_display_power_enabled_sw() for use in atomic ctx

[Paulo Zanoni]

2013/11/27 Imre Deak <imre.deak@intel.com>:
> Atm we call intel_display_power_enabled() from
> i915_capture_error_state() in IRQ context and then take a mutex. To fix
> this add a new intel_display_power_enabled_sw() which returns the domain
> state based on software tracking as opposed to reading the actual HW
> state.
>
> Since we use domain_use_count for this without locking on the reader
> side make sure we increase the counter only after enabling all required
> power wells and decrease it before disabling any of these power wells.
>
> Regression introduced in
> commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
> Author: Imre Deak <imre.deak@intel.com>
> Date:   Tue Sep 24 16:17:09 2013 +0300
>
>     drm/i915: support for multiple power wells
>
> Note that atm we depend on the value returned by
> intel_display_power_enabled_sw() in i915_capture_error_state() to avoid
> unclaimed register access reports. This was never guaranteed though,
> since another thread can disable the power concurrently. If this is a
> problem we need another explicit way to disable the reporting during
> error captures.
>
> v2:
> - remove barriers as the caller can't depend on the value
>   returned from i915_capture_error_state_sw() anyway (Ville)
> - dump the state of pipe/transcoder power domain state (Daniel)
>
> Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Imre Deak <imre.deak@intel.com>


Makes sense to me. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>


> ---
>  drivers/gpu/drm/i915/i915_drv.h      |  2 --
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++++++++---
>  drivers/gpu/drm/i915/intel_drv.h     |  2 ++
>  drivers/gpu/drm/i915/intel_pm.c      | 24 ++++++++++++++++--------
>  4 files changed, 30 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index ed7a1fc..cc06b9f 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -971,9 +971,7 @@ struct i915_power_domains {
>         int power_well_count;
>
>         struct mutex lock;
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>         int domain_use_count[POWER_DOMAIN_NUM];
> -#endif
>         struct i915_power_well *power_wells;
>  };
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e4dbae2..464ae66 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -11334,6 +11334,7 @@ struct intel_display_error_state {
>         } cursor[I915_MAX_PIPES];
>
>         struct intel_pipe_error_state {
> +               bool power_domain_on;
>                 u32 source;
>         } pipe[I915_MAX_PIPES];
>
> @@ -11348,6 +11349,7 @@ struct intel_display_error_state {
>         } plane[I915_MAX_PIPES];
>
>         struct intel_transcoder_error_state {
> +               bool power_domain_on;
>                 enum transcoder cpu_transcoder;
>
>                 u32 conf;
> @@ -11385,7 +11387,9 @@ intel_display_capture_error_state(struct drm_device *dev)
>                 error->power_well_driver = I915_READ(HSW_PWR_WELL_DRIVER);
>
>         for_each_pipe(i) {
> -               if (!intel_display_power_enabled(dev, POWER_DOMAIN_PIPE(i)))
> +               error->pipe[i].power_domain_on =
> +                       intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
> +               if (!error->pipe[i].power_domain_on)
>                         continue;
>
>                 if (INTEL_INFO(dev)->gen <= 6 || IS_VALLEYVIEW(dev)) {
> @@ -11421,8 +11425,9 @@ intel_display_capture_error_state(struct drm_device *dev)
>         for (i = 0; i < error->num_transcoders; i++) {
>                 enum transcoder cpu_transcoder = transcoders[i];
>
> -               if (!intel_display_power_enabled(dev,
> -                               POWER_DOMAIN_TRANSCODER(cpu_transcoder)))
> +               error->transcoder[i].power_domain_on =
> +                       intel_display_power_enabled_sw(dev, POWER_DOMAIN_PIPE(i));
> +               if (!error->transcoder[i].power_domain_on)
>                         continue;
>
>                 error->transcoder[i].cpu_transcoder = cpu_transcoder;
> @@ -11457,6 +11462,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>                            error->power_well_driver);
>         for_each_pipe(i) {
>                 err_printf(m, "Pipe [%d]:\n", i);
> +               err_printf(m, "  Power: %s\n",
> +                          error->pipe[i].power_domain_on ? "on" : "off");
>                 err_printf(m, "  SRC: %08x\n", error->pipe[i].source);
>
>                 err_printf(m, "Plane [%d]:\n", i);
> @@ -11482,6 +11489,8 @@ intel_display_print_error_state(struct drm_i915_error_state_buf *m,
>         for (i = 0; i < error->num_transcoders; i++) {
>                 err_printf(m, "CPU transcoder: %c\n",
>                            transcoder_name(error->transcoder[i].cpu_transcoder));
> +               err_printf(m, "  Power: %s\n",
> +                          error->transcoder[i].power_domain_on ? "on" : "off");
>                 err_printf(m, "  CONF: %08x\n", error->transcoder[i].conf);
>                 err_printf(m, "  HTOTAL: %08x\n", error->transcoder[i].htotal);
>                 err_printf(m, "  HBLANK: %08x\n", error->transcoder[i].hblank);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 0231281..5dea389 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -842,6 +842,8 @@ int intel_power_domains_init(struct drm_device *dev);
>  void intel_power_domains_remove(struct drm_device *dev);
>  bool intel_display_power_enabled(struct drm_device *dev,
>                                  enum intel_display_power_domain domain);
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +                                   enum intel_display_power_domain domain);
>  void intel_display_power_get(struct drm_device *dev,
>                              enum intel_display_power_domain domain);
>  void intel_display_power_put(struct drm_device *dev,
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 7149f60..311199c 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -5635,6 +5635,17 @@ static bool hsw_power_well_enabled(struct drm_device *dev,
>                      (HSW_PWR_WELL_ENABLE_REQUEST | HSW_PWR_WELL_STATE_ENABLED);
>  }
>
> +bool intel_display_power_enabled_sw(struct drm_device *dev,
> +                                   enum intel_display_power_domain domain)
> +{
> +       struct drm_i915_private *dev_priv = dev->dev_private;
> +       struct i915_power_domains *power_domains;
> +
> +       power_domains = &dev_priv->power_domains;
> +
> +       return power_domains->domain_use_count[domain];
> +}
> +
>  bool intel_display_power_enabled(struct drm_device *dev,
>                                  enum intel_display_power_domain domain)
>  {
> @@ -5755,12 +5766,11 @@ void intel_display_power_get(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
> -       power_domains->domain_use_count[domain]++;
> -#endif
>         for_each_power_well(i, power_well, BIT(domain), power_domains)
>                 __intel_power_well_get(dev, power_well);
>
> +       power_domains->domain_use_count[domain]++;
> +
>         mutex_unlock(&power_domains->lock);
>  }
>
> @@ -5776,13 +5786,11 @@ void intel_display_power_put(struct drm_device *dev,
>
>         mutex_lock(&power_domains->lock);
>
> -       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> -               __intel_power_well_put(dev, power_well);
> -
> -#if IS_ENABLED(CONFIG_DEBUG_FS)
>         WARN_ON(!power_domains->domain_use_count[domain]);
>         power_domains->domain_use_count[domain]--;
> -#endif
> +
> +       for_each_power_well_rev(i, power_well, BIT(domain), power_domains)
> +               __intel_power_well_put(dev, power_well);
>
>         mutex_unlock(&power_domains->lock);
>  }
> --
> 1.8.4
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add intel_display_power_enabled_sw() for use in atomic ctx

[Daniel Vetter]

On Thu, Nov 28, 2013 at 11:50:33AM -0200, Paulo Zanoni wrote:
> 2013/11/27 Imre Deak <imre.deak@intel.com>:
> > Atm we call intel_display_power_enabled() from
> > i915_capture_error_state() in IRQ context and then take a mutex. To fix
> > this add a new intel_display_power_enabled_sw() which returns the domain
> > state based on software tracking as opposed to reading the actual HW
> > state.
> >
> > Since we use domain_use_count for this without locking on the reader
> > side make sure we increase the counter only after enabling all required
> > power wells and decrease it before disabling any of these power wells.
> >
> > Regression introduced in
> > commit 1b02383464b4a915627ef3b8fd0ad7f07168c54c
> > Author: Imre Deak <imre.deak@intel.com>
> > Date:   Tue Sep 24 16:17:09 2013 +0300
> >
> >     drm/i915: support for multiple power wells
> >
> > Note that atm we depend on the value returned by
> > intel_display_power_enabled_sw() in i915_capture_error_state() to avoid
> > unclaimed register access reports. This was never guaranteed though,
> > since another thread can disable the power concurrently. If this is a
> > problem we need another explicit way to disable the reporting during
> > error captures.
> >
> > v2:
> > - remove barriers as the caller can't depend on the value
> >   returned from i915_capture_error_state_sw() anyway (Ville)
> > - dump the state of pipe/transcoder power domain state (Daniel)
> >
> > Reported-by: Chris Wilson <chris@chris-wilson.co.uk>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> 
> 
> Makes sense to me. Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Queued for -next, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch