[PATCH 1/2] drm/i915: enable deepest RC6 state

[PATCH 1/2] drm/i915: enable deepest RC6 state

[Eugeni Dodonov]

This should allow even more energy saving when GPU is not in use.
According to the testing, this state results in around 0.1 - 0.4 W better
power usage.

No issues or regressions observed so far, but additional testing is
certainly welcome.

Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 981b1f1..d1e5726 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7924,6 +7924,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 
 	if (i915_enable_rc6)
 		rc6_mask = GEN6_RC_CTL_RC6p_ENABLE |
+			GEN6_RC_CTL_RC6pp_ENABLE |
 			GEN6_RC_CTL_RC6_ENABLE;
 
 	I915_WRITE(GEN6_RC_CONTROL,
-- 
1.7.7.4

[PATCH 2/2] drm/i915: add debugfs interface to control rc6 and rps

[Eugeni Dodonov]

This allows to enable/disable rps and rc6 from userspace. This is
necessary for to have predictable results from hardware counters, and also
to provide a finer granularity over power control from userspace.

As an additional trick, we also change the value of i915_enable_rc6 by
using this value, to allow the module to know the latest status of rc6
requested by user.

Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_debugfs.c  |  101 ++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |    2 +-
 drivers/gpu/drm/i915/intel_drv.h     |    3 +
 3 files changed, 105 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_debugfs.c b/drivers/gpu/drm/i915/i915_debugfs.c
index 4f40f1c..dffc998 100644
--- a/drivers/gpu/drm/i915/i915_debugfs.c
+++ b/drivers/gpu/drm/i915/i915_debugfs.c
@@ -1340,6 +1340,79 @@ static const struct file_operations i915_wedged_fops = {
 };
 
 static int
+i915_enable_rc6_open(struct inode *inode,
+		   struct file *filp)
+{
+	filp->private_data = inode->i_private;
+	return 0;
+}
+
+static ssize_t
+i915_enable_rc6_read(struct file *filp,
+		   char __user *ubuf,
+		   size_t max,
+		   loff_t *ppos)
+{
+	char buf[80];
+	int len;
+
+	len = snprintf(buf, sizeof(buf),
+		       "%d\n", i915_enable_rc6);
+
+	if (len > sizeof(buf))
+		len = sizeof(buf);
+
+	return simple_read_from_buffer(ubuf, max, ppos, buf, len);
+}
+
+static ssize_t
+i915_enable_rc6_write(struct file *filp,
+		  const char __user *ubuf,
+		  size_t cnt,
+		  loff_t *ppos)
+{
+	struct drm_device *dev = filp->private_data;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	char buf[20];
+	int val = -1;
+
+	if (cnt > 0) {
+		if (cnt > sizeof(buf) - 1)
+			return -EINVAL;
+
+		if (copy_from_user(buf, ubuf, cnt))
+			return -EFAULT;
+		buf[cnt] = 0;
+
+		val = simple_strtol(buf, NULL, 0);
+	}
+
+	if (INTEL_INFO(dev)->gen < 5)
+		return cnt;
+
+	DRM_DEBUG_DRIVER("Manually setting rps and rc6 status to %d\n", val);
+	i915_enable_rc6 = val;
+
+	if (val == 0) {
+		if (IS_IRONLAKE_M(dev))
+			ironlake_disable_rc6(dev);
+		else {
+			gen6_disable_rps(dev);
+			gen6_update_ring_freq(dev_priv);
+		}
+	} else {
+		if (IS_IRONLAKE_M(dev))
+			ironlake_disable_rc6(dev);
+		else {
+			gen6_enable_rps(dev_priv);
+			gen6_update_ring_freq(dev_priv);
+		}
+	}
+
+	return cnt;
+}
+
+static int
 i915_max_freq_open(struct inode *inode,
 		   struct file *filp)
 {
@@ -1401,6 +1474,14 @@ i915_max_freq_write(struct file *filp,
 	return cnt;
 }
 
+static const struct file_operations i915_enable_rc6_fops = {
+	.owner = THIS_MODULE,
+	.open = i915_enable_rc6_open,
+	.read = i915_enable_rc6_read,
+	.write = i915_enable_rc6_write,
+	.llseek = default_llseek,
+};
+
 static const struct file_operations i915_max_freq_fops = {
 	.owner = THIS_MODULE,
 	.open = i915_max_freq_open,
@@ -1605,6 +1686,21 @@ static int i915_max_freq_create(struct dentry *root, struct drm_minor *minor)
 	return drm_add_fake_info_node(minor, ent, &i915_max_freq_fops);
 }
 
+static int i915_enable_rc6_create(struct dentry *root, struct drm_minor *minor)
+{
+	struct drm_device *dev = minor->dev;
+	struct dentry *ent;
+
+	ent = debugfs_create_file("i915_enable_rc6",
+				  S_IRUGO | S_IWUSR,
+				  root, dev,
+				  &i915_enable_rc6_fops);
+	if (IS_ERR(ent))
+		return PTR_ERR(ent);
+
+	return drm_add_fake_info_node(minor, ent, &i915_enable_rc6_fops);
+}
+
 static int i915_cache_sharing_create(struct dentry *root, struct drm_minor *minor)
 {
 	struct drm_device *dev = minor->dev;
@@ -1676,6 +1772,9 @@ int i915_debugfs_init(struct drm_minor *minor)
 	ret = i915_max_freq_create(minor->debugfs_root, minor);
 	if (ret)
 		return ret;
+	ret = i915_enable_rc6_create(minor->debugfs_root, minor);
+	if (ret)
+		return ret;
 	ret = i915_cache_sharing_create(minor->debugfs_root, minor);
 	if (ret)
 		return ret;
@@ -1695,6 +1794,8 @@ void i915_debugfs_cleanup(struct drm_minor *minor)
 				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_max_freq_fops,
 				 1, minor);
+	drm_debugfs_remove_files((struct drm_info_list *) &i915_enable_rc6_fops,
+				 1, minor);
 	drm_debugfs_remove_files((struct drm_info_list *) &i915_cache_sharing_fops,
 				 1, minor);
 }
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d1e5726..cb15c8a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8311,7 +8311,7 @@ static void ironlake_teardown_rc6(struct drm_device *dev)
 	}
 }
 
-static void ironlake_disable_rc6(struct drm_device *dev)
+void ironlake_disable_rc6(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bd9a604..ce11cc3 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -353,6 +353,9 @@ extern void gen6_update_ring_freq(struct drm_i915_private *dev_priv);
 extern void gen6_disable_rps(struct drm_device *dev);
 extern void intel_init_emon(struct drm_device *dev);
 
+extern void ironlake_enable_rc6(struct drm_device *dev);
+extern void ironlake_disable_rc6(struct drm_device *dev);
+
 extern int intel_pin_and_fence_fb_obj(struct drm_device *dev,
 				      struct drm_i915_gem_object *obj,
 				      struct intel_ring_buffer *pipelined);
-- 
1.7.7.4

Re: [PATCH 2/2] drm/i915: add debugfs interface to control rc6 and rps

[Chris Wilson]

On Tue, 29 Nov 2011 10:55:05 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This allows to enable/disable rps and rc6 from userspace. This is
> necessary for to have predictable results from hardware counters, and also
> to provide a finer granularity over power control from userspace.

Reading registers and counters is handled with i915_forcewake_user. So
the question is what benefit does htis give over module reload. It looks
far riskier...
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: add debugfs interface to control rc6 and rps

[Daniel Vetter]

On Tue, Nov 29, 2011 at 01:05:03PM +0000, Chris Wilson wrote:
> On Tue, 29 Nov 2011 10:55:05 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> > This allows to enable/disable rps and rc6 from userspace. This is
> > necessary for to have predictable results from hardware counters, and also
> > to provide a finer granularity over power control from userspace.
> 
> Reading registers and counters is handled with i915_forcewake_user. So
> the question is what benefit does htis give over module reload. It looks
> far riskier...

The observation architecture bspec says that we need to disable rc6,
otherwise we'll get garbage in the gpu perf counters. But imo I think this
should be handled in the kernel (we neeed to write tons of funny registers
to set things up anyway), at least for the time-based perf counter
sampling. For prototyping things with intel_reg_write disabling rc6 at
module load time seems sufficient to me.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH 2/2] drm/i915: add debugfs interface to control rc6 and rps

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 2060 bytes --]

On Tue, Nov 29, 2011 at 12:12, Daniel Vetter <daniel@ffwll.ch> wrote:

> On Tue, Nov 29, 2011 at 01:05:03PM +0000, Chris Wilson wrote:
> > On Tue, 29 Nov 2011 10:55:05 -0200, Eugeni Dodonov <
> eugeni.dodonov@intel.com> wrote:
> > > This allows to enable/disable rps and rc6 from userspace. This is
> > > necessary for to have predictable results from hardware counters, and
> also
> > > to provide a finer granularity over power control from userspace.
> >
> > Reading registers and counters is handled with i915_forcewake_user. So
> > the question is what benefit does htis give over module reload. It looks
> > far riskier...
>
> The observation architecture bspec says that we need to disable rc6,
> otherwise we'll get garbage in the gpu perf counters. But imo I think this
> should be handled in the kernel (we neeed to write tons of funny registers
> to set things up anyway), at least for the time-based perf counter
> sampling. For prototyping things with intel_reg_write disabling rc6 at
> module load time seems sufficient to me.
>

The major idea behind this was to provide additional facilities for
controlling power and performance-related details of the driver from
userspace, without module reloading or reboot.

Currently, both the GPU turbo and rc6/fbc operations are mostly autonomous,
there is not much we can influence on their behavior without a reboot or
module reload. So my idea was to have a way to toggle - and in some future,
ideally, fine-tune their behavior from userspace during the execution. For
GPU turbo, we can control it to some point from userspace via
i915_max_freq. And with this patch, we could also control the rps/rc6
behavior to some point as well.

For the perf counters, I thought on the following flow of execution with
this patch:
1. prev_val=$(cat /sys/kernel/debug/dri/0/i915_enable_rc6)
2. echo 0 > /sys/kernel/debug/dri/0/i915_enable_rc6
3. do the perf counters aquisition and testing
4. echo $(prev_val) > /sys/kernel/debug/dri/0/i915_enable_rc6

Comments?

-- 
Eugeni Dodonov
 <http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2553 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/2] drm/i915: add debugfs interface to control rc6 and rps

[Chris Wilson]

On Tue, 29 Nov 2011 12:26:23 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote:
> For the perf counters, I thought on the following flow of execution with
> this patch:
> 1. prev_val=$(cat /sys/kernel/debug/dri/0/i915_enable_rc6)
> 2. echo 0 > /sys/kernel/debug/dri/0/i915_enable_rc6
> 3. do the perf counters aquisition and testing
> 4. echo $(prev_val) > /sys/kernel/debug/dri/0/i915_enable_rc6
> 
> Comments?

debugfs should never be ABI. Development and debug testing is one thing,
but if we are serious about this, we need to start building an acceptable
user interface elsewhere in /sys.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: enable deepest RC6 state

[Chris Wilson]

On Tue, 29 Nov 2011 10:55:04 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This should allow even more energy saving when GPU is not in use.
> According to the testing, this state results in around 0.1 - 0.4 W better
> power usage.
> 
> No issues or regressions observed so far, but additional testing is
> certainly welcome.

The docs I saw said "not implemented; do not use". Do we have it on good
authority that this is safe and useful to enable? And doesn't it
require programming of more transition thresholds?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: enable deepest RC6 state

[Ben Widawsky]

On Tue, Nov 29, 2011 at 01:01:31PM +0000, Chris Wilson wrote:
> On Tue, 29 Nov 2011 10:55:04 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> > This should allow even more energy saving when GPU is not in use.
> > According to the testing, this state results in around 0.1 - 0.4 W better
> > power usage.
> > 
> > No issues or regressions observed so far, but additional testing is
> > certainly welcome.
> 
> The docs I saw said "not implemented; do not use". Do we have it on good
> authority that this is safe and useful to enable? And doesn't it
> require programming of more transition thresholds?
> -Chris

Yes, I think we do have to program more stuff to make this work. Perhaps
the BIOS puts in decent values for these registers though? We would have
to restore those on reset and resume I'd guess. If BIOS doesn't use
anything there, you probably aren't even entering these states.

Also, for posterity, there are 3 rc6 states, rc6, deep rc6, and deepest
rc6. I think deepest rc6 was recommended to avoid (though I don't recall
a specific root caused issue, just some data from Jesse, from the
windows team that it didn't seem stable). And I think deepest rc6 is
also referred to as rc7 sometimes.

IIRC, Jesse had patches for deep rc6, but it was very minimal
performance gain. Someone should check if the windows driver uses it.

Ben

Re: [PATCH 1/2] drm/i915: enable deepest RC6 state

[Eugeni Dodonov]

[-- Attachment #1.1: Type: text/plain, Size: 2130 bytes --]

On Tue, Nov 29, 2011 at 19:42, Ben Widawsky <ben@bwidawsk.net> wrote:

> On Tue, Nov 29, 2011 at 01:01:31PM +0000, Chris Wilson wrote:
> > On Tue, 29 Nov 2011 10:55:04 -0200, Eugeni Dodonov <
> eugeni.dodonov@intel.com> wrote:
> > > This should allow even more energy saving when GPU is not in use.
> > > According to the testing, this state results in around 0.1 - 0.4 W
> better
> > > power usage.
> > >
> > > No issues or regressions observed so far, but additional testing is
> > > certainly welcome.
> >
> > The docs I saw said "not implemented; do not use". Do we have it on good
> > authority that this is safe and useful to enable? And doesn't it
> > require programming of more transition thresholds?
> > -Chris
>
> Yes, I think we do have to program more stuff to make this work. Perhaps
> the BIOS puts in decent values for these registers though? We would have
> to restore those on reset and resume I'd guess. If BIOS doesn't use
> anything there, you probably aren't even entering these states.
>
> Also, for posterity, there are 3 rc6 states, rc6, deep rc6, and deepest
> rc6. I think deepest rc6 was recommended to avoid (though I don't recall
> a specific root caused issue, just some data from Jesse, from the
> windows team that it didn't seem stable). And I think deepest rc6 is
> also referred to as rc7 sometimes.
>

We already setup the variables for both deep and deepest rc6 in our driver
(GEN6_RC6p_* and GEN6_RC6pp_*), but we weren't using this additional state
previously - if I understood the documentation and the code correctly, we
do enable plain rc6 and deep rc6 currently. I haven't found any indications
which would tell to avoid it in the latest docs, and I also haven't seen
any regressions or issues with it being enabled on any of the machines, so
I thought it would be worth trying that additional state as well.

>From the testing which QA did for this patch, looks like we save between
0.1 and 0.4W when compared to what we had with i915_enable_rc6=1.

But in any case, this is all highly experimental and I'll do more testing
with it :).

-- 
Eugeni Dodonov
<http://eugeni.dodonov.net/>

[-- Attachment #1.2: Type: text/html, Size: 2684 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx