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 14:34:25 +0000 Message-ID: <20160113143425.363ce07a.john@metanate.com> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@phenom.ffwll.local> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: In-Reply-To: <20160113142320.GQ19130@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 T24gV2VkLCAxMyBKYW4gMjAxNiAxNToyMzoyMCArMDEwMCwgRGFuaWVsIFZldHRlciB3cm90ZToK Cj4gT24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMTI6NTM6MzRQTSArMDAwMCwgSm9obiBLZWVwaW5n IHdyb3RlOgo+ID4gQXMgY29tbWVudGVkIGluIGRybV9hdG9taWNfaGVscGVyX3dhaXRfZm9yX3Zi bGFua3MoKSwgdXNlcnNwYWNlCj4gPiByZWxpZXMgb24gY3Vyc29yIGlvY3RscyBiZWluZyB1bnN5 bmNlZC4gIENvbnZlcnRpbmcgdGhlIHJvY2tjaGlwCj4gPiBkcml2ZXIgdG8gYXRvbWljIGhhcyBz aWduaWZpY2FudGx5IGltcGFjdGVkIGN1cnNvciBwZXJmb3JtYW5jZSBieQo+ID4gbWFraW5nIGV2 ZXJ5IGN1cnNvciB1cGRhdGUgd2FpdCBmb3IgdmJsYW5rLgo+ID4gCj4gPiBCeSBza2lwcGluZyB0 aGUgdmJsYW5rIHN5bmMgd2hlbiB0aGUgZnJhbWVidWZmZXIgaGFzIG5vdCBjaGFuZ2VkCj4gPiAo YXMgaXMgZG9uZSBpbiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCkpIHdlIGNh biBhdm9pZAo+ID4gdGhpcyBmb3IgdGhlIGNvbW1vbiBjYXNlIG9mIG1vdmluZyB0aGUgY3Vyc29y IGFuZCBvbmx5IG5lZWQgdG8KPiA+IGRlbGF5IHRoZSBjdXJzb3IgaW9jdGwgd2hlbiB0aGUgY3Vy c29yIGljb24gY2hhbmdlcy4KPiA+IAo+ID4gSSBvcmlnaW5hbGx5IGluc2VydGVkIGEgY2hlY2sg b24gbGVnYWN5X2N1cnNvcl91cGRhdGUgYXMgd2VsbCwgYnV0Cj4gPiB0aGF0IGNhdXNlZCBhIHN0 b3JtIG9mIGlvbW11IHBhZ2UgZmF1bHRzLiAgSSBkaWRuJ3QgaW52ZXN0aWdhdGUgdGhlCj4gPiBj YXVzZSBvZiB0aG9zZSBzaW5jZSB0aGlzIGNoYW5nZSBnaXZlcyBlbm91Z2ggb2YgYSBwZXJmb3Jt YW5jZQo+ID4gaW1wcm92ZW1lbnQgZm9yIG15IHVzZSBjYXNlLgo+ID4gCj4gPiBUaGlzIGlzIFJG QyBiZWNhdXNlIG9mIHRoYXQgYW5kIGJlY2F1c2UgdGhlIGZyYW1lYnVmZmVyX2NoYW5nZWQoKQo+ ID4gZnVuY3Rpb24gaXMgY29waWVkIGZyb20gZHJtX2F0b21pY19oZWxwZXIuYyBhcyBhIHF1aWNr IHdheSB0byB0ZXN0Cj4gPiB0aGUgcmVzdWx0Lgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBKb2hu IEtlZXBpbmcgPGpvaG5AbWV0YW5hdGUuY29tPgo+ID4gLS0tCj4gPiAgZHJpdmVycy9ncHUvZHJt L3JvY2tjaGlwL3JvY2tjaGlwX2RybV9mYi5jIHwgMjcKPiA+ICsrKysrKysrKysrKysrKysrKysr KysrKystLSAxIGZpbGUgY2hhbmdlZCwgMjUgaW5zZXJ0aW9ucygrKSwgMgo+ID4gZGVsZXRpb25z KC0pCj4gPiAKPiA+IGRpZmYgLS1naXQgYS9kcml2ZXJzL2dwdS9kcm0vcm9ja2NoaXAvcm9ja2No aXBfZHJtX2ZiLmMKPiA+IGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tjaGlwX2RybV9m Yi5jIGluZGV4IGY3ODQ0ODguLjhmZDk4MjEKPiA+IDEwMDY0NCAtLS0gYS9kcml2ZXJzL2dwdS9k cm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMKPiA+ICsrKyBiL2RyaXZlcnMvZ3B1L2RybS9y b2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYwo+ID4gQEAgLTE3Nyw4ICsxNzcsMjggQEAgc3RhdGlj IHZvaWQKPiA+IHJvY2tjaGlwX2NydGNfd2FpdF9mb3JfdXBkYXRlKHN0cnVjdCBkcm1fY3J0YyAq Y3J0YykKPiA+IGNydGNfZnVuY3MtPndhaXRfZm9yX3VwZGF0ZShjcnRjKTsgfQo+ID4gIAo+ID4g K3N0YXRpYyBib29sIGZyYW1lYnVmZmVyX2NoYW5nZWQoc3RydWN0IGRybV9kZXZpY2UgKmRldiwK PiA+ICsJCQkJc3RydWN0IGRybV9hdG9taWNfc3RhdGUgKm9sZF9zdGF0ZSwKPiA+ICsJCQkJc3Ry dWN0IGRybV9jcnRjICpjcnRjKQo+ID4gK3sKPiA+ICsJc3RydWN0IGRybV9wbGFuZSAqcGxhbmU7 Cj4gPiArCXN0cnVjdCBkcm1fcGxhbmVfc3RhdGUgKm9sZF9wbGFuZV9zdGF0ZTsKPiA+ICsJaW50 IGk7Cj4gPiArCj4gPiArCWZvcl9lYWNoX3BsYW5lX2luX3N0YXRlKG9sZF9zdGF0ZSwgcGxhbmUs IG9sZF9wbGFuZV9zdGF0ZSwKPiA+IGkpIHsKPiA+ICsJCWlmIChwbGFuZS0+c3RhdGUtPmNydGMg IT0gY3J0YyAmJgo+ID4gKwkJICAgIG9sZF9wbGFuZV9zdGF0ZS0+Y3J0YyAhPSBjcnRjKQo+ID4g KwkJCWNvbnRpbnVlOwo+ID4gKwo+ID4gKwkJaWYgKHBsYW5lLT5zdGF0ZS0+ZmIgIT0gb2xkX3Bs YW5lX3N0YXRlLT5mYikKPiA+ICsJCQlyZXR1cm4gdHJ1ZTsKPiA+ICsJfQo+ID4gKwo+ID4gKwly ZXR1cm4gZmFsc2U7Cj4gPiArfSAgCj4gCj4gUGxlYXNlIGRvbid0IGhhbmQtcm9sbCBsb2dpYyB0 aGF0IGFmZmVjdHMgc2VtYW50aWNzIGxpa2UgdGhpcy4gSW5zdGVhZAo+IHBsZWFzZSB1c2UgZHJt X2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpLCB3aGljaCBzaG91bGQgZG8gdGhpcwo+ IGNvcnJlY3RseSBmb3IgeW91Lgo+IAo+IElmIHRoYXQncyBub3QgdGhlIGNhc2UgdGhlbiB3ZSBu ZWVkIHRvIGltcHJvdmUgdGhlIGdlbmVyaWMgaGVscGVyLCBvcgo+IGZpZ3VyZSBvdXQgd2hhdCdz IGRpZmZlcmVudCB3aXRoIHJvY2toaXAuCgpBY2NvcmRpbmcgdG8gY29tbWl0IDYzZWJiOWYgKGRy bS9yb2NrY2hpcDogQ29udmVydCB0byBzdXBwb3J0IGF0b21pYwpBUEkpIGl0J3MgYmVjYXVzZSBy b2NrY2hpcCBkb2Vzbid0IGhhdmUgYSBoYXJkd2FyZSB2YmxhbmsgY291bnRlci4KCkknbSBub3Qg ZW50aXJlbHkgY2xlYXIgb24gd2h5IHRoaXMgcHJldmVudHMgdGhlIHVzZSBvZgpkcm1fYXRvbWlj X2hlbHBlcl93YWl0X2Zvcl92YmxhbmtzKCkuCgo+ID4gKwo+ID4gIHN0YXRpYyB2b2lkCj4gPiAt cm9ja2NoaXBfYXRvbWljX3dhaXRfZm9yX2NvbXBsZXRlKHN0cnVjdCBkcm1fYXRvbWljX3N0YXRl Cj4gPiAqb2xkX3N0YXRlKSArcm9ja2NoaXBfYXRvbWljX3dhaXRfZm9yX2NvbXBsZXRlKHN0cnVj dCBkcm1fZGV2aWNlCj4gPiAqZGV2LCBzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAqb2xkX3N0YXRl KSB7Cj4gPiAgCXN0cnVjdCBkcm1fY3J0Y19zdGF0ZSAqb2xkX2NydGNfc3RhdGU7Cj4gPiAgCXN0 cnVjdCBkcm1fY3J0YyAqY3J0YzsKPiA+IEBAIC0xOTQsNiArMjE0LDkgQEAgcm9ja2NoaXBfYXRv bWljX3dhaXRfZm9yX2NvbXBsZXRlKHN0cnVjdAo+ID4gZHJtX2F0b21pY19zdGF0ZSAqb2xkX3N0 YXRlKSBpZiAoIWNydGMtPnN0YXRlLT5hY3RpdmUpCj4gPiAgCQkJY29udGludWU7Cj4gPiAgCj4g PiArCQlpZiAoIWZyYW1lYnVmZmVyX2NoYW5nZWQoZGV2LCBvbGRfc3RhdGUsIGNydGMpKQo+ID4g KwkJCWNvbnRpbnVlOwo+ID4gKwo+ID4gIAkJcmV0ID0gZHJtX2NydGNfdmJsYW5rX2dldChjcnRj KTsKPiA+ICAJCWlmIChyZXQgIT0gMCkKPiA+ICAJCQljb250aW51ZTsKPiA+IEBAIC0yNDEsNyAr MjY0LDcgQEAgcm9ja2NoaXBfYXRvbWljX2NvbW1pdF9jb21wbGV0ZShzdHJ1Y3QKPiA+IHJvY2tj aGlwX2F0b21pY19jb21taXQgKmNvbW1pdCkgCj4gPiAgCWRybV9hdG9taWNfaGVscGVyX2NvbW1p dF9wbGFuZXMoZGV2LCBzdGF0ZSwgdHJ1ZSk7Cj4gPiAgCj4gPiAtCXJvY2tjaGlwX2F0b21pY193 YWl0X2Zvcl9jb21wbGV0ZShzdGF0ZSk7Cj4gPiArCXJvY2tjaGlwX2F0b21pY193YWl0X2Zvcl9j b21wbGV0ZShkZXYsIHN0YXRlKTsKPiA+ICAKPiA+ICAJZHJtX2F0b21pY19oZWxwZXJfY2xlYW51 cF9wbGFuZXMoZGV2LCBzdGF0ZSk7Cj4gPiAgCj4gPiAtLSAKPiA+IDIuNy4wLnJjMy4xNDAuZzUy MGEwOTMKPiA+IAo+ID4gX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX18KPiA+IGRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKPiA+IGRyaS1kZXZlbEBsaXN0cy5mcmVl ZGVza3RvcC5vcmcKPiA+IGh0dHA6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9kcmktZGV2ZWwgIAo+IApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVz a3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2Ry aS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: john@metanate.com (John Keeping) Date: Wed, 13 Jan 2016 14:34:25 +0000 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <20160113142320.GQ19130@phenom.ffwll.local> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@phenom.ffwll.local> Message-ID: <20160113143425.363ce07a.john@metanate.com> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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(). > > + > > static void > > -rockchip_atomic_wait_for_complete(struct drm_atomic_state > > *old_state) +rockchip_atomic_wait_for_complete(struct drm_device > > *dev, struct drm_atomic_state *old_state) { > > struct drm_crtc_state *old_crtc_state; > > struct drm_crtc *crtc; > > @@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct > > drm_atomic_state *old_state) if (!crtc->state->active) > > continue; > > > > + if (!framebuffer_changed(dev, old_state, crtc)) > > + continue; > > + > > ret = drm_crtc_vblank_get(crtc); > > if (ret != 0) > > continue; > > @@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct > > rockchip_atomic_commit *commit) > > drm_atomic_helper_commit_planes(dev, state, true); > > > > - rockchip_atomic_wait_for_complete(state); > > + rockchip_atomic_wait_for_complete(dev, state); > > > > drm_atomic_helper_cleanup_planes(dev, state); > > > > -- > > 2.7.0.rc3.140.g520a093 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel at lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel > From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755253AbcAMOe5 (ORCPT ); Wed, 13 Jan 2016 09:34:57 -0500 Received: from dougal.metanate.com ([90.155.101.14]:46289 "EHLO metanate.com" rhost-flags-OK-OK-OK-FAIL) by vger.kernel.org with ESMTP id S1752035AbcAMOe4 (ORCPT ); Wed, 13 Jan 2016 09:34:56 -0500 Date: Wed, 13 Jan 2016 14:34:25 +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: <20160113143425.363ce07a.john@metanate.com> In-Reply-To: <20160113142320.GQ19130@phenom.ffwll.local> References: <1452689614-6570-1-git-send-email-john@metanate.com> <20160113142320.GQ19130@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 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(). > > + > > static void > > -rockchip_atomic_wait_for_complete(struct drm_atomic_state > > *old_state) +rockchip_atomic_wait_for_complete(struct drm_device > > *dev, struct drm_atomic_state *old_state) { > > struct drm_crtc_state *old_crtc_state; > > struct drm_crtc *crtc; > > @@ -194,6 +214,9 @@ rockchip_atomic_wait_for_complete(struct > > drm_atomic_state *old_state) if (!crtc->state->active) > > continue; > > > > + if (!framebuffer_changed(dev, old_state, crtc)) > > + continue; > > + > > ret = drm_crtc_vblank_get(crtc); > > if (ret != 0) > > continue; > > @@ -241,7 +264,7 @@ rockchip_atomic_commit_complete(struct > > rockchip_atomic_commit *commit) > > drm_atomic_helper_commit_planes(dev, state, true); > > > > - rockchip_atomic_wait_for_complete(state); > > + rockchip_atomic_wait_for_complete(dev, state); > > > > drm_atomic_helper_cleanup_planes(dev, state); > > > > -- > > 2.7.0.rc3.140.g520a093 > > > > _______________________________________________ > > dri-devel mailing list > > dri-devel@lists.freedesktop.org > > http://lists.freedesktop.org/mailman/listinfo/dri-devel >