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@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, Thank You for the patch. Reviewed-by: Khaled Almahallawy <khaled.almahallawy@xxxxxxxxx> _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx https://lists.freedesktop.org/mailman/listinfo/intel-gfx
