On Wednesday 12 August 2009 03:05:28 Mauro Carvalho Chehab wrote: > Em Tue, 11 Aug 2009 17:08:27 +0200 > "Hans Verkuil" <hverkuil@xxxxxxxxx> escreveu: > > > > > > > I also agree with Trent that, instead of having to maintain some sort of > > > list > > > or code to identify string controls, the better would be to reserve one > > > bit to > > > indicate pointer controls (like bit 31). People tend to forget about > > > v4l2-compat32 layer, and I bet that, if we do some sort of list, people > > > will > > > forget to update it when adding a new control, and compat32 layer will > > > break. > > > So, ctrl_is_pointer() can be just: > > > > > > #define ctrl_is_pointer(id) (id & (1 << 31)) > > > > Now THAT is ugly. Anyway, you can't do it like that. Control IDs are not > > random but are grouped in control classes so that similar controls are > > together when you enumerate them. Adding flags to mark it as a specific > > type is 1) an ugly hack to work around an infrastructure problem, and 2) > > will make it impossible to group string controls together with controls > > that have a similar purpose. > > You just need to create separate groups for strings. As there are very few > cases where passing a string to a control is valid, I don't see this as a big > issue. Yes it is. You are moving strings into a separate class for no good reason. Why would RDS-related controls be split over two control classes? That makes not sense whatsoever. I absolutely categorically refuse to do something like that. > The point here is that compat layer adds an extra parsing time, since it > needs to repeat what is done later at v4l2-ioctl. That's said, once we have all > drivers migrated to use video_ioctl2, we can work on optimizing it by removing > the compat layer parser from it and letting this job to be done after parsed, > at v4l2-ioctl. This will help to save the costs added by this layer. The root of all evil is premature optimization. You are trying to fix something that isn't a problem. There are no performance issues with any of the V4L2 APIs. We have complexity problems, making it hard to write drivers and, in some cases, applications (inconsistent control handling is IMHO a big problem for applications). > > What should have happened is that we could test whether the size field is > > non-zero. Unfortunately, some apps do not set that field (currently a > > reserved field) to 0 and we never checked that in the kernel. So sadly we > > have to do it another way. > > It is not a Kernel task to check if the userspace is bad coded. If the > userspace apps are not respecting V4L2 API by not zeroing the reserved fields, > they need to be fixed, or they'll break sooner or later. What applications are > know to break with such change? Both mythtv and mplayer. vlc is OK. And yes, we should have checked. We check other arguments as well, so if we require that reserved should be zeroed, then we should check that as well. If we had done that, then I could have just checked the 'size' value. > > >> Also note that it is very likely that these two functions will disappear > > >> again in a few months: once the v4l framework has proper control support > > >> these functions should no longer be needed since this information can > > >> then be obtained directly from the framework. > > > > > > This don't change the fact that you'll need a sort of list or code that > > > associates a control id with a type, called by the compat layer. Testing > > > for a > > > bit costs less than any table seek method. > > > > O(log N) where N is the number of registered controls. It's implemented as > > a binary search. It is generic and will work with whatever types we might > > create in the future. > > Since you'll probably need to run it again when parsing the control code at the > framework, it will be, in fact, doubled, since you'll need to run this code > twice. On the other hand, a bit seek or a a test if size is zero is O(0) And when the framework is in place then all the O(N) comparisons that are currently used in drivers to lookup the controls are also replaced by O(log N). Of course, let's not forget the usual end-result of these ioctls: an i2c read or write: these are orders of magnitudes slower than anything else we are doing. I'm NOT going to fuck with the API just to shave of that single nanosecond of performance that you get when using extended controls running a 32-bit app in a 64 bit OS. As an alternative, I'm willing to make patches for mplayer and mythtv. Then we can check whether size is non-zero. Of course, this will break using extended controls in a 32-bit app in a 64-bit OS as long as these updated apps are not available upstream. I'm personally not willing to risk that just for that single nanosecond. Regards, Hans -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- 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
