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

From: Keith Packard
Date: Wed Sep 28 2011 - 12:36:24 EST

On Wed, 28 Sep 2011 10:09:13 +0100, Chris Wilson <chris@xxxxxxxxxxxxxxxxxx> 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:

There are newer ones which provide the (required?) 120MHz output
specified in the bspec.

However, if you go read:

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

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@xxxxxxxxxxxxxxxxxx>

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 */

- if (dev_priv->display_clock_mode)
+ if (has_ck505)
@@ -5164,7 +5174,7 @@ static void ironlake_init_pch_refclk(struct drm_device *dev)

/* 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");
@@ -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");


Attachment: pgp00000.pgp
Description: PGP signature