diff mbox

[RFC,2/6] x86: provide platform-devices for boot-framebuffers

Message ID 1372112849-670-3-git-send-email-dh.herrmann@gmail.com (mailing list archive)
State New, archived
Headers show

Commit Message

David Herrmann June 24, 2013, 10:27 p.m. UTC
The current situation regarding boot-framebuffers (VGA, VESA/VBE, EFI) on
x86 causes troubles when loading multiple fbdev drivers. The global
"struct screen_info" does not provide any state-tracking about which
drivers use the FBs. request_mem_region() theoretically works, but
unfortunately vesafb/efifb ignore it due to quirks for broken boards.

Avoid this by creating a "platform-framebuffer" device with a pointer
to the "struct screen_info" as platform-data. Drivers can now create
platform-drivers and the driver-core will refuse multiple drivers being
active simultaneously.

We keep the screen_info available for backwards-compatibility. Drivers
can be converted in follow-up patches.

Apart from "platform-framebuffer" devices, this also introduces a
compatibility option for "simple-framebuffer" drivers which recently got
introduced for OF based systems. If CONFIG_X86_SYSFB is selected, we
try to match the screen_info against a simple-framebuffer supported
format. If we succeed, we create a "simple-framebuffer" device instead
of a platform-framebuffer.
This allows to reuse the simplefb.c driver across architectures and also
to introduce a SimpleDRM driver. There is no need to have vesafb.c,
efifb.c, simplefb.c and more just to have architecture specific quirks
in their setup-routines.

Instead, we now move the architecture specific quirks into x86-setup and
provide a generic simple-framebuffer. For backwards-compatibility (if
strange formats are used), we still allow vesafb/efifb to be loaded
simultaneously and pick up all remaining devices.

Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 arch/x86/Kconfig         |  18 ++++++
 arch/x86/kernel/Makefile |   1 +
 arch/x86/kernel/sysfb.c  | 157 +++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 176 insertions(+)
 create mode 100644 arch/x86/kernel/sysfb.c

Comments

Stephen Warren June 26, 2013, 8:49 p.m. UTC | #1
On 06/24/2013 04:27 PM, David Herrmann wrote:
> The current situation regarding boot-framebuffers (VGA, VESA/VBE, EFI) on
> x86 causes troubles when loading multiple fbdev drivers. The global
> "struct screen_info" does not provide any state-tracking about which
> drivers use the FBs. request_mem_region() theoretically works, but
> unfortunately vesafb/efifb ignore it due to quirks for broken boards.
> 
> Avoid this by creating a "platform-framebuffer" device with a pointer
> to the "struct screen_info" as platform-data. Drivers can now create
> platform-drivers and the driver-core will refuse multiple drivers being
> active simultaneously.
> 
> We keep the screen_info available for backwards-compatibility. Drivers
> can be converted in follow-up patches.
> 
> Apart from "platform-framebuffer" devices, this also introduces a
> compatibility option for "simple-framebuffer" drivers which recently got
> introduced for OF based systems. If CONFIG_X86_SYSFB is selected, we
> try to match the screen_info against a simple-framebuffer supported
> format. If we succeed, we create a "simple-framebuffer" device instead
> of a platform-framebuffer.
> This allows to reuse the simplefb.c driver across architectures and also
> to introduce a SimpleDRM driver. There is no need to have vesafb.c,
> efifb.c, simplefb.c and more just to have architecture specific quirks
> in their setup-routines.
> 
> Instead, we now move the architecture specific quirks into x86-setup and
> provide a generic simple-framebuffer. For backwards-compatibility (if
> strange formats are used), we still allow vesafb/efifb to be loaded
> simultaneously and pick up all remaining devices.

> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig

> +config X86_SYSFB
> +	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +	help
> +	  Firmwares often provide initial graphics framebuffers so the BIOS,
> +	  bootloader or kernel can show basic video-output during boot for
> +	  user-guidance and debugging. Historically, x86 used the VESA BIOS
> +	  Extensions and EFI-framebuffers for this, which are mostly limited
> +	  to x86. However, a generic system-framebuffer initialization emerged
> +	  recently on some non-x86 architectures.

After this patch has been in the kernel a while, that very last won't
really be true; simplefb won't have been introduced recently. Perhaps
just delete that one sentence?

> +	  This option, if enabled, marks VGA/VBE/EFI framebuffers as generic
> +	  framebuffers so the new generic system-framebuffer drivers can be
> +	  used on x86.
> +
> +	  This breaks any x86-only driver like efifb, vesafb, uvesafb, which
> +	  will not work if this is selected.

Doesn't that imply that some form of conflicts or depends ! statement
should be added here?

> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile

> +obj-y					+= sysfb.o

I suspect that should be obj-$(CONFIG_X86_SYSFB) += sysfb.o.

> diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c

> +#ifdef CONFIG_X86_SYSFB

Rather than ifdef'ing the body of this file, why not create a dummy
static inline version of add_sysfb() and put that into a header file
that users include. There should be a header file to prototype the
function anyway. That way, you avoid all of the ifdefs and static inline
functions in this file.

> +static bool parse_mode(const struct screen_info *si,
> +		       struct simplefb_platform_data *mode)

> +			strlcpy(mode->format, f->name, sizeof(mode->format));

Per my comments about the type of mode->format, I think that could just be:

mode->format = f->name;

... since formats[] (i.e. f) isn't initdata.

> +#else /* CONFIG_X86_SYSFB */
> +
> +static bool parse_mode(const struct screen_info *si,
> +		       struct simplefb_platform_data *mode)
> +{
> +	return false;
> +}
> +
> +static int create_simplefb(const struct screen_info *si,
> +			   const struct simplefb_platform_data *mode)
> +{
> +	return -EINVAL;
> +}
> +
> +#endif /* CONFIG_X86_SYSFB */

Following on from my ifdef comment above, I believe those versions of
those functions will always cause add_sysfb() to return -ENODEV, so you
may as well provide a static inline for add_sysfb() instead.
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
David Herrmann June 28, 2013, 10:11 a.m. UTC | #2
Hi

On Wed, Jun 26, 2013 at 10:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
> On 06/24/2013 04:27 PM, David Herrmann wrote:
>> The current situation regarding boot-framebuffers (VGA, VESA/VBE, EFI) on
>> x86 causes troubles when loading multiple fbdev drivers. The global
>> "struct screen_info" does not provide any state-tracking about which
>> drivers use the FBs. request_mem_region() theoretically works, but
>> unfortunately vesafb/efifb ignore it due to quirks for broken boards.
>>
>> Avoid this by creating a "platform-framebuffer" device with a pointer
>> to the "struct screen_info" as platform-data. Drivers can now create
>> platform-drivers and the driver-core will refuse multiple drivers being
>> active simultaneously.
>>
>> We keep the screen_info available for backwards-compatibility. Drivers
>> can be converted in follow-up patches.
>>
>> Apart from "platform-framebuffer" devices, this also introduces a
>> compatibility option for "simple-framebuffer" drivers which recently got
>> introduced for OF based systems. If CONFIG_X86_SYSFB is selected, we
>> try to match the screen_info against a simple-framebuffer supported
>> format. If we succeed, we create a "simple-framebuffer" device instead
>> of a platform-framebuffer.
>> This allows to reuse the simplefb.c driver across architectures and also
>> to introduce a SimpleDRM driver. There is no need to have vesafb.c,
>> efifb.c, simplefb.c and more just to have architecture specific quirks
>> in their setup-routines.
>>
>> Instead, we now move the architecture specific quirks into x86-setup and
>> provide a generic simple-framebuffer. For backwards-compatibility (if
>> strange formats are used), we still allow vesafb/efifb to be loaded
>> simultaneously and pick up all remaining devices.
>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>
>> +config X86_SYSFB
>> +     bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
>> +     help
>> +       Firmwares often provide initial graphics framebuffers so the BIOS,
>> +       bootloader or kernel can show basic video-output during boot for
>> +       user-guidance and debugging. Historically, x86 used the VESA BIOS
>> +       Extensions and EFI-framebuffers for this, which are mostly limited
>> +       to x86. However, a generic system-framebuffer initialization emerged
>> +       recently on some non-x86 architectures.
>
> After this patch has been in the kernel a while, that very last won't
> really be true; simplefb won't have been introduced recently. Perhaps
> just delete that one sentence?

It rather belongs in the commit message, right. I will rephrase that.

>> +       This option, if enabled, marks VGA/VBE/EFI framebuffers as generic
>> +       framebuffers so the new generic system-framebuffer drivers can be
>> +       used on x86.
>> +
>> +       This breaks any x86-only driver like efifb, vesafb, uvesafb, which
>> +       will not work if this is selected.
>
> Doesn't that imply that some form of conflicts or depends ! statement
> should be added here?

There is no real conflict here. You still can use vesafb/... with this
option, but they will not pick up the device. I intend to fix these up
to use "platform-framebuffer" devices instead of globally binding to
"struct screen_info". This way, framebuffers either end up as
simple-framebuffers or platform-framebuffers. This option selects
which device they end up as.

As all non-compatible framebuffers (with incompatible pixel-formats)
always end up as "platform-framebuffer", it still makes sense to use
vesafb as fallback. Hence, I'd not introduce any "conflicts"
dependency here.
Maybe I should rephrase the warning to something that makes clear that
if this option is selected, you need simplefb.c or simpledrm to make
use of these devices.

>> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
>
>> +obj-y                                        += sysfb.o
>
> I suspect that should be obj-$(CONFIG_X86_SYSFB) += sysfb.o.

No. This patch tries to solve two things: First of all, every
system-framebuffer now gets a "platform-framebuffer" platform-device.
Iff X86_SYSFB is selected, it additionally tries to parse the
framebuffer information as "simple-framebuffer". If it succeeds, it
created a "simple-framebuffer" object, if it doesn't, a fallback
"platform-framebuffer" is provided.

This series is missing vesafb/efifb/.. patches, which should now bind
to "platform-framebuffer" devices instead of using "struct
screen_info" directly. I intend to add these in the next round.

>> diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
>
>> +#ifdef CONFIG_X86_SYSFB
>
> Rather than ifdef'ing the body of this file, why not create a dummy
> static inline version of add_sysfb() and put that into a header file
> that users include. There should be a header file to prototype the
> function anyway. That way, you avoid all of the ifdefs and static inline
> functions in this file.
>
>> +static bool parse_mode(const struct screen_info *si,
>> +                    struct simplefb_platform_data *mode)
>
>> +                     strlcpy(mode->format, f->name, sizeof(mode->format));
>
> Per my comments about the type of mode->format, I think that could just be:
>
> mode->format = f->name;
>
> ... since formats[] (i.e. f) isn't initdata.
>
>> +#else /* CONFIG_X86_SYSFB */
>> +
>> +static bool parse_mode(const struct screen_info *si,
>> +                    struct simplefb_platform_data *mode)
>> +{
>> +     return false;
>> +}
>> +
>> +static int create_simplefb(const struct screen_info *si,
>> +                        const struct simplefb_platform_data *mode)
>> +{
>> +     return -EINVAL;
>> +}
>> +
>> +#endif /* CONFIG_X86_SYSFB */
>
> Following on from my ifdef comment above, I believe those versions of
> those functions will always cause add_sysfb() to return -ENODEV, so you
> may as well provide a static inline for add_sysfb() instead.

No. add_sysfb() is supposed to always succeed. However, if
parse_mode/create_simplefb fail, it creates a "platform-framebuffer"
as fallback. I don't see any way to avoid these ifdefs. Considering
the explanation above, could you elaborate how you think this should
work?

Thanks
David
--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Stephen Warren July 1, 2013, 3:48 p.m. UTC | #3
On 06/28/2013 04:11 AM, David Herrmann wrote:
> Hi
> 
> On Wed, Jun 26, 2013 at 10:49 PM, Stephen Warren <swarren@wwwdotorg.org> wrote:
>> On 06/24/2013 04:27 PM, David Herrmann wrote:
>>> The current situation regarding boot-framebuffers (VGA, VESA/VBE, EFI) on
>>> x86 causes troubles when loading multiple fbdev drivers. The global
>>> "struct screen_info" does not provide any state-tracking about which
>>> drivers use the FBs. request_mem_region() theoretically works, but
>>> unfortunately vesafb/efifb ignore it due to quirks for broken boards.
>>>
>>> Avoid this by creating a "platform-framebuffer" device with a pointer
>>> to the "struct screen_info" as platform-data. Drivers can now create
>>> platform-drivers and the driver-core will refuse multiple drivers being
>>> active simultaneously.
>>>
>>> We keep the screen_info available for backwards-compatibility. Drivers
>>> can be converted in follow-up patches.
>>>
>>> Apart from "platform-framebuffer" devices, this also introduces a
>>> compatibility option for "simple-framebuffer" drivers which recently got
>>> introduced for OF based systems. If CONFIG_X86_SYSFB is selected, we
>>> try to match the screen_info against a simple-framebuffer supported
>>> format. If we succeed, we create a "simple-framebuffer" device instead
>>> of a platform-framebuffer.
...

>>> diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
...
>>> +#else /* CONFIG_X86_SYSFB */
>>> +
>>> +static bool parse_mode(const struct screen_info *si,
>>> +                    struct simplefb_platform_data *mode)
>>> +{
>>> +     return false;
>>> +}
>>> +
>>> +static int create_simplefb(const struct screen_info *si,
>>> +                        const struct simplefb_platform_data *mode)
>>> +{
>>> +     return -EINVAL;
>>> +}
>>> +
>>> +#endif /* CONFIG_X86_SYSFB */
>>
>> Following on from my ifdef comment above, I believe those versions of
>> those functions will always cause add_sysfb() to return -ENODEV, so you
>> may as well provide a static inline for add_sysfb() instead.
> 
> No. add_sysfb() is supposed to always succeed. However, if
> parse_mode/create_simplefb fail, it creates a "platform-framebuffer"
> as fallback. I don't see any way to avoid these ifdefs. Considering
> the explanation above, could you elaborate how you think this should
> work?

Ah, I wasn't getting the fallback mechanism; that if creating a simplefb
wasn't possible or didn't succeed, a platformfb device would be created
instead.

Perhaps the following might be slightly clearer; there are certainly
fewer nesting levels:

static __init int add_sysfb(void)
{
	const struct screen_info *si = &screen_info;
	struct simplefb_platform_data mode;
	struct platform_device *pd;
	bool compatible = false;
	int ret;

	compatible = parse_mode(si, &mode);
	if (compatible) {
		ret = create_simplefb(si, &mode);
		if (!ret)
			return 0;
	}

	pd = platform_device_register_resndata(NULL,
					"platform-framebuffer", 0,
					NULL, 0, si, sizeof(*si));
	ret = IS_ERR(pd) ? PTR_ERR(pd) : 0;

	return ret;
}

--
To unsubscribe from this list: send the line "unsubscribe linux-fbdev" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
diff mbox

Patch

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index fe120da..8eb06b5 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2255,6 +2255,24 @@  config RAPIDIO
 
 source "drivers/rapidio/Kconfig"
 
+config X86_SYSFB
+	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	help
+	  Firmwares often provide initial graphics framebuffers so the BIOS,
+	  bootloader or kernel can show basic video-output during boot for
+	  user-guidance and debugging. Historically, x86 used the VESA BIOS
+	  Extensions and EFI-framebuffers for this, which are mostly limited
+	  to x86. However, a generic system-framebuffer initialization emerged
+	  recently on some non-x86 architectures.
+	  This option, if enabled, marks VGA/VBE/EFI framebuffers as generic
+	  framebuffers so the new generic system-framebuffer drivers can be
+	  used on x86.
+
+	  This breaks any x86-only driver like efifb, vesafb, uvesafb, which
+	  will not work if this is selected.
+
+	  If unsure, say N.
+
 endmenu
 
 
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 7bd3bd3..1e1005a 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -100,6 +100,7 @@  obj-$(CONFIG_X86_CHECK_BIOS_CORRUPTION) += check.o
 obj-$(CONFIG_SWIOTLB)			+= pci-swiotlb.o
 obj-$(CONFIG_OF)			+= devicetree.o
 obj-$(CONFIG_UPROBES)			+= uprobes.o
+obj-y					+= sysfb.o
 
 obj-$(CONFIG_PERF_EVENTS)		+= perf_regs.o
 
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
new file mode 100644
index 0000000..8272958
--- /dev/null
+++ b/arch/x86/kernel/sysfb.c
@@ -0,0 +1,157 @@ 
+/*
+ * Generic System Framebuffers on x86
+ * Copyright (c) 2012-2013 David Herrmann <dh.herrmann@gmail.com>
+ */
+
+/*
+ * This program is free software; you can redistribute it and/or modify it
+ * under the terms of the GNU General Public License as published by the Free
+ * Software Foundation; either version 2 of the License, or (at your option)
+ * any later version.
+ */
+
+/*
+ * Simple-Framebuffer support for x86 systems
+ * Create a platform-device for any available boot framebuffer. The
+ * simple-framebuffer platform device is already available on DT systems, so
+ * this module parses the global "screen_info" object and creates a suitable
+ * platform device compatible with the "simple-framebuffer" DT object. If
+ * the framebuffer is incompatible, we instead create a "platform-framebuffer"
+ * device and pass the screen_info as platform_data. This allows legacy drivers
+ * to pick these devices up without messing with simple-framebuffer drivers.
+ *
+ * If CONFIG_X86_SYSFB is not selected, we never register "simple-framebuffer"
+ * platform devices, but only use "platform-framebuffer" devices for
+ * backwards compatibility.
+ *
+ * TODO: We set the dev_id field of all platform-devices to 0. This allows
+ * other x86 OF/DT parsers to create such devices, too. However, they must
+ * start at offset 1 for this to work.
+ */
+
+#include <linux/err.h>
+#include <linux/init.h>
+#include <linux/kernel.h>
+#include <linux/mm.h>
+#include <linux/platform_data/simplefb.h>
+#include <linux/platform_device.h>
+#include <linux/screen_info.h>
+
+#ifdef CONFIG_X86_SYSFB
+
+static const char simplefb_resname[] = "BOOTFB";
+
+static const struct simplefb_format formats[] = {
+	SIMPLEFB_FORMATS
+};
+
+static bool parse_mode(const struct screen_info *si,
+		       struct simplefb_platform_data *mode)
+{
+	const struct simplefb_format *f;
+	__u8 type;
+	unsigned int i;
+
+	/* TODO: apply quirks from efifb.c here before probing the devices */
+
+	type = si->orig_video_isVGA;
+	if (type != VIDEO_TYPE_VLFB && type != VIDEO_TYPE_EFI)
+		return false;
+
+	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
+		f = &formats[i];
+		if (si->lfb_depth == f->bits_per_pixel &&
+		    si->red_size == f->red.length &&
+		    si->red_pos == f->red.offset &&
+		    si->green_size == f->green.length &&
+		    si->green_pos == f->green.offset &&
+		    si->blue_size == f->blue.length &&
+		    si->blue_pos == f->blue.offset &&
+		    si->rsvd_size == f->transp.length &&
+		    si->rsvd_pos == f->transp.offset) {
+			strlcpy(mode->format, f->name, sizeof(mode->format));
+			mode->width = si->lfb_width;
+			mode->height = si->lfb_height;
+			mode->stride = si->lfb_linelength;
+			return true;
+		}
+	}
+
+	return false;
+}
+
+static int create_simplefb(const struct screen_info *si,
+			   const struct simplefb_platform_data *mode)
+{
+	struct platform_device *pd;
+	struct resource memres;
+	unsigned long len;
+
+	/* don't use lfb_size as it may contain the whole VMEM instead of only
+	 * the part that is occupied by the framebuffer */
+	len = mode->height * mode->stride;
+	len = PAGE_ALIGN(len);
+	if (len > si->lfb_size << 16) {
+		printk(KERN_WARNING "sysfb: VRAM smaller than advertised\n");
+		return -EINVAL;
+	}
+
+	/* setup IORESOURCE_MEM as framebuffer memory */
+	memset(&memres, 0, sizeof(memres));
+	memres.flags = IORESOURCE_MEM;
+	memres.name = simplefb_resname;
+	memres.start = si->lfb_base;
+	memres.end = si->lfb_base + len - 1;
+	if (memres.end <= memres.start)
+		return -EINVAL;
+
+	pd = platform_device_register_resndata(NULL,
+					       "simple-framebuffer", 0,
+					       &memres, 1,
+					       mode, sizeof(*mode));
+	if (IS_ERR(pd))
+		return PTR_ERR(pd);
+
+	return 0;
+}
+
+#else /* CONFIG_X86_SYSFB */
+
+static bool parse_mode(const struct screen_info *si,
+		       struct simplefb_platform_data *mode)
+{
+	return false;
+}
+
+static int create_simplefb(const struct screen_info *si,
+			   const struct simplefb_platform_data *mode)
+{
+	return -EINVAL;
+}
+
+#endif /* CONFIG_X86_SYSFB */
+
+static __init int add_sysfb(void)
+{
+	const struct screen_info *si = &screen_info;
+	struct simplefb_platform_data mode;
+	struct platform_device *pd;
+	bool compatible = false;
+	int ret;
+
+	compatible = parse_mode(si, &mode);
+
+	ret = -ENODEV;
+	if (compatible)
+		ret = create_simplefb(si, &mode);
+
+	if (ret) {
+		pd = platform_device_register_resndata(NULL,
+						"platform-framebuffer", 0,
+						NULL, 0, si, sizeof(*si));
+		ret = IS_ERR(pd) ? PTR_ERR(pd) : 0;
+	}
+
+	return ret;
+}
+device_initcall(add_sysfb);