From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from perceval.ideasonboard.com ([95.142.166.194]:58249 "EHLO perceval.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752661Ab1GFMM6 (ORCPT ); Wed, 6 Jul 2011 08:12:58 -0400 From: Laurent Pinchart To: Mauro Carvalho Chehab 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 Cc: Hans Verkuil , Tomasz Stanislawski , linux-media@vger.kernel.org, m.szyprowski@samsung.com, kyungmin.park@samsung.com References: <1309351877-32444-1-git-send-email-t.stanislaws@samsung.com> <201107061403.48526.laurent.pinchart@ideasonboard.com> <4E145082.5080406@redhat.com> In-Reply-To: <4E145082.5080406@redhat.com> MIME-Version: 1.0 Content-Type: Text/Plain; charset="utf-8" Content-Transfer-Encoding: 7bit Message-Id: <201107061413.33300.laurent.pinchart@ideasonboard.com> List-ID: Sender: 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