Adding an fb_sync() operation to fb_ops

Adding an fb_sync() operation to fb_ops

[Antonino Daplas]

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 


_______________________________________________________________

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

Re: Adding an fb_sync() operation to fb_ops

[James Simmons]

> 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.


_______________________________________________________________

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

Re: Adding an fb_sync() operation to fb_ops

[Geert Uytterhoeven]

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


_______________________________________________________________

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

Re: Adding an fb_sync() operation to fb_ops

[Antonino Daplas]

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


_______________________________________________________________

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

Re: Adding an fb_sync() operation to fb_ops

[Antonino Daplas]

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


_______________________________________________________________

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

RE: Adding an fb_sync() operation to fb_ops

[Sottek, Matthew J]

I agree, I have some API's that I've been using for embedded applications
and it always seems that if you allow mmap() you must have a sync()
function. (A flush is nice too but not required)

If you are going to allow user apps to drive the blitter you don't want to
force a sync after each call. An application that is doing a lot of blitting
would be wasting lots of time syncing when there is only need for just one
sync at the end. The whole point of ring buffers is to pipeline
instructions, you'd loose this if you always synced.

-Matt



-----Original Message-----
From: Antonino Daplas [mailto:adaplas@pol.net]
Sent: Friday, May 31, 2002 8:07 PM
To: fbdev
Subject: [Linux-fbdev-devel] Adding an fb_sync() operation to fb_ops


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 


_______________________________________________________________

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

_______________________________________________
Linux-fbdev-devel mailing list
Linux-fbdev-devel@lists.sourceforge.net
https://lists.sourceforge.net/lists/listinfo/linux-fbdev-devel

_______________________________________________________________

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

RE: Adding an fb_sync() operation to fb_ops

[James Simmons]

> I agree, I have some API's that I've been using for embedded applications
> and it always seems that if you allow mmap() you must have a sync()
> function. (A flush is nice too but not required)

Hm. There is also the issue of the cache and memory going out of sync. The
question is do we need device specific hooks?


_______________________________________________________________

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

RE: Adding an fb_sync() operation to fb_ops

[Sottek, Matthew J]

>> 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?

Tony is correct. I think James got it backwards.

When you have DMA, Ring Buffers, Fifo's you don't know that the command
has happened, you only know that it went into one of the aforementioned
structures. (And actually in the case are DMA buffers you don't even know
that the DMA buffer will ever be processes, which leads to the need for
flush()  in order to keep this simple we can just make sure that doesn't
happen)

So here is the obvious problem case.

Application mmap's the framebuffer.
Application Draws some pixels into the framebuffer.
Application issues an ioctl that would lead to a blit, the command is put
 in a ring buffer but hasn't happened yet.
The application draws some more pixels (via the mmap) in a region that
 intersects the previously blitted region.
The hardware gets around to the blit command and overwrites the pixels you
  just put in the frambuffer.


This is quite likely to happen when mixing direct access with asynchronous
commands. It is also possible with synchronous commands but not as likely.
The blitter does take time to process so if you access the framebuffer
during the blit you could get mixed results. (And I hear that this can
actually hang some hardware)

-Matt

_______________________________________________________________

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

RE: Adding an fb_sync() operation to fb_ops

[Antonino Daplas]

On Mon, 2002-06-03 at 23:49, Sottek, Matthew J wrote:
> 
> So here is the obvious problem case.
> 
> Application mmap's the framebuffer.
> Application Draws some pixels into the framebuffer.
> Application issues an ioctl that would lead to a blit, the command is put
>  in a ring buffer but hasn't happened yet.
> The application draws some more pixels (via the mmap) in a region that
>  intersects the previously blitted region.
> The hardware gets around to the blit command and overwrites the pixels you
>   just put in the frambuffer.
> 
> 
And it does not have to happen outside the kernel.  Within the kernel
too.  For instance, a particular accelerator may only support fillrect
and copyarea, but not imageblit (neofb).  It has no choice but to mix in
cfb_imageblit and the sync problem will arise unless the accelerator
forces a sync -- which neofb does by the way.
  
Tony


_______________________________________________________________

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