| Message ID | 1380283844-30766-1-git-send-email-jani.nikula@intel.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
2013/9/27 Jani Nikula <jani.nikula@intel.com>: > Neither the DP spec nor the compliance test spec state or imply that we > should write the DP_TRAINING_PATTERN_SET at every voltage swing and > pre-emphasis change. Indeed we probably shouldn't. So don't. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49402 > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > I haven't tested this much, but it fixes the Zotac DP -> dual-hdmi > dongle for me. Still boots Haswell with eDP+DP, no new error messages. I'll leave the review to Todd :) Smoke-tested-by: Paulo Zanoni <paulo.r.zanoni@intel.com> > --- > drivers/gpu/drm/i915/intel_dp.c | 129 ++++++++++++++++++++++++++------------- > 1 file changed, 87 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 95a3159..ab805d3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2313,7 +2313,7 @@ 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_reg_value, > + uint32_t *DP, > uint8_t dp_train_pat) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -2349,50 +2349,51 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > I915_WRITE(DP_TP_CTL(port), temp); > > } else if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A)) { > - dp_reg_value &= ~DP_LINK_TRAIN_MASK_CPT; > + *DP &= ~DP_LINK_TRAIN_MASK_CPT; > > switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { > case DP_TRAINING_PATTERN_DISABLE: > - dp_reg_value |= DP_LINK_TRAIN_OFF_CPT; > + *DP |= DP_LINK_TRAIN_OFF_CPT; > break; > case DP_TRAINING_PATTERN_1: > - dp_reg_value |= DP_LINK_TRAIN_PAT_1_CPT; > + *DP |= DP_LINK_TRAIN_PAT_1_CPT; > break; > case DP_TRAINING_PATTERN_2: > - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; > + *DP |= DP_LINK_TRAIN_PAT_2_CPT; > break; > case DP_TRAINING_PATTERN_3: > DRM_ERROR("DP training pattern 3 not supported\n"); > - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; > + *DP |= DP_LINK_TRAIN_PAT_2_CPT; > break; > } > > } else { > - dp_reg_value &= ~DP_LINK_TRAIN_MASK; > + *DP &= ~DP_LINK_TRAIN_MASK; > > switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { > case DP_TRAINING_PATTERN_DISABLE: > - dp_reg_value |= DP_LINK_TRAIN_OFF; > + *DP |= DP_LINK_TRAIN_OFF; > break; > case DP_TRAINING_PATTERN_1: > - dp_reg_value |= DP_LINK_TRAIN_PAT_1; > + *DP |= DP_LINK_TRAIN_PAT_1; > break; > case DP_TRAINING_PATTERN_2: > - dp_reg_value |= DP_LINK_TRAIN_PAT_2; > + *DP |= DP_LINK_TRAIN_PAT_2; > break; > case DP_TRAINING_PATTERN_3: > DRM_ERROR("DP training pattern 3 not supported\n"); > - dp_reg_value |= DP_LINK_TRAIN_PAT_2; > + *DP |= DP_LINK_TRAIN_PAT_2; > break; > } > } > > - I915_WRITE(intel_dp->output_reg, dp_reg_value); > + I915_WRITE(intel_dp->output_reg, *DP); > POSTING_READ(intel_dp->output_reg); > > - intel_dp_aux_native_write_1(intel_dp, > - DP_TRAINING_PATTERN_SET, > - dp_train_pat); > + ret = intel_dp_aux_native_write_1(intel_dp, DP_TRAINING_PATTERN_SET, > + dp_train_pat); > + if (ret != 1) > + return false; > > if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) != > DP_TRAINING_PATTERN_DISABLE) { > @@ -2407,6 +2408,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > return true; > } > > +static bool > +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, > + uint8_t dp_train_pat) > +{ > + memset(intel_dp->train_set, 0, 4); > + intel_dp_set_signal_levels(intel_dp, DP); > + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); > +} > + > +static bool > +intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, > + uint8_t link_status[DP_LINK_STATUS_SIZE]) > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + int ret; > + > + intel_get_adjust_train(intel_dp, link_status); > + intel_dp_set_signal_levels(intel_dp, DP); > + > + I915_WRITE(intel_dp->output_reg, *DP); > + POSTING_READ(intel_dp->output_reg); > + > + ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET, > + intel_dp->train_set, > + intel_dp->lane_count); > + > + return ret == intel_dp->lane_count; > +} > + > static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -2459,21 +2491,19 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > DP |= DP_PORT_EN; > > - memset(intel_dp->train_set, 0, 4); > + /* clock recovery */ > + if (!intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to enable link training\n"); > + return; > + } > + > voltage = 0xff; > voltage_tries = 0; > loop_tries = 0; > for (;;) { > - /* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */ > - uint8_t link_status[DP_LINK_STATUS_SIZE]; > - > - intel_dp_set_signal_levels(intel_dp, &DP); > - > - /* Set training pattern 1 */ > - if (!intel_dp_set_link_train(intel_dp, DP, > - DP_TRAINING_PATTERN_1 | > - DP_LINK_SCRAMBLING_DISABLE)) > - break; > + 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)) { > @@ -2496,7 +2526,9 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > DRM_DEBUG_KMS("too many full retries, give up\n"); > break; > } > - memset(intel_dp->train_set, 0, 4); > + intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE); > voltage_tries = 0; > continue; > } > @@ -2512,8 +2544,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > voltage_tries = 0; > voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; > > - /* Compute new intel_dp->train_set as requested by target */ > - intel_get_adjust_train(intel_dp, link_status); > + /* Update training set as requested by target */ > + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { > + DRM_ERROR("failed to update link training\n"); > + break; > + } > } > > intel_dp->DP = DP; > @@ -2527,11 +2562,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > uint32_t DP = intel_dp->DP; > > /* channel equalization */ > + if (!intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to start channel equalization\n"); > + return; > + } > + > tries = 0; > cr_tries = 0; > channel_eq = false; > for (;;) { > - uint8_t link_status[DP_LINK_STATUS_SIZE]; > + uint8_t link_status[DP_LINK_STATUS_SIZE]; > > if (cr_tries > 5) { > DRM_ERROR("failed to train DP, aborting\n"); > @@ -2539,21 +2581,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > break; > } > > - intel_dp_set_signal_levels(intel_dp, &DP); > - > - /* channel eq pattern */ > - if (!intel_dp_set_link_train(intel_dp, DP, > - DP_TRAINING_PATTERN_2 | > - DP_LINK_SCRAMBLING_DISABLE)) > - break; > - > drm_dp_link_train_channel_eq_delay(intel_dp->dpcd); > - if (!intel_dp_get_link_status(intel_dp, link_status)) > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + DRM_ERROR("failed to get link status\n"); > break; > + } > > /* Make sure clock is still ok */ > if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { > intel_dp_start_link_train(intel_dp); > + intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE); > cr_tries++; > continue; > } > @@ -2567,13 +2606,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > if (tries > 5) { > intel_dp_link_down(intel_dp); > intel_dp_start_link_train(intel_dp); > + intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE); > tries = 0; > cr_tries++; > continue; > } > > - /* Compute new intel_dp->train_set as requested by target */ > - intel_get_adjust_train(intel_dp, link_status); > + /* Update training set as requested by target */ > + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { > + DRM_ERROR("failed to update link training\n"); > + break; > + } > ++tries; > } > > @@ -2588,7 +2633,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > > 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, &intel_dp->DP, > DP_TRAINING_PATTERN_DISABLE); > } > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 27, 2013 at 03:10:44PM +0300, Jani Nikula wrote: > Neither the DP spec nor the compliance test spec state or imply that we > should write the DP_TRAINING_PATTERN_SET at every voltage swing and > pre-emphasis change. Indeed we probably shouldn't. So don't. > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49402 > Signed-off-by: Jani Nikula <jani.nikula@intel.com> > > --- > > I haven't tested this much, but it fixes the Zotac DP -> dual-hdmi > dongle for me. > --- > drivers/gpu/drm/i915/intel_dp.c | 129 ++++++++++++++++++++++++++------------- > 1 file changed, 87 insertions(+), 42 deletions(-) > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > index 95a3159..ab805d3 100644 > --- a/drivers/gpu/drm/i915/intel_dp.c > +++ b/drivers/gpu/drm/i915/intel_dp.c > @@ -2313,7 +2313,7 @@ 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_reg_value, > + uint32_t *DP, > uint8_t dp_train_pat) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -2349,50 +2349,51 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > I915_WRITE(DP_TP_CTL(port), temp); > > } else if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A)) { > - dp_reg_value &= ~DP_LINK_TRAIN_MASK_CPT; > + *DP &= ~DP_LINK_TRAIN_MASK_CPT; > > switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { > case DP_TRAINING_PATTERN_DISABLE: > - dp_reg_value |= DP_LINK_TRAIN_OFF_CPT; > + *DP |= DP_LINK_TRAIN_OFF_CPT; > break; > case DP_TRAINING_PATTERN_1: > - dp_reg_value |= DP_LINK_TRAIN_PAT_1_CPT; > + *DP |= DP_LINK_TRAIN_PAT_1_CPT; > break; > case DP_TRAINING_PATTERN_2: > - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; > + *DP |= DP_LINK_TRAIN_PAT_2_CPT; > break; > case DP_TRAINING_PATTERN_3: > DRM_ERROR("DP training pattern 3 not supported\n"); > - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; > + *DP |= DP_LINK_TRAIN_PAT_2_CPT; > break; > } > > } else { > - dp_reg_value &= ~DP_LINK_TRAIN_MASK; > + *DP &= ~DP_LINK_TRAIN_MASK; > > switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { > case DP_TRAINING_PATTERN_DISABLE: > - dp_reg_value |= DP_LINK_TRAIN_OFF; > + *DP |= DP_LINK_TRAIN_OFF; > break; > case DP_TRAINING_PATTERN_1: > - dp_reg_value |= DP_LINK_TRAIN_PAT_1; > + *DP |= DP_LINK_TRAIN_PAT_1; > break; > case DP_TRAINING_PATTERN_2: > - dp_reg_value |= DP_LINK_TRAIN_PAT_2; > + *DP |= DP_LINK_TRAIN_PAT_2; > break; > case DP_TRAINING_PATTERN_3: > DRM_ERROR("DP training pattern 3 not supported\n"); > - dp_reg_value |= DP_LINK_TRAIN_PAT_2; > + *DP |= DP_LINK_TRAIN_PAT_2; > break; > } > } > > - I915_WRITE(intel_dp->output_reg, dp_reg_value); > + I915_WRITE(intel_dp->output_reg, *DP); > POSTING_READ(intel_dp->output_reg); > > - intel_dp_aux_native_write_1(intel_dp, > - DP_TRAINING_PATTERN_SET, > - dp_train_pat); > + ret = intel_dp_aux_native_write_1(intel_dp, DP_TRAINING_PATTERN_SET, > + dp_train_pat); > + if (ret != 1) > + return false; As a followup patch I think we should do a burst write here with the pattern and lane set. My rationale from the spec: "The upstream device may start Full Link Training with non-minimum differential voltage swing and/or non-zero pre-emphasis and/or non-zero Post Cursor2 if the optimal setting is already known, for example, as is the case in embedded application. In this case, the upstream device must write the swing and pre-emphasis settings at which it is starting training to TRAINING_LANEx_SET as part of the burst write in which it writes 21h to TRAINING_PATTERN_SET. If non-zero Post Cursor 2 is used, then the transmitter must write the post cursor 2 setting to the LANEx POST CURSOR2 SET fields immediately after writing 21h to TRAINING_PATTERN_SET." Now, we don't currently start training using non-zero values, but if we decide to do it in the future, this little bit of detail would be easy to overlook, and then we're again left with some random failures no-one is able to figure out. The ordering here is also a bit questionable without the burst write. The spec says in one place the we should write the lane_set before the pattern, but in some other place it seems to say to opposite. Burst write would get rid of that ambiguity. > > if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) != > DP_TRAINING_PATTERN_DISABLE) { > @@ -2407,6 +2408,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, > return true; > } > > +static bool > +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, > + uint8_t dp_train_pat) > +{ > + memset(intel_dp->train_set, 0, 4); sizeof(train_set) > + intel_dp_set_signal_levels(intel_dp, DP); > + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); > +} > + > +static bool > +intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, > + uint8_t link_status[DP_LINK_STATUS_SIZE]) You could constify all the link_status[] stuff that doesn't need modifications. But that should be a separate patch since the constification should extend all the way down to dp_link_status() & co. in drm_dp_helper. > +{ > + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > + struct drm_device *dev = intel_dig_port->base.base.dev; > + struct drm_i915_private *dev_priv = dev->dev_private; > + int ret; > + > + intel_get_adjust_train(intel_dp, link_status); > + intel_dp_set_signal_levels(intel_dp, DP); > + > + I915_WRITE(intel_dp->output_reg, *DP); > + POSTING_READ(intel_dp->output_reg); > + > + ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET, > + intel_dp->train_set, > + intel_dp->lane_count); > + > + return ret == intel_dp->lane_count; > +} > + > static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) > { > struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); > @@ -2459,21 +2491,19 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > > DP |= DP_PORT_EN; > > - memset(intel_dp->train_set, 0, 4); > + /* clock recovery */ > + if (!intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to enable link training\n"); > + return; > + } > + > voltage = 0xff; > voltage_tries = 0; > loop_tries = 0; > for (;;) { > - /* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */ > - uint8_t link_status[DP_LINK_STATUS_SIZE]; > - > - intel_dp_set_signal_levels(intel_dp, &DP); > - > - /* Set training pattern 1 */ > - if (!intel_dp_set_link_train(intel_dp, DP, > - DP_TRAINING_PATTERN_1 | > - DP_LINK_SCRAMBLING_DISABLE)) > - break; > + 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)) { > @@ -2496,7 +2526,9 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > DRM_DEBUG_KMS("too many full retries, give up\n"); > break; > } > - memset(intel_dp->train_set, 0, 4); > + intel_dp_reset_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_1 | > + DP_LINK_SCRAMBLING_DISABLE); > voltage_tries = 0; Not an issue with your patch but it seems we sould reset voltage = 0xff here too. Otherwise we end up comparing the max voltage setting from the last loop during the next full retry. The full retry shouldn't really depend on the previous attempt at all. Although in reality we shouldn't even do any full retries w/o reducing the bitrate first. But that's an issue that can't be solved by fiddling with the link training code itself. > continue; > } > @@ -2512,8 +2544,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) > voltage_tries = 0; > voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; > > - /* Compute new intel_dp->train_set as requested by target */ > - intel_get_adjust_train(intel_dp, link_status); > + /* Update training set as requested by target */ > + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { > + DRM_ERROR("failed to update link training\n"); > + break; > + } > } > > intel_dp->DP = DP; > @@ -2527,11 +2562,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > uint32_t DP = intel_dp->DP; > > /* channel equalization */ > + if (!intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE)) { > + DRM_ERROR("failed to start channel equalization\n"); > + return; > + } > + > tries = 0; > cr_tries = 0; > channel_eq = false; > for (;;) { > - uint8_t link_status[DP_LINK_STATUS_SIZE]; > + uint8_t link_status[DP_LINK_STATUS_SIZE]; > > if (cr_tries > 5) { > DRM_ERROR("failed to train DP, aborting\n"); > @@ -2539,21 +2581,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > break; > } > > - intel_dp_set_signal_levels(intel_dp, &DP); > - > - /* channel eq pattern */ > - if (!intel_dp_set_link_train(intel_dp, DP, > - DP_TRAINING_PATTERN_2 | > - DP_LINK_SCRAMBLING_DISABLE)) > - break; > - > drm_dp_link_train_channel_eq_delay(intel_dp->dpcd); > - if (!intel_dp_get_link_status(intel_dp, link_status)) > + if (!intel_dp_get_link_status(intel_dp, link_status)) { > + DRM_ERROR("failed to get link status\n"); > break; > + } > > /* Make sure clock is still ok */ > if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { > intel_dp_start_link_train(intel_dp); Missing a 'DP = intel_dp->DP' here maybe? It's a bit hard to see where we need to the DP register shadows to be exactly up to date. I guess we don't actually touch anything there besides the vswing/pre-emph settings, and those get fully overwritten by intel_dp_set_link_train(), so I suppose this works as is, but the local copies make the whole thing feel very fragile. So I think as a separate patch I would like to see the local copies killed off. > + intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE); > cr_tries++; > continue; > } > @@ -2567,13 +2606,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > if (tries > 5) { > intel_dp_link_down(intel_dp); > intel_dp_start_link_train(intel_dp); Same DP vs. intel_dp->DP confusion here as well. Also I'm think this whole thing might be more understandable as a state machine, but that's another bigger topic of discussion. OK, so it looks like I didn't find anything really wrong with this patch itself, and now the code matches the spec much better, so: Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> It's quite amazing, and a little troubling, that the spec has very clear flow charts, but no one (including me) saw the discrepancy until now :) Good job. > + intel_dp_set_link_train(intel_dp, &DP, > + DP_TRAINING_PATTERN_2 | > + DP_LINK_SCRAMBLING_DISABLE); > tries = 0; > cr_tries++; > continue; > } > > - /* Compute new intel_dp->train_set as requested by target */ > - intel_get_adjust_train(intel_dp, link_status); > + /* Update training set as requested by target */ > + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { > + DRM_ERROR("failed to update link training\n"); > + break; > + } > ++tries; > } > > @@ -2588,7 +2633,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) > > 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, &intel_dp->DP, > DP_TRAINING_PATTERN_DISABLE); > } > > -- > 1.7.9.5 > > _______________________________________________ > Intel-gfx mailing list > Intel-gfx@lists.freedesktop.org > http://lists.freedesktop.org/mailman/listinfo/intel-gfx
On Fri, Sep 27, 2013 at 06:13:11PM +0300, Ville Syrjälä wrote: [snip] > OK, so it looks like I didn't find anything really wrong with this > patch itself, and now the code matches the spec much better, so: > Reviewed-by: Ville Syrjälä <ville.syrjala@linux.intel.com> Queued for -next, thanks for the patch. I agree with Ville's observation and beating the dp link train code into better shape would be pretty terrific! -Daniel
diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c index 95a3159..ab805d3 100644 --- a/drivers/gpu/drm/i915/intel_dp.c +++ b/drivers/gpu/drm/i915/intel_dp.c @@ -2313,7 +2313,7 @@ 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_reg_value, + uint32_t *DP, uint8_t dp_train_pat) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -2349,50 +2349,51 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, I915_WRITE(DP_TP_CTL(port), temp); } else if (HAS_PCH_CPT(dev) && (IS_GEN7(dev) || port != PORT_A)) { - dp_reg_value &= ~DP_LINK_TRAIN_MASK_CPT; + *DP &= ~DP_LINK_TRAIN_MASK_CPT; switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { case DP_TRAINING_PATTERN_DISABLE: - dp_reg_value |= DP_LINK_TRAIN_OFF_CPT; + *DP |= DP_LINK_TRAIN_OFF_CPT; break; case DP_TRAINING_PATTERN_1: - dp_reg_value |= DP_LINK_TRAIN_PAT_1_CPT; + *DP |= DP_LINK_TRAIN_PAT_1_CPT; break; case DP_TRAINING_PATTERN_2: - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; + *DP |= DP_LINK_TRAIN_PAT_2_CPT; break; case DP_TRAINING_PATTERN_3: DRM_ERROR("DP training pattern 3 not supported\n"); - dp_reg_value |= DP_LINK_TRAIN_PAT_2_CPT; + *DP |= DP_LINK_TRAIN_PAT_2_CPT; break; } } else { - dp_reg_value &= ~DP_LINK_TRAIN_MASK; + *DP &= ~DP_LINK_TRAIN_MASK; switch (dp_train_pat & DP_TRAINING_PATTERN_MASK) { case DP_TRAINING_PATTERN_DISABLE: - dp_reg_value |= DP_LINK_TRAIN_OFF; + *DP |= DP_LINK_TRAIN_OFF; break; case DP_TRAINING_PATTERN_1: - dp_reg_value |= DP_LINK_TRAIN_PAT_1; + *DP |= DP_LINK_TRAIN_PAT_1; break; case DP_TRAINING_PATTERN_2: - dp_reg_value |= DP_LINK_TRAIN_PAT_2; + *DP |= DP_LINK_TRAIN_PAT_2; break; case DP_TRAINING_PATTERN_3: DRM_ERROR("DP training pattern 3 not supported\n"); - dp_reg_value |= DP_LINK_TRAIN_PAT_2; + *DP |= DP_LINK_TRAIN_PAT_2; break; } } - I915_WRITE(intel_dp->output_reg, dp_reg_value); + I915_WRITE(intel_dp->output_reg, *DP); POSTING_READ(intel_dp->output_reg); - intel_dp_aux_native_write_1(intel_dp, - DP_TRAINING_PATTERN_SET, - dp_train_pat); + ret = intel_dp_aux_native_write_1(intel_dp, DP_TRAINING_PATTERN_SET, + dp_train_pat); + if (ret != 1) + return false; if ((dp_train_pat & DP_TRAINING_PATTERN_MASK) != DP_TRAINING_PATTERN_DISABLE) { @@ -2407,6 +2408,37 @@ intel_dp_set_link_train(struct intel_dp *intel_dp, return true; } +static bool +intel_dp_reset_link_train(struct intel_dp *intel_dp, uint32_t *DP, + uint8_t dp_train_pat) +{ + memset(intel_dp->train_set, 0, 4); + intel_dp_set_signal_levels(intel_dp, DP); + return intel_dp_set_link_train(intel_dp, DP, dp_train_pat); +} + +static bool +intel_dp_update_link_train(struct intel_dp *intel_dp, uint32_t *DP, + uint8_t link_status[DP_LINK_STATUS_SIZE]) +{ + struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); + struct drm_device *dev = intel_dig_port->base.base.dev; + struct drm_i915_private *dev_priv = dev->dev_private; + int ret; + + intel_get_adjust_train(intel_dp, link_status); + intel_dp_set_signal_levels(intel_dp, DP); + + I915_WRITE(intel_dp->output_reg, *DP); + POSTING_READ(intel_dp->output_reg); + + ret = intel_dp_aux_native_write(intel_dp, DP_TRAINING_LANE0_SET, + intel_dp->train_set, + intel_dp->lane_count); + + return ret == intel_dp->lane_count; +} + static void intel_dp_set_idle_link_train(struct intel_dp *intel_dp) { struct intel_digital_port *intel_dig_port = dp_to_dig_port(intel_dp); @@ -2459,21 +2491,19 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) DP |= DP_PORT_EN; - memset(intel_dp->train_set, 0, 4); + /* clock recovery */ + if (!intel_dp_reset_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_ERROR("failed to enable link training\n"); + return; + } + voltage = 0xff; voltage_tries = 0; loop_tries = 0; for (;;) { - /* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */ - uint8_t link_status[DP_LINK_STATUS_SIZE]; - - intel_dp_set_signal_levels(intel_dp, &DP); - - /* Set training pattern 1 */ - if (!intel_dp_set_link_train(intel_dp, DP, - DP_TRAINING_PATTERN_1 | - DP_LINK_SCRAMBLING_DISABLE)) - break; + 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)) { @@ -2496,7 +2526,9 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) DRM_DEBUG_KMS("too many full retries, give up\n"); break; } - memset(intel_dp->train_set, 0, 4); + intel_dp_reset_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_1 | + DP_LINK_SCRAMBLING_DISABLE); voltage_tries = 0; continue; } @@ -2512,8 +2544,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp) voltage_tries = 0; voltage = intel_dp->train_set[0] & DP_TRAIN_VOLTAGE_SWING_MASK; - /* Compute new intel_dp->train_set as requested by target */ - intel_get_adjust_train(intel_dp, link_status); + /* Update training set as requested by target */ + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { + DRM_ERROR("failed to update link training\n"); + break; + } } intel_dp->DP = DP; @@ -2527,11 +2562,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) uint32_t DP = intel_dp->DP; /* channel equalization */ + if (!intel_dp_set_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_2 | + DP_LINK_SCRAMBLING_DISABLE)) { + DRM_ERROR("failed to start channel equalization\n"); + return; + } + tries = 0; cr_tries = 0; channel_eq = false; for (;;) { - uint8_t link_status[DP_LINK_STATUS_SIZE]; + uint8_t link_status[DP_LINK_STATUS_SIZE]; if (cr_tries > 5) { DRM_ERROR("failed to train DP, aborting\n"); @@ -2539,21 +2581,18 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) break; } - intel_dp_set_signal_levels(intel_dp, &DP); - - /* channel eq pattern */ - if (!intel_dp_set_link_train(intel_dp, DP, - DP_TRAINING_PATTERN_2 | - DP_LINK_SCRAMBLING_DISABLE)) - break; - drm_dp_link_train_channel_eq_delay(intel_dp->dpcd); - if (!intel_dp_get_link_status(intel_dp, link_status)) + if (!intel_dp_get_link_status(intel_dp, link_status)) { + DRM_ERROR("failed to get link status\n"); break; + } /* Make sure clock is still ok */ if (!drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) { intel_dp_start_link_train(intel_dp); + intel_dp_set_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_2 | + DP_LINK_SCRAMBLING_DISABLE); cr_tries++; continue; } @@ -2567,13 +2606,19 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) if (tries > 5) { intel_dp_link_down(intel_dp); intel_dp_start_link_train(intel_dp); + intel_dp_set_link_train(intel_dp, &DP, + DP_TRAINING_PATTERN_2 | + DP_LINK_SCRAMBLING_DISABLE); tries = 0; cr_tries++; continue; } - /* Compute new intel_dp->train_set as requested by target */ - intel_get_adjust_train(intel_dp, link_status); + /* Update training set as requested by target */ + if (!intel_dp_update_link_train(intel_dp, &DP, link_status)) { + DRM_ERROR("failed to update link training\n"); + break; + } ++tries; } @@ -2588,7 +2633,7 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp) 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, &intel_dp->DP, DP_TRAINING_PATTERN_DISABLE); }
Neither the DP spec nor the compliance test spec state or imply that we should write the DP_TRAINING_PATTERN_SET at every voltage swing and pre-emphasis change. Indeed we probably shouldn't. So don't. Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=49402 Signed-off-by: Jani Nikula <jani.nikula@intel.com> --- I haven't tested this much, but it fixes the Zotac DP -> dual-hdmi dongle for me. --- drivers/gpu/drm/i915/intel_dp.c | 129 ++++++++++++++++++++++++++------------- 1 file changed, 87 insertions(+), 42 deletions(-)