[Intel-gfx] [PATCH] drm/i915/dp: Prevent setting the LTTPR LT mode if no LTTPRs are detected

[Imre Deak]

On Tue, Jan 19, 2021 at 08:47:25AM +0200, Almahallawy, Khaled wrote:
> On Mon, 2021-01-18 at 20:31 +0200, Imre Deak wrote:
> > Atm, the driver programs explicitly the default transparent link
> > training mode (0x55) to DP_PHY_REPEATER_MODE even if no LTTPRs are
> > detected.
> >
> > This conforms to the spec (3.6.6.1):
> > "DP upstream devices that do not enable the Non-transparent mode of
> >  LTTPRs shall program the PHY_REPEATER_MODE register (DPCD Address
> >  F0003h) to 55h (default) prior to link training"
> >
> > however writing the default value to this DPCD register seems to
> > cause
> > occasional link training errors at least for a DELL WD19TB TBT dock,
> > when
> > no LTTPRs are detected.
> 
> I think this patch is more aligned with: DP v2.0 SCR on 8b/10b DPTX and
> LTTPR Requirements Update to Section 3.6
> 
> The SCR made it clear that we only need to program PHY_REPEATER_MODE to
> transparent mode if we detect LTTPR.

Yes, the updated version is clearer in this. In any case I don't see any
reason now to set the default mode if there's no LTTPR on the link.

> Quoting from SCR:
> “A DPTX supporting 3.2-ms AUX Reply Timeout shall issue AUX read
> transaction to LTTPR DPCD Capability and ID Field at DPCD F0000h ~
> F0007 (refer to Section 3.6.4.1) as the first AUX transaction upon HPD
> signal assertion detection (1) to determine whether LTTPR’s are present
> in the link between itself and the downstream DPRX and (2) to put the
> LTTPR’s, if present, in LTTPR Transparent Mode.”
> 
> Also section 3.6.6 title is updated as the following “Section 3.6.6
> Link Training in LTTPR Non-transparent Mode”. This reflects it only
> relevant after we detect LTTPR.
> 
> However it is still interesting that Dell Dock failed after setting
> LTTPR to transparent mode.

Yes, sinks should handle writing to this DPCD register regardless if
there's any LTTPR on the link or not.

> Thank You for your effort to enable LTTPR.
> Khaled
> >
> > Writing to DP_PHY_REPEATER_MODE will also cause an unnecessary
> > timeout
> > on systems without any LTTPR.
> >
> > To fix the above two issues let's assume that setting the default
> > mode
> > is redundant when no LTTPRs are detected. Keep the existing behavior
> > and
> > program the default mode if more than 8 LTTPRs are detected or in
> > case
> > the read from DP_PHY_REPEATER_CNT returns an invalid value.
> >
> > References: https://gitlab.freedesktop.org/drm/intel/-/issues/2801
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 36 ++++++++---------
> > --
> >  1 file changed, 15 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > index d8c6d7054d11..fad9e9874c7b 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -34,18 +34,6 @@ intel_dp_dump_link_status(const u8
> > link_status[DP_LINK_STATUS_SIZE])
> >        link_status[3], link_status[4], link_status[5]);
> >  }
> >
> > -static int intel_dp_lttpr_count(struct intel_dp *intel_dp)
> > -{
> > -int count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps);
> > -
> > -/*
> > - * Pretend no LTTPRs in case of LTTPR detection error, or
> > - * if too many (>8) LTTPRs are detected. This translates to
> > link
> > - * training in transparent mode.
> > - */
> > -return count <= 0 ? 0 : count;
> > -}
> > -
> >  static void intel_dp_reset_lttpr_count(struct intel_dp *intel_dp)
> >  {
> >  intel_dp->lttpr_common_caps[DP_PHY_REPEATER_CNT -
> > @@ -142,6 +130,17 @@ int intel_dp_lttpr_init(struct intel_dp
> > *intel_dp)
> >  return 0;
> >
> >  ret = intel_dp_read_lttpr_common_caps(intel_dp);
> > +if (!ret)
> > +return 0;
> > +
> > +lttpr_count = drm_dp_lttpr_count(intel_dp->lttpr_common_caps);
> > +/*
> > + * Prevent setting LTTPR transparent mode explicitly if no
> > LTTPRs are
> > + * detected as this breaks link training at least on the Dell
> > WD19TB
> > + * dock.
> > + */
> > +if (lttpr_count == 0)
> > +return 0;
> >
> >  /*
> >   * See DP Standard v2.0 3.6.6.1. about the explicit disabling
> > of
> > @@ -150,17 +149,12 @@ int intel_dp_lttpr_init(struct intel_dp
> > *intel_dp)
> >   */
> >  intel_dp_set_lttpr_transparent_mode(intel_dp, true);
> >
> > -if (!ret)
> > -return 0;
> > -
> > -lttpr_count = intel_dp_lttpr_count(intel_dp);
> > -
> >  /*
> >   * In case of unsupported number of LTTPRs or failing to switch
> > to
> >   * non-transparent mode fall-back to transparent link training
> > mode,
> >   * still taking into account any LTTPR common lane- rate/count
> > limits.
> >   */
> > -if (lttpr_count == 0)
> > +if (lttpr_count < 0)
> >  return 0;
> >
> >  if (!intel_dp_set_lttpr_transparent_mode(intel_dp, false)) {
> > @@ -222,11 +216,11 @@ intel_dp_phy_is_downstream_of_source(struct
> > intel_dp *intel_dp,
> >       enum drm_dp_phy dp_phy)
> >  {
> >  struct drm_i915_private *i915 = dp_to_i915(intel_dp);
> > -int lttpr_count = intel_dp_lttpr_count(intel_dp);
> > +int lttpr_count = drm_dp_lttpr_count(intel_dp-
> > >lttpr_common_caps);
> >
> > -drm_WARN_ON_ONCE(&i915->drm, lttpr_count == 0 && dp_phy !=
> > DP_PHY_DPRX);
> > +drm_WARN_ON_ONCE(&i915->drm, lttpr_count <= 0 && dp_phy !=
> > DP_PHY_DPRX);
> >
> > -return lttpr_count == 0 || dp_phy == DP_PHY_LTTPR(lttpr_count -
> > 1);
> > +return lttpr_count <= 0 || dp_phy == DP_PHY_LTTPR(lttpr_count -
> > 1);
> >  }
> >
> >  static u8 intel_dp_phy_voltage_max(struct intel_dp *intel_dp,