| Message ID | 1424450288-26444-2-git-send-email-k.debski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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@collabora.com> > Signed-off-by: Kamil Debski <k.debski@samsung.com> > --- > 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? > + 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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi, Thank you for the review :) > From: Hans Verkuil [mailto:hverkuil@xs4all.nl] > 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@collabora.com> > > Signed-off-by: Kamil Debski <k.debski@samsung.com> > > --- > > 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,
On 02/23/2015 12:58 PM, Kamil Debski wrote: > Hi, > > Thank you for the review :) > >> From: Hans Verkuil [mailto:hverkuil@xs4all.nl] >> 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@collabora.com> >>> Signed-off-by: Kamil Debski <k.debski@samsung.com> >>> --- >>> 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. I agree with that. Regards, Hans > >> >>> + 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, > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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"); + 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 + * 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;
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@collabora.com> Signed-off-by: Kamil Debski <k.debski@samsung.com> --- drivers/media/v4l2-core/videobuf2-core.c | 39 +++++++++++++++++++++++++----- include/media/videobuf2-core.h | 2 ++ 2 files changed, 35 insertions(+), 6 deletions(-)