* [RFC v2 5/7] V4L: Events: Limit event queue length
2009-12-22 16:43 ` [RFC v2 4/7] V4L: Events: Add backend Sakari Ailus
@ 2009-12-22 16:43 ` Sakari Ailus
2009-12-22 16:43 ` [RFC v2 6/7] V4L: Events: Sequence numbers Sakari Ailus
2010-01-18 12:58 ` [RFC v2 5/7] V4L: Events: Limit event queue length Hans Verkuil
2009-12-23 1:01 ` [RFC v2 4/7] V4L: Events: Add backend Andy Walls
2010-01-18 12:43 ` Hans Verkuil
2 siblings, 2 replies; 22+ messages in thread
From: Sakari Ailus @ 2009-12-22 16:43 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, iivanov, hverkuil, gururaj.nagendra
Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
further events will be dropped.
This patch also updates the count field properly, setting it to exactly to
number of further available events.
Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
drivers/media/video/v4l2-event.c | 10 +++++++++-
include/media/v4l2-event.h | 5 +++++
2 files changed, 14 insertions(+), 1 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 9fc0c81..72fdf7f 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
INIT_LIST_HEAD(&events->available);
INIT_LIST_HEAD(&events->subscribed);
+
+ atomic_set(&events->navailable, 0);
}
EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
@@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
ev = list_first_entry(&events->available, struct _v4l2_event, list);
list_del(&ev->list);
- ev->event.count = !list_empty(&events->available);
+ atomic_dec(&events->navailable);
+ ev->event.count = atomic_read(&events->navailable);
spin_unlock_irqrestore(&events->lock, flags);
@@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
if (!v4l2_event_subscribed(fh, ev->type))
continue;
+ if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
+ continue;
+
_ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
if (!_ev)
continue;
@@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
list_add_tail(&_ev->list, &fh->events.available);
spin_unlock(&fh->events.lock);
+ atomic_inc(&fh->events.navailable);
+
wake_up_all(&fh->events.wait);
}
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index b11de92..69305c6 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -28,6 +28,10 @@
#include <linux/types.h>
#include <linux/videodev2.h>
+#include <asm/atomic.h>
+
+#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for everyone. */
+
struct v4l2_fh;
struct video_device;
@@ -39,6 +43,7 @@ struct _v4l2_event {
struct v4l2_events {
spinlock_t lock; /* Protect everything here. */
struct list_head available;
+ atomic_t navailable;
wait_queue_head_t wait;
struct list_head subscribed; /* Subscribed events. */
};
--
1.5.6.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFC v2 6/7] V4L: Events: Sequence numbers
2009-12-22 16:43 ` [RFC v2 5/7] V4L: Events: Limit event queue length Sakari Ailus
@ 2009-12-22 16:43 ` Sakari Ailus
2009-12-22 16:43 ` [RFC v2 7/7] V4L: Events: Support all events Sakari Ailus
2010-01-18 12:48 ` [RFC v2 6/7] V4L: Events: Sequence numbers Hans Verkuil
2010-01-18 12:58 ` [RFC v2 5/7] V4L: Events: Limit event queue length Hans Verkuil
1 sibling, 2 replies; 22+ messages in thread
From: Sakari Ailus @ 2009-12-22 16:43 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, iivanov, hverkuil, gururaj.nagendra
Add sequence numbers to events.
Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
drivers/media/video/v4l2-event.c | 8 ++++++++
include/media/v4l2-event.h | 1 +
2 files changed, 9 insertions(+), 0 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index 72fdf7f..cc2bf57 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -58,6 +58,7 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
INIT_LIST_HEAD(&events->subscribed);
atomic_set(&events->navailable, 0);
+ events->sequence = 0;
}
EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
@@ -158,10 +159,16 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
list_for_each_entry(fh, &vdev->fh, list) {
struct _v4l2_event *_ev;
+ u32 sequence;
if (!v4l2_event_subscribed(fh, ev->type))
continue;
+ spin_lock(&fh->events.lock);
+ sequence = fh->events.sequence;
+ fh->events.sequence++;
+ spin_unlock(&fh->events.lock);
+
if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
continue;
@@ -172,6 +179,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
_ev->event = *ev;
spin_lock(&fh->events.lock);
+ _ev->event.sequence = sequence;
list_add_tail(&_ev->list, &fh->events.available);
spin_unlock(&fh->events.lock);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
index 69305c6..5a778d4 100644
--- a/include/media/v4l2-event.h
+++ b/include/media/v4l2-event.h
@@ -44,6 +44,7 @@ struct v4l2_events {
spinlock_t lock; /* Protect everything here. */
struct list_head available;
atomic_t navailable;
+ u32 sequence;
wait_queue_head_t wait;
struct list_head subscribed; /* Subscribed events. */
};
--
1.5.6.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* [RFC v2 7/7] V4L: Events: Support all events
2009-12-22 16:43 ` [RFC v2 6/7] V4L: Events: Sequence numbers Sakari Ailus
@ 2009-12-22 16:43 ` Sakari Ailus
2010-01-18 12:48 ` [RFC v2 6/7] V4L: Events: Sequence numbers Hans Verkuil
1 sibling, 0 replies; 22+ messages in thread
From: Sakari Ailus @ 2009-12-22 16:43 UTC (permalink / raw)
To: linux-media; +Cc: laurent.pinchart, iivanov, hverkuil, gururaj.nagendra
Add support for subscribing V4L2_EVENT_ALL. After V4L2_EVENT_ALL is
subscribed, unsubscribing any event leads to unsubscription of all events.
Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
drivers/media/video/v4l2-event.c | 34 ++++++++++++++++++++++++----------
1 files changed, 24 insertions(+), 10 deletions(-)
diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
index cc2bf57..95b3917 100644
--- a/drivers/media/video/v4l2-event.c
+++ b/drivers/media/video/v4l2-event.c
@@ -62,6 +62,22 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
}
EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
+static void __v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
+{
+ struct v4l2_events *events = &fh->events;
+
+ while (!list_empty(&events->subscribed)) {
+ struct v4l2_subscribed_event *sub;
+
+ sub = list_entry(events->subscribed.next,
+ struct v4l2_subscribed_event, list);
+
+ list_del(&sub->list);
+
+ kfree(sub);
+ }
+}
+
void v4l2_event_exit_fh(struct v4l2_fh *fh)
{
struct v4l2_events *events = &fh->events;
@@ -77,16 +93,7 @@ void v4l2_event_exit_fh(struct v4l2_fh *fh)
kmem_cache_free(event_kmem, ev);
}
- while (!list_empty(&events->subscribed)) {
- struct v4l2_subscribed_event *sub;
-
- sub = list_entry(events->subscribed.next,
- struct v4l2_subscribed_event, list);
-
- list_del(&sub->list);
-
- kfree(sub);
- }
+ __v4l2_event_unsubscribe_all(fh);
}
EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
@@ -125,6 +132,11 @@ static struct v4l2_subscribed_event *__v4l2_event_subscribed(
struct v4l2_events *events = &fh->events;
struct v4l2_subscribed_event *ev;
+ ev = container_of(events->subscribed.next,
+ struct v4l2_subscribed_event, list);
+ if (ev->type == V4L2_EVENT_ALL)
+ return ev;
+
list_for_each_entry(ev, &events->subscribed, list) {
if (ev->type == type)
return ev;
@@ -237,6 +249,8 @@ int v4l2_event_subscribe(struct v4l2_fh *fh,
INIT_LIST_HEAD(&ev->list);
ev->type = sub->type;
+ if (ev->type == V4L2_EVENT_ALL)
+ __v4l2_event_unsubscribe_all(fh);
list_add(&ev->list, &events->subscribed);
out:
--
1.5.6.5
^ permalink raw reply related [flat|nested] 22+ messages in thread* Re: [RFC v2 6/7] V4L: Events: Sequence numbers
2009-12-22 16:43 ` [RFC v2 6/7] V4L: Events: Sequence numbers Sakari Ailus
2009-12-22 16:43 ` [RFC v2 7/7] V4L: Events: Support all events Sakari Ailus
@ 2010-01-18 12:48 ` Hans Verkuil
1 sibling, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2010-01-18 12:48 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, iivanov, gururaj.nagendra
Hi Sakari,
Some more review comments:
On Tue, 22 Dec 2009, Sakari Ailus wrote:
> Add sequence numbers to events.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/v4l2-event.c | 8 ++++++++
> include/media/v4l2-event.h | 1 +
> 2 files changed, 9 insertions(+), 0 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 72fdf7f..cc2bf57 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -58,6 +58,7 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
> INIT_LIST_HEAD(&events->subscribed);
>
> atomic_set(&events->navailable, 0);
> + events->sequence = 0;
Why not make this atomic_t as well?
> }
> EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
>
> @@ -158,10 +159,16 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
>
> list_for_each_entry(fh, &vdev->fh, list) {
> struct _v4l2_event *_ev;
> + u32 sequence;
>
> if (!v4l2_event_subscribed(fh, ev->type))
> continue;
>
> + spin_lock(&fh->events.lock);
> + sequence = fh->events.sequence;
> + fh->events.sequence++;
> + spin_unlock(&fh->events.lock);
Then you can use atomic_inc_return() here.
> +
> if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
> continue;
>
> @@ -172,6 +179,7 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> _ev->event = *ev;
>
> spin_lock(&fh->events.lock);
> + _ev->event.sequence = sequence;
> list_add_tail(&_ev->list, &fh->events.available);
> spin_unlock(&fh->events.lock);
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index 69305c6..5a778d4 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -44,6 +44,7 @@ struct v4l2_events {
> spinlock_t lock; /* Protect everything here. */
> struct list_head available;
> atomic_t navailable;
> + u32 sequence;
> wait_queue_head_t wait;
> struct list_head subscribed; /* Subscribed events. */
> };
> --
> 1.5.6.5
>
Regards,
Hans
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC v2 5/7] V4L: Events: Limit event queue length
2009-12-22 16:43 ` [RFC v2 5/7] V4L: Events: Limit event queue length Sakari Ailus
2009-12-22 16:43 ` [RFC v2 6/7] V4L: Events: Sequence numbers Sakari Ailus
@ 2010-01-18 12:58 ` Hans Verkuil
2010-01-19 8:11 ` Laurent Pinchart
2010-01-24 13:06 ` Sakari Ailus
1 sibling, 2 replies; 22+ messages in thread
From: Hans Verkuil @ 2010-01-18 12:58 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, iivanov, gururaj.nagendra
Hi Sakari,
More comments:
On Tue, 22 Dec 2009, Sakari Ailus wrote:
> Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
> further events will be dropped.
>
> This patch also updates the count field properly, setting it to exactly to
> number of further available events.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/v4l2-event.c | 10 +++++++++-
> include/media/v4l2-event.h | 5 +++++
> 2 files changed, 14 insertions(+), 1 deletions(-)
>
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> index 9fc0c81..72fdf7f 100644
> --- a/drivers/media/video/v4l2-event.c
> +++ b/drivers/media/video/v4l2-event.c
> @@ -56,6 +56,8 @@ void v4l2_event_init_fh(struct v4l2_fh *fh)
>
> INIT_LIST_HEAD(&events->available);
> INIT_LIST_HEAD(&events->subscribed);
> +
> + atomic_set(&events->navailable, 0);
> }
> EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
>
> @@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> ev = list_first_entry(&events->available, struct _v4l2_event, list);
> list_del(&ev->list);
>
> - ev->event.count = !list_empty(&events->available);
> + atomic_dec(&events->navailable);
> + ev->event.count = atomic_read(&events->navailable);
Combine these two lines to atomic_dec_return().
>
> spin_unlock_irqrestore(&events->lock, flags);
>
> @@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> if (!v4l2_event_subscribed(fh, ev->type))
> continue;
>
> + if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
> + continue;
> +
> _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> if (!_ev)
> continue;
> @@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> list_add_tail(&_ev->list, &fh->events.available);
> spin_unlock(&fh->events.lock);
>
> + atomic_inc(&fh->events.navailable);
> +
> wake_up_all(&fh->events.wait);
> }
>
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> index b11de92..69305c6 100644
> --- a/include/media/v4l2-event.h
> +++ b/include/media/v4l2-event.h
> @@ -28,6 +28,10 @@
> #include <linux/types.h>
> #include <linux/videodev2.h>
>
> +#include <asm/atomic.h>
> +
> +#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for everyone. */
I think this should be programmable by the driver. Most drivers do not use
events at all, so by default it should be 0 or perhaps it can check whether
the ioctl callback structure contains the event ioctls and set it to 0 or
some initial default value.
And you want this to be controlled on a per-filehandle basis even. If I look
at ivtv, then most of the device nodes will not have events, only a few will
support events. And for one device node type I know that there will only be
a single event when stopping the streaming, while another device node type
will get an event each frame.
So being able to adjust the event queue dynamically will give more control
and prevent unnecessary waste of memory resources.
Regards,
Hans
> +
> struct v4l2_fh;
> struct video_device;
>
> @@ -39,6 +43,7 @@ struct _v4l2_event {
> struct v4l2_events {
> spinlock_t lock; /* Protect everything here. */
> struct list_head available;
> + atomic_t navailable;
> wait_queue_head_t wait;
> struct list_head subscribed; /* Subscribed events. */
> };
> --
> 1.5.6.5
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFC v2 5/7] V4L: Events: Limit event queue length
2010-01-18 12:58 ` [RFC v2 5/7] V4L: Events: Limit event queue length Hans Verkuil
@ 2010-01-19 8:11 ` Laurent Pinchart
2010-01-19 9:03 ` Hans Verkuil
2010-01-24 13:06 ` Sakari Ailus
1 sibling, 1 reply; 22+ messages in thread
From: Laurent Pinchart @ 2010-01-19 8:11 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Sakari Ailus, linux-media, iivanov, gururaj.nagendra
Hi Hans,
On Monday 18 January 2010 13:58:09 Hans Verkuil wrote:
> On Tue, 22 Dec 2009, Sakari Ailus wrote:
> > Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
> > further events will be dropped.
> >
> > This patch also updates the count field properly, setting it to exactly
> > to number of further available events.
> >
> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > ---
> > drivers/media/video/v4l2-event.c | 10 +++++++++-
> > include/media/v4l2-event.h | 5 +++++
> > 2 files changed, 14 insertions(+), 1 deletions(-)
[snip]
> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> > index b11de92..69305c6 100644
> > --- a/include/media/v4l2-event.h
> > +++ b/include/media/v4l2-event.h
> > @@ -28,6 +28,10 @@
> > #include <linux/types.h>
> > #include <linux/videodev2.h>
> >
> > +#include <asm/atomic.h>
> > +
> > +#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for everyone. */
>
> I think this should be programmable by the driver. Most drivers do not use
> events at all, so by default it should be 0 or perhaps it can check whether
> the ioctl callback structure contains the event ioctls and set it to 0 or
> some initial default value.
>
> And you want this to be controlled on a per-filehandle basis even. If I
> look at ivtv, then most of the device nodes will not have events, only a
> few will support events. And for one device node type I know that there
> will only be a single event when stopping the streaming, while another
> device node type will get an event each frame.
Don't you mean per video node instead of per file handle ? In that case we
could add a new field to video_device structure that must be initialized by
drivers before registering the device.
> So being able to adjust the event queue dynamically will give more control
> and prevent unnecessary waste of memory resources.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC v2 5/7] V4L: Events: Limit event queue length
2010-01-19 8:11 ` Laurent Pinchart
@ 2010-01-19 9:03 ` Hans Verkuil
0 siblings, 0 replies; 22+ messages in thread
From: Hans Verkuil @ 2010-01-19 9:03 UTC (permalink / raw)
To: Laurent Pinchart; +Cc: Sakari Ailus, linux-media, iivanov, gururaj.nagendra
> Hi Hans,
>
> On Monday 18 January 2010 13:58:09 Hans Verkuil wrote:
>> On Tue, 22 Dec 2009, Sakari Ailus wrote:
>> > Limit event queue length to V4L2_MAX_EVENTS. If the queue is full any
>> > further events will be dropped.
>> >
>> > This patch also updates the count field properly, setting it to
>> exactly
>> > to number of further available events.
>> >
>> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> > ---
>> > drivers/media/video/v4l2-event.c | 10 +++++++++-
>> > include/media/v4l2-event.h | 5 +++++
>> > 2 files changed, 14 insertions(+), 1 deletions(-)
>
> [snip]
>
>> > diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
>> > index b11de92..69305c6 100644
>> > --- a/include/media/v4l2-event.h
>> > +++ b/include/media/v4l2-event.h
>> > @@ -28,6 +28,10 @@
>> > #include <linux/types.h>
>> > #include <linux/videodev2.h>
>> >
>> > +#include <asm/atomic.h>
>> > +
>> > +#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for everyone. */
>>
>> I think this should be programmable by the driver. Most drivers do not
>> use
>> events at all, so by default it should be 0 or perhaps it can check
>> whether
>> the ioctl callback structure contains the event ioctls and set it to 0
>> or
>> some initial default value.
>>
>> And you want this to be controlled on a per-filehandle basis even. If I
>> look at ivtv, then most of the device nodes will not have events, only
>> a
>> few will support events. And for one device node type I know that there
>> will only be a single event when stopping the streaming, while another
>> device node type will get an event each frame.
>
> Don't you mean per video node instead of per file handle ? In that case we
> could add a new field to video_device structure that must be initialized
> by
> drivers before registering the device.
Yes, that's what I meant (although you state it much more clearly :-) ).
Regards,
Hans
>> So being able to adjust the event queue dynamically will give more
>> control
>> and prevent unnecessary waste of memory resources.
>
> --
> Regards,
>
> Laurent Pinchart
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC v2 5/7] V4L: Events: Limit event queue length
2010-01-18 12:58 ` [RFC v2 5/7] V4L: Events: Limit event queue length Hans Verkuil
2010-01-19 8:11 ` Laurent Pinchart
@ 2010-01-24 13:06 ` Sakari Ailus
1 sibling, 0 replies; 22+ messages in thread
From: Sakari Ailus @ 2010-01-24 13:06 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, laurent.pinchart, iivanov, gururaj.nagendra
Hans Verkuil wrote:
> Hi Sakari,
Hi Hans,
And thanks for the comments!
...
>> @@ -103,7 +105,8 @@ int v4l2_event_dequeue(struct v4l2_fh *fh, struct
>> v4l2_event *event)
>> ev = list_first_entry(&events->available, struct _v4l2_event, list);
>> list_del(&ev->list);
>>
>> - ev->event.count = !list_empty(&events->available);
>> + atomic_dec(&events->navailable);
>> + ev->event.count = atomic_read(&events->navailable);
>
> Combine these two lines to atomic_dec_return().
Will fix this.
>>
>> spin_unlock_irqrestore(&events->lock, flags);
>>
>> @@ -159,6 +162,9 @@ void v4l2_event_queue(struct video_device *vdev,
>> struct v4l2_event *ev)
>> if (!v4l2_event_subscribed(fh, ev->type))
>> continue;
>>
>> + if (atomic_read(&fh->events.navailable) >= V4L2_MAX_EVENTS)
>> + continue;
>> +
>> _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
>> if (!_ev)
>> continue;
>> @@ -169,6 +175,8 @@ void v4l2_event_queue(struct video_device *vdev,
>> struct v4l2_event *ev)
>> list_add_tail(&_ev->list, &fh->events.available);
>> spin_unlock(&fh->events.lock);
>>
>> + atomic_inc(&fh->events.navailable);
>> +
>> wake_up_all(&fh->events.wait);
>> }
>>
>> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
>> index b11de92..69305c6 100644
>> --- a/include/media/v4l2-event.h
>> +++ b/include/media/v4l2-event.h
>> @@ -28,6 +28,10 @@
>> #include <linux/types.h>
>> #include <linux/videodev2.h>
>>
>> +#include <asm/atomic.h>
>> +
>> +#define V4L2_MAX_EVENTS 1024 /* Ought to be enough for
>> everyone. */
>
> I think this should be programmable by the driver. Most drivers do not use
> events at all, so by default it should be 0 or perhaps it can check whether
> the ioctl callback structure contains the event ioctls and set it to 0 or
> some initial default value.
Right. I'll make the event queue size to be defined by the driver.
I'm now planning to make a queue for free events common to file handles
in video device. A statically allocated queue for each file handle is
probably too much overkill. But a device global queue also means that a
process that doesn't dequeue its events will starve the others.
> And you want this to be controlled on a per-filehandle basis even. If I
> look
> at ivtv, then most of the device nodes will not have events, only a few
> will
> support events. And for one device node type I know that there will only be
> a single event when stopping the streaming, while another device node type
> will get an event each frame.
Instead of initialising the events by the V4L2, the driver could do this
and specify the queue size at the same time. The overhead for the
drivers not using events would be the event information in the
video_device structure. That could be made a pointer as well.
> So being able to adjust the event queue dynamically will give more control
> and prevent unnecessary waste of memory resources.
This sounds to me like trying to re-invent the kmem_cache now. :-)
If the event queue is allocated in some other means than kmem_cache I
think the size should be fixed. The driver probably knows the best
what's the reasonable maximum event queue size and that could be
allocated statically. If that overflows then so be it.
Regards,
--
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC v2 4/7] V4L: Events: Add backend
2009-12-22 16:43 ` [RFC v2 4/7] V4L: Events: Add backend Sakari Ailus
2009-12-22 16:43 ` [RFC v2 5/7] V4L: Events: Limit event queue length Sakari Ailus
@ 2009-12-23 1:01 ` Andy Walls
2009-12-23 10:15 ` Sakari Ailus
2010-01-18 12:43 ` Hans Verkuil
2 siblings, 1 reply; 22+ messages in thread
From: Andy Walls @ 2009-12-23 1:01 UTC (permalink / raw)
To: Sakari Ailus
Cc: linux-media, laurent.pinchart, iivanov, hverkuil,
gururaj.nagendra
On Tue, 2009-12-22 at 18:43 +0200, Sakari Ailus wrote:
> Add event handling backend to V4L2. The backend handles event subscription
> and delivery to file handles. Event subscriptions are based on file handle.
> Events may be delivered to all subscribed file handles on a device
> independent of where they originate from.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/Makefile | 3 +-
> drivers/media/video/v4l2-dev.c | 21 +++-
> drivers/media/video/v4l2-event.c | 254 ++++++++++++++++++++++++++++++++++++++
> drivers/media/video/v4l2-fh.c | 4 +
> include/media/v4l2-event.h | 65 ++++++++++
> include/media/v4l2-fh.h | 3 +
> 6 files changed, 346 insertions(+), 4 deletions(-)
> create mode 100644 drivers/media/video/v4l2-event.c
> create mode 100644 include/media/v4l2-event.h
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 1947146..dd6a853 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -10,7 +10,8 @@ stkwebcam-objs := stk-webcam.o stk-sensor.o
>
> omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o
>
> -videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
> +videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> + v4l2-event.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 15b2ac8..6d25297 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -613,22 +613,36 @@ static int __init videodev_init(void)
> dev_t dev = MKDEV(VIDEO_MAJOR, 0);
> int ret;
>
> + ret = v4l2_event_init();
> + if (ret < 0) {
> + printk(KERN_WARNING "videodev: unable to initialise events\n");
> + return ret;
> + }
> +
> printk(KERN_INFO "Linux video capture interface: v2.00\n");
> ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
> if (ret < 0) {
> printk(KERN_WARNING "videodev: unable to get major %d\n",
> VIDEO_MAJOR);
> - return ret;
> + goto out_register_chrdev_region;
> }
>
> ret = class_register(&video_class);
> if (ret < 0) {
> - unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> printk(KERN_WARNING "video_dev: class_register failed\n");
> - return -EIO;
> + ret = -EIO;
> + goto out_class_register;
> }
>
> return 0;
> +
> +out_class_register:
> + unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +
> +out_register_chrdev_region:
> + v4l2_event_exit();
> +
> + return ret;
> }
>
> static void __exit videodev_exit(void)
> @@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
>
> class_unregister(&video_class);
> unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> + v4l2_event_exit();
> }
>
> module_init(videodev_init)
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..9fc0c81
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,254 @@
> +/*
> + * drivers/media/video/v4l2-event.c
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +static struct kmem_cache *event_kmem;
> +
> +int v4l2_event_init(void)
> +{
> + event_kmem = kmem_cache_create("event_kmem",
> + sizeof(struct _v4l2_event), 0,
> + SLAB_HWCACHE_ALIGN,
> + NULL);
> +
> + if (!event_kmem)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +void v4l2_event_exit(void)
> +{
> + kmem_cache_destroy(event_kmem);
> +}
> +
> +void v4l2_event_init_fh(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> +
> + init_waitqueue_head(&events->wait);
> + spin_lock_init(&events->lock);
> +
> + INIT_LIST_HEAD(&events->available);
> + INIT_LIST_HEAD(&events->subscribed);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
> +
> +void v4l2_event_exit_fh(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> +
> + while (!list_empty(&events->available)) {
> + struct _v4l2_event *ev;
> +
> + ev = list_entry(events->available.next,
> + struct _v4l2_event, list);
> +
> + list_del(&ev->list);
> +
> + kmem_cache_free(event_kmem, ev);
> + }
> +
> + while (!list_empty(&events->subscribed)) {
> + struct v4l2_subscribed_event *sub;
> +
> + sub = list_entry(events->subscribed.next,
> + struct v4l2_subscribed_event, list);
> +
> + list_del(&sub->list);
> +
> + kfree(sub);
> + }
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct _v4l2_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + if (list_empty(&events->available)) {
> + spin_unlock_irqrestore(&events->lock, flags);
> + return -ENOENT;
> + }
> +
> + ev = list_first_entry(&events->available, struct _v4l2_event, list);
> + list_del(&ev->list);
> +
> + ev->event.count = !list_empty(&events->available);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + memcpy(event, &ev->event, sizeof(ev->event));
> +
> + kmem_cache_free(event_kmem, ev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
> +
> +static struct v4l2_subscribed_event *__v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> +
> + list_for_each_entry(ev, &events->subscribed, list) {
> + if (ev->type == type)
> + return ev;
> + }
> +
> + return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + ev = __v4l2_event_subscribed(fh, type);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribed);
> +
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> +{
> + struct v4l2_fh *fh;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> + list_for_each_entry(fh, &vdev->fh, list) {
> + struct _v4l2_event *_ev;
> +
> + if (!v4l2_event_subscribed(fh, ev->type))
> + continue;
> +
> + _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> + if (!_ev)
> + continue;
> +
> + _ev->event = *ev;
> +
> + spin_lock(&fh->events.lock);
> + list_add_tail(&_ev->list, &fh->events.available);
> + spin_unlock(&fh->events.lock);
> +
> + wake_up_all(&fh->events.wait);
> + }
> +
> + spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&events->lock, flags);
> + ret = !list_empty(&events->available);
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
Hi Sakari,
Disabling and restoring local interrupts to check if any events are
pending seems excessive.
Since you added an atomic_t with the number of events available in patch
5/7, why don't you just check that atomic_t here?
Regards,
Andy
> +
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> + int ret = 0;
> +
> + /* Allow subscribing to valid events only. */
> + if (sub->type < V4L2_EVENT_PRIVATE_START)
> + switch (sub->type) {
> + case V4L2_EVENT_ALL:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> + if (!ev)
> + return -ENOMEM;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + INIT_LIST_HEAD(&ev->list);
> + ev->type = sub->type;
> +
> + list_add(&ev->list, &events->subscribed);
> +
> +out:
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + if (ret)
> + kfree(ev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + ev = __v4l2_event_subscribed(fh, sub->type);
> +
> + if (ev != NULL)
> + list_del(&ev->list);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ev == NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 406e4ac..2b25ec9 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -32,6 +32,8 @@ int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
> {
> unsigned long flags;
>
> + v4l2_event_init_fh(fh);
> +
> spin_lock_irqsave(&vdev->fh_lock, flags);
> list_add(&fh->list, &vdev->fh);
> spin_unlock_irqrestore(&vdev->fh_lock, flags);
> @@ -44,6 +46,8 @@ void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
> {
> unsigned long flags;
>
> + v4l2_event_exit_fh(fh);
> +
> spin_lock_irqsave(&vdev->fh_lock, flags);
> list_del(&fh->list);
> spin_unlock_irqrestore(&vdev->fh_lock, flags);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..b11de92
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,65 @@
> +/*
> + * include/media/v4l2-event.h
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef V4L2_EVENT_H
> +#define V4L2_EVENT_H
> +
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +
> +struct v4l2_fh;
> +struct video_device;
> +
> +struct _v4l2_event {
> + struct list_head list;
> + struct v4l2_event event;
> +};
> +
> +struct v4l2_events {
> + spinlock_t lock; /* Protect everything here. */
> + struct list_head available;
> + wait_queue_head_t wait;
> + struct list_head subscribed; /* Subscribed events. */
> +};
> +
> +struct v4l2_subscribed_event {
> + struct list_head list;
> + u32 type;
> +};
> +
> +int v4l2_event_init(void);
> +void v4l2_event_exit(void);
> +void v4l2_event_init_fh(struct v4l2_fh *fh);
> +void v4l2_event_exit_fh(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type);
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
> +int v4l2_event_pending(struct v4l2_fh *fh);
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub);
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub);
> +
> +#endif /* V4L2_EVENT_H */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index 1efa916..c15bd13 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,8 +28,11 @@
> #include <linux/types.h>
> #include <linux/list.h>
>
> +#include <media/v4l2-event.h>
> +
> struct v4l2_fh {
> struct list_head list;
> + struct v4l2_events events; /* events, pending and subscribed */
> };
>
> struct video_device;
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFC v2 4/7] V4L: Events: Add backend
2009-12-23 1:01 ` [RFC v2 4/7] V4L: Events: Add backend Andy Walls
@ 2009-12-23 10:15 ` Sakari Ailus
0 siblings, 0 replies; 22+ messages in thread
From: Sakari Ailus @ 2009-12-23 10:15 UTC (permalink / raw)
To: Andy Walls
Cc: linux-media, laurent.pinchart, iivanov, hverkuil,
gururaj.nagendra
Hi Andy,
Andy Walls wrote:
>> +int v4l2_event_pending(struct v4l2_fh *fh)
>> +{
>> + struct v4l2_events *events =&fh->events;
>> + unsigned long flags;
>> + int ret;
>> +
>> + spin_lock_irqsave(&events->lock, flags);
>> + ret = !list_empty(&events->available);
>> + spin_unlock_irqrestore(&events->lock, flags);
>> +
>> + return ret;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
>
> Hi Sakari,
>
> Disabling and restoring local interrupts to check if any events are
> pending seems excessive.
>
> Since you added an atomic_t with the number of events available in patch
> 5/7, why don't you just check that atomic_t here?
Thanks for the comments!
I'll put the fix to patch 5.
--
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com
^ permalink raw reply [flat|nested] 22+ messages in thread
* Re: [RFC v2 4/7] V4L: Events: Add backend
2009-12-22 16:43 ` [RFC v2 4/7] V4L: Events: Add backend Sakari Ailus
2009-12-22 16:43 ` [RFC v2 5/7] V4L: Events: Limit event queue length Sakari Ailus
2009-12-23 1:01 ` [RFC v2 4/7] V4L: Events: Add backend Andy Walls
@ 2010-01-18 12:43 ` Hans Verkuil
2010-01-22 17:10 ` Sakari Ailus
2 siblings, 1 reply; 22+ messages in thread
From: Hans Verkuil @ 2010-01-18 12:43 UTC (permalink / raw)
To: Sakari Ailus; +Cc: linux-media, laurent.pinchart, iivanov, gururaj.nagendra
Hi Sakari,
The code looks good, but I'm not so sure about the use of kmem_cache_*. It
seems serious overkill to me.
Most of the time there will only be a handful of event messages pending. So
setting up a kmem_cache for this seems unnecessary to me.
A much better way to ensure fast event reporting IMHO would be to keep a list
of empty event messages rather than destroying an event after it is dequeued.
So you have two queues per filehandle: pending and empty; initially both are
empty. When a new event has to be queued the code checks if there are events
available for reuse in the empty queue, and if not a new event struct is
allocated and added to the pending queue.
When userspace dequeues the event the kernel will not destroy the event struct
but requeue it in the empty queue for reuse. This is also an ideal place to
check for queue overflowing (which is definitely needed).
In general allocating memory is a slow operation, so not freeing that memory
will speed up things. Drivers might even preallocate a certain number of events
in the empty queue if the last drop of performance is needed.
But right now I don't think there is any need for that.
Regards,
Hans
On Tue, 22 Dec 2009, Sakari Ailus wrote:
> Add event handling backend to V4L2. The backend handles event subscription
> and delivery to file handles. Event subscriptions are based on file handle.
> Events may be delivered to all subscribed file handles on a device
> independent of where they originate from.
>
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
> drivers/media/video/Makefile | 3 +-
> drivers/media/video/v4l2-dev.c | 21 +++-
> drivers/media/video/v4l2-event.c | 254 ++++++++++++++++++++++++++++++++++++++
> drivers/media/video/v4l2-fh.c | 4 +
> include/media/v4l2-event.h | 65 ++++++++++
> include/media/v4l2-fh.h | 3 +
> 6 files changed, 346 insertions(+), 4 deletions(-)
> create mode 100644 drivers/media/video/v4l2-event.c
> create mode 100644 include/media/v4l2-event.h
>
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 1947146..dd6a853 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -10,7 +10,8 @@ stkwebcam-objs := stk-webcam.o stk-sensor.o
>
> omap2cam-objs := omap24xxcam.o omap24xxcam-dma.o
>
> -videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
> +videodev-objs := v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o \
> + v4l2-event.o
>
> # V4L2 core modules
>
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
> index 15b2ac8..6d25297 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -613,22 +613,36 @@ static int __init videodev_init(void)
> dev_t dev = MKDEV(VIDEO_MAJOR, 0);
> int ret;
>
> + ret = v4l2_event_init();
> + if (ret < 0) {
> + printk(KERN_WARNING "videodev: unable to initialise events\n");
> + return ret;
> + }
> +
> printk(KERN_INFO "Linux video capture interface: v2.00\n");
> ret = register_chrdev_region(dev, VIDEO_NUM_DEVICES, VIDEO_NAME);
> if (ret < 0) {
> printk(KERN_WARNING "videodev: unable to get major %d\n",
> VIDEO_MAJOR);
> - return ret;
> + goto out_register_chrdev_region;
> }
>
> ret = class_register(&video_class);
> if (ret < 0) {
> - unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> printk(KERN_WARNING "video_dev: class_register failed\n");
> - return -EIO;
> + ret = -EIO;
> + goto out_class_register;
> }
>
> return 0;
> +
> +out_class_register:
> + unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> +
> +out_register_chrdev_region:
> + v4l2_event_exit();
> +
> + return ret;
> }
>
> static void __exit videodev_exit(void)
> @@ -637,6 +651,7 @@ static void __exit videodev_exit(void)
>
> class_unregister(&video_class);
> unregister_chrdev_region(dev, VIDEO_NUM_DEVICES);
> + v4l2_event_exit();
> }
>
> module_init(videodev_init)
> diff --git a/drivers/media/video/v4l2-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..9fc0c81
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,254 @@
> +/*
> + * drivers/media/video/v4l2-event.c
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +static struct kmem_cache *event_kmem;
> +
> +int v4l2_event_init(void)
> +{
> + event_kmem = kmem_cache_create("event_kmem",
> + sizeof(struct _v4l2_event), 0,
> + SLAB_HWCACHE_ALIGN,
> + NULL);
> +
> + if (!event_kmem)
> + return -ENOMEM;
> +
> + return 0;
> +}
> +
> +void v4l2_event_exit(void)
> +{
> + kmem_cache_destroy(event_kmem);
> +}
> +
> +void v4l2_event_init_fh(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> +
> + init_waitqueue_head(&events->wait);
> + spin_lock_init(&events->lock);
> +
> + INIT_LIST_HEAD(&events->available);
> + INIT_LIST_HEAD(&events->subscribed);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_init_fh);
> +
> +void v4l2_event_exit_fh(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> +
> + while (!list_empty(&events->available)) {
> + struct _v4l2_event *ev;
> +
> + ev = list_entry(events->available.next,
> + struct _v4l2_event, list);
> +
> + list_del(&ev->list);
> +
> + kmem_cache_free(event_kmem, ev);
> + }
> +
> + while (!list_empty(&events->subscribed)) {
> + struct v4l2_subscribed_event *sub;
> +
> + sub = list_entry(events->subscribed.next,
> + struct v4l2_subscribed_event, list);
> +
> + list_del(&sub->list);
> +
> + kfree(sub);
> + }
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_exit_fh);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct _v4l2_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + if (list_empty(&events->available)) {
> + spin_unlock_irqrestore(&events->lock, flags);
> + return -ENOENT;
> + }
> +
> + ev = list_first_entry(&events->available, struct _v4l2_event, list);
> + list_del(&ev->list);
> +
> + ev->event.count = !list_empty(&events->available);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + memcpy(event, &ev->event, sizeof(ev->event));
> +
> + kmem_cache_free(event_kmem, ev);
> +
> + return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
> +
> +static struct v4l2_subscribed_event *__v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> +
> + list_for_each_entry(ev, &events->subscribed, list) {
> + if (ev->type == type)
> + return ev;
> + }
> +
> + return NULL;
> +}
> +
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + ev = __v4l2_event_subscribed(fh, type);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ev;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribed);
> +
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev)
> +{
> + struct v4l2_fh *fh;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> + list_for_each_entry(fh, &vdev->fh, list) {
> + struct _v4l2_event *_ev;
> +
> + if (!v4l2_event_subscribed(fh, ev->type))
> + continue;
> +
> + _ev = kmem_cache_alloc(event_kmem, GFP_ATOMIC);
> + if (!_ev)
> + continue;
> +
> + _ev->event = *ev;
> +
> + spin_lock(&fh->events.lock);
> + list_add_tail(&_ev->list, &fh->events.available);
> + spin_unlock(&fh->events.lock);
> +
> + wake_up_all(&fh->events.wait);
> + }
> +
> + spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> + struct v4l2_events *events = &fh->events;
> + unsigned long flags;
> + int ret;
> +
> + spin_lock_irqsave(&events->lock, flags);
> + ret = !list_empty(&events->available);
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_pending);
> +
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> + int ret = 0;
> +
> + /* Allow subscribing to valid events only. */
> + if (sub->type < V4L2_EVENT_PRIVATE_START)
> + switch (sub->type) {
> + case V4L2_EVENT_ALL:
> + break;
> + default:
> + return -EINVAL;
> + }
> +
> + ev = kmalloc(sizeof(*ev), GFP_KERNEL);
> + if (!ev)
> + return -ENOMEM;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + if (__v4l2_event_subscribed(fh, sub->type) != NULL) {
> + ret = -EBUSY;
> + goto out;
> + }
> +
> + INIT_LIST_HEAD(&ev->list);
> + ev->type = sub->type;
> +
> + list_add(&ev->list, &events->subscribed);
> +
> +out:
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + if (ret)
> + kfree(ev);
> +
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub)
> +{
> + struct v4l2_events *events = &fh->events;
> + struct v4l2_subscribed_event *ev;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&events->lock, flags);
> +
> + ev = __v4l2_event_subscribed(fh, sub->type);
> +
> + if (ev != NULL)
> + list_del(&ev->list);
> +
> + spin_unlock_irqrestore(&events->lock, flags);
> +
> + return ev == NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index 406e4ac..2b25ec9 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -32,6 +32,8 @@ int v4l2_fh_add(struct video_device *vdev, struct v4l2_fh *fh)
> {
> unsigned long flags;
>
> + v4l2_event_init_fh(fh);
> +
> spin_lock_irqsave(&vdev->fh_lock, flags);
> list_add(&fh->list, &vdev->fh);
> spin_unlock_irqrestore(&vdev->fh_lock, flags);
> @@ -44,6 +46,8 @@ void v4l2_fh_del(struct video_device *vdev, struct v4l2_fh *fh)
> {
> unsigned long flags;
>
> + v4l2_event_exit_fh(fh);
> +
> spin_lock_irqsave(&vdev->fh_lock, flags);
> list_del(&fh->list);
> spin_unlock_irqrestore(&vdev->fh_lock, flags);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..b11de92
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,65 @@
> +/*
> + * include/media/v4l2-event.h
> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.
> + *
> + * Contact: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> + *
> + * This program is free software; you can redistribute it and/or
> + * modify it under the terms of the GNU General Public License
> + * version 2 as published by the Free Software Foundation.
> + *
> + * This program is distributed in the hope that it will be useful, but
> + * WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
> + * General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program; if not, write to the Free Software
> + * Foundation, Inc., 51 Franklin St, Fifth Floor, Boston, MA
> + * 02110-1301 USA
> + */
> +
> +#ifndef V4L2_EVENT_H
> +#define V4L2_EVENT_H
> +
> +#include <linux/types.h>
> +#include <linux/videodev2.h>
> +
> +struct v4l2_fh;
> +struct video_device;
> +
> +struct _v4l2_event {
> + struct list_head list;
> + struct v4l2_event event;
> +};
> +
> +struct v4l2_events {
> + spinlock_t lock; /* Protect everything here. */
> + struct list_head available;
> + wait_queue_head_t wait;
> + struct list_head subscribed; /* Subscribed events. */
> +};
> +
> +struct v4l2_subscribed_event {
> + struct list_head list;
> + u32 type;
> +};
> +
> +int v4l2_event_init(void);
> +void v4l2_event_exit(void);
> +void v4l2_event_init_fh(struct v4l2_fh *fh);
> +void v4l2_event_exit_fh(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +struct v4l2_subscribed_event *v4l2_event_subscribed(
> + struct v4l2_fh *fh, u32 type);
> +void v4l2_event_queue(struct video_device *vdev, struct v4l2_event *ev);
> +int v4l2_event_pending(struct v4l2_fh *fh);
> +int v4l2_event_subscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub);
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> + struct v4l2_event_subscription *sub);
> +
> +#endif /* V4L2_EVENT_H */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> index 1efa916..c15bd13 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,8 +28,11 @@
> #include <linux/types.h>
> #include <linux/list.h>
>
> +#include <media/v4l2-event.h>
> +
> struct v4l2_fh {
> struct list_head list;
> + struct v4l2_events events; /* events, pending and subscribed */
> };
>
> struct video_device;
> --
> 1.5.6.5
>
^ permalink raw reply [flat|nested] 22+ messages in thread* Re: [RFC v2 4/7] V4L: Events: Add backend
2010-01-18 12:43 ` Hans Verkuil
@ 2010-01-22 17:10 ` Sakari Ailus
0 siblings, 0 replies; 22+ messages in thread
From: Sakari Ailus @ 2010-01-22 17:10 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, laurent.pinchart, iivanov, gururaj.nagendra
Hans Verkuil wrote:
> Hi Sakari,
Hi, Hans!
> The code looks good, but I'm not so sure about the use of kmem_cache_*. It
> seems serious overkill to me.
>
> Most of the time there will only be a handful of event messages pending. So
> setting up a kmem_cache for this seems unnecessary to me.
>
> A much better way to ensure fast event reporting IMHO would be to keep a
> list
> of empty event messages rather than destroying an event after it is
> dequeued.
>
> So you have two queues per filehandle: pending and empty; initially both
> are
> empty. When a new event has to be queued the code checks if there are
> events
> available for reuse in the empty queue, and if not a new event struct is
> allocated and added to the pending queue.
I actually had this kind of setup there for a while. Then I thought it'd
be too ugly and went for kmem_cache instead.
The other reason is that it's convenient to keep the memory allocated
even if there are no events subscribed or the device isn't open. For
1000 events that's 96 kiB. I guess an unused kmem_cache object consumes
extra memory very little. The cached slabs can be explicitly freed
anyway by the driver.
The size of the kmem_cache also adjusts based on the number of events in
the queue. Allocating kmem_cache objects is fast if they already exist,
too. There can be temporary delays from allocation, of course.
I can bring it back, sure, if you see a fixed allocation better.
--
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com
^ permalink raw reply [flat|nested] 22+ messages in thread