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

[Chris Wilson <chris@chris-wilson.co.uk>]

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