From mboxrd@z Thu Jan 1 00:00:00 1970 From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Date: Fri, 22 Apr 2016 17:28:17 +0000 Subject: Re: [PATCH 4/8] drm/fb-helper: Add fb_deferred_io support Message-Id: <571A5F31.8050301@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> <571A326A.9070407@tronnes.org> <20160422170501.GE2510@phenom.ffwll.local> In-Reply-To: <20160422170501.GE2510@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 19:05, skrev Daniel Vetter: > On Fri, Apr 22, 2016 at 04:17:14PM +0200, Noralf Tr=F8nnes wrote: >> 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 colle= cted >>>>> + * damage. There's no need to worry about the possibility that the f= b_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 a= nd >>>> 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 render= ing >>> 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 whi= ch >> 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. This patch extend the use of the fbdev deferred_io worker to also handle the fbdev drawing operations, which can happen in atomic context. The qxl driver adds an extra worker (struct qxl_device).fb_work which is used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io()) code which is run by the deferred io worker and the fbdev drawing operations (qxl_fb_fillrect() etc.) schedule this fb_work worker. So this patch uses 1 worker, qxl uses 2 workers. It's no big deal for me, fbtft has used 1 worker since 2013 without anyone pointing out that this has been a problem. And and extra worker can be added later without changing the drivers. But since qxl used an extra worker I thought maybe there's a reason for that and it would remove my worry about those page faults being held up. Noralf. > > 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 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 19:28:17 +0200 Message-ID: <571A5F31.8050301@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> <571A326A.9070407@tronnes.org> <20160422170501.GE2510@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 E72066EF36 for ; Fri, 22 Apr 2016 17:28:24 +0000 (UTC) In-Reply-To: <20160422170501.GE2510@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 CkRlbiAyMi4wNC4yMDE2IDE5OjA1LCBza3JldiBEYW5pZWwgVmV0dGVyOgo+IE9uIEZyaSwgQXBy IDIyLCAyMDE2IGF0IDA0OjE3OjE0UE0gKzAyMDAsIE5vcmFsZiBUcsO4bm5lcyB3cm90ZToKPj4g RGVuIDIyLjA0LjIwMTYgMTA6MjcsIHNrcmV2IERhbmllbCBWZXR0ZXI6Cj4+PiBPbiBUaHUsIEFw ciAyMSwgMjAxNiBhdCAwODo1NDo0NVBNICswMjAwLCBOb3JhbGYgVHLDuG5uZXMgd3JvdGU6Cj4+ Pj4gRGVuIDIwLjA0LjIwMTYgMTc6MjUsIHNrcmV2IE5vcmFsZiBUcsO4bm5lczoKPj4+Pj4gVGhp cyBhZGRzIGRlZmVycmVkIGlvIHN1cHBvcnQgaWYgQ09ORklHX0ZCX0RFRkVSUkVEX0lPIGlzIGVu YWJsZWQuCj4+Pj4+IEFjY3VtdWxhdGVkIGZiZGV2IGZyYW1lYnVmZmVyIGNoYW5nZXMgYXJlIHNp Z25hbGVkIHVzaW5nIHRoZSBjYWxsYmFjawo+Pj4+PiAoc3RydWN0IGRybV9mcmFtZWJ1ZmZlcl9m dW5jcyAqKS0+ZGlydHkoKQo+Pj4+Pgo+Pj4+PiBUaGUgZHJtX2ZiX2hlbHBlcl9zeXNfKigpIGZ1 bmN0aW9ucyB3aWxsIGFjY3VtdWxhdGUgY2hhbmdlcyBhbmQKPj4+Pj4gc2NoZWR1bGUgZmJfaW5m by5kZWZlcnJlZF93b3JrIF9pZl8gZmJfaW5mby5mYmRlZmlvIGlzIHNldC4KPj4+Pj4gVGhpcyB3 b3JrZXIgaXMgdXNlZCBieSB0aGUgZGVmZXJyZWQgaW8gbW1hcCBjb2RlIHRvIHNpZ25hbCB0aGF0 IGl0Cj4+Pj4+IGhhcyBiZWVuIGNvbGxlY3RpbmcgcGFnZSBmYXVsdHMuIFRoZSBwYWdlIGZhdWx0 cyBhbmQvb3Igb3RoZXIgY2hhbmdlcwo+Pj4+PiBhcmUgdGhlbiBtZXJnZWQgaW50byBhIGRybV9j bGlwX3JlY3QgYW5kIHBhc3NlZCB0byB0aGUgZnJhbWVidWZmZXIKPj4+Pj4gZGlydHkoKSBmdW5j dGlvbi4KPj4+Pj4KPj4+Pj4gVGhlIGRyaXZlciBpcyByZXNwb25zaWJsZSBmb3Igc2V0dGluZyB1 cCB0aGUgZmJfaW5mby5mYmRlZmlvIHN0cnVjdHVyZQo+Pj4+PiBhbmQgY2FsbGluZyBmYl9kZWZl cnJlZF9pb19pbml0KCkgdXNpbmcgdGhlIHByb3ZpZGVkIGNhbGxiYWNrOgo+Pj4+PiAoc3RydWN0 IGZiX2RlZmVycmVkX2lvKS5kZWZlcnJlZF9pbyA9IGRybV9mYl9oZWxwZXJfZGVmZXJyZWRfaW87 Cj4+Pj4+Cj4+Pj4+IFNpZ25lZC1vZmYtYnk6IE5vcmFsZiBUcsO4bm5lcyA8bm9yYWxmQHRyb25u ZXMub3JnPgo+Pj4+PiAtLS0KPj4+Pj4gICBkcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5j IHwgMTE5ICsrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKy0KPj4+Pj4gICBp bmNsdWRlL2RybS9kcm1fZmJfaGVscGVyLmggICAgIHwgIDE1ICsrKysrCj4+Pj4+ICAgMiBmaWxl cyBjaGFuZ2VkLCAxMzMgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+Pj4+Pgo+Pj4+PiBk aWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYyBiL2RyaXZlcnMvZ3B1 L2RybS9kcm1fZmJfaGVscGVyLmMKPj4+PiBbLi4uXQo+Pj4+Cj4+Pj4+ICsjaWZkZWYgQ09ORklH X0ZCX0RFRkVSUkVEX0lPCj4+Pj4+ICsvKioKPj4+Pj4gKyAqIGRybV9mYl9oZWxwZXJfZGVmZXJy ZWRfaW8oKSAtIChzdHJ1Y3QgZmJfZGVmZXJyZWRfaW8gKiktPmRlZmVycmVkX2lvIGNhbGxiYWNr Cj4+Pj4+ICsgKiAgICAgICAgICAgICAgICAgICAgICAgICAgICAgICBmdW5jdGlvbgo+Pj4+PiAr ICoKPj4+Pj4gKyAqIFRoaXMgZnVuY3Rpb24gYWx3YXlzIHJ1bnMgaW4gcHJvY2VzcyBjb250ZXh0 IChzdHJ1Y3QgZGVsYXllZF93b3JrKSBhbmQgY2FsbHMKPj4+Pj4gKyAqIHRoZSAoc3RydWN0IGRy bV9mcmFtZWJ1ZmZlcl9mdW5jcyktPmRpcnR5IGZ1bmN0aW9uIHdpdGggdGhlIGNvbGxlY3RlZAo+ Pj4+PiArICogZGFtYWdlLiBUaGVyZSdzIG5vIG5lZWQgdG8gd29ycnkgYWJvdXQgdGhlIHBvc3Np YmlsaXR5IHRoYXQgdGhlIGZiX3N5c18qKCkKPj4+Pj4gKyAqIGZ1bmN0aW9ucyBjb3VsZCBiZSBy dW5uaW5nIGluIGF0b21pYyBjb250ZXh0LiBUaGV5IG9ubHkgc2NoZWR1bGUgdGhlCj4+Pj4+ICsg KiBkZWxheWVkIHdvcmtlciB3aGljaCB0aGVuIGNhbGxzIHRoaXMgZGVmZXJyZWRfaW8gY2FsbGJh Y2suCj4+Pj4+ICsgKi8KPj4+Pj4gK3ZvaWQgZHJtX2ZiX2hlbHBlcl9kZWZlcnJlZF9pbyhzdHJ1 Y3QgZmJfaW5mbyAqaW5mbywKPj4+Pj4gKwkJCSAgICAgICBzdHJ1Y3QgbGlzdF9oZWFkICpwYWdl bGlzdCkKPj4+Pj4gK3sKPj4+Pj4gKwlzdHJ1Y3QgZHJtX2ZiX2hlbHBlciAqaGVscGVyID0gaW5m by0+cGFyOwo+Pj4+PiArCXVuc2lnbmVkIGxvbmcgc3RhcnQsIGVuZCwgbWluLCBtYXg7Cj4+Pj4+ ICsJc3RydWN0IGRybV9jbGlwX3JlY3QgY2xpcDsKPj4+Pj4gKwl1bnNpZ25lZCBsb25nIGZsYWdz Owo+Pj4+PiArCXN0cnVjdCBwYWdlICpwYWdlOwo+Pj4+PiArCj4+Pj4+ICsJaWYgKCFoZWxwZXIt PmZiLT5mdW5jcy0+ZGlydHkpCj4+Pj4+ICsJCXJldHVybjsKPj4+Pj4gKwo+Pj4+PiArCXNwaW5f bG9ja19pcnFzYXZlKCZoZWxwZXItPmRpcnR5X2xvY2ssIGZsYWdzKTsKPj4+Pj4gKwljbGlwID0g aGVscGVyLT5kaXJ0eV9jbGlwOwo+Pj4+PiArCWRybV9jbGlwX3JlY3RfcmVzZXQoJmhlbHBlci0+ ZGlydHlfY2xpcCk7Cj4+Pj4+ICsJc3Bpbl91bmxvY2tfaXJxcmVzdG9yZSgmaGVscGVyLT5kaXJ0 eV9sb2NrLCBmbGFncyk7Cj4+Pj4+ICsKPj4+Pj4gKwltaW4gPSBVTE9OR19NQVg7Cj4+Pj4+ICsJ bWF4ID0gMDsKPj4+Pj4gKwlsaXN0X2Zvcl9lYWNoX2VudHJ5KHBhZ2UsIHBhZ2VsaXN0LCBscnUp IHsKPj4+Pj4gKwkJc3RhcnQgPSBwYWdlLT5pbmRleCA8PCBQQUdFX1NISUZUOwo+Pj4+PiArCQll bmQgPSBzdGFydCArIFBBR0VfU0laRSAtIDE7Cj4+Pj4+ICsJCW1pbiA9IG1pbihtaW4sIHN0YXJ0 KTsKPj4+Pj4gKwkJbWF4ID0gbWF4KG1heCwgZW5kKTsKPj4+Pj4gKwl9Cj4+Pj4+ICsKPj4+Pj4g KwlpZiAobWluIDwgbWF4KSB7Cj4+Pj4+ICsJCXN0cnVjdCBkcm1fY2xpcF9yZWN0IG1tYXBfY2xp cDsKPj4+Pj4gKwo+Pj4+PiArCQltbWFwX2NsaXAueDEgPSAwOwo+Pj4+PiArCQltbWFwX2NsaXAu eDIgPSBpbmZvLT52YXIueHJlczsKPj4+Pj4gKwkJbW1hcF9jbGlwLnkxID0gbWluIC8gaW5mby0+ Zml4LmxpbmVfbGVuZ3RoOwo+Pj4+PiArCQltbWFwX2NsaXAueTIgPSBtaW5fdCh1MzIsIG1heCAv IGluZm8tPmZpeC5saW5lX2xlbmd0aCwKPj4+Pj4gKwkJCQkgICAgIGluZm8tPnZhci55cmVzKTsK Pj4+Pj4gKwkJZHJtX2NsaXBfcmVjdF9tZXJnZSgmY2xpcCwgJm1tYXBfY2xpcCwgMSwgMCwgMCwg MCk7Cj4+Pj4+ICsJfQo+Pj4+PiArCj4+Pj4+ICsJaWYgKCFkcm1fY2xpcF9yZWN0X2lzX2VtcHR5 KCZjbGlwKSkKPj4+Pj4gKwkJaGVscGVyLT5mYi0+ZnVuY3MtPmRpcnR5KGhlbHBlci0+ZmIsIE5V TEwsIDAsIDAsICZjbGlwLCAxKTsKPj4+Pj4gK30KPj4+Pj4gK0VYUE9SVF9TWU1CT0woZHJtX2Zi X2hlbHBlcl9kZWZlcnJlZF9pbyk7Cj4+Pj4gVGhlcmUgaXMgb25lIHRoaW5nIEkgaGF2ZSB3b25k ZXJlZCBhYm91dCB3aGVuIGl0IGNvbWVzIHRvIGRlZmVycmVkIGlvIGFuZAo+Pj4+IGxvbmcgcnVu IHRpbWVzIGZvciB0aGUgZGVmaW8gd29ya2VyIHdpdGggbXkgZGlzcGxheXM6Cj4+Pj4KPj4+PiBV c2Vyc3BhY2Ugd3JpdGVzIHRvIHNvbWUgcGFnZXMgdGhlbiB0aGUgZGVmZXJyZWQgaW8gd29ya2Vy IGtpY2tzIG9mZiBhbmQKPj4+PiBydW5zIGZvciAxMDBtcyBob2xkaW5nIHRoZSBwYWdlIGxpc3Qg bXV0ZXguIFdoaWxlIHRoaXMgaXMgaGFwcGVuaW5nLAo+Pj4+IHVzZXJzcGFjZSB3cml0ZXMgdG8g YSBuZXcgcGFnZSB0cmlnZ2VyaW5nIGEgcGFnZV9ta3dyaXRlLiBOb3cgdGhpcwo+Pj4+IGZ1bmN0 aW9uIGhhcyB0byB3YWl0IGZvciB0aGUgbXV0ZXggdG8gYmUgcmVsZWFzZWQuCj4+Pj4KPj4+PiBX aG8gaXMgYWN0dWFsbHkgd2FpdGluZyBoZXJlIGFuZCBpcyB0aGVyZSBhIHByb2JsZW0gdGhhdCB0 aGlzIGNhbiBsYXN0Cj4+Pj4gZm9yIDEwMG1zPwo+Pj4gTm8gaWRlYSBhdCBhbGwgLSBJIGhhdmVu J3QgbG9va2VkIHRoYXQgY2xvc2VseSBhdCAgZmJkZXYgZGVmaW8uIEJ1dCBvbmUKPj4+IHJlYXNv biB3ZSBoYXZlIGFuIGV4cGxpY2l0IGlvY3RsIGluIGRybSB0byBmbHVzaCBvdXQgZnJvbnRidWZm ZXIgcmVuZGVyaW5nCj4+PiBpcyBleGFjdGx5IHRoYXQgZmx1c2hpbmcgY291bGQgdGFrZSBzb21l IHRpbWUsIGFuZCBzaG91bGQgb25seSBiZSBkb25lCj4+PiBvbmNlIHVzZXJzcGFjZSBoYXMgY29t cGxldGVkIHNvbWUgcmVuZGVyaW5nLiBOb3QgcmlnaHQgaW4gdGhlIG1pZGRsZSBvZiBhbgo+Pj4g b3AuCj4+Pgo+Pj4gSSBndWVzcyBmaXggdXAgeW91ciB1c2Vyc3BhY2UgdG8gdXNlIGR1bWIgZHJt IGZiICsgZHJtIGRpcnR5ZmIgaW9jdGw/Cj4+PiBPdGhlcndpc2UgeW91J2xsIGdldCB0byBpbXBy b3ZlIGZiZGV2IGRlZmlvLCBhbmQgZmJkZXYgaXMgZGVwcmVjYXRlZAo+Pj4gc3Vic3lzdGVtIGFu ZCBhIHJlYWwgaG9ycm9yIHNob3cuIEkgaGlnaGx5IHJlY29tbWVuZCBhZ2FpbnN0IHRvdWNoaW5n IGl0Cj4+PiA7LSkKPj4gSSBoYXZlIHRyaWVkIHRvIHRyYWNrIHRoZSBjYWxsIGNoYWluIGFuZCBp dCBzZWVtcyB0byBiZSBwYXJ0IG9mIHRoZQo+PiBwYWdlIGZhdWx0IGhhbmRsZXIuIFdoaWNoIG1l YW5zIGl0J3MgdXNlcnNwYWNlIHdhbnRpbmcgdG8gd3JpdGUgdG8gdGhlCj4+IHBhZ2UgdGhhdCBo YXMgdG8gd2FpdC4gQW5kIGl0IGhhcyB0byB3YWl0IGF0IHNvbWUgcmFuZG9tIHBvaW50IGluCj4+ IHdoYXRldmVyIHJlbmRlcmluZyBpdCdzIGRvaW5nLgo+Pgo+PiBVbmxlc3Mgc29tZW9uZSBoYXMg YW55IG9iamVjdGlvbnMsIEkgd2lsbCBtYWtlIGEgY2hhbmdlIGFuZCBhZGQgYSB3b3JrZXIKPj4g bGlrZSBxeGwgZG9lcy4gVGhpcyBkZWNvdXBsZXMgdGhlIGRlZmVycmVkX2lvIHdvcmtlciBob2xk aW5nIHRoZSBtdXRleAo+PiBmcm9tIHRoZSBmcmFtZWJ1ZmZlciBmbHVzaGluZyBqb2IuIEhvd2V2 ZXIgSSBpbnRlbmQgdG8gZGlmZmVyIGZyb20gcXhsIGluCj4+IHRoYXQgSSB3aWxsIHVzZSBhIGRl bGF5ZWQgd29ya2VyIChydW4gaW1tZWRpYXRlbHkgZnJvbSB0aGUgbW1hcCBzaWRlIHdoaWNoCj4+ IGhhcyBhbHJlYWR5IGJlZW4gZGVmZXJyZWQpLiBTaW5jZSBJIGRvbid0IHNlZSBhbnkgcG9pbnQg aW4gZmx1c2hpbmcgdGhlCj4+IGZyYW1lYnVmZmVyIGltbWVkaWF0ZWx5IHdoZW4gZmJjb24gaGFz IHB1dCBvdXQgb25seSBvbmUgZ2x5cGgsIG1pZ2h0IGFzCj4+IHdlbGwgZGVmZXIgaXQgYSBjb3Vw bGUgb2YgamlmZmllcyB0byBiZSBhYmxlIHRvIGNhcHR1cmUgc29tZSBtb3JlIGdseXBocy4KPj4K Pj4gQWRkaW5nIGEgd29ya2VyIGFsc28gbWVhbnMgdGhhdCB1ZGwgZG9lc24ndCBoYXZlIHRvIGlu aXRpYWxpemUgZGVmZXJyZWRfaW8KPj4gYmVjYXVzZSB3ZSB3b24ndCBiZSB1c2luZyB0aGUgZGVm ZXJyZWRfd29yayB3b3JrZXIgZm9yIGZsdXNoaW5nIGZiXyooKS4KPiBJJ20gY29uZnVzZWQgLi4u IEkgdGhvdWdodCB3ZSBhbHJlYWR5IGhhdmUgZW5vdWdoIHdvcmtlcnM/IE9uZSBpbiB0aGUKPiBm YmRldiBkZWZlcnJlZF9pbyBpbXBsZW1lbnRhdGlvbiB1c2VkIGJ5IGRlZmF1bHQuIFRoZSBvdGhl ciBpbiBjYXNlIHdlIGdldAo+IGEgZHJhdyBjYWxsIGZyb20gYW4gYXRvbWljIGNvbnRleHQuCgpU aGlzIHBhdGNoIGV4dGVuZCB0aGUgdXNlIG9mIHRoZSBmYmRldiBkZWZlcnJlZF9pbyB3b3JrZXIg dG8gYWxzbyBoYW5kbGUKdGhlIGZiZGV2IGRyYXdpbmcgb3BlcmF0aW9ucywgd2hpY2ggY2FuIGhh cHBlbiBpbiBhdG9taWMgY29udGV4dC4KVGhlIHF4bCBkcml2ZXIgYWRkcyBhbiBleHRyYSB3b3Jr ZXIgKHN0cnVjdCBxeGxfZGV2aWNlKS5mYl93b3JrIHdoaWNoIGlzCnVzZWQgdG8gZmx1c2ggdGhl IGZyYW1lYnVmZmVyLiBCb3RoIHRoZSBtbWFwIGRlZmVycmVkX2lvIChxeGxfZGVmZXJyZWRfaW8o KSkKY29kZSB3aGljaCBpcyBydW4gYnkgdGhlIGRlZmVycmVkIGlvIHdvcmtlciBhbmQgdGhlIGZi ZGV2IGRyYXdpbmcgb3BlcmF0aW9ucwoocXhsX2ZiX2ZpbGxyZWN0KCkgZXRjLikgc2NoZWR1bGUg dGhpcyBmYl93b3JrIHdvcmtlci4KClNvIHRoaXMgcGF0Y2ggdXNlcyAxIHdvcmtlciwgcXhsIHVz ZXMgMiB3b3JrZXJzLgoKSXQncyBubyBiaWcgZGVhbCBmb3IgbWUsIGZidGZ0IGhhcyB1c2VkIDEg d29ya2VyIHNpbmNlIDIwMTMgd2l0aG91dCBhbnlvbmUKcG9pbnRpbmcgb3V0IHRoYXQgdGhpcyBo YXMgYmVlbiBhIHByb2JsZW0uIEFuZCBhbmQgZXh0cmEgd29ya2VyIGNhbiBiZQphZGRlZCBsYXRl ciB3aXRob3V0IGNoYW5naW5nIHRoZSBkcml2ZXJzLgpCdXQgc2luY2UgcXhsIHVzZWQgYW4gZXh0 cmEgd29ya2VyIEkgdGhvdWdodCBtYXliZSB0aGVyZSdzIGEgcmVhc29uIGZvcgp0aGF0IGFuZCBp dCB3b3VsZCByZW1vdmUgbXkgd29ycnkgYWJvdXQgdGhvc2UgcGFnZSBmYXVsdHMgYmVpbmcgaGVs ZCB1cC4KCgpOb3JhbGYuCgo+Cj4gV2h5IGRvIHdlIG5lZWQgZXZlbiBtb3JlIHdvcmtlcnM/IEhh dmUgeW91IG1lYXN1cmVkIHRoYXQgeW91IGFjdHVhbGx5IGhpdAo+IHRoaXMgZGVsYXksIG9yIGp1 c3QgY29uamVjdHVyZSBmcm9tIHJlYWRpbmcgdGhlIGNvZGU/IEJlY2F1c2UgbXkgcmVhZGluZwo+ IHNheXMgdGhhdCB0aGUgZGVmaW8gbW1hcCBzdXBwb3J0IGluIGZiZGV2IGFscmVhZHkgZG9lcyB3 aGF0IHlvdSB3YW50LCBhbmQKPiBzaG91bGQgc3VmZmljaWVudGx5IGNvYWxlc2NlIG1tYXAgYWNj ZXNzLiBUaGVyZSdzIGEgZGVsYXllZCB3b3JrL3RpbWVyIGluCj4gdGhlcmUgdG8gbWFrZSBzdXJl IGl0IGRvZXNuJ3QgZmx1c2ggb24gdGhlIHZlcnkgZmlyc3QgZmF1bHRlZCBwYWdlLgo+IC1EYW5p ZWwKPgo+PiBBbmQgeWVzLCB1c2luZyBkcm0gZnJvbSB1c2Vyc3BhY2UgaXMgIlRoZSBzb2x1dGlv biIgaGVyZSA6LSksIGhvd2V2ZXIKPj4gSSB3YW50IHRvIG1ha2UgdGhlIGJlc3Qgb3V0IG9mIGZi ZGV2IHNpbmNlIHNvbWUgb2YgdGhlIHRpbnlkcm0gdXNlcnMKPj4gY29taW5nIGZyb20gZHJpdmVy cy9zdGFnaW5nL2ZidGZ0IHdpbGwgcHJvYmFibHkgY29udGludWUgd2l0aCBmYmRldi4KPj4KPj4K Pj4gTm9yYWxmLgo+Pgo+PiBfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f X19fX19fXwo+PiBkcmktZGV2ZWwgbWFpbGluZyBsaXN0Cj4+IGRyaS1kZXZlbEBsaXN0cy5mcmVl ZGVza3RvcC5vcmcKPj4gaHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0 aW5mby9kcmktZGV2ZWwKCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f 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 S932349AbcDVR21 (ORCPT ); Fri, 22 Apr 2016 13:28:27 -0400 Received: from smtp.domeneshop.no ([194.63.252.55]:46296 "EHLO smtp.domeneshop.no" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1754092AbcDVR20 (ORCPT ); Fri, 22 Apr 2016 13:28:26 -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> <571A326A.9070407@tronnes.org> <20160422170501.GE2510@phenom.ffwll.local> From: =?UTF-8?Q?Noralf_Tr=c3=b8nnes?= Message-ID: <571A5F31.8050301@tronnes.org> Date: Fri, 22 Apr 2016 19:28:17 +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: <20160422170501.GE2510@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 19:05, skrev Daniel Vetter: > 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. This patch extend the use of the fbdev deferred_io worker to also handle the fbdev drawing operations, which can happen in atomic context. The qxl driver adds an extra worker (struct qxl_device).fb_work which is used to flush the framebuffer. Both the mmap deferred_io (qxl_deferred_io()) code which is run by the deferred io worker and the fbdev drawing operations (qxl_fb_fillrect() etc.) schedule this fb_work worker. So this patch uses 1 worker, qxl uses 2 workers. It's no big deal for me, fbtft has used 1 worker since 2013 without anyone pointing out that this has been a problem. And and extra worker can be added later without changing the drivers. But since qxl used an extra worker I thought maybe there's a reason for that and it would remove my worry about those page faults being held up. Noralf. > > 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