Re: [RFC PATCH v4 8/8] [media] videobuf2: Remove v4l2-dependencies from videobuf2-core

[Hans Verkuil <hverkuil@xs4all.nl>]

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@samsung.com>
> Signed-off-by: Geunyoung Kim <nenggun.kim@samsung.com>
> Acked-by: Seung-Woo Kim <sw0312.kim@samsung.com>
> Acked-by: Inki Dae <inki.dae@samsung.com>
> ---
>  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(&current->mm->mmap_sem);
> -		ret = __qbuf_userptr(vb, b);
> +		ret = __qbuf_userptr(vb, pb);
>  		up_read(&current->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