Linux Framebuffer Layer development
 help / color / mirror / Atom feed
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: Stephen Warren @ 2013-12-19 19:09 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: David Herrmann, linux-kernel, Takashi Iwai,
	the arch/x86 maintainers, linux-fbdev, Geert Uytterhoeven, stable
In-Reply-To: <20131219185517.GA303@gmail.com>

On 12/19/2013 11:55 AM, Ingo Molnar wrote:
> 
> * Stephen Warren <swarren@wwwdotorg.org> wrote:
> 
>> On 12/19/2013 03:13 AM, David Herrmann wrote:
>>> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
>>> resource-conflicts and drivers will refuse to load. A call to
>>> request_mem_region() will fail, if the region overlaps with the mem-region
>>> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
>>> are not affected as they don't reserve their resources, but some others
>>> do, including (nvidiafb, cirrus, ..).
>>
>> I have validated that this doesn't cause any regressions on the/a
>> non-x86 platform using simplefb, although given the main point of this
>> patch is to fix issues on x86, I'm rather hesitant to give a tested-by
>> tag in case someone looking back interprets it incorrectly:-)
> 
> Tested-on-ARM-by: Stephen Warren <swarren@wwwdotorg.org>

I suppose with CC: stable there's some precedent for a comment after the
tag, so perhaps:

Tested-by: Stephen Warren <swarren@wwwdotorg.org> # on ARM only


^ permalink raw reply
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar @ 2013-12-19 18:55 UTC (permalink / raw)
  To: Stephen Warren
  Cc: David Herrmann, linux-kernel, Takashi Iwai,
	the arch/x86 maintainers, linux-fbdev, Geert Uytterhoeven, stable
In-Reply-To: <52B340CB.90807@wwwdotorg.org>


* Stephen Warren <swarren@wwwdotorg.org> wrote:

> On 12/19/2013 03:13 AM, David Herrmann wrote:
> > With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> > resource-conflicts and drivers will refuse to load. A call to
> > request_mem_region() will fail, if the region overlaps with the mem-region
> > used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> > are not affected as they don't reserve their resources, but some others
> > do, including (nvidiafb, cirrus, ..).
> 
> I have validated that this doesn't cause any regressions on the/a
> non-x86 platform using simplefb, although given the main point of this
> patch is to fix issues on x86, I'm rather hesitant to give a tested-by
> tag in case someone looking back interprets it incorrectly:-)

Tested-on-ARM-by: Stephen Warren <swarren@wwwdotorg.org>

?

Thanks,

	Ingo

^ permalink raw reply
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: Stephen Warren @ 2013-12-19 18:54 UTC (permalink / raw)
  To: David Herrmann, linux-kernel
  Cc: Takashi Iwai, the arch/x86 maintainers, linux-fbdev,
	Geert Uytterhoeven, Ingo Molnar, stable
In-Reply-To: <1387448038-8260-1-git-send-email-dh.herrmann@gmail.com>

On 12/19/2013 03:13 AM, David Herrmann wrote:
> With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
> resource-conflicts and drivers will refuse to load. A call to
> request_mem_region() will fail, if the region overlaps with the mem-region
> used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
> are not affected as they don't reserve their resources, but some others
> do, including (nvidiafb, cirrus, ..).

I have validated that this doesn't cause any regressions on the/a
non-x86 platform using simplefb, although given the main point of this
patch is to fix issues on x86, I'm rather hesitant to give a tested-by
tag in case someone looking back interprets it incorrectly:-)

^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Tony Lindgren @ 2013-12-19 18:34 UTC (permalink / raw)
  To: Tomi Valkeinen, Benoît Cousson, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131219170028.GA24230@earth.universe>

* Sebastian Reichel <sre@ring0.de> [131219 09:01]:
> On Thu, Dec 19, 2013 at 08:42:31AM -0800, Tony Lindgren wrote:
> > * Sebastian Reichel <sre@debian.org> [131219 05:57]:
> > > On Thu, Dec 19, 2013 at 11:08:40AM +0100, Sebastian Reichel wrote:
> > > > > Are you able to check if the bootloader muxes dat3 to SDI mode?
> > > > 
> > > > The bootloader's source code is not available as far as i know.
> > > > 
> > > > [...], but I get an external abort on non-linefetch.
> > > > So I can't check it :(
> > > 
> > > Ok. This is fixed by applying [0] (Thanks for the hint, Tomi!).
> > > This is the mux configuration from the bootloader:
> > > 
> > > [...]
> > > 
> > > [0] https://patchwork.kernel.org/patch/3283781/
> > 
> > Do we need to update Laurent's patch with this?
> 
> No, the patch is only needed to avoid the mentioned external abort
> on non-linefetch when doing "cat /sys/kernel/debug/pinctrl/.../pins".
> 
> > Or can we use it as it is and maybe you can ack it?
> 
> Sure. I will add an Ack.

OK thanks.

Here's my current legacy mux dumping tool if you want to look at
how the pins are muxed by the bootloader while booted to DT mode.

This is against omap-for-v3.14/dt branch, the list in pdata-quirks.c
may need updating depending on the board. And this does not account
for splitting up the pinctrl core for omap3..

Regards,

Tony

8< -----------------------------------
From 2fb3765fd5739e8f5fb4318e2576081be6d535f2 Mon Sep 17 00:00:00 2001
From: Tony Lindgren <tony@atomide.com>
Date: Thu, 19 Dec 2013 10:31:03 -0800
Subject: [PATCH] Not for merging: Allows dumping out mux entries in .dts
 format using legacy mux

For pinctrl-single.c we should eventually have a user space
app to configure and display pin settings. Meanwhile, allow
using the legacy mux interface to do that:

# mount -t debugfs debugfs /sys/kernel/debug
# cat /sys/kernel/debug/omap_mux/board/wkup | grep fref_clk0_out
	0x14 0x2        /* fref_clk0_out.sys_drm_msecure gpio6 OUTPUT | MODE2 */

diff --git a/arch/arm/mach-omap2/mux.c b/arch/arm/mach-omap2/mux.c
index 48094b58..40658d9 100644
--- a/arch/arm/mach-omap2/mux.c
+++ b/arch/arm/mach-omap2/mux.c
@@ -35,11 +35,10 @@
 #include <linux/irq.h>
 #include <linux/interrupt.h>
 
-
 #include "omap_hwmod.h"
-
 #include "soc.h"
 #include "control.h"
+#include "id.h"
 #include "mux.h"
 #include "prm.h"
 #include "common.h"
@@ -505,17 +504,17 @@ void omap_hwmod_mux(struct omap_hwmod_mux_info *hmux, u8 state)
 #define OMAP_MUX_TEST_FLAG(val, mask)				\
 	if (((val) & (mask)) = (mask)) {			\
 		i++;						\
-		flags[i] =  #mask;				\
+		flags[i] =  #mask + sizeof("OMAP_") - 1;	\
 	}
 
 /* REVISIT: Add checking for non-optimal mux settings */
 static inline void omap_mux_decode(struct seq_file *s, u16 val)
 {
 	char *flags[OMAP_MUX_MAX_NR_FLAGS];
-	char mode[sizeof("OMAP_MUX_MODE") + 1];
+	char mode[sizeof("MUX_MODE") + 1];
 	int i = -1;
 
-	sprintf(mode, "OMAP_MUX_MODE%d", val & 0x7);
+	sprintf(mode, "MUX_MODE%d", val & 0x7);
 	i++;
 	flags[i] = mode;
 
@@ -553,7 +552,7 @@ static inline void omap_mux_decode(struct seq_file *s, u16 val)
 		}
 	} else {
 		i++;
-		flags[i] = "OMAP_PIN_OUTPUT";
+		flags[i] = "PIN_OUTPUT";
 	}
 
 	do {
@@ -568,15 +567,26 @@ static inline void omap_mux_decode(struct seq_file *s, u16 val)
 static int omap_mux_dbg_board_show(struct seq_file *s, void *unused)
 {
 	struct omap_mux_partition *partition = s->private;
+	int pbase = (int)partition->base;
 	struct omap_mux_entry *e;
-	u8 omap_gen = omap_rev() >> 28;
+
+	if (!(pbase & 0xfff))
+		pbase = 0x40;
+	else
+		pbase = 0;
+
+	seq_printf(s, "\t\tpinctrl-single,pins = <\n");
 
 	list_for_each_entry(e, &partition->muxmodes, node) {
 		struct omap_mux *m = &e->mux;
 		char m0_def[OMAP_MUX_DEFNAME_LEN];
 		char *m0_name = m->muxnames[0];
 		u16 val;
-		int i, mode;
+		int padconf_offset, i, mode;
+
+		padconf_offset = m->reg_offset - pbase;
+		if (cpu_is_omap3630() && padconf_offset > 0x5ca)
+			continue;
 
 		if (!m0_name)
 			continue;
@@ -591,18 +601,18 @@ static int omap_mux_dbg_board_show(struct seq_file *s, void *unused)
 		}
 		val = omap_mux_read(partition, m->reg_offset);
 		mode = val & OMAP_MUX_MODE7;
-		if (mode != 0)
-			seq_printf(s, "/* %s */\n", m->muxnames[mode]);
-
-		/*
-		 * XXX: Might be revisited to support differences across
-		 * same OMAP generation.
-		 */
-		seq_printf(s, "OMAP%d_MUX(%s, ", omap_gen, m0_def);
+		seq_printf(s, "\t\t\t0x%x (",
+			   padconf_offset);
 		omap_mux_decode(s, val);
-		seq_printf(s, "),\n");
+		seq_printf(s, ")\t/* %s.%s */",
+			   m->muxnames[0], m->muxnames[mode]);
+		seq_printf(s, " //0x%x gpio%i",
+			   val, m->gpio);
+		seq_printf(s, "\n");
 	}
 
+	seq_printf(s, "\t\t>;\n");
+
 	return 0;
 }
 
diff --git a/arch/arm/mach-omap2/pdata-quirks.c b/arch/arm/mach-omap2/pdata-quirks.c
index 3d5b24d..a6d6e7e 100644
--- a/arch/arm/mach-omap2/pdata-quirks.c
+++ b/arch/arm/mach-omap2/pdata-quirks.c
@@ -22,6 +22,7 @@
 #include "common-board-devices.h"
 #include "dss-common.h"
 #include "control.h"
+#include "mux.h"
 
 struct pdata_init {
 	const char *compatible;
@@ -277,6 +278,39 @@ static struct pdata_init pdata_quirks[] __initdata = {
 	{ /* sentinel */ },
 };
 
+struct board_package {
+	const char *compatible;
+	int (*fn)(struct omap_board_mux *board_mux, int flags);
+	int flags;
+};
+
+static struct board_package packages[] __initdata = {
+	{ "nokia,n800", omap2420_mux_init, OMAP_PACKAGE_ZAC, },
+	{ "nokia,n810", omap2420_mux_init, OMAP_PACKAGE_ZAC, },
+	{ "ti,omap2430-sdp", omap2430_mux_init, OMAP_PACKAGE_ZAC, },
+	{ "nokia,omap3-n900", omap3_mux_init, OMAP_PACKAGE_CBB, },
+	{ "ti,omap3-evm-37xx", omap3_mux_init, OMAP_PACKAGE_CBB, },
+	{ "ti,omap3-ldp", omap3_mux_init, OMAP_PACKAGE_CBB, },
+	{ "ti,omap3-zoom3", omap3_mux_init, OMAP_PACKAGE_CBP, },
+	{ "ti,omap4-sdp", omap4_mux_init, OMAP_PACKAGE_CBS, },
+	{ "ti,omap4-panda", omap4_mux_init, OMAP_PACKAGE_CBS, },
+	{ /* sentinel */ },
+};
+
+static void legacy_mux_init(void)
+{
+	struct board_package *pkg = packages;
+
+	while (pkg->compatible) {
+		if (of_machine_is_compatible(pkg->compatible)) {
+			if (pkg->fn)
+				pkg->fn(NULL, pkg->flags);
+			break;
+		}
+		pkg++;
+	}
+}
+
 static void pdata_quirks_check(struct pdata_init *quirks)
 {
 	while (quirks->compatible) {
@@ -296,4 +330,5 @@ void __init pdata_quirks_init(struct of_device_id *omap_dt_match_table)
 	of_platform_populate(NULL, omap_dt_match_table,
 			     omap_auxdata_lookup, NULL);
 	pdata_quirks_check(pdata_quirks);
+	legacy_mux_init();
 }

^ permalink raw reply related
* Re: [PATCH v4] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar @ 2013-12-19 18:33 UTC (permalink / raw)
  To: David Herrmann
  Cc: linux-kernel, H. Peter Anvin, Thomas Gleixner, x86, linux-fbdev,
	stable
In-Reply-To: <1387473881-26179-1-git-send-email-dh.herrmann@gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> +/*
> + * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
> + * called from any context except recursively or from sysfb_register().
> + * Used by remove_conflicting_framebuffers() and friends.
> + */
> +void sysfb_unregister(const struct apertures_struct *apert, bool primary)
> +{
> +	mutex_lock(&sysfb_lock);
> +	if (!IS_ERR(sysfb_dev) && sysfb_dev) {
> +		if (sysfb_match(apert, primary)) {
> +			platform_device_unregister(sysfb_dev);
> +			sysfb_dev = ERR_PTR(-EALREADY);
> +		}
> +	} else {
> +		/* if !sysfb_dev, set err so no new sysfb is probed later */
> +		sysfb_dev = ERR_PTR(-EALREADY);

Just a small detail: we can get into this 'else' branch not just with 
NULL, but also with IS_ERR(sysfb_dev). In that case we override 
whatever error code is contained in sysfb_dev and overwrite it with 
ERR_PTR(-EALREADY).

(Probably not a big deal, because we don't actually ever seem to 
extract the error code from the pointer, but wanted to mention it.)

> +#ifdef CONFIG_X86_SYSFB
> +#include <asm/sysfb.h>
> +#endif

Pet peeve, this looks sexier:

#ifdef CONFIG_X86_SYSFB
# include <asm/sysfb.h>
#endif

> @@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
>  	}
>  }
>  
> +static void remove_conflicting_sysfb(const struct apertures_struct *apert,
> +				     bool primary)
> +{
> +	if (!apert)
> +		return;
> +
> +#ifdef CONFIG_X86_SYSFB
> +	sysfb_unregister(apert, primary);
> +#endif
> +}

So why not make sysfb_unregister() accept a !apert parameter (it would 
simply return), at which point remove_conflicting_sysfb() could be 
eliminated and just be replaced with a direct sysfb_unregister() call 
- with no #ifdefs.

We only need #ifdefs for the sysfb_unregister() declaration in the .h 
file.

At first sight this looks simpler and cleaner for the fix itself - no 
need for extra cleanups for this detail.

Thanks,

	Ingo

^ permalink raw reply
* [PATCH v4] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-19 17:24 UTC (permalink / raw)
  To: linux-kernel
  Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-fbdev,
	David Herrmann, stable
In-Reply-To: <1387448038-8260-1-git-send-email-dh.herrmann@gmail.com>

With CONFIG_X86_SYSFB=y, probing real hw-drivers may result in
resource-conflicts and drivers will refuse to load. A call to
request_mem_region() will fail, if the region overlaps with the mem-region
used by simplefb. The common desktop DRM drivers (intel, nouveau, radeon)
are not affected as they don't reserve their resources, but some others
do, including (nvidiafb, cirrus, ..).

The problem is that we add an IORESOURCE_MEM to the simple-framebuffer
platform-device during bootup but never release it. Probing simplefb on
this platform-device is fine, but the handover to real-hw via
remove_conflicting_framebuffers() will only unload the fbdev driver, but
keep the platform-device around. Thus, if the real hw-driver calls
request_mem_region() and friends on the same PCI region, we will get a
resource conflict and most drivers refuse to load. Users will see
errors like:
  "nvidiafb: cannot reserve video memory at <xyz>"

vesafb and efifb don't store any resources, so disabling CONFIG_X86_SYSFB
and falling back to those drivers will avoid the bug. With sysfb enabled,
we need to properly unload the simple-framebuffer devices before probing
real hw-drivers.

This patch adds sysfb_unregister() for that purpose. It can be called from
any context (except from the platform-device ->probe and ->remove callback
path), synchronously unloads any global sysfb and prevents new sysfbs from
getting registered. Thus, you can call it even before any sysfb has been
loaded. Note that for now we only do that for simple-framebuffer devices,
as efi/vesa-framebuffer platform-drivers lack ->remove() callbacks.
They're not affected by the resource-conflicts, so we can fix those later.

This also changes remove_conflicting_framebuffer() to call this helper
*before* trying its heuristic to remove conflicting drivers. This way, we
unload sysfbs properly on any conflict. But to avoid dead-locks in
register_framebuffer(), we must not call sysfb_unregister() for
framebuffers probing on sysfb devices. Hence, we simply remove any
aperture from simplefb and we're good to go. simplefb is unregistered by
sysfb_unregister() now, so no reason to keep the apertures (on non-x86,
there currently is no handover from simplefb, so we're fine. If it's
added, they need to provide something like sysfb_unregister() too)

Cc: <stable@vger.kernel.org> # v3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
v4:
 - change #ifdef to CONFIG_X86_SYSFB again

 arch/x86/include/asm/sysfb.h     |  6 ++++
 arch/x86/kernel/sysfb.c          | 68 ++++++++++++++++++++++++++++++++++++++++
 arch/x86/kernel/sysfb_simplefb.c |  9 ++----
 drivers/video/fbmem.c            | 19 +++++++++++
 drivers/video/simplefb.c         |  8 -----
 5 files changed, 95 insertions(+), 15 deletions(-)

diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
index 2aeb3e2..0877cf1 100644
--- a/arch/x86/include/asm/sysfb.h
+++ b/arch/x86/include/asm/sysfb.h
@@ -11,6 +11,7 @@
  * any later version.
  */
 
+#include <linux/fb.h>
 #include <linux/kernel.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/screen_info.h>
@@ -59,6 +60,11 @@ struct efifb_dmi_info {
 	int flags;
 };
 
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size);
+void sysfb_unregister(const struct apertures_struct *apert, bool primary);
+
 #ifdef CONFIG_EFI
 
 extern struct efifb_dmi_info efifb_dmi_list[];
diff --git a/arch/x86/kernel/sysfb.c b/arch/x86/kernel/sysfb.c
index 193ec2c..882dc7c 100644
--- a/arch/x86/kernel/sysfb.c
+++ b/arch/x86/kernel/sysfb.c
@@ -33,11 +33,79 @@
 #include <linux/init.h>
 #include <linux/kernel.h>
 #include <linux/mm.h>
+#include <linux/mutex.h>
 #include <linux/platform_data/simplefb.h>
 #include <linux/platform_device.h>
 #include <linux/screen_info.h>
 #include <asm/sysfb.h>
 
+static DEFINE_MUTEX(sysfb_lock);
+static struct platform_device *sysfb_dev;
+
+int __init sysfb_register(const char *name, int id,
+			  const struct resource *res, unsigned int res_num,
+			  const void *data, size_t data_size)
+{
+	struct platform_device *pd;
+	int ret = 0;
+
+	mutex_lock(&sysfb_lock);
+	if (!sysfb_dev) {
+		pd = platform_device_register_resndata(NULL, name, id,
+						       res, res_num,
+						       data, data_size);
+		if (IS_ERR(pd))
+			ret = PTR_ERR(pd);
+		else
+			sysfb_dev = pd;
+	}
+	mutex_unlock(&sysfb_lock);
+
+	return ret;
+}
+
+static bool sysfb_match(const struct apertures_struct *apert, bool primary)
+{
+	struct screen_info *si = &screen_info;
+	unsigned int i;
+	const struct aperture *a;
+
+	if (!apert || primary)
+		return true;
+
+	for (i = 0; i < apert->count; ++i) {
+		a = &apert->ranges[i];
+		if (a->base >= si->lfb_base &&
+		    a->base < si->lfb_base + ((u64)si->lfb_size << 16))
+			return true;
+		if (si->lfb_base >= a->base &&
+		    si->lfb_base < a->base + a->size)
+			return true;
+	}
+
+	return false;
+}
+
+/*
+ * Unregister the sysfb and prevent new sysfbs from getting registered. Can be
+ * called from any context except recursively or from sysfb_register().
+ * Used by remove_conflicting_framebuffers() and friends.
+ */
+void sysfb_unregister(const struct apertures_struct *apert, bool primary)
+{
+	mutex_lock(&sysfb_lock);
+	if (!IS_ERR(sysfb_dev) && sysfb_dev) {
+		if (sysfb_match(apert, primary)) {
+			platform_device_unregister(sysfb_dev);
+			sysfb_dev = ERR_PTR(-EALREADY);
+		}
+	} else {
+		/* if !sysfb_dev, set err so no new sysfb is probed later */
+		sysfb_dev = ERR_PTR(-EALREADY);
+	}
+	mutex_unlock(&sysfb_lock);
+}
+
 static __init int sysfb_init(void)
 {
 	struct screen_info *si = &screen_info;
diff --git a/arch/x86/kernel/sysfb_simplefb.c b/arch/x86/kernel/sysfb_simplefb.c
index 86179d4..a760d47 100644
--- a/arch/x86/kernel/sysfb_simplefb.c
+++ b/arch/x86/kernel/sysfb_simplefb.c
@@ -64,7 +64,6 @@ __init bool parse_mode(const struct screen_info *si,
 __init int create_simplefb(const struct screen_info *si,
 			   const struct simplefb_platform_data *mode)
 {
-	struct platform_device *pd;
 	struct resource res;
 	unsigned long len;
 
@@ -86,10 +85,6 @@ __init int create_simplefb(const struct screen_info *si,
 	if (res.end <= res.start)
 		return -EINVAL;
 
-	pd = platform_device_register_resndata(NULL, "simple-framebuffer", 0,
-					       &res, 1, mode, sizeof(*mode));
-	if (IS_ERR(pd))
-		return PTR_ERR(pd);
-
-	return 0;
+	return sysfb_register("simple-framebuffer", 0, &res, 1, mode,
+			      sizeof(*mode));
 }
diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index 010d191..7dc0491 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -35,6 +35,9 @@
 
 #include <asm/fb.h>
 
+#ifdef CONFIG_X86_SYSFB
+#include <asm/sysfb.h>
+#endif
 
     /*
      *  Frame buffer device initialization and setup routines
@@ -1604,6 +1607,17 @@ static void do_remove_conflicting_framebuffers(struct apertures_struct *a,
 	}
 }
 
+static void remove_conflicting_sysfb(const struct apertures_struct *apert,
+				     bool primary)
+{
+	if (!apert)
+		return;
+
+#ifdef CONFIG_X86_SYSFB
+	sysfb_unregister(apert, primary);
+#endif
+}
+
 static int do_register_framebuffer(struct fb_info *fb_info)
 {
 	int i;
@@ -1742,6 +1756,8 @@ EXPORT_SYMBOL(unlink_framebuffer);
 void remove_conflicting_framebuffers(struct apertures_struct *a,
 				     const char *name, bool primary)
 {
+	remove_conflicting_sysfb(a, primary);
+
 	mutex_lock(&registration_lock);
 	do_remove_conflicting_framebuffers(a, name, primary);
 	mutex_unlock(&registration_lock);
@@ -1762,6 +1778,9 @@ register_framebuffer(struct fb_info *fb_info)
 {
 	int ret;
 
+	remove_conflicting_sysfb(fb_info->apertures,
+				 fb_is_primary_device(fb_info));
+
 	mutex_lock(&registration_lock);
 	ret = do_register_framebuffer(fb_info);
 	mutex_unlock(&registration_lock);
diff --git a/drivers/video/simplefb.c b/drivers/video/simplefb.c
index 210f3a0..9f4a0cf 100644
--- a/drivers/video/simplefb.c
+++ b/drivers/video/simplefb.c
@@ -209,14 +209,6 @@ static int simplefb_probe(struct platform_device *pdev)
 	info->var.blue = params.format->blue;
 	info->var.transp = params.format->transp;
 
-	info->apertures = alloc_apertures(1);
-	if (!info->apertures) {
-		framebuffer_release(info);
-		return -ENOMEM;
-	}
-	info->apertures->ranges[0].base = info->fix.smem_start;
-	info->apertures->ranges[0].size = info->fix.smem_len;
-
 	info->fbops = &simplefb_ops;
 	info->flags = FBINFO_DEFAULT | FBINFO_MISC_FIRMWARE;
 	info->screen_base = ioremap_wc(info->fix.smem_start,
-- 
1.8.5.1


^ permalink raw reply related
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-19 17:18 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, Takashi Iwai, Stephen Warren,
	the arch/x86 maintainers, linux-fbdev@vger.kernel.org,
	Geert Uytterhoeven, stable
In-Reply-To: <20131219170927.GC30382@gmail.com>

Hi

On Thu, Dec 19, 2013 at 6:09 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
>> >
>> > * David Herrmann <dh.herrmann@gmail.com> wrote:
>> >
>> >> --- a/drivers/video/fbmem.c
>> >> +++ b/drivers/video/fbmem.c
>> >> @@ -35,6 +35,9 @@
>> >>
>> >>  #include <asm/fb.h>
>> >>
>> >> +#if IS_ENABLED(CONFIG_X86)
>> >> +#include <asm/sysfb.h>
>> >> +#endif
>> >
>> > I think this can be written as:
>> >
>> > #ifdef CONFIG_X86
>> > # include <asm/sysfb.h>
>> > #endif
>> >
>> > also ... the dependency on a large, unrelated option like CONFIG_X86
>> > looks pretty ugly here - is there no other, more specific CONFIG_
>> > option that can be used here - such as CONFIG_FB_SIMPLE or
>> > CONFIG_X86_SYSFB?
>> >
>> >> +#if IS_ENABLED(CONFIG_X86)
>> >> +     sysfb_unregister(apert, primary);
>> >> +#endif
>> >
>> > Ditto.
>>
>> CONFIG_X86 is probably never 'm'.. will fix that. It was
>> CONFIG_X86_SYSFB before and that works, too, but the broader X86
>> seemed more appropriate as sysfb is available on all x86.
>
> Well, sysfb is available if CONFIG_X86_SYSFB is set, right? So on
> !CONFIG_X86_SYSFB x86 kernels this code should not run, right?

No. The sysfb code is always enabled. It provides platform-devices for
firmware-framebuffers which efifb/vesafb pick up. The X86_SYSFB option
only controls whether generic system-framebuffers should be used
instead of the specific efi/vesa FBs (to be compatible to other
architectures and keep efi/vesa specifics in arch/x86/).

But the system-framebuffer conversion (to a format compatible to
simplefb/simple-framebuffer) is what may break as the legacy fbs don't
reserve resources. Hence, putting it enclosed into X86_SYSFB works for
now. But if we start registering efifb/vesafb with sysfb, too, then we
need to change it to CONFIG_X86 as all fbs are affected then.

I think using X86_SYSFB (as in v1 of this patch) is the way to go. I
will fix it up and resend v4. The broader #ifdef can be used once we
do the more complex cleanups, in which case it will go away entirely.

>> Note that I have patches here locally which move
>> sysfb_register/unregister to drivers/video/sysfb.c and add
>> include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected
>> to avoid the #ifdef. This will allow other architectures to do
>> gfx-hand-over, too. They seem too big for stable, though. That's why
>> I split them up and added it to x86/kernel/sysfb.c first.
>
> Yeah, it's fine to do those cleanups after the minimal fix. But using
> a sensible config option must still be done - we cannot just slap
> broad CONFIG_X86 dependencies into random code.

Ok.

Thanks
David

^ permalink raw reply
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar @ 2013-12-19 17:09 UTC (permalink / raw)
  To: David Herrmann
  Cc: linux-kernel, Takashi Iwai, Stephen Warren,
	the arch/x86 maintainers, linux-fbdev@vger.kernel.org,
	Geert Uytterhoeven, stable
In-Reply-To: <CANq1E4Ss30G72t3R1iza9vOO7ESYQynjS74mB+nJ-zT2zAvyzQ@mail.gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> Hi
> 
> On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> --- a/drivers/video/fbmem.c
> >> +++ b/drivers/video/fbmem.c
> >> @@ -35,6 +35,9 @@
> >>
> >>  #include <asm/fb.h>
> >>
> >> +#if IS_ENABLED(CONFIG_X86)
> >> +#include <asm/sysfb.h>
> >> +#endif
> >
> > I think this can be written as:
> >
> > #ifdef CONFIG_X86
> > # include <asm/sysfb.h>
> > #endif
> >
> > also ... the dependency on a large, unrelated option like CONFIG_X86
> > looks pretty ugly here - is there no other, more specific CONFIG_
> > option that can be used here - such as CONFIG_FB_SIMPLE or
> > CONFIG_X86_SYSFB?
> >
> >> +#if IS_ENABLED(CONFIG_X86)
> >> +     sysfb_unregister(apert, primary);
> >> +#endif
> >
> > Ditto.
> 
> CONFIG_X86 is probably never 'm'.. will fix that. It was 
> CONFIG_X86_SYSFB before and that works, too, but the broader X86 
> seemed more appropriate as sysfb is available on all x86.

Well, sysfb is available if CONFIG_X86_SYSFB is set, right? So on 
!CONFIG_X86_SYSFB x86 kernels this code should not run, right?

> Note that I have patches here locally which move 
> sysfb_register/unregister to drivers/video/sysfb.c and add 
> include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected 
> to avoid the #ifdef. This will allow other architectures to do 
> gfx-hand-over, too. They seem too big for stable, though. That's why 
> I split them up and added it to x86/kernel/sysfb.c first.

Yeah, it's fine to do those cleanups after the minimal fix. But using 
a sensible config option must still be done - we cannot just slap 
broad CONFIG_X86 dependencies into random code.

Thanks,

	Ingo

^ permalink raw reply
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-19 17:03 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, Takashi Iwai, Stephen Warren,
	the arch/x86 maintainers, linux-fbdev@vger.kernel.org,
	Geert Uytterhoeven, stable
In-Reply-To: <20131219163606.GA29243@gmail.com>

Hi

On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -35,6 +35,9 @@
>>
>>  #include <asm/fb.h>
>>
>> +#if IS_ENABLED(CONFIG_X86)
>> +#include <asm/sysfb.h>
>> +#endif
>
> I think this can be written as:
>
> #ifdef CONFIG_X86
> # include <asm/sysfb.h>
> #endif
>
> also ... the dependency on a large, unrelated option like CONFIG_X86
> looks pretty ugly here - is there no other, more specific CONFIG_
> option that can be used here - such as CONFIG_FB_SIMPLE or
> CONFIG_X86_SYSFB?
>
>> +#if IS_ENABLED(CONFIG_X86)
>> +     sysfb_unregister(apert, primary);
>> +#endif
>
> Ditto.

CONFIG_X86 is probably never 'm'.. will fix that.
It was CONFIG_X86_SYSFB before and that works, too, but the broader
X86 seemed more appropriate as sysfb is available on all x86.

Note that I have patches here locally which move
sysfb_register/unregister to drivers/video/sysfb.c and add
include/linux/sysfb.h with dummies if CONFIG_SYSFB is not selected to
avoid the #ifdef. This will allow other architectures to do
gfx-hand-over, too. They seem too big for stable, though. That's why I
split them up and added it to x86/kernel/sysfb.c first.

Thanks
David

^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-19 17:00 UTC (permalink / raw)
  To: Tony Lindgren
  Cc: Tomi Valkeinen, Benoît Cousson, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131219163557.GM27438@atomide.com>

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

On Thu, Dec 19, 2013 at 08:42:31AM -0800, Tony Lindgren wrote:
> * Sebastian Reichel <sre@debian.org> [131219 05:57]:
> > On Thu, Dec 19, 2013 at 11:08:40AM +0100, Sebastian Reichel wrote:
> > > > Are you able to check if the bootloader muxes dat3 to SDI mode?
> > > 
> > > The bootloader's source code is not available as far as i know.
> > > 
> > > [...], but I get an external abort on non-linefetch.
> > > So I can't check it :(
> > 
> > Ok. This is fixed by applying [0] (Thanks for the hint, Tomi!).
> > This is the mux configuration from the bootloader:
> > 
> > [...]
> > 
> > [0] https://patchwork.kernel.org/patch/3283781/
> 
> Do we need to update Laurent's patch with this?

No, the patch is only needed to avoid the mentioned external abort
on non-linefetch when doing "cat /sys/kernel/debug/pinctrl/.../pins".

> Or can we use it as it is and maybe you can ack it?

Sure. I will add an Ack.

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Tony Lindgren @ 2013-12-19 16:42 UTC (permalink / raw)
  To: Tomi Valkeinen, Benoît Cousson, Rob Herring, Pawel Moll,
	Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
	linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131219135608.GA11654@earth.universe>

* Sebastian Reichel <sre@debian.org> [131219 05:57]:
> On Thu, Dec 19, 2013 at 11:08:40AM +0100, Sebastian Reichel wrote:
> > > Are you able to check if the bootloader muxes dat3 to SDI mode?
> > 
> > The bootloader's source code is not available as far as i know.
> > 
> > [...], but I get an external abort on non-linefetch.
> > So I can't check it :(
> 
> Ok. This is fixed by applying [0] (Thanks for the hint, Tomi!).
> This is the mux configuration from the bootloader:
> 
> ...
> pin 96 (480020f0.0) 00000001 pinctrl-single     // sdi dat1n
> pin 97 (480020f2.0) 00000001 pinctrl-single     // sdi dat1p
> pin 98 (480020f4.0) 00000001 pinctrl-single     // sdi dat2n
> pin 99 (480020f6.0) 00000001 pinctrl-single     // sdi dat2p
> pin 100 (480020f8.0) 00000007 pinctrl-single    // sdi dat3n
> pin 101 (480020fa.0) 00000007 pinctrl-single    // sdi dat3p
> pin 102 (480020fc.0) 00000004 pinctrl-single 
> pin 103 (480020fe.0) 00000004 pinctrl-single 
> pin 104 (48002100.0) 00000004 pinctrl-single    // sdi vsync
> pin 105 (48002102.0) 00004104 pinctrl-single    // sdi hsync
> pin 106 (48002104.0) 00000004 pinctrl-single    // sdi den
> pin 107 (48002106.0) 00000004 pinctrl-single    // sdi stp
> pin 108 (48002108.0) 00000001 pinctrl-single    // sdi clkp
> pin 109 (4800210a.0) 00000001 pinctrl-single    // sdi clkn
> ...
> 
> I guess the following entry should be added to the omap3-n900.dts file:
> 
> 	dss_sdi_pins: pinmux_dss_sdi_pins {
> 		pinctrl-single,pins = <
> 			0x0c0 (PIN_OUTPUT | MUX_MODE1)   /* dss_data10.sdi_dat1n */
> 			0x0c2 (PIN_OUTPUT | MUX_MODE1)   /* dss_data11.sdi_dat1p */
> 			0x0c4 (PIN_OUTPUT | MUX_MODE1)   /* dss_data12.sdi_dat2n */
> 			0x0c6 (PIN_OUTPUT | MUX_MODE1)   /* dss_data13.sdi_dat2p */
> 			0x0d8 (PIN_OUTPUT | MUX_MODE1)   /* dss_data22.sdi_clkp */
> 			0x0da (PIN_OUTPUT | MUX_MODE1)   /* dss_data23.sdi_clkn */
> 		>;
> 	};
> 
> [0] https://patchwork.kernel.org/patch/3283781/

Do we need to update Laurent's patch with this? Or can we use it as
it is and maybe you can ack it?

Regards,

Tony

^ permalink raw reply
* Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: Ingo Molnar @ 2013-12-19 16:36 UTC (permalink / raw)
  To: David Herrmann
  Cc: linux-kernel, Takashi Iwai, Stephen Warren,
	the arch/x86 maintainers, linux-fbdev, Geert Uytterhoeven, stable
In-Reply-To: <1387448038-8260-1-git-send-email-dh.herrmann@gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -35,6 +35,9 @@
>  
>  #include <asm/fb.h>
>  
> +#if IS_ENABLED(CONFIG_X86)
> +#include <asm/sysfb.h>
> +#endif

I think this can be written as:

#ifdef CONFIG_X86
# include <asm/sysfb.h>
#endif

also ... the dependency on a large, unrelated option like CONFIG_X86 
looks pretty ugly here - is there no other, more specific CONFIG_ 
option that can be used here - such as CONFIG_FB_SIMPLE or 
CONFIG_X86_SYSFB?

> +#if IS_ENABLED(CONFIG_X86)
> +	sysfb_unregister(apert, primary);
> +#endif

Ditto.

Thanks,

	Ingo 

^ permalink raw reply
* [PATCH v2] x86: sysfb: fool-proof CONFIG_X86_SYSFB
From: David Herrmann @ 2013-12-19 16:18 UTC (permalink / raw)
  To: linux-kernel
  Cc: H. Peter Anvin, Thomas Gleixner, Ingo Molnar, x86, linux-fbdev,
	David Herrmann, stable
In-Reply-To: <1387460330-13989-1-git-send-email-dh.herrmann@gmail.com>

Turns out, people do not read help-texts of new config-options and enable
them nonetheless. So several reports came in with X86_SYSFB=y and
FB_SIMPLE=n, which in almost all situations prevents firmware-fbs from
being probed.

X86_SYSFB clearly states that it turns legacy vesa/efi framebuffers into a
format compatible to simplefb (and does nothing else..). So to avoid
further complaints about missing gfx-support during boot, simply depend on
FB_SIMPLE now.
As FB_SIMPLE is disabled by default and usually only enabled on selected
ARM architectures, x86 users should thus never see the X86_SYSFB
config-option. And if they do, everything is fine as simplefb will be
available.

Note that most of the sysfb code is enabled independently of X86_SYSFB.
The config option only selects a compatibility mode for simplefb. It was
introduced to ease the transition to SimpleDRM and disabling fbdev. As
this is still ongoing, there's no need for non-developers to care for
X86_SYSFB so we can safely hide it behind FB_SIMPLE.

Cc: <stable@vger.kernel.org> # 3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
v2:
 - keep the "If unsure, say Y"
 - (FB_SIMPLE = y) => FB_SIMPLE
 - adjust commit-msg

 arch/x86/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e903c71..c9d2952 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2298,6 +2298,7 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	depends on FB_SIMPLE
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for
-- 
1.8.5.1


^ permalink raw reply related
* Re: [PATCH] x86: sysfb: fool-proof CONFIG_X86_SYSFB
From: Ingo Molnar @ 2013-12-19 15:58 UTC (permalink / raw)
  To: David Herrmann
  Cc: linux-kernel, Takashi Iwai, the arch/x86 maintainers,
	linux-fbdev@vger.kernel.org, stable, H. Peter Anvin,
	Thomas Gleixner
In-Reply-To: <CANq1E4TSh7-WWiRfBUAXZPK+_87=Ven_kK=NHeN8C+daNWdSFQ@mail.gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> Hi
> 
> On Thu, Dec 19, 2013 at 4:40 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> Turns out, people do not read help-texts of new config-options and
> >> enable them nonetheless. [...]
> >
> > Yeah, I too don't read them either, whenever an option name seems
> > obvious to enable, so this is really something that happens frequently
> > ;-)
> >
> >> [...] So several reports came in with X86_SYSFB=y and FB_SIMPLE=n,
> >> which in almost all situations prevents firmware-fbs from being
> >> probed.
> >>
> >> X86_SYSFB clearly states that it turns legacy vesa/efi framebuffers
> >> into a format compatible to simplefb (and does nothing else..). So
> >> to avoid further complaints about missing gfx-support during boot,
> >> simply depend on FB_SIMPLE now.
> >>
> >> As FB_SIMPLE is disabled by default and usually only enabled on
> >> selected ARM architectures, x86 users should thus never see the
> >> X86_SYSFB config-option. And if they do, everything is fine as
> >> simplefb will be available.
> >>
> >> Note that most of the sysfb code is enabled independently of
> >> X86_SYSFB. The config option only selects a compatibility mode for
> >> simplefb. It was introduced to ease the transition to SimpleDRM and
> >> disabling fbdev. As this is still ongoing, there's no need for
> >> non-developers to care for X86_SYSFB so just change the help-text
> >> recommendation to "n".
> >>
> >> Cc: <stable@vger.kernel.org> # 3.11+
> >> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> >> ---
> >>  arch/x86/Kconfig | 3 ++-
> >>  1 file changed, 2 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index e903c71..9317ede 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -2298,6 +2298,7 @@ source "drivers/rapidio/Kconfig"
> >>
> >>  config X86_SYSFB
> >>       bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> >> +     depends on (FB_SIMPLE = y)
> >
> > Could that be written as:
> >
> >         depends on FB_SIMPLE
> >
> > Or is there some complication with modular builds?
> 
> simplefb is actually "bool" so yeah, I can remove the "= y".
> 
> Note that *if* it ever is built as module, it will not get loaded
> during boot, thus not showing any boot-messages. But that's true for
> vesafb/efifb, too, so we're fine.
> 
> >>       help
> >>         Firmwares often provide initial graphics framebuffers so the BIOS,
> >>         bootloader or kernel can show basic video-output during boot for
> >> @@ -2320,7 +2321,7 @@ config X86_SYSFB
> >>         and others enabled as fallback if a system framebuffer is
> >>         incompatible with simplefb.
> >>
> >> -       If unsure, say Y.
> >> +       If unsure, say N.
> >
> > We might in fact leave this bit alone and suggest 'Y' - with the
> > robustification fixes it's not possible anymore to crash the boot or
> > to create an unintentionally headless system, right?
> 
> Nope, we will not cause a headless system, but the handover to other
> fbdev/DRM drivers (other than the common nouveau/radeon/i915) may
> still fail without the other patch (x86: sysfb: remove sysfb when
> probing real hw).
> 
> I will gladly keep the Y, if no-one disagrees. Given that the other 
> patch is rather complex (and a no-op with X86_SYSFB=n) I wasn't 
> quite sure. But as SYSFB is set to 'n' now if FB_SIMPLE wasn't 
> explicitly selected, I think we're fine both ways.

Well, as long as fixes keep coming when people report them, I see no 
problem with keeping the 'Y'. Bugs happen.

And I think we want to apply the other fix regardless of the default.

Thanks,

	Ingo

^ permalink raw reply
* Re: [PATCH] x86: sysfb: fool-proof CONFIG_X86_SYSFB
From: David Herrmann @ 2013-12-19 15:54 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: linux-kernel, Takashi Iwai, the arch/x86 maintainers,
	linux-fbdev@vger.kernel.org, stable, H. Peter Anvin,
	Thomas Gleixner
In-Reply-To: <20131219154020.GB20044@gmail.com>

Hi

On Thu, Dec 19, 2013 at 4:40 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> Turns out, people do not read help-texts of new config-options and
>> enable them nonetheless. [...]
>
> Yeah, I too don't read them either, whenever an option name seems
> obvious to enable, so this is really something that happens frequently
> ;-)
>
>> [...] So several reports came in with X86_SYSFB=y and FB_SIMPLE=n,
>> which in almost all situations prevents firmware-fbs from being
>> probed.
>>
>> X86_SYSFB clearly states that it turns legacy vesa/efi framebuffers
>> into a format compatible to simplefb (and does nothing else..). So
>> to avoid further complaints about missing gfx-support during boot,
>> simply depend on FB_SIMPLE now.
>>
>> As FB_SIMPLE is disabled by default and usually only enabled on
>> selected ARM architectures, x86 users should thus never see the
>> X86_SYSFB config-option. And if they do, everything is fine as
>> simplefb will be available.
>>
>> Note that most of the sysfb code is enabled independently of
>> X86_SYSFB. The config option only selects a compatibility mode for
>> simplefb. It was introduced to ease the transition to SimpleDRM and
>> disabling fbdev. As this is still ongoing, there's no need for
>> non-developers to care for X86_SYSFB so just change the help-text
>> recommendation to "n".
>>
>> Cc: <stable@vger.kernel.org> # 3.11+
>> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
>> ---
>>  arch/x86/Kconfig | 3 ++-
>>  1 file changed, 2 insertions(+), 1 deletion(-)
>>
>> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
>> index e903c71..9317ede 100644
>> --- a/arch/x86/Kconfig
>> +++ b/arch/x86/Kconfig
>> @@ -2298,6 +2298,7 @@ source "drivers/rapidio/Kconfig"
>>
>>  config X86_SYSFB
>>       bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
>> +     depends on (FB_SIMPLE = y)
>
> Could that be written as:
>
>         depends on FB_SIMPLE
>
> Or is there some complication with modular builds?

simplefb is actually "bool" so yeah, I can remove the "= y".

Note that *if* it ever is built as module, it will not get loaded
during boot, thus not showing any boot-messages. But that's true for
vesafb/efifb, too, so we're fine.

>>       help
>>         Firmwares often provide initial graphics framebuffers so the BIOS,
>>         bootloader or kernel can show basic video-output during boot for
>> @@ -2320,7 +2321,7 @@ config X86_SYSFB
>>         and others enabled as fallback if a system framebuffer is
>>         incompatible with simplefb.
>>
>> -       If unsure, say Y.
>> +       If unsure, say N.
>
> We might in fact leave this bit alone and suggest 'Y' - with the
> robustification fixes it's not possible anymore to crash the boot or
> to create an unintentionally headless system, right?

Nope, we will not cause a headless system, but the handover to other
fbdev/DRM drivers (other than the common nouveau/radeon/i915) may
still fail without the other patch (x86: sysfb: remove sysfb when
probing real hw).

I will gladly keep the Y, if no-one disagrees. Given that the other
patch is rather complex (and a no-op with X86_SYSFB=n) I wasn't quite
sure. But as SYSFB is set to 'n' now if FB_SIMPLE wasn't explicitly
selected, I think we're fine both ways.

Thanks
David

^ permalink raw reply
* Re: [PATCH] x86: sysfb: fool-proof CONFIG_X86_SYSFB
From: Ingo Molnar @ 2013-12-19 15:40 UTC (permalink / raw)
  To: David Herrmann
  Cc: linux-kernel, Takashi Iwai, x86, linux-fbdev, stable,
	H. Peter Anvin, Thomas Gleixner
In-Reply-To: <1387460330-13989-1-git-send-email-dh.herrmann@gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> Turns out, people do not read help-texts of new config-options and 
> enable them nonetheless. [...]

Yeah, I too don't read them either, whenever an option name seems 
obvious to enable, so this is really something that happens frequently 
;-)

> [...] So several reports came in with X86_SYSFB=y and FB_SIMPLE=n, 
> which in almost all situations prevents firmware-fbs from being 
> probed.
> 
> X86_SYSFB clearly states that it turns legacy vesa/efi framebuffers 
> into a format compatible to simplefb (and does nothing else..). So 
> to avoid further complaints about missing gfx-support during boot, 
> simply depend on FB_SIMPLE now.
>
> As FB_SIMPLE is disabled by default and usually only enabled on 
> selected ARM architectures, x86 users should thus never see the 
> X86_SYSFB config-option. And if they do, everything is fine as 
> simplefb will be available.
> 
> Note that most of the sysfb code is enabled independently of 
> X86_SYSFB. The config option only selects a compatibility mode for 
> simplefb. It was introduced to ease the transition to SimpleDRM and 
> disabling fbdev. As this is still ongoing, there's no need for 
> non-developers to care for X86_SYSFB so just change the help-text 
> recommendation to "n".
> 
> Cc: <stable@vger.kernel.org> # 3.11+
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
>  arch/x86/Kconfig | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
> 
> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> index e903c71..9317ede 100644
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2298,6 +2298,7 @@ source "drivers/rapidio/Kconfig"
>  
>  config X86_SYSFB
>  	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +	depends on (FB_SIMPLE = y)

Could that be written as:

	depends on FB_SIMPLE

Or is there some complication with modular builds?

>  	help
>  	  Firmwares often provide initial graphics framebuffers so the BIOS,
>  	  bootloader or kernel can show basic video-output during boot for
> @@ -2320,7 +2321,7 @@ config X86_SYSFB
>  	  and others enabled as fallback if a system framebuffer is
>  	  incompatible with simplefb.
>  
> -	  If unsure, say Y.
> +	  If unsure, say N.

We might in fact leave this bit alone and suggest 'Y' - with the 
robustification fixes it's not possible anymore to crash the boot or 
to create an unintentionally headless system, right?

Thanks,

	Ingo

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-19 14:22 UTC (permalink / raw)
  To: David Herrmann
  Cc: One Thousand Gnomes, Ingo Molnar, linux-kernel, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <CANq1E4QxEniHHpTOfrGmzPaSU+DLFfMkfZPsBpZ4aF0Nyda2ig@mail.gmail.com>

At Thu, 19 Dec 2013 15:13:59 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 3:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> >> The "default <wx> if <yz>" only works if the config hasn't been set at
> >> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> >> >> be ignored.
> >> >
> >> > Yes.  I didn't suggest the strict dependency like below, just because
> >> > I assumed you didn't add it intentionally.
> >>
> >> It doesn't work. If CONFIG_FB=n this breaks.
> >>
> >> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> >> >> will try that and send a patch.
> >> >
> >> > That's in general a bad approach.  If the strict dependency is
> >> > allowed, a more intuitive way is to give a reverse selection.  But
> >> > then you'd need to correct the help text, too, as it's implemented
> >> > already as a dependency.
> >>
> >> Well, X86_SYSFB just changes the way x86-bootup code handles
> >> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
> >> CONFIG_DRM=n. However, currently the only driver that can profit from
> >> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
> >> no-one should ever bother (except for people who *really* want this).
> >>
> >> Unfortunately, people still enable it without understanding it.
> >> Changing FB_SIMPLE or other config options doesn't protect against
> >> that, instead I need to make X86_SYSFB fool-proof so it's only
> >> activated if people seriously *want* it.
> >
> > It's not N as default.  It's just unset.  People don't take it
> > seriously as default N unless you explicitly write it.  And, more
> > importantly, your help text even suggests to choose Y as default (in
> > the end of text).  And it doesn't mention how to enable simplefb
> > although it's recommended.  So you can't expect that people do the
> > right thing only from that.
> 
> Yepp, that was my fault.. Already changed to 'N' in the new patch.
> 
> >> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
> >> behavior of "select".
> >
> > There is one single dependency in FB_SIMPLE, so it's relatively easy
> > in this case.
> >
> >> It will only enable FB_SIMPLE but not the
> >> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
> >> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
> >> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
> >> X86_SYSFB from all non-developers who currently shouldn't care anyway.
> >> Most sysfb code is enabled regardless of X86_SYSFB, the option really
> >> just controls the compatibility mode. And unless SimpleDRM is merged,
> >> there's little gain by setting it for non-developers.
> >
> > Then better to have mentioned in the text or changelog.  Or make it
> > depends on CONFIG_EXPERT (it doesn't help much, though, but can be
> > some excuse :)
> 
> I didn't expect it to blow up in my face this way.. probably should
> have. But that's why the changelog didn't mention it. Regarding
> CONFIG_EXPERT, I doubt that helps much and it's a little bit late for
> an excuse ;)
> 
> >> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
> >> a patch for that. If you're still not convinced, I'd be glad to hear
> >> your concerns.
> >
> > Why not just select both FB and FB_SIMPLE from X86_SYSFB?
> >
> > --- a/arch/x86/Kconfig
> > +++ b/arch/x86/Kconfig
> > @@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
> >
> >  config X86_SYSFB
> >         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> > +       select FB
> > +       select SIMPLE_FB
> >         help
> >           Firmwares often provide initial graphics framebuffers so the BIOS,
> >           bootloader or kernel can show basic video-output during boot for
> >
> >
> > The "depends on" hides the option until the dependency is satisfied,
> > so especially when the dependency go over different sections, it
> > should be avoided.  The reverse selection can be problematic
> > sometimes, but as long as the dependency chain is short, it works more
> > easily.
> 
> Is the select-chain recursive?

Yes, the select-chain is recursive.  But select doesn't care about
"depends on", so if a selected item depends on anything else, all
these have to be selected explicitly as well.  In your case, luckily
it's a trivial one-level selection.

> So are FB_SIMPLE-select lines recognized?
> If yes, this could work. Although given the complaints about bad
> simplefb performance compared to vesafb, I'm still not sure. Yeah,
> this time people are to blame as they *explicitly* selected X86_SYSFB
> instead of plain old vesafb, but it's still annoying to hear the
> complaints.

Right.  The performance is a different level of problem, and it's
their choice now.

> I will think about it some more time and send a v2-patch later today
> if nothing else comes to mind.

Great, thanks!


Takashi

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-19 14:13 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: One Thousand Gnomes, Ingo Molnar, linux-kernel, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <s5hlhzhrl8y.wl%tiwai@suse.de>

Hi

On Thu, Dec 19, 2013 at 3:03 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> >> The "default <wx> if <yz>" only works if the config hasn't been set at
>> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
>> >> be ignored.
>> >
>> > Yes.  I didn't suggest the strict dependency like below, just because
>> > I assumed you didn't add it intentionally.
>>
>> It doesn't work. If CONFIG_FB=n this breaks.
>>
>> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
>> >> will try that and send a patch.
>> >
>> > That's in general a bad approach.  If the strict dependency is
>> > allowed, a more intuitive way is to give a reverse selection.  But
>> > then you'd need to correct the help text, too, as it's implemented
>> > already as a dependency.
>>
>> Well, X86_SYSFB just changes the way x86-bootup code handles
>> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
>> CONFIG_DRM=n. However, currently the only driver that can profit from
>> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
>> no-one should ever bother (except for people who *really* want this).
>>
>> Unfortunately, people still enable it without understanding it.
>> Changing FB_SIMPLE or other config options doesn't protect against
>> that, instead I need to make X86_SYSFB fool-proof so it's only
>> activated if people seriously *want* it.
>
> It's not N as default.  It's just unset.  People don't take it
> seriously as default N unless you explicitly write it.  And, more
> importantly, your help text even suggests to choose Y as default (in
> the end of text).  And it doesn't mention how to enable simplefb
> although it's recommended.  So you can't expect that people do the
> right thing only from that.

Yepp, that was my fault.. Already changed to 'N' in the new patch.

>> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
>> behavior of "select".
>
> There is one single dependency in FB_SIMPLE, so it's relatively easy
> in this case.
>
>> It will only enable FB_SIMPLE but not the
>> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
>> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
>> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
>> X86_SYSFB from all non-developers who currently shouldn't care anyway.
>> Most sysfb code is enabled regardless of X86_SYSFB, the option really
>> just controls the compatibility mode. And unless SimpleDRM is merged,
>> there's little gain by setting it for non-developers.
>
> Then better to have mentioned in the text or changelog.  Or make it
> depends on CONFIG_EXPERT (it doesn't help much, though, but can be
> some excuse :)

I didn't expect it to blow up in my face this way.. probably should
have. But that's why the changelog didn't mention it. Regarding
CONFIG_EXPERT, I doubt that helps much and it's a little bit late for
an excuse ;)

>> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
>> a patch for that. If you're still not convinced, I'd be glad to hear
>> your concerns.
>
> Why not just select both FB and FB_SIMPLE from X86_SYSFB?
>
> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
>
>  config X86_SYSFB
>         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +       select FB
> +       select SIMPLE_FB
>         help
>           Firmwares often provide initial graphics framebuffers so the BIOS,
>           bootloader or kernel can show basic video-output during boot for
>
>
> The "depends on" hides the option until the dependency is satisfied,
> so especially when the dependency go over different sections, it
> should be avoided.  The reverse selection can be problematic
> sometimes, but as long as the dependency chain is short, it works more
> easily.

Is the select-chain recursive? So are FB_SIMPLE-select lines recognized?
If yes, this could work. Although given the complaints about bad
simplefb performance compared to vesafb, I'm still not sure. Yeah,
this time people are to blame as they *explicitly* selected X86_SYSFB
instead of plain old vesafb, but it's still annoying to hear the
complaints.

I will think about it some more time and send a v2-patch later today
if nothing else comes to mind.

Thanks for the reviews!
David

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-19 14:03 UTC (permalink / raw)
  To: David Herrmann
  Cc: One Thousand Gnomes, Ingo Molnar, linux-kernel, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <CANq1E4S-wKvCiz21QM=_YRZV5h9iOXsVxESCVy6w7Lh88AvR9Q@mail.gmail.com>

At Thu, 19 Dec 2013 14:37:35 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 2:22 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 19 Dec 2013 13:36:38 +0100,
> > David Herrmann wrote:
> >>
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> >> > At Thu, 19 Dec 2013 11:46:51 +0100,
> >> > David Herrmann wrote:
> >> >>
> >> >> Hi
> >> >>
> >> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> >> > the old behavior.
> >> >> >>
> >> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >> >
> >> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> >> >> > mga.
> >> >>
> >> >> Thanks for the hints. I've read through all I could find and tried to
> >> >> provide some help.
> >> >>
> >> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> >> 'n' by default) but don't read the help text. I did my best to tell
> >> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> >> read the help-text you won't notice that. Don't know what to do about
> >> >> that..
> >> >
> >> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
> >> > like:
> >> >
> >> > --- a/drivers/video/Kconfig
> >> > +++ b/drivers/video/Kconfig
> >> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
> >> >  config FB_SIMPLE
> >> >         bool "Simple framebuffer support"
> >> >         depends on (FB = y)
> >> > +       default y if X86_SYSFB
> >> >         select FB_CFB_FILLRECT
> >> >         select FB_CFB_COPYAREA
> >> >         select FB_CFB_IMAGEBLIT
> >>
> >> The "default <wx> if <yz>" only works if the config hasn't been set at
> >> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> >> be ignored.
> >
> > Yes.  I didn't suggest the strict dependency like below, just because
> > I assumed you didn't add it intentionally.
> 
> It doesn't work. If CONFIG_FB=n this breaks.
> 
> >> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> >> will try that and send a patch.
> >
> > That's in general a bad approach.  If the strict dependency is
> > allowed, a more intuitive way is to give a reverse selection.  But
> > then you'd need to correct the help text, too, as it's implemented
> > already as a dependency.
> 
> Well, X86_SYSFB just changes the way x86-bootup code handles
> firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
> CONFIG_DRM=n. However, currently the only driver that can profit from
> this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
> no-one should ever bother (except for people who *really* want this).
>
> Unfortunately, people still enable it without understanding it.
> Changing FB_SIMPLE or other config options doesn't protect against
> that, instead I need to make X86_SYSFB fool-proof so it's only
> activated if people seriously *want* it.

It's not N as default.  It's just unset.  People don't take it
seriously as default N unless you explicitly write it.  And, more
importantly, your help text even suggests to choose Y as default (in
the end of text).  And it doesn't mention how to enable simplefb
although it's recommended.  So you can't expect that people do the
right thing only from that.

> Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
> behavior of "select".

There is one single dependency in FB_SIMPLE, so it's relatively easy
in this case.

> It will only enable FB_SIMPLE but not the
> dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
> (FB_SIMPLE = y)". And I actually like this, because there's no gain in
> using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
> X86_SYSFB from all non-developers who currently shouldn't care anyway.
> Most sysfb code is enabled regardless of X86_SYSFB, the option really
> just controls the compatibility mode. And unless SimpleDRM is merged,
> there's little gain by setting it for non-developers.

Then better to have mentioned in the text or changelog.  Or make it
depends on CONFIG_EXPERT (it doesn't help much, though, but can be
some excuse :)

> So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
> a patch for that. If you're still not convinced, I'd be glad to hear
> your concerns.

Why not just select both FB and FB_SIMPLE from X86_SYSFB?

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2299,6 +2299,8 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	select FB
+	select SIMPLE_FB
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for


The "depends on" hides the option until the dependency is satisfied,
so especially when the dependency go over different sections, it
should be avoided.  The reverse selection can be problematic
sometimes, but as long as the dependency chain is short, it works more
easily.


Takashi

^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-19 13:56 UTC (permalink / raw)
  To: Tomi Valkeinen, Benoît Cousson, Tony Lindgren, Rob Herring,
	Pawel Moll, Mark Rutland, Stephen Warren, Ian Campbell,
	Rob Landley, linux-omap-u79uwXL29TY76Z2rM5mHXA,
	linux-fbdev-u79uwXL29TY76Z2rM5mHXA,
	devicetree-u79uwXL29TY76Z2rM5mHXA
In-Reply-To: <20131219100840.GA923-SfvFxonMDyemK9LvCR3Hrw@public.gmane.org>

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

On Thu, Dec 19, 2013 at 11:08:40AM +0100, Sebastian Reichel wrote:
> > Are you able to check if the bootloader muxes dat3 to SDI mode?
> 
> The bootloader's source code is not available as far as i know.
> 
> [...], but I get an external abort on non-linefetch.
> So I can't check it :(

Ok. This is fixed by applying [0] (Thanks for the hint, Tomi!).
This is the mux configuration from the bootloader:

...
pin 96 (480020f0.0) 00000001 pinctrl-single     // sdi dat1n
pin 97 (480020f2.0) 00000001 pinctrl-single     // sdi dat1p
pin 98 (480020f4.0) 00000001 pinctrl-single     // sdi dat2n
pin 99 (480020f6.0) 00000001 pinctrl-single     // sdi dat2p
pin 100 (480020f8.0) 00000007 pinctrl-single    // sdi dat3n
pin 101 (480020fa.0) 00000007 pinctrl-single    // sdi dat3p
pin 102 (480020fc.0) 00000004 pinctrl-single 
pin 103 (480020fe.0) 00000004 pinctrl-single 
pin 104 (48002100.0) 00000004 pinctrl-single    // sdi vsync
pin 105 (48002102.0) 00004104 pinctrl-single    // sdi hsync
pin 106 (48002104.0) 00000004 pinctrl-single    // sdi den
pin 107 (48002106.0) 00000004 pinctrl-single    // sdi stp
pin 108 (48002108.0) 00000001 pinctrl-single    // sdi clkp
pin 109 (4800210a.0) 00000001 pinctrl-single    // sdi clkn
...

I guess the following entry should be added to the omap3-n900.dts file:

	dss_sdi_pins: pinmux_dss_sdi_pins {
		pinctrl-single,pins = <
			0x0c0 (PIN_OUTPUT | MUX_MODE1)   /* dss_data10.sdi_dat1n */
			0x0c2 (PIN_OUTPUT | MUX_MODE1)   /* dss_data11.sdi_dat1p */
			0x0c4 (PIN_OUTPUT | MUX_MODE1)   /* dss_data12.sdi_dat2n */
			0x0c6 (PIN_OUTPUT | MUX_MODE1)   /* dss_data13.sdi_dat2p */
			0x0d8 (PIN_OUTPUT | MUX_MODE1)   /* dss_data22.sdi_clkp */
			0x0da (PIN_OUTPUT | MUX_MODE1)   /* dss_data23.sdi_clkn */
		>;
	};

[0] https://patchwork.kernel.org/patch/3283781/

-- Sebastian

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply
* [PATCH] x86: sysfb: fool-proof CONFIG_X86_SYSFB
From: David Herrmann @ 2013-12-19 13:38 UTC (permalink / raw)
  To: linux-kernel
  Cc: Takashi Iwai, Ingo Molnar, x86, linux-fbdev, David Herrmann,
	stable

Turns out, people do not read help-texts of new config-options and enable
them nonetheless. So several reports came in with X86_SYSFB=y and
FB_SIMPLE=n, which in almost all situations prevents firmware-fbs from
being probed.

X86_SYSFB clearly states that it turns legacy vesa/efi framebuffers into a
format compatible to simplefb (and does nothing else..). So to avoid
further complaints about missing gfx-support during boot, simply depend on
FB_SIMPLE now.
As FB_SIMPLE is disabled by default and usually only enabled on selected
ARM architectures, x86 users should thus never see the X86_SYSFB
config-option. And if they do, everything is fine as simplefb will be
available.

Note that most of the sysfb code is enabled independently of X86_SYSFB.
The config option only selects a compatibility mode for simplefb. It was
introduced to ease the transition to SimpleDRM and disabling fbdev. As
this is still ongoing, there's no need for non-developers to care for
X86_SYSFB so just change the help-text recommendation to "n".

Cc: <stable@vger.kernel.org> # 3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
---
 arch/x86/Kconfig | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
index e903c71..9317ede 100644
--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2298,6 +2298,7 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	depends on (FB_SIMPLE = y)
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for
@@ -2320,7 +2321,7 @@ config X86_SYSFB
 	  and others enabled as fallback if a system framebuffer is
 	  incompatible with simplefb.
 
-	  If unsure, say Y.
+	  If unsure, say N.
 
 endmenu
 
-- 
1.8.5.1


^ permalink raw reply related
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-19 13:37 UTC (permalink / raw)
  To: Takashi Iwai
  Cc: One Thousand Gnomes, Ingo Molnar, linux-kernel, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <s5hppotrn5l.wl%tiwai@suse.de>

Hi

On Thu, Dec 19, 2013 at 2:22 PM, Takashi Iwai <tiwai@suse.de> wrote:
> At Thu, 19 Dec 2013 13:36:38 +0100,
> David Herrmann wrote:
>>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
>> > At Thu, 19 Dec 2013 11:46:51 +0100,
>> > David Herrmann wrote:
>> >>
>> >> Hi
>> >>
>> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
>> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> >> > That bug always existed, simplefb is just the first driver to hit it
>> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> >> >> > the old behavior.
>> >> >>
>> >> >> This looks like a regression, so we'll either need a fix or we'll have
>> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>> >> >
>> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
>> >> > mga.
>> >>
>> >> Thanks for the hints. I've read through all I could find and tried to
>> >> provide some help.
>> >>
>> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
>> >> 'n' by default) but don't read the help text. I did my best to tell
>> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
>> >> read the help-text you won't notice that. Don't know what to do about
>> >> that..
>> >
>> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
>> > like:
>> >
>> > --- a/drivers/video/Kconfig
>> > +++ b/drivers/video/Kconfig
>> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
>> >  config FB_SIMPLE
>> >         bool "Simple framebuffer support"
>> >         depends on (FB = y)
>> > +       default y if X86_SYSFB
>> >         select FB_CFB_FILLRECT
>> >         select FB_CFB_COPYAREA
>> >         select FB_CFB_IMAGEBLIT
>>
>> The "default <wx> if <yz>" only works if the config hasn't been set at
>> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
>> be ignored.
>
> Yes.  I didn't suggest the strict dependency like below, just because
> I assumed you didn't add it intentionally.

It doesn't work. If CONFIG_FB=n this breaks.

>> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
>> will try that and send a patch.
>
> That's in general a bad approach.  If the strict dependency is
> allowed, a more intuitive way is to give a reverse selection.  But
> then you'd need to correct the help text, too, as it's implemented
> already as a dependency.

Well, X86_SYSFB just changes the way x86-bootup code handles
firmware-framebuffers. The option makes sense with CONFIG_FB=n or also
CONFIG_DRM=n. However, currently the only driver that can profit from
this is FB_SIMPLE. So I decided to set X86_SYSFB=n as default and
no-one should ever bother (except for people who *really* want this).

Unfortunately, people still enable it without understanding it.
Changing FB_SIMPLE or other config options doesn't protect against
that, instead I need to make X86_SYSFB fool-proof so it's only
activated if people seriously *want* it.

Doing a "select FB_SIMPLE" doesn't work due to the non-recursive
behavior of "select". It will only enable FB_SIMPLE but not the
dependencies of FB_SIMPLE (eg., FB). So I'm now left with "depends on
(FB_SIMPLE = y)". And I actually like this, because there's no gain in
using X86_SYSFB if you don't have FB_SIMPLE enabled. And it hides
X86_SYSFB from all non-developers who currently shouldn't care anyway.
Most sysfb code is enabled regardless of X86_SYSFB, the option really
just controls the compatibility mode. And unless SimpleDRM is merged,
there's little gain by setting it for non-developers.

So I guess "depends on (FB_SIMPLE = y)" is the safest way. I will send
a patch for that. If you're still not convinced, I'd be glad to hear
your concerns. The only other idea I had is this:

config X86_SYSFB
         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
         depends on (FB_SIMPLE = y)
         default y

But I'm not sure I want the "default y" if FB_SIMPLE is active right
now. We can add it later without any problems.

Thanks
David

> --- a/arch/x86/Kconfig
> +++ b/arch/x86/Kconfig
> @@ -2299,6 +2299,7 @@ source "drivers/rapidio/Kconfig"
>
>  config X86_SYSFB
>         bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
> +       select SIMPLE_FB
>         help
>           Firmwares often provide initial graphics framebuffers so the BIOS,
>           bootloader or kernel can show basic video-output during boot for
>
>
> Takashi

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-19 13:22 UTC (permalink / raw)
  To: David Herrmann
  Cc: One Thousand Gnomes, Ingo Molnar, linux-kernel, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <CANq1E4TW3O9czVzX1bj4F5oMrDHZ+ZsLHLCk9ANsg2e14QXmfg@mail.gmail.com>

At Thu, 19 Dec 2013 13:36:38 +0100,
David Herrmann wrote:
> 
> Hi
> 
> On Thu, Dec 19, 2013 at 12:06 PM, Takashi Iwai <tiwai@suse.de> wrote:
> > At Thu, 19 Dec 2013 11:46:51 +0100,
> > David Herrmann wrote:
> >>
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> > the old behavior.
> >> >>
> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >
> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and matrox
> >> > mga.
> >>
> >> Thanks for the hints. I've read through all I could find and tried to
> >> provide some help.
> >>
> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> 'n' by default) but don't read the help text. I did my best to tell
> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> read the help-text you won't notice that. Don't know what to do about
> >> that..
> >
> > You can set FB_SIMPLE default value depending on X86_SYSFB, something
> > like:
> >
> > --- a/drivers/video/Kconfig
> > +++ b/drivers/video/Kconfig
> > @@ -2455,6 +2455,7 @@ config FB_HYPERV
> >  config FB_SIMPLE
> >         bool "Simple framebuffer support"
> >         depends on (FB = y)
> > +       default y if X86_SYSFB
> >         select FB_CFB_FILLRECT
> >         select FB_CFB_COPYAREA
> >         select FB_CFB_IMAGEBLIT
> 
> The "default <wx> if <yz>" only works if the config hasn't been set at
> all, yet. Even a "# CONFIG_* is unset" causes the "default" value to
> be ignored.

Yes.  I didn't suggest the strict dependency like below, just because
I assumed you didn't add it intentionally.

> How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
> will try that and send a patch.

That's in general a bad approach.  If the strict dependency is
allowed, a more intuitive way is to give a reverse selection.  But
then you'd need to correct the help text, too, as it's implemented
already as a dependency.

--- a/arch/x86/Kconfig
+++ b/arch/x86/Kconfig
@@ -2299,6 +2299,7 @@ source "drivers/rapidio/Kconfig"
 
 config X86_SYSFB
 	bool "Mark VGA/VBE/EFI FB as generic system framebuffer"
+	select SIMPLE_FB
 	help
 	  Firmwares often provide initial graphics framebuffers so the BIOS,
 	  bootloader or kernel can show basic video-output during boot for


Takashi

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Ingo Molnar @ 2013-12-19 12:44 UTC (permalink / raw)
  To: David Herrmann
  Cc: One Thousand Gnomes, linux-kernel, Takashi Iwai, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <CANq1E4R5fF+Xtub8E_64bh6akZi8uU6d2ntG-rzsc_D7KFBY=g@mail.gmail.com>


* David Herrmann <dh.herrmann@gmail.com> wrote:

> Hi
> 
> On Thu, Dec 19, 2013 at 1:31 PM, Ingo Molnar <mingo@kernel.org> wrote:
> >
> > * David Herrmann <dh.herrmann@gmail.com> wrote:
> >
> >> Hi
> >>
> >> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
> >> <gnomes@lxorguk.ukuu.org.uk> wrote:
> >> >> > That bug always existed, simplefb is just the first driver to hit it
> >> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
> >> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
> >> >> > the old behavior.
> >> >>
> >> >> This looks like a regression, so we'll either need a fix or we'll have
> >> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
> >> >
> >> > Kernel bugzilla has entries for simplefb breaking both vesafb and
> >> > matrox mga.
> >>
> >> Thanks for the hints. I've read through all I could find and tried
> >> to provide some help.
> >>
> >> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
> >> 'n' by default) but don't read the help text. I did my best to tell
> >> people that this option requires CONFIG_FB_SIMPLE, but if you don't
> >> read the help-text you won't notice that. Don't know what to do
> >> about that..
> >
> > People generally don't read the help text - still the kernel should
> > not break. So please the Kconfig angle (and the bootup logic, etc.)
> > fool-proof, graphics failures are not fun to debug!
> 
> There're dozens of combinations that break gfx-boot. [...]

Then that's a bug too.

> [...] Even non-obvious things like disabling VT_HW_CONSOLE_BINDING 
> will break gfx-handover. Anyhow, bad examples are no excuse.. I will 
> send patches to fool-proof sysfb.

Cool, thanks!

Arguably there's a lot of broken gfx legacy in Linux.

<SoapBox>: 15 years ago we should have merged GGI that really got the 
integrated gfx principles right from the get go IMHO - and we've been 
struggling with fragmented, disjunct gfx concepts since then, but IMHO 
it's getting better gradually, so please don't give up! ;-)

	Ingo

^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-19 12:39 UTC (permalink / raw)
  To: Ingo Molnar
  Cc: One Thousand Gnomes, linux-kernel, Takashi Iwai, Stephen Warren,
	linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <20131219123137.GC18110@gmail.com>

Hi

On Thu, Dec 19, 2013 at 1:31 PM, Ingo Molnar <mingo@kernel.org> wrote:
>
> * David Herrmann <dh.herrmann@gmail.com> wrote:
>
>> Hi
>>
>> On Thu, Dec 19, 2013 at 1:03 AM, One Thousand Gnomes
>> <gnomes@lxorguk.ukuu.org.uk> wrote:
>> >> > That bug always existed, simplefb is just the first driver to hit it
>> >> > (vesafb/efifb didn't use resources). I'm aware of the issue but as a
>> >> > workaround you can simply disable CONFIG_X86_SYSFB. That restores
>> >> > the old behavior.
>> >>
>> >> This looks like a regression, so we'll either need a fix or we'll have
>> >> to mark CONFIG_X86_SYSFB as CONFIG_BROKEN.
>> >
>> > Kernel bugzilla has entries for simplefb breaking both vesafb and
>> > matrox mga.
>>
>> Thanks for the hints. I've read through all I could find and tried
>> to provide some help.
>>
>> I'm kind of confused, most of them enable CONFIG_X86_SYSFB (which is
>> 'n' by default) but don't read the help text. I did my best to tell
>> people that this option requires CONFIG_FB_SIMPLE, but if you don't
>> read the help-text you won't notice that. Don't know what to do
>> about that..
>
> People generally don't read the help text - still the kernel should
> not break. So please the Kconfig angle (and the bootup logic, etc.)
> fool-proof, graphics failures are not fun to debug!

There're dozens of combinations that break gfx-boot. Even non-obvious
things like disabling VT_HW_CONSOLE_BINDING will break gfx-handover.
Anyhow, bad examples are no excuse.. I will send patches to fool-proof
sysfb.

David

^ permalink raw reply
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox