Discussion:
[PATCH V2] video: implement a simple framebuffer driver
Stephen Warren
2013-04-04 02:39:43 UTC
Permalink
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.

This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.

Signed-off-by: Stephen Warren <swarren-3lzwWm7+***@public.gmane.org>
---
v2: s/dumb/simple/ throughout. Provide more details on pixel format.

I ended up going with a separate FB driver:
* DRM/KMS look much more complex, and don't provide any benefit that I can
tell for this simple driver.
* Creating a separate driver rather than adjusting offb.c to work allows a
new clean binding to be defined, and doesn't require removing or ifdefing
PPC-isms in offb.c.
---
.../bindings/video/simple-framebuffer.txt | 25 +++
drivers/video/Kconfig | 17 ++
drivers/video/Makefile | 1 +
drivers/video/simplefb.c | 234 ++++++++++++++++++++
4 files changed, 277 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/simple-framebuffer.txt
create mode 100644 drivers/video/simplefb.c

diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
new file mode 100644
index 0000000..3ea4605
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -0,0 +1,25 @@
+Simple Framebuffer
+
+A simple frame-buffer describes a raw memory region that may be rendered to,
+with the assumption that the display hardware has already been set up to scan
+out from that buffer.
+
+Required properties:
+- compatible: "simple-framebuffer"
+- reg: Should contain the location and size of the framebuffer memory.
+- width: The width of the framebuffer in pixels.
+- height: The height of the framebuffer in pixels.
+- stride: The number of bytes in each line of the framebuffer.
+- format: The format of the framebuffer surface. Valid values are:
+ - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+
+Example:
+
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
diff --git a/drivers/video/Kconfig b/drivers/video/Kconfig
index 0181a87..e946e9c 100644
--- a/drivers/video/Kconfig
+++ b/drivers/video/Kconfig
@@ -2462,6 +2462,23 @@ config FB_HYPERV
help
This framebuffer driver supports Microsoft Hyper-V Synthetic Video.

+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
+ select FB_CFB_FILLRECT
+ select FB_CFB_COPYAREA
+ select FB_CFB_IMAGEBLIT
+ help
+ Say Y if you want support for a simple frame-buffer.
+
+ This driver assumes that the display hardware has been initialized
+ before the kernel boots, and the kernel will simply render to the
+ pre-allocated frame buffer surface.
+
+ Configuration re: surface address, size, and format must be provided
+ through device tree, or potentially plain old platform data in the
+ future.
+
source "drivers/video/omap/Kconfig"
source "drivers/video/omap2/Kconfig"
source "drivers/video/exynos/Kconfig"
diff --git a/drivers/video/Makefile b/drivers/video/Makefile
index 97f7b6d..859f1fa 100644
--- a/drivers/video/Makefile
+++ b/drivers/video/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_FB_MX3) += mx3fb.o
obj-$(CONFIG_FB_DA8XX) += da8xx-fb.o
obj-$(CONFIG_FB_MXS) += mxsfb.o
obj-$(CONFIG_FB_SSD1307) += ssd1307fb.o
+obj-$(CONFIG_FB_SIMPLE) += simplefb.o

# the test framebuffer is last
obj-$(CONFIG_FB_VIRTUAL) += vfb.o
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
new file mode 100644
index 0000000..8105c3d
--- /dev/null
+++ b/drivers/video/simplefb.c
@@ -0,0 +1,234 @@
+/*
+ * Simplest possible simple frame-buffer driver, as a platform device
+ *
+ * Copyright (c) 2013, Stephen Warren
+ *
+ * Based on q40fb.c, which was:
+ * Copyright (C) 2001 Richard Zidlicky <rz-***@public.gmane.org>
+ *
+ * Also based on offb.c, which was:
+ * Copyright (C) 1997 Geert Uytterhoeven
+ * Copyright (C) 1996 Paul Mackerras
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static struct fb_fix_screeninfo simplefb_fix = {
+ .id = "simple",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .visual = FB_VISUAL_TRUECOLOR,
+ .accel = FB_ACCEL_NONE,
+};
+
+static struct fb_var_screeninfo simplefb_var = {
+ .height = -1,
+ .width = -1,
+ .activate = FB_ACTIVATE_NOW,
+ .vmode = FB_VMODE_NONINTERLACED,
+};
+
+static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
+ u_int transp, struct fb_info *info)
+{
+ u32 *pal = info->pseudo_palette;
+ u32 cr = red >> (16 - info->var.red.length);
+ u32 cg = green >> (16 - info->var.green.length);
+ u32 cb = blue >> (16 - info->var.blue.length);
+ u32 value;
+
+ if (regno >= 16)
+ return -EINVAL;
+
+ value = (cr << info->var.red.offset) |
+ (cg << info->var.green.offset) |
+ (cb << info->var.blue.offset);
+ if (info->var.transp.length > 0) {
+ u32 mask = (1 << info->var.transp.length) - 1;
+ mask <<= info->var.transp.offset;
+ value |= mask;
+ }
+ pal[regno] = value;
+
+ return 0;
+}
+
+static struct fb_ops simplefb_ops = {
+ .owner = THIS_MODULE,
+ .fb_setcolreg = simplefb_setcolreg,
+ .fb_fillrect = cfb_fillrect,
+ .fb_copyarea = cfb_copyarea,
+ .fb_imageblit = cfb_imageblit,
+};
+
+struct simplefb_format {
+ const char *name;
+ u32 bits_per_pixel;
+ struct fb_bitfield red;
+ struct fb_bitfield green;
+ struct fb_bitfield blue;
+ struct fb_bitfield transp;
+};
+
+struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
+
+struct simplefb_params {
+ u32 width;
+ u32 height;
+ u32 stride;
+ struct simplefb_format *format;
+};
+
+static int simplefb_parse_dt(struct platform_device *pdev,
+ struct simplefb_params *params)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+ const char *format;
+ int i;
+
+ ret = of_property_read_u32(np, "width", &params->width);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse width property\n");
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "height", &params->height);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse height property\n");
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "stride", &params->stride);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse stride property\n");
+ return ret;
+ }
+
+ ret = of_property_read_string(np, "format", &format);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse format property\n");
+ return ret;
+ }
+ params->format = NULL;
+ for (i = 0; i < ARRAY_SIZE(simplefb_formats); i++) {
+ if (strcmp(format, simplefb_formats[i].name))
+ continue;
+ params->format = &simplefb_formats[i];
+ break;
+ }
+ if (!params->format) {
+ dev_err(&pdev->dev, "Invalid format value\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int simplefb_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct simplefb_params params;
+ struct fb_info *info;
+ struct resource *mem;
+
+ if (fb_get_options("simplefb", NULL))
+ return -ENODEV;
+
+ ret = simplefb_parse_dt(pdev, &params);
+ if (ret)
+ return ret;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem) {
+ dev_err(&pdev->dev, "No memory resource\n");
+ return -EINVAL;
+ }
+
+ info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+ if (!info)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, info);
+
+ info->fix = simplefb_fix;
+ info->fix.smem_start = mem->start;
+ info->fix.smem_len = resource_size(mem);
+ info->fix.line_length = params.stride;
+
+ info->var = simplefb_var;
+ info->var.xres = params.width;
+ info->var.yres = params.height;
+ info->var.xres_virtual = params.width;
+ info->var.yres_virtual = params.height;
+ info->var.bits_per_pixel = params.format->bits_per_pixel;
+ info->var.red = params.format->red;
+ info->var.green = params.format->green;
+ info->var.blue = params.format->blue;
+ info->var.transp = params.format->transp;
+
+ info->fbops = &simplefb_ops;
+ info->flags = FBINFO_DEFAULT;
+ info->screen_base = devm_ioremap(&pdev->dev, info->fix.smem_start,
+ info->fix.smem_len);
+ if (!info->screen_base) {
+ framebuffer_release(info);
+ return -ENODEV;
+ }
+ info->pseudo_palette = (void *)(info + 1);
+
+ ret = register_framebuffer(info);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
+ framebuffer_release(info);
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
+
+ return 0;
+}
+
+static int simplefb_remove(struct platform_device *pdev)
+{
+ struct fb_info *info = platform_get_drvdata(pdev);
+
+ unregister_framebuffer(info);
+ framebuffer_release(info);
+
+ return 0;
+}
+
+static const struct of_device_id simplefb_of_match[] = {
+ { .compatible = "simple-framebuffer", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, simplefb_of_match);
+
+static struct platform_driver simplefb_driver = {
+ .driver = {
+ .name = "simple-framebuffer",
+ .owner = THIS_MODULE,
+ .of_match_table = simplefb_of_match,
+ },
+ .probe = simplefb_probe,
+ .remove = simplefb_remove,
+};
+module_platform_driver(simplefb_driver);
+
+MODULE_AUTHOR("Stephen Warren <swarren-3lzwWm7+***@public.gmane.org>");
+MODULE_DESCRIPTION("Simple framebuffer driver");
+MODULE_LICENSE("GPL v2");
--
1.7.10.4
Andrew Morton
2013-04-09 00:16:37 UTC
Permalink
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
Post by Stephen Warren
+struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
I'll make this static.
Stephen Warren
2013-04-09 03:16:00 UTC
Permalink
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I wouldn't be at all surprised if others want to feed it platform data
rather than parameterizing it through device tree. All my platforms use
DT though, so I didn't want to add that support without any user; it'd
just be dead code for now. But, if someone wants that, I can certainly
code it up or test/review after the change etc.
Geert Uytterhoeven
2013-04-09 08:08:12 UTC
Permalink
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
Indeed. Instead of consolidating, we seem to get more of them: offb, vesafb,
efifb, ...

cfr. near the tail of drivers/video/Makefile (don't know about all of them):
# Platform or fallback drivers go here
obj-$(CONFIG_FB_UVESA) += uvesafb.o
obj-$(CONFIG_FB_VESA) += vesafb.o
obj-$(CONFIG_FB_EFI) += efifb.o
obj-$(CONFIG_FB_VGA16) += vga16fb.o
obj-$(CONFIG_FB_OF) += offb.o
obj-$(CONFIG_FB_BF537_LQ035) += bf537-lq035.o
obj-$(CONFIG_FB_BF54X_LQ043) += bf54x-lq043fb.o
obj-$(CONFIG_FB_BFIN_LQ035Q1) += bfin-lq035q1-fb.o
obj-$(CONFIG_FB_BFIN_T350MCQB) += bfin-t350mcqb-fb.o
obj-$(CONFIG_FB_BFIN_7393) += bfin_adv7393fb.o
obj-$(CONFIG_FB_MX3) += mx3fb.o
obj-$(CONFIG_FB_DA8XX) += da8xx-fb.o
obj-$(CONFIG_FB_MXS) += mxsfb.o
obj-$(CONFIG_FB_SSD1307) += ssd1307fb.o

# the test framebuffer is last
obj-$(CONFIG_FB_VIRTUAL) += vfb.o

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-***@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Laurent Pinchart
2013-04-11 09:56:31 UTC
Permalink
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if it
helps deprecating fbdev in the long term, but I don't want it to offer an
excuse not to implement a DRM/KMS driver. In particular adding DT bindings
would force us to keep supporting the ABI for a (too) long time.
Post by Andrew Morton
Post by Stephen Warren
+struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
I'll make this static.
--
Regards,

Laurent Pinchart
Stephen Warren
2013-04-11 16:06:49 UTC
Permalink
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if it
helps deprecating fbdev in the long term, but I don't want it to offer an
excuse not to implement a DRM/KMS driver. In particular adding DT bindings
would force us to keep supporting the ABI for a (too) long time.
The platforms I intend to use this with only support device tree. Adding
support for platform data right now would just be dead code. If somebody
wants to use this driver with a board file based system rather than a DT
based system, it would be trivial do modify the driver to add a platform
data structure at that time.

Adding support for a platform data structure won't remove the need for
DT support in the driver; any platform that uses DT will always
configure this driver through the DT binding irrespective of whether
some other platform could configure it using platform_data.

I don't believe the DT bindings imply that they must be implemented by
an FB driver rather than a KMS driver. It's just that it's much simpler
to do so at present. If the whole FB subsystem goes away at some time,
it should be possible to implement a simplest-possible KMS driver that
supports the same DT binding. I didn't do it this way because supporting
a pre-allocated FB in DRM/KMS apparently means implementing a custom
memory allocator for this in the driver, which would be a lot of code
overhead when right now the driver can just use the FB subsystem and
simply return the address directly. The simplest possible FB driver
appears much simpler (less code size, less maintenance) than the
simplest possible KMS driver.

My inclination is that for many platforms, the bootloader support for
graphics output will appear first (before the kernel's), and this driver
will allow for the kernel to have a graphical console, allowing a more
complete/useful system to be available earlier. In many cases, that
window may be small; a DRM/KMS driver may appear soon after the basic
CPU/board/... support, and then people can switch to using it if they want.

That said, I also don't really see a problem not implementing a DRM/KMS
driver for a platform; a dumb frame-buffer works perfectly well for my
needs. Nobody would be forced to continue using it once a better
alternative existed.
Laurent Pinchart
2013-04-11 20:06:47 UTC
Permalink
Hi Stephen,
Post by Stephen Warren
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if
it helps deprecating fbdev in the long term, but I don't want it to offer
an excuse not to implement a DRM/KMS driver. In particular adding DT
bindings would force us to keep supporting the ABI for a (too) long time.
The platforms I intend to use this with only support device tree. Adding
support for platform data right now would just be dead code. If somebody
wants to use this driver with a board file based system rather than a DT
based system, it would be trivial do modify the driver to add a platform
data structure at that time.
What about using module parameters instead of DT bindings and platform data ?
As this is a hack designed to work around the lack of a proper display driver
the frame buffer address and size could just be passed by the boot loader
through the kernel command line, and the device would then be instantiated by
the driver.
Post by Stephen Warren
Adding support for a platform data structure won't remove the need for
DT support in the driver; any platform that uses DT will always
configure this driver through the DT binding irrespective of whether
some other platform could configure it using platform_data.
I don't believe the DT bindings imply that they must be implemented by
an FB driver rather than a KMS driver. It's just that it's much simpler
to do so at present. If the whole FB subsystem goes away at some time,
it should be possible to implement a simplest-possible KMS driver that
supports the same DT binding. I didn't do it this way because supporting
a pre-allocated FB in DRM/KMS apparently means implementing a custom
memory allocator for this in the driver, which would be a lot of code
overhead when right now the driver can just use the FB subsystem and
simply return the address directly. The simplest possible FB driver
appears much simpler (less code size, less maintenance) than the
simplest possible KMS driver.
My inclination is that for many platforms, the bootloader support for
graphics output will appear first (before the kernel's), and this driver
will allow for the kernel to have a graphical console, allowing a more
complete/useful system to be available earlier. In many cases, that
window may be small; a DRM/KMS driver may appear soon after the basic
CPU/board/... support, and then people can switch to using it if they want.
That said, I also don't really see a problem not implementing a DRM/KMS
driver for a platform; a dumb frame-buffer works perfectly well for my
needs. Nobody would be forced to continue using it once a better
alternative existed.
--
Regards,

Laurent Pinchart
Stephen Warren
2013-04-11 20:38:21 UTC
Permalink
Post by Laurent Pinchart
Hi Stephen,
Post by Stephen Warren
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if
it helps deprecating fbdev in the long term, but I don't want it to offer
an excuse not to implement a DRM/KMS driver. In particular adding DT
bindings would force us to keep supporting the ABI for a (too) long time.
The platforms I intend to use this with only support device tree. Adding
support for platform data right now would just be dead code. If somebody
wants to use this driver with a board file based system rather than a DT
based system, it would be trivial do modify the driver to add a platform
data structure at that time.
What about using module parameters instead of DT bindings and platform data ?
As this is a hack designed to work around the lack of a proper display driver
the frame buffer address and size could just be passed by the boot loader
through the kernel command line, and the device would then be instantiated by
the driver.
I'm afraid I strongly dislike the idea of using module parameters. One
of the things that I dislike the most about NVIDIA's downstream Android
kernels is the huge number of random parameters that are required on the
kernel command-line just to get it to boot.

I'd specifically prefer the configuration for this driver to be a device
tree binding so that it /is/ an ABI. That way, arbitrary software that
boots the Linux kernel will be able to target a common standard for how
to pass this information to the kernel. If we move to module parameters
instead, especially with the specific intent that the format of those
parameters is no longer an ABI, we run in to issues where a new kernel
requires a bootloader update in order to generate the new set of module
parameters that are required by the driver. If we say the module
parameters will never change except in a backwards-compatible fashion,
and hence that issue won't arise, then why not just make it a DT binding?

Do module parameters handle the case of multiple device instances?

Also, I really don't see this driver as a hack. It's a perfectly
reasonable situation to have some other SW set up the frame-buffer, and
Linux simply continue to use it. Perhaps that other SW even ran on a
difference CPU, or the parameters are hard-coded in HW design, and so
there's no HW that Linux ever could drive, so it just isn't possible to
create any more advanced driver.
Laurent Pinchart
2013-04-29 20:56:56 UTC
Permalink
Hi Stephen,

Sorry for the late reply.
Post by Stephen Warren
Post by Laurent Pinchart
Post by Stephen Warren
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be
rendered to, with the assumption that the display hardware has already
been set up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if
it helps deprecating fbdev in the long term, but I don't want it to
offer an excuse not to implement a DRM/KMS driver. In particular adding
DT bindings would force us to keep supporting the ABI for a (too) long
time.
The platforms I intend to use this with only support device tree. Adding
support for platform data right now would just be dead code. If somebody
wants to use this driver with a board file based system rather than a DT
based system, it would be trivial do modify the driver to add a platform
data structure at that time.
What about using module parameters instead of DT bindings and platform
data ? As this is a hack designed to work around the lack of a proper
display driver the frame buffer address and size could just be passed by
the boot loader through the kernel command line, and the device would
then be instantiated by the driver.
I'm afraid I strongly dislike the idea of using module parameters. One
of the things that I dislike the most about NVIDIA's downstream Android
kernels is the huge number of random parameters that are required on the
kernel command-line just to get it to boot.
I'd specifically prefer the configuration for this driver to be a device
tree binding so that it /is/ an ABI. That way, arbitrary software that
boots the Linux kernel will be able to target a common standard for how
to pass this information to the kernel. If we move to module parameters
instead, especially with the specific intent that the format of those
parameters is no longer an ABI, we run in to issues where a new kernel
requires a bootloader update in order to generate the new set of module
parameters that are required by the driver. If we say the module
parameters will never change except in a backwards-compatible fashion,
and hence that issue won't arise, then why not just make it a DT binding?
Do module parameters handle the case of multiple device instances?
Not well. It can be handled manually by using parameter arrays, but that's a
bit hackish.
Post by Stephen Warren
Also, I really don't see this driver as a hack. It's a perfectly reasonable
situation to have some other SW set up the frame-buffer, and Linux simply
continue to use it. Perhaps that other SW even ran on a difference CPU, or
the parameters are hard-coded in HW design, and so there's no HW that Linux
ever could drive, so it just isn't possible to create any more advanced
driver.
I totally agree that this driver would be very useful during board bring-up.
My concern is that it would also give an incentive to vendors not to develop a
proper KMS driver (or rather remove an incentive to develop one).
--
Regards,

Laurent Pinchart
Tomasz Figa
2013-04-29 21:15:13 UTC
Permalink
Hi Laurent,
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be
rendered to, with the assumption that the display hardware has
already been set up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if
it helps deprecating fbdev in the long term, but I don't want it to
offer an excuse not to implement a DRM/KMS driver. In particular adding
DT bindings would force us to keep supporting the ABI for a (too) long
time.
Well, there is also at least one legitimate use case for this driver.

I believe there exist embedded devices on which there is no need to
dynamically control the framebuffer. It needs one time initialization,
usually in bootloader, and then it is used as is, using constant
parameters as long as the system is running.

I doubt there is a need for any KMS (or any other control) driver for such
devices - dumb framebuffer driver would be everything needed in such case.

Best regards,
Tomasz
Laurent Pinchart
2013-04-29 21:20:43 UTC
Permalink
Hi Tomasz,
Post by Tomasz Figa
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be
rendered to, with the assumption that the display hardware has
already been set up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the
display hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware. Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer driver if
it helps deprecating fbdev in the long term, but I don't want it to
offer an excuse not to implement a DRM/KMS driver. In particular adding
DT bindings would force us to keep supporting the ABI for a (too) long
time.
Well, there is also at least one legitimate use case for this driver.
I believe there exist embedded devices on which there is no need to
dynamically control the framebuffer. It needs one time initialization,
usually in bootloader, and then it is used as is, using constant
parameters as long as the system is running.
I doubt there is a need for any KMS (or any other control) driver for such
devices - dumb framebuffer driver would be everything needed in such case.
As we want to deprecate the fbdev API we would need to move to KMS at some
point anyway, even if the device can't be controlled. I don't think writting
such a KMS driver would be that difficult.
--
Regards,

Laurent Pinchart
Tomasz Figa
2013-04-29 21:31:30 UTC
Permalink
Post by Laurent Pinchart
Hi Tomasz,
Post by Tomasz Figa
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be
rendered to, with the assumption that the display hardware has
already been set up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the
display hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware.
Could
it be generalised in some way so that the small amount of setup info
could be provided by other means (eg, module_param) or does the
dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer
driver if it helps deprecating fbdev in the long term, but I don't
want it to offer an excuse not to implement a DRM/KMS driver. In
particular adding DT bindings would force us to keep supporting the
ABI for a (too) long time.
Well, there is also at least one legitimate use case for this driver.
I believe there exist embedded devices on which there is no need to
dynamically control the framebuffer. It needs one time initialization,
usually in bootloader, and then it is used as is, using constant
parameters as long as the system is running.
I doubt there is a need for any KMS (or any other control) driver for
such devices - dumb framebuffer driver would be everything needed in
such case.
As we want to deprecate the fbdev API we would need to move to KMS at
some point anyway, even if the device can't be controlled. I don't
think writting such a KMS driver would be that difficult.
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?

Best regards,
Tomasz
Laurent Pinchart
2013-04-29 21:40:57 UTC
Permalink
Post by Tomasz Figa
Post by Laurent Pinchart
Post by Tomasz Figa
Post by Laurent Pinchart
Post by Andrew Morton
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be
rendered to, with the assumption that the display hardware has
already been set up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up
the display hardware, but a Linux driver doesn't yet exist for the
display hardware.
...
+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
It's sad that this simple little thing requires Open Firmware.
Could it be generalised in some way so that the small amount of
setup info could be provided by other means (eg, module_param) or
does the dependency go deeper than that?
I second that request. I like the idea of a simple framebuffer
driver if it helps deprecating fbdev in the long term, but I don't
want it to offer an excuse not to implement a DRM/KMS driver. In
particular adding DT bindings would force us to keep supporting the
ABI for a (too) long time.
Well, there is also at least one legitimate use case for this driver.
I believe there exist embedded devices on which there is no need to
dynamically control the framebuffer. It needs one time initialization,
usually in bootloader, and then it is used as is, using constant
parameters as long as the system is running.
I doubt there is a need for any KMS (or any other control) driver for
such devices - dumb framebuffer driver would be everything needed in
such case.
As we want to deprecate the fbdev API we would need to move to KMS at
some point anyway, even if the device can't be controlled. I don't
think writting such a KMS driver would be that difficult.
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use case it
will work fine.
--
Regards,

Laurent Pinchart
Arnd Bergmann
2013-04-29 22:04:20 UTC
Permalink
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use case it
will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong
in his attempt, or the KMS interface isn't as good as it should be
if we want to move people away from frame buffer drivers.

Arnd
Laurent Pinchart
2013-04-29 22:23:31 UTC
Permalink
Hi Arnd,
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use
case it will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong in his
attempt, or the KMS interface isn't as good as it should be if we want to
move people away from frame buffer drivers.
Implementing a KMS driver requires a fair amount of code. The DRM/KMS
subsystem provides helpers that significantly reduce the driver size. Those
helpers have been designed around common use cases found in existing KMS
drivers.

I'm pretty sure we will be able to add useful helpers for existing fbdev
drivers that will make porting them to KMS pretty easy, but that first
requires starting to port drivers to find out what common code could be
factored out.
--
Regards,

Laurent Pinchart
Olof Johansson
2013-04-29 22:40:44 UTC
Permalink
On Mon, Apr 29, 2013 at 3:23 PM, Laurent Pinchart
Post by Laurent Pinchart
Hi Arnd,
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use
case it will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong in his
attempt, or the KMS interface isn't as good as it should be if we want to
move people away from frame buffer drivers.
Implementing a KMS driver requires a fair amount of code. The DRM/KMS
subsystem provides helpers that significantly reduce the driver size. Those
helpers have been designed around common use cases found in existing KMS
drivers.
I'm pretty sure we will be able to add useful helpers for existing fbdev
drivers that will make porting them to KMS pretty easy, but that first
requires starting to port drivers to find out what common code could be
factored out.
Meanwhile this tiny driver will allow us to use hardware that can't
otherwise be used (because the proper graphics drivers for said
hardware is _also_ stuck behind large reworks, i.e. common display
framework, which has uncertain progress at this time).

As Stephen mentioned in the original patch, this particular driver is
very useful for Raspberry Pi. But it is also the best way forward
(right now) for getting basic upstream usability out of the Samsung
ARM-based Chromebook. As a matter of fact, as of this weekend
linux-next boots on that platform with keyboard, trackpad, framebuffer
and USB2.0 without a single out-of-tree patch. Getting the same
functionality out of 3.10 would be very desirable.

So, I clearly understand the desire to move over to a more modern
framework. At the same time, there's definite value in merging this
small driver while that's happening. Holding useful code hostage isn't
always very productive.


-Olof
Laurent Pinchart
2013-04-30 09:50:38 UTC
Permalink
Hi Olof,
Post by Olof Johansson
Post by Laurent Pinchart
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes
out of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use
case it will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong in
his attempt, or the KMS interface isn't as good as it should be if we
want to move people away from frame buffer drivers.
Implementing a KMS driver requires a fair amount of code. The DRM/KMS
subsystem provides helpers that significantly reduce the driver size.
Those helpers have been designed around common use cases found in existing
KMS drivers.
I'm pretty sure we will be able to add useful helpers for existing fbdev
drivers that will make porting them to KMS pretty easy, but that first
requires starting to port drivers to find out what common code could be
factored out.
Meanwhile this tiny driver will allow us to use hardware that can't
otherwise be used (because the proper graphics drivers for said hardware is
_also_ stuck behind large reworks, i.e. common display framework, which has
uncertain progress at this time).
Point taken :-)
Post by Olof Johansson
As Stephen mentioned in the original patch, this particular driver is very
useful for Raspberry Pi. But it is also the best way forward (right now) for
getting basic upstream usability out of the Samsung ARM-based Chromebook. As
a matter of fact, as of this weekend linux-next boots on that platform with
keyboard, trackpad, framebuffer and USB2.0 without a single out-of-tree
patch. Getting the same functionality out of 3.10 would be very desirable.
So, I clearly understand the desire to move over to a more modern framework.
At the same time, there's definite value in merging this small driver while
that's happening. Holding useful code hostage isn't always very productive.
I want to be clear about this, even though I believe we should put as much
effort as possible behind KMS drivers, I won't nack this patch. It has real
use cases and is useful, I'm just concerned it will get abused (but it's far
from being the only piece of code that could be subject to abuse).
--
Regards,

Laurent Pinchart
Stephen Warren
2013-05-02 18:25:07 UTC
Permalink
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use case it
will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong
in his attempt, or the KMS interface isn't as good as it should be
if we want to move people away from frame buffer drivers.
Well, I didn't actually attempt to write the KMS driver; I simply took a
look at existing KMS drivers (and perhaps some stub KMS driver; I forget
right now) to see what it'd take, and it looked quite scary.

The other issue is that the KMS semantics appear to desire that the
driver allocate FB memory from some pool, and then point the display
scanout at the allocated memory. However, this driver's semantics are
that some other entity has allocated and reserved some memory region for
scanout, and the simple FB driver exists solely to scribble to that
memory region. Rob Clark said the thought this could be handled by
writing a custom memory allocator to support this, but it seemed a
little pointless to write a whole memory allocator when the existing FB
interface allows the driver to just set a struct member to the address
and be done with it.
Geert Uytterhoeven
2013-05-02 18:35:45 UTC
Permalink
Post by Stephen Warren
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use case it
will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong
in his attempt, or the KMS interface isn't as good as it should be
if we want to move people away from frame buffer drivers.
Well, I didn't actually attempt to write the KMS driver; I simply took a
look at existing KMS drivers (and perhaps some stub KMS driver; I forget
right now) to see what it'd take, and it looked quite scary.
The other issue is that the KMS semantics appear to desire that the
driver allocate FB memory from some pool, and then point the display
scanout at the allocated memory. However, this driver's semantics are
that some other entity has allocated and reserved some memory region for
scanout, and the simple FB driver exists solely to scribble to that
memory region. Rob Clark said the thought this could be handled by
writing a custom memory allocator to support this, but it seemed a
little pointless to write a whole memory allocator when the existing FB
interface allows the driver to just set a struct member to the address
and be done with it.
I'm also curious to see the code for the first real simplekms/vesakms/ofkms/...
driver...

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ***@linux-m68k.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Laurent Pinchart
2013-05-03 10:06:24 UTC
Permalink
Hi Stephen,
Post by Stephen Warren
Post by Arnd Bergmann
Post by Laurent Pinchart
Post by Tomasz Figa
Good point. Stephen, would it be a problem to make this a KMS driver
instead? Old fbdev API could be emulated on top of it, until it goes out
of use, couldn't it?
There's already an fbdev emulation layer in KMS, for such a simple use
case it will work fine.
I suggested the same to Stephen when he first brought up this driver.
Unfortunately his attempt to create a simple KMS driver resulted in a
significantly larger driver than the one he did for the framebuffer
interface. This means that either Stephen did something really wrong
in his attempt, or the KMS interface isn't as good as it should be
if we want to move people away from frame buffer drivers.
Well, I didn't actually attempt to write the KMS driver; I simply took a
look at existing KMS drivers (and perhaps some stub KMS driver; I forget
right now) to see what it'd take, and it looked quite scary.
The other issue is that the KMS semantics appear to desire that the
driver allocate FB memory from some pool, and then point the display
scanout at the allocated memory. However, this driver's semantics are
that some other entity has allocated and reserved some memory region for
scanout, and the simple FB driver exists solely to scribble to that
memory region. Rob Clark said the thought this could be handled by
writing a custom memory allocator to support this, but it seemed a
little pointless to write a whole memory allocator when the existing FB
interface allows the driver to just set a struct member to the address
and be done with it.
KMS handles frame buffers through two objects, a GEM object that represents a
piece of memory and a frame buffer object that represents, well, a frame
buffer :-)

GEM objects management requires an allocator, and frame buffers reference one
or more GEM objects to model the frame buffer memory. As allocating new GEM
objects isn't possible in this case, the driver could create a GEM object and
a frame buffer at initialization time, and implement GEM allocation stubs that
would simply return an error unconditionally.
--
Regards,

Laurent Pinchart
Andrew Morton
2013-05-07 21:33:38 UTC
Permalink
We don't seem to have a well-defined path to travel here, and I don't
get the feeling that anyone has signed up to walk it?

So I'm inclined to merge Stephen's patch as-is into 3.10. It's pretty
simple and standalone. Any strenuous objections?

BTW, Olof told me off-list that this patch is a "critical piece" for
running mainline Linux on ARM Chromebooks. That was important
information - more important than anything else we were told about this
patch!

So Stephen, could I please have a new changelog for this patch which
explains this application, and any other interesting/important things
which were left out? Quickly, please.




From: Stephen Warren <swarren-3lzwWm7+***@public.gmane.org>
Subject: drivers/video: implement a simple framebuffer driver

A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set up
to scan out from that buffer.

This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.

[akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+***@public.gmane.org: make simplefb_formats[] static]
Signed-off-by: Stephen Warren <swarren-3lzwWm7+***@public.gmane.org>
Cc: Arnd Bergmann <arnd-***@public.gmane.org>
Cc: Olof Johansson <olof-***@public.gmane.org>
Cc: Rob Clark <robclark-***@public.gmane.org>
Cc: Florian Tobias Schandinat <FlorianSchandinat-***@public.gmane.org>
Cc: Tomasz Figa <tomasz.figa-***@public.gmane.org>
Cc: Laurent Pinchart <laurent.pinchart-ryLnwIuWjnjg/***@public.gmane.org>
Signed-off-by: Andrew Morton <akpm-de/tnXTf+JLsfHDXvbKv3WD2FQJk+8+***@public.gmane.org>
---

Documentation/devicetree/bindings/video/simple-framebuffer.txt | 25 +
drivers/video/Kconfig | 17
drivers/video/Makefile | 1
drivers/video/simplefb.c | 234 ++++++++++
4 files changed, 277 insertions(+)

diff -puN /dev/null Documentation/devicetree/bindings/video/simple-framebuffer.txt
--- /dev/null
+++ a/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -0,0 +1,25 @@
+Simple Framebuffer
+
+A simple frame-buffer describes a raw memory region that may be rendered to,
+with the assumption that the display hardware has already been set up to scan
+out from that buffer.
+
+Required properties:
+- compatible: "simple-framebuffer"
+- reg: Should contain the location and size of the framebuffer memory.
+- width: The width of the framebuffer in pixels.
+- height: The height of the framebuffer in pixels.
+- stride: The number of bytes in each line of the framebuffer.
+- format: The format of the framebuffer surface. Valid values are:
+ - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+
+Example:
+
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
diff -puN drivers/video/Kconfig~drivers-video-implement-a-simple-framebuffer-driver drivers/video/Kconfig
--- a/drivers/video/Kconfig~drivers-video-implement-a-simple-framebuffer-driver
+++ a/drivers/video/Kconfig
@@ -2453,6 +2453,23 @@ config FB_HYPERV
help
This framebuffer driver supports Microsoft Hyper-V Synthetic Video.

+config FB_SIMPLE
+ bool "Simple framebuffer support"
+ depends on (FB = y) && OF
+ select FB_CFB_FILLRECT
+ select FB_CFB_COPYAREA
+ select FB_CFB_IMAGEBLIT
+ help
+ Say Y if you want support for a simple frame-buffer.
+
+ This driver assumes that the display hardware has been initialized
+ before the kernel boots, and the kernel will simply render to the
+ pre-allocated frame buffer surface.
+
+ Configuration re: surface address, size, and format must be provided
+ through device tree, or potentially plain old platform data in the
+ future.
+
source "drivers/video/omap/Kconfig"
source "drivers/video/omap2/Kconfig"
source "drivers/video/exynos/Kconfig"
diff -puN drivers/video/Makefile~drivers-video-implement-a-simple-framebuffer-driver drivers/video/Makefile
--- a/drivers/video/Makefile~drivers-video-implement-a-simple-framebuffer-driver
+++ a/drivers/video/Makefile
@@ -166,6 +166,7 @@ obj-$(CONFIG_FB_MX3) += mx3fb.o
obj-$(CONFIG_FB_DA8XX) += da8xx-fb.o
obj-$(CONFIG_FB_MXS) += mxsfb.o
obj-$(CONFIG_FB_SSD1307) += ssd1307fb.o
+obj-$(CONFIG_FB_SIMPLE) += simplefb.o

# the test framebuffer is last
obj-$(CONFIG_FB_VIRTUAL) += vfb.o
diff -puN /dev/null drivers/video/simplefb.c
--- /dev/null
+++ a/drivers/video/simplefb.c
@@ -0,0 +1,234 @@
+/*
+ * Simplest possible simple frame-buffer driver, as a platform device
+ *
+ * Copyright (c) 2013, Stephen Warren
+ *
+ * Based on q40fb.c, which was:
+ * Copyright (C) 2001 Richard Zidlicky <rz-***@public.gmane.org>
+ *
+ * Also based on offb.c, which was:
+ * Copyright (C) 1997 Geert Uytterhoeven
+ * Copyright (C) 1996 Paul Mackerras
+ *
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms and conditions of the GNU General Public License,
+ * version 2, as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope it will be useful, but WITHOUT
+ * ANY WARRANTY; without even the implied warranty of MERCHANTABILITY or
+ * FITNESS FOR A PARTICULAR PURPOSE. See the GNU General Public License for
+ * more details.
+ */
+
+#include <linux/errno.h>
+#include <linux/fb.h>
+#include <linux/io.h>
+#include <linux/module.h>
+#include <linux/platform_device.h>
+
+static struct fb_fix_screeninfo simplefb_fix = {
+ .id = "simple",
+ .type = FB_TYPE_PACKED_PIXELS,
+ .visual = FB_VISUAL_TRUECOLOR,
+ .accel = FB_ACCEL_NONE,
+};
+
+static struct fb_var_screeninfo simplefb_var = {
+ .height = -1,
+ .width = -1,
+ .activate = FB_ACTIVATE_NOW,
+ .vmode = FB_VMODE_NONINTERLACED,
+};
+
+static int simplefb_setcolreg(u_int regno, u_int red, u_int green, u_int blue,
+ u_int transp, struct fb_info *info)
+{
+ u32 *pal = info->pseudo_palette;
+ u32 cr = red >> (16 - info->var.red.length);
+ u32 cg = green >> (16 - info->var.green.length);
+ u32 cb = blue >> (16 - info->var.blue.length);
+ u32 value;
+
+ if (regno >= 16)
+ return -EINVAL;
+
+ value = (cr << info->var.red.offset) |
+ (cg << info->var.green.offset) |
+ (cb << info->var.blue.offset);
+ if (info->var.transp.length > 0) {
+ u32 mask = (1 << info->var.transp.length) - 1;
+ mask <<= info->var.transp.offset;
+ value |= mask;
+ }
+ pal[regno] = value;
+
+ return 0;
+}
+
+static struct fb_ops simplefb_ops = {
+ .owner = THIS_MODULE,
+ .fb_setcolreg = simplefb_setcolreg,
+ .fb_fillrect = cfb_fillrect,
+ .fb_copyarea = cfb_copyarea,
+ .fb_imageblit = cfb_imageblit,
+};
+
+struct simplefb_format {
+ const char *name;
+ u32 bits_per_pixel;
+ struct fb_bitfield red;
+ struct fb_bitfield green;
+ struct fb_bitfield blue;
+ struct fb_bitfield transp;
+};
+
+static struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
+
+struct simplefb_params {
+ u32 width;
+ u32 height;
+ u32 stride;
+ struct simplefb_format *format;
+};
+
+static int simplefb_parse_dt(struct platform_device *pdev,
+ struct simplefb_params *params)
+{
+ struct device_node *np = pdev->dev.of_node;
+ int ret;
+ const char *format;
+ int i;
+
+ ret = of_property_read_u32(np, "width", &params->width);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse width property\n");
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "height", &params->height);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse height property\n");
+ return ret;
+ }
+
+ ret = of_property_read_u32(np, "stride", &params->stride);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse stride property\n");
+ return ret;
+ }
+
+ ret = of_property_read_string(np, "format", &format);
+ if (ret) {
+ dev_err(&pdev->dev, "Can't parse format property\n");
+ return ret;
+ }
+ params->format = NULL;
+ for (i = 0; i < ARRAY_SIZE(simplefb_formats); i++) {
+ if (strcmp(format, simplefb_formats[i].name))
+ continue;
+ params->format = &simplefb_formats[i];
+ break;
+ }
+ if (!params->format) {
+ dev_err(&pdev->dev, "Invalid format value\n");
+ return -EINVAL;
+ }
+
+ return 0;
+}
+
+static int simplefb_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct simplefb_params params;
+ struct fb_info *info;
+ struct resource *mem;
+
+ if (fb_get_options("simplefb", NULL))
+ return -ENODEV;
+
+ ret = simplefb_parse_dt(pdev, &params);
+ if (ret)
+ return ret;
+
+ mem = platform_get_resource(pdev, IORESOURCE_MEM, 0);
+ if (!mem) {
+ dev_err(&pdev->dev, "No memory resource\n");
+ return -EINVAL;
+ }
+
+ info = framebuffer_alloc(sizeof(u32) * 16, &pdev->dev);
+ if (!info)
+ return -ENOMEM;
+ platform_set_drvdata(pdev, info);
+
+ info->fix = simplefb_fix;
+ info->fix.smem_start = mem->start;
+ info->fix.smem_len = resource_size(mem);
+ info->fix.line_length = params.stride;
+
+ info->var = simplefb_var;
+ info->var.xres = params.width;
+ info->var.yres = params.height;
+ info->var.xres_virtual = params.width;
+ info->var.yres_virtual = params.height;
+ info->var.bits_per_pixel = params.format->bits_per_pixel;
+ info->var.red = params.format->red;
+ info->var.green = params.format->green;
+ info->var.blue = params.format->blue;
+ info->var.transp = params.format->transp;
+
+ info->fbops = &simplefb_ops;
+ info->flags = FBINFO_DEFAULT;
+ info->screen_base = devm_ioremap(&pdev->dev, info->fix.smem_start,
+ info->fix.smem_len);
+ if (!info->screen_base) {
+ framebuffer_release(info);
+ return -ENODEV;
+ }
+ info->pseudo_palette = (void *)(info + 1);
+
+ ret = register_framebuffer(info);
+ if (ret < 0) {
+ dev_err(&pdev->dev, "Unable to register simplefb: %d\n", ret);
+ framebuffer_release(info);
+ return ret;
+ }
+
+ dev_info(&pdev->dev, "fb%d: simplefb registered!\n", info->node);
+
+ return 0;
+}
+
+static int simplefb_remove(struct platform_device *pdev)
+{
+ struct fb_info *info = platform_get_drvdata(pdev);
+
+ unregister_framebuffer(info);
+ framebuffer_release(info);
+
+ return 0;
+}
+
+static const struct of_device_id simplefb_of_match[] = {
+ { .compatible = "simple-framebuffer", },
+ { },
+};
+MODULE_DEVICE_TABLE(of, simplefb_of_match);
+
+static struct platform_driver simplefb_driver = {
+ .driver = {
+ .name = "simple-framebuffer",
+ .owner = THIS_MODULE,
+ .of_match_table = simplefb_of_match,
+ },
+ .probe = simplefb_probe,
+ .remove = simplefb_remove,
+};
+module_platform_driver(simplefb_driver);
+
+MODULE_AUTHOR("Stephen Warren <swarren-3lzwWm7+***@public.gmane.org>");
+MODULE_DESCRIPTION("Simple framebuffer driver");
+MODULE_LICENSE("GPL v2");
_
Stephen Warren
2013-05-08 02:36:38 UTC
Permalink
Post by Andrew Morton
We don't seem to have a well-defined path to travel here, and I don't
get the feeling that anyone has signed up to walk it?
So I'm inclined to merge Stephen's patch as-is into 3.10. It's pretty
simple and standalone. Any strenuous objections?
BTW, Olof told me off-list that this patch is a "critical piece" for
running mainline Linux on ARM Chromebooks. That was important
information - more important than anything else we were told about this
patch!
So Stephen, could I please have a new changelog for this patch which
explains this application, and any other interesting/important things
which were left out? Quickly, please.
OK, how about the version below:

----------
Subject: drivers/video: implement a simple framebuffer driver

A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.

This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.

Examples use-cases include:

* The built-in LCD panels on the Samsung ARM chromebook, and Tegra
devices, and likely many other ARM or embedded systems. These cannot yet
be supported using a full graphics driver, since the panel control
should be provided by the CDF (Common Display Framework), which has been
stuck in design/review for quite some time. One could support these
panels using custom SoC-specific code, but there is a desire to use
common infra-structure rather than having each SoC vendor invent their
own code, hence the desire to wait for CDF.

* Hardware for which a full graphics driver is not yet available, and
the path to obtain one upstream isn't yet clear. For example, the
Raspberry Pi.

* Any hardware in early stages of upstreaming, before a full graphics
driver has been tackled. This driver can provide a graphical boot
console (even full X support) much earlier in the upstreaming process,
thus making new SoC or board support more generally useful earlier.
----------

I assume you can just edit the patch you have and don't need me to
resend with the adjusted description.
Olof Johansson
2013-05-08 19:28:28 UTC
Permalink
Post by Stephen Warren
Post by Andrew Morton
We don't seem to have a well-defined path to travel here, and I don't
get the feeling that anyone has signed up to walk it?
So I'm inclined to merge Stephen's patch as-is into 3.10. It's pretty
simple and standalone. Any strenuous objections?
BTW, Olof told me off-list that this patch is a "critical piece" for
running mainline Linux on ARM Chromebooks. That was important
information - more important than anything else we were told about this
patch!
So Stephen, could I please have a new changelog for this patch which
explains this application, and any other interesting/important things
which were left out? Quickly, please.
----------
Subject: drivers/video: implement a simple framebuffer driver
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
* The built-in LCD panels on the Samsung ARM chromebook, and Tegra
devices, and likely many other ARM or embedded systems. These cannot yet
be supported using a full graphics driver, since the panel control
should be provided by the CDF (Common Display Framework), which has been
stuck in design/review for quite some time. One could support these
panels using custom SoC-specific code, but there is a desire to use
common infra-structure rather than having each SoC vendor invent their
own code, hence the desire to wait for CDF.
* Hardware for which a full graphics driver is not yet available, and
the path to obtain one upstream isn't yet clear. For example, the
Raspberry Pi.
* Any hardware in early stages of upstreaming, before a full graphics
driver has been tackled. This driver can provide a graphical boot
console (even full X support) much earlier in the upstreaming process,
thus making new SoC or board support more generally useful earlier.
----------
I assume you can just edit the patch you have and don't need me to
resend with the adjusted description.
Oh, looks like it never got a:

Acked-by: Olof Johansson <olof-***@public.gmane.org>

Either. Feel free to add, Andrew.


-Olof
Rob Landley
2013-05-08 20:58:30 UTC
Permalink
...
Post by Andrew Morton
Subject: drivers/video: implement a simple framebuffer driver
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
Virtual environments such as QEMU.

Rob
Tomi Valkeinen
2013-04-30 07:28:35 UTC
Permalink
Post by Tomasz Figa
Well, there is also at least one legitimate use case for this driver.
I believe there exist embedded devices on which there is no need to
dynamically control the framebuffer. It needs one time initialization,
usually in bootloader, and then it is used as is, using constant
parameters as long as the system is running.
I doubt there is a need for any KMS (or any other control) driver for such
devices - dumb framebuffer driver would be everything needed in such case.
Isn't there usually the need to power off the hardware when
blanking/suspending?

Tomi
Geert Uytterhoeven
2013-04-11 10:42:29 UTC
Permalink
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
Why "r5g6b5" instead of "rgb565", which is what's commonly used?

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert-***@public.gmane.org

In personal conversations with technical people, I call myself a hacker. But
when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds
Stephen Warren
2013-04-11 16:10:09 UTC
Permalink
Post by Geert Uytterhoeven
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
Why "r5g6b5" instead of "rgb565", which is what's commonly used?
Both representations appear commonly used.

I mentioned my rationale in response to V1: "r5g6b5" is a much more
well-defined structure. It's directly algorithmically parse-able if
required, whereas you'd need somewhat more complex heuristics to
determine exactly what rgb565 or argb2101010 mean, since all the numbers
are run together without delimiters.
Tomi Valkeinen
2013-04-30 07:27:01 UTC
Permalink
Hi,
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
I had an idea related to this driver. There are two requirements that I
believe are (or will be) quite common:

On embedded devices quite often the bootloader initializes the display,
with the purpose of having a valid image on the screen for the whole
boot up process (company logo, or whatever).

With multi-arch kernels, we'll have display drivers for all platforms
and panels compiled with the kernel. If all these are built-in, they
will possibly increase the kernel size quite a bit, so it would be good
to have the display drivers as modules.

Now, if we have the display drivers as modules, the point when the
kernel/userspace can update the screen will be pretty late. Also,
there's no chance to get any early kernel boot messages on the screen.

So how about if we had this kind of simple fb, built-in to the kernel,
used only for the boot time until the proper driver is loaded. A "bootfb".

The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.

Tomi
Arnd Bergmann
2013-04-30 10:28:42 UTC
Permalink
Post by Tomi Valkeinen
The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.
I think that's a great idea. What I'm not sure about is how that
infrastructure for switching frame buffers would work and how hard
it's to write.

Arnd
Laurent Pinchart
2013-04-30 11:42:50 UTC
Permalink
Post by Arnd Bergmann
Post by Tomi Valkeinen
The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.
This could be done with a KMS driver as well ;-)
Post by Arnd Bergmann
I think that's a great idea. What I'm not sure about is how that
infrastructure for switching frame buffers would work and how hard it's to
write.
--
Regards,

Laurent Pinchart
Tomi Valkeinen
2013-04-30 11:48:08 UTC
Permalink
Post by Laurent Pinchart
Post by Tomi Valkeinen
The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.
This could be done with a KMS driver as well ;-)
Right, I forgot to mention that. I had it in mind also =).

DRM has the same problem as fbdev with multi-arch and lots of display
and panel drivers.

Probably the same bootfb could be used for DRM also. Or do you see any
benefit of having a "bootkms" driver, or such?

Tomi
Laurent Pinchart
2013-04-30 11:49:28 UTC
Permalink
Post by Tomi Valkeinen
Post by Laurent Pinchart
Post by Tomi Valkeinen
The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.
This could be done with a KMS driver as well ;-)
Right, I forgot to mention that. I had it in mind also =).
DRM has the same problem as fbdev with multi-arch and lots of display
and panel drivers.
Probably the same bootfb could be used for DRM also. Or do you see any
benefit of having a "bootkms" driver, or such?
As part of the (long term) effort to deprecate fbdev I would of course prefer
a bootkms driver. The KMS fbdev emulation layer would provide fbdev
compatibility for free during the transition.
--
Regards,

Laurent Pinchart
Tomi Valkeinen
2013-04-30 11:46:20 UTC
Permalink
Post by Arnd Bergmann
Post by Tomi Valkeinen
The bootloader would init the display hardware, the bootfb would give an
early /dev/fb0 for the kernel and userspace, and when the real display
driver is loaded, the bootfb would be unbound and the real driver would
take over.
I think that's a great idea. What I'm not sure about is how that
infrastructure for switching frame buffers would work and how hard
it's to write.
I don't have any clear ideas either, just some vague ones:

I think the simplest option would be for the userspace to just unbind
the fbcon, then the bootfb device/driver, and then load the real driver.
I don't see why that would not work, but it's far from optimal. The fb
memory would become unallocated for a while, and the user could see
garbage on the display.

A proper hand-over would be more complex. So, I don't know... Maybe the
bootfb driver could have custom API for this. When the real driver is
loaded, it'd call the bootfb to get the fb memory, and to unbind the bootfb.

Would there be issues with passing the fb memory? If one uses dma_alloc,
the device is linked to the allocated memory, so I presume you can't
just pass that around and remove the original device.

Then again, dma_alloc would not be used here, as bootfb needs the fb
memory from a particular location, so I guess bootmem is needed here.

I think it would be reasonable to have a restriction of only a single
bootfb instance, so one could just use static global variables/funcs to
manage the hand-over.

All this, of course, presumes that nobody else than fbcon is using the
fb. But I think it's also a reasonable restriction that the fb device is
not used (mmapped) by anyone.

Tomi
Dave Airlie
2013-05-03 05:40:25 UTC
Permalink
Post by Tomi Valkeinen
I think it would be reasonable to have a restriction of only a single
bootfb instance, so one could just use static global variables/funcs to
manage the hand-over.
All this, of course, presumes that nobody else than fbcon is using the
fb. But I think it's also a reasonable restriction that the fb device is
not used (mmapped) by anyone.
We already have this on x86, though the handover isn't great,

FBINFO_MISC_FIRMWARE flag, and the aperture tracking stuff.

Dave.
Tomi Valkeinen
2013-04-30 07:39:43 UTC
Permalink
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
---
v2: s/dumb/simple/ throughout. Provide more details on pixel format.
* DRM/KMS look much more complex, and don't provide any benefit that I can
tell for this simple driver.
* Creating a separate driver rather than adjusting offb.c to work allows a
new clean binding to be defined, and doesn't require removing or ifdefing
PPC-isms in offb.c.
---
.../bindings/video/simple-framebuffer.txt | 25 +++
drivers/video/Kconfig | 17 ++
drivers/video/Makefile | 1 +
drivers/video/simplefb.c | 234 ++++++++++++++++++++
4 files changed, 277 insertions(+)
create mode 100644 Documentation/devicetree/bindings/video/simple-framebuffer.txt
create mode 100644 drivers/video/simplefb.c
diff --git a/Documentation/devicetree/bindings/video/simple-framebuffer.txt b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
new file mode 100644
index 0000000..3ea4605
--- /dev/null
+++ b/Documentation/devicetree/bindings/video/simple-framebuffer.txt
@@ -0,0 +1,25 @@
+Simple Framebuffer
+
+A simple frame-buffer describes a raw memory region that may be rendered to,
+with the assumption that the display hardware has already been set up to scan
+out from that buffer.
+
+- compatible: "simple-framebuffer"
+- reg: Should contain the location and size of the framebuffer memory.
+- width: The width of the framebuffer in pixels.
+- height: The height of the framebuffer in pixels.
+- stride: The number of bytes in each line of the framebuffer.
+ - r5g6b5 (16-bit pixels, d[15:11]=r, d[10:5]=g, d[4:0]=b).
+
+
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
I'm not an expert on DT, but I think the point of DT is to describe the
hardware. This doesn't describe the hardware at all.

Tomi
Arnd Bergmann
2013-04-30 10:34:16 UTC
Permalink
Post by Tomi Valkeinen
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
I'm not an expert on DT, but I think the point of DT is to describe the
hardware. This doesn't describe the hardware at all.
That's ok.

It's not uncommon to have settings in the device tree that describe how
hardware is set up. Other similar properties would be the line rate
of a serial port, or a keymap describing what each button is labeled.
They are not physical properties, but they are necessary platform specific
pieces of information that are not available otherwise.

Arnd
Alexander Shiyan
2013-04-30 14:38:05 UTC
Permalink
Post by Tomi Valkeinen
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
Post by Tomi Valkeinen
Post by Stephen Warren
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
I'm not an expert on DT, but I think the point of DT is to describe the
hardware. This doesn't describe the hardware at all.
On my incompetent opinion, "bpp" parameter can replace size of
framebuffer and "stride". Is not it?

---
Arnd Bergmann
2013-04-30 15:07:57 UTC
Permalink
Post by Alexander Shiyan
Post by Tomi Valkeinen
Post by Stephen Warren
A simple frame-buffer describes a raw memory region that may be rendered
to, with the assumption that the display hardware has already been set
up to scan out from that buffer.
This is useful in cases where a bootloader exists and has set up the
display hardware, but a Linux driver doesn't yet exist for the display
hardware.
...
Post by Tomi Valkeinen
Post by Stephen Warren
+ framebuffer {
+ compatible = "simple-framebuffer";
+ reg = <0x1d385000 (1600 * 1200 * 2)>;
+ width = <1600>;
+ height = <1200>;
+ stride = <(1600 * 2)>;
+ format = "r5g6b5";
+ };
I'm not an expert on DT, but I think the point of DT is to describe the
hardware. This doesn't describe the hardware at all.
On my incompetent opinion, "bpp" parameter can replace size of
framebuffer and "stride". Is not it?
Not necessarily: you could have a stride that is larger than a line.

Arnd
Alexandre Courbot
2013-05-18 10:29:53 UTC
Permalink
Post by Stephen Warren
+struct simplefb_format {
+ const char *name;
+ u32 bits_per_pixel;
+ struct fb_bitfield red;
+ struct fb_bitfield green;
+ struct fb_bitfield blue;
+ struct fb_bitfield transp;
+};
+
+struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
I have been adding a few extra formats to this list, and I wonder if
this could not simply be turned into a function that would directly
convert the name string into the corresponding right format. The
mapping between name and format seems to be a 1:1 and this would
probably avoid errors in the future. I'm especially thinking about
color order here - I started adding a mode that reads

{ "r8g8b8a8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8} },

while it should probably be called "a8b8g8r8" as the order of colors
is not the same as your r5g6b5.

I can submit a patch if there is no issue with that idea.

Alex.
Stephen Warren
2013-05-20 15:25:46 UTC
Permalink
Post by Alexandre Courbot
Post by Stephen Warren
+struct simplefb_format {
+ const char *name;
+ u32 bits_per_pixel;
+ struct fb_bitfield red;
+ struct fb_bitfield green;
+ struct fb_bitfield blue;
+ struct fb_bitfield transp;
+};
+
+struct simplefb_format simplefb_formats[] = {
+ { "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
+};
I have been adding a few extra formats to this list, and I wonder if
this could not simply be turned into a function that would directly
convert the name string into the corresponding right format. The
mapping between name and format seems to be a 1:1 and this would
probably avoid errors in the future. I'm especially thinking about
color order here - I started adding a mode that reads
{ "r8g8b8a8", 32, {0, 8}, {8, 8}, {16, 8}, {24, 8} },
while it should probably be called "a8b8g8r8" as the order of colors
is not the same as your r5g6b5.
I can submit a patch if there is no issue with that idea.
I chose r5g6b5 rather than rgb565 specifically to make that format
trivially name machine-parsable. So, I'm not opposed to converting that
table to code. I'm not 100% sure if it's worth it or necessary by the
time we get to just 2 formats in the array, but I don't see any big
disadvantage, so why not. The DT binding documentation might want
enhancing with a more general description of how formats should be
represented if this is implemented.

Loading...