Hi Marek,
On Thursday 25 November 2010 10:48:39 Marek Szyprowski wrote:
> 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.
An explanation of what to do (and perhaps even more importantly what *not* to
do) to use VB2. Look at Hans' documentation for the control framework for
instance, there's an easy step-by-step explanation at the beginning. That
really helps getting things right.
> > 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.
I agree about the irq_spinlock. The vb_lock, however, is only used to protect
internal queue information, so I'm not sure why it would be a problem even for
drivers that have multiple queues.
> > > +/**
> > > + * __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.
That's not what I meant. You initialize mem_priv to NULL here, but you don't
initialize it to NULL in __vb2_buf_mem_free. Shouldn't it be done there too ?
<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.
I could accept that, but don't try to justify inefficient code by a fear of
over-optimization :-)
> > > +/**
> > > + * 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.
What I meant is that the allocator specific inline will call this function,
which will in turn call a function of the same allocator. Can't the allocator
inline function call to itself directly ?
> > > +/**
> > > + * __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."
I can see this being a potential problem for some applications. We would need
to change the spec to solve it though, so I'm fine with the implementation.
> > > +/**
> > > + * 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.
Let's do that then :-)
> > > +
> > > +/**
> > > + * 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.
OK.
> > > + * @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.
OK, I was too tired :-)
> > > + * @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.
I would personally prefer giving the size of the driver's own buffer object to
VB2.
> > > + * @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.
Hmmm... I'll comment on this when I'll review the patches that implement
read()/write().
> > > +/**
> > > + * 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.
I expect many drivers to create structures to embed queues in, with a 1-1
relationship, but there will probably be drivers that will have 2 queues in
the same structure, so I agree with you.
--
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
