On 29/07/2021 16:26, Laurent Pinchart wrote:
> Hi Hans,
>
> Thank you for the patch.
>
> On Thu, Jul 29, 2021 at 10:35:33AM +0200, Hans Verkuil wrote:
>> If a vb2_queue sets q->min_buffers_needed then if the number of
>> queued buffers reaches that number vb2_core_qbuf() will call
>> the start_streaming() callback. If that returns an error, then that
>> was just returned, but that left the buffer still queued. But userspace
>
> The three "that" in the sentence are confusing. Do you mean "If that
> function returns an error, the error code is just returned, but the
> buffer is left still queued." ?
>
>> expects that if VIDIOC_QBUF fails, the buffer wasn't queued.
>>
>> So if start_streaming() fails, then remove the buffer from the queue,
>> thus avoiding this unwanted side-effect.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xxxxxxxxx>
>> Tested-by: Kieran Bingham <kieran.bingham@xxxxxxxxxxxxxxxx>
>> Fixes: b3379c6201bb ("[media] vb2: only call start_streaming if sufficient buffers are queued")
>
> Possibly with the commit message updated,
This is the new commit message (not going to repost the patch, I'll just
update the PR):
If a vb2_queue sets q->min_buffers_needed then when the number of
queued buffers reaches q->min_buffers_needed, vb2_core_qbuf() will call
the start_streaming() callback. If start_streaming() returns an error,
then that error was just returned by vb2_core_qbuf(), but the buffer
was still queued. However, userspace expects that if VIDIOC_QBUF fails,
the buffer is returned dequeued.
So if start_streaming() fails, then remove the buffer from the queue,
thus avoiding this unwanted side-effect.
Regards,
Hans
>
> Reviewed-by: Laurent Pinchart <laurent.pinchart@xxxxxxxxxxxxxxxx>
>
>> ---
>> drivers/media/common/videobuf2/videobuf2-core.c | 13 ++++++++++++-
>> 1 file changed, 12 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/media/common/videobuf2/videobuf2-core.c b/drivers/media/common/videobuf2/videobuf2-core.c
>> index 02281d13505f..508ac295eb06 100644
>> --- a/drivers/media/common/videobuf2/videobuf2-core.c
>> +++ b/drivers/media/common/videobuf2/videobuf2-core.c
>> @@ -1573,6 +1573,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>> struct media_request *req)
>> {
>> struct vb2_buffer *vb;
>> + enum vb2_buffer_state orig_state;
>> int ret;
>>
>> if (q->error) {
>> @@ -1673,6 +1674,7 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>> * Add to the queued buffers list, a buffer will stay on it until
>> * dequeued in dqbuf.
>> */
>> + orig_state = vb->state;
>> list_add_tail(&vb->queued_entry, &q->queued_list);
>> q->queued_count++;
>> q->waiting_for_buffers = false;
>> @@ -1703,8 +1705,17 @@ int vb2_core_qbuf(struct vb2_queue *q, unsigned int index, void *pb,
>> if (q->streaming && !q->start_streaming_called &&
>> q->queued_count >= q->min_buffers_needed) {
>> ret = vb2_start_streaming(q);
>> - if (ret)
>> + if (ret) {
>> + /*
>> + * Since vb2_core_qbuf will return with an error,
>> + * we should return it to state DEQUEUED since
>> + * the error indicates that the buffer wasn't queued.
>> + */
>> + list_del(&vb->queued_entry);
>> + q->queued_count--;
>> + vb->state = orig_state;
>> return ret;
>> + }
>> }
>>
>> dprintk(q, 2, "qbuf of buffer %d succeeded\n", vb->index);
>
