| Message ID | 1393001548-2883-7-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Fri, 21 Feb 2014 13:52:23 -0300 Paulo Zanoni <przanoni@gmail.com> wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > These are places where we read (not write) registers while we're > runtime suspended. > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > index d90a707..34e347f 100644 > --- a/drivers/gpu/drm/i915/i915_debugfs.c > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > return 0; > } > > + intel_runtime_pm_get(dev_priv); > + > if (intel_fbc_enabled(dev)) { > seq_puts(m, "FBC enabled\n"); > } else { > @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > } > seq_putc(m, '\n'); > } > + > + intel_runtime_pm_put(dev_priv); > + > return 0; > } > > @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused) > return 0; > } > > + intel_runtime_pm_get(dev_priv); > + > if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) > seq_puts(m, "enabled\n"); > else > seq_puts(m, "disabled\n"); > > + intel_runtime_pm_put(dev_priv); > + > return 0; > } > > @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) > drm_i915_private_t *dev_priv = dev->dev_private; > bool sr_enabled = false; > > + intel_runtime_pm_get(dev_priv); > + > if (HAS_PCH_SPLIT(dev)) > sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN; > else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev)) > @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) > else if (IS_PINEVIEW(dev)) > sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; > > + intel_runtime_pm_put(dev_priv); > + > seq_printf(m, "self-refresh: %s\n", > sr_enabled ? "enabled" : "disabled"); > > @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data) > if (INTEL_INFO(dev)->gen < 6) > return -ENODEV; > > + intel_runtime_pm_get(dev_priv); > + > rdmsrl(MSR_RAPL_POWER_UNIT, power); > power = (power & 0x1f00) >> 8; > units = 1000000 / (1 << power); /* convert to uJ */ > power = I915_READ(MCH_SECP_NRG_STTS); > power *= units; > > + intel_runtime_pm_put(dev_priv); > + > seq_printf(m, "%llu", (long long unsigned)power); > > return 0; Looks like there are more places we need this too.. wonder if it would be better to put the get into some wrapper around our sysfs files... But these bits look correct, if not sufficient, so: Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > On Fri, 21 Feb 2014 13:52:23 -0300 > Paulo Zanoni <przanoni@gmail.com> wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> These are places where we read (not write) registers while we're >> runtime suspended. >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> >> --- >> drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++ >> 1 file changed, 17 insertions(+) >> >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c >> index d90a707..34e347f 100644 >> --- a/drivers/gpu/drm/i915/i915_debugfs.c >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c >> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused) >> return 0; >> } >> >> + intel_runtime_pm_get(dev_priv); >> + >> if (intel_fbc_enabled(dev)) { >> seq_puts(m, "FBC enabled\n"); >> } else { >> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused) >> } >> seq_putc(m, '\n'); >> } >> + >> + intel_runtime_pm_put(dev_priv); >> + >> return 0; >> } >> >> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused) >> return 0; >> } >> >> + intel_runtime_pm_get(dev_priv); >> + >> if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) >> seq_puts(m, "enabled\n"); >> else >> seq_puts(m, "disabled\n"); >> >> + intel_runtime_pm_put(dev_priv); >> + >> return 0; >> } >> >> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) >> drm_i915_private_t *dev_priv = dev->dev_private; >> bool sr_enabled = false; >> >> + intel_runtime_pm_get(dev_priv); >> + >> if (HAS_PCH_SPLIT(dev)) >> sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN; >> else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev)) >> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) >> else if (IS_PINEVIEW(dev)) >> sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; >> >> + intel_runtime_pm_put(dev_priv); >> + >> seq_printf(m, "self-refresh: %s\n", >> sr_enabled ? "enabled" : "disabled"); >> >> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data) >> if (INTEL_INFO(dev)->gen < 6) >> return -ENODEV; >> >> + intel_runtime_pm_get(dev_priv); >> + >> rdmsrl(MSR_RAPL_POWER_UNIT, power); >> power = (power & 0x1f00) >> 8; >> units = 1000000 / (1 << power); /* convert to uJ */ >> power = I915_READ(MCH_SECP_NRG_STTS); >> power *= units; >> >> + intel_runtime_pm_put(dev_priv); >> + >> seq_printf(m, "%llu", (long long unsigned)power); >> >> return 0; > > Looks like there are more places we need this too.. wonder if it would > be better to put the get into some wrapper around our sysfs files... Well, at least the deubgfs-read test from pm_pc8 doesn't complain on my HSW and SNB. But yeah, I thought about the wrapper too. I imagine people adding new debugfs files will always forget about the runtime PM refcounts, so maybe the wrapper is safer. But I could do this in a separate patch. I also thought about wrapping the connector->detect functions. Thanks for the reviews! > > But these bits look correct, if not sufficient, so: > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > -- > Jesse Barnes, Intel Open Source Technology Center
On Fri, Feb 21, 2014 at 02:46:55PM -0300, Paulo Zanoni wrote: > 2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes@virtuousgeek.org>: > > On Fri, 21 Feb 2014 13:52:23 -0300 > > Paulo Zanoni <przanoni@gmail.com> wrote: > > > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> These are places where we read (not write) registers while we're > >> runtime suspended. > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> --- > >> drivers/gpu/drm/i915/i915_debugfs.c | 17 +++++++++++++++++ > >> 1 file changed, 17 insertions(+) > >> > >> diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > >> index d90a707..34e347f 100644 > >> --- a/drivers/gpu/drm/i915/i915_debugfs.c > >> +++ b/drivers/gpu/drm/i915/i915_debugfs.c > >> @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > >> return 0; > >> } > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> if (intel_fbc_enabled(dev)) { > >> seq_puts(m, "FBC enabled\n"); > >> } else { > >> @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused) > >> } > >> seq_putc(m, '\n'); > >> } > >> + > >> + intel_runtime_pm_put(dev_priv); > >> + > >> return 0; > >> } > >> > >> @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused) > >> return 0; > >> } > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) > >> seq_puts(m, "enabled\n"); > >> else > >> seq_puts(m, "disabled\n"); > >> > >> + intel_runtime_pm_put(dev_priv); > >> + > >> return 0; > >> } > >> > >> @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) > >> drm_i915_private_t *dev_priv = dev->dev_private; > >> bool sr_enabled = false; > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> if (HAS_PCH_SPLIT(dev)) > >> sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN; > >> else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev)) > >> @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) > >> else if (IS_PINEVIEW(dev)) > >> sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; > >> > >> + intel_runtime_pm_put(dev_priv); > >> + > >> seq_printf(m, "self-refresh: %s\n", > >> sr_enabled ? "enabled" : "disabled"); > >> > >> @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data) > >> if (INTEL_INFO(dev)->gen < 6) > >> return -ENODEV; > >> > >> + intel_runtime_pm_get(dev_priv); > >> + > >> rdmsrl(MSR_RAPL_POWER_UNIT, power); > >> power = (power & 0x1f00) >> 8; > >> units = 1000000 / (1 << power); /* convert to uJ */ > >> power = I915_READ(MCH_SECP_NRG_STTS); > >> power *= units; > >> > >> + intel_runtime_pm_put(dev_priv); > >> + > >> seq_printf(m, "%llu", (long long unsigned)power); > >> > >> return 0; > > > > Looks like there are more places we need this too.. wonder if it would > > be better to put the get into some wrapper around our sysfs files... > > Well, at least the deubgfs-read test from pm_pc8 doesn't complain on > my HSW and SNB. But yeah, I thought about the wrapper too. I imagine > people adding new debugfs files will always forget about the runtime > PM refcounts, so maybe the wrapper is safer. But I could do this in a > separate patch. > > I also thought about wrapping the connector->detect functions. I prefer explicit get/put calls instead of magic wrappers. runtime pm get/put calls can be rather heavy-weight functions when we need to actually bring up a domain again and restore piles of registers. So being explicit is imo a feature. Even more so with our explosion of power domains - I expect that some of the debugfs files need to grab multiple different domains all over the place. -Daniel > > Thanks for the reviews! > > > > > But these bits look correct, if not sufficient, so: > > > > Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org> > > > > -- > > Jesse Barnes, Intel Open Source Technology Center > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c index d90a707..34e347f 100644 --- a/drivers/gpu/drm/i915/i915_debugfs.c +++ b/drivers/gpu/drm/i915/i915_debugfs.c @@ -1348,6 +1348,8 @@ static int i915_fbc_status(struct seq_file *m, void *unused) return 0; } + intel_runtime_pm_get(dev_priv); + if (intel_fbc_enabled(dev)) { seq_puts(m, "FBC enabled\n"); } else { @@ -1391,6 +1393,9 @@ static int i915_fbc_status(struct seq_file *m, void *unused) } seq_putc(m, '\n'); } + + intel_runtime_pm_put(dev_priv); + return 0; } @@ -1405,11 +1410,15 @@ static int i915_ips_status(struct seq_file *m, void *unused) return 0; } + intel_runtime_pm_get(dev_priv); + if (IS_BROADWELL(dev) || I915_READ(IPS_CTL) & IPS_ENABLE) seq_puts(m, "enabled\n"); else seq_puts(m, "disabled\n"); + intel_runtime_pm_put(dev_priv); + return 0; } @@ -1420,6 +1429,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) drm_i915_private_t *dev_priv = dev->dev_private; bool sr_enabled = false; + intel_runtime_pm_get(dev_priv); + if (HAS_PCH_SPLIT(dev)) sr_enabled = I915_READ(WM1_LP_ILK) & WM1_LP_SR_EN; else if (IS_CRESTLINE(dev) || IS_I945G(dev) || IS_I945GM(dev)) @@ -1429,6 +1440,8 @@ static int i915_sr_status(struct seq_file *m, void *unused) else if (IS_PINEVIEW(dev)) sr_enabled = I915_READ(DSPFW3) & PINEVIEW_SELF_REFRESH_EN; + intel_runtime_pm_put(dev_priv); + seq_printf(m, "self-refresh: %s\n", sr_enabled ? "enabled" : "disabled"); @@ -1972,12 +1985,16 @@ static int i915_energy_uJ(struct seq_file *m, void *data) if (INTEL_INFO(dev)->gen < 6) return -ENODEV; + intel_runtime_pm_get(dev_priv); + rdmsrl(MSR_RAPL_POWER_UNIT, power); power = (power & 0x1f00) >> 8; units = 1000000 / (1 << power); /* convert to uJ */ power = I915_READ(MCH_SECP_NRG_STTS); power *= units; + intel_runtime_pm_put(dev_priv); + seq_printf(m, "%llu", (long long unsigned)power); return 0;