On Wed, Jul 17, 2013 at 12:46 PM, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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@xxxxxxxxx>
>> ---
>> 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
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
