From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753409AbbJGJsv (ORCPT ); Wed, 7 Oct 2015 05:48:51 -0400 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:44904 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752283AbbJGJsr (ORCPT ); Wed, 7 Oct 2015 05:48:47 -0400 Date: Wed, 7 Oct 2015 10:48:32 +0100 From: Russell King - ARM Linux To: Yakir Yang Cc: linux-rockchip@lists.infradead.org, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org, linux-arm-kernel@lists.infradead.org, Philipp Zabel , Andy Yan , Fabio Estevam , Sascha Hauer , Jon Nettleton , David Airlie Subject: Re: [PATCH 10/12] drm: bridge/dw_hdmi: fix phy enable/disable handling Message-ID: <20151007094831.GF21513@n2100.arm.linux.org.uk> References: <20150808160251.GM7557@n2100.arm.linux.org.uk> <56149A11.4070102@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <56149A11.4070102@rock-chips.com> 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 On Wed, Oct 07, 2015 at 12:05:37PM +0800, Yakir Yang wrote: > > > On 08/09/2015 12:04 AM, Russell King wrote: > >The dw_hdmi enable/disable handling is particularly weak in several > >regards: > >* The hotplug interrupt could call hdmi_poweron() or hdmi_poweroff() > > while DRM is setting a mode, which could race with a mode being set. > >* Hotplug will always re-enable the phy whenever it detects an active > > hotplug signal, even if DRM has disabled the output. > > > >Resolve all of these by introducing a mutex to prevent races, and a > >state-tracking bool so we know whether DRM wishes the output to be > >enabled. We choose to use our own mutex rather than ->struct_mutex > >so that we can still process interrupts in a timely fashion. > > > >Signed-off-by: Russell King > >--- > > drivers/gpu/drm/bridge/dw_hdmi.c | 29 ++++++++++++++++++++++------- > > 1 file changed, 22 insertions(+), 7 deletions(-) > > > >diff --git a/drivers/gpu/drm/bridge/dw_hdmi.c b/drivers/gpu/drm/bridge/dw_hdmi.c > >index 7b8a4e942a71..0ee188930d26 100644 > >--- a/drivers/gpu/drm/bridge/dw_hdmi.c > >+++ b/drivers/gpu/drm/bridge/dw_hdmi.c > >@@ -125,6 +125,9 @@ struct dw_hdmi { > > bool sink_is_hdmi; > > bool sink_has_audio; > >+ struct mutex mutex; /* for state below and previous_mode */ > >+ bool disabled; /* DRM has disabled our bridge */ > >+ > > spinlock_t audio_lock; > > struct mutex audio_mutex; > > unsigned int sample_rate; > >@@ -1389,8 +1392,12 @@ static void dw_hdmi_bridge_mode_set(struct drm_bridge *bridge, > > { > > struct dw_hdmi *hdmi = bridge->driver_private; > >+ mutex_lock(&hdmi->mutex); > >+ > > /* Store the display mode for plugin/DKMS poweron events */ > > memcpy(&hdmi->previous_mode, mode, sizeof(hdmi->previous_mode)); > >+ > >+ mutex_unlock(&hdmi->mutex); > > } > > static bool dw_hdmi_bridge_mode_fixup(struct drm_bridge *bridge, > >@@ -1404,14 +1411,20 @@ static void dw_hdmi_bridge_disable(struct drm_bridge *bridge) > > { > > struct dw_hdmi *hdmi = bridge->driver_private; > >+ mutex_lock(&hdmi->mutex); > >+ hdmi->disabled = true; > > dw_hdmi_poweroff(hdmi); > >+ mutex_unlock(&hdmi->mutex); > > } > > static void dw_hdmi_bridge_enable(struct drm_bridge *bridge) > > { > > struct dw_hdmi *hdmi = bridge->driver_private; > >+ mutex_lock(&hdmi->mutex); > > dw_hdmi_poweron(hdmi); > >+ hdmi->disabled = false; > >+ mutex_unlock(&hdmi->mutex); > > } > > static void dw_hdmi_bridge_nop(struct drm_bridge *bridge) > >@@ -1534,20 +1547,20 @@ static irqreturn_t dw_hdmi_irq(int irq, void *dev_id) > > phy_int_pol = hdmi_readb(hdmi, HDMI_PHY_POL0); > > if (intr_stat & HDMI_IH_PHY_STAT0_HPD) { > >+ hdmi_modb(hdmi, ~phy_int_pol, HDMI_PHY_HPD, HDMI_PHY_POL0); > >+ mutex_lock(&hdmi->mutex); > > if (phy_int_pol & HDMI_PHY_HPD) { > > dev_dbg(hdmi->dev, "EVENT=plugin\n"); > >- hdmi_modb(hdmi, 0, HDMI_PHY_HPD, HDMI_PHY_POL0); > >- > >- dw_hdmi_poweron(hdmi); > >+ if (!hdmi->disabled) > >+ dw_hdmi_poweron(hdmi); > > } else { > > dev_dbg(hdmi->dev, "EVENT=plugout\n"); > >- hdmi_modb(hdmi, HDMI_PHY_HPD, HDMI_PHY_HPD, > >- HDMI_PHY_POL0); > >- > >- dw_hdmi_poweroff(hdmi); > >+ if (!hdmi->disabled) > >+ dw_hdmi_poweroff(hdmi); > > Just like my reply on 08/12, I thought this could be removed, so > poweron/poweroff would only be called with bridge->enable/ > bridge->disable, them maybe no need mutex here. The bridge enable/disable methods do not get called on hotplug changes. [ 1.363011] dwhdmi-imx 120000.hdmi: Detected HDMI controller 0x13:0xa:0xa0:0xc1 [ 1.371341] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(I:RX----,HPD P:RX3210,HPD S:RX----,HPD) [ 1.381345] imx-drm display-subsystem: bound 120000.hdmi (ops dw_hdmi_imx_ops) [ 1.448691] dwhdmi-imx 120000.hdmi: dw_hdmi_bridge_disable() [ 1.450963] dwhdmi-imx 120000.hdmi: dw_hdmi_bridge_enable() and then unplugging and re-plugging the HDMI cable: [ 68.307505] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(I:RX----,HPD P:RX3210,--- S:RX----,---) [ 73.813970] dwhdmi-imx 120000.hdmi: dw_hdmi_irq(I:RX----,HPD P:RX3210,HPD S:RX----,HPD) As you can see, during the period of disconnection for five seconds, dw_hdmi_bridge_disable() was not called. So, without the code above, we'd be needlessly wasting power with the bridge enabled, trying to drive a disconnected display. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.