[Intel-gfx] [PATCH 02/15] drm/i915: Don't pass *DP around to link training functions
Thulasimani, Sivakumar
sivakumar.thulasimani at intel.com
Mon Oct 19 02:01:47 PDT 2015
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 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 ?
> 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 at lists.freedesktop.org
>> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
More information about the Intel-gfx
mailing list