[PATCH v4 0/7] V4L2 file handles and event interface

[PATCH v4 0/7] V4L2 file handles and event interface

[Sakari Ailus]

Hi,

Here's the sixth version of the V4L2 file handle and event interface
patchset.

The first patch adds the V4L2 file handle support and the rest are for
V4L2 events.

The patchset has been tested with the OMAP 3 ISP driver. Patches for
OMAP 3 ISP are not part of this patchset but are available in Gitorious
(branch is called event):

	git://gitorious.org/omap3camera/mainline.git event

Some more comments from Hans. What has changed:

- Events are timestamped in v4l2_event_queue().
- No ioctl handler for vidioc_dqevent or vidioc_unsubscribe_event
anymore. The __video_do_ioctl() calls directly v4l2_event_dequeue() and
v4l2_event_unsubscribe().
- v4l2_event->navailable and v4l2_event->max_alloc (was max_events) are
now unsigned int instead of atomic_t. They are modified only when the
video_device->fh_lock is held.
- No longer possible to allocate any more events than the limit in
v4l2_event_alloc().

- Possibly something else I don't happen to remember just now.

Comments are welcome as always.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

[PATCH v4 1/7] V4L: File handles

[Sakari Ailus]

This patch adds a list of v4l2_fh structures to every video_device.
It allows using file handle related information in V4L2. The event interface
is one example of such use.

Video device drivers should use the v4l2_fh pointer as their
file->private_data.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/Makefile   |    3 +-
 drivers/media/video/v4l2-dev.c |    2 +
 drivers/media/video/v4l2-fh.c  |   65 ++++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-dev.h       |    6 ++++
 include/media/v4l2-fh.h        |   47 +++++++++++++++++++++++++++++
 5 files changed, 122 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/video/v4l2-fh.c
 create mode 100644 include/media/v4l2-fh.h

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 6e75647..b888ad1 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,8 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 
 omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
 
-videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
+			v4l2-fh.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 13a899d..c24c832 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -423,6 +423,8 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 	if (!vdev->release)
 		return -EINVAL;
 
+	v4l2_fhs_init(vdev);
+
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_GRABBER:
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
new file mode 100644
index 0000000..3c1cea2
--- /dev/null
+++ b/drivers/media/video/v4l2-fh.c
@@ -0,0 +1,65 @@
+/*
+ * drivers/media/video/v4l2-fh.c
+ *
+ * V4L2 file handles.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <media/v4l2-dev.h>
+#include <media/v4l2-fh.h>
+
+void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
+{
+	fh->vdev = vdev;
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_init);
+
+void v4l2_fh_add(struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_add(&fh->list, &fh->vdev->fh_list);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_add);
+
+void v4l2_fh_del(struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_del(&fh->list);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_del);
+
+void v4l2_fh_exit(struct v4l2_fh *fh)
+{
+	BUG_ON(fh->vdev == NULL);
+	fh->vdev = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_exit);
+
+void v4l2_fhs_init(struct video_device *vdev)
+{
+	spin_lock_init(&vdev->fh_lock);
+	INIT_LIST_HEAD(&vdev->fh_list);
+}
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 26d4e79..ee3a0c9 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -18,6 +18,8 @@
 
 #include <media/media-entity.h>
 
+#include <media/v4l2-fh.h>
+
 #define VIDEO_MAJOR	81
 
 #define VFL_TYPE_GRABBER	0
@@ -82,6 +84,10 @@ struct video_device
 	/* attribute to differentiate multiple indices on one physical device */
 	int index;
 
+	/* V4L2 file handles */
+	spinlock_t		fh_lock; /* Lock for all v4l2_fhs */
+	struct list_head	fh_list; /* List of struct v4l2_fh */
+
 	int debug;			/* Activates debug level*/
 
 	/* Video standard vars */
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
new file mode 100644
index 0000000..2e88031
--- /dev/null
+++ b/include/media/v4l2-fh.h
@@ -0,0 +1,47 @@
+/*
+ * include/media/v4l2-fh.h
+ *
+ * V4L2 file handle.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef V4L2_FH_H
+#define V4L2_FH_H
+
+#include <linux/types.h>
+#include <linux/list.h>
+
+#include <asm/atomic.h>
+
+struct video_device;
+
+struct v4l2_fh {
+	struct list_head	list;
+	struct video_device	*vdev;
+};
+
+void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
+void v4l2_fh_add(struct v4l2_fh *fh);
+void v4l2_fh_del(struct v4l2_fh *fh);
+void v4l2_fh_exit(struct v4l2_fh *fh);
+
+void v4l2_fhs_init(struct video_device *vdev);
+
+#endif /* V4L2_EVENT_H */
-- 
1.5.6.5

[PATCH v4 2/7] V4L: Events: Add new ioctls for events

[Sakari Ailus]

This patch adds a set of new ioctls to the V4L2 API. The ioctls conform to
V4L2 Events RFC version 2.3:

<URL:http://www.spinics.net/lists/linux-media/msg12033.html>

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
 drivers/media/video/v4l2-ioctl.c          |    3 +++
 include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
 3 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index 997975d..cba704c 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_DBG_G_REGISTER:
 	case VIDIOC_DBG_G_CHIP_IDENT:
 	case VIDIOC_S_HW_FREQ_SEEK:
+	case VIDIOC_DQEVENT:
+	case VIDIOC_SUBSCRIBE_EVENT:
+	case VIDIOC_UNSUBSCRIBE_EVENT:
 		ret = do_video_ioctl(file, cmd, arg);
 		break;
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 30cc334..bfc4696 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
 
 	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
 	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
+	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
+	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
+	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
 #endif
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 54af357..a19ae89 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
 };
 
 /*
+ *	E V E N T S
+ */
+
+struct v4l2_event {
+	__u32		count;
+	__u32		type;
+	__u32		sequence;
+	struct timespec	timestamp;
+	__u32		reserved[9];
+	__u8		data[64];
+};
+
+struct v4l2_event_subscription {
+	__u32		type;
+	__u32		reserved[7];
+};
+
+#define V4L2_EVENT_PRIVATE_START		0x08000000
+
+/*
  *	A D V A N C E D   D E B U G G I N G
  *
  *	NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
@@ -1651,6 +1671,9 @@ struct v4l2_dbg_chip_ident {
 #endif
 
 #define VIDIOC_S_HW_FREQ_SEEK	 _IOW('V', 82, struct v4l2_hw_freq_seek)
+#define VIDIOC_DQEVENT		 _IOR('V', 83, struct v4l2_event)
+#define VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 84, struct v4l2_event_subscription)
+#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 85, struct v4l2_event_subscription)
 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/video/v4l2-compat-ioctl32.c as well! */
 
-- 
1.5.6.5

[PATCH v4 3/7] V4L: Events: Add backend

[Sakari Ailus]

Add event handling backend to V4L2. The backend handles event subscription
and delivery to file handles. Event subscriptions are based on file handle.
Events may be delivered to all subscribed file handles on a device
independent of where they originate from.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-event.c |  261 ++++++++++++++++++++++++++++++++++++++
 drivers/media/video/v4l2-fh.c    |    4 +
 include/media/v4l2-event.h       |   64 +++++++++
 include/media/v4l2-fh.h          |    2 +
 4 files changed, 331 insertions(+), 0 deletions(-)
 create mode 100644 drivers/media/video/v4l2-event.c
 create mode 100644 include/media/v4l2-event.h

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 0000000..d13c1e9
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,261 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <media/v4l2-dev.h>
+#include <media/v4l2-event.h>
+
+#include <linux/sched.h>
+
+/* In error case, return number of events *not* allocated. */
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
+{
+	struct v4l2_events *events = fh->events;
+	unsigned long flags;
+
+	for (; n > 0; n--) {
+		struct v4l2_kevent *kev;
+
+		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
+		if (kev == NULL)
+			return -ENOMEM;
+
+		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+		list_add_tail(&kev->list, &events->free);
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_alloc);
+
+#define list_kfree(list, type, member)				\
+	while (!list_empty(list)) {				\
+		type *hi;					\
+		hi = list_first_entry(list, type, member);	\
+		list_del(&hi->member);				\
+		kfree(hi);					\
+	}
+
+void v4l2_event_exit(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+
+	if (!events)
+		return;
+
+	list_kfree(&events->free, struct v4l2_kevent, list);
+	list_kfree(&events->available, struct v4l2_kevent, list);
+	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
+
+	kfree(events);
+	fh->events = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_exit);
+
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
+{
+	int ret;
+
+	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
+	if (fh->events == NULL)
+		return -ENOMEM;
+
+	init_waitqueue_head(&fh->events->wait);
+
+	INIT_LIST_HEAD(&fh->events->free);
+	INIT_LIST_HEAD(&fh->events->available);
+	INIT_LIST_HEAD(&fh->events->subscribed);
+
+	ret = v4l2_event_alloc(fh, n);
+	if (ret < 0)
+		v4l2_event_exit(fh);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_init);
+
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_kevent *kev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	if (list_empty(&events->available)) {
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+		return -ENOENT;
+	}
+
+	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
+	list_move(&kev->list, &events->free);
+
+	kev->event.count = !list_empty(&events->available);
+
+	*event = kev->event;
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
+
+static struct v4l2_subscribed_event *__v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_subscribed_event *sev;
+
+	list_for_each_entry(sev, &events->subscribed, list) {
+		if (sev->type == type)
+			return sev;
+	}
+
+	return NULL;
+}
+
+struct v4l2_subscribed_event *v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type)
+{
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	sev = __v4l2_event_subscribed(fh, type);
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	return sev;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribed);
+
+void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
+{
+	struct v4l2_fh *fh;
+	unsigned long flags;
+
+	if (!ev->timestamp.tv_sec && !ev->timestamp.tv_nsec)
+		ktime_get_ts(&ev->timestamp);
+
+	spin_lock_irqsave(&vdev->fh_lock, flags);
+
+	list_for_each_entry(fh, &vdev->fh_list, list) {
+		struct v4l2_events *events = fh->events;
+		struct v4l2_kevent *kev;
+
+		/* Do we have any free events and are we subscribed? */
+		if (list_empty(&events->free) ||
+		    !__v4l2_event_subscribed(fh, ev->type))
+			continue;
+
+		/* Take one and fill it. */
+		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
+		kev->event = *ev;
+		list_move_tail(&kev->list, &events->available);
+
+		wake_up_all(&events->wait);
+	}
+
+	spin_unlock_irqrestore(&vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_queue);
+
+int v4l2_event_pending(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	ret = !list_empty(&events->available);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_pending);
+
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+			 struct v4l2_event_subscription *sub)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+	int ret = 0;
+
+	/* Allow subscribing to valid events only. */
+	if (sub->type < V4L2_EVENT_PRIVATE_START)
+		switch (sub->type) {
+		default:
+			return -EINVAL;
+		}
+
+	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
+	if (!sev)
+		return -ENOMEM;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&sev->list);
+	sev->type = sub->type;
+
+	list_add(&sev->list, &events->subscribed);
+
+out:
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	if (ret)
+		kfree(sev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub)
+{
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	sev = __v4l2_event_subscribed(fh, sub->type);
+
+	if (sev == NULL) {
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+		return -EINVAL;
+	}
+
+	list_del(&sev->list);
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 3c1cea2..7c13f5b 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -24,6 +24,7 @@
 
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 {
@@ -54,6 +55,9 @@ EXPORT_SYMBOL_GPL(v4l2_fh_del);
 void v4l2_fh_exit(struct v4l2_fh *fh)
 {
 	BUG_ON(fh->vdev == NULL);
+
+	v4l2_event_exit(fh);
+
 	fh->vdev = NULL;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
new file mode 100644
index 0000000..580c9d4
--- /dev/null
+++ b/include/media/v4l2-event.h
@@ -0,0 +1,64 @@
+/*
+ * include/media/v4l2-event.h
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef V4L2_EVENT_H
+#define V4L2_EVENT_H
+
+#include <linux/types.h>
+#include <linux/videodev2.h>
+
+struct v4l2_fh;
+struct video_device;
+
+struct v4l2_kevent {
+	struct list_head	list;
+	struct v4l2_event	event;
+};
+
+struct v4l2_subscribed_event {
+	struct list_head	list;
+	u32			type;
+};
+
+struct v4l2_events {
+	wait_queue_head_t	wait;
+	struct list_head	subscribed; /* Subscribed events */
+	struct list_head	available; /* Dequeueable event */
+	struct list_head	free; /* Events ready for use */
+};
+
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
+void v4l2_event_exit(struct v4l2_fh *fh);
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
+struct v4l2_subscribed_event *v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type);
+void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
+int v4l2_event_pending(struct v4l2_fh *fh);
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+			 struct v4l2_event_subscription *sub);
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub);
+
+#endif /* V4L2_EVENT_H */
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 2e88031..6d03a1e 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -31,10 +31,12 @@
 #include <asm/atomic.h>
 
 struct video_device;
+struct v4l2_events;
 
 struct v4l2_fh {
 	struct list_head	list;
 	struct video_device	*vdev;
+	struct v4l2_events      *events; /* events, pending and subscribed */
 };
 
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
-- 
1.5.6.5

[PATCH v4 4/7] V4L: Events: Support event handling in do_ioctl

[Sakari Ailus]

Add support for event handling to do_ioctl.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/Makefile     |    2 +-
 drivers/media/video/v4l2-ioctl.c |   49 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ioctl.h       |    5 ++++
 3 files changed, 55 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b888ad1..68253d6 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
-			v4l2-fh.o
+			v4l2-fh.o v4l2-event.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index bfc4696..e0b9401 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -25,6 +25,7 @@
 #endif
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-chip-ident.h>
 
 #define dbgarg(cmd, fmt, arg...) \
@@ -1797,7 +1798,55 @@ static long __video_do_ioctl(struct file *file,
 		}
 		break;
 	}
+	case VIDIOC_DQEVENT:
+	{
+		struct v4l2_event *ev = arg;
+
+		if (!ops->vidioc_subscribe_event)
+			break;
+
+		ret = v4l2_event_dequeue(fh, ev);
+		if (ret < 0) {
+			dbgarg(cmd, "no pending events?");
+			break;
+		}
+		dbgarg(cmd,
+		       "count=%d, type=0x%8.8x, sequence=%d, "
+		       "timestamp=%lu.%9.9lu ",
+		       ev->count, ev->type, ev->sequence,
+		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
+		break;
+	}
+	case VIDIOC_SUBSCRIBE_EVENT:
+	{
+		struct v4l2_event_subscription *sub = arg;
 
+		if (!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;
+
+		if (!ops->vidioc_subscribe_event)
+			break;
+
+		ret = v4l2_event_unsubscribe(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 7a4529d..ed3fff5 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 */
 
@@ -239,6 +241,9 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_enum_frameintervals) (struct file *file, void *fh,
 					   struct v4l2_frmivalenum *fival);
 
+	int (*vidioc_subscribe_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);
-- 
1.5.6.5

[PATCH v4 5/7] V4L: Events: Count event queue length

[Sakari Ailus]

Update the count field properly by setting it to exactly to number of
further available events.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-event.c |   29 +++++++++++++++++------------
 include/media/v4l2-event.h       |    6 +++++-
 2 files changed, 22 insertions(+), 13 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index d13c1e9..bbdc149 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -41,7 +41,13 @@ int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
 			return -ENOMEM;
 
 		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+		if (events->max_alloc == 0) {
+			spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+			kfree(kev);
+			return -ENOMEM;
+		}
 		list_add_tail(&kev->list, &events->free);
+		events->max_alloc--;
 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 	}
 
@@ -73,7 +79,7 @@ void v4l2_event_exit(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_exit);
 
-int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n, unsigned int max_alloc)
 {
 	int ret;
 
@@ -87,6 +93,9 @@ int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
 	INIT_LIST_HEAD(&fh->events->available);
 	INIT_LIST_HEAD(&fh->events->subscribed);
 
+	fh->events->navailable = 0;
+	fh->events->max_alloc = max_alloc;
+
 	ret = v4l2_event_alloc(fh, n);
 	if (ret < 0)
 		v4l2_event_exit(fh);
@@ -108,11 +117,13 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
 		return -ENOENT;
 	}
 
+	WARN_ON(&events->navailable == 0);
+
 	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
 	list_move(&kev->list, &events->free);
+	events->navailable--;
 
-	kev->event.count = !list_empty(&events->available);
-
+	kev->event.count = events->navailable;
 	*event = kev->event;
 
 	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
@@ -175,6 +186,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		kev->event = *ev;
 		list_move_tail(&kev->list, &events->available);
 
+		events->navailable++;
+
 		wake_up_all(&events->wait);
 	}
 
@@ -184,15 +197,7 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue);
 
 int v4l2_event_pending(struct v4l2_fh *fh)
 {
-	struct v4l2_events *events = fh->events;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
-	ret = !list_empty(&events->available);
-	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
-
-	return ret;
+	return fh->events->navailable;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 580c9d4..671c8f7 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,8 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
+#include <asm/atomic.h>
+
 struct v4l2_fh;
 struct video_device;
 
@@ -45,11 +47,13 @@ struct v4l2_events {
 	wait_queue_head_t	wait;
 	struct list_head	subscribed; /* Subscribed events */
 	struct list_head	available; /* Dequeueable event */
+	unsigned int		navailable;
+	unsigned int		max_alloc; /* Never allocate more. */
 	struct list_head	free; /* Events ready for use */
 };
 
 int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
-int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n, unsigned int max_alloc);
 void v4l2_event_exit(struct v4l2_fh *fh);
 int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
 struct v4l2_subscribed_event *v4l2_event_subscribed(
-- 
1.5.6.5

[PATCH v4 6/7] V4L: Events: Sequence numbers

[Sakari Ailus]

Add sequence numbers to events.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-event.c |   16 +++++++++++++---
 include/media/v4l2-event.h       |    1 +
 2 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index bbdc149..0af0de5 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -95,6 +95,7 @@ int v4l2_event_init(struct v4l2_fh *fh, unsigned int n, unsigned int max_alloc)
 
 	fh->events->navailable = 0;
 	fh->events->max_alloc = max_alloc;
+	fh->events->sequence = -1;
 
 	ret = v4l2_event_alloc(fh, n);
 	if (ret < 0)
@@ -175,15 +176,24 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 	list_for_each_entry(fh, &vdev->fh_list, list) {
 		struct v4l2_events *events = fh->events;
 		struct v4l2_kevent *kev;
+		u32 sequence;
 
-		/* Do we have any free events and are we subscribed? */
-		if (list_empty(&events->free) ||
-		    !__v4l2_event_subscribed(fh, ev->type))
+		/* Are we subscribed? */
+		if (!__v4l2_event_subscribed(fh, ev->type))
+			continue;
+
+		/* Increase event sequence number on fh. */
+		events->sequence++;
+		sequence = events->sequence;
+
+		/* Do we have any free events? */
+		if (list_empty(&events->free))
 			continue;
 
 		/* Take one and fill it. */
 		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
 		kev->event = *ev;
+		kev->event.sequence = sequence;
 		list_move_tail(&kev->list, &events->available);
 
 		events->navailable++;
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 671c8f7..5760597 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -50,6 +50,7 @@ struct v4l2_events {
 	unsigned int		navailable;
 	unsigned int		max_alloc; /* Never allocate more. */
 	struct list_head	free; /* Events ready for use */
+	u32			sequence;
 };
 
 int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
-- 
1.5.6.5

[PATCH v4 7/7] V4L: Events: Support all events

[Sakari Ailus]

Add support for subscribing all events with a special id V4L2_EVENT_ALL. If
V4L2_EVENT_ALL is subscribed, no other events may be subscribed. Otherwise
V4L2_EVENT_ALL is considered just as any other event.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-event.c |   13 ++++++++++++-
 include/linux/videodev2.h        |    1 +
 2 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 0af0de5..68b3cf4 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -139,6 +139,14 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed(
 	struct v4l2_events *events = fh->events;
 	struct v4l2_subscribed_event *sev;
 
+	if (list_empty(&events->subscribed))
+		return NULL;
+
+	sev = list_entry(events->subscribed.next,
+			 struct v4l2_subscribed_event, list);
+	if (sev->type == V4L2_EVENT_ALL)
+		return sev;
+
 	list_for_each_entry(sev, &events->subscribed, list) {
 		if (sev->type == type)
 			return sev;
@@ -222,6 +230,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	/* Allow subscribing to valid events only. */
 	if (sub->type < V4L2_EVENT_PRIVATE_START)
 		switch (sub->type) {
+		case V4L2_EVENT_ALL:
+			break;
 		default:
 			return -EINVAL;
 		}
@@ -262,7 +272,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
 
 	sev = __v4l2_event_subscribed(fh, sub->type);
 
-	if (sev == NULL) {
+	if (sev == NULL ||
+	    (sub->type != V4L2_EVENT_ALL && sev->type == V4L2_EVENT_ALL)) {
 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
 		return -EINVAL;
 	}
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index a19ae89..9ae9a1c 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1553,6 +1553,7 @@ struct v4l2_event_subscription {
 	__u32		reserved[7];
 };
 
+#define V4L2_EVENT_ALL				0
 #define V4L2_EVENT_PRIVATE_START		0x08000000
 
 /*
-- 
1.5.6.5

Re: [PATCH v4 7/7] V4L: Events: Support all events

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote:
> Add support for subscribing all events with a special id V4L2_EVENT_ALL. If
> V4L2_EVENT_ALL is subscribed, no other events may be subscribed. Otherwise
> V4L2_EVENT_ALL is considered just as any other event.

We should do this differently. I think that EVENT_ALL should not be used
internally (i.e. in the actual list of subscribed events), but just as a
special value for the subscribe and unsubscribe ioctls. So when used with
unsubscribe you can just unsubscribe all subscribed events and when used
with subscribe, then you just subscribe all valid events (valid for that
device node).

So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to quickly
unsubscribe all events.

In order to easily add all events from the driver it would help if the
v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type
as argument rather than the whole v4l2_event_subscription struct.

You will then get something like this in the driver:

	if (sub->type == V4L2_EVENT_ALL) {
		int ret = v4l2_event_alloc(fh, 60);

		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS);
		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC);
		return ret;
	}

An alternative might be to add a v4l2_event_subscribe_all(fh, const u32 *events)
where 'events' is a 0 terminated list of events that need to be subscribed.

For each event this function would then call:

fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub);

The nice thing about that is that in the driver you have a minimum of fuss.

I'm leaning towards this second solution due to the simple driver implementation.

Handling EVENT_ALL will simplify things substantially IMHO.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-event.c |   13 ++++++++++++-
>  include/linux/videodev2.h        |    1 +
>  2 files changed, 13 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 0af0de5..68b3cf4 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -139,6 +139,14 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed(
>  	struct v4l2_events *events = fh->events;
>  	struct v4l2_subscribed_event *sev;
>  
> +	if (list_empty(&events->subscribed))
> +		return NULL;
> +
> +	sev = list_entry(events->subscribed.next,
> +			 struct v4l2_subscribed_event, list);
> +	if (sev->type == V4L2_EVENT_ALL)
> +		return sev;
> +
>  	list_for_each_entry(sev, &events->subscribed, list) {
>  		if (sev->type == type)
>  			return sev;
> @@ -222,6 +230,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
>  	/* Allow subscribing to valid events only. */
>  	if (sub->type < V4L2_EVENT_PRIVATE_START)
>  		switch (sub->type) {
> +		case V4L2_EVENT_ALL:
> +			break;
>  		default:
>  			return -EINVAL;
>  		}
> @@ -262,7 +272,8 @@ int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>  
>  	sev = __v4l2_event_subscribed(fh, sub->type);
>  
> -	if (sev == NULL) {
> +	if (sev == NULL ||
> +	    (sub->type != V4L2_EVENT_ALL && sev->type == V4L2_EVENT_ALL)) {
>  		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>  		return -EINVAL;
>  	}
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index a19ae89..9ae9a1c 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1553,6 +1553,7 @@ struct v4l2_event_subscription {
>  	__u32		reserved[7];
>  };
>  
> +#define V4L2_EVENT_ALL				0
>  #define V4L2_EVENT_PRIVATE_START		0x08000000
>  
>  /*
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v4 7/7] V4L: Events: Support all events

[Laurent Pinchart]

Hi Hans,

On Saturday 13 February 2010 15:42:20 Hans Verkuil wrote:
> On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote:
> > Add support for subscribing all events with a special id V4L2_EVENT_ALL.
> > If V4L2_EVENT_ALL is subscribed, no other events may be subscribed.
> > Otherwise V4L2_EVENT_ALL is considered just as any other event.
> 
> We should do this differently. I think that EVENT_ALL should not be used
> internally (i.e. in the actual list of subscribed events), but just as a
> special value for the subscribe and unsubscribe ioctls. So when used with
> unsubscribe you can just unsubscribe all subscribed events and when used
> with subscribe, then you just subscribe all valid events (valid for that
> device node).
> 
> So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to quickly
> unsubscribe all events.
> 
> In order to easily add all events from the driver it would help if the
> v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type
> as argument rather than the whole v4l2_event_subscription struct.
> 
> You will then get something like this in the driver:
> 
> 	if (sub->type == V4L2_EVENT_ALL) {
> 		int ret = v4l2_event_alloc(fh, 60);
> 
> 		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS);
> 		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC);
> 		return ret;
> 	}
> 
> An alternative might be to add a v4l2_event_subscribe_all(fh, const u32
> *events) where 'events' is a 0 terminated list of events that need to be
> subscribed.

Then don't call it v4l2_event_subscribe_all if it only subscribes to a set of 
event :-)

> For each event this function would then call:
> 
> fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub);
> 
> The nice thing about that is that in the driver you have a minimum of fuss.
> 
> I'm leaning towards this second solution due to the simple driver
> implementation.
> 
> Handling EVENT_ALL will simplify things substantially IMHO.

I'm wondering if subscribing to all events should be allowed. Do we have use 
cases for that ? I'm always a bit cautious when adding APIs with no users, as 
that means the API has often not been properly tested against possible use 
cases and mistakes will need to be supported forever (or at least for a long 
time).

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 7/7] V4L: Events: Support all events

[Hans Verkuil]

> Hi Hans,
>
> On Saturday 13 February 2010 15:42:20 Hans Verkuil wrote:
>> On Wednesday 10 February 2010 15:58:09 Sakari Ailus wrote:
>> > Add support for subscribing all events with a special id
>> V4L2_EVENT_ALL.
>> > If V4L2_EVENT_ALL is subscribed, no other events may be subscribed.
>> > Otherwise V4L2_EVENT_ALL is considered just as any other event.
>>
>> We should do this differently. I think that EVENT_ALL should not be used
>> internally (i.e. in the actual list of subscribed events), but just as a
>> special value for the subscribe and unsubscribe ioctls. So when used
>> with
>> unsubscribe you can just unsubscribe all subscribed events and when used
>> with subscribe, then you just subscribe all valid events (valid for that
>> device node).
>>
>> So in v4l2-event.c you will have a v4l2_event_unsubscribe_all() to
>> quickly
>> unsubscribe all events.
>>
>> In order to easily add all events from the driver it would help if the
>> v4l2_event_subscribe and v4l2_event_unsubscribe just take the event type
>> as argument rather than the whole v4l2_event_subscription struct.
>>
>> You will then get something like this in the driver:
>>
>> 	if (sub->type == V4L2_EVENT_ALL) {
>> 		int ret = v4l2_event_alloc(fh, 60);
>>
>> 		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_EOS);
>> 		ret = ret ? ret : v4l2_event_subscribe(fh, V4L2_EVENT_VSYNC);
>> 		return ret;
>> 	}
>>
>> An alternative might be to add a v4l2_event_subscribe_all(fh, const u32
>> *events) where 'events' is a 0 terminated list of events that need to be
>> subscribed.
>
> Then don't call it v4l2_event_subscribe_all if it only subscribes to a set
> of
> event :-)
>
>> For each event this function would then call:
>>
>> fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub);
>>
>> The nice thing about that is that in the driver you have a minimum of
>> fuss.
>>
>> I'm leaning towards this second solution due to the simple driver
>> implementation.
>>
>> Handling EVENT_ALL will simplify things substantially IMHO.
>
> I'm wondering if subscribing to all events should be allowed. Do we have
> use
> cases for that ? I'm always a bit cautious when adding APIs with no users,
> as
> that means the API has often not been properly tested against possible use
> cases and mistakes will need to be supported forever (or at least for a
> long
> time).

I think that is a good point. Supporting V4L2_EVENT_ALL makes sense for
unsubscribe, but does it makes sense for subscribe as well? I think it
does not. It just doesn't feel right when I tried to implement it in ivtv.

I also wonder whether the unsubscribe API shouldn't just receive the event
type instead of the big subscription struct. Or get its own struct. I
don't think it makes much sense that they both have the same struct.

Regards,

       Hans

>
> --
> Regards,
>
> Laurent Pinchart
> --
> 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
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH v4 7/7] V4L: Events: Support all events

[Sakari Ailus]

Hans Verkuil wrote:
>> Then don't call it v4l2_event_subscribe_all if it only subscribes to a set
>> of
>> event :-)
>>
>>> For each event this function would then call:
>>>
>>> fh->vdev->ioctl_ops->vidioc_subscribe_event(fh, sub);
>>>
>>> The nice thing about that is that in the driver you have a minimum of
>>> fuss.
>>>
>>> I'm leaning towards this second solution due to the simple driver
>>> implementation.
>>>
>>> Handling EVENT_ALL will simplify things substantially IMHO.
>>
>> I'm wondering if subscribing to all events should be allowed. Do we have
>> use
>> cases for that ? I'm always a bit cautious when adding APIs with no users,
>> as
>> that means the API has often not been properly tested against possible use
>> cases and mistakes will need to be supported forever (or at least for a
>> long
>> time).
> 
> I think that is a good point. Supporting V4L2_EVENT_ALL makes sense for
> unsubscribe, but does it makes sense for subscribe as well? I think it
> does not. It just doesn't feel right when I tried to implement it in ivtv.

I don't see any harm in supporting it there. We could also specify that
drivers may support that. At least for testing purposes that could be
quite useful. :-) Perhaps not for regular use, though.

> I also wonder whether the unsubscribe API shouldn't just receive the event
> type instead of the big subscription struct. Or get its own struct. I
> don't think it makes much sense that they both have the same struct.

So for unsubscribing the argument would be just event type as __u32?

I don't see harm in having the struct there. There might be flags in
future, perhaps telling that events of that type should be cleaned up
from the event queue, for example. (I can't think of any other purposes
now. :))

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v4 5/7] V4L: Events: Count event queue length

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:07 Sakari Ailus wrote:
> Update the count field properly by setting it to exactly to number of
> further available events.

After working with this for a bit I realized that the max_alloc thing is not
going to work. The idea behind it was that you can increase the event queue
up to max_alloc if there is no more free room left in the event queue for a
particular file handle. Nice idea, but the only place you can do that is in
v4l2_event_queue and doing it there will produce a locking nightmare.

The right place is when you subscribe to an event. Basically you just want
two functions: v4l2_event_alloc and v4l2_event_free. The latter replaces
v4l2_event_exit, the first replaces v4l2_event_init and the current
v4l2_event_alloc.

The new v4l2_event_alloc just specifies the size of the event queue that you
want. If it is the first time this function is called, then the fh->events
struct is allocated first. Next it just checks if the total number of allocated
events is less than the specified amount and if so it allocates events until
the required number is reached.

So typically you would only call v4l2_event_alloc when the user subscribes
to a particular event. And based on the event type you can call this function
with the desired queue size. For example in the case of ivtv the EOS event
would need just a single event while the VSYNC event might need 60.

So as long as the user does not subscribe to an event there is no memory
wasted. Once he does you will only allocate as much memory as is needed
for that particular event.

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-event.c |   29 +++++++++++++++++------------
>  include/media/v4l2-event.h       |    6 +++++-
>  2 files changed, 22 insertions(+), 13 deletions(-)
> 

<cut>

> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 580c9d4..671c8f7 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -28,6 +28,8 @@
>  #include <linux/types.h>
>  #include <linux/videodev2.h>
>  
> +#include <asm/atomic.h>

This header is not needed.

Regards,

	Hans

> +
>  struct v4l2_fh;
>  struct video_device;
>  
> @@ -45,11 +47,13 @@ struct v4l2_events {
>  	wait_queue_head_t	wait;
>  	struct list_head	subscribed; /* Subscribed events */
>  	struct list_head	available; /* Dequeueable event */
> +	unsigned int		navailable;
> +	unsigned int		max_alloc; /* Never allocate more. */
>  	struct list_head	free; /* Events ready for use */
>  };
>  
>  int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> -int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
> +int v4l2_event_init(struct v4l2_fh *fh, unsigned int n, unsigned int max_alloc);
>  void v4l2_event_exit(struct v4l2_fh *fh);
>  int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
>  struct v4l2_subscribed_event *v4l2_event_subscribed(
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v4 4/7] V4L: Events: Support event handling in do_ioctl

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:06 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/Makefile     |    2 +-
>  drivers/media/video/v4l2-ioctl.c |   49 ++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-ioctl.h       |    5 ++++
>  3 files changed, 55 insertions(+), 1 deletions(-)
> 
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index b888ad1..68253d6 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
>  
>  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
> -			v4l2-fh.o
> +			v4l2-fh.o v4l2-event.o
>  
>  # V4L2 core modules
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index bfc4696..e0b9401 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -25,6 +25,7 @@
>  #endif
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-chip-ident.h>
>  
>  #define dbgarg(cmd, fmt, arg...) \
> @@ -1797,7 +1798,55 @@ static long __video_do_ioctl(struct file *file,
>  		}
>  		break;
>  	}
> +	case VIDIOC_DQEVENT:
> +	{
> +		struct v4l2_event *ev = arg;
> +
> +		if (!ops->vidioc_subscribe_event)
> +			break;
> +
> +		ret = v4l2_event_dequeue(fh, ev);
> +		if (ret < 0) {
> +			dbgarg(cmd, "no pending events?");
> +			break;
> +		}
> +		dbgarg(cmd,
> +		       "count=%d, type=0x%8.8x, sequence=%d, "
> +		       "timestamp=%lu.%9.9lu ",
> +		       ev->count, ev->type, ev->sequence,
> +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> +		break;
> +	}
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +	{
> +		struct v4l2_event_subscription *sub = arg;
>  
> +		if (!ops->vidioc_subscribe_event)
> +			break;

I know I said that we could use this test to determine if fh is of type
v4l2_fh, but that only works in this specific case, but not in the general
case. For example, I want to add support for the prio ioctls to v4l2_fh, and
then I probably have no vidioc_subscribe_event set since few drivers will
need that.

Instead I suggest that we add a new flag to v4l2-dev.h:

V4L2_FL_USES_V4L2_FH (1)

The v4l2_fh_add() function can then set this flag.

> +
> +		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;
> +
> +		if (!ops->vidioc_subscribe_event)
> +			break;
> +
> +		ret = v4l2_event_unsubscribe(fh, sub);

We should add an unsubscribe op as well. One reason is to add EVENT_ALL
support (see my comments in patch 7/7), the other is that in some cases
drivers might need to take some special action in response to subscribing
an event. And a driver needs a way to undo that when unsubscribing.

Regards,

	Hans

> +		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 7a4529d..ed3fff5 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 */
>  
> @@ -239,6 +241,9 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_enum_frameintervals) (struct file *file, void *fh,
>  					   struct v4l2_frmivalenum *fival);
>  
> +	int (*vidioc_subscribe_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);
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v4 4/7] V4L: Events: Support event handling in do_ioctl

[Laurent Pinchart]

On Saturday 13 February 2010 14:49:31 Hans Verkuil wrote:
> On Wednesday 10 February 2010 15:58:06 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/Makefile     |    2 +-
> >  drivers/media/video/v4l2-ioctl.c |   49
> >  ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h      
> >  |    5 ++++
> >  3 files changed, 55 insertions(+), 1 deletions(-)
> > 
> > diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> > index b888ad1..68253d6 100644
> > --- a/drivers/media/video/Makefile
> > +++ b/drivers/media/video/Makefile
> > @@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
> > 
> >  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
> >  
> >  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
> > 
> > -			v4l2-fh.o
> > +			v4l2-fh.o v4l2-event.o
> > 
> >  # V4L2 core modules
> > 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index bfc4696..e0b9401 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -25,6 +25,7 @@
> > 
> >  #endif
> >  #include <media/v4l2-common.h>
> >  #include <media/v4l2-ioctl.h>
> > 
> > +#include <media/v4l2-event.h>
> > 
> >  #include <media/v4l2-chip-ident.h>
> >  
> >  #define dbgarg(cmd, fmt, arg...) \
> > 
> > @@ -1797,7 +1798,55 @@ static long __video_do_ioctl(struct file *file,
> > 
> >  		}
> >  		break;
> >  	
> >  	}
> > 
> > +	case VIDIOC_DQEVENT:
> > +	{
> > +		struct v4l2_event *ev = arg;
> > +
> > +		if (!ops->vidioc_subscribe_event)
> > +			break;
> > +
> > +		ret = v4l2_event_dequeue(fh, ev);
> > +		if (ret < 0) {
> > +			dbgarg(cmd, "no pending events?");
> > +			break;
> > +		}
> > +		dbgarg(cmd,
> > +		       "count=%d, type=0x%8.8x, sequence=%d, "
> > +		       "timestamp=%lu.%9.9lu ",
> > +		       ev->count, ev->type, ev->sequence,
> > +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> > +		break;
> > +	}
> > +	case VIDIOC_SUBSCRIBE_EVENT:
> > +	{
> > +		struct v4l2_event_subscription *sub = arg;
> > 
> > +		if (!ops->vidioc_subscribe_event)
> > +			break;
> 
> I know I said that we could use this test to determine if fh is of type
> v4l2_fh, but that only works in this specific case, but not in the general
> case. For example, I want to add support for the prio ioctls to v4l2_fh,
> and then I probably have no vidioc_subscribe_event set since few drivers
> will need that.
> 
> Instead I suggest that we add a new flag to v4l2-dev.h:
> 
> V4L2_FL_USES_V4L2_FH (1)
> 
> The v4l2_fh_add() function can then set this flag.
> 
> > +
> > +		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;
> > +
> > +		if (!ops->vidioc_subscribe_event)
> > +			break;
> > +
> > +		ret = v4l2_event_unsubscribe(fh, sub);
> 
> We should add an unsubscribe op as well. One reason is to add EVENT_ALL
> support (see my comments in patch 7/7), the other is that in some cases
> drivers might need to take some special action in response to subscribing
> an event. And a driver needs a way to undo that when unsubscribing.

Agreed. Should we allow drivers not to define the unsubscribe operation when 
they don't need it ? In that case v4l2_event_unsubscribe should be called in 
VIDIOC_UNSUBSCRIBE_EVENT, outside of the operation handler.

Similarly, shouldn't v4l2_event_subscribe be called in VIDIOC_SUBSCRIBE_EVENT, 
outside of the operation handler ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 4/7] V4L: Events: Support event handling in do_ioctl

[Hans Verkuil]

> On Saturday 13 February 2010 14:49:31 Hans Verkuil wrote:
>> On Wednesday 10 February 2010 15:58:06 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/Makefile     |    2 +-
>> >  drivers/media/video/v4l2-ioctl.c |   49
>> >  ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h
>> >  |    5 ++++
>> >  3 files changed, 55 insertions(+), 1 deletions(-)
>> >
>> > diff --git a/drivers/media/video/Makefile
>> b/drivers/media/video/Makefile
>> > index b888ad1..68253d6 100644
>> > --- a/drivers/media/video/Makefile
>> > +++ b/drivers/media/video/Makefile
>> > @@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>> >
>> >  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
>> >
>> >  videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
>> \
>> >
>> > -			v4l2-fh.o
>> > +			v4l2-fh.o v4l2-event.o
>> >
>> >  # V4L2 core modules
>> >
>> > diff --git a/drivers/media/video/v4l2-ioctl.c
>> > b/drivers/media/video/v4l2-ioctl.c index bfc4696..e0b9401 100644
>> > --- a/drivers/media/video/v4l2-ioctl.c
>> > +++ b/drivers/media/video/v4l2-ioctl.c
>> > @@ -25,6 +25,7 @@
>> >
>> >  #endif
>> >  #include <media/v4l2-common.h>
>> >  #include <media/v4l2-ioctl.h>
>> >
>> > +#include <media/v4l2-event.h>
>> >
>> >  #include <media/v4l2-chip-ident.h>
>> >
>> >  #define dbgarg(cmd, fmt, arg...) \
>> >
>> > @@ -1797,7 +1798,55 @@ static long __video_do_ioctl(struct file *file,
>> >
>> >  		}
>> >  		break;
>> >
>> >  	}
>> >
>> > +	case VIDIOC_DQEVENT:
>> > +	{
>> > +		struct v4l2_event *ev = arg;
>> > +
>> > +		if (!ops->vidioc_subscribe_event)
>> > +			break;
>> > +
>> > +		ret = v4l2_event_dequeue(fh, ev);
>> > +		if (ret < 0) {
>> > +			dbgarg(cmd, "no pending events?");
>> > +			break;
>> > +		}
>> > +		dbgarg(cmd,
>> > +		       "count=%d, type=0x%8.8x, sequence=%d, "
>> > +		       "timestamp=%lu.%9.9lu ",
>> > +		       ev->count, ev->type, ev->sequence,
>> > +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
>> > +		break;
>> > +	}
>> > +	case VIDIOC_SUBSCRIBE_EVENT:
>> > +	{
>> > +		struct v4l2_event_subscription *sub = arg;
>> >
>> > +		if (!ops->vidioc_subscribe_event)
>> > +			break;
>>
>> I know I said that we could use this test to determine if fh is of type
>> v4l2_fh, but that only works in this specific case, but not in the
>> general
>> case. For example, I want to add support for the prio ioctls to v4l2_fh,
>> and then I probably have no vidioc_subscribe_event set since few drivers
>> will need that.
>>
>> Instead I suggest that we add a new flag to v4l2-dev.h:
>>
>> V4L2_FL_USES_V4L2_FH (1)
>>
>> The v4l2_fh_add() function can then set this flag.
>>
>> > +
>> > +		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;
>> > +
>> > +		if (!ops->vidioc_subscribe_event)
>> > +			break;
>> > +
>> > +		ret = v4l2_event_unsubscribe(fh, sub);
>>
>> We should add an unsubscribe op as well. One reason is to add EVENT_ALL
>> support (see my comments in patch 7/7), the other is that in some cases
>> drivers might need to take some special action in response to
>> subscribing
>> an event. And a driver needs a way to undo that when unsubscribing.
>
> Agreed. Should we allow drivers not to define the unsubscribe operation
> when
> they don't need it ? In that case v4l2_event_unsubscribe should be called
> in
> VIDIOC_UNSUBSCRIBE_EVENT, outside of the operation handler.

Drivers can just use v4l2_event_unsubscribe directly as the ioctl op.

>
> Similarly, shouldn't v4l2_event_subscribe be called in
> VIDIOC_SUBSCRIBE_EVENT,
> outside of the operation handler ?

It can be done, but I am no fan of hiding such things in the core. It's
just a single function, after all.

Regards,

       Hans

>
> --
> Regards,
>
> Laurent Pinchart
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH v4 3/7] V4L: Events: Add backend

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:05 Sakari Ailus wrote:
> Add event handling backend to V4L2. The backend handles event subscription
> and delivery to file handles. Event subscriptions are based on file handle.
> Events may be delivered to all subscribed file handles on a device
> independent of where they originate from.

More comments based on my attempt to implement this in ivtv...

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-event.c |  261 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-fh.c    |    4 +
>  include/media/v4l2-event.h       |   64 +++++++++
>  include/media/v4l2-fh.h          |    2 +
>  4 files changed, 331 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-event.c
>  create mode 100644 include/media/v4l2-event.h
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..d13c1e9
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,261 @@
> +/*
> + * drivers/media/video/v4l2-event.c
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +/* In error case, return number of events *not* allocated. */
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +
> +	for (; n > 0; n--) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
> +		if (kev == NULL)
> +			return -ENOMEM;
> +
> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +		list_add_tail(&kev->list, &events->free);
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_alloc);
> +
> +#define list_kfree(list, type, member)				\
> +	while (!list_empty(list)) {				\
> +		type *hi;					\
> +		hi = list_first_entry(list, type, member);	\
> +		list_del(&hi->member);				\
> +		kfree(hi);					\
> +	}
> +
> +void v4l2_event_exit(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +
> +	if (!events)
> +		return;
> +
> +	list_kfree(&events->free, struct v4l2_kevent, list);
> +	list_kfree(&events->available, struct v4l2_kevent, list);
> +	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
> +
> +	kfree(events);
> +	fh->events = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit);
> +
> +int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
> +{
> +	int ret;
> +
> +	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
> +	if (fh->events == NULL)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&fh->events->wait);
> +
> +	INIT_LIST_HEAD(&fh->events->free);
> +	INIT_LIST_HEAD(&fh->events->available);
> +	INIT_LIST_HEAD(&fh->events->subscribed);
> +
> +	ret = v4l2_event_alloc(fh, n);
> +	if (ret < 0)
> +		v4l2_event_exit(fh);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_kevent *kev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
> +	list_move(&kev->list, &events->free);
> +
> +	kev->event.count = !list_empty(&events->available);
> +
> +	*event = kev->event;
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
> +
> +static struct v4l2_subscribed_event *__v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_subscribed_event *sev;
> +
> +	list_for_each_entry(sev, &events->subscribed, list) {
> +		if (sev->type == type)
> +			return sev;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	sev = __v4l2_event_subscribed(fh, type);
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return sev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribed);

The result pointer is unusable since right after the spin_unlock someone
might unsubscribe this, leaving you with an invalid pointer.

So either this function should return a bool (which might be out of date
as well), or be removed altogether. Frankly, I do not see a need for this
and I would remove it.

> +
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)

Make this a const struct v4l2_event *ev. That allows you to define static const
event structs.

> +{
> +	struct v4l2_fh *fh;
> +	unsigned long flags;

	struct timespec ts;

> +
> +	if (!ev->timestamp.tv_sec && !ev->timestamp.tv_nsec)
> +		ktime_get_ts(&ev->timestamp);

This becomes:

	if (ev->timestamp.tv_sec || ev->timestamp.tv_nsec)
		ts = ev->timestamp;
	else
		ktime_get_ts(&ts);

> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fh_list, list) {
> +		struct v4l2_events *events = fh->events;
> +		struct v4l2_kevent *kev;
> +
> +		/* Do we have any free events and are we subscribed? */
> +		if (list_empty(&events->free) ||
> +		    !__v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		/* Take one and fill it. */
> +		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
> +		kev->event = *ev;

This becomes:

                kev->event.type = ev->type;
		kev->event.u = ev->u;
                kev->event.timestamp = ts;

> +		list_move_tail(&kev->list, &events->available);
> +
> +		wake_up_all(&events->wait);
> +	}
> +
> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	ret = !list_empty(&events->available);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
> +
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* Allow subscribing to valid events only. */
> +	if (sub->type < V4L2_EVENT_PRIVATE_START)
> +		switch (sub->type) {
> +		default:
> +			return -EINVAL;
> +		}

This part makes no sense. The driver should check for valid events, we cannot
do that here.

The only check that is needed here is a check against V4L2_EVENT_ALL since that
shouldn't be allowed. More about that in the comments for the patch adding
EVENT_ALL support.

> +
> +	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
> +	if (!sev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
> +		ret = -EBUSY;

I think we should just return 0 here. I see no reason why subscribing an
already subscribed event should return an error. The call succeeds after all:
the event *is* subscribed.

> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&sev->list);
> +	sev->type = sub->type;
> +
> +	list_add(&sev->list, &events->subscribed);
> +
> +out:
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (ret)
> +		kfree(sev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +

Check for V4L2_EVENT_ALL and return -EINVAL in that case.

> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	sev = __v4l2_event_subscribed(fh, sub->type);
> +
> +	if (sev == NULL) {
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		return -EINVAL;

Same as above: just return 0.

> +	}
> +
> +	list_del(&sev->list);

Missing kfree(sev);

> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 3c1cea2..7c13f5b 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -24,6 +24,7 @@
>  
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  {
> @@ -54,6 +55,9 @@ EXPORT_SYMBOL_GPL(v4l2_fh_del);
>  void v4l2_fh_exit(struct v4l2_fh *fh)
>  {
>  	BUG_ON(fh->vdev == NULL);
> +
> +	v4l2_event_exit(fh);
> +
>  	fh->vdev = NULL;
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..580c9d4
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,64 @@
> +/*
> + * include/media/v4l2-event.h
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef V4L2_EVENT_H
> +#define V4L2_EVENT_H
> +
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +
> +struct v4l2_fh;
> +struct video_device;
> +
> +struct v4l2_kevent {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +struct v4l2_events {
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events */
> +	struct list_head	available; /* Dequeueable event */
> +	struct list_head	free; /* Events ready for use */
> +};
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> +int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
> +void v4l2_event_exit(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type);
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
> +int v4l2_event_pending(struct v4l2_fh *fh);
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub);
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub);
> +
> +#endif /* V4L2_EVENT_H */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index 2e88031..6d03a1e 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -31,10 +31,12 @@
>  #include <asm/atomic.h>
>  
>  struct video_device;
> +struct v4l2_events;
>  
>  struct v4l2_fh {
>  	struct list_head	list;
>  	struct video_device	*vdev;
> +	struct v4l2_events      *events; /* events, pending and subscribed */
>  };
>  
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v4 2/7] V4L: Events: Add new ioctls for events

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> This patch adds a set of new ioctls to the V4L2 API. The ioctls conform to
> V4L2 Events RFC version 2.3:

I've experimented with the events API to try and support it with ivtv and
I realized that it had some problems.

See comments below.

> 
> <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
>  drivers/media/video/v4l2-ioctl.c          |    3 +++
>  include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
>  3 files changed, 29 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
> index 997975d..cba704c 100644
> --- a/drivers/media/video/v4l2-compat-ioctl32.c
> +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
>  	case VIDIOC_DBG_G_REGISTER:
>  	case VIDIOC_DBG_G_CHIP_IDENT:
>  	case VIDIOC_S_HW_FREQ_SEEK:
> +	case VIDIOC_DQEVENT:
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +	case VIDIOC_UNSUBSCRIBE_EVENT:
>  		ret = do_video_ioctl(file, cmd, arg);
>  		break;
>  
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 30cc334..bfc4696 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
>  
>  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
>  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>  #endif
>  };
>  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 54af357..a19ae89 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
>  };
>  
>  /*
> + *	E V E N T S
> + */
> +
> +struct v4l2_event {
> +	__u32		count;

The name 'count' is confusing. Count of what? I think the name 'pending' might
be more understandable. A comment after the definition would also help.

> +	__u32		type;
> +	__u32		sequence;
> +	struct timespec	timestamp;
> +	__u32		reserved[9];
> +	__u8		data[64];
> +};

I also think we should reorder the fields and add a union. For ivtv I would
need this:

#define V4L2_EVENT_ALL                          0
#define V4L2_EVENT_VSYNC                        1
#define V4L2_EVENT_EOS                          2
#define V4L2_EVENT_PRIVATE_START                0x08000000

/* Payload for V4L2_EVENT_VSYNC */
struct v4l2_event_vsync {
        /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
        u8 field;
} __attribute__ ((packed));

struct v4l2_event {
        __u32           type;
        union {
                struct v4l2_event_vsync vsync;
                __u8    data[64];
        } u;
        __u32           sequence;
        struct timespec timestamp;
        __u32           pending;
        __u32           reserved[9];
};

The reason for rearranging the fields has to do with the fact that the first
two fields (type and the union) form the actual event data. The others are
more for administrative purposes. Separating those two makes sense to me.

So when I define an event for queuing it is nice if I can do just this:

static const struct v4l2_event ev_top = {
	.type = V4L2_EVENT_VSYNC,
	.u.vsync.field = V4L2_FIELD_TOP,
};

I would have preferred to have an anonymous union. Unfortunately gcc has
problems with initializers for fields inside an anonymous union. Hence the
need for a named union.

Regards,

	Hans

> +
> +struct v4l2_event_subscription {
> +	__u32		type;
> +	__u32		reserved[7];
> +};
> +
> +#define V4L2_EVENT_PRIVATE_START		0x08000000
> +
> +/*
>   *	A D V A N C E D   D E B U G G I N G
>   *
>   *	NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
> @@ -1651,6 +1671,9 @@ struct v4l2_dbg_chip_ident {
>  #endif
>  
>  #define VIDIOC_S_HW_FREQ_SEEK	 _IOW('V', 82, struct v4l2_hw_freq_seek)
> +#define VIDIOC_DQEVENT		 _IOR('V', 83, struct v4l2_event)
> +#define VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 84, struct v4l2_event_subscription)
> +#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 85, struct v4l2_event_subscription)
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/video/v4l2-compat-ioctl32.c as well! */
>  
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v4 2/7] V4L: Events: Add new ioctls for events

[Laurent Pinchart]

Hi Hans,

On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> > This patch adds a set of new ioctls to the V4L2 API. The ioctls conform
> > to V4L2 Events RFC version 2.3:
> I've experimented with the events API to try and support it with ivtv and
> I realized that it had some problems.
> 
> See comments below.
> 
> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > ---
> > 
> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
> >  include/linux/videodev2.h                 |   23 +++++++++++++++++++++++
> >  3 files changed, 29 insertions(+), 0 deletions(-)
> > 
> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
> > 100644
> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
> > unsigned int cmd, unsigned long arg)
> > 
> >  	case VIDIOC_DBG_G_REGISTER:
> >  	case VIDIOC_DBG_G_CHIP_IDENT:
> > 
> >  	case VIDIOC_S_HW_FREQ_SEEK:
> > +	case VIDIOC_DQEVENT:
> > +	case VIDIOC_SUBSCRIBE_EVENT:
> > 
> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
> >  		ret = do_video_ioctl(file, cmd, arg);
> >  		break;
> > 
> > diff --git a/drivers/media/video/v4l2-ioctl.c
> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
> > --- a/drivers/media/video/v4l2-ioctl.c
> > +++ b/drivers/media/video/v4l2-ioctl.c
> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
> > 
> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> > 
> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> > 
> >  #endif
> >  };
> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> > 
> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> > index 54af357..a19ae89 100644
> > --- a/include/linux/videodev2.h
> > +++ b/include/linux/videodev2.h
> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
> > 
> >  };
> >  
> >  /*
> > 
> > + *	E V E N T S
> > + */
> > +
> > +struct v4l2_event {
> > +	__u32		count;
> 
> The name 'count' is confusing. Count of what? I think the name 'pending'
> might be more understandable. A comment after the definition would also
> help.
> 
> > +	__u32		type;
> > +	__u32		sequence;
> > +	struct timespec	timestamp;
> > +	__u32		reserved[9];
> > +	__u8		data[64];
> > +};
> 
> I also think we should reorder the fields and add a union. For ivtv I would
> need this:
> 
> #define V4L2_EVENT_ALL                          0
> #define V4L2_EVENT_VSYNC                        1
> #define V4L2_EVENT_EOS                          2
> #define V4L2_EVENT_PRIVATE_START                0x08000000
> 
> /* Payload for V4L2_EVENT_VSYNC */
> struct v4l2_event_vsync {
>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
>         u8 field;
> } __attribute__ ((packed));
> 
> struct v4l2_event {
>         __u32           type;
>         union {
>                 struct v4l2_event_vsync vsync;
>                 __u8    data[64];
>         } u;
>         __u32           sequence;
>         struct timespec timestamp;
>         __u32           pending;
>         __u32           reserved[9];
> };
> 
> The reason for rearranging the fields has to do with the fact that the
> first two fields (type and the union) form the actual event data. The
> others are more for administrative purposes. Separating those two makes
> sense to me.
> 
> So when I define an event for queuing it is nice if I can do just this:
> 
> static const struct v4l2_event ev_top = {
> 	.type = V4L2_EVENT_VSYNC,
> 	.u.vsync.field = V4L2_FIELD_TOP,
> };
> 
> I would have preferred to have an anonymous union. Unfortunately gcc has
> problems with initializers for fields inside an anonymous union. Hence the
> need for a named union.

Will all drivers add private events to the union ? This would then soon become 
a mess. Wouldn't it be better for drivers to define their own event structures 
(standard ones could be shared between drivers in videodev2.h) and cast the 
pointer to data to a pointer to the appropriate event structure ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 2/7] V4L: Events: Add new ioctls for events

[Hans Verkuil]

> Hi Hans,
>
> On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
>> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
>> > This patch adds a set of new ioctls to the V4L2 API. The ioctls
>> conform
>> > to V4L2 Events RFC version 2.3:
>> I've experimented with the events API to try and support it with ivtv
>> and
>> I realized that it had some problems.
>>
>> See comments below.
>>
>> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
>> >
>> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> > ---
>> >
>> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
>> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
>> >  include/linux/videodev2.h                 |   23
>> +++++++++++++++++++++++
>> >  3 files changed, 29 insertions(+), 0 deletions(-)
>> >
>> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
>> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
>> > 100644
>> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
>> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
>> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
>> > unsigned int cmd, unsigned long arg)
>> >
>> >  	case VIDIOC_DBG_G_REGISTER:
>> >  	case VIDIOC_DBG_G_CHIP_IDENT:
>> >
>> >  	case VIDIOC_S_HW_FREQ_SEEK:
>> > +	case VIDIOC_DQEVENT:
>> > +	case VIDIOC_SUBSCRIBE_EVENT:
>> >
>> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
>> >  		ret = do_video_ioctl(file, cmd, arg);
>> >  		break;
>> >
>> > diff --git a/drivers/media/video/v4l2-ioctl.c
>> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
>> > --- a/drivers/media/video/v4l2-ioctl.c
>> > +++ b/drivers/media/video/v4l2-ioctl.c
>> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
>> >
>> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
>> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
>> >
>> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
>> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
>> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
>> >
>> >  #endif
>> >  };
>> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
>> >
>> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
>> > index 54af357..a19ae89 100644
>> > --- a/include/linux/videodev2.h
>> > +++ b/include/linux/videodev2.h
>> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
>> >
>> >  };
>> >
>> >  /*
>> >
>> > + *	E V E N T S
>> > + */
>> > +
>> > +struct v4l2_event {
>> > +	__u32		count;
>>
>> The name 'count' is confusing. Count of what? I think the name 'pending'
>> might be more understandable. A comment after the definition would also
>> help.
>>
>> > +	__u32		type;
>> > +	__u32		sequence;
>> > +	struct timespec	timestamp;
>> > +	__u32		reserved[9];
>> > +	__u8		data[64];
>> > +};
>>
>> I also think we should reorder the fields and add a union. For ivtv I
>> would
>> need this:
>>
>> #define V4L2_EVENT_ALL                          0
>> #define V4L2_EVENT_VSYNC                        1
>> #define V4L2_EVENT_EOS                          2
>> #define V4L2_EVENT_PRIVATE_START                0x08000000
>>
>> /* Payload for V4L2_EVENT_VSYNC */
>> struct v4l2_event_vsync {
>>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
>>         u8 field;
>> } __attribute__ ((packed));
>>
>> struct v4l2_event {
>>         __u32           type;
>>         union {
>>                 struct v4l2_event_vsync vsync;
>>                 __u8    data[64];
>>         } u;
>>         __u32           sequence;
>>         struct timespec timestamp;
>>         __u32           pending;
>>         __u32           reserved[9];
>> };
>>
>> The reason for rearranging the fields has to do with the fact that the
>> first two fields (type and the union) form the actual event data. The
>> others are more for administrative purposes. Separating those two makes
>> sense to me.
>>
>> So when I define an event for queuing it is nice if I can do just this:
>>
>> static const struct v4l2_event ev_top = {
>> 	.type = V4L2_EVENT_VSYNC,
>> 	.u.vsync.field = V4L2_FIELD_TOP,
>> };
>>
>> I would have preferred to have an anonymous union. Unfortunately gcc has
>> problems with initializers for fields inside an anonymous union. Hence
>> the
>> need for a named union.
>
> Will all drivers add private events to the union ? This would then soon
> become
> a mess. Wouldn't it be better for drivers to define their own event
> structures
> (standard ones could be shared between drivers in videodev2.h) and cast
> the
> pointer to data to a pointer to the appropriate event structure ?

I would prefer to have the actual event type defines in videodev2.h (just
as we do for private control IDs). The actual payload structure can be
defined elsewhere as far as I am concerned.

An alternative in the long run might be to split off the event structs
into a separate public header. I have been thinking along those lines as
well for the controls. videodev2.h is getting pretty huge and it might be
more managable if it is split up in multiple parts with videodev2.h
including those parts.

Regards,

        Hans

>
> --
> Regards,
>
> Laurent Pinchart
>


-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH v4 2/7] V4L: Events: Add new ioctls for events

[Laurent Pinchart]

Hi Hans,

On Monday 15 February 2010 11:28:52 Hans Verkuil wrote:
> > Hi Hans,
> > 
> > On Saturday 13 February 2010 14:19:55 Hans Verkuil wrote:
> >> On Wednesday 10 February 2010 15:58:04 Sakari Ailus wrote:
> >> > This patch adds a set of new ioctls to the V4L2 API. The ioctls
> >> 
> >> conform
> >> 
> >> > to V4L2 Events RFC version 2.3:
> >> I've experimented with the events API to try and support it with ivtv
> >> and
> >> I realized that it had some problems.
> >> 
> >> See comments below.
> >> 
> >> > <URL:http://www.spinics.net/lists/linux-media/msg12033.html>
> >> > 
> >> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> >> > ---
> >> > 
> >> >  drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
> >> >  drivers/media/video/v4l2-ioctl.c          |    3 +++
> >> >  include/linux/videodev2.h                 |   23
> >> 
> >> +++++++++++++++++++++++
> >> 
> >> >  3 files changed, 29 insertions(+), 0 deletions(-)
> >> > 
> >> > diff --git a/drivers/media/video/v4l2-compat-ioctl32.c
> >> > b/drivers/media/video/v4l2-compat-ioctl32.c index 997975d..cba704c
> >> > 100644
> >> > --- a/drivers/media/video/v4l2-compat-ioctl32.c
> >> > +++ b/drivers/media/video/v4l2-compat-ioctl32.c
> >> > @@ -1077,6 +1077,9 @@ long v4l2_compat_ioctl32(struct file *file,
> >> > unsigned int cmd, unsigned long arg)
> >> > 
> >> >  	case VIDIOC_DBG_G_REGISTER:
> >> >  	case VIDIOC_DBG_G_CHIP_IDENT:
> >> > 
> >> >  	case VIDIOC_S_HW_FREQ_SEEK:
> >> > +	case VIDIOC_DQEVENT:
> >> > +	case VIDIOC_SUBSCRIBE_EVENT:
> >> > 
> >> > +	case VIDIOC_UNSUBSCRIBE_EVENT:
> >> >  		ret = do_video_ioctl(file, cmd, arg);
> >> >  		break;
> >> > 
> >> > diff --git a/drivers/media/video/v4l2-ioctl.c
> >> > b/drivers/media/video/v4l2-ioctl.c index 30cc334..bfc4696 100644
> >> > --- a/drivers/media/video/v4l2-ioctl.c
> >> > +++ b/drivers/media/video/v4l2-ioctl.c
> >> > @@ -283,6 +283,9 @@ static const char *v4l2_ioctls[] = {
> >> > 
> >> >  	[_IOC_NR(VIDIOC_DBG_G_CHIP_IDENT)] = "VIDIOC_DBG_G_CHIP_IDENT",
> >> >  	[_IOC_NR(VIDIOC_S_HW_FREQ_SEEK)]   = "VIDIOC_S_HW_FREQ_SEEK",
> >> > 
> >> > +	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
> >> > +	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
> >> > +	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
> >> > 
> >> >  #endif
> >> >  };
> >> >  #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
> >> > 
> >> > diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> >> > index 54af357..a19ae89 100644
> >> > --- a/include/linux/videodev2.h
> >> > +++ b/include/linux/videodev2.h
> >> > @@ -1536,6 +1536,26 @@ struct v4l2_streamparm {
> >> > 
> >> >  };
> >> >  
> >> >  /*
> >> > 
> >> > + *	E V E N T S
> >> > + */
> >> > +
> >> > +struct v4l2_event {
> >> > +	__u32		count;
> >> 
> >> The name 'count' is confusing. Count of what? I think the name 'pending'
> >> might be more understandable. A comment after the definition would also
> >> help.
> >> 
> >> > +	__u32		type;
> >> > +	__u32		sequence;
> >> > +	struct timespec	timestamp;
> >> > +	__u32		reserved[9];
> >> > +	__u8		data[64];
> >> > +};
> >> 
> >> I also think we should reorder the fields and add a union. For ivtv I
> >> would
> >> need this:
> >> 
> >> #define V4L2_EVENT_ALL                          0
> >> #define V4L2_EVENT_VSYNC                        1
> >> #define V4L2_EVENT_EOS                          2
> >> #define V4L2_EVENT_PRIVATE_START                0x08000000
> >> 
> >> /* Payload for V4L2_EVENT_VSYNC */
> >> struct v4l2_event_vsync {
> >> 
> >>         /* Can be V4L2_FIELD_ANY, _NONE, _TOP or _BOTTOM */
> >>         u8 field;
> >> 
> >> } __attribute__ ((packed));
> >> 
> >> struct v4l2_event {
> >> 
> >>         __u32           type;
> >>         union {
> >>         
> >>                 struct v4l2_event_vsync vsync;
> >>                 __u8    data[64];
> >>         
> >>         } u;
> >>         __u32           sequence;
> >>         struct timespec timestamp;
> >>         __u32           pending;
> >>         __u32           reserved[9];
> >> 
> >> };
> >> 
> >> The reason for rearranging the fields has to do with the fact that the
> >> first two fields (type and the union) form the actual event data. The
> >> others are more for administrative purposes. Separating those two makes
> >> sense to me.
> >> 
> >> So when I define an event for queuing it is nice if I can do just this:
> >> 
> >> static const struct v4l2_event ev_top = {
> >> 
> >> 	.type = V4L2_EVENT_VSYNC,
> >> 	.u.vsync.field = V4L2_FIELD_TOP,
> >> 
> >> };
> >> 
> >> I would have preferred to have an anonymous union. Unfortunately gcc has
> >> problems with initializers for fields inside an anonymous union. Hence
> >> the need for a named union.
> > 
> > Will all drivers add private events to the union ? This would then soon
> > become a mess. Wouldn't it be better for drivers to define their own event
> > structures (standard ones could be shared between drivers in videodev2.h)
> > and cast the pointer to data to a pointer to the appropriate event
> > structure ?
> 
> I would prefer to have the actual event type defines in videodev2.h (just
> as we do for private control IDs). The actual payload structure can be
> defined elsewhere as far as I am concerned.

I'm OK with defining the types in videodev2.h, but using a big union would 
require every event type to add a field to the union. That's the part that 
would become messy.

> An alternative in the long run might be to split off the event structs
> into a separate public header. I have been thinking along those lines as
> well for the controls. videodev2.h is getting pretty huge and it might be
> more managable if it is split up in multiple parts with videodev2.h
> including those parts.

Agreed.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v4 1/7] V4L: File handles

[Hans Verkuil]

On Wednesday 10 February 2010 15:58:03 Sakari Ailus wrote:
> This patch adds a list of v4l2_fh structures to every video_device.
> It allows using file handle related information in V4L2. The event interface
> is one example of such use.
> 
> Video device drivers should use the v4l2_fh pointer as their
> file->private_data.

See small review comment below.

Regards,

	Hans

> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/Makefile   |    3 +-
>  drivers/media/video/v4l2-dev.c |    2 +
>  drivers/media/video/v4l2-fh.c  |   65 ++++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h       |    6 ++++
>  include/media/v4l2-fh.h        |   47 +++++++++++++++++++++++++++++
>  5 files changed, 122 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-fh.c
>  create mode 100644 include/media/v4l2-fh.h
> 
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 6e75647..b888ad1 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -10,7 +10,8 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
>  
>  omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
>  
> -videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
> +			v4l2-fh.o
>  
>  # V4L2 core modules
>  
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 13a899d..c24c832 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -423,6 +423,8 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
>  	if (!vdev->release)
>  		return -EINVAL;
>  
> +	v4l2_fhs_init(vdev);
> +
>  	/* Part 1: check device type */
>  	switch (type) {
>  	case VFL_TYPE_GRABBER:
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> new file mode 100644
> index 0000000..3c1cea2
> --- /dev/null
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -0,0 +1,65 @@
> +/*
> + * drivers/media/video/v4l2-fh.c
> + *
> + * V4L2 file handles.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-fh.h>
> +
> +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> +{
> +	fh->vdev = vdev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_init);
> +
> +void v4l2_fh_add(struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	list_add(&fh->list, &fh->vdev->fh_list);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_add);
> +
> +void v4l2_fh_del(struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	list_del(&fh->list);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_del);
> +
> +void v4l2_fh_exit(struct v4l2_fh *fh)
> +{
> +	BUG_ON(fh->vdev == NULL);
> +	fh->vdev = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> +
> +void v4l2_fhs_init(struct video_device *vdev)
> +{
> +	spin_lock_init(&vdev->fh_lock);
> +	INIT_LIST_HEAD(&vdev->fh_list);
> +}

Move the contents of this function to __video_register_device. I do not see
any particular need to have a separate function here.

> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 26d4e79..ee3a0c9 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -18,6 +18,8 @@
>  
>  #include <media/media-entity.h>
>  
> +#include <media/v4l2-fh.h>
> +
>  #define VIDEO_MAJOR	81
>  
>  #define VFL_TYPE_GRABBER	0
> @@ -82,6 +84,10 @@ struct video_device
>  	/* attribute to differentiate multiple indices on one physical device */
>  	int index;
>  
> +	/* V4L2 file handles */
> +	spinlock_t		fh_lock; /* Lock for all v4l2_fhs */
> +	struct list_head	fh_list; /* List of struct v4l2_fh */
> +
>  	int debug;			/* Activates debug level*/
>  
>  	/* Video standard vars */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> new file mode 100644
> index 0000000..2e88031
> --- /dev/null
> +++ b/include/media/v4l2-fh.h
> @@ -0,0 +1,47 @@
> +/*
> + * include/media/v4l2-fh.h
> + *
> + * V4L2 file handle.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef V4L2_FH_H
> +#define V4L2_FH_H
> +
> +#include <linux/types.h>
> +#include <linux/list.h>
> +
> +#include <asm/atomic.h>
> +
> +struct video_device;
> +
> +struct v4l2_fh {
> +	struct list_head	list;
> +	struct video_device	*vdev;
> +};
> +
> +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> +void v4l2_fh_add(struct v4l2_fh *fh);
> +void v4l2_fh_del(struct v4l2_fh *fh);
> +void v4l2_fh_exit(struct v4l2_fh *fh);
> +
> +void v4l2_fhs_init(struct video_device *vdev);
> +
> +#endif /* V4L2_EVENT_H */
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG