drm/i915/dp: rest of refactoring, link rate fallback

PATCH In review Jani Nikula 5 participants 28 messages Apr 6, 2017 – Apr 13, 2017

Subsystems (2)

DRM drivers Intel i915

lore

Share & tools

lore

Jani Nikula

author

2017-04-06 13:44 UTC #0 lore raw

v4 cover drm/i915/dp: rest of refactoring, link rate fallback

v4 of [1], after patches 1-5 have been merged, and with Manasi's patches [2] and
[3] rebased on top.

BR,
Jani.

[1] http://mid.mail-archive.com/[email protected]
[2] http://patchwork.freedesktop.org/patch/msgid/[email protected]
[3] http://patchwork.freedesktop.org/patch/msgid/[email protected]

Jani Nikula (9):
drm/i915/dp: use the sink rates array for max sink rates drm/i915/dp: cache common rates with sink rates drm/i915/dp: do not limit rate seek when not needed drm/i915/dp: don't call the link parameters sink parameters drm/i915/dp: add functions for max common link rate and lane count drm/i915/mst: use max link not sink lane count drm/i915/dp: localize link rate index variable more drm/i915/dp: use readb and writeb calls for single byte DPCD access drm/i915/dp: read sink count to a temporary variable first

Manasi Navare (2):
drm/i915/dp: Validate cached link rate and lane count before
retraining
drm/i915: Implement Link Rate fallback on Link training failure

 drivers/gpu/drm/i915/intel_dp.c               | 236 +++++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  22 ++-
 drivers/gpu/drm/i915/intel_dp_mst.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h              |  15 +-
 4 files changed, 172 insertions(+), 105 deletions(-)

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #1 lore raw

PATCH v4 1/11 drm/i915/dp: use the sink rates array for max sink rates

Looking at DPCD DP_MAX_LINK_RATE may be completely bogus for eDP 1.4 which is allowed to use link rate select method and have 0 in max link rate. With this change, it makes sense to store the max rate as the actual rate rather than as a bw code.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 28 +++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h |  2 +-
 2 files changed, 8 insertions(+), 22 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +7 ~~-21~~

index b38cba7d5abc..e4650f1625cc 100644
@@ -161,23 +161,9 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
 	intel_dp->num_sink_rates = num_rates;
 }
 
-static int
-intel_dp_max_link_bw(struct intel_dp  *intel_dp)
+static int intel_dp_max_sink_rate(struct intel_dp *intel_dp)
 {
-	int max_link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
-
-	switch (max_link_bw) {
-	case DP_LINK_BW_1_62:
-	case DP_LINK_BW_2_7:
-	case DP_LINK_BW_5_4:
-		break;
-	default:
-		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
-		     max_link_bw);
-		max_link_bw = DP_LINK_BW_1_62;
-		break;
-	}
-	return max_link_bw;
+	return intel_dp->sink_rates[intel_dp->num_sink_rates - 1];
 }
 
 static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)

@@ -301,7 +287,7 @@ static int intel_dp_rate_index(const int *rates, int len, int rate)
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	int max_rate = drm_dp_bw_code_to_link_rate(intel_dp->max_sink_link_bw);
+	int max_rate = intel_dp->max_sink_link_rate;
 	int i, common_len;
 
 	common_len = intersect_rates(intel_dp->source_rates,

@@ -339,10 +325,10 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 						   common_rates,
 						   link_rate);
 	if (link_rate_index > 0) {
-		intel_dp->max_sink_link_bw = drm_dp_link_rate_to_bw_code(common_rates[link_rate_index - 1]);
+		intel_dp->max_sink_link_rate = common_rates[link_rate_index - 1];
 		intel_dp->max_sink_lane_count = lane_count;
 	} else if (lane_count > 1) {
-		intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp);
+		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
 		intel_dp->max_sink_lane_count = lane_count >> 1;
 	} else {
 		DRM_ERROR("Link Training Unsuccessful\n");

@@ -4652,8 +4638,8 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		/* Set the max lane count for sink */
 		intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
 
-		/* Set the max link BW for sink */
-		intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp);
+		/* Set the max link rate for sink */
+		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
 
 		intel_dp->reset_link_params = false;
 	}

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 7bc0c25b3396..92e353dc61bf 100644
@@ -959,7 +959,7 @@ struct intel_dp {
 	/* Max lane count for the sink as per DPCD registers */
 	uint8_t max_sink_lane_count;
 	/* Max link BW for the sink as per DPCD registers */
-	int max_sink_link_bw;
+	int max_sink_link_rate;
 	/* sink or branch descriptor */
 	struct intel_dp_desc desc;
 	struct drm_dp_aux aux;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #3 lore raw

PATCH v4 2/11 drm/i915/dp: cache common rates with sink rates

In reply to Jani Nikula

Now that source rates are static and sink rates are updated whenever DPCD is updated, we can do and cache the intersection of them whenever sink rates are updated. This reduces code complexity, as we don't have to keep calling the functions to intersect. We also get rid of several common rates arrays on stack.

Limiting the common rates by a max link rate can be done by picking the first N elements of the cached common rates.

v2: get rid of the local common_rates variable (Manasi)
v3: don't clobber cached eDP rates on short pulse (Ville)

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 75 ++++++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h |  3 ++
 2 files changed, 45 insertions(+), 33 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +42 ~~-33~~

index e4650f1625cc..1808af6d635d 100644
@@ -284,17 +284,29 @@ static int intel_dp_rate_index(const int *rates, int len, int rate)
 	return -1;
 }
 
-static int intel_dp_common_rates(struct intel_dp *intel_dp,
-				 int *common_rates)
+static void intel_dp_set_common_rates(struct intel_dp *intel_dp)
 {
-	int max_rate = intel_dp->max_sink_link_rate;
-	int i, common_len;
+	WARN_ON(!intel_dp->num_source_rates || !intel_dp->num_sink_rates);
 
-	common_len = intersect_rates(intel_dp->source_rates,
-				     intel_dp->num_source_rates,
-				     intel_dp->sink_rates,
-				     intel_dp->num_sink_rates,
-				     common_rates);
+	intel_dp->num_common_rates = intersect_rates(intel_dp->source_rates,
+						     intel_dp->num_source_rates,
+						     intel_dp->sink_rates,
+						     intel_dp->num_sink_rates,
+						     intel_dp->common_rates);
+
+	/* Paranoia, there should always be something in common. */
+	if (WARN_ON(intel_dp->num_common_rates == 0)) {
+		intel_dp->common_rates[0] = default_rates[0];
+		intel_dp->num_common_rates = 1;
+	}
+}
+
+/* get length of common rates potentially limited by max_rate */
+static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
+					  int max_rate)
+{
+	const int *common_rates = intel_dp->common_rates;
+	int i, common_len = intel_dp->num_common_rates;
 
 	/* Limit results by potentially reduced max rate */
 	for (i = 0; i < common_len; i++) {

@@ -305,25 +317,23 @@ static int intel_dp_common_rates(struct intel_dp *intel_dp,
 	return 0;
 }
 
-static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
-				    int *common_rates, int link_rate)
+static int intel_dp_link_rate_index(struct intel_dp *intel_dp, int link_rate)
 {
 	int common_len;
 
-	common_len = intel_dp_common_rates(intel_dp, common_rates);
+	common_len = intel_dp_common_len_rate_limit(intel_dp,
+						    intel_dp->max_sink_link_rate);
 
-	return intel_dp_rate_index(common_rates, common_len, link_rate);
+	return intel_dp_rate_index(intel_dp->common_rates, common_len, link_rate);
 }
 
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {
-	int common_rates[DP_MAX_SUPPORTED_RATES];
+	const int *common_rates = intel_dp->common_rates;
 	int link_rate_index;
 
-	link_rate_index = intel_dp_link_rate_index(intel_dp,
-						   common_rates,
-						   link_rate);
+	link_rate_index = intel_dp_link_rate_index(intel_dp, link_rate);
 	if (link_rate_index > 0) {
 		intel_dp->max_sink_link_rate = common_rates[link_rate_index - 1];
 		intel_dp->max_sink_lane_count = lane_count;

@@ -1506,8 +1516,6 @@ static void snprintf_int_array(char *str, size_t len,
 
 static void intel_dp_print_rates(struct intel_dp *intel_dp)
 {
-	int common_len;
-	int common_rates[DP_MAX_SUPPORTED_RATES];
 	char str[128]; /* FIXME: too big for stack? */
 
 	if ((drm_debug & DRM_UT_KMS) == 0)

@@ -1521,8 +1529,8 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 			   intel_dp->sink_rates, intel_dp->num_sink_rates);
 	DRM_DEBUG_KMS("sink rates: %s\n", str);
 
-	common_len = intel_dp_common_rates(intel_dp, common_rates);
-	snprintf_int_array(str, sizeof(str), common_rates, common_len);
+	snprintf_int_array(str, sizeof(str),
+			   intel_dp->common_rates, intel_dp->num_common_rates);
 	DRM_DEBUG_KMS("common rates: %s\n", str);
 }

@@ -1560,14 +1568,14 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
 int
 intel_dp_max_link_rate(struct intel_dp *intel_dp)
 {
-	int rates[DP_MAX_SUPPORTED_RATES] = {};
 	int len;
 
-	len = intel_dp_common_rates(intel_dp, rates);
+	len = intel_dp_common_len_rate_limit(intel_dp,
+					     intel_dp->max_sink_link_rate);
 	if (WARN_ON(len <= 0))
 		return 162000;
 
-	return rates[len - 1];
+	return intel_dp->common_rates[len - 1];
 }
 
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)

@@ -1636,11 +1644,11 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int link_rate_index;
 	int bpp, mode_rate;
 	int link_avail, link_clock;
-	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
 	int common_len;
 	uint8_t link_bw, rate_select;
 
-	common_len = intel_dp_common_rates(intel_dp, common_rates);
+	common_len = intel_dp_common_len_rate_limit(intel_dp,
+						    intel_dp->max_sink_link_rate);
 
 	/* No common link rates between source and sink */
 	WARN_ON(common_len <= 0);

@@ -1678,7 +1686,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
 		link_rate_index = intel_dp_link_rate_index(intel_dp,
-							   common_rates,
 							   intel_dp->compliance.test_link_rate);
 		if (link_rate_index >= 0)
 			min_clock = max_clock = link_rate_index;

@@ -1686,7 +1693,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	}
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max bw %d pixel clock %iKHz\n",
-		      max_lane_count, common_rates[max_clock],
+		      max_lane_count, intel_dp->common_rates[max_clock],
 		      adjusted_mode->crtc_clock);
 
 	/* Walk through all bpp values. Luckily they're all nicely spaced with 2

@@ -1722,7 +1729,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 				lane_count <= max_lane_count;
 				lane_count <<= 1) {
 
-				link_clock = common_rates[clock];
+				link_clock = intel_dp->common_rates[clock];
 				link_avail = intel_dp_max_data_rate(link_clock,
 								    lane_count);

@@ -1754,7 +1761,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->lane_count = lane_count;
 
 	pipe_config->pipe_bpp = bpp;
-	pipe_config->port_clock = common_rates[clock];
+	pipe_config->port_clock = intel_dp->common_rates[clock];
 
 	intel_dp_compute_rate(intel_dp, pipe_config->port_clock,
 			      &link_bw, &rate_select);

@@ -3715,6 +3722,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 	else
 		intel_dp_set_sink_rates(intel_dp);
 
+	intel_dp_set_common_rates(intel_dp);
+
 	return true;
 }

@@ -3726,8 +3735,10 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		return false;
 
 	/* Don't clobber cached eDP rates. */
-	if (!is_edp(intel_dp))
+	if (!is_edp(intel_dp)) {
 		intel_dp_set_sink_rates(intel_dp);
+		intel_dp_set_common_rates(intel_dp);
+	}
 
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
 			     &intel_dp->sink_count, 1) < 0)

@@ -3950,7 +3961,6 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 {
 	int status = 0;
 	int min_lane_count = 1;
-	int common_rates[DP_MAX_SUPPORTED_RATES] = {};
 	int link_rate_index, test_link_rate;
 	uint8_t test_lane_count, test_link_bw;
 	/* (DP CTS 1.2)

@@ -3979,7 +3989,6 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 	/* Validate the requested link rate */
 	test_link_rate = drm_dp_bw_code_to_link_rate(test_link_bw);
 	link_rate_index = intel_dp_link_rate_index(intel_dp,
-						   common_rates,
 						   test_link_rate);
 	if (link_rate_index < 0)
 		return DP_TEST_NAK;

drivers/gpu/drm/i915/intel_drv.h +3 -0

index 92e353dc61bf..4a4bf9cb0d90 100644
@@ -956,6 +956,9 @@ struct intel_dp {
 	int num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
 	bool use_rate_select;
+	/* intersection of source and sink rates */
+	int num_common_rates;
+	int common_rates[DP_MAX_SUPPORTED_RATES];
 	/* Max lane count for the sink as per DPCD registers */
 	uint8_t max_sink_lane_count;
 	/* Max link BW for the sink as per DPCD registers */

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #4 lore raw

PATCH v4 3/11 drm/i915/dp: do not limit rate seek when not needed

In reply to Jani Nikula

In link training fallback, we're trying to find a rate that we know is in a sorted array of common link rates. We don't need to limit the array using the max rate. For test request, the DP CTS doesn't say we should limit the rate based on earlier fallback. This lets us get rid of intel_dp_link_rate_index() and use intel_dp_rate_index() instead.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 31 ++++++++++++-------------------
 1 file changed, 12 insertions(+), 19 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +12 ~~-19~~

index 1808af6d635d..8c061c54d481 100644
@@ -317,25 +317,16 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
-static int intel_dp_link_rate_index(struct intel_dp *intel_dp, int link_rate)
-{
-	int common_len;
-
-	common_len = intel_dp_common_len_rate_limit(intel_dp,
-						    intel_dp->max_sink_link_rate);
-
-	return intel_dp_rate_index(intel_dp->common_rates, common_len, link_rate);
-}
-
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {
-	const int *common_rates = intel_dp->common_rates;
-	int link_rate_index;
+	int index;
 
-	link_rate_index = intel_dp_link_rate_index(intel_dp, link_rate);
-	if (link_rate_index > 0) {
-		intel_dp->max_sink_link_rate = common_rates[link_rate_index - 1];
+	index = intel_dp_rate_index(intel_dp->common_rates,
+				    intel_dp->num_common_rates,
+				    link_rate);
+	if (index > 0) {
+		intel_dp->max_sink_link_rate = intel_dp->common_rates[index - 1];
 		intel_dp->max_sink_lane_count = lane_count;
 	} else if (lane_count > 1) {
 		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);

@@ -1685,8 +1676,9 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		link_rate_index = intel_dp_link_rate_index(intel_dp,
-							   intel_dp->compliance.test_link_rate);
+		link_rate_index = intel_dp_rate_index(intel_dp->common_rates,
+						      intel_dp->num_common_rates,
+						      intel_dp->compliance.test_link_rate);
 		if (link_rate_index >= 0)
 			min_clock = max_clock = link_rate_index;
 		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;

@@ -3988,8 +3980,9 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 	}
 	/* Validate the requested link rate */
 	test_link_rate = drm_dp_bw_code_to_link_rate(test_link_bw);
-	link_rate_index = intel_dp_link_rate_index(intel_dp,
-						   test_link_rate);
+	link_rate_index = intel_dp_rate_index(intel_dp->common_rates,
+					      intel_dp->num_common_rates,
+					      test_link_rate);
 	if (link_rate_index < 0)
 		return DP_TEST_NAK;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #5 lore raw

PATCH v4 4/11 drm/i915/dp: don't call the link parameters sink parameters

In reply to Jani Nikula

If we modify these on the fly depending on the link conditions, don't pretend they are sink properties.

Some link vs. sink confusion still remains, but we'll take care of them in follow-up patches.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c  | 25 ++++++++++++-------------
 drivers/gpu/drm/i915/intel_drv.h |  8 ++++----
 2 files changed, 16 insertions(+), 17 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +12 ~~-13~~

index 8c061c54d481..a0082a3784e8 100644
@@ -172,7 +172,7 @@ static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
 	u8 source_max, sink_max;
 
 	source_max = intel_dig_port->max_lanes;
-	sink_max = intel_dp->max_sink_lane_count;
+	sink_max = intel_dp->max_link_lane_count;
 
 	return min(source_max, sink_max);
 }

@@ -326,11 +326,11 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 				    intel_dp->num_common_rates,
 				    link_rate);
 	if (index > 0) {
-		intel_dp->max_sink_link_rate = intel_dp->common_rates[index - 1];
-		intel_dp->max_sink_lane_count = lane_count;
+		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
+		intel_dp->max_link_lane_count = lane_count;
 	} else if (lane_count > 1) {
-		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
-		intel_dp->max_sink_lane_count = lane_count >> 1;
+		intel_dp->max_link_rate = intel_dp_max_sink_rate(intel_dp);
+		intel_dp->max_link_lane_count = lane_count >> 1;
 	} else {
 		DRM_ERROR("Link Training Unsuccessful\n");
 		return -1;

@@ -1561,8 +1561,7 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
 {
 	int len;
 
-	len = intel_dp_common_len_rate_limit(intel_dp,
-					     intel_dp->max_sink_link_rate);
+	len = intel_dp_common_len_rate_limit(intel_dp, intel_dp->max_link_rate);
 	if (WARN_ON(len <= 0))
 		return 162000;

@@ -1639,7 +1638,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	uint8_t link_bw, rate_select;
 
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
-						    intel_dp->max_sink_link_rate);
+						    intel_dp->max_link_rate);
 
 	/* No common link rates between source and sink */
 	WARN_ON(common_len <= 0);

@@ -3969,7 +3968,7 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 	test_lane_count &= DP_MAX_LANE_COUNT_MASK;
 	/* Validate the requested lane count */
 	if (test_lane_count < min_lane_count ||
-	    test_lane_count > intel_dp->max_sink_lane_count)
+	    test_lane_count > intel_dp->max_link_lane_count)
 		return DP_TEST_NAK;
 
 	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_LINK_RATE,

@@ -4637,11 +4636,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		      yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
 
 	if (intel_dp->reset_link_params) {
-		/* Set the max lane count for sink */
-		intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+		/* Set the max lane count for link */
+		intel_dp->max_link_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
 
-		/* Set the max link rate for sink */
-		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
+		/* Set the max link rate for link */
+		intel_dp->max_link_rate = intel_dp_max_sink_rate(intel_dp);
 
 		intel_dp->reset_link_params = false;
 	}

drivers/gpu/drm/i915/intel_drv.h +4 -4

index 4a4bf9cb0d90..f97603b74a28 100644
@@ -959,10 +959,10 @@ struct intel_dp {
 	/* intersection of source and sink rates */
 	int num_common_rates;
 	int common_rates[DP_MAX_SUPPORTED_RATES];
-	/* Max lane count for the sink as per DPCD registers */
-	uint8_t max_sink_lane_count;
-	/* Max link BW for the sink as per DPCD registers */
-	int max_sink_link_rate;
+	/* Max lane count for the current link */
+	int max_link_lane_count;
+	/* Max rate for the current link */
+	int max_link_rate;
 	/* sink or branch descriptor */
 	struct intel_dp_desc desc;
 	struct drm_dp_aux aux;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #6 lore raw

PATCH v4 5/11 drm/i915/dp: add functions for max common link rate and lane count

In reply to Jani Nikula

These are the theoretical maximums common for source and sink. These are the maximums we should start with. They may be degraded in case of link training failures, and the dynamic link values are stored separately.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 29 +++++++++++++++++------------
 1 file changed, 17 insertions(+), 12 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +17 ~~-12~~

index a0082a3784e8..b3df2082eac9 100644
@@ -161,22 +161,27 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
 	intel_dp->num_sink_rates = num_rates;
 }
 
-static int intel_dp_max_sink_rate(struct intel_dp *intel_dp)
+/* Theoretical max between source and sink */
+static int intel_dp_max_common_rate(struct intel_dp *intel_dp)
 {
-	return intel_dp->sink_rates[intel_dp->num_sink_rates - 1];
+	return intel_dp->common_rates[intel_dp->num_common_rates - 1];
 }
 
-static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)
+/* Theoretical max between source and sink */
+static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
-	u8 source_max, sink_max;
-
-	source_max = intel_dig_port->max_lanes;
-	sink_max = intel_dp->max_link_lane_count;
+	int source_max = intel_dig_port->max_lanes;
+	int sink_max = drm_dp_max_lane_count(intel_dp->dpcd);
 
 	return min(source_max, sink_max);
 }
 
+static int intel_dp_max_lane_count(struct intel_dp *intel_dp)
+{
+	return intel_dp->max_link_lane_count;
+}
+
 int
 intel_dp_link_required(int pixel_clock, int bpp)
 {

@@ -329,7 +334,7 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 		intel_dp->max_link_rate = intel_dp->common_rates[index - 1];
 		intel_dp->max_link_lane_count = lane_count;
 	} else if (lane_count > 1) {
-		intel_dp->max_link_rate = intel_dp_max_sink_rate(intel_dp);
+		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
 		intel_dp->max_link_lane_count = lane_count >> 1;
 	} else {
 		DRM_ERROR("Link Training Unsuccessful\n");

@@ -4636,11 +4641,11 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		      yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
 
 	if (intel_dp->reset_link_params) {
-		/* Set the max lane count for link */
-		intel_dp->max_link_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+		/* Initial max link lane count */
+		intel_dp->max_link_lane_count = intel_dp_max_common_lane_count(intel_dp);
 
-		/* Set the max link rate for link */
-		intel_dp->max_link_rate = intel_dp_max_sink_rate(intel_dp);
+		/* Initial max link rate */
+		intel_dp->max_link_rate = intel_dp_max_common_rate(intel_dp);
 
 		intel_dp->reset_link_params = false;
 	}

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #7 lore raw

PATCH v4 6/11 drm/i915/mst: use max link not sink lane count

In reply to Jani Nikula

The source might not support as many lanes as the sink, or the link training might have failed at higher lane counts. Take these into account.

Cc Dhinakaran Pandiyan <[email protected]>

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c     | 2 +-
 drivers/gpu/drm/i915/intel_dp_mst.c | 4 ++--
 drivers/gpu/drm/i915/intel_drv.h    | 1 +
 3 files changed, 4 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +1 -1

index b3df2082eac9..95f2278700e3 100644
@@ -177,7 +177,7 @@ static int intel_dp_max_common_lane_count(struct intel_dp *intel_dp)
 	return min(source_max, sink_max);
 }
 
-static int intel_dp_max_lane_count(struct intel_dp *intel_dp)
+int intel_dp_max_lane_count(struct intel_dp *intel_dp)
 {
 	return intel_dp->max_link_lane_count;
 }

drivers/gpu/drm/i915/intel_dp_mst.c +2 -2

index c1f62eb07c07..3451e2abb23b 100644
@@ -56,7 +56,7 @@ static bool intel_dp_mst_compute_config(struct intel_encoder *encoder,
 	 * for MST we always configure max link bw - the spec doesn't
 	 * seem to suggest we should do otherwise.
 	 */
-	lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
+	lane_count = intel_dp_max_lane_count(intel_dp);
 
 	pipe_config->lane_count = lane_count;

@@ -343,7 +343,7 @@ intel_dp_mst_mode_valid(struct drm_connector *connector,
 	int max_rate, mode_rate, max_lanes, max_link_clock;
 
 	max_link_clock = intel_dp_max_link_rate(intel_dp);
-	max_lanes = drm_dp_max_lane_count(intel_dp->dpcd);
+	max_lanes = intel_dp_max_lane_count(intel_dp);
 
 	max_rate = intel_dp_max_data_rate(max_link_clock, max_lanes);
 	mode_rate = intel_dp_link_required(mode->clock, bpp);

drivers/gpu/drm/i915/intel_drv.h +1 -0

index f97603b74a28..5f6e1aad909d 100644
@@ -1503,6 +1503,7 @@ void intel_dp_add_properties(struct intel_dp *intel_dp, struct drm_connector *co
 void intel_dp_mst_suspend(struct drm_device *dev);
 void intel_dp_mst_resume(struct drm_device *dev);
 int intel_dp_max_link_rate(struct intel_dp *intel_dp);
+int intel_dp_max_lane_count(struct intel_dp *intel_dp);
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate);
 void intel_dp_hot_plug(struct intel_encoder *intel_encoder);
 void intel_power_sequencer_reset(struct drm_i915_private *dev_priv);

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #8 lore raw

PATCH v4 7/11 drm/i915/dp: localize link rate index variable more

In reply to Jani Nikula

Localize link_rate_index to the if block, and rename to just index to reduce indent.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 13 +++++++------
 1 file changed, 7 insertions(+), 6 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +7 -6

index 95f2278700e3..6f743490855b 100644
@@ -1636,7 +1636,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	/* Conveniently, the link BW constants become indices with a shift...*/
 	int min_clock = 0;
 	int max_clock;
-	int link_rate_index;
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_len;

@@ -1680,11 +1679,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		link_rate_index = intel_dp_rate_index(intel_dp->common_rates,
-						      intel_dp->num_common_rates,
-						      intel_dp->compliance.test_link_rate);
-		if (link_rate_index >= 0)
-			min_clock = max_clock = link_rate_index;
+		int index;
+
+		index = intel_dp_rate_index(intel_dp->common_rates,
+					    intel_dp->num_common_rates,
+					    intel_dp->compliance.test_link_rate);
+		if (index >= 0)
+			min_clock = max_clock = index;
 		min_lane_count = max_lane_count = intel_dp->compliance.test_lane_count;
 	}
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #9 lore raw

PATCH v4 8/11 drm/i915/dp: use readb and writeb calls for single byte DPCD access

In reply to Jani Nikula

This is what we have the readb and writeb variants for. Do some minor return value and variable cleanup while at it.

Cc Manasi Navare <[email protected]>

Cc Ville Syrjälä <[email protected]>

Reviewed-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 37 +++++++++++++++++--------------------
 1 file changed, 17 insertions(+), 20 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +17 ~~-20~~

index 6f743490855b..81682fd2804b 100644
@@ -3661,9 +3661,9 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 		uint8_t frame_sync_cap;
 
 		dev_priv->psr.sink_support = true;
-		drm_dp_dpcd_read(&intel_dp->aux,
-				 DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
-				 &frame_sync_cap, 1);
+		drm_dp_dpcd_readb(&intel_dp->aux,
+				  DP_SINK_DEVICE_AUX_FRAME_SYNC_CAP,
+				  &frame_sync_cap);
 		dev_priv->psr.aux_frame_sync = frame_sync_cap ? true : false;
 		/* PSR2 needs frame sync as well */
 		dev_priv->psr.psr2_support = dev_priv->psr.aux_frame_sync;

@@ -3737,8 +3737,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		intel_dp_set_common_rates(intel_dp);
 	}
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
-			     &intel_dp->sink_count, 1) < 0)
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
+			      &intel_dp->sink_count) <= 0)
 		return false;
 
 	/*

@@ -3775,7 +3775,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 static bool
 intel_dp_can_mst(struct intel_dp *intel_dp)
 {
-	u8 buf[1];
+	u8 mstm_cap;
 
 	if (!i915.enable_dp_mst)
 		return false;

@@ -3786,10 +3786,10 @@ intel_dp_can_mst(struct intel_dp *intel_dp)
 	if (intel_dp->dpcd[DP_DPCD_REV] < 0x12)
 		return false;
 
-	if (drm_dp_dpcd_read(&intel_dp->aux, DP_MSTM_CAP, buf, 1) != 1)
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_MSTM_CAP, &mstm_cap) != 1)
 		return false;
 
-	return buf[0] & DP_MST_CAP;
+	return mstm_cap & DP_MST_CAP;
 }
 
 static void

@@ -3935,9 +3935,8 @@ int intel_dp_sink_crc(struct intel_dp *intel_dp, u8 *crc)
 static bool
 intel_dp_get_sink_irq(struct intel_dp *intel_dp, u8 *sink_irq_vector)
 {
-	return drm_dp_dpcd_read(&intel_dp->aux,
-				       DP_DEVICE_SERVICE_IRQ_VECTOR,
-				       sink_irq_vector, 1) == 1;
+	return drm_dp_dpcd_readb(&intel_dp->aux, DP_DEVICE_SERVICE_IRQ_VECTOR,
+				 sink_irq_vector) == 1;
 }
 
 static bool

@@ -4000,13 +3999,13 @@ static uint8_t intel_dp_autotest_link_training(struct intel_dp *intel_dp)
 static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 {
 	uint8_t test_pattern;
-	uint16_t test_misc;
+	uint8_t test_misc;
 	__be16 h_width, v_height;
 	int status = 0;
 
 	/* Read the TEST_PATTERN (DP CTS 3.1.5) */
-	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_PATTERN,
-				  &test_pattern, 1);
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_PATTERN,
+				   &test_pattern);
 	if (status <= 0) {
 		DRM_DEBUG_KMS("Test pattern read failed\n");
 		return DP_TEST_NAK;

@@ -4028,8 +4027,8 @@ static uint8_t intel_dp_autotest_video_pattern(struct intel_dp *intel_dp)
 		return DP_TEST_NAK;
 	}
 
-	status = drm_dp_dpcd_read(&intel_dp->aux, DP_TEST_MISC0,
-				  &test_misc, 1);
+	status = drm_dp_dpcd_readb(&intel_dp->aux, DP_TEST_MISC0,
+				   &test_misc);
 	if (status <= 0) {
 		DRM_DEBUG_KMS("TEST MISC read failed\n");
 		return DP_TEST_NAK;

@@ -4088,10 +4087,8 @@ static uint8_t intel_dp_autotest_edid(struct intel_dp *intel_dp)
 		 */
 		block += intel_connector->detect_edid->extensions;
 
-		if (!drm_dp_dpcd_write(&intel_dp->aux,
-					DP_TEST_EDID_CHECKSUM,
-					&block->checksum,
-					1))
+		if (drm_dp_dpcd_writeb(&intel_dp->aux, DP_TEST_EDID_CHECKSUM,
+				       block->checksum) <= 0)
 			DRM_DEBUG_KMS("Failed to write EDID checksum\n");
 
 		test_result = DP_TEST_ACK | DP_TEST_EDID_CHECKSUM_WRITE;

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #10 lore raw

PATCH v4 9/11 drm/i915/dp: read sink count to a temporary variable first

In reply to Jani Nikula

Don't clobber intel_dp->sink_count with the raw value.

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +4 -3

index 81682fd2804b..3c5c80da9ea3 100644
@@ -3728,6 +3728,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 static bool
 intel_dp_get_dpcd(struct intel_dp *intel_dp)
 {
+	u8 sink_count;
+
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;

@@ -3737,8 +3739,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		intel_dp_set_common_rates(intel_dp);
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
-			      &intel_dp->sink_count) <= 0)
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0)
 		return false;
 
 	/*

@@ -3746,7 +3747,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	 * a member variable in intel_dp will track any changes
 	 * between short pulse interrupts.
 	 */
-	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+	intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count);
 
 	/*
 	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #14 lore raw

PATCH v4 10/11 drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Jani Nikula

From: Manasi Navare <[email protected]>

Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.

This patch validates these values against the common_rates and max lane count values.

This is absolutely required incase the common_rates or max lane count are now different due to link fallback.

v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)

v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes

Cc Ville Syrjala <[email protected]>

Cc Jani Nikula <[email protected]>

Signed-off-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +27 -3

index 3c5c80da9ea3..6915d89a238c 100644
@@ -322,6 +322,28 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	int index;
+
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
+	index = intel_dp_rate_index(intel_dp->common_rates,
+				    intel_dp->num_common_rates,
+				    intel_dp->link_rate);
+	if (index < 0)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4275,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-06 13:44 UTC #22 lore raw

PATCH v4 11/11 drm/i915: Implement Link Rate fallback on Link training failure

In reply to Jani Nikula

From: Manasi Navare <[email protected]>

If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed, update the link status property to "BAD" and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it.

This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6.

This patch is a resend of the original commit id (233ce881dd91fb "drm/i915: Implement Link Rate fallback on Link training failure") which got reverted in this commit id (afc1ebf4562a14 Revert "drm/i915: Implement Link Rate fallback on Link training failure") due to CI failures.

After investigating the CI failures it was found that these were essentially the failures which were always there but hidden because they used to be DRM_DEBUG_KMS messages for link failures so never got caught by CI. But now this patch actually throws DRM_ERROR if the link training fails at RBR and 1 lane. So it caught these link train failures.

There were two failures:
1. On SKL 6700k this was because the machine in CI lab is a SKL desktop without eDP on Port A. But our VBT initialization code in the driver writes VBT defaults in a way that it always sets DP flag on Port A and this does not get cleared after parsing the VBT outputs. This has been fixed in commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate errors from intel_bios_init())

2. On ILK-650 desktop - This was happening because of a bad monitor desktop combination. I switched the monitor in the CI lab and that helped get rid of the link failures on ILK system.

v10:
* Rebase on drm-tip and resend after revert
v9:
* Use the trimmed max values of link rate/lane count based on link train fallback (Daniel Vetter)
v8:
* Set link_status to BAD first and then call mode_valid (Jani Nikula) v7:
Remove the redundant variable in previous patch itself
v6:
* Obtain link rate index from fallback_link_rate using
the helper intel_dp_link_rate_index (Jani Nikula)
* Include fallback within intel_dp_start_link_train (Jani Nikula) v5:
* Move set link status to drm core (Daniel Vetter, Jani Nikula) v4:
* Add fallback support for non DDI platforms too
* Set connector->link status inside set_link_status function (Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variables link_train_failed and link_status to indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)

Acked-by Tony Cheng <[email protected]>

Acked-by Harry Wentland <[email protected]>

Cc Jani Nikula <[email protected]>

Cc Daniel Vetter <[email protected]>

Cc Ville Syrjala <[email protected]>

Signed-off-by Manasi Navare <[email protected]>

Reviewed-by Jani Nikula <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c               | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h              |  3 +++
 3 files changed, 50 insertions(+), 2 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +27 -0

index 6915d89a238c..f2d792a1b18e 100644
@@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
 	}
 }
 
+static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
+{
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+
+	intel_connector = container_of(work, typeof(*intel_connector),
+				       modeset_retry_work);
+	connector = &intel_connector->base;
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
+		      connector->name);
+
+	/* Grab the locks before changing connector property*/
+	mutex_lock(&connector->dev->mode_config.mutex);
+	/* Set connector link status to BAD and send a Uevent to notify
+	 * userspace to do a modeset.
+	 */
+	drm_mode_connector_set_link_status_property(connector,
+						    DRM_MODE_LINK_STATUS_BAD);
+	mutex_unlock(&connector->dev->mode_config.mutex);
+	/* Send Hotplug uevent so userspace can reprobe */
+	drm_kms_helper_hotplug_event(connector->dev);
+}
+
 bool
 intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector)

@@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	enum port port = intel_dig_port->port;
 	int type;
 
+	/* Initialize the work for modeset in case of link train failure */
+	INIT_WORK(&intel_connector->modeset_retry_work,
+		  intel_dp_modeset_retry_work_fn);
+
 	if (WARN(intel_dig_port->max_lanes < 1,
 		 "Not enough lanes (%d) for DP on port %c\n",
 		 intel_dig_port->max_lanes, port_name(port)))

drivers/gpu/drm/i915/intel_dp_link_training.c +20 -2

index 694ad0ffb523..b79c1c0e404c 100644
@@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 void
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_link_training_clock_recovery(intel_dp);
-	intel_dp_link_training_channel_equalization(intel_dp);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+
+	if (!intel_dp_link_training_clock_recovery(intel_dp))
+		goto failure_handling;
+	if (!intel_dp_link_training_channel_equalization(intel_dp))
+		goto failure_handling;
+
+	DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	return;
+
+ failure_handling:
+	DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	if (!intel_dp_get_link_train_fallback_values(intel_dp,
+						     intel_dp->link_rate,
+						     intel_dp->lane_count))
+		/* Schedule a Hotplug Uevent to userspace to start modeset */
+		schedule_work(&intel_connector->modeset_retry_work);
+	return;
 }

drivers/gpu/drm/i915/intel_drv.h +3 -0

index 5f6e1aad909d..d36026eff7cd 100644
@@ -321,6 +321,9 @@ struct intel_connector {
 	void *port; /* store this opaque as its illegal to dereference it */
 
 	struct intel_dp *mst_port;
+
+	/* Work struct to schedule a uevent on link train failure */
+	struct work_struct modeset_retry_work;
 };
 
 struct dpll {

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

Reviewed-by on v4 1/11

2017-04-06 14:24 UTC #2 lore raw

Re: [PATCH v4 01/11] drm/i915/dp: use the sink rates array for max sink rates

In reply to Jani Nikula

Quoted 8 lines · Jani Nikula

Looking at DPCD DP_MAX_LINK_RATE may be completely bogus for eDP 1.4 which is allowed to use link rate select method and have 0 in max link rate. With this change, it makes sense to store the max rate as the actual rate rather than as a bw code.
Cc Manasi Navare <[email protected]>
Cc Ville Syrjälä <[email protected]>
Signed-off-by Jani Nikula <[email protected]>

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 83 lines

---
 drivers/gpu/drm/i915/intel_dp.c  | 28 +++++++---------------------
 drivers/gpu/drm/i915/intel_drv.h |  2 +-
 2 files changed, 8 insertions(+), 22 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +7 ~~-21~~

index b38cba7d5abc..e4650f1625cc 100644
@@ -161,23 +161,9 @@ static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
 	intel_dp->num_sink_rates = num_rates;
 }
 
-static int
-intel_dp_max_link_bw(struct intel_dp  *intel_dp)
+static int intel_dp_max_sink_rate(struct intel_dp *intel_dp)
 {
-	int max_link_bw = intel_dp->dpcd[DP_MAX_LINK_RATE];
-
-	switch (max_link_bw) {
-	case DP_LINK_BW_1_62:
-	case DP_LINK_BW_2_7:
-	case DP_LINK_BW_5_4:
-		break;
-	default:
-		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
-		     max_link_bw);
-		max_link_bw = DP_LINK_BW_1_62;
-		break;
-	}
-	return max_link_bw;
+	return intel_dp->sink_rates[intel_dp->num_sink_rates - 1];
 }
 
 static u8 intel_dp_max_lane_count(struct intel_dp *intel_dp)

@@ -301,7 +287,7 @@ static int intel_dp_rate_index(const int *rates, int len, int rate)
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	int max_rate = drm_dp_bw_code_to_link_rate(intel_dp->max_sink_link_bw);
+	int max_rate = intel_dp->max_sink_link_rate;
 	int i, common_len;
 
 	common_len = intersect_rates(intel_dp->source_rates,

@@ -339,10 +325,10 @@ int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 						   common_rates,
 						   link_rate);
 	if (link_rate_index > 0) {
-		intel_dp->max_sink_link_bw = drm_dp_link_rate_to_bw_code(common_rates[link_rate_index - 1]);
+		intel_dp->max_sink_link_rate = common_rates[link_rate_index - 1];
 		intel_dp->max_sink_lane_count = lane_count;
 	} else if (lane_count > 1) {
-		intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp);
+		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
 		intel_dp->max_sink_lane_count = lane_count >> 1;
 	} else {
 		DRM_ERROR("Link Training Unsuccessful\n");

@@ -4652,8 +4638,8 @@ intel_dp_long_pulse(struct intel_connector *intel_connector)
 		/* Set the max lane count for sink */
 		intel_dp->max_sink_lane_count = drm_dp_max_lane_count(intel_dp->dpcd);
 
-		/* Set the max link BW for sink */
-		intel_dp->max_sink_link_bw = intel_dp_max_link_bw(intel_dp);
+		/* Set the max link rate for sink */
+		intel_dp->max_sink_link_rate = intel_dp_max_sink_rate(intel_dp);
 
 		intel_dp->reset_link_params = false;
 	}

drivers/gpu/drm/i915/intel_drv.h +1 -1

index 7bc0c25b3396..92e353dc61bf 100644
@@ -959,7 +959,7 @@ struct intel_dp {
 	/* Max lane count for the sink as per DPCD registers */
 	uint8_t max_sink_lane_count;
 	/* Max link BW for the sink as per DPCD registers */
-	int max_sink_link_bw;
+	int max_sink_link_rate;
 	/* sink or branch descriptor */
 	struct intel_dp_desc desc;
 	struct drm_dp_aux aux;

Signature · 1 line

2.1.4

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

Reviewed-by on v4 9/11

2017-04-06 14:25 UTC #11 lore raw

Re: [PATCH v4 09/11] drm/i915/dp: read sink count to a temporary variable first

In reply to Jani Nikula

Quoted 4 lines · Jani Nikula

Don't clobber intel_dp->sink_count with the raw value.
Suggested-by Ville Syrjälä <[email protected]>
Signed-off-by Jani Nikula <[email protected]>

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 38 lines

---
 drivers/gpu/drm/i915/intel_dp.c | 7 ++++---
 1 file changed, 4 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +4 -3

index 81682fd2804b..3c5c80da9ea3 100644
@@ -3728,6 +3728,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 static bool
 intel_dp_get_dpcd(struct intel_dp *intel_dp)
 {
+	u8 sink_count;
+
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;

@@ -3737,8 +3739,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		intel_dp_set_common_rates(intel_dp);
 	}
 
-	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT,
-			      &intel_dp->sink_count) <= 0)
+	if (drm_dp_dpcd_readb(&intel_dp->aux, DP_SINK_COUNT, &sink_count) <= 0)
 		return false;
 
 	/*

@@ -3746,7 +3747,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	 * a member variable in intel_dp will track any changes
 	 * between short pulse interrupts.
 	 */
-	intel_dp->sink_count = DP_GET_SINK_COUNT(intel_dp->sink_count);
+	intel_dp->sink_count = DP_GET_SINK_COUNT(sink_count);
 
 	/*
 	 * SINK_COUNT == 0 and DOWNSTREAM_PORT_PRESENT == 1 implies that

Signature · 1 line

2.1.4

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

2017-04-06 14:31 UTC #15 lore raw

Re: [PATCH v4 10/11] drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Jani Nikula

Quoted 45 lines · Jani Nikula

From: Manasi Navare <[email protected]>
Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.
This patch validates these values against the common_rates and max lane count values.
This is absolutely required incase the common_rates or max lane count are now different due to link fallback.
v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)
v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes
Cc Ville Syrjala <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +4 -0
index 3c5c80da9ea3..6915d89a238c 100644
@@ -322,6 +322,28 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	int index;
+

drivers/gpu/drm/i915/intel_dp.c static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, · line 338

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +10 -0

+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
+	index = intel_dp_rate_index(intel_dp->common_rates,
+				    intel_dp->num_common_rates,
+				    intel_dp->link_rate);
+	if (index < 0)
+		return false;

Hmm. Isn't common_rates[] still just the intersection of the source and sink rates? So it looks to me like this should actually just check for 'link_rate == 0 || link_rate > max_link_rate', or something similar.

Quoted 28 lines

drivers/gpu/drm/i915/intel_dp.c +13 -3

+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4275,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 1 line

2.1.4

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2017-04-06 14:35 UTC #25 lore raw

✓ Fi.CI.BAT: success for drm/i915/dp: rest of refactoring, link rate fallback

In reply to Jani Nikula

== Series Details ==

Series: drm/i915/dp: rest of refactoring, link rate fallback
URL : https://patchwork.freedesktop.org/series/22586/
State : success

== Summary ==

Series 22586v1 drm/i915/dp: rest of refactoring, link rate fallback
https://patchwork.freedesktop.org/api/1.0/series/22586/revisions/1/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 428s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 426s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 566s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 508s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 546s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 483s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 485s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 411s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 402s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 419s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 489s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 474s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 454s
fi-kbl-7560u     total:278  pass:267  dwarn:1   dfail:0   fail:0   skip:10  time: 566s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 450s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 574s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 462s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 492s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 435s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 526s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 402s

e087f8395ca39c6988de8680bd6f80a20b08c0f4 drm-tip: 2017y-04m-06d-13h-28m-42s UTC integration manifest 32e033d drm/i915: Implement Link Rate fallback on Link training failure
7b669fb drm/i915/dp: Validate cached link rate and lane count before retraining eb1b3252 drm/i915/dp: read sink count to a temporary variable first
e575e14 drm/i915/dp: use readb and writeb calls for single byte DPCD access
3df04b8 drm/i915/dp: localize link rate index variable more
dcd419b drm/i915/mst: use max link not sink lane count
c57a383 drm/i915/dp: add functions for max common link rate and lane count
688e493 drm/i915/dp: don't call the link parameters sink parameters
960daae drm/i915/dp: do not limit rate seek when not needed
2f414b3 drm/i915/dp: cache common rates with sink rates
f648e2e drm/i915/dp: use the sink rates array for max sink rates

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4420/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Manasi Navare

2017-04-06 18:25 UTC #16 lore raw

Re: [PATCH v4 10/11] drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Ville Syrjälä

Quoted 52 lines · Ville Syrjälä

Quoted 51 lines · Jani Nikula
From: Manasi Navare <[email protected]>
Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.
This patch validates these values against the common_rates and max lane count values.
This is absolutely required incase the common_rates or max lane count are now different due to link fallback.
v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)
v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes
Cc Ville Syrjala <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 30 +++++++++++++++++++++++++++---
 1 file changed, 27 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +10 -0
index 3c5c80da9ea3..6915d89a238c 100644
@@ -322,6 +322,28 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	int index;
+
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
+	index = intel_dp_rate_index(intel_dp->common_rates,

Quoted 9 lines

drivers/gpu/drm/i915/intel_dp.c static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, · line 338
Quoted 4 lines
drivers/gpu/drm/i915/intel_dp.c +4 -0
+				    intel_dp->num_common_rates,
+				    intel_dp->link_rate);
+	if (index < 0)
+		return false;
Hmm. Isn't common_rates[] still just the intersection of the source and sink rates? So it looks to me like this should actually just check for 'link_rate == 0 || link_rate > max_link_rate', or something similar.

Yes that is true. Passing common_rates worked earlier to see if the link rate was within the bounds and still less than the lowered fallback link rate becuase we were changing the common_rates.

But now with this new series, you are right that we cannot use common_rates array because it is still the intersection of just sink and source rates and wont take into account the lowered fallback rate unless we pass the length of common rates array limited by the max link rate by calling intel_dp_common_len_rate_limit() or we could just check that it is greater than 0 and less than the max_link_rate.

Does that sound right to you?

I will update this logic and send a new revision.

Regards
Manasi

Quoted 25 lines

drivers/gpu/drm/i915/intel_dp.c +13 -3

+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4275,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;

Quoted 7 lines

Quoted 3 lines
drivers/gpu/drm/i915/intel_dp.c +0 -0
 	/* Retrain if Channel EQ or CR not ok */
Signature · 1 line
2.1.4
Signature · 2 lines
Ville Syrjälä
Intel OTC

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Manasi Navare

2017-04-06 20:39 UTC #17 lore raw

PATCH v5 1/1 drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Ville Syrjälä

This patch validates these values against the max link rate and max lane count values.

This is absolutely required incase the common_rates or max lane count are now different due to link fallback.

v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)

v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes

v4:
* Compare the link rate against max link rate not the common_rates since common_rates does not account for the lowered fallback link rate value. (Ville Syrjala)

Cc Ville Syrjala <[email protected]>

Cc Jani Nikula <[email protected]>

Signed-off-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 28 +++++++++++++++++++++++++---
 1 file changed, 25 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +25 -3

index 3c5c80d..f3cca3c 100644
@@ -322,6 +322,26 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	int index;
+
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
+	if (intel_dp->link_rate == 0 ||
+	    intel_dp->link_rate > intel_dp->max_link_rate)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4273,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2017-04-06 20:42 UTC #26 lore raw

✗ Fi.CI.BAT: failure for drm/i915/dp: rest of refactoring, link rate fallback (rev2)

In reply to Jani Nikula

== Series Details ==

Series: drm/i915/dp: rest of refactoring, link rate fallback (rev2)
URL : https://patchwork.freedesktop.org/series/22586/
State : failure

== Summary ==

  CC [M]  drivers/gpu/drm/i915/gvt/display.o
  CC [M]  drivers/gpu/drm/i915/gvt/edid.o
  LD      drivers/rtc/built-in.o
  CC [M]  drivers/gpu/drm/i915/gvt/execlist.o
  CC [M]  drivers/gpu/drm/i915/gvt/scheduler.o
  CC [M]  drivers/gpu/drm/i915/gvt/sched_policy.o
  CC [M]  drivers/gpu/drm/i915/gvt/render.o
  CC [M]  drivers/gpu/drm/i915/gvt/cmd_parser.o
  LD [M]  drivers/ssb/ssb.o
  CC [M]  drivers/gpu/drm/i915/intel_lpe_audio.o
  AR      lib/lib.a
  LD      net/xfrm/built-in.o
  EXPORTS lib/lib-ksyms.o
  LD      net/packet/built-in.o
  LD      lib/built-in.o
  LD      drivers/mmc/core/mmc_core.o
  LD      drivers/mmc/host/built-in.o
  LD      drivers/pci/pcie/pcieportdrv.o
  LD      drivers/mmc/core/mmc_block.o
  LD      drivers/mmc/core/built-in.o
  LD      drivers/mmc/built-in.o
  LD      drivers/gpu/drm/drm.o
  LD      drivers/usb/storage/usb-storage.o
  LD      kernel/sched/built-in.o
  LD      drivers/usb/storage/built-in.o
  LD      kernel/built-in.o
  LD      drivers/tty/serial/8250/8250.o
  LD      drivers/pci/pcie/aer/aerdriver.o
  LD      drivers/pci/pcie/aer/built-in.o
  LD      drivers/pci/pcie/built-in.o
  LD      net/ipv6/ipv6.o
  LD [M]  drivers/usb/serial/usbserial.o
  LD [M]  drivers/net/ethernet/intel/igbvf/igbvf.o
  LD [M]  drivers/net/ethernet/broadcom/genet/genet.o
  LD      net/ipv6/built-in.o
  LD      drivers/video/fbdev/core/fb.o
  LD      drivers/thermal/thermal_sys.o
  LD      drivers/video/fbdev/core/built-in.o
  LD      drivers/thermal/built-in.o
  LD      drivers/pci/built-in.o
drivers/gpu/drm/i915/intel_dp.c: In function ‘intel_dp_link_params_valid’:
drivers/gpu/drm/i915/intel_dp.c:327:6: error: unused variable ‘index’ [-Werror=unused-variable]
  int index;
      ^
  LD      drivers/video/fbdev/built-in.o
  LD [M]  drivers/misc/mei/mei-me.o
  LD      drivers/iommu/built-in.o
  LD      drivers/misc/built-in.o
  LD      drivers/usb/gadget/udc/udc-core.o
  LD      drivers/usb/gadget/udc/built-in.o
  LD      drivers/scsi/scsi_mod.o
  LD      drivers/tty/serial/8250/8250_base.o
  LD      drivers/tty/serial/8250/built-in.o
  LD      drivers/tty/serial/built-in.o
  LD      drivers/spi/built-in.o
  LD      drivers/video/console/built-in.o
  LD      drivers/video/built-in.o
  LD      drivers/usb/gadget/libcomposite.o
  LD      drivers/usb/gadget/built-in.o
  LD      net/ipv4/built-in.o
  LD      drivers/scsi/sd_mod.o
  LD      drivers/scsi/built-in.o
  LD      fs/btrfs/btrfs.o
  LD      fs/btrfs/built-in.o
  LD      drivers/tty/vt/built-in.o
  LD      drivers/tty/built-in.o
  LD [M]  drivers/net/ethernet/intel/e1000/e1000.o
  CC      arch/x86/kernel/cpu/capflags.o
  LD      net/core/built-in.o
  LD      arch/x86/kernel/cpu/built-in.o
  LD      arch/x86/kernel/built-in.o
  LD      drivers/usb/core/usbcore.o
  LD      net/built-in.o
  LD      drivers/usb/core/built-in.o
  LD      fs/ext4/ext4.o
  LD      arch/x86/built-in.o
  LD      fs/ext4/built-in.o
  LD      fs/built-in.o
  LD      drivers/md/md-mod.o
  LD      drivers/md/built-in.o
  LD [M]  drivers/net/ethernet/intel/igb/igb.o
  LD      drivers/usb/host/xhci-hcd.o
  LD      drivers/usb/host/built-in.o

cc1: all warnings being treated as errors
LD drivers/usb/built-in.o
scripts/Makefile.build:294: recipe for target 'drivers/gpu/drm/i915/intel_dp.o' failed make[4]: *** [drivers/gpu/drm/i915/intel_dp.o] Error 1
make[4]: *** Waiting for unfinished jobs....
LD [M] drivers/net/ethernet/intel/e1000e/e1000e.o
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm/i915' failed make[3]: *** [drivers/gpu/drm/i915] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu/drm' failed make[2]: *** [drivers/gpu/drm] Error 2
scripts/Makefile.build:553: recipe for target 'drivers/gpu' failed make[1]: *** [drivers/gpu] Error 2
make[1]: *** Waiting for unfinished jobs....
LD drivers/net/ethernet/built-in.o
LD drivers/net/built-in.o
Makefile:1002: recipe for target 'drivers' failed
make: *** [drivers] Error 2

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Manasi Navare

2017-04-06 20:56 UTC #18 lore raw

PATCH v6 1/1 drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Manasi Navare

This patch validates these values against the max link rate and max lane count values.

This is absolutely required incase the common_rates or max lane count are now different due to link fallback.

v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)

v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes

v4:
* Compare the link rate against max link rate not the common_rates since common_rates does not account for the lowered fallback link rate value. (Ville Syrjala)

v5:
* Fixed a warning for unused variable (Manasi)

Cc Ville Syrjala <[email protected]>

Cc Jani Nikula <[email protected]>

Signed-off-by Manasi Navare <[email protected]>

Signed-off-by Jani Nikula <[email protected]>

---
 drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +23 -3

index 3c5c80d..36485be 100644
@@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
+	if (intel_dp->link_rate == 0 ||
+	    intel_dp->link_rate > intel_dp->max_link_rate)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4271,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 6 lines

2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Patchwork

2017-04-06 21:54 UTC #27 lore raw

✓ Fi.CI.BAT: success for drm/i915/dp: rest of refactoring, link rate fallback (rev3)

In reply to Jani Nikula

== Series Details ==

Series: drm/i915/dp: rest of refactoring, link rate fallback (rev3)
URL : https://patchwork.freedesktop.org/series/22586/
State : success

== Summary ==

Series 22586v3 drm/i915/dp: rest of refactoring, link rate fallback
https://patchwork.freedesktop.org/api/1.0/series/22586/revisions/3/mbox/

fi-bdw-5557u     total:278  pass:267  dwarn:0   dfail:0   fail:0   skip:11  time: 432s
fi-bdw-gvtdvm    total:278  pass:256  dwarn:8   dfail:0   fail:0   skip:14  time: 425s
fi-bsw-n3050     total:278  pass:242  dwarn:0   dfail:0   fail:0   skip:36  time: 577s
fi-bxt-j4205     total:278  pass:259  dwarn:0   dfail:0   fail:0   skip:19  time: 514s
fi-bxt-t5700     total:278  pass:258  dwarn:0   dfail:0   fail:0   skip:20  time: 541s
fi-byt-j1900     total:278  pass:254  dwarn:0   dfail:0   fail:0   skip:24  time: 486s
fi-byt-n2820     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 482s
fi-hsw-4770      total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 408s
fi-hsw-4770r     total:278  pass:262  dwarn:0   dfail:0   fail:0   skip:16  time: 405s
fi-ilk-650       total:278  pass:228  dwarn:0   dfail:0   fail:0   skip:50  time: 424s
fi-ivb-3520m     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 492s
fi-ivb-3770      total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 487s
fi-kbl-7500u     total:278  pass:260  dwarn:0   dfail:0   fail:0   skip:18  time: 452s
fi-kbl-7560u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 567s
fi-skl-6260u     total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 454s
fi-skl-6700hq    total:278  pass:261  dwarn:0   dfail:0   fail:0   skip:17  time: 567s
fi-skl-6700k     total:278  pass:256  dwarn:4   dfail:0   fail:0   skip:18  time: 464s
fi-skl-6770hq    total:278  pass:268  dwarn:0   dfail:0   fail:0   skip:10  time: 494s
fi-skl-gvtdvm    total:278  pass:265  dwarn:0   dfail:0   fail:0   skip:13  time: 429s
fi-snb-2520m     total:278  pass:250  dwarn:0   dfail:0   fail:0   skip:28  time: 524s
fi-snb-2600      total:278  pass:249  dwarn:0   dfail:0   fail:0   skip:29  time: 401s

7aafd5c8395fea9a79fbee82de4ffc63c04790d6 drm-tip: 2017y-04m-06d-21h-02m-00s UTC integration manifest d09e78a drm/i915: Implement Link Rate fallback on Link training failure
91e58a6 drm/i915/dp: Validate cached link rate and lane count before retraining d49bedb drm/i915/dp: read sink count to a temporary variable first
c5150ed drm/i915/dp: use readb and writeb calls for single byte DPCD access
5f81a04 drm/i915/dp: localize link rate index variable more
f7874d5 drm/i915/mst: use max link not sink lane count
ffeaa33 drm/i915/dp: add functions for max common link rate and lane count
dd6ff8d drm/i915/dp: don't call the link parameters sink parameters
c6ffc38 drm/i915/dp: do not limit rate seek when not needed
f9d73b0 drm/i915/dp: cache common rates with sink rates
e45eeb8 drm/i915/dp: use the sink rates array for max sink rates

== Logs ==

For more details see: https://intel-gfx-ci.01.org/CI/Patchwork_4431/
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-11 14:19 UTC #12 lore raw

Re: [PATCH v4 09/11] drm/i915/dp: read sink count to a temporary variable first

In reply to Ville Syrjälä

Quoted 7 lines · Ville Syrjälä

Quoted 4 lines · Jani Nikula
Don't clobber intel_dp->sink_count with the raw value.
Suggested-by Ville Syrjälä <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>

Thanks for the reviews, pushed patches 1-9 to drm-intel-next-queued.

BR,
Jani.

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi D

2017-04-11 15:44 UTC #13 lore raw

Re: [PATCH v4 09/11] drm/i915/dp: read sink count to a temporary variable first

Thanks Jani for pushing patches 1-9.
Now we just need review on Patch 10 (Validate cached link rate and lane count), may Be Ville can review that. I have submitted new revision based on his comments already.

And Patch 11 already has your R-b.

Regards
Manasi

Quoted 7 lines · Ville Syrjälä

Quoted 4 lines · Jani Nikula
Don't clobber intel_dp->sink_count with the raw value.
Suggested-by Ville Syrjälä <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
Reviewed-by Ville Syrjälä <[email protected]>

Thanks for the reviews, pushed patches 1-9 to drm-intel-next-queued.

BR,
Jani.

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Ville Syrjälä

Reviewed-by on v6 1/1

2017-04-12 14:00 UTC #19 lore raw

Re: [PATCH v6] drm/i915/dp: Validate cached link rate and lane count before retraining

In reply to Manasi Navare

Quoted 54 lines · Manasi Navare

Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.
This patch validates these values against the max link rate and max lane count values.
This is absolutely required incase the common_rates or max lane count are now different due to link fallback.
v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)
v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes
v4:
* Compare the link rate against max link rate not the common_rates since common_rates does not account for the lowered fallback link rate value. (Ville Syrjala)
v5:
* Fixed a warning for unused variable (Manasi)
Cc Ville Syrjala <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +7 -0
index 3c5c80d..36485be 100644
@@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */

drivers/gpu/drm/i915/intel_dp.c static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, · line 341

Quoted 10 lines

drivers/gpu/drm/i915/intel_dp.c +10 -0

+	if (intel_dp->link_rate == 0 ||
+	    intel_dp->link_rate > intel_dp->max_link_rate)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}

This lgtm

Reviewed-by Ville Syrjälä <[email protected]>

Quoted 21 lines

drivers/gpu/drm/i915/intel_dp.c +6 -3

+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4271,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 1 line

2.1.4

Signature · 6 lines

Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

maintainer Applied

2017-04-12 14:13 UTC #20 lore raw

Re: [PATCH v6] drm/i915/dp: Validate cached link rate and lane count before retraining

Quoted 68 lines · Ville Syrjälä

Quoted 54 lines · Manasi Navare
Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.
This patch validates these values against the max link rate and max lane count values.
This is absolutely required incase the common_rates or max lane count are now different due to link fallback.
v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)
v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes
v4:
* Compare the link rate against max link rate not the common_rates since common_rates does not account for the lowered fallback link rate value. (Ville Syrjala)
v5:
* Fixed a warning for unused variable (Manasi)
Cc Ville Syrjala <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +7 -0
index 3c5c80d..36485be 100644
@@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp)
+{
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
drivers/gpu/drm/i915/intel_dp.c static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, · line 341
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10 -0
+	if (intel_dp->link_rate == 0 ||
+	    intel_dp->link_rate > intel_dp->max_link_rate)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
This lgtm

Quoted 1 line

Reviewed-by Ville Syrjälä <[email protected]>

Pushed to drm-intel-next-queued, thanks for the patch and review.

BR,
Jani.

Quoted 22 lines

drivers/gpu/drm/i915/intel_dp.c +6 -3

+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)
 {

@@ -4253,9 +4271,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;
 
 	/* Retrain if Channel EQ or CR not ok */

Signature · 1 line

2.1.4

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Navare, Manasi D

Applied

2017-04-12 16:50 UTC #21 lore raw

Re: [PATCH v6] drm/i915/dp: Validate cached link rate and lane count before retraining

Thanks Ville for the review and thanks Jani for pushing this patch. Now we are down to 1 patch to get merged!

Regards
Manasi

Quoted 67 lines · Ville Syrjälä

Quoted 53 lines · Manasi Navare
Currently intel_dp_check_link_status() tries to retrain the link if Clock recovery or Channel EQ for any of the lanes indicated by intel_dp->lane_count is not set. However these values cached in intel_dp structure can be stale if link training has failed for these values during previous modeset. Or these values can get stale since we have now re read the DPCD registers or it can be 0 in case of connected boot case.
This patch validates these values against the max link rate and max lane count values.
This is absolutely required incase the common_rates or max lane count are now different due to link fallback.
v2:
* Include the FIXME commnet inside the function (Ville Syrjala)
* Remove the redundant parenthesis (Ville Syrjala)
v3 by Jani:
* rebase on the DP refactoring series
* rename intel_dp_link_params_is_valid to intel_dp_link_params_valid
* minor stylistic changes
v4:
* Compare the link rate against max link rate not the common_rates since common_rates does not account for the lowered fallback link rate value. (Ville Syrjala)
v5:
* Fixed a warning for unused variable (Manasi)
Cc Ville Syrjala <[email protected]>
Cc Jani Nikula <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++++++++++++++++---
 1 file changed, 23 insertions(+), 3 deletions(-)
(quoted diff) +0 -0
b/drivers/gpu/drm/i915/intel_dp.c index 3c5c80d..36485be 100644
drivers/gpu/drm/i915/intel_dp.c +6 -0
@@ -322,6 +322,24 @@ static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp,
 	return 0;
 }
 
+static bool intel_dp_link_params_valid(struct intel_dp *intel_dp) {
+	/*
+	 * FIXME: we need to synchronize the current link parameters with
+	 * hardware readout. Currently fast link training doesn't work on
+	 * boot-up.
+	 */
drivers/gpu/drm/i915/intel_dp.c static int intel_dp_common_len_rate_limit(struct intel_dp *intel_dp, · line 340
Quoted 10 lines
drivers/gpu/drm/i915/intel_dp.c +10 -0
+	if (intel_dp->link_rate == 0 ||
+	    intel_dp->link_rate > intel_dp->max_link_rate)
+		return false;
+
+	if (intel_dp->lane_count == 0 ||
+	    intel_dp->lane_count > intel_dp_max_lane_count(intel_dp))
+		return false;
+
+	return true;
+}
This lgtm

Quoted 1 line

Reviewed-by Ville Syrjälä <[email protected]>

Pushed to drm-intel-next-queued, thanks for the patch and review.

BR,
Jani.

Quoted 18 lines

drivers/gpu/drm/i915/intel_dp.c +6 -3

+
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,
 					    int link_rate, uint8_t lane_count)  { @@ -4253,9 +4271,11 
 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 	if (!to_intel_crtc(intel_encoder->base.crtc)->active)
 		return;
 
-	/* FIXME: we need to synchronize this sort of stuff with hardware
-	 * readout. Currently fast link training doesn't work on boot-up. */
-	if (!intel_dp->lane_count)
+	/*
+	 * Validate the cached values of intel_dp->link_rate and
+	 * intel_dp->lane_count before attempting to retrain.
+	 */
+	if (!intel_dp_link_params_valid(intel_dp))
 		return;

Quoted 3 lines

Quoted 3 lines
/* Retrain if Channel EQ or CR not ok */
--
2.1.4

--
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Manasi Navare

2017-04-13 17:21 UTC #23 lore raw

Re: [PATCH v4 11/11] drm/i915: Implement Link Rate fallback on Link training failure

In reply to Jani Nikula

Hi Jani,

This patch has the necessary ACKs and R-bs, anything else blocking this from getting merged?

Regards
Manasi

Quoted 172 lines · Jani Nikula

From: Manasi Navare <[email protected]>
If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed, update the link status property to "BAD" and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it.
This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6.
This patch is a resend of the original commit id (233ce881dd91fb "drm/i915: Implement Link Rate fallback on Link training failure") which got reverted in this commit id (afc1ebf4562a14 Revert "drm/i915: Implement Link Rate fallback on Link training failure") due to CI failures.
After investigating the CI failures it was found that these were essentially the failures which were always there but hidden because they used to be DRM_DEBUG_KMS messages for link failures so never got caught by CI. But now this patch actually throws DRM_ERROR if the link training fails at RBR and 1 lane. So it caught these link train failures.
There were two failures:
1. On SKL 6700k this was because the machine in CI lab is a SKL desktop without eDP on Port A. But our VBT initialization code in the driver writes VBT defaults in a way that it always sets DP flag on Port A and this does not get cleared after parsing the VBT outputs. This has been fixed in commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate errors from intel_bios_init())
2. On ILK-650 desktop - This was happening because of a bad monitor desktop combination. I switched the monitor in the CI lab and that helped get rid of the link failures on ILK system.
v10:
* Rebase on drm-tip and resend after revert
v9:
* Use the trimmed max values of link rate/lane count based on link train fallback (Daniel Vetter)
v8:
* Set link_status to BAD first and then call mode_valid (Jani Nikula) v7:
Remove the redundant variable in previous patch itself
v6:
* Obtain link rate index from fallback_link_rate using
the helper intel_dp_link_rate_index (Jani Nikula)
* Include fallback within intel_dp_start_link_train (Jani Nikula) v5:
* Move set link status to drm core (Daniel Vetter, Jani Nikula) v4:
* Add fallback support for non DDI platforms too
* Set connector->link status inside set_link_status function (Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variables link_train_failed and link_status to indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)
Acked-by Tony Cheng <[email protected]>
Acked-by Harry Wentland <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Ville Syrjala <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Reviewed-by Jani Nikula <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c               | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h              |  3 +++
 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +27 -0
index 6915d89a238c..f2d792a1b18e 100644
@@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
 	}
 }
 
+static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
+{
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+
+	intel_connector = container_of(work, typeof(*intel_connector),
+				       modeset_retry_work);
+	connector = &intel_connector->base;
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
+		      connector->name);
+
+	/* Grab the locks before changing connector property*/
+	mutex_lock(&connector->dev->mode_config.mutex);
+	/* Set connector link status to BAD and send a Uevent to notify
+	 * userspace to do a modeset.
+	 */
+	drm_mode_connector_set_link_status_property(connector,
+						    DRM_MODE_LINK_STATUS_BAD);
+	mutex_unlock(&connector->dev->mode_config.mutex);
+	/* Send Hotplug uevent so userspace can reprobe */
+	drm_kms_helper_hotplug_event(connector->dev);
+}
+
 bool
 intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector)
@@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	enum port port = intel_dig_port->port;
 	int type;
 
+	/* Initialize the work for modeset in case of link train failure */
+	INIT_WORK(&intel_connector->modeset_retry_work,
+		  intel_dp_modeset_retry_work_fn);
+
 	if (WARN(intel_dig_port->max_lanes < 1,
 		 "Not enough lanes (%d) for DP on port %c\n",
 		 intel_dig_port->max_lanes, port_name(port)))
drivers/gpu/drm/i915/intel_dp_link_training.c +20 -2
index 694ad0ffb523..b79c1c0e404c 100644
@@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 void
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_link_training_clock_recovery(intel_dp);
-	intel_dp_link_training_channel_equalization(intel_dp);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+
+	if (!intel_dp_link_training_clock_recovery(intel_dp))
+		goto failure_handling;
+	if (!intel_dp_link_training_channel_equalization(intel_dp))
+		goto failure_handling;
+
+	DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	return;
+
+ failure_handling:
+	DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	if (!intel_dp_get_link_train_fallback_values(intel_dp,
+						     intel_dp->link_rate,
+						     intel_dp->lane_count))
+		/* Schedule a Hotplug Uevent to userspace to start modeset */
+		schedule_work(&intel_connector->modeset_retry_work);
+	return;
 }
drivers/gpu/drm/i915/intel_drv.h +3 -0
index 5f6e1aad909d..d36026eff7cd 100644
@@ -321,6 +321,9 @@ struct intel_connector {
 	void *port; /* store this opaque as its illegal to dereference it */
 
 	struct intel_dp *mst_port;
+
+	/* Work struct to schedule a uevent on link train failure */
+	struct work_struct modeset_retry_work;
 };
 
 struct dpll {
Signature · 1 line
2.1.4

_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Jani Nikula

author

2017-04-13 19:16 UTC #24 lore raw

Re: [PATCH v4 11/11] drm/i915: Implement Link Rate fallback on Link training failure

Quoted 4 lines · Manasi Navare

Hi Jani,
This patch has the necessary ACKs and R-bs, anything else blocking this from getting merged?

Pushed, finally, sorry for the delay.

BR,
Jani.

Quoted 177 lines

Regards
Manasi
Quoted 172 lines · Jani Nikula
From: Manasi Navare <[email protected]>
If link training at a link rate optimal for a particular mode fails during modeset's atomic commit phase, then we let the modeset complete and then retry. We save the link rate value at which link training failed, update the link status property to "BAD" and use a lower link rate to prune the modes. It will redo the modeset on the current mode at lower link rate or if the current mode gets pruned due to lower link constraints then, it will send a hotplug uevent for userspace to handle it.
This is also required to pass DP CTS tests 4.3.1.3, 4.3.1.4, 4.3.1.6.
This patch is a resend of the original commit id (233ce881dd91fb "drm/i915: Implement Link Rate fallback on Link training failure") which got reverted in this commit id (afc1ebf4562a14 Revert "drm/i915: Implement Link Rate fallback on Link training failure") due to CI failures.
After investigating the CI failures it was found that these were essentially the failures which were always there but hidden because they used to be DRM_DEBUG_KMS messages for link failures so never got caught by CI. But now this patch actually throws DRM_ERROR if the link training fails at RBR and 1 lane. So it caught these link train failures.
There were two failures:
1. On SKL 6700k this was because the machine in CI lab is a SKL desktop without eDP on Port A. But our VBT initialization code in the driver writes VBT defaults in a way that it always sets DP flag on Port A and this does not get cleared after parsing the VBT outputs. This has been fixed in commit id (bb1d132935c2f8 "drm/i915/vbt: split out defaults that are set when there is no VBT) and (665788572c6410b "drm/i915/vbt: don't propagate errors from intel_bios_init())
2. On ILK-650 desktop - This was happening because of a bad monitor desktop combination. I switched the monitor in the CI lab and that helped get rid of the link failures on ILK system.
v10:
* Rebase on drm-tip and resend after revert
v9:
* Use the trimmed max values of link rate/lane count based on link train fallback (Daniel Vetter)
v8:
* Set link_status to BAD first and then call mode_valid (Jani Nikula) v7:
Remove the redundant variable in previous patch itself
v6:
* Obtain link rate index from fallback_link_rate using
the helper intel_dp_link_rate_index (Jani Nikula)
* Include fallback within intel_dp_start_link_train (Jani Nikula) v5:
* Move set link status to drm core (Daniel Vetter, Jani Nikula) v4:
* Add fallback support for non DDI platforms too
* Set connector->link status inside set_link_status function (Jani Nikula)
v3:
* Set link status property to BAd unconditionally (Jani Nikula)
* Dont use two separate variables link_train_failed and link_status to indicate same thing (Jani Nikula)
v2:
* Squashed a few patches (Jani Nikula)
Acked-by Tony Cheng <[email protected]>
Acked-by Harry Wentland <[email protected]>
Cc Jani Nikula <[email protected]>
Cc Daniel Vetter <[email protected]>
Cc Ville Syrjala <[email protected]>
Signed-off-by Manasi Navare <[email protected]>
Reviewed-by Jani Nikula <[email protected]>
Signed-off-by Jani Nikula <[email protected]>
---
 drivers/gpu/drm/i915/intel_dp.c               | 27 +++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 22 ++++++++++++++++++++--
 drivers/gpu/drm/i915/intel_drv.h              |  3 +++
 3 files changed, 50 insertions(+), 2 deletions(-)
drivers/gpu/drm/i915/intel_dp.c +27 -0
index 6915d89a238c..f2d792a1b18e 100644
@@ -5977,6 +5977,29 @@ intel_dp_init_connector_port_info(struct intel_digital_port *intel_dig_port)
 	}
 }
 
+static void intel_dp_modeset_retry_work_fn(struct work_struct *work)
+{
+	struct intel_connector *intel_connector;
+	struct drm_connector *connector;
+
+	intel_connector = container_of(work, typeof(*intel_connector),
+				       modeset_retry_work);
+	connector = &intel_connector->base;
+	DRM_DEBUG_KMS("[CONNECTOR:%d:%s]\n", connector->base.id,
+		      connector->name);
+
+	/* Grab the locks before changing connector property*/
+	mutex_lock(&connector->dev->mode_config.mutex);
+	/* Set connector link status to BAD and send a Uevent to notify
+	 * userspace to do a modeset.
+	 */
+	drm_mode_connector_set_link_status_property(connector,
+						    DRM_MODE_LINK_STATUS_BAD);
+	mutex_unlock(&connector->dev->mode_config.mutex);
+	/* Send Hotplug uevent so userspace can reprobe */
+	drm_kms_helper_hotplug_event(connector->dev);
+}
+
 bool
 intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			struct intel_connector *intel_connector)
@@ -5989,6 +6012,10 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	enum port port = intel_dig_port->port;
 	int type;
 
+	/* Initialize the work for modeset in case of link train failure */
+	INIT_WORK(&intel_connector->modeset_retry_work,
+		  intel_dp_modeset_retry_work_fn);
+
 	if (WARN(intel_dig_port->max_lanes < 1,
 		 "Not enough lanes (%d) for DP on port %c\n",
 		 intel_dig_port->max_lanes, port_name(port)))
drivers/gpu/drm/i915/intel_dp_link_training.c +20 -2
index 694ad0ffb523..b79c1c0e404c 100644
@@ -314,6 +314,24 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 void
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_link_training_clock_recovery(intel_dp);
-	intel_dp_link_training_channel_equalization(intel_dp);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+
+	if (!intel_dp_link_training_clock_recovery(intel_dp))
+		goto failure_handling;
+	if (!intel_dp_link_training_channel_equalization(intel_dp))
+		goto failure_handling;
+
+	DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	return;
+
+ failure_handling:
+	DRM_DEBUG_KMS("Link Training failed at link rate = %d, lane count = %d",
+		      intel_dp->link_rate, intel_dp->lane_count);
+	if (!intel_dp_get_link_train_fallback_values(intel_dp,
+						     intel_dp->link_rate,
+						     intel_dp->lane_count))
+		/* Schedule a Hotplug Uevent to userspace to start modeset */
+		schedule_work(&intel_connector->modeset_retry_work);
+	return;
 }
drivers/gpu/drm/i915/intel_drv.h +3 -0
index 5f6e1aad909d..d36026eff7cd 100644
@@ -321,6 +321,9 @@ struct intel_connector {
 	void *port; /* store this opaque as its illegal to dereference it */
 
 	struct intel_dp *mst_port;
+
+	/* Work struct to schedule a uevent on link train failure */
+	struct work_struct modeset_retry_work;
 };
 
 struct dpll {
Signature · 1 line
2.1.4

Signature · 5 lines

Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
[email protected]
https://lists.freedesktop.org/mailman/listinfo/intel-gfx