On Mon, Nov 25, 2013 at 03:27:08PM -0200, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@xxxxxxxxx>
>
> 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@xxxxxxxxx>
Reviewed-by: Damien Lespiau <damien.lespiau@xxxxxxxxx>
/*
* 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@xxxxxxxxxxxxxxxxxxxxx
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
