| Message ID | 1385400428-1597-1-git-send-email-przanoni@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 :) */
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
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
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);