[PATCH 0/9] dp refactoring and minor other stuff

[PATCH 0/9] dp refactoring and minor other stuff

[Daniel Vetter]

Hi all,

Mostly just moves the dp code to saner places, but I've also thrown 3 patches on
top to remove some bloat around the watermark handling.

Right the next patch in my modeset-rework branch after this pile is the
introduction of the intel_crtc_config to precompute the pipe config. I'm not yet
sure whether that's going in the exact right direction, so still needs more
work.

Cheers, Daniel

Daniel Vetter (9):
  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      |  15 +++-
 drivers/gpu/drm/i915/intel_display.c | 133 ++++++++++-------------------------
 drivers/gpu/drm/i915/intel_dp.c      |  87 +++++++++++------------
 drivers/gpu/drm/i915/intel_drv.h     |   2 -
 drivers/gpu/drm/i915/intel_pm.c      |  37 ----------
 5 files changed, 92 insertions(+), 182 deletions(-)

-- 
1.7.11.7

[PATCH 1/9] 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 27fc014..a221eb9 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2297,43 +2297,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;
@@ -5433,9 +5396,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);
@@ -5532,10 +5492,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 2/9] 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 3/9] 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 4/9] 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 5/9] 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 rather 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 | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index a221eb9..0c923a6 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5349,15 +5349,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)
@@ -5472,18 +5465,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);
-	} 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;
 
-- 
1.7.11.7

[PATCH 6/9] 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 c4339c2..23cea8f 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 0c923a6..5432e9c 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3956,16 +3956,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;
@@ -3973,20 +3965,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)
@@ -5100,7 +5090,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;
 
@@ -5158,8 +5148,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 7/9] 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 5432e9c..1ab1935 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -4715,8 +4715,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;
 }
 
@@ -5388,8 +5386,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;
@@ -5472,8 +5468,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 8/9] drm/i915: don't call update_watermarks from haswell enable/disable code

[Daniel Vetter]

We don't use these on haswell, since haswell as 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 1ab1935..633cc3d 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3360,7 +3360,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);
 
@@ -3548,7 +3547,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 9/9] 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 23cea8f..8868a3f 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 633cc3d..bad1a36 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -5384,11 +5384,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,
@@ -5466,7 +5491,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 ee4a4ba..ee8f78d 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 8f15616..33cbfff 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1885,33 +1885,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,
@@ -2107,15 +2080,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)
 {
@@ -3977,7 +3941,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 1/9] drm/i915: move set_pll_edp to intel_dp.c

[Paulo Zanoni]

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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>

Yay to all those magic unnamed registers....

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

> ---
>  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 27fc014..a221eb9 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -2297,43 +2297,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;
> @@ -5433,9 +5396,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);
> @@ -5532,10 +5492,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 2/9] drm/i915: rip out pre-production ilk cpu edp w/a

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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>

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

> 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.
> +                */

And if we're lucky, this fixes bugs :)

> +               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



-- 
Paulo Zanoni

Re: [PATCH 4/9] drm/i915: WARN on !crtc in intel_dp_link_down

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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>

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

> ---
>  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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 5/9] drm/i915: drop unnecessary clearing of pch dp transcoder timings

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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 rather 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.

Notice that we have the CPU M/N registers and the PCH M/N registers.
And the PCH M/N registers are only used by DisplayPort, so it makes
sense to zero them for non-DisplayPort. If we really want this, how
about we add this patch only in the beginning of the 3.9 development
cycle?

>
> Signed-off-by: Daniel Vetter <daniel.vetter@ffwll.ch>
> ---
>  drivers/gpu/drm/i915/intel_display.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
> index a221eb9..0c923a6 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -5349,15 +5349,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)
> @@ -5472,18 +5465,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);
> -       } 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);
> -               }
> -       }

This is the only code chunk I'm sure won't cause any problems, so you
might add it to the LVDS patch that already removes IBX/CPT stuff from
haswell_crtc_mode_set :)

>
>         intel_crtc->lowfreq_avail = false;
>
> --
> 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 5/9] drm/i915: drop unnecessary clearing of pch dp transcoder timings

[Daniel Vetter]

On Fri, Nov 16, 2012 at 8:52 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> Notice that we have the CPU M/N registers and the PCH M/N registers.
> And the PCH M/N registers are only used by DisplayPort, so it makes
> sense to zero them for non-DisplayPort. If we really want this, how
> about we add this patch only in the beginning of the 3.9 development
> cycle?

Well, all that "let's clear things harder" is imo all a symptom of our
pre-modeset-rework days where we simply didn't keep track of the hw
state well enough. Hence I'm on a holy killing spree against them all
;-)

And yes, this is all for 3.9
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 6/9] drm/i915: extract common link_m_n helpers

[Paulo Zanoni]

Hi

2012/11/5 Daniel Vetter <daniel.vetter@ffwll.ch>:
> 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>

This is certainly a first improvement to the m_n code and doing the
exact same thing as you did was even on my TODO list. I think there's
a lot more to do, but this patch is certainly the first step. There is
just one minor bikeshed below:

> ---
>  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 c4339c2..23cea8f 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 0c923a6..5432e9c 100644
> --- a/drivers/gpu/drm/i915/intel_display.c
> +++ b/drivers/gpu/drm/i915/intel_display.c
> @@ -3956,16 +3956,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;
> @@ -3973,20 +3965,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;

I think here we should keep the gmch_m and gmch_n calculations used in
the DP code instead of this one, since it matches the spec a little
better... The spec says that gmch_m should be "dot clock * bytes per
pixel" and gmch_n should be "ls_clk * number of lanes". So the lines
that are currently inside intel_dp_compute_m_n look better:

       m_n->gmch_m = (pixel_clock * bpp) >> 3;
       m_n->gmch_n = link_clock * nlanes;

(Also, in this case we'll need even higher numbers before we overflow)

I applied a version with the DP values locally and tested. So my
version of this patch is Reviewed-by and Tested-by me.

> -       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)
> @@ -5100,7 +5090,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;
>
> @@ -5158,8 +5148,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
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx



-- 
Paulo Zanoni

Re: [PATCH 6/9] drm/i915: extract common link_m_n helpers

[Daniel Vetter]

On Tue, Nov 20, 2012 at 12:24 PM, Paulo Zanoni <przanoni@gmail.com> wrote:
> I think here we should keep the gmch_m and gmch_n calculations used in
> the DP code instead of this one, since it matches the spec a little
> better... The spec says that gmch_m should be "dot clock * bytes per
> pixel" and gmch_n should be "ls_clk * number of lanes". So the lines
> that are currently inside intel_dp_compute_m_n look better:
>
>        m_n->gmch_m = (pixel_clock * bpp) >> 3;
>        m_n->gmch_n = link_clock * nlanes;
>
> (Also, in this case we'll need even higher numbers before we overflow)
>
> I applied a version with the DP values locally and tested. So my
> version of this patch is Reviewed-by and Tested-by me.

How exactly should dot_clock * bytes_per_pixel work for e.g. 6bpc?
Note that the above essentially just does part of the ratio reduction
intel_reduce does and doesn't gain us any headroom for overflows,
since in both cases we compute pixel_clock * bpp. The only difference
is that we don't compute link_clock *nlanes * 8, but since the data
can (almost) fill the link bw, we don't really gain anything.

So imo computing the ration of bits that flow through the link is much
more natural, and trying to compute the ration as bytes only
obfuscates things.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch