[PATCH] drm/i915/dp: Don't stop the link when retraining

[PATCH] drm/i915/dp: Don't stop the link when retraining

[Daniel Vetter]

On pre-ddi platforms we don't shut down the link when changing link
training parameters. Except when clock recovery fails too hard and we
restart with channel eq training. Which doesn't make a lot of sense
really, since just stopping/restarting the DP port at this point
violates the modeset sequence documented in the Bspec.

So let's tempt fate and try this.

This patch is motivated by a WARN_ON triggered by

commit bc76e320f21f8bd790a72bd5dc06909617432352
Author: Daniel Vetter <daniel.vetter@ffwll.ch>
Date:   Tue May 20 22:46:50 2014 +0200

    drm/i915: Drop now misleading DDI comment from dp_link_down

References: https://bugs.freedesktop.org/show_bug.cgi?id=85670
Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
---
 drivers/gpu/drm/i915/intel_dp.c | 1 -
 1 file changed, 1 deletion(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index f6a3fdd5589e..e48ca3a87199 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -3598,7 +3598,6 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 
 		/* 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);
 			intel_dp_set_link_train(intel_dp, &DP,
 						training_pattern |
-- 
2.1.1

_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Don't stop the link when retraining

[Jesse Barnes]

On Mon,  3 Nov 2014 11:39:24 +0100
Daniel Vetter <daniel.vetter@ffwll.ch> wrote:

> On pre-ddi platforms we don't shut down the link when changing link
> training parameters. Except when clock recovery fails too hard and we
> restart with channel eq training. Which doesn't make a lot of sense
> really, since just stopping/restarting the DP port at this point
> violates the modeset sequence documented in the Bspec.
> 
> So let's tempt fate and try this.
> 
> This patch is motivated by a WARN_ON triggered by
> 
> commit bc76e320f21f8bd790a72bd5dc06909617432352
> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> Date:   Tue May 20 22:46:50 2014 +0200
> 
>     drm/i915: Drop now misleading DDI comment from dp_link_down
> 
> References: https://bugs.freedesktop.org/show_bug.cgi?id=85670
> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> ---
>  drivers/gpu/drm/i915/intel_dp.c | 1 -
>  1 file changed, 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_dp.c
> b/drivers/gpu/drm/i915/intel_dp.c index f6a3fdd5589e..e48ca3a87199
> 100644 --- a/drivers/gpu/drm/i915/intel_dp.c
> +++ b/drivers/gpu/drm/i915/intel_dp.c
> @@ -3598,7 +3598,6 @@ intel_dp_complete_link_train(struct intel_dp
> *intel_dp) 
>  		/* 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);
>  			intel_dp_set_link_train(intel_dp, &DP,
>  						training_pattern |


Didn't look like it helped the reporter?  Or at least I didn't see it
tried in the bug above...

I'm a bit worried about this because istr the spec indicating that we
do need to down the link when retrying clock recovery.  I guess I'll
need to check again.
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Don't stop the link when retraining

[Ville Syrjälä]

On Mon, Nov 10, 2014 at 10:01:56AM -0800, Jesse Barnes wrote:
> On Mon,  3 Nov 2014 11:39:24 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> 
> > On pre-ddi platforms we don't shut down the link when changing link
> > training parameters. Except when clock recovery fails too hard and we
> > restart with channel eq training. Which doesn't make a lot of sense
> > really, since just stopping/restarting the DP port at this point
> > violates the modeset sequence documented in the Bspec.
> > 
> > So let's tempt fate and try this.
> > 
> > This patch is motivated by a WARN_ON triggered by
> > 
> > commit bc76e320f21f8bd790a72bd5dc06909617432352
> > Author: Daniel Vetter <daniel.vetter@ffwll.ch>
> > Date:   Tue May 20 22:46:50 2014 +0200
> > 
> >     drm/i915: Drop now misleading DDI comment from dp_link_down
> > 
> > References: https://bugs.freedesktop.org/show_bug.cgi?id=85670
> > Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
> > ---
> >  drivers/gpu/drm/i915/intel_dp.c | 1 -
> >  1 file changed, 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_dp.c
> > b/drivers/gpu/drm/i915/intel_dp.c index f6a3fdd5589e..e48ca3a87199
> > 100644 --- a/drivers/gpu/drm/i915/intel_dp.c
> > +++ b/drivers/gpu/drm/i915/intel_dp.c
> > @@ -3598,7 +3598,6 @@ intel_dp_complete_link_train(struct intel_dp
> > *intel_dp) 
> >  		/* 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);
> >  			intel_dp_set_link_train(intel_dp, &DP,
> >  						training_pattern |
> 
> 
> Didn't look like it helped the reporter?  Or at least I didn't see it
> tried in the bug above...
> 
> I'm a bit worried about this because istr the spec indicating that we
> do need to down the link when retrying clock recovery.  I guess I'll
> need to check again.

I had a similar notion in my head. Can't recall where I saw it exactly.

But the current code is a bit inconistent anyway since it doesn't call
intel_dp_link_down() in all the failure cases. Which combined with an
initially stuck power sequencer on chv sometimes resulted in success
on the first retry and sometimes all the retries would just fail,
entirely depending on which codepath it used when restarting the link
training. I never bothered figuring out what really made it pick one
path over the other, but IIRC it was totally consistent in its
choice for a given combination of modeset operations.

Anyway, I suppose if we really want to follow the correct restart
procedure we might have to shut down _everything_ and start again from
the top. But that seems rather difficult to do given that the link
training isn't driving the entire modeset sequence for us.

-- 
Ville Syrjälä
Intel OTC
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/dp: Don't stop the link when retraining

[Jani Nikula]

On Mon, 10 Nov 2014, Jesse Barnes <jbarnes@virtuousgeek.org> wrote:
> On Mon,  3 Nov 2014 11:39:24 +0100
> Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
>
>> On pre-ddi platforms we don't shut down the link when changing link
>> training parameters. Except when clock recovery fails too hard and we
>> restart with channel eq training. Which doesn't make a lot of sense
>> really, since just stopping/restarting the DP port at this point
>> violates the modeset sequence documented in the Bspec.
>> 
>> So let's tempt fate and try this.
>> 
>> This patch is motivated by a WARN_ON triggered by
>> 
>> commit bc76e320f21f8bd790a72bd5dc06909617432352
>> Author: Daniel Vetter <daniel.vetter@ffwll.ch>
>> Date:   Tue May 20 22:46:50 2014 +0200
>> 
>>     drm/i915: Drop now misleading DDI comment from dp_link_down
>> 
>> References: https://bugs.freedesktop.org/show_bug.cgi?id=85670
>> Signed-off-by: Daniel Vetter <daniel.vetter@intel.com>
>> ---
>>  drivers/gpu/drm/i915/intel_dp.c | 1 -
>>  1 file changed, 1 deletion(-)
>> 
>> diff --git a/drivers/gpu/drm/i915/intel_dp.c
>> b/drivers/gpu/drm/i915/intel_dp.c index f6a3fdd5589e..e48ca3a87199
>> 100644 --- a/drivers/gpu/drm/i915/intel_dp.c
>> +++ b/drivers/gpu/drm/i915/intel_dp.c
>> @@ -3598,7 +3598,6 @@ intel_dp_complete_link_train(struct intel_dp
>> *intel_dp) 
>>  		/* 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);
>>  			intel_dp_set_link_train(intel_dp, &DP,
>>  						training_pattern |
>
>
> Didn't look like it helped the reporter?  Or at least I didn't see it
> tried in the bug above...

This wouldn't fix the bug, this would fix the backtrace from
WARN_ON(HAS_DDI(dev)) in intel_dp_link_down that's also present in the
bug. Just one more item in the long list of things that suck about our
DP link training.

BR,
Jani.


>
> I'm a bit worried about this because istr the spec indicating that we
> do need to down the link when retrying clock recovery.  I guess I'll
> need to check again.
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Jani Nikula, Intel Open Source Technology Center
_______________________________________________
Intel-gfx mailing list
Intel-gfx@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/intel-gfx