From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH 2/2] drm/i915: Turn off g4x DP port in .post_disable() Date: Thu, 14 Jun 2018 21:17:07 +0300 Message-ID: <20180614181707.GL20518@intel.com> References: <20180613160553.11664-1-ville.syrjala@linux.intel.com> <20180613160553.11664-2-ville.syrjala@linux.intel.com> <87h8m5zghp.fsf@intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id D1ADC6E8A5 for ; Thu, 14 Jun 2018 18:17:10 +0000 (UTC) Content-Disposition: inline In-Reply-To: <87h8m5zghp.fsf@intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gVGh1LCBKdW4gMTQsIDIwMTggYXQgMDM6NDI6NDJQTSArMDMwMCwgSmFuaSBOaWt1bGEgd3Jv dGU6Cj4gT24gV2VkLCAxMyBKdW4gMjAxOCwgVmlsbGUgU3lyamFsYSA8dmlsbGUuc3lyamFsYUBs aW51eC5pbnRlbC5jb20+IHdyb3RlOgo+ID4gRnJvbTogVmlsbGUgU3lyasOkbMOkIDx2aWxsZS5z eXJqYWxhQGxpbnV4LmludGVsLmNvbT4KPiA+Cj4gPiBXaGlsZSBCc3BlYyBkb2Vzbid0IGxpc3Qg YSBzcGVjaWZpYyBzZXF1ZW5jZSBmb3IgdHVybmluZyBvZmYgdGhlIERQIHBvcnQKPiA+IG9uIGc0 eCB3ZSBhcmUgZ2V0dGluZyBhbiB1bmRlcnJ1biBpZiB0aGUgcG9ydCBpcyBkaXNhYmxlZCBpbiB0 aGUKPiA+IC5kaXNhYmxlKCkgaG9vay4gTG9va3MgbGlrZSB0aGUgcGlwZSBzdG9wcyB3aGVuIHRo ZSBwb3J0IHN0b3BzLCBhbmQgYnkKPiA+IHRoYXQgdGltZSB0aGUgcGxhbmUgZGlzYWJsZSBtYXkg bm90IGhhdmUgY29tcGxldGVkIHlldC4gQWxzbyB0aGUgcGxhbmUocykKPiA+IHNlZW0gdG8gZW5k IHVwIGluIHNvbWUgd29ua3kgc3RhdGUgd2hlbiB0aGlzIGhhcHBlbnMgYXMgdGhleSBhbHNvIHNp Z25hbAo+ID4gYW5vdGhlciB1bmRlcnJ1biBpbW1lZGlhdGVseSBhZnRlciB3ZSB0dXJuIHRoZW0g YmFjayBvbiBkdXJpbmcgdGhlIG5leHQKPiA+IGVuYWJsZSBzZXF1ZW5jZS4KPiA+Cj4gPiBXZSBj b3VsZCBhZGQgYSB2Ymxhbmsgd2FpdCBpbiAuZGlzYWJsZSgpIHRvIGF2b2lkIHdlZGdpbmcgdGhl IHBsYW5lcywKPiA+IGJ1dCBJIGFzc3VtZSB3ZSdyZSBzdGlsbCB0cmlwcGluZyB1cCB0aGUgcGlw ZSBpbiBzb21lIHdheS4gU28gaXQgc2VlbXMKPiA+IGJldHRlciB0byBtZSB0byBqdXN0IGZvbGxv dyB0aGUgSUxLKyBzZXF1ZW5jZSBhbmQgdHVybiBvZmYgdGhlIERQIHBvcnQKPiA+IGluIC5wb3N0 X2Rpc2FibGUoKSBpbnN0ZWFkLiBUaGlzIHNlcXVlbmNlIGRvZXNuJ3Qgc2VlbSB0byBzdWZmZXIg ZnJvbQo+ID4gdGhpcyBwcm9ibGVtLiBDb3VsZCBiZSBpdCB3YXMgYWx3YXlzIHRoZSBpbnRlbmRl ZCBzZXF1ZW5jZSBmb3IgRFAgYW5kCj4gPiB0aGUgZ2VuNCBic3BlYyB3YXMganVzdCBuZXZlciB1 cGRhdGVkIHRvIGluY2x1ZGUgaXQuCj4gPgo+ID4gQ2M6IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcK PiA+IFNpZ25lZC1vZmYtYnk6IFZpbGxlIFN5cmrDpGzDpCA8dmlsbGUuc3lyamFsYUBsaW51eC5p bnRlbC5jb20+Cj4gCj4gVGhlIGNoYW5nZSBmb3IgUENIIHdhcwo+IAo+IGNvbW1pdCAwOGFmZjNm ZTI2YWU3YTBkNmYzMDJhYzJlMWI3ZTJlYjk5MzNjZDQyCj4gQXV0aG9yOiBWaWxsZSBTeXJqw6Rs w6QgPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgo+IERhdGU6ICAgTW9uIEF1ZyAxOCAy MjoxNjowOSAyMDE0ICswMzAwCj4gCj4gICAgIGRybS9pOTE1OiBNb3ZlIERQIHBvcnQgZGlzYWJs ZSB0byBwb3N0X2Rpc2FibGUgZm9yIHBjaCBwbGF0Zm9ybXMKPiAKPiB3aGVyZSB5b3UgZXhwbGlj aXRseSBsZWZ0IG91dCBnNHggcGVyIG1vZGVzZXQgc2VxdWVuY2UuIEkgZ3Vlc3MgeW91Cj4gY291 bGQgcmVmZXJlbmNlIHRoYXQgaW4gdGhlIGNvbW1pdCBtZXNzYWdlLgo+IAo+ID4gLS0tCj4gPiAg ZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHAuYyB8IDI2ICsrKysrKysrKy0tLS0tLS0tLS0t LS0tLS0tCj4gPiAgMSBmaWxlIGNoYW5nZWQsIDkgaW5zZXJ0aW9ucygrKSwgMTcgZGVsZXRpb25z KC0pCj4gPgo+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMg Yi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcC5jCj4gPiBpbmRleCA2MDY4OTg2ZmQ5ODUu LjVmMDlhODAxNWM4OSAxMDA2NDQKPiA+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVs X2RwLmMKPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMKPiA+IEBAIC0y ODAzLDE2ICsyODAzLDYgQEAgc3RhdGljIHZvaWQgaW50ZWxfZGlzYWJsZV9kcChzdHJ1Y3QgaW50 ZWxfZW5jb2RlciAqZW5jb2RlciwKPiA+ICBzdGF0aWMgdm9pZCBnNHhfZGlzYWJsZV9kcChzdHJ1 Y3QgaW50ZWxfZW5jb2RlciAqZW5jb2RlciwKPiA+ICAJCQkgICBjb25zdCBzdHJ1Y3QgaW50ZWxf Y3J0Y19zdGF0ZSAqb2xkX2NydGNfc3RhdGUsCj4gPiAgCQkJICAgY29uc3Qgc3RydWN0IGRybV9j b25uZWN0b3Jfc3RhdGUgKm9sZF9jb25uX3N0YXRlKQo+ID4gLXsKPiA+IC0JaW50ZWxfZGlzYWJs ZV9kcChlbmNvZGVyLCBvbGRfY3J0Y19zdGF0ZSwgb2xkX2Nvbm5fc3RhdGUpOwo+ID4gLQo+ID4g LQkvKiBkaXNhYmxlIHRoZSBwb3J0IGJlZm9yZSB0aGUgcGlwZSBvbiBnNHggKi8KPiA+IC0JaW50 ZWxfZHBfbGlua19kb3duKGVuY29kZXIsIG9sZF9jcnRjX3N0YXRlKTsKPiA+IC19Cj4gPiAtCj4g PiAtc3RhdGljIHZvaWQgaWxrX2Rpc2FibGVfZHAoc3RydWN0IGludGVsX2VuY29kZXIgKmVuY29k ZXIsCj4gPiAtCQkJICAgY29uc3Qgc3RydWN0IGludGVsX2NydGNfc3RhdGUgKm9sZF9jcnRjX3N0 YXRlLAo+ID4gLQkJCSAgIGNvbnN0IHN0cnVjdCBkcm1fY29ubmVjdG9yX3N0YXRlICpvbGRfY29u bl9zdGF0ZSkKPiA+ICB7Cj4gPiAgCWludGVsX2Rpc2FibGVfZHAoZW5jb2Rlciwgb2xkX2NydGNf c3RhdGUsIG9sZF9jb25uX3N0YXRlKTsKPiA+ICB9Cj4gPiBAQCAtMjgyOCwxMyArMjgxOCwxOSBA QCBzdGF0aWMgdm9pZCB2bHZfZGlzYWJsZV9kcChzdHJ1Y3QgaW50ZWxfZW5jb2RlciAqZW5jb2Rl ciwKPiA+ICAJaW50ZWxfZGlzYWJsZV9kcChlbmNvZGVyLCBvbGRfY3J0Y19zdGF0ZSwgb2xkX2Nv bm5fc3RhdGUpOwo+ID4gIH0KPiA+ICAKPiA+IC1zdGF0aWMgdm9pZCBpbGtfcG9zdF9kaXNhYmxl X2RwKHN0cnVjdCBpbnRlbF9lbmNvZGVyICplbmNvZGVyLAo+ID4gK3N0YXRpYyB2b2lkIGc0eF9w b3N0X2Rpc2FibGVfZHAoc3RydWN0IGludGVsX2VuY29kZXIgKmVuY29kZXIsCj4gPiAgCQkJCWNv bnN0IHN0cnVjdCBpbnRlbF9jcnRjX3N0YXRlICpvbGRfY3J0Y19zdGF0ZSwKPiA+ICAJCQkJY29u c3Qgc3RydWN0IGRybV9jb25uZWN0b3Jfc3RhdGUgKm9sZF9jb25uX3N0YXRlKQo+ID4gIHsKPiA+ ICAJc3RydWN0IGludGVsX2RwICppbnRlbF9kcCA9IGVuY190b19pbnRlbF9kcCgmZW5jb2Rlci0+ YmFzZSk7Cj4gPiAgCWVudW0gcG9ydCBwb3J0ID0gZW5jb2Rlci0+cG9ydDsKPiA+ICAKPiA+ICsJ LyoKPiA+ICsJICogQnNwZWMgZG9lcyBub3QgbGlzdCBhIHNwZWNpZmljIGRpc2FibGUgc2VxdWVu Y2UgZm9yIGc0eCBEUC4KPiA+ICsJICogRm9sbG93IHRoZSBpbGsrIHNlcXVlbmNlIChkaXNhYmxl IHBpcGUgYmVmb3JlIHRoZSBwb3J0KSBmb3IKPiA+ICsJICogZzR4IERQIGFzIGl0IGRvZXMgbm90 IHN1ZmZlciBmcm9tIHVuZGVycnVucyBsaWtlIHRoZSBub3JtYWwKPiA+ICsJICogZzR4IG1vZGVz ZXQgc2VxdWVuY2UgKGRpc2FibGUgcGlwZSBhZnRlciB0aGUgcG9ydCkuCj4gPiArCSAqLwo+ID4g IAlpbnRlbF9kcF9saW5rX2Rvd24oZW5jb2Rlciwgb2xkX2NydGNfc3RhdGUpOwo+ID4gIAo+ID4g IAkvKiBPbmx5IGlsaysgaGFzIHBvcnQgQSAqLwo+ID4gQEAgLTY0NTAsMTUgKzY0NDYsMTEgQEAg Ym9vbCBpbnRlbF9kcF9pbml0KHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdiwKPiA+ ICAJCWludGVsX2VuY29kZXItPmVuYWJsZSA9IHZsdl9lbmFibGVfZHA7Cj4gPiAgCQlpbnRlbF9l bmNvZGVyLT5kaXNhYmxlID0gdmx2X2Rpc2FibGVfZHA7Cj4gPiAgCQlpbnRlbF9lbmNvZGVyLT5w b3N0X2Rpc2FibGUgPSB2bHZfcG9zdF9kaXNhYmxlX2RwOwo+ID4gLQl9IGVsc2UgaWYgKElOVEVM X0dFTihkZXZfcHJpdikgPj0gNSkgewo+ID4gLQkJaW50ZWxfZW5jb2Rlci0+cHJlX2VuYWJsZSA9 IGc0eF9wcmVfZW5hYmxlX2RwOwo+ID4gLQkJaW50ZWxfZW5jb2Rlci0+ZW5hYmxlID0gZzR4X2Vu YWJsZV9kcDsKPiA+IC0JCWludGVsX2VuY29kZXItPmRpc2FibGUgPSBpbGtfZGlzYWJsZV9kcDsK PiA+IC0JCWludGVsX2VuY29kZXItPnBvc3RfZGlzYWJsZSA9IGlsa19wb3N0X2Rpc2FibGVfZHA7 Cj4gPiAtCX0gZWxzZSB7Cj4gPiArCX0gZWxzZXsKPiAKPiBTcGFjZSBtaXNzaW5nIGJlZm9yZSB7 Lgo+IAo+IFRoZSBjb2RlIG1hdGNoZXMgdGhlIGNvbW1pdCBtZXNzYWdlLCBzbyBzdHJpY3RseSBp biB0aGF0IHNlbnNlLAo+IAo+IFJldmlld2VkLWJ5OiBKYW5pIE5pa3VsYSA8amFuaS5uaWt1bGFA aW50ZWwuY29tPgoKRml4ZWQgdXAgdGhlIHdoaXRlc3BhY2UsIGFtZW5kZWQgdGhlIGNvbW1pdCBt ZXNzYWdlIGEgYml0IHdpdGggdGhlIHBjaApjb21taXQgZGV0YWlscywgYW5kIHB1c2hlZCB0aGUg c2VyaWVzIHRvIGRpbnEuIFRoYW5rcyBmb3IgdGhlIHJldmlldy4KCj4gCj4gYnV0IEkgcmVhbGx5 IGRvbid0IGtub3cgaWYgdGhpcyBpcyB0aGUgcmlnaHQgdGhpbmcgdG8gZG8uIFlvdXIKPiBjYWxs LiAoQW5kIHlvdXIgcmVzcG9uc2liaWxpdHkgdG8gZml4IHRoZSByZWdyZXNzaW9ucyEgOykKCkkn bSBmZWVsaW5nIHVudXN1YWxseSBjb25maWRlbnQgYWJvdXQgdGhpcyBvbmUgOikKCj4gCj4gCj4g PiAgCQlpbnRlbF9lbmNvZGVyLT5wcmVfZW5hYmxlID0gZzR4X3ByZV9lbmFibGVfZHA7Cj4gPiAg CQlpbnRlbF9lbmNvZGVyLT5lbmFibGUgPSBnNHhfZW5hYmxlX2RwOwo+ID4gIAkJaW50ZWxfZW5j b2Rlci0+ZGlzYWJsZSA9IGc0eF9kaXNhYmxlX2RwOwo+ID4gKwkJaW50ZWxfZW5jb2Rlci0+cG9z dF9kaXNhYmxlID0gZzR4X3Bvc3RfZGlzYWJsZV9kcDsKPiA+ICAJfQo+ID4gIAo+ID4gIAlpbnRl bF9kaWdfcG9ydC0+ZHAub3V0cHV0X3JlZyA9IG91dHB1dF9yZWc7Cj4gCj4gLS0gCj4gSmFuaSBO aWt1bGEsIEludGVsIE9wZW4gU291cmNlIEdyYXBoaWNzIENlbnRlcgoKLS0gClZpbGxlIFN5cmrD pGzDpApJbnRlbApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpJbnRlbC1nZnggbWFpbGluZyBsaXN0CkludGVsLWdmeEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9pbnRlbC1nZngK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga05.intel.com ([192.55.52.43]:6572 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754713AbeFNSRK (ORCPT ); Thu, 14 Jun 2018 14:17:10 -0400 Date: Thu, 14 Jun 2018 21:17:07 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Jani Nikula Cc: intel-gfx@lists.freedesktop.org, stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Turn off g4x DP port in .post_disable() Message-ID: <20180614181707.GL20518@intel.com> References: <20180613160553.11664-1-ville.syrjala@linux.intel.com> <20180613160553.11664-2-ville.syrjala@linux.intel.com> <87h8m5zghp.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <87h8m5zghp.fsf@intel.com> Sender: stable-owner@vger.kernel.org List-ID: On Thu, Jun 14, 2018 at 03:42:42PM +0300, Jani Nikula wrote: > On Wed, 13 Jun 2018, Ville Syrjala wrote: > > From: Ville Syrj�l� > > > > While Bspec doesn't list a specific sequence for turning off the DP port > > on g4x we are getting an underrun if the port is disabled in the > > .disable() hook. Looks like the pipe stops when the port stops, and by > > that time the plane disable may not have completed yet. Also the plane(s) > > seem to end up in some wonky state when this happens as they also signal > > another underrun immediately after we turn them back on during the next > > enable sequence. > > > > We could add a vblank wait in .disable() to avoid wedging the planes, > > but I assume we're still tripping up the pipe in some way. So it seems > > better to me to just follow the ILK+ sequence and turn off the DP port > > in .post_disable() instead. This sequence doesn't seem to suffer from > > this problem. Could be it was always the intended sequence for DP and > > the gen4 bspec was just never updated to include it. > > > > Cc: stable@vger.kernel.org > > Signed-off-by: Ville Syrj�l� > > The change for PCH was > > commit 08aff3fe26ae7a0d6f302ac2e1b7e2eb9933cd42 > Author: Ville Syrj�l� > Date: Mon Aug 18 22:16:09 2014 +0300 > > drm/i915: Move DP port disable to post_disable for pch platforms > > where you explicitly left out g4x per modeset sequence. I guess you > could reference that in the commit message. > > > --- > > drivers/gpu/drm/i915/intel_dp.c | 26 +++++++++----------------- > > 1 file changed, 9 insertions(+), 17 deletions(-) > > > > diff --git a/drivers/gpu/drm/i915/intel_dp.c b/drivers/gpu/drm/i915/intel_dp.c > > index 6068986fd985..5f09a8015c89 100644 > > --- a/drivers/gpu/drm/i915/intel_dp.c > > +++ b/drivers/gpu/drm/i915/intel_dp.c > > @@ -2803,16 +2803,6 @@ static void intel_disable_dp(struct intel_encoder *encoder, > > static void g4x_disable_dp(struct intel_encoder *encoder, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *old_conn_state) > > -{ > > - intel_disable_dp(encoder, old_crtc_state, old_conn_state); > > - > > - /* disable the port before the pipe on g4x */ > > - intel_dp_link_down(encoder, old_crtc_state); > > -} > > - > > -static void ilk_disable_dp(struct intel_encoder *encoder, > > - const struct intel_crtc_state *old_crtc_state, > > - const struct drm_connector_state *old_conn_state) > > { > > intel_disable_dp(encoder, old_crtc_state, old_conn_state); > > } > > @@ -2828,13 +2818,19 @@ static void vlv_disable_dp(struct intel_encoder *encoder, > > intel_disable_dp(encoder, old_crtc_state, old_conn_state); > > } > > > > -static void ilk_post_disable_dp(struct intel_encoder *encoder, > > +static void g4x_post_disable_dp(struct intel_encoder *encoder, > > const struct intel_crtc_state *old_crtc_state, > > const struct drm_connector_state *old_conn_state) > > { > > struct intel_dp *intel_dp = enc_to_intel_dp(&encoder->base); > > enum port port = encoder->port; > > > > + /* > > + * Bspec does not list a specific disable sequence for g4x DP. > > + * Follow the ilk+ sequence (disable pipe before the port) for > > + * g4x DP as it does not suffer from underruns like the normal > > + * g4x modeset sequence (disable pipe after the port). > > + */ > > intel_dp_link_down(encoder, old_crtc_state); > > > > /* Only ilk+ has port A */ > > @@ -6450,15 +6446,11 @@ bool intel_dp_init(struct drm_i915_private *dev_priv, > > intel_encoder->enable = vlv_enable_dp; > > intel_encoder->disable = vlv_disable_dp; > > intel_encoder->post_disable = vlv_post_disable_dp; > > - } else if (INTEL_GEN(dev_priv) >= 5) { > > - intel_encoder->pre_enable = g4x_pre_enable_dp; > > - intel_encoder->enable = g4x_enable_dp; > > - intel_encoder->disable = ilk_disable_dp; > > - intel_encoder->post_disable = ilk_post_disable_dp; > > - } else { > > + } else{ > > Space missing before {. > > The code matches the commit message, so strictly in that sense, > > Reviewed-by: Jani Nikula Fixed up the whitespace, amended the commit message a bit with the pch commit details, and pushed the series to dinq. Thanks for the review. > > but I really don't know if this is the right thing to do. Your > call. (And your responsibility to fix the regressions! ;) I'm feeling unusually confident about this one :) > > > > intel_encoder->pre_enable = g4x_pre_enable_dp; > > intel_encoder->enable = g4x_enable_dp; > > intel_encoder->disable = g4x_disable_dp; > > + intel_encoder->post_disable = g4x_post_disable_dp; > > } > > > > intel_dig_port->dp.output_reg = output_reg; > > -- > Jani Nikula, Intel Open Source Graphics Center -- Ville Syrj�l� Intel