| Message ID | 1361309508-4901-2-git-send-email-jbarnes@virtuousgeek.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote: > From: Chris Wilson <chris@chris-wilson.co.uk> > > Modifying the clock sources (via the DREF control on the PCH) is a slow > multi-stage process as we need to let the clocks stabilise between each > stage. If we are not actually changing the clock sources, then we can > return early. > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > --- > drivers/gpu/drm/i915/intel_display.c | 83 +++++++++++++++++++++++++--------- > 1 file changed, 61 insertions(+), 22 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3e6dadf..f20555e 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4758,7 +4758,7 @@ 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; > struct intel_encoder *encoder; > - u32 temp; > + u32 val, final; > bool has_lvds = false; > bool has_cpu_edp = false; > bool has_pch_edp = false; > @@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) > * PCH B stepping, previous chipset stepping should be > * ignoring this setting. > */ > - temp = I915_READ(PCH_DREF_CONTROL); > + val = I915_READ(PCH_DREF_CONTROL); > + > + /* As we must carefully and slowly disable/enable each source in turn, > + * compute the final state we want first and check if we need to > + * make any changes at all. > + */ > + final = val; > + final &= ~DREF_NONSPREAD_SOURCE_MASK; > + if (has_ck505) > + final |= DREF_NONSPREAD_CK505_ENABLE; > + else > + final |= DREF_NONSPREAD_SOURCE_ENABLE; > + > + final &= ~DREF_SSC_SOURCE_MASK; > + final &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > + final &= ~DREF_SSC1_ENABLE; > + > + if (has_panel) { > + final |= DREF_SSC_SOURCE_ENABLE; > + > + if (intel_panel_use_ssc(dev_priv) && can_ssc) > + final |= DREF_SSC1_ENABLE; > + > + if (has_cpu_edp) { > + if (intel_panel_use_ssc(dev_priv) && can_ssc) > + final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; > + else > + final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; > + } else > + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > + } else { > + final |= DREF_SSC_SOURCE_DISABLE; > + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > + } This would be clearer in a separate function with a 'check_only' flag, then we could do away with the code below. --Imre > + > + if (final == val) > + return; > + > /* Always enable nonspread source */ > - temp &= ~DREF_NONSPREAD_SOURCE_MASK; > + val &= ~DREF_NONSPREAD_SOURCE_MASK; > > if (has_ck505) > - temp |= DREF_NONSPREAD_CK505_ENABLE; > + val |= DREF_NONSPREAD_CK505_ENABLE; > else > - temp |= DREF_NONSPREAD_SOURCE_ENABLE; > + val |= DREF_NONSPREAD_SOURCE_ENABLE; > > if (has_panel) { > - temp &= ~DREF_SSC_SOURCE_MASK; > - temp |= DREF_SSC_SOURCE_ENABLE; > + val &= ~DREF_SSC_SOURCE_MASK; > + val |= DREF_SSC_SOURCE_ENABLE; > > /* SSC must be turned on before enabling the CPU output */ > if (intel_panel_use_ssc(dev_priv) && can_ssc) { > DRM_DEBUG_KMS("Using SSC on panel\n"); > - temp |= DREF_SSC1_ENABLE; > + val |= DREF_SSC1_ENABLE; > } else > - temp &= ~DREF_SSC1_ENABLE; > + val &= ~DREF_SSC1_ENABLE; > > /* Get SSC going before enabling the outputs */ > - I915_WRITE(PCH_DREF_CONTROL, temp); > + I915_WRITE(PCH_DREF_CONTROL, val); > POSTING_READ(PCH_DREF_CONTROL); > udelay(200); > > - temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > + val &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > > /* Enable CPU source on CPU attached eDP */ > if (has_cpu_edp) { > if (intel_panel_use_ssc(dev_priv) && can_ssc) { > DRM_DEBUG_KMS("Using SSC on eDP\n"); > - temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; > + val |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; > } > else > - temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; > + val |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; > } else > - temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > + val |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > > - I915_WRITE(PCH_DREF_CONTROL, temp); > + I915_WRITE(PCH_DREF_CONTROL, val); > POSTING_READ(PCH_DREF_CONTROL); > udelay(200); > } else { > DRM_DEBUG_KMS("Disabling SSC entirely\n"); > > - temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > + val &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > > /* Turn off CPU output */ > - temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > + val |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > > - I915_WRITE(PCH_DREF_CONTROL, temp); > + I915_WRITE(PCH_DREF_CONTROL, val); > POSTING_READ(PCH_DREF_CONTROL); > udelay(200); > > /* Turn off the SSC source */ > - temp &= ~DREF_SSC_SOURCE_MASK; > - temp |= DREF_SSC_SOURCE_DISABLE; > + val &= ~DREF_SSC_SOURCE_MASK; > + val |= DREF_SSC_SOURCE_DISABLE; > > /* Turn off SSC1 */ > - temp &= ~ DREF_SSC1_ENABLE; > + val &= ~ DREF_SSC1_ENABLE; > > - I915_WRITE(PCH_DREF_CONTROL, temp); > + I915_WRITE(PCH_DREF_CONTROL, val); > POSTING_READ(PCH_DREF_CONTROL); > udelay(200); > } > + > + BUG_ON(val != final); > } > > /* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */
On Wed, 20 Mar 2013 14:17:00 +0200 Imre Deak <imre.deak@intel.com> wrote: > On Tue, 2013-02-19 at 13:31 -0800, Jesse Barnes wrote: > > From: Chris Wilson <chris@chris-wilson.co.uk> > > > > Modifying the clock sources (via the DREF control on the PCH) is a slow > > multi-stage process as we need to let the clocks stabilise between each > > stage. If we are not actually changing the clock sources, then we can > > return early. > > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk> > > --- > > drivers/gpu/drm/i915/intel_display.c | 83 +++++++++++++++++++++++++--------- > > 1 file changed, 61 insertions(+), 22 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 3e6dadf..f20555e 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -4758,7 +4758,7 @@ 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; > > struct intel_encoder *encoder; > > - u32 temp; > > + u32 val, final; > > bool has_lvds = false; > > bool has_cpu_edp = false; > > bool has_pch_edp = false; > > @@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) > > * PCH B stepping, previous chipset stepping should be > > * ignoring this setting. > > */ > > - temp = I915_READ(PCH_DREF_CONTROL); > > + val = I915_READ(PCH_DREF_CONTROL); > > + > > + /* As we must carefully and slowly disable/enable each source in turn, > > + * compute the final state we want first and check if we need to > > + * make any changes at all. > > + */ > > + final = val; > > + final &= ~DREF_NONSPREAD_SOURCE_MASK; > > + if (has_ck505) > > + final |= DREF_NONSPREAD_CK505_ENABLE; > > + else > > + final |= DREF_NONSPREAD_SOURCE_ENABLE; > > + > > + final &= ~DREF_SSC_SOURCE_MASK; > > + final &= ~DREF_CPU_SOURCE_OUTPUT_MASK; > > + final &= ~DREF_SSC1_ENABLE; > > + > > + if (has_panel) { > > + final |= DREF_SSC_SOURCE_ENABLE; > > + > > + if (intel_panel_use_ssc(dev_priv) && can_ssc) > > + final |= DREF_SSC1_ENABLE; > > + > > + if (has_cpu_edp) { > > + if (intel_panel_use_ssc(dev_priv) && can_ssc) > > + final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; > > + else > > + final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; > > + } else > > + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > > + } else { > > + final |= DREF_SSC_SOURCE_DISABLE; > > + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; > > + } > > This would be clearer in a separate function with a 'check_only' flag, > then we could do away with the code below. Yeah, it would shorten things a bit, but since we only have one caller right now I'll leave that cleanup alone.
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3e6dadf..f20555e 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4758,7 +4758,7 @@ 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; struct intel_encoder *encoder; - u32 temp; + u32 val, final; bool has_lvds = false; bool has_cpu_edp = false; bool has_pch_edp = false; @@ -4801,70 +4801,109 @@ static void ironlake_init_pch_refclk(struct drm_device *dev) * PCH B stepping, previous chipset stepping should be * ignoring this setting. */ - temp = I915_READ(PCH_DREF_CONTROL); + val = I915_READ(PCH_DREF_CONTROL); + + /* As we must carefully and slowly disable/enable each source in turn, + * compute the final state we want first and check if we need to + * make any changes at all. + */ + final = val; + final &= ~DREF_NONSPREAD_SOURCE_MASK; + if (has_ck505) + final |= DREF_NONSPREAD_CK505_ENABLE; + else + final |= DREF_NONSPREAD_SOURCE_ENABLE; + + final &= ~DREF_SSC_SOURCE_MASK; + final &= ~DREF_CPU_SOURCE_OUTPUT_MASK; + final &= ~DREF_SSC1_ENABLE; + + if (has_panel) { + final |= DREF_SSC_SOURCE_ENABLE; + + if (intel_panel_use_ssc(dev_priv) && can_ssc) + final |= DREF_SSC1_ENABLE; + + if (has_cpu_edp) { + if (intel_panel_use_ssc(dev_priv) && can_ssc) + final |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; + else + final |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; + } else + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; + } else { + final |= DREF_SSC_SOURCE_DISABLE; + final |= DREF_CPU_SOURCE_OUTPUT_DISABLE; + } + + if (final == val) + return; + /* Always enable nonspread source */ - temp &= ~DREF_NONSPREAD_SOURCE_MASK; + val &= ~DREF_NONSPREAD_SOURCE_MASK; if (has_ck505) - temp |= DREF_NONSPREAD_CK505_ENABLE; + val |= DREF_NONSPREAD_CK505_ENABLE; else - temp |= DREF_NONSPREAD_SOURCE_ENABLE; + val |= DREF_NONSPREAD_SOURCE_ENABLE; if (has_panel) { - temp &= ~DREF_SSC_SOURCE_MASK; - temp |= DREF_SSC_SOURCE_ENABLE; + val &= ~DREF_SSC_SOURCE_MASK; + val |= DREF_SSC_SOURCE_ENABLE; /* SSC must be turned on before enabling the CPU output */ if (intel_panel_use_ssc(dev_priv) && can_ssc) { DRM_DEBUG_KMS("Using SSC on panel\n"); - temp |= DREF_SSC1_ENABLE; + val |= DREF_SSC1_ENABLE; } else - temp &= ~DREF_SSC1_ENABLE; + val &= ~DREF_SSC1_ENABLE; /* Get SSC going before enabling the outputs */ - I915_WRITE(PCH_DREF_CONTROL, temp); + I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); - temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK; + val &= ~DREF_CPU_SOURCE_OUTPUT_MASK; /* Enable CPU source on CPU attached eDP */ if (has_cpu_edp) { if (intel_panel_use_ssc(dev_priv) && can_ssc) { DRM_DEBUG_KMS("Using SSC on eDP\n"); - temp |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; + val |= DREF_CPU_SOURCE_OUTPUT_DOWNSPREAD; } else - temp |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; + val |= DREF_CPU_SOURCE_OUTPUT_NONSPREAD; } else - temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE; + val |= DREF_CPU_SOURCE_OUTPUT_DISABLE; - I915_WRITE(PCH_DREF_CONTROL, temp); + I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); } else { DRM_DEBUG_KMS("Disabling SSC entirely\n"); - temp &= ~DREF_CPU_SOURCE_OUTPUT_MASK; + val &= ~DREF_CPU_SOURCE_OUTPUT_MASK; /* Turn off CPU output */ - temp |= DREF_CPU_SOURCE_OUTPUT_DISABLE; + val |= DREF_CPU_SOURCE_OUTPUT_DISABLE; - I915_WRITE(PCH_DREF_CONTROL, temp); + I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); /* Turn off the SSC source */ - temp &= ~DREF_SSC_SOURCE_MASK; - temp |= DREF_SSC_SOURCE_DISABLE; + val &= ~DREF_SSC_SOURCE_MASK; + val |= DREF_SSC_SOURCE_DISABLE; /* Turn off SSC1 */ - temp &= ~ DREF_SSC1_ENABLE; + val &= ~ DREF_SSC1_ENABLE; - I915_WRITE(PCH_DREF_CONTROL, temp); + I915_WRITE(PCH_DREF_CONTROL, val); POSTING_READ(PCH_DREF_CONTROL); udelay(200); } + + BUG_ON(val != final); } /* Sequence to enable CLKOUT_DP for FDI usage and configure PCH FDI I/O. */