[RFC] Support for events with a large payload

[RFC] Support for events with a large payload

[Hans Verkuil]

Currently the event API allows for a payload of up to 64 bytes. Sometimes we
would like to pass larger payloads to userspace such as metadata associated
with a particular video stream.

A typical example of that would be object detection events.

This RFC describes one approach for doing this.

The event framework has the nice property of being able to use from within
interrupts. Copying large payloads does not fit into that framework, so
such payloads should be adminstrated separately.

In addition, I wouldn't allow large payloads to be filled in from interrupt
context: a worker queue would be much more appropriate.

Note that the event API is only useful for relatively low-bandwidth data
since the data is always copied. When dealing with high-bandwidth data the
data should either be a separate plane or become a special stream I/O buffer
type.

The userspace API enhancements in order to achieve this would be the
following:

- Any event that has a large payload would specify a payload_sequence counter
  and a payload size value (in bytes).

- A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event type,
  the payload_sequence counter and a pointer to a buffer to the kernel, and the
  payload is returned, or an error is returned if the payload data is no longer
  available.

Optional enhancements:

- Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
  preallocate payload memory, but it might be overkill).

- Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
  events in the event queue for the filehandle. If the payload is large,
  you may want to limit the number of allocated payload buffers. For
  example: when dealing with metadata associated with frame you might want
  to limit the number of payload buffers to the number of allocated frames.

I feel that this can always be added later if we decide it is really needed,
and leave it out for now.

So the userspace API would be quite simple.

The internal implementation would look like this:

struct v4l2_event_payload {
	u32 payload_size;
	u32 payload_sequence;
	void *payload;
};

struct v4l2_event_payloads {
	// lock serializing access to this struct
	struct mutex lock;
	// global payload sequence number counter
	u32 payload_sequence;
	// size of the payload array
	unsigned payloads;
	// index of the oldest payload
	unsigned first;
	// number of payloads available
	unsigned in_use; 
	struct v4l2_event_payload payloads[];
};

and a pointer to struct v4l2_event_payloads would be added to struct
v4l2_subscribed_event.

It is up to the driver to decide whether there is one v4l2_event_payloads
struct per filehandle or whether there is a global struct shared by any
filehandle subscribed to this event. This will depend on the event and the
size of the payload. Most likely it will be the latter option (a global
queue of payloads).

Internal functions would look like this:

// Initialize the structs.
void v4l2_event_payloads_init(struct v4l2_event_payloads *p, unsigned payloads);
// Get the first available payload (the mutex must be locked). If no payloads
// are available then the oldest payload will be reused. A new sequence number
// will be generated as well.
struct v4l2_event_payload *v4l2_event_payloads_new(struct v4l2_event_payloads *p);
// Find the payload with the given sequence number. The mutex must be locked.
struct v4l2_event_payload *v4l2_event_payloads_find(struct v4l2_event_payloads *p, unsigned seqnr);

So when a new payload arrives the driver would take the mutex, call
v4l2_event_payloads_new(), set payload_size and fill the payload data,
remember the payload_size and payload_sequence values, release the mutex
and queue the event with the remembered size and sequence values. Setting
up the payload part cannot be done from interrupt context.

When calling DQEVENT_PAYLOAD the core will use the pointer to struct
v4l2_event_payloads from struct v4l2_subscribed_event, take the mutex,
find the payload, copy it to userspace and release the mutex.

Right now the mutex is in struct v4l2_event_payloads. This is not optimal:
it might be better to have a spinlock for controlling access to the
v4l2_event_payloads struct and a mutex for each v4l2_event_payload struct.
That way setting and getting two different payload structs wouldn't depend
on one another.

Comments?

	Hans

PS: I have no immediate plans to implement this, but based on recent discussions
it seems there is a desire to have support for this at some point in the future,
so I decided to see how this could be implemented.

Re: [RFC] Support for events with a large payload

[Sakari Ailus]

Hi Hans,

Thanks for the RFC! :-)

On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> Currently the event API allows for a payload of up to 64 bytes. Sometimes we
> would like to pass larger payloads to userspace such as metadata associated
> with a particular video stream.
> 
> A typical example of that would be object detection events.
> 
> This RFC describes one approach for doing this.
> 
> The event framework has the nice property of being able to use from within
> interrupts. Copying large payloads does not fit into that framework, so
> such payloads should be adminstrated separately.
> 
> In addition, I wouldn't allow large payloads to be filled in from interrupt
> context: a worker queue would be much more appropriate.

How large really is "large"? 65 bytes? 64 kiB?

The newer CPUs tend to be faster and faster and the same applies to memory.
I guess threaded interrupt handlers are still nice. But using a mutex in
order to serialise access to the struct will force drivers to use threaded
interrupt handlers should they want to generate large events.

> Note that the event API is only useful for relatively low-bandwidth data
> since the data is always copied. When dealing with high-bandwidth data the
> data should either be a separate plane or become a special stream I/O buffer
> type.
> 
> The userspace API enhancements in order to achieve this would be the
> following:
> 
> - Any event that has a large payload would specify a payload_sequence counter
>   and a payload size value (in bytes).
> 
> - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event type,
>   the payload_sequence counter and a pointer to a buffer to the kernel, and the
>   payload is returned, or an error is returned if the payload data is no longer
>   available.

Do you think we should have a separate IOCTL for this? The downside is that
to dequeue events, the application would need to try both in the worst case
just to obtain an event.

I think it'd be nicer to be able to fit that into the same IOCTL. There are
plenty of reserved fields and, actually, the event data as well: we could
consider the large-payload event a meta-event which would contain the
required information to pass the event data to the user space. The type of
such an event could be V4L2_EVENT_LARGE, for instance.

> Optional enhancements:
> 
> - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
>   preallocate payload memory, but it might be overkill).

Why so? We could use a reserved field as well. The size would be zero if the
maximum would be less than 64.

> - Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
>   events in the event queue for the filehandle. If the payload is large,
>   you may want to limit the number of allocated payload buffers. For
>   example: when dealing with metadata associated with frame you might want
>   to limit the number of payload buffers to the number of allocated frames.

Are we talking now about high level metadata which is typically obtained by
the driver by other means than hardware writing it into a memory buffer?

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Support for events with a large payload

[Laurent Pinchart]

Hi Hans and Sakari,

On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the RFC! :-)
> 
> On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > we would like to pass larger payloads to userspace such as metadata
> > associated with a particular video stream.
> > 
> > A typical example of that would be object detection events.
> > 
> > This RFC describes one approach for doing this.
> > 
> > The event framework has the nice property of being able to use from within
> > interrupts. Copying large payloads does not fit into that framework, so
> > such payloads should be adminstrated separately.
> > 
> > In addition, I wouldn't allow large payloads to be filled in from
> > interrupt context: a worker queue would be much more appropriate.
> 
> How large really is "large"? 65 bytes? 64 kiB?
> 
> The newer CPUs tend to be faster and faster and the same applies to memory.
> I guess threaded interrupt handlers are still nice. But using a mutex in
> order to serialise access to the struct will force drivers to use threaded
> interrupt handlers should they want to generate large events.
> 
> > Note that the event API is only useful for relatively low-bandwidth data
> > since the data is always copied. When dealing with high-bandwidth data the
> > data should either be a separate plane or become a special stream I/O
> > buffer type.
> > 
> > The userspace API enhancements in order to achieve this would be the
> > following:
> > 
> > - Any event that has a large payload would specify a payload_sequence
> >   counter and a payload size value (in bytes).
> > 
> > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> >   type, the payload_sequence counter and a pointer to a buffer to the
> >   kernel, and the payload is returned, or an error is returned if the
> >   payload data is no longer available.
> 
> Do you think we should have a separate IOCTL for this? The downside is that
> to dequeue events, the application would need to try both in the worst case
> just to obtain an event.
> 
> I think it'd be nicer to be able to fit that into the same IOCTL. There are
> plenty of reserved fields and, actually, the event data as well: we could
> consider the large-payload event a meta-event which would contain the
> required information to pass the event data to the user space. The type of
> such an event could be V4L2_EVENT_LARGE, for instance.

The problem is that userspace doesn't know in advance how much memory it would 
need to allocate to store the event payload. Using two ioctls allow retrieving 
the size first, and then the payload. We could work around the problem by 
passing the maximum size to userspace and forcing preallocation of large 
enough buffers.

> > Optional enhancements:
> > 
> > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> >   preallocate payload memory, but it might be overkill).
> 
> Why so? We could use a reserved field as well. The size would be zero if the
> maximum would be less than 64.
> 
> > - Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
> >   events in the event queue for the filehandle. If the payload is large,
> >   you may want to limit the number of allocated payload buffers. For
> >   example: when dealing with metadata associated with frame you might want
> >   to limit the number of payload buffers to the number of allocated
> >   frames.
> 
> Are we talking now about high level metadata which is typically obtained by
> the driver by other means than hardware writing it into a memory buffer?
> 
> > I feel that this can always be added later if we decide it is really
> > needed, and leave it out for now.
> > 
> > So the userspace API would be quite simple.
> > 
> > The internal implementation would look like this:
> > 
> > struct v4l2_event_payload {
> > 	u32 payload_size;
> > 	u32 payload_sequence;
> > 	void *payload;
> > };
> > 
> > struct v4l2_event_payloads {
> > 	// lock serializing access to this struct
> > 	struct mutex lock;
> > 	// global payload sequence number counter
> > 	u32 payload_sequence;
> > 	// size of the payload array
> > 	unsigned payloads;
> > 	// index of the oldest payload
> > 	unsigned first;
> > 	// number of payloads available
> > 	unsigned in_use;
> > 	struct v4l2_event_payload payloads[];
> > };
> > 
> > and a pointer to struct v4l2_event_payloads would be added to struct
> > v4l2_subscribed_event.
> > 
> > It is up to the driver to decide whether there is one v4l2_event_payloads
> > struct per filehandle or whether there is a global struct shared by any
> > filehandle subscribed to this event. This will depend on the event and the
> > size of the payload. Most likely it will be the latter option (a global
> > queue of payloads).
> > 
> > Internal functions would look like this:
> > 
> > // Initialize the structs.
> > void v4l2_event_payloads_init(struct v4l2_event_payloads *p, unsigned
> > payloads);
> > // Get the first available payload (the mutex must be locked). If no
> > // payloads are available then the oldest payload will be reused. A new
> > // sequence number will be generated as well.
> > struct v4l2_event_payload *
> > v4l2_event_payloads_new(struct v4l2_event_payloads *p);
> > // Find the payload with the given sequence number. The mutex must be
> > // locked.
> > struct v4l2_event_payload
> > *v4l2_event_payloads_find(struct v4l2_event_payloads *p, unsigned seqnr);
> > 
> > So when a new payload arrives the driver would take the mutex, call
> > v4l2_event_payloads_new(), set payload_size and fill the payload data,
> > remember the payload_size and payload_sequence values, release the mutex
> > and queue the event with the remembered size and sequence values. Setting
> > up the payload part cannot be done from interrupt context.
> > 
> > When calling DQEVENT_PAYLOAD the core will use the pointer to struct
> > v4l2_event_payloads from struct v4l2_subscribed_event, take the mutex,
> > find the payload, copy it to userspace and release the mutex.

Do we want to allow dequeuing payloads out-of-order ? If we store payload in a 
global location that would be unavoidable, but if we store them in the file 
handle then we could force in-order dequeuing. It might make sense to forbit 
out-of-order dequeuing in the API to leave some flexibility to the in-kernel 
implementation.

> > Right now the mutex is in struct v4l2_event_payloads. This is not optimal:
> > it might be better to have a spinlock for controlling access to the
> > v4l2_event_payloads struct and a mutex for each v4l2_event_payload struct.
> > That way setting and getting two different payload structs wouldn't depend
> > on one another.

-- 
Regards,

Laurent Pinchart

Re: [RFC] Support for events with a large payload

[Hans Verkuil]

On Tue June 18 2013 23:22:33 Laurent Pinchart wrote:
> Hi Hans and Sakari,
> 
> On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the RFC! :-)
> > 
> > On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > > we would like to pass larger payloads to userspace such as metadata
> > > associated with a particular video stream.
> > > 
> > > A typical example of that would be object detection events.
> > > 
> > > This RFC describes one approach for doing this.
> > > 
> > > The event framework has the nice property of being able to use from within
> > > interrupts. Copying large payloads does not fit into that framework, so
> > > such payloads should be adminstrated separately.
> > > 
> > > In addition, I wouldn't allow large payloads to be filled in from
> > > interrupt context: a worker queue would be much more appropriate.
> > 
> > How large really is "large"? 65 bytes? 64 kiB?

More than 64 bytes, which is what v4l2_event supports today.

In general events have no or small payloads. So for the majority 64 bytes
is enough.

> > 
> > The newer CPUs tend to be faster and faster and the same applies to memory.
> > I guess threaded interrupt handlers are still nice. But using a mutex in
> > order to serialise access to the struct will force drivers to use threaded
> > interrupt handlers should they want to generate large events.

But there are still lots of embedded systems out there that are not fast. Or
that run at reduced frequency to preserve power, etc., etc.

Interrupts should be fast, and 64 bytes seems a reasonable limit to me.

> > > Note that the event API is only useful for relatively low-bandwidth data
> > > since the data is always copied. When dealing with high-bandwidth data the
> > > data should either be a separate plane or become a special stream I/O
> > > buffer type.
> > > 
> > > The userspace API enhancements in order to achieve this would be the
> > > following:
> > > 
> > > - Any event that has a large payload would specify a payload_sequence
> > >   counter and a payload size value (in bytes).
> > > 
> > > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> > >   type, the payload_sequence counter and a pointer to a buffer to the
> > >   kernel, and the payload is returned, or an error is returned if the
> > >   payload data is no longer available.
> > 
> > Do you think we should have a separate IOCTL for this? The downside is that
> > to dequeue events, the application would need to try both in the worst case
> > just to obtain an event.
> > 
> > I think it'd be nicer to be able to fit that into the same IOCTL. There are
> > plenty of reserved fields and, actually, the event data as well: we could
> > consider the large-payload event a meta-event which would contain the
> > required information to pass the event data to the user space. The type of
> > such an event could be V4L2_EVENT_LARGE, for instance.
> 
> The problem is that userspace doesn't know in advance how much memory it would 
> need to allocate to store the event payload. Using two ioctls allow retrieving 
> the size first, and then the payload. We could work around the problem by 
> passing the maximum size to userspace and forcing preallocation of large 
> enough buffers.

The problem is also that you don't know what event you will get when calling
DQEVENT. So you would have to figure out the maximum of the payload sizes of all
subscribed events which is rather awkward.

> > > Optional enhancements:
> > > 
> > > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> > >   preallocate payload memory, but it might be overkill).
> > 
> > Why so? We could use a reserved field as well. The size would be zero if the
> > maximum would be less than 64.

I'm undecided here. Implementing support for this in an application is probably
the best way to discover whether or not it is useful to supply the maximum
payload size to the user. I suspect that this is actually useful.
 
> > > - Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
> > >   events in the event queue for the filehandle. If the payload is large,
> > >   you may want to limit the number of allocated payload buffers. For
> > >   example: when dealing with metadata associated with frame you might want
> > >   to limit the number of payload buffers to the number of allocated
> > >   frames.
> > 
> > Are we talking now about high level metadata which is typically obtained by
> > the driver by other means than hardware writing it into a memory buffer?

I would say that it relates to metadata that is related to a video frame, but
that does not arrive together with the video frame itself (either earlier or
later).

Since the number of buffers depends on what userspace requests, it might make
sense to give the same number when subscribing to the event.

There are other reasons why this might be useful: while the event mechanism
is pretty smart and can either drop the oldest event or merges the last two
oldest events into one whenever you try to add an event to a full event queue,
you still lose intermediate events. There may be cases where you want to have
a larger event queue to prevent that.

In all honesty, I think this is something that should only be done if we have
a concrete use-case for it.

> > > I feel that this can always be added later if we decide it is really
> > > needed, and leave it out for now.
> > > 
> > > So the userspace API would be quite simple.
> > > 
> > > The internal implementation would look like this:
> > > 
> > > struct v4l2_event_payload {
> > > 	u32 payload_size;
> > > 	u32 payload_sequence;
> > > 	void *payload;
> > > };
> > > 
> > > struct v4l2_event_payloads {
> > > 	// lock serializing access to this struct
> > > 	struct mutex lock;
> > > 	// global payload sequence number counter
> > > 	u32 payload_sequence;
> > > 	// size of the payload array
> > > 	unsigned payloads;
> > > 	// index of the oldest payload
> > > 	unsigned first;
> > > 	// number of payloads available
> > > 	unsigned in_use;
> > > 	struct v4l2_event_payload payloads[];
> > > };
> > > 
> > > and a pointer to struct v4l2_event_payloads would be added to struct
> > > v4l2_subscribed_event.
> > > 
> > > It is up to the driver to decide whether there is one v4l2_event_payloads
> > > struct per filehandle or whether there is a global struct shared by any
> > > filehandle subscribed to this event. This will depend on the event and the
> > > size of the payload. Most likely it will be the latter option (a global
> > > queue of payloads).
> > > 
> > > Internal functions would look like this:
> > > 
> > > // Initialize the structs.
> > > void v4l2_event_payloads_init(struct v4l2_event_payloads *p, unsigned
> > > payloads);
> > > // Get the first available payload (the mutex must be locked). If no
> > > // payloads are available then the oldest payload will be reused. A new
> > > // sequence number will be generated as well.
> > > struct v4l2_event_payload *
> > > v4l2_event_payloads_new(struct v4l2_event_payloads *p);
> > > // Find the payload with the given sequence number. The mutex must be
> > > // locked.
> > > struct v4l2_event_payload
> > > *v4l2_event_payloads_find(struct v4l2_event_payloads *p, unsigned seqnr);
> > > 
> > > So when a new payload arrives the driver would take the mutex, call
> > > v4l2_event_payloads_new(), set payload_size and fill the payload data,
> > > remember the payload_size and payload_sequence values, release the mutex
> > > and queue the event with the remembered size and sequence values. Setting
> > > up the payload part cannot be done from interrupt context.
> > > 
> > > When calling DQEVENT_PAYLOAD the core will use the pointer to struct
> > > v4l2_event_payloads from struct v4l2_subscribed_event, take the mutex,
> > > find the payload, copy it to userspace and release the mutex.
> 
> Do we want to allow dequeuing payloads out-of-order ? If we store payload in a 
> global location that would be unavoidable, but if we store them in the file 
> handle then we could force in-order dequeuing. It might make sense to forbit 
> out-of-order dequeuing in the API to leave some flexibility to the in-kernel 
> implementation.

You must allow this: different filehandles may be subscribed to the same event.
So the first fh might request payload 10, while the second fh is lagging behind
and requests payload 8.

In the end it is an implementation issue since there is no guarantee anyway
that you will find the payload since there is only a limited number of payloads
available. So if you wait too long to obtain the payload it will be gone.

Regards,

	Hans

> > > Right now the mutex is in struct v4l2_event_payloads. This is not optimal:
> > > it might be better to have a spinlock for controlling access to the
> > > v4l2_event_payloads struct and a mutex for each v4l2_event_payload struct.
> > > That way setting and getting two different payload structs wouldn't depend
> > > on one another.
> 
> 

Re: [RFC] Support for events with a large payload

[Sakari Ailus]

Hi Laurent,

On Tue, Jun 18, 2013 at 11:22:33PM +0200, Laurent Pinchart wrote:
> On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> > On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > > we would like to pass larger payloads to userspace such as metadata
> > > associated with a particular video stream.
> > > 
> > > A typical example of that would be object detection events.
> > > 
> > > This RFC describes one approach for doing this.
> > > 
> > > The event framework has the nice property of being able to use from within
> > > interrupts. Copying large payloads does not fit into that framework, so
> > > such payloads should be adminstrated separately.
> > > 
> > > In addition, I wouldn't allow large payloads to be filled in from
> > > interrupt context: a worker queue would be much more appropriate.
> > 
> > How large really is "large"? 65 bytes? 64 kiB?
> > 
> > The newer CPUs tend to be faster and faster and the same applies to memory.
> > I guess threaded interrupt handlers are still nice. But using a mutex in
> > order to serialise access to the struct will force drivers to use threaded
> > interrupt handlers should they want to generate large events.
> > 
> > > Note that the event API is only useful for relatively low-bandwidth data
> > > since the data is always copied. When dealing with high-bandwidth data the
> > > data should either be a separate plane or become a special stream I/O
> > > buffer type.
> > > 
> > > The userspace API enhancements in order to achieve this would be the
> > > following:
> > > 
> > > - Any event that has a large payload would specify a payload_sequence
> > >   counter and a payload size value (in bytes).
> > > 
> > > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> > >   type, the payload_sequence counter and a pointer to a buffer to the
> > >   kernel, and the payload is returned, or an error is returned if the
> > >   payload data is no longer available.
> > 
> > Do you think we should have a separate IOCTL for this? The downside is that
> > to dequeue events, the application would need to try both in the worst case
> > just to obtain an event.
> > 
> > I think it'd be nicer to be able to fit that into the same IOCTL. There are
> > plenty of reserved fields and, actually, the event data as well: we could
> > consider the large-payload event a meta-event which would contain the
> > required information to pass the event data to the user space. The type of
> > such an event could be V4L2_EVENT_LARGE, for instance.
> 
> The problem is that userspace doesn't know in advance how much memory it would 
> need to allocate to store the event payload. Using two ioctls allow retrieving 
> the size first, and then the payload. We could work around the problem by 
> passing the maximum size to userspace and forcing preallocation of large 
> enough buffers.

That would also force using double the number of IOCTLs to do essentially
the same. I don't like the idea, even if IOCTLs are relatively cheap.
Maximun size should be defined, at least per event type, so that the user
knows maximum what to expect.

> > > Optional enhancements:
> > > 
> > > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> > >   preallocate payload memory, but it might be overkill).
> > 
> > Why so? We could use a reserved field as well. The size would be zero if the
> > maximum would be less than 64.
> > 
> > > - Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
> > >   events in the event queue for the filehandle. If the payload is large,
> > >   you may want to limit the number of allocated payload buffers. For
> > >   example: when dealing with metadata associated with frame you might want
> > >   to limit the number of payload buffers to the number of allocated
> > >   frames.
> > 
> > Are we talking now about high level metadata which is typically obtained by
> > the driver by other means than hardware writing it into a memory buffer?
> > 
> > > I feel that this can always be added later if we decide it is really
> > > needed, and leave it out for now.
> > > 
> > > So the userspace API would be quite simple.
> > > 
> > > The internal implementation would look like this:
> > > 
> > > struct v4l2_event_payload {
> > > 	u32 payload_size;
> > > 	u32 payload_sequence;
> > > 	void *payload;
> > > };
> > > 
> > > struct v4l2_event_payloads {
> > > 	// lock serializing access to this struct
> > > 	struct mutex lock;
> > > 	// global payload sequence number counter
> > > 	u32 payload_sequence;
> > > 	// size of the payload array
> > > 	unsigned payloads;
> > > 	// index of the oldest payload
> > > 	unsigned first;
> > > 	// number of payloads available
> > > 	unsigned in_use;
> > > 	struct v4l2_event_payload payloads[];
> > > };
> > > 
> > > and a pointer to struct v4l2_event_payloads would be added to struct
> > > v4l2_subscribed_event.
> > > 
> > > It is up to the driver to decide whether there is one v4l2_event_payloads
> > > struct per filehandle or whether there is a global struct shared by any
> > > filehandle subscribed to this event. This will depend on the event and the
> > > size of the payload. Most likely it will be the latter option (a global
> > > queue of payloads).
> > > 
> > > Internal functions would look like this:
> > > 
> > > // Initialize the structs.
> > > void v4l2_event_payloads_init(struct v4l2_event_payloads *p, unsigned
> > > payloads);
> > > // Get the first available payload (the mutex must be locked). If no
> > > // payloads are available then the oldest payload will be reused. A new
> > > // sequence number will be generated as well.
> > > struct v4l2_event_payload *
> > > v4l2_event_payloads_new(struct v4l2_event_payloads *p);
> > > // Find the payload with the given sequence number. The mutex must be
> > > // locked.
> > > struct v4l2_event_payload
> > > *v4l2_event_payloads_find(struct v4l2_event_payloads *p, unsigned seqnr);
> > > 
> > > So when a new payload arrives the driver would take the mutex, call
> > > v4l2_event_payloads_new(), set payload_size and fill the payload data,
> > > remember the payload_size and payload_sequence values, release the mutex
> > > and queue the event with the remembered size and sequence values. Setting
> > > up the payload part cannot be done from interrupt context.
> > > 
> > > When calling DQEVENT_PAYLOAD the core will use the pointer to struct
> > > v4l2_event_payloads from struct v4l2_subscribed_event, take the mutex,
> > > find the payload, copy it to userspace and release the mutex.
> 
> Do we want to allow dequeuing payloads out-of-order ? If we store payload in a 
> global location that would be unavoidable, but if we store them in the file 
> handle then we could force in-order dequeuing. It might make sense to forbit 
> out-of-order dequeuing in the API to leave some flexibility to the in-kernel 
> implementation.

The events must be always delivered to the user in the same order they
arrive, it's important. Speaking of that --- there's a slight issue in
timestamping and queueing them, i.e. timestamps are made before queueing the
event, so it's theoretically possible that event a that precedes event b in
the queue actually is older than b.

One memory copy could also be avoided by starting to reference count payload
buffers. This way delivering the same event to multiple file handles would
avoid almost half of the memory copies --- currently per-handle copy is done
at the time of event queueing and there's another at dequeueing time.

Events that fit to regular 64 bytes will be delivered using that, and the
user-provided pointer will only be used in the case a large event is
delivered to the user. So in order to be able to receive large events, the
user must always provide the pointer even if it's not always used.

-- 
Cheers,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Support for events with a large payload

[Sakari Ailus]

Hi Hans,

On Wed, Jun 19, 2013 at 08:32:35AM +0200, Hans Verkuil wrote:
> On Tue June 18 2013 23:22:33 Laurent Pinchart wrote:
> > Hi Hans and Sakari,
> > 
> > On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> > > Hi Hans,
> > > 
> > > Thanks for the RFC! :-)
> > > 
> > > On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > > > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > > > we would like to pass larger payloads to userspace such as metadata
> > > > associated with a particular video stream.
> > > > 
> > > > A typical example of that would be object detection events.
> > > > 
> > > > This RFC describes one approach for doing this.
> > > > 
> > > > The event framework has the nice property of being able to use from within
> > > > interrupts. Copying large payloads does not fit into that framework, so
> > > > such payloads should be adminstrated separately.
> > > > 
> > > > In addition, I wouldn't allow large payloads to be filled in from
> > > > interrupt context: a worker queue would be much more appropriate.
> > > 
> > > How large really is "large"? 65 bytes? 64 kiB?
> 
> More than 64 bytes, which is what v4l2_event supports today.
> 
> In general events have no or small payloads. So for the majority 64 bytes
> is enough.
> 
> > > 
> > > The newer CPUs tend to be faster and faster and the same applies to memory.
> > > I guess threaded interrupt handlers are still nice. But using a mutex in
> > > order to serialise access to the struct will force drivers to use threaded
> > > interrupt handlers should they want to generate large events.
> 
> But there are still lots of embedded systems out there that are not fast. Or
> that run at reduced frequency to preserve power, etc., etc.
> 
> Interrupts should be fast, and 64 bytes seems a reasonable limit to me.
> 
> > > > Note that the event API is only useful for relatively low-bandwidth data
> > > > since the data is always copied. When dealing with high-bandwidth data the
> > > > data should either be a separate plane or become a special stream I/O
> > > > buffer type.
> > > > 
> > > > The userspace API enhancements in order to achieve this would be the
> > > > following:
> > > > 
> > > > - Any event that has a large payload would specify a payload_sequence
> > > >   counter and a payload size value (in bytes).
> > > > 
> > > > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> > > >   type, the payload_sequence counter and a pointer to a buffer to the
> > > >   kernel, and the payload is returned, or an error is returned if the
> > > >   payload data is no longer available.
> > > 
> > > Do you think we should have a separate IOCTL for this? The downside is that
> > > to dequeue events, the application would need to try both in the worst case
> > > just to obtain an event.
> > > 
> > > I think it'd be nicer to be able to fit that into the same IOCTL. There are
> > > plenty of reserved fields and, actually, the event data as well: we could
> > > consider the large-payload event a meta-event which would contain the
> > > required information to pass the event data to the user space. The type of
> > > such an event could be V4L2_EVENT_LARGE, for instance.
> > 
> > The problem is that userspace doesn't know in advance how much memory it would 
> > need to allocate to store the event payload. Using two ioctls allow retrieving 
> > the size first, and then the payload. We could work around the problem by 
> > passing the maximum size to userspace and forcing preallocation of large 
> > enough buffers.
> 
> The problem is also that you don't know what event you will get when calling
> DQEVENT. So you would have to figure out the maximum of the payload sizes of all
> subscribed events which is rather awkward.

Is it? The application knows which events it has subscribed, and the maximum
event size is the maximum of that of maximum of different event types.

> > > > Optional enhancements:
> > > > 
> > > > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> > > >   preallocate payload memory, but it might be overkill).
> > > 
> > > Why so? We could use a reserved field as well. The size would be zero if the
> > > maximum would be less than 64.
> 
> I'm undecided here. Implementing support for this in an application is probably
> the best way to discover whether or not it is useful to supply the maximum
> payload size to the user. I suspect that this is actually useful.

I agree. Otherwise the application would have to figure it out through other
ways which are less generic and also more difficult. On the other hand, the
maximum event size could be dependent on unrelated parameters, such as image
size. If the user changes the image size without changing the event
subscription this could be a problem. (Well, there's an obvious solution,
too, if we run into this: provide an event on it, just as on control
parameters. :-))

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Support for events with a large payload

[Hans Verkuil]

On Sun June 23 2013 00:46:57 Sakari Ailus wrote:
> Hi Hans,
> 
> On Wed, Jun 19, 2013 at 08:32:35AM +0200, Hans Verkuil wrote:
> > On Tue June 18 2013 23:22:33 Laurent Pinchart wrote:
> > > Hi Hans and Sakari,
> > > 
> > > On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> > > > Hi Hans,
> > > > 
> > > > Thanks for the RFC! :-)
> > > > 
> > > > On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > > > > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > > > > we would like to pass larger payloads to userspace such as metadata
> > > > > associated with a particular video stream.
> > > > > 
> > > > > A typical example of that would be object detection events.
> > > > > 
> > > > > This RFC describes one approach for doing this.
> > > > > 
> > > > > The event framework has the nice property of being able to use from within
> > > > > interrupts. Copying large payloads does not fit into that framework, so
> > > > > such payloads should be adminstrated separately.
> > > > > 
> > > > > In addition, I wouldn't allow large payloads to be filled in from
> > > > > interrupt context: a worker queue would be much more appropriate.
> > > > 
> > > > How large really is "large"? 65 bytes? 64 kiB?
> > 
> > More than 64 bytes, which is what v4l2_event supports today.
> > 
> > In general events have no or small payloads. So for the majority 64 bytes
> > is enough.
> > 
> > > > 
> > > > The newer CPUs tend to be faster and faster and the same applies to memory.
> > > > I guess threaded interrupt handlers are still nice. But using a mutex in
> > > > order to serialise access to the struct will force drivers to use threaded
> > > > interrupt handlers should they want to generate large events.
> > 
> > But there are still lots of embedded systems out there that are not fast. Or
> > that run at reduced frequency to preserve power, etc., etc.
> > 
> > Interrupts should be fast, and 64 bytes seems a reasonable limit to me.
> > 
> > > > > Note that the event API is only useful for relatively low-bandwidth data
> > > > > since the data is always copied. When dealing with high-bandwidth data the
> > > > > data should either be a separate plane or become a special stream I/O
> > > > > buffer type.
> > > > > 
> > > > > The userspace API enhancements in order to achieve this would be the
> > > > > following:
> > > > > 
> > > > > - Any event that has a large payload would specify a payload_sequence
> > > > >   counter and a payload size value (in bytes).
> > > > > 
> > > > > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> > > > >   type, the payload_sequence counter and a pointer to a buffer to the
> > > > >   kernel, and the payload is returned, or an error is returned if the
> > > > >   payload data is no longer available.
> > > > 
> > > > Do you think we should have a separate IOCTL for this? The downside is that
> > > > to dequeue events, the application would need to try both in the worst case
> > > > just to obtain an event.
> > > > 
> > > > I think it'd be nicer to be able to fit that into the same IOCTL. There are
> > > > plenty of reserved fields and, actually, the event data as well: we could
> > > > consider the large-payload event a meta-event which would contain the
> > > > required information to pass the event data to the user space. The type of
> > > > such an event could be V4L2_EVENT_LARGE, for instance.
> > > 
> > > The problem is that userspace doesn't know in advance how much memory it would 
> > > need to allocate to store the event payload. Using two ioctls allow retrieving 
> > > the size first, and then the payload. We could work around the problem by 
> > > passing the maximum size to userspace and forcing preallocation of large 
> > > enough buffers.
> > 
> > The problem is also that you don't know what event you will get when calling
> > DQEVENT. So you would have to figure out the maximum of the payload sizes of all
> > subscribed events which is rather awkward.
> 
> Is it? The application knows which events it has subscribed, and the maximum
> event size is the maximum of that of maximum of different event types.
> 
> > > > > Optional enhancements:
> > > > > 
> > > > > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> > > > >   preallocate payload memory, but it might be overkill).
> > > > 
> > > > Why so? We could use a reserved field as well. The size would be zero if the
> > > > maximum would be less than 64.
> > 
> > I'm undecided here. Implementing support for this in an application is probably
> > the best way to discover whether or not it is useful to supply the maximum
> > payload size to the user. I suspect that this is actually useful.
> 
> I agree. Otherwise the application would have to figure it out through other
> ways which are less generic and also more difficult. On the other hand, the
> maximum event size could be dependent on unrelated parameters, such as image
> size. If the user changes the image size without changing the event
> subscription this could be a problem. (Well, there's an obvious solution,
> too, if we run into this: provide an event on it, just as on control
> parameters. :-))

A meta-event. You're evil :-)

I do think that in practice there is always an upper worst case bound. Although
it a probably a good idea to allow for a case where no such bound can be given.

Regards,

	Hans

Re: [RFC] Support for events with a large payload

[Hans Verkuil]

On Sat June 22 2013 22:58:01 Sakari Ailus wrote:
> Hi Laurent,
> 
> On Tue, Jun 18, 2013 at 11:22:33PM +0200, Laurent Pinchart wrote:
> > On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
> > > On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
> > > > Currently the event API allows for a payload of up to 64 bytes. Sometimes
> > > > we would like to pass larger payloads to userspace such as metadata
> > > > associated with a particular video stream.
> > > > 
> > > > A typical example of that would be object detection events.
> > > > 
> > > > This RFC describes one approach for doing this.
> > > > 
> > > > The event framework has the nice property of being able to use from within
> > > > interrupts. Copying large payloads does not fit into that framework, so
> > > > such payloads should be adminstrated separately.
> > > > 
> > > > In addition, I wouldn't allow large payloads to be filled in from
> > > > interrupt context: a worker queue would be much more appropriate.
> > > 
> > > How large really is "large"? 65 bytes? 64 kiB?
> > > 
> > > The newer CPUs tend to be faster and faster and the same applies to memory.
> > > I guess threaded interrupt handlers are still nice. But using a mutex in
> > > order to serialise access to the struct will force drivers to use threaded
> > > interrupt handlers should they want to generate large events.
> > > 
> > > > Note that the event API is only useful for relatively low-bandwidth data
> > > > since the data is always copied. When dealing with high-bandwidth data the
> > > > data should either be a separate plane or become a special stream I/O
> > > > buffer type.
> > > > 
> > > > The userspace API enhancements in order to achieve this would be the
> > > > following:
> > > > 
> > > > - Any event that has a large payload would specify a payload_sequence
> > > >   counter and a payload size value (in bytes).
> > > > 
> > > > - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
> > > >   type, the payload_sequence counter and a pointer to a buffer to the
> > > >   kernel, and the payload is returned, or an error is returned if the
> > > >   payload data is no longer available.
> > > 
> > > Do you think we should have a separate IOCTL for this? The downside is that
> > > to dequeue events, the application would need to try both in the worst case
> > > just to obtain an event.
> > > 
> > > I think it'd be nicer to be able to fit that into the same IOCTL. There are
> > > plenty of reserved fields and, actually, the event data as well: we could
> > > consider the large-payload event a meta-event which would contain the
> > > required information to pass the event data to the user space. The type of
> > > such an event could be V4L2_EVENT_LARGE, for instance.
> > 
> > The problem is that userspace doesn't know in advance how much memory it would 
> > need to allocate to store the event payload. Using two ioctls allow retrieving 
> > the size first, and then the payload. We could work around the problem by 
> > passing the maximum size to userspace and forcing preallocation of large 
> > enough buffers.
> 
> That would also force using double the number of IOCTLs to do essentially
> the same. I don't like the idea, even if IOCTLs are relatively cheap.
> Maximun size should be defined, at least per event type, so that the user
> knows maximum what to expect.
> 
> > > > Optional enhancements:
> > > > 
> > > > - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
> > > >   preallocate payload memory, but it might be overkill).
> > > 
> > > Why so? We could use a reserved field as well. The size would be zero if the
> > > maximum would be less than 64.
> > > 
> > > > - Add functionality to VIDIOC_SUBSCRIBE_EVENT to define the number of
> > > >   events in the event queue for the filehandle. If the payload is large,
> > > >   you may want to limit the number of allocated payload buffers. For
> > > >   example: when dealing with metadata associated with frame you might want
> > > >   to limit the number of payload buffers to the number of allocated
> > > >   frames.
> > > 
> > > Are we talking now about high level metadata which is typically obtained by
> > > the driver by other means than hardware writing it into a memory buffer?
> > > 
> > > > I feel that this can always be added later if we decide it is really
> > > > needed, and leave it out for now.
> > > > 
> > > > So the userspace API would be quite simple.
> > > > 
> > > > The internal implementation would look like this:
> > > > 
> > > > struct v4l2_event_payload {
> > > > 	u32 payload_size;
> > > > 	u32 payload_sequence;
> > > > 	void *payload;
> > > > };
> > > > 
> > > > struct v4l2_event_payloads {
> > > > 	// lock serializing access to this struct
> > > > 	struct mutex lock;
> > > > 	// global payload sequence number counter
> > > > 	u32 payload_sequence;
> > > > 	// size of the payload array
> > > > 	unsigned payloads;
> > > > 	// index of the oldest payload
> > > > 	unsigned first;
> > > > 	// number of payloads available
> > > > 	unsigned in_use;
> > > > 	struct v4l2_event_payload payloads[];
> > > > };
> > > > 
> > > > and a pointer to struct v4l2_event_payloads would be added to struct
> > > > v4l2_subscribed_event.
> > > > 
> > > > It is up to the driver to decide whether there is one v4l2_event_payloads
> > > > struct per filehandle or whether there is a global struct shared by any
> > > > filehandle subscribed to this event. This will depend on the event and the
> > > > size of the payload. Most likely it will be the latter option (a global
> > > > queue of payloads).
> > > > 
> > > > Internal functions would look like this:
> > > > 
> > > > // Initialize the structs.
> > > > void v4l2_event_payloads_init(struct v4l2_event_payloads *p, unsigned
> > > > payloads);
> > > > // Get the first available payload (the mutex must be locked). If no
> > > > // payloads are available then the oldest payload will be reused. A new
> > > > // sequence number will be generated as well.
> > > > struct v4l2_event_payload *
> > > > v4l2_event_payloads_new(struct v4l2_event_payloads *p);
> > > > // Find the payload with the given sequence number. The mutex must be
> > > > // locked.
> > > > struct v4l2_event_payload
> > > > *v4l2_event_payloads_find(struct v4l2_event_payloads *p, unsigned seqnr);
> > > > 
> > > > So when a new payload arrives the driver would take the mutex, call
> > > > v4l2_event_payloads_new(), set payload_size and fill the payload data,
> > > > remember the payload_size and payload_sequence values, release the mutex
> > > > and queue the event with the remembered size and sequence values. Setting
> > > > up the payload part cannot be done from interrupt context.
> > > > 
> > > > When calling DQEVENT_PAYLOAD the core will use the pointer to struct
> > > > v4l2_event_payloads from struct v4l2_subscribed_event, take the mutex,
> > > > find the payload, copy it to userspace and release the mutex.
> > 
> > Do we want to allow dequeuing payloads out-of-order ? If we store payload in a 
> > global location that would be unavoidable, but if we store them in the file 
> > handle then we could force in-order dequeuing. It might make sense to forbit 
> > out-of-order dequeuing in the API to leave some flexibility to the in-kernel 
> > implementation.
> 
> The events must be always delivered to the user in the same order they
> arrive, it's important. Speaking of that --- there's a slight issue in
> timestamping and queueing them, i.e. timestamps are made before queueing the
> event, so it's theoretically possible that event a that precedes event b in
> the queue actually is older than b.
> 
> One memory copy could also be avoided by starting to reference count payload
> buffers. This way delivering the same event to multiple file handles would
> avoid almost half of the memory copies --- currently per-handle copy is done
> at the time of event queueing and there's another at dequeueing time.

I suspect that this would complicate the code enormously. One advantage of the
code today is that it is quite simple. Also, referencing payloads will not work
in the case where two events have to be merged (as can happen with control
events). You should also realize that having multiple file handles subscribed
to the same event is actually quite rare.

This seems to be a case of premature optimization.

Most likely the additional logic trying to take care of the refcounting is
slower than just doing a memcpy.

> Events that fit to regular 64 bytes will be delivered using that, and the
> user-provided pointer will only be used in the case a large event is
> delivered to the user. So in order to be able to receive large events, the
> user must always provide the pointer even if it's not always used.

This is an option. The easiest approach would be to extend v4l2_kevent with
a pointer to a struct v4l2_event_payload, which would be refcounted (it's
basically a struct kref + size + payload). Since this would have to be allocated
you can't use this in interrupt context.

Since the payloads are larger I am less concerned about speed. There is one
problem, though: if you dequeue the event and the buffer that should receive
the payload is too small, then you have lost that payload. You can't allocate
a new, larger, buffer and retry. So this approach can only work if you really
know the maximum payload size.

The advantage is also that you won't lose payloads.

You are starting to convince me :-) Just don't change the current implementation
for small payloads, that's one that really works very well.

Regards,

	Hans

Re: [RFC] Support for events with a large payload

[Sylwester Nawrocki]

Hi all,

On 06/24/2013 02:57 PM, Hans Verkuil wrote:
> On Sun June 23 2013 00:46:57 Sakari Ailus wrote:
>> On Wed, Jun 19, 2013 at 08:32:35AM +0200, Hans Verkuil wrote:
>>> On Tue June 18 2013 23:22:33 Laurent Pinchart wrote:
>>>> On Friday 07 June 2013 00:38:04 Sakari Ailus wrote:
>>>>> On Mon, May 13, 2013 at 02:14:43PM +0200, Hans Verkuil wrote:
>>>>>> Currently the event API allows for a payload of up to 64 bytes. Sometimes
>>>>>> we would like to pass larger payloads to userspace such as metadata
>>>>>> associated with a particular video stream.
>>>>>>
>>>>>> A typical example of that would be object detection events.
>>>>>>
>>>>>> This RFC describes one approach for doing this.
>>>>>>
>>>>>> The event framework has the nice property of being able to use from within
>>>>>> interrupts. Copying large payloads does not fit into that framework, so
>>>>>> such payloads should be adminstrated separately.

But does it really need to be reflected in user space that much, i.e. by
introducing a separate ioctl for de-queueing the payload ?

>>>>>> In addition, I wouldn't allow large payloads to be filled in from
>>>>>> interrupt context: a worker queue would be much more appropriate.
>>>>>
>>>>> How large really is "large"? 65 bytes? 64 kiB?
>>>
>>> More than 64 bytes, which is what v4l2_event supports today.

This seems some artificial distinction which is likely inaccurate across
various systems. For many of them there is not much difference whether
we copy 64 B or 1 kB. Of course it have to be dealt in the kernel from
what context events of what size are passed from a device driver to the
v4l2-core. But once the event object has been queued in the core it
is not really relevant from what context it has been queued, is it ?

>>> In general events have no or small payloads. So for the majority 64 bytes
>>> is enough.
>>>
>>>>>
>>>>> The newer CPUs tend to be faster and faster and the same applies to memory.
>>>>> I guess threaded interrupt handlers are still nice. But using a mutex in
>>>>> order to serialise access to the struct will force drivers to use threaded
>>>>> interrupt handlers should they want to generate large events.
>>>
>>> But there are still lots of embedded systems out there that are not fast. Or
>>> that run at reduced frequency to preserve power, etc., etc.

Again, IMHO it's not the API design that should make the border, rather
a driver for a particular device/system, provided with a non-blocking API
for small/fast events and with a blocking one for the larger payload events.

>>> Interrupts should be fast, and 64 bytes seems a reasonable limit to me.

>>>>>> Note that the event API is only useful for relatively low-bandwidth data
>>>>>> since the data is always copied. When dealing with high-bandwidth data the
>>>>>> data should either be a separate plane or become a special stream I/O
>>>>>> buffer type.
>>>>>>
>>>>>> The userspace API enhancements in order to achieve this would be the
>>>>>> following:
>>>>>>
>>>>>> - Any event that has a large payload would specify a payload_sequence
>>>>>>    counter and a payload size value (in bytes).
>>>>>>
>>>>>> - A new VIDIOC_DQEVENT_PAYLOAD ioctl would be added which passes the event
>>>>>>    type, the payload_sequence counter and a pointer to a buffer to the
>>>>>>    kernel, and the payload is returned, or an error is returned if the
>>>>>>    payload data is no longer available.
>>>>>
>>>>> Do you think we should have a separate IOCTL for this? The downside is that
>>>>> to dequeue events, the application would need to try both in the worst case
>>>>> just to obtain an event.

Yes, I'm also not convinced doubling the number of ioctls and adding all
complexity related with matching an event and the payload is a good 
approach.
The system has more data to deal with and we're complicating the API and 
adding
more overhead for this case ?

>>>>> I think it'd be nicer to be able to fit that into the same IOCTL. There are
>>>>> plenty of reserved fields and, actually, the event data as well: we could
>>>>> consider the large-payload event a meta-event which would contain the
>>>>> required information to pass the event data to the user space. The type of
>>>>> such an event could be V4L2_EVENT_LARGE, for instance.
>>>>
>>>> The problem is that userspace doesn't know in advance how much memory it would
>>>> need to allocate to store the event payload. Using two ioctls allow retrieving
>>>> the size first, and then the payload. We could work around the problem by
>>>> passing the maximum size to userspace and forcing preallocation of large
>>>> enough buffers.
>>>
>>> The problem is also that you don't know what event you will get when calling
>>> DQEVENT. So you would have to figure out the maximum of the payload sizes of all
>>> subscribed events which is rather awkward.
>>
>> Is it? The application knows which events it has subscribed, and the maximum
>> event size is the maximum of that of maximum of different event types.

I side Sakari here. Since we cannot pass the information about a payload 
buffer
size to the kernel in same ioctl it looks acceptable to me to have user 
space
provided with an API to retrieve the maximum event's payload size, 
preallocating
the buffers and using them when de-queueing any event.

Also, couldn't an application subscribe to "large payload" event(s) on 
one file
handle and to any other events, like currently supported, on a separate 
file
handle ?

>>>>>> Optional enhancements:
>>>>>>
>>>>>> - Have VIDIOC_SUBSCRIBE_EVENT return the maximum payload size (lets apps
>>>>>>    preallocate payload memory, but it might be overkill).
>>>>>
>>>>> Why so? We could use a reserved field as well. The size would be zero if the
>>>>> maximum would be less than 64.
>>>
>>> I'm undecided here. Implementing support for this in an application is probably
>>> the best way to discover whether or not it is useful to supply the maximum
>>> payload size to the user. I suspect that this is actually useful.
>>
>> I agree. Otherwise the application would have to figure it out through other
>> ways which are less generic and also more difficult. On the other hand, the
>> maximum event size could be dependent on unrelated parameters, such as image
>> size. If the user changes the image size without changing the event
>> subscription this could be a problem. (Well, there's an obvious solution,
>> too, if we run into this: provide an event on it, just as on control
>> parameters. :-))

Oops, that might be indeed a bit of a problem. But first someone needs 
to come
up with such a flexible payload event... Just poking to see if there is 
anyone
already waiting with such one out there... :)

> A meta-event. You're evil :-)
>
> I do think that in practice there is always an upper worst case bound. Although
> it a probably a good idea to allow for a case where no such bound can be given.

Regards,
Sylwester

Re: [RFC] Support for events with a large payload

[Sakari Ailus]

Hi Hans,

On Mon, Jun 24, 2013 at 03:40:14PM +0200, Hans Verkuil wrote:
...
> > Events that fit to regular 64 bytes will be delivered using that, and the
> > user-provided pointer will only be used in the case a large event is
> > delivered to the user. So in order to be able to receive large events, the
> > user must always provide the pointer even if it's not always used.
> 
> This is an option. The easiest approach would be to extend v4l2_kevent with
> a pointer to a struct v4l2_event_payload, which would be refcounted (it's
> basically a struct kref + size + payload). Since this would have to be allocated
> you can't use this in interrupt context.

Well, one can use GFP_NOWAIT. I would allow this. But surely better done
outside interrupt context.

> Since the payloads are larger I am less concerned about speed. There is one
> problem, though: if you dequeue the event and the buffer that should receive
> the payload is too small, then you have lost that payload. You can't allocate
> a new, larger, buffer and retry. So this approach can only work if you really
> know the maximum payload size.
> 
> The advantage is also that you won't lose payloads.
> 
> You are starting to convince me :-) Just don't change the current implementation
> for small payloads, that's one that really works very well.

I meant only large payload buffers above but failed to write it down.
Smaller ones could stay as they are.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Support for events with a large payload

[Sakari Ailus]

On Mon, Jun 24, 2013 at 03:40:14PM +0200, Hans Verkuil wrote:
...
> Since the payloads are larger I am less concerned about speed. There is one
> problem, though: if you dequeue the event and the buffer that should receive
> the payload is too small, then you have lost that payload. You can't allocate
> a new, larger, buffer and retry. So this approach can only work if you really
> know the maximum payload size.
> 
> The advantage is also that you won't lose payloads.

Forgot to answer this one --- I think it's fair to assume the user knows the
maximum size of the payload. What we also could do in such a case is to
return the error (e.g. ENOSPC) and put the required size to the large event
size field. But first someone must come up with a variable size event
without well defined maximum size for this to make much sense.

-- 
Kind regards,

Sakari Ailus
e-mail: sakari.ailus@iki.fi	XMPP: sailus@retiisi.org.uk

Re: [RFC] Support for events with a large payload

[Laurent Pinchart]

On Wednesday 03 July 2013 02:01:59 Sakari Ailus wrote:
> On Mon, Jun 24, 2013 at 03:40:14PM +0200, Hans Verkuil wrote:
> ...
> 
> > Since the payloads are larger I am less concerned about speed. There is
> > one problem, though: if you dequeue the event and the buffer that should
> > receive the payload is too small, then you have lost that payload. You
> > can't allocate a new, larger, buffer and retry. So this approach can only
> > work if you really know the maximum payload size.
> > 
> > The advantage is also that you won't lose payloads.
> 
> Forgot to answer this one --- I think it's fair to assume the user knows the
> maximum size of the payload. What we also could do in such a case is to
> return the error (e.g. ENOSPC) and put the required size to the large event
> size field. But first someone must come up with a variable size event
> without well defined maximum size for this to make much sense.

And while we're discussing use cases, Hans, what are you current use cases for 
>64 bytes event payloads ?

-- 
Regards,

Laurent Pinchart

Re: [RFC] Support for events with a large payload

[Sylwester Nawrocki]

On 07/03/2013 09:34 PM, Laurent Pinchart wrote:
> On Wednesday 03 July 2013 02:01:59 Sakari Ailus wrote:
>> On Mon, Jun 24, 2013 at 03:40:14PM +0200, Hans Verkuil wrote:
>> ...
>>
>>> Since the payloads are larger I am less concerned about speed. There is
>>> one problem, though: if you dequeue the event and the buffer that should
>>> receive the payload is too small, then you have lost that payload. You
>>> can't allocate a new, larger, buffer and retry. So this approach can only
>>> work if you really know the maximum payload size.
>>>
>>> The advantage is also that you won't lose payloads.
>>
>> Forgot to answer this one --- I think it's fair to assume the user knows the
>> maximum size of the payload. What we also could do in such a case is to
>> return the error (e.g. ENOSPC) and put the required size to the large event
>> size field. But first someone must come up with a variable size event
>> without well defined maximum size for this to make much sense.
>
> And while we're discussing use cases, Hans, what are you current use cases for
> 64 bytes event payloads ?

One of the use cases could be face detection events. A face marker would
contain at least 4 rectangle data structures (face, left/right eye, 
mouth,...),
which is itself 64 bytes. Plus Euler angle information, confidence, 
smile/blink
level etc. We could add an object detection specific ioctl(s) (I'm not sure
if such won't be needed anyway), but the event API looks like a good
infrastructure to handle this kind of data.

--
Regards,
Sylwester