Em 06-07-2011 09:13, Laurent Pinchart escreveu:
> On Wednesday 06 July 2011 14:09:38 Mauro Carvalho Chehab wrote:
>> Em 06-07-2011 09:03, Laurent Pinchart escreveu:
>>> On Wednesday 06 July 2011 13:48:35 Mauro Carvalho Chehab wrote:
>>>> Em 06-07-2011 08:31, Hans Verkuil escreveu:
>>>>>> Em 05-07-2011 10:20, Hans Verkuil escreveu:
>>>>>>>> I failed to see what information is provided by the "presets" name.
>>>>>>>> If this were removed from the ioctl, and fps would be added
>>>>>>>> instead, the API would be clearer. The only adjustment would be to
>>>>>>>> use "index" as the preset selection key. Anyway, it is too late for
>>>>>>>> such change. We need to live with that.
>>>>>>>
>>>>>>> Adding the fps solves nothing. Because that still does not give you
>>>>>>> specific timings. You can have 1920x1080P60 that has quite different
>>>>>>> timings from the CEA-861 standard and that may not be supported by a
>>>>>>> TV.
>>>>>>>
>>>>>>> If you are working with HDMI, then you may want to filter all
>>>>>>> supported presets to those of the CEA standard.
>>>>>>>
>>>>>>> That's one thing that is missing at the moment: that presets
>>>>>>> belonging to a certain standard get their own range. Since we only
>>>>>>> do CEA861 right now it hasn't been an issue, but it will.
>>>>>>
>>>>>> I prepared a long email about that, but then I realized that we're
>>>>>> investing our time intosomething broken, at the light of all DV timing
>>>>>> standards. So, I've dropped it and started from scratch.
>>>>>>
>>>>>> From what I've got, there are some hardware that can only do a limited
>>>>>> set of DV timings. If this were not the case, we could simply just use
>>>>>> the VIDIOC_S_DV_TIMINGS/VIDIOC_G_DV_TIMINGS, and put the CEA 861 and
>>>>>> VESA timings into some userspace library.
>>>>>>
>>>>>> In other words, the PRESET API is meant to solve the case where
>>>>>> hardware only support a limited set of frequencies, that may or may
>>>>>> not be inside the CEA standard.
>>>>>>
>>>>>> Let's assume we never added the current API, and discuss how it would
>>>>>> properly fulfill the user needs. An API that would likely work is:
>>>>>>
>>>>>> struct v4l2_dv_enum_preset2 {
>>>>>>
>>>>>> __u32 index;
>>>>>> __u8 name[32]; /* Name of the preset timing */
>>>>>>
>>>>>> struct v4l2_fract fps;
>>>>>>
>>>>>> #define DV_PRESET_IS_PROGRESSIVE 1<<31
>>>>>> #define DV_PRESET_SPEC(flag) (flag && 0xff)
>>>>>> #define DV_PRESET_IS_CEA861 1
>>>>>> #define DV_PRESET_IS_DMT 2
>>>>>> #define DV_PRESET_IS_CVF 3
>>>>>> #define DV_PRESET_IS_GTF 4
>>>>>> #define DV_PRESET_IS_VENDOR_SPECIFIC 5
>>>>>>
>>>>>> __u32 flags; /* Interlaced/progressive, DV specs, etc */
>>>>>>
>>>>>> __u32 width; /* width in pixels */
>>>>>> __u32 height; /* height in lines */
>>>>>> __u32 polarities; /* Positive or negative polarity */
>>>>>> __u64 pixelclock; /* Pixel clock in HZ. Ex. 74.25MHz->74250000 */
>>>>>> __u32 hfrontporch; /* Horizpontal front porch in pixels */
>>>>>> __u32 hsync; /* Horizontal Sync length in pixels */
>>>>>> __u32 hbackporch; /* Horizontal back porch in pixels */
>>>>>> __u32 vfrontporch; /* Vertical front porch in pixels */
>>>>>> __u32 vsync; /* Vertical Sync length in lines */
>>>>>> __u32 vbackporch; /* Vertical back porch in lines */
>>>>>> __u32 il_vfrontporch; /* Vertical front porch for bottom field of
>>>>>>
>>>>>> * interlaced field formats
>>>>>> */
>>>>>>
>>>>>> __u32 il_vsync; /* Vertical sync length for bottom field of
>>>>>>
>>>>>> * interlaced field formats
>>>>>> */
>>>>>>
>>>>>> __u32 il_vbackporch; /* Vertical back porch for bottom field of
>>>>>>
>>>>>> * interlaced field formats
>>>>>> */
>>>>>>
>>>>>> __u32 reserved[4];
>>>>>>
>>>>>> };
>>>>>>
>>>>>> #define VIDIOC_ENUM_DV_PRESETS2 _IOWR('V', 83, struct
>>>>>> v4l2_dv_enum_preset2)
>>>>>> #define VIDIOC_S_DV_PRESET2 _IOWR('V', 84, u32 index)
>>>>>> #define VIDIOC_G_DV_PRESET2 _IOWR('V', 85, u32 index)
>>>>>>
>>>>>> Such preset API seems to work for all cases. Userspace can use any DV
>>>>>> timing information to select the desired format, and don't need to
>>>>>> have a switch for a preset macro to try to guess what the format
>>>>>> actually means. Also, there's no need to touch at the API spec every
>>>>>> time a new DV timeline is needed.
>>>>>>
>>>>>> Also, it should be noticed that, since the size of the data on the
>>>>>> above definitions are different than the old ones, _IO macros will
>>>>>> provide a different magic number, so, adding these won't break the
>>>>>> existing API.
>>>>>>
>>>>>> So, I think we should work on this proposal, and mark the existing one
>>>>>> as deprecated.
>>>>>
>>>>> This proposal makes it very hard for applications to directly select a
>>>>> format like 720p50 because the indices can change at any time.
>>>>
>>>> Why? All the application needs to do is to call VIDIOC_ENUM_DV_PRESETS2,
>>>> check what line it wants, and do a S_DV_PRESET2, just like any other
>>>> place where V4L2 defines an ENUM function.
>>>
>>> Forcing applications to enumerate all presets when they already know what
>>> preset they want doesn't seem like a very good solution to me.
>>
>> If the app already know, it might simply do VIDIOC_S_DV_PRESET2(index).
>> This would work for an embedded hardware. The only care to be taken is to
>> change the index number if the Kernel changes, or to be sure that, on the
>> embedded tree, that newer DV lines will be added only after the previous
>> one.
>>
>> Anyway, a broken API cannot be justified by a weak argument that not
>> needing to do an ENUM will save a few nanosseconds for some embedded
>> hardware during application initialization time.
>
> We're talking about dozens of syscalls, not a couple of nanoseconds.
hundreds of nanoseconds? Those systemcalls are just reading a table, and,
once the right standard is selected, application can stop the loop. The
current tables have up to 18 DV formats. I doubt that it would affect
application performance on any way.
The old ir-keytable used to do 128.000 system calls to cleanup the IR table
and add a new one, due to an API limitation. It were capable of doing that
on fractions of a second.
Mauro.
--
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
