From mboxrd@z Thu Jan 1 00:00:00 1970 From: Robert Foss Subject: Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event. Date: Tue, 3 May 2016 10:20:14 -0400 Message-ID: <5728B39E.3070104@collabora.com> References: <1462217122-13071-1-git-send-email-robert.foss@collabora.com> <1462217122-13071-2-git-send-email-robert.foss@collabora.com> <87wpncot4u.fsf@eliezer.anholt.net> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from bhuna.collabora.co.uk (bhuna.collabora.co.uk [46.235.227.227]) by gabe.freedesktop.org (Postfix) with ESMTPS id C5A1B6E893 for ; Tue, 3 May 2016 14:20:36 +0000 (UTC) In-Reply-To: <87wpncot4u.fsf@eliezer.anholt.net> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Eric Anholt , daniel.vetter@ffwll.ch, airlied@linux.ieaniel.vetter@ffwll.ch, fengguang.wu@intel.com, maarten.lankhorst@linux.intel.com, julia.lawall@lip6.fr, alexander.deucher@amd.com, daniels@collabora.com, derekf@osg.samsung.com, varadgautam@gmail.com, linux-kernel@vger.kernel.orgmaarten.lankhorst@linux.intel.com Cc: dri-devel@lists.freedesktop.org List-Id: dri-devel@lists.freedesktop.org CgpPbiAwNS8wMi8yMDE2IDA4OjU3IFBNLCBFcmljIEFuaG9sdCB3cm90ZToKPiByb2JlcnQuZm9z c0Bjb2xsYWJvcmEuY29tIHdyaXRlczoKPgo+PiBGcm9tOiBSb2JlcnQgRm9zcyA8cm9iZXJ0LmZv c3NAY29sbGFib3JhLmNvbT4KPj4KPj4gQXMgcGVyIHRoZSBkb2NzLCBhdG9taWNfY29tbWl0IHNo b3VsZCByZXR1cm4gLUVCVVNZICJpZiBhbiBhc3ljbmhyb25vdXMKPj4gdXBkYXRlIGlzIHJlcXVl c3RlZCBhbmQgdGhlcmUgaXMgYW4gZWFybGllciB1cGRhdGUgcGVuZGluZyIuCj4KPiBOb3RlOiBk b2NzIGNpdGVkIGhlcmUgYXJlIGRybV9jcnRjLmgsIGFuZCB0aGUgd2hvbGUgcXVvdGUgaXM6Cj4K PiAJICogIC0gLUVCVVNZLCBpZiBhbiBhc3luY2hyb25vdXMgdXBkYXRlZCBpcyByZXF1ZXN0ZWQg YW5kIHRoZXJlIGlzCj4gCSAqICAgIGFuIGVhcmxpZXIgdXBkYXRlZCBwZW5kaW5nLiBEcml2ZXJz IGFyZSBhbGxvd2VkIHRvIHN1cHBvcnQgYSBxdWV1ZQo+IAkgKiAgICBvZiBvdXRzdGFuZGluZyB1 cGRhdGVzLCBidXQgY3VycmVudGx5IG5vIGRyaXZlciBzdXBwb3J0cyB0aGF0Lgo+IAkgKiAgICBO b3RlIHRoYXQgZHJpdmVycyBtdXN0IHdhaXQgZm9yIHByZWNlZGluZyB1cGRhdGVzIHRvIGNvbXBs ZXRlIGlmIGEKPiAJICogICAgc3luY2hyb25vdXMgdXBkYXRlIGlzIHJlcXVlc3RlZCwgdGhleSBh cmUgbm90IGFsbG93ZWQgdG8gZmFpbCB0aGUKPiAJICogICAgY29tbWl0IGluIHRoYXQgY2FzZS4K Pgo+PiBTaWduZWQtb2ZmLWJ5OiBSb2JlcnQgRm9zcyA8cm9iZXJ0LmZvc3NAY29sbGFib3JhLmNv bT4KPj4gLS0tCj4+ICAgZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfY3J0Yy5jIHwgIDYgKysrKysr Cj4+ICAgZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfZHJ2LmggIHwgIDEgKwo+PiAgIGRyaXZlcnMv Z3B1L2RybS92YzQvdmM0X2ttcy5jICB8IDIwICsrKysrKysrKysrKysrKysrKy0tCj4+ICAgMyBm aWxlcyBjaGFuZ2VkLCAyNSBpbnNlcnRpb25zKCspLCAyIGRlbGV0aW9ucygtKQo+Pgo+PiBkaWZm IC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfY3J0Yy5jIGIvZHJpdmVycy9ncHUvZHJt L3ZjNC92YzRfY3J0Yy5jCj4+IGluZGV4IDM1NWVlNGIuLjQzMTkzYTMgMTAwNjQ0Cj4+IC0tLSBh L2RyaXZlcnMvZ3B1L2RybS92YzQvdmM0X2NydGMuYwo+PiArKysgYi9kcml2ZXJzL2dwdS9kcm0v dmM0L3ZjNF9jcnRjLmMKPj4gQEAgLTgwMiwzICs4MDIsOSBAQCBzdHJ1Y3QgcGxhdGZvcm1fZHJp dmVyIHZjNF9jcnRjX2RyaXZlciA9IHsKPj4gICAJCS5vZl9tYXRjaF90YWJsZSA9IHZjNF9jcnRj X2R0X21hdGNoLAo+PiAgIAl9LAo+PiAgIH07Cj4+ICsKPj4gK2Jvb2wgdmM0X2NydGNfaGFzX3Bl bmRpbmdfZXZlbnQoc3RydWN0IGRybV9jcnRjICpjcnRjKQo+PiArewo+PiArCWFzc2VydF9zcGlu X2xvY2tlZCgmY3J0Yy0+ZGV2LT5ldmVudF9sb2NrKTsKPj4gKwlyZXR1cm4gdG9fdmM0X2NydGMo Y3J0YyktPmV2ZW50Owo+PiArfQo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3ZjNC92 YzRfZHJ2LmggYi9kcml2ZXJzL2dwdS9kcm0vdmM0L3ZjNF9kcnYuaAo+PiBpbmRleCBmYTJhZDE1 Li41NGMxZmI1IDEwMDY0NAo+PiAtLS0gYS9kcml2ZXJzL2dwdS9kcm0vdmM0L3ZjNF9kcnYuaAo+ PiArKysgYi9kcml2ZXJzL2dwdS9kcm0vdmM0L3ZjNF9kcnYuaAo+PiBAQCAtNDE0LDYgKzQxNCw3 IEBAIGV4dGVybiBzdHJ1Y3QgcGxhdGZvcm1fZHJpdmVyIHZjNF9jcnRjX2RyaXZlcjsKPj4gICBp bnQgdmM0X2VuYWJsZV92Ymxhbmsoc3RydWN0IGRybV9kZXZpY2UgKmRldiwgdW5zaWduZWQgaW50 IGNydGNfaWQpOwo+PiAgIHZvaWQgdmM0X2Rpc2FibGVfdmJsYW5rKHN0cnVjdCBkcm1fZGV2aWNl ICpkZXYsIHVuc2lnbmVkIGludCBjcnRjX2lkKTsKPj4gICBpbnQgdmM0X2NydGNfZGVidWdmc19y ZWdzKHN0cnVjdCBzZXFfZmlsZSAqbSwgdm9pZCAqYXJnKTsKPj4gK2Jvb2wgdmM0X2NydGNfaGFz X3BlbmRpbmdfZXZlbnQoc3RydWN0IGRybV9jcnRjICpjcnRjKTsKPj4KPj4gICAvKiB2YzRfZGVi dWdmcy5jICovCj4+ICAgaW50IHZjNF9kZWJ1Z2ZzX2luaXQoc3RydWN0IGRybV9taW5vciAqbWlu b3IpOwo+PiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfa21zLmMgYi9kcml2 ZXJzL2dwdS9kcm0vdmM0L3ZjNF9rbXMuYwo+PiBpbmRleCA0NzE4YWU1Li5kYzE1N2ExZSAxMDA2 NDQKPj4gLS0tIGEvZHJpdmVycy9ncHUvZHJtL3ZjNC92YzRfa21zLmMKPj4gKysrIGIvZHJpdmVy cy9ncHUvZHJtL3ZjNC92YzRfa21zLmMKPj4gQEAgLTEwNywxMCArMTA3LDI2IEBAIHN0YXRpYyBp bnQgdmM0X2F0b21pY19jb21taXQoc3RydWN0IGRybV9kZXZpY2UgKmRldiwKPj4gICAJCQkgICAg IGJvb2wgYXN5bmMpCj4+ICAgewo+PiAgIAlzdHJ1Y3QgdmM0X2RldiAqdmM0ID0gdG9fdmM0X2Rl dihkZXYpOwo+PiAtCWludCByZXQ7Cj4+IC0JaW50IGk7Cj4+ICAgCXVpbnQ2NF90IHdhaXRfc2Vx bm8gPSAwOwo+PiAgIAlzdHJ1Y3QgdmM0X2NvbW1pdCAqYzsKPj4gKwlzdHJ1Y3QgZHJtX2NydGNf c3RhdGUgKmNydGNfc3RhdGU7Cj4+ICsJc3RydWN0IGRybV9jcnRjICpjcnRjOwo+PiArCXVuc2ln bmVkIGxvbmcgZmxhZ3M7Cj4+ICsJaW50IGksIHJldDsKPj4gKwo+PiArCWlmIChhc3luYykgewo+ PiArCQlmb3JfZWFjaF9jcnRjX2luX3N0YXRlKHN0YXRlLCBjcnRjLCBjcnRjX3N0YXRlLCBpKSB7 Cj4+ICsKPj4gKwkJCXNwaW5fbG9ja19pcnFzYXZlKCZkZXYtPmV2ZW50X2xvY2ssIGZsYWdzKTsK Pj4gKwo+PiArCQkJaWYgKGNydGMtPnN0YXRlLT5ldmVudCB8fAo+PiArCQkJCXZjNF9jcnRjX2hh c19wZW5kaW5nX2V2ZW50KGNydGMpKSB7Cj4+ICsJCQkJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgm ZGV2LT5ldmVudF9sb2NrLCBmbGFncyk7Cj4+ICsJCQkJcmV0dXJuIC1FQlVTWTsKPj4gKwkJCX0K Pj4gKwkJCXNwaW5fdW5sb2NrX2lycXJlc3RvcmUoJmRldi0+ZXZlbnRfbG9jaywgZmxhZ3MpOwo+ PiArCQl9Cj4+ICsJfQo+Cj4gQnkgbXkgcmVhZGluZywgdGhlIHBvaW50IG9mIHJldHVybmluZyAt RUJVU1kgaGVyZSBpcyBqdXN0IHRvIGF2b2lkCj4gYmxvY2tpbmcgd2hlbiB0aGUgY29tcG9zaXRv ciBhc2tlZCBmb3Igbm9uYmxvY2tpbmcuICBZb3UncmUgY2hlY2tpbmcKPiB0aGF0IGVhY2ggQ1JU QyBpbnZvbHZlZCBpbiB0aGUgY29tbWl0IGRvZXNuJ3QgaGF2ZSBhIHF1ZXVlZCBwYWdlZmxpcAo+ IGV2ZW50LCBleGNlcHQgdGhhdCB0aGVuIHdlIGltbWVkaWF0ZWx5IHByb2NlZWQgdG86Cj4KPiAJ LyogTWFrZSBzdXJlIHRoYXQgYW55IG91dHN0YW5kaW5nIG1vZGVzZXRzIGhhdmUgZmluaXNoZWQu ICovCj4gCXJldCA9IGRvd25faW50ZXJydXB0aWJsZSgmdmM0LT5hc3luY19tb2Rlc2V0KTsKPiAJ aWYgKHJldCkgewo+IAkJa2ZyZWUoYyk7Cj4gCQlyZXR1cm4gcmV0Owo+IAl9Cj4KPiBzbyB0aGlz IHBlci1DUlRDIGNoZWNrIGRvZXNuJ3QgcHJldmVudCBibG9ja2luZyBpZiB0aGUgc2V0IG9mIENS VENzIGZvcgo+IHRoaXMgY29tbWl0IHdhcyBkaXNqb2ludCBmcm9tIHRoZSBsYXN0IG9uZSwgcmln aHQ/ICBUbyBpbXBsZW1lbnQgdGhlCj4gbWluaW1hbCBiZWhhdmlvciwgSSB0aGluayB3ZSdkIHdh bnQgdG8ganVzdCBkb3duX3RyeWxvY2sgaW5zdGVhZCBpbiB0aGUKPiBhc3luYyBjYXNlLCBJIHRo aW5rLiAgQW5kIHRvIGltcGxlbWVudCB0aGUgZGlzam9pbnQgQ1JUQyB0aGluZyB5b3Ugd2VyZQo+ IHRyeWluZyBmb3IsIEkgdGhpbmsgd2UnZCBuZWVkIHRvIHRyYWNrIGEgbWFzayBraW5kIG9mIGxp a2UgbXNtJ3MKPiBwZW5kaW5nX2NydGNzLgo+CgpTbyB3aGF0IHlvdSdyZSBzYXlpbmcgaXMgdGhh dCB0aGUgc2V0IG9mIENSVENzIGluIHN0YXRlCm1pZ2h0IG5vdCBjb250YWluIGFsbCBDUlRDcyBh bmQgdGhhdCB0aGlzIGNoZWNrIG1pZ2h0IGJlIGluY29tcGxldGUgZm9yIAp0aGF0IHJlYXNvbj8K CkknbSBmYWlybHkgbmV3IHRvIHRoZXNlIHdhdGVycywgc28gZG9uJ3QgaGVzaXRhdGUgdG8gdGVs bCBtZSBpZiBJIHNlZW0KdG8gYmUgbWlzdW5kZXJzdGFuZGluZyBzb21ldGhpbmcgb3IgYW0gb24g YSB3aWxkIGdvb3NlIGNoYXNlIG9mIHNvbWUgc29ydC4KClNvIHlvdSdyZSBzdWdnZXN0aW5nIHNv bWV0aGluZyBsaWtlIHRoaXMgaW5zdGVhZCBvZgp0aGUgcGVyIENSVEMgY2hlY2s6CgotIAkvKiBN YWtlIHN1cmUgdGhhdCBhbnkgb3V0c3RhbmRpbmcgbW9kZXNldHMgaGF2ZSBmaW5pc2hlZC4gKi8K LSAJcmV0ID0gZG93bl9pbnRlcnJ1cHRpYmxlKCZ2YzQtPmFzeW5jX21vZGVzZXQpOwotIAlpZiAo cmV0KSB7Ci0gCQlrZnJlZShjKTsKLSAJCXJldHVybiByZXQ7Ci0gCX0KKyAJLyogTWFrZSBzdXJl IHRoYXQgYW55IG91dHN0YW5kaW5nIG1vZGVzZXRzIGhhdmUgZmluaXNoZWQuICovCisgCXJldCA9 IGRvd25fdHJ5bG9jaygmdmM0LT5hc3luY19tb2Rlc2V0KTsKKyAJaWYgKHJldCkgeworIAkJa2Zy ZWUoYyk7CisgCQlyZXR1cm4gLUVCVVNZOworIAl9CgpJJ3ZlIHF1aWNrbHkgdGVzdGVkIHRoZSBh Ym92ZSBwYXRjaCBhbmQgaXQgZG9lcyBzZWVtIHRvIHdvcmsgYW5kIGZ1bGZpbGwgCmFsbCBvZiBt eSByZXF1aXJlbWVudHMuCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9w Lm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1k ZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755986AbcECOUi (ORCPT ); Tue, 3 May 2016 10:20:38 -0400 Received: from bhuna.collabora.co.uk ([46.235.227.227]:60798 "EHLO bhuna.collabora.co.uk" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755316AbcECOUg (ORCPT ); Tue, 3 May 2016 10:20:36 -0400 Subject: Re: [PATCH] drm/vc4: Return -EBUSY if there's already a pending flip event. To: Eric Anholt , daniel.vetter@ffwll.ch, airlied@linux.ie, aniel.vetter@ffwll.ch, fengguang.wu@intel.com, maarten.lankhorst@linux.intel.com, julia.lawall@lip6.fr, alexander.deucher@amd.com, daniels@collabora.com, derekf@osg.samsung.com, varadgautam@gmail.com, linux-kernel@vger.kernel.org, maarten.lankhorst@linux.intel.com References: <1462217122-13071-1-git-send-email-robert.foss@collabora.com> <1462217122-13071-2-git-send-email-robert.foss@collabora.com> <87wpncot4u.fsf@eliezer.anholt.net> Cc: dri-devel@lists.freedesktop.org From: Robert Foss Message-ID: <5728B39E.3070104@collabora.com> Date: Tue, 3 May 2016 10:20:14 -0400 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.7.2 MIME-Version: 1.0 In-Reply-To: <87wpncot4u.fsf@eliezer.anholt.net> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 7bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On 05/02/2016 08:57 PM, Eric Anholt wrote: > robert.foss@collabora.com writes: > >> From: Robert Foss >> >> As per the docs, atomic_commit should return -EBUSY "if an asycnhronous >> update is requested and there is an earlier update pending". > > Note: docs cited here are drm_crtc.h, and the whole quote is: > > * - -EBUSY, if an asynchronous updated is requested and there is > * an earlier updated pending. Drivers are allowed to support a queue > * of outstanding updates, but currently no driver supports that. > * Note that drivers must wait for preceding updates to complete if a > * synchronous update is requested, they are not allowed to fail the > * commit in that case. > >> Signed-off-by: Robert Foss >> --- >> drivers/gpu/drm/vc4/vc4_crtc.c | 6 ++++++ >> drivers/gpu/drm/vc4/vc4_drv.h | 1 + >> drivers/gpu/drm/vc4/vc4_kms.c | 20 ++++++++++++++++++-- >> 3 files changed, 25 insertions(+), 2 deletions(-) >> >> diff --git a/drivers/gpu/drm/vc4/vc4_crtc.c b/drivers/gpu/drm/vc4/vc4_crtc.c >> index 355ee4b..43193a3 100644 >> --- a/drivers/gpu/drm/vc4/vc4_crtc.c >> +++ b/drivers/gpu/drm/vc4/vc4_crtc.c >> @@ -802,3 +802,9 @@ struct platform_driver vc4_crtc_driver = { >> .of_match_table = vc4_crtc_dt_match, >> }, >> }; >> + >> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc) >> +{ >> + assert_spin_locked(&crtc->dev->event_lock); >> + return to_vc4_crtc(crtc)->event; >> +} >> diff --git a/drivers/gpu/drm/vc4/vc4_drv.h b/drivers/gpu/drm/vc4/vc4_drv.h >> index fa2ad15..54c1fb5 100644 >> --- a/drivers/gpu/drm/vc4/vc4_drv.h >> +++ b/drivers/gpu/drm/vc4/vc4_drv.h >> @@ -414,6 +414,7 @@ extern struct platform_driver vc4_crtc_driver; >> int vc4_enable_vblank(struct drm_device *dev, unsigned int crtc_id); >> void vc4_disable_vblank(struct drm_device *dev, unsigned int crtc_id); >> int vc4_crtc_debugfs_regs(struct seq_file *m, void *arg); >> +bool vc4_crtc_has_pending_event(struct drm_crtc *crtc); >> >> /* vc4_debugfs.c */ >> int vc4_debugfs_init(struct drm_minor *minor); >> diff --git a/drivers/gpu/drm/vc4/vc4_kms.c b/drivers/gpu/drm/vc4/vc4_kms.c >> index 4718ae5..dc157a1e 100644 >> --- a/drivers/gpu/drm/vc4/vc4_kms.c >> +++ b/drivers/gpu/drm/vc4/vc4_kms.c >> @@ -107,10 +107,26 @@ static int vc4_atomic_commit(struct drm_device *dev, >> bool async) >> { >> struct vc4_dev *vc4 = to_vc4_dev(dev); >> - int ret; >> - int i; >> uint64_t wait_seqno = 0; >> struct vc4_commit *c; >> + struct drm_crtc_state *crtc_state; >> + struct drm_crtc *crtc; >> + unsigned long flags; >> + int i, ret; >> + >> + if (async) { >> + for_each_crtc_in_state(state, crtc, crtc_state, i) { >> + >> + spin_lock_irqsave(&dev->event_lock, flags); >> + >> + if (crtc->state->event || >> + vc4_crtc_has_pending_event(crtc)) { >> + spin_unlock_irqrestore(&dev->event_lock, flags); >> + return -EBUSY; >> + } >> + spin_unlock_irqrestore(&dev->event_lock, flags); >> + } >> + } > > By my reading, the point of returning -EBUSY here is just to avoid > blocking when the compositor asked for nonblocking. You're checking > that each CRTC involved in the commit doesn't have a queued pageflip > event, except that then we immediately proceed to: > > /* Make sure that any outstanding modesets have finished. */ > ret = down_interruptible(&vc4->async_modeset); > if (ret) { > kfree(c); > return ret; > } > > so this per-CRTC check doesn't prevent blocking if the set of CRTCs for > this commit was disjoint from the last one, right? To implement the > minimal behavior, I think we'd want to just down_trylock instead in the > async case, I think. And to implement the disjoint CRTC thing you were > trying for, I think we'd need to track a mask kind of like msm's > pending_crtcs. > So what you're saying is that the set of CRTCs in state might not contain all CRTCs and that this check might be incomplete for that reason? I'm fairly new to these waters, so don't hesitate to tell me if I seem to be misunderstanding something or am on a wild goose chase of some sort. So you're suggesting something like this instead of the per CRTC check: - /* Make sure that any outstanding modesets have finished. */ - ret = down_interruptible(&vc4->async_modeset); - if (ret) { - kfree(c); - return ret; - } + /* Make sure that any outstanding modesets have finished. */ + ret = down_trylock(&vc4->async_modeset); + if (ret) { + kfree(c); + return -EBUSY; + } I've quickly tested the above patch and it does seem to work and fulfill all of my requirements.