V4L2 file handles and event interface

Add sequence numbers to events.

Signed-off-by Sakari Ailus <[email protected]>

---

drivers/media/video/v4l2-event.c |    8 ++++++++
include/media/v4l2-event.h       |    1 +
2 files changed, 9 insertions(+), 0 deletions(-)

drivers/media/video/v4l2-event.c +0 -0

index 72fdf7f..cc2bf57 100644

drivers/media/video/v4l2-event.c +0 -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) {

drivers/media/video/v4l2-event.c +2 -0

+
 		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);
 

include/media/v4l2-event.h +1 -0

index 69305c6..5a778d4 100644
@@ -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. */
 };

Signature · 1 line

1.5.6.5

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 <[email protected]>

---
drivers/media/video/v4l2-event.c | 10 +++++++++-
include/media/v4l2-event.h | 5 +++++

2 files changed, 14 insertions(+), 1 deletions(-)

drivers/media/video/v4l2-event.c +2 -0

index 9fc0c81..72fdf7f 100644
@@ -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);
 

drivers/media/video/v4l2-event.c +5 -0

 	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);
 	}
 

include/media/v4l2-event.h +0 -0

index b11de92..69305c6 100644

include/media/v4l2-event.h +2 -0

+
 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. */
 };

Signature · 1 line

1.5.6.5

Quoted 11 lines · 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 <[email protected]>

---
drivers/media/video/v4l2-event.c | 10 +++++++++-
include/media/v4l2-event.h | 5 +++++

2 files changed, 14 insertions(+), 1 deletions(-)

include/media/v4l2-event.h line 33

Quoted 11 lines

include/media/v4l2-event.h +3 -0

index b11de92..69305c6 100644
@@ -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.

Hi Hans,

Quoted 13 lines · On Monday 18 January 2010 13:58:09 Hans Verkuil wrote:

Quoted 4 lines · 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

Quoted 7 lines

to number of further available events.

Signed-off-by Sakari Ailus <[email protected]>

---
drivers/media/video/v4l2-event.c | 10 +++++++++-
include/media/v4l2-event.h | 5 +++++

2 files changed, 14 insertions(+), 1 deletions(-)

[snip]

Quoted 20 lines

include/media/v4l2-event.h line 33

Quoted 11 lines

include/media/v4l2-event.h +3 -0

index b11de92..69305c6 100644
@@ -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.

Quoted 6 lines

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.

Quoted 3 lines

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

--
Regards,

Laurent Pinchart

Hi Sakari,

drivers/media/video/v4l2-event.c int v4l2_event_dequeue(struct v4l2_fh *fh, struct · line 110

Quoted 8 lines

drivers/media/video/v4l2-event.c +2 -1

@@ -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().

Quoted 32 lines

drivers/media/video/v4l2-event.c +3 -0

    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;

drivers/media/video/v4l2-event.c +2 -0

@@ -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);
    }

include/media/v4l2-event.h +0 -0

index b11de92..69305c6 100644
@@ -28,6 +28,10 @@
 #include <linux/types.h>
 #include <linux/videodev2.h>
 

Quoted 4 lines

include/media/v4l2-event.h +3 -0

+#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.

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 <[email protected]>

---
 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

drivers/media/video/Makefile +2 -1

index 1947146..dd6a853 100644
@@ -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
 

drivers/media/video/v4l2-dev.c +18 -3

index 15b2ac8..6d25297 100644
@@ -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)

drivers/media/video/v4l2-event.c new file +181 -0

new file mode 100644
index 0000000..9fc0c81
@@ -0,0 +1,254 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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;

drivers/media/video/v4l2-event.c new file +63 -0

+
+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);

drivers/media/video/v4l2-fh.c +4 -0

index 406e4ac..2b25ec9 100644
@@ -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);

include/media/v4l2-event.h new file +65 -0

new file mode 100644
index 0000000..b11de92
@@ -0,0 +1,65 @@
+/*
+ * include/media/v4l2-event.h
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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 */

include/media/v4l2-fh.h +3 -0

index 1efa916..c15bd13 100644
@@ -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;

drivers/media/video/v4l2-event.c line 191

Quoted 13 lines

drivers/media/video/v4l2-event.c new file +13 -0

+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?

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 <[email protected]>

---

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

drivers/media/video/Makefile +0 -0

index 1947146..dd6a853 100644
@@ -10,7 +10,8 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 

omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o

drivers/media/video/Makefile +2 -1

-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

drivers/media/video/v4l2-dev.c +18 -3

index 15b2ac8..6d25297 100644
@@ -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)

drivers/media/video/v4l2-event.c new file +254 -0

new file mode 100644
index 0000000..9fc0c81
@@ -0,0 +1,254 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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);

drivers/media/video/v4l2-fh.c +4 -0

index 406e4ac..2b25ec9 100644
@@ -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);

include/media/v4l2-event.h new file +65 -0

new file mode 100644
index 0000000..b11de92
@@ -0,0 +1,65 @@
+/*
+ * include/media/v4l2-event.h
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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 */

include/media/v4l2-fh.h +3 -0

index 1efa916..c15bd13 100644
@@ -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;

Signature · 1 line

1.5.6.5

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.

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 <[email protected]>

---

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

drivers/media/video/Makefile +0 -0

index a61e3f3..1947146 100644
@@ -10,7 +10,7 @@ stkwebcam-objs	:=	stk-webcam.o stk-sensor.o
 

omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o

drivers/media/video/Makefile +1 -1

-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

drivers/media/video/v4l2-dev.c +2 -0

index 7090699..15b2ac8 100644
@@ -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:

drivers/media/video/v4l2-fh.c new file +19 -0

new file mode 100644
index 0000000..406e4ac
@@ -0,0 +1,57 @@
+/*
+ * drivers/media/video/v4l2-fh.c
+ *
+ * V4L2 file handles.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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

drivers/media/video/v4l2-fh.c new file +28 -0

+
+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);
+}

include/media/v4l2-dev.h +4 -0

index 2dee938..8eac93d 100644
@@ -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 */

include/media/v4l2-fh.h new file +41 -0

new file mode 100644
index 0000000..1efa916
@@ -0,0 +1,41 @@
+/*
+ * include/media/v4l2-fh.h
+ *
+ * V4L2 file handle.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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 */

Signature · 1 line

1.5.6.5

Hi Sakari,

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

Quoted 81 lines

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 <[email protected]>

---

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

drivers/media/video/Makefile +0 -0

index a61e3f3..1947146 100644
@@ -10,7 +10,7 @@ stkwebcam-objs    :=    stk-webcam.o stk-sensor.o
 

omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o

drivers/media/video/Makefile +1 -1

-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

drivers/media/video/v4l2-dev.c +2 -0

index 7090699..15b2ac8 100644
@@ -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:

drivers/media/video/v4l2-fh.c new file +25 -0

new file mode 100644
index 0000000..406e4ac
@@ -0,0 +1,57 @@
+/*
+ * drivers/media/video/v4l2-fh.c
+ *
+ * V4L2 file handles.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus <[email protected]>
+ *
+ * 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>

drivers/media/video/v4l2-fh.c line 29

Quoted 4 lines

drivers/media/video/v4l2-fh.c new file +4 -0

+#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.

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,

Signature · 2 lines

Sakari Ailus
[email protected]

Quoted 32 lines · 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."

Quoted 3 lines

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.

Quoted 1 line

- Sequence numbers are local to file handles.

That is how it should be.

Quoted 6 lines

- 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.

Hi Hans,

Quoted 38 lines · On Monday 18 January 2010 14:07:33 Hans Verkuil wrote:

Quoted 9 lines · 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

Quoted 7 lines

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

Quoted 1 line

this based on Hans Verkuil's suggestion. Sequence numbers and event

queue

Quoted 9 lines

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?

Quoted 7 lines

- 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

Quoted 8 lines

Quoted 1 line

might be in future. For example there might be some radio or DVB

related

Quoted 1 line

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.

Quoted 11 lines

Quoted 2 lines

- Sequence numbers are local to file handles.

That is how it should be.

Quoted 1 line

- Subscribing V4L2_EVENT_ALL causes any other events to be

unsubscribed.

Quoted 3 lines

- 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

Quoted 8 lines

Quoted 1 line

difficult to work around since this would require the event system to

be

Quoted 1 line

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