From mboxrd@z Thu Jan 1 00:00:00 1970 From: Matt Roper Subject: Re: [PATCH v5 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs Date: Tue, 2 Aug 2016 14:05:27 -0700 Message-ID: <20160802210527.GX32025@intel.com> References: <1470163975-30467-1-git-send-email-cpaul@redhat.com> <1470163975-30467-2-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: <1470163975-30467-2-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 T24gVHVlLCBBdWcgMDIsIDIwMTYgYXQgMDI6NTI6NDlQTSAtMDQwMCwgTHl1ZGUgd3JvdGU6Cj4g 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+IENoYW5nZXMgc2luY2UgdjU6Cj4g IC0gRG9uJ3QgdXNlIGlzX3Bvd2VyX29mXzIuIE1ha2VzIHRoaW5ncyBjb25mdXNpbmcKPiAgLSBE b24ndCB1c2UgdGhlIG9sZCBzdGF0ZSB0byBmaWd1cmUgb3V0IHdoZXRoZXIgb3Igbm90IHRvCj4g ICAgZW5hYmxlL2Rpc2FibGUgdGhlIHNhZ3YsIHVzZSB0aGUgbmV3IG9uZQo+ICAtIFNwbGl0IHRo ZSBsb29wIGluIHNrbF9kaXNhYmxlX3NhZ3YgaW50byBpdCdzIG93biBmdW5jdGlvbgo+IENoYW5n ZXMgc2luY2UgdjQ6Cj4gIC0gVXNlIGlzX3Bvd2VyX29mXzIgYWdhaW5zdCBhY3RpdmVfY3J0Y3Mg dG8gY2hlY2sgd2hldGhlciB3ZSBoYXZlID4gMQo+ICAgIHBpcGUgZW5hYmxlZAo+ICAtIEZpeCBz a2xfc2Fndl9nZXRfaHdfc3RhdGUoKTogKHRlbXAgJiAweDEpIGluZGljYXRlcyBkaXNhYmxlZCwg MHgwCj4gICAgZW5hYmxlZAo+ICAtIENhbGwgc2tsX3NhZ3ZfZW5hYmxlL2Rpc2FibGUoKSBmcm9t IHByZS9wb3N0LXBsYW5lIHVwZGF0ZXMKPiBDaGFuZ2VzIHNpbmNlIHYzOgo+ICAtIFVzZSB0aW1l X2JlZm9yZSgpIHRvIGNvbXBhcmUgdGltZW91dCB0byBqaWZmaWVzCj4gQ2hhbmdlcyBzaW5jZSB2 MjoKPiAgLSBSZWFsbHkgYXBwbHkgbWlub3Igc3R5bGUgbml0cGlja3MgdG8gcGF0Y2ggdGhpcyB0 aW1lCj4gQ2hhbmdlcyBzaW5jZSB2MToKPiAgLSBBZGRlZCBjb21tZW50cyBhYm91dCB0aGlzIHBy b2JhYmx5IGJlaW5nIG9uZSBvZiB0aGUgcmVxdWlyZW1lbnRzIHRvCj4gICAgZml4aW5nIFNreWxh a2UncyB3YXRlcm1hcmsgaXNzdWVzCj4gIC0gTWlub3Igc3R5bGUgbml0cGlja3MgZnJvbSBNYXR0 IFJvcGVyCj4gIC0gRGlzYWJsZSB0aGVzZSBmdW5jdGlvbnMgb24gQnJveHRvbiwgc2luY2UgaXQg ZG9lc24ndCBoYXZlIGFuIFNBR1YKPiAKPiBSZXZpZXdlZC1ieTogTWF0dCBSb3BlciA8bWF0dGhl dy5kLnJvcGVyQGludGVsLmNvbT4KPiBTaWduZWQtb2ZmLWJ5OiBMeXVkZSA8Y3BhdWxAcmVkaGF0 LmNvbT4KPiBDYzogRGFuaWVsIFZldHRlciA8ZGFuaWVsLnZldHRlckBmZndsbC5jaD4KPiBDYzog VmlsbGUgU3lyasOkbMOkIDx2aWxsZS5zeXJqYWxhQGxpbnV4LmludGVsLmNvbT4KPiBDYzogc3Rh YmxlQHZnZXIua2VybmVsLm9yZwo+IC0tLQo+ICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X2Ry di5oICAgICAgfCAgIDIgKwo+ICBkcml2ZXJzL2dwdS9kcm0vaTkxNS9pOTE1X3JlZy5oICAgICAg fCAgIDUgKysKPiAgZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZGlzcGxheS5jIHwgIDEyICsr KysKPiAgZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfZHJ2LmggICAgIHwgICAyICsKPiAgZHJp dmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfcG0uYyAgICAgIHwgMTEyICsrKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrCj4gIDUgZmlsZXMgY2hhbmdlZCwgMTMzIGluc2VydGlvbnMoKykK PiAKPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9kcnYuaCBiL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmgKPiBpbmRleCA2NWFkYTVkLi44NzAxOGQzIDEwMDY0 NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfZHJ2LmgKPiArKysgYi9kcml2ZXJz L2dwdS9kcm0vaTkxNS9pOTE1X2Rydi5oCj4gQEAgLTE5NjIsNiArMTk2Miw4IEBAIHN0cnVjdCBk cm1faTkxNV9wcml2YXRlIHsKPiAgCXN0cnVjdCBpOTE1X3N1c3BlbmRfc2F2ZWRfcmVnaXN0ZXJz IHJlZ2ZpbGU7Cj4gIAlzdHJ1Y3Qgdmx2X3MwaXhfc3RhdGUgdmx2X3MwaXhfc3RhdGU7Cj4gIAo+ ICsJYm9vbCBza2xfc2Fndl9lbmFibGVkOwo+ICsKPiAgCXN0cnVjdCB7Cj4gIAkJLyoKPiAgCQkg KiBSYXcgd2F0ZXJtYXJrIGxhdGVuY3kgdmFsdWVzOgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dw dS9kcm0vaTkxNS9pOTE1X3JlZy5oIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaTkxNV9yZWcuaAo+ IGluZGV4IDJmOTNkNGEuLjVmYjFjNjMgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5 MTUvaTkxNV9yZWcuaAo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2k5MTVfcmVnLmgKPiBA QCAtNzE3MCw2ICs3MTcwLDExIEBAIGVudW0gewo+ICAjZGVmaW5lICAgSFNXX1BDT0RFX0RFX1dS SVRFX0ZSRVFfUkVRCQkweDE3Cj4gICNkZWZpbmUgICBESVNQTEFZX0lQU19DT05UUk9MCQkJMHgx OQo+ICAjZGVmaW5lCSAgSFNXX1BDT0RFX0RZTkFNSUNfRFVUWV9DWUNMRV9DT05UUk9MCTB4MUEK PiArI2RlZmluZSAgIEdFTjlfUENPREVfU0FHVl9DT05UUk9MCQkweDIxCj4gKyNkZWZpbmUgICAg IEdFTjlfU0FHVl9ESVNBQkxFCQkJMHgwCj4gKyNkZWZpbmUgICAgIEdFTjlfU0FHVl9MT1dfRlJF UQkJCTB4MQo+ICsjZGVmaW5lICAgICBHRU45X1NBR1ZfSElHSF9GUkVRCQkJMHgyCj4gKyNkZWZp bmUgICAgIEdFTjlfU0FHVl9EWU5BTUlDX0ZSRVEgICAgICAgICAgICAgIDB4Mwo+ICAjZGVmaW5l IEdFTjZfUENPREVfREFUQQkJCQlfTU1JTygweDEzODEyOCkKPiAgI2RlZmluZSAgIEdFTjZfUENP REVfRlJFUV9JQV9SQVRJT19TSElGVAk4Cj4gICNkZWZpbmUgICBHRU42X1BDT0RFX0ZSRVFfUklO R19SQVRJT19TSElGVAkxNgo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRl bF9kaXNwbGF5LmMgYi9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kaXNwbGF5LmMKPiBpbmRl eCBhOGU4Y2M4Li43NmJhNzlmIDEwMDY0NAo+IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2lu dGVsX2Rpc3BsYXkuYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rpc3BsYXku Ywo+IEBAIC00NTY1LDYgKzQ1NjUsNyBAQCBzdGF0aWMgdm9pZCBpbnRlbF9wb3N0X3BsYW5lX3Vw ZGF0ZShzdHJ1Y3QgaW50ZWxfY3J0Y19zdGF0ZSAqb2xkX2NydGNfc3RhdGUpCj4gIAlzdHJ1Y3Qg aW50ZWxfY3J0Y19zdGF0ZSAqcGlwZV9jb25maWcgPQo+ICAJCXRvX2ludGVsX2NydGNfc3RhdGUo Y3J0Yy0+YmFzZS5zdGF0ZSk7Cj4gIAlzdHJ1Y3QgZHJtX2RldmljZSAqZGV2ID0gY3J0Yy0+YmFz ZS5kZXY7Cj4gKwlzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3ByaXYgPSB0b19pOTE1KGRl dik7Cj4gIAlzdHJ1Y3QgZHJtX3BsYW5lICpwcmltYXJ5ID0gY3J0Yy0+YmFzZS5wcmltYXJ5Owo+ ICAJc3RydWN0IGRybV9wbGFuZV9zdGF0ZSAqb2xkX3ByaV9zdGF0ZSA9Cj4gIAkJZHJtX2F0b21p Y19nZXRfZXhpc3RpbmdfcGxhbmVfc3RhdGUob2xkX3N0YXRlLCBwcmltYXJ5KTsKPiBAQCAtNDU4 OSw2ICs0NTkwLDkgQEAgc3RhdGljIHZvaWQgaW50ZWxfcG9zdF9wbGFuZV91cGRhdGUoc3RydWN0 IGludGVsX2NydGNfc3RhdGUgKm9sZF9jcnRjX3N0YXRlKQo+ICAJCSAgICAgIW9sZF9wcmltYXJ5 X3N0YXRlLT52aXNpYmxlKSkKPiAgCQkJaW50ZWxfcG9zdF9lbmFibGVfcHJpbWFyeSgmY3J0Yy0+ YmFzZSk7Cj4gIAl9Cj4gKwo+ICsJaWYgKGh3ZWlnaHQzMihkZXZfcHJpdi0+YWN0aXZlX2NydGNz KSA8PSAxKQo+ICsJCXNrbF9lbmFibGVfc2FndihkZXZfcHJpdik7CgpUaGVyZSBtaWdodCBiZSBh IHNsaWdodGx5IGJldHRlciBwbGFjZSB0byBoYW5kbGUgdGhpczsgc2VlIGNvbW1lbnQKYmVsb3cu Cgo+ICB9Cj4gIAo+ICBzdGF0aWMgdm9pZCBpbnRlbF9wcmVfcGxhbmVfdXBkYXRlKHN0cnVjdCBp bnRlbF9jcnRjX3N0YXRlICpvbGRfY3J0Y19zdGF0ZSkKPiBAQCAtNDY0OSw2ICs0NjUzLDE0IEBA IHN0YXRpYyB2b2lkIGludGVsX3ByZV9wbGFuZV91cGRhdGUoc3RydWN0IGludGVsX2NydGNfc3Rh dGUgKm9sZF9jcnRjX3N0YXRlKQo+ICAJfQo+ICAKPiAgCS8qCj4gKwkgKiBTS0wgd29ya2Fyb3Vu ZDogYnNwZWMgcmVjb21tZW5kcyB3ZSBkaXNhYmxlIHRoZSBTQUdWIHdoZW4gd2UgaGF2ZQo+ICsJ ICogbW9yZSB0aGVuIG9uZSBwaXBlIGVuYWJsZWQKPiArCSAqLwo+ICsJaWYgKHBpcGVfY29uZmln LT5iYXNlLmFjdGl2ZSAmJgo+ICsJICAgIGh3ZWlnaHQzMihkZXZfcHJpdi0+YWN0aXZlX2NydGNz IHwgZHJtX2NydGNfbWFzaygmY3J0Yy0+YmFzZSkpID4gMSkKPiArCQlza2xfZGlzYWJsZV9zYWd2 KGRldl9wcml2KTsKCkFzIEhhbnMgcG9pbnRlZCBvdXQsIHRoaXMgZG9lc24ndCBsb29rIHJpZ2h0 LiAgSSdtIGd1ZXNzaW5nIHlvdSdyZQp0cnlpbmcgdG8gZ3VhcmQgYWdhaW5zdCB0aGUgY2FzZSB3 aGVyZSB0aGVyZSdzIG5vIGludGVyc2VjdGlvbiBiZXR3ZWVuCnRoZSBzdGFydGluZyBwaXBlIHVz YWdlIGFuZCB0aGUgZmluYWwgcGlwZSB1c2FnZT8gIEUuZy4sIG9ubHkgcGlwZSBBCmFjdGl2ZSBi ZWZvcmUgY29tbWl0LCBvbmx5IHBpcGUgQiBhY3RpdmUgYWZ0ZXIgY29tbWl0LCBhbmQgeW91J3Jl CndvcnJpZWQgdGhlcmUgbWlnaHQgYmUgYSBicmllZiBwb2ludCB3aGVyZSBib3RoIEEgYW5kIEIg YXJlIG9uIHRvZ2V0aGVyPwpJIGRvbid0IHRoaW5rIHRoaXMgc2hvdWxkIHJlYWxseSBtYXR0ZXIg c2luY2Ugd2UgcG93ZXIgZXZlcnl0aGluZwpvbGQtY2hhbmdpbmcgZG93biBiZWZvcmUgcG93ZXJp bmcgYW55dGhpbmcgbmV3LWNoYW5naW5nIHVwLiAgU28gSSB0aGluawphbGwgd2UgcmVhbGx5IG5l ZWQgdG8gY2FyZSBhYm91dCBmb3IgZW5hYmxpbmcvZGlzYWJsaW5nIGlzIGhvdyBtYW55CkNSVEMn cyB0aGVyZSBhcmUgaW4gdGhlIGZpbmFsIHN0YXRlLgoKSXQgc2VlbXMgbGlrZSBhIG1vcmUgbmF0 dXJhbCBwbGFjZSB0byBwbGFjZSB0byBoYW5kbGUgZW5hYmxlL2Rpc2FibGUKd291bGQgYmUgaW4g dGhlICdpZiAoaW50ZWxfc3RhdGUtPm1vZGVzZXQpJyBibG9ja3Mgb2YKaW50ZWxfYXRvbWljX2Nv bW1pdF90YWlsKCkgKHNpbmNlIG9ubHkgYSBtb2Rlc2V0IG9wZXJhdGlvbiBjb3VsZCBjaGFuZ2UK dGhlIG51bWJlciBvZiBDUlRDJ3MgaW4gdXNlIHRvIHRyaWdnZXIgYSBTQUdWIHRvZ2dsZSkuICBU aGF0IGFsc28gaGFzCnRoZSBzbGlnaHQgYmVuZWZpdCBvZiBvbmx5IGdldHRpbmcgcnVuIG9uY2Ug Zm9yIHRoZSBhdG9taWMgdHJhbnNhY3Rpb24KcmF0aGVyIHRoYW4gb25jZSBmb3IgZWFjaCBDUlRD IChhbmQgeW91IGNhbiBqdXN0IGRvIGEgc2ltcGxlIHRlc3Qgb2YKaHdlaWdodChpbnRlbF9zdGF0 ZS0+YWN0aXZlX2NydGNzKSB0byBmaWd1cmUgb3V0IGhvdyBtYW55IENSVEMncyBhcmUKZ29pbmcg dG8gYmUgb24gYWZ0ZXIgdGhlIHRyYW5zYWN0aW9uIGNvbXBsZXRlcy4KCgo+ICsKPiArCS8qCj4g IAkgKiBJZiB3ZSdyZSBkb2luZyBhIG1vZGVzZXQsIHdlJ3JlIGRvbmUuICBObyBuZWVkIHRvIGRv IGFueSBwcmUtdmJsYW5rCj4gIAkgKiB3YXRlcm1hcmsgcHJvZ3JhbW1pbmcgaGVyZS4KPiAgCSAq Lwo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcnYuaCBiL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5oCj4gaW5kZXggNTBjZGM4OS4uNmIwNTMyYSAxMDA2 NDQKPiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vaTkxNS9pbnRlbF9kcnYuaAo+ICsrKyBiL2RyaXZl cnMvZ3B1L2RybS9pOTE1L2ludGVsX2Rydi5oCj4gQEAgLTE3MDksNiArMTcwOSw4IEBAIHZvaWQg aWxrX3dtX2dldF9od19zdGF0ZShzdHJ1Y3QgZHJtX2RldmljZSAqZGV2KTsKPiAgdm9pZCBza2xf d21fZ2V0X2h3X3N0YXRlKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYpOwo+ICB2b2lkIHNrbF9kZGJf Z2V0X2h3X3N0YXRlKHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdiwKPiAgCQkJICBz dHJ1Y3Qgc2tsX2RkYl9hbGxvY2F0aW9uICpkZGIgLyogb3V0ICovKTsKPiAraW50IHNrbF9lbmFi bGVfc2FndihzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3ByaXYpOwo+ICtpbnQgc2tsX2Rp c2FibGVfc2FndihzdHJ1Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3ByaXYpOwo+ICB1aW50MzJf dCBpbGtfcGlwZV9waXhlbF9yYXRlKGNvbnN0IHN0cnVjdCBpbnRlbF9jcnRjX3N0YXRlICpwaXBl X2NvbmZpZyk7Cj4gIGJvb2wgaWxrX2Rpc2FibGVfbHBfd20oc3RydWN0IGRybV9kZXZpY2UgKmRl dik7Cj4gIGludCBzYW5pdGl6ZV9yYzZfb3B0aW9uKHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpk ZXZfcHJpdiwgaW50IGVuYWJsZV9yYzYpOwo+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0v aTkxNS9pbnRlbF9wbS5jIGIvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50ZWxfcG0uYwo+IGluZGV4 IGY2MTBiNzEuLjY4NzIxYTUgMTAwNjQ0Cj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL2k5MTUvaW50 ZWxfcG0uYwo+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9pOTE1L2ludGVsX3BtLmMKPiBAQCAtMjg4 NCw2ICsyODg0LDExNiBAQCBza2xfd21fcGxhbmVfaWQoY29uc3Qgc3RydWN0IGludGVsX3BsYW5l ICpwbGFuZSkKPiAgfQo+ICAKPiAgc3RhdGljIHZvaWQKPiArc2tsX3NhZ3ZfZ2V0X2h3X3N0YXRl KHN0cnVjdCBkcm1faTkxNV9wcml2YXRlICpkZXZfcHJpdikKPiArewo+ICsJdTMyIHRlbXA7Cj4g KwlpbnQgcmV0Owo+ICsKPiArCWlmIChJU19CUk9YVE9OKGRldl9wcml2KSkKPiArCQlyZXR1cm47 Cj4gKwo+ICsJbXV0ZXhfbG9jaygmZGV2X3ByaXYtPnJwcy5od19sb2NrKTsKPiArCXJldCA9IHNh bmR5YnJpZGdlX3Bjb2RlX3JlYWQoZGV2X3ByaXYsIEdFTjlfUENPREVfU0FHVl9DT05UUk9MLCAm dGVtcCk7Cj4gKwltdXRleF91bmxvY2soJmRldl9wcml2LT5ycHMuaHdfbG9jayk7Cj4gKwo+ICsJ aWYgKCFyZXQpIHsKPiArCQlkZXZfcHJpdi0+c2tsX3NhZ3ZfZW5hYmxlZCA9ICEodGVtcCAmIDB4 MSk7Cj4gKwl9IGVsc2Ugewo+ICsJCS8qCj4gKwkJICogSWYgZm9yIHNvbWUgcmVhc29uIHdlIGNh bid0IGFjY2VzcyB0aGUgU0FHViBzdGF0ZSwgZm9sbG93Cj4gKwkJICogdGhlIGJzcGVjIGFuZCBh c3N1bWUgaXQncyBlbmFibGVkCj4gKwkJICovCj4gKwkJRFJNX0VSUk9SKCJGYWlsZWQgdG8gZ2V0 IFNBR1Ygc3RhdGUsIGFzc3VtaW5nIGVuYWJsZWRcbiIpOwo+ICsJCWRldl9wcml2LT5za2xfc2Fn dl9lbmFibGVkID0gdHJ1ZTsKPiArCX0KPiArfQo+ICsKPiArLyoKPiArICogU0FHViBkeW5hbWlj YWxseSBhZGp1c3RzIHRoZSBzeXN0ZW0gYWdlbnQgdm9sdGFnZSBhbmQgY2xvY2sgZnJlcXVlbmNp ZXMKPiArICogZGVwZW5kaW5nIG9uIHBvd2VyIGFuZCBwZXJmb3JtYW5jZSByZXF1aXJlbWVudHMu IFRoZSBkaXNwbGF5IGVuZ2luZSBhY2Nlc3MKPiArICogdG8gc3lzdGVtIG1lbW9yeSBpcyBibG9j a2VkIGR1cmluZyB0aGUgYWRqdXN0bWVudCB0aW1lLiBIYXZpbmcgdGhpcyBlbmFibGVkCj4gKyAq IGluIG11bHRpLXBpcGUgY29uZmlndXJhdGlvbnMgY2FuIGNhdXNlIGlzc3VlcyAoc3VjaCBhcyB1 bmRlcnJ1bnMgY2F1c2luZwo+ICsgKiBmdWxsIHN5c3RlbSBoYW5ncyksIGFuZCB0aGUgYnNwZWMg YWxzbyBzdWdnZXN0cyB0aGF0IHNvZnR3YXJlIGRpc2FibGUgaXQKPiArICogd2hlbiBtb3JlIHRo ZW4gb25lIHBpcGUgaXMgZW5hYmxlZC4KPiArICovCj4gK2ludAo+ICtza2xfZW5hYmxlX3NhZ3Yo c3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRldl9wcml2KQo+ICt7Cj4gKwlpbnQgcmV0Owo+ICsK PiArCWlmIChJU19CUk9YVE9OKGRldl9wcml2KSkKPiArCQlyZXR1cm4gMDsKPiArCWlmIChkZXZf cHJpdi0+c2tsX3NhZ3ZfZW5hYmxlZCkKPiArCQlyZXR1cm4gMDsKPiArCj4gKwltdXRleF9sb2Nr KCZkZXZfcHJpdi0+cnBzLmh3X2xvY2spOwo+ICsJRFJNX0RFQlVHX0tNUygiRW5hYmxpbmcgdGhl IFNBR1ZcbiIpOwo+ICsKPiArCXJldCA9IHNhbmR5YnJpZGdlX3Bjb2RlX3dyaXRlKGRldl9wcml2 LCBHRU45X1BDT0RFX1NBR1ZfQ09OVFJPTCwKPiArCQkJCSAgICAgIEdFTjlfU0FHVl9EWU5BTUlD X0ZSRVEpOwo+ICsJaWYgKCFyZXQpCj4gKwkJZGV2X3ByaXYtPnNrbF9zYWd2X2VuYWJsZWQgPSB0 cnVlOwo+ICsJZWxzZQo+ICsJCURSTV9FUlJPUigiRmFpbGVkIHRvIGVuYWJsZSB0aGUgU0FHVlxu Iik7Cj4gKwo+ICsJLyogV2UgZG9uJ3QgbmVlZCB0byB3YWl0IGZvciBTQUdWIHdoZW4gZW5hYmxp bmcgKi8KPiArCW11dGV4X3VubG9jaygmZGV2X3ByaXYtPnJwcy5od19sb2NrKTsKPiArCXJldHVy biByZXQ7Cj4gK30KPiArCj4gK3N0YXRpYyBpbnQKPiArc2tsX2RvX3NhZ3ZfZGlzYWJsZShzdHJ1 Y3QgZHJtX2k5MTVfcHJpdmF0ZSAqZGV2X3ByaXYpCj4gK3sKPiArCWludCByZXQ7Cj4gKwl1aW50 MzJfdCB0ZW1wOwo+ICsKPiArCXJldCA9IHNhbmR5YnJpZGdlX3Bjb2RlX3dyaXRlKGRldl9wcml2 LCBHRU45X1BDT0RFX1NBR1ZfQ09OVFJPTCwKPiArCQkJCSAgICAgIEdFTjlfU0FHVl9ESVNBQkxF KTsKPiArCWlmIChyZXQpIHsKPiArCQlEUk1fRVJST1IoIkZhaWxlZCB0byBkaXNhYmxlIHRoZSBT QUdWXG4iKTsKPiArCQlyZXR1cm4gcmV0Owo+ICsJfQo+ICsKPiArCXJldCA9IHNhbmR5YnJpZGdl X3Bjb2RlX3JlYWQoZGV2X3ByaXYsIEdFTjlfUENPREVfU0FHVl9DT05UUk9MLAo+ICsJCQkJICAg ICAmdGVtcCk7Cj4gKwlpZiAocmV0KSB7Cj4gKwkJRFJNX0VSUk9SKCJGYWlsZWQgdG8gY2hlY2sg dGhlIHN0YXR1cyBvZiB0aGUgU0FHVlxuIik7Cj4gKwkJcmV0dXJuIHJldDsKPiArCX0KPiArCj4g KwlyZXR1cm4gdGVtcCAmIDB4MTsKPiArfQo+ICsKPiAraW50Cj4gK3NrbF9kaXNhYmxlX3NhZ3Yo c3RydWN0IGRybV9pOTE1X3ByaXZhdGUgKmRldl9wcml2KQo+ICt7Cj4gKwlpbnQgcmV0LCByZXN1 bHQ7Cj4gKwo+ICsJaWYgKElTX0JST1hUT04oZGV2X3ByaXYpKQo+ICsJCXJldHVybiAwOwo+ICsJ aWYgKCFkZXZfcHJpdi0+c2tsX3NhZ3ZfZW5hYmxlZCkKPiArCQlyZXR1cm4gMDsKPiArCj4gKwlt dXRleF9sb2NrKCZkZXZfcHJpdi0+cnBzLmh3X2xvY2spOwo+ICsJRFJNX0RFQlVHX0tNUygiRGlz YWJsaW5nIHRoZSBTQUdWXG4iKTsKPiArCj4gKwkvKiBic3BlYyBzYXlzIHRvIGtlZXAgcmV0cnlp bmcgZm9yIGF0IGxlYXN0IDEgbXMgKi8KPiArCXJldCA9IHdhaXRfZm9yKHJlc3VsdCA9IHNrbF9k b19zYWd2X2Rpc2FibGUoZGV2X3ByaXYpLCAxKTsKPiArCW11dGV4X3VubG9jaygmZGV2X3ByaXYt PnJwcy5od19sb2NrKTsKPiArCj4gKwlpZiAocmV0ID09IC1FVElNRURPVVQpCj4gKwkJRFJNX0VS Uk9SKCJSZXF1ZXN0IHRvIGRpc2FibGUgU0FHViB0aW1lZCBvdXRcbiIpOwo+ICsJZWxzZSB7CgpN aW5vciBzdHlsZSBuaXRwaWNrOyBpZiBlaXRoZXIgYnJhbmNoIG9mIGFuIGlmL2Vsc2UgbmVlZHMg YnJhY2VzLCB0aGV5CmJvdGggbmVlZCBicmFjZXMuCgo+ICsJCWlmIChyZXN1bHQgPT0gMSkKCllv dSd2ZSBnb3QgYSBmZXcgdXNlcyBvZiAweDEgYXMgYSBtYWdpYyBudW1iZXIgdGhhdCBhcmUgY29y cmVjdCwgYnV0CnNlZW0gY291bnRlcmludHVpdGl2ZSB0byBzb21lb25lIG5vdCBsb29raW5nIGF0 IHRoZSBic3BlYyAoMT1vZmYpLgpNYXliZSByZXBsYWNpbmcgdGhlc2Ugd2l0aCBhICNkZWZpbmUg bWlnaHQgaGVscCBjbGFyaWZ5IHNsaWdodGx5PwoKCk1hdHQKCj4gKwkJCWRldl9wcml2LT5za2xf c2Fndl9lbmFibGVkID0gZmFsc2U7Cj4gKwo+ICsJCXJldCA9IHJlc3VsdDsKPiArCX0KPiArCj4g KwlyZXR1cm4gcmV0Owo+ICt9Cj4gKwo+ICtzdGF0aWMgdm9pZAo+ICBza2xfZGRiX2dldF9waXBl X2FsbG9jYXRpb25fbGltaXRzKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYsCj4gIAkJCQkgICBjb25z dCBzdHJ1Y3QgaW50ZWxfY3J0Y19zdGF0ZSAqY3N0YXRlLAo+ICAJCQkJICAgc3RydWN0IHNrbF9k ZGJfZW50cnkgKmFsbG9jLCAvKiBvdXQgKi8KPiBAQCAtNDIzNiw2ICs0MzQ2LDggQEAgdm9pZCBz a2xfd21fZ2V0X2h3X3N0YXRlKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYpCj4gIAkJLyogRWFzeS9j b21tb24gY2FzZTsganVzdCBzYW5pdGl6ZSBEREIgbm93IGlmIGV2ZXJ5dGhpbmcgb2ZmICovCj4g IAkJbWVtc2V0KGRkYiwgMCwgc2l6ZW9mKCpkZGIpKTsKPiAgCX0KPiArCj4gKwlza2xfc2Fndl9n ZXRfaHdfc3RhdGUoZGV2X3ByaXYpOwo+ICB9Cj4gIAo+ICBzdGF0aWMgdm9pZCBpbGtfcGlwZV93 bV9nZXRfaHdfc3RhdGUoc3RydWN0IGRybV9jcnRjICpjcnRjKQo+IC0tIAo+IDIuNy40Cj4gCgot LSAKTWF0dCBSb3BlcgpHcmFwaGljcyBTb2Z0d2FyZSBFbmdpbmVlcgpJb1RHIFBsYXRmb3JtIEVu YWJsaW5nICYgRGV2ZWxvcG1lbnQKSW50ZWwgQ29ycG9yYXRpb24KKDkxNikgMzU2LTI3OTUKX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KSW50ZWwtZ2Z4IG1h aWxpbmcgbGlzdApJbnRlbC1nZnhAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHBzOi8vbGlzdHMu ZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGluZm8vaW50ZWwtZ2Z4Cg== From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755586AbcHBVOE (ORCPT ); Tue, 2 Aug 2016 17:14:04 -0400 Received: from mga03.intel.com ([134.134.136.65]:33098 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754138AbcHBVNw (ORCPT ); Tue, 2 Aug 2016 17:13:52 -0400 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.28,462,1464678000"; d="scan'208";a="1028581931" Date: Tue, 2 Aug 2016 14:05:27 -0700 From: Matt Roper To: Lyude Cc: intel-gfx@lists.freedesktop.org, Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Maarten Lankhorst , 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 v5 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs Message-ID: <20160802210527.GX32025@intel.com> References: <1470163975-30467-1-git-send-email-cpaul@redhat.com> <1470163975-30467-2-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: <1470163975-30467-2-git-send-email-cpaul@redhat.com> User-Agent: Mutt/1.5.20 (2009-06-14) Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Tue, Aug 02, 2016 at 02:52:49PM -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 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 > 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 > > Reviewed-by: Matt Roper > Signed-off-by: Lyude > Cc: Daniel Vetter > Cc: Ville Syrjälä > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 5 ++ > drivers/gpu/drm/i915/intel_display.c | 12 ++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 112 +++++++++++++++++++++++++++++++++++ > 5 files changed, 133 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 65ada5d..87018d3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1962,6 +1962,8 @@ struct drm_i915_private { > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > + bool skl_sagv_enabled; > + > struct { > /* > * Raw watermark latency values: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2f93d4a..5fb1c63 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7170,6 +7170,11 @@ 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_LOW_FREQ 0x1 > +#define GEN9_SAGV_HIGH_FREQ 0x2 > +#define GEN9_SAGV_DYNAMIC_FREQ 0x3 > #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 a8e8cc8..76ba79f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4565,6 +4565,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc->base.state); > struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_plane *primary = crtc->base.primary; > struct drm_plane_state *old_pri_state = > drm_atomic_get_existing_plane_state(old_state, primary); > @@ -4589,6 +4590,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > !old_primary_state->visible)) > intel_post_enable_primary(&crtc->base); > } > + > + if (hweight32(dev_priv->active_crtcs) <= 1) > + skl_enable_sagv(dev_priv); There might be a slightly better place to handle this; see comment below. > } > > static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > @@ -4649,6 +4653,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > } > > /* > + * SKL workaround: bspec recommends we disable the SAGV when we have > + * more then one pipe enabled > + */ > + if (pipe_config->base.active && > + hweight32(dev_priv->active_crtcs | drm_crtc_mask(&crtc->base)) > 1) > + skl_disable_sagv(dev_priv); As Hans pointed out, this doesn't look right. I'm guessing you're trying to guard against the case where there's no intersection between the starting pipe usage and the final pipe usage? E.g., only pipe A active before commit, only pipe B active after commit, and you're worried there might be a brief point where both A and B are on together? I don't think this should really matter since we power everything old-changing down before powering anything new-changing up. So I think all we really need to care about for enabling/disabling is how many CRTC's there are in the final state. It seems like a more natural place to place to handle enable/disable would be in the 'if (intel_state->modeset)' blocks of intel_atomic_commit_tail() (since only a modeset operation could change the number of CRTC's in use to trigger a SAGV toggle). That also has the slight benefit of only getting run once for the atomic transaction rather than once for each CRTC (and you can just do a simple test of hweight(intel_state->active_crtcs) to figure out how many CRTC's are going to be on after the transaction completes. > + > + /* > * If we're doing a modeset, we're done. No need to do any pre-vblank > * watermark programming here. > */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 50cdc89..6b0532a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1709,6 +1709,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 f610b71..68721a5 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2884,6 +2884,116 @@ skl_wm_plane_id(const struct intel_plane *plane) > } > > static void > +skl_sagv_get_hw_state(struct drm_i915_private *dev_priv) > +{ > + u32 temp; > + int ret; > + > + if (IS_BROXTON(dev_priv)) > + return; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_enabled = !(temp & 0x1); > + } else { > + /* > + * If for some reason we can't access the SAGV state, follow > + * the bspec and assume it's enabled > + */ > + DRM_ERROR("Failed to get SAGV state, assuming enabled\n"); > + dev_priv->skl_sagv_enabled = true; > + } > +} > + > +/* > + * 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 (IS_BROXTON(dev_priv)) > + return 0; > + if (dev_priv->skl_sagv_enabled) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Enabling the SAGV\n"); > + > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DYNAMIC_FREQ); > + if (!ret) > + dev_priv->skl_sagv_enabled = true; > + else > + DRM_ERROR("Failed to enable the SAGV\n"); > + > + /* We don't need to wait for SAGV when enabling */ > + mutex_unlock(&dev_priv->rps.hw_lock); > + return ret; > +} > + > +static int > +skl_do_sagv_disable(struct drm_i915_private *dev_priv) > +{ > + int ret; > + uint32_t temp; > + > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DISABLE); > + if (ret) { > + DRM_ERROR("Failed to disable the SAGV\n"); > + return ret; > + } > + > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + &temp); > + if (ret) { > + DRM_ERROR("Failed to check the status of the SAGV\n"); > + return ret; > + } > + > + return temp & 0x1; > +} > + > +int > +skl_disable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret, result; > + > + if (IS_BROXTON(dev_priv)) > + return 0; > + if (!dev_priv->skl_sagv_enabled) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Disabling the SAGV\n"); > + > + /* 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 == -ETIMEDOUT) > + DRM_ERROR("Request to disable SAGV timed out\n"); > + else { Minor style nitpick; if either branch of an if/else needs braces, they both need braces. > + if (result == 1) You've got a few uses of 0x1 as a magic number that are correct, but seem counterintuitive to someone not looking at the bspec (1=off). Maybe replacing these with a #define might help clarify slightly? Matt > + dev_priv->skl_sagv_enabled = false; > + > + ret = result; > + } > + > + return ret; > +} > + > +static void > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > const struct intel_crtc_state *cstate, > struct skl_ddb_entry *alloc, /* out */ > @@ -4236,6 +4346,8 @@ void skl_wm_get_hw_state(struct drm_device *dev) > /* Easy/common case; just sanitize DDB now if everything off */ > memset(ddb, 0, sizeof(*ddb)); > } > + > + skl_sagv_get_hw_state(dev_priv); > } > > static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc) > -- > 2.7.4 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795 From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mga03.intel.com ([134.134.136.65]:33098 "EHLO mga03.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754138AbcHBVNw (ORCPT ); Tue, 2 Aug 2016 17:13:52 -0400 Date: Tue, 2 Aug 2016 14:05:27 -0700 From: Matt Roper To: Lyude Cc: intel-gfx@lists.freedesktop.org, Ville =?iso-8859-1?Q?Syrj=E4l=E4?= , Maarten Lankhorst , 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 v5 1/6] drm/i915/skl: Add support for the SAGV, fix underrun hangs Message-ID: <20160802210527.GX32025@intel.com> References: <1470163975-30467-1-git-send-email-cpaul@redhat.com> <1470163975-30467-2-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: <1470163975-30467-2-git-send-email-cpaul@redhat.com> Sender: stable-owner@vger.kernel.org List-ID: On Tue, Aug 02, 2016 at 02:52:49PM -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 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 > 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 > > Reviewed-by: Matt Roper > Signed-off-by: Lyude > Cc: Daniel Vetter > Cc: Ville Syrj�l� > Cc: stable@vger.kernel.org > --- > drivers/gpu/drm/i915/i915_drv.h | 2 + > drivers/gpu/drm/i915/i915_reg.h | 5 ++ > drivers/gpu/drm/i915/intel_display.c | 12 ++++ > drivers/gpu/drm/i915/intel_drv.h | 2 + > drivers/gpu/drm/i915/intel_pm.c | 112 +++++++++++++++++++++++++++++++++++ > 5 files changed, 133 insertions(+) > > diff --git a/drivers/gpu/drm/i915/i915_drv.h b/drivers/gpu/drm/i915/i915_drv.h > index 65ada5d..87018d3 100644 > --- a/drivers/gpu/drm/i915/i915_drv.h > +++ b/drivers/gpu/drm/i915/i915_drv.h > @@ -1962,6 +1962,8 @@ struct drm_i915_private { > struct i915_suspend_saved_registers regfile; > struct vlv_s0ix_state vlv_s0ix_state; > > + bool skl_sagv_enabled; > + > struct { > /* > * Raw watermark latency values: > diff --git a/drivers/gpu/drm/i915/i915_reg.h b/drivers/gpu/drm/i915/i915_reg.h > index 2f93d4a..5fb1c63 100644 > --- a/drivers/gpu/drm/i915/i915_reg.h > +++ b/drivers/gpu/drm/i915/i915_reg.h > @@ -7170,6 +7170,11 @@ 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_LOW_FREQ 0x1 > +#define GEN9_SAGV_HIGH_FREQ 0x2 > +#define GEN9_SAGV_DYNAMIC_FREQ 0x3 > #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 a8e8cc8..76ba79f 100644 > --- a/drivers/gpu/drm/i915/intel_display.c > +++ b/drivers/gpu/drm/i915/intel_display.c > @@ -4565,6 +4565,7 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > struct intel_crtc_state *pipe_config = > to_intel_crtc_state(crtc->base.state); > struct drm_device *dev = crtc->base.dev; > + struct drm_i915_private *dev_priv = to_i915(dev); > struct drm_plane *primary = crtc->base.primary; > struct drm_plane_state *old_pri_state = > drm_atomic_get_existing_plane_state(old_state, primary); > @@ -4589,6 +4590,9 @@ static void intel_post_plane_update(struct intel_crtc_state *old_crtc_state) > !old_primary_state->visible)) > intel_post_enable_primary(&crtc->base); > } > + > + if (hweight32(dev_priv->active_crtcs) <= 1) > + skl_enable_sagv(dev_priv); There might be a slightly better place to handle this; see comment below. > } > > static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > @@ -4649,6 +4653,14 @@ static void intel_pre_plane_update(struct intel_crtc_state *old_crtc_state) > } > > /* > + * SKL workaround: bspec recommends we disable the SAGV when we have > + * more then one pipe enabled > + */ > + if (pipe_config->base.active && > + hweight32(dev_priv->active_crtcs | drm_crtc_mask(&crtc->base)) > 1) > + skl_disable_sagv(dev_priv); As Hans pointed out, this doesn't look right. I'm guessing you're trying to guard against the case where there's no intersection between the starting pipe usage and the final pipe usage? E.g., only pipe A active before commit, only pipe B active after commit, and you're worried there might be a brief point where both A and B are on together? I don't think this should really matter since we power everything old-changing down before powering anything new-changing up. So I think all we really need to care about for enabling/disabling is how many CRTC's there are in the final state. It seems like a more natural place to place to handle enable/disable would be in the 'if (intel_state->modeset)' blocks of intel_atomic_commit_tail() (since only a modeset operation could change the number of CRTC's in use to trigger a SAGV toggle). That also has the slight benefit of only getting run once for the atomic transaction rather than once for each CRTC (and you can just do a simple test of hweight(intel_state->active_crtcs) to figure out how many CRTC's are going to be on after the transaction completes. > + > + /* > * If we're doing a modeset, we're done. No need to do any pre-vblank > * watermark programming here. > */ > diff --git a/drivers/gpu/drm/i915/intel_drv.h b/drivers/gpu/drm/i915/intel_drv.h > index 50cdc89..6b0532a 100644 > --- a/drivers/gpu/drm/i915/intel_drv.h > +++ b/drivers/gpu/drm/i915/intel_drv.h > @@ -1709,6 +1709,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 f610b71..68721a5 100644 > --- a/drivers/gpu/drm/i915/intel_pm.c > +++ b/drivers/gpu/drm/i915/intel_pm.c > @@ -2884,6 +2884,116 @@ skl_wm_plane_id(const struct intel_plane *plane) > } > > static void > +skl_sagv_get_hw_state(struct drm_i915_private *dev_priv) > +{ > + u32 temp; > + int ret; > + > + if (IS_BROXTON(dev_priv)) > + return; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, &temp); > + mutex_unlock(&dev_priv->rps.hw_lock); > + > + if (!ret) { > + dev_priv->skl_sagv_enabled = !(temp & 0x1); > + } else { > + /* > + * If for some reason we can't access the SAGV state, follow > + * the bspec and assume it's enabled > + */ > + DRM_ERROR("Failed to get SAGV state, assuming enabled\n"); > + dev_priv->skl_sagv_enabled = true; > + } > +} > + > +/* > + * 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 (IS_BROXTON(dev_priv)) > + return 0; > + if (dev_priv->skl_sagv_enabled) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Enabling the SAGV\n"); > + > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DYNAMIC_FREQ); > + if (!ret) > + dev_priv->skl_sagv_enabled = true; > + else > + DRM_ERROR("Failed to enable the SAGV\n"); > + > + /* We don't need to wait for SAGV when enabling */ > + mutex_unlock(&dev_priv->rps.hw_lock); > + return ret; > +} > + > +static int > +skl_do_sagv_disable(struct drm_i915_private *dev_priv) > +{ > + int ret; > + uint32_t temp; > + > + ret = sandybridge_pcode_write(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + GEN9_SAGV_DISABLE); > + if (ret) { > + DRM_ERROR("Failed to disable the SAGV\n"); > + return ret; > + } > + > + ret = sandybridge_pcode_read(dev_priv, GEN9_PCODE_SAGV_CONTROL, > + &temp); > + if (ret) { > + DRM_ERROR("Failed to check the status of the SAGV\n"); > + return ret; > + } > + > + return temp & 0x1; > +} > + > +int > +skl_disable_sagv(struct drm_i915_private *dev_priv) > +{ > + int ret, result; > + > + if (IS_BROXTON(dev_priv)) > + return 0; > + if (!dev_priv->skl_sagv_enabled) > + return 0; > + > + mutex_lock(&dev_priv->rps.hw_lock); > + DRM_DEBUG_KMS("Disabling the SAGV\n"); > + > + /* 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 == -ETIMEDOUT) > + DRM_ERROR("Request to disable SAGV timed out\n"); > + else { Minor style nitpick; if either branch of an if/else needs braces, they both need braces. > + if (result == 1) You've got a few uses of 0x1 as a magic number that are correct, but seem counterintuitive to someone not looking at the bspec (1=off). Maybe replacing these with a #define might help clarify slightly? Matt > + dev_priv->skl_sagv_enabled = false; > + > + ret = result; > + } > + > + return ret; > +} > + > +static void > skl_ddb_get_pipe_allocation_limits(struct drm_device *dev, > const struct intel_crtc_state *cstate, > struct skl_ddb_entry *alloc, /* out */ > @@ -4236,6 +4346,8 @@ void skl_wm_get_hw_state(struct drm_device *dev) > /* Easy/common case; just sanitize DDB now if everything off */ > memset(ddb, 0, sizeof(*ddb)); > } > + > + skl_sagv_get_hw_state(dev_priv); > } > > static void ilk_pipe_wm_get_hw_state(struct drm_crtc *crtc) > -- > 2.7.4 > -- Matt Roper Graphics Software Engineer IoTG Platform Enabling & Development Intel Corporation (916) 356-2795