> 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@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html