[PATCH 0/8] V4L2 file handles and event interface

[PATCH 0/8] V4L2 file handles and event interface

[Sakari Ailus]

[-- Attachment #1: Type: text/plain, Size: 1568 bytes --]

Hi,

Here's the third version of the V4L2 file handle and event interface
patchset. I've marked this as PATCH instead of RFC for the first time.

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

The second patch adds reference count for the v4l2_fh structures. This
is useful later when queueing events to file handles.

The next six patches are for the event interface.

The patchset has been tested with the OMAP 3 ISP driver. A simple test
program can be found as the attachment. Patches for OMAP 3 ISP are not
part of this patchset but are available in Gitorious (branch is called
event):

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

The major change since the last RFC set is changing the kmem_cache
allocation to static per file handle allocation. The number of
allocatable events that way is likely small and can now be set by the
driver. Adding further events to free queue is also possible runtime.
Also, if a driver does not use events, the event system overhead for the
driver is limited to one pointer in v4l2_fh structure.

Locking has been also improved. The v4l2_fh structures have reference
count which makes it possible to free the list lock when working on
individual file handles. The spinlocks have been moved from events to
the file handles themselves.

To unsubscribe V4L2_EVENT_ALL, VIDIOC_UNSUBSCRIBE_EVENT has to be issued
for that event ID.

Comments would be welcome indeed. Especially on the new locking and
refcounting scheme.

Cheers,

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

[-- Attachment #2: polltest2.c --]
[-- Type: text/x-csrc, Size: 1071 bytes --]

#include <stdio.h>

#include <sys/select.h>
#include <sys/types.h>
#include <sys/stat.h>
#include <fcntl.h>

#include <linux/videodev2.h>
#include <mach/isp_user.h>

int main(int argc, char *argv) {
	struct v4l2_event_subscription sub;
	struct v4l2_event ev;
	int fd;
	fd_set exfds;
	int ret;

	fd = open("/dev/video0", O_RDONLY);
	if (fd < 0) {
		perror("select()");
		return 1;
	}

	printf("fd %d\n", fd);

	sub.type = V4L2_EVENT_ALL;

	ret = ioctl(fd, VIDIOC_SUBSCRIBE_EVENT, &sub);
	if (ret < 0) {
		perror("subscribe()");
	} else
		printf("okay...\n");

	FD_ZERO(&exfds);
	FD_SET(fd, &exfds);

	while (1) {
		ret = pselect(fd + 1, NULL, NULL, &exfds, NULL, NULL);
		printf("pselect %d\n", ret);
		if (ret < 0)
			break;
	
		ret = ioctl(fd, VIDIOC_DQEVENT, &ev);
		if (ret < 0) {
			perror("dequeue");
			return 1;
		} else
			printf("okay...\n");
	
		printf("type         %x\n",ev.type);
		printf("sequence     %d\n",ev.sequence);
		printf("count        %d\n",ev.count);
		printf("timestamp    %u.%9.9u\n",ev.timestamp.tv_sec,ev.timestamp.tv_nsec);
	}

	return 0;
}

[PATCH 1/8] V4L: File handles

[Sakari Ailus]

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

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

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

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 6e75647..b888ad1 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,8 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 
 omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
 
-videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
+			v4l2-fh.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 13a899d..d8c14a5 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -423,6 +423,8 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 	if (!vdev->release)
 		return -EINVAL;
 
+	v4l2_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..48bd32f
--- /dev/null
+++ b/drivers/media/video/v4l2-fh.c
@@ -0,0 +1,54 @@
+/*
+ * 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>
+
+int v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vdev->fhs.lock, flags);
+	list_add(&fh->list, &vdev->fhs.list);
+	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_new);
+
+void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&vdev->fhs.lock, flags);
+	list_del(&fh->list);
+	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_put);
+
+void v4l2_fh_init(struct video_device *vdev)
+{
+	spin_lock_init(&vdev->fhs.lock);
+	INIT_LIST_HEAD(&vdev->fhs.list);
+}
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 26d4e79..65d9dc8 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -18,6 +18,8 @@
 
 #include <media/media-entity.h>
 
+#include <media/v4l2-fh.h>
+
 #define VIDEO_MAJOR	81
 
 #define VFL_TYPE_GRABBER	0
@@ -82,6 +84,9 @@ struct video_device
 	/* attribute to differentiate multiple indices on one physical device */
 	int index;
 
+	/* V4L2 file handles */
+	struct v4l2_fhs	fhs;
+
 	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..b9e277c
--- /dev/null
+++ b/include/media/v4l2-fh.h
@@ -0,0 +1,49 @@
+/*
+ * 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;
+};
+
+/* File handle related data for video_device. */
+struct v4l2_fhs {
+	/* Lock for file handle list */
+	spinlock_t		lock;
+	/* File handle list */
+	struct list_head	list;
+};
+
+struct video_device;
+
+int v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh);
+void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh);
+void v4l2_fh_init(struct video_device *vdev);
+
+#endif /* V4L2_EVENT_H */
-- 
1.5.6.5

[PATCH 2/8] V4L: File handles: Add refcount to v4l2_fh

[Sakari Ailus]

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

diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 48bd32f..1728e1c 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -25,22 +25,32 @@
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
 
-int v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh)
+void v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh)
 {
 	unsigned long flags;
 
+	spin_lock_init(&fh->lock);
+	atomic_set(&fh->refcount, 1);
+
 	spin_lock_irqsave(&vdev->fhs.lock, flags);
 	list_add(&fh->list, &vdev->fhs.list);
 	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
-
-	return 0;
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_new);
 
+int v4l2_fh_get(struct video_device *vdev, struct v4l2_fh *fh)
+{
+	return !atomic_add_unless(&fh->refcount, 1, 0);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_get);
+
 void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh)
 {
 	unsigned long flags;
 
+	if (atomic_dec_return(&fh->refcount))
+		return;
+
 	spin_lock_irqsave(&vdev->fhs.lock, flags);
 	list_del(&fh->list);
 	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index b9e277c..51d6508 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -28,8 +28,12 @@
 #include <linux/types.h>
 #include <linux/list.h>
 
+#include <asm/atomic.h>
+
 struct v4l2_fh {
 	struct list_head	list;
+	atomic_t		refcount;
+	spinlock_t		lock;
 };
 
 /* File handle related data for video_device. */
@@ -42,7 +46,8 @@ struct v4l2_fhs {
 
 struct video_device;
 
-int v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh);
+void v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh);
+int v4l2_fh_get(struct video_device *vdev, struct v4l2_fh *fh);
 void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh);
 void v4l2_fh_init(struct video_device *vdev);
 
-- 
1.5.6.5

[PATCH 3/8] V4L: Events: Add new ioctls for events

[Sakari Ailus]

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

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

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

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

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

[Sakari Ailus]

Add support for event handling to do_ioctl.

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

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index b888ad1..68253d6 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -11,7 +11,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 omap2cam-objs	:=	omap24xxcam.o omap24xxcam-dma.o
 
 videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-subdev.o \
-			v4l2-fh.o
+			v4l2-fh.o v4l2-event.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index bfc4696..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

[PATCH 5/8] V4L: Events: Add backend

[Sakari Ailus]

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

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

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 0000000..7ae763f
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,317 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include <media/v4l2-dev.h>
+#include <media/v4l2-event.h>
+
+#include <linux/sched.h>
+
+/* In error case, return number of events *not* allocated. */
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
+{
+	struct v4l2_events *events = fh->events;
+	unsigned long flags;
+
+	for (; n > 0; n--) {
+		struct v4l2_kevent *kev;
+
+		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
+		if (kev == NULL)
+			return n;
+
+		spin_lock_irqsave(&fh->lock, flags);
+		list_add_tail(&kev->list, &events->free);
+		spin_unlock_irqrestore(&fh->lock, flags);
+
+	}
+
+	return n;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_alloc);
+
+void v4l2_event_exit(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+
+	if (!events)
+		return;
+
+	while (!list_empty(&events->free)) {
+		struct v4l2_kevent *kev;
+
+		kev = list_first_entry(&events->free,
+				       struct v4l2_kevent, list);
+		list_del(&kev->list);
+
+		kfree(kev);
+	}
+
+	while (!list_empty(&events->available)) {
+		struct v4l2_kevent *kev;
+
+		kev = list_first_entry(&events->available,
+				       struct v4l2_kevent, list);
+		list_del(&kev->list);
+
+		kfree(kev);
+	}
+
+	while (!list_empty(&events->subscribed)) {
+		struct v4l2_subscribed_event *sub;
+
+		sub = list_first_entry(&events->subscribed,
+				       struct v4l2_subscribed_event, list);
+
+		list_del(&sub->list);
+
+		kfree(sub);
+	}
+
+	kfree(events);
+	fh->events = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_exit);
+
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
+{
+	int ret;
+
+	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
+	if (fh->events == NULL)
+		return -ENOMEM;
+
+	init_waitqueue_head(&fh->events->wait);
+
+	INIT_LIST_HEAD(&fh->events->free);
+	INIT_LIST_HEAD(&fh->events->available);
+	INIT_LIST_HEAD(&fh->events->subscribed);
+
+	ret = v4l2_event_alloc(fh, n);
+	if (ret < 0)
+		v4l2_event_exit(fh);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_init);
+
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_kevent *kev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->lock, flags);
+
+	if (list_empty(&events->available)) {
+		spin_unlock_irqrestore(&fh->lock, flags);
+		return -ENOENT;
+	}
+
+	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
+	list_del(&kev->list);
+
+	kev->event.count = !list_empty(&events->available);
+
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	*event = kev->event;
+
+	spin_lock_irqsave(&fh->lock, flags);
+	list_add(&kev->list, &events->free);
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
+
+static struct v4l2_subscribed_event *__v4l2_kevent_subscribed(
+	struct v4l2_fh *fh, u32 type)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_subscribed_event *sev;
+
+	list_for_each_entry(sev, &events->subscribed, list) {
+		if (sev->type == type)
+			return sev;
+	}
+
+	return NULL;
+}
+
+struct v4l2_subscribed_event *v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type)
+{
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->lock, flags);
+
+	sev = __v4l2_kevent_subscribed(fh, type);
+
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	return sev;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribed);
+
+void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
+{
+	struct v4l2_fh *fh;
+	unsigned long flags;
+	struct v4l2_fh *put_me = NULL;
+
+	spin_lock_irqsave(&vdev->fhs.lock, flags);
+
+	list_for_each_entry(fh, &vdev->fhs.list, list) {
+		struct v4l2_events *events = fh->events;
+		struct v4l2_kevent *kev;
+
+		/* Is it subscribed? */
+		if (!v4l2_event_subscribed(fh, ev->type))
+			continue;
+
+		/* Can we get the file handle? */
+		if (v4l2_fh_get(vdev, fh))
+			continue;
+
+		/* List lock no longer required. */
+		spin_unlock_irqrestore(&vdev->fhs.lock, flags);
+
+		/* Put earlier v4l2_fh. */
+		if (put_me) {
+			v4l2_fh_put(vdev, put_me);
+			put_me = NULL;
+		}
+		put_me = fh;
+
+		/* Do we have any free events? */
+		spin_lock_irqsave(&fh->lock, flags);
+		if (list_empty(&events->free)) {
+			spin_unlock_irqrestore(&fh->lock, flags);
+			spin_lock_irqsave(&vdev->fhs.lock, flags);
+			continue;
+		}
+
+		/* Take one and fill it. */
+		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
+		list_del(&kev->list);
+		spin_unlock_irqrestore(&fh->lock, flags);
+
+		kev->event = *ev;
+
+		/* And add to the available list. */
+		spin_lock_irqsave(&fh->lock, flags);
+		list_add_tail(&kev->list, &events->available);
+		spin_unlock_irqrestore(&fh->lock, flags);
+
+		wake_up_all(&events->wait);
+
+		spin_lock_irqsave(&vdev->fhs.lock, flags);
+	}
+
+	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
+
+	/* Put final v4l2_fh if exists. */
+	if (put_me)
+		v4l2_fh_put(vdev, put_me);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_queue);
+
+int v4l2_event_pending(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&fh->lock, flags);
+	ret = !list_empty(&events->available);
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_pending);
+
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+			 struct v4l2_event_subscription *sub)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+	int ret = 0;
+
+	/* Allow subscribing to valid events only. */
+	if (sub->type < V4L2_EVENT_PRIVATE_START)
+		switch (sub->type) {
+		default:
+			return -EINVAL;
+		}
+
+	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
+	if (!sev)
+		return -ENOMEM;
+
+	spin_lock_irqsave(&fh->lock, flags);
+
+	if (__v4l2_kevent_subscribed(fh, sub->type) != NULL) {
+		ret = -EBUSY;
+		goto out;
+	}
+
+	INIT_LIST_HEAD(&sev->list);
+	sev->type = sub->type;
+
+	list_add(&sev->list, &events->subscribed);
+
+out:
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	if (ret)
+		kfree(sev);
+
+	return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub)
+{
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->lock, flags);
+
+	sev = __v4l2_kevent_subscribed(fh, sub->type);
+
+	if (sev == NULL) {
+		spin_unlock_irqrestore(&fh->lock, flags);
+		return -EINVAL;
+	}
+
+	list_del(&sev->list);
+
+	spin_unlock_irqrestore(&fh->lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index 1728e1c..3bd40f8 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -24,6 +24,7 @@
 
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
 void v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh)
 {
@@ -54,6 +55,8 @@ void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh)
 	spin_lock_irqsave(&vdev->fhs.lock, flags);
 	list_del(&fh->list);
 	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
+
+	v4l2_event_exit(fh);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_put);
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
new file mode 100644
index 0000000..580c9d4
--- /dev/null
+++ b/include/media/v4l2-event.h
@@ -0,0 +1,64 @@
+/*
+ * include/media/v4l2-event.h
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#ifndef V4L2_EVENT_H
+#define V4L2_EVENT_H
+
+#include <linux/types.h>
+#include <linux/videodev2.h>
+
+struct v4l2_fh;
+struct video_device;
+
+struct v4l2_kevent {
+	struct list_head	list;
+	struct v4l2_event	event;
+};
+
+struct v4l2_subscribed_event {
+	struct list_head	list;
+	u32			type;
+};
+
+struct v4l2_events {
+	wait_queue_head_t	wait;
+	struct list_head	subscribed; /* Subscribed events */
+	struct list_head	available; /* Dequeueable event */
+	struct list_head	free; /* Events ready for use */
+};
+
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
+int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
+void v4l2_event_exit(struct v4l2_fh *fh);
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
+struct v4l2_subscribed_event *v4l2_event_subscribed(
+	struct v4l2_fh *fh, u32 type);
+void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
+int v4l2_event_pending(struct v4l2_fh *fh);
+int v4l2_event_subscribe(struct v4l2_fh *fh,
+			 struct v4l2_event_subscription *sub);
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub);
+
+#endif /* V4L2_EVENT_H */
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
index 51d6508..d9589c2 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -34,6 +34,7 @@ struct v4l2_fh {
 	struct list_head	list;
 	atomic_t		refcount;
 	spinlock_t		lock;
+	struct v4l2_events      *events; /* events, pending and subscribed */
 };
 
 /* File handle related data for video_device. */
-- 
1.5.6.5

[PATCH 6/8] V4L: Events: Count event queue length

[Sakari Ailus]

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

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

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 7ae763f..b921229 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -107,6 +107,8 @@ int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
 	INIT_LIST_HEAD(&fh->events->available);
 	INIT_LIST_HEAD(&fh->events->subscribed);
 
+	atomic_set(&fh->events->navailable, 0);
+
 	ret = v4l2_event_alloc(fh, n);
 	if (ret < 0)
 		v4l2_event_exit(fh);
@@ -128,10 +130,12 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
 		return -ENOENT;
 	}
 
+	BUG_ON(&events->navailable == 0);
+
 	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
 	list_del(&kev->list);
 
-	kev->event.count = !list_empty(&events->available);
+	kev->event.count = atomic_dec_return(&events->navailable);
 
 	spin_unlock_irqrestore(&fh->lock, flags);
 
@@ -225,6 +229,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		list_add_tail(&kev->list, &events->available);
 		spin_unlock_irqrestore(&fh->lock, flags);
 
+		atomic_inc(&events->navailable);
+
 		wake_up_all(&events->wait);
 
 		spin_lock_irqsave(&vdev->fhs.lock, flags);
@@ -240,15 +246,7 @@ EXPORT_SYMBOL_GPL(v4l2_event_queue);
 
 int v4l2_event_pending(struct v4l2_fh *fh)
 {
-	struct v4l2_events *events = fh->events;
-	unsigned long flags;
-	int ret;
-
-	spin_lock_irqsave(&fh->lock, flags);
-	ret = !list_empty(&events->available);
-	spin_unlock_irqrestore(&fh->lock, flags);
-
-	return ret;
+	return atomic_read(&fh->events->navailable);
 }
 EXPORT_SYMBOL_GPL(v4l2_event_pending);
 
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 580c9d4..282d215 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,8 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 
+#include <asm/atomic.h>
+
 struct v4l2_fh;
 struct video_device;
 
@@ -45,6 +47,7 @@ struct v4l2_events {
 	wait_queue_head_t	wait;
 	struct list_head	subscribed; /* Subscribed events */
 	struct list_head	available; /* Dequeueable event */
+	atomic_t                navailable;
 	struct list_head	free; /* Events ready for use */
 };
 
-- 
1.5.6.5

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

diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index b921229..7446b3d 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -108,6 +108,7 @@ int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
 	INIT_LIST_HEAD(&fh->events->subscribed);
 
 	atomic_set(&fh->events->navailable, 0);
+	atomic_set(&fh->events->sequence, -1);
 
 	ret = v4l2_event_alloc(fh, n);
 	if (ret < 0)
@@ -190,6 +191,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 	list_for_each_entry(fh, &vdev->fhs.list, list) {
 		struct v4l2_events *events = fh->events;
 		struct v4l2_kevent *kev;
+		u32 sequence;
 
 		/* Is it subscribed? */
 		if (!v4l2_event_subscribed(fh, ev->type))
@@ -209,6 +211,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		}
 		put_me = fh;
 
+		/* Increase event sequence number on fh. */
+		sequence = atomic_inc_return(&events->sequence);
+
 		/* Do we have any free events? */
 		spin_lock_irqsave(&fh->lock, flags);
 		if (list_empty(&events->free)) {
@@ -223,6 +228,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
 		spin_unlock_irqrestore(&fh->lock, flags);
 
 		kev->event = *ev;
+		kev->event.sequence = sequence;
 
 		/* And add to the available list. */
 		spin_lock_irqsave(&fh->lock, flags);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 282d215..3db0c3b 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -49,6 +49,7 @@ struct v4l2_events {
 	struct list_head	available; /* Dequeueable event */
 	atomic_t                navailable;
 	struct list_head	free; /* Events ready for use */
+	atomic_t                sequence;
 };
 
 int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
-- 
1.5.6.5

[PATCH 8/8] V4L: Events: Support all events

[Sakari Ailus]

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

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

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

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

[Sakari Ailus]

Sakari Ailus wrote:
> Add support for event handling to do_ioctl.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
...
> @@ -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);

Replying to myself, there seems to be valid use for the struct file as
an argument to the function fields. That is a way to get the video
device pointer using video_devdata(). Otherwise the video device pointer
has to be stored in the file handle instead.

So I'm going to add the file pointers as first arguments here as they
are in other functions unless there are objections. The type of second
argument is still going to be struct v4l2_fh *.

Regards,

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

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

[Hans Verkuil]

On Sunday 07 February 2010 13:15:12 Sakari Ailus wrote:
> Sakari Ailus wrote:
> > Add support for event handling to do_ioctl.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ...
> > @@ -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);
> 
> Replying to myself, there seems to be valid use for the struct file as
> an argument to the function fields. That is a way to get the video
> device pointer using video_devdata(). Otherwise the video device pointer
> has to be stored in the file handle instead.
> 
> So I'm going to add the file pointers as first arguments here as they
> are in other functions unless there are objections. The type of second
> argument is still going to be struct v4l2_fh *.

No, instead v4l2_fh should have a pointer to video_device. Much cleaner and
consistent with the other v4l2 internal structures.

And see also my review I'm posting soon.

Regards,

	Hans

> 
> Regards,
> 
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 5/8] V4L: Events: Add backend

[Hans Verkuil]

Hi Sakari,

Once again, thanks for all the work you're doing on this! Very much appreciated!

As you requested, I've reviewed this code with special attention to refcounting
and locking.

In general, locking is hard, so the fewer locks there are, the easier it is
to get locking right and to maintain/understand the code. 

Currently you have a lock in v4l2_fhs (at the video_device level) and a lock and
a refcount in v4l2_fh.

In my opinion this causes more complexity than is needed. I think a single lock
in v4l2_fhs would suffice. This does mean that struct v4l2_fh needs a pointer
to video_device (which is a good idea anyway).

The only place where this locks more than is strictly necessary is the dequeue
function. On the other hand, the lock that is taken there is very short and
I prefer simplicity over unproven performance increases. I actually suspect
that all the complexity might introduce slower performance than slightly
sub-optimal locking. I have rewritten the queue and dequeue event functions
below to what I think should be done there. As you can see, they are now a lot
easier to understand and a lot shorter.

Regards,

	Hans

On Saturday 06 February 2010 19:02:08 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/v4l2-event.c |  317 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-fh.c    |    3 +
>  include/media/v4l2-event.h       |   64 ++++++++
>  include/media/v4l2-fh.h          |    1 +
>  4 files changed, 385 insertions(+), 0 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-event.c
>  create mode 100644 include/media/v4l2-event.h
> 
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..7ae763f
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,317 @@
> +/*
> + * drivers/media/video/v4l2-event.c
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +/* In error case, return number of events *not* allocated. */
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +
> +	for (; n > 0; n--) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
> +		if (kev == NULL)
> +			return n;
> +
> +		spin_lock_irqsave(&fh->lock, flags);
> +		list_add_tail(&kev->list, &events->free);
> +		spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	}
> +
> +	return n;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_alloc);
> +
> +void v4l2_event_exit(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +
> +	if (!events)
> +		return;
> +
> +	while (!list_empty(&events->free)) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = list_first_entry(&events->free,
> +				       struct v4l2_kevent, list);
> +		list_del(&kev->list);
> +
> +		kfree(kev);
> +	}

This can be done cleaner via a static inline function to delete an event
list.

> +
> +	while (!list_empty(&events->available)) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = list_first_entry(&events->available,
> +				       struct v4l2_kevent, list);
> +		list_del(&kev->list);
> +
> +		kfree(kev);
> +	}
> +
> +	while (!list_empty(&events->subscribed)) {
> +		struct v4l2_subscribed_event *sub;
> +
> +		sub = list_first_entry(&events->subscribed,
> +				       struct v4l2_subscribed_event, list);
> +
> +		list_del(&sub->list);
> +
> +		kfree(sub);
> +	}
> +
> +	kfree(events);
> +	fh->events = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit);
> +
> +int v4l2_event_init(struct v4l2_fh *fh, unsigned int n)
> +{
> +	int ret;
> +
> +	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
> +	if (fh->events == NULL)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&fh->events->wait);
> +
> +	INIT_LIST_HEAD(&fh->events->free);
> +	INIT_LIST_HEAD(&fh->events->available);
> +	INIT_LIST_HEAD(&fh->events->subscribed);
> +
> +	ret = v4l2_event_alloc(fh, n);
> +	if (ret < 0)
> +		v4l2_event_exit(fh);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_kevent *kev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&fh->lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
> +	list_del(&kev->list);
> +
> +	kev->event.count = !list_empty(&events->available);
> +
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	*event = kev->event;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +	list_add(&kev->list, &events->free);
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);

int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
{
	struct v4l2_events *events = fh->events;
	struct v4l2_kevent *kev;
	unsigned long flags;

	spin_lock_irqsave(&fh->vdev->fhs.lock, flags);

	if (list_empty(&events->available)) {
		spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);
		return -ENOENT;
	}

	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
	list_move(&kev->list, &events->free);

	kev->event.count = !list_empty(&events->available);

	*event = kev->event;

	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);

	return 0;
}

One possible optimization might be to do this (as you did in the original code):

	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
	list_del(&kev->list);
	kev->event.count = !list_empty(&events->available);
	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);

	*event = kev->event;

	spin_lock_irqsave(&fh->vdev->fhs.lock, flags);
	list_add(&kev->list, &events->free);
	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);

However, this would need some testing first to see what the performance
trade-off is between unlocking and locking vs. copying 120 bytes.

> +
> +static struct v4l2_subscribed_event *__v4l2_kevent_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_subscribed_event *sev;
> +
> +	list_for_each_entry(sev, &events->subscribed, list) {
> +		if (sev->type == type)
> +			return sev;
> +	}
> +
> +	return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +
> +	sev = __v4l2_kevent_subscribed(fh, type);
> +
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	return sev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribed);
> +
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> +{
> +	struct v4l2_fh *fh;
> +	unsigned long flags;
> +	struct v4l2_fh *put_me = NULL;
> +
> +	spin_lock_irqsave(&vdev->fhs.lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fhs.list, list) {
> +		struct v4l2_events *events = fh->events;
> +		struct v4l2_kevent *kev;
> +
> +		/* Is it subscribed? */
> +		if (!v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		/* Can we get the file handle? */
> +		if (v4l2_fh_get(vdev, fh))
> +			continue;
> +
> +		/* List lock no longer required. */
> +		spin_unlock_irqrestore(&vdev->fhs.lock, flags);
> +
> +		/* Put earlier v4l2_fh. */
> +		if (put_me) {
> +			v4l2_fh_put(vdev, put_me);
> +			put_me = NULL;
> +		}
> +		put_me = fh;
> +
> +		/* Do we have any free events? */
> +		spin_lock_irqsave(&fh->lock, flags);
> +		if (list_empty(&events->free)) {
> +			spin_unlock_irqrestore(&fh->lock, flags);
> +			spin_lock_irqsave(&vdev->fhs.lock, flags);
> +			continue;
> +		}
> +
> +		/* Take one and fill it. */
> +		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
> +		list_del(&kev->list);
> +		spin_unlock_irqrestore(&fh->lock, flags);
> +
> +		kev->event = *ev;
> +
> +		/* And add to the available list. */
> +		spin_lock_irqsave(&fh->lock, flags);
> +		list_add_tail(&kev->list, &events->available);
> +		spin_unlock_irqrestore(&fh->lock, flags);
> +
> +		wake_up_all(&events->wait);
> +
> +		spin_lock_irqsave(&vdev->fhs.lock, flags);
> +	}
> +
> +	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
> +
> +	/* Put final v4l2_fh if exists. */
> +	if (put_me)
> +		v4l2_fh_put(vdev, put_me);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);

void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
{
	struct v4l2_fh *fh;
	unsigned long flags;

	spin_lock_irqsave(&vdev->fhs.lock, flags);

	list_for_each_entry(fh, &vdev->fhs.list, list) {
		struct v4l2_events *events = fh->events;
		struct v4l2_kevent *kev;

		/* Do we have any free events and are we subscribed? */
		if (list_empty(&events->free) ||
		    !__v4l2_event_subscribed(fh, ev->type))
			continue;

		/* Take one and fill it. */
		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
		kev->event = *ev;
		list_move_tail(&kev->list, &events->available);

		wake_up_all(&events->wait);
	}

	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
}

> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +	int ret;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +	ret = !list_empty(&events->available);
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
> +
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	/* Allow subscribing to valid events only. */
> +	if (sub->type < V4L2_EVENT_PRIVATE_START)
> +		switch (sub->type) {
> +		default:
> +			return -EINVAL;
> +		}
> +
> +	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
> +	if (!sev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +
> +	if (__v4l2_kevent_subscribed(fh, sub->type) != NULL) {
> +		ret = -EBUSY;
> +		goto out;
> +	}
> +
> +	INIT_LIST_HEAD(&sev->list);
> +	sev->type = sub->type;
> +
> +	list_add(&sev->list, &events->subscribed);
> +
> +out:
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	if (ret)
> +		kfree(sev);
> +
> +	return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->lock, flags);
> +
> +	sev = __v4l2_kevent_subscribed(fh, sub->type);
> +
> +	if (sev == NULL) {
> +		spin_unlock_irqrestore(&fh->lock, flags);
> +		return -EINVAL;
> +	}
> +
> +	list_del(&sev->list);
> +
> +	spin_unlock_irqrestore(&fh->lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 1728e1c..3bd40f8 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -24,6 +24,7 @@
>  
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  
>  void v4l2_fh_new(struct video_device *vdev, struct v4l2_fh *fh)
>  {
> @@ -54,6 +55,8 @@ void v4l2_fh_put(struct video_device *vdev, struct v4l2_fh *fh)
>  	spin_lock_irqsave(&vdev->fhs.lock, flags);
>  	list_del(&fh->list);
>  	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
> +
> +	v4l2_event_exit(fh);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_put);
>  
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..580c9d4
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,64 @@
> +/*
> + * include/media/v4l2-event.h
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef V4L2_EVENT_H
> +#define V4L2_EVENT_H
> +
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +
> +struct v4l2_fh;
> +struct video_device;
> +
> +struct v4l2_kevent {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +struct v4l2_events {
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events */
> +	struct list_head	available; /* Dequeueable event */
> +	struct list_head	free; /* Events ready for use */
> +};
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> +int v4l2_event_init(struct v4l2_fh *fh, unsigned int n);
> +void v4l2_event_exit(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type);
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
> +int v4l2_event_pending(struct v4l2_fh *fh);
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> +			 struct v4l2_event_subscription *sub);
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub);
> +
> +#endif /* V4L2_EVENT_H */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index 51d6508..d9589c2 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -34,6 +34,7 @@ struct v4l2_fh {
>  	struct list_head	list;
>  	atomic_t		refcount;
>  	spinlock_t		lock;
> +	struct v4l2_events      *events; /* events, pending and subscribed */
>  };
>  
>  /* File handle related data for video_device. */
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH 5/8] V4L: Events: Add backend

[Sakari Ailus]

Hans Verkuil wrote:
> Hi Sakari,
> 
> Once again, thanks for all the work you're doing on this! Very much appreciated!

You're welcome. I think I want to see this finished as much as you do.
:-) And many thanks for the review to you!

> As you requested, I've reviewed this code with special attention to refcounting
> and locking.
>
> In general, locking is hard, so the fewer locks there are, the easier it is
> to get locking right and to maintain/understand the code. 
> 
> Currently you have a lock in v4l2_fhs (at the video_device level) and a lock and
> a refcount in v4l2_fh.
> 
> In my opinion this causes more complexity than is needed. I think a single lock
> in v4l2_fhs would suffice. This does mean that struct v4l2_fh needs a pointer
> to video_device (which is a good idea anyway).
> 
> The only place where this locks more than is strictly necessary is the dequeue
> function. On the other hand, the lock that is taken there is very short and
> I prefer simplicity over unproven performance increases. I actually suspect
> that all the complexity might introduce slower performance than slightly
> sub-optimal locking. I have rewritten the queue and dequeue event functions
> below to what I think should be done there. As you can see, they are now a lot
> easier to understand and a lot shorter.

Could be true. Usually the number of file handles keeping a device open
is rather small, as is the number of subscriptions. My idea there was to
keep the time during which interrupts were disabled small.

> Regards,
> 
> 	Hans
> 
...
>> +void v4l2_event_exit(struct v4l2_fh *fh)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +
>> +	if (!events)
>> +		return;
>> +
>> +	while (!list_empty(&events->free)) {
>> +		struct v4l2_kevent *kev;
>> +
>> +		kev = list_first_entry(&events->free,
>> +				       struct v4l2_kevent, list);
>> +		list_del(&kev->list);
>> +
>> +		kfree(kev);
>> +	}
> 
> This can be done cleaner via a static inline function to delete an event
> list.

Made a macro out of that. All three can be freed that way.

...

>> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	struct v4l2_kevent *kev;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&fh->lock, flags);
>> +
>> +	if (list_empty(&events->available)) {
>> +		spin_unlock_irqrestore(&fh->lock, flags);
>> +		return -ENOENT;
>> +	}
>> +
>> +	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
>> +	list_del(&kev->list);
>> +
>> +	kev->event.count = !list_empty(&events->available);
>> +
>> +	spin_unlock_irqrestore(&fh->lock, flags);
>> +
>> +	*event = kev->event;
>> +
>> +	spin_lock_irqsave(&fh->lock, flags);
>> +	list_add(&kev->list, &events->free);
>> +	spin_unlock_irqrestore(&fh->lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
> 
> int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> {
> 	struct v4l2_events *events = fh->events;
> 	struct v4l2_kevent *kev;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&fh->vdev->fhs.lock, flags);
> 
> 	if (list_empty(&events->available)) {
> 		spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);
> 		return -ENOENT;
> 	}
> 
> 	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
> 	list_move(&kev->list, &events->free);
> 
> 	kev->event.count = !list_empty(&events->available);
> 
> 	*event = kev->event;
> 
> 	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);
> 
> 	return 0;
> }
> 
> One possible optimization might be to do this (as you did in the original code):
> 
> 	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
> 	list_del(&kev->list);
> 	kev->event.count = !list_empty(&events->available);
> 	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);
> 
> 	*event = kev->event;
> 
> 	spin_lock_irqsave(&fh->vdev->fhs.lock, flags);
> 	list_add(&kev->list, &events->free);
> 	spin_unlock_irqrestore(&fh->vdev->fhs.lock, flags);
> 
> However, this would need some testing first to see what the performance
> trade-off is between unlocking and locking vs. copying 120 bytes.

Probably makes no sense to drop the lock for that duration. I now keep
it all the time in the updated patch.

...

>> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
>> +{
>> +	struct v4l2_fh *fh;
>> +	unsigned long flags;
>> +	struct v4l2_fh *put_me = NULL;
>> +
>> +	spin_lock_irqsave(&vdev->fhs.lock, flags);
>> +
>> +	list_for_each_entry(fh, &vdev->fhs.list, list) {
>> +		struct v4l2_events *events = fh->events;
>> +		struct v4l2_kevent *kev;
>> +
>> +		/* Is it subscribed? */
>> +		if (!v4l2_event_subscribed(fh, ev->type))
>> +			continue;
>> +
>> +		/* Can we get the file handle? */
>> +		if (v4l2_fh_get(vdev, fh))
>> +			continue;
>> +
>> +		/* List lock no longer required. */
>> +		spin_unlock_irqrestore(&vdev->fhs.lock, flags);
>> +
>> +		/* Put earlier v4l2_fh. */
>> +		if (put_me) {
>> +			v4l2_fh_put(vdev, put_me);
>> +			put_me = NULL;
>> +		}
>> +		put_me = fh;
>> +
>> +		/* Do we have any free events? */
>> +		spin_lock_irqsave(&fh->lock, flags);
>> +		if (list_empty(&events->free)) {
>> +			spin_unlock_irqrestore(&fh->lock, flags);
>> +			spin_lock_irqsave(&vdev->fhs.lock, flags);
>> +			continue;
>> +		}
>> +
>> +		/* Take one and fill it. */
>> +		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
>> +		list_del(&kev->list);
>> +		spin_unlock_irqrestore(&fh->lock, flags);
>> +
>> +		kev->event = *ev;
>> +
>> +		/* And add to the available list. */
>> +		spin_lock_irqsave(&fh->lock, flags);
>> +		list_add_tail(&kev->list, &events->available);
>> +		spin_unlock_irqrestore(&fh->lock, flags);
>> +
>> +		wake_up_all(&events->wait);
>> +
>> +		spin_lock_irqsave(&vdev->fhs.lock, flags);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
>> +
>> +	/* Put final v4l2_fh if exists. */
>> +	if (put_me)
>> +		v4l2_fh_put(vdev, put_me);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> 
> void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> {
> 	struct v4l2_fh *fh;
> 	unsigned long flags;
> 
> 	spin_lock_irqsave(&vdev->fhs.lock, flags);
> 
> 	list_for_each_entry(fh, &vdev->fhs.list, list) {
> 		struct v4l2_events *events = fh->events;
> 		struct v4l2_kevent *kev;
> 
> 		/* Do we have any free events and are we subscribed? */
> 		if (list_empty(&events->free) ||
> 		    !__v4l2_event_subscribed(fh, ev->type))
> 			continue;
> 
> 		/* Take one and fill it. */
> 		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
> 		kev->event = *ev;
> 		list_move_tail(&kev->list, &events->available);
> 
> 		wake_up_all(&events->wait);
> 	}
> 
> 	spin_unlock_irqrestore(&vdev->fhs.lock, flags);
> }

Fixed.

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

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

[Sakari Ailus]

Hans Verkuil wrote:
> On Sunday 07 February 2010 13:15:12 Sakari Ailus wrote:
>> Sakari Ailus wrote:
>>> Add support for event handling to do_ioctl.
>>>
>>> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> ...
>>> @@ -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);
>>
>> Replying to myself, there seems to be valid use for the struct file as
>> an argument to the function fields. That is a way to get the video
>> device pointer using video_devdata(). Otherwise the video device pointer
>> has to be stored in the file handle instead.
>>
>> So I'm going to add the file pointers as first arguments here as they
>> are in other functions unless there are objections. The type of second
>> argument is still going to be struct v4l2_fh *.
> 
> No, instead v4l2_fh should have a pointer to video_device. Much cleaner and
> consistent with the other v4l2 internal structures.

Right. Fixed that. There's now struct video_device pointer in struct
v4l2_fh.

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