On 02/23/2015 12:58 PM, Kamil Debski wrote:
> Hi,
>
> Thank you for the review :)
>
>> From: Hans Verkuil [mailto:hverk...@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.dufre...@collabora.com>
>>> Signed-off-by: Kamil Debski <k.deb...@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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
