Date: Fri, 7 Aug 2015 23:40:28 +0100
From: Russell King - ARM Linux <linux@arm.linux.org.uk>
To: Lucas Stach <l.stach@pengutronix.de>
Cc: Sascha Hauer <s.hauer@pengutronix.de>, Thierry Reding <treding@nvidia.com>,
        dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org
Subject: Re: [PATCH 1/2] drm: bridge/dw_hdmi: Cache edid data
Message-ID: <20150807224028.GI7557@n2100.arm.linux.org.uk>
References: <1438955961-27232-1-git-send-email-s.hauer@pengutronix.de>
 <1438955961-27232-2-git-send-email-s.hauer@pengutronix.de>
 <20150807141346.GH7557@n2100.arm.linux.org.uk>
 <1438957247.2467.17.camel@pengutronix.de>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <1438957247.2467.17.camel@pengutronix.de>
User-Agent: Mutt/1.5.23 (2014-03-12)
Sender: linux-kernel-owner@vger.kernel.org
Content-Length: 3232
Lines: 88

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.
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/