From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed Date: Wed, 13 Jan 2016 18:19:17 +0100 Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20160113164038.5fcf670f.john@metanate.com> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: John Keeping Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org List-Id: linux-rockchip.vger.kernel.org T24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDQ6NDA6MzhQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdy b3RlOgo+IE9uIFdlZCwgMTMgSmFuIDIwMTYgMTc6MjE6NTYgKzAxMDAsIERhbmllbCBWZXR0ZXIg d3JvdGU6Cj4gCj4gPiBPbiBXZWQsIEphbiAxMywgMjAxNiBhdCAwMzo1NToyOVBNICswMDAwLCBK b2huIEtlZXBpbmcgd3JvdGU6Cj4gPiA+IE9uIFdlZCwgMTMgSmFuIDIwMTYgMTY6NDA6MDUgKzAx MDAsIERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gPiA+ICAgCj4gPiA+ID4gT24gV2VkLCBKYW4gMTMs IDIwMTYgYXQgMDI6MzQ6MjVQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdyb3RlOiAgCj4gPiA+ID4g PiBPbiBXZWQsIDEzIEphbiAyMDE2IDE1OjIzOjIwICswMTAwLCBEYW5pZWwgVmV0dGVyIHdyb3Rl Ogo+ID4gPiA+ID4gICAgIAo+ID4gPiA+ID4gPiBPbiBXZWQsIEphbiAxMywgMjAxNiBhdCAxMjo1 MzozNFBNICswMDAwLCBKb2huIEtlZXBpbmcgd3JvdGU6ICAgIAo+ID4gPiA+ID4gPiA+IEFzIGNv bW1lbnRlZCBpbiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCksIHVzZXJzcGFj ZQo+ID4gPiA+ID4gPiA+IHJlbGllcyBvbiBjdXJzb3IgaW9jdGxzIGJlaW5nIHVuc3luY2VkLiAg Q29udmVydGluZyB0aGUgcm9ja2NoaXAKPiA+ID4gPiA+ID4gPiBkcml2ZXIgdG8gYXRvbWljIGhh cyBzaWduaWZpY2FudGx5IGltcGFjdGVkIGN1cnNvciBwZXJmb3JtYW5jZSBieQo+ID4gPiA+ID4g PiA+IG1ha2luZyBldmVyeSBjdXJzb3IgdXBkYXRlIHdhaXQgZm9yIHZibGFuay4KPiA+ID4gPiA+ ID4gPiAKPiA+ID4gPiA+ID4gPiBCeSBza2lwcGluZyB0aGUgdmJsYW5rIHN5bmMgd2hlbiB0aGUg ZnJhbWVidWZmZXIgaGFzIG5vdCBjaGFuZ2VkCj4gPiA+ID4gPiA+ID4gKGFzIGlzIGRvbmUgaW4g ZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpKSB3ZSBjYW4gYXZvaWQKPiA+ID4g PiA+ID4gPiB0aGlzIGZvciB0aGUgY29tbW9uIGNhc2Ugb2YgbW92aW5nIHRoZSBjdXJzb3IgYW5k IG9ubHkgbmVlZCB0bwo+ID4gPiA+ID4gPiA+IGRlbGF5IHRoZSBjdXJzb3IgaW9jdGwgd2hlbiB0 aGUgY3Vyc29yIGljb24gY2hhbmdlcy4KPiA+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gPiBJIG9y aWdpbmFsbHkgaW5zZXJ0ZWQgYSBjaGVjayBvbiBsZWdhY3lfY3Vyc29yX3VwZGF0ZSBhcyB3ZWxs LCBidXQKPiA+ID4gPiA+ID4gPiB0aGF0IGNhdXNlZCBhIHN0b3JtIG9mIGlvbW11IHBhZ2UgZmF1 bHRzLiAgSSBkaWRuJ3QgaW52ZXN0aWdhdGUgdGhlCj4gPiA+ID4gPiA+ID4gY2F1c2Ugb2YgdGhv c2Ugc2luY2UgdGhpcyBjaGFuZ2UgZ2l2ZXMgZW5vdWdoIG9mIGEgcGVyZm9ybWFuY2UKPiA+ID4g PiA+ID4gPiBpbXByb3ZlbWVudCBmb3IgbXkgdXNlIGNhc2UuCj4gPiA+ID4gPiA+ID4gCj4gPiA+ ID4gPiA+ID4gVGhpcyBpcyBSRkMgYmVjYXVzZSBvZiB0aGF0IGFuZCBiZWNhdXNlIHRoZSBmcmFt ZWJ1ZmZlcl9jaGFuZ2VkKCkKPiA+ID4gPiA+ID4gPiBmdW5jdGlvbiBpcyBjb3BpZWQgZnJvbSBk cm1fYXRvbWljX2hlbHBlci5jIGFzIGEgcXVpY2sgd2F5IHRvIHRlc3QKPiA+ID4gPiA+ID4gPiB0 aGUgcmVzdWx0Lgo+ID4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEpv aG4gS2VlcGluZyA8am9obkBtZXRhbmF0ZS5jb20+Cj4gPiA+ID4gPiA+ID4gLS0tCj4gPiA+ID4g PiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYyB8IDI3Cj4g PiA+ID4gPiA+ID4gKysrKysrKysrKysrKysrKysrKysrKysrKy0tIDEgZmlsZSBjaGFuZ2VkLCAy NSBpbnNlcnRpb25zKCspLCAyCj4gPiA+ID4gPiA+ID4gZGVsZXRpb25zKC0pCj4gPiA+ID4gPiA+ ID4gCj4gPiA+ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9y b2NrY2hpcF9kcm1fZmIuYwo+ID4gPiA+ID4gPiA+IGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlw L3JvY2tjaGlwX2RybV9mYi5jIGluZGV4IGY3ODQ0ODguLjhmZDk4MjEKPiA+ID4gPiA+ID4gPiAx MDA2NDQgLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jCj4g PiA+ID4gPiA+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9m Yi5jCj4gPiA+ID4gPiA+ID4gQEAgLTE3Nyw4ICsxNzcsMjggQEAgc3RhdGljIHZvaWQKPiA+ID4g PiA+ID4gPiByb2NrY2hpcF9jcnRjX3dhaXRfZm9yX3VwZGF0ZShzdHJ1Y3QgZHJtX2NydGMgKmNy dGMpCj4gPiA+ID4gPiA+ID4gY3J0Y19mdW5jcy0+d2FpdF9mb3JfdXBkYXRlKGNydGMpOyB9Cj4g PiA+ID4gPiA+ID4gIAo+ID4gPiA+ID4gPiA+ICtzdGF0aWMgYm9vbCBmcmFtZWJ1ZmZlcl9jaGFu Z2VkKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYsCj4gPiA+ID4gPiA+ID4gKwkJCQlzdHJ1Y3QgZHJt X2F0b21pY19zdGF0ZSAqb2xkX3N0YXRlLAo+ID4gPiA+ID4gPiA+ICsJCQkJc3RydWN0IGRybV9j cnRjICpjcnRjKQo+ID4gPiA+ID4gPiA+ICt7Cj4gPiA+ID4gPiA+ID4gKwlzdHJ1Y3QgZHJtX3Bs YW5lICpwbGFuZTsKPiA+ID4gPiA+ID4gPiArCXN0cnVjdCBkcm1fcGxhbmVfc3RhdGUgKm9sZF9w bGFuZV9zdGF0ZTsKPiA+ID4gPiA+ID4gPiArCWludCBpOwo+ID4gPiA+ID4gPiA+ICsKPiA+ID4g PiA+ID4gPiArCWZvcl9lYWNoX3BsYW5lX2luX3N0YXRlKG9sZF9zdGF0ZSwgcGxhbmUsIG9sZF9w bGFuZV9zdGF0ZSwKPiA+ID4gPiA+ID4gPiBpKSB7Cj4gPiA+ID4gPiA+ID4gKwkJaWYgKHBsYW5l LT5zdGF0ZS0+Y3J0YyAhPSBjcnRjICYmCj4gPiA+ID4gPiA+ID4gKwkJICAgIG9sZF9wbGFuZV9z dGF0ZS0+Y3J0YyAhPSBjcnRjKQo+ID4gPiA+ID4gPiA+ICsJCQljb250aW51ZTsKPiA+ID4gPiA+ ID4gPiArCj4gPiA+ID4gPiA+ID4gKwkJaWYgKHBsYW5lLT5zdGF0ZS0+ZmIgIT0gb2xkX3BsYW5l X3N0YXRlLT5mYikKPiA+ID4gPiA+ID4gPiArCQkJcmV0dXJuIHRydWU7Cj4gPiA+ID4gPiA+ID4g Kwl9Cj4gPiA+ID4gPiA+ID4gKwo+ID4gPiA+ID4gPiA+ICsJcmV0dXJuIGZhbHNlOwo+ID4gPiA+ ID4gPiA+ICt9ICAgICAgCj4gPiA+ID4gPiA+IAo+ID4gPiA+ID4gPiBQbGVhc2UgZG9uJ3QgaGFu ZC1yb2xsIGxvZ2ljIHRoYXQgYWZmZWN0cyBzZW1hbnRpY3MgbGlrZSB0aGlzLiBJbnN0ZWFkCj4g PiA+ID4gPiA+IHBsZWFzZSB1c2UgZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygp LCB3aGljaCBzaG91bGQgZG8gdGhpcwo+ID4gPiA+ID4gPiBjb3JyZWN0bHkgZm9yIHlvdS4KPiA+ ID4gPiA+ID4gCj4gPiA+ID4gPiA+IElmIHRoYXQncyBub3QgdGhlIGNhc2UgdGhlbiB3ZSBuZWVk IHRvIGltcHJvdmUgdGhlIGdlbmVyaWMgaGVscGVyLCBvcgo+ID4gPiA+ID4gPiBmaWd1cmUgb3V0 IHdoYXQncyBkaWZmZXJlbnQgd2l0aCByb2NraGlwLiAgICAKPiA+ID4gPiA+IAo+ID4gPiA+ID4g QWNjb3JkaW5nIHRvIGNvbW1pdCA2M2ViYjlmIChkcm0vcm9ja2NoaXA6IENvbnZlcnQgdG8gc3Vw cG9ydCBhdG9taWMKPiA+ID4gPiA+IEFQSSkgaXQncyBiZWNhdXNlIHJvY2tjaGlwIGRvZXNuJ3Qg aGF2ZSBhIGhhcmR3YXJlIHZibGFuayBjb3VudGVyLgo+ID4gPiA+ID4gCj4gPiA+ID4gPiBJJ20g bm90IGVudGlyZWx5IGNsZWFyIG9uIHdoeSB0aGlzIHByZXZlbnRzIHRoZSB1c2Ugb2YKPiA+ID4g PiA+IGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKS4gICAgCj4gPiA+ID4gCj4g PiA+ID4gSG0sIHRoYXQgY29tbWl0IGlzbid0IHRlcnJpYmx5IGhlbHBmdWwuIElmIHRoYXQncyBy ZWFsbHkgbmVlZGVkIHRoZW4gaW1vIEkKPiA+ID4gPiB0aGluayB3ZSBzaG91bGQgZXh0cmFjdCBh ICJkcm1fYXRvbWljX2hlbHBlcl9wbGFuZV9uZWVkc192Ymxhbmtfd2FpdCgpIgo+ID4gPiA+IGhl bHBlciB0aGF0J3MgdXNlZCBieSBib3RoLiBCdXQgc2luY2Ugcm9ja2NoaXAgZG9lcyB2Ymxhbmtf Z2V0L3B1dCBjYWxscwo+ID4gPiA+IEknZCBob3BlIHZibGFua3MgYWN0dWFsbHkgd29yayBjb3Jy ZWN0bHkuIEFuZCB0aGVuIHRoZSBoZWxwZXIgc2hvdWxkIHdvcmsKPiA+ID4gPiB0b28uICAKPiA+ ID4gCj4gPiA+IEkgdHJpZWQgc3dpdGNoaW5nIHRoZSBjYWxsIHRvIHJvY2tjaGlwX2NydGNfd2Fp dF9mb3JfdXBkYXRlKCkgdG8KPiA+ID4gZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5r cygpIGFuZCBpdCB3b3JrcyBmaW5lIHVudGlsIEkgc3dpdGNoCj4gPiA+IHRoZSBidWZmZXIgYXNz b2NpYXRlZCB3aXRoIGEgY3Vyc29yLCBhdCB3aGljaCBwb2ludCBJIGdldCBpb21tdSBwYWdlCj4g PiA+IGZhdWx0cywgcHJlc3VtYWJseSBiZWNhdXNlIHRoZSBHRU0gYnVmZmVyIGlzIHVucmVmZXJl bmNlZCB0b28gZWFybHkuCj4gPiA+IAo+ID4gPiBBRkFJQ1QgdGhlIGJ1ZmZlciB3aWxsIGJlIHJl bGVhc2VkIHZpYSBkcm1fYXRvbWljX3N0YXRlX2ZyZWUoKQo+ID4gPiB1bmNvbmRpdGlvbmFsbHks IGJ1dCBJIHN1c3BlY3QgSSdtIG1pc3Npbmcgc29tZXRoaW5nIHNpbmNlIHRoYXQgd291bGQKPiA+ ID4gbWVhbiBldmVyeSBkcml2ZXIgd291bGQgaGl0IGEgc2ltaWxhciBwcm9ibGVtLiAgCj4gPiAK PiA+IFllYWgsIHdpdGggdGhlIGhlbHBlciB3ZSBhbHdheXMgc2tpcCwgd2hpY2ggbWVhbnMgd2hl biB0aGUgY3Vyc29yIGJvCj4gPiBjaGFuZ2VzIHlvdSBpbmRlZWQgdW5tYXAgdG9vIGVhcmx5LiBT byBjYW4ndCBldmVuIHNoYXJlIHRoZSBvdmVyYWxsCj4gPiBjb25kaXRpb24sIGJ1dCB3ZSBjb3Vs ZCBkZWZpbml0ZWx5IHNoYXJlIHRoZSBsaXR0bGUgZnJhbWVidWZmZXJfY2hhbmdlZAo+ID4gaGVs cGVyLgo+IAo+IFRoYXQgbGVhdmVzIG1lIHdpdGggdGhlIHF1ZXN0aW9uOiB3aHkgZG8gb3RoZXIg YXRvbWljIGRyaXZlcnMgd29yaz8KPiAKPiBJZiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92 YmxhbmtzKCkgc2tpcHBpbmcgdmJsYW5rcyByZXN1bHRzIGluIHRoZQo+IGN1cnNvciBibyBiZWlu ZyB1bm1hcHBlZCB0b28gZWFybHkgZm9yIHJvY2tjaGlwLCB3aHkgaXMgaXQgbm90IHVubWFwcGVk Cj4gdG9vIGVhcmx5IGZvciBhbGwgb2YgdGhlIG90aGVyIGRyaXZlcnMgdXNpbmcgdGhhdCBoZWxw ZXI/CgpJdCdzIHVubWFwcGVkIHRvbyBlYXJseSBmb3IgZXZlcnlvbmUsIGl0J3MganVzdCB0aGF0 IG5vcm1hbGx5IHRoYXQgZG9lc24ndApyZXN1bHQgaW4gYSBmaXJld29ya3Mgc2hvdy4gV2hhdCB3 ZSBtYXliZSBjb3VsZC9zaG91bGQgZG8gaXMgZG8gdGhlCnVubWFwcGluZyBhc3luY2hyb25vdXNs eSwgYnV0IHRoYXQgcnVucyBpbnRvIHRoZSBvdmVyYWxsICJjdXJyZW50IGF0b21pYwpoZWxwZXJz IGRvbid0IGRvIGFzeW5jIHlldCIgcHJvYmxlbS4gTWlnaHQgYmUgYSBnb29kIHBvaW50IHRvIHN0 YXJ0IGZpeGluZwp0aGlzIHVwIHRob3VnaC4KLURhbmllbAotLSAKRGFuaWVsIFZldHRlcgpTb2Z0 d2FyZSBFbmdpbmVlciwgSW50ZWwgQ29ycG9yYXRpb24KaHR0cDovL2Jsb2cuZmZ3bGwuY2gKX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJpLWRldmVsIG1h aWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6Ly9saXN0cy5m cmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Wed, 13 Jan 2016 18:19:17 +0100 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <20160113164038.5fcf670f.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> Message-ID: <20160113171917.GC19130@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S932146AbcAMRTW (ORCPT ); Wed, 13 Jan 2016 12:19:22 -0500 Received: from mail-wm0-f52.google.com ([74.125.82.52]:32802 "EHLO mail-wm0-f52.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754487AbcAMRTQ (ORCPT ); Wed, 13 Jan 2016 12:19:16 -0500 Date: Wed, 13 Jan 2016 18:19:17 +0100 From: Daniel Vetter To: John Keeping Cc: Daniel Vetter , Mark Yao , 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: <20160113171917.GC19130@phenom.ffwll.local> Mail-Followup-To: John Keeping , Mark Yao , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, linux-rockchip@lists.infradead.org, linux-arm-kernel@lists.infradead.org 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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160113164038.5fcf670f.john@metanate.com> X-Operating-System: Linux phenom 4.3.0-1-amd64 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 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. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch