Re: [PATCH] drm/i915: make IVB FDI training match spec v2

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux