| Message ID | 20241211-drm-dp-msm-add-lttpr-transparent-mode-set-v2-4-d5906ed38b28@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/dp: Rework LTTPR transparent mode handling and add support to msm driver | expand |
On Wed, Dec 11, 2024 at 03:04:15PM +0200, Abel Vesa wrote: > +static void msm_dp_display_lttpr_init(struct msm_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); I was gonna say shouldn't you handle errors here, but that explains the non-negative check I commented on the first patch in the series. This looks error prone, but I think you should at least update the kernel doc comment to drm_dp_lttpr_init() in the first patch so that it's clear that you pass in the number of LTTPRs *or* an errno. > + > + drm_dp_lttpr_init(dp->aux, lttpr_count); > +} > + > 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); It looks like you ignore errors on purpose so I guess that's fine. > + > rc = msm_dp_panel_read_sink_caps(dp->panel, connector); > if (rc) > goto end; Either way, this is needed for external display on my x1e80100 machines, while not breaking the X13s: Tested-by: Johan Hovold <johan+linaro@kernel.org> Johan
On 24-12-11 15:56:53, Johan Hovold wrote: > On Wed, Dec 11, 2024 at 03:04:15PM +0200, Abel Vesa wrote: > > > +static void msm_dp_display_lttpr_init(struct msm_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); > > I was gonna say shouldn't you handle errors here, but that explains the > non-negative check I commented on the first patch in the series. So lttpr_count is a bit weird. It's either between 0 and 8, or -ERANGE if more than 8 LTTPRs are found, or -EINVAL if for some reason the DP_PHY_REPEATER_CNT register contains an invalid value. (see drm_dp_lttpr_count()) Now, I think I should just drop the lttr_count local variable here entirely. > > This looks error prone, but I think you should at least update the > kernel doc comment to drm_dp_lttpr_init() in the first patch so that > it's clear that you pass in the number of LTTPRs *or* an errno. Yes, I'll do that. Will mention all possible values and what they mean. And will probably point to the drm_dp_lttpr_count() as well, just to be safe. > > > + > > + drm_dp_lttpr_init(dp->aux, lttpr_count); > > +} > > + > > 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); > > It looks like you ignore errors on purpose so I guess that's fine. Maybe I should at least throw an error, just like the i915 does. Will do that. > > > + > > rc = msm_dp_panel_read_sink_caps(dp->panel, connector); > > if (rc) > > goto end; > > Either way, this is needed for external display on my x1e80100 machines, > while not breaking the X13s: > > Tested-by: Johan Hovold <johan+linaro@kernel.org> > > Johan Thanks for reviewing and testing, Abel
diff --git a/drivers/gpu/drm/msm/dp/dp_display.c b/drivers/gpu/drm/msm/dp/dp_display.c index aff51bb973ebe0835c96420d16547ebae0c6c0f2..a8d5563538bbcd83cf88a159dc86080e2c897fe1 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 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); + + drm_dp_lttpr_init(dp->aux, lttpr_count); +} + 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;
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. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/gpu/drm/msm/dp/dp_display.c | 17 +++++++++++++++++ 1 file changed, 17 insertions(+)