From mboxrd@z Thu Jan 1 00:00:00 1970 Return-path: Received: from smtp-vbr15.xs4all.nl ([194.109.24.35]:4263 "EHLO smtp-vbr15.xs4all.nl" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751777AbaCMH6z (ORCPT ); Thu, 13 Mar 2014 03:58:55 -0400 Message-ID: <5321652D.3020009@xs4all.nl> Date: Thu, 13 Mar 2014 08:58:37 +0100 From: Hans Verkuil MIME-Version: 1.0 To: Mauro Carvalho Chehab CC: linux-media@vger.kernel.org, laurent.pinchart@ideasonboard.com, s.nawrocki@samsung.com, ismael.luceno@corp.bluecherry.net, pete@sensoray.com, sakari.ailus@iki.fi, Hans Verkuil Subject: Re: [REVIEWv3 PATCH 19/35] DocBook media: document VIDIOC_QUERY_EXT_CTRL. References: <1392631070-41868-1-git-send-email-hverkuil@xs4all.nl> <1392631070-41868-20-git-send-email-hverkuil@xs4all.nl> <20140312111338.79bb561f@samsung.com> In-Reply-To: <20140312111338.79bb561f@samsung.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Sender: linux-media-owner@vger.kernel.org List-ID: On 03/12/2014 03:13 PM, Mauro Carvalho Chehab wrote: > Em Mon, 17 Feb 2014 10:57:34 +0100 > Hans Verkuil escreveu: > >> From: Hans Verkuil >> >> Signed-off-by: Hans Verkuil >> Reviewed-by: Sylwester Nawrocki >> --- >> .../DocBook/media/v4l/vidioc-queryctrl.xml | 223 +++++++++++++++++---- >> 1 file changed, 189 insertions(+), 34 deletions(-) >> >> diff --git a/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml b/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml >> index e6645b9..da0e534 100644 >> --- a/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml >> +++ b/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml >> @@ -1,11 +1,12 @@ >> >> >> - ioctl VIDIOC_QUERYCTRL, VIDIOC_QUERYMENU >> + ioctl VIDIOC_QUERYCTRL, VIDIOC_QUERY_EXT_CTRL, VIDIOC_QUERYMENU >> &manvol; >> >> >> >> VIDIOC_QUERYCTRL >> + VIDIOC_QUERY_EXT_CTRL >> VIDIOC_QUERYMENU >> Enumerate controls and menu control items >> >> @@ -24,6 +25,14 @@ >> int ioctl >> int fd >> int request >> + struct v4l2_query_ext_ctrl *argp >> + >> + >> + >> + >> + int ioctl >> + int fd >> + int request >> struct v4l2_querymenu *argp >> >> >> @@ -42,7 +51,7 @@ >> >> request >> >> - VIDIOC_QUERYCTRL, VIDIOC_QUERYMENU >> + VIDIOC_QUERYCTRL, VIDIOC_QUERY_EXT_CTRL, VIDIOC_QUERYMENU >> >> >> >> @@ -91,7 +100,26 @@ prematurely end the enumeration). >> V4L2_CTRL_FLAG_NEXT_CTRL the driver returns the >> next supported control, or EINVAL if there is >> none. Drivers which do not support this flag yet always return >> -EINVAL. >> +EINVAL. Hidden controls (i.e. controls >> +with the V4L2_CTRL_FLAG_HIDDEN flag set) are >> +skipped when using the V4L2_CTRL_FLAG_NEXT_CTRL >> +flag. Use the VIDIOC_QUERY_EXT_CTRL for that. > > I suspect that most of the things (if not all) here already commented > on the initial patches) > > In this case, as already commented, I don't see any reason why > to deny VIDIOC_QUERYCTRL to get the hidden controls if V4L2_CTRL_FLAG_HIDDEN > is used. > >> + >> + The VIDIOC_QUERY_EXT_CTRL ioctl was >> +introduced in order to better support controls that can use complex >> +types, and to expose additional control information that cannot be >> +returned in &v4l2-queryctrl; since that structure is full. > > s/complex/compound/g > >> + >> + VIDIOC_QUERY_EXT_CTRL is used in the >> +same way as VIDIOC_QUERYCTRL, except that the >> +reserved array must be zeroed as well. >> +In addition, the V4L2_CTRL_FLAG_NEXT_HIDDEN flag >> +can be specified to enumerate all hidden controls (i.e. controls >> +with the V4L2_CTRL_FLAG_HIDDEN flag set, which >> +includes all controls with complex types). Specify both >> +V4L2_CTRL_FLAG_NEXT_CTRL and >> +V4L2_CTRL_FLAG_NEXT_HIDDEN in order to enumerate >> +all controls, hidden or not. >> >> Additional information is required for menu controls: the >> names of the menu items. To query them applications set the >> @@ -142,38 +170,23 @@ string. This information is intended for the user. >> __s32 >> minimum >> Minimum value, inclusive. This field gives a lower >> -bound for V4L2_CTRL_TYPE_INTEGER controls and the >> -lowest valid index for V4L2_CTRL_TYPE_MENU controls. >> -For V4L2_CTRL_TYPE_STRING controls the minimum value >> -gives the minimum length of the string. This length does not include the terminating >> -zero. It may not be valid for any other type of control, including >> -V4L2_CTRL_TYPE_INTEGER64 controls. Note that this is a >> -signed value. >> +bound for the control. See &v4l2-ctrl-type; how the minimum value is to >> +be used for each possible control type. Note that this a signed 32-bit value. >> >> >> __s32 >> maximum >> Maximum value, inclusive. This field gives an upper >> -bound for V4L2_CTRL_TYPE_INTEGER controls and the >> -highest valid index for V4L2_CTRL_TYPE_MENU >> -controls. For V4L2_CTRL_TYPE_BITMASK controls it is the >> -set of usable bits. >> -For V4L2_CTRL_TYPE_STRING controls the maximum value >> -gives the maximum length of the string. This length does not include the terminating >> -zero. It may not be valid for any other type of control, including >> -V4L2_CTRL_TYPE_INTEGER64 controls. Note that this is a >> -signed value. >> +bound for the control. See &v4l2-ctrl-type; how the maximum value is to >> +be used for each possible control type. Note that this a signed 32-bit value. >> >> >> __s32 >> step >> - This field gives a step size for >> -V4L2_CTRL_TYPE_INTEGER controls. For >> -V4L2_CTRL_TYPE_STRING controls this field refers to >> -the string length that has to be a multiple of this step size. >> -It may not be valid for any other type of control, including >> -V4L2_CTRL_TYPE_INTEGER64 >> -controls.Generally drivers should not scale hardware >> + This field gives a step size for the control. >> +See &v4l2-ctrl-type; how the step value is to be used for each possible >> +control type. Note that this an unsigned 32-bit value. >> +Generally drivers should not scale hardware >> control values. It may be necessary for example when the >> name or id imply >> a particular unit and the hardware actually accepts only multiples of >> @@ -192,10 +205,11 @@ be always positive. >> default_value >> The default value of a >> V4L2_CTRL_TYPE_INTEGER, >> -_BOOLEAN or _MENU control. >> -Not valid for other types of controls. Drivers reset controls only >> -when the driver is loaded, not later, in particular not when the >> -func-open; is called. >> +_BOOLEAN, _BITMASK, >> +_MENU or _INTEGER_MENU control. >> +Not valid for other types of controls. >> +Note that drivers reset controls to their default value only when the >> +driver is first loaded, never afterwards. >> >> >> __u32 >> @@ -213,6 +227,129 @@ the array to zero. >> >> >> >> + >> + struct <structname>v4l2_query_ext_ctrl</structname> >> + >> + &cs-str; >> + >> + >> + __u32 >> + id >> + Identifies the control, set by the application. See >> + for predefined IDs. When the ID is ORed >> +with V4L2_CTRL_FLAG_NEXT_CTRL the driver clears the >> +flag and returns the first non-hidden control with a higher ID. When the >> +ID is ORed with V4L2_CTRL_FLAG_NEXT_HIDDEN the driver >> +clears the flag and returns the first hidden control with a higher ID. >> +Set both to get the first control (hidden or not) with a higher ID. >> + >> + >> + __u32 >> + type >> + Type of control, see > + linkend="v4l2-ctrl-type" />. >> + >> + >> + char >> + name[32] >> + Name of the control, a NUL-terminated ASCII >> +string. This information is intended for the user. >> + This is something I should point your attention to: the type of name[] has changed from u8 in v4l2_queryctrl to char here. The use of u8 in v4l2_queryctrl always causes problems (e.g. you can't assign it directly to a std::string in C++ since that expects a char *). I never understood why u8 was ever chosen in the first place. But is this something you agree with? >> + >> + char >> + unit[32] >> + The name of the unit of the control's value, a NUL-terminated ASCII >> +string. This information is intended for the user. This may be an empty string if no >> +unit is known or if it is not applicable to this particular control. > > Charset? There are some units, like ångström (Å) that are not ASCII. > Also, there are several units that use greek letters. So, I think we need > to specify a charset here. IMHO, the better is to use UTF-8. I agree. Should I specify the same for name[] in both this struct and in v4l2_queryctrl? > >> + >> + >> + __s64 >> + minimum >> + Minimum value, inclusive. This field gives a lower >> +bound for the control. See &v4l2-ctrl-type; how the minimum value is to >> +be used for each possible control type. Note that this a signed 64-bit value. >> + >> + >> + __s64 >> + maximum >> + Maximum value, inclusive. This field gives an upper >> +bound for the control. See &v4l2-ctrl-type; how the maximum value is to >> +be used for each possible control type. Note that this a signed 64-bit value. >> + >> + >> + __u64 >> + step >> + This field gives a step size for the control. >> +See &v4l2-ctrl-type; how the step value is to be used for each possible >> +control type. Note that this an unsigned 64-bit value. >> +Generally drivers should not scale hardware >> +control values. It may be necessary for example when the >> +name or id imply >> +a particular unit and the hardware actually accepts only multiples of >> +said unit. If so, drivers must take care values are properly rounded >> +when scaling, such that errors will not accumulate on repeated >> +read-write cycles.This field gives the smallest change of >> +an integer control actually affecting hardware. Often the information >> +is needed when the user can change controls by keyboard or GUI >> +buttons, rather than a slider. When for example a hardware register >> +accepts values 0-511 and the driver reports 0-65535, step should be >> +128. >> + >> + >> + __s64 >> + default_value >> + The default value of a >> +V4L2_CTRL_TYPE_INTEGER, _INTEGER64, >> +_BOOLEAN, _BITMASK, >> +_MENU or _INTEGER_MENU control. >> +Not valid for other types of controls. >> +Note that drivers reset controls to their default value only when the >> +driver is first loaded, never afterwards. >> + >> + >> + >> + __u32 >> + flags >> + Control flags, see > + linkend="control-flags" />. >> + >> + >> + __u32 >> + cols >> + The number of columns in the matrix. If this control >> +is not a matrix, then both cols and >> +rows are 1. cols >> +can never be 0. >> + >> + >> + __u32 >> + rows >> + The number of rows in the matrix. If this control >> +is not a matrix, then both cols and >> +rows are 1. rows >> +can never be 0. >> + > > As already commented, cols/rows are matrix specific. If the control type > is not a matrix, those makes no sense. > >> + >> + __u32 >> + elem_size >> + The size in bytes of a single element of the matrix. >> +Given a char pointer p to the matrix you can find the >> +position of cell (y, x) as follows: >> +p + (y * cols + x) * elem_size. elem_size >> +is always valid, also when the control isn't a matrix. For string controls >> +elem_size is equal to maximum + 1. >> + >> + >> + >> + __u32 >> + reserved[17] >> + Reserved for future extensions. Applications and drivers >> +must set the array to zero. >> + >> + >> + >> +
>> + >> >> struct <structname>v4l2_querymenu</structname> >> >> @@ -347,11 +484,14 @@ Drivers must ignore the value passed with >> >> >> V4L2_CTRL_TYPE_INTEGER64 >> - n/a >> - n/a >> - n/a >> + any >> + any >> + any >> A 64-bit integer valued control. Minimum, maximum >> -and step size cannot be queried. >> +and step size cannot be queried using VIDIOC_QUERYCTRL. >> +Only VIDIOC_QUERY_EXT_CTRL can retrieve the 64-bit >> +min/max/step values, they should be interpreted as n/a when using >> +VIDIOC_QUERYCTRL. >> >> >> V4L2_CTRL_TYPE_STRING >> @@ -450,6 +590,21 @@ is in auto-gain mode. In such a case the hardware calculates the gain value base >> the lighting conditions which can change over time. Note that setting a new value for >> a volatile control will have no effect. The new value will just be ignored. >> >> + >> + V4L2_CTRL_FLAG_HIDDEN >> + 0x0100 >> + This control is hidden and should not be shown in a GUI application. >> +Hidden controls are skipped when enumerating them using V4L2_CTRL_FLAG_NEXT_CTRL, >> +and they can only be enumerated by using the V4L2_CTRL_FLAG_NEXT_HIDDEN >> +flag. All controls that have a complex type have this flag set. >> + >> + >> + V4L2_CTRL_FLAG_IS_PTR >> + 0x0200 >> + This control has a pointer type, so its value has to be accessed >> +using one of the pointer fields of &v4l2-ext-control;. This flag is set for controls >> +that are a matrix, string, or have a complex type. > > This seems messy: use IS_PTR for string too. As already pointed, string > is a NUL-terminated sequence, while pointer is a size defined > element, sequence or matrix. Let's not mix this at the API. This is already > complex enough. How about this: HAS_PAYLOAD? What I discovered when trying to use the API in an application is that you need a simple way to discover if the type is a simple s32 or s64 type which is just set as part of v4l2_ext_control, or if it is a more complicated type where you need to use the size field and allocate memory to store the value. In the past that was a simple 'type == STRING' test, but that does not suffice anymore. And in order to be able to do generic control manipulation you do not want to have to test for all sorts of types or create complicated conditions. I found that a simple flag actually made life much easier. Regards, Hans