Johan Hovold
2024-Dec-11 14:42 UTC
[PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode
On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote:> +/** > + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode > + * @aux: DisplayPort AUX channel > + * @enable: Enable or disable transparent mode > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable) > +{ > + u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT : > + DP_PHY_REPEATER_MODE_NON_TRANSPARENT; > + int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val); > + > + return ret == 1 ? 0 : ret;This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make sure it never returns 0 (for short transfers).> +} > +EXPORT_SYMBOL(drm_dp_lttpr_set_transparent_mode);This appears to be what the driver currently uses, but why not EXPORT_SYMBOL_GPL?> + > +/** > + * drm_dp_lttpr_init - init LTTPR transparency mode according to DP standard > + * > + * @aux: DisplayPort AUX channel > + * @lttpr_count: Number of LTTPRs > + * > + * Returns 0 on success or a negative error code on failure. > + */ > +int drm_dp_lttpr_init(struct drm_dp_aux *aux, int lttpr_count) > +{ > + if (!lttpr_count) > + return 0; > + > + /* > + * See DP Standard v2.0 3.6.6.1 about the explicit disabling of > + * non-transparent mode and the disable->enable non-transparent mode > + * sequence. > + */ > + drm_dp_lttpr_set_transparent_mode(aux, true);Error handling?> + > + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false))No need to check lttpr_count again here.> + return 0;I'd check for errors instead of success here and do the rollback before returning -EINVAL.> + > + /* > + * Roll-back to tranparent mode if setting non-tranparent mode failed or > + * the number of LTTPRs is invalid > + */ > + drm_dp_lttpr_set_transparent_mode(aux, true); > + > + return -EINVAL;And return 0 explicitly here.> +} > +EXPORT_SYMBOL(drm_dp_lttpr_init);In any case this works well and is needed for external display on the Lenovo ThinkPad T14s, while not breaking the X13s which does not need it: Tested-by: Johan Hovold <johan+linaro at kernel.org> Johan
Dmitry Baryshkov
2024-Dec-11 18:32 UTC
[PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode
On Wed, Dec 11, 2024 at 03:42:27PM +0100, Johan Hovold wrote:> On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote: > > > +/** > > + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode > > + * @aux: DisplayPort AUX channel > > + * @enable: Enable or disable transparent mode > > + * > > + * Returns 0 on success or a negative error code on failure. > > + */ > > +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable) > > +{ > > + u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT : > > + DP_PHY_REPEATER_MODE_NON_TRANSPARENT; > > + int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val); > > + > > + return ret == 1 ? 0 : ret; > > This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make > sure it never returns 0 (for short transfers). > > > +} > > +EXPORT_SYMBOL(drm_dp_lttpr_set_transparent_mode); > > This appears to be what the driver currently uses, but why not > EXPORT_SYMBOL_GPL?$ git grep EXPORT_SYMBOL drivers/gpu/drm/*.c | wc -l 962 $ git grep EXPORT_SYMBOL_GPL drivers/gpu/drm/*.c | wc -l 93 -- With best wishes Dmitry
Dmitry Baryshkov
2024-Dec-11 19:00 UTC
[PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode
On Wed, Dec 11, 2024 at 03:42:27PM +0100, Johan Hovold wrote:> On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote: > > > +/** > > + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode > > + * @aux: DisplayPort AUX channel > > + * @enable: Enable or disable transparent mode > > + * > > + * Returns 0 on success or a negative error code on failure. > > + */ > > +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable) > > +{ > > + u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT : > > + DP_PHY_REPEATER_MODE_NON_TRANSPARENT; > > + int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val); > > + > > + return ret == 1 ? 0 : ret; > > This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make > sure it never returns 0 (for short transfers).Indeed. It got me a while to check that drm_dp_dpcd_writeb() -> drm_dp_mst_dpcd_write() -> drm_dp_send_dpcd_write() -> drm_dp_mst_wait_tx_reply() never returns '0'. I'd prefer an explicit if (ret < 0) return ret; return (ret == 1) ? 0 : -EIO;> > > +} > > +EXPORT_SYMBOL(drm_dp_lttpr_set_transparent_mode); >-- With best wishes Dmitry
neil.armstrong at linaro.org
2024-Dec-12 08:31 UTC
[PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode
On 11/12/2024 15:42, Johan Hovold wrote:> On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote: > >> +/** >> + * drm_dp_lttpr_set_transparent_mode - set the LTTPR in transparent mode >> + * @aux: DisplayPort AUX channel >> + * @enable: Enable or disable transparent mode >> + * >> + * Returns 0 on success or a negative error code on failure. >> + */ >> +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable) >> +{ >> + u8 val = enable ? DP_PHY_REPEATER_MODE_TRANSPARENT : >> + DP_PHY_REPEATER_MODE_NON_TRANSPARENT; >> + int ret = drm_dp_dpcd_writeb(aux, DP_PHY_REPEATER_MODE, val); >> + >> + return ret == 1 ? 0 : ret; > > This looks correct, but I had to go look at drm_dp_dpcd_writeb() to make > sure it never returns 0 (for short transfers). > >> +} >> +EXPORT_SYMBOL(drm_dp_lttpr_set_transparent_mode); > > This appears to be what the driver currently uses, but why not > EXPORT_SYMBOL_GPL?drivers/gpu/drm/display/drm_dp_helper.c is not GPL licenced, so this is the right macro to use. Neil> >> + >> +/** >> + * drm_dp_lttpr_init - init LTTPR transparency mode according to DP standard >> + * >> + * @aux: DisplayPort AUX channel >> + * @lttpr_count: Number of LTTPRs >> + * >> + * Returns 0 on success or a negative error code on failure. >> + */ >> +int drm_dp_lttpr_init(struct drm_dp_aux *aux, int lttpr_count) >> +{ >> + if (!lttpr_count) >> + return 0; >> + >> + /* >> + * See DP Standard v2.0 3.6.6.1 about the explicit disabling of >> + * non-transparent mode and the disable->enable non-transparent mode >> + * sequence. >> + */ >> + drm_dp_lttpr_set_transparent_mode(aux, true); > > Error handling? > >> + >> + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) > > No need to check lttpr_count again here. > >> + return 0; > > I'd check for errors instead of success here and do the rollback before > returning -EINVAL. > >> + >> + /* >> + * Roll-back to tranparent mode if setting non-tranparent mode failed or >> + * the number of LTTPRs is invalid >> + */ >> + drm_dp_lttpr_set_transparent_mode(aux, true); >> + >> + return -EINVAL; > > And return 0 explicitly here. > >> +} >> +EXPORT_SYMBOL(drm_dp_lttpr_init); > > In any case this works well and is needed for external display on the > Lenovo ThinkPad T14s, while not breaking the X13s which does not need > it: > > Tested-by: Johan Hovold <johan+linaro at kernel.org> > > Johan >