Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S944402AbcJaQhz (ORCPT ); Mon, 31 Oct 2016 12:37:55 -0400 Received: from pandora.armlinux.org.uk ([78.32.30.218]:32782 "EHLO pandora.armlinux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S933807AbcJaQhv (ORCPT ); Mon, 31 Oct 2016 12:37:51 -0400 Date: Mon, 31 Oct 2016 16:37:36 +0000 From: Russell King - ARM Linux To: James Le Cuirot Cc: David Airlie , Daniel Vetter , Philipp Zabel , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH] drm/bridge: dw-hdmi: Set sink_is_hdmi and sink_has_audio in mode_set Message-ID: <20161031163736.GL1041@n2100.armlinux.org.uk> References: <20161030135625.11117-1-chewi@gentoo.org> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20161030135625.11117-1-chewi@gentoo.org> User-Agent: Mutt/1.5.23 (2014-03-12) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 4551 Lines: 116 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.