On Wed, 28 Sep 2011 10:09:13 +0100, Chris Wilson chris@chris-wilson.co.uk wrote:
My understanding was that we could not enable SSC at all if we had a VGA, DVI/HDMI or TV output; DP may or may not work with SSC.
Yeah, which makes no sense at all. If this were true, we'd have to turn off the LVDS/eDP panel whenever enabling one of the non-SSC outputs.
The patch says that we will want to enable SSC if we have an SSC capbable LVDS or eDP, which is certainly true. And that we can always do so if we remember to set a magic bit in refclk to prevent non-SSC capable outputs from being upset. I have not seen anything to support that last statement, but, then again, I have not seen anything that actually explains what CK505 is!
CK505 is an Intel specification for external clock synthesizers. Here's an older one made by Silego:
http://www.silego.com/uploads/Products/product_54/xSLG8SP585r101_10062009.pd...
There are newer ones which provide the (required?) 120MHz output specified in the bspec.
However, if you go read:
http://www.advantech.com.tw/embcore/promotions/Whitepaper/2nd_Gen_Intel_Core...
you'll see that Cougarpoint is reported to have a fully integrated clocking solution and not require an external CK505. Which makes the lack of the display_clock_mode bit in the VBIOS sources a lot more understandable.
So, I think we should not look for a CK505 on Cougar Point systems, only on Ibex Peak systems, and that we probably cannot use SSC on Ibex Peak unless we have a CK505.
Having said that, this is an obvious improvement over the current situation in that we do choose correctly in more circumstances and we do not reprogram the refclk whilst active.
I think we can reprogram the refclk after init time, but only if no-one is using it, which is something we can do later on.
As an incremental improvement [in my understanding ;-]: Reviewed-by: Chris Wilson chris@chris-wilson.co.uk
Perhaps this patch on top of the existing patch?
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 1cc0962..4bf49eb 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -5122,6 +5122,8 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) bool has_cpu_edp = false; bool has_pch_edp = false; bool has_panel = false; + bool has_ck505 = false; + bool can_ssc = false;
/* We need to take the global config into account */ list_for_each_entry(encoder, &mode_config->encoder_list, @@ -5141,9 +5143,17 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) } }
+ if (HAS_PCH_IBX(dev)) { + has_ck505 = dev_priv->display_clock_mode; + can_ssc = has_ck505; + } else { + has_ck505 = false; + can_ssc = true; + } + DRM_DEBUG_KMS("has_panel %d has_lvds %d has_pch_edp %d has_cpu_edp %d has_ck505 %d\n", has_panel, has_lvds, has_pch_edp, has_cpu_edp, - dev_priv->display_clock_mode); + has_ck505);
/* Ironlake: try to setup display ref clock before DPLL * enabling. This is only under driver's control after @@ -5154,7 +5164,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) /* Always enable nonspread source */ temp &= ~DREF_NONSPREAD_SOURCE_MASK;
- if (dev_priv->display_clock_mode) + if (has_ck505) temp |= DREF_NONSPREAD_CK505_ENABLE; else temp |= DREF_NONSPREAD_SOURCE_ENABLE; @@ -5164,7 +5174,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) temp |= DREF_SSC_SOURCE_ENABLE;
/* SSC must be turned on before enabling the CPU output */ - if (intel_panel_use_ssc(dev_priv)) { + if (intel_panel_use_ssc(dev_priv) && can_ssc) { DRM_DEBUG_KMS("Using SSC on panel\n"); temp |= DREF_SSC1_ENABLE; } @@ -5178,7 +5188,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)
/* Enable CPU source on CPU attached eDP */ if (has_cpu_edp) { - if (intel_panel_use_ssc(dev_priv)) { + if (intel_panel_use_ssc(dev_priv) && can_ssc) { DRM_DEBUG_KMS("Using SSC on eDP\n"); temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; }