[PATCH 00/17] lvds/dp/watermark refactorings

[PATCH 00/17] lvds/dp/watermark refactorings

[Daniel Vetter]

Hi all,

Second round for my lvds/dp/watermark refactorings, which are a prep work for
the next step in reworking the modeset code (which is currently brewing at a
rather slow rate in my modeset-rework branch). Safe where I've disagreed this
should incorporate all review feedback from Paulo Zanoni.

Comments&flames highly welcome.

Thanks, Daniel

Daniel Vetter (17):
  drm/i915: add encoder->pre_pll_enable callback
  drm/i915: replace ad-hoc dual-link lvds checks
  drm/i915: move is_dual_link_lvds to intel_lvds.c
  drm/i915: track is_dual_link in intel_lvds
  drm/i915: add intel_lvds->reg
  drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable
  drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too
  drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set
  drm/i915: move set_pll_edp to intel_dp.c
  drm/i915: rip out pre-production ilk cpu edp w/a
  drm/i915: use wait_for_vblank instead of msleep(17)
  drm/i915: WARN on !crtc in intel_dp_link_down
  drm/i915: drop unnecessary clearing of pch dp transcoder timings
  drm/i915: extract common link_m_n helpers
  drm/i915: don't call update_watermark in crtc_mode_set
  drm/i915: don't call update_watermarks from haswell enable/disable
    code
  drm/i915: rip out update_linetime_wm abstraction

 drivers/gpu/drm/i915/i915_drv.h      |  16 +-
 drivers/gpu/drm/i915/intel_display.c | 430 +++++------------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  87 ++++---
 drivers/gpu/drm/i915/intel_drv.h     |   4 +-
 drivers/gpu/drm/i915/intel_lvds.c    | 160 +++++++++++--
 drivers/gpu/drm/i915/intel_pm.c      |  37 ---
 6 files changed, 258 insertions(+), 476 deletions(-)

-- 
1.7.11.7

[PATCH 01/17] drm/i915: add encoder->pre_pll_enable callback

[Daniel Vetter]

Currently we have two encoder specific bits in the common mode_set
functions:
- lvds pin pair enabling
- dp m/n setting and computation

Now the lvds stuff needs to happen before the pll is enabled. Since
that is done in the crtc_mode_set functions, we need to add a new
callback to be able to move them to the encoder code (where they
belong). The dp m/n stuff is a giant mess anyway (since it also
confuses itself with the fdi link m/n handling), so that needs to be
handled separately.

I think that we can move the pll enabling down quite a bit, which
might allow us to eventually merge encoder->pre_enable with this new
pre_pll_enable callback. But for now this will allow us to clean
things up a bit.

Note that vlv doesn't support lvds, hence we don't need to change
anything in there.

v2: Fixup commit message, both suggested from Paulo Zanoni.
- dp m/n doesn't need to happen before pll enabling
- lvds doesn't exist on vlv, hence no changes required in the vlv pll
  function.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 2 files changed, 15 insertions(+)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 769fc8f..647f16e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4462,6 +4462,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	u32 dpll;
 	bool is_sdvo;
@@ -4530,6 +4531,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 	POSTING_READ(DPLL(pipe));
 	udelay(150);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
 	 * This is an exception to the general rule that mode_set doesn't turn
 	 * things on.
@@ -4574,6 +4579,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
+	struct intel_encoder *encoder;
 	int pipe = intel_crtc->pipe;
 	u32 dpll;
 
@@ -4607,6 +4613,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 	POSTING_READ(DPLL(pipe));
 	udelay(150);
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
 	 * This is an exception to the general rule that mode_set doesn't turn
 	 * things on.
@@ -5535,6 +5545,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
 	}
 
+	for_each_encoder_on_crtc(dev, crtc, encoder)
+		if (encoder->pre_pll_enable)
+			encoder->pre_pll_enable(encoder);
+
 	if (intel_crtc->pch_pll) {
 		I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index bcc5241..42a40a1 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -153,6 +153,7 @@ struct intel_encoder {
 	bool cloneable;
 	bool connectors_active;
 	void (*hot_plug)(struct intel_encoder *);
+	void (*pre_pll_enable)(struct intel_encoder *);
 	void (*pre_enable)(struct intel_encoder *);
 	void (*enable)(struct intel_encoder *);
 	void (*disable)(struct intel_encoder *);
-- 
1.7.11.7

[PATCH 02/17] drm/i915: replace ad-hoc dual-link lvds checks

[Daniel Vetter]

... with is_dual_link_lvds introduced in

commit b03543857fd75876b96e10d4320b775e95041bb7
Author: Takashi Iwai <tiwai@suse.de>
Date:   Tue Mar 20 13:07:05 2012 +0100

    drm/i915: Check VBIOS value for determining LVDS dual channel mode, too

All these checks predate this commit and have simply been overlooked.
Since we don't support switching between single-link and dual-link
modes anyway, this different checks could at best only get in the way
of refactorings, and in the worst case cause inconsistencies.

v2: Update the comment, we now have a solid way to figure out whether
we need dual-link lvds or not (falling back to vbt values as a last
resort). We still don't know how to switch between dual-link and
single link so leave that part intact. I'm not sure though whether
switching between these two modes makes any sense - we always drive
the panel at its fixed mode (with a fixed bpc) anyway ...

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 647f16e..dd4cb43 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -690,13 +690,11 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 	intel_clock_t clock;
 	int err = target;
 
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
-	    (I915_READ(LVDS)) != 0) {
+	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
 		/*
-		 * For LVDS, if the panel is on, just rely on its current
-		 * settings for dual-channel.  We haven't figured out how to
-		 * reliably set up different single/dual channel state, if we
-		 * even can.
+		 * For LVDS just rely on its current settings for dual-channel.
+		 * We haven't figured out how to reliably set up different
+		 * single/dual channel state, if we even can.
 		 */
 		if (is_dual_link_lvds(dev_priv, LVDS))
 			clock.p2 = limit->p2.p2_fast;
@@ -766,8 +764,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			lvds_reg = PCH_LVDS;
 		else
 			lvds_reg = LVDS;
-		if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
-		    LVDS_CLKB_POWER_UP)
+		if (is_dual_link_lvds(dev_priv, lvds_reg))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -5357,7 +5354,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (is_lvds) {
 		if ((intel_panel_use_ssc(dev_priv) &&
 		     dev_priv->lvds_ssc_freq == 100) ||
-		    (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
+		    is_dual_link_lvds(dev_priv, PCH_LVDS))
 			factor = 25;
 	} else if (is_sdvo && is_tv)
 		factor = 20;
-- 
1.7.11.7

[PATCH 03/17] drm/i915: move is_dual_link_lvds to intel_lvds.c

[Daniel Vetter]

Just a prep patch to make this a property of intel_lvds. Makes more
sense, removes clutter from intel_display.c and eventually I want to
move all the encoder special cases wrt clock handling to encoders
anyway.

v2: Add an intel_ prefixe to is_dual_link_lvds since it's non-static
now.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 60 +++---------------------------------
 drivers/gpu/drm/i915/intel_drv.h     |  1 +
 drivers/gpu/drm/i915/intel_lvds.c    | 53 +++++++++++++++++++++++++++++++
 3 files changed, 59 insertions(+), 55 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index dd4cb43..1048595 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -472,61 +472,14 @@ static void vlv_init_dpio(struct drm_device *dev)
 	POSTING_READ(DPIO_CTL);
 }
 
-static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
-{
-	DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
-	return 1;
-}
-
-static const struct dmi_system_id intel_dual_link_lvds[] = {
-	{
-		.callback = intel_dual_link_lvds_callback,
-		.ident = "Apple MacBook Pro (Core i5/i7 Series)",
-		.matches = {
-			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
-			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
-		},
-	},
-	{ }	/* terminating entry */
-};
-
-static bool is_dual_link_lvds(struct drm_i915_private *dev_priv,
-			      unsigned int reg)
-{
-	unsigned int val;
-
-	/* use the module option value if specified */
-	if (i915_lvds_channel_mode > 0)
-		return i915_lvds_channel_mode == 2;
-
-	if (dmi_check_system(intel_dual_link_lvds))
-		return true;
-
-	if (dev_priv->lvds_val)
-		val = dev_priv->lvds_val;
-	else {
-		/* BIOS should set the proper LVDS register value at boot, but
-		 * in reality, it doesn't set the value when the lid is closed;
-		 * we need to check "the value to be set" in VBT when LVDS
-		 * register is uninitialized.
-		 */
-		val = I915_READ(reg);
-		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
-			val = dev_priv->bios_lvds_val;
-		dev_priv->lvds_val = val;
-	}
-	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
-}
-
 static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 						int refclk)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if (is_dual_link_lvds(dev_priv, PCH_LVDS)) {
+		if (intel_is_dual_link_lvds(dev)) {
 			/* LVDS dual channel */
 			if (refclk == 100000)
 				limit = &intel_limits_ironlake_dual_lvds_100m;
@@ -550,11 +503,10 @@ static const intel_limit_t *intel_ironlake_limit(struct drm_crtc *crtc,
 static const intel_limit_t *intel_g4x_limit(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	const intel_limit_t *limit;
 
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
-		if (is_dual_link_lvds(dev_priv, LVDS))
+		if (intel_is_dual_link_lvds(dev))
 			/* LVDS with dual channel */
 			limit = &intel_limits_g4x_dual_channel_lvds;
 		else
@@ -686,7 +638,6 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	intel_clock_t clock;
 	int err = target;
 
@@ -696,7 +647,7 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 		 * We haven't figured out how to reliably set up different
 		 * single/dual channel state, if we even can.
 		 */
-		if (is_dual_link_lvds(dev_priv, LVDS))
+		if (intel_is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -749,7 +700,6 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			intel_clock_t *best_clock)
 {
 	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
 	intel_clock_t clock;
 	int max_n;
 	bool found;
@@ -764,7 +714,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
 			lvds_reg = PCH_LVDS;
 		else
 			lvds_reg = LVDS;
-		if (is_dual_link_lvds(dev_priv, lvds_reg))
+		if (intel_is_dual_link_lvds(dev))
 			clock.p2 = limit->p2.p2_fast;
 		else
 			clock.p2 = limit->p2.p2_slow;
@@ -5354,7 +5304,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
 	if (is_lvds) {
 		if ((intel_panel_use_ssc(dev_priv) &&
 		     dev_priv->lvds_ssc_freq == 100) ||
-		    is_dual_link_lvds(dev_priv, PCH_LVDS))
+		    intel_is_dual_link_lvds(dev))
 			factor = 25;
 	} else if (is_sdvo && is_tv)
 		factor = 20;
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 42a40a1..3614fbb 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -439,6 +439,7 @@ extern void intel_mark_idle(struct drm_device *dev);
 extern void intel_mark_fb_busy(struct drm_i915_gem_object *obj);
 extern void intel_mark_fb_idle(struct drm_i915_gem_object *obj);
 extern bool intel_lvds_init(struct drm_device *dev);
+extern bool intel_is_dual_link_lvds(struct drm_device *dev);
 extern void intel_dp_init(struct drm_device *dev, int output_reg,
 			  enum port port);
 extern void intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index ffa0051..84255d0 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -900,6 +900,59 @@ static bool lvds_is_present_in_vbt(struct drm_device *dev,
 	return false;
 }
 
+static int intel_dual_link_lvds_callback(const struct dmi_system_id *id)
+{
+	DRM_INFO("Forcing lvds to dual link mode on %s\n", id->ident);
+	return 1;
+}
+
+static const struct dmi_system_id intel_dual_link_lvds[] = {
+	{
+		.callback = intel_dual_link_lvds_callback,
+		.ident = "Apple MacBook Pro (Core i5/i7 Series)",
+		.matches = {
+			DMI_MATCH(DMI_SYS_VENDOR, "Apple Inc."),
+			DMI_MATCH(DMI_PRODUCT_NAME, "MacBookPro8,2"),
+		},
+	},
+	{ }	/* terminating entry */
+};
+
+bool intel_is_dual_link_lvds(struct drm_device *dev)
+{
+	unsigned int val;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 lvds_reg;
+
+	if (HAS_PCH_SPLIT(dev)) {
+		lvds_reg = PCH_LVDS;
+	} else {
+		lvds_reg = LVDS;
+	}
+
+	/* use the module option value if specified */
+	if (i915_lvds_channel_mode > 0)
+		return i915_lvds_channel_mode == 2;
+
+	if (dmi_check_system(intel_dual_link_lvds))
+		return true;
+
+	if (dev_priv->lvds_val)
+		val = dev_priv->lvds_val;
+	else {
+		/* BIOS should set the proper LVDS register value at boot, but
+		 * in reality, it doesn't set the value when the lid is closed;
+		 * we need to check "the value to be set" in VBT when LVDS
+		 * register is uninitialized.
+		 */
+		val = I915_READ(lvds_reg);
+		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
+			val = dev_priv->bios_lvds_val;
+		dev_priv->lvds_val = val;
+	}
+	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
+}
+
 static bool intel_lvds_supported(struct drm_device *dev)
 {
 	/* With the introduction of the PCH we gained a dedicated
-- 
1.7.11.7

[PATCH 04/17] drm/i915: track is_dual_link in intel_lvds

[Daniel Vetter]

Yeah, all users (both the clock selection special cases and the lvds
pin pair stuff) are still in common code, but this will change.

v2: Rebase on top of Jani Nikula's panel rework.

v3: Incorporate review from Paulo Zanoni:
- s/__is_dual_link_lvds/compute_is_dual_link_lvds
- kill dev_priv->lvds_val
- drop spurious whitespace change

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h   |  1 -
 drivers/gpu/drm/i915/intel_lvds.c | 42 +++++++++++++++++++++++++++------------
 2 files changed, 29 insertions(+), 14 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 3229f04..2e92617 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -704,7 +704,6 @@ typedef struct drm_i915_private {
 	unsigned int display_clock_mode:1;
 	int lvds_ssc_freq;
 	unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
-	unsigned int lvds_val; /* used for checking LVDS channel mode */
 	struct {
 		int rate;
 		int lanes;
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 84255d0..1090f1b 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -52,6 +52,7 @@ struct intel_lvds_encoder {
 	u32 pfit_control;
 	u32 pfit_pgm_ratios;
 	bool pfit_dirty;
+	bool is_dual_link;
 
 	struct intel_lvds_connector *attached_connector;
 };
@@ -920,6 +921,23 @@ static const struct dmi_system_id intel_dual_link_lvds[] = {
 
 bool intel_is_dual_link_lvds(struct drm_device *dev)
 {
+	struct intel_encoder *encoder;
+	struct intel_lvds_encoder *lvds_encoder;
+
+	list_for_each_entry(encoder, &dev->mode_config.encoder_list,
+			    base.head) {
+		if (encoder->type == INTEL_OUTPUT_LVDS) {
+			lvds_encoder = to_lvds_encoder(&encoder->base);
+
+			return lvds_encoder->is_dual_link;
+		}
+	}
+
+	return false;
+}
+
+static bool compute_is_dual_link_lvds(struct drm_device *dev)
+{
 	unsigned int val;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	u32 lvds_reg;
@@ -937,19 +955,15 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
 	if (dmi_check_system(intel_dual_link_lvds))
 		return true;
 
-	if (dev_priv->lvds_val)
-		val = dev_priv->lvds_val;
-	else {
-		/* BIOS should set the proper LVDS register value at boot, but
-		 * in reality, it doesn't set the value when the lid is closed;
-		 * we need to check "the value to be set" in VBT when LVDS
-		 * register is uninitialized.
-		 */
-		val = I915_READ(lvds_reg);
-		if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
-			val = dev_priv->bios_lvds_val;
-		dev_priv->lvds_val = val;
-	}
+	/* BIOS should set the proper LVDS register value at boot, but
+	 * in reality, it doesn't set the value when the lid is closed;
+	 * we need to check "the value to be set" in VBT when LVDS
+	 * register is uninitialized.
+	 */
+	val = I915_READ(lvds_reg);
+	if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
+		val = dev_priv->bios_lvds_val;
+
 	return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
 }
 
@@ -1148,6 +1162,8 @@ bool intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
+	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(dev);
+
 	/*
 	 * Unlock registers and just
 	 * leave them unlocked
-- 
1.7.11.7

[PATCH 05/17] drm/i915: add intel_lvds->reg

[Daniel Vetter]

To ditch at least some of the PCH_SPLIT ? PCH_LVDS : LVDS code ...

v2: Rebase on top of Jani Nikula's panel rework.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_lvds.c | 48 ++++++++++++++++-----------------------
 1 file changed, 20 insertions(+), 28 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index 1090f1b..a6aec59 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -53,6 +53,7 @@ struct intel_lvds_encoder {
 	u32 pfit_pgm_ratios;
 	bool pfit_dirty;
 	bool is_dual_link;
+	u32 reg;
 
 	struct intel_lvds_connector *attached_connector;
 };
@@ -72,15 +73,10 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 {
 	struct drm_device *dev = encoder->base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 lvds_reg, tmp;
-
-	if (HAS_PCH_SPLIT(dev)) {
-		lvds_reg = PCH_LVDS;
-	} else {
-		lvds_reg = LVDS;
-	}
+	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
+	u32 tmp;
 
-	tmp = I915_READ(lvds_reg);
+	tmp = I915_READ(lvds_encoder->reg);
 
 	if (!(tmp & LVDS_PORT_EN))
 		return false;
@@ -102,19 +98,17 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
 	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 ctl_reg, lvds_reg, stat_reg;
+	u32 ctl_reg, stat_reg;
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ctl_reg = PCH_PP_CONTROL;
-		lvds_reg = PCH_LVDS;
 		stat_reg = PCH_PP_STATUS;
 	} else {
 		ctl_reg = PP_CONTROL;
-		lvds_reg = LVDS;
 		stat_reg = PP_STATUS;
 	}
 
-	I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN);
+	I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) | LVDS_PORT_EN);
 
 	if (lvds_encoder->pfit_dirty) {
 		/*
@@ -133,7 +127,7 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
 	}
 
 	I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
-	POSTING_READ(lvds_reg);
+	POSTING_READ(lvds_encoder->reg);
 	if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
 		DRM_ERROR("timed out waiting for panel to power on\n");
 
@@ -145,15 +139,13 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
 	struct drm_device *dev = encoder->base.dev;
 	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 ctl_reg, lvds_reg, stat_reg;
+	u32 ctl_reg, stat_reg;
 
 	if (HAS_PCH_SPLIT(dev)) {
 		ctl_reg = PCH_PP_CONTROL;
-		lvds_reg = PCH_LVDS;
 		stat_reg = PCH_PP_STATUS;
 	} else {
 		ctl_reg = PP_CONTROL;
-		lvds_reg = LVDS;
 		stat_reg = PP_STATUS;
 	}
 
@@ -168,8 +160,8 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
 		lvds_encoder->pfit_dirty = true;
 	}
 
-	I915_WRITE(lvds_reg, I915_READ(lvds_reg) & ~LVDS_PORT_EN);
-	POSTING_READ(lvds_reg);
+	I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) & ~LVDS_PORT_EN);
+	POSTING_READ(lvds_encoder->reg);
 }
 
 static int intel_lvds_mode_valid(struct drm_connector *connector,
@@ -936,17 +928,11 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
 	return false;
 }
 
-static bool compute_is_dual_link_lvds(struct drm_device *dev)
+static bool compute_is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
 {
+	struct drm_device *dev = lvds_encoder->base.base.dev;
 	unsigned int val;
 	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 lvds_reg;
-
-	if (HAS_PCH_SPLIT(dev)) {
-		lvds_reg = PCH_LVDS;
-	} else {
-		lvds_reg = LVDS;
-	}
 
 	/* use the module option value if specified */
 	if (i915_lvds_channel_mode > 0)
@@ -960,7 +946,7 @@ static bool compute_is_dual_link_lvds(struct drm_device *dev)
 	 * we need to check "the value to be set" in VBT when LVDS
 	 * register is uninitialized.
 	 */
-	val = I915_READ(lvds_reg);
+	val = I915_READ(lvds_encoder->reg);
 	if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
 		val = dev_priv->bios_lvds_val;
 
@@ -1073,6 +1059,12 @@ bool intel_lvds_init(struct drm_device *dev)
 	connector->interlace_allowed = false;
 	connector->doublescan_allowed = false;
 
+	if (HAS_PCH_SPLIT(dev)) {
+		lvds_encoder->reg = PCH_LVDS;
+	} else {
+		lvds_encoder->reg = LVDS;
+	}
+
 	/* create the scaling mode property */
 	drm_mode_create_scaling_mode_property(dev);
 	drm_connector_attach_property(&intel_connector->base,
@@ -1162,7 +1154,7 @@ bool intel_lvds_init(struct drm_device *dev)
 		goto failed;
 
 out:
-	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(dev);
+	lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
 
 	/*
 	 * Unlock registers and just
-- 
1.7.11.7

[PATCH 06/17] drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable

[Daniel Vetter]

A few things needed to change:
- HAS_PCH_SPLIT since ilk+ is not yet converted to this.
- s/LVDS/intel_lvds->reg/ to prep for ilk conversion
- replace the clock.p2 == 7 check with a is_dual_link check
- s/adjusted_mode/intel_lvds->fixed_mode

v2: Rebase on top of Jani Nikula's panel rework. I'm wondering whether
we shouldn't add an attached_panel pointer to intel_encoder, to
replace the encoder private ->attached_connector pointers, since
that's essentially what we need.

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 59 ------------------------------------
 drivers/gpu/drm/i915/intel_lvds.c    | 57 ++++++++++++++++++++++++++++++++++
 2 files changed, 57 insertions(+), 59 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1048595..1e88364 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4255,51 +4255,6 @@ static void i9xx_update_pll_dividers(struct drm_crtc *crtc,
 	}
 }
 
-static void intel_update_lvds(struct drm_crtc *crtc, intel_clock_t *clock,
-			      struct drm_display_mode *adjusted_mode)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
-	int pipe = intel_crtc->pipe;
-	u32 temp;
-
-	temp = I915_READ(LVDS);
-	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-	if (pipe == 1) {
-		temp |= LVDS_PIPEB_SELECT;
-	} else {
-		temp &= ~LVDS_PIPEB_SELECT;
-	}
-	/* set the corresponsding LVDS_BORDER bit */
-	temp |= dev_priv->lvds_border_bits;
-	/* Set the B0-B3 data pairs corresponding to whether we're going to
-	 * set the DPLLs for dual-channel mode or not.
-	 */
-	if (clock->p2 == 7)
-		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-	else
-		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
-
-	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
-	 * appropriately here, but we need to look more thoroughly into how
-	 * panels behave in the two modes.
-	 */
-	/* set the dithering flag on LVDS as needed */
-	if (INTEL_INFO(dev)->gen >= 4) {
-		if (dev_priv->lvds_dither)
-			temp |= LVDS_ENABLE_DITHER;
-		else
-			temp &= ~LVDS_ENABLE_DITHER;
-	}
-	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-	if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-		temp |= LVDS_HSYNC_POLARITY;
-	if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-		temp |= LVDS_VSYNC_POLARITY;
-	I915_WRITE(LVDS, temp);
-}
-
 static void vlv_update_pll(struct drm_crtc *crtc,
 			   struct drm_display_mode *mode,
 			   struct drm_display_mode *adjusted_mode,
@@ -4482,13 +4437,6 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
 		if (encoder->pre_pll_enable)
 			encoder->pre_pll_enable(encoder);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
-		intel_update_lvds(crtc, clock, adjusted_mode);
-
 	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_DISPLAYPORT))
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 
@@ -4564,13 +4512,6 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
 		if (encoder->pre_pll_enable)
 			encoder->pre_pll_enable(encoder);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS))
-		intel_update_lvds(crtc, clock, adjusted_mode);
-
 	I915_WRITE(DPLL(pipe), dpll);
 
 	/* Wait for the clocks to stabilize. */
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index a6aec59..e3a68a1 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -89,6 +89,62 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
 	return true;
 }
 
+/* The LVDS pin pair needs to be on before the DPLLs are enabled.
+ * This is an exception to the general rule that mode_set doesn't turn
+ * things on.
+ */
+static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
+{
+	struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
+	struct drm_device *dev = encoder->base.dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
+	struct drm_display_mode *fixed_mode =
+		lvds_encoder->attached_connector->base.panel.fixed_mode;
+	int pipe = intel_crtc->pipe;
+	u32 temp;
+
+	/* pch split platforms are not yet converted. */
+	if (HAS_PCH_SPLIT(dev))
+		return;
+
+	temp = I915_READ(lvds_encoder->reg);
+	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
+	if (pipe == 1) {
+		temp |= LVDS_PIPEB_SELECT;
+	} else {
+		temp &= ~LVDS_PIPEB_SELECT;
+	}
+	/* set the corresponsding LVDS_BORDER bit */
+	temp |= dev_priv->lvds_border_bits;
+	/* Set the B0-B3 data pairs corresponding to whether we're going to
+	 * set the DPLLs for dual-channel mode or not.
+	 */
+	if (lvds_encoder->is_dual_link)
+		temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
+	else
+		temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
+
+	/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
+	 * appropriately here, but we need to look more thoroughly into how
+	 * panels behave in the two modes.
+	 */
+	/* set the dithering flag on LVDS as needed */
+	if (INTEL_INFO(dev)->gen >= 4) {
+		if (dev_priv->lvds_dither)
+			temp |= LVDS_ENABLE_DITHER;
+		else
+			temp &= ~LVDS_ENABLE_DITHER;
+	}
+	temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
+	if (fixed_mode->flags & DRM_MODE_FLAG_NHSYNC)
+		temp |= LVDS_HSYNC_POLARITY;
+	if (fixed_mode->flags & DRM_MODE_FLAG_NVSYNC)
+		temp |= LVDS_VSYNC_POLARITY;
+
+	I915_WRITE(lvds_encoder->reg, temp);
+}
+
 /**
  * Sets the power state for the panel.
  */
@@ -1038,6 +1094,7 @@ bool intel_lvds_init(struct drm_device *dev)
 			 DRM_MODE_ENCODER_LVDS);
 
 	intel_encoder->enable = intel_enable_lvds;
+	intel_encoder->pre_pll_enable = intel_pre_pll_enable_lvds;
 	intel_encoder->disable = intel_disable_lvds;
 	intel_encoder->get_hw_state = intel_lvds_get_hw_state;
 	intel_connector->get_hw_state = intel_connector_get_hw_state;
-- 
1.7.11.7

[PATCH 07/17] drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too

[Daniel Vetter]

Only two things needed adjustment:
- pipe select for PCH_CPT
- There's no dithering bit on ilk+ in the lvds ctl reg

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 40 ------------------------------------
 drivers/gpu/drm/i915/intel_lvds.c    | 24 ++++++++++++++--------
 2 files changed, 15 insertions(+), 49 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1e88364..8f6362e 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5320,7 +5320,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	bool ok, has_reduced_clock = false;
 	bool is_lvds = false, is_dp = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
-	u32 temp;
 	int ret;
 	bool dither, fdi_config_ok;
 
@@ -5384,45 +5383,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	} else
 		intel_put_pch_pll(intel_crtc);
 
-	/* The LVDS pin pair needs to be on before the DPLLs are enabled.
-	 * This is an exception to the general rule that mode_set doesn't turn
-	 * things on.
-	 */
-	if (is_lvds) {
-		temp = I915_READ(PCH_LVDS);
-		temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-		if (HAS_PCH_CPT(dev)) {
-			temp &= ~PORT_TRANS_SEL_MASK;
-			temp |= PORT_TRANS_SEL_CPT(pipe);
-		} else {
-			if (pipe == 1)
-				temp |= LVDS_PIPEB_SELECT;
-			else
-				temp &= ~LVDS_PIPEB_SELECT;
-		}
-
-		/* set the corresponsding LVDS_BORDER bit */
-		temp |= dev_priv->lvds_border_bits;
-		/* Set the B0-B3 data pairs corresponding to whether we're going to
-		 * set the DPLLs for dual-channel mode or not.
-		 */
-		if (clock.p2 == 7)
-			temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-		else
-			temp &= ~(LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP);
-
-		/* It would be nice to set 24 vs 18-bit mode (LVDS_A3_POWER_UP)
-		 * appropriately here, but we need to look more thoroughly into how
-		 * panels behave in the two modes.
-		 */
-		temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-			temp |= LVDS_HSYNC_POLARITY;
-		if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-			temp |= LVDS_VSYNC_POLARITY;
-		I915_WRITE(PCH_LVDS, temp);
-	}
-
 	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
 	} else {
diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
index e3a68a1..59e31aa 100644
--- a/drivers/gpu/drm/i915/intel_lvds.c
+++ b/drivers/gpu/drm/i915/intel_lvds.c
@@ -104,17 +104,20 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	int pipe = intel_crtc->pipe;
 	u32 temp;
 
-	/* pch split platforms are not yet converted. */
-	if (HAS_PCH_SPLIT(dev))
-		return;
-
 	temp = I915_READ(lvds_encoder->reg);
 	temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-	if (pipe == 1) {
-		temp |= LVDS_PIPEB_SELECT;
+
+	if (HAS_PCH_CPT(dev)) {
+		temp &= ~PORT_TRANS_SEL_MASK;
+		temp |= PORT_TRANS_SEL_CPT(pipe);
 	} else {
-		temp &= ~LVDS_PIPEB_SELECT;
+		if (pipe == 1) {
+			temp |= LVDS_PIPEB_SELECT;
+		} else {
+			temp &= ~LVDS_PIPEB_SELECT;
+		}
 	}
+
 	/* set the corresponsding LVDS_BORDER bit */
 	temp |= dev_priv->lvds_border_bits;
 	/* Set the B0-B3 data pairs corresponding to whether we're going to
@@ -129,8 +132,11 @@ static void intel_pre_pll_enable_lvds(struct intel_encoder *encoder)
 	 * appropriately here, but we need to look more thoroughly into how
 	 * panels behave in the two modes.
 	 */
-	/* set the dithering flag on LVDS as needed */
-	if (INTEL_INFO(dev)->gen >= 4) {
+
+	/* Set the dithering flag on LVDS as needed, note that there is no
+	 * 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)
 			temp |= LVDS_ENABLE_DITHER;
 		else
-- 
1.7.11.7

[PATCH 08/17] drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

[Daniel Vetter]

Especially getting rid of all things lvds is ... great!

v2: Drop the two additional pre-hsw hunks noticed by Paulo Zanoni.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 8f6362e..40f63b0 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5341,9 +5341,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		num_connectors++;
 	}
 
-	WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
-	     "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
-
 	ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
 				     &has_reduced_clock, &reduced_clock);
 	if (!ok) {
@@ -5462,23 +5459,14 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	int pipe = intel_crtc->pipe;
 	int plane = intel_crtc->plane;
 	int num_connectors = 0;
-	intel_clock_t clock, reduced_clock;
-	u32 dpll = 0, fp = 0, fp2 = 0;
-	bool ok, has_reduced_clock = false;
-	bool is_lvds = false, is_dp = false, is_cpu_edp = false;
+	bool is_dp = false, is_cpu_edp = false;
 	struct intel_encoder *encoder;
-	u32 temp;
 	int ret;
 	bool dither;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
-		case INTEL_OUTPUT_LVDS:
-			is_lvds = true;
-			break;
 		case INTEL_OUTPUT_DISPLAYPORT:
-			is_dp = true;
-			break;
 		case INTEL_OUTPUT_EDP:
 			is_dp = true;
 			if (!intel_encoder_is_pch_edp(&encoder->base))
@@ -5509,133 +5497,21 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
 		return -EINVAL;
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
-					     &has_reduced_clock,
-					     &reduced_clock);
-		if (!ok) {
-			DRM_ERROR("Couldn't find PLL settings for mode!\n");
-			return -EINVAL;
-		}
-	}
-
 	/* Ensure that the cursor is valid for the new mode before changing... */
 	intel_crtc_update_cursor(crtc, true);
 
 	/* determine panel color depth */
 	dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
 					      adjusted_mode);
-	if (is_lvds && dev_priv->lvds_dither)
-		dither = true;
 
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		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, adjusted_mode, &clock,
-					     fp);
-
-		/* CPU eDP is the only output that doesn't need a PCH PLL of its
-		 * own on pre-Haswell/LPT generation */
-		if (!is_cpu_edp) {
-			struct intel_pch_pll *pll;
-
-			pll = intel_get_pch_pll(intel_crtc, dpll, fp);
-			if (pll == NULL) {
-				DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
-						 pipe);
-				return -EINVAL;
-			}
-		} else
-			intel_put_pch_pll(intel_crtc);
-
-		/* The LVDS pin pair needs to be on before the DPLLs are
-		 * enabled.  This is an exception to the general rule that
-		 * mode_set doesn't turn things on.
-		 */
-		if (is_lvds) {
-			temp = I915_READ(PCH_LVDS);
-			temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
-			if (HAS_PCH_CPT(dev)) {
-				temp &= ~PORT_TRANS_SEL_MASK;
-				temp |= PORT_TRANS_SEL_CPT(pipe);
-			} else {
-				if (pipe == 1)
-					temp |= LVDS_PIPEB_SELECT;
-				else
-					temp &= ~LVDS_PIPEB_SELECT;
-			}
-
-			/* set the corresponsding LVDS_BORDER bit */
-			temp |= dev_priv->lvds_border_bits;
-			/* Set the B0-B3 data pairs corresponding to whether
-			 * we're going to set the DPLLs for dual-channel mode or
-			 * not.
-			 */
-			if (clock.p2 == 7)
-				temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
-			else
-				temp &= ~(LVDS_B0B3_POWER_UP |
-					  LVDS_CLKB_POWER_UP);
-
-			/* It would be nice to set 24 vs 18-bit mode
-			 * (LVDS_A3_POWER_UP) appropriately here, but we need to
-			 * look more thoroughly into how panels behave in the
-			 * two modes.
-			 */
-			temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
-			if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
-				temp |= LVDS_HSYNC_POLARITY;
-			if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
-				temp |= LVDS_VSYNC_POLARITY;
-			I915_WRITE(PCH_LVDS, temp);
-		}
-	}
-
 	if (is_dp && !is_cpu_edp) {
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
-	} else {
-		if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-			/* For non-DP output, clear any trans DP clock recovery
-			 * setting.*/
-			I915_WRITE(TRANSDATA_M1(pipe), 0);
-			I915_WRITE(TRANSDATA_N1(pipe), 0);
-			I915_WRITE(TRANSDPLINK_M1(pipe), 0);
-			I915_WRITE(TRANSDPLINK_N1(pipe), 0);
-		}
 	}
 
 	intel_crtc->lowfreq_avail = false;
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
-		if (intel_crtc->pch_pll) {
-			I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-
-			/* Wait for the clocks to stabilize. */
-			POSTING_READ(intel_crtc->pch_pll->pll_reg);
-			udelay(150);
-
-			/* The pixel multiplier can only be updated once the
-			 * DPLL is enabled and the clocks are stable.
-			 *
-			 * So write it again.
-			 */
-			I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
-		}
-
-		if (intel_crtc->pch_pll) {
-			if (is_lvds && has_reduced_clock && i915_powersave) {
-				I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
-				intel_crtc->lowfreq_avail = true;
-			} else {
-				I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
-			}
-		}
-	}
 
 	intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
 
-- 
1.7.11.7

[PATCH 09/17] drm/i915: move set_pll_edp to intel_dp.c

[Daniel Vetter]

Now that we enable the cpu edp pll in intel_dp->pre_enable and no
longer in crtc_mode_set, we can also move the modeset part to the
intel_dp->mode_set callback. Previously this was not possible because
the encoder ->mode_set callbacks are called after the crtc mode set
callback.

v2: Rebase on top of copy&pasted hsw crtc_mode_set.

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_display.c | 44 ------------------------------------
 drivers/gpu/drm/i915/intel_dp.c      | 40 ++++++++++++++++++++++++++++++++
 2 files changed, 40 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 40f63b0..7be2532 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2296,43 +2296,6 @@ intel_pipe_set_base(struct drm_crtc *crtc, int x, int y,
 	return 0;
 }
 
-static void ironlake_set_pll_edp(struct drm_crtc *crtc, int clock)
-{
-	struct drm_device *dev = crtc->dev;
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 dpa_ctl;
-
-	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", clock);
-	dpa_ctl = I915_READ(DP_A);
-	dpa_ctl &= ~DP_PLL_FREQ_MASK;
-
-	if (clock < 200000) {
-		u32 temp;
-		dpa_ctl |= DP_PLL_FREQ_160MHZ;
-		/* workaround for 160Mhz:
-		   1) program 0x4600c bits 15:0 = 0x8124
-		   2) program 0x46010 bit 0 = 1
-		   3) program 0x46034 bit 24 = 1
-		   4) program 0x64000 bit 14 = 1
-		   */
-		temp = I915_READ(0x4600c);
-		temp &= 0xffff0000;
-		I915_WRITE(0x4600c, temp | 0x8124);
-
-		temp = I915_READ(0x46010);
-		I915_WRITE(0x46010, temp | 1);
-
-		temp = I915_READ(0x46034);
-		I915_WRITE(0x46034, temp | (1 << 24));
-	} else {
-		dpa_ctl |= DP_PLL_FREQ_270MHZ;
-	}
-	I915_WRITE(DP_A, dpa_ctl);
-
-	POSTING_READ(DP_A);
-	udelay(500);
-}
-
 static void intel_fdi_normal_train(struct drm_crtc *crtc)
 {
 	struct drm_device *dev = crtc->dev;
@@ -5427,9 +5390,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	fdi_config_ok = ironlake_check_fdi_lanes(intel_crtc);
 
-	if (is_cpu_edp)
-		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
-
 	ironlake_set_pipeconf(crtc, adjusted_mode, dither);
 
 	intel_wait_for_vblank(dev, pipe);
@@ -5518,10 +5478,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	if (!is_dp || is_cpu_edp)
 		ironlake_set_m_n(crtc, mode, adjusted_mode);
 
-	if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev))
-		if (is_cpu_edp)
-			ironlake_set_pll_edp(crtc, adjusted_mode->clock);
-
 	haswell_set_pipeconf(crtc, adjusted_mode, dither);
 
 	/* Set up the display plane register */
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 34ac746..e5f496b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -851,6 +851,43 @@ void intel_dp_init_link_config(struct intel_dp *intel_dp)
 	}
 }
 
+static void ironlake_set_pll_edp(struct drm_crtc *crtc, int clock)
+{
+	struct drm_device *dev = crtc->dev;
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 dpa_ctl;
+
+	DRM_DEBUG_KMS("eDP PLL enable for clock %d\n", clock);
+	dpa_ctl = I915_READ(DP_A);
+	dpa_ctl &= ~DP_PLL_FREQ_MASK;
+
+	if (clock < 200000) {
+		u32 temp;
+		dpa_ctl |= DP_PLL_FREQ_160MHZ;
+		/* workaround for 160Mhz:
+		   1) program 0x4600c bits 15:0 = 0x8124
+		   2) program 0x46010 bit 0 = 1
+		   3) program 0x46034 bit 24 = 1
+		   4) program 0x64000 bit 14 = 1
+		   */
+		temp = I915_READ(0x4600c);
+		temp &= 0xffff0000;
+		I915_WRITE(0x4600c, temp | 0x8124);
+
+		temp = I915_READ(0x46010);
+		I915_WRITE(0x46010, temp | 1);
+
+		temp = I915_READ(0x46034);
+		I915_WRITE(0x46034, temp | (1 << 24));
+	} else {
+		dpa_ctl |= DP_PLL_FREQ_270MHZ;
+	}
+	I915_WRITE(DP_A, dpa_ctl);
+
+	POSTING_READ(DP_A);
+	udelay(500);
+}
+
 static void
 intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 		  struct drm_display_mode *adjusted_mode)
@@ -950,6 +987,9 @@ intel_dp_mode_set(struct drm_encoder *encoder, struct drm_display_mode *mode,
 	} else {
 		intel_dp->DP |= DP_LINK_TRAIN_OFF_CPT;
 	}
+
+	if (is_cpu_edp(intel_dp))
+		ironlake_set_pll_edp(crtc, adjusted_mode->clock);
 }
 
 #define IDLE_ON_MASK		(PP_ON | 0 	  | PP_SEQUENCE_MASK | 0                     | PP_SEQUENCE_STATE_MASK)
-- 
1.7.11.7

[PATCH 10/17] drm/i915: rip out pre-production ilk cpu edp w/a

[Daniel Vetter]

While reading docs I've noticed that this special workaround to select
the 1.6 GHz DP clock only applies to pre-production ilk machines.
Since the registers we're touching here are rather undocumented and
might be harmful on later chips, rip it out.

For the Bspec reference of this w/a look in "vol4g CPU Display
Registers [DevILK]", Section 4.1.7.1 "DP_A—DisplayPort A
Control Register", "DP_PLL_Frequency_Select".

v2: Keep a debug message as a hint in case something regresses.
Requested by Chris Wilson.

Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/intel_dp.c | 21 +++++----------------
 1 file changed, 5 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e5f496b..b2aa666 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -862,26 +862,15 @@ static void ironlake_set_pll_edp(struct drm_crtc *crtc, int clock)
 	dpa_ctl &= ~DP_PLL_FREQ_MASK;
 
 	if (clock < 200000) {
-		u32 temp;
+		/* For a long time we've carried around a ILK-DevA w/a for the
+		 * 160MHz clock. If we're really unlucky, it's still required.
+		 */
+		DRM_DEBUG_KMS("160MHz cpu eDP clock, might need ilk devA w/a\n");
 		dpa_ctl |= DP_PLL_FREQ_160MHZ;
-		/* workaround for 160Mhz:
-		   1) program 0x4600c bits 15:0 = 0x8124
-		   2) program 0x46010 bit 0 = 1
-		   3) program 0x46034 bit 24 = 1
-		   4) program 0x64000 bit 14 = 1
-		   */
-		temp = I915_READ(0x4600c);
-		temp &= 0xffff0000;
-		I915_WRITE(0x4600c, temp | 0x8124);
-
-		temp = I915_READ(0x46010);
-		I915_WRITE(0x46010, temp | 1);
-
-		temp = I915_READ(0x46034);
-		I915_WRITE(0x46034, temp | (1 << 24));
 	} else {
 		dpa_ctl |= DP_PLL_FREQ_270MHZ;
 	}
+
 	I915_WRITE(DP_A, dpa_ctl);
 
 	POSTING_READ(DP_A);
-- 
1.7.11.7

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

[PATCH 11/17] drm/i915: use wait_for_vblank instead of msleep(17)

[Daniel Vetter]

17 ms is eerily close to 60 Hz ^-1

Unfortunately this goes back to the original DP enabling for ilk, and
unfortunately does not come with a reason for it's existance attached.

Some closer inspection of the code and DP specs shows that we set the
idle link pattern before we disable the port. And it seems like that
the DP spec (or at least our hw) only switch to the idle pattern on
the next vblank. Hence a vblank wait at this spot makes _much_ more
sense than a really long wait.

v2: Rebase fixup.

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b2aa666..b782094 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1988,6 +1988,8 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
+	struct intel_crtc *intel_crtc =
+		to_intel_crtc(intel_dig_port->base.base.crtc);
 	uint32_t DP = intel_dp->DP;
 
 	/*
@@ -2022,7 +2024,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 	}
 	POSTING_READ(intel_dp->output_reg);
 
-	msleep(17);
+	intel_wait_for_vblank(dev, intel_crtc->pipe);
 
 	if (HAS_PCH_IBX(dev) &&
 	    I915_READ(intel_dp->output_reg) & DP_PIPEB_SELECT) {
@@ -2054,7 +2056,7 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 			POSTING_READ(intel_dp->output_reg);
 			msleep(50);
 		} else
-			intel_wait_for_vblank(dev, to_intel_crtc(crtc)->pipe);
+			intel_wait_for_vblank(dev, intel_crtc->pipe);
 	}
 
 	DP &= ~DP_AUDIO_OUTPUT_ENABLE;
-- 
1.7.11.7

[PATCH 12/17] drm/i915: WARN on !crtc in intel_dp_link_down

[Daniel Vetter]

This could have happened with the old crtc helper based modeset code,
but can't happen any longer with the new code.

Hence put in a WARN and adjust the comment. If no one hits this, we
can eventually remove it (like a few other such cases across our
code).

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

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b782094..8ec4d1c 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2044,15 +2044,10 @@ intel_dp_link_down(struct intel_dp *intel_dp)
 		/* Changes to enable or select take place the vblank
 		 * after being written.
 		 */
-		if (crtc == NULL) {
-			/* We can arrive here never having been attached
-			 * to a CRTC, for instance, due to inheriting
-			 * random state from the BIOS.
-			 *
-			 * If the pipe is not running, play safe and
-			 * wait for the clocks to stabilise before
-			 * continuing.
-			 */
+		if (WARN_ON(crtc == NULL)) {
+			/* We should never try to disable a port without a crtc
+			 * attached. For paranoia keep the code around for a
+			 * bit. */
 			POSTING_READ(intel_dp->output_reg);
 			msleep(50);
 		} else
-- 
1.7.11.7

[PATCH 13/17] drm/i915: drop unnecessary clearing of pch dp transcoder timings

[Daniel Vetter]

This has originally been added in

commit 8db9d77b1b14fd730561f64beea8c00e4478d7c5
Author: Zhenyu Wang <zhenyuw@linux.intel.com>
Date:   Wed Apr 7 16:15:54 2010 +0800

    drm/i915: Support for Cougarpoint PCH display pipeline

probably to combat issues with hw state left behind by the BIOS. And
indeed, I've checked out that specific revision, and there is no DP
support yet. So the pch dp transcoder won't be correctly disabled, and
that's important since it requires a rether special disable dance:
Just writing 0 to TRANS_DP_CTL won't cut it, since we need to select
the NONE port when disabling, too.

And indeed, things seem to still work, so let's just remove this.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7be2532..9cfc3cd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5343,15 +5343,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	} else
 		intel_put_pch_pll(intel_crtc);
 
-	if (is_dp && !is_cpu_edp) {
+	if (is_dp && !is_cpu_edp)
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
-	} else {
-		/* For non-DP output, clear any trans DP clock recovery setting.*/
-		I915_WRITE(TRANSDATA_M1(pipe), 0);
-		I915_WRITE(TRANSDATA_N1(pipe), 0);
-		I915_WRITE(TRANSDPLINK_M1(pipe), 0);
-		I915_WRITE(TRANSDPLINK_N1(pipe), 0);
-	}
 
 	for_each_encoder_on_crtc(dev, crtc, encoder)
 		if (encoder->pre_pll_enable)
@@ -5467,9 +5460,8 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 	DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
 	drm_mode_debug_printmodeline(mode);
 
-	if (is_dp && !is_cpu_edp) {
+	if (is_dp && !is_cpu_edp)
 		intel_dp_set_m_n(crtc, mode, adjusted_mode);
-	}
 
 	intel_crtc->lowfreq_avail = false;
 
-- 
1.7.11.7

[PATCH 14/17] drm/i915: extract common link_m_n helpers

[Daniel Vetter]

Both the dp and fdi code use the exact same computations (ignore minor
differences in conversion between bits and bytes).

This makes it even more apparent that we have a _massive_ mess between
cpu transcoder/fdi link/pch transcoder and pch link settings. And also
that we have hilarious amounts of confusion between edp and dp
(despite that they're identical at a link level).

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      | 13 ++++++++++++
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++-------------------
 drivers/gpu/drm/i915/intel_dp.c      | 39 +++---------------------------------
 3 files changed, 26 insertions(+), 57 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 2e92617..39bdc14 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -101,6 +101,19 @@ struct intel_pch_pll {
 };
 #define I915_NUM_PLLS 2
 
+/* Used by dp and fdi links */
+struct intel_link_m_n {
+	uint32_t	tu;
+	uint32_t	gmch_m;
+	uint32_t	gmch_n;
+	uint32_t	link_m;
+	uint32_t	link_n;
+};
+
+void intel_link_compute_m_n(int bpp, int nlanes,
+			    int pixel_clock, int link_clock,
+			    struct intel_link_m_n *m_n);
+
 struct intel_ddi_plls {
 	int spll_refcount;
 	int wrpll1_refcount;
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 9cfc3cd..1ab4592 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3952,16 +3952,8 @@ static int i830_get_display_clock_speed(struct drm_device *dev)
 	return 133000;
 }
 
-struct fdi_m_n {
-	u32        tu;
-	u32        gmch_m;
-	u32        gmch_n;
-	u32        link_m;
-	u32        link_n;
-};
-
 static void
-fdi_reduce_ratio(u32 *num, u32 *den)
+intel_reduce_ratio(uint32_t *num, uint32_t *den)
 {
 	while (*num > 0xffffff || *den > 0xffffff) {
 		*num >>= 1;
@@ -3969,20 +3961,18 @@ fdi_reduce_ratio(u32 *num, u32 *den)
 	}
 }
 
-static void
-ironlake_compute_m_n(int bits_per_pixel, int nlanes, int pixel_clock,
-		     int link_clock, struct fdi_m_n *m_n)
+void
+intel_link_compute_m_n(int bits_per_pixel, int nlanes,
+		       int pixel_clock, int link_clock,
+		       struct intel_link_m_n *m_n)
 {
-	m_n->tu = 64; /* default size */
-
-	/* BUG_ON(pixel_clock > INT_MAX / 36); */
+	m_n->tu = 64;
 	m_n->gmch_m = bits_per_pixel * pixel_clock;
 	m_n->gmch_n = link_clock * nlanes * 8;
-	fdi_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n);
-
+	intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n);
 	m_n->link_m = pixel_clock;
 	m_n->link_n = link_clock;
-	fdi_reduce_ratio(&m_n->link_m, &m_n->link_n);
+	intel_reduce_ratio(&m_n->link_m, &m_n->link_n);
 }
 
 static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
@@ -5096,7 +5086,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
 	struct intel_encoder *intel_encoder, *edp_encoder = NULL;
-	struct fdi_m_n m_n = {0};
+	struct intel_link_m_n m_n = {0};
 	int target_clock, pixel_multiplier, lane, link_bw;
 	bool is_dp = false, is_cpu_edp = false;
 
@@ -5154,8 +5144,7 @@ static void ironlake_set_m_n(struct drm_crtc *crtc,
 
 	if (pixel_multiplier > 1)
 		link_bw *= pixel_multiplier;
-	ironlake_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw,
-			     &m_n);
+	intel_link_compute_m_n(intel_crtc->bpp, lane, target_clock, link_bw, &m_n);
 
 	I915_WRITE(PIPE_DATA_M1(cpu_transcoder), TU_SIZE(m_n.tu) | m_n.gmch_m);
 	I915_WRITE(PIPE_DATA_N1(cpu_transcoder), m_n.gmch_n);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8ec4d1c..5d8ae65 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -742,39 +742,6 @@ intel_dp_mode_fixup(struct drm_encoder *encoder,
 	return false;
 }
 
-struct intel_dp_m_n {
-	uint32_t	tu;
-	uint32_t	gmch_m;
-	uint32_t	gmch_n;
-	uint32_t	link_m;
-	uint32_t	link_n;
-};
-
-static void
-intel_reduce_ratio(uint32_t *num, uint32_t *den)
-{
-	while (*num > 0xffffff || *den > 0xffffff) {
-		*num >>= 1;
-		*den >>= 1;
-	}
-}
-
-static void
-intel_dp_compute_m_n(int bpp,
-		     int nlanes,
-		     int pixel_clock,
-		     int link_clock,
-		     struct intel_dp_m_n *m_n)
-{
-	m_n->tu = 64;
-	m_n->gmch_m = (pixel_clock * bpp) >> 3;
-	m_n->gmch_n = link_clock * nlanes;
-	intel_reduce_ratio(&m_n->gmch_m, &m_n->gmch_n);
-	m_n->link_m = pixel_clock;
-	m_n->link_n = link_clock;
-	intel_reduce_ratio(&m_n->link_m, &m_n->link_n);
-}
-
 void
 intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 		 struct drm_display_mode *adjusted_mode)
@@ -785,7 +752,7 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
 	int lane_count = 4;
-	struct intel_dp_m_n m_n;
+	struct intel_link_m_n m_n;
 	int pipe = intel_crtc->pipe;
 	enum transcoder cpu_transcoder = intel_crtc->cpu_transcoder;
 
@@ -808,8 +775,8 @@ intel_dp_set_m_n(struct drm_crtc *crtc, struct drm_display_mode *mode,
 	 * the number of bytes_per_pixel post-LUT, which we always
 	 * set up for 8-bits of R/G/B, or 3 bytes total.
 	 */
-	intel_dp_compute_m_n(intel_crtc->bpp, lane_count,
-			     mode->clock, adjusted_mode->clock, &m_n);
+	intel_link_compute_m_n(intel_crtc->bpp, lane_count,
+			       mode->clock, adjusted_mode->clock, &m_n);
 
 	if (IS_HASWELL(dev)) {
 		I915_WRITE(PIPE_DATA_M1(cpu_transcoder),
-- 
1.7.11.7

[PATCH 15/17] drm/i915: don't call update_watermark in crtc_mode_set

[Daniel Vetter]

We already update watermarks at all the right places in the
crtc_enable/disable callbacks. And since ->mode_set doesn't change the
active state, nothing will have changed ...

v2: Rebase on top of latest haswell copy&pasta.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 1ab4592..bd6b409 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4711,8 +4711,6 @@ static int i9xx_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
-
 	return ret;
 }
 
@@ -5382,8 +5380,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
-
 	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
 
 	return fdi_config_ok ? ret : -EINVAL;
@@ -5467,8 +5463,6 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_watermarks(dev);
-
 	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
 
 	return ret;
-- 
1.7.11.7

[PATCH 16/17] drm/i915: don't call update_watermarks from haswell enable/disable code

[Daniel Vetter]

We don't use these on haswell, since haswell has per-pipe watermarks
and nothing global any more.

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

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index bd6b409..b3e25d4 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3356,7 +3356,6 @@ static void haswell_crtc_enable(struct drm_crtc *crtc)
 		return;
 
 	intel_crtc->active = true;
-	intel_update_watermarks(dev);
 
 	is_pch_port = haswell_crtc_driving_pch(crtc);
 
@@ -3544,7 +3543,6 @@ static void haswell_crtc_disable(struct drm_crtc *crtc)
 	}
 
 	intel_crtc->active = false;
-	intel_update_watermarks(dev);
 
 	mutex_lock(&dev->struct_mutex);
 	intel_update_fbc(dev);
-- 
1.7.11.7

[PATCH 17/17] drm/i915: rip out update_linetime_wm abstraction

[Daniel Vetter]

I like abstraction and vfuncs, but only if they actually abstract anything.
In this case here they just obfuscate, so let's rip this stuff out.

Aside: We really should move all the haswell stuff into it's own file ...

Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
---
 drivers/gpu/drm/i915/i915_drv.h      |  2 --
 drivers/gpu/drm/i915/intel_display.c | 31 +++++++++++++++++++++++++++---
 drivers/gpu/drm/i915/intel_drv.h     |  2 --
 drivers/gpu/drm/i915/intel_pm.c      | 37 ------------------------------------
 4 files changed, 28 insertions(+), 44 deletions(-)

diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
index 39bdc14..7389aa1 100644
--- a/drivers/gpu/drm/i915/i915_drv.h
+++ b/drivers/gpu/drm/i915/i915_drv.h
@@ -268,8 +268,6 @@ struct drm_i915_display_funcs {
 	void (*update_wm)(struct drm_device *dev);
 	void (*update_sprite_wm)(struct drm_device *dev, int pipe,
 				 uint32_t sprite_width, int pixel_size);
-	void (*update_linetime_wm)(struct drm_device *dev, int pipe,
-				 struct drm_display_mode *mode);
 	void (*modeset_global_resources)(struct drm_device *dev);
 	int (*crtc_mode_set)(struct drm_crtc *crtc,
 			     struct drm_display_mode *mode,
diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b3e25d4..021e813 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5378,11 +5378,36 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
-
 	return fdi_config_ok ? ret : -EINVAL;
 }
 
+static void
+haswell_update_linetime_wm(struct drm_device *dev, int pipe,
+				 struct drm_display_mode *mode)
+{
+	struct drm_i915_private *dev_priv = dev->dev_private;
+	u32 temp;
+
+	temp = I915_READ(PIPE_WM_LINETIME(pipe));
+	temp &= ~PIPE_WM_LINETIME_MASK;
+
+	/* The WM are computed with base on how long it takes to fill a single
+	 * row at the given clock rate, multiplied by 8.
+	 * */
+	temp |= PIPE_WM_LINETIME_TIME(
+		((mode->crtc_hdisplay * 1000) / mode->clock) * 8);
+
+	/* IPS watermarks are only used by pipe A, and are ignored by
+	 * pipes B and C.  They are calculated similarly to the common
+	 * linetime values, except that we are using CD clock frequency
+	 * in MHz instead of pixel rate for the division.
+	 *
+	 * This is a placeholder for the IPS watermark calculation code.
+	 */
+
+	I915_WRITE(PIPE_WM_LINETIME(pipe), temp);
+}
+
 static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 				 struct drm_display_mode *mode,
 				 struct drm_display_mode *adjusted_mode,
@@ -5461,7 +5486,7 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
 
 	ret = intel_pipe_set_base(crtc, x, y, fb);
 
-	intel_update_linetime_watermarks(dev, pipe, adjusted_mode);
+	haswell_update_linetime_wm(dev, pipe, adjusted_mode);
 
 	return ret;
 }
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 3614fbb..8a63d77 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -620,8 +620,6 @@ extern void intel_update_watermarks(struct drm_device *dev);
 extern void intel_update_sprite_watermarks(struct drm_device *dev, int pipe,
 					   uint32_t sprite_width,
 					   int pixel_size);
-extern void intel_update_linetime_watermarks(struct drm_device *dev, int pipe,
-			 struct drm_display_mode *mode);
 
 extern unsigned long intel_gen4_compute_offset_xtiled(int *x, int *y,
 						      unsigned int bpp,
diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 0edb549..0e6585b 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1890,33 +1890,6 @@ static void sandybridge_update_wm(struct drm_device *dev)
 		   cursor_wm);
 }
 
-static void
-haswell_update_linetime_wm(struct drm_device *dev, int pipe,
-				 struct drm_display_mode *mode)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-	u32 temp;
-
-	temp = I915_READ(PIPE_WM_LINETIME(pipe));
-	temp &= ~PIPE_WM_LINETIME_MASK;
-
-	/* The WM are computed with base on how long it takes to fill a single
-	 * row at the given clock rate, multiplied by 8.
-	 * */
-	temp |= PIPE_WM_LINETIME_TIME(
-		((mode->crtc_hdisplay * 1000) / mode->clock) * 8);
-
-	/* IPS watermarks are only used by pipe A, and are ignored by
-	 * pipes B and C.  They are calculated similarly to the common
-	 * linetime values, except that we are using CD clock frequency
-	 * in MHz instead of pixel rate for the division.
-	 *
-	 * This is a placeholder for the IPS watermark calculation code.
-	 */
-
-	I915_WRITE(PIPE_WM_LINETIME(pipe), temp);
-}
-
 static bool
 sandybridge_compute_sprite_wm(struct drm_device *dev, int plane,
 			      uint32_t sprite_width, int pixel_size,
@@ -2112,15 +2085,6 @@ void intel_update_watermarks(struct drm_device *dev)
 		dev_priv->display.update_wm(dev);
 }
 
-void intel_update_linetime_watermarks(struct drm_device *dev,
-		int pipe, struct drm_display_mode *mode)
-{
-	struct drm_i915_private *dev_priv = dev->dev_private;
-
-	if (dev_priv->display.update_linetime_wm)
-		dev_priv->display.update_linetime_wm(dev, pipe, mode);
-}
-
 void intel_update_sprite_watermarks(struct drm_device *dev, int pipe,
 				    uint32_t sprite_width, int pixel_size)
 {
@@ -3983,7 +3947,6 @@ void intel_init_pm(struct drm_device *dev)
 			if (SNB_READ_WM0_LATENCY()) {
 				dev_priv->display.update_wm = sandybridge_update_wm;
 				dev_priv->display.update_sprite_wm = sandybridge_update_sprite_wm;
-				dev_priv->display.update_linetime_wm = haswell_update_linetime_wm;
 			} else {
 				DRM_DEBUG_KMS("Failed to read display plane latency. "
 					      "Disable CxSR\n");
-- 
1.7.11.7

Re: [PATCH 05/17] drm/i915: add intel_lvds->reg

[Paulo Zanoni]

Hi

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> To ditch at least some of the PCH_SPLIT ? PCH_LVDS : LVDS code ...
>
> v2: Rebase on top of Jani Nikula's panel rework.
>
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_lvds.c | 48 ++++++++++++++++-----------------------
>  1 file changed, 20 insertions(+), 28 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 1090f1b..a6aec59 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -53,6 +53,7 @@ struct intel_lvds_encoder {
>         u32 pfit_pgm_ratios;
>         bool pfit_dirty;
>         bool is_dual_link;
> +       u32 reg;
>
>         struct intel_lvds_connector *attached_connector;
>  };
> @@ -72,15 +73,10 @@ static bool intel_lvds_get_hw_state(struct intel_encoder *encoder,
>  {
>         struct drm_device *dev = encoder->base.dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 lvds_reg, tmp;
> -
> -       if (HAS_PCH_SPLIT(dev)) {
> -               lvds_reg = PCH_LVDS;
> -       } else {
> -               lvds_reg = LVDS;
> -       }
> +       struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
> +       u32 tmp;
>
> -       tmp = I915_READ(lvds_reg);
> +       tmp = I915_READ(lvds_encoder->reg);
>
>         if (!(tmp & LVDS_PORT_EN))
>                 return false;
> @@ -102,19 +98,17 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
>         struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>         struct intel_crtc *intel_crtc = to_intel_crtc(encoder->base.crtc);
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 ctl_reg, lvds_reg, stat_reg;
> +       u32 ctl_reg, stat_reg;
>
>         if (HAS_PCH_SPLIT(dev)) {
>                 ctl_reg = PCH_PP_CONTROL;
> -               lvds_reg = PCH_LVDS;
>                 stat_reg = PCH_PP_STATUS;
>         } else {
>                 ctl_reg = PP_CONTROL;
> -               lvds_reg = LVDS;
>                 stat_reg = PP_STATUS;
>         }
>
> -       I915_WRITE(lvds_reg, I915_READ(lvds_reg) | LVDS_PORT_EN);
> +       I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) | LVDS_PORT_EN);
>
>         if (lvds_encoder->pfit_dirty) {
>                 /*
> @@ -133,7 +127,7 @@ static void intel_enable_lvds(struct intel_encoder *encoder)
>         }
>
>         I915_WRITE(ctl_reg, I915_READ(ctl_reg) | POWER_TARGET_ON);
> -       POSTING_READ(lvds_reg);
> +       POSTING_READ(lvds_encoder->reg);
>         if (wait_for((I915_READ(stat_reg) & PP_ON) != 0, 1000))
>                 DRM_ERROR("timed out waiting for panel to power on\n");
>
> @@ -145,15 +139,13 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
>         struct drm_device *dev = encoder->base.dev;
>         struct intel_lvds_encoder *lvds_encoder = to_lvds_encoder(&encoder->base);
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 ctl_reg, lvds_reg, stat_reg;
> +       u32 ctl_reg, stat_reg;
>
>         if (HAS_PCH_SPLIT(dev)) {
>                 ctl_reg = PCH_PP_CONTROL;
> -               lvds_reg = PCH_LVDS;
>                 stat_reg = PCH_PP_STATUS;
>         } else {
>                 ctl_reg = PP_CONTROL;
> -               lvds_reg = LVDS;
>                 stat_reg = PP_STATUS;
>         }
>
> @@ -168,8 +160,8 @@ static void intel_disable_lvds(struct intel_encoder *encoder)
>                 lvds_encoder->pfit_dirty = true;
>         }
>
> -       I915_WRITE(lvds_reg, I915_READ(lvds_reg) & ~LVDS_PORT_EN);
> -       POSTING_READ(lvds_reg);
> +       I915_WRITE(lvds_encoder->reg, I915_READ(lvds_encoder->reg) & ~LVDS_PORT_EN);
> +       POSTING_READ(lvds_encoder->reg);
>  }
>
>  static int intel_lvds_mode_valid(struct drm_connector *connector,
> @@ -936,17 +928,11 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
>         return false;
>  }
>
> -static bool compute_is_dual_link_lvds(struct drm_device *dev)
> +static bool compute_is_dual_link_lvds(struct intel_lvds_encoder *lvds_encoder)
>  {
> +       struct drm_device *dev = lvds_encoder->base.base.dev;
>         unsigned int val;
>         struct drm_i915_private *dev_priv = dev->dev_private;
> -       u32 lvds_reg;
> -
> -       if (HAS_PCH_SPLIT(dev)) {
> -               lvds_reg = PCH_LVDS;
> -       } else {
> -               lvds_reg = LVDS;
> -       }
>
>         /* use the module option value if specified */
>         if (i915_lvds_channel_mode > 0)
> @@ -960,7 +946,7 @@ static bool compute_is_dual_link_lvds(struct drm_device *dev)
>          * we need to check "the value to be set" in VBT when LVDS
>          * register is uninitialized.
>          */
> -       val = I915_READ(lvds_reg);
> +       val = I915_READ(lvds_encoder->reg);
>         if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
>                 val = dev_priv->bios_lvds_val;
>
> @@ -1073,6 +1059,12 @@ bool intel_lvds_init(struct drm_device *dev)
>         connector->interlace_allowed = false;
>         connector->doublescan_allowed = false;
>
> +       if (HAS_PCH_SPLIT(dev)) {
> +               lvds_encoder->reg = PCH_LVDS;
> +       } else {
> +               lvds_encoder->reg = LVDS;
> +       }
> +
>         /* create the scaling mode property */
>         drm_mode_create_scaling_mode_property(dev);
>         drm_connector_attach_property(&intel_connector->base,

Patch does not apply because of the line above (replaced by
drm_object_attach_property).

> @@ -1162,7 +1154,7 @@ bool intel_lvds_init(struct drm_device *dev)
>                 goto failed;
>
>  out:
> -       lvds_encoder->is_dual_link = compute_is_dual_link_lvds(dev);
> +       lvds_encoder->is_dual_link = compute_is_dual_link_lvds(lvds_encoder);
>
>         /*
>          * Unlock registers and just
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 09/17] drm/i915: move set_pll_edp to intel_dp.c

[Daniel Vetter]

On Mon, Nov 26, 2012 at 05:22:15PM +0100, Daniel Vetter wrote:
> Now that we enable the cpu edp pll in intel_dp->pre_enable and no
> longer in crtc_mode_set, we can also move the modeset part to the
> intel_dp->mode_set callback. Previously this was not possible because
> the encoder ->mode_set callbacks are called after the crtc mode set
> callback.
> 
> v2: Rebase on top of copy&pasted hsw crtc_mode_set.
> 
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Paulo pointed out that patches 9,10&12 already have an r-b tag from him
from the previous review round that I've failed to apply. Fixed in my
local tree now.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 08/17] drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set

[Paulo Zanoni]

Hi

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Especially getting rid of all things lvds is ... great!
>
> v2: Drop the two additional pre-hsw hunks noticed by Paulo Zanoni.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 126 +----------------------------------
>  1 file changed, 1 insertion(+), 125 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 8f6362e..40f63b0 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5341,9 +5341,6 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 num_connectors++;
>         }
>
> -       WARN(!(HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)),
> -            "Unexpected PCH type %d\n", INTEL_PCH_TYPE(dev));
> -
>         ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
>                                      &has_reduced_clock, &reduced_clock);
>         if (!ok) {
> @@ -5462,23 +5459,14 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         int pipe = intel_crtc->pipe;
>         int plane = intel_crtc->plane;
>         int num_connectors = 0;
> -       intel_clock_t clock, reduced_clock;
> -       u32 dpll = 0, fp = 0, fp2 = 0;
> -       bool ok, has_reduced_clock = false;
> -       bool is_lvds = false, is_dp = false, is_cpu_edp = false;
> +       bool is_dp = false, is_cpu_edp = false;
>         struct intel_encoder *encoder;
> -       u32 temp;
>         int ret;
>         bool dither;
>
>         for_each_encoder_on_crtc(dev, crtc, encoder) {
>                 switch (encoder->type) {
> -               case INTEL_OUTPUT_LVDS:
> -                       is_lvds = true;
> -                       break;
>                 case INTEL_OUTPUT_DISPLAYPORT:
> -                       is_dp = true;
> -                       break;

Removing these 2 lines is wrong. Since my "normal DP" is not a "pch
edp", we set "is_cpu_edp = true" and then everything breaks.

>                 case INTEL_OUTPUT_EDP:
>                         is_dp = true;
>                         if (!intel_encoder_is_pch_edp(&encoder->base))
> @@ -5509,133 +5497,21 @@ static int haswell_crtc_mode_set(struct drm_crtc *crtc,
>         if (!intel_ddi_pll_mode_set(crtc, adjusted_mode->clock))
>                 return -EINVAL;
>
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               ok = ironlake_compute_clocks(crtc, adjusted_mode, &clock,
> -                                            &has_reduced_clock,
> -                                            &reduced_clock);
> -               if (!ok) {
> -                       DRM_ERROR("Couldn't find PLL settings for mode!\n");
> -                       return -EINVAL;
> -               }
> -       }
> -
>         /* Ensure that the cursor is valid for the new mode before changing... */
>         intel_crtc_update_cursor(crtc, true);
>
>         /* determine panel color depth */
>         dither = intel_choose_pipe_bpp_dither(crtc, fb, &intel_crtc->bpp,
>                                               adjusted_mode);
> -       if (is_lvds && dev_priv->lvds_dither)
> -               dither = true;
>
>         DRM_DEBUG_KMS("Mode for pipe %d:\n", pipe);
>         drm_mode_debug_printmodeline(mode);
>
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               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, adjusted_mode, &clock,
> -                                            fp);
> -
> -               /* CPU eDP is the only output that doesn't need a PCH PLL of its
> -                * own on pre-Haswell/LPT generation */
> -               if (!is_cpu_edp) {
> -                       struct intel_pch_pll *pll;
> -
> -                       pll = intel_get_pch_pll(intel_crtc, dpll, fp);
> -                       if (pll == NULL) {
> -                               DRM_DEBUG_DRIVER("failed to find PLL for pipe %d\n",
> -                                                pipe);
> -                               return -EINVAL;
> -                       }
> -               } else
> -                       intel_put_pch_pll(intel_crtc);
> -
> -               /* The LVDS pin pair needs to be on before the DPLLs are
> -                * enabled.  This is an exception to the general rule that
> -                * mode_set doesn't turn things on.
> -                */
> -               if (is_lvds) {
> -                       temp = I915_READ(PCH_LVDS);
> -                       temp |= LVDS_PORT_EN | LVDS_A0A2_CLKA_POWER_UP;
> -                       if (HAS_PCH_CPT(dev)) {
> -                               temp &= ~PORT_TRANS_SEL_MASK;
> -                               temp |= PORT_TRANS_SEL_CPT(pipe);
> -                       } else {
> -                               if (pipe == 1)
> -                                       temp |= LVDS_PIPEB_SELECT;
> -                               else
> -                                       temp &= ~LVDS_PIPEB_SELECT;
> -                       }
> -
> -                       /* set the corresponsding LVDS_BORDER bit */
> -                       temp |= dev_priv->lvds_border_bits;
> -                       /* Set the B0-B3 data pairs corresponding to whether
> -                        * we're going to set the DPLLs for dual-channel mode or
> -                        * not.
> -                        */
> -                       if (clock.p2 == 7)
> -                               temp |= LVDS_B0B3_POWER_UP | LVDS_CLKB_POWER_UP;
> -                       else
> -                               temp &= ~(LVDS_B0B3_POWER_UP |
> -                                         LVDS_CLKB_POWER_UP);
> -
> -                       /* It would be nice to set 24 vs 18-bit mode
> -                        * (LVDS_A3_POWER_UP) appropriately here, but we need to
> -                        * look more thoroughly into how panels behave in the
> -                        * two modes.
> -                        */
> -                       temp &= ~(LVDS_HSYNC_POLARITY | LVDS_VSYNC_POLARITY);
> -                       if (adjusted_mode->flags & DRM_MODE_FLAG_NHSYNC)
> -                               temp |= LVDS_HSYNC_POLARITY;
> -                       if (adjusted_mode->flags & DRM_MODE_FLAG_NVSYNC)
> -                               temp |= LVDS_VSYNC_POLARITY;
> -                       I915_WRITE(PCH_LVDS, temp);
> -               }
> -       }
> -
>         if (is_dp && !is_cpu_edp) {
>                 intel_dp_set_m_n(crtc, mode, adjusted_mode);
> -       } else {
> -               if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -                       /* For non-DP output, clear any trans DP clock recovery
> -                        * setting.*/
> -                       I915_WRITE(TRANSDATA_M1(pipe), 0);
> -                       I915_WRITE(TRANSDATA_N1(pipe), 0);
> -                       I915_WRITE(TRANSDPLINK_M1(pipe), 0);
> -                       I915_WRITE(TRANSDPLINK_N1(pipe), 0);
> -               }
>         }
>
>         intel_crtc->lowfreq_avail = false;
> -       if (HAS_PCH_IBX(dev) || HAS_PCH_CPT(dev)) {
> -               if (intel_crtc->pch_pll) {
> -                       I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -
> -                       /* Wait for the clocks to stabilize. */
> -                       POSTING_READ(intel_crtc->pch_pll->pll_reg);
> -                       udelay(150);
> -
> -                       /* The pixel multiplier can only be updated once the
> -                        * DPLL is enabled and the clocks are stable.
> -                        *
> -                        * So write it again.
> -                        */
> -                       I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
> -               }
> -
> -               if (intel_crtc->pch_pll) {
> -                       if (is_lvds && has_reduced_clock && i915_powersave) {
> -                               I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp2);
> -                               intel_crtc->lowfreq_avail = true;
> -                       } else {
> -                               I915_WRITE(intel_crtc->pch_pll->fp1_reg, fp);
> -                       }
> -               }
> -       }
>
>         intel_set_pipe_timings(intel_crtc, mode, adjusted_mode);
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 01/17] drm/i915: add encoder->pre_pll_enable callback

[Paulo Zanoni]

Hi

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Currently we have two encoder specific bits in the common mode_set
> functions:
> - lvds pin pair enabling
> - dp m/n setting and computation
>
> Now the lvds stuff needs to happen before the pll is enabled. Since
> that is done in the crtc_mode_set functions, we need to add a new
> callback to be able to move them to the encoder code (where they
> belong). The dp m/n stuff is a giant mess anyway (since it also
> confuses itself with the fdi link m/n handling), so that needs to be
> handled separately.
>
> I think that we can move the pll enabling down quite a bit, which
> might allow us to eventually merge encoder->pre_enable with this new
> pre_pll_enable callback. But for now this will allow us to clean
> things up a bit.
>
> Note that vlv doesn't support lvds, hence we don't need to change
> anything in there.

My only worry is that in the future we might use the pre_pll_enable
callback for some non-LVDS encoder, and then we'll forget about
calling this on VLV.

Even if you don't think this is necessary, the patch looks correct, so:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Although I'd prefer to see "v3".

>
> v2: Fixup commit message, both suggested from Paulo Zanoni.
> - dp m/n doesn't need to happen before pll enabling
> - lvds doesn't exist on vlv, hence no changes required in the vlv pll
>   function.
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 14 ++++++++++++++
>  drivers/gpu/drm/i915/intel_drv.h     |  1 +
>  2 files changed, 15 insertions(+)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 769fc8f..647f16e 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -4462,6 +4462,7 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       struct intel_encoder *encoder;
>         int pipe = intel_crtc->pipe;
>         u32 dpll;
>         bool is_sdvo;
> @@ -4530,6 +4531,10 @@ static void i9xx_update_pll(struct drm_crtc *crtc,
>         POSTING_READ(DPLL(pipe));
>         udelay(150);
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         /* The LVDS pin pair needs to be on before the DPLLs are enabled.
>          * This is an exception to the general rule that mode_set doesn't turn
>          * things on.
> @@ -4574,6 +4579,7 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         struct drm_device *dev = crtc->dev;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         struct intel_crtc *intel_crtc = to_intel_crtc(crtc);
> +       struct intel_encoder *encoder;
>         int pipe = intel_crtc->pipe;
>         u32 dpll;
>
> @@ -4607,6 +4613,10 @@ static void i8xx_update_pll(struct drm_crtc *crtc,
>         POSTING_READ(DPLL(pipe));
>         udelay(150);
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         /* The LVDS pin pair needs to be on before the DPLLs are enabled.
>          * This is an exception to the general rule that mode_set doesn't turn
>          * things on.
> @@ -5535,6 +5545,10 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                 I915_WRITE(TRANSDPLINK_N1(pipe), 0);
>         }
>
> +       for_each_encoder_on_crtc(dev, crtc, encoder)
> +               if (encoder->pre_pll_enable)
> +                       encoder->pre_pll_enable(encoder);
> +
>         if (intel_crtc->pch_pll) {
>                 I915_WRITE(intel_crtc->pch_pll->pll_reg, dpll);
>
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index bcc5241..42a40a1 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -153,6 +153,7 @@ struct intel_encoder {
>         bool cloneable;
>         bool connectors_active;
>         void (*hot_plug)(struct intel_encoder *);
> +       void (*pre_pll_enable)(struct intel_encoder *);
>         void (*pre_enable)(struct intel_encoder *);
>         void (*enable)(struct intel_encoder *);
>         void (*disable)(struct intel_encoder *);
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 04/17] drm/i915: track is_dual_link in intel_lvds

[Paulo Zanoni]

Hi

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Yeah, all users (both the clock selection special cases and the lvds
> pin pair stuff) are still in common code, but this will change.
>
> v2: Rebase on top of Jani Nikula's panel rework.
>
> v3: Incorporate review from Paulo Zanoni:
> - s/__is_dual_link_lvds/compute_is_dual_link_lvds
> - kill dev_priv->lvds_val
> - drop spurious whitespace change
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/i915_drv.h   |  1 -
>  drivers/gpu/drm/i915/intel_lvds.c | 42 +++++++++++++++++++++++++++------------
>  2 files changed, 29 insertions(+), 14 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h
> index 3229f04..2e92617 100644
> --- a/drivers/gpu/drm/i915/i915_drv.h
> +++ b/drivers/gpu/drm/i915/i915_drv.h
> @@ -704,7 +704,6 @@ typedef struct drm_i915_private {
>         unsigned int display_clock_mode:1;
>         int lvds_ssc_freq;
>         unsigned int bios_lvds_val; /* initial [PCH_]LVDS reg val in VBIOS */
> -       unsigned int lvds_val; /* used for checking LVDS channel mode */
>         struct {
>                 int rate;
>                 int lanes;
> diff --git a/drivers/gpu/drm/i915/intel_lvds.c b/drivers/gpu/drm/i915/intel_lvds.c
> index 84255d0..1090f1b 100644
> --- a/drivers/gpu/drm/i915/intel_lvds.c
> +++ b/drivers/gpu/drm/i915/intel_lvds.c
> @@ -52,6 +52,7 @@ struct intel_lvds_encoder {
>         u32 pfit_control;
>         u32 pfit_pgm_ratios;
>         bool pfit_dirty;
> +       bool is_dual_link;
>
>         struct intel_lvds_connector *attached_connector;
>  };
> @@ -920,6 +921,23 @@ static const struct dmi_system_id intel_dual_link_lvds[] = {
>
>  bool intel_is_dual_link_lvds(struct drm_device *dev)
>  {
> +       struct intel_encoder *encoder;
> +       struct intel_lvds_encoder *lvds_encoder;
> +
> +       list_for_each_entry(encoder, &dev->mode_config.encoder_list,
> +                           base.head) {
> +               if (encoder->type == INTEL_OUTPUT_LVDS) {
> +                       lvds_encoder = to_lvds_encoder(&encoder->base);
> +
> +                       return lvds_encoder->is_dual_link;
> +               }
> +       }
> +
> +       return false;
> +}
> +
> +static bool compute_is_dual_link_lvds(struct drm_device *dev)
> +{
>         unsigned int val;
>         struct drm_i915_private *dev_priv = dev->dev_private;
>         u32 lvds_reg;
> @@ -937,19 +955,15 @@ bool intel_is_dual_link_lvds(struct drm_device *dev)
>         if (dmi_check_system(intel_dual_link_lvds))
>                 return true;
>
> -       if (dev_priv->lvds_val)
> -               val = dev_priv->lvds_val;
> -       else {
> -               /* BIOS should set the proper LVDS register value at boot, but
> -                * in reality, it doesn't set the value when the lid is closed;
> -                * we need to check "the value to be set" in VBT when LVDS
> -                * register is uninitialized.
> -                */
> -               val = I915_READ(lvds_reg);
> -               if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> -                       val = dev_priv->bios_lvds_val;
> -               dev_priv->lvds_val = val;
> -       }
> +       /* BIOS should set the proper LVDS register value at boot, but
> +        * in reality, it doesn't set the value when the lid is closed;
> +        * we need to check "the value to be set" in VBT when LVDS
> +        * register is uninitialized.
> +        */
> +       val = I915_READ(lvds_reg);
> +       if (!(val & ~(LVDS_PIPE_MASK | LVDS_DETECTED)))
> +               val = dev_priv->bios_lvds_val;
> +
>         return (val & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP;
>  }
>
> @@ -1148,6 +1162,8 @@ bool intel_lvds_init(struct drm_device *dev)
>                 goto failed;
>
>  out:
> +       lvds_encoder->is_dual_link = compute_is_dual_link_lvds(dev);
> +

<bikeshedding>
Here we could try to add some debug message telling whether we found
dual-link LVDS or not. I don't know how useful it would be, but I
added it to test your patches on my machine (I was not really sure
whether it supported dual-link LVDS or not).

Something like:
+ if (lvds_encoder->is_dual_link)
+        DRM_DEBUG_KMS("Dual link LVDS found\n");
</bikeshedding>

Still, the patch looks correct, so with or without the debug message:
Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>



>         /*
>          * Unlock registers and just
>          * leave them unlocked
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 02/17] drm/i915: replace ad-hoc dual-link lvds checks

[Paulo Zanoni]

Hi

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> ... with is_dual_link_lvds introduced in
>
> commit b03543857fd75876b96e10d4320b775e95041bb7
> Author: Takashi Iwai <tiwai@suse.de>
> Date:   Tue Mar 20 13:07:05 2012 +0100
>
>     drm/i915: Check VBIOS value for determining LVDS dual channel mode, too
>
> All these checks predate this commit and have simply been overlooked.
> Since we don't support switching between single-link and dual-link
> modes anyway, this different checks could at best only get in the way
> of refactorings, and in the worst case cause inconsistencies.
>
> v2: Update the comment, we now have a solid way to figure out whether
> we need dual-link lvds or not (falling back to vbt values as a last
> resort). We still don't know how to switch between dual-link and
> single link so leave that part intact. I'm not sure though whether
> switching between these two modes makes any sense - we always drive
> the panel at its fixed mode (with a fixed bpc) anyway ...
>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>

Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_display.c | 15 ++++++---------
>  1 file changed, 6 insertions(+), 9 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index 647f16e..dd4cb43 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -690,13 +690,11 @@ intel_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>         intel_clock_t clock;
>         int err = target;
>
> -       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS) &&
> -           (I915_READ(LVDS)) != 0) {
> +       if (intel_pipe_has_type(crtc, INTEL_OUTPUT_LVDS)) {
>                 /*
> -                * For LVDS, if the panel is on, just rely on its current
> -                * settings for dual-channel.  We haven't figured out how to
> -                * reliably set up different single/dual channel state, if we
> -                * even can.
> +                * For LVDS just rely on its current settings for dual-channel.
> +                * We haven't figured out how to reliably set up different
> +                * single/dual channel state, if we even can.
>                  */
>                 if (is_dual_link_lvds(dev_priv, LVDS))
>                         clock.p2 = limit->p2.p2_fast;
> @@ -766,8 +764,7 @@ intel_g4x_find_best_PLL(const intel_limit_t *limit, struct drm_crtc *crtc,
>                         lvds_reg = PCH_LVDS;
>                 else
>                         lvds_reg = LVDS;
> -               if ((I915_READ(lvds_reg) & LVDS_CLKB_POWER_MASK) ==
> -                   LVDS_CLKB_POWER_UP)
> +               if (is_dual_link_lvds(dev_priv, lvds_reg))
>                         clock.p2 = limit->p2.p2_fast;
>                 else
>                         clock.p2 = limit->p2.p2_slow;
> @@ -5357,7 +5354,7 @@ static uint32_t ironlake_compute_dpll(struct intel_crtc *intel_crtc,
>         if (is_lvds) {
>                 if ((intel_panel_use_ssc(dev_priv) &&
>                      dev_priv->lvds_ssc_freq == 100) ||
> -                   (I915_READ(PCH_LVDS) & LVDS_CLKB_POWER_MASK) == LVDS_CLKB_POWER_UP)
> +                   is_dual_link_lvds(dev_priv, PCH_LVDS))
>                         factor = 25;
>         } else if (is_sdvo && is_tv)
>                 factor = 20;
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 00/17] lvds/dp/watermark refactorings

[Paulo Zanoni]

2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> Hi all,
>
> Second round for my lvds/dp/watermark refactorings, which are a prep work for
> the next step in reworking the modeset code (which is currently brewing at a
> rather slow rate in my modeset-rework branch). Safe where I've disagreed this
> should incorporate all review feedback from Paulo Zanoni.
>
> Comments&flames highly welcome.

I just briefly tested patches 1 to 7 on an ILK laptop with a dual-link
LVDS. I just booted, changed modes, checked dmesg and then turned the
machine off. So "Briefly-tested-by: Paulo Zanoni
<paulo.r.zanonI@intel.com>". All the 7 patches have "Reviewed-by"
stamps now, so If you don't plan to apply any of the bikesheds, you
could just merge them and then we can focus on only discussing patches
8 to 17.

>
> Thanks, Daniel
>
> Daniel Vetter (17):
>   drm/i915: add encoder->pre_pll_enable callback
>   drm/i915: replace ad-hoc dual-link lvds checks
>   drm/i915: move is_dual_link_lvds to intel_lvds.c
>   drm/i915: track is_dual_link in intel_lvds
>   drm/i915: add intel_lvds->reg
>   drm/i915: move intel_update_lvds to intel_lvds->pre_pll_enable
>   drm/i915: enable intel_lvds->pre_pll_enable for ilk+, too
>   drm/i915: rip out pre-DDI stuff from haswell_crtc_mode_set
>   drm/i915: move set_pll_edp to intel_dp.c
>   drm/i915: rip out pre-production ilk cpu edp w/a
>   drm/i915: use wait_for_vblank instead of msleep(17)
>   drm/i915: WARN on !crtc in intel_dp_link_down
>   drm/i915: drop unnecessary clearing of pch dp transcoder timings
>   drm/i915: extract common link_m_n helpers
>   drm/i915: don't call update_watermark in crtc_mode_set
>   drm/i915: don't call update_watermarks from haswell enable/disable
>     code
>   drm/i915: rip out update_linetime_wm abstraction
>
>  drivers/gpu/drm/i915/i915_drv.h      |  16 +-
>  drivers/gpu/drm/i915/intel_display.c | 430 +++++------------------------------
>  drivers/gpu/drm/i915/intel_dp.c      |  87 ++++---
>  drivers/gpu/drm/i915/intel_drv.h     |   4 +-
>  drivers/gpu/drm/i915/intel_lvds.c    | 160 +++++++++++--
>  drivers/gpu/drm/i915/intel_pm.c      |  37 ---
>  6 files changed, 258 insertions(+), 476 deletions(-)
>
> --
> 1.7.11.7
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 01/17] drm/i915: add encoder->pre_pll_enable callback

[Daniel Vetter]

On Tue, Nov 27, 2012 at 06:38:18PM -0200, Paulo Zanoni wrote:
> Hi
> 
> 2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Currently we have two encoder specific bits in the common mode_set
> > functions:
> > - lvds pin pair enabling
> > - dp m/n setting and computation
> >
> > Now the lvds stuff needs to happen before the pll is enabled. Since
> > that is done in the crtc_mode_set functions, we need to add a new
> > callback to be able to move them to the encoder code (where they
> > belong). The dp m/n stuff is a giant mess anyway (since it also
> > confuses itself with the fdi link m/n handling), so that needs to be
> > handled separately.
> >
> > I think that we can move the pll enabling down quite a bit, which
> > might allow us to eventually merge encoder->pre_enable with this new
> > pre_pll_enable callback. But for now this will allow us to clean
> > things up a bit.
> >
> > Note that vlv doesn't support lvds, hence we don't need to change
> > anything in there.
> 
> My only worry is that in the future we might use the pre_pll_enable
> callback for some non-LVDS encoder, and then we'll forget about
> calling this on VLV.

I really hope that we've cleaned things up by then and this pre_pll_enable
hook is gone again. So merged as-is.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 04/17] drm/i915: track is_dual_link in intel_lvds

[Daniel Vetter]

On Tue, Nov 27, 2012 at 06:58:51PM -0200, Paulo Zanoni wrote:
> <bikeshedding>
> Here we could try to add some debug message telling whether we found
> dual-link LVDS or not. I don't know how useful it would be, but I
> added it to test your patches on my machine (I was not really sure
> whether it supported dual-link LVDS or not).
> 
> Something like:
> + if (lvds_encoder->is_dual_link)
> +        DRM_DEBUG_KMS("Dual link LVDS found\n");
> </bikeshedding>
> 
> Still, the patch looks correct, so with or without the debug message:
> Reviewed-by: Paulo Zanoni <paulo.r.zanoni@intel.com>

Excellent idea, I've slightly generalized the output so that it tells
whether single or dual link has been detected.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 00/17] lvds/dp/watermark refactorings

[Daniel Vetter]

On Tue, Nov 27, 2012 at 07:14:01PM -0200, Paulo Zanoni wrote:
> 2012/11/26 Daniel Vetter <daniel.vetter@ffwll.ch>:
> > Hi all,
> >
> > Second round for my lvds/dp/watermark refactorings, which are a prep work for
> > the next step in reworking the modeset code (which is currently brewing at a
> > rather slow rate in my modeset-rework branch). Safe where I've disagreed this
> > should incorporate all review feedback from Paulo Zanoni.
> >
> > Comments&flames highly welcome.
> 
> I just briefly tested patches 1 to 7 on an ILK laptop with a dual-link
> LVDS. I just booted, changed modes, checked dmesg and then turned the
> machine off. So "Briefly-tested-by: Paulo Zanoni
> <paulo.r.zanonI@intel.com>". All the 7 patches have "Reviewed-by"
> stamps now, so If you don't plan to apply any of the bikesheds, you
> could just merge them and then we can focus on only discussing patches
> 8 to 17.

Ok, patches 1-7 merged to dinq. I'll rework 8, including your r-b tags
where I've missed them for the DP patches, and then patchbom again.

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