[Intel-gfx] [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 at ffwll.ch> wrote:
> > From: Chris Wilson <chris at 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 at 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 at 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 at hotmail.com>
> > Bugzilla: https://bugs.freedesktop.org/show_bug.cgi?id=50501
> > Signed-off-by: Chris Wilson <chris at 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 at ffwll.ch
Mobile: +41 (0)79 365 57 48