Re: [RFC] DV timings spec fixes at V4L2 API - was: [PATCH 1/8] v4l: add macro for 1080p59_54 preset

[Tomasz Stanislawski <t.stanislaws@samsung.com>]

Hi Mauro, Hans,

I am really surprised by the havoc caused by the little 2-line patch.

Let me sum up what I (don't) like in Hans' and Mauro's approaches:

Hans approach:
- extend v4l2_enum_dv_preset with fps and flags fields,
- allow enumerating presets by both index and preset code
- add standard to macro names for presets

Pros:
- backward compatible with existing api
- very simple and effective. Setting desired preset using only 2 lines 
of code
- easy to add unfortunate 1080p59_94
- easy to differentiate 1080p59_94 from 1080p60 using VIDIOC_ENUM_DV_PRESET

Cons:
- number of existing macros must increase extensionally with number of 
features. Height, progressiveness, frequency are already present. 
Standard family is added in Hans' RFC. Some presets involve width. 
Therefore multiple holes are expected making usage of macros very 
unreliable.
- it is not possible to use VIDIOC_S_DV_PRESET to handle case when 
application just wants a preset that has 720 height. The application has 
to enumerate all existing/possible presets  (number of possible 
combinations may be large) to find a preset that suits to the 
application's needs.
- unnecessary redundancy, preset is nothing more than a standardized index

Mauro's approach:
- enumerate all possible presets using VIDIOC_ENUM_DV_PRESETS2
- choose suitable preset using index field from

Pros:
- consistency: preset can only be addressed by index field,
- no preset macros

Cons:
- structure v4l2_dv_enum_preset2 contains BT.656/BT.1120 timing related 
data, the structure should be more general. Most application would not 
use timing fields, so maybe there is no need of adding them to the 
structure.
- applications still has to enumerate through all possible combinations 
to find a suitable preset
- not compatible with existing API, two way to configure DV hardware

I propose following requirements for DV preset api basing on pros and 
cons from overmentioned approaches.
- an application should be able to choose a preset with desired 
parameters using single ioctl call
- preset should be accessed using single key. I prefer to use index as a 
key because it gives more flexibility to a driver.
- compatible with existing api as much as possible

What do you think about approach similar to S_FMT?
Please look at the code below.

struct v4l2_dv_preset2 {
    u16 width;
    u16 height;
    v4l2_fract fps;
    u32 flags; /* progressiveness, standard hints, rounding constraints */
    u32 reserved[];
};

/* Values for the standard field */
#define V4L2_DV_BT_656_1120     0       /* BT.656/1120 timing type */

struct v4l2_enum_dv_preset2 {
    u32 index;
    char name[32];
    struct v4l2_dv_preset2 preset;
    struct v4l2_dv_timings timings; /* to be removed ? */
    u32 reserved[];
};

#define    VIDIOC_ENUM_DV_PRESETS2    _IOWR('V', 83, struct 
v4l2_dv_enum_preset2)
#define    VIDIOC_S_DV_PRESET2    _IOWR('V', 84, struct v4l2_dv_preset2)
#define    VIDIOC_G_DV_PRESET2    _IOWR('V', 85, struct v4l2_dv_preset2)
#define    VIDIOC_TRY_DV_PRESET2    _IOWR('V', 86, struct v4l2_dv_preset2)

To set a mode with height 720 lines the applications would execute code 
below:

struct v4l2_dv_preset2 preset = {    .height = 720 };
ioctl(fd, VIDIOC_S_DV_PRESET2, &preset);

The preset is selected using the most interesting features like 
width/height/frequency and progressiveness.
The driver would find the preset with vertical resolution as close as 
possible to 720.
The width and fps is zero so driver is free to choose suitable/default ones.
The field flags may contain hind about choosing preset that belong to 
specific DV standard family.

I do not feel competent in the field of DV standard. Could give me more 
ideas about flags?
The flags could contain  constraint  bits similar to ones presented in 
SELECTION api.
Maybe structures v4l2_dv_preset and v4l2_enum_dv_presets could be 
utilized for purpose of presented api.
Maybe using some union/structure align magic it would be possible to 
keep binary compatibility with existing programs.

For now, I have removed unfortunate 1080P59_94 format from S5P-TV driver.
I would be very happy if the driver was merged into 3.1.
I think that it would be not possible due to 1080p59_94 issue.
The driver did not lose much of its functionality because it still 
supports 1080p60.
Moreover, adding 1080p59_94 is relatively trivial.

I hope you find this information useful.

Regards,
Tomasz Stanislawski