From mboxrd@z Thu Jan 1 00:00:00 1970 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Date: Mon, 13 Feb 2017 16:45:33 +0200 Message-ID: <20170213144533.GI31595@intel.com> References: <20170210140605.GC31595@intel.com> <20170213103518.6xck2ag7bwgdjgki@lukather> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mga11.intel.com (mga11.intel.com [192.55.52.93]) by gabe.freedesktop.org (Postfix) with ESMTPS id BD84B6E41D for ; Mon, 13 Feb 2017 14:45:38 +0000 (UTC) Content-Disposition: inline In-Reply-To: <20170213103518.6xck2ag7bwgdjgki@lukather> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Maxime Ripard Cc: linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org, Daniel Vetter , Stefan Christ List-Id: dri-devel@lists.freedesktop.org T24gTW9uLCBGZWIgMTMsIDIwMTcgYXQgMTE6MzU6MThBTSArMDEwMCwgTWF4aW1lIFJpcGFyZCB3 cm90ZToKPiBIaSBWaWxsZSwKPiAKPiBPbiBGcmksIEZlYiAxMCwgMjAxNyBhdCAwNDowNjowNVBN ICswMjAwLCBWaWxsZSBTeXJqw6Rsw6Qgd3JvdGU6Cj4gPiBPbiBUaHUsIEZlYiAwMiwgMjAxNyBh dCAxMTozMTo1N0FNICswMTAwLCBNYXhpbWUgUmlwYXJkIHdyb3RlOgo+ID4gPiBGcm9tOiBTdGVm YW4gQ2hyaXN0IDxzLmNocmlzdEBwaHl0ZWMuZGU+Cj4gPiA+IAo+ID4gPiBJbXBsZW1lbnQgbGVn YWN5IGZyYW1lYnVmZmVyIGlvY3RsIEZCSU9fV0FJVEZPUlZTWU5DIGluIHRoZSBnZW5lcmljCj4g PiA+IGZyYW1lYnVmZmVyIGVtdWxhdGlvbiBkcml2ZXIuIExlZ2FjeSBmcmFtZWJ1ZmZlciB1c2Vy cyBsaWtlIG5vbiBrbXMvZHJtCj4gPiA+IGJhc2VkIE9wZW5HTChFUykvRUdMIGltcGxlbWVudGF0 aW9ucyBtYXkgcmVxdWlyZSB0aGUgaW9jdGwgdG8KPiA+ID4gc3luY2hyb25pemUgZHJhd2luZyBv ciBidWZmZXIgZmxpcCBmb3IgZG91YmxlIGJ1ZmZlcmluZy4gSXQgaXMgdGVzdGVkIG9uCj4gPiA+ IHRoZSBpLk1YNi4KPiA+ID4gCj4gPiA+IENvZGUgaXMgYmFzZWQgb24KPiA+ID4gICAgIGh0dHBz Oi8vZ2l0aHViLmNvbS9YaWxpbngvbGludXgteGxueC9ibG9iL21hc3Rlci9kcml2ZXJzL2dwdS9k cm0veGlsaW54L3hpbGlueF9kcm1fZmIuYyNMMTk2Cj4gPiA+IAo+ID4gPiBTaWduZWQtb2ZmLWJ5 OiBTdGVmYW4gQ2hyaXN0IDxzLmNocmlzdEBwaHl0ZWMuZGU+Cj4gPiA+IFNpZ25lZC1vZmYtYnk6 IE1heGltZSBSaXBhcmQgPG1heGltZS5yaXBhcmRAZnJlZS1lbGVjdHJvbnMuY29tPgo+ID4gPiAt LS0KPiA+ID4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgfCA1NSArKysrKysrKysr KysrKysrKysrKysrKysrKysrKysrKysrLQo+ID4gPiAgaW5jbHVkZS9kcm0vZHJtX2ZiX2hlbHBl ci5oICAgICB8ICA1ICsrLQo+ID4gPiAgMiBmaWxlcyBjaGFuZ2VkLCA1OSBpbnNlcnRpb25zKCsp LCAxIGRlbGV0aW9uKC0pCj4gPiA+IAo+ID4gPiBkaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJt L2RybV9mYl9oZWxwZXIuYyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiA+ID4g aW5kZXggZTkzNGI1NDFmZWVhLi4zOWEzNTMyZTMxMWMgMTAwNjQ0Cj4gPiA+IC0tLSBhL2RyaXZl cnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiA+ID4gKysrIGIvZHJpdmVycy9ncHUvZHJtL2Ry bV9mYl9oZWxwZXIuYwo+ID4gPiBAQCAtMTIzNCw2ICsxMjM0LDYxIEBAIGludCBkcm1fZmJfaGVs cGVyX3NldGNtYXAoc3RydWN0IGZiX2NtYXAgKmNtYXAsIHN0cnVjdCBmYl9pbmZvICppbmZvKQo+ ID4gPiAgRVhQT1JUX1NZTUJPTChkcm1fZmJfaGVscGVyX3NldGNtYXApOwo+ID4gPiAgCj4gPiA+ ICAvKioKPiA+ID4gKyAqIGRybV9mYl9oZWxwZXJfaW9jdGwgLSBsZWdhY3kgaW9jdGwgaW1wbGVt ZW50YXRpb24KPiA+ID4gKyAqIEBpbmZvOiBmYmRldiByZWdpc3RlcmVkIGJ5IHRoZSBoZWxwZXIK PiA+ID4gKyAqIEBjbWQ6IGlvY3RsIGNvbW1hbmQKPiA+ID4gKyAqIEBhcmc6IGlvY3RsIGFyZ3Vt ZW50Cj4gPiA+ICsgKgo+ID4gPiArICogQSBoZWxwZXIgdG8gaW1wbGVtZW50IHRoZSBzdGFuZGFy ZCBmYmRldiBpb2N0bC4gT25seQo+ID4gPiArICogRkJJT19XQUlURk9SVlNZTkMgaXMgaW1wbGVt ZW50ZWQgZm9yIG5vdy4KPiA+ID4gKyAqLwo+ID4gPiAraW50IGRybV9mYl9oZWxwZXJfaW9jdGwo c3RydWN0IGZiX2luZm8gKmluZm8sIHVuc2lnbmVkIGludCBjbWQsIHVuc2lnbmVkIGxvbmcgYXJn KQo+ID4gPiArewo+ID4gPiArCXN0cnVjdCBkcm1fZmJfaGVscGVyICpmYl9oZWxwZXIgPSBpbmZv LT5wYXI7Cj4gPiA+ICsJc3RydWN0IGRybV9kZXZpY2UgKmRldiA9IGZiX2hlbHBlci0+ZGV2Owo+ ID4gPiArCXVuc2lnbmVkIGludCBpOwo+ID4gPiArCWludCByZXQgPSAwOwo+ID4gPiArCj4gPiA+ ICsJZHJtX21vZGVzZXRfbG9ja19hbGwoZGV2KTsKPiA+ID4gKwlpZiAoIWRybV9mYl9oZWxwZXJf aXNfYm91bmQoZmJfaGVscGVyKSkgewo+ID4gPiArCQlkcm1fbW9kZXNldF91bmxvY2tfYWxsKGRl dik7Cj4gPiA+ICsJCXJldHVybiAtRUJVU1k7Cj4gPiA+ICsJfQo+ID4gPiArCj4gPiA+ICsJc3dp dGNoIChjbWQpIHsKPiA+ID4gKwljYXNlIEZCSU9fV0FJVEZPUlZTWU5DOgo+ID4gPiArCQlmb3Ig KGkgPSAwOyBpIDwgZmJfaGVscGVyLT5jcnRjX2NvdW50OyBpKyspIHsKPiA+IAo+ID4gRkJJT19X QUlURk9SVlNZTkMgdGFrZXMgdGhlIGNydGMgYXMgYSBwYXJtZXRlciwgc28gSSdtIG5vdCBzdXJl IHdlIHdhbnQKPiA+IHRvIGRvIHRoaXMgZm9yIGFsbCB0aGUgY3J0Y3MuIFRob3VnaCB3aGF0IHRo YXQgY3J0YyBtZWFucyBmb3IgZmIgaXMKPiA+IHJhdGhlciBwb29ybHkgZGVmaW5lZC4KPiAKPiBJ IGd1ZXNzIEkgY291bGQganVzdCB1c2UgdGhhdCBpbmRleCB0byByZXRyaWV2ZSBvbmx5IHRoZSBy aWdodCBDUlRDIGluCj4gZmJfaGVscGVyLT5jcnRjX2luZm8uCj4gCj4gPiAKPiA+ID4gKwkJCXN0 cnVjdCBkcm1fbW9kZV9zZXQgKm1vZGVfc2V0Owo+ID4gPiArCQkJc3RydWN0IGRybV9jcnRjICpj cnRjOwo+ID4gPiArCj4gPiA+ICsJCQltb2RlX3NldCA9ICZmYl9oZWxwZXItPmNydGNfaW5mb1tp XS5tb2RlX3NldDsKPiA+ID4gKwkJCWNydGMgPSBtb2RlX3NldC0+Y3J0YzsKPiA+ID4gKwo+ID4g PiArCQkJLyoKPiA+ID4gKwkJCSAqIE9ubHkgY2FsbCBkcm1fY3J0Y193YWl0X29uZV92Ymxhbmsg Zm9yIGNydGNzIHRoYXQKPiA+ID4gKwkJCSAqIGFyZSBjdXJyZW50bHkgZW5hYmxlZC4KPiA+ID4g KwkJCSAqLwo+ID4gPiArCQkJaWYgKCFjcnRjLT5lbmFibGVkKQo+ID4gPiArCQkJCWNvbnRpbnVl Owo+ID4gPiArCj4gPiA+ICsJCQlyZXQgPSBkcm1fY3J0Y192YmxhbmtfZ2V0KGNydGMpOwo+ID4g PiArCQkJaWYgKCFyZXQpIHsKPiA+ID4gKwkJCQlkcm1fY3J0Y193YWl0X29uZV92YmxhbmsoY3J0 Yyk7Cj4gPiA+ICsJCQkJZHJtX2NydGNfdmJsYW5rX3B1dChjcnRjKTsKPiA+ID4gKwkJCX0KPiA+ IAo+ID4gVGhpcyBsb29rcyBxdWl0ZSBzdWItb3B0aW1hbC4gSXQgc2hvdWxkIHJhdGhlciBkbyBz b21ldGhpbmcgYWxvbmcgdGhlCj4gPiBsaW5lcyBvZiB3aGF0IGRybV9hdG9taWNfaGVscGVyX3dh aXRfZm9yX3ZibGFua3MoKSBkb2VzLgo+IAo+IEhvdyBpcyB0aGF0IHN1Ym9wdGltYWw/CgpZb3Un cmUgc2VyaWFsaXppbmcgdGhlIHdhaXRzIHJhdGhlciB0aGFuIGRvaW5nIHRoZW0gaW4gcGFyYWxs ZWwuCgpMZXQncyBsb29rIGF0IGEgc2ltcGxlIHRocmVlIGNydGMgZXhhbXBsZSAofD12Ymxhbmsp OgoKICAgICAgICB0aW1lIC0tPgpDUlRDIDE6ICAgICB8ICAgICB8ICAgICB8ICAgICB8CkNSVEMg MjogICB8ICAgICB8ICAgICB8ICAgICB8CkNSVEMgMzogfCAgICAgfCAgICAgfCAgICAgfAoKWW91 ciBjb2RlIHdhaXRzIHVudGlsIGhlcmU6CiAgICAgICAgICAgIHwgICB8ICAgfAogICAgICAgICAg ICAxICAgMiAgIDMKICAgICAgICAgICAgICAgICAgICBeCgpPcHRpbWFsIGNvZGUgd291bGQgd2Fp dCB1bnRpbCBoZXJlOgogICAgICAgICAgICB8CiAgICAgICAgICAgIF4KIAotLSAKVmlsbGUgU3ly asOkbMOkCkludGVsIE9UQwpfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3Rv cC5vcmcKaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmkt ZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752674AbdBMOpv (ORCPT ); Mon, 13 Feb 2017 09:45:51 -0500 Received: from mga05.intel.com ([192.55.52.43]:61652 "EHLO mga05.intel.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752623AbdBMOpt (ORCPT ); Mon, 13 Feb 2017 09:45:49 -0500 X-ExtLoop1: 1 X-IronPort-AV: E=Sophos;i="5.35,156,1484035200"; d="scan'208";a="1106760203" Date: Mon, 13 Feb 2017 16:45:33 +0200 From: Ville =?iso-8859-1?Q?Syrj=E4l=E4?= To: Maxime Ripard Cc: Daniel Vetter , David Airlie , Jani Nikula , Sean Paul , Stefan Christ , linux-kernel@vger.kernel.org, dri-devel@lists.freedesktop.org Subject: Re: [PATCH v2 2/2] drm/fb_helper: implement ioctl FBIO_WAITFORVSYNC Message-ID: <20170213144533.GI31595@intel.com> References: <20170210140605.GC31595@intel.com> <20170213103518.6xck2ag7bwgdjgki@lukather> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <20170213103518.6xck2ag7bwgdjgki@lukather> 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 Mon, Feb 13, 2017 at 11:35:18AM +0100, Maxime Ripard wrote: > Hi Ville, > > On Fri, Feb 10, 2017 at 04:06:05PM +0200, Ville Syrjälä wrote: > > On Thu, Feb 02, 2017 at 11:31:57AM +0100, Maxime Ripard wrote: > > > From: Stefan Christ > > > > > > Implement legacy framebuffer ioctl FBIO_WAITFORVSYNC in the generic > > > framebuffer emulation driver. Legacy framebuffer users like non kms/drm > > > based OpenGL(ES)/EGL implementations may require the ioctl to > > > synchronize drawing or buffer flip for double buffering. It is tested on > > > the i.MX6. > > > > > > Code is based on > > > https://github.com/Xilinx/linux-xlnx/blob/master/drivers/gpu/drm/xilinx/xilinx_drm_fb.c#L196 > > > > > > Signed-off-by: Stefan Christ > > > Signed-off-by: Maxime Ripard > > > --- > > > drivers/gpu/drm/drm_fb_helper.c | 55 ++++++++++++++++++++++++++++++++++- > > > include/drm/drm_fb_helper.h | 5 ++- > > > 2 files changed, 59 insertions(+), 1 deletion(-) > > > > > > diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > > > index e934b541feea..39a3532e311c 100644 > > > --- a/drivers/gpu/drm/drm_fb_helper.c > > > +++ b/drivers/gpu/drm/drm_fb_helper.c > > > @@ -1234,6 +1234,61 @@ int drm_fb_helper_setcmap(struct fb_cmap *cmap, struct fb_info *info) > > > EXPORT_SYMBOL(drm_fb_helper_setcmap); > > > > > > /** > > > + * drm_fb_helper_ioctl - legacy ioctl implementation > > > + * @info: fbdev registered by the helper > > > + * @cmd: ioctl command > > > + * @arg: ioctl argument > > > + * > > > + * A helper to implement the standard fbdev ioctl. Only > > > + * FBIO_WAITFORVSYNC is implemented for now. > > > + */ > > > +int drm_fb_helper_ioctl(struct fb_info *info, unsigned int cmd, unsigned long arg) > > > +{ > > > + struct drm_fb_helper *fb_helper = info->par; > > > + struct drm_device *dev = fb_helper->dev; > > > + unsigned int i; > > > + int ret = 0; > > > + > > > + drm_modeset_lock_all(dev); > > > + if (!drm_fb_helper_is_bound(fb_helper)) { > > > + drm_modeset_unlock_all(dev); > > > + return -EBUSY; > > > + } > > > + > > > + switch (cmd) { > > > + case FBIO_WAITFORVSYNC: > > > + for (i = 0; i < fb_helper->crtc_count; i++) { > > > > FBIO_WAITFORVSYNC takes the crtc as a parmeter, so I'm not sure we want > > to do this for all the crtcs. Though what that crtc means for fb is > > rather poorly defined. > > I guess I could just use that index to retrieve only the right CRTC in > fb_helper->crtc_info. > > > > > > + struct drm_mode_set *mode_set; > > > + struct drm_crtc *crtc; > > > + > > > + mode_set = &fb_helper->crtc_info[i].mode_set; > > > + crtc = mode_set->crtc; > > > + > > > + /* > > > + * Only call drm_crtc_wait_one_vblank for crtcs that > > > + * are currently enabled. > > > + */ > > > + if (!crtc->enabled) > > > + continue; > > > + > > > + ret = drm_crtc_vblank_get(crtc); > > > + if (!ret) { > > > + drm_crtc_wait_one_vblank(crtc); > > > + drm_crtc_vblank_put(crtc); > > > + } > > > > This looks quite sub-optimal. It should rather do something along the > > lines of what drm_atomic_helper_wait_for_vblanks() does. > > How is that suboptimal? You're serializing the waits rather than doing them in parallel. Let's look at a simple three crtc example (|=vblank): time --> CRTC 1: | | | | CRTC 2: | | | | CRTC 3: | | | | Your code waits until here: | | | 1 2 3 ^ Optimal code would wait until here: | ^ -- Ville Syrjälä Intel OTC