From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Date: Fri, 22 Apr 2016 17:05:01 +0000 Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Message-Id: <20160422170501.GE2510@phenom.ffwll.local> List-Id: References: <1461165929-11344-1-git-send-email-noralf@tronnes.org> <1461165929-11344-5-git-send-email-noralf@tronnes.org> <571921F5.2080007@tronnes.org> <20160422082719.GH2510@phenom.ffwll.local> <571A326A.9070407@tronnes.org> In-Reply-To: <571A326A.9070407@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: linux-fbdev@vger.kernel.org, tomi.valkeinen@ti.com, laurent.pinchart@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Tr=F8nnes wrote: >=20 > Den 22.04.2016 10:27, skrev Daniel Vetter: > >On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Tr=F8nnes wrote: > >>Den 20.04.2016 17:25, skrev Noralf Tr=F8nnes: > >>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. > >>>Accumulated fbdev framebuffer changes are signaled using the callback > >>>(struct drm_framebuffer_funcs *)->dirty() > >>> > >>>The drm_fb_helper_sys_*() functions will accumulate changes and > >>>schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. > >>>This worker is used by the deferred io mmap code to signal that it > >>>has been collecting page faults. The page faults and/or other changes > >>>are then merged into a drm_clip_rect and passed to the framebuffer > >>>dirty() function. > >>> > >>>The driver is responsible for setting up the fb_info.fbdefio structure > >>>and calling fb_deferred_io_init() using the provided callback: > >>>(struct fb_deferred_io).deferred_io =3D drm_fb_helper_deferred_io; > >>> > >>>Signed-off-by: Noralf Tr=F8nnes > >>>--- > >>> drivers/gpu/drm/drm_fb_helper.c | 119 ++++++++++++++++++++++++++++++= +++++++++- > >>> include/drm/drm_fb_helper.h | 15 +++++ > >>> 2 files changed, 133 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_= helper.c > >>[...] > >> > >>>+#ifdef CONFIG_FB_DEFERRED_IO > >>>+/** > >>>+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_= io callback > >>>+ * function > >>>+ * > >>>+ * This function always runs in process context (struct delayed_work)= and calls > >>>+ * the (struct drm_framebuffer_funcs)->dirty function with the collec= ted > >>>+ * damage. There's no need to worry about the possibility that the fb= _sys_*() > >>>+ * functions could be running in atomic context. They only schedule t= he > >>>+ * delayed worker which then calls this deferred_io callback. > >>>+ */ > >>>+void drm_fb_helper_deferred_io(struct fb_info *info, > >>>+ struct list_head *pagelist) > >>>+{ > >>>+ struct drm_fb_helper *helper =3D info->par; > >>>+ unsigned long start, end, min, max; > >>>+ struct drm_clip_rect clip; > >>>+ unsigned long flags; > >>>+ struct page *page; > >>>+ > >>>+ if (!helper->fb->funcs->dirty) > >>>+ return; > >>>+ > >>>+ spin_lock_irqsave(&helper->dirty_lock, flags); > >>>+ clip =3D helper->dirty_clip; > >>>+ drm_clip_rect_reset(&helper->dirty_clip); > >>>+ spin_unlock_irqrestore(&helper->dirty_lock, flags); > >>>+ > >>>+ min =3D ULONG_MAX; > >>>+ max =3D 0; > >>>+ list_for_each_entry(page, pagelist, lru) { > >>>+ start =3D page->index << PAGE_SHIFT; > >>>+ end =3D start + PAGE_SIZE - 1; > >>>+ min =3D min(min, start); > >>>+ max =3D max(max, end); > >>>+ } > >>>+ > >>>+ if (min < max) { > >>>+ struct drm_clip_rect mmap_clip; > >>>+ > >>>+ mmap_clip.x1 =3D 0; > >>>+ mmap_clip.x2 =3D info->var.xres; > >>>+ mmap_clip.y1 =3D min / info->fix.line_length; > >>>+ mmap_clip.y2 =3D min_t(u32, max / info->fix.line_length, > >>>+ info->var.yres); > >>>+ drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0); > >>>+ } > >>>+ > >>>+ if (!drm_clip_rect_is_empty(&clip)) > >>>+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1); > >>>+} > >>>+EXPORT_SYMBOL(drm_fb_helper_deferred_io); > >>There is one thing I have wondered about when it comes to deferred io a= nd > >>long run times for the defio worker with my displays: > >> > >>Userspace writes to some pages then the deferred io worker kicks off and > >>runs for 100ms holding the page list mutex. While this is happening, > >>userspace writes to a new page triggering a page_mkwrite. Now this > >>function has to wait for the mutex to be released. > >> > >>Who is actually waiting here and is there a problem that this can last > >>for 100ms? > >No idea at all - I haven't looked that closely at fbdev defio. But one > >reason we have an explicit ioctl in drm to flush out frontbuffer renderi= ng > >is exactly that flushing could take some time, and should only be done > >once userspace has completed some rendering. Not right in the middle of = an > >op. > > > >I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? > >Otherwise you'll get to improve fbdev defio, and fbdev is deprecated > >subsystem and a real horror show. I highly recommend against touching it > >;-) >=20 > I have tried to track the call chain and it seems to be part of the > page fault handler. Which means it's userspace wanting to write to the > page that has to wait. And it has to wait at some random point in > whatever rendering it's doing. >=20 > Unless someone has any objections, I will make a change and add a worker > like qxl does. This decouples the deferred_io worker holding the mutex > from the framebuffer flushing job. However I intend to differ from qxl in > that I will use a delayed worker (run immediately from the mmap side which > has already been deferred). Since I don't see any point in flushing the > framebuffer immediately when fbcon has put out only one glyph, might as > well defer it a couple of jiffies to be able to capture some more glyphs. >=20 > Adding a worker also means that udl doesn't have to initialize deferred_io > because we won't be using the deferred_work worker for flushing fb_*(). I'm confused ... I thought we already have enough workers? One in the fbdev deferred_io implementation used by default. The other in case we get a draw call from an atomic context. Why do we need even more workers? Have you measured that you actually hit this delay, or just conjecture from reading the code? Because my reading says that the defio mmap support in fbdev already does what you want, and should sufficiently coalesce mmap access. There's a delayed work/timer in there to make sure it doesn't flush on the very first faulted page. -Daniel > And yes, using drm from userspace is "The solution" here :-), however > I want to make the best out of fbdev since some of the tinydrm users > coming from drivers/staging/fbtft will probably continue with fbdev. >=20 >=20 > Noralf. >=20 > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel --=20 Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Date: Fri, 22 Apr 2016 19:05:01 +0200 Message-ID: <20160422170501.GE2510@phenom.ffwll.local> References: <1461165929-11344-1-git-send-email-noralf@tronnes.org> <1461165929-11344-5-git-send-email-noralf@tronnes.org> <571921F5.2080007@tronnes.org> <20160422082719.GH2510@phenom.ffwll.local> <571A326A.9070407@tronnes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x22a.google.com (mail-wm0-x22a.google.com [IPv6:2a00:1450:400c:c09::22a]) by gabe.freedesktop.org (Postfix) with ESMTPS id D4B476E0A2 for ; Fri, 22 Apr 2016 17:05:05 +0000 (UTC) Received: by mail-wm0-x22a.google.com with SMTP id u206so36303500wme.1 for ; Fri, 22 Apr 2016 10:05:05 -0700 (PDT) Content-Disposition: inline In-Reply-To: <571A326A.9070407@tronnes.org> List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: dri-devel-bounces@lists.freedesktop.org Sender: "dri-devel" To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: linux-fbdev@vger.kernel.org, tomi.valkeinen@ti.com, laurent.pinchart@ideasonboard.com, dri-devel@lists.freedesktop.org, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org T24gRnJpLCBBcHIgMjIsIDIwMTYgYXQgMDQ6MTc6MTRQTSArMDIwMCwgTm9yYWxmIFRyw7hubmVz IHdyb3RlOgo+IAo+IERlbiAyMi4wNC4yMDE2IDEwOjI3LCBza3JldiBEYW5pZWwgVmV0dGVyOgo+ ID5PbiBUaHUsIEFwciAyMSwgMjAxNiBhdCAwODo1NDo0NVBNICswMjAwLCBOb3JhbGYgVHLDuG5u ZXMgd3JvdGU6Cj4gPj5EZW4gMjAuMDQuMjAxNiAxNzoyNSwgc2tyZXYgTm9yYWxmIFRyw7hubmVz Ogo+ID4+PlRoaXMgYWRkcyBkZWZlcnJlZCBpbyBzdXBwb3J0IGlmIENPTkZJR19GQl9ERUZFUlJF RF9JTyBpcyBlbmFibGVkLgo+ID4+PkFjY3VtdWxhdGVkIGZiZGV2IGZyYW1lYnVmZmVyIGNoYW5n ZXMgYXJlIHNpZ25hbGVkIHVzaW5nIHRoZSBjYWxsYmFjawo+ID4+PihzdHJ1Y3QgZHJtX2ZyYW1l YnVmZmVyX2Z1bmNzICopLT5kaXJ0eSgpCj4gPj4+Cj4gPj4+VGhlIGRybV9mYl9oZWxwZXJfc3lz XyooKSBmdW5jdGlvbnMgd2lsbCBhY2N1bXVsYXRlIGNoYW5nZXMgYW5kCj4gPj4+c2NoZWR1bGUg ZmJfaW5mby5kZWZlcnJlZF93b3JrIF9pZl8gZmJfaW5mby5mYmRlZmlvIGlzIHNldC4KPiA+Pj5U aGlzIHdvcmtlciBpcyB1c2VkIGJ5IHRoZSBkZWZlcnJlZCBpbyBtbWFwIGNvZGUgdG8gc2lnbmFs IHRoYXQgaXQKPiA+Pj5oYXMgYmVlbiBjb2xsZWN0aW5nIHBhZ2UgZmF1bHRzLiBUaGUgcGFnZSBm YXVsdHMgYW5kL29yIG90aGVyIGNoYW5nZXMKPiA+Pj5hcmUgdGhlbiBtZXJnZWQgaW50byBhIGRy bV9jbGlwX3JlY3QgYW5kIHBhc3NlZCB0byB0aGUgZnJhbWVidWZmZXIKPiA+Pj5kaXJ0eSgpIGZ1 bmN0aW9uLgo+ID4+Pgo+ID4+PlRoZSBkcml2ZXIgaXMgcmVzcG9uc2libGUgZm9yIHNldHRpbmcg dXAgdGhlIGZiX2luZm8uZmJkZWZpbyBzdHJ1Y3R1cmUKPiA+Pj5hbmQgY2FsbGluZyBmYl9kZWZl cnJlZF9pb19pbml0KCkgdXNpbmcgdGhlIHByb3ZpZGVkIGNhbGxiYWNrOgo+ID4+PihzdHJ1Y3Qg ZmJfZGVmZXJyZWRfaW8pLmRlZmVycmVkX2lvID0gZHJtX2ZiX2hlbHBlcl9kZWZlcnJlZF9pbzsK PiA+Pj4KPiA+Pj5TaWduZWQtb2ZmLWJ5OiBOb3JhbGYgVHLDuG5uZXMgPG5vcmFsZkB0cm9ubmVz Lm9yZz4KPiA+Pj4tLS0KPiA+Pj4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgfCAx MTkgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLQo+ID4+PiAgaW5jbHVk ZS9kcm0vZHJtX2ZiX2hlbHBlci5oICAgICB8ICAxNSArKysrKwo+ID4+PiAgMiBmaWxlcyBjaGFu Z2VkLCAxMzMgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+ID4+Pgo+ID4+PmRpZmYgLS1n aXQgYS9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jIGIvZHJpdmVycy9ncHUvZHJtL2Ry bV9mYl9oZWxwZXIuYwo+ID4+Wy4uLl0KPiA+Pgo+ID4+PisjaWZkZWYgQ09ORklHX0ZCX0RFRkVS UkVEX0lPCj4gPj4+Ky8qKgo+ID4+PisgKiBkcm1fZmJfaGVscGVyX2RlZmVycmVkX2lvKCkgLSAo c3RydWN0IGZiX2RlZmVycmVkX2lvICopLT5kZWZlcnJlZF9pbyBjYWxsYmFjawo+ID4+PisgKiAg ICAgICAgICAgICAgICAgICAgICAgICAgICAgICBmdW5jdGlvbgo+ID4+PisgKgo+ID4+PisgKiBU aGlzIGZ1bmN0aW9uIGFsd2F5cyBydW5zIGluIHByb2Nlc3MgY29udGV4dCAoc3RydWN0IGRlbGF5 ZWRfd29yaykgYW5kIGNhbGxzCj4gPj4+KyAqIHRoZSAoc3RydWN0IGRybV9mcmFtZWJ1ZmZlcl9m dW5jcyktPmRpcnR5IGZ1bmN0aW9uIHdpdGggdGhlIGNvbGxlY3RlZAo+ID4+PisgKiBkYW1hZ2Uu IFRoZXJlJ3Mgbm8gbmVlZCB0byB3b3JyeSBhYm91dCB0aGUgcG9zc2liaWxpdHkgdGhhdCB0aGUg ZmJfc3lzXyooKQo+ID4+PisgKiBmdW5jdGlvbnMgY291bGQgYmUgcnVubmluZyBpbiBhdG9taWMg Y29udGV4dC4gVGhleSBvbmx5IHNjaGVkdWxlIHRoZQo+ID4+PisgKiBkZWxheWVkIHdvcmtlciB3 aGljaCB0aGVuIGNhbGxzIHRoaXMgZGVmZXJyZWRfaW8gY2FsbGJhY2suCj4gPj4+KyAqLwo+ID4+ Pit2b2lkIGRybV9mYl9oZWxwZXJfZGVmZXJyZWRfaW8oc3RydWN0IGZiX2luZm8gKmluZm8sCj4g Pj4+KwkJCSAgICAgICBzdHJ1Y3QgbGlzdF9oZWFkICpwYWdlbGlzdCkKPiA+Pj4rewo+ID4+PisJ c3RydWN0IGRybV9mYl9oZWxwZXIgKmhlbHBlciA9IGluZm8tPnBhcjsKPiA+Pj4rCXVuc2lnbmVk IGxvbmcgc3RhcnQsIGVuZCwgbWluLCBtYXg7Cj4gPj4+KwlzdHJ1Y3QgZHJtX2NsaXBfcmVjdCBj bGlwOwo+ID4+PisJdW5zaWduZWQgbG9uZyBmbGFnczsKPiA+Pj4rCXN0cnVjdCBwYWdlICpwYWdl Owo+ID4+PisKPiA+Pj4rCWlmICghaGVscGVyLT5mYi0+ZnVuY3MtPmRpcnR5KQo+ID4+PisJCXJl dHVybjsKPiA+Pj4rCj4gPj4+KwlzcGluX2xvY2tfaXJxc2F2ZSgmaGVscGVyLT5kaXJ0eV9sb2Nr LCBmbGFncyk7Cj4gPj4+KwljbGlwID0gaGVscGVyLT5kaXJ0eV9jbGlwOwo+ID4+PisJZHJtX2Ns aXBfcmVjdF9yZXNldCgmaGVscGVyLT5kaXJ0eV9jbGlwKTsKPiA+Pj4rCXNwaW5fdW5sb2NrX2ly cXJlc3RvcmUoJmhlbHBlci0+ZGlydHlfbG9jaywgZmxhZ3MpOwo+ID4+PisKPiA+Pj4rCW1pbiA9 IFVMT05HX01BWDsKPiA+Pj4rCW1heCA9IDA7Cj4gPj4+KwlsaXN0X2Zvcl9lYWNoX2VudHJ5KHBh Z2UsIHBhZ2VsaXN0LCBscnUpIHsKPiA+Pj4rCQlzdGFydCA9IHBhZ2UtPmluZGV4IDw8IFBBR0Vf U0hJRlQ7Cj4gPj4+KwkJZW5kID0gc3RhcnQgKyBQQUdFX1NJWkUgLSAxOwo+ID4+PisJCW1pbiA9 IG1pbihtaW4sIHN0YXJ0KTsKPiA+Pj4rCQltYXggPSBtYXgobWF4LCBlbmQpOwo+ID4+PisJfQo+ ID4+PisKPiA+Pj4rCWlmIChtaW4gPCBtYXgpIHsKPiA+Pj4rCQlzdHJ1Y3QgZHJtX2NsaXBfcmVj dCBtbWFwX2NsaXA7Cj4gPj4+Kwo+ID4+PisJCW1tYXBfY2xpcC54MSA9IDA7Cj4gPj4+KwkJbW1h cF9jbGlwLngyID0gaW5mby0+dmFyLnhyZXM7Cj4gPj4+KwkJbW1hcF9jbGlwLnkxID0gbWluIC8g aW5mby0+Zml4LmxpbmVfbGVuZ3RoOwo+ID4+PisJCW1tYXBfY2xpcC55MiA9IG1pbl90KHUzMiwg bWF4IC8gaW5mby0+Zml4LmxpbmVfbGVuZ3RoLAo+ID4+PisJCQkJICAgICBpbmZvLT52YXIueXJl cyk7Cj4gPj4+KwkJZHJtX2NsaXBfcmVjdF9tZXJnZSgmY2xpcCwgJm1tYXBfY2xpcCwgMSwgMCwg MCwgMCk7Cj4gPj4+Kwl9Cj4gPj4+Kwo+ID4+PisJaWYgKCFkcm1fY2xpcF9yZWN0X2lzX2VtcHR5 KCZjbGlwKSkKPiA+Pj4rCQloZWxwZXItPmZiLT5mdW5jcy0+ZGlydHkoaGVscGVyLT5mYiwgTlVM TCwgMCwgMCwgJmNsaXAsIDEpOwo+ID4+Pit9Cj4gPj4+K0VYUE9SVF9TWU1CT0woZHJtX2ZiX2hl bHBlcl9kZWZlcnJlZF9pbyk7Cj4gPj5UaGVyZSBpcyBvbmUgdGhpbmcgSSBoYXZlIHdvbmRlcmVk IGFib3V0IHdoZW4gaXQgY29tZXMgdG8gZGVmZXJyZWQgaW8gYW5kCj4gPj5sb25nIHJ1biB0aW1l cyBmb3IgdGhlIGRlZmlvIHdvcmtlciB3aXRoIG15IGRpc3BsYXlzOgo+ID4+Cj4gPj5Vc2Vyc3Bh Y2Ugd3JpdGVzIHRvIHNvbWUgcGFnZXMgdGhlbiB0aGUgZGVmZXJyZWQgaW8gd29ya2VyIGtpY2tz IG9mZiBhbmQKPiA+PnJ1bnMgZm9yIDEwMG1zIGhvbGRpbmcgdGhlIHBhZ2UgbGlzdCBtdXRleC4g V2hpbGUgdGhpcyBpcyBoYXBwZW5pbmcsCj4gPj51c2Vyc3BhY2Ugd3JpdGVzIHRvIGEgbmV3IHBh Z2UgdHJpZ2dlcmluZyBhIHBhZ2VfbWt3cml0ZS4gTm93IHRoaXMKPiA+PmZ1bmN0aW9uIGhhcyB0 byB3YWl0IGZvciB0aGUgbXV0ZXggdG8gYmUgcmVsZWFzZWQuCj4gPj4KPiA+PldobyBpcyBhY3R1 YWxseSB3YWl0aW5nIGhlcmUgYW5kIGlzIHRoZXJlIGEgcHJvYmxlbSB0aGF0IHRoaXMgY2FuIGxh c3QKPiA+PmZvciAxMDBtcz8KPiA+Tm8gaWRlYSBhdCBhbGwgLSBJIGhhdmVuJ3QgbG9va2VkIHRo YXQgY2xvc2VseSBhdCAgZmJkZXYgZGVmaW8uIEJ1dCBvbmUKPiA+cmVhc29uIHdlIGhhdmUgYW4g ZXhwbGljaXQgaW9jdGwgaW4gZHJtIHRvIGZsdXNoIG91dCBmcm9udGJ1ZmZlciByZW5kZXJpbmcK PiA+aXMgZXhhY3RseSB0aGF0IGZsdXNoaW5nIGNvdWxkIHRha2Ugc29tZSB0aW1lLCBhbmQgc2hv dWxkIG9ubHkgYmUgZG9uZQo+ID5vbmNlIHVzZXJzcGFjZSBoYXMgY29tcGxldGVkIHNvbWUgcmVu ZGVyaW5nLiBOb3QgcmlnaHQgaW4gdGhlIG1pZGRsZSBvZiBhbgo+ID5vcC4KPiA+Cj4gPkkgZ3Vl c3MgZml4IHVwIHlvdXIgdXNlcnNwYWNlIHRvIHVzZSBkdW1iIGRybSBmYiArIGRybSBkaXJ0eWZi IGlvY3RsPwo+ID5PdGhlcndpc2UgeW91J2xsIGdldCB0byBpbXByb3ZlIGZiZGV2IGRlZmlvLCBh bmQgZmJkZXYgaXMgZGVwcmVjYXRlZAo+ID5zdWJzeXN0ZW0gYW5kIGEgcmVhbCBob3Jyb3Igc2hv dy4gSSBoaWdobHkgcmVjb21tZW5kIGFnYWluc3QgdG91Y2hpbmcgaXQKPiA+Oy0pCj4gCj4gSSBo YXZlIHRyaWVkIHRvIHRyYWNrIHRoZSBjYWxsIGNoYWluIGFuZCBpdCBzZWVtcyB0byBiZSBwYXJ0 IG9mIHRoZQo+IHBhZ2UgZmF1bHQgaGFuZGxlci4gV2hpY2ggbWVhbnMgaXQncyB1c2Vyc3BhY2Ug d2FudGluZyB0byB3cml0ZSB0byB0aGUKPiBwYWdlIHRoYXQgaGFzIHRvIHdhaXQuIEFuZCBpdCBo YXMgdG8gd2FpdCBhdCBzb21lIHJhbmRvbSBwb2ludCBpbgo+IHdoYXRldmVyIHJlbmRlcmluZyBp dCdzIGRvaW5nLgo+IAo+IFVubGVzcyBzb21lb25lIGhhcyBhbnkgb2JqZWN0aW9ucywgSSB3aWxs IG1ha2UgYSBjaGFuZ2UgYW5kIGFkZCBhIHdvcmtlcgo+IGxpa2UgcXhsIGRvZXMuIFRoaXMgZGVj b3VwbGVzIHRoZSBkZWZlcnJlZF9pbyB3b3JrZXIgaG9sZGluZyB0aGUgbXV0ZXgKPiBmcm9tIHRo ZSBmcmFtZWJ1ZmZlciBmbHVzaGluZyBqb2IuIEhvd2V2ZXIgSSBpbnRlbmQgdG8gZGlmZmVyIGZy b20gcXhsIGluCj4gdGhhdCBJIHdpbGwgdXNlIGEgZGVsYXllZCB3b3JrZXIgKHJ1biBpbW1lZGlh dGVseSBmcm9tIHRoZSBtbWFwIHNpZGUgd2hpY2gKPiBoYXMgYWxyZWFkeSBiZWVuIGRlZmVycmVk KS4gU2luY2UgSSBkb24ndCBzZWUgYW55IHBvaW50IGluIGZsdXNoaW5nIHRoZQo+IGZyYW1lYnVm ZmVyIGltbWVkaWF0ZWx5IHdoZW4gZmJjb24gaGFzIHB1dCBvdXQgb25seSBvbmUgZ2x5cGgsIG1p Z2h0IGFzCj4gd2VsbCBkZWZlciBpdCBhIGNvdXBsZSBvZiBqaWZmaWVzIHRvIGJlIGFibGUgdG8g Y2FwdHVyZSBzb21lIG1vcmUgZ2x5cGhzLgo+IAo+IEFkZGluZyBhIHdvcmtlciBhbHNvIG1lYW5z IHRoYXQgdWRsIGRvZXNuJ3QgaGF2ZSB0byBpbml0aWFsaXplIGRlZmVycmVkX2lvCj4gYmVjYXVz ZSB3ZSB3b24ndCBiZSB1c2luZyB0aGUgZGVmZXJyZWRfd29yayB3b3JrZXIgZm9yIGZsdXNoaW5n IGZiXyooKS4KCkknbSBjb25mdXNlZCAuLi4gSSB0aG91Z2h0IHdlIGFscmVhZHkgaGF2ZSBlbm91 Z2ggd29ya2Vycz8gT25lIGluIHRoZQpmYmRldiBkZWZlcnJlZF9pbyBpbXBsZW1lbnRhdGlvbiB1 c2VkIGJ5IGRlZmF1bHQuIFRoZSBvdGhlciBpbiBjYXNlIHdlIGdldAphIGRyYXcgY2FsbCBmcm9t IGFuIGF0b21pYyBjb250ZXh0LgoKV2h5IGRvIHdlIG5lZWQgZXZlbiBtb3JlIHdvcmtlcnM/IEhh dmUgeW91IG1lYXN1cmVkIHRoYXQgeW91IGFjdHVhbGx5IGhpdAp0aGlzIGRlbGF5LCBvciBqdXN0 IGNvbmplY3R1cmUgZnJvbSByZWFkaW5nIHRoZSBjb2RlPyBCZWNhdXNlIG15IHJlYWRpbmcKc2F5 cyB0aGF0IHRoZSBkZWZpbyBtbWFwIHN1cHBvcnQgaW4gZmJkZXYgYWxyZWFkeSBkb2VzIHdoYXQg eW91IHdhbnQsIGFuZApzaG91bGQgc3VmZmljaWVudGx5IGNvYWxlc2NlIG1tYXAgYWNjZXNzLiBU aGVyZSdzIGEgZGVsYXllZCB3b3JrL3RpbWVyIGluCnRoZXJlIHRvIG1ha2Ugc3VyZSBpdCBkb2Vz bid0IGZsdXNoIG9uIHRoZSB2ZXJ5IGZpcnN0IGZhdWx0ZWQgcGFnZS4KLURhbmllbAoKPiBBbmQg eWVzLCB1c2luZyBkcm0gZnJvbSB1c2Vyc3BhY2UgaXMgIlRoZSBzb2x1dGlvbiIgaGVyZSA6LSks IGhvd2V2ZXIKPiBJIHdhbnQgdG8gbWFrZSB0aGUgYmVzdCBvdXQgb2YgZmJkZXYgc2luY2Ugc29t ZSBvZiB0aGUgdGlueWRybSB1c2Vycwo+IGNvbWluZyBmcm9tIGRyaXZlcnMvc3RhZ2luZy9mYnRm dCB3aWxsIHByb2JhYmx5IGNvbnRpbnVlIHdpdGggZmJkZXYuCj4gCj4gCj4gTm9yYWxmLgo+IAo+ IF9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCj4gZHJpLWRl dmVsIG1haWxpbmcgbGlzdAo+IGRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcKPiBodHRw czovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAoKLS0g CkRhbmllbCBWZXR0ZXIKU29mdHdhcmUgRW5naW5lZXIsIEludGVsIENvcnBvcmF0aW9uCmh0dHA6 Ly9ibG9nLmZmd2xsLmNoCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f 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 S1754665AbcDVRFJ (ORCPT ); Fri, 22 Apr 2016 13:05:09 -0400 Received: from mail-wm0-f48.google.com ([74.125.82.48]:36169 "EHLO mail-wm0-f48.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753937AbcDVRFG (ORCPT ); Fri, 22 Apr 2016 13:05:06 -0400 Date: Fri, 22 Apr 2016 19:05:01 +0200 From: Daniel Vetter To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= Cc: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Message-ID: <20160422170501.GE2510@phenom.ffwll.local> Mail-Followup-To: Noralf =?iso-8859-1?Q?Tr=F8nnes?= , dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org References: <1461165929-11344-1-git-send-email-noralf@tronnes.org> <1461165929-11344-5-git-send-email-noralf@tronnes.org> <571921F5.2080007@tronnes.org> <20160422082719.GH2510@phenom.ffwll.local> <571A326A.9070407@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <571A326A.9070407@tronnes.org> X-Operating-System: Linux phenom 4.4.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 Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Trønnes wrote: > > Den 22.04.2016 10:27, skrev Daniel Vetter: > >On Thu, Apr 21, 2016 at 08:54:45PM +0200, Noralf Trønnes wrote: > >>Den 20.04.2016 17:25, skrev Noralf Trønnes: > >>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. > >>>Accumulated fbdev framebuffer changes are signaled using the callback > >>>(struct drm_framebuffer_funcs *)->dirty() > >>> > >>>The drm_fb_helper_sys_*() functions will accumulate changes and > >>>schedule fb_info.deferred_work _if_ fb_info.fbdefio is set. > >>>This worker is used by the deferred io mmap code to signal that it > >>>has been collecting page faults. The page faults and/or other changes > >>>are then merged into a drm_clip_rect and passed to the framebuffer > >>>dirty() function. > >>> > >>>The driver is responsible for setting up the fb_info.fbdefio structure > >>>and calling fb_deferred_io_init() using the provided callback: > >>>(struct fb_deferred_io).deferred_io = drm_fb_helper_deferred_io; > >>> > >>>Signed-off-by: Noralf Trønnes > >>>--- > >>> drivers/gpu/drm/drm_fb_helper.c | 119 +++++++++++++++++++++++++++++++++++++++- > >>> include/drm/drm_fb_helper.h | 15 +++++ > >>> 2 files changed, 133 insertions(+), 1 deletion(-) > >>> > >>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >>[...] > >> > >>>+#ifdef CONFIG_FB_DEFERRED_IO > >>>+/** > >>>+ * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_io callback > >>>+ * function > >>>+ * > >>>+ * This function always runs in process context (struct delayed_work) and calls > >>>+ * the (struct drm_framebuffer_funcs)->dirty function with the collected > >>>+ * damage. There's no need to worry about the possibility that the fb_sys_*() > >>>+ * functions could be running in atomic context. They only schedule the > >>>+ * delayed worker which then calls this deferred_io callback. > >>>+ */ > >>>+void drm_fb_helper_deferred_io(struct fb_info *info, > >>>+ struct list_head *pagelist) > >>>+{ > >>>+ struct drm_fb_helper *helper = info->par; > >>>+ unsigned long start, end, min, max; > >>>+ struct drm_clip_rect clip; > >>>+ unsigned long flags; > >>>+ struct page *page; > >>>+ > >>>+ if (!helper->fb->funcs->dirty) > >>>+ return; > >>>+ > >>>+ spin_lock_irqsave(&helper->dirty_lock, flags); > >>>+ clip = helper->dirty_clip; > >>>+ drm_clip_rect_reset(&helper->dirty_clip); > >>>+ spin_unlock_irqrestore(&helper->dirty_lock, flags); > >>>+ > >>>+ min = ULONG_MAX; > >>>+ max = 0; > >>>+ list_for_each_entry(page, pagelist, lru) { > >>>+ start = page->index << PAGE_SHIFT; > >>>+ end = start + PAGE_SIZE - 1; > >>>+ min = min(min, start); > >>>+ max = max(max, end); > >>>+ } > >>>+ > >>>+ if (min < max) { > >>>+ struct drm_clip_rect mmap_clip; > >>>+ > >>>+ mmap_clip.x1 = 0; > >>>+ mmap_clip.x2 = info->var.xres; > >>>+ mmap_clip.y1 = min / info->fix.line_length; > >>>+ mmap_clip.y2 = min_t(u32, max / info->fix.line_length, > >>>+ info->var.yres); > >>>+ drm_clip_rect_merge(&clip, &mmap_clip, 1, 0, 0, 0); > >>>+ } > >>>+ > >>>+ if (!drm_clip_rect_is_empty(&clip)) > >>>+ helper->fb->funcs->dirty(helper->fb, NULL, 0, 0, &clip, 1); > >>>+} > >>>+EXPORT_SYMBOL(drm_fb_helper_deferred_io); > >>There is one thing I have wondered about when it comes to deferred io and > >>long run times for the defio worker with my displays: > >> > >>Userspace writes to some pages then the deferred io worker kicks off and > >>runs for 100ms holding the page list mutex. While this is happening, > >>userspace writes to a new page triggering a page_mkwrite. Now this > >>function has to wait for the mutex to be released. > >> > >>Who is actually waiting here and is there a problem that this can last > >>for 100ms? > >No idea at all - I haven't looked that closely at fbdev defio. But one > >reason we have an explicit ioctl in drm to flush out frontbuffer rendering > >is exactly that flushing could take some time, and should only be done > >once userspace has completed some rendering. Not right in the middle of an > >op. > > > >I guess fix up your userspace to use dumb drm fb + drm dirtyfb ioctl? > >Otherwise you'll get to improve fbdev defio, and fbdev is deprecated > >subsystem and a real horror show. I highly recommend against touching it > >;-) > > I have tried to track the call chain and it seems to be part of the > page fault handler. Which means it's userspace wanting to write to the > page that has to wait. And it has to wait at some random point in > whatever rendering it's doing. > > Unless someone has any objections, I will make a change and add a worker > like qxl does. This decouples the deferred_io worker holding the mutex > from the framebuffer flushing job. However I intend to differ from qxl in > that I will use a delayed worker (run immediately from the mmap side which > has already been deferred). Since I don't see any point in flushing the > framebuffer immediately when fbcon has put out only one glyph, might as > well defer it a couple of jiffies to be able to capture some more glyphs. > > Adding a worker also means that udl doesn't have to initialize deferred_io > because we won't be using the deferred_work worker for flushing fb_*(). I'm confused ... I thought we already have enough workers? One in the fbdev deferred_io implementation used by default. The other in case we get a draw call from an atomic context. Why do we need even more workers? Have you measured that you actually hit this delay, or just conjecture from reading the code? Because my reading says that the defio mmap support in fbdev already does what you want, and should sufficiently coalesce mmap access. There's a delayed work/timer in there to make sure it doesn't flush on the very first faulted page. -Daniel > And yes, using drm from userspace is "The solution" here :-), however > I want to make the best out of fbdev since some of the tinydrm users > coming from drivers/staging/fbtft will probably continue with fbdev. > > > Noralf. > > _______________________________________________ > dri-devel mailing list > dri-devel@lists.freedesktop.org > https://lists.freedesktop.org/mailman/listinfo/dri-devel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch