From mboxrd@z Thu Jan 1 00:00:00 1970 From: Daniel Vetter Date: Tue, 26 Apr 2016 17:19:02 +0000 Subject: Re: [PATCH v2 4/8] drm/fb-helper: Add fb_deferred_io support Message-Id: <20160426171902.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> In-Reply-To: <571F9656.1090506@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 Tue, Apr 26, 2016 at 06:24:54PM +0200, Noralf Tr=F8nnes wrote: >=20 > 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_h= elper.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, d= rivers > >> * 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} functio= ns > >>+ * will accumulate changes and schedule (struct fb_helper).dirty_work = to run > >>+ * right away. This worker then calls the dirty() function ensuring th= at 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 a= s the > >>+ * deferred_io callback it will also schedule dirty_work with the dama= ge > >>+ * 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. >=20 > 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 >=20 > 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. >=20 > 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 better ;-) >=20 > >> */ > >> > >> /** > >>@@ -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) 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? >=20 > 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 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: Tue, 26 Apr 2016 19:19:02 +0200 Message-ID: <20160426171902.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> Mime-Version: 1.0 Content-Type: text/plain; charset="utf-8" Content-Transfer-Encoding: base64 Return-path: Received: from mail-wm0-x241.google.com (mail-wm0-x241.google.com [IPv6:2a00:1450:400c:c09::241]) by gabe.freedesktop.org (Postfix) with ESMTPS id 6E7476E297 for ; Tue, 26 Apr 2016 17:19:19 +0000 (UTC) Received: by mail-wm0-x241.google.com with SMTP id n3so6952683wmn.1 for ; Tue, 26 Apr 2016 10:19:19 -0700 (PDT) Content-Disposition: inline In-Reply-To: <571F9656.1090506@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 T24gVHVlLCBBcHIgMjYsIDIwMTYgYXQgMDY6MjQ6NTRQTSArMDIwMCwgTm9yYWxmIFRyw7hubmVz IHdyb3RlOgo+IAo+IERlbiAyNS4wNC4yMDE2IDExOjA5LCBza3JldiBEYW5pZWwgVmV0dGVyOgo+ ID5PbiBTdW4sIEFwciAyNCwgMjAxNiBhdCAxMDo0ODo1OFBNICswMjAwLCBOb3JhbGYgVHLDuG5u ZXMgd3JvdGU6Cj4gPj5UaGlzIGFkZHMgZGVmZXJyZWQgaW8gc3VwcG9ydCBpZiBDT05GSUdfRkJf REVGRVJSRURfSU8gaXMgZW5hYmxlZC4KPiA+PlRoZSBmYmRldiBmcmFtZWJ1ZmZlciBjaGFuZ2Vz IGFyZSBmbHVzaGVkIHVzaW5nIHRoZSBjYWxsYmFjawo+ID4+KHN0cnVjdCBkcm1fZnJhbWVidWZm ZXIgKiktPmZ1bmNzLT5kaXJ0eSgpIGJ5IGEgZGVkaWNhdGVkIHdvcmtlcgo+ID4+ZW5zdXJpbmcg dGhhdCBpdCBhbHdheXMgcnVucyBpbiBwcm9jZXNzIGNvbnRleHQuCj4gPj4KPiA+PlNpZ25lZC1v ZmYtYnk6IE5vcmFsZiBUcsO4bm5lcyA8bm9yYWxmQHRyb25uZXMub3JnPgo+ID4+LS0tCj4gPj4K PiA+PkNoYW5nZXMgc2luY2UgdjE6Cj4gPj4tIFVzZSBhIGRlZGljYXRlZCB3b3JrZXIgdG8gcnVu IHRoZSBmcmFtZWJ1ZmZlciBmbHVzaGluZyBsaWtlIHF4bCBkb2VzCj4gPj4tIEFkZCBwYXJhbWV0 ZXIgZGVzY3JpcHRpb25zIHRvIGRybV9mYl9oZWxwZXJfZGVmZXJyZWRfaW8KPiA+Pgo+ID4+ICBk cml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jIHwgMTI3ICsrKysrKysrKysrKysrKysrKysr KysrKysrKysrKysrKysrKysrKy0KPiA+PiAgaW5jbHVkZS9kcm0vZHJtX2ZiX2hlbHBlci5oICAg ICB8ICAxNyArKysrKysKPiA+PiAgMiBmaWxlcyBjaGFuZ2VkLCAxNDMgaW5zZXJ0aW9ucygrKSwg MSBkZWxldGlvbigtKQo+ID4+Cj4gPj5kaWZmIC0tZ2l0IGEvZHJpdmVycy9ncHUvZHJtL2RybV9m Yl9oZWxwZXIuYyBiL2RyaXZlcnMvZ3B1L2RybS9kcm1fZmJfaGVscGVyLmMKPiA+PmluZGV4IDg1 NTEwOGUuLjQ2ZWU2ZjggMTAwNjQ0Cj4gPj4tLS0gYS9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hl bHBlci5jCj4gPj4rKysgYi9kcml2ZXJzL2dwdS9kcm0vZHJtX2ZiX2hlbHBlci5jCj4gPj5AQCAt NDAsNiArNDAsNyBAQAo+ID4+ICAjaW5jbHVkZSA8ZHJtL2RybV9jcnRjX2hlbHBlci5oPgo+ID4+ ICAjaW5jbHVkZSA8ZHJtL2RybV9hdG9taWMuaD4KPiA+PiAgI2luY2x1ZGUgPGRybS9kcm1fYXRv bWljX2hlbHBlci5oPgo+ID4+KyNpbmNsdWRlIDxkcm0vZHJtX3JlY3QuaD4KPiA+Pgo+ID4+ICBz dGF0aWMgYm9vbCBkcm1fZmJkZXZfZW11bGF0aW9uID0gdHJ1ZTsKPiA+PiAgbW9kdWxlX3BhcmFt X25hbWVkKGZiZGV2X2VtdWxhdGlvbiwgZHJtX2ZiZGV2X2VtdWxhdGlvbiwgYm9vbCwgMDYwMCk7 Cj4gPj5AQCAtNDgsNiArNDksMTAgQEAgTU9EVUxFX1BBUk1fREVTQyhmYmRldl9lbXVsYXRpb24s Cj4gPj4KPiA+PiAgc3RhdGljIExJU1RfSEVBRChrZXJuZWxfZmJfaGVscGVyX2xpc3QpOwo+ID4+ Cj4gPj4rc3RhdGljIHZvaWQgZHJtX2ZiX2hlbHBlcl9kaXJ0eV9pbml0KHN0cnVjdCBkcm1fZmJf aGVscGVyICpoZWxwZXIpOwo+ID4+K3N0YXRpYyB2b2lkIGRybV9mYl9oZWxwZXJfZGlydHkoc3Ry dWN0IGZiX2luZm8gKmluZm8sIHUzMiB4LCB1MzIgeSwKPiA+PisJCQkJdTMyIHdpZHRoLCB1MzIg aGVpZ2h0KTsKPiA+PisKPiA+PiAgLyoqCj4gPj4gICAqIERPQzogZmJkZXYgaGVscGVycwo+ID4+ ICAgKgo+ID4+QEAgLTg0LDYgKzg5LDE2IEBAIHN0YXRpYyBMSVNUX0hFQUQoa2VybmVsX2ZiX2hl bHBlcl9saXN0KTsKPiA+PiAgICogYW5kIHNldCB1cCBhbiBpbml0aWFsIGNvbmZpZ3VyYXRpb24g dXNpbmcgdGhlIGRldGVjdGVkIGhhcmR3YXJlLCBkcml2ZXJzCj4gPj4gICAqIHNob3VsZCBjYWxs IGRybV9mYl9oZWxwZXJfc2luZ2xlX2FkZF9hbGxfY29ubmVjdG9ycygpIGZvbGxvd2VkIGJ5Cj4g Pj4gICAqIGRybV9mYl9oZWxwZXJfaW5pdGlhbF9jb25maWcoKS4KPiA+PisgKgo+ID4+KyAqIElm IENPTkZJR19GQl9ERUZFUlJFRF9JTyBpcyBlbmFibGVkIGFuZAo+ID4+KyAqIChzdHJ1Y3QgZHJt X2ZyYW1lYnVmZmVyICopLT5mdW5jcy0+ZGlydHkgaXMgc2V0LCB0aGUKPiA+PisgKiBkcm1fZmJf aGVscGVyX3tjZmIsc3lzfV97d3JpdGUsZmlsbHJlY3QsY29weWFyZWEsaW1hZ2VibGl0fSBmdW5j dGlvbnMKPiA+PisgKiB3aWxsIGFjY3VtdWxhdGUgY2hhbmdlcyBhbmQgc2NoZWR1bGUgKHN0cnVj dCBmYl9oZWxwZXIpLmRpcnR5X3dvcmsgdG8gcnVuCj4gPj4rICogcmlnaHQgYXdheS4gVGhpcyB3 b3JrZXIgdGhlbiBjYWxscyB0aGUgZGlydHkoKSBmdW5jdGlvbiBlbnN1cmluZyB0aGF0IGl0Cj4g Pj4rICogd2lsbCBhbHdheXMgcnVuIGluIHByb2Nlc3MgY29udGV4dCBzaW5jZSB0aGUgZmJfKigp IGZ1bmN0aW9uIGNvdWxkIGJlCj4gPj4rICogcnVubmluZyBpbiBhdG9taWMgY29udGV4dC4gSWYg ZHJtX2ZiX2hlbHBlcl9kZWZlcnJlZF9pbygpIGlzIHVzZWQgYXMgdGhlCj4gPj4rICogZGVmZXJy ZWRfaW8gY2FsbGJhY2sgaXQgd2lsbCBhbHNvIHNjaGVkdWxlIGRpcnR5X3dvcmsgd2l0aCB0aGUg ZGFtYWdlCj4gPj4rICogY29sbGVjdGVkIGZyb20gdGhlIG1tYXAgcGFnZSB3cml0ZXMuCj4gPk9u ZSB0aGluZyB0byBjb25zaWRlciAoYW5kIHBlcnNvbmFsbHkgSSBkb24ndCBjYXJlIGVpdGhlciB3 YXkpIGlzIHdoZXRoZXIKPiA+d2Ugc2hvdWxkbid0IGp1c3Qgc2VsZWN0IENPTkZJR19GQl9ERUZF UlJFRF9JTyBpZiB0aGUgZmJkZXYgaGVscGVycyBhcmUKPiA+ZW5hYmxlZC4gUHVzaGluZyB0aGF0 IG91dCB0byBkcml2ZXJzIGlzIGltbyBhIGJpdCBmcmFnaWxlLgo+ID4KPiA+QnV0IGxpa2UgSSBz YWlkIEknbSBvayB3aXRoIGVpdGhlciB3YXkuCj4gCj4gTXkgY29uY2VybiB3YXMgYWRkaW5nIGNv ZGUgYW5kIGRhdGEgdGhhdCBvbmx5IGEgZmV3IGRyaXZlcnMgd291bGQKPiBhY3R1YWxseSB1c2Uu IEJ1dCBvZiBjb3Vyc2UgdGhlcmUncyB0aGUgdHJhZGVvZmYgd2l0aCBjb21wbGV4aXR5Lgo+IEkg dXNlIHRoaXMgdG8gZW5hYmxlIGl0Ogo+ICAgICAgICAgc2VsZWN0IEZCX0RFRkVSUkVEX0lPIGlm IERSTV9LTVNfRkJfSEVMUEVSCj4gCj4gSSBndWVzcyB0aGUgbWFpbnRhaW5lciBoYXMgdG8gbWFr ZSB0aGlzIGNob2ljZSBiZXR3ZWVuIHNpemUgYW5kIGNvbXBsZXhpdHkKPiA6LSkKPiBJIGNhbiBl bmFibGUgaXQgYnkgZGVmYXVsdCBpZiB5b3Ugd2FudCwgZHJtIGlzIGJvdGggaHVnZSBhbmQgY29t cGxleCBzbyBJCj4gZG9uJ3Qga25vdyB3aGF0J3MgYmVzdC4KPiAKPiBBcyBhIHNpZGVub3RlLCBJ IGhhdmUgYWxzbyBwdXQgYWxsIHRoZSBmYmRldiBjb2RlIGluIGEgZmlsZSBvZiBpdCdzIG93biB0 bwo+IG1ha2UgaXQgc2ltcGxlIHdpdGggcmVnYXJkcyB0byB0aGUgRFJNX0ZCREVWX0VNVUxBVElP TiB1c2VyIG9wdGlvbjoKPiB0aW55ZHJtLSQoQ09ORklHX0RSTV9LTVNfRkJfSEVMUEVSKSAgICAg Kz0gdGlueWRybS1mYmRldi5vCgpPaywgaWYgeW91IGFzayBtYWludGFpbmVycyB0aGVuIHBsZWFz ZSBudWtlIHRoZSAjaWZkZWYgZnJvbSAuYyBmaWxlcy4gSWYKeW91IHNlbGVjdCBDT05GSUdfRFJN X0tNU19GQl9IRUxQRVIsIHRoZW4geW91IGdldCBoZG1pLCBlZGlkLCBkcCBhdXgsIGRwCm1zdCBh bmQgd2hhdGV2ZXIgZWxzZSBoZWxwZXJzLCBldmVuIGlmIHlvdSBkb24ndCBuZWVkIHRoZW0uIEFk ZGluZyAzCmZ1bmN0aW9ucyBmb3IgZGVmaW8gd2hlbiB5b3Ugc2VsZWN0IGZiZGV2IGhlbHBlcnMg YW5kIG1heWJlIGRvbid0IG5lZWQKdGhlbSBpcyB0b3RhbGx5IGhhcm1sZXNzLiBBbmQgcmVtb3Zp bmcgdGhlICNpZmRlZiB3aWxsIGxvb2sgc28gbXVjaCBiZXR0ZXIKOy0pCgo+IAo+ID4+ICAgKi8K PiA+Pgo+ID4+ICAvKioKPiA+PkBAIC00MDEsMTEgKzQxNiwxNCBAQCBiYWNrb2ZmOgo+ID4+ICBz dGF0aWMgaW50IHJlc3RvcmVfZmJkZXZfbW9kZShzdHJ1Y3QgZHJtX2ZiX2hlbHBlciAqZmJfaGVs cGVyKQo+ID4+ICB7Cj4gPj4gIAlzdHJ1Y3QgZHJtX2RldmljZSAqZGV2ID0gZmJfaGVscGVyLT5k ZXY7Cj4gPj4rCXN0cnVjdCBmYl9pbmZvICppbmZvID0gZmJfaGVscGVyLT5mYmRldjsKPiA+PiAg CXN0cnVjdCBkcm1fcGxhbmUgKnBsYW5lOwo+ID4+ICAJaW50IGk7Cj4gPj4KPiA+PiAgCWRybV93 YXJuX29uX21vZGVzZXRfbm90X2FsbF9sb2NrZWQoZGV2KTsKPiA+Pgo+ID4+Kwlkcm1fZmJfaGVs cGVyX2RpcnR5KGluZm8sIDAsIDAsIGluZm8tPnZhci54cmVzLCBpbmZvLT52YXIueXJlcyk7Cj4g PldoeSBpcyB0aGlzIG5lZWRlZD8gSWYgeW91IGRvIGEgbW9kZXNldCAob3IgcGFnZWZsaXAgb3Ig d2hhdGV2ZXIpIGRyaXZlcnMKPiA+YXJlIHN1cHBvc2VkIHRvIHJlLXVwbG9hZCB0aGUgZW50aXJl IHNjcmVlbi4gV2UndmUgdGFsa2VkIGFib3V0IGFkZGluZyBhCj4gPmRpcnR5IHJlY3RhbmdsZSB0 byBhdG9taWMgdG8gYWxsb3cgdXNlcnNwYWNlIHRvIG9wdGltaXplIHRoaXMsIGJ1dCB0aGVyZQo+ ID5zaG91bGQgX25ldmVyXyBiZSBhIG5lZWQgdG8gZG8gYSBkaXJ0eWZiIGNhbGwgYXJvdW5kIGEg bW9kZXNldC4gUHJvYmFibHkKPiA+anVzdCBhIGRyaXZlciBidWcgaW4geW91ciBwYW5lbCBkcm0g ZHJpdmVycz8KPiAKPiBPaywgaW4gdGlueWRybSBJIG5vdyBzZXQgYSBmbGFnIGluICZkcm1fc2lt cGxlX2Rpc3BsYXlfcGlwZV9mdW5jcwo+IC0+cGxhbmVfdXBkYXRlIHRvIGluZGljYXRlIHRoYXQg dGhlIG5leHQgZGlydHkoKSBzaG91bGQgZG8gdGhlIHdob2xlCj4gZnJhbWVidWZmZXIgd2hpY2gg c2VlbXMgdG8gd29yayBmaW5lLgo+IFNob3VsZCBJIGFjdHVhbGx5IHBlcmZvcm0gdGhlIHVwZGF0 ZSBhcyB3ZWxsPwo+IElmIHNvIEkgd291bGQgbmVlZCB0byBhZGQgYSB3b3JrZXIgaW4gdGlueWRy bSB0byBkbyB0aGF0LgoKWWVzLCBwbGFuZSB1cGRhdGUgc2hvdWxkIGFsd2F5cyBkbyBhIGZ1bGwg dXBkYXRlLiBOb3Qgc3VyZSBob3cgeW91IGdldAphd2F5IHdpdGggZGVsYXlpbmcgdGhhdCB0byAt PmRpcnR5LCBtYXliZSBtb2Rlc2V0dGluZyBpc24ndApkb3VibGUtYnVmZmVyaW5nIHdoZW4geW91 IGRvbid0IGhhdmUgYSBHTCB0aGF0IGNvdWxkIGRvIGdsYW1vdXIuCgotPmRpcnR5IGlzIF9vbmx5 XyBmb3IgZnJvbnRidWZmZXIgcmVuZGVyaW5nLCBub3QgZm9yIHBhZ2UtZmxpcHBpbmcgdG8gYW4K ZW50aXJlbHkgbmV3IGJ1ZmZlci4gSW4gc2hvcnQgaWYgc29tZW9uZSBjYWxscyAtPmRpcnR5IG9u IGEgYnVmZmVyIHdoaWNoCmlzIGN1cnJlbnRseSBub3QgYmVpbmcgZGlzcGxheWVkIHRoYW4gYSkg dGhleSdyZSBzaWxseSBiKSBkcml2ZXJzIHNob3VsZAp0cmVhdCBpdCBhcyBhIG5vLW9wLiBNYXli ZSB3ZSBuZWVkIGEgaGVscGVyIHRvIGRvIHRoYXQgLi4uCi1EYW5pZWwKLS0gCkRhbmllbCBWZXR0 ZXIKU29mdHdhcmUgRW5naW5lZXIsIEludGVsIENvcnBvcmF0aW9uCmh0dHA6Ly9ibG9nLmZmd2xs LmNoCl9fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fX19fCmRyaS1k ZXZlbCBtYWlsaW5nIGxpc3QKZHJpLWRldmVsQGxpc3RzLmZyZWVkZXNrdG9wLm9yZwpodHRwczov L2xpc3RzLmZyZWVkZXNrdG9wLm9yZy9tYWlsbWFuL2xpc3RpbmZvL2RyaS1kZXZlbAo= From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752673AbcDZRTV (ORCPT ); Tue, 26 Apr 2016 13:19:21 -0400 Received: from mail-wm0-f65.google.com ([74.125.82.65]:34819 "EHLO mail-wm0-f65.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752246AbcDZRTT (ORCPT ); Tue, 26 Apr 2016 13:19:19 -0400 Date: Tue, 26 Apr 2016 19:19:02 +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: <20160426171902.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> MIME-Version: 1.0 Content-Type: text/plain; charset=iso-8859-1 Content-Disposition: inline Content-Transfer-Encoding: 8bit In-Reply-To: <571F9656.1090506@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 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 ;-) > > >> */ > >> > >> /** > >>@@ -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 -- Daniel Vetter Software Engineer, Intel Corporation http://blog.ffwll.ch