From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from galahad.ideasonboard.com ([185.26.127.97]:42198 "EHLO galahad.ideasonboard.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1756729AbaITScv (ORCPT ); Sat, 20 Sep 2014 14:32:51 -0400 From: Laurent Pinchart To: Hans Verkuil Cc: linux-media@vger.kernel.org, m.chehab@samsung.com, Hans Verkuil Subject: Re: [PATCH 1/3] vb2: fix VBI/poll regression Date: Sat, 20 Sep 2014 21:32:54 +0300 Message-ID: <1554010.2WmUDpr8lj@avalon> In-Reply-To: <1411203375-15310-2-git-send-email-hverkuil@xs4all.nl> References: <1411203375-15310-1-git-send-email-hverkuil@xs4all.nl> <1411203375-15310-2-git-send-email-hverkuil@xs4all.nl> MIME-Version: 1.0 Content-Transfer-Encoding: 7Bit Content-Type: text/plain; charset="us-ascii" Sender: linux-media-owner@vger.kernel.org List-ID: Hi Hans, Thank you for the patch. On Saturday 20 September 2014 10:56:13 Hans Verkuil wrote: > From: Hans Verkuil > > 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 > --- > drivers/media/v4l2-core/videobuf2-core.c | 15 ++++++++++++--- > include/media/videobuf2-core.h | 4 ++++ > 2 files changed, 16 insertions(+), 3 deletions(-) > > diff --git a/drivers/media/v4l2-core/videobuf2-core.c > b/drivers/media/v4l2-core/videobuf2-core.c index 7e6aff6..f3762a8 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); I think you also need to set waiting_for_buffers at STREAMOFF time, otherwise a STREAMOFF/STREAMON without a REQBUFS in-between won't work as expected. > return 0; > } > @@ -1801,6 +1802,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)) { > /* > @@ -2583,10 +2585,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