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 15:55:29 +0000 Message-ID: <20160113155529.6048b2c1.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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160113154005.GT19130@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 T24gV2VkLCAxMyBKYW4gMjAxNiAxNjo0MDowNSArMDEwMCwgRGFuaWVsIFZldHRlciB3cm90ZToK Cj4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDI6MzQ6MjVQTSArMDAwMCwgSm9obiBLZWVwaW5n IHdyb3RlOgo+ID4gT24gV2VkLCAxMyBKYW4gMjAxNiAxNToyMzoyMCArMDEwMCwgRGFuaWVsIFZl dHRlciB3cm90ZToKPiA+ICAgCj4gPiA+IE9uIFdlZCwgSmFuIDEzLCAyMDE2IGF0IDEyOjUzOjM0 UE0gKzAwMDAsIEpvaG4gS2VlcGluZyB3cm90ZTogIAo+ID4gPiA+IEFzIGNvbW1lbnRlZCBpbiBk cm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCksIHVzZXJzcGFjZQo+ID4gPiA+IHJl bGllcyBvbiBjdXJzb3IgaW9jdGxzIGJlaW5nIHVuc3luY2VkLiAgQ29udmVydGluZyB0aGUgcm9j a2NoaXAKPiA+ID4gPiBkcml2ZXIgdG8gYXRvbWljIGhhcyBzaWduaWZpY2FudGx5IGltcGFjdGVk IGN1cnNvciBwZXJmb3JtYW5jZSBieQo+ID4gPiA+IG1ha2luZyBldmVyeSBjdXJzb3IgdXBkYXRl IHdhaXQgZm9yIHZibGFuay4KPiA+ID4gPiAKPiA+ID4gPiBCeSBza2lwcGluZyB0aGUgdmJsYW5r IHN5bmMgd2hlbiB0aGUgZnJhbWVidWZmZXIgaGFzIG5vdCBjaGFuZ2VkCj4gPiA+ID4gKGFzIGlz IGRvbmUgaW4gZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpKSB3ZSBjYW4gYXZv aWQKPiA+ID4gPiB0aGlzIGZvciB0aGUgY29tbW9uIGNhc2Ugb2YgbW92aW5nIHRoZSBjdXJzb3Ig YW5kIG9ubHkgbmVlZCB0bwo+ID4gPiA+IGRlbGF5IHRoZSBjdXJzb3IgaW9jdGwgd2hlbiB0aGUg Y3Vyc29yIGljb24gY2hhbmdlcy4KPiA+ID4gPiAKPiA+ID4gPiBJIG9yaWdpbmFsbHkgaW5zZXJ0 ZWQgYSBjaGVjayBvbiBsZWdhY3lfY3Vyc29yX3VwZGF0ZSBhcyB3ZWxsLCBidXQKPiA+ID4gPiB0 aGF0IGNhdXNlZCBhIHN0b3JtIG9mIGlvbW11IHBhZ2UgZmF1bHRzLiAgSSBkaWRuJ3QgaW52ZXN0 aWdhdGUgdGhlCj4gPiA+ID4gY2F1c2Ugb2YgdGhvc2Ugc2luY2UgdGhpcyBjaGFuZ2UgZ2l2ZXMg ZW5vdWdoIG9mIGEgcGVyZm9ybWFuY2UKPiA+ID4gPiBpbXByb3ZlbWVudCBmb3IgbXkgdXNlIGNh c2UuCj4gPiA+ID4gCj4gPiA+ID4gVGhpcyBpcyBSRkMgYmVjYXVzZSBvZiB0aGF0IGFuZCBiZWNh dXNlIHRoZSBmcmFtZWJ1ZmZlcl9jaGFuZ2VkKCkKPiA+ID4gPiBmdW5jdGlvbiBpcyBjb3BpZWQg ZnJvbSBkcm1fYXRvbWljX2hlbHBlci5jIGFzIGEgcXVpY2sgd2F5IHRvIHRlc3QKPiA+ID4gPiB0 aGUgcmVzdWx0Lgo+ID4gPiA+IAo+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEpvaG4gS2VlcGluZyA8 am9obkBtZXRhbmF0ZS5jb20+Cj4gPiA+ID4gLS0tCj4gPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9y b2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYyB8IDI3Cj4gPiA+ID4gKysrKysrKysrKysrKysrKysr KysrKysrKy0tIDEgZmlsZSBjaGFuZ2VkLCAyNSBpbnNlcnRpb25zKCspLCAyCj4gPiA+ID4gZGVs ZXRpb25zKC0pCj4gPiA+ID4gCj4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9y b2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYwo+ID4gPiA+IGIvZHJpdmVycy9ncHUvZHJtL3JvY2tj aGlwL3JvY2tjaGlwX2RybV9mYi5jIGluZGV4IGY3ODQ0ODguLjhmZDk4MjEKPiA+ID4gPiAxMDA2 NDQgLS0tIGEvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jCj4gPiA+ ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jCj4gPiA+ ID4gQEAgLTE3Nyw4ICsxNzcsMjggQEAgc3RhdGljIHZvaWQKPiA+ID4gPiByb2NrY2hpcF9jcnRj X3dhaXRfZm9yX3VwZGF0ZShzdHJ1Y3QgZHJtX2NydGMgKmNydGMpCj4gPiA+ID4gY3J0Y19mdW5j cy0+d2FpdF9mb3JfdXBkYXRlKGNydGMpOyB9Cj4gPiA+ID4gIAo+ID4gPiA+ICtzdGF0aWMgYm9v bCBmcmFtZWJ1ZmZlcl9jaGFuZ2VkKHN0cnVjdCBkcm1fZGV2aWNlICpkZXYsCj4gPiA+ID4gKwkJ CQlzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAqb2xkX3N0YXRlLAo+ID4gPiA+ICsJCQkJc3RydWN0 IGRybV9jcnRjICpjcnRjKQo+ID4gPiA+ICt7Cj4gPiA+ID4gKwlzdHJ1Y3QgZHJtX3BsYW5lICpw bGFuZTsKPiA+ID4gPiArCXN0cnVjdCBkcm1fcGxhbmVfc3RhdGUgKm9sZF9wbGFuZV9zdGF0ZTsK PiA+ID4gPiArCWludCBpOwo+ID4gPiA+ICsKPiA+ID4gPiArCWZvcl9lYWNoX3BsYW5lX2luX3N0 YXRlKG9sZF9zdGF0ZSwgcGxhbmUsIG9sZF9wbGFuZV9zdGF0ZSwKPiA+ID4gPiBpKSB7Cj4gPiA+ ID4gKwkJaWYgKHBsYW5lLT5zdGF0ZS0+Y3J0YyAhPSBjcnRjICYmCj4gPiA+ID4gKwkJICAgIG9s ZF9wbGFuZV9zdGF0ZS0+Y3J0YyAhPSBjcnRjKQo+ID4gPiA+ICsJCQljb250aW51ZTsKPiA+ID4g PiArCj4gPiA+ID4gKwkJaWYgKHBsYW5lLT5zdGF0ZS0+ZmIgIT0gb2xkX3BsYW5lX3N0YXRlLT5m YikKPiA+ID4gPiArCQkJcmV0dXJuIHRydWU7Cj4gPiA+ID4gKwl9Cj4gPiA+ID4gKwo+ID4gPiA+ ICsJcmV0dXJuIGZhbHNlOwo+ID4gPiA+ICt9ICAgIAo+ID4gPiAKPiA+ID4gUGxlYXNlIGRvbid0 IGhhbmQtcm9sbCBsb2dpYyB0aGF0IGFmZmVjdHMgc2VtYW50aWNzIGxpa2UgdGhpcy4gSW5zdGVh ZAo+ID4gPiBwbGVhc2UgdXNlIGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKSwg d2hpY2ggc2hvdWxkIGRvIHRoaXMKPiA+ID4gY29ycmVjdGx5IGZvciB5b3UuCj4gPiA+IAo+ID4g PiBJZiB0aGF0J3Mgbm90IHRoZSBjYXNlIHRoZW4gd2UgbmVlZCB0byBpbXByb3ZlIHRoZSBnZW5l cmljIGhlbHBlciwgb3IKPiA+ID4gZmlndXJlIG91dCB3aGF0J3MgZGlmZmVyZW50IHdpdGggcm9j a2hpcC4gIAo+ID4gCj4gPiBBY2NvcmRpbmcgdG8gY29tbWl0IDYzZWJiOWYgKGRybS9yb2NrY2hp cDogQ29udmVydCB0byBzdXBwb3J0IGF0b21pYwo+ID4gQVBJKSBpdCdzIGJlY2F1c2Ugcm9ja2No aXAgZG9lc24ndCBoYXZlIGEgaGFyZHdhcmUgdmJsYW5rIGNvdW50ZXIuCj4gPiAKPiA+IEknbSBu b3QgZW50aXJlbHkgY2xlYXIgb24gd2h5IHRoaXMgcHJldmVudHMgdGhlIHVzZSBvZgo+ID4gZHJt X2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpLiAgCj4gCj4gSG0sIHRoYXQgY29tbWl0 IGlzbid0IHRlcnJpYmx5IGhlbHBmdWwuIElmIHRoYXQncyByZWFsbHkgbmVlZGVkIHRoZW4gaW1v IEkKPiB0aGluayB3ZSBzaG91bGQgZXh0cmFjdCBhICJkcm1fYXRvbWljX2hlbHBlcl9wbGFuZV9u ZWVkc192Ymxhbmtfd2FpdCgpIgo+IGhlbHBlciB0aGF0J3MgdXNlZCBieSBib3RoLiBCdXQgc2lu Y2Ugcm9ja2NoaXAgZG9lcyB2YmxhbmtfZ2V0L3B1dCBjYWxscwo+IEknZCBob3BlIHZibGFua3Mg YWN0dWFsbHkgd29yayBjb3JyZWN0bHkuIEFuZCB0aGVuIHRoZSBoZWxwZXIgc2hvdWxkIHdvcmsK PiB0b28uCgpJIHRyaWVkIHN3aXRjaGluZyB0aGUgY2FsbCB0byByb2NrY2hpcF9jcnRjX3dhaXRf Zm9yX3VwZGF0ZSgpIHRvCmRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3ZibGFua3MoKSBhbmQg aXQgd29ya3MgZmluZSB1bnRpbCBJIHN3aXRjaAp0aGUgYnVmZmVyIGFzc29jaWF0ZWQgd2l0aCBh IGN1cnNvciwgYXQgd2hpY2ggcG9pbnQgSSBnZXQgaW9tbXUgcGFnZQpmYXVsdHMsIHByZXN1bWFi bHkgYmVjYXVzZSB0aGUgR0VNIGJ1ZmZlciBpcyB1bnJlZmVyZW5jZWQgdG9vIGVhcmx5LgoKQUZB SUNUIHRoZSBidWZmZXIgd2lsbCBiZSByZWxlYXNlZCB2aWEgZHJtX2F0b21pY19zdGF0ZV9mcmVl KCkKdW5jb25kaXRpb25hbGx5LCBidXQgSSBzdXNwZWN0IEknbSBtaXNzaW5nIHNvbWV0aGluZyBz aW5jZSB0aGF0IHdvdWxkCm1lYW4gZXZlcnkgZHJpdmVyIHdvdWxkIGhpdCBhIHNpbWlsYXIgcHJv YmxlbS4KX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX18KZHJp LWRldmVsIG1haWxpbmcgbGlzdApkcmktZGV2ZWxAbGlzdHMuZnJlZWRlc2t0b3Aub3JnCmh0dHA6 Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Wed, 13 Jan 2016 15:55:29 +0000 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <20160113154005.GT19130@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> Message-ID: <20160113155529.6048b2c1.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752847AbcAMP4M (ORCPT ); Wed, 13 Jan 2016 10:56:12 -0500 Received: from dougal.metanate.com ([90.155.101.14]:10755 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1750795AbcAMP4L (ORCPT ); Wed, 13 Jan 2016 10:56:11 -0500 Date: Wed, 13 Jan 2016 15:55:29 +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: <20160113155529.6048b2c1.john@metanate.com> In-Reply-To: <20160113154005.GT19130@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> 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 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.