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@xxxxxxxxx> > > --- > > .../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, _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
