| Message ID | 1438955961-27232-2-git-send-email-s.hauer@pengutronix.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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.
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. >
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.)
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.
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.
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"); }
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(-)