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@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 ? 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@xxxxxxxxxxxxxxxxxxxxx > http://lists.freedesktop.org/mailman/listinfo/intel-gfx _______________________________________________ Intel-gfx mailing list Intel-gfx@xxxxxxxxxxxxxxxxxxxxx http://lists.freedesktop.org/mailman/listinfo/intel-gfx