[PATCH V2] video: implement a simple framebuffer driver

[PATCH V2] video: implement a simple framebuffer driver

[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.

Signed-off-by: Stephen Warren <swarren@wwwdotorg.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@linux-m68k.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@wwwdotorg.org>");
+MODULE_DESCRIPTION("Simple framebuffer driver");
+MODULE_LICENSE("GPL v2");
-- 
1.7.10.4

Re: [PATCH V2] video: implement a simple framebuffer driver

[Andrew Morton]

On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote:

> 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?

> +struct simplefb_format simplefb_formats[] = {
> +	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> +};

I'll make this static.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 04/08/2013 06:16 PM, Andrew Morton wrote:
> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Geert Uytterhoeven]

On Tue, Apr 9, 2013 at 2:16 AM, Andrew Morton <akpm@linux-foundation.org> wrote:
> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren <swarren@wwwdotorg.org> wrote:
>> 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@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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> > 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.

> > +struct simplefb_format simplefb_formats[] = {
> > +	{ "r5g6b5", 16, {11, 5}, {5, 6}, {0, 5}, {0, 0} },
> > +};
> 
> I'll make this static.

-- 
Regards,

Laurent Pinchart

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 04/11/2013 03:56 AM, Laurent Pinchart wrote:
> On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
>> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
>>> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Stephen,

On Thursday 11 April 2013 10:06:49 Stephen Warren wrote:
> On 04/11/2013 03:56 AM, Laurent Pinchart wrote:
> > On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> >> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> >>> 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.

> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 04/11/2013 02:06 PM, Laurent Pinchart wrote:
> Hi Stephen,
> 
> On Thursday 11 April 2013 10:06:49 Stephen Warren wrote:
>> On 04/11/2013 03:56 AM, Laurent Pinchart wrote:
>>> On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
>>>> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
>>>>> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Stephen,

Sorry for the late reply.

On Thursday 11 April 2013 14:38:21 Stephen Warren wrote:
> On 04/11/2013 02:06 PM, Laurent Pinchart wrote:
> > On Thursday 11 April 2013 10:06:49 Stephen Warren wrote:
> >> On 04/11/2013 03:56 AM, Laurent Pinchart wrote:
> >>> On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> >>>> On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> >>>>> 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.

> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomasz Figa]

Hi Laurent,

On Thursday 11 of April 2013 11:56:31 Laurent Pinchart wrote:
> On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> > On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Tomasz,

On Monday 29 April 2013 23:15:13 Tomasz Figa wrote:
> On Thursday 11 of April 2013 11:56:31 Laurent Pinchart wrote:
> > On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> > > On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> > > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomasz Figa]

On Monday 29 of April 2013 23:20:43 Laurent Pinchart wrote:
> Hi Tomasz,
> 
> On Monday 29 April 2013 23:15:13 Tomasz Figa wrote:
> > On Thursday 11 of April 2013 11:56:31 Laurent Pinchart wrote:
> > > On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> > > > On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> > > > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
> On Monday 29 of April 2013 23:20:43 Laurent Pinchart wrote:
> > On Monday 29 April 2013 23:15:13 Tomasz Figa wrote:
> > > On Thursday 11 of April 2013 11:56:31 Laurent Pinchart wrote:
> > > > On Monday 08 April 2013 17:16:37 Andrew Morton wrote:
> > > > > On Wed,  3 Apr 2013 20:39:43 -0600 Stephen Warren wrote:
> > > > > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Arnd Bergmann]

On Monday 29 April 2013, Laurent Pinchart wrote:
> On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
>
> > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Arnd,

On Tuesday 30 April 2013 00:04:20 Arnd Bergmann wrote:
> On Monday 29 April 2013, Laurent Pinchart wrote:
> > On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
> > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Olof Johansson]

On Mon, Apr 29, 2013 at 3:23 PM, Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
> Hi Arnd,
>
> On Tuesday 30 April 2013 00:04:20 Arnd Bergmann wrote:
>> On Monday 29 April 2013, Laurent Pinchart wrote:
>> > On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
>> > > 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Olof,

On Monday 29 April 2013 15:40:44 Olof Johansson wrote:
> On Mon, Apr 29, 2013 at 3:23 PM, Laurent Pinchart wrote:
> > On Tuesday 30 April 2013 00:04:20 Arnd Bergmann wrote:
> >> On Monday 29 April 2013, Laurent Pinchart wrote:
> >> > On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
> >> > > 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 :-)

> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 04/29/2013 04:04 PM, Arnd Bergmann wrote:
> On Monday 29 April 2013, Laurent Pinchart wrote:
>> On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
>>
>>> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Geert Uytterhoeven]

On Thu, May 2, 2013 at 8:25 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 04/29/2013 04:04 PM, Arnd Bergmann wrote:
>> On Monday 29 April 2013, Laurent Pinchart wrote:
>>> On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
>>>
>>>> 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 -- geert@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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

Hi Stephen,

On Thursday 02 May 2013 12:25:07 Stephen Warren wrote:
> On 04/29/2013 04:04 PM, Arnd Bergmann wrote:
> > On Monday 29 April 2013, Laurent Pinchart wrote:
> >> On Monday 29 April 2013 23:31:30 Tomasz Figa wrote:
> >>> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[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.




From: Stephen Warren <swarren@wwwdotorg.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@linux-foundation.org: make simplefb_formats[] static]
Signed-off-by: Stephen Warren <swarren@wwwdotorg.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Olof Johansson <olof@lixom.net>
Cc: Rob Clark <robclark@gmail.com>
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Cc: Tomasz Figa <tomasz.figa@gmail.com>
Cc: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Signed-off-by: Andrew Morton <akpm@linux-foundation.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@linux-m68k.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@wwwdotorg.org>");
+MODULE_DESCRIPTION("Simple framebuffer driver");
+MODULE_LICENSE("GPL v2");
_

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 05/07/2013 03:33 PM, Andrew Morton wrote:
> 
> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Olof Johansson]

On Tue, May 7, 2013 at 7:36 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 05/07/2013 03:33 PM, Andrew Morton wrote:
>>
>> 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.

Oh, looks like it never got a:

Acked-by: Olof Johansson <olof@lixom.net>

Either. Feel free to add, Andrew.


-Olof

Re: [PATCH V2] video: implement a simple framebuffer driver

[Rob Landley]

On 05/07/2013 09:36:38 PM, Stephen Warren wrote:
> On 05/07/2013 03:33 PM, Andrew Morton wrote:
...
> 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:

Virtual environments such as QEMU.

Rob

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 646 bytes --]

On 04/30/2013 12:15 AM, Tomasz Figa wrote:

> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH V2] video: implement a simple framebuffer driver

[Geert Uytterhoeven]

On Thu, Apr 4, 2013 at 4:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> +       { "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@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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 04/11/2013 04:42 AM, Geert Uytterhoeven wrote:
> On Thu, Apr 4, 2013 at 4:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> +       { "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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomi Valkeinen]

Hi,

On 04/04/2013 05:39 AM, Stephen Warren wrote:
> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Arnd Bergmann]

On Tuesday 30 April 2013, Tomi Valkeinen wrote:

> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

On Tuesday 30 April 2013 12:28:42 Arnd Bergmann wrote:
> On Tuesday 30 April 2013, Tomi Valkeinen wrote:
> > 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 ;-)

> 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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 752 bytes --]

On 04/30/2013 02:42 PM, Laurent Pinchart wrote:
> On Tuesday 30 April 2013 12:28:42 Arnd Bergmann wrote:
>> On Tuesday 30 April 2013, Tomi Valkeinen wrote:
>>> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH V2] video: implement a simple framebuffer driver

[Laurent Pinchart]

[-- Attachment #1: Type: text/plain, Size: 1039 bytes --]

On Tuesday 30 April 2013 14:48:08 Tomi Valkeinen wrote:
> On 04/30/2013 02:42 PM, Laurent Pinchart wrote:
> > On Tuesday 30 April 2013 12:28:42 Arnd Bergmann wrote:
> >> On Tuesday 30 April 2013, Tomi Valkeinen wrote:
> >>> 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

[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 1808 bytes --]

On 04/30/2013 01:28 PM, Arnd Bergmann wrote:
> On Tuesday 30 April 2013, Tomi Valkeinen wrote:
> 
>> 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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH V2] video: implement a simple framebuffer driver

[Dave Airlie]

>
> 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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Tomi Valkeinen]

[-- Attachment #1: Type: text/plain, Size: 2694 bytes --]

On 04/04/2013 05:39 AM, Stephen Warren wrote:
> 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@wwwdotorg.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";
> +	};

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



[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]

Re: [PATCH V2] video: implement a simple framebuffer driver

[Arnd Bergmann]

On Tuesday 30 April 2013, Tomi Valkeinen wrote:
> > +     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

Re[2]: [PATCH V2] video: implement a simple f =?UTF-8?B?cmFtZWJ1ZmZlciBkc

[Alexander Shiyan]

PiBPbiAwNC8wNC8yMDEzIDA1OjM5IEFNLCBTdGVwaGVuIFdhcnJlbiB3cm90ZToKPiA+IEEgc2lt
cGxlIGZyYW1lLWJ1ZmZlciBkZXNjcmliZXMgYSByYXcgbWVtb3J5IHJlZ2lvbiB0aGF0IG1heSBi
ZSByZW5kZXJlZAo+ID4gdG8sIHdpdGggdGhlIGFzc3VtcHRpb24gdGhhdCB0aGUgZGlzcGxheSBo
YXJkd2FyZSBoYXMgYWxyZWFkeSBiZWVuIHNldAo+ID4gdXAgdG8gc2NhbiBvdXQgZnJvbSB0aGF0
IGJ1ZmZlci4KPiA+IAo+ID4gVGhpcyBpcyB1c2VmdWwgaW4gY2FzZXMgd2hlcmUgYSBib290bG9h
ZGVyIGV4aXN0cyBhbmQgaGFzIHNldCB1cCB0aGUKPiA+IGRpc3BsYXkgaGFyZHdhcmUsIGJ1dCBh
IExpbnV4IGRyaXZlciBkb2Vzbid0IHlldCBleGlzdCBmb3IgdGhlIGRpc3BsYXkKPiA+IGhhcmR3
YXJlLgo+ID4gCj4gPiBTaWduZWQtb2ZmLWJ5OiBTdGVwaGVuIFdhcnJlbiA8c3dhcnJlbkB3d3dk
b3Rvcmcub3JnPgouLi4KPiA+ICsJZnJhbWVidWZmZXIgewo+ID4gKwkJY29tcGF0aWJsZSA9ICJz
aW1wbGUtZnJhbWVidWZmZXIiOwo+ID4gKwkJcmVnID0gPDB4MWQzODUwMDAgKDE2MDAgKiAxMjAw
ICogMik+Owo+ID4gKwkJd2lkdGggPSA8MTYwMD47Cj4gPiArCQloZWlnaHQgPSA8MTIwMD47Cj4g
PiArCQlzdHJpZGUgPSA8KDE2MDAgKiAyKT47Cj4gPiArCQlmb3JtYXQgPSAicjVnNmI1IjsKPiA+
ICsJfTsKPiAKPiBJJ20gbm90IGFuIGV4cGVydCBvbiBEVCwgYnV0IEkgdGhpbmsgdGhlIHBvaW50
IG9mIERUIGlzIHRvIGRlc2NyaWJlIHRoZQo+IGhhcmR3YXJlLiBUaGlzIGRvZXNuJ3QgZGVzY3Jp
YmUgdGhlIGhhcmR3YXJlIGF0IGFsbC4KCk9uIG15IGluY29tcGV0ZW50IG9waW5pb24sICJicHAi
IHBhcmFtZXRlciBjYW4gcmVwbGFjZSBzaXplIG9mCmZyYW1lYnVmZmVyIGFuZCAic3RyaWRlIi4g
SXMgbm90IGl0PwoKLS0tCg=

Re: Re[2]: [PATCH V2] video: implement a simple framebuffer driver

[Arnd Bergmann]

On Tuesday 30 April 2013, Alexander Shiyan wrote:
> > On 04/04/2013 05:39 AM, Stephen Warren wrote:
> > > 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@wwwdotorg.org>
> ...
> > > +   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

Re: [PATCH V2] video: implement a simple framebuffer driver

[Alexandre Courbot]

On Thu, Apr 4, 2013 at 11:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> +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.

Re: [PATCH V2] video: implement a simple framebuffer driver

[Stephen Warren]

On 05/18/2013 04:29 AM, Alexandre Courbot wrote:
> On Thu, Apr 4, 2013 at 11:39 AM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> +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.