[PATCH] drm/i915: Extend i915_powersave parameter.

[PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

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@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) {
 		I915_WRITE(FP1(pipe), fp2);
 		crtc->config.dpll_hw_state.fp1 = fp2;
 		crtc->lowfreq_avail = true;
@@ -7156,7 +7157,7 @@ void intel_mark_idle(struct drm_device *dev)
 {
 	struct drm_crtc *crtc;
 
-	if (!i915_powersave)
+	if (i915_powersave == 0)
 		return;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
@@ -7173,7 +7174,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
 	struct drm_device *dev = obj->base.dev;
 	struct drm_crtc *crtc;
 
-	if (!i915_powersave)
+	if (i915_powersave == 0)
 		return;
 
 	list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 43031ec..405b475 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -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) {
 		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;
 	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;
 
 	if (IS_HASWELL(dev)) {
-- 
1.8.1.4

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

when playing with fbc and psr I noticed that this flag was underused
so I decided to extend it a bit...
I know that the force enable made it more difficult to implement along
with individual parameters, but I sent this patch as a startup for the
discussion...

please let me know if you have better ideas...

On Wed, Jul 17, 2013 at 12:36 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> 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@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) {
>                 I915_WRITE(FP1(pipe), fp2);
>                 crtc->config.dpll_hw_state.fp1 = fp2;
>                 crtc->lowfreq_avail = true;
> @@ -7156,7 +7157,7 @@ void intel_mark_idle(struct drm_device *dev)
>  {
>         struct drm_crtc *crtc;
>
> -       if (!i915_powersave)
> +       if (i915_powersave == 0)
>                 return;
>
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> @@ -7173,7 +7174,7 @@ void intel_mark_fb_busy(struct drm_i915_gem_object *obj,
>         struct drm_device *dev = obj->base.dev;
>         struct drm_crtc *crtc;
>
> -       if (!i915_powersave)
> +       if (i915_powersave == 0)
>                 return;
>
>         list_for_each_entry(crtc, &dev->mode_config.crtc_list, head) {
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 43031ec..405b475 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -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) {
>                 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;
>         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;
>
>         if (IS_HASWELL(dev)) {
> --
> 1.8.1.4
>



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

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Chris Wilson]

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@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.

> @@ -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.

>  		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.

>  	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

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 12:46 PM, Chris Wilson <chris@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@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

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Chris Wilson]

On Wed, Jul 17, 2013 at 02:45:29PM -0300, Rodrigo Vivi wrote:
> 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.

Just depends on whether powersave=0 is default and powersave=-1 is
force off, with powersave=1 as force on.
 
> 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.

My opinion is that we respect the specific module parameters, and if
they are left to default values, then apply the global powersave
parameter. If that too is default, then we apply the module default.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Daniel Vetter]

On Wed, Jul 17, 2013 at 10:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Wed, Jul 17, 2013 at 02:45:29PM -0300, Rodrigo Vivi wrote:
>> 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.
>
> Just depends on whether powersave=0 is default and powersave=-1 is
> force off, with powersave=1 as force on.
>
>> 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.
>
> My opinion is that we respect the specific module parameters, and if
> they are left to default values, then apply the global powersave
> parameter. If that too is default, then we apply the module default.

Jumping in a bit late, but: I've honestly never understood why we have
two levels of module options. Imo having individual knobs for each
delicate feature makes more sense, strange dependencies in module
option will only confuse dim-witted developers like me when looking at
a bug ;-)

So could we just reduce powersave to the few things that we haven't
touched yet (iirc only DRRS)?
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 6:00 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 17, 2013 at 10:13 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
>> On Wed, Jul 17, 2013 at 02:45:29PM -0300, Rodrigo Vivi wrote:
>>> 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.

While you were writing my git send-email on v2 was failling because of
dam proxy...
But It's never late. And I think I agree with you... I just started
this discussion exactly because I got confused about this flag and
didn't like the way it was at fbc.

The only advantage that I see on this two levels is an easy way to
disable all powersaving features at once, mainly now it is increasing
the numbers...
instead of boot a kernel and manually put at grub:

i915.powersave=0 i915.i915_enable_fbc=0 i915.i915_enable_rc6=0
i915.enable_ips=0 i915.enable_psr=0 i915.enable_slice_shutdown=0

you just have to write
i915.powersave=0

>>
>> Just depends on whether powersave=0 is default and powersave=-1 is
>> force off, with powersave=1 as force on.
>>
>>> 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.

>>
>> My opinion is that we respect the specific module parameters, and if
>> they are left to default values, then apply the global powersave
>> parameter. If that too is default, then we apply the module default.
>
> Jumping in a bit late, but: I've honestly never understood why we have
> two levels of module options. Imo having individual knobs for each
> delicate feature makes more sense, strange dependencies in module
> option will only confuse dim-witted developers like me when looking at
> a bug ;-)
>
> So could we just reduce powersave to the few things that we haven't
> touched yet (iirc only DRRS)?

That is fine for me too... either add all features under this umbrella
or make it be only one feature like drrs that doesn't have its own
parameter...
the only cons I see in this case is the name of parameter that is too
generic...

But honestly I don't have a stronger position I just wanted to start
the discussion because I don't like the way it is today... So it is up
to you... I can either send v2 or a new simple patch that removes fbc
from this i915_powersave. Just let me know what is better...


> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



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

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Daniel Vetter]

On Wed, Jul 17, 2013 at 11:23 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>>> My opinion is that we respect the specific module parameters, and if
>>> they are left to default values, then apply the global powersave
>>> parameter. If that too is default, then we apply the module default.
>>
>> Jumping in a bit late, but: I've honestly never understood why we have
>> two levels of module options. Imo having individual knobs for each
>> delicate feature makes more sense, strange dependencies in module
>> option will only confuse dim-witted developers like me when looking at
>> a bug ;-)
>>
>> So could we just reduce powersave to the few things that we haven't
>> touched yet (iirc only DRRS)?
>
> That is fine for me too... either add all features under this umbrella
> or make it be only one feature like drrs that doesn't have its own
> parameter...
> the only cons I see in this case is the name of parameter that is too
> generic...

We're allowed to kill module options now, so if we fix up drrs and
make powersave completely useless, we can just remove.

> But honestly I don't have a stronger position I just wanted to start
> the discussion because I don't like the way it is today... So it is up
> to you... I can either send v2 or a new simple patch that removes fbc
> from this i915_powersave. Just let me know what is better...

I vote for moving fbc out of powersave as a separate option. Worst
case we need to educate a users and tell him that frobbing around with
random module options isn't a good idea ;-)
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH] drm/i915: Extend i915_powersave parameter.

[Rodrigo Vivi]

On Wed, Jul 17, 2013 at 6:30 PM, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Wed, Jul 17, 2013 at 11:23 PM, Rodrigo Vivi <rodrigo.vivi@gmail.com> wrote:
>>>> My opinion is that we respect the specific module parameters, and if
>>>> they are left to default values, then apply the global powersave
>>>> parameter. If that too is default, then we apply the module default.
>>>
>>> Jumping in a bit late, but: I've honestly never understood why we have
>>> two levels of module options. Imo having individual knobs for each
>>> delicate feature makes more sense, strange dependencies in module
>>> option will only confuse dim-witted developers like me when looking at
>>> a bug ;-)
>>>
>>> So could we just reduce powersave to the few things that we haven't
>>> touched yet (iirc only DRRS)?
>>
>> That is fine for me too... either add all features under this umbrella
>> or make it be only one feature like drrs that doesn't have its own
>> parameter...
>> the only cons I see in this case is the name of parameter that is too
>> generic...
>
> We're allowed to kill module options now, so if we fix up drrs and
> make powersave completely useless, we can just remove.

I'm in favor of remove powersave parameter completely. Just let me
know what are the issues with drrs and how I can help to fix them and
remove this parameter completely.

>
>> But honestly I don't have a stronger position I just wanted to start
>> the discussion because I don't like the way it is today... So it is up
>> to you... I can either send v2 or a new simple patch that removes fbc
>> from this i915_powersave. Just let me know what is better...
>
> I vote for moving fbc out of powersave as a separate option. Worst
> case we need to educate a users and tell him that frobbing around with
> random module options isn't a good idea ;-)

Agree

> -Daniel
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



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