On 02/13/2012 12:16 AM, Eugeni Dodonov wrote: > On Feb 12, 2012 6:53 PM, "Ben Widawsky" <ben at bwidawsk.net > <mailto:ben at 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 at intel.com > <mailto:eugeni.dodonov at 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?
