[RFC v2 0/7] V4L2 file handles and event interface

[RFC v2 0/7] V4L2 file handles and event interface

[Sakari Ailus]

Hi,

Here's the second version of the V4L2 file handle and event interface 
patchset. Still RFC since I'd like to get more feedback on it.

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

The patchset works 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 events):

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

The major change since the last one v4l2_fh structure is now part of 
driver's own file handle. It's used as file->private_data as well. I did 
this based on Hans Verkuil's suggestion. Sequence numbers and event 
queue length limitation is there as well. There are countless of smaller 
changes, too.

A few notes on the patches:

- I don't like the locking too much. Perhaps the file handle specific 
lock (events->lock) could be dropped in favour of the lock for 
v4l2_file_handle in video_device?

- Poll. The V4L2 specifiction says:

	"When the application did not call VIDIOC_QBUF or
	VIDIOC_STREAMON yet the poll() function succeeds, but sets the
	POLLERR flag in the revents field."

   The current events for OMAP 3 ISP are related to streaming but not 
all might be in future. For example there might be some radio or DVB 
related events.

- Sequence numbers are local to file handles.

- Subscribing V4L2_EVENT_ALL causes any other events to be unsubscribed.

- If V4L2_EVENT_ALL has been subscribed, unsubscribing any one of the 
events leads to V4L2_EVENT_ALL to be unsubscribed. This problem would be 
difficult to work around since this would require the event system to be 
aware of the driver private events as well.

- If events are missed, the sequence number is incremented in any case. 
This way the user space knows events have been missed.

Comments would be very, very welcome.

Cheers,

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

[RFC v2 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   |    2 +-
 drivers/media/video/v4l2-dev.c |    2 +
 drivers/media/video/v4l2-fh.c  |   57 ++++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-dev.h       |    4 +++
 include/media/v4l2-fh.h        |   41 ++++++++++++++++++++++++++++
 5 files changed, 105 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 a61e3f3..1947146 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,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
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..15b2ac8 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -421,6 +421,8 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 	if (!vdev->release)
 		return -EINVAL;
 
+	v4l2_fh_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..406e4ac
--- /dev/null
+++ b/drivers/media/video/v4l2-fh.c
@@ -0,0 +1,57 @@
+/*
+ * 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>
+
+#include <linux/sched.h>
+#include <linux/vmalloc.h>
+
+int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vdev->fh_lock, flags);
+	list_add(&fh->list, &vdev->fh);
+	spin_unlock_irqrestore(&vdev->fh_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_add);
+
+void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vdev->fh_lock, flags);
+	list_del(&fh->list);
+	spin_unlock_irqrestore(&vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_del);
+
+void v4l2_fh_init(struct video_device *vdev)
+{
+	spin_lock_init(&vdev->fh_lock);
+	INIT_LIST_HEAD(&vdev->fh);
+}
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 2dee938..8eac93d 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -16,6 +16,8 @@
 #include <linux/mutex.h>
 #include <linux/videodev2.h>
 
+#include <media/v4l2-fh.h>
+
 #define VIDEO_MAJOR	81
 
 #define VFL_TYPE_GRABBER	0
@@ -77,6 +79,8 @@ struct video_device
 	/* attribute to differentiate multiple indices on one physical device */
 	int index;
 
+	spinlock_t fh_lock;		/* Lock for file handle list */
+	struct list_head fh;		/* File handle list */
 	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..1efa916
--- /dev/null
+++ b/include/media/v4l2-fh.h
@@ -0,0 +1,41 @@
+/*
+ * 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>
+
+struct v4l2_fh {
+	struct list_head	list;
+};
+
+struct video_device;
+
+int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh);
+void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh);
+void v4l2_fh_init(struct video_device *vdev);
+
+#endif /* V4L2_EVENT_H */
-- 
1.5.6.5

[RFC v2 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                 |   24 ++++++++++++++++++++++++
 3 files changed, 30 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..9ae9a1c 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1536,6 +1536,27 @@ 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_ALL				0
+#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 +1672,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

[RFC v2 3/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/v4l2-ioctl.c |   48 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ioctl.h       |    9 +++++++
 2 files changed, 57 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index bfc4696..6964bcc 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -1797,7 +1797,55 @@ static long __video_do_ioctl(struct file *file,
 		}
 		break;
 	}
+	case VIDIOC_DQEVENT:
+	{
+		struct v4l2_event *ev = arg;
+
+		if (!ops->vidioc_dqevent)
+			break;
+
+		ret = ops->vidioc_dqevent(file->private_data, 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(file->private_data, 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_unsubscribe_event)
+			break;
+
+		ret = ops->vidioc_unsubscribe_event(file->private_data, 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..29dd64b 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,13 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_enum_frameintervals) (struct file *file, void *fh,
 					   struct v4l2_frmivalenum *fival);
 
+	int (*vidioc_dqevent)	       (struct v4l2_fh *fh,
+					struct v4l2_event *ev);
+	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
+					struct v4l2_event_subscription *sub);
+	int (*vidioc_unsubscribe_event) (struct v4l2_fh *fh,
+					 struct v4l2_event_subscription *sub);
+
 	/* For other private ioctls */
 	long (*vidioc_default)	       (struct file *file, void *fh,
 					int cmd, void *arg);
-- 
1.5.6.5

[RFC v2 4/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/Makefile     |    3 +-
 drivers/media/video/v4l2-dev.c   |   21 +++-
 drivers/media/video/v4l2-event.c |  254 ++++++++++++++++++++++++++++++++++++++
 drivers/media/video/v4l2-fh.c    |    4 +
 include/media/v4l2-event.h       |   65 ++++++++++
 include/media/v4l2-fh.h          |    3 +
 6 files changed, 346 insertions(+), 4 deletions(-)
 create mode 100644 drivers/media/video/v4l2-event.c
 create mode 100644 include/media/v4l2-event.h

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 1947146..dd6a853 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-fh.o
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
+			v4l2-event.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 15b2ac8..6d25297 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -613,22 +613,36 @@ static int __init videodev_init(void)
 	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
 	int ret;
 
+	ret = v4l2_event_init();
+	if (ret < 0) {
+		printk(KERN_WARNING "videodev: unable to initialise events\n");
+		return ret;
+	}
+
 	printk(KERN_INFO "Linux video capture interface: v2.00\n");
 	ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
 	if (ret < 0) {
 		printk(KERN_WARNING "videodev: unable to get major %d\n",
 				VIDEO_MAJOR);
-		return ret;
+		goto out_register_chrdev_region;
 	}
 
 	ret = class_register(&video_class);
 	if (ret < 0) {
-		unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
 		printk(KERN_WARNING "video_dev: class_register failed\n");
-		return -EIO;
+		ret = -EIO;
+		goto out_class_register;
 	}
 
 	return 0;
+
+out_class_register:
+	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+
+out_register_chrdev_region:
+	v4l2_event_exit();
+
+	return ret;
 }
 
 static void __exit videodev_exit(void)
@@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
 
 	class_unregister(&video_class);
 	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+	v4l2_event_exit();
 }
 
 module_init(videodev_init)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 0000000..9fc0c81
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,254 @@
+/*
+ * 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>
+
+static struct kmem_cache *event_kmem;
+
+int v4l2_event_init(void)
+{
+	event_kmem = kmem_cache_create("event_kmem",
+				       sizeof(struct _v4l2_event), 0,
+				       SLAB_HWCACHE_ALIGN,
+				       NULL);
+
+	if (!event_kmem)
+		return -ENOMEM;
+
+	return 0;
+}
+
+void v4l2_event_exit(void)
+{
+	kmem_cache_destroy(event_kmem);
+}
+
+void v4l2_event_init_fh(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = &fh->events;
+
+	init_waitqueue_head(&events->wait);
+	spin_lock_init(&events->lock);
+
+	INIT_LIST_HEAD(&events->available);
+	INIT_LIST_HEAD(&events->subscribed);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
+
+void v4l2_event_exit_fh(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = &fh->events;
+
+	while (!list_empty(&events->available)) {
+		struct _v4l2_event *ev;
+
+		ev = list_entry(events->available.next,
+				struct _v4l2_event, list);
+
+		list_del(&ev->list);
+
+		kmem_cache_free(event_kmem, ev);
+	}
+
+	while (!list_empty(&events->subscribed)) {
+		struct v4l2_subscribed_event *sub;
+
+		sub = list_entry(events->subscribed.next,
+				struct v4l2_subscribed_event, list);
+
+		list_del(&sub->list);
+
+		kfree(sub);
+	}
+}
+EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
+
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
+{
+	struct v4l2_events *events = &fh->events;
+	struct _v4l2_event *ev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&events->lock, flags);
+
+	if (list_empty(&events->available)) {
+		spin_unlock_irqrestore(&events->lock, flags);
+		return -ENOENT;
+	}
+
+	ev = list_first_entry(&events->available, struct _v4l2_event, list);
+	list_del(&ev->list);
+
+	ev->event.count = !list_empty(&events->available);
+
+	spin_unlock_irqrestore(&events->lock, flags);
+
+	memcpy(event, &ev->event, sizeof(ev->event));
+
+	kmem_cache_free(event_kmem, ev);
+
+	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 *ev;
+
+	list_for_each_entry(ev, &events->subscribed, list) {
+		if (ev->type == type)
+			return ev;
+	}
+
+	return NULL;
+}
+
+struct v4l2_subscribed_event *v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type)
+{
+	struct v4l2_events *events = &fh->events;
+	struct v4l2_subscribed_event *ev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&events->lock, flags);
+
+	ev = __v4l2_event_subscribed(fh, type);
+
+	spin_unlock_irqrestore(&events->lock, flags);
+
+	return ev;
+}
+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;
+
+	spin_lock_irqsave(&vdev->fh_lock, flags);
+
+	list_for_each_entry(fh, &vdev->fh, list) {
+		struct _v4l2_event *_ev;
+
+		if (!v4l2_event_subscribed(fh, ev->type))
+			continue;
+
+		_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
+		if (!_ev)
+			continue;
+
+		_ev->event = *ev;
+
+		spin_lock(&fh->events.lock);
+		list_add_tail(&_ev->list, &fh->events.available);
+		spin_unlock(&fh->events.lock);
+
+		wake_up_all(&fh->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(&events->lock, flags);
+	ret = !list_empty(&events->available);
+	spin_unlock_irqrestore(&events->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 *ev;
+	unsigned long flags;
+	int ret = 0;
+
+	/* Allow subscribing to valid events only. */
+	if (sub->type < V4L2_EVENT_PRIVATE_START)
+		switch (sub->type) {
+		case V4L2_EVENT_ALL:
+			break;
+		default:
+			return -EINVAL;
+		}
+
+	ev = kmalloc(sizeof(*ev), GFP_KERNEL);
+	if (!ev)
+		return -ENOMEM;
+
+	spin_lock_irqsave(&events->lock, flags);
+
+	if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&ev->list);
+	ev->type = sub->type;
+
+	list_add(&ev->list, &events->subscribed);
+
+out:
+	spin_unlock_irqrestore(&events->lock, flags);
+
+	if (ret)
+		kfree(ev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub)
+{
+	struct v4l2_events *events = &fh->events;
+	struct v4l2_subscribed_event *ev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&events->lock, flags);
+
+	ev = __v4l2_event_subscribed(fh, sub->type);
+
+	if (ev != NULL)
+		list_del(&ev->list);
+
+	spin_unlock_irqrestore(&events->lock, flags);
+
+	return ev == NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 406e4ac..2b25ec9 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -32,6 +32,8 @@ int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
 {
 	unsigned long flags;
 
+	v4l2_event_init_fh(fh);
+
 	spin_lock_irqsave(&vdev->fh_lock, flags);
 	list_add(&fh->list, &vdev->fh);
 	spin_unlock_irqrestore(&vdev->fh_lock, flags);
@@ -44,6 +46,8 @@ void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
 {
 	unsigned long flags;
 
+	v4l2_event_exit_fh(fh);
+
 	spin_lock_irqsave(&vdev->fh_lock, flags);
 	list_del(&fh->list);
 	spin_unlock_irqrestore(&vdev->fh_lock, flags);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
new file mode 100644
index 0000000..b11de92
--- /dev/null
+++ b/include/media/v4l2-event.h
@@ -0,0 +1,65 @@
+/*
+ * 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_event {
+	struct list_head	list;
+	struct v4l2_event	event;
+};
+
+struct v4l2_events {
+	spinlock_t		lock; /* Protect everything here. */
+	struct list_head	available;
+	wait_queue_head_t	wait;
+	struct list_head	subscribed; /* Subscribed events. */
+};
+
+struct v4l2_subscribed_event {
+	struct list_head	list;
+	u32			type;
+};
+
+int v4l2_event_init(void);
+void v4l2_event_exit(void);
+void v4l2_event_init_fh(struct v4l2_fh *fh);
+void v4l2_event_exit_fh(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 1efa916..c15bd13 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -28,8 +28,11 @@
 #include <linux/types.h>
 #include <linux/list.h>
 
+#include <media/v4l2-event.h>
+
 struct v4l2_fh {
 	struct list_head	list;
+	struct v4l2_events      events; /* events, pending and subscribed */
 };
 
 struct video_device;
-- 
1.5.6.5

[RFC v2 5/7] V4L: Events: Limit event queue length

[Sakari Ailus]

Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
further events will be dropped.

This patch also updates the count field properly, 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 |   10 +++++++++-
 include/media/v4l2-event.h       |    5 +++++
 2 files changed, 14 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9fc0c81..72fdf7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
 
 	INIT_LIST_HEAD(&events->available);
 	INIT_LIST_HEAD(&events->subscribed);
+
+	atomic_set(&events->navailable, 0);
 }
 EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
 
@@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
 	ev = list_first_entry(&events->available, struct _v4l2_event, list);
 	list_del(&ev->list);
 
-	ev->event.count = !list_empty(&events->available);
+	atomic_dec(&events->navailable);
+	ev->event.count = atomic_read(&events->navailable);
 
 	spin_unlock_irqrestore(&events->lock, flags);
 
@@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		if (!v4l2_event_subscribed(fh, ev->type))
 			continue;
 
+		if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
+			continue;
+
 		_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
 		if (!_ev)
 			continue;
@@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		list_add_tail(&_ev->list, &fh->events.available);
 		spin_unlock(&fh->events.lock);
 
+		atomic_inc(&fh->events.navailable);
+
 		wake_up_all(&fh->events.wait);
 	}
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index b11de92..69305c6 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,10 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
+#include <asm/atomic.h>
+
+#define V4L2_MAX_EVENTS		1024 /* Ought to be enough for everyone. */
+
 struct v4l2_fh;
 struct video_device;
 
@@ -39,6 +43,7 @@ struct _v4l2_event {
 struct v4l2_events {
 	spinlock_t		lock; /* Protect everything here. */
 	struct list_head	available;
+	atomic_t		navailable;
 	wait_queue_head_t	wait;
 	struct list_head	subscribed; /* Subscribed events. */
 };
-- 
1.5.6.5

[RFC v2 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 |    8 ++++++++
 include/media/v4l2-event.h       |    1 +
 2 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 72fdf7f..cc2bf57 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -58,6 +58,7 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
 	INIT_LIST_HEAD(&events->subscribed);
 
 	atomic_set(&events->navailable, 0);
+	events->sequence = 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
 
@@ -158,10 +159,16 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 
 	list_for_each_entry(fh, &vdev->fh, list) {
 		struct _v4l2_event *_ev;
+		u32 sequence;
 
 		if (!v4l2_event_subscribed(fh, ev->type))
 			continue;
 
+		spin_lock(&fh->events.lock);
+		sequence = fh->events.sequence;
+		fh->events.sequence++;
+		spin_unlock(&fh->events.lock);
+
 		if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
 			continue;
 
@@ -172,6 +179,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		_ev->event = *ev;
 
 		spin_lock(&fh->events.lock);
+		_ev->event.sequence = sequence;
 		list_add_tail(&_ev->list, &fh->events.available);
 		spin_unlock(&fh->events.lock);
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 69305c6..5a778d4 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -44,6 +44,7 @@ struct v4l2_events {
 	spinlock_t		lock; /* Protect everything here. */
 	struct list_head	available;
 	atomic_t		navailable;
+	u32			sequence;
 	wait_queue_head_t	wait;
 	struct list_head	subscribed; /* Subscribed events. */
 };
-- 
1.5.6.5

[RFC v2 7/7] V4L: Events: Support all events

[Sakari Ailus]

Add support for subscribing V4L2_EVENT_ALL. After V4L2_EVENT_ALL is
subscribed, unsubscribing any event leads to unsubscription of all events.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-event.c |   34 ++++++++++++++++++++++++----------
 1 files changed, 24 insertions(+), 10 deletions(-)

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index cc2bf57..95b3917 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -62,6 +62,22 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
 }
 EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
 
+static void __v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = &fh->events;
+
+	while (!list_empty(&events->subscribed)) {
+		struct v4l2_subscribed_event *sub;
+
+		sub = list_entry(events->subscribed.next,
+				struct v4l2_subscribed_event, list);
+
+		list_del(&sub->list);
+
+		kfree(sub);
+	}
+}
+
 void v4l2_event_exit_fh(struct v4l2_fh *fh)
 {
 	struct v4l2_events *events = &fh->events;
@@ -77,16 +93,7 @@ void v4l2_event_exit_fh(struct v4l2_fh *fh)
 		kmem_cache_free(event_kmem, ev);
 	}
 
-	while (!list_empty(&events->subscribed)) {
-		struct v4l2_subscribed_event *sub;
-
-		sub = list_entry(events->subscribed.next,
-				struct v4l2_subscribed_event, list);
-
-		list_del(&sub->list);
-
-		kfree(sub);
-	}
+	__v4l2_event_unsubscribe_all(fh);
 }
 EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
 
@@ -125,6 +132,11 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed(
 	struct v4l2_events *events = &fh->events;
 	struct v4l2_subscribed_event *ev;
 
+	ev = container_of(events->subscribed.next,
+			  struct v4l2_subscribed_event, list);
+	if (ev->type == V4L2_EVENT_ALL)
+		return ev;
+
 	list_for_each_entry(ev, &events->subscribed, list) {
 		if (ev->type == type)
 			return ev;
@@ -237,6 +249,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
 	INIT_LIST_HEAD(&ev->list);
 	ev->type = sub->type;
 
+	if (ev->type == V4L2_EVENT_ALL)
+		__v4l2_event_unsubscribe_all(fh);
 	list_add(&ev->list, &events->subscribed);
 
 out:
-- 
1.5.6.5

Re: [RFC v2 4/7] V4L: Events: Add backend

[Andy Walls]

On Tue, 2009-12-22 at 18:43 +0200, 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.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/Makefile     |    3 +-
>  drivers/media/video/v4l2-dev.c   |   21 +++-
>  drivers/media/video/v4l2-event.c |  254 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-fh.c    |    4 +
>  include/media/v4l2-event.h       |   65 ++++++++++
>  include/media/v4l2-fh.h          |    3 +
>  6 files changed, 346 insertions(+), 4 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-event.c
>  create mode 100644 include/media/v4l2-event.h
> 
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 1947146..dd6a853 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-fh.o
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> +			v4l2-event.o
>  
>  # V4L2 core modules
>  
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 15b2ac8..6d25297 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -613,22 +613,36 @@ static int __init videodev_init(void)
>  	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
>  	int ret;
>  
> +	ret = v4l2_event_init();
> +	if (ret < 0) {
> +		printk(KERN_WARNING "videodev: unable to initialise events\n");
> +		return ret;
> +	}
> +
>  	printk(KERN_INFO "Linux video capture interface: v2.00\n");
>  	ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
>  	if (ret < 0) {
>  		printk(KERN_WARNING "videodev: unable to get major %d\n",
>  				VIDEO_MAJOR);
> -		return ret;
> +		goto out_register_chrdev_region;
>  	}
>  
>  	ret = class_register(&video_class);
>  	if (ret < 0) {
> -		unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
>  		printk(KERN_WARNING "video_dev: class_register failed\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto out_class_register;
>  	}
>  
>  	return 0;
> +
> +out_class_register:
> +	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +
> +out_register_chrdev_region:
> +	v4l2_event_exit();
> +
> +	return ret;
>  }
>  
>  static void __exit videodev_exit(void)
> @@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
>  
>  	class_unregister(&video_class);
>  	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +	v4l2_event_exit();
>  }
>  
>  module_init(videodev_init)
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..9fc0c81
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,254 @@
> +/*
> + * 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>
> +
> +static struct kmem_cache *event_kmem;
> +
> +int v4l2_event_init(void)
> +{
> +	event_kmem = kmem_cache_create("event_kmem",
> +				       sizeof(struct _v4l2_event), 0,
> +				       SLAB_HWCACHE_ALIGN,
> +				       NULL);
> +
> +	if (!event_kmem)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void v4l2_event_exit(void)
> +{
> +	kmem_cache_destroy(event_kmem);
> +}
> +
> +void v4l2_event_init_fh(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = &fh->events;
> +
> +	init_waitqueue_head(&events->wait);
> +	spin_lock_init(&events->lock);
> +
> +	INIT_LIST_HEAD(&events->available);
> +	INIT_LIST_HEAD(&events->subscribed);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
> +
> +void v4l2_event_exit_fh(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = &fh->events;
> +
> +	while (!list_empty(&events->available)) {
> +		struct _v4l2_event *ev;
> +
> +		ev = list_entry(events->available.next,
> +				struct _v4l2_event, list);
> +
> +		list_del(&ev->list);
> +
> +		kmem_cache_free(event_kmem, ev);
> +	}
> +
> +	while (!list_empty(&events->subscribed)) {
> +		struct v4l2_subscribed_event *sub;
> +
> +		sub = list_entry(events->subscribed.next,
> +				struct v4l2_subscribed_event, list);
> +
> +		list_del(&sub->list);
> +
> +		kfree(sub);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct _v4l2_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&events->lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	ev = list_first_entry(&events->available, struct _v4l2_event, list);
> +	list_del(&ev->list);
> +
> +	ev->event.count = !list_empty(&events->available);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	memcpy(event, &ev->event, sizeof(ev->event));
> +
> +	kmem_cache_free(event_kmem, ev);
> +
> +	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 *ev;
> +
> +	list_for_each_entry(ev, &events->subscribed, list) {
> +		if (ev->type == type)
> +			return ev;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct v4l2_subscribed_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	ev = __v4l2_event_subscribed(fh, type);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	return ev;
> +}
> +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;
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fh, list) {
> +		struct _v4l2_event *_ev;
> +
> +		if (!v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> +		if (!_ev)
> +			continue;
> +
> +		_ev->event = *ev;
> +
> +		spin_lock(&fh->events.lock);
> +		list_add_tail(&_ev->list, &fh->events.available);
> +		spin_unlock(&fh->events.lock);
> +
> +		wake_up_all(&fh->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(&events->lock, flags);
> +	ret = !list_empty(&events->available);
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_pending);

Hi Sakari,

Disabling and restoring local interrupts to check if any events are
pending seems excessive.

Since you added an atomic_t with the number of events available in patch
5/7, why don't you just check that atomic_t here?


Regards,
Andy

> +
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct v4l2_subscribed_event *ev;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* Allow subscribing to valid events only. */
> +	if (sub->type < V4L2_EVENT_PRIVATE_START)
> +		switch (sub->type) {
> +		case V4L2_EVENT_ALL:
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> +	if (!ev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&ev->list);
> +	ev->type = sub->type;
> +
> +	list_add(&ev->list, &events->subscribed);
> +
> +out:
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	if (ret)
> +		kfree(ev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct v4l2_subscribed_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	ev = __v4l2_event_subscribed(fh, sub->type);
> +
> +	if (ev != NULL)
> +		list_del(&ev->list);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	return ev == NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 406e4ac..2b25ec9 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -32,6 +32,8 @@ int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
>  {
>  	unsigned long flags;
>  
> +	v4l2_event_init_fh(fh);
> +
>  	spin_lock_irqsave(&vdev->fh_lock, flags);
>  	list_add(&fh->list, &vdev->fh);
>  	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> @@ -44,6 +46,8 @@ void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
>  {
>  	unsigned long flags;
>  
> +	v4l2_event_exit_fh(fh);
> +
>  	spin_lock_irqsave(&vdev->fh_lock, flags);
>  	list_del(&fh->list);
>  	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..b11de92
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,65 @@
> +/*
> + * 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_event {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_events {
> +	spinlock_t		lock; /* Protect everything here. */
> +	struct list_head	available;
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events. */
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +int v4l2_event_init(void);
> +void v4l2_event_exit(void);
> +void v4l2_event_init_fh(struct v4l2_fh *fh);
> +void v4l2_event_exit_fh(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 1efa916..c15bd13 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,8 +28,11 @@
>  #include <linux/types.h>
>  #include <linux/list.h>
>  
> +#include <media/v4l2-event.h>
> +
>  struct v4l2_fh {
>  	struct list_head	list;
> +	struct v4l2_events      events; /* events, pending and subscribed */
>  };
>  
>  struct video_device;

Re: [RFC v2 4/7] V4L: Events: Add backend

[Sakari Ailus]

Hi Andy,

Andy Walls wrote:
>> +int v4l2_event_pending(struct v4l2_fh *fh)
>> +{
>> +	struct v4l2_events *events =&fh->events;
>> +	unsigned long flags;
>> +	int ret;
>> +
>> +	spin_lock_irqsave(&events->lock, flags);
>> +	ret = !list_empty(&events->available);
>> +	spin_unlock_irqrestore(&events->lock, flags);
>> +
>> +	return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> Hi Sakari,
>
> Disabling and restoring local interrupts to check if any events are
> pending seems excessive.
>
> Since you added an atomic_t with the number of events available in patch
> 5/7, why don't you just check that atomic_t here?

Thanks for the comments!

I'll put the fix to patch 5.

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

Re: [RFC v2 1/7] V4L: File handles

[Hans Verkuil]

Hi Sakari,

I should have reviewed this weeks ago, but better late than never...

On Tue, 22 Dec 2009, 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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/Makefile   |    2 +-
> drivers/media/video/v4l2-dev.c |    2 +
> drivers/media/video/v4l2-fh.c  |   57 ++++++++++++++++++++++++++++++++++++++++
> include/media/v4l2-dev.h       |    4 +++
> include/media/v4l2-fh.h        |   41 ++++++++++++++++++++++++++++
> 5 files changed, 105 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 a61e3f3..1947146 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -10,7 +10,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
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 7090699..15b2ac8 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -421,6 +421,8 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
> 	if (!vdev->release)
> 		return -EINVAL;
>
> +	v4l2_fh_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..406e4ac
> --- /dev/null
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -0,0 +1,57 @@
> +/*
> + * 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>
> +
> +#include <linux/sched.h>
> +#include <linux/vmalloc.h>

Weird includes. I would expect to see only spinlock.h and list.h to be included
here.

Regards,

         Hans

> +
> +int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +	list_add(&fh->list, &vdev->fh);
> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_add);
> +
> +void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +	list_del(&fh->list);
> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_del);
> +
> +void v4l2_fh_init(struct video_device *vdev)
> +{
> +	spin_lock_init(&vdev->fh_lock);
> +	INIT_LIST_HEAD(&vdev->fh);
> +}
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 2dee938..8eac93d 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -16,6 +16,8 @@
> #include <linux/mutex.h>
> #include <linux/videodev2.h>
>
> +#include <media/v4l2-fh.h>
> +
> #define VIDEO_MAJOR	81
>
> #define VFL_TYPE_GRABBER	0
> @@ -77,6 +79,8 @@ struct video_device
> 	/* attribute to differentiate multiple indices on one physical device */
> 	int index;
>
> +	spinlock_t fh_lock;		/* Lock for file handle list */
> +	struct list_head fh;		/* File handle list */
> 	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..1efa916
> --- /dev/null
> +++ b/include/media/v4l2-fh.h
> @@ -0,0 +1,41 @@
> +/*
> + * 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>
> +
> +struct v4l2_fh {
> +	struct list_head	list;
> +};
> +
> +struct video_device;
> +
> +int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh);
> +void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh);
> +void v4l2_fh_init(struct video_device *vdev);
> +
> +#endif /* V4L2_EVENT_H */
> -- 
> 1.5.6.5
>

Re: [RFC v2 4/7] V4L: Events: Add backend

[Hans Verkuil]

Hi Sakari,

The code looks good, but I'm not so sure about the use of kmem_cache_*. It
seems serious overkill to me.

Most of the time there will only be a handful of event messages pending. So
setting up a kmem_cache for this seems unnecessary to me.

A much better way to ensure fast event reporting IMHO would be to keep a list
of empty event messages rather than destroying an event after it is dequeued.

So you have two queues per filehandle: pending and empty; initially both are
empty. When a new event has to be queued the code checks if there are events
available for reuse in the empty queue, and if not a new event struct is
allocated and added to the pending queue.

When userspace dequeues the event the kernel will not destroy the event struct
but requeue it in the empty queue for reuse. This is also an ideal place to
check for queue overflowing (which is definitely needed).

In general allocating memory is a slow operation, so not freeing that memory
will speed up things. Drivers might even preallocate a certain number of events
in the empty queue if the last drop of performance is needed.

But right now I don't think there is any need for that.

Regards,

           Hans

On Tue, 22 Dec 2009, 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.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/Makefile     |    3 +-
> drivers/media/video/v4l2-dev.c   |   21 +++-
> drivers/media/video/v4l2-event.c |  254 ++++++++++++++++++++++++++++++++++++++
> drivers/media/video/v4l2-fh.c    |    4 +
> include/media/v4l2-event.h       |   65 ++++++++++
> include/media/v4l2-fh.h          |    3 +
> 6 files changed, 346 insertions(+), 4 deletions(-)
> create mode 100644 drivers/media/video/v4l2-event.c
> create mode 100644 include/media/v4l2-event.h
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 1947146..dd6a853 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-fh.o
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> +			v4l2-event.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 15b2ac8..6d25297 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -613,22 +613,36 @@ static int __init videodev_init(void)
> 	dev_t dev = MKDEV(VIDEO_MAJOR, 0);
> 	int ret;
>
> +	ret = v4l2_event_init();
> +	if (ret < 0) {
> +		printk(KERN_WARNING "videodev: unable to initialise events\n");
> +		return ret;
> +	}
> +
> 	printk(KERN_INFO "Linux video capture interface: v2.00\n");
> 	ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
> 	if (ret < 0) {
> 		printk(KERN_WARNING "videodev: unable to get major %d\n",
> 				VIDEO_MAJOR);
> -		return ret;
> +		goto out_register_chrdev_region;
> 	}
>
> 	ret = class_register(&video_class);
> 	if (ret < 0) {
> -		unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> 		printk(KERN_WARNING "video_dev: class_register failed\n");
> -		return -EIO;
> +		ret = -EIO;
> +		goto out_class_register;
> 	}
>
> 	return 0;
> +
> +out_class_register:
> +	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +
> +out_register_chrdev_region:
> +	v4l2_event_exit();
> +
> +	return ret;
> }
>
> static void __exit videodev_exit(void)
> @@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
>
> 	class_unregister(&video_class);
> 	unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +	v4l2_event_exit();
> }
>
> module_init(videodev_init)
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..9fc0c81
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,254 @@
> +/*
> + * 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>
> +
> +static struct kmem_cache *event_kmem;
> +
> +int v4l2_event_init(void)
> +{
> +	event_kmem = kmem_cache_create("event_kmem",
> +				       sizeof(struct _v4l2_event), 0,
> +				       SLAB_HWCACHE_ALIGN,
> +				       NULL);
> +
> +	if (!event_kmem)
> +		return -ENOMEM;
> +
> +	return 0;
> +}
> +
> +void v4l2_event_exit(void)
> +{
> +	kmem_cache_destroy(event_kmem);
> +}
> +
> +void v4l2_event_init_fh(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = &fh->events;
> +
> +	init_waitqueue_head(&events->wait);
> +	spin_lock_init(&events->lock);
> +
> +	INIT_LIST_HEAD(&events->available);
> +	INIT_LIST_HEAD(&events->subscribed);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
> +
> +void v4l2_event_exit_fh(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = &fh->events;
> +
> +	while (!list_empty(&events->available)) {
> +		struct _v4l2_event *ev;
> +
> +		ev = list_entry(events->available.next,
> +				struct _v4l2_event, list);
> +
> +		list_del(&ev->list);
> +
> +		kmem_cache_free(event_kmem, ev);
> +	}
> +
> +	while (!list_empty(&events->subscribed)) {
> +		struct v4l2_subscribed_event *sub;
> +
> +		sub = list_entry(events->subscribed.next,
> +				struct v4l2_subscribed_event, list);
> +
> +		list_del(&sub->list);
> +
> +		kfree(sub);
> +	}
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct _v4l2_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&events->lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	ev = list_first_entry(&events->available, struct _v4l2_event, list);
> +	list_del(&ev->list);
> +
> +	ev->event.count = !list_empty(&events->available);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	memcpy(event, &ev->event, sizeof(ev->event));
> +
> +	kmem_cache_free(event_kmem, ev);
> +
> +	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 *ev;
> +
> +	list_for_each_entry(ev, &events->subscribed, list) {
> +		if (ev->type == type)
> +			return ev;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct v4l2_subscribed_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	ev = __v4l2_event_subscribed(fh, type);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	return ev;
> +}
> +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;
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fh, list) {
> +		struct _v4l2_event *_ev;
> +
> +		if (!v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> +		if (!_ev)
> +			continue;
> +
> +		_ev->event = *ev;
> +
> +		spin_lock(&fh->events.lock);
> +		list_add_tail(&_ev->list, &fh->events.available);
> +		spin_unlock(&fh->events.lock);
> +
> +		wake_up_all(&fh->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(&events->lock, flags);
> +	ret = !list_empty(&events->available);
> +	spin_unlock_irqrestore(&events->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 *ev;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* Allow subscribing to valid events only. */
> +	if (sub->type < V4L2_EVENT_PRIVATE_START)
> +		switch (sub->type) {
> +		case V4L2_EVENT_ALL:
> +			break;
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> +	if (!ev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&ev->list);
> +	ev->type = sub->type;
> +
> +	list_add(&ev->list, &events->subscribed);
> +
> +out:
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	if (ret)
> +		kfree(ev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_events *events = &fh->events;
> +	struct v4l2_subscribed_event *ev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&events->lock, flags);
> +
> +	ev = __v4l2_event_subscribed(fh, sub->type);
> +
> +	if (ev != NULL)
> +		list_del(&ev->list);
> +
> +	spin_unlock_irqrestore(&events->lock, flags);
> +
> +	return ev == NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 406e4ac..2b25ec9 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -32,6 +32,8 @@ int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
> {
> 	unsigned long flags;
>
> +	v4l2_event_init_fh(fh);
> +
> 	spin_lock_irqsave(&vdev->fh_lock, flags);
> 	list_add(&fh->list, &vdev->fh);
> 	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> @@ -44,6 +46,8 @@ void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
> {
> 	unsigned long flags;
>
> +	v4l2_event_exit_fh(fh);
> +
> 	spin_lock_irqsave(&vdev->fh_lock, flags);
> 	list_del(&fh->list);
> 	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..b11de92
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,65 @@
> +/*
> + * 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_event {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_events {
> +	spinlock_t		lock; /* Protect everything here. */
> +	struct list_head	available;
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events. */
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +int v4l2_event_init(void);
> +void v4l2_event_exit(void);
> +void v4l2_event_init_fh(struct v4l2_fh *fh);
> +void v4l2_event_exit_fh(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 1efa916..c15bd13 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,8 +28,11 @@
> #include <linux/types.h>
> #include <linux/list.h>
>
> +#include <media/v4l2-event.h>
> +
> struct v4l2_fh {
> 	struct list_head	list;
> +	struct v4l2_events      events; /* events, pending and subscribed */
> };
>
> struct video_device;
> -- 
> 1.5.6.5
>

Re: [RFC v2 6/7] V4L: Events: Sequence numbers

[Hans Verkuil]

Hi Sakari,

Some more review comments:

On Tue, 22 Dec 2009, Sakari Ailus wrote:

> Add sequence numbers to events.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/v4l2-event.c |    8 ++++++++
> include/media/v4l2-event.h       |    1 +
> 2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 72fdf7f..cc2bf57 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -58,6 +58,7 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
> 	INIT_LIST_HEAD(&events->subscribed);
>
> 	atomic_set(&events->navailable, 0);
> +	events->sequence = 0;

Why not make this atomic_t as well?

> }
> EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
>
> @@ -158,10 +159,16 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
>
> 	list_for_each_entry(fh, &vdev->fh, list) {
> 		struct _v4l2_event *_ev;
> +		u32 sequence;
>
> 		if (!v4l2_event_subscribed(fh, ev->type))
> 			continue;
>
> +		spin_lock(&fh->events.lock);
> +		sequence = fh->events.sequence;
> +		fh->events.sequence++;
> +		spin_unlock(&fh->events.lock);

Then you can use atomic_inc_return() here.

> +
> 		if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
> 			continue;
>
> @@ -172,6 +179,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> 		_ev->event = *ev;
>
> 		spin_lock(&fh->events.lock);
> +		_ev->event.sequence = sequence;
> 		list_add_tail(&_ev->list, &fh->events.available);
> 		spin_unlock(&fh->events.lock);
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 69305c6..5a778d4 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -44,6 +44,7 @@ struct v4l2_events {
> 	spinlock_t		lock; /* Protect everything here. */
> 	struct list_head	available;
> 	atomic_t		navailable;
> +	u32			sequence;
> 	wait_queue_head_t	wait;
> 	struct list_head	subscribed; /* Subscribed events. */
> };
> -- 
> 1.5.6.5
>

Regards,

 	Hans

Re: [RFC v2 5/7] V4L: Events: Limit event queue length

[Hans Verkuil]

Hi Sakari,

More comments:

On Tue, 22 Dec 2009, Sakari Ailus wrote:

> Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
> further events will be dropped.
>
> This patch also updates the count field properly, 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 |   10 +++++++++-
> include/media/v4l2-event.h       |    5 +++++
> 2 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 9fc0c81..72fdf7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
>
> 	INIT_LIST_HEAD(&events->available);
> 	INIT_LIST_HEAD(&events->subscribed);
> +
> +	atomic_set(&events->navailable, 0);
> }
> EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
>
> @@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> 	ev = list_first_entry(&events->available, struct _v4l2_event, list);
> 	list_del(&ev->list);
>
> -	ev->event.count = !list_empty(&events->available);
> +	atomic_dec(&events->navailable);
> +	ev->event.count = atomic_read(&events->navailable);

Combine these two lines to atomic_dec_return().

>
> 	spin_unlock_irqrestore(&events->lock, flags);
>
> @@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> 		if (!v4l2_event_subscribed(fh, ev->type))
> 			continue;
>
> +		if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
> +			continue;
> +
> 		_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> 		if (!_ev)
> 			continue;
> @@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> 		list_add_tail(&_ev->list, &fh->events.available);
> 		spin_unlock(&fh->events.lock);
>
> +		atomic_inc(&fh->events.navailable);
> +
> 		wake_up_all(&fh->events.wait);
> 	}
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index b11de92..69305c6 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -28,6 +28,10 @@
> #include <linux/types.h>
> #include <linux/videodev2.h>
>
> +#include <asm/atomic.h>
> +
> +#define V4L2_MAX_EVENTS		1024 /* Ought to be enough for everyone. */

I think this should be programmable by the driver. Most drivers do not use
events at all, so by default it should be 0 or perhaps it can check whether
the ioctl callback structure contains the event ioctls and set it to 0 or
some initial default value.

And you want this to be controlled on a per-filehandle basis even. If I look
at ivtv, then most of the device nodes will not have events, only a few will
support events. And for one device node type I know that there will only be
a single event when stopping the streaming, while another device node type
will get an event each frame.

So being able to adjust the event queue dynamically will give more control
and prevent unnecessary waste of memory resources.

Regards,

 	Hans

> +
> struct v4l2_fh;
> struct video_device;
>
> @@ -39,6 +43,7 @@ struct _v4l2_event {
> struct v4l2_events {
> 	spinlock_t		lock; /* Protect everything here. */
> 	struct list_head	available;
> +	atomic_t		navailable;
> 	wait_queue_head_t	wait;
> 	struct list_head	subscribed; /* Subscribed events. */
> };
> -- 
> 1.5.6.5
>

Re: [RFC v2 0/7] V4L2 file handles and event interface

[Hans Verkuil]

Hi Sakari,

Some notes on this as well (and that concludes my review):

On Tue, 22 Dec 2009, Sakari Ailus wrote:

> Hi,
>
> Here's the second version of the V4L2 file handle and event interface 
> patchset. Still RFC since I'd like to get more feedback on it.
>
> The first patch adds the V4L2 file handle support and the rest are for V4L2 
> events.
>
> The patchset works 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 
> events):
>
> 	git://gitorious.org/omap3camera/mainline.git event
>
> The major change since the last one v4l2_fh structure is now part of driver's 
> own file handle. It's used as file->private_data as well. I did this based on 
> Hans Verkuil's suggestion. Sequence numbers and event queue length limitation 
> is there as well. There are countless of smaller changes, too.
>
> A few notes on the patches:
>
> - I don't like the locking too much. Perhaps the file handle specific lock 
> (events->lock) could be dropped in favour of the lock for v4l2_file_handle in 
> video_device?
>
> - Poll. The V4L2 specifiction says:
>
> 	"When the application did not call VIDIOC_QBUF or
> 	VIDIOC_STREAMON yet the poll() function succeeds, but sets the
> 	POLLERR flag in the revents field."
>
>  The current events for OMAP 3 ISP are related to streaming but not all 
> might be in future. For example there might be some radio or DVB related 
> events.

I know for sure that we will have to be able to handle events when not
streaming. E.g. events that tell when a HDMI connector was plugged in
or when the EDID was read will need to arrive whether streaming is on
or off.

> - Sequence numbers are local to file handles.

That is how it should be.

> - Subscribing V4L2_EVENT_ALL causes any other events to be unsubscribed.
>
> - If V4L2_EVENT_ALL has been subscribed, unsubscribing any one of the events 
> leads to V4L2_EVENT_ALL to be unsubscribed. This problem would be difficult 
> to work around since this would require the event system to be aware of the 
> driver private events as well.

Good point. Perhaps attempting to unsubscribe a single event when EVENT_ALL
has been subscribed should result in an error? I.e., you can only unsubscribe
ALL when you subscribed to ALL in the first place.

Regards,

 	Hans

> - If events are missed, the sequence number is incremented in any case. This 
> way the user space knows events have been missed.
>
> Comments would be very, very welcome.
>
> Cheers,
>
> -- 
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com
>

Re: [RFC v2 5/7] V4L: Events: Limit event queue length

[Laurent Pinchart]

Hi Hans,

On Monday 18 January 2010 13:58:09 Hans Verkuil wrote:
> On Tue, 22 Dec 2009, Sakari Ailus wrote:
> > Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
> > further events will be dropped.
> >
> > This patch also updates the count field properly, 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 |   10 +++++++++-
> > include/media/v4l2-event.h       |    5 +++++
> > 2 files changed, 14 insertions(+), 1 deletions(-)

[snip]

> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index b11de92..69305c6 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -28,6 +28,10 @@
> > #include <linux/types.h>
> > #include <linux/videodev2.h>
> >
> > +#include <asm/atomic.h>
> > +
> > +#define V4L2_MAX_EVENTS		1024 /* Ought to be enough for everyone. */
> 
> I think this should be programmable by the driver. Most drivers do not use
> events at all, so by default it should be 0 or perhaps it can check whether
> the ioctl callback structure contains the event ioctls and set it to 0 or
> some initial default value.
> 
> And you want this to be controlled on a per-filehandle basis even. If I
>  look at ivtv, then most of the device nodes will not have events, only a
>  few will support events. And for one device node type I know that there
>  will only be a single event when stopping the streaming, while another
>  device node type will get an event each frame.

Don't you mean per video node instead of per file handle ? In that case we 
could add a new field to video_device structure that must be initialized by 
drivers before registering the device.

> So being able to adjust the event queue dynamically will give more control
> and prevent unnecessary waste of memory resources.

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 0/7] V4L2 file handles and event interface

[Laurent Pinchart]

Hi Hans,

On Monday 18 January 2010 14:07:33 Hans Verkuil wrote:
> On Tue, 22 Dec 2009, Sakari Ailus wrote:
> > Hi,
> >
> > Here's the second version of the V4L2 file handle and event interface
> > patchset. Still RFC since I'd like to get more feedback on it.
> >
> > The first patch adds the V4L2 file handle support and the rest are for
> > V4L2 events.
> >
> > The patchset works 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 events):
> >
> > 	git://gitorious.org/omap3camera/mainline.git event
> >
> > The major change since the last one v4l2_fh structure is now part of
> > driver's own file handle. It's used as file->private_data as well. I did
> > this based on Hans Verkuil's suggestion. Sequence numbers and event queue
> > length limitation is there as well. There are countless of smaller
> > changes, too.
> >
> > A few notes on the patches:
> >
> > - I don't like the locking too much. Perhaps the file handle specific
> > lock (events->lock) could be dropped in favour of the lock for
> > v4l2_file_handle in video_device?
> >
> > - Poll. The V4L2 specifiction says:
> >
> > 	"When the application did not call VIDIOC_QBUF or
> > 	VIDIOC_STREAMON yet the poll() function succeeds, but sets the
> > 	POLLERR flag in the revents field."
> >
> >  The current events for OMAP 3 ISP are related to streaming but not all
> > might be in future. For example there might be some radio or DVB related
> > events.
> 
> I know for sure that we will have to be able to handle events when not
> streaming. E.g. events that tell when a HDMI connector was plugged in
> or when the EDID was read will need to arrive whether streaming is on
> or off.

I agree with you. The V4L2 specification will then need to be changed, 
otherwise we won't be able to poll() for events. poll() wouldn't return 
immediately anymore if no buffer is queued or if the device isn't streaming. 
That might break existing applications (although we could argue that some of 
those applications are somehow broken already if they rely on such a weird 
feature).

If we want to avoid disturbing existing applications we could still return 
POLLERR immediately when not streaming if no event has been subscribed to.

> > - Sequence numbers are local to file handles.
> 
> That is how it should be.
> 
> > - Subscribing V4L2_EVENT_ALL causes any other events to be unsubscribed.
> >
> > - If V4L2_EVENT_ALL has been subscribed, unsubscribing any one of the
> > events leads to V4L2_EVENT_ALL to be unsubscribed. This problem would be
> > difficult to work around since this would require the event system to be
> > aware of the driver private events as well.
> 
> Good point. Perhaps attempting to unsubscribe a single event when EVENT_ALL
> has been subscribed should result in an error? I.e., you can only
> unsubscribe ALL when you subscribed to ALL in the first place.

-- 
Regards,

Laurent Pinchart

Re: [RFC v2 5/7] V4L: Events: Limit event queue length

[Hans Verkuil]

> Hi Hans,
>
> On Monday 18 January 2010 13:58:09 Hans Verkuil wrote:
>> On Tue, 22 Dec 2009, Sakari Ailus wrote:
>> > Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
>> > further events will be dropped.
>> >
>> > This patch also updates the count field properly, 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 |   10 +++++++++-
>> > include/media/v4l2-event.h       |    5 +++++
>> > 2 files changed, 14 insertions(+), 1 deletions(-)
>
> [snip]
>
>> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
>> > index b11de92..69305c6 100644
>> > --- a/include/media/v4l2-event.h
>> > +++ b/include/media/v4l2-event.h
>> > @@ -28,6 +28,10 @@
>> > #include <linux/types.h>
>> > #include <linux/videodev2.h>
>> >
>> > +#include <asm/atomic.h>
>> > +
>> > +#define V4L2_MAX_EVENTS		1024 /* Ought to be enough for everyone. */
>>
>> I think this should be programmable by the driver. Most drivers do not
>> use
>> events at all, so by default it should be 0 or perhaps it can check
>> whether
>> the ioctl callback structure contains the event ioctls and set it to 0
>> or
>> some initial default value.
>>
>> And you want this to be controlled on a per-filehandle basis even. If I
>>  look at ivtv, then most of the device nodes will not have events, only
>> a
>>  few will support events. And for one device node type I know that there
>>  will only be a single event when stopping the streaming, while another
>>  device node type will get an event each frame.
>
> Don't you mean per video node instead of per file handle ? In that case we
> could add a new field to video_device structure that must be initialized
> by
> drivers before registering the device.

Yes, that's what I meant (although you state it much more clearly :-) ).

Regards,

      Hans

>> So being able to adjust the event queue dynamically will give more
>> control
>> and prevent unnecessary waste of memory resources.
>
> --
> Regards,
>
> Laurent Pinchart
>


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

Re: [RFC v2 0/7] V4L2 file handles and event interface

[Hans Verkuil]

> Hi Hans,
>
> On Monday 18 January 2010 14:07:33 Hans Verkuil wrote:
>> On Tue, 22 Dec 2009, Sakari Ailus wrote:
>> > Hi,
>> >
>> > Here's the second version of the V4L2 file handle and event interface
>> > patchset. Still RFC since I'd like to get more feedback on it.
>> >
>> > The first patch adds the V4L2 file handle support and the rest are for
>> > V4L2 events.
>> >
>> > The patchset works 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 events):
>> >
>> > 	git://gitorious.org/omap3camera/mainline.git event
>> >
>> > The major change since the last one v4l2_fh structure is now part of
>> > driver's own file handle. It's used as file->private_data as well. I
>> did
>> > this based on Hans Verkuil's suggestion. Sequence numbers and event
>> queue
>> > length limitation is there as well. There are countless of smaller
>> > changes, too.
>> >
>> > A few notes on the patches:
>> >
>> > - I don't like the locking too much. Perhaps the file handle specific
>> > lock (events->lock) could be dropped in favour of the lock for
>> > v4l2_file_handle in video_device?
>> >
>> > - Poll. The V4L2 specifiction says:
>> >
>> > 	"When the application did not call VIDIOC_QBUF or
>> > 	VIDIOC_STREAMON yet the poll() function succeeds, but sets the
>> > 	POLLERR flag in the revents field."
>> >
>> >  The current events for OMAP 3 ISP are related to streaming but not
>> all
>> > might be in future. For example there might be some radio or DVB
>> related
>> > events.
>>
>> I know for sure that we will have to be able to handle events when not
>> streaming. E.g. events that tell when a HDMI connector was plugged in
>> or when the EDID was read will need to arrive whether streaming is on
>> or off.
>
> I agree with you. The V4L2 specification will then need to be changed,
> otherwise we won't be able to poll() for events. poll() wouldn't return
> immediately anymore if no buffer is queued or if the device isn't
> streaming.
> That might break existing applications (although we could argue that some
> of
> those applications are somehow broken already if they rely on such a weird
> feature).
>
> If we want to avoid disturbing existing applications we could still return
> POLLERR immediately when not streaming if no event has been subscribed to.

I think this is a very reasonable approach.

Regards,

         Hans

>
>> > - Sequence numbers are local to file handles.
>>
>> That is how it should be.
>>
>> > - Subscribing V4L2_EVENT_ALL causes any other events to be
>> unsubscribed.
>> >
>> > - If V4L2_EVENT_ALL has been subscribed, unsubscribing any one of the
>> > events leads to V4L2_EVENT_ALL to be unsubscribed. This problem would
>> be
>> > difficult to work around since this would require the event system to
>> be
>> > aware of the driver private events as well.
>>
>> Good point. Perhaps attempting to unsubscribe a single event when
>> EVENT_ALL
>> has been subscribed should result in an error? I.e., you can only
>> unsubscribe ALL when you subscribed to ALL in the first place.
>
> --
> Regards,
>
> Laurent Pinchart
>


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

Re: [RFC v2 1/7] V4L: File handles

[Sakari Ailus]

Hans Verkuil wrote:
> Hi Sakari,

Hi Hans!

> I should have reviewed this weeks ago, but better late than never...

No problem, and thanks for the review!

> On Tue, 22 Dec 2009, 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.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> ---
>> drivers/media/video/Makefile   |    2 +-
>> drivers/media/video/v4l2-dev.c |    2 +
>> drivers/media/video/v4l2-fh.c  |   57
>> ++++++++++++++++++++++++++++++++++++++++
>> include/media/v4l2-dev.h       |    4 +++
>> include/media/v4l2-fh.h        |   41 ++++++++++++++++++++++++++++
>> 5 files changed, 105 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 a61e3f3..1947146 100644
>> --- a/drivers/media/video/Makefile
>> +++ b/drivers/media/video/Makefile
>> @@ -10,7 +10,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
>> +videodev-objs    :=    v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
>>
>> # V4L2 core modules
>>
>> diff --git a/drivers/media/video/v4l2-dev.c
>> b/drivers/media/video/v4l2-dev.c
>> index 7090699..15b2ac8 100644
>> --- a/drivers/media/video/v4l2-dev.c
>> +++ b/drivers/media/video/v4l2-dev.c
>> @@ -421,6 +421,8 @@ static int __video_register_device(struct
>> video_device *vdev, int type, int nr,
>>     if (!vdev->release)
>>         return -EINVAL;
>>
>> +    v4l2_fh_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..406e4ac
>> --- /dev/null
>> +++ b/drivers/media/video/v4l2-fh.c
>> @@ -0,0 +1,57 @@
>> +/*
>> + * 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>
>> +
>> +#include <linux/sched.h>
>> +#include <linux/vmalloc.h>
> 
> Weird includes. I would expect to see only spinlock.h and list.h to be
> included
> here.

Oops. They're clearly leftovers from somewhere else. Turns out even
those are not needed.

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

Re: [RFC v2 4/7] V4L: Events: Add backend

[Sakari Ailus]

Hans Verkuil wrote:
> Hi Sakari,

Hi, Hans!

> The code looks good, but I'm not so sure about the use of kmem_cache_*. It
> seems serious overkill to me.
> 
> Most of the time there will only be a handful of event messages pending. So
> setting up a kmem_cache for this seems unnecessary to me.
> 
> A much better way to ensure fast event reporting IMHO would be to keep a
> list
> of empty event messages rather than destroying an event after it is
> dequeued.
> 
> So you have two queues per filehandle: pending and empty; initially both
> are
> empty. When a new event has to be queued the code checks if there are
> events
> available for reuse in the empty queue, and if not a new event struct is
> allocated and added to the pending queue.

I actually had this kind of setup there for a while. Then I thought it'd
be too ugly and went for kmem_cache instead.

The other reason is that it's convenient to keep the memory allocated
even if there are no events subscribed or the device isn't open. For
1000 events that's 96 kiB. I guess an unused kmem_cache object consumes
extra memory very little. The cached slabs can be explicitly freed
anyway by the driver.

The size of the kmem_cache also adjusts based on the number of events in
the queue. Allocating kmem_cache objects is fast if they already exist,
too. There can be temporary delays from allocation, of course.

I can bring it back, sure, if you see a fixed allocation better.

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

Re: [RFC v2 5/7] V4L: Events: Limit event queue length

[Sakari Ailus]

Hans Verkuil wrote:
> Hi Sakari,

Hi Hans,

And thanks for the comments!

...
>> @@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
>> v4l2_event *event)
>>     ev = list_first_entry(&events->available, struct _v4l2_event, list);
>>     list_del(&ev->list);
>>
>> -    ev->event.count = !list_empty(&events->available);
>> +    atomic_dec(&events->navailable);
>> +    ev->event.count = atomic_read(&events->navailable);
> 
> Combine these two lines to atomic_dec_return().

Will fix this.

>>
>>     spin_unlock_irqrestore(&events->lock, flags);
>>
>> @@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev,
>> struct v4l2_event *ev)
>>         if (!v4l2_event_subscribed(fh, ev->type))
>>             continue;
>>
>> +        if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
>> +            continue;
>> +
>>         _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
>>         if (!_ev)
>>             continue;
>> @@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev,
>> struct v4l2_event *ev)
>>         list_add_tail(&_ev->list, &fh->events.available);
>>         spin_unlock(&fh->events.lock);
>>
>> +        atomic_inc(&fh->events.navailable);
>> +
>>         wake_up_all(&fh->events.wait);
>>     }
>>
>> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
>> index b11de92..69305c6 100644
>> --- a/include/media/v4l2-event.h
>> +++ b/include/media/v4l2-event.h
>> @@ -28,6 +28,10 @@
>> #include <linux/types.h>
>> #include <linux/videodev2.h>
>>
>> +#include <asm/atomic.h>
>> +
>> +#define V4L2_MAX_EVENTS        1024 /* Ought to be enough for
>> everyone. */
> 
> I think this should be programmable by the driver. Most drivers do not use
> events at all, so by default it should be 0 or perhaps it can check whether
> the ioctl callback structure contains the event ioctls and set it to 0 or
> some initial default value.

Right. I'll make the event queue size to be defined by the driver.

I'm now planning to make a queue for free events common to file handles
in video device. A statically allocated queue for each file handle is
probably too much overkill. But a device global queue also means that a
process that doesn't dequeue its events will starve the others.

> And you want this to be controlled on a per-filehandle basis even. If I
> look
> at ivtv, then most of the device nodes will not have events, only a few
> will
> support events. And for one device node type I know that there will only be
> a single event when stopping the streaming, while another device node type
> will get an event each frame.

Instead of initialising the events by the V4L2, the driver could do this
and specify the queue size at the same time. The overhead for the
drivers not using events would be the event information in the
video_device structure. That could be made a pointer as well.

> So being able to adjust the event queue dynamically will give more control
> and prevent unnecessary waste of memory resources.

This sounds to me like trying to re-invent the kmem_cache now. :-)

If the event queue is allocated in some other means than kmem_cache I
think the size should be fixed. The driver probably knows the best
what's the reasonable maximum event queue size and that could be
allocated statically. If that overflows then so be it.

Regards,

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