The link training delays are different and/or available in different DPCD offsets depending on:
- Clock recovery vs. channel equalization - DPRX vs. LTTPR - 128b/132b vs. 8b/10b - DPCD 1.4+ vs. earlier
Add helpers to get the correct delays in us, reading DPCD if necessary. This is more straightforward than trying to retrofit the existing helpers to take 128b/132b into account.
Having to pass in the DPCD receiver cap field seems unavoidable, because reading it involves checking the revision and reading extended receiver cap. So unfortunately the interface is mixed cached and read as needed.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/drm_dp_helper.c | 132 ++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 21 ++++- 2 files changed, 151 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4d0d1e8e51fa..04ebef7f5aa7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -154,6 +154,138 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ } EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
+static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{ + if (rd_interval > 4) + drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n", + aux->name, rd_interval); + + if (rd_interval == 0) + return 100; + + return rd_interval * 4 * USEC_PER_MSEC; +} + +static int __8b10b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{ + if (rd_interval > 4) + drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n", + aux->name, rd_interval); + + if (rd_interval == 0) + return 400; + + return rd_interval * 4 * USEC_PER_MSEC; +} + +static int __128b132b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{ + switch (rd_interval) { + default: + drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x\n", + aux->name, rd_interval); + fallthrough; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US: + return 400; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS: + return 4000; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS: + return 8000; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS: + return 12000; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS: + return 16000; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS: + return 32000; + case DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS: + return 64000; + } +} + +/* + * The link training delays are different for: + * + * - Clock recovery vs. channel equalization + * - DPRX vs. LTTPR + * - 128b/132b vs. 8b/10b + * - DPCD rev 1.3 vs. later + * + * Get the correct delay in us, reading DPCD if necessary. + */ +static int __read_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE], + enum drm_dp_phy dp_phy, bool uhbr, bool cr) +{ + int (*parse)(const struct drm_dp_aux *aux, u8 rd_interval); + unsigned int offset; + u8 rd_interval, mask; + int delay_us; + + if (dp_phy == DP_PHY_DPRX) { + if (uhbr) { + if (cr) + return 100; + + offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL; + mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK; + parse = __128b132b_channel_eq_delay_us; + } else { + if (cr && dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14) + return 100; + + offset = DP_TRAINING_AUX_RD_INTERVAL; + mask = DP_TRAINING_AUX_RD_MASK; + if (cr) + parse = __8b10b_clock_recovery_delay_us; + else + parse = __8b10b_channel_eq_delay_us; + } + } else { + if (uhbr) { + offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy); + mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK; + parse = __128b132b_channel_eq_delay_us; + } else { + if (cr) + return 100; + + offset = DP_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy); + mask = DP_TRAINING_AUX_RD_MASK; + parse = __8b10b_channel_eq_delay_us; + } + } + + if (offset < DP_RECEIVER_CAP_SIZE) { + rd_interval = dpcd[offset]; + } else { + if (drm_dp_dpcd_readb(aux, offset, &rd_interval) != 1) { + drm_dbg_kms(aux->drm_dev, "%s: failed rd interval read\n", + aux->name); + /* arbitrary default delay */ + return 400; + } + } + + delay_us = parse(aux, rd_interval & mask); + if (delay_us < 0) + return 0; + + return delay_us; +} + +int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE], + enum drm_dp_phy dp_phy, bool uhbr) +{ + return __read_delay(aux, dpcd, dp_phy, uhbr, true); +} +EXPORT_SYMBOL(drm_dp_read_clock_recovery_delay); + +int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE], + enum drm_dp_phy dp_phy, bool uhbr) +{ + return __read_delay(aux, dpcd, dp_phy, uhbr, false); +} +EXPORT_SYMBOL(drm_dp_read_channel_eq_delay); + void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index b52df4db3e8f..afdf7f4183f9 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1114,8 +1114,15 @@ struct drm_panel; # define DP_UHBR20 (1 << 1) # define DP_UHBR13_5 (1 << 2)
-#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ -# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US 0x00 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS 0x01 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS 0x02 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS 0x03 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS 0x04 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS 0x05 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS 0x06
#define DP_TEST_264BIT_CUSTOM_PATTERN_7_0 0x2230 #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256 0x2250 @@ -1389,6 +1396,11 @@ enum drm_dp_phy { # define DP_VOLTAGE_SWING_LEVEL_3_SUPPORTED BIT(0) # define DP_PRE_EMPHASIS_LEVEL_3_SUPPORTED BIT(1)
+#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 0xf0022 /* 2.0 */ +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy) \ + DP_LTTPR_REG(dp_phy, DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1) +/* see DP_128B132B_TRAINING_AUX_RD_INTERVAL for values */ + #define DP_LANE0_1_STATUS_PHY_REPEATER1 0xf0030 /* 1.3 */ #define DP_LANE0_1_STATUS_PHY_REPEATER(dp_phy) \ DP_LTTPR_REG(dp_phy, DP_LANE0_1_STATUS_PHY_REPEATER1) @@ -1527,6 +1539,11 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ #define DP_LTTPR_COMMON_CAP_SIZE 8 #define DP_LTTPR_PHY_CAP_SIZE 3
+int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE], + enum drm_dp_phy dp_phy, bool uhbr); +int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE], + enum drm_dp_phy dp_phy, bool uhbr); + void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); void drm_dp_lttpr_link_train_clock_recovery_delay(void);
Use the new link training delay helpers, fixing the delays for 128b/132b.
For existing 8b/10b functionality, this will cause additional 1-byte DPCD reads for LTTPR delays instead of using the cached values. It's just too complicated to combine generic helpers with local caching in a sensible way.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- .../drm/i915/display/intel_dp_link_training.c | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c index 85676c953e0a..a72f2dc93718 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -683,15 +683,6 @@ intel_dp_prepare_link_train(struct intel_dp *intel_dp, return true; }
-static void intel_dp_link_training_clock_recovery_delay(struct intel_dp *intel_dp, - enum drm_dp_phy dp_phy) -{ - if (dp_phy == DP_PHY_DPRX) - drm_dp_link_train_clock_recovery_delay(&intel_dp->aux, intel_dp->dpcd); - else - drm_dp_lttpr_link_train_clock_recovery_delay(); -} - static bool intel_dp_adjust_request_changed(const struct intel_crtc_state *crtc_state, const u8 old_link_status[DP_LINK_STATUS_SIZE], const u8 new_link_status[DP_LINK_STATUS_SIZE]) @@ -750,6 +741,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp, u8 link_status[DP_LINK_STATUS_SIZE]; bool max_vswing_reached = false; char phy_name[10]; + int delay_us; + + delay_us = drm_dp_read_clock_recovery_delay(&intel_dp->aux, + intel_dp->dpcd, dp_phy, + intel_dp_is_uhbr(crtc_state));
intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name));
@@ -777,7 +773,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
voltage_tries = 1; for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) { - intel_dp_link_training_clock_recovery_delay(intel_dp, dp_phy); + usleep_range(delay_us, 2 * delay_us);
if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy, link_status) < 0) { @@ -895,19 +891,6 @@ static u32 intel_dp_training_pattern(struct intel_dp *intel_dp, return DP_TRAINING_PATTERN_2; }
-static void -intel_dp_link_training_channel_equalization_delay(struct intel_dp *intel_dp, - enum drm_dp_phy dp_phy) -{ - if (dp_phy == DP_PHY_DPRX) { - drm_dp_link_train_channel_eq_delay(&intel_dp->aux, intel_dp->dpcd); - } else { - const u8 *phy_caps = intel_dp_lttpr_phy_caps(intel_dp, dp_phy); - - drm_dp_lttpr_link_train_channel_eq_delay(&intel_dp->aux, phy_caps); - } -} - /* * Perform the link training channel equalization phase on the given DP PHY * using one of training pattern 2, 3 or 4 depending on the source and @@ -925,6 +908,11 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp, u8 link_status[DP_LINK_STATUS_SIZE]; bool channel_eq = false; char phy_name[10]; + int delay_us; + + delay_us = drm_dp_read_channel_eq_delay(&intel_dp->aux, + intel_dp->dpcd, dp_phy, + intel_dp_is_uhbr(crtc_state));
intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name));
@@ -944,8 +932,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp, }
for (tries = 0; tries < 5; tries++) { - intel_dp_link_training_channel_equalization_delay(intel_dp, - dp_phy); + usleep_range(delay_us, 2 * delay_us); + if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy, link_status) < 0) { drm_err(&i915->drm,
On Tue, Oct 12, 2021 at 05:43:21PM +0300, Jani Nikula wrote:
Use the new link training delay helpers, fixing the delays for 128b/132b.
For existing 8b/10b functionality, this will cause additional 1-byte DPCD reads for LTTPR delays instead of using the cached values. It's just too complicated to combine generic helpers with local caching in a sensible way.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com
I was just pondering if the extra DPCD reads might cause some grief for some compliance test stuff. But I guess if that happens we could just read them all at the very start of link training, or something.
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
.../drm/i915/display/intel_dp_link_training.c | 38 +++++++------------ 1 file changed, 13 insertions(+), 25 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.c b/drivers/gpu/drm/i915/display/intel_dp_link_training.c index 85676c953e0a..a72f2dc93718 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -683,15 +683,6 @@ intel_dp_prepare_link_train(struct intel_dp *intel_dp, return true; }
-static void intel_dp_link_training_clock_recovery_delay(struct intel_dp *intel_dp,
enum drm_dp_phy dp_phy)-{
- if (dp_phy == DP_PHY_DPRX)
drm_dp_link_train_clock_recovery_delay(&intel_dp->aux, intel_dp->dpcd);- else
drm_dp_lttpr_link_train_clock_recovery_delay();-}
static bool intel_dp_adjust_request_changed(const struct intel_crtc_state *crtc_state, const u8 old_link_status[DP_LINK_STATUS_SIZE], const u8 new_link_status[DP_LINK_STATUS_SIZE]) @@ -750,6 +741,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp, u8 link_status[DP_LINK_STATUS_SIZE]; bool max_vswing_reached = false; char phy_name[10];
int delay_us;
delay_us = drm_dp_read_clock_recovery_delay(&intel_dp->aux,
intel_dp->dpcd, dp_phy,intel_dp_is_uhbr(crtc_state));intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name));
@@ -777,7 +773,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp,
voltage_tries = 1; for (cr_tries = 0; cr_tries < max_cr_tries; ++cr_tries) {
intel_dp_link_training_clock_recovery_delay(intel_dp, dp_phy);
usleep_range(delay_us, 2 * delay_us);if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy, link_status) < 0) {
@@ -895,19 +891,6 @@ static u32 intel_dp_training_pattern(struct intel_dp *intel_dp, return DP_TRAINING_PATTERN_2; }
-static void -intel_dp_link_training_channel_equalization_delay(struct intel_dp *intel_dp,
enum drm_dp_phy dp_phy)-{
- if (dp_phy == DP_PHY_DPRX) {
drm_dp_link_train_channel_eq_delay(&intel_dp->aux, intel_dp->dpcd);- } else {
const u8 *phy_caps = intel_dp_lttpr_phy_caps(intel_dp, dp_phy);drm_dp_lttpr_link_train_channel_eq_delay(&intel_dp->aux, phy_caps);- }
-}
/*
- Perform the link training channel equalization phase on the given DP PHY
- using one of training pattern 2, 3 or 4 depending on the source and
@@ -925,6 +908,11 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp, u8 link_status[DP_LINK_STATUS_SIZE]; bool channel_eq = false; char phy_name[10];
int delay_us;
delay_us = drm_dp_read_channel_eq_delay(&intel_dp->aux,
intel_dp->dpcd, dp_phy,intel_dp_is_uhbr(crtc_state));intel_dp_phy_name(dp_phy, phy_name, sizeof(phy_name));
@@ -944,8 +932,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp, }
for (tries = 0; tries < 5; tries++) {
intel_dp_link_training_channel_equalization_delay(intel_dp,dp_phy);
usleep_range(delay_us, 2 * delay_us);- if (drm_dp_dpcd_read_phy_link_status(&intel_dp->aux, dp_phy, link_status) < 0) { drm_err(&i915->drm,
-- 2.30.2
On Tue, Oct 12, 2021 at 05:43:20PM +0300, Jani Nikula wrote:
The link training delays are different and/or available in different DPCD offsets depending on:
- Clock recovery vs. channel equalization
- DPRX vs. LTTPR
- 128b/132b vs. 8b/10b
- DPCD 1.4+ vs. earlier
Add helpers to get the correct delays in us, reading DPCD if necessary. This is more straightforward than trying to retrofit the existing helpers to take 128b/132b into account.
Having to pass in the DPCD receiver cap field seems unavoidable, because reading it involves checking the revision and reading extended receiver cap. So unfortunately the interface is mixed cached and read as needed.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com
drivers/gpu/drm/drm_dp_helper.c | 132 ++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 21 ++++- 2 files changed, 151 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4d0d1e8e51fa..04ebef7f5aa7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -154,6 +154,138 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ } EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
+static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- if (rd_interval > 4)
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n",aux->name, rd_interval);- if (rd_interval == 0)
return 100;- return rd_interval * 4 * USEC_PER_MSEC;
+}
+static int __8b10b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- if (rd_interval > 4)
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n",aux->name, rd_interval);- if (rd_interval == 0)
return 400;- return rd_interval * 4 * USEC_PER_MSEC;
+}
Is there a reason you're not reusing these in the existing sleepy functions? Maybe just passing in the dpcd receiver cap all the way would also be nicer since then these functions would do all the work, instead of splitting it partially between these and the caller. Also with the 1.4+ case handled elsewhere there won't be debug spew for illegal values (not sure we care too much though).
+static int __128b132b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- switch (rd_interval) {
- default:
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x\n",aux->name, rd_interval);fallthrough;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US:
return 400;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS:
return 4000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS:
return 8000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS:
return 12000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS:
return 16000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS:
return 32000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS:
return 64000;- }
+}
The spec does claim that only 00-06 are legal also for the CR delay. So here too we lose the debug spew if we don' have the CR version of this.
+/*
- The link training delays are different for:
- Clock recovery vs. channel equalization
- DPRX vs. LTTPR
- 128b/132b vs. 8b/10b
- DPCD rev 1.3 vs. later
- Get the correct delay in us, reading DPCD if necessary.
- */
+static int __read_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr, bool cr)+{
- int (*parse)(const struct drm_dp_aux *aux, u8 rd_interval);
- unsigned int offset;
- u8 rd_interval, mask;
- int delay_us;
- if (dp_phy == DP_PHY_DPRX) {
if (uhbr) {if (cr)return 100;offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL;mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;parse = __128b132b_channel_eq_delay_us;} else {if (cr && dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)return 100;offset = DP_TRAINING_AUX_RD_INTERVAL;mask = DP_TRAINING_AUX_RD_MASK;if (cr)parse = __8b10b_clock_recovery_delay_us;elseparse = __8b10b_channel_eq_delay_us;}- } else {
if (uhbr) {offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy);mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;parse = __128b132b_channel_eq_delay_us;} else {if (cr)return 100;offset = DP_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy);mask = DP_TRAINING_AUX_RD_MASK;parse = __8b10b_channel_eq_delay_us;}- }
- if (offset < DP_RECEIVER_CAP_SIZE) {
rd_interval = dpcd[offset];- } else {
if (drm_dp_dpcd_readb(aux, offset, &rd_interval) != 1) {drm_dbg_kms(aux->drm_dev, "%s: failed rd interval read\n",aux->name);/* arbitrary default delay */return 400;}- }
- delay_us = parse(aux, rd_interval & mask);
- if (delay_us < 0)
Is that possible?
return 0;- return delay_us;
+}
+int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr)+{
- return __read_delay(aux, dpcd, dp_phy, uhbr, true);
+} +EXPORT_SYMBOL(drm_dp_read_clock_recovery_delay);
+int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr)+{
- return __read_delay(aux, dpcd, dp_phy, uhbr, false);
+} +EXPORT_SYMBOL(drm_dp_read_channel_eq_delay);
void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index b52df4db3e8f..afdf7f4183f9 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1114,8 +1114,15 @@ struct drm_panel; # define DP_UHBR20 (1 << 1) # define DP_UHBR13_5 (1 << 2)
-#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ -# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US 0x00 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS 0x01 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS 0x02 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS 0x03 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS 0x04 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS 0x05 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS 0x06
What a wonderful mix of 4*x vs. 2^x. Sticking to one or the other would have made life too easy obviously. But could still use that rule to shorten the switch statement in __128b132b_channel_eq_delay_us() a bit. And for 16ms you could even have some fun flipping a coin to decide on which side it goes :P
Anyways, looks mostly about as sane as this stuff can be. Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
#define DP_TEST_264BIT_CUSTOM_PATTERN_7_0 0x2230 #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256 0x2250 @@ -1389,6 +1396,11 @@ enum drm_dp_phy { # define DP_VOLTAGE_SWING_LEVEL_3_SUPPORTED BIT(0) # define DP_PRE_EMPHASIS_LEVEL_3_SUPPORTED BIT(1)
+#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 0xf0022 /* 2.0 */ +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy) \
- DP_LTTPR_REG(dp_phy, DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1)
+/* see DP_128B132B_TRAINING_AUX_RD_INTERVAL for values */
#define DP_LANE0_1_STATUS_PHY_REPEATER1 0xf0030 /* 1.3 */ #define DP_LANE0_1_STATUS_PHY_REPEATER(dp_phy) \ DP_LTTPR_REG(dp_phy, DP_LANE0_1_STATUS_PHY_REPEATER1) @@ -1527,6 +1539,11 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ #define DP_LTTPR_COMMON_CAP_SIZE 8 #define DP_LTTPR_PHY_CAP_SIZE 3
+int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr);+int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr);void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); void drm_dp_lttpr_link_train_clock_recovery_delay(void); -- 2.30.2
On Thu, 14 Oct 2021, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Oct 12, 2021 at 05:43:20PM +0300, Jani Nikula wrote:
The link training delays are different and/or available in different DPCD offsets depending on:
- Clock recovery vs. channel equalization
- DPRX vs. LTTPR
- 128b/132b vs. 8b/10b
- DPCD 1.4+ vs. earlier
Add helpers to get the correct delays in us, reading DPCD if necessary. This is more straightforward than trying to retrofit the existing helpers to take 128b/132b into account.
Having to pass in the DPCD receiver cap field seems unavoidable, because reading it involves checking the revision and reading extended receiver cap. So unfortunately the interface is mixed cached and read as needed.
Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com
drivers/gpu/drm/drm_dp_helper.c | 132 ++++++++++++++++++++++++++++++++ include/drm/drm_dp_helper.h | 21 ++++- 2 files changed, 151 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/drm_dp_helper.c b/drivers/gpu/drm/drm_dp_helper.c index 4d0d1e8e51fa..04ebef7f5aa7 100644 --- a/drivers/gpu/drm/drm_dp_helper.c +++ b/drivers/gpu/drm/drm_dp_helper.c @@ -154,6 +154,138 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ } EXPORT_SYMBOL(drm_dp_get_adjust_request_post_cursor);
+static int __8b10b_clock_recovery_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- if (rd_interval > 4)
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n",aux->name, rd_interval);- if (rd_interval == 0)
return 100;- return rd_interval * 4 * USEC_PER_MSEC;
+}
+static int __8b10b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- if (rd_interval > 4)
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x (max 4)\n",aux->name, rd_interval);- if (rd_interval == 0)
return 400;- return rd_interval * 4 * USEC_PER_MSEC;
+}
Is there a reason you're not reusing these in the existing sleepy functions?
I decided to see how this flies first, and do that as a follow-up.
Maybe just passing in the dpcd receiver cap all the way would also be nicer since then these functions would do all the work, instead of splitting it partially between these and the caller.
It's a bit subtle. Checking for dpcd receiver cap in the caller allows early return without the dpcd read in some cases.
It's all really unnecessarily complicated how the delays are spread all over the place in dpcd. I couldn't think of a cleaner approach (without a massive dpcd caching rewrite using regmap, anyway ;).
Also with the 1.4+ case handled elsewhere there won't be debug spew for illegal values (not sure we care too much though).
That's subtle too. This leaves out the check when we're not using the value, which seems fine to me. However, the same dpcd offset is used for channel equalization, and that will have the check and debug print.
+static int __128b132b_channel_eq_delay_us(const struct drm_dp_aux *aux, u8 rd_interval) +{
- switch (rd_interval) {
- default:
drm_dbg_kms(aux->drm_dev, "%s: invalid AUX interval 0x%02x\n",aux->name, rd_interval);fallthrough;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US:
return 400;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS:
return 4000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS:
return 8000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS:
return 12000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS:
return 16000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS:
return 32000;- case DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS:
return 64000;- }
+}
The spec does claim that only 00-06 are legal also for the CR delay. So here too we lose the debug spew if we don' have the CR version of this.
+/*
- The link training delays are different for:
- Clock recovery vs. channel equalization
- DPRX vs. LTTPR
- 128b/132b vs. 8b/10b
- DPCD rev 1.3 vs. later
- Get the correct delay in us, reading DPCD if necessary.
- */
+static int __read_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr, bool cr)+{
- int (*parse)(const struct drm_dp_aux *aux, u8 rd_interval);
- unsigned int offset;
- u8 rd_interval, mask;
- int delay_us;
- if (dp_phy == DP_PHY_DPRX) {
if (uhbr) {if (cr)return 100;offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL;mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;parse = __128b132b_channel_eq_delay_us;} else {if (cr && dpcd[DP_DPCD_REV] >= DP_DPCD_REV_14)return 100;offset = DP_TRAINING_AUX_RD_INTERVAL;mask = DP_TRAINING_AUX_RD_MASK;if (cr)parse = __8b10b_clock_recovery_delay_us;elseparse = __8b10b_channel_eq_delay_us;}- } else {
if (uhbr) {offset = DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy);mask = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;parse = __128b132b_channel_eq_delay_us;} else {if (cr)return 100;offset = DP_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy);mask = DP_TRAINING_AUX_RD_MASK;parse = __8b10b_channel_eq_delay_us;}- }
- if (offset < DP_RECEIVER_CAP_SIZE) {
rd_interval = dpcd[offset];- } else {
if (drm_dp_dpcd_readb(aux, offset, &rd_interval) != 1) {drm_dbg_kms(aux->drm_dev, "%s: failed rd interval read\n",aux->name);/* arbitrary default delay */return 400;}- }
- delay_us = parse(aux, rd_interval & mask);
- if (delay_us < 0)
Is that possible?
It's not, this is leftover from when I thought of return negative errors but decided against it. I'll respin just for this.
return 0;- return delay_us;
+}
+int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr)+{
- return __read_delay(aux, dpcd, dp_phy, uhbr, true);
+} +EXPORT_SYMBOL(drm_dp_read_clock_recovery_delay);
+int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr)+{
- return __read_delay(aux, dpcd, dp_phy, uhbr, false);
+} +EXPORT_SYMBOL(drm_dp_read_channel_eq_delay);
void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]) { diff --git a/include/drm/drm_dp_helper.h b/include/drm/drm_dp_helper.h index b52df4db3e8f..afdf7f4183f9 100644 --- a/include/drm/drm_dp_helper.h +++ b/include/drm/drm_dp_helper.h @@ -1114,8 +1114,15 @@ struct drm_panel; # define DP_UHBR20 (1 << 1) # define DP_UHBR13_5 (1 << 2)
-#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ -# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK 0x7f +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_400_US 0x00 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_4_MS 0x01 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_8_MS 0x02 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_12_MS 0x03 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_16_MS 0x04 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_32_MS 0x05 +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_64_MS 0x06
What a wonderful mix of 4*x vs. 2^x. Sticking to one or the other would have made life too easy obviously. But could still use that rule to shorten the switch statement in __128b132b_channel_eq_delay_us() a bit. And for 16ms you could even have some fun flipping a coin to decide on which side it goes :P
I'm not sure it's worth it to add the arithmetic.
(But I really really don't understand why it's so hard to decide on a unit, and add a multiplier in dpcd. Or even reserve 1-2 high bits to choose the unit, and leave the rest for the multiplier. *sigh*.)
Anyways, looks mostly about as sane as this stuff can be. Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
Thanks, I'll respin for the return value check.
BR, Jani.
#define DP_TEST_264BIT_CUSTOM_PATTERN_7_0 0x2230 #define DP_TEST_264BIT_CUSTOM_PATTERN_263_256 0x2250 @@ -1389,6 +1396,11 @@ enum drm_dp_phy { # define DP_VOLTAGE_SWING_LEVEL_3_SUPPORTED BIT(0) # define DP_PRE_EMPHASIS_LEVEL_3_SUPPORTED BIT(1)
+#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1 0xf0022 /* 2.0 */ +#define DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER(dp_phy) \
- DP_LTTPR_REG(dp_phy, DP_128B132B_TRAINING_AUX_RD_INTERVAL_PHY_REPEATER1)
+/* see DP_128B132B_TRAINING_AUX_RD_INTERVAL for values */
#define DP_LANE0_1_STATUS_PHY_REPEATER1 0xf0030 /* 1.3 */ #define DP_LANE0_1_STATUS_PHY_REPEATER(dp_phy) \ DP_LTTPR_REG(dp_phy, DP_LANE0_1_STATUS_PHY_REPEATER1) @@ -1527,6 +1539,11 @@ u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZ #define DP_LTTPR_COMMON_CAP_SIZE 8 #define DP_LTTPR_PHY_CAP_SIZE 3
+int drm_dp_read_clock_recovery_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr);+int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE],
enum drm_dp_phy dp_phy, bool uhbr);void drm_dp_link_train_clock_recovery_delay(const struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIVER_CAP_SIZE]); void drm_dp_lttpr_link_train_clock_recovery_delay(void); -- 2.30.2
dri-devel@lists.freedesktop.org
-
Jani Nikula -
Ville Syrjälä