| Message ID | 1440590372-2377-3-git-send-email-jh1009.sung@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Wed, 26 Aug 2015 20:59:29 +0900 Junghak Sung <jh1009.sung@samsung.com> escreveu: > Remove v4l2-specific stuff from struct vb2_buffer and add member variables > related with buffer management. > > struct vb2_plane { > <snip> > /* plane information */ > unsigned int bytesused; > unsigned int length; > union { > unsigned int offset; > unsigned long userptr; > int fd; > } m; > unsigned int data_offset; > } > > struct vb2_buffer { > <snip> > /* buffer information */ > unsigned int num_planes; > unsigned int index; > unsigned int type; > unsigned int memory; > > struct vb2_plane planes[VIDEO_MAX_PLANES]; > <snip> > }; > > And create struct vb2_v4l2_buffer as container buffer for v4l2 use. > > struct vb2_v4l2_buffer { > struct vb2_buffer vb2_buf; > > __u32 flags; > __u32 field; > struct timeval timestamp; > struct v4l2_timecode timecode; > __u32 sequence; > }; The comments seemed a little hard for me to read, but the changes look ok. I made some comments mostly regarding to documentation. See below. > This patch includes only changes inside of videobuf2. So, it is required to > modify all device drivers which use videobuf2. So, in practice, we need to fold both patches 2 and 3 when merging upstream, to avoid breaking git bisectability. > > 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 | 324 +++++++++++++++++------------- > include/media/videobuf2-core.h | 50 ++--- > include/media/videobuf2-v4l2.h | 26 +++ > 3 files changed, 236 insertions(+), 164 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index ab00ea0..9266d50 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -35,10 +35,10 @@ > static int debug; > module_param(debug, int, 0644); > > -#define dprintk(level, fmt, arg...) \ > - do { \ > - if (debug >= level) \ > - pr_info("vb2: %s: " fmt, __func__, ## arg); \ > +#define dprintk(level, fmt, arg...) \ > + do { \ > + if (debug >= level) \ > + pr_info("vb2: %s: " fmt, __func__, ## arg); \ > } while (0) > > #ifdef CONFIG_VIDEO_ADV_DEBUG > @@ -53,7 +53,7 @@ module_param(debug, int, 0644); > > #define log_memop(vb, op) \ > dprintk(2, "call_memop(%p, %d, %s)%s\n", \ > - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ > + (vb)->vb2_queue, (vb)->index, #op, \ > (vb)->vb2_queue->mem_ops->op ? "" : " (nop)") > > #define call_memop(vb, op, args...) \ > @@ -115,7 +115,7 @@ module_param(debug, int, 0644); > > #define log_vb_qop(vb, op, args...) \ > dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \ > - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ > + (vb)->vb2_queue, (vb)->index, #op, \ > (vb)->vb2_queue->ops->op ? "" : " (nop)") > > #define call_vb_qop(vb, op, args...) \ > @@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) > unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]); > > mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane], > - size, dma_dir, q->gfp_flags); > + size, dma_dir, q->gfp_flags); > if (IS_ERR_OR_NULL(mem_priv)) > goto free; > > /* Associate allocator private data with this plane */ > vb->planes[plane].mem_priv = mem_priv; > - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; > + vb->planes[plane].length = q->plane_sizes[plane]; > } > > return 0; > @@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb) > for (plane = 0; plane < vb->num_planes; ++plane) { > call_void_memop(vb, put, vb->planes[plane].mem_priv); > vb->planes[plane].mem_priv = NULL; > - dprintk(3, "freed plane %d of buffer %d\n", plane, > - vb->v4l2_buf.index); > + dprintk(3, "freed plane %d of buffer %d\n", plane, vb->index); > } > } > > @@ -269,7 +268,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) > > call_void_memop(vb, detach_dmabuf, p->mem_priv); > dma_buf_put(p->dbuf); > - memset(p, 0, sizeof(*p)); > + p->mem_priv = NULL; > + p->dbuf = NULL; > + p->dbuf_mapped = 0; > } > > /** > @@ -299,7 +300,7 @@ static void __setup_lengths(struct vb2_queue *q, unsigned int n) > continue; > > for (plane = 0; plane < vb->num_planes; ++plane) > - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; > + vb->planes[plane].length = q->plane_sizes[plane]; > } > } > > @@ -314,10 +315,10 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > unsigned long off; > > if (q->num_buffers) { > - struct v4l2_plane *p; > + struct vb2_plane *p; > vb = q->bufs[q->num_buffers - 1]; > - p = &vb->v4l2_planes[vb->num_planes - 1]; > - off = PAGE_ALIGN(p->m.mem_offset + p->length); > + p = &vb->planes[vb->num_planes - 1]; > + off = PAGE_ALIGN(p->m.offset + p->length); > } else { > off = 0; > } > @@ -328,12 +329,12 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > continue; > > for (plane = 0; plane < vb->num_planes; ++plane) { > - vb->v4l2_planes[plane].m.mem_offset = off; > + vb->planes[plane].m.offset = off; > > dprintk(3, "buffer %d, plane %d offset 0x%08lx\n", > buffer, plane, off); > > - off += vb->v4l2_planes[plane].length; > + off += vb->planes[plane].length; > off = PAGE_ALIGN(off); > } > } > @@ -347,7 +348,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > * 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 int num_buffers, unsigned int num_planes) > { > unsigned int buffer; > struct vb2_buffer *vb; > @@ -361,16 +362,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, > 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 = q->num_buffers + buffer; > - vb->v4l2_buf.type = q->type; > - vb->v4l2_buf.memory = memory; > + vb->index = q->num_buffers + buffer; > + vb->type = q->type; > + vb->memory = memory; > > /* Allocate video buffer memory for the MMAP type */ > if (memory == V4L2_MEMORY_MMAP) { > @@ -418,7 +415,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers) > struct vb2_buffer *vb; > > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > - ++buffer) { > + ++buffer) { > vb = q->bufs[buffer]; > if (!vb) > continue; > @@ -451,7 +448,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > * just return -EAGAIN. > */ > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > - ++buffer) { > + ++buffer) { > if (q->bufs[buffer] == NULL) > continue; > if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) { > @@ -462,7 +459,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > > /* Call driver-provided cleanup function for each buffer, if provided */ > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > - ++buffer) { > + ++buffer) { > struct vb2_buffer *vb = q->bufs[buffer]; > > if (vb && vb->planes[0].mem_priv) > @@ -536,7 +533,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > > /* Free videobuf buffers */ > for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > - ++buffer) { > + ++buffer) { > kfree(q->bufs[buffer]); > q->bufs[buffer] = NULL; > } > @@ -590,23 +587,22 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) > if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > for (plane = 0; plane < vb->num_planes; ++plane) { > length = (b->memory == V4L2_MEMORY_USERPTR || > - b->memory == V4L2_MEMORY_DMABUF) > - ? b->m.planes[plane].length > - : vb->v4l2_planes[plane].length; > + b->memory == V4L2_MEMORY_DMABUF) > + ? b->m.planes[plane].length > + : vb->planes[plane].length; > bytesused = b->m.planes[plane].bytesused > - ? b->m.planes[plane].bytesused : length; > + ? b->m.planes[plane].bytesused : length; > > if (b->m.planes[plane].bytesused > length) > return -EINVAL; > > if (b->m.planes[plane].data_offset > 0 && > - b->m.planes[plane].data_offset >= bytesused) > + b->m.planes[plane].data_offset >= bytesused) > return -EINVAL; > } > } else { > length = (b->memory == V4L2_MEMORY_USERPTR) > - ? b->length : vb->v4l2_planes[0].length; > - bytesused = b->bytesused ? b->bytesused : length; > + ? b->length : vb->planes[0].length; > > if (b->bytesused > length) > return -EINVAL; > @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) > */ > static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > { > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > struct vb2_queue *q = vb->vb2_queue; > + unsigned int plane; > > /* Copy back data such as timestamp, flags, etc. */ > - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); > - b->reserved2 = vb->v4l2_buf.reserved2; > - b->reserved = vb->v4l2_buf.reserved; > + b->index = vb->index; > + b->type = vb->type; > + b->memory = vb->memory; > + b->bytesused = 0; > + > + b->flags = vbuf->flags; > + b->field = vbuf->field; > + b->timestamp = vbuf->timestamp; > + b->timecode = vbuf->timecode; > + b->sequence = vbuf->sequence; > > if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { > /* > @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > * for it. The caller has already verified memory and size. > */ > b->length = vb->num_planes; > - memcpy(b->m.planes, vb->v4l2_planes, > - b->length * sizeof(struct v4l2_plane)); > + for (plane = 0; plane < vb->num_planes; ++plane) { > + struct v4l2_plane *pdst = &b->m.planes[plane]; > + struct vb2_plane *psrc = &vb->planes[plane]; > + > + pdst->bytesused = psrc->bytesused; > + pdst->length = psrc->length; > + if (q->memory == V4L2_MEMORY_MMAP) > + pdst->m.mem_offset = psrc->m.offset; > + else if (q->memory == V4L2_MEMORY_USERPTR) > + pdst->m.userptr = psrc->m.userptr; > + else if (q->memory == V4L2_MEMORY_DMABUF) > + pdst->m.fd = psrc->m.fd; > + pdst->data_offset = psrc->data_offset; > + } > } else { > /* > * We use length and offset in v4l2_planes array even for > * single-planar buffers, but userspace does not. > */ > - b->length = vb->v4l2_planes[0].length; > - b->bytesused = vb->v4l2_planes[0].bytesused; > + b->length = vb->planes[0].length; > + b->bytesused = vb->planes[0].bytesused; > if (q->memory == V4L2_MEMORY_MMAP) > - b->m.offset = vb->v4l2_planes[0].m.mem_offset; > + b->m.offset = vb->planes[0].m.offset; > else if (q->memory == V4L2_MEMORY_USERPTR) > - b->m.userptr = vb->v4l2_planes[0].m.userptr; > + b->m.userptr = vb->planes[0].m.userptr; > else if (q->memory == V4L2_MEMORY_DMABUF) > - b->m.fd = vb->v4l2_planes[0].m.fd; > + b->m.fd = vb->planes[0].m.fd; > } > > /* > @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > b->flags &= ~V4L2_BUFFER_MASK_FLAGS; > b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; > if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != > - V4L2_BUF_FLAG_TIMESTAMP_COPY) { > + V4L2_BUF_FLAG_TIMESTAMP_COPY) { > /* > * For non-COPY timestamps, drop timestamp source bits > * and obtain the timestamp source from the queue. > @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); > static int __verify_userptr_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || > - !q->mem_ops->put_userptr) > + !q->mem_ops->put_userptr) > return -EINVAL; > > return 0; > @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) > static int __verify_mmap_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || > - !q->mem_ops->put || !q->mem_ops->mmap) > + !q->mem_ops->put || !q->mem_ops->mmap) > return -EINVAL; > > return 0; > @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) > static int __verify_dmabuf_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || > - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > - !q->mem_ops->unmap_dmabuf) > + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > + !q->mem_ops->unmap_dmabuf) > return -EINVAL; > > return 0; > @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, > enum v4l2_memory memory, enum v4l2_buf_type type) > { > if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && > - memory != V4L2_MEMORY_DMABUF) { > + memory != V4L2_MEMORY_DMABUF) { > dprintk(1, "unsupported memory type\n"); > return -EINVAL; > } > @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > * Driver also sets the size and allocator context for each plane. > */ > ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, > - q->plane_sizes, q->alloc_ctx); > + q->plane_sizes, q->alloc_ctx); > if (ret) > return ret; > > @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > num_buffers = allocated_buffers; > > ret = call_qop(q, queue_setup, q, NULL, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > > if (!ret && allocated_buffers < num_buffers) > ret = -ENOMEM; > @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > * buffer and their sizes are acceptable > */ > ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > if (ret) > return ret; > > @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > * queue driver has set up > */ > ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > > if (!ret && allocated_buffers < num_buffers) > ret = -ENOMEM; > @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > return; > > if (WARN_ON(state != VB2_BUF_STATE_DONE && > - state != VB2_BUF_STATE_ERROR && > - state != VB2_BUF_STATE_QUEUED)) > + state != VB2_BUF_STATE_ERROR && > + state != VB2_BUF_STATE_QUEUED)) > state = VB2_BUF_STATE_ERROR; > > #ifdef CONFIG_VIDEO_ADV_DEBUG > @@ -1195,7 +1212,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > vb->cnt_buf_done++; > #endif > dprintk(4, "done processing on buffer %d, state: %d\n", > - vb->v4l2_buf.index, state); > + vb->index, state); > > /* sync buffers */ > for (plane = 0; plane < vb->num_planes; ++plane) > @@ -1268,25 +1285,26 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer *vb) > * v4l2_buffer by the userspace. The caller has already verified that struct > * v4l2_buffer has a valid number of planes. > */ > -static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b, > - struct v4l2_plane *v4l2_planes) > +static void __fill_vb2_buffer(struct vb2_buffer *vb, > + const struct v4l2_buffer *b, struct vb2_plane *planes) > { > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > unsigned int plane; > > if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > if (b->memory == V4L2_MEMORY_USERPTR) { > for (plane = 0; plane < vb->num_planes; ++plane) { > - v4l2_planes[plane].m.userptr = > + planes[plane].m.userptr = > b->m.planes[plane].m.userptr; > - v4l2_planes[plane].length = > + planes[plane].length = > b->m.planes[plane].length; > } > } > if (b->memory == V4L2_MEMORY_DMABUF) { > for (plane = 0; plane < vb->num_planes; ++plane) { > - v4l2_planes[plane].m.fd = > + planes[plane].m.fd = > b->m.planes[plane].m.fd; > - v4l2_planes[plane].length = > + planes[plane].length = > b->m.planes[plane].length; > } > } > @@ -1310,7 +1328,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * applications working. > */ > for (plane = 0; plane < vb->num_planes; ++plane) { > - struct v4l2_plane *pdst = &v4l2_planes[plane]; > + struct vb2_plane *pdst = &planes[plane]; > struct v4l2_plane *psrc = &b->m.planes[plane]; > > if (psrc->bytesused == 0) > @@ -1340,13 +1358,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * old userspace applications working. > */ > if (b->memory == V4L2_MEMORY_USERPTR) { > - v4l2_planes[0].m.userptr = b->m.userptr; > - v4l2_planes[0].length = b->length; > + planes[0].m.userptr = b->m.userptr; > + planes[0].length = b->length; > } > > if (b->memory == V4L2_MEMORY_DMABUF) { > - v4l2_planes[0].m.fd = b->m.fd; > - v4l2_planes[0].length = b->length; > + planes[0].m.fd = b->m.fd; > + planes[0].length = b->length; > } > > if (V4L2_TYPE_IS_OUTPUT(b->type)) { > @@ -1354,25 +1372,26 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > vb2_warn_zero_bytesused(vb); > > if (vb->vb2_queue->allow_zero_bytesused) > - v4l2_planes[0].bytesused = b->bytesused; > + planes[0].bytesused = b->bytesused; > else > - v4l2_planes[0].bytesused = b->bytesused ? > - b->bytesused : v4l2_planes[0].length; > + planes[0].bytesused = b->bytesused ? > + b->bytesused : planes[0].length; > } else > - v4l2_planes[0].bytesused = 0; > + planes[0].bytesused = 0; > > } > > /* Zero flags that the vb2 core handles */ > - vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; > + vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; > if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != > - V4L2_BUF_FLAG_TIMESTAMP_COPY || !V4L2_TYPE_IS_OUTPUT(b->type)) { > + V4L2_BUF_FLAG_TIMESTAMP_COPY || > + !V4L2_TYPE_IS_OUTPUT(b->type)) { > /* > * Non-COPY timestamps and non-OUTPUT queues will get > * their timestamp and timestamp source flags from the > * queue. > */ > - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; > + vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; > } > > if (V4L2_TYPE_IS_OUTPUT(b->type)) { > @@ -1382,11 +1401,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > * The 'field' is valid metadata for this output buffer > * and so that needs to be copied here. > */ > - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; > - vb->v4l2_buf.field = b->field; > + vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE; > + vbuf->field = b->field; > } else { > /* Zero any output buffer flags as this is a capture buffer */ > - vb->v4l2_buf.flags &= ~V4L2_BUFFER_OUT_FLAGS; > + vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS; > } > } > > @@ -1395,7 +1414,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > */ > static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) > { > - __fill_vb2_buffer(vb, b, vb->v4l2_planes); > + __fill_vb2_buffer(vb, b, vb->planes); > return call_vb_qop(vb, buf_prepare, vb); > } > > @@ -1404,7 +1423,7 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) > */ > static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > { > - struct v4l2_plane planes[VIDEO_MAX_PLANES]; > + struct vb2_plane planes[VIDEO_MAX_PLANES]; > struct vb2_queue *q = vb->vb2_queue; > void *mem_priv; > unsigned int plane; > @@ -1419,9 +1438,9 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > > for (plane = 0; plane < vb->num_planes; ++plane) { > /* Skip the plane if already verified */ > - if (vb->v4l2_planes[plane].m.userptr && > - vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr > - && vb->v4l2_planes[plane].length == planes[plane].length) > + if (vb->planes[plane].m.userptr && > + vb->planes[plane].m.userptr == planes[plane].m.userptr > + && vb->planes[plane].length == planes[plane].length) > continue; > > dprintk(3, "userspace address for plane %d changed, " > @@ -1447,12 +1466,15 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > } > > vb->planes[plane].mem_priv = NULL; > - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); > + vb->planes[plane].bytesused = 0; > + vb->planes[plane].length = 0; > + vb->planes[plane].m.userptr = 0; > + vb->planes[plane].data_offset = 0; > > /* Acquire each plane's memory */ > mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane], > - planes[plane].m.userptr, > - planes[plane].length, dma_dir); > + planes[plane].m.userptr, > + planes[plane].length, dma_dir); > if (IS_ERR_OR_NULL(mem_priv)) { > dprintk(1, "failed acquiring userspace " > "memory for plane %d\n", plane); > @@ -1466,8 +1488,12 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > * Now that everything is in order, copy relevant information > * provided by userspace. > */ > - for (plane = 0; plane < vb->num_planes; ++plane) > - vb->v4l2_planes[plane] = planes[plane]; > + for (plane = 0; plane < vb->num_planes; ++plane) { > + vb->planes[plane].bytesused = planes[plane].bytesused; > + vb->planes[plane].length = planes[plane].length; > + vb->planes[plane].m.userptr = planes[plane].m.userptr; > + vb->planes[plane].data_offset = planes[plane].data_offset; > + } > > if (reacquired) { > /* > @@ -1494,10 +1520,11 @@ err: > /* In case of errors, release planes that were already acquired */ > for (plane = 0; plane < vb->num_planes; ++plane) { > if (vb->planes[plane].mem_priv) > - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); > + call_void_memop(vb, put_userptr, > + vb->planes[plane].mem_priv); > vb->planes[plane].mem_priv = NULL; > - vb->v4l2_planes[plane].m.userptr = 0; > - vb->v4l2_planes[plane].length = 0; > + vb->planes[plane].m.userptr = 0; > + vb->planes[plane].length = 0; > } > > return ret; > @@ -1508,7 +1535,7 @@ err: > */ > static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > { > - struct v4l2_plane planes[VIDEO_MAX_PLANES]; > + struct vb2_plane planes[VIDEO_MAX_PLANES]; > struct vb2_queue *q = vb->vb2_queue; > void *mem_priv; > unsigned int plane; > @@ -1544,7 +1571,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > > /* Skip the plane if already verified */ > if (dbuf == vb->planes[plane].dbuf && > - vb->v4l2_planes[plane].length == planes[plane].length) { > + vb->planes[plane].length == planes[plane].length) { > dma_buf_put(dbuf); > continue; > } > @@ -1558,11 +1585,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > > /* Release previously acquired memory if present */ > __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); > + vb->planes[plane].bytesused = 0; > + vb->planes[plane].length = 0; > + vb->planes[plane].m.fd = 0; > + vb->planes[plane].data_offset = 0; > > /* Acquire each plane's memory */ > - mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane], > - dbuf, planes[plane].length, dma_dir); > + mem_priv = call_ptr_memop(vb, attach_dmabuf, > + q->alloc_ctx[plane], dbuf, planes[plane].length, > + dma_dir); > if (IS_ERR(mem_priv)) { > dprintk(1, "failed to attach dmabuf\n"); > ret = PTR_ERR(mem_priv); > @@ -1592,8 +1623,12 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > * Now that everything is in order, copy relevant information > * provided by userspace. > */ > - for (plane = 0; plane < vb->num_planes; ++plane) > - vb->v4l2_planes[plane] = planes[plane]; > + for (plane = 0; plane < vb->num_planes; ++plane) { > + vb->planes[plane].bytesused = planes[plane].bytesused; > + vb->planes[plane].length = planes[plane].length; > + vb->planes[plane].m.fd = planes[plane].m.userptr; > + vb->planes[plane].data_offset = planes[plane].data_offset; > + } > > if (reacquired) { > /* > @@ -1644,6 +1679,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) > > static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > { > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > struct vb2_queue *q = vb->vb2_queue; > int ret; > > @@ -1672,9 +1708,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > } > > vb->state = VB2_BUF_STATE_PREPARING; > - vb->v4l2_buf.timestamp.tv_sec = 0; > - vb->v4l2_buf.timestamp.tv_usec = 0; > - vb->v4l2_buf.sequence = 0; > + vbuf->timestamp.tv_sec = 0; > + vbuf->timestamp.tv_usec = 0; > + vbuf->sequence = 0; > > switch (q->memory) { > case V4L2_MEMORY_MMAP: > @@ -1701,7 +1737,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > } > > static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b, > - const char *opname) > + const char *opname) > { > if (b->type != q->type) { > dprintk(1, "%s: invalid buffer type\n", opname); > @@ -1768,7 +1804,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) > /* Fill buffer information for the userspace */ > __fill_v4l2_buffer(vb, b); > > - dprintk(1, "prepare of buffer %d succeeded\n", vb->v4l2_buf.index); > + dprintk(1, "prepare of buffer %d succeeded\n", vb->index); > } > return ret; > } > @@ -1800,7 +1836,7 @@ static int vb2_start_streaming(struct vb2_queue *q) > /* Tell the driver to start streaming */ > q->start_streaming_called = 1; > ret = call_qop(q, start_streaming, q, > - atomic_read(&q->owned_by_drv_count)); > + atomic_read(&q->owned_by_drv_count)); > if (!ret) > return 0; > > @@ -1810,7 +1846,7 @@ static int vb2_start_streaming(struct vb2_queue *q) > /* > * If you see this warning, then the driver isn't cleaning up properly > * after a failed start_streaming(). See the start_streaming() > - * documentation in videobuf2-v4l2.h for more information how buffers > + * documentation in videobuf2-core.h for more information how buffers > * should be returned to vb2 in start_streaming(). > */ > if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { > @@ -1841,11 +1877,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > { > int ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); > struct vb2_buffer *vb; > + struct vb2_v4l2_buffer *vbuf; > > if (ret) > return ret; > > vb = q->bufs[b->index]; > + vbuf = to_vb2_v4l2_buffer(vb); > > switch (vb->state) { > case VB2_BUF_STATE_DEQUEUED: > @@ -1877,11 +1915,11 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > * and the timecode field and flag if needed. > */ > if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) == > - V4L2_BUF_FLAG_TIMESTAMP_COPY) > - vb->v4l2_buf.timestamp = b->timestamp; > - vb->v4l2_buf.flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; > + V4L2_BUF_FLAG_TIMESTAMP_COPY) > + vbuf->timestamp = b->timestamp; > + vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; > if (b->flags & V4L2_BUF_FLAG_TIMECODE) > - vb->v4l2_buf.timecode = b->timecode; > + vbuf->timecode = b->timecode; > } > > trace_vb2_qbuf(q, vb); > @@ -1903,13 +1941,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > * then we can finally call start_streaming(). > */ > if (q->streaming && !q->start_streaming_called && > - q->queued_count >= q->min_buffers_needed) { > + q->queued_count >= q->min_buffers_needed) { > ret = vb2_start_streaming(q); > if (ret) > return ret; > } > > - dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index); > + dprintk(1, "qbuf of buffer %d succeeded\n", vb->index); > return 0; > } > > @@ -2099,9 +2137,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb) > } > } > > -static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) > +static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, > + bool nonblocking) > { > struct vb2_buffer *vb = NULL; > + struct vb2_v4l2_buffer *vbuf = NULL; > int ret; > > if (b->type != q->type) { > @@ -2134,14 +2174,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n > > trace_vb2_dqbuf(q, vb); > > + vbuf = to_vb2_v4l2_buffer(vb); > if (!V4L2_TYPE_IS_OUTPUT(q->type) && > - vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST) > + vbuf->flags & V4L2_BUF_FLAG_LAST) > q->last_buffer_dequeued = true; > /* go back to dequeued state */ > __vb2_dqbuf(vb); > > dprintk(1, "dqbuf of buffer %d, with state %d\n", > - vb->v4l2_buf.index, vb->state); > + vb->index, vb->state); > > return 0; > } > @@ -2197,7 +2238,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > /* > * If you see this warning, then the driver isn't cleaning up properly > * in stop_streaming(). See the stop_streaming() documentation in > - * videobuf2-v4l2.h for more information how buffers should be returned > + * videobuf2-core.h for more information how buffers should be returned > * to vb2 in stop_streaming(). > */ > if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { > @@ -2399,7 +2440,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, > vb = q->bufs[buffer]; > > for (plane = 0; plane < vb->num_planes; ++plane) { > - if (vb->v4l2_planes[plane].m.mem_offset == off) { > + if (vb->planes[plane].m.offset == off) { > *_buffer = buffer; > *_plane = plane; > return 0; > @@ -2557,7 +2598,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) > * The buffer length was page_aligned at __vb2_buf_mem_alloc(), > * so, we need to do the same here. > */ > - length = PAGE_ALIGN(vb->v4l2_planes[plane].length); > + length = PAGE_ALIGN(vb->planes[plane].length); > if (length < (vma->vm_end - vma->vm_start)) { > dprintk(1, > "MMAP invalid, as it would overflow buffer length\n"); > @@ -2577,10 +2618,10 @@ EXPORT_SYMBOL_GPL(vb2_mmap); > > #ifndef CONFIG_MMU > unsigned long vb2_get_unmapped_area(struct vb2_queue *q, > - unsigned long addr, > - unsigned long len, > - unsigned long pgoff, > - unsigned long flags) > + unsigned long addr, > + unsigned long len, > + unsigned long pgoff, > + unsigned long flags) > { > unsigned long off = pgoff << PAGE_SHIFT; > struct vb2_buffer *vb; > @@ -2731,7 +2772,7 @@ EXPORT_SYMBOL_GPL(vb2_poll); > * responsible of clearing it's content and setting initial values for some > * required entries before calling this function. > * q->ops, q->mem_ops, q->type and q->io_modes are mandatory. Please refer > - * to the struct vb2_queue description in include/media/videobuf2-v4l2.h > + * to the struct vb2_queue description in include/media/videobuf2-core.h > * for more information. > */ > int vb2_queue_init(struct vb2_queue *q) > @@ -2739,16 +2780,16 @@ int vb2_queue_init(struct vb2_queue *q) > /* > * Sanity check > */ > - if (WARN_ON(!q) || > - WARN_ON(!q->ops) || > - WARN_ON(!q->mem_ops) || > - WARN_ON(!q->type) || > - WARN_ON(!q->io_modes) || > - WARN_ON(!q->ops->queue_setup) || > - WARN_ON(!q->ops->buf_queue) || > - WARN_ON(q->timestamp_flags & > - ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | > - V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) > + if (WARN_ON(!q) || > + WARN_ON(!q->ops) || > + WARN_ON(!q->mem_ops) || > + WARN_ON(!q->type) || > + WARN_ON(!q->io_modes) || > + WARN_ON(!q->ops->queue_setup) || > + WARN_ON(!q->ops->buf_queue) || > + WARN_ON(q->timestamp_flags & > + ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | > + V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) > return -EINVAL; > > /* Warn that the driver should choose an appropriate timestamp type */ > @@ -2852,7 +2893,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > * Sanity check > */ > if (WARN_ON((read && !(q->io_modes & VB2_READ)) || > - (!read && !(q->io_modes & VB2_WRITE)))) > + (!read && !(q->io_modes & VB2_WRITE)))) > return -EINVAL; > > /* > @@ -3063,9 +3104,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ > buf->queued = 0; > buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0) > : vb2_plane_size(q->bufs[index], 0); > - /* Compensate for data_offset on read in the multiplanar case. */ > + /* > + * Compensate for data_offset on read > + * in the multiplanar case > + */ > if (is_multiplanar && read && > - fileio->b.m.planes[0].data_offset < buf->size) { > + fileio->b.m.planes[0].data_offset < buf->size) { > buf->pos = fileio->b.m.planes[0].data_offset; > buf->size -= buf->pos; > } > @@ -3257,7 +3301,7 @@ static int vb2_thread(void *data) > * contact the linux-media mailinglist first. > */ > int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv, > - const char *thread_name) > + const char *thread_name) > { > struct vb2_threadio_data *threadio; > int ret = 0; > @@ -3491,7 +3535,7 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf, > if (vb2_queue_is_busy(vdev, file)) > goto exit; > err = vb2_write(vdev->queue, buf, count, ppos, > - file->f_flags & O_NONBLOCK); > + file->f_flags & O_NONBLOCK); > if (vdev->queue->fileio) > vdev->queue->owner = file->private_data; > exit: > @@ -3515,7 +3559,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf, > if (vb2_queue_is_busy(vdev, file)) > goto exit; > err = vb2_read(vdev->queue, buf, count, ppos, > - file->f_flags & O_NONBLOCK); > + file->f_flags & O_NONBLOCK); > if (vdev->queue->fileio) > vdev->queue->owner = file->private_data; > exit: > diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > index 155991e..8787a6c 100644 > --- a/include/media/videobuf2-core.h > +++ b/include/media/videobuf2-core.h > @@ -115,6 +115,16 @@ struct vb2_plane { > void *mem_priv; > struct dma_buf *dbuf; > unsigned int dbuf_mapped; > + > + /* plane information */ > + unsigned int bytesused; > + unsigned int length; > + union { > + unsigned int offset; > + unsigned long userptr; > + int fd; > + } m; > + unsigned int data_offset; > }; Nitpick: it would be good to add a documentation for struct vb2_plane, describing what each field means on this struct. Granted, this could be added after this patch series. Btw, I don't see much reason to have the: /* plane information */ comment here, as this struct is all about plane information, right? Or, maybe you wanted, instead, to comment that those fields should have what's there at struct v4l2_plane? That would make sense ;) So, I would change this comment to something like: /* * Should contain enough plane information to contain the * fields needed to fill struct v4l2_plane at videodev2.h */ > > /** > @@ -161,41 +171,33 @@ 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 > - * @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 > + * @index: id number of the buffer > + * @type: buffer type > + * @memory: the method, in which the actual data is passed > + * @num_planes: number of planes in the buffer > + * on an internal driver queue > * @planes: private per-plane information; do not change > */ > struct vb2_buffer { > - struct v4l2_buffer v4l2_buf; > - struct v4l2_plane v4l2_planes[VIDEO_MAX_PLANES]; > - > struct vb2_queue *vb2_queue; > > - unsigned int num_planes; > - > -/* Private: internal use only */ > + /* Private: internal use only */ > enum vb2_buffer_state state; > > struct list_head queued_entry; > struct list_head done_entry; > > + /* buffer information */ > + unsigned int index; > + unsigned int type; > + unsigned int memory; > + unsigned int num_planes; Nitpick: Those comments that follow Documentation/kernel-doc-nano-HOWTO.txt are used to produce DocBook data, on both html and manpages formats. As documented there, DocBook discards documentation for all fields after a /*private: ...*/ comment. In other words, we need to take care of putting things after /*private:*/ only when we're 100% sure that such fields are not meant to be filled/used by the callers, and will be used only internally, and don't deserve any documentation for the kABI. As you're adding documentation for those fields, and num_planes were documented before your series, I suspect that this is not what you want. So, please move them to be before the private: comment. Btw, if your patches are based on top of the current patchwork tree e. g. if it has this patch: http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=d071c833a0d30e7aae0ea565d92ef83c79106d6f Then you can make the Kernel to handle all those kernel-doc-nano comments with: make cleandocs && make DOCBOOKS=device-drivers.xml htmldocs It will produce some html pages like: http://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/mediadev.html With can be useful to check if the documentation tags are properly placed. > struct vb2_plane planes[VIDEO_MAX_PLANES]; > > #ifdef CONFIG_VIDEO_ADV_DEBUG > @@ -390,7 +392,7 @@ struct v4l2_fh; > * @threadio: thread io internal data, used only if thread is active > */ > struct vb2_queue { > - enum v4l2_buf_type type; > + unsigned int type; > unsigned int io_modes; > unsigned fileio_read_once:1; > unsigned fileio_write_immediately:1; > @@ -409,7 +411,7 @@ struct vb2_queue { > > /* private: internal use only */ > struct mutex mmap_lock; > - enum v4l2_memory memory; > + unsigned int memory; Will the vb2-core use type/memory fields or just vb2-v4l2? As you removed the enum, I suspect you won't be relying on having the videodev2.h header included here, right. If so, then the meaning of the type/memory fields are private to the caller of the VB2-core (e. .g. the meaning are private to vb2-v4l2 and vb2-dvb). So, you should be changing the description of those fields at the doc-nano to: ... * @type: private type whose content is defined by the vb2-core caller. * For example, for V4L2, it should match the V4L2_BUF_TYPE_* * in include/uapi/linux/videodev2.h ... to let it clear. > struct vb2_buffer *bufs[VIDEO_MAX_FRAME]; > unsigned int num_buffers; > > @@ -571,7 +573,7 @@ static inline void vb2_set_plane_payload(struct vb2_buffer *vb, > unsigned int plane_no, unsigned long size) > { > if (plane_no < vb->num_planes) > - vb->v4l2_planes[plane_no].bytesused = size; > + vb->planes[plane_no].bytesused = size; > } > > /** > @@ -583,7 +585,7 @@ static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb, > unsigned int plane_no) > { > if (plane_no < vb->num_planes) > - return vb->v4l2_planes[plane_no].bytesused; > + return vb->planes[plane_no].bytesused; > return 0; > } > > @@ -596,7 +598,7 @@ static inline unsigned long > vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no) > { > if (plane_no < vb->num_planes) > - return vb->v4l2_planes[plane_no].length; > + return vb->planes[plane_no].length; > return 0; > } > > diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h > index d4a4d9a..fc2dbe9 100644 > --- a/include/media/videobuf2-v4l2.h > +++ b/include/media/videobuf2-v4l2.h > @@ -12,6 +12,32 @@ > #ifndef _MEDIA_VIDEOBUF2_V4L2_H > #define _MEDIA_VIDEOBUF2_V4L2_H > > +#include <linux/videodev2.h> > #include <media/videobuf2-core.h> > > +/** > + * struct vb2_v4l2_buffer - video buffer information for v4l2 > + * @vb2_buf: video buffer 2 > + * @flags: buffer informational flags > + * @field: enum v4l2_field; field order of the image in the buffer > + * @timestamp: frame timestamp > + * @timecode: frame timecode > + * @sequence: sequence count of this frame > + */ > +struct vb2_v4l2_buffer { > + struct vb2_buffer vb2_buf; > + > + __u32 flags; > + __u32 field; > + struct timeval timestamp; > + struct v4l2_timecode timecode; > + __u32 sequence; > +}; > + > +/** > + * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer * > + */ > +#define to_vb2_v4l2_buffer(vb) \ > + (container_of(vb, struct vb2_v4l2_buffer, vb2_buf)) > + > #endif /* _MEDIA_VIDEOBUF2_V4L2_H */ Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Mauro, First of all, thank you for your detailed review. On 08/27/2015 07:28 PM, Mauro Carvalho Chehab wrote: > Em Wed, 26 Aug 2015 20:59:29 +0900 > Junghak Sung <jh1009.sung@samsung.com> escreveu: > >> Remove v4l2-specific stuff from struct vb2_buffer and add member variables >> related with buffer management. >> >> struct vb2_plane { >> <snip> >> /* plane information */ >> unsigned int bytesused; >> unsigned int length; >> union { >> unsigned int offset; >> unsigned long userptr; >> int fd; >> } m; >> unsigned int data_offset; >> } >> >> struct vb2_buffer { >> <snip> >> /* buffer information */ >> unsigned int num_planes; >> unsigned int index; >> unsigned int type; >> unsigned int memory; >> >> struct vb2_plane planes[VIDEO_MAX_PLANES]; >> <snip> >> }; >> >> And create struct vb2_v4l2_buffer as container buffer for v4l2 use. >> >> struct vb2_v4l2_buffer { >> struct vb2_buffer vb2_buf; >> >> __u32 flags; >> __u32 field; >> struct timeval timestamp; >> struct v4l2_timecode timecode; >> __u32 sequence; >> }; > > The comments seemed a little hard for me to read, but the changes > look ok. > Ok, I will modify these comments more clearly at next round. > I made some comments mostly regarding to documentation. See below. > >> This patch includes only changes inside of videobuf2. So, it is required to >> modify all device drivers which use videobuf2. > > So, in practice, we need to fold both patches 2 and 3 when merging upstream, > to avoid breaking git bisectability. > I'm sorry, but I can not understand the meaning of "fold both patches". Would you please explain more detailed what should I do at next round. If these two patches are get together to one patch, the size will be over 300KB, which causes a size problem to send it to ML. >> >> 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 | 324 +++++++++++++++++------------- >> include/media/videobuf2-core.h | 50 ++--- >> include/media/videobuf2-v4l2.h | 26 +++ >> 3 files changed, 236 insertions(+), 164 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index ab00ea0..9266d50 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -35,10 +35,10 @@ >> static int debug; >> module_param(debug, int, 0644); >> >> -#define dprintk(level, fmt, arg...) \ >> - do { \ >> - if (debug >= level) \ >> - pr_info("vb2: %s: " fmt, __func__, ## arg); \ >> +#define dprintk(level, fmt, arg...) \ >> + do { \ >> + if (debug >= level) \ >> + pr_info("vb2: %s: " fmt, __func__, ## arg); \ >> } while (0) >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> @@ -53,7 +53,7 @@ module_param(debug, int, 0644); >> >> #define log_memop(vb, op) \ >> dprintk(2, "call_memop(%p, %d, %s)%s\n", \ >> - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ >> + (vb)->vb2_queue, (vb)->index, #op, \ >> (vb)->vb2_queue->mem_ops->op ? "" : " (nop)") >> >> #define call_memop(vb, op, args...) \ >> @@ -115,7 +115,7 @@ module_param(debug, int, 0644); >> >> #define log_vb_qop(vb, op, args...) \ >> dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \ >> - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ >> + (vb)->vb2_queue, (vb)->index, #op, \ >> (vb)->vb2_queue->ops->op ? "" : " (nop)") >> >> #define call_vb_qop(vb, op, args...) \ >> @@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) >> unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]); >> >> mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane], >> - size, dma_dir, q->gfp_flags); >> + size, dma_dir, q->gfp_flags); >> if (IS_ERR_OR_NULL(mem_priv)) >> goto free; >> >> /* Associate allocator private data with this plane */ >> vb->planes[plane].mem_priv = mem_priv; >> - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; >> + vb->planes[plane].length = q->plane_sizes[plane]; >> } >> >> return 0; >> @@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb) >> for (plane = 0; plane < vb->num_planes; ++plane) { >> call_void_memop(vb, put, vb->planes[plane].mem_priv); >> vb->planes[plane].mem_priv = NULL; >> - dprintk(3, "freed plane %d of buffer %d\n", plane, >> - vb->v4l2_buf.index); >> + dprintk(3, "freed plane %d of buffer %d\n", plane, vb->index); >> } >> } >> >> @@ -269,7 +268,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) >> >> call_void_memop(vb, detach_dmabuf, p->mem_priv); >> dma_buf_put(p->dbuf); >> - memset(p, 0, sizeof(*p)); >> + p->mem_priv = NULL; >> + p->dbuf = NULL; >> + p->dbuf_mapped = 0; >> } >> >> /** >> @@ -299,7 +300,7 @@ static void __setup_lengths(struct vb2_queue *q, unsigned int n) >> continue; >> >> for (plane = 0; plane < vb->num_planes; ++plane) >> - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; >> + vb->planes[plane].length = q->plane_sizes[plane]; >> } >> } >> >> @@ -314,10 +315,10 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) >> unsigned long off; >> >> if (q->num_buffers) { >> - struct v4l2_plane *p; >> + struct vb2_plane *p; >> vb = q->bufs[q->num_buffers - 1]; >> - p = &vb->v4l2_planes[vb->num_planes - 1]; >> - off = PAGE_ALIGN(p->m.mem_offset + p->length); >> + p = &vb->planes[vb->num_planes - 1]; >> + off = PAGE_ALIGN(p->m.offset + p->length); >> } else { >> off = 0; >> } >> @@ -328,12 +329,12 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) >> continue; >> >> for (plane = 0; plane < vb->num_planes; ++plane) { >> - vb->v4l2_planes[plane].m.mem_offset = off; >> + vb->planes[plane].m.offset = off; >> >> dprintk(3, "buffer %d, plane %d offset 0x%08lx\n", >> buffer, plane, off); >> >> - off += vb->v4l2_planes[plane].length; >> + off += vb->planes[plane].length; >> off = PAGE_ALIGN(off); >> } >> } >> @@ -347,7 +348,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) >> * 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 int num_buffers, unsigned int num_planes) >> { >> unsigned int buffer; >> struct vb2_buffer *vb; >> @@ -361,16 +362,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, >> 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 = q->num_buffers + buffer; >> - vb->v4l2_buf.type = q->type; >> - vb->v4l2_buf.memory = memory; >> + vb->index = q->num_buffers + buffer; >> + vb->type = q->type; >> + vb->memory = memory; >> >> /* Allocate video buffer memory for the MMAP type */ >> if (memory == V4L2_MEMORY_MMAP) { >> @@ -418,7 +415,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers) >> struct vb2_buffer *vb; >> >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; >> - ++buffer) { >> + ++buffer) { >> vb = q->bufs[buffer]; >> if (!vb) >> continue; >> @@ -451,7 +448,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >> * just return -EAGAIN. >> */ >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; >> - ++buffer) { >> + ++buffer) { >> if (q->bufs[buffer] == NULL) >> continue; >> if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) { >> @@ -462,7 +459,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >> >> /* Call driver-provided cleanup function for each buffer, if provided */ >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; >> - ++buffer) { >> + ++buffer) { >> struct vb2_buffer *vb = q->bufs[buffer]; >> >> if (vb && vb->planes[0].mem_priv) >> @@ -536,7 +533,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) >> >> /* Free videobuf buffers */ >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; >> - ++buffer) { >> + ++buffer) { >> kfree(q->bufs[buffer]); >> q->bufs[buffer] = NULL; >> } >> @@ -590,23 +587,22 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { >> for (plane = 0; plane < vb->num_planes; ++plane) { >> length = (b->memory == V4L2_MEMORY_USERPTR || >> - b->memory == V4L2_MEMORY_DMABUF) >> - ? b->m.planes[plane].length >> - : vb->v4l2_planes[plane].length; >> + b->memory == V4L2_MEMORY_DMABUF) >> + ? b->m.planes[plane].length >> + : vb->planes[plane].length; >> bytesused = b->m.planes[plane].bytesused >> - ? b->m.planes[plane].bytesused : length; >> + ? b->m.planes[plane].bytesused : length; >> >> if (b->m.planes[plane].bytesused > length) >> return -EINVAL; >> >> if (b->m.planes[plane].data_offset > 0 && >> - b->m.planes[plane].data_offset >= bytesused) >> + b->m.planes[plane].data_offset >= bytesused) >> return -EINVAL; >> } >> } else { >> length = (b->memory == V4L2_MEMORY_USERPTR) >> - ? b->length : vb->v4l2_planes[0].length; >> - bytesused = b->bytesused ? b->bytesused : length; >> + ? b->length : vb->planes[0].length; >> >> if (b->bytesused > length) >> return -EINVAL; >> @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) >> */ >> static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> { >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> struct vb2_queue *q = vb->vb2_queue; >> + unsigned int plane; >> >> /* Copy back data such as timestamp, flags, etc. */ >> - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); >> - b->reserved2 = vb->v4l2_buf.reserved2; >> - b->reserved = vb->v4l2_buf.reserved; >> + b->index = vb->index; >> + b->type = vb->type; >> + b->memory = vb->memory; >> + b->bytesused = 0; >> + >> + b->flags = vbuf->flags; >> + b->field = vbuf->field; >> + b->timestamp = vbuf->timestamp; >> + b->timecode = vbuf->timecode; >> + b->sequence = vbuf->sequence; >> >> if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { >> /* >> @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> * for it. The caller has already verified memory and size. >> */ >> b->length = vb->num_planes; >> - memcpy(b->m.planes, vb->v4l2_planes, >> - b->length * sizeof(struct v4l2_plane)); >> + for (plane = 0; plane < vb->num_planes; ++plane) { >> + struct v4l2_plane *pdst = &b->m.planes[plane]; >> + struct vb2_plane *psrc = &vb->planes[plane]; >> + >> + pdst->bytesused = psrc->bytesused; >> + pdst->length = psrc->length; >> + if (q->memory == V4L2_MEMORY_MMAP) >> + pdst->m.mem_offset = psrc->m.offset; >> + else if (q->memory == V4L2_MEMORY_USERPTR) >> + pdst->m.userptr = psrc->m.userptr; >> + else if (q->memory == V4L2_MEMORY_DMABUF) >> + pdst->m.fd = psrc->m.fd; >> + pdst->data_offset = psrc->data_offset; >> + } >> } else { >> /* >> * We use length and offset in v4l2_planes array even for >> * single-planar buffers, but userspace does not. >> */ >> - b->length = vb->v4l2_planes[0].length; >> - b->bytesused = vb->v4l2_planes[0].bytesused; >> + b->length = vb->planes[0].length; >> + b->bytesused = vb->planes[0].bytesused; >> if (q->memory == V4L2_MEMORY_MMAP) >> - b->m.offset = vb->v4l2_planes[0].m.mem_offset; >> + b->m.offset = vb->planes[0].m.offset; >> else if (q->memory == V4L2_MEMORY_USERPTR) >> - b->m.userptr = vb->v4l2_planes[0].m.userptr; >> + b->m.userptr = vb->planes[0].m.userptr; >> else if (q->memory == V4L2_MEMORY_DMABUF) >> - b->m.fd = vb->v4l2_planes[0].m.fd; >> + b->m.fd = vb->planes[0].m.fd; >> } >> >> /* >> @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> b->flags &= ~V4L2_BUFFER_MASK_FLAGS; >> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; >> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != >> - V4L2_BUF_FLAG_TIMESTAMP_COPY) { >> + V4L2_BUF_FLAG_TIMESTAMP_COPY) { >> /* >> * For non-COPY timestamps, drop timestamp source bits >> * and obtain the timestamp source from the queue. >> @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); >> static int __verify_userptr_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || >> - !q->mem_ops->put_userptr) >> + !q->mem_ops->put_userptr) >> return -EINVAL; >> >> return 0; >> @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) >> static int __verify_mmap_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || >> - !q->mem_ops->put || !q->mem_ops->mmap) >> + !q->mem_ops->put || !q->mem_ops->mmap) >> return -EINVAL; >> >> return 0; >> @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) >> static int __verify_dmabuf_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || >> - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >> - !q->mem_ops->unmap_dmabuf) >> + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >> + !q->mem_ops->unmap_dmabuf) >> return -EINVAL; >> >> return 0; >> @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, >> enum v4l2_memory memory, enum v4l2_buf_type type) >> { >> if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && >> - memory != V4L2_MEMORY_DMABUF) { >> + memory != V4L2_MEMORY_DMABUF) { >> dprintk(1, "unsupported memory type\n"); >> return -EINVAL; >> } >> @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) >> * Driver also sets the size and allocator context for each plane. >> */ >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, >> - q->plane_sizes, q->alloc_ctx); >> + q->plane_sizes, q->alloc_ctx); >> if (ret) >> return ret; >> >> @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) >> num_buffers = allocated_buffers; >> >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> >> if (!ret && allocated_buffers < num_buffers) >> ret = -ENOMEM; >> @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create >> * buffer and their sizes are acceptable >> */ >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> if (ret) >> return ret; >> >> @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create >> * queue driver has set up >> */ >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> >> if (!ret && allocated_buffers < num_buffers) >> ret = -ENOMEM; >> @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> return; >> >> if (WARN_ON(state != VB2_BUF_STATE_DONE && >> - state != VB2_BUF_STATE_ERROR && >> - state != VB2_BUF_STATE_QUEUED)) >> + state != VB2_BUF_STATE_ERROR && >> + state != VB2_BUF_STATE_QUEUED)) >> state = VB2_BUF_STATE_ERROR; >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> @@ -1195,7 +1212,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> vb->cnt_buf_done++; >> #endif >> dprintk(4, "done processing on buffer %d, state: %d\n", >> - vb->v4l2_buf.index, state); >> + vb->index, state); >> >> /* sync buffers */ >> for (plane = 0; plane < vb->num_planes; ++plane) >> @@ -1268,25 +1285,26 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer *vb) >> * v4l2_buffer by the userspace. The caller has already verified that struct >> * v4l2_buffer has a valid number of planes. >> */ >> -static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b, >> - struct v4l2_plane *v4l2_planes) >> +static void __fill_vb2_buffer(struct vb2_buffer *vb, >> + const struct v4l2_buffer *b, struct vb2_plane *planes) >> { >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> unsigned int plane; >> >> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { >> if (b->memory == V4L2_MEMORY_USERPTR) { >> for (plane = 0; plane < vb->num_planes; ++plane) { >> - v4l2_planes[plane].m.userptr = >> + planes[plane].m.userptr = >> b->m.planes[plane].m.userptr; >> - v4l2_planes[plane].length = >> + planes[plane].length = >> b->m.planes[plane].length; >> } >> } >> if (b->memory == V4L2_MEMORY_DMABUF) { >> for (plane = 0; plane < vb->num_planes; ++plane) { >> - v4l2_planes[plane].m.fd = >> + planes[plane].m.fd = >> b->m.planes[plane].m.fd; >> - v4l2_planes[plane].length = >> + planes[plane].length = >> b->m.planes[plane].length; >> } >> } >> @@ -1310,7 +1328,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b >> * applications working. >> */ >> for (plane = 0; plane < vb->num_planes; ++plane) { >> - struct v4l2_plane *pdst = &v4l2_planes[plane]; >> + struct vb2_plane *pdst = &planes[plane]; >> struct v4l2_plane *psrc = &b->m.planes[plane]; >> >> if (psrc->bytesused == 0) >> @@ -1340,13 +1358,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b >> * old userspace applications working. >> */ >> if (b->memory == V4L2_MEMORY_USERPTR) { >> - v4l2_planes[0].m.userptr = b->m.userptr; >> - v4l2_planes[0].length = b->length; >> + planes[0].m.userptr = b->m.userptr; >> + planes[0].length = b->length; >> } >> >> if (b->memory == V4L2_MEMORY_DMABUF) { >> - v4l2_planes[0].m.fd = b->m.fd; >> - v4l2_planes[0].length = b->length; >> + planes[0].m.fd = b->m.fd; >> + planes[0].length = b->length; >> } >> >> if (V4L2_TYPE_IS_OUTPUT(b->type)) { >> @@ -1354,25 +1372,26 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b >> vb2_warn_zero_bytesused(vb); >> >> if (vb->vb2_queue->allow_zero_bytesused) >> - v4l2_planes[0].bytesused = b->bytesused; >> + planes[0].bytesused = b->bytesused; >> else >> - v4l2_planes[0].bytesused = b->bytesused ? >> - b->bytesused : v4l2_planes[0].length; >> + planes[0].bytesused = b->bytesused ? >> + b->bytesused : planes[0].length; >> } else >> - v4l2_planes[0].bytesused = 0; >> + planes[0].bytesused = 0; >> >> } >> >> /* Zero flags that the vb2 core handles */ >> - vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; >> + vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; >> if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != >> - V4L2_BUF_FLAG_TIMESTAMP_COPY || !V4L2_TYPE_IS_OUTPUT(b->type)) { >> + V4L2_BUF_FLAG_TIMESTAMP_COPY || >> + !V4L2_TYPE_IS_OUTPUT(b->type)) { >> /* >> * Non-COPY timestamps and non-OUTPUT queues will get >> * their timestamp and timestamp source flags from the >> * queue. >> */ >> - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; >> + vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; >> } >> >> if (V4L2_TYPE_IS_OUTPUT(b->type)) { >> @@ -1382,11 +1401,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b >> * The 'field' is valid metadata for this output buffer >> * and so that needs to be copied here. >> */ >> - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; >> - vb->v4l2_buf.field = b->field; >> + vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE; >> + vbuf->field = b->field; >> } else { >> /* Zero any output buffer flags as this is a capture buffer */ >> - vb->v4l2_buf.flags &= ~V4L2_BUFFER_OUT_FLAGS; >> + vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS; >> } >> } >> >> @@ -1395,7 +1414,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b >> */ >> static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> { >> - __fill_vb2_buffer(vb, b, vb->v4l2_planes); >> + __fill_vb2_buffer(vb, b, vb->planes); >> return call_vb_qop(vb, buf_prepare, vb); >> } >> >> @@ -1404,7 +1423,7 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> */ >> static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> { >> - struct v4l2_plane planes[VIDEO_MAX_PLANES]; >> + struct vb2_plane planes[VIDEO_MAX_PLANES]; >> struct vb2_queue *q = vb->vb2_queue; >> void *mem_priv; >> unsigned int plane; >> @@ -1419,9 +1438,9 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> >> for (plane = 0; plane < vb->num_planes; ++plane) { >> /* Skip the plane if already verified */ >> - if (vb->v4l2_planes[plane].m.userptr && >> - vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr >> - && vb->v4l2_planes[plane].length == planes[plane].length) >> + if (vb->planes[plane].m.userptr && >> + vb->planes[plane].m.userptr == planes[plane].m.userptr >> + && vb->planes[plane].length == planes[plane].length) >> continue; >> >> dprintk(3, "userspace address for plane %d changed, " >> @@ -1447,12 +1466,15 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> } >> >> vb->planes[plane].mem_priv = NULL; >> - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); >> + vb->planes[plane].bytesused = 0; >> + vb->planes[plane].length = 0; >> + vb->planes[plane].m.userptr = 0; >> + vb->planes[plane].data_offset = 0; >> >> /* Acquire each plane's memory */ >> mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane], >> - planes[plane].m.userptr, >> - planes[plane].length, dma_dir); >> + planes[plane].m.userptr, >> + planes[plane].length, dma_dir); >> if (IS_ERR_OR_NULL(mem_priv)) { >> dprintk(1, "failed acquiring userspace " >> "memory for plane %d\n", plane); >> @@ -1466,8 +1488,12 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> * Now that everything is in order, copy relevant information >> * provided by userspace. >> */ >> - for (plane = 0; plane < vb->num_planes; ++plane) >> - vb->v4l2_planes[plane] = planes[plane]; >> + for (plane = 0; plane < vb->num_planes; ++plane) { >> + vb->planes[plane].bytesused = planes[plane].bytesused; >> + vb->planes[plane].length = planes[plane].length; >> + vb->planes[plane].m.userptr = planes[plane].m.userptr; >> + vb->planes[plane].data_offset = planes[plane].data_offset; >> + } >> >> if (reacquired) { >> /* >> @@ -1494,10 +1520,11 @@ err: >> /* In case of errors, release planes that were already acquired */ >> for (plane = 0; plane < vb->num_planes; ++plane) { >> if (vb->planes[plane].mem_priv) >> - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); >> + call_void_memop(vb, put_userptr, >> + vb->planes[plane].mem_priv); >> vb->planes[plane].mem_priv = NULL; >> - vb->v4l2_planes[plane].m.userptr = 0; >> - vb->v4l2_planes[plane].length = 0; >> + vb->planes[plane].m.userptr = 0; >> + vb->planes[plane].length = 0; >> } >> >> return ret; >> @@ -1508,7 +1535,7 @@ err: >> */ >> static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> { >> - struct v4l2_plane planes[VIDEO_MAX_PLANES]; >> + struct vb2_plane planes[VIDEO_MAX_PLANES]; >> struct vb2_queue *q = vb->vb2_queue; >> void *mem_priv; >> unsigned int plane; >> @@ -1544,7 +1571,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> >> /* Skip the plane if already verified */ >> if (dbuf == vb->planes[plane].dbuf && >> - vb->v4l2_planes[plane].length == planes[plane].length) { >> + vb->planes[plane].length == planes[plane].length) { >> dma_buf_put(dbuf); >> continue; >> } >> @@ -1558,11 +1585,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> >> /* Release previously acquired memory if present */ >> __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); >> - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); >> + vb->planes[plane].bytesused = 0; >> + vb->planes[plane].length = 0; >> + vb->planes[plane].m.fd = 0; >> + vb->planes[plane].data_offset = 0; >> >> /* Acquire each plane's memory */ >> - mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane], >> - dbuf, planes[plane].length, dma_dir); >> + mem_priv = call_ptr_memop(vb, attach_dmabuf, >> + q->alloc_ctx[plane], dbuf, planes[plane].length, >> + dma_dir); >> if (IS_ERR(mem_priv)) { >> dprintk(1, "failed to attach dmabuf\n"); >> ret = PTR_ERR(mem_priv); >> @@ -1592,8 +1623,12 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> * Now that everything is in order, copy relevant information >> * provided by userspace. >> */ >> - for (plane = 0; plane < vb->num_planes; ++plane) >> - vb->v4l2_planes[plane] = planes[plane]; >> + for (plane = 0; plane < vb->num_planes; ++plane) { >> + vb->planes[plane].bytesused = planes[plane].bytesused; >> + vb->planes[plane].length = planes[plane].length; >> + vb->planes[plane].m.fd = planes[plane].m.userptr; >> + vb->planes[plane].data_offset = planes[plane].data_offset; >> + } >> >> if (reacquired) { >> /* >> @@ -1644,6 +1679,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) >> >> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> { >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> struct vb2_queue *q = vb->vb2_queue; >> int ret; >> >> @@ -1672,9 +1708,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> } >> >> vb->state = VB2_BUF_STATE_PREPARING; >> - vb->v4l2_buf.timestamp.tv_sec = 0; >> - vb->v4l2_buf.timestamp.tv_usec = 0; >> - vb->v4l2_buf.sequence = 0; >> + vbuf->timestamp.tv_sec = 0; >> + vbuf->timestamp.tv_usec = 0; >> + vbuf->sequence = 0; >> >> switch (q->memory) { >> case V4L2_MEMORY_MMAP: >> @@ -1701,7 +1737,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) >> } >> >> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b, >> - const char *opname) >> + const char *opname) >> { >> if (b->type != q->type) { >> dprintk(1, "%s: invalid buffer type\n", opname); >> @@ -1768,7 +1804,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) >> /* Fill buffer information for the userspace */ >> __fill_v4l2_buffer(vb, b); >> >> - dprintk(1, "prepare of buffer %d succeeded\n", vb->v4l2_buf.index); >> + dprintk(1, "prepare of buffer %d succeeded\n", vb->index); >> } >> return ret; >> } >> @@ -1800,7 +1836,7 @@ static int vb2_start_streaming(struct vb2_queue *q) >> /* Tell the driver to start streaming */ >> q->start_streaming_called = 1; >> ret = call_qop(q, start_streaming, q, >> - atomic_read(&q->owned_by_drv_count)); >> + atomic_read(&q->owned_by_drv_count)); >> if (!ret) >> return 0; >> >> @@ -1810,7 +1846,7 @@ static int vb2_start_streaming(struct vb2_queue *q) >> /* >> * If you see this warning, then the driver isn't cleaning up properly >> * after a failed start_streaming(). See the start_streaming() >> - * documentation in videobuf2-v4l2.h for more information how buffers >> + * documentation in videobuf2-core.h for more information how buffers >> * should be returned to vb2 in start_streaming(). >> */ >> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { >> @@ -1841,11 +1877,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) >> { >> int ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); >> struct vb2_buffer *vb; >> + struct vb2_v4l2_buffer *vbuf; >> >> if (ret) >> return ret; >> >> vb = q->bufs[b->index]; >> + vbuf = to_vb2_v4l2_buffer(vb); >> >> switch (vb->state) { >> case VB2_BUF_STATE_DEQUEUED: >> @@ -1877,11 +1915,11 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) >> * and the timecode field and flag if needed. >> */ >> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) == >> - V4L2_BUF_FLAG_TIMESTAMP_COPY) >> - vb->v4l2_buf.timestamp = b->timestamp; >> - vb->v4l2_buf.flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; >> + V4L2_BUF_FLAG_TIMESTAMP_COPY) >> + vbuf->timestamp = b->timestamp; >> + vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; >> if (b->flags & V4L2_BUF_FLAG_TIMECODE) >> - vb->v4l2_buf.timecode = b->timecode; >> + vbuf->timecode = b->timecode; >> } >> >> trace_vb2_qbuf(q, vb); >> @@ -1903,13 +1941,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) >> * then we can finally call start_streaming(). >> */ >> if (q->streaming && !q->start_streaming_called && >> - q->queued_count >= q->min_buffers_needed) { >> + q->queued_count >= q->min_buffers_needed) { >> ret = vb2_start_streaming(q); >> if (ret) >> return ret; >> } >> >> - dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index); >> + dprintk(1, "qbuf of buffer %d succeeded\n", vb->index); >> return 0; >> } >> >> @@ -2099,9 +2137,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb) >> } >> } >> >> -static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) >> +static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, >> + bool nonblocking) >> { >> struct vb2_buffer *vb = NULL; >> + struct vb2_v4l2_buffer *vbuf = NULL; >> int ret; >> >> if (b->type != q->type) { >> @@ -2134,14 +2174,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n >> >> trace_vb2_dqbuf(q, vb); >> >> + vbuf = to_vb2_v4l2_buffer(vb); >> if (!V4L2_TYPE_IS_OUTPUT(q->type) && >> - vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST) >> + vbuf->flags & V4L2_BUF_FLAG_LAST) >> q->last_buffer_dequeued = true; >> /* go back to dequeued state */ >> __vb2_dqbuf(vb); >> >> dprintk(1, "dqbuf of buffer %d, with state %d\n", >> - vb->v4l2_buf.index, vb->state); >> + vb->index, vb->state); >> >> return 0; >> } >> @@ -2197,7 +2238,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) >> /* >> * If you see this warning, then the driver isn't cleaning up properly >> * in stop_streaming(). See the stop_streaming() documentation in >> - * videobuf2-v4l2.h for more information how buffers should be returned >> + * videobuf2-core.h for more information how buffers should be returned >> * to vb2 in stop_streaming(). >> */ >> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { >> @@ -2399,7 +2440,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, >> vb = q->bufs[buffer]; >> >> for (plane = 0; plane < vb->num_planes; ++plane) { >> - if (vb->v4l2_planes[plane].m.mem_offset == off) { >> + if (vb->planes[plane].m.offset == off) { >> *_buffer = buffer; >> *_plane = plane; >> return 0; >> @@ -2557,7 +2598,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) >> * The buffer length was page_aligned at __vb2_buf_mem_alloc(), >> * so, we need to do the same here. >> */ >> - length = PAGE_ALIGN(vb->v4l2_planes[plane].length); >> + length = PAGE_ALIGN(vb->planes[plane].length); >> if (length < (vma->vm_end - vma->vm_start)) { >> dprintk(1, >> "MMAP invalid, as it would overflow buffer length\n"); >> @@ -2577,10 +2618,10 @@ EXPORT_SYMBOL_GPL(vb2_mmap); >> >> #ifndef CONFIG_MMU >> unsigned long vb2_get_unmapped_area(struct vb2_queue *q, >> - unsigned long addr, >> - unsigned long len, >> - unsigned long pgoff, >> - unsigned long flags) >> + unsigned long addr, >> + unsigned long len, >> + unsigned long pgoff, >> + unsigned long flags) >> { >> unsigned long off = pgoff << PAGE_SHIFT; >> struct vb2_buffer *vb; >> @@ -2731,7 +2772,7 @@ EXPORT_SYMBOL_GPL(vb2_poll); >> * responsible of clearing it's content and setting initial values for some >> * required entries before calling this function. >> * q->ops, q->mem_ops, q->type and q->io_modes are mandatory. Please refer >> - * to the struct vb2_queue description in include/media/videobuf2-v4l2.h >> + * to the struct vb2_queue description in include/media/videobuf2-core.h >> * for more information. >> */ >> int vb2_queue_init(struct vb2_queue *q) >> @@ -2739,16 +2780,16 @@ int vb2_queue_init(struct vb2_queue *q) >> /* >> * Sanity check >> */ >> - if (WARN_ON(!q) || >> - WARN_ON(!q->ops) || >> - WARN_ON(!q->mem_ops) || >> - WARN_ON(!q->type) || >> - WARN_ON(!q->io_modes) || >> - WARN_ON(!q->ops->queue_setup) || >> - WARN_ON(!q->ops->buf_queue) || >> - WARN_ON(q->timestamp_flags & >> - ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | >> - V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) >> + if (WARN_ON(!q) || >> + WARN_ON(!q->ops) || >> + WARN_ON(!q->mem_ops) || >> + WARN_ON(!q->type) || >> + WARN_ON(!q->io_modes) || >> + WARN_ON(!q->ops->queue_setup) || >> + WARN_ON(!q->ops->buf_queue) || >> + WARN_ON(q->timestamp_flags & >> + ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | >> + V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) >> return -EINVAL; >> >> /* Warn that the driver should choose an appropriate timestamp type */ >> @@ -2852,7 +2893,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) >> * Sanity check >> */ >> if (WARN_ON((read && !(q->io_modes & VB2_READ)) || >> - (!read && !(q->io_modes & VB2_WRITE)))) >> + (!read && !(q->io_modes & VB2_WRITE)))) >> return -EINVAL; >> >> /* >> @@ -3063,9 +3104,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ >> buf->queued = 0; >> buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0) >> : vb2_plane_size(q->bufs[index], 0); >> - /* Compensate for data_offset on read in the multiplanar case. */ >> + /* >> + * Compensate for data_offset on read >> + * in the multiplanar case >> + */ >> if (is_multiplanar && read && >> - fileio->b.m.planes[0].data_offset < buf->size) { >> + fileio->b.m.planes[0].data_offset < buf->size) { >> buf->pos = fileio->b.m.planes[0].data_offset; >> buf->size -= buf->pos; >> } >> @@ -3257,7 +3301,7 @@ static int vb2_thread(void *data) >> * contact the linux-media mailinglist first. >> */ >> int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv, >> - const char *thread_name) >> + const char *thread_name) >> { >> struct vb2_threadio_data *threadio; >> int ret = 0; >> @@ -3491,7 +3535,7 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf, >> if (vb2_queue_is_busy(vdev, file)) >> goto exit; >> err = vb2_write(vdev->queue, buf, count, ppos, >> - file->f_flags & O_NONBLOCK); >> + file->f_flags & O_NONBLOCK); >> if (vdev->queue->fileio) >> vdev->queue->owner = file->private_data; >> exit: >> @@ -3515,7 +3559,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf, >> if (vb2_queue_is_busy(vdev, file)) >> goto exit; >> err = vb2_read(vdev->queue, buf, count, ppos, >> - file->f_flags & O_NONBLOCK); >> + file->f_flags & O_NONBLOCK); >> if (vdev->queue->fileio) >> vdev->queue->owner = file->private_data; >> exit: >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h >> index 155991e..8787a6c 100644 >> --- a/include/media/videobuf2-core.h >> +++ b/include/media/videobuf2-core.h >> @@ -115,6 +115,16 @@ struct vb2_plane { >> void *mem_priv; >> struct dma_buf *dbuf; >> unsigned int dbuf_mapped; >> + >> + /* plane information */ >> + unsigned int bytesused; >> + unsigned int length; >> + union { >> + unsigned int offset; >> + unsigned long userptr; >> + int fd; >> + } m; >> + unsigned int data_offset; >> }; > > Nitpick: it would be good to add a documentation for struct vb2_plane, > describing what each field means on this struct. Granted, this could > be added after this patch series. > > Btw, I don't see much reason to have the: > /* plane information */ > comment here, as this struct is all about plane information, right? > Or, maybe you wanted, instead, to comment that those fields should > have what's there at struct v4l2_plane? That would make sense ;) Latter is right. > So, I would change this comment to something like: > > /* > * Should contain enough plane information to contain the > * fields needed to fill struct v4l2_plane at videodev2.h > */ > OK, I will change it. > >> >> /** >> @@ -161,41 +171,33 @@ 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 >> - * @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 >> + * @index: id number of the buffer >> + * @type: buffer type >> + * @memory: the method, in which the actual data is passed >> + * @num_planes: number of planes in the buffer >> + * on an internal driver queue >> * @planes: private per-plane information; do not change >> */ >> struct vb2_buffer { >> - struct v4l2_buffer v4l2_buf; >> - struct v4l2_plane v4l2_planes[VIDEO_MAX_PLANES]; >> - >> struct vb2_queue *vb2_queue; >> >> - unsigned int num_planes; >> - >> -/* Private: internal use only */ >> + /* Private: internal use only */ >> enum vb2_buffer_state state; >> >> struct list_head queued_entry; >> struct list_head done_entry; >> >> + /* buffer information */ >> + unsigned int index; >> + unsigned int type; >> + unsigned int memory; >> + unsigned int num_planes; > > Nitpick: Those comments that follow Documentation/kernel-doc-nano-HOWTO.txt > are used to produce DocBook data, on both html and manpages formats. > As documented there, DocBook discards documentation for all fields after a > /*private: ...*/ comment. > > In other words, we need to take care of putting things after /*private:*/ > only when we're 100% sure that such fields are not meant to be filled/used > by the callers, and will be used only internally, and don't deserve any > documentation for the kABI. > > As you're adding documentation for those fields, and num_planes were > documented before your series, I suspect that this is not what you want. > So, please move them to be before the private: comment. Oh, I didn't know that /*private: ...*/ comment have so much meaning. I will change it at next round. > > Btw, if your patches are based on top of the current patchwork tree > e. g. if it has this patch: > http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=d071c833a0d30e7aae0ea565d92ef83c79106d6f > > Then you can make the Kernel to handle all those kernel-doc-nano > comments with: > > make cleandocs && make DOCBOOKS=device-drivers.xml htmldocs > > It will produce some html pages like: > http://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/mediadev.html > > With can be useful to check if the documentation tags are properly placed. > >> struct vb2_plane planes[VIDEO_MAX_PLANES]; >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG >> @@ -390,7 +392,7 @@ struct v4l2_fh; >> * @threadio: thread io internal data, used only if thread is active >> */ >> struct vb2_queue { >> - enum v4l2_buf_type type; >> + unsigned int type; >> unsigned int io_modes; >> unsigned fileio_read_once:1; >> unsigned fileio_write_immediately:1; >> @@ -409,7 +411,7 @@ struct vb2_queue { >> >> /* private: internal use only */ >> struct mutex mmap_lock; >> - enum v4l2_memory memory; >> + unsigned int memory; > > Will the vb2-core use type/memory fields or just vb2-v4l2? As you > removed the enum, I suspect you won't be relying on having the videodev2.h > header included here, right. Exactly. > > If so, then the meaning of the type/memory fields are private to the > caller of the VB2-core (e. .g. the meaning are private to vb2-v4l2 and > vb2-dvb). So, you should be changing the description of those fields > at the doc-nano to: > ... > * @type: private type whose content is defined by the vb2-core caller. > * For example, for V4L2, it should match the V4L2_BUF_TYPE_* > * in include/uapi/linux/videodev2.h > ... > > to let it clear. > 0k, I will change it as your comment. >> struct vb2_buffer *bufs[VIDEO_MAX_FRAME]; >> unsigned int num_buffers; >> >> @@ -571,7 +573,7 @@ static inline void vb2_set_plane_payload(struct vb2_buffer *vb, >> unsigned int plane_no, unsigned long size) >> { >> if (plane_no < vb->num_planes) >> - vb->v4l2_planes[plane_no].bytesused = size; >> + vb->planes[plane_no].bytesused = size; >> } >> >> /** >> @@ -583,7 +585,7 @@ static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb, >> unsigned int plane_no) >> { >> if (plane_no < vb->num_planes) >> - return vb->v4l2_planes[plane_no].bytesused; >> + return vb->planes[plane_no].bytesused; >> return 0; >> } >> >> @@ -596,7 +598,7 @@ static inline unsigned long >> vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no) >> { >> if (plane_no < vb->num_planes) >> - return vb->v4l2_planes[plane_no].length; >> + return vb->planes[plane_no].length; >> return 0; >> } >> >> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h >> index d4a4d9a..fc2dbe9 100644 >> --- a/include/media/videobuf2-v4l2.h >> +++ b/include/media/videobuf2-v4l2.h >> @@ -12,6 +12,32 @@ >> #ifndef _MEDIA_VIDEOBUF2_V4L2_H >> #define _MEDIA_VIDEOBUF2_V4L2_H >> >> +#include <linux/videodev2.h> >> #include <media/videobuf2-core.h> >> >> +/** >> + * struct vb2_v4l2_buffer - video buffer information for v4l2 >> + * @vb2_buf: video buffer 2 >> + * @flags: buffer informational flags >> + * @field: enum v4l2_field; field order of the image in the buffer >> + * @timestamp: frame timestamp >> + * @timecode: frame timecode >> + * @sequence: sequence count of this frame >> + */ >> +struct vb2_v4l2_buffer { >> + struct vb2_buffer vb2_buf; >> + >> + __u32 flags; >> + __u32 field; >> + struct timeval timestamp; >> + struct v4l2_timecode timecode; >> + __u32 sequence; >> +}; >> + >> +/** >> + * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer * >> + */ >> +#define to_vb2_v4l2_buffer(vb) \ >> + (container_of(vb, struct vb2_v4l2_buffer, vb2_buf)) >> + >> #endif /* _MEDIA_VIDEOBUF2_V4L2_H */ > > Regards, > Mauro > > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Fri, 28 Aug 2015 10:26:48 +0900 Junghak Sung <jh1009.sung@samsung.com> escreveu: > > Hello Mauro, > > First of all, thank you for your detailed review. > > > On 08/27/2015 07:28 PM, Mauro Carvalho Chehab wrote: > > Em Wed, 26 Aug 2015 20:59:29 +0900 > > Junghak Sung <jh1009.sung@samsung.com> escreveu: > > > >> Remove v4l2-specific stuff from struct vb2_buffer and add member variables > >> related with buffer management. > >> > >> struct vb2_plane { > >> <snip> > >> /* plane information */ > >> unsigned int bytesused; > >> unsigned int length; > >> union { > >> unsigned int offset; > >> unsigned long userptr; > >> int fd; > >> } m; > >> unsigned int data_offset; > >> } > >> > >> struct vb2_buffer { > >> <snip> > >> /* buffer information */ > >> unsigned int num_planes; > >> unsigned int index; > >> unsigned int type; > >> unsigned int memory; > >> > >> struct vb2_plane planes[VIDEO_MAX_PLANES]; > >> <snip> > >> }; > >> > >> And create struct vb2_v4l2_buffer as container buffer for v4l2 use. > >> > >> struct vb2_v4l2_buffer { > >> struct vb2_buffer vb2_buf; > >> > >> __u32 flags; > >> __u32 field; > >> struct timeval timestamp; > >> struct v4l2_timecode timecode; > >> __u32 sequence; > >> }; > > > > The comments seemed a little hard for me to read, but the changes > > look ok. > > > > Ok, I will modify these comments more clearly at next round. > > > I made some comments mostly regarding to documentation. See below. > > > >> This patch includes only changes inside of videobuf2. So, it is required to > >> modify all device drivers which use videobuf2. > > > > So, in practice, we need to fold both patches 2 and 3 when merging upstream, > > to avoid breaking git bisectability. > > > > I'm sorry, but I can not understand the meaning of "fold both patches". > Would you please explain more detailed what should I do at next round. > If these two patches are get together to one patch, the size will > be over 300KB, which causes a size problem to send it to ML. No. What I'm meaning is that reviewers/(sub)maintainers need to be reminded that patches 2 and 3 should be merged together when the patches got applied, as otherwise they'll break the compilation. So, just add a notice on the next patch series, but keep them as two separate patches ;) Regards, Mauro > > >> > >> 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 | 324 +++++++++++++++++------------- > >> include/media/videobuf2-core.h | 50 ++--- > >> include/media/videobuf2-v4l2.h | 26 +++ > >> 3 files changed, 236 insertions(+), 164 deletions(-) > >> > >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > >> index ab00ea0..9266d50 100644 > >> --- a/drivers/media/v4l2-core/videobuf2-core.c > >> +++ b/drivers/media/v4l2-core/videobuf2-core.c > >> @@ -35,10 +35,10 @@ > >> static int debug; > >> module_param(debug, int, 0644); > >> > >> -#define dprintk(level, fmt, arg...) \ > >> - do { \ > >> - if (debug >= level) \ > >> - pr_info("vb2: %s: " fmt, __func__, ## arg); \ > >> +#define dprintk(level, fmt, arg...) \ > >> + do { \ > >> + if (debug >= level) \ > >> + pr_info("vb2: %s: " fmt, __func__, ## arg); \ > >> } while (0) > >> > >> #ifdef CONFIG_VIDEO_ADV_DEBUG > >> @@ -53,7 +53,7 @@ module_param(debug, int, 0644); > >> > >> #define log_memop(vb, op) \ > >> dprintk(2, "call_memop(%p, %d, %s)%s\n", \ > >> - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ > >> + (vb)->vb2_queue, (vb)->index, #op, \ > >> (vb)->vb2_queue->mem_ops->op ? "" : " (nop)") > >> > >> #define call_memop(vb, op, args...) \ > >> @@ -115,7 +115,7 @@ module_param(debug, int, 0644); > >> > >> #define log_vb_qop(vb, op, args...) \ > >> dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \ > >> - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ > >> + (vb)->vb2_queue, (vb)->index, #op, \ > >> (vb)->vb2_queue->ops->op ? "" : " (nop)") > >> > >> #define call_vb_qop(vb, op, args...) \ > >> @@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) > >> unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]); > >> > >> mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane], > >> - size, dma_dir, q->gfp_flags); > >> + size, dma_dir, q->gfp_flags); > >> if (IS_ERR_OR_NULL(mem_priv)) > >> goto free; > >> > >> /* Associate allocator private data with this plane */ > >> vb->planes[plane].mem_priv = mem_priv; > >> - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; > >> + vb->planes[plane].length = q->plane_sizes[plane]; > >> } > >> > >> return 0; > >> @@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb) > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> call_void_memop(vb, put, vb->planes[plane].mem_priv); > >> vb->planes[plane].mem_priv = NULL; > >> - dprintk(3, "freed plane %d of buffer %d\n", plane, > >> - vb->v4l2_buf.index); > >> + dprintk(3, "freed plane %d of buffer %d\n", plane, vb->index); > >> } > >> } > >> > >> @@ -269,7 +268,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) > >> > >> call_void_memop(vb, detach_dmabuf, p->mem_priv); > >> dma_buf_put(p->dbuf); > >> - memset(p, 0, sizeof(*p)); > >> + p->mem_priv = NULL; > >> + p->dbuf = NULL; > >> + p->dbuf_mapped = 0; > >> } > >> > >> /** > >> @@ -299,7 +300,7 @@ static void __setup_lengths(struct vb2_queue *q, unsigned int n) > >> continue; > >> > >> for (plane = 0; plane < vb->num_planes; ++plane) > >> - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; > >> + vb->planes[plane].length = q->plane_sizes[plane]; > >> } > >> } > >> > >> @@ -314,10 +315,10 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > >> unsigned long off; > >> > >> if (q->num_buffers) { > >> - struct v4l2_plane *p; > >> + struct vb2_plane *p; > >> vb = q->bufs[q->num_buffers - 1]; > >> - p = &vb->v4l2_planes[vb->num_planes - 1]; > >> - off = PAGE_ALIGN(p->m.mem_offset + p->length); > >> + p = &vb->planes[vb->num_planes - 1]; > >> + off = PAGE_ALIGN(p->m.offset + p->length); > >> } else { > >> off = 0; > >> } > >> @@ -328,12 +329,12 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > >> continue; > >> > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> - vb->v4l2_planes[plane].m.mem_offset = off; > >> + vb->planes[plane].m.offset = off; > >> > >> dprintk(3, "buffer %d, plane %d offset 0x%08lx\n", > >> buffer, plane, off); > >> > >> - off += vb->v4l2_planes[plane].length; > >> + off += vb->planes[plane].length; > >> off = PAGE_ALIGN(off); > >> } > >> } > >> @@ -347,7 +348,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) > >> * 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 int num_buffers, unsigned int num_planes) > >> { > >> unsigned int buffer; > >> struct vb2_buffer *vb; > >> @@ -361,16 +362,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, > >> 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 = q->num_buffers + buffer; > >> - vb->v4l2_buf.type = q->type; > >> - vb->v4l2_buf.memory = memory; > >> + vb->index = q->num_buffers + buffer; > >> + vb->type = q->type; > >> + vb->memory = memory; > >> > >> /* Allocate video buffer memory for the MMAP type */ > >> if (memory == V4L2_MEMORY_MMAP) { > >> @@ -418,7 +415,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers) > >> struct vb2_buffer *vb; > >> > >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > >> - ++buffer) { > >> + ++buffer) { > >> vb = q->bufs[buffer]; > >> if (!vb) > >> continue; > >> @@ -451,7 +448,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > >> * just return -EAGAIN. > >> */ > >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > >> - ++buffer) { > >> + ++buffer) { > >> if (q->bufs[buffer] == NULL) > >> continue; > >> if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) { > >> @@ -462,7 +459,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > >> > >> /* Call driver-provided cleanup function for each buffer, if provided */ > >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > >> - ++buffer) { > >> + ++buffer) { > >> struct vb2_buffer *vb = q->bufs[buffer]; > >> > >> if (vb && vb->planes[0].mem_priv) > >> @@ -536,7 +533,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) > >> > >> /* Free videobuf buffers */ > >> for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; > >> - ++buffer) { > >> + ++buffer) { > >> kfree(q->bufs[buffer]); > >> q->bufs[buffer] = NULL; > >> } > >> @@ -590,23 +587,22 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> length = (b->memory == V4L2_MEMORY_USERPTR || > >> - b->memory == V4L2_MEMORY_DMABUF) > >> - ? b->m.planes[plane].length > >> - : vb->v4l2_planes[plane].length; > >> + b->memory == V4L2_MEMORY_DMABUF) > >> + ? b->m.planes[plane].length > >> + : vb->planes[plane].length; > >> bytesused = b->m.planes[plane].bytesused > >> - ? b->m.planes[plane].bytesused : length; > >> + ? b->m.planes[plane].bytesused : length; > >> > >> if (b->m.planes[plane].bytesused > length) > >> return -EINVAL; > >> > >> if (b->m.planes[plane].data_offset > 0 && > >> - b->m.planes[plane].data_offset >= bytesused) > >> + b->m.planes[plane].data_offset >= bytesused) > >> return -EINVAL; > >> } > >> } else { > >> length = (b->memory == V4L2_MEMORY_USERPTR) > >> - ? b->length : vb->v4l2_planes[0].length; > >> - bytesused = b->bytesused ? b->bytesused : length; > >> + ? b->length : vb->planes[0].length; > >> > >> if (b->bytesused > length) > >> return -EINVAL; > >> @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) > >> */ > >> static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > >> { > >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > >> struct vb2_queue *q = vb->vb2_queue; > >> + unsigned int plane; > >> > >> /* Copy back data such as timestamp, flags, etc. */ > >> - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); > >> - b->reserved2 = vb->v4l2_buf.reserved2; > >> - b->reserved = vb->v4l2_buf.reserved; > >> + b->index = vb->index; > >> + b->type = vb->type; > >> + b->memory = vb->memory; > >> + b->bytesused = 0; > >> + > >> + b->flags = vbuf->flags; > >> + b->field = vbuf->field; > >> + b->timestamp = vbuf->timestamp; > >> + b->timecode = vbuf->timecode; > >> + b->sequence = vbuf->sequence; > >> > >> if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { > >> /* > >> @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > >> * for it. The caller has already verified memory and size. > >> */ > >> b->length = vb->num_planes; > >> - memcpy(b->m.planes, vb->v4l2_planes, > >> - b->length * sizeof(struct v4l2_plane)); > >> + for (plane = 0; plane < vb->num_planes; ++plane) { > >> + struct v4l2_plane *pdst = &b->m.planes[plane]; > >> + struct vb2_plane *psrc = &vb->planes[plane]; > >> + > >> + pdst->bytesused = psrc->bytesused; > >> + pdst->length = psrc->length; > >> + if (q->memory == V4L2_MEMORY_MMAP) > >> + pdst->m.mem_offset = psrc->m.offset; > >> + else if (q->memory == V4L2_MEMORY_USERPTR) > >> + pdst->m.userptr = psrc->m.userptr; > >> + else if (q->memory == V4L2_MEMORY_DMABUF) > >> + pdst->m.fd = psrc->m.fd; > >> + pdst->data_offset = psrc->data_offset; > >> + } > >> } else { > >> /* > >> * We use length and offset in v4l2_planes array even for > >> * single-planar buffers, but userspace does not. > >> */ > >> - b->length = vb->v4l2_planes[0].length; > >> - b->bytesused = vb->v4l2_planes[0].bytesused; > >> + b->length = vb->planes[0].length; > >> + b->bytesused = vb->planes[0].bytesused; > >> if (q->memory == V4L2_MEMORY_MMAP) > >> - b->m.offset = vb->v4l2_planes[0].m.mem_offset; > >> + b->m.offset = vb->planes[0].m.offset; > >> else if (q->memory == V4L2_MEMORY_USERPTR) > >> - b->m.userptr = vb->v4l2_planes[0].m.userptr; > >> + b->m.userptr = vb->planes[0].m.userptr; > >> else if (q->memory == V4L2_MEMORY_DMABUF) > >> - b->m.fd = vb->v4l2_planes[0].m.fd; > >> + b->m.fd = vb->planes[0].m.fd; > >> } > >> > >> /* > >> @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > >> b->flags &= ~V4L2_BUFFER_MASK_FLAGS; > >> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; > >> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != > >> - V4L2_BUF_FLAG_TIMESTAMP_COPY) { > >> + V4L2_BUF_FLAG_TIMESTAMP_COPY) { > >> /* > >> * For non-COPY timestamps, drop timestamp source bits > >> * and obtain the timestamp source from the queue. > >> @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); > >> static int __verify_userptr_ops(struct vb2_queue *q) > >> { > >> if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || > >> - !q->mem_ops->put_userptr) > >> + !q->mem_ops->put_userptr) > >> return -EINVAL; > >> > >> return 0; > >> @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) > >> static int __verify_mmap_ops(struct vb2_queue *q) > >> { > >> if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || > >> - !q->mem_ops->put || !q->mem_ops->mmap) > >> + !q->mem_ops->put || !q->mem_ops->mmap) > >> return -EINVAL; > >> > >> return 0; > >> @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) > >> static int __verify_dmabuf_ops(struct vb2_queue *q) > >> { > >> if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || > >> - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > >> - !q->mem_ops->unmap_dmabuf) > >> + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > >> + !q->mem_ops->unmap_dmabuf) > >> return -EINVAL; > >> > >> return 0; > >> @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, > >> enum v4l2_memory memory, enum v4l2_buf_type type) > >> { > >> if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && > >> - memory != V4L2_MEMORY_DMABUF) { > >> + memory != V4L2_MEMORY_DMABUF) { > >> dprintk(1, "unsupported memory type\n"); > >> return -EINVAL; > >> } > >> @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > >> * Driver also sets the size and allocator context for each plane. > >> */ > >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, > >> - q->plane_sizes, q->alloc_ctx); > >> + q->plane_sizes, q->alloc_ctx); > >> if (ret) > >> return ret; > >> > >> @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > >> num_buffers = allocated_buffers; > >> > >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, > >> - &num_planes, q->plane_sizes, q->alloc_ctx); > >> + &num_planes, q->plane_sizes, q->alloc_ctx); > >> > >> if (!ret && allocated_buffers < num_buffers) > >> ret = -ENOMEM; > >> @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > >> * buffer and their sizes are acceptable > >> */ > >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > >> - &num_planes, q->plane_sizes, q->alloc_ctx); > >> + &num_planes, q->plane_sizes, q->alloc_ctx); > >> if (ret) > >> return ret; > >> > >> @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > >> * queue driver has set up > >> */ > >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > >> - &num_planes, q->plane_sizes, q->alloc_ctx); > >> + &num_planes, q->plane_sizes, q->alloc_ctx); > >> > >> if (!ret && allocated_buffers < num_buffers) > >> ret = -ENOMEM; > >> @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > >> return; > >> > >> if (WARN_ON(state != VB2_BUF_STATE_DONE && > >> - state != VB2_BUF_STATE_ERROR && > >> - state != VB2_BUF_STATE_QUEUED)) > >> + state != VB2_BUF_STATE_ERROR && > >> + state != VB2_BUF_STATE_QUEUED)) > >> state = VB2_BUF_STATE_ERROR; > >> > >> #ifdef CONFIG_VIDEO_ADV_DEBUG > >> @@ -1195,7 +1212,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > >> vb->cnt_buf_done++; > >> #endif > >> dprintk(4, "done processing on buffer %d, state: %d\n", > >> - vb->v4l2_buf.index, state); > >> + vb->index, state); > >> > >> /* sync buffers */ > >> for (plane = 0; plane < vb->num_planes; ++plane) > >> @@ -1268,25 +1285,26 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer *vb) > >> * v4l2_buffer by the userspace. The caller has already verified that struct > >> * v4l2_buffer has a valid number of planes. > >> */ > >> -static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b, > >> - struct v4l2_plane *v4l2_planes) > >> +static void __fill_vb2_buffer(struct vb2_buffer *vb, > >> + const struct v4l2_buffer *b, struct vb2_plane *planes) > >> { > >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > >> unsigned int plane; > >> > >> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > >> if (b->memory == V4L2_MEMORY_USERPTR) { > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> - v4l2_planes[plane].m.userptr = > >> + planes[plane].m.userptr = > >> b->m.planes[plane].m.userptr; > >> - v4l2_planes[plane].length = > >> + planes[plane].length = > >> b->m.planes[plane].length; > >> } > >> } > >> if (b->memory == V4L2_MEMORY_DMABUF) { > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> - v4l2_planes[plane].m.fd = > >> + planes[plane].m.fd = > >> b->m.planes[plane].m.fd; > >> - v4l2_planes[plane].length = > >> + planes[plane].length = > >> b->m.planes[plane].length; > >> } > >> } > >> @@ -1310,7 +1328,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > >> * applications working. > >> */ > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> - struct v4l2_plane *pdst = &v4l2_planes[plane]; > >> + struct vb2_plane *pdst = &planes[plane]; > >> struct v4l2_plane *psrc = &b->m.planes[plane]; > >> > >> if (psrc->bytesused == 0) > >> @@ -1340,13 +1358,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > >> * old userspace applications working. > >> */ > >> if (b->memory == V4L2_MEMORY_USERPTR) { > >> - v4l2_planes[0].m.userptr = b->m.userptr; > >> - v4l2_planes[0].length = b->length; > >> + planes[0].m.userptr = b->m.userptr; > >> + planes[0].length = b->length; > >> } > >> > >> if (b->memory == V4L2_MEMORY_DMABUF) { > >> - v4l2_planes[0].m.fd = b->m.fd; > >> - v4l2_planes[0].length = b->length; > >> + planes[0].m.fd = b->m.fd; > >> + planes[0].length = b->length; > >> } > >> > >> if (V4L2_TYPE_IS_OUTPUT(b->type)) { > >> @@ -1354,25 +1372,26 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > >> vb2_warn_zero_bytesused(vb); > >> > >> if (vb->vb2_queue->allow_zero_bytesused) > >> - v4l2_planes[0].bytesused = b->bytesused; > >> + planes[0].bytesused = b->bytesused; > >> else > >> - v4l2_planes[0].bytesused = b->bytesused ? > >> - b->bytesused : v4l2_planes[0].length; > >> + planes[0].bytesused = b->bytesused ? > >> + b->bytesused : planes[0].length; > >> } else > >> - v4l2_planes[0].bytesused = 0; > >> + planes[0].bytesused = 0; > >> > >> } > >> > >> /* Zero flags that the vb2 core handles */ > >> - vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; > >> + vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; > >> if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != > >> - V4L2_BUF_FLAG_TIMESTAMP_COPY || !V4L2_TYPE_IS_OUTPUT(b->type)) { > >> + V4L2_BUF_FLAG_TIMESTAMP_COPY || > >> + !V4L2_TYPE_IS_OUTPUT(b->type)) { > >> /* > >> * Non-COPY timestamps and non-OUTPUT queues will get > >> * their timestamp and timestamp source flags from the > >> * queue. > >> */ > >> - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; > >> + vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; > >> } > >> > >> if (V4L2_TYPE_IS_OUTPUT(b->type)) { > >> @@ -1382,11 +1401,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > >> * The 'field' is valid metadata for this output buffer > >> * and so that needs to be copied here. > >> */ > >> - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; > >> - vb->v4l2_buf.field = b->field; > >> + vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE; > >> + vbuf->field = b->field; > >> } else { > >> /* Zero any output buffer flags as this is a capture buffer */ > >> - vb->v4l2_buf.flags &= ~V4L2_BUFFER_OUT_FLAGS; > >> + vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS; > >> } > >> } > >> > >> @@ -1395,7 +1414,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b > >> */ > >> static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> { > >> - __fill_vb2_buffer(vb, b, vb->v4l2_planes); > >> + __fill_vb2_buffer(vb, b, vb->planes); > >> return call_vb_qop(vb, buf_prepare, vb); > >> } > >> > >> @@ -1404,7 +1423,7 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> */ > >> static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> { > >> - struct v4l2_plane planes[VIDEO_MAX_PLANES]; > >> + struct vb2_plane planes[VIDEO_MAX_PLANES]; > >> struct vb2_queue *q = vb->vb2_queue; > >> void *mem_priv; > >> unsigned int plane; > >> @@ -1419,9 +1438,9 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> /* Skip the plane if already verified */ > >> - if (vb->v4l2_planes[plane].m.userptr && > >> - vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr > >> - && vb->v4l2_planes[plane].length == planes[plane].length) > >> + if (vb->planes[plane].m.userptr && > >> + vb->planes[plane].m.userptr == planes[plane].m.userptr > >> + && vb->planes[plane].length == planes[plane].length) > >> continue; > >> > >> dprintk(3, "userspace address for plane %d changed, " > >> @@ -1447,12 +1466,15 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> } > >> > >> vb->planes[plane].mem_priv = NULL; > >> - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); > >> + vb->planes[plane].bytesused = 0; > >> + vb->planes[plane].length = 0; > >> + vb->planes[plane].m.userptr = 0; > >> + vb->planes[plane].data_offset = 0; > >> > >> /* Acquire each plane's memory */ > >> mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane], > >> - planes[plane].m.userptr, > >> - planes[plane].length, dma_dir); > >> + planes[plane].m.userptr, > >> + planes[plane].length, dma_dir); > >> if (IS_ERR_OR_NULL(mem_priv)) { > >> dprintk(1, "failed acquiring userspace " > >> "memory for plane %d\n", plane); > >> @@ -1466,8 +1488,12 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> * Now that everything is in order, copy relevant information > >> * provided by userspace. > >> */ > >> - for (plane = 0; plane < vb->num_planes; ++plane) > >> - vb->v4l2_planes[plane] = planes[plane]; > >> + for (plane = 0; plane < vb->num_planes; ++plane) { > >> + vb->planes[plane].bytesused = planes[plane].bytesused; > >> + vb->planes[plane].length = planes[plane].length; > >> + vb->planes[plane].m.userptr = planes[plane].m.userptr; > >> + vb->planes[plane].data_offset = planes[plane].data_offset; > >> + } > >> > >> if (reacquired) { > >> /* > >> @@ -1494,10 +1520,11 @@ err: > >> /* In case of errors, release planes that were already acquired */ > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> if (vb->planes[plane].mem_priv) > >> - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); > >> + call_void_memop(vb, put_userptr, > >> + vb->planes[plane].mem_priv); > >> vb->planes[plane].mem_priv = NULL; > >> - vb->v4l2_planes[plane].m.userptr = 0; > >> - vb->v4l2_planes[plane].length = 0; > >> + vb->planes[plane].m.userptr = 0; > >> + vb->planes[plane].length = 0; > >> } > >> > >> return ret; > >> @@ -1508,7 +1535,7 @@ err: > >> */ > >> static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> { > >> - struct v4l2_plane planes[VIDEO_MAX_PLANES]; > >> + struct vb2_plane planes[VIDEO_MAX_PLANES]; > >> struct vb2_queue *q = vb->vb2_queue; > >> void *mem_priv; > >> unsigned int plane; > >> @@ -1544,7 +1571,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> > >> /* Skip the plane if already verified */ > >> if (dbuf == vb->planes[plane].dbuf && > >> - vb->v4l2_planes[plane].length == planes[plane].length) { > >> + vb->planes[plane].length == planes[plane].length) { > >> dma_buf_put(dbuf); > >> continue; > >> } > >> @@ -1558,11 +1585,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> > >> /* Release previously acquired memory if present */ > >> __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); > >> - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); > >> + vb->planes[plane].bytesused = 0; > >> + vb->planes[plane].length = 0; > >> + vb->planes[plane].m.fd = 0; > >> + vb->planes[plane].data_offset = 0; > >> > >> /* Acquire each plane's memory */ > >> - mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane], > >> - dbuf, planes[plane].length, dma_dir); > >> + mem_priv = call_ptr_memop(vb, attach_dmabuf, > >> + q->alloc_ctx[plane], dbuf, planes[plane].length, > >> + dma_dir); > >> if (IS_ERR(mem_priv)) { > >> dprintk(1, "failed to attach dmabuf\n"); > >> ret = PTR_ERR(mem_priv); > >> @@ -1592,8 +1623,12 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> * Now that everything is in order, copy relevant information > >> * provided by userspace. > >> */ > >> - for (plane = 0; plane < vb->num_planes; ++plane) > >> - vb->v4l2_planes[plane] = planes[plane]; > >> + for (plane = 0; plane < vb->num_planes; ++plane) { > >> + vb->planes[plane].bytesused = planes[plane].bytesused; > >> + vb->planes[plane].length = planes[plane].length; > >> + vb->planes[plane].m.fd = planes[plane].m.userptr; > >> + vb->planes[plane].data_offset = planes[plane].data_offset; > >> + } > >> > >> if (reacquired) { > >> /* > >> @@ -1644,6 +1679,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) > >> > >> static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> { > >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > >> struct vb2_queue *q = vb->vb2_queue; > >> int ret; > >> > >> @@ -1672,9 +1708,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> } > >> > >> vb->state = VB2_BUF_STATE_PREPARING; > >> - vb->v4l2_buf.timestamp.tv_sec = 0; > >> - vb->v4l2_buf.timestamp.tv_usec = 0; > >> - vb->v4l2_buf.sequence = 0; > >> + vbuf->timestamp.tv_sec = 0; > >> + vbuf->timestamp.tv_usec = 0; > >> + vbuf->sequence = 0; > >> > >> switch (q->memory) { > >> case V4L2_MEMORY_MMAP: > >> @@ -1701,7 +1737,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) > >> } > >> > >> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b, > >> - const char *opname) > >> + const char *opname) > >> { > >> if (b->type != q->type) { > >> dprintk(1, "%s: invalid buffer type\n", opname); > >> @@ -1768,7 +1804,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) > >> /* Fill buffer information for the userspace */ > >> __fill_v4l2_buffer(vb, b); > >> > >> - dprintk(1, "prepare of buffer %d succeeded\n", vb->v4l2_buf.index); > >> + dprintk(1, "prepare of buffer %d succeeded\n", vb->index); > >> } > >> return ret; > >> } > >> @@ -1800,7 +1836,7 @@ static int vb2_start_streaming(struct vb2_queue *q) > >> /* Tell the driver to start streaming */ > >> q->start_streaming_called = 1; > >> ret = call_qop(q, start_streaming, q, > >> - atomic_read(&q->owned_by_drv_count)); > >> + atomic_read(&q->owned_by_drv_count)); > >> if (!ret) > >> return 0; > >> > >> @@ -1810,7 +1846,7 @@ static int vb2_start_streaming(struct vb2_queue *q) > >> /* > >> * If you see this warning, then the driver isn't cleaning up properly > >> * after a failed start_streaming(). See the start_streaming() > >> - * documentation in videobuf2-v4l2.h for more information how buffers > >> + * documentation in videobuf2-core.h for more information how buffers > >> * should be returned to vb2 in start_streaming(). > >> */ > >> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { > >> @@ -1841,11 +1877,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > >> { > >> int ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); > >> struct vb2_buffer *vb; > >> + struct vb2_v4l2_buffer *vbuf; > >> > >> if (ret) > >> return ret; > >> > >> vb = q->bufs[b->index]; > >> + vbuf = to_vb2_v4l2_buffer(vb); > >> > >> switch (vb->state) { > >> case VB2_BUF_STATE_DEQUEUED: > >> @@ -1877,11 +1915,11 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > >> * and the timecode field and flag if needed. > >> */ > >> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) == > >> - V4L2_BUF_FLAG_TIMESTAMP_COPY) > >> - vb->v4l2_buf.timestamp = b->timestamp; > >> - vb->v4l2_buf.flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; > >> + V4L2_BUF_FLAG_TIMESTAMP_COPY) > >> + vbuf->timestamp = b->timestamp; > >> + vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; > >> if (b->flags & V4L2_BUF_FLAG_TIMECODE) > >> - vb->v4l2_buf.timecode = b->timecode; > >> + vbuf->timecode = b->timecode; > >> } > >> > >> trace_vb2_qbuf(q, vb); > >> @@ -1903,13 +1941,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) > >> * then we can finally call start_streaming(). > >> */ > >> if (q->streaming && !q->start_streaming_called && > >> - q->queued_count >= q->min_buffers_needed) { > >> + q->queued_count >= q->min_buffers_needed) { > >> ret = vb2_start_streaming(q); > >> if (ret) > >> return ret; > >> } > >> > >> - dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index); > >> + dprintk(1, "qbuf of buffer %d succeeded\n", vb->index); > >> return 0; > >> } > >> > >> @@ -2099,9 +2137,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb) > >> } > >> } > >> > >> -static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) > >> +static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, > >> + bool nonblocking) > >> { > >> struct vb2_buffer *vb = NULL; > >> + struct vb2_v4l2_buffer *vbuf = NULL; > >> int ret; > >> > >> if (b->type != q->type) { > >> @@ -2134,14 +2174,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n > >> > >> trace_vb2_dqbuf(q, vb); > >> > >> + vbuf = to_vb2_v4l2_buffer(vb); > >> if (!V4L2_TYPE_IS_OUTPUT(q->type) && > >> - vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST) > >> + vbuf->flags & V4L2_BUF_FLAG_LAST) > >> q->last_buffer_dequeued = true; > >> /* go back to dequeued state */ > >> __vb2_dqbuf(vb); > >> > >> dprintk(1, "dqbuf of buffer %d, with state %d\n", > >> - vb->v4l2_buf.index, vb->state); > >> + vb->index, vb->state); > >> > >> return 0; > >> } > >> @@ -2197,7 +2238,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) > >> /* > >> * If you see this warning, then the driver isn't cleaning up properly > >> * in stop_streaming(). See the stop_streaming() documentation in > >> - * videobuf2-v4l2.h for more information how buffers should be returned > >> + * videobuf2-core.h for more information how buffers should be returned > >> * to vb2 in stop_streaming(). > >> */ > >> if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { > >> @@ -2399,7 +2440,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, > >> vb = q->bufs[buffer]; > >> > >> for (plane = 0; plane < vb->num_planes; ++plane) { > >> - if (vb->v4l2_planes[plane].m.mem_offset == off) { > >> + if (vb->planes[plane].m.offset == off) { > >> *_buffer = buffer; > >> *_plane = plane; > >> return 0; > >> @@ -2557,7 +2598,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) > >> * The buffer length was page_aligned at __vb2_buf_mem_alloc(), > >> * so, we need to do the same here. > >> */ > >> - length = PAGE_ALIGN(vb->v4l2_planes[plane].length); > >> + length = PAGE_ALIGN(vb->planes[plane].length); > >> if (length < (vma->vm_end - vma->vm_start)) { > >> dprintk(1, > >> "MMAP invalid, as it would overflow buffer length\n"); > >> @@ -2577,10 +2618,10 @@ EXPORT_SYMBOL_GPL(vb2_mmap); > >> > >> #ifndef CONFIG_MMU > >> unsigned long vb2_get_unmapped_area(struct vb2_queue *q, > >> - unsigned long addr, > >> - unsigned long len, > >> - unsigned long pgoff, > >> - unsigned long flags) > >> + unsigned long addr, > >> + unsigned long len, > >> + unsigned long pgoff, > >> + unsigned long flags) > >> { > >> unsigned long off = pgoff << PAGE_SHIFT; > >> struct vb2_buffer *vb; > >> @@ -2731,7 +2772,7 @@ EXPORT_SYMBOL_GPL(vb2_poll); > >> * responsible of clearing it's content and setting initial values for some > >> * required entries before calling this function. > >> * q->ops, q->mem_ops, q->type and q->io_modes are mandatory. Please refer > >> - * to the struct vb2_queue description in include/media/videobuf2-v4l2.h > >> + * to the struct vb2_queue description in include/media/videobuf2-core.h > >> * for more information. > >> */ > >> int vb2_queue_init(struct vb2_queue *q) > >> @@ -2739,16 +2780,16 @@ int vb2_queue_init(struct vb2_queue *q) > >> /* > >> * Sanity check > >> */ > >> - if (WARN_ON(!q) || > >> - WARN_ON(!q->ops) || > >> - WARN_ON(!q->mem_ops) || > >> - WARN_ON(!q->type) || > >> - WARN_ON(!q->io_modes) || > >> - WARN_ON(!q->ops->queue_setup) || > >> - WARN_ON(!q->ops->buf_queue) || > >> - WARN_ON(q->timestamp_flags & > >> - ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | > >> - V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) > >> + if (WARN_ON(!q) || > >> + WARN_ON(!q->ops) || > >> + WARN_ON(!q->mem_ops) || > >> + WARN_ON(!q->type) || > >> + WARN_ON(!q->io_modes) || > >> + WARN_ON(!q->ops->queue_setup) || > >> + WARN_ON(!q->ops->buf_queue) || > >> + WARN_ON(q->timestamp_flags & > >> + ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | > >> + V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) > >> return -EINVAL; > >> > >> /* Warn that the driver should choose an appropriate timestamp type */ > >> @@ -2852,7 +2893,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) > >> * Sanity check > >> */ > >> if (WARN_ON((read && !(q->io_modes & VB2_READ)) || > >> - (!read && !(q->io_modes & VB2_WRITE)))) > >> + (!read && !(q->io_modes & VB2_WRITE)))) > >> return -EINVAL; > >> > >> /* > >> @@ -3063,9 +3104,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ > >> buf->queued = 0; > >> buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0) > >> : vb2_plane_size(q->bufs[index], 0); > >> - /* Compensate for data_offset on read in the multiplanar case. */ > >> + /* > >> + * Compensate for data_offset on read > >> + * in the multiplanar case > >> + */ > >> if (is_multiplanar && read && > >> - fileio->b.m.planes[0].data_offset < buf->size) { > >> + fileio->b.m.planes[0].data_offset < buf->size) { > >> buf->pos = fileio->b.m.planes[0].data_offset; > >> buf->size -= buf->pos; > >> } > >> @@ -3257,7 +3301,7 @@ static int vb2_thread(void *data) > >> * contact the linux-media mailinglist first. > >> */ > >> int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv, > >> - const char *thread_name) > >> + const char *thread_name) > >> { > >> struct vb2_threadio_data *threadio; > >> int ret = 0; > >> @@ -3491,7 +3535,7 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf, > >> if (vb2_queue_is_busy(vdev, file)) > >> goto exit; > >> err = vb2_write(vdev->queue, buf, count, ppos, > >> - file->f_flags & O_NONBLOCK); > >> + file->f_flags & O_NONBLOCK); > >> if (vdev->queue->fileio) > >> vdev->queue->owner = file->private_data; > >> exit: > >> @@ -3515,7 +3559,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf, > >> if (vb2_queue_is_busy(vdev, file)) > >> goto exit; > >> err = vb2_read(vdev->queue, buf, count, ppos, > >> - file->f_flags & O_NONBLOCK); > >> + file->f_flags & O_NONBLOCK); > >> if (vdev->queue->fileio) > >> vdev->queue->owner = file->private_data; > >> exit: > >> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h > >> index 155991e..8787a6c 100644 > >> --- a/include/media/videobuf2-core.h > >> +++ b/include/media/videobuf2-core.h > >> @@ -115,6 +115,16 @@ struct vb2_plane { > >> void *mem_priv; > >> struct dma_buf *dbuf; > >> unsigned int dbuf_mapped; > >> + > >> + /* plane information */ > >> + unsigned int bytesused; > >> + unsigned int length; > >> + union { > >> + unsigned int offset; > >> + unsigned long userptr; > >> + int fd; > >> + } m; > >> + unsigned int data_offset; > >> }; > > > > Nitpick: it would be good to add a documentation for struct vb2_plane, > > describing what each field means on this struct. Granted, this could > > be added after this patch series. > > > > Btw, I don't see much reason to have the: > > /* plane information */ > > comment here, as this struct is all about plane information, right? > > Or, maybe you wanted, instead, to comment that those fields should > > have what's there at struct v4l2_plane? That would make sense ;) > > Latter is right. > > > So, I would change this comment to something like: > > > > /* > > * Should contain enough plane information to contain the > > * fields needed to fill struct v4l2_plane at videodev2.h > > */ > > > OK, I will change it. > > > >> > >> /** > >> @@ -161,41 +171,33 @@ 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 > >> - * @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 > >> + * @index: id number of the buffer > >> + * @type: buffer type > >> + * @memory: the method, in which the actual data is passed > >> + * @num_planes: number of planes in the buffer > >> + * on an internal driver queue > >> * @planes: private per-plane information; do not change > >> */ > >> struct vb2_buffer { > >> - struct v4l2_buffer v4l2_buf; > >> - struct v4l2_plane v4l2_planes[VIDEO_MAX_PLANES]; > >> - > >> struct vb2_queue *vb2_queue; > >> > >> - unsigned int num_planes; > >> - > >> -/* Private: internal use only */ > >> + /* Private: internal use only */ > >> enum vb2_buffer_state state; > >> > >> struct list_head queued_entry; > >> struct list_head done_entry; > >> > >> + /* buffer information */ > >> + unsigned int index; > >> + unsigned int type; > >> + unsigned int memory; > >> + unsigned int num_planes; > > > > Nitpick: Those comments that follow Documentation/kernel-doc-nano-HOWTO.txt > > are used to produce DocBook data, on both html and manpages formats. > > As documented there, DocBook discards documentation for all fields after a > > /*private: ...*/ comment. > > > > In other words, we need to take care of putting things after /*private:*/ > > only when we're 100% sure that such fields are not meant to be filled/used > > by the callers, and will be used only internally, and don't deserve any > > documentation for the kABI. > > > > As you're adding documentation for those fields, and num_planes were > > documented before your series, I suspect that this is not what you want. > > So, please move them to be before the private: comment. > > Oh, I didn't know that /*private: ...*/ comment have so much meaning. > I will change it at next round. > > > > > Btw, if your patches are based on top of the current patchwork tree > > e. g. if it has this patch: > > http://git.linuxtv.org/cgit.cgi/media_tree.git/commit/?id=d071c833a0d30e7aae0ea565d92ef83c79106d6f > > > > Then you can make the Kernel to handle all those kernel-doc-nano > > comments with: > > > > make cleandocs && make DOCBOOKS=device-drivers.xml htmldocs > > > > It will produce some html pages like: > > http://linuxtv.org/downloads/v4l-dvb-internals/device-drivers/mediadev.html > > > > With can be useful to check if the documentation tags are properly placed. > > > >> struct vb2_plane planes[VIDEO_MAX_PLANES]; > >> > >> #ifdef CONFIG_VIDEO_ADV_DEBUG > >> @@ -390,7 +392,7 @@ struct v4l2_fh; > >> * @threadio: thread io internal data, used only if thread is active > >> */ > >> struct vb2_queue { > >> - enum v4l2_buf_type type; > >> + unsigned int type; > >> unsigned int io_modes; > >> unsigned fileio_read_once:1; > >> unsigned fileio_write_immediately:1; > >> @@ -409,7 +411,7 @@ struct vb2_queue { > >> > >> /* private: internal use only */ > >> struct mutex mmap_lock; > >> - enum v4l2_memory memory; > >> + unsigned int memory; > > > > Will the vb2-core use type/memory fields or just vb2-v4l2? As you > > removed the enum, I suspect you won't be relying on having the videodev2.h > > header included here, right. > > Exactly. > > > > > If so, then the meaning of the type/memory fields are private to the > > caller of the VB2-core (e. .g. the meaning are private to vb2-v4l2 and > > vb2-dvb). So, you should be changing the description of those fields > > at the doc-nano to: > > ... > > * @type: private type whose content is defined by the vb2-core caller. > > * For example, for V4L2, it should match the V4L2_BUF_TYPE_* > > * in include/uapi/linux/videodev2.h > > ... > > > > to let it clear. > > > > 0k, I will change it as your comment. > > >> struct vb2_buffer *bufs[VIDEO_MAX_FRAME]; > >> unsigned int num_buffers; > >> > >> @@ -571,7 +573,7 @@ static inline void vb2_set_plane_payload(struct vb2_buffer *vb, > >> unsigned int plane_no, unsigned long size) > >> { > >> if (plane_no < vb->num_planes) > >> - vb->v4l2_planes[plane_no].bytesused = size; > >> + vb->planes[plane_no].bytesused = size; > >> } > >> > >> /** > >> @@ -583,7 +585,7 @@ static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb, > >> unsigned int plane_no) > >> { > >> if (plane_no < vb->num_planes) > >> - return vb->v4l2_planes[plane_no].bytesused; > >> + return vb->planes[plane_no].bytesused; > >> return 0; > >> } > >> > >> @@ -596,7 +598,7 @@ static inline unsigned long > >> vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no) > >> { > >> if (plane_no < vb->num_planes) > >> - return vb->v4l2_planes[plane_no].length; > >> + return vb->planes[plane_no].length; > >> return 0; > >> } > >> > >> diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h > >> index d4a4d9a..fc2dbe9 100644 > >> --- a/include/media/videobuf2-v4l2.h > >> +++ b/include/media/videobuf2-v4l2.h > >> @@ -12,6 +12,32 @@ > >> #ifndef _MEDIA_VIDEOBUF2_V4L2_H > >> #define _MEDIA_VIDEOBUF2_V4L2_H > >> > >> +#include <linux/videodev2.h> > >> #include <media/videobuf2-core.h> > >> > >> +/** > >> + * struct vb2_v4l2_buffer - video buffer information for v4l2 > >> + * @vb2_buf: video buffer 2 > >> + * @flags: buffer informational flags > >> + * @field: enum v4l2_field; field order of the image in the buffer > >> + * @timestamp: frame timestamp > >> + * @timecode: frame timecode > >> + * @sequence: sequence count of this frame > >> + */ > >> +struct vb2_v4l2_buffer { > >> + struct vb2_buffer vb2_buf; > >> + > >> + __u32 flags; > >> + __u32 field; > >> + struct timeval timestamp; > >> + struct v4l2_timecode timecode; > >> + __u32 sequence; > >> +}; > >> + > >> +/** > >> + * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer * > >> + */ > >> +#define to_vb2_v4l2_buffer(vb) \ > >> + (container_of(vb, struct vb2_v4l2_buffer, vb2_buf)) > >> + > >> #endif /* _MEDIA_VIDEOBUF2_V4L2_H */ > > > > Regards, > > Mauro > > > > -- > > To unsubscribe from this list: send the line "unsubscribe linux-media" in > > the body of a message to majordomo@vger.kernel.org > > More majordomo info at http://vger.kernel.org/majordomo-info.html > > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Junghak, Thanks for this patch, it looks much better. I do have a number of comments, though... On 08/26/2015 01:59 PM, Junghak Sung wrote: > Remove v4l2-specific stuff from struct vb2_buffer and add member variables > related with buffer management. > > struct vb2_plane { > <snip> > /* plane information */ > unsigned int bytesused; > unsigned int length; > union { > unsigned int offset; > unsigned long userptr; > int fd; > } m; > unsigned int data_offset; > } > > struct vb2_buffer { > <snip> > /* buffer information */ > unsigned int num_planes; > unsigned int index; > unsigned int type; > unsigned int memory; > > struct vb2_plane planes[VIDEO_MAX_PLANES]; > <snip> > }; > > And create struct vb2_v4l2_buffer as container buffer for v4l2 use. > > struct vb2_v4l2_buffer { > struct vb2_buffer vb2_buf; > > __u32 flags; > __u32 field; > struct timeval timestamp; > struct v4l2_timecode timecode; > __u32 sequence; > }; > > This patch includes only changes inside of videobuf2. So, it is required to > modify all device drivers which use videobuf2. > > 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 | 324 +++++++++++++++++------------- > include/media/videobuf2-core.h | 50 ++--- > include/media/videobuf2-v4l2.h | 26 +++ > 3 files changed, 236 insertions(+), 164 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c > index ab00ea0..9266d50 100644 > --- a/drivers/media/v4l2-core/videobuf2-core.c > +++ b/drivers/media/v4l2-core/videobuf2-core.c > @@ -35,10 +35,10 @@ > static int debug; > module_param(debug, int, 0644); > > -#define dprintk(level, fmt, arg...) \ > - do { \ > - if (debug >= level) \ > - pr_info("vb2: %s: " fmt, __func__, ## arg); \ > +#define dprintk(level, fmt, arg...) \ > + do { \ > + if (debug >= level) \ > + pr_info("vb2: %s: " fmt, __func__, ## arg); \ These are just whitespace changes, and that is something it see *a lot* in this patch. And usually for no clear reason. Please remove those whitespace changes, it makes a difficult patch even harder to read than it already is. > } while (0) > > #ifdef CONFIG_VIDEO_ADV_DEBUG <snip> > @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) > */ > static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > { > + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); > struct vb2_queue *q = vb->vb2_queue; > + unsigned int plane; > > /* Copy back data such as timestamp, flags, etc. */ > - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); > - b->reserved2 = vb->v4l2_buf.reserved2; > - b->reserved = vb->v4l2_buf.reserved; Hmm, I'm not sure why these reserved fields were copied here. I think it was for compatibility reasons for some old drivers that abused the reserved field. However, in the new code these reserved fields should probably be explicitly initialized to 0. > + b->index = vb->index; > + b->type = vb->type; > + b->memory = vb->memory; > + b->bytesused = 0; > + > + b->flags = vbuf->flags; > + b->field = vbuf->field; > + b->timestamp = vbuf->timestamp; > + b->timecode = vbuf->timecode; > + b->sequence = vbuf->sequence; > > if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { > /* > @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > * for it. The caller has already verified memory and size. > */ > b->length = vb->num_planes; > - memcpy(b->m.planes, vb->v4l2_planes, > - b->length * sizeof(struct v4l2_plane)); A similar problem occurs here: the memcpy would have copied the reserved field in v4l2_plane as well, but that no longer happens, so you need to do an explicit memset for the reserved field in the new code. > + for (plane = 0; plane < vb->num_planes; ++plane) { > + struct v4l2_plane *pdst = &b->m.planes[plane]; > + struct vb2_plane *psrc = &vb->planes[plane]; > + > + pdst->bytesused = psrc->bytesused; > + pdst->length = psrc->length; > + if (q->memory == V4L2_MEMORY_MMAP) > + pdst->m.mem_offset = psrc->m.offset; > + else if (q->memory == V4L2_MEMORY_USERPTR) > + pdst->m.userptr = psrc->m.userptr; > + else if (q->memory == V4L2_MEMORY_DMABUF) > + pdst->m.fd = psrc->m.fd; > + pdst->data_offset = psrc->data_offset; > + } > } else { > /* > * We use length and offset in v4l2_planes array even for > * single-planar buffers, but userspace does not. > */ > - b->length = vb->v4l2_planes[0].length; > - b->bytesused = vb->v4l2_planes[0].bytesused; > + b->length = vb->planes[0].length; > + b->bytesused = vb->planes[0].bytesused; > if (q->memory == V4L2_MEMORY_MMAP) > - b->m.offset = vb->v4l2_planes[0].m.mem_offset; > + b->m.offset = vb->planes[0].m.offset; > else if (q->memory == V4L2_MEMORY_USERPTR) > - b->m.userptr = vb->v4l2_planes[0].m.userptr; > + b->m.userptr = vb->planes[0].m.userptr; > else if (q->memory == V4L2_MEMORY_DMABUF) > - b->m.fd = vb->v4l2_planes[0].m.fd; > + b->m.fd = vb->planes[0].m.fd; > } > > /* > @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) > b->flags &= ~V4L2_BUFFER_MASK_FLAGS; > b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; > if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != > - V4L2_BUF_FLAG_TIMESTAMP_COPY) { > + V4L2_BUF_FLAG_TIMESTAMP_COPY) { > /* > * For non-COPY timestamps, drop timestamp source bits > * and obtain the timestamp source from the queue. > @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); > static int __verify_userptr_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || > - !q->mem_ops->put_userptr) > + !q->mem_ops->put_userptr) > return -EINVAL; > > return 0; > @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) > static int __verify_mmap_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || > - !q->mem_ops->put || !q->mem_ops->mmap) > + !q->mem_ops->put || !q->mem_ops->mmap) > return -EINVAL; > > return 0; > @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) > static int __verify_dmabuf_ops(struct vb2_queue *q) > { > if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || > - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > - !q->mem_ops->unmap_dmabuf) > + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || > + !q->mem_ops->unmap_dmabuf) > return -EINVAL; > > return 0; > @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, > enum v4l2_memory memory, enum v4l2_buf_type type) > { > if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && > - memory != V4L2_MEMORY_DMABUF) { > + memory != V4L2_MEMORY_DMABUF) { > dprintk(1, "unsupported memory type\n"); > return -EINVAL; > } > @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > * Driver also sets the size and allocator context for each plane. > */ > ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, > - q->plane_sizes, q->alloc_ctx); > + q->plane_sizes, q->alloc_ctx); > if (ret) > return ret; > > @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) > num_buffers = allocated_buffers; > > ret = call_qop(q, queue_setup, q, NULL, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > > if (!ret && allocated_buffers < num_buffers) > ret = -ENOMEM; > @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > * buffer and their sizes are acceptable > */ > ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > if (ret) > return ret; > > @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create > * queue driver has set up > */ > ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, > - &num_planes, q->plane_sizes, q->alloc_ctx); > + &num_planes, q->plane_sizes, q->alloc_ctx); > > if (!ret && allocated_buffers < num_buffers) > ret = -ENOMEM; > @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) > return; > > if (WARN_ON(state != VB2_BUF_STATE_DONE && > - state != VB2_BUF_STATE_ERROR && > - state != VB2_BUF_STATE_QUEUED)) > + state != VB2_BUF_STATE_ERROR && > + state != VB2_BUF_STATE_QUEUED)) > state = VB2_BUF_STATE_ERROR; > > #ifdef CONFIG_VIDEO_ADV_DEBUG All the chunks above are all spurious whitespace changes. As mentioned in the beginning, please remove all those pointless whitespace changes! There are a lot more of these, but I won't comment on them anymore. Basically this patch looks good to me, so once I have the next version without all the whitespace confusion and with the reserved field issues solved I'll do a final review. BTW, did you test this with 'v4l2-compliance -s' and with the vivid driver? Just to make sure you didn't break anything. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hello Hans, Thank you for your review. I leave some comments in the body for reply. Regards, Junghak On 08/28/2015 10:31 PM, Hans Verkuil wrote: > Hi Junghak, > > Thanks for this patch, it looks much better. I do have a number of comments, though... > > On 08/26/2015 01:59 PM, Junghak Sung wrote: >> Remove v4l2-specific stuff from struct vb2_buffer and add member variables >> related with buffer management. >> >> struct vb2_plane { >> <snip> >> /* plane information */ >> unsigned int bytesused; >> unsigned int length; >> union { >> unsigned int offset; >> unsigned long userptr; >> int fd; >> } m; >> unsigned int data_offset; >> } >> >> struct vb2_buffer { >> <snip> >> /* buffer information */ >> unsigned int num_planes; >> unsigned int index; >> unsigned int type; >> unsigned int memory; >> >> struct vb2_plane planes[VIDEO_MAX_PLANES]; >> <snip> >> }; >> >> And create struct vb2_v4l2_buffer as container buffer for v4l2 use. >> >> struct vb2_v4l2_buffer { >> struct vb2_buffer vb2_buf; >> >> __u32 flags; >> __u32 field; >> struct timeval timestamp; >> struct v4l2_timecode timecode; >> __u32 sequence; >> }; >> >> This patch includes only changes inside of videobuf2. So, it is required to >> modify all device drivers which use videobuf2. >> >> 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 | 324 +++++++++++++++++------------- >> include/media/videobuf2-core.h | 50 ++--- >> include/media/videobuf2-v4l2.h | 26 +++ >> 3 files changed, 236 insertions(+), 164 deletions(-) >> >> diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c >> index ab00ea0..9266d50 100644 >> --- a/drivers/media/v4l2-core/videobuf2-core.c >> +++ b/drivers/media/v4l2-core/videobuf2-core.c >> @@ -35,10 +35,10 @@ >> static int debug; >> module_param(debug, int, 0644); >> >> -#define dprintk(level, fmt, arg...) \ >> - do { \ >> - if (debug >= level) \ >> - pr_info("vb2: %s: " fmt, __func__, ## arg); \ >> +#define dprintk(level, fmt, arg...) \ >> + do { \ >> + if (debug >= level) \ >> + pr_info("vb2: %s: " fmt, __func__, ## arg); \ > > These are just whitespace changes, and that is something it see *a lot* in this > patch. And usually for no clear reason. > > Please remove those whitespace changes, it makes a difficult patch even harder > to read than it already is. > I just wanted to remove unnecessary white spaces or adjust alignment. OK, I will revert those whitespace changes for better review. >> } while (0) >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG > > <snip> > >> @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) >> */ >> static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> { >> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >> struct vb2_queue *q = vb->vb2_queue; >> + unsigned int plane; >> >> /* Copy back data such as timestamp, flags, etc. */ >> - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); >> - b->reserved2 = vb->v4l2_buf.reserved2; >> - b->reserved = vb->v4l2_buf.reserved; > > Hmm, I'm not sure why these reserved fields were copied here. I think it was > for compatibility reasons for some old drivers that abused the reserved field. > However, in the new code these reserved fields should probably be explicitly > initialized to 0. > >> + b->index = vb->index; >> + b->type = vb->type; >> + b->memory = vb->memory; >> + b->bytesused = 0; >> + >> + b->flags = vbuf->flags; >> + b->field = vbuf->field; >> + b->timestamp = vbuf->timestamp; >> + b->timecode = vbuf->timecode; >> + b->sequence = vbuf->sequence; >> >> if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { >> /* >> @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> * for it. The caller has already verified memory and size. >> */ >> b->length = vb->num_planes; >> - memcpy(b->m.planes, vb->v4l2_planes, >> - b->length * sizeof(struct v4l2_plane)); > > A similar problem occurs here: the memcpy would have copied the reserved > field in v4l2_plane as well, but that no longer happens, so you need to > do an explicit memset for the reserved field in the new code. > It means that I'd better add reserved fields to struct vb2_buffer and struct vb2_plane in order to keep the information in struct v4l2_buffer and struct v4l2_plane??? >> + for (plane = 0; plane < vb->num_planes; ++plane) { >> + struct v4l2_plane *pdst = &b->m.planes[plane]; >> + struct vb2_plane *psrc = &vb->planes[plane]; >> + >> + pdst->bytesused = psrc->bytesused; >> + pdst->length = psrc->length; >> + if (q->memory == V4L2_MEMORY_MMAP) >> + pdst->m.mem_offset = psrc->m.offset; >> + else if (q->memory == V4L2_MEMORY_USERPTR) >> + pdst->m.userptr = psrc->m.userptr; >> + else if (q->memory == V4L2_MEMORY_DMABUF) >> + pdst->m.fd = psrc->m.fd; >> + pdst->data_offset = psrc->data_offset; >> + } >> } else { >> /* >> * We use length and offset in v4l2_planes array even for >> * single-planar buffers, but userspace does not. >> */ >> - b->length = vb->v4l2_planes[0].length; >> - b->bytesused = vb->v4l2_planes[0].bytesused; >> + b->length = vb->planes[0].length; >> + b->bytesused = vb->planes[0].bytesused; >> if (q->memory == V4L2_MEMORY_MMAP) >> - b->m.offset = vb->v4l2_planes[0].m.mem_offset; >> + b->m.offset = vb->planes[0].m.offset; >> else if (q->memory == V4L2_MEMORY_USERPTR) >> - b->m.userptr = vb->v4l2_planes[0].m.userptr; >> + b->m.userptr = vb->planes[0].m.userptr; >> else if (q->memory == V4L2_MEMORY_DMABUF) >> - b->m.fd = vb->v4l2_planes[0].m.fd; >> + b->m.fd = vb->planes[0].m.fd; >> } >> >> /* >> @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) >> b->flags &= ~V4L2_BUFFER_MASK_FLAGS; >> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; >> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != >> - V4L2_BUF_FLAG_TIMESTAMP_COPY) { >> + V4L2_BUF_FLAG_TIMESTAMP_COPY) { >> /* >> * For non-COPY timestamps, drop timestamp source bits >> * and obtain the timestamp source from the queue. >> @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); >> static int __verify_userptr_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || >> - !q->mem_ops->put_userptr) >> + !q->mem_ops->put_userptr) >> return -EINVAL; >> >> return 0; >> @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) >> static int __verify_mmap_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || >> - !q->mem_ops->put || !q->mem_ops->mmap) >> + !q->mem_ops->put || !q->mem_ops->mmap) >> return -EINVAL; >> >> return 0; >> @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) >> static int __verify_dmabuf_ops(struct vb2_queue *q) >> { >> if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || >> - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >> - !q->mem_ops->unmap_dmabuf) >> + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >> + !q->mem_ops->unmap_dmabuf) >> return -EINVAL; >> >> return 0; >> @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, >> enum v4l2_memory memory, enum v4l2_buf_type type) >> { >> if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && >> - memory != V4L2_MEMORY_DMABUF) { >> + memory != V4L2_MEMORY_DMABUF) { >> dprintk(1, "unsupported memory type\n"); >> return -EINVAL; >> } >> @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) >> * Driver also sets the size and allocator context for each plane. >> */ >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, >> - q->plane_sizes, q->alloc_ctx); >> + q->plane_sizes, q->alloc_ctx); >> if (ret) >> return ret; >> >> @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) >> num_buffers = allocated_buffers; >> >> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> >> if (!ret && allocated_buffers < num_buffers) >> ret = -ENOMEM; >> @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create >> * buffer and their sizes are acceptable >> */ >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> if (ret) >> return ret; >> >> @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create >> * queue driver has set up >> */ >> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >> - &num_planes, q->plane_sizes, q->alloc_ctx); >> + &num_planes, q->plane_sizes, q->alloc_ctx); >> >> if (!ret && allocated_buffers < num_buffers) >> ret = -ENOMEM; >> @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) >> return; >> >> if (WARN_ON(state != VB2_BUF_STATE_DONE && >> - state != VB2_BUF_STATE_ERROR && >> - state != VB2_BUF_STATE_QUEUED)) >> + state != VB2_BUF_STATE_ERROR && >> + state != VB2_BUF_STATE_QUEUED)) >> state = VB2_BUF_STATE_ERROR; >> >> #ifdef CONFIG_VIDEO_ADV_DEBUG > > All the chunks above are all spurious whitespace changes. As mentioned in the beginning, > please remove all those pointless whitespace changes! > > There are a lot more of these, but I won't comment on them anymore. > > Basically this patch looks good to me, so once I have the next version without all the > whitespace confusion and with the reserved field issues solved I'll do a final review. > > BTW, did you test this with 'v4l2-compliance -s' and with the vivid driver? Just to > make sure you didn't break anything. > Actually, I've tested with v4l2-compliance for just one v4l2 drivers - au0828. I'll try to test with vivid driver at next round. > Regards, > > Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2015 11:01 AM, Junghak Sung wrote: > Hello Hans, > > Thank you for your review. > I leave some comments in the body for reply. > > Regards, > Junghak > > > > On 08/28/2015 10:31 PM, Hans Verkuil wrote: >> Hi Junghak, >> >> Thanks for this patch, it looks much better. I do have a number of >> comments, though... >> >> On 08/26/2015 01:59 PM, Junghak Sung wrote: >>> Remove v4l2-specific stuff from struct vb2_buffer and add member >>> variables >>> related with buffer management. >>> >>> struct vb2_plane { >>> <snip> >>> /* plane information */ >>> unsigned int bytesused; >>> unsigned int length; >>> union { >>> unsigned int offset; >>> unsigned long userptr; >>> int fd; >>> } m; >>> unsigned int data_offset; >>> } >>> >>> struct vb2_buffer { >>> <snip> >>> /* buffer information */ >>> unsigned int num_planes; >>> unsigned int index; >>> unsigned int type; >>> unsigned int memory; >>> >>> struct vb2_plane planes[VIDEO_MAX_PLANES]; >>> <snip> >>> }; >>> >>> And create struct vb2_v4l2_buffer as container buffer for v4l2 use. >>> >>> struct vb2_v4l2_buffer { >>> struct vb2_buffer vb2_buf; >>> >>> __u32 flags; >>> __u32 field; >>> struct timeval timestamp; >>> struct v4l2_timecode timecode; >>> __u32 sequence; >>> }; >>> >>> This patch includes only changes inside of videobuf2. So, it is >>> required to >>> modify all device drivers which use videobuf2. >>> >>> 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 | 324 >>> +++++++++++++++++------------- >>> include/media/videobuf2-core.h | 50 ++--- >>> include/media/videobuf2-v4l2.h | 26 +++ >>> 3 files changed, 236 insertions(+), 164 deletions(-) >>> >>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>> b/drivers/media/v4l2-core/videobuf2-core.c >>> index ab00ea0..9266d50 100644 >>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>> @@ -35,10 +35,10 @@ >>> static int debug; >>> module_param(debug, int, 0644); >>> >>> -#define dprintk(level, fmt, arg...) \ >>> - do { \ >>> - if (debug >= level) \ >>> - pr_info("vb2: %s: " fmt, __func__, ## arg); \ >>> +#define dprintk(level, fmt, arg...) \ >>> + do { \ >>> + if (debug >= level) \ >>> + pr_info("vb2: %s: " fmt, __func__, ## arg); \ >> >> These are just whitespace changes, and that is something it see *a >> lot* in this >> patch. And usually for no clear reason. >> >> Please remove those whitespace changes, it makes a difficult patch >> even harder >> to read than it already is. >> > > I just wanted to remove unnecessary white spaces or adjust alignment. > OK, I will revert those whitespace changes for better review. > >>> } while (0) >>> >>> #ifdef CONFIG_VIDEO_ADV_DEBUG >> >> <snip> >> >>> @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) >>> */ >>> static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct >>> v4l2_buffer *b) >>> { >>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >>> struct vb2_queue *q = vb->vb2_queue; >>> + unsigned int plane; >>> >>> /* Copy back data such as timestamp, flags, etc. */ >>> - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); >>> - b->reserved2 = vb->v4l2_buf.reserved2; >>> - b->reserved = vb->v4l2_buf.reserved; >> >> Hmm, I'm not sure why these reserved fields were copied here. I think >> it was >> for compatibility reasons for some old drivers that abused the >> reserved field. >> However, in the new code these reserved fields should probably be >> explicitly >> initialized to 0. >> >>> + b->index = vb->index; >>> + b->type = vb->type; >>> + b->memory = vb->memory; >>> + b->bytesused = 0; >>> + >>> + b->flags = vbuf->flags; >>> + b->field = vbuf->field; >>> + b->timestamp = vbuf->timestamp; >>> + b->timecode = vbuf->timecode; >>> + b->sequence = vbuf->sequence; >>> >>> if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { >>> /* >>> @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct >>> vb2_buffer *vb, struct v4l2_buffer *b) >>> * for it. The caller has already verified memory and size. >>> */ >>> b->length = vb->num_planes; >>> - memcpy(b->m.planes, vb->v4l2_planes, >>> - b->length * sizeof(struct v4l2_plane)); >> >> A similar problem occurs here: the memcpy would have copied the reserved >> field in v4l2_plane as well, but that no longer happens, so you need to >> do an explicit memset for the reserved field in the new code. >> > > It means that I'd better add reserved fields to struct vb2_buffer and > struct vb2_plane in order to keep the information in struct v4l2_buffer > and struct v4l2_plane??? > Oh, I've mistaken your point. Now I got your point. In __fill_v4l2_buffer(), just initialize explicitly the reserved field like : memset(pdst->reserved, 0 sizeof(pdst->reserved)); Right? > >>> + for (plane = 0; plane < vb->num_planes; ++plane) { >>> + struct v4l2_plane *pdst = &b->m.planes[plane]; >>> + struct vb2_plane *psrc = &vb->planes[plane]; >>> + >>> + pdst->bytesused = psrc->bytesused; >>> + pdst->length = psrc->length; >>> + if (q->memory == V4L2_MEMORY_MMAP) >>> + pdst->m.mem_offset = psrc->m.offset; >>> + else if (q->memory == V4L2_MEMORY_USERPTR) >>> + pdst->m.userptr = psrc->m.userptr; >>> + else if (q->memory == V4L2_MEMORY_DMABUF) >>> + pdst->m.fd = psrc->m.fd; >>> + pdst->data_offset = psrc->data_offset; >>> + } >>> } else { >>> /* >>> * We use length and offset in v4l2_planes array even for >>> * single-planar buffers, but userspace does not. >>> */ >>> - b->length = vb->v4l2_planes[0].length; >>> - b->bytesused = vb->v4l2_planes[0].bytesused; >>> + b->length = vb->planes[0].length; >>> + b->bytesused = vb->planes[0].bytesused; >>> if (q->memory == V4L2_MEMORY_MMAP) >>> - b->m.offset = vb->v4l2_planes[0].m.mem_offset; >>> + b->m.offset = vb->planes[0].m.offset; >>> else if (q->memory == V4L2_MEMORY_USERPTR) >>> - b->m.userptr = vb->v4l2_planes[0].m.userptr; >>> + b->m.userptr = vb->planes[0].m.userptr; >>> else if (q->memory == V4L2_MEMORY_DMABUF) >>> - b->m.fd = vb->v4l2_planes[0].m.fd; >>> + b->m.fd = vb->planes[0].m.fd; >>> } >>> >>> /* >>> @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer >>> *vb, struct v4l2_buffer *b) >>> b->flags &= ~V4L2_BUFFER_MASK_FLAGS; >>> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; >>> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != >>> - V4L2_BUF_FLAG_TIMESTAMP_COPY) { >>> + V4L2_BUF_FLAG_TIMESTAMP_COPY) { >>> /* >>> * For non-COPY timestamps, drop timestamp source bits >>> * and obtain the timestamp source from the queue. >>> @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); >>> static int __verify_userptr_ops(struct vb2_queue *q) >>> { >>> if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || >>> - !q->mem_ops->put_userptr) >>> + !q->mem_ops->put_userptr) >>> return -EINVAL; >>> >>> return 0; >>> @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) >>> static int __verify_mmap_ops(struct vb2_queue *q) >>> { >>> if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || >>> - !q->mem_ops->put || !q->mem_ops->mmap) >>> + !q->mem_ops->put || !q->mem_ops->mmap) >>> return -EINVAL; >>> >>> return 0; >>> @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) >>> static int __verify_dmabuf_ops(struct vb2_queue *q) >>> { >>> if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || >>> - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >>> - !q->mem_ops->unmap_dmabuf) >>> + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >>> + !q->mem_ops->unmap_dmabuf) >>> return -EINVAL; >>> >>> return 0; >>> @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, >>> enum v4l2_memory memory, enum v4l2_buf_type type) >>> { >>> if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && >>> - memory != V4L2_MEMORY_DMABUF) { >>> + memory != V4L2_MEMORY_DMABUF) { >>> dprintk(1, "unsupported memory type\n"); >>> return -EINVAL; >>> } >>> @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct >>> v4l2_requestbuffers *req) >>> * Driver also sets the size and allocator context for each plane. >>> */ >>> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, >>> - q->plane_sizes, q->alloc_ctx); >>> + q->plane_sizes, q->alloc_ctx); >>> if (ret) >>> return ret; >>> >>> @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct >>> v4l2_requestbuffers *req) >>> num_buffers = allocated_buffers; >>> >>> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, >>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>> >>> if (!ret && allocated_buffers < num_buffers) >>> ret = -ENOMEM; >>> @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, >>> struct v4l2_create_buffers *create >>> * buffer and their sizes are acceptable >>> */ >>> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>> if (ret) >>> return ret; >>> >>> @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, >>> struct v4l2_create_buffers *create >>> * queue driver has set up >>> */ >>> ret = call_qop(q, queue_setup, q, &create->format, >>> &num_buffers, >>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>> >>> if (!ret && allocated_buffers < num_buffers) >>> ret = -ENOMEM; >>> @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, >>> enum vb2_buffer_state state) >>> return; >>> >>> if (WARN_ON(state != VB2_BUF_STATE_DONE && >>> - state != VB2_BUF_STATE_ERROR && >>> - state != VB2_BUF_STATE_QUEUED)) >>> + state != VB2_BUF_STATE_ERROR && >>> + state != VB2_BUF_STATE_QUEUED)) >>> state = VB2_BUF_STATE_ERROR; >>> >>> #ifdef CONFIG_VIDEO_ADV_DEBUG >> >> All the chunks above are all spurious whitespace changes. As mentioned >> in the beginning, >> please remove all those pointless whitespace changes! >> >> There are a lot more of these, but I won't comment on them anymore. >> >> Basically this patch looks good to me, so once I have the next version >> without all the >> whitespace confusion and with the reserved field issues solved I'll do >> a final review. >> >> BTW, did you test this with 'v4l2-compliance -s' and with the vivid >> driver? Just to >> make sure you didn't break anything. >> > > Actually, I've tested with v4l2-compliance for just one v4l2 drivers - > au0828. > I'll try to test with vivid driver at next round. > I tried to use vivid for test. But, I have failed to install the module (vivid.ko). I mainly referred to documentation/video4linux/vivid.txt. But, it not enough to test. Could you give me a guide? Best Regards, Junghak > >> Regards, >> >> Hans >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@vger.kernel.org >> More majordomo info at http://vger.kernel.org/majordomo-info.html >> > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 08/31/2015 09:56 AM, Junghak Sung wrote: > > > On 08/31/2015 11:01 AM, Junghak Sung wrote: >> Hello Hans, >> >> Thank you for your review. >> I leave some comments in the body for reply. >> >> Regards, >> Junghak >> >> >> >> On 08/28/2015 10:31 PM, Hans Verkuil wrote: >>> Hi Junghak, >>> >>> Thanks for this patch, it looks much better. I do have a number of >>> comments, though... >>> >>> On 08/26/2015 01:59 PM, Junghak Sung wrote: >>>> Remove v4l2-specific stuff from struct vb2_buffer and add member >>>> variables >>>> related with buffer management. >>>> >>>> struct vb2_plane { >>>> <snip> >>>> /* plane information */ >>>> unsigned int bytesused; >>>> unsigned int length; >>>> union { >>>> unsigned int offset; >>>> unsigned long userptr; >>>> int fd; >>>> } m; >>>> unsigned int data_offset; >>>> } >>>> >>>> struct vb2_buffer { >>>> <snip> >>>> /* buffer information */ >>>> unsigned int num_planes; >>>> unsigned int index; >>>> unsigned int type; >>>> unsigned int memory; >>>> >>>> struct vb2_plane planes[VIDEO_MAX_PLANES]; >>>> <snip> >>>> }; >>>> >>>> And create struct vb2_v4l2_buffer as container buffer for v4l2 use. >>>> >>>> struct vb2_v4l2_buffer { >>>> struct vb2_buffer vb2_buf; >>>> >>>> __u32 flags; >>>> __u32 field; >>>> struct timeval timestamp; >>>> struct v4l2_timecode timecode; >>>> __u32 sequence; >>>> }; >>>> >>>> This patch includes only changes inside of videobuf2. So, it is >>>> required to >>>> modify all device drivers which use videobuf2. >>>> >>>> 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 | 324 >>>> +++++++++++++++++------------- >>>> include/media/videobuf2-core.h | 50 ++--- >>>> include/media/videobuf2-v4l2.h | 26 +++ >>>> 3 files changed, 236 insertions(+), 164 deletions(-) >>>> >>>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c >>>> b/drivers/media/v4l2-core/videobuf2-core.c >>>> index ab00ea0..9266d50 100644 >>>> --- a/drivers/media/v4l2-core/videobuf2-core.c >>>> +++ b/drivers/media/v4l2-core/videobuf2-core.c >>>> @@ -35,10 +35,10 @@ >>>> static int debug; >>>> module_param(debug, int, 0644); >>>> >>>> -#define dprintk(level, fmt, arg...) \ >>>> - do { \ >>>> - if (debug >= level) \ >>>> - pr_info("vb2: %s: " fmt, __func__, ## arg); \ >>>> +#define dprintk(level, fmt, arg...) \ >>>> + do { \ >>>> + if (debug >= level) \ >>>> + pr_info("vb2: %s: " fmt, __func__, ## arg); \ >>> >>> These are just whitespace changes, and that is something it see *a >>> lot* in this >>> patch. And usually for no clear reason. >>> >>> Please remove those whitespace changes, it makes a difficult patch >>> even harder >>> to read than it already is. >>> >> >> I just wanted to remove unnecessary white spaces or adjust alignment. >> OK, I will revert those whitespace changes for better review. >> >>>> } while (0) >>>> >>>> #ifdef CONFIG_VIDEO_ADV_DEBUG >>> >>> <snip> >>> >>>> @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) >>>> */ >>>> static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct >>>> v4l2_buffer *b) >>>> { >>>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); >>>> struct vb2_queue *q = vb->vb2_queue; >>>> + unsigned int plane; >>>> >>>> /* Copy back data such as timestamp, flags, etc. */ >>>> - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); >>>> - b->reserved2 = vb->v4l2_buf.reserved2; >>>> - b->reserved = vb->v4l2_buf.reserved; >>> >>> Hmm, I'm not sure why these reserved fields were copied here. I think >>> it was >>> for compatibility reasons for some old drivers that abused the >>> reserved field. >>> However, in the new code these reserved fields should probably be >>> explicitly >>> initialized to 0. >>> >>>> + b->index = vb->index; >>>> + b->type = vb->type; >>>> + b->memory = vb->memory; >>>> + b->bytesused = 0; >>>> + >>>> + b->flags = vbuf->flags; >>>> + b->field = vbuf->field; >>>> + b->timestamp = vbuf->timestamp; >>>> + b->timecode = vbuf->timecode; >>>> + b->sequence = vbuf->sequence; >>>> >>>> if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { >>>> /* >>>> @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct >>>> vb2_buffer *vb, struct v4l2_buffer *b) >>>> * for it. The caller has already verified memory and size. >>>> */ >>>> b->length = vb->num_planes; >>>> - memcpy(b->m.planes, vb->v4l2_planes, >>>> - b->length * sizeof(struct v4l2_plane)); >>> >>> A similar problem occurs here: the memcpy would have copied the reserved >>> field in v4l2_plane as well, but that no longer happens, so you need to >>> do an explicit memset for the reserved field in the new code. >>> >> >> It means that I'd better add reserved fields to struct vb2_buffer and >> struct vb2_plane in order to keep the information in struct v4l2_buffer >> and struct v4l2_plane??? >> > > Oh, I've mistaken your point. > Now I got your point. > In __fill_v4l2_buffer(), just initialize explicitly the reserved > field like : > memset(pdst->reserved, 0 sizeof(pdst->reserved)); > Right? Right. > > >> >>>> + for (plane = 0; plane < vb->num_planes; ++plane) { >>>> + struct v4l2_plane *pdst = &b->m.planes[plane]; >>>> + struct vb2_plane *psrc = &vb->planes[plane]; >>>> + >>>> + pdst->bytesused = psrc->bytesused; >>>> + pdst->length = psrc->length; >>>> + if (q->memory == V4L2_MEMORY_MMAP) >>>> + pdst->m.mem_offset = psrc->m.offset; >>>> + else if (q->memory == V4L2_MEMORY_USERPTR) >>>> + pdst->m.userptr = psrc->m.userptr; >>>> + else if (q->memory == V4L2_MEMORY_DMABUF) >>>> + pdst->m.fd = psrc->m.fd; >>>> + pdst->data_offset = psrc->data_offset; >>>> + } >>>> } else { >>>> /* >>>> * We use length and offset in v4l2_planes array even for >>>> * single-planar buffers, but userspace does not. >>>> */ >>>> - b->length = vb->v4l2_planes[0].length; >>>> - b->bytesused = vb->v4l2_planes[0].bytesused; >>>> + b->length = vb->planes[0].length; >>>> + b->bytesused = vb->planes[0].bytesused; >>>> if (q->memory == V4L2_MEMORY_MMAP) >>>> - b->m.offset = vb->v4l2_planes[0].m.mem_offset; >>>> + b->m.offset = vb->planes[0].m.offset; >>>> else if (q->memory == V4L2_MEMORY_USERPTR) >>>> - b->m.userptr = vb->v4l2_planes[0].m.userptr; >>>> + b->m.userptr = vb->planes[0].m.userptr; >>>> else if (q->memory == V4L2_MEMORY_DMABUF) >>>> - b->m.fd = vb->v4l2_planes[0].m.fd; >>>> + b->m.fd = vb->planes[0].m.fd; >>>> } >>>> >>>> /* >>>> @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer >>>> *vb, struct v4l2_buffer *b) >>>> b->flags &= ~V4L2_BUFFER_MASK_FLAGS; >>>> b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; >>>> if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != >>>> - V4L2_BUF_FLAG_TIMESTAMP_COPY) { >>>> + V4L2_BUF_FLAG_TIMESTAMP_COPY) { >>>> /* >>>> * For non-COPY timestamps, drop timestamp source bits >>>> * and obtain the timestamp source from the queue. >>>> @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); >>>> static int __verify_userptr_ops(struct vb2_queue *q) >>>> { >>>> if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || >>>> - !q->mem_ops->put_userptr) >>>> + !q->mem_ops->put_userptr) >>>> return -EINVAL; >>>> >>>> return 0; >>>> @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) >>>> static int __verify_mmap_ops(struct vb2_queue *q) >>>> { >>>> if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || >>>> - !q->mem_ops->put || !q->mem_ops->mmap) >>>> + !q->mem_ops->put || !q->mem_ops->mmap) >>>> return -EINVAL; >>>> >>>> return 0; >>>> @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) >>>> static int __verify_dmabuf_ops(struct vb2_queue *q) >>>> { >>>> if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || >>>> - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >>>> - !q->mem_ops->unmap_dmabuf) >>>> + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || >>>> + !q->mem_ops->unmap_dmabuf) >>>> return -EINVAL; >>>> >>>> return 0; >>>> @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, >>>> enum v4l2_memory memory, enum v4l2_buf_type type) >>>> { >>>> if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && >>>> - memory != V4L2_MEMORY_DMABUF) { >>>> + memory != V4L2_MEMORY_DMABUF) { >>>> dprintk(1, "unsupported memory type\n"); >>>> return -EINVAL; >>>> } >>>> @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct >>>> v4l2_requestbuffers *req) >>>> * Driver also sets the size and allocator context for each plane. >>>> */ >>>> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, >>>> - q->plane_sizes, q->alloc_ctx); >>>> + q->plane_sizes, q->alloc_ctx); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct >>>> v4l2_requestbuffers *req) >>>> num_buffers = allocated_buffers; >>>> >>>> ret = call_qop(q, queue_setup, q, NULL, &num_buffers, >>>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>>> >>>> if (!ret && allocated_buffers < num_buffers) >>>> ret = -ENOMEM; >>>> @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, >>>> struct v4l2_create_buffers *create >>>> * buffer and their sizes are acceptable >>>> */ >>>> ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, >>>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>>> if (ret) >>>> return ret; >>>> >>>> @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, >>>> struct v4l2_create_buffers *create >>>> * queue driver has set up >>>> */ >>>> ret = call_qop(q, queue_setup, q, &create->format, >>>> &num_buffers, >>>> - &num_planes, q->plane_sizes, q->alloc_ctx); >>>> + &num_planes, q->plane_sizes, q->alloc_ctx); >>>> >>>> if (!ret && allocated_buffers < num_buffers) >>>> ret = -ENOMEM; >>>> @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, >>>> enum vb2_buffer_state state) >>>> return; >>>> >>>> if (WARN_ON(state != VB2_BUF_STATE_DONE && >>>> - state != VB2_BUF_STATE_ERROR && >>>> - state != VB2_BUF_STATE_QUEUED)) >>>> + state != VB2_BUF_STATE_ERROR && >>>> + state != VB2_BUF_STATE_QUEUED)) >>>> state = VB2_BUF_STATE_ERROR; >>>> >>>> #ifdef CONFIG_VIDEO_ADV_DEBUG >>> >>> All the chunks above are all spurious whitespace changes. As mentioned >>> in the beginning, >>> please remove all those pointless whitespace changes! >>> >>> There are a lot more of these, but I won't comment on them anymore. >>> >>> Basically this patch looks good to me, so once I have the next version >>> without all the >>> whitespace confusion and with the reserved field issues solved I'll do >>> a final review. >>> >>> BTW, did you test this with 'v4l2-compliance -s' and with the vivid >>> driver? Just to >>> make sure you didn't break anything. >>> >> >> Actually, I've tested with v4l2-compliance for just one v4l2 drivers - >> au0828. >> I'll try to test with vivid driver at next round. >> > > I tried to use vivid for test. But, I have failed to install the > module (vivid.ko). > I mainly referred to documentation/video4linux/vivid.txt. But, it > not enough to test. > Could you give me a guide? modprobe vivid no_error_inj=1 v4l2-compliance -d /dev/video0 -s v4l2-compliance -d /dev/video1 -s I always forget to pass the module option... The vivid module has controls for error injection which are all disable with that module option. Otherwise all the error injection controls would be used by v4l2-compliance and of course you'd get zillions of errors :-) Note that you will get two identical failures when testing /dev/video1: fail: v4l2-test-controls.cpp(243): no controls in class 00f00000 This is a vivid driver bug and you can ignore those two fails. You also want to test with the vim2m test driver: modprobe vim2m v4l2-compliance -d /dev/video2 -s Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index ab00ea0..9266d50 100644 --- a/drivers/media/v4l2-core/videobuf2-core.c +++ b/drivers/media/v4l2-core/videobuf2-core.c @@ -35,10 +35,10 @@ static int debug; module_param(debug, int, 0644); -#define dprintk(level, fmt, arg...) \ - do { \ - if (debug >= level) \ - pr_info("vb2: %s: " fmt, __func__, ## arg); \ +#define dprintk(level, fmt, arg...) \ + do { \ + if (debug >= level) \ + pr_info("vb2: %s: " fmt, __func__, ## arg); \ } while (0) #ifdef CONFIG_VIDEO_ADV_DEBUG @@ -53,7 +53,7 @@ module_param(debug, int, 0644); #define log_memop(vb, op) \ dprintk(2, "call_memop(%p, %d, %s)%s\n", \ - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ + (vb)->vb2_queue, (vb)->index, #op, \ (vb)->vb2_queue->mem_ops->op ? "" : " (nop)") #define call_memop(vb, op, args...) \ @@ -115,7 +115,7 @@ module_param(debug, int, 0644); #define log_vb_qop(vb, op, args...) \ dprintk(2, "call_vb_qop(%p, %d, %s)%s\n", \ - (vb)->vb2_queue, (vb)->v4l2_buf.index, #op, \ + (vb)->vb2_queue, (vb)->index, #op, \ (vb)->vb2_queue->ops->op ? "" : " (nop)") #define call_vb_qop(vb, op, args...) \ @@ -205,13 +205,13 @@ static int __vb2_buf_mem_alloc(struct vb2_buffer *vb) unsigned long size = PAGE_ALIGN(q->plane_sizes[plane]); mem_priv = call_ptr_memop(vb, alloc, q->alloc_ctx[plane], - size, dma_dir, q->gfp_flags); + size, dma_dir, q->gfp_flags); if (IS_ERR_OR_NULL(mem_priv)) goto free; /* Associate allocator private data with this plane */ vb->planes[plane].mem_priv = mem_priv; - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; + vb->planes[plane].length = q->plane_sizes[plane]; } return 0; @@ -235,8 +235,7 @@ static void __vb2_buf_mem_free(struct vb2_buffer *vb) for (plane = 0; plane < vb->num_planes; ++plane) { call_void_memop(vb, put, vb->planes[plane].mem_priv); vb->planes[plane].mem_priv = NULL; - dprintk(3, "freed plane %d of buffer %d\n", plane, - vb->v4l2_buf.index); + dprintk(3, "freed plane %d of buffer %d\n", plane, vb->index); } } @@ -269,7 +268,9 @@ static void __vb2_plane_dmabuf_put(struct vb2_buffer *vb, struct vb2_plane *p) call_void_memop(vb, detach_dmabuf, p->mem_priv); dma_buf_put(p->dbuf); - memset(p, 0, sizeof(*p)); + p->mem_priv = NULL; + p->dbuf = NULL; + p->dbuf_mapped = 0; } /** @@ -299,7 +300,7 @@ static void __setup_lengths(struct vb2_queue *q, unsigned int n) continue; for (plane = 0; plane < vb->num_planes; ++plane) - vb->v4l2_planes[plane].length = q->plane_sizes[plane]; + vb->planes[plane].length = q->plane_sizes[plane]; } } @@ -314,10 +315,10 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) unsigned long off; if (q->num_buffers) { - struct v4l2_plane *p; + struct vb2_plane *p; vb = q->bufs[q->num_buffers - 1]; - p = &vb->v4l2_planes[vb->num_planes - 1]; - off = PAGE_ALIGN(p->m.mem_offset + p->length); + p = &vb->planes[vb->num_planes - 1]; + off = PAGE_ALIGN(p->m.offset + p->length); } else { off = 0; } @@ -328,12 +329,12 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) continue; for (plane = 0; plane < vb->num_planes; ++plane) { - vb->v4l2_planes[plane].m.mem_offset = off; + vb->planes[plane].m.offset = off; dprintk(3, "buffer %d, plane %d offset 0x%08lx\n", buffer, plane, off); - off += vb->v4l2_planes[plane].length; + off += vb->planes[plane].length; off = PAGE_ALIGN(off); } } @@ -347,7 +348,7 @@ static void __setup_offsets(struct vb2_queue *q, unsigned int n) * 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 int num_buffers, unsigned int num_planes) { unsigned int buffer; struct vb2_buffer *vb; @@ -361,16 +362,12 @@ static int __vb2_queue_alloc(struct vb2_queue *q, enum v4l2_memory memory, 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 = q->num_buffers + buffer; - vb->v4l2_buf.type = q->type; - vb->v4l2_buf.memory = memory; + vb->index = q->num_buffers + buffer; + vb->type = q->type; + vb->memory = memory; /* Allocate video buffer memory for the MMAP type */ if (memory == V4L2_MEMORY_MMAP) { @@ -418,7 +415,7 @@ static void __vb2_free_mem(struct vb2_queue *q, unsigned int buffers) struct vb2_buffer *vb; for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; - ++buffer) { + ++buffer) { vb = q->bufs[buffer]; if (!vb) continue; @@ -451,7 +448,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) * just return -EAGAIN. */ for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; - ++buffer) { + ++buffer) { if (q->bufs[buffer] == NULL) continue; if (q->bufs[buffer]->state == VB2_BUF_STATE_PREPARING) { @@ -462,7 +459,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) /* Call driver-provided cleanup function for each buffer, if provided */ for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; - ++buffer) { + ++buffer) { struct vb2_buffer *vb = q->bufs[buffer]; if (vb && vb->planes[0].mem_priv) @@ -536,7 +533,7 @@ static int __vb2_queue_free(struct vb2_queue *q, unsigned int buffers) /* Free videobuf buffers */ for (buffer = q->num_buffers - buffers; buffer < q->num_buffers; - ++buffer) { + ++buffer) { kfree(q->bufs[buffer]); q->bufs[buffer] = NULL; } @@ -590,23 +587,22 @@ static int __verify_length(struct vb2_buffer *vb, const struct v4l2_buffer *b) if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { for (plane = 0; plane < vb->num_planes; ++plane) { length = (b->memory == V4L2_MEMORY_USERPTR || - b->memory == V4L2_MEMORY_DMABUF) - ? b->m.planes[plane].length - : vb->v4l2_planes[plane].length; + b->memory == V4L2_MEMORY_DMABUF) + ? b->m.planes[plane].length + : vb->planes[plane].length; bytesused = b->m.planes[plane].bytesused - ? b->m.planes[plane].bytesused : length; + ? b->m.planes[plane].bytesused : length; if (b->m.planes[plane].bytesused > length) return -EINVAL; if (b->m.planes[plane].data_offset > 0 && - b->m.planes[plane].data_offset >= bytesused) + b->m.planes[plane].data_offset >= bytesused) return -EINVAL; } } else { length = (b->memory == V4L2_MEMORY_USERPTR) - ? b->length : vb->v4l2_planes[0].length; - bytesused = b->bytesused ? b->bytesused : length; + ? b->length : vb->planes[0].length; if (b->bytesused > length) return -EINVAL; @@ -656,12 +652,21 @@ static bool __buffers_in_use(struct vb2_queue *q) */ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) { + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); struct vb2_queue *q = vb->vb2_queue; + unsigned int plane; /* Copy back data such as timestamp, flags, etc. */ - memcpy(b, &vb->v4l2_buf, offsetof(struct v4l2_buffer, m)); - b->reserved2 = vb->v4l2_buf.reserved2; - b->reserved = vb->v4l2_buf.reserved; + b->index = vb->index; + b->type = vb->type; + b->memory = vb->memory; + b->bytesused = 0; + + b->flags = vbuf->flags; + b->field = vbuf->field; + b->timestamp = vbuf->timestamp; + b->timecode = vbuf->timecode; + b->sequence = vbuf->sequence; if (V4L2_TYPE_IS_MULTIPLANAR(q->type)) { /* @@ -669,21 +674,33 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) * for it. The caller has already verified memory and size. */ b->length = vb->num_planes; - memcpy(b->m.planes, vb->v4l2_planes, - b->length * sizeof(struct v4l2_plane)); + for (plane = 0; plane < vb->num_planes; ++plane) { + struct v4l2_plane *pdst = &b->m.planes[plane]; + struct vb2_plane *psrc = &vb->planes[plane]; + + pdst->bytesused = psrc->bytesused; + pdst->length = psrc->length; + if (q->memory == V4L2_MEMORY_MMAP) + pdst->m.mem_offset = psrc->m.offset; + else if (q->memory == V4L2_MEMORY_USERPTR) + pdst->m.userptr = psrc->m.userptr; + else if (q->memory == V4L2_MEMORY_DMABUF) + pdst->m.fd = psrc->m.fd; + pdst->data_offset = psrc->data_offset; + } } else { /* * We use length and offset in v4l2_planes array even for * single-planar buffers, but userspace does not. */ - b->length = vb->v4l2_planes[0].length; - b->bytesused = vb->v4l2_planes[0].bytesused; + b->length = vb->planes[0].length; + b->bytesused = vb->planes[0].bytesused; if (q->memory == V4L2_MEMORY_MMAP) - b->m.offset = vb->v4l2_planes[0].m.mem_offset; + b->m.offset = vb->planes[0].m.offset; else if (q->memory == V4L2_MEMORY_USERPTR) - b->m.userptr = vb->v4l2_planes[0].m.userptr; + b->m.userptr = vb->planes[0].m.userptr; else if (q->memory == V4L2_MEMORY_DMABUF) - b->m.fd = vb->v4l2_planes[0].m.fd; + b->m.fd = vb->planes[0].m.fd; } /* @@ -692,7 +709,7 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b) b->flags &= ~V4L2_BUFFER_MASK_FLAGS; b->flags |= q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK; if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != - V4L2_BUF_FLAG_TIMESTAMP_COPY) { + V4L2_BUF_FLAG_TIMESTAMP_COPY) { /* * For non-COPY timestamps, drop timestamp source bits * and obtain the timestamp source from the queue. @@ -767,7 +784,7 @@ EXPORT_SYMBOL(vb2_querybuf); static int __verify_userptr_ops(struct vb2_queue *q) { if (!(q->io_modes & VB2_USERPTR) || !q->mem_ops->get_userptr || - !q->mem_ops->put_userptr) + !q->mem_ops->put_userptr) return -EINVAL; return 0; @@ -780,7 +797,7 @@ static int __verify_userptr_ops(struct vb2_queue *q) static int __verify_mmap_ops(struct vb2_queue *q) { if (!(q->io_modes & VB2_MMAP) || !q->mem_ops->alloc || - !q->mem_ops->put || !q->mem_ops->mmap) + !q->mem_ops->put || !q->mem_ops->mmap) return -EINVAL; return 0; @@ -793,8 +810,8 @@ static int __verify_mmap_ops(struct vb2_queue *q) static int __verify_dmabuf_ops(struct vb2_queue *q) { if (!(q->io_modes & VB2_DMABUF) || !q->mem_ops->attach_dmabuf || - !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || - !q->mem_ops->unmap_dmabuf) + !q->mem_ops->detach_dmabuf || !q->mem_ops->map_dmabuf || + !q->mem_ops->unmap_dmabuf) return -EINVAL; return 0; @@ -808,7 +825,7 @@ static int __verify_memory_type(struct vb2_queue *q, enum v4l2_memory memory, enum v4l2_buf_type type) { if (memory != V4L2_MEMORY_MMAP && memory != V4L2_MEMORY_USERPTR && - memory != V4L2_MEMORY_DMABUF) { + memory != V4L2_MEMORY_DMABUF) { dprintk(1, "unsupported memory type\n"); return -EINVAL; } @@ -927,7 +944,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) * Driver also sets the size and allocator context for each plane. */ ret = call_qop(q, queue_setup, q, NULL, &num_buffers, &num_planes, - q->plane_sizes, q->alloc_ctx); + q->plane_sizes, q->alloc_ctx); if (ret) return ret; @@ -952,7 +969,7 @@ static int __reqbufs(struct vb2_queue *q, struct v4l2_requestbuffers *req) num_buffers = allocated_buffers; ret = call_qop(q, queue_setup, q, NULL, &num_buffers, - &num_planes, q->plane_sizes, q->alloc_ctx); + &num_planes, q->plane_sizes, q->alloc_ctx); if (!ret && allocated_buffers < num_buffers) ret = -ENOMEM; @@ -1040,7 +1057,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create * buffer and their sizes are acceptable */ ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, - &num_planes, q->plane_sizes, q->alloc_ctx); + &num_planes, q->plane_sizes, q->alloc_ctx); if (ret) return ret; @@ -1063,7 +1080,7 @@ static int __create_bufs(struct vb2_queue *q, struct v4l2_create_buffers *create * queue driver has set up */ ret = call_qop(q, queue_setup, q, &create->format, &num_buffers, - &num_planes, q->plane_sizes, q->alloc_ctx); + &num_planes, q->plane_sizes, q->alloc_ctx); if (!ret && allocated_buffers < num_buffers) ret = -ENOMEM; @@ -1183,8 +1200,8 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) return; if (WARN_ON(state != VB2_BUF_STATE_DONE && - state != VB2_BUF_STATE_ERROR && - state != VB2_BUF_STATE_QUEUED)) + state != VB2_BUF_STATE_ERROR && + state != VB2_BUF_STATE_QUEUED)) state = VB2_BUF_STATE_ERROR; #ifdef CONFIG_VIDEO_ADV_DEBUG @@ -1195,7 +1212,7 @@ void vb2_buffer_done(struct vb2_buffer *vb, enum vb2_buffer_state state) vb->cnt_buf_done++; #endif dprintk(4, "done processing on buffer %d, state: %d\n", - vb->v4l2_buf.index, state); + vb->index, state); /* sync buffers */ for (plane = 0; plane < vb->num_planes; ++plane) @@ -1268,25 +1285,26 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer *vb) * v4l2_buffer by the userspace. The caller has already verified that struct * v4l2_buffer has a valid number of planes. */ -static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b, - struct v4l2_plane *v4l2_planes) +static void __fill_vb2_buffer(struct vb2_buffer *vb, + const struct v4l2_buffer *b, struct vb2_plane *planes) { + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); unsigned int plane; if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { if (b->memory == V4L2_MEMORY_USERPTR) { for (plane = 0; plane < vb->num_planes; ++plane) { - v4l2_planes[plane].m.userptr = + planes[plane].m.userptr = b->m.planes[plane].m.userptr; - v4l2_planes[plane].length = + planes[plane].length = b->m.planes[plane].length; } } if (b->memory == V4L2_MEMORY_DMABUF) { for (plane = 0; plane < vb->num_planes; ++plane) { - v4l2_planes[plane].m.fd = + planes[plane].m.fd = b->m.planes[plane].m.fd; - v4l2_planes[plane].length = + planes[plane].length = b->m.planes[plane].length; } } @@ -1310,7 +1328,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b * applications working. */ for (plane = 0; plane < vb->num_planes; ++plane) { - struct v4l2_plane *pdst = &v4l2_planes[plane]; + struct vb2_plane *pdst = &planes[plane]; struct v4l2_plane *psrc = &b->m.planes[plane]; if (psrc->bytesused == 0) @@ -1340,13 +1358,13 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b * old userspace applications working. */ if (b->memory == V4L2_MEMORY_USERPTR) { - v4l2_planes[0].m.userptr = b->m.userptr; - v4l2_planes[0].length = b->length; + planes[0].m.userptr = b->m.userptr; + planes[0].length = b->length; } if (b->memory == V4L2_MEMORY_DMABUF) { - v4l2_planes[0].m.fd = b->m.fd; - v4l2_planes[0].length = b->length; + planes[0].m.fd = b->m.fd; + planes[0].length = b->length; } if (V4L2_TYPE_IS_OUTPUT(b->type)) { @@ -1354,25 +1372,26 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b vb2_warn_zero_bytesused(vb); if (vb->vb2_queue->allow_zero_bytesused) - v4l2_planes[0].bytesused = b->bytesused; + planes[0].bytesused = b->bytesused; else - v4l2_planes[0].bytesused = b->bytesused ? - b->bytesused : v4l2_planes[0].length; + planes[0].bytesused = b->bytesused ? + b->bytesused : planes[0].length; } else - v4l2_planes[0].bytesused = 0; + planes[0].bytesused = 0; } /* Zero flags that the vb2 core handles */ - vb->v4l2_buf.flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; + vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS; if ((vb->vb2_queue->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) != - V4L2_BUF_FLAG_TIMESTAMP_COPY || !V4L2_TYPE_IS_OUTPUT(b->type)) { + V4L2_BUF_FLAG_TIMESTAMP_COPY || + !V4L2_TYPE_IS_OUTPUT(b->type)) { /* * Non-COPY timestamps and non-OUTPUT queues will get * their timestamp and timestamp source flags from the * queue. */ - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; + vbuf->flags &= ~V4L2_BUF_FLAG_TSTAMP_SRC_MASK; } if (V4L2_TYPE_IS_OUTPUT(b->type)) { @@ -1382,11 +1401,11 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b * The 'field' is valid metadata for this output buffer * and so that needs to be copied here. */ - vb->v4l2_buf.flags &= ~V4L2_BUF_FLAG_TIMECODE; - vb->v4l2_buf.field = b->field; + vbuf->flags &= ~V4L2_BUF_FLAG_TIMECODE; + vbuf->field = b->field; } else { /* Zero any output buffer flags as this is a capture buffer */ - vb->v4l2_buf.flags &= ~V4L2_BUFFER_OUT_FLAGS; + vbuf->flags &= ~V4L2_BUFFER_OUT_FLAGS; } } @@ -1395,7 +1414,7 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct v4l2_buffer *b */ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) { - __fill_vb2_buffer(vb, b, vb->v4l2_planes); + __fill_vb2_buffer(vb, b, vb->planes); return call_vb_qop(vb, buf_prepare, vb); } @@ -1404,7 +1423,7 @@ static int __qbuf_mmap(struct vb2_buffer *vb, const struct v4l2_buffer *b) */ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) { - struct v4l2_plane planes[VIDEO_MAX_PLANES]; + struct vb2_plane planes[VIDEO_MAX_PLANES]; struct vb2_queue *q = vb->vb2_queue; void *mem_priv; unsigned int plane; @@ -1419,9 +1438,9 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) for (plane = 0; plane < vb->num_planes; ++plane) { /* Skip the plane if already verified */ - if (vb->v4l2_planes[plane].m.userptr && - vb->v4l2_planes[plane].m.userptr == planes[plane].m.userptr - && vb->v4l2_planes[plane].length == planes[plane].length) + if (vb->planes[plane].m.userptr && + vb->planes[plane].m.userptr == planes[plane].m.userptr + && vb->planes[plane].length == planes[plane].length) continue; dprintk(3, "userspace address for plane %d changed, " @@ -1447,12 +1466,15 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) } vb->planes[plane].mem_priv = NULL; - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); + vb->planes[plane].bytesused = 0; + vb->planes[plane].length = 0; + vb->planes[plane].m.userptr = 0; + vb->planes[plane].data_offset = 0; /* Acquire each plane's memory */ mem_priv = call_ptr_memop(vb, get_userptr, q->alloc_ctx[plane], - planes[plane].m.userptr, - planes[plane].length, dma_dir); + planes[plane].m.userptr, + planes[plane].length, dma_dir); if (IS_ERR_OR_NULL(mem_priv)) { dprintk(1, "failed acquiring userspace " "memory for plane %d\n", plane); @@ -1466,8 +1488,12 @@ static int __qbuf_userptr(struct vb2_buffer *vb, const struct v4l2_buffer *b) * Now that everything is in order, copy relevant information * provided by userspace. */ - for (plane = 0; plane < vb->num_planes; ++plane) - vb->v4l2_planes[plane] = planes[plane]; + for (plane = 0; plane < vb->num_planes; ++plane) { + vb->planes[plane].bytesused = planes[plane].bytesused; + vb->planes[plane].length = planes[plane].length; + vb->planes[plane].m.userptr = planes[plane].m.userptr; + vb->planes[plane].data_offset = planes[plane].data_offset; + } if (reacquired) { /* @@ -1494,10 +1520,11 @@ err: /* In case of errors, release planes that were already acquired */ for (plane = 0; plane < vb->num_planes; ++plane) { if (vb->planes[plane].mem_priv) - call_void_memop(vb, put_userptr, vb->planes[plane].mem_priv); + call_void_memop(vb, put_userptr, + vb->planes[plane].mem_priv); vb->planes[plane].mem_priv = NULL; - vb->v4l2_planes[plane].m.userptr = 0; - vb->v4l2_planes[plane].length = 0; + vb->planes[plane].m.userptr = 0; + vb->planes[plane].length = 0; } return ret; @@ -1508,7 +1535,7 @@ err: */ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) { - struct v4l2_plane planes[VIDEO_MAX_PLANES]; + struct vb2_plane planes[VIDEO_MAX_PLANES]; struct vb2_queue *q = vb->vb2_queue; void *mem_priv; unsigned int plane; @@ -1544,7 +1571,7 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) /* Skip the plane if already verified */ if (dbuf == vb->planes[plane].dbuf && - vb->v4l2_planes[plane].length == planes[plane].length) { + vb->planes[plane].length == planes[plane].length) { dma_buf_put(dbuf); continue; } @@ -1558,11 +1585,15 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) /* Release previously acquired memory if present */ __vb2_plane_dmabuf_put(vb, &vb->planes[plane]); - memset(&vb->v4l2_planes[plane], 0, sizeof(struct v4l2_plane)); + vb->planes[plane].bytesused = 0; + vb->planes[plane].length = 0; + vb->planes[plane].m.fd = 0; + vb->planes[plane].data_offset = 0; /* Acquire each plane's memory */ - mem_priv = call_ptr_memop(vb, attach_dmabuf, q->alloc_ctx[plane], - dbuf, planes[plane].length, dma_dir); + mem_priv = call_ptr_memop(vb, attach_dmabuf, + q->alloc_ctx[plane], dbuf, planes[plane].length, + dma_dir); if (IS_ERR(mem_priv)) { dprintk(1, "failed to attach dmabuf\n"); ret = PTR_ERR(mem_priv); @@ -1592,8 +1623,12 @@ static int __qbuf_dmabuf(struct vb2_buffer *vb, const struct v4l2_buffer *b) * Now that everything is in order, copy relevant information * provided by userspace. */ - for (plane = 0; plane < vb->num_planes; ++plane) - vb->v4l2_planes[plane] = planes[plane]; + for (plane = 0; plane < vb->num_planes; ++plane) { + vb->planes[plane].bytesused = planes[plane].bytesused; + vb->planes[plane].length = planes[plane].length; + vb->planes[plane].m.fd = planes[plane].m.userptr; + vb->planes[plane].data_offset = planes[plane].data_offset; + } if (reacquired) { /* @@ -1644,6 +1679,7 @@ static void __enqueue_in_driver(struct vb2_buffer *vb) static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) { + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb); struct vb2_queue *q = vb->vb2_queue; int ret; @@ -1672,9 +1708,9 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) } vb->state = VB2_BUF_STATE_PREPARING; - vb->v4l2_buf.timestamp.tv_sec = 0; - vb->v4l2_buf.timestamp.tv_usec = 0; - vb->v4l2_buf.sequence = 0; + vbuf->timestamp.tv_sec = 0; + vbuf->timestamp.tv_usec = 0; + vbuf->sequence = 0; switch (q->memory) { case V4L2_MEMORY_MMAP: @@ -1701,7 +1737,7 @@ static int __buf_prepare(struct vb2_buffer *vb, const struct v4l2_buffer *b) } static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b, - const char *opname) + const char *opname) { if (b->type != q->type) { dprintk(1, "%s: invalid buffer type\n", opname); @@ -1768,7 +1804,7 @@ int vb2_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b) /* Fill buffer information for the userspace */ __fill_v4l2_buffer(vb, b); - dprintk(1, "prepare of buffer %d succeeded\n", vb->v4l2_buf.index); + dprintk(1, "prepare of buffer %d succeeded\n", vb->index); } return ret; } @@ -1800,7 +1836,7 @@ static int vb2_start_streaming(struct vb2_queue *q) /* Tell the driver to start streaming */ q->start_streaming_called = 1; ret = call_qop(q, start_streaming, q, - atomic_read(&q->owned_by_drv_count)); + atomic_read(&q->owned_by_drv_count)); if (!ret) return 0; @@ -1810,7 +1846,7 @@ static int vb2_start_streaming(struct vb2_queue *q) /* * If you see this warning, then the driver isn't cleaning up properly * after a failed start_streaming(). See the start_streaming() - * documentation in videobuf2-v4l2.h for more information how buffers + * documentation in videobuf2-core.h for more information how buffers * should be returned to vb2 in start_streaming(). */ if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { @@ -1841,11 +1877,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) { int ret = vb2_queue_or_prepare_buf(q, b, "qbuf"); struct vb2_buffer *vb; + struct vb2_v4l2_buffer *vbuf; if (ret) return ret; vb = q->bufs[b->index]; + vbuf = to_vb2_v4l2_buffer(vb); switch (vb->state) { case VB2_BUF_STATE_DEQUEUED: @@ -1877,11 +1915,11 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) * and the timecode field and flag if needed. */ if ((q->timestamp_flags & V4L2_BUF_FLAG_TIMESTAMP_MASK) == - V4L2_BUF_FLAG_TIMESTAMP_COPY) - vb->v4l2_buf.timestamp = b->timestamp; - vb->v4l2_buf.flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; + V4L2_BUF_FLAG_TIMESTAMP_COPY) + vbuf->timestamp = b->timestamp; + vbuf->flags |= b->flags & V4L2_BUF_FLAG_TIMECODE; if (b->flags & V4L2_BUF_FLAG_TIMECODE) - vb->v4l2_buf.timecode = b->timecode; + vbuf->timecode = b->timecode; } trace_vb2_qbuf(q, vb); @@ -1903,13 +1941,13 @@ static int vb2_internal_qbuf(struct vb2_queue *q, struct v4l2_buffer *b) * then we can finally call start_streaming(). */ if (q->streaming && !q->start_streaming_called && - q->queued_count >= q->min_buffers_needed) { + q->queued_count >= q->min_buffers_needed) { ret = vb2_start_streaming(q); if (ret) return ret; } - dprintk(1, "qbuf of buffer %d succeeded\n", vb->v4l2_buf.index); + dprintk(1, "qbuf of buffer %d succeeded\n", vb->index); return 0; } @@ -2099,9 +2137,11 @@ static void __vb2_dqbuf(struct vb2_buffer *vb) } } -static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool nonblocking) +static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, + bool nonblocking) { struct vb2_buffer *vb = NULL; + struct vb2_v4l2_buffer *vbuf = NULL; int ret; if (b->type != q->type) { @@ -2134,14 +2174,15 @@ static int vb2_internal_dqbuf(struct vb2_queue *q, struct v4l2_buffer *b, bool n trace_vb2_dqbuf(q, vb); + vbuf = to_vb2_v4l2_buffer(vb); if (!V4L2_TYPE_IS_OUTPUT(q->type) && - vb->v4l2_buf.flags & V4L2_BUF_FLAG_LAST) + vbuf->flags & V4L2_BUF_FLAG_LAST) q->last_buffer_dequeued = true; /* go back to dequeued state */ __vb2_dqbuf(vb); dprintk(1, "dqbuf of buffer %d, with state %d\n", - vb->v4l2_buf.index, vb->state); + vb->index, vb->state); return 0; } @@ -2197,7 +2238,7 @@ static void __vb2_queue_cancel(struct vb2_queue *q) /* * If you see this warning, then the driver isn't cleaning up properly * in stop_streaming(). See the stop_streaming() documentation in - * videobuf2-v4l2.h for more information how buffers should be returned + * videobuf2-core.h for more information how buffers should be returned * to vb2 in stop_streaming(). */ if (WARN_ON(atomic_read(&q->owned_by_drv_count))) { @@ -2399,7 +2440,7 @@ static int __find_plane_by_offset(struct vb2_queue *q, unsigned long off, vb = q->bufs[buffer]; for (plane = 0; plane < vb->num_planes; ++plane) { - if (vb->v4l2_planes[plane].m.mem_offset == off) { + if (vb->planes[plane].m.offset == off) { *_buffer = buffer; *_plane = plane; return 0; @@ -2557,7 +2598,7 @@ int vb2_mmap(struct vb2_queue *q, struct vm_area_struct *vma) * The buffer length was page_aligned at __vb2_buf_mem_alloc(), * so, we need to do the same here. */ - length = PAGE_ALIGN(vb->v4l2_planes[plane].length); + length = PAGE_ALIGN(vb->planes[plane].length); if (length < (vma->vm_end - vma->vm_start)) { dprintk(1, "MMAP invalid, as it would overflow buffer length\n"); @@ -2577,10 +2618,10 @@ EXPORT_SYMBOL_GPL(vb2_mmap); #ifndef CONFIG_MMU unsigned long vb2_get_unmapped_area(struct vb2_queue *q, - unsigned long addr, - unsigned long len, - unsigned long pgoff, - unsigned long flags) + unsigned long addr, + unsigned long len, + unsigned long pgoff, + unsigned long flags) { unsigned long off = pgoff << PAGE_SHIFT; struct vb2_buffer *vb; @@ -2731,7 +2772,7 @@ EXPORT_SYMBOL_GPL(vb2_poll); * responsible of clearing it's content and setting initial values for some * required entries before calling this function. * q->ops, q->mem_ops, q->type and q->io_modes are mandatory. Please refer - * to the struct vb2_queue description in include/media/videobuf2-v4l2.h + * to the struct vb2_queue description in include/media/videobuf2-core.h * for more information. */ int vb2_queue_init(struct vb2_queue *q) @@ -2739,16 +2780,16 @@ int vb2_queue_init(struct vb2_queue *q) /* * Sanity check */ - if (WARN_ON(!q) || - WARN_ON(!q->ops) || - WARN_ON(!q->mem_ops) || - WARN_ON(!q->type) || - WARN_ON(!q->io_modes) || - WARN_ON(!q->ops->queue_setup) || - WARN_ON(!q->ops->buf_queue) || - WARN_ON(q->timestamp_flags & - ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | - V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) + if (WARN_ON(!q) || + WARN_ON(!q->ops) || + WARN_ON(!q->mem_ops) || + WARN_ON(!q->type) || + WARN_ON(!q->io_modes) || + WARN_ON(!q->ops->queue_setup) || + WARN_ON(!q->ops->buf_queue) || + WARN_ON(q->timestamp_flags & + ~(V4L2_BUF_FLAG_TIMESTAMP_MASK | + V4L2_BUF_FLAG_TSTAMP_SRC_MASK))) return -EINVAL; /* Warn that the driver should choose an appropriate timestamp type */ @@ -2852,7 +2893,7 @@ static int __vb2_init_fileio(struct vb2_queue *q, int read) * Sanity check */ if (WARN_ON((read && !(q->io_modes & VB2_READ)) || - (!read && !(q->io_modes & VB2_WRITE)))) + (!read && !(q->io_modes & VB2_WRITE)))) return -EINVAL; /* @@ -3063,9 +3104,12 @@ static size_t __vb2_perform_fileio(struct vb2_queue *q, char __user *data, size_ buf->queued = 0; buf->size = read ? vb2_get_plane_payload(q->bufs[index], 0) : vb2_plane_size(q->bufs[index], 0); - /* Compensate for data_offset on read in the multiplanar case. */ + /* + * Compensate for data_offset on read + * in the multiplanar case + */ if (is_multiplanar && read && - fileio->b.m.planes[0].data_offset < buf->size) { + fileio->b.m.planes[0].data_offset < buf->size) { buf->pos = fileio->b.m.planes[0].data_offset; buf->size -= buf->pos; } @@ -3257,7 +3301,7 @@ static int vb2_thread(void *data) * contact the linux-media mailinglist first. */ int vb2_thread_start(struct vb2_queue *q, vb2_thread_fnc fnc, void *priv, - const char *thread_name) + const char *thread_name) { struct vb2_threadio_data *threadio; int ret = 0; @@ -3491,7 +3535,7 @@ ssize_t vb2_fop_write(struct file *file, const char __user *buf, if (vb2_queue_is_busy(vdev, file)) goto exit; err = vb2_write(vdev->queue, buf, count, ppos, - file->f_flags & O_NONBLOCK); + file->f_flags & O_NONBLOCK); if (vdev->queue->fileio) vdev->queue->owner = file->private_data; exit: @@ -3515,7 +3559,7 @@ ssize_t vb2_fop_read(struct file *file, char __user *buf, if (vb2_queue_is_busy(vdev, file)) goto exit; err = vb2_read(vdev->queue, buf, count, ppos, - file->f_flags & O_NONBLOCK); + file->f_flags & O_NONBLOCK); if (vdev->queue->fileio) vdev->queue->owner = file->private_data; exit: diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h index 155991e..8787a6c 100644 --- a/include/media/videobuf2-core.h +++ b/include/media/videobuf2-core.h @@ -115,6 +115,16 @@ struct vb2_plane { void *mem_priv; struct dma_buf *dbuf; unsigned int dbuf_mapped; + + /* plane information */ + unsigned int bytesused; + unsigned int length; + union { + unsigned int offset; + unsigned long userptr; + int fd; + } m; + unsigned int data_offset; }; /** @@ -161,41 +171,33 @@ 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 - * @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 + * @index: id number of the buffer + * @type: buffer type + * @memory: the method, in which the actual data is passed + * @num_planes: number of planes in the buffer + * on an internal driver queue * @planes: private per-plane information; do not change */ struct vb2_buffer { - struct v4l2_buffer v4l2_buf; - struct v4l2_plane v4l2_planes[VIDEO_MAX_PLANES]; - struct vb2_queue *vb2_queue; - unsigned int num_planes; - -/* Private: internal use only */ + /* Private: internal use only */ enum vb2_buffer_state state; struct list_head queued_entry; struct list_head done_entry; + /* buffer information */ + unsigned int index; + unsigned int type; + unsigned int memory; + unsigned int num_planes; struct vb2_plane planes[VIDEO_MAX_PLANES]; #ifdef CONFIG_VIDEO_ADV_DEBUG @@ -390,7 +392,7 @@ struct v4l2_fh; * @threadio: thread io internal data, used only if thread is active */ struct vb2_queue { - enum v4l2_buf_type type; + unsigned int type; unsigned int io_modes; unsigned fileio_read_once:1; unsigned fileio_write_immediately:1; @@ -409,7 +411,7 @@ struct vb2_queue { /* private: internal use only */ struct mutex mmap_lock; - enum v4l2_memory memory; + unsigned int memory; struct vb2_buffer *bufs[VIDEO_MAX_FRAME]; unsigned int num_buffers; @@ -571,7 +573,7 @@ static inline void vb2_set_plane_payload(struct vb2_buffer *vb, unsigned int plane_no, unsigned long size) { if (plane_no < vb->num_planes) - vb->v4l2_planes[plane_no].bytesused = size; + vb->planes[plane_no].bytesused = size; } /** @@ -583,7 +585,7 @@ static inline unsigned long vb2_get_plane_payload(struct vb2_buffer *vb, unsigned int plane_no) { if (plane_no < vb->num_planes) - return vb->v4l2_planes[plane_no].bytesused; + return vb->planes[plane_no].bytesused; return 0; } @@ -596,7 +598,7 @@ static inline unsigned long vb2_plane_size(struct vb2_buffer *vb, unsigned int plane_no) { if (plane_no < vb->num_planes) - return vb->v4l2_planes[plane_no].length; + return vb->planes[plane_no].length; return 0; } diff --git a/include/media/videobuf2-v4l2.h b/include/media/videobuf2-v4l2.h index d4a4d9a..fc2dbe9 100644 --- a/include/media/videobuf2-v4l2.h +++ b/include/media/videobuf2-v4l2.h @@ -12,6 +12,32 @@ #ifndef _MEDIA_VIDEOBUF2_V4L2_H #define _MEDIA_VIDEOBUF2_V4L2_H +#include <linux/videodev2.h> #include <media/videobuf2-core.h> +/** + * struct vb2_v4l2_buffer - video buffer information for v4l2 + * @vb2_buf: video buffer 2 + * @flags: buffer informational flags + * @field: enum v4l2_field; field order of the image in the buffer + * @timestamp: frame timestamp + * @timecode: frame timecode + * @sequence: sequence count of this frame + */ +struct vb2_v4l2_buffer { + struct vb2_buffer vb2_buf; + + __u32 flags; + __u32 field; + struct timeval timestamp; + struct v4l2_timecode timecode; + __u32 sequence; +}; + +/** + * to_vb2_v4l2_buffer() - cast struct vb2_buffer * to struct vb2_v4l2_buffer * + */ +#define to_vb2_v4l2_buffer(vb) \ + (container_of(vb, struct vb2_v4l2_buffer, vb2_buf)) + #endif /* _MEDIA_VIDEOBUF2_V4L2_H */
Remove v4l2-specific stuff from struct vb2_buffer and add member variables related with buffer management. struct vb2_plane { <snip> /* plane information */ unsigned int bytesused; unsigned int length; union { unsigned int offset; unsigned long userptr; int fd; } m; unsigned int data_offset; } struct vb2_buffer { <snip> /* buffer information */ unsigned int num_planes; unsigned int index; unsigned int type; unsigned int memory; struct vb2_plane planes[VIDEO_MAX_PLANES]; <snip> }; And create struct vb2_v4l2_buffer as container buffer for v4l2 use. struct vb2_v4l2_buffer { struct vb2_buffer vb2_buf; __u32 flags; __u32 field; struct timeval timestamp; struct v4l2_timecode timecode; __u32 sequence; }; This patch includes only changes inside of videobuf2. So, it is required to modify all device drivers which use videobuf2. 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 | 324 +++++++++++++++++------------- include/media/videobuf2-core.h | 50 ++--- include/media/videobuf2-v4l2.h | 26 +++ 3 files changed, 236 insertions(+), 164 deletions(-)