On 15/08/18 14:28, Mauro Carvalho Chehab wrote: > 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? No, it is not a bug fix. The old code worked fine since the timestamp was copied *after* fill_vb2_buffer was called. Now it is reversed and you need this check to avoid zeroing a copied timestamp. When I referred to the v4l2-compliance failures I meant failures I got testing with an early version of the request API that didn't have this modified timestamp handling code. Regards, Hans > > 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 >