[Intel-gfx] [PATCH] drm/i915/dp: do not write DP_TRAINING_PATTERN_SET all the time
Ville Syrjälä
ville.syrjala at linux.intel.com
Fri Sep 27 17:13:11 CEST 2013
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 at 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 at 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 at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx
--
Ville Syrjälä
Intel OTC
More information about the Intel-gfx
mailing list