From mboxrd@z Thu Jan 1 00:00:00 1970 From: Laurent Pinchart Subject: Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on Date: Tue, 22 Nov 2016 20:23:38 +0200 Message-ID: <3450618.hbnl5lRf5h@avalon> References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from galahad.ideasonboard.com (galahad.ideasonboard.com [185.26.127.97]) by gabe.freedesktop.org (Postfix) with ESMTPS id 68C2F6E426 for ; Tue, 22 Nov 2016 18:23:21 +0000 (UTC) In-Reply-To: List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: John Stultz Cc: Daniel Vetter , lkml , "dri-devel@lists.freedesktop.org" , Wolfram Sang List-Id: dri-devel@lists.freedesktop.org SGkgSm9obiwKCihDQydpbmcgRGFuaWVsKQoKT24gVHVlc2RheSAyMiBOb3YgMjAxNiAxMDowNzo1 MyBKb2huIFN0dWx0eiB3cm90ZToKPiBPbiBUdWUsIE5vdiAyMiwgMjAxNiBhdCA5OjM4IEFNLCBK b2huIFN0dWx0eiA8am9obi5zdHVsdHpAbGluYXJvLm9yZz4gd3JvdGU6Cj4gPiBJbnRlcmVzdGlu Z2x5LCB3aXRob3V0IHRoZSBtc2xlZXAgYWRkZWQgaW4gdGhpcyBwYXRjaCwgcmVtb3ZpbmcgdGhl Cj4gPiB3YWl0X2V2ZW50X2ludGVycnVwdGlibGVfdGltZW91dCgpIG1ldGhvZCBpbiBhZHY3NTEx X3dhaXRfZm9yX2VkaWQoKQo+ID4gYW5kIHVzaW5nIHRoZSBwb2xsaW5nIGxvb3Agc2VlbXMgdG8g bWFrZSB0aGluZ3MganVzdCBhcyByZWxpYWJsZS4gU28KPiA+IG1heWJlIHNvbWV0aGluZyBpcyBv ZmYgd2l0aCB0aGUgaXJxIGhhbmRsaW5nIGhlcmUgaW5zdGVhZD8KPiAKPiBBaGhoaC4uIFNvIEkg dGhpbmsgdGhlIHRyb3VibGUgaGVyZSBpcyB0aGUgdGhhdCB3aGVuIHdlIGZhaWwgd2FpdGluZwo+ IGZvciB0aGUgaXJxLCB0aGUgYmFja3RyYWNlIGlzIGFzIGZvbGxvd3M6Cj4gCj4gWyAgICA4LjMx ODY1NF0gWzxmZmZmZmY4MDA4MDg3YzI4Pl0gZHVtcF9iYWNrdHJhY2UrMHgwLzB4MWEwCj4gWyAg ICA4LjMxODY2MV0gWzxmZmZmZmY4MDA4MDg3ZGRjPl0gc2hvd19zdGFjaysweDE0LzB4MjAKPiBb ICAgIDguMzE4NjcxXSBbPGZmZmZmZjgwMDg0MzQ0ZjA+XSBkdW1wX3N0YWNrKzB4OTAvMHhiMAo+ IFsgICAgOC4zMTg2ODBdIFs8ZmZmZmZmODAwODUzNDY1MD5dIGFkdjc1MTFfZ2V0X2VkaWRfYmxv Y2srMHgyYzgvMHgzMjAKPiBbICAgIDguMzE4Njg3XSBbPGZmZmZmZjgwMDg1MjE0YTg+XSBkcm1f ZG9fZ2V0X2VkaWQrMHg3OC8weDI4MAo+IFsgICAgOC4zMTg2OTNdIFs8ZmZmZmZmODAwODUzNDcy OD5dIGFkdjc1MTFfZ2V0X21vZGVzKzB4ODAvMHhkOAo+IFsgICAgOC4zMTg3MDBdIFs8ZmZmZmZm ODAwODUzNDc5ND5dIGFkdjc1MTFfY29ubmVjdG9yX2dldF9tb2RlcysweDE0LzB4MjAKPiBbICAg IDguMzE4NzEwXSBbPGZmZmZmZjgwMDg1MDBhNTQ+XQo+IGRybV9oZWxwZXJfcHJvYmVfc2luZ2xl X2Nvbm5lY3Rvcl9tb2RlcysweDJiYy8weDUwMAo+IFsgICAgOC4zMTg3MThdIFs8ZmZmZmZmODAw ODUwZTQwMD5dIGRybV9mYl9oZWxwZXJfaG90cGx1Z19ldmVudCsweDEzMC8weDE4OAo+IFsgICAg OC4zMTg3MjZdIFs8ZmZmZmZmODAwODUwZWU2OD5dIGRybV9mYmRldl9jbWFfaG90cGx1Z19ldmVu dCsweDEwLzB4MjAKPiBbICAgIDguMzE4NzMzXSBbPGZmZmZmZjgwMDg1MzU3MTg+XQo+IGtpcmlu X2ZiZGV2X291dHB1dF9wb2xsX2NoYW5nZWQrMHgyMC8weDU4Cj4gWyAgICA4LjMxODc0MF0gWzxm ZmZmZmY4MDA4NTAwY2MwPl0gZHJtX2ttc19oZWxwZXJfaG90cGx1Z19ldmVudCsweDI4LzB4MzgK PiBbICAgIDguMzE4NzQ4XSBbPGZmZmZmZjgwMDg1MDEwZDg+XSBkcm1faGVscGVyX2hwZF9pcnFf ZXZlbnQrMHgxMzgvMHgxODAKPiBbICAgIDguMzE4NzU0XSBbPGZmZmZmZjgwMDg1MzM4NTA+XSBh ZHY3NTExX2lycV9wcm9jZXNzKzB4NzgvMHhkOAo+IFsgICAgOC4zMTg3NjFdIFs8ZmZmZmZmODAw ODUzMzhjND5dIGFkdjc1MTFfaXJxX2hhbmRsZXIrMHgxNC8weDI4Cj4gWyAgICA4LjMxODc2OV0g WzxmZmZmZmY4MDA4MTAwMDYwPl0gaXJxX3RocmVhZF9mbisweDI4LzB4NjgKPiBbICAgIDguMzE4 Nzc1XSBbPGZmZmZmZjgwMDgxMDAzNTA+XSBpcnFfdGhyZWFkKzB4MTI4LzB4MWU4Cj4gWyAgICA4 LjMxODc4Ml0gWzxmZmZmZmY4MDA4MGQyZTY4Pl0ga3RocmVhZCsweGQwLzB4ZTgKPiBbICAgIDgu MzE4Nzg4XSBbPGZmZmZmZjgwMDgwODJlODA+XSByZXRfZnJvbV9mb3JrKzB4MTAvMHg1MAo+IAo+ IFNvIHdlJ3JlIGFjdHVhbGx5IGluIGlycSBoYW5kbGluZyB0aGUgaG90cGx1ZyBpbnRlcnJ1cHQs IHdoaWNoIGlzIHdoeQo+IHdlIG5ldmVyIGdldCB0aGUgaXJxIG5vdGlmaWNhdGlvbiB3aGVuIHRo ZSBlZGlkIGlzIHJlYWQuCj4gCj4gSSBzdXNwZWN0IHdlIG5lZWQgdG8gdXNlIGEgd29ya3F1ZXVl IHRvIGRvIHRoZSBob3RwbHVnIGhhbmRsaW5nIG91dCBvZiBpcnEuCgpMb3ZlbHkgOi0pCgpRdW90 aW5nIHRoZSBEUk0gZG9jdW1lbnRhdGlvbjoKCi8qKgogKiBkcm1faGVscGVyX2hwZF9pcnFfZXZl bnQgLSBob3RwbHVnIHByb2Nlc3NpbmcKICogQGRldjogZHJtX2RldmljZQogKgogKiBEcml2ZXJz IGNhbiB1c2UgdGhpcyBoZWxwZXIgZnVuY3Rpb24gdG8gcnVuIGEgZGV0ZWN0IGN5Y2xlIG9uIGFs bCAKY29ubmVjdG9ycwogKiB3aGljaCBoYXZlIHRoZSBEUk1fQ09OTkVDVE9SX1BPTExfSFBEIGZs YWcgc2V0IGluIHRoZWlyICZwb2xsZWQgbWVtYmVyLiBBbGwKICogb3RoZXIgY29ubmVjdG9ycyBh cmUgaWdub3JlZCwgd2hpY2ggaXMgdXNlZnVsIHRvIGF2b2lkIHJlcHJvYmluZyBmaXhlZAogKiBw YW5lbHMuCiAqCiAqIFRoaXMgaGVscGVyIGZ1bmN0aW9uIGlzIHVzZWZ1bCBmb3IgZHJpdmVycyB3 aGljaCBjYW4ndCBvciBkb24ndCB0cmFjayAKaG90cGx1ZwogKiBpbnRlcnJ1cHRzIGZvciBlYWNo IGNvbm5lY3Rvci4KICoKICogRHJpdmVycyB3aGljaCBzdXBwb3J0IGhvdHBsdWcgaW50ZXJydXB0 cyBmb3IgZWFjaCBjb25uZWN0b3IgaW5kaXZpZHVhbGx5IAphbmQKICogd2hpY2ggaGF2ZSBhIG1v cmUgZmluZS1ncmFpbmVkIGRldGVjdCBsb2dpYyBzaG91bGQgYnlwYXNzIHRoaXMgY29kZSBhbmQK ICogZGlyZWN0bHkgY2FsbCBkcm1fa21zX2hlbHBlcl9ob3RwbHVnX2V2ZW50KCkgaW4gY2FzZSB0 aGUgY29ubmVjdG9yIHN0YXRlCiAqIGNoYW5nZWQuCiAqCiAqIFRoaXMgZnVuY3Rpb24gbXVzdCBi ZSBjYWxsZWQgZnJvbSBwcm9jZXNzIGNvbnRleHQgd2l0aCBubyBtb2RlCiAqIHNldHRpbmcgbG9j a3MgaGVsZC4KICoKICogTm90ZSB0aGF0IGEgY29ubmVjdG9yIGNhbiBiZSBib3RoIHBvbGxlZCBh bmQgcHJvYmVkIGZyb20gdGhlIGhvdHBsdWcgCmhhbmRsZXIsCiAqIGluIGNhc2UgdGhlIGhvdHBs dWcgaW50ZXJydXB0IGlzIGtub3duIHRvIGJlIHVucmVsaWFibGUuCiAqLwoKU28gaXQgbG9va3Mg bGlrZSB3ZSBzaG91bGQgdXNlIGRybV9rbXNfaGVscGVyX2hvdHBsdWdfZXZlbnQoKSBpbnN0ZWFk LgoKLyoqCiAqIGRybV9rbXNfaGVscGVyX2hvdHBsdWdfZXZlbnQgLSBmaXJlIG9mZiBLTVMgaG90 cGx1ZyBldmVudHMKICogQGRldjogZHJtX2RldmljZSB3aG9zZSBjb25uZWN0b3Igc3RhdGUgY2hh bmdlZAogKgogKiBUaGlzIGZ1bmN0aW9uIGZpcmVzIG9mZiB0aGUgdWV2ZW50IGZvciB1c2Vyc3Bh Y2UgYW5kIGFsc28gY2FsbHMgdGhlCiAqIG91dHB1dF9wb2xsX2NoYW5nZWQgZnVuY3Rpb24sIHdo aWNoIGlzIG1vc3QgY29tbW9ubHkgdXNlZCB0byBpbmZvcm0gdGhlIApmYmRldgogKiBlbXVsYXRp b24gY29kZSBhbmQgYWxsb3cgaXQgdG8gdXBkYXRlIHRoZSBmYmNvbiBvdXRwdXQgY29uZmlndXJh dGlvbi4KICoKICogRHJpdmVycyBzaG91bGQgY2FsbCB0aGlzIGZyb20gdGhlaXIgaG90cGx1ZyBo YW5kbGluZyBjb2RlIHdoZW4gYSBjaGFuZ2UgaXMKICogZGV0ZWN0ZWQuIE5vdGUgdGhhdCB0aGlz IGZ1bmN0aW9uIGRvZXMgbm90IGRvIGFueSBvdXRwdXQgZGV0ZWN0aW9uIG9mIGl0cwogKiBvd24s IGxpa2UgZHJtX2hlbHBlcl9ocGRfaXJxX2V2ZW50KCkgZG9lcyAtIHRoaXMgaXMgYXNzdW1lZCB0 byBiZSBkb25lIGJ5IAp0aGUKICogZHJpdmVyIGFscmVhZHkuCiAqCiAqIFRoaXMgZnVuY3Rpb24g bXVzdCBiZSBjYWxsZWQgZnJvbSBwcm9jZXNzIGNvbnRleHQgd2l0aCBubyBtb2RlCiAqIHNldHRp bmcgbG9ja3MgaGVsZC4KICovCgpUaGUgZnVuY3Rpb24gc3VmZmVycyBmcm9tIHRoZSBzYW1lIHBy b2JsZW0gdGhvdWdoLCB0aGF0IGl0IG11c3QgYmUgY2FsbGVkIGZyb20gCnByb2Nlc3MgY29udGV4 dC4KCkRhbmllbCwgd2h5IGRvIHdlIGhhdmUgYW4gQVBJIHRoZSBpcyBjbGVhcmx5IHJlbGF0ZWQg dG8gaW50ZXJydXB0IGhhbmRsaW5nIGJ1dCAKcmVxdWlyZXMgdGhlIGNhbGxlciB0byBpbXBsZW1l bnQgYSB3b3JrcXVldWUgPwoKLS0gClJlZ2FyZHMsCgpMYXVyZW50IFBpbmNoYXJ0CgpfX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGlu ZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVl ZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S934371AbcKVSXX (ORCPT ); Tue, 22 Nov 2016 13:23:23 -0500 Received: from galahad.ideasonboard.com ([185.26.127.97]:48077 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S934345AbcKVSXW (ORCPT ); Tue, 22 Nov 2016 13:23:22 -0500 From: Laurent Pinchart To: John Stultz Cc: lkml , David Airlie , Archit Taneja , Wolfram Sang , Lars-Peter Clausen , "dri-devel@lists.freedesktop.org" , Daniel Vetter Subject: Re: [RFC][PATCH 2/3] drm/bridge: adv7511: Add 200ms delay on power-on Date: Tue, 22 Nov 2016 20:23:38 +0200 Message-ID: <3450618.hbnl5lRf5h@avalon> User-Agent: KMail/4.14.10 (Linux/4.8.6-gentoo; KDE/4.14.24; x86_64; ; ) In-Reply-To: References: <1479775052-28194-1-git-send-email-john.stultz@linaro.org> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Hi John, (CC'ing Daniel) On Tuesday 22 Nov 2016 10:07:53 John Stultz wrote: > On Tue, Nov 22, 2016 at 9:38 AM, John Stultz wrote: > > Interestingly, without the msleep added in this patch, removing the > > wait_event_interruptible_timeout() method in adv7511_wait_for_edid() > > and using the polling loop seems to make things just as reliable. So > > maybe something is off with the irq handling here instead? > > Ahhhh.. So I think the trouble here is the that when we fail waiting > for the irq, the backtrace is as follows: > > [ 8.318654] [] dump_backtrace+0x0/0x1a0 > [ 8.318661] [] show_stack+0x14/0x20 > [ 8.318671] [] dump_stack+0x90/0xb0 > [ 8.318680] [] adv7511_get_edid_block+0x2c8/0x320 > [ 8.318687] [] drm_do_get_edid+0x78/0x280 > [ 8.318693] [] adv7511_get_modes+0x80/0xd8 > [ 8.318700] [] adv7511_connector_get_modes+0x14/0x20 > [ 8.318710] [] > drm_helper_probe_single_connector_modes+0x2bc/0x500 > [ 8.318718] [] drm_fb_helper_hotplug_event+0x130/0x188 > [ 8.318726] [] drm_fbdev_cma_hotplug_event+0x10/0x20 > [ 8.318733] [] > kirin_fbdev_output_poll_changed+0x20/0x58 > [ 8.318740] [] drm_kms_helper_hotplug_event+0x28/0x38 > [ 8.318748] [] drm_helper_hpd_irq_event+0x138/0x180 > [ 8.318754] [] adv7511_irq_process+0x78/0xd8 > [ 8.318761] [] adv7511_irq_handler+0x14/0x28 > [ 8.318769] [] irq_thread_fn+0x28/0x68 > [ 8.318775] [] irq_thread+0x128/0x1e8 > [ 8.318782] [] kthread+0xd0/0xe8 > [ 8.318788] [] ret_from_fork+0x10/0x50 > > So we're actually in irq handling the hotplug interrupt, which is why > we never get the irq notification when the edid is read. > > I suspect we need to use a workqueue to do the hotplug handling out of irq. Lovely :-) Quoting the DRM documentation: /** * drm_helper_hpd_irq_event - hotplug processing * @dev: drm_device * * Drivers can use this helper function to run a detect cycle on all connectors * which have the DRM_CONNECTOR_POLL_HPD flag set in their &polled member. All * other connectors are ignored, which is useful to avoid reprobing fixed * panels. * * This helper function is useful for drivers which can't or don't track hotplug * interrupts for each connector. * * Drivers which support hotplug interrupts for each connector individually and * which have a more fine-grained detect logic should bypass this code and * directly call drm_kms_helper_hotplug_event() in case the connector state * changed. * * This function must be called from process context with no mode * setting locks held. * * Note that a connector can be both polled and probed from the hotplug handler, * in case the hotplug interrupt is known to be unreliable. */ So it looks like we should use drm_kms_helper_hotplug_event() instead. /** * drm_kms_helper_hotplug_event - fire off KMS hotplug events * @dev: drm_device whose connector state changed * * This function fires off the uevent for userspace and also calls the * output_poll_changed function, which is most commonly used to inform the fbdev * emulation code and allow it to update the fbcon output configuration. * * Drivers should call this from their hotplug handling code when a change is * detected. Note that this function does not do any output detection of its * own, like drm_helper_hpd_irq_event() does - this is assumed to be done by the * driver already. * * This function must be called from process context with no mode * setting locks held. */ The function suffers from the same problem though, that it must be called from process context. Daniel, why do we have an API the is clearly related to interrupt handling but requires the caller to implement a workqueue ? -- Regards, Laurent Pinchart