Re: [PATCH] drm/bridge: dw-hdmi: Set sink_is_hdmi and sink_has_audio in mode_set

[Russell King - ARM Linux <linux@armlinux.org.uk>]

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,


-- 
RMK's Patch system: http://www.armlinux.org.uk/developer/patches/
FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up
according to speedtest.net.