| Message ID | 20211007105727.18439-1-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | drm/i915/dp: take LTTPR into account in 128b/132b rates | expand |
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. > > 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? Looks more or less correct to me. I guess we'll find out eeventually how the spec has been interpreted. Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> > + } 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
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
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]; + } 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)
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. 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(+)