Hi Florian,

On Friday 25 November 2011 23:09:40 Florian Tobias Schandinat wrote:

On 11/24/2011 10:50 AM, Laurent Pinchart wrote:

quoted

Hi Florian,

Gentle ping ?

Sorry, but I'm very busy at the moment and therefore time-consuming things,
like solving challenging problems, are delayed for some time.

No worries.

quoted

On Sunday 20 November 2011 11:55:22 Laurent Pinchart wrote:

quoted

On Sunday 20 November 2011 03:00:33 Florian Tobias Schandinat wrote:

quoted

Hi Laurent,

On 08/31/2011 11:18 AM, Laurent Pinchart wrote:

quoted

This API will be used to support YUV frame buffer formats in a
standard way.

looks like the union is causing problems. With this patch applied I get

errors like this:
  CC [M]  drivers/auxdisplay/cfag12864bfb.o

drivers/auxdisplay/cfag12864bfb.c:57: error: unknown field ‘red’
specified in initializer

*ouch*

gcc < 4.6 chokes on anonymous unions initializers :-/

[snip]

quoted

quoted

@@ -246,12 +251,23 @@ struct fb_var_screeninfo {

 	__u32 yoffset;			/* resolution			*/
 	
 	__u32 bits_per_pixel;		/* guess what			*/

-	__u32 grayscale;		/* != 0 Graylevels instead of colors */

-	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			*/
+	union {
+		struct {		/* Legacy format API		*/
+			__u32 grayscale; /* 0 = color, 1 = grayscale	*/
+			/* 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];
+		} fourcc;
+	};

We can't name the union, otherwise this will change the userspace API.

We could "fix" the problem on the kernel side with

#ifdef __KERNEL__

	} color;

#else

	};

#endif

(and the structure that contains the grayscale, red, green, blue and
transp fields would need to be similarly named, the "rgb" name comes to
mind)

Which, I guess, would require modifying all drivers?

Unfortunately. That can be automated using coccinelle (I wrote a semantic 
patch for that), but it will still be around 10k lines of diff.

I don't consider that a good idea. Maybe the simplest solution would be to
drop the union idea and just accept an utterly misleading name "grayscale"
for setting the FOURCC value.

I'll see if we can add an accessor macro to make it more explicit.

The colorspace could use one of the reserved fields at the end or do you
worry that we need to add a lot of other things?

For FOURCC-based format configuration I don't think we will need much more. If 
we do need lots of additional fields in the future we might have to consider 
an fbdev2 API ;-)

I'll resubmit patches based on this.

-- 
Regards,

Laurent Pinchart