| Message ID | 1444028487-6501-3-git-send-email-ander.conselvan.de.oliveira@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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
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.
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
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
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
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(-)