* [PATCH 0/2] Another chapter in RC6 saga... @ 2012-02-11 12:34 Eugeni Dodonov 2012-02-11 12:34 ` [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter Eugeni Dodonov 2012-02-11 12:34 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov 0 siblings, 2 replies; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 12:34 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 12:34 [PATCH 0/2] Another chapter in RC6 saga Eugeni Dodonov @ 2012-02-11 12:34 ` Eugeni Dodonov 2012-02-11 12:56 ` Chris Wilson 2012-02-11 12:34 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov 1 sibling, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 12:34 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 12:34 ` [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter Eugeni Dodonov @ 2012-02-11 12:56 ` Chris Wilson 2012-02-11 18:13 ` Kai Krakow 2012-02-11 19:23 ` [PATCH 1/1] " Eugeni Dodonov 0 siblings, 2 replies; 21+ messages in thread From: Chris Wilson @ 2012-02-11 12:56 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 12:56 ` Chris Wilson @ 2012-02-11 18:13 ` Kai Krakow 2012-02-11 19:09 ` Eugeni Dodonov 2012-02-11 19:23 ` [PATCH 1/1] " Eugeni Dodonov 1 sibling, 1 reply; 21+ messages in thread From: Kai Krakow @ 2012-02-11 18:13 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 18:13 ` Kai Krakow @ 2012-02-11 19:09 ` Eugeni Dodonov 0 siblings, 0 replies; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 19:09 UTC (permalink / raw) To: Kai Krakow; +Cc: intel-gfx [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 12:56 ` Chris Wilson 2012-02-11 18:13 ` Kai Krakow @ 2012-02-11 19:23 ` Eugeni Dodonov 2012-02-11 19:57 ` Chris Wilson 2012-02-12 20:52 ` Ben Widawsky 1 sibling, 2 replies; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 19:23 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 19:23 ` [PATCH 1/1] " Eugeni Dodonov @ 2012-02-11 19:57 ` Chris Wilson 2012-02-12 20:52 ` Ben Widawsky 1 sibling, 0 replies; 21+ messages in thread From: Chris Wilson @ 2012-02-11 19:57 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-11 19:23 ` [PATCH 1/1] " Eugeni Dodonov 2012-02-11 19:57 ` Chris Wilson @ 2012-02-12 20:52 ` Ben Widawsky 2012-02-12 23:16 ` Eugeni Dodonov 1 sibling, 1 reply; 21+ messages in thread From: Ben Widawsky @ 2012-02-12 20:52 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx 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. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-12 20:52 ` Ben Widawsky @ 2012-02-12 23:16 ` Eugeni Dodonov 2012-02-13 8:33 ` Ben Widawsky 0 siblings, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-12 23:16 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-12 23:16 ` Eugeni Dodonov @ 2012-02-13 8:33 ` Ben Widawsky 2012-02-13 11:37 ` Eugeni Dodonov 0 siblings, 1 reply; 21+ messages in thread From: Ben Widawsky @ 2012-02-13 8:33 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx, Eugeni Dodonov 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? ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/1] drm/i915: allow to select rc6 modes via kernel parameter 2012-02-13 8:33 ` Ben Widawsky @ 2012-02-13 11:37 ` Eugeni Dodonov 0 siblings, 0 replies; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-13 11:37 UTC (permalink / raw) To: Ben Widawsky; +Cc: intel-gfx, 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-11 12:34 [PATCH 0/2] Another chapter in RC6 saga Eugeni Dodonov 2012-02-11 12:34 ` [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter Eugeni Dodonov @ 2012-02-11 12:34 ` Eugeni Dodonov 2012-02-11 12:59 ` Chris Wilson 1 sibling, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 12:34 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-11 12:34 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov @ 2012-02-11 12:59 ` Chris Wilson 2012-02-11 19:24 ` Eugeni Dodonov 0 siblings, 1 reply; 21+ messages in thread From: Chris Wilson @ 2012-02-11 12:59 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-11 12:59 ` Chris Wilson @ 2012-02-11 19:24 ` Eugeni Dodonov 2012-02-12 10:16 ` Kai Krakow 2012-02-12 20:10 ` Keith Packard 0 siblings, 2 replies; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-11 19:24 UTC (permalink / raw) To: Chris Wilson; +Cc: intel-gfx, 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-11 19:24 ` Eugeni Dodonov @ 2012-02-12 10:16 ` Kai Krakow 2012-02-12 20:10 ` Keith Packard 1 sibling, 0 replies; 21+ messages in thread From: Kai Krakow @ 2012-02-12 10:16 UTC (permalink / raw) To: intel-gfx 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-11 19:24 ` Eugeni Dodonov 2012-02-12 10:16 ` Kai Krakow @ 2012-02-12 20:10 ` Keith Packard 2012-02-13 0:34 ` Eugeni Dodonov 1 sibling, 1 reply; 21+ messages in thread From: Keith Packard @ 2012-02-12 20:10 UTC (permalink / raw) To: Eugeni Dodonov, Chris Wilson; +Cc: intel-gfx, Eugeni Dodonov <#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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-12 20:10 ` Keith Packard @ 2012-02-13 0:34 ` Eugeni Dodonov 2012-02-14 4:01 ` Keith Packard 0 siblings, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-02-13 0:34 UTC (permalink / raw) To: Keith Packard; +Cc: intel-gfx, 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-02-13 0:34 ` Eugeni Dodonov @ 2012-02-14 4:01 ` Keith Packard 0 siblings, 0 replies; 21+ messages in thread From: Keith Packard @ 2012-02-14 4:01 UTC (permalink / raw) To: Eugeni Dodonov; +Cc: intel-gfx, Eugeni Dodonov <#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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default @ 2012-02-11 20:26 Luigi Forte 0 siblings, 0 replies; 21+ messages in thread From: Luigi Forte @ 2012-02-11 20:26 UTC (permalink / raw) To: intel-gfx [-- 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 0/2] RC6 changes for 3.4 got missing @ 2012-03-23 14:57 Eugeni Dodonov 2012-03-23 14:57 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov 0 siblings, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-03-23 14:57 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-03-23 14:57 [PATCH 0/2] RC6 changes for 3.4 got missing Eugeni Dodonov @ 2012-03-23 14:57 ` Eugeni Dodonov 2012-03-23 15:06 ` Chris Wilson 0 siblings, 1 reply; 21+ messages in thread From: Eugeni Dodonov @ 2012-03-23 14:57 UTC (permalink / raw) To: intel-gfx; +Cc: 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 ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default 2012-03-23 14:57 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov @ 2012-03-23 15:06 ` Chris Wilson 0 siblings, 0 replies; 21+ messages in thread From: Chris Wilson @ 2012-03-23 15:06 UTC (permalink / raw) To: intel-gfx; +Cc: Eugeni Dodonov 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 ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2012-03-23 15:06 UTC | newest] Thread overview: 21+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2012-02-11 12:34 [PATCH 0/2] Another chapter in RC6 saga Eugeni Dodonov 2012-02-11 12:34 ` [PATCH 1/2] drm/i915: allow to select rc6 modes via kernel parameter Eugeni Dodonov 2012-02-11 12:56 ` Chris Wilson 2012-02-11 18:13 ` Kai Krakow 2012-02-11 19:09 ` Eugeni Dodonov 2012-02-11 19:23 ` [PATCH 1/1] " Eugeni Dodonov 2012-02-11 19:57 ` Chris Wilson 2012-02-12 20:52 ` Ben Widawsky 2012-02-12 23:16 ` Eugeni Dodonov 2012-02-13 8:33 ` Ben Widawsky 2012-02-13 11:37 ` Eugeni Dodonov 2012-02-11 12:34 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov 2012-02-11 12:59 ` Chris Wilson 2012-02-11 19:24 ` Eugeni Dodonov 2012-02-12 10:16 ` Kai Krakow 2012-02-12 20:10 ` Keith Packard 2012-02-13 0:34 ` Eugeni Dodonov 2012-02-14 4:01 ` Keith Packard -- strict thread matches above, loose matches on Subject: below -- 2012-02-11 20:26 Luigi Forte 2012-03-23 14:57 [PATCH 0/2] RC6 changes for 3.4 got missing Eugeni Dodonov 2012-03-23 14:57 ` [PATCH 2/2] drm/i915: enable plain RC6 on Sandy Bridge by default Eugeni Dodonov 2012-03-23 15:06 ` Chris Wilson
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.