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

2011-11-15 Thread Ville Syrjälä 
On Tue, Nov 15, 2011 at 08:16:04AM -0800, Jesse Barnes wrote:
> On Tue, 15 Nov 2011 14:57:02 +0200
> Ville Syrj?l?  wrote:
> > 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_XRGB... /* [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.
> 
> Looks great.  Want to send Dave an incremental patch?  I'll apply the
> final version to libdrm for use by userland code.

What I listed there doesn't match what v4l2 has. So I'm not sure what
to put in a patch.

It looks like the v4l2 fourccs have explicit endianness (ie. LE or BE).
If we follow that, and assuming people still want to use hardware byte
swappers, it means user space needs some ifdefs to select the approriate
format based on the host endianness. Or, we could do that in the header
file itself, so we would provide three definitions for each format LE,
BE, and NE (which would point to LE or BE depending on host endianness).

One extra issue I just realized is that the 8bpp and 16bpp v4l2 formats
are in fact BGR nor RGB, that is the component order is such that blue
occupies the most significant bit, red the lsb. I've never even seen
a PC graphics card that supports such formats. Adding insult to injury
PIX_FMT_RGB444 is defined the opposite way, ie. matching what most
graphics cards would expect.

-- 
Ville Syrj?l?
Intel OTC

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

2011-11-15 Thread Ville Syrjälä 
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?  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 ARGB, XRGB, 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_XRGB... /* [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

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

2011-11-15 Thread Jesse Barnes 
On Tue, 15 Nov 2011 22:30:36 +0200
Ville Syrj?l?  wrote:

> On Tue, Nov 15, 2011 at 08:16:04AM -0800, Jesse Barnes wrote:
> > On Tue, 15 Nov 2011 14:57:02 +0200
> > Ville Syrj?l?  wrote:
> > > 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_XRGB... /* [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.
> > 
> > Looks great.  Want to send Dave an incremental patch?  I'll apply the
> > final version to libdrm for use by userland code.
> 
> What I listed there doesn't match what v4l2 has. So I'm not sure what
> to put in a patch.
> 
> It looks like the v4l2 fourccs have explicit endianness (ie. LE or BE).
> If we follow that, and assuming people still want to use hardware byte
> swappers, it means user space needs some ifdefs to select the approriate
> format based on the host endianness. Or, we could do that in the header
> file itself, so we would provide three definitions for each format LE,
> BE, and NE (which would point to LE or BE depending on host endianness).
> 
> One extra issue I just realized is that the 8bpp and 16bpp v4l2 formats
> are in fact BGR nor RGB, that is the component order is such that blue
> occupies the most significant bit, red the lsb. I've never even seen
> a PC graphics card that supports such formats. Adding insult to injury
> PIX_FMT_RGB444 is defined the opposite way, ie. matching what most
> graphics cards would expect.

Heh, well you can just pick whatever makes sense then for RGB, and
remove the _FOURCC_ strings to make it clear we're using sane RGB
definitions and not some half-specified fourcc stuff.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

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

2011-11-15 Thread Jesse Barnes 
On Tue, 15 Nov 2011 14:57:02 +0200
Ville Syrj?l?  wrote:
> 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_XRGB... /* [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.

Looks great.  Want to send Dave an incremental patch?  I'll apply the
final version to libdrm for use by userland code.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

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

2011-11-15 Thread Ville Syrjälä 
On Mon, Nov 14, 2011 at 01:35:57PM -0800, Jesse Barnes wrote:
> On Mon, 14 Nov 2011 23:24:55 +0200
> Ville Syrj?l?  wrote:
> 
> > On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
> > > +struct drm_mode_fb_cmd2 {
> > > + __u32 fb_id;
> > > + __u32 width, height;
> > > + __u32 pixel_format; /* fourcc code from videodev2.h */
> > > +
> > > + /*
> > > +  * In case of planar formats, this ioctl allows up to 4
> > > +  * buffer objects with offets and pitches per plane.
> > > +  * The pitch and offset order is dictated by the fourcc,
> > > +  * e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
> > > +  *
> > > +  *   YUV 4:2:0 image with a plane of 8 bit Y samples
> > > +  *   followed by an interleaved U/V plane containing
> > > +  *   8 bit 2x2 subsampled colour difference samples.
> > > +  *
> > > +  * So it would consist of Y as offset[0] and UV as
> > > +  * offeset[1].  Note that offset[0] will generally
> > > +  * be 0.
> > > +  */
> > > + __u32 handles[4];
> > > + __u32 pitches[4]; /* pitch for each plane */
> > > + __u32 offsets[4]; /* offset of each plane */
> > > +};
> > 
> > Hey, what about those interlaced buffers? We talked privately about
> > adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
> > drm_mode_set_plane.
> > 
> > We could stick something like these to those flags:
> > fb_cmd2.flags:
> > #define DRM_MODE_FB_INTERLACED 0x1
> > 
> > set_plane.flags:
> > #define DRM_MODE_PRESENT_TOP_FIELD 0x1
> > #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2
> 
> Oh sorry I lost track of the internal discussion.
> 
> Are those attributes of the fb or of each object?  E.g. could you mix
> interlaced and non-interlaced buffers in a planar format?

I suppose it might be possible that you'd want to treat luma as
interlaced and chroma as progressive under some circumstances.
But I can't see why simply defining another flag for that wouldn't
be enough.

> Maybe I need to rename this ioctl to addfb_swissarmyknife :)

Now I'm just waiting for someone to jump in and say that they 
want independent buffers for each interlaced field :)

Me? I'll be happy with just those two flags members... for now at least ;)

-- 
Ville Syrj?l?
Intel OTC

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

2011-11-15 Thread Ville Syrjälä 
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.syrj...@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 ARGB, XRGB, 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_XRGB... /* [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
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-15 Thread Jesse Barnes 
On Tue, 15 Nov 2011 14:57:02 +0200
Ville Syrjälä ville.syrj...@linux.intel.com wrote:
 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_XRGB... /* [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.

Looks great.  Want to send Dave an incremental patch?  I'll apply the
final version to libdrm for use by userland code.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-15 Thread Ville Syrjälä 
On Tue, Nov 15, 2011 at 08:16:04AM -0800, Jesse Barnes wrote:
 On Tue, 15 Nov 2011 14:57:02 +0200
 Ville Syrjälä ville.syrj...@linux.intel.com wrote:
  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_XRGB... /* [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.
 
 Looks great.  Want to send Dave an incremental patch?  I'll apply the
 final version to libdrm for use by userland code.

What I listed there doesn't match what v4l2 has. So I'm not sure what
to put in a patch.

It looks like the v4l2 fourccs have explicit endianness (ie. LE or BE).
If we follow that, and assuming people still want to use hardware byte
swappers, it means user space needs some ifdefs to select the approriate
format based on the host endianness. Or, we could do that in the header
file itself, so we would provide three definitions for each format LE,
BE, and NE (which would point to LE or BE depending on host endianness).

One extra issue I just realized is that the 8bpp and 16bpp v4l2 formats
are in fact BGR nor RGB, that is the component order is such that blue
occupies the most significant bit, red the lsb. I've never even seen
a PC graphics card that supports such formats. Adding insult to injury
PIX_FMT_RGB444 is defined the opposite way, ie. matching what most
graphics cards would expect.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-15 Thread Jesse Barnes 
On Tue, 15 Nov 2011 22:30:36 +0200
Ville Syrjälä ville.syrj...@linux.intel.com wrote:

 On Tue, Nov 15, 2011 at 08:16:04AM -0800, Jesse Barnes wrote:
  On Tue, 15 Nov 2011 14:57:02 +0200
  Ville Syrjälä ville.syrj...@linux.intel.com wrote:
   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_XRGB... /* [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.
  
  Looks great.  Want to send Dave an incremental patch?  I'll apply the
  final version to libdrm for use by userland code.
 
 What I listed there doesn't match what v4l2 has. So I'm not sure what
 to put in a patch.
 
 It looks like the v4l2 fourccs have explicit endianness (ie. LE or BE).
 If we follow that, and assuming people still want to use hardware byte
 swappers, it means user space needs some ifdefs to select the approriate
 format based on the host endianness. Or, we could do that in the header
 file itself, so we would provide three definitions for each format LE,
 BE, and NE (which would point to LE or BE depending on host endianness).
 
 One extra issue I just realized is that the 8bpp and 16bpp v4l2 formats
 are in fact BGR nor RGB, that is the component order is such that blue
 occupies the most significant bit, red the lsb. I've never even seen
 a PC graphics card that supports such formats. Adding insult to injury
 PIX_FMT_RGB444 is defined the opposite way, ie. matching what most
 graphics cards would expect.

Heh, well you can just pick whatever makes sense then for RGB, and
remove the _FOURCC_ strings to make it clear we're using sane RGB
definitions and not some half-specified fourcc stuff.

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Ville Syrjälä 
On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
> +struct drm_mode_fb_cmd2 {
> + __u32 fb_id;
> + __u32 width, height;
> + __u32 pixel_format; /* fourcc code from videodev2.h */
> +
> + /*
> +  * In case of planar formats, this ioctl allows up to 4
> +  * buffer objects with offets and pitches per plane.
> +  * The pitch and offset order is dictated by the fourcc,
> +  * e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
> +  *
> +  *   YUV 4:2:0 image with a plane of 8 bit Y samples
> +  *   followed by an interleaved U/V plane containing
> +  *   8 bit 2x2 subsampled colour difference samples.
> +  *
> +  * So it would consist of Y as offset[0] and UV as
> +  * offeset[1].  Note that offset[0] will generally
> +  * be 0.
> +  */
> + __u32 handles[4];
> + __u32 pitches[4]; /* pitch for each plane */
> + __u32 offsets[4]; /* offset of each plane */
> +};

Hey, what about those interlaced buffers? We talked privately about
adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
drm_mode_set_plane.

We could stick something like these to those flags:
fb_cmd2.flags:
#define DRM_MODE_FB_INTERLACED 0x1

set_plane.flags:
#define DRM_MODE_PRESENT_TOP_FIELD 0x1
#define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2

-- 
Ville Syrj?l?
Intel OTC

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

2011-11-14 Thread Ville Syrjälä 
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 ARGB, XRGB, XRGB1555 etc.?

Also the channel and byte order should be documented clearly.

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.

-- 
Ville Syrj?l?
Intel OTC

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

2011-11-14 Thread Rob Clark 
On Mon, Nov 14, 2011 at 3:16 PM, Ville Syrj?l?
 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 ARGB, XRGB, XRGB1555 etc.?
>
> Also the channel and byte order should be documented clearly.
>
> 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.

I expect that v4l just made up their own fourcc values for some of the
RGB formats..  which isn't a horrible idea, and seems worthwhile to be
aligned with names/values that v4l already picked.  At least then we
don't have two different sets of more or less arbitrary rgb fourcc
names..

BR,
-R

> --
> Ville Syrj?l?
> Intel OTC
> ___
> dri-devel mailing list
> dri-devel at lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel
>

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

2011-11-14 Thread Jesse Barnes 
On Tue, 15 Nov 2011 00:37:47 +0200
Ville Syrj?l?  wrote:

> On Mon, Nov 14, 2011 at 01:35:57PM -0800, Jesse Barnes wrote:
> > On Mon, 14 Nov 2011 23:24:55 +0200
> > Ville Syrj?l?  wrote:
> > 
> > > On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
> > > > +struct drm_mode_fb_cmd2 {
> > > > +   __u32 fb_id;
> > > > +   __u32 width, height;
> > > > +   __u32 pixel_format; /* fourcc code from videodev2.h */
> > > > +
> > > > +   /*
> > > > +* In case of planar formats, this ioctl allows up to 4
> > > > +* buffer objects with offets and pitches per plane.
> > > > +* The pitch and offset order is dictated by the fourcc,
> > > > +* e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
> > > > +*
> > > > +*   YUV 4:2:0 image with a plane of 8 bit Y samples
> > > > +*   followed by an interleaved U/V plane containing
> > > > +*   8 bit 2x2 subsampled colour difference samples.
> > > > +*
> > > > +* So it would consist of Y as offset[0] and UV as
> > > > +* offeset[1].  Note that offset[0] will generally
> > > > +* be 0.
> > > > +*/
> > > > +   __u32 handles[4];
> > > > +   __u32 pitches[4]; /* pitch for each plane */
> > > > +   __u32 offsets[4]; /* offset of each plane */
> > > > +};
> > > 
> > > Hey, what about those interlaced buffers? We talked privately about
> > > adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
> > > drm_mode_set_plane.
> > > 
> > > We could stick something like these to those flags:
> > > fb_cmd2.flags:
> > > #define DRM_MODE_FB_INTERLACED 0x1
> > > 
> > > set_plane.flags:
> > > #define DRM_MODE_PRESENT_TOP_FIELD 0x1
> > > #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2
> > 
> > Oh sorry I lost track of the internal discussion.
> > 
> > Are those attributes of the fb or of each object?  E.g. could you mix
> > interlaced and non-interlaced buffers in a planar format?
> 
> I suppose it might be possible that you'd want to treat luma as
> interlaced and chroma as progressive under some circumstances.
> But I can't see why simply defining another flag for that wouldn't
> be enough.
> 
> > Maybe I need to rename this ioctl to addfb_swissarmyknife :)
> 
> Now I'm just waiting for someone to jump in and say that they 
> want independent buffers for each interlaced field :)
> 
> Me? I'll be happy with just those two flags members... for now at least ;)

Ok, added, see the latest patchset.

Dave, please pound the gavel on this now and declare it sold before
someone asks me to add braille support. :)

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

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

2011-11-14 Thread Jesse Barnes 
On Mon, 14 Nov 2011 23:24:55 +0200
Ville Syrj?l?  wrote:

> On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
> > +struct drm_mode_fb_cmd2 {
> > +   __u32 fb_id;
> > +   __u32 width, height;
> > +   __u32 pixel_format; /* fourcc code from videodev2.h */
> > +
> > +   /*
> > +* In case of planar formats, this ioctl allows up to 4
> > +* buffer objects with offets and pitches per plane.
> > +* The pitch and offset order is dictated by the fourcc,
> > +* e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
> > +*
> > +*   YUV 4:2:0 image with a plane of 8 bit Y samples
> > +*   followed by an interleaved U/V plane containing
> > +*   8 bit 2x2 subsampled colour difference samples.
> > +*
> > +* So it would consist of Y as offset[0] and UV as
> > +* offeset[1].  Note that offset[0] will generally
> > +* be 0.
> > +*/
> > +   __u32 handles[4];
> > +   __u32 pitches[4]; /* pitch for each plane */
> > +   __u32 offsets[4]; /* offset of each plane */
> > +};
> 
> Hey, what about those interlaced buffers? We talked privately about
> adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
> drm_mode_set_plane.
> 
> We could stick something like these to those flags:
> fb_cmd2.flags:
> #define DRM_MODE_FB_INTERLACED 0x1
> 
> set_plane.flags:
> #define DRM_MODE_PRESENT_TOP_FIELD 0x1
> #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2

Oh sorry I lost track of the internal discussion.

Are those attributes of the fb or of each object?  E.g. could you mix
interlaced and non-interlaced buffers in a planar format?

Maybe I need to rename this ioctl to addfb_swissarmyknife :)

-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

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

2011-11-14 Thread Jesse Barnes 
On Mon, 14 Nov 2011 23:16:44 +0200
Ville Syrj?l?  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 ARGB, XRGB, 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

> Also the channel and byte order should be documented clearly.

Supposedly the fourcc code is supposed to provide that?

> 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...

-- 
Jesse Barnes, Intel Open Source Technology Center
-- next part --
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 836 bytes
Desc: not available
URL:

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

2011-11-14 Thread Jesse Barnes 
To properly support the various plane formats supported by different
hardware, the kernel must know the pixel format of a framebuffer object.
So add a new ioctl taking a format argument corresponding to a fourcc
name from the new drm_fourcc.h header file.  Implement the fb creation
hooks in terms of the new mode_fb_cmd2 using helpers where the old
bpp/depth values are needed.

v2: create DRM specific fourcc header file for sharing with libdrm etc
v3: fix rebase failure and use DRM fourcc codes in intel_display.c and
update commit message
v4: make fb_cmd2 handle field into an array for multi-object formats
pull in Ville's fix for the memcpy in drm_plane_init
apply Ville's cleanup to zero out fb_cmd2 arg in drm_mode_addfb

Signed-off-by: Ville Syrj?l? 
Acked-by: Alan Cox 
Reviewed-by: Rob Clark 
Signed-off-by: Jesse Barnes 
---
 drivers/gpu/drm/drm_crtc.c|  111 +++--
 drivers/gpu/drm/drm_crtc_helper.c |   51 -
 drivers/gpu/drm/drm_drv.c |1 +
 drivers/gpu/drm/i915/intel_display.c  |   39 ++-
 drivers/gpu/drm/i915/intel_drv.h  |2 +-
 drivers/gpu/drm/i915/intel_fb.c   |   11 ++--
 drivers/gpu/drm/nouveau/nouveau_display.c |6 +-
 drivers/gpu/drm/nouveau/nouveau_fb.h  |2 +-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   13 ++--
 drivers/gpu/drm/radeon/radeon_display.c   |8 +-
 drivers/gpu/drm/radeon/radeon_fb.c|   18 +++--
 drivers/gpu/drm/radeon/radeon_mode.h  |2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |   22 --
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h   |1 +
 drivers/staging/gma500/framebuffer.c  |2 +-
 include/drm/drm.h |1 +
 include/drm/drm_crtc.h|9 ++-
 include/drm/drm_crtc_helper.h |4 +-
 include/drm/drm_fourcc.h  |   63 
 include/drm/drm_mode.h|   24 ++
 20 files changed, 324 insertions(+), 66 deletions(-)
 create mode 100644 include/drm/drm_fourcc.h

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 804ef12..30a70a4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -35,6 +35,7 @@
 #include "drmP.h"
 #include "drm_crtc.h"
 #include "drm_edid.h"
+#include "drm_fourcc.h"

 struct drm_prop_enum_list {
int type;
@@ -563,7 +564,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane 
*plane,
return -ENOMEM;
}

-   memcpy(plane->format_types, formats, format_count);
+   memcpy(plane->format_types, formats, format_count * sizeof(uint32_t));
plane->format_count = format_count;
plane->possible_crtcs = possible_crtcs;

@@ -1910,6 +1911,42 @@ out:
return ret;
 }

+/* Original addfb only supported RGB formats, so figure out which one */
+uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
+{
+   uint32_t fmt;
+
+   switch (bpp) {
+   case 8:
+   fmt = DRM_FOURCC_RGB332;
+   break;
+   case 16:
+   if (depth == 15)
+   fmt = DRM_FOURCC_RGB555;
+   else
+   fmt = DRM_FOURCC_RGB565;
+   break;
+   case 24:
+   fmt = DRM_FOURCC_RGB24;
+   break;
+   case 32:
+   if (depth == 24)
+   fmt = DRM_FOURCC_RGB24;
+   else if (depth == 30)
+   fmt = DRM_INTEL_RGB30;
+   else
+   fmt = DRM_FOURCC_RGB32;
+   break;
+   default:
+   DRM_ERROR("bad bpp, assuming RGB24 pixel format\n");
+   fmt = DRM_FOURCC_RGB24;
+   break;
+   }
+
+   return fmt;
+}
+EXPORT_SYMBOL(drm_mode_legacy_fb_format);
+
 /**
  * drm_mode_addfb - add an FB to the graphics configuration
  * @inode: inode from the ioctl
@@ -1930,7 +1967,74 @@ out:
 int drm_mode_addfb(struct drm_device *dev,
   void *data, struct drm_file *file_priv)
 {
-   struct drm_mode_fb_cmd *r = data;
+   struct drm_mode_fb_cmd *or = data;
+   struct drm_mode_fb_cmd2 r = {};
+   struct drm_mode_config *config = >mode_config;
+   struct drm_framebuffer *fb;
+   int ret = 0;
+
+   /* Use new struct with format internally */
+   r.fb_id = or->fb_id;
+   r.width = or->width;
+   r.height = or->height;
+   r.pitches[0] = or->pitch;
+   r.pixel_format = drm_mode_legacy_fb_format(or->bpp, or->depth);
+   r.handles[0] = or->handle;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   if ((config->min_width > r.width) || (r.width > config->max_width)) {
+   DRM_ERROR("mode new framebuffer width not within limits\n");
+   return -EINVAL;
+   }
+   if ((config->min_height > r.height) || (r.height > config->max_height)) 
{
+

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

2011-11-14 Thread Jesse Barnes 
To properly support the various plane formats supported by different
hardware, the kernel must know the pixel format of a framebuffer object.
So add a new ioctl taking a format argument corresponding to a fourcc
name from the new drm_fourcc.h header file.  Implement the fb creation
hooks in terms of the new mode_fb_cmd2 using helpers where the old
bpp/depth values are needed.

v2: create DRM specific fourcc header file for sharing with libdrm etc
v3: fix rebase failure and use DRM fourcc codes in intel_display.c and
update commit message
v4: make fb_cmd2 handle field into an array for multi-object formats
pull in Ville's fix for the memcpy in drm_plane_init
apply Ville's cleanup to zero out fb_cmd2 arg in drm_mode_addfb

Signed-off-by: Ville Syrjälä ville.syrj...@linux.intel.com
Acked-by: Alan Cox a...@lxorguk.ukuu.org.uk
Reviewed-by: Rob Clark rob.cl...@linaro.org
Signed-off-by: Jesse Barnes jbar...@virtuousgeek.org
---
 drivers/gpu/drm/drm_crtc.c|  111 +++--
 drivers/gpu/drm/drm_crtc_helper.c |   51 -
 drivers/gpu/drm/drm_drv.c |1 +
 drivers/gpu/drm/i915/intel_display.c  |   39 ++-
 drivers/gpu/drm/i915/intel_drv.h  |2 +-
 drivers/gpu/drm/i915/intel_fb.c   |   11 ++--
 drivers/gpu/drm/nouveau/nouveau_display.c |6 +-
 drivers/gpu/drm/nouveau/nouveau_fb.h  |2 +-
 drivers/gpu/drm/nouveau/nouveau_fbcon.c   |   13 ++--
 drivers/gpu/drm/radeon/radeon_display.c   |8 +-
 drivers/gpu/drm/radeon/radeon_fb.c|   18 +++--
 drivers/gpu/drm/radeon/radeon_mode.h  |2 +-
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.c   |   22 --
 drivers/gpu/drm/vmwgfx/vmwgfx_kms.h   |1 +
 drivers/staging/gma500/framebuffer.c  |2 +-
 include/drm/drm.h |1 +
 include/drm/drm_crtc.h|9 ++-
 include/drm/drm_crtc_helper.h |4 +-
 include/drm/drm_fourcc.h  |   63 
 include/drm/drm_mode.h|   24 ++
 20 files changed, 324 insertions(+), 66 deletions(-)
 create mode 100644 include/drm/drm_fourcc.h

diff --git a/drivers/gpu/drm/drm_crtc.c b/drivers/gpu/drm/drm_crtc.c
index 804ef12..30a70a4 100644
--- a/drivers/gpu/drm/drm_crtc.c
+++ b/drivers/gpu/drm/drm_crtc.c
@@ -35,6 +35,7 @@
 #include drmP.h
 #include drm_crtc.h
 #include drm_edid.h
+#include drm_fourcc.h
 
 struct drm_prop_enum_list {
int type;
@@ -563,7 +564,7 @@ int drm_plane_init(struct drm_device *dev, struct drm_plane 
*plane,
return -ENOMEM;
}
 
-   memcpy(plane-format_types, formats, format_count);
+   memcpy(plane-format_types, formats, format_count * sizeof(uint32_t));
plane-format_count = format_count;
plane-possible_crtcs = possible_crtcs;
 
@@ -1910,6 +1911,42 @@ out:
return ret;
 }
 
+/* Original addfb only supported RGB formats, so figure out which one */
+uint32_t drm_mode_legacy_fb_format(uint32_t bpp, uint32_t depth)
+{
+   uint32_t fmt;
+
+   switch (bpp) {
+   case 8:
+   fmt = DRM_FOURCC_RGB332;
+   break;
+   case 16:
+   if (depth == 15)
+   fmt = DRM_FOURCC_RGB555;
+   else
+   fmt = DRM_FOURCC_RGB565;
+   break;
+   case 24:
+   fmt = DRM_FOURCC_RGB24;
+   break;
+   case 32:
+   if (depth == 24)
+   fmt = DRM_FOURCC_RGB24;
+   else if (depth == 30)
+   fmt = DRM_INTEL_RGB30;
+   else
+   fmt = DRM_FOURCC_RGB32;
+   break;
+   default:
+   DRM_ERROR(bad bpp, assuming RGB24 pixel format\n);
+   fmt = DRM_FOURCC_RGB24;
+   break;
+   }
+
+   return fmt;
+}
+EXPORT_SYMBOL(drm_mode_legacy_fb_format);
+
 /**
  * drm_mode_addfb - add an FB to the graphics configuration
  * @inode: inode from the ioctl
@@ -1930,7 +1967,74 @@ out:
 int drm_mode_addfb(struct drm_device *dev,
   void *data, struct drm_file *file_priv)
 {
-   struct drm_mode_fb_cmd *r = data;
+   struct drm_mode_fb_cmd *or = data;
+   struct drm_mode_fb_cmd2 r = {};
+   struct drm_mode_config *config = dev-mode_config;
+   struct drm_framebuffer *fb;
+   int ret = 0;
+
+   /* Use new struct with format internally */
+   r.fb_id = or-fb_id;
+   r.width = or-width;
+   r.height = or-height;
+   r.pitches[0] = or-pitch;
+   r.pixel_format = drm_mode_legacy_fb_format(or-bpp, or-depth);
+   r.handles[0] = or-handle;
+
+   if (!drm_core_check_feature(dev, DRIVER_MODESET))
+   return -EINVAL;
+
+   if ((config-min_width  r.width) || (r.width  config-max_width)) {
+   DRM_ERROR(mode new framebuffer width not within limits\n);
+   return -EINVAL;
+   }
+

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

2011-11-14 Thread Ville Syrjälä 
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 ARGB, XRGB, XRGB1555 etc.?

Also the channel and byte order should be documented clearly.

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.

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Jesse Barnes 
On Mon, 14 Nov 2011 23:16:44 +0200
Ville Syrjälä ville.syrj...@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 ARGB, XRGB, 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

 Also the channel and byte order should be documented clearly.

Supposedly the fourcc code is supposed to provide that?

 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...

-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Ville Syrjälä 
On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
 +struct drm_mode_fb_cmd2 {
 + __u32 fb_id;
 + __u32 width, height;
 + __u32 pixel_format; /* fourcc code from videodev2.h */
 +
 + /*
 +  * In case of planar formats, this ioctl allows up to 4
 +  * buffer objects with offets and pitches per plane.
 +  * The pitch and offset order is dictated by the fourcc,
 +  * e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
 +  *
 +  *   YUV 4:2:0 image with a plane of 8 bit Y samples
 +  *   followed by an interleaved U/V plane containing
 +  *   8 bit 2x2 subsampled colour difference samples.
 +  *
 +  * So it would consist of Y as offset[0] and UV as
 +  * offeset[1].  Note that offset[0] will generally
 +  * be 0.
 +  */
 + __u32 handles[4];
 + __u32 pitches[4]; /* pitch for each plane */
 + __u32 offsets[4]; /* offset of each plane */
 +};

Hey, what about those interlaced buffers? We talked privately about
adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
drm_mode_set_plane.

We could stick something like these to those flags:
fb_cmd2.flags:
#define DRM_MODE_FB_INTERLACED 0x1

set_plane.flags:
#define DRM_MODE_PRESENT_TOP_FIELD 0x1
#define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Jesse Barnes 
On Mon, 14 Nov 2011 23:24:55 +0200
Ville Syrjälä ville.syrj...@linux.intel.com wrote:

 On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
  +struct drm_mode_fb_cmd2 {
  +   __u32 fb_id;
  +   __u32 width, height;
  +   __u32 pixel_format; /* fourcc code from videodev2.h */
  +
  +   /*
  +* In case of planar formats, this ioctl allows up to 4
  +* buffer objects with offets and pitches per plane.
  +* The pitch and offset order is dictated by the fourcc,
  +* e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
  +*
  +*   YUV 4:2:0 image with a plane of 8 bit Y samples
  +*   followed by an interleaved U/V plane containing
  +*   8 bit 2x2 subsampled colour difference samples.
  +*
  +* So it would consist of Y as offset[0] and UV as
  +* offeset[1].  Note that offset[0] will generally
  +* be 0.
  +*/
  +   __u32 handles[4];
  +   __u32 pitches[4]; /* pitch for each plane */
  +   __u32 offsets[4]; /* offset of each plane */
  +};
 
 Hey, what about those interlaced buffers? We talked privately about
 adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
 drm_mode_set_plane.
 
 We could stick something like these to those flags:
 fb_cmd2.flags:
 #define DRM_MODE_FB_INTERLACED 0x1
 
 set_plane.flags:
 #define DRM_MODE_PRESENT_TOP_FIELD 0x1
 #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2

Oh sorry I lost track of the internal discussion.

Are those attributes of the fb or of each object?  E.g. could you mix
interlaced and non-interlaced buffers in a planar format?

Maybe I need to rename this ioctl to addfb_swissarmyknife :)

-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Rob Clark 
On Mon, Nov 14, 2011 at 3:16 PM, Ville Syrjälä
ville.syrj...@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 ARGB, XRGB, XRGB1555 etc.?

 Also the channel and byte order should be documented clearly.

 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.

I expect that v4l just made up their own fourcc values for some of the
RGB formats..  which isn't a horrible idea, and seems worthwhile to be
aligned with names/values that v4l already picked.  At least then we
don't have two different sets of more or less arbitrary rgb fourcc
names..

BR,
-R

 --
 Ville Syrjälä
 Intel OTC
 ___
 dri-devel mailing list
 dri-devel@lists.freedesktop.org
 http://lists.freedesktop.org/mailman/listinfo/dri-devel

___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Ville Syrjälä 
On Mon, Nov 14, 2011 at 01:35:57PM -0800, Jesse Barnes wrote:
 On Mon, 14 Nov 2011 23:24:55 +0200
 Ville Syrjälä ville.syrj...@linux.intel.com wrote:
 
  On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
   +struct drm_mode_fb_cmd2 {
   + __u32 fb_id;
   + __u32 width, height;
   + __u32 pixel_format; /* fourcc code from videodev2.h */
   +
   + /*
   +  * In case of planar formats, this ioctl allows up to 4
   +  * buffer objects with offets and pitches per plane.
   +  * The pitch and offset order is dictated by the fourcc,
   +  * e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
   +  *
   +  *   YUV 4:2:0 image with a plane of 8 bit Y samples
   +  *   followed by an interleaved U/V plane containing
   +  *   8 bit 2x2 subsampled colour difference samples.
   +  *
   +  * So it would consist of Y as offset[0] and UV as
   +  * offeset[1].  Note that offset[0] will generally
   +  * be 0.
   +  */
   + __u32 handles[4];
   + __u32 pitches[4]; /* pitch for each plane */
   + __u32 offsets[4]; /* offset of each plane */
   +};
  
  Hey, what about those interlaced buffers? We talked privately about
  adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
  drm_mode_set_plane.
  
  We could stick something like these to those flags:
  fb_cmd2.flags:
  #define DRM_MODE_FB_INTERLACED 0x1
  
  set_plane.flags:
  #define DRM_MODE_PRESENT_TOP_FIELD 0x1
  #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2
 
 Oh sorry I lost track of the internal discussion.
 
 Are those attributes of the fb or of each object?  E.g. could you mix
 interlaced and non-interlaced buffers in a planar format?

I suppose it might be possible that you'd want to treat luma as
interlaced and chroma as progressive under some circumstances.
But I can't see why simply defining another flag for that wouldn't
be enough.

 Maybe I need to rename this ioctl to addfb_swissarmyknife :)

Now I'm just waiting for someone to jump in and say that they 
want independent buffers for each interlaced field :)

Me? I'll be happy with just those two flags members... for now at least ;)

-- 
Ville Syrjälä
Intel OTC
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

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

2011-11-14 Thread Jesse Barnes 
On Tue, 15 Nov 2011 00:37:47 +0200
Ville Syrjälä ville.syrj...@linux.intel.com wrote:

 On Mon, Nov 14, 2011 at 01:35:57PM -0800, Jesse Barnes wrote:
  On Mon, 14 Nov 2011 23:24:55 +0200
  Ville Syrjälä ville.syrj...@linux.intel.com wrote:
  
   On Mon, Nov 14, 2011 at 12:21:55PM -0800, Jesse Barnes wrote:
+struct drm_mode_fb_cmd2 {
+   __u32 fb_id;
+   __u32 width, height;
+   __u32 pixel_format; /* fourcc code from videodev2.h */
+
+   /*
+* In case of planar formats, this ioctl allows up to 4
+* buffer objects with offets and pitches per plane.
+* The pitch and offset order is dictated by the fourcc,
+* e.g. NV12 (http://fourcc.org/yuv.php#NV12) is described as:
+*
+*   YUV 4:2:0 image with a plane of 8 bit Y samples
+*   followed by an interleaved U/V plane containing
+*   8 bit 2x2 subsampled colour difference samples.
+*
+* So it would consist of Y as offset[0] and UV as
+* offeset[1].  Note that offset[0] will generally
+* be 0.
+*/
+   __u32 handles[4];
+   __u32 pitches[4]; /* pitch for each plane */
+   __u32 offsets[4]; /* offset of each plane */
+};
   
   Hey, what about those interlaced buffers? We talked privately about
   adding a '__u32 flags' member to both drm_mode_fb_cmd2 and
   drm_mode_set_plane.
   
   We could stick something like these to those flags:
   fb_cmd2.flags:
   #define DRM_MODE_FB_INTERLACED 0x1
   
   set_plane.flags:
   #define DRM_MODE_PRESENT_TOP_FIELD 0x1
   #define DRM_MODE_PRESENT_BOTTOM_FIELD 0x2
  
  Oh sorry I lost track of the internal discussion.
  
  Are those attributes of the fb or of each object?  E.g. could you mix
  interlaced and non-interlaced buffers in a planar format?
 
 I suppose it might be possible that you'd want to treat luma as
 interlaced and chroma as progressive under some circumstances.
 But I can't see why simply defining another flag for that wouldn't
 be enough.
 
  Maybe I need to rename this ioctl to addfb_swissarmyknife :)
 
 Now I'm just waiting for someone to jump in and say that they 
 want independent buffers for each interlaced field :)
 
 Me? I'll be happy with just those two flags members... for now at least ;)

Ok, added, see the latest patchset.

Dave, please pound the gavel on this now and declare it sold before
someone asks me to add braille support. :)

Thanks,
-- 
Jesse Barnes, Intel Open Source Technology Center


signature.asc
Description: PGP signature
___
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel