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.s...@samsung.com>
>>>> Signed-off-by: Geunyoung Kim <nenggun....@samsung.com>
>>>> Acked-by: Seung-Woo Kim <sw0312....@samsung.com>
>>>> Acked-by: Inki Dae <inki....@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
