From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Remove redundant reprobe in i915_drm_resume Date: Thu, 03 Nov 2016 12:11:09 -0400 Message-ID: <1478189469.28703.8.camel@redhat.com> References: <1478187758-32740-1-git-send-email-lyude@redhat.com> <1478187758-32740-2-git-send-email-lyude@redhat.com> <20161103160253.GA24715@nuc-i3427.alporthouse.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20161103160253.GA24715@nuc-i3427.alporthouse.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Chris Wilson , Lyude Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCAyMDE2LTExLTAzIGF0IDE2OjAyICswMDAwLCBDaHJpcyBXaWxzb24gd3JvdGU6Cj4g T24gVGh1LCBOb3YgMDMsIDIwMTYgYXQgMTE6NDI6MzdBTSAtMDQwMCwgTHl1ZGUgd3JvdGU6Cj4g PiAKPiA+IFdlaW5lJ3MgaW52ZXN0aWdhdGlvbiBvbiBiZW5jaG1hcmtpbmcgdGhlIHN1c3BlbmQv cmVzdW1lIHByb2Nlc3MgcG9pbnRlZAo+ID4gb3V0IGEgbG90IG9mIHRoZSB0aW1lIGluIHN1c3Bl bmQvcmVzdW1lIGlzIGJlaW5nIHNwZW50IHJlcHJvYmluZy4gV2hpbGUKPiA+IHRoZSByZXByb2Jp bmcgcHJvY2VzcyBpcyBhIGxlbmd0aHkgb25lIGZvciBnb29kIHJlYXNvbiwgd2UgZG9uJ3QgbmVl ZCB0bwo+ID4gaG9sZCB1cCB0aGUgZW50aXJlIHN1c3BlbmQvcmVzdW1lIHByb2Nlc3Mgd2hpbGUg d2Ugd2FpdCBmb3IgaXQgdG8KPiA+IGZpbmlzaC4gTHVja2lseSBhcyBpdCB0dXJucyBvdXQsIHdl IGFscmVhZHkgdHJpZ2dlciBhIGZ1bGwgY29ubmVjdG9yCj4gPiByZXByb2JlIGluIGk5MTVfaHBk X3BvbGxfaW5pdF93b3JrKCksIHNvIHdlIGNhbiBqdXN0IGRpdGNoIHJlcHJvYmluZyBpbgo+ID4g aTkxNV9kcm1fcmVzdW1lKCkgZW50aXJlbHkuCj4gPiAKPiA+IFRoaXMgd29uJ3QgbGVhZCB0byBs ZXNzIHRpbWUgc3BlbnQgcmVzdW1pbmcganVzdCB5ZXQgc2luY2Ugbm93IHRoZQo+ID4gYm90dGxl bmVjayB3aWxsIGJlIHdhaXRpbmcgZm9yIHRoZSBtb2RlX2NvbmZpZyBsb2NrIGluCj4gPiBkcm1f a21zX2hlbHBlcl9wb2xsX2VuYWJsZSgpLCBzaW5jZSB0aGF0IHdpbGwgYmUgaGVsZCBhcyBsb25n IGFzCj4gPiBpOTE1X2hwZF9wb2xsX2luaXRfd29yaygpIGlzIHJlcHJvYmluZyBhbGwgb2YgdGhl IGNvbm5lY3RvcnMuIEJ1dCB3ZSdsbAo+ID4gYWRkcmVzcyB0aGF0IGluIHRoZSBuZXh0IHBhdGNo Lgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBMeXVkZSA8bHl1ZGVAcmVkaGF0LmNvbT4KPiA+IENj OiBEYXZpZCBXZWluZWhhbGwgPGRhdmlkLndlaW5laGFsbEBsaW51eC5pbnRlbC5jb20+Cj4gPiAt LS0KPiA+IMKgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYyB8IDIgLS0KPiA+IMKgMSBm aWxlIGNoYW5nZWQsIDIgZGVsZXRpb25zKC0pCj4gPiAKPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJz L2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCj4gPiBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVf ZHJ2LmMKPiA+IGluZGV4IGJmYjJlZmQuLjUzMmNjMGYgMTAwNjQ0Cj4gPiAtLS0gYS9kcml2ZXJz L2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCj4gPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9p OTE1X2Rydi5jCj4gPiBAQCAtMTYwMiw4ICsxNjAyLDYgQEAgc3RhdGljIGludCBpOTE1X2RybV9y ZXN1bWUoc3RydWN0IGRybV9kZXZpY2UgKmRldikKPiA+IMKgCcKgKiBub3RpZmljYXRpb25zLgo+ ID4gwqAJwqAqICovCj4gPiDCoAlpbnRlbF9ocGRfaW5pdChkZXZfcHJpdik7Cj4gPiAtCS8qIENv bmZpZyBtYXkgaGF2ZSBjaGFuZ2VkIGJldHdlZW4gc3VzcGVuZCBhbmQgcmVzdW1lICovCj4gPiAt CWRybV9oZWxwZXJfaHBkX2lycV9ldmVudChkZXYpOwo+IAo+IFRoZSBjb21tZW50IGlzIHN0aWxs IGFwdC4gVGhpcyBjb2RlIGlzIGtub3duIHRvIGJlIGJyb2tlbiBzaW5jZSBpdAo+IGRvZXNuJ3Qg ZGV0ZWN0IGEgY2hhbmdlIGluIG1vbml0b3JzIChlLmcuIGEgY2hhbmdlIGluIGV4dGVybmFsIGNv bm5lY3RvcnMKPiBmcm9tIGRvY2tpbmcpIGJldHdlZW4gc3VzcGVuZCBhbmQgcmVzZW5kLiBXZSBz dGlsbCBoYXZlIHRvIHNlbmQgdGhlIHVldmVudC4KPiAKPiArCWRybV9rbXNfaGVscGVyX2hvdHBs dWdfZXZlbnQoZGV2KTsKCkkgbWlnaHQgbm90IGhhdmUgZXhwbGFpbmVkIG15c2VsZiB2ZXJ5IHdl bGwuIFRoZSB3YXkgdGhpbmdzIHNob3VsZCBsb29rIHdpdGgKdGhpcyBwYXRjaCBpcyBsaWtlIHRo aXM6CgppOTE1X2RybV9yZXN1bWUoKQrCoC0+IGludGVsX2hwZF9pbml0KCkKwqAgwqAtPiBzZXRz IGRldl9wcml2LT5ob3RwbHVnLnBvbGxfZW5hYmxlZCB0byB0cnVlCsKgIMKgLT4gc2NoZWR1bGVz IGRldl9wcml2LT5ob3RwbHVnLnBvbGxfaW5pdF93b3JrCsKgLT4gY29udGludWUgcmVzdW1l4oCm CgphdCB0aGUgc2FtZSB0aW1lOgoKaTkxNV9ocGRfcG9sbF9pbml0X3dvcmsoKSBnZXRzIHNjaGVk dWxlZCBhbmQgc3RhcnRzCsKgLT4gc2luY2UgZGV2X3ByaXYtPmhvdHBsdWcucG9sbF9lbmFibGVk ID09IGZhbHNlLCBkcm1faGVscGVyX2hwZF9pcnFfZXZlbnQoKSBpcyBjYWxsZWQKwqAgLT4gZHJt X2hlbHBlcl9ocGRfaXJxX2V2ZW50KCkgcmVwcm9iZXMgY29ubmVjdG9ycwogICAtPiBpZiBhbnl0 aGluZyBjaGFuZ2VkLCBkcm1fa21zX2hlbHBlcl9ob3RwbHVnX2V2ZW50KCkgZ2V0cyBjYWxsZWQu CgpTbyB3ZSdyZSBzdGlsbCBwb2xsaW5nIHRoZSBjb25uZWN0b3JzIHdoZW4gY29taW5nIG91dCBv ZiByZXN1bWUganVzdCBsaWtlCmJlZm9yZSwgZXhjZXB0IG5vdyB3ZSdyZSBkb2luZyBpdCB3aXRo b3V0IG5lZWRsZXNzbHkgbWFraW5nIHRoZSB3aG9sZSByZXN1bWUKcHJvY2VzcyBzdGFsbCB1bnRp bCB3ZSdyZSBkb25lLiBXZSdyZSBhbHNvIG5vIGxvbmdlciByZXByb2JpbmcgZGlzcGxheQpjb25u ZWN0b3JzIHR3aWNl4oCmCgo+IAo+IHdoaWNoIGFsc28gZGVwZW5kcyB1cG9uIHVzIGFjdHVhbGx5 IHJlc2V0aW5nIHRoZSBjb25uZWN0b3ItPnN0YXR1cyB0bwo+IHVua25vd24gaW4gZHJtX21vZGVf Y29uZmlnX3Jlc2V0KCksIERhbmllbCEKPiAtQ2hyaXMKPiAKX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2 ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21h aWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1758595AbcKCQLL (ORCPT ); Thu, 3 Nov 2016 12:11:11 -0400 Received: from mx1.redhat.com ([209.132.183.28]:37562 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1758470AbcKCQLK (ORCPT ); Thu, 3 Nov 2016 12:11:10 -0400 Message-ID: <1478189469.28703.8.camel@redhat.com> Subject: Re: [Intel-gfx] [PATCH 1/2] drm/i915: Remove redundant reprobe in i915_drm_resume From: Lyude Paul To: Chris Wilson , Lyude Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Date: Thu, 03 Nov 2016 12:11:09 -0400 In-Reply-To: <20161103160253.GA24715@nuc-i3427.alporthouse.com> References: <1478187758-32740-1-git-send-email-lyude@redhat.com> <1478187758-32740-2-git-send-email-lyude@redhat.com> <20161103160253.GA24715@nuc-i3427.alporthouse.com> Organization: Red Hat Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.28]); Thu, 03 Nov 2016 16:11:10 +0000 (UTC) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, 2016-11-03 at 16:02 +0000, Chris Wilson wrote: > On Thu, Nov 03, 2016 at 11:42:37AM -0400, Lyude wrote: > > > > Weine's investigation on benchmarking the suspend/resume process pointed > > out a lot of the time in suspend/resume is being spent reprobing. While > > the reprobing process is a lengthy one for good reason, we don't need to > > hold up the entire suspend/resume process while we wait for it to > > finish. Luckily as it turns out, we already trigger a full connector > > reprobe in i915_hpd_poll_init_work(), so we can just ditch reprobing in > > i915_drm_resume() entirely. > > > > This won't lead to less time spent resuming just yet since now the > > bottleneck will be waiting for the mode_config lock in > > drm_kms_helper_poll_enable(), since that will be held as long as > > i915_hpd_poll_init_work() is reprobing all of the connectors. But we'll > > address that in the next patch. > > > > Signed-off-by: Lyude > > Cc: David Weinehall > > --- > >  drivers/gpu/drm/i915/i915_drv.c | 2 -- > >  1 file changed, 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index bfb2efd..532cc0f 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -1602,8 +1602,6 @@ static int i915_drm_resume(struct drm_device *dev) > >    * notifications. > >    * */ > >   intel_hpd_init(dev_priv); > > - /* Config may have changed between suspend and resume */ > > - drm_helper_hpd_irq_event(dev); > > The comment is still apt. This code is known to be broken since it > doesn't detect a change in monitors (e.g. a change in external connectors > from docking) between suspend and resend. We still have to send the uevent. > > + drm_kms_helper_hotplug_event(dev); I might not have explained myself very well. The way things should look with this patch is like this: i915_drm_resume()  -> intel_hpd_init()    -> sets dev_priv->hotplug.poll_enabled to true    -> schedules dev_priv->hotplug.poll_init_work  -> continue resume… at the same time: i915_hpd_poll_init_work() gets scheduled and starts  -> since dev_priv->hotplug.poll_enabled == false, drm_helper_hpd_irq_event() is called   -> drm_helper_hpd_irq_event() reprobes connectors -> if anything changed, drm_kms_helper_hotplug_event() gets called. So we're still polling the connectors when coming out of resume just like before, except now we're doing it without needlessly making the whole resume process stall until we're done. We're also no longer reprobing display connectors twice… > > which also depends upon us actually reseting the connector->status to > unknown in drm_mode_config_reset(), Daniel! > -Chris >