Hi Junghak,
Patch 7/8 helped a lot in reducing the size of this one. But it is still difficult
to review so I would like to request one final (honest!) split for this patch:
Move all the code that does not depend on the new buf_ops into a separate patch.
So the new q->is_output and q->multiplanar field can be moved to that patch.
But also the changes to functions like vb2_expbuf() or streamon/off where some
checks are moved from core.c to v4l2.c can be done separately.
All such pretty easy to review modifications should be put in a separate patch,
leaving me with one remaining patch that I really need to study.
I recommend that you wait until 4.3-rc1 is released and merged back in our tree
since that will contain a number of vb2 changes (Jan Kara's work). So it makes
sense to rebase on top of that first before doing more work on this.
I did find a few things in this patch as well, see my comments below:
On 09/09/2015 01:19 PM, Junghak Sung wrote:
> Move v4l2-stuffs from videobuf2-core to videobuf2-v4l2. And make
> wrappers that use the vb2_core_* functions.
>
> Signed-off-by: Junghak Sung <jh1009.sung@xxxxxxxxxxx>
> Signed-off-by: Geunyoung Kim <nenggun.kim@xxxxxxxxxxx>
> Acked-by: Seung-Woo Kim <sw0312.kim@xxxxxxxxxxx>
> Acked-by: Inki Dae <inki.dae@xxxxxxxxxxx>
> ---
> drivers/media/v4l2-core/videobuf2-core.c | 517 ++++++++++++++++----------
> drivers/media/v4l2-core/videobuf2-internal.h | 51 +--
> drivers/media/v4l2-core/videobuf2-v4l2.c | 312 ++++++++++++----
> include/media/videobuf2-core.h | 20 +-
> include/media/videobuf2-v4l2.h | 3 +-
> 5 files changed, 601 insertions(+), 302 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c
> index 3e6ee0e..56d34f2 100644
> --- a/drivers/media/v4l2-core/videobuf2-core.c
> +++ b/drivers/media/v4l2-core/videobuf2-core.c
<snip>
> @@ -454,13 +426,34 @@ static bool __buffers_in_use(struct vb2_queue *q)
> {
> unsigned int buffer;
> for (buffer = 0; buffer < q->num_buffers; ++buffer) {
> - if (__buffer_in_use(q, q->bufs[buffer]))
> + if (vb2_buffer_in_use(q, q->bufs[buffer]))
> return true;
> }
> return false;
> }
>
> /**
> + * vb2_core_querybuf() - query video buffer information
> + * @q: videobuf queue
> + * @index: id number of the buffer
> + * @pb: buffer struct passed from userspace
> + *
> + * Should be called from vidioc_querybuf ioctl handler in driver.
> + * The passed buffer should have been verified.
> + * This function fills the relevant information for the userspace.
> + *
> + * The return values from this function are intended to be directly returned
> + * from vidioc_querybuf handler in driver.
> + */
> +int vb2_core_querybuf(struct vb2_queue *q, unsigned int index, void *pb)
> +{
> + call_bufop(q, fill_user_buffer, q->bufs[index], pb);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_core_querybuf);
This should be a void function since it never returns an error.
But do we need it at all? It really doesn't do anything that is core-specific.
The querybuf ioctl is really pure V4L2 and has nothing to do with the vb2 core.
> +
> +/**
> * __verify_userptr_ops() - verify that all memory operations required for
> * USERPTR queue type have been provided
> */
> @@ -1182,52 +1174,27 @@ static void __enqueue_in_driver(struct vb2_buffer *vb)
> call_void_vb_qop(vb, buf_queue, vb);
> }
>
> -int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> +static int __buf_prepare(struct vb2_buffer *vb, void *pb)
> {
> - struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
> struct vb2_queue *q = vb->vb2_queue;
> int ret;
>
> - ret = __verify_length(vb, b);
> - if (ret < 0) {
> - dprintk(1, "plane parameters verification failed: %d\n", ret);
> - return ret;
> - }
> - if (b->field == V4L2_FIELD_ALTERNATE && V4L2_TYPE_IS_OUTPUT(q->type)) {
> - /*
> - * If the format's field is ALTERNATE, then the buffer's field
> - * should be either TOP or BOTTOM, not ALTERNATE since that
> - * makes no sense. The driver has to know whether the
> - * buffer represents a top or a bottom field in order to
> - * program any DMA correctly. Using ALTERNATE is wrong, since
> - * that just says that it is either a top or a bottom field,
> - * but not which of the two it is.
> - */
> - dprintk(1, "the field is incorrectly set to ALTERNATE for an output buffer\n");
> - return -EINVAL;
> - }
> -
> if (q->error) {
> dprintk(1, "fatal error occurred on queue\n");
> return -EIO;
> }
>
> - vb->state = VB2_BUF_STATE_PREPARING;
Hmmm, this moved to v4l2.c. Why? This still belongs here as far as I can tell.
I suspect that was a copy-and-paste mistake.
> - vbuf->timestamp.tv_sec = 0;
> - vbuf->timestamp.tv_usec = 0;
> - vbuf->sequence = 0;
> -
> switch (q->memory) {
> case VB2_MEMORY_MMAP:
> - ret = __qbuf_mmap(vb, b);
> + ret = __qbuf_mmap(vb, pb);
> break;
> case VB2_MEMORY_USERPTR:
> down_read(¤t->mm->mmap_sem);
> - ret = __qbuf_userptr(vb, b);
> + ret = __qbuf_userptr(vb, pb);
> up_read(¤t->mm->mmap_sem);
> break;
> case VB2_MEMORY_DMABUF:
> - ret = __qbuf_dmabuf(vb, b);
> + ret = __qbuf_dmabuf(vb, pb);
> break;
> default:
> WARN(1, "Invalid queue type\n");
> @@ -1241,32 +1208,94 @@ int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b)
> return ret;
> }
>
> -int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
> - const char *opname)
> +/**
> + * vb2_verify_buffer() - verify the buffer information passed from userspace
> + */
> +int vb2_verify_buffer(struct vb2_queue *q,
> + enum vb2_memory memory, unsigned int type,
> + unsigned int index, unsigned int nplanes,
> + void *pplane, const char *opname)
> {
> - if (b->type != q->type) {
> + if (type != q->type) {
> dprintk(1, "%s: invalid buffer type\n", opname);
> return -EINVAL;
> }
>
> - if (b->index >= q->num_buffers) {
> + if (index >= q->num_buffers) {
> dprintk(1, "%s: buffer index out of range\n", opname);
> return -EINVAL;
> }
>
> - if (q->bufs[b->index] == NULL) {
> + if (q->bufs[index] == NULL) {
> /* Should never happen */
> dprintk(1, "%s: buffer is NULL\n", opname);
> return -EINVAL;
> }
>
> - if (b->memory != q->memory) {
> + if (memory != VB2_MEMORY_UNKNOWN && memory != q->memory) {
> dprintk(1, "%s: invalid memory type\n", opname);
> return -EINVAL;
> }
>
> - return __verify_planes_array(q->bufs[b->index], b);
> + if (q->is_multiplanar) {
> + struct vb2_buffer *vb = q->bufs[index];
> +
> + /* Is memory for copying plane information present? */
> + if (NULL == pplane) {
> + dprintk(1, "%s: multi-planar buffer passed but "
> + "planes array not provided\n", opname);
> + return -EINVAL;
> + }
This doesn't belong here. pplane is very much v4l2 specific and should be
tested there.
> +
> + if (nplanes < vb->num_planes || nplanes > VB2_MAX_PLANES) {
> + dprintk(1, "%s: incorrect planes array length, "
> + "expected %d, got %d\n",
> + opname, vb->num_planes, nplanes);
> + return -EINVAL;
> + }
> + }
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(vb2_verify_buffer);
> +
> +/**
> + * vb2_core_prepare_buf() - Pass ownership of a buffer from userspace to the kernel
> + * @q: videobuf2 queue
> + * @index: id number of the buffer
> + * @pb: buffer structure passed from userspace to vidioc_prepare_buf
> + * handler in driver
> + *
> + * Should be called from vidioc_prepare_buf ioctl handler of a driver.
> + * The passed buffer should have been verified.
> + * This function calls buf_prepare callback in the driver (if provided),
> + * in which driver-specific buffer initialization can be performed,
> + *
> + * The return values from this function are intended to be directly returned
> + * from vidioc_prepare_buf handler in driver.
> + */
> +int vb2_core_prepare_buf(struct vb2_queue *q, unsigned int index, void *pb)
> +{
> + struct vb2_buffer *vb;
> + int ret;
> +
> + vb = q->bufs[index];
> + if (vb->state != VB2_BUF_STATE_DEQUEUED) {
> + dprintk(1, "invalid buffer state %d\n",
> + vb->state);
> + return -EINVAL;
> + }
> +
> + ret = __buf_prepare(vb, pb);
> + if (!ret) {
> + /* Fill buffer information for the userspace */
> + call_bufop(q, fill_user_buffer, vb, pb);
> +
> + dprintk(1, "prepare of buffer %d succeeded\n", vb->index);
> + }
> + return ret;
> }
> +EXPORT_SYMBOL_GPL(vb2_core_prepare_buf);
>
> /**
> * vb2_start_streaming() - Attempt to start streaming.
Regards,
Hans
--
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
