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
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"),
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
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
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;
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
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 */
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); }
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
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);
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
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.
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
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; }
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;
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
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
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
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)
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
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?
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...
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)
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
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.
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
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.
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?
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.
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. :)
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?
dri-devel@lists.freedesktop.org
-
Chris Wilson -
Jesse Barnes -
Keith Packard -
Paulo Zanoni