Re: [Intel-gfx] [PATCH] drm/i915/dp: take LTTPR into account in 128b/132b rates

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

On Thu, 07 Oct 2021, Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> On Thu, Oct 07, 2021 at 01:57:27PM +0300, Jani Nikula wrote:
>> Limit the supported UHBR rates based on the repeater support, if there
>> are repeaters.
>> 
>> This should be done in DP helper level, but that requires an overhaul of
>> the LTTPR handling, as the max rate is not enough to represent how
>> 128b/132b rates may be masked along the way.
>> 
>> Curiously, the spec says:
>> 
>> * Shall be cleared to 00h when operating in 8b/10b Link Layer.
>> 
>> * Each LTTPR on the way back to the DPTX shall clear the bits that do
>>   not correspond to the LTTPR's current bit rate.
>> 
>> It's rather vague if we can reliably use the field at this time due to
>> the wording "operating" and "current". But it would seem bizarre to have
>> to wait until trying to operate a 128b/132b link layer at a certain bit
>> rate to figure this out.
>
> The DP spec does kind of have that silly idea that you can
> just arbitraily reduce you link params during link training.
> Doesn't work like that of course for us since we alreday told
> userspace "Sure, I'll light up your display at this resolution".
>
> Hopefully this doesn't depend on that. Although I suppose
> we could sort of deal with it with the normal "link training
> failed -> reduce max params and signal userspace to do something" 
> approach.

*sigh*

>
>> 
>> Cc: Ville Syrjälä <ville.syrjala@linux.intel.com>
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/display/intel_dp.c | 17 +++++++++++++++++
>>  1 file changed, 17 insertions(+)
>> 
>> diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c
>> index 74a657ae131a..5824b7d9f4a8 100644
>> --- a/drivers/gpu/drm/i915/display/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/display/intel_dp.c
>> @@ -140,6 +140,9 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
>>  		return;
>>  	}
>>  
>> +	/*
>> +	 * Sink rates for 8b/10b.
>> +	 */
>>  	max_rate = drm_dp_bw_code_to_link_rate(intel_dp->dpcd[DP_MAX_LINK_RATE]);
>>  	max_lttpr_rate = drm_dp_lttpr_max_link_rate(intel_dp->lttpr_common_caps);
>>  	if (max_lttpr_rate)
>> @@ -163,6 +166,20 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
>>  		drm_dp_dpcd_readb(&intel_dp->aux,
>>  				  DP_128B132B_SUPPORTED_LINK_RATES, &uhbr_rates);
>>  
>> +		if (drm_dp_lttpr_count(intel_dp->lttpr_common_caps)) {
>> +			/* We have a repeater */
>> +			if (intel_dp->lttpr_common_caps[0] >= 0x20 &&
>> +			    intel_dp->lttpr_common_caps[DP_MAIN_LINK_CHANNEL_CODING_PHY_REPEATER -
>> +							DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV] &
>> +			    DP_PHY_REPEATER_128B132B_SUPPORTED) {
>> +				/* Repeater supports 128b/132b, valid UHBR rates */
>> +				uhbr_rates &= intel_dp->lttpr_common_caps[DP_PHY_REPEATER_128B132B_RATES - DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV];
>
> Didn't we have something to hide that
> -DP_LT_TUNABLE_PHY_REPEATER_FIELD_DATA_STRUCTURE_REV stuff?

We have but it's hidden as dp_lttpr_common_cap() in
drm_dp_helper.c. Long term I'd rather figure out a way to move this from
the driver to helpers instead of exposing that function. But I guess we
should first see how this fares in the real world.

>
> Looks more or less correct to me. I guess we'll find out eeventually how
> the spec has been interpreted.

Fingers crossed!

>
> Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com>

Thanks,
Jani.

>
>
>> +			} else {
>> +				/* Does not support 128b/132b */
>> +				uhbr_rates = 0;
>> +			}
>> +		}
>> +
>>  		if (uhbr_rates & DP_UHBR10)
>>  			intel_dp->sink_rates[i++] = 1000000;
>>  		if (uhbr_rates & DP_UHBR13_5)
>> -- 
>> 2.30.2

-- 
Jani Nikula, Intel Open Source Graphics Center