* Re: [PATCH v2] x86: sysfb: remove sysfb when probing real hw
From: David Herrmann @ 2013-12-18 14:15 UTC (permalink / raw)
To: linux-kernel, Pavel Roskin
Cc: Takashi Iwai, Stephen Warren, the arch/x86 maintainers,
linux-fbdev@vger.kernel.org, Geert Uytterhoeven, Ingo Molnar,
David Herrmann
In-Reply-To: <1387374611-12493-1-git-send-email-dh.herrmann@gmail.com>
Hi Pavel
On Wed, Dec 18, 2013 at 2:50 PM, David Herrmann <dh.herrmann@gmail.com> 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, ..).
>
> 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)
As you reported issues regarding nvidiafb, can you give this a try, too?
Thanks
David
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
> - remove simplefb apertures to avoid dead-lock
> - skip sysfb_unregister() if no apertures are set
> - merge sysfb_alloc() into sysfb_register()
> - simplify sysfb_unregister() logic
> - call sysfb_unregister() even if SYSFB=n but X86=y
>
> Tested with efifb and simplefb + i915 handover on x86.
>
> Thanks
> David
>
> arch/x86/include/asm/sysfb.h | 6 ++++
> arch/x86/kernel/sysfb.c | 63 ++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/sysfb_simplefb.c | 9 ++----
> drivers/video/fbmem.c | 19 ++++++++++++
> drivers/video/simplefb.c | 8 -----
> 5 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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;
> };
>
> +__init int 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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
> #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;
> +
> +__init int 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 */
> +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 {
> + 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
* Re: [PATCH v2] x86: sysfb: remove sysfb when probing real hw
From: Takashi Iwai @ 2013-12-18 14:21 UTC (permalink / raw)
To: David Herrmann
Cc: linux-kernel, Stephen Warren, the arch/x86 maintainers,
linux-fbdev, Geert Uytterhoeven, Ingo Molnar
In-Reply-To: <1387374611-12493-1-git-send-email-dh.herrmann@gmail.com>
At Wed, 18 Dec 2013 14:50:11 +0100,
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, ..).
>
> 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)
>
> Signed-off-by: David Herrmann <dh.herrmann@gmail.com>
> ---
> v2:
> - remove simplefb apertures to avoid dead-lock
> - skip sysfb_unregister() if no apertures are set
> - merge sysfb_alloc() into sysfb_register()
> - simplify sysfb_unregister() logic
> - call sysfb_unregister() even if SYSFB=n but X86=y
>
> Tested with efifb and simplefb + i915 handover on x86.
The patch worked fine on QEMU with cirrus driver.
I'd like to see comments, as suggested in my previous mail, in a
couple of places. Other than that, the patch looks good to me.
So, feel free to my tested-by and reviewed-by tags:
Reviewed-by: Takashi Iwai <tiwai@suse.de>
Tested-by: Takashi Iwai <tiwai@suse.de>
thanks,
Takashi
>
> Thanks
> David
>
> arch/x86/include/asm/sysfb.h | 6 ++++
> arch/x86/kernel/sysfb.c | 63 ++++++++++++++++++++++++++++++++++++++++
> arch/x86/kernel/sysfb_simplefb.c | 9 ++----
> drivers/video/fbmem.c | 19 ++++++++++++
> drivers/video/simplefb.c | 8 -----
> 5 files changed, 90 insertions(+), 15 deletions(-)
>
> diff --git a/arch/x86/include/asm/sysfb.h b/arch/x86/include/asm/sysfb.h
> index 2aeb3e2..10d719d 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;
> };
>
> +__init int sysfb_register(const char *name, int id,
> + const struct resource *res, unsigned int res_num,
> + const void *data, size_t data_size);
In most cases, we declare like
int __init sysfb_register(...
> +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..9d8da8d 100644
> --- a/arch/x86/kernel/sysfb.c
> +++ b/arch/x86/kernel/sysfb.c
> @@ -33,11 +33,74 @@
> #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;
> +
> +__init int 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 */
> +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 {
> + 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
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tony Lindgren @ 2013-12-18 17:32 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B14AEE.8090807@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131217 23:14]:
> On 2013-12-17 17:30, Tony Lindgren wrote:
> > * Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 22:47]:
> >> On 2013-12-16 20:41, Tony Lindgren wrote:
> >>> * Tomi Valkeinen <tomi.valkeinen@ti.com> [131216 06:59]:
> >>>> As a temporary solution to enable DSS for selected boards when booting
> >>>> with DT, a hack was added to board-generic.c in
> >>>> 63d5fc0c2f748e20f38a0a0ec1c8494bddf5c288 (OMAP: board-generic: enable
> >>>> DSS for panda & sdp boards).
> >>>>
> >>>> We're now adding proper DT support, so the hack can be removed.
> >>>
> >>> I guess this patch should be merged later on after we have the DT support
> >>> working?
> >>
> >> I'll move this patch also to the end of the series.
> >>
> >> "Merged later" sounds like you mean this could be merged in a separate
> >> series. I think this and the other removal can be in this series, they
> >> just need to be at the end.
> >
> > Well yeah let's keep those separate still as at least Russell needed
> > some more time with the legacy booting. The point we can drop the
> > legacy booting for omap3 may still need to wait a bit, maybe even
> > until v3.15 to keep things working.
>
> They can't be separate. Once I add DT support for a board, I have to
> remove the legacy support for that board. This patch removes legacy
> support for the boards that are converted in the series.
Oh OK yeah makes sense. I was worried about the legacy board-*.c files..
> If I don't remove the legacy support, both DT and legacy side will be
> ran, and both create the DSS devices...
>
> But, it's true that this patch removes the whole file, as all the boards
> currently there are converted. But if new boards are added to the DSS
> quirks, then I can't remove the file. So I'll change this patch to only
> remove the parts for the converted boards, not the whole file.
OK
> But but... If I understand right, the plan is to remove all omap3 board
> files for the next merge window. I'm not totally familiar with the
> quirks system, but how should this be handled:
>
> omap3.dtsi will contain the SoC's DSS nodes. This means that DSS devices
> are created via DT code. But if the display (i.e. panels) for a
> particular omap3 board has not been converted to DT, we should still use
> the legacy DSS initialization. But the DSS is already initialized via DT.
>
> I guess I can set the status for all the DSS nodes to "disabled" in
> omap3.dtsi, and that should prevent DT code from creating the DSS
> devices. Then, each omap3-board.dts that has been converted to DSS DT,
> can override those to "enabled".
>
> That way the DT code should not create DSS devices by default, and the
> quirks system would probably work fine.
No need for DSS related quirks for the DT boot case. I was concerned about
the legacy board-*.c case.
Tony
^ permalink raw reply
* Re: [PATCHv2 01/27] ARM: OMAP: remove DSS DT hack
From: Tony Lindgren @ 2013-12-18 17:33 UTC (permalink / raw)
To: Tomi Valkeinen; +Cc: linux-omap, linux-fbdev, devicetree
In-Reply-To: <52B17729.3040807@ti.com>
* Tomi Valkeinen <tomi.valkeinen@ti.com> [131218 02:22]:
> On 2013-12-18 09:12, Tomi Valkeinen wrote:
>
> >> Well yeah let's keep those separate still as at least Russell needed
> >> some more time with the legacy booting. The point we can drop the
> >> legacy booting for omap3 may still need to wait a bit, maybe even
> >> until v3.15 to keep things working.
> >
> > They can't be separate. Once I add DT support for a board, I have to
> > remove the legacy support for that board. This patch removes legacy
> > support for the boards that are converted in the series.
> >
> > If I don't remove the legacy support, both DT and legacy side will be
> > ran, and both create the DSS devices...
> >
> > But, it's true that this patch removes the whole file, as all the boards
> > currently there are converted. But if new boards are added to the DSS
> > quirks, then I can't remove the file. So I'll change this patch to only
> > remove the parts for the converted boards, not the whole file.
> >
> > But but... If I understand right, the plan is to remove all omap3 board
> > files for the next merge window. I'm not totally familiar with the
> > quirks system, but how should this be handled:
> >
> > omap3.dtsi will contain the SoC's DSS nodes. This means that DSS devices
> > are created via DT code. But if the display (i.e. panels) for a
> > particular omap3 board has not been converted to DT, we should still use
> > the legacy DSS initialization. But the DSS is already initialized via DT.
> >
> > I guess I can set the status for all the DSS nodes to "disabled" in
> > omap3.dtsi, and that should prevent DT code from creating the DSS
> > devices. Then, each omap3-board.dts that has been converted to DSS DT,
> > can override those to "enabled".
> >
> > That way the DT code should not create DSS devices by default, and the
> > quirks system would probably work fine.
>
> I changed the DSS DT nodes to be disabled by default, and I think this
> works nicely. It's actually better this way in any case, as we can leave
> blocks like DSI out for boards that don't need it.
>
> Also, I now remove the quirks only for the boards that are converted to
> DT, not the whole dss-common.c file. So I think we can now add new
> boards to dss-common.c, if and when there are ones that cannot be
> converted to use DSS DT yet.
OK
Tony
^ permalink raw reply
* 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
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