Re: [RFC v2 5/7] V4L: Events: Limit event queue length
Hans Verkuil wrote: Hi Sakari, Hi Hans, And thanks for the comments! ... @@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event) ev = list_first_entry(events-available, struct _v4l2_event, list); list_del(ev-list); -ev-event.count = !list_empty(events-available); +atomic_dec(events-navailable); +ev-event.count = atomic_read(events-navailable); Combine these two lines to atomic_dec_return(). Will fix this. spin_unlock_irqrestore(events-lock, flags); @@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev) if (!v4l2_event_subscribed(fh, ev-type)) continue; +if (atomic_read(fh-events.navailable) = V4L2_MAX_EVENTS) +continue; + _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC); if (!_ev) continue; @@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev) list_add_tail(_ev-list, fh-events.available); spin_unlock(fh-events.lock); +atomic_inc(fh-events.navailable); + wake_up_all(fh-events.wait); } diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h index b11de92..69305c6 100644 --- a/include/media/v4l2-event.h +++ b/include/media/v4l2-event.h @@ -28,6 +28,10 @@ #include linux/types.h #include linux/videodev2.h +#include asm/atomic.h + +#define V4L2_MAX_EVENTS1024 /* Ought to be enough for everyone. */ I think this should be programmable by the driver. Most drivers do not use events at all, so by default it should be 0 or perhaps it can check whether the ioctl callback structure contains the event ioctls and set it to 0 or some initial default value. Right. I'll make the event queue size to be defined by the driver. I'm now planning to make a queue for free events common to file handles in video device. A statically allocated queue for each file handle is probably too much overkill. But a device global queue also means that a process that doesn't dequeue its events will starve the others. And you want this to be controlled on a per-filehandle basis even. If I look at ivtv, then most of the device nodes will not have events, only a few will support events. And for one device node type I know that there will only be a single event when stopping the streaming, while another device node type will get an event each frame. Instead of initialising the events by the V4L2, the driver could do this and specify the queue size at the same time. The overhead for the drivers not using events would be the event information in the video_device structure. That could be made a pointer as well. So being able to adjust the event queue dynamically will give more control and prevent unnecessary waste of memory resources. This sounds to me like trying to re-invent the kmem_cache now. :-) If the event queue is allocated in some other means than kmem_cache I think the size should be fixed. The driver probably knows the best what's the reasonable maximum event queue size and that could be allocated statically. If that overflows then so be it. Regards, -- Sakari Ailus sakari.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 5/7] V4L: Events: Limit event queue length
Hi Hans, On Monday 18 January 2010 13:58:09 Hans Verkuil wrote: On Tue, 22 Dec 2009, Sakari Ailus wrote: Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any further events will be dropped. This patch also updates the count field properly, setting it to exactly to number of further available events. Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com --- drivers/media/video/v4l2-event.c | 10 +- include/media/v4l2-event.h |5 + 2 files changed, 14 insertions(+), 1 deletions(-) [snip] diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h index b11de92..69305c6 100644 --- a/include/media/v4l2-event.h +++ b/include/media/v4l2-event.h @@ -28,6 +28,10 @@ #include linux/types.h #include linux/videodev2.h +#include asm/atomic.h + +#define V4L2_MAX_EVENTS1024 /* Ought to be enough for everyone. */ I think this should be programmable by the driver. Most drivers do not use events at all, so by default it should be 0 or perhaps it can check whether the ioctl callback structure contains the event ioctls and set it to 0 or some initial default value. And you want this to be controlled on a per-filehandle basis even. If I look at ivtv, then most of the device nodes will not have events, only a few will support events. And for one device node type I know that there will only be a single event when stopping the streaming, while another device node type will get an event each frame. Don't you mean per video node instead of per file handle ? In that case we could add a new field to video_device structure that must be initialized by drivers before registering the device. So being able to adjust the event queue dynamically will give more control and prevent unnecessary waste of memory resources. -- Regards, Laurent Pinchart -- 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 5/7] V4L: Events: Limit event queue length
Hi Hans, On Monday 18 January 2010 13:58:09 Hans Verkuil wrote: On Tue, 22 Dec 2009, Sakari Ailus wrote: Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any further events will be dropped. This patch also updates the count field properly, setting it to exactly to number of further available events. Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com --- drivers/media/video/v4l2-event.c | 10 +- include/media/v4l2-event.h |5 + 2 files changed, 14 insertions(+), 1 deletions(-) [snip] diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h index b11de92..69305c6 100644 --- a/include/media/v4l2-event.h +++ b/include/media/v4l2-event.h @@ -28,6 +28,10 @@ #include linux/types.h #include linux/videodev2.h +#include asm/atomic.h + +#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for everyone. */ I think this should be programmable by the driver. Most drivers do not use events at all, so by default it should be 0 or perhaps it can check whether the ioctl callback structure contains the event ioctls and set it to 0 or some initial default value. And you want this to be controlled on a per-filehandle basis even. If I look at ivtv, then most of the device nodes will not have events, only a few will support events. And for one device node type I know that there will only be a single event when stopping the streaming, while another device node type will get an event each frame. Don't you mean per video node instead of per file handle ? In that case we could add a new field to video_device structure that must be initialized by drivers before registering the device. Yes, that's what I meant (although you state it much more clearly :-) ). Regards, Hans So being able to adjust the event queue dynamically will give more control and prevent unnecessary waste of memory resources. -- Regards, Laurent Pinchart -- Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom -- 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 5/7] V4L: Events: Limit event queue length
Hi Sakari,
More comments:
On Tue, 22 Dec 2009, Sakari Ailus wrote:
Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
further events will be dropped.
This patch also updates the count field properly, setting it to exactly to
number of further available events.
Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
---
drivers/media/video/v4l2-event.c | 10 +-
include/media/v4l2-event.h |5 +
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9fc0c81..72fdf7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
INIT_LIST_HEAD(events-available);
INIT_LIST_HEAD(events-subscribed);
+
+ atomic_set(events-navailable, 0);
}
EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
@@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
v4l2_event *event)
ev = list_first_entry(events-available, struct _v4l2_event, list);
list_del(ev-list);
- ev-event.count = !list_empty(events-available);
+ atomic_dec(events-navailable);
+ ev-event.count = atomic_read(events-navailable);
Combine these two lines to atomic_dec_return().
spin_unlock_irqrestore(events-lock, flags);
@@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct
v4l2_event *ev)
if (!v4l2_event_subscribed(fh, ev-type))
continue;
+ if (atomic_read(fh-events.navailable) = V4L2_MAX_EVENTS)
+ continue;
+
_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
if (!_ev)
continue;
@@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct
v4l2_event *ev)
list_add_tail(_ev-list, fh-events.available);
spin_unlock(fh-events.lock);
+ atomic_inc(fh-events.navailable);
+
wake_up_all(fh-events.wait);
}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index b11de92..69305c6 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,10 @@
#include linux/types.h
#include linux/videodev2.h
+#include asm/atomic.h
+
+#define V4L2_MAX_EVENTS1024 /* Ought to be enough for
everyone. */
I think this should be programmable by the driver. Most drivers do not use
events at all, so by default it should be 0 or perhaps it can check whether
the ioctl callback structure contains the event ioctls and set it to 0 or
some initial default value.
And you want this to be controlled on a per-filehandle basis even. If I look
at ivtv, then most of the device nodes will not have events, only a few will
support events. And for one device node type I know that there will only be
a single event when stopping the streaming, while another device node type
will get an event each frame.
So being able to adjust the event queue dynamically will give more control
and prevent unnecessary waste of memory resources.
Regards,
Hans
+
struct v4l2_fh;
struct video_device;
@@ -39,6 +43,7 @@ struct _v4l2_event {
struct v4l2_events {
spinlock_t lock; /* Protect everything here. */
struct list_headavailable;
+ atomic_tnavailable;
wait_queue_head_t wait;
struct list_headsubscribed; /* Subscribed events. */
};
--
1.5.6.5
--
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 5/7] V4L: Events: Limit event queue length
Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
further events will be dropped.
This patch also updates the count field properly, setting it to exactly to
number of further available events.
Signed-off-by: Sakari Ailus sakari.ai...@maxwell.research.nokia.com
---
drivers/media/video/v4l2-event.c | 10 +-
include/media/v4l2-event.h |5 +
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9fc0c81..72fdf7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
INIT_LIST_HEAD(events-available);
INIT_LIST_HEAD(events-subscribed);
+
+ atomic_set(events-navailable, 0);
}
EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
@@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
v4l2_event *event)
ev = list_first_entry(events-available, struct _v4l2_event, list);
list_del(ev-list);
- ev-event.count = !list_empty(events-available);
+ atomic_dec(events-navailable);
+ ev-event.count = atomic_read(events-navailable);
spin_unlock_irqrestore(events-lock, flags);
@@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct
v4l2_event *ev)
if (!v4l2_event_subscribed(fh, ev-type))
continue;
+ if (atomic_read(fh-events.navailable) = V4L2_MAX_EVENTS)
+ continue;
+
_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
if (!_ev)
continue;
@@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct
v4l2_event *ev)
list_add_tail(_ev-list, fh-events.available);
spin_unlock(fh-events.lock);
+ atomic_inc(fh-events.navailable);
+
wake_up_all(fh-events.wait);
}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index b11de92..69305c6 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,10 @@
#include linux/types.h
#include linux/videodev2.h
+#include asm/atomic.h
+
+#define V4L2_MAX_EVENTS1024 /* Ought to be enough for
everyone. */
+
struct v4l2_fh;
struct video_device;
@@ -39,6 +43,7 @@ struct _v4l2_event {
struct v4l2_events {
spinlock_t lock; /* Protect everything here. */
struct list_headavailable;
+ atomic_tnavailable;
wait_queue_head_t wait;
struct list_headsubscribed; /* Subscribed events. */
};
--
1.5.6.5
--
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
