[PATCH 1/5] drm/i915/dp: Don't switch the LTTPR mode on an active link

[Imre Deak]

On Fri, Jul 05, 2024 at 06:16:45PM +0300, Ville Syrjälä wrote:
> On Wed, Jul 03, 2024 at 06:59:33PM +0300, Imre Deak wrote:
> > Switching to transparent mode leads to a loss of link synchronization,
> > so prevent doing this on an active link. This happened at least on an
> > Intel N100 system / DELL UD22 dock, the LTTPR residing either on the
> > host or the dock. To fix the issue, keep the current mode on an active
> > link, adjusting the LTTPR count accordingly (resetting it to 0 in
> > transparent mode).
> > 
> > Fixes: 7b2a4ab8b0ef ("drm/i915: Switch to LTTPR transparent mode link training")
> > Reported-and-tested-by: Gareth Yu <gareth.yu at intel.com>
> > Closes: https://gitlab.freedesktop.org/drm/i915/kernel/-/issues/10902
> > Cc: <stable at vger.kernel.org> # v5.15+
> > Cc: Ville Syrjälä <ville.syrjala at linux.intel.com>
> > Signed-off-by: Imre Deak <imre.deak at intel.com>
> > ---
> >  .../drm/i915/display/intel_dp_link_training.c | 49 +++++++++++++++++--
> >  1 file changed, 45 insertions(+), 4 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 1bc4ef84ff3bc..08a27fe077917 100644
> > --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c
> > @@ -117,10 +117,24 @@ intel_dp_set_lttpr_transparent_mode(struct intel_dp *intel_dp, bool enable)
> >  	return drm_dp_dpcd_write(&intel_dp->aux, DP_PHY_REPEATER_MODE, &val, 1) == 1;
> >  }
> >  
> > -static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > +static bool intel_dp_lttpr_transparent_mode_enabled(struct intel_dp *intel_dp)
> > +{
> > +	return intel_dp->lttpr_common_caps[DP_PHY_REPEATER_MODE -
> > +					   DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV] ==
> > +		DP_PHY_REPEATER_MODE_TRANSPARENT;
> > +}
> > +
> > +/*
> > + * Read the LTTPR common capabilities and switch the LTTPR PHYs to
> > + * non-transparent mode if this is supported. Preserve the
> > + * transparent/non-transparent mode on an active link.
> > + *
> > + * Return the number of detected LTTPRs in non-transparent mode or 0 if the
> > + * LTTPRs are in transparent mode or the detection failed.
> > + */
> > +static int intel_dp_init_lttpr_phys(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> >  {
> >  	int lttpr_count;
> > -	int i;
> >  
> >  	if (!intel_dp_read_lttpr_common_caps(intel_dp, dpcd))
> >  		return 0;
> > @@ -134,6 +148,19 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEI
> >  	if (lttpr_count == 0)
> >  		return 0;
> >  
> > +	/*
> > +	 * Don't change the mode on an active link, to prevent a loss of link
> > +	 * synchronization. See DP Standard v2.0 3.6.7. about the LTTPR
> > +	 * resetting its internal state when the mode is changed from
> > +	 * non-transparent to transparent.
> > +	 */
> > +	if (intel_dp->link_trained) {
> > +		if (lttpr_count < 0 || intel_dp_lttpr_transparent_mode_enabled(intel_dp))
> > +			goto out_reset_lttpr_count;
> 
> I was pondering whether we should flag this for LTTPR reinit
> on the next link training, but looks like we already do that
> unconditionally. So the TODO in intel_dp_start_link_train()
> should perhaps be removed if it's the behaviour we now want?

Yes missed that. With this constraint the mode can be changed only
when the link is not active and that's only guaranteed during link
training. I'll update the TODO comment there.

> However, it looks like we leave link_trained==true when
> using the non-modeset link retrain path. So that will again
> skip the LTTPR mode change, whereas the modeset based path
> will do the mode change. Doesn't really matter I suppose,
> but probably good to keep that change in behaviour in mind
> when we get rid of the non-modeset retrain path for good.

Yes, the non-modeset SST link retrain will happen now without changing
to non-transparent LTTPR mode, didn't think of that. This should matter
only for an enabled link taken over from BIOS if it didn't switch to
non-transparent mode already. Yes, imo it's probably ok to retrain in
transparent mode in that case (until the next modeset) and this won't
matter after switching to a modeset retrain for SST as well. However for
stable it's still better to keep the current behavior, so I could follow
up with a change disabling link_trained in intel_dp_retrain_link()
(along with initing link_trained during HW readout which I also missed).

> Series is
> Reviewed-by: Ville Syrjälä <ville.syrjala at linux.intel.com>

Thanks.

> > +
> > +		return lttpr_count;
> > +	}
> > +
> >  	/*
> >  	 * See DP Standard v2.0 3.6.6.1. about the explicit disabling of
> >  	 * non-transparent mode and the disable->enable non-transparent mode
> > @@ -154,11 +181,25 @@ static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEI
> >  		       "Switching to LTTPR non-transparent LT mode failed, fall-back to transparent mode\n");
> >  
> >  		intel_dp_set_lttpr_transparent_mode(intel_dp, true);
> > -		intel_dp_reset_lttpr_count(intel_dp);
> >  
> > -		return 0;
> > +		goto out_reset_lttpr_count;
> >  	}
> >  
> > +	return lttpr_count;
> > +
> > +out_reset_lttpr_count:
> > +	intel_dp_reset_lttpr_count(intel_dp);
> > +
> > +	return 0;
> > +}
> > +
> > +static int intel_dp_init_lttpr(struct intel_dp *intel_dp, const u8 dpcd[DP_RECEIVER_CAP_SIZE])
> > +{
> > +	int lttpr_count;
> > +	int i;
> > +
> > +	lttpr_count = intel_dp_init_lttpr_phys(intel_dp, dpcd);
> > +
> >  	for (i = 0; i < lttpr_count; i++)
> >  		intel_dp_read_lttpr_phy_caps(intel_dp, dpcd, DP_PHY_LTTPR(i));
> >  
> > -- 
> > 2.43.3
> 
> -- 
> Ville Syrjälä
> Intel