On Thu, 07 Oct 2021, Ville Syrjälä <ville.syrjala@xxxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxxx>
>> Signed-off-by: Jani Nikula <jani.nikula@xxxxxxxxx>
>> ---
>> 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@xxxxxxxxxxxxxxx>
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
