Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

[Russell King - ARM Linux]

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.

Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

[Lucas Stach]

Am Montag, den 14.04.2014, 10:10 +0100 schrieb Russell King - ARM Linux:
> 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.
> 

And that's exactly what happens now. We do not poll the EDID in any way,
until we are explicitly asked to do so, which happens only very few
occasions.

Please go back and read the code after this patch. What we do now in the
regular case (nobody is calling detect() explicitly) is the following:

1. We wait for the HDMI irq to signal
2. If we got a HDMI hpd event we call drm_helper_hpd_irq_event()
3. In response to this event DRM calls our detect() function, which
tries to fetch the EDID.
4. If an EDID is found we report a connected display.

This sequence is completely in line with what the HDMI spec says and
what you demand has to be done.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

[Russell King - ARM Linux]

On Mon, Apr 14, 2014 at 11:38:43AM +0200, Lucas Stach wrote:
> Am Montag, den 14.04.2014, 10:10 +0100 schrieb Russell King - ARM Linux:
> > 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.
> > 
> 
> And that's exactly what happens now. We do not poll the EDID in any way,
> until we are explicitly asked to do so, which happens only very few
> occasions.
> 
> Please go back and read the code after this patch. What we do now in the
> regular case (nobody is calling detect() explicitly) is the following:

Now *you* please go back and read what you said about kms/userspace being
able to poll the connector, thereby causing an EDID read attempt while
HPD may not be active.

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

Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

[Lucas Stach]

Am Montag, den 14.04.2014, 11:09 +0100 schrieb Russell King - ARM Linux:
> On Mon, Apr 14, 2014 at 11:38:43AM +0200, Lucas Stach wrote:
> > Am Montag, den 14.04.2014, 10:10 +0100 schrieb Russell King - ARM Linux:
> > > 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.
> > > 
> > 
> > And that's exactly what happens now. We do not poll the EDID in any way,
> > until we are explicitly asked to do so, which happens only very few
> > occasions.
> > 
> > Please go back and read the code after this patch. What we do now in the
> > regular case (nobody is calling detect() explicitly) is the following:
> 
> Now *you* please go back and read what you said about kms/userspace being
> able to poll the connector, thereby causing an EDID read attempt while
> HPD may not be active.
> 
Yes, userspace may trigger an explicit detect because it might suspect
that a sink is present while it has not received any HP event. What
userspace expects to happen in this situation is an explicit poll of the
connector regardless of the HP status.

If you then just report the connector as disconnected because you didn't
receive a HP event before, you break the use-case for which userspace is
calling an explicit detect in the first place.

The userspace expects to encounter potentially long timeouts when
calling this function. For properly working setups userspace never does
this, as the HP event is enough to handle those.

Regards,
Lucas
-- 
Pengutronix e.K.                           | Lucas Stach                 |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-5076 |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: [PATCH 2/4] staging: imx-hdmi: correct connector detect and hotplug

[Russell King - ARM Linux]

On Mon, Apr 14, 2014 at 12:24:45PM +0200, Lucas Stach wrote:
> Am Montag, den 14.04.2014, 11:09 +0100 schrieb Russell King - ARM Linux:
> > Now *you* please go back and read what you said about kms/userspace being
> > able to poll the connector, thereby causing an EDID read attempt while
> > HPD may not be active.
> > 
> Yes, userspace may trigger an explicit detect because it might suspect
> that a sink is present while it has not received any HP event. What
> userspace expects to happen in this situation is an explicit poll of the
> connector regardless of the HP status.

So, if you issue this poll, and the sink has lowered the HPD signal
because it wants to update the EDID EEPROM, and is in the middle of
doing so.  Meanwhile you start an I2C transaction in the DDC bus.
Maybe you win the arbitration, maybe you gain access because you
manage to get your transaction in while the sink is between two I2C
transactions.

The result is you can end up reading inconsistent EDID data from the
sink.

There is no race free way to do this - HPD is the indication on HDMI
that the sink is available, and that the EDID can be read by the
source.  If HPD is not active, then the EDID should *not* be read.

> If you then just report the connector as disconnected because you didn't
> receive a HP event before, you break the use-case for which userspace is
> calling an explicit detect in the first place.

What is wrong is that we store the interrupt-generated cached state,
rather than reporting the actual HPD signal state when the "detect"
method is used.  We need to be reporting the real live state of the
HPD signal in that function - or, in the case where HPD has not been
correctly wired, your fallback of the RXSENSE bits.

HPD *is* the signal which says "the HDMI sink is *properly* connected
and the EDID data is available for you to read" when it is asserted.
When HPD is not asserted, the HDMI sink is saying that the EDID data
is not available.

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