diff mbox

[v2] drm/i915: Put back lane_count into intel_dp and add link_rate too

Message ID 1439823912-1813-1-git-send-email-ville.syrjala@linux.intel.com (mailing list archive)
State New, archived
Headers show

Commit Message

Ville Syrjälä Aug. 17, 2015, 3:05 p.m. UTC
From: Ville Syrjälä <ville.syrjala@linux.intel.com>

With MST there won't be a crtc assigned to the main link encoder, so
trying to dig up the pipe_config from there is a recipe for an oops.

Instead store the parameters (lane_count and link_rate) in the encoder,
and use those values during link training etc. Since those parameters
are now assigned only when the link is actually enabled,
.compute_config() won't clobber them as it did before.

Hardware state readout is still bonkers though as we don't transfer the
link parameters from pipe_config intel_dp. We should do that during
encoder sanitation. But since we don't even do a proper job of reading
out the main link encoder state for MST there's littel point in
worrying about this now.

Fixes a regression with MST caused by:
 commit 90a6b7b052b1aa17fbb98b049e9c8b7f729c35a7
 Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
 Date:   Mon Jul 6 16:39:15 2015 +0300

    drm/i915: Move intel_dp->lane_count into pipe_config

v2: Different apporoach that should keep intel_dp_check_mst_status()
    somewhat less oopsy

Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    |  5 ++--
 drivers/gpu/drm/i915/intel_dp.c     | 53 +++++++++++++++++--------------------
 drivers/gpu/drm/i915/intel_dp_mst.c |  2 ++
 drivers/gpu/drm/i915/intel_drv.h    |  4 +++
 4 files changed, 33 insertions(+), 31 deletions(-)

Comments

Maarten Lankhorst Aug. 18, 2015, 11:56 a.m. UTC | #1
Hey,

Op 17-08-15 om 17:05 schreef ville.syrjala@linux.intel.com:
> From: Ville Syrjälä <ville.syrjala@linux.intel.com>
>
> With MST there won't be a crtc assigned to the main link encoder, so
> trying to dig up the pipe_config from there is a recipe for an oops.
>
> Instead store the parameters (lane_count and link_rate) in the encoder,
> and use those values during link training etc. Since those parameters
> are now assigned only when the link is actually enabled,
> .compute_config() won't clobber them as it did before.
>
> Hardware state readout is still bonkers though as we don't transfer the
> link parameters from pipe_config intel_dp. We should do that during
> encoder sanitation. But since we don't even do a proper job of reading
> out the main link encoder state for MST there's littel point in
> worrying about this now.
>
> Fixes a regression with MST caused by:
>  commit 90a6b7b052b1aa17fbb98b049e9c8b7f729c35a7
>  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
>  Date:   Mon Jul 6 16:39:15 2015 +0300
>
>     drm/i915: Move intel_dp->lane_count into pipe_config
>
> v2: Different apporoach that should keep intel_dp_check_mst_status()
>     somewhat less oopsy
>
> Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> ---
Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>

Thanks, that seems to make MST work as expected again.
Daniel Vetter Aug. 26, 2015, 7:58 a.m. UTC | #2
On Tue, Aug 18, 2015 at 01:56:08PM +0200, Maarten Lankhorst wrote:
> Hey,
> 
> Op 17-08-15 om 17:05 schreef ville.syrjala@linux.intel.com:
> > From: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >
> > With MST there won't be a crtc assigned to the main link encoder, so
> > trying to dig up the pipe_config from there is a recipe for an oops.
> >
> > Instead store the parameters (lane_count and link_rate) in the encoder,
> > and use those values during link training etc. Since those parameters
> > are now assigned only when the link is actually enabled,
> > .compute_config() won't clobber them as it did before.
> >
> > Hardware state readout is still bonkers though as we don't transfer the
> > link parameters from pipe_config intel_dp. We should do that during
> > encoder sanitation. But since we don't even do a proper job of reading
> > out the main link encoder state for MST there's littel point in
> > worrying about this now.
> >
> > Fixes a regression with MST caused by:
> >  commit 90a6b7b052b1aa17fbb98b049e9c8b7f729c35a7
> >  Author: Ville Syrjälä <ville.syrjala@linux.intel.com>
> >  Date:   Mon Jul 6 16:39:15 2015 +0300
> >
> >     drm/i915: Move intel_dp->lane_count into pipe_config
> >
> > v2: Different apporoach that should keep intel_dp_check_mst_status()
> >     somewhat less oopsy
> >
> > Cc: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Reported-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > Signed-off-by: Ville Syrjälä <ville.syrjala@linux.intel.com>
> > ---
> Tested-by: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> 
> Thanks, that seems to make MST work as expected again.

Queued for -next, thanks for the patch.
-Daniel
diff mbox

Patch

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 56d778f..5dff8b7 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -728,11 +728,10 @@  void intel_ddi_init_dp_buf_reg(struct intel_encoder *encoder)
 	struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base);
 	struct intel_digital_port *intel_dig_port =
 		enc_to_dig_port(&encoder->base);
-	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 
 	intel_dp->DP = intel_dig_port->saved_port_bits |
 		DDI_BUF_CTL_ENABLE | DDI_BUF_TRANS_SELECT(0);
-	intel_dp->DP |= DDI_PORT_WIDTH(crtc->config->lane_count);
+	intel_dp->DP |= DDI_PORT_WIDTH(intel_dp->lane_count);
 }
 
 static struct intel_encoder *
@@ -2314,6 +2313,8 @@  static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 	if (type == INTEL_OUTPUT_DISPLAYPORT || type == INTEL_OUTPUT_EDP) {
 		struct intel_dp *intel_dp = enc_to_intel_dp(encoder);
 
+		intel_dp_set_link_params(intel_dp, crtc->config);
+
 		intel_ddi_init_dp_buf_reg(intel_encoder);
 
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b905c19..dcda86a 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1584,6 +1584,13 @@  static void ironlake_set_pll_cpu_edp(struct intel_dp *intel_dp)
 	udelay(500);
 }
 
+void intel_dp_set_link_params(struct intel_dp *intel_dp,
+			      const struct intel_crtc_state *pipe_config)
+{
+	intel_dp->link_rate = pipe_config->port_clock;
+	intel_dp->lane_count = pipe_config->lane_count;
+}
+
 static void intel_dp_prepare(struct intel_encoder *encoder)
 {
 	struct drm_device *dev = encoder->base.dev;
@@ -1593,6 +1600,8 @@  static void intel_dp_prepare(struct intel_encoder *encoder)
 	struct intel_crtc *crtc = to_intel_crtc(encoder->base.crtc);
 	struct drm_display_mode *adjusted_mode = &crtc->config->base.adjusted_mode;
 
+	intel_dp_set_link_params(intel_dp, crtc->config);
+
 	/*
 	 * There are four kinds of DP registers:
 	 *
@@ -3348,15 +3357,13 @@  static void
 intel_get_adjust_train(struct intel_dp *intel_dp,
 		       const uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
-	struct intel_crtc *crtc =
-		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	uint8_t v = 0;
 	uint8_t p = 0;
 	int lane;
 	uint8_t voltage_max;
 	uint8_t preemph_max;
 
-	for (lane = 0; lane < crtc->config->lane_count; lane++) {
+	for (lane = 0; lane < intel_dp->lane_count; lane++) {
 		uint8_t this_v = drm_dp_get_adjust_request_voltage(link_status, lane);
 		uint8_t this_p = drm_dp_get_adjust_request_pre_emphasis(link_status, lane);
 
@@ -3527,8 +3534,6 @@  intel_dp_set_link_train(struct intel_dp *intel_dp,
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv =
 		to_i915(intel_dig_port->base.base.dev);
-	struct intel_crtc *crtc =
-		to_intel_crtc(intel_dig_port->base.base.crtc);
 	uint8_t buf[sizeof(intel_dp->train_set) + 1];
 	int ret, len;
 
@@ -3544,8 +3549,8 @@  intel_dp_set_link_train(struct intel_dp *intel_dp,
 		len = 1;
 	} else {
 		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
-		memcpy(buf + 1, intel_dp->train_set, crtc->config->lane_count);
-		len = crtc->config->lane_count + 1;
+		memcpy(buf + 1, intel_dp->train_set, intel_dp->lane_count);
+		len = intel_dp->lane_count + 1;
 	}
 
 	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_PATTERN_SET,
@@ -3571,8 +3576,6 @@  intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_i915_private *dev_priv =
 		to_i915(intel_dig_port->base.base.dev);
-	struct intel_crtc *crtc =
-		to_intel_crtc(intel_dig_port->base.base.crtc);
 	int ret;
 
 	intel_get_adjust_train(intel_dp, link_status);
@@ -3582,9 +3585,9 @@  intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	POSTING_READ(intel_dp->output_reg);
 
 	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
-				intel_dp->train_set, crtc->config->lane_count);
+				intel_dp->train_set, intel_dp->lane_count);
 
-	return ret == crtc->config->lane_count;
+	return ret == intel_dp->lane_count;
 }
 
 static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
@@ -3623,8 +3626,6 @@  void
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
 	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	struct drm_device *dev = encoder->dev;
 	int i;
 	uint8_t voltage;
@@ -3636,12 +3637,12 @@  intel_dp_start_link_train(struct intel_dp *intel_dp)
 	if (HAS_DDI(dev))
 		intel_ddi_prepare_link_retrain(encoder);
 
-	intel_dp_compute_rate(intel_dp, crtc->config->port_clock,
+	intel_dp_compute_rate(intel_dp, intel_dp->link_rate,
 			      &link_bw, &rate_select);
 
 	/* Write the link configuration data */
 	link_config[0] = link_bw;
-	link_config[1] = crtc->config->lane_count;
+	link_config[1] = intel_dp->lane_count;
 	if (drm_dp_enhanced_frame_cap(intel_dp->dpcd))
 		link_config[1] |= DP_LANE_COUNT_ENHANCED_FRAME_EN;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_BW_SET, link_config, 2);
@@ -3675,7 +3676,7 @@  intel_dp_start_link_train(struct intel_dp *intel_dp)
 			break;
 		}
 
-		if (drm_dp_clock_recovery_ok(link_status, crtc->config->lane_count)) {
+		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
 			DRM_DEBUG_KMS("clock recovery OK\n");
 			break;
 		}
@@ -3698,10 +3699,10 @@  intel_dp_start_link_train(struct intel_dp *intel_dp)
 		}
 
 		/* Check to see if we've tried the max voltage */
-		for (i = 0; i < crtc->config->lane_count; i++)
+		for (i = 0; i < intel_dp->lane_count; i++)
 			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
 				break;
-		if (i == crtc->config->lane_count) {
+		if (i == intel_dp->lane_count) {
 			++loop_tries;
 			if (loop_tries == 5) {
 				DRM_ERROR("too many full retries, give up\n");
@@ -3738,15 +3739,13 @@  intel_dp_start_link_train(struct intel_dp *intel_dp)
 void
 intel_dp_complete_link_train(struct intel_dp *intel_dp)
 {
-	struct intel_crtc *crtc =
-		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	bool channel_eq = false;
 	int tries, cr_tries;
 	uint32_t DP = intel_dp->DP;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
 
 	/* Training Pattern 3 for HBR2 or 1.2 devices that support it*/
-	if (crtc->config->port_clock == 540000 || intel_dp->use_tps3)
+	if (intel_dp->link_rate == 540000 || intel_dp->use_tps3)
 		training_pattern = DP_TRAINING_PATTERN_3;
 
 	/* channel equalization */
@@ -3776,7 +3775,7 @@  intel_dp_complete_link_train(struct intel_dp *intel_dp)
 
 		/* Make sure clock is still ok */
 		if (!drm_dp_clock_recovery_ok(link_status,
-					      crtc->config->lane_count)) {
+					      intel_dp->lane_count)) {
 			intel_dp->train_set_valid = false;
 			intel_dp_start_link_train(intel_dp);
 			intel_dp_set_link_train(intel_dp, &DP,
@@ -3787,7 +3786,7 @@  intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		}
 
 		if (drm_dp_channel_eq_ok(link_status,
-					 crtc->config->lane_count)) {
+					 intel_dp->lane_count)) {
 			channel_eq = true;
 			break;
 		}
@@ -4285,8 +4284,6 @@  update_status:
 static int
 intel_dp_check_mst_status(struct intel_dp *intel_dp)
 {
-	struct intel_crtc *crtc =
-		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	bool bret;
 
 	if (intel_dp->is_mst) {
@@ -4300,7 +4297,7 @@  go_again:
 
 			/* check link status - esi[10] = 0x200c */
 			if (intel_dp->active_mst_links &&
-			    !drm_dp_channel_eq_ok(&esi[10], crtc->config->lane_count)) {
+			    !drm_dp_channel_eq_ok(&esi[10], intel_dp->lane_count)) {
 				DRM_DEBUG_KMS("channel EQ not ok, retraining\n");
 				intel_dp_start_link_train(intel_dp);
 				intel_dp_complete_link_train(intel_dp);
@@ -4355,8 +4352,6 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	struct intel_encoder *intel_encoder = &dp_to_dig_port(intel_dp)->base;
-	struct intel_crtc *crtc =
-		to_intel_crtc(dp_to_dig_port(intel_dp)->base.base.crtc);
 	u8 sink_irq_vector;
 	u8 link_status[DP_LINK_STATUS_SIZE];
 
@@ -4392,7 +4387,7 @@  intel_dp_check_link_status(struct intel_dp *intel_dp)
 			DRM_DEBUG_DRIVER("CP or sink specific irq unhandled\n");
 	}
 
-	if (!drm_dp_channel_eq_ok(link_status, crtc->config->lane_count)) {
+	if (!drm_dp_channel_eq_ok(link_status, intel_dp->lane_count)) {
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 		intel_dp_start_link_train(intel_dp);
diff --git a/drivers/gpu/drm/i915/intel_dp_mst.c b/drivers/gpu/drm/i915/intel_dp_mst.c
index 9ec5c20..ebf2054 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -165,6 +165,8 @@  static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 	if (intel_dp->active_mst_links == 0) {
 		enum port port = intel_ddi_get_encoder_port(encoder);
 
+		intel_dp_set_link_params(intel_dp, intel_crtc->config);
+
 		/* FIXME: add support for SKL */
 		if (INTEL_INFO(dev)->gen < 9)
 			I915_WRITE(PORT_CLK_SEL(port),
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 81b7d77..71a2e18 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -708,6 +708,8 @@  struct intel_dp {
 	uint32_t output_reg;
 	uint32_t aux_ch_ctl_reg;
 	uint32_t DP;
+	int link_rate;
+	uint8_t lane_count;
 	bool has_audio;
 	enum hdmi_force_audio force_audio;
 	bool limited_color_range;
@@ -1161,6 +1163,8 @@  void assert_csr_loaded(struct drm_i915_private *dev_priv);
 void intel_dp_init(struct drm_device *dev, int output_reg, enum port port);
 bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 			     struct intel_connector *intel_connector);
+void intel_dp_set_link_params(struct intel_dp *intel_dp,
+			      const struct intel_crtc_state *pipe_config);
 void intel_dp_start_link_train(struct intel_dp *intel_dp);
 void intel_dp_complete_link_train(struct intel_dp *intel_dp);
 void intel_dp_stop_link_train(struct intel_dp *intel_dp);