* [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT
@ 2019-01-26 11:06 Hans Verkuil
2019-01-28 9:21 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2019-01-26 11:06 UTC (permalink / raw)
To: Linux Media Mailing List; +Cc: Sakari Ailus, Laurent Pinchart
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! */
^ permalink raw reply related [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT
2019-01-26 11:06 [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT Hans Verkuil
@ 2019-01-28 9:21 ` Sakari Ailus
2019-01-28 9:52 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2019-01-28 9:21 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT
2019-01-28 9:21 ` Sakari Ailus
@ 2019-01-28 9:52 ` Hans Verkuil
2019-01-28 10:41 ` Sakari Ailus
0 siblings, 1 reply; 5+ messages in thread
From: Hans Verkuil @ 2019-01-28 9:52 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Linux Media Mailing List, Laurent Pinchart
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! */
>
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT
2019-01-28 9:52 ` Hans Verkuil
@ 2019-01-28 10:41 ` Sakari Ailus
2019-01-28 10:54 ` Hans Verkuil
0 siblings, 1 reply; 5+ messages in thread
From: Sakari Ailus @ 2019-01-28 10:41 UTC (permalink / raw)
To: Hans Verkuil; +Cc: Linux Media Mailing List, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 5+ messages in thread
* Re: [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT
2019-01-28 10:41 ` Sakari Ailus
@ 2019-01-28 10:54 ` Hans Verkuil
0 siblings, 0 replies; 5+ messages in thread
From: Hans Verkuil @ 2019-01-28 10:54 UTC (permalink / raw)
To: Sakari Ailus; +Cc: Linux Media Mailing List, Laurent Pinchart
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
^ permalink raw reply [flat|nested] 5+ messages in thread
end of thread, other threads:[~2019-01-28 10:54 UTC | newest]
Thread overview: 5+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-01-26 11:06 [RFC PATCH] videodev2.h: introduce VIDIOC_DQEXTEVENT Hans Verkuil
2019-01-28 9:21 ` Sakari Ailus
2019-01-28 9:52 ` Hans Verkuil
2019-01-28 10:41 ` Sakari Ailus
2019-01-28 10:54 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).