fbdev: workaround for broken X servers

Hi !

XFree (and X.org) has both always been broken in their conversion of the DPMS blanking mode when passing to the kernel ioctl FBIOBLANK (in fbdev mode).

This patch makes sure that at least the value passed by userland stays in the legal range, which has the side effect that an X DPMS POWERDOWN request will result as an fbdev VESA_POWERDOWN request at the fbdev level now, instead of an out-of-range value.

Signed-off-by Benjamin Herrenschmidt <[email protected]>

Index: linux-work/drivers/video/fbmem.c
===================================================================

drivers/video/fbmem.c line 756

Quoted 8 lines · Benjamin Herrenschmidt

Hi !

drivers/video/fbmem.c +4 -0

+	/* Workaround for broken X servers */
+	if (blank > VESA_POWERDOWN)
+		blank = VESA_POWERDOWN;
+
 	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
 		return 0;

This goes against documented behaviour (see fb.h). Also drivers/char/vt.c seems to agree with fb.h, as do at least atyfb and matroxfb. I also used the vesa level+1 convention in DirectFB. So AFAICS this patch would confuse console power management, XFree86/X.Org, DirectFB and some fb drivers.

Quoted 15 lines · Ville Syrjälä

drivers/video/fbmem.c line 756

Quoted 8 lines · Benjamin Herrenschmidt

Hi !

drivers/video/fbmem.c +4 -0

+	/* Workaround for broken X servers */
+	if (blank > VESA_POWERDOWN)
+		blank = VESA_POWERDOWN;
+
 	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
 		return 0;

This goes against documented behaviour (see fb.h). Also drivers/char/vt.c seems to agree with fb.h, as do at least atyfb and matroxfb. I also used the vesa level+1 convention in DirectFB. So AFAICS this patch would confuse console power management, XFree86/X.Org, DirectFB and some fb drivers.

True enough. The convention is if the display is active, then suspend. If it is suspended, then powerdown. And if already powered down, then do nothing. It does so by by sending a blank flag of VESA_POWERDOWN + 1.

This is the convention used by the vt layer, I don't what is used by user apps.

The above patch won't break console power management as the console calls fbcon_blank(), not fb_blank().

Quoted 6 lines · Antonino A. Daplas

Quoted 2 lines · Ville Syrjälä

Quoted 1 line · Benjamin Herrenschmidt

Hi !

The above patch won't break console power management as the console calls fbcon_blank(), not fb_blank().

But fbcon_blank() calls fb_blank() does it not?

Quoted 9 lines

Index: linux-work/drivers/video/fbmem.c
===================================================================

drivers/video/fbmem.c +0 -0

@@ -748,6 +748,10 @@
 	u16 *black = NULL;
 	int err = 0;
 

drivers/video/fbmem.c line 757

Quoted 7 lines

drivers/video/fbmem.c +4 -0

+	/* Workaround for broken X servers */
+	if (blank > VESA_POWERDOWN)
+		blank = VESA_POWERDOWN;
+
 	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
 		return 0;
 

This goes against documented behaviour (see fb.h). Also drivers/char/vt.c seems to agree with fb.h, as do at least atyfb and matroxfb. I also used the vesa level+1 convention in DirectFB. So AFAICS this patch would confuse console power management, XFree86/X.Org, DirectFB and some fb drivers.

Quoted 8 lines

Quoted 8 lines

Index: linux-work/drivers/video/fbmem.c
===================================================================

drivers/video/fbmem.c +0 -0

@@ -748,6 +748,10 @@
 	u16 *black = NULL;
 	int err = 0;
 

Quoted 13 lines

drivers/video/fbmem.c line 757

Quoted 7 lines

drivers/video/fbmem.c +4 -0

+	/* Workaround for broken X servers */
+	if (blank > VESA_POWERDOWN)
+		blank = VESA_POWERDOWN;
+
 	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
 		return 0;
 

This goes against documented behaviour (see fb.h). Also drivers/char/vt.c seems to agree with fb.h, as do at least atyfb and matroxfb. I also used the vesa level+1 convention in DirectFB. So AFAICS this patch would confuse console power management, XFree86/X.Org, DirectFB and some fb drivers.

Ok, can we agree here on what has to be done ? X is hard-coding numbers that don't seem to be make any sense vs. our constants. In radeonfb, I used switch case on constants, not +1, so there is something I'm not getting. Besides, radeon always try to power down TFT panels when blank != 0, maybe I should improve that ... What is this +1 thing, where does it comes from ?

Quoted 9 lines

Quoted 9 lines

Index: linux-work/drivers/video/fbmem.c
===================================================================

drivers/video/fbmem.c +0 -0

@@ -748,6 +748,10 @@
 	u16 *black = NULL;
 	int err = 0;
 

Quoted 13 lines

drivers/video/fbmem.c line 757

Quoted 7 lines

drivers/video/fbmem.c +4 -0

+	/* Workaround for broken X servers */
+	if (blank > VESA_POWERDOWN)
+		blank = VESA_POWERDOWN;
+
 	if (info->fbops->fb_blank && !info->fbops->fb_blank(blank, info))
 		return 0;
 

This goes against documented behaviour (see fb.h). Also drivers/char/vt.c seems to agree with fb.h, as do at least atyfb and matroxfb. I also used the vesa level+1 convention in DirectFB. So AFAICS this patch would confuse console power management, XFree86/X.Org, DirectFB and some fb drivers.

Ok, can we agree here on what has to be done ? X is hard-coding numbers that don't seem to be make any sense vs. our constants. In radeonfb, I used switch case on constants, not +1, so there is something I'm not getting. Besides, radeon always try to power down TFT panels when blank != 0, maybe I should improve that ... What is this +1 thing, where does it comes from ?

Quoted 6 lines · Benjamin Herrenschmidt

Ok, can we agree here on what has to be done ? X is hard-coding numbers that don't seem to be make any sense vs. our constants. In radeonfb, I used switch case on constants, not +1, so there is something I'm not getting. Besides, radeon always try to power down TFT panels when blank != 0, maybe I should improve that ... What is this +1 thing, where does it comes from ?

Maybe it's there because someone wanted to blank the display without disabling sync signals.

I suggest a new set of constants that cover all legal values. Here's the first drawft.

include/linux/fb.h +1 -1

@@ -27,7 +27,7 @@
 /* #define FBIOSWITCH_MONIBIT	0x460E */
 #define FBIOGET_CON2FBMAP	0x460F
 #define FBIOPUT_CON2FBMAP	0x4610
-#define FBIOBLANK		0x4611		/* arg: 0 or vesa level + 1 */
+#define FBIOBLANK		0x4611		/* see FB_BLANK_* */
 #define FBIOGET_VBLANK		_IOR('F', 0x12, struct fb_vblank)
 #define FBIO_ALLOC              0x4613
 #define FBIO_FREE               0x4614

Quoted 10 lines · On Fri, 5 Nov 2004, Ville [iso-8859-1] Syrjälä wrote:

Quoted 6 lines · Benjamin Herrenschmidt

Ok, can we agree here on what has to be done ? X is hard-coding numbers that don't seem to be make any sense vs. our constants. In radeonfb, I used switch case on constants, not +1, so there is something I'm not getting. Besides, radeon always try to power down TFT panels when blank != 0, maybe I should improve that ... What is this +1 thing, where does it comes from ?

Maybe it's there because someone wanted to blank the display without disabling sync signals.

Yes of course! (/me wasn't completely awake yet)

Quoted 15 lines

I suggest a new set of constants that cover all legal values. Here's the first drawft.

include/linux/fb.h +1 -1

@@ -27,7 +27,7 @@
 /* #define FBIOSWITCH_MONIBIT	0x460E */
 #define FBIOGET_CON2FBMAP	0x460F
 #define FBIOPUT_CON2FBMAP	0x4610
-#define FBIOBLANK		0x4611		/* arg: 0 or vesa level + 1 */
+#define FBIOBLANK		0x4611		/* see FB_BLANK_* */
 #define FBIOGET_VBLANK		_IOR('F', 0x12, struct fb_vblank)
 #define FBIO_ALLOC              0x4613
 #define FBIO_FREE               0x4614

include/linux/fb.h line 266

Quoted 9 lines

include/linux/fb.h +6 -0

@@ -258,6 +258,12 @@
 #define VESA_HSYNC_SUSPEND      2
 #define VESA_POWERDOWN          3
 
+#define FB_BLANK_NO_BLANKING	0
+#define FB_BLANK_BLANKING	1
+#define FB_BLANK_VSYNC_SUSPEND	2
+#define FB_BLANK_HSYNC_SUSPEND	3
+#define FB_BLANK_POWERDOWN	4
+

Yep, these nicely cover the current intended behavior.

But every existing application that uses (shiver) the kernel headers will break after this change...

But every existing application that uses (shiver) the kernel headers will break after this change...

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

Quoted 7 lines · Geert Uytterhoeven

Quoted 3 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

Quoted 5 lines

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

And make it black is usually done in software (by memsetting the screen with the black color).

Quoted 15 lines · Antonino A. Daplas

Quoted 13 lines · Geert Uytterhoeven

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

Quoted 2 lines

And make it black is usually done in software (by memsetting the screen with the black color).

Or by setting all color palette registers to black, if available, since it's cheaper...

Quoted 6 lines

Quoted 2 lines

And make it black is usually done in software (by memsetting the screen with the black color).

Or by setting all color palette registers to black, if available, since it's cheaper...

Ok. So, if pseudocolor/directcolor, set palette to all black, otherwise memset the screen.

Quoted 8 lines · Antonino A. Daplas

Quoted 5 lines

Quoted 2 lines

And make it black is usually done in software (by memsetting the screen with the black color).

Or by setting all color palette registers to black, if available, since it's cheaper...

Ok. So, if pseudocolor/directcolor, set palette to all black, otherwise memset the screen.

Still not there -- some RAMDACs can be asked to do blanking themselves.

And there are these interesting implementations like the Bt463 that have a palette even though using the truecolor mode.

Quoted 2 lines

And there are these interesting implementations like the Bt463 that have a palette even though using the truecolor mode.

Doesn't that one do directcolor instead of truecolor?

And make it black is usually done in software (by memsetting the screen with the black color).

So why not this: In fbcon_blank(), if the blank parameter is:

0 - unblank the screen, and call info->fbops->fb_blank(VESA_NO_BLANKING),
then return a nonzero value to force a redraw.
1,2,3,4 - clear the screen with all black
2,3,4 - call info->fbops->fb_blank(blank - 1);

Similarly, we can do the same thing in fbmem.c:fb_blank(), but without the software blanking.

Drivers will just need to follow the VESA_* defines in fb.h (which happens to be also defined in console.h)

Quoted 10 lines · Antonino A. Daplas

And make it black is usually done in software (by memsetting the screen with the black color).

So why not this: In fbcon_blank(), if the blank parameter is:

0 - unblank the screen, and call info->fbops->fb_blank(VESA_NO_BLANKING),
then return a nonzero value to force a redraw.
1,2,3,4 - clear the screen with all black
2,3,4 - call info->fbops->fb_blank(blank - 1);

Quoted 5 lines

Similarly, we can do the same thing in fbmem.c:fb_blank(), but without the software blanking.

Drivers will just need to follow the VESA_* defines in fb.h (which happens to be also defined in console.h)

Ah... so radeonfb is correct then, and fb_blank() is not ? X is definitely passing +1 values it seems.

Currently, fbcon_blank() does the first method (pass blank unchanged), which means radeonfb is wrong. Adopting the second method will make radeonfb do the right thing, but will require changes to a lot of drivers.

So, what will it be then?

Quoted 6 lines · Antonino A. Daplas

Currently, fbcon_blank() does the first method (pass blank unchanged), which means radeonfb is wrong. Adopting the second method will make radeonfb do the right thing, but will require changes to a lot of drivers.

So, what will it be then?

I'd rather define FB_BLANK_* constants and convert all drivers... the conversion is trivial.

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

The Story About The Breakage
----------------------------
Every application that passes VESA_*+1 will break when recompiled, since (most of) the VESA_* values are incremented by 1 by the patch.

Quoted 14 lines · Geert Uytterhoeven

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

Quoted 4 lines

The Story About The Breakage
----------------------------
Every application that passes VESA_*+1 will break when recompiled, since (most of) the VESA_* values are incremented by 1 by the patch.

My proposed patch doesn't touch the VESA_* values. It just adds new constants which match the VESA_*+1 convention. That way old apps will continue to work and developers don't have to guess what values are actully legal.

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

The Story About The Breakage
----------------------------
Every application that passes VESA_*+1 will break when recompiled, since (most of) the VESA_* values are incremented by 1 by the patch.

Gr{oetje,eeting}s,

Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- [email protected]

In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that.
-- Linus Torvalds

Quoted 6 lines · Geert Uytterhoeven

Quoted 4 lines · Benjamin Herrenschmidt

Quoted 2 lines

But every existing application that uses (shiver) the kernel headers will break after this change...

Geert, can you explain the whole story please ?

Quoted 7 lines

The story about the +1 or the story about the breakage?

The Story About The +1
----------------------
Since the VESA levels do not provide a way to blank (`make it black') the screen, the +1 is introduced. Hence 0 means unblank, 1 means black screen, 2 means lowest power save level, and so on...

So why don't we have a nice set of #define's or an enum at least describing those ? :)

Quoted 4 lines

The Story About The Breakage
----------------------------
Every application that passes VESA_*+1 will break when recompiled, since (most of) the VESA_* values are incremented by 1 by the patch.

What patch ? Mine ? It doesn't increment the VESA values, it just clamps the max. But then, everything seem to be totally inconsistent. So can we instead define a set of FB_BLANK_**** values to use and have fbcon convert VESA->FB_BLANK and FBIOBLANK pass FB_BLANK_* as-is, thus the drivers would switch/case on those and no more +1 games ?

Maybe it's there because someone wanted to blank the display without disabling sync signals.

Quoted 2 lines

Maybe it's there because someone wanted to blank the display without disabling sync signals.

Is there a point doing so ?