[RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[Hans Verkuil]

This patch adds an extended version of VIDIOC_DQEVENT that:

1) is Y2038 safe by using a __u64 for the timestamp
2) needs no compat32 conversion code
3) is able to handle control events from 64-bit control types
   by changing the type of the minimum, maximum, step and default_value
   field to __u64

All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
only place where the old event ioctl and structs are used.

Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Please let me know if there are additional requests for such a new ioctl.

Note that I am using number 104 for the ioctl, but perhaps it would be better to
use an unused ioctl number like 1 or 3. There are quite a few holes in the
ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
---
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 9a920f071ff9..969e775b8c25 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -2303,6 +2303,37 @@ struct v4l2_event {
 	__u32				reserved[8];
 };

+struct v4l2_event_ext_ctrl {
+	__u32 changes;
+	__u32 type;
+	union {
+		__s32 value;
+		__s64 value64;
+	};
+	__s64 minimum;
+	__s64 maximum;
+	__s64 step;
+	__s64 default_value;
+	__u32 flags;
+};
+
+struct v4l2_ext_event {
+	__u32				type;
+	__u32				id;
+	union {
+		struct v4l2_event_vsync		vsync;
+		struct v4l2_event_ext_ctrl	ctrl;
+		struct v4l2_event_frame_sync	frame_sync;
+		struct v4l2_event_src_change	src_change;
+		struct v4l2_event_motion_det	motion_det;
+		__u8				data[64];
+	} u;
+	__u64				timestamp;
+	__u32				pending;
+	__u32				sequence;
+	__u32				reserved[8];
+};
+
 #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
 #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)

@@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
 #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)

 #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
+#define	VIDIOC_DQEXTEVENT	 _IOR('V', 104, struct v4l2_ext_event)

 /* Reminder: when adding new ioctls please add support for them to
    drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */

Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[Sakari Ailus]

Hi Hans,

Thanks for the patch.

On Sat, Jan 26, 2019 at 12:06:19PM +0100, Hans Verkuil wrote:
> This patch adds an extended version of VIDIOC_DQEVENT that:
> 
> 1) is Y2038 safe by using a __u64 for the timestamp
> 2) needs no compat32 conversion code
> 3) is able to handle control events from 64-bit control types
>    by changing the type of the minimum, maximum, step and default_value
>    field to __u64
> 
> All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
> only place where the old event ioctl and structs are used.
> 
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Please let me know if there are additional requests for such a new ioctl.
> 
> Note that I am using number 104 for the ioctl, but perhaps it would be better to
> use an unused ioctl number like 1 or 3. There are quite a few holes in the
> ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
> ---
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 9a920f071ff9..969e775b8c25 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -2303,6 +2303,37 @@ struct v4l2_event {
>  	__u32				reserved[8];
>  };
> 
> +struct v4l2_event_ext_ctrl {
> +	__u32 changes;
> +	__u32 type;
> +	union {
> +		__s32 value;
> +		__s64 value64;
> +	};
> +	__s64 minimum;
> +	__s64 maximum;
> +	__s64 step;
> +	__s64 default_value;
> +	__u32 flags;
> +};
> +
> +struct v4l2_ext_event {
> +	__u32				type;
> +	__u32				id;
> +	union {
> +		struct v4l2_event_vsync		vsync;
> +		struct v4l2_event_ext_ctrl	ctrl;
> +		struct v4l2_event_frame_sync	frame_sync;
> +		struct v4l2_event_src_change	src_change;
> +		struct v4l2_event_motion_det	motion_det;
> +		__u8				data[64];
> +	} u;

If I'd change something in the event IOCTL, I'd probably put the reserved
fields here. That'd allow later taking some for the use of the event data
if needed.

I might also increase the size of the event data. 64 bytes is not that
much. But you indeed end up copying it around all the time... So it's a
trade-off.

> +	__u64				timestamp;
> +	__u32				pending;
> +	__u32				sequence;
> +	__u32				reserved[8];
> +};
> +
>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
> 
> @@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
> 
>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> +#define	VIDIOC_DQEXTEVENT	 _IOR('V', 104, struct v4l2_ext_event)

How do you plan to name the new buffer handling IOCTLs? I.e. with or
without underscores around "EXT"?

> 
>  /* Reminder: when adding new ioctls please add support for them to
>     drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */

-- 
Kind regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[Hans Verkuil]

On 1/28/19 10:21 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> Thanks for the patch.
> 
> On Sat, Jan 26, 2019 at 12:06:19PM +0100, Hans Verkuil wrote:
>> This patch adds an extended version of VIDIOC_DQEVENT that:
>>
>> 1) is Y2038 safe by using a __u64 for the timestamp
>> 2) needs no compat32 conversion code
>> 3) is able to handle control events from 64-bit control types
>>    by changing the type of the minimum, maximum, step and default_value
>>    field to __u64
>>
>> All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
>> only place where the old event ioctl and structs are used.
>>
>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>> ---
>> Please let me know if there are additional requests for such a new ioctl.
>>
>> Note that I am using number 104 for the ioctl, but perhaps it would be better to
>> use an unused ioctl number like 1 or 3. There are quite a few holes in the
>> ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
>> ---
>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>> index 9a920f071ff9..969e775b8c25 100644
>> --- a/include/uapi/linux/videodev2.h
>> +++ b/include/uapi/linux/videodev2.h
>> @@ -2303,6 +2303,37 @@ struct v4l2_event {
>>  	__u32				reserved[8];
>>  };
>>
>> +struct v4l2_event_ext_ctrl {
>> +	__u32 changes;
>> +	__u32 type;
>> +	union {
>> +		__s32 value;
>> +		__s64 value64;
>> +	};
>> +	__s64 minimum;
>> +	__s64 maximum;
>> +	__s64 step;
>> +	__s64 default_value;
>> +	__u32 flags;
>> +};
>> +
>> +struct v4l2_ext_event {
>> +	__u32				type;
>> +	__u32				id;
>> +	union {
>> +		struct v4l2_event_vsync		vsync;
>> +		struct v4l2_event_ext_ctrl	ctrl;
>> +		struct v4l2_event_frame_sync	frame_sync;
>> +		struct v4l2_event_src_change	src_change;
>> +		struct v4l2_event_motion_det	motion_det;
>> +		__u8				data[64];
>> +	} u;
> 
> If I'd change something in the event IOCTL, I'd probably put the reserved
> fields here. That'd allow later taking some for the use of the event data
> if needed.

Good point, I'll do that.

> I might also increase the size of the event data. 64 bytes is not that
> much. But you indeed end up copying it around all the time... So it's a
> trade-off.

I decided to leave this alone. I think by putting the reserved array after
the union (nice idea) we allow for such future extension should it be
necessary.

> 
>> +	__u64				timestamp;
>> +	__u32				pending;
>> +	__u32				sequence;
>> +	__u32				reserved[8];
>> +};
>> +
>>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
>>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
>>
>> @@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
>>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>
>>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
>> +#define	VIDIOC_DQEXTEVENT	 _IOR('V', 104, struct v4l2_ext_event)
> 
> How do you plan to name the new buffer handling IOCTLs? I.e. with or
> without underscores around "EXT"?

It's a good question. In my old patch I named them VIDIOC_EXT_QBUF etc. See:
https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3

So I think I should probably rename this to VIDIOC_EXT_DQEVENT.

Alternatively, perhaps we should ditch the _ext_ usage and instead use a
version suffix: VIDIOC_DQEVENT_V2.

The problem with EXT is that if you want to make a newer version of such a
control, you can't just name it EXT_EXT, that would be silly. But naming it
_V3 would be fine.

Frankly, the extended control ioctls have that problem, also due to awful
64 bit alignment issues. It would be really nice to have _V3 versions of
those ioctls that do not require compat32 code.

Feedback on this would be very welcome!

Regards,

	Hans

> 
>>
>>  /* Reminder: when adding new ioctls please add support for them to
>>     drivers/media/v4l2-core/v4l2-compat-ioctl32.c as well! */
> 

Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[Sakari Ailus]

Hi Hans,

On Mon, Jan 28, 2019 at 10:52:40AM +0100, Hans Verkuil wrote:
> On 1/28/19 10:21 AM, Sakari Ailus wrote:
> > Hi Hans,
> > 
> > Thanks for the patch.
> > 
> > On Sat, Jan 26, 2019 at 12:06:19PM +0100, Hans Verkuil wrote:
> >> This patch adds an extended version of VIDIOC_DQEVENT that:
> >>
> >> 1) is Y2038 safe by using a __u64 for the timestamp
> >> 2) needs no compat32 conversion code
> >> 3) is able to handle control events from 64-bit control types
> >>    by changing the type of the minimum, maximum, step and default_value
> >>    field to __u64
> >>
> >> All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
> >> only place where the old event ioctl and structs are used.
> >>
> >> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> >> ---
> >> Please let me know if there are additional requests for such a new ioctl.
> >>
> >> Note that I am using number 104 for the ioctl, but perhaps it would be better to
> >> use an unused ioctl number like 1 or 3. There are quite a few holes in the
> >> ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
> >> ---
> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> >> index 9a920f071ff9..969e775b8c25 100644
> >> --- a/include/uapi/linux/videodev2.h
> >> +++ b/include/uapi/linux/videodev2.h
> >> @@ -2303,6 +2303,37 @@ struct v4l2_event {
> >>  	__u32				reserved[8];
> >>  };
> >>
> >> +struct v4l2_event_ext_ctrl {
> >> +	__u32 changes;
> >> +	__u32 type;
> >> +	union {
> >> +		__s32 value;
> >> +		__s64 value64;
> >> +	};
> >> +	__s64 minimum;
> >> +	__s64 maximum;
> >> +	__s64 step;
> >> +	__s64 default_value;
> >> +	__u32 flags;
> >> +};
> >> +
> >> +struct v4l2_ext_event {
> >> +	__u32				type;
> >> +	__u32				id;
> >> +	union {
> >> +		struct v4l2_event_vsync		vsync;
> >> +		struct v4l2_event_ext_ctrl	ctrl;
> >> +		struct v4l2_event_frame_sync	frame_sync;
> >> +		struct v4l2_event_src_change	src_change;
> >> +		struct v4l2_event_motion_det	motion_det;
> >> +		__u8				data[64];
> >> +	} u;
> > 
> > If I'd change something in the event IOCTL, I'd probably put the reserved
> > fields here. That'd allow later taking some for the use of the event data
> > if needed.
> 
> Good point, I'll do that.
> 
> > I might also increase the size of the event data. 64 bytes is not that
> > much. But you indeed end up copying it around all the time... So it's a
> > trade-off.
> 
> I decided to leave this alone. I think by putting the reserved array after
> the union (nice idea) we allow for such future extension should it be
> necessary.

Agreed.

> 
> > 
> >> +	__u64				timestamp;
> >> +	__u32				pending;
> >> +	__u32				sequence;
> >> +	__u32				reserved[8];
> >> +};
> >> +
> >>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
> >>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
> >>
> >> @@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
> >>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
> >>
> >>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
> >> +#define	VIDIOC_DQEXTEVENT	 _IOR('V', 104, struct v4l2_ext_event)
> > 
> > How do you plan to name the new buffer handling IOCTLs? I.e. with or
> > without underscores around "EXT"?
> 
> It's a good question. In my old patch I named them VIDIOC_EXT_QBUF etc. See:
> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3
> 
> So I think I should probably rename this to VIDIOC_EXT_DQEVENT.
> 
> Alternatively, perhaps we should ditch the _ext_ usage and instead use a
> version suffix: VIDIOC_DQEVENT_V2.
> 
> The problem with EXT is that if you want to make a newer version of such a
> control, you can't just name it EXT_EXT, that would be silly. But naming it

You could use "EXT2" as well, I think that'd be fine, too. Think of ext4fs,
for instance. :-)

> _V3 would be fine.

We have such a pattern on MC. But I'd still favour "EXT" since we already
use that in V4L2.

> 
> Frankly, the extended control ioctls have that problem, also due to awful
> 64 bit alignment issues. It would be really nice to have _V3 versions of
> those ioctls that do not require compat32 code.

VIDIOC_G_EXT_CTRLS_V3 or VIDIOC_G_EXT2_CTRLS?

It'd be nice to hear other opinions, too..

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT

[Hans Verkuil]

On 1/28/19 11:41 AM, Sakari Ailus wrote:
> Hi Hans,
> 
> On Mon, Jan 28, 2019 at 10:52:40AM +0100, Hans Verkuil wrote:
>> On 1/28/19 10:21 AM, Sakari Ailus wrote:
>>> Hi Hans,
>>>
>>> Thanks for the patch.
>>>
>>> On Sat, Jan 26, 2019 at 12:06:19PM +0100, Hans Verkuil wrote:
>>>> This patch adds an extended version of VIDIOC_DQEVENT that:
>>>>
>>>> 1) is Y2038 safe by using a __u64 for the timestamp
>>>> 2) needs no compat32 conversion code
>>>> 3) is able to handle control events from 64-bit control types
>>>>    by changing the type of the minimum, maximum, step and default_value
>>>>    field to __u64
>>>>
>>>> All drivers and frameworks will be using this, and v4l2-ioctl.c would be the
>>>> only place where the old event ioctl and structs are used.
>>>>
>>>> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>>> ---
>>>> Please let me know if there are additional requests for such a new ioctl.
>>>>
>>>> Note that I am using number 104 for the ioctl, but perhaps it would be better to
>>>> use an unused ioctl number like 1 or 3. There are quite a few holes in the
>>>> ioctl numbers. We currently have only 82 ioctls, yet are up to ioctl number 103.
>>>> ---
>>>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
>>>> index 9a920f071ff9..969e775b8c25 100644
>>>> --- a/include/uapi/linux/videodev2.h
>>>> +++ b/include/uapi/linux/videodev2.h
>>>> @@ -2303,6 +2303,37 @@ struct v4l2_event {
>>>>  	__u32				reserved[8];
>>>>  };
>>>>
>>>> +struct v4l2_event_ext_ctrl {
>>>> +	__u32 changes;
>>>> +	__u32 type;
>>>> +	union {
>>>> +		__s32 value;
>>>> +		__s64 value64;
>>>> +	};
>>>> +	__s64 minimum;
>>>> +	__s64 maximum;
>>>> +	__s64 step;
>>>> +	__s64 default_value;
>>>> +	__u32 flags;
>>>> +};
>>>> +
>>>> +struct v4l2_ext_event {
>>>> +	__u32				type;
>>>> +	__u32				id;
>>>> +	union {
>>>> +		struct v4l2_event_vsync		vsync;
>>>> +		struct v4l2_event_ext_ctrl	ctrl;
>>>> +		struct v4l2_event_frame_sync	frame_sync;
>>>> +		struct v4l2_event_src_change	src_change;
>>>> +		struct v4l2_event_motion_det	motion_det;
>>>> +		__u8				data[64];
>>>> +	} u;
>>>
>>> If I'd change something in the event IOCTL, I'd probably put the reserved
>>> fields here. That'd allow later taking some for the use of the event data
>>> if needed.
>>
>> Good point, I'll do that.
>>
>>> I might also increase the size of the event data. 64 bytes is not that
>>> much. But you indeed end up copying it around all the time... So it's a
>>> trade-off.
>>
>> I decided to leave this alone. I think by putting the reserved array after
>> the union (nice idea) we allow for such future extension should it be
>> necessary.
> 
> Agreed.
> 
>>
>>>
>>>> +	__u64				timestamp;
>>>> +	__u32				pending;
>>>> +	__u32				sequence;
>>>> +	__u32				reserved[8];
>>>> +};
>>>> +
>>>>  #define V4L2_EVENT_SUB_FL_SEND_INITIAL		(1 << 0)
>>>>  #define V4L2_EVENT_SUB_FL_ALLOW_FEEDBACK	(1 << 1)
>>>>
>>>> @@ -2475,6 +2506,7 @@ struct v4l2_create_buffers {
>>>>  #define VIDIOC_DBG_G_CHIP_INFO  _IOWR('V', 102, struct v4l2_dbg_chip_info)
>>>>
>>>>  #define VIDIOC_QUERY_EXT_CTRL	_IOWR('V', 103, struct v4l2_query_ext_ctrl)
>>>> +#define	VIDIOC_DQEXTEVENT	 _IOR('V', 104, struct v4l2_ext_event)
>>>
>>> How do you plan to name the new buffer handling IOCTLs? I.e. with or
>>> without underscores around "EXT"?
>>
>> It's a good question. In my old patch I named them VIDIOC_EXT_QBUF etc. See:
>> https://git.linuxtv.org/hverkuil/media_tree.git/commit/?h=v4l2-buffer&id=a95549df06d9900f3559afdbb9da06bd4b22d1f3
>>
>> So I think I should probably rename this to VIDIOC_EXT_DQEVENT.
>>
>> Alternatively, perhaps we should ditch the _ext_ usage and instead use a
>> version suffix: VIDIOC_DQEVENT_V2.
>>
>> The problem with EXT is that if you want to make a newer version of such a
>> control, you can't just name it EXT_EXT, that would be silly. But naming it
> 
> You could use "EXT2" as well, I think that'd be fine, too. Think of ext4fs,
> for instance. :-)
> 
>> _V3 would be fine.
> 
> We have such a pattern on MC. But I'd still favour "EXT" since we already
> use that in V4L2.
> 
>>
>> Frankly, the extended control ioctls have that problem, also due to awful
>> 64 bit alignment issues. It would be really nice to have _V3 versions of
>> those ioctls that do not require compat32 code.
> 
> VIDIOC_G_EXT_CTRLS_V3 or VIDIOC_G_EXT2_CTRLS?

That might work, I hadn't thought about that. It's probably better than
introducing a new naming scheme.

> 
> It'd be nice to hear other opinions, too..
> 

Indeed.

Regards,

	Hans