Re: cirrusdrmfb broken with simplefb

At Thu, 19 Dec 2013 15:13:59 +0100,
David Herrmann wrote:

Hi

On Thu, Dec 19, 2013 at 3:03 PM, Takashi Iwai [off-list ref] wrote:

quoted

quoted

quoted

quoted

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.

quoted

quoted

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.

quoted

quoted

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.

quoted

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 ;)

quoted

quoted

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