[Intel-gfx] [PATCH v2 12/22] drm/i915: Preserve SSC earlier
On Mon, 31 Aug 2015, Jesse Barnes 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/04.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
>> [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
>> Tested-by: William Brown
>> [MBP 8,2 2011 intel SNB + amd turks pre-retina]
>> Tested-by: Lukas Wunner
>> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
>> Tested-by: Bruno Bierbaumer
>> [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
>> ---
>> 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(>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-gfx] [PATCH v2 12/22] drm/i915: Preserve SSC earlier
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/04.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
> [MBP 6,2 2010 intel ILK + nvidia GT216 pre-retina]
> Tested-by: William Brown
> [MBP 8,2 2011 intel SNB + amd turks pre-retina]
> Tested-by: Lukas Wunner
> [MBP 9,1 2012 intel IVB + nvidia GK107 pre-retina]
> Tested-by: Bruno Bierbaumer
> [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
> ---
> 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(>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
