diff mbox

[02/19] drm/i915: get/put runtime PM without holding rps.hw_lock

Message ID 1387461309-2756-3-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Dec. 19, 2013, 1:54 p.m. UTC 
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

We'll need this when we merge PC8 and Runtime PM: the PC8
enable/disable functions need that lock.

Also, it's good practice to not hold a lock for longer than strictly
needed.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

Comments

Jesse Barnes Dec. 19, 2013, 6:30 p.m. UTC | #1 
On Thu, 19 Dec 2013 11:54:52 -0200
Paulo Zanoni <przanoni@gmail.com> wrote:

> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We'll need this when we merge PC8 and Runtime PM: the PC8
> enable/disable functions need that lock.
> 
> Also, it's good practice to not hold a lock for longer than strictly
> needed.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 430eb3e..1cdc5dd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1414,7 +1414,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	int ret;
> +	int ret = 0;
>  	int gpu_freq, ia_freq;
>  
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> @@ -1422,12 +1422,13 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>  
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>  	if (ret)
> -		return ret;
> -	intel_runtime_pm_get(dev_priv);
> +		goto out;
>  
>  	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>  
> @@ -1444,10 +1445,11 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  			   ((ia_freq >> 8) & 0xff) * 100);
>  	}
>  
> -	intel_runtime_pm_put(dev_priv);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> -	return 0;
> +out:
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
>  }
>  
>  static int i915_gfxec(struct seq_file *m, void *unused)

So we have these runtime_pm_get/put calls all over now.  Is the plan to
convert those to specific wells as we add support for new platforms so
we can have fine grained well control rather than just global control?
I guess I need to dig out Imre's latest stuff and check...
Daniel Vetter Dec. 19, 2013, 9:20 p.m. UTC | #2 
On Thu, Dec 19, 2013 at 7:30 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> So we have these runtime_pm_get/put calls all over now.  Is the plan to
> convert those to specific wells as we add support for new platforms so
> we can have fine grained well control rather than just global control?
> I guess I need to dig out Imre's latest stuff and check...

My idea was that the power well stuff was just for the display side,
since there the hw engineers love to slice&dice things up in always
new ways. Other parts of the driver call get/put on specific power
domain functions (atm pc8+D3, but soon only D3).
-Daniel
Paulo Zanoni Dec. 19, 2013, 9:31 p.m. UTC | #3 
2013/12/19 Daniel Vetter <daniel@ffwll.ch>:
> On Thu, Dec 19, 2013 at 7:30 PM, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> So we have these runtime_pm_get/put calls all over now.  Is the plan to
>> convert those to specific wells as we add support for new platforms so
>> we can have fine grained well control rather than just global control?
>> I guess I need to dig out Imre's latest stuff and check...
>
> My idea was that the power well stuff was just for the display side,
> since there the hw engineers love to slice&dice things up in always
> new ways. Other parts of the driver call get/put on specific power
> domain functions (atm pc8+D3, but soon only D3).

I'm open to suggestions. This series removes all the calls to PC8
get/put, leaving only the "runtime PM" get/put and "power domains"
get/put, so IMHO we already do some cleanup.

Since Imre's recent rework I actually started to like the power
domains, and IMHO a nice approach for the future would be to add power
domains as needed and replace the "runtime PM get/put" calls with
proper power domain get/put calls. On this series there's a patch that
makes us get runtime PM every time we get a power domain, so it should
be easy to just add new domains. And I think it makes sense to also
add power domains for non-display things (as long as we rename
intel_display_power_{get,put} to intel_power_domain_{get,put}.

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch
Imre Deak Feb. 27, 2014, 1:45 p.m. UTC | #4 
On Thu, 2013-12-19 at 11:54 -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> We'll need this when we merge PC8 and Runtime PM: the PC8
> enable/disable functions need that lock.
> 
> Also, it's good practice to not hold a lock for longer than strictly
> needed.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Reviewed-by: Imre Deak <imre.deak@intel.com>

> ---
>  drivers/gpu/drm/i915/i915_debugfs.c | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
> index 430eb3e..1cdc5dd 100644
> --- a/drivers/gpu/drm/i915/i915_debugfs.c
> +++ b/drivers/gpu/drm/i915/i915_debugfs.c
> @@ -1414,7 +1414,7 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  	struct drm_info_node *node = (struct drm_info_node *) m->private;
>  	struct drm_device *dev = node->minor->dev;
>  	drm_i915_private_t *dev_priv = dev->dev_private;
> -	int ret;
> +	int ret = 0;
>  	int gpu_freq, ia_freq;
>  
>  	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
> @@ -1422,12 +1422,13 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  		return 0;
>  	}
>  
> +	intel_runtime_pm_get(dev_priv);
> +
>  	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
>  
>  	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
>  	if (ret)
> -		return ret;
> -	intel_runtime_pm_get(dev_priv);
> +		goto out;
>  
>  	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
>  
> @@ -1444,10 +1445,11 @@ static int i915_ring_freq_table(struct seq_file *m, void *unused)
>  			   ((ia_freq >> 8) & 0xff) * 100);
>  	}
>  
> -	intel_runtime_pm_put(dev_priv);
>  	mutex_unlock(&dev_priv->rps.hw_lock);
>  
> -	return 0;
> +out:
> +	intel_runtime_pm_put(dev_priv);
> +	return ret;
>  }
>  
>  static int i915_gfxec(struct seq_file *m, void *unused)
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 430eb3e..1cdc5dd 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1414,7 +1414,7 @@  static int i915_ring_freq_table(struct seq_file *m, void *unused)
 	struct drm_info_node *node = (struct drm_info_node *) m->private;
 	struct drm_device *dev = node->minor->dev;
 	drm_i915_private_t *dev_priv = dev->dev_private;
-	int ret;
+	int ret = 0;
 	int gpu_freq, ia_freq;
 
 	if (!(IS_GEN6(dev) || IS_GEN7(dev))) {
@@ -1422,12 +1422,13 @@  static int i915_ring_freq_table(struct seq_file *m, void *unused)
 		return 0;
 	}
 
+	intel_runtime_pm_get(dev_priv);
+
 	flush_delayed_work(&dev_priv->rps.delayed_resume_work);
 
 	ret = mutex_lock_interruptible(&dev_priv->rps.hw_lock);
 	if (ret)
-		return ret;
-	intel_runtime_pm_get(dev_priv);
+		goto out;
 
 	seq_puts(m, "GPU freq (MHz)\tEffective CPU freq (MHz)\tEffective Ring freq (MHz)\n");
 
@@ -1444,10 +1445,11 @@  static int i915_ring_freq_table(struct seq_file *m, void *unused)
 			   ((ia_freq >> 8) & 0xff) * 100);
 	}
 
-	intel_runtime_pm_put(dev_priv);
 	mutex_unlock(&dev_priv->rps.hw_lock);
 
-	return 0;
+out:
+	intel_runtime_pm_put(dev_priv);
+	return ret;
 }
 
 static int i915_gfxec(struct seq_file *m, void *unused)