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,