Hello,
On Thursday, November 25, 2010 2:17 AM Laurent Pinchart wrote:
> On Friday 19 November 2010 16:55:38 Marek Szyprowski wrote:
> > From: Pawel Osciak <p.osciak@xxxxxxxxxxx>
> >
> > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
> > multimedia devices. It acts as an intermediate layer between userspace
> > applications and device drivers. It also provides low-level, modular
> > memory management functions for drivers.
> >
> > Videobuf2 eases driver development, reduces drivers' code size and aids in
> > proper and consistent implementation of V4L2 API in drivers.
> >
> > Videobuf2 memory management backend is fully modular. This allows custom
> > memory management routines for devices and platforms with non-standard
> > memory management requirements to be plugged in, without changing the
> > high-level buffer management functions and API.
> >
> > The framework provides:
> > - implementations of streaming I/O V4L2 ioctls and file operations
> > - high-level video buffer, video queue and state management functions
> > - video buffer memory allocation and management
>
> Excellent job. I'm really pleased by the outstanding code quality.
> Nevertheless I got a bunch of comments (I wonder if I'm known as an annoying
> nit-picking reviewer in the community :-)).
>
> I've reviewed the patch from bottom to top (starting at the header file), so
> comments at the bottom can also apply to functions at the top when I got tired
> repeating the same information. Sorry about that weird order.
>
> Feel free to disagree with my comments, I've probably been overzealous during
> the review to make sure everything I had a doubt about would be properly
> addressed.
>
> It's now past 2AM and I don't have enough courage to review the other patches.
> I'm afraid they will have to wait until tomorrow. If they're of the same
> quality as this one I'm sure they will be good.
Thanks for your hard work! I can imagine how much time you had to spend on catching
all these things.
> BTW, where's the patch for Documentation/video4linux ? :-)
Well, I hope it will be ready one day ;) What kind of documentation do you expect
there? Vb2 structures and functions are already documented directly in the source.
> One last comment, why was the decision to remove all locking from vb2 taken ?
The idea came from Hans after posting version 1 and I really like it. It simplifies
a lot of things in the drivers. The original idea of irq_spinlock and vb_lock was
horrible. Having more than one queue in the driver meant that you need to make a
lot of nasty hacks to ensure proper locking, because in some cases you had to take
locks from all queues. The irq_lock usage was even worse. Videobuf used it to
silently mess around the buffers that have been already queued to the hardware.
Now, after Hans changes to v4l2 core (BKL removal in favor of simple mutex) almost
all this mess can be simply removed. Proper locking can be easily ensured by the
new mutex in the v4l2 core. There is no need to have any locks inside vb2. By
removing irq_lock the design of the drivers is easier to understand, because there
is no implicit actions on buffers that are processed by the hardware.
<snip>
> > +/**
> > + * __vb2_buf_userptr_put() - release userspace memory associated
> > associated + * with a USERPTR buffer
> > + */
> > +static void __vb2_buf_userptr_put(struct vb2_buffer *vb)
> > +{
> > + struct vb2_queue *q = vb->vb2_queue;
> > + unsigned int plane;
> > +
> > + for (plane = 0; plane < vb->num_planes; ++plane) {
> > + call_memop(q, plane, put_userptr, vb->planes[plane].mem_priv);
> > + vb->planes[plane].mem_priv = NULL;
>
> Any reason to initialize mem_priv to NULL here but not in __vb2_buf_mem_free ?
Yes, __vb2_buf_userptr_put is called when user calls QBUF with different address
than in the previous call, so the buffer is reinitialized without a call to
__vb2_buf_mem_free.
<snip>
> > +/**
> > + * __vb2_queue_alloc() - allocate videobuf buffer structures and (for MMAP
> > type) + * video buffer memory for all buffers/planes on the queue and
> > initializes the + * queue
> > + *
> > + * Returns the number of buffers successfully allocated.
> > + */
> > +static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory,
> > + unsigned int num_buffers, unsigned int num_planes)
> > +{
> > + unsigned long plane_sizes[VIDEO_MAX_PLANES];
> > + unsigned int buffer, plane;
> > + struct vb2_buffer *vb;
> > + int ret;
> > +
> > + /* Get requested plane sizes from the driver */
> > + for (plane = 0; plane < num_planes; ++plane) {
> > + ret = call_qop(q, plane_setup, q, plane, &plane_sizes[plane]);
> > + if (ret) {
> > + dprintk(1, "Plane setup failed\n");
> > + return ret;
> > + }
> > + }
> > +
> > + for (buffer = 0; buffer < num_buffers; ++buffer) {
> > + /* Allocate videobuf buffer structures */
> > + vb = __vb2_buf_alloc(q);
> > + if (!vb) {
> > + dprintk(1, "Memory alloc for buffer struct failed\n");
> > + break;
> > + }
> > +
> > + /* Length stores number of planes for multiplanar buffers */
> > + if (V4L2_TYPE_IS_MULTIPLANAR(q->type))
> > + vb->v4l2_buf.length = num_planes;
> > +
> > + vb->state = VB2_BUF_STATE_DEQUEUED;
> > + vb->vb2_queue = q;
> > + vb->num_planes = num_planes;
> > + vb->v4l2_buf.index = buffer;
> > + vb->v4l2_buf.type = q->type;
> > + vb->v4l2_buf.memory = memory;
> > +
> > + /* Allocate video buffer memory for the MMAP type */
> > + if (memory == V4L2_MEMORY_MMAP) {
> > + ret = __vb2_buf_mem_alloc(vb, plane_sizes);
> > + if (ret) {
> > + dprintk(1, "Failed allocating memory for "
> > + "buffer %d\n", buffer);
> > + __vb2_buf_free(q, vb);
> > + break;
>
> You're not cleaning up the other allocated buffers in case of failure, is that
> on purpose ?
>
> BTW, if allocation for the requested number of buffers fails, it would be nice
> to fall back to a smaller number of buffers. You could just use the number of
> buffers that have been allocated successfully and return that to userspace. Or
> is that what you're doing already ? In that case you should run that number
> through the queue_negotiate operation again to verify the driver can live with
> it.
Currently it falls back to the number of buffers that have been successfully
allocated, but you are right, that another call to queue_negotiate is required
because the driver might not be able to handle properly too small number of
buffers.
<snip>
> > +/**
> > + * __vb2_queue_free() - free the queue - video memory and related
> > information + * and return the queue to an uninitialized state
> > + */
> > +static int __vb2_queue_free(struct vb2_queue *q)
> > +{
> > + unsigned int buffer;
> > +
> > + /* Call driver-provided cleanup function for each buffer, if provided */
> > + if (q->ops->buf_cleanup) {
> > + for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> > + if (NULL == q->bufs[buffer])
> > + continue;
> > + q->ops->buf_cleanup(q->bufs[buffer]);
> > + }
> > + }
> > +
> > + /* Release video buffer memory */
> > + __vb2_free_mem(q);
>
> This is the only call to __vb2_free_mem. Maybe you could combine both
> functions and use a single loop instead of 3 separate ones.
IMHO having 2 separate functions makes the design easier to understand. A single loop
will be just an over-optimization as there is no performance issue in current version.
<snip>
> > +/**
> > + * vb2_plane_cookie() - Return allocator specific cookie for the given
> > plane + * @vb: vb2_buffer to which the plane in question belongs to
> > + * @plane_no: plane number for which the address is to be returned
> > + *
> > + * This function returns an allocator specific cookie for a given plane if
> > + * available, NULL otherwise. The allocator should provide some simple
> > static + * inline function which converts this cookie to the allocator
> > specific type + * that can be used directly by the driver to access the
> > buffer. This can be + * for example physical address, pointer to scatter
> > list or iommu mapping. + */
> > +void *vb2_plane_cookie(struct vb2_buffer *vb, unsigned int plane_no)
> > +{
> > + struct vb2_queue *q = vb->vb2_queue;
> > +
> > + if (plane_no > vb->num_planes)
> > + return NULL;
> > +
> > + return call_memop(q, plane_no, cookie, vb->planes[plane_no].mem_priv);
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_plane_cookie);
>
> If this function is meant to be called only from allocator-specific functions
> that cast the return value to an allocator-specific type, can't the allocator
> just calls to itself directly without going through the vb2 core ?
This function will be called from allocator specific inline, which in turn will
be called by the driver. The problem is the fact that the driver will have only
a pointer to vb2_buffer, not the allocator context associated with that buffer.
I don't want to mess with allocator data directly from the driver, so this
function is really needed.
> > +/**
> > + * vb2_qbuf() - Queue a buffer from userspace
> > + * @q: videobuf2 queue
> > + * @b: buffer structure passed from userspace to vidioc_qbuf handler
> > + * in driver
> > + *
> > + * Should be called from vidioc_qbuf ioctl handler of a driver.
> > + * This function:
> > + * 1) verifies the passed buffer,
> > + * 2) calls buf_prepare callback in the driver (if provided), in which
> > + * driver-specific buffer initialization can be performed,
> > + * 3) if streaming is on, queues the buffer in driver by the means of
> > buf_queue + * callback for processing.
> > + *
> > + * The return values from this function are intended to be directly
> > returned + * from vidioc_qbuf handler in driver.
> > + */
> > +int vb2_qbuf(struct vb2_queue *q, struct v4l2_buffer *b)
> > +{
> > + struct vb2_buffer *vb;
> > + int ret = 0;
> > +
> > +
> > + if (b->type != q->type) {
> > + dprintk(1, "qbuf: invalid buffer type\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (b->index >= q->num_buffers) {
> > + dprintk(1, "qbuf: buffer index out of range\n");
> > + return -EINVAL;
> > + }
> > +
> > + vb = q->bufs[b->index];
> > + if (NULL == vb) {
> > + /* Should never happen */
> > + dprintk(1, "qbuf: buffer is NULL\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (b->memory != q->memory) {
> > + dprintk(1, "qbuf: invalid memory type\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> > + dprintk(1, "qbuf: buffer already in use\n");
> > + return -EINVAL;
> > + }
> > +
> > + if (q->memory == V4L2_MEMORY_MMAP)
> > + ret = __qbuf_mmap(vb, b);
> > + else if (q->memory == V4L2_MEMORY_USERPTR)
> > + ret = __qbuf_userptr(vb, b);
>
> What if q->memory is neither of those ? Is that possible ?
Not it is not possible, but this way it will be easier to add new memory access
method in the future (V4L2_MEMORY_POOL ?).
<snip>
> > +/**
> > + * __vb2_wait_for_done_vb() - wait for a buffer to become available
> > + * for dequeuing
> > + *
> > + * Will sleep if required for nonblocking == false.
> > + */
> > +static int __vb2_wait_for_done_vb(struct vb2_queue *q, int nonblocking)
> > +{
> > +checks:
>
> Labels for cleanup after errors are OK, labels for other purposes should
> really be avoided where possible. Is there no simple way to write the function
> without using labels ?
Most if labels are probably a left-overs from removing the locks
>
> > + if (!q->streaming) {
> > + dprintk(1, "Streaming off, will not wait for buffers\n");
> > + return -EINVAL;
> > + }
>
> This means userspace applications won't be able to dequeue remaining buffers
> after stream off. Isn't it a serious restriction ?
Well, such restriction is written directly in the V4l2 spec: "The VIDIOC_STREAMOFF
ioctl, apart of aborting or finishing any DMA in progress, unlocks any user
pointer buffers locked in physical memory, and it removes all buffers from the
incoming and outgoing queues. That means all images captured but not dequeued
yet will be lost, likewise all images enqueued for output but not transmitted yet."
<snip>
> > +/**
> > + * __find_plane_by_off() - find plane associated with the given offset off
> > + */
>
> I think we can afford calling this __find_plane_by_offset :-)
>
> > +int __find_plane_by_off(struct vb2_queue *q, unsigned long off,
> > + unsigned int *_buffer, unsigned int *_plane)
>
> The function is not declared in the vb2 header file, should it be static ?
>
Yes, looks I've missed something.
> > +{
> > + struct vb2_buffer *vb;
> > + unsigned int buffer, plane;
>
> Linux discourages the declaration of multiple variables on the same line.
Declaring two completely not related variables in the same line should be in fact
avoided, but I don't see any reasons why not to declare variables for similar things
together. checkpatch doesn't find any problems with such construction ;)
<snip>
> > +/**
> > + * vb2_mmap() - map video buffers into application address space
> > + * @q: videobuf2 queue
> > + * @vma: vma passed to the mmap file operation handler in the driver
> > + *
> > + * Should be called from mmap file operation handler of a driver.
> > + * This function maps one plane of one of the available video buffers to
> > + * userspace. To map whole video memory allocated on reqbufs, this
> > function + * has to be called once per each plane per each buffer
> > previously allocated. + *
> > + * When the userspace application calls mmap, it passes to it an offset
> > returned + * to it earlier by the means of vidioc_querybuf handler. That
> > offset acts as + * a "cookie", which is then used to identify the plane to
> > be mapped. + * This function finds a plane with a matching offset and a
> > mapping is performed + * by the means of a provided memory operation.
> > + *
> > + * The return values from this function are intended to be directly
> > returned + * from the mmap handler in driver.
> > + */
> > +int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma)
> > +{
> > + unsigned long off = vma->vm_pgoff << PAGE_SHIFT;
> > + struct vb2_plane *vb_plane;
> > + struct vb2_buffer *vb;
> > + unsigned int buffer, plane;
<snip>
> > +
> > + vb = q->bufs[buffer];
> > + vb_plane = &vb->planes[plane];
> > +
> > + if (vb_plane->mapped) {
> > + dprintk(1, "Plane already mapped\n");
>
> What is the reason to disallow buffers from being mapped several times ? If
> there's a valid one, it would be worth adding it in a comment here.
I see no reason, maybe Pawel will explain it a bit more. IMHO this check can be removed.
>
> > + goto end;
>
> This will return 0, is that what you want ?
>
> > + }
> > +
> > + if (!mem_ops(q, plane)->mmap) {
> > + dprintk(1, "mmap not supported\n");
>
> Can that happen ? Can we have a queue of type MMAP with an allocator not
> supporting mmap ? That seems pretty pointless to me :-)
Well, right. This check should be moved to the reqbufs.
<snip>
> > +/**
> > + * vb2_has_consumers() - return true if the userspace is waiting for a
> > buffer + * @q: videobuf2 queue
> > + *
> > + * This function returns true if a userspace application is waiting for a
> > buffer + * to be ready to dequeue (on which a hardware operation has been
> > finished). + */
> > +bool vb2_has_consumers(struct vb2_queue *q)
> > +{
> > + return waitqueue_active(&q->done_wq);
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_has_consumers);
>
> What is this for ? Do you have use cases in mind ?
The vivi driver is using it, but frankly it should be redesigned to use some
other check.
<snip>
> > +
> > +/**
> > + * struct vb2_mem_ops - memory handling/memory allocator operations
> > + * @alloc: allocate video memory and, optionally, allocator private data,
> > + * return NULL on failure or a pointer to allocator private,
> > + * per-buffer data on success, NULL on failure; the returned
>
> Is the "return NULL on failure" information so important that you need to say
> it twice ? :-)
>
> > + * private structure will then be passed as buf_priv argument
> > + * to other ops in this structure
> > + * @put: inform the allocator that the buffer will no longer be used;
> > + * usually will result in the allocator freeing the buffer (if
> > + * no other users of this buffer are present); the buf_priv
> > + * argument is the allocator private per-buffer structure
> > + * previously returned from the alloc callback
>
> I understand that this is the opposite of alloc. Why isn't it called free ?
The problem is that not in all cases it will cause memory freeing. If you application
mmaped the buffer and closes the file, the mapping is still there so the buffer cannot
be freed. That's why this callback has been named put. If this is confusing, then
maybe a 'release' will be a better name. 'Free' is not a good name, because it
explicitly suggest that the memory will be freed imiedetely.
> > + * @get_userptr: acquire userspace memory for a hardware operation; used
> > for + * USERPTR memory types; vaddr is the address passed to the
> > + * videobuf layer when queuing a video buffer of USERPTR type;
> > + * should return an allocator private per-buffer structure
> > + * associated with the buffer on success, NULL on failure;
> > + * the returned private structure will then be passed as buf_priv
> > + * argument to other ops in this structure
> > + * @put_userptr: inform the allocator that a USERPTR buffer will no longer
> > + * be used
> > + * @vaddr: return a kernel virtual address to a given memory buffer
> > + * associated with the passed private structure or NULL if no
> > + * such mapping exists
> > + * @cookie: return allocator specific cookie for a given memory buffer
> > + * associated with the passed private structure or NULL if not
> > + * available
> > + * @num_users: return the current number of users of a memory buffer;
> > + * return 1 if the videobuf layer (or actually the driver using
> > + * it) is the only user
> > + * @mmap: setup a userspace mapping for a given memory buffer under
> > + * the provided virtual memory region
>
> Why is there no munmap() ?
Because there is no place to call such operation? munmap callback have been
removed from file operations some time ago. You can track mapping usage by
providing your own vm_ops with vm_open and vm_close callbacks. This is how it
is implemented by all vb2 mem allocators.
<snip>
> > +/**
> > + * struct vb2_ops - driver-specific callbacks
> > + *
> > + * @queue_negotiate: called from a VIDIOC_REQBUFS handler, before
> > + * memory allocation; driver should return the required
> > + * number of buffers in num_buffers and the required number
> > + * of planes per buffer in num_planes
>
> To be consistent with plane_setup, shouldn't this be called queue_setup ?
Right, this name is more adequate.
> > + * @plane_setup: called before memory allocation num_planes times;
> > + * driver should return the required size of plane number
> > + * plane_no
> > + * @unlock: release any locks taken while calling vb2 functions;
> > + * it is called before poll_wait function in vb2_poll
> > + * implementation; required to avoid deadlock when vb2_poll
> > + * function waits for a buffer
> > + * @lock: reacquire all locks released in the previous callback;
> > + * required to continue operation after sleeping in
> > + * poll_wait function
>
> Those names were not very clear to me at first sight. What about renaming
> those two operations poll_prepare and poll_finish (or similar) ? Feel free to
> disagree here, I'm not sure what I would prefer, but I thought I would throw
> the idea in.
I see your point here but I'm not sure it will make the code easier to understand.
Hans - could you comment on this?
>
> > + * @buf_alloc: called to allocate a struct vb2_buffer; driver usually
> > + * embeds it in its own custom buffer structure; returns
> > + * a pointer to vb2_buffer or NULL if failed; if not
> > + * provided kmalloc(sizeof(struct vb2_buffer, GFP_KERNEL)
> > + * is used
> > + * @buf_free: called to free the structure allocated by @buf_alloc;
> > + * if not provided kfree(vb) is used
>
> I'm curious, do we have use cases for buf_alloc != kmalloc and buf_free !=
> kfree ?
Well, the problem is not which function to call, but the size that is passed as the
argument. Drivers usually wants to embed the struct vb2 inside its own 'buffer'
structure. See vivi driver ported to vb2 for the reference. The driver get a pointer
to vb2 and the uses containerof() to get his own buffer. To make it possible the
buffer need to be allocated by the driver not the vb2. Currently I found no other
way of solving this than such callbacks.
> > + * @buf_init: called once after allocating a buffer (in MMAP case)
> > + * or after acquiring a new USERPTR buffer; drivers may
> > + * perform additional buffer-related initialization;
> > + * initialization failure (return != 0) will prevent
> > + * queue setup from completing successfully; optional
> > + * @buf_prepare: called every time the buffer is queued from userspace;
> > + * drivers may perform any initialization required before
> > + * each hardware operation in this callback;
> > + * if an error is returned, the buffer will not be queued
> > + * in driver; optional
> > + * @buf_finish: called before every dequeue of the buffer back to
> > + * userspace; drivers may perform any operations required
> > + * before userspace accesses the buffer; optional
> > + * @buf_cleanup: called once before the buffer is freed; drivers may
> > + * perform any additional cleanup; optional
> > + * @start_streaming: called once before entering 'streaming' state;
> > enables + * driver to recieve buffers over buf_queue() callback
> > + * @stop_streaming: called when 'streaming' state must be disabled; driver
> > + * should stop any dma transactions or wait until they
> > + * finish and give back all buffers it got from buf_queue()
> > + * callback
>
> start_streaming is only called in response to vb2_streamon, and stop_streaming
> in response to vb2_streamoff or vb2_release (implicit streamoff). I think it
> would be better to require the driver to stop streaming before calling
> vb2_streamoff and vb2_release, and have the driver start streaming only when
> vb2_streamon returns. I don't really like vb2 trying to manage the driver's
> streaming state. Those two operations could then be removed.
Hardware streaming starts not only from STREAMON ioctl, but also from read/write
and poll in case of file io access types. My idea was to make all vb2 functions
to be simple callback for the respective ioctls or file ops, and move all the logic
that need to be implemented in the driver to the queue callbacks. Such separation
make the driver much easier to understand and avoids some common mistakes like
forgetting to 'start streaming' in the poll implementation or so.
<snip>
> > +/**
> > + * struct vb2_queue - a videobuf queue
> > + *
> > + * @type: current queue type
> > + * @memory: current memory type used
>
> current implies that the value can change over the life of the object. I think
> that's not the case here.
>
> > + * @drv_priv: driver private data, passed on vb2_queue_init
>
> What about letting drivers embed vb2_queue inside a structure of their own if
> they need private data, and use container_of() instead of drv_priv ?
I'm not sure this is a better idea. Even now you can use container_of() way, but I see some
advantages of drv_priv approach. Imagine a driver with more than one queue. Usually all
queues will have drv_priv pointing to the same place, so some common callbacks can be
assigned to all of them. In container_of() approach callback for each queue will need to
use different method of accessing the driver private data (different offset), so you will
get additional almost duplicated code.
>
> > + * @bufs: videobuf buffer structures
> > + * @num_buffers: number of allocated/used buffers
> > + * @vb_lock: for ioctl handler and queue state changes synchronization
> > + * @queued_list: list of buffers currently queued from userspace
> > + * @done_list: list of buffers ready to be dequeued to userspace
> > + * @done_lock: lock to protect done_list list
> > + * @done_wq: waitqueue for processes waiting for buffers ready to be
> > dequeued + * @ops: driver-specific callbacks
> > + * @alloc_ctx: memory type/allocator-specific callbacks
> > + * @streaming: current streaming state
> > + * @userptr_supported: true if queue supports USERPTR types
> > + * @mmap_supported: true if queue supports MMAP types
> > + */
> > +struct vb2_queue {
> > + enum v4l2_buf_type type;
> > + enum v4l2_memory memory;
> > + void *drv_priv;
> > +
> > +/* private: internal use only */
> > + struct vb2_buffer *bufs[VIDEO_MAX_FRAME];
>
> What about dynamically allocating this based on the number of buffers ? It
> might not be worth the hassle.
IMHO these few bytes are not worth the amount of work required to change the design.
<snip>
> > +/**
> > + * vb2_plane_size() - return plane size in bytes
> > + * @vb: buffer for which plane size should be returned
> > + * @plane_no: plane number for which size should be returned
> > + */
> > +static inline unsigned long
> > +vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no)
> > +{
> > + if (plane_no < vb->num_planes)
> > + return vb->v4l2_planes[plane_no].length;
>
> Why do you check that plane_no is valid here and not in the above two
> functions ?
My fault. I will add checks.
Best regards
--
Marek Szyprowski
Samsung Poland R&D Center
--
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
