Re: [Intel-gfx] [PATCH 1/8] drm/i915: Skip modifying PCH DREF if not changing clock sources
On Wed, 2013-02-06 at 11:10 +, Chris Wilson wrote: > 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 > --- > 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 d75c6a0..f1dbd01 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4749,7 +4749,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; > @@ -4792,70 +4792,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; > + Nitpick: this would be clearer in a seperate function with a 'check_only' param, then the below could be removed. > /* 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); > + I
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Skip modifying PCH DREF if not changing clock sources
On Thu, Feb 7, 2013 at 1:45 PM, Paulo Zanoni wrote: > 2013/2/6 Chris Wilson : >> 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 > > The patch looks correct, so: > Reviewed-by: Paulo Zanoni > > But this patch only saves 400us. I thought this was on Daniel's "don't > care" range (e.g., on "drm/i915: don't send DP "idle" pattern before > "normal" on HSW PORT_A" I was asked to keep a potentially bigger delay > that's not needed at all). For modeset operations it's on my don't care list, since that'll take much longer anyway. But this tries to avoid it in boot-up, where fastboot aims to completely avoid the modeset. And I'm toying around with pushing all the edid reading to workqueues. So this could very much be in the "I care" bucket ;-) Hence also my proposal to simply push this into the modeset sequence instead of making the code more complicated ... -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ___ Intel-gfx mailing list Intel-gfx@lists.freedesktop.org http://lists.freedesktop.org/mailman/listinfo/intel-gfx
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Skip modifying PCH DREF if not changing clock sources
Hi 2013/2/6 Chris Wilson : > 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 The patch looks correct, so: Reviewed-by: Paulo Zanoni But this patch only saves 400us. I thought this was on Daniel's "don't care" range (e.g., on "drm/i915: don't send DP "idle" pattern before "normal" on HSW PORT_A" I was asked to keep a potentially bigger delay that's not needed at all). > --- > 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 d75c6a0..f1dbd01 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4749,7 +4749,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; > @@ -4792,70 +4792,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_NONSP
Re: [Intel-gfx] [PATCH 1/8] drm/i915: Skip modifying PCH DREF if not changing clock sources
On Wed, Feb 06, 2013 at 11:10:21AM +, Chris Wilson wrote: > 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 One idea I've been pondering for the refclock stuff is to simply shovel this into the ->modeset_global_resources callback. That way no clever tricks are required, and we'd avoid this for all cases where fastboot works complety. Presumably ofc that the bios didn't set up a config which does not work. As a bonus, we could only set up the refclocks the new configuration actually needs, i.e. filter for encoder->crtc != NULL ... A bit a risky patch, but might uncover some subtle bugs if we do the refclock adjusting more often. Too insane? -Daniel > --- > 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 d75c6a0..f1dbd01 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4749,7 +4749,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; > @@ -4792,70 +4792,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_OU
[Intel-gfx] [PATCH 1/8] drm/i915: Skip modifying PCH DREF if not changing clock sources
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 --- 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 d75c6a0..f1dbd01 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -4749,7 +4749,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; @@ -4792,70 +4792,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_MAS