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
--
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
