On Mon, Apr 08, 2013 at 05:50:07PM -0300, Paulo Zanoni wrote: > Hi > > 2013/3/28 Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx>: > > The existing code was trying different vswing and preemphasis settings > > in the wrong place, and wasn't trying them enough. So add a loop to > > walk through them, properly disabling FDI TX and RX in between if a > > failure is detected. > > > > v2: remove unneeded reg writes, add delays around bit lock checks (Jesse) > > > > Signed-off-by: Jesse Barnes <jbarnes@xxxxxxxxxxxxxxxx> Apparently this patch can curb fdi link train fail on some machines: https://bugs.freedesktop.org/show_bug.cgi?id=51983 Althouhg there's still other fail going on. Can you pls update your patch to Paulo's review and resend? -Daniel > > --- > > drivers/gpu/drm/i915/intel_display.c | 141 +++++++++++++++++----------------- > > 1 file changed, 71 insertions(+), 70 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > > index 5e8b91f..a57f086 100644 > > --- a/drivers/gpu/drm/i915/intel_display.c > > +++ b/drivers/gpu/drm/i915/intel_display.c > > @@ -2709,7 +2709,7 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc) > > struct drm_i915_private *dev_priv = dev->dev_private; > > struct intel_crtc *intel_crtc = to_intel_crtc(crtc); > > int pipe = intel_crtc->pipe; > > - u32 reg, temp, i; > > + u32 reg, temp, i, j; > > > > /* Train 1: umask FDI RX Interrupt symbol_lock and bit_lock bit > > for train result */ > > @@ -2725,97 +2725,98 @@ static void ivb_manual_fdi_link_train(struct drm_crtc *crtc) > > DRM_DEBUG_KMS("FDI_RX_IIR before link train 0x%x\n", > > I915_READ(FDI_RX_IIR(pipe))); > > > > - /* enable CPU FDI TX and PCH FDI RX */ > > - reg = FDI_TX_CTL(pipe); > > - temp = I915_READ(reg); > > - temp &= ~(7 << 19); > > - temp |= (intel_crtc->fdi_lanes - 1) << 19; > > - temp &= ~(FDI_LINK_TRAIN_AUTO | FDI_LINK_TRAIN_NONE_IVB); > > - temp |= FDI_LINK_TRAIN_PATTERN_1_IVB; > > - temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK; > > - temp |= FDI_LINK_TRAIN_400MV_0DB_SNB_B; > > - temp |= FDI_COMPOSITE_SYNC; > > - I915_WRITE(reg, temp | FDI_TX_ENABLE); > > - > > - I915_WRITE(FDI_RX_MISC(pipe), > > - FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90); > > - > > - reg = FDI_RX_CTL(pipe); > > - temp = I915_READ(reg); > > - temp &= ~FDI_LINK_TRAIN_AUTO; > > - temp &= ~FDI_LINK_TRAIN_PATTERN_MASK_CPT; > > - temp |= FDI_LINK_TRAIN_PATTERN_1_CPT; > > - temp |= FDI_COMPOSITE_SYNC; > > - I915_WRITE(reg, temp | FDI_RX_ENABLE); > > + /* Try each vswing and preemphasis setting twice before moving on */ > > + for (j = 0; j < ARRAY_SIZE(snb_b_fdi_train_param) * 2; j++) { > > + /* disable first in case we need to retry */ > > + reg = FDI_TX_CTL(pipe); > > + temp = I915_READ(reg); > > + temp &= ~FDI_TX_ENABLE; > > + I915_WRITE(reg, temp); > > FDI TX CTL bit 10: when disabling FDI, clear the FDI Transmitter Auto > Train Enable bit in the same write as the FDI Tx enable is cleared, > and clear the FDI Receiver Auto Train Enable bit in the same write as > the FDI Rx enable is cleared. > > > > > > - POSTING_READ(reg); > > - udelay(150); > > + reg = FDI_RX_CTL(pipe); > > + temp = I915_READ(reg); > > + temp &= ~FDI_RX_ENABLE; > > + I915_WRITE(reg, temp); > > > > - for (i = 0; i < 4; i++) { > > + /* enable CPU FDI TX and PCH FDI RX */ > > reg = FDI_TX_CTL(pipe); > > temp = I915_READ(reg); > > + temp &= ~(7 << 19); > > + temp |= (intel_crtc->fdi_lanes - 1) << 19; > > + temp &= ~(FDI_LINK_TRAIN_AUTO | FDI_LINK_TRAIN_NONE_IVB); > > So I guess we could move this to the "disable chunk" above. > > > > + temp |= FDI_LINK_TRAIN_PATTERN_1_IVB; > > temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK; > > - temp |= snb_b_fdi_train_param[i]; > > - I915_WRITE(reg, temp); > > + temp |= snb_b_fdi_train_param[j/2]; > > + temp |= FDI_COMPOSITE_SYNC; > > + I915_WRITE(reg, temp | FDI_TX_ENABLE); > > > > - POSTING_READ(reg); > > - udelay(500); > > + I915_WRITE(FDI_RX_MISC(pipe), > > + FDI_RX_TP1_TO_TP2_48 | FDI_RX_FDI_DELAY_90); > > > > - reg = FDI_RX_IIR(pipe); > > + reg = FDI_RX_CTL(pipe); > > temp = I915_READ(reg); > > - DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp); > > - > > - if (temp & FDI_RX_BIT_LOCK || > > - (I915_READ(reg) & FDI_RX_BIT_LOCK)) { > > - I915_WRITE(reg, temp | FDI_RX_BIT_LOCK); > > - DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", i); > > - break; > > - } > > - } > > - if (i == 4) > > - DRM_ERROR("FDI train 1 fail!\n"); > > + temp &= ~FDI_LINK_TRAIN_AUTO; > > And we could also move this to the upper chunk. > > > > + temp &= ~FDI_LINK_TRAIN_PATTERN_MASK_CPT; > > + temp |= FDI_LINK_TRAIN_PATTERN_1_CPT; > > + temp |= FDI_COMPOSITE_SYNC; > > + I915_WRITE(reg, temp | FDI_RX_ENABLE); > > > > - /* Train 2 */ > > - reg = FDI_TX_CTL(pipe); > > - temp = I915_READ(reg); > > - temp &= ~FDI_LINK_TRAIN_NONE_IVB; > > - temp |= FDI_LINK_TRAIN_PATTERN_2_IVB; > > - temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK; > > - temp |= FDI_LINK_TRAIN_400MV_0DB_SNB_B; > > - I915_WRITE(reg, temp); > > + POSTING_READ(reg); > > + udelay(150); > > 150us? Spec says "0.5uS" here. > > > > > > - reg = FDI_RX_CTL(pipe); > > - temp = I915_READ(reg); > > - temp &= ~FDI_LINK_TRAIN_PATTERN_MASK_CPT; > > - temp |= FDI_LINK_TRAIN_PATTERN_2_CPT; > > - I915_WRITE(reg, temp); > > + for (i = 0; i < 4; i++) { > > + reg = FDI_RX_IIR(pipe); > > + temp = I915_READ(reg); > > + DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp); > > > > - POSTING_READ(reg); > > - udelay(150); > > + if (temp & FDI_RX_BIT_LOCK || > > + (I915_READ(reg) & FDI_RX_BIT_LOCK)) { > > + I915_WRITE(reg, temp | FDI_RX_BIT_LOCK); > > + DRM_DEBUG_KMS("FDI train 1 done, level %i.\n", > > + i); > > + break; > > + } > > + udelay(50); > > And besides the 150us above, here we have more 4x50us. So possible > 350us when we should wait "0.5uS". > > > > + } > > + if (i == 4) { > > + DRM_DEBUG_KMS("FDI train 1 fail on vswing %d\n", j / 2); > > + continue; > > + } > > > > - for (i = 0; i < 4; i++) { > > + /* Train 2 */ > > reg = FDI_TX_CTL(pipe); > > temp = I915_READ(reg); > > - temp &= ~FDI_LINK_TRAIN_VOL_EMP_MASK; > > - temp |= snb_b_fdi_train_param[i]; > > + temp &= ~FDI_LINK_TRAIN_NONE_IVB; > > + temp |= FDI_LINK_TRAIN_PATTERN_2_IVB; > > + I915_WRITE(reg, temp); > > + > > + reg = FDI_RX_CTL(pipe); > > + temp = I915_READ(reg); > > + temp &= ~FDI_LINK_TRAIN_PATTERN_MASK_CPT; > > + temp |= FDI_LINK_TRAIN_PATTERN_2_CPT; > > I915_WRITE(reg, temp); > > > > POSTING_READ(reg); > > - udelay(500); > > + udelay(150); > > And for the second pattern, spec says we need to wait "1.5uS", and you > have 150us here + 4x50us below. > > > > > > - reg = FDI_RX_IIR(pipe); > > - temp = I915_READ(reg); > > - DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp); > > + for (i = 0; i < 4; i++) { > > + reg = FDI_RX_IIR(pipe); > > + temp = I915_READ(reg); > > + DRM_DEBUG_KMS("FDI_RX_IIR 0x%x\n", temp); > > > > - if (temp & FDI_RX_SYMBOL_LOCK) { > > - I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK); > > - DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", i); > > - break; > > + if (temp & FDI_RX_SYMBOL_LOCK) { > > Here you just check for "temp & FDI_RX_SYMBOL_LOCK", but on the "Train > 1" check you also do a new "I915_READ(reg) & FDI_RX_SYMBOL_LOCK". Both > checks could be identical :) > > > > + I915_WRITE(reg, temp | FDI_RX_SYMBOL_LOCK); > > + DRM_DEBUG_KMS("FDI train 2 done, level %i.\n", > > + i); > > + goto train_done; > > + } > > + udelay(50); > > } > > + if (i == 4) > > + DRM_DEBUG_KMS("FDI train 2 fail on vswing %d\n", j / 2); > > } > > - if (i == 4) > > - DRM_ERROR("FDI train 2 fail!\n"); > > > > +train_done: > > DRM_DEBUG_KMS("FDI train done.\n"); > > } > > > > Besides the comments above, everything else looks correct. > > It looks like we should also call intel_fdi_normal_train() way earlier > (AKA just after dev_priv->display.fdi_link_train). This should be done > as a separate patch, of course :) > > > -- > > 1.7.9.5 > > > > _______________________________________________ > > Intel-gfx mailing list > > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > > http://lists.freedesktop.org/mailman/listinfo/intel-gfx > > > > -- > Paulo Zanoni > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx -- Daniel Vetter Software Engineer, Intel Corporation +41 (0) 79 365 57 48 - http://blog.ffwll.ch _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx
