[Intel-gfx] [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 12:46 PM, Chris Wilson <chris at chris-wilson.co.uk> wrote:
> On Wed, Jul 17, 2013 at 12:36:12PM -0300, Rodrigo Vivi wrote:
>> i915_powersave was already an umbrella for disabling downclocking and fbc.
>> Now on it is extended to also force enabling them.
>> Also more powersavings features has been added under it: RC6 and IPS.
>>
>> In the future it can cover more powersavings features like
>> PSR, Slice Shutdown, etc. So this will be the easiest path to disable or
>> enable all of them together.
>>
>> Signed-off-by: Rodrigo Vivi <rodrigo.vivi at gmail.com>
>> ---
>>  drivers/gpu/drm/i915/i915_drv.c      |  9 +++++++--
>>  drivers/gpu/drm/i915/intel_display.c |  9 +++++----
>>  drivers/gpu/drm/i915/intel_pm.c      | 13 ++++++-------
>>  3 files changed, 18 insertions(+), 13 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
>> index b07362f..e4fb431 100644
>> --- a/drivers/gpu/drm/i915/i915_drv.c
>> +++ b/drivers/gpu/drm/i915/i915_drv.c
>> @@ -53,10 +53,15 @@ MODULE_PARM_DESC(panel_ignore_lid,
>>               "Override lid status (0=autodetect, 1=autodetect disabled [default], "
>>               "-1=force lid closed, -2=force lid open)");
>>
>> -unsigned int i915_powersave __read_mostly = 1;
>> +unsigned int i915_powersave __read_mostly = -1;
>>  module_param_named(powersave, i915_powersave, int, 0600);
>>  MODULE_PARM_DESC(powersave,
>> -             "Enable powersavings, fbc, downclocking, etc. (default: true)");
>> +             "Force Enable/Disable powersavings,"
>> +             "ignoring individual parameters when available."
>> +             "Features covered: downclocking, fbc, rc6 and ips"
>> +             "0 = forcibly disables all powersavings features"
>> +             "1 = forcibly enables all powersavings features"
>> +             "default: -1 (use per-feature default/parameter)");
>>
>>  int i915_semaphores __read_mostly = -1;
>>  module_param_named(semaphores, i915_semaphores, int, 0600);
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index f53359b..e84a7d9 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4089,7 +4089,8 @@ retry:
>>  static void hsw_compute_ips_config(struct intel_crtc *crtc,
>>                                  struct intel_crtc_config *pipe_config)
>>  {
>> -     pipe_config->ips_enabled = i915_enable_ips &&
>> +     pipe_config->ips_enabled = i915_powersave != 0 &&
>> +                                (i915_powersave == 1 || i915_enable_ips) &&
>>                                  hsw_crtc_supports_ips(crtc) &&
>>                                  pipe_config->pipe_bpp == 24;
>>  }
>> @@ -4329,7 +4330,7 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>>
>>       crtc->lowfreq_avail = false;
>>       if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_LVDS) &&
>> -         reduced_clock && i915_powersave) {
>> +         reduced_clock && i915_powersave == 0) {
>
> Sense inverted.
!= I meant! stupid me! :P

>
>> @@ -452,9 +452,6 @@ void intel_update_fbc(struct drm_device *dev)
>>       struct drm_i915_gem_object *obj;
>>       unsigned int max_hdisplay, max_vdisplay;
>>
>> -     if (!i915_powersave)
>> -             return;
>> -
>>       if (!I915_HAS_FBC(dev))
>>               return;
>>
>> @@ -491,13 +488,13 @@ void intel_update_fbc(struct drm_device *dev)
>>       intel_fb = to_intel_framebuffer(fb);
>>       obj = intel_fb->obj;
>>
>> -     if (i915_enable_fbc < 0 &&
>> +     if (i915_enable_fbc < 0 && i915_powersave < 0 &&
>>           INTEL_INFO(dev)->gen <= 7 && !IS_HASWELL(dev)) {
>>               DRM_DEBUG_KMS("disabled per chip default\n");
>>               dev_priv->fbc.no_fbc_reason = FBC_CHIP_DEFAULT;
>>               goto out_disable;
>>       }
>> -     if (!i915_enable_fbc) {
>> +     if (!i915_enable_fbc || i915_powersave == 0) {
>
> This is unreadable. Split it up like intel_enable_rc6() and make it
> verbose.

Yes, I know.. even worse on ips case :(

One idea Paulo had is to change i915_enable_fbc based on
i915_powersave and put a debug message.

>
>>               DRM_DEBUG_KMS("fbc disabled per module param\n");
>>               dev_priv->fbc.no_fbc_reason = FBC_MODULE_PARAM;
>>               goto out_disable;
>> @@ -3171,12 +3168,14 @@ int intel_enable_rc6(const struct drm_device *dev)
>>       if (INTEL_INFO(dev)->gen < 5)
>>               return 0;
>>
>> -     /* Respect the kernel parameter if it is set */
>> +     /* Respect the kernel parameters if they are set */
>> +     if (i915_powersave == 0)
>> +             return 0;
>
> Semantics broken here. Force enabling a specific tunable such as rc6
> should override a default powersave.

As I said this is just a start point for discussion.

So if I understood correctly you suggest that if we have
i915_powersave=0 and i915_enable_rc6=1 we should enable rc6?
This is not how it is implemented nowadays on fbc right now... And
this lead me to use pwoesave as an umbrella for force disable/enable
ignoring individual parameters. But I'm open to do the other way
around as long as we have a standardized behavior.

So, we can either respect individual parameters when they are set or
completely ignore. In the way it is now it doesn't respect individual
fbc parameter for powersave=0.

>
>>       if (i915_enable_rc6 >= 0)
>>               return i915_enable_rc6;
>>
>>       /* Disable RC6 on Ironlake */
>> -     if (INTEL_INFO(dev)->gen == 5)
>> +     if (INTEL_INFO(dev)->gen == 5 && i915_powersave < 0)
>>               return 0;
>
> --
> Chris Wilson, Intel Open Source Technology Centre



--
Rodrigo Vivi
Blog: http://blog.vivi.eng.br