* [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).