Hi all,
Marek has already answered most of the comments, some of mine below.
On Thu, Nov 25, 2010 at 01:48, Marek Szyprowski
<m.szyprow...@samsung.com> wrote:
> 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.osc...@samsung.com>
>> >
>> > Videobuf2 is a Video for Linux 2 API-compatible driver framework for
[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.
It's on purpose to fall back, as Marek explained. I agree this should
be renegotiated with the driver though.
>
> <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.
>
Yes, this was solely to make the code cleaner. I purposedly made most
of those functions modular, because they were difficult to grasp
otherwise. I would really like them to stay that way.
>>
>> > + 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."
>
Yes, I didn't like it too much either, but as Marek pointed out,
that's what the spec says.
> <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.
First thing, I think we talked about that on IRC with Hans and agreed
that we don't really need to map a buffer multiple times from one
process. This is also carried over from vb1 as far as I can remember.
I think it was there because vb1 used to allocate memory on mmap, to
prevent multiple allocations. This check could be removed, but do we
really need such a feature?
>
>>
>> > + 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.
Yeah, this is a bit overcautious. You don't need to move it though,
there is a check for that in reqbufs already (__verify_mmap_ops()).
>
> <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.
Yes vivi uses it. Why do you think it should be redesigned? Is there a
better way to check whether an application is waiting for a buffer
than checking whether any application is in the queue designed
especially for that? :)
>> > +/**
>> > + * 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.
>
We discussed this many times already, never thought it'd resurface
again. I believe "negotiate" is a much better name. This function
negotiates the number of buffers and planes with the driver, it can be
readjusted depending on the situation. But plane_setup is not
negotiating anything, just asks for sizes and uses them as provided.
Pretty sure Hans will back me up on this one :)
>>
>> > + * @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 really didn't like how the concept of embedding structures
complicated both vb1 and is now complicating vb2. Maybe we should
think about going back to driver private structures and fixed-size
videobuf buffers?
>> > + * @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.
>
I agree with Marek, I remember the problems with starting streaming in
OMAP hardware, but believe the simplest case should be the easiest to
implement. Unless this prevents OMAP drivers from using vb2, I thnik
it should stay as is. Would this cause trouble for OMAP drivers?
> <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.
>
I think we'd like to make vb2 as easy to use, as possible, to
encourage adoption. Sorry, but I believe that container_of doesn't
make it easier to use and understand :)
>>
>> > + * @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.
My first implementation actually had that array dynamically allocated
as well. But the code for that was quite complicated only to gain
those few bytes and I decided to make this static. This is just a few
bytes as Marek says and I'm willing to pay that price for simplicity
and less potential bugs, not mentioning readability and
maintainability.
--
Best regards,
Pawel Osciak
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
