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

[Ben Widawsky <ben@bwidawsk.net>]

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?