Re: [RFC API] Renumber subdev ioctls

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

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