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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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@xxxxxxxxx>
--

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@xxxxxxxxxxxxxxxxxxxxx
http://lists.freedesktop.org/mailman/listinfo/intel-gfx
[Index of Archives]     [Linux USB Devel]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]
  Powered by Linux