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.szyprowski@xxxxxxxxxxx> 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.osciak@xxxxxxxxxxx>
>> >
>> > 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 majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
