From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v11 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs Date: Fri, 12 Aug 2016 15:04:10 +0300 Message-ID: <20160812120410.GY4329@intel.com> References: <1470945277-7973-1-git-send-email-cpaul@redhat.com> <1470945277-7973-3-git-send-email-cpaul@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <1470945277-7973-3-git-send-email-cpaul@redhat.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: intel-gfx-bounces@lists.freedesktop.org Sender: "Intel-gfx" To: Lyude Cc: dri-devel@lists.freedesktop.org, David Airlie , Daniel Vetter , intel-gfx@lists.freedesktop.org, linux-kernel@vger.kernel.org, stable@vger.kernel.org, Daniel Vetter List-Id: dri-devel@lists.freedesktop.org T24gVGh1LCBBdWcgMTEsIDIwMTYgYXQgMDM6NTQ6MzFQTSAtMDQwMCwgTHl1ZGUgd3JvdGU6Cj4g U2luY2UgdGhlIHdhdGVybWFyayBjYWxjdWxhdGlvbnMgZm9yIFNreWxha2UgYXJlIHN0aWxsIGJy b2tlbiwgd2UncmUgYXB0Cj4gdG8gaGl0dGluZyB1bmRlcnJ1bnMgdmVyeSBlYXNpbHkgdW5kZXIg bXVsdGktbW9uaXRvciBjb25maWd1cmF0aW9ucy4KPiBXaGlsZSBpdCB3b3VsZCBiZSBsb3ZlbHkg aWYgdGhpcyB3YXMgZml4ZWQsIGl0J3Mgbm90LiBBbm90aGVyIHByb2JsZW0KPiB0aGF0J3MgYmVl biBjb21pbmcgZnJvbSB0aGlzIGhvd2V2ZXIsIGlzIHRoZSBteXN0ZXJpb3VzIGlzc3VlIG9mCj4g dW5kZXJydW5zIGNhdXNpbmcgZnVsbCBzeXN0ZW0gaGFuZ3MuIEFuIGVhc3kgd2F5IHRvIHJlcHJv ZHVjZSB0aGlzIHdpdGgKPiBhIHNreWxha2Ugc3lzdGVtOgo+IAo+IC0gR2V0IGEgbGFwdG9wIHdp dGggYSBza3lsYWtlIEdQVSwgYW5kIGhvb2sgdXAgdHdvIGV4dGVybmFsIG1vbml0b3JzIHRvCj4g ICBpdAo+IC0gTW92ZSB0aGUgY3Vyc29yIGZyb20gdGhlIGJ1aWx0LWluIExDRCB0byBvbmUgb2Yg dGhlIGV4dGVybmFsIGRpc3BsYXlzCj4gICBhcyBxdWlja2x5IGFzIHlvdSBjYW4KPiAtIFlvdSds bCBnZXQgYSBmZXcgcGlwZSB1bmRlcnJ1bnMsIGFuZCBldmVudHVhbGx5IHRoZSBlbnRpcmUgc3lz dGVtIHdpbGwKPiAgIGp1c3QgZnJlZXplLgo+IAo+IEFmdGVyIGRvaW5nIGEgbG90IG9mIGludmVz dGlnYXRpb24gYW5kIHJlYWRpbmcgdGhyb3VnaCB0aGUgYnNwZWMsIEkKPiBmb3VuZCB0aGUgZXhp c3RlbmNlIG9mIHRoZSBTQUdWLCB3aGljaCBpcyByZXNwb25zaWJsZSBmb3IgYWRqdXN0aW5nIHRo ZQo+IHN5c3RlbSBhZ2VudCB2b2x0YWdlIGFuZCBjbG9jayBmcmVxdWVuY2llcyBkZXBlbmRpbmcg b24gaG93IG11Y2ggcG93ZXIKPiB3ZSBuZWVkLiBBY2NvcmRpbmcgdG8gdGhlIGJzcGVjOgo+IAo+ ICJUaGUgZGlzcGxheSBlbmdpbmUgYWNjZXNzIHRvIHN5c3RlbSBtZW1vcnkgaXMgYmxvY2tlZCBk dXJpbmcgdGhlCj4gIGFkanVzdG1lbnQgdGltZS4gU0FHViBkZWZhdWx0cyB0byBlbmFibGVkLiBT b2Z0d2FyZSBtdXN0IHVzZSB0aGUKPiAgR1QtZHJpdmVyIHBjb2RlIG1haWxib3ggdG8gZGlzYWJs ZSBTQUdWIHdoZW4gdGhlIGRpc3BsYXkgZW5naW5lIGlzIG5vdAo+ICBhYmxlIHRvIHRvbGVyYXRl IHRoZSBibG9ja2luZyB0aW1lLiIKPiAKPiBUaGUgcmVzdCBvZiB0aGUgYnNwZWMgZ29lcyBvbiB0 byBleHBsYWluIHRoYXQgc29mdHdhcmUgY2FuIHNpbXBseSBsZWF2ZQo+IHRoZSBTQUdWIGVuYWJs ZWQsIGFuZCBkaXNhYmxlIGl0IHdoZW4gd2UgdXNlIGludGVybGFjZWQgcGlwZXMvaGF2ZSBtb3Jl Cj4gdGhlbiBvbmUgcGlwZSBhY3RpdmUuCj4gCj4gU3VyZSBlbm91Z2gsIHdpdGggdGhpcyBwYXRj aHNldCB0aGUgc3lzdGVtIGhhbmdzIHJlc3VsdGluZyBmcm9tIHBpcGUKPiB1bmRlcnJ1bnMgb24g U2t5bGFrZSBoYXZlIGNvbXBsZXRlbHkgdmFuaXNoZWQgb24gbXkgVDQ2MHMuIEFkZGl0aW9uYWxs eSwKPiB0aGUgYnNwZWMgbWVudGlvbnMgdHVybmluZyBvZmYgdGhlIFNBR1YJd2l0aCBtb3JlIHRo ZW4gb25lIHBpcGUgZW5hYmxlZAo+IGFzIGEgd29ya2Fyb3VuZCBmb3IgZGlzcGxheSB1bmRlcnJ1 bnMuIFdoaWxlIHRoaXMgcGF0Y2ggZG9lc24ndCBlbnRpcmVseQo+IGZpeCB0aGF0LCBpdCBsb29r cyBsaWtlIGl0IGRvZXMgaW1wcm92ZSB0aGUgc2l0dWF0aW9uIGEgbGl0dGxlIGJpdCBzbwo+IGl0 J3MgbGlrZWx5IHRoaXMgaXMgZ29pbmcgdG8gYmUgcmVxdWlyZWQgdG8gbWFrZSB3YXRlcm1hcmtz IG9uIFNreWxha2UKPiBmdWxseSBmdW5jdGlvbmFsLgo+IAo+IENoYW5nZXMgc2luY2UgdjEwOgo+ ICAtIEFwcGFyZW50bHkgc2FuZHlicmlkZ2VfcGNvZGVfcmVhZCBhY3R1YWxseSB3cml0ZXMgdmFs dWVzIGFuZCByZWFkcwo+ICAgIHRoZW0gYmFjaywgZGVzcGl0ZSBpdCdzIG1pc2xlYWRpbmcgZnVu Y3Rpb24gbmFtZS4gVGhpcyBtZWFucyB3ZSd2ZQo+ICAgIGJlZW4gZG9pbmcgdGhpcyBtb3N0bHkg d3JvbmcgYW5kIGhhdmUgYmVlbiB3cml0aW5nIGdhcmJhZ2UgdG8gdGhlCj4gICAgU0FHViBjb250 cm9sLiBCZWNhdXNlIG9mIHRoaXMsIHdlIG5vIGxvbmdlciBhdHRlbXB0IHRvIHJlYWQgdGhlIFNB R1YKPiAgICBzdGF0dXMgZHVyaW5nIGluaXRpYWxpemF0aW9uIChzaW5jZSB0aGVyZSBhcmUgbm8g aGVscGVycyBmb3IgdGhpcykuCj4gIC0gbWxhbmtob3JzdCBub3RpY2VkIHRoYXQgdGhpcyBwYXRj aCB3YXMgYnJlYWtpbmcgb24gc29tZSB2ZXJ5IGVhcmx5Cj4gICAgcHJlLXJlbGVhc2UgU2t5bGFr ZSBtYWNoaW5lcywgd2hpY2ggYXBwYXJlbnRseSBkb24ndCBhbGxvdyB5b3UgdG8KPiAgICBkaXNh YmxlIHRoZSBTQUdWLiBUbyBwcmV2ZW50IG1hY2hpbmVzIGZyb20gZmFpbGluZyB0ZXN0cyBkdWUg dG8gU0FHVgo+ICAgIGVycm9ycywgaWYgdGhlIGZpcnN0IHRpbWUgd2UgdHJ5IHRvIGNvbnRyb2wg dGhlIFNBR1YgcmVzdWx0cyBpbiB0aGUKPiAgICBtYWlsYm94IGluZGljYXRpbmcgYW4gaW52YWxp ZCBjb21tYW5kLCB3ZSBqdXN0IGRpc2FibGUgZnV0dXJlIGF0dGVtcHRzCj4gICAgdG8gY29udHJv bCB0aGUgU0FHViBzdGF0ZSBieSBzZXR0aW5nIGRldl9wcml2LT5za2xfc2Fndl9zdGF0dXMgdG8K PiAgICBJOTE1X1NLTF9TQUdWX05PVF9DT05UUk9MTEVEIGFuZCBtYWtlIGEgbm90ZSBvZiBpdCBp biBkbWVzZy4KPiAgLSBNb3ZlIG11dGV4X3VubG9jaygpIGEgbGl0dGxlIGhpZ2hlciBpbiBza2xf ZW5hYmxlX3NhZ3YoKS4gVGhpcwo+ICAgIGRvZXNuJ3QgYWN0dWFsbHkgZml4IGFueXRoaW5nLCBi dXQgbGV0cyB1cyByZWxlYXNlIHRoZSBsb2NrIGEgbGl0dGxlCj4gICAgc29vbmVyIHNpbmNlIHdl J3JlIGZpbmlzaGVkIHdpdGggaXQuCj4gQ2hhbmdlcyBzaW5jZSB2OToKPiAgLSBPbmx5IGVuYWJs ZS9kaXNhYmxlIHNhZ3Ygb24gU2t5bGFrZQo+IENoYW5nZXMgc2luY2Ugdjg6Cj4gIC0gQWRkIGlu dGVsX3N0YXRlLT5tb2Rlc2V0IGd1YXJkIHRvIHRoZSBjb25kaXRpb25hbCBmb3IKPiAgICBza2xf ZW5hYmxlX3NhZ3YoKQo+IENoYW5nZXMgc2luY2Ugdjc6Cj4gIC0gUmVtb3ZlIEdFTjlfU0FHVl9M T1dfRlJFUSwgcmVwbGFjZSB3aXRoIEdFTjlfU0FHVl9JU19FTkFCTEVEICh0aGF0J3MKPiAgICBh bGwgd2UgdXNlIGl0IGZvciBhbnl3YXkpCj4gIC0gVXNlIEdFTjlfU0FHVl9JU19FTkFCTEVEIGlu c3RlYWQgb2YgMHgxIGZvciBjbGFyaWZpY2F0aW9uCj4gIC0gRml4IGEgc3R5bGluZyBlcnJvciB0 aGF0IHNudWNrIHBhc3QgbWUKPiBDaGFuZ2VzIHNpbmNlIHY2Ogo+ICAtIFByb3RlY3Qgc2tsX2Vu YWJsZV9zYWd2KCkgd2l0aCBpbnRlbF9zdGF0ZS0+bW9kZXNldCBjb25kaXRpb25hbCBpbgo+ICAg IGludGVsX2F0b21pY19jb21taXRfdGFpbCgpCj4gQ2hhbmdlcyBzaW5jZSB2NToKPiAgLSBEb24n dCB1c2UgaXNfcG93ZXJfb2ZfMi4gTWFrZXMgdGhpbmdzIGNvbmZ1c2luZwo+ICAtIERvbid0IHVz ZSB0aGUgb2xkIHN0YXRlIHRvIGZpZ3VyZSBvdXQgd2hldGhlciBvciBub3QgdG8KPiAgICBlbmFi bGUvZGlzYWJsZSB0aGUgc2FndiwgdXNlIHRoZSBuZXcgb25lCj4gIC0gU3BsaXQgdGhlIGxvb3Ag aW4gc2tsX2Rpc2FibGVfc2FndiBpbnRvIGl0J3Mgb3duIGZ1bmN0aW9uCj4gIC0gTW92ZSBza2xf c2Fndl9lbmFibGUvZGlzYWJsZSgpIGNhbGxzIGludG8gaW50ZWxfYXRvbWljX2NvbW1pdF90YWls KCkKPiBDaGFuZ2VzIHNpbmNlIHY0Ogo+ICAtIFVzZSBpc19wb3dlcl9vZl8yIGFnYWluc3QgYWN0 aXZlX2NydGNzIHRvIGNoZWNrIHdoZXRoZXIgd2UgaGF2ZSA+IDEKPiAgICBwaXBlIGVuYWJsZWQK PiAgLSBGaXggc2tsX3NhZ3ZfZ2V0X2h3X3N0YXRlKCk6ICh0ZW1wICYgMHgxKSBpbmRpY2F0ZXMg ZGlzYWJsZWQsIDB4MAo+ICAgIGVuYWJsZWQKPiAgLSBDYWxsIHNrbF9zYWd2X2VuYWJsZS9kaXNh YmxlKCkgZnJvbSBwcmUvcG9zdC1wbGFuZSB1cGRhdGVzCj4gQ2hhbmdlcyBzaW5jZSB2MzoKPiAg LSBVc2UgdGltZV9iZWZvcmUoKSB0byBjb21wYXJlIHRpbWVvdXQgdG8gamlmZmllcwo+IENoYW5n ZXMgc2luY2UgdjI6Cj4gIC0gUmVhbGx5IGFwcGx5IG1pbm9yIHN0eWxlIG5pdHBpY2tzIHRvIHBh dGNoIHRoaXMgdGltZQo+IENoYW5nZXMgc2luY2UgdjE6Cj4gIC0gQWRkZWQgY29tbWVudHMgYWJv dXQgdGhpcyBwcm9iYWJseSBiZWluZyBvbmUgb2YgdGhlIHJlcXVpcmVtZW50cyB0bwo+ICAgIGZp eGluZyBTa3lsYWtlJ3Mgd2F0ZXJtYXJrIGlzc3Vlcwo+ICAtIE1pbm9yIHN0eWxlIG5pdHBpY2tz IGZyb20gTWF0dCBSb3Blcgo+ICAtIERpc2FibGUgdGhlc2UgZnVuY3Rpb25zIG9uIEJyb3h0b24s IHNpbmNlIGl0IGRvZXNuJ3QgaGF2ZSBhbiBTQUdWCj4gCj4gU2lnbmVkLW9mZi1ieTogTHl1ZGUg PGNwYXVsQHJlZGhhdC5jb20+Cj4gQ2M6IE1hdHQgUm9wZXIgPG1hdHRoZXcuZC5yb3BlckBpbnRl bC5jb20+Cj4gQ2M6IE1hYXJ0ZW4gTGFua2hvcnN0IDxtYWFydGVuLmxhbmtob3JzdEBsaW51eC5p bnRlbC5jb20+Cj4gQ2M6IERhbmllbCBWZXR0ZXIgPGRhbmllbC52ZXR0ZXJAZmZ3bGwuY2g+Cj4g Q2M6IFZpbGxlIFN5cmrDpGzDpCA8dmlsbGUuc3lyamFsYUBsaW51eC5pbnRlbC5jb20+Cj4gQ2M6 IHN0YWJsZUB2Z2VyLmtlcm5lbC5vcmcKPiAtLS0KPiAgZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkx NV9kcnYuaCAgICAgIHwgIDcgKysrCj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfcmVnLmgg ICAgICB8ICA0ICsrCj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rpc3BsYXkuYyB8IDEy ICsrKysrCj4gIGRyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5oICAgICB8ICAyICsKPiAg ZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfcG0uYyAgICAgIHwgODkgKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrCj4gIDUgZmlsZXMgY2hhbmdlZCwgMTE0IGluc2VydGlvbnMo KykKPiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuaCBiL2Ry aXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmgKPiBpbmRleCA3OTcxYzc2Li5kNzRkMTY2IDEw MDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmgKPiArKysgYi9kcml2 ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5oCj4gQEAgLTE5NDUsNiArMTk0NSwxMyBAQCBzdHJ1 Y3QgZHJtX2k5MTVfcHJpdmF0ZSB7Cj4gIAlzdHJ1Y3QgaTkxNV9zdXNwZW5kX3NhdmVkX3JlZ2lz dGVycyByZWdmaWxlOwo+ICAJc3RydWN0IHZsdl9zMGl4X3N0YXRlIHZsdl9zMGl4X3N0YXRlOwo+ ICAKPiArCWVudW0gewo+ICsJCUk5MTVfU0tMX1NBR1ZfVU5LTk9XTiA9IDAsCj4gKwkJSTkxNV9T S0xfU0FHVl9ESVNBQkxFRCwKPiArCQlJOTE1X1NLTF9TQUdWX0VOQUJMRUQsCj4gKwkJSTkxNV9T S0xfU0FHVl9OT1RfQ09OVFJPTExFRAo+ICsJfSBza2xfc2Fndl9zdGF0dXM7Cj4gKwo+ICAJc3Ry dWN0IHsKPiAgCQkvKgo+ICAJCSAqIFJhdyB3YXRlcm1hcmsgbGF0ZW5jeSB2YWx1ZXM6Cj4gZGlm ZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfcmVnLmggYi9kcml2ZXJzL2dwdS9k cm0vaTkxNS9pOTE1X3JlZy5oCj4gaW5kZXggNzNiM2Q0ZC4uNDk4MGNmZSAxMDA2NDQKPiAtLS0g YS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3JlZy5oCj4gKysrIGIvZHJpdmVycy9ncHUvZHJt L2k5MTUvaTkxNV9yZWcuaAo+IEBAIC03MTQ0LDYgKzcxNDQsMTAgQEAgZW51bSB7Cj4gICNkZWZp bmUgICBIU1dfUENPREVfREVfV1JJVEVfRlJFUV9SRVEJCTB4MTcKPiAgI2RlZmluZSAgIERJU1BM QVlfSVBTX0NPTlRST0wJCQkweDE5Cj4gICNkZWZpbmUJICBIU1dfUENPREVfRFlOQU1JQ19EVVRZ X0NZQ0xFX0NPTlRST0wJMHgxQQo+ICsjZGVmaW5lICAgR0VOOV9QQ09ERV9TQUdWX0NPTlRST0wJ CTB4MjEKPiArI2RlZmluZSAgICAgR0VOOV9TQUdWX0RJU0FCTEUJCQkweDAKPiArI2RlZmluZSAg ICAgR0VOOV9TQUdWX0lTX0RJU0FCTEVECQkweDEKPiArI2RlZmluZSAgICAgR0VOOV9TQUdWX0RZ TkFNSUNfRlJFUSAgICAgICAgICAgICAgMHgzCgpIbW0uIFRoZSBkZWZpbml0aW9uIG9mIHRoZXNl IGJpdHMgaXMgZGVmaW5pdGVseSBwZWN1bGlhci4gVW5mb3J0dW5hdGVseQp0aGUgc3BlYyBkb2Vz bid0IHNlZW0gdG8gZXhwbGFpbiB0aGVtLiBGaXJzdCBJIHRob3VnaCBiaXQgMCBtaWdodCBiZQpt b3JlIG9mIGFuIGFjayBiaXQsIGJ1dCB0aGVuIHdoeSB3b3VsZCB3ZSBzZXQgaXQgZm9yIHRoZSAi ZW5hYmxlIiByZXF1ZXN0PwoKSSdkIG1heWJlIGRvIHMvRFlOQU1JQ19GUkVRL0VOQUJMRS8gdG8g bWFrZSBpdCBtb3JlIGNsZWFyIHdoYXQgaXMgdGhlCmNvdW50ZXJwYXJ0IHRvIERJU0FCTEUuCgo+ ICAjZGVmaW5lIEdFTjZfUENPREVfREFUQQkJCQlfTU1JTygweDEzODEyOCkKPiAgI2RlZmluZSAg IEdFTjZfUENPREVfRlJFUV9JQV9SQVRJT19TSElGVAk4Cj4gICNkZWZpbmUgICBHRU42X1BDT0RF X0ZSRVFfUklOR19SQVRJT19TSElGVAkxNgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pbnRlbF9kaXNwbGF5LmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5 LmMKPiBpbmRleCBjNWMwYzM1Li4zNWJkZDY3IDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2Ry bS9pOTE1L2ludGVsX2Rpc3BsYXkuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVs X2Rpc3BsYXkuYwo+IEBAIC0xNDE0Miw2ICsxNDE0MiwxNCBAQCBzdGF0aWMgdm9pZCBpbnRlbF9h dG9taWNfY29tbWl0X3RhaWwoc3RydWN0IGRybV9hdG9taWNfc3RhdGUgKnN0YXRlKQo+ICAJCSAg ICAgaW50ZWxfc3RhdGUtPmNkY2xrX3BsbF92Y28gIT0gZGV2X3ByaXYtPmNkY2xrX3BsbC52Y28p KQo+ICAJCQlkZXZfcHJpdi0+ZGlzcGxheS5tb2Rlc2V0X2NvbW1pdF9jZGNsayhzdGF0ZSk7Cj4g IAo+ICsJCS8qCj4gKwkJICogU0tMIHdvcmthcm91bmQ6IGJzcGVjIHJlY29tbWVuZHMgd2UgZGlz YWJsZSB0aGUgU0FHViB3aGVuIHdlCj4gKwkJICogaGF2ZSBtb3JlIHRoZW4gb25lIHBpcGUgZW5h YmxlZAo+ICsJCSAqLwoKSXQgZG9lc24ndCBhY3R1YWxseSBzYXkgdGhhdCBBRkFJQ1MuIFdoYXQg aXQgc2F5cyBpcyB0aGUgKmlmKiB5b3UKZ3VhcmFudGVlIHRoYXQgaW4gc2luZ2xlIHBpcGUgbW9k ZSB5b3UgbmV2ZXIgcnVuIGFmb3VsIG9mIHRoZSBTQUdWCmJsb2NrIHRpbWUgKDMwdXMgb24gU0tM KSwgdGhlbiB5b3UgY2FuIHNpbXBseSBkaXNhYmxlIFNBR1YgZGVwZW5kaW5nCm9uIHRoZSBudW1i ZXIgb2YgYWN0aXZlIHBpcGVzLiBJIGd1ZXNzIHRoYXQgY2FuIHNvcnQgb2YgaW1wbHkgdGhhdCB5 b3UKc2hvdWxkbid0IGVuYWJsZSBpdCB3aXRoIG11bHRpcGxlIHBpcGVzLCBidXQgaXQncyBhIHZl cnkgdmFndWUgd2F5IG9mCnNheWluZyB0aGF0IGZvciBzdXJlLgoKVGhlbiBpdCBnb2VzIG9uIHRv IHNheSBzb21ldGhpbmcgdGhhdCBpZiB5b3UgY2FuJ3QgZW5hYmxlIHdhdGVybWFyawpsZXZlbHMg d2l0aCBsYXRlbmN5Pj1TQUdWIGJsb2NrIHRpbWUsIHRoZW4geW91IHNob3VsZG4ndCBlbmFibGUg U0FHVgplaXRoZXIuIEkgd291bGQgaW50ZXJwcmV0IHRoYXQgc28gdGhhdCB5b3UgaGF2ZSB0byBo YXZlIHRvIGhhdmUKZW5hYmxlZCBhdCBsZWFzdCB0aGUgZmlyc3Qgd2F0ZXJtYXJrIGxldmVsIHdp dGggaGlnaGVyIGxhdGVuY3kgdGhhbgp0aGUgU0FHViBibG9jayB0aW1lLCBzaW5jZSB0aGF0IG1l YW5zIHRoZSBwbGFuZSBjYW4gdG9sZXJhdGUgYWxzbwp0aGUgU0FHViBibG9jayB0aW1lLgoKRWcu IG9uIG9uZSBTS0wgSSBoYXZlIHRoZSBmb2xsb3dpbmcgV00gbGF0ZW5jaWVzOgpXTTAgbGF0ZW5j eSAyICgyLjAgdXNlYykKV00xIGxhdGVuY3kgMTkgKDE5LjAgdXNlYykKV00yIGxhdGVuY3kgMjgg KDI4LjAgdXNlYykKV00zIGxhdGVuY3kgMzIgKDMyLjAgdXNlYykKV000IGxhdGVuY3kgNjMgKDYz LjAgdXNlYykKV001IGxhdGVuY3kgNzcgKDc3LjAgdXNlYykKV002IGxhdGVuY3kgODMgKDgzLjAg dXNlYykKV003IGxhdGVuY3kgOTkgKDk5LjAgdXNlYykKClNvIGlmIHdlIGNhbid0IGVuYWJsZSBX TTMgZm9yIGFsbCBvZiB0aGUgYWN0aXZlIHBsYW5lcywgdGhlbiB3ZSBzaG91bGQKbm90IGVuYWJs ZSBTQUdWLiBCdXQgaWYgYWxsIHBsYW5lcyBjYW4gZW5hYmxlIFdNMyBvciBoaWdoZXIsIHdlIGNh bgphbHNvIGVuYWJsZSBTQUdWLgoKQW55d2F5cywgZG9pbmcgaXQgYWxsIHByb3Blcmx5IHNlZW1z IGxpa2UgYSBiaXQgbW9yZSB3b3JrLCBzbyB3ZSBjYW4KZGVmaW5pdGVseSBsZWF2ZSBpdCBmb3Ig YSBmdXR1cmUgaW1wcm92ZW1lbnQuCgo+ICsJCWlmIChJU19TS1lMQUtFKGRldl9wcml2KSAmJgo+ ICsJCSAgICBod2VpZ2h0MzIoaW50ZWxfc3RhdGUtPmFjdGl2ZV9jcnRjcykgPiAxKQo+ICsJCQlz a2xfZGlzYWJsZV9zYWd2KGRldl9wcml2KTsKPiArCj4gIAkJaW50ZWxfbW9kZXNldF92ZXJpZnlf ZGlzYWJsZWQoZGV2KTsKPiAgCX0KPiAgCj4gQEAgLTE0MjE1LDYgKzE0MjIzLDEwIEBAIHN0YXRp YyB2b2lkIGludGVsX2F0b21pY19jb21taXRfdGFpbChzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAq c3RhdGUpCj4gIAkJaW50ZWxfbW9kZXNldF92ZXJpZnlfY3J0YyhjcnRjLCBvbGRfY3J0Y19zdGF0 ZSwgY3J0Yy0+c3RhdGUpOwo+ICAJfQo+ICAKPiArCWlmIChJU19TS1lMQUtFKGRldl9wcml2KSAm JiBpbnRlbF9zdGF0ZS0+bW9kZXNldCAmJgo+ICsJICAgIGh3ZWlnaHQzMihpbnRlbF9zdGF0ZS0+ YWN0aXZlX2NydGNzKSA8PSAxKQo+ICsJCXNrbF9lbmFibGVfc2FndihkZXZfcHJpdik7Cj4gKwo+ ICAJZHJtX2F0b21pY19oZWxwZXJfY29tbWl0X2h3X2RvbmUoc3RhdGUpOwo+ICAKPiAgCWlmIChp bnRlbF9zdGF0ZS0+bW9kZXNldCkKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUv aW50ZWxfZHJ2LmggYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcnYuaAo+IGluZGV4IDk1 MzlmMGUuLjc2ZTc4YjggMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxf ZHJ2LmgKPiArKysgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcnYuaAo+IEBAIC0xNzIx LDYgKzE3MjEsOCBAQCB2b2lkIGlsa193bV9nZXRfaHdfc3RhdGUoc3RydWN0IGRybV9kZXZpY2Ug KmRldik7Cj4gIHZvaWQgc2tsX3dtX2dldF9od19zdGF0ZShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2 KTsKPiAgdm9pZCBza2xfZGRiX2dldF9od19zdGF0ZShzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAq ZGV2X3ByaXYsCj4gIAkJCSAgc3RydWN0IHNrbF9kZGJfYWxsb2NhdGlvbiAqZGRiIC8qIG91dCAq Lyk7Cj4gK2ludCBza2xfZW5hYmxlX3NhZ3Yoc3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRldl9w cml2KTsKPiAraW50IHNrbF9kaXNhYmxlX3NhZ3Yoc3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRl dl9wcml2KTsKPiAgdWludDMyX3QgaWxrX3BpcGVfcGl4ZWxfcmF0ZShjb25zdCBzdHJ1Y3QgaW50 ZWxfY3J0Y19zdGF0ZSAqcGlwZV9jb25maWcpOwo+ICBib29sIGlsa19kaXNhYmxlX2xwX3dtKHN0 cnVjdCBkcm1fZGV2aWNlICpkZXYpOwo+ICBpbnQgc2FuaXRpemVfcmM2X29wdGlvbihzdHJ1Y3Qg ZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3ByaXYsIGludCBlbmFibGVfcmM2KTsKPiBkaWZmIC0tZ2l0 IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfcG0uYyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1 L2ludGVsX3BtLmMKPiBpbmRleCA4NzUyNzMwLi4wYTIwMjI2NCAxMDA2NDQKPiAtLS0gYS9kcml2 ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9wbS5jCj4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUv aW50ZWxfcG0uYwo+IEBAIC0yODgzLDYgKzI4ODMsOTUgQEAgc2tsX3dtX3BsYW5lX2lkKGNvbnN0 IHN0cnVjdCBpbnRlbF9wbGFuZSAqcGxhbmUpCj4gIAl9Cj4gIH0KPiAgCj4gKy8qCj4gKyAqIFNB R1YgZHluYW1pY2FsbHkgYWRqdXN0cyB0aGUgc3lzdGVtIGFnZW50IHZvbHRhZ2UgYW5kIGNsb2Nr IGZyZXF1ZW5jaWVzCj4gKyAqIGRlcGVuZGluZyBvbiBwb3dlciBhbmQgcGVyZm9ybWFuY2UgcmVx dWlyZW1lbnRzLiBUaGUgZGlzcGxheSBlbmdpbmUgYWNjZXNzCj4gKyAqIHRvIHN5c3RlbSBtZW1v cnkgaXMgYmxvY2tlZCBkdXJpbmcgdGhlIGFkanVzdG1lbnQgdGltZS4gSGF2aW5nIHRoaXMgZW5h YmxlZAo+ICsgKiBpbiBtdWx0aS1waXBlIGNvbmZpZ3VyYXRpb25zIGNhbiBjYXVzZSBpc3N1ZXMg KHN1Y2ggYXMgdW5kZXJydW5zIGNhdXNpbmcKPiArICogZnVsbCBzeXN0ZW0gaGFuZ3MpLCBhbmQg dGhlIGJzcGVjIGFsc28gc3VnZ2VzdHMgdGhhdCBzb2Z0d2FyZSBkaXNhYmxlIGl0Cj4gKyAqIHdo ZW4gbW9yZSB0aGVuIG9uZSBwaXBlIGlzIGVuYWJsZWQuCj4gKyAqLwo+ICtpbnQKPiArc2tsX2Vu YWJsZV9zYWd2KHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdikKPiArewo+ICsJaW50 IHJldDsKPiArCj4gKwlpZiAoZGV2X3ByaXYtPnNrbF9zYWd2X3N0YXR1cyAmJgoKV2FzIHRoYXQg c3VwcG9zZWQgdG8gY2hlY2sgZm9yIE5PVF9DT05UUk9MTEVEPwoKPiArCSAgICBkZXZfcHJpdi0+ c2tsX3NhZ3Zfc3RhdHVzICE9IEk5MTVfU0tMX1NBR1ZfRElTQUJMRUQpCj4gKwkJcmV0dXJuIDA7 Cj4gKwo+ICsJbXV0ZXhfbG9jaygmZGV2X3ByaXYtPnJwcy5od19sb2NrKTsKPiArCURSTV9ERUJV R19LTVMoIkVuYWJsaW5nIHRoZSBTQUdWXG4iKTsKPiArCgpwcmludGsgY2FuIGJlIG91dHNpZGUg dGhlIG11dGV4LgoKPiArCXJldCA9IHNhbmR5YnJpZGdlX3Bjb2RlX3dyaXRlKGRldl9wcml2LCBH RU45X1BDT0RFX1NBR1ZfQ09OVFJPTCwKPiArCQkJCSAgICAgIEdFTjlfU0FHVl9EWU5BTUlDX0ZS RVEpOwo+ICsJbXV0ZXhfdW5sb2NrKCZkZXZfcHJpdi0+cnBzLmh3X2xvY2spOwo+ICsKPiArCWlm ICghcmV0KSB7Cj4gKwkJZGV2X3ByaXYtPnNrbF9zYWd2X3N0YXR1cyA9IEk5MTVfU0tMX1NBR1Zf RU5BQkxFRDsKPiArCX0gZWxzZSBpZiAocmV0ID09IC1FSU5WQUwpIHsKPiArCQlEUk1fREVCVUdf RFJJVkVSKCJObyBTQUdWIGZvdW5kIG9uIHN5c3RlbSwgaWdub3JpbmdcbiIpOwo+ICsJCWRldl9w cml2LT5za2xfc2Fndl9zdGF0dXMgPSBJOTE1X1NLTF9TQUdWX05PVF9DT05UUk9MTEVEOwo+ICsJ CXJldCA9IDA7Cj4gKwl9IGVsc2Ugewo+ICsJCURSTV9FUlJPUigiRmFpbGVkIHRvIGVuYWJsZSB0 aGUgU0FHVlxuIik7Cj4gKwl9Cj4gKwo+ICsJLyogV2UgZG9uJ3QgbmVlZCB0byB3YWl0IGZvciBT QUdWIHdoZW4gZW5hYmxpbmcgKi8KPiArCXJldHVybiByZXQ7Cj4gK30KPiArCj4gK3N0YXRpYyBp bnQKPiArc2tsX2RvX3NhZ3ZfZGlzYWJsZShzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3By aXYpCj4gK3sKPiArCWludCByZXQ7Cj4gKwl1aW50MzJfdCB0ZW1wID0gR0VOOV9TQUdWX0RJU0FC TEU7Cj4gKwo+ICsJcmV0ID0gc2FuZHlicmlkZ2VfcGNvZGVfcmVhZChkZXZfcHJpdiwgR0VOOV9Q Q09ERV9TQUdWX0NPTlRST0wsCj4gKwkJCQkgICAgICZ0ZW1wKTsKPiArCWlmIChyZXQpIHsKPiAr CQkvKgo+ICsJCSAqIFNvbWUgdmVyeSBlYXJseSBTa3lsYWtlIHN5c3RlbXMgZG9uJ3QgYWN0dWFs bHkgbGV0IHlvdQo+ICsJCSAqIGNvbnRyb2wgdGhlIFNBR1YsIHdoaWNoIGlzIG5vcm1hbC4KPiAr CQkgKi8KPiArCQlpZiAocmV0ICE9IC1FSU5WQUwpCj4gKwkJCURSTV9FUlJPUigiRmFpbGVkIHRv IGRpc2FibGUgdGhlIFNBR1ZcbiIpOwoKV2h5IGFyZSB3ZSBwcmludGluZyBlcnJvcnMgYm90aCBo ZXJlIGFuZCBpbiB0aGUgY2FsbGVyPwoKPiArCj4gKwkJcmV0dXJuIHJldDsKPiArCX0KPiArCj4g KwlyZXR1cm4gdGVtcCAmIEdFTjlfU0FHVl9JU19ESVNBQkxFRDsKPiArfQo+ICsKPiAraW50Cj4g K3NrbF9kaXNhYmxlX3NhZ3Yoc3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRldl9wcml2KQo+ICt7 Cj4gKwlpbnQgcmV0LCByZXN1bHQ7Cj4gKwo+ICsJaWYgKGRldl9wcml2LT5za2xfc2Fndl9zdGF0 dXMgJiYKCnNhbWUgcXVlc3Rpb24gYWJvdXQgTk9UX0NPTlRST0xMRUQKCj4gKwkgICAgZGV2X3By aXYtPnNrbF9zYWd2X3N0YXR1cyAhPSBJOTE1X1NLTF9TQUdWX0VOQUJMRUQpCj4gKwkJcmV0dXJu IDA7Cj4gKwo+ICsJbXV0ZXhfbG9jaygmZGV2X3ByaXYtPnJwcy5od19sb2NrKTsKPiArCURSTV9E RUJVR19LTVMoIkRpc2FibGluZyB0aGUgU0FHVlxuIik7Cgp0aGlzIHByaW50ayBjYW4gbW92ZSB1 cCBhcyB3ZWxsCgo+ICsKPiArCS8qIGJzcGVjIHNheXMgdG8ga2VlcCByZXRyeWluZyBmb3IgYXQg bGVhc3QgMSBtcyAqLwo+ICsJcmV0ID0gd2FpdF9mb3IocmVzdWx0ID0gc2tsX2RvX3NhZ3ZfZGlz YWJsZShkZXZfcHJpdiksIDEpOwo+ICsJbXV0ZXhfdW5sb2NrKCZkZXZfcHJpdi0+cnBzLmh3X2xv Y2spOwo+ICsKPiArCWlmICghcmV0KSB7Cj4gKwkJZGV2X3ByaXYtPnNrbF9zYWd2X3N0YXR1cyA9 IEk5MTVfU0tMX1NBR1ZfRElTQUJMRUQ7Cj4gKwl9IGVsc2UgaWYgKHJldCA9PSAtRVRJTUVET1VU KSB7Cj4gKwkJRFJNX0VSUk9SKCJSZXF1ZXN0IHRvIGRpc2FibGUgU0FHViB0aW1lZCBvdXRcbiIp Owo+ICsJfSBlbHNlIGlmIChyZXN1bHQgPT0gLUVJTlZBTCkgewo+ICsJCURSTV9ERUJVR19EUklW RVIoIk5vIFNBR1YgZm91bmQgb24gc3lzdGVtLCBpZ25vcmluZ1xuIik7Cj4gKwkJZGV2X3ByaXYt PnNrbF9zYWd2X3N0YXR1cyA9IEk5MTVfU0tMX1NBR1ZfTk9UX0NPTlRST0xMRUQ7Cj4gKwkJcmV0 ID0gMDsKPiArCX0KClRoZSByZXQvcmVzdWx0IHRoaW5nIGhlcmUgbG9va3MgbWVzc3kuIE1heWJl IGRlYWwgd2l0aCAncmV0JyBmaXJzdCwKYW5kIHRoZSB3aXRoICdyZXN1bHQnOgoKaWYgKHJldCkg ewoJRFJNX0VSUk9SLi4uCglyZXR1cm4gcmV0Owp9Cgpzd2l0Y2ggKHJlc3VsdCkgewpjYXNlIC1F SU5WQUw6CgkuLi4KY2FzZSAwOgpjYXNlIDE6CgkuLi4KZGVmYXVsdDoKCS4uLgp9CgpvciB3aGF0 ZXZlciBhcmUgYWxsIHRoZSBkaWZmZXJlbnQgY2FzZXMgeW91IHdhbnQgdG8gZGlzdGluZ3Vpc2gu IE9yIGRpZApJIG1pc3Mgc29tZSBzdWJ0bGUgbG9naWMgaGVyZT8KCj4gKwo+ICsJcmV0dXJuIHJl dDsKPiArfQo+ICsKPiAgc3RhdGljIHZvaWQKPiAgc2tsX2RkYl9nZXRfcGlwZV9hbGxvY2F0aW9u X2xpbWl0cyhzdHJ1Y3QgZHJtX2RldmljZSAqZGV2LAo+ICAJCQkJICAgY29uc3Qgc3RydWN0IGlu dGVsX2NydGNfc3RhdGUgKmNzdGF0ZSwKPiAtLSAKPiAyLjcuNAoKLS0gClZpbGxlIFN5cmrDpGzD pApJbnRlbCBPVEMKX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X18KSW50ZWwtZ2Z4IG1haWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3Jn Cmh0dHBzOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4 Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752744AbcHLMES (ORCPT ); Fri, 12 Aug 2016 08:04:18 -0400 Received: from mga02.intel.com ([134.134.136.20]:33339 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751574AbcHLMEQ (ORCPT ); Fri, 12 Aug 2016 08:04:16 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,510,1464678000"; d="scan'208";a="747805289" Date: Fri, 12 Aug 2016 15:04:10 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Cc: intel-gfx@lists.freedesktop.org, Maarten Lankhorst , Matt Roper , Daniel Vetter , stable@vger.kernel.org, Daniel Vetter , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs Message-ID: <20160812120410.GY4329@intel.com> References: <1470945277-7973-1-git-send-email-cpaul@redhat.com> <1470945277-7973-3-git-send-email-cpaul@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1470945277-7973-3-git-send-email-cpaul@redhat.com> User-Agent: Mutt/1.5.24 (2015-08-30) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Thu, Aug 11, 2016 at 03:54:31PM -0400, Lyude wrote: > Since the watermark calculations for Skylake are still broken, we're apt > to hitting underruns very easily under multi-monitor configurations. > While it would be lovely if this was fixed, it's not. Another problem > that's been coming from this however, is the mysterious issue of > underruns causing full system hangs. An easy way to reproduce this with > a skylake system: > > - Get a laptop with a skylake GPU, and hook up two external monitors to > it > - Move the cursor from the built-in LCD to one of the external displays > as quickly as you can > - You'll get a few pipe underruns, and eventually the entire system will > just freeze. > > After doing a lot of investigation and reading through the bspec, I > found the existence of the SAGV, which is responsible for adjusting the > system agent voltage and clock frequencies depending on how much power > we need. According to the bspec: > > "The display engine access to system memory is blocked during the > adjustment time. SAGV defaults to enabled. Software must use the > GT-driver pcode mailbox to disable SAGV when the display engine is not > able to tolerate the blocking time." > > The rest of the bspec goes on to explain that software can simply leave > the SAGV enabled, and disable it when we use interlaced pipes/have more > then one pipe active. > > Sure enough, with this patchset the system hangs resulting from pipe > underruns on Skylake have completely vanished on my T460s. Additionally, > the bspec mentions turning off the SAGV with more then one pipe enabled > as a workaround for display underruns. While this patch doesn't entirely > fix that, it looks like it does improve the situation a little bit so > it's likely this is going to be required to make watermarks on Skylake > fully functional. > > Changes since v10: > - Apparently sandybridge_pcode_read actually writes values and reads > them back, despite it's misleading function name. This means we've > been doing this mostly wrong and have been writing garbage to the > SAGV control. Because of this, we no longer attempt to read the SAGV > status during initialization (since there are no helpers for this). > - mlankhorst noticed that this patch was breaking on some very early > pre-release Skylake machines, which apparently don't allow you to > disable the SAGV. To prevent machines from failing tests due to SAGV > errors, if the first time we try to control the SAGV results in the > mailbox indicating an invalid command, we just disable future attempts > to control the SAGV state by setting dev_priv->skl_sagv_status to > I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg. > - Move mutex_unlock() a little higher in skl_enable_sagv(). This > doesn't actually fix anything, but lets us release the lock a little > sooner since we're finished with it. > Changes since v9: > - Only enable/disable sagv on Skylake > Changes since v8: > - Add intel_state->modeset guard to the conditional for > skl_enable_sagv() > Changes since v7: > - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's > all we use it for anyway) > - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification > - Fix a styling error that snuck past me > Changes since v6: > - Protect skl_enable_sagv() with intel_state->modeset conditional in > intel_atomic_commit_tail() > Changes since v5: > - Don't use is_power_of_2. Makes things confusing > - Don't use the old state to figure out whether or not to > enable/disable the sagv, use the new one > - Split the loop in skl_disable_sagv into it's own function > - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail() > Changes since v4: > - Use is_power_of_2 against active_crtcs to check whether we have > 1 > pipe enabled > - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0 > enabled > - Call skl_sagv_enable/disable() from pre/post-plane updates > Changes since v3: > - Use time_before() to compare timeout to jiffies > Changes since v2: > - Really apply minor style nitpicks to patch this time > Changes since v1: > - Added comments about this probably being one of the requirements to > fixing Skylake's watermark issues > - Minor style nitpicks from Matt Roper > - Disable these functions on Broxton, since it doesn't have an SAGV > > Signed-off-by: Lyude > Cc: Matt Roper > Cc: Maarten Lankhorst > Cc: Daniel Vetter > Cc: Ville Syrjälä > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++ > drivers/gpu/drm/i915/i915_reg.h | 4 ++ > drivers/gpu/drm/i915/intel_display.c | 12 +++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 89 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 114 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7971c76..d74d166 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1945,6 +1945,13 @@ struct drm_i915_private { > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > + enum { > + I915_SKL_SAGV_UNKNOWN = 0, > + I915_SKL_SAGV_DISABLED, > + I915_SKL_SAGV_ENABLED, > + I915_SKL_SAGV_NOT_CONTROLLED > + } skl_sagv_status; > + > struct { > /* > * Raw watermark latency values: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 73b3d4d..4980cfe 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7144,6 +7144,10 @@ enum { > #define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17 > #define DISPLAY_IPS_CONTROL 0x19 > #define HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL 0x1A > +#define GEN9_PCODE_SAGV_CONTROL 0x21 > +#define GEN9_SAGV_DISABLE 0x0 > +#define GEN9_SAGV_IS_DISABLED 0x1 > +#define GEN9_SAGV_DYNAMIC_FREQ 0x3 Hmm. The definition of these bits is definitely peculiar. Unfortunately the spec doesn't seem to explain them. First I though bit 0 might be more of an ack bit, but then why would we set it for the "enable" request? I'd maybe do s/DYNAMIC_FREQ/ENABLE/ to make it more clear what is the counterpart to DISABLE. > #define GEN6_PCODE_DATA _MMIO(0x138128) > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 > #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index c5c0c35..35bdd67 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)) > dev_priv->display.modeset_commit_cdclk(state); > > + /* > + * SKL workaround: bspec recommends we disable the SAGV when we > + * have more then one pipe enabled > + */ It doesn't actually say that AFAICS. What it says is the *if* you guarantee that in single pipe mode you never run afoul of the SAGV block time (30us on SKL), then you can simply disable SAGV depending on the number of active pipes. I guess that can sort of imply that you shouldn't enable it with multiple pipes, but it's a very vague way of saying that for sure. Then it goes on to say something that if you can't enable watermark levels with latency>=SAGV block time, then you shouldn't enable SAGV either. I would interpret that so that you have to have to have enabled at least the first watermark level with higher latency than the SAGV block time, since that means the plane can tolerate also the SAGV block time. Eg. on one SKL I have the following WM latencies: WM0 latency 2 (2.0 usec) WM1 latency 19 (19.0 usec) WM2 latency 28 (28.0 usec) WM3 latency 32 (32.0 usec) WM4 latency 63 (63.0 usec) WM5 latency 77 (77.0 usec) WM6 latency 83 (83.0 usec) WM7 latency 99 (99.0 usec) So if we can't enable WM3 for all of the active planes, then we should not enable SAGV. But if all planes can enable WM3 or higher, we can also enable SAGV. Anyways, doing it all properly seems like a bit more work, so we can definitely leave it for a future improvement. > + if (IS_SKYLAKE(dev_priv) && > + hweight32(intel_state->active_crtcs) > 1) > + skl_disable_sagv(dev_priv); > + > intel_modeset_verify_disabled(dev); > } > > @@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); > } > > + if (IS_SKYLAKE(dev_priv) && intel_state->modeset && > + hweight32(intel_state->active_crtcs) <= 1) > + skl_enable_sagv(dev_priv); > + > drm_atomic_helper_commit_hw_done(state); > > if (intel_state->modeset) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 9539f0e..76e78b8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1721,6 +1721,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev); > void skl_wm_get_hw_state(struct drm_device *dev); > void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, > struct skl_ddb_allocation *ddb /* out */); > +int skl_enable_sagv(struct drm_i915_private *dev_priv); > +int skl_disable_sagv(struct drm_i915_private *dev_priv); > uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config); > bool ilk_disable_lp_wm(struct drm_device *dev); > int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8752730..0a202264 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane) > } > } > > +/* > + * SAGV dynamically adjusts the system agent voltage and clock frequencies > + * depending on power and performance requirements. The display engine access > + * to system memory is blocked during the adjustment time. Having this enabled > + * in multi-pipe configurations can cause issues (such as underruns causing > + * full system hangs), and the bspec also suggests that software disable it > + * when more then one pipe is enabled. > + */ > +int > +skl_enable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret; > + > + if (dev_priv->skl_sagv_status && Was that supposed to check for NOT_CONTROLLED? > + dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Enabling the SAGV\n"); > + printk can be outside the mutex. > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DYNAMIC_FREQ); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED; > + } else if (ret == -EINVAL) { > + DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n"); > + dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED; > + ret = 0; > + } else { > + DRM_ERROR("Failed to enable the SAGV\n"); > + } > + > + /* We don't need to wait for SAGV when enabling */ > + return ret; > +} > + > +static int > +skl_do_sagv_disable(struct drm_i915_private *dev_priv) > +{ > + int ret; > + uint32_t temp = GEN9_SAGV_DISABLE; > + > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + &temp); > + if (ret) { > + /* > + * Some very early Skylake systems don't actually let you > + * control the SAGV, which is normal. > + */ > + if (ret != -EINVAL) > + DRM_ERROR("Failed to disable the SAGV\n"); Why are we printing errors both here and in the caller? > + > + return ret; > + } > + > + return temp & GEN9_SAGV_IS_DISABLED; > +} > + > +int > +skl_disable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret, result; > + > + if (dev_priv->skl_sagv_status && same question about NOT_CONTROLLED > + dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Disabling the SAGV\n"); this printk can move up as well > + > + /* bspec says to keep retrying for at least 1 ms */ > + ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED; > + } else if (ret == -ETIMEDOUT) { > + DRM_ERROR("Request to disable SAGV timed out\n"); > + } else if (result == -EINVAL) { > + DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n"); > + dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED; > + ret = 0; > + } The ret/result thing here looks messy. Maybe deal with 'ret' first, and the with 'result': if (ret) { DRM_ERROR... return ret; } switch (result) { case -EINVAL: ... case 0: case 1: ... default: ... } or whatever are all the different cases you want to distinguish. Or did I miss some subtle logic here? > + > + return ret; > +} > + > static void > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > const struct intel_crtc_state *cstate, > -- > 2.7.4 -- Ville Syrjälä Intel OTC From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga02.intel.com ([134.134.136.20]:33339 "EHLO mga02.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751574AbcHLMEQ (ORCPT ); Fri, 12 Aug 2016 08:04:16 -0400 Date: Fri, 12 Aug 2016 15:04:10 +0300 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Lyude Cc: intel-gfx@lists.freedesktop.org, Maarten Lankhorst , Matt Roper , Daniel Vetter , stable@vger.kernel.org, Daniel Vetter , Jani Nikula , David Airlie , dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org Subject: Re: [PATCH v11 2/7] drm/i915/skl: Add support for the SAGV, fix underrun hangs Message-ID: <20160812120410.GY4329@intel.com> References: <1470945277-7973-1-git-send-email-cpaul@redhat.com> <1470945277-7973-3-git-send-email-cpaul@redhat.com> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <1470945277-7973-3-git-send-email-cpaul@redhat.com> Sender: stable-owner@vger.kernel.org List-ID: On Thu, Aug 11, 2016 at 03:54:31PM -0400, Lyude wrote: > Since the watermark calculations for Skylake are still broken, we're apt > to hitting underruns very easily under multi-monitor configurations. > While it would be lovely if this was fixed, it's not. Another problem > that's been coming from this however, is the mysterious issue of > underruns causing full system hangs. An easy way to reproduce this with > a skylake system: > > - Get a laptop with a skylake GPU, and hook up two external monitors to > it > - Move the cursor from the built-in LCD to one of the external displays > as quickly as you can > - You'll get a few pipe underruns, and eventually the entire system will > just freeze. > > After doing a lot of investigation and reading through the bspec, I > found the existence of the SAGV, which is responsible for adjusting the > system agent voltage and clock frequencies depending on how much power > we need. According to the bspec: > > "The display engine access to system memory is blocked during the > adjustment time. SAGV defaults to enabled. Software must use the > GT-driver pcode mailbox to disable SAGV when the display engine is not > able to tolerate the blocking time." > > The rest of the bspec goes on to explain that software can simply leave > the SAGV enabled, and disable it when we use interlaced pipes/have more > then one pipe active. > > Sure enough, with this patchset the system hangs resulting from pipe > underruns on Skylake have completely vanished on my T460s. Additionally, > the bspec mentions turning off the SAGV with more then one pipe enabled > as a workaround for display underruns. While this patch doesn't entirely > fix that, it looks like it does improve the situation a little bit so > it's likely this is going to be required to make watermarks on Skylake > fully functional. > > Changes since v10: > - Apparently sandybridge_pcode_read actually writes values and reads > them back, despite it's misleading function name. This means we've > been doing this mostly wrong and have been writing garbage to the > SAGV control. Because of this, we no longer attempt to read the SAGV > status during initialization (since there are no helpers for this). > - mlankhorst noticed that this patch was breaking on some very early > pre-release Skylake machines, which apparently don't allow you to > disable the SAGV. To prevent machines from failing tests due to SAGV > errors, if the first time we try to control the SAGV results in the > mailbox indicating an invalid command, we just disable future attempts > to control the SAGV state by setting dev_priv->skl_sagv_status to > I915_SKL_SAGV_NOT_CONTROLLED and make a note of it in dmesg. > - Move mutex_unlock() a little higher in skl_enable_sagv(). This > doesn't actually fix anything, but lets us release the lock a little > sooner since we're finished with it. > Changes since v9: > - Only enable/disable sagv on Skylake > Changes since v8: > - Add intel_state->modeset guard to the conditional for > skl_enable_sagv() > Changes since v7: > - Remove GEN9_SAGV_LOW_FREQ, replace with GEN9_SAGV_IS_ENABLED (that's > all we use it for anyway) > - Use GEN9_SAGV_IS_ENABLED instead of 0x1 for clarification > - Fix a styling error that snuck past me > Changes since v6: > - Protect skl_enable_sagv() with intel_state->modeset conditional in > intel_atomic_commit_tail() > Changes since v5: > - Don't use is_power_of_2. Makes things confusing > - Don't use the old state to figure out whether or not to > enable/disable the sagv, use the new one > - Split the loop in skl_disable_sagv into it's own function > - Move skl_sagv_enable/disable() calls into intel_atomic_commit_tail() > Changes since v4: > - Use is_power_of_2 against active_crtcs to check whether we have > 1 > pipe enabled > - Fix skl_sagv_get_hw_state(): (temp & 0x1) indicates disabled, 0x0 > enabled > - Call skl_sagv_enable/disable() from pre/post-plane updates > Changes since v3: > - Use time_before() to compare timeout to jiffies > Changes since v2: > - Really apply minor style nitpicks to patch this time > Changes since v1: > - Added comments about this probably being one of the requirements to > fixing Skylake's watermark issues > - Minor style nitpicks from Matt Roper > - Disable these functions on Broxton, since it doesn't have an SAGV > > Signed-off-by: Lyude > Cc: Matt Roper > Cc: Maarten Lankhorst > Cc: Daniel Vetter > Cc: Ville Syrj�l� > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_drv.h | 7 +++ > drivers/gpu/drm/i915/i915_reg.h | 4 ++ > drivers/gpu/drm/i915/intel_display.c | 12 +++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 89 ++++++++++++++++++++++++++++++++++++ > 5 files changed, 114 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 7971c76..d74d166 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1945,6 +1945,13 @@ struct drm_i915_private { > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > + enum { > + I915_SKL_SAGV_UNKNOWN = 0, > + I915_SKL_SAGV_DISABLED, > + I915_SKL_SAGV_ENABLED, > + I915_SKL_SAGV_NOT_CONTROLLED > + } skl_sagv_status; > + > struct { > /* > * Raw watermark latency values: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 73b3d4d..4980cfe 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7144,6 +7144,10 @@ enum { > #define HSW_PCODE_DE_WRITE_FREQ_REQ 0x17 > #define DISPLAY_IPS_CONTROL 0x19 > #define HSW_PCODE_DYNAMIC_DUTY_CYCLE_CONTROL 0x1A > +#define GEN9_PCODE_SAGV_CONTROL 0x21 > +#define GEN9_SAGV_DISABLE 0x0 > +#define GEN9_SAGV_IS_DISABLED 0x1 > +#define GEN9_SAGV_DYNAMIC_FREQ 0x3 Hmm. The definition of these bits is definitely peculiar. Unfortunately the spec doesn't seem to explain them. First I though bit 0 might be more of an ack bit, but then why would we set it for the "enable" request? I'd maybe do s/DYNAMIC_FREQ/ENABLE/ to make it more clear what is the counterpart to DISABLE. > #define GEN6_PCODE_DATA _MMIO(0x138128) > #define GEN6_PCODE_FREQ_IA_RATIO_SHIFT 8 > #define GEN6_PCODE_FREQ_RING_RATIO_SHIFT 16 > diff --git a/drivers/gpu/drm/i915/intel_display.c b/drivers/gpu/drm/i915/intel_display.c > index c5c0c35..35bdd67 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -14142,6 +14142,14 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > intel_state->cdclk_pll_vco != dev_priv->cdclk_pll.vco)) > dev_priv->display.modeset_commit_cdclk(state); > > + /* > + * SKL workaround: bspec recommends we disable the SAGV when we > + * have more then one pipe enabled > + */ It doesn't actually say that AFAICS. What it says is the *if* you guarantee that in single pipe mode you never run afoul of the SAGV block time (30us on SKL), then you can simply disable SAGV depending on the number of active pipes. I guess that can sort of imply that you shouldn't enable it with multiple pipes, but it's a very vague way of saying that for sure. Then it goes on to say something that if you can't enable watermark levels with latency>=SAGV block time, then you shouldn't enable SAGV either. I would interpret that so that you have to have to have enabled at least the first watermark level with higher latency than the SAGV block time, since that means the plane can tolerate also the SAGV block time. Eg. on one SKL I have the following WM latencies: WM0 latency 2 (2.0 usec) WM1 latency 19 (19.0 usec) WM2 latency 28 (28.0 usec) WM3 latency 32 (32.0 usec) WM4 latency 63 (63.0 usec) WM5 latency 77 (77.0 usec) WM6 latency 83 (83.0 usec) WM7 latency 99 (99.0 usec) So if we can't enable WM3 for all of the active planes, then we should not enable SAGV. But if all planes can enable WM3 or higher, we can also enable SAGV. Anyways, doing it all properly seems like a bit more work, so we can definitely leave it for a future improvement. > + if (IS_SKYLAKE(dev_priv) && > + hweight32(intel_state->active_crtcs) > 1) > + skl_disable_sagv(dev_priv); > + > intel_modeset_verify_disabled(dev); > } > > @@ -14215,6 +14223,10 @@ static void intel_atomic_commit_tail(struct drm_atomic_state *state) > intel_modeset_verify_crtc(crtc, old_crtc_state, crtc->state); > } > > + if (IS_SKYLAKE(dev_priv) && intel_state->modeset && > + hweight32(intel_state->active_crtcs) <= 1) > + skl_enable_sagv(dev_priv); > + > drm_atomic_helper_commit_hw_done(state); > > if (intel_state->modeset) > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 9539f0e..76e78b8 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1721,6 +1721,8 @@ void ilk_wm_get_hw_state(struct drm_device *dev); > void skl_wm_get_hw_state(struct drm_device *dev); > void skl_ddb_get_hw_state(struct drm_i915_private *dev_priv, > struct skl_ddb_allocation *ddb /* out */); > +int skl_enable_sagv(struct drm_i915_private *dev_priv); > +int skl_disable_sagv(struct drm_i915_private *dev_priv); > uint32_t ilk_pipe_pixel_rate(const struct intel_crtc_state *pipe_config); > bool ilk_disable_lp_wm(struct drm_device *dev); > int sanitize_rc6_option(struct drm_i915_private *dev_priv, int enable_rc6); > diff --git a/drivers/gpu/drm/i915/intel_pm.c b/drivers/gpu/drm/i915/intel_pm.c > index 8752730..0a202264 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2883,6 +2883,95 @@ skl_wm_plane_id(const struct intel_plane *plane) > } > } > > +/* > + * SAGV dynamically adjusts the system agent voltage and clock frequencies > + * depending on power and performance requirements. The display engine access > + * to system memory is blocked during the adjustment time. Having this enabled > + * in multi-pipe configurations can cause issues (such as underruns causing > + * full system hangs), and the bspec also suggests that software disable it > + * when more then one pipe is enabled. > + */ > +int > +skl_enable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret; > + > + if (dev_priv->skl_sagv_status && Was that supposed to check for NOT_CONTROLLED? > + dev_priv->skl_sagv_status != I915_SKL_SAGV_DISABLED) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Enabling the SAGV\n"); > + printk can be outside the mutex. > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DYNAMIC_FREQ); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_status = I915_SKL_SAGV_ENABLED; > + } else if (ret == -EINVAL) { > + DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n"); > + dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED; > + ret = 0; > + } else { > + DRM_ERROR("Failed to enable the SAGV\n"); > + } > + > + /* We don't need to wait for SAGV when enabling */ > + return ret; > +} > + > +static int > +skl_do_sagv_disable(struct drm_i915_private *dev_priv) > +{ > + int ret; > + uint32_t temp = GEN9_SAGV_DISABLE; > + > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + &temp); > + if (ret) { > + /* > + * Some very early Skylake systems don't actually let you > + * control the SAGV, which is normal. > + */ > + if (ret != -EINVAL) > + DRM_ERROR("Failed to disable the SAGV\n"); Why are we printing errors both here and in the caller? > + > + return ret; > + } > + > + return temp & GEN9_SAGV_IS_DISABLED; > +} > + > +int > +skl_disable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret, result; > + > + if (dev_priv->skl_sagv_status && same question about NOT_CONTROLLED > + dev_priv->skl_sagv_status != I915_SKL_SAGV_ENABLED) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Disabling the SAGV\n"); this printk can move up as well > + > + /* bspec says to keep retrying for at least 1 ms */ > + ret = wait_for(result = skl_do_sagv_disable(dev_priv), 1); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_status = I915_SKL_SAGV_DISABLED; > + } else if (ret == -ETIMEDOUT) { > + DRM_ERROR("Request to disable SAGV timed out\n"); > + } else if (result == -EINVAL) { > + DRM_DEBUG_DRIVER("No SAGV found on system, ignoring\n"); > + dev_priv->skl_sagv_status = I915_SKL_SAGV_NOT_CONTROLLED; > + ret = 0; > + } The ret/result thing here looks messy. Maybe deal with 'ret' first, and the with 'result': if (ret) { DRM_ERROR... return ret; } switch (result) { case -EINVAL: ... case 0: case 1: ... default: ... } or whatever are all the different cases you want to distinguish. Or did I miss some subtle logic here? > + > + return ret; > +} > + > static void > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > const struct intel_crtc_state *cstate, > -- > 2.7.4 -- Ville Syrj�l� Intel OTC