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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Laurent,

On 11/24/2011 10:50 AM, Laurent Pinchart wrote:
> 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.

> 
> On Sunday 20 November 2011 11:55:22 Laurent Pinchart wrote:
>> On Sunday 20 November 2011 03:00:33 Florian Tobias Schandinat wrote:
>>> Hi Laurent,
>>>
>>> On 08/31/2011 11:18 AM, Laurent Pinchart wrote:
>>>> 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]
>>
>>>> @@ -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?
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. 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?


Best regards,

Florian Tobias Schandinat

> 
>> That's quite hackish though... What's your opinion ?
>>
>> It would also not handle userspace code that initializes an
>> fb_var_screeninfo structure with named initializers, but that shouldn't
>> happen, as application should read fb_var_screeninfo , modify it and write
>> it back.
>>
>>>>  	__u32 nonstd;			/* != 0 Non standard pixel format */
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html


[Index of Archives]     [Linux Input]     [Video for Linux]     [Gstreamer Embedded]     [Mplayer Users]     [Linux USB Devel]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [Yosemite Backpacking]
  Powered by Linux