Re: [PATCH v3 4/4] drm/i915/edp: Be less aggressive about changing link config on eDP

Manasi Navare
2017-07-12T21:30:08Z

On Wed, Jul 12, 2017 at 12:16:13AM +0100, Chris Wilson wrote:
> Quoting Jim Bride (2017-07-11 23:19:56)
> > @@ -174,21 +176,25 @@ intel_dp_link_training_clock_recovery(struct intel_dp *intel_dp)
> >
> > if (!intel_dp_get_link_status(intel_dp, link_status)) {
> > DRM_ERROR("failed to get link status\n");
> > + intel_dp->train_set_valid = false;
> > return false;
> > }
> >
> > if (drm_dp_clock_recovery_ok(link_status, intel_dp->lane_count)) {
> > DRM_DEBUG_KMS("clock recovery OK\n");
> > + intel_dp->train_set_valid = is_edp(intel_dp);
>
> Ouch, that was hard to spot amongst the decoys. How about setting
> intel_dp->train_set_valid = false at the very start of training, and
> only on success set it to true, something like
>

Or like I suggested, just set train_set_valid to false in the
failure_handling and set it to true only on success.

Manasi

> @@ -316,6 +316,8 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
> {
> struct intel_connector *intel_connector = intel_dp->attached_connector;
>
> + intel_dp->train_set_valid = false;
> +
> if (!intel_dp_link_training_clock_recovery(intel_dp))
> goto failure_handling;
> if (!intel_dp_link_training_channel_equalization(intel_dp))
> @@ -323,6 +325,11 @@ intel_dp_start_link_train(struct intel_dp *intel_dp)
>
> DRM_DEBUG_KMS("Link Training Passed at Link Rate = %d, Lane count = %d",
> intel_dp->link_rate, intel_dp->lane_count);
> + /*
> + * Record the last known working parameters for quick retraining
> + * next time. We only trust eDP for now.
> + */
> + intel_dp->train_set_valid = is_edp(intel_dp);
> return;
>
> failure_handling:
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/intel-gfx