2012-06-16 22:04:51

by David Herrmann

[permalink] [raw]
Subject: [RFC 00/10] fblog: framebuffer kernel log driver

Hi

As some might know I am working on making CONFIG_VT obsolete. But as a developer
it is often useful to have a kernel-log on the screen during boot to debug many
common kernel(-config) errors. However, without CONFIG_VT we cannot use the
VGA/framebbufer consoles either. Therefore, I am working on a small driver
called "fblog".

This driver simply writes the kernel log to all connected framebuffers. It works
similar to fbcon but removes all the complexity of the virtual terminals. There
is a sysfs attribute called "active" that allows to enable/disable fblog so
user-space can start an xserver or similar.

The main purpose is debugging kernel boot problems. Therefore, it is not
optimized for speed and I tried keeping it simple. I splitted the patches
into 10 small chunks to make review easier.

I would be glad if someone could review this and tell me whether this is
something we could include in mainline or not.


There are still some issues but apart from them it works fine on my
machine (x86):
- I register the fblog device during module_init and need to call
module_get(). However, this means it is impossible to call "rmmod fblog" as
fblog has a reference to itself. Using "rmmod -f fblog" works fine but is a
bit ugly. Is there a nice way to fix this? Otherwise I would need to call
device_get() in module_exit() if there is a pending user of the fblog-device
even though I unregistered it.
- I redraw all framebuffers while holding the console-lock. This may slow down
machines with more than 2 framebuffers (like 10 or 20). However, as this is
supposed to be a debug driver, I think I can ignore this? If someone wants
to improve the redraw logic to avoid redrawing the whole screen all the
time, I would be glad to include it in this patchset :)
- I am really no expert regarding the framebuffer subsystem. So I would
appreciate it if someone could comment whether I need to handle the events
in a different way or whether it is ok the way it is now.

I run this on my machine with CONFIG_VT=n and it works quite nice. If someone
wants to test it, this also works with CONFIG_VT=y and x11 running. Just load
the driver from your xserver and it will redraw the screen with the console.
Switching VTs back and forth will make the xserver redraw the whole screen
again. You need to remove "depends on !VT" of course.

Thanks and regards
David

David Herrmann (10):
fblog: new framebuffer kernel log dummy driver
fblog: implement buffer management
fblog: register framebuffer objects
fblog: implement fblog_redraw()
fblog: add framebuffer helpers
fblog: allow enabling/disabling fblog on runtime
fblog: forward kernel log messages to all framebuffers
fblog: react on framebuffer events
fblog: register all handlers on module-init
fblog: add "activate" module parameter

Documentation/ABI/testing/sysfs-fblog | 9 +
drivers/video/Kconfig | 5 +-
drivers/video/Makefile | 2 +-
drivers/video/console/Kconfig | 37 +-
drivers/video/console/Makefile | 1 +
drivers/video/console/fblog.c | 694 +++++++++++++++++++++++++++++++++
6 files changed, 735 insertions(+), 13 deletions(-)
create mode 100644 Documentation/ABI/testing/sysfs-fblog
create mode 100644 drivers/video/console/fblog.c

--
1.7.10.4


2012-06-16 22:05:00

by David Herrmann

[permalink] [raw]
Subject: [PATCH 08/10] fblog: react on framebuffer events

This provides an fb-notifier object that can be registered with the
framebuffer subsystem. We are then notified about events on all
framebuffers.
Most of the events are only of interest for fbcon so we can safely ignore
them. However, we need to handle REGISTERED/UNBIND to add new framebbufers
and we need to react on mode-changes (probably triggered by user-space).

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 113 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 113 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 5297eca..79bfbcc 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -449,6 +449,119 @@ static void fblog_deactivate(void)
fblog_unregister_all();
}

+static int fblog_event(struct notifier_block *self, unsigned long action,
+ void *data)
+{
+ struct fb_event *event = data;
+ struct fb_info *info = event->info;
+ struct fblog_fb *fb = fblog_info2fb(info);
+ int *blank;
+
+ if (action == FB_EVENT_FB_REGISTERED) {
+ /* This is called when a low-level system driver registers a new
+ * framebuffer. The registration lock is held but the console
+ * lock might not be held when this is called (really?). */
+ fblog_register(info);
+ return 0;
+ }
+
+ if (!fb)
+ return 0;
+
+ switch(action) {
+ case FB_EVENT_FB_UNREGISTERED:
+ /* This is called when a low-level system driver unregisters a
+ * framebuffer. The registration lock is held but the console
+ * lock might not be held (really?). */
+ /* ignore; see UNBIND */
+ break;
+ case FB_EVENT_FB_UNBIND:
+ /* Called directly before unregistering an FB. The FB is still
+ * valid here and the registration lock is held but the console
+ * lock might not be held (really?). */
+ fblog_unregister(fb);
+ break;
+ case FB_EVENT_SUSPEND:
+ /* This is called when the low-level display driver suspends the
+ * video system. We should not access the video system while it
+ * is suspended. This is called with the console lock held. */
+ set_bit(FBLOG_SUSPENDED, &fb->flags);
+ break;
+ case FB_EVENT_RESUME:
+ /* This is called when the low-level display driver resumes
+ * operating. It is called with the console lock held. */
+ clear_bit(FBLOG_SUSPENDED, &fb->flags);
+ break;
+ case FB_EVENT_MODE_DELETE:
+ /* This is sent when a video mode is removed. The current video
+ * mode is never removed! The console lock is held while this is
+ * called. */
+ /* fallthrough */
+ case FB_EVENT_NEW_MODELIST:
+ /* This is sent when the modelist got changed. The console-lock
+ * is held and we should reset the mode. */
+ /* fallthrough */
+ case FB_EVENT_MODE_CHANGE_ALL:
+ /* This is the same as below but notifies us that the user used
+ * the FB_ACTIVATE_ALL flag when setting the video mode. */
+ /* fallthrough */
+ case FB_EVENT_MODE_CHANGE:
+ /* This is called when the _user_ changes the video mode via
+ * ioctls. It is not sent, when the kernel changes the mode
+ * internally. This callback is called inside fb_set_var() so
+ * the console lock is held. */
+ fblog_refresh(fb);
+ break;
+ case FB_EVENT_BLANK:
+ /* This gets called _after_ the framebuffer was successfully
+ * blanked. The console-lock is always held while fb_blank is
+ * called and during this callback. */
+ blank = (int*)event->data;
+ if (*blank == FB_BLANK_UNBLANK)
+ clear_bit(FBLOG_BLANKED, &fb->flags);
+ else
+ set_bit(FBLOG_BLANKED, &fb->flags);
+ break;
+ case FB_EVENT_GET_REQ:
+ /* When fb_set_var() is called, this callback is called to get
+ * our display requirements. They are then compared with the
+ * display properties and only if they fulfill the requirements,
+ * the new mode is activated. The console-lock should be held
+ * while calling fb_set_var() so we can assume it is locked
+ * here. */
+ /* ignore */
+ break;
+ case FB_EVENT_CONBLANK:
+ /* This is sent by fbcon when doing a fake blank. That
+ * is, blanking the screen when the fb driver failed to perform
+ * an fb_blank(). It simply writes empty lines to the screen.
+ * We are not interested in this signal. We should also never
+ * run together with fbcon so this should never be catched. */
+ /* ignore */
+ break;
+ case FB_EVENT_GET_CONSOLE_MAP:
+ /* fallthrough */
+ case FB_EVENT_SET_CONSOLE_MAP:
+ /* Is there any reason why we should support this? We
+ * ignore it as we consider ourself not to be the classic linux
+ * console. Hence, this request is not targeted at us. */
+ /* ignore */
+ break;
+ case FB_EVENT_REMAP_ALL_CONSOLE:
+ /* What are we supposed to do here? Do we have to remap
+ * the primary device to the framebuffer given by \info? Like
+ * above we currently ignore it for the same reasons. */
+ /* ignore */
+ break;
+ }
+
+ return 0;
+}
+
+static struct notifier_block fblog_notifier = {
+ .notifier_call = fblog_event,
+};
+
static void fblog_con_write(struct console *con, const char *buf,
unsigned int len)
{
--
1.7.10.4

2012-06-16 22:05:15

by David Herrmann

[permalink] [raw]
Subject: [PATCH 10/10] fblog: add "activate" module parameter

This new parameter controls whether fblog is automatically activated when
it is loaded. This defaults to "true".

We can now compile with CONFIG_VT=n and CONFIG_FBLOG=y and control fblog
with fblog.activate=0/1 on the kernel command line to enable/disable
debugging.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 10 ++++++++++
1 file changed, 10 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 9d3b072..cabc550 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -94,6 +94,7 @@ struct fblog_fb {
static struct fblog_fb *fblog_fbs[FB_MAX];
static struct device *fblog_device;
static atomic_t fblog_active;
+static bool activate = 1;

static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
size_t height)
@@ -653,6 +654,12 @@ static int __init fblog_init(void)

register_console(&fblog_con_driver);

+ if (activate) {
+ console_lock();
+ fblog_activate();
+ console_unlock();
+ }
+
return 0;

err_fb:
@@ -677,6 +684,9 @@ static void __exit fblog_exit(void)
put_device(fblog_device);
}

+module_param(activate, bool, S_IRUGO);
+MODULE_PARM_DESC(activate, "Activate fblog by default");
+
module_init(fblog_init);
module_exit(fblog_exit);
MODULE_LICENSE("GPL");
--
1.7.10.4

2012-06-16 22:04:57

by David Herrmann

[permalink] [raw]
Subject: [PATCH 03/10] fblog: register framebuffer objects

We register each available framebuffer in the system with the fblog driver
so we always know all active devices. We directly open the fb-driver,
initialize the buffer and load a font so we are ready for drawing
operations. If a device cannot be opened, we mark it as dead and ignore it
in all other functions.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 108 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 108 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 1504ba9..8038dcc 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -39,6 +39,14 @@
#include <linux/font.h>
#include <linux/module.h>

+#define FBLOG_STR(x) x, sizeof(x) - 1
+
+enum fblog_flags {
+ FBLOG_KILLED,
+ FBLOG_SUSPENDED,
+ FBLOG_BLANKED,
+};
+
/**
* struct fblog_buf: Console text buffer
*
@@ -61,6 +69,30 @@ struct fblog_buf {
size_t pos_y;
};

+/**
+ * struct fblog_fb: Framebuffer object
+ *
+ * For each framebuffer we register this object. It contains all data we need to
+ * display the console log on it. The index of a framebuffer in registered_fb[]
+ * is the same as in fblog_fbs[]. So the following must always be true if the
+ * pointers are non-NULL:
+ * registered_fb[idx] == fblog_fbs[idx]->info
+ * fblog_fbs[idx]->info->node == idx
+ *
+ * flags: Framebuffer flags (see fblog_flags)
+ * info: Pointer to the associated framebuffer device
+ * font: Currently used font
+ * buf: Console text buffer
+ */
+struct fblog_fb {
+ unsigned long flags;
+ struct fb_info *info;
+ const struct font_desc *font;
+ struct fblog_buf buf;
+};
+
+static struct fblog_fb *fblog_fbs[FB_MAX];
+
static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
size_t height)
{
@@ -165,6 +197,82 @@ static void fblog_buf_write(struct fblog_buf *buf, const char *str, size_t len)
}
}

+static struct fblog_fb *fblog_info2fb(struct fb_info *info)
+{
+ if (!info || info->node < 0 || info->node >= FB_MAX ||
+ !registered_fb[info->node])
+ return NULL;
+
+ return fblog_fbs[info->node];
+}
+
+static void fblog_register(struct fb_info *info)
+{
+ struct fblog_fb *fb;
+ struct fb_var_screeninfo var;
+ const struct fb_videomode *mode;
+ unsigned int width, height;
+
+ if (!info || info->node < 0 || info->node >= FB_MAX)
+ return;
+ if (!registered_fb[info->node] || fblog_fbs[info->node])
+ return;
+
+ fb = kzalloc(sizeof(*fb), GFP_KERNEL);
+ if (!fb)
+ return;
+
+ fblog_fbs[info->node] = fb;
+ fb->info = info;
+ fblog_buf_init(&fb->buf);
+ fblog_buf_write(&fb->buf, FBLOG_STR("Framebuffer log initialized\n"));
+
+ if (!try_module_get(info->fbops->owner))
+ goto out_killed;
+ if (info->fbops->fb_open && info->fbops->fb_open(info, 0))
+ goto out_unref;
+
+ var = info->var;
+ mode = fb_find_best_mode(&var, &info->modelist);
+ var.activate = FB_ACTIVATE_NOW | FB_ACTIVATE_FORCE;
+ fb_set_var(info, &var);
+
+ fb->font = get_default_font(info->var.xres, info->var.yres,
+ info->pixmap.blit_x,
+ info->pixmap.blit_y);
+ if (fb->font) {
+ width = info->var.xres / fb->font->width;
+ height = info->var.yres / fb->font->height;
+ fblog_buf_resize(&fb->buf, width, height);
+ }
+
+ return;
+
+out_unref:
+ module_put(info->fbops->owner);
+out_killed:
+ set_bit(FBLOG_KILLED, &fb->flags);
+}
+
+static void fblog_unregister(struct fblog_fb *fb)
+{
+ struct fb_info *info;
+
+ if (!fb)
+ return;
+
+ info = fb->info;
+ if (!test_bit(FBLOG_KILLED, &fb->flags)) {
+ if (info->fbops->fb_release)
+ info->fbops->fb_release(info, 0);
+ module_put(info->fbops->owner);
+ }
+
+ fblog_buf_deinit(&fb->buf);
+ fblog_fbs[info->node] = NULL;
+ kfree(fb);
+}
+
static int __init fblog_init(void)
{
return 0;
--
1.7.10.4

2012-06-16 22:05:46

by David Herrmann

[permalink] [raw]
Subject: [PATCH 09/10] fblog: register all handlers on module-init

We now create a new "fblog" device when initializing the fblog module. We
register the "active" sysfs-file with it so user-space can now access
fblog. We also register the framebuffer-notifier and console-handler so
fblog is ready to go.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 59 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 59 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 79bfbcc..9d3b072 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -92,6 +92,7 @@ struct fblog_fb {
};

static struct fblog_fb *fblog_fbs[FB_MAX];
+static struct device *fblog_device;
static atomic_t fblog_active;

static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
@@ -609,13 +610,71 @@ static ssize_t fblog_dev_active_store(struct device *dev,
static DEVICE_ATTR(active, S_IRUGO | S_IWUSR | S_IWGRP, fblog_dev_active_show,
fblog_dev_active_store);

+static void fblog_dev_release(struct device *dev)
+{
+ kfree(dev);
+ module_put(THIS_MODULE);
+}
+
static int __init fblog_init(void)
{
+ int ret;
+
+ fblog_device = kzalloc(sizeof(*fblog_device), GFP_KERNEL);
+ if (!fblog_device) {
+ pr_err("fblog: cannot allocate device\n");
+ ret = -ENOMEM;
+ goto err_out;
+ }
+
+ __module_get(THIS_MODULE);
+ device_initialize(fblog_device);
+ fblog_device->class = fb_class;
+ fblog_device->release = fblog_dev_release;
+ dev_set_name(fblog_device, "fblog");
+
+ ret = device_add(fblog_device);
+ if (ret) {
+ pr_err("fblog: cannot add device\n");
+ goto err_dev;
+ }
+
+ ret = fb_register_client(&fblog_notifier);
+ if (ret) {
+ pr_err("fblog: cannot register framebuffer notifier\n");
+ goto err_dev_rm;
+ }
+
+ ret = device_create_file(fblog_device, &dev_attr_active);
+ if (ret) {
+ pr_err("fblog: cannot create sysfs entry\n");
+ goto err_fb;
+ }
+
+ register_console(&fblog_con_driver);
+
return 0;
+
+err_fb:
+ fb_unregister_client(&fblog_notifier);
+err_dev_rm:
+ device_del(fblog_device);
+err_dev:
+ put_device(fblog_device);
+err_out:
+ return ret;
}

static void __exit fblog_exit(void)
{
+ unregister_console(&fblog_con_driver);
+ device_remove_file(fblog_device, &dev_attr_active);
+ device_del(fblog_device);
+ fb_unregister_client(&fblog_notifier);
+ console_lock();
+ fblog_deactivate();
+ console_unlock();
+ put_device(fblog_device);
}

module_init(fblog_init);
--
1.7.10.4

2012-06-16 22:06:09

by David Herrmann

[permalink] [raw]
Subject: [PATCH 07/10] fblog: forward kernel log messages to all framebuffers

This provides a console-driver that forwards all log messages to all
framebuffers and redraws them.

To avoid redrawing multiple times in short intervals, we could use a
work-queue here by simply pushing the task onto the system work-queue.
However, fblog is not performance critical and only used for debugging so
we avoid the complexity for now. This may change in the future, though.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 19 +++++++++++++++++++
1 file changed, 19 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 9b05c56..5297eca 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -449,6 +449,25 @@ static void fblog_deactivate(void)
fblog_unregister_all();
}

+static void fblog_con_write(struct console *con, const char *buf,
+ unsigned int len)
+{
+ int i;
+
+ for (i = 0; i < FB_MAX; ++i) {
+ if (fblog_fbs[i]) {
+ fblog_buf_write(&fblog_fbs[i]->buf, buf, len);
+ fblog_redraw(fblog_fbs[i]);
+ }
+ }
+}
+
+static struct console fblog_con_driver = {
+ .name = "fblog",
+ .write = fblog_con_write,
+ .flags = CON_PRINTBUFFER | CON_ENABLED,
+};
+
static ssize_t fblog_dev_active_show(struct device *dev,
struct device_attribute *attr,
char *buf)
--
1.7.10.4

2012-06-16 22:06:31

by David Herrmann

[permalink] [raw]
Subject: [PATCH 05/10] fblog: add framebuffer helpers

These helpers scan the system for all available framebuffers and register
or unregister them. This is needed during startup and stopping fblog so we
are aware of all connected displays.

The third helper handles mode changes by rescanning the mode and adjusting
the buffer size.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 29 +++++++++++++++++++++++++++++
1 file changed, 29 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index e790971..7d4032e 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -399,6 +399,35 @@ static void fblog_unregister(struct fblog_fb *fb)
kfree(fb);
}

+static void fblog_register_all(void)
+{
+ int i;
+
+ for (i = 0; i < FB_MAX; ++i)
+ fblog_register(registered_fb[i]);
+}
+
+static void fblog_unregister_all(void)
+{
+ int i;
+
+ for (i = 0; i < FB_MAX; ++i)
+ fblog_unregister(fblog_info2fb(registered_fb[i]));
+}
+
+static void fblog_refresh(struct fblog_fb *fb)
+{
+ unsigned int width, height;
+
+ if (!fb || !fb->font)
+ return;
+
+ width = fb->info->var.xres / fb->font->width;
+ height = fb->info->var.yres / fb->font->height;
+ fblog_buf_resize(&fb->buf, width, height);
+ fblog_redraw(fb);
+}
+
static int __init fblog_init(void)
{
return 0;
--
1.7.10.4

2012-06-16 22:06:30

by David Herrmann

[permalink] [raw]
Subject: [PATCH 06/10] fblog: allow enabling/disabling fblog on runtime

A sysfs file called "active" can be used to enable and disable fblog on
runtime. For example, the init-process can run "echo '0' >.../active"
after booting the system. This will allow other applications like X11 to
use the graphics subsystem. During shutdown, we can write '1' to get
system messages again.

When disabling fblog, we remove all framebuffers and will prevent new
hotplugged framebuffers from being added. When enabling fblog again, we
rescan the system for all framebuffers and resume operating.

The sysfs file is not registered, yet, as we do not have a "struct device"
yet. This will follow shortly, though.

Signed-off-by: David Herrmann <[email protected]>
---
Documentation/ABI/testing/sysfs-fblog | 9 ++++++
drivers/video/console/fblog.c | 49 +++++++++++++++++++++++++++++++++
2 files changed, 58 insertions(+)
create mode 100644 Documentation/ABI/testing/sysfs-fblog

diff --git a/Documentation/ABI/testing/sysfs-fblog b/Documentation/ABI/testing/sysfs-fblog
new file mode 100644
index 0000000..596393c
--- /dev/null
+++ b/Documentation/ABI/testing/sysfs-fblog
@@ -0,0 +1,9 @@
+What: /sys/class/graphics/fblog/active
+Date: June 2012
+KernelVersion: 3.6
+Contact: David Herrmann <[email protected]>
+Description: Enable/Disable fblog. When setting this to 0, fblog will stop
+ writing to framebuffers and other applications can use the
+ graphics subsystem. When setting this to 1, fblog will rescan
+ the system for all framebuffers and resume drawing the kernel
+ log onto all framebuffers.
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 7d4032e..9b05c56 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -92,6 +92,7 @@ struct fblog_fb {
};

static struct fblog_fb *fblog_fbs[FB_MAX];
+static atomic_t fblog_active;

static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
size_t height)
@@ -338,6 +339,8 @@ static void fblog_register(struct fb_info *info)
const struct fb_videomode *mode;
unsigned int width, height;

+ if (!atomic_read(&fblog_active))
+ return;
if (!info || info->node < 0 || info->node >= FB_MAX)
return;
if (!registered_fb[info->node] || fblog_fbs[info->node])
@@ -428,6 +431,52 @@ static void fblog_refresh(struct fblog_fb *fb)
fblog_redraw(fb);
}

+static void fblog_activate(void)
+{
+ if (atomic_read(&fblog_active))
+ return;
+
+ atomic_set(&fblog_active, 1);
+ fblog_register_all();
+}
+
+static void fblog_deactivate(void)
+{
+ if (!atomic_read(&fblog_active))
+ return;
+
+ atomic_set(&fblog_active, 0);
+ fblog_unregister_all();
+}
+
+static ssize_t fblog_dev_active_show(struct device *dev,
+ struct device_attribute *attr,
+ char *buf)
+{
+ return snprintf(buf, PAGE_SIZE, "%d\n", atomic_read(&fblog_active));
+}
+
+static ssize_t fblog_dev_active_store(struct device *dev,
+ struct device_attribute *attr,
+ const char *buf,
+ size_t count)
+{
+ unsigned long num;
+
+ num = simple_strtoul(buf, NULL, 10);
+ console_lock();
+ if (num)
+ fblog_activate();
+ else
+ fblog_deactivate();
+ console_unlock();
+
+ return count;
+}
+
+static DEVICE_ATTR(active, S_IRUGO | S_IWUSR | S_IWGRP, fblog_dev_active_show,
+ fblog_dev_active_store);
+
static int __init fblog_init(void)
{
return 0;
--
1.7.10.4

2012-06-16 22:04:55

by David Herrmann

[permalink] [raw]
Subject: [PATCH 01/10] fblog: new framebuffer kernel log dummy driver

Fblog displays all kernel log messages on all connected framebuffers. It
replaces fbcon when CONFIG_VT=n is selected. Its main purpose is to debug
boot problems by displaying the whole boot log on the screen. This patch
provides the first dummy module-init/deinit functions.

As is uses all the font and fb functions I placed it in
drivers/video/console. However, this means that we need to move the check
for CONFIG_VT in Makefile/Kconfig from drivers/video into
drivers/video/console as fblog does not depend on CONFIG_VT.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/Kconfig | 5 +---
drivers/video/Makefile | 2 +-
drivers/video/console/Kconfig | 37 +++++++++++++++++++++------
drivers/video/console/Makefile | 1 +
drivers/video/console/fblog.c | 55 ++++++++++++++++++++++++++++++++++++++++
5 files changed, 87 insertions(+), 13 deletions(-)
create mode 100644 drivers/video/console/fblog.c

diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0217f74..e8fd53d 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2448,10 +2448,7 @@ source "drivers/video/omap/Kconfig"
source "drivers/video/omap2/Kconfig"
source "drivers/video/exynos/Kconfig"
source "drivers/video/backlight/Kconfig"
-
-if VT
- source "drivers/video/console/Kconfig"
-endif
+source "drivers/video/console/Kconfig"

if FB || SGI_NEWPORT_CONSOLE
source "drivers/video/logo/Kconfig"
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index ee8dafb..9f8a7f0 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -11,7 +11,7 @@ fb-y := fbmem.o fbmon.o fbcmap.o fbsysfs.o \
modedb.o fbcvt.o
fb-objs := $(fb-y)

-obj-$(CONFIG_VT) += console/
+obj-y += console/
obj-$(CONFIG_LOGO) += logo/
obj-y += backlight/

diff --git a/drivers/video/console/Kconfig b/drivers/video/console/Kconfig
index c2d11fe..cfee482 100644
--- a/drivers/video/console/Kconfig
+++ b/drivers/video/console/Kconfig
@@ -6,7 +6,7 @@ menu "Console display driver support"

config VGA_CONSOLE
bool "VGA text console" if EXPERT || !X86
- depends on !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
+ depends on VT && !4xx && !8xx && !SPARC && !M68K && !PARISC && !FRV && !SUPERH && !BLACKFIN && !AVR32 && !MN10300 && (!ARM || ARCH_FOOTBRIDGE || ARCH_INTEGRATOR || ARCH_NETWINDER)
default y
help
Saying Y here will allow you to use Linux in text mode through a
@@ -45,7 +45,7 @@ config VGACON_SOFT_SCROLLBACK_SIZE
screenfuls of scrollback buffer

config MDA_CONSOLE
- depends on !M68K && !PARISC && ISA
+ depends on VT && !M68K && !PARISC && ISA
tristate "MDA text console (dual-headed) (EXPERIMENTAL)"
---help---
Say Y here if you have an old MDA or monochrome Hercules graphics
@@ -61,14 +61,14 @@ config MDA_CONSOLE

config SGI_NEWPORT_CONSOLE
tristate "SGI Newport Console support"
- depends on SGI_IP22
+ depends on VT && SGI_IP22
help
Say Y here if you want the console on the Newport aka XL graphics
card of your Indy. Most people say Y here.

config DUMMY_CONSOLE
bool
- depends on VGA_CONSOLE!=y || SGI_NEWPORT_CONSOLE!=y
+ depends on VT && (VGA_CONSOLE!=y || SGI_NEWPORT_CONSOLE!=y)
default y

config DUMMY_CONSOLE_COLUMNS
@@ -89,7 +89,7 @@ config DUMMY_CONSOLE_ROWS

config FRAMEBUFFER_CONSOLE
tristate "Framebuffer Console support"
- depends on FB
+ depends on VT && FB
select CRC32
help
Low-level framebuffer-based console driver.
@@ -122,16 +122,37 @@ config FRAMEBUFFER_CONSOLE_ROTATION

config STI_CONSOLE
bool "STI text console"
- depends on PARISC
+ depends on VT && PARISC
default y
help
The STI console is the builtin display/keyboard on HP-PARISC
machines. Say Y here to build support for it into your kernel.
The alternative is to use your primary serial port as a console.

+config FBLOG
+ tristate "Framebuffer Kernel Log Driver"
+ depends on !VT && FB
+ default n
+ help
+ This driver displays all kernel log messages on all connected
+ framebuffers. It is mutually exclusive with CONFIG_FRAMEBUFFER_CONSOLE
+ and CONFIG_VT. It was mainly created for debugging purposes when
+ CONFIG_VT is not selected but you still want kernel boot messages on
+ the screen.
+
+ This driver overwrites all other graphics output on the framebuffer as
+ long as it is active so the kernel log will always be visible. You
+ need to disable this driver via sysfs to be able to start another
+ graphics application.
+
+ If unsure, say N.
+
+ To compile this driver as a module, choose M here: the module will
+ be called fblog.
+
config FONTS
bool "Select compiled-in fonts"
- depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE
+ depends on FRAMEBUFFER_CONSOLE || STI_CONSOLE || FBLOG
help
Say Y here if you would like to use fonts other than the default
your frame buffer console usually use.
@@ -158,7 +179,7 @@ config FONT_8x8

config FONT_8x16
bool "VGA 8x16 font" if FONTS
- depends on FRAMEBUFFER_CONSOLE || SGI_NEWPORT_CONSOLE || STI_CONSOLE || USB_SISUSBVGA_CON
+ depends on FRAMEBUFFER_CONSOLE || SGI_NEWPORT_CONSOLE || STI_CONSOLE || USB_SISUSBVGA_CON || FBLOG
default y if !SPARC && !FONTS
help
This is the "high resolution" font for the VGA frame buffer (the one
diff --git a/drivers/video/console/Makefile b/drivers/video/console/Makefile
index a862e91..f608c97 100644
--- a/drivers/video/console/Makefile
+++ b/drivers/video/console/Makefile
@@ -20,6 +20,7 @@ font-objs += $(font-objs-y)

# Each configuration option enables a list of files.

+obj-$(CONFIG_FBLOG) += fblog.o font.o
obj-$(CONFIG_DUMMY_CONSOLE) += dummycon.o
obj-$(CONFIG_SGI_NEWPORT_CONSOLE) += newport_con.o font.o
obj-$(CONFIG_STI_CONSOLE) += sticon.o sticore.o font.o
diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
new file mode 100644
index 0000000..ea83643
--- /dev/null
+++ b/drivers/video/console/fblog.c
@@ -0,0 +1,55 @@
+/*
+ * Framebuffer Kernel Log Driver
+ * Copyright (c) 2012 David Herrmann <[email protected]>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+/*
+ * Framebuffer Kernel Log
+ * This driver prints the kernel log to all connected display devices. It
+ * replaces CONFIG_VT and cannot run simultaneously with it. It does not provide
+ * any virtual-terminal, though. It should only be used to get kernel boot
+ * messages to debug kernel errors.
+ * Hence, this driver is neither optimized for speed, nor does it provide any
+ * fancy features like colored text output. After booting is done, the init
+ * process should set /sys/class/graphics/fblog/active to 0 which disables this
+ * driver and you can start using the graphics devices. During shutdown, you can
+ * set this to 1 to get log messages again.
+ * This driver forcibly writes to the framebuffer while active, therefore, you
+ * cannot run other graphics applications simultaneously.
+ *
+ * fblog_redraw_line() is heavily based on the fbcon driver. See bitblit.c for
+ * the original implementation copyrighted by:
+ * Copyright (C) 2004 Antonino Daplas <[email protected]>
+ *
+ * Please note that nearly all functions here must be called with console_lock
+ * held. This way, we have no locking problems and do not need special
+ * synchronization.
+ */
+
+#include <linux/atomic.h>
+#include <linux/console.h>
+#include <linux/fb.h>
+#include <linux/font.h>
+#include <linux/module.h>
+
+static int __init fblog_init(void)
+{
+ return 0;
+}
+
+static void __exit fblog_exit(void)
+{
+}
+
+module_init(fblog_init);
+module_exit(fblog_exit);
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("David Herrmann <[email protected]>");
+MODULE_DESCRIPTION("Framebuffer Kernel Log Driver");
--
1.7.10.4

2012-06-16 22:07:10

by David Herrmann

[permalink] [raw]
Subject: [PATCH 04/10] fblog: implement fblog_redraw()

This mostly copies the functionality from drivers/video/console/bitblit.c
so we can draw the console content on all available framebuffers.

All speed optimizations have been removed for simplicity. The original
code depends heavily on CONFIG_VT so we cannot share the codebase here.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 126 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 126 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index 8038dcc..e790971 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -197,6 +197,131 @@ static void fblog_buf_write(struct fblog_buf *buf, const char *str, size_t len)
}
}

+static void fblog_redraw_aligned(struct fblog_fb *fb, const char *s, u32 cnt,
+ u32 d_pitch, u32 s_pitch, u32 cellsize,
+ struct fb_image *image, u8 *dst)
+{
+ struct fb_info *info = fb->info;
+ const struct font_desc *font = fb->font;
+ u32 idx = font->width >> 3;
+ u8 *src;
+
+ while (cnt--) {
+ src = (void*)(font->data + (*s++ & 0xff) * cellsize);
+ fb_pad_aligned_buffer(dst, d_pitch, src, idx, image->height);
+ dst += s_pitch;
+ }
+
+ info->fbops->fb_imageblit(info, image);
+}
+
+static void fblog_redraw_unaligned(struct fblog_fb *fb, const char *s, u32 cnt,
+ u32 d_pitch, u32 s_pitch, u32 cellsize,
+ struct fb_image *image, u8 *dst)
+{
+ struct fb_info *info = fb->info;
+ const struct font_desc *font = fb->font;
+ u32 shift_low = 0, mod = font->width % 8;
+ u32 shift_high = 8;
+ u32 idx = font->width >> 3;
+ u8 *src;
+
+ while (cnt--) {
+ src = (void*)(font->data + (*s++ & 0xff) * cellsize);
+ fb_pad_unaligned_buffer(dst, d_pitch, src, idx,
+ image->height, shift_high,
+ shift_low, mod);
+ shift_low += mod;
+ dst += (shift_low >= 8) ? s_pitch : s_pitch - 1;
+ shift_low &= 7;
+ shift_high = 8 - shift_low;
+ }
+
+ info->fbops->fb_imageblit(info, image);
+}
+
+static void fblog_redraw_line(struct fblog_fb *fb, size_t line,
+ const char *str, size_t len)
+{
+ struct fb_info *info = fb->info;
+ const struct font_desc *font = fb->font;
+ struct fb_image image;
+ u32 width = DIV_ROUND_UP(font->width, 8);
+ u32 cellsize = width * font->height;
+ u32 maxcnt = info->pixmap.size / cellsize;
+ u32 scan_align = info->pixmap.scan_align - 1;
+ u32 buf_align = info->pixmap.buf_align - 1;
+ u32 mod = font->width % 8;
+ u32 cnt, pitch, size;
+ u8 *dst;
+
+ image.fg_color = 7;
+ image.bg_color = 0;
+ image.dx = 0;
+ image.dy = line * font->height;
+ image.height = font->height;
+ image.depth = 1;
+
+ while (len) {
+ if (len > maxcnt)
+ cnt = maxcnt;
+ else
+ cnt = len;
+
+ image.width = font->width * cnt;
+ pitch = DIV_ROUND_UP(image.width, 8) + scan_align;
+ pitch &= ~scan_align;
+ size = pitch * image.height + buf_align;
+ size &= ~buf_align;
+ dst = fb_get_buffer_offset(info, &info->pixmap, size);
+ image.data = dst;
+
+ if (!mod)
+ fblog_redraw_aligned(fb, str, cnt, pitch, width,
+ cellsize, &image, dst);
+ else
+ fblog_redraw_unaligned(fb, str, cnt, pitch, width,
+ cellsize, &image, dst);
+
+ image.dx += cnt * font->width;
+ len -= cnt;
+ str += cnt;
+ }
+}
+
+static void fblog_redraw_clear(struct fblog_fb *fb)
+{
+ struct fb_fillrect region;
+ struct fb_info *info = fb->info;
+
+ region.color = 0;
+ region.dx = 0;
+ region.dy = 0;
+ region.width = info->var.xres;
+ region.height = info->var.yres;
+ region.rop = ROP_COPY;
+
+ info->fbops->fb_fillrect(info, &region);
+}
+
+static void fblog_redraw(struct fblog_fb *fb)
+{
+ size_t i, len;
+
+ if (!fb || !fb->font || test_bit(FBLOG_KILLED, &fb->flags) ||
+ test_bit(FBLOG_SUSPENDED, &fb->flags) ||
+ test_bit(FBLOG_BLANKED, &fb->flags))
+ return;
+
+ fblog_redraw_clear(fb);
+
+ for (i = 0; i < fb->buf.height; ++i) {
+ len = strnlen(fb->buf.lines[i], fb->buf.width);
+ if (len)
+ fblog_redraw_line(fb, i, fb->buf.lines[i], len);
+ }
+}
+
static struct fblog_fb *fblog_info2fb(struct fb_info *info)
{
if (!info || info->node < 0 || info->node >= FB_MAX ||
@@ -244,6 +369,7 @@ static void fblog_register(struct fb_info *info)
width = info->var.xres / fb->font->width;
height = info->var.yres / fb->font->height;
fblog_buf_resize(&fb->buf, width, height);
+ fblog_redraw(fb);
}

return;
--
1.7.10.4

2012-06-16 22:07:37

by David Herrmann

[permalink] [raw]
Subject: [PATCH 02/10] fblog: implement buffer management

Each available framebuffer can have a different font and buffer size
inside of fblog. Therefore, we need to remember all the log messages that
are currently printed on screen. We save them as an array of lines which
can be rotated and traversed very fast.

This also implements a very trivial way of resizing the buffer but still
keeping the content. As there is no need to improve this for speed, we can
keep it this way.

Signed-off-by: David Herrmann <[email protected]>
---
drivers/video/console/fblog.c | 126 +++++++++++++++++++++++++++++++++++++++++
1 file changed, 126 insertions(+)

diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
index ea83643..1504ba9 100644
--- a/drivers/video/console/fblog.c
+++ b/drivers/video/console/fblog.c
@@ -39,6 +39,132 @@
#include <linux/font.h>
#include <linux/module.h>

+/**
+ * struct fblog_buf: Console text buffer
+ *
+ * Each framebuffer has its own text buffer which contains all characters that
+ * are currently printed on screen. The buffers might have different sizes and
+ * can be resized during runtime. When the buffer content changes, we redraw the
+ * screen.
+ *
+ * width: Width of buffer in characters
+ * height: Height of buffer in characters
+ * lines: Array of lines
+ * pos_x: Cursor x-position
+ * pos_y: Cursor y-position
+ */
+struct fblog_buf {
+ size_t width;
+ size_t height;
+ char **lines;
+ size_t pos_x;
+ size_t pos_y;
+};
+
+static void fblog_buf_resize(struct fblog_buf *buf, size_t width,
+ size_t height)
+{
+ char **lines = NULL;
+ size_t i, j, minw, minh;
+
+ if (buf->height == height && buf->width == width)
+ return;
+
+ if (width && height) {
+ lines = kzalloc(height * sizeof(char*), GFP_KERNEL);
+ if (!lines)
+ return;
+
+ for (i = 0; i < height; ++i) {
+ lines[i] = kzalloc(width * sizeof(char), GFP_KERNEL);
+ if (!lines[i]) {
+ while (i--)
+ kfree(lines[i]);
+ return;
+ }
+ }
+
+ /* copy old lines */
+ minw = min(width, buf->width);
+ minh = min(height, buf->height);
+ if (height >= buf->height)
+ i = 0;
+ else
+ i = buf->height - height;
+
+ for (j = 0; j < minh; ++i, ++j)
+ memcpy(lines[j], buf->lines[i], minw * sizeof(char));
+ } else {
+ width = 0;
+ height = 0;
+ }
+
+ for (i = 0; i < buf->height; ++i)
+ kfree(buf->lines[i]);
+ kfree(buf->lines);
+
+ buf->lines = lines;
+ buf->width = width;
+ buf->height = height;
+}
+
+static void fblog_buf_init(struct fblog_buf *buf)
+{
+ fblog_buf_resize(buf, 80, 24);
+}
+
+static void fblog_buf_deinit(struct fblog_buf *buf)
+{
+ fblog_buf_resize(buf, 0, 0);
+}
+
+static void fblog_buf_rotate(struct fblog_buf *buf)
+{
+ char *line;
+
+ if (!buf->height)
+ return;
+
+ line = buf->lines[0];
+ memset(line, 0, sizeof(char) * buf->width);
+
+ memmove(buf->lines, &buf->lines[1], sizeof(char*) * (buf->height - 1));
+ buf->lines[buf->height - 1] = line;
+}
+
+static void fblog_buf_write(struct fblog_buf *buf, const char *str, size_t len)
+{
+ char c;
+
+ if (!buf->height)
+ return;
+
+ while (len--) {
+ c = *str++;
+
+ if (c == '\n') {
+ buf->pos_x = 0;
+ if (++buf->pos_y >= buf->height) {
+ buf->pos_y = buf->height - 1;
+ fblog_buf_rotate(buf);
+ }
+ } else if (c == 0) {
+ /* ignore */
+ } else {
+ if (buf->pos_x >= buf->width) {
+ buf->pos_x = 0;
+ ++buf->pos_y;
+ }
+ if (buf->pos_y >= buf->height) {
+ buf->pos_y = buf->height - 1;
+ fblog_buf_rotate(buf);
+ }
+
+ buf->lines[buf->pos_y][buf->pos_x++] = c;
+ }
+ }
+}
+
static int __init fblog_init(void)
{
return 0;
--
1.7.10.4

2012-06-16 22:31:47

by Alan

[permalink] [raw]
Subject: Re: [PATCH 04/10] fblog: implement fblog_redraw()

On Sun, 17 Jun 2012 00:04:20 +0200
David Herrmann <[email protected]> wrote:

> This mostly copies the functionality from drivers/video/console/bitblit.c
> so we can draw the console content on all available framebuffers.
>
> All speed optimizations have been removed for simplicity. The original
> code depends heavily on CONFIG_VT so we cannot share the codebase here.

No. That means we've got two sets of code to maintain not one. Fix the
dependancy.

Pull the relevant subset of struct vc_data into another struct
Make struct vc_data be

struct vc_data {
struct vc_whatever
rest
}

Alan

2012-06-16 22:42:27

by Bruno Prémont

[permalink] [raw]
Subject: Re: [PATCH 05/10] fblog: add framebuffer helpers

On Sun, 17 June 2012 David Herrmann <[email protected]> wrote:
> These helpers scan the system for all available framebuffers and register
> or unregister them. This is needed during startup and stopping fblog so we
> are aware of all connected displays.
>
> The third helper handles mode changes by rescanning the mode and adjusting
> the buffer size.
>
> Signed-off-by: David Herrmann <[email protected]>
> ---
> drivers/video/console/fblog.c | 29 +++++++++++++++++++++++++++++
> 1 file changed, 29 insertions(+)
>
> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
> index e790971..7d4032e 100644
> --- a/drivers/video/console/fblog.c
> +++ b/drivers/video/console/fblog.c
> @@ -399,6 +399,35 @@ static void fblog_unregister(struct fblog_fb *fb)
> kfree(fb);
> }
>
> +static void fblog_register_all(void)
> +{
> + int i;
> +
> + for (i = 0; i < FB_MAX; ++i)
> + fblog_register(registered_fb[i]);

You should take registration_lock mutex for accessing registered_fb[],
even better would be to make use of get_fb_info() and put_fb_info()

> +}
> +
> +static void fblog_unregister_all(void)
> +{
> + int i;
> +
> + for (i = 0; i < FB_MAX; ++i)
> + fblog_unregister(fblog_info2fb(registered_fb[i]));

Same here.

Though for unregistering I'm wondering why you still scan through
registered_fb[], you should just scan your fblog_fbs[] array!

But here again, make sure to have proper locking to not get races with
registration of new framebuffers or removal of existing ones via
notifications.

> +}
> +
> +static void fblog_refresh(struct fblog_fb *fb)
> +{
> + unsigned int width, height;
> +
> + if (!fb || !fb->font)
> + return;
> +
> + width = fb->info->var.xres / fb->font->width;
> + height = fb->info->var.yres / fb->font->height;
> + fblog_buf_resize(&fb->buf, width, height);
> + fblog_redraw(fb);
> +}
> +

All these new functions are still unused, for easier following of your
patch series it would be nice to have them connected when they are
introduced as otherwise on has to search all following patches for
finding possible users.

> static int __init fblog_init(void)
> {
> return 0;

Bruno

2012-06-18 18:36:52

by David Herrmann

[permalink] [raw]
Subject: Re: [PATCH 04/10] fblog: implement fblog_redraw()

Hi Alan

On Sun, Jun 17, 2012 at 12:35 AM, Alan Cox <[email protected]> wrote:
> On Sun, 17 Jun 2012 00:04:20 +0200
> David Herrmann <[email protected]> wrote:
>
>> This mostly copies the functionality from drivers/video/console/bitblit.c
>> so we can draw the console content on all available framebuffers.
>>
>> All speed optimizations have been removed for simplicity. The original
>> code depends heavily on CONFIG_VT so we cannot share the codebase here.
>
> No. That means we've got two sets of code to maintain not one. Fix the
> dependancy.
>
> Pull the relevant subset of struct vc_data into another struct
> Make struct vc_data be
>
> struct vc_data {
> ? ? ? ?struct vc_whatever
> ? ? ? ?rest
> }

It's a bit more complex as we cannot call scr_read() either. Hence, I
need to assemble the array of printed characters before calling the
redraw functions. But that should be feasible, too. I just need to
figure out how to avoid heavy allocations during redraw to avoid
slowdowns.

If there are no objections I will send these patches as a separate
patchset as we can apply it without fblog.

> Alan

Thanks for reviewing
David

2012-06-18 18:50:16

by David Herrmann

[permalink] [raw]
Subject: Re: [PATCH 05/10] fblog: add framebuffer helpers

Hi Bruno

On Sun, Jun 17, 2012 at 12:33 AM, Bruno Pr?mont
<[email protected]> wrote:
> On Sun, 17 June 2012 David Herrmann <[email protected]> wrote:
>> These helpers scan the system for all available framebuffers and register
>> or unregister them. This is needed during startup and stopping fblog so we
>> are aware of all connected displays.
>>
>> The third helper handles mode changes by rescanning the mode and adjusting
>> the buffer size.
>>
>> Signed-off-by: David Herrmann <[email protected]>
>> ---
>> ?drivers/video/console/fblog.c | ? 29 +++++++++++++++++++++++++++++
>> ?1 file changed, 29 insertions(+)
>>
>> diff --git a/drivers/video/console/fblog.c b/drivers/video/console/fblog.c
>> index e790971..7d4032e 100644
>> --- a/drivers/video/console/fblog.c
>> +++ b/drivers/video/console/fblog.c
>> @@ -399,6 +399,35 @@ static void fblog_unregister(struct fblog_fb *fb)
>> ? ? ? kfree(fb);
>> ?}
>>
>> +static void fblog_register_all(void)
>> +{
>> + ? ? int i;
>> +
>> + ? ? for (i = 0; i < FB_MAX; ++i)
>> + ? ? ? ? ? ? fblog_register(registered_fb[i]);
>
> You should take registration_lock mutex for accessing registered_fb[],
> even better would be to make use of get_fb_info() and put_fb_info()

Indeed, I will change it to use get_fb_info()/put_fb_info().

Btw., is it safe to call console_lock() during
FB_EVENT_FB_(UN)REGISTERED? I definitely need the console lock when
calling fblog_register() but I am not sure whether all fb-drivers lock
the console during "(un)register_framebuffer()". Otherwise, I would
have to avoid redrawing the console inside of fblog_register().

>> +}
>> +
>> +static void fblog_unregister_all(void)
>> +{
>> + ? ? int i;
>> +
>> + ? ? for (i = 0; i < FB_MAX; ++i)
>> + ? ? ? ? ? ? fblog_unregister(fblog_info2fb(registered_fb[i]));
>
> Same here.
>
> Though for unregistering I'm wondering why you still scan through
> registered_fb[], you should just scan your fblog_fbs[] array!

Oops, you're right. I will change it.

> But here again, make sure to have proper locking to not get races with
> registration of new framebuffers or removal of existing ones via
> notifications.
>
>> +}
>> +
>> +static void fblog_refresh(struct fblog_fb *fb)
>> +{
>> + ? ? unsigned int width, height;
>> +
>> + ? ? if (!fb || !fb->font)
>> + ? ? ? ? ? ? return;
>> +
>> + ? ? width = fb->info->var.xres / fb->font->width;
>> + ? ? height = fb->info->var.yres / fb->font->height;
>> + ? ? fblog_buf_resize(&fb->buf, width, height);
>> + ? ? fblog_redraw(fb);
>> +}
>> +
>
> All these new functions are still unused, for easier following of your
> patch series it would be nice to have them connected when they are
> introduced as otherwise on has to search all following patches for
> finding possible users.

I have to admit the split was crap. I am sorry. I will recreate the
patchset with a proper split. Thanks!

>> ?static int __init fblog_init(void)
>> ?{
>> ? ? ? return 0;
>
> Bruno

Thanks for reviewing, regards
David

2012-06-18 19:06:20

by David Herrmann

[permalink] [raw]
Subject: Re: [RFC 00/10] fblog: framebuffer kernel log driver

Hi

On Sun, Jun 17, 2012 at 12:04 AM, David Herrmann
<[email protected]> wrote:
> Hi
>
> As some might know I am working on making CONFIG_VT obsolete. But as a developer
> it is often useful to have a kernel-log on the screen during boot to debug many
> common kernel(-config) errors. However, without CONFIG_VT we cannot use the
> VGA/framebbufer consoles either. Therefore, I am working on a small driver
> called "fblog".
>
> This driver simply writes the kernel log to all connected framebuffers. It works
> similar to fbcon but removes all the complexity of the virtual terminals. There
> is a sysfs attribute called "active" that allows to enable/disable fblog so
> user-space can start an xserver or similar.
>
> The main purpose is debugging kernel boot problems. Therefore, it is not
> optimized for speed and I tried keeping it simple. I splitted the patches
> into 10 small chunks to make review easier.
>
> I would be glad if someone could review this and tell me whether this is
> something we could include in mainline or not.
>
>
> There are still some issues but apart from them it works fine on my
> machine (x86):
> ?- I register the fblog device during module_init and need to call
> ? ?module_get(). However, this means it is impossible to call "rmmod fblog" as
> ? ?fblog has a reference to itself. Using "rmmod -f fblog" works fine but is a
> ? ?bit ugly. Is there a nice way to fix this? Otherwise I would need to call
> ? ?device_get() in module_exit() if there is a pending user of the fblog-device
> ? ?even though I unregistered it.
> ?- I redraw all framebuffers while holding the console-lock. This may slow down
> ? ?machines with more than 2 framebuffers (like 10 or 20). However, as this is
> ? ?supposed to be a debug driver, I think I can ignore this? If someone wants
> ? ?to improve the redraw logic to avoid redrawing the whole screen all the
> ? ?time, I would be glad to include it in this patchset :)
> ?- I am really no expert regarding the framebuffer subsystem. So I would
> ? ?appreciate it if someone could comment whether I need to handle the events
> ? ?in a different way or whether it is ok the way it is now.

One additional issue:
With udlfb.c we have hotplug capable framebuffers. However, fbcon and
fblog currently never close a framebuffer if not explicitely requested
by user-space. Therefore, if a framebuffer device is removed, the
FB_EVENT_FB_UNREGISTER event will never be sent because fbcon/fblog
still have a reference to the framebuffer(-driver). Therefore, the
number of available fbs will grow until there are no more free
indices.

See dlfb_usb_disconnect() in udlfb.c for an example. It does not
invoke unregister_framebuffer() unless the last user closed the FB.
udlfb disables the console on its framebuffer devices to avoid this,
but this doesn't seem to be a good solution.
How about sending an FB_EVENT_FB_DISCONNECT event during unlink_framebuffer()?

This still doesn't force user-space to close /dev/fbX but it at least
will make it possible to fblog/fbcon to close the framebuffer. fbmem.c
can then still be modified to mark the open file as dead so user-space
will also close the device hopefully.

Regards
David