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

["Hans Verkuil" <hverkuil@xs4all.nl>]

> 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 into
>>> something 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,

It's not so easy as you think to find the right timings: you have to check
many parameters to be certain you have the right one and not some subtle
variation.

> and do a S_DV_PRESET2, just like any other place
> where V4L2 defines an ENUM function.
>
> The enum won't change during application runtime, so, they can be stored
> if the application would need to switch to other formats latter.
>
>> I think
>> this is a very desirable feature, particularly for apps running on
>> embedded systems where the hardware is known. This was one of the design
>> considerations at the time this API was made.
>
> This is a very weak argument. With just one ENUM loop, the application can
> quickly get the right format(s), and associate them with any internal
> namespace.

That actually isn't easy at all.

>> But looking at this I wonder if we shouldn't just make a
>> VIDIOC_G_PRESET_TIMINGS function? You give it the preset ID and you get
>> all the timing information back. No need to deprecate anything. I'm not
>> even sure if with this change we need to modify struct
>> v4l2_dv_enum_preset
>> as I proposed in my RFC, although I think we should.
>
> Won't solve the issue: one new #define is needed for each video timing,
> namespaces will be confusing, no support for VESA GVF/ VESA CVT timings
> (or worse: we'll end by having thousands of formats at the end of the
> day),
> instead of just one ENUM ioctl, an extra ioctl will be required for each
> returned value, etc.

Presets for GTF/CVT are useless (and I have never seen hardware that has
such presets). In practice you have CEA and DMT presets and nothing else.

We may need to add PRESET_PRIVATE (just like we do for controls) should we
get non-CEA/DMT formats as well. There is no point in having specific
preset defines for those IMHO.

I see very little advantage in throwing away an API that works quite well
in practice only to add a new one that isn't much better IMO. Instead we
can easily improve the existing API.

I *want* to be able to specify the most common CEA/DMT standards directly
and unambiguously through presets. It is very easy and nice to use in
practice. Once you go into the realm of GTF/CVT, then the preset API is
too limited and G/S_DV_TIMINGS need to be used, but that requires much
more effort on the part of the application.

I hope others will pitch in as well with their opinions.

Regards,

     Hans