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
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;
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
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)
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 clockinstead of link clock */if (is_dp)target_clock = mode->clock;elsetarget_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.
"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!
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;elsetarget_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.
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);
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
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);
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?
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; }
/*
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
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);
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
"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.
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)
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
dri-devel@lists.freedesktop.org
-
Daniel Vetter -
Keith Packard -
Lespiau, Damien