[PATCH 0/7] dp dpll pipe_config conversion + random stuff

[PATCH 0/7] dp dpll pipe_config conversion + random stuff

[Daniel Vetter]

Hi all,

So I've resurrected the pch dpll conversion patch which somehow was lost and DP
now also works on ilk+. For fun I've also thrown in a few other minor patches to
make good use of pipe_config (or at least stuff I've noticed while doing
pipe_config related conversions).

Cheers, Daniel

Daniel Vetter (7):
  drm/i915: consolidate pch pll computations a bit
  drm/i915: shovel compute clock into crtc->config.dpll on ilk
  drm/i915: move dp clock computations to encoder->compute_config
  drm/i915: use pipe_config for lvds dithering
  drm/i915: don't force matching p1 for g4x/ilk+ reduced pll settings
  drm/i915: remove redundant has_pch_encoder check
  drm/i915: simplify config->pixel_multiplier handling

 drivers/gpu/drm/i915/intel_display.c | 240 +++++++++++------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  45 +++++++
 drivers/gpu/drm/i915/intel_drv.h     |   5 +
 drivers/gpu/drm/i915/intel_lvds.c    |  12 +-
 4 files changed, 130 insertions(+), 172 deletions(-)

-- 
1.7.11.7

[PATCH 1/7] drm/i915: consolidate pch pll computations a bit

[Daniel Vetter]

We need the dpll/fp/fp2 values only when we need a pch pll. So move
them together with the code to acquire such a pll.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c90605..ca2433b 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5716,7 +5716,7 @@ 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;
+	u32 dpll = 0, fp = 0, fp2 = 0;
 	bool ok, has_reduced_clock = false;
 	bool is_lvds = false;
 	struct intel_encoder *encoder;
@@ -5761,14 +5761,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (is_lvds && dev_priv->lvds_dither)
 		dither = true;
 
-	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;
-
-	dpll = ironlake_compute_dpll(intel_crtc, &clock, &fp, &reduced_clock,
-				     has_reduced_clock ? &fp2 : NULL);
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5776,6 +5768,15 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder) {
 		struct intel_pch_pll *pll;
 
+		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;
+
+		dpll = ironlake_compute_dpll(intel_crtc, &clock,
+					     &fp, &reduced_clock,
+					     has_reduced_clock ? &fp2 : NULL);
+
 		pll = intel_get_pch_pll(intel_crtc, dpll, fp);
 		if (pll == NULL) {
 			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
-- 
1.7.11.7

[PATCH 2/7] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Daniel Vetter]

This was somehow lost in the pipe_config->dpll introduction in

commit f47709a9502f3715cc488b788ca91cf0c142b1b1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Mar 28 10:42:02 2013 +0100

    drm/i915: create pipe_config->dpll for clock state

While at it, extract a few small helpers for common computations.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
 1 file changed, 29 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index ca2433b..5437a5e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -561,6 +561,11 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
 	clock->dot = clock->vco / clock->p;
 }
 
+static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
+{
+	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
+}
+
 static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
 {
 	if (IS_PINEVIEW(dev)) {
@@ -4253,6 +4258,16 @@ static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc *crtc)
 	crtc->config.clock_set = true;
 }
 
+static uint32_t pnv_dpll_compute_fp(struct dpll *dpll)
+{
+	return (1 << dpll->n) << 16 | dpll->m1 << 8 | dpll->m2;
+}
+
+static uint32_t i9xx_dpll_compute_fp(struct dpll *dpll)
+{
+	return dpll->n << 16 | dpll->m1 << 8 | dpll->m2;
+}
+
 static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
 				     intel_clock_t *reduced_clock)
 {
@@ -4260,15 +4275,14 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe = crtc->pipe;
 	u32 fp, fp2 = 0;
-	struct dpll *clock = &crtc->config.dpll;
 
 	if (IS_PINEVIEW(dev)) {
-		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
+		fp = pnv_dpll_compute_fp(&crtc->config.dpll);
 		if (reduced_clock)
 			fp2 = (1 << reduced_clock->n) << 16 |
 				reduced_clock->m1 << 8 | reduced_clock->m2;
 	} else {
-		fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
+		fp = i9xx_dpll_compute_fp(&crtc->config.dpll);
 		if (reduced_clock)
 			fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
 				reduced_clock->m2;
@@ -5604,8 +5618,13 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
 	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
 }
 
+static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
+{
+	return i9xx_dpll_compute_m(dpll) < factor * dpll->n;
+}
+
 static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
-				      intel_clock_t *clock, u32 *fp,
+				      u32 *fp,
 				      intel_clock_t *reduced_clock, u32 *fp2)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
@@ -5645,7 +5664,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	} else if (is_sdvo && is_tv)
 		factor = 20;
 
-	if (clock->m < factor * clock->n)
+	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
 		*fp |= FP_CB_TUNE;
 
 	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
@@ -5669,11 +5688,11 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
-	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	/* also FPA1 */
-	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
 
-	switch (clock->p2) {
+	switch (intel_crtc->config.dpll.p2) {
 	case 5:
 		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
 		break;
@@ -5768,12 +5787,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder) {
 		struct intel_pch_pll *pll;
 
-		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
+		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
 		if (has_reduced_clock)
 			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
 				reduced_clock.m2;
 
-		dpll = ironlake_compute_dpll(intel_crtc, &clock,
+		dpll = ironlake_compute_dpll(intel_crtc,
 					     &fp, &reduced_clock,
 					     has_reduced_clock ? &fp2 : NULL);
 
-- 
1.7.11.7

[PATCH 3/7] drm/i915: move dp clock computations to encoder->compute_config

[Daniel Vetter]

With the exception of hsw, which has dedicated DP clocks which run at
the fixed frequency already, and vlv, which doesn't have optmized
pre-defined dp clock parameters (yet).

Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 97 +-----------------------------------
 drivers/gpu/drm/i915/intel_dp.c      | 45 +++++++++++++++++
 2 files changed, 46 insertions(+), 96 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 5437a5e..d91bad8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -114,15 +114,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			intel_clock_t *best_clock);
 
 static bool
-intel_find_pll_g4x_dp(const intel_limit_t *, struct drm_crtc *crtc,
-		      int target, int refclk, intel_clock_t *match_clock,
-		      intel_clock_t *best_clock);
-static bool
-intel_find_pll_ironlake_dp(const intel_limit_t *, struct drm_crtc *crtc,
-			   int target, int refclk, intel_clock_t *match_clock,
-			   intel_clock_t *best_clock);
-
-static bool
 intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc,
 			int target, int refclk, intel_clock_t *match_clock,
 			intel_clock_t *best_clock);
@@ -254,20 +245,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
 	.find_pll = intel_g4x_find_best_PLL,
 };
 
-static const intel_limit_t intel_limits_g4x_display_port = {
-	.dot = { .min = 161670, .max = 227000 },
-	.vco = { .min = 1750000, .max = 3500000},
-	.n = { .min = 1, .max = 2 },
-	.m = { .min = 97, .max = 108 },
-	.m1 = { .min = 0x10, .max = 0x12 },
-	.m2 = { .min = 0x05, .max = 0x06 },
-	.p = { .min = 10, .max = 20 },
-	.p1 = { .min = 1, .max = 2},
-	.p2 = { .dot_limit = 0,
-		.p2_slow = 10, .p2_fast = 10 },
-	.find_pll = intel_find_pll_g4x_dp,
-};
-
 static const intel_limit_t intel_limits_pineview_sdvo = {
 	.dot = { .min = 20000, .max = 400000},
 	.vco = { .min = 1700000, .max = 3500000 },
@@ -374,20 +351,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
 	.find_pll = intel_g4x_find_best_PLL,
 };
 
-static const intel_limit_t intel_limits_ironlake_display_port = {
-	.dot = { .min = 25000, .max = 350000 },
-	.vco = { .min = 1760000, .max = 3510000},
-	.n = { .min = 1, .max = 2 },
-	.m = { .min = 81, .max = 90 },
-	.m1 = { .min = 12, .max = 22 },
-	.m2 = { .min = 5, .max = 9 },
-	.p = { .min = 10, .max = 20 },
-	.p1 = { .min = 1, .max = 2},
-	.p2 = { .dot_limit = 0,
-		.p2_slow = 10, .p2_fast = 10 },
-	.find_pll = intel_find_pll_ironlake_dp,
-};
-
 static const intel_limit_t intel_limits_vlv_dac = {
 	.dot = { .min = 25000, .max = 270000 },
 	.vco = { .min = 4000000, .max = 6000000 },
@@ -485,10 +448,7 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 			else
 				limit = &intel_limits_ironlake_single_lvds;
 		}
-	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
-		   intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
-		limit = &intel_limits_ironlake_display_port;
-	else
+	} else
 		limit = &intel_limits_ironlake_dac;
 
 	return limit;
@@ -509,8 +469,6 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
 		limit = &intel_limits_g4x_hdmi;
 	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
 		limit = &intel_limits_g4x_sdvo;
-	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
-		limit = &intel_limits_g4x_display_port;
 	} else /* The option is for other outputs */
 		limit = &intel_limits_i9xx_sdvo;
 
@@ -758,59 +716,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 }
 
 static bool
-intel_find_pll_ironlake_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
-			   int target, int refclk, intel_clock_t *match_clock,
-			   intel_clock_t *best_clock)
-{
-	struct drm_device *dev = crtc->dev;
-	intel_clock_t clock;
-
-	if (target < 200000) {
-		clock.n = 1;
-		clock.p1 = 2;
-		clock.p2 = 10;
-		clock.m1 = 12;
-		clock.m2 = 9;
-	} else {
-		clock.n = 2;
-		clock.p1 = 1;
-		clock.p2 = 10;
-		clock.m1 = 14;
-		clock.m2 = 8;
-	}
-	intel_clock(dev, refclk, &clock);
-	memcpy(best_clock, &clock, sizeof(intel_clock_t));
-	return true;
-}
-
-/* DisplayPort has only two frequencies, 162MHz and 270MHz */
-static bool
-intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
-		      int target, int refclk, intel_clock_t *match_clock,
-		      intel_clock_t *best_clock)
-{
-	intel_clock_t clock;
-	if (target < 200000) {
-		clock.p1 = 2;
-		clock.p2 = 10;
-		clock.n = 2;
-		clock.m1 = 23;
-		clock.m2 = 8;
-	} else {
-		clock.p1 = 1;
-		clock.p2 = 10;
-		clock.n = 1;
-		clock.m1 = 14;
-		clock.m2 = 2;
-	}
-	clock.m = 5 * (clock.m1 + 2) + (clock.m2 + 2);
-	clock.p = (clock.p1 * clock.p2);
-	clock.dot = 96000 * clock.m / (clock.n + 2) / clock.p;
-	clock.vco = 0;
-	memcpy(best_clock, &clock, sizeof(intel_clock_t));
-	return true;
-}
-static bool
 intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc,
 			int target, int refclk, intel_clock_t *match_clock,
 			intel_clock_t *best_clock)
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b55f3ec..b1a3a64 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -660,6 +660,49 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
 	return ret;
 }
 
+static void
+intel_dp_set_clock(struct intel_encoder *encoder,
+		   struct intel_crtc_config *pipe_config, int link_bw)
+{
+	struct drm_device *dev = encoder->base.dev;
+
+	if (IS_G4X(dev)) {
+		if (link_bw == DP_LINK_BW_1_62) {
+			pipe_config->dpll.p1 = 2;
+			pipe_config->dpll.p2 = 10;
+			pipe_config->dpll.n = 2;
+			pipe_config->dpll.m1 = 23;
+			pipe_config->dpll.m2 = 8;
+		} else {
+			pipe_config->dpll.p1 = 1;
+			pipe_config->dpll.p2 = 10;
+			pipe_config->dpll.n = 1;
+			pipe_config->dpll.m1 = 14;
+			pipe_config->dpll.m2 = 2;
+		}
+		pipe_config->clock_set = true;
+	} else if (IS_HASWELL(dev)) {
+		/* Haswell has special-purpose DP DDI clocks. */
+	} else if (HAS_PCH_SPLIT(dev)) {
+		if (link_bw == DP_LINK_BW_1_62) {
+			pipe_config->dpll.n = 1;
+			pipe_config->dpll.p1 = 2;
+			pipe_config->dpll.p2 = 10;
+			pipe_config->dpll.m1 = 12;
+			pipe_config->dpll.m2 = 9;
+		} else {
+			pipe_config->dpll.n = 2;
+			pipe_config->dpll.p1 = 1;
+			pipe_config->dpll.p2 = 10;
+			pipe_config->dpll.m1 = 14;
+			pipe_config->dpll.m2 = 8;
+		}
+		pipe_config->clock_set = true;
+	} else if (IS_VALLEYVIEW(dev)) {
+		/* FIXME: Need to figure out optimized DP clocks for vlv. */
+	}
+}
+
 bool
 intel_dp_compute_config(struct intel_encoder *encoder,
 			struct intel_crtc_config *pipe_config)
@@ -765,6 +808,8 @@ found:
 	}
 	pipe_config->pipe_bpp = bpp;
 
+	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
+
 	return true;
 }
 
-- 
1.7.11.7

[PATCH 4/7] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

Up to now we've relied on the bios to get this right for us. Let's try
out whether our code has improved a bit, since we should dither
always when the output bpp doesn't match the plane bpp.
- gen5+ should be fine, since we only use the bios hint as an upgrade.
- gen4 changes, since here dithering is still controlled in the lvds
  register.
- gen2/3 has implicit dithering depeding upon whether you use 2 or 3
  lvds pairs (which makes sense, since it only supports 8bpc pipe
  outpu configurations).
- hsw doesn't support lvds.

v2: Remove redudant dither setting.

v3: Completly drop reliance on dev_priv->lvds_dither.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_lvds.c    | 12 +++++++-----
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index d91bad8..2a82bd8 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5160,8 +5160,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 }
 
 static void ironlake_set_pipeconf(struct drm_crtc *crtc,
-				  struct drm_display_mode *adjusted_mode,
-				  bool dither)
+				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5190,7 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	}
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK;
@@ -5273,8 +5272,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
 }
 
 static void haswell_set_pipeconf(struct drm_crtc *crtc,
-				 struct drm_display_mode *adjusted_mode,
-				 bool dither)
+				 struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5284,7 +5282,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
 	val = I915_READ(PIPECONF(cpu_transcoder));
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK_HSW;
@@ -5645,7 +5643,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_lvds = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither, fdi_config_ok;
+	bool fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5680,11 +5678,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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5752,7 +5745,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
-	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
+	ironlake_set_pipeconf(crtc, adjusted_mode);
 
 	/* Set up the display plane register */
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
@@ -5829,7 +5822,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5865,9 +5857,6 @@ static int haswell_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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5881,7 +5870,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_fdi_set_m_n(crtc);
 
-	haswell_set_pipeconf(crtc, adjusted_mode, dither);
+	haswell_set_pipeconf(crtc, adjusted_mode);
 
 	intel_set_pipe_csc(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c20201d..e3ca7e7 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -201,6 +201,11 @@ struct intel_crtc_config {
 	/* DP has a bunch of special case unfortunately, so mark the pipe
 	 * accordingly. */
 	bool has_dp_encoder;
+
+	/*
+	 * Enable dithering, used when the selected pipe bpp doesn't match the
+	 * plane bpp.
+	 */
 	bool dither;
 
 	/* Controls for the clock computation, to override various stages. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 563f505..58a98ff 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * special lvds dither control bit on pch-split platforms, dithering is
 	 * only controlled through the PIPECONF reg. */
 	if (INTEL_INFO(dev)->gen == 4) {
-		if (dev_priv->lvds_dither)
+		if (intel_crtc->config.dither)
 			temp |= LVDS_ENABLE_DITHER;
 		else
 			temp &= ~LVDS_ENABLE_DITHER;
@@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
 			      pipe_config->pipe_bpp, lvds_bpp);
 		pipe_config->pipe_bpp = lvds_bpp;
+
+		/* Make sure pre-965 set dither correctly */
+		if (INTEL_INFO(dev)->gen < 4)
+			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
+
 	}
+
 	/*
 	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
@@ -470,10 +476,6 @@ out:
 		pfit_pgm_ratios = 0;
 	}
 
-	/* Make sure pre-965 set dither correctly */
-	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
-		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
-
 	if (pfit_control != lvds_encoder->pfit_control ||
 	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
 		lvds_encoder->pfit_control = pfit_control;
-- 
1.7.11.7

[PATCH 5/7] drm/i915: don't force matching p1 for g4x/ilk+ reduced pll settings

[Daniel Vetter]

g4x dplls and ilk+ pch plls have a separate field for the reduced p1
setting, so this restriction does not apply. Only older platforms have
the restriction that the p1 divisors must match.

This unnecessary restriction has been introduced in

commit cec2f356d59d9e070413e5966a3c5a1af136d948
Author: Sean Paul <seanpaul@chromium.org>
Date:   Tue Jan 10 15:09:36 2012 -0800

    drm/i915: Only look for matching clocks for LVDS downcloc

Note that with lvds the p2 divisors _always_ match for LVDS, and we
don't support auto-downclocking anywhere else. On eDP downclocking
works with separate data m/n settings, using the same link clock.

Cc:  Sean Paul <seanpaul@chromium.org>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 3 ---
 1 file changed, 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 2a82bd8..9a69d83 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -697,9 +697,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 					if (!intel_PLL_is_valid(dev, limit,
 								&clock))
 						continue;
-					if (match_clock &&
-					    clock.p != match_clock->p)
-						continue;
 
 					this_err = abs(clock.dot - target);
 					if (this_err < err_most) {
-- 
1.7.11.7

[PATCH 6/7] drm/i915: remove redundant has_pch_encoder check

[Daniel Vetter]

If we compute the pch pll state, we _have_ a pch encoder.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9a69d83..eef5abd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5583,8 +5583,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		}
 		dpll |= DPLL_DVO_HIGH_SPEED;
 	}
-	if (intel_crtc->config.has_dp_encoder &&
-	    intel_crtc->config.has_pch_encoder)
+	if (intel_crtc->config.has_dp_encoder)
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
-- 
1.7.11.7

[PATCH 7/7] drm/i915: simplify config->pixel_multiplier handling

[Daniel Vetter]

We only ever check whether it's strictly bigger than one, so all the
is_sdvo/is_hdmi checks are redundant. Flatten the code a bit.

Also, s/temp/dpll_md/

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++++++-------------------
 1 file changed, 27 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index eef5abd..6e265b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4249,7 +4249,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	u32 dpll, mdiv;
 	u32 bestn, bestm1, bestm2, bestp1, bestp2;
 	bool is_hdmi;
-	u32 coreclk, reg_val, temp;
+	u32 coreclk, reg_val, dpll_md;
 
 	mutex_lock(&dev_priv->dpio_lock);
 
@@ -4347,16 +4347,13 @@ static void vlv_update_pll(struct intel_crtc *crtc)
 	if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
 		DRM_ERROR("DPLL %d failed to lock\n", pipe);
 
-	if (is_hdmi) {
-		temp = 0;
-		if (crtc->config.pixel_multiplier > 1) {
-			temp = (crtc->config.pixel_multiplier - 1)
-				<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-		}
-
-		I915_WRITE(DPLL_MD(pipe), temp);
-		POSTING_READ(DPLL_MD(pipe));
+	dpll_md = 0;
+	if (crtc->config.pixel_multiplier > 1) {
+		dpll_md = (crtc->config.pixel_multiplier - 1)
+			<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
 	}
+	I915_WRITE(DPLL_MD(pipe), dpll_md);
+	POSTING_READ(DPLL_MD(pipe));
 
 	if (crtc->config.has_dp_encoder)
 		intel_dp_set_m_n(crtc);
@@ -4388,14 +4385,15 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
 
-	if (is_sdvo) {
-		if ((crtc->config.pixel_multiplier > 1) &&
-		    (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
-			dpll |= (crtc->config.pixel_multiplier - 1)
-				<< SDVO_MULTIPLIER_SHIFT_HIRES;
-		}
-		dpll |= DPLL_DVO_HIGH_SPEED;
+	if ((crtc->config.pixel_multiplier > 1) &&
+	    (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
+		dpll |= (crtc->config.pixel_multiplier - 1)
+			<< SDVO_MULTIPLIER_SHIFT_HIRES;
 	}
+
+	if (is_sdvo)
+		dpll |= DPLL_DVO_HIGH_SPEED;
+
 	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
@@ -4455,15 +4453,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
 	udelay(150);
 
 	if (INTEL_INFO(dev)->gen >= 4) {
-		u32 temp = 0;
-		if (is_sdvo) {
-			temp = 0;
-			if (crtc->config.pixel_multiplier > 1) {
-				temp = (crtc->config.pixel_multiplier - 1)
-					<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
-			}
+		u32 dpll_md = 0;
+		if (crtc->config.pixel_multiplier > 1) {
+			dpll_md = (crtc->config.pixel_multiplier - 1)
+				<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
 		}
-		I915_WRITE(DPLL_MD(pipe), temp);
+		I915_WRITE(DPLL_MD(pipe), dpll_md);
 	} else {
 		/* The pixel multiplier can only be updated once the
 		 * DPLL is enabled and the clocks are stable.
@@ -5576,13 +5571,14 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		dpll |= DPLLB_MODE_LVDS;
 	else
 		dpll |= DPLLB_MODE_DAC_SERIAL;
-	if (is_sdvo) {
-		if (intel_crtc->config.pixel_multiplier > 1) {
-			dpll |= (intel_crtc->config.pixel_multiplier - 1)
-				<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
-		}
-		dpll |= DPLL_DVO_HIGH_SPEED;
+
+	if (intel_crtc->config.pixel_multiplier > 1) {
+		dpll |= (intel_crtc->config.pixel_multiplier - 1)
+			<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
 	}
+
+	if (is_sdvo)
+		dpll |= DPLL_DVO_HIGH_SPEED;
 	if (intel_crtc->config.has_dp_encoder)
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
-- 
1.7.11.7

Re: [PATCH 2/7] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:32AM +0200, Daniel Vetter wrote:
> This was somehow lost in the pipe_config->dpll introduction in
> 
> commit f47709a9502f3715cc488b788ca91cf0c142b1b1
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Mar 28 10:42:02 2013 +0100
> 
>     drm/i915: create pipe_config->dpll for clock state
> 
> While at it, extract a few small helpers for common computations.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 39 +++++++++++++++++++++++++++---------
>  1 file changed, 29 insertions(+), 10 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index ca2433b..5437a5e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -561,6 +561,11 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
>  	clock->dot = clock->vco / clock->p;
>  }
>  
> +static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> +{
> +	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> +}

There are more places where this same computation appears. There
could be more unification if we either used intel_clock_t everywhere
instead if struct dpll, or if we embed struct dpll inside intel_clock_t.

> +
>  static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
>  {
>  	if (IS_PINEVIEW(dev)) {
> @@ -4253,6 +4258,16 @@ static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc *crtc)
>  	crtc->config.clock_set = true;
>  }
>  
> +static uint32_t pnv_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return (1 << dpll->n) << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
> +static uint32_t i9xx_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return dpll->n << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
>  static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  				     intel_clock_t *reduced_clock)
>  {
> @@ -4260,15 +4275,14 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe = crtc->pipe;
>  	u32 fp, fp2 = 0;
> -	struct dpll *clock = &crtc->config.dpll;
>  
>  	if (IS_PINEVIEW(dev)) {
> -		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
> +		fp = pnv_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
>  			fp2 = (1 << reduced_clock->n) << 16 |
>  				reduced_clock->m1 << 8 | reduced_clock->m2;
>  	} else {
> -		fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
> +		fp = i9xx_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
>  			fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
>  				reduced_clock->m2;
> @@ -5604,8 +5618,13 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
>  }
>  
> +static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
> +{
> +	return i9xx_dpll_compute_m(dpll) < factor * dpll->n;
> +}
> +
>  static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> -				      intel_clock_t *clock, u32 *fp,
> +				      u32 *fp,
>  				      intel_clock_t *reduced_clock, u32 *fp2)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
> @@ -5645,7 +5664,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	} else if (is_sdvo && is_tv)
>  		factor = 20;
>  
> -	if (clock->m < factor * clock->n)
> +	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
>  		*fp |= FP_CB_TUNE;
>  
>  	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
> @@ -5669,11 +5688,11 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
>  	/* compute bitmask from p1 value */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	/* also FPA1 */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
>  
> -	switch (clock->p2) {
> +	switch (intel_crtc->config.dpll.p2) {
>  	case 5:
>  		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
>  		break;
> @@ -5768,12 +5787,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder) {
>  		struct intel_pch_pll *pll;
>  
> -		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> +		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
>  		if (has_reduced_clock)
>  			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
>  				reduced_clock.m2;
>  
> -		dpll = ironlake_compute_dpll(intel_crtc, &clock,
> +		dpll = ironlake_compute_dpll(intel_crtc,
>  					     &fp, &reduced_clock,
>  					     has_reduced_clock ? &fp2 : NULL);
>  
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

[RFC][PATCH] drm/i915: Make struct dpll == intel_clock_t

[ville.syrjala]

From: Ville Syrjälä <ville.syrjala@linux.intel.com>

This allows unifying a bunch of the PLL calculations and whatnot.

Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_display.c | 12 ------------
 drivers/gpu/drm/i915/intel_drv.h     | 18 +++++++++++++-----
 2 files changed, 13 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 3c90605..74156e2 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -46,18 +46,6 @@ static void intel_increase_pllclock(struct drm_crtc *crtc);
 static void intel_crtc_update_cursor(struct drm_crtc *crtc, bool on);
 
 typedef struct {
-	/* given values */
-	int n;
-	int m1, m2;
-	int p1, p2;
-	/* derived values */
-	int	dot;
-	int	vco;
-	int	m;
-	int	p;
-} intel_clock_t;
-
-typedef struct {
 	int	min, max;
 } intel_range_t;
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index c20201d..66922f8 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -177,6 +177,18 @@ struct intel_connector {
 	u8 polled;
 };
 
+typedef struct dpll {
+	/* given values */
+	int n;
+	int m1, m2;
+	int p1, p2;
+	/* derived values */
+	int	dot;
+	int	vco;
+	int	m;
+	int	p;
+} intel_clock_t;
+
 struct intel_crtc_config {
 	struct drm_display_mode requested_mode;
 	struct drm_display_mode adjusted_mode;
@@ -208,11 +220,7 @@ struct intel_crtc_config {
 
 	/* Settings for the intel dpll used on pretty much everything but
 	 * haswell. */
-	struct dpll {
-		unsigned n;
-		unsigned m1, m2;
-		unsigned p1, p2;
-	} dpll;
+	struct dpll dpll;
 
 	int pipe_bpp;
 	struct intel_link_m_n dp_m_n;
-- 
1.8.1.5

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915: don't force matching p1 for g4x/ilk+ reduced pll settings

[Sean Paul]

On Fri, Apr 19, 2013 at 5:14 AM, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> g4x dplls and ilk+ pch plls have a separate field for the reduced p1
> setting, so this restriction does not apply. Only older platforms have
> the restriction that the p1 divisors must match.
>
> This unnecessary restriction has been introduced in
>
> commit cec2f356d59d9e070413e5966a3c5a1af136d948
> Author: Sean Paul <seanpaul@chromium.org>
> Date:   Tue Jan 10 15:09:36 2012 -0800
>
>     drm/i915: Only look for matching clocks for LVDS downcloc
>
> Note that with lvds the p2 divisors _always_ match for LVDS, and we
> don't support auto-downclocking anywhere else. On eDP downclocking
> works with separate data m/n settings, using the same link clock.
>
> Cc:  Sean Paul <seanpaul@chromium.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Sean Paul <seanpaul@chromium.org>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 ---
>  1 file changed, 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 2a82bd8..9a69d83 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -697,9 +697,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                                         if (!intel_PLL_is_valid(dev, limit,
>                                                                 &clock))
>                                                 continue;
> -                                       if (match_clock &&
> -                                           clock.p != match_clock->p)
> -                                               continue;
>
>                                         this_err = abs(clock.dot - target);
>                                         if (this_err < err_most) {
> --
> 1.7.11.7
>

Re: [RFC][PATCH] drm/i915: Make struct dpll == intel_clock_t

[Daniel Vetter]

On Fri, Apr 19, 2013 at 02:36:51PM +0300, ville.syrjala@linux.intel.com wrote:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> 
> This allows unifying a bunch of the PLL calculations and whatnot.
> 
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Yeah, makes sense. Originally my idea was to have a clear cut between
old&new pll computation code, but in hindsight less churn looks like the
better idea.

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

[PATCH] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Daniel Vetter]

This was somehow lost in the pipe_config->dpll introduction in

commit f47709a9502f3715cc488b788ca91cf0c142b1b1
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Thu Mar 28 10:42:02 2013 +0100

    drm/i915: create pipe_config->dpll for clock state

While at it, extract a few small helpers for common computations.

v2: Use the newly added helpers more thanks to Ville's trick to
typedef the legacy intel_clock_t as the new-world struct dpll.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++------------
 1 file changed, 33 insertions(+), 17 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 88c19bb..63c6557 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -549,13 +549,18 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
 	clock->dot = clock->vco / clock->p;
 }
 
+static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
+{
+	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
+}
+
 static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
 {
 	if (IS_PINEVIEW(dev)) {
 		pineview_clock(refclk, clock);
 		return;
 	}
-	clock->m = 5 * (clock->m1 + 2) + (clock->m2 + 2);
+	clock->m = i9xx_dpll_compute_m(clock);
 	clock->p = clock->p1 * clock->p2;
 	clock->vco = refclk * clock->m / (clock->n + 2);
 	clock->dot = clock->vco / clock->p;
@@ -4241,6 +4246,16 @@ static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc *crtc)
 	crtc->config.clock_set = true;
 }
 
+static uint32_t pnv_dpll_compute_fp(struct dpll *dpll)
+{
+	return (1 << dpll->n) << 16 | dpll->m1 << 8 | dpll->m2;
+}
+
+static uint32_t i9xx_dpll_compute_fp(struct dpll *dpll)
+{
+	return dpll->n << 16 | dpll->m1 << 8 | dpll->m2;
+}
+
 static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
 				     intel_clock_t *reduced_clock)
 {
@@ -4248,18 +4263,15 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	int pipe = crtc->pipe;
 	u32 fp, fp2 = 0;
-	struct dpll *clock = &crtc->config.dpll;
 
 	if (IS_PINEVIEW(dev)) {
-		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
+		fp = pnv_dpll_compute_fp(&crtc->config.dpll);
 		if (reduced_clock)
-			fp2 = (1 << reduced_clock->n) << 16 |
-				reduced_clock->m1 << 8 | reduced_clock->m2;
+			fp2 = pnv_dpll_compute_fp(reduced_clock);
 	} else {
-		fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
+		fp = i9xx_dpll_compute_fp(&crtc->config.dpll);
 		if (reduced_clock)
-			fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
-				reduced_clock->m2;
+			fp2 = i9xx_dpll_compute_fp(reduced_clock);
 	}
 
 	I915_WRITE(FP0(pipe), fp);
@@ -5592,8 +5604,13 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
 	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
 }
 
+static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
+{
+	return i9xx_dpll_compute_m(dpll) < factor * dpll->n;
+}
+
 static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
-				      intel_clock_t *clock, u32 *fp,
+				      u32 *fp,
 				      intel_clock_t *reduced_clock, u32 *fp2)
 {
 	struct drm_crtc *crtc = &intel_crtc->base;
@@ -5633,7 +5650,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	} else if (is_sdvo && is_tv)
 		factor = 20;
 
-	if (clock->m < factor * clock->n)
+	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
 		*fp |= FP_CB_TUNE;
 
 	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
@@ -5657,11 +5674,11 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 		dpll |= DPLL_DVO_HIGH_SPEED;
 
 	/* compute bitmask from p1 value */
-	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
 	/* also FPA1 */
-	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
+	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
 
-	switch (clock->p2) {
+	switch (intel_crtc->config.dpll.p2) {
 	case 5:
 		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
 		break;
@@ -5756,12 +5773,11 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder) {
 		struct intel_pch_pll *pll;
 
-		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
+		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
 		if (has_reduced_clock)
-			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
-				reduced_clock.m2;
+			fp2 = i9xx_dpll_compute_fp(&reduced_clock);
 
-		dpll = ironlake_compute_dpll(intel_crtc, &clock,
+		dpll = ironlake_compute_dpll(intel_crtc,
 					     &fp, &reduced_clock,
 					     has_reduced_clock ? &fp2 : NULL);
 
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Ville Syrjälä]

On Sat, Apr 20, 2013 at 05:19:46PM +0200, Daniel Vetter wrote:
> This was somehow lost in the pipe_config->dpll introduction in
> 
> commit f47709a9502f3715cc488b788ca91cf0c142b1b1
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Mar 28 10:42:02 2013 +0100
> 
>     drm/i915: create pipe_config->dpll for clock state
> 
> While at it, extract a few small helpers for common computations.
> 
> v2: Use the newly added helpers more thanks to Ville's trick to
> typedef the legacy intel_clock_t as the new-world struct dpll.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88c19bb..63c6557 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -549,13 +549,18 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
>  	clock->dot = clock->vco / clock->p;
>  }
>  
> +static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> +{
> +	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> +}

intel_find_pll_g4x_dp() has another use of this formula.

Otherwise everything looks good to me.

> +
>  static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
>  {
>  	if (IS_PINEVIEW(dev)) {
>  		pineview_clock(refclk, clock);
>  		return;
>  	}
> -	clock->m = 5 * (clock->m1 + 2) + (clock->m2 + 2);
> +	clock->m = i9xx_dpll_compute_m(clock);
>  	clock->p = clock->p1 * clock->p2;
>  	clock->vco = refclk * clock->m / (clock->n + 2);
>  	clock->dot = clock->vco / clock->p;
> @@ -4241,6 +4246,16 @@ static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc *crtc)
>  	crtc->config.clock_set = true;
>  }
>  
> +static uint32_t pnv_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return (1 << dpll->n) << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
> +static uint32_t i9xx_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return dpll->n << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
>  static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  				     intel_clock_t *reduced_clock)
>  {
> @@ -4248,18 +4263,15 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe = crtc->pipe;
>  	u32 fp, fp2 = 0;
> -	struct dpll *clock = &crtc->config.dpll;
>  
>  	if (IS_PINEVIEW(dev)) {
> -		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
> +		fp = pnv_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
> -			fp2 = (1 << reduced_clock->n) << 16 |
> -				reduced_clock->m1 << 8 | reduced_clock->m2;
> +			fp2 = pnv_dpll_compute_fp(reduced_clock);
>  	} else {
> -		fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
> +		fp = i9xx_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
> -			fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
> -				reduced_clock->m2;
> +			fp2 = i9xx_dpll_compute_fp(reduced_clock);
>  	}
>  
>  	I915_WRITE(FP0(pipe), fp);
> @@ -5592,8 +5604,13 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
>  }
>  
> +static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
> +{
> +	return i9xx_dpll_compute_m(dpll) < factor * dpll->n;
> +}
> +
>  static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> -				      intel_clock_t *clock, u32 *fp,
> +				      u32 *fp,
>  				      intel_clock_t *reduced_clock, u32 *fp2)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
> @@ -5633,7 +5650,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	} else if (is_sdvo && is_tv)
>  		factor = 20;
>  
> -	if (clock->m < factor * clock->n)
> +	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
>  		*fp |= FP_CB_TUNE;
>  
>  	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
> @@ -5657,11 +5674,11 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
>  	/* compute bitmask from p1 value */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	/* also FPA1 */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
>  
> -	switch (clock->p2) {
> +	switch (intel_crtc->config.dpll.p2) {
>  	case 5:
>  		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
>  		break;
> @@ -5756,12 +5773,11 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder) {
>  		struct intel_pch_pll *pll;
>  
> -		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> +		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
>  		if (has_reduced_clock)
> -			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> -				reduced_clock.m2;
> +			fp2 = i9xx_dpll_compute_fp(&reduced_clock);
>  
> -		dpll = ironlake_compute_dpll(intel_crtc, &clock,
> +		dpll = ironlake_compute_dpll(intel_crtc,
>  					     &fp, &reduced_clock,
>  					     has_reduced_clock ? &fp2 : NULL);
>  
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Daniel Vetter]

On Mon, Apr 22, 2013 at 02:13:44PM +0300, Ville Syrjälä wrote:
> On Sat, Apr 20, 2013 at 05:19:46PM +0200, Daniel Vetter wrote:
> > This was somehow lost in the pipe_config->dpll introduction in
> > 
> > commit f47709a9502f3715cc488b788ca91cf0c142b1b1
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Thu Mar 28 10:42:02 2013 +0100
> > 
> >     drm/i915: create pipe_config->dpll for clock state
> > 
> > While at it, extract a few small helpers for common computations.
> > 
> > v2: Use the newly added helpers more thanks to Ville's trick to
> > typedef the legacy intel_clock_t as the new-world struct dpll.
> > 
> > Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> > ---
> >  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++------------
> >  1 file changed, 33 insertions(+), 17 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> > index 88c19bb..63c6557 100644
> > --- a/drivers/gpu/drm/i915/intel_display.c
> > +++ b/drivers/gpu/drm/i915/intel_display.c
> > @@ -549,13 +549,18 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
> >  	clock->dot = clock->vco / clock->p;
> >  }
> >  
> > +static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> > +{
> > +	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> > +}
> 
> intel_find_pll_g4x_dp() has another use of this formula.

Oh, that one will die in fire in the next patch, so I didn't notice it
while doing the fixup.

> Otherwise everything looks good to me.

Still r-b? Or maybe just review the other ones, too ;-)

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

Re: [PATCH 1/7] drm/i915: consolidate pch pll computations a bit

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:31AM +0200, Daniel Vetter wrote:
> We need the dpll/fp/fp2 values only when we need a pch pll. So move
> them together with the code to acquire such a pll.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 19 ++++++++++---------
>  1 file changed, 10 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 3c90605..ca2433b 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5716,7 +5716,7 @@ 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;
> +	u32 dpll = 0, fp = 0, fp2 = 0;
>  	bool ok, has_reduced_clock = false;
>  	bool is_lvds = false;
>  	struct intel_encoder *encoder;
> @@ -5761,14 +5761,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (is_lvds && dev_priv->lvds_dither)
>  		dither = true;
>  
> -	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;
> -
> -	dpll = ironlake_compute_dpll(intel_crtc, &clock, &fp, &reduced_clock,
> -				     has_reduced_clock ? &fp2 : NULL);
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5776,6 +5768,15 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder) {
>  		struct intel_pch_pll *pll;
>  
> +		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;
> +
> +		dpll = ironlake_compute_dpll(intel_crtc, &clock,
> +					     &fp, &reduced_clock,
> +					     has_reduced_clock ? &fp2 : NULL);
> +
>  		pll = intel_get_pch_pll(intel_crtc, dpll, fp);
>  		if (pll == NULL) {
>  			DRM_DEBUG_DRIVER("failed to find PLL for pipe %c\n",
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH] drm/i915: shovel compute clock into crtc->config.dpll on ilk

[Ville Syrjälä]

On Sat, Apr 20, 2013 at 05:19:46PM +0200, Daniel Vetter wrote:
> This was somehow lost in the pipe_config->dpll introduction in
> 
> commit f47709a9502f3715cc488b788ca91cf0c142b1b1
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Thu Mar 28 10:42:02 2013 +0100
> 
>     drm/i915: create pipe_config->dpll for clock state
> 
> While at it, extract a few small helpers for common computations.
> 
> v2: Use the newly added helpers more thanks to Ville's trick to
> typedef the legacy intel_clock_t as the new-world struct dpll.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 50 ++++++++++++++++++++++++------------
>  1 file changed, 33 insertions(+), 17 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 88c19bb..63c6557 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -549,13 +549,18 @@ static void pineview_clock(int refclk, intel_clock_t *clock)
>  	clock->dot = clock->vco / clock->p;
>  }
>  
> +static uint32_t i9xx_dpll_compute_m(struct dpll *dpll)
> +{
> +	return 5 * (dpll->m1 + 2) + (dpll->m2 + 2);
> +}
> +
>  static void intel_clock(struct drm_device *dev, int refclk, intel_clock_t *clock)
>  {
>  	if (IS_PINEVIEW(dev)) {
>  		pineview_clock(refclk, clock);
>  		return;
>  	}
> -	clock->m = 5 * (clock->m1 + 2) + (clock->m2 + 2);
> +	clock->m = i9xx_dpll_compute_m(clock);
>  	clock->p = clock->p1 * clock->p2;
>  	clock->vco = refclk * clock->m / (clock->n + 2);
>  	clock->dot = clock->vco / clock->p;
> @@ -4241,6 +4246,16 @@ static void i9xx_adjust_sdvo_tv_clock(struct intel_crtc *crtc)
>  	crtc->config.clock_set = true;
>  }
>  
> +static uint32_t pnv_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return (1 << dpll->n) << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
> +static uint32_t i9xx_dpll_compute_fp(struct dpll *dpll)
> +{
> +	return dpll->n << 16 | dpll->m1 << 8 | dpll->m2;
> +}
> +
>  static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  				     intel_clock_t *reduced_clock)
>  {
> @@ -4248,18 +4263,15 @@ static void i9xx_update_pll_dividers(struct intel_crtc *crtc,
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	int pipe = crtc->pipe;
>  	u32 fp, fp2 = 0;
> -	struct dpll *clock = &crtc->config.dpll;
>  
>  	if (IS_PINEVIEW(dev)) {
> -		fp = (1 << clock->n) << 16 | clock->m1 << 8 | clock->m2;
> +		fp = pnv_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
> -			fp2 = (1 << reduced_clock->n) << 16 |
> -				reduced_clock->m1 << 8 | reduced_clock->m2;
> +			fp2 = pnv_dpll_compute_fp(reduced_clock);
>  	} else {
> -		fp = clock->n << 16 | clock->m1 << 8 | clock->m2;
> +		fp = i9xx_dpll_compute_fp(&crtc->config.dpll);
>  		if (reduced_clock)
> -			fp2 = reduced_clock->n << 16 | reduced_clock->m1 << 8 |
> -				reduced_clock->m2;
> +			fp2 = i9xx_dpll_compute_fp(reduced_clock);
>  	}
>  
>  	I915_WRITE(FP0(pipe), fp);
> @@ -5592,8 +5604,13 @@ static void ironlake_fdi_set_m_n(struct drm_crtc *crtc)
>  	intel_cpu_transcoder_set_m_n(intel_crtc, &m_n);
>  }
>  
> +static bool ironlake_needs_fb_cb_tune(struct dpll *dpll, int factor)
> +{
> +	return i9xx_dpll_compute_m(dpll) < factor * dpll->n;
> +}
> +
>  static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
> -				      intel_clock_t *clock, u32 *fp,
> +				      u32 *fp,
>  				      intel_clock_t *reduced_clock, u32 *fp2)
>  {
>  	struct drm_crtc *crtc = &intel_crtc->base;
> @@ -5633,7 +5650,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  	} else if (is_sdvo && is_tv)
>  		factor = 20;
>  
> -	if (clock->m < factor * clock->n)
> +	if (ironlake_needs_fb_cb_tune(&intel_crtc->config.dpll, factor))
>  		*fp |= FP_CB_TUNE;
>  
>  	if (fp2 && (reduced_clock->m < factor * reduced_clock->n))
> @@ -5657,11 +5674,11 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
>  	/* compute bitmask from p1 value */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA01_P1_POST_DIV_SHIFT;
>  	/* also FPA1 */
> -	dpll |= (1 << (clock->p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
> +	dpll |= (1 << (intel_crtc->config.dpll.p1 - 1)) << DPLL_FPA1_P1_POST_DIV_SHIFT;
>  
> -	switch (clock->p2) {
> +	switch (intel_crtc->config.dpll.p2) {
>  	case 5:
>  		dpll |= DPLL_DAC_SERIAL_P2_CLOCK_DIV_5;
>  		break;
> @@ -5756,12 +5773,11 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder) {
>  		struct intel_pch_pll *pll;
>  
> -		fp = clock.n << 16 | clock.m1 << 8 | clock.m2;
> +		fp = i9xx_dpll_compute_fp(&intel_crtc->config.dpll);
>  		if (has_reduced_clock)
> -			fp2 = reduced_clock.n << 16 | reduced_clock.m1 << 8 |
> -				reduced_clock.m2;
> +			fp2 = i9xx_dpll_compute_fp(&reduced_clock);
>  
> -		dpll = ironlake_compute_dpll(intel_crtc, &clock,
> +		dpll = ironlake_compute_dpll(intel_crtc,
>  					     &fp, &reduced_clock,
>  					     has_reduced_clock ? &fp2 : NULL);
>  
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/7] drm/i915: move dp clock computations to encoder->compute_config

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:33AM +0200, Daniel Vetter wrote:
> With the exception of hsw, which has dedicated DP clocks which run at
> the fixed frequency already, and vlv, which doesn't have optmized
> pre-defined dp clock parameters (yet).

So it looks like were still going through the full compute clocks thing,
which will possibly produce something different, and then we overwrite it
with the fixed clocks afterwards. I'm assuming that's just a transitional
issue and will get fixed later.

The only real concern I had was that the fixed clocks don't include the
derived m factor for example, but it looks like we shouldn't need that
except for the LVDS reduced clock thing.

So yeah, I think it should be OK.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> 
> Reviewed-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 97 +-----------------------------------
>  drivers/gpu/drm/i915/intel_dp.c      | 45 +++++++++++++++++
>  2 files changed, 46 insertions(+), 96 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 5437a5e..d91bad8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -114,15 +114,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>  			intel_clock_t *best_clock);
>  
>  static bool
> -intel_find_pll_g4x_dp(const intel_limit_t *, struct drm_crtc *crtc,
> -		      int target, int refclk, intel_clock_t *match_clock,
> -		      intel_clock_t *best_clock);
> -static bool
> -intel_find_pll_ironlake_dp(const intel_limit_t *, struct drm_crtc *crtc,
> -			   int target, int refclk, intel_clock_t *match_clock,
> -			   intel_clock_t *best_clock);
> -
> -static bool
>  intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc,
>  			int target, int refclk, intel_clock_t *match_clock,
>  			intel_clock_t *best_clock);
> @@ -254,20 +245,6 @@ static const intel_limit_t intel_limits_g4x_dual_channel_lvds = {
>  	.find_pll = intel_g4x_find_best_PLL,
>  };
>  
> -static const intel_limit_t intel_limits_g4x_display_port = {
> -	.dot = { .min = 161670, .max = 227000 },
> -	.vco = { .min = 1750000, .max = 3500000},
> -	.n = { .min = 1, .max = 2 },
> -	.m = { .min = 97, .max = 108 },
> -	.m1 = { .min = 0x10, .max = 0x12 },
> -	.m2 = { .min = 0x05, .max = 0x06 },
> -	.p = { .min = 10, .max = 20 },
> -	.p1 = { .min = 1, .max = 2},
> -	.p2 = { .dot_limit = 0,
> -		.p2_slow = 10, .p2_fast = 10 },
> -	.find_pll = intel_find_pll_g4x_dp,
> -};
> -
>  static const intel_limit_t intel_limits_pineview_sdvo = {
>  	.dot = { .min = 20000, .max = 400000},
>  	.vco = { .min = 1700000, .max = 3500000 },
> @@ -374,20 +351,6 @@ static const intel_limit_t intel_limits_ironlake_dual_lvds_100m = {
>  	.find_pll = intel_g4x_find_best_PLL,
>  };
>  
> -static const intel_limit_t intel_limits_ironlake_display_port = {
> -	.dot = { .min = 25000, .max = 350000 },
> -	.vco = { .min = 1760000, .max = 3510000},
> -	.n = { .min = 1, .max = 2 },
> -	.m = { .min = 81, .max = 90 },
> -	.m1 = { .min = 12, .max = 22 },
> -	.m2 = { .min = 5, .max = 9 },
> -	.p = { .min = 10, .max = 20 },
> -	.p1 = { .min = 1, .max = 2},
> -	.p2 = { .dot_limit = 0,
> -		.p2_slow = 10, .p2_fast = 10 },
> -	.find_pll = intel_find_pll_ironlake_dp,
> -};
> -
>  static const intel_limit_t intel_limits_vlv_dac = {
>  	.dot = { .min = 25000, .max = 270000 },
>  	.vco = { .min = 4000000, .max = 6000000 },
> @@ -485,10 +448,7 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
>  			else
>  				limit = &intel_limits_ironlake_single_lvds;
>  		}
> -	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT) ||
> -		   intel_pipe_has_type(crtc, INTEL_OUTPUT_EDP))
> -		limit = &intel_limits_ironlake_display_port;
> -	else
> +	} else
>  		limit = &intel_limits_ironlake_dac;
>  
>  	return limit;
> @@ -509,8 +469,6 @@ static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
>  		limit = &intel_limits_g4x_hdmi;
>  	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_SDVO)) {
>  		limit = &intel_limits_g4x_sdvo;
> -	} else if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT)) {
> -		limit = &intel_limits_g4x_display_port;
>  	} else /* The option is for other outputs */
>  		limit = &intel_limits_i9xx_sdvo;
>  
> @@ -758,59 +716,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>  }
>  
>  static bool
> -intel_find_pll_ironlake_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
> -			   int target, int refclk, intel_clock_t *match_clock,
> -			   intel_clock_t *best_clock)
> -{
> -	struct drm_device *dev = crtc->dev;
> -	intel_clock_t clock;
> -
> -	if (target < 200000) {
> -		clock.n = 1;
> -		clock.p1 = 2;
> -		clock.p2 = 10;
> -		clock.m1 = 12;
> -		clock.m2 = 9;
> -	} else {
> -		clock.n = 2;
> -		clock.p1 = 1;
> -		clock.p2 = 10;
> -		clock.m1 = 14;
> -		clock.m2 = 8;
> -	}
> -	intel_clock(dev, refclk, &clock);
> -	memcpy(best_clock, &clock, sizeof(intel_clock_t));
> -	return true;
> -}
> -
> -/* DisplayPort has only two frequencies, 162MHz and 270MHz */
> -static bool
> -intel_find_pll_g4x_dp(const intel_limit_t *limit, struct drm_crtc *crtc,
> -		      int target, int refclk, intel_clock_t *match_clock,
> -		      intel_clock_t *best_clock)
> -{
> -	intel_clock_t clock;
> -	if (target < 200000) {
> -		clock.p1 = 2;
> -		clock.p2 = 10;
> -		clock.n = 2;
> -		clock.m1 = 23;
> -		clock.m2 = 8;
> -	} else {
> -		clock.p1 = 1;
> -		clock.p2 = 10;
> -		clock.n = 1;
> -		clock.m1 = 14;
> -		clock.m2 = 2;
> -	}
> -	clock.m = 5 * (clock.m1 + 2) + (clock.m2 + 2);
> -	clock.p = (clock.p1 * clock.p2);
> -	clock.dot = 96000 * clock.m / (clock.n + 2) / clock.p;
> -	clock.vco = 0;
> -	memcpy(best_clock, &clock, sizeof(intel_clock_t));
> -	return true;
> -}
> -static bool
>  intel_vlv_find_best_pll(const intel_limit_t *limit, struct drm_crtc *crtc,
>  			int target, int refclk, intel_clock_t *match_clock,
>  			intel_clock_t *best_clock)
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index b55f3ec..b1a3a64 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -660,6 +660,49 @@ intel_dp_i2c_init(struct intel_dp *intel_dp,
>  	return ret;
>  }
>  
> +static void
> +intel_dp_set_clock(struct intel_encoder *encoder,
> +		   struct intel_crtc_config *pipe_config, int link_bw)
> +{
> +	struct drm_device *dev = encoder->base.dev;
> +
> +	if (IS_G4X(dev)) {
> +		if (link_bw == DP_LINK_BW_1_62) {
> +			pipe_config->dpll.p1 = 2;
> +			pipe_config->dpll.p2 = 10;
> +			pipe_config->dpll.n = 2;
> +			pipe_config->dpll.m1 = 23;
> +			pipe_config->dpll.m2 = 8;
> +		} else {
> +			pipe_config->dpll.p1 = 1;
> +			pipe_config->dpll.p2 = 10;
> +			pipe_config->dpll.n = 1;
> +			pipe_config->dpll.m1 = 14;
> +			pipe_config->dpll.m2 = 2;
> +		}
> +		pipe_config->clock_set = true;
> +	} else if (IS_HASWELL(dev)) {
> +		/* Haswell has special-purpose DP DDI clocks. */
> +	} else if (HAS_PCH_SPLIT(dev)) {
> +		if (link_bw == DP_LINK_BW_1_62) {
> +			pipe_config->dpll.n = 1;
> +			pipe_config->dpll.p1 = 2;
> +			pipe_config->dpll.p2 = 10;
> +			pipe_config->dpll.m1 = 12;
> +			pipe_config->dpll.m2 = 9;
> +		} else {
> +			pipe_config->dpll.n = 2;
> +			pipe_config->dpll.p1 = 1;
> +			pipe_config->dpll.p2 = 10;
> +			pipe_config->dpll.m1 = 14;
> +			pipe_config->dpll.m2 = 8;
> +		}
> +		pipe_config->clock_set = true;
> +	} else if (IS_VALLEYVIEW(dev)) {
> +		/* FIXME: Need to figure out optimized DP clocks for vlv. */
> +	}
> +}
> +
>  bool
>  intel_dp_compute_config(struct intel_encoder *encoder,
>  			struct intel_crtc_config *pipe_config)
> @@ -765,6 +808,8 @@ found:
>  	}
>  	pipe_config->pipe_bpp = bpp;
>  
> +	intel_dp_set_clock(encoder, pipe_config, intel_dp->link_bw);
> +
>  	return true;
>  }
>  
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/7] drm/i915: use pipe_config for lvds dithering

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote:
> Up to now we've relied on the bios to get this right for us. Let's try
> out whether our code has improved a bit, since we should dither
> always when the output bpp doesn't match the plane bpp.
> - gen5+ should be fine, since we only use the bios hint as an upgrade.
> - gen4 changes, since here dithering is still controlled in the lvds
>   register.
> - gen2/3 has implicit dithering depeding upon whether you use 2 or 3
>   lvds pairs (which makes sense, since it only supports 8bpc pipe
>   outpu configurations).
> - hsw doesn't support lvds.
> 
> v2: Remove redudant dither setting.
> 
> v3: Completly drop reliance on dev_priv->lvds_dither.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_lvds.c    | 12 +++++++-----
>  3 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index d91bad8..2a82bd8 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5160,8 +5160,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
>  }
>  
>  static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> -				  struct drm_display_mode *adjusted_mode,
> -				  bool dither)
> +				  struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5190,7 +5189,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	}
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK;
> @@ -5273,8 +5272,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
>  }
>  
>  static void haswell_set_pipeconf(struct drm_crtc *crtc,
> -				 struct drm_display_mode *adjusted_mode,
> -				 bool dither)
> +				 struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5284,7 +5282,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
>  	val = I915_READ(PIPECONF(cpu_transcoder));
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK_HSW;
> @@ -5645,7 +5643,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_lvds = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither, fdi_config_ok;
> +	bool fdi_config_ok;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5680,11 +5678,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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -	if (is_lvds && dev_priv->lvds_dither)
> -		dither = true;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5752,7 +5745,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
>  
> -	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
> +	ironlake_set_pipeconf(crtc, adjusted_mode);
>  
>  	/* Set up the display plane register */
>  	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
> @@ -5829,7 +5822,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_cpu_edp = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5865,9 +5857,6 @@ static int haswell_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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5881,7 +5870,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder)
>  		ironlake_fdi_set_m_n(crtc);
>  
> -	haswell_set_pipeconf(crtc, adjusted_mode, dither);
> +	haswell_set_pipeconf(crtc, adjusted_mode);
>  
>  	intel_set_pipe_csc(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index c20201d..e3ca7e7 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -201,6 +201,11 @@ struct intel_crtc_config {
>  	/* DP has a bunch of special case unfortunately, so mark the pipe
>  	 * accordingly. */
>  	bool has_dp_encoder;
> +
> +	/*
> +	 * Enable dithering, used when the selected pipe bpp doesn't match the
> +	 * plane bpp.
> +	 */
>  	bool dither;
>  
>  	/* Controls for the clock computation, to override various stages. */
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 563f505..58a98ff 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	 * special lvds dither control bit on pch-split platforms, dithering is
>  	 * only controlled through the PIPECONF reg. */
>  	if (INTEL_INFO(dev)->gen == 4) {
> -		if (dev_priv->lvds_dither)
> +		if (intel_crtc->config.dither)
>  			temp |= LVDS_ENABLE_DITHER;
>  		else
>  			temp &= ~LVDS_ENABLE_DITHER;
> @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
>  			      pipe_config->pipe_bpp, lvds_bpp);
>  		pipe_config->pipe_bpp = lvds_bpp;
> +
> +		/* Make sure pre-965 set dither correctly */
> +		if (INTEL_INFO(dev)->gen < 4)
> +			pfit_control |= PANEL_8TO6_DITHER_ENABLE;

I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe
always 8bpc, and then we should enable dithering on the port/pfit
when lvds is 6bpc.

Right now I think we'll start with pipe_bpp=18 when the primary plane
surface is 16bpp, and then we wouldn't enable dithering here for 6bpc
lvds.

> +
>  	}
> +
>  	/*
>  	 * We have timings from the BIOS for the panel, put them in
>  	 * to the adjusted mode.  The CRTC will be set up for this mode,
> @@ -470,10 +476,6 @@ out:
>  		pfit_pgm_ratios = 0;
>  	}
>  
> -	/* Make sure pre-965 set dither correctly */
> -	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
> -		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> -
>  	if (pfit_control != lvds_encoder->pfit_control ||
>  	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
>  		lvds_encoder->pfit_control = pfit_control;
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 6/7] drm/i915: remove redundant has_pch_encoder check

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:36AM +0200, Daniel Vetter wrote:
> If we compute the pch pll state, we _have_ a pch encoder.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 9a69d83..eef5abd 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5583,8 +5583,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  		}
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  	}
> -	if (intel_crtc->config.has_dp_encoder &&
> -	    intel_crtc->config.has_pch_encoder)
> +	if (intel_crtc->config.has_dp_encoder)
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
>  	/* compute bitmask from p1 value */
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/7] drm/i915: move dp clock computations to encoder->compute_config

[Daniel Vetter]

On Thu, Apr 25, 2013 at 02:34:39PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 19, 2013 at 11:14:33AM +0200, Daniel Vetter wrote:
> > With the exception of hsw, which has dedicated DP clocks which run at
> > the fixed frequency already, and vlv, which doesn't have optmized
> > pre-defined dp clock parameters (yet).
> 
> So it looks like were still going through the full compute clocks thing,
> which will possibly produce something different, and then we overwrite it
> with the fixed clocks afterwards. I'm assuming that's just a transitional
> issue and will get fixed later.

Yeah, I guess I should have spilled a few more words about where I
eventually want to end up with this.

So ultimately my idea is that in the compute config stage first the crtc
code puts the default platform pll limits into the pipe_config. Then
encoders can either overwrite that limit structure with their own special
stuff (mostly for lvds madness). Or they can pick some or all of the
parameters (e.g. just the p2 switchover on hdmi, or all the clock
parameters for dp/sdvo tv).

Once that's done then the generic crtc code can fill out any missing bits
(using the find_best_pll code) and then try to assign which pll to use (if
it's a platform with shared plls). In the end the modeset could should
simply write the computed stuff into registers and never be able to fail.

Of course there's still a lot of data to be moved into pipe_config to make
this all happen, hence some of the temporary ugliness.

> The only real concern I had was that the fixed clocks don't include the
> derived m factor for example, but it looks like we shouldn't need that
> except for the LVDS reduced clock thing.

The idea behind leaving out m (and compute dotclock/vco) from the
pipe_config stuff is that we don't store that into the hw. It's only
really used in the pll computation. So once this has all settled I think
we should rip this all out from the clock structure and compute it
as-needed in the find_best_pll functions. I'm working on some patches
already to move into that direction a bit.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] drm/i915: simplify config->pixel_multiplier handling

[Ville Syrjälä]

On Fri, Apr 19, 2013 at 11:14:37AM +0200, Daniel Vetter wrote:
> We only ever check whether it's strictly bigger than one, so all the
> is_sdvo/is_hdmi checks are redundant. Flatten the code a bit.
> 
> Also, s/temp/dpll_md/
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>


Looks OK. Note that we actually never set pixel_multipler on VLV since
it doesn't support SDVO, but supposedly it could be used for HDMI too.
So I guess it doesn't hurt to keep the code for it.

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 58 +++++++++++++++++-------------------
>  1 file changed, 27 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index eef5abd..6e265b0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4249,7 +4249,7 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>  	u32 dpll, mdiv;
>  	u32 bestn, bestm1, bestm2, bestp1, bestp2;
>  	bool is_hdmi;
> -	u32 coreclk, reg_val, temp;
> +	u32 coreclk, reg_val, dpll_md;
>  
>  	mutex_lock(&dev_priv->dpio_lock);
>  
> @@ -4347,16 +4347,13 @@ static void vlv_update_pll(struct intel_crtc *crtc)
>  	if (wait_for(((I915_READ(DPLL(pipe)) & DPLL_LOCK_VLV) == DPLL_LOCK_VLV), 1))
>  		DRM_ERROR("DPLL %d failed to lock\n", pipe);
>  
> -	if (is_hdmi) {
> -		temp = 0;
> -		if (crtc->config.pixel_multiplier > 1) {
> -			temp = (crtc->config.pixel_multiplier - 1)
> -				<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
> -		}
> -
> -		I915_WRITE(DPLL_MD(pipe), temp);
> -		POSTING_READ(DPLL_MD(pipe));
> +	dpll_md = 0;
> +	if (crtc->config.pixel_multiplier > 1) {
> +		dpll_md = (crtc->config.pixel_multiplier - 1)
> +			<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
>  	}
> +	I915_WRITE(DPLL_MD(pipe), dpll_md);
> +	POSTING_READ(DPLL_MD(pipe));
>  
>  	if (crtc->config.has_dp_encoder)
>  		intel_dp_set_m_n(crtc);
> @@ -4388,14 +4385,15 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  	else
>  		dpll |= DPLLB_MODE_DAC_SERIAL;
>  
> -	if (is_sdvo) {
> -		if ((crtc->config.pixel_multiplier > 1) &&
> -		    (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
> -			dpll |= (crtc->config.pixel_multiplier - 1)
> -				<< SDVO_MULTIPLIER_SHIFT_HIRES;
> -		}
> -		dpll |= DPLL_DVO_HIGH_SPEED;
> +	if ((crtc->config.pixel_multiplier > 1) &&
> +	    (IS_I945G(dev) || IS_I945GM(dev) || IS_G33(dev))) {
> +		dpll |= (crtc->config.pixel_multiplier - 1)
> +			<< SDVO_MULTIPLIER_SHIFT_HIRES;
>  	}
> +
> +	if (is_sdvo)
> +		dpll |= DPLL_DVO_HIGH_SPEED;
> +
>  	if (intel_pipe_has_type(&crtc->base, INTEL_OUTPUT_DISPLAYPORT))
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
> @@ -4455,15 +4453,12 @@ static void i9xx_update_pll(struct intel_crtc *crtc,
>  	udelay(150);
>  
>  	if (INTEL_INFO(dev)->gen >= 4) {
> -		u32 temp = 0;
> -		if (is_sdvo) {
> -			temp = 0;
> -			if (crtc->config.pixel_multiplier > 1) {
> -				temp = (crtc->config.pixel_multiplier - 1)
> -					<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
> -			}
> +		u32 dpll_md = 0;
> +		if (crtc->config.pixel_multiplier > 1) {
> +			dpll_md = (crtc->config.pixel_multiplier - 1)
> +				<< DPLL_MD_UDI_MULTIPLIER_SHIFT;
>  		}
> -		I915_WRITE(DPLL_MD(pipe), temp);
> +		I915_WRITE(DPLL_MD(pipe), dpll_md);
>  	} else {
>  		/* The pixel multiplier can only be updated once the
>  		 * DPLL is enabled and the clocks are stable.
> @@ -5576,13 +5571,14 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>  		dpll |= DPLLB_MODE_LVDS;
>  	else
>  		dpll |= DPLLB_MODE_DAC_SERIAL;
> -	if (is_sdvo) {
> -		if (intel_crtc->config.pixel_multiplier > 1) {
> -			dpll |= (intel_crtc->config.pixel_multiplier - 1)
> -				<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
> -		}
> -		dpll |= DPLL_DVO_HIGH_SPEED;
> +
> +	if (intel_crtc->config.pixel_multiplier > 1) {
> +		dpll |= (intel_crtc->config.pixel_multiplier - 1)
> +			<< PLL_REF_SDVO_HDMI_MULTIPLIER_SHIFT;
>  	}
> +
> +	if (is_sdvo)
> +		dpll |= DPLL_DVO_HIGH_SPEED;
>  	if (intel_crtc->config.has_dp_encoder)
>  		dpll |= DPLL_DVO_HIGH_SPEED;
>  
> -- 
> 1.7.11.7
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 3/7] drm/i915: move dp clock computations to encoder->compute_config

[Ville Syrjälä]

On Thu, Apr 25, 2013 at 02:04:45PM +0200, Daniel Vetter wrote:
> On Thu, Apr 25, 2013 at 02:34:39PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 19, 2013 at 11:14:33AM +0200, Daniel Vetter wrote:
> > > With the exception of hsw, which has dedicated DP clocks which run at
> > > the fixed frequency already, and vlv, which doesn't have optmized
> > > pre-defined dp clock parameters (yet).
> > 
> > So it looks like were still going through the full compute clocks thing,
> > which will possibly produce something different, and then we overwrite it
> > with the fixed clocks afterwards. I'm assuming that's just a transitional
> > issue and will get fixed later.
> 
> Yeah, I guess I should have spilled a few more words about where I
> eventually want to end up with this.
> 
> So ultimately my idea is that in the compute config stage first the crtc
> code puts the default platform pll limits into the pipe_config. Then
> encoders can either overwrite that limit structure with their own special
> stuff (mostly for lvds madness). Or they can pick some or all of the
> parameters (e.g. just the p2 switchover on hdmi, or all the clock
> parameters for dp/sdvo tv).
> 
> Once that's done then the generic crtc code can fill out any missing bits
> (using the find_best_pll code) and then try to assign which pll to use (if
> it's a platform with shared plls). In the end the modeset could should
> simply write the computed stuff into registers and never be able to fail.
> 
> Of course there's still a lot of data to be moved into pipe_config to make
> this all happen, hence some of the temporary ugliness.
> 
> > The only real concern I had was that the fixed clocks don't include the
> > derived m factor for example, but it looks like we shouldn't need that
> > except for the LVDS reduced clock thing.
> 
> The idea behind leaving out m (and compute dotclock/vco) from the
> pipe_config stuff is that we don't store that into the hw. It's only
> really used in the pll computation. So once this has all settled I think
> we should rip this all out from the clock structure and compute it
> as-needed in the find_best_pll functions. I'm working on some patches
> already to move into that direction a bit.

Yeah I had the same idea about always computing the derived stuff
on-demand always. Would remove the risk of accidentally using an
unpopulated value. Of course it wasn't such a big issue before my
intel_clock_to == struct dpll patch, since you didn't have the
derived stuff in the struct dpll in the first place. Win some,
lose some.

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/7] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote:
> > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > index 563f505..58a98ff 100644
> > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
> >  	 * special lvds dither control bit on pch-split platforms, dithering is
> >  	 * only controlled through the PIPECONF reg. */
> >  	if (INTEL_INFO(dev)->gen == 4) {
> > -		if (dev_priv->lvds_dither)
> > +		if (intel_crtc->config.dither)
> >  			temp |= LVDS_ENABLE_DITHER;
> >  		else
> >  			temp &= ~LVDS_ENABLE_DITHER;
> > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> >  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
> >  			      pipe_config->pipe_bpp, lvds_bpp);
> >  		pipe_config->pipe_bpp = lvds_bpp;
> > +
> > +		/* Make sure pre-965 set dither correctly */
> > +		if (INTEL_INFO(dev)->gen < 4)
> > +			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> 
> I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe
> always 8bpc, and then we should enable dithering on the port/pfit
> when lvds is 6bpc.
> 
> Right now I think we'll start with pipe_bpp=18 when the primary plane
> surface is 16bpp, and then we wouldn't enable dithering here for 6bpc
> lvds.

Yeah, the patch does slightly change behaviour as we no longer blindly
follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane
(even if the pipe is running internally at 8bpc) is a bit pointless, since
there's simply no intermediate levels to dither down to 6bpc. Otoh just
using the dither flag unconditionally gives us a notch more unified code.
So I've opted for that.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 7/7] drm/i915: simplify config->pixel_multiplier handling

[Daniel Vetter]

On Thu, Apr 25, 2013 at 03:08:32PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 19, 2013 at 11:14:37AM +0200, Daniel Vetter wrote:
> > We only ever check whether it's strictly bigger than one, so all the
> > is_sdvo/is_hdmi checks are redundant. Flatten the code a bit.
> > 
> > Also, s/temp/dpll_md/
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> Looks OK. Note that we actually never set pixel_multipler on VLV since
> it doesn't support SDVO, but supposedly it could be used for HDMI too.
> So I guess it doesn't hurt to keep the code for it.

Yeah, finally enabling correct pixel multiplier support for hdmi is on our
eternal todo list. Last time I've tried it something blew up and it didn't
really work out too well. We need to look into this again.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 4/7] drm/i915: use pipe_config for lvds dithering

[Ville Syrjälä]

On Thu, Apr 25, 2013 at 02:24:50PM +0200, Daniel Vetter wrote:
> On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote:
> > On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote:
> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> > > index 563f505..58a98ff 100644
> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
> > > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
> > >  	 * special lvds dither control bit on pch-split platforms, dithering is
> > >  	 * only controlled through the PIPECONF reg. */
> > >  	if (INTEL_INFO(dev)->gen == 4) {
> > > -		if (dev_priv->lvds_dither)
> > > +		if (intel_crtc->config.dither)
> > >  			temp |= LVDS_ENABLE_DITHER;
> > >  		else
> > >  			temp &= ~LVDS_ENABLE_DITHER;
> > > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
> > >  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
> > >  			      pipe_config->pipe_bpp, lvds_bpp);
> > >  		pipe_config->pipe_bpp = lvds_bpp;
> > > +
> > > +		/* Make sure pre-965 set dither correctly */
> > > +		if (INTEL_INFO(dev)->gen < 4)
> > > +			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> > 
> > I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe
> > always 8bpc, and then we should enable dithering on the port/pfit
> > when lvds is 6bpc.
> > 
> > Right now I think we'll start with pipe_bpp=18 when the primary plane
> > surface is 16bpp, and then we wouldn't enable dithering here for 6bpc
> > lvds.
> 
> Yeah, the patch does slightly change behaviour as we no longer blindly
> follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane
> (even if the pipe is running internally at 8bpc) is a bit pointless, since
> there's simply no intermediate levels to dither down to 6bpc. Otoh just
> using the dither flag unconditionally gives us a notch more unified code.
> So I've opted for that.

I was just wondering what happens when we have 16bpp surface so
pipe_bpp is 18, and then we have 24bit lvds which means this hunk of code
will enable PANEL_8TO6_DITHER_ENABLE. Is that going to produce something
that still looks sensible?

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 4/7] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

On Thu, Apr 25, 2013 at 2:42 PM, Ville Syrjälä
<ville.syrjala@linux.intel.com> wrote:
> On Thu, Apr 25, 2013 at 02:24:50PM +0200, Daniel Vetter wrote:
>> On Thu, Apr 25, 2013 at 02:57:16PM +0300, Ville Syrjälä wrote:
>> > On Fri, Apr 19, 2013 at 11:14:34AM +0200, Daniel Vetter wrote:
>> > > diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
>> > > index 563f505..58a98ff 100644
>> > > --- a/drivers/gpu/drm/i915/intel_lvds.c
>> > > +++ b/drivers/gpu/drm/i915/intel_lvds.c
>> > > @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>> > >    * special lvds dither control bit on pch-split platforms, dithering is
>> > >    * only controlled through the PIPECONF reg. */
>> > >   if (INTEL_INFO(dev)->gen == 4) {
>> > > -         if (dev_priv->lvds_dither)
>> > > +         if (intel_crtc->config.dither)
>> > >                   temp |= LVDS_ENABLE_DITHER;
>> > >           else
>> > >                   temp &= ~LVDS_ENABLE_DITHER;
>> > > @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>> > >           DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
>> > >                         pipe_config->pipe_bpp, lvds_bpp);
>> > >           pipe_config->pipe_bpp = lvds_bpp;
>> > > +
>> > > +         /* Make sure pre-965 set dither correctly */
>> > > +         if (INTEL_INFO(dev)->gen < 4)
>> > > +                 pfit_control |= PANEL_8TO6_DITHER_ENABLE;
>> >
>> > I'm not quite sure about the gen4 and earlier stuff. Isn't the pipe
>> > always 8bpc, and then we should enable dithering on the port/pfit
>> > when lvds is 6bpc.
>> >
>> > Right now I think we'll start with pipe_bpp=18 when the primary plane
>> > surface is 16bpp, and then we wouldn't enable dithering here for 6bpc
>> > lvds.
>>
>> Yeah, the patch does slightly change behaviour as we no longer blindly
>> follow the bios wrt dithering lvds. And imo trying to dither a 16bpp plane
>> (even if the pipe is running internally at 8bpc) is a bit pointless, since
>> there's simply no intermediate levels to dither down to 6bpc. Otoh just
>> using the dither flag unconditionally gives us a notch more unified code.
>> So I've opted for that.
>
> I was just wondering what happens when we have 16bpp surface so
> pipe_bpp is 18, and then we have 24bit lvds which means this hunk of code
> will enable PANEL_8TO6_DITHER_ENABLE. Is that going to produce something
> that still looks sensible?

Hm, indeed I've ignored this case a bit. I guess it would result in
something ok-ish, but I'm not sure. I'll do a similar check to what
I've added for g4x/vlv in another patch for 30bpp modes, and simply
only enabling dithering when we think it's needed and when we think
the "pipe" runs at 18bpp. I'll update the patch a bit.
-Daniel
--
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

[PATCH] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

Up to now we've relied on the bios to get this right for us. Let's try
out whether our code has improved a bit, since we should dither
always when the output bpp doesn't match the plane bpp.
- gen5+ should be fine, since we only use the bios hint as an upgrade.
- gen4 changes, since here dithering is still controlled in the lvds
  register.
- gen2/3 has implicit dithering depeding upon whether you use 2 or 3
  lvds pairs (which makes sense, since it only supports 8bpc pipe
  outpu configurations).
- hsw doesn't support lvds.

v2: Remove redudant dither setting.

v3: Completly drop reliance on dev_priv->lvds_dither.

v4: Enable dithering on gen2/3 only when we have a 18bpp panel, since
up-dithering to a 24bpp panel is not supported by the hw. Spotted by
Ville.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_lvds.c    | 12 +++++++-----
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16106fe..f26a867 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5156,8 +5156,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 }
 
 static void ironlake_set_pipeconf(struct drm_crtc *crtc,
-				  struct drm_display_mode *adjusted_mode,
-				  bool dither)
+				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5186,7 +5185,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	}
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK;
@@ -5269,8 +5268,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
 }
 
 static void haswell_set_pipeconf(struct drm_crtc *crtc,
-				 struct drm_display_mode *adjusted_mode,
-				 bool dither)
+				 struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5280,7 +5278,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
 	val = I915_READ(PIPECONF(cpu_transcoder));
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK_HSW;
@@ -5641,7 +5639,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_lvds = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither, fdi_config_ok;
+	bool fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5676,11 +5674,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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5747,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
-	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
+	ironlake_set_pipeconf(crtc, adjusted_mode);
 
 	/* Set up the display plane register */
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
@@ -5824,7 +5817,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5860,9 +5852,6 @@ static int haswell_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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5876,7 +5865,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_fdi_set_m_n(crtc);
 
-	haswell_set_pipeconf(crtc, adjusted_mode, dither);
+	haswell_set_pipeconf(crtc, adjusted_mode);
 
 	intel_set_pipe_csc(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66922f8..723cac9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -213,6 +213,11 @@ struct intel_crtc_config {
 	/* DP has a bunch of special case unfortunately, so mark the pipe
 	 * accordingly. */
 	bool has_dp_encoder;
+
+	/*
+	 * Enable dithering, used when the selected pipe bpp doesn't match the
+	 * plane bpp.
+	 */
 	bool dither;
 
 	/* Controls for the clock computation, to override various stages. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 563f505..fdfc820 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * special lvds dither control bit on pch-split platforms, dithering is
 	 * only controlled through the PIPECONF reg. */
 	if (INTEL_INFO(dev)->gen == 4) {
-		if (dev_priv->lvds_dither)
+		if (intel_crtc->config.dither)
 			temp |= LVDS_ENABLE_DITHER;
 		else
 			temp &= ~LVDS_ENABLE_DITHER;
@@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
 			      pipe_config->pipe_bpp, lvds_bpp);
 		pipe_config->pipe_bpp = lvds_bpp;
+
+		/* Make sure pre-965 set dither correctly for 18bpp panels. */
+		if (INTEL_INFO(dev)->gen < 4 && lvds_bpp == 18)
+			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
+
 	}
+
 	/*
 	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
@@ -470,10 +476,6 @@ out:
 		pfit_pgm_ratios = 0;
 	}
 
-	/* Make sure pre-965 set dither correctly */
-	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
-		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
-
 	if (pfit_control != lvds_encoder->pfit_control ||
 	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
 		lvds_encoder->pfit_control = pfit_control;
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use pipe_config for lvds dithering

[Ville Syrjälä]

On Thu, Apr 25, 2013 at 03:20:12PM +0200, Daniel Vetter wrote:
> Up to now we've relied on the bios to get this right for us. Let's try
> out whether our code has improved a bit, since we should dither
> always when the output bpp doesn't match the plane bpp.
> - gen5+ should be fine, since we only use the bios hint as an upgrade.
> - gen4 changes, since here dithering is still controlled in the lvds
>   register.
> - gen2/3 has implicit dithering depeding upon whether you use 2 or 3
>   lvds pairs (which makes sense, since it only supports 8bpc pipe
>   outpu configurations).
> - hsw doesn't support lvds.
> 
> v2: Remove redudant dither setting.
> 
> v3: Completly drop reliance on dev_priv->lvds_dither.
> 
> v4: Enable dithering on gen2/3 only when we have a 18bpp panel, since
> up-dithering to a 24bpp panel is not supported by the hw. Spotted by
> Ville.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_lvds.c    | 12 +++++++-----
>  3 files changed, 19 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 16106fe..f26a867 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5156,8 +5156,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
>  }
>  
>  static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> -				  struct drm_display_mode *adjusted_mode,
> -				  bool dither)
> +				  struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5186,7 +5185,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	}
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK;
> @@ -5269,8 +5268,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
>  }
>  
>  static void haswell_set_pipeconf(struct drm_crtc *crtc,
> -				 struct drm_display_mode *adjusted_mode,
> -				 bool dither)
> +				 struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5280,7 +5278,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
>  	val = I915_READ(PIPECONF(cpu_transcoder));
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK_HSW;
> @@ -5641,7 +5639,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_lvds = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither, fdi_config_ok;
> +	bool fdi_config_ok;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5676,11 +5674,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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -	if (is_lvds && dev_priv->lvds_dither)
> -		dither = true;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5747,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
>  
> -	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
> +	ironlake_set_pipeconf(crtc, adjusted_mode);
>  
>  	/* Set up the display plane register */
>  	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
> @@ -5824,7 +5817,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_cpu_edp = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5860,9 +5852,6 @@ static int haswell_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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5876,7 +5865,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder)
>  		ironlake_fdi_set_m_n(crtc);
>  
> -	haswell_set_pipeconf(crtc, adjusted_mode, dither);
> +	haswell_set_pipeconf(crtc, adjusted_mode);
>  
>  	intel_set_pipe_csc(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 66922f8..723cac9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -213,6 +213,11 @@ struct intel_crtc_config {
>  	/* DP has a bunch of special case unfortunately, so mark the pipe
>  	 * accordingly. */
>  	bool has_dp_encoder;
> +
> +	/*
> +	 * Enable dithering, used when the selected pipe bpp doesn't match the
> +	 * plane bpp.
> +	 */
>  	bool dither;
>  
>  	/* Controls for the clock computation, to override various stages. */
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 563f505..fdfc820 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	 * special lvds dither control bit on pch-split platforms, dithering is
>  	 * only controlled through the PIPECONF reg. */
>  	if (INTEL_INFO(dev)->gen == 4) {
> -		if (dev_priv->lvds_dither)
> +		if (intel_crtc->config.dither)

The description of the lvds port dither bit for gen4 also seems to
suggest it should only be enabled for 18bpp lvds.

>  			temp |= LVDS_ENABLE_DITHER;
>  		else
>  			temp &= ~LVDS_ENABLE_DITHER;
> @@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
>  			      pipe_config->pipe_bpp, lvds_bpp);
>  		pipe_config->pipe_bpp = lvds_bpp;
> +
> +		/* Make sure pre-965 set dither correctly for 18bpp panels. */
> +		if (INTEL_INFO(dev)->gen < 4 && lvds_bpp == 18)
> +			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> +
>  	}
> +
>  	/*
>  	 * We have timings from the BIOS for the panel, put them in
>  	 * to the adjusted mode.  The CRTC will be set up for this mode,
> @@ -470,10 +476,6 @@ out:
>  		pfit_pgm_ratios = 0;
>  	}
>  
> -	/* Make sure pre-965 set dither correctly */
> -	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
> -		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> -
>  	if (pfit_control != lvds_encoder->pfit_control ||
>  	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
>  		lvds_encoder->pfit_control = pfit_control;
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

[PATCH] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

Up to now we've relied on the bios to get this right for us. Let's try
out whether our code has improved a bit, since we should dither
always when the output bpp doesn't match the plane bpp.
- gen5+ should be fine, since we only use the bios hint as an upgrade.
- gen4 changes, since here dithering is still controlled in the lvds
  register.
- gen2/3 has implicit dithering depeding upon whether you use 2 or 3
  lvds pairs (which makes sense, since it only supports 8bpc pipe
  outpu configurations).
- hsw doesn't support lvds.

v2: Remove redudant dither setting.

v3: Completly drop reliance on dev_priv->lvds_dither.

v4: Enable dithering on gen2/3 only when we have a 18bpp panel, since
up-dithering to a 24bpp panel is not supported by the hw. Spotted by
Ville.

v5: Also only enable lvds port dithering on gen4 for 18bpp modes. In
practice this only excludes dithering a 10bpc plane down for a 24bpp
lvds panel. Not something we truly care about. Again noticed by Ville.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_lvds.c    | 12 +++++++-----
 3 files changed, 19 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16106fe..f26a867 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5156,8 +5156,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 }
 
 static void ironlake_set_pipeconf(struct drm_crtc *crtc,
-				  struct drm_display_mode *adjusted_mode,
-				  bool dither)
+				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5186,7 +5185,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	}
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK;
@@ -5269,8 +5268,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
 }
 
 static void haswell_set_pipeconf(struct drm_crtc *crtc,
-				 struct drm_display_mode *adjusted_mode,
-				 bool dither)
+				 struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5280,7 +5278,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
 	val = I915_READ(PIPECONF(cpu_transcoder));
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK_HSW;
@@ -5641,7 +5639,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_lvds = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither, fdi_config_ok;
+	bool fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5676,11 +5674,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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5747,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
-	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
+	ironlake_set_pipeconf(crtc, adjusted_mode);
 
 	/* Set up the display plane register */
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
@@ -5824,7 +5817,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5860,9 +5852,6 @@ static int haswell_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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5876,7 +5865,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_fdi_set_m_n(crtc);
 
-	haswell_set_pipeconf(crtc, adjusted_mode, dither);
+	haswell_set_pipeconf(crtc, adjusted_mode);
 
 	intel_set_pipe_csc(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66922f8..723cac9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -213,6 +213,11 @@ struct intel_crtc_config {
 	/* DP has a bunch of special case unfortunately, so mark the pipe
 	 * accordingly. */
 	bool has_dp_encoder;
+
+	/*
+	 * Enable dithering, used when the selected pipe bpp doesn't match the
+	 * plane bpp.
+	 */
 	bool dither;
 
 	/* Controls for the clock computation, to override various stages. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 563f505..fdfc820 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -136,7 +136,7 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * special lvds dither control bit on pch-split platforms, dithering is
 	 * only controlled through the PIPECONF reg. */
 	if (INTEL_INFO(dev)->gen == 4) {
-		if (dev_priv->lvds_dither)
+		if (intel_crtc->config.dither)
 			temp |= LVDS_ENABLE_DITHER;
 		else
 			temp &= ~LVDS_ENABLE_DITHER;
@@ -335,7 +335,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
 			      pipe_config->pipe_bpp, lvds_bpp);
 		pipe_config->pipe_bpp = lvds_bpp;
+
+		/* Make sure pre-965 set dither correctly for 18bpp panels. */
+		if (INTEL_INFO(dev)->gen < 4 && lvds_bpp == 18)
+			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
+
 	}
+
 	/*
 	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
@@ -470,10 +476,6 @@ out:
 		pfit_pgm_ratios = 0;
 	}
 
-	/* Make sure pre-965 set dither correctly */
-	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
-		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
-
 	if (pfit_control != lvds_encoder->pfit_control ||
 	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
 		lvds_encoder->pfit_control = pfit_control;
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH] drm/i915: use pipe_config for lvds dithering

[Daniel Vetter]

Up to now we've relied on the bios to get this right for us. Let's try
out whether our code has improved a bit, since we should dither
always when the output bpp doesn't match the plane bpp.
- gen5+ should be fine, since we only use the bios hint as an upgrade.
- gen4 changes, since here dithering is still controlled in the lvds
  register.
- gen2/3 has implicit dithering depeding upon whether you use 2 or 3
  lvds pairs (which makes sense, since it only supports 8bpc pipe
  outpu configurations).
- hsw doesn't support lvds.

v2: Remove redudant dither setting.

v3: Completly drop reliance on dev_priv->lvds_dither.

v4: Enable dithering on gen2/3 only when we have a 18bpp panel, since
up-dithering to a 24bpp panel is not supported by the hw. Spotted by
Ville.

v5: Also only enable lvds port dithering on gen4 for 18bpp modes. In
practice this only excludes dithering a 10bpc plane down for a 24bpp
lvds panel. Not something we truly care about. Again noticed by Ville.

v6: Actually git add.

Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
 drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
 drivers/gpu/drm/i915/intel_lvds.c    | 15 ++++++++++-----
 3 files changed, 22 insertions(+), 23 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 16106fe..f26a867 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5156,8 +5156,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 }
 
 static void ironlake_set_pipeconf(struct drm_crtc *crtc,
-				  struct drm_display_mode *adjusted_mode,
-				  bool dither)
+				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5186,7 +5185,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
 	}
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK;
@@ -5269,8 +5268,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
 }
 
 static void haswell_set_pipeconf(struct drm_crtc *crtc,
-				 struct drm_display_mode *adjusted_mode,
-				 bool dither)
+				 struct drm_display_mode *adjusted_mode)
 {
 	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
@@ -5280,7 +5278,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
 	val = I915_READ(PIPECONF(cpu_transcoder));
 
 	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
-	if (dither)
+	if (intel_crtc->config.dither)
 		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
 
 	val &= ~PIPECONF_INTERLACE_MASK_HSW;
@@ -5641,7 +5639,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_lvds = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither, fdi_config_ok;
+	bool fdi_config_ok;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5676,11 +5674,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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5747,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
-	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
+	ironlake_set_pipeconf(crtc, adjusted_mode);
 
 	/* Set up the display plane register */
 	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
@@ -5824,7 +5817,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	bool is_cpu_edp = false;
 	struct intel_encoder *encoder;
 	int ret;
-	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -5860,9 +5852,6 @@ static int haswell_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);
 
-	/* determine panel color depth */
-	dither = intel_crtc->config.dither;
-
 	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
 	drm_mode_debug_printmodeline(mode);
 
@@ -5876,7 +5865,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (intel_crtc->config.has_pch_encoder)
 		ironlake_fdi_set_m_n(crtc);
 
-	haswell_set_pipeconf(crtc, adjusted_mode, dither);
+	haswell_set_pipeconf(crtc, adjusted_mode);
 
 	intel_set_pipe_csc(crtc);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 66922f8..723cac9 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -213,6 +213,11 @@ struct intel_crtc_config {
 	/* DP has a bunch of special case unfortunately, so mark the pipe
 	 * accordingly. */
 	bool has_dp_encoder;
+
+	/*
+	 * Enable dithering, used when the selected pipe bpp doesn't match the
+	 * plane bpp.
+	 */
 	bool dither;
 
 	/* Controls for the clock computation, to override various stages. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 563f505..8408545 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -136,7 +136,10 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * special lvds dither control bit on pch-split platforms, dithering is
 	 * only controlled through the PIPECONF reg. */
 	if (INTEL_INFO(dev)->gen == 4) {
-		if (dev_priv->lvds_dither)
+		/* Bspec wording suggests that LVDS port dithering only exists
+		 * for 18bpp panels. */
+		if (intel_crtc->config.dither &&
+		    intel_crtc->config.pipe_bpp == 18)
 			temp |= LVDS_ENABLE_DITHER;
 		else
 			temp &= ~LVDS_ENABLE_DITHER;
@@ -335,7 +338,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
 		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
 			      pipe_config->pipe_bpp, lvds_bpp);
 		pipe_config->pipe_bpp = lvds_bpp;
+
+		/* Make sure pre-965 set dither correctly for 18bpp panels. */
+		if (INTEL_INFO(dev)->gen < 4 && lvds_bpp == 18)
+			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
+
 	}
+
 	/*
 	 * We have timings from the BIOS for the panel, put them in
 	 * to the adjusted mode.  The CRTC will be set up for this mode,
@@ -470,10 +479,6 @@ out:
 		pfit_pgm_ratios = 0;
 	}
 
-	/* Make sure pre-965 set dither correctly */
-	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
-		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
-
 	if (pfit_control != lvds_encoder->pfit_control ||
 	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
 		lvds_encoder->pfit_control = pfit_control;
-- 
1.7.11.7

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: use pipe_config for lvds dithering

[Ville Syrjälä]

On Thu, Apr 25, 2013 at 05:54:44PM +0200, Daniel Vetter wrote:
> Up to now we've relied on the bios to get this right for us. Let's try
> out whether our code has improved a bit, since we should dither
> always when the output bpp doesn't match the plane bpp.
> - gen5+ should be fine, since we only use the bios hint as an upgrade.
> - gen4 changes, since here dithering is still controlled in the lvds
>   register.
> - gen2/3 has implicit dithering depeding upon whether you use 2 or 3
>   lvds pairs (which makes sense, since it only supports 8bpc pipe
>   outpu configurations).
> - hsw doesn't support lvds.
> 
> v2: Remove redudant dither setting.
> 
> v3: Completly drop reliance on dev_priv->lvds_dither.
> 
> v4: Enable dithering on gen2/3 only when we have a 18bpp panel, since
> up-dithering to a 24bpp panel is not supported by the hw. Spotted by
> Ville.
> 
> v5: Also only enable lvds port dithering on gen4 for 18bpp modes. In
> practice this only excludes dithering a 10bpc plane down for a 24bpp
> lvds panel. Not something we truly care about. Again noticed by Ville.
> 
> v6: Actually git add.
> 
> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 25 +++++++------------------
>  drivers/gpu/drm/i915/intel_drv.h     |  5 +++++
>  drivers/gpu/drm/i915/intel_lvds.c    | 15 ++++++++++-----
>  3 files changed, 22 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 16106fe..f26a867 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5156,8 +5156,7 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
>  }
>  
>  static void ironlake_set_pipeconf(struct drm_crtc *crtc,
> -				  struct drm_display_mode *adjusted_mode,
> -				  bool dither)
> +				  struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5186,7 +5185,7 @@ static void ironlake_set_pipeconf(struct drm_crtc *crtc,
>  	}
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK;
> @@ -5269,8 +5268,7 @@ static void intel_set_pipe_csc(struct drm_crtc *crtc)
>  }
>  
>  static void haswell_set_pipeconf(struct drm_crtc *crtc,
> -				 struct drm_display_mode *adjusted_mode,
> -				 bool dither)
> +				 struct drm_display_mode *adjusted_mode)
>  {
>  	struct drm_i915_private *dev_priv = crtc->dev->dev_private;
>  	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> @@ -5280,7 +5278,7 @@ static void haswell_set_pipeconf(struct drm_crtc *crtc,
>  	val = I915_READ(PIPECONF(cpu_transcoder));
>  
>  	val &= ~(PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_MASK);
> -	if (dither)
> +	if (intel_crtc->config.dither)
>  		val |= (PIPECONF_DITHER_EN | PIPECONF_DITHER_TYPE_SP);
>  
>  	val &= ~PIPECONF_INTERLACE_MASK_HSW;
> @@ -5641,7 +5639,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_lvds = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither, fdi_config_ok;
> +	bool fdi_config_ok;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5676,11 +5674,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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -	if (is_lvds && dev_priv->lvds_dither)
> -		dither = true;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5747,7 +5740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>  
>  	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
>  
> -	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
> +	ironlake_set_pipeconf(crtc, adjusted_mode);
>  
>  	/* Set up the display plane register */
>  	I915_WRITE(DSPCNTR(plane), DISPPLANE_GAMMA_ENABLE);
> @@ -5824,7 +5817,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	bool is_cpu_edp = false;
>  	struct intel_encoder *encoder;
>  	int ret;
> -	bool dither;
>  
>  	for_each_encoder_on_crtc(dev, crtc, encoder) {
>  		switch (encoder->type) {
> @@ -5860,9 +5852,6 @@ static int haswell_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);
>  
> -	/* determine panel color depth */
> -	dither = intel_crtc->config.dither;
> -
>  	DRM_DEBUG_KMS("Mode for pipe %c:\n", pipe_name(pipe));
>  	drm_mode_debug_printmodeline(mode);
>  
> @@ -5876,7 +5865,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>  	if (intel_crtc->config.has_pch_encoder)
>  		ironlake_fdi_set_m_n(crtc);
>  
> -	haswell_set_pipeconf(crtc, adjusted_mode, dither);
> +	haswell_set_pipeconf(crtc, adjusted_mode);
>  
>  	intel_set_pipe_csc(crtc);
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index 66922f8..723cac9 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -213,6 +213,11 @@ struct intel_crtc_config {
>  	/* DP has a bunch of special case unfortunately, so mark the pipe
>  	 * accordingly. */
>  	bool has_dp_encoder;
> +
> +	/*
> +	 * Enable dithering, used when the selected pipe bpp doesn't match the
> +	 * plane bpp.
> +	 */
>  	bool dither;
>  
>  	/* Controls for the clock computation, to override various stages. */
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 563f505..8408545 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -136,7 +136,10 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
>  	 * special lvds dither control bit on pch-split platforms, dithering is
>  	 * only controlled through the PIPECONF reg. */
>  	if (INTEL_INFO(dev)->gen == 4) {
> -		if (dev_priv->lvds_dither)
> +		/* Bspec wording suggests that LVDS port dithering only exists
> +		 * for 18bpp panels. */
> +		if (intel_crtc->config.dither &&
> +		    intel_crtc->config.pipe_bpp == 18)
>  			temp |= LVDS_ENABLE_DITHER;
>  		else
>  			temp &= ~LVDS_ENABLE_DITHER;
> @@ -335,7 +338,13 @@ static bool intel_lvds_compute_config(struct intel_encoder *intel_encoder,
>  		DRM_DEBUG_KMS("forcing display bpp (was %d) to LVDS (%d)\n",
>  			      pipe_config->pipe_bpp, lvds_bpp);
>  		pipe_config->pipe_bpp = lvds_bpp;
> +
> +		/* Make sure pre-965 set dither correctly for 18bpp panels. */
> +		if (INTEL_INFO(dev)->gen < 4 && lvds_bpp == 18)
> +			pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> +
>  	}
> +
>  	/*
>  	 * We have timings from the BIOS for the panel, put them in
>  	 * to the adjusted mode.  The CRTC will be set up for this mode,
> @@ -470,10 +479,6 @@ out:
>  		pfit_pgm_ratios = 0;
>  	}
>  
> -	/* Make sure pre-965 set dither correctly */
> -	if (INTEL_INFO(dev)->gen < 4 && dev_priv->lvds_dither)
> -		pfit_control |= PANEL_8TO6_DITHER_ENABLE;
> -
>  	if (pfit_control != lvds_encoder->pfit_control ||
>  	    pfit_pgm_ratios != lvds_encoder->pfit_pgm_ratios) {
>  		lvds_encoder->pfit_control = pfit_control;
> -- 
> 1.7.11.7

-- 
Ville Syrjälä
Intel OTC

Re: [PATCH 7/7] drm/i915: simplify config->pixel_multiplier handling

[Daniel Vetter]

On Thu, Apr 25, 2013 at 03:08:32PM +0300, Ville Syrjälä wrote:
> On Fri, Apr 19, 2013 at 11:14:37AM +0200, Daniel Vetter wrote:
> > We only ever check whether it's strictly bigger than one, so all the
> > is_sdvo/is_hdmi checks are redundant. Flatten the code a bit.
> > 
> > Also, s/temp/dpll_md/
> > 
> > Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> 
> 
> Looks OK. Note that we actually never set pixel_multipler on VLV since
> it doesn't support SDVO, but supposedly it could be used for HDMI too.
> So I guess it doesn't hurt to keep the code for it.
> 
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Merged the entire series, with the commit message for patch 3 massively
pimped with my follow up. Thanks a lot for the review.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch