[Intel-gfx] [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c

[Daniel Vetter]

On Fri, Feb 21, 2014 at 02:46:55PM -0300, Paulo Zanoni wrote:
> 2014-02-21 14:41 GMT-03:00 Jesse Barnes <jbarnes at virtuousgeek.org>:
> > On Fri, 21 Feb 2014 13:52:23 -0300
> > Paulo Zanoni <przanoni at gmail.com> wrote:
> >
> >> From: Paulo Zanoni <paulo.r.zanoni at intel.com>
> >>
> >> These are places where we read (not write) registers while we're
> >> runtime suspended.
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni at 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 at virtuousgeek.org>
> >
> > --
> > Jesse Barnes, Intel Open Source Technology Center
> 
> 
> 
> -- 
> Paulo Zanoni
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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