Hi, Thank you for the review :) > From: Hans Verkuil [mailto:hverkuil@xxxxxxxxx] > Sent: Friday, February 20, 2015 5:52 PM > > Hi Kamil, > > One question and one typo below... > > On 02/20/2015 05:38 PM, Kamil Debski wrote: > > The vb2: fix bytesused == 0 handling (8a75ffb) patch changed the > > behavior of __fill_vb2_buffer function, so that if bytesused is 0 it > > is set to the size of the buffer. However, bytesused set to 0 is used > > by older codec drivers as as indication used to mark the end of > stream. > > > > To keep backward compatibility, this patch adds a flag passed to the > > vb2_queue_init function - allow_zero_bytesused. If the flag is set > > upon initialization of the queue, the videobuf2 keeps the value of > > bytesused intact in the OUTPUT queue and passes it to the driver. > > > > Reported-by: Nicolas Dufresne <nicolas.dufresne@xxxxxxxxxxxxx> > > Signed-off-by: Kamil Debski <k.debski@xxxxxxxxxxx> > > --- > > drivers/media/v4l2-core/videobuf2-core.c | 39 > +++++++++++++++++++++++++----- > > include/media/videobuf2-core.h | 2 ++ > > 2 files changed, 35 insertions(+), 6 deletions(-) > > > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > > b/drivers/media/v4l2-core/videobuf2-core.c > > index 5cd60bf..5d77670 100644 > > --- a/drivers/media/v4l2-core/videobuf2-core.c > > +++ b/drivers/media/v4l2-core/videobuf2-core.c > > @@ -1247,6 +1247,16 @@ static void __fill_vb2_buffer(struct > vb2_buffer > > *vb, const struct v4l2_buffer *b { > > unsigned int plane; > > > > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { > > + if (WARN_ON_ONCE(b->bytesused == 0)) { > > + pr_warn_once("use of bytesused == 0 is deprecated and > will be > > +removed in 2017,\n"); > > I wonder if we should give a specific year, or just say 'in the > future'. > > What do you think? I think I would prefer to use the phrase "in the future". If you are ok with that I will post an updated patch set soon. > > > + if (vb->vb2_queue->allow_zero_bytesused) > > + pr_warn_once("use > VIDIOC_DECODER_CMD(V4L2_DEC_CMD_STOP) instead.\n"); > > + else > > + pr_warn_once("use the actual size instead.\n"); > > + } > > + } > > + > > if (V4L2_TYPE_IS_MULTIPLANAR(b->type)) { > > if (b->memory == V4L2_MEMORY_USERPTR) { > > for (plane = 0; plane < vb->num_planes; ++plane) { @@ > -1276,13 > > +1286,22 @@ static void __fill_vb2_buffer(struct vb2_buffer *vb, > const struct v4l2_buffer *b > > * userspace clearly never bothered to set it and > > * it's a safe assumption that they really meant to > > * use the full plane sizes. > > + * > > + * Some drivers, e.g. old codec drivers, use > bytesused > > + * == 0 as a way to indicate that streaming is > finished. > > + * In that case, the driver should use the > > + * allow_zero_bytesused flag to keep old userspace > > + * applications working. > > */ > > for (plane = 0; plane < vb->num_planes; ++plane) { > > struct v4l2_plane *pdst = &v4l2_planes[plane]; > > struct v4l2_plane *psrc = &b->m.planes[plane]; > > > > - pdst->bytesused = psrc->bytesused ? > > - psrc->bytesused : pdst->length; > > + if (vb->vb2_queue->allow_zero_bytesused) > > + pdst->bytesused = psrc->bytesused; > > + else > > + pdst->bytesused = psrc->bytesused ? > > + psrc->bytesused : pdst->length; > > pdst->data_offset = psrc->data_offset; > > } > > } > > @@ -1295,6 +1314,11 @@ static void __fill_vb2_buffer(struct > vb2_buffer *vb, const struct v4l2_buffer *b > > * > > * If bytesused == 0 for the output buffer, then fall back > > * to the full buffer size as that's a sensible default. > > + * > > + * Some drivers, e.g. old codec drivers, use bytesused * == > 0 as > > Small typo: > > s/bytesused * == 0/bytesused == 0/ > > > + * a way to indicate that streaming is finished. In that > case, > > + * the driver should use the allow_zero_bytesused flag to > keep > > + * old userspace applications working. > > */ > > if (b->memory == V4L2_MEMORY_USERPTR) { > > v4l2_planes[0].m.userptr = b->m.userptr; @@ -1306,10 > +1330,13 @@ > > static void __fill_vb2_buffer(struct vb2_buffer *vb, const struct > v4l2_buffer *b > > v4l2_planes[0].length = b->length; > > } > > > > - if (V4L2_TYPE_IS_OUTPUT(b->type)) > > - v4l2_planes[0].bytesused = b->bytesused ? > > - b->bytesused : v4l2_planes[0].length; > > - else > > + if (V4L2_TYPE_IS_OUTPUT(b->type)) { > > + if (vb->vb2_queue->allow_zero_bytesused) > > + v4l2_planes[0].bytesused = b->bytesused; > > + else > > + v4l2_planes[0].bytesused = b->bytesused ? > > + b->bytesused : v4l2_planes[0].length; > > + } else > > v4l2_planes[0].bytesused = 0; > > > > } > > diff --git a/include/media/videobuf2-core.h > > b/include/media/videobuf2-core.h index e49dc6b..a5790fd 100644 > > --- a/include/media/videobuf2-core.h > > +++ b/include/media/videobuf2-core.h > > @@ -337,6 +337,7 @@ struct v4l2_fh; > > * @io_modes: supported io methods (see vb2_io_modes enum) > > * @fileio_read_once: report EOF after reading the first > buffer > > * @fileio_write_immediately: queue buffer after each write() call > > + * @allow_zero_bytesused: allow bytesused == 0 to be passed to the > driver > > * @lock: pointer to a mutex that protects the vb2_queue > struct. The > > * driver can set this to a mutex to let the v4l2 core > serialize > > * the queuing ioctls. If the driver wants to handle locking > > @@ -388,6 +389,7 @@ struct vb2_queue { > > unsigned int io_modes; > > unsigned fileio_read_once:1; > > unsigned fileio_write_immediately:1; > > + unsigned allow_zero_bytesused:1; > > > > struct mutex *lock; > > struct v4l2_fh *owner; > > > > Regards, > > Hans Best wishes, -- Kamil Debski Samsung R&D Institute Poland -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
