LWN: Comments on "The videobuf2 API"

The videobuf2 API

neilbrown — Tue, 28 Jun 2011 01:45:44 +0000

I had a bit of a look at this code ... it is always nice to have a little motivation to look at something new to see what I can learn.

I think the code we are talking about is in drivers/media/video/videobuf2-core.c, starting at vb2_reqbufs().

The low level driver is expected to call this function to "Initiate Streaming".

This function calls back into the driver via 'queue_setup' to negotiate the number of buffers (and related details) - possibly twice if the original request could not be met. It also calls back into the driver using the buf_init callback (this is inside sub-function __vb2_queue_alloc) for device-specific initialisation of each allocated buffer.

The driver gets to control the size allocate for the 'vb2_buffer' by setting a 'buf_struct_size' field in the vb2_queue. You could almost look at this like a constant callback. The library code does ask the driver to do something for it, but as that something is just "give me a number" not "perform an operation", it is a number rather than a function.

The underlying pattern here is that of a complex library function that uses call-backs to handle some caller-specific details. This is a pattern that I find should be used with caution.

In simple cases it is a perfectly good pattern. A simple example is qsort which needs a callback to compare (and maybe another to swap) two entries in the array being sorted.

The 'page cache' library in Linux follows this pattern too with all the callbacks into the filesystem being collected in 'address_space_operation', and I think that is mostly OK, though bits of it seem a little complex ... I should probably give it more thought.

While I haven't done a proper study of this pattern, my feeling is that a sign of problems is when the library needs to call back to ask questions, rather than just to perform actions. This is a problem because it is hard to know if you have really asked all the questions that could be asked. i.e. Maybe the driver writer wants to do something a little bit beyond what the library writer foresaw. If you find that the library is in control rather than the driver being in control, then it is possible that the design is suffering from what I have called "The midlayer mistake", or something very like it. So thae aspect of this interface that concerns me is really queue_setup rather than the buffer allocation details.

The alternative is to provide a collection of fine-grained functionality in the library that the driver can use to create the desired result, and leave the driver to handle the control logic (though with suitable documentation and templates to help avoid common mistakes).

Were this approach applied to vb2_reqbufs (and I'm not saying that it should be, just exploring possibilities), then the driver would code the main loop that allocates N buffers for whatever N it finds to be appropriate, and would handle errors directly, thus making queue_setup redundant. The driver would initialise the buffers that it allocated (so buf_struct_size and buf_init would not be needed), but it would call in to the library to initialise the part of the structures that the library owns, and possibly to do some other common validity checking.

The important distinction between this approach and the currently implemented approach is about where control is - in the library or in the driver. The cost of putting control in the driver is that it is duplicated and more error prone. The cost of putting control in the library is that if a driver writer finds that they need something a little bit different it can be very hard to implement that cleanly.

Which one is most appropriate for videobuf2 I cannot say. One would need to implement both in at least a few drivers and compare the complexity to be sure. And even if you did find that one seemed a little bit cleaner than the other, you would need strong motivation to change working code!

The videobuf2 API

corbet — Fri, 24 Jun 2011 13:18:07 +0000

Please note that the article did not say "the authors of videobuf2 have no knowledge about object-oriented pattern in the kernel." I did make a sort of lame joke about not having read an article we had published a whole week before; apologies if that didn't come through.

The videobuf2 API

m.szyprowski — Fri, 24 Jun 2011 06:12:46 +0000

It is not true that the authors of videobuf2 have no knowledge about object-oriented pattern in the kernel. Earlier versions of videobuf2 patches had support for buf_allocate and buf_free driver's method (see for example: http://www.spinics.net/lists/linux-media/msg25386.html and http://www.spinics.net/lists/linux-media/msg25389.html). They have been removed later in favour of buffer_size entry in struct vb2_queue, because they were considered superfluous by other v4l2 developers (some discussion can be found here: http://linuxtv.org/irc/v4l/index.php?date=2010-11-25 ).

The videobuf2 API

jonabbey — Thu, 16 Jun 2011 23:27:14 +0000

Articles like this (and the CMA one above) make LWN.net well worth the subscription cost, Jonathan. Kudos!

Locking & the videobuf2 API

corbet — Thu, 16 Jun 2011 16:34:32 +0000

Understood, that all makes sense, sorry if I sounded critical. You've put in a lot of work and, as a result, V4L2 has gotten a lot better in the time I've been paying attention to it.

Locking & the videobuf2 API

hverkuil — Thu, 16 Jun 2011 16:28:32 +0000

I know. We don't hide it, BTW, you have to explicitly set it up. The simple fact is that the vast majority of V4L2 drivers need to serialize ioctl calls anyway. This is usually implemented pretty badly.

Another reason for doing this was the BKL removal were we needed something reasonably simple to convert old (usually unmaintained) drivers without having to do extensive code reviews.

Locking & the videobuf2 API

corbet — Thu, 16 Jun 2011 16:03:09 +0000

As you know, some of us are not entirely in agreement there. I don't think you can do a complex driver without being aware of locking; trying to hide it looks to me like an attempt to return to the good old days of uniprocessor systems. I don't know of any other kernel subsystem which tries to hide locking in the midlayers in this way - though there almost certainly is one somewhere.

Locking & the videobuf2 API

hverkuil — Thu, 16 Jun 2011 06:52:11 +0000

Well, locking is explained in Documentation/video4linux/v4l2-framework, section "v4l2_file_operations and locking". But it doesn't mention videobuf2 (must be fixed) and should probably be expanded/improved a bit.

But in a nutshell: you either set the 'lock' field in struct video_device to a mutex and then the framework will take care of serialization of file operations (mostly ioctls) for you, or you leave it at NULL and you have to do your own locking.

I tend to advocate using the framework serialization rather than doing it yourself since the chances of actually getting the locking right in a complex driver if you do it yourself are pretty close to zero, especially after the driver has undergone a few years of maintenance.