diff mbox

[2/2] drm/i915: split intel_ddi_pll_mode_set in 2 pieces

Message ID 1383164864-1523-2-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Oct. 30, 2013, 8:27 p.m. UTC 
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(-)

Comments

Lespiau, Damien Nov. 18, 2013, 12:06 p.m. UTC | #1 
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
diff mbox

Patch

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);