From mboxrd@z Thu Jan 1 00:00:00 1970 From: John Keeping Subject: Re: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed Date: Wed, 13 Jan 2016 17:39:54 +0000 Message-ID: <20160113173954.52e688fc.john@metanate.com> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@phenom.ffwll.local> <20160113143425.363ce07a.john@metanate.com> <20160113154005.GT19130@phenom.ffwll.local> <20160113155529.6048b2c1.john@metanate.com> <20160113162156.GA19130@phenom.ffwll.local> <20160113164038.5fcf670f.john@metanate.com> <20160113171917.GC19130@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160113171917.GC19130@phenom.ffwll.local> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Daniel Vetter , Mark Yao Cc: linux-arm-kernel@lists.infradead.org, linux-rockchip@lists.infradead.org, linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org List-Id: linux-rockchip.vger.kernel.org T24gV2VkLCAxMyBKYW4gMjAxNiAxODoxOToxNyArMDEwMCwgRGFuaWVsIFZldHRlciB3cm90ZToK Cj4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDQ6NDA6MzhQTSArMDAwMCwgSm9obiBLZWVwaW5n IHdyb3RlOgo+ID4gT24gV2VkLCAxMyBKYW4gMjAxNiAxNzoyMTo1NiArMDEwMCwgRGFuaWVsIFZl dHRlciB3cm90ZToKPiA+ICAgCj4gPiA+IE9uIFdlZCwgSmFuIDEzLCAyMDE2IGF0IDAzOjU1OjI5 UE0gKzAwMDAsIEpvaG4gS2VlcGluZyB3cm90ZTogIAo+ID4gPiA+IE9uIFdlZCwgMTMgSmFuIDIw MTYgMTY6NDA6MDUgKzAxMDAsIERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gPiA+ID4gICAgIAo+ID4g PiA+ID4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDI6MzQ6MjVQTSArMDAwMCwgSm9obiBLZWVw aW5nIHdyb3RlOiAgICAKPiA+ID4gPiA+ID4gT24gV2VkLCAxMyBKYW4gMjAxNiAxNToyMzoyMCAr MDEwMCwgRGFuaWVsIFZldHRlciB3cm90ZToKPiA+ID4gPiA+ID4gICAgICAgCj4gPiA+ID4gPiA+ ID4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMTI6NTM6MzRQTSArMDAwMCwgSm9obiBLZWVwaW5n IHdyb3RlOiAgICAgIAo+ID4gPiA+ID4gPiA+ID4gQXMgY29tbWVudGVkIGluIGRybV9hdG9taWNf aGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKSwgdXNlcnNwYWNlCj4gPiA+ID4gPiA+ID4gPiByZWxp ZXMgb24gY3Vyc29yIGlvY3RscyBiZWluZyB1bnN5bmNlZC4gIENvbnZlcnRpbmcgdGhlIHJvY2tj aGlwCj4gPiA+ID4gPiA+ID4gPiBkcml2ZXIgdG8gYXRvbWljIGhhcyBzaWduaWZpY2FudGx5IGlt cGFjdGVkIGN1cnNvciBwZXJmb3JtYW5jZSBieQo+ID4gPiA+ID4gPiA+ID4gbWFraW5nIGV2ZXJ5 IGN1cnNvciB1cGRhdGUgd2FpdCBmb3IgdmJsYW5rLgo+ID4gPiA+ID4gPiA+ID4gCj4gPiA+ID4g PiA+ID4gPiBCeSBza2lwcGluZyB0aGUgdmJsYW5rIHN5bmMgd2hlbiB0aGUgZnJhbWVidWZmZXIg aGFzIG5vdCBjaGFuZ2VkCj4gPiA+ID4gPiA+ID4gPiAoYXMgaXMgZG9uZSBpbiBkcm1fYXRvbWlj X2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCkpIHdlIGNhbiBhdm9pZAo+ID4gPiA+ID4gPiA+ID4g dGhpcyBmb3IgdGhlIGNvbW1vbiBjYXNlIG9mIG1vdmluZyB0aGUgY3Vyc29yIGFuZCBvbmx5IG5l ZWQgdG8KPiA+ID4gPiA+ID4gPiA+IGRlbGF5IHRoZSBjdXJzb3IgaW9jdGwgd2hlbiB0aGUgY3Vy c29yIGljb24gY2hhbmdlcy4KPiA+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+ID4gSSBvcmln aW5hbGx5IGluc2VydGVkIGEgY2hlY2sgb24gbGVnYWN5X2N1cnNvcl91cGRhdGUgYXMgd2VsbCwg YnV0Cj4gPiA+ID4gPiA+ID4gPiB0aGF0IGNhdXNlZCBhIHN0b3JtIG9mIGlvbW11IHBhZ2UgZmF1 bHRzLiAgSSBkaWRuJ3QgaW52ZXN0aWdhdGUgdGhlCj4gPiA+ID4gPiA+ID4gPiBjYXVzZSBvZiB0 aG9zZSBzaW5jZSB0aGlzIGNoYW5nZSBnaXZlcyBlbm91Z2ggb2YgYSBwZXJmb3JtYW5jZQo+ID4g PiA+ID4gPiA+ID4gaW1wcm92ZW1lbnQgZm9yIG15IHVzZSBjYXNlLgo+ID4gPiA+ID4gPiA+ID4g Cj4gPiA+ID4gPiA+ID4gPiBUaGlzIGlzIFJGQyBiZWNhdXNlIG9mIHRoYXQgYW5kIGJlY2F1c2Ug dGhlIGZyYW1lYnVmZmVyX2NoYW5nZWQoKQo+ID4gPiA+ID4gPiA+ID4gZnVuY3Rpb24gaXMgY29w aWVkIGZyb20gZHJtX2F0b21pY19oZWxwZXIuYyBhcyBhIHF1aWNrIHdheSB0byB0ZXN0Cj4gPiA+ ID4gPiA+ID4gPiB0aGUgcmVzdWx0Lgo+ID4gPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+ID4gPiBT aWduZWQtb2ZmLWJ5OiBKb2huIEtlZXBpbmcgPGpvaG5AbWV0YW5hdGUuY29tPgo+ID4gPiA+ID4g PiA+ID4gLS0tCj4gPiA+ID4gPiA+ID4gPiAgZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tj aGlwX2RybV9mYi5jIHwgMjcKPiA+ID4gPiA+ID4gPiA+ICsrKysrKysrKysrKysrKysrKysrKysr KystLSAxIGZpbGUgY2hhbmdlZCwgMjUgaW5zZXJ0aW9ucygrKSwgMgo+ID4gPiA+ID4gPiA+ID4g ZGVsZXRpb25zKC0pCj4gPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiA+IGRpZmYgLS1naXQg YS9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMKPiA+ID4gPiA+ID4g PiA+IGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jIGluZGV4IGY3 ODQ0ODguLjhmZDk4MjEKPiA+ID4gPiA+ID4gPiA+IDEwMDY0NCAtLS0gYS9kcml2ZXJzL2dwdS9k cm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMKPiA+ID4gPiA+ID4gPiA+ICsrKyBiL2RyaXZl cnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYwo+ID4gPiA+ID4gPiA+ID4gQEAg LTE3Nyw4ICsxNzcsMjggQEAgc3RhdGljIHZvaWQKPiA+ID4gPiA+ID4gPiA+IHJvY2tjaGlwX2Ny dGNfd2FpdF9mb3JfdXBkYXRlKHN0cnVjdCBkcm1fY3J0YyAqY3J0YykKPiA+ID4gPiA+ID4gPiA+ IGNydGNfZnVuY3MtPndhaXRfZm9yX3VwZGF0ZShjcnRjKTsgfQo+ID4gPiA+ID4gPiA+ID4gIAo+ ID4gPiA+ID4gPiA+ID4gK3N0YXRpYyBib29sIGZyYW1lYnVmZmVyX2NoYW5nZWQoc3RydWN0IGRy bV9kZXZpY2UgKmRldiwKPiA+ID4gPiA+ID4gPiA+ICsJCQkJc3RydWN0IGRybV9hdG9taWNfc3Rh dGUgKm9sZF9zdGF0ZSwKPiA+ID4gPiA+ID4gPiA+ICsJCQkJc3RydWN0IGRybV9jcnRjICpjcnRj KQo+ID4gPiA+ID4gPiA+ID4gK3sKPiA+ID4gPiA+ID4gPiA+ICsJc3RydWN0IGRybV9wbGFuZSAq cGxhbmU7Cj4gPiA+ID4gPiA+ID4gPiArCXN0cnVjdCBkcm1fcGxhbmVfc3RhdGUgKm9sZF9wbGFu ZV9zdGF0ZTsKPiA+ID4gPiA+ID4gPiA+ICsJaW50IGk7Cj4gPiA+ID4gPiA+ID4gPiArCj4gPiA+ ID4gPiA+ID4gPiArCWZvcl9lYWNoX3BsYW5lX2luX3N0YXRlKG9sZF9zdGF0ZSwgcGxhbmUsIG9s ZF9wbGFuZV9zdGF0ZSwKPiA+ID4gPiA+ID4gPiA+IGkpIHsKPiA+ID4gPiA+ID4gPiA+ICsJCWlm IChwbGFuZS0+c3RhdGUtPmNydGMgIT0gY3J0YyAmJgo+ID4gPiA+ID4gPiA+ID4gKwkJICAgIG9s ZF9wbGFuZV9zdGF0ZS0+Y3J0YyAhPSBjcnRjKQo+ID4gPiA+ID4gPiA+ID4gKwkJCWNvbnRpbnVl Owo+ID4gPiA+ID4gPiA+ID4gKwo+ID4gPiA+ID4gPiA+ID4gKwkJaWYgKHBsYW5lLT5zdGF0ZS0+ ZmIgIT0gb2xkX3BsYW5lX3N0YXRlLT5mYikKPiA+ID4gPiA+ID4gPiA+ICsJCQlyZXR1cm4gdHJ1 ZTsKPiA+ID4gPiA+ID4gPiA+ICsJfQo+ID4gPiA+ID4gPiA+ID4gKwo+ID4gPiA+ID4gPiA+ID4g KwlyZXR1cm4gZmFsc2U7Cj4gPiA+ID4gPiA+ID4gPiArfSAgICAgICAgCj4gPiA+ID4gPiA+ID4g Cj4gPiA+ID4gPiA+ID4gUGxlYXNlIGRvbid0IGhhbmQtcm9sbCBsb2dpYyB0aGF0IGFmZmVjdHMg c2VtYW50aWNzIGxpa2UgdGhpcy4gSW5zdGVhZAo+ID4gPiA+ID4gPiA+IHBsZWFzZSB1c2UgZHJt X2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpLCB3aGljaCBzaG91bGQgZG8gdGhpcwo+ ID4gPiA+ID4gPiA+IGNvcnJlY3RseSBmb3IgeW91Lgo+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4g PiA+IElmIHRoYXQncyBub3QgdGhlIGNhc2UgdGhlbiB3ZSBuZWVkIHRvIGltcHJvdmUgdGhlIGdl bmVyaWMgaGVscGVyLCBvcgo+ID4gPiA+ID4gPiA+IGZpZ3VyZSBvdXQgd2hhdCdzIGRpZmZlcmVu dCB3aXRoIHJvY2toaXAuICAgICAgCj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiBBY2NvcmRpbmcg dG8gY29tbWl0IDYzZWJiOWYgKGRybS9yb2NrY2hpcDogQ29udmVydCB0byBzdXBwb3J0IGF0b21p Ywo+ID4gPiA+ID4gPiBBUEkpIGl0J3MgYmVjYXVzZSByb2NrY2hpcCBkb2Vzbid0IGhhdmUgYSBo YXJkd2FyZSB2YmxhbmsgY291bnRlci4KPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IEknbSBub3Qg ZW50aXJlbHkgY2xlYXIgb24gd2h5IHRoaXMgcHJldmVudHMgdGhlIHVzZSBvZgo+ID4gPiA+ID4g PiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCkuICAgICAgCj4gPiA+ID4gPiAK PiA+ID4gPiA+IEhtLCB0aGF0IGNvbW1pdCBpc24ndCB0ZXJyaWJseSBoZWxwZnVsLiBJZiB0aGF0 J3MgcmVhbGx5IG5lZWRlZCB0aGVuIGltbyBJCj4gPiA+ID4gPiB0aGluayB3ZSBzaG91bGQgZXh0 cmFjdCBhICJkcm1fYXRvbWljX2hlbHBlcl9wbGFuZV9uZWVkc192Ymxhbmtfd2FpdCgpIgo+ID4g PiA+ID4gaGVscGVyIHRoYXQncyB1c2VkIGJ5IGJvdGguIEJ1dCBzaW5jZSByb2NrY2hpcCBkb2Vz IHZibGFua19nZXQvcHV0IGNhbGxzCj4gPiA+ID4gPiBJJ2QgaG9wZSB2YmxhbmtzIGFjdHVhbGx5 IHdvcmsgY29ycmVjdGx5LiBBbmQgdGhlbiB0aGUgaGVscGVyIHNob3VsZCB3b3JrCj4gPiA+ID4g PiB0b28uICAgIAo+ID4gPiA+IAo+ID4gPiA+IEkgdHJpZWQgc3dpdGNoaW5nIHRoZSBjYWxsIHRv IHJvY2tjaGlwX2NydGNfd2FpdF9mb3JfdXBkYXRlKCkgdG8KPiA+ID4gPiBkcm1fYXRvbWljX2hl bHBlcl93YWl0X2Zvcl92YmxhbmtzKCkgYW5kIGl0IHdvcmtzIGZpbmUgdW50aWwgSSBzd2l0Y2gK PiA+ID4gPiB0aGUgYnVmZmVyIGFzc29jaWF0ZWQgd2l0aCBhIGN1cnNvciwgYXQgd2hpY2ggcG9p bnQgSSBnZXQgaW9tbXUgcGFnZQo+ID4gPiA+IGZhdWx0cywgcHJlc3VtYWJseSBiZWNhdXNlIHRo ZSBHRU0gYnVmZmVyIGlzIHVucmVmZXJlbmNlZCB0b28gZWFybHkuCj4gPiA+ID4gCj4gPiA+ID4g QUZBSUNUIHRoZSBidWZmZXIgd2lsbCBiZSByZWxlYXNlZCB2aWEgZHJtX2F0b21pY19zdGF0ZV9m cmVlKCkKPiA+ID4gPiB1bmNvbmRpdGlvbmFsbHksIGJ1dCBJIHN1c3BlY3QgSSdtIG1pc3Npbmcg c29tZXRoaW5nIHNpbmNlIHRoYXQgd291bGQKPiA+ID4gPiBtZWFuIGV2ZXJ5IGRyaXZlciB3b3Vs ZCBoaXQgYSBzaW1pbGFyIHByb2JsZW0uICAgIAo+ID4gPiAKPiA+ID4gWWVhaCwgd2l0aCB0aGUg aGVscGVyIHdlIGFsd2F5cyBza2lwLCB3aGljaCBtZWFucyB3aGVuIHRoZSBjdXJzb3IgYm8KPiA+ ID4gY2hhbmdlcyB5b3UgaW5kZWVkIHVubWFwIHRvbyBlYXJseS4gU28gY2FuJ3QgZXZlbiBzaGFy ZSB0aGUgb3ZlcmFsbAo+ID4gPiBjb25kaXRpb24sIGJ1dCB3ZSBjb3VsZCBkZWZpbml0ZWx5IHNo YXJlIHRoZSBsaXR0bGUgZnJhbWVidWZmZXJfY2hhbmdlZAo+ID4gPiBoZWxwZXIuICAKPiA+IAo+ ID4gVGhhdCBsZWF2ZXMgbWUgd2l0aCB0aGUgcXVlc3Rpb246IHdoeSBkbyBvdGhlciBhdG9taWMg ZHJpdmVycyB3b3JrPwo+ID4gCj4gPiBJZiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92Ymxh bmtzKCkgc2tpcHBpbmcgdmJsYW5rcyByZXN1bHRzIGluIHRoZQo+ID4gY3Vyc29yIGJvIGJlaW5n IHVubWFwcGVkIHRvbyBlYXJseSBmb3Igcm9ja2NoaXAsIHdoeSBpcyBpdCBub3QgdW5tYXBwZWQK PiA+IHRvbyBlYXJseSBmb3IgYWxsIG9mIHRoZSBvdGhlciBkcml2ZXJzIHVzaW5nIHRoYXQgaGVs cGVyPyAgCj4gCj4gSXQncyB1bm1hcHBlZCB0b28gZWFybHkgZm9yIGV2ZXJ5b25lLCBpdCdzIGp1 c3QgdGhhdCBub3JtYWxseSB0aGF0IGRvZXNuJ3QKPiByZXN1bHQgaW4gYSBmaXJld29ya3Mgc2hv dy4gV2hhdCB3ZSBtYXliZSBjb3VsZC9zaG91bGQgZG8gaXMgZG8gdGhlCj4gdW5tYXBwaW5nIGFz eW5jaHJvbm91c2x5LCBidXQgdGhhdCBydW5zIGludG8gdGhlIG92ZXJhbGwgImN1cnJlbnQgYXRv bWljCj4gaGVscGVycyBkb24ndCBkbyBhc3luYyB5ZXQiIHByb2JsZW0uIE1pZ2h0IGJlIGEgZ29v ZCBwb2ludCB0byBzdGFydCBmaXhpbmcKPiB0aGlzIHVwIHRob3VnaC4KCk9LLCB0aGFua3MsIEkg dGhpbmsgSSdtIGJlZ2lubmluZyB0byB1bmRlcnN0YW5kIGhvdyB0aGlzIGFsbCBmaXRzCnRvZ2V0 aGVyLgoKSXQgbG9va3MgbGlrZSB0aGVyZSBhcmUgdHdvIG9wdGlvbnMgZm9yIG1lIHRvIGdldCBy ZWFzb25hYmxlIGN1cnNvcgpwZXJmb3JtYW5jZSBvbiByb2NrY2hpcCBpbiB0aGUgc2hvcnQgdGVy bToKCjEpIEV4cG9ydCB0aGUgY3VycmVudCBmcmFtZWJ1ZmZlcl9jaGFuZ2VkKCkgZnVuY3Rpb24g YXMKICAgZHJtX2F0b21pY19oZWxwZXJfZnJhbWVidWZmZXJfY2hhbmdlZCgpIGFuZCB1c2UgaXQg aW4KICAgcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91cGRhdGUoKS4KCjIpIEFkZCBhIG1lY2hhbmlz bSB0byBzdXBwcmVzcyB0aGUgbGVnYWN5X2N1cnNvcl91cGRhdGUgY2hlY2sgaW4KICAgZHJtX2F0 b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpIGFuZCBzd2l0Y2ggdGhlIHJvY2tjaGlwIGRy aXZlcgogICBvdmVyIHRvIGl0LgoKSW4gYm90aCBvZiB0aGVzZSBjYXNlcyB3ZSdyZSBvbmx5IHJl c3RvcmluZyB0aGUgdW5zeW5jZWQgY3Vyc29yIGlvY3RscwpiZWhhdmlvdXIgd2hlbiB0aGUgY3Vy c29yIGlzIG1vdmVkIGJ1dCBpdCB3aWxsIHN0aWxsIGJlIGV4cGVuc2l2ZSB3aGVuCnRoZSBjdXJz b3IgYm8gY2hhbmdlcy4gIFRoYXQgZ2l2ZXMgc3VmZmljaWVudCBwZXJmb3JtYW5jZSBpbiBteSB0 ZXN0aW5nLgpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpk cmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0 cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Wed, 13 Jan 2016 17:39:54 +0000 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <20160113171917.GC19130@phenom.ffwll.local> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@phenom.ffwll.local> <20160113143425.363ce07a.john@metanate.com> <20160113154005.GT19130@phenom.ffwll.local> <20160113155529.6048b2c1.john@metanate.com> <20160113162156.GA19130@phenom.ffwll.local> <20160113164038.5fcf670f.john@metanate.com> <20160113171917.GC19130@phenom.ffwll.local> Message-ID: <20160113173954.52e688fc.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote: > On Wed, Jan 13, 2016 at 04:40:38PM +0000, John Keeping wrote: > > On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote: > > > > > On Wed, Jan 13, 2016 at 03:55:29PM +0000, John Keeping wrote: > > > > On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote: > > > > > > > > > On Wed, Jan 13, 2016 at 02:34:25PM +0000, John Keeping wrote: > > > > > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote: > > > > > > > > > > > > > On Wed, Jan 13, 2016 at 12:53:34PM +0000, John Keeping wrote: > > > > > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace > > > > > > > > relies on cursor ioctls being unsynced. Converting the rockchip > > > > > > > > driver to atomic has significantly impacted cursor performance by > > > > > > > > making every cursor update wait for vblank. > > > > > > > > > > > > > > > > By skipping the vblank sync when the framebuffer has not changed > > > > > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid > > > > > > > > this for the common case of moving the cursor and only need to > > > > > > > > delay the cursor ioctl when the cursor icon changes. > > > > > > > > > > > > > > > > I originally inserted a check on legacy_cursor_update as well, but > > > > > > > > that caused a storm of iommu page faults. I didn't investigate the > > > > > > > > cause of those since this change gives enough of a performance > > > > > > > > improvement for my use case. > > > > > > > > > > > > > > > > This is RFC because of that and because the framebuffer_changed() > > > > > > > > function is copied from drm_atomic_helper.c as a quick way to test > > > > > > > > the result. > > > > > > > > > > > > > > > > Signed-off-by: John Keeping > > > > > > > > --- > > > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 > > > > > > > > +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 > > > > > > > > deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 > > > > > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > @@ -177,8 +177,28 @@ static void > > > > > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc) > > > > > > > > crtc_funcs->wait_for_update(crtc); } > > > > > > > > > > > > > > > > +static bool framebuffer_changed(struct drm_device *dev, > > > > > > > > + struct drm_atomic_state *old_state, > > > > > > > > + struct drm_crtc *crtc) > > > > > > > > +{ > > > > > > > > + struct drm_plane *plane; > > > > > > > > + struct drm_plane_state *old_plane_state; > > > > > > > > + int i; > > > > > > > > + > > > > > > > > + for_each_plane_in_state(old_state, plane, old_plane_state, > > > > > > > > i) { > > > > > > > > + if (plane->state->crtc != crtc && > > > > > > > > + old_plane_state->crtc != crtc) > > > > > > > > + continue; > > > > > > > > + > > > > > > > > + if (plane->state->fb != old_plane_state->fb) > > > > > > > > + return true; > > > > > > > > + } > > > > > > > > + > > > > > > > > + return false; > > > > > > > > +} > > > > > > > > > > > > > > Please don't hand-roll logic that affects semantics like this. Instead > > > > > > > please use drm_atomic_helper_wait_for_vblanks(), which should do this > > > > > > > correctly for you. > > > > > > > > > > > > > > If that's not the case then we need to improve the generic helper, or > > > > > > > figure out what's different with rockhip. > > > > > > > > > > > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic > > > > > > API) it's because rockchip doesn't have a hardware vblank counter. > > > > > > > > > > > > I'm not entirely clear on why this prevents the use of > > > > > > drm_atomic_helper_wait_for_vblanks(). > > > > > > > > > > Hm, that commit isn't terribly helpful. If that's really needed then imo I > > > > > think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" > > > > > helper that's used by both. But since rockchip does vblank_get/put calls > > > > > I'd hope vblanks actually work correctly. And then the helper should work > > > > > too. > > > > > > > > I tried switching the call to rockchip_crtc_wait_for_update() to > > > > drm_atomic_helper_wait_for_vblanks() and it works fine until I switch > > > > the buffer associated with a cursor, at which point I get iommu page > > > > faults, presumably because the GEM buffer is unreferenced too early. > > > > > > > > AFAICT the buffer will be released via drm_atomic_state_free() > > > > unconditionally, but I suspect I'm missing something since that would > > > > mean every driver would hit a similar problem. > > > > > > Yeah, with the helper we always skip, which means when the cursor bo > > > changes you indeed unmap too early. So can't even share the overall > > > condition, but we could definitely share the little framebuffer_changed > > > helper. > > > > That leaves me with the question: why do other atomic drivers work? > > > > If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the > > cursor bo being unmapped too early for rockchip, why is it not unmapped > > too early for all of the other drivers using that helper? > > It's unmapped too early for everyone, it's just that normally that doesn't > result in a fireworks show. What we maybe could/should do is do the > unmapping asynchronously, but that runs into the overall "current atomic > helpers don't do async yet" problem. Might be a good point to start fixing > this up though. OK, thanks, I think I'm beginning to understand how this all fits together. It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term: 1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update(). 2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it. In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932400AbcAMRkc (ORCPT ); Wed, 13 Jan 2016 12:40:32 -0500 Received: from dougal.metanate.com ([90.155.101.14]:33245 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S932374AbcAMRkb (ORCPT ); Wed, 13 Jan 2016 12:40:31 -0500 Date: Wed, 13 Jan 2016 17:39:54 +0000 From: John Keeping To: Daniel Vetter , Mark Yao Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org Subject: Re: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed Message-ID: <20160113173954.52e688fc.john@metanate.com> In-Reply-To: <20160113171917.GC19130@phenom.ffwll.local> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@phenom.ffwll.local> <20160113143425.363ce07a.john@metanate.com> <20160113154005.GT19130@phenom.ffwll.local> <20160113155529.6048b2c1.john@metanate.com> <20160113162156.GA19130@phenom.ffwll.local> <20160113164038.5fcf670f.john@metanate.com> <20160113171917.GC19130@phenom.ffwll.local> Organization: Metanate Ltd X-Mailer: Claws Mail 3.13.1 (GTK+ 2.24.29; x86_64-unknown-linux-gnu) MIME-Version: 1.0 Content-Type: text/plain; charset=US-ASCII Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, 13 Jan 2016 18:19:17 +0100, Daniel Vetter wrote: > On Wed, Jan 13, 2016 at 04:40:38PM +0000, John Keeping wrote: > > On Wed, 13 Jan 2016 17:21:56 +0100, Daniel Vetter wrote: > > > > > On Wed, Jan 13, 2016 at 03:55:29PM +0000, John Keeping wrote: > > > > On Wed, 13 Jan 2016 16:40:05 +0100, Daniel Vetter wrote: > > > > > > > > > On Wed, Jan 13, 2016 at 02:34:25PM +0000, John Keeping wrote: > > > > > > On Wed, 13 Jan 2016 15:23:20 +0100, Daniel Vetter wrote: > > > > > > > > > > > > > On Wed, Jan 13, 2016 at 12:53:34PM +0000, John Keeping wrote: > > > > > > > > As commented in drm_atomic_helper_wait_for_vblanks(), userspace > > > > > > > > relies on cursor ioctls being unsynced. Converting the rockchip > > > > > > > > driver to atomic has significantly impacted cursor performance by > > > > > > > > making every cursor update wait for vblank. > > > > > > > > > > > > > > > > By skipping the vblank sync when the framebuffer has not changed > > > > > > > > (as is done in drm_atomic_helper_wait_for_vblanks()) we can avoid > > > > > > > > this for the common case of moving the cursor and only need to > > > > > > > > delay the cursor ioctl when the cursor icon changes. > > > > > > > > > > > > > > > > I originally inserted a check on legacy_cursor_update as well, but > > > > > > > > that caused a storm of iommu page faults. I didn't investigate the > > > > > > > > cause of those since this change gives enough of a performance > > > > > > > > improvement for my use case. > > > > > > > > > > > > > > > > This is RFC because of that and because the framebuffer_changed() > > > > > > > > function is copied from drm_atomic_helper.c as a quick way to test > > > > > > > > the result. > > > > > > > > > > > > > > > > Signed-off-by: John Keeping > > > > > > > > --- > > > > > > > > drivers/gpu/drm/rockchip/rockchip_drm_fb.c | 27 > > > > > > > > +++++++++++++++++++++++++-- 1 file changed, 25 insertions(+), 2 > > > > > > > > deletions(-) > > > > > > > > > > > > > > > > diff --git a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c index f784488..8fd9821 > > > > > > > > 100644 --- a/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > +++ b/drivers/gpu/drm/rockchip/rockchip_drm_fb.c > > > > > > > > @@ -177,8 +177,28 @@ static void > > > > > > > > rockchip_crtc_wait_for_update(struct drm_crtc *crtc) > > > > > > > > crtc_funcs->wait_for_update(crtc); } > > > > > > > > > > > > > > > > +static bool framebuffer_changed(struct drm_device *dev, > > > > > > > > + struct drm_atomic_state *old_state, > > > > > > > > + struct drm_crtc *crtc) > > > > > > > > +{ > > > > > > > > + struct drm_plane *plane; > > > > > > > > + struct drm_plane_state *old_plane_state; > > > > > > > > + int i; > > > > > > > > + > > > > > > > > + for_each_plane_in_state(old_state, plane, old_plane_state, > > > > > > > > i) { > > > > > > > > + if (plane->state->crtc != crtc && > > > > > > > > + old_plane_state->crtc != crtc) > > > > > > > > + continue; > > > > > > > > + > > > > > > > > + if (plane->state->fb != old_plane_state->fb) > > > > > > > > + return true; > > > > > > > > + } > > > > > > > > + > > > > > > > > + return false; > > > > > > > > +} > > > > > > > > > > > > > > Please don't hand-roll logic that affects semantics like this. Instead > > > > > > > please use drm_atomic_helper_wait_for_vblanks(), which should do this > > > > > > > correctly for you. > > > > > > > > > > > > > > If that's not the case then we need to improve the generic helper, or > > > > > > > figure out what's different with rockhip. > > > > > > > > > > > > According to commit 63ebb9f (drm/rockchip: Convert to support atomic > > > > > > API) it's because rockchip doesn't have a hardware vblank counter. > > > > > > > > > > > > I'm not entirely clear on why this prevents the use of > > > > > > drm_atomic_helper_wait_for_vblanks(). > > > > > > > > > > Hm, that commit isn't terribly helpful. If that's really needed then imo I > > > > > think we should extract a "drm_atomic_helper_plane_needs_vblank_wait()" > > > > > helper that's used by both. But since rockchip does vblank_get/put calls > > > > > I'd hope vblanks actually work correctly. And then the helper should work > > > > > too. > > > > > > > > I tried switching the call to rockchip_crtc_wait_for_update() to > > > > drm_atomic_helper_wait_for_vblanks() and it works fine until I switch > > > > the buffer associated with a cursor, at which point I get iommu page > > > > faults, presumably because the GEM buffer is unreferenced too early. > > > > > > > > AFAICT the buffer will be released via drm_atomic_state_free() > > > > unconditionally, but I suspect I'm missing something since that would > > > > mean every driver would hit a similar problem. > > > > > > Yeah, with the helper we always skip, which means when the cursor bo > > > changes you indeed unmap too early. So can't even share the overall > > > condition, but we could definitely share the little framebuffer_changed > > > helper. > > > > That leaves me with the question: why do other atomic drivers work? > > > > If drm_atomic_helper_wait_for_vblanks() skipping vblanks results in the > > cursor bo being unmapped too early for rockchip, why is it not unmapped > > too early for all of the other drivers using that helper? > > It's unmapped too early for everyone, it's just that normally that doesn't > result in a fireworks show. What we maybe could/should do is do the > unmapping asynchronously, but that runs into the overall "current atomic > helpers don't do async yet" problem. Might be a good point to start fixing > this up though. OK, thanks, I think I'm beginning to understand how this all fits together. It looks like there are two options for me to get reasonable cursor performance on rockchip in the short term: 1) Export the current framebuffer_changed() function as drm_atomic_helper_framebuffer_changed() and use it in rockchip_crtc_wait_for_update(). 2) Add a mechanism to suppress the legacy_cursor_update check in drm_atomic_helper_wait_for_vblanks() and switch the rockchip driver over to it. In both of these cases we're only restoring the unsynced cursor ioctls behaviour when the cursor is moved but it will still be expensive when the cursor bo changes. That gives sufficient performance in my testing.