[PATCH 0/8] drm/dp, drm/i915: 128b/132b updates

[PATCH 0/8] drm/dp, drm/i915: 128b/132b updates

[Jani Nikula]

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(-)

-- 
2.30.2

[PATCH 1/8] drm/dp: add drm_dp_128b132b_read_aux_rd_interval()

[Jani Nikula]

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);
 
-- 
2.30.2

[PATCH 2/8] drm/dp: add 128b/132b link status helpers from DP 2.0 E11

[Jani Nikula]

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);
-- 
2.30.2

[PATCH 3/8] drm/dp: add some new DPCD macros from DP 2.0 E11

[Jani Nikula]

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,
-- 
2.30.2

[PATCH 4/8] drm/i915/dp: move intel_dp_prepare_link_train() call

[Jani Nikula]

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);
 }
-- 
2.30.2

[PATCH 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

[Jani Nikula]

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);
 }
-- 
2.30.2

[PATCH 6/8] drm/i915/dp: add 128b/132b support to link status checks

[Jani Nikula]

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)
 {
-- 
2.30.2

[PATCH 7/8] drm/i915/dp: give more time for CDS

[Jani Nikula]

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);
 
-- 
2.30.2

[PATCH 8/8] drm/i915/mst: update slot information for 128b/132b

[Jani Nikula]

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
-- 
2.30.2

Re: [PATCH 8/8] drm/i915/mst: update slot information for 128b/132b

[Lyude Paul]

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-v1

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

-- 
Cheers,
 Lyude Paul (she/her)
 Software Engineer at Red Hat

Re: [PATCH 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [PATCH 1/8] drm/dp: add drm_dp_128b132b_read_aux_rd_interval()

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [PATCH 3/8] drm/dp: add some new DPCD macros from DP 2.0 E11

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [PATCH 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

[Ville Syrjälä]

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;
> +}

-- 
Ville Syrjälä
Intel

Re: [PATCH 6/8] drm/i915/dp: add 128b/132b support to link status checks

[Ville Syrjälä]

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

-- 
Ville Syrjälä
Intel

Re: [PATCH 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

[Jani Nikula]

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

-- 
Jani Nikula, Intel Open Source Graphics Center

Re: [PATCH 5/8] drm/i915/dp: rewrite DP 2.0 128b/132b link training based on errata

[Jani Nikula]

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;
>> +}

-- 
Jani Nikula, Intel Open Source Graphics Center