[RFC] Video events, version 2

[RFC] Video events, version 2

[Sakari Ailus]

Hi,


Here's the second version of the video events RFC. It's based on Laurent 
Pinchart's original RFC. My aim is to address the issues found in the 
old RFC during the V4L-DVB mini-summit in the Linux plumbers conference 
2009. To get a good grasp of the problem at hand it's probably a good 
idea read the original RFC as well:

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


Changes to version 1
----------------------------------

struct video_event has been renamed to v4l2_event. The struct is used in 
userspace and V4L related structures appear to have v4l2 prefix so that 
should be better than video.

The "entity" field has been removed from the struct v4l2_event since the 
subdevices will have their own device nodes --- the events should come 
from them instead of the media controller. Video nodes could be used for 
events, too.

A few reserved fields have been added. There are new ioctls as well for 
enumeration and (un)subscribing.


Interface description
---------------------

Event type is either a standard event or private event. Standard events 
will be defined in videodev2.h. Private event types begin from 
V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future use.

#define V4L2_EVENT_PRIVATE_START	0x08000000
#define V4L2_EVENT_RESERVED		0x10000000

VIDIOC_ENUM_EVENT is used to enumerate the available event types. It 
works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next 
event type by calling it with the last type in the type field. The 
difference is that the range is not continuous like in querying controls.

VIDIOC_G_EVENT is used to get events. sequence is the event sequence 
number and the data is specific to driver or event type.

The user will get the information that there's an event through 
exception file descriptors by using select(2). When an event is 
available the poll handler sets POLLPRI which wakes up select. -EINVAL 
will be returned if there are no pending events.

VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to 
subscribe and unsubscribe from events. The argument is event type.


struct v4l2_eventdesc {
	__u32		type;
	__u8		description[64];
	__u32		reserved[4];
};

struct v4l2_event {
	__u32		type;
	__u32		sequence;
	struct timeval	timestamp;
	__u8		data[64];
	__u32		reserved[4];
};

#define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
#define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
#define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
#define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)


As it was discussed in the LPC, event subscriptions should be bound to 
file handle. The implementation, however, is not visible to userspace. 
This is why I'm not specifying it in this RFC.

While the number of possible standard (and probably private) events 
would be quite small and the implementation could be a bit field, I do 
see that the interface must be using types passed as numbers instead of 
bit fields.

Is it necessary to buffer events of same type or will an event replace 
an older event of the same type? It probably depends on event type which 
is better. This is also a matter of implementation.


Comments and questions are more than welcome.

Cheers,

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

Re: [RFC] Video events, version 2

[Hans Verkuil]

On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
> Hi,
> 
> 
> Here's the second version of the video events RFC. It's based on Laurent 
> Pinchart's original RFC. My aim is to address the issues found in the 
> old RFC during the V4L-DVB mini-summit in the Linux plumbers conference 
> 2009. To get a good grasp of the problem at hand it's probably a good 
> idea read the original RFC as well:
> 
> <URL:http://www.spinics.net/lists/linux-media/msg10217.html>
> 
> 
> Changes to version 1
> ----------------------------------
> 
> struct video_event has been renamed to v4l2_event. The struct is used in 
> userspace and V4L related structures appear to have v4l2 prefix so that 
> should be better than video.
> 
> The "entity" field has been removed from the struct v4l2_event since the 
> subdevices will have their own device nodes --- the events should come 
> from them instead of the media controller. Video nodes could be used for 
> events, too.
> 
> A few reserved fields have been added. There are new ioctls as well for 
> enumeration and (un)subscribing.
> 
> 
> Interface description
> ---------------------
> 
> Event type is either a standard event or private event. Standard events 
> will be defined in videodev2.h. Private event types begin from 
> V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future use.
> 
> #define V4L2_EVENT_PRIVATE_START	0x08000000
> #define V4L2_EVENT_RESERVED		0x10000000

Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer V4L2_EVENT?

> 
> VIDIOC_ENUM_EVENT is used to enumerate the available event types. It 
> works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next 
> event type by calling it with the last type in the type field. The 
> difference is that the range is not continuous like in querying controls.

Question: why do we need an ENUM_EVENT? I don't really see a use-case for this.

Also note that there are three methods in use for enumerating within V4L:

1) there is an index field in the struct that starts at 0 and that the
application increases by 1 until the ioctl returns an error.

2) old-style controls where just enumerated from CID_BASE to CID_LASTP1,
which is very, very ugly.

3) controls new-style allow one to set bit 31 on the control ID and in that
case the ioctl will give you the first control with an ID that is higher than
the specified ID.

1 or 3 are both valid options IMHO.

But again, I don't see why we need it in the first place.

> VIDIOC_G_EVENT is used to get events. sequence is the event sequence 
> number and the data is specific to driver or event type.
> 
> The user will get the information that there's an event through 
> exception file descriptors by using select(2). When an event is 
> available the poll handler sets POLLPRI which wakes up select. -EINVAL 
> will be returned if there are no pending events.
> 
> VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to 
> subscribe and unsubscribe from events. The argument is event type.
> 

Two event types can be defined already (used by ivtv):

#define V4L2_EVENT_DECODER_STOPPED   1
#define V4L2_EVENT_OUTPUT_VSYNC      2

> 
> struct v4l2_eventdesc {
> 	__u32		type;
> 	__u8		description[64];
> 	__u32		reserved[4];
> };
> 
> struct v4l2_event {
> 	__u32		type;
> 	__u32		sequence;
> 	struct timeval	timestamp;
> 	__u8		data[64];

This should be a union:


union {
	enum v4l2_field ev_output_vsync;
	__u8 data[64];
};

> 	__u32		reserved[4];
> };
> 
> #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
> #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
> #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
> #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)

For (un)subscribe I suggest that we also use a struct with the event type
and a few reserved fields.

> 
> 
> As it was discussed in the LPC, event subscriptions should be bound to 
> file handle. The implementation, however, is not visible to userspace. 
> This is why I'm not specifying it in this RFC.
> 
> While the number of possible standard (and probably private) events 
> would be quite small and the implementation could be a bit field, I do 
> see that the interface must be using types passed as numbers instead of 
> bit fields.
> 
> Is it necessary to buffer events of same type or will an event replace 
> an older event of the same type? It probably depends on event type which 
> is better. This is also a matter of implementation.
> 
> 
> Comments and questions are more than welcome.

Here's a mixed bag of idea/comments:

We need to define what to do when you unsubscribe an event and there are still
events of that type pending. Do we remove those pending events as well?
I think we should just keep them, but I'm open for other opinions.

I was wondering if a 'count' field in v4l2_event might be useful: e.g. if you
get multiple identical events, and that event is already registered, then you
can just increase the count rather than adding the same event again. This
might be overengineering, though. And to be honest, I can't think of a
use-case, but it's something to keep in mind perhaps.

Would we ever need a VIDIOC_S_EVENT to let the application set an event?
('software events').

Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT might be
more appropriate.

How do we prevent the event queue from overflowing? Just hardcode a watermark?
Alternatively, when subscribing an event we can also pass the maximum number
of allowed events as an argument.

Regards,

	Hans

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

Re: [RFC] Video events, version 2

[Laurent Pinchart]

Hi Sakari,

On Wednesday 14 October 2009 19:48:33 Hans Verkuil wrote:
> On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
> > Here's the second version of the video events RFC. It's based on Laurent
> > Pinchart's original RFC. My aim is to address the issues found in the
> > old RFC during the V4L-DVB mini-summit in the Linux plumbers conference
> > 2009. To get a good grasp of the problem at hand it's probably a good
> > idea read the original RFC as well:
> >
> > <URL:http://www.spinics.net/lists/linux-media/msg10217.html>

Thanks for the RFC update.

> > Changes to version 1
> > ----------------------------------
> >
> > struct video_event has been renamed to v4l2_event. The struct is used in
> > userspace and V4L related structures appear to have v4l2 prefix so that
> > should be better than video.

In the end we will probably rename that to media_ or something similar in the 
big media controller rename (if that ever happens). For now let's keep v4l2_, 
that will be more consistent.

> > The "entity" field has been removed from the struct v4l2_event since the
> > subdevices will have their own device nodes --- the events should come
> > from them instead of the media controller. Video nodes could be used for
> > events, too.

I would still keep the entity field. It would allow for parents to report 
children events and there could be use cases for that.

> > A few reserved fields have been added. There are new ioctls as well for
> > enumeration and (un)subscribing.
> >
> >
> > Interface description
> > ---------------------
> >
> > Event type is either a standard event or private event. Standard events
> > will be defined in videodev2.h. Private event types begin from
> > V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future
> > use.
> >
> > #define V4L2_EVENT_PRIVATE_START	0x08000000
> > #define V4L2_EVENT_RESERVED		0x10000000
> 
> Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer
>  V4L2_EVENT?

EV could be confused with electron volt, exposure value, or even escape 
velocity (don't underestimate the use of V4L2 in the spaceship market ;-)). On 
a more serious note, while I like to keep identifiers short, is the 3 
characters gain worth it here ?

> > VIDIOC_ENUM_EVENT is used to enumerate the available event types. It
> > works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next
> > event type by calling it with the last type in the type field. The
> > difference is that the range is not continuous like in querying controls.
> 
> Question: why do we need an ENUM_EVENT? I don't really see a use-case for
>  this.
> 
> Also note that there are three methods in use for enumerating within V4L:
> 
> 1) there is an index field in the struct that starts at 0 and that the
> application increases by 1 until the ioctl returns an error.
> 
> 2) old-style controls where just enumerated from CID_BASE to CID_LASTP1,
> which is very, very ugly.
> 
> 3) controls new-style allow one to set bit 31 on the control ID and in that
> case the ioctl will give you the first control with an ID that is higher
>  than the specified ID.
> 
> 1 or 3 are both valid options IMHO.
> 
> But again, I don't see why we need it in the first place.

Applications will only subscribe to the events they can handle, so I don't 
think enumeration is really required. We might want to provide "subscribe to 
all" and "subscribe to none" options though, maybe as special events 
(V4L2_EVENT_NONE, V4L2_EVENT_ALL)

> > VIDIOC_G_EVENT is used to get events. sequence is the event sequence
> > number and the data is specific to driver or event type.

For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to 
retrieve multiple events.

struct v4l2_events {
	__u32 count;
	struct v4l2_event __user *events;
};

#define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)

> > The user will get the information that there's an event through
> > exception file descriptors by using select(2). When an event is
> > available the poll handler sets POLLPRI which wakes up select. -EINVAL
> > will be returned if there are no pending events.
> >
> > VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to
> > subscribe and unsubscribe from events. The argument is event type.
> 
> Two event types can be defined already (used by ivtv):
> 
> #define V4L2_EVENT_DECODER_STOPPED   1
> #define V4L2_EVENT_OUTPUT_VSYNC      2
> 
> > struct v4l2_eventdesc {
> > 	__u32		type;
> > 	__u8		description[64];
> > 	__u32		reserved[4];
> > };
> >
> > struct v4l2_event {
> > 	__u32		type;
> > 	__u32		sequence;
> > 	struct timeval	timestamp;
> > 	__u8		data[64];
> 
> This should be a union:
> 
> 
> union {
> 	enum v4l2_field ev_output_vsync;
> 	__u8 data[64];
> };

The union will grow pretty big and I'm scared it would soon become a mess.

> > 	__u32		reserved[4];
> > };
> >
> > #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
> > #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
> > #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
> > #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)
> 
> For (un)subscribe I suggest that we also use a struct with the event type
> and a few reserved fields.

Agreed.

> > As it was discussed in the LPC, event subscriptions should be bound to
> > file handle. The implementation, however, is not visible to userspace.
> > This is why I'm not specifying it in this RFC.
> >
> > While the number of possible standard (and probably private) events
> > would be quite small and the implementation could be a bit field, I do
> > see that the interface must be using types passed as numbers instead of
> > bit fields.
> >
> > Is it necessary to buffer events of same type or will an event replace
> > an older event of the same type? It probably depends on event type which
> > is better. This is also a matter of implementation.
> >
> >
> > Comments and questions are more than welcome.
> 
> Here's a mixed bag of idea/comments:
> 
> We need to define what to do when you unsubscribe an event and there are
>  still events of that type pending. Do we remove those pending events as
>  well? I think we should just keep them, but I'm open for other opinions.

It would be easier to keep them and I don't think that would hurt.

> I was wondering if a 'count' field in v4l2_event might be useful: e.g. if
>  you get multiple identical events, and that event is already registered,
>  then you can just increase the count rather than adding the same event
>  again. This might be overengineering, though. And to be honest, I can't
>  think of a use-case, but it's something to keep in mind perhaps.

That's called events compression in the GUI world. The main reason to 
implement this is efficiency when dealing with events that can occur at a high 
frequency. For instance, when moving a window and thus exposing previously 
unexposed parts that need to be redrawn, compressing all the redraw events 
generated while the window moves make sense. There could be use cases in the 
media world as well, but I think this is a case of overengineering at the 
moment. We can always implement it later, and I don't think a count field 
would be useful anyway, as events that could be repeated will probably be 
intermixed with other events.

> Would we ever need a VIDIOC_S_EVENT to let the application set an event?
> ('software events').

Using a kernel driver to pass information from one userspace application to 
another doesn't seem like a very good design IMHO. Let's not do that for now.

> Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT might
> be more appropriate.

No preference there.

> How do we prevent the event queue from overflowing? Just hardcode a
> watermark? Alternatively, when subscribing an event we can also pass the
> maximum number of allowed events as an argument.

We can't prevent it from overflowing if the userspace application isn't fast 
enough. In that case events will be discarded, and the application will find 
out using the sequence number.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Video events, version 2

[Hans Verkuil]

On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Wednesday 14 October 2009 19:48:33 Hans Verkuil wrote:
> > On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
> > > Here's the second version of the video events RFC. It's based on Laurent
> > > Pinchart's original RFC. My aim is to address the issues found in the
> > > old RFC during the V4L-DVB mini-summit in the Linux plumbers conference
> > > 2009. To get a good grasp of the problem at hand it's probably a good
> > > idea read the original RFC as well:
> > >
> > > <URL:http://www.spinics.net/lists/linux-media/msg10217.html>
> 
> Thanks for the RFC update.
> 
> > > Changes to version 1
> > > ----------------------------------
> > >
> > > struct video_event has been renamed to v4l2_event. The struct is used in
> > > userspace and V4L related structures appear to have v4l2 prefix so that
> > > should be better than video.
> 
> In the end we will probably rename that to media_ or something similar in the 
> big media controller rename (if that ever happens). For now let's keep v4l2_, 
> that will be more consistent.
> 
> > > The "entity" field has been removed from the struct v4l2_event since the
> > > subdevices will have their own device nodes --- the events should come
> > > from them instead of the media controller. Video nodes could be used for
> > > events, too.
> 
> I would still keep the entity field. It would allow for parents to report 
> children events and there could be use cases for that.

We can always convert one of the reserved fields to an entity field in the
future. Adding support in the new API for an even newer and as yet highly
experimental API is not a good idea.

> > > A few reserved fields have been added. There are new ioctls as well for
> > > enumeration and (un)subscribing.
> > >
> > >
> > > Interface description
> > > ---------------------
> > >
> > > Event type is either a standard event or private event. Standard events
> > > will be defined in videodev2.h. Private event types begin from
> > > V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future
> > > use.
> > >
> > > #define V4L2_EVENT_PRIVATE_START	0x08000000
> > > #define V4L2_EVENT_RESERVED		0x10000000
> > 
> > Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer
> >  V4L2_EVENT?
> 
> EV could be confused with electron volt, exposure value, or even escape 
> velocity (don't underestimate the use of V4L2 in the spaceship market ;-)). On 
> a more serious note, while I like to keep identifiers short, is the 3 
> characters gain worth it here ?
> 
> > > VIDIOC_ENUM_EVENT is used to enumerate the available event types. It
> > > works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next
> > > event type by calling it with the last type in the type field. The
> > > difference is that the range is not continuous like in querying controls.
> > 
> > Question: why do we need an ENUM_EVENT? I don't really see a use-case for
> >  this.
> > 
> > Also note that there are three methods in use for enumerating within V4L:
> > 
> > 1) there is an index field in the struct that starts at 0 and that the
> > application increases by 1 until the ioctl returns an error.
> > 
> > 2) old-style controls where just enumerated from CID_BASE to CID_LASTP1,
> > which is very, very ugly.
> > 
> > 3) controls new-style allow one to set bit 31 on the control ID and in that
> > case the ioctl will give you the first control with an ID that is higher
> >  than the specified ID.
> > 
> > 1 or 3 are both valid options IMHO.
> > 
> > But again, I don't see why we need it in the first place.
> 
> Applications will only subscribe to the events they can handle, so I don't 
> think enumeration is really required. We might want to provide "subscribe to 
> all" and "subscribe to none" options though, maybe as special events 
> (V4L2_EVENT_NONE, V4L2_EVENT_ALL)

Nice idea. Although we only need an EVENT_ALL. 'Subscribe to none' equals
'unsubscribe all' after all :-)

> > > VIDIOC_G_EVENT is used to get events. sequence is the event sequence
> > > number and the data is specific to driver or event type.
> 
> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to 
> retrieve multiple events.
> 
> struct v4l2_events {
> 	__u32 count;
> 	struct v4l2_event __user *events;
> };
> 
> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)

Hmm. Premature optimization. Perhaps as a future extension.

> > > The user will get the information that there's an event through
> > > exception file descriptors by using select(2). When an event is
> > > available the poll handler sets POLLPRI which wakes up select. -EINVAL
> > > will be returned if there are no pending events.
> > >
> > > VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to
> > > subscribe and unsubscribe from events. The argument is event type.
> > 
> > Two event types can be defined already (used by ivtv):
> > 
> > #define V4L2_EVENT_DECODER_STOPPED   1
> > #define V4L2_EVENT_OUTPUT_VSYNC      2
> > 
> > > struct v4l2_eventdesc {
> > > 	__u32		type;
> > > 	__u8		description[64];
> > > 	__u32		reserved[4];
> > > };
> > >
> > > struct v4l2_event {
> > > 	__u32		type;
> > > 	__u32		sequence;
> > > 	struct timeval	timestamp;
> > > 	__u8		data[64];
> > 
> > This should be a union:
> > 
> > 
> > union {
> > 	enum v4l2_field ev_output_vsync;
> > 	__u8 data[64];
> > };
> 
> The union will grow pretty big and I'm scared it would soon become a mess.

But otherwise apps need to unpack the data array. That's very user-unfriendly.
I've no problem with big unions.

As an aside: I think that eventually videodev2.h should be split up. Especially
the control section should be moved to a separate header and just be included
by videodev2.h.

> 
> > > 	__u32		reserved[4];
> > > };
> > >
> > > #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
> > > #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
> > > #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
> > > #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)
> > 
> > For (un)subscribe I suggest that we also use a struct with the event type
> > and a few reserved fields.
> 
> Agreed.
> 
> > > As it was discussed in the LPC, event subscriptions should be bound to
> > > file handle. The implementation, however, is not visible to userspace.
> > > This is why I'm not specifying it in this RFC.
> > >
> > > While the number of possible standard (and probably private) events
> > > would be quite small and the implementation could be a bit field, I do
> > > see that the interface must be using types passed as numbers instead of
> > > bit fields.
> > >
> > > Is it necessary to buffer events of same type or will an event replace
> > > an older event of the same type? It probably depends on event type which
> > > is better. This is also a matter of implementation.
> > >
> > >
> > > Comments and questions are more than welcome.
> > 
> > Here's a mixed bag of idea/comments:
> > 
> > We need to define what to do when you unsubscribe an event and there are
> >  still events of that type pending. Do we remove those pending events as
> >  well? I think we should just keep them, but I'm open for other opinions.
> 
> It would be easier to keep them and I don't think that would hurt.
> 
> > I was wondering if a 'count' field in v4l2_event might be useful: e.g. if
> >  you get multiple identical events, and that event is already registered,
> >  then you can just increase the count rather than adding the same event
> >  again. This might be overengineering, though. And to be honest, I can't
> >  think of a use-case, but it's something to keep in mind perhaps.
> 
> That's called events compression in the GUI world. The main reason to 
> implement this is efficiency when dealing with events that can occur at a high 
> frequency. For instance, when moving a window and thus exposing previously 
> unexposed parts that need to be redrawn, compressing all the redraw events 
> generated while the window moves make sense. There could be use cases in the 
> media world as well, but I think this is a case of overengineering at the 
> moment. We can always implement it later, and I don't think a count field 
> would be useful anyway, as events that could be repeated will probably be 
> intermixed with other events.
> 
> > Would we ever need a VIDIOC_S_EVENT to let the application set an event?
> > ('software events').
> 
> Using a kernel driver to pass information from one userspace application to 
> another doesn't seem like a very good design IMHO. Let's not do that for now.
> 
> > Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT might
> > be more appropriate.
> 
> No preference there.
> 
> > How do we prevent the event queue from overflowing? Just hardcode a
> > watermark? Alternatively, when subscribing an event we can also pass the
> > maximum number of allowed events as an argument.
> 
> We can't prevent it from overflowing if the userspace application isn't fast 
> enough. In that case events will be discarded, and the application will find 
> out using the sequence number.

Obviously, but my question is whether we use a fixed internal queue or
whether we make this something that the application can configure.

That said, I think the initial implementation should be that the subscribe
ioctl gets a struct with the event type and a few reserved fields so that
in the future we can use one of the reserved fields as a configuration
parameter. So for now we just have some default watermark that is set by the
driver.

Regards,

	Hans

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

Re: [RFC] Video events, version 2

[Sakari Ailus]

Hans Verkuil wrote:
> On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
>> Hi Sakari,
>>
>> On Wednesday 14 October 2009 19:48:33 Hans Verkuil wrote:
>>> On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
>>>> Here's the second version of the video events RFC. It's based on Laurent
>>>> Pinchart's original RFC. My aim is to address the issues found in the
>>>> old RFC during the V4L-DVB mini-summit in the Linux plumbers conference
>>>> 2009. To get a good grasp of the problem at hand it's probably a good
>>>> idea read the original RFC as well:
>>>>
>>>> <URL:http://www.spinics.net/lists/linux-media/msg10217.html>
>> Thanks for the RFC update.

You're welcome.

>>>> Changes to version 1
>>>> ----------------------------------
>>>>
>>>> struct video_event has been renamed to v4l2_event. The struct is used in
>>>> userspace and V4L related structures appear to have v4l2 prefix so that
>>>> should be better than video.
>> In the end we will probably rename that to media_ or something similar in the 
>> big media controller rename (if that ever happens). For now let's keep v4l2_, 
>> that will be more consistent.
>>
>>>> The "entity" field has been removed from the struct v4l2_event since the
>>>> subdevices will have their own device nodes --- the events should come
>>>> from them instead of the media controller. Video nodes could be used for
>>>> events, too.
>> I would still keep the entity field. It would allow for parents to report 
>> children events and there could be use cases for that.
> 
> We can always convert one of the reserved fields to an entity field in the
> future. Adding support in the new API for an even newer and as yet highly
> experimental API is not a good idea.

Then the entity field stays away for now?

>>>> A few reserved fields have been added. There are new ioctls as well for
>>>> enumeration and (un)subscribing.
>>>>
>>>>
>>>> Interface description
>>>> ---------------------
>>>>
>>>> Event type is either a standard event or private event. Standard events
>>>> will be defined in videodev2.h. Private event types begin from
>>>> V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future
>>>> use.
>>>>
>>>> #define V4L2_EVENT_PRIVATE_START	0x08000000
>>>> #define V4L2_EVENT_RESERVED		0x10000000
>>> Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer
>>>  V4L2_EVENT?
>> EV could be confused with electron volt, exposure value, or even escape 
>> velocity (don't underestimate the use of V4L2 in the spaceship market ;-)). On 
>> a more serious note, while I like to keep identifiers short, is the 3 
>> characters gain worth it here ?

I'll use V4L2_EVENT_ in the next RFC, too.

>>>> VIDIOC_ENUM_EVENT is used to enumerate the available event types. It
>>>> works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next
>>>> event type by calling it with the last type in the type field. The
>>>> difference is that the range is not continuous like in querying controls.
>>> Question: why do we need an ENUM_EVENT? I don't really see a use-case for
>>>  this.
>>>
>>> Also note that there are three methods in use for enumerating within V4L:
>>>
>>> 1) there is an index field in the struct that starts at 0 and that the
>>> application increases by 1 until the ioctl returns an error.
>>>
>>> 2) old-style controls where just enumerated from CID_BASE to CID_LASTP1,
>>> which is very, very ugly.
>>>
>>> 3) controls new-style allow one to set bit 31 on the control ID and in that
>>> case the ioctl will give you the first control with an ID that is higher
>>>  than the specified ID.
>>>
>>> 1 or 3 are both valid options IMHO.
>>>
>>> But again, I don't see why we need it in the first place.
>> Applications will only subscribe to the events they can handle, so I don't 
>> think enumeration is really required. We might want to provide "subscribe to 
>> all" and "subscribe to none" options though, maybe as special events 
>> (V4L2_EVENT_NONE, V4L2_EVENT_ALL)
> 
> Nice idea. Although we only need an EVENT_ALL. 'Subscribe to none' equals
> 'unsubscribe all' after all :-)

Ok.

>>>> VIDIOC_G_EVENT is used to get events. sequence is the event sequence
>>>> number and the data is specific to driver or event type.
>> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to 
>> retrieve multiple events.
>>
>> struct v4l2_events {
>> 	__u32 count;
>> 	struct v4l2_event __user *events;
>> };
>>
>> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)
> 
> Hmm. Premature optimization. Perhaps as a future extension.

That *could* save one ioctl sometimes --- then you'd no there are no 
more events coming right now. But just one should be supported IMO, 
VIDIOC_G_EVENT or VIDIOC_G_EVENTS.

>>>> The user will get the information that there's an event through
>>>> exception file descriptors by using select(2). When an event is
>>>> available the poll handler sets POLLPRI which wakes up select. -EINVAL
>>>> will be returned if there are no pending events.
>>>>
>>>> VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to
>>>> subscribe and unsubscribe from events. The argument is event type.
>>> Two event types can be defined already (used by ivtv):
>>>
>>> #define V4L2_EVENT_DECODER_STOPPED   1
>>> #define V4L2_EVENT_OUTPUT_VSYNC      2
>>>
>>>> struct v4l2_eventdesc {
>>>> 	__u32		type;
>>>> 	__u8		description[64];
>>>> 	__u32		reserved[4];
>>>> };
>>>>
>>>> struct v4l2_event {
>>>> 	__u32		type;
>>>> 	__u32		sequence;
>>>> 	struct timeval	timestamp;
>>>> 	__u8		data[64];
>>> This should be a union:
>>>
>>>
>>> union {
>>> 	enum v4l2_field ev_output_vsync;
>>> 	__u8 data[64];
>>> };
>> The union will grow pretty big and I'm scared it would soon become a mess.
> 
> But otherwise apps need to unpack the data array. That's very user-unfriendly.
> I've no problem with big unions.

The size of the structure is now 96 bytes. I guess we could make that 
around 128 to allow a bit more space for data without really affecting 
performance.

> As an aside: I think that eventually videodev2.h should be split up. Especially
> the control section should be moved to a separate header and just be included
> by videodev2.h.
> 
>>>> 	__u32		reserved[4];
>>>> };
>>>>
>>>> #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
>>>> #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
>>>> #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
>>>> #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)
>>> For (un)subscribe I suggest that we also use a struct with the event type
>>> and a few reserved fields.
>> Agreed.

Ack.

>>>> As it was discussed in the LPC, event subscriptions should be bound to
>>>> file handle. The implementation, however, is not visible to userspace.
>>>> This is why I'm not specifying it in this RFC.
>>>>
>>>> While the number of possible standard (and probably private) events
>>>> would be quite small and the implementation could be a bit field, I do
>>>> see that the interface must be using types passed as numbers instead of
>>>> bit fields.
>>>>
>>>> Is it necessary to buffer events of same type or will an event replace
>>>> an older event of the same type? It probably depends on event type which
>>>> is better. This is also a matter of implementation.
>>>>
>>>>
>>>> Comments and questions are more than welcome.
>>> Here's a mixed bag of idea/comments:
>>>
>>> We need to define what to do when you unsubscribe an event and there are
>>>  still events of that type pending. Do we remove those pending events as
>>>  well? I think we should just keep them, but I'm open for other opinions.
>> It would be easier to keep them and I don't think that would hurt.

I'd guess so, too.

>>> I was wondering if a 'count' field in v4l2_event might be useful: e.g. if
>>>  you get multiple identical events, and that event is already registered,
>>>  then you can just increase the count rather than adding the same event
>>>  again. This might be overengineering, though. And to be honest, I can't
>>>  think of a use-case, but it's something to keep in mind perhaps.
>> That's called events compression in the GUI world. The main reason to 
>> implement this is efficiency when dealing with events that can occur at a high 
>> frequency. For instance, when moving a window and thus exposing previously 
>> unexposed parts that need to be redrawn, compressing all the redraw events 
>> generated while the window moves make sense. There could be use cases in the 
>> media world as well, but I think this is a case of overengineering at the 
>> moment. We can always implement it later, and I don't think a count field 
>> would be useful anyway, as events that could be repeated will probably be 
>> intermixed with other events.

Perhaps more than four reserved fields should be allocated for the event 
structure? :-) :-)

>>> Would we ever need a VIDIOC_S_EVENT to let the application set an event?
>>> ('software events').
>> Using a kernel driver to pass information from one userspace application to 
>> another doesn't seem like a very good design IMHO. Let's not do that for now.
>>
>>> Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT might
>>> be more appropriate.
>> No preference there.

VIDIOC_DQEVENTS? :-)

>>> How do we prevent the event queue from overflowing? Just hardcode a
>>> watermark? Alternatively, when subscribing an event we can also pass the
>>> maximum number of allowed events as an argument.
>> We can't prevent it from overflowing if the userspace application isn't fast 
>> enough. In that case events will be discarded, and the application will find 
>> out using the sequence number.
> 
> Obviously, but my question is whether we use a fixed internal queue or
> whether we make this something that the application can configure.
> 
> That said, I think the initial implementation should be that the subscribe
> ioctl gets a struct with the event type and a few reserved fields so that
> in the future we can use one of the reserved fields as a configuration
> parameter. So for now we just have some default watermark that is set by the
> driver.

I'd like to think a queue size defined by the driver is fine at this 
point. It's probably depending on the driver rather than application how 
long the queue should to be. At some point old events start becoming 
uninteresting...

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

Re: [RFC] Video events, version 2

[Laurent Pinchart]

On Friday 16 October 2009 09:36:51 Sakari Ailus wrote:
> Hans Verkuil wrote:
> > On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
> >> Hi Sakari,
> >>
> >> On Wednesday 14 October 2009 19:48:33 Hans Verkuil wrote:
> >>> On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
> >>>> Here's the second version of the video events RFC. It's based on
> >>>> Laurent Pinchart's original RFC. My aim is to address the issues found
> >>>> in the old RFC during the V4L-DVB mini-summit in the Linux plumbers
> >>>> conference 2009. To get a good grasp of the problem at hand it's
> >>>> probably a good idea read the original RFC as well:
> >>>>
> >>>> <URL:http://www.spinics.net/lists/linux-media/msg10217.html>
> >>
> >> Thanks for the RFC update.
> 
> You're welcome.
> 
> >>>> Changes to version 1
> >>>> ----------------------------------
> >>>>
> >>>> struct video_event has been renamed to v4l2_event. The struct is used
> >>>> in userspace and V4L related structures appear to have v4l2 prefix so
> >>>> that should be better than video.
> >>
> >> In the end we will probably rename that to media_ or something similar
> >> in the big media controller rename (if that ever happens). For now let's
> >> keep v4l2_, that will be more consistent.
> >>
> >>>> The "entity" field has been removed from the struct v4l2_event since
> >>>> the subdevices will have their own device nodes --- the events should
> >>>> come from them instead of the media controller. Video nodes could be
> >>>> used for events, too.
> >>
> >> I would still keep the entity field. It would allow for parents to
> >> report children events and there could be use cases for that.
> >
> > We can always convert one of the reserved fields to an entity field in
> > the future. Adding support in the new API for an even newer and as yet
> > highly experimental API is not a good idea.
> 
> Then the entity field stays away for now?

As long as you add a reserved field that's marked as "don't dare touching 
this, it will be used as an entity Id later" I'm fine ;-)
 
> >>>> A few reserved fields have been added. There are new ioctls as well
> >>>> for enumeration and (un)subscribing.
> >>>>
> >>>>
> >>>> Interface description
> >>>> ---------------------
> >>>>
> >>>> Event type is either a standard event or private event. Standard
> >>>> events will be defined in videodev2.h. Private event types begin from
> >>>> V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future
> >>>> use.
> >>>>
> >>>> #define V4L2_EVENT_PRIVATE_START	0x08000000
> >>>> #define V4L2_EVENT_RESERVED		0x10000000
> >>>
> >>> Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer
> >>>  V4L2_EVENT?
> >>
> >> EV could be confused with electron volt, exposure value, or even escape
> >> velocity (don't underestimate the use of V4L2 in the spaceship market
> >> ;-)). On a more serious note, while I like to keep identifiers short, is
> >> the 3 characters gain worth it here ?
> 
> I'll use V4L2_EVENT_ in the next RFC, too.
> 
> >>>> VIDIOC_ENUM_EVENT is used to enumerate the available event types. It
> >>>> works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next
> >>>> event type by calling it with the last type in the type field. The
> >>>> difference is that the range is not continuous like in querying
> >>>> controls.
> >>>
> >>> Question: why do we need an ENUM_EVENT? I don't really see a use-case
> >>> for this.
> >>>
> >>> Also note that there are three methods in use for enumerating within
> >>> V4L:
> >>>
> >>> 1) there is an index field in the struct that starts at 0 and that the
> >>> application increases by 1 until the ioctl returns an error.
> >>>
> >>> 2) old-style controls where just enumerated from CID_BASE to
> >>> CID_LASTP1, which is very, very ugly.
> >>>
> >>> 3) controls new-style allow one to set bit 31 on the control ID and in
> >>> that case the ioctl will give you the first control with an ID that is
> >>> higher than the specified ID.
> >>>
> >>> 1 or 3 are both valid options IMHO.
> >>>
> >>> But again, I don't see why we need it in the first place.
> >>
> >> Applications will only subscribe to the events they can handle, so I
> >> don't think enumeration is really required. We might want to provide
> >> "subscribe to all" and "subscribe to none" options though, maybe as
> >> special events (V4L2_EVENT_NONE, V4L2_EVENT_ALL)
> >
> > Nice idea. Although we only need an EVENT_ALL. 'Subscribe to none' equals
> > 'unsubscribe all' after all :-)
> 
> Ok.
> 
> >>>> VIDIOC_G_EVENT is used to get events. sequence is the event sequence
> >>>> number and the data is specific to driver or event type.
> >>
> >> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to
> >> retrieve multiple events.
> >>
> >> struct v4l2_events {
> >> 	__u32 count;
> >> 	struct v4l2_event __user *events;
> >> };
> >>
> >> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)
> >
> > Hmm. Premature optimization. Perhaps as a future extension.
> 
> That *could* save one ioctl sometimes --- then you'd no there are no
> more events coming right now. But just one should be supported IMO,
> VIDIOC_G_EVENT or VIDIOC_G_EVENTS.

I forgot to mention in my last mail that we should add a flag to the 
v4l2_event structure to report if more events are pending (or even possible a 
pending event count).

> >>>> The user will get the information that there's an event through
> >>>> exception file descriptors by using select(2). When an event is
> >>>> available the poll handler sets POLLPRI which wakes up select. -EINVAL
> >>>> will be returned if there are no pending events.
> >>>>
> >>>> VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to
> >>>> subscribe and unsubscribe from events. The argument is event type.
> >>>
> >>> Two event types can be defined already (used by ivtv):
> >>>
> >>> #define V4L2_EVENT_DECODER_STOPPED   1
> >>> #define V4L2_EVENT_OUTPUT_VSYNC      2
> >>>
> >>>> struct v4l2_eventdesc {
> >>>> 	__u32		type;
> >>>> 	__u8		description[64];
> >>>> 	__u32		reserved[4];
> >>>> };
> >>>>
> >>>> struct v4l2_event {
> >>>> 	__u32		type;
> >>>> 	__u32		sequence;
> >>>> 	struct timeval	timestamp;
> >>>> 	__u8		data[64];
> >>>
> >>> This should be a union:
> >>>
> >>>
> >>> union {
> >>> 	enum v4l2_field ev_output_vsync;

Forgot to mention this, we shouldn't use enums in kernel-userspace APIs. They 
introduce ABI issues on some platforms.

> >>> 	__u8 data[64];
> >>> };
> >>
> >> The union will grow pretty big and I'm scared it would soon become a
> >> mess.
> >
> > But otherwise apps need to unpack the data array. That's very
> > user-unfriendly. I've no problem with big unions.

But then the header file that defines v4l2_event will need to include all 
headers that define the various structure for all possible events.

> The size of the structure is now 96 bytes. I guess we could make that
> around 128 to allow a bit more space for data without really affecting
> performance.
> 
> > As an aside: I think that eventually videodev2.h should be split up.
> > Especially the control section should be moved to a separate header and
> > just be included by videodev2.h.
> >
> >>>> 	__u32		reserved[4];
> >>>> };
> >>>>
> >>>> #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
> >>>> #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
> >>>> #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
> >>>> #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)
> >>>
> >>> For (un)subscribe I suggest that we also use a struct with the event
> >>> type and a few reserved fields.
> >>
> >> Agreed.
> 
> Ack.
> 
> >>>> As it was discussed in the LPC, event subscriptions should be bound to
> >>>> file handle. The implementation, however, is not visible to userspace.
> >>>> This is why I'm not specifying it in this RFC.
> >>>>
> >>>> While the number of possible standard (and probably private) events
> >>>> would be quite small and the implementation could be a bit field, I do
> >>>> see that the interface must be using types passed as numbers instead
> >>>> of bit fields.
> >>>>
> >>>> Is it necessary to buffer events of same type or will an event replace
> >>>> an older event of the same type? It probably depends on event type
> >>>> which is better. This is also a matter of implementation.
> >>>>
> >>>>
> >>>> Comments and questions are more than welcome.
> >>>
> >>> Here's a mixed bag of idea/comments:
> >>>
> >>> We need to define what to do when you unsubscribe an event and there
> >>> are still events of that type pending. Do we remove those pending
> >>> events as well? I think we should just keep them, but I'm open for
> >>> other opinions.
> >>
> >> It would be easier to keep them and I don't think that would hurt.
> 
> I'd guess so, too.
> 
> >>> I was wondering if a 'count' field in v4l2_event might be useful: e.g.
> >>> if you get multiple identical events, and that event is already
> >>> registered, then you can just increase the count rather than adding the
> >>> same event again. This might be overengineering, though. And to be
> >>> honest, I can't think of a use-case, but it's something to keep in mind
> >>> perhaps.
> >>
> >> That's called events compression in the GUI world. The main reason to
> >> implement this is efficiency when dealing with events that can occur at
> >> a high frequency. For instance, when moving a window and thus exposing
> >> previously unexposed parts that need to be redrawn, compressing all the
> >> redraw events generated while the window moves make sense. There could
> >> be use cases in the media world as well, but I think this is a case of
> >> overengineering at the moment. We can always implement it later, and I
> >> don't think a count field would be useful anyway, as events that could
> >> be repeated will probably be intermixed with other events.
> 
> Perhaps more than four reserved fields should be allocated for the event
> structure? :-) :-)

As you mentioned that we have 32 bytes left if we want to pad to 128 bytes, 
I'd go for additional reserved fields.

> >>> Would we ever need a VIDIOC_S_EVENT to let the application set an
> >>> event? ('software events').
> >>
> >> Using a kernel driver to pass information from one userspace application
> >> to another doesn't seem like a very good design IMHO. Let's not do that
> >> for now.
> >>
> >>> Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT
> >>> might be more appropriate.
> >>
> >> No preference there.
> 
> VIDIOC_DQEVENTS? :-)
> 
> >>> How do we prevent the event queue from overflowing? Just hardcode a
> >>> watermark? Alternatively, when subscribing an event we can also pass
> >>> the maximum number of allowed events as an argument.
> >>
> >> We can't prevent it from overflowing if the userspace application isn't
> >> fast enough. In that case events will be discarded, and the application
> >> will find out using the sequence number.
> >
> > Obviously, but my question is whether we use a fixed internal queue or
> > whether we make this something that the application can configure.
> >
> > That said, I think the initial implementation should be that the
> > subscribe ioctl gets a struct with the event type and a few reserved
> > fields so that in the future we can use one of the reserved fields as a
> > configuration parameter. So for now we just have some default watermark
> > that is set by the driver.
> 
> I'd like to think a queue size defined by the driver is fine at this
> point. It's probably depending on the driver rather than application how
> long the queue should to be. At some point old events start becoming
> uninteresting...

I agree, let's not worry about it right now.

-- 
Laurent Pinchart

Re: [RFC] Video events, version 2

[Hans Verkuil]

> Hans Verkuil wrote:
>> On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
>>> Hi Sakari,
>>>
>>> On Wednesday 14 October 2009 19:48:33 Hans Verkuil wrote:
>>>> On Wednesday 14 October 2009 15:02:14 Sakari Ailus wrote:
>>>>> Here's the second version of the video events RFC. It's based on
>>>>> Laurent
>>>>> Pinchart's original RFC. My aim is to address the issues found in the
>>>>> old RFC during the V4L-DVB mini-summit in the Linux plumbers
>>>>> conference
>>>>> 2009. To get a good grasp of the problem at hand it's probably a good
>>>>> idea read the original RFC as well:
>>>>>
>>>>> <URL:http://www.spinics.net/lists/linux-media/msg10217.html>
>>> Thanks for the RFC update.
>
> You're welcome.
>
>>>>> Changes to version 1
>>>>> ----------------------------------
>>>>>
>>>>> struct video_event has been renamed to v4l2_event. The struct is used
>>>>> in
>>>>> userspace and V4L related structures appear to have v4l2 prefix so
>>>>> that
>>>>> should be better than video.
>>> In the end we will probably rename that to media_ or something similar
>>> in the
>>> big media controller rename (if that ever happens). For now let's keep
>>> v4l2_,
>>> that will be more consistent.
>>>
>>>>> The "entity" field has been removed from the struct v4l2_event since
>>>>> the
>>>>> subdevices will have their own device nodes --- the events should
>>>>> come
>>>>> from them instead of the media controller. Video nodes could be used
>>>>> for
>>>>> events, too.
>>> I would still keep the entity field. It would allow for parents to
>>> report
>>> children events and there could be use cases for that.
>>
>> We can always convert one of the reserved fields to an entity field in
>> the
>> future. Adding support in the new API for an even newer and as yet
>> highly
>> experimental API is not a good idea.
>
> Then the entity field stays away for now?

Yup.

>>>>> A few reserved fields have been added. There are new ioctls as well
>>>>> for
>>>>> enumeration and (un)subscribing.
>>>>>
>>>>>
>>>>> Interface description
>>>>> ---------------------
>>>>>
>>>>> Event type is either a standard event or private event. Standard
>>>>> events
>>>>> will be defined in videodev2.h. Private event types begin from
>>>>> V4L2_EVENT_PRIVATE. Some high order bits could be reserved for future
>>>>> use.
>>>>>
>>>>> #define V4L2_EVENT_PRIVATE_START	0x08000000
>>>>> #define V4L2_EVENT_RESERVED		0x10000000
>>>> Suggestion: use the V4L2_EV_ prefix perhaps instead of the longer
>>>>  V4L2_EVENT?
>>> EV could be confused with electron volt, exposure value, or even escape
>>> velocity (don't underestimate the use of V4L2 in the spaceship market
>>> ;-)). On
>>> a more serious note, while I like to keep identifiers short, is the 3
>>> characters gain worth it here ?
>
> I'll use V4L2_EVENT_ in the next RFC, too.
>
>>>>> VIDIOC_ENUM_EVENT is used to enumerate the available event types. It
>>>>> works a bit the same way than VIDIOC_ENUM_FMT i.e. you get the next
>>>>> event type by calling it with the last type in the type field. The
>>>>> difference is that the range is not continuous like in querying
>>>>> controls.
>>>> Question: why do we need an ENUM_EVENT? I don't really see a use-case
>>>> for
>>>>  this.
>>>>
>>>> Also note that there are three methods in use for enumerating within
>>>> V4L:
>>>>
>>>> 1) there is an index field in the struct that starts at 0 and that the
>>>> application increases by 1 until the ioctl returns an error.
>>>>
>>>> 2) old-style controls where just enumerated from CID_BASE to
>>>> CID_LASTP1,
>>>> which is very, very ugly.
>>>>
>>>> 3) controls new-style allow one to set bit 31 on the control ID and in
>>>> that
>>>> case the ioctl will give you the first control with an ID that is
>>>> higher
>>>>  than the specified ID.
>>>>
>>>> 1 or 3 are both valid options IMHO.
>>>>
>>>> But again, I don't see why we need it in the first place.
>>> Applications will only subscribe to the events they can handle, so I
>>> don't
>>> think enumeration is really required. We might want to provide
>>> "subscribe to
>>> all" and "subscribe to none" options though, maybe as special events
>>> (V4L2_EVENT_NONE, V4L2_EVENT_ALL)
>>
>> Nice idea. Although we only need an EVENT_ALL. 'Subscribe to none'
>> equals
>> 'unsubscribe all' after all :-)
>
> Ok.
>
>>>>> VIDIOC_G_EVENT is used to get events. sequence is the event sequence
>>>>> number and the data is specific to driver or event type.
>>> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to
>>> retrieve multiple events.
>>>
>>> struct v4l2_events {
>>> 	__u32 count;
>>> 	struct v4l2_event __user *events;
>>> };
>>>
>>> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)
>>
>> Hmm. Premature optimization. Perhaps as a future extension.
>
> That *could* save one ioctl sometimes --- then you'd no there are no
> more events coming right now. But just one should be supported IMO,
> VIDIOC_G_EVENT or VIDIOC_G_EVENTS.

I'm not keen on using pointers insides structures unless there is a very
good reason to do so. In practice it complicates the driver code
substantially due to all the kernel-to-userspace copies that need to be
done that are normally handled by video_ioctl2. In addition it requires
custom code in the compat-ioctl32 part as well.

>>>>> The user will get the information that there's an event through
>>>>> exception file descriptors by using select(2). When an event is
>>>>> available the poll handler sets POLLPRI which wakes up select.
>>>>> -EINVAL
>>>>> will be returned if there are no pending events.
>>>>>
>>>>> VIDIOC_SUBSCRIBE_EVENT and VIDIOC_UNSUBSCRIBE_EVENT are used to
>>>>> subscribe and unsubscribe from events. The argument is event type.
>>>> Two event types can be defined already (used by ivtv):
>>>>
>>>> #define V4L2_EVENT_DECODER_STOPPED   1
>>>> #define V4L2_EVENT_OUTPUT_VSYNC      2
>>>>
>>>>> struct v4l2_eventdesc {
>>>>> 	__u32		type;
>>>>> 	__u8		description[64];
>>>>> 	__u32		reserved[4];
>>>>> };
>>>>>
>>>>> struct v4l2_event {
>>>>> 	__u32		type;
>>>>> 	__u32		sequence;
>>>>> 	struct timeval	timestamp;
>>>>> 	__u8		data[64];
>>>> This should be a union:
>>>>
>>>>
>>>> union {
>>>> 	enum v4l2_field ev_output_vsync;
>>>> 	__u8 data[64];
>>>> };
>>> The union will grow pretty big and I'm scared it would soon become a
>>> mess.
>>
>> But otherwise apps need to unpack the data array. That's very
>> user-unfriendly.
>> I've no problem with big unions.
>
> The size of the structure is now 96 bytes. I guess we could make that
> around 128 to allow a bit more space for data without really affecting
> performance.

With 'big unions' I didn't mean the memory size. I think 64 bytes (16
longs) is a decent size. I was talking about the union definition in the
videodev2.h header.

>> As an aside: I think that eventually videodev2.h should be split up.
>> Especially
>> the control section should be moved to a separate header and just be
>> included
>> by videodev2.h.
>>
>>>>> 	__u32		reserved[4];
>>>>> };
>>>>>
>>>>> #define VIDIOC_ENUM_EVENT	_IORW('V', 83, struct v4l2_eventdesc)
>>>>> #define VIDIOC_G_EVENT		_IOR('V', 84, struct v4l2_event)
>>>>> #define VIDIOC_SUBSCRIBE_EVENT	_IOW('V', 85, __u32)
>>>>> #define VIDIOC_UNSUBSCRIBE_EVENT _IOW('V', 86, __u32)
>>>> For (un)subscribe I suggest that we also use a struct with the event
>>>> type
>>>> and a few reserved fields.
>>> Agreed.
>
> Ack.
>
>>>>> As it was discussed in the LPC, event subscriptions should be bound
>>>>> to
>>>>> file handle. The implementation, however, is not visible to
>>>>> userspace.
>>>>> This is why I'm not specifying it in this RFC.
>>>>>
>>>>> While the number of possible standard (and probably private) events
>>>>> would be quite small and the implementation could be a bit field, I
>>>>> do
>>>>> see that the interface must be using types passed as numbers instead
>>>>> of
>>>>> bit fields.
>>>>>
>>>>> Is it necessary to buffer events of same type or will an event
>>>>> replace
>>>>> an older event of the same type? It probably depends on event type
>>>>> which
>>>>> is better. This is also a matter of implementation.
>>>>>
>>>>>
>>>>> Comments and questions are more than welcome.
>>>> Here's a mixed bag of idea/comments:
>>>>
>>>> We need to define what to do when you unsubscribe an event and there
>>>> are
>>>>  still events of that type pending. Do we remove those pending events
>>>> as
>>>>  well? I think we should just keep them, but I'm open for other
>>>> opinions.
>>> It would be easier to keep them and I don't think that would hurt.
>
> I'd guess so, too.
>
>>>> I was wondering if a 'count' field in v4l2_event might be useful: e.g.
>>>> if
>>>>  you get multiple identical events, and that event is already
>>>> registered,
>>>>  then you can just increase the count rather than adding the same
>>>> event
>>>>  again. This might be overengineering, though. And to be honest, I
>>>> can't
>>>>  think of a use-case, but it's something to keep in mind perhaps.
>>> That's called events compression in the GUI world. The main reason to
>>> implement this is efficiency when dealing with events that can occur at
>>> a high
>>> frequency. For instance, when moving a window and thus exposing
>>> previously
>>> unexposed parts that need to be redrawn, compressing all the redraw
>>> events
>>> generated while the window moves make sense. There could be use cases
>>> in the
>>> media world as well, but I think this is a case of overengineering at
>>> the
>>> moment. We can always implement it later, and I don't think a count
>>> field
>>> would be useful anyway, as events that could be repeated will probably
>>> be
>>> intermixed with other events.
>
> Perhaps more than four reserved fields should be allocated for the event
> structure? :-) :-)
>
>>>> Would we ever need a VIDIOC_S_EVENT to let the application set an
>>>> event?
>>>> ('software events').
>>> Using a kernel driver to pass information from one userspace
>>> application to
>>> another doesn't seem like a very good design IMHO. Let's not do that
>>> for now.
>>>
>>>> Rather than naming the ioctl VIDIOC_G_EVENT, perhaps VIDIOC_DQEVENT
>>>> might
>>>> be more appropriate.
>>> No preference there.
>
> VIDIOC_DQEVENTS? :-)
>
>>>> How do we prevent the event queue from overflowing? Just hardcode a
>>>> watermark? Alternatively, when subscribing an event we can also pass
>>>> the
>>>> maximum number of allowed events as an argument.
>>> We can't prevent it from overflowing if the userspace application isn't
>>> fast
>>> enough. In that case events will be discarded, and the application will
>>> find
>>> out using the sequence number.
>>
>> Obviously, but my question is whether we use a fixed internal queue or
>> whether we make this something that the application can configure.
>>
>> That said, I think the initial implementation should be that the
>> subscribe
>> ioctl gets a struct with the event type and a few reserved fields so
>> that
>> in the future we can use one of the reserved fields as a configuration
>> parameter. So for now we just have some default watermark that is set by
>> the
>> driver.
>
> I'd like to think a queue size defined by the driver is fine at this
> point. It's probably depending on the driver rather than application how
> long the queue should to be. At some point old events start becoming
> uninteresting...

Question: will we drop old events or new events? Or make this
configurable? Or driver dependent?

Regards,

         Hans

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

Re: [RFC] Video events, version 2

[Sakari Ailus]

Hans Verkuil wrote:
[clip]
> I'm not keen on using pointers insides structures unless there is a very
> good reason to do so. In practice it complicates the driver code
> substantially due to all the kernel-to-userspace copies that need to be
> done that are normally handled by video_ioctl2. In addition it requires
> custom code in the compat-ioctl32 part as well.

VIDIOC_DQEVENT then.

[clip]
>> The size of the structure is now 96 bytes. I guess we could make that
>> around 128 to allow a bit more space for data without really affecting
>> performance.
> 
> With 'big unions' I didn't mean the memory size. I think 64 bytes (16
> longs) is a decent size. I was talking about the union definition in the
> videodev2.h header.

That was a badly placed comment, but I meant the memory size. I have 
currently no opinion on whether to use union or not.

[clip]
>>> That said, I think the initial implementation should be that the
>>> subscribe
>>> ioctl gets a struct with the event type and a few reserved fields so
>>> that
>>> in the future we can use one of the reserved fields as a configuration
>>> parameter. So for now we just have some default watermark that is set by
>>> the
>>> driver.
>> I'd like to think a queue size defined by the driver is fine at this
>> point. It's probably depending on the driver rather than application how
>> long the queue should to be. At some point old events start becoming
>> uninteresting...
> 
> Question: will we drop old events or new events? Or make this
> configurable? Or driver dependent?

This should the same than for video buffers. I guess it's undefined? I'd 
let the driver decide at this point.

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

Re: [RFC] Video events, version 2

[Laurent Pinchart]

On Friday 16 October 2009 10:55:04 Sakari Ailus wrote:
> Hans Verkuil wrote:
> [clip]
> 
> > I'm not keen on using pointers insides structures unless there is a very
> > good reason to do so. In practice it complicates the driver code
> > substantially due to all the kernel-to-userspace copies that need to be
> > done that are normally handled by video_ioctl2. In addition it requires
> > custom code in the compat-ioctl32 part as well.
> 
> VIDIOC_DQEVENT then.
> 
> [clip]
> 
> >> The size of the structure is now 96 bytes. I guess we could make that
> >> around 128 to allow a bit more space for data without really affecting
> >> performance.
> >
> > With 'big unions' I didn't mean the memory size. I think 64 bytes (16
> > longs) is a decent size. I was talking about the union definition in the
> > videodev2.h header.
> 
> That was a badly placed comment, but I meant the memory size. I have
> currently no opinion on whether to use union or not.
> 
> [clip]
> 
> >>> That said, I think the initial implementation should be that the
> >>> subscribe
> >>> ioctl gets a struct with the event type and a few reserved fields so
> >>> that
> >>> in the future we can use one of the reserved fields as a configuration
> >>> parameter. So for now we just have some default watermark that is set
> >>> by the
> >>> driver.
> >>
> >> I'd like to think a queue size defined by the driver is fine at this
> >> point. It's probably depending on the driver rather than application how
> >> long the queue should to be. At some point old events start becoming
> >> uninteresting...
> >
> > Question: will we drop old events or new events? Or make this
> > configurable? Or driver dependent?
> 
> This should the same than for video buffers. I guess it's undefined? I'd
> let the driver decide at this point.

>From the user point of view it won't make much difference. The same number of 
consecutive events will be lost. Which ones will depend on the events arrival 
time and when/how long the application fails to retrieve pending events. I 
agree to let the driver decide (or rather the v4l2 core, as the queue will be 
implemented there).

-- 
Laurent Pinchart

Re: [RFC] Video events, version 2

[Sakari Ailus]

Laurent Pinchart wrote:
> On Friday 16 October 2009 09:36:51 Sakari Ailus wrote:
>> Hans Verkuil wrote:
>>> On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
>>>> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to
>>>> retrieve multiple events.
>>>>
>>>> struct v4l2_events {
>>>> 	__u32 count;
>>>> 	struct v4l2_event __user *events;
>>>> };
>>>>
>>>> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)
>>> Hmm. Premature optimization. Perhaps as a future extension.
>> That *could* save one ioctl sometimes --- then you'd no there are no
>> more events coming right now. But just one should be supported IMO,
>> VIDIOC_G_EVENT or VIDIOC_G_EVENTS.
> 
> I forgot to mention in my last mail that we should add a flag to the 
> v4l2_event structure to report if more events are pending (or even possible a 
> pending event count).

Then the V4L (or driver) would have to check the queue for that type of 
events. OTOH, the queue size could be quite small and it'd never 
overflow since the maximum size is number of different event types.

Can there be situations when the first or last event timestamp of 
certain event would be necessary? If we put count there, then we need to 
make a decision which one is useful for the userspace. The last one is 
obviously useful for the AF/AEWB algorightms. I currently see no use for 
the first one, but that doesn't mean there couldn't be use for it.

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

Re: [RFC] Video events, version 2

[Laurent Pinchart]

On Friday 16 October 2009 14:34:28 Sakari Ailus wrote:
> Laurent Pinchart wrote:
> > On Friday 16 October 2009 09:36:51 Sakari Ailus wrote:
> >> Hans Verkuil wrote:
> >>> On Thursday 15 October 2009 23:11:33 Laurent Pinchart wrote:
> >>>> For efficiency reasons a V4L2_G_EVENTS ioctl could also be provided to
> >>>> retrieve multiple events.
> >>>>
> >>>> struct v4l2_events {
> >>>> 	__u32 count;
> >>>> 	struct v4l2_event __user *events;
> >>>> };
> >>>>
> >>>> #define VIDIOC_G_EVENTS _IOW('V', xx, struct v4l2_events)
> >>>
> >>> Hmm. Premature optimization. Perhaps as a future extension.
> >>
> >> That *could* save one ioctl sometimes --- then you'd no there are no
> >> more events coming right now. But just one should be supported IMO,
> >> VIDIOC_G_EVENT or VIDIOC_G_EVENTS.
> >
> > I forgot to mention in my last mail that we should add a flag to the
> > v4l2_event structure to report if more events are pending (or even
> > possible a pending event count).
> 
> Then the V4L (or driver) would have to check the queue for that type of
> events. OTOH, the queue size could be quite small and it'd never
> overflow since the maximum size is number of different event types.
> 
> Can there be situations when the first or last event timestamp of
> certain event would be necessary? If we put count there, then we need to
> make a decision which one is useful for the userspace. The last one is
> obviously useful for the AF/AEWB algorightms. I currently see no use for
> the first one, but that doesn't mean there couldn't be use for it.

That's not what I meant. The idea of a count field is to report the number of 
events still pending after that one, type aside. If v4l2_event::count equals 0 
the userspace application will know there is no need to call VIDIOC_G_EVENT 
just to get a -EAGAIN. 

-- 
Laurent Pinchart

Re: [RFC] Video events, version 2

[Sakari Ailus]

Laurent Pinchart wrote:
> That's not what I meant. The idea of a count field is to report the number of 
> events still pending after that one, type aside. If v4l2_event::count equals 0 
> the userspace application will know there is no need to call VIDIOC_G_EVENT 
> just to get a -EAGAIN. 

Thanks for the clarification.

Sounds good to me, I'll put this to the next version.

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