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

[Almahallawy, Khaled]

On Thu, 2021-01-21 at 15:15 +0200, Imre Deak wrote:
> 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,

Thank You for the patch.

Reviewed-by: Khaled Almahallawy <khaled.almahallawy at intel.com>