Re: [PATCH v2 1/4] drm/dp: Add helper to set LTTPRs in transparent mode

[Jani Nikula <jani.nikula@linux.intel.com>]

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
>

-- 
Jani Nikula, Intel