Message ID | 1387461309-2756-3-git-send-email-przanoni@gmail.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
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...
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
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
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 --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)