128b/132b updates to reflect DP 2.0 errata changes, plus some other DP updates.
Jani Nikula (8): drm/dp: add drm_dp_128b132b_read_aux_rd_interval() drm/dp: add 128b/132b link status helpers from DP 2.0 E11 drm/dp: add some new DPCD macros from DP 2.0 E11 drm/i915/dp: move intel_dp_prepare_link_train() call drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata drm/i915/dp: add 128b/132b support to link status checks drm/i915/dp: give more time for CDS drm/i915/mst: update slot information for 128b/132b
drivers/gpu/drm/dp/drm_dp.c | 83 ++++++ drivers/gpu/drm/i915/display/intel_dp.c | 39 ++- .../drm/i915/display/intel_dp_link_training.c | 262 +++++++++++++++++- .../drm/i915/display/intel_dp_link_training.h | 4 + drivers/gpu/drm/i915/display/intel_dp_mst.c | 29 +- include/drm/dp/drm_dp_helper.h | 24 +- 6 files changed, 420 insertions(+), 21 deletions(-)
The DP 2.0 errata changes DP_128B132B_TRAINING_AUX_RD_INTERVAL (DPCD 0x2216) completely. Add a new function to read that. Follow-up will need to clean up existing functions.
v2: fix reversed interpretation of bit 7 meaning (Uma)
Cc: Uma Shankar uma.shankar@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/dp/drm_dp.c | 20 ++++++++++++++++++++ include/drm/dp/drm_dp_helper.h | 3 +++ 2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/dp/drm_dp.c b/drivers/gpu/drm/dp/drm_dp.c index 6d43325acca5..52c6da510142 100644 --- a/drivers/gpu/drm/dp/drm_dp.c +++ b/drivers/gpu/drm/dp/drm_dp.c @@ -281,6 +281,26 @@ int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIV } EXPORT_SYMBOL(drm_dp_read_channel_eq_delay);
+/* Per DP 2.0 Errata */ +int drm_dp_128b132b_read_aux_rd_interval(struct drm_dp_aux *aux) +{ + int unit; + u8 val; + + if (drm_dp_dpcd_readb(aux, DP_128B132B_TRAINING_AUX_RD_INTERVAL, &val) != 1) { + drm_err(aux->drm_dev, "%s: failed rd interval read\n", + aux->name); + /* default to max */ + val = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK; + } + + unit = (val & DP_128B132B_TRAINING_AUX_RD_INTERVAL_1MS_UNIT) ? 1 : 2; + val &= DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK; + + return (val + 1) * unit * 1000; +} +EXPORT_SYMBOL(drm_dp_128b132b_read_aux_rd_interval); + 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/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h index 98d020835b49..aa73dfc817ff 100644 --- a/include/drm/dp/drm_dp_helper.h +++ b/include/drm/dp/drm_dp_helper.h @@ -1112,6 +1112,7 @@ struct drm_panel; # define DP_UHBR13_5 (1 << 2)
#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_1MS_UNIT (1 << 7) # 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 @@ -1549,6 +1550,8 @@ void drm_dp_link_train_channel_eq_delay(const struct drm_dp_aux *aux, void drm_dp_lttpr_link_train_channel_eq_delay(const struct drm_dp_aux *aux, const u8 caps[DP_LTTPR_PHY_CAP_SIZE]);
+int drm_dp_128b132b_read_aux_rd_interval(struct drm_dp_aux *aux); + u8 drm_dp_link_rate_to_bw_code(int link_rate); int drm_dp_bw_code_to_link_rate(u8 link_bw);
On Tue, Jan 25, 2022 at 07:03:39PM +0200, Jani Nikula wrote:
The DP 2.0 errata changes DP_128B132B_TRAINING_AUX_RD_INTERVAL (DPCD 0x2216) completely. Add a new function to read that. Follow-up will need to clean up existing functions.
v2: fix reversed interpretation of bit 7 meaning (Uma)
Cc: Uma Shankar uma.shankar@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
drivers/gpu/drm/dp/drm_dp.c | 20 ++++++++++++++++++++ include/drm/dp/drm_dp_helper.h | 3 +++ 2 files changed, 23 insertions(+)
diff --git a/drivers/gpu/drm/dp/drm_dp.c b/drivers/gpu/drm/dp/drm_dp.c index 6d43325acca5..52c6da510142 100644 --- a/drivers/gpu/drm/dp/drm_dp.c +++ b/drivers/gpu/drm/dp/drm_dp.c @@ -281,6 +281,26 @@ int drm_dp_read_channel_eq_delay(struct drm_dp_aux *aux, const u8 dpcd[DP_RECEIV } EXPORT_SYMBOL(drm_dp_read_channel_eq_delay);
+/* Per DP 2.0 Errata */ +int drm_dp_128b132b_read_aux_rd_interval(struct drm_dp_aux *aux) +{
- int unit;
- u8 val;
- if (drm_dp_dpcd_readb(aux, DP_128B132B_TRAINING_AUX_RD_INTERVAL, &val) != 1) {
drm_err(aux->drm_dev, "%s: failed rd interval read\n",
aux->name);
/* default to max */
val = DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;
- }
- unit = (val & DP_128B132B_TRAINING_AUX_RD_INTERVAL_1MS_UNIT) ? 1 : 2;
- val &= DP_128B132B_TRAINING_AUX_RD_INTERVAL_MASK;
- return (val + 1) * unit * 1000;
+} +EXPORT_SYMBOL(drm_dp_128b132b_read_aux_rd_interval);
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/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h index 98d020835b49..aa73dfc817ff 100644 --- a/include/drm/dp/drm_dp_helper.h +++ b/include/drm/dp/drm_dp_helper.h @@ -1112,6 +1112,7 @@ struct drm_panel; # define DP_UHBR13_5 (1 << 2)
#define DP_128B132B_TRAINING_AUX_RD_INTERVAL 0x2216 /* 2.0 */ +# define DP_128B132B_TRAINING_AUX_RD_INTERVAL_1MS_UNIT (1 << 7) # 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 @@ -1549,6 +1550,8 @@ void drm_dp_link_train_channel_eq_delay(const struct drm_dp_aux *aux, void drm_dp_lttpr_link_train_channel_eq_delay(const struct drm_dp_aux *aux, const u8 caps[DP_LTTPR_PHY_CAP_SIZE]);
+int drm_dp_128b132b_read_aux_rd_interval(struct drm_dp_aux *aux);
u8 drm_dp_link_rate_to_bw_code(int link_rate); int drm_dp_bw_code_to_link_rate(u8 link_bw);
-- 2.30.2
The DP 2.0 errata redefines link training. There are some new status bits, and some of the old ones need to be checked independently. Add helpers to do this.
Cc: Uma Shankar uma.shankar@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- drivers/gpu/drm/dp/drm_dp.c | 63 ++++++++++++++++++++++++++++++++++ include/drm/dp/drm_dp_helper.h | 19 +++++++--- 2 files changed, 77 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/dp/drm_dp.c b/drivers/gpu/drm/dp/drm_dp.c index 52c6da510142..a20b0f8f24b8 100644 --- a/drivers/gpu/drm/dp/drm_dp.c +++ b/drivers/gpu/drm/dp/drm_dp.c @@ -144,6 +144,69 @@ u8 drm_dp_get_adjust_tx_ffe_preset(const u8 link_status[DP_LINK_STATUS_SIZE], } EXPORT_SYMBOL(drm_dp_get_adjust_tx_ffe_preset);
+/* DP 2.0 errata for 128b/132b */ +bool drm_dp_128b132b_lane_channel_eq_done(const u8 link_status[DP_LINK_STATUS_SIZE], + int lane_count) +{ + u8 lane_align, lane_status; + int lane; + + lane_align = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED); + if (!(lane_align & DP_INTERLANE_ALIGN_DONE)) + return false; + + for (lane = 0; lane < lane_count; lane++) { + lane_status = dp_get_lane_status(link_status, lane); + if (!(lane_status & DP_LANE_CHANNEL_EQ_DONE)) + return false; + } + return true; +} +EXPORT_SYMBOL(drm_dp_128b132b_lane_channel_eq_done); + +/* DP 2.0 errata for 128b/132b */ +bool drm_dp_128b132b_lane_symbol_locked(const u8 link_status[DP_LINK_STATUS_SIZE], + int lane_count) +{ + u8 lane_status; + int lane; + + for (lane = 0; lane < lane_count; lane++) { + lane_status = dp_get_lane_status(link_status, lane); + if (!(lane_status & DP_LANE_SYMBOL_LOCKED)) + return false; + } + return true; +} +EXPORT_SYMBOL(drm_dp_128b132b_lane_symbol_locked); + +/* DP 2.0 errata for 128b/132b */ +bool drm_dp_128b132b_eq_interlane_align_done(const u8 link_status[DP_LINK_STATUS_SIZE]) +{ + u8 status = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED); + + return status & DP_128B132B_DPRX_EQ_INTERLANE_ALIGN_DONE; +} +EXPORT_SYMBOL(drm_dp_128b132b_eq_interlane_align_done); + +/* DP 2.0 errata for 128b/132b */ +bool drm_dp_128b132b_cds_interlane_align_done(const u8 link_status[DP_LINK_STATUS_SIZE]) +{ + u8 status = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED); + + return status & DP_128B132B_DPRX_CDS_INTERLANE_ALIGN_DONE; +} +EXPORT_SYMBOL(drm_dp_128b132b_cds_interlane_align_done); + +/* DP 2.0 errata for 128b/132b */ +bool drm_dp_128b132b_link_training_failed(const u8 link_status[DP_LINK_STATUS_SIZE]) +{ + u8 status = dp_link_status(link_status, DP_LANE_ALIGN_STATUS_UPDATED); + + return status & DP_128B132B_LT_FAILED; +} +EXPORT_SYMBOL(drm_dp_128b132b_link_training_failed); + u8 drm_dp_get_adjust_request_post_cursor(const u8 link_status[DP_LINK_STATUS_SIZE], unsigned int lane) { diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h index aa73dfc817ff..c499d735b992 100644 --- a/include/drm/dp/drm_dp_helper.h +++ b/include/drm/dp/drm_dp_helper.h @@ -738,11 +738,13 @@ struct drm_panel; DP_LANE_CHANNEL_EQ_DONE | \ DP_LANE_SYMBOL_LOCKED)
-#define DP_LANE_ALIGN_STATUS_UPDATED 0x204 - -#define DP_INTERLANE_ALIGN_DONE (1 << 0) -#define DP_DOWNSTREAM_PORT_STATUS_CHANGED (1 << 6) -#define DP_LINK_STATUS_UPDATED (1 << 7) +#define DP_LANE_ALIGN_STATUS_UPDATED 0x204 +#define DP_INTERLANE_ALIGN_DONE (1 << 0) +#define DP_128B132B_DPRX_EQ_INTERLANE_ALIGN_DONE (1 << 2) /* 2.0 E11 */ +#define DP_128B132B_DPRX_CDS_INTERLANE_ALIGN_DONE (1 << 3) /* 2.0 E11 */ +#define DP_128B132B_LT_FAILED (1 << 4) /* 2.0 E11 */ +#define DP_DOWNSTREAM_PORT_STATUS_CHANGED (1 << 6) +#define DP_LINK_STATUS_UPDATED (1 << 7)
#define DP_SINK_STATUS 0x205 # define DP_RECEIVE_PORT_0_STATUS (1 << 0) @@ -1551,6 +1553,13 @@ void drm_dp_lttpr_link_train_channel_eq_delay(const struct drm_dp_aux *aux, const u8 caps[DP_LTTPR_PHY_CAP_SIZE]);
int drm_dp_128b132b_read_aux_rd_interval(struct drm_dp_aux *aux); +bool drm_dp_128b132b_lane_channel_eq_done(const u8 link_status[DP_LINK_STATUS_SIZE], + int lane_count); +bool drm_dp_128b132b_lane_symbol_locked(const u8 link_status[DP_LINK_STATUS_SIZE], + int lane_count); +bool drm_dp_128b132b_eq_interlane_align_done(const u8 link_status[DP_LINK_STATUS_SIZE]); +bool drm_dp_128b132b_cds_interlane_align_done(const u8 link_status[DP_LINK_STATUS_SIZE]); +bool drm_dp_128b132b_link_training_failed(const u8 link_status[DP_LINK_STATUS_SIZE]);
u8 drm_dp_link_rate_to_bw_code(int link_rate); int drm_dp_bw_code_to_link_rate(u8 link_bw);
Add some of the new additions from DP 2.0 E11.
Cc: Uma Shankar uma.shankar@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com --- include/drm/dp/drm_dp_helper.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h index c499d735b992..69487bd8ed56 100644 --- a/include/drm/dp/drm_dp_helper.h +++ b/include/drm/dp/drm_dp_helper.h @@ -560,6 +560,7 @@ struct drm_panel; # define DP_TRAINING_PATTERN_DISABLE 0 # define DP_TRAINING_PATTERN_1 1 # define DP_TRAINING_PATTERN_2 2 +# define DP_TRAINING_PATTERN_2_CDS 3 /* 2.0 E11 */ # define DP_TRAINING_PATTERN_3 3 /* 1.2 */ # define DP_TRAINING_PATTERN_4 7 /* 1.4 */ # define DP_TRAINING_PATTERN_MASK 0x3 @@ -1350,6 +1351,7 @@ struct drm_panel; # define DP_PHY_REPEATER_128B132B_SUPPORTED (1 << 0) /* See DP_128B132B_SUPPORTED_LINK_RATES for values */ #define DP_PHY_REPEATER_128B132B_RATES 0xf0007 /* 2.0 */ +#define DP_PHY_REPEATER_EQ_DONE 0xf0008 /* 2.0 E11 */
enum drm_dp_phy { DP_PHY_DPRX,
On Tue, Jan 25, 2022 at 07:03:41PM +0200, Jani Nikula wrote:
Add some of the new additions from DP 2.0 E11.
Cc: Uma Shankar uma.shankar@intel.com Cc: Ville Syrjälä ville.syrjala@linux.intel.com Signed-off-by: Jani Nikula jani.nikula@intel.com
Reviewed-by: Ville Syrjälä ville.syrjala@linux.intel.com
include/drm/dp/drm_dp_helper.h | 2 ++ 1 file changed, 2 insertions(+)
diff --git a/include/drm/dp/drm_dp_helper.h b/include/drm/dp/drm_dp_helper.h index c499d735b992..69487bd8ed56 100644 --- a/include/drm/dp/drm_dp_helper.h +++ b/include/drm/dp/drm_dp_helper.h @@ -560,6 +560,7 @@ struct drm_panel; # define DP_TRAINING_PATTERN_DISABLE 0 # define DP_TRAINING_PATTERN_1 1 # define DP_TRAINING_PATTERN_2 2 +# define DP_TRAINING_PATTERN_2_CDS 3 /* 2.0 E11 */ # define DP_TRAINING_PATTERN_3 3 /* 1.2 */ # define DP_TRAINING_PATTERN_4 7 /* 1.4 */ # define DP_TRAINING_PATTERN_MASK 0x3 @@ -1350,6 +1351,7 @@ struct drm_panel; # define DP_PHY_REPEATER_128B132B_SUPPORTED (1 << 0) /* See DP_128B132B_SUPPORTED_LINK_RATES for values */ #define DP_PHY_REPEATER_128B132B_RATES 0xf0007 /* 2.0 */ +#define DP_PHY_REPEATER_EQ_DONE 0xf0008 /* 2.0 E11 */
Wonder if we should look at that at some point? The spec doesn't really say so. Or maybe we should just dump it out of the link training failed?
enum drm_dp_phy { DP_PHY_DPRX, -- 2.30.2
Call it from the higher level function, as it will be shared between two code paths.
Cc: Uma Shankar uma.shankar@intel.com 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_link_training.c | 4 ++-- 1 file changed, 2 insertions(+), 2 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 9451f336f28f..4e507aa75a03 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1083,8 +1083,6 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, bool ret = true; int i;
- intel_dp_prepare_link_train(intel_dp, crtc_state); - for (i = lttpr_count - 1; i >= 0; i--) { enum drm_dp_phy dp_phy = DP_PHY_LTTPR(i);
@@ -1127,6 +1125,8 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp, /* Still continue with enabling the port and link training. */ lttpr_count = 0;
+ intel_dp_prepare_link_train(intel_dp, crtc_state); + if (!intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count)) intel_dp_schedule_fallback_link_training(intel_dp, crtc_state); }
The DP 2.0 errata completely overhauls the 128b/132b link training, with no provisions for backward compatibility with the original DP 2.0 specification.
The changes are too intrusive to consider reusing the same code for both 8b/10b and 128b/132b, mainly because the LTTPR channel equalisation is done concurrently instead of serialized.
NOTES:
* It's a bit unclear when to wait for DP_INTERLANE_ALIGN_DONE and per-lane DP_LANE_SYMBOL_LOCKED. Figure xx4 in the SCR implies the LANEx_CHANNEL_EQ_DONE sequence may end with either 0x77,0x77,0x85 *or* 0x33,0x33,0x84 (for four lane configuration in DPCD 0x202..0x204) i.e. without the above bits set. Text elsewhere seems contradictory or incomplete.
* We read entire link status (6 bytes) everywhere instead of individual DPCD addresses.
* There are some subtle ambiguities or contradictions in the order of some DPCD access and TPS signal enables/disables. It's also not clear whether these are significant.
Cc: Uma Shankar uma.shankar@intel.com 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 | 252 +++++++++++++++++- 1 file changed, 251 insertions(+), 1 deletion(-)
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 4e507aa75a03..8bb6a296f421 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1102,6 +1102,250 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, return ret; }
+ +/* + * 128b/132b DP LANEx_EQ_DONE Sequence (DP 2.0 E11 3.5.2.16.1) + */ +static bool +intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state) +{ + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; + struct drm_i915_private *i915 = to_i915(encoder->base.dev); + u8 link_status[DP_LINK_STATUS_SIZE]; + int delay_us; + int try, max_tries = 20; + unsigned long deadline; + + /* + * Reset signal levels. Start transmitting 128b/132b TPS1. + * + * Put DPRX and LTTPRs (if any) into intra-hop AUX mode by writing TPS1 + * in DP_TRAINING_PATTERN_SET. + */ + if (!intel_dp_reset_link_train(intel_dp, crtc_state, DP_PHY_DPRX, + DP_TRAINING_PATTERN_1)) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to start 128b/132b TPS1\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux); + + /* Read the initial TX FFE settings. */ + if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to read TX FFE presets\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + /* Update signal levels and training set as requested. */ + intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status); + if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to set initial TX FFE settings\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + /* Start transmitting 128b/132b TPS2. */ + if (!intel_dp_set_link_train(intel_dp, crtc_state, DP_PHY_DPRX, + DP_TRAINING_PATTERN_2)) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to start 128b/132b TPS2\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + for (try = 0; try < max_tries; try++) { + usleep_range(delay_us, 2 * delay_us); + + /* + * The delay may get updated. The transmitter shall read the + * delay before link status during link training. + */ + delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux); + + if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to read link status\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + if (drm_dp_128b132b_link_training_failed(link_status)) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] Downstream link training failure\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + if (drm_dp_128b132b_lane_channel_eq_done(link_status, crtc_state->lane_count)) { + drm_dbg_kms(&i915->drm, + "[ENCODER:%d:%s] Lane channel eq done\n", + encoder->base.base.id, encoder->base.name); + break; + } + + /* Update signal levels and training set as requested. */ + intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status); + if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to update TX FFE settings\n", + encoder->base.base.id, encoder->base.name); + return false; + } + } + + if (try == max_tries) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] Max loop count reached\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + /* + * FIXME: This should probably be the total time budget for the complete + * LANEx_EQ_DONE Sequence, including the loop above and the aux rd + * intervals. + */ + deadline = jiffies + msecs_to_jiffies(400); + for (;;) { + if (drm_dp_128b132b_eq_interlane_align_done(link_status)) { + drm_dbg_kms(&i915->drm, + "[ENCODER:%d:%s] Interlane align done\n", + encoder->base.base.id, encoder->base.name); + break; + } + + if (time_after(jiffies, deadline)) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] Interlane align timeout\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + usleep_range(2000, 3000); + + if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to read link status\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + if (drm_dp_128b132b_link_training_failed(link_status)) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] Downstream link training failure\n", + encoder->base.base.id, encoder->base.name); + return false; + } + } + + return true; +} + +/* + * 128b/132b DP LANEx_CDS_DONE Sequence (DP 2.0 E11 3.5.2.16.2) + */ +static bool +intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state, + int lttpr_count) +{ + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; + struct drm_i915_private *i915 = to_i915(encoder->base.dev); + u8 link_status[DP_LINK_STATUS_SIZE]; + unsigned long deadline; + + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TRAINING_PATTERN_SET, + DP_TRAINING_PATTERN_2_CDS) != 1) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to start 128b/132b TPS2 CDS\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20); + for (;;) { + usleep_range(2000, 3000); + + if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) { + drm_err(&i915->drm, + "[ENCODER:%d:%s] Failed to read link status\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + if (drm_dp_128b132b_cds_interlane_align_done(link_status) && + drm_dp_128b132b_lane_symbol_locked(link_status, crtc_state->lane_count)) { + drm_dbg_kms(&i915->drm, + "[ENCODER:%d:%s] CDS interlane align done\n", + encoder->base.base.id, encoder->base.name); + break; + } + + if (drm_dp_128b132b_link_training_failed(link_status)) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] Downstream link training failure\n", + encoder->base.base.id, encoder->base.name); + return false; + } + + if (time_after(jiffies, deadline)) { + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_err(&i915->drm, + "[ENCODER:%d:%s] CDS timeout\n", + encoder->base.base.id, encoder->base.name); + return false; + } + } + + /* FIXME: Should DP_TRAINING_PATTERN_DISABLE be written first? */ + if (intel_dp->set_idle_link_train) + intel_dp->set_idle_link_train(intel_dp, crtc_state); + + return true; +} + +/* + * 128b/132b link training sequence. (DP 2.0 E11 SCR on link training.) + */ +static bool +intel_dp_128b132b_link_train(struct intel_dp *intel_dp, + const struct intel_crtc_state *crtc_state, + int lttpr_count) +{ + struct intel_connector *connector = intel_dp->attached_connector; + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; + bool passed = false; + + /* + * FIXME: Validate previous LT termination by reading Intra-Hop AUX + * Reply Indication by reading bit 3 of SINK_STATUS at 00205h + */ + + if (intel_dp_128b132b_lane_eq(intel_dp, crtc_state) && + intel_dp_128b132b_lane_cds(intel_dp, crtc_state, lttpr_count)) + passed = true; + + drm_dbg_kms(&dp_to_i915(intel_dp)->drm, + "[CONNECTOR:%d:%s][ENCODER:%d:%s] 128b/132b Link Training %s at link rate = %d, lane count = %d\n", + connector->base.base.id, connector->base.name, + encoder->base.base.id, encoder->base.name, + passed ? "passed" : "failed", + crtc_state->port_clock, crtc_state->lane_count); + + return passed; +} + /** * intel_dp_start_link_train - start link training * @intel_dp: DP struct @@ -1115,6 +1359,7 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, void intel_dp_start_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) { + static bool passed; /* * TODO: Reiniting LTTPRs here won't be needed once proper connector * HW state readout is added. @@ -1127,6 +1372,11 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
intel_dp_prepare_link_train(intel_dp, crtc_state);
- if (!intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count)) + if (intel_dp_is_uhbr(crtc_state)) + passed = intel_dp_128b132b_link_train(intel_dp, crtc_state, lttpr_count); + else + passed = intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count); + + if (!passed) intel_dp_schedule_fallback_link_training(intel_dp, crtc_state); }
On Tue, Jan 25, 2022 at 07:03:43PM +0200, Jani Nikula wrote:
The DP 2.0 errata completely overhauls the 128b/132b link training, with no provisions for backward compatibility with the original DP 2.0 specification.
The changes are too intrusive to consider reusing the same code for both 8b/10b and 128b/132b, mainly because the LTTPR channel equalisation is done concurrently instead of serialized.
NOTES:
It's a bit unclear when to wait for DP_INTERLANE_ALIGN_DONE and per-lane DP_LANE_SYMBOL_LOCKED. Figure xx4 in the SCR implies the LANEx_CHANNEL_EQ_DONE sequence may end with either 0x77,0x77,0x85 *or* 0x33,0x33,0x84 (for four lane configuration in DPCD 0x202..0x204) i.e. without the above bits set. Text elsewhere seems contradictory or incomplete.
We read entire link status (6 bytes) everywhere instead of individual DPCD addresses.
There are some subtle ambiguities or contradictions in the order of some DPCD access and TPS signal enables/disables. It's also not clear whether these are significant.
Cc: Uma Shankar uma.shankar@intel.com 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 | 252 +++++++++++++++++- 1 file changed, 251 insertions(+), 1 deletion(-)
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 4e507aa75a03..8bb6a296f421 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1102,6 +1102,250 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, return ret; }
+/*
- 128b/132b DP LANEx_EQ_DONE Sequence (DP 2.0 E11 3.5.2.16.1)
- */
+static bool +intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state)
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- u8 link_status[DP_LINK_STATUS_SIZE];
- int delay_us;
- int try, max_tries = 20;
- unsigned long deadline;
- /*
* Reset signal levels. Start transmitting 128b/132b TPS1.
*
* Put DPRX and LTTPRs (if any) into intra-hop AUX mode by writing TPS1
* in DP_TRAINING_PATTERN_SET.
*/
- if (!intel_dp_reset_link_train(intel_dp, crtc_state, DP_PHY_DPRX,
DP_TRAINING_PATTERN_1)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS1\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
- /* Read the initial TX FFE settings. */
- if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read TX FFE presets\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /* Update signal levels and training set as requested. */
- intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status);
- if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to set initial TX FFE settings\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /* Start transmitting 128b/132b TPS2. */
- if (!intel_dp_set_link_train(intel_dp, crtc_state, DP_PHY_DPRX,
DP_TRAINING_PATTERN_2)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS2\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- for (try = 0; try < max_tries; try++) {
usleep_range(delay_us, 2 * delay_us);
/*
* The delay may get updated. The transmitter shall read the
* delay before link status during link training.
*/
delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_lane_channel_eq_done(link_status, crtc_state->lane_count)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] Lane channel eq done\n",
encoder->base.base.id, encoder->base.name);
break;
}
/* Update signal levels and training set as requested. */
intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status);
if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to update TX FFE settings\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- if (try == max_tries) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Max loop count reached\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /*
* FIXME: This should probably be the total time budget for the complete
* LANEx_EQ_DONE Sequence, including the loop above and the aux rd
* intervals.
*/
That is what my spec reading skills tell me. I suspect it's just there to make sure some broken device can't specify an overly long aux rd interval and make the link training too long. 20*256ms=~5 seconds max.
I'd either drop this for now, or try to intergrate properly inte loop.
- deadline = jiffies + msecs_to_jiffies(400);
- for (;;) {
if (drm_dp_128b132b_eq_interlane_align_done(link_status)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] Interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (time_after(jiffies, deadline)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Interlane align timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
usleep_range(2000, 3000);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- return true;
+}
+/*
- 128b/132b DP LANEx_CDS_DONE Sequence (DP 2.0 E11 3.5.2.16.2)
- */
+static bool +intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state,
int lttpr_count)
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- u8 link_status[DP_LINK_STATUS_SIZE];
- unsigned long deadline;
- if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
DP_TRAINING_PATTERN_2_CDS) != 1) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS2 CDS\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20);
- for (;;) {
usleep_range(2000, 3000);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_cds_interlane_align_done(link_status) &&
drm_dp_128b132b_lane_symbol_locked(link_status, crtc_state->lane_count)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] CDS interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (time_after(jiffies, deadline)) {
This is racy. There's no telling how much time has passed since the last time we checked the status. Standard rule of timeouts: always check one last time after the timeout has expired.
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] CDS timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- /* FIXME: Should DP_TRAINING_PATTERN_DISABLE be written first? */
Not sure. The other thing missing is waiting for the intra-hop AUX indicator to go away after we exit link training.
It seems we should also block all other aux transfers during LT because of the intra-hop AUX mode. I can imagine things might get a bit confused if the LTTPRs start answering directly to AUX transfers that are intended to pass through to the DPRX.
- if (intel_dp->set_idle_link_train)
intel_dp->set_idle_link_train(intel_dp, crtc_state);
- return true;
+}
+/*
- 128b/132b link training sequence. (DP 2.0 E11 SCR on link training.)
- */
+static bool +intel_dp_128b132b_link_train(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state,
int lttpr_count)
+{
- struct intel_connector *connector = intel_dp->attached_connector;
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- bool passed = false;
- /*
* FIXME: Validate previous LT termination by reading Intra-Hop AUX
* Reply Indication by reading bit 3 of SINK_STATUS at 00205h
*/
- if (intel_dp_128b132b_lane_eq(intel_dp, crtc_state) &&
intel_dp_128b132b_lane_cds(intel_dp, crtc_state, lttpr_count))
passed = true;
- drm_dbg_kms(&dp_to_i915(intel_dp)->drm,
"[CONNECTOR:%d:%s][ENCODER:%d:%s] 128b/132b Link Training %s at link rate = %d, lane count = %d\n",
connector->base.base.id, connector->base.name,
encoder->base.base.id, encoder->base.name,
passed ? "passed" : "failed",
crtc_state->port_clock, crtc_state->lane_count);
- return passed;
+}
/**
- intel_dp_start_link_train - start link training
- @intel_dp: DP struct
@@ -1115,6 +1359,7 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, void intel_dp_start_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) {
- static bool passed; /*
- TODO: Reiniting LTTPRs here won't be needed once proper connector
- HW state readout is added.
@@ -1127,6 +1372,11 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
intel_dp_prepare_link_train(intel_dp, crtc_state);
- if (!intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count))
- if (intel_dp_is_uhbr(crtc_state))
passed = intel_dp_128b132b_link_train(intel_dp, crtc_state, lttpr_count);
- else
passed = intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count);
- if (!passed) intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
}
2.30.2
On Wed, 26 Jan 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jan 25, 2022 at 07:03:43PM +0200, Jani Nikula wrote:
The DP 2.0 errata completely overhauls the 128b/132b link training, with no provisions for backward compatibility with the original DP 2.0 specification.
The changes are too intrusive to consider reusing the same code for both 8b/10b and 128b/132b, mainly because the LTTPR channel equalisation is done concurrently instead of serialized.
NOTES:
It's a bit unclear when to wait for DP_INTERLANE_ALIGN_DONE and per-lane DP_LANE_SYMBOL_LOCKED. Figure xx4 in the SCR implies the LANEx_CHANNEL_EQ_DONE sequence may end with either 0x77,0x77,0x85 *or* 0x33,0x33,0x84 (for four lane configuration in DPCD 0x202..0x204) i.e. without the above bits set. Text elsewhere seems contradictory or incomplete.
We read entire link status (6 bytes) everywhere instead of individual DPCD addresses.
There are some subtle ambiguities or contradictions in the order of some DPCD access and TPS signal enables/disables. It's also not clear whether these are significant.
Cc: Uma Shankar uma.shankar@intel.com 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 | 252 +++++++++++++++++- 1 file changed, 251 insertions(+), 1 deletion(-)
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 4e507aa75a03..8bb6a296f421 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1102,6 +1102,250 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, return ret; }
+/*
- 128b/132b DP LANEx_EQ_DONE Sequence (DP 2.0 E11 3.5.2.16.1)
- */
+static bool +intel_dp_128b132b_lane_eq(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state)
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- u8 link_status[DP_LINK_STATUS_SIZE];
- int delay_us;
- int try, max_tries = 20;
- unsigned long deadline;
- /*
* Reset signal levels. Start transmitting 128b/132b TPS1.
*
* Put DPRX and LTTPRs (if any) into intra-hop AUX mode by writing TPS1
* in DP_TRAINING_PATTERN_SET.
*/
- if (!intel_dp_reset_link_train(intel_dp, crtc_state, DP_PHY_DPRX,
DP_TRAINING_PATTERN_1)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS1\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
- /* Read the initial TX FFE settings. */
- if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read TX FFE presets\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /* Update signal levels and training set as requested. */
- intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status);
- if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to set initial TX FFE settings\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /* Start transmitting 128b/132b TPS2. */
- if (!intel_dp_set_link_train(intel_dp, crtc_state, DP_PHY_DPRX,
DP_TRAINING_PATTERN_2)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS2\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- for (try = 0; try < max_tries; try++) {
usleep_range(delay_us, 2 * delay_us);
/*
* The delay may get updated. The transmitter shall read the
* delay before link status during link training.
*/
delay_us = drm_dp_128b132b_read_aux_rd_interval(&intel_dp->aux);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_lane_channel_eq_done(link_status, crtc_state->lane_count)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] Lane channel eq done\n",
encoder->base.base.id, encoder->base.name);
break;
}
/* Update signal levels and training set as requested. */
intel_dp_get_adjust_train(intel_dp, crtc_state, DP_PHY_DPRX, link_status);
if (!intel_dp_update_link_train(intel_dp, crtc_state, DP_PHY_DPRX)) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to update TX FFE settings\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- if (try == max_tries) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Max loop count reached\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- /*
* FIXME: This should probably be the total time budget for the complete
* LANEx_EQ_DONE Sequence, including the loop above and the aux rd
* intervals.
*/
That is what my spec reading skills tell me. I suspect it's just there to make sure some broken device can't specify an overly long aux rd interval and make the link training too long. 20*256ms=~5 seconds max.
I'd either drop this for now, or try to intergrate properly inte loop.
Extended it over the entire sequence.
- deadline = jiffies + msecs_to_jiffies(400);
- for (;;) {
if (drm_dp_128b132b_eq_interlane_align_done(link_status)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] Interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (time_after(jiffies, deadline)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Interlane align timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
usleep_range(2000, 3000);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- return true;
+}
+/*
- 128b/132b DP LANEx_CDS_DONE Sequence (DP 2.0 E11 3.5.2.16.2)
- */
+static bool +intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state,
int lttpr_count)
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- u8 link_status[DP_LINK_STATUS_SIZE];
- unsigned long deadline;
- if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
DP_TRAINING_PATTERN_2_CDS) != 1) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS2 CDS\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20);
- for (;;) {
usleep_range(2000, 3000);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_cds_interlane_align_done(link_status) &&
drm_dp_128b132b_lane_symbol_locked(link_status, crtc_state->lane_count)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] CDS interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (time_after(jiffies, deadline)) {
This is racy. There's no telling how much time has passed since the last time we checked the status. Standard rule of timeouts: always check one last time after the timeout has expired.
Right, thanks, fixed.
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] CDS timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- /* FIXME: Should DP_TRAINING_PATTERN_DISABLE be written first? */
Not sure. The other thing missing is waiting for the intra-hop AUX indicator to go away after we exit link training.
Hum, I guess I've missed that. I thought we should check for that before we start. (And yes, that's missing, there's another FIXME.)
It seems we should also block all other aux transfers during LT because of the intra-hop AUX mode. I can imagine things might get a bit confused if the LTTPRs start answering directly to AUX transfers that are intended to pass through to the DPRX.
Yeah, I've left that for the future. I haven't figured out a clean way to do it yet.
- if (intel_dp->set_idle_link_train)
intel_dp->set_idle_link_train(intel_dp, crtc_state);
- return true;
+}
+/*
- 128b/132b link training sequence. (DP 2.0 E11 SCR on link training.)
- */
+static bool +intel_dp_128b132b_link_train(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state,
int lttpr_count)
+{
- struct intel_connector *connector = intel_dp->attached_connector;
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- bool passed = false;
- /*
* FIXME: Validate previous LT termination by reading Intra-Hop AUX
* Reply Indication by reading bit 3 of SINK_STATUS at 00205h
*/
- if (intel_dp_128b132b_lane_eq(intel_dp, crtc_state) &&
intel_dp_128b132b_lane_cds(intel_dp, crtc_state, lttpr_count))
passed = true;
- drm_dbg_kms(&dp_to_i915(intel_dp)->drm,
"[CONNECTOR:%d:%s][ENCODER:%d:%s] 128b/132b Link Training %s at link rate = %d, lane count = %d\n",
connector->base.base.id, connector->base.name,
encoder->base.base.id, encoder->base.name,
passed ? "passed" : "failed",
crtc_state->port_clock, crtc_state->lane_count);
- return passed;
+}
/**
- intel_dp_start_link_train - start link training
- @intel_dp: DP struct
@@ -1115,6 +1359,7 @@ intel_dp_link_train_all_phys(struct intel_dp *intel_dp, void intel_dp_start_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state) {
- static bool passed; /*
- TODO: Reiniting LTTPRs here won't be needed once proper connector
- HW state readout is added.
@@ -1127,6 +1372,11 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp,
intel_dp_prepare_link_train(intel_dp, crtc_state);
- if (!intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count))
- if (intel_dp_is_uhbr(crtc_state))
passed = intel_dp_128b132b_link_train(intel_dp, crtc_state, lttpr_count);
- else
passed = intel_dp_link_train_all_phys(intel_dp, crtc_state, lttpr_count);
- if (!passed) intel_dp_schedule_fallback_link_training(intel_dp, crtc_state);
}
2.30.2
On Tue, Jan 25, 2022 at 07:03:43PM +0200, Jani Nikula wrote: <snip>
+static bool +intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp,
const struct intel_crtc_state *crtc_state,
int lttpr_count)
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- u8 link_status[DP_LINK_STATUS_SIZE];
- unsigned long deadline;
- if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
DP_TRAINING_PATTERN_2_CDS) != 1) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to start 128b/132b TPS2 CDS\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20);
- for (;;) {
usleep_range(2000, 3000);
if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) {
drm_err(&i915->drm,
"[ENCODER:%d:%s] Failed to read link status\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (drm_dp_128b132b_cds_interlane_align_done(link_status) &&
drm_dp_128b132b_lane_symbol_locked(link_status, crtc_state->lane_count)) {
I'm thinkin we want to check for both eq done and symbol locked here, just like we do with 8b10b.
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] CDS interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (time_after(jiffies, deadline)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] CDS timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- /* FIXME: Should DP_TRAINING_PATTERN_DISABLE be written first? */
- if (intel_dp->set_idle_link_train)
intel_dp->set_idle_link_train(intel_dp, crtc_state);
- return true;
+}
On Thu, 27 Jan 2022, Ville Syrjälä ville.syrjala@linux.intel.com wrote:
On Tue, Jan 25, 2022 at 07:03:43PM +0200, Jani Nikula wrote:
<snip> > +static bool > +intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp, > + const struct intel_crtc_state *crtc_state, > + int lttpr_count) > +{ > + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; > + struct drm_i915_private *i915 = to_i915(encoder->base.dev); > + u8 link_status[DP_LINK_STATUS_SIZE]; > + unsigned long deadline; > + > + if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TRAINING_PATTERN_SET, > + DP_TRAINING_PATTERN_2_CDS) != 1) { > + drm_err(&i915->drm, > + "[ENCODER:%d:%s] Failed to start 128b/132b TPS2 CDS\n", > + encoder->base.base.id, encoder->base.name); > + return false; > + } > + > + deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20); > + for (;;) { > + usleep_range(2000, 3000); > + > + if (drm_dp_dpcd_read_link_status(&intel_dp->aux, link_status) < 0) { > + drm_err(&i915->drm, > + "[ENCODER:%d:%s] Failed to read link status\n", > + encoder->base.base.id, encoder->base.name); > + return false; > + } > + > + if (drm_dp_128b132b_cds_interlane_align_done(link_status) && > + drm_dp_128b132b_lane_symbol_locked(link_status, crtc_state->lane_count)) {
I'm thinkin we want to check for both eq done and symbol locked here, just like we do with 8b10b.
I guess so, although I don't think the spec explicitly calls that out.
Fixed anyway.
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] CDS interlane align done\n",
encoder->base.base.id, encoder->base.name);
break;
}
if (drm_dp_128b132b_link_training_failed(link_status)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] Downstream link training failure\n",
encoder->base.base.id, encoder->base.name);
return false;
}
if (time_after(jiffies, deadline)) {
intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
drm_err(&i915->drm,
"[ENCODER:%d:%s] CDS timeout\n",
encoder->base.base.id, encoder->base.name);
return false;
}
- }
- /* FIXME: Should DP_TRAINING_PATTERN_DISABLE be written first? */
- if (intel_dp->set_idle_link_train)
intel_dp->set_idle_link_train(intel_dp, crtc_state);
- return true;
+}
Abstract link status check to a function that takes 128b/132b and 8b/10b into account, and use it. Also dump link status on failures.
Cc: Uma Shankar uma.shankar@intel.com 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 | 39 ++++++++++++++----- .../drm/i915/display/intel_dp_link_training.c | 2 +- .../drm/i915/display/intel_dp_link_training.h | 4 ++ 3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 4d4579a301f6..80fedd0e6212 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -3628,6 +3628,32 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) "Could not write test response to sink\n"); }
+static bool intel_dp_link_ok(struct intel_dp *intel_dp, + u8 link_status[DP_LINK_STATUS_SIZE]) +{ + struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base; + struct drm_i915_private *i915 = to_i915(encoder->base.dev); + bool uhbr = intel_dp->link_rate >= 1000000; + bool ok; + + if (uhbr) + ok = drm_dp_128b132b_lane_channel_eq_done(link_status, + intel_dp->lane_count); + else + ok = drm_dp_channel_eq_ok(link_status, intel_dp->lane_count); + + if (ok) + return true; + + intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status); + drm_dbg_kms(&i915->drm, + "[ENCODER:%d:%s] %s link not ok, retraining\n", + encoder->base.base.id, encoder->base.name, + uhbr ? "128b/132b" : "8b/10b"); + + return false; +} + static void intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, u8 *esi, u8 *ack) { @@ -3658,14 +3684,7 @@ static bool intel_dp_mst_link_status(struct intel_dp *intel_dp) return false; }
- if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) { - drm_dbg_kms(&i915->drm, - "[ENCODER:%d:%s] channel EQ not ok, retraining\n", - encoder->base.base.id, encoder->base.name); - return false; - } - - return true; + return intel_dp_link_ok(intel_dp, link_status); }
/** @@ -3779,8 +3798,8 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp) intel_dp->lane_count)) return false;
- /* Retrain if Channel EQ or CR not ok */ - return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count); + /* Retrain if link not ok */ + return !intel_dp_link_ok(intel_dp, link_status); }
static bool intel_dp_has_connector(struct intel_dp *intel_dp, 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 8bb6a296f421..1e41a560204a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -712,7 +712,7 @@ static bool intel_dp_adjust_request_changed(const struct intel_crtc_state *crtc_ return false; }
-static void +void intel_dp_dump_link_status(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy, const u8 link_status[DP_LINK_STATUS_SIZE]) { diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.h b/drivers/gpu/drm/i915/display/intel_dp_link_training.h index dbfb15705aaa..dc1556b46b85 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.h +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.h @@ -29,6 +29,10 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp, void intel_dp_stop_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state);
+void +intel_dp_dump_link_status(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy, + const u8 link_status[DP_LINK_STATUS_SIZE]); + /* Get the TPSx symbol type of the value programmed to DP_TRAINING_PATTERN_SET */ static inline u8 intel_dp_training_pattern_symbol(u8 pattern) {
On Tue, Jan 25, 2022 at 07:03:44PM +0200, Jani Nikula wrote:
Abstract link status check to a function that takes 128b/132b and 8b/10b into account, and use it. Also dump link status on failures.
Cc: Uma Shankar uma.shankar@intel.com 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 | 39 ++++++++++++++----- .../drm/i915/display/intel_dp_link_training.c | 2 +- .../drm/i915/display/intel_dp_link_training.h | 4 ++ 3 files changed, 34 insertions(+), 11 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp.c b/drivers/gpu/drm/i915/display/intel_dp.c index 4d4579a301f6..80fedd0e6212 100644 --- a/drivers/gpu/drm/i915/display/intel_dp.c +++ b/drivers/gpu/drm/i915/display/intel_dp.c @@ -3628,6 +3628,32 @@ static void intel_dp_handle_test_request(struct intel_dp *intel_dp) "Could not write test response to sink\n"); }
+static bool intel_dp_link_ok(struct intel_dp *intel_dp,
u8 link_status[DP_LINK_STATUS_SIZE])
+{
- struct intel_encoder *encoder = &dp_to_dig_port(intel_dp)->base;
- struct drm_i915_private *i915 = to_i915(encoder->base.dev);
- bool uhbr = intel_dp->link_rate >= 1000000;
- bool ok;
- if (uhbr)
ok = drm_dp_128b132b_lane_channel_eq_done(link_status,
intel_dp->lane_count);
That will only check the eq done bits. I think we want to keep the symbol locked checks as well.
- else
ok = drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
- if (ok)
return true;
- intel_dp_dump_link_status(intel_dp, DP_PHY_DPRX, link_status);
- drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] %s link not ok, retraining\n",
encoder->base.base.id, encoder->base.name,
uhbr ? "128b/132b" : "8b/10b");
- return false;
+}
static void intel_dp_mst_hpd_irq(struct intel_dp *intel_dp, u8 *esi, u8 *ack) { @@ -3658,14 +3684,7 @@ static bool intel_dp_mst_link_status(struct intel_dp *intel_dp) return false; }
- if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
drm_dbg_kms(&i915->drm,
"[ENCODER:%d:%s] channel EQ not ok, retraining\n",
encoder->base.base.id, encoder->base.name);
return false;
- }
- return true;
- return intel_dp_link_ok(intel_dp, link_status);
}
/** @@ -3779,8 +3798,8 @@ intel_dp_needs_link_retrain(struct intel_dp *intel_dp) intel_dp->lane_count)) return false;
- /* Retrain if Channel EQ or CR not ok */
- return !drm_dp_channel_eq_ok(link_status, intel_dp->lane_count);
- /* Retrain if link not ok */
- return !intel_dp_link_ok(intel_dp, link_status);
}
static bool intel_dp_has_connector(struct intel_dp *intel_dp, 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 8bb6a296f421..1e41a560204a 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -712,7 +712,7 @@ static bool intel_dp_adjust_request_changed(const struct intel_crtc_state *crtc_ return false; }
-static void +void intel_dp_dump_link_status(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy, const u8 link_status[DP_LINK_STATUS_SIZE]) { diff --git a/drivers/gpu/drm/i915/display/intel_dp_link_training.h b/drivers/gpu/drm/i915/display/intel_dp_link_training.h index dbfb15705aaa..dc1556b46b85 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.h +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.h @@ -29,6 +29,10 @@ void intel_dp_start_link_train(struct intel_dp *intel_dp, void intel_dp_stop_link_train(struct intel_dp *intel_dp, const struct intel_crtc_state *crtc_state);
+void +intel_dp_dump_link_status(struct intel_dp *intel_dp, enum drm_dp_phy dp_phy,
const u8 link_status[DP_LINK_STATUS_SIZE]);
/* Get the TPSx symbol type of the value programmed to DP_TRAINING_PATTERN_SET */ static inline u8 intel_dp_training_pattern_symbol(u8 pattern) { -- 2.30.2
Try to avoid the timeout during debugging.
Cc: Uma Shankar uma.shankar@intel.com 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_link_training.c | 4 ++++ 1 file changed, 4 insertions(+)
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 1e41a560204a..b1cf99fb3a2d 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_link_training.c +++ b/drivers/gpu/drm/i915/display/intel_dp_link_training.c @@ -1273,6 +1273,10 @@ intel_dp_128b132b_lane_cds(struct intel_dp *intel_dp, }
deadline = jiffies + msecs_to_jiffies((lttpr_count + 1) * 20); + + /* FIXME: Give some slack for CDS. */ + deadline += msecs_to_jiffies(500); + for (;;) { usleep_range(2000, 3000);
128b/132b supports using 64 slots starting from 0, while 8b/10b reserves slot 0 for metadata.
Commit d6c6a76f80a1 ("drm: Update MST First Link Slot Information Based on Encoding Format") added support for updating the topology state accordingly, and commit 41724ea273cd ("drm/amd/display: Add DP 2.0 MST DM Support") started using it in the amd driver.
This feels more than a little cumbersome, especially updating the information in atomic check. For i915, add the update to MST connector .compute_config hook rather than iterating over all MST managers and connectors in global mode config .atomic_check. Fingers crossed.
v2: - Update in .compute_config() not .atomic_check (Ville)
Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Lyude Paul lyude@redhat.com Cc: Uma Shankar uma.shankar@intel.com 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_mst.c | 29 +++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index b8bc7d397c81..ff75e22bde5c 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -99,6 +99,27 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; }
+static void intel_dp_mst_update_slots(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct drm_i915_private *i915 = to_i915(encoder->base.dev); + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); + struct intel_dp *intel_dp = &intel_mst->primary->dp; + struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr; + struct drm_dp_mst_topology_state *topology_state; + u8 link_coding_cap = intel_dp_is_uhbr(crtc_state) ? + DP_CAP_ANSI_128B132B : DP_CAP_ANSI_8B10B; + + topology_state = drm_atomic_get_mst_topology_state(conn_state->state, mgr); + if (IS_ERR(topology_state)) { + drm_dbg_kms(&i915->drm, "slot update failed\n"); + return; + } + + drm_dp_mst_update_slots(topology_state, link_coding_cap); +} + static int intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -155,6 +176,8 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, if (ret) return ret;
+ intel_dp_mst_update_slots(encoder, pipe_config, conn_state); + pipe_config->limited_color_range = intel_dp_limited_color_range(pipe_config, conn_state);
@@ -357,6 +380,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state, struct intel_connector *connector = to_intel_connector(old_conn_state->connector); struct drm_i915_private *i915 = to_i915(connector->base.dev); + int start_slot = intel_dp_is_uhbr(old_crtc_state) ? 0 : 1; int ret;
drm_dbg_kms(&i915->drm, "active links %d\n", @@ -366,7 +390,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state,
drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port);
- ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, start_slot); if (ret) { drm_dbg_kms(&i915->drm, "failed to update payload %d\n", ret); } @@ -475,6 +499,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_connector *connector = to_intel_connector(conn_state->connector); + int start_slot = intel_dp_is_uhbr(pipe_config) ? 0 : 1; int ret; bool first_mst_stream;
@@ -509,7 +534,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state,
intel_dp->active_mst_links++;
- ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, start_slot);
/* * Before Gen 12 this is not done as part of
Acked-by: Lyude Paul lyude@redhat.com
BTW - I made a ton of progress last week on getting all of this stuff moved into the atomic state :), mainly just trying to get amd hooked up with this now (and need to rebase):
https://gitlab.freedesktop.org/lyudess/linux/-/commits/wip/mst-atomic-only-v...
So we soon won't need this slots hack
On Tue, 2022-01-25 at 19:03 +0200, Jani Nikula wrote:
128b/132b supports using 64 slots starting from 0, while 8b/10b reserves slot 0 for metadata.
Commit d6c6a76f80a1 ("drm: Update MST First Link Slot Information Based on Encoding Format") added support for updating the topology state accordingly, and commit 41724ea273cd ("drm/amd/display: Add DP 2.0 MST DM Support") started using it in the amd driver.
This feels more than a little cumbersome, especially updating the information in atomic check. For i915, add the update to MST connector .compute_config hook rather than iterating over all MST managers and connectors in global mode config .atomic_check. Fingers crossed.
v2:
- Update in .compute_config() not .atomic_check (Ville)
Cc: Bhawanpreet Lakha Bhawanpreet.Lakha@amd.com Cc: Lyude Paul lyude@redhat.com Cc: Uma Shankar uma.shankar@intel.com 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_mst.c | 29 +++++++++++++++++++-- 1 file changed, 27 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/i915/display/intel_dp_mst.c b/drivers/gpu/drm/i915/display/intel_dp_mst.c index b8bc7d397c81..ff75e22bde5c 100644 --- a/drivers/gpu/drm/i915/display/intel_dp_mst.c +++ b/drivers/gpu/drm/i915/display/intel_dp_mst.c @@ -99,6 +99,27 @@ static int intel_dp_mst_compute_link_config(struct intel_encoder *encoder, return 0; } +static void intel_dp_mst_update_slots(struct intel_encoder *encoder, + struct intel_crtc_state *crtc_state, + struct drm_connector_state *conn_state) +{ + struct drm_i915_private *i915 = to_i915(encoder->base.dev); + struct intel_dp_mst_encoder *intel_mst = enc_to_mst(encoder); + struct intel_dp *intel_dp = &intel_mst->primary->dp; + struct drm_dp_mst_topology_mgr *mgr = &intel_dp->mst_mgr; + struct drm_dp_mst_topology_state *topology_state; + u8 link_coding_cap = intel_dp_is_uhbr(crtc_state) ? + DP_CAP_ANSI_128B132B : DP_CAP_ANSI_8B10B;
+ topology_state = drm_atomic_get_mst_topology_state(conn_state-
state, mgr);
+ if (IS_ERR(topology_state)) { + drm_dbg_kms(&i915->drm, "slot update failed\n"); + return; + }
+ drm_dp_mst_update_slots(topology_state, link_coding_cap); +}
static int intel_dp_mst_compute_config(struct intel_encoder *encoder, struct intel_crtc_state *pipe_config, struct drm_connector_state *conn_state) @@ -155,6 +176,8 @@ static int intel_dp_mst_compute_config(struct intel_encoder *encoder, if (ret) return ret; + intel_dp_mst_update_slots(encoder, pipe_config, conn_state);
pipe_config->limited_color_range = intel_dp_limited_color_range(pipe_config, conn_state); @@ -357,6 +380,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state, struct intel_connector *connector = to_intel_connector(old_conn_state->connector); struct drm_i915_private *i915 = to_i915(connector->base.dev); + int start_slot = intel_dp_is_uhbr(old_crtc_state) ? 0 : 1; int ret; drm_dbg_kms(&i915->drm, "active links %d\n", @@ -366,7 +390,7 @@ static void intel_mst_disable_dp(struct intel_atomic_state *state, drm_dp_mst_reset_vcpi_slots(&intel_dp->mst_mgr, connector->port); - ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, start_slot); if (ret) { drm_dbg_kms(&i915->drm, "failed to update payload %d\n", ret); } @@ -475,6 +499,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state, struct drm_i915_private *dev_priv = to_i915(encoder->base.dev); struct intel_connector *connector = to_intel_connector(conn_state->connector); + int start_slot = intel_dp_is_uhbr(pipe_config) ? 0 : 1; int ret; bool first_mst_stream; @@ -509,7 +534,7 @@ static void intel_mst_pre_enable_dp(struct intel_atomic_state *state, intel_dp->active_mst_links++; - ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, 1); + ret = drm_dp_update_payload_part1(&intel_dp->mst_mgr, start_slot); /* * Before Gen 12 this is not done as part of
dri-devel@lists.freedesktop.org