These were previously set in dw_hdmi_connector_get_modes but this isn't called when the EDID is overridden. This can be seen in drm_helper_probe_single_connector_modes. Using the drm_kms_helper.edid_firmware parameter therefore always resulted in silence, even when providing the very same EDID that had previously been read from /sys.
I saw that other drivers set similar properties in mode_set rather than get_modes. radeon_audio_hdmi_mode_set is one such example. It calls radeon_connector_edid to retrieve the EDID so I drew inspiration from this for the fix.
I have tested this with a Utilite Pro on 4.9-rc3. I tried overriding the EDID with my own, not overriding the EDID, hotplugging the display after booting, and overriding the EDID with 1920x1080.bin. The latter has no audio parameters so no sound was heard as expected.
Signed-off-by: James Le Cuirot chewi@gentoo.org ---
Notes: I do have some questions.
I don't know the significance of the mutex lock. I put my code inside it because I am modifying the hdmi properties. Is this necessary? Should it go before or after the lock instead?
I'm also wondering whether I should initially set both properties to false in case the EDID is missing but the driver didn't do this previously. Perhaps it should have?
I have only contributed one line to the kernel before, many years ago, so please be gentle. :)
drivers/gpu/drm/bridge/dw-hdmi.c | 13 +++++++++++-- 1 file changed, 11 insertions(+), 2 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index ab7023e..a30b691 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1383,12 +1383,23 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, struct drm_display_mode *mode) { struct dw_hdmi *hdmi = bridge->driver_private; + struct drm_property_blob *edid_blob; + struct edid *edid;
mutex_lock(&hdmi->mutex);
/* Store the display mode for plugin/DKMS poweron events */ memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode));
+ edid_blob = hdmi->connector.edid_blob_ptr; + if (edid_blob) { + edid = (struct edid *) edid_blob->data; + if (edid) { + hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); + hdmi->sink_has_audio = drm_detect_monitor_audio(edid); + } + } + mutex_unlock(&hdmi->mutex); }
@@ -1445,8 +1456,6 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm);
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); - hdmi->sink_has_audio = drm_detect_monitor_audio(edid); drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); /* Store the ELD */
On Sun, Oct 30, 2016 at 01:56:25PM +0000, James Le Cuirot wrote:
These were previously set in dw_hdmi_connector_get_modes but this isn't called when the EDID is overridden. This can be seen in drm_helper_probe_single_connector_modes. Using the drm_kms_helper.edid_firmware parameter therefore always resulted in silence, even when providing the very same EDID that had previously been read from /sys.
I saw that other drivers set similar properties in mode_set rather than get_modes. radeon_audio_hdmi_mode_set is one such example. It calls radeon_connector_edid to retrieve the EDID so I drew inspiration from this for the fix.
I have tested this with a Utilite Pro on 4.9-rc3. I tried overriding the EDID with my own, not overriding the EDID, hotplugging the display after booting, and overriding the EDID with 1920x1080.bin. The latter has no audio parameters so no sound was heard as expected.
I'm not sure I particularly like this approach - the issue seems to be that drm_helper_probe_single_connector_modes() can avoid calling ->get_modes(), at which point our ideas about the EDID-based capabilities become stale.
I think it would be better to provide our own ->fill_modes implementation which calls drm_helper_probe_single_connector_modes(), and then parses the resulting EDID, rather than re-parsing it each time we set a mode.
We also need to apply this to the ELD as well - and several other drivers are similarly buggy, and are going to need similar fixes (thanks for pointing this problem out!)
Notes: I do have some questions.
I don't know the significance of the mutex lock. I put my code inside it because I am modifying the hdmi properties. Is this necessary? Should it go before or after the lock instead?
It's there to ensure that ->previous_mode, ->disabled, and the power management all operate atomically.
I'm also wondering whether I should initially set both properties to false in case the EDID is missing but the driver didn't do this previously. Perhaps it should have?
The driver's private data is initially zero-ed, so that should be unnecessary.
So maybe something like this instead - can you test please?
drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 77ab47341658..878568af2d41 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) mutex_unlock(&hdmi->mutex); }
+static int dw_hdmi_connector_fill_modes(struct drm_connector *connector, + uint32_t maxX, uint32_t maxY) +{ + struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi, + connector); + struct edid *edid; + int ret; + + ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY); + + edid = connector->edid_blob_ptr; + if (edid) { + hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); + hdmi->sink_has_audio = drm_detect_monitor_audio(edid); + /* Store the ELD */ + drm_edid_to_eld(connector, edid); + } else { + hdmi->sink_is_hdmi = false; + hdmi->sink_has_audio = false; + } + + return ret; +} + static enum drm_connector_status dw_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm);
- hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid); - hdmi->sink_has_audio = drm_detect_monitor_audio(edid); drm_mode_connector_update_edid_property(connector, edid); ret = drm_add_edid_modes(connector, edid); - /* Store the ELD */ - drm_edid_to_eld(connector, edid); kfree(edid); } else { dev_dbg(hdmi->dev, "failed to get edid\n"); @@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
static const struct drm_connector_funcs dw_hdmi_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms, - .fill_modes = drm_helper_probe_single_connector_modes, + .fill_modes = dw_hdmi_connector_fill_modes, .detect = dw_hdmi_connector_detect, .destroy = dw_hdmi_connector_destroy, .force = dw_hdmi_connector_force,
On Mon, 31 Oct 2016 16:37:36 +0000 Russell King - ARM Linux linux@armlinux.org.uk wrote:
On Sun, Oct 30, 2016 at 01:56:25PM +0000, James Le Cuirot wrote:
These were previously set in dw_hdmi_connector_get_modes but this isn't called when the EDID is overridden. This can be seen in drm_helper_probe_single_connector_modes. Using the drm_kms_helper.edid_firmware parameter therefore always resulted in silence, even when providing the very same EDID that had previously been read from /sys.
I saw that other drivers set similar properties in mode_set rather than get_modes. radeon_audio_hdmi_mode_set is one such example. It calls radeon_connector_edid to retrieve the EDID so I drew inspiration from this for the fix.
I'm not sure I particularly like this approach - the issue seems to be that drm_helper_probe_single_connector_modes() can avoid calling ->get_modes(), at which point our ideas about the EDID-based capabilities become stale.
I think it would be better to provide our own ->fill_modes implementation which calls drm_helper_probe_single_connector_modes(), and then parses the resulting EDID, rather than re-parsing it each time we set a mode.
I also thought it was a little odd to do it via mode_set but figured it was like that for a reason. I can't really comment on which approach is better so some input from others would be appreciated.
We also need to apply this to the ELD as well - and several other drivers are similarly buggy, and are going to need similar fixes (thanks for pointing this problem out!)
Are you sure that is necessary? If you take a look at drm_load_edid_firmware, you'll see that it already calls drm_edid_to_eld when the drm_kms_helper.edid_firmware parameter is given.
Notes: I do have some questions.
I'm also wondering whether I should initially set bothproperties to false in case the EDID is missing but the driver didn't do this previously. Perhaps it should have?
The driver's private data is initially zero-ed, so that should be unnecessary.
Right, but what if you hotplug from a display that has a readable EDID to one that doesn't? You did this in your patch anyway.
So maybe something like this instead - can you test please?
I briefly tested while giving drm_kms_helper.edid_firmware and it works, though I did have to assign edid via edid_blob->data like I did in my own patch. edid_blob_ptr is not an edid struct.
drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 77ab47341658..878568af2d41 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) mutex_unlock(&hdmi->mutex); }
+static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
uint32_t maxX, uint32_t maxY)+{
- struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
connector);- struct edid *edid;
- int ret;
- ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
- edid = connector->edid_blob_ptr;
- if (edid) {
hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);hdmi->sink_has_audio = drm_detect_monitor_audio(edid);/* Store the ELD */drm_edid_to_eld(connector, edid);- } else {
hdmi->sink_is_hdmi = false;hdmi->sink_has_audio = false;- }
- return ret;
+}
static enum drm_connector_status dw_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm);
hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);hdmi->sink_has_audio = drm_detect_monitor_audio(edid);/* Store the ELD */drm_edid_to_eld(connector, edid);@@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
static const struct drm_connector_funcs dw_hdmi_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .fill_modes = dw_hdmi_connector_fill_modes, .detect = dw_hdmi_connector_detect, .destroy = dw_hdmi_connector_destroy, .force = dw_hdmi_connector_force,
On Wed, Nov 02, 2016 at 09:47:50PM +0000, James Le Cuirot wrote:
On Mon, 31 Oct 2016 16:37:36 +0000 Russell King - ARM Linux linux@armlinux.org.uk wrote:
We also need to apply this to the ELD as well - and several other drivers are similarly buggy, and are going to need similar fixes (thanks for pointing this problem out!)
Are you sure that is necessary? If you take a look at drm_load_edid_firmware, you'll see that it already calls drm_edid_to_eld when the drm_kms_helper.edid_firmware parameter is given.
Hmm, thanks for pointing that out - you are correct that we only need to call drm_edid_to_eld() in get_modes().
I've also been digging more into the CEA861B spec, and it seems that keying the infoframes off the result of drm_detect_hdmi_monitor() is not entirely correct - CEA861B allows version 2 AVI infoframes to be sent if the EDID version is 3, and audio (along with audio infoframes) where audio is reported as supported.
I've been fixing this in TDA998x primarily at the moment, because that works. I've been avoiding iMX6 stuff (where dw-hdmi is used) while iMX stabilises after the merge window - the autobuilers have been reporting boot failures on those platforms, although it looks like rc3 has solved them all now.
I briefly tested while giving drm_kms_helper.edid_firmware and it works, though I did have to assign edid via edid_blob->data like I did in my own patch. edid_blob_ptr is not an edid struct.
Yes, found that with applying a similar approach to TDA998x, I'll fix this dw-hdmi patch up for that once I've finished with TDA998x. Nevertheless, your report is good news.
drivers/gpu/drm/bridge/dw-hdmi.c | 30 +++++++++++++++++++++++++----- 1 file changed, 25 insertions(+), 5 deletions(-)
diff --git a/drivers/gpu/drm/bridge/dw-hdmi.c b/drivers/gpu/drm/bridge/dw-hdmi.c index 77ab47341658..878568af2d41 100644 --- a/drivers/gpu/drm/bridge/dw-hdmi.c +++ b/drivers/gpu/drm/bridge/dw-hdmi.c @@ -1413,6 +1413,30 @@ static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) mutex_unlock(&hdmi->mutex); }
+static int dw_hdmi_connector_fill_modes(struct drm_connector *connector,
uint32_t maxX, uint32_t maxY)+{
- struct dw_hdmi *hdmi = container_of(connector, struct dw_hdmi,
connector);- struct edid *edid;
- int ret;
- ret = drm_helper_probe_single_connector_modes(connector, maxX, maxY);
- edid = connector->edid_blob_ptr;
- if (edid) {
hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);hdmi->sink_has_audio = drm_detect_monitor_audio(edid);/* Store the ELD */drm_edid_to_eld(connector, edid);- } else {
hdmi->sink_is_hdmi = false;hdmi->sink_has_audio = false;- }
- return ret;
+}
static enum drm_connector_status dw_hdmi_connector_detect(struct drm_connector *connector, bool force) { @@ -1444,12 +1468,8 @@ static int dw_hdmi_connector_get_modes(struct drm_connector *connector) dev_dbg(hdmi->dev, "got edid: width[%d] x height[%d]\n", edid->width_cm, edid->height_cm);
hdmi->sink_is_hdmi = drm_detect_hdmi_monitor(edid);hdmi->sink_has_audio = drm_detect_monitor_audio(edid);/* Store the ELD */drm_edid_to_eld(connector, edid);@@ -1496,7 +1516,7 @@ static void dw_hdmi_connector_force(struct drm_connector *connector)
static const struct drm_connector_funcs dw_hdmi_connector_funcs = { .dpms = drm_atomic_helper_connector_dpms,
- .fill_modes = drm_helper_probe_single_connector_modes,
- .fill_modes = dw_hdmi_connector_fill_modes, .detect = dw_hdmi_connector_detect, .destroy = dw_hdmi_connector_destroy, .force = dw_hdmi_connector_force,
dri-devel@lists.freedesktop.org
-
James Le Cuirot -
Russell King - ARM Linux