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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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.

-- 
Regards,

Laurent Pinchart