Hello,
On Monday, November 22, 2010 12:41 PM Sewoon Park wrote:
> Hi~ Marek.
> Some comments in here.
>
> Saturday, November 20, 2010 12:56 AM Marek Szyprowski wrote :
> > To: linux-media@xxxxxxxxxxxxxxx
> > Cc: m.szyprowski@xxxxxxxxxxx; pawel@xxxxxxxxxx; kyungmin.park@xxxxxxxxxxx
> > Subject: [PATCH 1/7] v4l: add videobuf2 Video for Linux 2 driver framework
> >
> > 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.
> >
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:
> > + if (!q->streaming) {
> > + dprintk(1, "Streaming off, will not wait for buffers\n");
> > + return -EINVAL;
> > + }
> > +
> > + /*
> > + * Buffers may be added to vb_done_list without holding the
> > driver's
> > + * lock, but removal is performed only while holding both driver's
> > + * lock and the vb_done_lock spinlock. Thus we can be sure that as
> > + * long as we hold lock, the list will remain not empty if this
> > + * check succeeds.
> > + */
> > + if (list_empty(&q->done_list)) {
> > + int retval;
> > + if (nonblocking) {
> > + dprintk(1, "Nonblocking and no buffers to dequeue, "
> > + "will not wait\n");
> > + return -EAGAIN;
> > + }
> > +
> > + /*
> > + * We are streaming and nonblocking, wait for another buffer
> > to
> > + * become ready or for streamoff. Driver's lock is released
> > to
> > + * allow streamoff or qbuf to be called while waiting.
> > + */
> > + call_qop(q, unlock, q);
> > +
> > + /*
> > + * Although the mutex is released here, we will be
> > reevaluating
> > + * both conditions again after reacquiring it.
> > + */
> > + dprintk(3, "Will sleep waiting for buffers\n");
> > + retval = wait_event_interruptible(q->done_wq,
> > + !list_empty(&q->done_list) || !q->streaming);
>
> How about use wait_event_timeout() or other wait functions?
> If h/w or sub-device(e.g camera sensor module) does not operating due to
> any reason, then maybe fall into blocking state forever.
Videobuf2 is not a right place for such timeout handling. If your hardware is known
to fail for some unknown reason it is your responsibility to add a proper timer and
handle this in your own driver.
> > +int vb2_queue_init(struct vb2_queue *q, const struct vb2_ops *ops,
> > + const struct vb2_alloc_ctx *alloc_ctx,
> > + enum v4l2_buf_type type, void *drv_priv)
> > +{
> > + unsigned int i;
> > +
> > + BUG_ON(!q);
> > + BUG_ON(!ops);
> > + BUG_ON(!ops->queue_negotiate);
> > + BUG_ON(!ops->plane_setup);
> > + BUG_ON(!ops->buf_queue);
> > +
> > + BUG_ON(!alloc_ctx);
> > + BUG_ON(!alloc_ctx->mem_ops);
> > +
> > + memset(q, 0, sizeof *q);
> > + q->ops = ops;
> > +
> > + for (i = 0; i < VIDEO_MAX_PLANES; ++i)
> > + q->alloc_ctx[i] = alloc_ctx;
>
> According to my understanding, the alloc_ctx means that
> each plane per buffer use each allocator.
> for example,
> It is possible alloc_ctx[0] use CMA, alloc_ctx[1] use DMA-coherent allocator.
The idea was to enable a possibility to use different context for the SAME allocator type
what makes a bit more sense than using 2 completely different allocators so planes inside
the same buffer.
You can create for example 2 different contexts of CMA allocator, first for memory bank A
and second for memory bank B. Then you can assign the first CMA context to planes numer 0
and second to planes numer 1.
> How to set in these case?
> I think you don't control about it like above for-loop.
The original idea was to use vb2_set_alloc_ctx() function to reassign the context in runtime
after the queue has been initialized. I'm not sure this is the most clean approach. I'm
considering to add one another callback (get_allocator) but this will need some more design
changes in the way the format and number of planes are negotiated. I need to check how much
more complicated it will be.
> > +
> > + q->type = type;
> > + q->drv_priv = drv_priv;
> > +
> > + INIT_LIST_HEAD(&q->queued_list);
> > + INIT_LIST_HEAD(&q->done_list);
> > + spin_lock_init(&q->done_lock);
> > + init_waitqueue_head(&q->done_wq);
> > +
> > + return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(vb2_queue_init);
>
> snip
>
> > +
> > +struct vb2_queue;
> > +
> > +/**
> > + * struct vb2_buffer - represents a video buffer
> > + * @v4l2_buf: struct v4l2_buffer associated with this buffer;
> > can
> > + * be read by the driver and relevant entries can be
> > + * changed by the driver in case of CAPTURE types
> > + * (such as timestamp)
> > + * @v4l2_planes: struct v4l2_planes associated with this buffer; can
> > + * be read by the driver and relevant entries can be
> > + * changed by the driver in case of CAPTURE types
> > + * (such as bytesused); NOTE that even for single-planar
> > + * types, the v4l2_planes[0] struct should be used
> > + * instead of v4l2_buf for filling bytesused - drivers
> > + * should use the vb2_set_plane_payload() function for
> > that
> > + * @vb2_queue: the queue to which this driver belongs
> > + * @drv_entry: list entry to be used by driver for storing the
> > buffer
>
> Outdated comment. VB2 doesn't use this field any longer.
Right, I thought I've removed all of it...
> From now on,
> driver will use own buffer that is included vb2_buffer, right?
Right, this is the goal
> I wonder why remove drv_entry field in struct vb2_buffer?
I've decided to remove it to makes things clear which structure elements are used
by vb2 and which are not. There was a lot of confusion how this drv_entry element
should be initialized and used. Moving it to driver private buffer (which is
created anyway) makes it much easier to understand how the ownership of the buffer
is handled between the driver and videobuf2.
>
> > + * @num_planes: number of planes in the buffer
> > + * on an internal driver queue
> > + * @state: current buffer state; do not change
> > + * @queued_entry: entry on the queued buffers list, which holds all
> > + * buffers queued from userspace
> > + * @done_entry: entry on the list that stores all buffers ready
> > to
> > + * be dequeued to userspace
> > + * @planes: private per-plane information; do not change
> > + * @num_planes_mapped: number of mapped planes; do not change
> > + */
>
> snip
>
> > +/**
> > + * struct vb2_queue - a videobuf queue
> > + *
> > + * @type: current queue type
> > + * @memory: current memory type used
> > + * @drv_priv: driver private data, passed on vb2_queue_init
> > + * @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];
> > + unsigned int num_buffers;
> > +
> > + struct list_head queued_list;
> > +
> > + struct list_head done_list;
> > + spinlock_t done_lock;
> > + wait_queue_head_t done_wq;
> > +
> > + const struct vb2_ops *ops;
> > + const struct vb2_alloc_ctx *alloc_ctx[VIDEO_MAX_PLANES];
> > +
> > + int streaming:1;
> > + int userptr_supported:1;
> > + int mmap_supported:1;
> > +};
>
> As you know, VIDIOC_REQBUFS is checking I/O support or not.
> Memory mapping(or user pointer) I/O is not supported the ioctl
> returns an EINVAL error code.
> Therefore, two kinds of variables(userptr_supported, mmap_supported)
> to use vb2_queue, right?
Right, they must be some kind of leftovers from the earlier versions. Please note that
videobuf2 has been developed for quite a long time and its internal design changed a few
times before the first public version. I will remove them in the next version of the
patches.
> But, there's no usage anywhere.
> Why donât you initialize in vb2_queue_init() when is called from driver?
> and check this variables in vb2_reqbufs().
I'm not sure this is the right way to handle this. MMap/UserPtr modes depends mainly on
the allocator itself, so videobuf2 should check whether all required ops for particular
memory access type are provided.
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
