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.
_______________________________________________
dri-devel mailing list
dri-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/dri-devel
