From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [PATCH v2] drm/i915: Discard previous atomic state on resume if connectors change Date: Mon, 09 May 2016 11:31:27 -0400 Message-ID: <1462807887.8121.9.camel@redhat.com> References: <1462570796-22500-1-git-send-email-cpaul@redhat.com> <20160509064214.GH27098@phenom.ffwll.local> <1462802828.8121.8.camel@redhat.com> <20160509145351.GB27098@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160509145351.GB27098@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Daniel Vetter Cc: Daniel Vetter , intel-gfx@lists.freedesktop.org, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., linux-kernel@vger.kernel.org open list" , stable@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org bm8gcHJvYmxlbSwgSSdsbCBsZXQgeW91IGtub3cgd2hlbiB0aGV5J3JlIHJlYWR5CgpPbiBNb24s IDIwMTYtMDUtMDkgYXQgMTY6NTMgKzAyMDAsIERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gT24gTW9u LCBNYXkgMDksIDIwMTYgYXQgMTA6MDc6MDhBTSAtMDQwMCwgTHl1ZGUgUGF1bCB3cm90ZToKPiA+ IAo+ID4gWWVwLCBEYXZlJ3MgcGF0Y2hlcyBmaXggdGhlIGlzc3VlIG9uIHRoZWlyIG93biBzbyB0 aGlzIGlzIG9ubHkgZ29pbmcgdG8gYmUKPiA+IG5lZWRlZCBmb3IgNC42Lgo+IE9rLCBzbyBub3Qg dGhlIGZpcnN0IG9uZSB0aGF0IG9ubHkgbmVlZHMgdG8gYmUgYXBwbGllZCB0byBzdGFibGUga2Vy bmVscy4KPiBGb3IgdGhhdCB0byB3b3JrIHlvdSBuZWVkIHRvIGV4cGxhaW4gdGhhdCB0aGlzIGlz IHRoZSBtb3N0IG1pbmltYWwgZml4IGZvcgo+IHN0YWJlbCBrZXJuZWxzLCBhbmQgYWRkIGEgcmVm ZXJlbmNlIHRvIHRoZSBmaW5hbCBjb21taXQgaW4gdXBzdHJlYW0gdGhhdAo+IGZpeGVzIHRoZSBp c3N1ZSBmb3IgcmVhbC4gT3RoZXJ3aXNlIEdyZWcgS0ggd29uJ3QgdGFrZSBpdC4gSXQgc3RpbGwg bmVlZHMKPiBteSBhY2suCj4gCj4gUHJvYmFibHkgYmVzdCB0byBoYXZlIGFuIGVudGlyZSBkcCBt c3Qgc2VyaWVzIGZvciBhbGwgb2YgdGhvc2UsIGJ1dCBtYXliZQo+IHBpbmcgbWUgZmlyc3Qgb24g aXJjIHNvIEkgY2FuIGNoZWNrIHRoZW0gcXVpY2tseSBiZWZvcmUgeW91IGhpdCBzZW5kLgo+IC1E YW5pZWwKPiAKPiA+IAo+ID4gCj4gPiBPbiBNb24sIDIwMTYtMDUtMDkgYXQgMDg6NDIgKzAyMDAs IERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gPiA+IAo+ID4gPiBPbiBGcmksIE1heSAwNiwgMjAxNiBh dCAwNTozOTo1NlBNIC0wNDAwLCBMeXVkZSB3cm90ZToKPiA+ID4gPiAKPiA+ID4gPiAKPiA+ID4g PiBJZiBhbiBNU1QgZGV2aWNlIGlzIGRpc2Nvbm5lY3RlZCB3aGlsZSB0aGUgbWFjaGluZSBpcyBz dXNwZW5kZWQsIHRoZQo+ID4gPiA+IG51bWJlciBvZiBjb25uZWN0b3JzIHdpbGwgY2hhbmdlIGFz IHdlbGwgYWZ0ZXIgd2UgY2FsbAo+ID4gPiA+IGludGVsX2RwX21zdF9yZXN1bWUoKS4gVGhpcyBt ZWFucyB0aGF0IGFueSBwcmV2aW91cyBhdG9taWMgc3RhdGUgd2UgaGFkCj4gPiA+ID4gYmVmb3Jl IHN1c3BlbmRpbmcgaXMgbm8gbG9uZ2VyIHZhbGlkLCBzaW5jZSBpdCdsbCBzdGlsbCBiZSBwb2lu dGluZyB0bwo+ID4gPiA+IG1pc3NpbmcgY29ubmVjdG9ycy4gV2UgbmVlZCB0byBjaGVjayBmb3Ig dGhpcyBiZWZvcmUgY29tbWl0dGluZyB0aGUKPiA+ID4gPiBzdGF0ZSwgb3RoZXJ3aXNlIHdlJ2xs IGtlcm5lbCBwYW5pYyBvbiByZXN1bWUgd2hlbmV2ZXIgaWYgYW55IE1TVAo+ID4gPiA+IGRpc3Bs YXkgd2FzIGRpc2Nvbm5lY3RlZCBiZWZvcmUgd2Ugc3RhcnRlZCByZXN1bWluZzoKPiA+ID4gPiAK PiA+ID4gPiBCVUc6IHVuYWJsZSB0byBoYW5kbGUga2VybmVsIE5VTEwgcG9pbnRlciBkZXJlZmVy ZW5jZSBhdAo+ID4gPiA+IDAwMDAwMDAwMDAwMDAwMDgKPiA+ID4gPiBJUDogWzxmZmZmZmZmZmEw MTU4OGVmPl0gZHJtX2F0b21pY19oZWxwZXJfY2hlY2tfbW9kZXNldCsweDI5Zi8weGI0MAo+ID4g PiA+IFtkcm1fa21zX2hlbHBlcl0KPiA+ID4gPiBDYWxsIFRyYWNlOgo+ID4gPiA+IMKgWzxmZmZm ZmZmZmEwMjM1NGY0Pl0gaW50ZWxfYXRvbWljX2NoZWNrKzB4MzQvMHgxMTgwIFtpOTE1XQo+ID4g PiA+IMKgWzxmZmZmZmZmZjgxMGU2YzNmPl0gPyBtYXJrX2hlbGRfbG9ja3MrMHg2Zi8weGEwCj4g PiA+ID4gwqBbPGZmZmZmZmZmODEwZTZkOTk+XSA/IHRyYWNlX2hhcmRpcnFzX29uX2NhbGxlcisw eDEyOS8weDFiMAo+ID4gPiA+IMKgWzxmZmZmZmZmZmEwMGZmMWQyPl0gZHJtX2F0b21pY19jaGVj a19vbmx5KzB4MTkyLzB4NjIwIFtkcm1dCj4gPiA+ID4gwqBbPGZmZmZmZmZmODEzZWUwMDE+XSA/ IHBjaV9wbV90aGF3KzB4MjEvMHg5MAo+ID4gPiA+IMKgWzxmZmZmZmZmZmEwMGZmNjc3Pl0gZHJt X2F0b21pY19jb21taXQrMHgxNy8weDYwIFtkcm1dCj4gPiA+ID4gwqBbPGZmZmZmZmZmYTAyM2Uw YWQ+XSBpbnRlbF9kaXNwbGF5X3Jlc3VtZSsweGJkLzB4MTYwIFtpOTE1XQo+ID4gPiA+IMKgWzxm ZmZmZmZmZjgxM2VlMDcwPl0gPyBwY2lfcG1fdGhhdysweDkwLzB4OTAKPiA+ID4gPiDCoFs8ZmZm ZmZmZmZhMDFiNjBkOD5dIGk5MTVfZHJtX3Jlc3VtZSsweGQ4LzB4MTYwIFtpOTE1XQo+ID4gPiA+ IMKgWzxmZmZmZmZmZmEwMWI2MTg1Pl0gaTkxNV9wbV9yZXN1bWUrMHgyNS8weDMwIFtpOTE1XQo+ ID4gPiA+IMKgWzxmZmZmZmZmZjgxM2VlMGQ0Pl0gcGNpX3BtX3Jlc3VtZSsweDY0LzB4YTAKPiA+ ID4gPiDCoFs8ZmZmZmZmZmY4MTRkOWVhMD5dIGRwbV9ydW5fY2FsbGJhY2srMHg5MC8weDE5MAo+ ID4gPiA+IMKgWzxmZmZmZmZmZjgxNGRhNDU1Pl0gZGV2aWNlX3Jlc3VtZSsweGQ1LzB4MWYwCj4g PiA+ID4gwqBbPGZmZmZmZmZmODE0ZGE1OGQ+XSBhc3luY19yZXN1bWUrMHgxZC8weDUwCj4gPiA+ ID4gwqBbPGZmZmZmZmZmODEwYjY3MTg+XSBhc3luY19ydW5fZW50cnlfZm4rMHg0OC8weDE1MAo+ ID4gPiA+IMKgWzxmZmZmZmZmZjgxMGFjYzE5Pl0gcHJvY2Vzc19vbmVfd29yaysweDFlOS8weDVj MAo+ID4gPiA+IMKgWzxmZmZmZmZmZjgxMGFjYjk2Pl0gPyBwcm9jZXNzX29uZV93b3JrKzB4MTY2 LzB4NWMwCj4gPiA+ID4gwqBbPGZmZmZmZmZmODEwYWQwMzg+XSB3b3JrZXJfdGhyZWFkKzB4NDgv MHg0ZTAKPiA+ID4gPiDCoFs8ZmZmZmZmZmY4MTBhY2ZmMD5dID8gcHJvY2Vzc19vbmVfd29yaysw eDVjMC8weDVjMAo+ID4gPiA+IMKgWzxmZmZmZmZmZjgxMGIzNzk0Pl0ga3RocmVhZCsweGU0LzB4 MTAwCj4gPiA+ID4gwqBbPGZmZmZmZmZmODE3NDI2NzI+XSByZXRfZnJvbV9mb3JrKzB4MjIvMHg1 MAo+ID4gPiA+IMKgWzxmZmZmZmZmZjgxMGIzNmIwPl0gPyBrdGhyZWFkX2NyZWF0ZV9vbl9ub2Rl KzB4MjAwLzB4MjAwCj4gPiA+ID4gCj4gPiA+ID4gQ2hhbmdlcyBzaW5jZSB2MToKPiA+ID4gPiDC oCAtIE1vdmUgZHJtX2F0b21pY19zdGF0ZV9mcmVlKCkgY2FsbCBkb3duIHNvIHdlJ3JlIGhvbGRp bmcgdGhlCj4gPiA+ID4gwqDCoMKgwqBhcHByb3ByaWF0ZSBsb2NrcyB3aGVuIGRlc3Ryb3lpbmcg dGhlIGF0b21pYyBzdGF0ZQo+ID4gPiA+IAo+ID4gPiA+IENjOiBzdGFibGVAdmdlci5rZXJuZWwu b3JnCj4gPiA+ID4gU2lnbmVkLW9mZi1ieTogTHl1ZGUgPGNwYXVsQHJlZGhhdC5jb20+Cj4gPiA+ IElzIHRoaXMgc3RpbGwgYW4gaXNzdWUgb24gLW5pZ2h0bHkgd2l0aCB0aGUgY29ubmVjdG9yIHJl ZmNvdW50aW5nIGZpeGVkPwo+ID4gPiAtRGFuaWVsCj4gPiA+IAo+ID4gPiA+IAo+ID4gPiA+IAo+ ID4gPiA+IC0tLQo+ID4gPiA+IMKgZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5j IHwgMTIgKysrKysrKysrKysrCj4gPiA+ID4gwqAxIGZpbGUgY2hhbmdlZCwgMTIgaW5zZXJ0aW9u cygrKQo+ID4gPiA+IAo+ID4gPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9p bnRlbF9kaXNwbGF5LmMKPiA+ID4gPiBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rpc3Bs YXkuYwo+ID4gPiA+IGluZGV4IDBmMjllZjYuLmQ2OGVmYzcgMTAwNjQ0Cj4gPiA+ID4gLS0tIGEv ZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5jCj4gPiA+ID4gKysrIGIvZHJpdmVy cy9ncHUvZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5jCj4gPiA+ID4gQEAgLTE1OTM0LDYgKzE1OTM0 LDE4IEBAIHZvaWQgaW50ZWxfZGlzcGxheV9yZXN1bWUoc3RydWN0IGRybV9kZXZpY2UKPiA+ID4g PiAqZGV2KQo+ID4gPiA+IMKgcmV0cnk6Cj4gPiA+ID4gwqAJcmV0ID0gZHJtX21vZGVzZXRfbG9j a19hbGxfY3R4KGRldiwgJmN0eCk7Cj4gPiA+ID4gwqAKPiA+ID4gPiArCS8qCj4gPiA+ID4gKwnC oCogV2l0aCBNU1QsIHRoZSBudW1iZXIgb2YgY29ubmVjdG9ycyBjYW4gY2hhbmdlIGJldHdlZW4K PiA+ID4gPiBzdXNwZW5kCj4gPiA+ID4gYW5kCj4gPiA+ID4gKwnCoCogcmVzdW1lLCB3aGljaCBt ZWFucyB0aGF0IHRoZSBzdGF0ZSB3ZSB3YW50IHRvIHJlc3RvcmUgbWlnaHQKPiA+ID4gPiBub3cK PiA+ID4gPiBiZQo+ID4gPiA+ICsJwqAqIGltcG9zc2libGUgdG8gdXNlIHNpbmNlIGl0J2xsIGJl IHBvaW50aW5nIHRvIG5vbi1leGlzdGFudAo+ID4gPiA+ICsJwqAqIGNvbm5lY3RvcnMuCj4gPiA+ ID4gKwnCoCovCj4gPiA+ID4gKwlpZiAocmV0ID09IDAgJiYKPiA+ID4gPiArCcKgwqDCoMKgc3Rh dGUtPm51bV9jb25uZWN0b3IgIT0gZGV2LT5tb2RlX2NvbmZpZy5udW1fY29ubmVjdG9yKSB7Cj4g PiA+ID4gKwkJZHJtX2F0b21pY19zdGF0ZV9mcmVlKHN0YXRlKTsKPiA+ID4gPiArCQlzdGF0ZSA9 IE5VTEw7Cj4gPiA+ID4gKwl9Cj4gPiA+ID4gKwo+ID4gPiA+IMKgCWlmIChyZXQgPT0gMCAmJiAh c2V0dXApIHsKPiA+ID4gPiDCoAkJc2V0dXAgPSB0cnVlOwo+ID4gPiA+IMKgCj4gPiA+ID4gLS3C oAo+ID4gPiA+IDIuNS41Cj4gPiA+ID4gCj4gPiA+ID4gX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX18KPiA+ID4gPiBkcmktZGV2ZWwgbWFpbGluZyBsaXN0Cj4g PiA+ID4gZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwo+ID4gPiA+IGh0dHBzOi8vbGlz dHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vZHJpLWRldmVsCj4gPiAtLcKgCj4g PiBDaGVlcnMsCj4gPiAJTHl1ZGUKPiA+IApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fXwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5m cmVlZGVza3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:56491 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750950AbcEIPb3 (ORCPT ); Mon, 9 May 2016 11:31:29 -0400 Message-ID: <1462807887.8121.9.camel@redhat.com> Subject: Re: [PATCH v2] drm/i915: Discard previous atomic state on resume if connectors change From: Lyude Paul To: Daniel Vetter Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., " "linux-kernel@vger.kernel.org open list" , Daniel Vetter Date: Mon, 09 May 2016 11:31:27 -0400 In-Reply-To: <20160509145351.GB27098@phenom.ffwll.local> References: <1462570796-22500-1-git-send-email-cpaul@redhat.com> <20160509064214.GH27098@phenom.ffwll.local> <1462802828.8121.8.camel@redhat.com> <20160509145351.GB27098@phenom.ffwll.local> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: no problem, I'll let you know when they're ready On Mon, 2016-05-09 at 16:53 +0200, Daniel Vetter wrote: > On Mon, May 09, 2016 at 10:07:08AM -0400, Lyude Paul wrote: > > > > Yep, Dave's patches fix the issue on their own so this is only going to be > > needed for 4.6. > Ok, so not the first one that only needs to be applied to stable kernels. > For that to work you need to explain that this is the most minimal fix for > stabel kernels, and add a reference to the final commit in upstream that > fixes the issue for real. Otherwise Greg KH won't take it. It still needs > my ack. > > Probably best to have an entire dp mst series for all of those, but maybe > ping me first on irc so I can check them quickly before you hit send. > -Daniel > > > > > > > On Mon, 2016-05-09 at 08:42 +0200, Daniel Vetter wrote: > > > > > > On Fri, May 06, 2016 at 05:39:56PM -0400, Lyude wrote: > > > > > > > > > > > > If an MST device is disconnected while the machine is suspended, the > > > > number of connectors will change as well after we call > > > > intel_dp_mst_resume(). This means that any previous atomic state we had > > > > before suspending is no longer valid, since it'll still be pointing to > > > > missing connectors. We need to check for this before committing the > > > > state, otherwise we'll kernel panic on resume whenever if any MST > > > > display was disconnected before we started resuming: > > > > > > > > BUG: unable to handle kernel NULL pointer dereference at > > > > 0000000000000008 > > > > IP: [] drm_atomic_helper_check_modeset+0x29f/0xb40 > > > > [drm_kms_helper] > > > > Call Trace: > > > >  [] intel_atomic_check+0x34/0x1180 [i915] > > > >  [] ? mark_held_locks+0x6f/0xa0 > > > >  [] ? trace_hardirqs_on_caller+0x129/0x1b0 > > > >  [] drm_atomic_check_only+0x192/0x620 [drm] > > > >  [] ? pci_pm_thaw+0x21/0x90 > > > >  [] drm_atomic_commit+0x17/0x60 [drm] > > > >  [] intel_display_resume+0xbd/0x160 [i915] > > > >  [] ? pci_pm_thaw+0x90/0x90 > > > >  [] i915_drm_resume+0xd8/0x160 [i915] > > > >  [] i915_pm_resume+0x25/0x30 [i915] > > > >  [] pci_pm_resume+0x64/0xa0 > > > >  [] dpm_run_callback+0x90/0x190 > > > >  [] device_resume+0xd5/0x1f0 > > > >  [] async_resume+0x1d/0x50 > > > >  [] async_run_entry_fn+0x48/0x150 > > > >  [] process_one_work+0x1e9/0x5c0 > > > >  [] ? process_one_work+0x166/0x5c0 > > > >  [] worker_thread+0x48/0x4e0 > > > >  [] ? process_one_work+0x5c0/0x5c0 > > > >  [] kthread+0xe4/0x100 > > > >  [] ret_from_fork+0x22/0x50 > > > >  [] ? kthread_create_on_node+0x200/0x200 > > > > > > > > Changes since v1: > > > >   - Move drm_atomic_state_free() call down so we're holding the > > > >     appropriate locks when destroying the atomic state > > > > > > > > Cc: stable@vger.kernel.org > > > > Signed-off-by: Lyude > > > Is this still an issue on -nightly with the connector refcounting fixed? > > > -Daniel > > > > > > > > > > > > > > > --- > > > >  drivers/gpu/drm/i915/intel_display.c | 12 ++++++++++++ > > > >  1 file changed, 12 insertions(+) > > > > > > > > diff --git a/drivers/gpu/drm/i915/intel_display.c > > > > b/drivers/gpu/drm/i915/intel_display.c > > > > index 0f29ef6..d68efc7 100644 > > > > --- a/drivers/gpu/drm/i915/intel_display.c > > > > +++ b/drivers/gpu/drm/i915/intel_display.c > > > > @@ -15934,6 +15934,18 @@ void intel_display_resume(struct drm_device > > > > *dev) > > > >  retry: > > > >   ret = drm_modeset_lock_all_ctx(dev, &ctx); > > > >   > > > > + /* > > > > +  * With MST, the number of connectors can change between > > > > suspend > > > > and > > > > +  * resume, which means that the state we want to restore might > > > > now > > > > be > > > > +  * impossible to use since it'll be pointing to non-existant > > > > +  * connectors. > > > > +  */ > > > > + if (ret == 0 && > > > > +     state->num_connector != dev->mode_config.num_connector) { > > > > + drm_atomic_state_free(state); > > > > + state = NULL; > > > > + } > > > > + > > > >   if (ret == 0 && !setup) { > > > >   setup = true; > > > >   > > > > --  > > > > 2.5.5 > > > > > > > > _______________________________________________ > > > > dri-devel mailing list > > > > dri-devel@lists.freedesktop.org > > > > https://lists.freedesktop.org/mailman/listinfo/dri-devel > > --  > > Cheers, > > Lyude > >