Re: [PATCHv2 1/3] vb2: fix VBI/poll regression

[Hans Verkuil <hverkuil@xs4all.nl>]

On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> Thank you for the patch.
> 
> On Saturday 20 September 2014 21:16:35 Hans Verkuil wrote:
>> From: Hans Verkuil <hans.verkuil@cisco.com>
>>
>> The recent conversion of saa7134 to vb2 unconvered a poll() bug that
>> broke the teletext applications alevt and mtt. These applications
>> expect that calling poll() without having called VIDIOC_STREAMON will
>> cause poll() to return POLLERR. That did not happen in vb2.
>>
>> This patch fixes that behavior. It also fixes what should happen when
>> poll() is called when STREAMON is called but no buffers have been
>> queued. In that case poll() will also return POLLERR, but only for
>> capture queues since output queues will always return POLLOUT
>> anyway in that situation.
>>
>> This brings the vb2 behavior in line with the old videobuf behavior.
>>
>> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
>> ---
>>  drivers/media/v4l2-core/videobuf2-core.c | 17 ++++++++++++++---
>>  include/media/videobuf2-core.h           |  4 ++++
>>  2 files changed, 18 insertions(+), 3 deletions(-)
>>
>> diff --git a/drivers/media/v4l2-core/videobuf2-core.c
>> b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..a0aa694 100644
>> --- a/drivers/media/v4l2-core/videobuf2-core.c
>> +++ b/drivers/media/v4l2-core/videobuf2-core.c
>> @@ -977,6 +977,7 @@ static int __reqbufs(struct vb2_queue *q, struct
>> v4l2_requestbuffers *req) * to the userspace.
>>  	 */
>>  	req->count = allocated_buffers;
>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>
>>  	return 0;
>>  }
>> @@ -1024,6 +1025,7 @@ static int __create_bufs(struct vb2_queue *q, struct
>> v4l2_create_buffers *create memset(q->plane_sizes, 0,
>> sizeof(q->plane_sizes));
>>  		memset(q->alloc_ctx, 0, sizeof(q->alloc_ctx));
>>  		q->memory = create->memory;
>> +		q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
> 
> Wouldn't it be easier to set the flag when creating the queue and in 
> vb2_internal_streamoff() instead of in __create_bufs and __reqbufs ? I'll let 
> you decide.

Sorry, I don't follow. 'When creating the queue'? __create_bufs and __reqbufs
are the functions that create the queue (i.e. allocate the buffers).

Regards,

	Hans

> 
>>  	}
>>
>>  	num_buffers = min(create->count, VIDEO_MAX_FRAME - q->num_buffers);
>> @@ -1801,6 +1803,7 @@ static int vb2_internal_qbuf(struct vb2_queue *q,
>> struct v4l2_buffer *b) */
>>  	list_add_tail(&vb->queued_entry, &q->queued_list);
>>  	q->queued_count++;
>> +	q->waiting_for_buffers = false;
>>  	vb->state = VB2_BUF_STATE_QUEUED;
>>  	if (V4L2_TYPE_IS_OUTPUT(q->type)) {
>>  		/*
>> @@ -2261,6 +2264,7 @@ static int vb2_internal_streamoff(struct vb2_queue *q,
>> enum v4l2_buf_type type) * their normal dequeued state.
>>  	 */
>>  	__vb2_queue_cancel(q);
>> +	q->waiting_for_buffers = !V4L2_TYPE_IS_OUTPUT(q->type);
>>
>>  	dprintk(3, "successful\n");
>>  	return 0;
>> @@ -2583,10 +2587,17 @@ unsigned int vb2_poll(struct vb2_queue *q, struct
>> file *file, poll_table *wait) }
>>
>>  	/*
>> -	 * There is nothing to wait for if no buffer has been queued and the
>> -	 * queue isn't streaming, or if the error flag is set.
>> +	 * There is nothing to wait for if the queue isn't streaming, or if the
>> +	 * error flag is set.
>>  	 */
>> -	if ((list_empty(&q->queued_list) && !vb2_is_streaming(q)) || q->error)
>> +	if (!vb2_is_streaming(q) || q->error)
>> +		return res | POLLERR;
>> +	/*
>> +	 * For compatibility with vb1: if QBUF hasn't been called yet, then
>> +	 * return POLLERR as well. This only affects capture queues, output
>> +	 * queues will always initialize waiting_for_buffers to false.
>> +	 */
>> +	if (q->waiting_for_buffers)
>>  		return res | POLLERR;
>>
>>  	/*
>> diff --git a/include/media/videobuf2-core.h b/include/media/videobuf2-core.h
>> index 5a10d8d..84f790c 100644
>> --- a/include/media/videobuf2-core.h
>> +++ b/include/media/videobuf2-core.h
>> @@ -381,6 +381,9 @@ struct v4l2_fh;
>>   * @start_streaming_called: start_streaming() was called successfully and
>> we *		started streaming.
>>   * @error:	a fatal error occurred on the queue
>> + * @waiting_for_buffers: used in poll() to check if vb2 is still waiting
>> for + *		buffers. Only set for capture queues if qbuf has not yet been +
>> *		called since poll() needs to return POLLERR in that situation. *
>> @fileio:	file io emulator internal data, used only if emulator is active 
> *
>> @threadio:	thread io internal data, used only if thread is active */
>> @@ -419,6 +422,7 @@ struct vb2_queue {
>>  	unsigned int			streaming:1;
>>  	unsigned int			start_streaming_called:1;
>>  	unsigned int			error:1;
>> +	unsigned int			waiting_for_buffers:1;
>>
>>  	struct vb2_fileio_data		*fileio;
>>  	struct vb2_threadio_data	*threadio;
>