On 14/08/18 21:47, Mauro Carvalho Chehab wrote:
> Em Tue, 14 Aug 2018 16:20:31 +0200
> Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
<snip>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-v4l2.c b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> index 57848ddc584f..360dc4e7d413 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-v4l2.c
>> @@ -154,17 +154,11 @@ static void vb2_warn_zero_bytesused(struct vb2_buffer *vb)
>> pr_warn("use the actual size instead.\n");
>> }
>>
>> -/*
>> - * __fill_vb2_buffer() - fill a vb2_buffer with information provided in a
>> - * v4l2_buffer by the userspace. It also verifies that struct
>> - * v4l2_buffer has a valid number of planes.
>> - */
>> -static int __fill_vb2_buffer(struct vb2_buffer *vb,
>> - const void *pb, struct vb2_plane *planes)
>> +static int vb2_fill_vb2_v4l2_buffer(struct vb2_buffer *vb, struct v4l2_buffer *b)
>> {
>> struct vb2_queue *q = vb->vb2_queue;
>> - const struct v4l2_buffer *b = pb;
>> struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + struct vb2_plane *planes = vbuf->planes;
>> unsigned int plane;
>> int ret;
>>
>> @@ -186,7 +180,6 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>> dprintk(1, "the field is incorrectly set to ALTERNATE for an output buffer\n");
>> return -EINVAL;
>> }
>> - vb->timestamp = 0;
>
> See my note below about this removal. On a quick look, I guess we may have
> a regression here for output buffers (non-m2m).
Note that this is no longer the __fill_vb2_buffer() callback, and the timestamp
is not handled in vb2_fill_vb2_v4l2_buffer(). That's why it is removed here.
It is handled in the new __fill_vb2_buffer function, see below for comments.
>
>> vbuf->sequence = 0;
>>
>> if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) {
>> @@ -208,6 +201,12 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>> }
>> break;
>> default:
>> + for (plane = 0; plane < vb->num_planes; ++plane) {
>> + planes[plane].m.offset =
>> + vb->planes[plane].m.offset;
>> + planes[plane].length =
>> + vb->planes[plane].length;
>> + }
>> break;
>> }
>>
>> @@ -269,9 +268,12 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>> planes[0].length = b->length;
>> break;
>> default:
>> + planes[0].m.offset = vb->planes[0].m.offset;
>> + planes[0].length = vb->planes[0].length;
>> break;
>> }
>>
>> + planes[0].data_offset = 0;
>> if (V4L2_TYPE_IS_OUTPUT(b->type)) {
>> if (b->bytesused == 0)
>> vb2_warn_zero_bytesused(vb);
>> @@ -286,7 +288,7 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>>
>> }
>>
>> - /* Zero flags that the vb2 core handles */
>> + /* Zero flags that we handle */
>> vbuf->flags = b->flags & ~V4L2_BUFFER_MASK_FLAGS;
>> if (!vb->vb2_queue->copy_timestamp || !V4L2_TYPE_IS_OUTPUT(b->type)) {
>> /*
>> @@ -319,6 +321,10 @@ static int __fill_vb2_buffer(struct vb2_buffer *vb,
>> static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
>> const char *opname)
>> {
>> + struct vb2_v4l2_buffer *vbuf;
>> + struct vb2_buffer *vb;
>> + int ret;
>> +
>> if (b->type != q->type) {
>> dprintk(1, "%s: invalid buffer type\n", opname);
>> return -EINVAL;
>> @@ -340,7 +346,15 @@ static int vb2_queue_or_prepare_buf(struct vb2_queue *q, struct v4l2_buffer *b,
>> return -EINVAL;
>> }
>>
>> - return __verify_planes_array(q->bufs[b->index], b);
>> + vb = q->bufs[b->index];
>> + vbuf = to_vb2_v4l2_buffer(vb);
>> + ret = __verify_planes_array(vb, b);
>> + if (ret)
>> + return ret;
>> +
>> + /* Copy relevant information provided by the userspace */
>> + memset(vbuf->planes, 0, sizeof(vbuf->planes[0]) * vb->num_planes);
>> + return vb2_fill_vb2_v4l2_buffer(vb, b);
>> }
>>
>> /*
>> @@ -448,6 +462,30 @@ static void __fill_v4l2_buffer(struct vb2_buffer *vb, void *pb)
>> q->last_buffer_dequeued = true;
>> }
>>
>> +/*
>> + * __fill_vb2_buffer() - fill a vb2_buffer with information provided in a
>> + * v4l2_buffer by the userspace. It also verifies that struct
>> + * v4l2_buffer has a valid number of planes.
>> + */
>> +static int __fill_vb2_buffer(struct vb2_buffer *vb, struct vb2_plane *planes)
>> +{
>> + struct vb2_v4l2_buffer *vbuf = to_vb2_v4l2_buffer(vb);
>> + unsigned int plane;
>> +
>> + if (!vb->vb2_queue->is_output || !vb->vb2_queue->copy_timestamp)
>> + vb->timestamp = 0;
>
> When vb->vb2_queue->copy_timestamp is not NULL, timestamp will be copied,
> but how VB2 will fill it if is_output?
vb->vb2_queue->copy_timestamp is a bool, not a pointer. It is true if the timestamps
should be copied from output to capture by the driver.
So the timestamp provided by the application for an output queue that also wants to
copy timestamps needs to be preserved so the driver can copy it to a capture buffer.
In all other cases the vb->timestamp should be zeroed and the driver will fill it in
later when it is done with the capture or output buffer.
Without the request API the sequence during a VIDIOC_QBUF is:
1) call __buf_prepare() which in turn calls the fill_vb2_buffer callback. This zeroed
the timestamp.
2) call the copy_timestamp callback of the vb buffer which fills in the timestamp
from the user-provided v4l2_buffer.
With the request API this no longer works since when you queue a buffer to a request
it is parked internally and not actually queued to the driver until the request itself
is queued. So the sequence in that case is:
1) call copy_timestamp callback to store the user-provided timestamp
And when the request is queued:
2) call __buf_prepare() which in turn calls the fill_vb2_buffer callback.
So the order of calling copy_timestamp and __buf_prepare is now reversed.
I can't call copy_timestamp when the request is queued since I no longer have
access to the original struct v4l2_buffer.
So __fill_vb2_buffer now leaves the timestamp alone for output buffers that
need to copy the timestamp.
>
> I suspect that the right logic here would be just:
>
> if (!vb->vb2_queue->copy_timestamp)
> vb->timestamp = 0;
No, it also needs the !vb->vb2_queue->is_output check: capture queues can also
have vb2_queue->copy_timestamp set. But there it is the driver that will copy
the timestamp from the output side, so we need to zero it here.
Note that v4l2-compliance tests this timestamp handling. And in fact, the changes
I had to make here to do correct timestamp handling for requests were the result
of failures in v4l2-compliance.
A general note: I feel that vb2 is getting a bit too complex and could do with some
refactoring. It is something I want to look at in the future.
Regards,
Hans
