Re: [Linux-fbdev-devel] Comments on fbgen.c and fbcon-accel.c

Re: [Linux-fbdev-devel] Comments on fbgen.c and fbcon-accel.c

[James Simmons]

> I have a few observations on fbgen and fbcon-accel.

Don't mix fbgen with fbcon-accel. The new gen_* stuff in fbgen.c is meant
to replace the old fbgen_* stuff. That is why the below doesn't work.
 
> 1.  fbcon_accel_clear_margins may not work correctly with fbgen since
> fbcon_accel will use the xoffset and yoffset values from info->var.  
> 
>     void fbcon_accel_clear_margins(struct vc_data *vc, struct display
>     *p,
>     			       int bottom_only)
>     {
>     	<<<snip>>> 
>     
>     	if (bh) {
>     		region.dx = info->var.xoffset;
>                     region.dy = info->var.yoffset + bs;
>                     region.width  = rs;
>                     region.height = bh;
>     		info->fbops->fb_fillrect(info, &region);
>     	}
>     }
> 
> However fbgen_pan_display updates the xoffset and yoffset in
> fb_display[con].var. So margins don't get cleared if the driver supports
> y-panning or y-wrapping.
> 
>     int fbgen_pan_display(struct fb_var_screeninfo *var, int con,
>     		      struct fb_info *info)
>     {
>     	<<< snip >>>
>     
>         if (con == info->currcon) {
>     	if (fbhw->pan_display) {
>     	    if ((err = fbhw->pan_display(var, info2)))
>     		return err;
>     	} else
>     	    return -EINVAL;
>         }
>         fb_display[con].var.xoffset = var->xoffset;
>         fb_display[con].var.yoffset = var->yoffset;
>     
>     	<<< snip >>>
>     }

> 2. Also, fbgen_switch basically just do an fbgen_do_set_var()
> (decode_var(), followed by set_par()).  This is okay most times, but
> it's probably better if fbgen_switch also does an encode_fix() since
> fbcon's drawing functions also rely on fix->line_length.

Most likely that is also broken. I haven't thought about it since I plan
to make all the old fbgen_* functions go away.

> If an fb_fix_screeninfo is not updated, display corruption occurs when
> switching to another display with a different pixelformat.

Correct. That is why I require info->fix to be updated when set_par is
called.

Re: [Linux-fbdev-devel] Comments on fbgen.c and fbcon-accel.c

[Antonino Daplas]

On Sat, 2002-05-04 at 05:47, James Simmons wrote:
> 
> > I have a few observations on fbgen and fbcon-accel.
> 
> Don't mix fbgen with fbcon-accel. The new gen_* stuff in fbgen.c is meant
> to replace the old fbgen_* stuff. That is why the below doesn't work.
>  
Okay, I've succeeded in rewriting the i810/i815 driver to use the gen_*
stuff instead of fbgen_*.  As far as I can tell everything works :) --
y-panning, accel, etc -- although gen_update_var() may not work
properly.  I'm still getting incorrect cursor colors in 8 bpp, but
that's probably my fault.  And you're right, it's actually easier to
write the driver using the gen_* stuff.  

> 
> > 2. Also, fbgen_switch basically just do an fbgen_do_set_var()
> > (decode_var(), followed by set_par()).  This is okay most times, but
> > it's probably better if fbgen_switch also does an encode_fix() since
> > fbcon's drawing functions also rely on fix->line_length.
> 
> Most likely that is also broken. I haven't thought about it since I plan
> to make all the old fbgen_* functions go away.
> 
fb_gen_switch may be broken, but I think gen_switch works just okay as
long as info->fix is updated in set_par().

> > If an fb_fix_screeninfo is not updated, display corruption occurs when
> > switching to another display with a different pixelformat.
> 
> Correct. That is why I require info->fix to be updated when set_par is
> called.
> 
Right.

The i810fb patch is at
http://prdownloads.sourceforge.net/i810fb/linux-2.5.13-i810fb.tar.bz2.

Tony

--- fbgen.c.orig	Sat May  4 14:35:32 2002
+++ fbgen.c	Sat May  4 15:02:37 2002
@@ -514,7 +514,8 @@
     
 	if (con == info->currcon) {
 		if (info->fbops->fb_pan_display) {
-			if ((err = info->fbops->fb_pan_display(&info->var, con, info)))
+		        /* Tony: offsets are still in disp->var, not info->var */
+			if ((err = info->fbops->fb_pan_display(&fb_display[con].var, con, info)))
 				return err;
 		}
 	}