On Wed, Dec 11, 2024 at 11:08:16AM +0200, Abel Vesa wrote:
> On 24-10-31 18:54:25, Dmitry Baryshkov wrote:
> > On Thu, Oct 31, 2024 at 05:12:48PM +0200, Abel Vesa wrote:
> > > Link Training Tunable PHY Repeaters (LTTPRs) are defined in DisplayPort
> > > 1.4a specification. As the name suggests, these PHY repeaters are
> > > capable of adjusting their output for link training purposes.
> > >
> > > The msm DP driver is currently lacking any handling of LTTPRs.
> > > This means that if at least one LTTPR is found between DPTX and DPRX,
> > > the link training would fail if that LTTPR was not already configured
> > > in transparent mode.
> >
> > It might be nice to mention what is the transparent mode, especially for
> > those who do not have the standard at hand.
>
> Sorry for the late reply.
>
> Will do in the next version.
>
> >
> > > The section 3.6.6.1 from the DisplayPort v2.0 specification mandates
> > > that before link training with the LTTPR is started, the DPTX may place
> > > the LTTPR in non-transparent mode by first switching to transparent mode
> > > and then to non-transparent mode. This operation seems to be needed only
> > > on first link training and doesn't need to be done again until device is
> > > unplugged.
> > >
> > > It has been observed on a few X Elite-based platforms which have
> > > such LTTPRs in their board design that the DPTX needs to follow the
> > > procedure described above in order for the link training to be successful.
> > >
> > > So add support for reading the LTTPR DPCD caps to figure out the number
> > > of such LTTPRs first. Then, for platforms (or Type-C dongles) that have
> > > at least one such an LTTPR, set its operation mode to transparent mode
> > > first and then to non-transparent, just like the mentioned section of
> > > the specification mandates.
> > >
> > > Signed-off-by: Abel Vesa <abel.vesa@xxxxxxxxxx>
> > > ---
> > > drivers/gpu/drm/msm/dp/dp_display.c | 25 +++++++++++++++++++++++++
> > > 1 file changed, 25 insertions(+)
> > >
> > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c
> > > index f01980b0888a40b719d3958cb96c6341feada077..5d3d318d7b87ce3bf567d8b7435931d8e087f713 100644
> > > --- a/drivers/gpu/drm/msm/dp/dp_display.c
> > > +++ b/drivers/gpu/drm/msm/dp/dp_display.c
> > > @@ -107,6 +107,8 @@ struct dp_display_private {
> > > struct dp_event event_list[DP_EVENT_Q_MAX];
> > > spinlock_t event_lock;
> > >
> > > + u8 lttpr_caps[DP_LTTPR_COMMON_CAP_SIZE];
> > > +
> > > bool wide_bus_supported;
> > >
> > > struct dp_audio *audio;
> > > @@ -367,12 +369,35 @@ static int dp_display_send_hpd_notification(struct dp_display_private *dp,
> > > return 0;
> > > }
> > >
> > > +static void dp_display_lttpr_init(struct dp_display_private *dp)
> > > +{
> > > + int lttpr_count;
> > > +
> > > + if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd,
> > > + dp->lttpr_caps))
> > > + return;
> > > +
> > > + lttpr_count = drm_dp_lttpr_count(dp->lttpr_caps);
> > > +
> > > + if (lttpr_count) {
> > > + drm_dp_lttpr_set_transparent_mode(dp->aux, true);
> > > +
> > > + if (lttpr_count > 0) {
> > > + if (drm_dp_lttpr_set_transparent_mode(dp->aux, false) != 1)
> > > + drm_dp_lttpr_set_transparent_mode(dp->aux, true);
> > > + }
> > > + }
> > > +}
> > > +
> > > static int dp_display_process_hpd_high(struct dp_display_private *dp)
> > > {
> > > struct drm_connector *connector = dp->dp_display.connector;
> > > const struct drm_display_info *info = &connector->display_info;
> > > int rc = 0;
> > >
> > > + if (!dp->dp_display.is_edp)
> > > + dp_display_lttpr_init(dp);
> >
> > Why is it limited to non-eDP cases only.
>
> In case of eDP, I don't think that there will ever by a case that will
> need an LTTPR in between the eDP PHY and the actual panel. It just
> doesn't make sense.
>
> IIUC, the LTTPRs, since are Training Tunnable capable, they help when
> the physical link between the PHY and the sink can differ based on
> different dongles and cables. This is obviously not applicable to eDP.
I think I just have a different paradigm: if the driver explicitly skips
calling a function in some codepath, I assume that the usecase it broken
or expected not to work (e.g. I read your patch like: LTTPR is expected
not to work in eDP). If you would prefer to keep two separate code
paths, please add a comment like 'we don't expect LTTPRs in eDP
usecase`.
> > > +
> > > rc = dp_panel_read_sink_caps(dp->panel, connector);
> > > if (rc)
> > > goto end;
> > >
> > > --
> > > 2.34.1
> > >
> >
> > --
> > With best wishes
> > Dmitry
--
With best wishes
Dmitry
