From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 06/11] drm/i915: get/put runtime PM in more places at i915_debugfs.c Date: Wed, 5 Mar 2014 14:29:36 +0100 Message-ID: <20140305132936.GK17001@phenom.ffwll.local> References: <1393001548-2883-1-git-send-email-przanoni@gmail.com> <1393001548-2883-7-git-send-email-przanoni@gmail.com> <20140221094148.729c35da@jbarnes-desktop> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from mail-ee0-f48.google.com (mail-ee0-f48.google.com [74.125.83.48]) by gabe.freedesktop.org (Postfix) with ESMTP id 5B6EFFA629 for ; Wed, 5 Mar 2014 05:29:41 -0800 (PST) Received: by mail-ee0-f48.google.com with SMTP id e51so457710eek.7 for ; Wed, 05 Mar 2014 05:29:40 -0800 (PST) Content-Disposition: inline In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces@lists.freedesktop.org Errors-To: intel-gfx-bounces@lists.freedesktop.org To: Paulo Zanoni Cc: Intel Graphics Development , Paulo Zanoni List-Id: intel-gfx@lists.freedesktop.org On Fri, Feb 21, 2014 at 02:46:55PM -0300, Paulo Zanoni wrote: > 2014-02-21 14:41 GMT-03:00 Jesse Barnes : > > On Fri, 21 Feb 2014 13:52:23 -0300 > > Paulo Zanoni wrote: > > > >> From: Paulo Zanoni > >> > >> These are places where we read (not write) registers while we're > >> runtime suspended. > >> > >> Signed-off-by: Paulo Zanoni > >> --- > >> 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 > > > > -- > > 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 -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch