RE: [Linux-fbdev-devel] Re: [PATCH] fbdev accel wrapper. II

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

> James,
>   If the upper layers don't know what the data is (Because it is
> device specific) then why can they even see it? Device specific
> data should remain in device specific data structures. Just because
> almost all cards are going to need a similar place to store the
> console colors doesn't mean it goes in a device independent data
> structure. (I realize it's always been there, and it isn't the only
> device dependent thing in there, but that doesn't make it correct)

I know. I have been doing even more deep thinking about this. First lets
look at what the original intent was. Originally nearly all fbdev drivers
have something like:

  union {
#ifdef FBCON_HAS_CFB16
    u16 cfb16[16];
#endif
#ifdef FBCON_HAS_CFB24
    u32 cfb24[16];
#endif
#ifdef FBCON_HAS_CFB32
    u32 cfb32[16];
#endif
  } fbcon_cmap;

Which is just plain disgusting. So my original motivation was to remove
this. Replace it with a cleaner pseudo_palette that everyone could us. I
original thought about having it a u32 field. Then I wanted to trim the
amount of memory used and I also wanted to expand the idea behind
pseudo_palette. For example we have 17 instead of 16 indexs where the 17
index is used to store the cursor XOR mask. Also since my major goal was
to remove struct display complete I wanted to replace void *dispsw_data in
struct display with something like it in struct fb_info. So pseudo_palette
was the replacement. 
    The original idea behind dispsw_data was to provide a quick fake color
palette for fbcon. struct fb_cmap represents the color map and the
console system emulates a device with a color palette. Well for non-palette 
modes we had to fake a color palette. Their is no real need for fb_cmap
here. So the values in dispsw_data was the values we wrote to the pixels
in the framebuffer memory. So it was

console palette           fbcon  
   PSEUDOCOLOR            TRUECOLOR 
   color -> [index] ---> [index]->dispsw_data -> pixel value ->
				                 place value into memory

   PSEUDOCOLOR            PSEUDOCOLOR
   color -> [index] ---> [index] -> place value of index itself into
                                    memory  

Where fb_set_cmap would have set the hardware color palette to match the 
software one.

     I wanted to see pseudo_palette to be evern more than this. It could
also be a array of device specific structs to represent who knows what for
a device. Values that you need to write to hardware reisters for example.  
     
------------------------------------------------------------------------------

So that is what the original ideas where. Now to what I'm thinking know.

>   What you need is a color format in your region (which I think you
> have when doing image blit). 

Actually struct fb_fillrect already has a color field :-) 

> Then you just send the palette entry
> for the color. Fillrect does a switch for the types of things it can
> do in hardware or via a quick lookup or it returns an error.
> Basically fill rect acts just like an image blit.
> 
> Something like this:

>     region.color = attr_bgcol_ec(p,vc);
>     region.visual = FB_VISUAL_PSEUDOCOLOR;

>     info->fbops->fb_fillrect(info, &region);
> }

We don't have to worry about the visual field since this is in 
info->fix.visual. That can be handled internall by xxxfb_fillrect. I have
been think about just sending the colormap index. Since you can create a
color at anytime via fb_set_cmap and we have a color map in struct fb_info. 
We can use the index placed into region.color and after passing to the
below use the color map in struct fb_info to extract the data we need to
pass to the hardware. As Petr pointed out we can extract values that can
speed up the accelerators. It hides more data away from the upper layers.
The only thing is I hope people don't do the above fbcon_cmap mess again.
Of course that is what code review is for. 

> Perhaps we can take a step back and look at all the data structures
> and justify why things are the way they are. If you are changing the
> data structures we might as well make sure they make sense.

Expect alot of useless stuff to be removed in the near future.