* [PATCH 1/2] drm/i915: reuse WRPLL when possible @ 2013-10-30 20:27 Paulo Zanoni 2013-10-30 20:27 ` [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces Paulo Zanoni 2013-11-18 12:01 ` [PATCH 1/2] drm/i915: reuse WRPLL when possible Damien Lespiau 0 siblings, 2 replies; 9+ messages in thread From: Paulo Zanoni @ 2013-10-30 20:27 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni From: Paulo Zanoni <paulo.r.zanoni@intel.com> It seems we do have machines with 3 HDMI/DVI outputs, so sharing WRPLLs is the only way to get 3 pipes working. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68485 Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 37 ++++++++++++++++++++++--------------- 1 file changed, 22 insertions(+), 15 deletions(-) It is that easy because I already planned to enable PLL sharing when I wrote the original code :) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 31f4fe2..f2144b2 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -636,8 +636,6 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) uint32_t reg, val; int clock = intel_crtc->config.port_clock; - /* TODO: reuse PLLs when possible (compare values) */ - intel_ddi_put_crtc_pll(crtc); if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { @@ -665,31 +663,40 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) } else if (type == INTEL_OUTPUT_HDMI) { unsigned p, n2, r2; - if (plls->wrpll1_refcount == 0) { + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); + + val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | + WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) | + WRPLL_DIVIDER_POST(p); + + if (val == I915_READ(WRPLL_CTL1)) { + DRM_DEBUG_KMS("Reusing WRPLL 1 on pipe %c\n", + pipe_name(pipe)); + reg = WRPLL_CTL1; + } else if (val == I915_READ(WRPLL_CTL2)) { + DRM_DEBUG_KMS("Reusing WRPLL 2 on pipe %c\n", + pipe_name(pipe)); + reg = WRPLL_CTL2; + } else if (plls->wrpll1_refcount == 0) { DRM_DEBUG_KMS("Using WRPLL 1 on pipe %c\n", pipe_name(pipe)); - plls->wrpll1_refcount++; reg = WRPLL_CTL1; - intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; } else if (plls->wrpll2_refcount == 0) { DRM_DEBUG_KMS("Using WRPLL 2 on pipe %c\n", pipe_name(pipe)); - plls->wrpll2_refcount++; reg = WRPLL_CTL2; - intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2; } else { DRM_ERROR("No WRPLLs available!\n"); return false; } - WARN(I915_READ(reg) & WRPLL_PLL_ENABLE, - "WRPLL already enabled\n"); - - intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); - - val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | - WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) | - WRPLL_DIVIDER_POST(p); + if (reg == WRPLL_CTL1) { + plls->wrpll1_refcount++; + intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; + } else { + plls->wrpll2_refcount++; + intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2; + } } else if (type == INTEL_OUTPUT_ANALOG) { if (plls->spll_refcount == 0) { -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-10-30 20:27 [PATCH 1/2] drm/i915: reuse WRPLL when possible Paulo Zanoni @ 2013-10-30 20:27 ` Paulo Zanoni 2013-11-18 12:06 ` Damien Lespiau 2013-11-18 12:01 ` [PATCH 1/2] drm/i915: reuse WRPLL when possible Damien Lespiau 1 sibling, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-10-30 20:27 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni From: Paulo Zanoni <paulo.r.zanoni@intel.com> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to the CRTC, but doesn't write any register. It can also fail in case it doesn't find a PLL. The second piece, intel_ddi_pll_enable, uses the information stored by intel_ddi_pll_select to actually enable the PLL by writing to its register. This function can't fail. We also have some refcount sanity checks here. The idea is that one day we'll remove all the functions that touch registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll call intel_ddi_pll_select at haswell_crtc_mode_set and then call intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already touching this code, let's take care of this particular split today. Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 101 ++++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_display.c | 3 +- drivers/gpu/drm/i915/intel_drv.h | 3 +- 3 files changed, 87 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index f2144b2..9d9ed18 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -619,21 +619,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */, *n2_out = best.n2; *p_out = best.p; *r2_out = best.r2; - - DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", - clock, *p_out, *n2_out, *r2_out); } -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually + * enable the PLL. */ +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_crtc *crtc = &intel_crtc->base; struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); struct drm_encoder *encoder = &intel_encoder->base; struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_ddi_plls *plls = &dev_priv->ddi_plls; int type = intel_encoder->type; enum pipe pipe = intel_crtc->pipe; - uint32_t reg, val; int clock = intel_crtc->config.port_clock; intel_ddi_put_crtc_pll(crtc); @@ -657,10 +657,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) return false; } - /* We don't need to turn any PLL on because we'll use LCPLL. */ - return true; - } else if (type == INTEL_OUTPUT_HDMI) { + uint32_t reg, val; unsigned p, n2, r2; intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); @@ -690,6 +688,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) return false; } + DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", + clock, p, n2, r2); + if (reg == WRPLL_CTL1) { plls->wrpll1_refcount++; intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; @@ -703,29 +704,93 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) DRM_DEBUG_KMS("Using SPLL on pipe %c\n", pipe_name(pipe)); plls->spll_refcount++; - reg = SPLL_CTL; intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; } else { DRM_ERROR("SPLL already in use\n"); return false; } - WARN(I915_READ(reg) & SPLL_PLL_ENABLE, - "SPLL already enabled\n"); - - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; - } else { WARN(1, "Invalid DDI encoder type %d\n", type); return false; } - I915_WRITE(reg, val); - udelay(20); - return true; } +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be + * used, this one actually enables the PLL. */ +void intel_ddi_pll_enable(struct intel_crtc *crtc) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ddi_plls *plls = &dev_priv->ddi_plls; + int clock = crtc->config.port_clock; + uint32_t reg, cur_val, new_val; + int refcount; + const char *pll_name; + uint32_t enable_bit = (1 << 31); + unsigned int p, n2, r2; + + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE); + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE); + + switch (crtc->ddi_pll_sel) { + case PORT_CLK_SEL_LCPLL_2700: + case PORT_CLK_SEL_LCPLL_1350: + case PORT_CLK_SEL_LCPLL_810: + /* LCPLL should always be enabled at this point of the mode set + * sequence, so nothing to do. */ + return; + + case PORT_CLK_SEL_SPLL: + pll_name = "SPLL"; + reg = SPLL_CTL; + refcount = plls->spll_refcount; + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | + SPLL_PLL_SSC; + break; + + case PORT_CLK_SEL_WRPLL1: + case PORT_CLK_SEL_WRPLL2: + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) { + pll_name = "WRPLL1"; + reg = WRPLL_CTL1; + refcount = plls->wrpll1_refcount; + } else { + pll_name = "WRPLL2"; + reg = WRPLL_CTL2; + refcount = plls->wrpll2_refcount; + } + + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); + + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | + WRPLL_DIVIDER_REFERENCE(r2) | + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); + + break; + + case PORT_CLK_SEL_NONE: + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n"); + return; + default: + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel); + return; + } + + cur_val = I915_READ(reg); + + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount); + if (refcount == 1) { + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name); + I915_WRITE(reg, new_val); + udelay(20); + } else { + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name); + } +} + void intel_ddi_set_pipe_settings(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index 3c3d199..e841cd7 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6661,8 +6661,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, int plane = intel_crtc->plane; int ret; - if (!intel_ddi_pll_mode_set(crtc)) + if (!intel_ddi_pll_select(intel_crtc)) return -EINVAL; + intel_ddi_pll_enable(intel_crtc); if (intel_crtc->config.has_dp_encoder) intel_dp_set_m_n(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 9d2624f..8857dec 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -601,7 +601,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); void intel_ddi_setup_hw_pll_state(struct drm_device *dev); -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc); +bool intel_ddi_pll_select(struct intel_crtc *crtc); +void intel_ddi_pll_enable(struct intel_crtc *crtc); void intel_ddi_put_crtc_pll(struct drm_crtc *crtc); void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-10-30 20:27 ` [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces Paulo Zanoni @ 2013-11-18 12:06 ` Damien Lespiau 2013-11-25 17:27 ` [PATCH] " Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Damien Lespiau @ 2013-11-18 12:06 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Wed, Oct 30, 2013 at 06:27:44PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > @@ -657,10 +657,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > return false; > } > > - /* We don't need to turn any PLL on because we'll use LCPLL. */ > - return true; > - > } else if (type == INTEL_OUTPUT_HDMI) { > + uint32_t reg, val; > unsigned p, n2, r2; > > intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > @@ -690,6 +688,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > return false; > } > > + DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", > + clock, p, n2, r2); clock is now in KHz > + > if (reg == WRPLL_CTL1) { > plls->wrpll1_refcount++; > intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; > @@ -703,29 +704,93 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > DRM_DEBUG_KMS("Using SPLL on pipe %c\n", > pipe_name(pipe)); > plls->spll_refcount++; > - reg = SPLL_CTL; > intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; > } else { > DRM_ERROR("SPLL already in use\n"); > return false; > } > > - WARN(I915_READ(reg) & SPLL_PLL_ENABLE, > - "SPLL already enabled\n"); > - > - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; > - > } else { > WARN(1, "Invalid DDI encoder type %d\n", type); > return false; > } > > - I915_WRITE(reg, val); > - udelay(20); > - > return true; > } > > +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be > + * used, this one actually enables the PLL. */ > +void intel_ddi_pll_enable(struct intel_crtc *crtc) > +{ > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ddi_plls *plls = &dev_priv->ddi_plls; > + int clock = crtc->config.port_clock; > + uint32_t reg, cur_val, new_val; > + int refcount; > + const char *pll_name; > + uint32_t enable_bit = (1 << 31); > + unsigned int p, n2, r2; > + > + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE); > + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE); > + > + switch (crtc->ddi_pll_sel) { > + case PORT_CLK_SEL_LCPLL_2700: > + case PORT_CLK_SEL_LCPLL_1350: > + case PORT_CLK_SEL_LCPLL_810: > + /* LCPLL should always be enabled at this point of the mode set > + * sequence, so nothing to do. */ > + return; > + > + case PORT_CLK_SEL_SPLL: > + pll_name = "SPLL"; > + reg = SPLL_CTL; > + refcount = plls->spll_refcount; > + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | > + SPLL_PLL_SSC; > + break; > + > + case PORT_CLK_SEL_WRPLL1: > + case PORT_CLK_SEL_WRPLL2: > + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) { > + pll_name = "WRPLL1"; > + reg = WRPLL_CTL1; > + refcount = plls->wrpll1_refcount; > + } else { > + pll_name = "WRPLL2"; > + reg = WRPLL_CTL2; > + refcount = plls->wrpll2_refcount; > + } > + > + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > + > + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | > + WRPLL_DIVIDER_REFERENCE(r2) | > + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); > + > + break; > + > + case PORT_CLK_SEL_NONE: > + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n"); > + return; > + default: > + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel); > + return; > + } > + > + cur_val = I915_READ(reg); > + > + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount); > + if (refcount == 1) { > + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name); > + I915_WRITE(reg, new_val); > + udelay(20); I was thinking that we may want a posting read before the wait here. > + } else { > + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name); > + } > +} > + > void intel_ddi_set_pipe_settings(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index 3c3d199..e841cd7 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6661,8 +6661,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > int plane = intel_crtc->plane; > int ret; > > - if (!intel_ddi_pll_mode_set(crtc)) > + if (!intel_ddi_pll_select(intel_crtc)) > return -EINVAL; > + intel_ddi_pll_enable(intel_crtc); > > if (intel_crtc->config.has_dp_encoder) > intel_dp_set_m_n(intel_crtc); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 9d2624f..8857dec 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -601,7 +601,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, > void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); > void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); > void intel_ddi_setup_hw_pll_state(struct drm_device *dev); > -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc); > +bool intel_ddi_pll_select(struct intel_crtc *crtc); > +void intel_ddi_pll_enable(struct intel_crtc *crtc); > void intel_ddi_put_crtc_pll(struct drm_crtc *crtc); > void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); > void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-11-18 12:06 ` Damien Lespiau @ 2013-11-25 17:27 ` Paulo Zanoni 2013-12-12 10:14 ` Damien Lespiau 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-11-25 17:27 UTC (permalink / raw) To: intel-gfx; +Cc: Paulo Zanoni From: Paulo Zanoni <paulo.r.zanoni@intel.com> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to the CRTC, but doesn't write any register. It can also fail in case it doesn't find a PLL. The second piece, intel_ddi_pll_enable, uses the information stored by intel_ddi_pll_select to actually enable the PLL by writing to its register. This function can't fail. We also have some refcount sanity checks here. The idea is that one day we'll remove all the functions that touch registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll call intel_ddi_pll_select at haswell_crtc_mode_set and then call intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already touching this code, let's take care of this particular split today. v2: - Clock on the debug message is in KHz - Add missing POSTING_READ Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> --- drivers/gpu/drm/i915/intel_ddi.c | 102 ++++++++++++++++++++++++++++------- drivers/gpu/drm/i915/intel_display.c | 3 +- drivers/gpu/drm/i915/intel_drv.h | 3 +- 3 files changed, 88 insertions(+), 20 deletions(-) diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c index 731a919..4263ec3 100644 --- a/drivers/gpu/drm/i915/intel_ddi.c +++ b/drivers/gpu/drm/i915/intel_ddi.c @@ -696,21 +696,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */, *n2_out = best.n2; *p_out = best.p; *r2_out = best.r2; - - DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", - clock, *p_out, *n2_out, *r2_out); } -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually + * enable the PLL. */ +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc) { - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); + struct drm_crtc *crtc = &intel_crtc->base; struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); struct drm_encoder *encoder = &intel_encoder->base; struct drm_i915_private *dev_priv = crtc->dev->dev_private; struct intel_ddi_plls *plls = &dev_priv->ddi_plls; int type = intel_encoder->type; enum pipe pipe = intel_crtc->pipe; - uint32_t reg, val; int clock = intel_crtc->config.port_clock; intel_ddi_put_crtc_pll(crtc); @@ -734,10 +734,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) return false; } - /* We don't need to turn any PLL on because we'll use LCPLL. */ - return true; - } else if (type == INTEL_OUTPUT_HDMI) { + uint32_t reg, val; unsigned p, n2, r2; intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); @@ -767,6 +765,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) return false; } + DRM_DEBUG_KMS("WRPLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n", + clock, p, n2, r2); + if (reg == WRPLL_CTL1) { plls->wrpll1_refcount++; intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; @@ -780,29 +781,94 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) DRM_DEBUG_KMS("Using SPLL on pipe %c\n", pipe_name(pipe)); plls->spll_refcount++; - reg = SPLL_CTL; intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; } else { DRM_ERROR("SPLL already in use\n"); return false; } - WARN(I915_READ(reg) & SPLL_PLL_ENABLE, - "SPLL already enabled\n"); - - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; - } else { WARN(1, "Invalid DDI encoder type %d\n", type); return false; } - I915_WRITE(reg, val); - udelay(20); - return true; } +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be + * used, this one actually enables the PLL. */ +void intel_ddi_pll_enable(struct intel_crtc *crtc) +{ + struct drm_device *dev = crtc->base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + struct intel_ddi_plls *plls = &dev_priv->ddi_plls; + int clock = crtc->config.port_clock; + uint32_t reg, cur_val, new_val; + int refcount; + const char *pll_name; + uint32_t enable_bit = (1 << 31); + unsigned int p, n2, r2; + + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE); + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE); + + switch (crtc->ddi_pll_sel) { + case PORT_CLK_SEL_LCPLL_2700: + case PORT_CLK_SEL_LCPLL_1350: + case PORT_CLK_SEL_LCPLL_810: + /* LCPLL should always be enabled at this point of the mode set + * sequence, so nothing to do. */ + return; + + case PORT_CLK_SEL_SPLL: + pll_name = "SPLL"; + reg = SPLL_CTL; + refcount = plls->spll_refcount; + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | + SPLL_PLL_SSC; + break; + + case PORT_CLK_SEL_WRPLL1: + case PORT_CLK_SEL_WRPLL2: + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) { + pll_name = "WRPLL1"; + reg = WRPLL_CTL1; + refcount = plls->wrpll1_refcount; + } else { + pll_name = "WRPLL2"; + reg = WRPLL_CTL2; + refcount = plls->wrpll2_refcount; + } + + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); + + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | + WRPLL_DIVIDER_REFERENCE(r2) | + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); + + break; + + case PORT_CLK_SEL_NONE: + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n"); + return; + default: + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel); + return; + } + + cur_val = I915_READ(reg); + + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount); + if (refcount == 1) { + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name); + I915_WRITE(reg, new_val); + POSTING_READ(reg); + udelay(20); + } else { + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name); + } +} + void intel_ddi_set_pipe_settings(struct drm_crtc *crtc) { struct drm_i915_private *dev_priv = crtc->dev->dev_private; diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c index e85d838..667f641 100644 --- a/drivers/gpu/drm/i915/intel_display.c +++ b/drivers/gpu/drm/i915/intel_display.c @@ -6880,8 +6880,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, int plane = intel_crtc->plane; int ret; - if (!intel_ddi_pll_mode_set(crtc)) + if (!intel_ddi_pll_select(intel_crtc)) return -EINVAL; + intel_ddi_pll_enable(intel_crtc); if (intel_crtc->config.has_dp_encoder) intel_dp_set_m_n(intel_crtc); diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h index 0231281..76f52b6 100644 --- a/drivers/gpu/drm/i915/intel_drv.h +++ b/drivers/gpu/drm/i915/intel_drv.h @@ -612,7 +612,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); void intel_ddi_setup_hw_pll_state(struct drm_device *dev); -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc); +bool intel_ddi_pll_select(struct intel_crtc *crtc); +void intel_ddi_pll_enable(struct intel_crtc *crtc); void intel_ddi_put_crtc_pll(struct drm_crtc *crtc); void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); -- 1.8.3.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-11-25 17:27 ` [PATCH] " Paulo Zanoni @ 2013-12-12 10:14 ` Damien Lespiau 2013-12-12 13:17 ` Paulo Zanoni 0 siblings, 1 reply; 9+ messages in thread From: Damien Lespiau @ 2013-12-12 10:14 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > The first piece, intel_ddi_pll_select, finds a PLL and assigns it to > the CRTC, but doesn't write any register. It can also fail in case it > doesn't find a PLL. > > The second piece, intel_ddi_pll_enable, uses the information stored by > intel_ddi_pll_select to actually enable the PLL by writing to its > register. This function can't fail. We also have some refcount sanity > checks here. > > The idea is that one day we'll remove all the functions that touch > registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll > call intel_ddi_pll_select at haswell_crtc_mode_set and then call > intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already > touching this code, let's take care of this particular split today. > > v2: - Clock on the debug message is in KHz > - Add missing POSTING_READ > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> /* * As a side note, CodingStyle stipulates this is the preferred style * for multi-line comments. Really a small detail to keep in my in the * future :) */ -- Damien > --- > drivers/gpu/drm/i915/intel_ddi.c | 102 ++++++++++++++++++++++++++++------- > drivers/gpu/drm/i915/intel_display.c | 3 +- > drivers/gpu/drm/i915/intel_drv.h | 3 +- > 3 files changed, 88 insertions(+), 20 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 731a919..4263ec3 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -696,21 +696,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */, > *n2_out = best.n2; > *p_out = best.p; > *r2_out = best.r2; > - > - DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", > - clock, *p_out, *n2_out, *r2_out); > } > > -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and > + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to > + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually > + * enable the PLL. */ > +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc) > { > - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > + struct drm_crtc *crtc = &intel_crtc->base; > struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); > struct drm_encoder *encoder = &intel_encoder->base; > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > struct intel_ddi_plls *plls = &dev_priv->ddi_plls; > int type = intel_encoder->type; > enum pipe pipe = intel_crtc->pipe; > - uint32_t reg, val; > int clock = intel_crtc->config.port_clock; > > intel_ddi_put_crtc_pll(crtc); > @@ -734,10 +734,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > return false; > } > > - /* We don't need to turn any PLL on because we'll use LCPLL. */ > - return true; > - > } else if (type == INTEL_OUTPUT_HDMI) { > + uint32_t reg, val; > unsigned p, n2, r2; > > intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > @@ -767,6 +765,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > return false; > } > > + DRM_DEBUG_KMS("WRPLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n", > + clock, p, n2, r2); > + > if (reg == WRPLL_CTL1) { > plls->wrpll1_refcount++; > intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; > @@ -780,29 +781,94 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > DRM_DEBUG_KMS("Using SPLL on pipe %c\n", > pipe_name(pipe)); > plls->spll_refcount++; > - reg = SPLL_CTL; > intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; > } else { > DRM_ERROR("SPLL already in use\n"); > return false; > } > > - WARN(I915_READ(reg) & SPLL_PLL_ENABLE, > - "SPLL already enabled\n"); > - > - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; > - > } else { > WARN(1, "Invalid DDI encoder type %d\n", type); > return false; > } > > - I915_WRITE(reg, val); > - udelay(20); > - > return true; > } > > +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be > + * used, this one actually enables the PLL. */ > +void intel_ddi_pll_enable(struct intel_crtc *crtc) > +{ > + struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + struct intel_ddi_plls *plls = &dev_priv->ddi_plls; > + int clock = crtc->config.port_clock; > + uint32_t reg, cur_val, new_val; > + int refcount; > + const char *pll_name; > + uint32_t enable_bit = (1 << 31); > + unsigned int p, n2, r2; > + > + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE); > + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE); > + > + switch (crtc->ddi_pll_sel) { > + case PORT_CLK_SEL_LCPLL_2700: > + case PORT_CLK_SEL_LCPLL_1350: > + case PORT_CLK_SEL_LCPLL_810: > + /* LCPLL should always be enabled at this point of the mode set > + * sequence, so nothing to do. */ > + return; > + > + case PORT_CLK_SEL_SPLL: > + pll_name = "SPLL"; > + reg = SPLL_CTL; > + refcount = plls->spll_refcount; > + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | > + SPLL_PLL_SSC; > + break; > + > + case PORT_CLK_SEL_WRPLL1: > + case PORT_CLK_SEL_WRPLL2: > + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) { > + pll_name = "WRPLL1"; > + reg = WRPLL_CTL1; > + refcount = plls->wrpll1_refcount; > + } else { > + pll_name = "WRPLL2"; > + reg = WRPLL_CTL2; > + refcount = plls->wrpll2_refcount; > + } > + > + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > + > + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | > + WRPLL_DIVIDER_REFERENCE(r2) | > + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); > + > + break; > + > + case PORT_CLK_SEL_NONE: > + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n"); > + return; > + default: > + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel); > + return; > + } > + > + cur_val = I915_READ(reg); > + > + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount); > + if (refcount == 1) { > + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name); > + I915_WRITE(reg, new_val); > + POSTING_READ(reg); > + udelay(20); > + } else { > + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name); > + } > +} > + > void intel_ddi_set_pipe_settings(struct drm_crtc *crtc) > { > struct drm_i915_private *dev_priv = crtc->dev->dev_private; > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index e85d838..667f641 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -6880,8 +6880,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, > int plane = intel_crtc->plane; > int ret; > > - if (!intel_ddi_pll_mode_set(crtc)) > + if (!intel_ddi_pll_select(intel_crtc)) > return -EINVAL; > + intel_ddi_pll_enable(intel_crtc); > > if (intel_crtc->config.has_dp_encoder) > intel_dp_set_m_n(intel_crtc); > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 0231281..76f52b6 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -612,7 +612,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, > void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); > void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); > void intel_ddi_setup_hw_pll_state(struct drm_device *dev); > -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc); > +bool intel_ddi_pll_select(struct intel_crtc *crtc); > +void intel_ddi_pll_enable(struct intel_crtc *crtc); > void intel_ddi_put_crtc_pll(struct drm_crtc *crtc); > void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); > void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-12-12 10:14 ` Damien Lespiau @ 2013-12-12 13:17 ` Paulo Zanoni 2013-12-12 14:31 ` Daniel Vetter 0 siblings, 1 reply; 9+ messages in thread From: Paulo Zanoni @ 2013-12-12 13:17 UTC (permalink / raw) To: Damien Lespiau; +Cc: Intel Graphics Development, Paulo Zanoni 2013/12/12 Damien Lespiau <damien.lespiau@intel.com>: > On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote: >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> >> >> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to >> the CRTC, but doesn't write any register. It can also fail in case it >> doesn't find a PLL. >> >> The second piece, intel_ddi_pll_enable, uses the information stored by >> intel_ddi_pll_select to actually enable the PLL by writing to its >> register. This function can't fail. We also have some refcount sanity >> checks here. >> >> The idea is that one day we'll remove all the functions that touch >> registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll >> call intel_ddi_pll_select at haswell_crtc_mode_set and then call >> intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already >> touching this code, let's take care of this particular split today. >> >> v2: - Clock on the debug message is in KHz >> - Add missing POSTING_READ >> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > /* > * As a side note, CodingStyle stipulates this is the preferred style > * for multi-line comments. Really a small detail to keep in my in the > * future :) > */ I know, but our internal documentation says that "Multi-line comment delimiters (i.e. /* and */) do not need to be on empty lines.", so I was following this recommendation. Thanks for the review! > > -- > Damien > >> --- >> drivers/gpu/drm/i915/intel_ddi.c | 102 ++++++++++++++++++++++++++++------- >> drivers/gpu/drm/i915/intel_display.c | 3 +- >> drivers/gpu/drm/i915/intel_drv.h | 3 +- >> 3 files changed, 88 insertions(+), 20 deletions(-) >> >> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c >> index 731a919..4263ec3 100644 >> --- a/drivers/gpu/drm/i915/intel_ddi.c >> +++ b/drivers/gpu/drm/i915/intel_ddi.c >> @@ -696,21 +696,21 @@ intel_ddi_calculate_wrpll(int clock /* in Hz */, >> *n2_out = best.n2; >> *p_out = best.p; >> *r2_out = best.r2; >> - >> - DRM_DEBUG_KMS("WRPLL: %dHz refresh rate with p=%d, n2=%d r2=%d\n", >> - clock, *p_out, *n2_out, *r2_out); >> } >> >> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) >> +/* Tries to find a PLL for the CRTC. If it finds, it increases the refcount and >> + * stores it in intel_crtc->ddi_pll_sel, so other mode sets won't be able to >> + * steal the selected PLL. You need to call intel_ddi_pll_enable to actually >> + * enable the PLL. */ >> +bool intel_ddi_pll_select(struct intel_crtc *intel_crtc) >> { >> - struct intel_crtc *intel_crtc = to_intel_crtc(crtc); >> + struct drm_crtc *crtc = &intel_crtc->base; >> struct intel_encoder *intel_encoder = intel_ddi_get_crtc_encoder(crtc); >> struct drm_encoder *encoder = &intel_encoder->base; >> struct drm_i915_private *dev_priv = crtc->dev->dev_private; >> struct intel_ddi_plls *plls = &dev_priv->ddi_plls; >> int type = intel_encoder->type; >> enum pipe pipe = intel_crtc->pipe; >> - uint32_t reg, val; >> int clock = intel_crtc->config.port_clock; >> >> intel_ddi_put_crtc_pll(crtc); >> @@ -734,10 +734,8 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) >> return false; >> } >> >> - /* We don't need to turn any PLL on because we'll use LCPLL. */ >> - return true; >> - >> } else if (type == INTEL_OUTPUT_HDMI) { >> + uint32_t reg, val; >> unsigned p, n2, r2; >> >> intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); >> @@ -767,6 +765,9 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) >> return false; >> } >> >> + DRM_DEBUG_KMS("WRPLL: %dKHz refresh rate with p=%d, n2=%d r2=%d\n", >> + clock, p, n2, r2); >> + >> if (reg == WRPLL_CTL1) { >> plls->wrpll1_refcount++; >> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; >> @@ -780,29 +781,94 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) >> DRM_DEBUG_KMS("Using SPLL on pipe %c\n", >> pipe_name(pipe)); >> plls->spll_refcount++; >> - reg = SPLL_CTL; >> intel_crtc->ddi_pll_sel = PORT_CLK_SEL_SPLL; >> } else { >> DRM_ERROR("SPLL already in use\n"); >> return false; >> } >> >> - WARN(I915_READ(reg) & SPLL_PLL_ENABLE, >> - "SPLL already enabled\n"); >> - >> - val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | SPLL_PLL_SSC; >> - >> } else { >> WARN(1, "Invalid DDI encoder type %d\n", type); >> return false; >> } >> >> - I915_WRITE(reg, val); >> - udelay(20); >> - >> return true; >> } >> >> +/* To be called after intel_ddi_pll_select(). That one selects the PLL to be >> + * used, this one actually enables the PLL. */ >> +void intel_ddi_pll_enable(struct intel_crtc *crtc) >> +{ >> + struct drm_device *dev = crtc->base.dev; >> + struct drm_i915_private *dev_priv = dev->dev_private; >> + struct intel_ddi_plls *plls = &dev_priv->ddi_plls; >> + int clock = crtc->config.port_clock; >> + uint32_t reg, cur_val, new_val; >> + int refcount; >> + const char *pll_name; >> + uint32_t enable_bit = (1 << 31); >> + unsigned int p, n2, r2; >> + >> + BUILD_BUG_ON(enable_bit != SPLL_PLL_ENABLE); >> + BUILD_BUG_ON(enable_bit != WRPLL_PLL_ENABLE); >> + >> + switch (crtc->ddi_pll_sel) { >> + case PORT_CLK_SEL_LCPLL_2700: >> + case PORT_CLK_SEL_LCPLL_1350: >> + case PORT_CLK_SEL_LCPLL_810: >> + /* LCPLL should always be enabled at this point of the mode set >> + * sequence, so nothing to do. */ >> + return; >> + >> + case PORT_CLK_SEL_SPLL: >> + pll_name = "SPLL"; >> + reg = SPLL_CTL; >> + refcount = plls->spll_refcount; >> + new_val = SPLL_PLL_ENABLE | SPLL_PLL_FREQ_1350MHz | >> + SPLL_PLL_SSC; >> + break; >> + >> + case PORT_CLK_SEL_WRPLL1: >> + case PORT_CLK_SEL_WRPLL2: >> + if (crtc->ddi_pll_sel == PORT_CLK_SEL_WRPLL1) { >> + pll_name = "WRPLL1"; >> + reg = WRPLL_CTL1; >> + refcount = plls->wrpll1_refcount; >> + } else { >> + pll_name = "WRPLL2"; >> + reg = WRPLL_CTL2; >> + refcount = plls->wrpll2_refcount; >> + } >> + >> + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); >> + >> + new_val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | >> + WRPLL_DIVIDER_REFERENCE(r2) | >> + WRPLL_DIVIDER_FEEDBACK(n2) | WRPLL_DIVIDER_POST(p); >> + >> + break; >> + >> + case PORT_CLK_SEL_NONE: >> + WARN(1, "Bad selected pll: PORT_CLK_SEL_NONE\n"); >> + return; >> + default: >> + WARN(1, "Bad selected pll: 0x%08x\n", crtc->ddi_pll_sel); >> + return; >> + } >> + >> + cur_val = I915_READ(reg); >> + >> + WARN(refcount < 1, "Bad %s refcount: %d\n", pll_name, refcount); >> + if (refcount == 1) { >> + WARN(cur_val & enable_bit, "%s already enabled\n", pll_name); >> + I915_WRITE(reg, new_val); >> + POSTING_READ(reg); >> + udelay(20); >> + } else { >> + WARN((cur_val & enable_bit) == 0, "%s disabled\n", pll_name); >> + } >> +} >> + >> void intel_ddi_set_pipe_settings(struct drm_crtc *crtc) >> { >> struct drm_i915_private *dev_priv = crtc->dev->dev_private; >> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c >> index e85d838..667f641 100644 >> --- a/drivers/gpu/drm/i915/intel_display.c >> +++ b/drivers/gpu/drm/i915/intel_display.c >> @@ -6880,8 +6880,9 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc, >> int plane = intel_crtc->plane; >> int ret; >> >> - if (!intel_ddi_pll_mode_set(crtc)) >> + if (!intel_ddi_pll_select(intel_crtc)) >> return -EINVAL; >> + intel_ddi_pll_enable(intel_crtc); >> >> if (intel_crtc->config.has_dp_encoder) >> intel_dp_set_m_n(intel_crtc); >> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h >> index 0231281..76f52b6 100644 >> --- a/drivers/gpu/drm/i915/intel_drv.h >> +++ b/drivers/gpu/drm/i915/intel_drv.h >> @@ -612,7 +612,8 @@ void intel_ddi_disable_transcoder_func(struct drm_i915_private *dev_priv, >> void intel_ddi_enable_pipe_clock(struct intel_crtc *intel_crtc); >> void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc); >> void intel_ddi_setup_hw_pll_state(struct drm_device *dev); >> -bool intel_ddi_pll_mode_set(struct drm_crtc *crtc); >> +bool intel_ddi_pll_select(struct intel_crtc *crtc); >> +void intel_ddi_pll_enable(struct intel_crtc *crtc); >> void intel_ddi_put_crtc_pll(struct drm_crtc *crtc); >> void intel_ddi_set_pipe_settings(struct drm_crtc *crtc); >> void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder); >> -- >> 1.8.3.1 >> >> _______________________________________________ >> Intel-gfx mailing list >> Intel-gfx@lists.freedesktop.org >> http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Paulo Zanoni ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH] drm/i915: split intel_ddi_pll_mode_set in 2 pieces 2013-12-12 13:17 ` Paulo Zanoni @ 2013-12-12 14:31 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2013-12-12 14:31 UTC (permalink / raw) To: Paulo Zanoni; +Cc: Intel Graphics Development, Paulo Zanoni On Thu, Dec 12, 2013 at 11:17:39AM -0200, Paulo Zanoni wrote: > 2013/12/12 Damien Lespiau <damien.lespiau@intel.com>: > > On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote: > >> From: Paulo Zanoni <paulo.r.zanoni@intel.com> > >> > >> The first piece, intel_ddi_pll_select, finds a PLL and assigns it to > >> the CRTC, but doesn't write any register. It can also fail in case it > >> doesn't find a PLL. > >> > >> The second piece, intel_ddi_pll_enable, uses the information stored by > >> intel_ddi_pll_select to actually enable the PLL by writing to its > >> register. This function can't fail. We also have some refcount sanity > >> checks here. > >> > >> The idea is that one day we'll remove all the functions that touch > >> registers from haswell_crtc_mode_set to haswell_crtc_enable, so we'll > >> call intel_ddi_pll_select at haswell_crtc_mode_set and then call > >> intel_ddi_pll_enable at haswell_crtc_enable. Since I'm already > >> touching this code, let's take care of this particular split today. > >> > >> v2: - Clock on the debug message is in KHz > >> - Add missing POSTING_READ > >> > >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> > > > > /* > > * As a side note, CodingStyle stipulates this is the preferred style > > * for multi-line comments. Really a small detail to keep in my in the > > * future :) > > */ > > I know, but our internal documentation says that "Multi-line comment > delimiters (i.e. /* and */) do not need to be on empty lines.", so I > was following this recommendation. Documentation is now fixed (and also comments updated in the patch). > Thanks for the review! Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: reuse WRPLL when possible 2013-10-30 20:27 [PATCH 1/2] drm/i915: reuse WRPLL when possible Paulo Zanoni 2013-10-30 20:27 ` [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces Paulo Zanoni @ 2013-11-18 12:01 ` Damien Lespiau 2013-11-18 16:15 ` Daniel Vetter 1 sibling, 1 reply; 9+ messages in thread From: Damien Lespiau @ 2013-11-18 12:01 UTC (permalink / raw) To: Paulo Zanoni; +Cc: intel-gfx, Paulo Zanoni On Wed, Oct 30, 2013 at 06:27:43PM -0200, Paulo Zanoni wrote: > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > It seems we do have machines with 3 HDMI/DVI outputs, so sharing > WRPLLs is the only way to get 3 pipes working. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68485 > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> -- Damien > --- > drivers/gpu/drm/i915/intel_ddi.c | 37 ++++++++++++++++++++++--------------- > 1 file changed, 22 insertions(+), 15 deletions(-) > > It is that easy because I already planned to enable PLL sharing when I wrote the > original code :) > > > diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c > index 31f4fe2..f2144b2 100644 > --- a/drivers/gpu/drm/i915/intel_ddi.c > +++ b/drivers/gpu/drm/i915/intel_ddi.c > @@ -636,8 +636,6 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > uint32_t reg, val; > int clock = intel_crtc->config.port_clock; > > - /* TODO: reuse PLLs when possible (compare values) */ > - > intel_ddi_put_crtc_pll(crtc); > > if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) { > @@ -665,31 +663,40 @@ bool intel_ddi_pll_mode_set(struct drm_crtc *crtc) > } else if (type == INTEL_OUTPUT_HDMI) { > unsigned p, n2, r2; > > - if (plls->wrpll1_refcount == 0) { > + intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > + > + val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | > + WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) | > + WRPLL_DIVIDER_POST(p); > + > + if (val == I915_READ(WRPLL_CTL1)) { > + DRM_DEBUG_KMS("Reusing WRPLL 1 on pipe %c\n", > + pipe_name(pipe)); > + reg = WRPLL_CTL1; > + } else if (val == I915_READ(WRPLL_CTL2)) { > + DRM_DEBUG_KMS("Reusing WRPLL 2 on pipe %c\n", > + pipe_name(pipe)); > + reg = WRPLL_CTL2; > + } else if (plls->wrpll1_refcount == 0) { > DRM_DEBUG_KMS("Using WRPLL 1 on pipe %c\n", > pipe_name(pipe)); > - plls->wrpll1_refcount++; > reg = WRPLL_CTL1; > - intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; > } else if (plls->wrpll2_refcount == 0) { > DRM_DEBUG_KMS("Using WRPLL 2 on pipe %c\n", > pipe_name(pipe)); > - plls->wrpll2_refcount++; > reg = WRPLL_CTL2; > - intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2; > } else { > DRM_ERROR("No WRPLLs available!\n"); > return false; > } > > - WARN(I915_READ(reg) & WRPLL_PLL_ENABLE, > - "WRPLL already enabled\n"); > - > - intel_ddi_calculate_wrpll(clock * 1000, &r2, &n2, &p); > - > - val = WRPLL_PLL_ENABLE | WRPLL_PLL_SELECT_LCPLL_2700 | > - WRPLL_DIVIDER_REFERENCE(r2) | WRPLL_DIVIDER_FEEDBACK(n2) | > - WRPLL_DIVIDER_POST(p); > + if (reg == WRPLL_CTL1) { > + plls->wrpll1_refcount++; > + intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL1; > + } else { > + plls->wrpll2_refcount++; > + intel_crtc->ddi_pll_sel = PORT_CLK_SEL_WRPLL2; > + } > > } else if (type == INTEL_OUTPUT_ANALOG) { > if (plls->spll_refcount == 0) { > -- > 1.8.3.1 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH 1/2] drm/i915: reuse WRPLL when possible 2013-11-18 12:01 ` [PATCH 1/2] drm/i915: reuse WRPLL when possible Damien Lespiau @ 2013-11-18 16:15 ` Daniel Vetter 0 siblings, 0 replies; 9+ messages in thread From: Daniel Vetter @ 2013-11-18 16:15 UTC (permalink / raw) To: Damien Lespiau; +Cc: intel-gfx, Paulo Zanoni On Mon, Nov 18, 2013 at 12:01:13PM +0000, Damien Lespiau wrote: > On Wed, Oct 30, 2013 at 06:27:43PM -0200, Paulo Zanoni wrote: > > From: Paulo Zanoni <paulo.r.zanoni@intel.com> > > > > It seems we do have machines with 3 HDMI/DVI outputs, so sharing > > WRPLLs is the only way to get 3 pipes working. > > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=68485 > > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > > Reviewed-by: Damien Lespiau <damien.lespiau@intel.com> Queued for -next, thanks for the patch. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2013-12-12 14:30 UTC | newest] Thread overview: 9+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2013-10-30 20:27 [PATCH 1/2] drm/i915: reuse WRPLL when possible Paulo Zanoni 2013-10-30 20:27 ` [PATCH 2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces Paulo Zanoni 2013-11-18 12:06 ` Damien Lespiau 2013-11-25 17:27 ` [PATCH] " Paulo Zanoni 2013-12-12 10:14 ` Damien Lespiau 2013-12-12 13:17 ` Paulo Zanoni 2013-12-12 14:31 ` Daniel Vetter 2013-11-18 12:01 ` [PATCH 1/2] drm/i915: reuse WRPLL when possible Damien Lespiau 2013-11-18 16:15 ` Daniel Vetter
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).