[v2,12/22] drm/i915: Preserve SSC earlier
diff mbox

Message ID 88b49891e95ced43bfb57cc8472bb69985827153.1439288957.git.lukas@wunner.de
State New
Headers show

Commit Message

Lukas Wunner July 15, 2015, 11:57 a.m. UTC
Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
added code to intel_modeset_gem_init to override the SSC status read
from VBT with the SSC status set by BIOS.

However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
which calls intel_setup_outputs, which *modifies* SSC status by way of
intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
doesn't preserve the SSC status set by BIOS but whatever
intel_init_pch_refclk decided on.

This is a problem on dual gpu laptops such as the MacBook Pro which
require either a handler to switch DDC lines, or the discrete gpu
to proxy DDC/AUX communication: Both the handler and the discrete
gpu may initialize after the i915 driver, and consequently, an LVDS
connector may initially seem disconnected and the SSC therefore
is disabled by intel_init_pch_refclk, but on reprobe the connector
may turn out to be connected and the SSC must then be enabled.

Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
it is assumed BIOS disabled it while in fact it was disabled by
intel_init_pch_refclk.

Also, because the SSC status is preserved so late, the preserved value
only ever gets used on resume but not on panel initialization:
intel_modeset_init calls intel_init_display which indirectly calls
intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
is the sole user of dev_priv->vbt.lvds_use_ssc).

Fix this by moving the code introduced by 92122789b2d6 from
intel_modeset_gem_init to intel_modeset_init before the invocation
of intel_setup_outputs and intel_init_display.

Add a DRM_DEBUG_KMS as suggested way back by Jani:
http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html

Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
    [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
Tested-by: William Brown <william@blackhats.net.au>
    [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
Tested-by: Lukas Wunner <lukas@wunner.de>
    [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
    [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]

Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
Signed-off-by: Lukas Wunner <lukas@wunner.de>
---
 drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
 1 file changed, 18 insertions(+), 11 deletions(-)

Comments

Jesse Barnes Aug. 31, 2015, 8:23 p.m. UTC | #1
On 07/15/2015 04:57 AM, Lukas Wunner wrote:
> Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
> added code to intel_modeset_gem_init to override the SSC status read
> from VBT with the SSC status set by BIOS.
> 
> However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
> which calls intel_setup_outputs, which *modifies* SSC status by way of
> intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
> doesn't preserve the SSC status set by BIOS but whatever
> intel_init_pch_refclk decided on.
> 
> This is a problem on dual gpu laptops such as the MacBook Pro which
> require either a handler to switch DDC lines, or the discrete gpu
> to proxy DDC/AUX communication: Both the handler and the discrete
> gpu may initialize after the i915 driver, and consequently, an LVDS
> connector may initially seem disconnected and the SSC therefore
> is disabled by intel_init_pch_refclk, but on reprobe the connector
> may turn out to be connected and the SSC must then be enabled.
> 
> Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
> it is assumed BIOS disabled it while in fact it was disabled by
> intel_init_pch_refclk.
> 
> Also, because the SSC status is preserved so late, the preserved value
> only ever gets used on resume but not on panel initialization:
> intel_modeset_init calls intel_init_display which indirectly calls
> intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
> overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
> is the sole user of dev_priv->vbt.lvds_use_ssc).
> 
> Fix this by moving the code introduced by 92122789b2d6 from
> intel_modeset_gem_init to intel_modeset_init before the invocation
> of intel_setup_outputs and intel_init_display.
> 
> Add a DRM_DEBUG_KMS as suggested way back by Jani:
> http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
> Tested-by: William Brown <william@blackhats.net.au>
>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
> Tested-by: Lukas Wunner <lukas@wunner.de>
>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
> 
> Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
> Signed-off-by: Lukas Wunner <lukas@wunner.de>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
>  1 file changed, 18 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index af0bcfe..6335883 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)
>  	if (INTEL_INFO(dev)->num_pipes == 0)
>  		return;
>  
> +	/*
> +	 * There may be no VBT; and if the BIOS enabled SSC we can
> +	 * just keep using it to avoid unnecessary flicker.  Whereas if the
> +	 * BIOS isn't using it, don't assume it will work even if the VBT
> +	 * indicates as much.
> +	 */
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> +		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
> +					    DREF_SSC1_ENABLE);
> +
> +		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
> +			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
> +				     bios_lvds_use_ssc ? "en" : "dis",
> +				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
> +			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
> +		}
> +	}
> +
>  	intel_init_display(dev);
>  	intel_init_audio(dev);
>  
> @@ -15446,7 +15464,6 @@ err:
>  
>  void intel_modeset_gem_init(struct drm_device *dev)
>  {
> -	struct drm_i915_private *dev_priv = dev->dev_private;
>  	struct drm_crtc *c;
>  	struct drm_i915_gem_object *obj;
>  	int ret;
> @@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>  	intel_init_gt_powersave(dev);
>  	mutex_unlock(&dev->struct_mutex);
>  
> -	/*
> -	 * There may be no VBT; and if the BIOS enabled SSC we can
> -	 * just keep using it to avoid unnecessary flicker.  Whereas if the
> -	 * BIOS isn't using it, don't assume it will work even if the VBT
> -	 * indicates as much.
> -	 */
> -	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
> -		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
> -						DREF_SSC1_ENABLE);
> -
>  	intel_modeset_init_hw(dev);
>  
>  	intel_setup_overlay(dev);
> 

Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Thanks,
Jesse
Jani Nikula Sept. 1, 2015, 6:46 a.m. UTC | #2
On Mon, 31 Aug 2015, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On 07/15/2015 04:57 AM, Lukas Wunner wrote:
>> Commit 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
>> added code to intel_modeset_gem_init to override the SSC status read
>> from VBT with the SSC status set by BIOS.
>> 
>> However, intel_modeset_gem_init is invoked *after* intel_modeset_init,
>> which calls intel_setup_outputs, which *modifies* SSC status by way of
>> intel_init_pch_refclk. So unlike advertised, intel_modeset_gem_init
>> doesn't preserve the SSC status set by BIOS but whatever
>> intel_init_pch_refclk decided on.
>> 
>> This is a problem on dual gpu laptops such as the MacBook Pro which
>> require either a handler to switch DDC lines, or the discrete gpu
>> to proxy DDC/AUX communication: Both the handler and the discrete
>> gpu may initialize after the i915 driver, and consequently, an LVDS
>> connector may initially seem disconnected and the SSC therefore
>> is disabled by intel_init_pch_refclk, but on reprobe the connector
>> may turn out to be connected and the SSC must then be enabled.
>> 
>> Due to 92122789b2d6 however, the SSC is not enabled on reprobe since
>> it is assumed BIOS disabled it while in fact it was disabled by
>> intel_init_pch_refclk.
>> 
>> Also, because the SSC status is preserved so late, the preserved value
>> only ever gets used on resume but not on panel initialization:
>> intel_modeset_init calls intel_init_display which indirectly calls
>> intel_panel_use_ssc via multiple subroutines, *before* the BIOS value
>> overrides the VBT value in intel_modeset_gem_init (intel_panel_use_ssc
>> is the sole user of dev_priv->vbt.lvds_use_ssc).
>> 
>> Fix this by moving the code introduced by 92122789b2d6 from
>> intel_modeset_gem_init to intel_modeset_init before the invocation
>> of intel_setup_outputs and intel_init_display.
>> 
>> Add a DRM_DEBUG_KMS as suggested way back by Jani:
>> http://lists.freedesktop.org/archives/intel-gfx/2014-June/046666.html
>> 
>> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=88861
>> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=61115
>> Tested-by: Paul Hordiienko <pvt.gord@gmail.com>
>>     [MBP  6,2 2010  intel ILK + nvidia GT216  pre-retina]
>> Tested-by: William Brown <william@blackhats.net.au>
>>     [MBP  8,2 2011  intel SNB + amd turks     pre-retina]
>> Tested-by: Lukas Wunner <lukas@wunner.de>
>>     [MBP  9,1 2012  intel IVB + nvidia GK107  pre-retina]
>> Tested-by: Bruno Bierbaumer <bruno@bierbaumer.net>
>>     [MBP 11,3 2013  intel HSW + nvidia GK107  retina -- work in progress]
>> 
>> Fixes: 92122789b2d6 ("drm/i915: preserve SSC if previously set v3")
>> Signed-off-by: Lukas Wunner <lukas@wunner.de>
>> ---
>>  drivers/gpu/drm/i915/intel_display.c | 29 ++++++++++++++++++-----------
>>  1 file changed, 18 insertions(+), 11 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index af0bcfe..6335883 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -14893,6 +14893,24 @@ void intel_modeset_init(struct drm_device *dev)
>>  	if (INTEL_INFO(dev)->num_pipes == 0)
>>  		return;
>>  
>> +	/*
>> +	 * There may be no VBT; and if the BIOS enabled SSC we can
>> +	 * just keep using it to avoid unnecessary flicker.  Whereas if the
>> +	 * BIOS isn't using it, don't assume it will work even if the VBT
>> +	 * indicates as much.
>> +	 */
>> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>> +		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
>> +					    DREF_SSC1_ENABLE);
>> +
>> +		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
>> +			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
>> +				     bios_lvds_use_ssc ? "en" : "dis",
>> +				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
>> +			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
>> +		}
>> +	}
>> +
>>  	intel_init_display(dev);
>>  	intel_init_audio(dev);
>>  
>> @@ -15446,7 +15464,6 @@ err:
>>  
>>  void intel_modeset_gem_init(struct drm_device *dev)
>>  {
>> -	struct drm_i915_private *dev_priv = dev->dev_private;
>>  	struct drm_crtc *c;
>>  	struct drm_i915_gem_object *obj;
>>  	int ret;
>> @@ -15455,16 +15472,6 @@ void intel_modeset_gem_init(struct drm_device *dev)
>>  	intel_init_gt_powersave(dev);
>>  	mutex_unlock(&dev->struct_mutex);
>>  
>> -	/*
>> -	 * There may be no VBT; and if the BIOS enabled SSC we can
>> -	 * just keep using it to avoid unnecessary flicker.  Whereas if the
>> -	 * BIOS isn't using it, don't assume it will work even if the VBT
>> -	 * indicates as much.
>> -	 */
>> -	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
>> -		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
>> -						DREF_SSC1_ENABLE);
>> -
>>  	intel_modeset_init_hw(dev);
>>  
>>  	intel_setup_overlay(dev);
>> 
>
> Yeah looks good (and I'm having deja vu here; I thought I ran into the same ordering issue at one point in a report from krh, but I guess I never fixed it; didn't have test hw at the time).
>
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>

Pushed this one patch to drm-intel-next-fixes, thanks for the patch and
review.

BR,
Jani.


>
> Thanks,
> Jesse
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patch
diff mbox

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index af0bcfe..6335883 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -14893,6 +14893,24 @@  void intel_modeset_init(struct drm_device *dev)
 	if (INTEL_INFO(dev)->num_pipes == 0)
 		return;
 
+	/*
+	 * There may be no VBT; and if the BIOS enabled SSC we can
+	 * just keep using it to avoid unnecessary flicker.  Whereas if the
+	 * BIOS isn't using it, don't assume it will work even if the VBT
+	 * indicates as much.
+	 */
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		bool bios_lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
+					    DREF_SSC1_ENABLE);
+
+		if (dev_priv->vbt.lvds_use_ssc != bios_lvds_use_ssc) {
+			DRM_DEBUG_KMS("SSC %sabled by BIOS, overriding VBT which says %sabled\n",
+				     bios_lvds_use_ssc ? "en" : "dis",
+				     dev_priv->vbt.lvds_use_ssc ? "en" : "dis");
+			dev_priv->vbt.lvds_use_ssc = bios_lvds_use_ssc;
+		}
+	}
+
 	intel_init_display(dev);
 	intel_init_audio(dev);
 
@@ -15446,7 +15464,6 @@  err:
 
 void intel_modeset_gem_init(struct drm_device *dev)
 {
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_crtc *c;
 	struct drm_i915_gem_object *obj;
 	int ret;
@@ -15455,16 +15472,6 @@  void intel_modeset_gem_init(struct drm_device *dev)
 	intel_init_gt_powersave(dev);
 	mutex_unlock(&dev->struct_mutex);
 
-	/*
-	 * There may be no VBT; and if the BIOS enabled SSC we can
-	 * just keep using it to avoid unnecessary flicker.  Whereas if the
-	 * BIOS isn't using it, don't assume it will work even if the VBT
-	 * indicates as much.
-	 */
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
-		dev_priv->vbt.lvds_use_ssc = !!(I915_READ(PCH_DREF_CONTROL) &
-						DREF_SSC1_ENABLE);
-
 	intel_modeset_init_hw(dev);
 
 	intel_setup_overlay(dev);