From: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
To: Mauro Carvalho Chehab <mchehab@redhat.com>
Cc: Hans Verkuil <hverkuil@xs4all.nl>,
Tomasz Stanislawski <t.stanislaws@samsung.com>,
linux-media@vger.kernel.org, m.szyprowski@samsung.com,
kyungmin.park@samsung.com
Subject: Re: [RFC] DV timings spec fixes at V4L2 API - was: [PATCH 1/8] v4l: add macro for 1080p59_54 preset
Date: Wed, 6 Jul 2011 14:13:32 +0200 [thread overview]
Message-ID: <201107061413.33300.laurent.pinchart@ideasonboard.com> (raw)
In-Reply-To: <4E145082.5080406@redhat.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
next prev parent reply other threads:[~2011-07-06 12:12 UTC|newest]
Thread overview: 37+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-06-29 12:51 [PATCH v6 0/8] TV drivers for Samsung S5P platform (media part) Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 1/8] v4l: add macro for 1080p59_54 preset Tomasz Stanislawski
2011-07-04 16:09 ` [RFC] DV timings spec fixes at V4L2 API - was: " Mauro Carvalho Chehab
2011-07-04 22:47 ` Laurent Pinchart
2011-07-04 23:28 ` Mauro Carvalho Chehab
2011-07-05 6:46 ` Hans Verkuil
2011-07-05 7:26 ` Hans Verkuil
2011-07-05 12:08 ` Mauro Carvalho Chehab
2011-07-05 13:20 ` Hans Verkuil
2011-07-05 19:02 ` Andy Walls
2011-07-05 23:25 ` Mauro Carvalho Chehab
2011-07-06 1:05 ` Andy Walls
2011-07-06 11:04 ` Mauro Carvalho Chehab
2011-07-06 11:31 ` Hans Verkuil
2011-07-06 11:48 ` Mauro Carvalho Chehab
2011-07-06 12:03 ` Laurent Pinchart
2011-07-06 12:09 ` Mauro Carvalho Chehab
2011-07-06 12:13 ` Laurent Pinchart [this message]
2011-07-06 12:20 ` Mauro Carvalho Chehab
2011-07-06 12:14 ` Hans Verkuil
2011-07-06 12:31 ` Mauro Carvalho Chehab
2011-07-06 12:56 ` Hans Verkuil
2011-07-06 14:10 ` Mauro Carvalho Chehab
2011-07-06 19:39 ` Mauro Carvalho Chehab
2011-07-07 11:33 ` Hans Verkuil
2011-07-07 13:52 ` Mauro Carvalho Chehab
2011-07-07 14:58 ` Hans Verkuil
2011-07-07 16:18 ` Mauro Carvalho Chehab
2011-07-07 17:52 ` Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 2/8] v4l: add g_tvnorms_output callback to V4L2 subdev Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 3/8] v4l: add g_dv_preset " Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 4/8] v4l: add g_std_output " Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 5/8] v4l: fix v4l_fill_dv_preset_info function Tomasz Stanislawski
2011-07-14 16:02 ` Mauro Carvalho Chehab
2011-06-29 12:51 ` [PATCH 6/8] v4l: s5p-tv: add drivers for HDMI on Samsung S5P platform Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 7/8] v4l: s5p-tv: add SDO driver for " Tomasz Stanislawski
2011-06-29 12:51 ` [PATCH 8/8] v4l: s5p-tv: add TV Mixer " Tomasz Stanislawski
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201107061413.33300.laurent.pinchart@ideasonboard.com \
--to=laurent.pinchart@ideasonboard.com \
--cc=hverkuil@xs4all.nl \
--cc=kyungmin.park@samsung.com \
--cc=linux-media@vger.kernel.org \
--cc=m.szyprowski@samsung.com \
--cc=mchehab@redhat.com \
--cc=t.stanislaws@samsung.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).