| Message ID | 1424694379-11115-2-git-send-email-k.debski@samsung.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 02/23/2015 01:26 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> Acked-by: Hans Verkuil <hans.verkuil@cisco.com> Thanks! Hans > --- > 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..33a5d93 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 the future,\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; > -- 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
Hello,
While working on fixing occurrences of
[ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future,
[ 54.388026] use the actual size instead.
in libcamera, I realized that the patch that initially introduced the
warning and deprecated setting bytesused to 0 didn't change the V4L2 API
specification, which still documents bytesused as
[...] If the application sets this to 0 for an output stream, then
bytesused will be set to the size of the buffer (see the length
field of this struct) by the driver. [...]
for both v4l2_buffer and v4l2_plane.
This deprecated behaviour has been present in the kernel for 7 years and
a half now. I'm wondering if it's really deprecated, in which case we
should update the API specification, or if it should be considered
supported, in which case the warning should be dropped.
On Mon, Feb 23, 2015 at 01:26:17PM +0100, 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..33a5d93 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 the future,\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;
On 10/2/22 01:48, Laurent Pinchart wrote: > Hello, > > While working on fixing occurrences of > > [ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future, > [ 54.388026] use the actual size instead. > > in libcamera, I realized that the patch that initially introduced the > warning and deprecated setting bytesused to 0 didn't change the V4L2 API > specification, which still documents bytesused as > > [...] If the application sets this to 0 for an output stream, then > bytesused will be set to the size of the buffer (see the length > field of this struct) by the driver. [...] > > for both v4l2_buffer and v4l2_plane. > > This deprecated behaviour has been present in the kernel for 7 years and > a half now. I'm wondering if it's really deprecated, in which case we > should update the API specification, or if it should be considered > supported, in which case the warning should be dropped. It's a good question. I do believe it should be removed from the spec. It is IMHO a bad idea to just leave bytesused at 0 for an output buffer. Applications should be explicit. But on the other hand, I think we need to keep the current behavior in the kernel of replacing bytesused == 0 with the length of the buffer. I don't think we can return an error in that case, it would almost certainly break userspace. Regarding the warning: I think we need to keep it, to warn applications that what they are doing is a bad idea, but that the text should change from: "use of bytesused == 0 is deprecated and will be removed in the future" to: "use of bytesused == 0 is not recommended" Regards, Hans > > On Mon, Feb 23, 2015 at 01:26:17PM +0100, 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..33a5d93 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 the future,\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; >
Hi Hans, Laurent, On Tue, Oct 04, 2022 at 11:12:45AM +0200, Hans Verkuil wrote: > > > On 10/2/22 01:48, Laurent Pinchart wrote: > > Hello, > > > > While working on fixing occurrences of > > > > [ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future, > > [ 54.388026] use the actual size instead. > > > > in libcamera, I realized that the patch that initially introduced the > > warning and deprecated setting bytesused to 0 didn't change the V4L2 API > > specification, which still documents bytesused as > > > > [...] If the application sets this to 0 for an output stream, then > > bytesused will be set to the size of the buffer (see the length > > field of this struct) by the driver. [...] > > > > for both v4l2_buffer and v4l2_plane. > > > > This deprecated behaviour has been present in the kernel for 7 years and > > a half now. I'm wondering if it's really deprecated, in which case we > > should update the API specification, or if it should be considered > > supported, in which case the warning should be dropped. > > It's a good question. > > I do believe it should be removed from the spec. It is IMHO a bad idea to > just leave bytesused at 0 for an output buffer. Applications should be explicit. > > But on the other hand, I think we need to keep the current behavior in the > kernel of replacing bytesused == 0 with the length of the buffer. I don't > think we can return an error in that case, it would almost certainly break > userspace. > > Regarding the warning: I think we need to keep it, to warn applications that > what they are doing is a bad idea, but that the text should change from: > > "use of bytesused == 0 is deprecated and will be removed in the future" > > to: > > "use of bytesused == 0 is not recommended" If we ever intend to drop this support, we should warn we're going to do so. Otherwise there's little point in recommending against using it. The spec should document it as deprecated and to be removed in the future as well. (Or alternatively, the warning should be removed altogether.)
Hello, On Tue, Oct 04, 2022 at 12:23:24PM +0300, Sakari Ailus wrote: > On Tue, Oct 04, 2022 at 11:12:45AM +0200, Hans Verkuil wrote: > > On 10/2/22 01:48, Laurent Pinchart wrote: > > > Hello, > > > > > > While working on fixing occurrences of > > > > > > [ 54.375534] use of bytesused == 0 is deprecated and will be removed in the future, > > > [ 54.388026] use the actual size instead. > > > > > > in libcamera, I realized that the patch that initially introduced the > > > warning and deprecated setting bytesused to 0 didn't change the V4L2 API > > > specification, which still documents bytesused as > > > > > > [...] If the application sets this to 0 for an output stream, then > > > bytesused will be set to the size of the buffer (see the length > > > field of this struct) by the driver. [...] > > > > > > for both v4l2_buffer and v4l2_plane. > > > > > > This deprecated behaviour has been present in the kernel for 7 years and > > > a half now. I'm wondering if it's really deprecated, in which case we > > > should update the API specification, or if it should be considered > > > supported, in which case the warning should be dropped. > > > > It's a good question. > > > > I do believe it should be removed from the spec. It is IMHO a bad idea to > > just leave bytesused at 0 for an output buffer. Applications should be explicit. OK. I'll write a patch. > > But on the other hand, I think we need to keep the current behavior in the > > kernel of replacing bytesused == 0 with the length of the buffer. I don't > > think we can return an error in that case, it would almost certainly break > > userspace. Yes, I don't think we can change the implementation for the time being, the risk of breakages is just too high (I'm fixing the libcamera side though). > > Regarding the warning: I think we need to keep it, to warn applications that > > what they are doing is a bad idea, but that the text should change from: > > > > "use of bytesused == 0 is deprecated and will be removed in the future" > > > > to: > > > > "use of bytesused == 0 is not recommended" > > If we ever intend to drop this support, we should warn we're going to do > so. Otherwise there's little point in recommending against using it. I agree. Just saying it's not recommended is pointless. Either we want to deprecate this behaviour, which means that it may get removed in the future (one option could be to WARN_ONCE() for a few years, although even that may not be enough), or we conclude that removing it will never be possible, in which case I'd drop the message. > The > spec should document it as deprecated and to be removed in the future as > well. (Or alternatively, the warning should be removed altogether.) I wouldn't document it at all, if it's deprecated it doesn't deserve a mention in the spec. It's hard enough for people to understand how to do the right thing when reading documentation without being told about the things that work but shouldn't be done :-)
Hi Laurent, On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote: > > If we ever intend to drop this support, we should warn we're going to do > > so. Otherwise there's little point in recommending against using it. > > I agree. Just saying it's not recommended is pointless. Either we want > to deprecate this behaviour, which means that it may get removed in the > future (one option could be to WARN_ONCE() for a few years, although > even that may not be enough), or we conclude that removing it will never > be possible, in which case I'd drop the message. I think displaying a warning for a few seconds would do it. ;-) ;-) > > > The > > spec should document it as deprecated and to be removed in the future as > > well. (Or alternatively, the warning should be removed altogether.) > > I wouldn't document it at all, if it's deprecated it doesn't deserve a > mention in the spec. It's hard enough for people to understand how to do > the right thing when reading documentation without being told about the > things that work but shouldn't be done :-) I would document it as deprecated so that application developers reading it could get the hint. Otherwise they won't (unless they look at the kernel logs of course). Up to you, I don't have a strong opinion on this.
Hi Sakari, On Tue, Oct 04, 2022 at 09:36:56PM +0300, Sakari Ailus wrote: > On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote: > > > If we ever intend to drop this support, we should warn we're going to do > > > so. Otherwise there's little point in recommending against using it. > > > > I agree. Just saying it's not recommended is pointless. Either we want > > to deprecate this behaviour, which means that it may get removed in the > > future (one option could be to WARN_ONCE() for a few years, although > > even that may not be enough), or we conclude that removing it will never > > be possible, in which case I'd drop the message. > > I think displaying a warning for a few seconds would do it. ;-) ;-) > > > > The > > > spec should document it as deprecated and to be removed in the future as > > > well. (Or alternatively, the warning should be removed altogether.) > > > > I wouldn't document it at all, if it's deprecated it doesn't deserve a > > mention in the spec. It's hard enough for people to understand how to do > > the right thing when reading documentation without being told about the > > things that work but shouldn't be done :-) > > I would document it as deprecated so that application developers reading it > could get the hint. Otherwise they won't (unless they look at the kernel > logs of course). Up to you, I don't have a strong opinion on this. Why do you think that would be better than documenting the non-deprecated behaviour only ? I doubt anyone would read the documentation, realize that the feature is deprecated, and then go fix an application. I may be wrong though, is that why you would like to see a mention of the deprecated feature in the documentation ?
Hi Laurent, On Tue, Oct 04, 2022 at 10:47:49PM +0300, Laurent Pinchart wrote: > Hi Sakari, > > On Tue, Oct 04, 2022 at 09:36:56PM +0300, Sakari Ailus wrote: > > On Tue, Oct 04, 2022 at 05:29:46PM +0300, Laurent Pinchart wrote: > > > > If we ever intend to drop this support, we should warn we're going to do > > > > so. Otherwise there's little point in recommending against using it. > > > > > > I agree. Just saying it's not recommended is pointless. Either we want > > > to deprecate this behaviour, which means that it may get removed in the > > > future (one option could be to WARN_ONCE() for a few years, although > > > even that may not be enough), or we conclude that removing it will never > > > be possible, in which case I'd drop the message. > > > > I think displaying a warning for a few seconds would do it. ;-) ;-) > > > > > > The > > > > spec should document it as deprecated and to be removed in the future as > > > > well. (Or alternatively, the warning should be removed altogether.) > > > > > > I wouldn't document it at all, if it's deprecated it doesn't deserve a > > > mention in the spec. It's hard enough for people to understand how to do > > > the right thing when reading documentation without being told about the > > > things that work but shouldn't be done :-) > > > > I would document it as deprecated so that application developers reading it > > could get the hint. Otherwise they won't (unless they look at the kernel > > logs of course). Up to you, I don't have a strong opinion on this. > > Why do you think that would be better than documenting the > non-deprecated behaviour only ? I doubt anyone would read the > documentation, realize that the feature is deprecated, and then go fix > an application. I may be wrong though, is that why you would like to see > a mention of the deprecated feature in the documentation ? Yes. I agree the likelihood of that happening is not very big presumably. As noted, I don't have a strong opimion on this. On the other hand, I'm certain the other option I proposed above would be far more efficient in having applications fixed.
diff --git a/drivers/media/v4l2-core/videobuf2-core.c b/drivers/media/v4l2-core/videobuf2-core.c index 5cd60bf..33a5d93 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 the future,\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(-)