From mboxrd@z Thu Jan 1 00:00:00 1970 From: Jani Nikula Subject: Re: [PATCH 2/2] drm/i915: Turn off g4x DP port in .post_disable() Date: Thu, 14 Jun 2018 15:42:42 +0300 Message-ID: <87h8m5zghp.fsf@intel.com> References: <20180613160553.11664-1-ville.syrjala@linux.intel.com> <20180613160553.11664-2-ville.syrjala@linux.intel.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga05.intel.com (mga05.intel.com [192.55.52.43]) by gabe.freedesktop.org (Postfix) with ESMTPS id A18766E75E for ; Thu, 14 Jun 2018 12:42:50 +0000 (UTC) In-Reply-To: <20180613160553.11664-2-ville.syrjala@linux.intel.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Ville Syrjala , intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org List-Id: intel-gfx@lists.freedesktop.org T24gV2VkLCAxMyBKdW4gMjAxOCwgVmlsbGUgU3lyamFsYSA8dmlsbGUuc3lyamFsYUBsaW51eC5p bnRlbC5jb20+IHdyb3RlOgo+IEZyb206IFZpbGxlIFN5cmrDpGzDpCA8dmlsbGUuc3lyamFsYUBs aW51eC5pbnRlbC5jb20+Cj4KPiBXaGlsZSBCc3BlYyBkb2Vzbid0IGxpc3QgYSBzcGVjaWZpYyBz ZXF1ZW5jZSBmb3IgdHVybmluZyBvZmYgdGhlIERQIHBvcnQKPiBvbiBnNHggd2UgYXJlIGdldHRp bmcgYW4gdW5kZXJydW4gaWYgdGhlIHBvcnQgaXMgZGlzYWJsZWQgaW4gdGhlCj4gLmRpc2FibGUo KSBob29rLiBMb29rcyBsaWtlIHRoZSBwaXBlIHN0b3BzIHdoZW4gdGhlIHBvcnQgc3RvcHMsIGFu ZCBieQo+IHRoYXQgdGltZSB0aGUgcGxhbmUgZGlzYWJsZSBtYXkgbm90IGhhdmUgY29tcGxldGVk IHlldC4gQWxzbyB0aGUgcGxhbmUocykKPiBzZWVtIHRvIGVuZCB1cCBpbiBzb21lIHdvbmt5IHN0 YXRlIHdoZW4gdGhpcyBoYXBwZW5zIGFzIHRoZXkgYWxzbyBzaWduYWwKPiBhbm90aGVyIHVuZGVy cnVuIGltbWVkaWF0ZWx5IGFmdGVyIHdlIHR1cm4gdGhlbSBiYWNrIG9uIGR1cmluZyB0aGUgbmV4 dAo+IGVuYWJsZSBzZXF1ZW5jZS4KPgo+IFdlIGNvdWxkIGFkZCBhIHZibGFuayB3YWl0IGluIC5k aXNhYmxlKCkgdG8gYXZvaWQgd2VkZ2luZyB0aGUgcGxhbmVzLAo+IGJ1dCBJIGFzc3VtZSB3ZSdy ZSBzdGlsbCB0cmlwcGluZyB1cCB0aGUgcGlwZSBpbiBzb21lIHdheS4gU28gaXQgc2VlbXMKPiBi ZXR0ZXIgdG8gbWUgdG8ganVzdCBmb2xsb3cgdGhlIElMSysgc2VxdWVuY2UgYW5kIHR1cm4gb2Zm IHRoZSBEUCBwb3J0Cj4gaW4gLnBvc3RfZGlzYWJsZSgpIGluc3RlYWQuIFRoaXMgc2VxdWVuY2Ug ZG9lc24ndCBzZWVtIHRvIHN1ZmZlciBmcm9tCj4gdGhpcyBwcm9ibGVtLiBDb3VsZCBiZSBpdCB3 YXMgYWx3YXlzIHRoZSBpbnRlbmRlZCBzZXF1ZW5jZSBmb3IgRFAgYW5kCj4gdGhlIGdlbjQgYnNw ZWMgd2FzIGp1c3QgbmV2ZXIgdXBkYXRlZCB0byBpbmNsdWRlIGl0Lgo+Cj4gQ2M6IHN0YWJsZUB2 Z2VyLmtlcm5lbC5vcmcKPiBTaWduZWQtb2ZmLWJ5OiBWaWxsZSBTeXJqw6Rsw6QgPHZpbGxlLnN5 cmphbGFAbGludXguaW50ZWwuY29tPgoKVGhlIGNoYW5nZSBmb3IgUENIIHdhcwoKY29tbWl0IDA4 YWZmM2ZlMjZhZTdhMGQ2ZjMwMmFjMmUxYjdlMmViOTkzM2NkNDIKQXV0aG9yOiBWaWxsZSBTeXJq w6Rsw6QgPHZpbGxlLnN5cmphbGFAbGludXguaW50ZWwuY29tPgpEYXRlOiAgIE1vbiBBdWcgMTgg MjI6MTY6MDkgMjAxNCArMDMwMAoKICAgIGRybS9pOTE1OiBNb3ZlIERQIHBvcnQgZGlzYWJsZSB0 byBwb3N0X2Rpc2FibGUgZm9yIHBjaCBwbGF0Zm9ybXMKCndoZXJlIHlvdSBleHBsaWNpdGx5IGxl ZnQgb3V0IGc0eCBwZXIgbW9kZXNldCBzZXF1ZW5jZS4gSSBndWVzcyB5b3UKY291bGQgcmVmZXJl bmNlIHRoYXQgaW4gdGhlIGNvbW1pdCBtZXNzYWdlLgoKPiAtLS0KPiAgZHJpdmVycy9ncHUvZHJt L2k5MTUvaW50ZWxfZHAuYyB8IDI2ICsrKysrKysrKy0tLS0tLS0tLS0tLS0tLS0tCj4gIDEgZmls ZSBjaGFuZ2VkLCA5IGluc2VydGlvbnMoKyksIDE3IGRlbGV0aW9ucygtKQo+Cj4gZGlmZiAtLWdp dCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2RwLmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkx NS9pbnRlbF9kcC5jCj4gaW5kZXggNjA2ODk4NmZkOTg1Li41ZjA5YTgwMTVjODkgMTAwNjQ0Cj4g LS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHAuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1 L2RybS9pOTE1L2ludGVsX2RwLmMKPiBAQCAtMjgwMywxNiArMjgwMyw2IEBAIHN0YXRpYyB2b2lk IGludGVsX2Rpc2FibGVfZHAoc3RydWN0IGludGVsX2VuY29kZXIgKmVuY29kZXIsCj4gIHN0YXRp YyB2b2lkIGc0eF9kaXNhYmxlX2RwKHN0cnVjdCBpbnRlbF9lbmNvZGVyICplbmNvZGVyLAo+ICAJ CQkgICBjb25zdCBzdHJ1Y3QgaW50ZWxfY3J0Y19zdGF0ZSAqb2xkX2NydGNfc3RhdGUsCj4gIAkJ CSAgIGNvbnN0IHN0cnVjdCBkcm1fY29ubmVjdG9yX3N0YXRlICpvbGRfY29ubl9zdGF0ZSkKPiAt ewo+IC0JaW50ZWxfZGlzYWJsZV9kcChlbmNvZGVyLCBvbGRfY3J0Y19zdGF0ZSwgb2xkX2Nvbm5f c3RhdGUpOwo+IC0KPiAtCS8qIGRpc2FibGUgdGhlIHBvcnQgYmVmb3JlIHRoZSBwaXBlIG9uIGc0 eCAqLwo+IC0JaW50ZWxfZHBfbGlua19kb3duKGVuY29kZXIsIG9sZF9jcnRjX3N0YXRlKTsKPiAt fQo+IC0KPiAtc3RhdGljIHZvaWQgaWxrX2Rpc2FibGVfZHAoc3RydWN0IGludGVsX2VuY29kZXIg KmVuY29kZXIsCj4gLQkJCSAgIGNvbnN0IHN0cnVjdCBpbnRlbF9jcnRjX3N0YXRlICpvbGRfY3J0 Y19zdGF0ZSwKPiAtCQkJICAgY29uc3Qgc3RydWN0IGRybV9jb25uZWN0b3Jfc3RhdGUgKm9sZF9j b25uX3N0YXRlKQo+ICB7Cj4gIAlpbnRlbF9kaXNhYmxlX2RwKGVuY29kZXIsIG9sZF9jcnRjX3N0 YXRlLCBvbGRfY29ubl9zdGF0ZSk7Cj4gIH0KPiBAQCAtMjgyOCwxMyArMjgxOCwxOSBAQCBzdGF0 aWMgdm9pZCB2bHZfZGlzYWJsZV9kcChzdHJ1Y3QgaW50ZWxfZW5jb2RlciAqZW5jb2RlciwKPiAg CWludGVsX2Rpc2FibGVfZHAoZW5jb2Rlciwgb2xkX2NydGNfc3RhdGUsIG9sZF9jb25uX3N0YXRl KTsKPiAgfQo+ICAKPiAtc3RhdGljIHZvaWQgaWxrX3Bvc3RfZGlzYWJsZV9kcChzdHJ1Y3QgaW50 ZWxfZW5jb2RlciAqZW5jb2RlciwKPiArc3RhdGljIHZvaWQgZzR4X3Bvc3RfZGlzYWJsZV9kcChz dHJ1Y3QgaW50ZWxfZW5jb2RlciAqZW5jb2RlciwKPiAgCQkJCWNvbnN0IHN0cnVjdCBpbnRlbF9j cnRjX3N0YXRlICpvbGRfY3J0Y19zdGF0ZSwKPiAgCQkJCWNvbnN0IHN0cnVjdCBkcm1fY29ubmVj dG9yX3N0YXRlICpvbGRfY29ubl9zdGF0ZSkKPiAgewo+ICAJc3RydWN0IGludGVsX2RwICppbnRl bF9kcCA9IGVuY190b19pbnRlbF9kcCgmZW5jb2Rlci0+YmFzZSk7Cj4gIAllbnVtIHBvcnQgcG9y dCA9IGVuY29kZXItPnBvcnQ7Cj4gIAo+ICsJLyoKPiArCSAqIEJzcGVjIGRvZXMgbm90IGxpc3Qg YSBzcGVjaWZpYyBkaXNhYmxlIHNlcXVlbmNlIGZvciBnNHggRFAuCj4gKwkgKiBGb2xsb3cgdGhl IGlsaysgc2VxdWVuY2UgKGRpc2FibGUgcGlwZSBiZWZvcmUgdGhlIHBvcnQpIGZvcgo+ICsJICog ZzR4IERQIGFzIGl0IGRvZXMgbm90IHN1ZmZlciBmcm9tIHVuZGVycnVucyBsaWtlIHRoZSBub3Jt YWwKPiArCSAqIGc0eCBtb2Rlc2V0IHNlcXVlbmNlIChkaXNhYmxlIHBpcGUgYWZ0ZXIgdGhlIHBv cnQpLgo+ICsJICovCj4gIAlpbnRlbF9kcF9saW5rX2Rvd24oZW5jb2Rlciwgb2xkX2NydGNfc3Rh dGUpOwo+ICAKPiAgCS8qIE9ubHkgaWxrKyBoYXMgcG9ydCBBICovCj4gQEAgLTY0NTAsMTUgKzY0 NDYsMTEgQEAgYm9vbCBpbnRlbF9kcF9pbml0KHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZf cHJpdiwKPiAgCQlpbnRlbF9lbmNvZGVyLT5lbmFibGUgPSB2bHZfZW5hYmxlX2RwOwo+ICAJCWlu dGVsX2VuY29kZXItPmRpc2FibGUgPSB2bHZfZGlzYWJsZV9kcDsKPiAgCQlpbnRlbF9lbmNvZGVy LT5wb3N0X2Rpc2FibGUgPSB2bHZfcG9zdF9kaXNhYmxlX2RwOwo+IC0JfSBlbHNlIGlmIChJTlRF TF9HRU4oZGV2X3ByaXYpID49IDUpIHsKPiAtCQlpbnRlbF9lbmNvZGVyLT5wcmVfZW5hYmxlID0g ZzR4X3ByZV9lbmFibGVfZHA7Cj4gLQkJaW50ZWxfZW5jb2Rlci0+ZW5hYmxlID0gZzR4X2VuYWJs ZV9kcDsKPiAtCQlpbnRlbF9lbmNvZGVyLT5kaXNhYmxlID0gaWxrX2Rpc2FibGVfZHA7Cj4gLQkJ aW50ZWxfZW5jb2Rlci0+cG9zdF9kaXNhYmxlID0gaWxrX3Bvc3RfZGlzYWJsZV9kcDsKPiAtCX0g ZWxzZSB7Cj4gKwl9IGVsc2V7CgpTcGFjZSBtaXNzaW5nIGJlZm9yZSB7LgoKVGhlIGNvZGUgbWF0 Y2hlcyB0aGUgY29tbWl0IG1lc3NhZ2UsIHNvIHN0cmljdGx5IGluIHRoYXQgc2Vuc2UsCgpSZXZp ZXdlZC1ieTogSmFuaSBOaWt1bGEgPGphbmkubmlrdWxhQGludGVsLmNvbT4KCmJ1dCBJIHJlYWxs eSBkb24ndCBrbm93IGlmIHRoaXMgaXMgdGhlIHJpZ2h0IHRoaW5nIHRvIGRvLiBZb3VyCmNhbGwu IChBbmQgeW91ciByZXNwb25zaWJpbGl0eSB0byBmaXggdGhlIHJlZ3Jlc3Npb25zISA7KQoKCj4g IAkJaW50ZWxfZW5jb2Rlci0+cHJlX2VuYWJsZSA9IGc0eF9wcmVfZW5hYmxlX2RwOwo+ICAJCWlu dGVsX2VuY29kZXItPmVuYWJsZSA9IGc0eF9lbmFibGVfZHA7Cj4gIAkJaW50ZWxfZW5jb2Rlci0+ ZGlzYWJsZSA9IGc0eF9kaXNhYmxlX2RwOwo+ICsJCWludGVsX2VuY29kZXItPnBvc3RfZGlzYWJs ZSA9IGc0eF9wb3N0X2Rpc2FibGVfZHA7Cj4gIAl9Cj4gIAo+ICAJaW50ZWxfZGlnX3BvcnQtPmRw Lm91dHB1dF9yZWcgPSBvdXRwdXRfcmVnOwoKLS0gCkphbmkgTmlrdWxhLCBJbnRlbCBPcGVuIFNv dXJjZSBHcmFwaGljcyBDZW50ZXIKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRl c2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8v aW50ZWwtZ2Z4Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga07.intel.com ([134.134.136.100]:11177 "EHLO mga07.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754948AbeFNMmv (ORCPT ); Thu, 14 Jun 2018 08:42:51 -0400 From: Jani Nikula To: Ville Syrjala , intel-gfx@lists.freedesktop.org Cc: stable@vger.kernel.org Subject: Re: [Intel-gfx] [PATCH 2/2] drm/i915: Turn off g4x DP port in .post_disable() In-Reply-To: <20180613160553.11664-2-ville.syrjala@linux.intel.com> References: <20180613160553.11664-1-ville.syrjala@linux.intel.com> <20180613160553.11664-2-ville.syrjala@linux.intel.com> Date: Thu, 14 Jun 2018 15:42:42 +0300 Message-ID: <87h8m5zghp.fsf@intel.com> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Sender: stable-owner@vger.kernel.org List-ID: 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 but I really don't know if this is the right thing to do. Your call. (And your responsibility to fix the regressions! ;) > 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