[PATCH] drm/i915: expose energy counter on SNB and IVB

[PATCH] drm/i915: expose energy counter on SNB and IVB

[Jesse Barnes]

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 sysfs
to make it easy to do power comparisons with different configurations.

If the platform supports it, the file will show up under the
drm/card0/power subdirectory of the PCI device in sysfs as gt_energy_uJ.
The value in the file is a running total of energy (in microjoules)
consumed by the graphics device.

v2: move to sysfs (Ben, Daniel)
    expose a simple value (Chris)
    drop unrelated hunk (Ben)

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/i915_reg.h   |    2 +
 drivers/gpu/drm/i915/i915_sysfs.c |   39 +++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0a61481..ca6a620 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1224,6 +1224,8 @@
 #define CLKCFG_MEM_800					(3 << 4)
 #define CLKCFG_MEM_MASK					(7 << 4)
 
+#define SECP_NRG_STTS			(MCHBAR_MIRROR_BASE_SNB + 0x592c)
+
 #define TSC1			0x11001
 #define   TSE			(1<<0)
 #define TR1			0x11006
diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
index 2f5388a..c7fe7bd 100644
--- a/drivers/gpu/drm/i915/i915_sysfs.c
+++ b/drivers/gpu/drm/i915/i915_sysfs.c
@@ -93,6 +93,37 @@ static struct attribute_group rc6_attr_group = {
 	.attrs =  rc6_attrs
 };
 
+#define MSR_IA32_PACKAGE_POWER_SKU_UNIT		0x00000606
+
+static ssize_t
+show_gt_energy_uJ(struct device *dev, struct device_attribute *attr, char *buf)
+{
+	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
+	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
+	u64 ppsu;
+	u32 val, units;
+
+	rdmsrl(MSR_IA32_PACKAGE_POWER_SKU_UNIT, ppsu);
+
+	ppsu = (ppsu & 0x1f00) >> 8;
+	units = 1000000 / (1 << ppsu); /* convert to uJ */
+	val = I915_READ(SECP_NRG_STTS);
+
+	return snprintf(buf, PAGE_SIZE, "%u", val * units);
+}
+
+static DEVICE_ATTR(gt_energy_uJ, S_IRUGO, show_gt_energy_uJ, NULL);
+
+static struct attribute *gt_attrs[] = {
+	&dev_attr_gt_energy_uJ.attr,
+	NULL,
+};
+
+static struct attribute_group gt_attr_group = {
+	.name = power_group_name,
+	.attrs = gt_attrs,
+};
+
 static int l3_access_valid(struct drm_device *dev, loff_t offset)
 {
 	if (!IS_IVYBRIDGE(dev))
@@ -217,10 +248,18 @@ void i915_setup_sysfs(struct drm_device *dev)
 		if (ret)
 			DRM_ERROR("l3 parity sysfs setup failed\n");
 	}
+
+	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
+		ret = sysfs_merge_group(&dev->primary->kdev.kobj,
+					&gt_attr_group);
+		if (ret)
+			DRM_ERROR("GT energy sysfs setup failed\n");
+	}
 }
 
 void i915_teardown_sysfs(struct drm_device *dev)
 {
 	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
 	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
+	sysfs_unmerge_group(&dev->primary->kdev.kobj, &gt_attr_group);
 }
-- 
1.7.4.1

Re: [PATCH] drm/i915: expose energy counter on SNB and IVB

[Ben Widawsky]

On Wed, 20 Jun 2012 14:48:58 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> 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 sysfs
> to make it easy to do power comparisons with different configurations.
> 
> If the platform supports it, the file will show up under the
> drm/card0/power subdirectory of the PCI device in sysfs as gt_energy_uJ.
> The value in the file is a running total of energy (in microjoules)
> consumed by the graphics device.
> 
> v2: move to sysfs (Ben, Daniel)
>     expose a simple value (Chris)
>     drop unrelated hunk (Ben)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

I have some complaints, and a bikeshed - but it's
Acked-by: Ben Widawsky <ben@bwidawsk.net>
either way.

And color-me-danvet, but I kind of get why review is so mean on ABI
stuff.

I don't really care whether you address the other complaints, but I
won't do a a proper reviewed-by until the doc complaints are addressed.

> ---
>  drivers/gpu/drm/i915/i915_reg.h   |    2 +
>  drivers/gpu/drm/i915/i915_sysfs.c |   39 +++++++++++++++++++++++++++++++++++++
>  2 files changed, 41 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0a61481..ca6a620 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1224,6 +1224,8 @@
>  #define CLKCFG_MEM_800					(3 << 4)
>  #define CLKCFG_MEM_MASK					(7 << 4)
>  
> +#define SECP_NRG_STTS			(MCHBAR_MIRROR_BASE_SNB + 0x592c)
> +

I can't find where this is defined, so it's hard to review. Could you
please point me to the docs for this?

>  #define TSC1			0x11001
>  #define   TSE			(1<<0)
>  #define TR1			0x11006
> diff --git a/drivers/gpu/drm/i915/i915_sysfs.c b/drivers/gpu/drm/i915/i915_sysfs.c
> index 2f5388a..c7fe7bd 100644
> --- a/drivers/gpu/drm/i915/i915_sysfs.c
> +++ b/drivers/gpu/drm/i915/i915_sysfs.c
> @@ -93,6 +93,37 @@ static struct attribute_group rc6_attr_group = {
>  	.attrs =  rc6_attrs
>  };
>  
> +#define MSR_IA32_PACKAGE_POWER_SKU_UNIT		0x00000606

I honestly didn't try to look for this definition, but let's assume I
can't find it. Where is this one?

> +
> +static ssize_t
> +show_gt_energy_uJ(struct device *dev, struct device_attribute *attr, char *buf)
> +{
> +	struct drm_minor *dminor = container_of(dev, struct drm_minor, kdev);
> +	struct drm_i915_private *dev_priv = dminor->dev->dev_private;
> +	u64 ppsu;
> +	u32 val, units;
> +
> +	rdmsrl(MSR_IA32_PACKAGE_POWER_SKU_UNIT, ppsu);
> +
> +	ppsu = (ppsu & 0x1f00) >> 8;
> +	units = 1000000 / (1 << ppsu); /* convert to uJ */
> +	val = I915_READ(SECP_NRG_STTS);
> +
> +	return snprintf(buf, PAGE_SIZE, "%u", val * units);
> +}
> +
> +static DEVICE_ATTR(gt_energy_uJ, S_IRUGO, show_gt_energy_uJ, NULL);
> +
> +static struct attribute *gt_attrs[] = {
> +	&dev_attr_gt_energy_uJ.attr,
> +	NULL,
> +};

I think convention dictates it should be all lowercase. And while on
that, gt_energy_uJ is about as descriptive a name as rc6 (what jerk
named that anyway?). I think something like consumed_microjoules is
better.

> +
> +static struct attribute_group gt_attr_group = {
> +	.name = power_group_name,
> +	.attrs = gt_attrs,
> +};
> +
>  static int l3_access_valid(struct drm_device *dev, loff_t offset)
>  {
>  	if (!IS_IVYBRIDGE(dev))
> @@ -217,10 +248,18 @@ void i915_setup_sysfs(struct drm_device *dev)
>  		if (ret)
>  			DRM_ERROR("l3 parity sysfs setup failed\n");
>  	}
> +
> +	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
> +		ret = sysfs_merge_group(&dev->primary->kdev.kobj,
> +					&gt_attr_group);
> +		if (ret)
> +			DRM_ERROR("GT energy sysfs setup failed\n");
> +	}
>  }
>  
>  void i915_teardown_sysfs(struct drm_device *dev)
>  {
>  	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
>  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> +	sysfs_unmerge_group(&dev->primary->kdev.kobj, &gt_attr_group);
>  }

teardown should occur in reverse of init, so I'd put this on top.



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: expose energy counter on SNB and IVB

[Daniel Vetter]

On Fri, Jun 22, 2012 at 07:04:29PM -0700, Ben Widawsky wrote:
> On Wed, 20 Jun 2012 14:48:58 -0700
> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > +static struct attribute *gt_attrs[] = {
> > +	&dev_attr_gt_energy_uJ.attr,
> > +	NULL,
> > +};
> 
> I think convention dictates it should be all lowercase. And while on
> that, gt_energy_uJ is about as descriptive a name as rc6 (what jerk
> named that anyway?). I think something like consumed_microjoules is
> better.

I admit that the uJ makes tons of sense for me - J is the official SI
abbrev. for joules (I'm a bit unsure about u for \mu, but it seems to be
customary). Adding consumed makes some sense I think, but otherwise it's
imo good if we stick with the names vpg ppl have come up. So
gt_consumed_energy_uJ anyone? I can bikeshed this name while applying ...
-Daniel

> 
> > +
> > +static struct attribute_group gt_attr_group = {
> > +	.name = power_group_name,
> > +	.attrs = gt_attrs,
> > +};
> > +
> >  static int l3_access_valid(struct drm_device *dev, loff_t offset)
> >  {
> >  	if (!IS_IVYBRIDGE(dev))
> > @@ -217,10 +248,18 @@ void i915_setup_sysfs(struct drm_device *dev)
> >  		if (ret)
> >  			DRM_ERROR("l3 parity sysfs setup failed\n");
> >  	}
> > +
> > +	if (IS_GEN6(dev) || IS_IVYBRIDGE(dev)) {
> > +		ret = sysfs_merge_group(&dev->primary->kdev.kobj,
> > +					&gt_attr_group);
> > +		if (ret)
> > +			DRM_ERROR("GT energy sysfs setup failed\n");
> > +	}
> >  }
> >  
> >  void i915_teardown_sysfs(struct drm_device *dev)
> >  {
> >  	device_remove_bin_file(&dev->primary->kdev,  &dpf_attrs);
> >  	sysfs_unmerge_group(&dev->primary->kdev.kobj, &rc6_attr_group);
> > +	sysfs_unmerge_group(&dev->primary->kdev.kobj, &gt_attr_group);
> >  }
> 
> teardown should occur in reverse of init, so I'd put this on top.
> 
> 
> 
> -- 
> Ben Widawsky, Intel Open Source Technology Center
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915: expose energy counter on SNB and IVB

[Eugeni Dodonov]

On 06/20/2012 06:48 PM, 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 sysfs
> to make it easy to do power comparisons with different configurations.
> 
> If the platform supports it, the file will show up under the
> drm/card0/power subdirectory of the PCI device in sysfs as gt_energy_uJ.
> The value in the file is a running total of energy (in microjoules)
> consumed by the graphics device.
> 
> v2: move to sysfs (Ben, Daniel)
>     expose a simple value (Chris)
>     drop unrelated hunk (Ben)
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

(Assuming I haven't checked all the magic register values but I trust
that they work):

Reviewed-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

Eugeni

Re: [PATCH] drm/i915: expose energy counter on SNB and IVB

[Eugeni Dodonov]

On 06/24/2012 07:01 AM, Daniel Vetter wrote:
> On Fri, Jun 22, 2012 at 07:04:29PM -0700, Ben Widawsky wrote:
>> On Wed, 20 Jun 2012 14:48:58 -0700
>> Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> +static struct attribute *gt_attrs[] = {
>>> +	&dev_attr_gt_energy_uJ.attr,
>>> +	NULL,
>>> +};
>>
>> I think convention dictates it should be all lowercase. And while on
>> that, gt_energy_uJ is about as descriptive a name as rc6 (what jerk
>> named that anyway?). I think something like consumed_microjoules is
>> better.
> 
> I admit that the uJ makes tons of sense for me - J is the official SI
> abbrev. for joules (I'm a bit unsure about u for \mu, but it seems to be
> customary). Adding consumed makes some sense I think, but otherwise it's
> imo good if we stick with the names vpg ppl have come up. So
> gt_consumed_energy_uJ anyone? I can bikeshed this name while applying ...
> -Daniel

<bikeshed>
I'd vote for gt_energy or gt_consumed_energy, which would provide
results in plain J instead of mJ or uJ. This would result in a smaller
name + power readings which are standardized.
</bikeshed>

But if we settle on uJ values, gt_consumed_energy_uJ seems to be more
self-explainable to me.

Eugeni