fb_imageblit()

fb_imageblit()

[Geert Uytterhoeven]

I have a few questions and comments related to fb_imageblit().

 1. Why is the logo data in fb_image.data stored in an `unpacked' way? I.e.
    each byte of the logo data corresponds to one pixel of the image,
    irrespective of the depth of the image.

    However, I do see one good reason: it makes life easier for planar
    displays, since a fast c2p (chunky-to-planar) convertor doesn't have to
    care about the image depth, the source data is always 1 byte per pixel.

 2. If fb_image.depth == 1, how can fb_imageblit() distinguish between drawing
    a monochrome image (e.g. penguin logo) and color expanding a monochrome
    image (e.g. drawing text)? It's not possible to handle them the same, since
    image data for color expansion is packed, while image data for monochrome
    image drawing is not.
    
    If monochrome image data would be packed as well, it could be handled by
    setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
    mono10), and fb_set_logo() becomes simpler as well.

    If we retain the unpacked data for images, we need some other flag to
    indicate color expansion. Perhaps setting fb_image.depth to 0?

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



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fb_imageblit()

[Antonino Daplas]

On Mon, 2003-01-06 at 00:38, Geert Uytterhoeven wrote:
> 
> I have a few questions and comments related to fb_imageblit().
> 
>  1. Why is the logo data in fb_image.data stored in an `unpacked' way? I.e.
>     each byte of the logo data corresponds to one pixel of the image,
>     irrespective of the depth of the image.
> 
I asked before what would be the content of the logo data and it was
agreed that the logo data would either contain indices to the
pseudo_palette for truecolor and directcolor or the pixel itself for the
rest so it's consistent with the rest of the soft accel functions. 
Fixing the minimum unit to one byte seems a reasonable compromise
between maximum number of colors available and size of logo data.  Plus,
it would be simpler to code.

>     However, I do see one good reason: it makes life easier for planar
>     displays, since a fast c2p (chunky-to-planar) convertor doesn't have to
>     care about the image depth, the source data is always 1 byte per pixel.
> 
>  2. If fb_image.depth == 1, how can fb_imageblit() distinguish between drawing
>     a monochrome image (e.g. penguin logo) and color expanding a monochrome
>     image (e.g. drawing text)? It's not possible to handle them the same, since
>     image data for color expansion is packed, while image data for monochrome
>     image drawing is not.
I noticed this before but completely forgot about it because I have no
monochrome graphics card to test :-)

>     
>     If monochrome image data would be packed as well, it could be handled by
>     setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
>     mono10), and fb_set_logo() becomes simpler as well.
> 
>     If we retain the unpacked data for images, we need some other flag to
>     indicate color expansion. Perhaps setting fb_image.depth to 0?
> 
If we change the contents of the logo data, then it makes sense to pack
the logo data also.  However, if we stick to indices, we might as well
retain the "unpacked" 8-bit format.  I think setting fb_image.depth to 0
to mean color expansion is more appropriate. Drivers that will need
trivial changing would be tgafb, i810fb, rivafb, tdfxfb, atyfb, vga16fb
and of course cfb_imgblt.c, softcursor.c and fbcon.c.

I'll submit a patch if everyone agrees.  

Tony




-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fb_imageblit()

[Geert Uytterhoeven]

On 6 Jan 2003, Antonino Daplas wrote:
> On Mon, 2003-01-06 at 00:38, Geert Uytterhoeven wrote:
> > I have a few questions and comments related to fb_imageblit().
> > 
> >  1. Why is the logo data in fb_image.data stored in an `unpacked' way? I.e.
> >     each byte of the logo data corresponds to one pixel of the image,
> >     irrespective of the depth of the image.
> > 
> I asked before what would be the content of the logo data and it was
> agreed that the logo data would either contain indices to the
> pseudo_palette for truecolor and directcolor or the pixel itself for the
> rest so it's consistent with the rest of the soft accel functions. 
> Fixing the minimum unit to one byte seems a reasonable compromise
> between maximum number of colors available and size of logo data.  Plus,
> it would be simpler to code.

Yes, that's reasonable.

> >     However, I do see one good reason: it makes life easier for planar
> >     displays, since a fast c2p (chunky-to-planar) convertor doesn't have to
> >     care about the image depth, the source data is always 1 byte per pixel.
> > 
> >  2. If fb_image.depth == 1, how can fb_imageblit() distinguish between drawing
> >     a monochrome image (e.g. penguin logo) and color expanding a monochrome
> >     image (e.g. drawing text)? It's not possible to handle them the same, since
> >     image data for color expansion is packed, while image data for monochrome
> >     image drawing is not.
> I noticed this before but completely forgot about it because I have no
> monochrome graphics card to test :-)

I didn't think about that as well, until I tried all possible depths (1-8) with
amifb.

> >     If monochrome image data would be packed as well, it could be handled by
> >     setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
> >     mono10), and fb_set_logo() becomes simpler as well.
> > 
> >     If we retain the unpacked data for images, we need some other flag to
> >     indicate color expansion. Perhaps setting fb_image.depth to 0?
> > 
> If we change the contents of the logo data, then it makes sense to pack
> the logo data also.  However, if we stick to indices, we might as well
> retain the "unpacked" 8-bit format.  I think setting fb_image.depth to 0

OK.

> to mean color expansion is more appropriate. Drivers that will need

OK.

> trivial changing would be tgafb, i810fb, rivafb, tdfxfb, atyfb, vga16fb
> and of course cfb_imgblt.c, softcursor.c and fbcon.c.
> 
> I'll submit a patch if everyone agrees.  

Thanks!

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



-------------------------------------------------------
This sf.net email is sponsored by:ThinkGeek
Welcome to geek heaven.
http://thinkgeek.com/sf

Re: fb_imageblit()

[James Simmons]

> > I have a few questions and comments related to fb_imageblit().
> > 
> >  1. Why is the logo data in fb_image.data stored in an `unpacked' way? I.e.
> >     each byte of the logo data corresponds to one pixel of the image,
> >     irrespective of the depth of the image.
> > 
> I asked before what would be the content of the logo data and it was
> agreed that the logo data would either contain indices to the
> pseudo_palette for truecolor and directcolor or the pixel itself for the
> rest so it's consistent with the rest of the soft accel functions. 
> Fixing the minimum unit to one byte seems a reasonable compromise
> between maximum number of colors available and size of logo data.  Plus,
> it would be simpler to code.

I didn't realize that the data for the logo was unpacked. The imageblit 
blit function that I have written always assumes its packed data. This 
explains why sometimes I get weird effects. I don't mind if the logo image 
is restricked to 256 colors. Have a 65K color image would add alot fo 
bloat to the kernel image.

> >  2. If fb_image.depth == 1, how can fb_imageblit() distinguish between drawing
> >     a monochrome image (e.g. penguin logo) and color expanding a monochrome
> >     image (e.g. drawing text)? It's not possible to handle them the same, since
> >     image data for color expansion is packed, while image data for monochrome
> >     image drawing is not.
> I noticed this before but completely forgot about it because I have no
> monochrome graphics card to test :-)

> >     
> >     If monochrome image data would be packed as well, it could be handled by
> >     setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
> >     mono10), and fb_set_logo() becomes simpler as well.
> > 
> >     If we retain the unpacked data for images, we need some other flag to
> >     indicate color expansion. Perhaps setting fb_image.depth to 0?
> > 
> If we change the contents of the logo data, then it makes sense to pack
> the logo data also.  However, if we stick to indices, we might as well
> retain the "unpacked" 8-bit format.  I think setting fb_image.depth to 0
> to mean color expansion is more appropriate. Drivers that will need
> trivial changing would be tgafb, i810fb, rivafb, tdfxfb, atyfb, vga16fb
> and of course cfb_imgblt.c, softcursor.c and fbcon.c.

The requirement I made of imageblit was to always use packed data. What 
the indices approach was to to always use a struct fb_cmap. This way it 
didn't matter if used a pseudocolor mode or a directcolor mode.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Antonino Daplas]

On Wed, 2003-01-08 at 06:23, James Simmons wrote: 
> > >     
> > >     If monochrome image data would be packed as well, it could be handled by
> > >     setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
> > >     mono10), and fb_set_logo() becomes simpler as well.
> > > 
> > >     If we retain the unpacked data for images, we need some other flag to
> > >     indicate color expansion. Perhaps setting fb_image.depth to 0?
> > > 
> > If we change the contents of the logo data, then it makes sense to pack
> > the logo data also.  However, if we stick to indices, we might as well
> > retain the "unpacked" 8-bit format.  I think setting fb_image.depth to 0
> > to mean color expansion is more appropriate. Drivers that will need
> > trivial changing would be tgafb, i810fb, rivafb, tdfxfb, atyfb, vga16fb
> > and of course cfb_imgblt.c, softcursor.c and fbcon.c.
> 
> The requirement I made of imageblit was to always use packed data. What 
> the indices approach was to to always use a struct fb_cmap. This way it 
> didn't matter if used a pseudocolor mode or a directcolor mode.
 
I've thought of that also, packing the data according to the pixel depth.  
However, this will be very inefficient since image blitting
will go like this: 

a. Prepare logo data so each pixel of data is in directcolor format (if
we will use the cmap), so depth corresponds to framebuffer depth, and
data is packed. 

b. Pass the structure to cfb_imageblit. 

c. In cfb_imageblit, unpack the logo data: 

d. Get each color component from the cmap data. 

e. Recreate pixel data based on var.[color].offset and
var.[color].length. 

f. Write the pixel data in packed form to the framebuffer. 

Whereas, with the current approach: 

a. Prepare logo data such that each pixel corresponds to one byte. 

b. Pass the structure to cfb_imageblit. 

c. Read color information from pseudopalette if directcolor/truecolor. 

d. Write the pixel data in packed form to the framebuffer. 

Aside from the inefficiency of the method (pack, unpack, pack), drawing
the logo will become inconsistent with the behavior of the rest of the
functions, since it's the only one that gets color info differently.  If
you look at color_imageblit() and slow_imageblit(), they basically use
the same code logic. 

Secondly, indexing the cmap instead of the pseudo_palette means that
cfb_imageblit has to know the native framebuffer format.  This was
argued before that the generic drawing functions need not know of the
format, one of the reasons we have info->pseudopalette.  Actually, in
order to be really consistent, I would rather have everything refer to
the pseudopalette, regardless of the visual format.  This will be better
especially for some of the corner cases, like monochrome cards with
bits_per_pixel = 8. 

Thirdly, it's much simpler for drivers to draw the logo.  Just get the
corrct pixel data from it's own palette.  No need to construct each
pixel from the cmap.  That's tedious, slow, and will contribute to code
bloat.  Which is easier?  Get each byte from the logo data, and use it as an 
index to the pseudo_palette, or unpack the data, separate each unit to 4 
color components, and construct pixel data from cmap using the 4 extracted 
indices?

I agree that it would be faster in some cases to draw a packed logo
data, but if we're going to be inconsistent, let's do it all the way. 
Make the logo data match the native framebuffer format.  This will be
very efficient, and this is the one that I actually prefer.

Tony 





-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Geert Uytterhoeven]

On 8 Jan 2003, Antonino Daplas wrote:

> On Wed, 2003-01-08 at 06:23, James Simmons wrote: 
> > > >     
> > > >     If monochrome image data would be packed as well, it could be handled by
> > > >     setting fb_image.fg_color = 1 and fb_image.bg_color = 0 (or vice versa for
> > > >     mono10), and fb_set_logo() becomes simpler as well.
> > > > 
> > > >     If we retain the unpacked data for images, we need some other flag to
> > > >     indicate color expansion. Perhaps setting fb_image.depth to 0?
> > > > 
> > > If we change the contents of the logo data, then it makes sense to pack
> > > the logo data also.  However, if we stick to indices, we might as well
> > > retain the "unpacked" 8-bit format.  I think setting fb_image.depth to 0
> > > to mean color expansion is more appropriate. Drivers that will need
> > > trivial changing would be tgafb, i810fb, rivafb, tdfxfb, atyfb, vga16fb
> > > and of course cfb_imgblt.c, softcursor.c and fbcon.c.
> > 
> > The requirement I made of imageblit was to always use packed data. What 
> > the indices approach was to to always use a struct fb_cmap. This way it 
> > didn't matter if used a pseudocolor mode or a directcolor mode.
>  
> I've thought of that also, packing the data according to the pixel depth.  
> However, this will be very inefficient since image blitting
> will go like this: 
> 
> a. Prepare logo data so each pixel of data is in directcolor format (if
> we will use the cmap), so depth corresponds to framebuffer depth, and
> data is packed. 
> 
> b. Pass the structure to cfb_imageblit. 
> 
> c. In cfb_imageblit, unpack the logo data: 
> 
> d. Get each color component from the cmap data. 
> 
> e. Recreate pixel data based on var.[color].offset and
> var.[color].length. 
> 
> f. Write the pixel data in packed form to the framebuffer. 
> 
> Whereas, with the current approach: 
> 
> a. Prepare logo data such that each pixel corresponds to one byte. 
> 
> b. Pass the structure to cfb_imageblit. 
> 
> c. Read color information from pseudopalette if directcolor/truecolor. 

Hoever, pseudopalette has entries for the first 16 colors only!
Hence you are limited to the 16 color for directcolor/truecolor modes.

> d. Write the pixel data in packed form to the framebuffer. 

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



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Antonino Daplas]

On Wed, 2003-01-08 at 23:15, Geert Uytterhoeven wrote:
> > c. Read color information from pseudopalette if directcolor/truecolor. 
> 
> Hoever, pseudopalette has entries for the first 16 colors only!
> Hence you are limited to the 16 color for directcolor/truecolor modes.

That's why there's an fb_set_logo_directpalette(), for directcolor
visuals >= 24bpp, and fb_set_logo_truepalette(), for truecolor, in
fb_set_logo().  Basically, it temporarily replaces info->pseudo_palette
with one that has 256 entries to match linux_logo.  Logo drawing, using
cfb_imageblit() has always worked for me in directcolor and truecolor
modes.

Tony




-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Geert Uytterhoeven]

On 9 Jan 2003, Antonino Daplas wrote:
> On Wed, 2003-01-08 at 23:15, Geert Uytterhoeven wrote:
> > > c. Read color information from pseudopalette if directcolor/truecolor. 
> > 
> > Hoever, pseudopalette has entries for the first 16 colors only!
> > Hence you are limited to the 16 color for directcolor/truecolor modes.
> 
> That's why there's an fb_set_logo_directpalette(), for directcolor
> visuals >= 24bpp, and fb_set_logo_truepalette(), for truecolor, in
> fb_set_logo().  Basically, it temporarily replaces info->pseudo_palette
> with one that has 256 entries to match linux_logo.  Logo drawing, using
> cfb_imageblit() has always worked for me in directcolor and truecolor
> modes.

Bummer, /me should read the code more thoroughfully...

Perhaps renaming `{saved_,}palette' to `{saved_,}pseudo_palette' would make
this clearer...

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



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[James Simmons]

> Bummer, /me should read the code more thoroughfully...
> 
> Perhaps renaming `{saved_,}palette' to `{saved_,}pseudo_palette' would make
> this clearer...

I renamed it.



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Geert Uytterhoeven]

On 9 Jan 2003, Antonino Daplas wrote:
> On Wed, 2003-01-08 at 23:15, Geert Uytterhoeven wrote:
> > > c. Read color information from pseudopalette if directcolor/truecolor. 
> > 
> > Hoever, pseudopalette has entries for the first 16 colors only!
> > Hence you are limited to the 16 color for directcolor/truecolor modes.
> 
> That's why there's an fb_set_logo_directpalette(), for directcolor
> visuals >= 24bpp, and fb_set_logo_truepalette(), for truecolor, in
> fb_set_logo().  Basically, it temporarily replaces info->pseudo_palette
> with one that has 256 entries to match linux_logo.  Logo drawing, using
> cfb_imageblit() has always worked for me in directcolor and truecolor
> modes.

I see a small inconsistency here, which may cause problems with some exotic
hardware: info->pseudo_palette is always initialized by the fbdev driver itself
(which knows the hardware), except for logo drawing, where it's done by the
generic code in fbmem.

On virtually all hardware that will work fine. But on some hardware the exact
pixel format cannot be represented by the {red,green,blue,transp} bitfields in
fb_var_screeninfo.

E.g. the Amiga CyberVision64 card has a S3Trio64. Since Amigas are little
endian and PCI is big endian, they swapped the data bus to simplify 256-color
modes. However, this also means that 16-bit pixel values have to be swapped. So
in depth 15, the pixel format is not ARRRRRGGGGGBBBBB, but GGGBBBBBARRRRRGG.
This can be handled fine in cyberfb by setting up a byteswapped pseudo palette,
but the fb_set_logo_{direct,true}palette() don't know about this. And of course
user space doesn't know about this neither.

One possible solution is to extend the pseudo palette to 256+1 entries if the
depth is at least 8. To save memory, we can still use a 16+1 entry pseudo
palette if depth < 8, but then we have to move the cursor inversion value from
index 16 to index -1.

Note that the old logo code suffered from the same problem and cyberfb hasn't
been updated since a while (it lacks pseudo palette support), so it may not be
that important. I just wanted to mention this case.

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



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Antonino Daplas]

On Thu, 2003-01-09 at 17:50, Geert Uytterhoeven wrote:
> On 9 Jan 2003, Antonino Daplas wrote:
> > On Wed, 2003-01-08 at 23:15, Geert Uytterhoeven wrote:
> > > > c. Read color information from pseudopalette if directcolor/truecolor. 
> > > 
> > > Hoever, pseudopalette has entries for the first 16 colors only!
> > > Hence you are limited to the 16 color for directcolor/truecolor modes.
> > 
> > That's why there's an fb_set_logo_directpalette(), for directcolor
> > visuals >= 24bpp, and fb_set_logo_truepalette(), for truecolor, in
> > fb_set_logo().  Basically, it temporarily replaces info->pseudo_palette
> > with one that has 256 entries to match linux_logo.  Logo drawing, using
> > cfb_imageblit() has always worked for me in directcolor and truecolor
> > modes.
> 
> I see a small inconsistency here, which may cause problems with some exotic
> hardware: info->pseudo_palette is always initialized by the fbdev driver itself
> (which knows the hardware), except for logo drawing, where it's done by the
> generic code in fbmem.
> 
> On virtually all hardware that will work fine. But on some hardware the exact
> pixel format cannot be represented by the {red,green,blue,transp} bitfields in
> fb_var_screeninfo.
> 
> E.g. the Amiga CyberVision64 card has a S3Trio64. Since Amigas are little
> endian and PCI is big endian, they swapped the data bus to simplify 256-color
> modes. However, this also means that 16-bit pixel values have to be swapped. So
> in depth 15, the pixel format is not ARRRRRGGGGGBBBBB, but GGGBBBBBARRRRRGG.
> This can be handled fine in cyberfb by setting up a byteswapped pseudo palette,
> but the fb_set_logo_{direct,true}palette() don't know about this. And of course
> user space doesn't know about this neither.
> 

This will be a problem only for DirectColor at >= 24 bpp.  At bpp's less
than that, linux_logo_16 will be used.

Another possible solution (in case it supports 24bpp) is to have 2
pseudo_palettes, one which is 16 entries long and public, and another
256-entries long and private.  Then if image.depth is < 24, it's safe to
use cfb_imageblit. Otherwise, it has to use it's own imageblit, one that
will use the private pseudo_palette.  The driver will have the
opportunity to build this because fb_set_cmap() is called for
directcolor modes >= 24bpp, and pseudocolor == 8bpp.

Hopefully, exotics such as this will not export their visuals as
truecolor or static pseudocolor because fb_set_cmap() will not be
called. Otherwise, we'll just make it mandatory to call fb_set_cmap()
for all visual modes requiring linux_logo.
  
> One possible solution is to extend the pseudo palette to 256+1 entries if the
> depth is at least 8. To save memory, we can still use a 16+1 entry pseudo
> palette if depth < 8, but then we have to move the cursor inversion value from
> index 16 to index -1.
> 

I believe the cursor inversion value is unused anymore(?), since
fbcon_revc is gone. It has been replaced by the new cursor API which
allows the driver more intimate handling of the cursor.

Tony



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[James Simmons]

> Hopefully, exotics such as this will not export their visuals as
> truecolor or static pseudocolor because fb_set_cmap() will not be
> called. Otherwise, we'll just make it mandatory to call fb_set_cmap()
> for all visual modes requiring linux_logo.

Personally I think the best solution is to always call fb_set_cmap. Also 
the other issue is the 256 versus 16 length of pseudo_palette. We might 
have to have pseudo_palette either at the highest value, usually 256, or 
make pseudo_palette dynamic and change its size if the color depth 
changes.

> I believe the cursor inversion value is unused anymore(?), since
> fbcon_revc is gone. It has been replaced by the new cursor API which
> allows the driver more intimate handling of the cursor.

Correct. We don't need the 17th value now :-)



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[James Simmons]

> I've thought of that also, packing the data according to the pixel depth.  

Actually I was think more along the image depth. The largest the penguin 
logo gets is 224 colors. Meaning the largest image.depth is 8 bits per 
pixel. I do agree it is messy to pack the logo data. So for say the 16 
color logo (4 bpp) putting two pixels worth of data into each byte and 
then seperating it out would be time consuming. The only reason I was 
thinking this way is because of userland using the cursor means imageblit 
is visible to userland. 

> a. Prepare logo data so each pixel of data is in directcolor format (if
> we will use the cmap), so depth corresponds to framebuffer depth, and
> data is packed. 

The depth doesn't have to be the same as the framebuffer depth. The logo 
data is in terms of color map indices. I did think in terms of packed data 
tho.

> d. Get each color component from the cmap data. 
> 
> e. Recreate pixel data based on var.[color].offset and
> var.[color].length. 

Yuck no. Doing a fb_set_cmap should handle that before we call 
xxxfb_imageblit. Then xxxfb_imageblit only has to take the data in 
the logo data as indices of the color map. Of 
 
> f. Write the pixel data in packed form to the framebuffer. 

> Whereas, with the current approach: 
> 
> a. Prepare logo data such that each pixel corresponds to one byte. 
> 
> b. Pass the structure to cfb_imageblit. 
> 
> c. Read color information from pseudopalette if directcolor/truecolor. 
> 
> d. Write the pixel data in packed form to the framebuffer. 

I was think the same way except for the idea of more than one color 
indices per byte for less than 8 bpp modes.
 
> Secondly, indexing the cmap instead of the pseudo_palette means that
> cfb_imageblit has to know the native framebuffer format.  

No indexing the cmap. Instead call fb_set_cmap before we call 
xxxfb_imageblit. Personally I think we should call fb_set_cmap always.

> I would rather have everything refer to
> the pseudopalette, regardless of the visual format.  This will be better
> especially for some of the corner cases, like monochrome cards with
> bits_per_pixel = 8. 

It works pretty good for for any pack pixel type modes. Now for planar 
cards this isn't the case. Ideally struct fb_cmap should have contained 
a 

unsigned long pixel;

field. Like X does. It does not and changing that would break things :-(
 



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Geert Uytterhoeven]

On Fri, 10 Jan 2003, James Simmons wrote:
> > Secondly, indexing the cmap instead of the pseudo_palette means that
> > cfb_imageblit has to know the native framebuffer format.  
> 
> No indexing the cmap. Instead call fb_set_cmap before we call 
> xxxfb_imageblit. Personally I think we should call fb_set_cmap always.
> 
> > I would rather have everything refer to
> > the pseudopalette, regardless of the visual format.  This will be better
> > especially for some of the corner cases, like monochrome cards with
> > bits_per_pixel = 8. 
> 
> It works pretty good for for any pack pixel type modes. Now for planar 
> cards this isn't the case. Ideally struct fb_cmap should have contained 

What do you mean? I don't see a problem for planar modes. A pixel is still a
value of size n bits, it's just that the n bits are not located next to each
other, but spread across multiple words. planar_imageblit() will take care of
converting from chunky to planar mode.

BTW, I do have a working amifb now (it's in Linux/m68k CVS), but the
imageblit() needs more optimizations.

> a 
> 
> unsigned long pixel;
> 
> field. Like X does. It does not and changing that would break things :-(

And let the fbdev driver fill in the pixel values, based on the fb_cmap?

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



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com

Re: fb_imageblit()

[Antonino Daplas]

On Sat, 2003-01-11 at 03:23, James Simmons wrote:
> 
> > I've thought of that also, packing the data according to the pixel depth.  
> 
> Actually I was think more along the image depth. The largest the penguin 
> logo gets is 224 colors. Meaning the largest image.depth is 8 bits per 
> pixel. I do agree it is messy to pack the logo data. So for say the 16 
> color logo (4 bpp) putting two pixels worth of data into each byte and 
> then seperating it out would be time consuming. The only reason I was 
> thinking this way is because of userland using the cursor means imageblit 
> is visible to userland. 

That's why we don't need to support software cursor routines, which
implies that imageblit will not be exposed to user space.  It's
impractical to do that in kernel space anyway because you need lot's of
additional code:

1.You have to constantly save and restore the screen underneath the
cursor. In the console we can cheat because we know the current
character underneath the cursor and movement of the cursor is granulated
anyway, per character instead of per pixel.

2.  Imageblit has to support more ROP's.  Otherwise, the user will be
stuck with a rectangular cursor.  In fbcon, again, we can cheat, because
we always know that any given character is just made up of 2 colors, the
background and the foreground. So it's easy to do bit operations using
the cursor image, the cursor mask, and the font bitmap of the character
underneath the cursor. In user space, expect the image beneath the
cursor to be composed of > 2 colors.
 
3. Optionally, imageblit has to support color expansion other than
mono->framebuffer format  in case the cursor image depth != framebuffer
depth.  Note, imageblit is limited by length of info->pseudo_palette, so
the app cannot pass a cursor image whose depth is equal to the
framebuffer depth (unless bpp <= 4bpp).

4. Imageblit will not be able to practically support more than 256
colors at a time and still do it efficiently. Either we change the
formatting of the data to match the native framebuffer format, which is
the most efficient but requires intimate knowledge of the hardware, or
change it into directcolor format.  Changing it to directcolor format
means we have to reconstruct each pixel from the cmap. This is just too
slow.
  
So only drivers with hardware cursor support need to expose it to
userspace.  And we keep imageblit private to fbcon.

> 
> > a. Prepare logo data so each pixel of data is in directcolor format (if
> > we will use the cmap), so depth corresponds to framebuffer depth, and
> > data is packed. 
> 
> The depth doesn't have to be the same as the framebuffer depth. The logo 
> data is in terms of color map indices. I did think in terms of packed data 
> tho.
> 

So how will the driver know how the logo data is packed if it's not
based on the framebuffer depth? Unless you have conditionals like:

if (!(8 % var->bits_per_pixel) && var-bits_per_pixel <= 8)
	pixels_per_byte = 8/var->bits_per_pixel; /* 1, 2, 4, 8 bpp*/
else
	pixels_per_byte = 1; 

Or:

	pixels_per_byte = 1 /* always */

The latter is so much simpler.  

Or if you want to pack 1-8 bpp entirely, then be willing to add code for
packing 3, 5, 6, 7 bpp.
 
> > d. Get each color component from the cmap data. 
> > 
> > e. Recreate pixel data based on var.[color].offset and
> > var.[color].length. 
> 
> Yuck no. Doing a fb_set_cmap should handle that before we call 
> xxxfb_imageblit. Then xxxfb_imageblit only has to take the data in 
> the logo data as indices of the color map. Of 
>  

Which is what the current code is already doing, building a temporary
pseudo_palette, albeit without the driver's notice. By extending the
pseudo_palette to 256 entries, we can let the driver build it on its
own.

> > f. Write the pixel data in packed form to the framebuffer. 
> 
> > Whereas, with the current approach: 
> > 
> > a. Prepare logo data such that each pixel corresponds to one byte. 
> > 
> > b. Pass the structure to cfb_imageblit. 
> > 
> > c. Read color information from pseudopalette if directcolor/truecolor. 
> > 
> > d. Write the pixel data in packed form to the framebuffer. 
> 
> I was think the same way except for the idea of more than one color 
> indices per byte for less than 8 bpp modes.
>  

Same argument as the above. Too much complexity, too little gain.

> > Secondly, indexing the cmap instead of the pseudo_palette means that
> > cfb_imageblit has to know the native framebuffer format.  
> 
> No indexing the cmap. Instead call fb_set_cmap before we call 
> xxxfb_imageblit. Personally I think we should call fb_set_cmap always.
> 

Only if the pseudo_palette is extended to > 16 entries, or some exotic
hardware requires it.  Otherwise, it's useless besides setting the color
register.

In any case, it's an easy fix.

> > I would rather have everything refer to
> > the pseudopalette, regardless of the visual format.  This will be better
> > especially for some of the corner cases, like monochrome cards with
> > bits_per_pixel = 8. 
> 
> It works pretty good for for any pack pixel type modes. Now for planar 
> cards this isn't the case. Ideally struct fb_cmap should have contained 
> a 

I don't see a problem here.  vga16fb is planar, and logo drawing works
just fine.

> 
> unsigned long pixel;
> 
> field. Like X does. It does not and changing that would break things :-(
>  

Isn't that what info->pseudo_palette is used for?

Tony



-------------------------------------------------------
This SF.NET email is sponsored by:
SourceForge Enterprise Edition + IBM + LinuxWorld = Something 2 See!
http://www.vasoftware.com