[PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[Michel Alexandre Salim]

>From a90bf9ac2a40869242a79c88958c99dacc3da2a9 Mon Sep 17 00:00:00 2001
From: Michel Alexandre Salim <salimma@fedoraproject.org>
Date: Wed, 9 Nov 2011 14:18:45 +0100
Subject: [PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

Commit 72bbe58cd9568c7766cc219a779ea68a02132797 makes the check against
the quirk table unreachable if i915_panel_use_ssc is automatically set
to a non-zero value. This patch changes the tests so that unless
i915_panel_use_ssc is set to exactly 1, the quirk table is checked.

This ensures that on known quirky hardware, SSC is disabled unless
specifically enabled by the user.

Signed-off-by: Michel Alexandre Salim <salimma@fedoraproject.org>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 981b1f1..aa7b0c8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4660,10 +4660,13 @@ static void intel_update_watermarks(struct drm_device *dev)
 
 static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 {
+	if (i915_panel_use_ssc == 1)
+		return true;
+	if (dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE)
+		return false;
 	if (i915_panel_use_ssc >= 0)
 		return i915_panel_use_ssc != 0;
-	return dev_priv->lvds_use_ssc
-		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
+	return dev_priv->lvds_use_ssc;
 }
 
 /**
-- 
1.7.7

Re: [PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[Michel Alexandre Salim]

Additional note: while I've not touched the line since it does not
affect me, it seems that i915_panel_use_ssc *cannot* be less than 0
since that variable is declared as unsigned.

So the last line (the value in dev_priv) will never be used to determine
whether SSC is used anyway. Keith probably knows more since he
introduces that check in the commit that my patch referred to.

On 11/09/2011 05:12 PM, Michel Alexandre Salim wrote:
>  static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
>  {
> +	if (i915_panel_use_ssc == 1)
> +		return true;
> +	if (dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE)
> +		return false;
>  	if (i915_panel_use_ssc >= 0)
>  		return i915_panel_use_ssc != 0;
> -	return dev_priv->lvds_use_ssc
> -		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
> +	return dev_priv->lvds_use_ssc;
>  }
>  
>  /**


-- 
Michel Alexandre Salim
µblog:      http://identi.ca/hircus
            http://twitter.com/hircus
GPG key ID: 78884778

()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments

Re: [PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 2970 bytes --]

On Wed, 09 Nov 2011 17:30:29 +0100, Michel Alexandre Salim <salimma@fedoraproject.org> wrote:
> Additional note: while I've not touched the line since it does not
> affect me, it seems that i915_panel_use_ssc *cannot* be less than 0
> since that variable is declared as unsigned.

Oops. That's the bug here -- we're supposed to make it so that the
command line can override the quirks, but there's no way to use a quirk
given the mis-declared parameter.

This is untested...

From e64ecadef40e3c2035cd4e9b967ffd83489bdea0 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Wed, 9 Nov 2011 09:57:50 -0800
Subject: [PATCH] drm/i915: Module parameters using '-1' as default must be
 signed type

Testing i915_panel_use_ssc for the default value was broken, so the
driver would never autodetect the correct value.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.c |    4 ++--
 drivers/gpu/drm/i915/i915_drv.h |    4 ++--
 2 files changed, 4 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 548e04b..13488be 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -67,7 +67,7 @@ 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: true)");
 
-unsigned int i915_enable_fbc __read_mostly = -1;
+int i915_enable_fbc __read_mostly = -1;
 module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
 MODULE_PARM_DESC(i915_enable_fbc,
 		"Enable frame buffer compression for power savings "
@@ -79,7 +79,7 @@ MODULE_PARM_DESC(lvds_downclock,
 		"Use panel (LVDS/eDP) downclocking for power savings "
 		"(default: false)");
 
-unsigned int i915_panel_use_ssc __read_mostly = -1;
+int i915_panel_use_ssc __read_mostly = -1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
 MODULE_PARM_DESC(lvds_use_ssc,
 		"Use Spread Spectrum Clock with panels [LVDS/eDP] "
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index d2da91f..4a9c1b9 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1000,10 +1000,10 @@ extern int i915_panel_ignore_lid __read_mostly;
 extern unsigned int i915_powersave __read_mostly;
 extern unsigned int i915_semaphores __read_mostly;
 extern unsigned int i915_lvds_downclock __read_mostly;
-extern unsigned int i915_panel_use_ssc __read_mostly;
+extern int i915_panel_use_ssc __read_mostly;
 extern int i915_vbt_sdvo_panel_type __read_mostly;
 extern unsigned int i915_enable_rc6 __read_mostly;
-extern unsigned int i915_enable_fbc __read_mostly;
+extern int i915_enable_fbc __read_mostly;
 extern bool i915_enable_hangcheck __read_mostly;
 
 extern int i915_suspend(struct drm_device *dev, pm_message_t state);
-- 
1.7.7

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]

Re: [PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[Michel Alexandre Salim]

On Wed, 2011-11-09 at 10:07 -0800, Keith Packard wrote:
> On Wed, 09 Nov 2011 17:30:29 +0100, Michel Alexandre Salim <salimma@fedoraproject.org> wrote:
> > Additional note: while I've not touched the line since it does not
> > affect me, it seems that i915_panel_use_ssc *cannot* be less than 0
> > since that variable is declared as unsigned.
> 
> Oops. That's the bug here -- we're supposed to make it so that the
> command line can override the quirks, but there's no way to use a quirk
> given the mis-declared parameter.
Ah, now everything makes sense.
> 
> This is untested...
> 
Tested and it works fine:
- without extra parameter, the blacklist is used
- with i915.lvds_use_ssc=1, SSC use is enforced and the display turns
black on my unsupported hardware
- with i915.lvds_use_ssc=0, SSC is disabled

(ps the PGP/MIME signature made it hard to just extract the
git-formatted email; I ended up just editing the message by hand before
'git am')

> From e64ecadef40e3c2035cd4e9b967ffd83489bdea0 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp@keithp.com>
> Date: Wed, 9 Nov 2011 09:57:50 -0800
> Subject: [PATCH] drm/i915: Module parameters using '-1' as default must be
>  signed type
> 
> Testing i915_panel_use_ssc for the default value was broken, so the
> driver would never autodetect the correct value.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by:   Michel Alexandre Salim <salimma@fedoraproject.org>
Tested-by:     Michel Alexandre Salim <salimma@fedoraproject.org>

Should I send the patch that I applied with those added lines? Probably
not necessary.

Thanks,

-- 
Michel

Re: [PATCH v3] drm/i915: Honor SSC quirk table over the default, unless set by user

[Michel Alexandre Salim]

Hi Keith,

That patch is still not in 3.2-rc2, drm-intel-fixes or drm-intel-next.
I've been using it successfully on i915 (both SSC-blacklisted and not)
and non-i915 machines; feel free to set the Tested-by and Reviewed-by tags.

Thanks,

-- 
Michel

On 11/09/2011 07:07 PM, Keith Packard wrote:
> On Wed, 09 Nov 2011 17:30:29 +0100, Michel Alexandre Salim <salimma@fedoraproject.org> wrote:
>> Additional note: while I've not touched the line since it does not
>> affect me, it seems that i915_panel_use_ssc *cannot* be less than 0
>> since that variable is declared as unsigned.
> 
> Oops. That's the bug here -- we're supposed to make it so that the
> command line can override the quirks, but there's no way to use a quirk
> given the mis-declared parameter.
> 
> This is untested...
> 
> From e64ecadef40e3c2035cd4e9b967ffd83489bdea0 Mon Sep 17 00:00:00 2001
> From: Keith Packard <keithp@keithp.com>
> Date: Wed, 9 Nov 2011 09:57:50 -0800
> Subject: [PATCH] drm/i915: Module parameters using '-1' as default must be
>  signed type
> 
> Testing i915_panel_use_ssc for the default value was broken, so the
> driver would never autodetect the correct value.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/i915_drv.c |    4 ++--
>  drivers/gpu/drm/i915/i915_drv.h |    4 ++--
>  2 files changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
> index 548e04b..13488be 100644
> --- a/drivers/gpu/drm/i915/i915_drv.c
> +++ b/drivers/gpu/drm/i915/i915_drv.c
> @@ -67,7 +67,7 @@ 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: true)");
>  
> -unsigned int i915_enable_fbc __read_mostly = -1;
> +int i915_enable_fbc __read_mostly = -1;
>  module_param_named(i915_enable_fbc, i915_enable_fbc, int, 0600);
>  MODULE_PARM_DESC(i915_enable_fbc,
>  		"Enable frame buffer compression for power savings "
> @@ -79,7 +79,7 @@ MODULE_PARM_DESC(lvds_downclock,
>  		"Use panel (LVDS/eDP) downclocking for power savings "
>  		"(default: false)");
>  
> -unsigned int i915_panel_use_ssc __read_mostly = -1;
> +int i915_panel_use_ssc __read_mostly = -1;
>  module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
>  MODULE_PARM_DESC(lvds_use_ssc,
>  		"Use Spread Spectrum Clock with panels [LVDS/eDP] "
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index d2da91f..4a9c1b9 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -1000,10 +1000,10 @@ extern int i915_panel_ignore_lid __read_mostly;
>  extern unsigned int i915_powersave __read_mostly;
>  extern unsigned int i915_semaphores __read_mostly;
>  extern unsigned int i915_lvds_downclock __read_mostly;
> -extern unsigned int i915_panel_use_ssc __read_mostly;
> +extern int i915_panel_use_ssc __read_mostly;
>  extern int i915_vbt_sdvo_panel_type __read_mostly;
>  extern unsigned int i915_enable_rc6 __read_mostly;
> -extern unsigned int i915_enable_fbc __read_mostly;
> +extern int i915_enable_fbc __read_mostly;
>  extern bool i915_enable_hangcheck __read_mostly;
>  
>  extern int i915_suspend(struct drm_device *dev, pm_message_t state);
> 
> 
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx


-- 
Michel Alexandre Salim
µblog:      http://identi.ca/hircus
            http://twitter.com/hircus
GPG key ID: 78884778

()  ascii ribbon campaign - against html e-mail
/\  www.asciiribbon.org   - against proprietary attachments