| Message ID | 20250108-drm-dp-msm-add-lttpr-transparent-mode-set-v4-4-918949bc2e3a@linaro.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drm/dp: Rework LTTPR transparent mode handling and add support to msm driver | expand |
On Wed, Jan 08, 2025 at 04:31:46PM +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. > > According to the DisplayPort standard, LTTPRs have two operating > modes: > - non-transparent - it replies to DPCD LTTPR field specific AUX > requests, while passes through all other AUX requests > - transparent - it passes through all AUX requests. > > Switching between this two modes is done by the DPTX by issuing > an AUX write to the DPCD PHY_REPEATER_MODE register. > > 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. > > 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. > > Tested-by: Johan Hovold <johan+linaro@kernel.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 24dd37f1682bf5016bb0efbeb44489061deff060..ad09daa4c8ab5c0eb67890509b94e72820bab870 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -107,6 +107,8 @@ struct msm_dp_display_private { > struct msm_dp_event event_list[DP_EVENT_Q_MAX]; > spinlock_t event_lock; > > + u8 lttpr_caps[DP_LTTPR_COMMON_CAP_SIZE]; As far as I can see these 8 bytes are read and acted upon only within msm_dp_display_lttpr_init() below. Any particular reason why you don't just put them on the stack? Regards, Bjorn > + > bool wide_bus_supported; > > struct msm_dp_audio *audio; > @@ -367,12 +369,27 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d > return 0; > } > > +static void msm_dp_display_lttpr_init(struct msm_dp_display_private *dp) > +{ > + int rc; > + > + if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd, > + dp->lttpr_caps)) > + return; > + > + rc = drm_dp_lttpr_init(dp->aux, drm_dp_lttpr_count(dp->lttpr_caps)); > + if (rc) > + DRM_ERROR("failed to set LTTPRs transparency mode, rc=%d\n", rc); > +} > + > static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp) > { > struct drm_connector *connector = dp->msm_dp_display.connector; > const struct drm_display_info *info = &connector->display_info; > int rc = 0; > > + msm_dp_display_lttpr_init(dp); > + > rc = msm_dp_panel_read_sink_caps(dp->panel, connector); > if (rc) > goto end; > > -- > 2.34.1 >
On 1/8/2025 6:31 AM, 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. > > According to the DisplayPort standard, LTTPRs have two operating > modes: > - non-transparent - it replies to DPCD LTTPR field specific AUX > requests, while passes through all other AUX requests > - transparent - it passes through all AUX requests. > > Switching between this two modes is done by the DPTX by issuing > an AUX write to the DPCD PHY_REPEATER_MODE register. > > 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. > > 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. > > Tested-by: Johan Hovold <johan+linaro@kernel.org> > Reviewed-by: Dmitry Baryshkov <dmitry.baryshkov@linaro.org> > Reviewed-by: Johan Hovold <johan+linaro@kernel.org> > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > --- > drivers/gpu/drm/msm/dp/dp_display.c | 17 +++++++++++++++++ > 1 file changed, 17 insertions(+) > > diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c > index 24dd37f1682bf5016bb0efbeb44489061deff060..ad09daa4c8ab5c0eb67890509b94e72820bab870 100644 > --- a/drivers/gpu/drm/msm/dp/dp_display.c > +++ b/drivers/gpu/drm/msm/dp/dp_display.c > @@ -107,6 +107,8 @@ struct msm_dp_display_private { > struct msm_dp_event event_list[DP_EVENT_Q_MAX]; > spinlock_t event_lock; > > + u8 lttpr_caps[DP_LTTPR_COMMON_CAP_SIZE]; > + The reason downstream stored it panel is to read it first in dp_panel's read_sink_caps and call lttpr_init if drm_dp_lttpr_count() is non-zero. But here it looks like msm_dp_display_lttpr_init() internally handles this for us. So no need to store this? > bool wide_bus_supported; > > struct msm_dp_audio *audio; > @@ -367,12 +369,27 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d > return 0; > } > > +static void msm_dp_display_lttpr_init(struct msm_dp_display_private *dp) > +{ > + int rc; > + > + if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd, > + dp->lttpr_caps)) > + return; > + > + rc = drm_dp_lttpr_init(dp->aux, drm_dp_lttpr_count(dp->lttpr_caps)); > + if (rc) > + DRM_ERROR("failed to set LTTPRs transparency mode, rc=%d\n", rc); > +} > + > static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp) > { > struct drm_connector *connector = dp->msm_dp_display.connector; > const struct drm_display_info *info = &connector->display_info; > int rc = 0; > > + msm_dp_display_lttpr_init(dp); > + Can you pls move this call after msm_dp_panel_read_sink_caps()? If msm_dp_panel_read_sink_caps() fails there is no need to call msm_dp_display_lttpr_init(). > rc = msm_dp_panel_read_sink_caps(dp->panel, connector); > if (rc) > goto end; >
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index 24dd37f1682bf5016bb0efbeb44489061deff060..ad09daa4c8ab5c0eb67890509b94e72820bab870 100644 --- a/drivers/gpu/drm/msm/dp/dp_display.c +++ b/drivers/gpu/drm/msm/dp/dp_display.c @@ -107,6 +107,8 @@ struct msm_dp_display_private { struct msm_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 msm_dp_audio *audio; @@ -367,12 +369,27 @@ static int msm_dp_display_send_hpd_notification(struct msm_dp_display_private *d return 0; } +static void msm_dp_display_lttpr_init(struct msm_dp_display_private *dp) +{ + int rc; + + if (drm_dp_read_lttpr_common_caps(dp->aux, dp->panel->dpcd, + dp->lttpr_caps)) + return; + + rc = drm_dp_lttpr_init(dp->aux, drm_dp_lttpr_count(dp->lttpr_caps)); + if (rc) + DRM_ERROR("failed to set LTTPRs transparency mode, rc=%d\n", rc); +} + static int msm_dp_display_process_hpd_high(struct msm_dp_display_private *dp) { struct drm_connector *connector = dp->msm_dp_display.connector; const struct drm_display_info *info = &connector->display_info; int rc = 0; + msm_dp_display_lttpr_init(dp); + rc = msm_dp_panel_read_sink_caps(dp->panel, connector); if (rc) goto end;