| Message ID | 1390221974-28194-6-git-send-email-hverkuil@xs4all.nl (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
On 01/20/2014 01:45 PM, Hans Verkuil wrote: > 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 | 30 ++++++++++++++++++++++++++++++ > 1 file changed, 30 insertions(+) > > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h > index 4d7782a..9e5b7d4 100644 > --- a/include/uapi/linux/videodev2.h > +++ b/include/uapi/linux/videodev2.h > @@ -1272,6 +1272,34 @@ 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; Are these reserved[] arrays of any use ? > + __u32 flags; > + __u32 cols, rows; nit: I would put them on separate lines and use full words. > + __u32 elem_size; > + __u32 reserved[17]; > +}; > + > /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ > struct v4l2_querymenu { > __u32 id; > @@ -1965,6 +1993,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, Sylwester -- 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
On 01/23/2014 12:02 AM, Sylwester Nawrocki wrote: > On 01/20/2014 01:45 PM, Hans Verkuil wrote: >> 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 | 30 ++++++++++++++++++++++++++++++ >> 1 file changed, 30 insertions(+) >> >> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >> index 4d7782a..9e5b7d4 100644 >> --- a/include/uapi/linux/videodev2.h >> +++ b/include/uapi/linux/videodev2.h >> @@ -1272,6 +1272,34 @@ 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; > > Are these reserved[] arrays of any use ? Excellent question. I'd like to know as well :-) The idea is that if the type of the control is complex, then for certain types it might still make sense to have a range. E.g. say that the type is v4l2_rect, then you can define min/max/step/def v4l2_rect entries in the unions. Ditto for a v4l2_fract (it would be nice to be able to specify the min/max allowed scaling factors, for example). The question is, am I over-engineering or is this the best idea since sliced bread? Without the 'reserved' part this idea will be impossible to implement, and I don't think it hurts to have it in. > >> + __u32 flags; >> + __u32 cols, rows; > > nit: I would put them on separate lines and use full words. Separate lines: no problem, but do I really have to write 'columns' in full? :-( Regards, Hans > >> + __u32 elem_size; >> + __u32 reserved[17]; >> +}; >> + >> /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ >> struct v4l2_querymenu { >> __u32 id; >> @@ -1965,6 +1993,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, > Sylwester > -- 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
On 23/01/14 15:23, Hans Verkuil wrote: > On 01/23/2014 12:02 AM, Sylwester Nawrocki wrote: >> On 01/20/2014 01:45 PM, Hans Verkuil wrote: >>> 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 | 30 ++++++++++++++++++++++++++++++ >>> 1 file changed, 30 insertions(+) >>> >>> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h >>> index 4d7782a..9e5b7d4 100644 >>> --- a/include/uapi/linux/videodev2.h >>> +++ b/include/uapi/linux/videodev2.h >>> @@ -1272,6 +1272,34 @@ 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; >> >> Are these reserved[] arrays of any use ? > > Excellent question. I'd like to know as well :-) > > The idea is that if the type of the control is complex, then for certain types > it might still make sense to have a range. E.g. say that the type is v4l2_rect, > then you can define min/max/step/def v4l2_rect entries in the unions. Ditto > for a v4l2_fract (it would be nice to be able to specify the min/max allowed > scaling factors, for example). Huh, sorry, I misread the patch. Please ignore this comment. Certainly we need an ability to query other compound control types as well. 16 bytes seems a reasonable size, I guess it is going to be sufficient for most cases. If not we could add a pointer member to the union...? > The question is, am I over-engineering or is this the best idea since sliced > bread? Without the 'reserved' part this idea will be impossible to implement, > and I don't think it hurts to have it in. Yes, that's how I imagined it as well, I didn't mean questioning the union idea at all. >>> + __u32 flags; >>> + __u32 cols, rows; >> >> nit: I would put them on separate lines and use full words. > > Separate lines: no problem, but do I really have to write 'columns' in full? :-( Yes, sorry, one shall abide by the rules! :-) Really, it's up to you - as the author, I think you're entitled to decide about such details. ;) The short version looks probably neater anyway. -- Regards, Sylwester -- 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
Hi Hans, On Mon, Jan 20, 2014 at 01:45:58PM +0100, Hans Verkuil wrote: > + union { > + __u64 val; > + __u32 reserved[4]; > + } step; While I do not question that step is obviously always a positive value (or zero), using a different type from the value (and min and max) does add slight complications every time it is being used. I don't think there's a use case for using values over 2^62 for step either. Speaking of which --- do you think we should continue to have step in the interface? This has been proven to be slightly painful when the step is not an integer. Using a step of one in that case has been the only feasible solution. Step could be naturally be used as a hint but enforcing it often forces setting it to one.
Hi Sakari, On 01/24/2014 12:28 PM, Sakari Ailus wrote: > Hi Hans, > > On Mon, Jan 20, 2014 at 01:45:58PM +0100, Hans Verkuil wrote: >> + union { >> + __u64 val; >> + __u32 reserved[4]; >> + } step; > > While I do not question that step is obviously always a positive value (or > zero), using a different type from the value (and min and max) does add > slight complications every time it is being used. I don't think there's a > use case for using values over 2^62 for step either. What sort of complications? The reason I changed it is to avoid having to check for step < 0. It also makes it clear that it has to be positive. > > Speaking of which --- do you think we should continue to have step in the > interface? This has been proven to be slightly painful when the step is not > an integer. Using a step of one in that case has been the only feasible > solution. Step could be naturally be used as a hint but enforcing it often > forces setting it to one. Step is used quite often, so we can't remove it. If the step for a particular control isn't fixed over the range of possible values (at least, I think that is what you mean), then I don't see any solution that isn't painful. Not to mention that GUIs will have a hard time. Suggestions are welcome, though. 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
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h index 4d7782a..9e5b7d4 100644 --- a/include/uapi/linux/videodev2.h +++ b/include/uapi/linux/videodev2.h @@ -1272,6 +1272,34 @@ 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, rows; + __u32 elem_size; + __u32 reserved[17]; +}; + /* Used in the VIDIOC_QUERYMENU ioctl for querying menu items */ struct v4l2_querymenu { __u32 id; @@ -1965,6 +1993,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! */