From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jesse Barnes Subject: Re: [PATCH] drm/i915: add energy counter support for IVB Date: Wed, 20 Jun 2012 08:32:59 -0700 Message-ID: <20120620083259.28bf0518@jbarnes-desktop> References: <20120619132050.6a423e71@jbarnes-desktop> <878vfiijwo.fsf@intel.com> <20120620073825.GB4835@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="us-ascii" Content-Transfer-Encoding: 7bit Return-path: Received: from oproxy6-pub.bluehost.com (oproxy6-pub.bluehost.com [67.222.54.6]) by gabe.freedesktop.org (Postfix) with SMTP id 31A539E9A9 for ; Wed, 20 Jun 2012 08:40:12 -0700 (PDT) In-Reply-To: <20120620073825.GB4835@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Sender: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org Errors-To: intel-gfx-bounces+gcfxdi-intel-gfx=m.gmane.org@lists.freedesktop.org To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org List-Id: intel-gfx@lists.freedesktop.org On Wed, 20 Jun 2012 09:38:25 +0200 Daniel Vetter wrote: > On Wed, Jun 20, 2012 at 09:20:39AM +0300, Jani Nikula wrote: > > On Tue, 19 Jun 2012, Jesse Barnes wrote: > > > On SNB and IVB, there's an MSR (also exposed through MCHBAR) we can use > > > to read out the amount of energy used over time. Expose this in debugfs > > > to make it easy to do power comparisons with different configurations. > > > > > > Signed-off-by: Jesse Barnes > > > > > > diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c > > > index c0b7688..c8998b4 100644 > > > --- a/drivers/gpu/drm/i915/i915_debugfs.c > > > +++ b/drivers/gpu/drm/i915/i915_debugfs.c > > > @@ -1897,6 +1897,41 @@ static const struct file_operations i915_cache_sharing_fops = { > > > .llseek = default_llseek, > > > }; > > > > > > +#define MSR_IA32_PACKAGE_POWER_SKU_UNIT 0x00000606 > > > + > > > +static int > > > +i915_energy_status(struct seq_file *m, void *data) > > > +{ > > > + struct drm_info_node *node = (struct drm_info_node *) m->private; > > > + struct drm_device *dev = node->minor->dev; > > > + struct drm_i915_private *dev_priv = dev->dev_private; > > > + u64 ppsu; > > > + u32 val, diff, units; > > > + > > > + if (!(IS_GEN6(dev) || IS_GEN7(dev))) { > > > + seq_printf(m, "Unsupported platform\n"); > > > + return 0; > > > + } > > > + > > > + rdmsrl(MSR_IA32_PACKAGE_POWER_SKU_UNIT, ppsu); > > > + > > > + ppsu = (ppsu & 0x1f00) >> 8; > > > + > > > + units = 1000000 / (1 << ppsu); /* convert to uJ */ > > > + > > > + mutex_lock(&dev->struct_mutex); > > > + val = I915_READ(SECP_NRG_STTS); > > > + if (val < dev_priv->last_secp) > > > + diff = val + (0xffffffff - dev_priv->last_secp); > > > > From the nitpickery dept.: I think that's off-by-one. But nobody will > > ever notice from the output. ;) > > And from the bikeshed departement: Can't we just print a running number? I > know, substraction is bloody hard, but for anything else than total power > consumption (e.g. graphing power over time) the running thing is imo > simpler. We've had the same discussion for the rc6 sysfs residency timers > and concluded (after Arjan yelled at us) that doing the substraction in > userspace is better, least it allows multiple userspace tools to read > this. > Yeah that's a good point; this way happened to be simpler for what I was doing, but just exposing the cooked register value (converted to ujoules) is better. Will fix. I'll also drop the bug fix hunk that Ben noticed. -- Jesse Barnes, Intel Open Source Technology Center