Re: cirrusdrmfb broken with simplefb

[Takashi Iwai <tiwai@suse.de>]

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