linux-fbdev-devel

[Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops

[Antonino D. <ad...@po...>]

Hi,

I have been tesing the 2.5 API for some time now, and I really like the
new changes.  The trend is to consolidate hardware acceleration into 3
functions, fillrect, copyarea and imageblit.  However, there is still
something bugging me. I don't think we can totally avoid mixing direct
graphics memory access with hardware blitting.  For instance, some
drivers probably will have a mixture of accelerated and unaccelerated
drawing functions which may lock up the graphics engine.

In order to avoid that, should we:

a. Just force a hardware sync after each blit?; or

b. Add a new operation, fb_sync() which will be called whenever the
framebuffer memory will be accessed? fb_sync() should guarantee that the
graphics pipeline has been flushed and that there are no pending
hardware operations.

(a) is probably simpler (matrox is already doing that) but might
decrease performance a bit (not significant since fbcon is dealing with
text)

(b) a bit more complicated, but should not be difficult to add since all
drivers probably has some form of 'wait_for_idle' which can be wrapped. 
Then we can probably just add something like:

if (info->fbops->fb_sync())
	info->fbops->fb_sync(info);

at the top of fb_write and fb_read.

Secondly (not related to the topic), I was wondering if we can change
the color value passed to fillrect and imageblit.  Presently, the
palette index is always passed regardless of the visual. Should the
color value passed be reflective of the framebuffer format instead? 
Pass a palette index if pseudocolor, an RGB value for truecolor, etc.

Doing the latter will simplify the low-level drawing function and at the
same time, it will make the drawing functions more flexible -- ie,
possiblity of exporting to userspace.

Tony 

Re: [Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops

[James S. <jsi...@tr...>]

> I don't think we can totally avoid mixing direct
> graphics memory access with hardware blitting.

In userland true. In the kernel we can work around it.

> In order to avoid that, should we:
>
> a. Just force a hardware sync after each blit?; or

> (a) is probably simpler (matrox is already doing that) but might
> decrease performance a bit (not significant since fbcon is dealing with
> text)

This depends. For traditional MMIO we need to sync up. For async DMA, Ring
buffers, and FIFOs we don't need to sync up. That is why I have no hooks
for syncing. I figure drivers can call the syncing functions if they need
it inside their own accel functions.

> b. Add a new operation, fb_sync() which will be called whenever the
> framebuffer memory will be accessed? fb_sync() should guarantee that the
> graphics pipeline has been flushed and that there are no pending
> hardware operations.

> (b) a bit more complicated, but should not be difficult to add since all
> drivers probably has some form of 'wait_for_idle' which can be wrapped.
> Then we can probably just add something like:
>
> if (info->fbops->fb_sync())
> 	info->fbops->fb_sync(info);
>
> at the top of fb_write and fb_read.

  Orginally I had that. Then I realized this doesn't fit all hardware
models. You are right tho. The one time where we do need a sync is for
fb_write and fb_read. How do we solve this? I was planning anyways to
inplement read and write hooks. The reason being is some devices don't
have proper linear framebuffers. Some have banked buffers and some like
the epson 1385 is linear but filling in a solid color at 16bpp gives
strips due to the hardware only working with word or byte align access
only. So we need hooks in this case. So these hooks could also handle the
case of havingv to sync up as well.

> Secondly (not related to the topic), I was wondering if we can change
> the color value passed to fillrect and imageblit.  Presently, the
> palette index is always passed regardless of the visual. Should the
> color value passed be reflective of the framebuffer format instead?
> Pass a palette index if pseudocolor, an RGB value for truecolor, etc.
>
> Doing the latter will simplify the low-level drawing function and at the
> same time, it will make the drawing functions more flexible -- ie,
> possiblity of exporting to userspace.

We had this discussion sometime ago. We discovered it was just impossible
to handle all the possible different color formats in the higher levels.
We ended up with way to many #ifdefs. So it was decided to let the drivers
handle it instead. Actually if you wanted it to be useable to userland
then it makes even more sense to use the color map index instead. Think
about all the if() statements you would need in userland.

Re: [Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops

[Geert U. <ge...@li...>]

On Fri, 31 May 2002, James Simmons wrote:
> > Secondly (not related to the topic), I was wondering if we can change
> > the color value passed to fillrect and imageblit.  Presently, the
> > palette index is always passed regardless of the visual. Should the
> > color value passed be reflective of the framebuffer format instead?
> > Pass a palette index if pseudocolor, an RGB value for truecolor, etc.
> >
> > Doing the latter will simplify the low-level drawing function and at the
> > same time, it will make the drawing functions more flexible -- ie,
> > possiblity of exporting to userspace.
> 
> We had this discussion sometime ago. We discovered it was just impossible
> to handle all the possible different color formats in the higher levels.
> We ended up with way to many #ifdefs. So it was decided to let the drivers
> handle it instead. Actually if you wanted it to be useable to userland
> then it makes even more sense to use the color map index instead. Think
> about all the if() statements you would need in userland.

Userland can easily set up function pointers, depending on the used format
(for commonly used formats). Even for the generic case it can be done without
too much loss of efficiency using look-up tables, cfr. fbtest.

Gr{oetje,eeting}s,

						Geert

--
Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- ge...@li...

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

Re: [Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops

[Antonino D. <ad...@po...>]

On Sat, 2002-06-01 at 04:14, Geert Uytterhoeven wrote:
> On Fri, 31 May 2002, James Simmons wrote:
> > > Secondly (not related to the topic), I was wondering if we can change
> > > the color value passed to fillrect and imageblit.  Presently, the
> > > palette index is always passed regardless of the visual. Should the
> > > color value passed be reflective of the framebuffer format instead?
> > > Pass a palette index if pseudocolor, an RGB value for truecolor, etc.
> > >
> > > Doing the latter will simplify the low-level drawing function and at the
> > > same time, it will make the drawing functions more flexible -- ie,
> > > possiblity of exporting to userspace.
> > 
> > We had this discussion sometime ago. We discovered it was just impossible
> > to handle all the possible different color formats in the higher levels.
> > We ended up with way to many #ifdefs. So it was decided to let the drivers
> > handle it instead. Actually if you wanted it to be useable to userland
> > then it makes even more sense to use the color map index instead. Think
> > about all the if() statements you would need in userland.
> 
> Userland can easily set up function pointers, depending on the used format
> (for commonly used formats). Even for the generic case it can be done without
> too much loss of efficiency using look-up tables, cfr. fbtest.
> 

I think that's a nice idea.  We can add another fb_op (I know, another
one), something like:

static u32 XXXfb_getcolor(u32 index, struct fb_info *info)
{
	u32 color = 0;

	switch (info->var.bits_per_pixel) {
	case 8:
	     color = index;
	     break;
	case 16:
	case 24:
	case 32:
	     color = ((u32 *)(info->pseudo_palette[index]))
	     break;
	}
	
	return color;
}


Then in fbcon-accel.c, we can have something like:

	region.fg_color = info->fb_getcolor(attr_fgcol(p, c));
	region.bg_color = info->fb_getcolor(attr_bgcol(p, c));

This will simplify fbcon-accel and the drawing functions. 

Tony

Re: [Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops

[Antonino D. <ad...@po...>]

On Sat, 2002-06-01 at 03:56, James Simmons wrote:
> 
> > I don't think we can totally avoid mixing direct
> > graphics memory access with hardware blitting.
> 
> In userland true. In the kernel we can work around it.
> 
> > In order to avoid that, should we:
> >
> > a. Just force a hardware sync after each blit?; or
> 
> > (a) is probably simpler (matrox is already doing that) but might
> > decrease performance a bit (not significant since fbcon is dealing with
> > text)
> 
> This depends. For traditional MMIO we need to sync up. For async DMA, Ring
> buffers, and FIFOs we don't need to sync up. That is why I have no hooks
> for syncing. I figure drivers can call the syncing functions if they need
> it inside their own accel functions.

Is it the other way around, async DMA, ringbuffers, and FIFOS that will
need the syncing? Personally, whether we sync after each blit or sync
only when needed does not matter. Since fb is dealing mostly with
discontiguous regions (character cells), except for the occasional clear
screen, perfomance degradation is not significant. So, is it agreed that
for drivers that need sync's, they will just have to do it after each
blit?  It's just that it defeats the purpose of those FIFO's and
buffers.  

> 
> > b. Add a new operation, fb_sync() which will be called whenever the
> > framebuffer memory will be accessed? fb_sync() should guarantee that the
> > graphics pipeline has been flushed and that there are no pending
> > hardware operations.
> 
> > (b) a bit more complicated, but should not be difficult to add since all
> > drivers probably has some form of 'wait_for_idle' which can be wrapped.
> > Then we can probably just add something like:
> >
> > if (info->fbops->fb_sync())
> > 	info->fbops->fb_sync(info);
> >
> > at the top of fb_write and fb_read.
> 
>   Orginally I had that. Then I realized this doesn't fit all hardware
> models. You are right tho. The one time where we do need a sync is for
Still, a driver can always assign a NULL to fb_sync if it is not
needed.  


> > Secondly (not related to the topic), I was wondering if we can change
> > the color value passed to fillrect and imageblit.  Presently, the
> > palette index is always passed regardless of the visual. Should the
> > color value passed be reflective of the framebuffer format instead?
> > Pass a palette index if pseudocolor, an RGB value for truecolor, etc.
> >
> > Doing the latter will simplify the low-level drawing function and at the
> > same time, it will make the drawing functions more flexible -- ie,
> > possiblity of exporting to userspace.
> 
> We had this discussion sometime ago. We discovered it was just impossible
> to handle all the possible different color formats in the higher levels.
> We ended up with way to many #ifdefs. So it was decided to let the drivers
> handle it instead. Actually if you wanted it to be useable to userland
> then it makes even more sense to use the color map index instead. Think
> about all the if() statements you would need in userland.
> 
I agree that fbcon-accel will be swamped by  if-else/case-switch
statements, and passing palette indices may be the best solution
overall. The price, though, is that those drawing functions may only be
usable within the kernel. 

If these drawing functions are to be exported to userland, then the
current implementation will necessitate a workaround.  Within the kernel
we are dealing with 16 colors, and color palettes (in pseudocolor) have
a practical limit of 256. In userland we are dealing with thousands to
millions of colors.  So how will I convert a particular 16-bit RGB
colorvalue into a palette index?

a.  Reset the colormap so that a particular colorvalue can be indexed
from the map and keep track which colorvalue is in the map or not.  

b.  Tell the accelerator that it is not actually a palette index, but
treat it as directcolor;

c.  Or allocate a colormap that will encompass the entire color range
(insane).

I can't really think of any other way, so correct me if I'm wrong. In
the end though, it's much simpler to just pass the appropriate color
value.

Secondly, if I'm going to use, for instance, imageblit to tile and
expand an 8x8 mono bitmap, then for each tile, the accelerator has to
lookup the color which is too inefficient.  On the other hand, if the
color is looked up in the upper layer, then it's done only once.  Of
course, this is not done in the console, so it's a bad example. 


Tony