[PATCH v5 0/6] V4L2 file handles and event interface

[PATCH v5 0/6] V4L2 file handles and event interface

[Sakari Ailus]

Hi,

Here's the seventh version of the V4L2 file handle and event interface
patchset.

The patchset has been tested with the OMAP 3 ISP driver. Patches for
OMAP 3 ISP are not part of this patchset but are available in Gitorious
(branch is called event):

	git://gitorious.org/omap3camera/mainline.git event

The patchset I'm posting now is against the v4l-dvb tree instead of
linux-omap (ouch!). The omap3camera tree thus has a slightly different
version of these patches.

Some more comments from Hans and Laurent. What has changed:

- Documentation for both file handles and events.
- Sequence number and event queue length patches have been combined with
the events backend patch.
- Only VIDIOC_DQEVENT is now unconditionally handled by V4L2 without
driver's involvement.
- __video_do_ioctl() checks that events have been initialised when
handling event ioctls.
- There is a chance of being able to allocate a few more events to an
event queue than intended. This is unlikely to be anyhow harmful, however.
- v4l2_event_subscribe_all() is now v4l2_event_subscribe_many().
- V4L2_FL_USES_V4L2_FH is set on video_device.flags in v4l2_fh_init()
when the driver initialises the first file handle.

- Possibly something else I don't happen to remember just now.

Comments are welcome as always.

Cheers,

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

[PATCH v5 1/6] V4L: File handles

[Sakari Ailus]

This patch adds a list of v4l2_fh structures to every video_device.
It allows using file handle related information in V4L2. The event interface
is one example of such use.

Video device drivers should use the v4l2_fh pointer as their
file->private_data.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/Makefile   |    2 +-
 drivers/media/video/v4l2-dev.c |    4 ++
 drivers/media/video/v4l2-fh.c  |   64 ++++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-dev.h       |    5 +++
 include/media/v4l2-fh.h        |   42 ++++++++++++++++++++++++++
 5 files changed, 116 insertions(+), 1 deletions(-)
 create mode 100644 drivers/media/video/v4l2-fh.c
 create mode 100644 include/media/v4l2-fh.h

diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 5163289..14bf69a 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -10,7 +10,7 @@ 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
+videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
 
 # V4L2 core modules
 
diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-dev.c
index 7090699..65a7b30 100644
--- a/drivers/media/video/v4l2-dev.c
+++ b/drivers/media/video/v4l2-dev.c
@@ -421,6 +421,10 @@ static int __video_register_device(struct video_device *vdev, int type, int nr,
 	if (!vdev->release)
 		return -EINVAL;
 
+	/* v4l2_fh support */
+	spin_lock_init(&vdev->fh_lock);
+	INIT_LIST_HEAD(&vdev->fh_list);
+
 	/* Part 1: check device type */
 	switch (type) {
 	case VFL_TYPE_GRABBER:
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
new file mode 100644
index 0000000..c707930
--- /dev/null
+++ b/drivers/media/video/v4l2-fh.c
@@ -0,0 +1,64 @@
+/*
+ * drivers/media/video/v4l2-fh.c
+ *
+ * V4L2 file handles.
+ *
+ * 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 <linux/bitops.h>
+#include <media/v4l2-dev.h>
+#include <media/v4l2-fh.h>
+
+void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
+{
+	fh->vdev = vdev;
+	INIT_LIST_HEAD(&fh->list);
+	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_init);
+
+void v4l2_fh_add(struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_add(&fh->list, &fh->vdev->fh_list);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_add);
+
+void v4l2_fh_del(struct v4l2_fh *fh)
+{
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	list_del_init(&fh->list);
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_del);
+
+void v4l2_fh_exit(struct v4l2_fh *fh)
+{
+	if (fh->vdev == NULL)
+		return;
+
+	fh->vdev = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
index 2dee938..bebe44b 100644
--- a/include/media/v4l2-dev.h
+++ b/include/media/v4l2-dev.h
@@ -32,6 +32,7 @@ struct v4l2_device;
    Drivers can clear this flag if they want to block all future
    device access. It is cleared by video_unregister_device. */
 #define V4L2_FL_REGISTERED	(0)
+#define V4L2_FL_USES_V4L2_FH	(1)
 
 struct v4l2_file_operations {
 	struct module *owner;
@@ -77,6 +78,10 @@ struct video_device
 	/* attribute to differentiate multiple indices on one physical device */
 	int index;
 
+	/* V4L2 file handles */
+	spinlock_t		fh_lock; /* Lock for all v4l2_fhs */
+	struct list_head	fh_list; /* List of struct v4l2_fh */
+
 	int debug;			/* Activates debug level*/
 
 	/* Video standard vars */
diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
new file mode 100644
index 0000000..6b486aa
--- /dev/null
+++ b/include/media/v4l2-fh.h
@@ -0,0 +1,42 @@
+/*
+ * include/media/v4l2-fh.h
+ *
+ * V4L2 file handle.
+ *
+ * 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_FH_H
+#define V4L2_FH_H
+
+#include <linux/list.h>
+
+struct video_device;
+
+struct v4l2_fh {
+	struct list_head	list;
+	struct video_device	*vdev;
+};
+
+void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
+void v4l2_fh_add(struct v4l2_fh *fh);
+void v4l2_fh_del(struct v4l2_fh *fh);
+void v4l2_fh_exit(struct v4l2_fh *fh);
+
+#endif /* V4L2_EVENT_H */
-- 
1.5.6.5

RE: [PATCH v5 1/6] V4L: File handles

[Aguirre, Sergio]

Heippa!

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, February 19, 2010 1:22 PM
> To: linux-media@vger.kernel.org
> Cc: hverkuil@xs4all.nl; laurent.pinchart@ideasonboard.com; iivanov@mm-
> sol.com; gururaj.nagendra@intel.com; david.cohen@nokia.com; Sakari Ailus
> Subject: [PATCH v5 1/6] V4L: File handles
> 
> This patch adds a list of v4l2_fh structures to every video_device.
> It allows using file handle related information in V4L2. The event
> interface
> is one example of such use.
> 
> Video device drivers should use the v4l2_fh pointer as their
> file->private_data.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/Makefile   |    2 +-
>  drivers/media/video/v4l2-dev.c |    4 ++
>  drivers/media/video/v4l2-fh.c  |   64
> ++++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-dev.h       |    5 +++
>  include/media/v4l2-fh.h        |   42 ++++++++++++++++++++++++++
>  5 files changed, 116 insertions(+), 1 deletions(-)
>  create mode 100644 drivers/media/video/v4l2-fh.c
>  create mode 100644 include/media/v4l2-fh.h
> 
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 5163289..14bf69a 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -10,7 +10,7 @@ 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
> +videodev-objs	:=	v4l2-dev.o v4l2-ioctl.o v4l2-device.o v4l2-fh.o
> 
>  # V4L2 core modules
> 
> diff --git a/drivers/media/video/v4l2-dev.c b/drivers/media/video/v4l2-
> dev.c
> index 7090699..65a7b30 100644
> --- a/drivers/media/video/v4l2-dev.c
> +++ b/drivers/media/video/v4l2-dev.c
> @@ -421,6 +421,10 @@ static int __video_register_device(struct
> video_device *vdev, int type, int nr,
>  	if (!vdev->release)
>  		return -EINVAL;
> 
> +	/* v4l2_fh support */
> +	spin_lock_init(&vdev->fh_lock);
> +	INIT_LIST_HEAD(&vdev->fh_list);
> +
>  	/* Part 1: check device type */
>  	switch (type) {
>  	case VFL_TYPE_GRABBER:
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> new file mode 100644
> index 0000000..c707930
> --- /dev/null
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -0,0 +1,64 @@
> +/*
> + * drivers/media/video/v4l2-fh.c

[1] AFAIK, putting file paths is frowned upon.

Makes maintenance harder if in the future, this files get moved somewhere else.

> + *
> + * V4L2 file handles.
> + *
> + * Copyright (C) 2009 Nokia Corporation.

[2] Shouldn't it be "(C) 2010" already? :)

> + *
> + * 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 <linux/bitops.h>
> +#include <media/v4l2-dev.h>
> +#include <media/v4l2-fh.h>
> +
> +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
> +{
> +	fh->vdev = vdev;
> +	INIT_LIST_HEAD(&fh->list);
> +	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_init);
> +
> +void v4l2_fh_add(struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	list_add(&fh->list, &fh->vdev->fh_list);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_add);
> +
> +void v4l2_fh_del(struct v4l2_fh *fh)
> +{
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	list_del_init(&fh->list);
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_del);
> +
> +void v4l2_fh_exit(struct v4l2_fh *fh)
> +{
> +	if (fh->vdev == NULL)
> +		return;
> +
> +	fh->vdev = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-dev.h b/include/media/v4l2-dev.h
> index 2dee938..bebe44b 100644
> --- a/include/media/v4l2-dev.h
> +++ b/include/media/v4l2-dev.h
> @@ -32,6 +32,7 @@ struct v4l2_device;
>     Drivers can clear this flag if they want to block all future
>     device access. It is cleared by video_unregister_device. */
>  #define V4L2_FL_REGISTERED	(0)
> +#define V4L2_FL_USES_V4L2_FH	(1)
> 
>  struct v4l2_file_operations {
>  	struct module *owner;
> @@ -77,6 +78,10 @@ struct video_device
>  	/* attribute to differentiate multiple indices on one physical
> device */
>  	int index;
> 
> +	/* V4L2 file handles */
> +	spinlock_t		fh_lock; /* Lock for all v4l2_fhs */
> +	struct list_head	fh_list; /* List of struct v4l2_fh */
> +
>  	int debug;			/* Activates debug level*/
> 
>  	/* Video standard vars */
> diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> new file mode 100644
> index 0000000..6b486aa
> --- /dev/null
> +++ b/include/media/v4l2-fh.h
> @@ -0,0 +1,42 @@
> +/*
> + * include/media/v4l2-fh.h

Same as [1]

> + *
> + * V4L2 file handle.
> + *
> + * Copyright (C) 2009 Nokia Corporation.

Same as [2]

> + *
> + * 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_FH_H
> +#define V4L2_FH_H
> +
> +#include <linux/list.h>

Shouldn't you add one more header here?:

#include <media/v4l2-dev.h>

(for struct video_device)

> +
> +struct video_device;
> +
> +struct v4l2_fh {
> +	struct list_head	list;
> +	struct video_device	*vdev;
> +};
> +
> +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> +void v4l2_fh_add(struct v4l2_fh *fh);
> +void v4l2_fh_del(struct v4l2_fh *fh);
> +void v4l2_fh_exit(struct v4l2_fh *fh);
> +
> +#endif /* V4L2_EVENT_H */

Wrong comment, must have been:

	/* V4L2_FH_H */

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

Re: [PATCH v5 1/6] V4L: File handles

[Laurent Pinchart]

Hi Sergio,

On Friday 19 February 2010 23:29:54 Aguirre, Sergio wrote:
> Heippa!
> 
> > -----Original Message-----
> > From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> > owner@vger.kernel.org] On Behalf Of Sakari Ailus
> > Sent: Friday, February 19, 2010 1:22 PM
> > To: linux-media@vger.kernel.org
> > Cc: hverkuil@xs4all.nl; laurent.pinchart@ideasonboard.com; iivanov@mm-
> > sol.com; gururaj.nagendra@intel.com; david.cohen@nokia.com; Sakari Ailus
> > Subject: [PATCH v5 1/6] V4L: File handles
> > 
> > This patch adds a list of v4l2_fh structures to every video_device.
> > It allows using file handle related information in V4L2. The event
> > interface
> > is one example of such use.
> > 
> > Video device drivers should use the v4l2_fh pointer as their
> > file->private_data.
> > 
> > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > ---
> > 
> >  drivers/media/video/Makefile   |    2 +-
> >  drivers/media/video/v4l2-dev.c |    4 ++
> >  drivers/media/video/v4l2-fh.c  |   64
> > 
> > ++++++++++++++++++++++++++++++++++++++++
> > 
> >  include/media/v4l2-dev.h       |    5 +++
> >  include/media/v4l2-fh.h        |   42 ++++++++++++++++++++++++++
> >  5 files changed, 116 insertions(+), 1 deletions(-)
> >  create mode 100644 drivers/media/video/v4l2-fh.c
> >  create mode 100644 include/media/v4l2-fh.h

[snip]

> > diff --git a/drivers/media/video/v4l2-fh.c
> > b/drivers/media/video/v4l2-fh.c new file mode 100644
> > index 0000000..c707930
> > --- /dev/null
> > +++ b/drivers/media/video/v4l2-fh.c
> > @@ -0,0 +1,64 @@
> > +/*
> > + * drivers/media/video/v4l2-fh.c
> 
> [1] AFAIK, putting file paths is frowned upon.
> 
> Makes maintenance harder if in the future, this files get moved somewhere
> else.
> 
> > + *
> > + * V4L2 file handles.
> > + *
> > + * Copyright (C) 2009 Nokia Corporation.
> 
> [2] Shouldn't it be "(C) 2010" already? :)

That shows how long the V4L2 events API review is taking ;-)
 
[snip]

> > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > new file mode 100644
> > index 0000000..6b486aa
> > --- /dev/null
> > +++ b/include/media/v4l2-fh.h
> > @@ -0,0 +1,42 @@
> > +/*
> > + * include/media/v4l2-fh.h
> 
> Same as [1]
> 
> > + *
> > + * V4L2 file handle.
> > + *
> > + * Copyright (C) 2009 Nokia Corporation.
> 
> Same as [2]
> 
> > + *
> > + * 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_FH_H
> > +#define V4L2_FH_H
> > +
> > +#include <linux/list.h>
> 
> Shouldn't you add one more header here?:
> 
> #include <media/v4l2-dev.h>
> 
> (for struct video_device)

This header only needs struct video_device *, not struct video_device, so 
adding a forward definition will be more efficient (lower compilation time for 
compilation units that include v4l2-fh.h but not v4l2-dev.h).

> > +
> > +struct video_device;
> > +
> > +struct v4l2_fh {
> > +	struct list_head	list;
> > +	struct video_device	*vdev;
> > +};
> > +
> > +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> > +void v4l2_fh_add(struct v4l2_fh *fh);
> > +void v4l2_fh_del(struct v4l2_fh *fh);
> > +void v4l2_fh_exit(struct v4l2_fh *fh);
> > +
> > +#endif /* V4L2_EVENT_H */

-- 
Regards,

Laurent Pinchart

RE: [PATCH v5 1/6] V4L: File handles

[Aguirre, Sergio]

Laurent,

> -----Original Message-----
> From: Laurent Pinchart [mailto:laurent.pinchart@ideasonboard.com]
> Sent: Friday, February 19, 2010 4:35 PM
> To: Aguirre, Sergio
> Cc: Sakari Ailus; linux-media@vger.kernel.org; hverkuil@xs4all.nl;
> iivanov@mm-sol.com; gururaj.nagendra@intel.com; david.cohen@nokia.com
> Subject: Re: [PATCH v5 1/6] V4L: File handles
> 
> Hi Sergio,
> 
> On Friday 19 February 2010 23:29:54 Aguirre, Sergio wrote:
> > Heippa!
> >
> > > -----Original Message-----
> > > From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> > > owner@vger.kernel.org] On Behalf Of Sakari Ailus
> > > Sent: Friday, February 19, 2010 1:22 PM
> > > To: linux-media@vger.kernel.org
> > > Cc: hverkuil@xs4all.nl; laurent.pinchart@ideasonboard.com; iivanov@mm-
> > > sol.com; gururaj.nagendra@intel.com; david.cohen@nokia.com; Sakari
> Ailus
> > > Subject: [PATCH v5 1/6] V4L: File handles
> > >
> > > This patch adds a list of v4l2_fh structures to every video_device.
> > > It allows using file handle related information in V4L2. The event
> > > interface
> > > is one example of such use.
> > >
> > > Video device drivers should use the v4l2_fh pointer as their
> > > file->private_data.
> > >
> > > Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > > ---
> > >
> > >  drivers/media/video/Makefile   |    2 +-
> > >  drivers/media/video/v4l2-dev.c |    4 ++
> > >  drivers/media/video/v4l2-fh.c  |   64
> > >
> > > ++++++++++++++++++++++++++++++++++++++++
> > >
> > >  include/media/v4l2-dev.h       |    5 +++
> > >  include/media/v4l2-fh.h        |   42 ++++++++++++++++++++++++++
> > >  5 files changed, 116 insertions(+), 1 deletions(-)
> > >  create mode 100644 drivers/media/video/v4l2-fh.c
> > >  create mode 100644 include/media/v4l2-fh.h
> 
> [snip]
> 
> > > diff --git a/drivers/media/video/v4l2-fh.c
> > > b/drivers/media/video/v4l2-fh.c new file mode 100644
> > > index 0000000..c707930
> > > --- /dev/null
> > > +++ b/drivers/media/video/v4l2-fh.c
> > > @@ -0,0 +1,64 @@
> > > +/*
> > > + * drivers/media/video/v4l2-fh.c
> >
> > [1] AFAIK, putting file paths is frowned upon.
> >
> > Makes maintenance harder if in the future, this files get moved
> somewhere
> > else.
> >
> > > + *
> > > + * V4L2 file handles.
> > > + *
> > > + * Copyright (C) 2009 Nokia Corporation.
> >
> > [2] Shouldn't it be "(C) 2010" already? :)
> 
> That shows how long the V4L2 events API review is taking ;-)

:D

> 
> [snip]
> 
> > > diff --git a/include/media/v4l2-fh.h b/include/media/v4l2-fh.h
> > > new file mode 100644
> > > index 0000000..6b486aa
> > > --- /dev/null
> > > +++ b/include/media/v4l2-fh.h
> > > @@ -0,0 +1,42 @@
> > > +/*
> > > + * include/media/v4l2-fh.h
> >
> > Same as [1]
> >
> > > + *
> > > + * V4L2 file handle.
> > > + *
> > > + * Copyright (C) 2009 Nokia Corporation.
> >
> > Same as [2]
> >
> > > + *
> > > + * 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_FH_H
> > > +#define V4L2_FH_H
> > > +
> > > +#include <linux/list.h>
> >
> > Shouldn't you add one more header here?:
> >
> > #include <media/v4l2-dev.h>
> >
> > (for struct video_device)
> 
> This header only needs struct video_device *, not struct video_device, so
> adding a forward definition will be more efficient (lower compilation time
> for
> compilation units that include v4l2-fh.h but not v4l2-dev.h).

Ok, understood. Thanks for clarifying.

Regards,
Sergio

> 
> > > +
> > > +struct video_device;
> > > +
> > > +struct v4l2_fh {
> > > +	struct list_head	list;
> > > +	struct video_device	*vdev;
> > > +};
> > > +
> > > +void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> > > +void v4l2_fh_add(struct v4l2_fh *fh);
> > > +void v4l2_fh_del(struct v4l2_fh *fh);
> > > +void v4l2_fh_exit(struct v4l2_fh *fh);
> > > +
> > > +#endif /* V4L2_EVENT_H */
> 
> --
> Regards,
> 
> Laurent Pinchart

Re: [PATCH v5 1/6] V4L: File handles

[Sakari Ailus]

Aguirre, Sergio wrote:
> Heippa!

Hi, Sergio!

Thanks for comments!

...
>> @@ -0,0 +1,64 @@
>> +/*
>> + * drivers/media/video/v4l2-fh.c
> 
> [1] AFAIK, putting file paths is frowned upon.
> 
> Makes maintenance harder if in the future, this files get moved somewhere else.

Ack.

>> + *
>> + * V4L2 file handles.
>> + *
>> + * Copyright (C) 2009 Nokia Corporation.
> 
> [2] Shouldn't it be "(C) 2010" already? :)

It is. The patches have been floating around since 2009 and I've just
forgotten to update this. I hope no-one will notify 2010 must be
replaced by 2011 at some point... ;-)

...

>> +#endif /* V4L2_EVENT_H */
> 
> Wrong comment, must have been:
> 
> 	/* V4L2_FH_H */

Will fix.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

RE: [PATCH v5 1/6] V4L: File handles

[Aguirre, Sergio]

Hi Sakari,

From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]

...

> Will fix.

Seems that you missed to fix this patch comments on your v6 version...

:-/

Regards,
Sergio
> 
> --
> Sakari Ailus
> sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v5 1/6] V4L: File handles

[Sakari Ailus]

Aguirre, Sergio wrote:
> Hi Sakari,
> 
> From: Sakari Ailus [mailto:sakari.ailus@maxwell.research.nokia.com]
> 
> ...
> 
>> Will fix.
> 
> Seems that you missed to fix this patch comments on your v6 version...
> 
> :-/

Um, true! I'll make the changes for the next one then.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

[PATCH v5 2/6] V4L: File handles: Add documentation

[Sakari Ailus]

Add documentation on using V4L2 file handles (v4l2_fh) in V4L2 drivers.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 Documentation/video4linux/v4l2-framework.txt |   36 ++++++++++++++++++++++++++
 1 files changed, 36 insertions(+), 0 deletions(-)

diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 74d677c..08f9e59 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -695,3 +695,39 @@ The better way to understand it is to take a look at vivi driver. One
 of the main reasons for vivi is to be a videobuf usage example. the
 vivi_thread_tick() does the task that the IRQ callback would do on PCI
 drivers (or the irq callback on USB).
+
+struct v4l2_fh
+--------------
+
+struct v4l2_fh provides a way to easily keep file handle specific data
+that is used by the V4L2 framework.
+
+struct v4l2_fh is allocated as a part of the driver's own file handle
+structure and is set to file->private_data in the driver's open
+function by the driver. Drivers can extract their own file handle
+structure by using the container_of macro.
+
+Useful functions:
+
+- v4l2_fh_init()
+
+  Initialise the file handle.
+
+- v4l2_fh_add()
+
+  Add a v4l2_fh to video_device file handle list. May be called after
+  initialising the file handle.
+
+- v4l2_fh_del()
+
+  Unassociate the file handle from video_device(). The file handle
+  exit function may now be called.
+
+- v4l2_fh_exit()
+
+  Uninitialise the file handle. After uninitialisation the v4l2_fh
+  memory can be freed.
+
+The users of v4l2_fh know whether a driver uses v4l2_fh as its
+file.private_data pointer by testing the V4L2_FL_USES_V4L2_FH bit in
+video_device.flags.
-- 
1.5.6.5

Re: [PATCH v5 2/6] V4L: File handles: Add documentation

[Hans Verkuil]

On Friday 19 February 2010 20:21:56 Sakari Ailus wrote:
> Add documentation on using V4L2 file handles (v4l2_fh) in V4L2 drivers.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  Documentation/video4linux/v4l2-framework.txt |   36 ++++++++++++++++++++++++++
>  1 files changed, 36 insertions(+), 0 deletions(-)
> 
> diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
> index 74d677c..08f9e59 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -695,3 +695,39 @@ The better way to understand it is to take a look at vivi driver. One
>  of the main reasons for vivi is to be a videobuf usage example. the
>  vivi_thread_tick() does the task that the IRQ callback would do on PCI
>  drivers (or the irq callback on USB).
> +
> +struct v4l2_fh
> +--------------
> +
> +struct v4l2_fh provides a way to easily keep file handle specific data
> +that is used by the V4L2 framework.
> +
> +struct v4l2_fh is allocated as a part of the driver's own file handle
> +structure and is set to file->private_data in the driver's open
> +function by the driver. Drivers can extract their own file handle
> +structure by using the container_of macro.
> +
> +Useful functions:
> +
> +- v4l2_fh_init()
> +
> +  Initialise the file handle.
> +
> +- v4l2_fh_add()
> +
> +  Add a v4l2_fh to video_device file handle list. May be called after
> +  initialising the file handle.
> +
> +- v4l2_fh_del()
> +
> +  Unassociate the file handle from video_device(). The file handle
> +  exit function may now be called.
> +
> +- v4l2_fh_exit()
> +
> +  Uninitialise the file handle. After uninitialisation the v4l2_fh
> +  memory can be freed.
> +
> +The users of v4l2_fh know whether a driver uses v4l2_fh as its
> +file.private_data pointer by testing the V4L2_FL_USES_V4L2_FH bit in
> +video_device.flags.

Replace '.' by '->':

file->private_data
video_device->flags

> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

[PATCH v5 3/6] V4L: Events: Add new ioctls for events

[Sakari Ailus]

This patch adds a set of new ioctls to the V4L2 API. The ioctls conform to
V4L2 Events RFC version 2.3:

<URL:http://www.spinics.net/lists/linux-media/msg12033.html>

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-compat-ioctl32.c |    3 +++
 drivers/media/video/v4l2-ioctl.c          |    3 +++
 include/linux/videodev2.h                 |   26 ++++++++++++++++++++++++++
 3 files changed, 32 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-compat-ioctl32.c b/drivers/media/video/v4l2-compat-ioctl32.c
index f77f84b..9004a5f 100644
--- a/drivers/media/video/v4l2-compat-ioctl32.c
+++ b/drivers/media/video/v4l2-compat-ioctl32.c
@@ -1086,6 +1086,9 @@ long v4l2_compat_ioctl32(struct file *file, unsigned int cmd, unsigned long arg)
 	case VIDIOC_QUERY_DV_PRESET:
 	case VIDIOC_S_DV_TIMINGS:
 	case VIDIOC_G_DV_TIMINGS:
+	case VIDIOC_DQEVENT:
+	case VIDIOC_SUBSCRIBE_EVENT:
+	case VIDIOC_UNSUBSCRIBE_EVENT:
 		ret = do_video_ioctl(file, cmd, arg);
 		break;
 
diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 4b11257..34c7d6e 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -290,6 +290,9 @@ static const char *v4l2_ioctls[] = {
 	[_IOC_NR(VIDIOC_QUERY_DV_PRESET)]  = "VIDIOC_QUERY_DV_PRESET",
 	[_IOC_NR(VIDIOC_S_DV_TIMINGS)]     = "VIDIOC_S_DV_TIMINGS",
 	[_IOC_NR(VIDIOC_G_DV_TIMINGS)]     = "VIDIOC_G_DV_TIMINGS",
+	[_IOC_NR(VIDIOC_DQEVENT)]	   = "VIDIOC_DQEVENT",
+	[_IOC_NR(VIDIOC_SUBSCRIBE_EVENT)]  = "VIDIOC_SUBSCRIBE_EVENT",
+	[_IOC_NR(VIDIOC_UNSUBSCRIBE_EVENT)] = "VIDIOC_UNSUBSCRIBE_EVENT",
 };
 #define V4L2_IOCTLS ARRAY_SIZE(v4l2_ioctls)
 
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 3c26560..f7237fc 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1622,6 +1622,29 @@ struct v4l2_streamparm {
 };
 
 /*
+ *	E V E N T S
+ */
+
+struct v4l2_event {
+	__u32				type;
+	union {
+		__u8			data[64];
+	} u;
+	__u32				pending;
+	__u32				sequence;
+	struct timespec			timestamp;
+	__u32				reserved[9];
+};
+
+struct v4l2_event_subscription {
+	__u32				type;
+	__u32				reserved[7];
+};
+
+#define V4L2_EVENT_ALL				0
+#define V4L2_EVENT_PRIVATE_START		0x08000000
+
+/*
  *	A D V A N C E D   D E B U G G I N G
  *
  *	NOTE: EXPERIMENTAL API, NEVER RELY ON THIS IN APPLICATIONS!
@@ -1743,6 +1766,9 @@ struct v4l2_dbg_chip_ident {
 #define	VIDIOC_QUERY_DV_PRESET	_IOR('V',  86, struct v4l2_dv_preset)
 #define	VIDIOC_S_DV_TIMINGS	_IOWR('V', 87, struct v4l2_dv_timings)
 #define	VIDIOC_G_DV_TIMINGS	_IOWR('V', 88, struct v4l2_dv_timings)
+#define	VIDIOC_DQEVENT		 _IOR('V', 83, struct v4l2_event)
+#define	VIDIOC_SUBSCRIBE_EVENT	 _IOW('V', 84, struct v4l2_event_subscription)
+#define	VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 85, struct v4l2_event_subscription)
 
 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/video/v4l2-compat-ioctl32.c as well! */
-- 
1.5.6.5

[PATCH v5 4/6] V4L: Events: Add backend

[Sakari Ailus]

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-event.c |  286 ++++++++++++++++++++++++++++++++++++++
 drivers/media/video/v4l2-fh.c    |    4 +
 include/media/v4l2-event.h       |   65 +++++++++
 include/media/v4l2-fh.h          |    2 +
 5 files changed, 359 insertions(+), 1 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 14bf69a..b84abfe 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-event.c b/drivers/media/video/v4l2-event.c
new file mode 100644
index 0000000..ab31cc6
--- /dev/null
+++ b/drivers/media/video/v4l2-event.c
@@ -0,0 +1,286 @@
+/*
+ * 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-fh.h>
+#include <media/v4l2-event.h>
+
+#include <linux/sched.h>
+
+static int v4l2_event_init(struct v4l2_fh *fh)
+{
+	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
+	if (fh->events == NULL)
+		return -ENOMEM;
+
+	init_waitqueue_head(&fh->events->wait);
+
+	INIT_LIST_HEAD(&fh->events->free);
+	INIT_LIST_HEAD(&fh->events->available);
+	INIT_LIST_HEAD(&fh->events->subscribed);
+
+	fh->events->sequence = -1;
+
+	return 0;
+}
+
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
+{
+	struct v4l2_events *events;
+	unsigned long flags;
+	int ret;
+
+	if (!fh->events) {
+		ret = v4l2_event_init(fh);
+		if (ret)
+			return ret;
+	}
+
+	events = fh->events;
+
+	while (events->nallocated < n) {
+		struct v4l2_kevent *kev;
+
+		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
+		if (kev == NULL)
+			return -ENOMEM;
+
+		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+		list_add_tail(&kev->list, &events->free);
+		events->nallocated++;
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_alloc);
+
+#define list_kfree(list, type, member)				\
+	while (!list_empty(list)) {				\
+		type *hi;					\
+		hi = list_first_entry(list, type, member);	\
+		list_del(&hi->member);				\
+		kfree(hi);					\
+	}
+
+void v4l2_event_free(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+
+	if (!events)
+		return;
+
+	list_kfree(&events->free, struct v4l2_kevent, list);
+	list_kfree(&events->available, struct v4l2_kevent, list);
+	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
+
+	kfree(events);
+	fh->events = NULL;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_free);
+
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
+{
+	struct v4l2_events *events = fh->events;
+	struct v4l2_kevent *kev;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	if (list_empty(&events->available)) {
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+		return -ENOENT;
+	}
+
+	WARN_ON(events->navailable == 0);
+
+	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
+	list_move(&kev->list, &events->free);
+	events->navailable--;
+
+	kev->event.pending = events->navailable;
+	*event = kev->event;
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	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 *sev;
+
+	list_for_each_entry(sev, &events->subscribed, list) {
+		if (sev->type == type)
+			return sev;
+	}
+
+	return NULL;
+}
+
+void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
+{
+	struct v4l2_fh *fh;
+	unsigned long flags;
+	struct timespec timestamp;
+
+	ktime_get_ts(&timestamp);
+
+	spin_lock_irqsave(&vdev->fh_lock, flags);
+
+	list_for_each_entry(fh, &vdev->fh_list, list) {
+		struct v4l2_events *events = fh->events;
+		struct v4l2_kevent *kev;
+		u32 sequence;
+
+		/* Are we subscribed? */
+		if (!v4l2_event_subscribed(fh, ev->type))
+			continue;
+
+		/* Increase event sequence number on fh. */
+		events->sequence++;
+		sequence = events->sequence;
+
+		/* Do we have any free events? */
+		if (list_empty(&events->free))
+			continue;
+
+		/* Take one and fill it. */
+		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
+		kev->event.type = ev->type;
+		kev->event.u = ev->u;
+		kev->event.timestamp = timestamp;
+		kev->event.sequence = sequence;
+		list_move_tail(&kev->list, &events->available);
+
+		events->navailable++;
+
+		wake_up_all(&events->wait);
+	}
+
+	spin_unlock_irqrestore(&vdev->fh_lock, flags);
+}
+EXPORT_SYMBOL_GPL(v4l2_event_queue);
+
+int v4l2_event_pending(struct v4l2_fh *fh)
+{
+	return fh->events->navailable;
+}
+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 *sev;
+	unsigned long flags;
+
+	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
+	if (!sev)
+		return -ENOMEM;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	if (v4l2_event_subscribed(fh, sub->type) == NULL) {
+		INIT_LIST_HEAD(&sev->list);
+		sev->type = sub->type;
+
+		list_add(&sev->list, &events->subscribed);
+	}
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
+
+/* subscribe a zero-terminated array of events */
+int v4l2_event_subscribe_many(struct v4l2_fh *fh, const u32 *all)
+{
+	int ret;
+
+	for (; *all; all++) {
+		struct v4l2_event_subscription sub;
+
+		sub.type = *all;
+
+		ret = v4l2_event_subscribe(fh, &sub);
+		if (ret)
+			return ret;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_subscribe_many);
+
+static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
+{
+	struct v4l2_events *events = fh->events;
+	unsigned long flags;
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	while (!list_empty(&events->subscribed)) {
+		struct v4l2_subscribed_event *sev;
+
+		sev = list_first_entry(&events->subscribed,
+				       struct v4l2_subscribed_event, list);
+
+		list_del(&sev->list);
+		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+		kfree(sev);
+		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+	}
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+}
+
+int v4l2_event_unsubscribe(struct v4l2_fh *fh,
+			   struct v4l2_event_subscription *sub)
+{
+	struct v4l2_subscribed_event *sev;
+	unsigned long flags;
+
+	if (sub->type == V4L2_EVENT_ALL) {
+		v4l2_event_unsubscribe_all(fh);
+		return 0;
+	}
+
+	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
+
+	sev = v4l2_event_subscribed(fh, sub->type);
+	if (sev != NULL)
+		list_del(&sev->list);
+
+	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
+
+	if (sev != NULL)
+		kfree(sev);
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
index c707930..3a9bc7d 100644
--- a/drivers/media/video/v4l2-fh.c
+++ b/drivers/media/video/v4l2-fh.c
@@ -25,11 +25,13 @@
 #include <linux/bitops.h>
 #include <media/v4l2-dev.h>
 #include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
 {
 	fh->vdev = vdev;
 	INIT_LIST_HEAD(&fh->list);
+	fh->events = NULL;
 	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_init);
@@ -60,5 +62,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
 		return;
 
 	fh->vdev = NULL;
+
+	v4l2_event_free(fh);
 }
 EXPORT_SYMBOL_GPL(v4l2_fh_exit);
diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
new file mode 100644
index 0000000..284d495
--- /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_kevent {
+	struct list_head	list;
+	struct v4l2_event	event;
+};
+
+struct v4l2_subscribed_event {
+	struct list_head	list;
+	u32			type;
+};
+
+struct v4l2_events {
+	wait_queue_head_t	wait;
+	struct list_head	subscribed; /* Subscribed events */
+	struct list_head	available; /* Dequeueable event */
+	unsigned int		navailable;
+	struct list_head	free; /* Events ready for use */
+	unsigned int		nallocated; /* Number of allocated events */
+	u32			sequence;
+};
+
+int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
+void v4l2_event_free(struct v4l2_fh *fh);
+int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
+void v4l2_event_queue(struct video_device *vdev, const 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_subscribe_many(struct v4l2_fh *fh, const u32 *all);
+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 6b486aa..6c9df56 100644
--- a/include/media/v4l2-fh.h
+++ b/include/media/v4l2-fh.h
@@ -28,10 +28,12 @@
 #include <linux/list.h>
 
 struct video_device;
+struct v4l2_events;
 
 struct v4l2_fh {
 	struct list_head	list;
 	struct video_device	*vdev;
+	struct v4l2_events      *events; /* events, pending and subscribed */
 };
 
 void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
-- 
1.5.6.5

RE: [PATCH v5 4/6] V4L: Events: Add backend

[Aguirre, Sergio]

Heippa!

> -----Original Message-----
> From: linux-media-owner@vger.kernel.org [mailto:linux-media-
> owner@vger.kernel.org] On Behalf Of Sakari Ailus
> Sent: Friday, February 19, 2010 1:22 PM
> To: linux-media@vger.kernel.org
> Cc: hverkuil@xs4all.nl; laurent.pinchart@ideasonboard.com; iivanov@mm-
> sol.com; gururaj.nagendra@intel.com; david.cohen@nokia.com; Sakari Ailus
> Subject: [PATCH v5 4/6] 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.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/Makefile     |    3 +-
>  drivers/media/video/v4l2-event.c |  286
> ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-fh.c    |    4 +
>  include/media/v4l2-event.h       |   65 +++++++++
>  include/media/v4l2-fh.h          |    2 +
>  5 files changed, 359 insertions(+), 1 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 14bf69a..b84abfe 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-event.c b/drivers/media/video/v4l2-
> event.c
> new file mode 100644
> index 0000000..ab31cc6
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,286 @@
> +/*
> + * drivers/media/video/v4l2-event.c

No filepaths.

> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.

2010

> + *
> + * 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-fh.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +static int v4l2_event_init(struct v4l2_fh *fh)
> +{
> +	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
> +	if (fh->events == NULL)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&fh->events->wait);
> +
> +	INIT_LIST_HEAD(&fh->events->free);
> +	INIT_LIST_HEAD(&fh->events->available);
> +	INIT_LIST_HEAD(&fh->events->subscribed);
> +
> +	fh->events->sequence = -1;
> +
> +	return 0;
> +}
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
> +{
> +	struct v4l2_events *events;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!fh->events) {
> +		ret = v4l2_event_init(fh);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	events = fh->events;
> +
> +	while (events->nallocated < n) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
> +		if (kev == NULL)
> +			return -ENOMEM;
> +
> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +		list_add_tail(&kev->list, &events->free);
> +		events->nallocated++;
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_alloc);
> +
> +#define list_kfree(list, type, member)				\
> +	while (!list_empty(list)) {				\
> +		type *hi;					\
> +		hi = list_first_entry(list, type, member);	\
> +		list_del(&hi->member);				\
> +		kfree(hi);					\
> +	}
> +
> +void v4l2_event_free(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +
> +	if (!events)
> +		return;
> +
> +	list_kfree(&events->free, struct v4l2_kevent, list);
> +	list_kfree(&events->available, struct v4l2_kevent, list);
> +	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
> +
> +	kfree(events);
> +	fh->events = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_free);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_kevent *kev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	WARN_ON(events->navailable == 0);

I don't think the above warning will ever happen. Looks a bit over protective to me.

Whenever you update your "events->available" list, you're holding the fh_lock spinlock, so there's no chance that the list of events would contan a different number of elents to what the navailable var is holding. Is it?

Please correct me if I'm missing something...

Or if you insist in checking, you could just have done this instead:

	if (list_empty(&events->available) || (events->navailable == 0)) {
		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
		return -ENOENT;
	}

As it doesn't make sense to proceed if navailable is zero, I believe...

> +
> +	kev = list_first_entry(&events->available, struct v4l2_kevent,
> list);
> +	list_move(&kev->list, &events->free);
> +	events->navailable--;
> +
> +	kev->event.pending = events->navailable;
> +	*event = kev->event;
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	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 *sev;
> +
> +	list_for_each_entry(sev, &events->subscribed, list) {
> +		if (sev->type == type)
> +			return sev;
> +	}
> +
> +	return NULL;
> +}
> +
> +void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event
> *ev)
> +{
> +	struct v4l2_fh *fh;
> +	unsigned long flags;
> +	struct timespec timestamp;
> +
> +	ktime_get_ts(&timestamp);
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fh_list, list) {
> +		struct v4l2_events *events = fh->events;
> +		struct v4l2_kevent *kev;
> +		u32 sequence;
> +
> +		/* Are we subscribed? */
> +		if (!v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		/* Increase event sequence number on fh. */
> +		events->sequence++;
> +		sequence = events->sequence;
> +
> +		/* Do we have any free events? */
> +		if (list_empty(&events->free))
> +			continue;
> +
> +		/* Take one and fill it. */
> +		kev = list_first_entry(&events->free, struct v4l2_kevent,
> list);
> +		kev->event.type = ev->type;
> +		kev->event.u = ev->u;
> +		kev->event.timestamp = timestamp;
> +		kev->event.sequence = sequence;
> +		list_move_tail(&kev->list, &events->available);
> +
> +		events->navailable++;
> +
> +		wake_up_all(&events->wait);
> +	}
> +
> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> +	return fh->events->navailable;
> +}
> +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 *sev;
> +	unsigned long flags;
> +
> +	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
> +	if (!sev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (v4l2_event_subscribed(fh, sub->type) == NULL) {
> +		INIT_LIST_HEAD(&sev->list);
> +		sev->type = sub->type;
> +
> +		list_add(&sev->list, &events->subscribed);
> +	}
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +/* subscribe a zero-terminated array of events */
> +int v4l2_event_subscribe_many(struct v4l2_fh *fh, const u32 *all)
> +{
> +	int ret;
> +
> +	for (; *all; all++) {
> +		struct v4l2_event_subscription sub;
> +
> +		sub.type = *all;
> +
> +		ret = v4l2_event_subscribe(fh, &sub);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe_many);
> +
> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	while (!list_empty(&events->subscribed)) {
> +		struct v4l2_subscribed_event *sev;
> +
> +		sev = list_first_entry(&events->subscribed,
> +				       struct v4l2_subscribed_event, list);
> +
> +		list_del(&sev->list);
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		kfree(sev);
> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	}
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}
> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +
> +	if (sub->type == V4L2_EVENT_ALL) {
> +		v4l2_event_unsubscribe_all(fh);
> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	sev = v4l2_event_subscribed(fh, sub->type);
> +	if (sev != NULL)
> +		list_del(&sev->list);
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (sev != NULL)
> +		kfree(sev);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index c707930..3a9bc7d 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -25,11 +25,13 @@
>  #include <linux/bitops.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
> 
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  {
>  	fh->vdev = vdev;
>  	INIT_LIST_HEAD(&fh->list);
> +	fh->events = NULL;
>  	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> @@ -60,5 +62,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>  		return;
> 
>  	fh->vdev = NULL;
> +
> +	v4l2_event_free(fh);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..284d495
> --- /dev/null
> +++ b/include/media/v4l2-event.h
> @@ -0,0 +1,65 @@
> +/*
> + * include/media/v4l2-event.h

Again, no filepaths.

> + *
> + * V4L2 events.
> + *
> + * Copyright (C) 2009 Nokia Corporation.

2010

Regards,
Sergio

> + *
> + * 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_kevent {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +struct v4l2_events {
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events */
> +	struct list_head	available; /* Dequeueable event */
> +	unsigned int		navailable;
> +	struct list_head	free; /* Events ready for use */
> +	unsigned int		nallocated; /* Number of allocated events */
> +	u32			sequence;
> +};
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> +void v4l2_event_free(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +void v4l2_event_queue(struct video_device *vdev, const 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_subscribe_many(struct v4l2_fh *fh, const u32 *all);
> +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 6b486aa..6c9df56 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,10 +28,12 @@
>  #include <linux/list.h>
> 
>  struct video_device;
> +struct v4l2_events;
> 
>  struct v4l2_fh {
>  	struct list_head	list;
>  	struct video_device	*vdev;
> +	struct v4l2_events      *events; /* events, pending and subscribed
> */
>  };
> 
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> --
> 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

Re: [PATCH v5 4/6] V4L: Events: Add backend

[Sakari Ailus]

Aguirre, Sergio wrote:
> Heippa!

Hi, Sergio!

Your lines seem to be over 80 characters long. :I

...
>> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	struct v4l2_kevent *kev;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	if (list_empty(&events->available)) {
>> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +		return -ENOENT;
>> +	}
>> +
>> +	WARN_ON(events->navailable == 0);
> 
> I don't think the above warning will ever happen. Looks a bit over protective to me.

If it does it's a bug somewhere.

> Whenever you update your "events->available" list, you're holding the fh_lock spinlock, so there's no chance that the list of events would contan a different number of elents to what the navailable var is holding. Is it?
> 
> Please correct me if I'm missing something...

At the moment that is true as far as I see it. But if it's changed in
future chances are something goes wrong. It's a simple check but might
save some headaches.

> Or if you insist in checking, you could just have done this instead:
> 
> 	if (list_empty(&events->available) || (events->navailable == 0)) {
> 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 		return -ENOENT;
> 	}
> 
> As it doesn't make sense to proceed if navailable is zero, I believe...

It'd be a bug in the code so it must be WARN_ON().

I think the question is whether the check should be left there or removed.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v5 4/6] V4L: Events: Add backend

[Hans Verkuil]

Hi Sakari,

Here are some more comments.

On Friday 19 February 2010 20:21:58 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-event.c |  286 ++++++++++++++++++++++++++++++++++++++
>  drivers/media/video/v4l2-fh.c    |    4 +
>  include/media/v4l2-event.h       |   65 +++++++++
>  include/media/v4l2-fh.h          |    2 +
>  5 files changed, 359 insertions(+), 1 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 14bf69a..b84abfe 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-event.c b/drivers/media/video/v4l2-event.c
> new file mode 100644
> index 0000000..ab31cc6
> --- /dev/null
> +++ b/drivers/media/video/v4l2-event.c
> @@ -0,0 +1,286 @@
> +/*
> + * 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-fh.h>
> +#include <media/v4l2-event.h>
> +
> +#include <linux/sched.h>
> +
> +static int v4l2_event_init(struct v4l2_fh *fh)
> +{
> +	fh->events = kzalloc(sizeof(*fh->events), GFP_KERNEL);
> +	if (fh->events == NULL)
> +		return -ENOMEM;
> +
> +	init_waitqueue_head(&fh->events->wait);
> +
> +	INIT_LIST_HEAD(&fh->events->free);
> +	INIT_LIST_HEAD(&fh->events->available);
> +	INIT_LIST_HEAD(&fh->events->subscribed);
> +
> +	fh->events->sequence = -1;
> +
> +	return 0;
> +}
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n)
> +{
> +	struct v4l2_events *events;
> +	unsigned long flags;
> +	int ret;
> +
> +	if (!fh->events) {
> +		ret = v4l2_event_init(fh);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	events = fh->events;
> +
> +	while (events->nallocated < n) {
> +		struct v4l2_kevent *kev;
> +
> +		kev = kzalloc(sizeof(*kev), GFP_KERNEL);
> +		if (kev == NULL)
> +			return -ENOMEM;
> +
> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +		list_add_tail(&kev->list, &events->free);
> +		events->nallocated++;
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_alloc);
> +
> +#define list_kfree(list, type, member)				\
> +	while (!list_empty(list)) {				\
> +		type *hi;					\
> +		hi = list_first_entry(list, type, member);	\
> +		list_del(&hi->member);				\
> +		kfree(hi);					\
> +	}
> +
> +void v4l2_event_free(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +
> +	if (!events)
> +		return;
> +
> +	list_kfree(&events->free, struct v4l2_kevent, list);
> +	list_kfree(&events->available, struct v4l2_kevent, list);
> +	list_kfree(&events->subscribed, struct v4l2_subscribed_event, list);
> +
> +	kfree(events);
> +	fh->events = NULL;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_free);
> +
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_kevent *kev;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (list_empty(&events->available)) {
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		return -ENOENT;
> +	}
> +
> +	WARN_ON(events->navailable == 0);
> +
> +	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
> +	list_move(&kev->list, &events->free);
> +	events->navailable--;
> +
> +	kev->event.pending = events->navailable;
> +	*event = kev->event;
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
> +
> +static struct v4l2_subscribed_event *v4l2_event_subscribed(
> +	struct v4l2_fh *fh, u32 type)

Add a comment before this function that mentions that fh->vdev->fh_lock must
be held before calling this.

> +{
> +	struct v4l2_events *events = fh->events;
> +	struct v4l2_subscribed_event *sev;
> +
> +	list_for_each_entry(sev, &events->subscribed, list) {
> +		if (sev->type == type)
> +			return sev;
> +	}
> +
> +	return NULL;
> +}
> +
> +void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
> +{
> +	struct v4l2_fh *fh;
> +	unsigned long flags;
> +	struct timespec timestamp;
> +
> +	ktime_get_ts(&timestamp);
> +
> +	spin_lock_irqsave(&vdev->fh_lock, flags);
> +
> +	list_for_each_entry(fh, &vdev->fh_list, list) {
> +		struct v4l2_events *events = fh->events;
> +		struct v4l2_kevent *kev;
> +		u32 sequence;
> +
> +		/* Are we subscribed? */
> +		if (!v4l2_event_subscribed(fh, ev->type))
> +			continue;
> +
> +		/* Increase event sequence number on fh. */
> +		events->sequence++;
> +		sequence = events->sequence;

There is no need for a temp sequence variable...

> +
> +		/* Do we have any free events? */
> +		if (list_empty(&events->free))
> +			continue;
> +
> +		/* Take one and fill it. */
> +		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
> +		kev->event.type = ev->type;
> +		kev->event.u = ev->u;
> +		kev->event.timestamp = timestamp;
> +		kev->event.sequence = sequence;

... you can just use events->sequence directly here.

> +		list_move_tail(&kev->list, &events->available);
> +
> +		events->navailable++;
> +
> +		wake_up_all(&events->wait);
> +	}
> +
> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
> +
> +int v4l2_event_pending(struct v4l2_fh *fh)
> +{
> +	return fh->events->navailable;
> +}
> +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 *sev;
> +	unsigned long flags;
> +

Add this:

	if (events == NULL) {
		/* If we get here, then the driver forgot to allocate events. */
		WARN_ON(1);
		return -ENOMEM;
	}

If subscribe is called without the event queue being allocated, then the
driver did something wrong.

See also my review for patch 5/6.

> +	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
> +	if (!sev)
> +		return -ENOMEM;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	if (v4l2_event_subscribed(fh, sub->type) == NULL) {
> +		INIT_LIST_HEAD(&sev->list);
> +		sev->type = sub->type;
> +
> +		list_add(&sev->list, &events->subscribed);
> +	}
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);

If the event was already subscribed, then you need to kfree the earlier
allocated sev here. Otherwise you have a memory leak.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
> +
> +/* subscribe a zero-terminated array of events */
> +int v4l2_event_subscribe_many(struct v4l2_fh *fh, const u32 *all)
> +{
> +	int ret;
> +
> +	for (; *all; all++) {
> +		struct v4l2_event_subscription sub;
> +
> +		sub.type = *all;
> +
> +		ret = v4l2_event_subscribe(fh, &sub);
> +		if (ret)
> +			return ret;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe_many);

Supporting V4L2_EVENT_ALL is a bad idea when subscribing. It sounds nice initially,
but the longer I think about it the more convinced I am we should not do this.
The main argument is really that it can lead to unexpected behavior. Suppose a
userspace application subscribes to all events. And in a later version of the
driver new events are added. Suddenly those will also arrive in the userspace app.

This might cause it to crash if it was written badly and it didn't check against
unknown events. Or this new event might be a high-volume event which might flood
the application unexpectedly.

In other words, it can create uncertain future behavior. So we really should
support EVENT_ALL only for unsubscribe: there it is very useful as a simple
way to 'reset' the filehandle's event handling.

> +
> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> +{
> +	struct v4l2_events *events = fh->events;
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	while (!list_empty(&events->subscribed)) {
> +		struct v4l2_subscribed_event *sev;
> +
> +		sev = list_first_entry(&events->subscribed,
> +				       struct v4l2_subscribed_event, list);
> +
> +		list_del(&sev->list);
> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +		kfree(sev);
> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +	}
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +}

What about this:

static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
{
	struct v4l2_events *events = fh->events;
	struct v4l2_subscribed_event *sev;
	unsigned long flags;

	do {
		sev = NULL;

		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
		if (!list_empty(&events->subscribed)) {
			sev = list_first_entry(&events->subscribed,
				       struct v4l2_subscribed_event, list);
			list_del(&sev->list);
		}
		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
		kfree(sev);
	} while (sev);
}

This avoids the 'interleaved' locking which I never like.

> +
> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
> +			   struct v4l2_event_subscription *sub)
> +{
> +	struct v4l2_subscribed_event *sev;
> +	unsigned long flags;
> +
> +	if (sub->type == V4L2_EVENT_ALL) {
> +		v4l2_event_unsubscribe_all(fh);
> +		return 0;
> +	}
> +
> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> +
> +	sev = v4l2_event_subscribed(fh, sub->type);
> +	if (sev != NULL)
> +		list_del(&sev->list);
> +
> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> +
> +	if (sev != NULL)
> +		kfree(sev);

No need for the NULL check. kfree(NULL) is allowed.

> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_event_unsubscribe);
> diff --git a/drivers/media/video/v4l2-fh.c b/drivers/media/video/v4l2-fh.c
> index c707930..3a9bc7d 100644
> --- a/drivers/media/video/v4l2-fh.c
> +++ b/drivers/media/video/v4l2-fh.c
> @@ -25,11 +25,13 @@
>  #include <linux/bitops.h>
>  #include <media/v4l2-dev.h>
>  #include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev)
>  {
>  	fh->vdev = vdev;
>  	INIT_LIST_HEAD(&fh->list);
> +	fh->events = NULL;
>  	set_bit(V4L2_FL_USES_V4L2_FH, &fh->vdev->flags);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_init);
> @@ -60,5 +62,7 @@ void v4l2_fh_exit(struct v4l2_fh *fh)
>  		return;
>  
>  	fh->vdev = NULL;
> +
> +	v4l2_event_free(fh);
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fh_exit);
> diff --git a/include/media/v4l2-event.h b/include/media/v4l2-event.h
> new file mode 100644
> index 0000000..284d495
> --- /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_kevent {
> +	struct list_head	list;
> +	struct v4l2_event	event;
> +};
> +
> +struct v4l2_subscribed_event {
> +	struct list_head	list;
> +	u32			type;
> +};
> +
> +struct v4l2_events {
> +	wait_queue_head_t	wait;
> +	struct list_head	subscribed; /* Subscribed events */
> +	struct list_head	available; /* Dequeueable event */
> +	unsigned int		navailable;
> +	struct list_head	free; /* Events ready for use */

I would move this between the subscribed and available lists. Purely for
grouping the lists together.

> +	unsigned int		nallocated; /* Number of allocated events */
> +	u32			sequence;
> +};
> +
> +int v4l2_event_alloc(struct v4l2_fh *fh, unsigned int n);
> +void v4l2_event_free(struct v4l2_fh *fh);
> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event);
> +void v4l2_event_queue(struct video_device *vdev, const 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_subscribe_many(struct v4l2_fh *fh, const u32 *all);
> +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 6b486aa..6c9df56 100644
> --- a/include/media/v4l2-fh.h
> +++ b/include/media/v4l2-fh.h
> @@ -28,10 +28,12 @@
>  #include <linux/list.h>
>  
>  struct video_device;
> +struct v4l2_events;
>  
>  struct v4l2_fh {
>  	struct list_head	list;
>  	struct video_device	*vdev;
> +	struct v4l2_events      *events; /* events, pending and subscribed */
>  };
>  
>  void v4l2_fh_init(struct v4l2_fh *fh, struct video_device *vdev);
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v5 4/6] V4L: Events: Add backend

[Sakari Ailus]

Hans Verkuil wrote:
> Hi Sakari,

Hi Hans,

And many thanks for the comments again!

> Here are some more comments.
> 
...
>> +int v4l2_event_dequeue(struct v4l2_fh *fh, struct v4l2_event *event)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	struct v4l2_kevent *kev;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	if (list_empty(&events->available)) {
>> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +		return -ENOENT;
>> +	}
>> +
>> +	WARN_ON(events->navailable == 0);
>> +
>> +	kev = list_first_entry(&events->available, struct v4l2_kevent, list);
>> +	list_move(&kev->list, &events->free);
>> +	events->navailable--;
>> +
>> +	kev->event.pending = events->navailable;
>> +	*event = kev->event;
>> +
>> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_dequeue);
>> +
>> +static struct v4l2_subscribed_event *v4l2_event_subscribed(
>> +	struct v4l2_fh *fh, u32 type)
> 
> Add a comment before this function that mentions that fh->vdev->fh_lock must
> be held before calling this.

Ack.

I'll add WARN_ON(!lock_acquired()) as well.

>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	struct v4l2_subscribed_event *sev;
>> +
>> +	list_for_each_entry(sev, &events->subscribed, list) {
>> +		if (sev->type == type)
>> +			return sev;
>> +	}
>> +
>> +	return NULL;
>> +}
>> +
>> +void v4l2_event_queue(struct video_device *vdev, const struct v4l2_event *ev)
>> +{
>> +	struct v4l2_fh *fh;
>> +	unsigned long flags;
>> +	struct timespec timestamp;
>> +
>> +	ktime_get_ts(&timestamp);
>> +
>> +	spin_lock_irqsave(&vdev->fh_lock, flags);
>> +
>> +	list_for_each_entry(fh, &vdev->fh_list, list) {
>> +		struct v4l2_events *events = fh->events;
>> +		struct v4l2_kevent *kev;
>> +		u32 sequence;
>> +
>> +		/* Are we subscribed? */
>> +		if (!v4l2_event_subscribed(fh, ev->type))
>> +			continue;
>> +
>> +		/* Increase event sequence number on fh. */
>> +		events->sequence++;
>> +		sequence = events->sequence;
> 
> There is no need for a temp sequence variable...

Good point.

>> +
>> +		/* Do we have any free events? */
>> +		if (list_empty(&events->free))
>> +			continue;
>> +
>> +		/* Take one and fill it. */
>> +		kev = list_first_entry(&events->free, struct v4l2_kevent, list);
>> +		kev->event.type = ev->type;
>> +		kev->event.u = ev->u;
>> +		kev->event.timestamp = timestamp;
>> +		kev->event.sequence = sequence;
> 
> ... you can just use events->sequence directly here.
> 
>> +		list_move_tail(&kev->list, &events->available);
>> +
>> +		events->navailable++;
>> +
>> +		wake_up_all(&events->wait);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&vdev->fh_lock, flags);
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_queue);
>> +
>> +int v4l2_event_pending(struct v4l2_fh *fh)
>> +{
>> +	return fh->events->navailable;
>> +}
>> +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 *sev;
>> +	unsigned long flags;
>> +
> 
> Add this:
> 
> 	if (events == NULL) {
> 		/* If we get here, then the driver forgot to allocate events. */
> 		WARN_ON(1);
> 		return -ENOMEM;
> 	}
> 
> If subscribe is called without the event queue being allocated, then the
> driver did something wrong.

Ok.

> See also my review for patch 5/6.
> 
>> +	sev = kmalloc(sizeof(*sev), GFP_KERNEL);
>> +	if (!sev)
>> +		return -ENOMEM;
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	if (v4l2_event_subscribed(fh, sub->type) == NULL) {
>> +		INIT_LIST_HEAD(&sev->list);
>> +		sev->type = sub->type;
>> +
>> +		list_add(&sev->list, &events->subscribed);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 
> If the event was already subscribed, then you need to kfree the earlier
> allocated sev here. Otherwise you have a memory leak.

Thanks for catching this!

No matter how many times you read the code through yourself this kind of
bugs may still lie there.

>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe);
>> +
>> +/* subscribe a zero-terminated array of events */
>> +int v4l2_event_subscribe_many(struct v4l2_fh *fh, const u32 *all)
>> +{
>> +	int ret;
>> +
>> +	for (; *all; all++) {
>> +		struct v4l2_event_subscription sub;
>> +
>> +		sub.type = *all;
>> +
>> +		ret = v4l2_event_subscribe(fh, &sub);
>> +		if (ret)
>> +			return ret;
>> +	}
>> +
>> +	return 0;
>> +}
>> +EXPORT_SYMBOL_GPL(v4l2_event_subscribe_many);
> 
> Supporting V4L2_EVENT_ALL is a bad idea when subscribing. It sounds nice initially,
> but the longer I think about it the more convinced I am we should not do this.
> The main argument is really that it can lead to unexpected behavior. Suppose a
> userspace application subscribes to all events. And in a later version of the
> driver new events are added. Suddenly those will also arrive in the userspace app.
> 
> This might cause it to crash if it was written badly and it didn't check against
> unknown events. Or this new event might be a high-volume event which might flood
> the application unexpectedly.

Many events are more or less related to the drivers. If an application
is unable to cope with events it has subscribed then it's an application
problem IMO.

I'm fine with dropping V4L2_EVENT_ALL in subscription. It can always be
added later if it's ever required.

>> +
>> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
>> +{
>> +	struct v4l2_events *events = fh->events;
>> +	unsigned long flags;
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	while (!list_empty(&events->subscribed)) {
>> +		struct v4l2_subscribed_event *sev;
>> +
>> +		sev = list_first_entry(&events->subscribed,
>> +				       struct v4l2_subscribed_event, list);
>> +
>> +		list_del(&sev->list);
>> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +		kfree(sev);
>> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +	}
>> +
>> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +}
> 
> What about this:
> 
> static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> {
> 	struct v4l2_events *events = fh->events;
> 	struct v4l2_subscribed_event *sev;
> 	unsigned long flags;
> 
> 	do {
> 		sev = NULL;
> 
> 		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> 		if (!list_empty(&events->subscribed)) {
> 			sev = list_first_entry(&events->subscribed,
> 				       struct v4l2_subscribed_event, list);
> 			list_del(&sev->list);
> 		}
> 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> 		kfree(sev);
> 	} while (sev);
> }
> 
> This avoids the 'interleaved' locking which I never like.

Can do. I don't see anything bad in that kind of locking, though. ;-)

>> +
>> +int v4l2_event_unsubscribe(struct v4l2_fh *fh,
>> +			   struct v4l2_event_subscription *sub)
>> +{
>> +	struct v4l2_subscribed_event *sev;
>> +	unsigned long flags;
>> +
>> +	if (sub->type == V4L2_EVENT_ALL) {
>> +		v4l2_event_unsubscribe_all(fh);
>> +		return 0;
>> +	}
>> +
>> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
>> +
>> +	sev = v4l2_event_subscribed(fh, sub->type);
>> +	if (sev != NULL)
>> +		list_del(&sev->list);
>> +
>> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
>> +
>> +	if (sev != NULL)
>> +		kfree(sev);
> 
> No need for the NULL check. kfree(NULL) is allowed.

Done.

>> +struct v4l2_events {
>> +	wait_queue_head_t	wait;
>> +	struct list_head	subscribed; /* Subscribed events */
>> +	struct list_head	available; /* Dequeueable event */
>> +	unsigned int		navailable;
>> +	struct list_head	free; /* Events ready for use */
> 
> I would move this between the subscribed and available lists. Purely for
> grouping the lists together.

Done.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v5 4/6] V4L: Events: Add backend

[Hans Verkuil]

On Sunday 21 February 2010 21:57:47 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > Hi Sakari,
> 
> Hi Hans,
> 
> And many thanks for the comments again!
> 
> > Here are some more comments.
> > 
> ...

> >> +
> >> +static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> >> +{
> >> +	struct v4l2_events *events = fh->events;
> >> +	unsigned long flags;
> >> +
> >> +	spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >> +
> >> +	while (!list_empty(&events->subscribed)) {
> >> +		struct v4l2_subscribed_event *sev;
> >> +
> >> +		sev = list_first_entry(&events->subscribed,
> >> +				       struct v4l2_subscribed_event, list);
> >> +
> >> +		list_del(&sev->list);
> >> +		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >> +		kfree(sev);
> >> +		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> >> +	}
> >> +
> >> +	spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> >> +}
> > 
> > What about this:
> > 
> > static void v4l2_event_unsubscribe_all(struct v4l2_fh *fh)
> > {
> > 	struct v4l2_events *events = fh->events;
> > 	struct v4l2_subscribed_event *sev;
> > 	unsigned long flags;
> > 
> > 	do {
> > 		sev = NULL;
> > 
> > 		spin_lock_irqsave(&fh->vdev->fh_lock, flags);
> > 		if (!list_empty(&events->subscribed)) {
> > 			sev = list_first_entry(&events->subscribed,
> > 				       struct v4l2_subscribed_event, list);
> > 			list_del(&sev->list);
> > 		}
> > 		spin_unlock_irqrestore(&fh->vdev->fh_lock, flags);
> > 		kfree(sev);
> > 	} while (sev);
> > }
> > 
> > This avoids the 'interleaved' locking which I never like.
> 
> Can do. I don't see anything bad in that kind of locking, though. ;-)

Locking is hard to get right. So it is important to keep the locking code as
straightforward as possible. In the original code you really have to look
closely at the code to check that the locking is correct. In the rewritten
code it is much more obvious because of the simplified control flow.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

[PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Sakari Ailus]

Add support for event handling to do_ioctl.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 drivers/media/video/v4l2-ioctl.c |   58 ++++++++++++++++++++++++++++++++++++++
 include/media/v4l2-ioctl.h       |    7 ++++
 2 files changed, 65 insertions(+), 0 deletions(-)

diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
index 34c7d6e..f7d6177 100644
--- a/drivers/media/video/v4l2-ioctl.c
+++ b/drivers/media/video/v4l2-ioctl.c
@@ -25,6 +25,8 @@
 #endif
 #include <media/v4l2-common.h>
 #include <media/v4l2-ioctl.h>
+#include <media/v4l2-fh.h>
+#include <media/v4l2-event.h>
 #include <media/v4l2-chip-ident.h>
 
 #define dbgarg(cmd, fmt, arg...) \
@@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
 		}
 		break;
 	}
+	case VIDIOC_DQEVENT:
+	{
+		struct v4l2_event *ev = arg;
+		struct v4l2_fh *vfh = fh;
+
+		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
+		    || vfh->events == NULL)
+			break;
+
+		ret = v4l2_event_dequeue(fh, ev);
+		if (ret < 0) {
+			dbgarg(cmd, "no pending events?");
+			break;
+		}
+		dbgarg(cmd,
+		       "pending=%d, type=0x%8.8x, sequence=%d, "
+		       "timestamp=%lu.%9.9lu ",
+		       ev->pending, ev->type, ev->sequence,
+		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
+		break;
+	}
+	case VIDIOC_SUBSCRIBE_EVENT:
+	{
+		struct v4l2_event_subscription *sub = arg;
+		struct v4l2_fh *vfh = fh;
 
+		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
+		    || vfh->events == NULL
+		    || !ops->vidioc_subscribe_event)
+			break;
+
+		ret = ops->vidioc_subscribe_event(fh, sub);
+		if (ret < 0) {
+			dbgarg(cmd, "failed, ret=%ld", ret);
+			break;
+		}
+		dbgarg(cmd, "type=0x%8.8x", sub->type);
+		break;
+	}
+	case VIDIOC_UNSUBSCRIBE_EVENT:
+	{
+		struct v4l2_event_subscription *sub = arg;
+		struct v4l2_fh *vfh = fh;
+
+		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
+		    || vfh->events == NULL
+		    || !ops->vidioc_unsubscribe_event)
+			break;
+
+		ret = ops->vidioc_unsubscribe_event(fh, sub);
+		if (ret < 0) {
+			dbgarg(cmd, "failed, ret=%ld", ret);
+			break;
+		}
+		dbgarg(cmd, "type=0x%8.8x", sub->type);
+		break;
+	}
 	default:
 	{
 		if (!ops->vidioc_default)
diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
index e8ba0f2..06daa6e 100644
--- a/include/media/v4l2-ioctl.h
+++ b/include/media/v4l2-ioctl.h
@@ -21,6 +21,8 @@
 #include <linux/videodev2.h>
 #endif
 
+struct v4l2_fh;
+
 struct v4l2_ioctl_ops {
 	/* ioctl callbacks */
 
@@ -254,6 +256,11 @@ struct v4l2_ioctl_ops {
 	int (*vidioc_g_dv_timings) (struct file *file, void *fh,
 				    struct v4l2_dv_timings *timings);
 
+	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
+					struct v4l2_event_subscription *sub);
+	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
+					struct v4l2_event_subscription *sub);
+
 	/* For other private ioctls */
 	long (*vidioc_default)	       (struct file *file, void *fh,
 					int cmd, void *arg);
-- 
1.5.6.5

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Hans Verkuil]

More comments...

On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
> Add support for event handling to do_ioctl.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  drivers/media/video/v4l2-ioctl.c |   58 ++++++++++++++++++++++++++++++++++++++
>  include/media/v4l2-ioctl.h       |    7 ++++
>  2 files changed, 65 insertions(+), 0 deletions(-)
> 
> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> index 34c7d6e..f7d6177 100644
> --- a/drivers/media/video/v4l2-ioctl.c
> +++ b/drivers/media/video/v4l2-ioctl.c
> @@ -25,6 +25,8 @@
>  #endif
>  #include <media/v4l2-common.h>
>  #include <media/v4l2-ioctl.h>
> +#include <media/v4l2-fh.h>
> +#include <media/v4l2-event.h>
>  #include <media/v4l2-chip-ident.h>
>  
>  #define dbgarg(cmd, fmt, arg...) \
> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
>  		}
>  		break;
>  	}
> +	case VIDIOC_DQEVENT:
> +	{
> +		struct v4l2_event *ev = arg;
> +		struct v4l2_fh *vfh = fh;
> +
> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> +		    || vfh->events == NULL)
> +			break;

Change this to:

		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
			break;
		if (vfh->events == NULL)
			return -ENOENT;

But see also the next comment.

> +
> +		ret = v4l2_event_dequeue(fh, ev);

There is a crucial piece of functionality missing here: if the filehandle is
in blocking mode, then it should wait until an event arrives. That also means
that if vfh->events == NULL, you should still call v4l2_event_dequeue, and
that function should initialize vfh->events and wait for an event if the fh
is in blocking mode.

So I would remove the events == NULL test here and just call v4l2_event_dequeue
and let that function handle it.

> +		if (ret < 0) {
> +			dbgarg(cmd, "no pending events?");
> +			break;
> +		}
> +		dbgarg(cmd,
> +		       "pending=%d, type=0x%8.8x, sequence=%d, "
> +		       "timestamp=%lu.%9.9lu ",
> +		       ev->pending, ev->type, ev->sequence,
> +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> +		break;
> +	}
> +	case VIDIOC_SUBSCRIBE_EVENT:
> +	{
> +		struct v4l2_event_subscription *sub = arg;
> +		struct v4l2_fh *vfh = fh;
>  
> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)

Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event.

> +		    || vfh->events == NULL

Remove this test. If you allocate the event queue only when you first
subscribe to an event (as ivtv will do), then you have to be able to
call vidioc_subscribe_event even if vfh->events == NULL.

> +		    || !ops->vidioc_subscribe_event)
> +			break;
> +
> +		ret = ops->vidioc_subscribe_event(fh, sub);
> +		if (ret < 0) {
> +			dbgarg(cmd, "failed, ret=%ld", ret);
> +			break;
> +		}
> +		dbgarg(cmd, "type=0x%8.8x", sub->type);
> +		break;
> +	}
> +	case VIDIOC_UNSUBSCRIBE_EVENT:
> +	{
> +		struct v4l2_event_subscription *sub = arg;
> +		struct v4l2_fh *vfh = fh;
> +
> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> +		    || vfh->events == NULL
> +		    || !ops->vidioc_unsubscribe_event)

No need for the test_bit. This is sufficient:

		if (!ops->vidioc_unsubscribe_event ||
		    vfh->events == NULL)

Note that I am not so keen on testing against vfh->events here. I consider
this a v4l2_fh-internal field that should not be used outside the v4l2_event_
and v4l2_fh_ functions.

> +			break;
> +
> +		ret = ops->vidioc_unsubscribe_event(fh, sub);
> +		if (ret < 0) {
> +			dbgarg(cmd, "failed, ret=%ld", ret);
> +			break;
> +		}
> +		dbgarg(cmd, "type=0x%8.8x", sub->type);
> +		break;
> +	}
>  	default:
>  	{
>  		if (!ops->vidioc_default)
> diff --git a/include/media/v4l2-ioctl.h b/include/media/v4l2-ioctl.h
> index e8ba0f2..06daa6e 100644
> --- a/include/media/v4l2-ioctl.h
> +++ b/include/media/v4l2-ioctl.h
> @@ -21,6 +21,8 @@
>  #include <linux/videodev2.h>
>  #endif
>  
> +struct v4l2_fh;
> +
>  struct v4l2_ioctl_ops {
>  	/* ioctl callbacks */
>  
> @@ -254,6 +256,11 @@ struct v4l2_ioctl_ops {
>  	int (*vidioc_g_dv_timings) (struct file *file, void *fh,
>  				    struct v4l2_dv_timings *timings);
>  
> +	int (*vidioc_subscribe_event)  (struct v4l2_fh *fh,
> +					struct v4l2_event_subscription *sub);
> +	int (*vidioc_unsubscribe_event)(struct v4l2_fh *fh,
> +					struct v4l2_event_subscription *sub);
> +
>  	/* For other private ioctls */
>  	long (*vidioc_default)	       (struct file *file, void *fh,
>  					int cmd, void *arg);
> 

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Sakari Ailus]

Hans Verkuil wrote:
> More comments...
> 
> On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
>> Add support for event handling to do_ioctl.
>>
>> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
>> ---
>>  drivers/media/video/v4l2-ioctl.c |   58 ++++++++++++++++++++++++++++++++++++++
>>  include/media/v4l2-ioctl.h       |    7 ++++
>>  2 files changed, 65 insertions(+), 0 deletions(-)
>>
>> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
>> index 34c7d6e..f7d6177 100644
>> --- a/drivers/media/video/v4l2-ioctl.c
>> +++ b/drivers/media/video/v4l2-ioctl.c
>> @@ -25,6 +25,8 @@
>>  #endif
>>  #include <media/v4l2-common.h>
>>  #include <media/v4l2-ioctl.h>
>> +#include <media/v4l2-fh.h>
>> +#include <media/v4l2-event.h>
>>  #include <media/v4l2-chip-ident.h>
>>  
>>  #define dbgarg(cmd, fmt, arg...) \
>> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
>>  		}
>>  		break;
>>  	}
>> +	case VIDIOC_DQEVENT:
>> +	{
>> +		struct v4l2_event *ev = arg;
>> +		struct v4l2_fh *vfh = fh;
>> +
>> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
>> +		    || vfh->events == NULL)
>> +			break;
> 
> Change this to:
> 
> 		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> 			break;
> 		if (vfh->events == NULL)
> 			return -ENOENT;
> 
> But see also the next comment.
> 
>> +
>> +		ret = v4l2_event_dequeue(fh, ev);
> 
> There is a crucial piece of functionality missing here: if the filehandle is
> in blocking mode, then it should wait until an event arrives. That also means
> that if vfh->events == NULL, you should still call v4l2_event_dequeue, and
> that function should initialize vfh->events and wait for an event if the fh
> is in blocking mode.

I originally left this out intentionally. Most applications using events
would use select / poll as well by default. For completeness it should
be there, I agree.

This btw. suggests that we perhaps should put back the struct file
argument for the event functions in video_ioctl_ops. The blocking flag
is indeed part of the file structure. I'm open to better suggestions, too.

>> +		if (ret < 0) {
>> +			dbgarg(cmd, "no pending events?");
>> +			break;
>> +		}
>> +		dbgarg(cmd,
>> +		       "pending=%d, type=0x%8.8x, sequence=%d, "
>> +		       "timestamp=%lu.%9.9lu ",
>> +		       ev->pending, ev->type, ev->sequence,
>> +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
>> +		break;
>> +	}
>> +	case VIDIOC_SUBSCRIBE_EVENT:
>> +	{
>> +		struct v4l2_event_subscription *sub = arg;
>> +		struct v4l2_fh *vfh = fh;
>>  
>> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> 
> Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event.
> 
>> +		    || vfh->events == NULL
> 
> Remove this test. If you allocate the event queue only when you first
> subscribe to an event (as ivtv will do), then you have to be able to
> call vidioc_subscribe_event even if vfh->events == NULL.

How about calling v4l2_event_alloc() with zero events? That allocates
and initialises the v4l2_events structure. That's easier to handle in
drivers as well since they don't need to consider special cases like
fh->events happens to be NULL even if events are supported by the
driver. This is how I first thought it'd work.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Laurent Pinchart]

Hi Sakari,

On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > More comments...
> > 
> > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
> >> Add support for event handling to do_ioctl.
> >> 
> >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> >> ---
> >> 
> >>  drivers/media/video/v4l2-ioctl.c |   58
> >>  ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h     
> >>   |    7 ++++
> >>  2 files changed, 65 insertions(+), 0 deletions(-)
> >> 
> >> diff --git a/drivers/media/video/v4l2-ioctl.c
> >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644
> >> --- a/drivers/media/video/v4l2-ioctl.c
> >> +++ b/drivers/media/video/v4l2-ioctl.c
> >> @@ -25,6 +25,8 @@
> >> 
> >>  #endif
> >>  #include <media/v4l2-common.h>
> >>  #include <media/v4l2-ioctl.h>
> >> 
> >> +#include <media/v4l2-fh.h>
> >> +#include <media/v4l2-event.h>
> >> 
> >>  #include <media/v4l2-chip-ident.h>
> >>  
> >>  #define dbgarg(cmd, fmt, arg...) \
> >> 
> >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
> >> 
> >>  		}
> >>  		break;
> >>  	
> >>  	}
> >> 
> >> +	case VIDIOC_DQEVENT:
> >> +	{
> >> +		struct v4l2_event *ev = arg;
> >> +		struct v4l2_fh *vfh = fh;
> >> +
> >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> >> +		    || vfh->events == NULL)
> >> +			break;
> > 
> > Change this to:
> > 		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > 		
> > 			break;
> > 		
> > 		if (vfh->events == NULL)
> > 		
> > 			return -ENOENT;
> > 
> > But see also the next comment.
> > 
> >> +
> >> +		ret = v4l2_event_dequeue(fh, ev);
> > 
> > There is a crucial piece of functionality missing here: if the filehandle
> > is in blocking mode, then it should wait until an event arrives. That
> > also means that if vfh->events == NULL, you should still call
> > v4l2_event_dequeue, and that function should initialize vfh->events and
> > wait for an event if the fh is in blocking mode.
> 
> I originally left this out intentionally. Most applications using events
> would use select / poll as well by default. For completeness it should
> be there, I agree.
> 
> This btw. suggests that we perhaps should put back the struct file
> argument for the event functions in video_ioctl_ops. The blocking flag
> is indeed part of the file structure. I'm open to better suggestions, too.

If the only information we need from struct file is the flags, they could be 
copied to v4l2_fh in the open handler. We could also put a struct file * 
member in v4l2_fh.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Sakari Ailus]

Laurent Pinchart wrote:
> Hi Sakari,

Salut Laurent,

>>> There is a crucial piece of functionality missing here: if the filehandle
>>> is in blocking mode, then it should wait until an event arrives. That
>>> also means that if vfh->events == NULL, you should still call
>>> v4l2_event_dequeue, and that function should initialize vfh->events and
>>> wait for an event if the fh is in blocking mode.
>>
>> I originally left this out intentionally. Most applications using events
>> would use select / poll as well by default. For completeness it should
>> be there, I agree.
>>
>> This btw. suggests that we perhaps should put back the struct file
>> argument for the event functions in video_ioctl_ops. The blocking flag
>> is indeed part of the file structure. I'm open to better suggestions, too.
> 
> If the only information we need from struct file is the flags, they could be 
> copied to v4l2_fh in the open handler. We could also put a struct file * 
> member in v4l2_fh.

That could be one possibility. Copying the flags in open() isn't enough
as they can change as a result of fcntl call. As we're not handling that
call the flags in struct v4l2_fh would have to be updated for every
ioctl. I'm not a big fan of caching information in general either. :-) I
could accept this, though.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Hans Verkuil]

On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > More comments...
> > 
> > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
> >> Add support for event handling to do_ioctl.
> >>
> >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> >> ---
> >>  drivers/media/video/v4l2-ioctl.c |   58 ++++++++++++++++++++++++++++++++++++++
> >>  include/media/v4l2-ioctl.h       |    7 ++++
> >>  2 files changed, 65 insertions(+), 0 deletions(-)
> >>
> >> diff --git a/drivers/media/video/v4l2-ioctl.c b/drivers/media/video/v4l2-ioctl.c
> >> index 34c7d6e..f7d6177 100644
> >> --- a/drivers/media/video/v4l2-ioctl.c
> >> +++ b/drivers/media/video/v4l2-ioctl.c
> >> @@ -25,6 +25,8 @@
> >>  #endif
> >>  #include <media/v4l2-common.h>
> >>  #include <media/v4l2-ioctl.h>
> >> +#include <media/v4l2-fh.h>
> >> +#include <media/v4l2-event.h>
> >>  #include <media/v4l2-chip-ident.h>
> >>  
> >>  #define dbgarg(cmd, fmt, arg...) \
> >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
> >>  		}
> >>  		break;
> >>  	}
> >> +	case VIDIOC_DQEVENT:
> >> +	{
> >> +		struct v4l2_event *ev = arg;
> >> +		struct v4l2_fh *vfh = fh;
> >> +
> >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> >> +		    || vfh->events == NULL)
> >> +			break;
> > 
> > Change this to:
> > 
> > 		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > 			break;
> > 		if (vfh->events == NULL)
> > 			return -ENOENT;
> > 
> > But see also the next comment.
> > 
> >> +
> >> +		ret = v4l2_event_dequeue(fh, ev);
> > 
> > There is a crucial piece of functionality missing here: if the filehandle is
> > in blocking mode, then it should wait until an event arrives. That also means
> > that if vfh->events == NULL, you should still call v4l2_event_dequeue, and
> > that function should initialize vfh->events and wait for an event if the fh
> > is in blocking mode.
> 
> I originally left this out intentionally. Most applications using events
> would use select / poll as well by default. For completeness it should
> be there, I agree.

It has to be there. This is important functionality. For e.g. ivtv I would use
this to wait until the MPEG decoder flushed all buffers and displayed the last
frame of the stream. That's something you would often do in blocking mode.

> This btw. suggests that we perhaps should put back the struct file
> argument for the event functions in video_ioctl_ops. The blocking flag
> is indeed part of the file structure. I'm open to better suggestions, too.

My long term goal is that the file struct is only used inside v4l2-ioctl.c
and not in drivers. Drivers should not need this struct at all. The easiest
way to ensure this is by not passing it to the drivers at all :-)
 
> >> +		if (ret < 0) {
> >> +			dbgarg(cmd, "no pending events?");
> >> +			break;
> >> +		}
> >> +		dbgarg(cmd,
> >> +		       "pending=%d, type=0x%8.8x, sequence=%d, "
> >> +		       "timestamp=%lu.%9.9lu ",
> >> +		       ev->pending, ev->type, ev->sequence,
> >> +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> >> +		break;
> >> +	}
> >> +	case VIDIOC_SUBSCRIBE_EVENT:
> >> +	{
> >> +		struct v4l2_event_subscription *sub = arg;
> >> +		struct v4l2_fh *vfh = fh;
> >>  
> >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> > 
> > Testing for this bit is unnecessarily. Just test for ops->vidioc_subscribe_event.
> > 
> >> +		    || vfh->events == NULL
> > 
> > Remove this test. If you allocate the event queue only when you first
> > subscribe to an event (as ivtv will do), then you have to be able to
> > call vidioc_subscribe_event even if vfh->events == NULL.
> 
> How about calling v4l2_event_alloc() with zero events? That allocates
> and initialises the v4l2_events structure. That's easier to handle in
> drivers as well since they don't need to consider special cases like
> fh->events happens to be NULL even if events are supported by the
> driver. This is how I first thought it'd work.

Proposal: export a v4l2_event_init() call that sets up fh->events. Calling
v4l2_event_alloc(0) feels like a hack. So drivers that want to be able to
handle events should call v4l2_event_init after initializing the file handle.

Or (and that might even be nicer) test in v4l2_fh_init whether there is a
subscribe op in the ioctl_ops struct and let v4l2_fh_init set up fh->events
automatically.

Regards,

	Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Laurent Pinchart]

Hi Hans,

On Monday 22 February 2010 08:53:53 Hans Verkuil wrote:
> On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote:
> > Hans Verkuil wrote:
> > > More comments...
> > > 
> > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
> > >> Add support for event handling to do_ioctl.
> > >> 
> > >> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> > >> ---
> > >> 
> > >>  drivers/media/video/v4l2-ioctl.c |   58
> > >>  ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h   
> > >>     |    7 ++++
> > >>  2 files changed, 65 insertions(+), 0 deletions(-)
> > >> 
> > >> diff --git a/drivers/media/video/v4l2-ioctl.c
> > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644
> > >> --- a/drivers/media/video/v4l2-ioctl.c
> > >> +++ b/drivers/media/video/v4l2-ioctl.c
> > >> @@ -25,6 +25,8 @@
> > >> 
> > >>  #endif
> > >>  #include <media/v4l2-common.h>
> > >>  #include <media/v4l2-ioctl.h>
> > >> 
> > >> +#include <media/v4l2-fh.h>
> > >> +#include <media/v4l2-event.h>
> > >> 
> > >>  #include <media/v4l2-chip-ident.h>
> > >>  
> > >>  #define dbgarg(cmd, fmt, arg...) \
> > >> 
> > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file *file,
> > >> 
> > >>  		}
> > >>  		break;
> > >>  	
> > >>  	}
> > >> 
> > >> +	case VIDIOC_DQEVENT:
> > >> +	{
> > >> +		struct v4l2_event *ev = arg;
> > >> +		struct v4l2_fh *vfh = fh;
> > >> +
> > >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> > >> +		    || vfh->events == NULL)
> > >> +			break;
> > > 
> > > Change this to:
> > > 		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
> > > 		
> > > 			break;
> > > 		
> > > 		if (vfh->events == NULL)
> > > 		
> > > 			return -ENOENT;
> > > 
> > > But see also the next comment.
> > > 
> > >> +
> > >> +		ret = v4l2_event_dequeue(fh, ev);
> > > 
> > > There is a crucial piece of functionality missing here: if the
> > > filehandle is in blocking mode, then it should wait until an event
> > > arrives. That also means that if vfh->events == NULL, you should still
> > > call v4l2_event_dequeue, and that function should initialize
> > > vfh->events and wait for an event if the fh is in blocking mode.
> > 
> > I originally left this out intentionally. Most applications using events
> > would use select / poll as well by default. For completeness it should
> > be there, I agree.
> 
> It has to be there. This is important functionality. For e.g. ivtv I would
> use this to wait until the MPEG decoder flushed all buffers and displayed
> the last frame of the stream. That's something you would often do in
> blocking mode.

Blocking mode can easily be emulated using select().

> > This btw. suggests that we perhaps should put back the struct file
> > argument for the event functions in video_ioctl_ops. The blocking flag
> > is indeed part of the file structure. I'm open to better suggestions,
> > too.
> 
> My long term goal is that the file struct is only used inside v4l2-ioctl.c
> and not in drivers. Drivers should not need this struct at all. The easiest
> way to ensure this is by not passing it to the drivers at all :-)

Drivers still need a way to access the blocking flag. The interim solution of 
adding a file * member to v4l2_fh would allow that, while still removing most 
usage of file * from drivers.

> > >> +		if (ret < 0) {
> > >> +			dbgarg(cmd, "no pending events?");
> > >> +			break;
> > >> +		}
> > >> +		dbgarg(cmd,
> > >> +		       "pending=%d, type=0x%8.8x, sequence=%d, "
> > >> +		       "timestamp=%lu.%9.9lu ",
> > >> +		       ev->pending, ev->type, ev->sequence,
> > >> +		       ev->timestamp.tv_sec, ev->timestamp.tv_nsec);
> > >> +		break;
> > >> +	}
> > >> +	case VIDIOC_SUBSCRIBE_EVENT:
> > >> +	{
> > >> +		struct v4l2_event_subscription *sub = arg;
> > >> +		struct v4l2_fh *vfh = fh;
> > >> 
> > >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
> > > 
> > > Testing for this bit is unnecessarily. Just test for
> > > ops->vidioc_subscribe_event.
> > > 
> > >> +		    || vfh->events == NULL
> > > 
> > > Remove this test. If you allocate the event queue only when you first
> > > subscribe to an event (as ivtv will do), then you have to be able to
> > > call vidioc_subscribe_event even if vfh->events == NULL.
> > 
> > How about calling v4l2_event_alloc() with zero events? That allocates
> > and initialises the v4l2_events structure. That's easier to handle in
> > drivers as well since they don't need to consider special cases like
> > fh->events happens to be NULL even if events are supported by the
> > driver. This is how I first thought it'd work.
> 
> Proposal: export a v4l2_event_init() call that sets up fh->events. Calling
> v4l2_event_alloc(0) feels like a hack. So drivers that want to be able to
> handle events should call v4l2_event_init after initializing the file
> handle.
> 
> Or (and that might even be nicer) test in v4l2_fh_init whether there is a
> subscribe op in the ioctl_ops struct and let v4l2_fh_init set up fh->events
> automatically.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Hans Verkuil]

> Hi Hans,
>
> On Monday 22 February 2010 08:53:53 Hans Verkuil wrote:
>> On Sunday 21 February 2010 23:31:43 Sakari Ailus wrote:
>> > Hans Verkuil wrote:
>> > > More comments...
>> > >
>> > > On Friday 19 February 2010 20:21:59 Sakari Ailus wrote:
>> > >> Add support for event handling to do_ioctl.
>> > >>
>> > >> Signed-off-by: Sakari Ailus
>> <sakari.ailus@maxwell.research.nokia.com>
>> > >> ---
>> > >>
>> > >>  drivers/media/video/v4l2-ioctl.c |   58
>> > >>  ++++++++++++++++++++++++++++++++++++++ include/media/v4l2-ioctl.h
>> > >>     |    7 ++++
>> > >>  2 files changed, 65 insertions(+), 0 deletions(-)
>> > >>
>> > >> diff --git a/drivers/media/video/v4l2-ioctl.c
>> > >> b/drivers/media/video/v4l2-ioctl.c index 34c7d6e..f7d6177 100644
>> > >> --- a/drivers/media/video/v4l2-ioctl.c
>> > >> +++ b/drivers/media/video/v4l2-ioctl.c
>> > >> @@ -25,6 +25,8 @@
>> > >>
>> > >>  #endif
>> > >>  #include <media/v4l2-common.h>
>> > >>  #include <media/v4l2-ioctl.h>
>> > >>
>> > >> +#include <media/v4l2-fh.h>
>> > >> +#include <media/v4l2-event.h>
>> > >>
>> > >>  #include <media/v4l2-chip-ident.h>
>> > >>
>> > >>  #define dbgarg(cmd, fmt, arg...) \
>> > >>
>> > >> @@ -1944,7 +1946,63 @@ static long __video_do_ioctl(struct file
>> *file,
>> > >>
>> > >>  		}
>> > >>  		break;
>> > >>
>> > >>  	}
>> > >>
>> > >> +	case VIDIOC_DQEVENT:
>> > >> +	{
>> > >> +		struct v4l2_event *ev = arg;
>> > >> +		struct v4l2_fh *vfh = fh;
>> > >> +
>> > >> +		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags)
>> > >> +		    || vfh->events == NULL)
>> > >> +			break;
>> > >
>> > > Change this to:
>> > > 		if (!test_bit(V4L2_FL_USES_V4L2_FH, &vfd->flags))
>> > >
>> > > 			break;
>> > >
>> > > 		if (vfh->events == NULL)
>> > >
>> > > 			return -ENOENT;
>> > >
>> > > But see also the next comment.
>> > >
>> > >> +
>> > >> +		ret = v4l2_event_dequeue(fh, ev);
>> > >
>> > > There is a crucial piece of functionality missing here: if the
>> > > filehandle is in blocking mode, then it should wait until an event
>> > > arrives. That also means that if vfh->events == NULL, you should
>> still
>> > > call v4l2_event_dequeue, and that function should initialize
>> > > vfh->events and wait for an event if the fh is in blocking mode.
>> >
>> > I originally left this out intentionally. Most applications using
>> events
>> > would use select / poll as well by default. For completeness it should
>> > be there, I agree.
>>
>> It has to be there. This is important functionality. For e.g. ivtv I
>> would
>> use this to wait until the MPEG decoder flushed all buffers and
>> displayed
>> the last frame of the stream. That's something you would often do in
>> blocking mode.
>
> Blocking mode can easily be emulated using select().
>
>> > This btw. suggests that we perhaps should put back the struct file
>> > argument for the event functions in video_ioctl_ops. The blocking flag
>> > is indeed part of the file structure. I'm open to better suggestions,
>> > too.
>>
>> My long term goal is that the file struct is only used inside
>> v4l2-ioctl.c
>> and not in drivers. Drivers should not need this struct at all. The
>> easiest
>> way to ensure this is by not passing it to the drivers at all :-)
>
> Drivers still need a way to access the blocking flag. The interim solution
> of
> adding a file * member to v4l2_fh would allow that, while still removing
> most
> usage of file * from drivers.

Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let
v4l2-ioctl.c fill in that argument? That's how I would do it.

Regards,

        Hans

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG Telecom

Re: [PATCH v5 5/6] V4L: Events: Support event handling in do_ioctl

[Sakari Ailus]

Hans Verkuil wrote:
>>>>> There is a crucial piece of functionality missing here: if the
>>>>> filehandle is in blocking mode, then it should wait until an event
>>>>> arrives. That also means that if vfh->events == NULL, you should
>>> still
>>>>> call v4l2_event_dequeue, and that function should initialize
>>>>> vfh->events and wait for an event if the fh is in blocking mode.
>>>>
>>>> I originally left this out intentionally. Most applications using
>>> events
>>>> would use select / poll as well by default. For completeness it should
>>>> be there, I agree.
>>>
>>> It has to be there. This is important functionality. For e.g. ivtv I
>>> would
>>> use this to wait until the MPEG decoder flushed all buffers and
>>> displayed
>>> the last frame of the stream. That's something you would often do in
>>> blocking mode.
>>
>> Blocking mode can easily be emulated using select().

It's quite simple to implement still so I'll do that in the
VIDIOC_DQEVENT. Easier for applications anyway in use cases that I
haven't been thinking about, e.g. ivtv.

>>>> This btw. suggests that we perhaps should put back the struct file
>>>> argument for the event functions in video_ioctl_ops. The blocking flag
>>>> is indeed part of the file structure. I'm open to better suggestions,
>>>> too.
>>>
>>> My long term goal is that the file struct is only used inside
>>> v4l2-ioctl.c
>>> and not in drivers. Drivers should not need this struct at all. The
>>> easiest
>>> way to ensure this is by not passing it to the drivers at all :-)
>>
>> Drivers still need a way to access the blocking flag. The interim solution
>> of
>> adding a file * member to v4l2_fh would allow that, while still removing
>> most
>> usage of file * from drivers.
> 
> Why not just add a 'blocking' argument to the v4l2_event_dequeue? And let
> v4l2-ioctl.c fill in that argument? That's how I would do it.

Implemented already before reading your mail... :-) I'll try to repost
the patches today.

-- 
Sakari Ailus
sakari.ailus@maxwell.research.nokia.com

[PATCH v5 6/6] V4L: Events: Add documentation

[Sakari Ailus]

Add documentation on how to use V4L2 events, both for V4L2 drivers and for
V4L2 applications.

Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
---
 Documentation/DocBook/media-entities.tmpl          |    9 ++
 Documentation/DocBook/v4l/dev-event.xml            |   34 ++++++
 Documentation/DocBook/v4l/v4l2.xml                 |    3 +
 Documentation/DocBook/v4l/vidioc-dqevent.xml       |  124 ++++++++++++++++++++
 .../DocBook/v4l/vidioc-subscribe-event.xml         |  109 +++++++++++++++++
 Documentation/video4linux/v4l2-framework.txt       |   43 +++++++
 6 files changed, 322 insertions(+), 0 deletions(-)
 create mode 100644 Documentation/DocBook/v4l/dev-event.xml
 create mode 100644 Documentation/DocBook/v4l/vidioc-dqevent.xml
 create mode 100644 Documentation/DocBook/v4l/vidioc-subscribe-event.xml

diff --git a/Documentation/DocBook/media-entities.tmpl b/Documentation/DocBook/media-entities.tmpl
index c725cb8..770be3c 100644
--- a/Documentation/DocBook/media-entities.tmpl
+++ b/Documentation/DocBook/media-entities.tmpl
@@ -17,6 +17,7 @@
 <!ENTITY VIDIOC-DBG-G-REGISTER "<link linkend='vidioc-dbg-g-register'><constant>VIDIOC_DBG_G_REGISTER</constant></link>">
 <!ENTITY VIDIOC-DBG-S-REGISTER "<link linkend='vidioc-dbg-g-register'><constant>VIDIOC_DBG_S_REGISTER</constant></link>">
 <!ENTITY VIDIOC-DQBUF "<link linkend='vidioc-qbuf'><constant>VIDIOC_DQBUF</constant></link>">
+<!ENTITY VIDIOC-DQEVENT "<link linkend='vidioc-dqevent'><constant>VIDIOC_DQEVENT</constant></link>">
 <!ENTITY VIDIOC-ENCODER-CMD "<link linkend='vidioc-encoder-cmd'><constant>VIDIOC_ENCODER_CMD</constant></link>">
 <!ENTITY VIDIOC-ENUMAUDIO "<link linkend='vidioc-enumaudio'><constant>VIDIOC_ENUMAUDIO</constant></link>">
 <!ENTITY VIDIOC-ENUMAUDOUT "<link linkend='vidioc-enumaudioout'><constant>VIDIOC_ENUMAUDOUT</constant></link>">
@@ -60,6 +61,7 @@
 <!ENTITY VIDIOC-REQBUFS "<link linkend='vidioc-reqbufs'><constant>VIDIOC_REQBUFS</constant></link>">
 <!ENTITY VIDIOC-STREAMOFF "<link linkend='vidioc-streamon'><constant>VIDIOC_STREAMOFF</constant></link>">
 <!ENTITY VIDIOC-STREAMON "<link linkend='vidioc-streamon'><constant>VIDIOC_STREAMON</constant></link>">
+<!ENTITY VIDIOC-SUBSCRIBE-EVENT "<link linkend='vidioc-subscribe-event'><constant>VIDIOC_SUBSCRIBE_EVENT</constant></link>">
 <!ENTITY VIDIOC-S-AUDIO "<link linkend='vidioc-g-audio'><constant>VIDIOC_S_AUDIO</constant></link>">
 <!ENTITY VIDIOC-S-AUDOUT "<link linkend='vidioc-g-audioout'><constant>VIDIOC_S_AUDOUT</constant></link>">
 <!ENTITY VIDIOC-S-CROP "<link linkend='vidioc-g-crop'><constant>VIDIOC_S_CROP</constant></link>">
@@ -141,6 +143,8 @@
 <!ENTITY v4l2-enc-idx "struct&nbsp;<link linkend='v4l2-enc-idx'>v4l2_enc_idx</link>">
 <!ENTITY v4l2-enc-idx-entry "struct&nbsp;<link linkend='v4l2-enc-idx-entry'>v4l2_enc_idx_entry</link>">
 <!ENTITY v4l2-encoder-cmd "struct&nbsp;<link linkend='v4l2-encoder-cmd'>v4l2_encoder_cmd</link>">
+<!ENTITY v4l2-event "struct&nbsp;<link linkend='v4l2-event'>v4l2_event</link>">
+<!ENTITY v4l2-event-subscription "struct&nbsp;<link linkend='v4l2-event-subscription'>v4l2_event_subscription</link>">
 <!ENTITY v4l2-ext-control "struct&nbsp;<link linkend='v4l2-ext-control'>v4l2_ext_control</link>">
 <!ENTITY v4l2-ext-controls "struct&nbsp;<link linkend='v4l2-ext-controls'>v4l2_ext_controls</link>">
 <!ENTITY v4l2-fmtdesc "struct&nbsp;<link linkend='v4l2-fmtdesc'>v4l2_fmtdesc</link>">
@@ -200,6 +204,7 @@
 <!ENTITY sub-controls SYSTEM "v4l/controls.xml">
 <!ENTITY sub-dev-capture SYSTEM "v4l/dev-capture.xml">
 <!ENTITY sub-dev-codec SYSTEM "v4l/dev-codec.xml">
+<!ENTITY sub-dev-event SYSTEM "v4l/dev-event.xml">
 <!ENTITY sub-dev-effect SYSTEM "v4l/dev-effect.xml">
 <!ENTITY sub-dev-osd SYSTEM "v4l/dev-osd.xml">
 <!ENTITY sub-dev-output SYSTEM "v4l/dev-output.xml">
@@ -292,6 +297,8 @@
 <!ENTITY sub-v4l2grab-c SYSTEM "v4l/v4l2grab.c.xml">
 <!ENTITY sub-videodev2-h SYSTEM "v4l/videodev2.h.xml">
 <!ENTITY sub-v4l2 SYSTEM "v4l/v4l2.xml">
+<!ENTITY sub-dqevent SYSTEM "v4l/vidioc-dqevent.xml">
+<!ENTITY sub-subscribe-event SYSTEM "v4l/vidioc-subscribe-event.xml">
 <!ENTITY sub-intro SYSTEM "dvb/intro.xml">
 <!ENTITY sub-frontend SYSTEM "dvb/frontend.xml">
 <!ENTITY sub-dvbproperty SYSTEM "dvb/dvbproperty.xml">
@@ -381,3 +388,5 @@
 <!ENTITY reqbufs SYSTEM "v4l/vidioc-reqbufs.xml">
 <!ENTITY s-hw-freq-seek SYSTEM "v4l/vidioc-s-hw-freq-seek.xml">
 <!ENTITY streamon SYSTEM "v4l/vidioc-streamon.xml">
+<!ENTITY dqevent SYSTEM "v4l/vidioc-dqevent.xml">
+<!ENTITY subscribe_event SYSTEM "v4l/vidioc-subscribe-event.xml">
diff --git a/Documentation/DocBook/v4l/dev-event.xml b/Documentation/DocBook/v4l/dev-event.xml
new file mode 100644
index 0000000..70a9895
--- /dev/null
+++ b/Documentation/DocBook/v4l/dev-event.xml
@@ -0,0 +1,34 @@
+  <title>Event Interface</title>
+
+  <para>The V4L2 event interface provides means for user to get
+  immediately notified on certain conditions taking place on a device.
+  This might include start of frame or loss of signal events, for
+  example.
+  </para>
+
+  <para>To receive events, the events the user is interested first
+  must be subscribed using the &VIDIOC-SUBSCRIBE-EVENT; ioctl. Once an
+  event is subscribed, the events of subscribed types are dequeueable
+  using the &VIDIOC-DQEVENT; ioctl. Events may be unsubscribed using
+  VIDIOC_UNSUBSCRIBE_EVENT ioctl. The special event type
+  V4L2_EVENT_ALL may be used to subscribe or unsubscribe all the
+  events the driver supports.</para>
+
+  <para>The event subscriptions and event queues are specific to file
+  handles. Subscribing an event on one file handle does not affect
+  other file handles.
+  </para>
+
+  <para>The information on dequeueable events are obtained by using
+  select or poll system calls on video devices. The V4L2 events use
+  POLLPRI events on poll system call and exceptions on select system
+  call.
+  </para>
+
+  <!--
+Local Variables:
+mode: sgml
+sgml-parent-document: "v4l2.sgml"
+indent-tabs-mode: nil
+End:
+  -->
diff --git a/Documentation/DocBook/v4l/v4l2.xml b/Documentation/DocBook/v4l/v4l2.xml
index 060105a..9737243 100644
--- a/Documentation/DocBook/v4l/v4l2.xml
+++ b/Documentation/DocBook/v4l/v4l2.xml
@@ -401,6 +401,7 @@ and discussions on the V4L mailing list.</revremark>
     <section id="ttx"> &sub-dev-teletext; </section>
     <section id="radio"> &sub-dev-radio; </section>
     <section id="rds"> &sub-dev-rds; </section>
+    <section id="event"> &sub-dev-event; </section>
   </chapter>
 
   <chapter id="driver">
@@ -426,6 +427,7 @@ and discussions on the V4L mailing list.</revremark>
     &sub-cropcap;
     &sub-dbg-g-chip-ident;
     &sub-dbg-g-register;
+    &sub-dqevent;
     &sub-encoder-cmd;
     &sub-enumaudio;
     &sub-enumaudioout;
@@ -467,6 +469,7 @@ and discussions on the V4L mailing list.</revremark>
     &sub-reqbufs;
     &sub-s-hw-freq-seek;
     &sub-streamon;
+    &sub-subscribe-event;
     <!-- End of ioctls. -->
     &sub-mmap;
     &sub-munmap;
diff --git a/Documentation/DocBook/v4l/vidioc-dqevent.xml b/Documentation/DocBook/v4l/vidioc-dqevent.xml
new file mode 100644
index 0000000..eb45c16
--- /dev/null
+++ b/Documentation/DocBook/v4l/vidioc-dqevent.xml
@@ -0,0 +1,124 @@
+<refentry id="vidioc-dqevent">
+  <refmeta>
+    <refentrytitle>ioctl VIDIOC_DQEVENT</refentrytitle>
+    &manvol;
+  </refmeta>
+
+  <refnamediv>
+    <refname>VIDIOC_DQEVENT</refname>
+    <refpurpose>Dequeue event</refpurpose>
+  </refnamediv>
+
+  <refsynopsisdiv>
+    <funcsynopsis>
+      <funcprototype>
+	<funcdef>int <function>ioctl</function></funcdef>
+	<paramdef>int <parameter>fd</parameter></paramdef>
+	<paramdef>int <parameter>request</parameter></paramdef>
+	<paramdef>struct v4l2_event
+*<parameter>argp</parameter></paramdef>
+      </funcprototype>
+    </funcsynopsis>
+  </refsynopsisdiv>
+
+  <refsect1>
+    <title>Arguments</title>
+
+    <variablelist>
+      <varlistentry>
+	<term><parameter>fd</parameter></term>
+	<listitem>
+	  <para>&fd;</para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><parameter>request</parameter></term>
+	<listitem>
+	  <para>VIDIOC_DQEVENT</para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><parameter>argp</parameter></term>
+	<listitem>
+	  <para></para>
+	</listitem>
+      </varlistentry>
+    </variablelist>
+  </refsect1>
+
+  <refsect1>
+    <title>Description</title>
+
+    <para>Dequeue an event from a video device. No input is required
+    for this ioctl. All the fields of the &v4l2-event; structure are
+    filled by the driver. The file handle will also receive exceptions
+    which the application may get by e.g. using the select system
+    call.</para>
+
+    <table frame="none" pgwide="1" id="v4l2-event">
+      <title>struct <structname>v4l2_event</structname></title>
+      <tgroup cols="4">
+	&cs-str;
+	<tbody valign="top">
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>type</structfield></entry>
+            <entry></entry>
+	    <entry>Type of the event.</entry>
+	  </row>
+	  <row>
+	    <entry>union</entry>
+	    <entry><structfield>u</structfield></entry>
+            <entry></entry>
+	    <entry></entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__u8</entry>
+            <entry><structfield>data</structfield>[64]</entry>
+	    <entry>Event data. Defined by the event type. The union
+            should be used to define easily accessible type for
+            events.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>pending</structfield></entry>
+            <entry></entry>
+	    <entry>Number of pending events excluding this one.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>sequence</structfield></entry>
+            <entry></entry>
+	    <entry>Event sequence number. The sequence number is
+	    incremented for every subscribed event that takes place.
+	    If sequence numbers are not contiguous it means that
+	    events have been lost.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry>struct timeval</entry>
+	    <entry><structfield>timestamp</structfield></entry>
+            <entry></entry>
+	    <entry>Event timestamp.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>reserved</structfield>[9]</entry>
+            <entry></entry>
+	    <entry>Reserved for future extensions. Drivers must set
+	    the array to zero.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+  </refsect1>
+</refentry>
+<!--
+Local Variables:
+mode: sgml
+sgml-parent-document: "v4l2.sgml"
+indent-tabs-mode: nil
+End:
+-->
diff --git a/Documentation/DocBook/v4l/vidioc-subscribe-event.xml b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
new file mode 100644
index 0000000..943de5c
--- /dev/null
+++ b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
@@ -0,0 +1,109 @@
+<refentry id="vidioc-subscribe-event">
+  <refmeta>
+    <refentrytitle>ioctl VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</refentrytitle>
+    &manvol;
+  </refmeta>
+
+  <refnamediv>
+    <refname>VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</refname>
+    <refpurpose>Subscribe or unsubscribe event</refpurpose>
+  </refnamediv>
+
+  <refsynopsisdiv>
+    <funcsynopsis>
+      <funcprototype>
+	<funcdef>int <function>ioctl</function></funcdef>
+	<paramdef>int <parameter>fd</parameter></paramdef>
+	<paramdef>int <parameter>request</parameter></paramdef>
+	<paramdef>struct v4l2_event_subscription
+*<parameter>argp</parameter></paramdef>
+      </funcprototype>
+    </funcsynopsis>
+  </refsynopsisdiv>
+
+  <refsect1>
+    <title>Arguments</title>
+
+    <variablelist>
+      <varlistentry>
+	<term><parameter>fd</parameter></term>
+	<listitem>
+	  <para>&fd;</para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><parameter>request</parameter></term>
+	<listitem>
+	  <para>VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</para>
+	</listitem>
+      </varlistentry>
+      <varlistentry>
+	<term><parameter>argp</parameter></term>
+	<listitem>
+	  <para></para>
+	</listitem>
+      </varlistentry>
+    </variablelist>
+  </refsect1>
+
+  <refsect1>
+    <title>Description</title>
+
+    <para>Subscribe or unsubscribe V4L2 event. Subscribed events are
+    dequeued by using the &VIDIOC-SUBSCRIBE-EVENT; ioctl.</para>
+
+    There are standard and private events. New standard events must
+    use the smallest available event type.
+
+    <table frame="none" pgwide="1" id="v4l2-event-subscription">
+      <title>struct <structname>v4l2_event_subscription</structname></title>
+      <tgroup cols="3">
+	&cs-str;
+	<tbody valign="top">
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>type</structfield></entry>
+	    <entry>Type of the event.</entry>
+	  </row>
+	  <row>
+	    <entry>__u32</entry>
+	    <entry><structfield>reserved</structfield>[7]</entry>
+	    <entry>Reserved for future extensions. Drivers must set
+	    the array to zero.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+    <table frame="none" pgwide="1" id="event-type">
+      <title>Event Types</title>
+      <tgroup cols="3">
+	&cs-def;
+	<tbody valign="top">
+	  <row>
+	    <entry><constant>V4L2_EVENT_ALL</constant></entry>
+	    <entry>0</entry>
+	    <entry>All events. The user may subscribe
+	    for all events supported by the driver by subscribing a
+	    special event, V4L2_EVENT_ALL.</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_EVENT_PRIVATE_START</constant></entry>
+	    <entry>0x08000000</entry> <entry>Private events start from
+	    this number. The drivers must allocate their events
+	    starting from base (V4L2_EVENT_PRIVATE_START + n * 1024)
+	    where individual events start from base + 1.</entry>
+	  </row>
+	</tbody>
+      </tgroup>
+    </table>
+
+  </refsect1>
+</refentry>
+<!--
+Local Variables:
+mode: sgml
+sgml-parent-document: "v4l2.sgml"
+indent-tabs-mode: nil
+End:
+-->
diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
index 08f9e59..7494a64 100644
--- a/Documentation/video4linux/v4l2-framework.txt
+++ b/Documentation/video4linux/v4l2-framework.txt
@@ -731,3 +731,46 @@ Useful functions:
 The users of v4l2_fh know whether a driver uses v4l2_fh as its
 file.private_data pointer by testing the V4L2_FL_USES_V4L2_FH bit in
 video_device.flags.
+
+V4L2 events
+-----------
+
+The V4L2 events provide a generic way to pass events to user space.
+The driver must use v4l2_fh to be able to support V4L2 events. To
+support V4L2 events, the driver must
+
+- v4l2_event_alloc()
+
+  To use events, the driver must allocate events for the file handle. By
+  calling the function more than once, the driver may assure that at least n
+  events in total has been allocated. The function may not be called in
+  atomic context. If the driver only wants to initialise events it is fine
+  to call v4l2_event_alloc() with 0 allocation. This allows passing the
+  ioctls to the driver.
+
+- v4l2_event_queue()
+
+  Queue events to video device. The driver's only responsibility is to fill
+  in the type and the data fields. The other fields will be filled in by
+  V4L2.
+
+- Define vidioc_subscribe_event in struct v4l2_ioctl_ops. The
+  vidioc_subscribe_event must chech the driver is able to produce events
+  with specified event id. Then it calls v4l2_event_subscribe() to subscribe
+  the event.
+
+- Define vidioc_unsubscribe_event in struct v4l2_ioctl_ops. A driver may use
+  v4l2_event_unsubscribe directly unless it wants to be involved in
+  unsubscription process.
+
+- To support V4L2_EVENTS_ALL, the driver must handle the special id
+  V4L2_EVENTS_ALL when subscribing for events. The function
+  v4l2_event_subscribe_many() is useful in implementing this. Unsubscription
+  of all events is already handled by vidioc_unsubscribe_event().
+
+- Events are delivered to user space through the poll system call. The
+  driver should use a separate wait queue for the V4L2 events.
+
+An example on how the V4L2 events may be used can be found in the OMAP
+3 ISP driver available at <URL:http://gitorious.org/omap3camera> as of
+writing this.
-- 
1.5.6.5

Re: [PATCH v5 6/6] V4L: Events: Add documentation

[Hans Verkuil]

On Friday 19 February 2010 20:22:00 Sakari Ailus wrote:
> Add documentation on how to use V4L2 events, both for V4L2 drivers and for
> V4L2 applications.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@maxwell.research.nokia.com>
> ---
>  Documentation/DocBook/media-entities.tmpl          |    9 ++
>  Documentation/DocBook/v4l/dev-event.xml            |   34 ++++++
>  Documentation/DocBook/v4l/v4l2.xml                 |    3 +
>  Documentation/DocBook/v4l/vidioc-dqevent.xml       |  124 ++++++++++++++++++++
>  .../DocBook/v4l/vidioc-subscribe-event.xml         |  109 +++++++++++++++++
>  Documentation/video4linux/v4l2-framework.txt       |   43 +++++++
>  6 files changed, 322 insertions(+), 0 deletions(-)
>  create mode 100644 Documentation/DocBook/v4l/dev-event.xml
>  create mode 100644 Documentation/DocBook/v4l/vidioc-dqevent.xml
>  create mode 100644 Documentation/DocBook/v4l/vidioc-subscribe-event.xml
> 
> diff --git a/Documentation/DocBook/media-entities.tmpl b/Documentation/DocBook/media-entities.tmpl
> index c725cb8..770be3c 100644
> --- a/Documentation/DocBook/media-entities.tmpl
> +++ b/Documentation/DocBook/media-entities.tmpl
> @@ -17,6 +17,7 @@
>  <!ENTITY VIDIOC-DBG-G-REGISTER "<link linkend='vidioc-dbg-g-register'><constant>VIDIOC_DBG_G_REGISTER</constant></link>">
>  <!ENTITY VIDIOC-DBG-S-REGISTER "<link linkend='vidioc-dbg-g-register'><constant>VIDIOC_DBG_S_REGISTER</constant></link>">
>  <!ENTITY VIDIOC-DQBUF "<link linkend='vidioc-qbuf'><constant>VIDIOC_DQBUF</constant></link>">
> +<!ENTITY VIDIOC-DQEVENT "<link linkend='vidioc-dqevent'><constant>VIDIOC_DQEVENT</constant></link>">
>  <!ENTITY VIDIOC-ENCODER-CMD "<link linkend='vidioc-encoder-cmd'><constant>VIDIOC_ENCODER_CMD</constant></link>">
>  <!ENTITY VIDIOC-ENUMAUDIO "<link linkend='vidioc-enumaudio'><constant>VIDIOC_ENUMAUDIO</constant></link>">
>  <!ENTITY VIDIOC-ENUMAUDOUT "<link linkend='vidioc-enumaudioout'><constant>VIDIOC_ENUMAUDOUT</constant></link>">
> @@ -60,6 +61,7 @@
>  <!ENTITY VIDIOC-REQBUFS "<link linkend='vidioc-reqbufs'><constant>VIDIOC_REQBUFS</constant></link>">
>  <!ENTITY VIDIOC-STREAMOFF "<link linkend='vidioc-streamon'><constant>VIDIOC_STREAMOFF</constant></link>">
>  <!ENTITY VIDIOC-STREAMON "<link linkend='vidioc-streamon'><constant>VIDIOC_STREAMON</constant></link>">
> +<!ENTITY VIDIOC-SUBSCRIBE-EVENT "<link linkend='vidioc-subscribe-event'><constant>VIDIOC_SUBSCRIBE_EVENT</constant></link>">
>  <!ENTITY VIDIOC-S-AUDIO "<link linkend='vidioc-g-audio'><constant>VIDIOC_S_AUDIO</constant></link>">
>  <!ENTITY VIDIOC-S-AUDOUT "<link linkend='vidioc-g-audioout'><constant>VIDIOC_S_AUDOUT</constant></link>">
>  <!ENTITY VIDIOC-S-CROP "<link linkend='vidioc-g-crop'><constant>VIDIOC_S_CROP</constant></link>">
> @@ -141,6 +143,8 @@
>  <!ENTITY v4l2-enc-idx "struct&nbsp;<link linkend='v4l2-enc-idx'>v4l2_enc_idx</link>">
>  <!ENTITY v4l2-enc-idx-entry "struct&nbsp;<link linkend='v4l2-enc-idx-entry'>v4l2_enc_idx_entry</link>">
>  <!ENTITY v4l2-encoder-cmd "struct&nbsp;<link linkend='v4l2-encoder-cmd'>v4l2_encoder_cmd</link>">
> +<!ENTITY v4l2-event "struct&nbsp;<link linkend='v4l2-event'>v4l2_event</link>">
> +<!ENTITY v4l2-event-subscription "struct&nbsp;<link linkend='v4l2-event-subscription'>v4l2_event_subscription</link>">
>  <!ENTITY v4l2-ext-control "struct&nbsp;<link linkend='v4l2-ext-control'>v4l2_ext_control</link>">
>  <!ENTITY v4l2-ext-controls "struct&nbsp;<link linkend='v4l2-ext-controls'>v4l2_ext_controls</link>">
>  <!ENTITY v4l2-fmtdesc "struct&nbsp;<link linkend='v4l2-fmtdesc'>v4l2_fmtdesc</link>">
> @@ -200,6 +204,7 @@
>  <!ENTITY sub-controls SYSTEM "v4l/controls.xml">
>  <!ENTITY sub-dev-capture SYSTEM "v4l/dev-capture.xml">
>  <!ENTITY sub-dev-codec SYSTEM "v4l/dev-codec.xml">
> +<!ENTITY sub-dev-event SYSTEM "v4l/dev-event.xml">
>  <!ENTITY sub-dev-effect SYSTEM "v4l/dev-effect.xml">
>  <!ENTITY sub-dev-osd SYSTEM "v4l/dev-osd.xml">
>  <!ENTITY sub-dev-output SYSTEM "v4l/dev-output.xml">
> @@ -292,6 +297,8 @@
>  <!ENTITY sub-v4l2grab-c SYSTEM "v4l/v4l2grab.c.xml">
>  <!ENTITY sub-videodev2-h SYSTEM "v4l/videodev2.h.xml">
>  <!ENTITY sub-v4l2 SYSTEM "v4l/v4l2.xml">
> +<!ENTITY sub-dqevent SYSTEM "v4l/vidioc-dqevent.xml">
> +<!ENTITY sub-subscribe-event SYSTEM "v4l/vidioc-subscribe-event.xml">
>  <!ENTITY sub-intro SYSTEM "dvb/intro.xml">
>  <!ENTITY sub-frontend SYSTEM "dvb/frontend.xml">
>  <!ENTITY sub-dvbproperty SYSTEM "dvb/dvbproperty.xml">
> @@ -381,3 +388,5 @@
>  <!ENTITY reqbufs SYSTEM "v4l/vidioc-reqbufs.xml">
>  <!ENTITY s-hw-freq-seek SYSTEM "v4l/vidioc-s-hw-freq-seek.xml">
>  <!ENTITY streamon SYSTEM "v4l/vidioc-streamon.xml">
> +<!ENTITY dqevent SYSTEM "v4l/vidioc-dqevent.xml">
> +<!ENTITY subscribe_event SYSTEM "v4l/vidioc-subscribe-event.xml">
> diff --git a/Documentation/DocBook/v4l/dev-event.xml b/Documentation/DocBook/v4l/dev-event.xml
> new file mode 100644
> index 0000000..70a9895
> --- /dev/null
> +++ b/Documentation/DocBook/v4l/dev-event.xml
> @@ -0,0 +1,34 @@
> +  <title>Event Interface</title>
> +
> +  <para>The V4L2 event interface provides means for user to get
> +  immediately notified on certain conditions taking place on a device.
> +  This might include start of frame or loss of signal events, for
> +  example.
> +  </para>
> +
> +  <para>To receive events, the events the user is interested first
> +  must be subscribed using the &VIDIOC-SUBSCRIBE-EVENT; ioctl. Once an
> +  event is subscribed, the events of subscribed types are dequeueable
> +  using the &VIDIOC-DQEVENT; ioctl. Events may be unsubscribed using
> +  VIDIOC_UNSUBSCRIBE_EVENT ioctl. The special event type
> +  V4L2_EVENT_ALL may be used to subscribe or unsubscribe all the
> +  events the driver supports.</para>
> +
> +  <para>The event subscriptions and event queues are specific to file
> +  handles. Subscribing an event on one file handle does not affect
> +  other file handles.
> +  </para>
> +
> +  <para>The information on dequeueable events are obtained by using
> +  select or poll system calls on video devices. The V4L2 events use
> +  POLLPRI events on poll system call and exceptions on select system
> +  call.
> +  </para>
> +
> +  <!--
> +Local Variables:
> +mode: sgml
> +sgml-parent-document: "v4l2.sgml"
> +indent-tabs-mode: nil
> +End:
> +  -->
> diff --git a/Documentation/DocBook/v4l/v4l2.xml b/Documentation/DocBook/v4l/v4l2.xml
> index 060105a..9737243 100644
> --- a/Documentation/DocBook/v4l/v4l2.xml
> +++ b/Documentation/DocBook/v4l/v4l2.xml
> @@ -401,6 +401,7 @@ and discussions on the V4L mailing list.</revremark>
>      <section id="ttx"> &sub-dev-teletext; </section>
>      <section id="radio"> &sub-dev-radio; </section>
>      <section id="rds"> &sub-dev-rds; </section>
> +    <section id="event"> &sub-dev-event; </section>
>    </chapter>
>  
>    <chapter id="driver">
> @@ -426,6 +427,7 @@ and discussions on the V4L mailing list.</revremark>
>      &sub-cropcap;
>      &sub-dbg-g-chip-ident;
>      &sub-dbg-g-register;
> +    &sub-dqevent;
>      &sub-encoder-cmd;
>      &sub-enumaudio;
>      &sub-enumaudioout;
> @@ -467,6 +469,7 @@ and discussions on the V4L mailing list.</revremark>
>      &sub-reqbufs;
>      &sub-s-hw-freq-seek;
>      &sub-streamon;
> +    &sub-subscribe-event;
>      <!-- End of ioctls. -->
>      &sub-mmap;
>      &sub-munmap;
> diff --git a/Documentation/DocBook/v4l/vidioc-dqevent.xml b/Documentation/DocBook/v4l/vidioc-dqevent.xml
> new file mode 100644
> index 0000000..eb45c16
> --- /dev/null
> +++ b/Documentation/DocBook/v4l/vidioc-dqevent.xml
> @@ -0,0 +1,124 @@
> +<refentry id="vidioc-dqevent">
> +  <refmeta>
> +    <refentrytitle>ioctl VIDIOC_DQEVENT</refentrytitle>
> +    &manvol;
> +  </refmeta>
> +
> +  <refnamediv>
> +    <refname>VIDIOC_DQEVENT</refname>
> +    <refpurpose>Dequeue event</refpurpose>
> +  </refnamediv>
> +
> +  <refsynopsisdiv>
> +    <funcsynopsis>
> +      <funcprototype>
> +	<funcdef>int <function>ioctl</function></funcdef>
> +	<paramdef>int <parameter>fd</parameter></paramdef>
> +	<paramdef>int <parameter>request</parameter></paramdef>
> +	<paramdef>struct v4l2_event
> +*<parameter>argp</parameter></paramdef>
> +      </funcprototype>
> +    </funcsynopsis>
> +  </refsynopsisdiv>
> +
> +  <refsect1>
> +    <title>Arguments</title>
> +
> +    <variablelist>
> +      <varlistentry>
> +	<term><parameter>fd</parameter></term>
> +	<listitem>
> +	  <para>&fd;</para>
> +	</listitem>
> +      </varlistentry>
> +      <varlistentry>
> +	<term><parameter>request</parameter></term>
> +	<listitem>
> +	  <para>VIDIOC_DQEVENT</para>
> +	</listitem>
> +      </varlistentry>
> +      <varlistentry>
> +	<term><parameter>argp</parameter></term>
> +	<listitem>
> +	  <para></para>
> +	</listitem>
> +      </varlistentry>
> +    </variablelist>
> +  </refsect1>
> +
> +  <refsect1>
> +    <title>Description</title>
> +
> +    <para>Dequeue an event from a video device. No input is required
> +    for this ioctl. All the fields of the &v4l2-event; structure are
> +    filled by the driver. The file handle will also receive exceptions
> +    which the application may get by e.g. using the select system
> +    call.</para>
> +
> +    <table frame="none" pgwide="1" id="v4l2-event">
> +      <title>struct <structname>v4l2_event</structname></title>
> +      <tgroup cols="4">
> +	&cs-str;
> +	<tbody valign="top">
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>type</structfield></entry>
> +            <entry></entry>
> +	    <entry>Type of the event.</entry>
> +	  </row>
> +	  <row>
> +	    <entry>union</entry>
> +	    <entry><structfield>u</structfield></entry>
> +            <entry></entry>
> +	    <entry></entry>
> +	  </row>
> +	  <row>
> +	    <entry></entry>
> +	    <entry>__u8</entry>
> +            <entry><structfield>data</structfield>[64]</entry>
> +	    <entry>Event data. Defined by the event type. The union
> +            should be used to define easily accessible type for
> +            events.</entry>
> +	  </row>
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>pending</structfield></entry>
> +            <entry></entry>
> +	    <entry>Number of pending events excluding this one.</entry>
> +	  </row>
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>sequence</structfield></entry>
> +            <entry></entry>
> +	    <entry>Event sequence number. The sequence number is
> +	    incremented for every subscribed event that takes place.
> +	    If sequence numbers are not contiguous it means that
> +	    events have been lost.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry>struct timeval</entry>
> +	    <entry><structfield>timestamp</structfield></entry>
> +            <entry></entry>
> +	    <entry>Event timestamp.</entry>
> +	  </row>
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>reserved</structfield>[9]</entry>
> +            <entry></entry>
> +	    <entry>Reserved for future extensions. Drivers must set
> +	    the array to zero.</entry>
> +	  </row>
> +	</tbody>
> +      </tgroup>
> +    </table>
> +
> +  </refsect1>
> +</refentry>
> +<!--
> +Local Variables:
> +mode: sgml
> +sgml-parent-document: "v4l2.sgml"
> +indent-tabs-mode: nil
> +End:
> +-->
> diff --git a/Documentation/DocBook/v4l/vidioc-subscribe-event.xml b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
> new file mode 100644
> index 0000000..943de5c
> --- /dev/null
> +++ b/Documentation/DocBook/v4l/vidioc-subscribe-event.xml
> @@ -0,0 +1,109 @@
> +<refentry id="vidioc-subscribe-event">
> +  <refmeta>
> +    <refentrytitle>ioctl VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</refentrytitle>
> +    &manvol;
> +  </refmeta>
> +
> +  <refnamediv>
> +    <refname>VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</refname>
> +    <refpurpose>Subscribe or unsubscribe event</refpurpose>
> +  </refnamediv>
> +
> +  <refsynopsisdiv>
> +    <funcsynopsis>
> +      <funcprototype>
> +	<funcdef>int <function>ioctl</function></funcdef>
> +	<paramdef>int <parameter>fd</parameter></paramdef>
> +	<paramdef>int <parameter>request</parameter></paramdef>
> +	<paramdef>struct v4l2_event_subscription
> +*<parameter>argp</parameter></paramdef>
> +      </funcprototype>
> +    </funcsynopsis>
> +  </refsynopsisdiv>
> +
> +  <refsect1>
> +    <title>Arguments</title>
> +
> +    <variablelist>
> +      <varlistentry>
> +	<term><parameter>fd</parameter></term>
> +	<listitem>
> +	  <para>&fd;</para>
> +	</listitem>
> +      </varlistentry>
> +      <varlistentry>
> +	<term><parameter>request</parameter></term>
> +	<listitem>
> +	  <para>VIDIOC_SUBSCRIBE_EVENT, VIDIOC_UNSUBSCRIBE_EVENT</para>
> +	</listitem>
> +      </varlistentry>
> +      <varlistentry>
> +	<term><parameter>argp</parameter></term>
> +	<listitem>
> +	  <para></para>
> +	</listitem>
> +      </varlistentry>
> +    </variablelist>
> +  </refsect1>
> +
> +  <refsect1>
> +    <title>Description</title>
> +
> +    <para>Subscribe or unsubscribe V4L2 event. Subscribed events are
> +    dequeued by using the &VIDIOC-SUBSCRIBE-EVENT; ioctl.</para>

You mean VIDIOC-DQUEUE, I assume.

> +
> +    There are standard and private events. New standard events must
> +    use the smallest available event type.

That last sentence belongs in v4l2-framework.txt, not here.

> +
> +    <table frame="none" pgwide="1" id="v4l2-event-subscription">
> +      <title>struct <structname>v4l2_event_subscription</structname></title>
> +      <tgroup cols="3">
> +	&cs-str;
> +	<tbody valign="top">
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>type</structfield></entry>
> +	    <entry>Type of the event.</entry>
> +	  </row>
> +	  <row>
> +	    <entry>__u32</entry>
> +	    <entry><structfield>reserved</structfield>[7]</entry>
> +	    <entry>Reserved for future extensions. Drivers must set
> +	    the array to zero.</entry>
> +	  </row>
> +	</tbody>
> +      </tgroup>
> +    </table>
> +
> +    <table frame="none" pgwide="1" id="event-type">
> +      <title>Event Types</title>
> +      <tgroup cols="3">
> +	&cs-def;
> +	<tbody valign="top">
> +	  <row>
> +	    <entry><constant>V4L2_EVENT_ALL</constant></entry>
> +	    <entry>0</entry>
> +	    <entry>All events. The user may subscribe
> +	    for all events supported by the driver by subscribing a
> +	    special event, V4L2_EVENT_ALL.</entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_EVENT_PRIVATE_START</constant></entry>
> +	    <entry>0x08000000</entry> <entry>Private events start from
> +	    this number. The drivers must allocate their events
> +	    starting from base (V4L2_EVENT_PRIVATE_START + n * 1024)
> +	    where individual events start from base + 1.</entry>

Again, that last sentence belongs in v4l2-framework.txt.

> +	  </row>
> +	</tbody>
> +      </tgroup>
> +    </table>
> +
> +  </refsect1>
> +</refentry>
> +<!--
> +Local Variables:
> +mode: sgml
> +sgml-parent-document: "v4l2.sgml"
> +indent-tabs-mode: nil
> +End:
> +-->
> diff --git a/Documentation/video4linux/v4l2-framework.txt b/Documentation/video4linux/v4l2-framework.txt
> index 08f9e59..7494a64 100644
> --- a/Documentation/video4linux/v4l2-framework.txt
> +++ b/Documentation/video4linux/v4l2-framework.txt
> @@ -731,3 +731,46 @@ Useful functions:
>  The users of v4l2_fh know whether a driver uses v4l2_fh as its
>  file.private_data pointer by testing the V4L2_FL_USES_V4L2_FH bit in
>  video_device.flags.
> +
> +V4L2 events
> +-----------
> +
> +The V4L2 events provide a generic way to pass events to user space.
> +The driver must use v4l2_fh to be able to support V4L2 events. To
> +support V4L2 events, the driver must

Must what?

> +
> +- v4l2_event_alloc()
> +
> +  To use events, the driver must allocate events for the file handle. By
> +  calling the function more than once, the driver may assure that at least n
> +  events in total has been allocated. The function may not be called in
> +  atomic context. If the driver only wants to initialise events it is fine
> +  to call v4l2_event_alloc() with 0 allocation. This allows passing the
> +  ioctls to the driver.

This is unnecessarily complex. And based on my comments on the previous patches
it is also not needed. Drivers can call v4l2_event_alloc either after calling
v4l2_fh_init or in the subscribe_event op.

> +
> +- v4l2_event_queue()
> +
> +  Queue events to video device. The driver's only responsibility is to fill
> +  in the type and the data fields. The other fields will be filled in by
> +  V4L2.
> +
> +- Define vidioc_subscribe_event in struct v4l2_ioctl_ops. The
> +  vidioc_subscribe_event must chech the driver is able to produce events

typo: check

> +  with specified event id. Then it calls v4l2_event_subscribe() to subscribe
> +  the event.
> +
> +- Define vidioc_unsubscribe_event in struct v4l2_ioctl_ops. A driver may use
> +  v4l2_event_unsubscribe directly unless it wants to be involved in
> +  unsubscription process.
> +
> +- To support V4L2_EVENTS_ALL, the driver must handle the special id
> +  V4L2_EVENTS_ALL when subscribing for events. The function
> +  v4l2_event_subscribe_many() is useful in implementing this. Unsubscription
> +  of all events is already handled by vidioc_unsubscribe_event().

This should only be supported by unsubscribe.

> +
> +- Events are delivered to user space through the poll system call. The
> +  driver should use a separate wait queue for the V4L2 events.

I don't quite get what you mean by this. I am also missing a reference to the
fh->events->wait queue. That's a rather important piece of information to
document.

You should also document v4l2_event_pending and mention that dequeuing is
done internally through the v4l2_event_dequeue call that needs no driver
support.

Regards,

	Hans

> +
> +An example on how the V4L2 events may be used can be found in the OMAP
> +3 ISP driver available at <URL:http://gitorious.org/omap3camera> as of
> +writing this.
> 

-- 
Hans Verkuil - video4linux developer - sponsored by TANDBERG