[TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[Antonino Daplas]

Hi, 

Here's a diff to correct several small things that escaped through the
cracks.

1.  The YNOMOVE scrollmode for non-accelerated drivers is just very slow
because of a lot of block moves (leads to slow and jerky scrolling in
vesafb with ypanning enabled).  Depending on var->accel_flags, set the 
scrollmode to either YREDRAW or YNOMOVE. For drivers with hardware
acceleration, set var->accel_flags to nonzero for max speed.

2.  fb_pan_display() always returns an error.  User apps will complain.

3.  case FBIO_GETCMAP in fb_ioctl does not return immediately.  User
apps will complain.

4.  vgastate.c is not saving the correct blocks.

5.  logo drawing for monochrome displays is just incorrect.(alterations
were done by eyeballing only, no hardware for testing).

The above will only have a very small effect on the general state of
fbdev/fbcon.  Patch is against 2.5.51.

Tony 

diff -Naur linux-2.5.51/drivers/video/console/fbcon.c linux/drivers/video/console/fbcon.c
--- linux-2.5.51/drivers/video/console/fbcon.c	2002-12-10 21:55:41.000000000 +0000
+++ linux/drivers/video/console/fbcon.c	2002-12-10 21:44:46.000000000 +0000
@@ -291,7 +291,10 @@
 	struct display *display = fb_display + con;
 
 	display->can_soft_blank = info->fbops->fb_blank ? 1 : 0;
-	display->scrollmode = SCROLL_YNOMOVE;
+	if (info->var.accel_flags)
+		display->scrollmode = SCROLL_YNOMOVE;
+	else
+		display->scrollmode = SCROLL_YREDRAW;
 	fbcon_changevar(con);
 	return;
 }
@@ -2633,7 +2636,7 @@
 	default:
 		for (i = 0; i < (LOGO_W * LOGO_H)/8; i++) 
 			for (j = 0; j < 8; j++) 
-				logo[i*2] = (linux_logo_bw[i] &  (7 - j)) ? 
+				logo[i*8+j] = (linux_logo_bw[i] &  (7 - j)) ? 
 					((needs_logo == 1) ? 1 : 0) :
 					((needs_logo == 1) ? 0 : 1);
 				
diff -Naur linux-2.5.51/drivers/video/fbmem.c linux/drivers/video/fbmem.c
--- linux-2.5.51/drivers/video/fbmem.c	2002-12-10 21:55:15.000000000 +0000
+++ linux/drivers/video/fbmem.c	2002-12-10 21:47:22.000000000 +0000
@@ -471,11 +471,9 @@
             yoffset + info->var.yres > info->var.yres_virtual)
                 return -EINVAL;
         if (info->fbops->fb_pan_display) {
-                if ((err = info->fbops->fb_pan_display(var, info)))
-                        return err;
-                else
-                        return -EINVAL;
-        }
+		err = info->fbops->fb_pan_display(var, info);
+		if (err) return err;
+	}
         info->var.xoffset = var->xoffset;
         info->var.yoffset = var->yoffset;
         if (var->vmode & FB_VMODE_YWRAP)
@@ -571,6 +569,7 @@
 		if (copy_from_user(&cmap, (void *) arg, sizeof(cmap)))
 			return -EFAULT;
 		fb_copy_cmap(&info->cmap, &cmap, 0);
+		return 0;
 	case FBIOPAN_DISPLAY:
 		if (copy_from_user(&var, (void *) arg, sizeof(var)))
 			return -EFAULT;
diff -Naur linux-2.5.51/drivers/video/vgastate.c linux/drivers/video/vgastate.c
--- linux-2.5.51/drivers/video/vgastate.c	2002-12-10 21:55:20.000000000 +0000
+++ linux/drivers/video/vgastate.c	2002-12-10 21:52:31.000000000 +0000
@@ -111,7 +111,7 @@
 		vga_wgfx(state->vgabase, VGA_GFX_MODE, 0x0);
 		vga_wgfx(state->vgabase, VGA_GFX_MISC, 0x5);
 		for (i = 0; i < 8192; i++) 
-			saved->vga_text[i] = vga_r(fbbase + 2 * 8192, i); 
+			saved->vga_text[8192+i] = vga_r(fbbase, i); 
 	}
 
 	/* restore regs */
@@ -184,7 +184,7 @@
 		vga_wgfx(state->vgabase, VGA_GFX_PLANE_READ, 0x3);
 		vga_wgfx(state->vgabase, VGA_GFX_MODE, 0x0);
 		vga_wgfx(state->vgabase, VGA_GFX_MISC, 0x5);
-		for (i = 0; i < 4 * 8192; i++) 
+		for (i = 0; i < state->memsize; i++) 
 			vga_w(fbbase, i, saved->vga_font1[i]);
 	}
 	
@@ -204,8 +204,7 @@
 		vga_wgfx(state->vgabase, VGA_GFX_MODE, 0x0);
 		vga_wgfx(state->vgabase, VGA_GFX_MISC, 0x5);
 		for (i = 0; i < 8192; i++) 
-			vga_w(fbbase + 2 * 8192, i, 
-			      saved->vga_text[i]);
+			vga_w(fbbase, i, saved->vga_text[8192+i]);
 	}
 
 	/* unblank screen */

Re: [TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[James Simmons]

> Here's a diff to correct several small things that escaped through the
> cracks.

Ug. Now that a wider test base is being done and more drivers actually
compile we are going to see more cracks.

> 1.  The YNOMOVE scrollmode for non-accelerated drivers is just very slow
> because of a lot of block moves (leads to slow and jerky scrolling in
> vesafb with ypanning enabled).  Depending on var->accel_flags, set the
> scrollmode to either YREDRAW or YNOMOVE. For drivers with hardware
> acceleration, set var->accel_flags to nonzero for max speed.

Thanks. I have had several emails complementing the speed improvements.
Another speed boost will be a plus.

> 2.  fb_pan_display() always returns an error.  User apps will complain.

Fixed. Actually I used the following code.

int fb_pan_display(struct fb_var_screeninfo *var, struct fb_info *info)
{
        int xoffset = var->xoffset;
        int yoffset = var->yoffset;
        int err;

        if (xoffset < 0 || yoffset < 0 || info->fbops->fb_pan_display ||
            xoffset + info->var.xres > info->var.xres_virtual ||
            yoffset + info->var.yres > info->var.yres_virtual)
                return -EINVAL;
        if ((err = info->fbops->fb_pan_display(var, info)))
                return err;
        info->var.xoffset = var->xoffset;
        info->var.yoffset = var->yoffset;
        if (var->vmode & FB_VMODE_YWRAP)

instead. The reason is I didn't like the idea of xoffset and yoffset being
changed even if the hardware panning function failed. Comments?

> 3.  case FBIO_GETCMAP in fb_ioctl does not return immediately.  User
> apps will complain.

Fixed.

> 4.  vgastate.c is not saving the correct blocks.

Fixed.

> 5.  logo drawing for monochrome displays is just incorrect.(alterations
> were done by eyeballing only, no hardware for testing).

Will test on hgafb driver.

Re: [TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[Antonino Daplas]

On Wed, 2002-12-11 at 10:59, James Simmons wrote:
> 
> > Here's a diff to correct several small things that escaped through the
> > cracks.
> 
> Ug. Now that a wider test base is being done and more drivers actually
> compile we are going to see more cracks.
> 
> > 1.  The YNOMOVE scrollmode for non-accelerated drivers is just very slow
> > because of a lot of block moves (leads to slow and jerky scrolling in
> > vesafb with ypanning enabled).  Depending on var->accel_flags, set the
> > scrollmode to either YREDRAW or YNOMOVE. For drivers with hardware
> > acceleration, set var->accel_flags to nonzero for max speed.
> 
I think the scrollmode is better determined on a case-to-case basis, but
the above should be a good enough differentiation.

> Thanks. I have had several emails complementing the speed improvements.
> Another speed boost will be a plus.
> 
Yes, probably not much at low pixel depths, but at high pixel depths and
resolutions, 2.5 should be much better than 2.4 (even 2-3x faster in
some cases).  And for drivers with hardware acceleration, whatever the
color depth, it will be much better because they were written
specifically for them. 

Personally, the performance gain is just a minor side-effect. The
greatest benefit in all this is the separation of the fbdev-fbcon
structure into components, where each component has a defined use and
function, and very much independent of each other.  This really makes it
easier to understand the code, port or write drivers too it, write
smaller and more efficient code, and even debug.  And you basically did
all this, so great job :-)  

 > > 2.  fb_pan_display() always returns an error.  User apps will
complain.
> 
> Fixed. Actually I used the following code.
> 
> int fb_pan_display(struct fb_var_screeninfo *var, struct fb_info *info)
> {
>         int xoffset = var->xoffset;
>         int yoffset = var->yoffset;
>         int err;
> 
>         if (xoffset < 0 || yoffset < 0 || info->fbops->fb_pan_display ||
>             xoffset + info->var.xres > info->var.xres_virtual ||
>             yoffset + info->var.yres > info->var.yres_virtual)
>                 return -EINVAL;
>         if ((err = info->fbops->fb_pan_display(var, info)))
>                 return err;
>         info->var.xoffset = var->xoffset;
>         info->var.yoffset = var->yoffset;
>         if (var->vmode & FB_VMODE_YWRAP)
> 
> instead. The reason is I didn't like the idea of xoffset and yoffset being
> changed even if the hardware panning function failed. Comments?
> 
Yes, I think thats better.

Tony

Re: [TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[Petr Vandrovec]

On 10 Dec 02 at 21:59, James Simmons wrote:

> Fixed. Actually I used the following code.
> 
> int fb_pan_display(struct fb_var_screeninfo *var, struct fb_info *info)
> {
>         int xoffset = var->xoffset;
>         int yoffset = var->yoffset;
>         int err;
> 
>         if (xoffset < 0 || yoffset < 0 || info->fbops->fb_pan_display ||

I'm probably missing something important, but do not you want
                                           !info->fbops->fb_pan_display
instead?
                                                            Petr
                                                                                                       
>             xoffset + info->var.xres > info->var.xres_virtual ||
>             yoffset + info->var.yres > info->var.yres_virtual)
>                 return -EINVAL;
>         if ((err = info->fbops->fb_pan_display(var, info)))
>                 return err;
>         info->var.xoffset = var->xoffset;
>         info->var.yoffset = var->yoffset;
>         if (var->vmode & FB_VMODE_YWRAP)

Re: [TRIVIAL PATCH] FBDEV: Small impact patch for fbdev

[James Simmons]

> > int fb_pan_display(struct fb_var_screeninfo *var, struct fb_info *info)
> > {
> >         int xoffset = var->xoffset;
> >         int yoffset = var->yoffset;
> >         int err;
> >
> >         if (xoffset < 0 || yoffset < 0 || info->fbops->fb_pan_display ||
>
> I'm probably missing something important, but do not you want
>                                            !info->fbops->fb_pan_display
> instead?

Oops. Typo to the screen. That wasn't commited :-)