drm: bridge/dw_hdmi patches

drm: bridge/dw_hdmi patches

[Sascha Hauer]

Two small updates for the dw_hdmi driver. We should cache EDID
data to make handling it a bit more efficient. Also remove some unused code.

----------------------------------------------------------------
Sascha Hauer (2):
      drm: bridge/dw_hdmi: Cache edid data
      drm: bridge/dw_hdmi: remove unused code

 drivers/gpu/drm/bridge/dw_hdmi.c | 58 ++++++++++++++++++----------------------
 1 file changed, 26 insertions(+), 32 deletions(-)

[PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Sascha Hauer]

Instead of rereading the edid data each time userspace asks for them
read them once and cache them in the previously unused edid field in
struct dw_hdmi. This makes the code a little bit more efficient.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/bridge/dw_hdmi.c | 41 +++++++++++++++++++++++++---------------
 1 file changed, 26 insertions(+), 15 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 816d104..8d9b53c 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -117,7 +117,7 @@ struct dw_hdmi {
 
 	int vic;
 
-	u8 edid[HDMI_EDID_LEN];
+	struct edid *edid;
 	bool cable_plugin;
 
 	bool phy_enabled;
@@ -1386,28 +1386,39 @@ dw_hdmi_connector_detect(struct drm_connector *connector, bool force)
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
 
-	return hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD ?
-		connector_status_connected : connector_status_disconnected;
+	if (!(hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD))
+		return connector_status_disconnected;
+
+	if (hdmi->ddc) {
+		if (hdmi->edid) {
+			drm_mode_connector_update_edid_property(connector,
+					NULL);
+			kfree(hdmi->edid);
+		}
+
+		hdmi->edid = drm_get_edid(connector, hdmi->ddc);
+		if (hdmi->edid) {
+			drm_mode_connector_update_edid_property(connector,
+					hdmi->edid);
+
+			dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
+					hdmi->edid->width_cm,
+					hdmi->edid->height_cm);
+		}
+	}
+
+	return connector_status_connected;
 }
 
 static int dw_hdmi_connector_get_modes(struct drm_connector *connector)
 {
 	struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
 					     connector);
-	struct edid *edid;
 	int ret = 0;
 
-	if (!hdmi->ddc)
-		return 0;
-
-	edid = drm_get_edid(connector, hdmi->ddc);
-	if (edid) {
-		dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n",
-			edid->width_cm, edid->height_cm);
-
-		drm_mode_connector_update_edid_property(connector, edid);
-		ret = drm_add_edid_modes(connector, edid);
-		kfree(edid);
+	if (hdmi->edid) {
+		ret = drm_add_edid_modes(connector, hdmi->edid);
+		drm_edid_to_eld(connector, hdmi->edid);
 	} else {
 		dev_dbg(hdmi->dev, "failed to get edid\n");
 	}
-- 
2.4.6

Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 03:59:20PM +0200, Sascha Hauer wrote:
> Instead of rereading the edid data each time userspace asks for them
> read them once and cache them in the previously unused edid field in
> struct dw_hdmi. This makes the code a little bit more efficient.

How has this been tested?

Has it been tested with an AV amplifier in the path to the display device?
If not, it really needs to be tested, because such devices modify the EDID
data, or subsitute their own, and alter the EDID data depending on their
needs.  When they do, they lower and re-assert the HPD signal, possibly
for as little as 100ms - defined by the HDMI spec.

For example, an AV amplifier in standby can provide the displays EDID in
the first page of EDID, along with the displays extended EDID in the
second page.  When the AV amplifier is powered up, it can provide a
different second page of EDID information detailing the audio formats
that it can accept, and it will lower and re-assert the HPD signal to
cause the connected devices to read the updated EDID information.  Same
thing can happen when switching to standby mode too.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Lucas Stach]

Am Freitag, den 07.08.2015, 15:13 +0100 schrieb Russell King - ARM
Linux:
> On Fri, Aug 07, 2015 at 03:59:20PM +0200, Sascha Hauer wrote:
> > Instead of rereading the edid data each time userspace asks for them
> > read them once and cache them in the previously unused edid field in
> > struct dw_hdmi. This makes the code a little bit more efficient.
> 
> How has this been tested?
> 
> Has it been tested with an AV amplifier in the path to the display device?
> If not, it really needs to be tested, because such devices modify the EDID
> data, or subsitute their own, and alter the EDID data depending on their
> needs.  When they do, they lower and re-assert the HPD signal, possibly
> for as little as 100ms - defined by the HDMI spec.
> 
This has not been tested with an AV amp, but if the amp behaves the way
you describe it this should be fine.

If we get an HPD event we inform the DRM core, which will then call our
connector detect function, triggering a re-read of the EDID data.

Regards,
Lucas

> For example, an AV amplifier in standby can provide the displays EDID in
> the first page of EDID, along with the displays extended EDID in the
> second page.  When the AV amplifier is powered up, it can provide a
> different second page of EDID information detailing the audio formats
> that it can accept, and it will lower and re-assert the HPD signal to
> cause the connected devices to read the updated EDID information.  Same
> thing can happen when switching to standby mode too.
> 

-- 
Pengutronix e.K.             | Lucas Stach                 |
Industrial Linux Solutions   | http://www.pengutronix.de/  |

Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 04:20:47PM +0200, Lucas Stach wrote:
> Am Freitag, den 07.08.2015, 15:13 +0100 schrieb Russell King - ARM
> Linux:
> > On Fri, Aug 07, 2015 at 03:59:20PM +0200, Sascha Hauer wrote:
> > > Instead of rereading the edid data each time userspace asks for them
> > > read them once and cache them in the previously unused edid field in
> > > struct dw_hdmi. This makes the code a little bit more efficient.
> > 
> > How has this been tested?
> > 
> > Has it been tested with an AV amplifier in the path to the display device?
> > If not, it really needs to be tested, because such devices modify the EDID
> > data, or subsitute their own, and alter the EDID data depending on their
> > needs.  When they do, they lower and re-assert the HPD signal, possibly
> > for as little as 100ms - defined by the HDMI spec.
> > 
> This has not been tested with an AV amp, but if the amp behaves the way
> you describe it this should be fine.
> 
> If we get an HPD event we inform the DRM core, which will then call our
> connector detect function, triggering a re-read of the EDID data.

I'm still nervous about this.

Consider:
- HPD raised
- We start to read the EDID
- HPD dropped, EDID updates, HPD raised
- EDID checksum fails

We're now in the situation where we don't retry reading the EDID, because
we've effectively cached the failure.

I don't think we should be caching the failure - I think what we want is:

dw_hdmi_connector_detect(struct drm_connector *connector)
{
	if (!(hdmi_readb(hdmi, HDMI_PHY_STAT0) & HDMI_PHY_HPD))
		return connector_status_disconnected;

	if (hdmi->ddc)
		read_edid();

	return connector_status_connected;
}

dw_hdmi_connector_get_modes(struct drm_connector *connector)
{
	if (!hdmi->edid)
		read_edid();

	if (hdmi->edid) {
		ret = drm_add_edid_modes(connector, hdmi->edid);
		...
	} else {
		...
	}

	return ret;
}

so we at least have the ability to retry a failed EDID without having to
pull the connector and try plugging the sink back in.

However, if we do this, then we might as well just modify
dw_hdmi_connector_get_modes(), and arrange for a HPD de-assertion to
NULL and free hdmi->edid so the next get_modes() call triggers a re-read.

Alternatively, if you look at the complexity in
drm_helper_probe_single_connector_modes_merge_bits(), even fixing this
would still leave a significant amount of work being done on every
first call to get_connector, especially so if we're loading override
EDID from the firmware files.  Maybe rather than fixing this in every
driver, maybe it ought to be handled further up the stack, possibly
with an opt-in flag?  We'd probably need to get smarter at reporting
a disconnect to close a race there though (where we don't get around
to calling ->detect fast enough after we notice HPD has been deasserted,
but if it's taking longer than 100ms to get there, we're probably fscked
anyway.)

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Russell King - ARM Linux]

On Fri, Aug 07, 2015 at 11:40:28PM +0100, Russell King - ARM Linux wrote:
> However, if we do this, then we might as well just modify
> dw_hdmi_connector_get_modes(), and arrange for a HPD de-assertion to
> NULL and free hdmi->edid so the next get_modes() call triggers a re-read.

Testing with the AV amp seems to be working okay.

For reference, he's the RXSENSE / HPD activity, with the AV amp
configured for HDMI passthrough in standby mode.  This output is from
the IRQ thread rather than the hardware IRQ function, so timings may not
be accurate.

unplug:
[ 9128.933266] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,---)
[ 9128.935883] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)

plugin:
[ 9167.072748] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,HPD)
[ 9167.121773] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,---)
[ 9167.122323] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)
[ 9168.320609] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,HPD)

av poweron:
[ 9185.696008] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,---)
[ 9185.696376] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)
[ 9187.286580] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,HPD)

tv poweron:
[ 9207.701588] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,---)
[ 9207.701953] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)
[ 9210.638573] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,HPD)

av standby:
[ 9230.588697] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,---)
[ 9230.589062] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)
[ 9231.788225] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX3210,HPD)

tv standby:
(nothing)

... and a bit later, when the AV amp switches into a lower standby mode:
[ 9290.700523] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,HPD)
[ 9290.944270] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(RX----,---)

The interesting one is the plug-in event, where we see HPD dropped after
about 50ms.

I've previously had the opportunity to test the older code with another
manufacturer's AV amp, which I no longer have access to.  My notes for
that setup are as below - I didn't note down the kernel logs in this
case...

TV (no AV amp):
imxboot: initial(+rxsense,+hpd) ... -rxsense
standby->on: -hpd, +rxsense, +hpd, -hpd, -rxsense, (+rxsense +hpd)
on->unplug: -hpd, -rxsense
unplug->plug: (+rxsense +hpd)
plug->standby: no effect
standby->unplug: -hpd, -rxsense
unplug->plug: +rxsense, +hpd

AV (AV + TV in standby mode initially, AV connected to TV):
unplugged: -rxsense,-hpd
unplugged->plugged: +rxsense, +hpd
        EDID reads from AV
AV standby,TV standby->TV on: -hpd, 2s, +hpd
        EDID reads from TV
TV on,AV standby->AV on: -hpd, 1.2s, +hpd
	EDID reads from TV with AV capabilities merged
TV on,AV on->AV standby: -hpd, 1.2s, +hpd

I have a whole bunch of further patches for dw-hdmi, so if you don't
mind, I'll take your two, merge them on the front of my series, and
re-post them.  I think we're getting close to the time where we can
see initial audio support merged for dw-hdmi.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data

[Russell King - ARM Linux]

On Sat, Aug 08, 2015 at 02:35:40PM +0100, Russell King - ARM Linux wrote:
> I have a whole bunch of further patches for dw-hdmi, so if you don't
> mind, I'll take your two, merge them on the front of my series, and
> re-post them.  I think we're getting close to the time where we can
> see initial audio support merged for dw-hdmi.

Having tried to apply the patches on top of my previous pull request to
David (sent on 15th July), it would be much better if we waited.  David
is away at the moment, apparently due back in a little over a week's time.
Hence, there's no processing of any development pull requests at the
moment, which is why it hasn't shown up in linux-next.

My series is based on 9203dd016a5d ("ALSA: pcm: add IEC958 channel status
helper") which pre-dates the fixes to get_modes() to return non-zero,
which are already in Linus' tree.  I _could_ rebase it, but that's not
good practice once it's been asked to be pulled.

At the moment, my tree does not introduce any conflicts.

Trying to merge your patch will create conflicts: if I merge the fix into
my tree, we end up with duplicate commits.  If I tweak your patch to
apply to my tree as the code used to stand, git will end up with two
conflicting changes.  If I merge mainline into my tree, Linus doesn't
like cross-merges.

-- 
FTTC broadband for 0.8mile line: currently at 10.5Mbps down 400kbps up
according to speedtest.net.

[PATCH 2/2] drm: bridge/dw_hdmi: remove unused code

[Sascha Hauer]

The cable_plugin field in struct dw_hdmi is never set. Remove it and with
it all code that is only executed when the variable is true.

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/gpu/drm/bridge/dw_hdmi.c | 17 -----------------
 1 file changed, 17 deletions(-)

diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c
index 8d9b53c..401067e 100644
--- a/drivers/gpu/drm/bridge/dw_hdmi.c
+++ b/drivers/gpu/drm/bridge/dw_hdmi.c
@@ -118,7 +118,6 @@ struct dw_hdmi {
 	int vic;
 
 	struct edid *edid;
-	bool cable_plugin;
 
 	bool phy_enabled;
 	struct drm_display_mode previous_mode;
@@ -1164,24 +1163,10 @@ static void dw_hdmi_clear_overflow(struct dw_hdmi *hdmi)
 		hdmi_writeb(hdmi, val, HDMI_FC_INVIDCONF);
 }
 
-static void hdmi_enable_overflow_interrupts(struct dw_hdmi *hdmi)
-{
-	hdmi_writeb(hdmi, 0, HDMI_FC_MASK2);
-	hdmi_writeb(hdmi, 0, HDMI_IH_MUTE_FC_STAT2);
-}
-
-static void hdmi_disable_overflow_interrupts(struct dw_hdmi *hdmi)
-{
-	hdmi_writeb(hdmi, HDMI_IH_MUTE_FC_STAT2_OVERFLOW_MASK,
-		    HDMI_IH_MUTE_FC_STAT2);
-}
-
 static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 {
 	int ret;
 
-	hdmi_disable_overflow_interrupts(hdmi);
-
 	hdmi->vic = drm_match_cea_mode(mode);
 
 	if (!hdmi->vic) {
@@ -1255,8 +1240,6 @@ static int dw_hdmi_setup(struct dw_hdmi *hdmi, struct drm_display_mode *mode)
 	hdmi_tx_hdcp_config(hdmi);
 
 	dw_hdmi_clear_overflow(hdmi);
-	if (hdmi->cable_plugin && !hdmi->hdmi_data.video_mode.mdvi)
-		hdmi_enable_overflow_interrupts(hdmi);
 
 	return 0;
 }
-- 
2.4.6