From patchwork Mon Oct 31 16:37:36 2016 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: "Russell King (Oracle)" X-Patchwork-Id: 9406765 Return-Path: Received: from mail.wl.linuxfoundation.org (pdx-wl-mail.web.codeaurora.org [172.30.200.125]) by pdx-korg-patchwork.web.codeaurora.org (Postfix) with ESMTP id 6822560721 for ; Tue, 1 Nov 2016 01:08:40 +0000 (UTC) Received: from mail.wl.linuxfoundation.org (localhost [127.0.0.1]) by mail.wl.linuxfoundation.org (Postfix) with ESMTP id 54C8828ADB for ; Tue, 1 Nov 2016 01:08:40 +0000 (UTC) Received: by mail.wl.linuxfoundation.org (Postfix, from userid 486) id 4840729385; Tue, 1 Nov 2016 01:08:40 +0000 (UTC) X-Spam-Checker-Version: SpamAssassin 3.3.1 (2010-03-16) on pdx-wl-mail.web.codeaurora.org X-Spam-Level: X-Spam-Status: No, score=-4.1 required=2.0 tests=BAYES_00,DKIM_SIGNED, RCVD_IN_DNSWL_MED,T_DKIM_INVALID autolearn=ham version=3.3.1 Received: from gabe.freedesktop.org (gabe.freedesktop.org [131.252.210.177]) (using TLSv1.2 with cipher DHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by mail.wl.linuxfoundation.org (Postfix) with ESMTPS id 03E2328ADB for ; Tue, 1 Nov 2016 01:08:39 +0000 (UTC) Received: from gabe.freedesktop.org (localhost [127.0.0.1]) by gabe.freedesktop.org (Postfix) with ESMTP id F08BD6E39C; Tue, 1 Nov 2016 01:08:28 +0000 (UTC) X-Original-To: dri-devel@lists.freedesktop.org Delivered-To: dri-devel@lists.freedesktop.org Received: from pandora.armlinux.org.uk (pandora.armlinux.org.uk [IPv6:2001:4d48:ad52:3201:214:fdff:fe10:1be6]) by gabe.freedesktop.org (Postfix) with ESMTPS id B52946E352 for ; Mon, 31 Oct 2016 16:37:48 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; q=dns/txt; c=relaxed/relaxed; d=armlinux.org.uk; s=pandora-2014; h=Sender:In-Reply-To:Content-Type:MIME-Version:References:Message-ID:Subject:Cc:To:From:Date; bh=JFRlLuS71I17vrQ9QEXpFhGNSj79v4moqyGpVX3Y/aQ=; b=n4W0hjuff3oXYQ9C2u73AVfIWuKfGDqD05OLX6WP1ia0wtWOf30EFv1oIDwh4QUYp9zkZdFjy1wh4GpjiJZ+wrWmf5iO88zxXsvtdBrGMTrhxsoO77eYj5Ksbtx5Z0F8je5y15oIwF16bZXkz/qhPYxHjC0C7vxEDhkcy5mrfz4=; Received: from n2100.armlinux.org.uk ([2001:4d48:ad52:3201:214:fdff:fe10:4f86]:44983) by pandora.armlinux.org.uk with esmtpsa (TLSv1:DHE-RSA-AES256-SHA:256) (Exim 4.82_1-5b7a7c0-XX) (envelope-from ) id 1c1Fae-0007sC-78; Mon, 31 Oct 2016 16:37:40 +0000 Received: from linux by n2100.armlinux.org.uk with local (Exim 4.76) (envelope-from ) id 1c1Fab-0003Hm-3Q; Mon, 31 Oct 2016 16:37:37 +0000 Date: Mon, 31 Oct 2016 16:37:36 +0000 From: Russell King - ARM Linux To: James Le Cuirot 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-Disposition: inline In-Reply-To: <20161030135625.11117-1-chewi@gentoo.org> User-Agent: Mutt/1.5.23 (2014-03-12) X-Mailman-Approved-At: Tue, 01 Nov 2016 01:08:23 +0000 Cc: Daniel Vetter , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org X-BeenThere: dri-devel@lists.freedesktop.org X-Mailman-Version: 2.1.18 Precedence: list List-Id: Direct Rendering Infrastructure - Development List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" X-Virus-Scanned: ClamAV using ClamSMTP 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,