From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Date: Fri, 22 Apr 2016 14:17:14 +0000 Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Message-Id: <571A326A.9070407@tronnes.org> 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> In-Reply-To: <20160422082719.GH2510@phenom.ffwll.local> MIME-Version: 1.0 Content-Type: text/plain; charset="iso-8859-1" Content-Transfer-Encoding: quoted-printable To: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org 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_h= elper.c >> [...] >> >>> +#ifdef CONFIG_FB_DEFERRED_IO >>> +/** >>> + * drm_fb_helper_deferred_io() - (struct fb_deferred_io *)->deferred_i= o callback >>> + * function >>> + * >>> + * This function always runs in process context (struct delayed_work) = and calls >>> + * the (struct drm_framebuffer_funcs)->dirty function with the collect= ed >>> + * 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 =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 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_*(). 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. From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Date: Fri, 22 Apr 2016 16:17:14 +0200 Message-ID: <571A326A.9070407@tronnes.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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8"; Format="flowed" Content-Transfer-Encoding: base64 Return-path: Received: from smtp.domeneshop.no (smtp.domeneshop.no [IPv6:2a01:5b40:0:3005::1]) by gabe.freedesktop.org (Postfix) with ESMTPS id 3704F6EEFB for ; Fri, 22 Apr 2016 14:17:23 +0000 (UTC) In-Reply-To: <20160422082719.GH2510@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: dri-devel@lists.freedesktop.org, linux-fbdev@vger.kernel.org, laurent.pinchart@ideasonboard.com, tomi.valkeinen@ti.com, linux-kernel@vger.kernel.org List-Id: dri-devel@lists.freedesktop.org CkRlbiAyMi4wNC4yMDE2IDEwOjI3LCBza3JldiBEYW5pZWwgVmV0dGVyOgo+IE9uIFRodSwgQXBy IDIxLCAyMDE2IGF0IDA4OjU0OjQ1UE0gKzAyMDAsIE5vcmFsZiBUcsO4bm5lcyB3cm90ZToKPj4g RGVuIDIwLjA0LjIwMTYgMTc6MjUsIHNrcmV2IE5vcmFsZiBUcsO4bm5lczoKPj4+IFRoaXMgYWRk cyBkZWZlcnJlZCBpbyBzdXBwb3J0IGlmIENPTkZJR19GQl9ERUZFUlJFRF9JTyBpcyBlbmFibGVk Lgo+Pj4gQWNjdW11bGF0ZWQgZmJkZXYgZnJhbWVidWZmZXIgY2hhbmdlcyBhcmUgc2lnbmFsZWQg dXNpbmcgdGhlIGNhbGxiYWNrCj4+PiAoc3RydWN0IGRybV9mcmFtZWJ1ZmZlcl9mdW5jcyAqKS0+ ZGlydHkoKQo+Pj4KPj4+IFRoZSBkcm1fZmJfaGVscGVyX3N5c18qKCkgZnVuY3Rpb25zIHdpbGwg YWNjdW11bGF0ZSBjaGFuZ2VzIGFuZAo+Pj4gc2NoZWR1bGUgZmJfaW5mby5kZWZlcnJlZF93b3Jr IF9pZl8gZmJfaW5mby5mYmRlZmlvIGlzIHNldC4KPj4+IFRoaXMgd29ya2VyIGlzIHVzZWQgYnkg dGhlIGRlZmVycmVkIGlvIG1tYXAgY29kZSB0byBzaWduYWwgdGhhdCBpdAo+Pj4gaGFzIGJlZW4g Y29sbGVjdGluZyBwYWdlIGZhdWx0cy4gVGhlIHBhZ2UgZmF1bHRzIGFuZC9vciBvdGhlciBjaGFu Z2VzCj4+PiBhcmUgdGhlbiBtZXJnZWQgaW50byBhIGRybV9jbGlwX3JlY3QgYW5kIHBhc3NlZCB0 byB0aGUgZnJhbWVidWZmZXIKPj4+IGRpcnR5KCkgZnVuY3Rpb24uCj4+Pgo+Pj4gVGhlIGRyaXZl ciBpcyByZXNwb25zaWJsZSBmb3Igc2V0dGluZyB1cCB0aGUgZmJfaW5mby5mYmRlZmlvIHN0cnVj dHVyZQo+Pj4gYW5kIGNhbGxpbmcgZmJfZGVmZXJyZWRfaW9faW5pdCgpIHVzaW5nIHRoZSBwcm92 aWRlZCBjYWxsYmFjazoKPj4+IChzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8pLmRlZmVycmVkX2lvID0g ZHJtX2ZiX2hlbHBlcl9kZWZlcnJlZF9pbzsKPj4+Cj4+PiBTaWduZWQtb2ZmLWJ5OiBOb3JhbGYg VHLDuG5uZXMgPG5vcmFsZkB0cm9ubmVzLm9yZz4KPj4+IC0tLQo+Pj4gICBkcml2ZXJzL2dwdS9k cm0vZHJtX2ZiX2hlbHBlci5jIHwgMTE5ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysr KysrKysrKy0KPj4+ICAgaW5jbHVkZS9kcm0vZHJtX2ZiX2hlbHBlci5oICAgICB8ICAxNSArKysr Kwo+Pj4gICAyIGZpbGVzIGNoYW5nZWQsIDEzMyBpbnNlcnRpb25zKCspLCAxIGRlbGV0aW9uKC0p Cj4+Pgo+Pj4gZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgYi9k cml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4+IFsuLi5dCj4+Cj4+PiArI2lmZGVmIENP TkZJR19GQl9ERUZFUlJFRF9JTwo+Pj4gKy8qKgo+Pj4gKyAqIGRybV9mYl9oZWxwZXJfZGVmZXJy ZWRfaW8oKSAtIChzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8gKiktPmRlZmVycmVkX2lvIGNhbGxiYWNr Cj4+PiArICogICAgICAgICAgICAgICAgICAgICAgICAgICAgICAgZnVuY3Rpb24KPj4+ICsgKgo+ Pj4gKyAqIFRoaXMgZnVuY3Rpb24gYWx3YXlzIHJ1bnMgaW4gcHJvY2VzcyBjb250ZXh0IChzdHJ1 Y3QgZGVsYXllZF93b3JrKSBhbmQgY2FsbHMKPj4+ICsgKiB0aGUgKHN0cnVjdCBkcm1fZnJhbWVi dWZmZXJfZnVuY3MpLT5kaXJ0eSBmdW5jdGlvbiB3aXRoIHRoZSBjb2xsZWN0ZWQKPj4+ICsgKiBk YW1hZ2UuIFRoZXJlJ3Mgbm8gbmVlZCB0byB3b3JyeSBhYm91dCB0aGUgcG9zc2liaWxpdHkgdGhh dCB0aGUgZmJfc3lzXyooKQo+Pj4gKyAqIGZ1bmN0aW9ucyBjb3VsZCBiZSBydW5uaW5nIGluIGF0 b21pYyBjb250ZXh0LiBUaGV5IG9ubHkgc2NoZWR1bGUgdGhlCj4+PiArICogZGVsYXllZCB3b3Jr ZXIgd2hpY2ggdGhlbiBjYWxscyB0aGlzIGRlZmVycmVkX2lvIGNhbGxiYWNrLgo+Pj4gKyAqLwo+ Pj4gK3ZvaWQgZHJtX2ZiX2hlbHBlcl9kZWZlcnJlZF9pbyhzdHJ1Y3QgZmJfaW5mbyAqaW5mbywK Pj4+ICsJCQkgICAgICAgc3RydWN0IGxpc3RfaGVhZCAqcGFnZWxpc3QpCj4+PiArewo+Pj4gKwlz dHJ1Y3QgZHJtX2ZiX2hlbHBlciAqaGVscGVyID0gaW5mby0+cGFyOwo+Pj4gKwl1bnNpZ25lZCBs b25nIHN0YXJ0LCBlbmQsIG1pbiwgbWF4Owo+Pj4gKwlzdHJ1Y3QgZHJtX2NsaXBfcmVjdCBjbGlw Owo+Pj4gKwl1bnNpZ25lZCBsb25nIGZsYWdzOwo+Pj4gKwlzdHJ1Y3QgcGFnZSAqcGFnZTsKPj4+ ICsKPj4+ICsJaWYgKCFoZWxwZXItPmZiLT5mdW5jcy0+ZGlydHkpCj4+PiArCQlyZXR1cm47Cj4+ PiArCj4+PiArCXNwaW5fbG9ja19pcnFzYXZlKCZoZWxwZXItPmRpcnR5X2xvY2ssIGZsYWdzKTsK Pj4+ICsJY2xpcCA9IGhlbHBlci0+ZGlydHlfY2xpcDsKPj4+ICsJZHJtX2NsaXBfcmVjdF9yZXNl dCgmaGVscGVyLT5kaXJ0eV9jbGlwKTsKPj4+ICsJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmaGVs cGVyLT5kaXJ0eV9sb2NrLCBmbGFncyk7Cj4+PiArCj4+PiArCW1pbiA9IFVMT05HX01BWDsKPj4+ ICsJbWF4ID0gMDsKPj4+ICsJbGlzdF9mb3JfZWFjaF9lbnRyeShwYWdlLCBwYWdlbGlzdCwgbHJ1 KSB7Cj4+PiArCQlzdGFydCA9IHBhZ2UtPmluZGV4IDw8IFBBR0VfU0hJRlQ7Cj4+PiArCQllbmQg PSBzdGFydCArIFBBR0VfU0laRSAtIDE7Cj4+PiArCQltaW4gPSBtaW4obWluLCBzdGFydCk7Cj4+ PiArCQltYXggPSBtYXgobWF4LCBlbmQpOwo+Pj4gKwl9Cj4+PiArCj4+PiArCWlmIChtaW4gPCBt YXgpIHsKPj4+ICsJCXN0cnVjdCBkcm1fY2xpcF9yZWN0IG1tYXBfY2xpcDsKPj4+ICsKPj4+ICsJ CW1tYXBfY2xpcC54MSA9IDA7Cj4+PiArCQltbWFwX2NsaXAueDIgPSBpbmZvLT52YXIueHJlczsK Pj4+ICsJCW1tYXBfY2xpcC55MSA9IG1pbiAvIGluZm8tPmZpeC5saW5lX2xlbmd0aDsKPj4+ICsJ CW1tYXBfY2xpcC55MiA9IG1pbl90KHUzMiwgbWF4IC8gaW5mby0+Zml4LmxpbmVfbGVuZ3RoLAo+ Pj4gKwkJCQkgICAgIGluZm8tPnZhci55cmVzKTsKPj4+ICsJCWRybV9jbGlwX3JlY3RfbWVyZ2Uo JmNsaXAsICZtbWFwX2NsaXAsIDEsIDAsIDAsIDApOwo+Pj4gKwl9Cj4+PiArCj4+PiArCWlmICgh ZHJtX2NsaXBfcmVjdF9pc19lbXB0eSgmY2xpcCkpCj4+PiArCQloZWxwZXItPmZiLT5mdW5jcy0+ ZGlydHkoaGVscGVyLT5mYiwgTlVMTCwgMCwgMCwgJmNsaXAsIDEpOwo+Pj4gK30KPj4+ICtFWFBP UlRfU1lNQk9MKGRybV9mYl9oZWxwZXJfZGVmZXJyZWRfaW8pOwo+PiBUaGVyZSBpcyBvbmUgdGhp bmcgSSBoYXZlIHdvbmRlcmVkIGFib3V0IHdoZW4gaXQgY29tZXMgdG8gZGVmZXJyZWQgaW8gYW5k Cj4+IGxvbmcgcnVuIHRpbWVzIGZvciB0aGUgZGVmaW8gd29ya2VyIHdpdGggbXkgZGlzcGxheXM6 Cj4+Cj4+IFVzZXJzcGFjZSB3cml0ZXMgdG8gc29tZSBwYWdlcyB0aGVuIHRoZSBkZWZlcnJlZCBp byB3b3JrZXIga2lja3Mgb2ZmIGFuZAo+PiBydW5zIGZvciAxMDBtcyBob2xkaW5nIHRoZSBwYWdl IGxpc3QgbXV0ZXguIFdoaWxlIHRoaXMgaXMgaGFwcGVuaW5nLAo+PiB1c2Vyc3BhY2Ugd3JpdGVz IHRvIGEgbmV3IHBhZ2UgdHJpZ2dlcmluZyBhIHBhZ2VfbWt3cml0ZS4gTm93IHRoaXMKPj4gZnVu Y3Rpb24gaGFzIHRvIHdhaXQgZm9yIHRoZSBtdXRleCB0byBiZSByZWxlYXNlZC4KPj4KPj4gV2hv IGlzIGFjdHVhbGx5IHdhaXRpbmcgaGVyZSBhbmQgaXMgdGhlcmUgYSBwcm9ibGVtIHRoYXQgdGhp cyBjYW4gbGFzdAo+PiBmb3IgMTAwbXM/Cj4gTm8gaWRlYSBhdCBhbGwgLSBJIGhhdmVuJ3QgbG9v a2VkIHRoYXQgY2xvc2VseSBhdCAgZmJkZXYgZGVmaW8uIEJ1dCBvbmUKPiByZWFzb24gd2UgaGF2 ZSBhbiBleHBsaWNpdCBpb2N0bCBpbiBkcm0gdG8gZmx1c2ggb3V0IGZyb250YnVmZmVyIHJlbmRl cmluZwo+IGlzIGV4YWN0bHkgdGhhdCBmbHVzaGluZyBjb3VsZCB0YWtlIHNvbWUgdGltZSwgYW5k IHNob3VsZCBvbmx5IGJlIGRvbmUKPiBvbmNlIHVzZXJzcGFjZSBoYXMgY29tcGxldGVkIHNvbWUg cmVuZGVyaW5nLiBOb3QgcmlnaHQgaW4gdGhlIG1pZGRsZSBvZiBhbgo+IG9wLgo+Cj4gSSBndWVz cyBmaXggdXAgeW91ciB1c2Vyc3BhY2UgdG8gdXNlIGR1bWIgZHJtIGZiICsgZHJtIGRpcnR5ZmIg aW9jdGw/Cj4gT3RoZXJ3aXNlIHlvdSdsbCBnZXQgdG8gaW1wcm92ZSBmYmRldiBkZWZpbywgYW5k IGZiZGV2IGlzIGRlcHJlY2F0ZWQKPiBzdWJzeXN0ZW0gYW5kIGEgcmVhbCBob3Jyb3Igc2hvdy4g SSBoaWdobHkgcmVjb21tZW5kIGFnYWluc3QgdG91Y2hpbmcgaXQKPiA7LSkKCkkgaGF2ZSB0cmll ZCB0byB0cmFjayB0aGUgY2FsbCBjaGFpbiBhbmQgaXQgc2VlbXMgdG8gYmUgcGFydCBvZiB0aGUK cGFnZSBmYXVsdCBoYW5kbGVyLiBXaGljaCBtZWFucyBpdCdzIHVzZXJzcGFjZSB3YW50aW5nIHRv IHdyaXRlIHRvIHRoZQpwYWdlIHRoYXQgaGFzIHRvIHdhaXQuIEFuZCBpdCBoYXMgdG8gd2FpdCBh dCBzb21lIHJhbmRvbSBwb2ludCBpbgp3aGF0ZXZlciByZW5kZXJpbmcgaXQncyBkb2luZy4KClVu bGVzcyBzb21lb25lIGhhcyBhbnkgb2JqZWN0aW9ucywgSSB3aWxsIG1ha2UgYSBjaGFuZ2UgYW5k IGFkZCBhIHdvcmtlcgpsaWtlIHF4bCBkb2VzLiBUaGlzIGRlY291cGxlcyB0aGUgZGVmZXJyZWRf aW8gd29ya2VyIGhvbGRpbmcgdGhlIG11dGV4CmZyb20gdGhlIGZyYW1lYnVmZmVyIGZsdXNoaW5n IGpvYi4gSG93ZXZlciBJIGludGVuZCB0byBkaWZmZXIgZnJvbSBxeGwgaW4KdGhhdCBJIHdpbGwg dXNlIGEgZGVsYXllZCB3b3JrZXIgKHJ1biBpbW1lZGlhdGVseSBmcm9tIHRoZSBtbWFwIHNpZGUg d2hpY2gKaGFzIGFscmVhZHkgYmVlbiBkZWZlcnJlZCkuIFNpbmNlIEkgZG9uJ3Qgc2VlIGFueSBw b2ludCBpbiBmbHVzaGluZyB0aGUKZnJhbWVidWZmZXIgaW1tZWRpYXRlbHkgd2hlbiBmYmNvbiBo YXMgcHV0IG91dCBvbmx5IG9uZSBnbHlwaCwgbWlnaHQgYXMKd2VsbCBkZWZlciBpdCBhIGNvdXBs ZSBvZiBqaWZmaWVzIHRvIGJlIGFibGUgdG8gY2FwdHVyZSBzb21lIG1vcmUgZ2x5cGhzLgoKQWRk aW5nIGEgd29ya2VyIGFsc28gbWVhbnMgdGhhdCB1ZGwgZG9lc24ndCBoYXZlIHRvIGluaXRpYWxp emUgZGVmZXJyZWRfaW8KYmVjYXVzZSB3ZSB3b24ndCBiZSB1c2luZyB0aGUgZGVmZXJyZWRfd29y ayB3b3JrZXIgZm9yIGZsdXNoaW5nIGZiXyooKS4KCkFuZCB5ZXMsIHVzaW5nIGRybSBmcm9tIHVz ZXJzcGFjZSBpcyAiVGhlIHNvbHV0aW9uIiBoZXJlIDotKSwgaG93ZXZlcgpJIHdhbnQgdG8gbWFr ZSB0aGUgYmVzdCBvdXQgb2YgZmJkZXYgc2luY2Ugc29tZSBvZiB0aGUgdGlueWRybSB1c2Vycwpj b21pbmcgZnJvbSBkcml2ZXJzL3N0YWdpbmcvZmJ0ZnQgd2lsbCBwcm9iYWJseSBjb250aW51ZSB3 aXRoIGZiZGV2LgoKCk5vcmFsZi4KCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fX19fX19fCmRyaS1kZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVk ZXNrdG9wLm9yZwpodHRwczovL2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZv L2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1754073AbcDVOR1 (ORCPT ); Fri, 22 Apr 2016 10:17:27 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:52506 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753996AbcDVORZ (ORCPT ); Fri, 22 Apr 2016 10:17:25 -0400 Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support To: 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> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <571A326A.9070407@tronnes.org> Date: Fri, 22 Apr 2016 16:17:14 +0200 User-Agent: Mozilla/5.0 (Windows NT 6.3; WOW64; rv:38.0) Gecko/20100101 Thunderbird/38.7.1 MIME-Version: 1.0 In-Reply-To: <20160422082719.GH2510@phenom.ffwll.local> Content-Type: text/plain; charset=windows-1252; format=flowed Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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_*(). 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.