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

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
>
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html
  

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.
  

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