[PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

Whilst most monitors do wire up the HPD presence pin, it seems quite a
few KVM do not. Therefore if we simply rely on the HPD pin being
asserted to indicate a connected monitor we fail miserable, so fall back
to performing a DCC query for the EDID.

Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |    8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index f0223d0..804611e 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -453,13 +453,15 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	struct intel_load_detect_pipe tmp;
 
 	if (I915_HAS_HOTPLUG(dev)) {
+		/* We can not rely on the HPD pin always being correctly wired
+		 * up, for example many KVM do not pass it through, and so
+		 * only trust an assertion that the monitor is connected.
+		 */
 		if (intel_crt_detect_hotplug(connector)) {
 			DRM_DEBUG_KMS("CRT detected via hotplug\n");
 			return connector_status_connected;
-		} else {
+		} else
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
-			return connector_status_disconnected;
-		}
 	}
 
 	if (intel_crt_detect_ddc(connector))
-- 
1.7.10

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

On Thu, May 31, 2012 at 01:08:53PM +0100, Chris Wilson wrote:
> Whilst most monitors do wire up the HPD presence pin, it seems quite a
> few KVM do not. Therefore if we simply rely on the HPD pin being
> asserted to indicate a connected monitor we fail miserable, so fall back
> to performing a DCC query for the EDID.
> 
> Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

*sigh*

Picked up for -fixes, thanks for the patch.
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Dave Airlie]

On Thu, May 31, 2012 at 1:08 PM, Chris Wilson <chris@chris-wilson.co.uk> wrote:
> Whilst most monitors do wire up the HPD presence pin, it seems quite a
> few KVM do not. Therefore if we simply rely on the HPD pin being
> asserted to indicate a connected monitor we fail miserable, so fall back
> to performing a DCC query for the EDID.

This message is wrong though, the pin is a lie, there is no HPD pin on
the standard
VGA cable.

VGA load detect is done by measuring the resistance across some pins,
a lot of kvms use bad resistor values, should be 50Ohm, someone got some cheap
75Ohm from a TV or something.

Dave.

>
> Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_crt.c |    8 +++++---
>  1 file changed, 5 insertions(+), 3 deletions(-)
>
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index f0223d0..804611e 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -453,13 +453,15 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>        struct intel_load_detect_pipe tmp;
>
>        if (I915_HAS_HOTPLUG(dev)) {
> +               /* We can not rely on the HPD pin always being correctly wired
> +                * up, for example many KVM do not pass it through, and so
> +                * only trust an assertion that the monitor is connected.
> +                */
>                if (intel_crt_detect_hotplug(connector)) {
>                        DRM_DEBUG_KMS("CRT detected via hotplug\n");
>                        return connector_status_connected;
> -               } else {
> +               } else
>                        DRM_DEBUG_KMS("CRT not detected via hotplug\n");
> -                       return connector_status_disconnected;
> -               }
>        }
>
>        if (intel_crt_detect_ddc(connector))
> --
> 1.7.10
>
> _______________________________________________
> Intel-gfx mailing list
> Intel-gfx@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/intel-gfx

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

On Thu, May 31, 2012 at 01:08:53PM +0100, Chris Wilson wrote:
> Whilst most monitors do wire up the HPD presence pin, it seems quite a
> few KVM do not. Therefore if we simply rely on the HPD pin being
> asserted to indicate a connected monitor we fail miserable, so fall back
> to performing a DCC query for the EDID.
> 
> Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Ok, this blew up ... Can you please resend, with Dave's suggestion for a
rectified commit message & comment and with a check added such that we
don't try to do load_detect on HAS_HOTPLUG machines - I guess it doesn't
work too well.

Thanks, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

On Sat, 9 Jun 2012 00:22:12 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, May 31, 2012 at 01:08:53PM +0100, Chris Wilson wrote:
> > Whilst most monitors do wire up the HPD presence pin, it seems quite a
> > few KVM do not. Therefore if we simply rely on the HPD pin being
> > asserted to indicate a connected monitor we fail miserable, so fall back
> > to performing a DCC query for the EDID.
> > 
> > Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Ok, this blew up ... Can you please resend, with Dave's suggestion for a
> rectified commit message & comment and with a check added such that we
> don't try to do load_detect on HAS_HOTPLUG machines - I guess it doesn't
> work too well.

I disagree, if we cannot trust the hw autodetection, then we know that
there are monitors/kvm that do not report an EDID and so we need to do
the whole shebang. Which will continue to annoy Linus since his machine
is behaving as expected given the circumstances.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

On Fri, Jun 08, 2012 at 11:23:10PM +0100, Chris Wilson wrote:
> On Sat, 9 Jun 2012 00:22:12 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > On Thu, May 31, 2012 at 01:08:53PM +0100, Chris Wilson wrote:
> > > Whilst most monitors do wire up the HPD presence pin, it seems quite a
> > > few KVM do not. Therefore if we simply rely on the HPD pin being
> > > asserted to indicate a connected monitor we fail miserable, so fall back
> > > to performing a DCC query for the EDID.
> > > 
> > > Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > Ok, this blew up ... Can you please resend, with Dave's suggestion for a
> > rectified commit message & comment and with a check added such that we
> > don't try to do load_detect on HAS_HOTPLUG machines - I guess it doesn't
> > work too well.
> 
> I disagree, if we cannot trust the hw autodetection, then we know that
> there are monitors/kvm that do not report an EDID and so we need to do
> the whole shebang. Which will continue to annoy Linus since his machine
> is behaving as expected given the circumstances.

Well, I don't disagree on doing the whole shebang. The proplem is that the
load-detect code as-is is gen3 only (and maybe gen4, haven't checked
that) - it surely can't work on pch split platforms if half the registers
we use in there are gone.

Until that is fixed and properly tested on all relevant platforms, we
should be able to help the bug reporters by simply using the edid
detection, but bailing on the load detect stuff for all HAS_HOTPLUG
platforms (as we do now already). I'll whip up a patch.

For actual load-detect stuff is imo -next material, and I think we should
dodge that bullet until we have an actual bug reporter wanting it ...
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

On Sun, 10 Jun 2012 19:04:10 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Fri, Jun 08, 2012 at 11:23:10PM +0100, Chris Wilson wrote:
> > On Sat, 9 Jun 2012 00:22:12 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> > > On Thu, May 31, 2012 at 01:08:53PM +0100, Chris Wilson wrote:
> > > > Whilst most monitors do wire up the HPD presence pin, it seems quite a
> > > > few KVM do not. Therefore if we simply rely on the HPD pin being
> > > > asserted to indicate a connected monitor we fail miserable, so fall back
> > > > to performing a DCC query for the EDID.
> > > > 
> > > > Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> > > > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > > > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > Ok, this blew up ... Can you please resend, with Dave's suggestion for a
> > > rectified commit message & comment and with a check added such that we
> > > don't try to do load_detect on HAS_HOTPLUG machines - I guess it doesn't
> > > work too well.
> > 
> > I disagree, if we cannot trust the hw autodetection, then we know that
> > there are monitors/kvm that do not report an EDID and so we need to do
> > the whole shebang. Which will continue to annoy Linus since his machine
> > is behaving as expected given the circumstances.
> 
> Well, I don't disagree on doing the whole shebang. The proplem is that the
> load-detect code as-is is gen3 only (and maybe gen4, haven't checked
> that) - it surely can't work on pch split platforms if half the registers
> we use in there are gone.
> 
> Until that is fixed and properly tested on all relevant platforms, we
> should be able to help the bug reporters by simply using the edid
> detection, but bailing on the load detect stuff for all HAS_HOTPLUG
> platforms (as we do now already). I'll whip up a patch.
> 
> For actual load-detect stuff is imo -next material, and I think we should
> dodge that bullet until we have an actual bug reporter wanting it ...

I can send them one of my monitors that fails to report an EDID to them
so that they can put it behind their KVM that breaks autodetection...

Coming up with a solution to handling unknown connection status is indeed
-next material, so I'm not too concerned if we punt the entire thing so
that we can do a thorough job. If we could handle unknown cleanly, it
would have prevented a lot of misery over the years with spurious TV
detection and the like.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

From: Chris Wilson <chris@chris-wilson.co.uk>

VGA hotplug detection "works" by measuring the resistance across
certain pins. A lot of kvm switches fumble this and wire up cheap
resistors with the wrong value or don't bother at all.

To accomodate these, also try to detect a connected monitor by trying
to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
with an actual load-detection cycle when the output is life - that
would be actual work to implement because things moved around. This is
the big difference to Chris Wilson's original approach:

commit 9e612a008fa7fe493a473454def56aa321479495
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu May 31 13:08:53 2012 +0100

    drm/i915/crt: Do not rely upon the HPD presence pin

This blew up on Linus' machine because it errornously detected a vga
screen (without and edid and hence only the default modes), leading to
it's prompt removal:

commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri Jun 8 14:53:06 2012 -0700

    Revert "drm/i915/crt: Do not rely upon the HPD presence pin"

Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |   11 ++++++++---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 75a70c4..7155181 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -453,18 +453,23 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	struct intel_load_detect_pipe tmp;
 
 	if (I915_HAS_HOTPLUG(dev)) {
+		/* We can not rely on the HPD pin always being correctly wired
+		 * up, for example many KVM do not pass it through, and so
+		 * only trust an assertion that the monitor is connected.
+		 */
 		if (intel_crt_detect_hotplug(connector)) {
 			DRM_DEBUG_KMS("CRT detected via hotplug\n");
 			return connector_status_connected;
-		} else {
+		} else
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
-			return connector_status_disconnected;
-		}
 	}
 
 	if (intel_crt_detect_ddc(connector))
 		return connector_status_connected;
 
+	if (I915_HAS_HOTPLUG(dev))
+		return connector_status_disconnected;
+
 	if (!force)
 		return connector->status;
 
-- 
1.7.10

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

On Mon, 11 Jun 2012 09:29:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> VGA hotplug detection "works" by measuring the resistance across
> certain pins. A lot of kvm switches fumble this and wire up cheap
> resistors with the wrong value or don't bother at all.
> 
> To accomodate these, also try to detect a connected monitor by trying
> to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
> with an actual load-detection cycle when the output is life - that
> would be actual work to implement because things moved around. This is
> the big difference to Chris Wilson's original approach:
> 
> commit 9e612a008fa7fe493a473454def56aa321479495
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu May 31 13:08:53 2012 +0100
> 
>     drm/i915/crt: Do not rely upon the HPD presence pin
> 
> This blew up on Linus' machine because it errornously detected a vga
> screen (without and edid and hence only the default modes), leading to
> it's prompt removal:

That's purposely misleading since it did not claim to detect a monitor,
only reporting that it was not able to determine whether a monitor was
connected or not. It was X's treatment of the unknown condition that has
triggered the bug reports over the years for spurious and transient
modesetting. In that there is nothing unique to that patch.
 
> commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Fri Jun 8 14:53:06 2012 -0700
> 
>     Revert "drm/i915/crt: Do not rely upon the HPD presence pin"
> 
> Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> ---
>  drivers/gpu/drm/i915/intel_crt.c |   11 ++++++++---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> index 75a70c4..7155181 100644
> --- a/drivers/gpu/drm/i915/intel_crt.c
> +++ b/drivers/gpu/drm/i915/intel_crt.c
> @@ -453,18 +453,23 @@ intel_crt_detect(struct drm_connector *connector, bool force)
>  	struct intel_load_detect_pipe tmp;
>  
>  	if (I915_HAS_HOTPLUG(dev)) {
> +		/* We can not rely on the HPD pin always being correctly wired
> +		 * up, for example many KVM do not pass it through, and so
> +		 * only trust an assertion that the monitor is connected.
> +		 */
>  		if (intel_crt_detect_hotplug(connector)) {
>  			DRM_DEBUG_KMS("CRT detected via hotplug\n");
>  			return connector_status_connected;
> -		} else {
> +		} else
>  			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
> -			return connector_status_disconnected;
> -		}
>  	}
>  
>  	if (intel_crt_detect_ddc(connector))
>  		return connector_status_connected;
>  
> +	if (I915_HAS_HOTPLUG(dev))
> +		return connector_status_disconnected;
> +

This need a big warning because intel_crtc_detect_ddc() is unable to
accurately determine detection status.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

On Mon, Jun 11, 2012 at 08:58:19AM +0100, Chris Wilson wrote:
> On Mon, 11 Jun 2012 09:29:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > VGA hotplug detection "works" by measuring the resistance across
> > certain pins. A lot of kvm switches fumble this and wire up cheap
> > resistors with the wrong value or don't bother at all.
> > 
> > To accomodate these, also try to detect a connected monitor by trying
> > to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
> > with an actual load-detection cycle when the output is life - that
> > would be actual work to implement because things moved around. This is
> > the big difference to Chris Wilson's original approach:
> > 
> > commit 9e612a008fa7fe493a473454def56aa321479495
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Thu May 31 13:08:53 2012 +0100
> > 
> >     drm/i915/crt: Do not rely upon the HPD presence pin
> > 
> > This blew up on Linus' machine because it errornously detected a vga
> > screen (without and edid and hence only the default modes), leading to
> > it's prompt removal:
> 
> That's purposely misleading since it did not claim to detect a monitor,
> only reporting that it was not able to determine whether a monitor was
> connected or not. It was X's treatment of the unknown condition that has
> triggered the bug reports over the years for spurious and transient
> modesetting. In that there is nothing unique to that patch.

Hm, I don't see where we should return an unknown connector state with
that patch. When called from userspace, we always have force set. The only
other place where we return unknown is when we can't get a load detection
pipe, but I've figured that's unlikely in Linus' case with just one other
screen. intel_crt_load_detect itself seems to never return unknown.

So I've concluded that the problem is that intel_crt_load_detect returns a
bogus connected state for Linus' machine. Which is not too unexpected given
that we've never used that load detction on newer chips. Originally I've
thought that these registers disappeared completely, but that's just been
me being confused (I've thought of the new wait_vblank for pch_split
machines from Paulo). But reading Bspec still shows that the current load
detect code has zero chances of working correctly:

Public Snb Bspec, Vol3 Part1, 1.1.1 ST00 Input Status 0, bit4:

"RGB Comparator / Sense. This bit is here for compatibility and will
always return one. Monitor detection must be done be done through the
programming of registers in the MMIO space.
0 = Below threshold
1 = Above threshold"

No wonder we detect a bogus output.

> > commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Fri Jun 8 14:53:06 2012 -0700
> > 
> >     Revert "drm/i915/crt: Do not rely upon the HPD presence pin"
> > 
> > Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> > ---
> >  drivers/gpu/drm/i915/intel_crt.c |   11 ++++++++---
> >  1 file changed, 8 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
> > index 75a70c4..7155181 100644
> > --- a/drivers/gpu/drm/i915/intel_crt.c
> > +++ b/drivers/gpu/drm/i915/intel_crt.c
> > @@ -453,18 +453,23 @@ intel_crt_detect(struct drm_connector *connector, bool force)
> >  	struct intel_load_detect_pipe tmp;
> >  
> >  	if (I915_HAS_HOTPLUG(dev)) {
> > +		/* We can not rely on the HPD pin always being correctly wired
> > +		 * up, for example many KVM do not pass it through, and so
> > +		 * only trust an assertion that the monitor is connected.
> > +		 */
> >  		if (intel_crt_detect_hotplug(connector)) {
> >  			DRM_DEBUG_KMS("CRT detected via hotplug\n");
> >  			return connector_status_connected;
> > -		} else {
> > +		} else
> >  			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
> > -			return connector_status_disconnected;
> > -		}
> >  	}
> >  
> >  	if (intel_crt_detect_ddc(connector))
> >  		return connector_status_connected;
> >  
> > +	if (I915_HAS_HOTPLUG(dev))
> > +		return connector_status_disconnected;
> > +
> 
> This need a big warning because intel_crtc_detect_ddc() is unable to
> accurately determine detection status.

ddc detection helps in a few cases where the hotplug hw fails. So this is
strictly better than what's been there before because we only use it as a
fallback if the hw doesn't detect anything. Until someone figures out how
to do load-detection properly on newer machines I think we can just leave
things as-is. And hence I don't see the point in a scary warning.

Cheers, Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

On Mon, 11 Jun 2012 10:40:15 +0200, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Mon, Jun 11, 2012 at 08:58:19AM +0100, Chris Wilson wrote:
> > On Mon, 11 Jun 2012 09:29:47 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > > From: Chris Wilson <chris@chris-wilson.co.uk>
> > > 
> > > VGA hotplug detection "works" by measuring the resistance across
> > > certain pins. A lot of kvm switches fumble this and wire up cheap
> > > resistors with the wrong value or don't bother at all.
> > > 
> > > To accomodate these, also try to detect a connected monitor by trying
> > > to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
> > > with an actual load-detection cycle when the output is life - that
> > > would be actual work to implement because things moved around. This is
> > > the big difference to Chris Wilson's original approach:
> > > 
> > > commit 9e612a008fa7fe493a473454def56aa321479495
> > > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > > Date:   Thu May 31 13:08:53 2012 +0100
> > > 
> > >     drm/i915/crt: Do not rely upon the HPD presence pin
> > > 
> > > This blew up on Linus' machine because it errornously detected a vga
> > > screen (without and edid and hence only the default modes), leading to
> > > it's prompt removal:
> > 
> > That's purposely misleading since it did not claim to detect a monitor,
> > only reporting that it was not able to determine whether a monitor was
> > connected or not. It was X's treatment of the unknown condition that has
> > triggered the bug reports over the years for spurious and transient
> > modesetting. In that there is nothing unique to that patch.
> 
> Hm, I don't see where we should return an unknown connector state with
> that patch. When called from userspace, we always have force set. The only
> other place where we return unknown is when we can't get a load detection
> pipe, but I've figured that's unlikely in Linus' case with just one other
> screen. intel_crt_load_detect itself seems to never return unknown.

My apologies, I latched onto that the presentation of this bug was
similar to the large number of reports we've have over the years with
spurious connections causing abnormal mode configurations due to the
unknown connection status.

> > > +	if (I915_HAS_HOTPLUG(dev))
> > > +		return connector_status_disconnected;
> > > +
> > 
> > This need a big warning because intel_crtc_detect_ddc() is unable to
> > accurately determine detection status.
> 
> ddc detection helps in a few cases where the hotplug hw fails. So this is
> strictly better than what's been there before because we only use it as a
> fallback if the hw doesn't detect anything. Until someone figures out how
> to do load-detection properly on newer machines I think we can just leave
> things as-is. And hence I don't see the point in a scary warning.

The whole fallback to load-detection is there for the very real cases
where monitors fail. Combine the two pieces of horrible hardware and
we've just flummoxed our detect(). The history of KVM switches is also
scary when it comes to EDID reporting.  Adding a comment to say we know
that combination may exist but is going to be so unlikely (and so painful
to use, including the pain inflicted upon the vast majority who will
never have need of that feedback) isn't scary, but a useful reminder for 
the day when that combination turns up.
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

[PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

From: Chris Wilson <chris@chris-wilson.co.uk>

VGA hotplug detection "works" by measuring the resistance across
certain pins. A lot of kvm switches fumble this and wire up cheap
resistors with the wrong resistance or don't bother at all.

To accomodate these, also try to detect a connected monitor by trying
to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
with an actual load-detection cycle when the output is life - that
would be actual work to implement because things moved around. This is
the big difference to Chris Wilson's original approach:

commit 9e612a008fa7fe493a473454def56aa321479495
Author: Chris Wilson <chris@chris-wilson.co.uk>
Date:   Thu May 31 13:08:53 2012 +0100

    drm/i915/crt: Do not rely upon the HPD presence pin

This blew up on Linus' machine because it errornously detected a vga
screen (without and edid and hence only the default modes), leading to
it's prompt removal:

commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
Author: Linus Torvalds <torvalds@linux-foundation.org>
Date:   Fri Jun 8 14:53:06 2012 -0700

    Revert "drm/i915/crt: Do not rely upon the HPD presence pin"

Some digging around in Bspec shows the reason why load detect doesn't work on
newer chips - the legacy VGA load detect bit isn't wired up any longer:

Public Snb Bspec, Vol3 Part1, 1.1.1 ST00 Input Status 0, bit4:

"RGB Comparator / Sense. This bit is here for compatibility and will
always return one. Monitor detection must be done be done through the
programming of registers in the MMIO space.
0 = Below threshold
1 = Above threshold"

v2: Add a comment in the code that load detect on hotplug capable
machines is broken and pimp the commit message with a quote of Bspec
to show why.

Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
---
 drivers/gpu/drm/i915/intel_crt.c |   15 ++++++++++++---
 1 file changed, 12 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/i915/intel_crt.c b/drivers/gpu/drm/i915/intel_crt.c
index 75a70c4..5978490 100644
--- a/drivers/gpu/drm/i915/intel_crt.c
+++ b/drivers/gpu/drm/i915/intel_crt.c
@@ -453,18 +453,27 @@ intel_crt_detect(struct drm_connector *connector, bool force)
 	struct intel_load_detect_pipe tmp;
 
 	if (I915_HAS_HOTPLUG(dev)) {
+		/* We can not rely on the HPD pin always being correctly wired
+		 * up, for example many KVM do not pass it through, and so
+		 * only trust an assertion that the monitor is connected.
+		 */
 		if (intel_crt_detect_hotplug(connector)) {
 			DRM_DEBUG_KMS("CRT detected via hotplug\n");
 			return connector_status_connected;
-		} else {
+		} else
 			DRM_DEBUG_KMS("CRT not detected via hotplug\n");
-			return connector_status_disconnected;
-		}
 	}
 
 	if (intel_crt_detect_ddc(connector))
 		return connector_status_connected;
 
+	/* Load detection is broken on HPD capable machines. Whoever wants a
+	 * broken monitor (without edid) to work behind a broken kvm (that fails
+	 * to have the right resistors for HP detection) needs to fix this up.
+	 * For now just bail out. */
+	if (I915_HAS_HOTPLUG(dev))
+		return connector_status_disconnected;
+
 	if (!force)
 		return connector->status;
 
-- 
1.7.10

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Chris Wilson]

On Mon, 11 Jun 2012 16:38:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> From: Chris Wilson <chris@chris-wilson.co.uk>
> 
> VGA hotplug detection "works" by measuring the resistance across
> certain pins. A lot of kvm switches fumble this and wire up cheap
> resistors with the wrong resistance or don't bother at all.
> 
> To accomodate these, also try to detect a connected monitor by trying
> to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
> with an actual load-detection cycle when the output is life - that
> would be actual work to implement because things moved around. This is
> the big difference to Chris Wilson's original approach:
> 
> commit 9e612a008fa7fe493a473454def56aa321479495
> Author: Chris Wilson <chris@chris-wilson.co.uk>
> Date:   Thu May 31 13:08:53 2012 +0100
> 
>     drm/i915/crt: Do not rely upon the HPD presence pin
> 
> This blew up on Linus' machine because it errornously detected a vga
> screen (without and edid and hence only the default modes), leading to
> it's prompt removal:
> 
> commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
> Author: Linus Torvalds <torvalds@linux-foundation.org>
> Date:   Fri Jun 8 14:53:06 2012 -0700
> 
>     Revert "drm/i915/crt: Do not rely upon the HPD presence pin"
> 
> Some digging around in Bspec shows the reason why load detect doesn't work on
> newer chips - the legacy VGA load detect bit isn't wired up any longer:
> 
> Public Snb Bspec, Vol3 Part1, 1.1.1 ST00 Input Status 0, bit4:
> 
> "RGB Comparator / Sense. This bit is here for compatibility and will
> always return one. Monitor detection must be done be done through the
> programming of registers in the MMIO space.
> 0 = Below threshold
> 1 = Above threshold"
> 
> v2: Add a comment in the code that load detect on hotplug capable
> machines is broken and pimp the commit message with a quote of Bspec
> to show why.
> 
> Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>

Just missing your s-o-b, and you can add my reviewed-by to your
amendment. :)
-Chris

-- 
Chris Wilson, Intel Open Source Technology Centre

Re: [PATCH] drm/i915/crt: Do not rely upon the HPD presence pin

[Daniel Vetter]

On Mon, Jun 11, 2012 at 03:42:32PM +0100, Chris Wilson wrote:
> On Mon, 11 Jun 2012 16:38:40 +0200, Daniel Vetter <daniel.vetter@ffwll.ch> wrote:
> > From: Chris Wilson <chris@chris-wilson.co.uk>
> > 
> > VGA hotplug detection "works" by measuring the resistance across
> > certain pins. A lot of kvm switches fumble this and wire up cheap
> > resistors with the wrong resistance or don't bother at all.
> > 
> > To accomodate these, also try to detect a connected monitor by trying
> > to grab the edid. Contrary to !HAS_HOTPLUG platforms we don't bother
> > with an actual load-detection cycle when the output is life - that
> > would be actual work to implement because things moved around. This is
> > the big difference to Chris Wilson's original approach:
> > 
> > commit 9e612a008fa7fe493a473454def56aa321479495
> > Author: Chris Wilson <chris@chris-wilson.co.uk>
> > Date:   Thu May 31 13:08:53 2012 +0100
> > 
> >     drm/i915/crt: Do not rely upon the HPD presence pin
> > 
> > This blew up on Linus' machine because it errornously detected a vga
> > screen (without and edid and hence only the default modes), leading to
> > it's prompt removal:
> > 
> > commit 8f53369b753f5f4c7684c2eb0b592152abb1dd00
> > Author: Linus Torvalds <torvalds@linux-foundation.org>
> > Date:   Fri Jun 8 14:53:06 2012 -0700
> > 
> >     Revert "drm/i915/crt: Do not rely upon the HPD presence pin"
> > 
> > Some digging around in Bspec shows the reason why load detect doesn't work on
> > newer chips - the legacy VGA load detect bit isn't wired up any longer:
> > 
> > Public Snb Bspec, Vol3 Part1, 1.1.1 ST00 Input Status 0, bit4:
> > 
> > "RGB Comparator / Sense. This bit is here for compatibility and will
> > always return one. Monitor detection must be done be done through the
> > programming of registers in the MMIO space.
> > 0 = Below threshold
> > 1 = Above threshold"
> > 
> > v2: Add a comment in the code that load detect on hotplug capable
> > machines is broken and pimp the commit message with a quote of Bspec
> > to show why.
> > 
> > Reported-and-tested-by: Matthieu LAVIE <boiteamadmax@hotmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > Signed-off-by: Chris Wilson <chris@chris-wilson.co.uk>
> 
> Just missing your s-o-b, and you can add my reviewed-by to your
> amendment. :)

Fixed and applied to -fixes, thanks for the review (and kickstarting this
with your original patch).
-Daniel
-- 
Daniel Vetter
Mail: daniel@ffwll.ch
Mobile: +41 (0)79 365 57 48