| Message ID | 20241211-drm-dp-msm-add-lttpr-transparent-mode-set-v2-1-d5906ed38b28@linaro.org |
|---|---|
| State | Superseded |
| 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: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@kernel.org> Johan
On Wed, 11 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > 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 It's even more tilted under display/. BR, Jani.
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@kernel.org> > > Johan >
On Thu, Dec 12, 2024 at 09:31:09AM +0100, neil.armstrong@linaro.org wrote: > On 11/12/2024 15:42, Johan Hovold wrote: > >> +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. Wow. I should have checked the header. Thanks for explaining. Johan
On 24-12-11 21:22:00, Dmitry Baryshkov wrote: > On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote: > > 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. > > > > Add a generic helper that allows switching between these modes. > > > > Also add a generic wrapper for the helper that handles the explicit > > disabling of non-transparent mode and its disable->enable sequence > > mentioned in the DP Standard v2.0 section 3.6.6.1. Do this in order > > to move this handling out of the vendor specific driver implementation > > into the generic framework. > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > --- > > drivers/gpu/drm/display/drm_dp_helper.c | 50 +++++++++++++++++++++++++++++++++ > > include/drm/display/drm_dp_helper.h | 2 ++ > > 2 files changed, 52 insertions(+) > > > > > > +/** > > + * 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); > > + > > + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) > > + return 0; > > + > > + /* > > + * 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); > > This means that if lttpr_count < 0, then there will be two requests to > set LTTPRs to a transparent mode. Is that expected? Yes, exactly. If counting the LTTPRs (see drm_dp_lttpr_count) results in an invalid number (e.g. more than 8), then according to the DP standard, we need to roll back to transparent mode. Do you think I need to re-word the comment above more to make it more clearer? > > > + > > + return -EINVAL; > > +} > > +EXPORT_SYMBOL(drm_dp_lttpr_init); > > + > > -- > With best wishes > Dmitry Thanks for reviewing, Abel
On 24-12-11 15:42:27, 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). Will follow Dmitry's proposal here. if (ret < 0) return ret; return (ret == 1) ? 0 : -EIO; > > > +} > > +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? Yes, this makes sense. But other than throwing an error I don't think there is much to be done. I'll add an drm_err here just in case. > > > + > > + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) > > No need to check lttpr_count again here. So the logic behind lttpr_count and this transparency mode changing, as specified in the DP standard, is as follows: - If there are 0 LTTPRs counted, then nothing to be done, otherwise set to transparent mode. - Then, if there are between 0 and 8 LTTPRs counted, set non-transparent mode successfully. - Otherwise, rollback to transparent mode. This last rollback might result in two transparent mode settings without a non-transparent one in between, but AFAIU, that is OK. Making sure this doesn't happen would just make the implementation more ugly without any benefit, IMO. > > > + return 0; > > I'd check for errors instead of success here and do the rollback before > returning -EINVAL. > Yes, I think it would be more cleaner. Will do that. > > + > > + /* > > + * 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. Yes. Will do that. > > > +} > > +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@kernel.org> > > Johan Thanks for reviewing and testing! Abel
On Thu, Dec 26, 2024 at 02:09:46PM +0200, Abel Vesa wrote: > On 24-12-11 21:22:00, Dmitry Baryshkov wrote: > > On Wed, Dec 11, 2024 at 03:04:12PM +0200, Abel Vesa wrote: > > > 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. > > > > > > Add a generic helper that allows switching between these modes. > > > > > > Also add a generic wrapper for the helper that handles the explicit > > > disabling of non-transparent mode and its disable->enable sequence > > > mentioned in the DP Standard v2.0 section 3.6.6.1. Do this in order > > > to move this handling out of the vendor specific driver implementation > > > into the generic framework. > > > > > > Signed-off-by: Abel Vesa <abel.vesa@linaro.org> > > > --- > > > drivers/gpu/drm/display/drm_dp_helper.c | 50 +++++++++++++++++++++++++++++++++ > > > include/drm/display/drm_dp_helper.h | 2 ++ > > > 2 files changed, 52 insertions(+) > > > > > > > > > > +/** > > > + * 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); > > > + > > > + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) > > > + return 0; > > > + > > > + /* > > > + * 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); > > > > This means that if lttpr_count < 0, then there will be two requests to > > set LTTPRs to a transparent mode. Is that expected? > > Yes, exactly. If counting the LTTPRs (see drm_dp_lttpr_count) results in an > invalid number (e.g. more than 8), then according to the DP standard, > we need to roll back to transparent mode. > > Do you think I need to re-word the comment above more to make it more > clearer? My concern is that you already call drm_dp_lttpr_set_transparent_mode(true) first. So the DP should be in the transparent mode. Then if lttpr_count is 0 or less, then you call it _again_. Shouldn't it be instead: drm_dp_lttpr_set_transparent_mode(aux, true); if (lttpr_count <= 0) return 0? // or error? if (!drm_dp_lttpr_set_transparent_mode(aux, false)) return 0; /* roll-back */ drm_dp_lttpr_set_transparent_mode(aux, true); > > > > > > + > > > + return -EINVAL; > > > +} > > > +EXPORT_SYMBOL(drm_dp_lttpr_init); > > > + > > > > -- > > With best wishes > > Dmitry > > Thanks for reviewing, > Abel
On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@linaro.org> wrote: > On 24-12-11 15:42:27, 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). > > Will follow Dmitry's proposal here. > > if (ret < 0) > return ret; > > return (ret == 1) ? 0 : -EIO; Arguably this (well, with ret == len) is what we should've done with *all* of the drm_dp_dpcd_*() functions. I don't think there's a single case where we'd actually need to know that some but not all data was transferred. And if there are, they could be special cased. Now we have hundreds of cases where we check against length and it's just cumbersome all over the place. The question is, how confusing is it going to be to have some of the new functions return 0 instead of len? Very? Extremely? As painful as it would be, I'd be in favor of changing them all to return 0 on ret == len. If we find a volunteer. BR, Jani. > > >> >> > +} >> > +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? > > Yes, this makes sense. But other than throwing an error I don't think > there is much to be done. I'll add an drm_err here just in case. > >> >> > + >> > + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) >> >> No need to check lttpr_count again here. > > So the logic behind lttpr_count and this transparency mode changing, as > specified in the DP standard, is as follows: > > - If there are 0 LTTPRs counted, then nothing to be done, otherwise set to > transparent mode. > > - Then, if there are between 0 and 8 LTTPRs counted, set non-transparent > mode successfully. > > - Otherwise, rollback to transparent mode. > > This last rollback might result in two transparent mode settings without > a non-transparent one in between, but AFAIU, that is OK. Making sure this > doesn't happen would just make the implementation more ugly without any > benefit, IMO. > >> >> > + return 0; >> >> I'd check for errors instead of success here and do the rollback before >> returning -EINVAL. >> > > Yes, I think it would be more cleaner. Will do that. > >> > + >> > + /* >> > + * 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. > > Yes. Will do that. > >> >> > +} >> > +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@kernel.org> >> >> Johan > > Thanks for reviewing and testing! > Abel >
On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula wrote: > On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@linaro.org> wrote: > > On 24-12-11 15:42:27, 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). > > > > Will follow Dmitry's proposal here. > > > > if (ret < 0) > > return ret; > > > > return (ret == 1) ? 0 : -EIO; > > Arguably this (well, with ret == len) is what we should've done with > *all* of the drm_dp_dpcd_*() functions. I don't think there's a single > case where we'd actually need to know that some but not all data was > transferred. And if there are, they could be special cased. Now we have > hundreds of cases where we check against length and it's just cumbersome > all over the place. > > The question is, how confusing is it going to be to have some of the new > functions return 0 instead of len? Very? Extremely? > > As painful as it would be, I'd be in favor of changing them all to > return 0 on ret == len. If we find a volunteer. Maybe a correct Coccinelle script can do a significant part of such a conversion for us? Anyway, I think it a right thing to do. Could you possibly add a new set of API and use it inside i915 driver? Then during the next cycle we can start using new functions for all other drivers. Or would you rather add new API through drm-misc? Then we can concert e.g. existing helpers in the first place and then start working on the drivers.
On Mon, 30 Dec 2024, Dmitry Baryshkov <dmitry.baryshkov@linaro.org> wrote: > On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula wrote: >> On Thu, 26 Dec 2024, Abel Vesa <abel.vesa@linaro.org> wrote: >> > On 24-12-11 15:42:27, 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). >> > >> > Will follow Dmitry's proposal here. >> > >> > if (ret < 0) >> > return ret; >> > >> > return (ret == 1) ? 0 : -EIO; >> >> Arguably this (well, with ret == len) is what we should've done with >> *all* of the drm_dp_dpcd_*() functions. I don't think there's a single >> case where we'd actually need to know that some but not all data was >> transferred. And if there are, they could be special cased. Now we have >> hundreds of cases where we check against length and it's just cumbersome >> all over the place. >> >> The question is, how confusing is it going to be to have some of the new >> functions return 0 instead of len? Very? Extremely? >> >> As painful as it would be, I'd be in favor of changing them all to >> return 0 on ret == len. If we find a volunteer. > > Maybe a correct Coccinelle script can do a significant part of such a > conversion for us? > > Anyway, I think it a right thing to do. Could you possibly add a new set > of API and use it inside i915 driver? Then during the next cycle we can > start using new functions for all other drivers. Or would you rather add > new API through drm-misc? Then we can concert e.g. existing helpers in > the first place and then start working on the drivers. There are hundreds of drm_dp_dpcd_{read,readb,write,writeb} uses across drm, and then all the higher level helpers on top. I'm not sure adding a new API and using it in i915 achieves much. BR, Jani.
diff --git a/drivers/gpu/drm/display/drm_dp_helper.c b/drivers/gpu/drm/display/drm_dp_helper.c index da3c8521a7fa7d3c9761377363cdd4b44ab1106e..6abc54cd28e93d8101358ce05be51d4516778451 100644 --- a/drivers/gpu/drm/display/drm_dp_helper.c +++ b/drivers/gpu/drm/display/drm_dp_helper.c @@ -2817,6 +2817,56 @@ int drm_dp_lttpr_max_link_rate(const u8 caps[DP_LTTPR_COMMON_CAP_SIZE]) } EXPORT_SYMBOL(drm_dp_lttpr_max_link_rate); +/** + * 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; +} +EXPORT_SYMBOL(drm_dp_lttpr_set_transparent_mode); + +/** + * 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); + + if (lttpr_count > 0 && !drm_dp_lttpr_set_transparent_mode(aux, false)) + return 0; + + /* + * 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; +} +EXPORT_SYMBOL(drm_dp_lttpr_init); + /** * drm_dp_lttpr_max_lane_count - get the maximum lane count supported by all LTTPRs * @caps: LTTPR common capabilities diff --git a/include/drm/display/drm_dp_helper.h b/include/drm/display/drm_dp_helper.h index 8f4054a560396a43750570a8c2e95624039ab8ad..3311df3b58255cf0620391d0948ccf6b569a8a34 100644 --- a/include/drm/display/drm_dp_helper.h +++ b/include/drm/display/drm_dp_helper.h @@ -630,6 +630,8 @@ int drm_dp_read_lttpr_phy_caps(struct drm_dp_aux *aux, u8 caps[DP_LTTPR_PHY_CAP_SIZE]); int drm_dp_lttpr_count(const u8 cap[DP_LTTPR_COMMON_CAP_SIZE]); int drm_dp_lttpr_max_link_rate(const u8 caps[DP_LTTPR_COMMON_CAP_SIZE]); +int drm_dp_lttpr_set_transparent_mode(struct drm_dp_aux *aux, bool enable); +int drm_dp_lttpr_init(struct drm_dp_aux *aux, int lttpr_count); int drm_dp_lttpr_max_lane_count(const u8 caps[DP_LTTPR_COMMON_CAP_SIZE]); bool drm_dp_lttpr_voltage_swing_level_3_supported(const u8 caps[DP_LTTPR_PHY_CAP_SIZE]); bool drm_dp_lttpr_pre_emphasis_level_3_supported(const u8 caps[DP_LTTPR_PHY_CAP_SIZE]);
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. Add a generic helper that allows switching between these modes. Also add a generic wrapper for the helper that handles the explicit disabling of non-transparent mode and its disable->enable sequence mentioned in the DP Standard v2.0 section 3.6.6.1. Do this in order to move this handling out of the vendor specific driver implementation into the generic framework. Signed-off-by: Abel Vesa <abel.vesa@linaro.org> --- drivers/gpu/drm/display/drm_dp_helper.c | 50 +++++++++++++++++++++++++++++++++ include/drm/display/drm_dp_helper.h | 2 ++ 2 files changed, 52 insertions(+)