From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755245AbbJGTT5 (ORCPT ); Wed, 7 Oct 2015 15:19:57 -0400 Received: from pandora.arm.linux.org.uk ([78.32.30.218]:46093 "EHLO pandora.arm.linux.org.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751122AbbJGTTz (ORCPT ); Wed, 7 Oct 2015 15:19:55 -0400 Date: Wed, 7 Oct 2015 20:17:31 +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: <20151007191731.GA32064@n2100.arm.linux.org.uk> References: <20150808160251.GM7557@n2100.arm.linux.org.uk> <56149A11.4070102@rock-chips.com> <20151007094831.GF21513@n2100.arm.linux.org.uk> <5614F68B.6050808@rock-chips.com> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <5614F68B.6050808@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 06:40:11PM +0800, Yakir Yang wrote: > > > On 10/07/2015 05:48 PM, Russell King - ARM Linux wrote: > >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. > > Strangely, I do see bridge enable/disable in my side, past the log and > dump_stack bellow. > > And I guess your HDMI maybe not really hot pluged, could you confirm that > the "status" of HDMI display card have been changed between "connected" > and "disconnected". It does. > Do see bridge_disable when I unpluging the HDMI cable > [ 16.358717] dwhdmi-rockchip ff980000.hdmi: EVENT=plugout > [ 20.613221] [] (unwind_backtrace) from [] > (show_stack+0x20/0x24) > [ 20.631561] [] (show_stack) from [] > (dump_stack+0x70/0x8c) > [ 20.649337] [] (dump_stack) from [] > (dw_hdmi_bridge_disable+0x1c/0x88) > [ 20.668178] [] (dw_hdmi_bridge_disable) from [] > (drm_encoder_disable+0x34/0x78) > [ 20.687857] [] (drm_encoder_disable) from [] > (__drm_helper_disable_unused_functions+0x68/0xe4) > [ 20.708975] [] (__drm_helper_disable_unused_functions) from > [] (drm_crtc_helper_set_config+0x128/0x85c) > [ 20.731180] [] (drm_crtc_helper_set_config) from [] > (drm_mode_set_config_internal+0x58/0xdc) > [ 20.752507] [] (drm_mode_set_config_internal) from [] > (commit_crtc_state+0x124/0x1ec) > [ 20.773342] [] (commit_crtc_state) from [] > (atomic_commit.isra.3+0x5c/0xc8) > [ 20.793397] [] (atomic_commit.isra.3) from [] > (drm_atomic_commit+0x1c/0x20) > [ 20.813467] [] (drm_atomic_commit) from [] > (drm_mode_setcrtc+0x324/0x3e4) > [ 20.833379] [] (drm_mode_setcrtc) from [] > (drm_ioctl+0x304/0x478) > [ 20.852557] [] (drm_ioctl) from [] > (do_vfs_ioctl+0x494/0x5a8) > [ 20.871377] [] (do_vfs_ioctl) from [] > (SyS_ioctl+0x5c/0x84) > [ 20.890038] [] (SyS_ioctl) from [] > (__sys_trace_return+0x0/0x14) Your userspace is issuing an ioctl to disable the output. I guess you have other active outputs besides HDMI. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net. From mboxrd@z Thu Jan 1 00:00:00 1970 From: Russell King - ARM Linux Subject: Re: [PATCH 10/12] drm: bridge/dw_hdmi: fix phy enable/disable handling Date: Wed, 7 Oct 2015 20:17:31 +0100 Message-ID: <20151007191731.GA32064@n2100.arm.linux.org.uk> References: <20150808160251.GM7557@n2100.arm.linux.org.uk> <56149A11.4070102@rock-chips.com> <20151007094831.GF21513@n2100.arm.linux.org.uk> <5614F68B.6050808@rock-chips.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <5614F68B.6050808@rock-chips.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Yakir Yang Cc: Fabio Estevam , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, Andy Yan , linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gV2VkLCBPY3QgMDcsIDIwMTUgYXQgMDY6NDA6MTFQTSArMDgwMCwgWWFraXIgWWFuZyB3cm90 ZToKPiAKPiAKPiBPbiAxMC8wNy8yMDE1IDA1OjQ4IFBNLCBSdXNzZWxsIEtpbmcgLSBBUk0gTGlu dXggd3JvdGU6Cj4gPk9uIFdlZCwgT2N0IDA3LCAyMDE1IGF0IDEyOjA1OjM3UE0gKzA4MDAsIFlh a2lyIFlhbmcgd3JvdGU6Cj4gPj4KPiA+Pk9uIDA4LzA5LzIwMTUgMTI6MDQgQU0sIFJ1c3NlbGwg S2luZyB3cm90ZToKPiA+Pj5UaGUgZHdfaGRtaSBlbmFibGUvZGlzYWJsZSBoYW5kbGluZyBpcyBw YXJ0aWN1bGFybHkgd2VhayBpbiBzZXZlcmFsCj4gPj4+cmVnYXJkczoKPiA+Pj4qIFRoZSBob3Rw bHVnIGludGVycnVwdCBjb3VsZCBjYWxsIGhkbWlfcG93ZXJvbigpIG9yIGhkbWlfcG93ZXJvZmYo KQo+ID4+PiAgIHdoaWxlIERSTSBpcyBzZXR0aW5nIGEgbW9kZSwgd2hpY2ggY291bGQgcmFjZSB3 aXRoIGEgbW9kZSBiZWluZyBzZXQuCj4gPj4+KiBIb3RwbHVnIHdpbGwgYWx3YXlzIHJlLWVuYWJs ZSB0aGUgcGh5IHdoZW5ldmVyIGl0IGRldGVjdHMgYW4gYWN0aXZlCj4gPj4+ICAgaG90cGx1ZyBz aWduYWwsIGV2ZW4gaWYgRFJNIGhhcyBkaXNhYmxlZCB0aGUgb3V0cHV0Lgo+ID4+Pgo+ID4+PlJl c29sdmUgYWxsIG9mIHRoZXNlIGJ5IGludHJvZHVjaW5nIGEgbXV0ZXggdG8gcHJldmVudCByYWNl cywgYW5kIGEKPiA+Pj5zdGF0ZS10cmFja2luZyBib29sIHNvIHdlIGtub3cgd2hldGhlciBEUk0g d2lzaGVzIHRoZSBvdXRwdXQgdG8gYmUKPiA+Pj5lbmFibGVkLiAgV2UgY2hvb3NlIHRvIHVzZSBv dXIgb3duIG11dGV4IHJhdGhlciB0aGFuIC0+c3RydWN0X211dGV4Cj4gPj4+c28gdGhhdCB3ZSBj YW4gc3RpbGwgcHJvY2VzcyBpbnRlcnJ1cHRzIGluIGEgdGltZWx5IGZhc2hpb24uCj4gPj4+Cj4g Pj4+U2lnbmVkLW9mZi1ieTogUnVzc2VsbCBLaW5nIDxybWsra2VybmVsQGFybS5saW51eC5vcmcu dWs+Cj4gPj4+LS0tCj4gPj4+ICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3X2hkbWkuYyB8IDI5 ICsrKysrKysrKysrKysrKysrKysrKystLS0tLS0tCj4gPj4+ICAxIGZpbGUgY2hhbmdlZCwgMjIg aW5zZXJ0aW9ucygrKSwgNyBkZWxldGlvbnMoLSkKPiA+Pj4KPiA+Pj5kaWZmIC0tZ2l0IGEvZHJp dmVycy9ncHUvZHJtL2JyaWRnZS9kd19oZG1pLmMgYi9kcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3 X2hkbWkuYwo+ID4+PmluZGV4IDdiOGE0ZTk0MmE3MS4uMGVlMTg4OTMwZDI2IDEwMDY0NAo+ID4+ Pi0tLSBhL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHdfaGRtaS5jCj4gPj4+KysrIGIvZHJpdmVy cy9ncHUvZHJtL2JyaWRnZS9kd19oZG1pLmMKPiA+Pj5AQCAtMTI1LDYgKzEyNSw5IEBAIHN0cnVj dCBkd19oZG1pIHsKPiA+Pj4gIAlib29sIHNpbmtfaXNfaGRtaTsKPiA+Pj4gIAlib29sIHNpbmtf aGFzX2F1ZGlvOwo+ID4+PisJc3RydWN0IG11dGV4IG11dGV4OwkJLyogZm9yIHN0YXRlIGJlbG93 IGFuZCBwcmV2aW91c19tb2RlICovCj4gPj4+Kwlib29sIGRpc2FibGVkOwkJCS8qIERSTSBoYXMg ZGlzYWJsZWQgb3VyIGJyaWRnZSAqLwo+ID4+PisKPiA+Pj4gIAlzcGlubG9ja190IGF1ZGlvX2xv Y2s7Cj4gPj4+ICAJc3RydWN0IG11dGV4IGF1ZGlvX211dGV4Owo+ID4+PiAgCXVuc2lnbmVkIGlu dCBzYW1wbGVfcmF0ZTsKPiA+Pj5AQCAtMTM4OSw4ICsxMzkyLDEyIEBAIHN0YXRpYyB2b2lkIGR3 X2hkbWlfYnJpZGdlX21vZGVfc2V0KHN0cnVjdCBkcm1fYnJpZGdlICpicmlkZ2UsCj4gPj4+ICB7 Cj4gPj4+ICAJc3RydWN0IGR3X2hkbWkgKmhkbWkgPSBicmlkZ2UtPmRyaXZlcl9wcml2YXRlOwo+ ID4+PisJbXV0ZXhfbG9jaygmaGRtaS0+bXV0ZXgpOwo+ID4+PisKPiA+Pj4gIAkvKiBTdG9yZSB0 aGUgZGlzcGxheSBtb2RlIGZvciBwbHVnaW4vREtNUyBwb3dlcm9uIGV2ZW50cyAqLwo+ID4+PiAg CW1lbWNweSgmaGRtaS0+cHJldmlvdXNfbW9kZSwgbW9kZSwgc2l6ZW9mKGhkbWktPnByZXZpb3Vz X21vZGUpKTsKPiA+Pj4rCj4gPj4+KwltdXRleF91bmxvY2soJmhkbWktPm11dGV4KTsKPiA+Pj4g IH0KPiA+Pj4gIHN0YXRpYyBib29sIGR3X2hkbWlfYnJpZGdlX21vZGVfZml4dXAoc3RydWN0IGRy bV9icmlkZ2UgKmJyaWRnZSwKPiA+Pj5AQCAtMTQwNCwxNCArMTQxMSwyMCBAQCBzdGF0aWMgdm9p ZCBkd19oZG1pX2JyaWRnZV9kaXNhYmxlKHN0cnVjdCBkcm1fYnJpZGdlICpicmlkZ2UpCj4gPj4+ ICB7Cj4gPj4+ICAJc3RydWN0IGR3X2hkbWkgKmhkbWkgPSBicmlkZ2UtPmRyaXZlcl9wcml2YXRl Owo+ID4+PisJbXV0ZXhfbG9jaygmaGRtaS0+bXV0ZXgpOwo+ID4+PisJaGRtaS0+ZGlzYWJsZWQg PSB0cnVlOwo+ID4+PiAgCWR3X2hkbWlfcG93ZXJvZmYoaGRtaSk7Cj4gPj4+KwltdXRleF91bmxv Y2soJmhkbWktPm11dGV4KTsKPiA+Pj4gIH0KPiA+Pj4gIHN0YXRpYyB2b2lkIGR3X2hkbWlfYnJp ZGdlX2VuYWJsZShzdHJ1Y3QgZHJtX2JyaWRnZSAqYnJpZGdlKQo+ID4+PiAgewo+ID4+PiAgCXN0 cnVjdCBkd19oZG1pICpoZG1pID0gYnJpZGdlLT5kcml2ZXJfcHJpdmF0ZTsKPiA+Pj4rCW11dGV4 X2xvY2soJmhkbWktPm11dGV4KTsKPiA+Pj4gIAlkd19oZG1pX3Bvd2Vyb24oaGRtaSk7Cj4gPj4+ KwloZG1pLT5kaXNhYmxlZCA9IGZhbHNlOwo+ID4+PisJbXV0ZXhfdW5sb2NrKCZoZG1pLT5tdXRl eCk7Cj4gPj4+ICB9Cj4gPj4+ICBzdGF0aWMgdm9pZCBkd19oZG1pX2JyaWRnZV9ub3Aoc3RydWN0 IGRybV9icmlkZ2UgKmJyaWRnZSkKPiA+Pj5AQCAtMTUzNCwyMCArMTU0NywyMCBAQCBzdGF0aWMg aXJxcmV0dXJuX3QgZHdfaGRtaV9pcnEoaW50IGlycSwgdm9pZCAqZGV2X2lkKQo+ID4+PiAgCXBo eV9pbnRfcG9sID0gaGRtaV9yZWFkYihoZG1pLCBIRE1JX1BIWV9QT0wwKTsKPiA+Pj4gIAlpZiAo aW50cl9zdGF0ICYgSERNSV9JSF9QSFlfU1RBVDBfSFBEKSB7Cj4gPj4+KwkJaGRtaV9tb2RiKGhk bWksIH5waHlfaW50X3BvbCwgSERNSV9QSFlfSFBELCBIRE1JX1BIWV9QT0wwKTsKPiA+Pj4rCQlt dXRleF9sb2NrKCZoZG1pLT5tdXRleCk7Cj4gPj4+ICAJCWlmIChwaHlfaW50X3BvbCAmIEhETUlf UEhZX0hQRCkgewo+ID4+PiAgCQkJZGV2X2RiZyhoZG1pLT5kZXYsICJFVkVOVD1wbHVnaW5cbiIp Owo+ID4+Pi0JCQloZG1pX21vZGIoaGRtaSwgMCwgSERNSV9QSFlfSFBELCBIRE1JX1BIWV9QT0ww KTsKPiA+Pj4tCj4gPj4+LQkJCWR3X2hkbWlfcG93ZXJvbihoZG1pKTsKPiA+Pj4rCQkJaWYgKCFo ZG1pLT5kaXNhYmxlZCkKPiA+Pj4rCQkJCWR3X2hkbWlfcG93ZXJvbihoZG1pKTsKPiA+Pj4gIAkJ fSBlbHNlIHsKPiA+Pj4gIAkJCWRldl9kYmcoaGRtaS0+ZGV2LCAiRVZFTlQ9cGx1Z291dFxuIik7 Cj4gPj4+LQkJCWhkbWlfbW9kYihoZG1pLCBIRE1JX1BIWV9IUEQsIEhETUlfUEhZX0hQRCwKPiA+ Pj4tCQkJCSAgSERNSV9QSFlfUE9MMCk7Cj4gPj4+LQo+ID4+Pi0JCQlkd19oZG1pX3Bvd2Vyb2Zm KGhkbWkpOwo+ID4+PisJCQlpZiAoIWhkbWktPmRpc2FibGVkKQo+ID4+PisJCQkJZHdfaGRtaV9w b3dlcm9mZihoZG1pKTsKPiA+Pkp1c3QgbGlrZSBteSByZXBseSBvbiAwOC8xMiwgSSB0aG91Z2h0 IHRoaXMgY291bGQgYmUgcmVtb3ZlZCwgc28KPiA+PnBvd2Vyb24vcG93ZXJvZmYgd291bGQgb25s eSBiZSBjYWxsZWQgd2l0aCBicmlkZ2UtPmVuYWJsZS8KPiA+PmJyaWRnZS0+ZGlzYWJsZSwgdGhl bSBtYXliZSBubyBuZWVkIG11dGV4IGhlcmUuCj4gPlRoZSBicmlkZ2UgZW5hYmxlL2Rpc2FibGUg bWV0aG9kcyBkbyBub3QgZ2V0IGNhbGxlZCBvbiBob3RwbHVnIGNoYW5nZXMuCj4gPgo+ID5bICAg IDEuMzYzMDExXSBkd2hkbWktaW14IDEyMDAwMC5oZG1pOiBEZXRlY3RlZCBIRE1JIGNvbnRyb2xs ZXIgMHgxMzoweGE6MHhhMDoweGMxCj4gPlsgICAgMS4zNzEzNDFdIGR3aGRtaS1pbXggMTIwMDAw LmhkbWk6IGR3X2hkbWlfaXJxKEk6UlgtLS0tLEhQRCBQOlJYMzIxMCxIUEQgUzpSWC0tLS0sSFBE KQo+ID5bICAgIDEuMzgxMzQ1XSBpbXgtZHJtIGRpc3BsYXktc3Vic3lzdGVtOiBib3VuZCAxMjAw MDAuaGRtaSAob3BzIGR3X2hkbWlfaW14X29wcykKPiA+WyAgICAxLjQ0ODY5MV0gZHdoZG1pLWlt eCAxMjAwMDAuaGRtaTogZHdfaGRtaV9icmlkZ2VfZGlzYWJsZSgpCj4gPlsgICAgMS40NTA5NjNd IGR3aGRtaS1pbXggMTIwMDAwLmhkbWk6IGR3X2hkbWlfYnJpZGdlX2VuYWJsZSgpCj4gPgo+ID5h bmQgdGhlbiB1bnBsdWdnaW5nIGFuZCByZS1wbHVnZ2luZyB0aGUgSERNSSBjYWJsZToKPiA+Cj4g PlsgICA2OC4zMDc1MDVdIGR3aGRtaS1pbXggMTIwMDAwLmhkbWk6IGR3X2hkbWlfaXJxKEk6Ulgt LS0tLEhQRCBQOlJYMzIxMCwtLS0gUzpSWC0tLS0sLS0tKQo+ID5bICAgNzMuODEzOTcwXSBkd2hk bWktaW14IDEyMDAwMC5oZG1pOiBkd19oZG1pX2lycShJOlJYLS0tLSxIUEQgUDpSWDMyMTAsSFBE IFM6UlgtLS0tLEhQRCkKPiA+Cj4gPkFzIHlvdSBjYW4gc2VlLCBkdXJpbmcgdGhlIHBlcmlvZCBv ZiBkaXNjb25uZWN0aW9uIGZvciBmaXZlIHNlY29uZHMsCj4gPmR3X2hkbWlfYnJpZGdlX2Rpc2Fi bGUoKSB3YXMgbm90IGNhbGxlZC4KPiA+Cj4gPlNvLCB3aXRob3V0IHRoZSBjb2RlIGFib3ZlLCB3 ZSdkIGJlIG5lZWRsZXNzbHkgd2FzdGluZyBwb3dlciB3aXRoIHRoZQo+ID5icmlkZ2UgZW5hYmxl ZCwgdHJ5aW5nIHRvIGRyaXZlIGEgZGlzY29ubmVjdGVkIGRpc3BsYXkuCj4gCj4gU3RyYW5nZWx5 LCBJIGRvIHNlZSBicmlkZ2UgZW5hYmxlL2Rpc2FibGUgaW4gbXkgc2lkZSwgcGFzdCB0aGUgbG9n IGFuZAo+IGR1bXBfc3RhY2sgYmVsbG93Lgo+IAo+IEFuZCBJIGd1ZXNzIHlvdXIgSERNSSBtYXli ZSBub3QgcmVhbGx5IGhvdCBwbHVnZWQsIGNvdWxkIHlvdSBjb25maXJtIHRoYXQKPiB0aGUgInN0 YXR1cyIgb2YgSERNSSBkaXNwbGF5IGNhcmQgaGF2ZSBiZWVuIGNoYW5nZWQgYmV0d2VlbiAiY29u bmVjdGVkIgo+IGFuZCAiZGlzY29ubmVjdGVkIi4KCkl0IGRvZXMuCgo+IERvIHNlZSBicmlkZ2Vf ZGlzYWJsZSB3aGVuIEkgdW5wbHVnaW5nIHRoZSBIRE1JIGNhYmxlCj4gWyAgIDE2LjM1ODcxN10g ZHdoZG1pLXJvY2tjaGlwIGZmOTgwMDAwLmhkbWk6IEVWRU5UPXBsdWdvdXQKPiBbICAgMjAuNjEz MjIxXSBbPGMwMTBlMDMwPl0gKHVud2luZF9iYWNrdHJhY2UpIGZyb20gWzxjMDEwYTRlMD5dCj4g KHNob3dfc3RhY2srMHgyMC8weDI0KQo+IFsgICAyMC42MzE1NjFdIFs8YzAxMGE0ZTA+XSAoc2hv d19zdGFjaykgZnJvbSBbPGMwODk2ODI4Pl0KPiAoZHVtcF9zdGFjaysweDcwLzB4OGMpCj4gWyAg IDIwLjY0OTMzN10gWzxjMDg5NjgyOD5dIChkdW1wX3N0YWNrKSBmcm9tIFs8YzA0MTQwMzg+XQo+ IChkd19oZG1pX2JyaWRnZV9kaXNhYmxlKzB4MWMvMHg4OCkKPiBbICAgMjAuNjY4MTc4XSBbPGMw NDE0MDM4Pl0gKGR3X2hkbWlfYnJpZGdlX2Rpc2FibGUpIGZyb20gWzxjMDNlMzg4OD5dCj4gKGRy bV9lbmNvZGVyX2Rpc2FibGUrMHgzNC8weDc4KQo+IFsgICAyMC42ODc4NTddIFs8YzAzZTM4ODg+ XSAoZHJtX2VuY29kZXJfZGlzYWJsZSkgZnJvbSBbPGMwM2UzYjFjPl0KPiAoX19kcm1faGVscGVy X2Rpc2FibGVfdW51c2VkX2Z1bmN0aW9ucysweDY4LzB4ZTQpCj4gWyAgIDIwLjcwODk3NV0gWzxj MDNlM2IxYz5dIChfX2RybV9oZWxwZXJfZGlzYWJsZV91bnVzZWRfZnVuY3Rpb25zKSBmcm9tCj4g WzxjMDNlNDMyMD5dIChkcm1fY3J0Y19oZWxwZXJfc2V0X2NvbmZpZysweDEyOC8weDg1YykKPiBb ICAgMjAuNzMxMTgwXSBbPGMwM2U0MzIwPl0gKGRybV9jcnRjX2hlbHBlcl9zZXRfY29uZmlnKSBm cm9tIFs8YzAzZjVlM2M+XQo+IChkcm1fbW9kZV9zZXRfY29uZmlnX2ludGVybmFsKzB4NTgvMHhk YykKPiBbICAgMjAuNzUyNTA3XSBbPGMwM2Y1ZTNjPl0gKGRybV9tb2RlX3NldF9jb25maWdfaW50 ZXJuYWwpIGZyb20gWzxjMDQwNWVkMD5dCj4gKGNvbW1pdF9jcnRjX3N0YXRlKzB4MTI0LzB4MWVj KQo+IFsgICAyMC43NzMzNDJdIFs8YzA0MDVlZDA+XSAoY29tbWl0X2NydGNfc3RhdGUpIGZyb20g WzxjMDQwNTVkND5dCj4gKGF0b21pY19jb21taXQuaXNyYS4zKzB4NWMvMHhjOCkKPiBbICAgMjAu NzkzMzk3XSBbPGMwNDA1NWQ0Pl0gKGF0b21pY19jb21taXQuaXNyYS4zKSBmcm9tIFs8YzA0MDU2 NWM+XQo+IChkcm1fYXRvbWljX2NvbW1pdCsweDFjLzB4MjApCj4gWyAgIDIwLjgxMzQ2N10gWzxj MDQwNTY1Yz5dIChkcm1fYXRvbWljX2NvbW1pdCkgZnJvbSBbPGMwM2ZhNDgwPl0KPiAoZHJtX21v ZGVfc2V0Y3J0YysweDMyNC8weDNlNCkKPiBbICAgMjAuODMzMzc5XSBbPGMwM2ZhNDgwPl0gKGRy bV9tb2RlX3NldGNydGMpIGZyb20gWzxjMDNlYjMyMD5dCj4gKGRybV9pb2N0bCsweDMwNC8weDQ3 OCkKPiBbICAgMjAuODUyNTU3XSBbPGMwM2ViMzIwPl0gKGRybV9pb2N0bCkgZnJvbSBbPGMwMjFm MDI0Pl0KPiAoZG9fdmZzX2lvY3RsKzB4NDk0LzB4NWE4KQo+IFsgICAyMC44NzEzNzddIFs8YzAy MWYwMjQ+XSAoZG9fdmZzX2lvY3RsKSBmcm9tIFs8YzAyMWYxOTQ+XQo+IChTeVNfaW9jdGwrMHg1 Yy8weDg0KQo+IFsgICAyMC44OTAwMzhdIFs8YzAyMWYxOTQ+XSAoU3lTX2lvY3RsKSBmcm9tIFs8 YzAxMDY0NmM+XQo+IChfX3N5c190cmFjZV9yZXR1cm4rMHgwLzB4MTQpCgpZb3VyIHVzZXJzcGFj ZSBpcyBpc3N1aW5nIGFuIGlvY3RsIHRvIGRpc2FibGUgdGhlIG91dHB1dC4gIEkgZ3Vlc3MgeW91 CmhhdmUgb3RoZXIgYWN0aXZlIG91dHB1dHMgYmVzaWRlcyBIRE1JLgoKLS0gCkZUVEMgYnJvYWRi YW5kIGZvciAwLjhtaWxlIGxpbmU6IGN1cnJlbnRseSBhdCA5LjZNYnBzIGRvd24gNDAwa2JwcyB1 cAphY2NvcmRpbmcgdG8gc3BlZWR0ZXN0Lm5ldC4KX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlz dHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9s aXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: linux@arm.linux.org.uk (Russell King - ARM Linux) Date: Wed, 7 Oct 2015 20:17:31 +0100 Subject: [PATCH 10/12] drm: bridge/dw_hdmi: fix phy enable/disable handling In-Reply-To: <5614F68B.6050808@rock-chips.com> References: <20150808160251.GM7557@n2100.arm.linux.org.uk> <56149A11.4070102@rock-chips.com> <20151007094831.GF21513@n2100.arm.linux.org.uk> <5614F68B.6050808@rock-chips.com> Message-ID: <20151007191731.GA32064@n2100.arm.linux.org.uk> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, Oct 07, 2015 at 06:40:11PM +0800, Yakir Yang wrote: > > > On 10/07/2015 05:48 PM, Russell King - ARM Linux wrote: > >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. > > Strangely, I do see bridge enable/disable in my side, past the log and > dump_stack bellow. > > And I guess your HDMI maybe not really hot pluged, could you confirm that > the "status" of HDMI display card have been changed between "connected" > and "disconnected". It does. > Do see bridge_disable when I unpluging the HDMI cable > [ 16.358717] dwhdmi-rockchip ff980000.hdmi: EVENT=plugout > [ 20.613221] [] (unwind_backtrace) from [] > (show_stack+0x20/0x24) > [ 20.631561] [] (show_stack) from [] > (dump_stack+0x70/0x8c) > [ 20.649337] [] (dump_stack) from [] > (dw_hdmi_bridge_disable+0x1c/0x88) > [ 20.668178] [] (dw_hdmi_bridge_disable) from [] > (drm_encoder_disable+0x34/0x78) > [ 20.687857] [] (drm_encoder_disable) from [] > (__drm_helper_disable_unused_functions+0x68/0xe4) > [ 20.708975] [] (__drm_helper_disable_unused_functions) from > [] (drm_crtc_helper_set_config+0x128/0x85c) > [ 20.731180] [] (drm_crtc_helper_set_config) from [] > (drm_mode_set_config_internal+0x58/0xdc) > [ 20.752507] [] (drm_mode_set_config_internal) from [] > (commit_crtc_state+0x124/0x1ec) > [ 20.773342] [] (commit_crtc_state) from [] > (atomic_commit.isra.3+0x5c/0xc8) > [ 20.793397] [] (atomic_commit.isra.3) from [] > (drm_atomic_commit+0x1c/0x20) > [ 20.813467] [] (drm_atomic_commit) from [] > (drm_mode_setcrtc+0x324/0x3e4) > [ 20.833379] [] (drm_mode_setcrtc) from [] > (drm_ioctl+0x304/0x478) > [ 20.852557] [] (drm_ioctl) from [] > (do_vfs_ioctl+0x494/0x5a8) > [ 20.871377] [] (do_vfs_ioctl) from [] > (SyS_ioctl+0x5c/0x84) > [ 20.890038] [] (SyS_ioctl) from [] > (__sys_trace_return+0x0/0x14) Your userspace is issuing an ioctl to disable the output. I guess you have other active outputs besides HDMI. -- FTTC broadband for 0.8mile line: currently at 9.6Mbps down 400kbps up according to speedtest.net.