Em Wed, 15 Aug 2018 13:54:53 +0200
Hans Verkuil <hverkuil@xxxxxxxxx> escreveu:
> 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.
So, not zeroing it if !vb->vb2_queue->is_output is actually a bug fix, right?
Please put such change on a separate patchset then. It could make sense to
c/c stable for it.
>
> 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
Thanks,
Mauro
