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

["Thulasimani, Sivakumar" <sivakumar.thulasimani@intel.com>]

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