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

Commit Message

Ander Conselvan de Oliveira Oct. 21, 2015, 1:52 p.m. UTC

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 <[e-mail]>
---
 drivers/gpu/drm/i915/intel_dp.c | 43 +++++++++++++++++------------------------
 1 file changed, 18 insertions(+), 25 deletions(-)

Comments

Sivakumar Thulasimani Oct. 21, 2015, 2:08 p.m. UTC | #1

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 <[e-mail]>
> ---
>   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);
>   }
>

Patch

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