[Intel-gfx] [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Sun Oct 18 21:45:29 PDT 2015
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 at 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
More information about the Intel-gfx
mailing list