Re: [RFC v2 4/7] V4L: Events: Add backend
Hans Verkuil wrote: Hi Sakari, Hi, Hans! The code looks good, but I'm not so sure about the use of kmem_cache_*. It seems serious overkill to me. Most of the time there will only be a handful of event messages pending. So setting up a kmem_cache for this seems unnecessary to me. A much better way to ensure fast event reporting IMHO would be to keep a list of empty event messages rather than destroying an event after it is dequeued. So you have two queues per filehandle: pending and empty; initially both are empty. When a new event has to be queued the code checks if there are events available for reuse in the empty queue, and if not a new event struct is allocated and added to the pending queue. I actually had this kind of setup there for a while. Then I thought it'd be too ugly and went for kmem_cache instead. The other reason is that it's convenient to keep the memory allocated even if there are no events subscribed or the device isn't open. For 1000 events that's 96 kiB. I guess an unused kmem_cache object consumes extra memory very little. The cached slabs can be explicitly freed anyway by the driver. The size of the kmem_cache also adjusts based on the number of events in the queue. Allocating kmem_cache objects is fast if they already exist, too. There can be temporary delays from allocation, of course. I can bring it back, sure, if you see a fixed allocation better. -- Sakari Ailus sakari.ai...@maxwell.research.nokia.com -- To unsubscribe from this list: send the line unsubscribe linux-media in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [RFC v2 4/7] V4L: Events: Add backend
Hi Sakari,
The code looks good, but I'm not so sure about the use of kmem_cache_*. It
seems serious overkill to me.
Most of the time there will only be a handful of event messages pending. So
setting up a kmem_cache for this seems unnecessary to me.
A much better way to ensure fast event reporting IMHO would be to keep a list
of empty event messages rather than destroying an event after it is dequeued.
So you have two queues per filehandle: pending and empty; initially both are
empty. When a new event has to be queued the code checks if there are events
available for reuse in the empty queue, and if not a new event struct is
allocated and added to the pending queue.
When userspace dequeues the event the kernel will not destroy the event struct
but requeue it in the empty queue for reuse. This is also an ideal place to
check for queue overflowing (which is definitely needed).
In general allocating memory is a slow operation, so not freeing that memory
will speed up things. Drivers might even preallocate a certain number of events
in the empty queue if the last drop of performance is needed.
But right now I don't think there is any need for that.
Regards,
Hans
On Tue, 22 Dec 2009, Sakari Ailus wrote:
Add event handling backend to V4L2. The backend handles event subscription
and delivery to file handles. Event subscriptions are based on file handle.
Events may be delivered to all subscribed file handles on a device
independent of where they originate from.
Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
---
drivers/media/video/Makefile |3 +-
drivers/media/video/v4l2-dev.c | 21 +++-
drivers/media/video/v4l2-event.c | 254 ++
drivers/media/video/v4l2-fh.c|4 +
include/media/v4l2-event.h | 65 ++
include/media/v4l2-fh.h |3 +
6 files changed, 346 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/video/v4l2-event.c
create mode 100644 include/media/v4l2-event.h
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 1947146..dd6a853 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,8 @@ stkwebcam-objs:= stk-webcam.o stk-sensor.o
omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o
-videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
+videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
+ v4l2-event.o
# V4L2 core modules
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 15b2ac8..6d25297 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -613,22 +613,36 @@ static int __init videodev_init(void)
dev_t dev = MKDEV(VIDEO_MAJOR, 0);
int ret;
+ ret = v4l2_event_init();
+ if (ret 0) {
+ printk(KERN_WARNING videodev: unable to initialise events\n);
+ return ret;
+ }
+
printk(KERN_INFO Linux video capture interface: v2.00\n);
ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
if (ret 0) {
printk(KERN_WARNING videodev: unable to get major %d\n,
VIDEO_MAJOR);
- return ret;
+ goto out_register_chrdev_region;
}
ret = class_register(video_class);
if (ret 0) {
- unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
printk(KERN_WARNING video_dev: class_register failed\n);
- return -EIO;
+ ret = -EIO;
+ goto out_class_register;
}
return 0;
+
+out_class_register:
+ unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+
+out_register_chrdev_region:
+ v4l2_event_exit();
+
+ return ret;
}
static void __exit videodev_exit(void)
@@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
class_unregister(video_class);
unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+ v4l2_event_exit();
}
module_init(videodev_init)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 000..9fc0c81
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,254 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus sakari.ai...@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
Re: [RFC v2 4/7] V4L: Events: Add backend
Hi Andy,
Andy Walls wrote:
+int v4l2_event_pending(struct v4l2_fh *fh)
+{
+ struct v4l2_events *events =fh-events;
+ unsigned long flags;
+ int ret;
+
+ spin_lock_irqsave(events-lock, flags);
+ ret = !list_empty(events-available);
+ spin_unlock_irqrestore(events-lock, flags);
+
+ return ret;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_pending);
Hi Sakari,
Disabling and restoring local interrupts to check if any events are
pending seems excessive.
Since you added an atomic_t with the number of events available in patch
5/7, why don't you just check that atomic_t here?
Thanks for the comments!
I'll put the fix to patch 5.
--
Sakari Ailus
sakari.ai...@maxwell.research.nokia.com
--
To unsubscribe from this list: send the line unsubscribe linux-media in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[RFC v2 4/7] V4L: Events: Add backend
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.ai...@maxwell.research.nokia.com
---
drivers/media/video/Makefile |3 +-
drivers/media/video/v4l2-dev.c | 21 +++-
drivers/media/video/v4l2-event.c | 254 ++
drivers/media/video/v4l2-fh.c|4 +
include/media/v4l2-event.h | 65 ++
include/media/v4l2-fh.h |3 +
6 files changed, 346 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/video/v4l2-event.c
create mode 100644 include/media/v4l2-event.h
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 1947146..dd6a853 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,8 @@ stkwebcam-objs:= stk-webcam.o stk-sensor.o
omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o
-videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
+videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
+ v4l2-event.o
# V4L2 core modules
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 15b2ac8..6d25297 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -613,22 +613,36 @@ static int __init videodev_init(void)
dev_t dev = MKDEV(VIDEO_MAJOR, 0);
int ret;
+ ret = v4l2_event_init();
+ if (ret 0) {
+ printk(KERN_WARNING videodev: unable to initialise events\n);
+ return ret;
+ }
+
printk(KERN_INFO Linux video capture interface: v2.00\n);
ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
if (ret 0) {
printk(KERN_WARNING videodev: unable to get major %d\n,
VIDEO_MAJOR);
- return ret;
+ goto out_register_chrdev_region;
}
ret = class_register(video_class);
if (ret 0) {
- unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
printk(KERN_WARNING video_dev: class_register failed\n);
- return -EIO;
+ ret = -EIO;
+ goto out_class_register;
}
return 0;
+
+out_class_register:
+ unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+
+out_register_chrdev_region:
+ v4l2_event_exit();
+
+ return ret;
}
static void __exit videodev_exit(void)
@@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
class_unregister(video_class);
unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+ v4l2_event_exit();
}
module_init(videodev_init)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 000..9fc0c81
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,254 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include media/v4l2-dev.h
+#include media/v4l2-event.h
+
+#include linux/sched.h
+
+static struct kmem_cache *event_kmem;
+
+int v4l2_event_init(void)
+{
+ event_kmem = kmem_cache_create(event_kmem,
+ sizeof(struct _v4l2_event), 0,
+ SLAB_HWCACHE_ALIGN,
+ NULL);
+
+ if (!event_kmem)
+ return -ENOMEM;
+
+ return 0;
+}
+
+void v4l2_event_exit(void)
+{
+ kmem_cache_destroy(event_kmem);
+}
+
+void v4l2_event_init_fh(struct v4l2_fh *fh)
+{
+ struct v4l2_events *events = fh-events;
+
+ init_waitqueue_head(events-wait);
+ spin_lock_init(events-lock);
+
+ INIT_LIST_HEAD(events-available);
+ INIT_LIST_HEAD(events-subscribed);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
+
+void v4l2_event_exit_fh(struct v4l2_fh *fh)
+{
+ struct v4l2_events *events = fh-events;
+
+ while (!list_empty(events-available)) {
+ struct _v4l2_event *ev;
+
+
Re: [RFC v2 4/7] V4L: Events: Add backend
On Tue, 2009-12-22 at 18:43 +0200, Sakari Ailus wrote:
Add event handling backend to V4L2. The backend handles event subscription
and delivery to file handles. Event subscriptions are based on file handle.
Events may be delivered to all subscribed file handles on a device
independent of where they originate from.
Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
---
drivers/media/video/Makefile |3 +-
drivers/media/video/v4l2-dev.c | 21 +++-
drivers/media/video/v4l2-event.c | 254
++
drivers/media/video/v4l2-fh.c|4 +
include/media/v4l2-event.h | 65 ++
include/media/v4l2-fh.h |3 +
6 files changed, 346 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/video/v4l2-event.c
create mode 100644 include/media/v4l2-event.h
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 1947146..dd6a853 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,8 @@ stkwebcam-objs := stk-webcam.o stk-sensor.o
omap2cam-objs:= omap24xxcam.o omap24xxcam-dma.o
-videodev-objs:= v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
+videodev-objs:= v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
\
+ v4l2-event.o
# V4L2 core modules
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 15b2ac8..6d25297 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -613,22 +613,36 @@ static int __init videodev_init(void)
dev_t dev = MKDEV(VIDEO_MAJOR, 0);
int ret;
+ ret = v4l2_event_init();
+ if (ret 0) {
+ printk(KERN_WARNING videodev: unable to initialise events\n);
+ return ret;
+ }
+
printk(KERN_INFO Linux video capture interface: v2.00\n);
ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
if (ret 0) {
printk(KERN_WARNING videodev: unable to get major %d\n,
VIDEO_MAJOR);
- return ret;
+ goto out_register_chrdev_region;
}
ret = class_register(video_class);
if (ret 0) {
- unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
printk(KERN_WARNING video_dev: class_register failed\n);
- return -EIO;
+ ret = -EIO;
+ goto out_class_register;
}
return 0;
+
+out_class_register:
+ unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+
+out_register_chrdev_region:
+ v4l2_event_exit();
+
+ return ret;
}
static void __exit videodev_exit(void)
@@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
class_unregister(video_class);
unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
+ v4l2_event_exit();
}
module_init(videodev_init)
diff --git a/drivers/media/video/v4l2-event.c
b/drivers/media/video/v4l2-event.c
new file mode 100644
index 000..9fc0c81
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,254 @@
+/*
+ * drivers/media/video/v4l2-event.c
+ *
+ * V4L2 events.
+ *
+ * Copyright (C) 2009 Nokia Corporation.
+ *
+ * Contact: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
+ *
+ * This program is free software; you can redistribute it and/or
+ * modify it under the terms of the GNU General Public License
+ * version 2 as published by the Free Software Foundation.
+ *
+ * This program is distributed in the hope that it will be useful, but
+ * WITHOUT ANY WARRANTY; without even the implied warranty of
+ * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ * General Public License for more details.
+ *
+ * You should have received a copy of the GNU General Public License
+ * along with this program; if not, write to the Free Software
+ * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
+ * 02110-1301 USA
+ */
+
+#include media/v4l2-dev.h
+#include media/v4l2-event.h
+
+#include linux/sched.h
+
+static struct kmem_cache *event_kmem;
+
+int v4l2_event_init(void)
+{
+ event_kmem = kmem_cache_create(event_kmem,
+sizeof(struct _v4l2_event), 0,
+SLAB_HWCACHE_ALIGN,
+NULL);
+
+ if (!event_kmem)
+ return -ENOMEM;
+
+ return 0;
+}
+
+void v4l2_event_exit(void)
+{
+ kmem_cache_destroy(event_kmem);
+}
+
+void v4l2_event_init_fh(struct v4l2_fh *fh)
+{
+ struct v4l2_events *events = fh-events;
+
+ init_waitqueue_head(events-wait);
+ spin_lock_init(events-lock);
+
+ INIT_LIST_HEAD(events-available);
+ INIT_LIST_HEAD(events-subscribed);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
+
+void v4l2_event_exit_fh(struct v4l2_fh *fh)
+{
