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 16:40:38 +0000 Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160113162156.GA19130@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 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 T24gV2VkLCAxMyBKYW4gMjAxNiAxNzoyMTo1NiArMDEwMCwgRGFuaWVsIFZldHRlciB3cm90ZToK Cj4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDM6NTU6MjlQTSArMDAwMCwgSm9obiBLZWVwaW5n IHdyb3RlOgo+ID4gT24gV2VkLCAxMyBKYW4gMjAxNiAxNjo0MDowNSArMDEwMCwgRGFuaWVsIFZl dHRlciB3cm90ZToKPiA+ICAgCj4gPiA+IE9uIFdlZCwgSmFuIDEzLCAyMDE2IGF0IDAyOjM0OjI1 UE0gKzAwMDAsIEpvaG4gS2VlcGluZyB3cm90ZTogIAo+ID4gPiA+IE9uIFdlZCwgMTMgSmFuIDIw MTYgMTU6MjM6MjAgKzAxMDAsIERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gPiA+ID4gICAgIAo+ID4g PiA+ID4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMTI6NTM6MzRQTSArMDAwMCwgSm9obiBLZWVw aW5nIHdyb3RlOiAgICAKPiA+ID4gPiA+ID4gQXMgY29tbWVudGVkIGluIGRybV9hdG9taWNfaGVs cGVyX3dhaXRfZm9yX3ZibGFua3MoKSwgdXNlcnNwYWNlCj4gPiA+ID4gPiA+IHJlbGllcyBvbiBj dXJzb3IgaW9jdGxzIGJlaW5nIHVuc3luY2VkLiAgQ29udmVydGluZyB0aGUgcm9ja2NoaXAKPiA+ ID4gPiA+ID4gZHJpdmVyIHRvIGF0b21pYyBoYXMgc2lnbmlmaWNhbnRseSBpbXBhY3RlZCBjdXJz b3IgcGVyZm9ybWFuY2UgYnkKPiA+ID4gPiA+ID4gbWFraW5nIGV2ZXJ5IGN1cnNvciB1cGRhdGUg d2FpdCBmb3IgdmJsYW5rLgo+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4gQnkgc2tpcHBpbmcgdGhl IHZibGFuayBzeW5jIHdoZW4gdGhlIGZyYW1lYnVmZmVyIGhhcyBub3QgY2hhbmdlZAo+ID4gPiA+ ID4gPiAoYXMgaXMgZG9uZSBpbiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCkp IHdlIGNhbiBhdm9pZAo+ID4gPiA+ID4gPiB0aGlzIGZvciB0aGUgY29tbW9uIGNhc2Ugb2YgbW92 aW5nIHRoZSBjdXJzb3IgYW5kIG9ubHkgbmVlZCB0bwo+ID4gPiA+ID4gPiBkZWxheSB0aGUgY3Vy c29yIGlvY3RsIHdoZW4gdGhlIGN1cnNvciBpY29uIGNoYW5nZXMuCj4gPiA+ID4gPiA+IAo+ID4g PiA+ID4gPiBJIG9yaWdpbmFsbHkgaW5zZXJ0ZWQgYSBjaGVjayBvbiBsZWdhY3lfY3Vyc29yX3Vw ZGF0ZSBhcyB3ZWxsLCBidXQKPiA+ID4gPiA+ID4gdGhhdCBjYXVzZWQgYSBzdG9ybSBvZiBpb21t dSBwYWdlIGZhdWx0cy4gIEkgZGlkbid0IGludmVzdGlnYXRlIHRoZQo+ID4gPiA+ID4gPiBjYXVz ZSBvZiB0aG9zZSBzaW5jZSB0aGlzIGNoYW5nZSBnaXZlcyBlbm91Z2ggb2YgYSBwZXJmb3JtYW5j ZQo+ID4gPiA+ID4gPiBpbXByb3ZlbWVudCBmb3IgbXkgdXNlIGNhc2UuCj4gPiA+ID4gPiA+IAo+ ID4gPiA+ID4gPiBUaGlzIGlzIFJGQyBiZWNhdXNlIG9mIHRoYXQgYW5kIGJlY2F1c2UgdGhlIGZy YW1lYnVmZmVyX2NoYW5nZWQoKQo+ID4gPiA+ID4gPiBmdW5jdGlvbiBpcyBjb3BpZWQgZnJvbSBk cm1fYXRvbWljX2hlbHBlci5jIGFzIGEgcXVpY2sgd2F5IHRvIHRlc3QKPiA+ID4gPiA+ID4gdGhl IHJlc3VsdC4KPiA+ID4gPiA+ID4gCj4gPiA+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEpvaG4gS2Vl cGluZyA8am9obkBtZXRhbmF0ZS5jb20+Cj4gPiA+ID4gPiA+IC0tLQo+ID4gPiA+ID4gPiAgZHJp dmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jIHwgMjcKPiA+ID4gPiA+ID4g KysrKysrKysrKysrKysrKysrKysrKysrKy0tIDEgZmlsZSBjaGFuZ2VkLCAyNSBpbnNlcnRpb25z KCspLCAyCj4gPiA+ID4gPiA+IGRlbGV0aW9ucygtKQo+ID4gPiA+ID4gPiAKPiA+ID4gPiA+ID4g ZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYwo+ ID4gPiA+ID4gPiBiL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYyBp bmRleCBmNzg0NDg4Li44ZmQ5ODIxCj4gPiA+ID4gPiA+IDEwMDY0NCAtLS0gYS9kcml2ZXJzL2dw dS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMKPiA+ID4gPiA+ID4gKysrIGIvZHJpdmVy cy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jCj4gPiA+ID4gPiA+IEBAIC0xNzcs OCArMTc3LDI4IEBAIHN0YXRpYyB2b2lkCj4gPiA+ID4gPiA+IHJvY2tjaGlwX2NydGNfd2FpdF9m b3JfdXBkYXRlKHN0cnVjdCBkcm1fY3J0YyAqY3J0YykKPiA+ID4gPiA+ID4gY3J0Y19mdW5jcy0+ d2FpdF9mb3JfdXBkYXRlKGNydGMpOyB9Cj4gPiA+ID4gPiA+ICAKPiA+ID4gPiA+ID4gK3N0YXRp YyBib29sIGZyYW1lYnVmZmVyX2NoYW5nZWQoc3RydWN0IGRybV9kZXZpY2UgKmRldiwKPiA+ID4g PiA+ID4gKwkJCQlzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAqb2xkX3N0YXRlLAo+ID4gPiA+ID4g PiArCQkJCXN0cnVjdCBkcm1fY3J0YyAqY3J0YykKPiA+ID4gPiA+ID4gK3sKPiA+ID4gPiA+ID4g KwlzdHJ1Y3QgZHJtX3BsYW5lICpwbGFuZTsKPiA+ID4gPiA+ID4gKwlzdHJ1Y3QgZHJtX3BsYW5l X3N0YXRlICpvbGRfcGxhbmVfc3RhdGU7Cj4gPiA+ID4gPiA+ICsJaW50IGk7Cj4gPiA+ID4gPiA+ ICsKPiA+ID4gPiA+ID4gKwlmb3JfZWFjaF9wbGFuZV9pbl9zdGF0ZShvbGRfc3RhdGUsIHBsYW5l LCBvbGRfcGxhbmVfc3RhdGUsCj4gPiA+ID4gPiA+IGkpIHsKPiA+ID4gPiA+ID4gKwkJaWYgKHBs YW5lLT5zdGF0ZS0+Y3J0YyAhPSBjcnRjICYmCj4gPiA+ID4gPiA+ICsJCSAgICBvbGRfcGxhbmVf c3RhdGUtPmNydGMgIT0gY3J0YykKPiA+ID4gPiA+ID4gKwkJCWNvbnRpbnVlOwo+ID4gPiA+ID4g PiArCj4gPiA+ID4gPiA+ICsJCWlmIChwbGFuZS0+c3RhdGUtPmZiICE9IG9sZF9wbGFuZV9zdGF0 ZS0+ZmIpCj4gPiA+ID4gPiA+ICsJCQlyZXR1cm4gdHJ1ZTsKPiA+ID4gPiA+ID4gKwl9Cj4gPiA+ ID4gPiA+ICsKPiA+ID4gPiA+ID4gKwlyZXR1cm4gZmFsc2U7Cj4gPiA+ID4gPiA+ICt9ICAgICAg Cj4gPiA+ID4gPiAKPiA+ID4gPiA+IFBsZWFzZSBkb24ndCBoYW5kLXJvbGwgbG9naWMgdGhhdCBh ZmZlY3RzIHNlbWFudGljcyBsaWtlIHRoaXMuIEluc3RlYWQKPiA+ID4gPiA+IHBsZWFzZSB1c2Ug ZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpLCB3aGljaCBzaG91bGQgZG8gdGhp cwo+ID4gPiA+ID4gY29ycmVjdGx5IGZvciB5b3UuCj4gPiA+ID4gPiAKPiA+ID4gPiA+IElmIHRo YXQncyBub3QgdGhlIGNhc2UgdGhlbiB3ZSBuZWVkIHRvIGltcHJvdmUgdGhlIGdlbmVyaWMgaGVs cGVyLCBvcgo+ID4gPiA+ID4gZmlndXJlIG91dCB3aGF0J3MgZGlmZmVyZW50IHdpdGggcm9ja2hp cC4gICAgCj4gPiA+ID4gCj4gPiA+ID4gQWNjb3JkaW5nIHRvIGNvbW1pdCA2M2ViYjlmIChkcm0v cm9ja2NoaXA6IENvbnZlcnQgdG8gc3VwcG9ydCBhdG9taWMKPiA+ID4gPiBBUEkpIGl0J3MgYmVj YXVzZSByb2NrY2hpcCBkb2Vzbid0IGhhdmUgYSBoYXJkd2FyZSB2YmxhbmsgY291bnRlci4KPiA+ ID4gPiAKPiA+ID4gPiBJJ20gbm90IGVudGlyZWx5IGNsZWFyIG9uIHdoeSB0aGlzIHByZXZlbnRz IHRoZSB1c2Ugb2YKPiA+ID4gPiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCku ICAgIAo+ID4gPiAKPiA+ID4gSG0sIHRoYXQgY29tbWl0IGlzbid0IHRlcnJpYmx5IGhlbHBmdWwu IElmIHRoYXQncyByZWFsbHkgbmVlZGVkIHRoZW4gaW1vIEkKPiA+ID4gdGhpbmsgd2Ugc2hvdWxk IGV4dHJhY3QgYSAiZHJtX2F0b21pY19oZWxwZXJfcGxhbmVfbmVlZHNfdmJsYW5rX3dhaXQoKSIK PiA+ID4gaGVscGVyIHRoYXQncyB1c2VkIGJ5IGJvdGguIEJ1dCBzaW5jZSByb2NrY2hpcCBkb2Vz IHZibGFua19nZXQvcHV0IGNhbGxzCj4gPiA+IEknZCBob3BlIHZibGFua3MgYWN0dWFsbHkgd29y ayBjb3JyZWN0bHkuIEFuZCB0aGVuIHRoZSBoZWxwZXIgc2hvdWxkIHdvcmsKPiA+ID4gdG9vLiAg Cj4gPiAKPiA+IEkgdHJpZWQgc3dpdGNoaW5nIHRoZSBjYWxsIHRvIHJvY2tjaGlwX2NydGNfd2Fp dF9mb3JfdXBkYXRlKCkgdG8KPiA+IGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3Mo KSBhbmQgaXQgd29ya3MgZmluZSB1bnRpbCBJIHN3aXRjaAo+ID4gdGhlIGJ1ZmZlciBhc3NvY2lh dGVkIHdpdGggYSBjdXJzb3IsIGF0IHdoaWNoIHBvaW50IEkgZ2V0IGlvbW11IHBhZ2UKPiA+IGZh dWx0cywgcHJlc3VtYWJseSBiZWNhdXNlIHRoZSBHRU0gYnVmZmVyIGlzIHVucmVmZXJlbmNlZCB0 b28gZWFybHkuCj4gPiAKPiA+IEFGQUlDVCB0aGUgYnVmZmVyIHdpbGwgYmUgcmVsZWFzZWQgdmlh IGRybV9hdG9taWNfc3RhdGVfZnJlZSgpCj4gPiB1bmNvbmRpdGlvbmFsbHksIGJ1dCBJIHN1c3Bl Y3QgSSdtIG1pc3Npbmcgc29tZXRoaW5nIHNpbmNlIHRoYXQgd291bGQKPiA+IG1lYW4gZXZlcnkg ZHJpdmVyIHdvdWxkIGhpdCBhIHNpbWlsYXIgcHJvYmxlbS4gIAo+IAo+IFllYWgsIHdpdGggdGhl IGhlbHBlciB3ZSBhbHdheXMgc2tpcCwgd2hpY2ggbWVhbnMgd2hlbiB0aGUgY3Vyc29yIGJvCj4g Y2hhbmdlcyB5b3UgaW5kZWVkIHVubWFwIHRvbyBlYXJseS4gU28gY2FuJ3QgZXZlbiBzaGFyZSB0 aGUgb3ZlcmFsbAo+IGNvbmRpdGlvbiwgYnV0IHdlIGNvdWxkIGRlZmluaXRlbHkgc2hhcmUgdGhl IGxpdHRsZSBmcmFtZWJ1ZmZlcl9jaGFuZ2VkCj4gaGVscGVyLgoKVGhhdCBsZWF2ZXMgbWUgd2l0 aCB0aGUgcXVlc3Rpb246IHdoeSBkbyBvdGhlciBhdG9taWMgZHJpdmVycyB3b3JrPwoKSWYgZHJt X2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpIHNraXBwaW5nIHZibGFua3MgcmVzdWx0 cyBpbiB0aGUKY3Vyc29yIGJvIGJlaW5nIHVubWFwcGVkIHRvbyBlYXJseSBmb3Igcm9ja2NoaXAs IHdoeSBpcyBpdCBub3QgdW5tYXBwZWQKdG9vIGVhcmx5IGZvciBhbGwgb2YgdGhlIG90aGVyIGRy aXZlcnMgdXNpbmcgdGhhdCBoZWxwZXI/Cl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZy ZWVkZXNrdG9wLm9yZwpodHRwOi8vbGlzdHMuZnJlZWRlc2t0b3Aub3JnL21haWxtYW4vbGlzdGlu Zm8vZHJpLWRldmVsCg== From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Wed, 13 Jan 2016 16:40:38 +0000 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <20160113162156.GA19130@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> Message-ID: <20160113164038.5fcf670f.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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? From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754544AbcAMQlS (ORCPT ); Wed, 13 Jan 2016 11:41:18 -0500 Received: from dougal.metanate.com ([90.155.101.14]:17530 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1754251AbcAMQlN (ORCPT ); Wed, 13 Jan 2016 11:41:13 -0500 Date: Wed, 13 Jan 2016 16:40:38 +0000 From: John Keeping To: Daniel Vetter Cc: 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: <20160113164038.5fcf670f.john@metanate.com> In-Reply-To: <20160113162156.GA19130@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> 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 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?