[PATCH 0/8] Rework ironlake_crtc_mode_set

[PATCH 0/8] Rework ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Every time I have to change ironlake_crtc_mode_set I get scared by the
fact that it has 404 lines. I feel way more confident when I'm changing
small functions that do specific self-contained things.

Thus, the goal of this patch series is to extract from
ironlake_crtc_mode_set sub-functions that do specific things. The goal
of the series is to only change the coding style, not changing the
functionality.

The order that code executes was slightly changed in a few cases, but I
don't expect this to break anything. I tested this series on a SNB with
LVDS, HDMI, DP and VGA and everything worked. I don't have access to
more machines right now, but once I get to my office again I will test
more.

Also, I did a quick check on i9xx_crtc_mode_set and concluded we won't
be able to reuse any of the ironlake functions on it, although we can
try to split i9xx_crtc_mode_set in a similar way. Still, I'm not sure we
want to do that.

After these changes I'll be able to properly and sanely fix the
Haswell/LPT case (since Haswell/LPT is currently running
ironlake_crtc_mode_set):
  - fork a haswell_crtc_mode_set function
  - create haswell-specific versions of functions that should be
    different on Haswell
  - run IBX/CPT-specific code only on IBX/CPT, same for LPT

Paulo Zanoni (8):
  drm/i915: extract ironlake_set_pipeconf form ironlake_crtc_mode_set
  drm/i915: extract LVDS-specific code from ironlake_crtc_mode_set
  drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set
  drm/i915: simplify setting DSPCNTR inside ironlake_crtc_mode_set
  drm/i915: extract set_m_n from ironlake_crtc_mode_set
  drm/i915: extract compute_clocks from ironlake_crtc_mode_set
  drm/i915: extract pch_pll_set from ironlake_crtc_mode_set
  drm/i915: remove unused variables from ironlake_crtc_mode_set

 drivers/gpu/drm/i915/intel_display.c |  575 +++++++++++++++++++---------------
 drivers/gpu/drm/i915/intel_drv.h     |    3 +
 drivers/gpu/drm/i915/intel_lvds.c    |   43 +++
 3 files changed, 374 insertions(+), 247 deletions(-)

-- 
1.7.10.4

[PATCH 1/8] drm/i915: extract ironlake_set_pipeconf form ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because ironlake_crtc_mode_set is a giant function that used to have
404 lines. Let's try to make it less complex/confusing.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index e061acd..244bce6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4646,6 +4646,50 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 	return 120000;
 }
 
+static void ironlake_set_pipeconf(struct drm_crtc *crtc,
+				  struct drm_display_mode *adjusted_mode,
+				  bool dither)
+{
+	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	int pipe = intel_crtc->pipe;
+	uint32_t val;
+
+	val = I915_READ(PIPECONF(pipe));
+
+	val &= ~PIPE_BPC_MASK;
+	switch (intel_crtc->bpp) {
+	case 18:
+		val |= PIPE_6BPC;
+		break;
+	case 24:
+		val |= PIPE_8BPC;
+		break;
+	case 30:
+		val |= PIPE_10BPC;
+		break;
+	case 36:
+		val |= PIPE_12BPC;
+		break;
+	default:
+		val |= PIPE_8BPC;
+		break;
+	}
+
+	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
+	if (dither)
+		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
+
+	val &= ~PIPECONF_INTERLACE_MASK;
+	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
+		val |= PIPECONF_INTERLACED_ILK;
+	else
+		val |= PIPECONF_PROGRESSIVE;
+
+	I915_WRITE(PIPECONF(pipe), val);
+	POSTING_READ(PIPECONF(pipe));
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4659,7 +4703,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int refclk, num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	u32 dpll, fp = 0, fp2 = 0, dspcntr, pipeconf;
+	u32 dpll, fp = 0, fp2 = 0, dspcntr;
 	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;
@@ -4768,32 +4812,17 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		target_clock = adjusted_mode->clock;
 
 	/* determine panel color depth */
-	temp = I915_READ(PIPECONF(pipe));
-	temp &= ~PIPE_BPC_MASK;
 	dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
-	switch (pipe_bpp) {
-	case 18:
-		temp |= PIPE_6BPC;
-		break;
-	case 24:
-		temp |= PIPE_8BPC;
-		break;
-	case 30:
-		temp |= PIPE_10BPC;
-		break;
-	case 36:
-		temp |= PIPE_12BPC;
-		break;
-	default:
+	if (is_lvds && dev_priv->lvds_dither)
+		dither = true;
+
+	if (pipe_bpp != 18 && pipe_bpp != 24 && pipe_bpp != 30 &&
+	    pipe_bpp != 36) {
 		WARN(1, "intel_choose_pipe_bpp returned invalid value %d\n",
-			pipe_bpp);
-		temp |= PIPE_8BPC;
+		     pipe_bpp);
 		pipe_bpp = 24;
-		break;
 	}
-
 	intel_crtc->bpp = pipe_bpp;
-	I915_WRITE(PIPECONF(pipe), temp);
 
 	if (!lane) {
 		/*
@@ -4877,9 +4906,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	else
 		dpll |= PLL_REF_INPUT_DREFCLK;
 
-	/* setup pipeconf */
-	pipeconf = I915_READ(PIPECONF(pipe));
-
 	/* Set up the display plane register */
 	dspcntr = DISPPLANE_GAMMA_ENABLE;
 
@@ -4942,12 +4968,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		I915_WRITE(PCH_LVDS, temp);
 	}
 
-	pipeconf &= ~PIPECONF_DITHER_EN;
-	pipeconf &= ~PIPECONF_DITHER_TYPE_MASK;
-	if ((is_lvds && dev_priv->lvds_dither) || dither) {
-		pipeconf |= PIPECONF_DITHER_EN;
-		pipeconf |= PIPECONF_DITHER_TYPE_SP;
-	}
 	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
@@ -4983,9 +5003,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	pipeconf &= ~PIPECONF_INTERLACE_MASK;
 	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
-		pipeconf |= PIPECONF_INTERLACED_ILK;
 		/* the chip adds 2 halflines automatically */
 		adjusted_mode->crtc_vtotal -= 1;
 		adjusted_mode->crtc_vblank_end -= 1;
@@ -4993,7 +5011,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 			   adjusted_mode->crtc_hsync_start
 			   - adjusted_mode->crtc_htotal/2);
 	} else {
-		pipeconf |= PIPECONF_PROGRESSIVE;
 		I915_WRITE(VSYNCSHIFT(pipe), 0);
 	}
 
@@ -5031,8 +5048,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (is_cpu_edp)
 		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
 
-	I915_WRITE(PIPECONF(pipe), pipeconf);
-	POSTING_READ(PIPECONF(pipe));
+	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
 
 	intel_wait_for_vblank(dev, pipe);
 
-- 
1.7.10.4

[PATCH 2/8] drm/i915: extract LVDS-specific code from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

A next step would be to move this code to some of the encoder-specific
callbacks. But really, moving the function away is certainly the first
step.

Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
---
 drivers/gpu/drm/i915/intel_display.c |   37 ++---------------------------
 drivers/gpu/drm/i915/intel_drv.h     |    3 +++
 drivers/gpu/drm/i915/intel_lvds.c    |   43 ++++++++++++++++++++++++++++++++++
 3 files changed, 48 insertions(+), 35 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 244bce6..cf1e628 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4710,7 +4710,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	const intel_limit_t *limit;
 	int ret;
 	struct fdi_m_n m_n = {0};
-	u32 temp;
 	int target_clock, pixel_multiplier, lane, link_bw, factor;
 	unsigned int pipe_bpp;
 	bool dither;
@@ -4933,40 +4932,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	 * This is an exception to the general rule that mode_set doesn't turn
 	 * things on.
 	 */
-	if (is_lvds) {
-		temp = I915_READ(PCH_LVDS);
-		temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-		if (HAS_PCH_CPT(dev)) {
-			temp &= ~PORT_TRANS_SEL_MASK;
-			temp |= PORT_TRANS_SEL_CPT(pipe);
-		} else {
-			if (pipe == 1)
-				temp |= LVDS_PIPEB_SELECT;
-			else
-				temp &= ~LVDS_PIPEB_SELECT;
-		}
-
-		/* set the corresponsding LVDS_BORDER bit */
-		temp |= dev_priv->lvds_border_bits;
-		/* Set the B0-B3 data pairs corresponding to whether we're going to
-		 * set the DPLLs for dual-channel mode or not.
-		 */
-		if (clock.p2 == 7)
-			temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-		else
-			temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
-
-		/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
-		 * appropriately here, but we need to look more thoroughly into how
-		 * panels behave in the two modes.
-		 */
-		temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-			temp |= LVDS_HSYNC_POLARITY;
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-			temp |= LVDS_VSYNC_POLARITY;
-		I915_WRITE(PCH_LVDS, temp);
-	}
+	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);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 4f2b2d6..0b71f6c 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -412,6 +412,9 @@ extern void intel_mark_idle(struct drm_device *dev);
 extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
 extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
 extern bool intel_lvds_init(struct drm_device *dev);
+extern void ironlake_lvds_port_enable(struct intel_crtc *intel_crtc,
+				      struct drm_display_mode *adjusted_mode,
+				      int clock_p2);
 extern void intel_dp_init(struct drm_device *dev, int output_reg,
 			  enum port port);
 void
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 5646895..e418d95 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -587,6 +587,49 @@ static int intel_lvds_set_property(struct drm_connector *connector,
 	return 0;
 }
 
+void ironlake_lvds_port_enable(struct intel_crtc *intel_crtc,
+			       struct drm_display_mode *adjusted_mode,
+			       int clock_p2)
+{
+	struct drm_device *dev = intel_crtc->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	enum pipe pipe = intel_crtc->pipe;
+	uint32_t temp;
+
+	temp = I915_READ(PCH_LVDS);
+	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
+	if (HAS_PCH_CPT(dev)) {
+		temp &= ~PORT_TRANS_SEL_MASK;
+		temp |= PORT_TRANS_SEL_CPT(pipe);
+	} else {
+		if (pipe == 1)
+			temp |= LVDS_PIPEB_SELECT;
+		else
+			temp &= ~LVDS_PIPEB_SELECT;
+	}
+
+	/* set the corresponsding LVDS_BORDER bit */
+	temp |= dev_priv->lvds_border_bits;
+	/* Set the B0-B3 data pairs corresponding to whether we're going to
+	 * set the DPLLs for dual-channel mode or not.
+	 */
+	if (clock_p2 == 7)
+		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
+	else
+		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
+
+	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
+	 * appropriately here, but we need to look more thoroughly into how
+	 * panels behave in the two modes.
+	 */
+	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
+	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
+		temp |= LVDS_HSYNC_POLARITY;
+	if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
+		temp |= LVDS_VSYNC_POLARITY;
+	I915_WRITE(PCH_LVDS, temp);
+}
+
 static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
 	.mode_fixup = intel_lvds_mode_fixup,
 	.mode_set = intel_lvds_mode_set,
-- 
1.7.10.4

[PATCH 3/8] drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index cf1e628..5a4e363 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4690,6 +4690,51 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	POSTING_READ(PIPECONF(pipe));
 }
 
+static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
+				      struct drm_display_mode *mode,
+				      struct drm_display_mode *adjusted_mode)
+{
+	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
+	enum pipe pipe = intel_crtc->pipe;
+
+	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
+		/* the chip adds 2 halflines automatically */
+		adjusted_mode->crtc_vtotal -= 1;
+		adjusted_mode->crtc_vblank_end -= 1;
+		I915_WRITE(VSYNCSHIFT(pipe),
+			   adjusted_mode->crtc_hsync_start
+			   - adjusted_mode->crtc_htotal/2);
+	} else {
+		I915_WRITE(VSYNCSHIFT(pipe), 0);
+	}
+
+	I915_WRITE(HTOTAL(pipe),
+		   (adjusted_mode->crtc_hdisplay - 1) |
+		   ((adjusted_mode->crtc_htotal - 1) << 16));
+	I915_WRITE(HBLANK(pipe),
+		   (adjusted_mode->crtc_hblank_start - 1) |
+		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
+	I915_WRITE(HSYNC(pipe),
+		   (adjusted_mode->crtc_hsync_start - 1) |
+		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
+
+	I915_WRITE(VTOTAL(pipe),
+		   (adjusted_mode->crtc_vdisplay - 1) |
+		   ((adjusted_mode->crtc_vtotal - 1) << 16));
+	I915_WRITE(VBLANK(pipe),
+		   (adjusted_mode->crtc_vblank_start - 1) |
+		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
+	I915_WRITE(VSYNC(pipe),
+		   (adjusted_mode->crtc_vsync_start - 1) |
+		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
+
+	/* pipesrc controls the size that is scaled from, which should
+	 * always be the user's requested size.
+	 */
+	I915_WRITE(PIPESRC(pipe),
+		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4970,42 +5015,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		}
 	}
 
-	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
-		/* the chip adds 2 halflines automatically */
-		adjusted_mode->crtc_vtotal -= 1;
-		adjusted_mode->crtc_vblank_end -= 1;
-		I915_WRITE(VSYNCSHIFT(pipe),
-			   adjusted_mode->crtc_hsync_start
-			   - adjusted_mode->crtc_htotal/2);
-	} else {
-		I915_WRITE(VSYNCSHIFT(pipe), 0);
-	}
-
-	I915_WRITE(HTOTAL(pipe),
-		   (adjusted_mode->crtc_hdisplay - 1) |
-		   ((adjusted_mode->crtc_htotal - 1) << 16));
-	I915_WRITE(HBLANK(pipe),
-		   (adjusted_mode->crtc_hblank_start - 1) |
-		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
-	I915_WRITE(HSYNC(pipe),
-		   (adjusted_mode->crtc_hsync_start - 1) |
-		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
-
-	I915_WRITE(VTOTAL(pipe),
-		   (adjusted_mode->crtc_vdisplay - 1) |
-		   ((adjusted_mode->crtc_vtotal - 1) << 16));
-	I915_WRITE(VBLANK(pipe),
-		   (adjusted_mode->crtc_vblank_start - 1) |
-		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
-	I915_WRITE(VSYNC(pipe),
-		   (adjusted_mode->crtc_vsync_start - 1) |
-		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
-
-	/* pipesrc controls the size that is scaled from, which should
-	 * always be the user's requested size.
-	 */
-	I915_WRITE(PIPESRC(pipe),
-		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
+	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);
-- 
1.7.10.4

[PATCH 4/8] drm/i915: simplify setting DSPCNTR inside ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

Because declaring a variable in the beginning of the function, then
initializing it 100 lines later, then using it 100 lines later does
not make our code look good IMHO.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5a4e363..b657416 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4748,7 +4748,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int refclk, num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	u32 dpll, fp = 0, fp2 = 0, dspcntr;
+	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;
@@ -4950,9 +4950,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	else
 		dpll |= PLL_REF_INPUT_DREFCLK;
 
-	/* Set up the display plane register */
-	dspcntr = DISPPLANE_GAMMA_ENABLE;
-
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
@@ -5029,7 +5026,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	intel_wait_for_vblank(dev, pipe);
 
-	I915_WRITE(DSPCNTR(plane), dspcntr);
+	/* Set up the display plane register */
+	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
 	POSTING_READ(DSPCNTR(plane));
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
-- 
1.7.10.4

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

[Paulo Zanoni]

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);
+	}
+	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);
 
-- 
1.7.10.4

[PATCH 6/8] drm/i915: extract compute_clocks from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8d3b9d6..dc4132a 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4821,6 +4821,69 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 	I915_WRITE(PIPE_LINK_N1(pipe), m_n.link_n);
 }
 
+static bool ironlake_compute_clocks(struct drm_crtc *crtc,
+				    struct drm_display_mode *adjusted_mode,
+				    intel_clock_t *clock,
+				    bool *has_reduced_clock,
+				    intel_clock_t *reduced_clock)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_encoder *intel_encoder;
+	int refclk;
+	const intel_limit_t *limit;
+	bool ret, is_sdvo = false, is_tv = false, is_lvds = false;
+
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_LVDS:
+			is_lvds = true;
+			break;
+		case INTEL_OUTPUT_SDVO:
+		case INTEL_OUTPUT_HDMI:
+			is_sdvo = true;
+			if (intel_encoder->needs_tv_clock)
+				is_tv = true;
+			break;
+		case INTEL_OUTPUT_TVOUT:
+			is_tv = true;
+			break;
+		}
+	}
+
+	refclk = ironlake_get_refclk(crtc);
+
+	/*
+	 * Returns a set of divisors for the desired target clock with the given
+	 * refclk, or FALSE.  The returned values represent the clock equation:
+	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
+	 */
+	limit = intel_limit(crtc, refclk);
+	ret = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
+			      clock);
+	if (!ret)
+		return false;
+
+	if (is_lvds && dev_priv->lvds_downclock_avail) {
+		/*
+		 * Ensure we match the reduced clock's P to the target clock.
+		 * If the clocks don't match, we can't switch the display clock
+		 * by using the FP0/FP1. In such case we will disable the LVDS
+		 * downclock feature.
+		*/
+		*has_reduced_clock = limit->find_pll(limit, crtc,
+						     dev_priv->lvds_downclock,
+						     refclk,
+						     clock,
+						     reduced_clock);
+	}
+
+	if (is_sdvo && is_tv)
+		i9xx_adjust_sdvo_tv_clock(adjusted_mode, clock);
+
+	return true;
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4832,13 +4895,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
-	int refclk, num_connectors = 0;
+	int num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
 	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;
-	const intel_limit_t *limit;
 	int ret, factor;
 	unsigned int pipe_bpp;
 	bool dither;
@@ -4876,16 +4938,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	refclk = ironlake_get_refclk(crtc);
-
-	/*
-	 * Returns a set of divisors for the desired target clock with the given
-	 * refclk, or FALSE.  The returned values represent the clock equation:
-	 * reflck * (5 * (m1 + 2) + (m2 + 2)) / (n + 2) / p1 / p2.
-	 */
-	limit = intel_limit(crtc, refclk);
-	ok = limit->find_pll(limit, crtc, adjusted_mode->clock, refclk, NULL,
-			     &clock);
+	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
+				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
 		DRM_ERROR("Couldn't find PLL settings for mode!\n");
 		return -EINVAL;
@@ -4894,23 +4948,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
-	if (is_lvds && dev_priv->lvds_downclock_avail) {
-		/*
-		 * Ensure we match the reduced clock's P to the target clock.
-		 * If the clocks don't match, we can't switch the display clock
-		 * by using the FP0/FP1. In such case we will disable the LVDS
-		 * downclock feature.
-		*/
-		has_reduced_clock = limit->find_pll(limit, crtc,
-						    dev_priv->lvds_downclock,
-						    refclk,
-						    &clock,
-						    &reduced_clock);
-	}
-
-	if (is_sdvo && is_tv)
-		i9xx_adjust_sdvo_tv_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)
-- 
1.7.10.4

[PATCH 7/8] drm/i915: extract pch_pll_set from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

This is all the code responsible for setting the PCH PLLs.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dc4132a..7ffbcfd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4884,6 +4884,143 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
 	return true;
 }
 
+static bool ironlake_pch_pll_set(struct drm_crtc *crtc,
+				 struct drm_display_mode *adjusted_mode,
+				 intel_clock_t *clock,
+				 bool has_reduced_clock,
+				 intel_clock_t *reduced_clock)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *intel_encoder;
+	int num_connectors = 0;
+	u32 dpll, fp = 0, fp2 = 0;
+	int factor, pixel_multiplier;
+	struct intel_pch_pll *pll;
+	bool is_lvds = false, is_sdvo = false, is_tv = false;
+	bool is_dp = false, is_cpu_edp = false;
+
+	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
+		switch (intel_encoder->type) {
+		case INTEL_OUTPUT_LVDS:
+			is_lvds = true;
+			break;
+		case INTEL_OUTPUT_SDVO:
+		case INTEL_OUTPUT_HDMI:
+			is_sdvo = true;
+			if (intel_encoder->needs_tv_clock)
+				is_tv = true;
+			break;
+		case INTEL_OUTPUT_TVOUT:
+			is_tv = true;
+			break;
+		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;
+			break;
+		}
+
+		num_connectors++;
+	}
+
+	fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
+	if (has_reduced_clock)
+		fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
+			reduced_clock->m2;
+
+	/* Enable autotuning of the PLL clock (if permissible) */
+	factor = 21;
+	if (is_lvds) {
+		if ((intel_panel_use_ssc(dev_priv) &&
+		     dev_priv->lvds_ssc_freq == 100) ||
+		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
+			factor = 25;
+	} else if (is_sdvo && is_tv)
+		factor = 20;
+
+	if (clock->m < factor * clock->n)
+		fp |= FP_CB_TUNE;
+
+	dpll = 0;
+
+	if (is_lvds)
+		dpll |= DPLLB_MODE_LVDS;
+	else
+		dpll |= DPLLB_MODE_DAC_SERIAL;
+	if (is_sdvo) {
+		pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
+		if (pixel_multiplier > 1) {
+			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
+		}
+		dpll |= DPLL_DVO_HIGH_SPEED;
+	}
+	if (is_dp && !is_cpu_edp)
+		dpll |= DPLL_DVO_HIGH_SPEED;
+
+	/* compute bitmask from p1 value */
+	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
+	/* also FPA1 */
+	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
+
+	switch (clock->p2) {
+	case 5:
+		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
+		break;
+	case 7:
+		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
+		break;
+	case 10:
+		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
+		break;
+	case 14:
+		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
+		break;
+	}
+
+	if (is_sdvo && is_tv)
+		dpll |= PLL_REF_INPUT_TVCLKINBC;
+	else if (is_tv)
+		/* XXX: just matching BIOS for now */
+		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
+		dpll |= 3;
+	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
+		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
+	else
+		dpll |= PLL_REF_INPUT_DREFCLK;
+
+	pll = intel_get_pch_pll(intel_crtc, dpll, fp);
+	if (!pll)
+		return false;
+
+	I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
+
+	/* Wait for the clocks to stabilize. */
+	POSTING_READ(intel_crtc->pch_pll->pll_reg);
+	udelay(150);
+
+	/* The pixel multiplier can only be updated once the
+	 * DPLL is enabled and the clocks are stable.
+	 *
+	 * So write it again.
+	 */
+	I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
+
+	intel_crtc->lowfreq_avail = false;
+	if (is_lvds && has_reduced_clock && i915_powersave) {
+		I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
+		intel_crtc->lowfreq_avail = true;
+	} else {
+		I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
+	}
+
+	return true;
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4897,11 +5034,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	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;
-	int ret, factor;
+	int ret;
 	unsigned int pipe_bpp;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
@@ -4961,90 +5097,25 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	}
 	intel_crtc->bpp = pipe_bpp;
 
-	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
-	if (has_reduced_clock)
-		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
-			reduced_clock.m2;
-
-	/* Enable autotuning of the PLL clock (if permissible) */
-	factor = 21;
-	if (is_lvds) {
-		if ((intel_panel_use_ssc(dev_priv) &&
-		     dev_priv->lvds_ssc_freq == 100) ||
-		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
-			factor = 25;
-	} else if (is_sdvo && is_tv)
-		factor = 20;
-
-	if (clock.m < factor * clock.n)
-		fp |= FP_CB_TUNE;
-
-	dpll = 0;
-
-	if (is_lvds)
-		dpll |= DPLLB_MODE_LVDS;
-	else
-		dpll |= DPLLB_MODE_DAC_SERIAL;
-	if (is_sdvo) {
-		int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
-		if (pixel_multiplier > 1) {
-			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
-		}
-		dpll |= DPLL_DVO_HIGH_SPEED;
-	}
-	if (is_dp && !is_cpu_edp)
-		dpll |= DPLL_DVO_HIGH_SPEED;
-
-	/* compute bitmask from p1 value */
-	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
-	/* also FPA1 */
-	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
-
-	switch (clock.p2) {
-	case 5:
-		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
-		break;
-	case 7:
-		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
-		break;
-	case 10:
-		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
-		break;
-	case 14:
-		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
-		break;
-	}
-
-	if (is_sdvo && is_tv)
-		dpll |= PLL_REF_INPUT_TVCLKINBC;
-	else if (is_tv)
-		/* XXX: just matching BIOS for now */
-		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
-		dpll |= 3;
-	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
-		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
-	else
-		dpll |= PLL_REF_INPUT_DREFCLK;
-
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
-	/* CPU eDP is the only output that doesn't need a PCH PLL of its own on
-	 * pre-Haswell/LPT generation */
-	if (HAS_PCH_LPT(dev)) {
-		DRM_DEBUG_KMS("LPT detected: no PLL for pipe %d necessary\n",
-				pipe);
-	} else if (!is_cpu_edp) {
-		struct intel_pch_pll *pll;
-
-		pll = intel_get_pch_pll(intel_crtc, dpll, fp);
-		if (pll == NULL) {
-			DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
-					 pipe);
-			return -EINVAL;
+	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
+		/* CPU eDP is the only output that doesn't need a PCH PLL of its
+		 * own */
+		if (is_cpu_edp) {
+			intel_put_pch_pll(intel_crtc);
+		} else {
+			ok = ironlake_pch_pll_set(crtc, adjusted_mode, &clock,
+						  has_reduced_clock,
+						  &reduced_clock);
+			if (!ok) {
+				DRM_ERROR("Failed to find PLL for pipe %d\n",
+					  pipe);
+				return -EINVAL;
+			}
 		}
-	} else
-		intel_put_pch_pll(intel_crtc);
+	}
 
 	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
 	 * This is an exception to the general rule that mode_set doesn't turn
@@ -5055,31 +5126,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ironlake_set_m_n(crtc, mode, adjusted_mode);
 
-	if (intel_crtc->pch_pll) {
-		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-
-		/* Wait for the clocks to stabilize. */
-		POSTING_READ(intel_crtc->pch_pll->pll_reg);
-		udelay(150);
-
-		/* The pixel multiplier can only be updated once the
-		 * DPLL is enabled and the clocks are stable.
-		 *
-		 * So write it again.
-		 */
-		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-	}
-
-	intel_crtc->lowfreq_avail = false;
-	if (intel_crtc->pch_pll) {
-		if (is_lvds && has_reduced_clock && i915_powersave) {
-			I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
-			intel_crtc->lowfreq_avail = true;
-		} else {
-			I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
-		}
-	}
-
 	ironlake_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
 	if (is_cpu_edp)
-- 
1.7.10.4

[PATCH 8/8] drm/i915: remove unused variables from ironlake_crtc_mode_set

[Paulo Zanoni]

From: Paulo Zanoni <paulo.r.zanoni@intel.com>

They are actually set but not used [-Wunused-but-set-variable].

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7ffbcfd..05339ef 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5034,39 +5034,20 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
 	intel_clock_t clock, reduced_clock;
-	bool ok, has_reduced_clock = false, is_sdvo = false;
-	bool is_crt = false, is_lvds = false, is_tv = false, is_dp = false;
+	bool ok, has_reduced_clock = false;
+	bool is_lvds = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	int ret;
 	unsigned int pipe_bpp;
 	bool dither;
-	bool is_cpu_edp = false, is_pch_edp = false;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
 		case INTEL_OUTPUT_LVDS:
 			is_lvds = true;
 			break;
-		case INTEL_OUTPUT_SDVO:
-		case INTEL_OUTPUT_HDMI:
-			is_sdvo = true;
-			if (encoder->needs_tv_clock)
-				is_tv = true;
-			break;
-		case INTEL_OUTPUT_TVOUT:
-			is_tv = true;
-			break;
-		case INTEL_OUTPUT_ANALOG:
-			is_crt = true;
-			break;
-		case INTEL_OUTPUT_DISPLAYPORT:
-			is_dp = true;
-			break;
 		case INTEL_OUTPUT_EDP:
-			is_dp = true;
-			if (intel_encoder_is_pch_edp(&encoder->base))
-				is_pch_edp = true;
-			else
+			if (!intel_encoder_is_pch_edp(&encoder->base))
 				is_cpu_edp = true;
 			break;
 		}
-- 
1.7.10.4

Re: [PATCH 2/8] drm/i915: extract LVDS-specific code from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:30AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> A next step would be to move this code to some of the encoder-specific
> callbacks. But really, moving the function away is certainly the first
> step.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

I have a slightly more ambitious version of this in my modeset-rework
branch - it also consolidates the lvds port enabling with the pre-pch code
and encapsulates it within a new encoder->pre_pll_enable callback. Since
this is part of a larger rework of how we handle clock settings that I'm
toying around with, I'd like to keep things as-is for now.
-Daniel

> ---
>  drivers/gpu/drm/i915/intel_display.c |   37 ++---------------------------
>  drivers/gpu/drm/i915/intel_drv.h     |    3 +++
>  drivers/gpu/drm/i915/intel_lvds.c    |   43 ++++++++++++++++++++++++++++++++++
>  3 files changed, 48 insertions(+), 35 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 244bce6..cf1e628 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4710,7 +4710,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	const intel_limit_t *limit;
>  	int ret;
>  	struct fdi_m_n m_n = {0};
> -	u32 temp;
>  	int target_clock, pixel_multiplier, lane, link_bw, factor;
>  	unsigned int pipe_bpp;
>  	bool dither;
> @@ -4933,40 +4932,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	 * This is an exception to the general rule that mode_set doesn't turn
>  	 * things on.
>  	 */
> -	if (is_lvds) {
> -		temp = I915_READ(PCH_LVDS);
> -		temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -		if (HAS_PCH_CPT(dev)) {
> -			temp &= ~PORT_TRANS_SEL_MASK;
> -			temp |= PORT_TRANS_SEL_CPT(pipe);
> -		} else {
> -			if (pipe == 1)
> -				temp |= LVDS_PIPEB_SELECT;
> -			else
> -				temp &= ~LVDS_PIPEB_SELECT;
> -		}
> -
> -		/* set the corresponsding LVDS_BORDER bit */
> -		temp |= dev_priv->lvds_border_bits;
> -		/* Set the B0-B3 data pairs corresponding to whether we're going to
> -		 * set the DPLLs for dual-channel mode or not.
> -		 */
> -		if (clock.p2 == 7)
> -			temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -		else
> -			temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> -
> -		/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> -		 * appropriately here, but we need to look more thoroughly into how
> -		 * panels behave in the two modes.
> -		 */
> -		temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -		if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -			temp |= LVDS_HSYNC_POLARITY;
> -		if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -			temp |= LVDS_VSYNC_POLARITY;
> -		I915_WRITE(PCH_LVDS, temp);
> -	}
> +	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);
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 4f2b2d6..0b71f6c 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -412,6 +412,9 @@ extern void intel_mark_idle(struct drm_device *dev);
>  extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
>  extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
>  extern bool intel_lvds_init(struct drm_device *dev);
> +extern void ironlake_lvds_port_enable(struct intel_crtc *intel_crtc,
> +				      struct drm_display_mode *adjusted_mode,
> +				      int clock_p2);
>  extern void intel_dp_init(struct drm_device *dev, int output_reg,
>  			  enum port port);
>  void
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 5646895..e418d95 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -587,6 +587,49 @@ static int intel_lvds_set_property(struct drm_connector *connector,
>  	return 0;
>  }
>  
> +void ironlake_lvds_port_enable(struct intel_crtc *intel_crtc,
> +			       struct drm_display_mode *adjusted_mode,
> +			       int clock_p2)
> +{
> +	struct drm_device *dev = intel_crtc->base.dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	enum pipe pipe = intel_crtc->pipe;
> +	uint32_t temp;
> +
> +	temp = I915_READ(PCH_LVDS);
> +	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> +	if (HAS_PCH_CPT(dev)) {
> +		temp &= ~PORT_TRANS_SEL_MASK;
> +		temp |= PORT_TRANS_SEL_CPT(pipe);
> +	} else {
> +		if (pipe == 1)
> +			temp |= LVDS_PIPEB_SELECT;
> +		else
> +			temp &= ~LVDS_PIPEB_SELECT;
> +	}
> +
> +	/* set the corresponsding LVDS_BORDER bit */
> +	temp |= dev_priv->lvds_border_bits;
> +	/* Set the B0-B3 data pairs corresponding to whether we're going to
> +	 * set the DPLLs for dual-channel mode or not.
> +	 */
> +	if (clock_p2 == 7)
> +		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> +	else
> +		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
> +
> +	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
> +	 * appropriately here, but we need to look more thoroughly into how
> +	 * panels behave in the two modes.
> +	 */
> +	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> +		temp |= LVDS_HSYNC_POLARITY;
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> +		temp |= LVDS_VSYNC_POLARITY;
> +	I915_WRITE(PCH_LVDS, temp);
> +}
> +
>  static const struct drm_encoder_helper_funcs intel_lvds_helper_funcs = {
>  	.mode_fixup = intel_lvds_mode_fixup,
>  	.mode_set = intel_lvds_mode_set,
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 1/8] drm/i915: extract ironlake_set_pipeconf form ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:29AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because ironlake_crtc_mode_set is a giant function that used to have
> 404 lines. Let's try to make it less complex/confusing.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   88 ++++++++++++++++++++--------------
>  1 file changed, 52 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index e061acd..244bce6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4646,6 +4646,50 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
>  	return 120000;
>  }
>  
> +static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> +				  struct drm_display_mode *adjusted_mode,
> +				  bool dither)
> +{
> +	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	int pipe = intel_crtc->pipe;
> +	uint32_t val;
> +
> +	val = I915_READ(PIPECONF(pipe));
> +
> +	val &= ~PIPE_BPC_MASK;
> +	switch (intel_crtc->bpp) {
> +	case 18:
> +		val |= PIPE_6BPC;
> +		break;
> +	case 24:
> +		val |= PIPE_8BPC;
> +		break;
> +	case 30:
> +		val |= PIPE_10BPC;
> +		break;
> +	case 36:
> +		val |= PIPE_12BPC;
> +		break;
> +	default:
> +		val |= PIPE_8BPC;
> +		break;
> +	}
> +
> +	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> +	if (dither)
> +		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
> +
> +	val &= ~PIPECONF_INTERLACE_MASK;
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		val |= PIPECONF_INTERLACED_ILK;
> +	else
> +		val |= PIPECONF_PROGRESSIVE;
> +
> +	I915_WRITE(PIPECONF(pipe), val);
> +	POSTING_READ(PIPECONF(pipe));
> +}
> +
>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  				  struct drm_display_mode *mode,
>  				  struct drm_display_mode *adjusted_mode,
> @@ -4659,7 +4703,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int refclk, num_connectors = 0;
>  	intel_clock_t clock, reduced_clock;
> -	u32 dpll, fp = 0, fp2 = 0, dspcntr, pipeconf;
> +	u32 dpll, fp = 0, fp2 = 0, dspcntr;
>  	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;
> @@ -4768,32 +4812,17 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  		target_clock = adjusted_mode->clock;
>  
>  	/* determine panel color depth */
> -	temp = I915_READ(PIPECONF(pipe));
> -	temp &= ~PIPE_BPC_MASK;
>  	dither = intel_choose_pipe_bpp_dither(crtc, fb, &pipe_bpp, mode);
> -	switch (pipe_bpp) {
> -	case 18:
> -		temp |= PIPE_6BPC;
> -		break;
> -	case 24:
> -		temp |= PIPE_8BPC;
> -		break;
> -	case 30:
> -		temp |= PIPE_10BPC;
> -		break;
> -	case 36:
> -		temp |= PIPE_12BPC;
> -		break;
> -	default:
> +	if (is_lvds && dev_priv->lvds_dither)
> +		dither = true;
> +
> +	if (pipe_bpp != 18 && pipe_bpp != 24 && pipe_bpp != 30 &&
> +	    pipe_bpp != 36) {
>  		WARN(1, "intel_choose_pipe_bpp returned invalid value %d\n",
> -			pipe_bpp);
> -		temp |= PIPE_8BPC;
> +		     pipe_bpp);
>  		pipe_bpp = 24;
> -		break;

choose_pipe_bpp_dither already has this fallback code and we duplicate
things here, again. Can you follow up with a 2nd patch to rip this out and
add a BUG(); to the new ilk_set_pipeconf function for the default: bpp
case and directly store the bpp in intel_crtc->bpp?

Patch looks good, applied.
-Daniel
>  	}
> -
>  	intel_crtc->bpp = pipe_bpp;
> -	I915_WRITE(PIPECONF(pipe), temp);
>  
>  	if (!lane) {
>  		/*
> @@ -4877,9 +4906,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	else
>  		dpll |= PLL_REF_INPUT_DREFCLK;
>  
> -	/* setup pipeconf */
> -	pipeconf = I915_READ(PIPECONF(pipe));
> -
>  	/* Set up the display plane register */
>  	dspcntr = DISPPLANE_GAMMA_ENABLE;
>  
> @@ -4942,12 +4968,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  		I915_WRITE(PCH_LVDS, temp);
>  	}
>  
> -	pipeconf &= ~PIPECONF_DITHER_EN;
> -	pipeconf &= ~PIPECONF_DITHER_TYPE_MASK;
> -	if ((is_lvds && dev_priv->lvds_dither) || dither) {
> -		pipeconf |= PIPECONF_DITHER_EN;
> -		pipeconf |= PIPECONF_DITHER_TYPE_SP;
> -	}
>  	if (is_dp && !is_cpu_edp) {
>  		intel_dp_set_m_n(crtc, mode, adjusted_mode);
>  	} else {
> @@ -4983,9 +5003,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  		}
>  	}
>  
> -	pipeconf &= ~PIPECONF_INTERLACE_MASK;
>  	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> -		pipeconf |= PIPECONF_INTERLACED_ILK;
>  		/* the chip adds 2 halflines automatically */
>  		adjusted_mode->crtc_vtotal -= 1;
>  		adjusted_mode->crtc_vblank_end -= 1;
> @@ -4993,7 +5011,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  			   adjusted_mode->crtc_hsync_start
>  			   - adjusted_mode->crtc_htotal/2);
>  	} else {
> -		pipeconf |= PIPECONF_PROGRESSIVE;
>  		I915_WRITE(VSYNCSHIFT(pipe), 0);
>  	}
>  
> @@ -5031,8 +5048,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (is_cpu_edp)
>  		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
>  
> -	I915_WRITE(PIPECONF(pipe), pipeconf);
> -	POSTING_READ(PIPECONF(pipe));
> +	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
>  
>  	intel_wait_for_vblank(dev, pipe);
>  
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/8] drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:31AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Hm, I think we should extract the same code from i9xx_crtc_set_mode, too
and share it in a common intel_set_pipe_timings. Their almost identical
safe for:
- vsyncshift is only gen4+
- source position handling is a bit different, but I think it'd be
  semantically clearer if we leave that out of set_pipe_timings. Imo that
  belongs to the panel fitter settings, which are currently splattered all
  over. Meh.

Comments?
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |   82 +++++++++++++++++++---------------
>  1 file changed, 46 insertions(+), 36 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index cf1e628..5a4e363 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4690,6 +4690,51 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	POSTING_READ(PIPECONF(pipe));
>  }
>  
> +static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
> +				      struct drm_display_mode *mode,
> +				      struct drm_display_mode *adjusted_mode)
> +{
> +	struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
> +	enum pipe pipe = intel_crtc->pipe;
> +
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> +		/* the chip adds 2 halflines automatically */
> +		adjusted_mode->crtc_vtotal -= 1;
> +		adjusted_mode->crtc_vblank_end -= 1;
> +		I915_WRITE(VSYNCSHIFT(pipe),
> +			   adjusted_mode->crtc_hsync_start
> +			   - adjusted_mode->crtc_htotal/2);
> +	} else {
> +		I915_WRITE(VSYNCSHIFT(pipe), 0);
> +	}
> +
> +	I915_WRITE(HTOTAL(pipe),
> +		   (adjusted_mode->crtc_hdisplay - 1) |
> +		   ((adjusted_mode->crtc_htotal - 1) << 16));
> +	I915_WRITE(HBLANK(pipe),
> +		   (adjusted_mode->crtc_hblank_start - 1) |
> +		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
> +	I915_WRITE(HSYNC(pipe),
> +		   (adjusted_mode->crtc_hsync_start - 1) |
> +		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
> +
> +	I915_WRITE(VTOTAL(pipe),
> +		   (adjusted_mode->crtc_vdisplay - 1) |
> +		   ((adjusted_mode->crtc_vtotal - 1) << 16));
> +	I915_WRITE(VBLANK(pipe),
> +		   (adjusted_mode->crtc_vblank_start - 1) |
> +		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
> +	I915_WRITE(VSYNC(pipe),
> +		   (adjusted_mode->crtc_vsync_start - 1) |
> +		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
> +
> +	/* pipesrc controls the size that is scaled from, which should
> +	 * always be the user's requested size.
> +	 */
> +	I915_WRITE(PIPESRC(pipe),
> +		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +}
> +
>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  				  struct drm_display_mode *mode,
>  				  struct drm_display_mode *adjusted_mode,
> @@ -4970,42 +5015,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  		}
>  	}
>  
> -	if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
> -		/* the chip adds 2 halflines automatically */
> -		adjusted_mode->crtc_vtotal -= 1;
> -		adjusted_mode->crtc_vblank_end -= 1;
> -		I915_WRITE(VSYNCSHIFT(pipe),
> -			   adjusted_mode->crtc_hsync_start
> -			   - adjusted_mode->crtc_htotal/2);
> -	} else {
> -		I915_WRITE(VSYNCSHIFT(pipe), 0);
> -	}
> -
> -	I915_WRITE(HTOTAL(pipe),
> -		   (adjusted_mode->crtc_hdisplay - 1) |
> -		   ((adjusted_mode->crtc_htotal - 1) << 16));
> -	I915_WRITE(HBLANK(pipe),
> -		   (adjusted_mode->crtc_hblank_start - 1) |
> -		   ((adjusted_mode->crtc_hblank_end - 1) << 16));
> -	I915_WRITE(HSYNC(pipe),
> -		   (adjusted_mode->crtc_hsync_start - 1) |
> -		   ((adjusted_mode->crtc_hsync_end - 1) << 16));
> -
> -	I915_WRITE(VTOTAL(pipe),
> -		   (adjusted_mode->crtc_vdisplay - 1) |
> -		   ((adjusted_mode->crtc_vtotal - 1) << 16));
> -	I915_WRITE(VBLANK(pipe),
> -		   (adjusted_mode->crtc_vblank_start - 1) |
> -		   ((adjusted_mode->crtc_vblank_end - 1) << 16));
> -	I915_WRITE(VSYNC(pipe),
> -		   (adjusted_mode->crtc_vsync_start - 1) |
> -		   ((adjusted_mode->crtc_vsync_end - 1) << 16));
> -
> -	/* pipesrc controls the size that is scaled from, which should
> -	 * always be the user's requested size.
> -	 */
> -	I915_WRITE(PIPESRC(pipe),
> -		   ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
> +	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);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/8] drm/i915: simplify setting DSPCNTR inside ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:32AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Because declaring a variable in the beginning of the function, then
> initializing it 100 lines later, then using it 100 lines later does
> not make our code look good IMHO.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch. I've checked the gmch code and
there we have a few more bits (specifically pipe selection on gen2/3).
-Daniel
> ---
>  drivers/gpu/drm/i915/intel_display.c |    8 +++-----
>  1 file changed, 3 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5a4e363..b657416 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4748,7 +4748,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int refclk, num_connectors = 0;
>  	intel_clock_t clock, reduced_clock;
> -	u32 dpll, fp = 0, fp2 = 0, dspcntr;
> +	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;
> @@ -4950,9 +4950,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	else
>  		dpll |= PLL_REF_INPUT_DREFCLK;
>  
> -	/* Set up the display plane register */
> -	dspcntr = DISPPLANE_GAMMA_ENABLE;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5029,7 +5026,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	intel_wait_for_vblank(dev, pipe);
>  
> -	I915_WRITE(DSPCNTR(plane), dspcntr);
> +	/* Set up the display plane register */
> +	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
>  	POSTING_READ(DSPCNTR(plane));
>  
>  	ret = intel_pipe_set_base(crtc, x, y, fb);
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

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

[Daniel Vetter]

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

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/i915: extract compute_clocks from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:34AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Queued for -next, thanks for the patch. I've needed to resolve some
conflicts since I didn't pick up all previous patches, but nothing to
onerous.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/8] drm/i915: extract compute_clocks from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 04:31:50PM +0200, Daniel Vetter wrote:
> On Wed, Sep 12, 2012 at 10:06:34AM -0300, Paulo Zanoni wrote:
> > From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> > 
> > Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Queued for -next, thanks for the patch. I've needed to resolve some
> conflicts since I didn't pick up all previous patches, but nothing to
> onerous.

Wanted to add: I think the clock computation is one of the things most
responsible for tons of encoder type checks in the crtc mode_set code. My
idea is to move the clock computation out into an enhanced adjust_mode
stage, and deal with the encoder special-cases in the encoder->adjust_mode
callback.

That way we can also do (eventually) pll allocation and bandwidth
calculations/link allocations _before_ we start with the actual modeset
sequence. Which is obviously required for decent global modeset support.

But for now this is a good start to group the clock cruft together and
hide it someplace cuddly.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/8] drm/i915: extract pch_pll_set from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 12, 2012 at 10:06:35AM -0300, Paulo Zanoni wrote:
> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> 
> This is all the code responsible for setting the PCH PLLs.
> 
> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |  260 ++++++++++++++++++++--------------
>  1 file changed, 153 insertions(+), 107 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index dc4132a..7ffbcfd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4884,6 +4884,143 @@ static bool ironlake_compute_clocks(struct drm_crtc *crtc,
>  	return true;
>  }
>  
> +static bool ironlake_pch_pll_set(struct drm_crtc *crtc,
> +				 struct drm_display_mode *adjusted_mode,
> +				 intel_clock_t *clock,
> +				 bool has_reduced_clock,
> +				 intel_clock_t *reduced_clock)
> +{
> +	struct drm_device *dev = crtc->dev;
> +	struct drm_i915_private *dev_priv = dev->dev_private;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +	struct intel_encoder *intel_encoder;
> +	int num_connectors = 0;
> +	u32 dpll, fp = 0, fp2 = 0;
> +	int factor, pixel_multiplier;
> +	struct intel_pch_pll *pll;
> +	bool is_lvds = false, is_sdvo = false, is_tv = false;
> +	bool is_dp = false, is_cpu_edp = false;
> +
> +	for_each_encoder_on_crtc(dev, crtc, intel_encoder) {
> +		switch (intel_encoder->type) {
> +		case INTEL_OUTPUT_LVDS:
> +			is_lvds = true;
> +			break;
> +		case INTEL_OUTPUT_SDVO:
> +		case INTEL_OUTPUT_HDMI:
> +			is_sdvo = true;
> +			if (intel_encoder->needs_tv_clock)
> +				is_tv = true;
> +			break;
> +		case INTEL_OUTPUT_TVOUT:
> +			is_tv = true;
> +			break;
> +		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;
> +			break;
> +		}
> +
> +		num_connectors++;
> +	}
> +
> +	fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
> +	if (has_reduced_clock)
> +		fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
> +			reduced_clock->m2;
> +
> +	/* Enable autotuning of the PLL clock (if permissible) */
> +	factor = 21;
> +	if (is_lvds) {
> +		if ((intel_panel_use_ssc(dev_priv) &&
> +		     dev_priv->lvds_ssc_freq == 100) ||
> +		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> +			factor = 25;
> +	} else if (is_sdvo && is_tv)
> +		factor = 20;
> +
> +	if (clock->m < factor * clock->n)
> +		fp |= FP_CB_TUNE;
> +
> +	dpll = 0;
> +
> +	if (is_lvds)
> +		dpll |= DPLLB_MODE_LVDS;
> +	else
> +		dpll |= DPLLB_MODE_DAC_SERIAL;
> +	if (is_sdvo) {
> +		pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> +		if (pixel_multiplier > 1) {
> +			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> +		}
> +		dpll |= DPLL_DVO_HIGH_SPEED;
> +	}
> +	if (is_dp && !is_cpu_edp)
> +		dpll |= DPLL_DVO_HIGH_SPEED;
> +
> +	/* compute bitmask from p1 value */
> +	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +	/* also FPA1 */
> +	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +
> +	switch (clock->p2) {
> +	case 5:
> +		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> +		break;
> +	case 7:
> +		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> +		break;
> +	case 10:
> +		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> +		break;
> +	case 14:
> +		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> +		break;
> +	}
> +
> +	if (is_sdvo && is_tv)
> +		dpll |= PLL_REF_INPUT_TVCLKINBC;
> +	else if (is_tv)
> +		/* XXX: just matching BIOS for now */
> +		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
> +		dpll |= 3;
> +	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> +		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> +	else
> +		dpll |= PLL_REF_INPUT_DREFCLK;
> +
> +	pll = intel_get_pch_pll(intel_crtc, dpll, fp);
> +	if (!pll)
> +		return false;
> +
> +	I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> +
> +	/* Wait for the clocks to stabilize. */
> +	POSTING_READ(intel_crtc->pch_pll->pll_reg);
> +	udelay(150);
> +
> +	/* The pixel multiplier can only be updated once the
> +	 * DPLL is enabled and the clocks are stable.
> +	 *
> +	 * So write it again.
> +	 */
> +	I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> +
> +	intel_crtc->lowfreq_avail = false;
> +	if (is_lvds && has_reduced_clock && i915_powersave) {
> +		I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
> +		intel_crtc->lowfreq_avail = true;
> +	} else {
> +		I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
> +	}
> +
> +	return true;
> +}
> +
>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  				  struct drm_display_mode *mode,
>  				  struct drm_display_mode *adjusted_mode,
> @@ -4897,11 +5034,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	int plane = intel_crtc->plane;
>  	int num_connectors = 0;
>  	intel_clock_t clock, reduced_clock;
> -	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;
> -	int ret, factor;
> +	int ret;
>  	unsigned int pipe_bpp;
>  	bool dither;
>  	bool is_cpu_edp = false, is_pch_edp = false;
> @@ -4961,90 +5097,25 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	}
>  	intel_crtc->bpp = pipe_bpp;
>  
> -	fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> -	if (has_reduced_clock)
> -		fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> -			reduced_clock.m2;
> -
> -	/* Enable autotuning of the PLL clock (if permissible) */
> -	factor = 21;
> -	if (is_lvds) {
> -		if ((intel_panel_use_ssc(dev_priv) &&
> -		     dev_priv->lvds_ssc_freq == 100) ||
> -		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> -			factor = 25;
> -	} else if (is_sdvo && is_tv)
> -		factor = 20;
> -
> -	if (clock.m < factor * clock.n)
> -		fp |= FP_CB_TUNE;
> -
> -	dpll = 0;
> -
> -	if (is_lvds)
> -		dpll |= DPLLB_MODE_LVDS;
> -	else
> -		dpll |= DPLLB_MODE_DAC_SERIAL;
> -	if (is_sdvo) {
> -		int pixel_multiplier = intel_mode_get_pixel_multiplier(adjusted_mode);
> -		if (pixel_multiplier > 1) {
> -			dpll |= (pixel_multiplier - 1) << PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> -		}
> -		dpll |= DPLL_DVO_HIGH_SPEED;
> -	}
> -	if (is_dp && !is_cpu_edp)
> -		dpll |= DPLL_DVO_HIGH_SPEED;
> -
> -	/* compute bitmask from p1 value */
> -	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> -	/* also FPA1 */
> -	dpll |= (1 << (clock.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> -
> -	switch (clock.p2) {
> -	case 5:
> -		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
> -		break;
> -	case 7:
> -		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_7;
> -		break;
> -	case 10:
> -		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_10;
> -		break;
> -	case 14:
> -		dpll |= DPLLB_LVDS_P2_CLOCK_DIV_14;
> -		break;
> -	}
> -
> -	if (is_sdvo && is_tv)
> -		dpll |= PLL_REF_INPUT_TVCLKINBC;
> -	else if (is_tv)
> -		/* XXX: just matching BIOS for now */
> -		/*	dpll |= PLL_REF_INPUT_TVCLKINBC; */
> -		dpll |= 3;
> -	else if (is_lvds && intel_panel_use_ssc(dev_priv) && num_connectors < 2)
> -		dpll |= PLLB_REF_INPUT_SPREADSPECTRUMIN;
> -	else
> -		dpll |= PLL_REF_INPUT_DREFCLK;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>  	drm_mode_debug_printmodeline(mode);
>  
> -	/* CPU eDP is the only output that doesn't need a PCH PLL of its own on
> -	 * pre-Haswell/LPT generation */
> -	if (HAS_PCH_LPT(dev)) {
> -		DRM_DEBUG_KMS("LPT detected: no PLL for pipe %d necessary\n",
> -				pipe);
> -	} else if (!is_cpu_edp) {
> -		struct intel_pch_pll *pll;
> -
> -		pll = intel_get_pch_pll(intel_crtc, dpll, fp);
> -		if (pll == NULL) {
> -			DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
> -					 pipe);
> -			return -EINVAL;
> +	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {

Since the plan is to move all the hsw_crtc_stuff out into it's own
function I'd prefer a !HAS_PCH_LPT check here.

> +		/* CPU eDP is the only output that doesn't need a PCH PLL of its
> +		 * own */
> +		if (is_cpu_edp) {
> +			intel_put_pch_pll(intel_crtc);
> +		} else {
> +			ok = ironlake_pch_pll_set(crtc, adjusted_mode, &clock,
> +						  has_reduced_clock,
> +						  &reduced_clock);
> +			if (!ok) {
> +				DRM_ERROR("Failed to find PLL for pipe %d\n",
> +					  pipe);
> +				return -EINVAL;
> +			}
>  		}
> -	} else
> -		intel_put_pch_pll(intel_crtc);
> +	}
>  
>  	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
>  	 * This is an exception to the general rule that mode_set doesn't turn
> @@ -5055,31 +5126,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	ironlake_set_m_n(crtc, mode, adjusted_mode);
>  
> -	if (intel_crtc->pch_pll) {
> -		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -
> -		/* Wait for the clocks to stabilize. */
> -		POSTING_READ(intel_crtc->pch_pll->pll_reg);
> -		udelay(150);
> -
> -		/* The pixel multiplier can only be updated once the
> -		 * DPLL is enabled and the clocks are stable.
> -		 *
> -		 * So write it again.
> -		 */
> -		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -	}
> -
> -	intel_crtc->lowfreq_avail = false;
> -	if (intel_crtc->pch_pll) {
> -		if (is_lvds && has_reduced_clock && i915_powersave) {
> -			I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
> -			intel_crtc->lowfreq_avail = true;
> -		} else {
> -			I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
> -		}
> -	}
> -

Again, you're moving registers around - see the comment about that the
lvds pin pairs need to be enabled _before_ we fire up the pll. Yeah, this
is a mess and I think we should move the actual pll enabling to the
crtc_enable stage (and the also move the lvds pin pair/port enabling in
there). So
- the actuall pll enabling needs to stay here.
- maybe call the function update_pch_pll instead of set_pch_pll. This is
  more in line with Jesse's similar refactoring of i9xx_crtc_mode_set,
  where he called the split-out pll functions update_pll, too.

Cheers, Daniel
>  	ironlake_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>  
>  	if (is_cpu_edp)
> -- 
> 1.7.10.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/8] drm/i915: extract pch_pll_set from ironlake_crtc_mode_set

[Paulo Zanoni]

Hi

2012/9/12 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Sep 12, 2012 at 10:06:35AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
[ ... ]
>> +     if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>
> Since the plan is to move all the hsw_crtc_stuff out into it's own
> function I'd prefer a !HAS_PCH_LPT check here.

I don't agree with the LPT check. Let me give some more details about
my plan so you can understand why I did this.

My plan was to add even more (HAS_PCH_IBX || HAS_PCH_CPT) checks to
other places of this function and copy them all to the
haswell_crtc_mode_set version since I'm still not sure we'll never
ever have HSW with something older than LPT. After forking the Haswell
version, the plan was to add a WARN(!(HAS_PCH_IBX || HAS_PCH_CPT)) to
ironlake_crtc_mode_set and then remove the checks form it (leaving the
checks on haswell_crtc_mode_set untouched and prepared for the 42 new
PCHs they plan to ship after LPT). This code under the check was made
specifically for IBX/CPT (and PPT) and only tested on it, so I guess
the correct check is check for IBX/CPT.

So, may I keep the IBX/CPT check? Should I change the plan instead?

>> -     intel_crtc->lowfreq_avail = false;
>> -     if (intel_crtc->pch_pll) {
>> -             if (is_lvds && has_reduced_clock && i915_powersave) {
>> -                     I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
>> -                     intel_crtc->lowfreq_avail = true;
>> -             } else {
>> -                     I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
>> -             }
>> -     }
>> -
>
> Again, you're moving registers around - see the comment about that the
> lvds pin pairs need to be enabled _before_ we fire up the pll.

Oh, thanks! Let's hope that after the rework it's harder to make such
mistakes :)

> Yeah, this
> is a mess and I think we should move the actual pll enabling to the
> crtc_enable stage (and the also move the lvds pin pair/port enabling in
> there). So
> - the actuall pll enabling needs to stay here.
> - maybe call the function update_pch_pll instead of set_pch_pll. This is
>   more in line with Jesse's similar refactoring of i9xx_crtc_mode_set,
>   where he called the split-out pll functions update_pll, too.

Ok, will do.

-- 
Paulo Zanoni

Re: [PATCH 7/8] drm/i915: extract pch_pll_set from ironlake_crtc_mode_set

[Daniel Vetter]

On Tue, Sep 18, 2012 at 10:18 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> 2012/9/12 Daniel Vetter <daniel@ffwll.ch>:
>> On Wed, Sep 12, 2012 at 10:06:35AM -0300, Paulo Zanoni wrote:
>>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> [ ... ]
>>> +     if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
>>
>> Since the plan is to move all the hsw_crtc_stuff out into it's own
>> function I'd prefer a !HAS_PCH_LPT check here.
>
> I don't agree with the LPT check. Let me give some more details about
> my plan so you can understand why I did this.
>
> My plan was to add even more (HAS_PCH_IBX || HAS_PCH_CPT) checks to
> other places of this function and copy them all to the
> haswell_crtc_mode_set version since I'm still not sure we'll never
> ever have HSW with something older than LPT. After forking the Haswell
> version, the plan was to add a WARN(!(HAS_PCH_IBX || HAS_PCH_CPT)) to
> ironlake_crtc_mode_set and then remove the checks form it (leaving the
> checks on haswell_crtc_mode_set untouched and prepared for the 42 new
> PCHs they plan to ship after LPT). This code under the check was made
> specifically for IBX/CPT (and PPT) and only tested on it, so I guess
> the correct check is check for IBX/CPT.
>
> So, may I keep the IBX/CPT check? Should I change the plan instead?

Makes sense, so I'm ok with keeping these checks and code-blocks in
the haswell code. We can rip them out once haswell ships and we know
whether the hw guys want to ship 42 different pch models or not.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 3/8] drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set

[Paulo Zanoni]

Hi

2012/9/12 Daniel Vetter <daniel@ffwll.ch>:
> On Wed, Sep 12, 2012 at 10:06:31AM -0300, Paulo Zanoni wrote:
>> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
>>
>> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
>
> Hm, I think we should extract the same code from i9xx_crtc_set_mode, too
> and share it in a common intel_set_pipe_timings. Their almost identical
> safe for:
> - vsyncshift is only gen4+

This is easy to solve.

> - source position handling is a bit different, but I think it'd be
>   semantically clearer if we leave that out of set_pipe_timings. Imo that
>   belongs to the panel fitter settings, which are currently splattered all
>   over. Meh.

Well, the PIPESRC register is described inside the "pipe timings"
documentation section, so I think it should be inside the
set_pipe_timings function.

I actually implemented your suggestion locally and the only real
problem is that on i9xx_crtc_mode_set we currently write to DSPSIZE
and DSPPOS before writing to PIPESRC, so to make the code look good we
have 2 options:
1 - Write to DSPPOS and DSPSIZE before writing all the timing registers
2 - Write to DSPPOS and DSPSIZE after writing all the timing registers

In both cases we are changing the writing order. I looked at the
documentation and it seems we should be writing to the plane registers
only after setting the pipe registers, so maybe solution 2 is the
correct. The problem is that yes, we are changing the behavior and I
don't even have such machines to test.

So, how do we proceed here? Want the version, keep the old one, or do
something entirely different?

>
> Comments?
> -Daniel
>> ---
>>  drivers/gpu/drm/i915/intel_display.c |   82 +++++++++++++++++++---------------
>>  1 file changed, 46 insertions(+), 36 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
>> index cf1e628..5a4e363 100644
>> --- a/drivers/gpu/drm/i915/intel_display.c
>> +++ b/drivers/gpu/drm/i915/intel_display.c
>> @@ -4690,6 +4690,51 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>>       POSTING_READ(PIPECONF(pipe));
>>  }
>>
>> +static void ironlake_set_pipe_timings(struct intel_crtc *intel_crtc,
>> +                                   struct drm_display_mode *mode,
>> +                                   struct drm_display_mode *adjusted_mode)
>> +{
>> +     struct drm_i915_private *dev_priv = intel_crtc->base.dev->dev_private;
>> +     enum pipe pipe = intel_crtc->pipe;
>> +
>> +     if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>> +             /* the chip adds 2 halflines automatically */
>> +             adjusted_mode->crtc_vtotal -= 1;
>> +             adjusted_mode->crtc_vblank_end -= 1;
>> +             I915_WRITE(VSYNCSHIFT(pipe),
>> +                        adjusted_mode->crtc_hsync_start
>> +                        - adjusted_mode->crtc_htotal/2);
>> +     } else {
>> +             I915_WRITE(VSYNCSHIFT(pipe), 0);
>> +     }
>> +
>> +     I915_WRITE(HTOTAL(pipe),
>> +                (adjusted_mode->crtc_hdisplay - 1) |
>> +                ((adjusted_mode->crtc_htotal - 1) << 16));
>> +     I915_WRITE(HBLANK(pipe),
>> +                (adjusted_mode->crtc_hblank_start - 1) |
>> +                ((adjusted_mode->crtc_hblank_end - 1) << 16));
>> +     I915_WRITE(HSYNC(pipe),
>> +                (adjusted_mode->crtc_hsync_start - 1) |
>> +                ((adjusted_mode->crtc_hsync_end - 1) << 16));
>> +
>> +     I915_WRITE(VTOTAL(pipe),
>> +                (adjusted_mode->crtc_vdisplay - 1) |
>> +                ((adjusted_mode->crtc_vtotal - 1) << 16));
>> +     I915_WRITE(VBLANK(pipe),
>> +                (adjusted_mode->crtc_vblank_start - 1) |
>> +                ((adjusted_mode->crtc_vblank_end - 1) << 16));
>> +     I915_WRITE(VSYNC(pipe),
>> +                (adjusted_mode->crtc_vsync_start - 1) |
>> +                ((adjusted_mode->crtc_vsync_end - 1) << 16));
>> +
>> +     /* pipesrc controls the size that is scaled from, which should
>> +      * always be the user's requested size.
>> +      */
>> +     I915_WRITE(PIPESRC(pipe),
>> +                ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>> +}
>> +
>>  static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>                                 struct drm_display_mode *mode,
>>                                 struct drm_display_mode *adjusted_mode,
>> @@ -4970,42 +5015,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>>               }
>>       }
>>
>> -     if (adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE) {
>> -             /* the chip adds 2 halflines automatically */
>> -             adjusted_mode->crtc_vtotal -= 1;
>> -             adjusted_mode->crtc_vblank_end -= 1;
>> -             I915_WRITE(VSYNCSHIFT(pipe),
>> -                        adjusted_mode->crtc_hsync_start
>> -                        - adjusted_mode->crtc_htotal/2);
>> -     } else {
>> -             I915_WRITE(VSYNCSHIFT(pipe), 0);
>> -     }
>> -
>> -     I915_WRITE(HTOTAL(pipe),
>> -                (adjusted_mode->crtc_hdisplay - 1) |
>> -                ((adjusted_mode->crtc_htotal - 1) << 16));
>> -     I915_WRITE(HBLANK(pipe),
>> -                (adjusted_mode->crtc_hblank_start - 1) |
>> -                ((adjusted_mode->crtc_hblank_end - 1) << 16));
>> -     I915_WRITE(HSYNC(pipe),
>> -                (adjusted_mode->crtc_hsync_start - 1) |
>> -                ((adjusted_mode->crtc_hsync_end - 1) << 16));
>> -
>> -     I915_WRITE(VTOTAL(pipe),
>> -                (adjusted_mode->crtc_vdisplay - 1) |
>> -                ((adjusted_mode->crtc_vtotal - 1) << 16));
>> -     I915_WRITE(VBLANK(pipe),
>> -                (adjusted_mode->crtc_vblank_start - 1) |
>> -                ((adjusted_mode->crtc_vblank_end - 1) << 16));
>> -     I915_WRITE(VSYNC(pipe),
>> -                (adjusted_mode->crtc_vsync_start - 1) |
>> -                ((adjusted_mode->crtc_vsync_end - 1) << 16));
>> -
>> -     /* pipesrc controls the size that is scaled from, which should
>> -      * always be the user's requested size.
>> -      */
>> -     I915_WRITE(PIPESRC(pipe),
>> -                ((mode->hdisplay - 1) << 16) | (mode->vdisplay - 1));
>> +     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);
>> --
>> 1.7.10.4
>>
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
>
> --
> Daniel Vetter
> Software Engineer, Intel Corporation
> +41 (0) 79 365 57 48 - http://blog.ffwll.ch



-- 
Paulo Zanoni

Re: [PATCH 3/8] drm/i915: extract set_pipe_timings from ironlake_crtc_mode_set

[Daniel Vetter]

On Wed, Sep 19, 2012 at 03:11:33PM -0300, Paulo Zanoni wrote:
> Hi
> 
> 2012/9/12 Daniel Vetter <daniel@ffwll.ch>:
> > On Wed, Sep 12, 2012 at 10:06:31AM -0300, Paulo Zanoni wrote:
> >> From: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >>
> >> Signed-off-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> >
> > Hm, I think we should extract the same code from i9xx_crtc_set_mode, too
> > and share it in a common intel_set_pipe_timings. Their almost identical
> > safe for:
> > - vsyncshift is only gen4+
> 
> This is easy to solve.
> 
> > - source position handling is a bit different, but I think it'd be
> >   semantically clearer if we leave that out of set_pipe_timings. Imo that
> >   belongs to the panel fitter settings, which are currently splattered all
> >   over. Meh.
> 
> Well, the PIPESRC register is described inside the "pipe timings"
> documentation section, so I think it should be inside the
> set_pipe_timings function.
> 
> I actually implemented your suggestion locally and the only real
> problem is that on i9xx_crtc_mode_set we currently write to DSPSIZE
> and DSPPOS before writing to PIPESRC, so to make the code look good we
> have 2 options:
> 1 - Write to DSPPOS and DSPSIZE before writing all the timing registers
> 2 - Write to DSPPOS and DSPSIZE after writing all the timing registers
> 
> In both cases we are changing the writing order. I looked at the
> documentation and it seems we should be writing to the plane registers
> only after setting the pipe registers, so maybe solution 2 is the
> correct. The problem is that yes, we are changing the behavior and I
> don't even have such machines to test.
> 
> So, how do we proceed here? Want the version, keep the old one, or do
> something entirely different?

I guess a quick patch to move around the DSP* regs down and run it on a
few gen2/3 machines. Then move things around if it doesn't blow up. Since
I'm travelling I think you need to volunteer Chris for the testing ;-)
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch