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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

Hi Hans,

On Sunday 21 September 2014 11:00:02 Hans Verkuil wrote:
> On 09/20/2014 09:26 PM, Laurent Pinchart wrote:
> > 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).

I meant vb2_queue_init.

> >>  	}
> >>  	
> >>  	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;

-- 
Regards,

Laurent Pinchart