diff mbox

[5/8] drm/i915: extract set_m_n from ironlake_crtc_mode_set

Message ID 1347455196-5167-6-git-send-email-przanoni@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

Paulo Zanoni Sept. 12, 2012, 1:06 p.m. UTC
From: Paulo Zanoni <paulo.r.zanoni@intel.com>

The set_m_n code was spread all over the mode_set function.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |  152 ++++++++++++++++++++--------------
 1 file changed, 89 insertions(+), 63 deletions(-)

Comments

Daniel Vetter Sept. 12, 2012, 2:20 p.m. UTC | #1
On Wed, Sep 12, 2012 at 10:06:33AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> The set_m_n code was spread all over the mode_set function.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  152 ++++++++++++++++++++--------------
>  1 file changed, 89 insertions(+), 63 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index b657416..8d3b9d6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4735,6 +4735,92 @@ static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
>  		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>  }
>  
> +static void ironlake_set_m_n(struct drm_crtc *crtc,
> +			     struct drm_display_mode *mode,
> +			     struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	enum pipe pipe = intel_crtc->pipe;
> +	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
> +	struct fdi_m_n m_n = {0};
> +	int target_clock, pixel_multiplier, lane, link_bw;
> +	bool is_dp = false, is_cpu_edp = false;
> +
> +	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> +		switch (intel_encoder->type) {
> +		case INTEL_OUTPUT_DISPLAYPORT:
> +			is_dp = true;
> +			break;
> +		case INTEL_OUTPUT_EDP:
> +			is_dp = true;
> +			if (!intel_encoder_is_pch_edp(&intel_encoder->base))
> +				is_cpu_edp = true;
> +			edp_encoder = intel_encoder;
> +			break;
> +		}
> +	}
> +
> +	/* FDI link */
> +	pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> +	lane = 0;
> +	/* CPU eDP doesn't require FDI link, so just set DP M/N
> +	   according to current link config */
> +	if (is_cpu_edp) {
> +		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> +	} else {
> +		/* FDI is a binary signal running at ~2.7GHz, encoding
> +		 * each output octet as 10 bits. The actual frequency
> +		 * is stored as a divider into a 100MHz clock, and the
> +		 * mode pixel clock is stored in units of 1KHz.
> +		 * Hence the bw of each lane in terms of the mode signal
> +		 * is:
> +		 */
> +		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +	}
> +
> +	/* [e]DP over FDI requires target mode clock instead of link clock. */
> +	if (edp_encoder)
> +		target_clock = intel_edp_target_clock(edp_encoder, mode);
> +	else if (is_dp)
> +		target_clock = mode->clock;
> +	else
> +		target_clock = adjusted_mode->clock;
> +
> +	if (!lane) {
> +		/*
> +		 * Account for spread spectrum to avoid
> +		 * oversubscribing the link. Max center spread
> +		 * is 2.5%; use 5% for safety's sake.
> +		 */
> +		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
> +		lane = bps / (link_bw * 8) + 1;
> +	}
> +
> +	intel_crtc->fdi_lanes = lane;
> +
> +	if (pixel_multiplier > 1)
> +		link_bw *= pixel_multiplier;
> +
> +	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
> +			     &m_n);
> +
> +	if (is_dp && !is_cpu_edp) {
> +		intel_dp_set_m_n(crtc, mode, adjusted_mode);
> +	} else {
> +		/* For non-DP output, clear any trans DP clock recovery setting.*/
> +		I915_WRITE(TRANSDATA_M1(pipe), 0);
> +		I915_WRITE(TRANSDATA_N1(pipe), 0);
> +		I915_WRITE(TRANSDPLINK_M1(pipe), 0);
> +		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
> +	}

The pch DP transcoder m/n values are imo encoder-specific state - I'm
toying around with ideas to shovel this into encoder callbacks, similarly
to the lvds port enabling. Hence ...

[pls scroll down.]

> +	I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> +	I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
> +	I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
> +	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
> +}
> +
>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  				  struct drm_display_mode *mode,
>  				  struct drm_display_mode *adjusted_mode,
> @@ -4751,11 +4837,9 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	u32 dpll, fp = 0, fp2 = 0;
>  	bool ok, has_reduced_clock = false, is_sdvo = false;
>  	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
> -	struct intel_encoder *encoder, *edp_encoder = NULL;
> +	struct intel_encoder *encoder;
>  	const intel_limit_t *limit;
> -	int ret;
> -	struct fdi_m_n m_n = {0};
> -	int target_clock, pixel_multiplier, lane, link_bw, factor;
> +	int ret, factor;
>  	unsigned int pipe_bpp;
>  	bool dither;
>  	bool is_cpu_edp = false, is_pch_edp = false;
> @@ -4786,7 +4870,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  				is_pch_edp = true;
>  			else
>  				is_cpu_edp = true;
> -			edp_encoder = encoder;
>  			break;
>  		}
>  
> @@ -4828,33 +4911,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (is_sdvo && is_tv)
>  		i9xx_adjust_sdvo_tv_clock(adjusted_mode, &clock);
>  
> -
> -	/* FDI link */
> -	pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> -	lane = 0;
> -	/* CPU eDP doesn't require FDI link, so just set DP M/N
> -	   according to current link config */
> -	if (is_cpu_edp) {
> -		intel_edp_link_config(edp_encoder, &lane, &link_bw);
> -	} else {
> -		/* FDI is a binary signal running at ~2.7GHz, encoding
> -		 * each output octet as 10 bits. The actual frequency
> -		 * is stored as a divider into a 100MHz clock, and the
> -		 * mode pixel clock is stored in units of 1KHz.
> -		 * Hence the bw of each lane in terms of the mode signal
> -		 * is:
> -		 */
> -		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> -	}
> -
> -	/* [e]DP over FDI requires target mode clock instead of link clock. */
> -	if (edp_encoder)
> -		target_clock = intel_edp_target_clock(edp_encoder, mode);
> -	else if (is_dp)
> -		target_clock = mode->clock;
> -	else
> -		target_clock = adjusted_mode->clock;
> -
>  	/* determine panel color depth */
>  	dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
>  	if (is_lvds && dev_priv->lvds_dither)
> @@ -4868,23 +4924,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	}
>  	intel_crtc->bpp = pipe_bpp;
>  
> -	if (!lane) {
> -		/*
> -		 * Account for spread spectrum to avoid
> -		 * oversubscribing the link. Max center spread
> -		 * is 2.5%; use 5% for safety's sake.
> -		 */
> -		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
> -		lane = bps / (link_bw * 8) + 1;
> -	}
> -
> -	intel_crtc->fdi_lanes = lane;
> -
> -	if (pixel_multiplier > 1)
> -		link_bw *= pixel_multiplier;
> -	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
> -			     &m_n);
> -
>  	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
>  	if (has_reduced_clock)
>  		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> @@ -4977,15 +5016,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (is_lvds)
>  		ironlake_lvds_port_enable(intel_crtc, adjusted_mode, clock.p2);
>  
> -	if (is_dp && !is_cpu_edp) {
> -		intel_dp_set_m_n(crtc, mode, adjusted_mode);
> -	} else {
> -		/* For non-DP output, clear any trans DP clock recovery setting.*/
> -		I915_WRITE(TRANSDATA_M1(pipe), 0);
> -		I915_WRITE(TRANSDATA_N1(pipe), 0);
> -		I915_WRITE(TRANSDPLINK_M1(pipe), 0);
> -		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
> -	}
> +	ironlake_set_m_n(crtc, mode, adjusted_mode);

... I think we should leave this here for now ...
>  
>  	if (intel_crtc->pch_pll) {
>  		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> @@ -5014,11 +5045,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	ironlake_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>  
> -	I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
> -	I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
> -	I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
> -	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);

... and instead move the new ironlake_set_m_n call to this place - that
way we don't change anything in the register write sequence, too. And
somehow changing the register write sequence bites us every time we try :(

Also, I'm hunting a strange dp dpms issue on ilk/gm45 currently and I
suspect (without any evidence). So I prefer we're extra careful around dp
state for now.

Otherwise this looks nice.
-Daniel

> -
>  	if (is_cpu_edp)
>  		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> 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_display.c b/drivers/gpu/drm/i915/intel_display.c
index b657416..8d3b9d6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4735,6 +4735,92 @@  static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
 		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
 }
 
+static void ironlake_set_m_n(struct drm_crtc *crtc,
+			     struct drm_display_mode *mode,
+			     struct drm_display_mode *adjusted_mode)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	enum pipe pipe = intel_crtc->pipe;
+	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
+	struct fdi_m_n m_n = {0};
+	int target_clock, pixel_multiplier, lane, link_bw;
+	bool is_dp = false, is_cpu_edp = false;
+
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_DISPLAYPORT:
+			is_dp = true;
+			break;
+		case INTEL_OUTPUT_EDP:
+			is_dp = true;
+			if (!intel_encoder_is_pch_edp(&intel_encoder->base))
+				is_cpu_edp = true;
+			edp_encoder = intel_encoder;
+			break;
+		}
+	}
+
+	/* FDI link */
+	pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
+	lane = 0;
+	/* CPU eDP doesn't require FDI link, so just set DP M/N
+	   according to current link config */
+	if (is_cpu_edp) {
+		intel_edp_link_config(edp_encoder, &lane, &link_bw);
+	} else {
+		/* FDI is a binary signal running at ~2.7GHz, encoding
+		 * each output octet as 10 bits. The actual frequency
+		 * is stored as a divider into a 100MHz clock, and the
+		 * mode pixel clock is stored in units of 1KHz.
+		 * Hence the bw of each lane in terms of the mode signal
+		 * is:
+		 */
+		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
+	}
+
+	/* [e]DP over FDI requires target mode clock instead of link clock. */
+	if (edp_encoder)
+		target_clock = intel_edp_target_clock(edp_encoder, mode);
+	else if (is_dp)
+		target_clock = mode->clock;
+	else
+		target_clock = adjusted_mode->clock;
+
+	if (!lane) {
+		/*
+		 * Account for spread spectrum to avoid
+		 * oversubscribing the link. Max center spread
+		 * is 2.5%; use 5% for safety's sake.
+		 */
+		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
+		lane = bps / (link_bw * 8) + 1;
+	}
+
+	intel_crtc->fdi_lanes = lane;
+
+	if (pixel_multiplier > 1)
+		link_bw *= pixel_multiplier;
+
+	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
+			     &m_n);
+
+	if (is_dp && !is_cpu_edp) {
+		intel_dp_set_m_n(crtc, mode, adjusted_mode);
+	} else {
+		/* For non-DP output, clear any trans DP clock recovery setting.*/
+		I915_WRITE(TRANSDATA_M1(pipe), 0);
+		I915_WRITE(TRANSDATA_N1(pipe), 0);
+		I915_WRITE(TRANSDPLINK_M1(pipe), 0);
+		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
+	}
+	I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
+	I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
+	I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
+	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4751,11 +4837,9 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	u32 dpll, fp = 0, fp2 = 0;
 	bool ok, has_reduced_clock = false, is_sdvo = false;
 	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
-	struct intel_encoder *encoder, *edp_encoder = NULL;
+	struct intel_encoder *encoder;
 	const intel_limit_t *limit;
-	int ret;
-	struct fdi_m_n m_n = {0};
-	int target_clock, pixel_multiplier, lane, link_bw, factor;
+	int ret, factor;
 	unsigned int pipe_bpp;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
@@ -4786,7 +4870,6 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				is_pch_edp = true;
 			else
 				is_cpu_edp = true;
-			edp_encoder = encoder;
 			break;
 		}
 
@@ -4828,33 +4911,6 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (is_sdvo && is_tv)
 		i9xx_adjust_sdvo_tv_clock(adjusted_mode, &clock);
 
-
-	/* FDI link */
-	pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
-	lane = 0;
-	/* CPU eDP doesn't require FDI link, so just set DP M/N
-	   according to current link config */
-	if (is_cpu_edp) {
-		intel_edp_link_config(edp_encoder, &lane, &link_bw);
-	} else {
-		/* FDI is a binary signal running at ~2.7GHz, encoding
-		 * each output octet as 10 bits. The actual frequency
-		 * is stored as a divider into a 100MHz clock, and the
-		 * mode pixel clock is stored in units of 1KHz.
-		 * Hence the bw of each lane in terms of the mode signal
-		 * is:
-		 */
-		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
-	}
-
-	/* [e]DP over FDI requires target mode clock instead of link clock. */
-	if (edp_encoder)
-		target_clock = intel_edp_target_clock(edp_encoder, mode);
-	else if (is_dp)
-		target_clock = mode->clock;
-	else
-		target_clock = adjusted_mode->clock;
-
 	/* determine panel color depth */
 	dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
 	if (is_lvds && dev_priv->lvds_dither)
@@ -4868,23 +4924,6 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	}
 	intel_crtc->bpp = pipe_bpp;
 
-	if (!lane) {
-		/*
-		 * Account for spread spectrum to avoid
-		 * oversubscribing the link. Max center spread
-		 * is 2.5%; use 5% for safety's sake.
-		 */
-		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
-		lane = bps / (link_bw * 8) + 1;
-	}
-
-	intel_crtc->fdi_lanes = lane;
-
-	if (pixel_multiplier > 1)
-		link_bw *= pixel_multiplier;
-	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
-			     &m_n);
-
 	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
 	if (has_reduced_clock)
 		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
@@ -4977,15 +5016,7 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (is_lvds)
 		ironlake_lvds_port_enable(intel_crtc, adjusted_mode, clock.p2);
 
-	if (is_dp && !is_cpu_edp) {
-		intel_dp_set_m_n(crtc, mode, adjusted_mode);
-	} else {
-		/* For non-DP output, clear any trans DP clock recovery setting.*/
-		I915_WRITE(TRANSDATA_M1(pipe), 0);
-		I915_WRITE(TRANSDATA_N1(pipe), 0);
-		I915_WRITE(TRANSDPLINK_M1(pipe), 0);
-		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
-	}
+	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
 	if (intel_crtc->pch_pll) {
 		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
@@ -5014,11 +5045,6 @@  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ironlake_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
-	I915_WRITE(PIPE_DATA_M1(pipe), TU_SIZE(m_n.tu) | m_n.gmch_m);
-	I915_WRITE(PIPE_DATA_N1(pipe), m_n.gmch_n);
-	I915_WRITE(PIPE_LINK_M1(pipe), m_n.link_m);
-	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
-
 	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);