Menu
▾
▴
linux-fbdev-devel
|
From: Geert U. <ge...@li...> - 2003-01-05 16:39:49
|
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 -- 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
|
|
From: Antonino D. <ad...@po...> - 2003-01-05 18:19:22
|
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 |
|
From: Geert U. <ge...@li...> - 2003-01-05 19:00:37
|
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 -- 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
|
|
From: James S. <jsi...@in...> - 2003-01-07 22:25:05
|
> > 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. |
|
From: Antonino D. <ad...@po...> - 2003-01-08 02:56:23
|
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 |
|
From: Geert U. <ge...@li...> - 2003-01-08 15:18:41
|
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 -- 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
|
|
From: Antonino D. <ad...@po...> - 2003-01-08 16:37:07
|
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 |
|
From: Geert U. <ge...@li...> - 2003-01-08 16:51:36
|
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 -- 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
|
|
From: James S. <jsi...@in...> - 2003-01-10 18:38:16
|
> Bummer, /me should read the code more thoroughfully...
>
> Perhaps renaming `{saved_,}palette' to `{saved_,}pseudo_palette' would make
> this clearer...
I renamed it.
|
|
From: Geert U. <ge...@li...> - 2003-01-09 09:52:34
|
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 -- 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
|
|
From: Antonino D. <ad...@po...> - 2003-01-09 11:56:09
|
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
|
|
From: James S. <jsi...@in...> - 2003-01-10 18:48:39
|
> 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 :-) |
|
From: James S. <jsi...@in...> - 2003-01-10 19:24:17
|
> 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 :-( |
|
From: Geert U. <ge...@li...> - 2003-01-10 19:42:47
|
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 -- 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
|
|
From: Antonino D. <ad...@po...> - 2003-01-11 05:21:58
|
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 |
Thanks for helping keep SourceForge clean.
X