[PATCH 0/5] Add units to controls

[PATCH 0/5] Add units to controls

[Sakari Ailus]

Hi all,

This set adds a few things to the current control framework in terms of
what kind of information the user space may have on controls. It adds
support for units and prefixes, exponential base as well as information on
whether a control is linear or exponential, to struct v4l2_query_ext_ctrl.

The smiapp driver gains support for the feature. In the near term, some
controls could also be assigned the unit automatically. The pixel rate,
for instance. Fewer driver changes would be needed this way. A driver
could override the value if there's a need to.

I think I'll merge the undefined and no unit cases. Same for the
exponential base actually --- the flag can be removed, too...

Regarding Ricardo's suggestion --- I was thinking of adding a control flag
(yes, there are a few bits available) to tell how to round the value. The
user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or
previous) control value by incrementing the current value and setting the
appropriate flag. This is out of the scope of this set though.

Comments, questions?

Sakari Ailus (5):
  videodev2.h: Use 8 hexadecimals (32 bits) for control flags
  v4l: controls: Add support for exponential bases, prefixes and units
  Documentation: media: Document control exponential bases, units,
    prefixes
  v4l: controls: QUERY_EXT_CTRL support for base, prefix and unit
  smiapp: Set control units

 Documentation/media/uapi/v4l/extended-controls.rst |   2 +
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst  | 174 +++++++++++++++++++--
 Documentation/media/videodev2.h.rst.exceptions     |  22 +++
 drivers/media/i2c/smiapp/smiapp-core.c             |  16 +-
 drivers/media/v4l2-core/v4l2-ctrls.c               |   3 +
 include/media/v4l2-ctrls.h                         |   2 +
 include/uapi/linux/videodev2.h                     |  54 +++++--
 7 files changed, 242 insertions(+), 31 deletions(-)

-- 
2.11.0

[PATCH 1/5] videodev2.h: Use 8 hexadecimals (32 bits) for control flags

[Sakari Ailus]

The V4L2 control flags are a 32-bit bitmask. Use 32-bit hexadecimal
numbers to specify the flags (was 16).

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 22 +++++++++++-----------
 include/uapi/linux/videodev2.h                    | 22 +++++++++++-----------
 2 files changed, 22 insertions(+), 22 deletions(-)

diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
index 5bd26e8c9a1a0..ff2d131223b84 100644
--- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
+++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
@@ -439,37 +439,37 @@ See also the examples in :ref:`control`.
     :widths:       3 1 4
 
     * - ``V4L2_CTRL_FLAG_DISABLED``
-      - 0x0001
+      - 0x00000001
       - This control is permanently disabled and should be ignored by the
 	application. Any attempt to change the control will result in an
 	``EINVAL`` error code.
     * - ``V4L2_CTRL_FLAG_GRABBED``
-      - 0x0002
+      - 0x00000002
       - This control is temporarily unchangeable, for example because
 	another application took over control of the respective resource.
 	Such controls may be displayed specially in a user interface.
 	Attempts to change the control may result in an ``EBUSY`` error code.
     * - ``V4L2_CTRL_FLAG_READ_ONLY``
-      - 0x0004
+      - 0x00000004
       - This control is permanently readable only. Any attempt to change
 	the control will result in an ``EINVAL`` error code.
     * - ``V4L2_CTRL_FLAG_UPDATE``
-      - 0x0008
+      - 0x00000008
       - A hint that changing this control may affect the value of other
 	controls within the same control class. Applications should update
 	their user interface accordingly.
     * - ``V4L2_CTRL_FLAG_INACTIVE``
-      - 0x0010
+      - 0x00000010
       - This control is not applicable to the current configuration and
 	should be displayed accordingly in a user interface. For example
 	the flag may be set on a MPEG audio level 2 bitrate control when
 	MPEG audio encoding level 1 was selected with another control.
     * - ``V4L2_CTRL_FLAG_SLIDER``
-      - 0x0020
+      - 0x00000020
       - A hint that this control is best represented as a slider-like
 	element in a user interface.
     * - ``V4L2_CTRL_FLAG_WRITE_ONLY``
-      - 0x0040
+      - 0x00000040
       - This control is permanently writable only. Any attempt to read the
 	control will result in an ``EACCES`` error code error code. This flag
 	is typically present for relative controls or action controls
@@ -477,7 +477,7 @@ See also the examples in :ref:`control`.
 	action (e. g. motor control) but no meaningful value can be
 	returned.
     * - ``V4L2_CTRL_FLAG_VOLATILE``
-      - 0x0080
+      - 0x00000080
       - This control is volatile, which means that the value of the
 	control changes continuously. A typical example would be the
 	current gain value if the device is in auto-gain mode. In such a
@@ -493,7 +493,7 @@ See also the examples in :ref:`control`.
 	   Setting a new value for a volatile control will *never* trigger a
 	   :ref:`V4L2_EVENT_CTRL_CH_VALUE <ctrl-changes-flags>` event.
     * - ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
-      - 0x0100
+      - 0x00000100
       - This control has a pointer type, so its value has to be accessed
 	using one of the pointer fields of struct
 	:c:type:`v4l2_ext_control`. This flag is set
@@ -503,7 +503,7 @@ See also the examples in :ref:`control`.
     * .. _FLAG_EXECUTE_ON_WRITE:
 
       - ``V4L2_CTRL_FLAG_EXECUTE_ON_WRITE``
-      - 0x0200
+      - 0x00000200
       - The value provided to the control will be propagated to the driver
 	even if it remains constant. This is required when the control
 	represents an action on the hardware. For example: clearing an
@@ -512,7 +512,7 @@ See also the examples in :ref:`control`.
     * .. _FLAG_MODIFY_LAYOUT:
 
       - ``V4L2_CTRL_FLAG_MODIFY_LAYOUT``
-      - 0x0400
+      - 0x00000400
       - Changing this control value may modify the layout of the
         buffer (for video devices) or the media bus format (for sub-devices).
 
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 184e4dbe8f9c0..ae083978988f1 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1681,17 +1681,17 @@ struct v4l2_querymenu {
 } __attribute__ ((packed));
 
 /*  Control flags  */
-#define V4L2_CTRL_FLAG_DISABLED		0x0001
-#define V4L2_CTRL_FLAG_GRABBED		0x0002
-#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
-#define V4L2_CTRL_FLAG_UPDATE		0x0008
-#define V4L2_CTRL_FLAG_INACTIVE		0x0010
-#define V4L2_CTRL_FLAG_SLIDER		0x0020
-#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
-#define V4L2_CTRL_FLAG_VOLATILE		0x0080
-#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
-#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
-#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
+#define V4L2_CTRL_FLAG_DISABLED		0x00000001
+#define V4L2_CTRL_FLAG_GRABBED		0x00000002
+#define V4L2_CTRL_FLAG_READ_ONLY	0x00000004
+#define V4L2_CTRL_FLAG_UPDATE		0x00000008
+#define V4L2_CTRL_FLAG_INACTIVE		0x00000010
+#define V4L2_CTRL_FLAG_SLIDER		0x00000020
+#define V4L2_CTRL_FLAG_WRITE_ONLY	0x00000040
+#define V4L2_CTRL_FLAG_VOLATILE		0x00000080
+#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x00000100
+#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x00000200
+#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x00000400
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.11.0

[PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Sakari Ailus]

Add support for exponential bases, prefixes as well as units for V4L2
controls. This makes it possible to convey information on the relation
between the control value and the hardware feature being controlled.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++-
 1 file changed, 31 insertions(+), 1 deletion(-)

diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index ae083978988f1..23b02f2db85a1 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
 	__u32		     reserved[2];
 };
 
+/* V4L2 control exponential bases */
+#define V4L2_CTRL_BASE_UNDEFINED	0
+#define V4L2_CTRL_BASE_LINEAR		1
+#define V4L2_CTRL_BASE_2		2
+#define V4L2_CTRL_BASE_10		10
+
+/* V4L2 control unit prefixes */
+#define V4L2_CTRL_PREFIX_NANO		-9
+#define V4L2_CTRL_PREFIX_MICRO		-6
+#define V4L2_CTRL_PREFIX_MILLI		-3
+#define V4L2_CTRL_PREFIX_1		0
+#define V4L2_CTRL_PREFIX_KILO		3
+#define V4L2_CTRL_PREFIX_MEGA		6
+#define V4L2_CTRL_PREFIX_GIGA		9
+
+/* V4L2 control units */
+#define V4L2_CTRL_UNIT_UNDEFINED	0
+#define V4L2_CTRL_UNIT_NONE		1
+#define V4L2_CTRL_UNIT_SECOND		2
+#define V4L2_CTRL_UNIT_AMPERE		3
+#define V4L2_CTRL_UNIT_LINE		4
+#define V4L2_CTRL_UNIT_PIXEL		5
+#define V4L2_CTRL_UNIT_PIXELS_PER_SEC	6
+#define V4L2_CTRL_UNIT_HZ		7
+
+
 /*  Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
 struct v4l2_query_ext_ctrl {
 	__u32		     id;
@@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
 	__u32                elems;
 	__u32                nr_of_dims;
 	__u32                dims[V4L2_CTRL_MAX_DIMS];
-	__u32		     reserved[32];
+	__u8		     base;
+	__s8		     prefix;
+	__u16		     unit;
+	__u32		     reserved[31];
 };
 
 /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
@@ -1692,6 +1721,7 @@ struct v4l2_querymenu {
 #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x00000100
 #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x00000200
 #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x00000400
+#define V4L2_CTRL_FLAG_EXPONENTIAL	0x00000800
 
 /*  Query flags, to be ORed with the control ID */
 #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
-- 
2.11.0

[PATCH 3/5] Documentation: media: Document control exponential bases, units, prefixes

[Sakari Ailus]

Document V4L2 control exponential bases, units and prefixes, as well as
the control flag telling a control value is an exponent.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 Documentation/media/uapi/v4l/extended-controls.rst |   2 +
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst  | 152 ++++++++++++++++++++-
 Documentation/media/videodev2.h.rst.exceptions     |  22 +++
 3 files changed, 175 insertions(+), 1 deletion(-)

diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
index 9f7312bf33651..8461fd92d1b9e 100644
--- a/Documentation/media/uapi/v4l/extended-controls.rst
+++ b/Documentation/media/uapi/v4l/extended-controls.rst
@@ -3460,6 +3460,8 @@ Image Process Control IDs
     by selecting the desired horizontal and vertical blanking. The unit
     of this control is Hz.
 
+.. _v4l2_cid_pixel_rate:
+
 ``V4L2_CID_PIXEL_RATE (64-bit integer)``
     Pixel rate in the source pads of the subdev. This control is
     read-only and its unit is pixels / second.
diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
index ff2d131223b84..472378f5d7566 100644
--- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
+++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
@@ -269,8 +269,22 @@ See also the examples in :ref:`control`.
       - ``dims[V4L2_CTRL_MAX_DIMS]``
       - The size of each dimension. The first ``nr_of_dims`` elements of
 	this array must be non-zero, all remaining elements must be zero.
+    * - __u8
+      - ``base``
+      - The exponential base of the control value. Valid only if the
+	:ref:`V4L2_CTRL_FLAG_EXPONENTIAL <FLAG_EXPONENTIAL>` control flag is
+	set. This is an enumeration.
+    * - __u8
+      - ``prefix``
+      - Prefix of the unit. This is an enumeration.
+    * - __u16
+      - ``unit``
+      - Unit of the value. Together with the ``prefix`` as well as the ``base``
+	field (if :ref:`V4L2_CTRL_FLAG_EXPONENTIAL <FLAG_EXPONENTIAL>` is set),
+	defines the relation between the control value and the property of the
+	hardware being controlled. This is an enumeration.
     * - __u32
-      - ``reserved``\ [32]
+      - ``reserved``\ [31]
       - Reserved for future extensions. Applications and drivers must set
 	the array to zero.
 
@@ -523,6 +537,142 @@ See also the examples in :ref:`control`.
 	streaming is in progress since most drivers do not support changing
 	the format in that case.
 
+    * .. _FLAG_EXPONENTIAL:
+
+      - ``V4L2_CTRL_FLAG_EXPONENTIAL``
+      - 0x00000800
+
+      - The value of the control has an exponential relation to the feature
+	being controled instead of a linear relation. In other words, the value
+	of the control is an exponent of the base specified in the
+        base field in &struct v4l2_query_ext_ctrl.
+
+
+.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
+
+.. _control-bases:
+
+.. cssclass:: longtable
+
+.. flat-table:: Control Exponential Bases
+    :header-rows:  1
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - Base Name
+      - Value
+      - Description
+
+    * - ``V4L2_CTRL_BASE_UNDEFINED``
+      - 0
+      - The control exponential base is not defined.
+
+    * - ``V4L2_CTRL_BASE_LINEAR``
+      - 1
+      - The control is linear.
+
+    * - ``V4L2_CTRL_BASE_2``
+      - 2
+      - The base of the control is 2.
+
+    * - ``V4L2_CTRL_BASE_10``
+      - 10
+      - The base of the control is 10.
+
+.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
+
+.. _control-prefixes:
+
+.. cssclass:: longtable
+
+.. flat-table:: Control Prefixes
+    :header-rows:  1
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - Prefix Name
+      - Value
+      - Description
+
+    * - ``V4L2_CTRL_PREFIX_NANO``
+      - -9
+      - Nano
+
+    * - ``V4L2_CTRL_PREFIX_MICRO``
+      - -6
+      - Micro
+
+    * - ``V4L2_CTRL_PREFIX_MILLI``
+      - -3
+      - Milli
+
+    * - ``V4L2_CTRL_PREFIX_1``
+      - 0
+      - \-
+
+    * - ``V4L2_CTRL_PREFIX_KILO``
+      - 3
+      - Kilo
+
+    * - ``V4L2_CTRL_PREFIX_MEGA``
+      - 6
+      - Mega
+
+    * - ``V4L2_CTRL_PREFIX_GIGA``
+      - 9
+      - Giga
+
+.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
+
+.. _control-units:
+
+.. cssclass:: longtable
+
+.. flat-table:: Control Units
+    :header-rows:  1
+    :stub-columns: 0
+    :widths:       3 1 4
+
+    * - Unit Name
+      - Value
+      - Description
+
+    * - ``V4L2_CTRL_UNIT_UNDEFINED``
+      - 0
+      - The unit of the control is not defined.
+
+    * - ``V4L2_CTRL_UNIT_NONE``
+      - 1
+      - The control has no unit.
+
+    * - ``V4L2_CTRL_UNIT_SECOND``
+      - 2
+      - Second
+
+    * - ``V4L2_CTRL_UNIT_AMPERE``
+      - 3
+      - Ampère
+
+    * - ``V4L2_CTRL_UNIT_LINE``
+      - 4
+      - A line of pixels in sensor's pixel matrix. This is a unit of time
+        commonly used by camera sensors in e.g. exposure control, i.e. the time
+        it takes for a sensor to read a line of pixels from the sensor's pixel
+	matrix. See :ref:`V4L2_CID_PIXEL_RATE <V4L2_CID_PIXEL_RATE>`.
+
+    * - ``V4L2_CTRL_UNIT_PIXEL``
+      - 5
+      - A pixel in sensor's pixel matrix. This is a unit of time commonly used
+        by camera sensors in e.g. exposure control, i.e. the time it takes for
+	a sensor to read a pixel from the sensor's pixel matrix.
+
+    * - ``V4L2_CTRL_UNIT_PIXEL``
+      - 6
+      - Pixels per second
+
+    * - ``V4L2_CTRL_UNIT_HZ
+      - 7
+      - Hertz
 
 Return Value
 ============
diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
index 63fa131729c09..8183a4f3554b0 100644
--- a/Documentation/media/videodev2.h.rst.exceptions
+++ b/Documentation/media/videodev2.h.rst.exceptions
@@ -343,11 +343,33 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
 replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
 replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
 replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
+replace define V4L2_CTRL_FLAG_EXPONENTIAL control-flags
 
 replace define V4L2_CTRL_FLAG_NEXT_CTRL control
 replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
 replace define V4L2_CID_PRIVATE_BASE control
 
+# V4L2 control bases, prefixes and units
+replace define V4L2_CTRL_BASE_UNDEFINED control-bases
+replace define V4L2_CTRL_BASE_LINEAR control-bases
+replace define V4L2_CTRL_BASE_2 control-bases
+replace define V4L2_CTRL_BASE_10 control-bases
+
+replace define V4L2_CTRL_PREFIX_NANO control-prefixes
+replace define V4L2_CTRL_PREFIX_MICRO control-prefixes
+replace define V4L2_CTRL_PREFIX_MILLI control-prefixes
+replace define V4L2_CTRL_PREFIX_1 control-prefixes
+replace define V4L2_CTRL_PREFIX_KILO control-prefixes
+replace define V4L2_CTRL_PREFIX_MEGA control-prefixes
+replace define V4L2_CTRL_PREFIX_GIGA control-prefixes
+
+replace define V4L2_CTRL_UNIT_UNDEFINED control-units
+replace define V4L2_CTRL_UNIT_NONE control-units
+replace define V4L2_CTRL_UNIT_SECOND control-units
+replace define V4L2_CTRL_UNIT_AMPERE control-units
+replace define V4L2_CTRL_UNIT_LINE control-units
+replace define V4L2_CTRL_UNIT_PIXEL control-units
+
 # V4L2 tuner
 
 replace define V4L2_TUNER_CAP_LOW tuner-capability
-- 
2.11.0

[PATCH 4/5] v4l: controls: QUERY_EXT_CTRL support for base, prefix and unit

[Sakari Ailus]

Add support for conveying the information set by the driver to the user
space.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 3 +++
 include/media/v4l2-ctrls.h           | 2 ++
 2 files changed, 5 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ee006d34c19f0..8d2931b0a4701 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2732,6 +2732,9 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
 	qc->minimum = ctrl->minimum;
 	qc->maximum = ctrl->maximum;
 	qc->default_value = ctrl->default_value;
+	qc->base = ctrl->base;
+	qc->prefix = ctrl->prefix;
+	qc->unit = ctrl->unit;
 	if (ctrl->type == V4L2_CTRL_TYPE_MENU
 	    || ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU)
 		qc->step = 1;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index f615ba1b29dd9..d6aaf45b70381 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -220,6 +220,8 @@ struct v4l2_ctrl {
 	u32 elem_size;
 	u32 dims[V4L2_CTRL_MAX_DIMS];
 	u32 nr_of_dims;
+	u8 base, unit;
+	u16 prefix;
 	union {
 		u64 step;
 		u64 menu_skip_mask;
-- 
2.11.0

[PATCH 5/5] smiapp: Set control units

[Sakari Ailus]

Assign units for the controls exposed by the smiapp driver.

Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
---
 drivers/media/i2c/smiapp/smiapp-core.c | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
index 99f3b295ae3c7..289313c232430 100644
--- a/drivers/media/i2c/smiapp/smiapp-core.c
+++ b/drivers/media/i2c/smiapp/smiapp-core.c
@@ -562,17 +562,10 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 	sensor->vblank = v4l2_ctrl_new_std(
 		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
 		V4L2_CID_VBLANK, 0, 1, 1, 0);
-
-	if (sensor->vblank)
-		sensor->vblank->flags |= V4L2_CTRL_FLAG_UPDATE;
-
 	sensor->hblank = v4l2_ctrl_new_std(
 		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
 		V4L2_CID_HBLANK, 0, 1, 1, 0);
 
-	if (sensor->hblank)
-		sensor->hblank->flags |= V4L2_CTRL_FLAG_UPDATE;
-
 	sensor->pixel_rate_parray = v4l2_ctrl_new_std(
 		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
 		V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
@@ -589,6 +582,13 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 		return sensor->pixel_array->ctrl_handler.error;
 	}
 
+	sensor->exposure->unit = V4L2_CTRL_UNIT_LINE;
+	sensor->vblank->flags |= V4L2_CTRL_FLAG_UPDATE;
+	sensor->vblank->unit = V4L2_CTRL_UNIT_LINE;
+	sensor->hblank->flags |= V4L2_CTRL_FLAG_UPDATE;
+	sensor->hblank->unit = V4L2_CTRL_UNIT_PIXEL;
+	sensor->pixel_rate_parray->unit = V4L2_CTRL_UNIT_PIXELS_PER_SEC;
+
 	sensor->pixel_array->sd.ctrl_handler =
 		&sensor->pixel_array->ctrl_handler;
 
@@ -611,6 +611,8 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
 		return sensor->src->ctrl_handler.error;
 	}
 
+	sensor->pixel_rate_csi->unit = V4L2_CTRL_UNIT_PIXELS_PER_SEC;
+
 	sensor->src->sd.ctrl_handler = &sensor->src->ctrl_handler;
 
 	return 0;
-- 
2.11.0

Re: [PATCH 3/5] Documentation: media: Document control exponential bases, units, prefixes

[Helmut Grohne]

On Tue, Sep 25, 2018 at 12:14:32PM +0200, Sakari Ailus wrote:
> +    * - ``V4L2_CTRL_UNIT_PIXEL``
> +      - 5
> +      - A pixel in sensor's pixel matrix. This is a unit of time commonly used
> +        by camera sensors in e.g. exposure control, i.e. the time it takes for
> +	a sensor to read a pixel from the sensor's pixel matrix.
> +
> +    * - ``V4L2_CTRL_UNIT_PIXEL``
> +      - 6

The latter V4L2_CTRL_UNIT_PIXEL looks like a typo that should say
V4L2_CTRL_UNIT_PIXELS_PER_SEC.

Helmut

Re: [PATCH 3/5] Documentation: media: Document control exponential bases, units, prefixes

[Sakari Ailus]

On Tue, Sep 25, 2018 at 12:39:56PM +0200, Helmut Grohne wrote:
> On Tue, Sep 25, 2018 at 12:14:32PM +0200, Sakari Ailus wrote:
> > +    * - ``V4L2_CTRL_UNIT_PIXEL``
> > +      - 5
> > +      - A pixel in sensor's pixel matrix. This is a unit of time commonly used
> > +        by camera sensors in e.g. exposure control, i.e. the time it takes for
> > +	a sensor to read a pixel from the sensor's pixel matrix.
> > +
> > +    * - ``V4L2_CTRL_UNIT_PIXEL``
> > +      - 6
> 
> The latter V4L2_CTRL_UNIT_PIXEL looks like a typo that should say
> V4L2_CTRL_UNIT_PIXELS_PER_SEC.

Correct. I'll fix this for v2.

Thanks!

-- 
Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 0/5] Add units to controls

[Helmut Grohne]

On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
> This set adds a few things to the current control framework in terms of
> what kind of information the user space may have on controls. It adds
> support for units and prefixes, exponential base as well as information on
> whether a control is linear or exponential, to struct v4l2_query_ext_ctrl.

That's a great improvement. Being able to give meaning to controls is
great. However, I see two significant weaknesses in the approach being
taken:

1. There are a number of controls whose value is not easily described as
   either linear or exponential. I'm faced with at least two controls
   that actually are floating point numbers. One with two bits for the
   exponent and one (strange) bit for the mantissa (no joke) and another
   with three bits for the exponent and four bits for the mantissa.
   Neither can suitably be represented.

   Since the value ranges are small for the cases I mentioned, I looked
   into using an integer menu. The present approach does not allow for
   replacing an integer with an integer menu though. Each control id has
   a fixed type. I'm not sure how to solve this.

2. The present implementation places the responsibility of assigning
   units and scales into drivers. A number of controls (e.g.
   V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
   V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the
   documentation. Having each and every driver set units and scales in
   the documented way will lead to duplication and buggy code. Having
   each driver choose unit and scale will lead to inconsistency. It
   would be very good to have a mechanism that puts the framework in
   charge of maintaining units and scales for the standard control ids.

   What is a consumer supposed to do with a control that changes unit?
   The algorithm expected e.g. a duration. It might be able to convert
   to pixels, but what should it do if it gets back amperes? I argue
   that the unit should be a property of the control id and be
   documented (i.e. what is done now). If it is reported via an ioctl,
   the framework needs to be in charge of setting it.

   The story is much different for scales. Scaling the value up and down
   is something consumers can reasonably be expected to do. It allows
   shifting the value range such that the relevant values can be fully
   represented. Allowing drivers to change scales is much more
   reasonable. Still the framework should provide a default such that
   most drivers should not need any update.

I acknowledge that these expectations are high and see that they're
partially covered in your later remarks.

> The smiapp driver gains support for the feature. In the near term, some
> controls could also be assigned the unit automatically. The pixel rate,
> for instance. Fewer driver changes would be needed this way. A driver
> could override the value if there's a need to.

I believe that in the interest of keeping maintenance cost low, this
should happen rather sooner than later. Just even adding the support to
smiapp seems wrong when it would be possible to have the framework do
the work.

> I think I'll merge the undefined and no unit cases. Same for the
> exponential base actually --- the flag can be removed, too...

I'm not sure I understand. It reads like you are going to revert a
significant part of the patch.

> Regarding Ricardo's suggestion --- I was thinking of adding a control flag
> (yes, there are a few bits available) to tell how to round the value. The
> user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or
> previous) control value by incrementing the current value and setting the
> appropriate flag. This is out of the scope of this set though.

This approach sounds really useful to me. Having control over the
rounding would allow reading supported control values with reasonable
effort. With such an approach, a very sparsely populated control becomes
feasible and with integer64 controls that'd likely allow representing
most exponential controls with linear values. If going this route, I
don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.

Thus, I think that control over the rounding is tightly related to this
patchset and needs to be discussed together.

Helmut

Re: [PATCH 0/5] Add units to controls

[Sakari Ailus]

Hi Helmut,

Thanks for the comments.

On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote:
> On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
> > This set adds a few things to the current control framework in terms of
> > what kind of information the user space may have on controls. It adds
> > support for units and prefixes, exponential base as well as information on
> > whether a control is linear or exponential, to struct v4l2_query_ext_ctrl.
> 
> That's a great improvement. Being able to give meaning to controls is
> great. However, I see two significant weaknesses in the approach being
> taken:
> 
> 1. There are a number of controls whose value is not easily described as
>    either linear or exponential. I'm faced with at least two controls
>    that actually are floating point numbers. One with two bits for the
>    exponent and one (strange) bit for the mantissa (no joke) and another
>    with three bits for the exponent and four bits for the mantissa.
>    Neither can suitably be represented.
> 
>    Since the value ranges are small for the cases I mentioned, I looked
>    into using an integer menu. The present approach does not allow for
>    replacing an integer with an integer menu though. Each control id has
>    a fixed type. I'm not sure how to solve this.

The proposal does not address all potential situations, that's true.
There's no way to try to represent everything out there (without
enumerating the values) in an easily generalised way but something can be
done.

There are devices such as some flash LED controllers where the flash current
if simply a value you can pick from the list. It's currently implemented as
an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c .

> 
> 2. The present implementation places the responsibility of assigning
>    units and scales into drivers. A number of controls (e.g.
>    V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
>    V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the
>    documentation. Having each and every driver set units and scales in
>    the documented way will lead to duplication and buggy code. Having
>    each driver choose unit and scale will lead to inconsistency. It
>    would be very good to have a mechanism that puts the framework in
>    charge of maintaining units and scales for the standard control ids.
> 
>    What is a consumer supposed to do with a control that changes unit?
>    The algorithm expected e.g. a duration. It might be able to convert
>    to pixels, but what should it do if it gets back amperes? I argue
>    that the unit should be a property of the control id and be
>    documented (i.e. what is done now). If it is reported via an ioctl,

The fact is that the unit is specific to hardware. The documentation
documents something, and often suggests a unit, but if the hardware does
something else, then that's what you get --- independently of what the
documentation says.

Hence the need to convey it via the API.

Some controls could have the unit set by the framework if that makes sense.
Most drivers shouldn't actually need to touch this if they're fine with
defaults (whenever a control has a default).

>    the framework needs to be in charge of setting it.
> 
>    The story is much different for scales. Scaling the value up and down
>    is something consumers can reasonably be expected to do. It allows
>    shifting the value range such that the relevant values can be fully
>    represented. Allowing drivers to change scales is much more
>    reasonable. Still the framework should provide a default such that
>    most drivers should not need any update.
> 
> I acknowledge that these expectations are high and see that they're
> partially covered in your later remarks.
> 
> > The smiapp driver gains support for the feature. In the near term, some
> > controls could also be assigned the unit automatically. The pixel rate,
> > for instance. Fewer driver changes would be needed this way. A driver
> > could override the value if there's a need to.
> 
> I believe that in the interest of keeping maintenance cost low, this
> should happen rather sooner than later. Just even adding the support to
> smiapp seems wrong when it would be possible to have the framework do
> the work.
> 
> > I think I'll merge the undefined and no unit cases. Same for the
> > exponential base actually --- the flag can be removed, too...
> 
> I'm not sure I understand. It reads like you are going to revert a
> significant part of the patch.

A macro or two, it's not a major change. From the user space point of view,
does it make a difference if a control has no unit or when it's not known
what the unit is?

> 
> > Regarding Ricardo's suggestion --- I was thinking of adding a control flag
> > (yes, there are a few bits available) to tell how to round the value. The
> > user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or
> > previous) control value by incrementing the current value and setting the
> > appropriate flag. This is out of the scope of this set though.
> 
> This approach sounds really useful to me. Having control over the
> rounding would allow reading supported control values with reasonable
> effort. With such an approach, a very sparsely populated control becomes
> feasible and with integer64 controls that'd likely allow representing
> most exponential controls with linear values. If going this route, I
> don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.

Yes, I think the flag can be dropped as I suggested.

> 
> Thus, I think that control over the rounding is tightly related to this
> patchset and needs to be discussed together.

It addresses some of the same problem area but the implementation is
orthogonal to this.

Providing that would probably make the base field less useful: the valid
control values could be enumerated by the user using TRY_EXT_CTRLS without
the need to tell the valid values are powers of e.g. two.

I don't really have a strong opinion on that actually when it comes to the
API itself. The imx208 driver could proceed to use linear relation between
the control value and the digital gain. My worry is just the driver
implementation: this may not be entirely trivial. There's still no way to
address this problem in a generic way otherwise.

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH 0/5] Add units to controls

[Helmut Grohne]

On Tue, Sep 25, 2018 at 02:30:31PM +0200, Sakari Ailus wrote:
> On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote:
> > 1. There are a number of controls whose value is not easily described as
> >    either linear or exponential. I'm faced with at least two controls
> >    that actually are floating point numbers. One with two bits for the
> >    exponent and one (strange) bit for the mantissa (no joke) and another
> >    with three bits for the exponent and four bits for the mantissa.
> >    Neither can suitably be represented.

> The proposal does not address all potential situations, that's true.
> There's no way to try to represent everything out there (without
> enumerating the values) in an easily generalised way but something can be
> done.

Sure, but the rounding control flag would address that. The rounding
control flag could also solve exponential controls. Since there is a
certain overlap here. These proposals should be discussed together. We
should avoid solving problems twice.

> There are devices such as some flash LED controllers where the flash current
> if simply a value you can pick from the list. It's currently implemented as
> an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c .

Possibly, relaxing the "each control id has a fixed type" requirement is
another option. Allowing an integer menu wherever an integer is
specified could solve issues such as these in a different way.

Again it is fine that your proposal, does not cover that. Still these
uapi changes are interdependent and therefore need to be considered
together.

> The fact is that the unit is specific to hardware. The documentation
> documents something, and often suggests a unit, but if the hardware does
> something else, then that's what you get --- independently of what the
> documentation says.
> 
> Hence the need to convey it via the API.

That is vaguely convincing. It still seems like a niche case.

> Some controls could have the unit set by the framework if that makes sense.

I'd use a stronger s/could/should/ here as it directly translates into
maintenance cost.

> Most drivers shouldn't actually need to touch this if they're fine with
> defaults (whenever a control has a default).

Exactly this. Therefore I think you shouldn't update smiapp, but the
framework instead.

> A macro or two, it's not a major change. From the user space point of view,
> does it make a difference if a control has no unit or when it's not known
> what the unit is?

There are situations where there is a difference. If you count things
(e.g. reference pictures V4L2_CID_MPEG_VIDEO_MAX_REF_PIC), there is no
unit, but that's different from unknown. Having unknown separate from no
unit also helps with the transition period where controls lack units.

Therefore, i'm in favour of keeping the distinction between unknown and
no unit. Still, I don't think that merging them is fundamentally
"wrong".

> Yes, I think [V4L2_CTRL_FLAG_EXPONENTIAL] can be dropped as I suggested.

Ok. Looking forward to the rounding flag then.

> > Thus, I think that control over the rounding is tightly related to this
> > patchset and needs to be discussed together.
> 
> It addresses some of the same problem area but the implementation is
> orthogonal to this.

I don't disagree here. Still I think that the question "what should be
implemented" (not how) is dependent on those other problems.

> Providing that would probably make the base field less useful: the valid
> control values could be enumerated by the user using TRY_EXT_CTRLS without
> the need to tell the valid values are powers of e.g. two.

After dropping V4L2_CTRL_FLAG_EXPONENTIAL, the base field is
meaningless, no?

> I don't really have a strong opinion on that actually when it comes to the
> API itself. The imx208 driver could proceed to use linear relation between
> the control value and the digital gain. My worry is just the driver
> implementation: this may not be entirely trivial. There's still no way to
> address this problem in a generic way otherwise.

Yeah, I'm mostly looking at this from an uapi pov (as that is the one
that cannot be changed later) and have no good answer here. Allowing
integer menus for integers would be easy from a driver pov though.

Helmut

Re: [PATCH 4/5] v4l: controls: QUERY_EXT_CTRL support for base, prefix and unit

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 32186 bytes --]

Hi Sakari,

I love your patch! Perhaps something to improve:

[auto build test WARNING on linuxtv-media/master]
[also build test WARNING on v4.19-rc5 next-20180926]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/Sakari-Ailus/Add-units-to-controls/20180925-183703
base:   git://linuxtv.org/media_tree.git master
reproduce: make htmldocs

All warnings (new ones prefixed by >>):

   include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_ack_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.ampdu_len' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.antenna' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.tx_time' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.is_valid_ack_signal' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'status.status_driver_data' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'driver_rates' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'pad' not described in 'ieee80211_tx_info'
   include/net/mac80211.h:977: warning: Function parameter or member 'rate_driver_data' not described in 'ieee80211_tx_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'rx_stats_avg.chain_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.filtered' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.retry_count' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.lost_packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_tdls_pkt_time' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_retries' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.msdu_failed' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.last_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.ack_signal_filled' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'status_stats.avg_ack_signal' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.packets' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.bytes' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.last_rate' not described in 'sta_info'
   net/mac80211/sta_info.h:588: warning: Function parameter or member 'tx_stats.msdu' not described in 'sta_info'
   include/linux/mod_devicetable.h:763: warning: Function parameter or member 'driver_data' not described in 'typec_device_id'
   kernel/sched/fair.c:3371: warning: Function parameter or member 'flags' not described in 'attach_entity_load_avg'
   arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'i' description in 'arch_atomic_sub_and_test'
   arch/x86/include/asm/atomic.h:84: warning: Excess function parameter 'v' description in 'arch_atomic_sub_and_test'
   arch/x86/include/asm/atomic.h:96: warning: Excess function parameter 'v' description in 'arch_atomic_inc'
   arch/x86/include/asm/atomic.h:109: warning: Excess function parameter 'v' description in 'arch_atomic_dec'
   arch/x86/include/asm/atomic.h:124: warning: Excess function parameter 'v' description in 'arch_atomic_dec_and_test'
   arch/x86/include/asm/atomic.h:138: warning: Excess function parameter 'v' description in 'arch_atomic_inc_and_test'
   arch/x86/include/asm/atomic.h:153: warning: Excess function parameter 'i' description in 'arch_atomic_add_negative'
   arch/x86/include/asm/atomic.h:153: warning: Excess function parameter 'v' description in 'arch_atomic_add_negative'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_excl.active' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.cb' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.poll' not described in 'dma_buf'
   include/linux/dma-buf.h:304: warning: Function parameter or member 'cb_shared.active' not described in 'dma_buf'
   include/linux/dma-fence-array.h:54: warning: Function parameter or member 'work' not described in 'dma_fence_array'
   include/linux/gpio/driver.h:142: warning: Function parameter or member 'request_key' not described in 'gpio_irq_chip'
   include/linux/iio/hw-consumer.h:1: warning: no structured comments found
   include/linux/input/sparse-keymap.h:46: warning: Function parameter or member 'sw' not described in 'key_entry'
   drivers/pci/pci.c:218: warning: Excess function parameter 'p' description in 'pci_dev_str_match_path'
   include/linux/regulator/driver.h:227: warning: Function parameter or member 'resume' not described in 'regulator_ops'
   drivers/regulator/core.c:4479: warning: Excess function parameter 'state' description in 'regulator_suspend'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw0' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw1' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw2' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.esw3' not described in 'irb'
   arch/s390/include/asm/cio.h:245: warning: Function parameter or member 'esw.eadm' not described in 'irb'
   drivers/slimbus/stream.c:1: warning: no structured comments found
   drivers/target/target_core_device.c:1: warning: no structured comments found
   drivers/usb/dwc3/gadget.c:510: warning: Excess function parameter 'dwc' description in 'dwc3_gadget_start_config'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/bus.c:1: warning: no structured comments found
   drivers/usb/typec/bus.c:268: warning: Function parameter or member 'mode' not described in 'typec_match_altmode'
   drivers/usb/typec/class.c:1497: warning: Excess function parameter 'drvdata' description in 'typec_port_register_altmode'
   drivers/usb/typec/class.c:1: warning: no structured comments found
   include/linux/w1.h:281: warning: Function parameter or member 'of_match_table' not described in 'w1_family'
   fs/direct-io.c:257: warning: Excess function parameter 'offset' description in 'dio_complete'
   fs/file_table.c:1: warning: no structured comments found
   fs/libfs.c:477: warning: Excess function parameter 'available' description in 'simple_write_end'
   fs/posix_acl.c:646: warning: Function parameter or member 'inode' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'mode_p' not described in 'posix_acl_update_mode'
   fs/posix_acl.c:646: warning: Function parameter or member 'acl' not described in 'posix_acl_update_mode'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:183: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_read_lock'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:254: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_gfx'
   drivers/gpu/drm/amd/amdgpu/amdgpu_mn.c:302: warning: Function parameter or member 'blockable' not described in 'amdgpu_mn_invalidate_range_start_hsa'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2986: warning: Excess function parameter 'dev' description in 'amdgpu_vm_get_task_info'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2987: warning: Function parameter or member 'adev' not described in 'amdgpu_vm_get_task_info'
   drivers/gpu/drm/amd/amdgpu/amdgpu_vm.c:2987: warning: Excess function parameter 'dev' description in 'amdgpu_vm_get_task_info'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_pin' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_unpin' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_res_obj' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_get_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_import_sg_table' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vmap' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_vunmap' not described in 'drm_driver'
   include/drm/drm_drv.h:610: warning: Function parameter or member 'gem_prime_mmap' not described in 'drm_driver'
   include/drm/drm_panel.h:98: warning: Function parameter or member 'link' not described in 'drm_panel'
   drivers/gpu/drm/i915/i915_vma.h:49: warning: cannot understand function prototype: 'struct i915_vma '
   drivers/gpu/drm/i915/i915_vma.h:1: warning: no structured comments found
   drivers/gpu/drm/i915/intel_guc_fwif.h:553: warning: cannot understand function prototype: 'struct guc_log_buffer_state '
   drivers/gpu/drm/i915/i915_trace.h:1: warning: no structured comments found
>> include/media/v4l2-ctrls.h:242: warning: Function parameter or member 'base' not described in 'v4l2_ctrl'
>> include/media/v4l2-ctrls.h:242: warning: Function parameter or member 'unit' not described in 'v4l2_ctrl'
>> include/media/v4l2-ctrls.h:242: warning: Function parameter or member 'prefix' not described in 'v4l2_ctrl'
   include/linux/skbuff.h:860: warning: Function parameter or member 'dev_scratch' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'list' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'ip_defrag_offset' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'skb_mstamp' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member '__cloned_offset' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'head_frag' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member '__pkt_type_offset' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'encapsulation' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'encap_hdr_csum' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'csum_valid' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'csum_complete_sw' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'csum_level' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'inner_protocol_type' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'remcsum_offload' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'offload_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'offload_mr_fwd_mark' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'sender_cpu' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'reserved_tailroom' not described in 'sk_buff'
   include/linux/skbuff.h:860: warning: Function parameter or member 'inner_ipproto' not described in 'sk_buff'
   include/net/sock.h:238: warning: Function parameter or member 'skc_addrpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_portpair' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_ipv6only' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_net_refcnt' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_daddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_v6_rcv_saddr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_cookie' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_listener' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_dr' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_rcv_wnd' not described in 'sock_common'
   include/net/sock.h:238: warning: Function parameter or member 'skc_tw_rcv_nxt' not described in 'sock_common'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.rmem_alloc' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.len' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.head' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_backlog.tail' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_wq_raw' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'tcp_rtx_queue' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_route_forced_caps' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_txtime_report_errors' not described in 'sock'
   include/net/sock.h:509: warning: Function parameter or member 'sk_validate_xmit_skb' not described in 'sock'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'adj_list.upper' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'adj_list.lower' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'gso_partial_features' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'switchdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'l3mdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'xfrmdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'tlsdev_ops' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'name_assign_type' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'ieee802154_ptr' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'mpls_ptr' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'xdp_prog' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'gro_flush_timeout' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'nf_hooks_ingress' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member '____cacheline_aligned_in_smp' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'qdisc_hash' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'xps_cpus_map' not described in 'net_device'
   include/linux/netdevice.h:2018: warning: Function parameter or member 'xps_rxqs_map' not described in 'net_device'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(advertising' not described in 'phylink_link_state'
   include/linux/phylink.h:56: warning: Function parameter or member '__ETHTOOL_DECLARE_LINK_MODE_MASK(lp_advertising' not described in 'phylink_link_state'
   sound/soc/soc-core.c:2918: warning: Excess function parameter 'legacy_dai_naming' description in 'snd_soc_register_dais'
   Documentation/admin-guide/cgroup-v2.rst:1485: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/admin-guide/cgroup-v2.rst:1487: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/admin-guide/cgroup-v2.rst:1488: WARNING: Block quote ends without a blank line; unexpected unindent.
   Documentation/core-api/boot-time-mm.rst:78: ERROR: Error in "kernel-doc" directive:
   unknown option: "nodocs".

vim +242 include/media/v4l2-ctrls.h

8ac7a9493 Hans Verkuil          2012-09-07  107  
8c2721d57 Mauro Carvalho Chehab 2015-08-22  108  /**
8c2721d57 Mauro Carvalho Chehab 2015-08-22  109   * struct v4l2_ctrl - The control structure.
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  110   *
0996517cf Hans Verkuil          2010-08-01  111   * @node:	The list node.
77068d36d Hans Verkuil          2011-06-13  112   * @ev_subs:	The list of control event subscriptions.
0996517cf Hans Verkuil          2010-08-01  113   * @handler:	The handler that owns the control.
0996517cf Hans Verkuil          2010-08-01  114   * @cluster:	Point to start of cluster array.
0996517cf Hans Verkuil          2010-08-01  115   * @ncontrols:	Number of controls in cluster array.
0996517cf Hans Verkuil          2010-08-01  116   * @done:	Internal flag: set for each processed control.
2a863793b Hans Verkuil          2011-01-11  117   * @is_new:	Set when the user specified a new value for this control. It
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  118   *		is also set when called from v4l2_ctrl_handler_setup(). Drivers
2a863793b Hans Verkuil          2011-01-11  119   *		should never set this flag.
9ea1b7a4b Hans Verkuil          2014-01-17  120   * @has_changed: Set when the current value differs from the new value. Drivers
9ea1b7a4b Hans Verkuil          2014-01-17  121   *		should never use this flag.
0996517cf Hans Verkuil          2010-08-01  122   * @is_private: If set, then this control is private to its handler and it
0996517cf Hans Verkuil          2010-08-01  123   *		will not be added to any other handlers. Drivers can set
0996517cf Hans Verkuil          2010-08-01  124   *		this flag.
72d877cac Hans Verkuil          2011-06-10  125   * @is_auto:   If set, then this control selects whether the other cluster
72d877cac Hans Verkuil          2011-06-10  126   *		members are in 'automatic' mode or 'manual' mode. This is
72d877cac Hans Verkuil          2011-06-10  127   *		used for autogain/gain type clusters. Drivers should never
72d877cac Hans Verkuil          2011-06-10  128   *		set this flag directly.
d9a254715 Hans Verkuil          2014-06-12  129   * @is_int:    If set, then this control has a simple integer value (i.e. it
d9a254715 Hans Verkuil          2014-06-12  130   *		uses ctrl->val).
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  131   * @is_string: If set, then this control has type %V4L2_CTRL_TYPE_STRING.
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  132   * @is_ptr:	If set, then this control is an array and/or has type >=
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  133   *		%V4L2_CTRL_COMPOUND_TYPES
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  134   *		and/or has type %V4L2_CTRL_TYPE_STRING. In other words, &struct
d9a254715 Hans Verkuil          2014-06-12  135   *		v4l2_ext_control uses field p to point to the data.
998e76591 Hans Verkuil          2014-06-10  136   * @is_array: If set, then this control contains an N-dimensional array.
5626b8c75 Hans Verkuil          2011-08-26  137   * @has_volatiles: If set, then one or more members of the cluster are volatile.
5626b8c75 Hans Verkuil          2011-08-26  138   *		Drivers should never touch this flag.
8ac7a9493 Hans Verkuil          2012-09-07  139   * @call_notify: If set, then call the handler's notify function whenever the
8ac7a9493 Hans Verkuil          2012-09-07  140   *		control's value changes.
72d877cac Hans Verkuil          2011-06-10  141   * @manual_mode_value: If the is_auto flag is set, then this is the value
72d877cac Hans Verkuil          2011-06-10  142   *		of the auto control that determines if that control is in
72d877cac Hans Verkuil          2011-06-10  143   *		manual mode. So if the value of the auto control equals this
72d877cac Hans Verkuil          2011-06-10  144   *		value, then the whole cluster is in manual mode. Drivers should
72d877cac Hans Verkuil          2011-06-10  145   *		never set this flag directly.
0996517cf Hans Verkuil          2010-08-01  146   * @ops:	The control ops.
0176077a8 Hans Verkuil          2014-04-27  147   * @type_ops:	The control type ops.
0996517cf Hans Verkuil          2010-08-01  148   * @id:	The control ID.
0996517cf Hans Verkuil          2010-08-01  149   * @name:	The control name.
0996517cf Hans Verkuil          2010-08-01  150   * @type:	The control type.
0996517cf Hans Verkuil          2010-08-01  151   * @minimum:	The control's minimum value.
0996517cf Hans Verkuil          2010-08-01  152   * @maximum:	The control's maximum value.
0996517cf Hans Verkuil          2010-08-01  153   * @default_value: The control's default value.
0996517cf Hans Verkuil          2010-08-01  154   * @step:	The control's step value for non-menu controls.
20d88eef6 Hans Verkuil          2014-06-12  155   * @elems:	The number of elements in the N-dimensional array.
d9a254715 Hans Verkuil          2014-06-12  156   * @elem_size:	The size in bytes of the control.
20d88eef6 Hans Verkuil          2014-06-12  157   * @dims:	The size of each dimension.
20d88eef6 Hans Verkuil          2014-06-12  158   * @nr_of_dims:The number of dimensions in @dims.
0996517cf Hans Verkuil          2010-08-01  159   * @menu_skip_mask: The control's skip mask for menu controls. This makes it
0996517cf Hans Verkuil          2010-08-01  160   *		easy to skip menu items that are not valid. If bit X is set,
0996517cf Hans Verkuil          2010-08-01  161   *		then menu item X is skipped. Of course, this only works for
0996517cf Hans Verkuil          2010-08-01  162   *		menus with <= 32 menu items. There are no menus that come
0996517cf Hans Verkuil          2010-08-01  163   *		close to that number, so this is OK. Should we ever need more,
0996517cf Hans Verkuil          2010-08-01  164   *		then this will have to be extended to a u64 or a bit array.
0996517cf Hans Verkuil          2010-08-01  165   * @qmenu:	A const char * array for all menu items. Array entries that are
0996517cf Hans Verkuil          2010-08-01  166   *		empty strings ("") correspond to non-existing menu items (this
0996517cf Hans Verkuil          2010-08-01  167   *		is in addition to the menu_skip_mask above). The last entry
0996517cf Hans Verkuil          2010-08-01  168   *		must be NULL.
20139f185 Mauro Carvalho Chehab 2017-09-27  169   *		Used only if the @type is %V4L2_CTRL_TYPE_MENU.
20139f185 Mauro Carvalho Chehab 2017-09-27  170   * @qmenu_int:	A 64-bit integer array for with integer menu items.
20139f185 Mauro Carvalho Chehab 2017-09-27  171   *		The size of array must be equal to the menu size, e. g.:
20139f185 Mauro Carvalho Chehab 2017-09-27  172   *		:math:`ceil(\frac{maximum - minimum}{step}) + 1`.
20139f185 Mauro Carvalho Chehab 2017-09-27  173   *		Used only if the @type is %V4L2_CTRL_TYPE_INTEGER_MENU.
0996517cf Hans Verkuil          2010-08-01  174   * @flags:	The control's flags.
20139f185 Mauro Carvalho Chehab 2017-09-27  175   * @cur:	Structure to store the current value.
20139f185 Mauro Carvalho Chehab 2017-09-27  176   * @cur.val:	The control's current value, if the @type is represented via
20139f185 Mauro Carvalho Chehab 2017-09-27  177   *		a u32 integer (see &enum v4l2_ctrl_type).
0996517cf Hans Verkuil          2010-08-01  178   * @val:	The control's new s32 value.
0996517cf Hans Verkuil          2010-08-01  179   * @priv:	The control's private pointer. For use by the driver. It is
0996517cf Hans Verkuil          2010-08-01  180   *		untouched by the control framework. Note that this pointer is
0996517cf Hans Verkuil          2010-08-01  181   *		not freed when the control is deleted. Should this be needed
0996517cf Hans Verkuil          2010-08-01  182   *		then a new internal bitfield can be added to tell the framework
0996517cf Hans Verkuil          2010-08-01  183   *		to free this pointer.
bf7b70482 Baruch Siach          2018-07-05  184   * @p_cur:	The control's current value represented via a union which
7dc879190 Mauro Carvalho Chehab 2015-08-22  185   *		provides a standard way of accessing control types
7dc879190 Mauro Carvalho Chehab 2015-08-22  186   *		through a pointer.
bf7b70482 Baruch Siach          2018-07-05  187   * @p_new:	The control's new value represented via a union which provides
7dc879190 Mauro Carvalho Chehab 2015-08-22  188   *		a standard way of accessing control types
7dc879190 Mauro Carvalho Chehab 2015-08-22  189   *		through a pointer.
0996517cf Hans Verkuil          2010-08-01  190   */
0996517cf Hans Verkuil          2010-08-01  191  struct v4l2_ctrl {
0996517cf Hans Verkuil          2010-08-01  192  	/* Administrative fields */
0996517cf Hans Verkuil          2010-08-01  193  	struct list_head node;
77068d36d Hans Verkuil          2011-06-13  194  	struct list_head ev_subs;
0996517cf Hans Verkuil          2010-08-01  195  	struct v4l2_ctrl_handler *handler;
0996517cf Hans Verkuil          2010-08-01  196  	struct v4l2_ctrl **cluster;
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  197  	unsigned int ncontrols;
8ec4bee7c Mauro Carvalho Chehab 2016-07-22  198  
0996517cf Hans Verkuil          2010-08-01  199  	unsigned int done:1;
0996517cf Hans Verkuil          2010-08-01  200  
2a863793b Hans Verkuil          2011-01-11  201  	unsigned int is_new:1;
9ea1b7a4b Hans Verkuil          2014-01-17  202  	unsigned int has_changed:1;
0996517cf Hans Verkuil          2010-08-01  203  	unsigned int is_private:1;
72d877cac Hans Verkuil          2011-06-10  204  	unsigned int is_auto:1;
d9a254715 Hans Verkuil          2014-06-12  205  	unsigned int is_int:1;
d9a254715 Hans Verkuil          2014-06-12  206  	unsigned int is_string:1;
d9a254715 Hans Verkuil          2014-06-12  207  	unsigned int is_ptr:1;
998e76591 Hans Verkuil          2014-06-10  208  	unsigned int is_array:1;
5626b8c75 Hans Verkuil          2011-08-26  209  	unsigned int has_volatiles:1;
8ac7a9493 Hans Verkuil          2012-09-07  210  	unsigned int call_notify:1;
82a7c0494 Hans Verkuil          2011-06-28  211  	unsigned int manual_mode_value:8;
0996517cf Hans Verkuil          2010-08-01  212  
0996517cf Hans Verkuil          2010-08-01  213  	const struct v4l2_ctrl_ops *ops;
0176077a8 Hans Verkuil          2014-04-27  214  	const struct v4l2_ctrl_type_ops *type_ops;
0996517cf Hans Verkuil          2010-08-01  215  	u32 id;
0996517cf Hans Verkuil          2010-08-01  216  	const char *name;
0996517cf Hans Verkuil          2010-08-01  217  	enum v4l2_ctrl_type type;
0ba2aeb6d Hans Verkuil          2014-04-16  218  	s64 minimum, maximum, default_value;
20d88eef6 Hans Verkuil          2014-06-12  219  	u32 elems;
d9a254715 Hans Verkuil          2014-06-12  220  	u32 elem_size;
20d88eef6 Hans Verkuil          2014-06-12  221  	u32 dims[V4L2_CTRL_MAX_DIMS];
20d88eef6 Hans Verkuil          2014-06-12  222  	u32 nr_of_dims;
569dc4a72 Sakari Ailus          2018-09-25  223  	u8 base, unit;
569dc4a72 Sakari Ailus          2018-09-25  224  	u16 prefix;
0996517cf Hans Verkuil          2010-08-01  225  	union {
0ba2aeb6d Hans Verkuil          2014-04-16  226  		u64 step;
0ba2aeb6d Hans Verkuil          2014-04-16  227  		u64 menu_skip_mask;
0996517cf Hans Verkuil          2010-08-01  228  	};
ce580fe51 Sakari Ailus          2011-08-04  229  	union {
513521eae Hans Verkuil          2010-12-29  230  		const char * const *qmenu;
ce580fe51 Sakari Ailus          2011-08-04  231  		const s64 *qmenu_int;
ce580fe51 Sakari Ailus          2011-08-04  232  	};
0996517cf Hans Verkuil          2010-08-01  233  	unsigned long flags;
d9a254715 Hans Verkuil          2014-06-12  234  	void *priv;
0996517cf Hans Verkuil          2010-08-01  235  	s32 val;
2a9ec3731 Hans Verkuil          2014-04-27  236  	struct {
0996517cf Hans Verkuil          2010-08-01  237  		s32 val;
d9a254715 Hans Verkuil          2014-06-12  238  	} cur;
0176077a8 Hans Verkuil          2014-04-27  239  
0176077a8 Hans Verkuil          2014-04-27  240  	union v4l2_ctrl_ptr p_new;
0176077a8 Hans Verkuil          2014-04-27  241  	union v4l2_ctrl_ptr p_cur;
0996517cf Hans Verkuil          2010-08-01 @242  };
0996517cf Hans Verkuil          2010-08-01  243  

:::::: The code at line 242 was first introduced by commit
:::::: 0996517cf8eaded69b8502c8f5abeb8cec62b6d4 V4L/DVB: v4l2: Add new control handling framework

:::::: TO: Hans Verkuil <hverkuil@xs4all.nl>
:::::: CC: Mauro Carvalho Chehab <mchehab@redhat.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 6584 bytes --]

Re: [PATCH 1/5] videodev2.h: Use 8 hexadecimals (32 bits) for control flags

[Laurent Pinchart]

Hi Sakari,

Thank you for the patch.

On Tuesday, 25 September 2018 13:14:30 EEST Sakari Ailus wrote:
> The V4L2 control flags are a 32-bit bitmask. Use 32-bit hexadecimal
> numbers to specify the flags (was 16).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

This is the easy one,

Reviewed-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 22 ++++++++++----------
>  include/uapi/linux/videodev2.h                    | 22 ++++++++++----------
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst index
> 5bd26e8c9a1a0..ff2d131223b84 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -439,37 +439,37 @@ See also the examples in :ref:`control`.
> 
>      :widths:       3 1 4
> 
>      * - ``V4L2_CTRL_FLAG_DISABLED``
> -      - 0x0001
> +      - 0x00000001
>        - This control is permanently disabled and should be ignored by the
>  	application. Any attempt to change the control will result in an
>  	``EINVAL`` error code.
>      * - ``V4L2_CTRL_FLAG_GRABBED``
> -      - 0x0002
> +      - 0x00000002
>        - This control is temporarily unchangeable, for example because
>  	another application took over control of the respective resource.
>  	Such controls may be displayed specially in a user interface.
>  	Attempts to change the control may result in an ``EBUSY`` error code.
>      * - ``V4L2_CTRL_FLAG_READ_ONLY``
> -      - 0x0004
> +      - 0x00000004
>        - This control is permanently readable only. Any attempt to change
>  	the control will result in an ``EINVAL`` error code.
>      * - ``V4L2_CTRL_FLAG_UPDATE``
> -      - 0x0008
> +      - 0x00000008
>        - A hint that changing this control may affect the value of other
>  	controls within the same control class. Applications should update
>  	their user interface accordingly.
>      * - ``V4L2_CTRL_FLAG_INACTIVE``
> -      - 0x0010
> +      - 0x00000010
>        - This control is not applicable to the current configuration and
>  	should be displayed accordingly in a user interface. For example
>  	the flag may be set on a MPEG audio level 2 bitrate control when
>  	MPEG audio encoding level 1 was selected with another control.
>      * - ``V4L2_CTRL_FLAG_SLIDER``
> -      - 0x0020
> +      - 0x00000020
>        - A hint that this control is best represented as a slider-like
>  	element in a user interface.
>      * - ``V4L2_CTRL_FLAG_WRITE_ONLY``
> -      - 0x0040
> +      - 0x00000040
>        - This control is permanently writable only. Any attempt to read the
>  	control will result in an ``EACCES`` error code error code. This flag
>  	is typically present for relative controls or action controls
> @@ -477,7 +477,7 @@ See also the examples in :ref:`control`.
>  	action (e. g. motor control) but no meaningful value can be
>  	returned.
>      * - ``V4L2_CTRL_FLAG_VOLATILE``
> -      - 0x0080
> +      - 0x00000080
>        - This control is volatile, which means that the value of the
>  	control changes continuously. A typical example would be the
>  	current gain value if the device is in auto-gain mode. In such a
> @@ -493,7 +493,7 @@ See also the examples in :ref:`control`.
>  	   Setting a new value for a volatile control will *never* trigger a
> 
>  	   :ref:`V4L2_EVENT_CTRL_CH_VALUE <ctrl-changes-flags>` event.
> 
>      * - ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> -      - 0x0100
> +      - 0x00000100
>        - This control has a pointer type, so its value has to be accessed
>  	using one of the pointer fields of struct
> 
>  	:c:type:`v4l2_ext_control`. This flag is set
> 
> @@ -503,7 +503,7 @@ See also the examples in :ref:`control`.
>      * .. _FLAG_EXECUTE_ON_WRITE:
> 
>        - ``V4L2_CTRL_FLAG_EXECUTE_ON_WRITE``
> -      - 0x0200
> +      - 0x00000200
>        - The value provided to the control will be propagated to the driver
>  	even if it remains constant. This is required when the control
>  	represents an action on the hardware. For example: clearing an
> @@ -512,7 +512,7 @@ See also the examples in :ref:`control`.
>      * .. _FLAG_MODIFY_LAYOUT:
> 
>        - ``V4L2_CTRL_FLAG_MODIFY_LAYOUT``
> -      - 0x0400
> +      - 0x00000400
>        - Changing this control value may modify the layout of the
>          buffer (for video devices) or the media bus format (for
> sub-devices).
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 184e4dbe8f9c0..ae083978988f1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1681,17 +1681,17 @@ struct v4l2_querymenu {
>  } __attribute__ ((packed));
> 
>  /*  Control flags  */
> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> -#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
> -#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
> -#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
> +#define V4L2_CTRL_FLAG_DISABLED		0x00000001
> +#define V4L2_CTRL_FLAG_GRABBED		0x00000002
> +#define V4L2_CTRL_FLAG_READ_ONLY	0x00000004
> +#define V4L2_CTRL_FLAG_UPDATE		0x00000008
> +#define V4L2_CTRL_FLAG_INACTIVE		0x00000010
> +#define V4L2_CTRL_FLAG_SLIDER		0x00000020
> +#define V4L2_CTRL_FLAG_WRITE_ONLY	0x00000040
> +#define V4L2_CTRL_FLAG_VOLATILE		0x00000080
> +#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x00000100
> +#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x00000200
> +#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x00000400
> 
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000


-- 
Regards,

Laurent Pinchart

Re: [PATCH 0/5] Add units to controls

[Laurent Pinchart]

Hi Sakari,

On Tuesday, 25 September 2018 15:30:31 EEST Sakari Ailus wrote:
> On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote:
> > On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
> >> This set adds a few things to the current control framework in terms of
> >> what kind of information the user space may have on controls. It adds
> >> support for units and prefixes, exponential base as well as information
> >> on whether a control is linear or exponential, to struct
> >> v4l2_query_ext_ctrl.
> > 
> > That's a great improvement. Being able to give meaning to controls is
> > great. However, I see two significant weaknesses in the approach being
> > taken:
> > 
> > 1. There are a number of controls whose value is not easily described as
> >    either linear or exponential. I'm faced with at least two controls
> >    that actually are floating point numbers. One with two bits for the
> >    exponent and one (strange) bit for the mantissa (no joke) and another
> >    with three bits for the exponent and four bits for the mantissa.
> >    Neither can suitably be represented.
> >    
> >    Since the value ranges are small for the cases I mentioned, I looked
> >    into using an integer menu. The present approach does not allow for
> >    replacing an integer with an integer menu though. Each control id has
> >    a fixed type. I'm not sure how to solve this.
> 
> The proposal does not address all potential situations, that's true.
> There's no way to try to represent everything out there (without
> enumerating the values) in an easily generalised way but something can be
> done.

[1]

> There are devices such as some flash LED controllers where the flash current
> if simply a value you can pick from the list. It's currently implemented as
> an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c .
> 
> > 2. The present implementation places the responsibility of assigning
> >    units and scales into drivers. A number of controls (e.g.
> >    V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
> >    V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the
> >    documentation. Having each and every driver set units and scales in
> >    the documented way will lead to duplication and buggy code. Having
> >    each driver choose unit and scale will lead to inconsistency. It
> >    would be very good to have a mechanism that puts the framework in
> >    charge of maintaining units and scales for the standard control ids.
> >    
> >    What is a consumer supposed to do with a control that changes unit?
> >    The algorithm expected e.g. a duration. It might be able to convert
> >    to pixels, but what should it do if it gets back amperes? I argue
> >    that the unit should be a property of the control id and be
> >    documented (i.e. what is done now). If it is reported via an ioctl,
> 
> The fact is that the unit is specific to hardware. The documentation
> documents something, and often suggests a unit, but if the hardware does
> something else, then that's what you get --- independently of what the
> documentation says.
> 
> Hence the need to convey it via the API.
> 
> Some controls could have the unit set by the framework if that makes sense.
> Most drivers shouldn't actually need to touch this if they're fine with
> defaults (whenever a control has a default).

I have to agree with Helmut here. If we don't handle the 95% of the common 
cases in the core, we'll never get it right. Drivers will be buggy in many 
ways. Which leads me to a related comment, we need v4l2-compliance support for 
this, to verify that each control reports valid information.

> >    the framework needs to be in charge of setting it.
> >    
> >    The story is much different for scales. Scaling the value up and down
> >    is something consumers can reasonably be expected to do. It allows
> >    shifting the value range such that the relevant values can be fully
> >    represented. Allowing drivers to change scales is much more
> >    reasonable. Still the framework should provide a default such that
> >    most drivers should not need any update.
> > 
> > I acknowledge that these expectations are high and see that they're
> > partially covered in your later remarks.
> > 
> >> The smiapp driver gains support for the feature. In the near term, some
> >> controls could also be assigned the unit automatically. The pixel rate,
> >> for instance. Fewer driver changes would be needed this way. A driver
> >> could override the value if there's a need to.
> > 
> > I believe that in the interest of keeping maintenance cost low, this
> > should happen rather sooner than later. Just even adding the support to
> > smiapp seems wrong when it would be possible to have the framework do
> > the work.
> > 
> >> I think I'll merge the undefined and no unit cases. Same for the
> >> exponential base actually --- the flag can be removed, too...
> > 
> > I'm not sure I understand. It reads like you are going to revert a
> > significant part of the patch.
> 
> A macro or two, it's not a major change. From the user space point of view,
> does it make a difference if a control has no unit or when it's not known
> what the unit is?
> 
> >> Regarding Ricardo's suggestion --- I was thinking of adding a control
> >> flag (yes, there are a few bits available) to tell how to round the
> >> value. The user could use the TRY_EXT_CTRLS IOCTL to figure out the next
> >> (or previous) control value by incrementing the current value and
> >> setting the appropriate flag. This is out of the scope of this set
> >> though.
> > 
> > This approach sounds really useful to me. Having control over the
> > rounding would allow reading supported control values with reasonable
> > effort.

That would result in way too many ioctl calls though. If we design an API to 
report all valid values, we have to do it through a single ioctl call. I'm 
however not opposed to rounding flags, as there could be a separate use for 
that, but I'm concerned that drivers will not use them correctly and 
consistently if we don't implement support for them in the core (and for any 
integer control that can't be accurately represented by min, max and step, I 
don't think we can).

I'm also concerned about over-engineering a solution for a problem that I'm 
still not convinced does exist. Applications needing precise information about 
control will very likely be device-aware in my opinion (due to [1]). I'm open 
to debating this, in which case I would ask for real use cases, and a real 
implementation (in something else than a test program).

V4L2 is bloated with micro-features that have been badly designed and that are 
mostly unused, let's try to get it right this time.

> > With such an approach, a very sparsely populated control becomes
> > feasible and with integer64 controls that'd likely allow representing
> > most exponential controls with linear values. If going this route, I
> > don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.
> 
> Yes, I think the flag can be dropped as I suggested.
> 
> > Thus, I think that control over the rounding is tightly related to this
> > patchset and needs to be discussed together.
> 
> It addresses some of the same problem area but the implementation is
> orthogonal to this.
> 
> Providing that would probably make the base field less useful: the valid
> control values could be enumerated by the user using TRY_EXT_CTRLS without
> the need to tell the valid values are powers of e.g. two.
> 
> I don't really have a strong opinion on that actually when it comes to the
> API itself. The imx208 driver could proceed to use linear relation between
> the control value and the digital gain. My worry is just the driver
> implementation: this may not be entirely trivial. There's still no way to
> address this problem in a generic way otherwise.

-- 
Regards,

Laurent Pinchart

Re: [PATCH 1/5] videodev2.h: Use 8 hexadecimals (32 bits) for control flags

[Hans Verkuil]

On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> The V4L2 control flags are a 32-bit bitmask. Use 32-bit hexadecimal
> numbers to specify the flags (was 16).
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Regards,

	Hans

> ---
>  Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 22 +++++++++++-----------
>  include/uapi/linux/videodev2.h                    | 22 +++++++++++-----------
>  2 files changed, 22 insertions(+), 22 deletions(-)
> 
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index 5bd26e8c9a1a0..ff2d131223b84 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -439,37 +439,37 @@ See also the examples in :ref:`control`.
>      :widths:       3 1 4
>  
>      * - ``V4L2_CTRL_FLAG_DISABLED``
> -      - 0x0001
> +      - 0x00000001
>        - This control is permanently disabled and should be ignored by the
>  	application. Any attempt to change the control will result in an
>  	``EINVAL`` error code.
>      * - ``V4L2_CTRL_FLAG_GRABBED``
> -      - 0x0002
> +      - 0x00000002
>        - This control is temporarily unchangeable, for example because
>  	another application took over control of the respective resource.
>  	Such controls may be displayed specially in a user interface.
>  	Attempts to change the control may result in an ``EBUSY`` error code.
>      * - ``V4L2_CTRL_FLAG_READ_ONLY``
> -      - 0x0004
> +      - 0x00000004
>        - This control is permanently readable only. Any attempt to change
>  	the control will result in an ``EINVAL`` error code.
>      * - ``V4L2_CTRL_FLAG_UPDATE``
> -      - 0x0008
> +      - 0x00000008
>        - A hint that changing this control may affect the value of other
>  	controls within the same control class. Applications should update
>  	their user interface accordingly.
>      * - ``V4L2_CTRL_FLAG_INACTIVE``
> -      - 0x0010
> +      - 0x00000010
>        - This control is not applicable to the current configuration and
>  	should be displayed accordingly in a user interface. For example
>  	the flag may be set on a MPEG audio level 2 bitrate control when
>  	MPEG audio encoding level 1 was selected with another control.
>      * - ``V4L2_CTRL_FLAG_SLIDER``
> -      - 0x0020
> +      - 0x00000020
>        - A hint that this control is best represented as a slider-like
>  	element in a user interface.
>      * - ``V4L2_CTRL_FLAG_WRITE_ONLY``
> -      - 0x0040
> +      - 0x00000040
>        - This control is permanently writable only. Any attempt to read the
>  	control will result in an ``EACCES`` error code error code. This flag
>  	is typically present for relative controls or action controls
> @@ -477,7 +477,7 @@ See also the examples in :ref:`control`.
>  	action (e. g. motor control) but no meaningful value can be
>  	returned.
>      * - ``V4L2_CTRL_FLAG_VOLATILE``
> -      - 0x0080
> +      - 0x00000080
>        - This control is volatile, which means that the value of the
>  	control changes continuously. A typical example would be the
>  	current gain value if the device is in auto-gain mode. In such a
> @@ -493,7 +493,7 @@ See also the examples in :ref:`control`.
>  	   Setting a new value for a volatile control will *never* trigger a
>  	   :ref:`V4L2_EVENT_CTRL_CH_VALUE <ctrl-changes-flags>` event.
>      * - ``V4L2_CTRL_FLAG_HAS_PAYLOAD``
> -      - 0x0100
> +      - 0x00000100
>        - This control has a pointer type, so its value has to be accessed
>  	using one of the pointer fields of struct
>  	:c:type:`v4l2_ext_control`. This flag is set
> @@ -503,7 +503,7 @@ See also the examples in :ref:`control`.
>      * .. _FLAG_EXECUTE_ON_WRITE:
>  
>        - ``V4L2_CTRL_FLAG_EXECUTE_ON_WRITE``
> -      - 0x0200
> +      - 0x00000200
>        - The value provided to the control will be propagated to the driver
>  	even if it remains constant. This is required when the control
>  	represents an action on the hardware. For example: clearing an
> @@ -512,7 +512,7 @@ See also the examples in :ref:`control`.
>      * .. _FLAG_MODIFY_LAYOUT:
>  
>        - ``V4L2_CTRL_FLAG_MODIFY_LAYOUT``
> -      - 0x0400
> +      - 0x00000400
>        - Changing this control value may modify the layout of the
>          buffer (for video devices) or the media bus format (for sub-devices).
>  
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 184e4dbe8f9c0..ae083978988f1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1681,17 +1681,17 @@ struct v4l2_querymenu {
>  } __attribute__ ((packed));
>  
>  /*  Control flags  */
> -#define V4L2_CTRL_FLAG_DISABLED		0x0001
> -#define V4L2_CTRL_FLAG_GRABBED		0x0002
> -#define V4L2_CTRL_FLAG_READ_ONLY	0x0004
> -#define V4L2_CTRL_FLAG_UPDATE		0x0008
> -#define V4L2_CTRL_FLAG_INACTIVE		0x0010
> -#define V4L2_CTRL_FLAG_SLIDER		0x0020
> -#define V4L2_CTRL_FLAG_WRITE_ONLY	0x0040
> -#define V4L2_CTRL_FLAG_VOLATILE		0x0080
> -#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x0100
> -#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x0200
> -#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x0400
> +#define V4L2_CTRL_FLAG_DISABLED		0x00000001
> +#define V4L2_CTRL_FLAG_GRABBED		0x00000002
> +#define V4L2_CTRL_FLAG_READ_ONLY	0x00000004
> +#define V4L2_CTRL_FLAG_UPDATE		0x00000008
> +#define V4L2_CTRL_FLAG_INACTIVE		0x00000010
> +#define V4L2_CTRL_FLAG_SLIDER		0x00000020
> +#define V4L2_CTRL_FLAG_WRITE_ONLY	0x00000040
> +#define V4L2_CTRL_FLAG_VOLATILE		0x00000080
> +#define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x00000100
> +#define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x00000200
> +#define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x00000400
>  
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> 

Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Hans Verkuil]

On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Add support for exponential bases, prefixes as well as units for V4L2
> controls. This makes it possible to convey information on the relation
> between the control value and the hardware feature being controlled.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++-
>  1 file changed, 31 insertions(+), 1 deletion(-)
> 
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index ae083978988f1..23b02f2db85a1 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
>  	__u32		     reserved[2];
>  };
>  
> +/* V4L2 control exponential bases */
> +#define V4L2_CTRL_BASE_UNDEFINED	0
> +#define V4L2_CTRL_BASE_LINEAR		1

I'm not really sure you need BASE_LINEAR. That is effectively the same
as UNDEFINED since what else can you do? It's also weird to have this
as 'base' if the EXPONENTIAL flag is set.

I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
then you know the exponential base.

> +#define V4L2_CTRL_BASE_2		2
> +#define V4L2_CTRL_BASE_10		10
> +
> +/* V4L2 control unit prefixes */
> +#define V4L2_CTRL_PREFIX_NANO		-9
> +#define V4L2_CTRL_PREFIX_MICRO		-6
> +#define V4L2_CTRL_PREFIX_MILLI		-3
> +#define V4L2_CTRL_PREFIX_1		0

I would prefer PREFIX_NONE, since there is no prefix in this case.

I assume this prefix is only valid if the unit is not UNDEFINED and not
NONE?

Is 'base' also dependent on a valid unit? (it doesn't appear to be)

> +#define V4L2_CTRL_PREFIX_KILO		3
> +#define V4L2_CTRL_PREFIX_MEGA		6
> +#define V4L2_CTRL_PREFIX_GIGA		9
> +
> +/* V4L2 control units */
> +#define V4L2_CTRL_UNIT_UNDEFINED	0
> +#define V4L2_CTRL_UNIT_NONE		1
> +#define V4L2_CTRL_UNIT_SECOND		2
> +#define V4L2_CTRL_UNIT_AMPERE		3
> +#define V4L2_CTRL_UNIT_LINE		4
> +#define V4L2_CTRL_UNIT_PIXEL		5
> +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC	6
> +#define V4L2_CTRL_UNIT_HZ		7
> +
> +
>  /*  Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
>  struct v4l2_query_ext_ctrl {
>  	__u32		     id;
> @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
>  	__u32                elems;
>  	__u32                nr_of_dims;
>  	__u32                dims[V4L2_CTRL_MAX_DIMS];
> -	__u32		     reserved[32];
> +	__u8		     base;
> +	__s8		     prefix;
> +	__u16		     unit;
> +	__u32		     reserved[31];
>  };
>  
>  /*  Used in the VIDIOC_QUERYMENU ioctl for querying menu items */
> @@ -1692,6 +1721,7 @@ struct v4l2_querymenu {
>  #define V4L2_CTRL_FLAG_HAS_PAYLOAD	0x00000100
>  #define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE	0x00000200
>  #define V4L2_CTRL_FLAG_MODIFY_LAYOUT	0x00000400
> +#define V4L2_CTRL_FLAG_EXPONENTIAL	0x00000800
>  
>  /*  Query flags, to be ORed with the control ID */
>  #define V4L2_CTRL_FLAG_NEXT_CTRL	0x80000000
> 

Regards,

	Hans

Re: [PATCH 3/5] Documentation: media: Document control exponential bases, units, prefixes

[Hans Verkuil]

On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Document V4L2 control exponential bases, units and prefixes, as well as
> the control flag telling a control value is an exponent.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  Documentation/media/uapi/v4l/extended-controls.rst |   2 +
>  Documentation/media/uapi/v4l/vidioc-queryctrl.rst  | 152 ++++++++++++++++++++-
>  Documentation/media/videodev2.h.rst.exceptions     |  22 +++
>  3 files changed, 175 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/media/uapi/v4l/extended-controls.rst b/Documentation/media/uapi/v4l/extended-controls.rst
> index 9f7312bf33651..8461fd92d1b9e 100644
> --- a/Documentation/media/uapi/v4l/extended-controls.rst
> +++ b/Documentation/media/uapi/v4l/extended-controls.rst
> @@ -3460,6 +3460,8 @@ Image Process Control IDs
>      by selecting the desired horizontal and vertical blanking. The unit
>      of this control is Hz.
>  
> +.. _v4l2_cid_pixel_rate:
> +
>  ``V4L2_CID_PIXEL_RATE (64-bit integer)``
>      Pixel rate in the source pads of the subdev. This control is
>      read-only and its unit is pixels / second.
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index ff2d131223b84..472378f5d7566 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -269,8 +269,22 @@ See also the examples in :ref:`control`.
>        - ``dims[V4L2_CTRL_MAX_DIMS]``
>        - The size of each dimension. The first ``nr_of_dims`` elements of
>  	this array must be non-zero, all remaining elements must be zero.
> +    * - __u8
> +      - ``base``
> +      - The exponential base of the control value. Valid only if the
> +	:ref:`V4L2_CTRL_FLAG_EXPONENTIAL <FLAG_EXPONENTIAL>` control flag is
> +	set. This is an enumeration.
> +    * - __u8
> +      - ``prefix``
> +      - Prefix of the unit. This is an enumeration.
> +    * - __u16
> +      - ``unit``
> +      - Unit of the value. Together with the ``prefix`` as well as the ``base``
> +	field (if :ref:`V4L2_CTRL_FLAG_EXPONENTIAL <FLAG_EXPONENTIAL>` is set),
> +	defines the relation between the control value and the property of the
> +	hardware being controlled. This is an enumeration.
>      * - __u32
> -      - ``reserved``\ [32]
> +      - ``reserved``\ [31]
>        - Reserved for future extensions. Applications and drivers must set
>  	the array to zero.
>  
> @@ -523,6 +537,142 @@ See also the examples in :ref:`control`.
>  	streaming is in progress since most drivers do not support changing
>  	the format in that case.
>  
> +    * .. _FLAG_EXPONENTIAL:
> +
> +      - ``V4L2_CTRL_FLAG_EXPONENTIAL``
> +      - 0x00000800
> +
> +      - The value of the control has an exponential relation to the feature
> +	being controled instead of a linear relation. In other words, the value

controled -> controlled

> +	of the control is an exponent of the base specified in the
> +        base field in &struct v4l2_query_ext_ctrl.

It would be nice if you can add some math here. See colorspaces-details.rst
for examples of that.

> +
> +
> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> +
> +.. _control-bases:
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: Control Exponential Bases
> +    :header-rows:  1
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * - Base Name
> +      - Value
> +      - Description
> +
> +    * - ``V4L2_CTRL_BASE_UNDEFINED``
> +      - 0
> +      - The control exponential base is not defined.
> +
> +    * - ``V4L2_CTRL_BASE_LINEAR``
> +      - 1
> +      - The control is linear.
> +
> +    * - ``V4L2_CTRL_BASE_2``
> +      - 2
> +      - The base of the control is 2.
> +
> +    * - ``V4L2_CTRL_BASE_10``
> +      - 10
> +      - The base of the control is 10.
> +
> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> +
> +.. _control-prefixes:
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: Control Prefixes
> +    :header-rows:  1
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * - Prefix Name
> +      - Value
> +      - Description
> +
> +    * - ``V4L2_CTRL_PREFIX_NANO``
> +      - -9
> +      - Nano
> +
> +    * - ``V4L2_CTRL_PREFIX_MICRO``
> +      - -6
> +      - Micro
> +
> +    * - ``V4L2_CTRL_PREFIX_MILLI``
> +      - -3
> +      - Milli
> +
> +    * - ``V4L2_CTRL_PREFIX_1``
> +      - 0
> +      - \-
> +
> +    * - ``V4L2_CTRL_PREFIX_KILO``
> +      - 3
> +      - Kilo
> +
> +    * - ``V4L2_CTRL_PREFIX_MEGA``
> +      - 6
> +      - Mega
> +
> +    * - ``V4L2_CTRL_PREFIX_GIGA``
> +      - 9
> +      - Giga
> +
> +.. tabularcolumns:: |p{6.6cm}|p{2.2cm}|p{8.7cm}|
> +
> +.. _control-units:
> +
> +.. cssclass:: longtable
> +
> +.. flat-table:: Control Units
> +    :header-rows:  1
> +    :stub-columns: 0
> +    :widths:       3 1 4
> +
> +    * - Unit Name
> +      - Value
> +      - Description
> +
> +    * - ``V4L2_CTRL_UNIT_UNDEFINED``
> +      - 0
> +      - The unit of the control is not defined.
> +
> +    * - ``V4L2_CTRL_UNIT_NONE``
> +      - 1
> +      - The control has no unit.
> +
> +    * - ``V4L2_CTRL_UNIT_SECOND``
> +      - 2
> +      - Second
> +
> +    * - ``V4L2_CTRL_UNIT_AMPERE``
> +      - 3
> +      - Ampère
> +
> +    * - ``V4L2_CTRL_UNIT_LINE``
> +      - 4
> +      - A line of pixels in sensor's pixel matrix. This is a unit of time
> +        commonly used by camera sensors in e.g. exposure control, i.e. the time
> +        it takes for a sensor to read a line of pixels from the sensor's pixel
> +	matrix. See :ref:`V4L2_CID_PIXEL_RATE <V4L2_CID_PIXEL_RATE>`.

That's a bad name. I'd call it a PIXEL_LINE or FRAME_LINE or something like that.
When I first saw the define I thought it applied to a gpio line or a bus line or
something like that.

> +
> +    * - ``V4L2_CTRL_UNIT_PIXEL``
> +      - 5
> +      - A pixel in sensor's pixel matrix. This is a unit of time commonly used
> +        by camera sensors in e.g. exposure control, i.e. the time it takes for
> +	a sensor to read a pixel from the sensor's pixel matrix.
> +
> +    * - ``V4L2_CTRL_UNIT_PIXEL``

Duplicate name as above. You meant PIXELS_PER_SECOND.

I use this technique these days to avoid having to add defines to the videodev2.h.rst.exceptions
file:

    * .. _`V4L2-CTRL-UNIT-PIXEL`:

      - ``V4L2_CTRL_UNIT_PIXEL``
      - 5
      - A pixel in sensor's pixel matrix. This is a unit of time commonly used
        by camera sensors in e.g. exposure control, i.e. the time it takes for
	a sensor to read a pixel from the sensor's pixel matrix.

> +      - 6
> +      - Pixels per second
> +
> +    * - ``V4L2_CTRL_UNIT_HZ
> +      - 7
> +      - Hertz
>  
>  Return Value
>  ============
> diff --git a/Documentation/media/videodev2.h.rst.exceptions b/Documentation/media/videodev2.h.rst.exceptions
> index 63fa131729c09..8183a4f3554b0 100644
> --- a/Documentation/media/videodev2.h.rst.exceptions
> +++ b/Documentation/media/videodev2.h.rst.exceptions
> @@ -343,11 +343,33 @@ replace define V4L2_CTRL_FLAG_VOLATILE control-flags
>  replace define V4L2_CTRL_FLAG_HAS_PAYLOAD control-flags
>  replace define V4L2_CTRL_FLAG_EXECUTE_ON_WRITE control-flags
>  replace define V4L2_CTRL_FLAG_MODIFY_LAYOUT control-flags
> +replace define V4L2_CTRL_FLAG_EXPONENTIAL control-flags
>  
>  replace define V4L2_CTRL_FLAG_NEXT_CTRL control
>  replace define V4L2_CTRL_FLAG_NEXT_COMPOUND control
>  replace define V4L2_CID_PRIVATE_BASE control
>  
> +# V4L2 control bases, prefixes and units
> +replace define V4L2_CTRL_BASE_UNDEFINED control-bases
> +replace define V4L2_CTRL_BASE_LINEAR control-bases
> +replace define V4L2_CTRL_BASE_2 control-bases
> +replace define V4L2_CTRL_BASE_10 control-bases
> +
> +replace define V4L2_CTRL_PREFIX_NANO control-prefixes
> +replace define V4L2_CTRL_PREFIX_MICRO control-prefixes
> +replace define V4L2_CTRL_PREFIX_MILLI control-prefixes
> +replace define V4L2_CTRL_PREFIX_1 control-prefixes
> +replace define V4L2_CTRL_PREFIX_KILO control-prefixes
> +replace define V4L2_CTRL_PREFIX_MEGA control-prefixes
> +replace define V4L2_CTRL_PREFIX_GIGA control-prefixes
> +
> +replace define V4L2_CTRL_UNIT_UNDEFINED control-units
> +replace define V4L2_CTRL_UNIT_NONE control-units
> +replace define V4L2_CTRL_UNIT_SECOND control-units
> +replace define V4L2_CTRL_UNIT_AMPERE control-units
> +replace define V4L2_CTRL_UNIT_LINE control-units
> +replace define V4L2_CTRL_UNIT_PIXEL control-units

You need to add PIXELS_PER_SECOND here as well.

> +
>  # V4L2 tuner
>  
>  replace define V4L2_TUNER_CAP_LOW tuner-capability
> 

Regards,

	Hans

Re: [PATCH 4/5] v4l: controls: QUERY_EXT_CTRL support for base, prefix and unit

[Hans Verkuil]

On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Add support for conveying the information set by the driver to the user
> space.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 3 +++
>  include/media/v4l2-ctrls.h           | 2 ++
>  2 files changed, 5 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index ee006d34c19f0..8d2931b0a4701 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2732,6 +2732,9 @@ int v4l2_query_ext_ctrl(struct v4l2_ctrl_handler *hdl, struct v4l2_query_ext_ctr
>  	qc->minimum = ctrl->minimum;
>  	qc->maximum = ctrl->maximum;
>  	qc->default_value = ctrl->default_value;
> +	qc->base = ctrl->base;
> +	qc->prefix = ctrl->prefix;
> +	qc->unit = ctrl->unit;
>  	if (ctrl->type == V4L2_CTRL_TYPE_MENU
>  	    || ctrl->type == V4L2_CTRL_TYPE_INTEGER_MENU)
>  		qc->step = 1;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index f615ba1b29dd9..d6aaf45b70381 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -220,6 +220,8 @@ struct v4l2_ctrl {
>  	u32 elem_size;
>  	u32 dims[V4L2_CTRL_MAX_DIMS];
>  	u32 nr_of_dims;
> +	u8 base, unit;
> +	u16 prefix;
>  	union {
>  		u64 step;
>  		u64 menu_skip_mask;
> 

You need to add base/unit/prefix to struct v4l2_ctrl_config as well.

Regards,

	Hans

Re: [PATCH 5/5] smiapp: Set control units

[Hans Verkuil]

On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> Assign units for the controls exposed by the smiapp driver.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> ---
>  drivers/media/i2c/smiapp/smiapp-core.c | 16 +++++++++-------
>  1 file changed, 9 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/media/i2c/smiapp/smiapp-core.c b/drivers/media/i2c/smiapp/smiapp-core.c
> index 99f3b295ae3c7..289313c232430 100644
> --- a/drivers/media/i2c/smiapp/smiapp-core.c
> +++ b/drivers/media/i2c/smiapp/smiapp-core.c
> @@ -562,17 +562,10 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
>  	sensor->vblank = v4l2_ctrl_new_std(
>  		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
>  		V4L2_CID_VBLANK, 0, 1, 1, 0);
> -
> -	if (sensor->vblank)
> -		sensor->vblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> -
>  	sensor->hblank = v4l2_ctrl_new_std(
>  		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
>  		V4L2_CID_HBLANK, 0, 1, 1, 0);
>  
> -	if (sensor->hblank)
> -		sensor->hblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> -
>  	sensor->pixel_rate_parray = v4l2_ctrl_new_std(
>  		&sensor->pixel_array->ctrl_handler, &smiapp_ctrl_ops,
>  		V4L2_CID_PIXEL_RATE, 1, INT_MAX, 1, 1);
> @@ -589,6 +582,13 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
>  		return sensor->pixel_array->ctrl_handler.error;
>  	}
>  
> +	sensor->exposure->unit = V4L2_CTRL_UNIT_LINE;
> +	sensor->vblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> +	sensor->vblank->unit = V4L2_CTRL_UNIT_LINE;

You can fill in the unit for this control in v4l2_ctrl_fill(). That means that this
function gets extra arguments, but that's OK.

There are probably quite a few controls were you can be explicit about the unit.

BTW, it might be easier to add a new v4l2_ctrl_fill_units() function, I'm not sure
what is best.

> +	sensor->hblank->flags |= V4L2_CTRL_FLAG_UPDATE;
> +	sensor->hblank->unit = V4L2_CTRL_UNIT_PIXEL;
> +	sensor->pixel_rate_parray->unit = V4L2_CTRL_UNIT_PIXELS_PER_SEC;
> +
>  	sensor->pixel_array->sd.ctrl_handler =
>  		&sensor->pixel_array->ctrl_handler;
>  
> @@ -611,6 +611,8 @@ static int smiapp_init_controls(struct smiapp_sensor *sensor)
>  		return sensor->src->ctrl_handler.error;
>  	}
>  
> +	sensor->pixel_rate_csi->unit = V4L2_CTRL_UNIT_PIXELS_PER_SEC;
> +
>  	sensor->src->sd.ctrl_handler = &sensor->src->ctrl_handler;
>  
>  	return 0;
> 

Regards,

	Hans

Re: [PATCH 0/5] Add units to controls

[Hans Verkuil]

On 09/28/2018 03:25 PM, Laurent Pinchart wrote:
> Hi Sakari,
> 
> On Tuesday, 25 September 2018 15:30:31 EEST Sakari Ailus wrote:
>> On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote:
>>> On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
>>>> This set adds a few things to the current control framework in terms of
>>>> what kind of information the user space may have on controls. It adds
>>>> support for units and prefixes, exponential base as well as information
>>>> on whether a control is linear or exponential, to struct
>>>> v4l2_query_ext_ctrl.
>>>
>>> That's a great improvement. Being able to give meaning to controls is
>>> great. However, I see two significant weaknesses in the approach being
>>> taken:
>>>
>>> 1. There are a number of controls whose value is not easily described as
>>>    either linear or exponential. I'm faced with at least two controls
>>>    that actually are floating point numbers. One with two bits for the
>>>    exponent and one (strange) bit for the mantissa (no joke) and another
>>>    with three bits for the exponent and four bits for the mantissa.
>>>    Neither can suitably be represented.
>>>    
>>>    Since the value ranges are small for the cases I mentioned, I looked
>>>    into using an integer menu. The present approach does not allow for
>>>    replacing an integer with an integer menu though. Each control id has
>>>    a fixed type. I'm not sure how to solve this.
>>
>> The proposal does not address all potential situations, that's true.
>> There's no way to try to represent everything out there (without
>> enumerating the values) in an easily generalised way but something can be
>> done.
> 
> [1]
> 
>> There are devices such as some flash LED controllers where the flash current
>> if simply a value you can pick from the list. It's currently implemented as
>> an integer control. AFAIR the driver is drivers/leds/leds-aat1290.c .
>>
>>> 2. The present implementation places the responsibility of assigning
>>>    units and scales into drivers. A number of controls (e.g.
>>>    V4L2_CID_EXPOSURE_ABSOLUTE, V4L2_CID_AUDIO_LIMITER_RELEASE_TIME,
>>>    V4L2_CID_PIXEL_RATE, ...) clearly state the scale and unit in the
>>>    documentation. Having each and every driver set units and scales in
>>>    the documented way will lead to duplication and buggy code. Having
>>>    each driver choose unit and scale will lead to inconsistency. It
>>>    would be very good to have a mechanism that puts the framework in
>>>    charge of maintaining units and scales for the standard control ids.
>>>    
>>>    What is a consumer supposed to do with a control that changes unit?
>>>    The algorithm expected e.g. a duration. It might be able to convert
>>>    to pixels, but what should it do if it gets back amperes? I argue
>>>    that the unit should be a property of the control id and be
>>>    documented (i.e. what is done now). If it is reported via an ioctl,
>>
>> The fact is that the unit is specific to hardware. The documentation
>> documents something, and often suggests a unit, but if the hardware does
>> something else, then that's what you get --- independently of what the
>> documentation says.
>>
>> Hence the need to convey it via the API.
>>
>> Some controls could have the unit set by the framework if that makes sense.
>> Most drivers shouldn't actually need to touch this if they're fine with
>> defaults (whenever a control has a default).
> 
> I have to agree with Helmut here. If we don't handle the 95% of the common 
> cases in the core, we'll never get it right. Drivers will be buggy in many 
> ways. Which leads me to a related comment, we need v4l2-compliance support for 
> this, to verify that each control reports valid information.

Indeed.

> 
>>>    the framework needs to be in charge of setting it.
>>>    
>>>    The story is much different for scales. Scaling the value up and down
>>>    is something consumers can reasonably be expected to do. It allows
>>>    shifting the value range such that the relevant values can be fully
>>>    represented. Allowing drivers to change scales is much more
>>>    reasonable. Still the framework should provide a default such that
>>>    most drivers should not need any update.
>>>
>>> I acknowledge that these expectations are high and see that they're
>>> partially covered in your later remarks.
>>>
>>>> The smiapp driver gains support for the feature. In the near term, some
>>>> controls could also be assigned the unit automatically. The pixel rate,
>>>> for instance. Fewer driver changes would be needed this way. A driver
>>>> could override the value if there's a need to.
>>>
>>> I believe that in the interest of keeping maintenance cost low, this
>>> should happen rather sooner than later. Just even adding the support to
>>> smiapp seems wrong when it would be possible to have the framework do
>>> the work.
>>>
>>>> I think I'll merge the undefined and no unit cases. Same for the
>>>> exponential base actually --- the flag can be removed, too...
>>>
>>> I'm not sure I understand. It reads like you are going to revert a
>>> significant part of the patch.
>>
>> A macro or two, it's not a major change. From the user space point of view,
>> does it make a difference if a control has no unit or when it's not known
>> what the unit is?
>>
>>>> Regarding Ricardo's suggestion --- I was thinking of adding a control
>>>> flag (yes, there are a few bits available) to tell how to round the
>>>> value. The user could use the TRY_EXT_CTRLS IOCTL to figure out the next
>>>> (or previous) control value by incrementing the current value and
>>>> setting the appropriate flag. This is out of the scope of this set
>>>> though.
>>>
>>> This approach sounds really useful to me. Having control over the
>>> rounding would allow reading supported control values with reasonable
>>> effort.
> 
> That would result in way too many ioctl calls though. If we design an API to 
> report all valid values, we have to do it through a single ioctl call. I'm 
> however not opposed to rounding flags, as there could be a separate use for 
> that, but I'm concerned that drivers will not use them correctly and 
> consistently if we don't implement support for them in the core (and for any 
> integer control that can't be accurately represented by min, max and step, I 
> don't think we can).

We have rounding defines for the selection API as well: almost nobody uses them,
and they are hard to maintain and to verify. I can't say I am enthusiastic
about adding a rounding flag. Unless this can all be implemented in the core
together with v4l2-compliance checks to prevent regressions.

> 
> I'm also concerned about over-engineering a solution for a problem that I'm 
> still not convinced does exist. Applications needing precise information about 
> control will very likely be device-aware in my opinion (due to [1]). I'm open 
> to debating this, in which case I would ask for real use cases, and a real 
> implementation (in something else than a test program).
> 
> V4L2 is bloated with micro-features that have been badly designed and that are 
> mostly unused, let's try to get it right this time.
> 
>>> With such an approach, a very sparsely populated control becomes
>>> feasible and with integer64 controls that'd likely allow representing
>>> most exponential controls with linear values. If going this route, I
>>> don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.
>>
>> Yes, I think the flag can be dropped as I suggested.
>>
>>> Thus, I think that control over the rounding is tightly related to this
>>> patchset and needs to be discussed together.
>>
>> It addresses some of the same problem area but the implementation is
>> orthogonal to this.
>>
>> Providing that would probably make the base field less useful: the valid
>> control values could be enumerated by the user using TRY_EXT_CTRLS without
>> the need to tell the valid values are powers of e.g. two.

Another concern I have about the base field is how likely it is that it
actually follows the correct curve. Most gain or whatever curves depend on
the hardware details and are never perfect.

>>
>> I don't really have a strong opinion on that actually when it comes to the
>> API itself. The imx208 driver could proceed to use linear relation between
>> the control value and the digital gain. My worry is just the driver
>> implementation: this may not be entirely trivial. There's still no way to
>> address this problem in a generic way otherwise.
> 

Regards,

	Hans

Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Helmut Grohne]

On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote:
> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> > +/* V4L2 control unit prefixes */
> > +#define V4L2_CTRL_PREFIX_NANO		-9
> > +#define V4L2_CTRL_PREFIX_MICRO		-6
> > +#define V4L2_CTRL_PREFIX_MILLI		-3
> > +#define V4L2_CTRL_PREFIX_1		0
> 
> I would prefer PREFIX_NONE, since there is no prefix in this case.
> 
> I assume this prefix is only valid if the unit is not UNDEFINED and not
> NONE?

Why should it? The prefix is concerned with rescaling a value prior to
presenting it to a user. Even a unitless quantity or a value of
undefined unit can be reasonably scaled. Displaying a unit and scaling
look like orthogonal concepts to me.

> Is 'base' also dependent on a valid unit? (it doesn't appear to be)

I'd argue it should not depend on a valid unit like the prefix.

Helmut

Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Hans Verkuil]

On 10/01/2018 11:27 AM, Helmut Grohne wrote:
> On Fri, Sep 28, 2018 at 04:00:17PM +0200, Hans Verkuil wrote:
>> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
>>> +/* V4L2 control unit prefixes */
>>> +#define V4L2_CTRL_PREFIX_NANO		-9
>>> +#define V4L2_CTRL_PREFIX_MICRO		-6
>>> +#define V4L2_CTRL_PREFIX_MILLI		-3
>>> +#define V4L2_CTRL_PREFIX_1		0
>>
>> I would prefer PREFIX_NONE, since there is no prefix in this case.
>>
>> I assume this prefix is only valid if the unit is not UNDEFINED and not
>> NONE?
> 
> Why should it? The prefix is concerned with rescaling a value prior to
> presenting it to a user. Even a unitless quantity or a value of
> undefined unit can be reasonably scaled. Displaying a unit and scaling
> look like orthogonal concepts to me.

What's the point? If I have a unit-less control with values 1-1000, then
what would a prefix 'milli' tell me as a user? Why would 0.001-1 be better
compared to 1-1000?

Without a unit it is just an integer range and scaling is meaningless.

> 
>> Is 'base' also dependent on a valid unit? (it doesn't appear to be)
> 
> I'd argue it should not depend on a valid unit like the prefix.

I think I agree with that, although I am dubious about the value of the
base field as I commented on elsewhere.

Regards,

	Hans

Re: [PATCH 0/5] Add units to controls

[Tomasz Figa]

On Tue, Sep 25, 2018 at 9:30 PM Sakari Ailus
<sakari.ailus@linux.intel.com> wrote:
[snip]
> On Tue, Sep 25, 2018 at 01:48:02PM +0200, Helmut Grohne wrote:
> > On Tue, Sep 25, 2018 at 12:14:29PM +0200, Sakari Ailus wrote:
[snip]
> > > Regarding Ricardo's suggestion --- I was thinking of adding a control flag
> > > (yes, there are a few bits available) to tell how to round the value. The
> > > user could use the TRY_EXT_CTRLS IOCTL to figure out the next (or
> > > previous) control value by incrementing the current value and setting the
> > > appropriate flag. This is out of the scope of this set though.
> >
> > This approach sounds really useful to me. Having control over the
> > rounding would allow reading supported control values with reasonable
> > effort. With such an approach, a very sparsely populated control becomes
> > feasible and with integer64 controls that'd likely allow representing
> > most exponential controls with linear values. If going this route, I
> > don't see an application of V4L2_CTRL_FLAG_EXPONENTIAL.
>
> Yes, I think the flag can be dropped as I suggested.

Wouldn't that be just a duplicate of menu controls? Integer controls
are supposed to be described by min, max and step and any value
matching those should be valid.

Rather than introducing such tricky semantics, perhaps it would make
more sense to allow controls to be of different type, depending on the
driver? A driver that supports a contiguous range, would report the
control as INTEGER, while one that doesn't, would report it as
INTEGER_MENU.

Putting that aside, V4L2_CTRL_FLAG_EXPONENTIAL would actually make it
easier to enumerate the supported values to the userspace. Just one
QUERYCTRL would be needed, instead of 1 ioctl for each possible value.
(Although for the exponential case I wouldn't expect too many values
indeed...)

>
> >
> > Thus, I think that control over the rounding is tightly related to this
> > patchset and needs to be discussed together.
>
> It addresses some of the same problem area but the implementation is
> orthogonal to this.
>
> Providing that would probably make the base field less useful: the valid
> control values could be enumerated by the user using TRY_EXT_CTRLS without
> the need to tell the valid values are powers of e.g. two.
>
> I don't really have a strong opinion on that actually when it comes to the
> API itself. The imx208 driver could proceed to use linear relation between
> the control value and the digital gain. My worry is just the driver
> implementation: this may not be entirely trivial. There's still no way to
> address this problem in a generic way otherwise.

What's not trivial for the imx208 driver? It just registers an integer
control with a range from 0 to 4 and takes (1 << ctrl->val) as the
value for the hardware.

Best regards,
Tomasz

Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Pavel Machek]

Hi!

> Add support for exponential bases, prefixes as well as units for V4L2
> controls. This makes it possible to convey information on the relation
> between the control value and the hardware feature being controlled.
> 
> Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>

Sounds good.

> +/* V4L2 control exponential bases */
> +#define V4L2_CTRL_BASE_UNDEFINED	0
> +#define V4L2_CTRL_BASE_LINEAR		1
> +#define V4L2_CTRL_BASE_2		2
> +#define V4L2_CTRL_BASE_10		10

Do we also want to support logarithmic and 1/x? 

For example focus is usually in diopters and thats 1/meter...

> +/* V4L2 control units */
> +#define V4L2_CTRL_UNIT_UNDEFINED	0
> +#define V4L2_CTRL_UNIT_NONE		1
> +#define V4L2_CTRL_UNIT_SECOND		2
> +#define V4L2_CTRL_UNIT_AMPERE		3
> +#define V4L2_CTRL_UNIT_LINE		4
> +#define V4L2_CTRL_UNIT_PIXEL		5
> +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC	6
> +#define V4L2_CTRL_UNIT_HZ		7

And Hz is 1/second.

Should we also have some support for ISO-sensitivity and f/ aperture numbers?


Thanks,
								Pavel

Re: [PATCH 2/5] v4l: controls: Add support for exponential bases, prefixes and units

[Tomasz Figa]

Hi Sakari,

On Fri, Sep 28, 2018 at 11:00 PM Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> On 09/25/2018 12:14 PM, Sakari Ailus wrote:
> > Add support for exponential bases, prefixes as well as units for V4L2
> > controls. This makes it possible to convey information on the relation
> > between the control value and the hardware feature being controlled.
> >

Sorry for being late to the party.

Thanks for the series. I think it has a potential to be very useful.

Please see my comments below.

> > Signed-off-by: Sakari Ailus <sakari.ailus@linux.intel.com>
> > ---
> >  include/uapi/linux/videodev2.h | 32 +++++++++++++++++++++++++++++++-
> >  1 file changed, 31 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> > index ae083978988f1..23b02f2db85a1 100644
> > --- a/include/uapi/linux/videodev2.h
> > +++ b/include/uapi/linux/videodev2.h
> > @@ -1652,6 +1652,32 @@ struct v4l2_queryctrl {
> >       __u32                reserved[2];
> >  };
> >
> > +/* V4L2 control exponential bases */
> > +#define V4L2_CTRL_BASE_UNDEFINED     0
> > +#define V4L2_CTRL_BASE_LINEAR                1
>
> I'm not really sure you need BASE_LINEAR. That is effectively the same
> as UNDEFINED since what else can you do? It's also weird to have this
> as 'base' if the EXPONENTIAL flag is set.
>
> I don't see why you need the EXPONENTIAL flag at all: if this is non-0,
> then you know the exponential base.

Or vice versa, we could remove UNDEFINED and LINEAR altogether and
have the EXPONENTIAL flag actually signify the presence of a valid
base? Besides that, "linear exponential base" just doesn't sound right
or am I missing some basic maths? ;)

Then we could actually have a LOGARITHMIC flag and it could reuse the
same bases enum.

>
> > +#define V4L2_CTRL_BASE_2             2
> > +#define V4L2_CTRL_BASE_10            10
> > +
> > +/* V4L2 control unit prefixes */
> > +#define V4L2_CTRL_PREFIX_NANO                -9
> > +#define V4L2_CTRL_PREFIX_MICRO               -6
> > +#define V4L2_CTRL_PREFIX_MILLI               -3
> > +#define V4L2_CTRL_PREFIX_1           0
>
> I would prefer PREFIX_NONE, since there is no prefix in this case.
>
> I assume this prefix is only valid if the unit is not UNDEFINED and not
> NONE?
>
> Is 'base' also dependent on a valid unit? (it doesn't appear to be)
>
> > +#define V4L2_CTRL_PREFIX_KILO                3
> > +#define V4L2_CTRL_PREFIX_MEGA                6
> > +#define V4L2_CTRL_PREFIX_GIGA                9
> > +
> > +/* V4L2 control units */
> > +#define V4L2_CTRL_UNIT_UNDEFINED     0
> > +#define V4L2_CTRL_UNIT_NONE          1

Hmm, what's the meaning of NONE? How does it differ from UNDEFINED?

> > +#define V4L2_CTRL_UNIT_SECOND                2
> > +#define V4L2_CTRL_UNIT_AMPERE                3
> > +#define V4L2_CTRL_UNIT_LINE          4
> > +#define V4L2_CTRL_UNIT_PIXEL         5
> > +#define V4L2_CTRL_UNIT_PIXELS_PER_SEC        6
> > +#define V4L2_CTRL_UNIT_HZ            7
> > +
> > +
> >  /*  Used in the VIDIOC_QUERY_EXT_CTRL ioctl for querying extended controls */
> >  struct v4l2_query_ext_ctrl {
> >       __u32                id;
> > @@ -1666,7 +1692,10 @@ struct v4l2_query_ext_ctrl {
> >       __u32                elems;
> >       __u32                nr_of_dims;
> >       __u32                dims[V4L2_CTRL_MAX_DIMS];
> > -     __u32                reserved[32];
> > +     __u8                 base;
> > +     __s8                 prefix;

Should we make those bigger just in case, or leave some reserved
fields around so we can make them bigger when we need it?

Best regards,
Tomasz