| Message ID | 1415011164-31402-1-git-send-email-daniel.vetter@ffwll.ch (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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.
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
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 |
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(-)