From mboxrd@z Thu Jan 1 00:00:00 1970 From: Lyude Paul Subject: Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting Date: Thu, 25 Feb 2016 10:09:14 -0500 Message-ID: <1456412954.5486.4.camel@redhat.com> References: <1456265512-15670-1-git-send-email-cpaul@redhat.com> <56CD1660.2030203@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <56CD1660.2030203@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: "Thulasimani, Sivakumar" , intel-gfx@lists.freedesktop.org Cc: Daniel Vetter , "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 VW5mb3J0dW5hdGVseSBNU1QgaXMgYSB3aWxkIGJlYXN0LCBhbmQgZG9lc24ndCB3b3JrIGF0IGFs bCBsaWtlIG90aGVyCmNvbm5lY3RvcnMuIFRoaXMgYmVpbmcgc2FpZCwgcHV0dGluZyBpdCBhYm92 ZSBpbnRlbF9kaXNwbGF5X3Jlc3VtZSgpIHdhcyB0aGUKZmlyc3QgdGhpbmcgSSB0cmllZCBidXQg dGhhdCBkaWRuJ3Qgd29yay4gT3JpZ2luYWxseSBJIGhhZCB0aG91Z2h0IHB1dHRpbmcgaXQKdGhp cyBoaWdoIHVwIHdhcyByZXF1aXJlZCBiZWNhdXNlIEkgaGFkIGFzc3VtZWQgZHJtX21vZGVfY29u ZmlnX3Jlc2V0KCkgd2FzCmRvaW5nIG1vZGVzZXR0aW5nIGFzIHdlbGwgYnV0IGRvaW5nIHNvbWUg aW52ZXN0aWdhdGlvbiwgaXQgZG9lc24ndCBsb29rIGxpa2UKdGhhdCBjYWxsIGFjdHVhbGx5IGRv ZXMgYW55dGhpbmcuIEl0IGxvb2tzIGxpa2UgdGhlIHJlYWwgY3VscHJpdCBoZXJlIGlzCmludGVs X3J1bnRpbWVfcG1fZW5hYmxlX2ludGVycnVwdHMoKSwgc28gbG9uZyBhcyB0aGUgY2FsbCB0bwpp bnRlbF9kcF9tc3RfcmVzdW1lKCkgaXMgYWJvdmUgdGhhdCBldmVyeXRoaW5nIHdvcmtzLiBTbyBu b3cgSSdtIGEgbGl0dGxlIHVuc3VyZQp3aHkgdGhpcyBpcyB3b3JraW5nIGxpa2UgaXQgaXMsIGFs dGhvdWdoIEkgY2FuIGRlZmluaXRlbHkgc2VlIHRoZSBwYXRjaCBmaXhlcwp0aGUgaXNzdWUuIEkn bSBnb2luZyB0byBlZGl0IHRoZSBwYXRjaCB0byByZWZsZWN0IHRoaXMsIGFuZCBzZWUgaWYgSSBj YW4gZ2V0IGFueQptb3JlIGluc2lnaHQgYXMgdG8gd2h5IHRoaXMgZml4ZXMgaXQuCgoKT24gV2Vk LCAyMDE2LTAyLTI0IGF0IDA4OjAzICswNTMwLCBUaHVsYXNpbWFuaSwgU2l2YWt1bWFyIHdyb3Rl Ogo+IAo+IE9uIDIvMjQvMjAxNiAzOjQxIEFNLCBMeXVkZSB3cm90ZToKPiA+IEFzIGl0IHR1cm5z IG91dCwgcmVzdW1pbmcgRFAgTVNUIGlzIHJhY2V5IHNpbmNlIHdlIGRvbid0IG1ha2Ugc3VyZSBN U1QKPiA+IGlzIHJlYWR5IGJlZm9yZSB3ZSBzdGFydCBtb2Rlc2V0dGluZywgaXQganVzdCB1c3Vh bGx5IGhhcHBlbnMgdG8gYmUKPiA+IHJlYWR5IGluIHRpbWUuIFRoaXMgaXNuJ3QgdGhlIGNhc2Ug b24gYWxsIHN5c3RlbXMsIHBhcnRpY3VsYXJseSBhCj4gPiBUaGlua1BhZCBUNTYwIHdpdGggZGlz cGxheXMgY29ubmVjdGVkIHRocm91Z2ggdGhlIGRvY2suIE9uIHRoZXNlCj4gPiBzeXN0ZW1zLCBy ZXN1bWluZyB0aGUgbGFwdG9wIHdoaWxlIGNvbm5lY3RlZCB0byB0aGUgZG9jayB1c3VhbGx5IHJl c3VsdHMKPiA+IGluIGJsYW5rIG1vbml0b3JzLiBNYWtpbmcgc3VyZSBNU1QgaXMgcmVhZHkgYmVm b3JlIGRvaW5nIGFueSBraW5kIG9mCj4gPiBtb2Rlc2V0dGluZyBmaXhlcyB0aGlzIGlzc3VlLgo+ IGJhc2ljIHF1ZXN0aW9uIHNpbmNlIGkgaGF2ZW4ndCB3b3JrZWQgb24gTVNUIG11Y2guIE1TVCBz aG91bGQgd29yayBsaWtlIGFueQo+IG90aGVyIGRpZ2l0YWwgcGFuZWwgb24gcmVzdW1lLiBpLmUg ZGV0ZWN0IGZvbGxvd2VkIGJ5IG1vZGVzZXQuIGluIHRoZcKgCj4gbW9kaWZpZWQKPiBjb21taXQg bWVudGlvbmVkIGJlbG93IGlzIGl0IGZhaWxpbmcgdG8gZGV0ZWN0IHRoZSBwYW5lbCBvciBmYWls aW5nIGF0wqAKPiB0aGUgbW9kZXNldCA/Cj4gaWYgd2UgYXJlIGRlcGVuZGluZyBvbiB0aGUgaW50 ZWxfZGlzcGxheV9yZXN1bWUsIGhvdyBhYm91dCBtb3ZpbmcgdGhlCj4gaW50ZWxfZHBfbXN0X3Jl c3VtZSBqdXN0IGFib3ZlIGludGVsX2Rpc3BsYXlfcmVzdW1lPwo+IAo+IAo+IEdlbmVyaWMgcXVl c3Rpb24gdG8gb3RoZXJzIGluIG1haWwgbGlzdCBvbiBpOTE1X2RybV9yZXN1bWUKPiB3ZSBhcmUg ZG9pbmcgYSBtb2Rlc2V0IGFuZCB0aGVuIGRvaW5nIHRoZSBkZXRlY3QvaHBkIGluaXQuCj4gc2hv dWxkbid0IHRoaXMgYmUgdGhlIG90aGVyIHdheSByb3VuZCA/IGFsbW9zdCBhbGwgZGlzcGxheXMK PiB3aWxsIHBhc3MgYSBtb2Rlc2V0IGV2ZW4gaWYgZGlzcGxheSBpcyBub3QgY29ubmVjdGVkIHNv IHdlCj4gYXJlIHNwZW5kaW5nIHRpbWUgb24gbW9kZXNldCBldmVuIGZvciBkaXNwbGF5cyB0aGF0 IHdlcmUKPiByZW1vdmVkIGR1cmluZyB0aGUgc3VzcGVuZCBzdGF0ZS4gaWYgdGhpcyBpcyB0byBz aW1wbHkKPiBkcm1fc3RhdGUgYmVpbmcgc2F2ZWQgYW5kIHJlc3RvcmVkLAo+ID4gV2Ugb3JpZ2lu YWxseSBjaGFuZ2VkIHRoZSByZXN1bWUgb3JkZXIgaW4KPiA+IAo+ID4gCWNvbW1pdCBlN2Q2Zjdk NzA4MjkgKCJkcm0vaTkxNTogcmVzdW1lIE1TVCBhZnRlciByZWFkaW5nIGJhY2sgaHcgc3RhdGUi KQo+ID4gCj4gPiB0byBmaXggYSB0b24gb2YgV0FSTl9PTidzIGFmdGVyIHJlc3VtZSwgYnV0IHRo aXMgZG9lc24ndCBzZWVtIHRvIGJlIGFuCj4gPiBpc3N1ZSBub3cgYW55aG93Lgo+ID4gCj4gPiBD Qzogc3RhYmxlQHZnZXIua2VybmVsLm9yZwo+ID4gU2lnbmVkLW9mZi1ieTogTHl1ZGUgPGNwYXVs QHJlZGhhdC5jb20+Cj4gPiAtLS0KPiA+IMKgIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2 LmMgfCAxMCArKysrKysrKy0tCj4gPiDCoCAxIGZpbGUgY2hhbmdlZCwgOCBpbnNlcnRpb25zKCsp LCAyIGRlbGV0aW9ucygtKQo+ID4gCj4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5 MTUvaTkxNV9kcnYuYwo+ID4gYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5jCj4gPiBp bmRleCBmMzU3MDU4Li40ZGNmM2RkIDEwMDY0NAo+ID4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5 MTUvaTkxNV9kcnYuYwo+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuYwo+ ID4gQEAgLTczMyw2ICs3MzMsMTQgQEAgc3RhdGljIGludCBpOTE1X2RybV9yZXN1bWUoc3RydWN0 IGRybV9kZXZpY2UgKmRldikKPiA+IMKgwqAJaW50ZWxfb3ByZWdpb25fc2V0dXAoZGV2KTsKPiA+ IMKgwqAKPiA+IMKgwqAJaW50ZWxfaW5pdF9wY2hfcmVmY2xrKGRldik7Cj4gPiArCj4gPiArCS8q Cj4gPiArCcKgKiBXZSBuZWVkIHRvIG1ha2Ugc3VyZSB0aGF0IHdlIHJlc3VtZSBNU1QgYmVmb3Jl IGRvaW5nIGFueXRoaW5nCj4gPiArCcKgKiBkaXNwbGF5IHJlbGF0ZWQsIG90aGVyd2lzZSB3ZSBy aXNrIHRyeWluZyB0byBicmluZyB1cCBhIGRpc3BsYXkKPiA+IG9uCj4gPiArCcKgKiBNU1QgYmVm b3JlIHRoZSBodWIgaXMgYWN0dWFsbHkgcmVhZHkKPiA+ICsJwqAqLwo+ID4gKwlpbnRlbF9kcF9t c3RfcmVzdW1lKGRldik7Cj4gPiArCj4gVGhpcyBkb2VzIG5vdCBsb29rIHByb3Blci4gaWYgdGhl IENEIGNsb2NrIGlzIHR1cm5lZCBvZmYgZHVyaW5nIHN1c3BlbmQKPiBvdXIgZHBjZCByZWFkIGl0 c2VsZiBtaWdodCBmYWlsIHRpbGwgd2UgZW5hYmxlIENEIENsb2NrLgo+IAo+IHJlZ2FyZHMsCj4g U2l2YWt1bWFyCj4gPiDCoMKgCWRybV9tb2RlX2NvbmZpZ19yZXNldChkZXYpOwo+ID4gwqDCoAo+ ID4gwqDCoAkvKgo+ID4gQEAgLTc2NSw4ICs3NzMsNiBAQCBzdGF0aWMgaW50IGk5MTVfZHJtX3Jl c3VtZShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2KQo+ID4gwqDCoAlpbnRlbF9kaXNwbGF5X3Jlc3Vt ZShkZXYpOwo+ID4gwqDCoAlkcm1fbW9kZXNldF91bmxvY2tfYWxsKGRldik7Cj4gPiDCoMKgCj4g PiAtCWludGVsX2RwX21zdF9yZXN1bWUoZGV2KTsKPiA+IC0KPiA+IMKgwqAJLyoKPiA+IMKgwqAJ wqAqIC4uLiBidXQgYWxzbyBuZWVkIHRvIG1ha2Ugc3VyZSB0aGF0IGhvdHBsdWcgcHJvY2Vzc2lu Zwo+ID4gwqDCoAnCoCogZG9lc24ndCBjYXVzZSBoYXZvYy4gTGlrZSBpbiB0aGUgZHJpdmVyIGxv YWQgY29kZSB3ZSBkb24ndAo+IApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVz a3RvcC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9k cmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mx1.redhat.com ([209.132.183.28]:59096 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S932951AbcBYPJR (ORCPT ); Thu, 25 Feb 2016 10:09:17 -0500 Message-ID: <1456412954.5486.4.camel@redhat.com> Subject: Re: [Intel-gfx] [PATCH] drm/i915: Resume DP MST before doing any kind of modesetting From: Lyude Paul To: "Thulasimani, Sivakumar" , intel-gfx@lists.freedesktop.org Cc: David Airlie , stable@vger.kernel.org, "open list:INTEL DRM DRIVERS excluding Poulsbo, Moorestow..., " "linux-kernel@vger.kernel.org open list" , Daniel Vetter Date: Thu, 25 Feb 2016 10:09:14 -0500 In-Reply-To: <56CD1660.2030203@intel.com> References: <1456265512-15670-1-git-send-email-cpaul@redhat.com> <56CD1660.2030203@intel.com> Content-Type: text/plain; charset="UTF-8" Mime-Version: 1.0 Content-Transfer-Encoding: 8bit Sender: stable-owner@vger.kernel.org List-ID: Unfortunately MST is a wild beast, and doesn't work at all like other connectors. This being said, putting it above intel_display_resume() was the first thing I tried but that didn't work. Originally I had thought putting it this high up was required because I had assumed drm_mode_config_reset() was doing modesetting as well but doing some investigation, it doesn't look like that call actually does anything. It looks like the real culprit here is intel_runtime_pm_enable_interrupts(), so long as the call to intel_dp_mst_resume() is above that everything works. So now I'm a little unsure why this is working like it is, although I can definitely see the patch fixes the issue. I'm going to edit the patch to reflect this, and see if I can get any more insight as to why this fixes it. On Wed, 2016-02-24 at 08:03 +0530, Thulasimani, Sivakumar wrote: > > On 2/24/2016 3:41 AM, Lyude wrote: > > As it turns out, resuming DP MST is racey since we don't make sure MST > > is ready before we start modesetting, it just usually happens to be > > ready in time. This isn't the case on all systems, particularly a > > ThinkPad T560 with displays connected through the dock. On these > > systems, resuming the laptop while connected to the dock usually results > > in blank monitors. Making sure MST is ready before doing any kind of > > modesetting fixes this issue. > basic question since i haven't worked on MST much. MST should work like any > other digital panel on resume. i.e detect followed by modeset. in the  > modified > commit mentioned below is it failing to detect the panel or failing at  > the modeset ? > if we are depending on the intel_display_resume, how about moving the > intel_dp_mst_resume just above intel_display_resume? > > > Generic question to others in mail list on i915_drm_resume > we are doing a modeset and then doing the detect/hpd init. > shouldn't this be the other way round ? almost all displays > will pass a modeset even if display is not connected so we > are spending time on modeset even for displays that were > removed during the suspend state. if this is to simply > drm_state being saved and restored, > > We originally changed the resume order in > > > > commit e7d6f7d70829 ("drm/i915: resume MST after reading back hw state") > > > > to fix a ton of WARN_ON's after resume, but this doesn't seem to be an > > issue now anyhow. > > > > CC: stable@vger.kernel.org > > Signed-off-by: Lyude > > --- > >   drivers/gpu/drm/i915/i915_drv.c | 10 ++++++++-- > >   1 file changed, 8 insertions(+), 2 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/i915_drv.c > > b/drivers/gpu/drm/i915/i915_drv.c > > index f357058..4dcf3dd 100644 > > --- a/drivers/gpu/drm/i915/i915_drv.c > > +++ b/drivers/gpu/drm/i915/i915_drv.c > > @@ -733,6 +733,14 @@ static int i915_drm_resume(struct drm_device *dev) > >    intel_opregion_setup(dev); > >    > >    intel_init_pch_refclk(dev); > > + > > + /* > > +  * We need to make sure that we resume MST before doing anything > > +  * display related, otherwise we risk trying to bring up a display > > on > > +  * MST before the hub is actually ready > > +  */ > > + intel_dp_mst_resume(dev); > > + > This does not look proper. if the CD clock is turned off during suspend > our dpcd read itself might fail till we enable CD Clock. > > regards, > Sivakumar > >    drm_mode_config_reset(dev); > >    > >    /* > > @@ -765,8 +773,6 @@ static int i915_drm_resume(struct drm_device *dev) > >    intel_display_resume(dev); > >    drm_modeset_unlock_all(dev); > >    > > - intel_dp_mst_resume(dev); > > - > >    /* > >     * ... but also need to make sure that hotplug processing > >     * doesn't cause havoc. Like in the driver load code we don't >