drm/i915/dp: link rate and lane count refactoring

There is some conflation related to sink rates, making this change more complicated than it would otherwise have to be. There are three changes here that are rather difficult to split up:

1) Use the intel_dp->sink_rates array for all DP, not just eDP 1.4. We initialize it from DPCD on eDP 1.4 like before, but generate it based on DP_MAX_LINK_RATE on others. This reduces code complexity when we need to use the sink rates; they are all always in the sink_rates array.

2) Update the sink rate array whenever we read DPCD, and use the information from there. This increases code readability when we need the sink rates.

3) Disentangle fallback rate limiting from sink rates. In the code, the max rate is a dynamic property of the *link*, not of the *sink*. Do the limiting after intersecting the source and sink rates, which are static properties of the devices.

This paves the way for follow-up refactoring that I've refrained from doing here to keep this change as simple as it possibly can.

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               | 76 ++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h              |  4 +-
 3 files changed, 55 insertions(+), 28 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +45 -24

index cc2523363c8d..d13ce6746542 100644
@@ -133,6 +133,34 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
 static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
+static int intel_dp_num_rates(u8 link_bw_code)
+{
+	switch (link_bw_code) {
+	default:
+		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
+		     link_bw_code);
+	case DP_LINK_BW_1_62:
+		return 1;
+	case DP_LINK_BW_2_7:
+		return 2;
+	case DP_LINK_BW_5_4:
+		return 3;
+	}
+}
+
+/* update sink rates from dpcd */
+static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
+{
+	int i, num_rates;
+
+	num_rates = intel_dp_num_rates(intel_dp->dpcd[DP_MAX_LINK_RATE]);
+
+	for (i = 0; i < num_rates; i++)
+		intel_dp->sink_rates[i] = default_rates[i];
+
+	intel_dp->num_sink_rates = num_rates;
+}
+
 static int
 intel_dp_max_link_bw(struct intel_dp  *intel_dp)
 {

@@ -205,19 +233,6 @@ intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp)
 	return max_dotclk;
 }
 
-static int
-intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
-{
-	if (intel_dp->num_sink_rates) {
-		*sink_rates = intel_dp->sink_rates;
-		return intel_dp->num_sink_rates;
-	}
-
-	*sink_rates = default_rates;
-
-	return (intel_dp->max_sink_link_bw >> 3) + 1;
-}
-
 static void
 intel_dp_set_source_rates(struct intel_dp *intel_dp)
 {

@@ -285,15 +300,22 @@ static int intel_dp_find_rate(const int *rates, int len, int rate)
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	const int *sink_rates;
-	int sink_len;
+	int max_rate = drm_dp_bw_code_to_link_rate(intel_dp->max_sink_link_bw);
+	int i, common_len;
 
-	sink_len = intel_dp_sink_rates(intel_dp, &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);
 
-	return intersect_rates(intel_dp->source_rates,
-			       intel_dp->num_source_rates,
-			       sink_rates, sink_len,
-			       common_rates);
+	/* Limit results by potentially reduced max rate */
+	for (i = 0; i < common_len; i++) {
+		if (common_rates[common_len - i - 1] <= max_rate)
+			return common_len - i;
+	}
+
+	return 0;
 }
 
 static int intel_dp_link_rate_index(struct intel_dp *intel_dp,

@@ -1501,8 +1523,7 @@ static void snprintf_int_array(char *str, size_t len,
 
 static void intel_dp_print_rates(struct intel_dp *intel_dp)
 {
-	const int *sink_rates;
-	int sink_len, common_len;
+	int common_len;
 	int common_rates[DP_MAX_SUPPORTED_RATES];
 	char str[128]; /* FIXME: too big for stack? */
 

@@ -1513,8 +1534,8 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 			   intel_dp->source_rates, intel_dp->num_source_rates);
 	DRM_DEBUG_KMS("source rates: %s\n", str);
 
-	sink_len = intel_dp_sink_rates(intel_dp, &sink_rates);
-	snprintf_int_array(str, sizeof(str), sink_rates, sink_len);
+	snprintf_int_array(str, sizeof(str),
+			   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);

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

 		*link_bw = 0;
 		*rate_select =
 			intel_dp_rate_select(intel_dp, port_clock);

@@ -3713,6 +3735,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 			intel_dp->sink_rates[i] = (val * 200) / 10;
 		}
 		intel_dp->num_sink_rates = i;
+	} else {
+		intel_dp_set_sink_rates(intel_dp);
 	}
 
 	return true;

@@ -3725,6 +3749,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;
 
+	intel_dp_set_sink_rates(intel_dp);
+
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
 			     &intel_dp->sink_count, 1) < 0)
 		return false;

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

index 0048b520baf7..694ad0ffb523 100644
@@ -146,7 +146,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
 
-	if (intel_dp->num_sink_rates)
+	/* eDP 1.4 rate select method. */
+	if (!link_bw)
 		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
 				  &rate_select, 1);
 

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

index f132d4aea1ad..b914dd543362 100644
@@ -933,8 +933,8 @@ struct intel_dp {
 	/* source rates */
 	int num_source_rates;
 	const int *source_rates;
-	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
-	uint8_t num_sink_rates;
+	/* sink rates as reported by DP_MAX_LINK_RATE/DP_SUPPORTED_LINK_RATES */
+	int num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
 	/* Max lane count for the sink as per DPCD registers */
 	uint8_t max_sink_lane_count;

Signature · 1 line

2.1.4

Quoted 142 lines · Jani Nikula

There is some conflation related to sink rates, making this change more complicated than it would otherwise have to be. There are three changes here that are rather difficult to split up:

1) Use the intel_dp->sink_rates array for all DP, not just eDP 1.4. We initialize it from DPCD on eDP 1.4 like before, but generate it based on DP_MAX_LINK_RATE on others. This reduces code complexity when we need to use the sink rates; they are all always in the sink_rates array.

2) Update the sink rate array whenever we read DPCD, and use the information from there. This increases code readability when we need the sink rates.

3) Disentangle fallback rate limiting from sink rates. In the code, the max rate is a dynamic property of the *link*, not of the *sink*. Do the limiting after intersecting the source and sink rates, which are static properties of the devices.

This paves the way for follow-up refactoring that I've refrained from doing here to keep this change as simple as it possibly can.

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               | 76 ++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h              |  4 +-
 3 files changed, 55 insertions(+), 28 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +45 -24

index cc2523363c8d..d13ce6746542 100644
@@ -133,6 +133,34 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
 static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
+static int intel_dp_num_rates(u8 link_bw_code)
+{
+	switch (link_bw_code) {
+	default:
+		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
+		     link_bw_code);
+	case DP_LINK_BW_1_62:
+		return 1;
+	case DP_LINK_BW_2_7:
+		return 2;
+	case DP_LINK_BW_5_4:
+		return 3;
+	}
+}
+
+/* update sink rates from dpcd */
+static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
+{
+	int i, num_rates;
+
+	num_rates = intel_dp_num_rates(intel_dp->dpcd[DP_MAX_LINK_RATE]);
+
+	for (i = 0; i < num_rates; i++)
+		intel_dp->sink_rates[i] = default_rates[i];
+
+	intel_dp->num_sink_rates = num_rates;
+}
+
 static int
 intel_dp_max_link_bw(struct intel_dp  *intel_dp)
 {

@@ -205,19 +233,6 @@ intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp)
 	return max_dotclk;
 }
 
-static int
-intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
-{
-	if (intel_dp->num_sink_rates) {
-		*sink_rates = intel_dp->sink_rates;
-		return intel_dp->num_sink_rates;
-	}
-
-	*sink_rates = default_rates;
-
-	return (intel_dp->max_sink_link_bw >> 3) + 1;
-}
-
 static void
 intel_dp_set_source_rates(struct intel_dp *intel_dp)
 {

@@ -285,15 +300,22 @@ static int intel_dp_find_rate(const int *rates, int len, int rate)
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	const int *sink_rates;
-	int sink_len;
+	int max_rate = drm_dp_bw_code_to_link_rate(intel_dp->max_sink_link_bw);
+	int i, common_len;
 
-	sink_len = intel_dp_sink_rates(intel_dp, &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);
 
-	return intersect_rates(intel_dp->source_rates,
-			       intel_dp->num_source_rates,
-			       sink_rates, sink_len,
-			       common_rates);
+	/* Limit results by potentially reduced max rate */
+	for (i = 0; i < common_len; i++) {
+		if (common_rates[common_len - i - 1] <= max_rate)
+			return common_len - i;
+	}
+
+	return 0;
 }
 
 static int intel_dp_link_rate_index(struct intel_dp *intel_dp,

@@ -1501,8 +1523,7 @@ static void snprintf_int_array(char *str, size_t len,
 
 static void intel_dp_print_rates(struct intel_dp *intel_dp)
 {
-	const int *sink_rates;
-	int sink_len, common_len;
+	int common_len;
 	int common_rates[DP_MAX_SUPPORTED_RATES];
 	char str[128]; /* FIXME: too big for stack? */
 

@@ -1513,8 +1534,8 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 			   intel_dp->source_rates, intel_dp->num_source_rates);
 	DRM_DEBUG_KMS("source rates: %s\n", str);
 
-	sink_len = intel_dp_sink_rates(intel_dp, &sink_rates);
-	snprintf_int_array(str, sizeof(str), sink_rates, sink_len);
+	snprintf_int_array(str, sizeof(str),
+			   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);

drivers/gpu/drm/i915/intel_dp.c int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) · line 1605

Quoted 6 lines

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

@@ -1580,7 +1601,8 @@ int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select)
 {
-	if (intel_dp->num_sink_rates) {
+	/* eDP 1.4 rate select method. */
+	if (is_edp(intel_dp) && intel_dp->edp_dpcd[0] >= 0x03) {

I was convinced that this wasn't a mandatory feature, but the spec does seem to say "The table must contain at least one non-zero value at the first (lowest DPCD address) location."

But given historical evidence I'm still 99% convinced we'll eventually run into some panel somewhere that does this wrong, so I don't really like this idea.

Quoted 53 lines

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

 		*link_bw = 0;
 		*rate_select =
 			intel_dp_rate_select(intel_dp, port_clock);

@@ -3713,6 +3735,8 @@ intel_edp_init_dpcd(struct intel_dp *intel_dp)
 			intel_dp->sink_rates[i] = (val * 200) / 10;
 		}
 		intel_dp->num_sink_rates = i;
+	} else {
+		intel_dp_set_sink_rates(intel_dp);
 	}
 
 	return true;

@@ -3725,6 +3749,8 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	if (!intel_dp_read_dpcd(intel_dp))
 		return false;
 
+	intel_dp_set_sink_rates(intel_dp);
+
 	if (drm_dp_dpcd_read(&intel_dp->aux, DP_SINK_COUNT,
 			     &intel_dp->sink_count, 1) < 0)
 		return false;

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

index 0048b520baf7..694ad0ffb523 100644
@@ -146,7 +146,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
 
-	if (intel_dp->num_sink_rates)
+	/* eDP 1.4 rate select method. */
+	if (!link_bw)
 		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
 				  &rate_select, 1);
 

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

index f132d4aea1ad..b914dd543362 100644
@@ -933,8 +933,8 @@ struct intel_dp {
 	/* source rates */
 	int num_source_rates;
 	const int *source_rates;
-	/* sink rates as reported by DP_SUPPORTED_LINK_RATES */
-	uint8_t num_sink_rates;
+	/* sink rates as reported by DP_MAX_LINK_RATE/DP_SUPPORTED_LINK_RATES */
+	int num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
 	/* Max lane count for the sink as per DPCD registers */
 	uint8_t max_sink_lane_count;

Signature · 1 line

2.1.4

Quoted 154 lines · Ville Syrjälä

Quoted 142 lines · Jani Nikula

There is some conflation related to sink rates, making this change more complicated than it would otherwise have to be. There are three changes here that are rather difficult to split up:

1) Use the intel_dp->sink_rates array for all DP, not just eDP 1.4. We initialize it from DPCD on eDP 1.4 like before, but generate it based on DP_MAX_LINK_RATE on others. This reduces code complexity when we need to use the sink rates; they are all always in the sink_rates array.

2) Update the sink rate array whenever we read DPCD, and use the information from there. This increases code readability when we need the sink rates.

3) Disentangle fallback rate limiting from sink rates. In the code, the max rate is a dynamic property of the *link*, not of the *sink*. Do the limiting after intersecting the source and sink rates, which are static properties of the devices.

This paves the way for follow-up refactoring that I've refrained from doing here to keep this change as simple as it possibly can.

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               | 76 ++++++++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  3 +-
 drivers/gpu/drm/i915/intel_drv.h              |  4 +-
 3 files changed, 55 insertions(+), 28 deletions(-)

drivers/gpu/drm/i915/intel_dp.c +45 -24

index cc2523363c8d..d13ce6746542 100644
@@ -133,6 +133,34 @@ static void vlv_steal_power_sequencer(struct drm_device *dev,
 				      enum pipe pipe);
 static void intel_dp_unset_edid(struct intel_dp *intel_dp);
 
+static int intel_dp_num_rates(u8 link_bw_code)
+{
+	switch (link_bw_code) {
+	default:
+		WARN(1, "invalid max DP link bw val %x, using 1.62Gbps\n",
+		     link_bw_code);
+	case DP_LINK_BW_1_62:
+		return 1;
+	case DP_LINK_BW_2_7:
+		return 2;
+	case DP_LINK_BW_5_4:
+		return 3;
+	}
+}
+
+/* update sink rates from dpcd */
+static void intel_dp_set_sink_rates(struct intel_dp *intel_dp)
+{
+	int i, num_rates;
+
+	num_rates = intel_dp_num_rates(intel_dp->dpcd[DP_MAX_LINK_RATE]);
+
+	for (i = 0; i < num_rates; i++)
+		intel_dp->sink_rates[i] = default_rates[i];
+
+	intel_dp->num_sink_rates = num_rates;
+}
+
 static int
 intel_dp_max_link_bw(struct intel_dp  *intel_dp)
 {

@@ -205,19 +233,6 @@ intel_dp_downstream_max_dotclock(struct intel_dp *intel_dp)
 	return max_dotclk;
 }
 
-static int
-intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
-{
-	if (intel_dp->num_sink_rates) {
-		*sink_rates = intel_dp->sink_rates;
-		return intel_dp->num_sink_rates;
-	}
-
-	*sink_rates = default_rates;
-
-	return (intel_dp->max_sink_link_bw >> 3) + 1;
-}
-
 static void
 intel_dp_set_source_rates(struct intel_dp *intel_dp)
 {

@@ -285,15 +300,22 @@ static int intel_dp_find_rate(const int *rates, int len, int rate)
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	const int *sink_rates;
-	int sink_len;
+	int max_rate = drm_dp_bw_code_to_link_rate(intel_dp->max_sink_link_bw);
+	int i, common_len;
 
-	sink_len = intel_dp_sink_rates(intel_dp, &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);
 
-	return intersect_rates(intel_dp->source_rates,
-			       intel_dp->num_source_rates,
-			       sink_rates, sink_len,
-			       common_rates);
+	/* Limit results by potentially reduced max rate */
+	for (i = 0; i < common_len; i++) {
+		if (common_rates[common_len - i - 1] <= max_rate)
+			return common_len - i;
+	}
+
+	return 0;
 }
 
 static int intel_dp_link_rate_index(struct intel_dp *intel_dp,

@@ -1501,8 +1523,7 @@ static void snprintf_int_array(char *str, size_t len,
 
 static void intel_dp_print_rates(struct intel_dp *intel_dp)
 {
-	const int *sink_rates;
-	int sink_len, common_len;
+	int common_len;
 	int common_rates[DP_MAX_SUPPORTED_RATES];
 	char str[128]; /* FIXME: too big for stack? */
 

@@ -1513,8 +1534,8 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 			   intel_dp->source_rates, intel_dp->num_source_rates);
 	DRM_DEBUG_KMS("source rates: %s\n", str);
 
-	sink_len = intel_dp_sink_rates(intel_dp, &sink_rates);
-	snprintf_int_array(str, sizeof(str), sink_rates, sink_len);
+	snprintf_int_array(str, sizeof(str),
+			   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);

drivers/gpu/drm/i915/intel_dp.c int intel_dp_rate_select(struct intel_dp *intel_dp, int rate) · line 1605

Quoted 6 lines

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

@@ -1580,7 +1601,8 @@ int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select)
 {
-	if (intel_dp->num_sink_rates) {
+	/* eDP 1.4 rate select method. */
+	if (is_edp(intel_dp) && intel_dp->edp_dpcd[0] >= 0x03) {

I was convinced that this wasn't a mandatory feature, but the spec does seem to say "The table must contain at least one non-zero value at the first (lowest DPCD address) location."

Quoted 3 lines

But given historical evidence I'm still 99% convinced we'll eventually run into some panel somewhere that does this wrong, so I don't really like this idea.

Then I think this should be fixed in intel_edp_init_dpcd() by a) checking that there is at least one non-zero value, falling back to intel_dp_set_source_rates() if not, and b) setting a new intel_dp->use_rate_selet field at that time, which will be used instead of (is_edp(intel_dp) && intel_dp->edp_dpcd[0] >= 0x03). Sound good?

This is kind of version 1½ of [1], basically just rebased on current git

(including Manasi's test automation patches) and a couple of more cleanups slammed on top.

BR,
Jani.

[1] http://mid.mail-archive.com/[email protected]

Jani Nikula (13):
drm/i915/dp: use known correct array size in rate_to_index drm/i915/dp: return errors from rate_to_index() drm/i915/dp: rename rate_to_index() to intel_dp_find_rate() and reuse drm/i915/dp: cache source rates at init
drm/i915/dp: generate and cache sink rate array for all DP, not just

    eDP 1.4

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

 drivers/gpu/drm/i915/intel_dp.c               | 275 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_dp_link_training.c |   3 +-
 drivers/gpu/drm/i915/intel_dp_mst.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h              |  19 +-
 4 files changed, 166 insertions(+), 135 deletions(-)

This is kind of version 1½ of [1], basically just rebased on current git (including Manasi's test automation patches) and a couple of more cleanups slammed on top.

BR,
Jani.

[1] http://mid.mail-archive.com/[email protected]

Jani Nikula (13):
drm/i915/dp: use known correct array size in rate_to_index drm/i915/dp: return errors from rate_to_index() drm/i915/dp: rename rate_to_index() to intel_dp_find_rate() and reuse drm/i915/dp: cache source rates at init
drm/i915/dp: generate and cache sink rate array for all DP, not just

    eDP 1.4

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

 drivers/gpu/drm/i915/intel_dp.c               | 275 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_dp_link_training.c |   3 +-
 drivers/gpu/drm/i915/intel_dp_mst.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h              |  19 +-
 4 files changed, 166 insertions(+), 135 deletions(-)

Signature · 1 line

2.1.4

Rename the function, move it at the top, and reuse in intel_dp_link_rate_index(). If there was a reason in the past to use reverse search order here, there isn't now.

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 | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

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

index 1d66737a3a0f..f3068ff670a1 100644

drivers/gpu/drm/i915/intel_dp.c +11 -17

+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (rate == rates[i])
+			return i;
+
+	return -1;
+}
+
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {

@@ -284,15 +295,10 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
 				    int *common_rates, int link_rate)
 {
 	int common_len;
-	int index;
 
 	common_len = intel_dp_common_rates(intel_dp, common_rates);
-	for (index = 0; index < common_len; index++) {
-		if (link_rate == common_rates[common_len - index - 1])
-			return common_len - index - 1;
-	}
 
-	return -1;
+	return intel_dp_find_rate(common_rates, common_len, link_rate);
 }
 
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,

@@ -1542,17 +1548,6 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
 	return true;
 }
 
-static int rate_to_index(const int *rates, int len, int rate)
-{
-	int i;
-
-	for (i = 0; i < len; i++)
-		if (rate == rates[i])
-			return i;
-
-	return -1;
-}
-
 int
 intel_dp_max_link_rate(struct intel_dp *intel_dp)
 {

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.

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  | 66 ++++++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h |  3 ++
 2 files changed, 40 insertions(+), 29 deletions(-)

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

index f163391e61fa..7d3ff92000c3 100644
@@ -283,17 +283,29 @@ static int intel_dp_find_rate(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);
 

drivers/gpu/drm/i915/intel_dp.c +30 -20

+
+	/* 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++) {

@@ -304,25 +316,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_find_rate(common_rates, common_len, link_rate);
+	return intel_dp_find_rate(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;

@@ -1509,8 +1519,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)

@@ -1524,8 +1532,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);
 }
 

@@ -1563,14 +1571,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)

@@ -1639,11 +1647,12 @@ 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] = {};
+	const int *common_rates = intel_dp->common_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);

@@ -1681,7 +1690,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;

@@ -3724,6 +3732,7 @@ 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;
 }

@@ -3736,6 +3745,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		return false;
 
 	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)

@@ -3958,7 +3968,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)

@@ -3987,7 +3996,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 +0 -0

index ae7259eda420..3578a7dd584a 100644

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

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 +0 -1

index 7704d32286a3..429dc70c251a 100644
@@ -1639,7 +1639,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;
 	const int *common_rates = intel_dp->common_rates;

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

-		if (link_rate_index >= 0)
-			min_clock = max_clock = link_rate_index;
+		int index;
+
+		index = intel_dp_find_rate(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 · 1 line

2.1.4

Quoted 33 lines · 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.

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  | 66 ++++++++++++++++++++++------------------
 drivers/gpu/drm/i915/intel_drv.h |  3 ++
 2 files changed, 40 insertions(+), 29 deletions(-)

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

index f163391e61fa..7d3ff92000c3 100644
@@ -283,17 +283,29 @@ static int intel_dp_find_rate(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);
 

drivers/gpu/drm/i915/intel_dp.c static int intel_dp_find_rate(const int *rates, int len, int rate) · line 294

Quoted 10 lines

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

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

Do we need to pass all these args given that all of them are now available in intel_dp and intersect_rates() is not used anywhere else?

-DK

Quoted 156 lines

drivers/gpu/drm/i915/intel_dp.c +30 -20

+
+	/* 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++) {

@@ -304,25 +316,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_find_rate(common_rates, common_len, link_rate);
+	return intel_dp_find_rate(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;

@@ -1509,8 +1519,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)

@@ -1524,8 +1532,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);
 }
 

@@ -1563,14 +1571,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)

@@ -1639,11 +1647,12 @@ 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] = {};
+	const int *common_rates = intel_dp->common_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);

@@ -1681,7 +1690,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;

@@ -3724,6 +3732,7 @@ 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;
 }

@@ -3736,6 +3745,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 		return false;
 
 	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)

@@ -3958,7 +3968,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)

@@ -3987,7 +3996,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 ae7259eda420..3578a7dd584a 100644
@@ -936,6 +936,9 @@ struct intel_dp {
 	/* sink rates as reported by DP_MAX_LINK_RATE/DP_SUPPORTED_LINK_RATES */
 	int num_sink_rates;
 	int sink_rates[DP_MAX_SUPPORTED_RATES];
+	/* 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 */

Quoted 24 lines · 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]>

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 +0 -1

index 7704d32286a3..429dc70c251a 100644
@@ -1639,7 +1639,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;
 	const int *common_rates = intel_dp->common_rates;

@@ -1684,11 +1683,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 

drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder *encoder, · line 1689

Quoted 5 lines

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

 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		link_rate_index = intel_dp_find_rate(intel_dp->common_rates,
-						     intel_dp->num_common_rates,
-						     intel_dp->compliance.test_link_rate);

Can we not pass just the common_rates as an argument to this function since common_rates is already assigned intel_dp->common_rates.

Regards
Manasi

Quoted 15 lines

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

-		if (link_rate_index >= 0)
-			min_clock = max_clock = link_rate_index;
+		int index;
+
+		index = intel_dp_find_rate(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 · 1 line

2.1.4

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

Quoted 19 lines · Jani Nikula

Rename the function, move it at the top, and reuse in intel_dp_link_rate_index(). If there was a reason in the past to use reverse search order here, there isn't now.

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 | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

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

index 1d66737a3a0f..f3068ff670a1 100644
@@ -266,6 +266,17 @@ static int intersect_rates(const int *source_rates, int source_len,
 	return k;
 }
 

drivers/gpu/drm/i915/intel_dp.c static int intersect_rates(const int *source_rates, int source_len, · line 269

Quoted 1 line

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

+static int intel_dp_find_rate(const int *rates, int len, int rate)

I wonder if the function name can be more intuitive. The argument is rate and the function name indicates it also returns rate. I can't tell what the function does by it's name. Feel free to ignore this comment as I might be missing some context.

-DK

Quoted 59 lines

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

+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (rate == rates[i])
+			return i;
+
+	return -1;
+}
+
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {

@@ -284,15 +295,10 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
 				    int *common_rates, int link_rate)
 {
 	int common_len;
-	int index;
 
 	common_len = intel_dp_common_rates(intel_dp, common_rates);
-	for (index = 0; index < common_len; index++) {
-		if (link_rate == common_rates[common_len - index - 1])
-			return common_len - index - 1;
-	}
 
-	return -1;
+	return intel_dp_find_rate(common_rates, common_len, link_rate);
 }
 
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,

@@ -1542,17 +1548,6 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
 	return true;
 }
 
-static int rate_to_index(const int *rates, int len, int rate)
-{
-	int i;
-
-	for (i = 0; i < len; i++)
-		if (rate == rates[i])
-			return i;
-
-	return -1;
-}
-
 int
 intel_dp_max_link_rate(struct intel_dp *intel_dp)
 {

@@ -1568,8 +1563,8 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
 
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 {
-	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
-			      rate);
+	int i = intel_dp_find_rate(intel_dp->sink_rates,
+				   intel_dp->num_sink_rates, rate);
 
 	if (WARN_ON(i < 0))
 		i = 0;

Quoted 25 lines · Manasi Navare

Quoted 24 lines · 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]>

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 +0 -1

index 7704d32286a3..429dc70c251a 100644
@@ -1639,7 +1639,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;
 	const int *common_rates = intel_dp->common_rates;

@@ -1684,11 +1683,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder *encoder, · line 1689

Quoted 5 lines

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

 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		link_rate_index = intel_dp_find_rate(intel_dp->common_rates,
-						     intel_dp->num_common_rates,
-						     intel_dp->compliance.test_link_rate);

Can we not pass just the common_rates as an argument to this function since common_rates is already assigned intel_dp->common_rates.

Do you mean just pass intel_dp to intel_dp_find_rate? If yes, I think this keeps intel_dp_find_rate generic, independent of intel_dp or anything else, and you can be sure it's stateless (same as intersect_rates).

If you don't mean that, I don't know what you mean... please explain.

BR,
Jani.

Quoted 16 lines

Regards
Manasi

Quoted 11 lines

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

-		if (link_rate_index >= 0)
-			min_clock = max_clock = link_rate_index;
+		int index;
+
+		index = intel_dp_find_rate(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;
 	}

Quoted 8 lines

Quoted 4 lines

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

 	DRM_DEBUG_KMS("DP link computation with max lane count %i "

Signature · 1 line

2.1.4

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

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Are you planning on submitting a v2 for these pretty soon that can make it to patchwork/

Regards
Manasi

Quoted 7 lines · Jani Nikula

This is kind of version 1½ of [1], basically just rebased on current git (including Manasi's test automation patches) and a couple of more cleanups slammed on top.

BR,
Jani.

Quoted 26 lines

[1] http://mid.mail-archive.com/[email protected]

Jani Nikula (13):
drm/i915/dp: use known correct array size in rate_to_index drm/i915/dp: return errors from rate_to_index() drm/i915/dp: rename rate_to_index() to intel_dp_find_rate() and reuse drm/i915/dp: cache source rates at init
drm/i915/dp: generate and cache sink rate array for all DP, not just

    eDP 1.4

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

 drivers/gpu/drm/i915/intel_dp.c               | 275 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_dp_link_training.c |   3 +-
 drivers/gpu/drm/i915/intel_dp_mst.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h              |  19 +-
 4 files changed, 166 insertions(+), 135 deletions(-)

Quoted 3 lines

Signature · 1 line

2.1.4

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

Quoted 40 lines · Jani Nikula

Quoted 25 lines · Manasi Navare

Quoted 24 lines · 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]>

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 +0 -1

index 7704d32286a3..429dc70c251a 100644
@@ -1639,7 +1639,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;
 	const int *common_rates = intel_dp->common_rates;

@@ -1684,11 +1683,13 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 

Quoted 8 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_compute_config(struct intel_encoder *encoder, · line 1689

Quoted 5 lines

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

 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		link_rate_index = intel_dp_find_rate(intel_dp->common_rates,
-						     intel_dp->num_common_rates,
-						     intel_dp->compliance.test_link_rate);

Can we not pass just the common_rates as an argument to this function since common_rates is already assigned intel_dp->common_rates.

Do you mean just pass intel_dp to intel_dp_find_rate? If yes, I think this keeps intel_dp_find_rate generic, independent of intel_dp or anything else, and you can be sure it's stateless (same as intersect_rates).

Quoted 6 lines

If you don't mean that, I don't know what you mean... please explain.

BR,
Jani.

I agree that it needs to be stateless and hence we dont pass intel_dp directly. What I was saying was that at the beginning of the function we do initialize common_rates = intel_dp->common_rates;
so why cant we pass just common_rates to this function instead of intel_dp->common_rates? Not sure if this optimization will make any big impact but just a thought.

Regards
Manasi

Quoted 27 lines

Quoted 24 lines

Regards
Manasi

Quoted 15 lines

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

-		if (link_rate_index >= 0)
-			min_clock = max_clock = link_rate_index;
+		int index;
+
+		index = intel_dp_find_rate(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 · 1 line

2.1.4

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

Signature · 1 line

Jani Nikula, Intel Open Source Technology Center

Quoted 14 lines · Manasi Navare

Are you planning on submitting a v2 for these pretty soon that can make it to patchwork/

Regards
Manasi

Quoted 7 lines · Jani Nikula

This is kind of version 1½ of [1], basically just rebased on current git (including Manasi's test automation patches) and a couple of more cleanups slammed on top.

BR,
Jani.

So the problem we still have with link rate fallback after link failure->uevent-> fetch new modes->link retrain is that after we send a uevent, userspace fetches new modes which calls drm_helper_probe_single_connector_modes() that calls dp_detect which ends up calling intel_dp_long_pulse(). So while userspace is just trying to fetch new modes it overwrites the max_link_rate and max_lane_count values through intel_dp_long_pulse(). So while retraining, when it calls intel_dp_compute_config(), it will use the overwritten values of common_rates/max_link_rate/max_lane_count as opposed to the ones set in intel_dp_get_fallback_values(). I wonder if this patch series solves this problem somehow... else, do you have any insight on how we can solve this?

Regards
Manasi

Quoted 33 lines

Quoted 20 lines

[1] http://mid.mail-archive.com/[email protected]

Jani Nikula (13):
drm/i915/dp: use known correct array size in rate_to_index drm/i915/dp: return errors from rate_to_index() drm/i915/dp: rename rate_to_index() to intel_dp_find_rate() and reuse drm/i915/dp: cache source rates at init
drm/i915/dp: generate and cache sink rate array for all DP, not just

    eDP 1.4

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

Quoted 9 lines

 drivers/gpu/drm/i915/intel_dp.c               | 275 ++++++++++++++------------
 drivers/gpu/drm/i915/intel_dp_link_training.c |   3 +-
 drivers/gpu/drm/i915/intel_dp_mst.c           |   4 +-
 drivers/gpu/drm/i915/intel_drv.h              |  19 +-
 4 files changed, 166 insertions(+), 135 deletions(-)

Signature · 1 line

2.1.4

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

Quoted 20 lines · Dhinakaran

Quoted 19 lines · Jani Nikula

Rename the function, move it at the top, and reuse in intel_dp_link_rate_index(). If there was a reason in the past to use reverse search order here, there isn't now.

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 | 33 ++++++++++++++-------------------
 1 file changed, 14 insertions(+), 19 deletions(-)

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

index 1d66737a3a0f..f3068ff670a1 100644
@@ -266,6 +266,17 @@ static int intersect_rates(const int *source_rates, int source_len,
 	return k;
 }
 

Quoted 6 lines

drivers/gpu/drm/i915/intel_dp.c static int intersect_rates(const int *source_rates, int source_len, · line 269

Quoted 1 line

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

+static int intel_dp_find_rate(const int *rates, int len, int rate)

I wonder if the function name can be more intuitive. The argument is rate and the function name indicates it also returns rate. I can't tell what the function does by it's name. Feel free to ignore this comment as I might be missing some context.

Naming is hard. intel_dp_rate_index?

BR,
Jani.

Quoted 60 lines

-DK

Quoted 57 lines

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

+{
+	int i;
+
+	for (i = 0; i < len; i++)
+		if (rate == rates[i])
+			return i;
+
+	return -1;
+}
+
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {

@@ -284,15 +295,10 @@ static int intel_dp_link_rate_index(struct intel_dp *intel_dp,
 				    int *common_rates, int link_rate)
 {
 	int common_len;
-	int index;
 
 	common_len = intel_dp_common_rates(intel_dp, common_rates);
-	for (index = 0; index < common_len; index++) {
-		if (link_rate == common_rates[common_len - index - 1])
-			return common_len - index - 1;
-	}
 
-	return -1;
+	return intel_dp_find_rate(common_rates, common_len, link_rate);
 }
 
 int intel_dp_get_link_train_fallback_values(struct intel_dp *intel_dp,

@@ -1542,17 +1548,6 @@ bool intel_dp_read_desc(struct intel_dp *intel_dp)
 	return true;
 }
 
-static int rate_to_index(const int *rates, int len, int rate)
-{
-	int i;
-
-	for (i = 0; i < len; i++)
-		if (rate == rates[i])
-			return i;
-
-	return -1;
-}
-
 int
 intel_dp_max_link_rate(struct intel_dp *intel_dp)
 {

@@ -1568,8 +1563,8 @@ intel_dp_max_link_rate(struct intel_dp *intel_dp)
 
 int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 {
-	int i = rate_to_index(intel_dp->sink_rates, intel_dp->num_sink_rates,
-			      rate);
+	int i = intel_dp_find_rate(intel_dp->sink_rates,
+				   intel_dp->num_sink_rates, rate);
 

Quoted 3 lines

drivers/gpu/drm/i915/intel_dp.c intel_dp_max_link_rate(struct intel_dp *intel_dp) · line 1570

Quoted 2 lines

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

 	if (WARN_ON(i < 0))
 		i = 0;