Re: [PATCH v3] x86: sysfb: remove sysfb when probing real hw

Hi

On Thu, Dec 19, 2013 at 6:09 PM, Ingo Molnar [off-list ref] wrote:

* David Herrmann [off-list ref] wrote:

quoted

Hi

On Thu, Dec 19, 2013 at 5:36 PM, Ingo Molnar [off-list ref] wrote:

quoted

* David Herrmann [off-list ref] wrote:

quoted

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

quoted

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

Ditto.

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

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

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

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

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

quoted

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

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

Ok.

Thanks
David