Message ID | 1266607320-9974-5-git-send-email-sakari.ailus@maxwell.research.nokia.com (mailing list archive) |
---|---|
State | Superseded, archived |
Headers |
Return-path: <linux-media-owner@vger.kernel.org> Envelope-to: mchehab@infradead.org Delivery-date: Fri, 19 Feb 2010 19:23:59 +0000 Received: from bombadil.infradead.org [18.85.46.34] by pedra with IMAP (fetchmail-6.3.6) for <mchehab@localhost> (single-drop); Fri, 19 Feb 2010 17:24:25 -0200 (BRST) Received: from vger.kernel.org ([209.132.180.67]) by bombadil.infradead.org with esmtp (Exim 4.69 #1 (Red Hat Linux)) id 1NiYSE-0002Sn-R5; Fri, 19 Feb 2010 19:23:59 +0000 Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1755550Ab0BSTX6 (ORCPT <rfc822; kmpark@infradead.org> + 1 other); Fri, 19 Feb 2010 14:23:58 -0500 Received: from smtp.nokia.com ([192.100.105.134]:18359 "EHLO mgw-mx09.nokia.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755541Ab0BSTX4 (ORCPT <rfc822;linux-media@vger.kernel.org>); Fri, 19 Feb 2010 14:23:56 -0500 Received: from esebh106.NOE.Nokia.com (esebh106.ntc.nokia.com [172.21.138.213]) by mgw-mx09.nokia.com (Switch-3.3.3/Switch-3.3.3) with ESMTP id o1JJNDMW002221; Fri, 19 Feb 2010 13:23:54 -0600 Received: from vaebh104.NOE.Nokia.com ([10.160.244.30]) by esebh106.NOE.Nokia.com with Microsoft SMTPSVC(6.0.3790.3959); Fri, 19 Feb 2010 21:23:13 +0200 Received: from mgw-da01.ext.nokia.com ([147.243.128.24]) by vaebh104.NOE.Nokia.com over TLS secured channel with Microsoft SMTPSVC(6.0.3790.3959); Fri, 19 Feb 2010 21:23:10 +0200 Received: from maxwell.research.nokia.com (nese-loadbalancer-pip.nokia.com [172.16.16.250]) by mgw-da01.ext.nokia.com (Switch-3.3.3/Switch-3.3.3) with ESMTP id o1JJM1Fl014074 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=NO); Fri, 19 Feb 2010 21:22:07 +0200 Received: from kaali.localdomain (kaali.localdomain [192.168.239.7]) by maxwell.research.nokia.com (Postfix) with ESMTPS id 04AD27010C; Fri, 19 Feb 2010 21:22:01 +0200 (EET) Received: from sailus by kaali.localdomain with local (Exim 4.69) (envelope-from <sakari.ailus@maxwell.research.nokia.com>) id 1NiYQK-0002bn-UI; Fri, 19 Feb 2010 21:22:00 +0200 From: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> To: linux-media@vger.kernel.org Cc: hverkuil@xs4all.nl, laurent.pinchart@ideasonboard.com, iivanov@mm-sol.com, gururaj.nagendra@intel.com, david.cohen@nokia.com, Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> Subject: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl Date: Fri, 19 Feb 2010 21:21:59 +0200 Message-Id: <1266607320-9974-5-git-send-email-sakari.ailus@maxwell.research.nokia.com> X-Mailer: git-send-email 1.5.6.5 In-Reply-To: <4B7EE4A4.3080202@maxwell.research.nokia.com> References: <4B7EE4A4.3080202@maxwell.research.nokia.com> X-OriginalArrivalTime: 19 Feb 2010 19:23:11.0102 (UTC) FILETIME=[F968C9E0:01CAB198] X-Nokia-AV: Clean Sender: linux-media-owner@vger.kernel.org Precedence: bulk List-ID: <linux-media.vger.kernel.org> X-Mailing-List: linux-media@vger.kernel.org |
Commit Message
Sakari Ailus
Feb. 19, 2010, 7:21 p.m. UTC
Add support for event handling to do_ioctl.
Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
drivers/media/video/v4l2-ioctl.c | 58 ++++++++++++++++++++++++++++++++++++++
include/media/v4l2-ioctl.h | 7 ++++
2 files changed, 65 insertions(+), 0 deletions(-)
Comments
More comments... On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: > Add support for event handling to do_ioctl. > > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> > --- > drivers/media/video/v4l2-ioctl.c | 58 ++++++++++++++++++++++++++++++++++++++ > include/media/v4l2-ioctl.h | 7 ++++ > 2 files changed, 65 insertions(+), 0 deletions(-) > > diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c > index 34c7d6e..f7d6177 100644 > --- a/drivers/media/video/v4l2-ioctl.c > +++ b/drivers/media/video/v4l2-ioctl.c > @@ -25,6 +25,8 @@ > #endif > #include <media/v4l2-common.h> > #include <media/v4l2-ioctl.h> > +#include <media/v4l2-fh.h> > +#include <media/v4l2-event.h> > #include <media/v4l2-chip-ident.h> > > #define dbgarg(cmd, fmt, arg...) \ > @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, > } > break; > } > + case VIDIOC_DQEVENT: > + { > + struct v4l2_event *ev = arg; > + struct v4l2_fh *vfh = fh; > + > + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > + || vfh->events == NULL) > + break; Change this to: if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) break; if (vfh->events == NULL) return -ENOENT; But see also the next comment. > + > + ret = v4l2_event_dequeue(fh, ev); There is a crucial piece of functionality missing here: if the filehandle is in blocking mode, then it should wait until an event arrives. That also means that if vfh->events == NULL, you should still call v4l2_event_dequeue, and that function should initialize vfh->events and wait for an event if the fh is in blocking mode. So I would remove the events == NULL test here and just call v4l2_event_dequeue and let that function handle it. > + if (ret < 0) { > + dbgarg(cmd, "no pending events?"); > + break; > + } > + dbgarg(cmd, > + "pending=%d, type=0x%8.8x, sequence=%d, " > + "timestamp=%lu.%9.9lu ", > + ev->pending, ev->type, ev->sequence, > + ev->timestamp.tv_sec, ev->timestamp.tv_nsec); > + break; > + } > + case VIDIOC_SUBSCRIBE_EVENT: > + { > + struct v4l2_event_subscription *sub = arg; > + struct v4l2_fh *vfh = fh; > > + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event. > + || vfh->events == NULL Remove this test. If you allocate the event queue only when you first subscribe to an event (as ivtv will do), then you have to be able to call vidioc_subscribe_event even if vfh->events == NULL. > + || !ops->vidioc_subscribe_event) > + break; > + > + ret = ops->vidioc_subscribe_event(fh, sub); > + if (ret < 0) { > + dbgarg(cmd, "failed, ret=%ld", ret); > + break; > + } > + dbgarg(cmd, "type=0x%8.8x", sub->type); > + break; > + } > + case VIDIOC_UNSUBSCRIBE_EVENT: > + { > + struct v4l2_event_subscription *sub = arg; > + struct v4l2_fh *vfh = fh; > + > + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > + || vfh->events == NULL > + || !ops->vidioc_unsubscribe_event) No need for the test_bit. This is sufficient: if (!ops->vidioc_unsubscribe_event || vfh->events == NULL) Note that I am not so keen on testing against vfh->events here. I consider this a v4l2_fh-internal field that should not be used outside the v4l2_event_ and v4l2_fh_ functions. > + break; > + > + ret = ops->vidioc_unsubscribe_event(fh, sub); > + if (ret < 0) { > + dbgarg(cmd, "failed, ret=%ld", ret); > + break; > + } > + dbgarg(cmd, "type=0x%8.8x", sub->type); > + break; > + } > default: > { > if (!ops->vidioc_default) > diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h > index e8ba0f2..06daa6e 100644 > --- a/include/media/v4l2-ioctl.h > +++ b/include/media/v4l2-ioctl.h > @@ -21,6 +21,8 @@ > #include <linux/videodev2.h> > #endif > > +struct v4l2_fh; > + > struct v4l2_ioctl_ops { > /* ioctl callbacks */ > > @@ -254,6 +256,11 @@ struct v4l2_ioctl_ops { > int (*vidioc_g_dv_timings) (struct file *file, void *fh, > struct v4l2_dv_timings *timings); > > + int (*vidioc_subscribe_event) (struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub); > + int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh, > + struct v4l2_event_subscription *sub); > + > /* For other private ioctls */ > long (*vidioc_default) (struct file *file, void *fh, > int cmd, void *arg); > Regards, Hans
Hans Verkuil wrote: > More comments... > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: >> Add support for event handling to do_ioctl. >> >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> >> --- >> drivers/media/video/v4l2-ioctl.c | 58 ++++++++++++++++++++++++++++++++++++++ >> include/media/v4l2-ioctl.h | 7 ++++ >> 2 files changed, 65 insertions(+), 0 deletions(-) >> >> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c >> index 34c7d6e..f7d6177 100644 >> --- a/drivers/media/video/v4l2-ioctl.c >> +++ b/drivers/media/video/v4l2-ioctl.c >> @@ -25,6 +25,8 @@ >> #endif >> #include <media/v4l2-common.h> >> #include <media/v4l2-ioctl.h> >> +#include <media/v4l2-fh.h> >> +#include <media/v4l2-event.h> >> #include <media/v4l2-chip-ident.h> >> >> #define dbgarg(cmd, fmt, arg...) \ >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, >> } >> break; >> } >> + case VIDIOC_DQEVENT: >> + { >> + struct v4l2_event *ev = arg; >> + struct v4l2_fh *vfh = fh; >> + >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) >> + || vfh->events == NULL) >> + break; > > Change this to: > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) > break; > if (vfh->events == NULL) > return -ENOENT; > > But see also the next comment. > >> + >> + ret = v4l2_event_dequeue(fh, ev); > > There is a crucial piece of functionality missing here: if the filehandle is > in blocking mode, then it should wait until an event arrives. That also means > that if vfh->events == NULL, you should still call v4l2_event_dequeue, and > that function should initialize vfh->events and wait for an event if the fh > is in blocking mode. I originally left this out intentionally. Most applications using events would use select / poll as well by default. For completeness it should be there, I agree. This btw. suggests that we perhaps should put back the struct file argument for the event functions in video_ioctl_ops. The blocking flag is indeed part of the file structure. I'm open to better suggestions, too. >> + if (ret < 0) { >> + dbgarg(cmd, "no pending events?"); >> + break; >> + } >> + dbgarg(cmd, >> + "pending=%d, type=0x%8.8x, sequence=%d, " >> + "timestamp=%lu.%9.9lu ", >> + ev->pending, ev->type, ev->sequence, >> + ev->timestamp.tv_sec, ev->timestamp.tv_nsec); >> + break; >> + } >> + case VIDIOC_SUBSCRIBE_EVENT: >> + { >> + struct v4l2_event_subscription *sub = arg; >> + struct v4l2_fh *vfh = fh; >> >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > > Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event. > >> + || vfh->events == NULL > > Remove this test. If you allocate the event queue only when you first > subscribe to an event (as ivtv will do), then you have to be able to > call vidioc_subscribe_event even if vfh->events == NULL. How about calling v4l2_event_alloc() with zero events? That allocates and initialises the v4l2_events structure. That's easier to handle in drivers as well since they don't need to consider special cases like fh->events happens to be NULL even if events are supported by the driver. This is how I first thought it'd work.
Hi Sakari, On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote: > Hans Verkuil wrote: > > More comments... > > > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: > >> Add support for event handling to do_ioctl. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> > >> --- > >> > >> drivers/media/video/v4l2-ioctl.c | 58 > >> ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h > >> | 7 ++++ > >> 2 files changed, 65 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/media/video/v4l2-ioctl.c > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644 > >> --- a/drivers/media/video/v4l2-ioctl.c > >> +++ b/drivers/media/video/v4l2-ioctl.c > >> @@ -25,6 +25,8 @@ > >> > >> #endif > >> #include <media/v4l2-common.h> > >> #include <media/v4l2-ioctl.h> > >> > >> +#include <media/v4l2-fh.h> > >> +#include <media/v4l2-event.h> > >> > >> #include <media/v4l2-chip-ident.h> > >> > >> #define dbgarg(cmd, fmt, arg...) \ > >> > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, > >> > >> } > >> break; > >> > >> } > >> > >> + case VIDIOC_DQEVENT: > >> + { > >> + struct v4l2_event *ev = arg; > >> + struct v4l2_fh *vfh = fh; > >> + > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > >> + || vfh->events == NULL) > >> + break; > > > > Change this to: > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) > > > > break; > > > > if (vfh->events == NULL) > > > > return -ENOENT; > > > > But see also the next comment. > > > >> + > >> + ret = v4l2_event_dequeue(fh, ev); > > > > There is a crucial piece of functionality missing here: if the filehandle > > is in blocking mode, then it should wait until an event arrives. That > > also means that if vfh->events == NULL, you should still call > > v4l2_event_dequeue, and that function should initialize vfh->events and > > wait for an event if the fh is in blocking mode. > > I originally left this out intentionally. Most applications using events > would use select / poll as well by default. For completeness it should > be there, I agree. > > This btw. suggests that we perhaps should put back the struct file > argument for the event functions in video_ioctl_ops. The blocking flag > is indeed part of the file structure. I'm open to better suggestions, too. If the only information we need from struct file is the flags, they could be copied to v4l2_fh in the open handler. We could also put a struct file * member in v4l2_fh.
Laurent Pinchart wrote: > Hi Sakari, Salut Laurent, >>> There is a crucial piece of functionality missing here: if the filehandle >>> is in blocking mode, then it should wait until an event arrives. That >>> also means that if vfh->events == NULL, you should still call >>> v4l2_event_dequeue, and that function should initialize vfh->events and >>> wait for an event if the fh is in blocking mode. >> >> I originally left this out intentionally. Most applications using events >> would use select / poll as well by default. For completeness it should >> be there, I agree. >> >> This btw. suggests that we perhaps should put back the struct file >> argument for the event functions in video_ioctl_ops. The blocking flag >> is indeed part of the file structure. I'm open to better suggestions, too. > > If the only information we need from struct file is the flags, they could be > copied to v4l2_fh in the open handler. We could also put a struct file * > member in v4l2_fh. That could be one possibility. Copying the flags in open() isn't enough as they can change as a result of fcntl call. As we're not handling that call the flags in struct v4l2_fh would have to be updated for every ioctl. I'm not a big fan of caching information in general either. :-) I could accept this, though.
On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote: > Hans Verkuil wrote: > > More comments... > > > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: > >> Add support for event handling to do_ioctl. > >> > >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> > >> --- > >> drivers/media/video/v4l2-ioctl.c | 58 ++++++++++++++++++++++++++++++++++++++ > >> include/media/v4l2-ioctl.h | 7 ++++ > >> 2 files changed, 65 insertions(+), 0 deletions(-) > >> > >> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c > >> index 34c7d6e..f7d6177 100644 > >> --- a/drivers/media/video/v4l2-ioctl.c > >> +++ b/drivers/media/video/v4l2-ioctl.c > >> @@ -25,6 +25,8 @@ > >> #endif > >> #include <media/v4l2-common.h> > >> #include <media/v4l2-ioctl.h> > >> +#include <media/v4l2-fh.h> > >> +#include <media/v4l2-event.h> > >> #include <media/v4l2-chip-ident.h> > >> > >> #define dbgarg(cmd, fmt, arg...) \ > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, > >> } > >> break; > >> } > >> + case VIDIOC_DQEVENT: > >> + { > >> + struct v4l2_event *ev = arg; > >> + struct v4l2_fh *vfh = fh; > >> + > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > >> + || vfh->events == NULL) > >> + break; > > > > Change this to: > > > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) > > break; > > if (vfh->events == NULL) > > return -ENOENT; > > > > But see also the next comment. > > > >> + > >> + ret = v4l2_event_dequeue(fh, ev); > > > > There is a crucial piece of functionality missing here: if the filehandle is > > in blocking mode, then it should wait until an event arrives. That also means > > that if vfh->events == NULL, you should still call v4l2_event_dequeue, and > > that function should initialize vfh->events and wait for an event if the fh > > is in blocking mode. > > I originally left this out intentionally. Most applications using events > would use select / poll as well by default. For completeness it should > be there, I agree. It has to be there. This is important functionality. For e.g. ivtv I would use this to wait until the MPEG decoder flushed all buffers and displayed the last frame of the stream. That's something you would often do in blocking mode. > This btw. suggests that we perhaps should put back the struct file > argument for the event functions in video_ioctl_ops. The blocking flag > is indeed part of the file structure. I'm open to better suggestions, too. My long term goal is that the file struct is only used inside v4l2-ioctl.c and not in drivers. Drivers should not need this struct at all. The easiest way to ensure this is by not passing it to the drivers at all :-) > >> + if (ret < 0) { > >> + dbgarg(cmd, "no pending events?"); > >> + break; > >> + } > >> + dbgarg(cmd, > >> + "pending=%d, type=0x%8.8x, sequence=%d, " > >> + "timestamp=%lu.%9.9lu ", > >> + ev->pending, ev->type, ev->sequence, > >> + ev->timestamp.tv_sec, ev->timestamp.tv_nsec); > >> + break; > >> + } > >> + case VIDIOC_SUBSCRIBE_EVENT: > >> + { > >> + struct v4l2_event_subscription *sub = arg; > >> + struct v4l2_fh *vfh = fh; > >> > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > > > > Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event. > > > >> + || vfh->events == NULL > > > > Remove this test. If you allocate the event queue only when you first > > subscribe to an event (as ivtv will do), then you have to be able to > > call vidioc_subscribe_event even if vfh->events == NULL. > > How about calling v4l2_event_alloc() with zero events? That allocates > and initialises the v4l2_events structure. That's easier to handle in > drivers as well since they don't need to consider special cases like > fh->events happens to be NULL even if events are supported by the > driver. This is how I first thought it'd work. Proposal: export a v4l2_event_init() call that sets up fh->events. Calling v4l2_event_alloc(0) feels like a hack. So drivers that want to be able to handle events should call v4l2_event_init after initializing the file handle. Or (and that might even be nicer) test in v4l2_fh_init whether there is a subscribe op in the ioctl_ops struct and let v4l2_fh_init set up fh->events automatically. Regards, Hans
Hi Hans, On Monday 22 February 2010 08:53:53 Hans Verkuil wrote: > On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote: > > Hans Verkuil wrote: > > > More comments... > > > > > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: > > >> Add support for event handling to do_ioctl. > > >> > > >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com> > > >> --- > > >> > > >> drivers/media/video/v4l2-ioctl.c | 58 > > >> ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h > > >> | 7 ++++ > > >> 2 files changed, 65 insertions(+), 0 deletions(-) > > >> > > >> diff --git a/drivers/media/video/v4l2-ioctl.c > > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644 > > >> --- a/drivers/media/video/v4l2-ioctl.c > > >> +++ b/drivers/media/video/v4l2-ioctl.c > > >> @@ -25,6 +25,8 @@ > > >> > > >> #endif > > >> #include <media/v4l2-common.h> > > >> #include <media/v4l2-ioctl.h> > > >> > > >> +#include <media/v4l2-fh.h> > > >> +#include <media/v4l2-event.h> > > >> > > >> #include <media/v4l2-chip-ident.h> > > >> > > >> #define dbgarg(cmd, fmt, arg...) \ > > >> > > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, > > >> > > >> } > > >> break; > > >> > > >> } > > >> > > >> + case VIDIOC_DQEVENT: > > >> + { > > >> + struct v4l2_event *ev = arg; > > >> + struct v4l2_fh *vfh = fh; > > >> + > > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > > >> + || vfh->events == NULL) > > >> + break; > > > > > > Change this to: > > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) > > > > > > break; > > > > > > if (vfh->events == NULL) > > > > > > return -ENOENT; > > > > > > But see also the next comment. > > > > > >> + > > >> + ret = v4l2_event_dequeue(fh, ev); > > > > > > There is a crucial piece of functionality missing here: if the > > > filehandle is in blocking mode, then it should wait until an event > > > arrives. That also means that if vfh->events == NULL, you should still > > > call v4l2_event_dequeue, and that function should initialize > > > vfh->events and wait for an event if the fh is in blocking mode. > > > > I originally left this out intentionally. Most applications using events > > would use select / poll as well by default. For completeness it should > > be there, I agree. > > It has to be there. This is important functionality. For e.g. ivtv I would > use this to wait until the MPEG decoder flushed all buffers and displayed > the last frame of the stream. That's something you would often do in > blocking mode. Blocking mode can easily be emulated using select(). > > This btw. suggests that we perhaps should put back the struct file > > argument for the event functions in video_ioctl_ops. The blocking flag > > is indeed part of the file structure. I'm open to better suggestions, > > too. > > My long term goal is that the file struct is only used inside v4l2-ioctl.c > and not in drivers. Drivers should not need this struct at all. The easiest > way to ensure this is by not passing it to the drivers at all :-) Drivers still need a way to access the blocking flag. The interim solution of adding a file * member to v4l2_fh would allow that, while still removing most usage of file * from drivers. > > >> + if (ret < 0) { > > >> + dbgarg(cmd, "no pending events?"); > > >> + break; > > >> + } > > >> + dbgarg(cmd, > > >> + "pending=%d, type=0x%8.8x, sequence=%d, " > > >> + "timestamp=%lu.%9.9lu ", > > >> + ev->pending, ev->type, ev->sequence, > > >> + ev->timestamp.tv_sec, ev->timestamp.tv_nsec); > > >> + break; > > >> + } > > >> + case VIDIOC_SUBSCRIBE_EVENT: > > >> + { > > >> + struct v4l2_event_subscription *sub = arg; > > >> + struct v4l2_fh *vfh = fh; > > >> > > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) > > > > > > Testing for this bit is unnecessarily. Just test for > > > ops->vidioc_subscribe_event. > > > > > >> + || vfh->events == NULL > > > > > > Remove this test. If you allocate the event queue only when you first > > > subscribe to an event (as ivtv will do), then you have to be able to > > > call vidioc_subscribe_event even if vfh->events == NULL. > > > > How about calling v4l2_event_alloc() with zero events? That allocates > > and initialises the v4l2_events structure. That's easier to handle in > > drivers as well since they don't need to consider special cases like > > fh->events happens to be NULL even if events are supported by the > > driver. This is how I first thought it'd work. > > Proposal: export a v4l2_event_init() call that sets up fh->events. Calling > v4l2_event_alloc(0) feels like a hack. So drivers that want to be able to > handle events should call v4l2_event_init after initializing the file > handle. > > Or (and that might even be nicer) test in v4l2_fh_init whether there is a > subscribe op in the ioctl_ops struct and let v4l2_fh_init set up fh->events > automatically.
> Hi Hans, > > On Monday 22 February 2010 08:53:53 Hans Verkuil wrote: >> On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote: >> > Hans Verkuil wrote: >> > > More comments... >> > > >> > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote: >> > >> Add support for event handling to do_ioctl. >> > >> >> > >> Signed-off-by: Sakari Ailus >> <sakari.ailus@maxwell.research.nokia.com> >> > >> --- >> > >> >> > >> drivers/media/video/v4l2-ioctl.c | 58 >> > >> ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h >> > >> | 7 ++++ >> > >> 2 files changed, 65 insertions(+), 0 deletions(-) >> > >> >> > >> diff --git a/drivers/media/video/v4l2-ioctl.c >> > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644 >> > >> --- a/drivers/media/video/v4l2-ioctl.c >> > >> +++ b/drivers/media/video/v4l2-ioctl.c >> > >> @@ -25,6 +25,8 @@ >> > >> >> > >> #endif >> > >> #include <media/v4l2-common.h> >> > >> #include <media/v4l2-ioctl.h> >> > >> >> > >> +#include <media/v4l2-fh.h> >> > >> +#include <media/v4l2-event.h> >> > >> >> > >> #include <media/v4l2-chip-ident.h> >> > >> >> > >> #define dbgarg(cmd, fmt, arg...) \ >> > >> >> > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file >> *file, >> > >> >> > >> } >> > >> break; >> > >> >> > >> } >> > >> >> > >> + case VIDIOC_DQEVENT: >> > >> + { >> > >> + struct v4l2_event *ev = arg; >> > >> + struct v4l2_fh *vfh = fh; >> > >> + >> > >> + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) >> > >> + || vfh->events == NULL) >> > >> + break; >> > > >> > > Change this to: >> > > if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)) >> > > >> > > break; >> > > >> > > if (vfh->events == NULL) >> > > >> > > return -ENOENT; >> > > >> > > But see also the next comment. >> > > >> > >> + >> > >> + ret = v4l2_event_dequeue(fh, ev); >> > > >> > > There is a crucial piece of functionality missing here: if the >> > > filehandle is in blocking mode, then it should wait until an event >> > > arrives. That also means that if vfh->events == NULL, you should >> still >> > > call v4l2_event_dequeue, and that function should initialize >> > > vfh->events and wait for an event if the fh is in blocking mode. >> > >> > I originally left this out intentionally. Most applications using >> events >> > would use select / poll as well by default. For completeness it should >> > be there, I agree. >> >> It has to be there. This is important functionality. For e.g. ivtv I >> would >> use this to wait until the MPEG decoder flushed all buffers and >> displayed >> the last frame of the stream. That's something you would often do in >> blocking mode. > > Blocking mode can easily be emulated using select(). > >> > This btw. suggests that we perhaps should put back the struct file >> > argument for the event functions in video_ioctl_ops. The blocking flag >> > is indeed part of the file structure. I'm open to better suggestions, >> > too. >> >> My long term goal is that the file struct is only used inside >> v4l2-ioctl.c >> and not in drivers. Drivers should not need this struct at all. The >> easiest >> way to ensure this is by not passing it to the drivers at all :-) > > Drivers still need a way to access the blocking flag. The interim solution > of > adding a file * member to v4l2_fh would allow that, while still removing > most > usage of file * from drivers. Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let v4l2-ioctl.c fill in that argument? That's how I would do it. Regards, Hans
Hans Verkuil wrote: >>>>> There is a crucial piece of functionality missing here: if the >>>>> filehandle is in blocking mode, then it should wait until an event >>>>> arrives. That also means that if vfh->events == NULL, you should >>> still >>>>> call v4l2_event_dequeue, and that function should initialize >>>>> vfh->events and wait for an event if the fh is in blocking mode. >>>> >>>> I originally left this out intentionally. Most applications using >>> events >>>> would use select / poll as well by default. For completeness it should >>>> be there, I agree. >>> >>> It has to be there. This is important functionality. For e.g. ivtv I >>> would >>> use this to wait until the MPEG decoder flushed all buffers and >>> displayed >>> the last frame of the stream. That's something you would often do in >>> blocking mode. >> >> Blocking mode can easily be emulated using select(). It's quite simple to implement still so I'll do that in the VIDIOC_DQEVENT. Easier for applications anyway in use cases that I haven't been thinking about, e.g. ivtv. >>>> This btw. suggests that we perhaps should put back the struct file >>>> argument for the event functions in video_ioctl_ops. The blocking flag >>>> is indeed part of the file structure. I'm open to better suggestions, >>>> too. >>> >>> My long term goal is that the file struct is only used inside >>> v4l2-ioctl.c >>> and not in drivers. Drivers should not need this struct at all. The >>> easiest >>> way to ensure this is by not passing it to the drivers at all :-) >> >> Drivers still need a way to access the blocking flag. The interim solution >> of >> adding a file * member to v4l2_fh would allow that, while still removing >> most >> usage of file * from drivers. > > Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let > v4l2-ioctl.c fill in that argument? That's how I would do it. Implemented already before reading your mail... :-) I'll try to repost the patches today.
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644 --- a/drivers/media/video/v4l2-ioctl.c +++ b/drivers/media/video/v4l2-ioctl.c @@ -25,6 +25,8 @@ #endif #include <media/v4l2-common.h> #include <media/v4l2-ioctl.h> +#include <media/v4l2-fh.h> +#include <media/v4l2-event.h> #include <media/v4l2-chip-ident.h> #define dbgarg(cmd, fmt, arg...) \ @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file, } break; } + case VIDIOC_DQEVENT: + { + struct v4l2_event *ev = arg; + struct v4l2_fh *vfh = fh; + + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) + || vfh->events == NULL) + break; + + ret = v4l2_event_dequeue(fh, ev); + if (ret < 0) { + dbgarg(cmd, "no pending events?"); + break; + } + dbgarg(cmd, + "pending=%d, type=0x%8.8x, sequence=%d, " + "timestamp=%lu.%9.9lu ", + ev->pending, ev->type, ev->sequence, + ev->timestamp.tv_sec, ev->timestamp.tv_nsec); + break; + } + case VIDIOC_SUBSCRIBE_EVENT: + { + struct v4l2_event_subscription *sub = arg; + struct v4l2_fh *vfh = fh; + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) + || vfh->events == NULL + || !ops->vidioc_subscribe_event) + break; + + ret = ops->vidioc_subscribe_event(fh, sub); + if (ret < 0) { + dbgarg(cmd, "failed, ret=%ld", ret); + break; + } + dbgarg(cmd, "type=0x%8.8x", sub->type); + break; + } + case VIDIOC_UNSUBSCRIBE_EVENT: + { + struct v4l2_event_subscription *sub = arg; + struct v4l2_fh *vfh = fh; + + if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags) + || vfh->events == NULL + || !ops->vidioc_unsubscribe_event) + break; + + ret = ops->vidioc_unsubscribe_event(fh, sub); + if (ret < 0) { + dbgarg(cmd, "failed, ret=%ld", ret); + break; + } + dbgarg(cmd, "type=0x%8.8x", sub->type); + break; + } default: { if (!ops->vidioc_default) diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h index e8ba0f2..06daa6e 100644 --- a/include/media/v4l2-ioctl.h +++ b/include/media/v4l2-ioctl.h @@ -21,6 +21,8 @@ #include <linux/videodev2.h> #endif +struct v4l2_fh; + struct v4l2_ioctl_ops { /* ioctl callbacks */ @@ -254,6 +256,11 @@ struct v4l2_ioctl_ops { int (*vidioc_g_dv_timings) (struct file *file, void *fh, struct v4l2_dv_timings *timings); + int (*vidioc_subscribe_event) (struct v4l2_fh *fh, + struct v4l2_event_subscription *sub); + int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh, + struct v4l2_event_subscription *sub); + /* For other private ioctls */ long (*vidioc_default) (struct file *file, void *fh, int cmd, void *arg);