| Message ID | 1392631070-41868-6-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Em Mon, 17 Feb 2014 10:57:20 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > From: Hans Verkuil <hans.verkuil@cisco.com> > > Add a new struct and ioctl to extend the amount of information you can > get for a control. > > It gives back a unit string, the range is now a s64 type, and the matrix > and element size can be reported through cols/rows/elem_size. > > Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > --- > include/uapi/linux/videodev2.h | 31 +++++++++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 4d7782a..858a6f3 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1272,6 +1272,35 @@ struct v4l2_queryctrl { > __u32 reserved[2]; > }; > > +/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ > +struct v4l2_query_ext_ctrl { > + __u32 id; > + __u32 type; > + char name[32]; > + char unit[32]; > + union { > + __s64 val; > + __u32 reserved[4]; Why to reserve 16 bytes here? for anything bigger than 64 bits, we could use a pointer. Same applies to the other unions. > + } min; > + union { > + __s64 val; > + __u32 reserved[4]; > + } max; > + union { > + __u64 val; > + __u32 reserved[4]; > + } step; > + union { > + __s64 val; > + __u32 reserved[4]; > + } def; Please call it default. It is ok to simplify names inside a driver, but better to not do it at the API. > + __u32 flags; > + __u32 cols; > + __u32 rows; > + __u32 elem_size; The three above seem to be too specific for an array. I would put those on a separate struct and add here an union, like: union { struct v4l2_array arr; __u32 reserved[8]; } > + __u32 reserved[17]; This also seems too much. Why 17? > +}; > + > /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ > struct v4l2_querymenu { > __u32 id; > @@ -1965,6 +1994,8 @@ struct v4l2_create_buffers { > Never use these in applications! */ > #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) > + > /* Reminder: when adding new ioctls please add support for them to > drivers/media/video/v4l2-compat-ioctl32.c as well! */ >
On 03/11/2014 08:42 PM, Mauro Carvalho Chehab wrote: > Em Mon, 17 Feb 2014 10:57:20 +0100 > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > >> From: Hans Verkuil <hans.verkuil@cisco.com> >> >> Add a new struct and ioctl to extend the amount of information you can >> get for a control. >> >> It gives back a unit string, the range is now a s64 type, and the matrix >> and element size can be reported through cols/rows/elem_size. >> >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> >> --- >> include/uapi/linux/videodev2.h | 31 +++++++++++++++++++++++++++++++ >> 1 file changed, 31 insertions(+) >> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 4d7782a..858a6f3 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -1272,6 +1272,35 @@ struct v4l2_queryctrl { >> __u32 reserved[2]; >> }; >> >> +/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ >> +struct v4l2_query_ext_ctrl { >> + __u32 id; >> + __u32 type; >> + char name[32]; >> + char unit[32]; >> + union { >> + __s64 val; >> + __u32 reserved[4]; > > Why to reserve 16 bytes here? for anything bigger than 64 > bits, we could use a pointer. > > Same applies to the other unions. The idea was to allow space for min/max/step/def values for compound types if applicable. But that may have been overengineering. > >> + } min; >> + union { >> + __s64 val; >> + __u32 reserved[4]; >> + } max; >> + union { >> + __u64 val; >> + __u32 reserved[4]; >> + } step; >> + union { >> + __s64 val; >> + __u32 reserved[4]; >> + } def; > > Please call it default. It is ok to simplify names inside a driver, > but better to not do it at the API. default_value, then. 'default' is a keyword. I should probably rename min and max to minimum and maximum to stay in sync with v4l2_queryctrl. > >> + __u32 flags; > >> + __u32 cols; >> + __u32 rows; >> + __u32 elem_size; > > The three above seem to be too specific for an array. > > I would put those on a separate struct and add here an union, > like: > > union { > struct v4l2_array arr; > __u32 reserved[8]; > } I have to sleep on this. I'm not sure this helps in any way. > >> + __u32 reserved[17]; > > This also seems too much. Why 17? It aligned the struct up to some nice number. Also, experience tells me that whenever I limit the number of reserved fields it bites me later. > >> +}; > >> + >> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ >> struct v4l2_querymenu { >> __u32 id; >> @@ -1965,6 +1994,8 @@ struct v4l2_create_buffers { >> Never use these in applications! */ >> #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) >> + >> /* Reminder: when adding new ioctls please add support for them to >> drivers/media/video/v4l2-compat-ioctl32.c as well! */ >> > > Regards, Hans -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Em Tue, 11 Mar 2014 21:29:29 +0100 Hans Verkuil <hverkuil@xs4all.nl> escreveu: > On 03/11/2014 08:42 PM, Mauro Carvalho Chehab wrote: > > Em Mon, 17 Feb 2014 10:57:20 +0100 > > Hans Verkuil <hverkuil@xs4all.nl> escreveu: > > > >> From: Hans Verkuil <hans.verkuil@cisco.com> > >> > >> Add a new struct and ioctl to extend the amount of information you can > >> get for a control. > >> > >> It gives back a unit string, the range is now a s64 type, and the matrix > >> and element size can be reported through cols/rows/elem_size. > >> > >> Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com> > >> --- > >> include/uapi/linux/videodev2.h | 31 +++++++++++++++++++++++++++++++ > >> 1 file changed, 31 insertions(+) > >> > >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > >> index 4d7782a..858a6f3 100644 > >> --- a/include/uapi/linux/videodev2.h > >> +++ b/include/uapi/linux/videodev2.h > >> @@ -1272,6 +1272,35 @@ struct v4l2_queryctrl { > >> __u32 reserved[2]; > >> }; > >> > >> +/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ > >> +struct v4l2_query_ext_ctrl { > >> + __u32 id; > >> + __u32 type; > >> + char name[32]; > >> + char unit[32]; > >> + union { > >> + __s64 val; > >> + __u32 reserved[4]; > > > > Why to reserve 16 bytes here? for anything bigger than 64 > > bits, we could use a pointer. > > > > Same applies to the other unions. > > The idea was to allow space for min/max/step/def values for compound types > if applicable. But that may have been overengineering. It seems overengineering for me ;) > > > > >> + } min; > >> + union { > >> + __s64 val; > >> + __u32 reserved[4]; > >> + } max; > >> + union { > >> + __u64 val; > >> + __u32 reserved[4]; > >> + } step; > >> + union { > >> + __s64 val; > >> + __u32 reserved[4]; > >> + } def; > > > > Please call it default. It is ok to simplify names inside a driver, > > but better to not do it at the API. > > default_value, then. 'default' is a keyword. I should probably rename min and max > to minimum and maximum to stay in sync with v4l2_queryctrl. OK. > > > >> + __u32 flags; > > > >> + __u32 cols; > >> + __u32 rows; > >> + __u32 elem_size; > > > > The three above seem to be too specific for an array. > > > > I would put those on a separate struct and add here an union, > > like: > > > > union { > > struct v4l2_array arr; > > __u32 reserved[8]; > > } > > I have to sleep on this. I'm not sure this helps in any way. Well, for today's needs this may not bring any difference, but it may help if we need to add something else there. Also, it helps to make clear, at the documentation which parts of the struct will be filled every time, and with part is array-specific. > > > > >> + __u32 reserved[17]; > > > > This also seems too much. Why 17? > > It aligned the struct up to some nice number. Also, experience tells me that > whenever I limit the number of reserved fields it bites me later. > > > > >> +}; > > > >> + > >> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ > >> struct v4l2_querymenu { > >> __u32 id; > >> @@ -1965,6 +1994,8 @@ struct v4l2_create_buffers { > >> Never use these in applications! */ > >> #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) > >> + > >> /* Reminder: when adding new ioctls please add support for them to > >> drivers/media/video/v4l2-compat-ioctl32.c as well! */ > >> > > > > > > Regards, > > Hans
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 4d7782a..858a6f3 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -1272,6 +1272,35 @@ struct v4l2_queryctrl { __u32 reserved[2]; }; +/* Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */ +struct v4l2_query_ext_ctrl { + __u32 id; + __u32 type; + char name[32]; + char unit[32]; + union { + __s64 val; + __u32 reserved[4]; + } min; + union { + __s64 val; + __u32 reserved[4]; + } max; + union { + __u64 val; + __u32 reserved[4]; + } step; + union { + __s64 val; + __u32 reserved[4]; + } def; + __u32 flags; + __u32 cols; + __u32 rows; + __u32 elem_size; + __u32 reserved[17]; +}; + /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ struct v4l2_querymenu { __u32 id; @@ -1965,6 +1994,8 @@ struct v4l2_create_buffers { Never use these in applications! */ #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) + /* Reminder: when adding new ioctls please add support for them to drivers/media/video/v4l2-compat-ioctl32.c as well! */