Hi
2013/3/28 Jesse Barnes <jbarnes at virtuousgeek.org>:
> 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 at virtuousgeek.org>
> ---
> 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Paulo Zanoni
