Re: [PATCH 6/9] drm/i915: Fix PCH SSC reference clock settings

[Keith Packard <keithp@keithp.com>]

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

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

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

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;
 			}

-- 
keith.packard@intel.com

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