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

["Conselvan De Oliveira, Ander" <ander.conselvan.de.oliveira@intel.com>]

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