Re: [PATCH/RFC v2 1/3] fbdev: Add FOURCC-based format configuration API

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Geert,

Thanks for the review.

On Monday 29 August 2011 10:13:07 Geert Uytterhoeven wrote:
> On Fri, Aug 19, 2011 at 11:37, Laurent Pinchart wrote:

[snip]

> > +- FB_TYPE_PACKED_PIXELS
> > +
> > +Color components (usually RGB or YUV) are packed together into
> > macropixels +that are stored in a single plane. The exact color
> > components layout is +described in a visual-dependent way.
> > +
> > +Frame buffer visuals that don't use multiple color components per pixel
> > +(such as monochrome and pseudo-color visuals) are reported as packed
> > frame +buffer types, even though they don't stricly speaking pack color
> > components +into macropixels.
> 
> That's because the "packing" is not about the color components, but about
> the bits that represent a single pixel.
> 
> I.e. the bits that make up the pixel (the macropixel) are stored next
> to each other
> in memory.

OK, I've modified that last sentence to read

"Frame buffer visuals that don't use multiple color components per pixel (such 
as monochrome and pseudo-color visuals) are also reported as packed frame
buffer types, as the bits that make up individual pixels are packed next to
each other in memory."

> > +- FB_TYPE_PLANES
> > +
> > +Color components are stored in separate planes. Planes are located
> > +contiguously in memory.
> 
> The bits that make up a pixel are stored in separate planes. Planes are
> located contiguously in memory.

I'm not sure to agree with this. You make it sounds like FB_TYPE_PLANES stores 
each bit in a different plane. Is that really the case ?

> - FB_TYPE_INTERLEAVED_PLANES
> 
> The bits that make up a pixel are stored in separate planes. Planes
> are interleaved.
> The interleave factor (the distance in bytes between the planes in
> memory) is stored in the type_aux field.

That's a bit unclear to me. How are they interleaved ?

> > +- FB_VISUAL_MONO01
> > +
> > +Pixels are black or white and stored on one bit. A bit set to 1
> > represents a +black pixel and a bit set to 0 a white pixel. Pixels are
> > packed together in +bytes with 8 pixels per byte.
> 
> Actually we do have drivers that use 8 bits per pixel for a monochrome
> visual. Hence:
> 
> "Pixels are black or white. A black pixel is represented by all
> (typically one) bits set to ones, a white pixel by all bits set to zeroes."

OK. I've rephrased it as

"Pixels are black or white and stored on a number of bits (typically one)
specified by the variable screen information bpp field. 

Black pixels are represented by all bits set to 1 and white pixels by all bits
set to 0. When the number of bits per pixel is smaller than 8, several pixels 
are packed together in a byte."

> > +FB_VISUAL_MONO01 is used with FB_TYPE_PACKED_PIXELS only.
> 
> ... so this may also not be true (but it is for all current drivers, IIRC).
> There's a strict orthogonality between type (how is a pixel stored in
> memory) and visual (how the bits that represent the pixel are interpreted
> and converted to a color value).

What about

"FB_VISUAL_MONO01 is currently used with FB_TYPE_PACKED_PIXELS only." ?

> Same comments for FB_VISUAL_MONO10

Fixed the same way.

> > +- FB_VISUAL_TRUECOLOR
> > +
> > +Pixels are broken into red, green and blue components, and each
> > component +indexes a read-only lookup table for the corresponding value.
> > Lookup tables +are device-dependent, and provide linear or non-linear
> > ramps.
> > +
> > +Each component is stored in memory according to the variable screen
> > +information red, green, blue and transp fields.
> 
> "Each component is stored in a macropixel according to the variable screen
> information red, green, blue and transp fields."
> 
> Storage format in memory is determined by the FB_TYPE_* value.

How so ? With FB_TYPE_PLANES and FB_VISUAL_TRUECOLOR for an RGB format, how 
are the R, G and B planes ordered ? Are color components packed or padded 
inside a plane ? I understand that the design goal was to have orthogonal 
FB_TYPE_* and FB_VISUAL_* values, but we're missing too much information for 
that to be truly generic.

> > +- FB_VISUAL_PSEUDOCOLOR and FB_VISUAL_STATIC_PSEUDOCOLOR
> > +
> > +Pixel values are encoded as indices into a colormap that stores red,
> > green and +blue components. The colormap is read-only for
> > FB_VISUAL_STATIC_PSEUDOCOLOR +and read-write for FB_VISUAL_PSEUDOCOLOR.
> > +
> > +Each pixel value is stored in the number of bits reported by the
> > variable +screen information bits_per_pixel field. Pixels are contiguous
> > in memory.
> 
> Whether pixels are contiguous in memory or not is determined by the
> FB_TYPE_* value.

How can they not be contiguous in memory ? Can you please give an example ?

> > +FB_VISUAL_PSEUDOCOLOR and FB_VISUAL_STATIC_PSEUDOCOLOR are used with
> > +FB_TYPE_PACKED_PIXELS only.
> 
> Not true. Several drivers use bit planes or interleaved bitplanes.

How does that work ?

> > +- FB_VISUAL_DIRECTCOLOR
> > +
> > +Pixels are broken into red, green and blue components, and each
> > component +indexes a programmable lookup table for the corresponding
> > value. +
> > +Each component is stored in memory according to the variable screen
> > +information red, green, blue and transp fields.
> 
> "Each component is stored in a macropixel according to the variable screen
> information red, green, blue and transp fields."
> 
> > +- FB_VISUAL_FOURCC
> > +
> > +Pixels are stored in memory as described by the format FOURCC identifier
> > +stored in the variable screen information fourcc field.
> 
> ... stored in memory and interpreted ...
> 
> > +struct fb_var_screeninfo {
> > +       __u32 xres;                     /* visible resolution          
> > */ +       __u32 yres;
> > +       __u32 xres_virtual;             /* virtual resolution          
> > */ +       __u32 yres_virtual;
> > +       __u32 xoffset;                  /* offset from virtual to visible
> > */ +       __u32 yoffset;                  /* resolution                
> >   */ +
> > +       __u32 bits_per_pixel;           /* guess what                  
> > */ +       union {
> > +               struct {                /* Legacy format API          
> >  */ +                       __u32 grayscale; /* != 0 Graylevels instead
> > of colors */ +                       /* bitfields in fb mem if true
> > color, else only */ +                       /* length is significant    
> >                    */ +                       struct fb_bitfield red;
> > +                       struct fb_bitfield green;
> > +                       struct fb_bitfield blue;
> > +                       struct fb_bitfield transp;      /* transparency
> > */ +               };
> > +               struct {                /* FOURCC-based format API    
> >  */ +                       __u32 fourcc;           /* FOURCC format    
> >    */ +                       __u32 colorspace;
> > +                       __u32 reserved[11];
> > +               } format;
> > +       };
> > +
> > +       struct fb_bitfield red;         /* bitfield in fb mem if true
> > color, */ +       struct fb_bitfield green;       /* else only length is
> > significant */ +       struct fb_bitfield blue;
> > +       struct fb_bitfield transp;      /* transparency                
> > */
> 
> These four are duplicated, cfr. the union above.

Oops :-)

-- 
Regards,

Laurent Pinchart