All of lore.kernel.org
 help / color / mirror / Atom feed
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Lucas Stach <l.stach@pengutronix.de>, dri-devel@lists.freedesktop.org
Cc: devel@driverdev.osuosl.org,
	Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
	Tim Harvey <tharvey@gateworks.com>,
	kernel@pengutronix.de
Subject: Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug
Date: Mon, 14 Apr 2014 10:10:33 +0100	[thread overview]
Message-ID: <20140414091033.GF24070@n2100.arm.linux.org.uk> (raw)
In-Reply-To: <1397464952.4548.4.camel@weser.hi.pengutronix.de>

On Mon, Apr 14, 2014 at 10:42:32AM +0200, Lucas Stach wrote:
> Am Sonntag, den 13.04.2014, 15:58 +0100 schrieb Russell King - ARM
> Linux:
> > On Fri, Apr 11, 2014 at 04:13:33PM +0200, Lucas Stach wrote:
> > > Make sure that we probe for a display on detect regardless
> > > of previous hotplug events. Don't handle connector
> > > hotplug state ourselves, but let DRM do the right thing
> > > for us. This brings our hotplug handling in line with
> > > what other DRM drivers do.
> > 
> > Why should working setups have to pay the price for faulty setups when we
> > can adequately detect the hotplug signal on iMX SoCs when it's correctly
> > wired?
> > 
> > By "price" I mean - if we end up having to poll the connector, we end up
> > calling the i2c functions, and the i2c functions on iMX use a fixed
> > timeout of 100ms.  That means the context which runs the
> > imx_hdmi_connector_detect() function is forced to sleep for 100ms.  If
> > that's being run as part of a softirq (eg, via a work struct), that's
> > bad news because that could be any thread in the system.
> > 
> > The "price" should only be paid by those implementations where the hotplug
> > signal is not correctly wired.
> > 
> This change is not related to broken systems. It just uses the DRM
> framework as intended. The detect() callback, which triggers the EDID
> fetch will only be called by DRM when a hotplug event was received, or
> if someone (e.g. kms_fb_helper, or userspace) explicitly requests to
> poll the connector.
> 
> Not doing so is working around the DRM framework, not using it. So as
> mentioned this change just brings us in line with what other DRM drivers
> do to handle hotplug and connector detect.

I totally disagree with that.  What we're doing today using HPD to
detect connection is entirely in keeping with DRM and the HDMI spec,
and is more correct than your solution using EDID to detect the
presence of a connection.

HPD in HDMI indicates that the EDID is available for reading.  There
is no need what so ever to try reading the EDID to detect whether
a device is present.

Moreover, the HDMI spec does not say what state the DDC signals will
be when the sink is powered off - it seems to me that it is entirely
reasonable when HPD is lowered due to the sink being powered off that
the DDC signals may be clamped to logic zero by ESD diodes in the sink,
which would cause problems when trying to detect by reading the EDID.

Moreover, it is quite legal for a sink to modify the contents of its
EEPROM - and it can do this by manipulating the DDC signals itself.
Polling the EDID would open the possibilities of races, reading the
EDID before the sink had finished updating it.

-- 
FTTC broadband for 0.8mile line: now at 9.7Mbps down 460kbps up... slowly
improving, and getting towards what was expected from it.

       reply	other threads:[~2014-04-14  9:10 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
     [not found] <1397225615-17670-1-git-send-email-l.stach@pengutronix.de>
     [not found] ` <1397225615-17670-2-git-send-email-l.stach@pengutronix.de>
     [not found]   ` <20140413145810.GA24070@n2100.arm.linux.org.uk>
     [not found]     ` <1397464952.4548.4.camel@weser.hi.pengutronix.de>
2014-04-14  9:10       ` Russell King - ARM Linux [this message]
2014-04-14  9:38         ` [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug Lucas Stach
2014-04-14 10:09           ` Russell King - ARM Linux
2014-04-14 10:24             ` Lucas Stach
2014-04-14 20:57               ` Russell King - ARM Linux

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20140414091033.GF24070@n2100.arm.linux.org.uk \
    --to=linux@arm.linux.org.uk \
    --cc=devel@driverdev.osuosl.org \
    --cc=dri-devel@lists.freedesktop.org \
    --cc=gregkh@linuxfoundation.org \
    --cc=kernel@pengutronix.de \
    --cc=l.stach@pengutronix.de \
    --cc=tharvey@gateworks.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.