* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-18 21:55 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Benoît Cousson, Tony Lindgren, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B089FE.8060704@ti.com>
[-- Attachment #1: Type: text/plain, Size: 1977 bytes --]
On Tue, Dec 17, 2013 at 07:29:34PM +0200, Tomi Valkeinen wrote:
> >> I added N900 display DT support on top of my v2 series, including
> >> pinmuxing. Can you check if it looks right and works?
> >>
> >> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
> >
> > I just tried it and it does not work. On a first look the pinmuxing
> > looks fishy: 0x0d4 is muxed two times.
>
> Hmm, so it is.
>
> I'm not really familiar with SDI, I just muxed all the SDI pins, except
> datapair3. I previously thought that there's only the data and clock
> pairs for SDI, but the TRM revealed more sdi pins, so I included them.
> It is well possible that these can be removed:
>
> 0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
> 0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
> 0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
> 0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
Just removing the dss_data20.sdi_den pin was enough to get a working display. I
don't know if the other pins are needed, because the display pins are already
muxed correctly by the bootloader.
-- Sebastian
diff --git a/arch/arm/boot/dts/omap3-n900.dts b/arch/arm/boot/dts/omap3-n900.dts
index 39e5e50..33f29ac 100644
--- a/arch/arm/boot/dts/omap3-n900.dts
+++ b/arch/arm/boot/dts/omap3-n900.dts
@@ -163,7 +163,7 @@
0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
- 0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
+ //0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
0x0d8 (PIN_OUTPUT | MUX_MODE1) /* dss_data22.sdi_clkp */
0x0da (PIN_OUTPUT | MUX_MODE1) /* dss_data23.sdi_clkn */
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply related
* Re: cirrusdrmfb broken with simplefb
From: One Thousand Gnomes @ 2013-12-19 0:03 UTC (permalink / raw)
To: Ingo Molnar, linux-kernel
Cc: David Herrmann, Takashi Iwai, Stephen Warren,
linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <20131218092941.GA19210@gmail.com>
> > 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.
^ permalink raw reply
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-19 0:51 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, linux-fbdev, devicetree
In-Reply-To: <20131218215536.GA23674@earth.universe>
[-- Attachment #1: Type: text/plain, Size: 2535 bytes --]
On Wed, Dec 18, 2013 at 10:55:37PM +0100, Sebastian Reichel wrote:
> On Tue, Dec 17, 2013 at 07:29:34PM +0200, Tomi Valkeinen wrote:
> > >> I added N900 display DT support on top of my v2 series, including
> > >> pinmuxing. Can you check if it looks right and works?
> > >>
> > >> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
> > >
> > > I just tried it and it does not work. On a first look the pinmuxing
> > > looks fishy: 0x0d4 is muxed two times.
> >
> > Hmm, so it is.
> >
> > I'm not really familiar with SDI, I just muxed all the SDI pins, except
> > datapair3. I previously thought that there's only the data and clock
> > pairs for SDI, but the TRM revealed more sdi pins, so I included them.
> > It is well possible that these can be removed:
> >
> > 0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
> > 0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
> > 0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
> > 0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
>
> Just removing the dss_data20.sdi_den pin was enough to get a working display. I
> don't know if the other pins are needed, because the display pins are already
> muxed correctly by the bootloader.
I just had a look in the leaked n900 schematics. According to it the
following pins are connected to the display:
DSS_DATA20 (E28) GPIO 90 LCD_RST
DSS_DATA10 (AD28) SDI_DAT1N CDP 0
DSS_DATA11 (AD27) SDI_DAT1P CDP 1
DSS_DATA12 (AB28) SDI_DAT2N CDP 2
DSS_DATA13 (AB27) SDI_DAT2P CDP 3
DSS_DATA14 (AA28) SDI_DAT3N CDP 4
DSS_DATA15 (AA27) SDI_DAT3P CDP 5
DSS_DATA22 (AC27) SDI_CLKP CDP 6
DSS_DATA23 (AC28) SDI_CLKN CDP 7
I also noticed that dss_data19.sdi_hsync is used as gpio 89 for the
N900's proximity sensor. Thus I suggest the following SDI pin muxing:
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 */
0x0c8 (PIN_OUTPUT | MUX_MODE1) /* dss_data14.sdi_dat3n */
0x0ca (PIN_OUTPUT | MUX_MODE1) /* dss_data15.sdi_dat3p */
0x0d8 (PIN_OUTPUT | MUX_MODE1) /* dss_data22.sdi_clkp */
0x0da (PIN_OUTPUT | MUX_MODE1) /* dss_data23.sdi_clkn */
>;
};
-- 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: Tomi Valkeinen @ 2013-12-19 5:30 UTC (permalink / raw)
To: Sebastian Reichel
Cc: Benoît Cousson, Tony Lindgren, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
linux-omap, linux-fbdev, devicetree
In-Reply-To: <20131219005151.GA27928@earth.universe>
[-- Attachment #1: Type: text/plain, Size: 3003 bytes --]
On 2013-12-19 02:51, Sebastian Reichel wrote:
> On Wed, Dec 18, 2013 at 10:55:37PM +0100, Sebastian Reichel wrote:
>> On Tue, Dec 17, 2013 at 07:29:34PM +0200, Tomi Valkeinen wrote:
>>>>> I added N900 display DT support on top of my v2 series, including
>>>>> pinmuxing. Can you check if it looks right and works?
>>>>>
>>>>> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
>>>>
>>>> I just tried it and it does not work. On a first look the pinmuxing
>>>> looks fishy: 0x0d4 is muxed two times.
>>>
>>> Hmm, so it is.
>>>
>>> I'm not really familiar with SDI, I just muxed all the SDI pins, except
>>> datapair3. I previously thought that there's only the data and clock
>>> pairs for SDI, but the TRM revealed more sdi pins, so I included them.
>>> It is well possible that these can be removed:
>>>
>>> 0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
>>> 0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
>>> 0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
>>> 0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
>>
>> Just removing the dss_data20.sdi_den pin was enough to get a working display. I
>> don't know if the other pins are needed, because the display pins are already
>> muxed correctly by the bootloader.
>
> I just had a look in the leaked n900 schematics. According to it the
> following pins are connected to the display:
>
> DSS_DATA20 (E28) GPIO 90 LCD_RST
> DSS_DATA10 (AD28) SDI_DAT1N CDP 0
> DSS_DATA11 (AD27) SDI_DAT1P CDP 1
> DSS_DATA12 (AB28) SDI_DAT2N CDP 2
> DSS_DATA13 (AB27) SDI_DAT2P CDP 3
> DSS_DATA14 (AA28) SDI_DAT3N CDP 4
> DSS_DATA15 (AA27) SDI_DAT3P CDP 5
> DSS_DATA22 (AC27) SDI_CLKP CDP 6
> DSS_DATA23 (AC28) SDI_CLKN CDP 7
>
> I also noticed that dss_data19.sdi_hsync is used as gpio 89 for the
> N900's proximity sensor. Thus I suggest the following SDI pin muxing:
>
> 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 */
> 0x0c8 (PIN_OUTPUT | MUX_MODE1) /* dss_data14.sdi_dat3n */
> 0x0ca (PIN_OUTPUT | MUX_MODE1) /* dss_data15.sdi_dat3p */
>
> 0x0d8 (PIN_OUTPUT | MUX_MODE1) /* dss_data22.sdi_clkp */
> 0x0da (PIN_OUTPUT | MUX_MODE1) /* dss_data23.sdi_clkn */
> >;
> };
Thanks, I'll do the modifications. The dat3 lines are not needed, but if
they're connected to the panel, I don't see any harm in muxing them.
Although, makes me wonder. If the panel supports only 2 datalanes, why
does it have connectors for 3? And if it supports 3, why would N900 use
only 2?
Are you able to check if the bootloader muxes dat3 to SDI mode?
Tomi
[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 901 bytes --]
^ permalink raw reply
* [PATCH] video: mx3fb: Allow blocking during framebuffer allocation
From: Sascha Hauer @ 2013-12-19 8:23 UTC (permalink / raw)
To: linux-fbdev
Cc: Sascha Hauer, Jean-Christophe Plagniol-Villard, Tomi Valkeinen,
linux-kernel
No need to allocate the framebuffer from the atomic pool, we are not
in interrupt context. Adding GFP_KERNEL to the framebuffer allocation
allows to use the much bigger CMA pool to allocate the framebuffer.
Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
Cc: Jean-Christophe Plagniol-Villard <plagnioj@jcrosoft.com>
Cc: Tomi Valkeinen <tomi.valkeinen@ti.com>
Cc: linux-fbdev@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
---
drivers/video/mx3fb.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/drivers/video/mx3fb.c b/drivers/video/mx3fb.c
index cfdb380..c882bec 100644
--- a/drivers/video/mx3fb.c
+++ b/drivers/video/mx3fb.c
@@ -1263,7 +1263,7 @@ static int mx3fb_map_video_memory(struct fb_info *fbi, unsigned int mem_len,
fbi->screen_base = dma_alloc_writecombine(fbi->device,
mem_len,
- &addr, GFP_DMA);
+ &addr, GFP_DMA | GFP_KERNEL);
if (!fbi->screen_base) {
dev_err(fbi->device, "Cannot allocate %u bytes framebuffer memory\n",
--
1.8.5.1
^ permalink raw reply related
* Re: [PATCH 0/4] OMAPDSS: DT support for N900 panel
From: Sebastian Reichel @ 2013-12-19 10:08 UTC (permalink / raw)
To: Tomi Valkeinen
Cc: Benoît Cousson, Tony Lindgren, Rob Herring, Pawel Moll,
Mark Rutland, Stephen Warren, Ian Campbell, Rob Landley,
linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B2848A.80807@ti.com>
[-- Attachment #1: Type: text/plain, Size: 3487 bytes --]
On Thu, Dec 19, 2013 at 07:30:50AM +0200, Tomi Valkeinen wrote:
> On 2013-12-19 02:51, Sebastian Reichel wrote:
> > On Wed, Dec 18, 2013 at 10:55:37PM +0100, Sebastian Reichel wrote:
> >> On Tue, Dec 17, 2013 at 07:29:34PM +0200, Tomi Valkeinen wrote:
> >>>>> I added N900 display DT support on top of my v2 series, including
> >>>>> pinmuxing. Can you check if it looks right and works?
> >>>>>
> >>>>> git://git.kernel.org/pub/scm/linux/kernel/git/tomba/linux.git work/dss-dt
> >>>>
> >>>> I just tried it and it does not work. On a first look the pinmuxing
> >>>> looks fishy: 0x0d4 is muxed two times.
> >>>
> >>> Hmm, so it is.
> >>>
> >>> I'm not really familiar with SDI, I just muxed all the SDI pins, except
> >>> datapair3. I previously thought that there's only the data and clock
> >>> pairs for SDI, but the TRM revealed more sdi pins, so I included them.
> >>> It is well possible that these can be removed:
> >>>
> >>> 0x0d0 (PIN_OUTPUT | MUX_MODE1) /* dss_data18.sdi_vsync */
> >>> 0x0d2 (PIN_OUTPUT | MUX_MODE1) /* dss_data19.sdi_hsync */
> >>> 0x0d4 (PIN_OUTPUT | MUX_MODE1) /* dss_data20.sdi_den */
> >>> 0x0d6 (PIN_OUTPUT | MUX_MODE1) /* dss_data21.sdi_stp */
> >>
> >> Just removing the dss_data20.sdi_den pin was enough to get a working display. I
> >> don't know if the other pins are needed, because the display pins are already
> >> muxed correctly by the bootloader.
> >
> > I just had a look in the leaked n900 schematics. According to it the
> > following pins are connected to the display:
> >
> > DSS_DATA20 (E28) GPIO 90 LCD_RST
> > DSS_DATA10 (AD28) SDI_DAT1N CDP 0
> > DSS_DATA11 (AD27) SDI_DAT1P CDP 1
> > DSS_DATA12 (AB28) SDI_DAT2N CDP 2
> > DSS_DATA13 (AB27) SDI_DAT2P CDP 3
> > DSS_DATA14 (AA28) SDI_DAT3N CDP 4
> > DSS_DATA15 (AA27) SDI_DAT3P CDP 5
> > DSS_DATA22 (AC27) SDI_CLKP CDP 6
> > DSS_DATA23 (AC28) SDI_CLKN CDP 7
> >
> > I also noticed that dss_data19.sdi_hsync is used as gpio 89 for the
> > N900's proximity sensor. Thus I suggest the following SDI pin muxing:
> >
> > 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 */
> > 0x0c8 (PIN_OUTPUT | MUX_MODE1) /* dss_data14.sdi_dat3n */
> > 0x0ca (PIN_OUTPUT | MUX_MODE1) /* dss_data15.sdi_dat3p */
> >
> > 0x0d8 (PIN_OUTPUT | MUX_MODE1) /* dss_data22.sdi_clkp */
> > 0x0da (PIN_OUTPUT | MUX_MODE1) /* dss_data23.sdi_clkn */
> > >;
> > };
>
> Thanks, I'll do the modifications. The dat3 lines are not needed, but if
> they're connected to the panel, I don't see any harm in muxing them.
>
> Although, makes me wonder. If the panel supports only 2 datalanes, why
> does it have connectors for 3? And if it supports 3, why would N900 use
> only 2?
I wondered about the same and I also assumed, that the muxing should be
safe.
> 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.
I tried to cat /sys/kernel/debug/pinctrl/48002030.pinmux/pins, but I
get an external abort on non-linefetch.
So I can't check it :(
-- Sebastian
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]
^ permalink raw reply
* [PATCH v3] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-19 10:13 UTC (permalink / raw)
To: linux-kernel
Cc: Takashi Iwai, Stephen Warren, the arch/x86 maintainers,
linux-fbdev, Geert Uytterhoeven, Ingo Molnar, David Herrmann,
stable
In-Reply-To: <1387374611-12493-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> # 3.11+
Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
---
v3: add two comments to explain how sysfb_unregister() works
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..590a46a 100644
--- 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
/*
* 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;
+
+#if IS_ENABLED(CONFIG_X86)
+ 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(®istration_lock);
do_remove_conflicting_framebuffers(a, name, primary);
mutex_unlock(®istration_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(®istration_lock);
ret = do_register_framebuffer(fb_info);
mutex_unlock(®istration_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: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-19 10:46 UTC (permalink / raw)
To: One Thousand Gnomes
Cc: Ingo Molnar, linux-kernel, Takashi Iwai, Stephen Warren,
linux-fbdev@vger.kernel.org, the arch/x86 maintainers
In-Reply-To: <20131219000346.0f722b65@alan.etchedpixels.co.uk>
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..
Thanks
David
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Takashi Iwai @ 2013-12-19 11:06 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: <CANq1E4Q3KiLMC-ETFs+FAmTv3yWdCyqG2c+KzGFX4xZmcxLbJg@mail.gmail.com>
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
Takashi
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: Ingo Molnar @ 2013-12-19 12:31 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: <CANq1E4Q3KiLMC-ETFs+FAmTv3yWdCyqG2c+KzGFX4xZmcxLbJg@mail.gmail.com>
* 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!
Thanks,
Ingo
^ permalink raw reply
* Re: cirrusdrmfb broken with simplefb
From: David Herrmann @ 2013-12-19 12:36 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: <s5h61qlt7zm.wl%tiwai@suse.de>
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.
How about adding "depends on (FB_SIMPLE = y)" for X86_SYSFB? I guess I
will try that and send a patch.
Thanks
David
^ 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
* 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: 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: 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
* [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: [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
* 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: 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: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: [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: [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: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
* [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 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
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox