[PATCH 0/7] drm/i915: IVB FDI B/C fixes and misc cleanups

[PATCH 0/7] drm/i915: IVB FDI B/C fixes and misc cleanups

[Keith Packard]

This is the complete set of patches that yield a working 3-pipe mode
setting configuration on my test machines. It does not make DPMS work,
so I still need to figure that out. As the DPMS paths are almost
entirely different from mode setting (whose crazy idea was that,
anyway?), that may take a bit more time.

I've managed to reproduce this bug with only two monitors just by
forcing them onto FDI B/C, but only with 1920x1080 and larger modes:

 $ xrandr --output VGA1 --off --output DP1 --off
 $ xrandr --output VGA1 --auto --crtc 1
 $ xrandr --output DP1 --auto --crtc 2

That works about 50% of the time on the original kernel. If it works,
you can just try a couple of different modes to see if you can get it
to fail. Just use reasonably large modes -- 640x480 has never failed
for me. I'd love to know why

 $ xrandr --output DP1 --mode 1600x1200 --crtc 2

Finally, the X server has a terrible bug in the RandR code. It
computes a desired initial configuration, and if that fails in any
way, it refuses to start at all. So, if you connect three monitors
that all require separate PLLs, then you get two monitors lit up, the
third one fails and the X server aborts. Need to fix that so that if
*any* monitors light up, the X server will keep going.
 
                   *** The patches: ***

 [PATCH 1/7] drm/i915: Allow VGA on CRTC 2

Silly hold-over from the bad-old PLL sharing code.

 [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

Here's the patch which rejects modes that the FDI B/C lane sharing
hardware can't support. This only affects modes larger than 1920x1200
though as that mode and smaller all fit in two FDI lanes. I ran across
this while testing with a 2560x1200 monitor.

 [PATCH 3/7] drm/i915: Delay between FDI link training tries. Clear

This seems like a sensible change in the FDI link training code --
gives the FDI hardware time to adapt to changes in the
signalling. But, as I've never seen FDI fail to train, I'm not sure
it's useful.

 [PATCH 4/7] drm/i915: Check display_bpc against max_fdi_bpp after

This mostly just cleans up some debug messages by moving various BPP
computations around. "should" have no effect on the hardware.

 [PATCH 5/7] drm/i915: Pipe-C only configurations would not get SR

Just happened across this obvious bug while reading through the
driver.

 [PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

The specs don't say which order to do this in, but it doesn't make
sense to do these in the current order. With this in place, I saw mode
setting errors reduced quite a bit, but not gone.

 [PATCH 7/7] drm/i915: Merge FDI RX reg writes during training

This may be the only patch necessary to get mode setting working on
FDI-B/C, it ensures that error correction is always turned on during
link training. The old code left error correction disabled as there
was no posting read after setting that. I'm hoping this explains why
the problem wasn't reliably reproducible -- the problem depended on
how long the write waited to get to the hardware. I haven't done
enough testing of this patch in isolation to know if this is true or not.

-keith

[PATCH 1/7] drm/i915: Allow VGA on CRTC 2

[Keith Packard]

This is left over from the old PLL sharing code and isn't useful now
that PLLs are shared when possible.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_crt.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index bc5e2c9..7997b24 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -664,7 +664,7 @@ void intel_crt_init(struct drm_device *dev)
 	if (IS_HASWELL(dev))
 		crt->base.crtc_mask = (1 << 0);
 	else
-		crt->base.crtc_mask = (1 << 0) | (1 << 1);
+		crt->base.crtc_mask = (1 << 0) | (1 << 1) | (1 << 2);
 
 	if (IS_GEN2(dev))
 		connector->interlace_allowed = 0;
-- 
1.7.10.4

[PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Keith Packard]

IVB shares 4 lanes between FDI B and FDI C. When sharing, compute the
maximum BPC based on the available bandwidth.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |  101 +++++++++++++++++++++++++++++++---
 1 file changed, 94 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 70d30fc..7106807 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3575,7 +3575,7 @@ void intel_encoder_destroy(struct drm_encoder *encoder)
 }
 
 static bool intel_crtc_mode_fixup(struct drm_crtc *crtc,
-				  struct drm_display_mode *mode,
+				  const struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode)
 {
 	struct drm_device *dev = crtc->dev;
@@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct drm_i915_private *dev_priv)
  */
 static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 					 unsigned int *pipe_bpp,
-					 struct drm_display_mode *mode)
+					 struct drm_display_mode *mode,
+					 int max_fdi_bpp)
 {
 	struct drm_device *dev = crtc->dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
@@ -3800,6 +3801,15 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 		display_bpc = 6;
 	}
 
+	if (display_bpc * 3 > max_fdi_bpp) {
+		if (max_fdi_bpp < 24)
+			display_bpc = 6;
+		else if (max_fdi_bpp < 30)
+			display_bpc = 8;
+		else if (max_fdi_bpp < 36)
+			display_bpc = 10;
+		DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
+	}
 	/*
 	 * We could just drive the pipe at the highest bpc all the time and
 	 * enable dithering as needed, but that costs bandwidth.  So choose
@@ -4570,6 +4580,53 @@ static int ironlake_get_refclk(struct drm_crtc *crtc)
 	return 120000;
 }
 
+/*
+ * FDI C can only have 2 lanes, borrowed from FDI B
+ */
+
+static int ivb_fdi_max_lanes(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);
+	enum pipe other_pipe;
+	struct drm_crtc *other_crtc;
+	struct intel_crtc *other_intel_crtc;
+	int max_lanes;
+
+	/* FDI links B and C share 4 lanes */
+	switch (intel_crtc->pipe) {
+	case PIPE_B:
+		other_pipe = PIPE_C;
+		max_lanes = 4;
+		break;
+	case PIPE_C:
+		other_pipe = PIPE_B;
+		max_lanes = 2;
+		break;
+	default:
+		return 4;
+	}
+	other_crtc = dev_priv->pipe_to_crtc_mapping[other_pipe];
+	other_intel_crtc = to_intel_crtc(other_crtc);
+
+	/* If the other FDI link isn't running, we can use all of the
+	 * available lanes
+	 */
+	if (!other_intel_crtc->active)
+		return max_lanes;
+
+	/* If the other FDI link is using too many lanes, we can't have
+	 * any
+	 */
+	if (other_intel_crtc->fdi_lanes > 2)
+		return 0;
+
+	/* When both are running, we only get 2 lanes at most
+	 */
+	return 2;
+}
+
 static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 				  struct drm_display_mode *mode,
 				  struct drm_display_mode *adjusted_mode,
@@ -4595,6 +4652,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	unsigned int pipe_bpp;
 	bool dither;
 	bool is_cpu_edp = false, is_pch_edp = false;
+	int max_fdi_bpp;
+	int max_lane;
 
 	for_each_encoder_on_crtc(dev, crtc, encoder) {
 		switch (encoder->type) {
@@ -4672,7 +4731,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	   according to current link config */
 	if (is_cpu_edp) {
 		intel_edp_link_config(edp_encoder, &lane, &link_bw);
+		max_fdi_bpp = 0;
+		max_lane = lane;
 	} else {
+		u32	fdi_bw;
+
+		/* [e]DP over FDI requires target mode clock
+		   instead of link clock */
+		if (is_dp)
+			target_clock = mode->clock;
+		else
+			target_clock = adjusted_mode->clock;
+
 		/* FDI is a binary signal running at ~2.7GHz, encoding
 		 * each output octet as 10 bits. The actual frequency
 		 * is stored as a divider into a 100MHz clock, and the
@@ -4681,6 +4751,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		 * is:
 		 */
 		link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
+
+		max_lane = 4;
+		if (IS_IVYBRIDGE(dev))
+			max_lane = ivb_fdi_max_lanes(crtc);
+
+		/*
+		 * Compute the available FDI bandwidth, use that
+		 * to compute the maximum supported BPP
+		 */
+		fdi_bw = link_bw * max_lane * 19 / 20;
+		max_fdi_bpp = fdi_bw / target_clock;
+		DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
 	}
 
 	/* [e]DP over FDI requires target mode clock instead of link clock. */
@@ -4694,7 +4776,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 	/* determine panel color depth */
 	temp = I915_READ(PIPECONF(pipe));
 	temp &= ~PIPE_BPC_MASK;
-	dither = intel_choose_pipe_bpp_dither(crtc, &pipe_bpp, mode);
+	dither = intel_choose_pipe_bpp_dither(crtc, &pipe_bpp, mode, max_fdi_bpp);
 	switch (pipe_bpp) {
 	case 18:
 		temp |= PIPE_6BPC;
@@ -4716,19 +4798,24 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		break;
 	}
 
-	intel_crtc->bpp = pipe_bpp;
-	I915_WRITE(PIPECONF(pipe), temp);
-
 	if (!lane) {
 		/*
 		 * Account for spread spectrum to avoid
 		 * oversubscribing the link. Max center spread
 		 * is 2.5%; use 5% for safety's sake.
 		 */
-		u32 bps = target_clock * intel_crtc->bpp * 21 / 20;
+		u32 bps = target_clock * pipe_bpp * 21 / 20;
 		lane = bps / (link_bw * 8) + 1;
+		if (lane > max_lane) {
+			DRM_ERROR("Not enough lanes available for mode! (want %d have %d)\n",
+				  lane, max_lane);
+			return -EINVAL;
+		}
 	}
 
+	intel_crtc->bpp = pipe_bpp;
+	I915_WRITE(PIPECONF(pipe), temp);
+
 	intel_crtc->fdi_lanes = lane;
 
 	if (pixel_multiplier > 1)
-- 
1.7.10.4

[PATCH 3/7] drm/i915: Delay between FDI link training tries. Clear FDI_RX_IIR before training

[Keith Packard]

Just a bit of cleanup; it appears to have no effect.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |    7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 7106807..95248bd 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2391,6 +2391,7 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 	temp |= FDI_LINK_TRAIN_PATTERN_1;
 	I915_WRITE(reg, temp | FDI_TX_ENABLE);
 
+	I915_WRITE(FDI_RX_IIR(pipe), FDI_RX_BIT_LOCK);
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
 	temp &= ~FDI_LINK_TRAIN_NONE;
@@ -2398,10 +2399,10 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 	I915_WRITE(reg, temp | FDI_RX_ENABLE);
 
 	POSTING_READ(reg);
-	udelay(150);
 
 	/* Ironlake workaround, enable clock pointer after FDI enable*/
 	if (HAS_PCH_IBX(dev)) {
+		udelay(150);
 		I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR);
 		I915_WRITE(FDI_RX_CHICKEN(pipe), FDI_RX_PHASE_SYNC_POINTER_OVR |
 			   FDI_RX_PHASE_SYNC_POINTER_EN);
@@ -2409,6 +2410,7 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 
 	reg = FDI_RX_IIR(pipe);
 	for (tries = 0; tries < 5; tries++) {
+		udelay(150);
 		temp = I915_READ(reg);
 		DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp);
 
@@ -2422,6 +2424,7 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 		DRM_ERROR("FDI train 1 fail!\n");
 
 	/* Train 2 */
+	I915_WRITE(FDI_RX_IIR(pipe), FDI_RX_SYMBOL_LOCK);
 	reg = FDI_TX_CTL(pipe);
 	temp = I915_READ(reg);
 	temp &= ~FDI_LINK_TRAIN_NONE;
@@ -2435,10 +2438,10 @@ static void ironlake_fdi_link_train(struct drm_crtc *crtc)
 	I915_WRITE(reg, temp);
 
 	POSTING_READ(reg);
-	udelay(150);
 
 	reg = FDI_RX_IIR(pipe);
 	for (tries = 0; tries < 5; tries++) {
+		udelay(150);
 		temp = I915_READ(reg);
 		DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp);
 
-- 
1.7.10.4

[PATCH 4/7] drm/i915: Check display_bpc against max_fdi_bpp after display_bpc is set

[Keith Packard]

display_bpc might not have been set before comparing with the
requested mode, so wait until afterwards before comparing with the
supported fdi bandwidth. Not a significant change as any case that
mattered would have worked; this just makes the debug messages look nicer.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   39 ++++++++++++++++++++--------------
 1 file changed, 23 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 95248bd..b099a17 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -3804,15 +3804,6 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 		display_bpc = 6;
 	}
 
-	if (display_bpc * 3 > max_fdi_bpp) {
-		if (max_fdi_bpp < 24)
-			display_bpc = 6;
-		else if (max_fdi_bpp < 30)
-			display_bpc = 8;
-		else if (max_fdi_bpp < 36)
-			display_bpc = 10;
-		DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
-	}
 	/*
 	 * We could just drive the pipe at the highest bpc all the time and
 	 * enable dithering as needed, but that costs bandwidth.  So choose
@@ -3845,8 +3836,20 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
 
 	display_bpc = min(display_bpc, bpc);
 
-	DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d)\n",
-		      bpc, display_bpc);
+	display_bpc = 6;
+
+	if (display_bpc * 3 > max_fdi_bpp) {
+		if (max_fdi_bpp < 24)
+			display_bpc = 6;
+		else if (max_fdi_bpp < 30)
+			display_bpc = 8;
+		else if (max_fdi_bpp < 36)
+			display_bpc = 10;
+		DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
+	}
+
+	DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d) (max_fdi_bpp %d)\n",
+		      bpc, display_bpc, max_fdi_bpp);
 
 	*pipe_bpp = display_bpc * 3;
 
@@ -4737,8 +4740,7 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		max_fdi_bpp = 0;
 		max_lane = lane;
 	} else {
-		u32	fdi_bw;
-
+		u32 fdi_bw, pps;
 		/* [e]DP over FDI requires target mode clock
 		   instead of link clock */
 		if (is_dp)
@@ -4763,9 +4765,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		 * Compute the available FDI bandwidth, use that
 		 * to compute the maximum supported BPP
 		 */
-		fdi_bw = link_bw * max_lane * 19 / 20;
-		max_fdi_bpp = fdi_bw / target_clock;
-		DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
+		fdi_bw = (link_bw * 8) * max_lane;
+		pps = target_clock * 21 / 20;
+
+		max_fdi_bpp = fdi_bw / pps;
+		DRM_DEBUG_KMS("link_bw %d max_lane %d fdi_bw %u pps %u max_fdi_bpp %d\n",
+			      link_bw, max_lane, fdi_bw, pps, max_fdi_bpp);
 	}
 
 	/* [e]DP over FDI requires target mode clock instead of link clock. */
@@ -4809,6 +4814,8 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
 		 */
 		u32 bps = target_clock * pipe_bpp * 21 / 20;
 		lane = bps / (link_bw * 8) + 1;
+		DRM_DEBUG_KMS("target_clock %u pipe_bpp %u bps %u link_bw %u lane %u\n",
+			      target_clock, pipe_bpp, bps, link_bw, lane);
 		if (lane > max_lane) {
 			DRM_ERROR("Not enough lanes available for mode! (want %d have %d)\n",
 				  lane, max_lane);
-- 
1.7.10.4

[PATCH 5/7] drm/i915: Pipe-C only configurations would not get SR

[Keith Packard]

These should probably all look like

	enabled |= (1 << pipe)

so that the intent is clear...

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_pm.c |    2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
index 94aabca..1a84425 100644
--- a/drivers/gpu/drm/i915/intel_pm.c
+++ b/drivers/gpu/drm/i915/intel_pm.c
@@ -1815,7 +1815,7 @@ static void sandybridge_update_wm(struct drm_device *dev)
 		DRM_DEBUG_KMS("FIFO watermarks For pipe C -"
 			      " plane %d, cursor: %d\n",
 			      plane_wm, cursor_wm);
-		enabled |= 3;
+		enabled |= 4;
 	}
 
 	/*
-- 
1.7.10.4

[PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

[Keith Packard]

Doesn't make sense to disable in the other order.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index b099a17..754f10f 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2776,17 +2776,17 @@ static void ironlake_fdi_disable(struct drm_crtc *crtc)
 	u32 reg, temp;
 
 	/* disable CPU FDI tx and PCH FDI rx */
-	reg = FDI_TX_CTL(pipe);
-	temp = I915_READ(reg);
-	I915_WRITE(reg, temp & ~FDI_TX_ENABLE);
-	POSTING_READ(reg);
-
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
 	temp &= ~(0x7 << 16);
 	temp |= (I915_READ(PIPECONF(pipe)) & PIPE_BPC_MASK) << 11;
 	I915_WRITE(reg, temp & ~FDI_RX_ENABLE);
+	POSTING_READ(reg);
+	udelay(100);
 
+	reg = FDI_TX_CTL(pipe);
+	temp = I915_READ(reg);
+	I915_WRITE(reg, temp & ~FDI_TX_ENABLE);
 	POSTING_READ(reg);
 	udelay(100);
 
-- 
1.7.10.4

[PATCH 7/7] drm/i915: Merge FDI RX reg writes during training

[Keith Packard]

Need to turn on the error correction when enabling training or it
might not get enabled in time.

This seems to fix the FDI-B/FDI-C link training problem.

Signed-off-by: Keith Packard <keithp@keithp.com>
---
 drivers/gpu/drm/i915/intel_display.c |   11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c
index 754f10f..1d24d55 100644
--- a/drivers/gpu/drm/i915/intel_display.c
+++ b/drivers/gpu/drm/i915/intel_display.c
@@ -2324,6 +2324,8 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 		temp |= FDI_LINK_TRAIN_NONE | FDI_TX_ENHANCE_FRAME_ENABLE;
 	}
 	I915_WRITE(reg, temp);
+	POSTING_READ(reg);
+	udelay(100);
 
 	reg = FDI_RX_CTL(pipe);
 	temp = I915_READ(reg);
@@ -2334,16 +2336,15 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
 		temp &= ~FDI_LINK_TRAIN_NONE;
 		temp |= FDI_LINK_TRAIN_NONE;
 	}
+	/* IVB wants error correction enabled */
+	if (IS_IVYBRIDGE(dev))
+		temp |= FDI_FS_ERRC_ENABLE | FDI_FE_ERRC_ENABLE;
+
 	I915_WRITE(reg, temp | FDI_RX_ENHANCE_FRAME_ENABLE);
 
 	/* wait one idle pattern time */
 	POSTING_READ(reg);
 	udelay(1000);
-
-	/* IVB wants error correction enabled */
-	if (IS_IVYBRIDGE(dev))
-		I915_WRITE(reg, I915_READ(reg) | FDI_FS_ERRC_ENABLE |
-			   FDI_FE_ERRC_ENABLE);
 }
 
 static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)
-- 
1.7.10.4

Re: [PATCH 1/7] drm/i915: Allow VGA on CRTC 2

[Daniel Vetter]

On Mon, Aug 13, 2012 at 09:34:45PM -0700, Keith Packard wrote:
> This is left over from the old PLL sharing code and isn't useful now
> that PLLs are shared when possible.
> 
> Signed-off-by: Keith Packard <keithp@keithp.com>
Queued for -next, thanks for the patch. I'll hold off a bit on the others
until it's a bit clearer what's going on/wrong.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:

@@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
drm_i915_private *dev_priv)
  */
 static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
                                         unsigned int *pipe_bpp,
-                                        struct drm_display_mode *mode)
+                                        struct drm_display_mode *mode,
+                                        int max_fdi_bpp)

There's some kernel-doc for this function, maybe add a @max_fdi_bpp there?


> @@ -3800,6 +3801,15 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                 display_bpc = 6;
>         }
>
> +       if (display_bpc * 3 > max_fdi_bpp) {
> +               if (max_fdi_bpp < 24)
> +                       display_bpc = 6;
> +               else if (max_fdi_bpp < 30)
> +                       display_bpc = 8;
> +               else if (max_fdi_bpp < 36)
> +                       display_bpc = 10;
> +               DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
> +       }

This chunk is being moved around in a later patch in the series,
merging the two patches in one looks like a good idea?


> +       /* If the other FDI link is using too many lanes, we can't have
> +        * any
> +        */
> +       if (other_intel_crtc->fdi_lanes > 2)
> +               return 0;
> +
> +       /* When both are running, we only get 2 lanes at most
> +        */
> +       return 2;
> +}

I guess this does not cover the case of pipe B using 3 lanes meaning
pipe C can use 1?

> @@ -4672,7 +4731,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>            according to current link config */
>         if (is_cpu_edp) {
>                 intel_edp_link_config(edp_encoder, &lane, &link_bw);
> +               max_fdi_bpp = 0;
> +               max_lane = lane;
>         } else {
> +               u32     fdi_bw;
> +
> +               /* [e]DP over FDI requires target mode clock
> +                  instead of link clock */
> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;
> +

This duplicates the code just that is just a few lines away, instead
how about moving the logic to set target_clock up in front of this
whole if()?

>                 /* FDI is a binary signal running at ~2.7GHz, encoding
>                  * each output octet as 10 bits. The actual frequency
>                  * is stored as a divider into a 100MHz clock, and the
> @@ -4681,6 +4751,18 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                  * is:
>                  */
>                 link_bw = intel_fdi_link_freq(dev) * MHz(100)/KHz(1)/10;
> +
> +               max_lane = 4;
> +               if (IS_IVYBRIDGE(dev))
> +                       max_lane = ivb_fdi_max_lanes(crtc);
> +
> +               /*
> +                * Compute the available FDI bandwidth, use that
> +                * to compute the maximum supported BPP
> +                */
> +               fdi_bw = link_bw * max_lane * 19 / 20;
> +               max_fdi_bpp = fdi_bw / target_clock;
> +               DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
>         }

This chunk is also reworked in a later commit in this series.

-- 
Damien

Re: [PATCH 4/7] drm/i915: Check display_bpc against max_fdi_bpp after display_bpc is set

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> @@ -3845,8 +3836,20 @@ static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>
>         display_bpc = min(display_bpc, bpc);
>
> -       DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d)\n",
> -                     bpc, display_bpc);
> +       display_bpc = 6;

It seems that you are overriding display_bpc unconditionally here,
some left over from debugging?

> +       if (display_bpc * 3 > max_fdi_bpp) {
> +               if (max_fdi_bpp < 24)
> +                       display_bpc = 6;
> +               else if (max_fdi_bpp < 30)
> +                       display_bpc = 8;
> +               else if (max_fdi_bpp < 36)
> +                       display_bpc = 10;
> +               DRM_DEBUG_KMS("Dithering FDI to %dbpc\n", display_bpc);
> +       }
> +
> +       DRM_DEBUG_KMS("setting pipe bpc to %d (max display bpc %d) (max_fdi_bpp %d)\n",
> +                     bpc, display_bpc, max_fdi_bpp);
>
>         *pipe_bpp = display_bpc * 3;

"setting pipe bpc to %d", bpc and  *pipe_bpp = display_bpc, looks like
a bogus debug message to me.

> @@ -4763,9 +4765,12 @@ static int ironlake_crtc_mode_set(struct drm_crtc *crtc,
>                  * Compute the available FDI bandwidth, use that
>                  * to compute the maximum supported BPP
>                  */
> -               fdi_bw = link_bw * max_lane * 19 / 20;
> -               max_fdi_bpp = fdi_bw / target_clock;
> -               DRM_DEBUG_KMS("max lane %d yields max fdi bpp %d\n", max_lane, max_fdi_bpp);
> +               fdi_bw = (link_bw * 8) * max_lane;
> +               pps = target_clock * 21 / 20;
> +
> +               max_fdi_bpp = fdi_bw / pps;
> +               DRM_DEBUG_KMS("link_bw %d max_lane %d fdi_bw %u pps %u max_fdi_bpp %d\n",
> +                             link_bw, max_lane, fdi_bw, pps, max_fdi_bpp);
>         }

While I understood the first computation of max_fdi_bpp in patch 2 of
this series, I have to confess you lost me there. Would you mind
clarifying this?

-- 
Damien

Re: [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Keith Packard]

[-- Attachment #1.1: Type: text/plain, Size: 1597 bytes --]

"Lespiau, Damien" <damien.lespiau@intel.com> writes:

> On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
>
> @@ -3728,7 +3728,8 @@ static inline bool intel_panel_use_ssc(struct
> drm_i915_private *dev_priv)
>   */
>  static bool intel_choose_pipe_bpp_dither(struct drm_crtc *crtc,
>                                          unsigned int *pipe_bpp,
> -                                        struct drm_display_mode *mode)
> +                                        struct drm_display_mode *mode,
> +                                        int max_fdi_bpp)
>
> There's some kernel-doc for this function, maybe add a @max_fdi_bpp
> there?

Will do

> This chunk is being moved around in a later patch in the series,
> merging the two patches in one looks like a good idea?

Or at least move this into its final position in this patch.

> I guess this does not cover the case of pipe B using 3 lanes meaning
> pipe C can use 1?

It didn't look like that was a supported mode from the docs.

> This duplicates the code just that is just a few lines away, instead
> how about moving the logic to set target_clock up in front of this
> whole if()?

It's not the same, it's the inverse -- computing bpp from lanes+clock
clock instead of computing lanes from bpp+clock. But, yeah, it would be
nice to have these merged somehow. I couldn't figure out a good way though.

> This chunk is also reworked in a later commit in this series.

I'll see if I can't avoid that as it's confusing. Thanks for your review!

-- 
keith.packard@intel.com

[-- Attachment #1.2: Type: application/pgp-signature, Size: 827 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

Re: [Intel-gfx] [PATCH 2/7] drm/i915: FDI B/C share 4 lanes on Ivybridge

[Lespiau, Damien]

On Fri, Aug 17, 2012 at 4:00 PM, Keith Packard <keithp@keithp.com> wrote:
>> I guess this does not cover the case of pipe B using 3 lanes meaning
>> pipe C can use 1?
>
> It didn't look like that was a supported mode from the docs.

Ah yes, found it now "FDI B maximum port width is 4 lanes when FDI C
is disabled, 2 lanes when FDI C is enabled."

>> This duplicates the code just that is just a few lines away, instead
>> how about moving the logic to set target_clock up in front of this
>> whole if()?
>
> It's not the same, it's the inverse -- computing bpp from lanes+clock
> clock instead of computing lanes from bpp+clock. But, yeah, it would be
> nice to have these merged somehow. I couldn't figure out a good way though.

I meant the

> +               if (is_dp)
> +                       target_clock = mode->clock;
> +               else
> +                       target_clock = adjusted_mode->clock;

Just after that else block you have

        /* [e]DP over FDI requires target mode clock instead of link clock. */
        if (edp_encoder)
                target_clock = intel_edp_target_clock(edp_encoder, mode);
        else if (is_dp)
                target_clock = mode->clock;
        else
                target_clock = adjusted_mode->clock;

Those look strangely similar to me. The later could be moved up.

-- 
Damien

Re: [Intel-gfx] [PATCH 3/7] drm/i915: Delay between FDI link training tries. Clear FDI_RX_IIR before training

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> Just a bit of cleanup; it appears to have no effect.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |    7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)

Clearing the locking bit in FDI_RX_IIR looks like a good move and
waiting between tries can't hurt, looks good to me.

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [Intel-gfx] [PATCH 5/7] drm/i915: Pipe-C only configurations would not get SR

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> These should probably all look like
>
>         enabled |= (1 << pipe)
>
> so that the intent is clear...
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_pm.c |    2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c
> index 94aabca..1a84425 100644
> --- a/drivers/gpu/drm/i915/intel_pm.c
> +++ b/drivers/gpu/drm/i915/intel_pm.c
> @@ -1815,7 +1815,7 @@ static void sandybridge_update_wm(struct drm_device *dev)
>                 DRM_DEBUG_KMS("FIFO watermarks For pipe C -"
>                               " plane %d, cursor: %d\n",
>                               plane_wm, cursor_wm);
> -               enabled |= 3;
> +               enabled |= 4;
>         }

Looks like a very good catch!

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> Doesn't make sense to disable in the other order.
>
> Signed-off-by: Keith Packard <keithp@keithp.com>
> ---
>  drivers/gpu/drm/i915/intel_display.c |   10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)

I can't see anything in the docs about an order requirement for those.
Not sure why the other way does not make sense. Somehow disabling TX
before RX makes some sense to me (TX enabled without a ready RX looks
weird?, no data should flow as the pipe is shutdown at that point
anyway). Maybe it just does not matter?

Another detail is that disabling the PLLs seem to have an order in the
disabling sequence, TX, then RX.

I.  Disable CPU FDI Transmitter PLL
II. Disable PCH FDI Receiver PLL

-- 
Damien

Re: [Intel-gfx] [PATCH 7/7] drm/i915: Merge FDI RX reg writes during training

[Lespiau, Damien]

On Tue, Aug 14, 2012 at 5:34 AM, Keith Packard <keithp@keithp.com> wrote:
> @@ -2324,6 +2324,8 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>                 temp |= FDI_LINK_TRAIN_NONE | FDI_TX_ENHANCE_FRAME_ENABLE;
>         }
>         I915_WRITE(reg, temp);
> +       POSTING_READ(reg);
> +       udelay(100);

The docs don't mention a delay between writing the TX and RX training
patterns, the POSTING_READ() seems like a good idea.

>         reg = FDI_RX_CTL(pipe);
>         temp = I915_READ(reg);
> @@ -2334,16 +2336,15 @@ static void intel_fdi_normal_train(struct drm_crtc *crtc)
>                 temp &= ~FDI_LINK_TRAIN_NONE;
>                 temp |= FDI_LINK_TRAIN_NONE;
>         }
> +       /* IVB wants error correction enabled */
> +       if (IS_IVYBRIDGE(dev))
> +               temp |= FDI_FS_ERRC_ENABLE | FDI_FE_ERRC_ENABLE;
> +
>         I915_WRITE(reg, temp | FDI_RX_ENHANCE_FRAME_ENABLE);
>
>         /* wait one idle pattern time */
>         POSTING_READ(reg);
>         udelay(1000);
> -
> -       /* IVB wants error correction enabled */
> -       if (IS_IVYBRIDGE(dev))
> -               I915_WRITE(reg, I915_READ(reg) | FDI_FS_ERRC_ENABLE |
> -                          FDI_FE_ERRC_ENABLE);
>  }
>
>  static void cpt_phase_pointer_enable(struct drm_device *dev, int pipe)

Reviewed-by: Damien Lespiau <damien.lespiau@intel.com>

-- 
Damien

Re: [Intel-gfx] [PATCH 6/7] drm/i915: Disable FDI RX before FDI TX

[Keith Packard]

[-- Attachment #1: Type: text/plain, Size: 1044 bytes --]

"Lespiau, Damien" <damien.lespiau@intel.com> writes:

> I can't see anything in the docs about an order requirement for those.

Right, the docs don't say anything, which is a bit disconcerting.

> Not sure why the other way does not make sense. Somehow disabling TX
> before RX makes some sense to me (TX enabled without a ready RX looks
> weird?, no data should flow as the pipe is shutdown at that point
> anyway). Maybe it just does not matter?

And here I figured disabling RX before TX made more sense -- otherwise
the receiver wouldn't be seeing anything. In other areas of the driver,
we're careful to disable receivers before senders (disable CRTC before
PLL, etc).

> Another detail is that disabling the PLLs seem to have an order in the
> disabling sequence, TX, then RX.
>
> I.  Disable CPU FDI Transmitter PLL
> II. Disable PCH FDI Receiver PLL

That ordering doesn't matter as the FDI receiver and transmitter are
both disabled by that point, so they aren't talking at all.

-- 
keith.packard@intel.com

[-- Attachment #2: Type: application/pgp-signature, Size: 827 bytes --]