[PATCH 0/2] RC6 changes for 3.4 got missing

[PATCH 0/2] RC6 changes for 3.4 got missing

[Eugeni Dodonov]

Hi,

Daniel Vetter and Chris Wilson noticed that the latest rc6 patches went missing
in the latest drm merge requests. So I updated them to the latest Linus' tree,
and improved the description a bit.

Those settings have received a huge testing on Ubuntu (thanks a lot
Canonical!!), and so far it looks like they solve all the known issues on SNB
with RC6 which were out there for the past months.

However, one can never be too sure, so the earlier we get them in the main
kernel, the earlier we'll find out if there are still some bugs in the wild
(with any luck, there won't be any). And with this, we'll complete the
lets-try-rc6-by-default-one-more-time saga.

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      |   21 +++++++++++++++++++++
 drivers/gpu/drm/i915/intel_display.c |   28 ++++++++++++++++++++--------
 3 files changed, 46 insertions(+), 9 deletions(-)

-- 
1.7.9.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.

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

References: https://bugs.freedesktop.org/show_bug.cgi?id=42579
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
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 0694e17..8af21c1 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 c0f19f5..2c19227 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1053,6 +1053,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 d514719..c6d8bc4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -8215,7 +8215,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
@@ -8247,6 +8247,7 @@ void gen6_enable_rps(struct drm_i915_private *dev_priv)
 	u32 pcu_mbox, rc6_mask = 0;
 	u32 gtfifodbg;
 	int cur_freq, min_freq, max_freq;
+	int rc6_mode;
 	int i;
 
 	/* Here begins a magic sequence of register writes to enable
@@ -8284,9 +8285,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_RC6_ENABLE |
-			((IS_GEN7(dev_priv->dev)) ? GEN6_RC_CTL_RC6p_ENABLE : 0);
+	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.9.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.

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

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

Re: [PATCH 0/2] RC6 changes for 3.4 got missing

[Daniel Vetter]

On Fri, Mar 23, 2012 at 11:57:17AM -0300, Eugeni Dodonov wrote:
> Hi,
> 
> Daniel Vetter and Chris Wilson noticed that the latest rc6 patches went missing
> in the latest drm merge requests. So I updated them to the latest Linus' tree,
> and improved the description a bit.

Yeah, sorry again I've somehow lost track of these 2 here for 3.4.  Both
patches are

Acked-by: Daniel Vetter <daniel.vetter@ffwll.ch>

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

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

[Ben Widawsky]

On Fri, 23 Mar 2012 11:57:18 -0300
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.
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=42579
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> Signed-off-by: Eugeni Dodonov <eugeni.dodonov@intel.com>
If it's not too late, this is
Reviewed-by: Ben Widawsky <benjamin.widawsky@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 0694e17..8af21c1 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 c0f19f5..2c19227 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1053,6 +1053,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 d514719..c6d8bc4 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -8215,7 +8215,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
> @@ -8247,6 +8247,7 @@ void gen6_enable_rps(struct drm_i915_private
> *dev_priv) u32 pcu_mbox, rc6_mask = 0;
>  	u32 gtfifodbg;
>  	int cur_freq, min_freq, max_freq;
> +	int rc6_mode;
>  	int i;
>  
>  	/* Here begins a magic sequence of register writes to enable
> @@ -8284,9 +8285,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_RC6_ENABLE |
> -			((IS_GEN7(dev_priv->dev)) ?
> GEN6_RC_CTL_RC6p_ENABLE : 0);
> +	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 |