[PATCH] fbdev: Fix usage of blank value passed to fb_blank

[PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Antonino A. Daplas]

The fb_blank() hook accepts 5 blanking levels which include 1 level designed
for console use only.  Userspace is only aware of 4 levels. Thus, it's
possible for userspace to request VESA_VSYNC_SUSPEND which, in turn, is
interpreted by the fb driver as a request for FB_BLANK_NORMAL. A few drivers
return -EINVAL for this, confusing userspace apps that the driver may not
have VESA blanking support.

Fix by incrementing the blank value by one if the request originated from
userspace.

Signed-off-by: Antonino Daplas <adaplas@pol.net>
---

Ingo Oeser wrote:
> > May I suggest to hide this implementation detail?
> > 

Yes.  The change will be invisible to drivers.

Unfortunately, this may cause some userland breakage.  X should work.
However, some apps may have been developed that uses the FB_BLANK constants
(DirectFB?).  In these cases, they'll get a deeper blank level instead, so it
probably won't affect them significantly.  A follow up patch that hides the 
FB_BLANK constants may be worthwhile.

 drivers/video/fbmem.c |   13 +++++++++++++
 1 files changed, 13 insertions(+), 0 deletions(-)

diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
index d2dede6..5bed0fb 100644
--- a/drivers/video/fbmem.c
+++ b/drivers/video/fbmem.c
@@ -843,6 +843,19 @@ fb_blank(struct fb_info *info, int blank
 {	
  	int ret = -EINVAL;
 
+	/*
+	 * The framebuffer core supports 5 blanking levels (FB_BLANK), whereas
+	 * VESA defined only 4.  The extra level, FB_BLANK_NORMAL, is a
+	 * console invention and is not related to power management.
+	 * Unfortunately, fb_blank callers, especially X, pass VESA constants
+	 * leading to undefined behavior.
+	 *
+	 * To avoid confusion, fb_blank will assume VESA constants if coming
+	 * from userspace, and FB_BLANK constants if coming from the kernel.
+	 */
+	if (info->flags & FBINFO_MISC_USEREVENT && blank)
+		blank++;
+
  	if (blank > FB_BLANK_POWERDOWN)
  		blank = FB_BLANK_POWERDOWN;
 



-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Andrew Morton]

"Antonino A. Daplas" <adaplas@gmail.com> wrote:
>
> Unfortunately, this may cause some userland breakage.  X should work.
>  However, some apps may have been developed that uses the FB_BLANK constants
>  (DirectFB?).  In these cases, they'll get a deeper blank level instead, so it
>  probably won't affect them significantly.  A follow up patch that hides the 
>  FB_BLANK constants may be worthwhile.

Would prefer to avoid any userland breakage.  Is this followup patch
happening?

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Antonino A. Daplas]

Andrew Morton wrote:
> "Antonino A. Daplas" <adaplas@gmail.com> wrote:
>> Unfortunately, this may cause some userland breakage.  X should work.
>>  However, some apps may have been developed that uses the FB_BLANK constants
>>  (DirectFB?).  In these cases, they'll get a deeper blank level instead, so it
>>  probably won't affect them significantly.  A follow up patch that hides the 
>>  FB_BLANK constants may be worthwhile.
> 
> Would prefer to avoid any userland breakage.  Is this followup patch
> happening?

I'm still doing research on which major fb apps depend on the FB_BLANK constants, but
my guess is that the follow up patch is inevitable. We cannot avoid bending over
backwards for X :(

Tony

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Ville Syrjälä]

On Sun, Jan 29, 2006 at 10:18:19AM +0800, Antonino A. Daplas wrote:
> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> index d2dede6..5bed0fb 100644
> --- a/drivers/video/fbmem.c
> +++ b/drivers/video/fbmem.c
> @@ -843,6 +843,19 @@ fb_blank(struct fb_info *info, int blank
>  {	
>   	int ret = -EINVAL;
>  
> +	/*
> +	 * The framebuffer core supports 5 blanking levels (FB_BLANK), whereas
> +	 * VESA defined only 4.  The extra level, FB_BLANK_NORMAL, is a
> +	 * console invention and is not related to power management.
> +	 * Unfortunately, fb_blank callers, especially X, pass VESA constants
> +	 * leading to undefined behavior.

Since when? X.Org uses numbers 0,2,3,4 which match the FB_BLANK 
constants not the VESA constants.

-- 
Ville Syrjälä
syrjala@sci.fi
http://www.sci.fi/~syrjala/


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid\x103432&bid#0486&dat\x121642

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Antonino A. Daplas]

Ville Syrjälä wrote:
> On Sun, Jan 29, 2006 at 10:18:19AM +0800, Antonino A. Daplas wrote:
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index d2dede6..5bed0fb 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -843,6 +843,19 @@ fb_blank(struct fb_info *info, int blank
>>  {	
>>   	int ret = -EINVAL;
>>  
>> +	/*
>> +	 * The framebuffer core supports 5 blanking levels (FB_BLANK), whereas
>> +	 * VESA defined only 4.  The extra level, FB_BLANK_NORMAL, is a
>> +	 * console invention and is not related to power management.
>> +	 * Unfortunately, fb_blank callers, especially X, pass VESA constants
>> +	 * leading to undefined behavior.
> 
> Since when? X.Org uses numbers 0,2,3,4 which match the FB_BLANK 
> constants not the VESA constants.
> 

Hmm, you're correct... But I do remember that particular error before,
about FBIOBLANK returning -EINVAL. I think it may be coming from 
this particular code


Bool
fbdevHWSaveScreen(ScreenPtr pScreen, int mode)
{
	ScrnInfoPtr pScrn = xf86Screens[pScreen->myNum];
	fbdevHWPtr fPtr = FBDEVHWPTR(pScrn);
	unsigned long unblank;

	TRACE_ENTER("HWSaveScreen");
	if (!pScrn->vtSema)
		return TRUE;

	unblank = xf86IsUnblank(mode);

	if (-1 == ioctl(fPtr->fd, FBIOBLANK, (void *)(1-unblank))) {
		xf86DrvMsg(pScrn->scrnIndex, X_ERROR,
			   "FBIOBLANK: %s\n", strerror(errno));
		return FALSE;
	}

	return TRUE;
}

The FBIOBLANK ioctl call passes only 2 possible values, 0 and 1
(1-unblank).

Andrew, 

I think you should drop this patch, Xorg/XFree86's fbdevhw DPMS support
does the correct thing. It's only the above that may be producing the
particular error as reported by David. I'm not sure how to fix that
(kernel or X side).


Tony


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Antonino A. Daplas]

Ville Syrjälä wrote:
> On Sun, Jan 29, 2006 at 10:18:19AM +0800, Antonino A. Daplas wrote:
>> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
>> index d2dede6..5bed0fb 100644
>> --- a/drivers/video/fbmem.c
>> +++ b/drivers/video/fbmem.c
>> @@ -843,6 +843,19 @@ fb_blank(struct fb_info *info, int blank
>>  {	
>>   	int ret = -EINVAL;
>>  
>> +	/*
>> +	 * The framebuffer core supports 5 blanking levels (FB_BLANK), whereas
>> +	 * VESA defined only 4.  The extra level, FB_BLANK_NORMAL, is a
>> +	 * console invention and is not related to power management.
>> +	 * Unfortunately, fb_blank callers, especially X, pass VESA constants
>> +	 * leading to undefined behavior.
> 
> Since when? X.Org uses numbers 0,2,3,4 which match the FB_BLANK 
> constants not the VESA constants.
> 

How about if we silently convert FB_BLANK_NORMAL requests to
FB_BLANK_VSYNC_SUSPEND, would that work?

Tony

PS: Soft blanking is very difficult, if not impossible, to
implement correctly kernel-side, so we can either fail (current
code), silently fail but return success, or convert to the next
blank level.


-------------------------------------------------------
This SF.net email is sponsored by: Splunk Inc. Do you grep through log files
for problems?  Stop!  Download the new AJAX search engine that makes
searching your log files as easy as surfing the  web.  DOWNLOAD SPLUNK!
http://sel.as-us.falkag.net/sel?cmd=lnk&kid=103432&bid=230486&dat=121642

Re: [PATCH] fbdev: Fix usage of blank value passed to fb_blank

[Geert Uytterhoeven]

[-- Warning: decoded text below may be mangled, UTF-8 assumed --]
[-- Attachment #1: Type: TEXT/PLAIN; charset=UTF-8, Size: 1390 bytes --]

On Mon, 30 Jan 2006, Antonino A. Daplas wrote:
> Ville Syrjälä wrote:
> > On Sun, Jan 29, 2006 at 10:18:19AM +0800, Antonino A. Daplas wrote:
> >> diff --git a/drivers/video/fbmem.c b/drivers/video/fbmem.c
> >> index d2dede6..5bed0fb 100644
> >> --- a/drivers/video/fbmem.c
> >> +++ b/drivers/video/fbmem.c
> >> @@ -843,6 +843,19 @@ fb_blank(struct fb_info *info, int blank
> >>  {	
> >>   	int ret = -EINVAL;
> >>  
> >> +	/*
> >> +	 * The framebuffer core supports 5 blanking levels (FB_BLANK), whereas
> >> +	 * VESA defined only 4.  The extra level, FB_BLANK_NORMAL, is a
> >> +	 * console invention and is not related to power management.
> >> +	 * Unfortunately, fb_blank callers, especially X, pass VESA constants
> >> +	 * leading to undefined behavior.
> > 
> > Since when? X.Org uses numbers 0,2,3,4 which match the FB_BLANK 
> > constants not the VESA constants.
> > 
> 
> How about if we silently convert FB_BLANK_NORMAL requests to
> FB_BLANK_VSYNC_SUSPEND, would that work?

Do all (old, pre-VESA) monitors like it when vsync goes away?

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org

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