Nouveau - Dec 2024 - [PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode

On Thu, 26 Dec 2024, Abel Vesa <abel.vesa at 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 at kernel.org>
>> 
>> Johan
>
> Thanks for reviewing and testing!
> Abel
>
-- 
Jani Nikula, Intel

On Mon, Dec 30, 2024 at 03:18:35PM +0200, Jani Nikula
wrote:
> On Thu, 26 Dec 2024, Abel Vesa <abel.vesa at 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.


-- 
With best wishes
Dmitry