[PATCH 00/15] Making DP link training code more readable

[PATCH 00/15] Making DP link training code more readable

[Ander Conselvan de Oliveira]

Hi,

This patch series attempts to make the DP link training code more readable
by splitting the code to a separate file and doing some small improvements
here and there. There is still more work to be done, but I figured I'll
send these out before I drown in unmerged patches.

I used the tests I described in an earlier email to test these. The latest
code for the tests can be found on the following github repo:

  https://github.com/anderco/linux/tree/link-training

Thanks,
Ander


Ander Conselvan de Oliveira (15):
  drm/i915: Rename DP link training functions
  drm/i915: Don't pass *DP around to link training functions
  drm/i915: Split intel_dp_update_link_train()
  drm/i915: Split write of pattern to DP reg from
    intel_dp_set_link_train
  drm/i915: Don't call intel_dp_set_signal_levels() on link train reset
  drm/i915: Move generic link training code to a separate file
  drm/i915: Create intel_dp->prepare_link_retrain() hook
  drm/i915: Make intel_dp_source_supports_hbr2() take an intel_dp
    pointer
  drm/i915: Move link training setup code to separate functions
  drm/i915: Move test for max voltage on all lanes to separate function
  drm/i915: Add function for getting the current link training voltage
  drm/i915: Split full retries loop out of clock recovery code
  drm/i915: Make the link training test for same voltage 5 times smaller
  drm/i915: Move the voltage changed check into intel_get_adjust_train()
  drm/i915: Add missing newline to link training debug message

 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/intel_ddi.c              |   9 +-
 drivers/gpu/drm/i915/intel_dp.c               | 343 +++---------------------
 drivers/gpu/drm/i915/intel_dp_link_training.c | 360 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_dp_mst.c           |   1 -
 drivers/gpu/drm/i915/intel_drv.h              |  23 +-
 6 files changed, 415 insertions(+), 322 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_dp_link_training.c

-- 
2.4.3

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

[PATCH 01/15] drm/i915: Rename DP link training functions

[Ander Conselvan de Oliveira]

The link training functions had confusing names. The start function
actually does the clock recovery phase of the link training, and the
complete function does the channel equalization. So call them that
instead. Also, every call to intel_dp_start_link_train() was followed
by a call to intel_dp_complete_link_train(), so add a new start
function that calls clock_recory and channel_equalization.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c    |  1 -
 drivers/gpu/drm/i915/intel_dp.c     | 22 +++++++++++++---------
 drivers/gpu/drm/i915/intel_dp_mst.c |  1 -
 drivers/gpu/drm/i915/intel_drv.h    |  1 -
 4 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 2d3cc82..220679d 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2317,7 +2317,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
 
 		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 		intel_dp_start_link_train(intel_dp);
-		intel_dp_complete_link_train(intel_dp);
 		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
 			intel_dp_stop_link_train(intel_dp);
 	} else if (type == INTEL_OUTPUT_HDMI) {
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 97ed418..c420edf 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2604,7 +2604,6 @@ static void intel_enable_dp(struct intel_encoder *encoder)
 
 	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
 	intel_dp_start_link_train(intel_dp);
-	intel_dp_complete_link_train(intel_dp);
 	intel_dp_stop_link_train(intel_dp);
 
 	if (crtc->config->has_audio) {
@@ -3696,8 +3695,8 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 }
 
 /* Enable corresponding port and start training pattern 1 */
-void
-intel_dp_start_link_train(struct intel_dp *intel_dp)
+static void
+intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
 	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
 	struct drm_device *dev = encoder->dev;
@@ -3810,8 +3809,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
 	intel_dp->DP = DP;
 }
 
-void
-intel_dp_complete_link_train(struct intel_dp *intel_dp)
+static void
+intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
@@ -3864,7 +3863,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		if (!drm_dp_clock_recovery_ok(link_status,
 					      intel_dp->lane_count)) {
 			intel_dp->train_set_valid = false;
-			intel_dp_start_link_train(intel_dp);
+			intel_dp_link_training_clock_recovery(intel_dp);
 			intel_dp_set_link_train(intel_dp, &DP,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
@@ -3881,7 +3880,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		/* Try 5 times, then try clock recovery if that fails */
 		if (tries > 5) {
 			intel_dp->train_set_valid = false;
-			intel_dp_start_link_train(intel_dp);
+			intel_dp_link_training_clock_recovery(intel_dp);
 			intel_dp_set_link_train(intel_dp, &DP,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
@@ -3914,6 +3913,13 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
+void
+intel_dp_start_link_train(struct intel_dp *intel_dp)
+{
+	intel_dp_link_training_clock_recovery(intel_dp);
+	intel_dp_link_training_channel_equalization(intel_dp);
+}
+
 static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
@@ -4382,7 +4388,6 @@ go_again:
 			    !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);
 				intel_dp_stop_link_train(intel_dp);
 			}
 
@@ -4473,7 +4478,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
 			      intel_encoder->base.name);
 		intel_dp_start_link_train(intel_dp);
-		intel_dp_complete_link_train(intel_dp);
 		intel_dp_stop_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 ca4d022..1537259 100644
--- a/drivers/gpu/drm/i915/intel_dp_mst.c
+++ b/drivers/gpu/drm/i915/intel_dp_mst.c
@@ -188,7 +188,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
 
 
 		intel_dp_start_link_train(intel_dp);
-		intel_dp_complete_link_train(intel_dp);
 		intel_dp_stop_link_train(intel_dp);
 	}
 
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index dfd2d10..5366c9e 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1215,7 +1215,6 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 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);
 void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
 void intel_dp_encoder_destroy(struct drm_encoder *encoder);
-- 
2.4.3

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

Re: [PATCH 01/15] drm/i915: Rename DP link training functions

[Daniel Vetter]

On Mon, Oct 05, 2015 at 10:01:13AM +0300, Ander Conselvan de Oliveira wrote:
> The link training functions had confusing names. The start function
> actually does the clock recovery phase of the link training, and the
> complete function does the channel equalization. So call them that
> instead. Also, every call to intel_dp_start_link_train() was followed
> by a call to intel_dp_complete_link_train(), so add a new start
> function that calls clock_recory and channel_equalization.
> 
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>

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

> ---
>  drivers/gpu/drm/i915/intel_ddi.c    |  1 -
>  drivers/gpu/drm/i915/intel_dp.c     | 22 +++++++++++++---------
>  drivers/gpu/drm/i915/intel_dp_mst.c |  1 -
>  drivers/gpu/drm/i915/intel_drv.h    |  1 -
>  4 files changed, 13 insertions(+), 12 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
> index 2d3cc82..220679d 100644
> --- a/drivers/gpu/drm/i915/intel_ddi.c
> +++ b/drivers/gpu/drm/i915/intel_ddi.c
> @@ -2317,7 +2317,6 @@ static void intel_ddi_pre_enable(struct intel_encoder *intel_encoder)
>  
>  		intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>  		intel_dp_start_link_train(intel_dp);
> -		intel_dp_complete_link_train(intel_dp);
>  		if (port != PORT_A || INTEL_INFO(dev)->gen >= 9)
>  			intel_dp_stop_link_train(intel_dp);
>  	} else if (type == INTEL_OUTPUT_HDMI) {
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 97ed418..c420edf 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -2604,7 +2604,6 @@ static void intel_enable_dp(struct intel_encoder *encoder)
>  
>  	intel_dp_sink_dpms(intel_dp, DRM_MODE_DPMS_ON);
>  	intel_dp_start_link_train(intel_dp);
> -	intel_dp_complete_link_train(intel_dp);
>  	intel_dp_stop_link_train(intel_dp);
>  
>  	if (crtc->config->has_audio) {
> @@ -3696,8 +3695,8 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>  }
>  
>  /* Enable corresponding port and start training pattern 1 */
> -void
> -intel_dp_start_link_train(struct intel_dp *intel_dp)
> +static void
> +intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>  {
>  	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
>  	struct drm_device *dev = encoder->dev;
> @@ -3810,8 +3809,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>  	intel_dp->DP = DP;
>  }
>  
> -void
> -intel_dp_complete_link_train(struct intel_dp *intel_dp)
> +static void
> +intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>  {
>  	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>  	struct drm_device *dev = dig_port->base.base.dev;
> @@ -3864,7 +3863,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  		if (!drm_dp_clock_recovery_ok(link_status,
>  					      intel_dp->lane_count)) {
>  			intel_dp->train_set_valid = false;
> -			intel_dp_start_link_train(intel_dp);
> +			intel_dp_link_training_clock_recovery(intel_dp);
>  			intel_dp_set_link_train(intel_dp, &DP,
>  						training_pattern |
>  						DP_LINK_SCRAMBLING_DISABLE);
> @@ -3881,7 +3880,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  		/* Try 5 times, then try clock recovery if that fails */
>  		if (tries > 5) {
>  			intel_dp->train_set_valid = false;
> -			intel_dp_start_link_train(intel_dp);
> +			intel_dp_link_training_clock_recovery(intel_dp);
>  			intel_dp_set_link_train(intel_dp, &DP,
>  						training_pattern |
>  						DP_LINK_SCRAMBLING_DISABLE);
> @@ -3914,6 +3913,13 @@ void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>  				DP_TRAINING_PATTERN_DISABLE);
>  }
>  
> +void
> +intel_dp_start_link_train(struct intel_dp *intel_dp)
> +{
> +	intel_dp_link_training_clock_recovery(intel_dp);
> +	intel_dp_link_training_channel_equalization(intel_dp);
> +}
> +
>  static void
>  intel_dp_link_down(struct intel_dp *intel_dp)
>  {
> @@ -4382,7 +4388,6 @@ go_again:
>  			    !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);
>  				intel_dp_stop_link_train(intel_dp);
>  			}
>  
> @@ -4473,7 +4478,6 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
>  		DRM_DEBUG_KMS("%s: channel EQ not ok, retraining\n",
>  			      intel_encoder->base.name);
>  		intel_dp_start_link_train(intel_dp);
> -		intel_dp_complete_link_train(intel_dp);
>  		intel_dp_stop_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 ca4d022..1537259 100644
> --- a/drivers/gpu/drm/i915/intel_dp_mst.c
> +++ b/drivers/gpu/drm/i915/intel_dp_mst.c
> @@ -188,7 +188,6 @@ static void intel_mst_pre_enable_dp(struct intel_encoder *encoder)
>  
>  
>  		intel_dp_start_link_train(intel_dp);
> -		intel_dp_complete_link_train(intel_dp);
>  		intel_dp_stop_link_train(intel_dp);
>  	}
>  
> diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
> index dfd2d10..5366c9e 100644
> --- a/drivers/gpu/drm/i915/intel_drv.h
> +++ b/drivers/gpu/drm/i915/intel_drv.h
> @@ -1215,7 +1215,6 @@ bool intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
>  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);
>  void intel_dp_sink_dpms(struct intel_dp *intel_dp, int mode);
>  void intel_dp_encoder_destroy(struct drm_encoder *encoder);
> -- 
> 2.4.3
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

[PATCH 02/15] drm/i915: Don't pass *DP around to link training functions

[Ander Conselvan de Oliveira]

It just makes the code more confusing, so just reference intel_dp_>DP
directly. The old behavior of not updating the value in intel_dp if link
training fail is preserved by saving the previous value of DP in the
stack and restoring the old value in case of failure.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
--

I'm not sure the old behavior is correct, but to err in the side of
caution I tried not to change it.

---
 drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++---------------------
 1 file changed, 33 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index c420edf..391a367 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 
 static bool
 intel_dp_set_link_train(struct intel_dp *intel_dp,
-			uint32_t *DP,
 			uint8_t dp_train_pat)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 	uint8_t buf[sizeof(intel_dp->train_set) + 1];
 	int ret, len;
 
-	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
+	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
 
-	I915_WRITE(intel_dp->output_reg, *DP);
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
 
 	buf[0] = dp_train_pat;
@@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 }
 
 static bool
-intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
+intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
 	if (!intel_dp->train_set_valid)
 		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
-	intel_dp_set_signal_levels(intel_dp, DP);
-	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
+	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
+	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
 
 static bool
-intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
+intel_dp_update_link_train(struct intel_dp *intel_dp,
 			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	int ret;
 
 	intel_get_adjust_train(intel_dp, link_status);
-	intel_dp_set_signal_levels(intel_dp, DP);
+	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
 
-	I915_WRITE(intel_dp->output_reg, *DP);
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
 
 	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
@@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 }
 
 /* Enable corresponding port and start training pattern 1 */
-static void
+static bool
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
 	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
@@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	int i;
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
-	uint32_t DP = intel_dp->DP;
 	uint8_t link_config[2];
 	uint8_t link_bw, rate_select;
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 	if (HAS_DDI(dev))
 		intel_ddi_prepare_link_retrain(encoder);
@@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	link_config[1] = DP_SET_ANSI_8B10B;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
 
-	DP |= DP_PORT_EN;
+	intel_dp->DP |= DP_PORT_EN;
 
 	/* clock recovery */
-	if (!intel_dp_reset_link_train(intel_dp, &DP,
+	if (!intel_dp_reset_link_train(intel_dp,
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to enable link training\n");
-		return;
+		return false;
 	}
 
 	voltage = 0xff;
 	voltage_tries = 0;
 	loop_tries = 0;
 	for (;;) {
-		uint8_t link_status[DP_LINK_STATUS_SIZE];
-
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
 			DRM_ERROR("failed to get link status\n");
@@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 			DRM_DEBUG_KMS("clock recovery not ok, reset");
 			/* clear the flag as we are not reusing train set */
 			intel_dp->train_set_valid = false;
-			if (!intel_dp_reset_link_train(intel_dp, &DP,
+			if (!intel_dp_reset_link_train(intel_dp,
 						       DP_TRAINING_PATTERN_1 |
 						       DP_LINK_SCRAMBLING_DISABLE)) {
 				DRM_ERROR("failed to enable link training\n");
-				return;
+				return false;
 			}
 			continue;
 		}
@@ -3781,7 +3778,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 				DRM_ERROR("too many full retries, give up\n");
 				break;
 			}
-			intel_dp_reset_link_train(intel_dp, &DP,
+			intel_dp_reset_link_train(intel_dp,
 						  DP_TRAINING_PATTERN_1 |
 						  DP_LINK_SCRAMBLING_DISABLE);
 			voltage_tries = 0;
@@ -3800,23 +3797,22 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+		if (!intel_dp_update_link_train(intel_dp, link_status)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
 	}
 
-	intel_dp->DP = DP;
+	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
 }
 
-static void
+static bool
 intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = dig_port->base.base.dev;
 	bool channel_eq = false;
 	int tries, cr_tries;
-	uint32_t DP = intel_dp->DP;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
 
 	/*
@@ -3835,11 +3831,11 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
 
 	/* channel equalization */
-	if (!intel_dp_set_link_train(intel_dp, &DP,
+	if (!intel_dp_set_link_train(intel_dp,
 				     training_pattern |
 				     DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to start channel equalization\n");
-		return;
+		return false;
 	}
 
 	tries = 0;
@@ -3864,7 +3860,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 					      intel_dp->lane_count)) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp, &DP,
+			intel_dp_set_link_train(intel_dp,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
 			cr_tries++;
@@ -3881,7 +3877,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		if (tries > 5) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp, &DP,
+			intel_dp_set_link_train(intel_dp,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
 			tries = 0;
@@ -3890,7 +3886,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		}
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+		if (!intel_dp_update_link_train(intel_dp, link_status)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
@@ -3899,25 +3895,29 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 
 	intel_dp_set_idle_link_train(intel_dp);
 
-	intel_dp->DP = DP;
-
 	if (channel_eq) {
 		intel_dp->train_set_valid = true;
 		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
+		return true;
+	} else {
+		return false;
 	}
 }
 
 void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
+	intel_dp_set_link_train(intel_dp,
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
 void
 intel_dp_start_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_link_training_clock_recovery(intel_dp);
-	intel_dp_link_training_channel_equalization(intel_dp);
+	uint32_t DP = intel_dp->DP;
+
+	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
+	    !intel_dp_link_training_channel_equalization(intel_dp))
+		intel_dp->DP = DP;
 }
 
 static void
-- 
2.4.3

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

Re: [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions

[Thulasimani, Sivakumar]

On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote:
> It just makes the code more confusing, so just reference intel_dp_>DP
> directly. The old behavior of not updating the value in intel_dp if link
> training fail is preserved by saving the previous value of DP in the
> stack and restoring the old value in case of failure.
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> --
>
> I'm not sure the old behavior is correct, but to err in the side of
> caution I tried not to change it.
>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++---------------------
>   1 file changed, 33 insertions(+), 33 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index c420edf..391a367 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>   
>   static bool
>   intel_dp_set_link_train(struct intel_dp *intel_dp,
> -			uint32_t *DP,
>   			uint8_t dp_train_pat)
>   {
>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>   	uint8_t buf[sizeof(intel_dp->train_set) + 1];
>   	int ret, len;
>   
> -	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> +	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
>   
> -	I915_WRITE(intel_dp->output_reg, *DP);
> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>   	POSTING_READ(intel_dp->output_reg);
>   
>   	buf[0] = dp_train_pat;
> @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>   }
>   
>   static bool
> -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> +intel_dp_reset_link_train(struct intel_dp *intel_dp,
>   			uint8_t dp_train_pat)
>   {
>   	if (!intel_dp->train_set_valid)
>   		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> -	intel_dp_set_signal_levels(intel_dp, DP);
> -	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
> +	return intel_dp_set_link_train(intel_dp, dp_train_pat);
>   }
>   
>   static bool
> -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> +intel_dp_update_link_train(struct intel_dp *intel_dp,
>   			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
>   {
>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>   	int ret;
>   
>   	intel_get_adjust_train(intel_dp, link_status);
> -	intel_dp_set_signal_levels(intel_dp, DP);
> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
>   
> -	I915_WRITE(intel_dp->output_reg, *DP);
> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>   	POSTING_READ(intel_dp->output_reg);
>   
>   	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
> @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
>   }
>   
>   /* Enable corresponding port and start training pattern 1 */
> -static void
> +static bool
>   intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   {
>   	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
> @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   	int i;
>   	uint8_t voltage;
>   	int voltage_tries, loop_tries;
> -	uint32_t DP = intel_dp->DP;
>   	uint8_t link_config[2];
>   	uint8_t link_bw, rate_select;
> +	uint8_t link_status[DP_LINK_STATUS_SIZE];
>   
>   	if (HAS_DDI(dev))
>   		intel_ddi_prepare_link_retrain(encoder);
> @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   	link_config[1] = DP_SET_ANSI_8B10B;
>   	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>   
> -	DP |= DP_PORT_EN;
> +	intel_dp->DP |= DP_PORT_EN;
>   
>   	/* clock recovery */
> -	if (!intel_dp_reset_link_train(intel_dp, &DP,
> +	if (!intel_dp_reset_link_train(intel_dp,
>   				       DP_TRAINING_PATTERN_1 |
>   				       DP_LINK_SCRAMBLING_DISABLE)) {
>   		DRM_ERROR("failed to enable link training\n");
> -		return;
> +		return false;
>   	}
>   
>   	voltage = 0xff;
>   	voltage_tries = 0;
>   	loop_tries = 0;
>   	for (;;) {
> -		uint8_t link_status[DP_LINK_STATUS_SIZE];
> -
>   		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
>   		if (!intel_dp_get_link_status(intel_dp, link_status)) {
>   			DRM_ERROR("failed to get link status\n");
> @@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   			DRM_DEBUG_KMS("clock recovery not ok, reset");
>   			/* clear the flag as we are not reusing train set */
>   			intel_dp->train_set_valid = false;
> -			if (!intel_dp_reset_link_train(intel_dp, &DP,
> +			if (!intel_dp_reset_link_train(intel_dp,
>   						       DP_TRAINING_PATTERN_1 |
>   						       DP_LINK_SCRAMBLING_DISABLE)) {
>   				DRM_ERROR("failed to enable link training\n");
> -				return;
> +				return false;
>   			}
>   			continue;
>   		}
> @@ -3781,7 +3778,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   				DRM_ERROR("too many full retries, give up\n");
>   				break;
>   			}
> -			intel_dp_reset_link_train(intel_dp, &DP,
> +			intel_dp_reset_link_train(intel_dp,
>   						  DP_TRAINING_PATTERN_1 |
>   						  DP_LINK_SCRAMBLING_DISABLE);
>   			voltage_tries = 0;
> @@ -3800,23 +3797,22 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
>   
>   		/* Update training set as requested by target */
> -		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>   			DRM_ERROR("failed to update link training\n");
>   			break;
>   		}
>   	}
>   
> -	intel_dp->DP = DP;
> +	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
why are we calling the same function again? in best case this function 
is called and returned true,
  or worst case it was never called. so it will be simpler if we store 
the return value of this function
inside the loop and return that here ?
>   }
>   
> -static void
> +static bool
>   intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   {
>   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>   	struct drm_device *dev = dig_port->base.base.dev;
>   	bool channel_eq = false;
>   	int tries, cr_tries;
> -	uint32_t DP = intel_dp->DP;
>   	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
>   
>   	/*
> @@ -3835,11 +3831,11 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
>   
>   	/* channel equalization */
> -	if (!intel_dp_set_link_train(intel_dp, &DP,
> +	if (!intel_dp_set_link_train(intel_dp,
>   				     training_pattern |
>   				     DP_LINK_SCRAMBLING_DISABLE)) {
>   		DRM_ERROR("failed to start channel equalization\n");
> -		return;
> +		return false;
>   	}
>   
>   	tries = 0;
> @@ -3864,7 +3860,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   					      intel_dp->lane_count)) {
>   			intel_dp->train_set_valid = false;
>   			intel_dp_link_training_clock_recovery(intel_dp);
> -			intel_dp_set_link_train(intel_dp, &DP,
> +			intel_dp_set_link_train(intel_dp,
>   						training_pattern |
>   						DP_LINK_SCRAMBLING_DISABLE);
>   			cr_tries++;
> @@ -3881,7 +3877,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		if (tries > 5) {
>   			intel_dp->train_set_valid = false;
>   			intel_dp_link_training_clock_recovery(intel_dp);
> -			intel_dp_set_link_train(intel_dp, &DP,
> +			intel_dp_set_link_train(intel_dp,
>   						training_pattern |
>   						DP_LINK_SCRAMBLING_DISABLE);
>   			tries = 0;
> @@ -3890,7 +3886,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		}
>   
>   		/* Update training set as requested by target */
> -		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>   			DRM_ERROR("failed to update link training\n");
>   			break;
>   		}
> @@ -3899,25 +3895,29 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   
>   	intel_dp_set_idle_link_train(intel_dp);
>   
> -	intel_dp->DP = DP;
> -
>   	if (channel_eq) {
>   		intel_dp->train_set_valid = true;
>   		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> +		return true;
> +	} else {
> +		return false;
>   	}
>   }
>   
>   void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>   {
> -	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> +	intel_dp_set_link_train(intel_dp,
>   				DP_TRAINING_PATTERN_DISABLE);
>   }
>   
>   void
>   intel_dp_start_link_train(struct intel_dp *intel_dp)
>   {
> -	intel_dp_link_training_clock_recovery(intel_dp);
> -	intel_dp_link_training_channel_equalization(intel_dp);
> +	uint32_t DP = intel_dp->DP;
> +
> +	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
> +	    !intel_dp_link_training_channel_equalization(intel_dp))
> +		intel_dp->DP = DP;
it is wrong to restore the value of DP here, we have modified the value 
of port/ddi already inside the two functions.
if either of these two steps fail we should call stop link training and 
follow it with bspec disable sequence.
so saving and restoring will not help us in anyway but more hide the 
real status of HW.
>   }
>   
>   static void

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

Re: [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions

[Conselvan De Oliveira, Ander]

On Mon, 2015-10-19 at 10:15 +0530, Thulasimani, Sivakumar wrote:
> 
> On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote:
> > It just makes the code more confusing, so just reference intel_dp_>DP
> > directly. The old behavior of not updating the value in intel_dp if link
> > training fail is preserved by saving the previous value of DP in the
> > stack and restoring the old value in case of failure.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@intel.com>
> > --
> > 
> > I'm not sure the old behavior is correct, but to err in the side of
> > caution I tried not to change it.
> > 
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++-----------------
> > ----
> >   1 file changed, 33 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index c420edf..391a367 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp,
> > uint32_t *DP)
> >   
> >   static bool
> >   intel_dp_set_link_train(struct intel_dp *intel_dp,
> > -			uint32_t *DP,
> >   			uint8_t dp_train_pat)
> >   {
> >   	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> >   	uint8_t buf[sizeof(intel_dp->train_set) + 1];
> >   	int ret, len;
> >   
> > -	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> > +	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
> >   
> > -	I915_WRITE(intel_dp->output_reg, *DP);
> > +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> >   	POSTING_READ(intel_dp->output_reg);
> >   
> >   	buf[0] = dp_train_pat;
> > @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> >   }
> >   
> >   static bool
> > -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> > +intel_dp_reset_link_train(struct intel_dp *intel_dp,
> >   			uint8_t dp_train_pat)
> >   {
> >   	if (!intel_dp->train_set_valid)
> >   		memset(intel_dp->train_set, 0, sizeof(intel_dp
> > ->train_set));
> > -	intel_dp_set_signal_levels(intel_dp, DP);
> > -	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> > +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
> > +	return intel_dp_set_link_train(intel_dp, dp_train_pat);
> >   }
> >   
> >   static bool
> > -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> > +intel_dp_update_link_train(struct intel_dp *intel_dp,
> >   			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
> >   {
> >   	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp,
> > uint32_t *DP,
> >   	int ret;
> >   
> >   	intel_get_adjust_train(intel_dp, link_status);
> > -	intel_dp_set_signal_levels(intel_dp, DP);
> > +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
> >   
> > -	I915_WRITE(intel_dp->output_reg, *DP);
> > +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> >   	POSTING_READ(intel_dp->output_reg);
> >   
> >   	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
> > @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct
> > intel_dp *intel_dp)
> >   }
> >   
> >   /* Enable corresponding port and start training pattern 1 */
> > -static void
> > +static bool
> >   intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >   {
> >   	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)
> > ->base.base;
> > @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> > *intel_dp)
> >   	int i;
> >   	uint8_t voltage;
> >   	int voltage_tries, loop_tries;
> > -	uint32_t DP = intel_dp->DP;
> >   	uint8_t link_config[2];
> >   	uint8_t link_bw, rate_select;
> > +	uint8_t link_status[DP_LINK_STATUS_SIZE];
> >   
> >   	if (HAS_DDI(dev))
> >   		intel_ddi_prepare_link_retrain(encoder);
> > @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   	link_config[1] = DP_SET_ANSI_8B10B;
> >   	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config,
> > 2);
> >   
> > -	DP |= DP_PORT_EN;
> > +	intel_dp->DP |= DP_PORT_EN;
> >   
> >   	/* clock recovery */
> > -	if (!intel_dp_reset_link_train(intel_dp, &DP,
> > +	if (!intel_dp_reset_link_train(intel_dp,
> >   				       DP_TRAINING_PATTERN_1 |
> >   				       DP_LINK_SCRAMBLING_DISABLE)) {
> >   		DRM_ERROR("failed to enable link training\n");
> > -		return;
> > +		return false;
> >   	}
> >   
> >   	voltage = 0xff;
> >   	voltage_tries = 0;
> >   	loop_tries = 0;
> >   	for (;;) {
> > -		uint8_t link_status[DP_LINK_STATUS_SIZE];
> > -
> >   		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> >   		if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to get link status\n");
> > @@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   			DRM_DEBUG_KMS("clock recovery not ok, reset");
> >   			/* clear the flag as we are not reusing train set
> > */
> >   			intel_dp->train_set_valid = false;
> > -			if (!intel_dp_reset_link_train(intel_dp, &DP,
> > +			if (!intel_dp_reset_link_train(intel_dp,
> >   						      
> >  DP_TRAINING_PATTERN_1 |
> >   						      
> >  DP_LINK_SCRAMBLING_DISABLE)) {
> >   				DRM_ERROR("failed to enable link
> > training\n");
> > -				return;
> > +				return false;
> >   			}
> >   			continue;
> >   		}
> > @@ -3781,7 +3778,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> > *intel_dp)
> >   				DRM_ERROR("too many full retries, give
> > up\n");
> >   				break;
> >   			}
> > -			intel_dp_reset_link_train(intel_dp, &DP,
> > +			intel_dp_reset_link_train(intel_dp,
> >   						  DP_TRAINING_PATTERN_1 |
> >   						 
> >  DP_LINK_SCRAMBLING_DISABLE);
> >   			voltage_tries = 0;
> > @@ -3800,23 +3797,22 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   		voltage = intel_dp->train_set[0] &
> > DP_TRAIN_VOLTAGE_SWING_MASK;
> >   
> >   		/* Update training set as requested by target */
> > -		if (!intel_dp_update_link_train(intel_dp, &DP,
> > link_status)) {
> > +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to update link training\n");
> >   			break;
> >   		}
> >   	}
> >   
> > -	intel_dp->DP = DP;
> > +	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
> why are we calling the same function again? in best case this function 
> is called and returned true,
>   or worst case it was never called. so it will be simpler if we store 
> the return value of this function
> inside the loop and return that here ?
> >   }
> >   
> > -static void
> > +static bool
> >   intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >   {
> >   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >   	struct drm_device *dev = dig_port->base.base.dev;
> >   	bool channel_eq = false;
> >   	int tries, cr_tries;
> > -	uint32_t DP = intel_dp->DP;
> >   	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
> >   
> >   	/*
> > @@ -3835,11 +3831,11 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3
> > support\n");
> >   
> >   	/* channel equalization */
> > -	if (!intel_dp_set_link_train(intel_dp, &DP,
> > +	if (!intel_dp_set_link_train(intel_dp,
> >   				     training_pattern |
> >   				     DP_LINK_SCRAMBLING_DISABLE)) {
> >   		DRM_ERROR("failed to start channel equalization\n");
> > -		return;
> > +		return false;
> >   	}
> >   
> >   	tries = 0;
> > @@ -3864,7 +3860,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   					      intel_dp->lane_count)) {
> >   			intel_dp->train_set_valid = false;
> >   			intel_dp_link_training_clock_recovery(intel_dp);
> > -			intel_dp_set_link_train(intel_dp, &DP,
> > +			intel_dp_set_link_train(intel_dp,
> >   						training_pattern |
> >   						DP_LINK_SCRAMBLING_DISABLE
> > );
> >   			cr_tries++;
> > @@ -3881,7 +3877,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		if (tries > 5) {
> >   			intel_dp->train_set_valid = false;
> >   			intel_dp_link_training_clock_recovery(intel_dp);
> > -			intel_dp_set_link_train(intel_dp, &DP,
> > +			intel_dp_set_link_train(intel_dp,
> >   						training_pattern |
> >   						DP_LINK_SCRAMBLING_DISABLE
> > );
> >   			tries = 0;
> > @@ -3890,7 +3886,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		}
> >   
> >   		/* Update training set as requested by target */
> > -		if (!intel_dp_update_link_train(intel_dp, &DP,
> > link_status)) {
> > +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to update link training\n");
> >   			break;
> >   		}
> > @@ -3899,25 +3895,29 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   
> >   	intel_dp_set_idle_link_train(intel_dp);
> >   
> > -	intel_dp->DP = DP;
> > -
> >   	if (channel_eq) {
> >   		intel_dp->train_set_valid = true;
> >   		DRM_DEBUG_KMS("Channel EQ done. DP Training
> > successful\n")
> > +		return true;
> > +	} else {
> > +		return false;
> >   	}
> >   }
> >   
> >   void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> >   {
> > -	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> > +	intel_dp_set_link_train(intel_dp,
> >   				DP_TRAINING_PATTERN_DISABLE);
> >   }
> >   
> >   void
> >   intel_dp_start_link_train(struct intel_dp *intel_dp)
> >   {
> > -	intel_dp_link_training_clock_recovery(intel_dp);
> > -	intel_dp_link_training_channel_equalization(intel_dp);
> > +	uint32_t DP = intel_dp->DP;
> > +
> > +	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
> > +	    !intel_dp_link_training_channel_equalization(intel_dp))
> > +		intel_dp->DP = DP;
> it is wrong to restore the value of DP here, we have modified the value 
> of port/ddi already inside the two functions.
> if either of these two steps fail we should call stop link training and 
> follow it with bspec disable sequence.
> so saving and restoring will not help us in anyway but more hide the 
> real status of HW.

That makes sense. I dug a bit deeper in the history, and the whole story of
preserving or not the value of DP is pretty confusing.

In the beginning [1], the value set during link training wasn't preserved at
all. In that case, since the same function enabled and disabled link training,
the only thing that was lost was the bit DP_PORT_EN being set.

When the channel equalization part was split to intel_dp_complete_link_train()
[2], a 'intel_dp->DP = DP;' was added to the end of start train. So only the
changes done to DP during channel eq weren't preserved. The same assignment was
added to the end of channel eq when fixing link training for HSW with eDP on
port-A [3].

Later, return statements were added [4, 5] to start and complete link train,
opening the possibility of a mismatch between the real value and the preserved
one.

I think the right thing to do here is to make sure intel_dp->DP always match
what was written to the hardware. At least I couldn't see any logic that relies
on the wrong value in case of failure.


	[1] commit a4fc5ed69817c73e32571ad7837bb707f9890009
	[2] commit 33a34e4e5969c5272cd6cb88f2e01c97218dd80b
	[3] commit 3ab9c63705cb7b1b9f83ddce725d8bd9ef7c66a9
	[4] commit 70aff66c953054334bf0569696901c13e206ade6
	[5] commit 4e96c97742f4201edf1b0f8e1b1b6b2ac6ff33e7


Ander

---------------------------------------------------------------------
Intel Finland Oy
Registered Address: PL 281, 00181 Helsinki 
Business Identity Code: 0357606 - 4 
Domiciled in Helsinki 

This e-mail and any attachments may contain confidential material for
the sole use of the intended recipient(s). Any review or distribution
by others is strictly prohibited. If you are not the intended
recipient, please contact the sender and delete all copies.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions

[Ander Conselvan De Oliveira]

On Mon, 2015-10-19 at 10:15 +0530, Thulasimani, Sivakumar wrote:
> 
> On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote:
> > It just makes the code more confusing, so just reference intel_dp_>DP
> > directly. The old behavior of not updating the value in intel_dp if link
> > training fail is preserved by saving the previous value of DP in the
> > stack and restoring the old value in case of failure.
> > 
> > Signed-off-by: Ander Conselvan de Oliveira <
> > ander.conselvan.de.oliveira@intel.com>
> > --
> > 
> > I'm not sure the old behavior is correct, but to err in the side of
> > caution I tried not to change it.
> > 
> > ---
> >   drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++-----------------
> > ----
> >   1 file changed, 33 insertions(+), 33 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c
> > index c420edf..391a367 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp,
> > uint32_t *DP)
> >   
> >   static bool
> >   intel_dp_set_link_train(struct intel_dp *intel_dp,
> > -			uint32_t *DP,
> >   			uint8_t dp_train_pat)
> >   {
> >   	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> >   	uint8_t buf[sizeof(intel_dp->train_set) + 1];
> >   	int ret, len;
> >   
> > -	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> > +	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
> >   
> > -	I915_WRITE(intel_dp->output_reg, *DP);
> > +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> >   	POSTING_READ(intel_dp->output_reg);
> >   
> >   	buf[0] = dp_train_pat;
> > @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
> >   }
> >   
> >   static bool
> > -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> > +intel_dp_reset_link_train(struct intel_dp *intel_dp,
> >   			uint8_t dp_train_pat)
> >   {
> >   	if (!intel_dp->train_set_valid)
> >   		memset(intel_dp->train_set, 0, sizeof(intel_dp
> > ->train_set));
> > -	intel_dp_set_signal_levels(intel_dp, DP);
> > -	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> > +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
> > +	return intel_dp_set_link_train(intel_dp, dp_train_pat);
> >   }
> >   
> >   static bool
> > -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> > +intel_dp_update_link_train(struct intel_dp *intel_dp,
> >   			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
> >   {
> >   	struct intel_digital_port *intel_dig_port =
> > dp_to_dig_port(intel_dp);
> > @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp,
> > uint32_t *DP,
> >   	int ret;
> >   
> >   	intel_get_adjust_train(intel_dp, link_status);
> > -	intel_dp_set_signal_levels(intel_dp, DP);
> > +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
> >   
> > -	I915_WRITE(intel_dp->output_reg, *DP);
> > +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
> >   	POSTING_READ(intel_dp->output_reg);
> >   
> >   	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
> > @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct
> > intel_dp *intel_dp)
> >   }
> >   
> >   /* Enable corresponding port and start training pattern 1 */
> > -static void
> > +static bool
> >   intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >   {
> >   	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)
> > ->base.base;
> > @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> > *intel_dp)
> >   	int i;
> >   	uint8_t voltage;
> >   	int voltage_tries, loop_tries;
> > -	uint32_t DP = intel_dp->DP;
> >   	uint8_t link_config[2];
> >   	uint8_t link_bw, rate_select;
> > +	uint8_t link_status[DP_LINK_STATUS_SIZE];
> >   
> >   	if (HAS_DDI(dev))
> >   		intel_ddi_prepare_link_retrain(encoder);
> > @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   	link_config[1] = DP_SET_ANSI_8B10B;
> >   	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config,
> > 2);
> >   
> > -	DP |= DP_PORT_EN;
> > +	intel_dp->DP |= DP_PORT_EN;
> >   
> >   	/* clock recovery */
> > -	if (!intel_dp_reset_link_train(intel_dp, &DP,
> > +	if (!intel_dp_reset_link_train(intel_dp,
> >   				       DP_TRAINING_PATTERN_1 |
> >   				       DP_LINK_SCRAMBLING_DISABLE)) {
> >   		DRM_ERROR("failed to enable link training\n");
> > -		return;
> > +		return false;
> >   	}
> >   
> >   	voltage = 0xff;
> >   	voltage_tries = 0;
> >   	loop_tries = 0;
> >   	for (;;) {
> > -		uint8_t link_status[DP_LINK_STATUS_SIZE];
> > -
> >   		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
> >   		if (!intel_dp_get_link_status(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to get link status\n");
> > @@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   			DRM_DEBUG_KMS("clock recovery not ok, reset");
> >   			/* clear the flag as we are not reusing train set
> > */
> >   			intel_dp->train_set_valid = false;
> > -			if (!intel_dp_reset_link_train(intel_dp, &DP,
> > +			if (!intel_dp_reset_link_train(intel_dp,
> >   						      
> >  DP_TRAINING_PATTERN_1 |
> >   						      
> >  DP_LINK_SCRAMBLING_DISABLE)) {
> >   				DRM_ERROR("failed to enable link
> > training\n");
> > -				return;
> > +				return false;
> >   			}
> >   			continue;
> >   		}
> > @@ -3781,7 +3778,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
> > *intel_dp)
> >   				DRM_ERROR("too many full retries, give
> > up\n");
> >   				break;
> >   			}
> > -			intel_dp_reset_link_train(intel_dp, &DP,
> > +			intel_dp_reset_link_train(intel_dp,
> >   						  DP_TRAINING_PATTERN_1 |
> >   						 
> >  DP_LINK_SCRAMBLING_DISABLE);
> >   			voltage_tries = 0;
> > @@ -3800,23 +3797,22 @@ intel_dp_link_training_clock_recovery(struct
> > intel_dp *intel_dp)
> >   		voltage = intel_dp->train_set[0] &
> > DP_TRAIN_VOLTAGE_SWING_MASK;
> >   
> >   		/* Update training set as requested by target */
> > -		if (!intel_dp_update_link_train(intel_dp, &DP,
> > link_status)) {
> > +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to update link training\n");
> >   			break;
> >   		}
> >   	}
> >   
> > -	intel_dp->DP = DP;
> > +	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
> why are we calling the same function again? in best case this function 
> is called and returned true,
>   or worst case it was never called. so it will be simpler if we store 
> the return value of this function
> inside the loop and return that here ?

I missed this comment earlier. I don't think calling drm_dp_clock_recovery_ok()
would have a big impact, since it is a very simple function. But I can add a
variable if that is preferred.

Ander


> >   }
> >   
> > -static void
> > +static bool
> >   intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
> >   {
> >   	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
> >   	struct drm_device *dev = dig_port->base.base.dev;
> >   	bool channel_eq = false;
> >   	int tries, cr_tries;
> > -	uint32_t DP = intel_dp->DP;
> >   	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
> >   
> >   	/*
> > @@ -3835,11 +3831,11 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3
> > support\n");
> >   
> >   	/* channel equalization */
> > -	if (!intel_dp_set_link_train(intel_dp, &DP,
> > +	if (!intel_dp_set_link_train(intel_dp,
> >   				     training_pattern |
> >   				     DP_LINK_SCRAMBLING_DISABLE)) {
> >   		DRM_ERROR("failed to start channel equalization\n");
> > -		return;
> > +		return false;
> >   	}
> >   
> >   	tries = 0;
> > @@ -3864,7 +3860,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   					      intel_dp->lane_count)) {
> >   			intel_dp->train_set_valid = false;
> >   			intel_dp_link_training_clock_recovery(intel_dp);
> > -			intel_dp_set_link_train(intel_dp, &DP,
> > +			intel_dp_set_link_train(intel_dp,
> >   						training_pattern |
> >   						DP_LINK_SCRAMBLING_DISABLE
> > );
> >   			cr_tries++;
> > @@ -3881,7 +3877,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		if (tries > 5) {
> >   			intel_dp->train_set_valid = false;
> >   			intel_dp_link_training_clock_recovery(intel_dp);
> > -			intel_dp_set_link_train(intel_dp, &DP,
> > +			intel_dp_set_link_train(intel_dp,
> >   						training_pattern |
> >   						DP_LINK_SCRAMBLING_DISABLE
> > );
> >   			tries = 0;
> > @@ -3890,7 +3886,7 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   		}
> >   
> >   		/* Update training set as requested by target */
> > -		if (!intel_dp_update_link_train(intel_dp, &DP,
> > link_status)) {
> > +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
> >   			DRM_ERROR("failed to update link training\n");
> >   			break;
> >   		}
> > @@ -3899,25 +3895,29 @@ intel_dp_link_training_channel_equalization(struct
> > intel_dp *intel_dp)
> >   
> >   	intel_dp_set_idle_link_train(intel_dp);
> >   
> > -	intel_dp->DP = DP;
> > -
> >   	if (channel_eq) {
> >   		intel_dp->train_set_valid = true;
> >   		DRM_DEBUG_KMS("Channel EQ done. DP Training
> > successful\n");
> > +		return true;
> > +	} else {
> > +		return false;
> >   	}
> >   }
> >   
> >   void intel_dp_stop_link_train(struct intel_dp *intel_dp)
> >   {
> > -	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> > +	intel_dp_set_link_train(intel_dp,
> >   				DP_TRAINING_PATTERN_DISABLE);
> >   }
> >   
> >   void
> >   intel_dp_start_link_train(struct intel_dp *intel_dp)
> >   {
> > -	intel_dp_link_training_clock_recovery(intel_dp);
> > -	intel_dp_link_training_channel_equalization(intel_dp);
> > +	uint32_t DP = intel_dp->DP;
> > +
> > +	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
> > +	    !intel_dp_link_training_channel_equalization(intel_dp))
> > +		intel_dp->DP = DP;
> it is wrong to restore the value of DP here, we have modified the value 
> of port/ddi already inside the two functions.
> if either of these two steps fail we should call stop link training and 
> follow it with bspec disable sequence.
> so saving and restoring will not help us in anyway but more hide the 
> real status of HW.
> >   }
> >   
> >   static void
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions

[Thulasimani, Sivakumar]

On 10/19/2015 2:26 PM, Ander Conselvan De Oliveira wrote:
> On Mon, 2015-10-19 at 10:15 +0530, Thulasimani, Sivakumar wrote:
>> On 10/5/2015 12:31 PM, Ander Conselvan de Oliveira wrote:
>>> It just makes the code more confusing, so just reference intel_dp_>DP
>>> directly. The old behavior of not updating the value in intel_dp if link
>>> training fail is preserved by saving the previous value of DP in the
>>> stack and restoring the old value in case of failure.
>>>
>>> Signed-off-by: Ander Conselvan de Oliveira <
>>> ander.conselvan.de.oliveira@intel.com>
>>> --
>>>
>>> I'm not sure the old behavior is correct, but to err in the side of
>>> caution I tried not to change it.
>>>
>>> ---
>>>    drivers/gpu/drm/i915/intel_dp.c | 66 ++++++++++++++++++++-----------------
>>> ----
>>>    1 file changed, 33 insertions(+), 33 deletions(-)
>>>
>>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>>> b/drivers/gpu/drm/i915/intel_dp.c
>>> index c420edf..391a367 100644
>>> --- a/drivers/gpu/drm/i915/intel_dp.c
>>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>>> @@ -3601,7 +3601,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp,
>>> uint32_t *DP)
>>>    
>>>    static bool
>>>    intel_dp_set_link_train(struct intel_dp *intel_dp,
>>> -			uint32_t *DP,
>>>    			uint8_t dp_train_pat)
>>>    {
>>>    	struct intel_digital_port *intel_dig_port =
>>> dp_to_dig_port(intel_dp);
>>> @@ -3610,9 +3609,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>>>    	uint8_t buf[sizeof(intel_dp->train_set) + 1];
>>>    	int ret, len;
>>>    
>>> -	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
>>> +	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
>>>    
>>> -	I915_WRITE(intel_dp->output_reg, *DP);
>>> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>>>    	POSTING_READ(intel_dp->output_reg);
>>>    
>>>    	buf[0] = dp_train_pat;
>>> @@ -3633,17 +3632,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>>>    }
>>>    
>>>    static bool
>>> -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>>> +intel_dp_reset_link_train(struct intel_dp *intel_dp,
>>>    			uint8_t dp_train_pat)
>>>    {
>>>    	if (!intel_dp->train_set_valid)
>>>    		memset(intel_dp->train_set, 0, sizeof(intel_dp
>>> ->train_set));
>>> -	intel_dp_set_signal_levels(intel_dp, DP);
>>> -	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
>>> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
>>> +	return intel_dp_set_link_train(intel_dp, dp_train_pat);
>>>    }
>>>    
>>>    static bool
>>> -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>>> +intel_dp_update_link_train(struct intel_dp *intel_dp,
>>>    			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
>>>    {
>>>    	struct intel_digital_port *intel_dig_port =
>>> dp_to_dig_port(intel_dp);
>>> @@ -3652,9 +3651,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp,
>>> uint32_t *DP,
>>>    	int ret;
>>>    
>>>    	intel_get_adjust_train(intel_dp, link_status);
>>> -	intel_dp_set_signal_levels(intel_dp, DP);
>>> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
>>>    
>>> -	I915_WRITE(intel_dp->output_reg, *DP);
>>> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>>>    	POSTING_READ(intel_dp->output_reg);
>>>    
>>>    	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
>>> @@ -3695,7 +3694,7 @@ static void intel_dp_set_idle_link_train(struct
>>> intel_dp *intel_dp)
>>>    }
>>>    
>>>    /* Enable corresponding port and start training pattern 1 */
>>> -static void
>>> +static bool
>>>    intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>>>    {
>>>    	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)
>>> ->base.base;
>>> @@ -3703,9 +3702,9 @@ intel_dp_link_training_clock_recovery(struct intel_dp
>>> *intel_dp)
>>>    	int i;
>>>    	uint8_t voltage;
>>>    	int voltage_tries, loop_tries;
>>> -	uint32_t DP = intel_dp->DP;
>>>    	uint8_t link_config[2];
>>>    	uint8_t link_bw, rate_select;
>>> +	uint8_t link_status[DP_LINK_STATUS_SIZE];
>>>    
>>>    	if (HAS_DDI(dev))
>>>    		intel_ddi_prepare_link_retrain(encoder);
>>> @@ -3727,22 +3726,20 @@ intel_dp_link_training_clock_recovery(struct
>>> intel_dp *intel_dp)
>>>    	link_config[1] = DP_SET_ANSI_8B10B;
>>>    	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config,
>>> 2);
>>>    
>>> -	DP |= DP_PORT_EN;
>>> +	intel_dp->DP |= DP_PORT_EN;
>>>    
>>>    	/* clock recovery */
>>> -	if (!intel_dp_reset_link_train(intel_dp, &DP,
>>> +	if (!intel_dp_reset_link_train(intel_dp,
>>>    				       DP_TRAINING_PATTERN_1 |
>>>    				       DP_LINK_SCRAMBLING_DISABLE)) {
>>>    		DRM_ERROR("failed to enable link training\n");
>>> -		return;
>>> +		return false;
>>>    	}
>>>    
>>>    	voltage = 0xff;
>>>    	voltage_tries = 0;
>>>    	loop_tries = 0;
>>>    	for (;;) {
>>> -		uint8_t link_status[DP_LINK_STATUS_SIZE];
>>> -
>>>    		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
>>>    		if (!intel_dp_get_link_status(intel_dp, link_status)) {
>>>    			DRM_ERROR("failed to get link status\n");
>>> @@ -3762,11 +3759,11 @@ intel_dp_link_training_clock_recovery(struct
>>> intel_dp *intel_dp)
>>>    			DRM_DEBUG_KMS("clock recovery not ok, reset");
>>>    			/* clear the flag as we are not reusing train set
>>> */
>>>    			intel_dp->train_set_valid = false;
>>> -			if (!intel_dp_reset_link_train(intel_dp, &DP,
>>> +			if (!intel_dp_reset_link_train(intel_dp,
>>>    						
>>>   DP_TRAINING_PATTERN_1 |
>>>    						
>>>   DP_LINK_SCRAMBLING_DISABLE)) {
>>>    				DRM_ERROR("failed to enable link
>>> training\n");
>>> -				return;
>>> +				return false;
>>>    			}
>>>    			continue;
>>>    		}
>>> @@ -3781,7 +3778,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp
>>> *intel_dp)
>>>    				DRM_ERROR("too many full retries, give
>>> up\n");
>>>    				break;
>>>    			}
>>> -			intel_dp_reset_link_train(intel_dp, &DP,
>>> +			intel_dp_reset_link_train(intel_dp,
>>>    						  DP_TRAINING_PATTERN_1 |
>>>    						
>>>   DP_LINK_SCRAMBLING_DISABLE);
>>>    			voltage_tries = 0;
>>> @@ -3800,23 +3797,22 @@ intel_dp_link_training_clock_recovery(struct
>>> intel_dp *intel_dp)
>>>    		voltage = intel_dp->train_set[0] &
>>> DP_TRAIN_VOLTAGE_SWING_MASK;
>>>    
>>>    		/* Update training set as requested by target */
>>> -		if (!intel_dp_update_link_train(intel_dp, &DP,
>>> link_status)) {
>>> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>>>    			DRM_ERROR("failed to update link training\n");
>>>    			break;
>>>    		}
>>>    	}
>>>    
>>> -	intel_dp->DP = DP;
>>> +	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
>> why are we calling the same function again? in best case this function
>> is called and returned true,
>>    or worst case it was never called. so it will be simpler if we store
>> the return value of this function
>> inside the loop and return that here ?
> I missed this comment earlier. I don't think calling drm_dp_clock_recovery_ok()
> would have a big impact, since it is a very simple function. But I can add a
> variable if that is preferred.
>
> Ander
Agree it won't be costly operation but code would be simpler if this is 
called only once.
since we maintain a variable for equalization in the other function 
doing the same
for here will keep it inline.

regards,
Sivakumar
>
>>>    }
>>>    
>>> -static void
>>> +static bool
>>>    intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>>>    {
>>>    	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
>>>    	struct drm_device *dev = dig_port->base.base.dev;
>>>    	bool channel_eq = false;
>>>    	int tries, cr_tries;
>>> -	uint32_t DP = intel_dp->DP;
>>>    	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
>>>    
>>>    	/*
>>> @@ -3835,11 +3831,11 @@ intel_dp_link_training_channel_equalization(struct
>>> intel_dp *intel_dp)
>>>    		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3
>>> support\n");
>>>    
>>>    	/* channel equalization */
>>> -	if (!intel_dp_set_link_train(intel_dp, &DP,
>>> +	if (!intel_dp_set_link_train(intel_dp,
>>>    				     training_pattern |
>>>    				     DP_LINK_SCRAMBLING_DISABLE)) {
>>>    		DRM_ERROR("failed to start channel equalization\n");
>>> -		return;
>>> +		return false;
>>>    	}
>>>    
>>>    	tries = 0;
>>> @@ -3864,7 +3860,7 @@ intel_dp_link_training_channel_equalization(struct
>>> intel_dp *intel_dp)
>>>    					      intel_dp->lane_count)) {
>>>    			intel_dp->train_set_valid = false;
>>>    			intel_dp_link_training_clock_recovery(intel_dp);
>>> -			intel_dp_set_link_train(intel_dp, &DP,
>>> +			intel_dp_set_link_train(intel_dp,
>>>    						training_pattern |
>>>    						DP_LINK_SCRAMBLING_DISABLE
>>> );
>>>    			cr_tries++;
>>> @@ -3881,7 +3877,7 @@ intel_dp_link_training_channel_equalization(struct
>>> intel_dp *intel_dp)
>>>    		if (tries > 5) {
>>>    			intel_dp->train_set_valid = false;
>>>    			intel_dp_link_training_clock_recovery(intel_dp);
>>> -			intel_dp_set_link_train(intel_dp, &DP,
>>> +			intel_dp_set_link_train(intel_dp,
>>>    						training_pattern |
>>>    						DP_LINK_SCRAMBLING_DISABLE
>>> );
>>>    			tries = 0;
>>> @@ -3890,7 +3886,7 @@ intel_dp_link_training_channel_equalization(struct
>>> intel_dp *intel_dp)
>>>    		}
>>>    
>>>    		/* Update training set as requested by target */
>>> -		if (!intel_dp_update_link_train(intel_dp, &DP,
>>> link_status)) {
>>> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>>>    			DRM_ERROR("failed to update link training\n");
>>>    			break;
>>>    		}
>>> @@ -3899,25 +3895,29 @@ intel_dp_link_training_channel_equalization(struct
>>> intel_dp *intel_dp)
>>>    
>>>    	intel_dp_set_idle_link_train(intel_dp);
>>>    
>>> -	intel_dp->DP = DP;
>>> -
>>>    	if (channel_eq) {
>>>    		intel_dp->train_set_valid = true;
>>>    		DRM_DEBUG_KMS("Channel EQ done. DP Training
>>> successful\n");
>>> +		return true;
>>> +	} else {
>>> +		return false;
>>>    	}
>>>    }
>>>    
>>>    void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>>>    {
>>> -	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
>>> +	intel_dp_set_link_train(intel_dp,
>>>    				DP_TRAINING_PATTERN_DISABLE);
>>>    }
>>>    
>>>    void
>>>    intel_dp_start_link_train(struct intel_dp *intel_dp)
>>>    {
>>> -	intel_dp_link_training_clock_recovery(intel_dp);
>>> -	intel_dp_link_training_channel_equalization(intel_dp);
>>> +	uint32_t DP = intel_dp->DP;
>>> +
>>> +	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
>>> +	    !intel_dp_link_training_channel_equalization(intel_dp))
>>> +		intel_dp->DP = DP;
>> it is wrong to restore the value of DP here, we have modified the value
>> of port/ddi already inside the two functions.
>> if either of these two steps fail we should call stop link training and
>> follow it with bspec disable sequence.
>> so saving and restoring will not help us in anyway but more hide the
>> real status of HW.
>>>    }
>>>    
>>>    static void
>> _______________________________________________
>> Intel-gfx mailing list
>> Intel-gfx@lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

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

[PATCH v2] drm/i915: Don't pass *DP around to link training functions

[Ander Conselvan de Oliveira]

It just makes the code more confusing, so just reference intel_dp->DP
directly.

Note that this also fix a bug where the value of intel_dp->DP could be
different than the last value written to the hw, due to an early return
that would skip the 'intel_dp->DP = DP' line.

v2: Don't preserve old DP value on failure. (Sivakumar)
  - Don't call drm_dp_clock_recovery_ok() twice. (Sivakumar)
  - Keep return type of clock recovery and channel equalization
    functions as void. (Ander)

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 43 +++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 8287df4..f310dab 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3596,7 +3596,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 
 static bool
 intel_dp_set_link_train(struct intel_dp *intel_dp,
-			uint32_t *DP,
 			uint8_t dp_train_pat)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -3605,9 +3604,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 	uint8_t buf[sizeof(intel_dp->train_set) + 1];
 	int ret, len;
 
-	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
+	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
 
-	I915_WRITE(intel_dp->output_reg, *DP);
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
 
 	buf[0] = dp_train_pat;
@@ -3628,17 +3627,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
 }
 
 static bool
-intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
+intel_dp_reset_link_train(struct intel_dp *intel_dp,
 			uint8_t dp_train_pat)
 {
 	if (!intel_dp->train_set_valid)
 		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
-	intel_dp_set_signal_levels(intel_dp, DP);
-	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
+	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
+	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
 
 static bool
-intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
+intel_dp_update_link_train(struct intel_dp *intel_dp,
 			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -3647,9 +3646,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
 	int ret;
 
 	intel_get_adjust_train(intel_dp, link_status);
-	intel_dp_set_signal_levels(intel_dp, DP);
+	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
 
-	I915_WRITE(intel_dp->output_reg, *DP);
+	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
 
 	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
@@ -3698,7 +3697,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	int i;
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
-	uint32_t DP = intel_dp->DP;
 	uint8_t link_config[2];
 	uint8_t link_bw, rate_select;
 
@@ -3722,10 +3720,10 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	link_config[1] = DP_SET_ANSI_8B10B;
 	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
 
-	DP |= DP_PORT_EN;
+	intel_dp->DP |= DP_PORT_EN;
 
 	/* clock recovery */
-	if (!intel_dp_reset_link_train(intel_dp, &DP,
+	if (!intel_dp_reset_link_train(intel_dp,
 				       DP_TRAINING_PATTERN_1 |
 				       DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to enable link training\n");
@@ -3757,7 +3755,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 			DRM_DEBUG_KMS("clock recovery not ok, reset");
 			/* clear the flag as we are not reusing train set */
 			intel_dp->train_set_valid = false;
-			if (!intel_dp_reset_link_train(intel_dp, &DP,
+			if (!intel_dp_reset_link_train(intel_dp,
 						       DP_TRAINING_PATTERN_1 |
 						       DP_LINK_SCRAMBLING_DISABLE)) {
 				DRM_ERROR("failed to enable link training\n");
@@ -3776,7 +3774,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 				DRM_ERROR("too many full retries, give up\n");
 				break;
 			}
-			intel_dp_reset_link_train(intel_dp, &DP,
+			intel_dp_reset_link_train(intel_dp,
 						  DP_TRAINING_PATTERN_1 |
 						  DP_LINK_SCRAMBLING_DISABLE);
 			voltage_tries = 0;
@@ -3795,13 +3793,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+		if (!intel_dp_update_link_train(intel_dp, link_status)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
 	}
-
-	intel_dp->DP = DP;
 }
 
 static void
@@ -3811,7 +3807,6 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	struct drm_device *dev = dig_port->base.base.dev;
 	bool channel_eq = false;
 	int tries, cr_tries;
-	uint32_t DP = intel_dp->DP;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
 
 	/*
@@ -3830,7 +3825,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
 
 	/* channel equalization */
-	if (!intel_dp_set_link_train(intel_dp, &DP,
+	if (!intel_dp_set_link_train(intel_dp,
 				     training_pattern |
 				     DP_LINK_SCRAMBLING_DISABLE)) {
 		DRM_ERROR("failed to start channel equalization\n");
@@ -3859,7 +3854,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 					      intel_dp->lane_count)) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp, &DP,
+			intel_dp_set_link_train(intel_dp,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
 			cr_tries++;
@@ -3876,7 +3871,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		if (tries > 5) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp, &DP,
+			intel_dp_set_link_train(intel_dp,
 						training_pattern |
 						DP_LINK_SCRAMBLING_DISABLE);
 			tries = 0;
@@ -3885,7 +3880,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		}
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
+		if (!intel_dp_update_link_train(intel_dp, link_status)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
@@ -3894,8 +3889,6 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 
 	intel_dp_set_idle_link_train(intel_dp);
 
-	intel_dp->DP = DP;
-
 	if (channel_eq) {
 		intel_dp->train_set_valid = true;
 		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
@@ -3904,7 +3897,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 
 void intel_dp_stop_link_train(struct intel_dp *intel_dp)
 {
-	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
+	intel_dp_set_link_train(intel_dp,
 				DP_TRAINING_PATTERN_DISABLE);
 }
 
-- 
2.4.3

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

Re: [PATCH v2] drm/i915: Don't pass *DP around to link training functions

[Thulasimani, Sivakumar]

On 10/21/2015 7:22 PM, Ander Conselvan de Oliveira wrote:
> It just makes the code more confusing, so just reference intel_dp->DP
> directly.
>
> Note that this also fix a bug where the value of intel_dp->DP could be
> different than the last value written to the hw, due to an early return
> that would skip the 'intel_dp->DP = DP' line.
>
> v2: Don't preserve old DP value on failure. (Sivakumar)
>    - Don't call drm_dp_clock_recovery_ok() twice. (Sivakumar)
>    - Keep return type of clock recovery and channel equalization
>      functions as void. (Ander)
>
> Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
> ---
>   drivers/gpu/drm/i915/intel_dp.c | 43 +++++++++++++++++------------------------
>   1 file changed, 18 insertions(+), 25 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 8287df4..f310dab 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3596,7 +3596,6 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
>   
>   static bool
>   intel_dp_set_link_train(struct intel_dp *intel_dp,
> -			uint32_t *DP,
>   			uint8_t dp_train_pat)
>   {
>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -3605,9 +3604,9 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>   	uint8_t buf[sizeof(intel_dp->train_set) + 1];
>   	int ret, len;
>   
> -	_intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> +	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
>   
> -	I915_WRITE(intel_dp->output_reg, *DP);
> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>   	POSTING_READ(intel_dp->output_reg);
>   
>   	buf[0] = dp_train_pat;
> @@ -3628,17 +3627,17 @@ intel_dp_set_link_train(struct intel_dp *intel_dp,
>   }
>   
>   static bool
> -intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> +intel_dp_reset_link_train(struct intel_dp *intel_dp,
>   			uint8_t dp_train_pat)
>   {
>   	if (!intel_dp->train_set_valid)
>   		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
> -	intel_dp_set_signal_levels(intel_dp, DP);
> -	return intel_dp_set_link_train(intel_dp, DP, dp_train_pat);
> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
can this be removed as well ? we are already passing intel_dp so same as 
other places.
> +	return intel_dp_set_link_train(intel_dp, dp_train_pat);
>   }
>   
>   static bool
> -intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
> +intel_dp_update_link_train(struct intel_dp *intel_dp,
>   			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
>   {
>   	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
> @@ -3647,9 +3646,9 @@ intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP,
>   	int ret;
>   
>   	intel_get_adjust_train(intel_dp, link_status);
> -	intel_dp_set_signal_levels(intel_dp, DP);
> +	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
>   
> -	I915_WRITE(intel_dp->output_reg, *DP);
> +	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
>   	POSTING_READ(intel_dp->output_reg);
>   
>   	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
> @@ -3698,7 +3697,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   	int i;
>   	uint8_t voltage;
>   	int voltage_tries, loop_tries;
> -	uint32_t DP = intel_dp->DP;
>   	uint8_t link_config[2];
>   	uint8_t link_bw, rate_select;
>   
> @@ -3722,10 +3720,10 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   	link_config[1] = DP_SET_ANSI_8B10B;
>   	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
>   
> -	DP |= DP_PORT_EN;
> +	intel_dp->DP |= DP_PORT_EN;
>   
>   	/* clock recovery */
> -	if (!intel_dp_reset_link_train(intel_dp, &DP,
> +	if (!intel_dp_reset_link_train(intel_dp,
>   				       DP_TRAINING_PATTERN_1 |
>   				       DP_LINK_SCRAMBLING_DISABLE)) {
>   		DRM_ERROR("failed to enable link training\n");
> @@ -3757,7 +3755,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   			DRM_DEBUG_KMS("clock recovery not ok, reset");
>   			/* clear the flag as we are not reusing train set */
>   			intel_dp->train_set_valid = false;
> -			if (!intel_dp_reset_link_train(intel_dp, &DP,
> +			if (!intel_dp_reset_link_train(intel_dp,
>   						       DP_TRAINING_PATTERN_1 |
>   						       DP_LINK_SCRAMBLING_DISABLE)) {
>   				DRM_ERROR("failed to enable link training\n");
> @@ -3776,7 +3774,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   				DRM_ERROR("too many full retries, give up\n");
>   				break;
>   			}
> -			intel_dp_reset_link_train(intel_dp, &DP,
> +			intel_dp_reset_link_train(intel_dp,
>   						  DP_TRAINING_PATTERN_1 |
>   						  DP_LINK_SCRAMBLING_DISABLE);
>   			voltage_tries = 0;
> @@ -3795,13 +3793,11 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
>   		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
>   
>   		/* Update training set as requested by target */
> -		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>   			DRM_ERROR("failed to update link training\n");
>   			break;
>   		}
>   	}
> -
> -	intel_dp->DP = DP;
>   }
>   
>   static void
> @@ -3811,7 +3807,6 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   	struct drm_device *dev = dig_port->base.base.dev;
>   	bool channel_eq = false;
>   	int tries, cr_tries;
> -	uint32_t DP = intel_dp->DP;
>   	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
>   
>   	/*
> @@ -3830,7 +3825,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
>   
>   	/* channel equalization */
> -	if (!intel_dp_set_link_train(intel_dp, &DP,
> +	if (!intel_dp_set_link_train(intel_dp,
>   				     training_pattern |
>   				     DP_LINK_SCRAMBLING_DISABLE)) {
>   		DRM_ERROR("failed to start channel equalization\n");
> @@ -3859,7 +3854,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   					      intel_dp->lane_count)) {
>   			intel_dp->train_set_valid = false;
>   			intel_dp_link_training_clock_recovery(intel_dp);
> -			intel_dp_set_link_train(intel_dp, &DP,
> +			intel_dp_set_link_train(intel_dp,
>   						training_pattern |
>   						DP_LINK_SCRAMBLING_DISABLE);
>   			cr_tries++;
> @@ -3876,7 +3871,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		if (tries > 5) {
>   			intel_dp->train_set_valid = false;
>   			intel_dp_link_training_clock_recovery(intel_dp);
> -			intel_dp_set_link_train(intel_dp, &DP,
> +			intel_dp_set_link_train(intel_dp,
>   						training_pattern |
>   						DP_LINK_SCRAMBLING_DISABLE);
>   			tries = 0;
> @@ -3885,7 +3880,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   		}
>   
>   		/* Update training set as requested by target */
> -		if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) {
> +		if (!intel_dp_update_link_train(intel_dp, link_status)) {
>   			DRM_ERROR("failed to update link training\n");
>   			break;
>   		}
> @@ -3894,8 +3889,6 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   
>   	intel_dp_set_idle_link_train(intel_dp);
>   
> -	intel_dp->DP = DP;
> -
>   	if (channel_eq) {
>   		intel_dp->train_set_valid = true;
>   		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
> @@ -3904,7 +3897,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
>   
>   void intel_dp_stop_link_train(struct intel_dp *intel_dp)
>   {
> -	intel_dp_set_link_train(intel_dp, &intel_dp->DP,
> +	intel_dp_set_link_train(intel_dp,
>   				DP_TRAINING_PATTERN_DISABLE);
>   }
>   

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

[PATCH 03/15] drm/i915: Split intel_dp_update_link_train()

[Ander Conselvan de Oliveira]

Separate the bit that writes to DP register in its own function and move
the update of the train_set based on the sink adjust request out. The
objective is to split the generic link training logic from the i915
specific part.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 21 ++++++++++++++-------
 1 file changed, 14 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 391a367..027fce7 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3641,20 +3641,25 @@ intel_dp_reset_link_train(struct intel_dp *intel_dp,
 	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
 
-static bool
-intel_dp_update_link_train(struct intel_dp *intel_dp,
-			   const uint8_t link_status[DP_LINK_STATUS_SIZE])
+static void
+intel_dp_update_signal_levels(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);
-	int ret;
 
-	intel_get_adjust_train(intel_dp, link_status);
 	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
 
 	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
+}
+
+static bool
+intel_dp_update_link_train(struct intel_dp *intel_dp)
+{
+	int ret;
+
+	intel_dp_update_signal_levels(intel_dp);
 
 	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
 				intel_dp->train_set, intel_dp->lane_count);
@@ -3797,7 +3802,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, link_status)) {
+		intel_get_adjust_train(intel_dp, link_status);
+		if (!intel_dp_update_link_train(intel_dp)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
@@ -3886,7 +3892,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		}
 
 		/* Update training set as requested by target */
-		if (!intel_dp_update_link_train(intel_dp, link_status)) {
+		intel_get_adjust_train(intel_dp, link_status);
+		if (!intel_dp_update_link_train(intel_dp)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
 		}
-- 
2.4.3

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

[PATCH 04/15] drm/i915: Split write of pattern to DP reg from intel_dp_set_link_train

[Ander Conselvan de Oliveira]

Split the register write with the new link training pattern out of
intel_dp_set_link_train(), so that the i915 specific code is in a
separate function.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 027fce7..b04fef0 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3599,20 +3599,28 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	*DP = (*DP & ~mask) | signal_levels;
 }
 
-static bool
-intel_dp_set_link_train(struct intel_dp *intel_dp,
-			uint8_t dp_train_pat)
+static void
+intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
+				       uint8_t dp_train_pat)
 {
 	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);
-	uint8_t buf[sizeof(intel_dp->train_set) + 1];
-	int ret, len;
 
 	_intel_dp_set_link_train(intel_dp, &intel_dp->DP, dp_train_pat);
 
 	I915_WRITE(intel_dp->output_reg, intel_dp->DP);
 	POSTING_READ(intel_dp->output_reg);
+}
+
+static bool
+intel_dp_set_link_train(struct intel_dp *intel_dp,
+			uint8_t dp_train_pat)
+{
+	uint8_t buf[sizeof(intel_dp->train_set) + 1];
+	int ret, len;
+
+	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
 
 	buf[0] = dp_train_pat;
 	if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
-- 
2.4.3

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

[PATCH 05/15] drm/i915: Don't call intel_dp_set_signal_levels() on link train reset

[Ander Conselvan de Oliveira]

Instead of calling intel_dp_set_signal_levels(), which writes the
desired signal levels mask to a variable, just call the new function
intel_dp_update_signal_levels(). The effect should be the same, except
for an extra register write, but this creates a better split between the
generic link training logic and the i915 specific part.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index b04fef0..4653787 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3645,7 +3645,7 @@ intel_dp_reset_link_train(struct intel_dp *intel_dp,
 {
 	if (!intel_dp->train_set_valid)
 		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
-	intel_dp_set_signal_levels(intel_dp, &intel_dp->DP);
+	intel_dp_update_signal_levels(intel_dp);
 	return intel_dp_set_link_train(intel_dp, dp_train_pat);
 }
 
-- 
2.4.3

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

[PATCH 06/15] drm/i915: Move generic link training code to a separate file

[Ander Conselvan de Oliveira]

No functional changes, just moving code around.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/Makefile                 |   1 +
 drivers/gpu/drm/i915/intel_dp.c               | 328 +------------------------
 drivers/gpu/drm/i915/intel_dp_link_training.c | 336 ++++++++++++++++++++++++++
 drivers/gpu/drm/i915/intel_drv.h              |  16 ++
 4 files changed, 362 insertions(+), 319 deletions(-)
 create mode 100644 drivers/gpu/drm/i915/intel_dp_link_training.c

diff --git a/drivers/gpu/drm/i915/Makefile b/drivers/gpu/drm/i915/Makefile
index 44d290a..0851de07 100644
--- a/drivers/gpu/drm/i915/Makefile
+++ b/drivers/gpu/drm/i915/Makefile
@@ -77,6 +77,7 @@ i915-y += dvo_ch7017.o \
 	  dvo_tfp410.o \
 	  intel_crt.o \
 	  intel_ddi.o \
+	  intel_dp_link_training.o \
 	  intel_dp_mst.o \
 	  intel_dp.o \
 	  intel_dsi.o \
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 4653787..619b422 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1189,7 +1189,7 @@ intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
 	return (intel_dp_max_link_bw(intel_dp) >> 3) + 1;
 }
 
-static bool intel_dp_source_supports_hbr2(struct drm_device *dev)
+bool intel_dp_source_supports_hbr2(struct drm_device *dev)
 {
 	/* WaDisableHBR2:skl */
 	if (IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0)
@@ -1365,8 +1365,8 @@ int intel_dp_rate_select(struct intel_dp *intel_dp, int rate)
 	return rate_to_index(rate, intel_dp->sink_rates);
 }
 
-static void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
-				  uint8_t *link_bw, uint8_t *rate_select)
+void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
+			   uint8_t *link_bw, uint8_t *rate_select)
 {
 	if (intel_dp->num_sink_rates) {
 		*link_bw = 0;
@@ -3046,7 +3046,7 @@ intel_dp_dpcd_read_wake(struct drm_dp_aux *aux, unsigned int offset,
  * Fetch AUX CH registers 0x202 - 0x207 which contain
  * link status information
  */
-static bool
+bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
 	return intel_dp_dpcd_read_wake(&intel_dp->aux,
@@ -3056,7 +3056,7 @@ intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_
 }
 
 /* These are source-specific values. */
-static uint8_t
+uint8_t
 intel_dp_voltage_max(struct intel_dp *intel_dp)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -3079,7 +3079,7 @@ intel_dp_voltage_max(struct intel_dp *intel_dp)
 		return DP_TRAIN_VOLTAGE_SWING_LEVEL_2;
 }
 
-static uint8_t
+uint8_t
 intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing)
 {
 	struct drm_device *dev = intel_dp_to_dev(intel_dp);
@@ -3426,38 +3426,6 @@ static uint32_t chv_signal_levels(struct intel_dp *intel_dp)
 	return 0;
 }
 
-static void
-intel_get_adjust_train(struct intel_dp *intel_dp,
-		       const uint8_t link_status[DP_LINK_STATUS_SIZE])
-{
-	uint8_t v = 0;
-	uint8_t p = 0;
-	int lane;
-	uint8_t voltage_max;
-	uint8_t preemph_max;
-
-	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);
-
-		if (this_v > v)
-			v = this_v;
-		if (this_p > p)
-			p = this_p;
-	}
-
-	voltage_max = intel_dp_voltage_max(intel_dp);
-	if (v >= voltage_max)
-		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
-
-	preemph_max = intel_dp_pre_emphasis_max(intel_dp, v);
-	if (p >= preemph_max)
-		p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
-
-	for (lane = 0; lane < 4; lane++)
-		intel_dp->train_set[lane] = v | p;
-}
-
 static uint32_t
 gen4_signal_levels(uint8_t train_set)
 {
@@ -3599,7 +3567,7 @@ intel_dp_set_signal_levels(struct intel_dp *intel_dp, uint32_t *DP)
 	*DP = (*DP & ~mask) | signal_levels;
 }
 
-static void
+void
 intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
 				       uint8_t dp_train_pat)
 {
@@ -3613,43 +3581,7 @@ intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
 	POSTING_READ(intel_dp->output_reg);
 }
 
-static bool
-intel_dp_set_link_train(struct intel_dp *intel_dp,
-			uint8_t dp_train_pat)
-{
-	uint8_t buf[sizeof(intel_dp->train_set) + 1];
-	int ret, len;
-
-	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
-
-	buf[0] = dp_train_pat;
-	if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
-	    DP_TRAINING_PATTERN_DISABLE) {
-		/* don't write DP_TRAINING_LANEx_SET on disable */
-		len = 1;
-	} else {
-		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
-		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,
-				buf, len);
-
-	return ret == len;
-}
-
-static bool
-intel_dp_reset_link_train(struct intel_dp *intel_dp,
-			uint8_t dp_train_pat)
-{
-	if (!intel_dp->train_set_valid)
-		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
-	intel_dp_update_signal_levels(intel_dp);
-	return intel_dp_set_link_train(intel_dp, dp_train_pat);
-}
-
-static void
+void
 intel_dp_update_signal_levels(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
@@ -3662,20 +3594,7 @@ intel_dp_update_signal_levels(struct intel_dp *intel_dp)
 	POSTING_READ(intel_dp->output_reg);
 }
 
-static bool
-intel_dp_update_link_train(struct intel_dp *intel_dp)
-{
-	int ret;
-
-	intel_dp_update_signal_levels(intel_dp);
-
-	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
-				intel_dp->train_set, intel_dp->lane_count);
-
-	return ret == intel_dp->lane_count;
-}
-
-static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 {
 	struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp);
 	struct drm_device *dev = intel_dig_port->base.base.dev;
@@ -3706,235 +3625,6 @@ static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp)
 		DRM_ERROR("Timed out waiting for DP idle patterns\n");
 }
 
-/* Enable corresponding port and start training pattern 1 */
-static bool
-intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
-{
-	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
-	struct drm_device *dev = encoder->dev;
-	int i;
-	uint8_t voltage;
-	int voltage_tries, loop_tries;
-	uint8_t link_config[2];
-	uint8_t link_bw, rate_select;
-	uint8_t link_status[DP_LINK_STATUS_SIZE];
-
-	if (HAS_DDI(dev))
-		intel_ddi_prepare_link_retrain(encoder);
-
-	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] = 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);
-	if (intel_dp->num_sink_rates)
-		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
-				  &rate_select, 1);
-
-	link_config[0] = 0;
-	link_config[1] = DP_SET_ANSI_8B10B;
-	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
-
-	intel_dp->DP |= DP_PORT_EN;
-
-	/* clock recovery */
-	if (!intel_dp_reset_link_train(intel_dp,
-				       DP_TRAINING_PATTERN_1 |
-				       DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to enable link training\n");
-		return false;
-	}
-
-	voltage = 0xff;
-	voltage_tries = 0;
-	loop_tries = 0;
-	for (;;) {
-		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
-			DRM_ERROR("failed to get link status\n");
-			break;
-		}
-
-		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
-			DRM_DEBUG_KMS("clock recovery OK\n");
-			break;
-		}
-
-		/*
-		 * if we used previously trained voltage and pre-emphasis values
-		 * and we don't get clock recovery, reset link training values
-		 */
-		if (intel_dp->train_set_valid) {
-			DRM_DEBUG_KMS("clock recovery not ok, reset");
-			/* clear the flag as we are not reusing train set */
-			intel_dp->train_set_valid = false;
-			if (!intel_dp_reset_link_train(intel_dp,
-						       DP_TRAINING_PATTERN_1 |
-						       DP_LINK_SCRAMBLING_DISABLE)) {
-				DRM_ERROR("failed to enable link training\n");
-				return false;
-			}
-			continue;
-		}
-
-		/* Check to see if we've tried the max voltage */
-		for (i = 0; i < intel_dp->lane_count; i++)
-			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
-				break;
-		if (i == intel_dp->lane_count) {
-			++loop_tries;
-			if (loop_tries == 5) {
-				DRM_ERROR("too many full retries, give up\n");
-				break;
-			}
-			intel_dp_reset_link_train(intel_dp,
-						  DP_TRAINING_PATTERN_1 |
-						  DP_LINK_SCRAMBLING_DISABLE);
-			voltage_tries = 0;
-			continue;
-		}
-
-		/* Check to see if we've tried the same voltage 5 times */
-		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
-			++voltage_tries;
-			if (voltage_tries == 5) {
-				DRM_ERROR("too many voltage retries, give up\n");
-				break;
-			}
-		} else
-			voltage_tries = 0;
-		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
-
-		/* Update training set as requested by target */
-		intel_get_adjust_train(intel_dp, link_status);
-		if (!intel_dp_update_link_train(intel_dp)) {
-			DRM_ERROR("failed to update link training\n");
-			break;
-		}
-	}
-
-	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
-}
-
-static bool
-intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
-{
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = dig_port->base.base.dev;
-	bool channel_eq = false;
-	int tries, cr_tries;
-	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
-
-	/*
-	 * Training Pattern 3 for HBR2 or 1.2 devices that support it.
-	 *
-	 * Intel platforms that support HBR2 also support TPS3. TPS3 support is
-	 * also mandatory for downstream devices that support HBR2.
-	 *
-	 * Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is
-	 * supported but still not enabled.
-	 */
-	if (intel_dp_source_supports_hbr2(dev) &&
-	    drm_dp_tps3_supported(intel_dp->dpcd))
-		training_pattern = DP_TRAINING_PATTERN_3;
-	else if (intel_dp->link_rate == 540000)
-		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
-
-	/* channel equalization */
-	if (!intel_dp_set_link_train(intel_dp,
-				     training_pattern |
-				     DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to start channel equalization\n");
-		return false;
-	}
-
-	tries = 0;
-	cr_tries = 0;
-	channel_eq = false;
-	for (;;) {
-		uint8_t link_status[DP_LINK_STATUS_SIZE];
-
-		if (cr_tries > 5) {
-			DRM_ERROR("failed to train DP, aborting\n");
-			break;
-		}
-
-		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
-		if (!intel_dp_get_link_status(intel_dp, link_status)) {
-			DRM_ERROR("failed to get link status\n");
-			break;
-		}
-
-		/* Make sure clock is still ok */
-		if (!drm_dp_clock_recovery_ok(link_status,
-					      intel_dp->lane_count)) {
-			intel_dp->train_set_valid = false;
-			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp,
-						training_pattern |
-						DP_LINK_SCRAMBLING_DISABLE);
-			cr_tries++;
-			continue;
-		}
-
-		if (drm_dp_channel_eq_ok(link_status,
-					 intel_dp->lane_count)) {
-			channel_eq = true;
-			break;
-		}
-
-		/* Try 5 times, then try clock recovery if that fails */
-		if (tries > 5) {
-			intel_dp->train_set_valid = false;
-			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp,
-						training_pattern |
-						DP_LINK_SCRAMBLING_DISABLE);
-			tries = 0;
-			cr_tries++;
-			continue;
-		}
-
-		/* Update training set as requested by target */
-		intel_get_adjust_train(intel_dp, link_status);
-		if (!intel_dp_update_link_train(intel_dp)) {
-			DRM_ERROR("failed to update link training\n");
-			break;
-		}
-		++tries;
-	}
-
-	intel_dp_set_idle_link_train(intel_dp);
-
-	if (channel_eq) {
-		intel_dp->train_set_valid = true;
-		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
-		return true;
-	} else {
-		return false;
-	}
-}
-
-void intel_dp_stop_link_train(struct intel_dp *intel_dp)
-{
-	intel_dp_set_link_train(intel_dp,
-				DP_TRAINING_PATTERN_DISABLE);
-}
-
-void
-intel_dp_start_link_train(struct intel_dp *intel_dp)
-{
-	uint32_t DP = intel_dp->DP;
-
-	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
-	    !intel_dp_link_training_channel_equalization(intel_dp))
-		intel_dp->DP = DP;
-}
-
 static void
 intel_dp_link_down(struct intel_dp *intel_dp)
 {
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
new file mode 100644
index 0000000..f33cbbb
--- /dev/null
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -0,0 +1,336 @@
+/*
+ * Copyright © 2008-2015 Intel Corporation
+ *
+ * Permission is hereby granted, free of charge, to any person obtaining a
+ * copy of this software and associated documentation files (the "Software"),
+ * to deal in the Software without restriction, including without limitation
+ * the rights to use, copy, modify, merge, publish, distribute, sublicense,
+ * and/or sell copies of the Software, and to permit persons to whom the
+ * Software is furnished to do so, subject to the following conditions:
+ *
+ * The above copyright notice and this permission notice (including the next
+ * paragraph) shall be included in all copies or substantial portions of the
+ * Software.
+ *
+ * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
+ * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
+ * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
+ * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
+ * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
+ * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
+ * IN THE SOFTWARE.
+ */
+
+#include "intel_drv.h"
+
+static void
+intel_get_adjust_train(struct intel_dp *intel_dp,
+		       const uint8_t link_status[DP_LINK_STATUS_SIZE])
+{
+	uint8_t v = 0;
+	uint8_t p = 0;
+	int lane;
+	uint8_t voltage_max;
+	uint8_t preemph_max;
+
+	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);
+
+		if (this_v > v)
+			v = this_v;
+		if (this_p > p)
+			p = this_p;
+	}
+
+	voltage_max = intel_dp_voltage_max(intel_dp);
+	if (v >= voltage_max)
+		v = voltage_max | DP_TRAIN_MAX_SWING_REACHED;
+
+	preemph_max = intel_dp_pre_emphasis_max(intel_dp, v);
+	if (p >= preemph_max)
+		p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
+
+	for (lane = 0; lane < 4; lane++)
+		intel_dp->train_set[lane] = v | p;
+}
+
+static bool
+intel_dp_set_link_train(struct intel_dp *intel_dp,
+			uint8_t dp_train_pat)
+{
+	uint8_t buf[sizeof(intel_dp->train_set) + 1];
+	int ret, len;
+
+	intel_dp_program_link_training_pattern(intel_dp, dp_train_pat);
+
+	buf[0] = dp_train_pat;
+	if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) ==
+	    DP_TRAINING_PATTERN_DISABLE) {
+		/* don't write DP_TRAINING_LANEx_SET on disable */
+		len = 1;
+	} else {
+		/* DP_TRAINING_LANEx_SET follow DP_TRAINING_PATTERN_SET */
+		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,
+				buf, len);
+
+	return ret == len;
+}
+
+static bool
+intel_dp_reset_link_train(struct intel_dp *intel_dp,
+			uint8_t dp_train_pat)
+{
+	if (!intel_dp->train_set_valid)
+		memset(intel_dp->train_set, 0, sizeof(intel_dp->train_set));
+	intel_dp_update_signal_levels(intel_dp);
+	return intel_dp_set_link_train(intel_dp, dp_train_pat);
+}
+
+static bool
+intel_dp_update_link_train(struct intel_dp *intel_dp)
+{
+	int ret;
+
+	intel_dp_update_signal_levels(intel_dp);
+
+	ret = drm_dp_dpcd_write(&intel_dp->aux, DP_TRAINING_LANE0_SET,
+				intel_dp->train_set, intel_dp->lane_count);
+
+	return ret == intel_dp->lane_count;
+}
+
+/* Enable corresponding port and start training pattern 1 */
+static bool
+intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
+{
+	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
+	struct drm_device *dev = encoder->dev;
+	int i;
+	uint8_t voltage;
+	int voltage_tries, loop_tries;
+	uint8_t link_config[2];
+	uint8_t link_bw, rate_select;
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
+
+	if (HAS_DDI(dev))
+		intel_ddi_prepare_link_retrain(encoder);
+
+	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] = 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);
+	if (intel_dp->num_sink_rates)
+		drm_dp_dpcd_write(&intel_dp->aux, DP_LINK_RATE_SET,
+				  &rate_select, 1);
+
+	link_config[0] = 0;
+	link_config[1] = DP_SET_ANSI_8B10B;
+	drm_dp_dpcd_write(&intel_dp->aux, DP_DOWNSPREAD_CTRL, link_config, 2);
+
+	intel_dp->DP |= DP_PORT_EN;
+
+	/* clock recovery */
+	if (!intel_dp_reset_link_train(intel_dp,
+				       DP_TRAINING_PATTERN_1 |
+				       DP_LINK_SCRAMBLING_DISABLE)) {
+		DRM_ERROR("failed to enable link training\n");
+		return false;
+	}
+
+	voltage = 0xff;
+	voltage_tries = 0;
+	loop_tries = 0;
+	for (;;) {
+		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
+		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+			DRM_ERROR("failed to get link status\n");
+			break;
+		}
+
+		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
+			DRM_DEBUG_KMS("clock recovery OK\n");
+			break;
+		}
+
+		/*
+		 * if we used previously trained voltage and pre-emphasis values
+		 * and we don't get clock recovery, reset link training values
+		 */
+		if (intel_dp->train_set_valid) {
+			DRM_DEBUG_KMS("clock recovery not ok, reset");
+			/* clear the flag as we are not reusing train set */
+			intel_dp->train_set_valid = false;
+			if (!intel_dp_reset_link_train(intel_dp,
+						       DP_TRAINING_PATTERN_1 |
+						       DP_LINK_SCRAMBLING_DISABLE)) {
+				DRM_ERROR("failed to enable link training\n");
+				return false;
+			}
+			continue;
+		}
+
+		/* Check to see if we've tried the max voltage */
+		for (i = 0; i < intel_dp->lane_count; i++)
+			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
+				break;
+		if (i == intel_dp->lane_count) {
+			++loop_tries;
+			if (loop_tries == 5) {
+				DRM_ERROR("too many full retries, give up\n");
+				break;
+			}
+			intel_dp_reset_link_train(intel_dp,
+						  DP_TRAINING_PATTERN_1 |
+						  DP_LINK_SCRAMBLING_DISABLE);
+			voltage_tries = 0;
+			continue;
+		}
+
+		/* Check to see if we've tried the same voltage 5 times */
+		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
+			++voltage_tries;
+			if (voltage_tries == 5) {
+				DRM_ERROR("too many voltage retries, give up\n");
+				break;
+			}
+		} else
+			voltage_tries = 0;
+		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
+
+		/* Update training set as requested by target */
+		intel_get_adjust_train(intel_dp, link_status);
+		if (!intel_dp_update_link_train(intel_dp)) {
+			DRM_ERROR("failed to update link training\n");
+			break;
+		}
+	}
+
+	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
+}
+
+static bool
+intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
+{
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+	bool channel_eq = false;
+	int tries, cr_tries;
+	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
+
+	/*
+	 * Training Pattern 3 for HBR2 or 1.2 devices that support it.
+	 *
+	 * Intel platforms that support HBR2 also support TPS3. TPS3 support is
+	 * also mandatory for downstream devices that support HBR2.
+	 *
+	 * Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is
+	 * supported but still not enabled.
+	 */
+	if (intel_dp_source_supports_hbr2(dev) &&
+	    drm_dp_tps3_supported(intel_dp->dpcd))
+		training_pattern = DP_TRAINING_PATTERN_3;
+	else if (intel_dp->link_rate == 540000)
+		DRM_ERROR("5.4 Gbps link rate without HBR2/TPS3 support\n");
+
+	/* channel equalization */
+	if (!intel_dp_set_link_train(intel_dp,
+				     training_pattern |
+				     DP_LINK_SCRAMBLING_DISABLE)) {
+		DRM_ERROR("failed to start channel equalization\n");
+		return false;
+	}
+
+	tries = 0;
+	cr_tries = 0;
+	channel_eq = false;
+	for (;;) {
+		uint8_t link_status[DP_LINK_STATUS_SIZE];
+
+		if (cr_tries > 5) {
+			DRM_ERROR("failed to train DP, aborting\n");
+			break;
+		}
+
+		drm_dp_link_train_channel_eq_delay(intel_dp->dpcd);
+		if (!intel_dp_get_link_status(intel_dp, link_status)) {
+			DRM_ERROR("failed to get link status\n");
+			break;
+		}
+
+		/* Make sure clock is still ok */
+		if (!drm_dp_clock_recovery_ok(link_status,
+					      intel_dp->lane_count)) {
+			intel_dp->train_set_valid = false;
+			intel_dp_link_training_clock_recovery(intel_dp);
+			intel_dp_set_link_train(intel_dp,
+						training_pattern |
+						DP_LINK_SCRAMBLING_DISABLE);
+			cr_tries++;
+			continue;
+		}
+
+		if (drm_dp_channel_eq_ok(link_status,
+					 intel_dp->lane_count)) {
+			channel_eq = true;
+			break;
+		}
+
+		/* Try 5 times, then try clock recovery if that fails */
+		if (tries > 5) {
+			intel_dp->train_set_valid = false;
+			intel_dp_link_training_clock_recovery(intel_dp);
+			intel_dp_set_link_train(intel_dp,
+						training_pattern |
+						DP_LINK_SCRAMBLING_DISABLE);
+			tries = 0;
+			cr_tries++;
+			continue;
+		}
+
+		/* Update training set as requested by target */
+		intel_get_adjust_train(intel_dp, link_status);
+		if (!intel_dp_update_link_train(intel_dp)) {
+			DRM_ERROR("failed to update link training\n");
+			break;
+		}
+		++tries;
+	}
+
+	intel_dp_set_idle_link_train(intel_dp);
+
+	if (channel_eq) {
+		intel_dp->train_set_valid = true;
+		DRM_DEBUG_KMS("Channel EQ done. DP Training successful\n");
+		return true;
+	} else {
+		return false;
+	}
+}
+
+void intel_dp_stop_link_train(struct intel_dp *intel_dp)
+{
+	intel_dp_set_link_train(intel_dp,
+				DP_TRAINING_PATTERN_DISABLE);
+}
+
+void
+intel_dp_start_link_train(struct intel_dp *intel_dp)
+{
+	uint32_t DP = intel_dp->DP;
+
+	if (!intel_dp_link_training_clock_recovery(intel_dp) ||
+	    !intel_dp_link_training_channel_equalization(intel_dp))
+		intel_dp->DP = DP;
+}
+
+
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5366c9e..f6fd359 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1247,6 +1247,22 @@ bool intel_digital_port_connected(struct drm_i915_private *dev_priv,
 					 struct intel_digital_port *port);
 void hsw_dp_set_ddi_pll_sel(struct intel_crtc_state *pipe_config);
 
+void
+intel_dp_program_link_training_pattern(struct intel_dp *intel_dp,
+				       uint8_t dp_train_pat);
+void
+intel_dp_update_signal_levels(struct intel_dp *intel_dp);
+void intel_dp_set_idle_link_train(struct intel_dp *intel_dp);
+uint8_t
+intel_dp_voltage_max(struct intel_dp *intel_dp);
+uint8_t
+intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
+void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
+			   uint8_t *link_bw, uint8_t *rate_select);
+bool intel_dp_source_supports_hbr2(struct drm_device *dev);
+bool
+intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
+
 /* intel_dp_mst.c */
 int intel_dp_mst_encoder_init(struct intel_digital_port *intel_dig_port, int conn_id);
 void intel_dp_mst_encoder_cleanup(struct intel_digital_port *intel_dig_port);
-- 
2.4.3

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

[PATCH 07/15] drm/i915: Create intel_dp->prepare_link_retrain() hook

[Ander Conselvan de Oliveira]

In order to prepare for a link training with DDI, the state machine
would call intel_ddi_prepare_link_retrain(). To remove the dependency to
the hardware information, replace that direct call with a callback.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_ddi.c              | 8 ++++----
 drivers/gpu/drm/i915/intel_dp.c               | 3 +++
 drivers/gpu/drm/i915/intel_dp_link_training.c | 6 ++----
 drivers/gpu/drm/i915/intel_drv.h              | 6 +++++-
 4 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_ddi.c b/drivers/gpu/drm/i915/intel_ddi.c
index 220679d..d9af009 100644
--- a/drivers/gpu/drm/i915/intel_ddi.c
+++ b/drivers/gpu/drm/i915/intel_ddi.c
@@ -2968,11 +2968,11 @@ void intel_ddi_pll_init(struct drm_device *dev)
 	}
 }
 
-void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder)
+void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp)
 {
-	struct intel_digital_port *intel_dig_port = enc_to_dig_port(encoder);
-	struct intel_dp *intel_dp = &intel_dig_port->dp;
-	struct drm_i915_private *dev_priv = encoder->dev->dev_private;
+	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);
 	enum port port = intel_dig_port->port;
 	uint32_t val;
 	bool wait = false;
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 619b422..f64888f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -5746,6 +5746,9 @@ intel_dp_init_connector(struct intel_digital_port *intel_dig_port,
 	else
 		intel_dp->get_aux_send_ctl = i9xx_get_aux_send_ctl;
 
+	if (HAS_DDI(dev))
+		intel_dp->prepare_link_retrain = intel_ddi_prepare_link_retrain;
+
 	/* Preserve the current hw state. */
 	intel_dp->DP = I915_READ(intel_dp->output_reg);
 	intel_dp->attached_connector = intel_connector;
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index f33cbbb..e7f9320 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -108,8 +108,6 @@ intel_dp_update_link_train(struct intel_dp *intel_dp)
 static bool
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
-	struct drm_encoder *encoder = &dp_to_dig_port(intel_dp)->base.base;
-	struct drm_device *dev = encoder->dev;
 	int i;
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
@@ -117,8 +115,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	uint8_t link_bw, rate_select;
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 
-	if (HAS_DDI(dev))
-		intel_ddi_prepare_link_retrain(encoder);
+	if (intel_dp->prepare_link_retrain)
+		intel_dp->prepare_link_retrain(intel_dp);
 
 	intel_dp_compute_rate(intel_dp, intel_dp->link_rate,
 			      &link_bw, &rate_select);
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index f6fd359..5bb7eb5 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -798,6 +798,10 @@ struct intel_dp {
 				     bool has_aux_irq,
 				     int send_bytes,
 				     uint32_t aux_clock_divider);
+
+	/* This is called before a link training is starterd */
+	void (*prepare_link_retrain)(struct intel_dp *intel_dp);
+
 	bool train_set_valid;
 
 	/* Displayport compliance testing */
@@ -1002,7 +1006,7 @@ void intel_ddi_disable_pipe_clock(struct intel_crtc *intel_crtc);
 bool intel_ddi_pll_select(struct intel_crtc *crtc,
 			  struct intel_crtc_state *crtc_state);
 void intel_ddi_set_pipe_settings(struct drm_crtc *crtc);
-void intel_ddi_prepare_link_retrain(struct drm_encoder *encoder);
+void intel_ddi_prepare_link_retrain(struct intel_dp *intel_dp);
 bool intel_ddi_connector_get_hw_state(struct intel_connector *intel_connector);
 void intel_ddi_fdi_disable(struct drm_crtc *crtc);
 void intel_ddi_get_config(struct intel_encoder *encoder,
-- 
2.4.3

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

[PATCH 08/15] drm/i915: Make intel_dp_source_supports_hbr2() take an intel_dp pointer

[Ander Conselvan de Oliveira]

The function name implies it should get intel_dp, and it's mostly used
where there is an intel_dp in the context.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c               | 19 +++++++++++--------
 drivers/gpu/drm/i915/intel_dp_link_training.c |  4 +---
 drivers/gpu/drm/i915/intel_drv.h              |  2 +-
 3 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f64888f..b785f1f 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1189,8 +1189,11 @@ intel_dp_sink_rates(struct intel_dp *intel_dp, const int **sink_rates)
 	return (intel_dp_max_link_bw(intel_dp) >> 3) + 1;
 }
 
-bool intel_dp_source_supports_hbr2(struct drm_device *dev)
+bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp)
 {
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
+
 	/* WaDisableHBR2:skl */
 	if (IS_SKYLAKE(dev) && INTEL_REVID(dev) <= SKL_REVID_B0)
 		return false;
@@ -1203,8 +1206,10 @@ bool intel_dp_source_supports_hbr2(struct drm_device *dev)
 }
 
 static int
-intel_dp_source_rates(struct drm_device *dev, const int **source_rates)
+intel_dp_source_rates(struct intel_dp *intel_dp, const int **source_rates)
 {
+	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
+	struct drm_device *dev = dig_port->base.base.dev;
 	int size;
 
 	if (IS_BROXTON(dev)) {
@@ -1219,7 +1224,7 @@ intel_dp_source_rates(struct drm_device *dev, const int **source_rates)
 	}
 
 	/* This depends on the fact that 5.4 is last value in the array */
-	if (!intel_dp_source_supports_hbr2(dev))
+	if (!intel_dp_source_supports_hbr2(intel_dp))
 		size--;
 
 	return size;
@@ -1284,12 +1289,11 @@ static int intersect_rates(const int *source_rates, int source_len,
 static int intel_dp_common_rates(struct intel_dp *intel_dp,
 				 int *common_rates)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	const int *source_rates, *sink_rates;
 	int source_len, sink_len;
 
 	sink_len = intel_dp_sink_rates(intel_dp, &sink_rates);
-	source_len = intel_dp_source_rates(dev, &source_rates);
+	source_len = intel_dp_source_rates(intel_dp, &source_rates);
 
 	return intersect_rates(source_rates, source_len,
 			       sink_rates, sink_len,
@@ -1314,7 +1318,6 @@ static void snprintf_int_array(char *str, size_t len,
 
 static void intel_dp_print_rates(struct intel_dp *intel_dp)
 {
-	struct drm_device *dev = intel_dp_to_dev(intel_dp);
 	const int *source_rates, *sink_rates;
 	int source_len, sink_len, common_len;
 	int common_rates[DP_MAX_SUPPORTED_RATES];
@@ -1323,7 +1326,7 @@ static void intel_dp_print_rates(struct intel_dp *intel_dp)
 	if ((drm_debug & DRM_UT_KMS) == 0)
 		return;
 
-	source_len = intel_dp_source_rates(dev, &source_rates);
+	source_len = intel_dp_source_rates(intel_dp, &source_rates);
 	snprintf_int_array(str, sizeof(str), source_rates, source_len);
 	DRM_DEBUG_KMS("source rates: %s\n", str);
 
@@ -3726,7 +3729,7 @@ intel_dp_get_dpcd(struct intel_dp *intel_dp)
 	}
 
 	DRM_DEBUG_KMS("Display Port TPS3 support: source %s, sink %s\n",
-		      yesno(intel_dp_source_supports_hbr2(dev)),
+		      yesno(intel_dp_source_supports_hbr2(intel_dp)),
 		      yesno(drm_dp_tps3_supported(intel_dp->dpcd)));
 
 	/* Intermediate frequency support */
diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index e7f9320..4d2bdc0 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -219,8 +219,6 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 static bool
 intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 {
-	struct intel_digital_port *dig_port = dp_to_dig_port(intel_dp);
-	struct drm_device *dev = dig_port->base.base.dev;
 	bool channel_eq = false;
 	int tries, cr_tries;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
@@ -234,7 +232,7 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 	 * Due to WaDisableHBR2 SKL < B0 is the only exception where TPS3 is
 	 * supported but still not enabled.
 	 */
-	if (intel_dp_source_supports_hbr2(dev) &&
+	if (intel_dp_source_supports_hbr2(intel_dp) &&
 	    drm_dp_tps3_supported(intel_dp->dpcd))
 		training_pattern = DP_TRAINING_PATTERN_3;
 	else if (intel_dp->link_rate == 540000)
diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h
index 5bb7eb5..298050f 100644
--- a/drivers/gpu/drm/i915/intel_drv.h
+++ b/drivers/gpu/drm/i915/intel_drv.h
@@ -1263,7 +1263,7 @@ uint8_t
 intel_dp_pre_emphasis_max(struct intel_dp *intel_dp, uint8_t voltage_swing);
 void intel_dp_compute_rate(struct intel_dp *intel_dp, int port_clock,
 			   uint8_t *link_bw, uint8_t *rate_select);
-bool intel_dp_source_supports_hbr2(struct drm_device *dev);
+bool intel_dp_source_supports_hbr2(struct intel_dp *intel_dp);
 bool
 intel_dp_get_link_status(struct intel_dp *intel_dp, uint8_t link_status[DP_LINK_STATUS_SIZE]);
 
-- 
2.4.3

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

[PATCH 09/15] drm/i915: Move link training setup code to separate functions

[Ander Conselvan de Oliveira]

Move the setup code for the different phases of link training into
functions separate from the training loop. This shouldn't cause any
change in behavior, but make the code slightly less hard to read.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 48 ++++++++++++++++++---------
 1 file changed, 33 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 4d2bdc0..c936b9d 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -104,16 +104,11 @@ intel_dp_update_link_train(struct intel_dp *intel_dp)
 	return ret == intel_dp->lane_count;
 }
 
-/* Enable corresponding port and start training pattern 1 */
 static bool
-intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
+setup_clock_recovery(struct intel_dp *intel_dp)
 {
-	int i;
-	uint8_t voltage;
-	int voltage_tries, loop_tries;
 	uint8_t link_config[2];
 	uint8_t link_bw, rate_select;
-	uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 	if (intel_dp->prepare_link_retrain)
 		intel_dp->prepare_link_retrain(intel_dp);
@@ -145,6 +140,21 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	return true;
+}
+
+/* Enable corresponding port and start training pattern 1 */
+static bool
+intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
+{
+	int i;
+	uint8_t voltage;
+	int voltage_tries, loop_tries;
+	uint8_t link_status[DP_LINK_STATUS_SIZE];
+
+	if (!setup_clock_recovery(intel_dp))
+		return false;
+
 	voltage = 0xff;
 	voltage_tries = 0;
 	loop_tries = 0;
@@ -217,10 +227,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 }
 
 static bool
-intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
+setup_channel_equalization(struct intel_dp *intel_dp)
 {
-	bool channel_eq = false;
-	int tries, cr_tries;
 	uint32_t training_pattern = DP_TRAINING_PATTERN_2;
 
 	/*
@@ -246,6 +254,18 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		return false;
 	}
 
+	return true;
+}
+
+static bool
+intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
+{
+	bool channel_eq = false;
+	int tries, cr_tries;
+
+	if (!setup_channel_equalization(intel_dp))
+		return false;
+
 	tries = 0;
 	cr_tries = 0;
 	channel_eq = false;
@@ -268,9 +288,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 					      intel_dp->lane_count)) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp,
-						training_pattern |
-						DP_LINK_SCRAMBLING_DISABLE);
+			if (!setup_channel_equalization(intel_dp))
+				return false;
 			cr_tries++;
 			continue;
 		}
@@ -285,9 +304,8 @@ intel_dp_link_training_channel_equalization(struct intel_dp *intel_dp)
 		if (tries > 5) {
 			intel_dp->train_set_valid = false;
 			intel_dp_link_training_clock_recovery(intel_dp);
-			intel_dp_set_link_train(intel_dp,
-						training_pattern |
-						DP_LINK_SCRAMBLING_DISABLE);
+			if (!setup_channel_equalization(intel_dp))
+				return false;
 			tries = 0;
 			cr_tries++;
 			continue;
-- 
2.4.3

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

[PATCH 10/15] drm/i915: Move test for max voltage on all lanes to separate function

[Ander Conselvan de Oliveira]

Move the logic for checking if we have reached max voltage swing on all
lanes to a separate function to declutter the link training clock
recovery code.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index c936b9d..6f8de9d 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -143,11 +143,22 @@ setup_clock_recovery(struct intel_dp *intel_dp)
 	return true;
 }
 
+static bool
+max_voltage_reached_on_all_lanes(struct intel_dp *intel_dp)
+{
+	int i;
+
+	for (i = 0; i < intel_dp->lane_count; i++)
+		if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
+			return false;
+
+	return true;
+}
+
 /* Enable corresponding port and start training pattern 1 */
 static bool
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 {
-	int i;
 	uint8_t voltage;
 	int voltage_tries, loop_tries;
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
@@ -188,10 +199,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		}
 
 		/* Check to see if we've tried the max voltage */
-		for (i = 0; i < intel_dp->lane_count; i++)
-			if ((intel_dp->train_set[i] & DP_TRAIN_MAX_SWING_REACHED) == 0)
-				break;
-		if (i == intel_dp->lane_count) {
+		if (max_voltage_reached_on_all_lanes(intel_dp)) {
 			++loop_tries;
 			if (loop_tries == 5) {
 				DRM_ERROR("too many full retries, give up\n");
-- 
2.4.3

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

[PATCH 11/15] drm/i915: Add function for getting the current link training voltage

[Ander Conselvan de Oliveira]

Add a function for retrieving the current voltage swing being used for
link training. Using that function in the clock recovery code makes it a
bit more readable.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 10 ++++++++--
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 6f8de9d..cd16d65 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -155,6 +155,12 @@ max_voltage_reached_on_all_lanes(struct intel_dp *intel_dp)
 	return true;
 }
 
+static uint8_t
+intel_dp_get_train_voltage(struct intel_dp *intel_dp)
+{
+	return intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
+}
+
 /* Enable corresponding port and start training pattern 1 */
 static bool
 intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
@@ -213,7 +219,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 		}
 
 		/* Check to see if we've tried the same voltage 5 times */
-		if ((intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK) == voltage) {
+		if (intel_dp_get_train_voltage(intel_dp) == voltage) {
 			++voltage_tries;
 			if (voltage_tries == 5) {
 				DRM_ERROR("too many voltage retries, give up\n");
@@ -221,7 +227,7 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 			}
 		} else
 			voltage_tries = 0;
-		voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
+		voltage = intel_dp_get_train_voltage(intel_dp);
 
 		/* Update training set as requested by target */
 		intel_get_adjust_train(intel_dp, link_status);
-- 
2.4.3

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

[PATCH 12/15] drm/i915: Split full retries loop out of clock recovery code

[Ander Conselvan de Oliveira]

When a failure to achieve clock recovery happens, the link training code
repeats the training process starting with initial values up to five
times before giving up. The logic for the so called "full retries" and
the "voltage tries" was convoluted into a single loop. This patch splits
it into two separate loops, making it easier to follow.

Note that prior to this patch, a failure to get clock recovery with
previously know good values wouldn't count as a voltage or full retry
failure, but now that counts as a full retry failure.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 75 +++++++++++++--------------
 1 file changed, 35 insertions(+), 40 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index cd16d65..8b20970 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -132,14 +132,6 @@ setup_clock_recovery(struct intel_dp *intel_dp)
 
 	intel_dp->DP |= DP_PORT_EN;
 
-	/* clock recovery */
-	if (!intel_dp_reset_link_train(intel_dp,
-				       DP_TRAINING_PATTERN_1 |
-				       DP_LINK_SCRAMBLING_DISABLE)) {
-		DRM_ERROR("failed to enable link training\n");
-		return false;
-	}
-
 	return true;
 }
 
@@ -161,20 +153,13 @@ intel_dp_get_train_voltage(struct intel_dp *intel_dp)
 	return intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
 }
 
-/* Enable corresponding port and start training pattern 1 */
 static bool
-intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
+clock_recovery_voltage_step(struct intel_dp *intel_dp)
 {
-	uint8_t voltage;
-	int voltage_tries, loop_tries;
+	int voltage_tries = 0;
+	uint8_t voltage = 0xff;
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 
-	if (!setup_clock_recovery(intel_dp))
-		return false;
-
-	voltage = 0xff;
-	voltage_tries = 0;
-	loop_tries = 0;
 	for (;;) {
 		drm_dp_link_train_clock_recovery_delay(intel_dp->dpcd);
 		if (!intel_dp_get_link_status(intel_dp, link_status)) {
@@ -182,10 +167,8 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 			break;
 		}
 
-		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
-			DRM_DEBUG_KMS("clock recovery OK\n");
+		if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count))
 			break;
-		}
 
 		/*
 		 * if we used previously trained voltage and pre-emphasis values
@@ -195,28 +178,12 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 			DRM_DEBUG_KMS("clock recovery not ok, reset");
 			/* clear the flag as we are not reusing train set */
 			intel_dp->train_set_valid = false;
-			if (!intel_dp_reset_link_train(intel_dp,
-						       DP_TRAINING_PATTERN_1 |
-						       DP_LINK_SCRAMBLING_DISABLE)) {
-				DRM_ERROR("failed to enable link training\n");
-				return false;
-			}
-			continue;
+			break;
 		}
 
 		/* Check to see if we've tried the max voltage */
-		if (max_voltage_reached_on_all_lanes(intel_dp)) {
-			++loop_tries;
-			if (loop_tries == 5) {
-				DRM_ERROR("too many full retries, give up\n");
-				break;
-			}
-			intel_dp_reset_link_train(intel_dp,
-						  DP_TRAINING_PATTERN_1 |
-						  DP_LINK_SCRAMBLING_DISABLE);
-			voltage_tries = 0;
-			continue;
-		}
+		if (max_voltage_reached_on_all_lanes(intel_dp))
+			break;
 
 		/* Check to see if we've tried the same voltage 5 times */
 		if (intel_dp_get_train_voltage(intel_dp) == voltage) {
@@ -240,6 +207,34 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
 	return drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count);
 }
 
+/* Enable corresponding port and start training pattern 1 */
+static bool
+intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
+{
+	int loop_tries;
+
+	if (!setup_clock_recovery(intel_dp))
+		return false;
+
+	for (loop_tries = 0; loop_tries < 5; loop_tries++) {
+		if (!intel_dp_reset_link_train(intel_dp,
+					       DP_TRAINING_PATTERN_1 |
+					       DP_LINK_SCRAMBLING_DISABLE)) {
+			DRM_ERROR("failed to enable link training\n");
+			return false;
+		}
+
+		if (clock_recovery_voltage_step(intel_dp)) {
+			DRM_DEBUG_KMS("clock recovery OK\n");
+			return true;
+		}
+	}
+
+	DRM_ERROR("too many full retries, give up\n");
+
+	return false;
+}
+
 static bool
 setup_channel_equalization(struct intel_dp *intel_dp)
 {
-- 
2.4.3

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

[PATCH 13/15] drm/i915: Make the link training test for same voltage

[Ander Conselvan de Oliveira]

It makes it slightly easier to read.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 8b20970..ba640b7 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -186,14 +186,13 @@ clock_recovery_voltage_step(struct intel_dp *intel_dp)
 			break;
 
 		/* Check to see if we've tried the same voltage 5 times */
-		if (intel_dp_get_train_voltage(intel_dp) == voltage) {
-			++voltage_tries;
-			if (voltage_tries == 5) {
-				DRM_ERROR("too many voltage retries, give up\n");
-				break;
-			}
-		} else
+		if (intel_dp_get_train_voltage(intel_dp) != voltage) {
 			voltage_tries = 0;
+		} else if (++voltage_tries == 5) {
+			DRM_ERROR("too many voltage retries, give up\n");
+			break;
+		}
+
 		voltage = intel_dp_get_train_voltage(intel_dp);
 
 		/* Update training set as requested by target */
-- 
2.4.3

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

[PATCH v2] drm/i915: Make the link training test for same voltage smaller

[Ander Conselvan de Oliveira]

It makes it slightly easier to read.

v2: Add missing word in patch title. (Ander)

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 13 ++++++-------
 1 file changed, 6 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 8b20970..ba640b7 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -186,14 +186,13 @@ clock_recovery_voltage_step(struct intel_dp *intel_dp)
 			break;
 
 		/* Check to see if we've tried the same voltage 5 times */
-		if (intel_dp_get_train_voltage(intel_dp) == voltage) {
-			++voltage_tries;
-			if (voltage_tries == 5) {
-				DRM_ERROR("too many voltage retries, give up\n");
-				break;
-			}
-		} else
+		if (intel_dp_get_train_voltage(intel_dp) != voltage) {
 			voltage_tries = 0;
+		} else if (++voltage_tries == 5) {
+			DRM_ERROR("too many voltage retries, give up\n");
+			break;
+		}
+
 		voltage = intel_dp_get_train_voltage(intel_dp);
 
 		/* Update training set as requested by target */
-- 
2.4.3

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

[PATCH 14/15] drm/i915: Move the voltage changed check into intel_get_adjust_train()

[Ander Conselvan de Oliveira]

Move the check of whether the voltage changed into the function that
parses the sink adjust request, simplifying the voltage loop a bit.

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 32 ++++++++++++++-------------
 1 file changed, 17 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index ba640b7..5f7c229 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -23,7 +23,8 @@
 
 #include "intel_drv.h"
 
-static void
+/* Returns true if the voltage swing changed */
+static bool
 intel_get_adjust_train(struct intel_dp *intel_dp,
 		       const uint8_t link_status[DP_LINK_STATUS_SIZE])
 {
@@ -32,6 +33,8 @@ intel_get_adjust_train(struct intel_dp *intel_dp,
 	int lane;
 	uint8_t voltage_max;
 	uint8_t preemph_max;
+	uint8_t mask;
+	bool voltage_changed = false;
 
 	for (lane = 0; lane < intel_dp->lane_count; lane++) {
 		uint8_t this_v = drm_dp_get_adjust_request_voltage(link_status, lane);
@@ -51,8 +54,16 @@ intel_get_adjust_train(struct intel_dp *intel_dp,
 	if (p >= preemph_max)
 		p = preemph_max | DP_TRAIN_MAX_PRE_EMPHASIS_REACHED;
 
-	for (lane = 0; lane < 4; lane++)
+	mask = DP_TRAIN_VOLTAGE_SWING_MASK | DP_TRAIN_MAX_SWING_REACHED;
+
+	for (lane = 0; lane < 4; lane++) {
+		if ((intel_dp->train_set[lane] & mask) != v)
+			voltage_changed = true;
+
 		intel_dp->train_set[lane] = v | p;
+	}
+
+	return voltage_changed;
 }
 
 static bool
@@ -147,17 +158,10 @@ max_voltage_reached_on_all_lanes(struct intel_dp *intel_dp)
 	return true;
 }
 
-static uint8_t
-intel_dp_get_train_voltage(struct intel_dp *intel_dp)
-{
-	return intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK;
-}
-
 static bool
 clock_recovery_voltage_step(struct intel_dp *intel_dp)
 {
 	int voltage_tries = 0;
-	uint8_t voltage = 0xff;
 	uint8_t link_status[DP_LINK_STATUS_SIZE];
 
 	for (;;) {
@@ -185,18 +189,16 @@ clock_recovery_voltage_step(struct intel_dp *intel_dp)
 		if (max_voltage_reached_on_all_lanes(intel_dp))
 			break;
 
-		/* Check to see if we've tried the same voltage 5 times */
-		if (intel_dp_get_train_voltage(intel_dp) != voltage) {
+		/* Update training set and check to see if we've tried the same
+		 *  voltage 5 times */
+		if (intel_get_adjust_train(intel_dp, link_status)) {
 			voltage_tries = 0;
 		} else if (++voltage_tries == 5) {
 			DRM_ERROR("too many voltage retries, give up\n");
 			break;
 		}
 
-		voltage = intel_dp_get_train_voltage(intel_dp);
-
-		/* Update training set as requested by target */
-		intel_get_adjust_train(intel_dp, link_status);
+		/* Make the new training set effective */
 		if (!intel_dp_update_link_train(intel_dp)) {
 			DRM_ERROR("failed to update link training\n");
 			break;
-- 
2.4.3

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

[PATCH 15/15] drm/i915: Add missing newline to link training debug message

[Ander Conselvan de Oliveira]

Signed-off-by: Ander Conselvan de Oliveira <ander.conselvan.de.oliveira@intel.com>
---
 drivers/gpu/drm/i915/intel_dp_link_training.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp_link_training.c b/drivers/gpu/drm/i915/intel_dp_link_training.c
index 5f7c229..a33f5d4 100644
--- a/drivers/gpu/drm/i915/intel_dp_link_training.c
+++ b/drivers/gpu/drm/i915/intel_dp_link_training.c
@@ -179,7 +179,7 @@ clock_recovery_voltage_step(struct intel_dp *intel_dp)
 		 * and we don't get clock recovery, reset link training values
 		 */
 		if (intel_dp->train_set_valid) {
-			DRM_DEBUG_KMS("clock recovery not ok, reset");
+			DRM_DEBUG_KMS("clock recovery not ok, reset\n");
 			/* clear the flag as we are not reusing train set */
 			intel_dp->train_set_valid = false;
 			break;
-- 
2.4.3

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