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

Re: Comments on fbgen.c and fbcon-accel.c

[Michel Dänzer]

On Sat, 2002-05-04 at 20:01, Antonino Daplas wrote: 
> On Sat, 2002-05-04 at 05:47, James Simmons wrote:
> > 
> > > I have a few observations on fbgen and fbcon-accel.
> 
> One more thing I've noticed with gen_set_var.  Basically, gen_set_var
> will proceed if it satisfies 2 conditions -- during initialization (con
> < 0) and if the new var is different from the old var.  
> 
> The above is fine if everything is done in the console.  However,
> problems may arise if an app that touches the graphics hardware (ie X)
> is launched.  From the point of view of fbcon, the hardware state hasn't
> changed (compare of newvar with oldvar is false) when display is
> switched back to console.  And if that app did not fully restore the
> hardware state,  we will be left with a corrupted display.
> 
> So, it's probably better if set_par() and pan_display() are allowed to
> proceed unconditionally within gen_set_var.  It might take a few more
> milliseconds to switch consoles each time, but we are assured that the
> hardware state is always coherent with the current var.
> 
> What do you think?

I think this is giving away an advantage. The X server is a bad example
as it can use the framebuffer device fine.

If it's really a problem, maybe we could figure out a way to detect when
it's safe to optimize stuff away or as a last resort make it an option?


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast

_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net

Re: Comments on fbgen.c and fbcon-accel.c

[Antonino Daplas]

On Tue, 2002-05-07 at 06:24, Michel Dänzer wrote:
> On Sat, 2002-05-04 at 20:01, Antonino Daplas wrote: 
> > On Sat, 2002-05-04 at 05:47, James Simmons wrote:
> > > 
> > > > I have a few observations on fbgen and fbcon-accel.
> > 
> > One more thing I've noticed with gen_set_var.  Basically, gen_set_var
> > will proceed if it satisfies 2 conditions -- during initialization (con
> > < 0) and if the new var is different from the old var.  
> > 
> > The above is fine if everything is done in the console.  However,
> > problems may arise if an app that touches the graphics hardware (ie X)
> > is launched.  From the point of view of fbcon, the hardware state hasn't
> > changed (compare of newvar with oldvar is false) when display is
> > switched back to console.  And if that app did not fully restore the
> > hardware state,  we will be left with a corrupted display.
> > 
> > So, it's probably better if set_par() and pan_display() are allowed to
> > proceed unconditionally within gen_set_var.  It might take a few more
> > milliseconds to switch consoles each time, but we are assured that the
> > hardware state is always coherent with the current var.
> > 
> > What do you think?
> 
> I think this is giving away an advantage. The X server is a bad example
> as it can use the framebuffer device fine.
I was talking of X running with it's own accelerated drivers.  I have
checked most of the X accelerated drivers, and most will just attempt to
restore the VGA registers and some when switching to the console.  I
think this is not just enough, since fb drivers require more than that. 
Simiarly, I have also looked at some of the fbdev drivers that are not
using the gen_* interface (nvidia, ati, matrox), and they will also
unconditially set the hardware during switches/set_var. The
fbgen_switch() function also calls fbhw->set_par() within do_set_var().
> 
> If it's really a problem, maybe we could figure out a way to detect when
> it's safe to optimize stuff away or as a last resort make it an option?
> 
I think if the gen_* interface is to be adopted, it will become a
problem. Detection is the best solution, but right now X and DRI do not
know that fb even exist so we can't get X to detect fb unless we
persuade the X people to do that.  I have tried X detection before by
checking the previous console number.  If the previous number is not a
valid console, we can presume that a non-console app used that.  But
this is not clean and there are too many conditions where this check
will fail. But then, I really don't understand the underlying console
interface, so an easier and more effective way may exist that I don't
know about.

Tony



_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net

Re: Comments on fbgen.c and fbcon-accel.c

[Geert Uytterhoeven]

On 7 May 2002, Antonino Daplas wrote:
> On Tue, 2002-05-07 at 06:24, Michel Dänzer wrote:
> > On Sat, 2002-05-04 at 20:01, Antonino Daplas wrote: 
> > > On Sat, 2002-05-04 at 05:47, James Simmons wrote:
> > > > 
> > > > > I have a few observations on fbgen and fbcon-accel.
> > > 
> > > One more thing I've noticed with gen_set_var.  Basically, gen_set_var
> > > will proceed if it satisfies 2 conditions -- during initialization (con
> > > < 0) and if the new var is different from the old var.  
> > > 
> > > The above is fine if everything is done in the console.  However,
> > > problems may arise if an app that touches the graphics hardware (ie X)
> > > is launched.  From the point of view of fbcon, the hardware state hasn't
> > > changed (compare of newvar with oldvar is false) when display is
> > > switched back to console.  And if that app did not fully restore the
> > > hardware state,  we will be left with a corrupted display.
> > > 
> > > So, it's probably better if set_par() and pan_display() are allowed to
> > > proceed unconditionally within gen_set_var.  It might take a few more
> > > milliseconds to switch consoles each time, but we are assured that the
> > > hardware state is always coherent with the current var.

Which can be visible, especially if you reprogram the PLL...

> > > What do you think?
> > 
> > I think this is giving away an advantage. The X server is a bad example
> > as it can use the framebuffer device fine.

Indeed.

> I was talking of X running with it's own accelerated drivers.  I have
> checked most of the X accelerated drivers, and most will just attempt to
> restore the VGA registers and some when switching to the console.  I
> think this is not just enough, since fb drivers require more than that. 

If you run an application that's not fbdev aware, behavior has always been
undefined.

> Simiarly, I have also looked at some of the fbdev drivers that are not
> using the gen_* interface (nvidia, ati, matrox), and they will also
> unconditially set the hardware during switches/set_var. The
> fbgen_switch() function also calls fbhw->set_par() within do_set_var().

And set_par() could do some optimizations based on a shadow map of the register
contents, also to avoid artefacts when switching to a different VC with the
same video timings. Like:

  write_reg(reg, val)
  {
      if (shadow[reg] != val) {
	shadow[reg] = val;
	hardware[reg] = val;
      }
  }

> > If it's really a problem, maybe we could figure out a way to detect when
> > it's safe to optimize stuff away or as a last resort make it an option?
> > 
> I think if the gen_* interface is to be adopted, it will become a
> problem. Detection is the best solution, but right now X and DRI do not
> know that fb even exist so we can't get X to detect fb unless we
> persuade the X people to do that.  I have tried X detection before by
> checking the previous console number.  If the previous number is not a
> valid console, we can presume that a non-console app used that.  But
> this is not clean and there are too many conditions where this check
> will fail. But then, I really don't understand the underlying console
> interface, so an easier and more effective way may exist that I don't
> know about.

The problem is that most drivers in XFree86 don't _want_ to be fbdev compliant.
The solution is to convince the hardcode anti-fbdev XFree86 guys to use the
fbdev if it's present. Fbdev is part of the kernel API. Circumventing the API
is bad behavior.

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


_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net

Re: Comments on fbgen.c and fbcon-accel.c

[Antonino Daplas]

On Tue, 2002-05-07 at 16:00, Geert Uytterhoeven wrote:
> > I was talking of X running with it's own accelerated drivers.  I have
> > checked most of the X accelerated drivers, and most will just attempt to
> > restore the VGA registers and some when switching to the console.  I
> > think this is not just enough, since fb drivers require more than that. 
> 
> If you run an application that's not fbdev aware, behavior has always been
> undefined.
> 

In the same token, then the kernel should be made as robust as
possible.  I would rather have a user app crash than have it lock-up the
hardware or corrupt the display.
 
> > Simiarly, I have also looked at some of the fbdev drivers that are not
> > using the gen_* interface (nvidia, ati, matrox), and they will also
> > unconditially set the hardware during switches/set_var. The
> > fbgen_switch() function also calls fbhw->set_par() within do_set_var().
> 
> And set_par() could do some optimizations based on a shadow map of the register
> contents, also to avoid artefacts when switching to a different VC with the
> same video timings. Like:
> 
>   write_reg(reg, val)
>   {
>       if (shadow[reg] != val) {
> 	shadow[reg] = val;
> 	hardware[reg] = val;
>       }
>   }
> 

Actually, What I'm trying to point out is that gen_set_var() is not even
given a chance to call set_par().  If the generic interface is to be
adopted, it has to satisfy the requirements of most, if not all cards.

My current thinking is to let gen_set_var() call set_par()
unconditionally and at the same time pass a parameter or set a flag.
This flag or parameter can be something like, "yes, you really need to
set the hardware" or "no, var has not changed but you have the option to
set the hardware or not." Something to this effect:

if (memcmp(oldvar, newvar, sizeof(struct fb_var_screeninfo)))
	info->fbops->fb_set_par(info, 1);
else 
	info->fbops->fb_set_par(info, 0);

> > I think if the gen_* interface is to be adopted, it will become a
> > problem. Detection is the best solution, but right now X and DRI do not
> > know that fb even exist so we can't get X to detect fb unless we
> > persuade the X people to do that.  I have tried X detection before by
> > checking the previous console number.  If the previous number is not a
> > valid console, we can presume that a non-console app used that.  But
> > this is not clean and there are too many conditions where this check
> > will fail. But then, I really don't understand the underlying console
> > interface, so an easier and more effective way may exist that I don't
> > know about.
> 
> The problem is that most drivers in XFree86 don't _want_ to be fbdev compliant.
> The solution is to convince the hardcode anti-fbdev XFree86 guys to use the

I can't agree with you more.  When I was writing the i810/i815 fb
driver, I also modified the XFree86 i810 driver so it is fbdev-aware.
The "fbdev-awareness" is not a requirement, but it solves a lot of
problems and lessened code bloat.

But as you've said, they're hardcore :)

> fbdev if it's present. Fbdev is part of the kernel API. Circumventing the API
> is bad behavior.
> 

Which is why I abandoned that idea. 

Tony



_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net

Re: Comments on fbgen.c and fbcon-accel.c

[Michel Dänzer]

On Tue, 2002-05-07 at 10:00, Geert Uytterhoeven wrote: 
> > > If it's really a problem, maybe we could figure out a way to detect when
> > > it's safe to optimize stuff away or as a last resort make it an option?
> > > 
> > I think if the gen_* interface is to be adopted, it will become a
> > problem. Detection is the best solution, but right now X and DRI do not
> > know that fb even exist so we can't get X to detect fb unless we
> > persuade the X people to do that.  I have tried X detection before by
> > checking the previous console number.  If the previous number is not a
> > valid console, we can presume that a non-console app used that.  But
> > this is not clean and there are too many conditions where this check
> > will fail. But then, I really don't understand the underlying console
> > interface, so an easier and more effective way may exist that I don't
> > know about.
> 
> The problem is that most drivers in XFree86 don't _want_ to be fbdev compliant.
> The solution is to convince the hardcode anti-fbdev XFree86 guys to use the
> fbdev if it's present. Fbdev is part of the kernel API. Circumventing the API
> is bad behavior.

The opposition seems to be mostly against the fbdev support being spread
over the drivers, which is hard to maintain. If we could move it into an
common layer, there should be no problem. I do have the basic idea how
to do it but I suspect it would require changes to the driver interface
so it might have to wait for XFree86 5.x (provided anyone actually tries
:).


-- 
Earthling Michel Dänzer (MrCooper)/ Debian GNU/Linux (powerpc) developer
XFree86 and DRI project member   /  CS student, Free Software enthusiast

_______________________________________________________________

Have big pipes? SourceForge.net is looking for download mirrors. We supply
the hardware. You get the recognition. Email Us: bandwidth@sourceforge.net

Re: Comments on fbgen.c and fbcon-accel.c

[James Simmons]

> The above is fine if everything is done in the console.  However,
> problems may arise if an app that touches the graphics hardware (ie X)
> is launched.  From the point of view of fbcon, the hardware state hasn't
> changed (compare of newvar with oldvar is false) when display is
> switched back to console.  And if that app did not fully restore the
> hardware state,  we will be left with a corrupted display.

I'm aware of this flaw. I plan to fix this when the rewrite of the console
system starts. The console system should handle restore ths video mode.


_______________________________________________________________

Don't miss the 2002 Sprint PCS Application Developer's Conference
August 25-28 in Las Vegas -- http://devcon.sprintpcs.com/adp/index.cfm