[PATCH 1/2] drm/i915: don't disable the DP port if the link is lost

[PATCH 1/2] drm/i915: don't disable the DP port if the link is lost

[Imre Deak]

Currently if the DP link is lost (either because of a hot unplug, or
failed link status check) we disable the DP port, but leave the rest
of the pipe running. This is incompatible with the modeset disabling
sequence of some platforms/configurations. At least this is the case for
DP ports on the CPU as opposed to PCH.

Atm we'll also get a warning when we do a modeset disable after the
above link lost event, since we expect the DP port to be enabled at this
point (see the bugzilla ticket for the related dmesg).

Note that with this patch we'll still end up disabling the port, thanks
to the HPD uevent and subsequent modeset disable.

See also the next patch fixing the other half of this issue.

Solution suggested by Ville.

Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=70570
Signed-off-by: Imre Deak <imre.deak@intel.com>

	unload
---
 drivers/gpu/drm/i915/intel_dp.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c
index e265488..0c62035 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2891,13 +2891,11 @@ intel_dp_check_link_status(struct intel_dp *intel_dp)
 
 	/* Try to read receiver status if the link appears to be up */
 	if (!intel_dp_get_link_status(intel_dp, link_status)) {
-		intel_dp_link_down(intel_dp);
 		return;
 	}
 
 	/* Now read the DPCD to see if it's actually running */
 	if (!intel_dp_get_dpcd(intel_dp)) {
-		intel_dp_link_down(intel_dp);
 		return;
 	}
 
-- 
1.8.4

[PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Imre Deak]

The driver shouldn't disable the DP port itself, but let userspace do it
through a modeset. See the previous patch for the reasoning.

Keeping this as a separate patch for bisectability.

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@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 0c62035..d3371f5 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2638,7 +2638,6 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 
 		if (cr_tries > 5) {
 			DRM_ERROR("failed to train DP, aborting\n");
-			intel_dp_link_down(intel_dp);
 			break;
 		}
 
-- 
1.8.4

Re: [PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Chris Wilson]

On Thu, Jan 16, 2014 at 06:35:58PM +0200, Imre Deak wrote:
> The driver shouldn't disable the DP port itself, but let userspace do it
> through a modeset. See the previous patch for the reasoning.

Eh, this occurs not just during link detection, but also during
intel_enable_dp, so this comment does not hold.  The biggest
issue of this function is that it can fail but never propagates
that failure, which itself breaks the users and API expectations,
as neither the driver or userspace is aware that it is required to
takeaction.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Daniel Vetter]

On Thu, Jan 16, 2014 at 5:58 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> On Thu, Jan 16, 2014 at 06:35:58PM +0200, Imre Deak wrote:
>> The driver shouldn't disable the DP port itself, but let userspace do it
>> through a modeset. See the previous patch for the reasoning.
>
> Eh, this occurs not just during link detection, but also during
> intel_enable_dp, so this comment does not hold.  The biggest
> issue of this function is that it can fail but never propagates
> that failure, which itself breaks the users and API expectations,
> as neither the driver or userspace is aware that it is required to
> takeaction.

Yeah, but imo that's a separate issue - atm we kill the DP port, which
stops the pipe, which is something our code never expects. Resulting
in piles of funny bug reports from angry users who's machine got stuck
because of this.
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Chris Wilson]

On Thu, Jan 16, 2014 at 06:01:28PM +0100, Daniel Vetter wrote:
> On Thu, Jan 16, 2014 at 5:58 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > On Thu, Jan 16, 2014 at 06:35:58PM +0200, Imre Deak wrote:
> >> The driver shouldn't disable the DP port itself, but let userspace do it
> >> through a modeset. See the previous patch for the reasoning.
> >
> > Eh, this occurs not just during link detection, but also during
> > intel_enable_dp, so this comment does not hold.  The biggest
> > issue of this function is that it can fail but never propagates
> > that failure, which itself breaks the users and API expectations,
> > as neither the driver or userspace is aware that it is required to
> > takeaction.
> 
> Yeah, but imo that's a separate issue - atm we kill the DP port, which
> stops the pipe, which is something our code never expects. Resulting
> in piles of funny bug reports from angry users who's machine got stuck
> because of this.

But this alone will not stop the bug reports - the display will still be
blank but kernel/userspace will continue to believe that the modeset
took place.

I did not say that the patch was not sensible, just insufficient ;-)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Daniel Vetter]

On Thu, Jan 16, 2014 at 08:39:38PM +0000, Chris Wilson wrote:
> On Thu, Jan 16, 2014 at 06:01:28PM +0100, Daniel Vetter wrote:
> > On Thu, Jan 16, 2014 at 5:58 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> > > On Thu, Jan 16, 2014 at 06:35:58PM +0200, Imre Deak wrote:
> > >> The driver shouldn't disable the DP port itself, but let userspace do it
> > >> through a modeset. See the previous patch for the reasoning.
> > >
> > > Eh, this occurs not just during link detection, but also during
> > > intel_enable_dp, so this comment does not hold.  The biggest
> > > issue of this function is that it can fail but never propagates
> > > that failure, which itself breaks the users and API expectations,
> > > as neither the driver or userspace is aware that it is required to
> > > takeaction.
> > 
> > Yeah, but imo that's a separate issue - atm we kill the DP port, which
> > stops the pipe, which is something our code never expects. Resulting
> > in piles of funny bug reports from angry users who's machine got stuck
> > because of this.
> 
> But this alone will not stop the bug reports - the display will still be
> blank but kernel/userspace will continue to believe that the modeset
> took place.
> 
> I did not say that the patch was not sensible, just insufficient ;-)

Well if we're lucky the next modeset works, or the 2nd display still
works, or at least X isn't stuck and maybe apps can still autosave. I
agree it's not the full solution, but it should be quite a bit better than
what we currently do ;-)

The other issue is that atm I don't have a good idea for how we could wire
this up correctly. In a way for all other outputs we don't tell userspace
that stuff went wrong either ...
-Daniel
-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch

Re: [PATCH 2/2] drm/i915: don't disable DP port after a failed link training

[Imre Deak]

[-- Attachment #1.1: Type: text/plain, Size: 941 bytes --]

On Thu, 2014-01-16 at 16:58 +0000, Chris Wilson wrote:
> On Thu, Jan 16, 2014 at 06:35:58PM +0200, Imre Deak wrote:
> > The driver shouldn't disable the DP port itself, but let userspace do it
> > through a modeset. See the previous patch for the reasoning.
> 
> Eh, this occurs not just during link detection, but also during
> intel_enable_dp, so this comment does not hold.  The biggest
> issue of this function is that it can fail but never propagates
> that failure, which itself breaks the users and API expectations,
> as neither the driver or userspace is aware that it is required to
> takeaction.

Right, I haven't thought about this and the commit message is incorrect.
So in the end with proper error handling it's gonna be the kernel that
disables the port along with the rest of the pipe.

I agree with danvet, that we could still have this before a final
solution. (with a fixed commit message).

--Imre


[-- Attachment #1.2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

[-- Attachment #2: Type: text/plain, Size: 159 bytes --]

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

[PATCH] drm/i915: don't disable DP port after a failed link training

[Imre Deak]

Atm after a failed link training we disable the DP port. This can happen
during a modeset-enable or a DP link re-establishment. The latter can be
a problem and we shouldn't disable the DP port, see the previous patch for
the reasoning. In the former case the right thing would be to disable
the DP port, but also the rest of the pipe.

As a stop-gap solution leave the DP port enabled in both cases. It is an
improvement on its own (avoiding HW lock ups) and the proper solution
for the first case requires a bigger change, so let's keep that on the
TODO list.

v2:
- fix explanation of change impact (Chris)

Suggested-by: Daniel Vetter <daniel@ffwll.ch>
Signed-off-by: Imre Deak <imre.deak@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 7b630e9..689b832 100644
--- a/drivers/gpu/drm/i915/intel_dp.c
+++ b/drivers/gpu/drm/i915/intel_dp.c
@@ -2638,7 +2638,6 @@ intel_dp_complete_link_train(struct intel_dp *intel_dp)
 
 		if (cr_tries > 5) {
 			DRM_ERROR("failed to train DP, aborting\n");
-			intel_dp_link_down(intel_dp);
 			break;
 		}
 
-- 
1.8.4

Re: [PATCH] drm/i915: don't disable DP port after a failed link training

[Chris Wilson]

On Fri, Jan 17, 2014 at 03:46:43PM +0200, Imre Deak wrote:
> Atm after a failed link training we disable the DP port. This can happen
> during a modeset-enable or a DP link re-establishment. The latter can be
> a problem and we shouldn't disable the DP port, see the previous patch for
> the reasoning. In the former case the right thing would be to disable
> the DP port, but also the rest of the pipe.
> 
> As a stop-gap solution leave the DP port enabled in both cases. It is an
> improvement on its own (avoiding HW lock ups) and the proper solution
> for the first case requires a bigger change, so let's keep that on the
> TODO list.
> 
> v2:
> - fix explanation of change impact (Chris)
> 
> Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> Signed-off-by: Imre Deak <imre.deak@intel.com>
Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
and r-b for patch 1 as well (if I didn't send it).
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915: don't disable DP port after a failed link training

[Daniel Vetter]

On Fri, Jan 17, 2014 at 01:58:43PM +0000, Chris Wilson wrote:
> On Fri, Jan 17, 2014 at 03:46:43PM +0200, Imre Deak wrote:
> > Atm after a failed link training we disable the DP port. This can happen
> > during a modeset-enable or a DP link re-establishment. The latter can be
> > a problem and we shouldn't disable the DP port, see the previous patch for
> > the reasoning. In the former case the right thing would be to disable
> > the DP port, but also the rest of the pipe.
> > 
> > As a stop-gap solution leave the DP port enabled in both cases. It is an
> > improvement on its own (avoiding HW lock ups) and the proper solution
> > for the first case requires a bigger change, so let's keep that on the
> > TODO list.
> > 
> > v2:
> > - fix explanation of change impact (Chris)
> > 
> > Suggested-by: Daniel Vetter <daniel@ffwll.ch>
> > Signed-off-by: Imre Deak <imre.deak@intel.com>
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> and r-b for patch 1 as well (if I didn't send it).

Both merged, thanks.
-Daniel
> -Chris
> 
> -- 
> Chris Wilson, Intel Open Source Technology Centre
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

-- 
Daniel Vetter
Software Engineer, Intel Corporation
+41 (0) 79 365 57 48 - http://blog.ffwll.ch