Hi Hans, On Tuesday 21 August 2012 08:39:53 Hans Verkuil wrote: > On Mon August 20 2012 22:46:04 Sakari Ailus wrote: > > On Mon, Aug 20, 2012 at 04:05:03PM -0300, Mauro Carvalho Chehab wrote: > > > Em 20-08-2012 05:30, Hans Verkuil escreveu: > > > > Hi all! > > > > > > > > Recently I had to add two new ioctls for the subdev API > > > > (include/linux/v4l2-subdev.h) and I noticed that the numbering of the > > > > ioctls was somewhat random. > > > > > > > > In most cases the ioctl number was the same as the V4L2 API > > > > counterpart, but for subdev-specific ioctls no rule exists. > > > > > > > > There are a few problems with this: because of the lack of rules there > > > > is a chance that in the future a subdev ioctl may end up to be > > > > identical to an existing V4L2 ioctl. Also, because the numbering > > > > isn't nicely increasing it makes it hard to create a lookup table as > > > > was done for the V4L2 ioctls. Well, you could do it, but it would be > > > > a very sparse array, wasting a lot of memory. > > > > > > > > Lookup tables have proven to be very useful, so we might want to > > > > introduce them for the subdev core code as well in the future. > > > > > > > > Since the subdev API is still marked experimental, I propose to > > > > renumber the ioctls and use the letter 'v' instead of 'V'. 'v' was > > > > used for V4L1, and so it is now available for reuse. > > > > > > 'v' is already used (mainly by fs): > > > > > > 'v' 00-1F linux/ext2_fs.h conflict! > > > 'v' 00-1F linux/fs.h conflict! > > > 'v' 00-0F linux/sonypi.h conflict! > > > 'v' C0-FF linux/meye.h conflict! > > > > > > Reusing the ioctl numbering is a bad thing, as tracing code like strace > > > will likely say that a different type of ioctl was called. > > > > > > (Yeah, unfortunately, this end by merging with duplicated stuff :< ) > > > > > > Also, I don't like the idea of deprecating it just because of that: > > > interfaces are supposed to be stable. > > > > > > It should be noticed that there are very few ioctls there. So, > > > using a lookup table is overkill. > > > > > > IMO, the better is to sort the ioctl's there at the header file, in > > > order to avoid ioctl duplicaton. > > > > Many of the V4L2 IOCTLs are being used on subdevs, too, to the extent that > > subdev_do_ioctl() in drivers/media/v4l2-core/v4l2-subdev.c has a switch > > statement with over 20 cases. We'll get rid of two once the old crop > > IOCTLs are removed but we've still got over 20, and the number is likely > > to grow in the future. Still it's just a fraction of what V4L2 has. > > > > We decided to use 'V' also for subdev IOCTLs for a reason I no longer > > remember. It's true there can be clashes with regular V4L2 IOCTLs in terms > > of IOCTL codes if the size of the argument struct matches. One of the > > reasons to use 'V' might have been that then some of the IOCTLs on a > > device would have different type (the letter in question) which wasn't > > considered pretty. 'V' is for V4L2 after all, and V4L2 subdev interface is > > part of the V4L2. > > > > The numbering is based on using V4L2 IOCTLs as such if they were > > applicable to subdevs as such (controls) in which case they're defined in > > videodev2.h, and if there was even a loosely corresponding IOCTL in V4L2 > > then use the same number (e.g. formats vs. media bus pixel codes) and > > otherwise something else. The "something else" case hasn't happened yet. > > > > It might have made sense to use a different type for the IOCTLs that > > aren't V4L2 IOCTLs (i.e. are subdev IOCTLs) for clarity but it's quite > > late for such a change. However if we think we definitely should do it > > then it should be done now or not at all... > > > > If we want to just improve the efficiency of the switch statement in > > subdev_do_ioctl() we could divide the IOCTLs based on e.g. a few last bits > > of the IOCTL number into buckets. > > It's not so much the switch efficiency. In practice there will be no > measurable speed difference. But a lookup table allows one to easily look > up information about the ioctl. > > But the main goal would be to guarantee that subdev ioctls and V4L2 ioctls > will never clash, since both types of ioctls can be used with a subdev node. We could also do that by redefining the V4L2 ioctls we use on subdevs in include/linux/v4l2-subdev.h with a VIDIOC_SUBDEV_ prefix. That would guarantee that all subdev-related ioctls are listed in a single place. However, I agree that it could be confusing for tools like strace. > > I don't have a strong opinion on this either way, but unless there's a > > concrete problem related to it I'd keep it as-is. We will definitely pick > > a new type for the property API when once we get that far. ;-) > > > > Could you elaborate what you were about to add? Something that would fall > > into the "something else" category perhaps? > > Yes indeed. It's two new ioctls for setting/getting the EDID. > > Currently I've chosen ioctl numbers that are not used by V4L2 (there are a > number of 'holes' in the ioctl list). > > If people think it is not worth the effort, then so be it. But if we do want > to do this, then we can't wait any longer. I'm not opposed to renumber subdev ioctls. I agree with you and Sakari that we should do it now if we want to do it. It would actually be a good occasion to introduce a change I've been thinking about for some time now by redefining control ioctls for subdevs to include a pad number in the structures used as arguments (v4l2_queryctrl, v4l2_querymenu and v4l2_ext_control) to allow for pad-specific controls later. -- Regards, Laurent Pinchart -- 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
