[PATCH] drm/i915: make DP training try a little harder

[PATCH] drm/i915: make DP training try a little harder

[Jesse Barnes]

When trying to do channel equalization, we need to make sure we still
have clock recovery on all lanes while training.  We also need to try
clock recovery again if we lose the clock or if channel eq fails 5
times.  We'll try clock recovery up to 5 more times before giving up
entirely.

Gets suspend/resume working on my Vaio again and brings us back into
compliance with the DP training sequence spec.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   25 ++++++++++++++++++++++---
 1 files changed, 22 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1dc6040..35152cb 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1334,12 +1334,13 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool channel_eq = false;
-	int tries;
+	int tries, cr_tries;
 	u32 reg;
 	uint32_t DP = intel_dp->DP;
 
 	/* channel equalization */
 	tries = 0;
+	cr_tries = 0;
 	channel_eq = false;
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
@@ -1367,18 +1368,36 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		if (!intel_dp_get_link_status(intel_dp))
 			break;
 
+		/* Make sure clock is still ok */
+		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
+			intel_dp_start_link_train(intel_dp);
+			cr_tries++;
+			continue;
+		}
+
 		if (intel_channel_eq_ok(intel_dp)) {
 			channel_eq = true;
 			break;
 		}
 
-		/* Try 5 times */
-		if (tries > 5)
+		/* Try 5 times, then try clock recovery if that fails */
+		if (tries > 5) {
+			intel_dp_link_down(intel_dp);
+			intel_dp_start_link_train(intel_dp);
+			tries = 0;
+			cr_tries++;
 			break;
+		}
 
 		/* Compute new intel_dp->train_set as requested by target */
 		intel_get_adjust_train(intel_dp);
 		++tries;
+
+		if (cr_tries > 5) {
+			DRM_ERROR("failed to train DP, aborting\n");
+			intel_dp_link_down(intel_dp);
+			break;
+		}
 	}
 
 	if (HAS_PCH_CPT(dev) && !is_edp(intel_dp))
-- 
1.7.0.4

Re: [PATCH] drm/i915: make DP training try a little harder

[Yuanhan Liu]

On Tue, Jan 04, 2011 at 11:53:14AM -0800, Jesse Barnes wrote:
> When trying to do channel equalization, we need to make sure we still
> have clock recovery on all lanes while training.  We also need to try
> clock recovery again if we lose the clock or if channel eq fails 5
> times.  We'll try clock recovery up to 5 more times before giving up
> entirely.
> 
> Gets suspend/resume working on my Vaio again and brings us back into
> compliance with the DP training sequence spec.
> 
> Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> ---
>  drivers/gpu/drm/i915/intel_dp.c |   25 ++++++++++++++++++++++---
>  1 files changed, 22 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> index 1dc6040..35152cb 100644
> --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -1334,12 +1334,13 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  	struct drm_device *dev = intel_dp->base.base.dev;
>  	struct drm_i915_private *dev_priv = dev->dev_private;
>  	bool channel_eq = false;
> -	int tries;
> +	int tries, cr_tries;
>  	u32 reg;
>  	uint32_t DP = intel_dp->DP;
>  
>  	/* channel equalization */
>  	tries = 0;
> +	cr_tries = 0;
>  	channel_eq = false;
>  	for (;;) {
>  		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
> @@ -1367,18 +1368,36 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
>  		if (!intel_dp_get_link_status(intel_dp))
>  			break;
>  
> +		/* Make sure clock is still ok */
> +		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
> +			intel_dp_start_link_train(intel_dp);
> +			cr_tries++;
> +			continue;
> +		}
> +
Hi Jesse,

This will cause a endless loop when the clock recovery phase always
failed, just like what happend on my test machine.


And, FYI, the two patch would not fix bug
https://bugs.freedesktop.org/show_bug.cgi?id=32539
the train link still failed.


Thanks, 
Yuanhan Liu


>  		if (intel_channel_eq_ok(intel_dp)) {
>  			channel_eq = true;
>  			break;
>  		}
>  
> -		/* Try 5 times */
> -		if (tries > 5)
> +		/* Try 5 times, then try clock recovery if that fails */
> +		if (tries > 5) {
> +			intel_dp_link_down(intel_dp);
> +			intel_dp_start_link_train(intel_dp);
> +			tries = 0;
> +			cr_tries++;
>  			break;
> +		}
>  
>  		/* Compute new intel_dp->train_set as requested by target */
>  		intel_get_adjust_train(intel_dp);
>  		++tries;
> +
> +		if (cr_tries > 5) {
> +			DRM_ERROR("failed to train DP, aborting\n");
> +			intel_dp_link_down(intel_dp);
> +			break;
> +		}
>  	}
>  
>  	if (HAS_PCH_CPT(dev) && !is_edp(intel_dp))
> -- 
> 1.7.0.4
> 
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915: make DP training try a little harder

[Chris Wilson]

On Tue,  4 Jan 2011 11:53:14 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> When trying to do channel equalization, we need to make sure we still
> have clock recovery on all lanes while training.  We also need to try
> clock recovery again if we lose the clock or if channel eq fails 5
> times.  We'll try clock recovery up to 5 more times before giving up
> entirely.
> 
> Gets suspend/resume working on my Vaio again and brings us back into
> compliance with the DP training sequence spec.

There are still ways link training can silently fail. The next task for
the foolhardy is return error codes from modesetting (and accept that any
failure can leave the system in a partially defined state, with a forced
mode reset in the drm_kms_helper?).

> -		/* Try 5 times */
> -		if (tries > 5)
> +		/* Try 5 times, then try clock recovery if that fails */
> +		if (tries > 5) {
> +			intel_dp_link_down(intel_dp);
> +			intel_dp_start_link_train(intel_dp);
> +			tries = 0;
> +			cr_tries++;
>  			break;

This should be a continue, right?.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: make DP training try a little harder

[Jesse Barnes]

On Wed, 05 Jan 2011 10:43:41 +0000
Chris Wilson <chris@chris-wilson.co.uk> wrote:

> On Tue,  4 Jan 2011 11:53:14 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> > When trying to do channel equalization, we need to make sure we still
> > have clock recovery on all lanes while training.  We also need to try
> > clock recovery again if we lose the clock or if channel eq fails 5
> > times.  We'll try clock recovery up to 5 more times before giving up
> > entirely.
> > 
> > Gets suspend/resume working on my Vaio again and brings us back into
> > compliance with the DP training sequence spec.
> 
> There are still ways link training can silently fail. The next task for
> the foolhardy is return error codes from modesetting (and accept that any
> failure can leave the system in a partially defined state, with a forced
> mode reset in the drm_kms_helper?).
> 
> > -		/* Try 5 times */
> > -		if (tries > 5)
> > +		/* Try 5 times, then try clock recovery if that fails */
> > +		if (tries > 5) {
> > +			intel_dp_link_down(intel_dp);
> > +			intel_dp_start_link_train(intel_dp);
> > +			tries = 0;
> > +			cr_tries++;
> >  			break;
> 
> This should be a continue, right?.

Oops, yeah the loop is wrong; the cr_tries check needs to be above the
first continue as well.  Will fix, but it sounds like it won't help
with Yuanhan's problem...

-- 
Jesse Barnes, Intel Open Source Technology Center

Re: [PATCH] drm/i915: make DP training try a little harder

[Jesse Barnes]

On Wed, 5 Jan 2011 15:52:39 +0800
Yuanhan Liu <yuanhan.liu@linux.intel.com> wrote:

> On Tue, Jan 04, 2011 at 11:53:14AM -0800, Jesse Barnes wrote:
> > When trying to do channel equalization, we need to make sure we still
> > have clock recovery on all lanes while training.  We also need to try
> > clock recovery again if we lose the clock or if channel eq fails 5
> > times.  We'll try clock recovery up to 5 more times before giving up
> > entirely.
> > 
> > Gets suspend/resume working on my Vaio again and brings us back into
> > compliance with the DP training sequence spec.
> > 
> > Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c |   25 ++++++++++++++++++++++---
> >  1 files changed, 22 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
> > index 1dc6040..35152cb 100644
> > --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -1334,12 +1334,13 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> >  	struct drm_device *dev = intel_dp->base.base.dev;
> >  	struct drm_i915_private *dev_priv = dev->dev_private;
> >  	bool channel_eq = false;
> > -	int tries;
> > +	int tries, cr_tries;
> >  	u32 reg;
> >  	uint32_t DP = intel_dp->DP;
> >  
> >  	/* channel equalization */
> >  	tries = 0;
> > +	cr_tries = 0;
> >  	channel_eq = false;
> >  	for (;;) {
> >  		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
> > @@ -1367,18 +1368,36 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
> >  		if (!intel_dp_get_link_status(intel_dp))
> >  			break;
> >  
> > +		/* Make sure clock is still ok */
> > +		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
> > +			intel_dp_start_link_train(intel_dp);
> > +			cr_tries++;
> > +			continue;
> > +		}
> > +
> Hi Jesse,
> 
> This will cause a endless loop when the clock recovery phase always
> failed, just like what happend on my test machine.
> 
> 
> And, FYI, the two patch would not fix bug
> https://bugs.freedesktop.org/show_bug.cgi?id=32539
> the train link still failed.

Does this one at least keep it from hanging?

If it doesn't actually help with suspend/resume, I wonder if we need to
use fast link training on that machine.  The "use VBT provided values"
patch did that, using the VBT pre-emphasis and vswing values, but we
can also cache the last known good AUX handshake data and use that at
resume time too.  Can you give that a try?

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center

>From fdb4552b4da707bfa5f3ab9e5dd92f04b39e41d3 Mon Sep 17 00:00:00 2001
From: Jesse Barnes <jbarnes@virtuousgeek.org>
Date: Tue, 4 Jan 2011 11:50:32 -0800
Subject: [PATCH] drm/i915: make DP training try a little harder

When trying to do channel equalization, we need to make sure we still
have clock recovery on all lanes while training.  We also need to try
clock recovery again if we lose the clock or if channel eq fails 5
times.  We'll try clock recovery up to 5 more times before giving up
entirely.

Gets suspend/resume working on my Vaio again and brings us back into
compliance with the DP training sequence spec.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1dc6040..c768e30 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1334,17 +1334,24 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool channel_eq = false;
-	int tries;
+	int tries, cr_tries;
 	u32 reg;
 	uint32_t DP = intel_dp->DP;
 
 	/* channel equalization */
 	tries = 0;
+	cr_tries = 0;
 	channel_eq = false;
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
 		uint32_t    signal_levels;
 
+		if (cr_tries > 5) {
+			DRM_ERROR("failed to train DP, aborting\n");
+			intel_dp_link_down(intel_dp);
+			break;
+		}
+
 		if (IS_GEN6(dev) && is_edp(intel_dp)) {
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
@@ -1367,14 +1374,26 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		if (!intel_dp_get_link_status(intel_dp))
 			break;
 
+		/* Make sure clock is still ok */
+		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
+			intel_dp_start_link_train(intel_dp);
+			cr_tries++;
+			continue;
+		}
+
 		if (intel_channel_eq_ok(intel_dp)) {
 			channel_eq = true;
 			break;
 		}
 
-		/* Try 5 times */
-		if (tries > 5)
-			break;
+		/* Try 5 times, then try clock recovery if that fails */
+		if (tries > 5) {
+			intel_dp_link_down(intel_dp);
+			intel_dp_start_link_train(intel_dp);
+			tries = 0;
+			cr_tries++;
+			continue;
+		}
 
 		/* Compute new intel_dp->train_set as requested by target */
 		intel_get_adjust_train(intel_dp);
-- 
1.7.0.4

Re: drm/i915: make DP training try a little harder

[Jan-Hendrik Zab]

On 05/01/11 09:19 -0800, Jesse Barnes wrote:
[snip]

> Jesse Barnes, Intel Open Source Technology Center
> 
> From fdb4552b4da707bfa5f3ab9e5dd92f04b39e41d3 Mon Sep 17 00:00:00 2001
> From: Jesse Barnes <jbarnes@virtuousgeek.org>
> Date: Tue, 4 Jan 2011 11:50:32 -0800
> Subject: [PATCH] drm/i915: make DP training try a little harder
> 
> When trying to do channel equalization, we need to make sure we still
> have clock recovery on all lanes while training.  We also need to try
> clock recovery again if we lose the clock or if channel eq fails 5
> times.  We'll try clock recovery up to 5 more times before giving up
> entirely.
> 
> Gets suspend/resume working on my Vaio again and brings us back into
> compliance with the DP training sequence spec.

Hey,
the patch also gets suspend/resume working on my Vaio (VPCZ11C5E).

Same goes for the first patch in the thread.

	-jhz

[PATCH] drm/i915: make DP training try a little harder

[Jesse Barnes]

When trying to do channel equalization, we need to make sure we still
have clock recovery on all lanes while training.  We also need to try
clock recovery again if we lose the clock or if channel eq fails 5
times.  We'll try clock recovery up to 5 more times before giving up
entirely.

Gets suspend/resume working on my Vaio again and brings us back into
compliance with the DP training sequence spec.

Signed-off-by: Jesse Barnes <jbarnes@virtuousgeek.org>
---
 drivers/gpu/drm/i915/intel_dp.c |   27 +++++++++++++++++++++++----
 1 files changed, 23 insertions(+), 4 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index 1dc6040..c768e30 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -1334,17 +1334,24 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 	struct drm_device *dev = intel_dp->base.base.dev;
 	struct drm_i915_private *dev_priv = dev->dev_private;
 	bool channel_eq = false;
-	int tries;
+	int tries, cr_tries;
 	u32 reg;
 	uint32_t DP = intel_dp->DP;
 
 	/* channel equalization */
 	tries = 0;
+	cr_tries = 0;
 	channel_eq = false;
 	for (;;) {
 		/* Use intel_dp->train_set[0] to set the voltage and pre emphasis values */
 		uint32_t    signal_levels;
 
+		if (cr_tries > 5) {
+			DRM_ERROR("failed to train DP, aborting\n");
+			intel_dp_link_down(intel_dp);
+			break;
+		}
+
 		if (IS_GEN6(dev) && is_edp(intel_dp)) {
 			signal_levels = intel_gen6_edp_signal_levels(intel_dp->train_set[0]);
 			DP = (DP & ~EDP_LINK_TRAIN_VOL_EMP_MASK_SNB) | signal_levels;
@@ -1367,14 +1374,26 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 		if (!intel_dp_get_link_status(intel_dp))
 			break;
 
+		/* Make sure clock is still ok */
+		if (!intel_clock_recovery_ok(intel_dp->link_status, intel_dp->lane_count)) {
+			intel_dp_start_link_train(intel_dp);
+			cr_tries++;
+			continue;
+		}
+
 		if (intel_channel_eq_ok(intel_dp)) {
 			channel_eq = true;
 			break;
 		}
 
-		/* Try 5 times */
-		if (tries > 5)
-			break;
+		/* Try 5 times, then try clock recovery if that fails */
+		if (tries > 5) {
+			intel_dp_link_down(intel_dp);
+			intel_dp_start_link_train(intel_dp);
+			tries = 0;
+			cr_tries++;
+			continue;
+		}
 
 		/* Compute new intel_dp->train_set as requested by target */
 		intel_get_adjust_train(intel_dp);
-- 
1.7.0.4

Re: [PATCH] drm/i915: make DP training try a little harder

[Yuanhan Liu]

On Wed, Jan 05, 2011 at 09:19:19AM -0800, Jesse Barnes wrote:

[snip]

> > 
> > And, FYI, the two patch would not fix bug
> > https://bugs.freedesktop.org/show_bug.cgi?id=32539
> > the train link still failed.
> 
> Does this one at least keep it from hanging?

Oh, sorry, wrong bug number :( , it's 29791.
> 
> If it doesn't actually help with suspend/resume, I wonder if we need to
> use fast link training on that machine.  The "use VBT provided values"
> patch did that, using the VBT pre-emphasis and vswing values, but we
> can also cache the last known good AUX handshake data and use that at
> resume time too.  Can you give that a try?

Re: [PATCH] drm/i915: make DP training try a little harder

[Yuanhan Liu]

On Wed, Jan 05, 2011 at 02:45:24PM -0800, Jesse Barnes wrote:
> When trying to do channel equalization, we need to make sure we still
> have clock recovery on all lanes while training.  We also need to try
> clock recovery again if we lose the clock or if channel eq fails 5
> times.  We'll try clock recovery up to 5 more times before giving up
> entirely.
> 
> Gets suspend/resume working on my Vaio again and brings us back into
> compliance with the DP training sequence spec.

This fixes the suspend/resume issue on my side, too.

Re: [PATCH] drm/i915: make DP training try a little harder

[Chris Wilson]

On Wed,  5 Jan 2011 14:45:24 -0800, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> When trying to do channel equalization, we need to make sure we still
> have clock recovery on all lanes while training.  We also need to try
> clock recovery again if we lose the clock or if channel eq fails 5
> times.  We'll try clock recovery up to 5 more times before giving up
> entirely.
> 
> Gets suspend/resume working on my Vaio again and brings us back into
> compliance with the DP training sequence spec.

Applied to the pending -fixes. Will now push to -staging.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre