On Thu, 07 Oct 2021, Ville Syrjälä <[email protected]> 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ä <[email protected]> >> Signed-off-by: Jani Nikula <[email protected]> >> --- >> 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ä <[email protected]> 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
