Re: [PATCH 2/2] drm: add an fb creation ioctl that takes a pixel format v4

["Ville Syrjälä" <ville.syrjala@linux.intel.com>]

On Mon, Nov 14, 2011 at 01:22:10PM -0800, Jesse Barnes wrote:
> On Mon, 14 Nov 2011 23:16:44 +0200
> Ville Syrjälä <ville.syrjala@linux.intel.com> wrote:
> 
> > On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
> > > +#define fourcc_code(a,b,c,d) ((u32)(a) | ((u32)(b) << 8) | \
> > > +			      ((u32)(c) << 16) | ((u32)(d) << 24))
> > > +
> > > +/* RGB codes */
> > > +#define DRM_FOURCC_RGB332 fourcc_code('R','G','B','1')
> > > +#define DRM_FOURCC_RGB555 fourcc_code('R','G','B','O')
> > > +#define DRM_FOURCC_RGB565 fourcc_code('R','G','B','P')
> > > +#define DRM_FOURCC_RGB24  fourcc_code('R','G','B','3')
> > > +#define DRM_FOURCC_RGB32  fourcc_code('R','G','B','4')
> > > +
> > > +#define DRM_FOURCC_BGR24  fourcc_code('B','G','R','3')
> > > +#define DRM_FOURCC_BGR32  fourcc_code('B','G','R','4')
> > 
> > I'm confused by these. The code suggests RGB/BGR24 are in fact 32bpp
> > formats, so why do we need the RGB/BGR32 variants? If the difference
> > is in the alpha channel, I'd like to document that fact in the name.
> > 
> > Could we call them ARGB8888, XRGB8888, XRGB1555 etc.?
> 
> Yeah, the fourcc naming isn't very good, I'd have no problem changing
> the names to something more descriptive for 24 and 32.  fourcc.org
> itself seems ambiguous about the 24 bit format....

AFAICS fourcc.org only has three RGB fourccs; RGB2, RGBA and RGBT. None
of which specify anything about the bpp or component order. The only
thing they seem to specify is which components are present.

> > Also the channel and byte order should be documented clearly.
 
> Supposedly the fourcc code is supposed to provide that?

As I said the RGB fourccs don't seem to specify either of these.
The YUV fourccs generally seem to be specify the byte order. For
RGB you typically specify the component order within a pixel.
Also AYUV seems to follow the "RGB way", and I think generally
three byte 24 bpp RGB formats follow the "YUV way".

But videodev2.h lists big endian variants for 555 and 565 format, which
leads me to think they intended everything else to be little endian.
Of course I can't be sure because it's not clearly documented. What a mess!

Also reading videodev2.h leads me to think that they intended
RGB24/BGR24 to be three byte formats in reality. How I came to that
conclusion? Look at the comment about depth. It lists depth=16 for all
the 16bpp formats regardless of their actual depth. So I think they
meant to write bpp when they wrote depth.

> > And one other thing. I probably wouldn't call these fourccs
> > since they don't actually match the official fourccs. Not that
> > there is anything sensible defined for RGB formats in the
> > official list anyway. In fact, I'm not sure what we gain by
> > cooking our own fourccs when we know most of them won't match
> > the official list. AFAICS a simple running number would do
> > just as well as the format identifier.
> 
> They seem to match what's at fourcc.org, though I don't see the upper
> byte documented, and also share values with the v4l stuff, which I was
> assuming had the right fourcc codes.
> 
> If we don't match, we should strive to.  I'm just using what I find at
> fourcc.org and in the v4l code to check...

I'm fine with fourccs as long as the defines are named and documented
in way that avoids guesswork.

So what I'm thinking is something like this:

DRM_FOURCC_RGB332      ... /* [7:0] R:G:B 3:3:2 */
DRM_FOURCC_XRGB1555    ... /* [15:0] x:R:G:B 1:5:5:5, native endian */
DRM_FOURCC_RGB565      ... /* [15:0] R:G:B 5:6:5, native endian */
DRM_FOURCC_XRGB8888    ... /* [31:0] x:R:G:B 8:8:8:8, native endian */
DRM_FOURCC_XRGB2101010 ... /* [31:0] x:R:G:B 2:10:10:10, native endian */

DRM_FOURCC_RGB888      ... /* [23:0] R:G:B 8:8:8, little endian */
DRM_FOURCC_BGR888      ... /* [23:0] B:G:R 8:8:8, little endian */

DRM_FOURCC_YUYV        ... /* [31:0] Cr:Y1:Cb:Y0 8:8:8:8, little endian */
DRM_FOURCC_UYVY        ... /* [31:0] Y1:Cr:Y0:Cb 8:8:8:8, little endian */
DRM_FOURCC_YVYU        ... /* [31:0] Cb:Y1:Cr:Y0 8:8:8:8, little endian */
DRM_FOURCC_VYUY        ... /* [31:0] Y1:Cb:Y0:Cr 8:8:8:8, little endian */

That leaves no room for guesswork.

-- 
Ville Syrjälä
Intel OTC