From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Date: Wed, 27 Apr 2016 11:14:20 +0000 Subject: Re: [PATCH v2 4/8] drm/fb-helper: Add fb_deferred_io support Message-Id: <20160427111420.GA2558@phenom.ffwll.local> List-Id: References: <1461530942-22485-1-git-send-email-noralf@tronnes.org> <1461530942-22485-5-git-send-email-noralf@tronnes.org> <20160425090957.GQ2510@phenom.ffwll.local> <571F9656.1090506@tronnes.org> <20160426171902.GA2558@phenom.ffwll.local> <57208A3B.4020909@tronnes.org> In-Reply-To: <57208A3B.4020909@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 Wed, Apr 27, 2016 at 11:45:31AM +0200, Noralf Tr=F8nnes wrote: >=20 > Den 26.04.2016 19:19, skrev Daniel Vetter: > >On Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Tr=F8nnes wrote: > >>Den 25.04.2016 11:09, skrev Daniel Vetter: > >>>On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Tr=F8nnes wrote: > >>>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. > >>>>The fbdev framebuffer changes are flushed using the callback > >>>>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker > >>>>ensuring that it always runs in process context. > >>>> > >>>>Signed-off-by: Noralf Tr=F8nnes > >>>>--- > >>>> > >>>>Changes since v1: > >>>>- Use a dedicated worker to run the framebuffer flushing like qxl does > >>>>- Add parameter descriptions to drm_fb_helper_deferred_io > >>>> > >>>> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++= ++++++++++- > >>>> include/drm/drm_fb_helper.h | 17 ++++++ > >>>> 2 files changed, 143 insertions(+), 1 deletion(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb= _helper.c > >>>>index 855108e..46ee6f8 100644 > >>>>--- a/drivers/gpu/drm/drm_fb_helper.c > >>>>+++ b/drivers/gpu/drm/drm_fb_helper.c > >>>>@@ -40,6 +40,7 @@ > >>>> #include > >>>> #include > >>>> #include > >>>>+#include > >>>> > >>>> static bool drm_fbdev_emulation =3D true; > >>>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600= ); > >>>>@@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation, > >>>> > >>>> static LIST_HEAD(kernel_fb_helper_list); > >>>> > >>>>+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper); > >>>>+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, > >>>>+ u32 width, u32 height); > >>>>+ > >>>> /** > >>>> * DOC: fbdev helpers > >>>> * > >>>>@@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list); > >>>> * and set up an initial configuration using the detected hardware,= drivers > >>>> * should call drm_fb_helper_single_add_all_connectors() followed by > >>>> * drm_fb_helper_initial_config(). > >>>>+ * > >>>>+ * If CONFIG_FB_DEFERRED_IO is enabled and > >>>>+ * (struct drm_framebuffer *)->funcs->dirty is set, the > >>>>+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} funct= ions > >>>>+ * will accumulate changes and schedule (struct fb_helper).dirty_wor= k to run > >>>>+ * right away. This worker then calls the dirty() function ensuring = that it > >>>>+ * will always run in process context since the fb_*() function coul= d be > >>>>+ * running in atomic context. If drm_fb_helper_deferred_io() is used= as the > >>>>+ * deferred_io callback it will also schedule dirty_work with the da= mage > >>>>+ * collected from the mmap page writes. > >>>One thing to consider (and personally I don't care either way) is whet= her > >>>we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are > >>>enabled. Pushing that out to drivers is imo a bit fragile. > >>> > >>>But like I said I'm ok with either way. > >>My concern was adding code and data that only a few drivers would > >>actually use. But of course there's the tradeoff with complexity. > >>I use this to enable it: > >> select FB_DEFERRED_IO if DRM_KMS_FB_HELPER > >> > >>I guess the maintainer has to make this choice between size and complex= ity > >>:-) > >>I can enable it by default if you want, drm is both huge and complex so= I > >>don't know what's best. > >> > >>As a sidenote, I have also put all the fbdev code in a file of it's own= to > >>make it simple with regards to the DRM_FBDEV_EMULATION user option: > >>tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) +=3D tinydrm-fbdev.o > >Ok, if you ask maintainers then please nuke the #ifdef from .c files. If > >you select CONFIG_DRM_KMS_FB_HELPER, then you get hdmi, edid, dp aux, dp > >mst and whatever else helpers, even if you don't need them. Adding 3 > >functions for defio when you select fbdev helpers and maybe don't need > >them is totally harmless. And removing the #ifdef will look so much bett= er > >;-) >=20 > Will do :-) > Kernel development is just my hobby so I'm not well versed in all of this. You're doing great tbh! > >>>> */ > >>>> > >>>> /** > >>>>@@ -401,11 +416,14 @@ backoff: > >>>> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) > >>>> { > >>>> struct drm_device *dev =3D fb_helper->dev; > >>>>+ struct fb_info *info =3D fb_helper->fbdev; > >>>> struct drm_plane *plane; > >>>> int i; > >>>> > >>>> drm_warn_on_modeset_not_all_locked(dev); > >>>> > >>>>+ drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres); > >>>Why is this needed? If you do a modeset (or pageflip or whatever) driv= ers > >>>are supposed to re-upload the entire screen. We've talked about adding= a > >>>dirty rectangle to atomic to allow userspace to optimize this, but the= re > >>>should _never_ be a need to do a dirtyfb call around a modeset. Probab= ly > >>>just a driver bug in your panel drm drivers? > >>Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs > >>->plane_update to indicate that the next dirty() should do the whole > >>framebuffer which seems to work fine. > >>Should I actually perform the update as well? > >>If so I would need to add a worker in tinydrm to do that. > >Yes, plane update should always do a full update. Not sure how you get > >away with delaying that to ->dirty, maybe modesetting isn't > >double-buffering when you don't have a GL that could do glamour. > > > >->dirty is _only_ for frontbuffer rendering, not for page-flipping to an > >entirely new buffer. In short if someone calls ->dirty on a buffer which > >is currently not being displayed than a) they're silly b) drivers should > >treat it as a no-op. Maybe we need a helper to do that ... > >-Daniel >=20 > drm_fb_helper will call dirty() as long as there's fbdev activity, so the > driver needs to take that into account. For instance fbcon with a blinking > cursor will trigger calls even if a buffer has been set up on the drm sid= e. > tinydrm checks the fb against the fb set on the plane and if it differs > it's a no-op. Was really just an idea to make drivers a bit simpler, since pretty much all of them we need to do this check. But with a grand total of just 3 (4 with tinydrm) implementing a non-trivial dirty callback that's not really worth it I think. -Daniel --=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 v2 4/8] drm/fb-helper: Add fb_deferred_io support Date: Wed, 27 Apr 2016 13:14:20 +0200 Message-ID: <20160427111420.GA2558@phenom.ffwll.local> References: <1461530942-22485-1-git-send-email-noralf@tronnes.org> <1461530942-22485-5-git-send-email-noralf@tronnes.org> <20160425090957.GQ2510@phenom.ffwll.local> <571F9656.1090506@tronnes.org> <20160426171902.GA2558@phenom.ffwll.local> <57208A3B.4020909@tronnes.org> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x244.google.com (mail-wm0-x244.google.com [IPv6:2a00:1450:400c:c09::244]) by gabe.freedesktop.org (Postfix) with ESMTPS id F185C6EA6B for ; Wed, 27 Apr 2016 11:14:24 +0000 (UTC) Received: by mail-wm0-x244.google.com with SMTP id w143so11870106wmw.3 for ; Wed, 27 Apr 2016 04:14:24 -0700 (PDT) Content-Disposition: inline In-Reply-To: <57208A3B.4020909@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 T24gV2VkLCBBcHIgMjcsIDIwMTYgYXQgMTE6NDU6MzFBTSArMDIwMCwgTm9yYWxmIFRyw7hubmVz IHdyb3RlOgo+IAo+IERlbiAyNi4wNC4yMDE2IDE5OjE5LCBza3JldiBEYW5pZWwgVmV0dGVyOgo+ ID5PbiBUdWUsIEFwciAyNiwgMjAxNiBhdCAwNjoyNDo1NFBNICswMjAwLCBOb3JhbGYgVHLDuG5u ZXMgd3JvdGU6Cj4gPj5EZW4gMjUuMDQuMjAxNiAxMTowOSwgc2tyZXYgRGFuaWVsIFZldHRlcjoK PiA+Pj5PbiBTdW4sIEFwciAyNCwgMjAxNiBhdCAxMDo0ODo1OFBNICswMjAwLCBOb3JhbGYgVHLD uG5uZXMgd3JvdGU6Cj4gPj4+PlRoaXMgYWRkcyBkZWZlcnJlZCBpbyBzdXBwb3J0IGlmIENPTkZJ R19GQl9ERUZFUlJFRF9JTyBpcyBlbmFibGVkLgo+ID4+Pj5UaGUgZmJkZXYgZnJhbWVidWZmZXIg Y2hhbmdlcyBhcmUgZmx1c2hlZCB1c2luZyB0aGUgY2FsbGJhY2sKPiA+Pj4+KHN0cnVjdCBkcm1f ZnJhbWVidWZmZXIgKiktPmZ1bmNzLT5kaXJ0eSgpIGJ5IGEgZGVkaWNhdGVkIHdvcmtlcgo+ID4+ Pj5lbnN1cmluZyB0aGF0IGl0IGFsd2F5cyBydW5zIGluIHByb2Nlc3MgY29udGV4dC4KPiA+Pj4+ Cj4gPj4+PlNpZ25lZC1vZmYtYnk6IE5vcmFsZiBUcsO4bm5lcyA8bm9yYWxmQHRyb25uZXMub3Jn Pgo+ID4+Pj4tLS0KPiA+Pj4+Cj4gPj4+PkNoYW5nZXMgc2luY2UgdjE6Cj4gPj4+Pi0gVXNlIGEg ZGVkaWNhdGVkIHdvcmtlciB0byBydW4gdGhlIGZyYW1lYnVmZmVyIGZsdXNoaW5nIGxpa2UgcXhs IGRvZXMKPiA+Pj4+LSBBZGQgcGFyYW1ldGVyIGRlc2NyaXB0aW9ucyB0byBkcm1fZmJfaGVscGVy X2RlZmVycmVkX2lvCj4gPj4+Pgo+ID4+Pj4gIGRyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVy LmMgfCAxMjcgKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrKysrLQo+ID4+Pj4g IGluY2x1ZGUvZHJtL2RybV9mYl9oZWxwZXIuaCAgICAgfCAgMTcgKysrKysrCj4gPj4+PiAgMiBm aWxlcyBjaGFuZ2VkLCAxNDMgaW5zZXJ0aW9ucygrKSwgMSBkZWxldGlvbigtKQo+ID4+Pj4KPiA+ Pj4+ZGlmZiAtLWdpdCBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMgYi9kcml2ZXJz L2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4gPj4+PmluZGV4IDg1NTEwOGUuLjQ2ZWU2ZjggMTAw NjQ0Cj4gPj4+Pi0tLSBhL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiA+Pj4+Kysr IGIvZHJpdmVycy9ncHUvZHJtL2RybV9mYl9oZWxwZXIuYwo+ID4+Pj5AQCAtNDAsNiArNDAsNyBA QAo+ID4+Pj4gICNpbmNsdWRlIDxkcm0vZHJtX2NydGNfaGVscGVyLmg+Cj4gPj4+PiAgI2luY2x1 ZGUgPGRybS9kcm1fYXRvbWljLmg+Cj4gPj4+PiAgI2luY2x1ZGUgPGRybS9kcm1fYXRvbWljX2hl bHBlci5oPgo+ID4+Pj4rI2luY2x1ZGUgPGRybS9kcm1fcmVjdC5oPgo+ID4+Pj4KPiA+Pj4+ICBz dGF0aWMgYm9vbCBkcm1fZmJkZXZfZW11bGF0aW9uID0gdHJ1ZTsKPiA+Pj4+ICBtb2R1bGVfcGFy YW1fbmFtZWQoZmJkZXZfZW11bGF0aW9uLCBkcm1fZmJkZXZfZW11bGF0aW9uLCBib29sLCAwNjAw KTsKPiA+Pj4+QEAgLTQ4LDYgKzQ5LDEwIEBAIE1PRFVMRV9QQVJNX0RFU0MoZmJkZXZfZW11bGF0 aW9uLAo+ID4+Pj4KPiA+Pj4+ICBzdGF0aWMgTElTVF9IRUFEKGtlcm5lbF9mYl9oZWxwZXJfbGlz dCk7Cj4gPj4+Pgo+ID4+Pj4rc3RhdGljIHZvaWQgZHJtX2ZiX2hlbHBlcl9kaXJ0eV9pbml0KHN0 cnVjdCBkcm1fZmJfaGVscGVyICpoZWxwZXIpOwo+ID4+Pj4rc3RhdGljIHZvaWQgZHJtX2ZiX2hl bHBlcl9kaXJ0eShzdHJ1Y3QgZmJfaW5mbyAqaW5mbywgdTMyIHgsIHUzMiB5LAo+ID4+Pj4rCQkJ CXUzMiB3aWR0aCwgdTMyIGhlaWdodCk7Cj4gPj4+PisKPiA+Pj4+ICAvKioKPiA+Pj4+ICAgKiBE T0M6IGZiZGV2IGhlbHBlcnMKPiA+Pj4+ICAgKgo+ID4+Pj5AQCAtODQsNiArODksMTYgQEAgc3Rh dGljIExJU1RfSEVBRChrZXJuZWxfZmJfaGVscGVyX2xpc3QpOwo+ID4+Pj4gICAqIGFuZCBzZXQg dXAgYW4gaW5pdGlhbCBjb25maWd1cmF0aW9uIHVzaW5nIHRoZSBkZXRlY3RlZCBoYXJkd2FyZSwg ZHJpdmVycwo+ID4+Pj4gICAqIHNob3VsZCBjYWxsIGRybV9mYl9oZWxwZXJfc2luZ2xlX2FkZF9h bGxfY29ubmVjdG9ycygpIGZvbGxvd2VkIGJ5Cj4gPj4+PiAgICogZHJtX2ZiX2hlbHBlcl9pbml0 aWFsX2NvbmZpZygpLgo+ID4+Pj4rICoKPiA+Pj4+KyAqIElmIENPTkZJR19GQl9ERUZFUlJFRF9J TyBpcyBlbmFibGVkIGFuZAo+ID4+Pj4rICogKHN0cnVjdCBkcm1fZnJhbWVidWZmZXIgKiktPmZ1 bmNzLT5kaXJ0eSBpcyBzZXQsIHRoZQo+ID4+Pj4rICogZHJtX2ZiX2hlbHBlcl97Y2ZiLHN5c31f e3dyaXRlLGZpbGxyZWN0LGNvcHlhcmVhLGltYWdlYmxpdH0gZnVuY3Rpb25zCj4gPj4+PisgKiB3 aWxsIGFjY3VtdWxhdGUgY2hhbmdlcyBhbmQgc2NoZWR1bGUgKHN0cnVjdCBmYl9oZWxwZXIpLmRp cnR5X3dvcmsgdG8gcnVuCj4gPj4+PisgKiByaWdodCBhd2F5LiBUaGlzIHdvcmtlciB0aGVuIGNh bGxzIHRoZSBkaXJ0eSgpIGZ1bmN0aW9uIGVuc3VyaW5nIHRoYXQgaXQKPiA+Pj4+KyAqIHdpbGwg YWx3YXlzIHJ1biBpbiBwcm9jZXNzIGNvbnRleHQgc2luY2UgdGhlIGZiXyooKSBmdW5jdGlvbiBj b3VsZCBiZQo+ID4+Pj4rICogcnVubmluZyBpbiBhdG9taWMgY29udGV4dC4gSWYgZHJtX2ZiX2hl bHBlcl9kZWZlcnJlZF9pbygpIGlzIHVzZWQgYXMgdGhlCj4gPj4+PisgKiBkZWZlcnJlZF9pbyBj YWxsYmFjayBpdCB3aWxsIGFsc28gc2NoZWR1bGUgZGlydHlfd29yayB3aXRoIHRoZSBkYW1hZ2UK PiA+Pj4+KyAqIGNvbGxlY3RlZCBmcm9tIHRoZSBtbWFwIHBhZ2Ugd3JpdGVzLgo+ID4+Pk9uZSB0 aGluZyB0byBjb25zaWRlciAoYW5kIHBlcnNvbmFsbHkgSSBkb24ndCBjYXJlIGVpdGhlciB3YXkp IGlzIHdoZXRoZXIKPiA+Pj53ZSBzaG91bGRuJ3QganVzdCBzZWxlY3QgQ09ORklHX0ZCX0RFRkVS UkVEX0lPIGlmIHRoZSBmYmRldiBoZWxwZXJzIGFyZQo+ID4+PmVuYWJsZWQuIFB1c2hpbmcgdGhh dCBvdXQgdG8gZHJpdmVycyBpcyBpbW8gYSBiaXQgZnJhZ2lsZS4KPiA+Pj4KPiA+Pj5CdXQgbGlr ZSBJIHNhaWQgSSdtIG9rIHdpdGggZWl0aGVyIHdheS4KPiA+Pk15IGNvbmNlcm4gd2FzIGFkZGlu ZyBjb2RlIGFuZCBkYXRhIHRoYXQgb25seSBhIGZldyBkcml2ZXJzIHdvdWxkCj4gPj5hY3R1YWxs eSB1c2UuIEJ1dCBvZiBjb3Vyc2UgdGhlcmUncyB0aGUgdHJhZGVvZmYgd2l0aCBjb21wbGV4aXR5 Lgo+ID4+SSB1c2UgdGhpcyB0byBlbmFibGUgaXQ6Cj4gPj4gICAgICAgICBzZWxlY3QgRkJfREVG RVJSRURfSU8gaWYgRFJNX0tNU19GQl9IRUxQRVIKPiA+Pgo+ID4+SSBndWVzcyB0aGUgbWFpbnRh aW5lciBoYXMgdG8gbWFrZSB0aGlzIGNob2ljZSBiZXR3ZWVuIHNpemUgYW5kIGNvbXBsZXhpdHkK PiA+PjotKQo+ID4+SSBjYW4gZW5hYmxlIGl0IGJ5IGRlZmF1bHQgaWYgeW91IHdhbnQsIGRybSBp cyBib3RoIGh1Z2UgYW5kIGNvbXBsZXggc28gSQo+ID4+ZG9uJ3Qga25vdyB3aGF0J3MgYmVzdC4K PiA+Pgo+ID4+QXMgYSBzaWRlbm90ZSwgSSBoYXZlIGFsc28gcHV0IGFsbCB0aGUgZmJkZXYgY29k ZSBpbiBhIGZpbGUgb2YgaXQncyBvd24gdG8KPiA+Pm1ha2UgaXQgc2ltcGxlIHdpdGggcmVnYXJk cyB0byB0aGUgRFJNX0ZCREVWX0VNVUxBVElPTiB1c2VyIG9wdGlvbjoKPiA+PnRpbnlkcm0tJChD T05GSUdfRFJNX0tNU19GQl9IRUxQRVIpICAgICArPSB0aW55ZHJtLWZiZGV2Lm8KPiA+T2ssIGlm IHlvdSBhc2sgbWFpbnRhaW5lcnMgdGhlbiBwbGVhc2UgbnVrZSB0aGUgI2lmZGVmIGZyb20gLmMg ZmlsZXMuIElmCj4gPnlvdSBzZWxlY3QgQ09ORklHX0RSTV9LTVNfRkJfSEVMUEVSLCB0aGVuIHlv dSBnZXQgaGRtaSwgZWRpZCwgZHAgYXV4LCBkcAo+ID5tc3QgYW5kIHdoYXRldmVyIGVsc2UgaGVs cGVycywgZXZlbiBpZiB5b3UgZG9uJ3QgbmVlZCB0aGVtLiBBZGRpbmcgMwo+ID5mdW5jdGlvbnMg Zm9yIGRlZmlvIHdoZW4geW91IHNlbGVjdCBmYmRldiBoZWxwZXJzIGFuZCBtYXliZSBkb24ndCBu ZWVkCj4gPnRoZW0gaXMgdG90YWxseSBoYXJtbGVzcy4gQW5kIHJlbW92aW5nIHRoZSAjaWZkZWYg d2lsbCBsb29rIHNvIG11Y2ggYmV0dGVyCj4gPjstKQo+IAo+IFdpbGwgZG8gOi0pCj4gS2VybmVs IGRldmVsb3BtZW50IGlzIGp1c3QgbXkgaG9iYnkgc28gSSdtIG5vdCB3ZWxsIHZlcnNlZCBpbiBh bGwgb2YgdGhpcy4KCllvdSdyZSBkb2luZyBncmVhdCB0YmghCgo+ID4+Pj4gICAqLwo+ID4+Pj4K PiA+Pj4+ICAvKioKPiA+Pj4+QEAgLTQwMSwxMSArNDE2LDE0IEBAIGJhY2tvZmY6Cj4gPj4+PiAg c3RhdGljIGludCByZXN0b3JlX2ZiZGV2X21vZGUoc3RydWN0IGRybV9mYl9oZWxwZXIgKmZiX2hl bHBlcikKPiA+Pj4+ICB7Cj4gPj4+PiAgCXN0cnVjdCBkcm1fZGV2aWNlICpkZXYgPSBmYl9oZWxw ZXItPmRldjsKPiA+Pj4+KwlzdHJ1Y3QgZmJfaW5mbyAqaW5mbyA9IGZiX2hlbHBlci0+ZmJkZXY7 Cj4gPj4+PiAgCXN0cnVjdCBkcm1fcGxhbmUgKnBsYW5lOwo+ID4+Pj4gIAlpbnQgaTsKPiA+Pj4+ Cj4gPj4+PiAgCWRybV93YXJuX29uX21vZGVzZXRfbm90X2FsbF9sb2NrZWQoZGV2KTsKPiA+Pj4+ Cj4gPj4+PisJZHJtX2ZiX2hlbHBlcl9kaXJ0eShpbmZvLCAwLCAwLCBpbmZvLT52YXIueHJlcywg aW5mby0+dmFyLnlyZXMpOwo+ID4+PldoeSBpcyB0aGlzIG5lZWRlZD8gSWYgeW91IGRvIGEgbW9k ZXNldCAob3IgcGFnZWZsaXAgb3Igd2hhdGV2ZXIpIGRyaXZlcnMKPiA+Pj5hcmUgc3VwcG9zZWQg dG8gcmUtdXBsb2FkIHRoZSBlbnRpcmUgc2NyZWVuLiBXZSd2ZSB0YWxrZWQgYWJvdXQgYWRkaW5n IGEKPiA+Pj5kaXJ0eSByZWN0YW5nbGUgdG8gYXRvbWljIHRvIGFsbG93IHVzZXJzcGFjZSB0byBv cHRpbWl6ZSB0aGlzLCBidXQgdGhlcmUKPiA+Pj5zaG91bGQgX25ldmVyXyBiZSBhIG5lZWQgdG8g ZG8gYSBkaXJ0eWZiIGNhbGwgYXJvdW5kIGEgbW9kZXNldC4gUHJvYmFibHkKPiA+Pj5qdXN0IGEg ZHJpdmVyIGJ1ZyBpbiB5b3VyIHBhbmVsIGRybSBkcml2ZXJzPwo+ID4+T2ssIGluIHRpbnlkcm0g SSBub3cgc2V0IGEgZmxhZyBpbiAmZHJtX3NpbXBsZV9kaXNwbGF5X3BpcGVfZnVuY3MKPiA+Pi0+ cGxhbmVfdXBkYXRlIHRvIGluZGljYXRlIHRoYXQgdGhlIG5leHQgZGlydHkoKSBzaG91bGQgZG8g dGhlIHdob2xlCj4gPj5mcmFtZWJ1ZmZlciB3aGljaCBzZWVtcyB0byB3b3JrIGZpbmUuCj4gPj5T aG91bGQgSSBhY3R1YWxseSBwZXJmb3JtIHRoZSB1cGRhdGUgYXMgd2VsbD8KPiA+PklmIHNvIEkg d291bGQgbmVlZCB0byBhZGQgYSB3b3JrZXIgaW4gdGlueWRybSB0byBkbyB0aGF0Lgo+ID5ZZXMs IHBsYW5lIHVwZGF0ZSBzaG91bGQgYWx3YXlzIGRvIGEgZnVsbCB1cGRhdGUuIE5vdCBzdXJlIGhv dyB5b3UgZ2V0Cj4gPmF3YXkgd2l0aCBkZWxheWluZyB0aGF0IHRvIC0+ZGlydHksIG1heWJlIG1v ZGVzZXR0aW5nIGlzbid0Cj4gPmRvdWJsZS1idWZmZXJpbmcgd2hlbiB5b3UgZG9uJ3QgaGF2ZSBh IEdMIHRoYXQgY291bGQgZG8gZ2xhbW91ci4KPiA+Cj4gPi0+ZGlydHkgaXMgX29ubHlfIGZvciBm cm9udGJ1ZmZlciByZW5kZXJpbmcsIG5vdCBmb3IgcGFnZS1mbGlwcGluZyB0byBhbgo+ID5lbnRp cmVseSBuZXcgYnVmZmVyLiBJbiBzaG9ydCBpZiBzb21lb25lIGNhbGxzIC0+ZGlydHkgb24gYSBi dWZmZXIgd2hpY2gKPiA+aXMgY3VycmVudGx5IG5vdCBiZWluZyBkaXNwbGF5ZWQgdGhhbiBhKSB0 aGV5J3JlIHNpbGx5IGIpIGRyaXZlcnMgc2hvdWxkCj4gPnRyZWF0IGl0IGFzIGEgbm8tb3AuIE1h eWJlIHdlIG5lZWQgYSBoZWxwZXIgdG8gZG8gdGhhdCAuLi4KPiA+LURhbmllbAo+IAo+IGRybV9m Yl9oZWxwZXIgd2lsbCBjYWxsIGRpcnR5KCkgYXMgbG9uZyBhcyB0aGVyZSdzIGZiZGV2IGFjdGl2 aXR5LCBzbyB0aGUKPiBkcml2ZXIgbmVlZHMgdG8gdGFrZSB0aGF0IGludG8gYWNjb3VudC4gRm9y IGluc3RhbmNlIGZiY29uIHdpdGggYSBibGlua2luZwo+IGN1cnNvciB3aWxsIHRyaWdnZXIgY2Fs bHMgZXZlbiBpZiBhIGJ1ZmZlciBoYXMgYmVlbiBzZXQgdXAgb24gdGhlIGRybSBzaWRlLgo+IHRp bnlkcm0gY2hlY2tzIHRoZSBmYiBhZ2FpbnN0IHRoZSBmYiBzZXQgb24gdGhlIHBsYW5lIGFuZCBp ZiBpdCBkaWZmZXJzCj4gaXQncyBhIG5vLW9wLgoKV2FzIHJlYWxseSBqdXN0IGFuIGlkZWEgdG8g bWFrZSBkcml2ZXJzIGEgYml0IHNpbXBsZXIsIHNpbmNlIHByZXR0eSBtdWNoCmFsbCBvZiB0aGVt IHdlIG5lZWQgdG8gZG8gdGhpcyBjaGVjay4gQnV0IHdpdGggYSBncmFuZCB0b3RhbCBvZiBqdXN0 IDMgKDQKd2l0aCB0aW55ZHJtKSBpbXBsZW1lbnRpbmcgYSBub24tdHJpdmlhbCBkaXJ0eSBjYWxs YmFjayB0aGF0J3Mgbm90IHJlYWxseQp3b3J0aCBpdCBJIHRoaW5rLgotRGFuaWVsCi0tIApEYW5p ZWwgVmV0dGVyClNvZnR3YXJlIEVuZ2luZWVyLCBJbnRlbCBDb3Jwb3JhdGlvbgpodHRwOi8vYmxv Zy5mZndsbC5jaApfX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19f XwpkcmktZGV2ZWwgbWFpbGluZyBsaXN0CmRyaS1kZXZlbEBsaXN0cy5mcmVlZGVza3RvcC5vcmcK aHR0cHM6Ly9saXN0cy5mcmVlZGVza3RvcC5vcmcvbWFpbG1hbi9saXN0aW5mby9kcmktZGV2ZWwK From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1753897AbcD0LO1 (ORCPT ); Wed, 27 Apr 2016 07:14:27 -0400 Received: from mail-wm0-f66.google.com ([74.125.82.66]:36349 "EHLO mail-wm0-f66.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1753857AbcD0LOY (ORCPT ); Wed, 27 Apr 2016 07:14:24 -0400 Date: Wed, 27 Apr 2016 13:14:20 +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 v2 4/8] drm/fb-helper: Add fb_deferred_io support Message-ID: <20160427111420.GA2558@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: <1461530942-22485-1-git-send-email-noralf@tronnes.org> <1461530942-22485-5-git-send-email-noralf@tronnes.org> <20160425090957.GQ2510@phenom.ffwll.local> <571F9656.1090506@tronnes.org> <20160426171902.GA2558@phenom.ffwll.local> <57208A3B.4020909@tronnes.org> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <57208A3B.4020909@tronnes.org> X-Operating-System: Linux phenom 4.6.0-rc5+ 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 Wed, Apr 27, 2016 at 11:45:31AM +0200, Noralf Trønnes wrote: > > Den 26.04.2016 19:19, skrev Daniel Vetter: > >On Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Trønnes wrote: > >>Den 25.04.2016 11:09, skrev Daniel Vetter: > >>>On Sun, Apr 24, 2016 at 10:48:58PM +0200, Noralf Trønnes wrote: > >>>>This adds deferred io support if CONFIG_FB_DEFERRED_IO is enabled. > >>>>The fbdev framebuffer changes are flushed using the callback > >>>>(struct drm_framebuffer *)->funcs->dirty() by a dedicated worker > >>>>ensuring that it always runs in process context. > >>>> > >>>>Signed-off-by: Noralf Trønnes > >>>>--- > >>>> > >>>>Changes since v1: > >>>>- Use a dedicated worker to run the framebuffer flushing like qxl does > >>>>- Add parameter descriptions to drm_fb_helper_deferred_io > >>>> > >>>> drivers/gpu/drm/drm_fb_helper.c | 127 +++++++++++++++++++++++++++++++++++++++- > >>>> include/drm/drm_fb_helper.h | 17 ++++++ > >>>> 2 files changed, 143 insertions(+), 1 deletion(-) > >>>> > >>>>diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c > >>>>index 855108e..46ee6f8 100644 > >>>>--- a/drivers/gpu/drm/drm_fb_helper.c > >>>>+++ b/drivers/gpu/drm/drm_fb_helper.c > >>>>@@ -40,6 +40,7 @@ > >>>> #include > >>>> #include > >>>> #include > >>>>+#include > >>>> > >>>> static bool drm_fbdev_emulation = true; > >>>> module_param_named(fbdev_emulation, drm_fbdev_emulation, bool, 0600); > >>>>@@ -48,6 +49,10 @@ MODULE_PARM_DESC(fbdev_emulation, > >>>> > >>>> static LIST_HEAD(kernel_fb_helper_list); > >>>> > >>>>+static void drm_fb_helper_dirty_init(struct drm_fb_helper *helper); > >>>>+static void drm_fb_helper_dirty(struct fb_info *info, u32 x, u32 y, > >>>>+ u32 width, u32 height); > >>>>+ > >>>> /** > >>>> * DOC: fbdev helpers > >>>> * > >>>>@@ -84,6 +89,16 @@ static LIST_HEAD(kernel_fb_helper_list); > >>>> * and set up an initial configuration using the detected hardware, drivers > >>>> * should call drm_fb_helper_single_add_all_connectors() followed by > >>>> * drm_fb_helper_initial_config(). > >>>>+ * > >>>>+ * If CONFIG_FB_DEFERRED_IO is enabled and > >>>>+ * (struct drm_framebuffer *)->funcs->dirty is set, the > >>>>+ * drm_fb_helper_{cfb,sys}_{write,fillrect,copyarea,imageblit} functions > >>>>+ * will accumulate changes and schedule (struct fb_helper).dirty_work to run > >>>>+ * right away. This worker then calls the dirty() function ensuring that it > >>>>+ * will always run in process context since the fb_*() function could be > >>>>+ * running in atomic context. If drm_fb_helper_deferred_io() is used as the > >>>>+ * deferred_io callback it will also schedule dirty_work with the damage > >>>>+ * collected from the mmap page writes. > >>>One thing to consider (and personally I don't care either way) is whether > >>>we shouldn't just select CONFIG_FB_DEFERRED_IO if the fbdev helpers are > >>>enabled. Pushing that out to drivers is imo a bit fragile. > >>> > >>>But like I said I'm ok with either way. > >>My concern was adding code and data that only a few drivers would > >>actually use. But of course there's the tradeoff with complexity. > >>I use this to enable it: > >> select FB_DEFERRED_IO if DRM_KMS_FB_HELPER > >> > >>I guess the maintainer has to make this choice between size and complexity > >>:-) > >>I can enable it by default if you want, drm is both huge and complex so I > >>don't know what's best. > >> > >>As a sidenote, I have also put all the fbdev code in a file of it's own to > >>make it simple with regards to the DRM_FBDEV_EMULATION user option: > >>tinydrm-$(CONFIG_DRM_KMS_FB_HELPER) += tinydrm-fbdev.o > >Ok, if you ask maintainers then please nuke the #ifdef from .c files. If > >you select CONFIG_DRM_KMS_FB_HELPER, then you get hdmi, edid, dp aux, dp > >mst and whatever else helpers, even if you don't need them. Adding 3 > >functions for defio when you select fbdev helpers and maybe don't need > >them is totally harmless. And removing the #ifdef will look so much better > >;-) > > Will do :-) > Kernel development is just my hobby so I'm not well versed in all of this. You're doing great tbh! > >>>> */ > >>>> > >>>> /** > >>>>@@ -401,11 +416,14 @@ backoff: > >>>> static int restore_fbdev_mode(struct drm_fb_helper *fb_helper) > >>>> { > >>>> struct drm_device *dev = fb_helper->dev; > >>>>+ struct fb_info *info = fb_helper->fbdev; > >>>> struct drm_plane *plane; > >>>> int i; > >>>> > >>>> drm_warn_on_modeset_not_all_locked(dev); > >>>> > >>>>+ drm_fb_helper_dirty(info, 0, 0, info->var.xres, info->var.yres); > >>>Why is this needed? If you do a modeset (or pageflip or whatever) drivers > >>>are supposed to re-upload the entire screen. We've talked about adding a > >>>dirty rectangle to atomic to allow userspace to optimize this, but there > >>>should _never_ be a need to do a dirtyfb call around a modeset. Probably > >>>just a driver bug in your panel drm drivers? > >>Ok, in tinydrm I now set a flag in &drm_simple_display_pipe_funcs > >>->plane_update to indicate that the next dirty() should do the whole > >>framebuffer which seems to work fine. > >>Should I actually perform the update as well? > >>If so I would need to add a worker in tinydrm to do that. > >Yes, plane update should always do a full update. Not sure how you get > >away with delaying that to ->dirty, maybe modesetting isn't > >double-buffering when you don't have a GL that could do glamour. > > > >->dirty is _only_ for frontbuffer rendering, not for page-flipping to an > >entirely new buffer. In short if someone calls ->dirty on a buffer which > >is currently not being displayed than a) they're silly b) drivers should > >treat it as a no-op. Maybe we need a helper to do that ... > >-Daniel > > drm_fb_helper will call dirty() as long as there's fbdev activity, so the > driver needs to take that into account. For instance fbcon with a blinking > cursor will trigger calls even if a buffer has been set up on the drm side. > tinydrm checks the fb against the fb set on the plane and if it differs > it's a no-op. Was really just an idea to make drivers a bit simpler, since pretty much all of them we need to do this check. But with a grand total of just 3 (4 with tinydrm) implementing a non-trivial dirty callback that's not really worth it I think. -Daniel -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch