| Message ID | 1309351877-32444-2-git-send-email-t.stanislaws@samsung.com (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
Hi Mauro, On Monday 04 July 2011 18:09:18 Mauro Carvalho Chehab wrote: [snip] > 1) PRESET STANDARDS > ====== ========= > > There are 3 specs involved with DV presets: ITU-R BT 709 and BT 1120 and > CEA 861. > > At ITU-R BT.709, both 60Hz and 60/1.001 Hz are equally called as "60 Hz". > BT.1120 follows the same logic, as it uses BT.709 as a reference for video > timings. > > The CEA-861-E spec says at item 4, that: [snip] > At the same item, the table 2 describes several video parameters for each > preset, associating the Video Identification Codes (VIC) for each preset. This might be a bit out of scope, but why aren't we using the VICs as DV presets ?
Em 04-07-2011 19:47, Laurent Pinchart escreveu: > Hi Mauro, > > On Monday 04 July 2011 18:09:18 Mauro Carvalho Chehab wrote: > > [snip] > >> 1) PRESET STANDARDS >> ====== ========= >> >> There are 3 specs involved with DV presets: ITU-R BT 709 and BT 1120 and >> CEA 861. >> >> At ITU-R BT.709, both 60Hz and 60/1.001 Hz are equally called as "60 Hz". >> BT.1120 follows the same logic, as it uses BT.709 as a reference for video >> timings. >> >> The CEA-861-E spec says at item 4, that: > > [snip] > >> At the same item, the table 2 describes several video parameters for each >> preset, associating the Video Identification Codes (VIC) for each preset. > > This might be a bit out of scope, but why aren't we using the VICs as DV > presets ? I had the same question after analyzing the specs ;) That's said, abstracting from the spec could be a good idea if we have newer versions of the spec re-defining the VICs. Maybe the right thing to do would be to rename the presets as: V4L2_DV_CEA861_VIC_16 V4L2_DV_CEA861_VIC_35_36 ... (or course, preserving the old names with compatibility macros) Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Tuesday, July 05, 2011 00:47:43 Laurent Pinchart wrote: > Hi Mauro, > > On Monday 04 July 2011 18:09:18 Mauro Carvalho Chehab wrote: > > [snip] > > > 1) PRESET STANDARDS > > ====== ========= > > > > There are 3 specs involved with DV presets: ITU-R BT 709 and BT 1120 and > > CEA 861. > > > > At ITU-R BT.709, both 60Hz and 60/1.001 Hz are equally called as "60 Hz". > > BT.1120 follows the same logic, as it uses BT.709 as a reference for video > > timings. > > > > The CEA-861-E spec says at item 4, that: > > [snip] > > > At the same item, the table 2 describes several video parameters for each > > preset, associating the Video Identification Codes (VIC) for each preset. > > This might be a bit out of scope, but why aren't we using the VICs as DV > presets ? The VIC does more than just set the timings. It also determines the pixel aspect ratio. So exactly the same video timings may have two VICs, the only difference being the pixel aspect which is *not* part of the timings. The VIC is part of the AVI InfoFrame, however. So VIC != timings. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 05-07-2011 04:26, Hans Verkuil escreveu: > On Monday, July 04, 2011 18:09:18 Mauro Carvalho Chehab wrote: >> Em 29-06-2011 09:51, Tomasz Stanislawski escreveu: >>> The 1080p59_94 is supported by latest Samsung SoC. >>> >>> Signed-off-by: Tomasz Stanislawski <t.stanislaws@samsung.com> >>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com> >>> Reviewed-by: Hans Verkuil <hverkuil@xs4all.nl> >>> --- >>> drivers/media/video/v4l2-common.c | 1 + >>> include/linux/videodev2.h | 1 + >>> 2 files changed, 2 insertions(+), 0 deletions(-) >>> >>> diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c >>> index 06b9f9f..003e648 100644 >>> --- a/drivers/media/video/v4l2-common.c >>> +++ b/drivers/media/video/v4l2-common.c >>> @@ -582,6 +582,7 @@ int v4l_fill_dv_preset_info(u32 preset, struct v4l2_dv_enum_preset *info) >>> { 1920, 1080, "1080p@30" }, /* V4L2_DV_1080P30 */ >>> { 1920, 1080, "1080p@50" }, /* V4L2_DV_1080P50 */ >>> { 1920, 1080, "1080p@60" }, /* V4L2_DV_1080P60 */ >>> + { 1920, 1080, "1080p@59.94" }, /* V4L2_DV_1080P59_94 */ >>> }; >>> >>> if (info == NULL || preset >= ARRAY_SIZE(dv_presets)) >>> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h >>> index 8a4c309..7c77c4e 100644 >>> --- a/include/linux/videodev2.h >>> +++ b/include/linux/videodev2.h >>> @@ -872,6 +872,7 @@ struct v4l2_dv_enum_preset { >>> #define V4L2_DV_1080P30 16 /* SMPTE 296M */ >>> #define V4L2_DV_1080P50 17 /* BT.1120 */ >>> #define V4L2_DV_1080P60 18 /* BT.1120 */ >>> +#define V4L2_DV_1080P59_94 19 >>> >>> /* >>> * D V B T T I M I N G S >> >> This patch deserves further discussions, as the specs that define the presets >> are not so clear with respect to 60Hz and 60/1.001 Hz. >> >> Let me summarize the issue. >> >> >> >> 1) PRESET STANDARDS >> ====== ========= >> >> There are 3 specs involved with DV presets: ITU-R BT 709 and BT 1120 and CEA 861. >> >> At ITU-R BT.709, both 60Hz and 60/1.001 Hz are equally called as "60 Hz". BT.1120 >> follows the same logic, as it uses BT.709 as a reference for video timings. >> >> The CEA-861-E spec says at item 4, that: >> >> A video timing with a vertical frequency that is an integer multiple of 6.00 Hz (i.e. 24.00, 30.00, 60.00, >> 120.00 or 240.00 Hz) is considered to be the same as a video timing with the equivalent detailed timing >> information but where the vertical frequency is adjusted by a factor of 1000/1001 (i.e., 24/1.001, 30/1.001, >> 60/1.001, 120/1.001 or 240/1.001). That is, they are considered two versions of the same video timing but >> with slightly different pixel clock frequencies. Therefore, a DTV that declares it is capable of displaying a >> video timing with a vertical frequency that is either an integer multiple of 6 Hz or an integer multiple of 6 >> Hz adjusted by a factor of 1000/1001 shall be capable of displaying both versions of the video timing. >> >> At the same item, the table 2 describes several video parameters for each preset, associating the >> Video Identification Codes (VIC) for each preset. > > No, *multiple VICs* are associated with each preset. VIC != preset. There are two VIC's for several presets, as it is also used at the spec to determine the aspect ratio, but, basically, for one given VIC, there's just one timings preset at the table 2. > Also, the VICs do not differentiate between 60 and 59.94 Hz. Yes, but V4L2 DV timings also don't differentiate. >> Table 4 associates each VIC with the supported formats. For example, VIC 16 means a resolution of >> 1920x1080 at 59.94Hz/60Hz. The spec does explicitly allow that all vertical frequencies that are >> multiple of 6 Hz to accept both 59.94 Hz and 60 Hz, as said at note 3 of table 2: >> >> 3. A video timing with a vertical frequency that is an integer multiple of 6.00 Hz (i.e. 24.00, 30.00, 60.00, 120.00 or >> 240.00 Hz) is considered to be the same as a video timing with the equivalent detailed timing information but where >> the vertical frequency is adjusted by a factor of 1000/1001 (i.e., 24/1.001, 30/1.001, 60/1.001, 120/1.001 or >> 240/1.001). That is, they are considered two versions of the same video timing but with slightly different pixel clock >> frequencies. The vertical frequencies of the 240p, 480p, and 480i video formats are typically adjusted by a factor of >> exactly 1000/1001 for NTSC video compatibility, while the 576p, 576i, and the HDTV video formats are not. The >> VESA DMT standard [65] specifies a ± 0.5% pixel clock frequency tolerance. Therefore, the nominally 25.175 MHz >> pixel clock frequency value given for video identification code 1 may be adjusted to 25.2 MHz to obtain an exact 60 >> Hz vertical frequency. >> >> In other words, the preset for 1920x1080p@60Hz can be used for both 60Hz and 59.94 Hz, >> according with the above note, being 59.94 Hz the typical value (e. g. the value that >> should be used on most places). >> >> However, there are some "60 Hz" vertical resolutions that have VIC's with >> different framerates (like 59.94Hz, 60.054Hz, etc). Those seem to not be >> covered by the "multiple of 6.00 Hz" rule. > > No. A preset identifies one specific modeline (to use the terminology from the > GPU world). It defines the front/back porches, sync lengths, active area and > pixelclock frequency. Any ambiguities as to the timings of a preset should be > resolved by the documentation (which clearly needs a bit more work). As I pointed before, it is not just documentation. For all the 6.00 Hz multiple standard, CEA defines that, using the same DV timings, it is possible to choose to shift the clock by 1000/1001 to support the "59.94Hz". So, this applies to several VIC's for 60Hz, 120 Hz and 240Hz. While it is possible to duplicate the DV timing line for all affected VICs, We'll end by having a very messy API, as there will be two 60 Hz standards, one with the shift and the other without, for the same CEA VIC, plus the CEA standards that are defined for other vertical resolutions like 60.054 Hz, 59.826 Hz, 119.88 Hz, 239.76 Hz and even 59.940 Hz. Part of the problem is the u32 "preset" var, at v4l2_dv_enum_preset. It requires a namespace to identify the presets, but this can become very confusing with time. How would you distinguish from a CEA-861 60.00 Hz standard with a Vertical frequency shift of 1000/1001 and a CEA-861 59.94 Hz? The current namespace doesn't allow that, and creating a namespace to accommodate would be weird and would easily be very confusing. > In general, > though, all the currently defined presets refer to CEA-861. The standards > mentioned in videodev2.h all refer to the same things, but CEA-861 is the standard > where it all comes together. > > In practice there are four different standards that the preset API can use: > > CEA-861 for all things HDTV (HDMI) > VESA DMT timings for DVI-D (PC) type timings (VGA, XVGA, etc.) > VESA GTF timings (officially deprecated, but still quite common algorithm to > calculate timings) > VESA CVT timings (the newer algorithm to calculate timings) > > GTF and CVT pose there own problems and Cisco will be making a proposal for > how to handle this some time this year. > > VESA DMT timings as easy to add since they are well-defined. We plan on doing > that. Well, try to accomodate all those timings using the current namespace for preset. I bet you'll end by having lots of duplicated names for distinct timings. The API needs to be fixed, and it is better sooner than later. >> 2. V4L2 API >> ==== === >> >> The V4L2 specs define a DV timing as having those fields: >> >> __u32 width Width of the active video in pixels >> __u32 height Height of the active video in lines >> __u32 interlaced Progressive (0) or interlaced (1) >> __u32 polarities This is a bit mask that defines polarities of sync signals. >> __u64 pixelclock Pixel clock in Hz. Ex. 74.25MHz->74250000 >> __u32 hfrontporch Horizontal front porch in pixels >> __u32 hsync Horizontal sync length in pixels >> __u32 hbackporch Horizontal back porch in pixels >> __u32 vfrontporch Vertical front porch in lines >> __u32 vsync Vertical sync length in lines >> __u32 vbackporch Vertical back porch in lines >> __u32 il_vfrontporch Vertical front porch in lines for bottom field of interlaced field formats >> __u32 il_vsync Vertical sync length in lines for bottom field of interlaced field formats >> __u32 il_vbackporch Vertical back porch in lines for bottom field of interlaced field formats >> >> [1] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-g-dv-timings.html >> >> So, it basically allows adjusting the timings for each of the VIC's, but it seems that there >> is one limitation at the current API: >> >> vblank is an integer value, for both frames 0 and 1. So, it doesn't allow to adjust vblanks >> like 22.5. This prevents specifying presets like VICs 10/11. > > vblanks are never halflines. One field is one line longer than the other. As note 1 in table 2 > says: "fractional values indicate that the number of blanking lines varies". It's why we have > those il_ fields. OK. >> >> The presets ioctl's [2] provide the following fields: >> >> __u32 index Number of the DV preset, set by the application. >> __u32 preset This field identifies one of the DV preset values listed in Table A.15, “struct DV Presets”. >> __u8 name[24] Name of the preset, a NUL-terminated ASCII string, for example: "720P-60", "1080I-60". This information is intended for the user. >> __u32 width Width of the active video in pixels for the DV preset. >> __u32 height Height of the active video in lines for the DV preset. >> >> [2] http://linuxtv.org/downloads/v4l-dvb-apis/vidioc-enum-dv-presets.html#v4l2-dv-presets-vals >> >> Where "preset" can mean: >> >> V4L2_DV_INVALID 0 Invalid preset value. >> V4L2_DV_480P59_94 1 720x480 progressive video at 59.94 fps as per BT.1362. >> V4L2_DV_576P50 2 720x576 progressive video at 50 fps as per BT.1362. >> V4L2_DV_720P24 3 1280x720 progressive video at 24 fps as per SMPTE 296M. >> V4L2_DV_720P25 4 1280x720 progressive video at 25 fps as per SMPTE 296M. >> V4L2_DV_720P30 5 1280x720 progressive video at 30 fps as per SMPTE 296M. >> V4L2_DV_720P50 6 1280x720 progressive video at 50 fps as per SMPTE 296M. >> V4L2_DV_720P59_94 7 1280x720 progressive video at 59.94 fps as per SMPTE 274M. >> V4L2_DV_720P60 8 1280x720 progressive video at 60 fps as per SMPTE 274M/296M. >> V4L2_DV_1080I29_97 9 1920x1080 interlaced video at 29.97 fps as per BT.1120/SMPTE 274M. >> V4L2_DV_1080I30 10 1920x1080 interlaced video at 30 fps as per BT.1120/SMPTE 274M. >> V4L2_DV_1080I25 11 1920x1080 interlaced video at 25 fps as per BT.1120. >> V4L2_DV_1080I50 12 1920x1080 interlaced video at 50 fps as per SMPTE 296M. >> V4L2_DV_1080I60 13 1920x1080 interlaced video at 60 fps as per SMPTE 296M. >> V4L2_DV_1080P24 14 1920x1080 progressive video at 24 fps as per SMPTE 296M. >> V4L2_DV_1080P25 15 1920x1080 progressive video at 25 fps as per SMPTE 296M. >> V4L2_DV_1080P30 16 1920x1080 progressive video at 30 fps as per SMPTE 296M. >> V4L2_DV_1080P50 17 1920x1080 progressive video at 50 fps as per BT.1120. >> V4L2_DV_1080P60 18 1920x1080 progressive video at 60 fps as per BT.1120. > > All these standards need to be replaced with CEA-861. Sorry, but I couldn't understand your proposal here. >> >> >> 3. ISSUES AT V4L2 API >> ====== == ==== === >> >> There are some troubles at the way we currently define the presets: >> >> 3.1) The preset macros have the name of the active video lines, but this is also present at >> the height field; > > ??? While a human can see that V4L2_DV_1080P60 refers to 1080 lines, a computer > can't. The macro is a just a number, so you need to communicate the width and > height explicitly. Yes, but the namespace is confusing, and developers might code it as: if (preset >= V4L2_DV_1080I29_97 || preset <= V4L2_DV_1080P60) width = 1080; (or doing the same inside a switch, although I've seen several userspace applications implementing things like above). The above would be valid, and the end result is that a duplicated information is provided. 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. > >> 3.2) The preset macros don't have the name of the active video columns; > > For the current set of macros the industry convention is used. 1080P50 always > refers to 1920x1080. Anything else should be made explicit. > >> 3.3) If someone would want to add a preset for some CEA-861-E VICs, namespace conflicts will >> happen. For example, a preset for 1440x576@50Hz would have the same name as a preset >> for 2880x576p at 50 Hz. Both would be called as V4L2_DV_576P50. > > Obviously that would have to be called V4L2_DV_2880X576P50 (or something similar). So, the namespace will become messy, with will make developers very confused about what they should use. As you've mentioned, there are currently 4 standards with timings that will needed to be supported. So, at the end of the day, we'll have: V4L2_DV_576P50 V4L2_DV_576P50_foo V4L2_DV_576P50_bar V4L2_DV_576P50_delta V4L2_DV_2880X576P50 V4L2_DV_2880X576P50_foo V4L2_DV_2880X576P50_bar V4L2_DV_2880X576P50_delta ... where foo, bar, and delta would be some othe DV-timings based naming. It will become a nightmare for developers to discover, from the above list, what are the timings from VESA DMT and what are from CEA-861. The solution is to associate its name to the standards naming, like: V4L2_DV_CEA_861_VIC_35_36 For the timings associated with VICs 35 and 36. > >> 3.4) It doesn't mind what DV timing is used, CEA-861-E and BT.709 allows to use the 60Hz >> timings as either 60Hz or 59.94 Hz. That applies to all VIC format timings at table 2 >> for 60 Hz, 120 Hz and 240 Hz. > > No, the pixelclock is part of the preset timings. So V4L2_DV_1080P60 and V4L2_DV_1080P59_94 > are different presets with different timings. Just as they would be different modelines > for your graphics card. I really don't see the problem here. Sorry, but the current way that this documented is not clear enough. Also, there's nothing at VIDIOC_ENUM_DV_PRESETS that allows checking the fps, except for the preset name, which is confusing enough. >> 3.5) There are lots of format at CEA-861-E without a V4L2 preset. > > True. Frankly I have yet to encounter any of the weird ones (i.e. other than > 640x480, 480p/i, 576p/i, 720p/i and 1080p/i with their various framerates). > >> >> 4. PROPOSED SOLUTION >> ======== ======== >> >> In order to fix the issue, we need change the API without breaking the current apps that >> use the timings ioctls. Also, the vertical rate clock for 60Hz formats needs to allow >> a fractional adjustment of either 1 or 1000/1001, in order to support the specs. >> >> 4.1) Preset renaming >> --------------- >> >> To avoid having duplicated namespace conflicts, the better seems to rename the existing >> presets to contain both width and height at their macro definitions, like: >> >> #define V4L2_DV_1920_1080P60 18 1920x1080 progressive video at 60 fps as per BT.1120 >> >> (for the sake of simplicity, I just took one value from the table. The same fix is needed >> to be applied for the other macro definitions) >> >> To avoid breaking userspace, the old names need to be associated with the new ones, with: >> >> #define V4L2_DV_1080P60 V4L2_DV_1920_1080P60 > > I've no problem with that. 1080P60 is still a useful shorthand. As I said before, it will be better to change it to explicitly associate with the specs that defined that video timing, with whatever ID code there. So, for CEA-861, it would be, in this case: #define V4L2_DV_CEA_861_VIC_16 18 and the legacy compat macro: #define V4L2_DV_1080P60 V4L2_DV_CEA_861_VIC_16 >> >> This fixes issue 3.2 and 3.3. Unfortunately, fixing 3.1 is not possible anymore, so, >> we have to keep the same information duplicated on two places (at the macro name and >> at the width/height). > > As I mentioned above, for a program the macro name is useless. > >> The question that remains unsolved is what an userspace application would handle a driver >> that might eventually provide inconsistent data at width/height and at the macro names? > > That's a driver bug. > >> 4.2) Framerate selection for 60Hz preset >> ----------------------------------- >> >> As the spec allows using any format that it is multiple of 6.00 Hz multiplied by either >> 1 or 1000/1001, the selection betweem them should be done via VIDIOC_G_PARM/VIDIOC_S_PARM. >> So, V4L2 spec should say, at the "Digital Video (DV) Timings" section: >> >> Devices that implement DV timings shall implement VIDIOC_G_PARM/VIDIOC_S_PARM, >> in order to allow controlling the vertical frame rate for the presets whose >> vertical rate is multiple of 6.00 Hz, in order to allow setting the timing >> between 60 Hz and 59.94 Hz. The default value, at device init, shall be 59.94 Hz. > > NACK. > > The pixel clock is part of the video timings and hence defined by the preset. > > VIDIOC_S_PARM is used when the desired framerate differs from the actual framerate > and the driver can do frame repeating or frameskipping. NO. VIDIOC_S_PARM is used on all places where the user desires to specify a framerate. All webcams use it to select the desired framerate. > At least that's what the V4L2 spec says. I'll prepare a patch fixing it. This is one of the parts of the API that got outdated with time. > In practice it is used to set the framerate for sensors. I'm not sure > if there is any driver that actually uses it for frameskipping. There are some video capture drivers that implement it, so I think that some use it for frameskipping. > To my knowledge it is not used at all for video output. In any case, selecting a DV timing preset with an specific fps is currently confusing, and requires that the userspace application to have a switch(v4l2_dv_enum_preset.preset) in order to associate a preset macro name with a fps. This is a very bad idea. I see only two practical solutions for that. The first one is to use S_PARAM. The other one is to stop providing the frequency inside the macro name, and add a field for that at the v4l2_dv_enum_preset struct: struct v4l2_dv_enum_preset { __u32 index; __u32 preset; __u8 name[32]; /* Name of the preset timing */ __u32 width; __u32 height; v4l2_fract fps; __u32 reserved[2]; }; So, for example, a driver that supports only CEA-861 VIC 16 will report 2 presets: struct v4l2_dv_enum_preset dv[] = { { 0, V4L2_DV_CEA_861_VIC_16__60HZ, "CEA-861 1920x1080p 60Hz", 1920, 1080, {60, 1} }, { 1, V4L2_DV_CEA_861_VIC_16__59_94HZ, "CEA-861 1920x1080p 59.94Hz", 1920, 1080, {60000, 1001} }, }; Yet, we should be sure that this will also cover all current VESA standards. >> 4.3) Add the missing CEA-861-E presets >> --------------------------------- >> >> As those formats are part of the spec that is implemented by this V4L2 API, the better >> would be to implement all the missing formats at the V4L2 spec. As a generic rule, we >> don't add support at the Kernel without having a driver using it, but, in this specific >> case, we want to be able to be compatible with the specs, so, it seems a good idea to >> implement the remaining ones, or, at least reserve its namespace at the DocBook. This >> solves issue 3.5. > > I'm not so sure. There are lots of weird resolutions in CEA-861 that I have never > seen in use. A better approach IMHO is that whenever we add a resolution to the > preset table, then we do that for all variants of that resolution. This works for me. >> 5) S5P-TV SUPPORT FOR 59.94 HZ >> =========================== >> >> It is not clear, from this patch, if you're really wanting to implement support for VIC >> 16 format @59.94 Hz, or something else. From CEA-861-E, it seems to be the case, as >> this is the only 1920x1080p format for 60 Hz. If this is the case, according with my >> proposal, the driver should be using the 60Hz format, instead, and implement S_PARM >> to allow selecting between 60Hz and 59.94Hz. > > NACK. You need two presets: V4L2_DV_1080P60 and 1080P59_94. Just as is already there > for 720P60 and 720P59_94. I really don't see the problem. NACK adding it as "V4L2_DV_1080P59_94". Let's fix the namespace and provide a way to specify/enumberate the fps first, and then change the patch to follow whatever decided. Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hans Verkuil <hverkuil@xs4all.nl> wrote: >I can work on the proposal this week for that. The only reason the fps >hasn't been added >yet is that I never had the time to do the research on how to represent >the fps reliably >for all CEA/VESA formats. Hmm, pixelclock / total_framesize should >always work, of course. > >We can add a flags field as well (for interlaced vs progressive and >perhaps others such as >normal vs reduced blanking). > >That leaves the problem with GTF/CVT. I'll get back to that tomorrow. I >have ideas, but >I need to discuss it first. > >Regards, > > Hans >-- >To unsubscribe from this list: send the line "unsubscribe linux-media" >in >the body of a message to majordomo@vger.kernel.org >More majordomo info at http://vger.kernel.org/majordomo-info.html For fps you could use horizontal_line_freq/lines_per_frame. However, all of the non-integer fps numbers I have seen in this email chain all seem to be multiples of 29.97002997 Hz. So maybe you could just use the closest integer rate with a flag labeled "ntsc_bw_timing_hack" to indicate the fractional rates. :) That 29.97 Hz number comes from the NTSC decision in 1953(!) to change the horizontal line freq to 4.5 MHz/286. Note that (4.5 MHz/286)/525 = 30 * (1000/1001) = 29.97002997 Hz It is interesting to see one of the most ingenious analog hacks in TV history (to achieve color and B&W backward compatabilty while staying in the 10% tolerance of the old B&W receivers) being codified in digital standards over 50 years later. It boggles the mind... Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 05-07-2011 16:02, Andy Walls escreveu: > Hans Verkuil <hverkuil@xs4all.nl> wrote: >> I can work on the proposal this week for that. The only reason the fps >> hasn't been added >> yet is that I never had the time to do the research on how to represent >> the fps reliably >> for all CEA/VESA formats. Hmm, pixelclock / total_framesize should >> always work, of course. >> >> We can add a flags field as well (for interlaced vs progressive and >> perhaps others such as >> normal vs reduced blanking). >> >> That leaves the problem with GTF/CVT. I'll get back to that tomorrow. I >> have ideas, but >> I need to discuss it first. >> >> Regards, >> >> Hans >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" >> in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html > > For fps you could use horizontal_line_freq/lines_per_frame. > > However, all of the non-integer fps numbers I have seen in this email chain all seem to be multiples of 29.97002997 Hz. So maybe you could just use the closest integer rate with a flag labeled "ntsc_bw_timing_hack" to indicate the fractional rates. :) CEA-861 has some other timings that are not 60 Hz * 1000/1001. Yet, v4l2_fract is capable of handling any of such timings, as, whatever frequency taken, it needs to be a fractional number. Btw, even some userspace libraries prefer to represent fps using a fraction, instead of a float, to avoid rounding issues. > > That 29.97 Hz number comes from the NTSC decision in 1953(!) to change the horizontal line freq to 4.5 MHz/286. Note that > > (4.5 MHz/286)/525 = 30 * (1000/1001) = 29.97002997 Hz One of the rationale for that decision was to avoid flicking issues with cathodic ray monitors and fluorescent lamps. > It is interesting to see one of the most ingenious analog hacks in TV history (to achieve color and B&W backward compatabilty while staying in the 10% tolerance of the old B&W receivers) being codified in digital standards over 50 years later. It boggles the mind... Yes. Bad (and good) API decisions will stay forever. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Mauro Carvalho Chehab <mchehab@redhat.com> wrote: >Em 05-07-2011 16:02, Andy Walls escreveu: >> Hans Verkuil <hverkuil@xs4all.nl> wrote: >>> I can work on the proposal this week for that. The only reason the >fps >>> hasn't been added >>> yet is that I never had the time to do the research on how to >represent >>> the fps reliably >>> for all CEA/VESA formats. Hmm, pixelclock / total_framesize should >>> always work, of course. >>> >>> We can add a flags field as well (for interlaced vs progressive and >>> perhaps others such as >>> normal vs reduced blanking). >>> >>> That leaves the problem with GTF/CVT. I'll get back to that >tomorrow. I >>> have ideas, but >>> I need to discuss it first. >>> >>> Regards, >>> >>> Hans >>> -- >>> To unsubscribe from this list: send the line "unsubscribe >linux-media" >>> in >>> the body of a message to majordomo@vger.kernel.org >>> More majordomo info at http://vger.kernel.org/majordomo-info.html >> >> For fps you could use horizontal_line_freq/lines_per_frame. >> >> However, all of the non-integer fps numbers I have seen in this email >chain all seem to be multiples of 29.97002997 Hz. So maybe you could >just use the closest integer rate with a flag labeled >"ntsc_bw_timing_hack" to indicate the fractional rates. :) > >CEA-861 has some other timings that are not 60 Hz * 1000/1001. Yet, >v4l2_fract >is capable of handling any of such timings, as, whatever frequency >taken, it >needs to be a fractional number. Btw, even some userspace libraries >prefer to >represent fps using a fraction, instead of a float, to avoid rounding >issues. > >> >> That 29.97 Hz number comes from the NTSC decision in 1953(!) to >change the horizontal line freq to 4.5 MHz/286. Note that >> >> (4.5 MHz/286)/525 = 30 * (1000/1001) = 29.97002997 Hz > >One of the rationale for that decision was to avoid flicking issues >with cathodic >ray monitors and fluorescent lamps. > >> It is interesting to see one of the most ingenious analog hacks in TV >history (to achieve color and B&W backward compatabilty while staying >in the 10% tolerance of the old B&W receivers) being codified in >digital standards over 50 years later. It boggles the mind... > >Yes. Bad (and good) API decisions will stay forever. > >Cheers, >Mauro. Oops, yes I see the 60.054 Hz rate corresponds to a 262 line field (524 line frame) at the standard NTSC line rate. Weird stuff. Regards, Andy -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. Thanks, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> 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. 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. 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. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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, 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. > 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. Cheers, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. > > 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. > > > 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.
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. Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
> 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-07-2011 09:13, Laurent Pinchart escreveu: > 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. hundreds of nanoseconds? Those systemcalls are just reading a table, and, once the right standard is selected, application can stop the loop. The current tables have up to 18 DV formats. I doubt that it would affect application performance on any way. The old ir-keytable used to do 128.000 system calls to cleanup the IR table and add a new one, due to an API limitation. It were capable of doing that on fractions of a second. Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-07-2011 09:14, Hans Verkuil escreveu: >> 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. Or you can do a strcmp(v4l2_dv_enum_preset2.name,"my preset"). >> 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. A PRESET_PRIVATE would mean just one DV timing, as the "preset" is the index to get data. So, this won't fix the API. > > 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. With my proposal, you can do it. > 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. The usage of G/S_DV_TIMINGS require an extra care: in the past, old VGA hardware (and/or monitors) could be damaged if it is programed with some parameters. It used to have some viruses that damaged hardware using it. So, any driver implementing it will need to validate the timings, to be sure that they are on an acceptable range and won't damage the hardware in any way, or it will need to require some special capability (root access) to avoid that an userspace program to damage the hardware. The timings on a preset table should be ok, so it is safer to implement the *PRESET ioctls than the *TIMINGS one. > I hope others will pitch in as well with their opinions. Agreed. Thanks, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
> Em 06-07-2011 09:14, Hans Verkuil escreveu: >>> 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. > > Or you can do a strcmp(v4l2_dv_enum_preset2.name,"my preset"). Yuck. Then you also need to define the names so you know what name to match on. Since once you allow this you can never modify the names again. > >>> 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. > > A PRESET_PRIVATE would mean just one DV timing, as the "preset" is the > index > to get data. So, this won't fix the API. PRESET_PRIVATE, PRESET_PRIVATE+1, +2, etc. Just as is done in other places. > >> >> 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. > > With my proposal, you can do it. > >> 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. > > The usage of G/S_DV_TIMINGS require an extra care: in the past, old VGA > hardware > (and/or monitors) could be damaged if it is programed with some > parameters. > It used to have some viruses that damaged hardware using it. So, any > driver > implementing it will need to validate the timings, to be sure that they > are > on an acceptable range and won't damage the hardware in any way, or it > will > need to require some special capability (root access) to avoid that an > userspace > program to damage the hardware. Drivers need to validate if necessary, of course. > The timings on a preset table should be ok, so it is safer to implement > the *PRESET > ioctls than the *TIMINGS one. You will need the TIMINGS ioctls if you want to handle GTF/CVT formats. Those are calculated so the preset API is a poor fit. Regards, Hans >> I hope others will pitch in as well with their opinions. > > Agreed. > > Thanks, > Mauro. > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-07-2011 09:56, Hans Verkuil escreveu: >> Em 06-07-2011 09:14, Hans Verkuil escreveu: >>>> 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. >> >> Or you can do a strcmp(v4l2_dv_enum_preset2.name,"my preset"). > > Yuck. Then you also need to define the names so you know what name to > match on. Since once you allow this you can never modify the names again. Once you add names to the API, the above is allowed, as the name is part of the API. So, names can't be changed, as changing them can break things. The alternative is to remove the names from the new API. >>>> 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. >> >> A PRESET_PRIVATE would mean just one DV timing, as the "preset" is the >> index >> to get data. So, this won't fix the API. > > PRESET_PRIVATE, PRESET_PRIVATE+1, +2, etc. Just as is done in other places. Yuck. This is very ugly. NACK. >>> 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. >> >> With my proposal, you can do it. >> >>> 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. >> >> The usage of G/S_DV_TIMINGS require an extra care: in the past, old VGA >> hardware >> (and/or monitors) could be damaged if it is programed with some >> parameters. >> It used to have some viruses that damaged hardware using it. So, any >> driver >> implementing it will need to validate the timings, to be sure that they >> are >> on an acceptable range and won't damage the hardware in any way, or it >> will >> need to require some special capability (root access) to avoid that an >> userspace >> program to damage the hardware. > > Drivers need to validate if necessary, of course. > >> The timings on a preset table should be ok, so it is safer to implement >> the *PRESET >> ioctls than the *TIMINGS one. > > You will need the TIMINGS ioctls if you want to handle GTF/CVT formats. > Those are calculated so the preset API is a poor fit. If you want full support for it, yes. But, if all it is needed are the common resolutions/fps (1024x768p60, 1920x1080p50, ...), GTF/CVT format presets may fit well. Regards, Mauro. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 06-07-2011 09:14, Hans Verkuil escreveu: >> 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. For the trivial case where the application just wants one of the CEA861 standard (or VESA DMT), the check is trivial. The speed of the test can even be improved if the order at the struct would be changed to be: struct v4l2_dv_enum_preset2 { __u32 index; __u32 flags; struct v4l2_fract fps; __u32 width; /* width in pixels */ __u32 height; /* height in lines */ ... } The dv preset seek routine at the application can then be coded as: struct seek_preset { /* Need to follow the same order/arguments as v4l2_dv_enum_preset2 */ struct v4l2_fract fps; __u32 width; __u32 height; }; struct myapp_preset { __u32 flags; struct seek_preset preset; }; struct myapp_preset cea861_vic16 = { .flags = DV_PRESET_IS_PROGRESSIVE | DV_PRESET_IS_CEA861, .width = 1920, .height = 1080, }; int return_dv_preset_index(fp, struct myapp_preset *needed) { int found = -1; struct v4l2_dv_enum_preset2 preset; do { rc = ioctl(fp, VIDIOC_ENUM_DV_PRESETS, &preset); if (rc == -1) break; if ((preset.flags & needed->flags) != needed->flags) continue; if (!memcmp(&preset.fps, &needed->preset)) { found = preset->index; break; } } while (!rc && found < 0); } void main(void) { ... index = return_dv_preset_index(fp, cea861_vic16); ... } Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Wednesday, July 06, 2011 21:39:46 Mauro Carvalho Chehab wrote: > Em 06-07-2011 09:14, Hans Verkuil escreveu: > >> 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. > > For the trivial case where the application just wants one of the CEA861 standard > (or VESA DMT), the check is trivial. > > > The speed of the test can even be improved if the order at the struct would > be changed to be: > > struct v4l2_dv_enum_preset2 { > __u32 index; > __u32 flags; > > struct v4l2_fract fps; > __u32 width; /* width in pixels */ > __u32 height; /* height in lines */ > > ... > } > > The dv preset seek routine at the application can then be coded as: > > struct seek_preset { /* Need to follow the same order/arguments as v4l2_dv_enum_preset2 */ > struct v4l2_fract fps; > __u32 width; > __u32 height; > }; > > struct myapp_preset { > __u32 flags; > > struct seek_preset preset; > }; > > struct myapp_preset cea861_vic16 = { > .flags = DV_PRESET_IS_PROGRESSIVE | DV_PRESET_IS_CEA861, > .width = 1920, > .height = 1080, > }; > > int return_dv_preset_index(fp, struct myapp_preset *needed) > { > int found = -1; > struct v4l2_dv_enum_preset2 preset; > do { > rc = ioctl(fp, VIDIOC_ENUM_DV_PRESETS, &preset); > if (rc == -1) > break; > if ((preset.flags & needed->flags) != needed->flags) > continue; > if (!memcmp(&preset.fps, &needed->preset)) { > found = preset->index; > break; > } > } while (!rc && found < 0); > } > > void main(void) { > ... > index = return_dv_preset_index(fp, cea861_vic16); > ... > } And the current equivalent is: struct v4l_dv_preset preset = { V4L2_DV_1080P60 }; ioctl(f, VIDIOC_S_DV_PRESET, &preset); You want a whole new API that in my view makes things only more complicated and misses existing functionality (such as the one above). Whereas with a few tweaks and possibly a new VIDIOC_G_PRESET_TIMINGS ioctl you can offer the same functionality with the existing API. So, once again my proposal: ENUM_DV_PRESETS is extended with a flags field and a fps v4l2_fract (or frame_period, whatever works best). Flags give you progressive vs interlaced, and I've no problem adding things like IS_CEA861 or similar flags to that. The current set of presets remain in use (but get renamed with the old ones as aliases) for CEA861 and (in the near future) VESA DMT timings. Note that all the hardware I know that has predefined timings does so only for those two standards. Makes sense too: only the consumer electronic standards for SDTV/HDTV and the VGA-like PC monitor standards are typical standards. For presets not related to those standards the easiest method I see is just to assign a preset like (0x80000000 | index). We may need to add a VIDIOC_G_PRESET_TIMINGS, but I am not certain we really need that. ENUM_DV_PRESETS may give sufficient information already. Based on my experience with GTF/CVT formats I strongly suspect that drivers will need to implement a VIDIOC_QUERY_DV_TIMINGS ioctl and let a userspace library detect the GTF/CVT standard. This is surprisingly complex (mostly due to extremely shoddy standards). For GTF/CVT output you want to use VIDIOC_S_DV_TIMINGS anyway. The reason there is no GTF/CVT support yet is simply because I don't want to make proposals unless I actually implemented it and worked with it for some time to see what works best. Everything you can do with your proposal you can do with mine, and mine doesn't deprecate anything. BTW, in the case of HD-SDI transmitters/receivers the CEA-861 standard does not apply, strictly speaking. That standard is covered by SMPTE 292M. It does support most of the usual SDTV/HDTV formats as are defined in CEA-861, except that things like front/back porch etc. do not apply to this serial interface. The idea behind the presets is that it defines industry standard resolution/framerate combinations, but the standards behind those differ per interface type. You don't really care about those in an application. The user (or developer) just wants to select 1080P60 or WUXGA. I am frankly not certain anymore if we want to have the standard as part of the macro name. Something like V4L2_DV_HDTV_1920X1080P60 might be more appropriate, with comments next to it referring to the relevant standards depending on the physical interface type. And instead of using flags to denote the used standard, it might be better to reserve a u16 for the standard. History has shown that video formats stay around for a looong time, but the standards describing them evolve continuously. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em 07-07-2011 08:33, Hans Verkuil escreveu: > On Wednesday, July 06, 2011 21:39:46 Mauro Carvalho Chehab wrote: >> Em 06-07-2011 09:14, Hans Verkuil escreveu: >>>> 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. >> >> For the trivial case where the application just wants one of the CEA861 standard >> (or VESA DMT), the check is trivial. >> >> >> The speed of the test can even be improved if the order at the struct would >> be changed to be: >> >> struct v4l2_dv_enum_preset2 { >> __u32 index; >> __u32 flags; >> >> struct v4l2_fract fps; >> __u32 width; /* width in pixels */ >> __u32 height; /* height in lines */ >> >> ... >> } >> >> The dv preset seek routine at the application can then be coded as: >> >> struct seek_preset { /* Need to follow the same order/arguments as v4l2_dv_enum_preset2 */ >> struct v4l2_fract fps; >> __u32 width; >> __u32 height; >> }; >> >> struct myapp_preset { >> __u32 flags; >> >> struct seek_preset preset; >> }; >> >> struct myapp_preset cea861_vic16 = { >> .flags = DV_PRESET_IS_PROGRESSIVE | DV_PRESET_IS_CEA861, >> .width = 1920, >> .height = 1080, >> }; >> >> int return_dv_preset_index(fp, struct myapp_preset *needed) >> { >> int found = -1; >> struct v4l2_dv_enum_preset2 preset; >> do { >> rc = ioctl(fp, VIDIOC_ENUM_DV_PRESETS, &preset); >> if (rc == -1) >> break; >> if ((preset.flags & needed->flags) != needed->flags) >> continue; >> if (!memcmp(&preset.fps, &needed->preset)) { >> found = preset->index; >> break; >> } >> } while (!rc && found < 0); >> } >> >> void main(void) { >> ... >> index = return_dv_preset_index(fp, cea861_vic16); >> ... >> } > > And the current equivalent is: > > struct v4l_dv_preset preset = { V4L2_DV_1080P60 }; > ioctl(f, VIDIOC_S_DV_PRESET, &preset); Yes, except for the fact that: - API spec needs addition for every new preset that we standardize; - It doesn't support a vendor-specific preset, as there's no way to discover what are the timing constants for the preset; - Namespacing is broken. > You want a whole new API that in my view makes things only more complicated and > misses existing functionality (such as the one above). I prefer to fix the API, if it is possible/doable on a non-messy way. Yet, as this API is currently used only by two drivers (DaVinci and tvp7002), it is better to fix it sooner than later, to avoid more efforts on fixing it. > Whereas with a few tweaks and possibly a new VIDIOC_G_PRESET_TIMINGS ioctl you > can offer the same functionality with the existing API. You're proposing a new "enum" preset timings that would present the missing info that VIDIOC_S_DV_PRESET doesn't present, except that an application will need to call 2 ioctl's in order to enumerate the presets, instead of one. > So, once again my proposal: > > ENUM_DV_PRESETS is extended with a flags field and a fps v4l2_fract (or frame_period, > whatever works best). Flags give you progressive vs interlaced, and I've no problem > adding things like IS_CEA861 or similar flags to that. > > The current set of presets remain in use (but get renamed with the old ones as aliases) > for CEA861 and (in the near future) VESA DMT timings. > Note that all the hardware I > know that has predefined timings does so only for those two standards. Makes sense > too: only the consumer electronic standards for SDTV/HDTV and the VGA-like PC monitor > standards are typical standards. We need a further investigation about that. What are the predefined timings defined for Samgung hardware? Also, one possible implementation for output devices would be to use EDID to retrieve the timings acceptable by the monitor/TV and compare to its internal capabilities. This is probably the only way to avoid requiring CAP_SYS_ADMIN for the DV calls, as a bad timing may damage the monitor and/or the V4L device. If a vendor decides to implement something like that (either in firmware or on Kernel), then the list of presets will likely have a mix with all standards. > For presets not related to those standards the easiest method I see is just to assign > a preset like (0x80000000 | index). I've thinked about that already. Yeah, this would fix part of the problem, allowing the implementation of vendor-specific timings and the support for other standards not covered yet. There are, however, some issues: 1) the API will be messier; 2) Imagine that a new timing were added as a vendor-specific timing. Later, that standard got recognized by some forum and were added at some standard. In that case, the vendor cannot simply change the preset index, as it will break the API. Also, duplicating the information is not a good idea. With the preset standards as flags, when this happens, all the driver needs is to add a new flag, without breaking the API. > We may need to add a VIDIOC_G_PRESET_TIMINGS, but I am not certain we really need > that. ENUM_DV_PRESETS may give sufficient information already. This will only work if there aren't two timings with the same fps/resolution, including on the vendor-specific timings. Let's suppose that there are two non-DMT, non-CEA standars, from two different vendors, that have different timings. With the current API, there's no way to differentiate them. > Based on my experience with GTF/CVT formats I strongly suspect that drivers will > need to implement a VIDIOC_QUERY_DV_TIMINGS ioctl and let a userspace library detect > the GTF/CVT standard. This is surprisingly complex (mostly due to extremely shoddy > standards). Maybe, but I bet that there are a few GTF/CVT standards that are implemented on a large amount of TV/monitors. It may make sense to have those added as presets. I'd love to get some feedback from other developers about that. Samsung? The issue with S_DV_TIMINGS is that we likely need to request for CAP_SYS_ADMIN, as a bad timing may damage the hardware. Currently, there's not such requirement for DaVinci/tvp7002, nor v4l2-ioctl enforces that, but this needs a fix. > For GTF/CVT output you want to use VIDIOC_S_DV_TIMINGS anyway. True. > The reason > there is no GTF/CVT support yet is simply because I don't want to make proposals > unless I actually implemented it and worked with it for some time to see what works > best. > > Everything you can do with your proposal you can do with mine, and mine doesn't > deprecate anything. See above. > BTW, in the case of HD-SDI transmitters/receivers the CEA-861 standard does not > apply, strictly speaking. That standard is covered by SMPTE 292M. It does support > most of the usual SDTV/HDTV formats as are defined in CEA-861, except that things > like front/back porch etc. do not apply to this serial interface. The idea behind > the presets is that it defines industry standard resolution/framerate combinations, > but the standards behind those differ per interface type. You don't really care > about those in an application. The user (or developer) just wants to select 1080P60 or > WUXGA. Ok. > I am frankly not certain anymore if we want to have the standard as part of > the macro name. Something like V4L2_DV_HDTV_1920X1080P60 might be more appropriate, > with comments next to it referring to the relevant standards depending on the > physical interface type. That's the problem with the namespace. I think that that's basically the reason why Xorg never created a macro naming for the resolutions. Whatever namespace we choose, we'll have troubles. I bet that, with your proposal, we'll end by having conflicts between two different standards that implement the same resolution/fps/"progressiveness". > And instead of using flags to denote the used standard, it might be better to > reserve a u16 for the standard. It seems that a standards bitmask may work better, as, eventually, the same timing may be defined by more than one standard. > History has shown that video formats stay around for a looong time, but the standards > describing them evolve continuously. True. We might end to have some timings that are different on a new standard revision, in order to fix some issue. That's why I think we need to expose all the timings at the DV enum ioctl. This gives more flexibility to the application to reject an specific standard if needed for whatever reason. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/video/v4l2-common.c b/drivers/media/video/v4l2-common.c index 06b9f9f..003e648 100644 --- a/drivers/media/video/v4l2-common.c +++ b/drivers/media/video/v4l2-common.c @@ -582,6 +582,7 @@ int v4l_fill_dv_preset_info(u32 preset, struct v4l2_dv_enum_preset *info) { 1920, 1080, "1080p@30" }, /* V4L2_DV_1080P30 */ { 1920, 1080, "1080p@50" }, /* V4L2_DV_1080P50 */ { 1920, 1080, "1080p@60" }, /* V4L2_DV_1080P60 */ + { 1920, 1080, "1080p@59.94" }, /* V4L2_DV_1080P59_94 */ }; if (info == NULL || preset >= ARRAY_SIZE(dv_presets)) diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h index 8a4c309..7c77c4e 100644 --- a/include/linux/videodev2.h +++ b/include/linux/videodev2.h @@ -872,6 +872,7 @@ struct v4l2_dv_enum_preset { #define V4L2_DV_1080P30 16 /* SMPTE 296M */ #define V4L2_DV_1080P50 17 /* BT.1120 */ #define V4L2_DV_1080P60 18 /* BT.1120 */ +#define V4L2_DV_1080P59_94 19 /* * D V B T T I M I N G S