Hi Hans,
Thanks for the patch.
On Friday 22 June 2012 14:21:17 Hans Verkuil wrote:
> From: Hans Verkuil <hans.verkuil@xxxxxxxxx>
>
> Split off the memory and type validation. This is done both from reqbufs and
> create_bufs, and will also be done by vb2 helpers in a later patch.
>
> Signed-off-by: Hans Verkuil <hans.verkuil@xxxxxxxxx>
> ---
> drivers/media/video/videobuf2-core.c | 153 +++++++++++++++++--------------
> 1 file changed, 80 insertions(+), 73 deletions(-)
>
> diff --git a/drivers/media/video/videobuf2-core.c
> b/drivers/media/video/videobuf2-core.c index 9d4e9ed..8486e33 100644
> --- a/drivers/media/video/videobuf2-core.c
> +++ b/drivers/media/video/videobuf2-core.c
> @@ -454,7 +454,53 @@ static int __verify_mmap_ops(struct vb2_queue *q)
> }
>
> /**
> - * vb2_reqbufs() - Initiate streaming
> + * __verify_memory_type() - do basic checks for memory and type
I'd expand this comment a bit, as "basic checks" doesn't really tell much
about the purpose of the checks. Maybe something like
"Check whether the memory type and buffer type passed to a buffer operation
are compatible with the queue."
> + */
> +static int __verify_memory_type(struct vb2_queue *q, __u32 memory, __u32
> type)
What about using enum v4l2_memory and enum v4l2_buf_type instead of __u32 ?
> +{
> + if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR) {
> + dprintk(1, "reqbufs: unsupported memory type\n");
> + return -EINVAL;
> + }
> +
> + if (type != q->type) {
> + dprintk(1, "reqbufs: requested type is incorrect\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * Make sure all the required memory ops for given memory type
> + * are available.
> + */
> + if (memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> + dprintk(1, "reqbufs: MMAP for current setup unsupported\n");
> + return -EINVAL;
> + }
> +
> + if (memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> + dprintk(1, "reqbufs: USERPTR for current setup unsupported\n");
> + return -EINVAL;
> + }
> +
> + /*
> + * Place the busy tests at the end: -EBUSY can be ignored when
> + * create_bufs is called with count == 0, but count == 0 should still
> + * do the memory and type validation.
> + */
> + if (q->fileio) {
> + dprintk(1, "reqbufs: file io in progress\n");
> + return -EBUSY;
> + }
> +
> + if (q->streaming) {
> + dprintk(1, "reqbufs: streaming active\n");
> + return -EBUSY;
> + }
create_bufs didn't check for q->streaming. Isn't it legal to add a buffer
during streaming ?
> + return 0;
> +}
> +
> +/**
> + * __reqbufs() - Initiate streaming
> * @q: videobuf2 queue
> * @req: struct passed from userspace to vidioc_reqbufs handler in driver
> *
> @@ -476,45 +522,10 @@ static int __verify_mmap_ops(struct vb2_queue *q)
> * The return values from this function are intended to be directly
> returned * from vidioc_reqbufs handler in driver.
> */
> -int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> +static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> {
> unsigned int num_buffers, allocated_buffers, num_planes = 0;
> - int ret = 0;
> -
> - if (q->fileio) {
> - dprintk(1, "reqbufs: file io in progress\n");
> - return -EBUSY;
> - }
> -
> - if (req->memory != V4L2_MEMORY_MMAP
> - && req->memory != V4L2_MEMORY_USERPTR) {
> - dprintk(1, "reqbufs: unsupported memory type\n");
> - return -EINVAL;
> - }
> -
> - if (req->type != q->type) {
> - dprintk(1, "reqbufs: requested type is incorrect\n");
> - return -EINVAL;
> - }
> -
> - if (q->streaming) {
> - dprintk(1, "reqbufs: streaming active\n");
> - return -EBUSY;
> - }
> -
> - /*
> - * Make sure all the required memory ops for given memory type
> - * are available.
> - */
> - if (req->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> - dprintk(1, "reqbufs: MMAP for current setup unsupported\n");
> - return -EINVAL;
> - }
> -
> - if (req->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> - dprintk(1, "reqbufs: USERPTR for current setup unsupported\n");
> - return -EINVAL;
> - }
> + int ret;
>
> if (req->count == 0 || q->num_buffers != 0 || q->memory != req->memory) {
> /*
> @@ -595,10 +606,23 @@ int vb2_reqbufs(struct vb2_queue *q, struct
> v4l2_requestbuffers *req)
>
> return 0;
> }
> +
> +/**
> + * vb2_reqbufs() - Wrapper for __reqbufs() that also verifies the memory
> and + * type values.
> + * @q: videobuf2 queue
> + * @req: struct passed from userspace to vidioc_reqbufs handler in driver
> + */
> +int vb2_reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req)
> +{
> + int ret = __verify_memory_type(q, req->memory, req->type);
> +
> + return ret ? ret : __reqbufs(q, req);
> +}
> EXPORT_SYMBOL_GPL(vb2_reqbufs);
>
> /**
> - * vb2_create_bufs() - Allocate buffers and any required auxiliary structs
> + * __create_bufs() - Allocate buffers and any required auxiliary structs
> * @q: videobuf2 queue
> * @create: creation parameters, passed from userspace to
> vidioc_create_bufs * handler in driver
> @@ -612,40 +636,10 @@ EXPORT_SYMBOL_GPL(vb2_reqbufs);
> * The return values from this function are intended to be directly
> returned * from vidioc_create_bufs handler in driver.
> */
> -int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers
> *create) +static int __create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create) {
> unsigned int num_planes = 0, num_buffers, allocated_buffers;
> - int ret = 0;
> -
> - if (q->fileio) {
> - dprintk(1, "%s(): file io in progress\n", __func__);
> - return -EBUSY;
> - }
> -
> - if (create->memory != V4L2_MEMORY_MMAP
> - && create->memory != V4L2_MEMORY_USERPTR) {
> - dprintk(1, "%s(): unsupported memory type\n", __func__);
> - return -EINVAL;
> - }
> -
> - if (create->format.type != q->type) {
> - dprintk(1, "%s(): requested type is incorrect\n", __func__);
> - return -EINVAL;
> - }
> -
> - /*
> - * Make sure all the required memory ops for given memory type
> - * are available.
> - */
> - if (create->memory == V4L2_MEMORY_MMAP && __verify_mmap_ops(q)) {
> - dprintk(1, "%s(): MMAP for current setup unsupported\n", __func__);
> - return -EINVAL;
> - }
> -
> - if (create->memory == V4L2_MEMORY_USERPTR && __verify_userptr_ops(q)) {
> - dprintk(1, "%s(): USERPTR for current setup unsupported\n",
__func__);
> - return -EINVAL;
> - }
> + int ret;
>
> if (q->num_buffers == VIDEO_MAX_FRAME) {
> dprintk(1, "%s(): maximum number of buffers already allocated\n",
> @@ -653,8 +647,6 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create) return -ENOBUFS;
> }
>
> - create->index = q->num_buffers;
> -
> if (!q->num_buffers) {
> memset(q->plane_sizes, 0, sizeof(q->plane_sizes));
> memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
> @@ -719,6 +711,21 @@ int vb2_create_bufs(struct vb2_queue *q, struct
> v4l2_create_buffers *create)
>
> return 0;
> }
> +
> +/**
> + * vb2_reqbufs() - Wrapper for __reqbufs() that also verifies the memory
> and + * type values.
> + * @q: videobuf2 queue
> + * @create: creation parameters, passed from userspace to
> vidioc_create_bufs + * handler in driver
> + */
> +int vb2_create_bufs(struct vb2_queue *q, struct v4l2_create_buffers
> *create) +{
> + int ret = __verify_memory_type(q, create->memory, create->format.type);
> +
> + create->index = q->num_buffers;
I think this changes the behaviour of create_bufs, it should thus belong to
the next patch.
> + return ret ? ret : __create_bufs(q, create);
> +}
> EXPORT_SYMBOL_GPL(vb2_create_bufs);
>
> /**
--
Regards,
Laurent Pinchart
--
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
