From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754586AbbJHIfn (ORCPT ); Thu, 8 Oct 2015 04:35:43 -0400 Received: from lucky1.263xmail.com ([211.157.147.133]:35850 "EHLO lucky1.263xmail.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754060AbbJHIfh (ORCPT ); Thu, 8 Oct 2015 04:35:37 -0400 X-263anti-spam: KSV:0; X-MAIL-GRAY: 1 X-MAIL-DELIVERY: 0 X-KSVirus-check: 0 X-ABS-CHECKED: 4 X-ADDR-CHECKED: 0 X-RL-SENDER: ykk@rock-chips.com X-FST-TO: airlied@linux.ie X-SENDER-IP: 58.22.7.114 X-LOGIN-NAME: ykk@rock-chips.com X-UNIQUE-TAG: X-DNS-TYPE: 0 Message-ID: <56162ACA.3020501@rock-chips.com> Date: Thu, 08 Oct 2015 16:35:22 +0800 From: Yakir Yang User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.8.0 MIME-Version: 1.0 To: Russell King - ARM Linux 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 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> <20151007191731.GA32064@n2100.arm.linux.org.uk> In-Reply-To: <20151007191731.GA32064@n2100.arm.linux.org.uk> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Oh, I haven't noticed that those patches already have been merged into linux-next :-) On 10/08/2015 03:17 AM, Russell King - ARM Linux wrote: > 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. > > Yeah, I do have another active eDP screen, but after removed the eDP display card, I still see the bridge enable/disabled have been called. I try to track the some userspace code, but due to little knowledge about ChomeOS code, still can't found something directly. As drm framework won't make bridge disabled when connector plug out, so feel free to agree this isn't duplicate work. Thanks, - Yakir From mboxrd@z Thu Jan 1 00:00:00 1970 From: Yakir Yang Subject: Re: [PATCH 10/12] drm: bridge/dw_hdmi: fix phy enable/disable handling Date: Thu, 08 Oct 2015 16:35:22 +0800 Message-ID: <56162ACA.3020501@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> <20151007191731.GA32064@n2100.arm.linux.org.uk> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20151007191731.GA32064@n2100.arm.linux.org.uk> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Russell King - ARM Linux 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 Ck9oLCBJIGhhdmVuJ3Qgbm90aWNlZCB0aGF0IHRob3NlIHBhdGNoZXMgYWxyZWFkeSBoYXZlIGJl ZW4KbWVyZ2VkIGludG8gbGludXgtbmV4dCAgOi0pCgoKT24gMTAvMDgvMjAxNSAwMzoxNyBBTSwg UnVzc2VsbCBLaW5nIC0gQVJNIExpbnV4IHdyb3RlOgo+IE9uIFdlZCwgT2N0IDA3LCAyMDE1IGF0 IDA2OjQwOjExUE0gKzA4MDAsIFlha2lyIFlhbmcgd3JvdGU6Cj4+Cj4+IE9uIDEwLzA3LzIwMTUg MDU6NDggUE0sIFJ1c3NlbGwgS2luZyAtIEFSTSBMaW51eCB3cm90ZToKPj4+IE9uIFdlZCwgT2N0 IDA3LCAyMDE1IGF0IDEyOjA1OjM3UE0gKzA4MDAsIFlha2lyIFlhbmcgd3JvdGU6Cj4+Pj4gT24g MDgvMDkvMjAxNSAxMjowNCBBTSwgUnVzc2VsbCBLaW5nIHdyb3RlOgo+Pj4+PiBUaGUgZHdfaGRt aSBlbmFibGUvZGlzYWJsZSBoYW5kbGluZyBpcyBwYXJ0aWN1bGFybHkgd2VhayBpbiBzZXZlcmFs Cj4+Pj4+IHJlZ2FyZHM6Cj4+Pj4+ICogVGhlIGhvdHBsdWcgaW50ZXJydXB0IGNvdWxkIGNhbGwg aGRtaV9wb3dlcm9uKCkgb3IgaGRtaV9wb3dlcm9mZigpCj4+Pj4+ICAgIHdoaWxlIERSTSBpcyBz ZXR0aW5nIGEgbW9kZSwgd2hpY2ggY291bGQgcmFjZSB3aXRoIGEgbW9kZSBiZWluZyBzZXQuCj4+ Pj4+ICogSG90cGx1ZyB3aWxsIGFsd2F5cyByZS1lbmFibGUgdGhlIHBoeSB3aGVuZXZlciBpdCBk ZXRlY3RzIGFuIGFjdGl2ZQo+Pj4+PiAgICBob3RwbHVnIHNpZ25hbCwgZXZlbiBpZiBEUk0gaGFz IGRpc2FibGVkIHRoZSBvdXRwdXQuCj4+Pj4+Cj4+Pj4+IFJlc29sdmUgYWxsIG9mIHRoZXNlIGJ5 IGludHJvZHVjaW5nIGEgbXV0ZXggdG8gcHJldmVudCByYWNlcywgYW5kIGEKPj4+Pj4gc3RhdGUt dHJhY2tpbmcgYm9vbCBzbyB3ZSBrbm93IHdoZXRoZXIgRFJNIHdpc2hlcyB0aGUgb3V0cHV0IHRv IGJlCj4+Pj4+IGVuYWJsZWQuICBXZSBjaG9vc2UgdG8gdXNlIG91ciBvd24gbXV0ZXggcmF0aGVy IHRoYW4gLT5zdHJ1Y3RfbXV0ZXgKPj4+Pj4gc28gdGhhdCB3ZSBjYW4gc3RpbGwgcHJvY2VzcyBp bnRlcnJ1cHRzIGluIGEgdGltZWx5IGZhc2hpb24uCj4+Pj4+Cj4+Pj4+IFNpZ25lZC1vZmYtYnk6 IFJ1c3NlbGwgS2luZyA8cm1rK2tlcm5lbEBhcm0ubGludXgub3JnLnVrPgo+Pj4+PiAtLS0KPj4+ Pj4gICBkcml2ZXJzL2dwdS9kcm0vYnJpZGdlL2R3X2hkbWkuYyB8IDI5ICsrKysrKysrKysrKysr KysrKysrKystLS0tLS0tCj4+Pj4+ICAgMSBmaWxlIGNoYW5nZWQsIDIyIGluc2VydGlvbnMoKyks IDcgZGVsZXRpb25zKC0pCj4+Pj4+Cj4+Pj4+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0v YnJpZGdlL2R3X2hkbWkuYyBiL2RyaXZlcnMvZ3B1L2RybS9icmlkZ2UvZHdfaGRtaS5jCj4+Pj4+ IGluZGV4IDdiOGE0ZTk0MmE3MS4uMGVlMTg4OTMwZDI2IDEwMDY0NAo+Pj4+PiAtLS0gYS9kcml2 ZXJzL2dwdS9kcm0vYnJpZGdlL2R3X2hkbWkuYwo+Pj4+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0v YnJpZGdlL2R3X2hkbWkuYwo+Pj4+PiBAQCAtMTI1LDYgKzEyNSw5IEBAIHN0cnVjdCBkd19oZG1p IHsKPj4+Pj4gICAJYm9vbCBzaW5rX2lzX2hkbWk7Cj4+Pj4+ICAgCWJvb2wgc2lua19oYXNfYXVk aW87Cj4+Pj4+ICsJc3RydWN0IG11dGV4IG11dGV4OwkJLyogZm9yIHN0YXRlIGJlbG93IGFuZCBw cmV2aW91c19tb2RlICovCj4+Pj4+ICsJYm9vbCBkaXNhYmxlZDsJCQkvKiBEUk0gaGFzIGRpc2Fi bGVkIG91ciBicmlkZ2UgKi8KPj4+Pj4gKwo+Pj4+PiAgIAlzcGlubG9ja190IGF1ZGlvX2xvY2s7 Cj4+Pj4+ICAgCXN0cnVjdCBtdXRleCBhdWRpb19tdXRleDsKPj4+Pj4gICAJdW5zaWduZWQgaW50 IHNhbXBsZV9yYXRlOwo+Pj4+PiBAQCAtMTM4OSw4ICsxMzkyLDEyIEBAIHN0YXRpYyB2b2lkIGR3 X2hkbWlfYnJpZGdlX21vZGVfc2V0KHN0cnVjdCBkcm1fYnJpZGdlICpicmlkZ2UsCj4+Pj4+ICAg ewo+Pj4+PiAgIAlzdHJ1Y3QgZHdfaGRtaSAqaGRtaSA9IGJyaWRnZS0+ZHJpdmVyX3ByaXZhdGU7 Cj4+Pj4+ICsJbXV0ZXhfbG9jaygmaGRtaS0+bXV0ZXgpOwo+Pj4+PiArCj4+Pj4+ICAgCS8qIFN0 b3JlIHRoZSBkaXNwbGF5IG1vZGUgZm9yIHBsdWdpbi9ES01TIHBvd2Vyb24gZXZlbnRzICovCj4+ Pj4+ICAgCW1lbWNweSgmaGRtaS0+cHJldmlvdXNfbW9kZSwgbW9kZSwgc2l6ZW9mKGhkbWktPnBy ZXZpb3VzX21vZGUpKTsKPj4+Pj4gKwo+Pj4+PiArCW11dGV4X3VubG9jaygmaGRtaS0+bXV0ZXgp Owo+Pj4+PiAgIH0KPj4+Pj4gICBzdGF0aWMgYm9vbCBkd19oZG1pX2JyaWRnZV9tb2RlX2ZpeHVw KHN0cnVjdCBkcm1fYnJpZGdlICpicmlkZ2UsCj4+Pj4+IEBAIC0xNDA0LDE0ICsxNDExLDIwIEBA IHN0YXRpYyB2b2lkIGR3X2hkbWlfYnJpZGdlX2Rpc2FibGUoc3RydWN0IGRybV9icmlkZ2UgKmJy aWRnZSkKPj4+Pj4gICB7Cj4+Pj4+ICAgCXN0cnVjdCBkd19oZG1pICpoZG1pID0gYnJpZGdlLT5k cml2ZXJfcHJpdmF0ZTsKPj4+Pj4gKwltdXRleF9sb2NrKCZoZG1pLT5tdXRleCk7Cj4+Pj4+ICsJ aGRtaS0+ZGlzYWJsZWQgPSB0cnVlOwo+Pj4+PiAgIAlkd19oZG1pX3Bvd2Vyb2ZmKGhkbWkpOwo+ Pj4+PiArCW11dGV4X3VubG9jaygmaGRtaS0+bXV0ZXgpOwo+Pj4+PiAgIH0KPj4+Pj4gICBzdGF0 aWMgdm9pZCBkd19oZG1pX2JyaWRnZV9lbmFibGUoc3RydWN0IGRybV9icmlkZ2UgKmJyaWRnZSkK Pj4+Pj4gICB7Cj4+Pj4+ICAgCXN0cnVjdCBkd19oZG1pICpoZG1pID0gYnJpZGdlLT5kcml2ZXJf cHJpdmF0ZTsKPj4+Pj4gKwltdXRleF9sb2NrKCZoZG1pLT5tdXRleCk7Cj4+Pj4+ICAgCWR3X2hk bWlfcG93ZXJvbihoZG1pKTsKPj4+Pj4gKwloZG1pLT5kaXNhYmxlZCA9IGZhbHNlOwo+Pj4+PiAr CW11dGV4X3VubG9jaygmaGRtaS0+bXV0ZXgpOwo+Pj4+PiAgIH0KPj4+Pj4gICBzdGF0aWMgdm9p ZCBkd19oZG1pX2JyaWRnZV9ub3Aoc3RydWN0IGRybV9icmlkZ2UgKmJyaWRnZSkKPj4+Pj4gQEAg LTE1MzQsMjAgKzE1NDcsMjAgQEAgc3RhdGljIGlycXJldHVybl90IGR3X2hkbWlfaXJxKGludCBp cnEsIHZvaWQgKmRldl9pZCkKPj4+Pj4gICAJcGh5X2ludF9wb2wgPSBoZG1pX3JlYWRiKGhkbWks IEhETUlfUEhZX1BPTDApOwo+Pj4+PiAgIAlpZiAoaW50cl9zdGF0ICYgSERNSV9JSF9QSFlfU1RB VDBfSFBEKSB7Cj4+Pj4+ICsJCWhkbWlfbW9kYihoZG1pLCB+cGh5X2ludF9wb2wsIEhETUlfUEhZ X0hQRCwgSERNSV9QSFlfUE9MMCk7Cj4+Pj4+ICsJCW11dGV4X2xvY2soJmhkbWktPm11dGV4KTsK Pj4+Pj4gICAJCWlmIChwaHlfaW50X3BvbCAmIEhETUlfUEhZX0hQRCkgewo+Pj4+PiAgIAkJCWRl dl9kYmcoaGRtaS0+ZGV2LCAiRVZFTlQ9cGx1Z2luXG4iKTsKPj4+Pj4gLQkJCWhkbWlfbW9kYiho ZG1pLCAwLCBIRE1JX1BIWV9IUEQsIEhETUlfUEhZX1BPTDApOwo+Pj4+PiAtCj4+Pj4+IC0JCQlk d19oZG1pX3Bvd2Vyb24oaGRtaSk7Cj4+Pj4+ICsJCQlpZiAoIWhkbWktPmRpc2FibGVkKQo+Pj4+ PiArCQkJCWR3X2hkbWlfcG93ZXJvbihoZG1pKTsKPj4+Pj4gICAJCX0gZWxzZSB7Cj4+Pj4+ICAg CQkJZGV2X2RiZyhoZG1pLT5kZXYsICJFVkVOVD1wbHVnb3V0XG4iKTsKPj4+Pj4gLQkJCWhkbWlf bW9kYihoZG1pLCBIRE1JX1BIWV9IUEQsIEhETUlfUEhZX0hQRCwKPj4+Pj4gLQkJCQkgIEhETUlf UEhZX1BPTDApOwo+Pj4+PiAtCj4+Pj4+IC0JCQlkd19oZG1pX3Bvd2Vyb2ZmKGhkbWkpOwo+Pj4+ PiArCQkJaWYgKCFoZG1pLT5kaXNhYmxlZCkKPj4+Pj4gKwkJCQlkd19oZG1pX3Bvd2Vyb2ZmKGhk bWkpOwo+Pj4+IEp1c3QgbGlrZSBteSByZXBseSBvbiAwOC8xMiwgSSB0aG91Z2h0IHRoaXMgY291 bGQgYmUgcmVtb3ZlZCwgc28KPj4+PiBwb3dlcm9uL3Bvd2Vyb2ZmIHdvdWxkIG9ubHkgYmUgY2Fs bGVkIHdpdGggYnJpZGdlLT5lbmFibGUvCj4+Pj4gYnJpZGdlLT5kaXNhYmxlLCB0aGVtIG1heWJl IG5vIG5lZWQgbXV0ZXggaGVyZS4KPj4+IFRoZSBicmlkZ2UgZW5hYmxlL2Rpc2FibGUgbWV0aG9k cyBkbyBub3QgZ2V0IGNhbGxlZCBvbiBob3RwbHVnIGNoYW5nZXMuCj4+Pgo+Pj4gWyAgICAxLjM2 MzAxMV0gZHdoZG1pLWlteCAxMjAwMDAuaGRtaTogRGV0ZWN0ZWQgSERNSSBjb250cm9sbGVyIDB4 MTM6MHhhOjB4YTA6MHhjMQo+Pj4gWyAgICAxLjM3MTM0MV0gZHdoZG1pLWlteCAxMjAwMDAuaGRt aTogZHdfaGRtaV9pcnEoSTpSWC0tLS0sSFBEIFA6UlgzMjEwLEhQRCBTOlJYLS0tLSxIUEQpCj4+ PiBbICAgIDEuMzgxMzQ1XSBpbXgtZHJtIGRpc3BsYXktc3Vic3lzdGVtOiBib3VuZCAxMjAwMDAu aGRtaSAob3BzIGR3X2hkbWlfaW14X29wcykKPj4+IFsgICAgMS40NDg2OTFdIGR3aGRtaS1pbXgg MTIwMDAwLmhkbWk6IGR3X2hkbWlfYnJpZGdlX2Rpc2FibGUoKQo+Pj4gWyAgICAxLjQ1MDk2M10g ZHdoZG1pLWlteCAxMjAwMDAuaGRtaTogZHdfaGRtaV9icmlkZ2VfZW5hYmxlKCkKPj4+Cj4+PiBh bmQgdGhlbiB1bnBsdWdnaW5nIGFuZCByZS1wbHVnZ2luZyB0aGUgSERNSSBjYWJsZToKPj4+Cj4+ PiBbICAgNjguMzA3NTA1XSBkd2hkbWktaW14IDEyMDAwMC5oZG1pOiBkd19oZG1pX2lycShJOlJY LS0tLSxIUEQgUDpSWDMyMTAsLS0tIFM6UlgtLS0tLC0tLSkKPj4+IFsgICA3My44MTM5NzBdIGR3 aGRtaS1pbXggMTIwMDAwLmhkbWk6IGR3X2hkbWlfaXJxKEk6UlgtLS0tLEhQRCBQOlJYMzIxMCxI UEQgUzpSWC0tLS0sSFBEKQo+Pj4KPj4+IEFzIHlvdSBjYW4gc2VlLCBkdXJpbmcgdGhlIHBlcmlv ZCBvZiBkaXNjb25uZWN0aW9uIGZvciBmaXZlIHNlY29uZHMsCj4+PiBkd19oZG1pX2JyaWRnZV9k aXNhYmxlKCkgd2FzIG5vdCBjYWxsZWQuCj4+Pgo+Pj4gU28sIHdpdGhvdXQgdGhlIGNvZGUgYWJv dmUsIHdlJ2QgYmUgbmVlZGxlc3NseSB3YXN0aW5nIHBvd2VyIHdpdGggdGhlCj4+PiBicmlkZ2Ug ZW5hYmxlZCwgdHJ5aW5nIHRvIGRyaXZlIGEgZGlzY29ubmVjdGVkIGRpc3BsYXkuCj4+IFN0cmFu Z2VseSwgSSBkbyBzZWUgYnJpZGdlIGVuYWJsZS9kaXNhYmxlIGluIG15IHNpZGUsIHBhc3QgdGhl IGxvZyBhbmQKPj4gZHVtcF9zdGFjayBiZWxsb3cuCj4+Cj4+IEFuZCBJIGd1ZXNzIHlvdXIgSERN SSBtYXliZSBub3QgcmVhbGx5IGhvdCBwbHVnZWQsIGNvdWxkIHlvdSBjb25maXJtIHRoYXQKPj4g dGhlICJzdGF0dXMiIG9mIEhETUkgZGlzcGxheSBjYXJkIGhhdmUgYmVlbiBjaGFuZ2VkIGJldHdl ZW4gImNvbm5lY3RlZCIKPj4gYW5kICJkaXNjb25uZWN0ZWQiLgo+IEl0IGRvZXMuCj4KPj4gRG8g c2VlIGJyaWRnZV9kaXNhYmxlIHdoZW4gSSB1bnBsdWdpbmcgdGhlIEhETUkgY2FibGUKPj4gWyAg IDE2LjM1ODcxN10gZHdoZG1pLXJvY2tjaGlwIGZmOTgwMDAwLmhkbWk6IEVWRU5UPXBsdWdvdXQK Pj4gWyAgIDIwLjYxMzIyMV0gWzxjMDEwZTAzMD5dICh1bndpbmRfYmFja3RyYWNlKSBmcm9tIFs8 YzAxMGE0ZTA+XQo+PiAoc2hvd19zdGFjaysweDIwLzB4MjQpCj4+IFsgICAyMC42MzE1NjFdIFs8 YzAxMGE0ZTA+XSAoc2hvd19zdGFjaykgZnJvbSBbPGMwODk2ODI4Pl0KPj4gKGR1bXBfc3RhY2sr MHg3MC8weDhjKQo+PiBbICAgMjAuNjQ5MzM3XSBbPGMwODk2ODI4Pl0gKGR1bXBfc3RhY2spIGZy b20gWzxjMDQxNDAzOD5dCj4+IChkd19oZG1pX2JyaWRnZV9kaXNhYmxlKzB4MWMvMHg4OCkKPj4g WyAgIDIwLjY2ODE3OF0gWzxjMDQxNDAzOD5dIChkd19oZG1pX2JyaWRnZV9kaXNhYmxlKSBmcm9t IFs8YzAzZTM4ODg+XQo+PiAoZHJtX2VuY29kZXJfZGlzYWJsZSsweDM0LzB4NzgpCj4+IFsgICAy MC42ODc4NTddIFs8YzAzZTM4ODg+XSAoZHJtX2VuY29kZXJfZGlzYWJsZSkgZnJvbSBbPGMwM2Uz YjFjPl0KPj4gKF9fZHJtX2hlbHBlcl9kaXNhYmxlX3VudXNlZF9mdW5jdGlvbnMrMHg2OC8weGU0 KQo+PiBbICAgMjAuNzA4OTc1XSBbPGMwM2UzYjFjPl0gKF9fZHJtX2hlbHBlcl9kaXNhYmxlX3Vu dXNlZF9mdW5jdGlvbnMpIGZyb20KPj4gWzxjMDNlNDMyMD5dIChkcm1fY3J0Y19oZWxwZXJfc2V0 X2NvbmZpZysweDEyOC8weDg1YykKPj4gWyAgIDIwLjczMTE4MF0gWzxjMDNlNDMyMD5dIChkcm1f Y3J0Y19oZWxwZXJfc2V0X2NvbmZpZykgZnJvbSBbPGMwM2Y1ZTNjPl0KPj4gKGRybV9tb2RlX3Nl dF9jb25maWdfaW50ZXJuYWwrMHg1OC8weGRjKQo+PiBbICAgMjAuNzUyNTA3XSBbPGMwM2Y1ZTNj Pl0gKGRybV9tb2RlX3NldF9jb25maWdfaW50ZXJuYWwpIGZyb20gWzxjMDQwNWVkMD5dCj4+IChj b21taXRfY3J0Y19zdGF0ZSsweDEyNC8weDFlYykKPj4gWyAgIDIwLjc3MzM0Ml0gWzxjMDQwNWVk MD5dIChjb21taXRfY3J0Y19zdGF0ZSkgZnJvbSBbPGMwNDA1NWQ0Pl0KPj4gKGF0b21pY19jb21t aXQuaXNyYS4zKzB4NWMvMHhjOCkKPj4gWyAgIDIwLjc5MzM5N10gWzxjMDQwNTVkND5dIChhdG9t aWNfY29tbWl0LmlzcmEuMykgZnJvbSBbPGMwNDA1NjVjPl0KPj4gKGRybV9hdG9taWNfY29tbWl0 KzB4MWMvMHgyMCkKPj4gWyAgIDIwLjgxMzQ2N10gWzxjMDQwNTY1Yz5dIChkcm1fYXRvbWljX2Nv bW1pdCkgZnJvbSBbPGMwM2ZhNDgwPl0KPj4gKGRybV9tb2RlX3NldGNydGMrMHgzMjQvMHgzZTQp Cj4+IFsgICAyMC44MzMzNzldIFs8YzAzZmE0ODA+XSAoZHJtX21vZGVfc2V0Y3J0YykgZnJvbSBb PGMwM2ViMzIwPl0KPj4gKGRybV9pb2N0bCsweDMwNC8weDQ3OCkKPj4gWyAgIDIwLjg1MjU1N10g WzxjMDNlYjMyMD5dIChkcm1faW9jdGwpIGZyb20gWzxjMDIxZjAyND5dCj4+IChkb192ZnNfaW9j dGwrMHg0OTQvMHg1YTgpCj4+IFsgICAyMC44NzEzNzddIFs8YzAyMWYwMjQ+XSAoZG9fdmZzX2lv Y3RsKSBmcm9tIFs8YzAyMWYxOTQ+XQo+PiAoU3lTX2lvY3RsKzB4NWMvMHg4NCkKPj4gWyAgIDIw Ljg5MDAzOF0gWzxjMDIxZjE5ND5dIChTeVNfaW9jdGwpIGZyb20gWzxjMDEwNjQ2Yz5dCj4+IChf X3N5c190cmFjZV9yZXR1cm4rMHgwLzB4MTQpCj4gWW91ciB1c2Vyc3BhY2UgaXMgaXNzdWluZyBh biBpb2N0bCB0byBkaXNhYmxlIHRoZSBvdXRwdXQuICBJIGd1ZXNzIHlvdQo+IGhhdmUgb3RoZXIg YWN0aXZlIG91dHB1dHMgYmVzaWRlcyBIRE1JLgo+Cj4KClllYWgsIEkgZG8gaGF2ZSBhbm90aGVy IGFjdGl2ZSBlRFAgc2NyZWVuLCBidXQgYWZ0ZXIgcmVtb3ZlZCB0aGUgZURQCmRpc3BsYXkgY2Fy ZCwgSSBzdGlsbCBzZWUgdGhlIGJyaWRnZSBlbmFibGUvZGlzYWJsZWQgaGF2ZSBiZWVuIGNhbGxl ZC4KCkkgdHJ5IHRvIHRyYWNrIHRoZSBzb21lIHVzZXJzcGFjZSBjb2RlLCBidXQgZHVlIHRvIGxp dHRsZSBrbm93bGVkZ2UgYWJvdXQKQ2hvbWVPUyBjb2RlLCBzdGlsbCBjYW4ndCBmb3VuZCBzb21l dGhpbmcgZGlyZWN0bHkuIEFzIGRybSBmcmFtZXdvcmsKd29uJ3QgbWFrZSBicmlkZ2UgZGlzYWJs ZWQgd2hlbiBjb25uZWN0b3IgcGx1ZyBvdXQsIHNvIGZlZWwgZnJlZSB0byBhZ3JlZQp0aGlzIGlz bid0IGR1cGxpY2F0ZSB3b3JrLgoKVGhhbmtzLAotIFlha2lyCgpfX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1k ZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9t YWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: ykk@rock-chips.com (Yakir Yang) Date: Thu, 08 Oct 2015 16:35:22 +0800 Subject: [PATCH 10/12] drm: bridge/dw_hdmi: fix phy enable/disable handling In-Reply-To: <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> <20151007191731.GA32064@n2100.arm.linux.org.uk> Message-ID: <56162ACA.3020501@rock-chips.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org Oh, I haven't noticed that those patches already have been merged into linux-next :-) On 10/08/2015 03:17 AM, Russell King - ARM Linux wrote: > 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. > > Yeah, I do have another active eDP screen, but after removed the eDP display card, I still see the bridge enable/disabled have been called. I try to track the some userspace code, but due to little knowledge about ChomeOS code, still can't found something directly. As drm framework won't make bridge disabled when connector plug out, so feel free to agree this isn't duplicate work. Thanks, - Yakir