| Message ID | 1411203375-15310-2-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | Superseded, archived |
| Delegated to: | Hans Verkuil |
| Headers |
Received: from mail.tu-berlin.de ([130.149.7.33]) by www.linuxtv.org with esmtp (Exim 4.72) (envelope-from <linux-media-owner@vger.kernel.org>) id 1XVGTH-0005im-74; Sat, 20 Sep 2014 10:56:47 +0200 X-tubIT-Incoming-IP: 209.132.180.67 Received: from vger.kernel.org ([209.132.180.67]) by mail.tu-berlin.de (exim-4.72/mailfrontend-8) with esmtp id 1XVGTE-0008Qh-ko; Sat, 20 Sep 2014 10:56:46 +0200 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752732AbaITI4l (ORCPT <rfc822;mkrufky@linuxtv.org> + 1 other); Sat, 20 Sep 2014 04:56:41 -0400 Received: from smtp-vbr2.xs4all.nl ([194.109.24.22]:4000 "EHLO smtp-vbr2.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1752039AbaITI4i (ORCPT <rfc822;linux-media@vger.kernel.org>); Sat, 20 Sep 2014 04:56:38 -0400 Received: from tschai.lan (209.80-203-20.nextgentel.com [80.203.20.209] (may be forged)) (authenticated bits=0) by smtp-vbr2.xs4all.nl (8.13.8/8.13.8) with ESMTP id s8K8uL7N017745; Sat, 20 Sep 2014 10:56:23 +0200 (CEST) (envelope-from hverkuil@xs4all.nl) Received: from tschai.fritz.box (localhost [127.0.0.1]) by tschai.lan (Postfix) with ESMTPSA id 720F62A1CE1; Sat, 20 Sep 2014 10:56:17 +0200 (CEST) From: Hans Verkuil <hverkuil@xs4all.nl> To: linux-media@vger.kernel.org Cc: laurent.pinchart@ideasonboard.com, m.chehab@samsung.com, Hans Verkuil <hans.verkuil@cisco.com> Subject: [PATCH 1/3] vb2: fix VBI/poll regression Date: Sat, 20 Sep 2014 10:56:13 +0200 Message-Id: <1411203375-15310-2-git-send-email-hverkuil@xs4all.nl> X-Mailer: git-send-email 2.1.0 In-Reply-To: <1411203375-15310-1-git-send-email-hverkuil@xs4all.nl> References: <1411203375-15310-1-git-send-email-hverkuil@xs4all.nl> X-Virus-Scanned: by XS4ALL Virus Scanner Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org X-PMX-Version: 6.0.0.2142326, Antispam-Engine: 2.7.2.2107409, Antispam-Data: 2014.9.20.85119 X-PMX-Spam: Gauge=IIIIIIII, Probability=8%, Report=' MULTIPLE_RCPTS 0.1, HTML_00_01 0.05, HTML_00_10 0.05, BODY_SIZE_3000_3999 0, BODY_SIZE_5000_LESS 0, BODY_SIZE_7000_LESS 0, URI_ENDS_IN_HTML 0, __ANY_URI 0, __CP_URI_IN_BODY 0, __HAS_FROM 0, __HAS_MSGID 0, __HAS_X_MAILER 0, __HAS_X_MAILING_LIST 0, __IN_REP_TO 0, __MIME_TEXT_ONLY 0, __MULTIPLE_RCPTS_CC_X2 0, __SANE_MSGID 0, __SUBJ_ALPHA_END 0, __TO_MALFORMED_2 0, __TO_NO_NAME 0, __URI_NO_WWW 0, __URI_NS ' |
Commit Message
Hans Verkuil
Sept. 20, 2014, 8:56 a.m. UTC
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 | 15 ++++++++++++--- include/media/videobuf2-core.h | 4 ++++ 2 files changed, 16 insertions(+), 3 deletions(-)
Comments
> > 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. > What (mis)behaviour would this cause in userspace application? Thanks James -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On 09/20/2014 11:08 AM, James Harper wrote: >> >> 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. >> > > What (mis)behaviour would this cause in userspace application? If an app would rely on poll to return POLLERR to do the initial STREAMON (seen in e.g. alevt) or to do the initial QBUF (I'm not aware of any apps that do that, but they may exist), then that will currently fail with vb2 because poll() will just wait indefinitely in those cases. Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 20 Sep 2014 11:14:16 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 09/20/2014 11:08 AM, James Harper wrote: > >> > >> 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. > >> > > > > What (mis)behaviour would this cause in userspace application? > > If an app would rely on poll to return POLLERR to do the initial STREAMON > (seen in e.g. alevt) or to do the initial QBUF (I'm not aware of any apps > that do that, but they may exist), then that will currently fail with vb2 > because poll() will just wait indefinitely in those cases. > > Regards, > > Hans > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 20 Sep 2014 11:14:16 +0200 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 09/20/2014 11:08 AM, James Harper wrote: > >> > >> 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. > >> > > > > What (mis)behaviour would this cause in userspace application? > > If an app would rely on poll to return POLLERR to do the initial STREAMON > (seen in e.g. alevt) or to do the initial QBUF (I'm not aware of any apps > that do that, but they may exist), then that will currently fail with vb2 > because poll() will just wait indefinitely in those cases. You forgot to mention (also at the patch series) that the removal of list_empty() check solves the buffer underrun condition. With this fix, if a multi-threaded application goes into an underrun condition (e. g. if it de-queues faster than queues), a POLLERR would be received. The poll fixup patch series also fixes it. Regards, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Sat, 20 Sep 2014 07:08:08 -0300 Mauro Carvalho Chehab <mchehab@osg.samsung.com> escreveu: > Em Sat, 20 Sep 2014 11:14:16 +0200 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > On 09/20/2014 11:08 AM, James Harper wrote: > > >> > > >> 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. > > >> > > > > > > What (mis)behaviour would this cause in userspace application? > > > > If an app would rely on poll to return POLLERR to do the initial STREAMON > > (seen in e.g. alevt) or to do the initial QBUF (I'm not aware of any apps > > that do that, but they may exist), then that will currently fail with vb2 > > because poll() will just wait indefinitely in those cases. > > You forgot to mention (also at the patch series) that the removal of > list_empty() check solves the buffer underrun condition. Actually, no need to comment it there, as I'll be removing the revert patch from topic/devel. If James is using master (likely the case), then the list_empty issue is not affecting him, as the revert is just at topic/devel. > > With this fix, if a multi-threaded application goes into an underrun > condition (e. g. if it de-queues faster than queues), a POLLERR would be > received. The poll fixup patch series also fixes it. > > Regards, > Mauro > -- > To unsubscribe from this list: send the line "unsubscribe linux-media" in > the body of a message to majordomo@vger.kernel.org > More majordomo info at http://vger.kernel.org/majordomo-info.html -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Hi Hans, Thank you for the patch. On Saturday 20 September 2014 10:56:13 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 | 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;
On 09/20/2014 08:32 PM, Laurent Pinchart wrote: > Hi Hans, > > Thank you for the patch. > > On Saturday 20 September 2014 10:56:13 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 | 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. Good catch. And I realized that it should also be set when CREATE_BUFS is called instead of REQBUFS (although only when there are no buffers allocated yet). Regards, Hans > >> 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; > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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;