[PATCH] drm/i915: add missing "break"

[PATCH] drm/i915: add missing "break"

[przanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

It seems to be missing from this commit:
  "drm/i915: split out PCH refclk update code"

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index c829875..2921bac 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5120,6 +5120,7 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 			switch (encoder->type) {
 			case INTEL_OUTPUT_LVDS:
 				has_lvds = true;
+				break;
 			case INTEL_OUTPUT_EDP:
 				has_edp_encoder = encoder;
 				break;
-- 
1.7.4.1

Re: [PATCH] drm/i915: add missing "break"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 5712 bytes --]

On Thu, 22 Sep 2011 15:13:42 -0300, przanoni@gmail.com wrote:

> It seems to be missing from this commit:
>   "drm/i915: split out PCH refclk update code"

Oh, this code is missing far more than that. It doesn't deal with the
LVDS case at all.

Here's a patch which turns on the right bits of SSC, in the right order,
for any combination of CPU-eDP, PCH-eDP and LVDS.

Jesse: you wrote this stuff, can you review what I did? Also, my two SNB
systems (X220 and MacBook Air) both have a BIOS table that disables SSC,
is there any reason for us to believe the BIOS table on a PCH system?
Can't we always use SSC?

I've tested this on LVDS and CPU-connected eDP with and without SSC and
it seems to work for me.

From ea63d98df915f81c80cf4f91bf9d1bcaece7fce0 Mon Sep 17 00:00:00 2001
From: Keith Packard <keithp@keithp.com>
Date: Thu, 22 Sep 2011 12:01:57 -0700
Subject: [PATCH 1/3] drm/i915: Fix PCH SSC reference clock settings

The PCH refclk settings are global, so we need to look at all of the
encoders, not just the current encoder when deciding how to configure
it. Also, handle systems with more than one panel (any combination of
PCH/non-PCH eDP and LVDS).

Disable SSC clocks when no panels are connected.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   88 ++++++++++++++++++++++++---------
 1 files changed, 64 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 04411ad..01a52fc 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5113,9 +5113,11 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 	struct drm_mode_config *mode_config = &dev->mode_config;
 	struct drm_crtc *crtc;
 	struct intel_encoder *encoder;
-	struct intel_encoder *has_edp_encoder = NULL;
 	u32 temp;
 	bool has_lvds = false;
+	bool has_cpu_edp = false;
+	bool has_pch_edp = false;
+	bool has_panel = false;
 
 	/* We need to take the global config into account */
 	list_for_each_entry(crtc, &mode_config->crtc_list, head) {
@@ -5124,18 +5126,23 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 
 		list_for_each_entry(encoder, &mode_config->encoder_list,
 				    base.head) {
-			if (encoder->base.crtc != crtc)
-				continue;
-
 			switch (encoder->type) {
 			case INTEL_OUTPUT_LVDS:
+				has_panel = true;
 				has_lvds = true;
+				break;
 			case INTEL_OUTPUT_EDP:
-				has_edp_encoder = encoder;
+				has_panel = true;
+				if (intel_encoder_is_pch_edp(&encoder->base))
+					has_pch_edp = true;
+				else
+					has_cpu_edp = true;
 				break;
 			}
 		}
 	}
+	DRM_DEBUG_KMS("has_panel %d has_lvds %d has_pch_edp %d has_cpu_edp %d\n",
+		      has_panel, has_lvds, has_pch_edp, has_cpu_edp);
 
 	/* Ironlake: try to setup display ref clock before DPLL
 	 * enabling. This is only under driver's control after
@@ -5146,36 +5153,69 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 	/* Always enable nonspread source */
 	temp &= ~DREF_NONSPREAD_SOURCE_MASK;
 	temp |= DREF_NONSPREAD_SOURCE_ENABLE;
-	temp &= ~DREF_SSC_SOURCE_MASK;
-	temp |= DREF_SSC_SOURCE_ENABLE;
-	I915_WRITE(PCH_DREF_CONTROL, temp);
 
-	POSTING_READ(PCH_DREF_CONTROL);
-	udelay(200);
+	if (has_panel) {
+		temp &= ~DREF_SSC_SOURCE_MASK;
+		temp |= DREF_SSC_SOURCE_ENABLE;
 
-	if (has_edp_encoder) {
+		/* SSC must be turned on before enabling the CPU output  */
 		if (intel_panel_use_ssc(dev_priv)) {
+			DRM_DEBUG_KMS("Using SSC on panel\n");
 			temp |= DREF_SSC1_ENABLE;
-			I915_WRITE(PCH_DREF_CONTROL, temp);
-
-			POSTING_READ(PCH_DREF_CONTROL);
-			udelay(200);
 		}
+
+		/* Get SSC going before enabling the outputs */
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+
 		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+		temp &= ~DREF_SUPERSPREAD_SOURCE_MASK;
 
 		/* Enable CPU source on CPU attached eDP */
-		if (!intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
-			if (intel_panel_use_ssc(dev_priv))
+		if (has_cpu_edp) {
+			if (intel_panel_use_ssc(dev_priv)) {
+				DRM_DEBUG_KMS("Using SSC on eDP\n");
 				temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
+			}
 			else
 				temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
-		} else {
-			/* Enable SSC on PCH eDP if needed */
-			if (intel_panel_use_ssc(dev_priv)) {
-				DRM_ERROR("enabling SSC on PCH\n");
-				temp |= DREF_SUPERSPREAD_SOURCE_ENABLE;
-			}
-		}
+		} else
+			temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+
+		/* Enable SSC on PCH eDP or LVDS if needed */
+		if ((has_pch_edp || has_lvds) && intel_panel_use_ssc(dev_priv)) {
+			DRM_DEBUG_KMS("Using SSC on PCH eDP or LVDS\n");
+			temp |= DREF_SUPERSPREAD_SOURCE_ENABLE;
+		} else
+			temp |= DREF_SUPERSPREAD_SOURCE_DISABLE;
+
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+	} else {
+		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+		temp &= ~DREF_SUPERSPREAD_SOURCE_MASK;
+
+		DRM_DEBUG_KMS("Disabling SSC entirely\n");
+
+		/* Turn off CPU output */
+		temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+
+		/* Disable SSC on PCH */
+		temp |= DREF_SUPERSPREAD_SOURCE_DISABLE;
+
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+
+		/* Turn off the SSC source */
+		temp &= ~DREF_SSC_SOURCE_MASK;
+		temp |= DREF_SSC_SOURCE_DISABLE;
+
+		/* Turn off SSC1 */
+		temp &= ~ DREF_SSC1_ENABLE;
+
 		I915_WRITE(PCH_DREF_CONTROL, temp);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
-- 
1.7.6.3




-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: add missing "break"

[Jesse Barnes]

On Thu, 22 Sep 2011 12:55:22 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Thu, 22 Sep 2011 15:13:42 -0300, przanoni@gmail.com wrote:
> 
> > It seems to be missing from this commit:
> >   "drm/i915: split out PCH refclk update code"
> 
> Oh, this code is missing far more than that. It doesn't deal with the
> LVDS case at all.
> 
> Here's a patch which turns on the right bits of SSC, in the right
> order, for any combination of CPU-eDP, PCH-eDP and LVDS.
> 
> Jesse: you wrote this stuff, can you review what I did? Also, my two
> SNB systems (X220 and MacBook Air) both have a BIOS table that
> disables SSC, is there any reason for us to believe the BIOS table on
> a PCH system? Can't we always use SSC?

I think it depends on the platform.  On some, enabling SSC may actually
create more noise than not for some components (not that I've run the
EMF calculations...).

I don't have this code in my tree though... is this the patch I sent
awhile back?  I thought it broke external outputs too?  The last time
we touched this we broke the dual head case (a config change caused one
head to go blank), did you test that?

What I don't understand about the refclk code is that we should be able
to leave everything enabled and just select the right clock source in
the DPLL_SEL bits.  But that doesn't seem to help the wavy VGA bug,
since in that case I think we're explicitly choosing the non-SSC clock
and we still get waviness.

I *think* the code you changed is ok; just needs lots of testing and
verification that the SSC bits are set like we expect as we change
configurations.  I like the "has_panel" cleanup too; previous versions
of this code had is_lvds || is_edp && pch_edp etc sprinkled all over.

Jesse

Re: [PATCH] drm/i915: add missing "break"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1913 bytes --]

On Fri, 23 Sep 2011 08:13:15 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> I think it depends on the platform.  On some, enabling SSC may actually
> create more noise than not for some components (not that I've run the
> EMF calculations...).

So, why is the MBA EFI code enabling SSC while the VBT table says not to
use it? It's all mysterious to me.

> I don't have this code in my tree though... is this the patch I sent
> awhile back?  I thought it broke external outputs too?  The last time
> we touched this we broke the dual head case (a config change caused one
> head to go blank), did you test that?

You sent two pieces, I think; the second one tried to turn unused clocks
off and that broke stuff. Which is understandable given that it was only
looking at one CRTC when doing the global configuration.

> What I don't understand about the refclk code is that we should be able
> to leave everything enabled and just select the right clock source in
> the DPLL_SEL bits.  But that doesn't seem to help the wavy VGA bug,
> since in that case I think we're explicitly choosing the non-SSC clock
> and we still get waviness.

We don't have any hardware anywhere which exhibits this problem, do we?
Getting hold of some would let us poke at it.

> I *think* the code you changed is ok; just needs lots of testing and
> verification that the SSC bits are set like we expect as we change
> configurations.  I like the "has_panel" cleanup too; previous versions
> of this code had is_lvds || is_edp && pch_edp etc sprinkled all over.

Yeah, it's longer, but I think it's more readable now.

My big concern is that we're guessing which pipes will use SSC in this
function and then computing which pipes actually use SSC separately. I
think we should figure out which pipes want to use SSC and then go set
the refclks. as needed.

-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: add missing "break"

[Paulo Zanoni]

Hi

2011/9/23 Keith Packard <keithp@keithp.com>:
> On Fri, 23 Sep 2011 08:13:15 +0530, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
>> What I don't understand about the refclk code is that we should be able
>> to leave everything enabled and just select the right clock source in
>> the DPLL_SEL bits.  But that doesn't seem to help the wavy VGA bug,
>> since in that case I think we're explicitly choosing the non-SSC clock
>> and we still get waviness.
>
> We don't have any hardware anywhere which exhibits this problem, do we?
> Getting hold of some would let us poke at it.

My personal laptop reproduces the "wavy output" problem.

If you look at the patch posted on comment #20 of bug #38750 [0],
you'll see that it checks on the video bios for a field called
"display_clock_mode" and then uses this field in the following code:
+		if (dev_priv->display_clock_mode)
+			temp |= DREF_NONSPREAD_CK505_ENABLE;
+		else
+			temp |= DREF_NONSPREAD_SOURCE_ENABLE;

This is what fixes the wavy output problem: setting
DREF_NONSPREAD_CK505_ENABLE instead of the other bits.

And for Keith's patch:

Tested-By: Paulo Zanoni <paulo.r.zanoni@intel.com>

[0]: https://bugs.freedesktop.org/attachment.cgi?id=49888

-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add missing "break"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 2017 bytes --]

On Fri, 23 Sep 2011 09:06:31 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:

> If you look at the patch posted on comment #20 of bug #38750 [0],
> you'll see that it checks on the video bios for a field called
> "display_clock_mode" and then uses this field in the following code:
> +		if (dev_priv->display_clock_mode)
> +			temp |= DREF_NONSPREAD_CK505_ENABLE;
> +		else
> +			temp |= DREF_NONSPREAD_SOURCE_ENABLE;
> 
> This is what fixes the wavy output problem: setting
> DREF_NONSPREAD_CK505_ENABLE instead of the other bits.

Ok, so I found out what CK505 is -- it's an external clock synthesizer
for ancient CPUs (from 2006).

It seems like 

Next, I went and looked at the Sandybridge VBIOS sources. That's quite
interesting. I expected to see it looking at the display_clock_mode bit
to select between the internal and CK505 sources for the non-spread
clock, but it doesn't -- it always uses the CK505 source. In other news,
it has a nice comment about the clock source not updating unless the
DPLL is off.

I also found a few more interesting bits in the general features table:

@@ -133,7 +133,10 @@ struct bdb_general_features {
         /* bits 5 */
 	u8 int_crt_support:1;
 	u8 int_tv_support:1;
-	u8 rsvd11:6; /* finish byte */
+	u8 int_efp_support:1;
+	u8 dp_ssc_enb:1;	/* PCH attached eDP supports SSC */
+	u8 dp_ssc_freq:1;	/* SSC freq for PCH attached eDP */
+	u8 rsvd11:3; /* finish byte */
 } __attribute__((packed));
 
 /* pre-915 */

What I didn't find there was any mention of the display_clock_mode
field; perhaps jbarnes has newer VBIOS sources or actual BDB
documentation.

What I'm starting to suspect is that there are two clock sources, the
internal one and the CK505 one and that if we need a non-SSC clock and
can't use the CK505 one, then we can't turn on SSC for the internal one.

> And for Keith's patch:
> 
> Tested-By: Paulo Zanoni <paulo.r.zanoni@intel.com>

Can you explain what you tested?

-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: add missing "break"

[Paulo Zanoni]

2011/9/23 Keith Packard <keithp@keithp.com>:
>
> Can you explain what you tested?

Sure. It's an Ironlake laptop so I tested its LVDS, and I also
plugged/unplugged the VGA output a few times, checked the value of the
DREF_CONTROL register, tried to replace the SOURCE_ENABLE for
CK505_ENABLE (to stop the wavy external output), rebooted a few times
and used the machine for a few hours since yesterday. The patch is
applied on top of your drm-intel-next tree. Nothing exploded so far.


-- 
Paulo Zanoni

Re: [PATCH] drm/i915: add missing "break"

[Chris Wilson]

On Fri, 23 Sep 2011 09:15:05 -0700, Keith Packard <keithp@keithp.com> wrote:
> What I didn't find there was any mention of the display_clock_mode
> field; perhaps jbarnes has newer VBIOS sources or actual BDB
> documentation.

iirc display_clock_mode was found in the Capella VBIOS assembly, so
indeed comparatively ancient.

I do enjoy how our best guides for programming the hardware comes from
RE the bioses. :(
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: add missing "break"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1190 bytes --]

On Fri, 23 Sep 2011 20:07:52 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Fri, 23 Sep 2011 09:15:05 -0700, Keith Packard <keithp@keithp.com> wrote:
> > What I didn't find there was any mention of the display_clock_mode
> > field; perhaps jbarnes has newer VBIOS sources or actual BDB
> > documentation.
> 
> iirc display_clock_mode was found in the Capella VBIOS assembly, so
> indeed comparatively ancient.

Ok, so it appears to have disappeared in more recent hardware. I'm
betting that with PCH hardware, there is always a CK505 source to use,
so the BIOS just uses it unconditionally and may not actually be
configured in the VBT.

> I do enjoy how our best guides for programming the hardware comes from
> RE the bioses. :(

It's not the best, it's just yet another source of information :-)

So, what I think we should be doing is using display_clock_mode to
select between ck505 and internal clock source on pre-PCH hardware. For
PCH hardware, we unconditionally use the ck505 source for the non-SSC
reference. When we need to drive two outputs and are *not* using ck505,
then we disable SSC.

Seem reasonable?

-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [PATCH] drm/i915: add missing "break"

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 693 bytes --]

On Mon, 26 Sep 2011 13:56:17 -0700, Keith Packard <keithp@keithp.com> wrote:

> So, what I think we should be doing is using display_clock_mode to
> select between ck505 and internal clock source on pre-PCH hardware. For
> PCH hardware, we unconditionally use the ck505 source for the non-SSC
> reference. When we need to drive two outputs and are *not* using ck505,
> then we disable SSC.

Ok, I'm not sure this is trivial -- without CK505, how do we switch from
LVDS to LVDS+VGA (and back)? Seems like we'd have to turn the LVDS off
and back on to play with SSC. Is it worth dealing with that case? Or do
we just ignore SSC on non-CK505 hardware?

-- 
keith.packard@intel.com

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

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

PCH reference clock cleanups

[Keith Packard]

Here's a patch sequence which cleans up a bunch of PCH refclk related
bits. There are a couple of questionable patches that I'd like to see
people look at:

 [PATCH 6/9] drm/i915: Fix PCH SSC reference clock settings
 [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

Here's the main patch -- this looks at the global set of encoders and
figures out what the refclk should be to make all of those work
correctly. Nothing is dependent on the active configuration, so we
aren't reprogramming this register during run-time. The last patch in
the sequence moves the setting of this register from modeset time to
init time.

 [PATCH 7/9] drm/i915: Use CK505 as non-SSC source where available

This is a small piece straight from Jesse's patch; just uses the VBT
configuration for CK505 clock sources.

 [PATCH 8/9] drm/i915: All PCH refclks are 120MHz

Ok, so I'd love to know where in any PCH reference matter someone has
found a place where the reference clock for any of the PLLs is
anything other than 120MHz. Can someone find a reference for other frequencies?

--
keith.packard@intel.com

[PATCH 1/9] drm/i915: broken copyright encoding in intel_bios.c

[Keith Packard]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: text/plain; charset=UTF-8, Size: 613 bytes --]

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_bios.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 61abef8..4c530fa 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -1,5 +1,5 @@
 /*
- * Copyright © 2006 Intel Corporation
+ * Copyright © 2006 Intel Corporation
  *
  * Permission is hereby granted, free of charge, to any person obtaining a
  * copy of this software and associated documentation files (the "Software"),
-- 
1.7.6.3

[PATCH 2/9] drm/i915: Use DRM_DEBUG_KMS for all messages in intel_bios.c

[Keith Packard]

These are all KMS related anyways, so don't hide them under other
debug levels.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_bios.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index 4c530fa..dcbc839 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -309,6 +309,11 @@ parse_general_features(struct drm_i915_private *dev_priv,
 		dev_priv->lvds_use_ssc = general->enable_ssc;
 		dev_priv->lvds_ssc_freq =
 			intel_bios_ssc_frequency(dev, general->ssc_freq);
+		DRM_DEBUG_KMS("BDB_GENERAL_FEATURES int_tv_support %d int_crt_support %d lvds_use_ssc %d lvds_ssc_freq %d\n",
+			      dev_priv->int_tv_support,
+			      dev_priv->int_crt_support,
+			      dev_priv->lvds_use_ssc,
+			      dev_priv->lvds_ssc_freq);
 	}
 }
 
@@ -610,7 +615,7 @@ init_vbt_defaults(struct drm_i915_private *dev_priv)
 	/* Default to using SSC */
 	dev_priv->lvds_use_ssc = 1;
 	dev_priv->lvds_ssc_freq = intel_bios_ssc_frequency(dev, 1);
-	DRM_DEBUG("Set default to SSC at %dMHz\n", dev_priv->lvds_ssc_freq);
+	DRM_DEBUG_KMS("Set default to SSC at %dMHz\n", dev_priv->lvds_ssc_freq);
 
 	/* eDP data */
 	dev_priv->edp.bpp = 18;
@@ -639,7 +644,7 @@ intel_parse_bios(struct drm_device *dev)
 	if (dev_priv->opregion.vbt) {
 		struct vbt_header *vbt = dev_priv->opregion.vbt;
 		if (memcmp(vbt->signature, "$VBT", 4) == 0) {
-			DRM_DEBUG_DRIVER("Using VBT from OpRegion: %20s\n",
+			DRM_DEBUG_KMS("Using VBT from OpRegion: %20s\n",
 					 vbt->signature);
 			bdb = (struct bdb_header *)((char *)vbt + vbt->bdb_offset);
 		} else
-- 
1.7.6.3

[PATCH 3/9] drv/i915: Pull display_clock_mode out of VBT table

[Keith Packard]

This tells the driver whether a CK505 clock source is available on
pre-PCH hardware. If so, it should be used as the non-SSC source,
leaving the internal clock for use as the SSC source.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.h   |    1 +
 drivers/gpu/drm/i915/intel_bios.c |    6 ++++--
 drivers/gpu/drm/i915/intel_bios.h |    4 +++-
 3 files changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 7916bd9..18df595 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -357,6 +357,7 @@ typedef struct drm_i915_private {
 	unsigned int lvds_vbt:1;
 	unsigned int int_crt_support:1;
 	unsigned int lvds_use_ssc:1;
+	unsigned int display_clock_mode:1;
 	int lvds_ssc_freq;
 	struct {
 		int rate;
diff --git a/drivers/gpu/drm/i915/intel_bios.c b/drivers/gpu/drm/i915/intel_bios.c
index dcbc839..eb58784 100644
--- a/drivers/gpu/drm/i915/intel_bios.c
+++ b/drivers/gpu/drm/i915/intel_bios.c
@@ -309,11 +309,13 @@ parse_general_features(struct drm_i915_private *dev_priv,
 		dev_priv->lvds_use_ssc = general->enable_ssc;
 		dev_priv->lvds_ssc_freq =
 			intel_bios_ssc_frequency(dev, general->ssc_freq);
-		DRM_DEBUG_KMS("BDB_GENERAL_FEATURES int_tv_support %d int_crt_support %d lvds_use_ssc %d lvds_ssc_freq %d\n",
+		dev_priv->display_clock_mode = general->display_clock_mode;
+		DRM_DEBUG_KMS("BDB_GENERAL_FEATURES int_tv_support %d int_crt_support %d lvds_use_ssc %d lvds_ssc_freq %d display_clock_mode %d\n",
 			      dev_priv->int_tv_support,
 			      dev_priv->int_crt_support,
 			      dev_priv->lvds_use_ssc,
-			      dev_priv->lvds_ssc_freq);
+			      dev_priv->lvds_ssc_freq,
+			      dev_priv->display_clock_mode);
 	}
 }
 
diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 5f8e4ed..02b1b624 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -120,7 +120,9 @@ struct bdb_general_features {
 	u8 ssc_freq:1;
 	u8 enable_lfp_on_override:1;
 	u8 disable_ssc_ddt:1;
-	u8 rsvd8:3; /* finish byte */
+	u8 rsvd7:1;
+	u8 display_clock_mode:1;
+	u8 rsvd8:1; /* finish byte */
 
         /* bits 3 */
 	u8 disable_smooth_vision:1;
-- 
1.7.6.3

[PATCH 4/9] drm/i915: Document a few more BDB_GENERAL_FEATURES bits from PCH BIOS

[Keith Packard]

This includes whether an eDP panel is present, and whether that should
use SSC (and at what frequency)

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_bios.h |    5 ++++-
 1 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_bios.h b/drivers/gpu/drm/i915/intel_bios.h
index 02b1b624..72fb500 100644
--- a/drivers/gpu/drm/i915/intel_bios.h
+++ b/drivers/gpu/drm/i915/intel_bios.h
@@ -135,7 +135,10 @@ struct bdb_general_features {
         /* bits 5 */
 	u8 int_crt_support:1;
 	u8 int_tv_support:1;
-	u8 rsvd11:6; /* finish byte */
+	u8 int_efp_support:1;
+	u8 dp_ssc_enb:1;	/* PCH attached eDP supports SSC */
+	u8 dp_ssc_freq:1;	/* SSC freq for PCH attached eDP */
+	u8 rsvd11:3; /* finish byte */
 } __attribute__((packed));
 
 /* pre-915 */
-- 
1.7.6.3

[PATCH 5/9] drm/i915: Allow SSC parameter to override VBT value

[Keith Packard]

Allow SSC to be enabled even when the BIOS disables it for testing SSC paths.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/i915_drv.c      |    4 ++--
 drivers/gpu/drm/i915/intel_display.c |    4 +++-
 2 files changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index f07e425..58480de 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -79,11 +79,11 @@ MODULE_PARM_DESC(lvds_downclock,
 		"Use panel (LVDS/eDP) downclocking for power savings "
 		"(default: false)");
 
-unsigned int i915_panel_use_ssc __read_mostly = 1;
+unsigned int i915_panel_use_ssc __read_mostly = -1;
 module_param_named(lvds_use_ssc, i915_panel_use_ssc, int, 0600);
 MODULE_PARM_DESC(lvds_use_ssc,
 		"Use Spread Spectrum Clock with panels [LVDS/eDP] "
-		"(default: true)");
+		"(default: auto from VBT)");
 
 int i915_vbt_sdvo_panel_type __read_mostly = -1;
 module_param_named(vbt_sdvo_panel_type, i915_vbt_sdvo_panel_type, int, 0600);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 04411ad..6039496 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4584,7 +4584,9 @@ static void intel_update_watermarks(struct drm_device *dev)
 
 static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
 {
-	return dev_priv->lvds_use_ssc && i915_panel_use_ssc
+	if (i915_panel_use_ssc >= 0)
+		return i915_panel_use_ssc != 0;
+	return dev_priv->lvds_use_ssc
 		&& !(dev_priv->quirks & QUIRK_LVDS_SSC_DISABLE);
 }
 
-- 
1.7.6.3

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

[Keith Packard]

The PCH refclk settings are global, so we need to look at all of the
encoders, not just the current encoder when deciding how to configure
it. Also, handle systems with more than one panel (any combination of
PCH/non-PCH eDP and LVDS).

Disable SSC clocks when no panels are connected.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   96 +++++++++++++++++++++-------------
 1 files changed, 59 insertions(+), 37 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 6039496..f999935 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5113,31 +5113,32 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
-	struct drm_crtc *crtc;
 	struct intel_encoder *encoder;
-	struct intel_encoder *has_edp_encoder = NULL;
 	u32 temp;
 	bool has_lvds = false;
+	bool has_cpu_edp = false;
+	bool has_pch_edp = false;
+	bool has_panel = false;
 
 	/* We need to take the global config into account */
-	list_for_each_entry(crtc, &mode_config->crtc_list, head) {
-		if (!crtc->enabled)
-			continue;
-
-		list_for_each_entry(encoder, &mode_config->encoder_list,
-				    base.head) {
-			if (encoder->base.crtc != crtc)
-				continue;
-
-			switch (encoder->type) {
-			case INTEL_OUTPUT_LVDS:
-				has_lvds = true;
-			case INTEL_OUTPUT_EDP:
-				has_edp_encoder = encoder;
-				break;
-			}
+	list_for_each_entry(encoder, &mode_config->encoder_list,
+			    base.head) {
+		switch (encoder->type) {
+		case INTEL_OUTPUT_LVDS:
+			has_panel = true;
+			has_lvds = true;
+			break;
+		case INTEL_OUTPUT_EDP:
+			has_panel = true;
+			if (intel_encoder_is_pch_edp(&encoder->base))
+				has_pch_edp = true;
+			else
+				has_cpu_edp = true;
+			break;
 		}
 	}
+	DRM_DEBUG_KMS("has_panel %d has_lvds %d has_pch_edp %d has_cpu_edp %d\n",
+		      has_panel, has_lvds, has_pch_edp, has_cpu_edp);
 
 	/* Ironlake: try to setup display ref clock before DPLL
 	 * enabling. This is only under driver's control after
@@ -5148,36 +5149,57 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 	/* Always enable nonspread source */
 	temp &= ~DREF_NONSPREAD_SOURCE_MASK;
 	temp |= DREF_NONSPREAD_SOURCE_ENABLE;
-	temp &= ~DREF_SSC_SOURCE_MASK;
-	temp |= DREF_SSC_SOURCE_ENABLE;
-	I915_WRITE(PCH_DREF_CONTROL, temp);
 
-	POSTING_READ(PCH_DREF_CONTROL);
-	udelay(200);
+	if (has_panel) {
+		temp &= ~DREF_SSC_SOURCE_MASK;
+		temp |= DREF_SSC_SOURCE_ENABLE;
 
-	if (has_edp_encoder) {
+		/* SSC must be turned on before enabling the CPU output  */
 		if (intel_panel_use_ssc(dev_priv)) {
+			DRM_DEBUG_KMS("Using SSC on panel\n");
 			temp |= DREF_SSC1_ENABLE;
-			I915_WRITE(PCH_DREF_CONTROL, temp);
-
-			POSTING_READ(PCH_DREF_CONTROL);
-			udelay(200);
 		}
+
+		/* Get SSC going before enabling the outputs */
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+
 		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
 
 		/* Enable CPU source on CPU attached eDP */
-		if (!intel_encoder_is_pch_edp(&has_edp_encoder->base)) {
-			if (intel_panel_use_ssc(dev_priv))
+		if (has_cpu_edp) {
+			if (intel_panel_use_ssc(dev_priv)) {
+				DRM_DEBUG_KMS("Using SSC on eDP\n");
 				temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD;
+			}
 			else
 				temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD;
-		} else {
-			/* Enable SSC on PCH eDP if needed */
-			if (intel_panel_use_ssc(dev_priv)) {
-				DRM_ERROR("enabling SSC on PCH\n");
-				temp |= DREF_SUPERSPREAD_SOURCE_ENABLE;
-			}
-		}
+		} else
+			temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+	} else {
+		DRM_DEBUG_KMS("Disabling SSC entirely\n");
+
+		temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK;
+
+		/* Turn off CPU output */
+		temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE;
+
+		I915_WRITE(PCH_DREF_CONTROL, temp);
+		POSTING_READ(PCH_DREF_CONTROL);
+		udelay(200);
+
+		/* Turn off the SSC source */
+		temp &= ~DREF_SSC_SOURCE_MASK;
+		temp |= DREF_SSC_SOURCE_DISABLE;
+
+		/* Turn off SSC1 */
+		temp &= ~ DREF_SSC1_ENABLE;
+
 		I915_WRITE(PCH_DREF_CONTROL, temp);
 		POSTING_READ(PCH_DREF_CONTROL);
 		udelay(200);
-- 
1.7.6.3

[PATCH 7/9] drm/i915: Use CK505 as non-SSC source where available

[Keith Packard]

This eliminates VGA shimmer on some Ironlake machines which have a
CK505 clock source.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   12 +++++++++---
 1 files changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index f999935..66cd351 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5137,8 +5137,10 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 			break;
 		}
 	}
-	DRM_DEBUG_KMS("has_panel %d has_lvds %d has_pch_edp %d has_cpu_edp %d\n",
-		      has_panel, has_lvds, has_pch_edp, has_cpu_edp);
+
+	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);  
 
 	/* Ironlake: try to setup display ref clock before DPLL
 	 * enabling. This is only under driver's control after
@@ -5148,7 +5150,11 @@ static void ironlake_update_pch_refclk(struct drm_device *dev)
 	temp = I915_READ(PCH_DREF_CONTROL);
 	/* Always enable nonspread source */
 	temp &= ~DREF_NONSPREAD_SOURCE_MASK;
-	temp |= DREF_NONSPREAD_SOURCE_ENABLE;
+
+	if (dev_priv->display_clock_mode)
+		temp |= DREF_NONSPREAD_CK505_ENABLE;
+	else
+		temp |= DREF_NONSPREAD_SOURCE_ENABLE;
 
 	if (has_panel) {
 		temp &= ~DREF_SSC_SOURCE_MASK;
-- 
1.7.6.3

[PATCH 8/9] drm/i915: All PCH refclks are 120MHz

[Keith Packard]

I can't find any reference clocks which run at 96MHz as seems to be
indicated from the comments in this code.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   14 ++++----------
 1 files changed, 4 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 66cd351..919db79 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5271,16 +5271,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2) {
-		refclk = dev_priv->lvds_ssc_freq * 1000;
-		DRM_DEBUG_KMS("using SSC reference clock of %d MHz\n",
-			      refclk / 1000);
-	} else {
-		refclk = 96000;
-		if (!has_edp_encoder ||
-		    intel_encoder_is_pch_edp(&has_edp_encoder->base))
-			refclk = 120000; /* 120Mhz refclk */
-	}
+	/*
+	 * Every reference clock in a PCH system is 120MHz
+	 */
+	refclk = 120000;
 
 	/*
 	 * Returns a set of divisors for the desired target clock with the given
-- 
1.7.6.3

[PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Keith Packard]

The reference clock configuration must be done before any mode setting
can occur as all outputs must be disabled to change
anything. Initialize the clocks after turning everything off during
the initialization process.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   10 +++++++---
 1 files changed, 7 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 919db79..1cc0962 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5109,7 +5109,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	return ret;
 }
 
-static void ironlake_update_pch_refclk(struct drm_device *dev)
+/*
+ * Initialize reference clocks when the driver loads
+ */
+static void ironlake_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
@@ -5401,8 +5404,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
 			     &m_n);
 
-	ironlake_update_pch_refclk(dev);
-
 	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
 	if (has_reduced_clock)
 		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
@@ -7274,6 +7275,9 @@ static void intel_setup_outputs(struct drm_device *dev)
 
 	/* disable all the possible outputs/crtcs before entering KMS mode */
 	drm_helper_disable_unused_functions(dev);
+
+	if (HAS_PCH_SPLIT(dev))
+		ironlake_init_pch_refclk(dev);
 }
 
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
-- 
1.7.6.3

Re: PCH reference clock cleanups

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:37 -0700, Keith Packard <keithp@keithp.com> wrote:
> Ok, so I'd love to know where in any PCH reference matter someone has
> found a place where the reference clock for any of the PLLs is
> anything other than 120MHz. Can someone find a reference for other frequencies?

Oddly in the diagram SSC4 is given as a 100MHz clock that can be used for
any output other than DP_A. However, the configuration register marks that
as being a test-only mode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: PCH reference clock cleanups

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:37 -0700, Keith Packard <keithp@keithp.com> wrote:
> Ok, so I'd love to know where in any PCH reference matter someone has
> found a place where the reference clock for any of the PLLs is
> anything other than 120MHz. Can someone find a reference for other frequencies?

Oddly in the diagram SSC4 is given as a 100MHz clock that can be used for
any output other than DP_A. However, the configuration register marks that
as being a test-only mode.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/9] drm/i915: Use DRM_DEBUG_KMS for all messages in intel_bios.c

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:39 -0700, Keith Packard <keithp@keithp.com> wrote:
> These are all KMS related anyways, so don't hide them under other
> debug levels.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/9] drm/i915: Use DRM_DEBUG_KMS for all messages in intel_bios.c

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:39 -0700, Keith Packard <keithp@keithp.com> wrote:
> These are all KMS related anyways, so don't hide them under other
> debug levels.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/9] drv/i915: Pull display_clock_mode out of VBT table

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:40 -0700, Keith Packard <keithp@keithp.com> wrote:
> This tells the driver whether a CK505 clock source is available on
> pre-PCH hardware. If so, it should be used as the non-SSC source,
> leaving the internal clock for use as the SSC source.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wison <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 3/9] drv/i915: Pull display_clock_mode out of VBT table

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:40 -0700, Keith Packard <keithp@keithp.com> wrote:
> This tells the driver whether a CK505 clock source is available on
> pre-PCH hardware. If so, it should be used as the non-SSC source,
> leaving the internal clock for use as the SSC source.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wison <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/9] drm/i915: Allow SSC parameter to override VBT value

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> Allow SSC to be enabled even when the BIOS disables it for testing SSC paths.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 5/9] drm/i915: Allow SSC parameter to override VBT value

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:42 -0700, Keith Packard <keithp@keithp.com> wrote:
> Allow SSC to be enabled even when the BIOS disables it for testing SSC paths.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:43 -0700, Keith Packard <keithp@keithp.com> wrote:
> The PCH refclk settings are global, so we need to look at all of the
> encoders, not just the current encoder when deciding how to configure
> it. Also, handle systems with more than one panel (any combination of
> PCH/non-PCH eDP and LVDS).

As I read it, this sets the refclk not on the active configuration, but
on all the hardware detected for the system whether enabled or not.

There are two basic changes here, the cleanup and improvement to the logic
based on what type of output is connected and the second change to
determine which outputs are active.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:43 -0700, Keith Packard <keithp@keithp.com> wrote:
> The PCH refclk settings are global, so we need to look at all of the
> encoders, not just the current encoder when deciding how to configure
> it. Also, handle systems with more than one panel (any combination of
> PCH/non-PCH eDP and LVDS).

As I read it, this sets the refclk not on the active configuration, but
on all the hardware detected for the system whether enabled or not.

There are two basic changes here, the cleanup and improvement to the logic
based on what type of output is connected and the second change to
determine which outputs are active.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: Use CK505 as non-SSC source where available

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:44 -0700, Keith Packard <keithp@keithp.com> wrote:
> This eliminates VGA shimmer on some Ironlake machines which have a
> CK505 clock source.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
References: https://bugzilla.kernel.org/show_bug.cgi?id=21742
References: https://bugs.freedesktop.org/show_bug.cgi?id=38750
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 7/9] drm/i915: Use CK505 as non-SSC source where available

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:44 -0700, Keith Packard <keithp@keithp.com> wrote:
> This eliminates VGA shimmer on some Ironlake machines which have a
> CK505 clock source.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
References: https://bugzilla.kernel.org/show_bug.cgi?id=21742
References: https://bugs.freedesktop.org/show_bug.cgi?id=38750
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/9] drm/i915: All PCH refclks are 120MHz

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:45 -0700, Keith Packard <keithp@keithp.com> wrote:
> I can't find any reference clocks which run at 96MHz as seems to be
> indicated from the comments in this code.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

I think there exists a 100MHz test mode (certainly there is reference to
such in the diagram and DPCTL). But as we never use that, this should be
true.

I could find no reference at all to the 96MHz clock, so from that
perspective,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 8/9] drm/i915: All PCH refclks are 120MHz

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:45 -0700, Keith Packard <keithp@keithp.com> wrote:
> I can't find any reference clocks which run at 96MHz as seems to be
> indicated from the comments in this code.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>

I think there exists a 100MHz test mode (certainly there is reference to
such in the diagram and DPCTL). But as we never use that, this should be
true.

I could find no reference at all to the 96MHz clock, so from that
perspective,
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: PCH reference clock cleanups

[Keith Packard]

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

On Tue, 27 Sep 2011 10:01:33 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Oddly in the diagram SSC4 is given as a 100MHz clock that can be used for
> any output other than DP_A. However, the configuration register marks that
> as being a test-only mode.

Ok, it's all irrelevant -- the only configurations using anything other
than a fixed 120MHz were eDP and LVDS. LVDS used a value from the BIOS, which is
presumably always 120MHz. eDP ignored the refclk and used fixed PLL
values.

So, yes, we can always set the refclk to 120MHz; the cases which matter
were using that value already.

-- 
keith.packard@intel.com

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

Re: [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:46 -0700, Keith Packard <keithp@keithp.com> wrote:
> The reference clock configuration must be done before any mode setting
> can occur as all outputs must be disabled to change
> anything. Initialize the clocks after turning everything off during
> the initialization process.

Ah, now I see why we moved from using the active configuration earlier. ;-)

Doesn't this prevent us from ever using SSC though, as virtually every
single PCH machine has HDMI encoders that haven't been masked out through
the chicken fuses or VBT?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Chris Wilson]

On Mon, 26 Sep 2011 23:11:46 -0700, Keith Packard <keithp@keithp.com> wrote:
> The reference clock configuration must be done before any mode setting
> can occur as all outputs must be disabled to change
> anything. Initialize the clocks after turning everything off during
> the initialization process.

Ah, now I see why we moved from using the active configuration earlier. ;-)

Doesn't this prevent us from ever using SSC though, as virtually every
single PCH machine has HDMI encoders that haven't been masked out through
the chicken fuses or VBT?
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Keith Packard]

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

On Tue, 27 Sep 2011 17:47:10 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Mon, 26 Sep 2011 23:11:43 -0700, Keith Packard <keithp@keithp.com> wrote:
> > The PCH refclk settings are global, so we need to look at all of the
> > encoders, not just the current encoder when deciding how to configure
> > it. Also, handle systems with more than one panel (any combination of
> > PCH/non-PCH eDP and LVDS).
> 
> As I read it, this sets the refclk not on the active configuration, but
> on all the hardware detected for the system whether enabled or not.

Correct. We cannot randomly turn ref clocks on/off without also
disconnecting them from the PLLs that they drive.

What we could do is figure out which of the two clocks need to be
enabled and modify the mode set code to turn them on when needed before
setting the mode, and then turn them off after, when they aren't
needed. This would leave them off until needed, which might be nice?

This will make changing the driver to not disable the panel at startup
time harder; we'll need to switch the panel to the non-SSC reference,
turn the SSC reference off, reconfigure it and then switch the panel
back to the SSC reference. That's a project for a future change though.

> There are two basic changes here, the cleanup and improvement to the logic
> based on what type of output is connected and the second change to
> determine which outputs are active.

Right, the logic fixes ensure that the clocks are programmed in the
right sequence and that LVDS, eDP and pch-EDP all get SSC as necessary.

The change in dealing with the outputs means that the clocks are
programmed based not on which outputs are active, but on all possible
outputs, ensuring that the programming never changes in response to mode
setting requests.

-- 
keith.packard@intel.com

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

Re: [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Keith Packard]

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

On Tue, 27 Sep 2011 17:56:39 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:

> Ah, now I see why we moved from using the active configuration earlier. ;-)

My evil plan is revealed!

> Doesn't this prevent us from ever using SSC though, as virtually every
> single PCH machine has HDMI encoders that haven't been masked out through
> the chicken fuses or VBT?

That wasn't my intent -- the SSC source gets modulated whenever the VBT
table says it can, so when the panel uses the SSC source, it will get
SSC. Did I mess something up here? Or is it just some interaction with
the mode setting code that I didn't get right?

-- 
keith.packard@intel.com

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

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

[Chris Wilson]

On Tue, 27 Sep 2011 11:03:43 -0700, Keith Packard <keithp@keithp.com> wrote:
Non-text part: multipart/signed
> On Tue, 27 Sep 2011 17:47:10 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Mon, 26 Sep 2011 23:11:43 -0700, Keith Packard <keithp@keithp.com> wrote:
> > > The PCH refclk settings are global, so we need to look at all of the
> > > encoders, not just the current encoder when deciding how to configure
> > > it. Also, handle systems with more than one panel (any combination of
> > > PCH/non-PCH eDP and LVDS).
> > 
> > As I read it, this sets the refclk not on the active configuration, but
> > on all the hardware detected for the system whether enabled or not.
> 
> Correct. We cannot randomly turn ref clocks on/off without also
> disconnecting them from the PLLs that they drive.
> 
> What we could do is figure out which of the two clocks need to be
> enabled and modify the mode set code to turn them on when needed before
> setting the mode, and then turn them off after, when they aren't
> needed. This would leave them off until needed, which might be nice?
> 
> This will make changing the driver to not disable the panel at startup
> time harder; we'll need to switch the panel to the non-SSC reference,
> turn the SSC reference off, reconfigure it and then switch the panel
> back to the SSC reference. That's a project for a future change though.

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.

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!

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.

As an incremental improvement [in my understanding ;-]:
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

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

[Keith Packard]

[-- 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 --]

Re: [Intel-gfx] PCH reference clock cleanups

[Paulo Zanoni]

2011/9/27 Keith Packard <keithp@keithp.com>:
> Here's a patch sequence which cleans up a bunch of PCH refclk related
> bits.

For the series: Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Tested all the patches on Ironlake (LVDS + VGA). Fixes fd.o bug #38750 for me.

I also tested the patch you sent today 1 hour ago (inline in one of
the emails) and things still work with it. I'll keep using these
patches since they fix my laptop. Any problem will be reported.

Maybe my email client/server is ruining things, but I believe patch 7
includes whitespace errors.

> There are a couple of questionable patches that I'd like to see
> people look at:
>
>  [PATCH 6/9] drm/i915: Fix PCH SSC reference clock settings
>  [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time
>
> Here's the main patch -- this looks at the global set of encoders and
> figures out what the refclk should be to make all of those work
> correctly. Nothing is dependent on the active configuration, so we
> aren't reprogramming this register during run-time. The last patch in
> the sequence moves the setting of this register from modeset time to
> init time.
>
>  [PATCH 7/9] drm/i915: Use CK505 as non-SSC source where available
>
> This is a small piece straight from Jesse's patch; just uses the VBT
> configuration for CK505 clock sources.
>
>  [PATCH 8/9] drm/i915: All PCH refclks are 120MHz
>
> Ok, so I'd love to know where in any PCH reference matter someone has
> found a place where the reference clock for any of the PLLs is
> anything other than 120MHz. Can someone find a reference for other frequencies?
>
> --
> keith.packard@intel.com
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>

-- 
Paulo Zanoni

Re: [Intel-gfx] PCH reference clock cleanups

[Keith Packard]

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

On Wed, 28 Sep 2011 15:22:48 -0300, Paulo Zanoni <przanoni@gmail.com> wrote:

> I also tested the patch you sent today 1 hour ago (inline in one of
> the emails) and things still work with it. I'll keep using these
> patches since they fix my laptop. Any problem will be reported.

Thanks. I think we're failing to reset the register on resume, leading
to a black screen for me. I may just break down and fix this the 'right'
way, enabling and disabling the clock sources during mode setting,
depending on which outputs are in use.

-- 
keith.packard@intel.com

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

[PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Keith Packard]

The reference clock configuration must be done before any mode setting
can occur as all outputs must be disabled to change
anything. Initialize the clocks after turning everything off during
the initialization process.

Also, re-initialize the refclk at resume time.

Signed-off-by: Keith Packard <keithp@keithp.com>
---

v2 - re-initialize the refclk at resume time.

 drivers/gpu/drm/i915/i915_drv.c      |    3 +++
 drivers/gpu/drm/i915/i915_drv.h      |    1 +
 drivers/gpu/drm/i915/intel_display.c |   10 +++++++---
 3 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.c b/drivers/gpu/drm/i915/i915_drv.c
index 58480de..2b6c2d2 100644
--- a/drivers/gpu/drm/i915/i915_drv.c
+++ b/drivers/gpu/drm/i915/i915_drv.c
@@ -471,6 +471,9 @@ static int i915_drm_thaw(struct drm_device *dev)
 		error = i915_gem_init_ringbuffer(dev);
 		mutex_unlock(&dev->struct_mutex);
 
+		if (HAS_PCH_SPLIT(dev))
+			ironlake_init_pch_refclk(dev);
+
 		drm_mode_config_reset(dev);
 		drm_irq_install(dev);
 
diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 18df595..98f2e0b 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -1302,6 +1302,7 @@ extern int intel_modeset_vga_set_state(struct drm_device *dev, bool state);
 extern bool intel_fbc_enabled(struct drm_device *dev);
 extern void intel_disable_fbc(struct drm_device *dev);
 extern bool ironlake_set_drps(struct drm_device *dev, u8 val);
+extern void ironlake_init_pch_refclk(struct drm_device *dev);
 extern void ironlake_enable_rc6(struct drm_device *dev);
 extern void gen6_set_rps(struct drm_device *dev, u8 val);
 extern void intel_detect_pch (struct drm_device *dev);
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b072a35..91d7d5ed 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5109,7 +5109,10 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 	return ret;
 }
 
-static void ironlake_update_pch_refclk(struct drm_device *dev)
+/*
+ * Initialize reference clocks when the driver loads
+ */
+void ironlake_init_pch_refclk(struct drm_device *dev)
 {
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct drm_mode_config *mode_config = &dev->mode_config;
@@ -5411,8 +5414,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
 			     &m_n);
 
-	ironlake_update_pch_refclk(dev);
-
 	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
 	if (has_reduced_clock)
 		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
@@ -7284,6 +7285,9 @@ static void intel_setup_outputs(struct drm_device *dev)
 
 	/* disable all the possible outputs/crtcs before entering KMS mode */
 	drm_helper_disable_unused_functions(dev);
+
+	if (HAS_PCH_SPLIT(dev))
+		ironlake_init_pch_refclk(dev);
 }
 
 static void intel_user_framebuffer_destroy(struct drm_framebuffer *fb)
-- 
1.7.6.3

Re: [Intel-gfx] [PATCH 9/9] drm/i915: Initialize PCH refclks at modeset init time

[Jesse Barnes]

On Tue, 27 Sep 2011 11:11:57 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Tue, 27 Sep 2011 17:56:39 +0100, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> 
> > Ah, now I see why we moved from using the active configuration earlier. ;-)
> 
> My evil plan is revealed!
> 
> > Doesn't this prevent us from ever using SSC though, as virtually every
> > single PCH machine has HDMI encoders that haven't been masked out through
> > the chicken fuses or VBT?
> 
> That wasn't my intent -- the SSC source gets modulated whenever the VBT
> table says it can, so when the panel uses the SSC source, it will get
> SSC. Did I mess something up here? Or is it just some interaction with
> the mode setting code that I didn't get right?

Assuming we're selecting the proper reference clock in the PLL
selection code anyway...

Doing it all up front seems nicer; did you get confirmation that the
"wavy VGA" bug was fixed with this series?  Overall seems like a good
improvement over our old PCH refclk code...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] PCH reference clock cleanups

[Jesse Barnes]

On Wed, 28 Sep 2011 15:22:48 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2011/9/27 Keith Packard <keithp@keithp.com>:
> > Here's a patch sequence which cleans up a bunch of PCH refclk related
> > bits.
> 
> For the series: Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Tested all the patches on Ironlake (LVDS + VGA). Fixes fd.o bug #38750 for me.
> 
> I also tested the patch you sent today 1 hour ago (inline in one of
> the emails) and things still work with it. I'll keep using these
> patches since they fix my laptop. Any problem will be reported.
> 
> Maybe my email client/server is ruining things, but I believe patch 7
> includes whitespace errors.

Yay excellent.

Now... is keeping the various refclks enabled costing us any power?
IOW, should we be trying to disable them when everything has been
DPMS'd off too?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: PCH reference clock cleanups

[Jesse Barnes]

On Wed, 28 Sep 2011 15:22:48 -0300
Paulo Zanoni <przanoni@gmail.com> wrote:

> 2011/9/27 Keith Packard <keithp@keithp.com>:
> > Here's a patch sequence which cleans up a bunch of PCH refclk related
> > bits.
> 
> For the series: Tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Tested all the patches on Ironlake (LVDS + VGA). Fixes fd.o bug #38750 for me.
> 
> I also tested the patch you sent today 1 hour ago (inline in one of
> the emails) and things still work with it. I'll keep using these
> patches since they fix my laptop. Any problem will be reported.
> 
> Maybe my email client/server is ruining things, but I believe patch 7
> includes whitespace errors.

Yay excellent.

Now... is keeping the various refclks enabled costing us any power?
IOW, should we be trying to disable them when everything has been
DPMS'd off too?

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] PCH reference clock cleanups

[Keith Packard]

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

On Mon, 3 Oct 2011 14:14:23 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Now... is keeping the various refclks enabled costing us any power?
> IOW, should we be trying to disable them when everything has been
> DPMS'd off too?

That's the same as tracking usage and enabling/disabling on the fly as
modes are set. I think it's possible, but I'd like to have the 'simpler'
fix present before we try a power saving move.

-- 
keith.packard@intel.com

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

Re: [Intel-gfx] PCH reference clock cleanups

[Jesse Barnes]

On Mon, 03 Oct 2011 16:18:48 -0700
Keith Packard <keithp@keithp.com> wrote:

> On Mon, 3 Oct 2011 14:14:23 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> 
> > Now... is keeping the various refclks enabled costing us any power?
> > IOW, should we be trying to disable them when everything has been
> > DPMS'd off too?
> 
> That's the same as tracking usage and enabling/disabling on the fly as
> modes are set. I think it's possible, but I'd like to have the 'simpler'
> fix present before we try a power saving move.

Agreed; fortunately shutting everything off when no outputs are
active should be simpler than trying flip the bits on & off every mode
set. :)

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [Intel-gfx] PCH reference clock cleanups

[Keith Packard]

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

On Mon, 3 Oct 2011 16:21:08 -0700, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:

> Agreed; fortunately shutting everything off when no outputs are
> active should be simpler than trying flip the bits on & off every mode
> set. :)

We'd have to track when outputs are shut off; just tracking per clock
doesn't seem any harder. I'll expect a patch in the morning, ok?

-- 
keith.packard@intel.com

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