[PATCH 0/2] Another chapter in RC6 saga...

[PATCH 0/2] Another chapter in RC6 saga...

[Eugeni Dodonov]

Hi,

so far, apparently all the RC6-related issues on Sandy Bridge seem to be gone
when we enable RC6 but do not enable deep RC6. This apparently also cures
the symptoms we were seeing with RC6 when VTd was active, so in theory we
won't need the intel_iommu duct-taping anymore.

Somehow, we haven't attempted to flip the default RC6 status once again this
year (yet), so this seems to be a good timing to get this into -next and ask
the ones affected by RC6 bugs with 3.2 kernel to test.

If this indeed would fix the problems with rc6, I guess we can slip this
into -fixes; but with all the RC6 default status flip-flopping of the past
months I prefer to be sure that it won't get reverted once again.

Note that this patch also gives a way to get into deepest rc6 - but it
really never should be enabled by default. It is provided merely as a way to
easy the debugging of those RC6 issues in the future.

Eugeni Dodonov (2):
  drm/i915: allow to select rc6 modes via kernel parameter
  drm/i915: enable plain RC6 on Sandy Bridge by default

 drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
 drivers/gpu/drm/i915/i915_drv.h      |    5 +++++
 drivers/gpu/drm/i915/intel_display.c |   32 ++++++++++++++++++++++++--------
 3 files changed, 34 insertions(+), 9 deletions(-)

-- 
1.7.8.4

[PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter

[Eugeni Dodonov]

This allows to select which rc6 modes are to be used via kernel parameter,
via a bitmask parameter. E.g.:

- to enable rc6, i915_enable_rc6=1
- to enable rc6 and deep rc6, i915_enable_rc6=3
- to enable rc6 and deepest rc6, use i915_enable_rc6=5
- to enable rc6, deep and deepest rc6, use i915_enable_rc6=7

Please keep in mind that the deepest RC6 state really should NOT be used
by default, as it could potentially worsen the issues with deep RC6. So do
enable it only when you know what you are doing. However, having it around
could help solving possible future rc6-related issues and their debugging
on user machines.

Note that this changes behavior - previously, value of 1 would enable both
RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
stages must be enabled manually.

Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
 drivers/gpu/drm/i915/i915_drv.h      |    5 +++++
 drivers/gpu/drm/i915/intel_display.c |   24 ++++++++++++++++++++----
 3 files changed, 30 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a1103fc..b7a91db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -66,7 +66,11 @@ MODULE_PARM_DESC(semaphores,
 int i915_enable_rc6 __read_mostly = -1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 MODULE_PARM_DESC(i915_enable_rc6,
-		"Enable power-saving render C-state 6 (default: -1 (use per-chip default)");
+		"Enable power-saving render C-state 6. "
+		"Different stages can be selected via bitmask values "
+		"(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 = enable deepest rc6). "
+		"For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. "
+		"default: -1 (use per-chip default)");
 
 int i915_enable_fbc __read_mostly = -1;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 554bef7..51ee407 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -997,6 +997,11 @@ struct drm_i915_file_private {
 
 #include "i915_trace.h"
 
+/* RC6 modes */
+#define INTEL_RC6_ENABLE			(1<<0)
+#define INTEL_RC6p_ENABLE			(1<<1)
+#define INTEL_RC6pp_ENABLE			(1<<2)
+
 extern struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
 extern unsigned int i915_fbpercrtc __always_unused;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index daa5743..d636fc2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7907,7 +7907,7 @@ void intel_init_emon(struct drm_device *dev)
 	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
-static bool intel_enable_rc6(struct drm_device *dev)
+static int intel_enable_rc6(struct drm_device *dev)
 {
 	/*
 	 * Respect the kernel parameter if it is set
@@ -7938,6 +7938,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 	u32 pcu_mbox, rc6_mask = 0;
 	int cur_freq, min_freq, max_freq;
+	int rc6_mode;
 	int i;
 
 	/* Here begins a magic sequence of register writes to enable
@@ -7968,9 +7969,24 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_RC6p_THRESHOLD, 100000);
 	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
 
-	if (intel_enable_rc6(dev_priv->dev))
-		rc6_mask = GEN6_RC_CTL_RC6p_ENABLE |
-			GEN6_RC_CTL_RC6_ENABLE;
+	rc6_mode = intel_enable_rc6(dev_priv->dev);
+	/* Are we enabling rc6? */
+	if (rc6_mode > 0) {
+		if (rc6_mode & INTEL_RC6_ENABLE) {
+			DRM_DEBUG("i915: Enabling RC6\n");
+			rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
+		}
+
+		if (rc6_mode & INTEL_RC6p_ENABLE) {
+			DRM_DEBUG("i915: Enabling deep RC6\n");
+			rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
+		}
+
+		if (rc6_mode & INTEL_RC6pp_ENABLE) {
+			DRM_DEBUG("i915: Enabling deepest RC6\n");
+			rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
+		}
+	}
 
 	I915_WRITE(GEN6_RC_CONTROL,
 		   rc6_mask |
-- 
1.7.8.4

[PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Eugeni Dodonov]

This is yet another chapter in the ongoing saga of bringing RC6 to Sandy
Bridge machines by default.

Now that we have discovered that RC6 issues are triggered by RC6+ state,
let's try to disable it by default. Plain RC6 is the one responsible for
most energy savings, and so far it haven't given any problems - at least,
none we are aware of.

So with this, when i915_enable_rc6=-1 (e.g., the default value), we'll
attempt to enable plain RC6 only on SNB. For Ivy Bridge, the behavior
stays the same as always - we enable both RC6 and deep RC6.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d636fc2..a1e3329 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7925,11 +7925,11 @@ static int intel_enable_rc6(struct drm_device *dev)
 	 * Disable rc6 on Sandybridge
 	 */
 	if (INTEL_INFO(dev)->gen == 6) {
-		DRM_DEBUG_DRIVER("Sandybridge: RC6 disabled\n");
-		return 0;
+		DRM_DEBUG_DRIVER("Sandybridge: deep RC6 disabled\n");
+		return INTEL_RC6_ENABLE;
 	}
-	DRM_DEBUG_DRIVER("RC6 enabled\n");
-	return 1;
+	DRM_DEBUG_DRIVER("RC6 and deep RC6 enabled\n");
+	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
 void gen6_enable_rps(struct drm_i915_private *dev_priv)
-- 
1.7.8.4

Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter

[Chris Wilson]

On Sat, 11 Feb 2012 10:34:14 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This allows to select which rc6 modes are to be used via kernel parameter,
> via a bitmask parameter. E.g.:
> 
> - to enable rc6, i915_enable_rc6=1
> - to enable rc6 and deep rc6, i915_enable_rc6=3
> - to enable rc6 and deepest rc6, use i915_enable_rc6=5
> - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
> 
> Please keep in mind that the deepest RC6 state really should NOT be used
> by default, as it could potentially worsen the issues with deep RC6. So do
> enable it only when you know what you are doing. However, having it around
> could help solving possible future rc6-related issues and their debugging
> on user machines.
> 
> Note that this changes behavior - previously, value of 1 would enable both
> RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
> stages must be enabled manually.
> 
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>

A couple of nits, but the code does what it says on the tin. I was
going to give an r-b, but those nits seem to be growing...

> +/* RC6 modes */
If you're going to put a comment here, at least put a descriptive
comment. What is an rc mode, why should I care about the differences,
when should it be used, by whom and perhaps how?
> +#define INTEL_RC6_ENABLE			(1<<0)
> +#define INTEL_RC6p_ENABLE			(1<<1)
> +#define INTEL_RC6pp_ENABLE			(1<<2)
> +


> -	if (intel_enable_rc6(dev_priv->dev))
> -		rc6_mask = GEN6_RC_CTL_RC6p_ENABLE |
> -			GEN6_RC_CTL_RC6_ENABLE;
> +	rc6_mode = intel_enable_rc6(dev_priv->dev);
> +	/* Are we enabling rc6? */
/* This comment is pure fluff. Don't tell me what, tell me why, or rm! */
> +	if (rc6_mode > 0) {
I'm a little uneasy mixing signs and bitmasks, and this test looks a
little pointless as intel_enable_rc6 now returns the bitmask. Kill it
and we kill one level of indentation!
> +		if (rc6_mode & INTEL_RC6_ENABLE) {
> +			DRM_DEBUG("i915: Enabling RC6\n");
No need to include i915 here, it will be added by DRM_DEBUG. I think
these are worthy of being INFO, if you can tidy them up into a single
string and further prettify them.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Chris Wilson]

On Sat, 11 Feb 2012 10:34:15 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This is yet another chapter in the ongoing saga of bringing RC6 to Sandy
> Bridge machines by default.
> 
> Now that we have discovered that RC6 issues are triggered by RC6+ state,
> let's try to disable it by default. Plain RC6 is the one responsible for
> most energy savings, and so far it haven't given any problems - at least,
> none we are aware of.
> 
> So with this, when i915_enable_rc6=-1 (e.g., the default value), we'll
> attempt to enable plain RC6 only on SNB. For Ivy Bridge, the behavior
> stays the same as always - we enable both RC6 and deep RC6.

We appear to be lacking a bunch of tested-bys for what we proclaim to
have been tested.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter

[Kai Krakow]

Chris Wilson <chris@chris-wilson.co.uk> schrieb:

>> +     if (rc6_mode > 0) {
> I'm a little uneasy mixing signs and bitmasks, and this test looks a
> little pointless as intel_enable_rc6 now returns the bitmask. Kill it
> and we kill one level of indentation!

I think not, because Eugeni documented "-1" as the chipset default...

Just my 2 cents...

Regards,
Kai

Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter

[Eugeni Dodonov]

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

On Sat, Feb 11, 2012 at 16:13, Kai Krakow <hurikhan77@gmail.com> wrote:

> Chris Wilson <chris@chris-wilson.co.uk> schrieb:
>
> >> +     if (rc6_mode > 0) {
> > I'm a little uneasy mixing signs and bitmasks, and this test looks a
> > little pointless as intel_enable_rc6 now returns the bitmask. Kill it
> > and we kill one level of indentation!
>
> I think not, because Eugeni documented "-1" as the chipset default...
>

Not quite, Chris is right, we can get rid of this if block. Chipset default
values are being handled by the intel_enable_rc6(), that function returns a
valid bitmask now. So it will always return some meaningful values to us
and explicitly say if RC6 should be disabled or enabled (and if enabled,
which states are active).

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

[-- Attachment #1.2: Type: text/html, Size: 1223 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

[PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Eugeni Dodonov]

This allows to select which rc6 modes are to be used via kernel parameter,
via a bitmask parameter. E.g.:

- to enable rc6, i915_enable_rc6=1
- to enable rc6 and deep rc6, i915_enable_rc6=3
- to enable rc6 and deepest rc6, use i915_enable_rc6=5
- to enable rc6, deep and deepest rc6, use i915_enable_rc6=7

Please keep in mind that the deepest RC6 state really should NOT be used
by default, as it could potentially worsen the issues with deep RC6. So do
enable it only when you know what you are doing. However, having it around
could help solving possible future rc6-related issues and their debugging
on user machines.

Note that this changes behavior - previously, value of 1 would enable both
RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
stages must be enabled manually.

v2: address Chris Wilson comments and clean up the code.

Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
 drivers/gpu/drm/i915/i915_drv.h      |   21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++++----
 3 files changed, 42 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index a1103fc..b7a91db 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -66,7 +66,11 @@ MODULE_PARM_DESC(semaphores,
 int i915_enable_rc6 __read_mostly = -1;
 module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
 MODULE_PARM_DESC(i915_enable_rc6,
-		"Enable power-saving render C-state 6 (default: -1 (use per-chip default)");
+		"Enable power-saving render C-state 6. "
+		"Different stages can be selected via bitmask values "
+		"(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 = enable deepest rc6). "
+		"For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. "
+		"default: -1 (use per-chip default)");
 
 int i915_enable_fbc __read_mostly = -1;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 554bef7..c17bccf 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -997,6 +997,27 @@ struct drm_i915_file_private {
 
 #include "i915_trace.h"
 
+/**
+ * RC6 is a special power stage which allows the GPU to enter an very
+ * low-voltage mode when idle, using down to 0V while at this stage.  This
+ * stage is entered automatically when the GPU is idle when RC6 support is
+ * enabled, and as soon as new workload arises GPU wakes up automatically as well.
+ *
+ * There are different RC6 modes available in Intel GPU, which differentiate
+ * among each other with the latency required to enter and leave RC6 and
+ * voltage consumed by the GPU in different states.
+ *
+ * The combination of the following flags define which states GPU is allowed
+ * to enter, while RC6 is the normal RC6 state, RC6p is the deep RC6, and
+ * RC6pp is deepest RC6. Their support by hardware varies according to the
+ * GPU, BIOS, chipset and platform. RC6 is usually the safest one and the one
+ * which brings the most power savings; deeper states save more power, but
+ * require higher latency to switch to and wake up.
+ */
+#define INTEL_RC6_ENABLE			(1<<0)
+#define INTEL_RC6p_ENABLE			(1<<1)
+#define INTEL_RC6pp_ENABLE			(1<<2)
+
 extern struct drm_ioctl_desc i915_ioctls[];
 extern int i915_max_ioctl;
 extern unsigned int i915_fbpercrtc __always_unused;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index daa5743..d4abb73 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -7907,7 +7907,7 @@ void intel_init_emon(struct drm_device *dev)
 	dev_priv->corr = (lcfuse & LCFUSE_HIV_MASK);
 }
 
-static bool intel_enable_rc6(struct drm_device *dev)
+static int intel_enable_rc6(struct drm_device *dev)
 {
 	/*
 	 * Respect the kernel parameter if it is set
@@ -7938,6 +7938,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	u32 gt_perf_status = I915_READ(GEN6_GT_PERF_STATUS);
 	u32 pcu_mbox, rc6_mask = 0;
 	int cur_freq, min_freq, max_freq;
+	int rc6_mode;
 	int i;
 
 	/* Here begins a magic sequence of register writes to enable
@@ -7968,9 +7969,20 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	I915_WRITE(GEN6_RC6p_THRESHOLD, 100000);
 	I915_WRITE(GEN6_RC6pp_THRESHOLD, 64000); /* unused */
 
-	if (intel_enable_rc6(dev_priv->dev))
-		rc6_mask = GEN6_RC_CTL_RC6p_ENABLE |
-			GEN6_RC_CTL_RC6_ENABLE;
+	rc6_mode = intel_enable_rc6(dev_priv->dev);
+	if (rc6_mode & INTEL_RC6_ENABLE)
+		rc6_mask |= GEN6_RC_CTL_RC6_ENABLE;
+
+	if (rc6_mode & INTEL_RC6p_ENABLE)
+		rc6_mask |= GEN6_RC_CTL_RC6p_ENABLE;
+
+	if (rc6_mode & INTEL_RC6pp_ENABLE)
+		rc6_mask |= GEN6_RC_CTL_RC6pp_ENABLE;
+
+	DRM_INFO("Enabling RC6 states: RC6 %s, RC6p %s, RC6pp %s\n",
+			(rc6_mode & INTEL_RC6_ENABLE) ? "on" : "off",
+			(rc6_mode & INTEL_RC6p_ENABLE) ? "on" : "off",
+			(rc6_mode & INTEL_RC6pp_ENABLE) ? "on" : "off");
 
 	I915_WRITE(GEN6_RC_CONTROL,
 		   rc6_mask |
-- 
1.7.8.4

Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Eugeni Dodonov]

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

On Sat, Feb 11, 2012 at 10:59, Chris Wilson <chris@chris-wilson.co.uk>wrote:

> On Sat, 11 Feb 2012 10:34:15 -0200, Eugeni Dodonov <
> eugeni.dodonov@intel.com> wrote:
> > This is yet another chapter in the ongoing saga of bringing RC6 to Sandy
> > Bridge machines by default.
> >
> > Now that we have discovered that RC6 issues are triggered by RC6+ state,
> > let's try to disable it by default. Plain RC6 is the one responsible for
> > most energy savings, and so far it haven't given any problems - at least,
> > none we are aware of.
> >
> > So with this, when i915_enable_rc6=-1 (e.g., the default value), we'll
> > attempt to enable plain RC6 only on SNB. For Ivy Bridge, the behavior
> > stays the same as always - we enable both RC6 and deep RC6.
>
> We appear to be lacking a bunch of tested-bys for what we proclaim to
> have been tested.
>

So far I had 2 reports saying that RC6 solved the problems which were
present with RC6+RC6p (previous default) on SNB.

I'll wait a bit more and send a v2 version of this patch with all the
Tested-by's. Indeed, it makes sense to have those on this patch before
picking it into any kernel tree. Meanwhile, if there are any other
volunteers who were affected by SNB RC6 voodoo in the past and are willing
to give these patches a try, please, do so!

(Not that it wouldn't prevent all the possible issues, but it would certify
that it is not a placebo effect at least for someone...)

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

[-- Attachment #1.2: Type: text/html, Size: 1974 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 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Chris Wilson]

On Sat, 11 Feb 2012 17:23:32 -0200, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This allows to select which rc6 modes are to be used via kernel parameter,
> via a bitmask parameter. E.g.:
> 
> - to enable rc6, i915_enable_rc6=1
> - to enable rc6 and deep rc6, i915_enable_rc6=3
> - to enable rc6 and deepest rc6, use i915_enable_rc6=5
> - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
> 
> Please keep in mind that the deepest RC6 state really should NOT be used
> by default, as it could potentially worsen the issues with deep RC6. So do
> enable it only when you know what you are doing. However, having it around
> could help solving possible future rc6-related issues and their debugging
> on user machines.
> 
> Note that this changes behavior - previously, value of 1 would enable both
> RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
> stages must be enabled manually.
> 
> v2: address Chris Wilson comments and clean up the code.

Wow, that comment is superb! Definitely a standard we should try to
achieve everywhere.

> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
References: https://bugs.freedesktop.org/show_bug.cgi?id=42579
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Kai Krakow]

Eugeni Dodonov <eugeni@dodonov.net> schrieb:

> So far I had 2 reports saying that RC6 solved the problems which were
> present with RC6+RC6p (previous default) on SNB.

I was probably one of the early reporters... After testing a while, 
including video and 3D, I probably can safely say: The new default works 
rock solid for me.

Thank you.

Regards,
Kai

Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Keith Packard]

<#part sign=pgpmime>
On Sat, 11 Feb 2012 17:24:16 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote:

> So far I had 2 reports saying that RC6 solved the problems which were
> present with RC6+RC6p (previous default) on SNB.

For -fixes, I think we shouldn't enable rc6 by default, but we may want
to change what enabling rc6 does on SNB to stop using deeper states?
That should let more people enable it manually at least.

-- 
keith.packard@intel.com

Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Ben Widawsky]

On 02/11/2012 08:23 PM, Eugeni Dodonov wrote:
> This allows to select which rc6 modes are to be used via kernel parameter,
> via a bitmask parameter. E.g.:
> 
> - to enable rc6, i915_enable_rc6=1
> - to enable rc6 and deep rc6, i915_enable_rc6=3
> - to enable rc6 and deepest rc6, use i915_enable_rc6=5
> - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
> 
> Please keep in mind that the deepest RC6 state really should NOT be used
> by default, as it could potentially worsen the issues with deep RC6. So do
> enable it only when you know what you are doing. However, having it around
> could help solving possible future rc6-related issues and their debugging
> on user machines.
> 
> Note that this changes behavior - previously, value of 1 would enable both
> RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
> stages must be enabled manually.
> 
> v2: address Chris Wilson comments and clean up the code.
> 
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
>  drivers/gpu/drm/i915/i915_drv.h      |   21 +++++++++++++++++++++
>  drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++++----
>  3 files changed, 42 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index a1103fc..b7a91db 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -66,7 +66,11 @@ MODULE_PARM_DESC(semaphores,
>  int i915_enable_rc6 __read_mostly = -1;
>  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>  MODULE_PARM_DESC(i915_enable_rc6,
> -		"Enable power-saving render C-state 6 (default: -1 (use per-chip default)");
> +		"Enable power-saving render C-state 6. "
> +		"Different stages can be selected via bitmask values "
> +		"(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 = enable deepest rc6). "
> +		"For example, 3 would enable rc6 and deep rc6, and 7 would enable everything. "
> +		"default: -1 (use per-chip default)");
>  
>  int i915_enable_fbc __read_mostly = -1;
>  module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 554bef7..c17bccf 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -997,6 +997,27 @@ struct drm_i915_file_private {
>  
>  #include "i915_trace.h"
>  
> +/**
> + * RC6 is a special power stage which allows the GPU to enter an very
> + * low-voltage mode when idle, using down to 0V while at this stage.  This
> + * stage is entered automatically when the GPU is idle when RC6 support is
> + * enabled, and as soon as new workload arises GPU wakes up automatically as well.
> + *
> + * There are different RC6 modes available in Intel GPU, which differentiate
> + * among each other with the latency required to enter and leave RC6 and
> + * voltage consumed by the GPU in different states.
> + *
> + * The combination of the following flags define which states GPU is allowed
> + * to enter, while RC6 is the normal RC6 state, RC6p is the deep RC6, and
> + * RC6pp is deepest RC6. Their support by hardware varies according to the
> + * GPU, BIOS, chipset and platform. RC6 is usually the safest one and the one
> + * which brings the most power savings; deeper states save more power, but
> + * require higher latency to switch to and wake up.
> + */
> +#define INTEL_RC6_ENABLE			(1<<0)
> +#define INTEL_RC6p_ENABLE			(1<<1)
> +#define INTEL_RC6pp_ENABLE			(1<<2)
> +
>  extern struct drm_ioctl_desc i915_ioctls[];
>  extern int i915_max_ioctl;
>  extern unsigned int i915_fbpercrtc __always_unused;

Does it ever make sense to enable a deeper state without enabling the
one before it? Ie. would we ever enable RC6p and not RC6? Maybe we
don't want to have discrete bitfields and instead do:
> +#define INTEL_RC6_ENABLE			(1<<0)
> +#define INTEL_RC6p_ENABLE			(1<<1) | INTEL_RC6_ENABLE
> +#define INTEL_RC6pp_ENABLE			(1<<2) | INTEL_RC6p_ENABLE

Just a thought for the sake of hearing myself speak.

Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Eugeni Dodonov]

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

On Feb 12, 2012 6:53 PM, "Ben Widawsky" <ben@bwidawsk.net> wrote:
>
> On 02/11/2012 08:23 PM, Eugeni Dodonov wrote:
> > This allows to select which rc6 modes are to be used via kernel
parameter,
> > via a bitmask parameter. E.g.:
> >
> > - to enable rc6, i915_enable_rc6=1
> > - to enable rc6 and deep rc6, i915_enable_rc6=3
> > - to enable rc6 and deepest rc6, use i915_enable_rc6=5
> > - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
> >
> > Please keep in mind that the deepest RC6 state really should NOT be used
> > by default, as it could potentially worsen the issues with deep RC6. So
do
> > enable it only when you know what you are doing. However, having it
around
> > could help solving possible future rc6-related issues and their
debugging
> > on user machines.
> >
> > Note that this changes behavior - previously, value of 1 would enable
both
> > RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
> > stages must be enabled manually.
> >
> > v2: address Chris Wilson comments and clean up the code.
> >
> > Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
> > ---
> >  drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
> >  drivers/gpu/drm/i915/i915_drv.h      |   21 +++++++++++++++++++++
> >  drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++++----
> >  3 files changed, 42 insertions(+), 5 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
b/drivers/gpu/drm/i915/i915_drv.c
> > index a1103fc..b7a91db 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.c
> > +++ b/drivers/gpu/drm/i915/i915_drv.c
> > @@ -66,7 +66,11 @@ MODULE_PARM_DESC(semaphores,
> >  int i915_enable_rc6 __read_mostly = -1;
> >  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
> >  MODULE_PARM_DESC(i915_enable_rc6,
> > -             "Enable power-saving render C-state 6 (default: -1 (use
per-chip default)");
> > +             "Enable power-saving render C-state 6. "
> > +             "Different stages can be selected via bitmask values "
> > +             "(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4 =
enable deepest rc6). "
> > +             "For example, 3 would enable rc6 and deep rc6, and 7
would enable everything. "
> > +             "default: -1 (use per-chip default)");
> >
> >  int i915_enable_fbc __read_mostly = -1;
> >  module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
b/drivers/gpu/drm/i915/i915_drv.h
> > index 554bef7..c17bccf 100644
> > --- a/drivers/gpu/drm/i915/i915_drv.h
> > +++ b/drivers/gpu/drm/i915/i915_drv.h
> > @@ -997,6 +997,27 @@ struct drm_i915_file_private {
> >
> >  #include "i915_trace.h"
> >
> > +/**
> > + * RC6 is a special power stage which allows the GPU to enter an very
> > + * low-voltage mode when idle, using down to 0V while at this stage.
 This
> > + * stage is entered automatically when the GPU is idle when RC6
support is
> > + * enabled, and as soon as new workload arises GPU wakes up
automatically as well.
> > + *
> > + * There are different RC6 modes available in Intel GPU, which
differentiate
> > + * among each other with the latency required to enter and leave RC6
and
> > + * voltage consumed by the GPU in different states.
> > + *
> > + * The combination of the following flags define which states GPU is
allowed
> > + * to enter, while RC6 is the normal RC6 state, RC6p is the deep RC6,
and
> > + * RC6pp is deepest RC6. Their support by hardware varies according to
the
> > + * GPU, BIOS, chipset and platform. RC6 is usually the safest one and
the one
> > + * which brings the most power savings; deeper states save more power,
but
> > + * require higher latency to switch to and wake up.
> > + */
> > +#define INTEL_RC6_ENABLE                     (1<<0)
> > +#define INTEL_RC6p_ENABLE                    (1<<1)
> > +#define INTEL_RC6pp_ENABLE                   (1<<2)
> > +
> >  extern struct drm_ioctl_desc i915_ioctls[];
> >  extern int i915_max_ioctl;
> >  extern unsigned int i915_fbpercrtc __always_unused;
>
> Does it ever make sense to enable a deeper state without enabling the
> one before it? Ie. would we ever enable RC6p and not RC6? Maybe we
> don't want to have discrete bitfields and instead do:
> > +#define INTEL_RC6_ENABLE                     (1<<0)
> > +#define INTEL_RC6p_ENABLE                    (1<<1) | INTEL_RC6_ENABLE
> > +#define INTEL_RC6pp_ENABLE                   (1<<2) | INTEL_RC6p_ENABLE
>
> Just a thought for the sake of hearing myself speak.

We checked with Jesse, and these states are independent.. So it should be
possible to have RC6p and not plain RC6.

[-- Attachment #1.2: Type: text/html, Size: 5770 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: enable plain RC6 on Sandy Bridge by default

[Eugeni Dodonov]

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

On Sun, Feb 12, 2012 at 18:10, Keith Packard <keithp@keithp.com> wrote:

> <#part sign=pgpmime>
> On Sat, 11 Feb 2012 17:24:16 -0200, Eugeni Dodonov <eugeni@dodonov.net>
> wrote:
>
> > So far I had 2 reports saying that RC6 solved the problems which were
> > present with RC6+RC6p (previous default) on SNB.
>
> For -fixes, I think we shouldn't enable rc6 by default, but we may want
> to change what enabling rc6 does on SNB to stop using deeper states?
> That should let more people enable it manually at least.
>

Hi Keith,

what if we pick only the 1st patch in this series for -fixes? It won't
change the defaults in any way, but it will allow the ones willing to
enable it manually on SNB to prevent issues.

E.g., with this patch, passing i915_enable_rc6=1 will enable plain RC6, and
leave deep RC6 off. This is probably the safest setting so far - it results
in nice power savings and all the other benefits of RC6, but avoid entering
RC6+ - which seems to be responsible for all the issues on the affected
machines apparently. And it would further help to isolate which rc6 states
are the guilty ones on the affected machines, so in case any other problem
arises, we can ask people to test with different 'i915_enable_rc6=N'
parameter prior to fully reverting it again..

The change of the default value for SNB could go into -next kernel. By the
time we'll get to the first release of 3.4 we'll certainly know if there
are any issues with that.

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

[-- Attachment #1.2: Type: text/html, Size: 1953 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 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Ben Widawsky]

On 02/13/2012 12:16 AM, Eugeni Dodonov wrote:
> On Feb 12, 2012 6:53 PM, "Ben Widawsky" <ben@bwidawsk.net
> <mailto:ben@bwidawsk.net>> wrote:
>>
>> On 02/11/2012 08:23 PM, Eugeni Dodonov wrote:
>> > This allows to select which rc6 modes are to be used via kernel
> parameter,
>> > via a bitmask parameter. E.g.:
>> >
>> > - to enable rc6, i915_enable_rc6=1
>> > - to enable rc6 and deep rc6, i915_enable_rc6=3
>> > - to enable rc6 and deepest rc6, use i915_enable_rc6=5
>> > - to enable rc6, deep and deepest rc6, use i915_enable_rc6=7
>> >
>> > Please keep in mind that the deepest RC6 state really should NOT be used
>> > by default, as it could potentially worsen the issues with deep RC6.
> So do
>> > enable it only when you know what you are doing. However, having it
> around
>> > could help solving possible future rc6-related issues and their
> debugging
>> > on user machines.
>> >
>> > Note that this changes behavior - previously, value of 1 would
> enable both
>> > RC6 and deep RC6. Now it should only enable RC6 and deep/deepest RC6
>> > stages must be enabled manually.
>> >
>> > v2: address Chris Wilson comments and clean up the code.
>> >
>> > Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com
> <mailto:eugeni.dodonov@intel.com>>
>> > ---
>> >  drivers/gpu/drm/i915/i915_drv.c      |    6 +++++-
>> >  drivers/gpu/drm/i915/i915_drv.h      |   21 +++++++++++++++++++++
>> >  drivers/gpu/drm/i915/intel_display.c |   20 ++++++++++++++++----
>> >  3 files changed, 42 insertions(+), 5 deletions(-)
>> >
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.c
> b/drivers/gpu/drm/i915/i915_drv.c
>> > index a1103fc..b7a91db 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.c
>> > +++ b/drivers/gpu/drm/i915/i915_drv.c
>> > @@ -66,7 +66,11 @@ MODULE_PARM_DESC(semaphores,
>> >  int i915_enable_rc6 __read_mostly = -1;
>> >  module_param_named(i915_enable_rc6, i915_enable_rc6, int, 0600);
>> >  MODULE_PARM_DESC(i915_enable_rc6,
>> > -             "Enable power-saving render C-state 6 (default: -1
> (use per-chip default)");
>> > +             "Enable power-saving render C-state 6. "
>> > +             "Different stages can be selected via bitmask values "
>> > +             "(0 = disable; 1 = enable rc6; 2 = enable deep rc6; 4
> = enable deepest rc6). "
>> > +             "For example, 3 would enable rc6 and deep rc6, and 7
> would enable everything. "
>> > +             "default: -1 (use per-chip default)");
>> >
>> >  int i915_enable_fbc __read_mostly = -1;
>> >  module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
>> > diff --git a/drivers/gpu/drm/i915/i915_drv.h
> b/drivers/gpu/drm/i915/i915_drv.h
>> > index 554bef7..c17bccf 100644
>> > --- a/drivers/gpu/drm/i915/i915_drv.h
>> > +++ b/drivers/gpu/drm/i915/i915_drv.h
>> > @@ -997,6 +997,27 @@ struct drm_i915_file_private {
>> >
>> >  #include "i915_trace.h"
>> >
>> > +/**
>> > + * RC6 is a special power stage which allows the GPU to enter an very
>> > + * low-voltage mode when idle, using down to 0V while at this
> stage.  This
>> > + * stage is entered automatically when the GPU is idle when RC6
> support is
>> > + * enabled, and as soon as new workload arises GPU wakes up
> automatically as well.
>> > + *
>> > + * There are different RC6 modes available in Intel GPU, which
> differentiate
>> > + * among each other with the latency required to enter and leave
> RC6 and
>> > + * voltage consumed by the GPU in different states.
>> > + *
>> > + * The combination of the following flags define which states GPU
> is allowed
>> > + * to enter, while RC6 is the normal RC6 state, RC6p is the deep
> RC6, and
>> > + * RC6pp is deepest RC6. Their support by hardware varies according
> to the
>> > + * GPU, BIOS, chipset and platform. RC6 is usually the safest one
> and the one
>> > + * which brings the most power savings; deeper states save more
> power, but
>> > + * require higher latency to switch to and wake up.
>> > + */
>> > +#define INTEL_RC6_ENABLE                     (1<<0)
>> > +#define INTEL_RC6p_ENABLE                    (1<<1)
>> > +#define INTEL_RC6pp_ENABLE                   (1<<2)
>> > +
>> >  extern struct drm_ioctl_desc i915_ioctls[];
>> >  extern int i915_max_ioctl;
>> >  extern unsigned int i915_fbpercrtc __always_unused;
>>
>> Does it ever make sense to enable a deeper state without enabling the
>> one before it? Ie. would we ever enable RC6p and not RC6? Maybe we
>> don't want to have discrete bitfields and instead do:
>> > +#define INTEL_RC6_ENABLE                     (1<<0)
>> > +#define INTEL_RC6p_ENABLE                    (1<<1) | INTEL_RC6_ENABLE
>> > +#define INTEL_RC6pp_ENABLE                   (1<<2) | INTEL_RC6p_ENABLE
>>
>> Just a thought for the sake of hearing myself speak.
> 
> We checked with Jesse, and these states are independent.. So it should
> be possible to have RC6p and not plain RC6.

I'm sure it's possible, the question was, does it ever make sense?

Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter

[Eugeni Dodonov]

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

On Mon, Feb 13, 2012 at 06:33, Ben Widawsky <ben@bwidawsk.net> wrote:

>  > We checked with Jesse, and these states are independent.. So it should
> > be possible to have RC6p and not plain RC6.
>
> I'm sure it's possible, the question was, does it ever make sense?
>

One theory we had in mind for this is that the issue could be caused by
RC6, but avoided by more aggressive RC6 states. There was mostly motivated
by one case which said that deep rc6 was working while rc6+deep rc6 were
giving issues.

SNB with RC6 seems to be stable now, but perhaps it would be interesting to
leave this possibility, so in case we ever observe something similar on any
other GEN arch we'd have a quick way to debug.

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

[-- Attachment #1.2: Type: text/html, Size: 1098 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: enable plain RC6 on Sandy Bridge by default

[Keith Packard]

<#part sign=pgpmime>
On Sun, 12 Feb 2012 22:34:48 -0200, Eugeni Dodonov <eugeni@dodonov.net> wrote:

> what if we pick only the 1st patch in this series for -fixes? It won't
> change the defaults in any way, but it will allow the ones willing to
> enable it manually on SNB to prevent issues.

That seems like too big a change at this point in the release cycle. We
should be focused purely on fixing functionality bugs, not adding new
features. So, instead of adding the ability to individually control RC6
levels, we should just change what RC6 does on SNB when it is enabled,
then encourage people to test that and see if the shallowest RC6 states
actually work. That will provide sufficient information to know whether
it will be safe to turn that on by default in the next release, which is
what we really want.

The smallest change that gives us the data we need is what I'd like to have.

-- 
keith.packard@intel.com

Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Chris Wilson]

On Fri, 23 Mar 2012 11:57:19 -0300, Eugeni Dodonov <eugeni.dodonov@intel.com> wrote:
> This is yet another chapter in the ongoing saga of bringing RC6 to Sandy
> Bridge machines by default.
> 
> Now that we have discovered that RC6 issues are triggered by RC6+ state,
> let's try to disable it by default. Plain RC6 is the one responsible for
> most energy savings, and so far it haven't given any problems - at least,
> none we are aware of.
> 
> So with this, when i915_enable_rc6=-1 (e.g., the default value), we'll
> attempt to enable plain RC6 only on SNB. For Ivy Bridge, the behavior
> stays the same as always - we enable both RC6 and deep RC6.
> 
> Note that while this exact patch does not has explicit tested-by's, the
> equivalent settings were fixed in 3.3 kernel by a smaller patch. And it
> has also received considerable testing through Canonical RC6 task-force
> testing at https://wiki.ubuntu.com/Kernel/PowerManagementRC6. Up to date,
> it looks like all the known issues are gone.
> 
> v2: improve description and reference a couple of open bugs related to
> RC6 which seem to be fixed with this change.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=41682
> References: https://bugs.freedesktop.org/show_bug.cgi?id=38567
> References: https://bugs.freedesktop.org/show_bug.cgi?id=44867
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
Acked-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Eugeni Dodonov]

This is yet another chapter in the ongoing saga of bringing RC6 to Sandy
Bridge machines by default.

Now that we have discovered that RC6 issues are triggered by RC6+ state,
let's try to disable it by default. Plain RC6 is the one responsible for
most energy savings, and so far it haven't given any problems - at least,
none we are aware of.

So with this, when i915_enable_rc6=-1 (e.g., the default value), we'll
attempt to enable plain RC6 only on SNB. For Ivy Bridge, the behavior
stays the same as always - we enable both RC6 and deep RC6.

Note that while this exact patch does not has explicit tested-by's, the
equivalent settings were fixed in 3.3 kernel by a smaller patch. And it
has also received considerable testing through Canonical RC6 task-force
testing at https://wiki.ubuntu.com/Kernel/PowerManagementRC6. Up to date,
it looks like all the known issues are gone.

v2: improve description and reference a couple of open bugs related to
RC6 which seem to be fixed with this change.

References: https://bugs.freedesktop.org/show_bug.cgi?id=41682
References: https://bugs.freedesktop.org/show_bug.cgi?id=38567
References: https://bugs.freedesktop.org/show_bug.cgi?id=44867
Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c6d8bc4..dcae2c9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8233,11 +8233,11 @@ static int intel_enable_rc6(struct drm_device *dev)
 	 * Disable rc6 on Sandybridge
 	 */
 	if (INTEL_INFO(dev)->gen == 6) {
-		DRM_DEBUG_DRIVER("Sandybridge: RC6 disabled\n");
-		return 0;
+		DRM_DEBUG_DRIVER("Sandybridge: deep RC6 disabled\n");
+		return INTEL_RC6_ENABLE;
 	}
-	DRM_DEBUG_DRIVER("RC6 enabled\n");
-	return 1;
+	DRM_DEBUG_DRIVER("RC6 and deep RC6 enabled\n");
+	return (INTEL_RC6_ENABLE | INTEL_RC6p_ENABLE);
 }
 
 void gen6_enable_rps(struct drm_i915_private *dev_priv)
-- 
1.7.9.4

[PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default

[Luigi Forte]

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

Hi!
I think it's not just a placebo effect. I'm one of the users affected by
video corruption with rc6 enabled. Now I have used my pc for two days with
the patched kernel and everything seem working just fine. The glitches that
I had with the previous kernel versions were very noticeable and sometimes
covered more than half the screen, as I showed to Eugeni.
I can understand that after the previous experiences you'd like to move
slowly, but as much as I can tell, rc6 now is working as it should, so far.

Of course I will continue testing (using, actually) this version of the
kernel to provide info in case something goes wrong.
Cheers,
Luigi

[-- Attachment #1.2: Type: text/html, Size: 727 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