[PATCH] drm/i915: add energy counter support for IVB

[PATCH] drm/i915: add energy counter support for 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 debugfs
to make it easy to do power comparisons with different configurations.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>

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);
+	else
+		diff = val - dev_priv->last_secp;
+	dev_priv->last_secp = val;
+	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
+	mutex_unlock(&dev->struct_mutex);
+
+	return 0;
+}
+
 /* As the drm_debugfs_init() routines are called before dev->dev_private is
  * allocated we need to hook into the minor for release. */
 static int
@@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[] = {
 	{"i915_swizzle_info", i915_swizzle_info, 0},
 	{"i915_ppgtt_info", i915_ppgtt_info, 0},
 	{"i915_dpio", i915_dpio_info, 0},
+	{"i915_energy", i915_energy_status, 0},
 };
 #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
 
@@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_ring_stop_fops,
 				 1, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_error_state_fops,
+				 1, minor);
 }
 
 #endif /* CONFIG_DEBUG_FS */
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 11c7a6a..fb07415 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -802,6 +802,8 @@ typedef struct drm_i915_private {
 	u8 corr;
 	spinlock_t *mchdev_lock;
 
+	u32 last_secp;
+
 	enum no_fbc_reason no_fbc_reason;
 
 	struct drm_mm_node *compressed_fb;
diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
index 0f45a18..b7d1310 100644
--- a/drivers/gpu/drm/i915/i915_reg.h
+++ b/drivers/gpu/drm/i915/i915_reg.h
@@ -1219,6 +1219,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

Re: [PATCH] drm/i915: add energy counter support for IVB

[Ben Widawsky]

On Tue, 19 Jun 2012 13:20:50 -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 debugfs
> to make it easy to do power comparisons with different configurations.

I'd like some more explanation of why this belongs in i915. Furthermore,
it seems like something which belongs in sysfs, not debugfs.

> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> 
> 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);
> +	else
> +		diff = val - dev_priv->last_secp;
> +	dev_priv->last_secp = val;
> +	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
>  /* As the drm_debugfs_init() routines are called before dev->dev_private is
>   * allocated we need to hook into the minor for release. */
>  static int
> @@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_swizzle_info", i915_swizzle_info, 0},
>  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
>  	{"i915_dpio", i915_dpio_info, 0},
> +	{"i915_energy", i915_energy_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>  
> @@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
>  				 1, minor);
>  	drm_debugfs_remove_files((struct drm_info_list *) &i915_ring_stop_fops,
>  				 1, minor);
> +	drm_debugfs_remove_files((struct drm_info_list *) &i915_error_state_fops,
> +				 1, minor);
>  }
>  
>  #endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 11c7a6a..fb07415 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -802,6 +802,8 @@ typedef struct drm_i915_private {
>  	u8 corr;
>  	spinlock_t *mchdev_lock;
>  
> +	u32 last_secp;
> +
>  	enum no_fbc_reason no_fbc_reason;
>  
>  	struct drm_mm_node *compressed_fb;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0f45a18..b7d1310 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1219,6 +1219,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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add energy counter support for IVB

[Jesse Barnes]

Ben Widawsky <ben@bwidawsk.net> wrote:

>On Tue, 19 Jun 2012 13:20:50 -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
>debugfs
>> to make it easy to do power comparisons with different
>configurations.
>
>I'd like some more explanation of why this belongs in i915.
>Furthermore,
>it seems like something which belongs in sysfs, not debugfs.
>
>> 
>> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>> 
>> 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);
>> +	else
>> +		diff = val - dev_priv->last_secp;
>> +	dev_priv->last_secp = val;
>> +	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
>> +	mutex_unlock(&dev->struct_mutex);
>> +
>> +	return 0;
>> +}
>> +
>>  /* As the drm_debugfs_init() routines are called before
>dev->dev_private is
>>   * allocated we need to hook into the minor for release. */
>>  static int
>> @@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[]
>= {
>>  	{"i915_swizzle_info", i915_swizzle_info, 0},
>>  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
>>  	{"i915_dpio", i915_dpio_info, 0},
>> +	{"i915_energy", i915_energy_status, 0},
>>  };
>>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>>  
>> @@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor
>*minor)
>>  				 1, minor);
>>  	drm_debugfs_remove_files((struct drm_info_list *)
>&i915_ring_stop_fops,
>>  				 1, minor);
>> +	drm_debugfs_remove_files((struct drm_info_list *)
>&i915_error_state_fops,
>> +				 1, minor);
>>  }
>>  
>>  #endif /* CONFIG_DEBUG_FS */
>> diff --git a/drivers/gpu/drm/i915/i915_drv.h
>b/drivers/gpu/drm/i915/i915_drv.h
>> index 11c7a6a..fb07415 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.h
>> +++ b/drivers/gpu/drm/i915/i915_drv.h
>> @@ -802,6 +802,8 @@ typedef struct drm_i915_private {
>>  	u8 corr;
>>  	spinlock_t *mchdev_lock;
>>  
>> +	u32 last_secp;
>> +
>>  	enum no_fbc_reason no_fbc_reason;
>>  
>>  	struct drm_mm_node *compressed_fb;
>> diff --git a/drivers/gpu/drm/i915/i915_reg.h
>b/drivers/gpu/drm/i915/i915_reg.h
>> index 0f45a18..b7d1310 100644
>> --- a/drivers/gpu/drm/i915/i915_reg.h
>> +++ b/drivers/gpu/drm/i915/i915_reg.h
>> @@ -1219,6 +1219,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
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
>
>
>-- 
>Ben Widawsky, Intel Open Source Technology Center

The power reported here is for the GT, so I think i915 is appropriate. As for sysfs vs debugfs, I'm a wimp and don't want to define a new ABI. 
-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add energy counter support for IVB

[Ben Widawsky]

On Tue, 19 Jun 2012 17:59:13 -0700
Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Ben Widawsky <ben@bwidawsk.net> wrote:
> 
> >On Tue, 19 Jun 2012 13:20:50 -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
> >debugfs
> >> to make it easy to do power comparisons with different
> >configurations.
> >
> >I'd like some more explanation of why this belongs in i915.
> >Furthermore,
> >it seems like something which belongs in sysfs, not debugfs.
> >
> >> 
> >> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >> 
> >> 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);
> >> +	else
> >> +		diff = val - dev_priv->last_secp;
> >> +	dev_priv->last_secp = val;
> >> +	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
> >> +	mutex_unlock(&dev->struct_mutex);
> >> +
> >> +	return 0;
> >> +}
> >> +
> >>  /* As the drm_debugfs_init() routines are called before
> >dev->dev_private is
> >>   * allocated we need to hook into the minor for release. */
> >>  static int
> >> @@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[]
> >= {
> >>  	{"i915_swizzle_info", i915_swizzle_info, 0},
> >>  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
> >>  	{"i915_dpio", i915_dpio_info, 0},
> >> +	{"i915_energy", i915_energy_status, 0},
> >>  };
> >>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> >>  
> >> @@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor
> >*minor)
> >>  				 1, minor);
> >>  	drm_debugfs_remove_files((struct drm_info_list *)
> >&i915_ring_stop_fops,
> >>  				 1, minor);
> >> +	drm_debugfs_remove_files((struct drm_info_list *)
> >&i915_error_state_fops,
> >> +				 1, minor);

What is up with this hunk?

> >>  }
> >>  
> >>  #endif /* CONFIG_DEBUG_FS */
> >> diff --git a/drivers/gpu/drm/i915/i915_drv.h
> >b/drivers/gpu/drm/i915/i915_drv.h
> >> index 11c7a6a..fb07415 100644
> >> --- a/drivers/gpu/drm/i915/i915_drv.h
> >> +++ b/drivers/gpu/drm/i915/i915_drv.h
> >> @@ -802,6 +802,8 @@ typedef struct drm_i915_private {
> >>  	u8 corr;
> >>  	spinlock_t *mchdev_lock;
> >>  
> >> +	u32 last_secp;
> >> +
> >>  	enum no_fbc_reason no_fbc_reason;
> >>  
> >>  	struct drm_mm_node *compressed_fb;
> >> diff --git a/drivers/gpu/drm/i915/i915_reg.h
> >b/drivers/gpu/drm/i915/i915_reg.h
> >> index 0f45a18..b7d1310 100644
> >> --- a/drivers/gpu/drm/i915/i915_reg.h
> >> +++ b/drivers/gpu/drm/i915/i915_reg.h
> >> @@ -1219,6 +1219,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
> >> _______________________________________________
> >> Intel-gfx mailing list
> >> Intel-gfx@lists.freedesktop.org
> >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> >
> >
> >
> >-- 
> >Ben Widawsky, Intel Open Source Technology Center
> 
> The power reported here is for the GT, so I think i915 is appropriate. As for sysfs vs debugfs, I'm a wimp and don't want to define a new ABI. 

Ah, okay the MSR just give the units. I apologize for reading over it
too quickly.

-- 
Ben Widawsky, Intel Open Source Technology Center

Re: [PATCH] drm/i915: add energy counter support for IVB

[Jani Nikula]

On Tue, 19 Jun 2012, 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 debugfs
> to make it easy to do power comparisons with different configurations.
>
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
>
> 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. ;)

Jani.


> +	else
> +		diff = val - dev_priv->last_secp;
> +	dev_priv->last_secp = val;
> +	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
> +	mutex_unlock(&dev->struct_mutex);
> +
> +	return 0;
> +}
> +
>  /* As the drm_debugfs_init() routines are called before dev->dev_private is
>   * allocated we need to hook into the minor for release. */
>  static int
> @@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[] = {
>  	{"i915_swizzle_info", i915_swizzle_info, 0},
>  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
>  	{"i915_dpio", i915_dpio_info, 0},
> +	{"i915_energy", i915_energy_status, 0},
>  };
>  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
>  
> @@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
>  				 1, minor);
>  	drm_debugfs_remove_files((struct drm_info_list *) &i915_ring_stop_fops,
>  				 1, minor);
> +	drm_debugfs_remove_files((struct drm_info_list *) &i915_error_state_fops,
> +				 1, minor);
>  }
>  
>  #endif /* CONFIG_DEBUG_FS */
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 11c7a6a..fb07415 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -802,6 +802,8 @@ typedef struct drm_i915_private {
>  	u8 corr;
>  	spinlock_t *mchdev_lock;
>  
> +	u32 last_secp;
> +
>  	enum no_fbc_reason no_fbc_reason;
>  
>  	struct drm_mm_node *compressed_fb;
> diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> index 0f45a18..b7d1310 100644
> --- a/drivers/gpu/drm/i915/i915_reg.h
> +++ b/drivers/gpu/drm/i915/i915_reg.h
> @@ -1219,6 +1219,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
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: add energy counter support for IVB

[Daniel Vetter]

On Wed, Jun 20, 2012 at 09:20:39AM +0300, Jani Nikula wrote:
> On Tue, 19 Jun 2012, 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 debugfs
> > to make it easy to do power comparisons with different configurations.
> >
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> >
> > 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.

-Daniel

> 
> Jani.
> 
> 
> > +	else
> > +		diff = val - dev_priv->last_secp;
> > +	dev_priv->last_secp = val;
> > +	seq_printf(m, "Energy since last read: %u uJ\n", diff * units);
> > +	mutex_unlock(&dev->struct_mutex);
> > +
> > +	return 0;
> > +}
> > +
> >  /* As the drm_debugfs_init() routines are called before dev->dev_private is
> >   * allocated we need to hook into the minor for release. */
> >  static int
> > @@ -2035,6 +2070,7 @@ static struct drm_info_list i915_debugfs_list[] = {
> >  	{"i915_swizzle_info", i915_swizzle_info, 0},
> >  	{"i915_ppgtt_info", i915_ppgtt_info, 0},
> >  	{"i915_dpio", i915_dpio_info, 0},
> > +	{"i915_energy", i915_energy_status, 0},
> >  };
> >  #define I915_DEBUGFS_ENTRIES ARRAY_SIZE(i915_debugfs_list)
> >  
> > @@ -2102,6 +2138,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
> >  				 1, minor);
> >  	drm_debugfs_remove_files((struct drm_info_list *) &i915_ring_stop_fops,
> >  				 1, minor);
> > +	drm_debugfs_remove_files((struct drm_info_list *) &i915_error_state_fops,
> > +				 1, minor);
> >  }
> >  
> >  #endif /* CONFIG_DEBUG_FS */
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> > index 11c7a6a..fb07415 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -802,6 +802,8 @@ typedef struct drm_i915_private {
> >  	u8 corr;
> >  	spinlock_t *mchdev_lock;
> >  
> > +	u32 last_secp;
> > +
> >  	enum no_fbc_reason no_fbc_reason;
> >  
> >  	struct drm_mm_node *compressed_fb;
> > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h
> > index 0f45a18..b7d1310 100644
> > --- a/drivers/gpu/drm/i915/i915_reg.h
> > +++ b/drivers/gpu/drm/i915/i915_reg.h
> > @@ -1219,6 +1219,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
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
> _______________________________________________
> 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: add energy counter support for IVB

[Jesse Barnes]

On Wed, 20 Jun 2012 09:38:25 +0200
Daniel Vetter <daniel@ffwll.ch> wrote:

> On Wed, Jun 20, 2012 at 09:20:39AM +0300, Jani Nikula wrote:
> > On Tue, 19 Jun 2012, 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 debugfs
> > > to make it easy to do power comparisons with different configurations.
> > >
> > > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > >
> > > 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

Re: [PATCH] drm/i915: add energy counter support for IVB

[Chris Wilson]

On Wed, 20 Jun 2012 08:32:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Wed, 20 Jun 2012 09:38:25 +0200
> Daniel Vetter <daniel@ffwll.ch> wrote:
> > 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.

And to keep it easy to parse, just make it return -ENODEV on older
platforms and a simple number for gen6+.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add energy counter support for IVB

[Daniel Vetter]

On Wed, Jun 20, 2012 at 5:53 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, 20 Jun 2012 08:32:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> On Wed, 20 Jun 2012 09:38:25 +0200
>> Daniel Vetter <daniel@ffwll.ch> wrote:
>> > 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.
>
> And to keep it easy to parse, just make it return -ENODEV on older
> platforms and a simple number for gen6+.

At that point you might also add the uJ suffix and move it to sysfs ;-)
-Daniel
-- 
Daniel Vetter
daniel.vetter@ffwll.ch - +41 (0) 79 364 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: add energy counter support for IVB

[Eugeni Dodonov]

On 06/20/2012 12:59 PM, Daniel Vetter wrote:
> On Wed, Jun 20, 2012 at 5:53 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Wed, 20 Jun 2012 08:32:59 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>>> On Wed, 20 Jun 2012 09:38:25 +0200
>>> Daniel Vetter <daniel@ffwll.ch> wrote:
>>>> 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.
>>
>> And to keep it easy to parse, just make it return -ENODEV on older
>> platforms and a simple number for gen6+.
> 
> At that point you might also add the uJ suffix and move it to sysfs ;-)

And as a final minor suggestion, could we integrate this with the
/sys/kernel/debug/dri/0/i915_emon_status we have on ILK, so we could
have just one file for power readings?

Eugeni