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 17:21:56 +0100 Message-ID: <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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Content-Disposition: inline In-Reply-To: <20160113155529.6048b2c1.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 T24gV2VkLCBKYW4gMTMsIDIwMTYgYXQgMDM6NTU6MjlQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdy b3RlOgo+IE9uIFdlZCwgMTMgSmFuIDIwMTYgMTY6NDA6MDUgKzAxMDAsIERhbmllbCBWZXR0ZXIg d3JvdGU6Cj4gCj4gPiBPbiBXZWQsIEphbiAxMywgMjAxNiBhdCAwMjozNDoyNVBNICswMDAwLCBK b2huIEtlZXBpbmcgd3JvdGU6Cj4gPiA+IE9uIFdlZCwgMTMgSmFuIDIwMTYgMTU6MjM6MjAgKzAx MDAsIERhbmllbCBWZXR0ZXIgd3JvdGU6Cj4gPiA+ICAgCj4gPiA+ID4gT24gV2VkLCBKYW4gMTMs IDIwMTYgYXQgMTI6NTM6MzRQTSArMDAwMCwgSm9obiBLZWVwaW5nIHdyb3RlOiAgCj4gPiA+ID4g PiBBcyBjb21tZW50ZWQgaW4gZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpLCB1 c2Vyc3BhY2UKPiA+ID4gPiA+IHJlbGllcyBvbiBjdXJzb3IgaW9jdGxzIGJlaW5nIHVuc3luY2Vk LiAgQ29udmVydGluZyB0aGUgcm9ja2NoaXAKPiA+ID4gPiA+IGRyaXZlciB0byBhdG9taWMgaGFz IHNpZ25pZmljYW50bHkgaW1wYWN0ZWQgY3Vyc29yIHBlcmZvcm1hbmNlIGJ5Cj4gPiA+ID4gPiBt YWtpbmcgZXZlcnkgY3Vyc29yIHVwZGF0ZSB3YWl0IGZvciB2YmxhbmsuCj4gPiA+ID4gPiAKPiA+ ID4gPiA+IEJ5IHNraXBwaW5nIHRoZSB2Ymxhbmsgc3luYyB3aGVuIHRoZSBmcmFtZWJ1ZmZlciBo YXMgbm90IGNoYW5nZWQKPiA+ID4gPiA+IChhcyBpcyBkb25lIGluIGRybV9hdG9taWNfaGVscGVy X3dhaXRfZm9yX3ZibGFua3MoKSkgd2UgY2FuIGF2b2lkCj4gPiA+ID4gPiB0aGlzIGZvciB0aGUg Y29tbW9uIGNhc2Ugb2YgbW92aW5nIHRoZSBjdXJzb3IgYW5kIG9ubHkgbmVlZCB0bwo+ID4gPiA+ ID4gZGVsYXkgdGhlIGN1cnNvciBpb2N0bCB3aGVuIHRoZSBjdXJzb3IgaWNvbiBjaGFuZ2VzLgo+ ID4gPiA+ID4gCj4gPiA+ID4gPiBJIG9yaWdpbmFsbHkgaW5zZXJ0ZWQgYSBjaGVjayBvbiBsZWdh Y3lfY3Vyc29yX3VwZGF0ZSBhcyB3ZWxsLCBidXQKPiA+ID4gPiA+IHRoYXQgY2F1c2VkIGEgc3Rv cm0gb2YgaW9tbXUgcGFnZSBmYXVsdHMuICBJIGRpZG4ndCBpbnZlc3RpZ2F0ZSB0aGUKPiA+ID4g PiA+IGNhdXNlIG9mIHRob3NlIHNpbmNlIHRoaXMgY2hhbmdlIGdpdmVzIGVub3VnaCBvZiBhIHBl cmZvcm1hbmNlCj4gPiA+ID4gPiBpbXByb3ZlbWVudCBmb3IgbXkgdXNlIGNhc2UuCj4gPiA+ID4g PiAKPiA+ID4gPiA+IFRoaXMgaXMgUkZDIGJlY2F1c2Ugb2YgdGhhdCBhbmQgYmVjYXVzZSB0aGUg ZnJhbWVidWZmZXJfY2hhbmdlZCgpCj4gPiA+ID4gPiBmdW5jdGlvbiBpcyBjb3BpZWQgZnJvbSBk cm1fYXRvbWljX2hlbHBlci5jIGFzIGEgcXVpY2sgd2F5IHRvIHRlc3QKPiA+ID4gPiA+IHRoZSBy ZXN1bHQuCj4gPiA+ID4gPiAKPiA+ID4gPiA+IFNpZ25lZC1vZmYtYnk6IEpvaG4gS2VlcGluZyA8 am9obkBtZXRhbmF0ZS5jb20+Cj4gPiA+ID4gPiAtLS0KPiA+ID4gPiA+ICBkcml2ZXJzL2dwdS9k cm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMgfCAyNwo+ID4gPiA+ID4gKysrKysrKysrKysr KysrKysrKysrKysrKy0tIDEgZmlsZSBjaGFuZ2VkLCAyNSBpbnNlcnRpb25zKCspLCAyCj4gPiA+ ID4gPiBkZWxldGlvbnMoLSkKPiA+ID4gPiA+IAo+ID4gPiA+ID4gZGlmZiAtLWdpdCBhL2RyaXZl cnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hpcF9kcm1fZmIuYwo+ID4gPiA+ID4gYi9kcml2ZXJz L2dwdS9kcm0vcm9ja2NoaXAvcm9ja2NoaXBfZHJtX2ZiLmMgaW5kZXggZjc4NDQ4OC4uOGZkOTgy MQo+ID4gPiA+ID4gMTAwNjQ0IC0tLSBhL2RyaXZlcnMvZ3B1L2RybS9yb2NrY2hpcC9yb2NrY2hp cF9kcm1fZmIuYwo+ID4gPiA+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL3JvY2tjaGlwL3JvY2tj aGlwX2RybV9mYi5jCj4gPiA+ID4gPiBAQCAtMTc3LDggKzE3NywyOCBAQCBzdGF0aWMgdm9pZAo+ ID4gPiA+ID4gcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91cGRhdGUoc3RydWN0IGRybV9jcnRjICpj cnRjKQo+ID4gPiA+ID4gY3J0Y19mdW5jcy0+d2FpdF9mb3JfdXBkYXRlKGNydGMpOyB9Cj4gPiA+ ID4gPiAgCj4gPiA+ID4gPiArc3RhdGljIGJvb2wgZnJhbWVidWZmZXJfY2hhbmdlZChzdHJ1Y3Qg ZHJtX2RldmljZSAqZGV2LAo+ID4gPiA+ID4gKwkJCQlzdHJ1Y3QgZHJtX2F0b21pY19zdGF0ZSAq b2xkX3N0YXRlLAo+ID4gPiA+ID4gKwkJCQlzdHJ1Y3QgZHJtX2NydGMgKmNydGMpCj4gPiA+ID4g PiArewo+ID4gPiA+ID4gKwlzdHJ1Y3QgZHJtX3BsYW5lICpwbGFuZTsKPiA+ID4gPiA+ICsJc3Ry dWN0IGRybV9wbGFuZV9zdGF0ZSAqb2xkX3BsYW5lX3N0YXRlOwo+ID4gPiA+ID4gKwlpbnQgaTsK PiA+ID4gPiA+ICsKPiA+ID4gPiA+ICsJZm9yX2VhY2hfcGxhbmVfaW5fc3RhdGUob2xkX3N0YXRl LCBwbGFuZSwgb2xkX3BsYW5lX3N0YXRlLAo+ID4gPiA+ID4gaSkgewo+ID4gPiA+ID4gKwkJaWYg KHBsYW5lLT5zdGF0ZS0+Y3J0YyAhPSBjcnRjICYmCj4gPiA+ID4gPiArCQkgICAgb2xkX3BsYW5l X3N0YXRlLT5jcnRjICE9IGNydGMpCj4gPiA+ID4gPiArCQkJY29udGludWU7Cj4gPiA+ID4gPiAr Cj4gPiA+ID4gPiArCQlpZiAocGxhbmUtPnN0YXRlLT5mYiAhPSBvbGRfcGxhbmVfc3RhdGUtPmZi KQo+ID4gPiA+ID4gKwkJCXJldHVybiB0cnVlOwo+ID4gPiA+ID4gKwl9Cj4gPiA+ID4gPiArCj4g PiA+ID4gPiArCXJldHVybiBmYWxzZTsKPiA+ID4gPiA+ICt9ICAgIAo+ID4gPiA+IAo+ID4gPiA+ IFBsZWFzZSBkb24ndCBoYW5kLXJvbGwgbG9naWMgdGhhdCBhZmZlY3RzIHNlbWFudGljcyBsaWtl IHRoaXMuIEluc3RlYWQKPiA+ID4gPiBwbGVhc2UgdXNlIGRybV9hdG9taWNfaGVscGVyX3dhaXRf Zm9yX3ZibGFua3MoKSwgd2hpY2ggc2hvdWxkIGRvIHRoaXMKPiA+ID4gPiBjb3JyZWN0bHkgZm9y IHlvdS4KPiA+ID4gPiAKPiA+ID4gPiBJZiB0aGF0J3Mgbm90IHRoZSBjYXNlIHRoZW4gd2UgbmVl ZCB0byBpbXByb3ZlIHRoZSBnZW5lcmljIGhlbHBlciwgb3IKPiA+ID4gPiBmaWd1cmUgb3V0IHdo YXQncyBkaWZmZXJlbnQgd2l0aCByb2NraGlwLiAgCj4gPiA+IAo+ID4gPiBBY2NvcmRpbmcgdG8g Y29tbWl0IDYzZWJiOWYgKGRybS9yb2NrY2hpcDogQ29udmVydCB0byBzdXBwb3J0IGF0b21pYwo+ ID4gPiBBUEkpIGl0J3MgYmVjYXVzZSByb2NrY2hpcCBkb2Vzbid0IGhhdmUgYSBoYXJkd2FyZSB2 YmxhbmsgY291bnRlci4KPiA+ID4gCj4gPiA+IEknbSBub3QgZW50aXJlbHkgY2xlYXIgb24gd2h5 IHRoaXMgcHJldmVudHMgdGhlIHVzZSBvZgo+ID4gPiBkcm1fYXRvbWljX2hlbHBlcl93YWl0X2Zv cl92YmxhbmtzKCkuICAKPiA+IAo+ID4gSG0sIHRoYXQgY29tbWl0IGlzbid0IHRlcnJpYmx5IGhl bHBmdWwuIElmIHRoYXQncyByZWFsbHkgbmVlZGVkIHRoZW4gaW1vIEkKPiA+IHRoaW5rIHdlIHNo b3VsZCBleHRyYWN0IGEgImRybV9hdG9taWNfaGVscGVyX3BsYW5lX25lZWRzX3ZibGFua193YWl0 KCkiCj4gPiBoZWxwZXIgdGhhdCdzIHVzZWQgYnkgYm90aC4gQnV0IHNpbmNlIHJvY2tjaGlwIGRv ZXMgdmJsYW5rX2dldC9wdXQgY2FsbHMKPiA+IEknZCBob3BlIHZibGFua3MgYWN0dWFsbHkgd29y ayBjb3JyZWN0bHkuIEFuZCB0aGVuIHRoZSBoZWxwZXIgc2hvdWxkIHdvcmsKPiA+IHRvby4KPiAK PiBJIHRyaWVkIHN3aXRjaGluZyB0aGUgY2FsbCB0byByb2NrY2hpcF9jcnRjX3dhaXRfZm9yX3Vw ZGF0ZSgpIHRvCj4gZHJtX2F0b21pY19oZWxwZXJfd2FpdF9mb3JfdmJsYW5rcygpIGFuZCBpdCB3 b3JrcyBmaW5lIHVudGlsIEkgc3dpdGNoCj4gdGhlIGJ1ZmZlciBhc3NvY2lhdGVkIHdpdGggYSBj dXJzb3IsIGF0IHdoaWNoIHBvaW50IEkgZ2V0IGlvbW11IHBhZ2UKPiBmYXVsdHMsIHByZXN1bWFi bHkgYmVjYXVzZSB0aGUgR0VNIGJ1ZmZlciBpcyB1bnJlZmVyZW5jZWQgdG9vIGVhcmx5Lgo+IAo+ IEFGQUlDVCB0aGUgYnVmZmVyIHdpbGwgYmUgcmVsZWFzZWQgdmlhIGRybV9hdG9taWNfc3RhdGVf ZnJlZSgpCj4gdW5jb25kaXRpb25hbGx5LCBidXQgSSBzdXNwZWN0IEknbSBtaXNzaW5nIHNvbWV0 aGluZyBzaW5jZSB0aGF0IHdvdWxkCj4gbWVhbiBldmVyeSBkcml2ZXIgd291bGQgaGl0IGEgc2lt aWxhciBwcm9ibGVtLgoKWWVhaCwgd2l0aCB0aGUgaGVscGVyIHdlIGFsd2F5cyBza2lwLCB3aGlj aCBtZWFucyB3aGVuIHRoZSBjdXJzb3IgYm8KY2hhbmdlcyB5b3UgaW5kZWVkIHVubWFwIHRvbyBl YXJseS4gU28gY2FuJ3QgZXZlbiBzaGFyZSB0aGUgb3ZlcmFsbApjb25kaXRpb24sIGJ1dCB3ZSBj b3VsZCBkZWZpbml0ZWx5IHNoYXJlIHRoZSBsaXR0bGUgZnJhbWVidWZmZXJfY2hhbmdlZApoZWxw ZXIuIFBsdXMgcm9ja2NoaXBfY3J0Y193YWl0X2Zvcl91cGRhdGUgc2hvdWxkIGhhdmUgYSBiaWcg Y29tbWVudApleHBsYWluaW5nIHdoeSB3ZSBoYXZlIGRpZmZlcmVudCBydWxlcyB0aGFuIGNvcmUg aGVscGVycyEKCkNoZWVycywgRGFuaWVsCi0tIApEYW5pZWwgVmV0dGVyClNvZnR3YXJlIEVuZ2lu ZWVyLCBJbnRlbCBDb3Jwb3JhdGlvbgpodHRwOi8vYmxvZy5mZndsbC5jaApfX19fX19fX19fX19f X19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0 CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKaHR0cDovL2xpc3RzLmZyZWVkZXNrdG9w Lm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 From: daniel@ffwll.ch (Daniel Vetter) Date: Wed, 13 Jan 2016 17:21:56 +0100 Subject: [RFC/PATCH] drm/rockchip: don't wait for vblank if fb hasn't changed In-Reply-To: <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> <20160113155529.6048b2c1.john@metanate.com> Message-ID: <20160113162156.GA19130@phenom.ffwll.local> To: linux-arm-kernel@lists.infradead.org List-Id: linux-arm-kernel.lists.infradead.org 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. Plus rockchip_crtc_wait_for_update should have a big comment explaining why we have different rules than core helpers! Cheers, 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 S1753568AbcAMQV5 (ORCPT ); Wed, 13 Jan 2016 11:21:57 -0500 Received: from mail-wm0-f54.google.com ([74.125.82.54]:36639 "EHLO mail-wm0-f54.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752665AbcAMQVz (ORCPT ); Wed, 13 Jan 2016 11:21:55 -0500 Date: Wed, 13 Jan 2016 17:21:56 +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: <20160113162156.GA19130@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> MIME-Version: 1.0 Content-Type: text/plain; charset=us-ascii Content-Disposition: inline In-Reply-To: <20160113155529.6048b2c1.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 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. Plus rockchip_crtc_wait_for_update should have a big comment explaining why we have different rules than core helpers! Cheers, Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch