Re: [RFC] Standardize YUV support in the fbdev API

[Florian Tobias Schandinat <FlorianSchandinat@gmx.de>]

Hi Laurent,

On 05/23/2011 09:00 PM, Laurent Pinchart wrote:
> On Saturday 21 May 2011 00:33:02 Florian Tobias Schandinat wrote:
>> On 05/17/2011 10:07 PM, Laurent Pinchart wrote:
>>> - Other solutions are possible, such as adding new ioctls. Those
>>> solutions are more intrusive, and require larger changes to both
>>> userspace and kernelspace code.
>>
>> I'm against (ab)using the nonstd field (probably the only sane thing we can
>> do with it is declare it non-standard which interpretation is completely
>> dependent on the specific driver) or requiring previously unused fields to
>> have a special value so I'd like to suggest a different method:
>>
>> I remembered an earlier discussion:
>> [ http://marc.info/?l=linux-fbdev&m\x129896686208130&w=2 ]
>>
>> On 03/01/2011 08:07 AM, Geert Uytterhoeven wrote:
>>   >  On Tue, Mar 1, 2011 at 04:13, Damian<dhobsong@igel.co.jp>   wrote:
>>   >>  On 2011/02/24 15:05, Geert Uytterhoeven wrote:
>>   >>>  For YUV (do you mean YCbCr?), I'm inclined to suggest adding a new
>>   >>>  FB_VISUAL_*
>>   >>>  type instead, which indicates the fb_var_screeninfo.{red,green,blue}
>>   >>>  fields are
>>   >>>  YCbCr instead of RGB.
>>   >>>  Depending on the frame buffer organization, you also need new
>>   >>>  FB_TYPE_*/FB_AUX_*
>>   >>>  types.
>>   >>
>>   >>  I just wanted to clarify here.  Is your comment about these new flags
>>   >>  in specific reference to this patch or to Magnus' "going forward"
>>   >>  comment?  It
>>   >
>>   >  About new flags.
>>   >
>>   >>  seems like the beginnings of a method to standardize YCbCr support in
>>   >>  fbdev across all platforms.
>>   >>  Also, do I understand correctly that FB_VISUAL_ would specify the
>>   >>  colorspace
>>   >
>>   >  FB_VISUAL_* specifies how pixel values (which may be tuples) are mapped
>>   >  to colors on the screen, so to me it looks like the sensible way to set
>>   >  up YCbCr.
>>   >
>>   >>  (RGB, YCbCr), FB_TYPE_* would be a format specifier (i.e. planar,
>>   >>  semiplanar, interleaved, etc)?  I'm not really sure what you are
>>   >>  referring to with the FB_AUX_* however.
>>   >
>>   >  Yep, FB_TYPE_* specifies how pixel values/tuples are laid out in frame
>>   >  buffer memory.
>>   >
>>   >  FB_AUX_* is only used if a specific value of FB_TYPE_* needs an
>>   >  additional parameter (e.g. the interleave value for interleaved
>>   >  bitplanes).
>>
>> Adding new standard values for these fb_fix_screeninfo fields would solve
>> the issue for framebuffers which only support a single format.
>
> I've never liked changing fixed screen information :-) It would be consistent
> with the API though.

Fixed does only mean that it can't be directly manipulated by applications. The 
driver has to modify it anyway on about every mode change (line_length). Yes 
perhaps some of these fields would be in var today and certainly others 
shouldn't exist at all but I do not blame anyone for not being capable to look 
into the future.

>> If you have the need to switch
>
> Yes I need that. This requires an API to set the mode through
> fb_var_screeninfo, which is why I skipped modifying fb_fix_screeinfo.
>
> A new FB_TYPE_* could be used to report that we use a 4CC-based mode, with the
> exact mode reported in one of the fb_fix_screeninfo reserved fields (or the
> type_aux field). This would duplicate the information passed through
> fb_var_screeninfo though. Do you think it's worth it ?

I think it's more like a FB_VISUAL_FOURCC as you want to express how the color 
<-> pixel mapping is. The FB_TYPE_* is more about whether pixel are packed or 
represented as planes (the 2 format groups mentioned on fourcc.org).
That's certainly something I'd introduce as it would (hopefully) work to prevent 
old applications which don't know your extension manipulating a FOURCC format 
thinking that it is RGB.
So I think we should
fix.visual = FB_VISUAL_FOURCC;
fix.type = FB_TYPE_PACKED_PIXELS | FB_TYPE_PLANES; /* (?) */
or maybe add a FB_TYPE_FOURCC and rely only on the information in FOURCC (as 
things like UYVY could become confusing as macropixel!=pixel)

>> I guess it would be a good idea to add a new flag to the vmode bitfield in
>> fb_var_screeninfo which looks like a general purpose modifier for the
>> videomode. You could than reuse any RGB-specific field you like to pass more
>> information.
>
> That looks good to me. The grayscale field could be reused to pass the 4CC.

var.grayscale = <FOURCC_FORMAT>;
var.vmode = FB_VMODE_FOURCC;
and if this vmode flag is not set it means traditional mode (based on RGBA).

>> Maybe we should also use this chance to declare one of the fix_screeninfo
>> reserved fields to be used for capability flags or an API version as we can
>> assume that those are 0 (at least in sane drivers).
>
> That's always good, although it's not a hard requirement for the purpose of
> YUV support.

Sure. But it's good to let the application know whether you support the new 
extension or whether you just ignore the flag. So I'm voting for a
fix.caps = FB_CAP_FOURCC;


Best regards,

Florian Tobias Schandinat