[PATCH 0/7] drm/i915/dp: link config compute refactoring

[PATCH 0/7] drm/i915/dp: link config compute refactoring

[Jani Nikula]

I looked into potentially applying a fast-and-narrow link config policy on eDP,
and realized it's pretty difficult to do this sensibly with the current
code. The intel_dp_compute_config() function has grown out of hands. Refactor.

The functional changes should be minimal, apart from some debug logging and
potential compliance test fixes.

I think this should make it easier to add the DSC code too.

BR,
Jani.

Jani Nikula (7):
  drm/i915/dp: remove stale comment about bw constants
  drm/i915/dp: move link_bw and rate_select debugging where used
  drm/i915/dp: abstract dp link config computation from the rest
  drm/i915/dp: move eDP VBT bpp claming code to intel_dp_compute_bpp()
  drm/i915/dp: group link config limits in a struct
  drm/i915/dp: abstract link config selection
  drm/i915/dp: fix compliance test adjustments

 drivers/gpu/drm/i915/intel_dp.c               | 276 +++++++++++++++-----------
 drivers/gpu/drm/i915/intel_dp_link_training.c |   5 +
 2 files changed, 168 insertions(+), 113 deletions(-)

-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 1/7] drm/i915/dp: remove stale comment about bw constants

[Jani Nikula]

We haven't used the DP bw constants here for a while. No functional
changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 62f82c4298ac..5f4b30faf6a2 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1701,7 +1701,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int lane_count, clock;
 	int min_lane_count = 1;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
-	/* Conveniently, the link BW constants become indices with a shift...*/
 	int min_clock = 0;
 	int max_clock;
 	int bpp, mode_rate;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 2/7] drm/i915/dp: move link_bw and rate_select debugging where used

[Jani Nikula]

The debug prints make more sense where the results are actually used,
and this cleans up extra clutter from the already overcrowded
intel_dp_compute_config().

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c               | 9 ++-------
 drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 5f4b30faf6a2..81cf363e71af 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_len;
-	uint8_t link_bw, rate_select;
 	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
 					   DP_DPCD_QUIRK_LIMITED_M_N);
 
@@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	pipe_config->pipe_bpp = bpp;
 	pipe_config->port_clock = intel_dp->common_rates[clock];
 
-	intel_dp_compute_rate(intel_dp, pipe_config->port_clock,
-			      &link_bw, &rate_select);
-
-	DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n",
-		      link_bw, rate_select, pipe_config->lane_count,
-		      pipe_config->port_clock, bpp);
+	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
+		      pipe_config->lane_count, pipe_config->port_clock, bpp);
 	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
 		      mode_rate, link_avail);
 
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index f59b59bb0a21..3fcaa98b9055 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	intel_dp_compute_rate(intel_dp, intel_dp->link_rate,
 			      &link_bw, &rate_select);
 
+	if (link_bw)
+		DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw);
+	else
+		DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select);
+
 	/* Write the link configuration data */
 	link_config[0] = link_bw;
 	link_config[1] = intel_dp->lane_count;
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 3/7] drm/i915/dp: abstract dp link config computation from the rest

[Jani Nikula]

Abstract a new intel_dp_compute_link_config() from
intel_dp_compute_config(), with the parts related to link configuration,
i.e. bpp, link rate, and lane count selection. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 160 ++++++++++++++++++++++------------------
 1 file changed, 87 insertions(+), 73 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 81cf363e71af..19fe5eb8d32a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1685,19 +1685,14 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
 	return bres;
 }
 
-bool
-intel_dp_compute_config(struct intel_encoder *encoder,
-			struct intel_crtc_state *pipe_config,
-			struct drm_connector_state *conn_state)
+static bool
+intel_dp_compute_link_config(struct intel_encoder *encoder,
+			     struct intel_crtc_state *pipe_config)
 {
 	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	enum port port = encoder->port;
-	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
 	struct intel_connector *intel_connector = intel_dp->attached_connector;
-	struct intel_digital_connector_state *intel_conn_state =
-		to_intel_digital_connector_state(conn_state);
 	int lane_count, clock;
 	int min_lane_count = 1;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
@@ -1706,9 +1701,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	int bpp, mode_rate;
 	int link_avail, link_clock;
 	int common_len;
-	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
-					   DP_DPCD_QUIRK_LIMITED_M_N);
-
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
 						    intel_dp->max_link_rate);
 
@@ -1717,51 +1709,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 
 	max_clock = common_len - 1;
 
-	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
-		pipe_config->has_pch_encoder = true;
-
-	pipe_config->has_drrs = false;
-	if (IS_G4X(dev_priv) || port == PORT_A)
-		pipe_config->has_audio = false;
-	else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
-		pipe_config->has_audio = intel_dp->has_audio;
-	else
-		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
-
-	if (intel_dp_is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
-		struct drm_display_mode *panel_mode =
-			intel_connector->panel.alt_fixed_mode;
-		struct drm_display_mode *req_mode = &pipe_config->base.mode;
-
-		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
-			panel_mode = intel_connector->panel.fixed_mode;
-
-		drm_mode_debug_printmodeline(panel_mode);
-
-		intel_fixed_panel_mode(panel_mode, adjusted_mode);
-
-		if (INTEL_GEN(dev_priv) >= 9) {
-			int ret;
-			ret = skl_update_scaler_crtc(pipe_config);
-			if (ret)
-				return ret;
-		}
-
-		if (HAS_GMCH_DISPLAY(dev_priv))
-			intel_gmch_panel_fitting(intel_crtc, pipe_config,
-						 conn_state->scaling_mode);
-		else
-			intel_pch_panel_fitting(intel_crtc, pipe_config,
-						conn_state->scaling_mode);
-	}
-
-	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
-	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
-		return false;
-
-	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
-		return false;
-
 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
 		int index;
@@ -1831,6 +1778,82 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	return false;
 
 found:
+	pipe_config->lane_count = lane_count;
+	pipe_config->pipe_bpp = bpp;
+	pipe_config->port_clock = intel_dp->common_rates[clock];
+
+	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
+		      pipe_config->lane_count, pipe_config->port_clock, bpp);
+	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
+		      mode_rate, link_avail);
+
+	return true;
+}
+
+bool
+intel_dp_compute_config(struct intel_encoder *encoder,
+			struct intel_crtc_state *pipe_config,
+			struct drm_connector_state *conn_state)
+{
+	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
+	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
+	enum port port = encoder->port;
+	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
+	struct intel_digital_connector_state *intel_conn_state =
+		to_intel_digital_connector_state(conn_state);
+	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
+					   DP_DPCD_QUIRK_LIMITED_M_N);
+
+	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
+		pipe_config->has_pch_encoder = true;
+
+	pipe_config->has_drrs = false;
+	if (IS_G4X(dev_priv) || port == PORT_A)
+		pipe_config->has_audio = false;
+	else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
+		pipe_config->has_audio = intel_dp->has_audio;
+	else
+		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
+
+	if (intel_dp_is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
+		struct drm_display_mode *panel_mode =
+			intel_connector->panel.alt_fixed_mode;
+		struct drm_display_mode *req_mode = &pipe_config->base.mode;
+
+		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
+			panel_mode = intel_connector->panel.fixed_mode;
+
+		drm_mode_debug_printmodeline(panel_mode);
+
+		intel_fixed_panel_mode(panel_mode, adjusted_mode);
+
+		if (INTEL_GEN(dev_priv) >= 9) {
+			int ret;
+			ret = skl_update_scaler_crtc(pipe_config);
+			if (ret)
+				return ret;
+		}
+
+		if (HAS_GMCH_DISPLAY(dev_priv))
+			intel_gmch_panel_fitting(intel_crtc, pipe_config,
+						 conn_state->scaling_mode);
+		else
+			intel_pch_panel_fitting(intel_crtc, pipe_config,
+						conn_state->scaling_mode);
+	}
+
+	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
+	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
+		return false;
+
+	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
+		return false;
+
+	if (!intel_dp_compute_link_config(encoder, pipe_config))
+		return false;
+
 	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
 		/*
 		 * See:
@@ -1838,7 +1861,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 		 * VESA DisplayPort Ver.1.2a - 5.1.1.1 Video Colorimetry
 		 */
 		pipe_config->limited_color_range =
-			bpp != 18 &&
+			pipe_config->pipe_bpp != 18 &&
 			drm_default_rgb_quant_range(adjusted_mode) ==
 			HDMI_QUANTIZATION_RANGE_LIMITED;
 	} else {
@@ -1846,17 +1869,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
 	}
 
-	pipe_config->lane_count = lane_count;
-
-	pipe_config->pipe_bpp = bpp;
-	pipe_config->port_clock = intel_dp->common_rates[clock];
-
-	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
-		      pipe_config->lane_count, pipe_config->port_clock, bpp);
-	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
-		      mode_rate, link_avail);
-
-	intel_link_compute_m_n(bpp, lane_count,
+	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
 			       adjusted_mode->crtc_clock,
 			       pipe_config->port_clock,
 			       &pipe_config->dp_m_n,
@@ -1865,11 +1878,12 @@ intel_dp_compute_config(struct intel_encoder *encoder,
 	if (intel_connector->panel.downclock_mode != NULL &&
 		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
 			pipe_config->has_drrs = true;
-			intel_link_compute_m_n(bpp, lane_count,
-				intel_connector->panel.downclock_mode->clock,
-				pipe_config->port_clock,
-				&pipe_config->dp_m2_n2,
-				reduce_m_n);
+			intel_link_compute_m_n(pipe_config->pipe_bpp,
+					       pipe_config->lane_count,
+					       intel_connector->panel.downclock_mode->clock,
+					       pipe_config->port_clock,
+					       &pipe_config->dp_m2_n2,
+					       reduce_m_n);
 	}
 
 	/*
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 4/7] drm/i915/dp: move eDP VBT bpp claming code to intel_dp_compute_bpp()

[Jani Nikula]

Keep related things together. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++++-----------
 1 file changed, 13 insertions(+), 11 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 19fe5eb8d32a..dd42e0422af6 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1650,6 +1650,8 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config)
 {
+	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
+	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int bpp, bpc;
 
 	bpp = pipe_config->pipe_bpp;
@@ -1665,6 +1667,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
 			      pipe_config->pipe_bpp);
 	}
+
+	if (intel_dp_is_edp(intel_dp)) {
+		/* Get bpp from vbt only for panels that dont have bpp in edid */
+		if (intel_connector->base.display_info.bpc == 0 &&
+			(dev_priv->vbt.edp.bpp && dev_priv->vbt.edp.bpp < bpp)) {
+			DRM_DEBUG_KMS("clamping bpp for eDP panel to BIOS-provided %i\n",
+				      dev_priv->vbt.edp.bpp);
+			bpp = dev_priv->vbt.edp.bpp;
+		}
+	}
+
 	return bpp;
 }
 
@@ -1689,10 +1702,8 @@ static bool
 intel_dp_compute_link_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config)
 {
-	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	struct intel_connector *intel_connector = intel_dp->attached_connector;
 	int lane_count, clock;
 	int min_lane_count = 1;
 	int max_lane_count = intel_dp_max_lane_count(intel_dp);
@@ -1735,15 +1746,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 	 * bpc in between. */
 	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 	if (intel_dp_is_edp(intel_dp)) {
-
-		/* Get bpp from vbt only for panels that dont have bpp in edid */
-		if (intel_connector->base.display_info.bpc == 0 &&
-			(dev_priv->vbt.edp.bpp && dev_priv->vbt.edp.bpp < bpp)) {
-			DRM_DEBUG_KMS("clamping bpp for eDP panel to BIOS-provided %i\n",
-				      dev_priv->vbt.edp.bpp);
-			bpp = dev_priv->vbt.edp.bpp;
-		}
-
 		/*
 		 * Use the maximum clock and number of lanes the eDP panel
 		 * advertizes being capable of. The panels are generally
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 5/7] drm/i915/dp: group link config limits in a struct

[Jani Nikula]

Also use same min/max model for bpp, and adjust debug logging while at
it.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 57 ++++++++++++++++++++++++-----------------
 1 file changed, 33 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index dd42e0422af6..3c5fbdf42b9b 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1647,6 +1647,12 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 	}
 }
 
+struct link_config_limits {
+	int min_clock, max_clock;
+	int min_lane_count, max_lane_count;
+	int min_bpp, max_bpp;
+};
+
 static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 				struct intel_crtc_state *pipe_config)
 {
@@ -1704,21 +1710,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 {
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
-	int lane_count, clock;
-	int min_lane_count = 1;
-	int max_lane_count = intel_dp_max_lane_count(intel_dp);
-	int min_clock = 0;
-	int max_clock;
-	int bpp, mode_rate;
-	int link_avail, link_clock;
+	struct link_config_limits limits;
+	int bpp, clock, lane_count;
+	int mode_rate, link_avail, link_clock;
 	int common_len;
+
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
 						    intel_dp->max_link_rate);
 
 	/* No common link rates between source and sink */
 	WARN_ON(common_len <= 0);
 
-	max_clock = common_len - 1;
+	limits.min_clock = 0;
+	limits.max_clock = common_len - 1;
+
+	limits.min_lane_count = 1;
+	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
+
+	limits.min_bpp = 6 * 3;
+	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 
 	/* Use values requested by Compliance Test Request */
 	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
@@ -1733,18 +1743,11 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 						    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;
+				limits.min_clock = limits.max_clock = index;
+			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;
 		}
 	}
-	DRM_DEBUG_KMS("DP link computation with max lane count %i "
-		      "max bw %d pixel clock %iKHz\n",
-		      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
-	 * bpc in between. */
-	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 	if (intel_dp_is_edp(intel_dp)) {
 		/*
 		 * Use the maximum clock and number of lanes the eDP panel
@@ -1753,18 +1756,24 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 		 * configuration, and typically these values correspond to the
 		 * native resolution of the panel.
 		 */
-		min_lane_count = max_lane_count;
-		min_clock = max_clock;
+		limits.min_lane_count = limits.max_lane_count;
+		limits.min_clock = limits.max_clock;
 	}
 
-	for (; bpp >= 6*3; bpp -= 2*3) {
+	DRM_DEBUG_KMS("DP link computation with max lane count %i "
+		      "max rate %d max bpp %d pixel clock %iKHz\n",
+		      limits.max_lane_count,
+		      intel_dp->common_rates[limits.max_clock],
+		      limits.max_bpp, adjusted_mode->crtc_clock);
+
+	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
 		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
 						   bpp);
 
-		for (clock = min_clock; clock <= max_clock; clock++) {
-			for (lane_count = min_lane_count;
-				lane_count <= max_lane_count;
-				lane_count <<= 1) {
+		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
+			for (lane_count = limits.min_lane_count;
+			     lane_count <= limits.max_lane_count;
+			     lane_count <<= 1) {
 
 				link_clock = intel_dp->common_rates[clock];
 				link_avail = intel_dp_max_data_rate(link_clock,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 6/7] drm/i915/dp: abstract link config selection

[Jani Nikula]

For now, there's just the one link config selection, optimizing for slow
and wide link. No functional changes.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 81 +++++++++++++++++++++++++----------------
 1 file changed, 50 insertions(+), 31 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 3c5fbdf42b9b..c98626b3af65 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1704,6 +1704,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
 	return bres;
 }
 
+/* Optimize link config in order: max bpp, min clock, min lanes */
+static bool
+intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
+				  struct intel_crtc_state *pipe_config,
+				  const struct link_config_limits *limits)
+{
+	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
+	int bpp, clock, lane_count;
+	int mode_rate, link_clock, link_avail;
+
+	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
+		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
+						   bpp);
+
+		for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
+			for (lane_count = limits->min_lane_count;
+			     lane_count <= limits->max_lane_count;
+			     lane_count <<= 1) {
+				link_clock = intel_dp->common_rates[clock];
+				link_avail = intel_dp_max_data_rate(link_clock,
+								    lane_count);
+
+				if (mode_rate <= link_avail) {
+					pipe_config->lane_count = lane_count;
+					pipe_config->pipe_bpp = bpp;
+					pipe_config->port_clock = link_clock;
+
+					return true;
+				}
+			}
+		}
+	}
+
+	return false;
+}
+
 static bool
 intel_dp_compute_link_config(struct intel_encoder *encoder,
 			     struct intel_crtc_state *pipe_config)
@@ -1711,8 +1747,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct link_config_limits limits;
-	int bpp, clock, lane_count;
-	int mode_rate, link_avail, link_clock;
 	int common_len;
 
 	common_len = intel_dp_common_len_rate_limit(intel_dp,
@@ -1766,37 +1800,22 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 		      intel_dp->common_rates[limits.max_clock],
 		      limits.max_bpp, adjusted_mode->crtc_clock);
 
-	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
-		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
-						   bpp);
-
-		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
-			for (lane_count = limits.min_lane_count;
-			     lane_count <= limits.max_lane_count;
-			     lane_count <<= 1) {
-
-				link_clock = intel_dp->common_rates[clock];
-				link_avail = intel_dp_max_data_rate(link_clock,
-								    lane_count);
-
-				if (mode_rate <= link_avail) {
-					goto found;
-				}
-			}
-		}
-	}
-
-	return false;
-
-found:
-	pipe_config->lane_count = lane_count;
-	pipe_config->pipe_bpp = bpp;
-	pipe_config->port_clock = intel_dp->common_rates[clock];
+	/*
+	 * Optimize for slow and wide. This is the place to add alternative
+	 * optimization policy.
+	 */
+	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
+		return false;
 
 	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
-		      pipe_config->lane_count, pipe_config->port_clock, bpp);
-	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
-		      mode_rate, link_avail);
+		      pipe_config->lane_count, pipe_config->port_clock,
+		      pipe_config->pipe_bpp);
+
+	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
+		      intel_dp_link_required(adjusted_mode->crtc_clock,
+					     pipe_config->pipe_bpp),
+		      intel_dp_max_data_rate(pipe_config->port_clock,
+					     pipe_config->lane_count));
 
 	return true;
 }
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 7/7] drm/i915/dp: fix compliance test adjustments

[Jani Nikula]

Abstract compliance test adjustments to a single function. Also make the
bpc adjustments affect the limits, actually forcing the bpc. Seems like
directly changing the pipe_bpp in the past could not have been
effective.

Signed-off-by: Jani Nikula <jani.nikula@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 64 ++++++++++++++++++++++++-----------------
 1 file changed, 38 insertions(+), 26 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c98626b3af65..4ddb9dc61f46 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1666,14 +1666,6 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
 	if (bpc > 0)
 		bpp = min(bpp, 3*bpc);
 
-	/* For DP Compliance we override the computed bpp for the pipe */
-	if (intel_dp->compliance.test_data.bpc != 0) {
-		pipe_config->pipe_bpp =	3*intel_dp->compliance.test_data.bpc;
-		pipe_config->dither_force_disable = pipe_config->pipe_bpp == 6*3;
-		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
-			      pipe_config->pipe_bpp);
-	}
-
 	if (intel_dp_is_edp(intel_dp)) {
 		/* Get bpp from vbt only for panels that dont have bpp in edid */
 		if (intel_connector->base.display_info.bpc == 0 &&
@@ -1704,6 +1696,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
 	return bres;
 }
 
+/* Adjust link config limits based on compliance test requests. */
+static void
+intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
+				  struct intel_crtc_state *pipe_config,
+				  struct link_config_limits *limits)
+{
+	/* For DP Compliance we override the computed bpp for the pipe */
+	if (intel_dp->compliance.test_data.bpc != 0) {
+		int bpp = 3 * intel_dp->compliance.test_data.bpc;
+
+		limits->min_bpp = limits->max_bpp = bpp;
+		pipe_config->dither_force_disable = bpp == 6 * 3;
+
+		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n", bpp);
+	}
+
+	/* Use values requested by Compliance Test Request */
+	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
+		int index;
+
+		/* Validate the compliance test data since max values
+		 * might have changed due to link train fallback.
+		 */
+		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
+					       intel_dp->compliance.test_lane_count)) {
+			index = intel_dp_rate_index(intel_dp->common_rates,
+						    intel_dp->num_common_rates,
+						    intel_dp->compliance.test_link_rate);
+			if (index >= 0)
+				limits->min_clock = limits->max_clock = index;
+			limits->min_lane_count = limits->max_lane_count =
+				intel_dp->compliance.test_lane_count;
+		}
+	}
+}
+
 /* Optimize link config in order: max bpp, min clock, min lanes */
 static bool
 intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
@@ -1764,24 +1792,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 	limits.min_bpp = 6 * 3;
 	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
 
-	/* Use values requested by Compliance Test Request */
-	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
-		int index;
-
-		/* Validate the compliance test data since max values
-		 * might have changed due to link train fallback.
-		 */
-		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
-					       intel_dp->compliance.test_lane_count)) {
-			index = intel_dp_rate_index(intel_dp->common_rates,
-						    intel_dp->num_common_rates,
-						    intel_dp->compliance.test_link_rate);
-			if (index >= 0)
-				limits.min_clock = limits.max_clock = index;
-			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;
-		}
-	}
-
 	if (intel_dp_is_edp(intel_dp)) {
 		/*
 		 * Use the maximum clock and number of lanes the eDP panel
@@ -1794,6 +1804,8 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
 		limits.min_clock = limits.max_clock;
 	}
 
+	intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits);
+
 	DRM_DEBUG_KMS("DP link computation with max lane count %i "
 		      "max rate %d max bpp %d pixel clock %iKHz\n",
 		      limits.max_lane_count,
-- 
2.11.0

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✗ Fi.CI.CHECKPATCH: warning for drm/i915/dp: link config compute refactoring

[Patchwork]

== Series Details ==

Series: drm/i915/dp: link config compute refactoring
URL   : https://patchwork.freedesktop.org/series/41215/
State : warning

== Summary ==

$ dim checkpatch origin/drm-tip
19ed1ef3a8e0 drm/i915/dp: remove stale comment about bw constants
462b0f09da14 drm/i915/dp: move link_bw and rate_select debugging where used
91ccfa6a90e0 drm/i915/dp: abstract dp link config computation from the rest
-:159: WARNING:LINE_SPACING: Missing a blank line after declarations
#159: FILE: drivers/gpu/drm/i915/intel_dp.c:1834:
+			int ret;
+			ret = skl_update_scaler_crtc(pipe_config);

total: 0 errors, 1 warnings, 0 checks, 207 lines checked
a41f8820d9e6 drm/i915/dp: move eDP VBT bpp claming code to intel_dp_compute_bpp()
-:32: CHECK:PARENTHESIS_ALIGNMENT: Alignment should match open parenthesis
#32: FILE: drivers/gpu/drm/i915/intel_dp.c:1674:
+		if (intel_connector->base.display_info.bpc == 0 &&
+			(dev_priv->vbt.edp.bpp && dev_priv->vbt.edp.bpp < bpp)) {

total: 0 errors, 0 warnings, 1 checks, 50 lines checked
ee8d5e24aa74 drm/i915/dp: group link config limits in a struct
-:68: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#68: FILE: drivers/gpu/drm/i915/intel_dp.c:1746:
+				limits.min_clock = limits.max_clock = index;

-:69: WARNING:LONG_LINE: line over 100 characters
#69: FILE: drivers/gpu/drm/i915/intel_dp.c:1747:
+			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;

-:69: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#69: FILE: drivers/gpu/drm/i915/intel_dp.c:1747:
+			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;

total: 0 errors, 1 warnings, 2 checks, 96 lines checked
046eaebcfc30 drm/i915/dp: abstract link config selection
d714852b2c0a drm/i915/dp: fix compliance test adjustments
-:46: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#46: FILE: drivers/gpu/drm/i915/intel_dp.c:1709:
+		limits->min_bpp = limits->max_bpp = bpp;

-:65: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#65: FILE: drivers/gpu/drm/i915/intel_dp.c:1728:
+				limits->min_clock = limits->max_clock = index;

-:66: CHECK:MULTIPLE_ASSIGNMENTS: multiple assignments should be avoided
#66: FILE: drivers/gpu/drm/i915/intel_dp.c:1729:
+			limits->min_lane_count = limits->max_lane_count =

total: 0 errors, 0 warnings, 3 checks, 88 lines checked

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.BAT: success for drm/i915/dp: link config compute refactoring

[Patchwork]

== Series Details ==

Series: drm/i915/dp: link config compute refactoring
URL   : https://patchwork.freedesktop.org/series/41215/
State : success

== Summary ==

Series 41215v1 drm/i915/dp: link config compute refactoring
https://patchwork.freedesktop.org/api/1.0/series/41215/revisions/1/mbox/

---- Known issues:

Test kms_pipe_crc_basic:
        Subgroup suspend-read-crc-pipe-c:
                dmesg-warn -> PASS       (fi-glk-j4005) fdo#105644
Test prime_vgem:
        Subgroup basic-fence-flip:
                fail       -> PASS       (fi-ilk-650) fdo#104008

fdo#105644 https://bugs.freedesktop.org/show_bug.cgi?id=105644
fdo#104008 https://bugs.freedesktop.org/show_bug.cgi?id=104008

fi-bdw-5557u     total:285  pass:264  dwarn:0   dfail:0   fail:0   skip:21  time:431s
fi-bdw-gvtdvm    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:440s
fi-blb-e6850     total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:386s
fi-bsw-n3050     total:285  pass:239  dwarn:0   dfail:0   fail:0   skip:46  time:539s
fi-bwr-2160      total:285  pass:180  dwarn:0   dfail:0   fail:0   skip:105 time:300s
fi-bxt-dsi       total:285  pass:255  dwarn:0   dfail:0   fail:0   skip:30  time:517s
fi-bxt-j4205     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:528s
fi-byt-j1900     total:285  pass:250  dwarn:0   dfail:0   fail:0   skip:35  time:530s
fi-byt-n2820     total:285  pass:246  dwarn:0   dfail:0   fail:0   skip:39  time:515s
fi-cfl-8700k     total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:414s
fi-cfl-s3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:562s
fi-cfl-u         total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:511s
fi-cnl-y3        total:285  pass:259  dwarn:0   dfail:0   fail:0   skip:26  time:581s
fi-elk-e7500     total:285  pass:226  dwarn:0   dfail:0   fail:0   skip:59  time:423s
fi-gdg-551       total:285  pass:176  dwarn:0   dfail:0   fail:1   skip:108 time:317s
fi-glk-1         total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:540s
fi-glk-j4005     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:488s
fi-hsw-4770      total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:404s
fi-ilk-650       total:285  pass:225  dwarn:0   dfail:0   fail:0   skip:60  time:431s
fi-ivb-3520m     total:285  pass:256  dwarn:0   dfail:0   fail:0   skip:29  time:462s
fi-ivb-3770      total:285  pass:252  dwarn:0   dfail:0   fail:0   skip:33  time:436s
fi-kbl-7500u     total:285  pass:260  dwarn:1   dfail:0   fail:0   skip:24  time:474s
fi-kbl-7567u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:462s
fi-kbl-r         total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:511s
fi-pnv-d510      total:285  pass:220  dwarn:1   dfail:0   fail:0   skip:64  time:665s
fi-skl-6260u     total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:442s
fi-skl-6600u     total:285  pass:258  dwarn:0   dfail:0   fail:0   skip:27  time:534s
fi-skl-6700k2    total:285  pass:261  dwarn:0   dfail:0   fail:0   skip:24  time:508s
fi-skl-6770hq    total:285  pass:265  dwarn:0   dfail:0   fail:0   skip:20  time:510s
fi-skl-guc       total:285  pass:257  dwarn:0   dfail:0   fail:0   skip:28  time:432s
fi-skl-gvtdvm    total:285  pass:262  dwarn:0   dfail:0   fail:0   skip:23  time:447s
fi-snb-2520m     total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:587s
fi-snb-2600      total:285  pass:245  dwarn:0   dfail:0   fail:0   skip:40  time:410s
Blacklisted hosts:
fi-cnl-psr       total:285  pass:256  dwarn:3   dfail:0   fail:0   skip:26  time:516s

0eddede73765b01ec287cad00e23bee23c216a16 drm-tip: 2018y-04m-05d-09h-51m-03s UTC integration manifest
d714852b2c0a drm/i915/dp: fix compliance test adjustments
046eaebcfc30 drm/i915/dp: abstract link config selection
ee8d5e24aa74 drm/i915/dp: group link config limits in a struct
a41f8820d9e6 drm/i915/dp: move eDP VBT bpp claming code to intel_dp_compute_bpp()
91ccfa6a90e0 drm/i915/dp: abstract dp link config computation from the rest
462b0f09da14 drm/i915/dp: move link_bw and rate_select debugging where used
19ed1ef3a8e0 drm/i915/dp: remove stale comment about bw constants

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8598/issues.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/dp: remove stale comment about bw constants

[Rodrigo Vivi]

On Thu, Apr 05, 2018 at 05:38:59PM +0300, Jani Nikula wrote:
> We haven't used the DP bw constants here for a while. No functional
> changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..5f4b30faf6a2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1701,7 +1701,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	int lane_count, clock;
>  	int min_lane_count = 1;
>  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> -	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int min_clock = 0;
>  	int max_clock;
>  	int bpp, mode_rate;
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915/dp: move link_bw and rate_select debugging where used

[Rodrigo Vivi]

On Thu, Apr 05, 2018 at 05:39:00PM +0300, Jani Nikula wrote:
> The debug prints make more sense where the results are actually used,
> and this cleans up extra clutter from the already overcrowded
> intel_dp_compute_config().
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c               | 9 ++-------
>  drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++
>  2 files changed, 7 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 5f4b30faf6a2..81cf363e71af 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	int bpp, mode_rate;
>  	int link_avail, link_clock;
>  	int common_len;
> -	uint8_t link_bw, rate_select;
>  	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
>  					   DP_DPCD_QUIRK_LIMITED_M_N);
>  
> @@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	pipe_config->pipe_bpp = bpp;
>  	pipe_config->port_clock = intel_dp->common_rates[clock];
>  
> -	intel_dp_compute_rate(intel_dp, pipe_config->port_clock,
> -			      &link_bw, &rate_select);

the commit message state more about the debug message itself,
but by removing this call here it seems that you change
the behavior of eDP. So I couldn't follow the changes you are
actually aiming to do here.

> -
> -	DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n",
> -		      link_bw, rate_select, pipe_config->lane_count,
> -		      pipe_config->port_clock, bpp);
> +	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> +		      pipe_config->lane_count, pipe_config->port_clock, bpp);
>  	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>  		      mode_rate, link_avail);
>  
> diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> index f59b59bb0a21..3fcaa98b9055 100644
> --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> @@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  	intel_dp_compute_rate(intel_dp, intel_dp->link_rate,
>  			      &link_bw, &rate_select);
>  
> +	if (link_bw)
> +		DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw);
> +	else
> +		DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select);
> +
>  	/* Write the link configuration data */
>  	link_config[0] = link_bw;
>  	link_config[1] = intel_dp->lane_count;
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

✓ Fi.CI.IGT: success for drm/i915/dp: link config compute refactoring

[Patchwork]

== Series Details ==

Series: drm/i915/dp: link config compute refactoring
URL   : https://patchwork.freedesktop.org/series/41215/
State : success

== Summary ==

---- Possible new issues:

Test kms_cursor_legacy:
        Subgroup cursor-vs-flip-toggle:
                fail       -> PASS       (shard-hsw)

---- Known issues:

Test kms_flip:
        Subgroup 2x-dpms-vs-vblank-race:
                pass       -> FAIL       (shard-hsw) fdo#103060 +1
        Subgroup flip-vs-expired-vblank-interruptible:
                pass       -> FAIL       (shard-hsw) fdo#102887
Test kms_flip_tiling:
        Subgroup flip-to-yf-tiled:
                pass       -> FAIL       (shard-apl) fdo#103822
Test kms_frontbuffer_tracking:
        Subgroup fbc-1p-primscrn-pri-indfb-draw-render:
                fail       -> PASS       (shard-snb) fdo#103167
Test kms_setmode:
        Subgroup basic:
                fail       -> PASS       (shard-apl) fdo#99912

fdo#103060 https://bugs.freedesktop.org/show_bug.cgi?id=103060
fdo#102887 https://bugs.freedesktop.org/show_bug.cgi?id=102887
fdo#103822 https://bugs.freedesktop.org/show_bug.cgi?id=103822
fdo#103167 https://bugs.freedesktop.org/show_bug.cgi?id=103167
fdo#99912 https://bugs.freedesktop.org/show_bug.cgi?id=99912

shard-apl        total:2680 pass:1835 dwarn:1   dfail:0   fail:7   skip:836 time:12686s
shard-hsw        total:2680 pass:1783 dwarn:2   dfail:0   fail:3   skip:891 time:11538s
shard-snb        total:2680 pass:1376 dwarn:1   dfail:0   fail:4   skip:1299 time:6944s
Blacklisted hosts:
shard-kbl        total:2622 pass:1860 dwarn:18  dfail:1   fail:20  skip:722 time:8645s

== Logs ==

For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/Patchwork_8598/shards.html
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 1/7] drm/i915/dp: remove stale comment about bw constants

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:38:59PM +0300, Jani Nikula wrote:
> We haven't used the DP bw constants here for a while. No functional
> changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 62f82c4298ac..5f4b30faf6a2 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1701,7 +1701,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	int lane_count, clock;
>  	int min_lane_count = 1;
>  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> -	/* Conveniently, the link BW constants become indices with a shift...*/
>  	int min_clock = 0;
>  	int max_clock;
>  	int bpp, mode_rate;
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 2/7] drm/i915/dp: move link_bw and rate_select debugging where used

[Manasi Navare]

On Thu, Apr 05, 2018 at 10:22:38AM -0700, Rodrigo Vivi wrote:
> On Thu, Apr 05, 2018 at 05:39:00PM +0300, Jani Nikula wrote:
> > The debug prints make more sense where the results are actually used,
> > and this cleans up extra clutter from the already overcrowded
> > intel_dp_compute_config().
> > 
> > Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c               | 9 ++-------
> >  drivers/gpu/drm/i915/intel_dp_link_training.c | 5 +++++
> >  2 files changed, 7 insertions(+), 7 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 5f4b30faf6a2..81cf363e71af 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1706,7 +1706,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	int bpp, mode_rate;
> >  	int link_avail, link_clock;
> >  	int common_len;
> > -	uint8_t link_bw, rate_select;
> >  	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
> >  					   DP_DPCD_QUIRK_LIMITED_M_N);
> >  
> > @@ -1852,12 +1851,8 @@ intel_dp_compute_config(struct intel_encoder *encoder,
> >  	pipe_config->pipe_bpp = bpp;
> >  	pipe_config->port_clock = intel_dp->common_rates[clock];
> >  
> > -	intel_dp_compute_rate(intel_dp, pipe_config->port_clock,
> > -			      &link_bw, &rate_select);
> 
> the commit message state more about the debug message itself,
> but by removing this call here it seems that you change
> the behavior of eDP. So I couldn't follow the changes you are
> actually aiming to do here.
>

Actually in dp_compute_config(), we just compute the optimum port_clock
selected from the common_rates array. We dont really use rate_select or link_bw here
in this function. These two parameters get used directly in the clock_recovery
where we either write rate_select or link_bw to the dpcd link config registers.
So by removing this call to intel_dp_compute_rate() from compute_config() makes sense
to me. And it does not alter the eDP compute config behaviour.

However I think to be clear, the above explanation needs to be included in th commit message.
With that included,

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi 
> > -
> > -	DRM_DEBUG_KMS("DP link bw %02x rate select %02x lane count %d clock %d bpp %d\n",
> > -		      link_bw, rate_select, pipe_config->lane_count,
> > -		      pipe_config->port_clock, bpp);
> > +	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> > +		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> >  	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> >  		      mode_rate, link_avail);
> >  
> > diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > index f59b59bb0a21..3fcaa98b9055 100644
> > --- a/drivers/gpu/drm/i915/intel_dp_link_training.c
> > +++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
> > @@ -139,6 +139,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >  	intel_dp_compute_rate(intel_dp, intel_dp->link_rate,
> >  			      &link_bw, &rate_select);
> >  
> > +	if (link_bw)
> > +		DRM_DEBUG_KMS("Using LINK_BW_SET value %02x\n", link_bw);
> > +	else
> > +		DRM_DEBUG_KMS("Using LINK_RATE_SET value %02x\n", rate_select);
> > +
> >  	/* Write the link configuration data */
> >  	link_config[0] = link_bw;
> >  	link_config[1] = intel_dp->lane_count;
> > -- 
> > 2.11.0
> > 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 4/7] drm/i915/dp: move eDP VBT bpp claming code to intel_dp_compute_bpp()

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:39:02PM +0300, Jani Nikula wrote:
> Keep related things together. No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---

Definitely looks more organized.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

>  drivers/gpu/drm/i915/intel_dp.c | 24 +++++++++++++-----------
>  1 file changed, 13 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 19fe5eb8d32a..dd42e0422af6 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1650,6 +1650,8 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>  static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  				struct intel_crtc_state *pipe_config)
>  {
> +	struct drm_i915_private *dev_priv = to_i915(intel_dp_to_dev(intel_dp));
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	int bpp, bpc;
>  
>  	bpp = pipe_config->pipe_bpp;
> @@ -1665,6 +1667,17 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
>  			      pipe_config->pipe_bpp);
>  	}
> +
> +	if (intel_dp_is_edp(intel_dp)) {
> +		/* Get bpp from vbt only for panels that dont have bpp in edid */
> +		if (intel_connector->base.display_info.bpc == 0 &&
> +			(dev_priv->vbt.edp.bpp && dev_priv->vbt.edp.bpp < bpp)) {
> +			DRM_DEBUG_KMS("clamping bpp for eDP panel to BIOS-provided %i\n",
> +				      dev_priv->vbt.edp.bpp);
> +			bpp = dev_priv->vbt.edp.bpp;
> +		}
> +	}
> +
>  	return bpp;
>  }
>  
> @@ -1689,10 +1702,8 @@ static bool
>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config)
>  {
> -	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	struct intel_connector *intel_connector = intel_dp->attached_connector;
>  	int lane_count, clock;
>  	int min_lane_count = 1;
>  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> @@ -1735,15 +1746,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	 * bpc in between. */
>  	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  	if (intel_dp_is_edp(intel_dp)) {
> -
> -		/* Get bpp from vbt only for panels that dont have bpp in edid */
> -		if (intel_connector->base.display_info.bpc == 0 &&
> -			(dev_priv->vbt.edp.bpp && dev_priv->vbt.edp.bpp < bpp)) {
> -			DRM_DEBUG_KMS("clamping bpp for eDP panel to BIOS-provided %i\n",
> -				      dev_priv->vbt.edp.bpp);
> -			bpp = dev_priv->vbt.edp.bpp;
> -		}
> -
>  		/*
>  		 * Use the maximum clock and number of lanes the eDP panel
>  		 * advertizes being capable of. The panels are generally
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/dp: abstract link config selection

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:39:04PM +0300, Jani Nikula wrote:
> For now, there's just the one link config selection, optimizing for slow
> and wide link. No functional changes.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 81 +++++++++++++++++++++++++----------------
>  1 file changed, 50 insertions(+), 31 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 3c5fbdf42b9b..c98626b3af65 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1704,6 +1704,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
>  	return bres;
>  }
>  
> +/* Optimize link config in order: max bpp, min clock, min lanes */
> +static bool
> +intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *pipe_config,
> +				  const struct link_config_limits *limits)
> +{
> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	int bpp, clock, lane_count;
> +	int mode_rate, link_clock, link_avail;
> +
> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> +						   bpp);
> +
> +		for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> +			for (lane_count = limits->min_lane_count;
> +			     lane_count <= limits->max_lane_count;
> +			     lane_count <<= 1) {
> +				link_clock = intel_dp->common_rates[clock];
> +				link_avail = intel_dp_max_data_rate(link_clock,
> +								    lane_count);
> +
> +				if (mode_rate <= link_avail) {
> +					pipe_config->lane_count = lane_count;
> +					pipe_config->pipe_bpp = bpp;
> +					pipe_config->port_clock = link_clock;
> +
> +					return true;
> +				}
> +			}
> +		}
> +	}
> +
> +	return false;
> +}
> +
>  static bool
>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>  			     struct intel_crtc_state *pipe_config)
> @@ -1711,8 +1747,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>  	struct link_config_limits limits;
> -	int bpp, clock, lane_count;
> -	int mode_rate, link_avail, link_clock;
>  	int common_len;
>  
>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> @@ -1766,37 +1800,22 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		      intel_dp->common_rates[limits.max_clock],
>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>  
> -	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
> -		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> -						   bpp);
> -
> -		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
> -			for (lane_count = limits.min_lane_count;
> -			     lane_count <= limits.max_lane_count;
> -			     lane_count <<= 1) {
> -
> -				link_clock = intel_dp->common_rates[clock];
> -				link_avail = intel_dp_max_data_rate(link_clock,
> -								    lane_count);
> -
> -				if (mode_rate <= link_avail) {
> -					goto found;
> -				}
> -			}
> -		}
> -	}
> -
> -	return false;
> -
> -found:
> -	pipe_config->lane_count = lane_count;
> -	pipe_config->pipe_bpp = bpp;
> -	pipe_config->port_clock = intel_dp->common_rates[clock];
> +	/*
> +	 * Optimize for slow and wide. This is the place to add alternative
> +	 * optimization policy.
> +	 */
> +	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> +		return false;
>  
>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> -		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> -	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> -		      mode_rate, link_avail);
> +		      pipe_config->lane_count, pipe_config->port_clock,
> +		      pipe_config->pipe_bpp);
> +
> +	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> +		      intel_dp_link_required(adjusted_mode->crtc_clock,
> +					     pipe_config->pipe_bpp),
> +		      intel_dp_max_data_rate(pipe_config->port_clock,
> +					     pipe_config->lane_count));

Wouldnt it be better if we move this Debug message about Available and
required link rate in the other function right after the condition
if (mode_rate <= link_avail) is true, before returning true from that function.
I think it will be just more intuitive there.

Everything else looks good. So

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

>  
>  	return true;
>  }
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 7/7] drm/i915/dp: fix compliance test adjustments

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:39:05PM +0300, Jani Nikula wrote:
> Abstract compliance test adjustments to a single function. Also make the
> bpc adjustments affect the limits, actually forcing the bpc. Seems like
> directly changing the pipe_bpp in the past could not have been
> effective.
> 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>

Looks good to me w.r.t compliance test request parameters.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

> ---
>  drivers/gpu/drm/i915/intel_dp.c | 64 ++++++++++++++++++++++++-----------------
>  1 file changed, 38 insertions(+), 26 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c98626b3af65..4ddb9dc61f46 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1666,14 +1666,6 @@ static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  	if (bpc > 0)
>  		bpp = min(bpp, 3*bpc);
>  
> -	/* For DP Compliance we override the computed bpp for the pipe */
> -	if (intel_dp->compliance.test_data.bpc != 0) {
> -		pipe_config->pipe_bpp =	3*intel_dp->compliance.test_data.bpc;
> -		pipe_config->dither_force_disable = pipe_config->pipe_bpp == 6*3;
> -		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n",
> -			      pipe_config->pipe_bpp);
> -	}
> -
>  	if (intel_dp_is_edp(intel_dp)) {
>  		/* Get bpp from vbt only for panels that dont have bpp in edid */
>  		if (intel_connector->base.display_info.bpc == 0 &&
> @@ -1704,6 +1696,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
>  	return bres;
>  }
>  
> +/* Adjust link config limits based on compliance test requests. */
> +static void
> +intel_dp_adjust_compliance_config(struct intel_dp *intel_dp,
> +				  struct intel_crtc_state *pipe_config,
> +				  struct link_config_limits *limits)
> +{
> +	/* For DP Compliance we override the computed bpp for the pipe */
> +	if (intel_dp->compliance.test_data.bpc != 0) {
> +		int bpp = 3 * intel_dp->compliance.test_data.bpc;
> +
> +		limits->min_bpp = limits->max_bpp = bpp;
> +		pipe_config->dither_force_disable = bpp == 6 * 3;
> +
> +		DRM_DEBUG_KMS("Setting pipe_bpp to %d\n", bpp);
> +	}
> +
> +	/* Use values requested by Compliance Test Request */
> +	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> +		int index;
> +
> +		/* Validate the compliance test data since max values
> +		 * might have changed due to link train fallback.
> +		 */
> +		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
> +					       intel_dp->compliance.test_lane_count)) {
> +			index = intel_dp_rate_index(intel_dp->common_rates,
> +						    intel_dp->num_common_rates,
> +						    intel_dp->compliance.test_link_rate);
> +			if (index >= 0)
> +				limits->min_clock = limits->max_clock = index;
> +			limits->min_lane_count = limits->max_lane_count =
> +				intel_dp->compliance.test_lane_count;
> +		}
> +	}
> +}
> +
>  /* Optimize link config in order: max bpp, min clock, min lanes */
>  static bool
>  intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> @@ -1764,24 +1792,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  	limits.min_bpp = 6 * 3;
>  	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  
> -	/* Use values requested by Compliance Test Request */
> -	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> -		int index;
> -
> -		/* Validate the compliance test data since max values
> -		 * might have changed due to link train fallback.
> -		 */
> -		if (intel_dp_link_params_valid(intel_dp, intel_dp->compliance.test_link_rate,
> -					       intel_dp->compliance.test_lane_count)) {
> -			index = intel_dp_rate_index(intel_dp->common_rates,
> -						    intel_dp->num_common_rates,
> -						    intel_dp->compliance.test_link_rate);
> -			if (index >= 0)
> -				limits.min_clock = limits.max_clock = index;
> -			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;
> -		}
> -	}
> -
>  	if (intel_dp_is_edp(intel_dp)) {
>  		/*
>  		 * Use the maximum clock and number of lanes the eDP panel
> @@ -1794,6 +1804,8 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		limits.min_clock = limits.max_clock;
>  	}
>  
> +	intel_dp_adjust_compliance_config(intel_dp, pipe_config, &limits);
> +
>  	DRM_DEBUG_KMS("DP link computation with max lane count %i "
>  		      "max rate %d max bpp %d pixel clock %iKHz\n",
>  		      limits.max_lane_count,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/dp: abstract link config selection

[Jani Nikula]

On Thu, 05 Apr 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> On Thu, Apr 05, 2018 at 05:39:04PM +0300, Jani Nikula wrote:
>> For now, there's just the one link config selection, optimizing for slow
>> and wide link. No functional changes.
>> 
>> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 81 +++++++++++++++++++++++++----------------
>>  1 file changed, 50 insertions(+), 31 deletions(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
>> index 3c5fbdf42b9b..c98626b3af65 100644
>> --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -1704,6 +1704,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
>>  	return bres;
>>  }
>>  
>> +/* Optimize link config in order: max bpp, min clock, min lanes */
>> +static bool
>> +intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
>> +				  struct intel_crtc_state *pipe_config,
>> +				  const struct link_config_limits *limits)
>> +{
>> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>> +	int bpp, clock, lane_count;
>> +	int mode_rate, link_clock, link_avail;
>> +
>> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
>> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> +						   bpp);
>> +
>> +		for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
>> +			for (lane_count = limits->min_lane_count;
>> +			     lane_count <= limits->max_lane_count;
>> +			     lane_count <<= 1) {
>> +				link_clock = intel_dp->common_rates[clock];
>> +				link_avail = intel_dp_max_data_rate(link_clock,
>> +								    lane_count);
>> +
>> +				if (mode_rate <= link_avail) {
>> +					pipe_config->lane_count = lane_count;
>> +					pipe_config->pipe_bpp = bpp;
>> +					pipe_config->port_clock = link_clock;
>> +
>> +					return true;
>> +				}
>> +			}
>> +		}
>> +	}
>> +
>> +	return false;
>> +}
>> +
>>  static bool
>>  intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  			     struct intel_crtc_state *pipe_config)
>> @@ -1711,8 +1747,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
>>  	struct link_config_limits limits;
>> -	int bpp, clock, lane_count;
>> -	int mode_rate, link_avail, link_clock;
>>  	int common_len;
>>  
>>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
>> @@ -1766,37 +1800,22 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>>  		      intel_dp->common_rates[limits.max_clock],
>>  		      limits.max_bpp, adjusted_mode->crtc_clock);
>>  
>> -	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
>> -		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>> -						   bpp);
>> -
>> -		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
>> -			for (lane_count = limits.min_lane_count;
>> -			     lane_count <= limits.max_lane_count;
>> -			     lane_count <<= 1) {
>> -
>> -				link_clock = intel_dp->common_rates[clock];
>> -				link_avail = intel_dp_max_data_rate(link_clock,
>> -								    lane_count);
>> -
>> -				if (mode_rate <= link_avail) {
>> -					goto found;
>> -				}
>> -			}
>> -		}
>> -	}
>> -
>> -	return false;
>> -
>> -found:
>> -	pipe_config->lane_count = lane_count;
>> -	pipe_config->pipe_bpp = bpp;
>> -	pipe_config->port_clock = intel_dp->common_rates[clock];
>> +	/*
>> +	 * Optimize for slow and wide. This is the place to add alternative
>> +	 * optimization policy.
>> +	 */
>> +	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
>> +		return false;
>>  
>>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
>> -		      pipe_config->lane_count, pipe_config->port_clock, bpp);
>> -	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
>> -		      mode_rate, link_avail);
>> +		      pipe_config->lane_count, pipe_config->port_clock,
>> +		      pipe_config->pipe_bpp);
>> +
>> +	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
>> +		      intel_dp_link_required(adjusted_mode->crtc_clock,
>> +					     pipe_config->pipe_bpp),
>> +		      intel_dp_max_data_rate(pipe_config->port_clock,
>> +					     pipe_config->lane_count));
>
> Wouldnt it be better if we move this Debug message about Available and
> required link rate in the other function right after the condition
> if (mode_rate <= link_avail) is true, before returning true from that function.
> I think it will be just more intuitive there.

That's what I thought too at first, but then realized if we're going to
add an alternative call to an alternative approach, and perhaps yet
another for DSC, the debugging gets duplicated in all of them.

BR,
Jani.


>
> Everything else looks good. So
>
> Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
>
> Manasi
>
>>  
>>  	return true;
>>  }
>> -- 
>> 2.11.0
>> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

Re: [PATCH 6/7] drm/i915/dp: abstract link config selection

[Manasi Navare]

On Mon, Apr 09, 2018 at 05:12:03PM +0300, Jani Nikula wrote:
> On Thu, 05 Apr 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Thu, Apr 05, 2018 at 05:39:04PM +0300, Jani Nikula wrote:
> >> For now, there's just the one link config selection, optimizing for slow
> >> and wide link. No functional changes.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 81 +++++++++++++++++++++++++----------------
> >>  1 file changed, 50 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 3c5fbdf42b9b..c98626b3af65 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1704,6 +1704,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> >>  	return bres;
> >>  }
> >>  
> >> +/* Optimize link config in order: max bpp, min clock, min lanes */
> >> +static bool
> >> +intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >> +				  struct intel_crtc_state *pipe_config,
> >> +				  const struct link_config_limits *limits)
> >> +{
> >> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >> +	int bpp, clock, lane_count;
> >> +	int mode_rate, link_clock, link_avail;
> >> +
> >> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> >> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> +						   bpp);
> >> +
> >> +		for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> >> +			for (lane_count = limits->min_lane_count;
> >> +			     lane_count <= limits->max_lane_count;
> >> +			     lane_count <<= 1) {
> >> +				link_clock = intel_dp->common_rates[clock];
> >> +				link_avail = intel_dp_max_data_rate(link_clock,
> >> +								    lane_count);
> >> +
> >> +				if (mode_rate <= link_avail) {
> >> +					pipe_config->lane_count = lane_count;
> >> +					pipe_config->pipe_bpp = bpp;
> >> +					pipe_config->port_clock = link_clock;
> >> +
> >> +					return true;
> >> +				}
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >>  static bool
> >>  intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  			     struct intel_crtc_state *pipe_config)
> >> @@ -1711,8 +1747,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >>  	struct link_config_limits limits;
> >> -	int bpp, clock, lane_count;
> >> -	int mode_rate, link_avail, link_clock;
> >>  	int common_len;
> >>  
> >>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> >> @@ -1766,37 +1800,22 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  		      intel_dp->common_rates[limits.max_clock],
> >>  		      limits.max_bpp, adjusted_mode->crtc_clock);
> >>  
> >> -	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
> >> -		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> -						   bpp);
> >> -
> >> -		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
> >> -			for (lane_count = limits.min_lane_count;
> >> -			     lane_count <= limits.max_lane_count;
> >> -			     lane_count <<= 1) {
> >> -
> >> -				link_clock = intel_dp->common_rates[clock];
> >> -				link_avail = intel_dp_max_data_rate(link_clock,
> >> -								    lane_count);
> >> -
> >> -				if (mode_rate <= link_avail) {
> >> -					goto found;
> >> -				}
> >> -			}
> >> -		}
> >> -	}
> >> -
> >> -	return false;
> >> -
> >> -found:
> >> -	pipe_config->lane_count = lane_count;
> >> -	pipe_config->pipe_bpp = bpp;
> >> -	pipe_config->port_clock = intel_dp->common_rates[clock];
> >> +	/*
> >> +	 * Optimize for slow and wide. This is the place to add alternative
> >> +	 * optimization policy.
> >> +	 */
> >> +	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> >> +		return false;
> >>  
> >>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> >> -		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> >> -	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> >> -		      mode_rate, link_avail);
> >> +		      pipe_config->lane_count, pipe_config->port_clock,
> >> +		      pipe_config->pipe_bpp);
> >> +
> >> +	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> >> +		      intel_dp_link_required(adjusted_mode->crtc_clock,
> >> +					     pipe_config->pipe_bpp),
> >> +		      intel_dp_max_data_rate(pipe_config->port_clock,
> >> +					     pipe_config->lane_count));
> >
> > Wouldnt it be better if we move this Debug message about Available and
> > required link rate in the other function right after the condition
> > if (mode_rate <= link_avail) is true, before returning true from that function.
> > I think it will be just more intuitive there.
> 
> That's what I thought too at first, but then realized if we're going to
> add an alternative call to an alternative approach, and perhaps yet
> another for DSC, the debugging gets duplicated in all of them.
> 
> BR,
> Jani.
>

Hmm yea that would make sense. So for DSC, I do have a seprate intel_dp_dsc_compute_config()
should that be called from within the link_config function before returning to the intel_dp_compute_config()

Manasi
 
> 
> >
> > Everything else looks good. So
> >
> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> >
> > Manasi
> >
> >>  
> >>  	return true;
> >>  }
> >> -- 
> >> 2.11.0
> >> 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 3/7] drm/i915/dp: abstract dp link config computation from the rest

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:39:01PM +0300, Jani Nikula wrote:
> Abstract a new intel_dp_compute_link_config() from
> intel_dp_compute_config(), with the parts related to link configuration,
> i.e. bpp, link rate, and lane count selection. No functional changes.
>

This abstraction makes it cleaner and easy to add dsc compute config.

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
 
> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 160 ++++++++++++++++++++++------------------
>  1 file changed, 87 insertions(+), 73 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 81cf363e71af..19fe5eb8d32a 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1685,19 +1685,14 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
>  	return bres;
>  }
>  
> -bool
> -intel_dp_compute_config(struct intel_encoder *encoder,
> -			struct intel_crtc_state *pipe_config,
> -			struct drm_connector_state *conn_state)
> +static bool
> +intel_dp_compute_link_config(struct intel_encoder *encoder,
> +			     struct intel_crtc_state *pipe_config)
>  {
>  	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	enum port port = encoder->port;
> -	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
>  	struct intel_connector *intel_connector = intel_dp->attached_connector;
> -	struct intel_digital_connector_state *intel_conn_state =
> -		to_intel_digital_connector_state(conn_state);
>  	int lane_count, clock;
>  	int min_lane_count = 1;
>  	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> @@ -1706,9 +1701,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	int bpp, mode_rate;
>  	int link_avail, link_clock;
>  	int common_len;
> -	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
> -					   DP_DPCD_QUIRK_LIMITED_M_N);
> -
>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
>  						    intel_dp->max_link_rate);
>  
> @@ -1717,51 +1709,6 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  
>  	max_clock = common_len - 1;
>  
> -	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> -		pipe_config->has_pch_encoder = true;
> -
> -	pipe_config->has_drrs = false;
> -	if (IS_G4X(dev_priv) || port == PORT_A)
> -		pipe_config->has_audio = false;
> -	else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
> -		pipe_config->has_audio = intel_dp->has_audio;
> -	else
> -		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
> -
> -	if (intel_dp_is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> -		struct drm_display_mode *panel_mode =
> -			intel_connector->panel.alt_fixed_mode;
> -		struct drm_display_mode *req_mode = &pipe_config->base.mode;
> -
> -		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> -			panel_mode = intel_connector->panel.fixed_mode;
> -
> -		drm_mode_debug_printmodeline(panel_mode);
> -
> -		intel_fixed_panel_mode(panel_mode, adjusted_mode);
> -
> -		if (INTEL_GEN(dev_priv) >= 9) {
> -			int ret;
> -			ret = skl_update_scaler_crtc(pipe_config);
> -			if (ret)
> -				return ret;
> -		}
> -
> -		if (HAS_GMCH_DISPLAY(dev_priv))
> -			intel_gmch_panel_fitting(intel_crtc, pipe_config,
> -						 conn_state->scaling_mode);
> -		else
> -			intel_pch_panel_fitting(intel_crtc, pipe_config,
> -						conn_state->scaling_mode);
> -	}
> -
> -	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> -	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> -		return false;
> -
> -	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> -		return false;
> -
>  	/* Use values requested by Compliance Test Request */
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
>  		int index;
> @@ -1831,6 +1778,82 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	return false;
>  
>  found:
> +	pipe_config->lane_count = lane_count;
> +	pipe_config->pipe_bpp = bpp;
> +	pipe_config->port_clock = intel_dp->common_rates[clock];
> +
> +	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> +		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> +	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> +		      mode_rate, link_avail);
> +
> +	return true;
> +}
> +
> +bool
> +intel_dp_compute_config(struct intel_encoder *encoder,
> +			struct intel_crtc_state *pipe_config,
> +			struct drm_connector_state *conn_state)
> +{
> +	struct drm_i915_private *dev_priv = to_i915(encoder->base.dev);
> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> +	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> +	enum port port = encoder->port;
> +	struct intel_crtc *intel_crtc = to_intel_crtc(pipe_config->base.crtc);
> +	struct intel_connector *intel_connector = intel_dp->attached_connector;
> +	struct intel_digital_connector_state *intel_conn_state =
> +		to_intel_digital_connector_state(conn_state);
> +	bool reduce_m_n = drm_dp_has_quirk(&intel_dp->desc,
> +					   DP_DPCD_QUIRK_LIMITED_M_N);
> +
> +	if (HAS_PCH_SPLIT(dev_priv) && !HAS_DDI(dev_priv) && port != PORT_A)
> +		pipe_config->has_pch_encoder = true;
> +
> +	pipe_config->has_drrs = false;
> +	if (IS_G4X(dev_priv) || port == PORT_A)
> +		pipe_config->has_audio = false;
> +	else if (intel_conn_state->force_audio == HDMI_AUDIO_AUTO)
> +		pipe_config->has_audio = intel_dp->has_audio;
> +	else
> +		pipe_config->has_audio = intel_conn_state->force_audio == HDMI_AUDIO_ON;
> +
> +	if (intel_dp_is_edp(intel_dp) && intel_connector->panel.fixed_mode) {
> +		struct drm_display_mode *panel_mode =
> +			intel_connector->panel.alt_fixed_mode;
> +		struct drm_display_mode *req_mode = &pipe_config->base.mode;
> +
> +		if (!intel_edp_compare_alt_mode(req_mode, panel_mode))
> +			panel_mode = intel_connector->panel.fixed_mode;
> +
> +		drm_mode_debug_printmodeline(panel_mode);
> +
> +		intel_fixed_panel_mode(panel_mode, adjusted_mode);
> +
> +		if (INTEL_GEN(dev_priv) >= 9) {
> +			int ret;
> +			ret = skl_update_scaler_crtc(pipe_config);
> +			if (ret)
> +				return ret;
> +		}
> +
> +		if (HAS_GMCH_DISPLAY(dev_priv))
> +			intel_gmch_panel_fitting(intel_crtc, pipe_config,
> +						 conn_state->scaling_mode);
> +		else
> +			intel_pch_panel_fitting(intel_crtc, pipe_config,
> +						conn_state->scaling_mode);
> +	}
> +
> +	if ((IS_VALLEYVIEW(dev_priv) || IS_CHERRYVIEW(dev_priv)) &&
> +	    adjusted_mode->flags & DRM_MODE_FLAG_INTERLACE)
> +		return false;
> +
> +	if (adjusted_mode->flags & DRM_MODE_FLAG_DBLCLK)
> +		return false;
> +
> +	if (!intel_dp_compute_link_config(encoder, pipe_config))
> +		return false;
> +
>  	if (intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_AUTO) {
>  		/*
>  		 * See:
> @@ -1838,7 +1861,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  		 * VESA DisplayPort Ver.1.2a - 5.1.1.1 Video Colorimetry
>  		 */
>  		pipe_config->limited_color_range =
> -			bpp != 18 &&
> +			pipe_config->pipe_bpp != 18 &&
>  			drm_default_rgb_quant_range(adjusted_mode) ==
>  			HDMI_QUANTIZATION_RANGE_LIMITED;
>  	} else {
> @@ -1846,17 +1869,7 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  			intel_conn_state->broadcast_rgb == INTEL_BROADCAST_RGB_LIMITED;
>  	}
>  
> -	pipe_config->lane_count = lane_count;
> -
> -	pipe_config->pipe_bpp = bpp;
> -	pipe_config->port_clock = intel_dp->common_rates[clock];
> -
> -	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> -		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> -	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> -		      mode_rate, link_avail);
> -
> -	intel_link_compute_m_n(bpp, lane_count,
> +	intel_link_compute_m_n(pipe_config->pipe_bpp, pipe_config->lane_count,
>  			       adjusted_mode->crtc_clock,
>  			       pipe_config->port_clock,
>  			       &pipe_config->dp_m_n,
> @@ -1865,11 +1878,12 @@ intel_dp_compute_config(struct intel_encoder *encoder,
>  	if (intel_connector->panel.downclock_mode != NULL &&
>  		dev_priv->drrs.type == SEAMLESS_DRRS_SUPPORT) {
>  			pipe_config->has_drrs = true;
> -			intel_link_compute_m_n(bpp, lane_count,
> -				intel_connector->panel.downclock_mode->clock,
> -				pipe_config->port_clock,
> -				&pipe_config->dp_m2_n2,
> -				reduce_m_n);
> +			intel_link_compute_m_n(pipe_config->pipe_bpp,
> +					       pipe_config->lane_count,
> +					       intel_connector->panel.downclock_mode->clock,
> +					       pipe_config->port_clock,
> +					       &pipe_config->dp_m2_n2,
> +					       reduce_m_n);
>  	}
>  
>  	/*
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 5/7] drm/i915/dp: group link config limits in a struct

[Manasi Navare]

On Thu, Apr 05, 2018 at 05:39:03PM +0300, Jani Nikula wrote:
> Also use same min/max model for bpp, and adjust debug logging while at
> it.
> 

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 57 ++++++++++++++++++++++++-----------------
>  1 file changed, 33 insertions(+), 24 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index dd42e0422af6..3c5fbdf42b9b 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1647,6 +1647,12 @@ void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
>  	}
>  }
>  
> +struct link_config_limits {
> +	int min_clock, max_clock;
> +	int min_lane_count, max_lane_count;
> +	int min_bpp, max_bpp;
> +};
> +
>  static int intel_dp_compute_bpp(struct intel_dp *intel_dp,
>  				struct intel_crtc_state *pipe_config)
>  {
> @@ -1704,21 +1710,25 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  {
>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> -	int lane_count, clock;
> -	int min_lane_count = 1;
> -	int max_lane_count = intel_dp_max_lane_count(intel_dp);
> -	int min_clock = 0;
> -	int max_clock;
> -	int bpp, mode_rate;
> -	int link_avail, link_clock;
> +	struct link_config_limits limits;
> +	int bpp, clock, lane_count;
> +	int mode_rate, link_avail, link_clock;
>  	int common_len;
> +
>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
>  						    intel_dp->max_link_rate);
>  
>  	/* No common link rates between source and sink */
>  	WARN_ON(common_len <= 0);
>  
> -	max_clock = common_len - 1;
> +	limits.min_clock = 0;
> +	limits.max_clock = common_len - 1;
> +
> +	limits.min_lane_count = 1;
> +	limits.max_lane_count = intel_dp_max_lane_count(intel_dp);
> +
> +	limits.min_bpp = 6 * 3;
> +	limits.max_bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  
>  	/* Use values requested by Compliance Test Request */
>  	if (intel_dp->compliance.test_type == DP_TEST_LINK_TRAINING) {
> @@ -1733,18 +1743,11 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  						    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;
> +				limits.min_clock = limits.max_clock = index;
> +			limits.min_lane_count = limits.max_lane_count = intel_dp->compliance.test_lane_count;
>  		}
>  	}
> -	DRM_DEBUG_KMS("DP link computation with max lane count %i "
> -		      "max bw %d pixel clock %iKHz\n",
> -		      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
> -	 * bpc in between. */
> -	bpp = intel_dp_compute_bpp(intel_dp, pipe_config);
>  	if (intel_dp_is_edp(intel_dp)) {
>  		/*
>  		 * Use the maximum clock and number of lanes the eDP panel
> @@ -1753,18 +1756,24 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
>  		 * configuration, and typically these values correspond to the
>  		 * native resolution of the panel.
>  		 */
> -		min_lane_count = max_lane_count;
> -		min_clock = max_clock;
> +		limits.min_lane_count = limits.max_lane_count;
> +		limits.min_clock = limits.max_clock;
>  	}
>  
> -	for (; bpp >= 6*3; bpp -= 2*3) {
> +	DRM_DEBUG_KMS("DP link computation with max lane count %i "
> +		      "max rate %d max bpp %d pixel clock %iKHz\n",
> +		      limits.max_lane_count,
> +		      intel_dp->common_rates[limits.max_clock],
> +		      limits.max_bpp, adjusted_mode->crtc_clock);
> +
> +	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
>  		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
>  						   bpp);
>  
> -		for (clock = min_clock; clock <= max_clock; clock++) {
> -			for (lane_count = min_lane_count;
> -				lane_count <= max_lane_count;
> -				lane_count <<= 1) {
> +		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
> +			for (lane_count = limits.min_lane_count;
> +			     lane_count <= limits.max_lane_count;
> +			     lane_count <<= 1) {
>  
>  				link_clock = intel_dp->common_rates[clock];
>  				link_avail = intel_dp_max_data_rate(link_clock,
> -- 
> 2.11.0
> 
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 6/7] drm/i915/dp: abstract link config selection

[Manasi Navare]

On Mon, Apr 09, 2018 at 05:12:03PM +0300, Jani Nikula wrote:
> On Thu, 05 Apr 2018, Manasi Navare <manasi.d.navare@intel.com> wrote:
> > On Thu, Apr 05, 2018 at 05:39:04PM +0300, Jani Nikula wrote:
> >> For now, there's just the one link config selection, optimizing for slow
> >> and wide link. No functional changes.
> >> 
> >> Signed-off-by: Jani Nikula <jani.nikula@intel.com>
> >> ---
> >>  drivers/gpu/drm/i915/intel_dp.c | 81 +++++++++++++++++++++++++----------------
> >>  1 file changed, 50 insertions(+), 31 deletions(-)
> >> 
> >> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> >> index 3c5fbdf42b9b..c98626b3af65 100644
> >> --- a/drivers/gpu/drm/i915/intel_dp.c
> >> +++ b/drivers/gpu/drm/i915/intel_dp.c
> >> @@ -1704,6 +1704,42 @@ static bool intel_edp_compare_alt_mode(struct drm_display_mode *m1,
> >>  	return bres;
> >>  }
> >>  
> >> +/* Optimize link config in order: max bpp, min clock, min lanes */
> >> +static bool
> >> +intel_dp_compute_link_config_wide(struct intel_dp *intel_dp,
> >> +				  struct intel_crtc_state *pipe_config,
> >> +				  const struct link_config_limits *limits)
> >> +{
> >> +	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >> +	int bpp, clock, lane_count;
> >> +	int mode_rate, link_clock, link_avail;
> >> +
> >> +	for (bpp = limits->max_bpp; bpp >= limits->min_bpp; bpp -= 2 * 3) {
> >> +		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> +						   bpp);
> >> +
> >> +		for (clock = limits->min_clock; clock <= limits->max_clock; clock++) {
> >> +			for (lane_count = limits->min_lane_count;
> >> +			     lane_count <= limits->max_lane_count;
> >> +			     lane_count <<= 1) {
> >> +				link_clock = intel_dp->common_rates[clock];
> >> +				link_avail = intel_dp_max_data_rate(link_clock,
> >> +								    lane_count);
> >> +
> >> +				if (mode_rate <= link_avail) {
> >> +					pipe_config->lane_count = lane_count;
> >> +					pipe_config->pipe_bpp = bpp;
> >> +					pipe_config->port_clock = link_clock;
> >> +
> >> +					return true;
> >> +				}
> >> +			}
> >> +		}
> >> +	}
> >> +
> >> +	return false;
> >> +}
> >> +
> >>  static bool
> >>  intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  			     struct intel_crtc_state *pipe_config)
> >> @@ -1711,8 +1747,6 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  	struct drm_display_mode *adjusted_mode = &pipe_config->base.adjusted_mode;
> >>  	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
> >>  	struct link_config_limits limits;
> >> -	int bpp, clock, lane_count;
> >> -	int mode_rate, link_avail, link_clock;
> >>  	int common_len;
> >>  
> >>  	common_len = intel_dp_common_len_rate_limit(intel_dp,
> >> @@ -1766,37 +1800,22 @@ intel_dp_compute_link_config(struct intel_encoder *encoder,
> >>  		      intel_dp->common_rates[limits.max_clock],
> >>  		      limits.max_bpp, adjusted_mode->crtc_clock);
> >>  
> >> -	for (bpp = limits.max_bpp; bpp >= limits.min_bpp; bpp -= 2 * 3) {
> >> -		mode_rate = intel_dp_link_required(adjusted_mode->crtc_clock,
> >> -						   bpp);
> >> -
> >> -		for (clock = limits.min_clock; clock <= limits.max_clock; clock++) {
> >> -			for (lane_count = limits.min_lane_count;
> >> -			     lane_count <= limits.max_lane_count;
> >> -			     lane_count <<= 1) {
> >> -
> >> -				link_clock = intel_dp->common_rates[clock];
> >> -				link_avail = intel_dp_max_data_rate(link_clock,
> >> -								    lane_count);
> >> -
> >> -				if (mode_rate <= link_avail) {
> >> -					goto found;
> >> -				}
> >> -			}
> >> -		}
> >> -	}
> >> -
> >> -	return false;
> >> -
> >> -found:
> >> -	pipe_config->lane_count = lane_count;
> >> -	pipe_config->pipe_bpp = bpp;
> >> -	pipe_config->port_clock = intel_dp->common_rates[clock];
> >> +	/*
> >> +	 * Optimize for slow and wide. This is the place to add alternative
> >> +	 * optimization policy.
> >> +	 */
> >> +	if (!intel_dp_compute_link_config_wide(intel_dp, pipe_config, &limits))
> >> +		return false;
> >>  
> >>  	DRM_DEBUG_KMS("DP lane count %d clock %d bpp %d\n",
> >> -		      pipe_config->lane_count, pipe_config->port_clock, bpp);
> >> -	DRM_DEBUG_KMS("DP link bw required %i available %i\n",
> >> -		      mode_rate, link_avail);
> >> +		      pipe_config->lane_count, pipe_config->port_clock,
> >> +		      pipe_config->pipe_bpp);
> >> +
> >> +	DRM_DEBUG_KMS("DP link rate required %i available %i\n",
> >> +		      intel_dp_link_required(adjusted_mode->crtc_clock,
> >> +					     pipe_config->pipe_bpp),
> >> +		      intel_dp_max_data_rate(pipe_config->port_clock,
> >> +					     pipe_config->lane_count));
> >
> > Wouldnt it be better if we move this Debug message about Available and
> > required link rate in the other function right after the condition
> > if (mode_rate <= link_avail) is true, before returning true from that function.
> > I think it will be just more intuitive there.
> 
> That's what I thought too at first, but then realized if we're going to
> add an alternative call to an alternative approach, and perhaps yet
> another for DSC, the debugging gets duplicated in all of them.
> 
> BR,
> Jani.

Yes that makes sense then to have the debug messages outside in the main compute_config function.
Thanks for the clarification. So with that:

Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>

Manasi

> 
> 
> >
> > Everything else looks good. So
> >
> > Reviewed-by: Manasi Navare <manasi.d.navare@intel.com>
> >
> > Manasi
> >
> >>  
> >>  	return true;
> >>  }
> >> -- 
> >> 2.11.0
> >> 
> > _______________________________________________
> > Intel-gfx mailing list
> > Intel-gfx@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/intel-gfx
> 
> -- 
> Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/intel-gfx