Hi all,
This patch series aims to add support for complex lens controllers in V4L2.
Complex lens controllers usually feature one focus lens and one (or more)
zoom lens(es), which are driven by motors (e.g., stepper motors). As a
consequence, a few crucial differences to simple lens controllers (such as
voice coil motor (VCM) drivers, which are already well supported in V4L2)
arise:
- Focus and zoom are slow.
Compared to a simple VCM driver, which reacts almost instantaneously, the
motors that control the lens groups may require some time to reach their
target position. Therefore, the control process in user space needs
to receive feedback on the current status of each lens group, such as the
current position and whether or not the lens group is moving. Patch 3/6
adds volatile and read-only status controls for each lens group.
- The velocity of focus and zoom can be selected.
In contrast to a simple VCM driver, the motors can move at different
velocities. Since the produced noise depends on the velocity, the
control process may want to optimize the chosen velocity. Also, some auto
focus algorithms use different velocities in different phases (e.g., a
coarse and fast scan vs. a slow and precise scan). Patch 4/6 adds speed
controls for the focus lens group and the zoom lens group.
- Calibration may be required.
Since moving mechanical parts are involved, calibration is most likely
necessary. Patch 5/6 introduces controls to control calibration procedures.
In the scope of calibration, the relation between the lens positions may be
fine-tuned. This requires the ability to control the individual lenses and
gather feedback on their current status. Patch 6/6 introduces a pair of
controls for five zoom lenses. (Five is a placeholder here. The most
complex objective we had at hand happened to feature five zoom lenses.)
On the user space side, it is envisaged that libcamera operates the newly
introduced controls. Please note that no tests with libcamera have been
carried out yet, the integration will be discussed after the first round of
feedback to this RFC.
Version 2 of this series include two new patches that fix mistakes in the
documentation of existing controls. These mistakes have been pointed out
during the review phase of the first iteration of this series.
Looking forward to your comments!
---
Changes in v2:
- add patch 1/6 that fixes unit description of V4L2_CID_FOCUS_ABSOLUTE
- add patch 2/6 that clarifies when to implement V4L2_CID_FOCUS_RELATIVE
- remove compound controls _STATUS (struct) and add controls _STATUS
(bitmask) and _CURRENT (integer) instead
- fix V4L2_CID_LENS_CALIB_STATUS documentation
- Link to v1: https://lore.kernel.org/r/[email protected]
---
Michael Riesch (6):
media: v4l2-ctrls: fix documentation of V4L2_CID_FOCUS_ABSOLUTE unit
media: v4l2-ctrls: clarify documentation of V4L2_CID_FOCUS_RELATIVE
media: v4l2-ctrls: add lens group status controls for zoom and focus
media: v4l2-ctrls: add lens group speed controls for zoom and focus
media: v4l2-ctrls: add lens calibration controls
media: v4l2-ctrls: add controls for individual zoom lenses
.../userspace-api/media/v4l/ext-ctrls-camera.rst | 129 ++++++++++++++++++++-
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 41 +++++++
include/uapi/linux/v4l2-controls.h | 41 +++++++
3 files changed, 208 insertions(+), 3 deletions(-)
---
base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
change-id: 20230406-feature-controls-lens-b85575d3443a
Best regards,
--
Michael Riesch <[email protected]>
The current unit description of the V4L2_CID_FOCUS_ABSOLUTE does not
make sense and was probably copy-pasted from V4L2_CID_FOCUS_RELATIVE.
Fix the unit description in the documentation.
Signed-off-by: Michael Riesch <[email protected]>
---
Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index daa4f40869f8..df29150dce7b 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -140,8 +140,8 @@ enum v4l2_exposure_metering -
``V4L2_CID_FOCUS_ABSOLUTE (integer)``
This control sets the focal point of the camera to the specified
- position. The unit is undefined. Positive values set the focus
- closer to the camera, negative values towards infinity.
+ position. The unit is undefined. Larger values move the focus closer to
+ the camera, smaller values move the focus to infinity.
``V4L2_CID_FOCUS_RELATIVE (integer)``
This control moves the focal point of the camera by the specified
--
2.37.2
The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
Clarify this in the documentation.
Signed-off-by: Michael Riesch <[email protected]>
---
Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
1 file changed, 3 insertions(+), 1 deletion(-)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index df29150dce7b..42cf4c3cda0c 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
This control moves the focal point of the camera by the specified
amount. The unit is undefined. Positive values move the focus closer
to the camera, negative values towards infinity. This is a
- write-only control.
+ write-only control. It should be implemented only if the device cannot
+ handle absolute values.
+
``V4L2_CID_FOCUS_AUTO (boolean)``
Enables continuous automatic focus adjustments. The effect of manual
--
2.37.2
Add the controls V4L2_CID_{FOCUS,ZOOM}_{CURRENT,STATUS} that report the
current position and status, respectively, of the zoom lens group and
the focus lens group.
Signed-off-by: Michael Riesch <[email protected]>
---
.../userspace-api/media/v4l/ext-ctrls-camera.rst | 46 ++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 10 +++++
include/uapi/linux/v4l2-controls.h | 9 +++++
3 files changed, 65 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 42cf4c3cda0c..3ea4175f9619 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -150,6 +150,29 @@ enum v4l2_exposure_metering -
write-only control. It should be implemented only if the device cannot
handle absolute values.
+``V4L2_CID_FOCUS_CURRENT (integer)``
+ The current position of the focal point. The unit is undefined. Larger
+ values indicate that the focus is closer to the camera, smaller values
+ indicate that the focus is closer to infinity. This is a read-only control.
+
+``V4L2_CID_FOCUS_STATUS (bitmask)``
+ The status of the focus lens group. The possible flags are described in
+ the table below. This is a read-only control.
+
+.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - ``V4L2_LENS_STATUS_IDLE``
+ - Focus lens group is at rest.
+ * - ``V4L2_LENS_STATUS_BUSY``
+ - Focus lens group is moving.
+ * - ``V4L2_LENS_STATUS_FAILED``
+ - Focus lens group has failed to reach its target position. The driver
+ will not transition from this state until another action is performed
+ by an application.
``V4L2_CID_FOCUS_AUTO (boolean)``
Enables continuous automatic focus adjustments. The effect of manual
@@ -241,6 +264,29 @@ enum v4l2_auto_focus_range -
movement. A negative value moves the zoom lens group towards the
wide-angle direction. The zoom speed unit is driver-specific.
+``V4L2_CID_ZOOM_CURRENT (integer)``
+ The current objective lens focal length. The unit is undefined and
+ its value should be a positive integer. This is a read-only control.
+
+``V4L2_CID_ZOOM_STATUS (bitmask)``
+ The status of the zoom lens group. The possible flags are described in
+ the table below. This is a read-only control.
+
+.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - ``V4L2_LENS_STATUS_IDLE``
+ - Zoom lens group is at rest.
+ * - ``V4L2_LENS_STATUS_BUSY``
+ - Zoom lens group is moving.
+ * - ``V4L2_LENS_STATUS_FAILED``
+ - Zoom lens group has failed to reach its target position. The driver will
+ not transition from this state until another action is performed by an
+ application.
+
``V4L2_CID_IRIS_ABSOLUTE (integer)``
This control sets the camera's aperture to the specified value. The
unit is undefined. Larger values open the iris wider, smaller values
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 564fedee2c88..794ef3ab0c02 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1044,6 +1044,10 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
case V4L2_CID_HDR_SENSOR_MODE: return "HDR Sensor Mode";
+ case V4L2_CID_FOCUS_CURRENT: return "Focus, Current";
+ case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
+ case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
+ case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1593,6 +1597,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
break;
+ case V4L2_CID_FOCUS_CURRENT:
+ case V4L2_CID_FOCUS_STATUS:
+ case V4L2_CID_ZOOM_CURRENT:
+ case V4L2_CID_ZOOM_STATUS:
+ *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
+ break;
case V4L2_CID_FLASH_STROBE_STATUS:
case V4L2_CID_AUTO_FOCUS_STATUS:
case V4L2_CID_FLASH_READY:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 5e80daa4ffe0..793ee8c65e87 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -993,6 +993,15 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36)
+#define V4L2_LENS_STATUS_IDLE (0 << 0)
+#define V4L2_LENS_STATUS_BUSY (1 << 0)
+#define V4L2_LENS_STATUS_FAILED (1 << 2)
+
+#define V4L2_CID_FOCUS_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+37)
+#define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
+#define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
+#define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
+
/* FM Modulator class control IDs */
#define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
--
2.37.2
Add the controls V4L2_CID_LENS_CALIB_CONTROL and V4L2_CID_LENS_CALIB_STATUS
that facilitate the control of the lens group calibration procedure.
Signed-off-by: Michael Riesch <[email protected]>
---
.../userspace-api/media/v4l/ext-ctrls-camera.rst | 35 ++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++
include/uapi/linux/v4l2-controls.h | 12 ++++++++
3 files changed, 51 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index a17620ab03b9..8b54a0f3a617 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -297,6 +297,41 @@ enum v4l2_auto_focus_range -
(V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
driver-specific. The value should be a positive integer.
+``V4L2_CID_LENS_CALIB_CONTROL (bitmask)``
+ Control the calibration procedure (or individual parts thereof) of the lens
+ groups. For example, this could include the mechanical range detection
+ of zoom lens motors. This is a write-only control.
+
+.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - ``V4L2_LENS_CALIB_STOP``
+ - Stop the lens calibration procedure.
+ * - ``V4L2_LENS_CALIB_START``
+ - Start the complete lens calibration procedure.
+
+``V4L2_CID_LENS_CALIB_STATUS (bitmask)``
+ The status of the calibration procedure (or individual parts thereof) of
+ the lens groups. This is a read-only control.
+
+.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - ``V4L2_LENS_CALIB_IDLE``
+ - Lens calibration procedure has not yet been started.
+ * - ``V4L2_LENS_CALIB_BUSY``
+ - Lens calibration procedure is in progress.
+ * - ``V4L2_LENS_CALIB_COMPLETE``
+ - Lens calibration procedure is complete.
+ * - ``V4L2_LENS_CALIB_FAILED``
+ - Lens calibration procedure has failed.
+
``V4L2_CID_IRIS_ABSOLUTE (integer)``
This control sets the camera's aperture to the specified value. The
unit is undefined. Larger values open the iris wider, smaller values
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 3ef465ba73bd..faddfecba6d9 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1050,6 +1050,8 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
+ case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
+ case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1596,6 +1598,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_FOCUS_RELATIVE:
case V4L2_CID_IRIS_RELATIVE:
case V4L2_CID_ZOOM_RELATIVE:
+ case V4L2_CID_LENS_CALIB_CONTROL:
*flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
break;
@@ -1603,6 +1606,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_FOCUS_STATUS:
case V4L2_CID_ZOOM_CURRENT:
case V4L2_CID_ZOOM_STATUS:
+ case V4L2_CID_LENS_CALIB_STATUS:
*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
break;
case V4L2_CID_FLASH_STROBE_STATUS:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 8d84508d4db8..24c0eb5f4d29 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1004,6 +1004,18 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
#define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
+#define V4L2_LENS_CALIB_STOP (0 << 0)
+#define V4L2_LENS_CALIB_START (1 << 0)
+
+#define V4L2_CID_LENS_CALIB_CONTROL (V4L2_CID_CAMERA_CLASS_BASE+43)
+
+#define V4L2_LENS_CALIB_IDLE (0 << 0)
+#define V4L2_LENS_CALIB_BUSY (1 << 0)
+#define V4L2_LENS_CALIB_COMPLETE (1 << 1)
+#define V4L2_LENS_CALIB_FAILED (1 << 2)
+
+#define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
+
/* FM Modulator class control IDs */
#define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
--
2.37.2
A zoom lens group may consist of several lenses, and in a calibration
context it may be necessary to position the lenses individually. Add a
tuple of V4L2_CID_LENS_CALIB_ZOOMx_{ABSOLUTE,CURRENT,STATUS} controls
for each individual lens, where x = {1...5}.
Signed-off-by: Michael Riesch <[email protected]>
---
.../userspace-api/media/v4l/ext-ctrls-camera.rst | 30 ++++++++++++++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 25 ++++++++++++++++++
include/uapi/linux/v4l2-controls.h | 18 +++++++++++++
3 files changed, 73 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 8b54a0f3a617..21391f076971 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -332,6 +332,36 @@ enum v4l2_auto_focus_range -
* - ``V4L2_LENS_CALIB_FAILED``
- Lens calibration procedure has failed.
+``V4L2_CID_LENS_CALIB_ZOOM{1...5}_ABSOLUTE`` (integer)
+ Set the absolute position of the individual lens of the zoom lens group.
+ Most likely, this is done in a calibration context. The unit is
+ driver-specific.
+
+``V4L2_CID_LENS_CALIB_ZOOM{1...5}_CURRENT`` (integer)
+ The current absolute position of the individual lens of the zoom lens group.
+ Most likely, this is done in a calibration context. The unit is
+ driver-specific. This is a read-only control.
+
+``V4L2_CID_LENS_CALIB_ZOOM{1...5}_STATUS`` (bitmask)
+ The current status of the individual lens of the zoom lens group.
+ Most likely, this is done in a calibration context. The possible flags are
+ described in the table below. This is a read-only control.
+
+.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
+
+.. flat-table::
+ :header-rows: 0
+ :stub-columns: 0
+
+ * - ``V4L2_LENS_STATUS_IDLE``
+ - Zoom lens is at rest.
+ * - ``V4L2_LENS_STATUS_BUSY``
+ - Zoom lens is moving.
+ * - ``V4L2_LENS_STATUS_FAILED``
+ - Zoom lens has failed to reach its target position. The driver will
+ not transition from this state until another action is performed by an
+ application.
+
``V4L2_CID_IRIS_ABSOLUTE (integer)``
This control sets the camera's aperture to the specified value. The
unit is undefined. Larger values open the iris wider, smaller values
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index faddfecba6d9..8a78cffcd3e8 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1052,6 +1052,21 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
+ case V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE: return "Zoom1, Absolute";
+ case V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE: return "Zoom2, Absolute";
+ case V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE: return "Zoom3, Absolute";
+ case V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE: return "Zoom4, Absolute";
+ case V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE: return "Zoom5, Absolute";
+ case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT: return "Zoom1, Current";
+ case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT: return "Zoom1, Current";
+ case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT: return "Zoom1, Current";
+ case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT: return "Zoom1, Current";
+ case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT: return "Zoom1, Current";
+ case V4L2_CID_LENS_CALIB_ZOOM1_STATUS: return "Zoom1, Status";
+ case V4L2_CID_LENS_CALIB_ZOOM2_STATUS: return "Zoom2, Status";
+ case V4L2_CID_LENS_CALIB_ZOOM3_STATUS: return "Zoom3, Status";
+ case V4L2_CID_LENS_CALIB_ZOOM4_STATUS: return "Zoom4, Status";
+ case V4L2_CID_LENS_CALIB_ZOOM5_STATUS: return "Zoom5, Status";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1607,6 +1622,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
case V4L2_CID_ZOOM_CURRENT:
case V4L2_CID_ZOOM_STATUS:
case V4L2_CID_LENS_CALIB_STATUS:
+ case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:
+ case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:
+ case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:
+ case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:
+ case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:
+ case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:
+ case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:
+ case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:
+ case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:
+ case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:
*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
break;
case V4L2_CID_FLASH_STROBE_STATUS:
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 24c0eb5f4d29..7c49c0ba23d4 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1016,6 +1016,24 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
+#define V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+45)
+#define V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+46)
+#define V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+47)
+#define V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+48)
+#define V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+49)
+
+#define V4L2_CID_LENS_CALIB_ZOOM1_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+50)
+#define V4L2_CID_LENS_CALIB_ZOOM2_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+51)
+#define V4L2_CID_LENS_CALIB_ZOOM3_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+52)
+#define V4L2_CID_LENS_CALIB_ZOOM4_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+53)
+#define V4L2_CID_LENS_CALIB_ZOOM5_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+54)
+
+#define V4L2_CID_LENS_CALIB_ZOOM1_STATUS (V4L2_CID_CAMERA_CLASS_BASE+55)
+#define V4L2_CID_LENS_CALIB_ZOOM2_STATUS (V4L2_CID_CAMERA_CLASS_BASE+56)
+#define V4L2_CID_LENS_CALIB_ZOOM3_STATUS (V4L2_CID_CAMERA_CLASS_BASE+57)
+#define V4L2_CID_LENS_CALIB_ZOOM4_STATUS (V4L2_CID_CAMERA_CLASS_BASE+58)
+#define V4L2_CID_LENS_CALIB_ZOOM5_STATUS (V4L2_CID_CAMERA_CLASS_BASE+59)
+
/* FM Modulator class control IDs */
#define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
--
2.37.2
Add the controls V4L2_CID_FOCUS_SPEED and V4L2_CID_ZOOM_SPEED that set
the speed of the zoom lens group and focus lens group, respectively.
Signed-off-by: Michael Riesch <[email protected]>
---
Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
include/uapi/linux/v4l2-controls.h | 2 ++
3 files changed, 14 insertions(+)
diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
index 3ea4175f9619..a17620ab03b9 100644
--- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
+++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
@@ -174,6 +174,11 @@ enum v4l2_exposure_metering -
will not transition from this state until another action is performed
by an application.
+``V4L2_CID_FOCUS_SPEED (integer)``
+ Set the speed with which the focus lens group of the camera is moved
+ (V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_RELATIVE). The unit is
+ driver-specific. The value should be a positive integer.
+
``V4L2_CID_FOCUS_AUTO (boolean)``
Enables continuous automatic focus adjustments. The effect of manual
focus adjustments while this feature is enabled is undefined,
@@ -287,6 +292,11 @@ enum v4l2_auto_focus_range -
not transition from this state until another action is performed by an
application.
+``V4L2_CID_ZOOM_SPEED (integer)``
+ Set the speed with which the zoom lens group of the camera is moved
+ (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
+ driver-specific. The value should be a positive integer.
+
``V4L2_CID_IRIS_ABSOLUTE (integer)``
This control sets the camera's aperture to the specified value. The
unit is undefined. Larger values open the iris wider, smaller values
diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
index 794ef3ab0c02..3ef465ba73bd 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
@@ -1048,6 +1048,8 @@ const char *v4l2_ctrl_get_name(u32 id)
case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
+ case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
+ case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
/* FM Radio Modulator controls */
/* Keep the order of the 'case's the same as in v4l2-controls.h! */
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 793ee8c65e87..8d84508d4db8 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -1001,6 +1001,8 @@ enum v4l2_auto_focus_range {
#define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
#define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
#define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
+#define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
+#define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
/* FM Modulator class control IDs */
--
2.37.2
Hi Michael
Quoting Michael Riesch (2023-04-25 10:45:10)
> Hi all,
>
> This patch series aims to add support for complex lens controllers in V4L2.
> Complex lens controllers usually feature one focus lens and one (or more)
> zoom lens(es), which are driven by motors (e.g., stepper motors). As a
> consequence, a few crucial differences to simple lens controllers (such as
> voice coil motor (VCM) drivers, which are already well supported in V4L2)
> arise:
>
> - Focus and zoom are slow.
>
> Compared to a simple VCM driver, which reacts almost instantaneously, the
> motors that control the lens groups may require some time to reach their
> target position. Therefore, the control process in user space needs
> to receive feedback on the current status of each lens group, such as the
> current position and whether or not the lens group is moving. Patch 3/6
> adds volatile and read-only status controls for each lens group.
>
> - The velocity of focus and zoom can be selected.
>
> In contrast to a simple VCM driver, the motors can move at different
> velocities. Since the produced noise depends on the velocity, the
> control process may want to optimize the chosen velocity. Also, some auto
> focus algorithms use different velocities in different phases (e.g., a
> coarse and fast scan vs. a slow and precise scan). Patch 4/6 adds speed
> controls for the focus lens group and the zoom lens group.
>
> - Calibration may be required.
>
> Since moving mechanical parts are involved, calibration is most likely
> necessary. Patch 5/6 introduces controls to control calibration procedures.
>
> In the scope of calibration, the relation between the lens positions may be
> fine-tuned. This requires the ability to control the individual lenses and
> gather feedback on their current status. Patch 6/6 introduces a pair of
> controls for five zoom lenses. (Five is a placeholder here. The most
> complex objective we had at hand happened to feature five zoom lenses.)
I realise the 'five' is a bit arbitrary at the moment. Have you looked
at if it's possible to expose each lens as a distinct subdevice?
That would give full control over each one at least. I'm not sure yet
what the implications are with the 'group' though. I would expect then
the media graph could show links/connections sequentially between each
lens portraying the path that light travels through them. Are they
always linear? (or maybe the better word is sequential?) or do they
arrange in different orders ... I hope there's not something as complex
as:
1 -> 2 \
6
4 -> 5 /
>
> On the user space side, it is envisaged that libcamera operates the newly
> introduced controls. Please note that no tests with libcamera have been
> carried out yet, the integration will be discussed after the first round of
> feedback to this RFC.
If they are split into individual components then at least libcamera can
identify the arrangement from the media graph?
Will you expect applications to manage each lens manually? or would this
all be handled within libcamera (perhaps in some custom IPA?)
> Version 2 of this series include two new patches that fix mistakes in the
> documentation of existing controls. These mistakes have been pointed out
> during the review phase of the first iteration of this series.
>
> Looking forward to your comments!
>
> ---
> Changes in v2:
> - add patch 1/6 that fixes unit description of V4L2_CID_FOCUS_ABSOLUTE
> - add patch 2/6 that clarifies when to implement V4L2_CID_FOCUS_RELATIVE
> - remove compound controls _STATUS (struct) and add controls _STATUS
> (bitmask) and _CURRENT (integer) instead
> - fix V4L2_CID_LENS_CALIB_STATUS documentation
> - Link to v1: https://lore.kernel.org/r/[email protected]
>
> ---
> Michael Riesch (6):
> media: v4l2-ctrls: fix documentation of V4L2_CID_FOCUS_ABSOLUTE unit
> media: v4l2-ctrls: clarify documentation of V4L2_CID_FOCUS_RELATIVE
> media: v4l2-ctrls: add lens group status controls for zoom and focus
> media: v4l2-ctrls: add lens group speed controls for zoom and focus
> media: v4l2-ctrls: add lens calibration controls
> media: v4l2-ctrls: add controls for individual zoom lenses
>
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 129 ++++++++++++++++++++-
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 41 +++++++
> include/uapi/linux/v4l2-controls.h | 41 +++++++
> 3 files changed, 208 insertions(+), 3 deletions(-)
> ---
> base-commit: fe15c26ee26efa11741a7b632e9f23b01aca4cc6
> change-id: 20230406-feature-controls-lens-b85575d3443a
>
> Best regards,
> --
> Michael Riesch <[email protected]>
>
On 25/04/2023 11:45, Michael Riesch wrote:
> A zoom lens group may consist of several lenses, and in a calibration
> context it may be necessary to position the lenses individually. Add a
> tuple of V4L2_CID_LENS_CALIB_ZOOMx_{ABSOLUTE,CURRENT,STATUS} controls
> for each individual lens, where x = {1...5}.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 30 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 25 ++++++++++++++++++
> include/uapi/linux/v4l2-controls.h | 18 +++++++++++++
> 3 files changed, 73 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 8b54a0f3a617..21391f076971 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -332,6 +332,36 @@ enum v4l2_auto_focus_range -
> * - ``V4L2_LENS_CALIB_FAILED``
> - Lens calibration procedure has failed.
>
> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_ABSOLUTE`` (integer)
> + Set the absolute position of the individual lens of the zoom lens group.
> + Most likely, this is done in a calibration context. The unit is
> + driver-specific.
> +
> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_CURRENT`` (integer)
> + The current absolute position of the individual lens of the zoom lens group.
> + Most likely, this is done in a calibration context. The unit is
> + driver-specific. This is a read-only control.
> +
> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_STATUS`` (bitmask)
> + The current status of the individual lens of the zoom lens group.
> + Most likely, this is done in a calibration context. The possible flags are
> + described in the table below. This is a read-only control.
Wouldn't it be better to have this as an array control? That way the number of
lenses is set by the driver and can be easily read by userspace.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_STATUS_IDLE``
> + - Zoom lens is at rest.
> + * - ``V4L2_LENS_STATUS_BUSY``
> + - Zoom lens is moving.
> + * - ``V4L2_LENS_STATUS_FAILED``
> + - Zoom lens has failed to reach its target position. The driver will
> + not transition from this state until another action is performed by an
> + application.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index faddfecba6d9..8a78cffcd3e8 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1052,6 +1052,21 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
> case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
> case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
> + case V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE: return "Zoom1, Absolute";
> + case V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE: return "Zoom2, Absolute";
> + case V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE: return "Zoom3, Absolute";
> + case V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE: return "Zoom4, Absolute";
> + case V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE: return "Zoom5, Absolute";
> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT: return "Zoom1, Current";
> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT: return "Zoom1, Current";
> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT: return "Zoom1, Current";
> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT: return "Zoom1, Current";
> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT: return "Zoom1, Current";
You forget to update the number, it's all Zoom1 here.
> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS: return "Zoom1, Status";
> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS: return "Zoom2, Status";
> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS: return "Zoom3, Status";
> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS: return "Zoom4, Status";
> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS: return "Zoom5, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1607,6 +1622,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_ZOOM_CURRENT:
> case V4L2_CID_ZOOM_STATUS:
> case V4L2_CID_LENS_CALIB_STATUS:
> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:
> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:
> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:
> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:
> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:
> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:
> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:
> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:
> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:
> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:
> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
Same issue as for patch 3 w.r.t. VOLATILE.
> break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 24c0eb5f4d29..7c49c0ba23d4 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1016,6 +1016,24 @@ enum v4l2_auto_focus_range {
>
> #define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
>
> +#define V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+45)
> +#define V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+46)
> +#define V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+47)
> +#define V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+48)
> +#define V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+49)
> +
> +#define V4L2_CID_LENS_CALIB_ZOOM1_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+50)
> +#define V4L2_CID_LENS_CALIB_ZOOM2_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+51)
> +#define V4L2_CID_LENS_CALIB_ZOOM3_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+52)
> +#define V4L2_CID_LENS_CALIB_ZOOM4_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+53)
> +#define V4L2_CID_LENS_CALIB_ZOOM5_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+54)
> +
> +#define V4L2_CID_LENS_CALIB_ZOOM1_STATUS (V4L2_CID_CAMERA_CLASS_BASE+55)
> +#define V4L2_CID_LENS_CALIB_ZOOM2_STATUS (V4L2_CID_CAMERA_CLASS_BASE+56)
> +#define V4L2_CID_LENS_CALIB_ZOOM3_STATUS (V4L2_CID_CAMERA_CLASS_BASE+57)
> +#define V4L2_CID_LENS_CALIB_ZOOM4_STATUS (V4L2_CID_CAMERA_CLASS_BASE+58)
> +#define V4L2_CID_LENS_CALIB_ZOOM5_STATUS (V4L2_CID_CAMERA_CLASS_BASE+59)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
Disclaimer: I do not have enough domain knowledge to comment on if this is the
right solution or not. I can only comment on the control framework specifics.
Regards,
Hans
On 25/04/2023 11:45, Michael Riesch wrote:
> The current unit description of the V4L2_CID_FOCUS_ABSOLUTE does not
> make sense and was probably copy-pasted from V4L2_CID_FOCUS_RELATIVE.
> Fix the unit description in the documentation.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index daa4f40869f8..df29150dce7b 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -140,8 +140,8 @@ enum v4l2_exposure_metering -
>
> ``V4L2_CID_FOCUS_ABSOLUTE (integer)``
> This control sets the focal point of the camera to the specified
> - position. The unit is undefined. Positive values set the focus
> - closer to the camera, negative values towards infinity.
> + position. The unit is undefined. Larger values move the focus closer to
> + the camera, smaller values move the focus to infinity.
>
> ``V4L2_CID_FOCUS_RELATIVE (integer)``
> This control moves the focal point of the camera by the specified
>
Can you repost patches 1 and 2 separately (i.e. without the RFC tag)? This
seems reasonable to apply, so let's split this off from the remainder of
this series.
Regards,
Hans
On 25/04/2023 11:45, Michael Riesch wrote:
> Add the controls V4L2_CID_{FOCUS,ZOOM}_{CURRENT,STATUS} that report the
> current position and status, respectively, of the zoom lens group and
> the focus lens group.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 46 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 10 +++++
> include/uapi/linux/v4l2-controls.h | 9 +++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 42cf4c3cda0c..3ea4175f9619 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -150,6 +150,29 @@ enum v4l2_exposure_metering -
> write-only control. It should be implemented only if the device cannot
> handle absolute values.
>
> +``V4L2_CID_FOCUS_CURRENT (integer)``
> + The current position of the focal point. The unit is undefined. Larger
> + values indicate that the focus is closer to the camera, smaller values
> + indicate that the focus is closer to infinity. This is a read-only control.
> +
> +``V4L2_CID_FOCUS_STATUS (bitmask)``
> + The status of the focus lens group. The possible flags are described in
> + the table below. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_STATUS_IDLE``
> + - Focus lens group is at rest.
> + * - ``V4L2_LENS_STATUS_BUSY``
> + - Focus lens group is moving.
> + * - ``V4L2_LENS_STATUS_FAILED``
> + - Focus lens group has failed to reach its target position. The driver
> + will not transition from this state until another action is performed
> + by an application.
>
> ``V4L2_CID_FOCUS_AUTO (boolean)``
> Enables continuous automatic focus adjustments. The effect of manual
> @@ -241,6 +264,29 @@ enum v4l2_auto_focus_range -
> movement. A negative value moves the zoom lens group towards the
> wide-angle direction. The zoom speed unit is driver-specific.
>
> +``V4L2_CID_ZOOM_CURRENT (integer)``
> + The current objective lens focal length. The unit is undefined and
> + its value should be a positive integer. This is a read-only control.
> +
> +``V4L2_CID_ZOOM_STATUS (bitmask)``
> + The status of the zoom lens group. The possible flags are described in
> + the table below. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_STATUS_IDLE``
> + - Zoom lens group is at rest.
> + * - ``V4L2_LENS_STATUS_BUSY``
> + - Zoom lens group is moving.
> + * - ``V4L2_LENS_STATUS_FAILED``
> + - Zoom lens group has failed to reach its target position. The driver will
> + not transition from this state until another action is performed by an
> + application.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 564fedee2c88..794ef3ab0c02 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1044,6 +1044,10 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> case V4L2_CID_HDR_SENSOR_MODE: return "HDR Sensor Mode";
> + case V4L2_CID_FOCUS_CURRENT: return "Focus, Current";
> + case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
> + case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
> + case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1593,6 +1597,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> break;
> + case V4L2_CID_FOCUS_CURRENT:
> + case V4L2_CID_FOCUS_STATUS:
> + case V4L2_CID_ZOOM_CURRENT:
> + case V4L2_CID_ZOOM_STATUS:
> + *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
Drop the VOLATILE bit. It is up to the driver to set that.
If the hardware can keep the driver informed of position or status changes
(e.g. via an interrupt), then the driver can just update the control and
userspace is informed of the change through a control event.
If this is not possible, then userspace has to poll, and in that case VOLATILE
has to be set.
This looks good otherwise.
Regards,
Hans
> + break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> case V4L2_CID_AUTO_FOCUS_STATUS:
> case V4L2_CID_FLASH_READY:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 5e80daa4ffe0..793ee8c65e87 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -993,6 +993,15 @@ enum v4l2_auto_focus_range {
>
> #define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36)
>
> +#define V4L2_LENS_STATUS_IDLE (0 << 0)
> +#define V4L2_LENS_STATUS_BUSY (1 << 0)
> +#define V4L2_LENS_STATUS_FAILED (1 << 2)
> +
> +#define V4L2_CID_FOCUS_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+37)
> +#define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
> +#define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
> +#define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
On 25/04/2023 11:45, Michael Riesch wrote:
> Add the controls V4L2_CID_LENS_CALIB_CONTROL and V4L2_CID_LENS_CALIB_STATUS
> that facilitate the control of the lens group calibration procedure.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 35 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++
> include/uapi/linux/v4l2-controls.h | 12 ++++++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index a17620ab03b9..8b54a0f3a617 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -297,6 +297,41 @@ enum v4l2_auto_focus_range -
> (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
> driver-specific. The value should be a positive integer.
>
> +``V4L2_CID_LENS_CALIB_CONTROL (bitmask)``
> + Control the calibration procedure (or individual parts thereof) of the lens
> + groups. For example, this could include the mechanical range detection
> + of zoom lens motors. This is a write-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_STOP``
> + - Stop the lens calibration procedure.
> + * - ``V4L2_LENS_CALIB_START``
> + - Start the complete lens calibration procedure.
I don't like this as a bitmask control. Wouldn't it be better to have
two 'button' controls? One to start the calibration, one to stop?
Are more calibration control actions expected?
> +
> +``V4L2_CID_LENS_CALIB_STATUS (bitmask)``
> + The status of the calibration procedure (or individual parts thereof) of
> + the lens groups. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_IDLE``
> + - Lens calibration procedure has not yet been started.
> + * - ``V4L2_LENS_CALIB_BUSY``
> + - Lens calibration procedure is in progress.
> + * - ``V4L2_LENS_CALIB_COMPLETE``
> + - Lens calibration procedure is complete.
> + * - ``V4L2_LENS_CALIB_FAILED``
> + - Lens calibration procedure has failed.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 3ef465ba73bd..faddfecba6d9 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1050,6 +1050,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
> case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
> + case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
> + case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1596,6 +1598,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_RELATIVE:
> case V4L2_CID_IRIS_RELATIVE:
> case V4L2_CID_ZOOM_RELATIVE:
> + case V4L2_CID_LENS_CALIB_CONTROL:
> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> break;
> @@ -1603,6 +1606,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_STATUS:
> case V4L2_CID_ZOOM_CURRENT:
> case V4L2_CID_ZOOM_STATUS:
> + case V4L2_CID_LENS_CALIB_STATUS:
> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
It makes no sense that this is volatile. Besides the comments I made in patch 3/6, there
is also the fact that I would expect the device to provide some sort of interrupt to
indicate calibration status changes. Otherwise userspace would have to keep polling to
know when the calibration finishes, which would be a really poor hardware design.
Is this really the case for the hardware you are considering?
> break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 8d84508d4db8..24c0eb5f4d29 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1004,6 +1004,18 @@ enum v4l2_auto_focus_range {
> #define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
> #define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
>
> +#define V4L2_LENS_CALIB_STOP (0 << 0)
> +#define V4L2_LENS_CALIB_START (1 << 0)
> +
> +#define V4L2_CID_LENS_CALIB_CONTROL (V4L2_CID_CAMERA_CLASS_BASE+43)
> +
> +#define V4L2_LENS_CALIB_IDLE (0 << 0)
> +#define V4L2_LENS_CALIB_BUSY (1 << 0)
> +#define V4L2_LENS_CALIB_COMPLETE (1 << 1)
> +#define V4L2_LENS_CALIB_FAILED (1 << 2)
> +
> +#define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
Regards,
Hans
Hi Michael,
Thank you for the patch.
On Tue, Apr 25, 2023 at 11:45:11AM +0200, Michael Riesch wrote:
> The current unit description of the V4L2_CID_FOCUS_ABSOLUTE does not
> make sense and was probably copy-pasted from V4L2_CID_FOCUS_RELATIVE.
> Fix the unit description in the documentation.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 ++--
> 1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index daa4f40869f8..df29150dce7b 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -140,8 +140,8 @@ enum v4l2_exposure_metering -
>
> ``V4L2_CID_FOCUS_ABSOLUTE (integer)``
> This control sets the focal point of the camera to the specified
> - position. The unit is undefined. Positive values set the focus
> - closer to the camera, negative values towards infinity.
> + position. The unit is undefined. Larger values move the focus closer to
> + the camera, smaller values move the focus to infinity.
Reviewed-by: Laurent Pinchart <[email protected]>
>
> ``V4L2_CID_FOCUS_RELATIVE (integer)``
> This control moves the focal point of the camera by the specified
>
--
Regards,
Laurent Pinchart
Hi Michael,
Thank you for the patch.
On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> Clarify this in the documentation.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> 1 file changed, 3 insertions(+), 1 deletion(-)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index df29150dce7b..42cf4c3cda0c 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> This control moves the focal point of the camera by the specified
> amount. The unit is undefined. Positive values move the focus closer
> to the camera, negative values towards infinity. This is a
> - write-only control.
> + write-only control. It should be implemented only if the device cannot
> + handle absolute values.
> +
Extra blank line.
I don't think this is right. The control was added for the UVC driver,
and there are devices that implement both absolute and relative focus.
>
> ``V4L2_CID_FOCUS_AUTO (boolean)``
> Enables continuous automatic focus adjustments. The effect of manual
>
--
Regards,
Laurent Pinchart
Hi Michael
On Tue, 25 Apr 2023 at 10:45, Michael Riesch
<[email protected]> wrote:
>
> Add the controls V4L2_CID_LENS_CALIB_CONTROL and V4L2_CID_LENS_CALIB_STATUS
> that facilitate the control of the lens group calibration procedure.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 35 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++
> include/uapi/linux/v4l2-controls.h | 12 ++++++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index a17620ab03b9..8b54a0f3a617 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -297,6 +297,41 @@ enum v4l2_auto_focus_range -
> (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
> driver-specific. The value should be a positive integer.
>
> +``V4L2_CID_LENS_CALIB_CONTROL (bitmask)``
> + Control the calibration procedure (or individual parts thereof) of the lens
> + groups. For example, this could include the mechanical range detection
> + of zoom lens motors. This is a write-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_STOP``
> + - Stop the lens calibration procedure.
> + * - ``V4L2_LENS_CALIB_START``
> + - Start the complete lens calibration procedure.
> +
> +``V4L2_CID_LENS_CALIB_STATUS (bitmask)``
> + The status of the calibration procedure (or individual parts thereof) of
> + the lens groups. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_IDLE``
> + - Lens calibration procedure has not yet been started.
Idle as a term would generally encompass both COMPLETE and FAILED as
well as this case.
Use V4L2_LENS_CALIB_UNCABLIBRATED or similar instead?
> + * - ``V4L2_LENS_CALIB_BUSY``
> + - Lens calibration procedure is in progress.
> + * - ``V4L2_LENS_CALIB_COMPLETE``
> + - Lens calibration procedure is complete.
> + * - ``V4L2_LENS_CALIB_FAILED``
> + - Lens calibration procedure has failed.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 3ef465ba73bd..faddfecba6d9 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1050,6 +1050,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
> case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
> + case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
> + case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1596,6 +1598,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_RELATIVE:
> case V4L2_CID_IRIS_RELATIVE:
> case V4L2_CID_ZOOM_RELATIVE:
> + case V4L2_CID_LENS_CALIB_CONTROL:
> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> break;
> @@ -1603,6 +1606,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_STATUS:
> case V4L2_CID_ZOOM_CURRENT:
> case V4L2_CID_ZOOM_STATUS:
> + case V4L2_CID_LENS_CALIB_STATUS:
> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
> break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 8d84508d4db8..24c0eb5f4d29 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1004,6 +1004,18 @@ enum v4l2_auto_focus_range {
> #define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
> #define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
>
> +#define V4L2_LENS_CALIB_STOP (0 << 0)
> +#define V4L2_LENS_CALIB_START (1 << 0)
Why a bitmask? What would setting both bits at the same time mean?
> +
> +#define V4L2_CID_LENS_CALIB_CONTROL (V4L2_CID_CAMERA_CLASS_BASE+43)
> +
> +#define V4L2_LENS_CALIB_IDLE (0 << 0)
> +#define V4L2_LENS_CALIB_BUSY (1 << 0)
> +#define V4L2_LENS_CALIB_COMPLETE (1 << 1)
> +#define V4L2_LENS_CALIB_FAILED (1 << 2)
Ditto. The status will only ever be one of these values, other than
possibly COMPLETE | FAILED. Renaming COMPLETE to SUCCESS would remove
that ambiguity and definitely make it one bit at a time.
Dave
> +
> +#define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
> --
> 2.37.2
>
Hi Laurent,
On 6/6/23 12:36, Laurent Pinchart wrote:
> Hi Michael,
>
> Thank you for the patch.
>
> On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
>> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
>> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
>> Clarify this in the documentation.
>>
>> Signed-off-by: Michael Riesch <[email protected]>
>> ---
>> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
>> 1 file changed, 3 insertions(+), 1 deletion(-)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index df29150dce7b..42cf4c3cda0c 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
>> This control moves the focal point of the camera by the specified
>> amount. The unit is undefined. Positive values move the focus closer
>> to the camera, negative values towards infinity. This is a
>> - write-only control.
>> + write-only control. It should be implemented only if the device cannot
>> + handle absolute values.
>> +
>
> Extra blank line.
Will fix that.
> I don't think this is right. The control was added for the UVC driver,
> and there are devices that implement both absolute and relative focus.
I am by no means an expert here and just following Sakari's suggestion
here (see [0]). I can drop the patch, leave it as-is, or modify it.
Whatever makes sense to you guys. But maybe I should leave this to
someone more knowledgeable in this area and drop the patch from my
series. The changes above are completely orthogonal to my work.
Best regards,
Michael
>
>>
>> ``V4L2_CID_FOCUS_AUTO (boolean)``
>> Enables continuous automatic focus adjustments. The effect of manual
>>
>
[0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%[email protected]/
Hi Michael,
On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> On 6/6/23 12:36, Laurent Pinchart wrote:
> > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> >> Clarify this in the documentation.
> >>
> >> Signed-off-by: Michael Riesch <[email protected]>
> >> ---
> >> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> >> 1 file changed, 3 insertions(+), 1 deletion(-)
> >>
> >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> index df29150dce7b..42cf4c3cda0c 100644
> >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> >> This control moves the focal point of the camera by the specified
> >> amount. The unit is undefined. Positive values move the focus closer
> >> to the camera, negative values towards infinity. This is a
> >> - write-only control.
> >> + write-only control. It should be implemented only if the device cannot
> >> + handle absolute values.
> >> +
> >
> > Extra blank line.
>
> Will fix that.
>
> > I don't think this is right. The control was added for the UVC driver,
> > and there are devices that implement both absolute and relative focus.
>
> I am by no means an expert here and just following Sakari's suggestion
> here (see [0]). I can drop the patch, leave it as-is, or modify it.
> Whatever makes sense to you guys. But maybe I should leave this to
> someone more knowledgeable in this area and drop the patch from my
> series. The changes above are completely orthogonal to my work.
V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
UVC, and to my surprise, it turns out it has never been implemented in
the uvcvideo driver. The 3 devices I know of that implement the UVC
relative focus control also implement the UVC absolute focus control.
I'm tempted to deprecate this control completely. Sakari, any opinion ?
> >>
> >> ``V4L2_CID_FOCUS_AUTO (boolean)``
> >> Enables continuous automatic focus adjustments. The effect of manual
> >>
> >
>
> [0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%[email protected]/
--
Regards,
Laurent Pinchart
Hi Michael,
Thank you for the patch.
On Tue, Apr 25, 2023 at 11:45:13AM +0200, Michael Riesch wrote:
> Add the controls V4L2_CID_{FOCUS,ZOOM}_{CURRENT,STATUS} that report the
> current position and status, respectively, of the zoom lens group and
> the focus lens group.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 46 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 10 +++++
> include/uapi/linux/v4l2-controls.h | 9 +++++
> 3 files changed, 65 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 42cf4c3cda0c..3ea4175f9619 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -150,6 +150,29 @@ enum v4l2_exposure_metering -
> write-only control. It should be implemented only if the device cannot
> handle absolute values.
>
> +``V4L2_CID_FOCUS_CURRENT (integer)``
> + The current position of the focal point. The unit is undefined. Larger
> + values indicate that the focus is closer to the camera, smaller values
> + indicate that the focus is closer to infinity. This is a read-only control.
I think you should also update the definition of the
V4L2_CID_FOCUS_ABSOLUTE control to indicate the reading the control back
will return the focus *target*, not the current focus position. This
control should also refer to V4L2_CID_FOCUS_ABSOLUTE.
I think we should also require the V4L2_CID_FOCUS_CURRENT and
V4L2_CID_FOCUS_ABSOLUTE controls to have the same unit.
Is this control expected to generate events, when the lens reaches its
target position, or during movement ?
How should we deal with the drivers that implement
V4L2_CID_FOCUS_ABSOLUTE, do any of them implement reading
V4L2_CID_FOCUS_ABSOLUTE with the semantics of the V4L2_CID_FOCUS_CURRENT
control, instead of returning the focus target ?
> +
> +``V4L2_CID_FOCUS_STATUS (bitmask)``
> + The status of the focus lens group. The possible flags are described in
> + the table below. This is a read-only control.
Is this control expected to generate events ?
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_STATUS_IDLE``
> + - Focus lens group is at rest.
> + * - ``V4L2_LENS_STATUS_BUSY``
> + - Focus lens group is moving.
V4L2_LENS_STATUS_MOVING would be a better name if it's defined as "is
moving".
> + * - ``V4L2_LENS_STATUS_FAILED``
> + - Focus lens group has failed to reach its target position. The driver
What are the expected reasons for this ?
> + will not transition from this state until another action is performed
> + by an application.
You're talking about transitions here, I think you should document the
state machine for the other states too. I expect the control to
transition from IDLE to MOVING when the V4L2_CID_FOCUS_ABSOLUTE control
is set, and transition from MOVING to IDLE or FAILED at the end of the
motion. What happens if the user sets V4L2_CID_FOCUS_ABSOLUTE while the
status is MOVING also needs to be documented.
It sounds we need helper functions to implement this state machine and
generate events, leaving it to drivers would open the door to different
behaviours for different devices.
All these comments apply to zoom too.
>
> ``V4L2_CID_FOCUS_AUTO (boolean)``
> Enables continuous automatic focus adjustments. The effect of manual
> @@ -241,6 +264,29 @@ enum v4l2_auto_focus_range -
> movement. A negative value moves the zoom lens group towards the
> wide-angle direction. The zoom speed unit is driver-specific.
>
> +``V4L2_CID_ZOOM_CURRENT (integer)``
> + The current objective lens focal length. The unit is undefined and
> + its value should be a positive integer. This is a read-only control.
> +
> +``V4L2_CID_ZOOM_STATUS (bitmask)``
> + The status of the zoom lens group. The possible flags are described in
> + the table below. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_STATUS_IDLE``
> + - Zoom lens group is at rest.
> + * - ``V4L2_LENS_STATUS_BUSY``
> + - Zoom lens group is moving.
> + * - ``V4L2_LENS_STATUS_FAILED``
> + - Zoom lens group has failed to reach its target position. The driver will
> + not transition from this state until another action is performed by an
> + application.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 564fedee2c88..794ef3ab0c02 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1044,6 +1044,10 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
> case V4L2_CID_HDR_SENSOR_MODE: return "HDR Sensor Mode";
> + case V4L2_CID_FOCUS_CURRENT: return "Focus, Current";
> + case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
> + case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
> + case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1593,6 +1597,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> break;
> + case V4L2_CID_FOCUS_CURRENT:
> + case V4L2_CID_FOCUS_STATUS:
> + case V4L2_CID_ZOOM_CURRENT:
> + case V4L2_CID_ZOOM_STATUS:
> + *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
> + break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> case V4L2_CID_AUTO_FOCUS_STATUS:
> case V4L2_CID_FLASH_READY:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 5e80daa4ffe0..793ee8c65e87 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -993,6 +993,15 @@ enum v4l2_auto_focus_range {
>
> #define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36)
>
> +#define V4L2_LENS_STATUS_IDLE (0 << 0)
> +#define V4L2_LENS_STATUS_BUSY (1 << 0)
> +#define V4L2_LENS_STATUS_FAILED (1 << 2)
> +
> +#define V4L2_CID_FOCUS_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+37)
> +#define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
> +#define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
> +#define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
--
Regards,
Laurent Pinchart
On Tue, Jun 06, 2023 at 01:34:21PM +0300, Laurent Pinchart wrote:
> On Tue, Apr 25, 2023 at 11:45:11AM +0200, Michael Riesch wrote:
> > The current unit description of the V4L2_CID_FOCUS_ABSOLUTE does not
> > make sense and was probably copy-pasted from V4L2_CID_FOCUS_RELATIVE.
> > Fix the unit description in the documentation.
> >
> > Signed-off-by: Michael Riesch <[email protected]>
> > ---
> > Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 ++--
> > 1 file changed, 2 insertions(+), 2 deletions(-)
> >
> > diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > index daa4f40869f8..df29150dce7b 100644
> > --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > @@ -140,8 +140,8 @@ enum v4l2_exposure_metering -
> >
> > ``V4L2_CID_FOCUS_ABSOLUTE (integer)``
> > This control sets the focal point of the camera to the specified
> > - position. The unit is undefined. Positive values set the focus
> > - closer to the camera, negative values towards infinity.
> > + position. The unit is undefined. Larger values move the focus closer to
> > + the camera, smaller values move the focus to infinity.
>
> Reviewed-by: Laurent Pinchart <[email protected]>
I just noticed that the UVC specification states
4.2.2.1.6 Focus (Absolute) Control
The Focus (Absolute) Control is used to specify the distance to the
optimally focused target. This value is expressed in millimeters. The
default value is implementation-specific.
This is the opposite of the V4L2_CID_FOCUS_ABSOLUTE control :-( I
suppose this will need to be solved in the uvcvideo driver by converting
the value.
> >
> > ``V4L2_CID_FOCUS_RELATIVE (integer)``
> > This control moves the focal point of the camera by the specified
> >
--
Regards,
Laurent Pinchart
Hi Michael,
Thank you for the patch.
On Tue, Apr 25, 2023 at 11:45:14AM +0200, Michael Riesch wrote:
> Add the controls V4L2_CID_FOCUS_SPEED and V4L2_CID_ZOOM_SPEED that set
> the speed of the zoom lens group and focus lens group, respectively.
Ah, now the UVC relative focus and zoom controls could be implemented
;-) They still don't match the definition of the corresponding V4L2
controls, so I'm still tempted to deprecate both.
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
> include/uapi/linux/v4l2-controls.h | 2 ++
> 3 files changed, 14 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index 3ea4175f9619..a17620ab03b9 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -174,6 +174,11 @@ enum v4l2_exposure_metering -
> will not transition from this state until another action is performed
> by an application.
>
> +``V4L2_CID_FOCUS_SPEED (integer)``
> + Set the speed with which the focus lens group of the camera is moved
> + (V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_RELATIVE). The unit is
> + driver-specific. The value should be a positive integer.
> +
Could you explain your expected use cases for focus and zoom speed ?
> ``V4L2_CID_FOCUS_AUTO (boolean)``
> Enables continuous automatic focus adjustments. The effect of manual
> focus adjustments while this feature is enabled is undefined,
> @@ -287,6 +292,11 @@ enum v4l2_auto_focus_range -
> not transition from this state until another action is performed by an
> application.
>
> +``V4L2_CID_ZOOM_SPEED (integer)``
> + Set the speed with which the zoom lens group of the camera is moved
> + (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
> + driver-specific. The value should be a positive integer.
> +
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 794ef3ab0c02..3ef465ba73bd 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1048,6 +1048,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
> case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
> case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
> + case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
> + case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 793ee8c65e87..8d84508d4db8 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1001,6 +1001,8 @@ enum v4l2_auto_focus_range {
> #define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
> #define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
> #define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
> +#define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
> +#define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
>
> /* FM Modulator class control IDs */
>
--
Regards,
Laurent Pinchart
On Wed, Jun 07, 2023 at 09:42:07AM +0300, Laurent Pinchart wrote:
> On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> > On 6/6/23 12:36, Laurent Pinchart wrote:
> > > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> > >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> > >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> > >> Clarify this in the documentation.
> > >>
> > >> Signed-off-by: Michael Riesch <[email protected]>
> > >> ---
> > >> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > >>
> > >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> index df29150dce7b..42cf4c3cda0c 100644
> > >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> > >> This control moves the focal point of the camera by the specified
> > >> amount. The unit is undefined. Positive values move the focus closer
> > >> to the camera, negative values towards infinity. This is a
> > >> - write-only control.
> > >> + write-only control. It should be implemented only if the device cannot
> > >> + handle absolute values.
> > >> +
> > >
> > > Extra blank line.
> >
> > Will fix that.
> >
> > > I don't think this is right. The control was added for the UVC driver,
> > > and there are devices that implement both absolute and relative focus.
> >
> > I am by no means an expert here and just following Sakari's suggestion
> > here (see [0]). I can drop the patch, leave it as-is, or modify it.
> > Whatever makes sense to you guys. But maybe I should leave this to
> > someone more knowledgeable in this area and drop the patch from my
> > series. The changes above are completely orthogonal to my work.
>
> V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
> UVC, and to my surprise, it turns out it has never been implemented in
> the uvcvideo driver. The 3 devices I know of that implement the UVC
> relative focus control also implement the UVC absolute focus control.
>
> I'm tempted to deprecate this control completely. Sakari, any opinion ?
This is how the UVC relative focus control is defined.
4.2.2.1.7 Focus (Relative) Control
The Focus (Relative) Control is used to move the focus lens group to
specify the distance to the optimally focused target.
The bFocusRelative field indicates whether the focus lens group is
stopped or is moving for near or for infinity direction. A value of 1
indicates that the focus lens group is moved for near direction. A
value of 0 indicates that the focus lens group is stopped. And a value
of 0xFF indicates that the lens group is moved for infinity direction.
The GET_MIN, GET_MAX, GET_RES and GET_DEF requests will return zero
for this field.
The bSpeed field indicates the speed of the lens group movement. A low
number indicates a slow speed and a high number indicates a high
speed. The GET_MIN, GET_MAX and GET_RES requests are used to retrieve
the range and resolution for this field. The GET_DEF request is used
to retrieve the default value for this field. If the control does not
support speed control, it will return the value 1 in this field for
all these requests.
If both Relative and Absolute Controls are supported, a SET_CUR to the
Relative Control with a value other than 0x00 shall result in a
Control Change interrupt for the Absolute Control at the end of the
movement (see section 2.4.2.2, “Status Interrupt Endpoint”). The end
of movement can be due to physical device limits, or due to an
explicit request by the host to stop the movement. If the end of
movement is due to physical device limits (such as a limit in range of
motion), a Control Change interrupt shall be generated for this
Relative Control. If there is no limit in range of motion, a Control
Change interrupt is not required.
It seems there's no way we can just map this to V4L2_CID_FOCUS_RELATIVE,
making the V4L2 relative focus control quite useless.
> > >>
> > >> ``V4L2_CID_FOCUS_AUTO (boolean)``
> > >> Enables continuous automatic focus adjustments. The effect of manual
> > >>
> >
> > [0] https://lore.kernel.org/all/ZDbChgZJHVaaX3%[email protected]/
--
Regards,
Laurent Pinchart
Hi Michael,
Thank you for the patch.
On Tue, Apr 25, 2023 at 11:45:15AM +0200, Michael Riesch wrote:
> Add the controls V4L2_CID_LENS_CALIB_CONTROL and V4L2_CID_LENS_CALIB_STATUS
> that facilitate the control of the lens group calibration procedure.
>
> Signed-off-by: Michael Riesch <[email protected]>
> ---
> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 35 ++++++++++++++++++++++
> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 4 +++
> include/uapi/linux/v4l2-controls.h | 12 ++++++++
> 3 files changed, 51 insertions(+)
>
> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> index a17620ab03b9..8b54a0f3a617 100644
> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> @@ -297,6 +297,41 @@ enum v4l2_auto_focus_range -
> (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
> driver-specific. The value should be a positive integer.
>
> +``V4L2_CID_LENS_CALIB_CONTROL (bitmask)``
> + Control the calibration procedure (or individual parts thereof) of the lens
> + groups. For example, this could include the mechanical range detection
> + of zoom lens motors. This is a write-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_STOP``
> + - Stop the lens calibration procedure.
> + * - ``V4L2_LENS_CALIB_START``
> + - Start the complete lens calibration procedure.
> +
> +``V4L2_CID_LENS_CALIB_STATUS (bitmask)``
> + The status of the calibration procedure (or individual parts thereof) of
> + the lens groups. This is a read-only control.
> +
> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
> +
> +.. flat-table::
> + :header-rows: 0
> + :stub-columns: 0
> +
> + * - ``V4L2_LENS_CALIB_IDLE``
> + - Lens calibration procedure has not yet been started.
> + * - ``V4L2_LENS_CALIB_BUSY``
> + - Lens calibration procedure is in progress.
> + * - ``V4L2_LENS_CALIB_COMPLETE``
> + - Lens calibration procedure is complete.
> + * - ``V4L2_LENS_CALIB_FAILED``
> + - Lens calibration procedure has failed.
> +
This sounds *very* ad-hoc for the device you're working on. Does it
really make sense as a standardized V4L2 control ? A device-specific
control could be better.
If you want to standardize this, I'd like to see multiple use cases, as
well as a detailed documentation of the calibration process.
> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
> This control sets the camera's aperture to the specified value. The
> unit is undefined. Larger values open the iris wider, smaller values
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> index 3ef465ba73bd..faddfecba6d9 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
> @@ -1050,6 +1050,8 @@ const char *v4l2_ctrl_get_name(u32 id)
> case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
> case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
> + case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
> + case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
>
> /* FM Radio Modulator controls */
> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1596,6 +1598,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_RELATIVE:
> case V4L2_CID_IRIS_RELATIVE:
> case V4L2_CID_ZOOM_RELATIVE:
> + case V4L2_CID_LENS_CALIB_CONTROL:
> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
> break;
> @@ -1603,6 +1606,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
> case V4L2_CID_FOCUS_STATUS:
> case V4L2_CID_ZOOM_CURRENT:
> case V4L2_CID_ZOOM_STATUS:
> + case V4L2_CID_LENS_CALIB_STATUS:
> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
> break;
> case V4L2_CID_FLASH_STROBE_STATUS:
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 8d84508d4db8..24c0eb5f4d29 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -1004,6 +1004,18 @@ enum v4l2_auto_focus_range {
> #define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
> #define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
>
> +#define V4L2_LENS_CALIB_STOP (0 << 0)
> +#define V4L2_LENS_CALIB_START (1 << 0)
> +
> +#define V4L2_CID_LENS_CALIB_CONTROL (V4L2_CID_CAMERA_CLASS_BASE+43)
> +
> +#define V4L2_LENS_CALIB_IDLE (0 << 0)
> +#define V4L2_LENS_CALIB_BUSY (1 << 0)
> +#define V4L2_LENS_CALIB_COMPLETE (1 << 1)
> +#define V4L2_LENS_CALIB_FAILED (1 << 2)
> +
> +#define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
> +
> /* FM Modulator class control IDs */
>
> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>
--
Regards,
Laurent Pinchart
Hi Laurent,
On Wed, Jun 07, 2023 at 09:55:20AM +0300, Laurent Pinchart wrote:
> On Wed, Jun 07, 2023 at 09:42:07AM +0300, Laurent Pinchart wrote:
> > On Tue, Jun 06, 2023 at 03:15:33PM +0200, Michael Riesch wrote:
> > > On 6/6/23 12:36, Laurent Pinchart wrote:
> > > > On Tue, Apr 25, 2023 at 11:45:12AM +0200, Michael Riesch wrote:
> > > >> The control V4L2_CID_FOCUS_RELATIVE only makes sense if the device cannot
> > > >> handle absolute focal point positioning with V4L2_CID_FOCUS_ABSOLUTE.
> > > >> Clarify this in the documentation.
> > > >>
> > > >> Signed-off-by: Michael Riesch <[email protected]>
> > > >> ---
> > > >> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 4 +++-
> > > >> 1 file changed, 3 insertions(+), 1 deletion(-)
> > > >>
> > > >> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> index df29150dce7b..42cf4c3cda0c 100644
> > > >> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
> > > >> @@ -147,7 +147,9 @@ enum v4l2_exposure_metering -
> > > >> This control moves the focal point of the camera by the specified
> > > >> amount. The unit is undefined. Positive values move the focus closer
> > > >> to the camera, negative values towards infinity. This is a
> > > >> - write-only control.
> > > >> + write-only control. It should be implemented only if the device cannot
> > > >> + handle absolute values.
> > > >> +
> > > >
> > > > Extra blank line.
> > >
> > > Will fix that.
> > >
> > > > I don't think this is right. The control was added for the UVC driver,
> > > > and there are devices that implement both absolute and relative focus.
> > >
> > > I am by no means an expert here and just following Sakari's suggestion
> > > here (see [0]). I can drop the patch, leave it as-is, or modify it.
> > > Whatever makes sense to you guys. But maybe I should leave this to
> > > someone more knowledgeable in this area and drop the patch from my
> > > series. The changes above are completely orthogonal to my work.
> >
> > V4L2_CID_FOCUS_RELATIVE is an annoying control. It was introduced for
> > UVC, and to my surprise, it turns out it has never been implemented in
> > the uvcvideo driver. The 3 devices I know of that implement the UVC
> > relative focus control also implement the UVC absolute focus control.
> >
> > I'm tempted to deprecate this control completely. Sakari, any opinion ?
>
> This is how the UVC relative focus control is defined.
>
> 4.2.2.1.7 Focus (Relative) Control
>
> The Focus (Relative) Control is used to move the focus lens group to
> specify the distance to the optimally focused target.
>
> The bFocusRelative field indicates whether the focus lens group is
> stopped or is moving for near or for infinity direction. A value of 1
> indicates that the focus lens group is moved for near direction. A
> value of 0 indicates that the focus lens group is stopped. And a value
> of 0xFF indicates that the lens group is moved for infinity direction.
> The GET_MIN, GET_MAX, GET_RES and GET_DEF requests will return zero
> for this field.
>
> The bSpeed field indicates the speed of the lens group movement. A low
> number indicates a slow speed and a high number indicates a high
> speed. The GET_MIN, GET_MAX and GET_RES requests are used to retrieve
> the range and resolution for this field. The GET_DEF request is used
> to retrieve the default value for this field. If the control does not
> support speed control, it will return the value 1 in this field for
> all these requests.
>
> If both Relative and Absolute Controls are supported, a SET_CUR to the
> Relative Control with a value other than 0x00 shall result in a
> Control Change interrupt for the Absolute Control at the end of the
> movement (see section 2.4.2.2, “Status Interrupt Endpoint”). The end
> of movement can be due to physical device limits, or due to an
> explicit request by the host to stop the movement. If the end of
> movement is due to physical device limits (such as a limit in range of
> motion), a Control Change interrupt shall be generated for this
> Relative Control. If there is no limit in range of motion, a Control
> Change interrupt is not required.
>
> It seems there's no way we can just map this to V4L2_CID_FOCUS_RELATIVE,
> making the V4L2 relative focus control quite useless.
I'd deprecate this control as it doesn't have any use.
The control documentation + other references in V4L2 control framework
could be even removed but the control ID definition needs to stay as user
space may refer to it.
--
Regards,
Sakari Ailus
Hi Laurent,
Thanks for the review. I'll address your comments in the next iteration,
but please be aware that it may take some time due to other things
needing my attention.
On 6/7/23 09:03, Laurent Pinchart wrote:
> Hi Michael,
>
> Thank you for the patch.
>
> On Tue, Apr 25, 2023 at 11:45:13AM +0200, Michael Riesch wrote:
>> Add the controls V4L2_CID_{FOCUS,ZOOM}_{CURRENT,STATUS} that report the
>> current position and status, respectively, of the zoom lens group and
>> the focus lens group.
>>
>> Signed-off-by: Michael Riesch <[email protected]>
>> ---
>> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 46 ++++++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 10 +++++
>> include/uapi/linux/v4l2-controls.h | 9 +++++
>> 3 files changed, 65 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 42cf4c3cda0c..3ea4175f9619 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -150,6 +150,29 @@ enum v4l2_exposure_metering -
>> write-only control. It should be implemented only if the device cannot
>> handle absolute values.
>>
>> +``V4L2_CID_FOCUS_CURRENT (integer)``
>> + The current position of the focal point. The unit is undefined. Larger
>> + values indicate that the focus is closer to the camera, smaller values
>> + indicate that the focus is closer to infinity. This is a read-only control.
>
> I think you should also update the definition of the
> V4L2_CID_FOCUS_ABSOLUTE control to indicate the reading the control back
> will return the focus *target*, not the current focus position. This
> control should also refer to V4L2_CID_FOCUS_ABSOLUTE.
>
> I think we should also require the V4L2_CID_FOCUS_CURRENT and
> V4L2_CID_FOCUS_ABSOLUTE controls to have the same unit.
>
> Is this control expected to generate events, when the lens reaches its
> target position, or during movement ?
Although I can't rule it out completely, it wouldn't expect that the
current position of a lens triggers events. I would say that this is a
continuous movement (well, in case of a stepper motor a quasi-continuous
one), so when exactly should the controller generate in interrupt/event?
I expect that the current position is polled instead, e.g., once per
frame in the scope of an autofocus algorithm.
When the lens reaches its target, an event could be generated, but this
would be related to the _STATUS control below.
> How should we deal with the drivers that implement
> V4L2_CID_FOCUS_ABSOLUTE, do any of them implement reading
> V4L2_CID_FOCUS_ABSOLUTE with the semantics of the V4L2_CID_FOCUS_CURRENT
> control, instead of returning the focus target ?
Not sure whether I understand your question correctly, but drivers that
implement V4L2_CID_FOCUS_ABSOLUTE are mostly VCMs where I believe it is
assumed that target always equals the current position due to their fast
response.
But then there is the UVC driver of course. Only had a quick look, but
it seems to me that Focus (Absolute) does not support GET_CUR. I would
guess that GET_CUR could return the current value in the sense of
V4L2_CID_FOCUS_CURRENT.
>> +
>> +``V4L2_CID_FOCUS_STATUS (bitmask)``
>> + The status of the focus lens group. The possible flags are described in
>> + the table below. This is a read-only control.
>
> Is this control expected to generate events ?
Yes, one could think of events when the lens reached its target or a
collision prevented exactly that.
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> + :header-rows: 0
>> + :stub-columns: 0
>> +
>> + * - ``V4L2_LENS_STATUS_IDLE``
>> + - Focus lens group is at rest.
>> + * - ``V4L2_LENS_STATUS_BUSY``
>> + - Focus lens group is moving.
>
> V4L2_LENS_STATUS_MOVING would be a better name if it's defined as "is
> moving".
>
>> + * - ``V4L2_LENS_STATUS_FAILED``
>> + - Focus lens group has failed to reach its target position. The driver
>
> What are the expected reasons for this ?
Variable focal length optics are complex beasts. It could happen, for
instance, that the range of the focus lens depends on the position of
the zoom lens(es). A smart controller could handle such situations
properly and the failure will never occur, but there might be
not-so-smart controllers as well.
In our case the status of the individual lenses is very important for
the initial calibration. Here, we need to be informed when a
collision/failure happened. Once the calibration is complete, this
should not occur anymore.
Best regards,
Michael
>> + will not transition from this state until another action is performed
>> + by an application.
>
> You're talking about transitions here, I think you should document the
> state machine for the other states too. I expect the control to
> transition from IDLE to MOVING when the V4L2_CID_FOCUS_ABSOLUTE control
> is set, and transition from MOVING to IDLE or FAILED at the end of the
> motion. What happens if the user sets V4L2_CID_FOCUS_ABSOLUTE while the
> status is MOVING also needs to be documented.
>
> It sounds we need helper functions to implement this state machine and
> generate events, leaving it to drivers would open the door to different
> behaviours for different devices.
>
> All these comments apply to zoom too.
>
>>
>> ``V4L2_CID_FOCUS_AUTO (boolean)``
>> Enables continuous automatic focus adjustments. The effect of manual
>> @@ -241,6 +264,29 @@ enum v4l2_auto_focus_range -
>> movement. A negative value moves the zoom lens group towards the
>> wide-angle direction. The zoom speed unit is driver-specific.
>>
>> +``V4L2_CID_ZOOM_CURRENT (integer)``
>> + The current objective lens focal length. The unit is undefined and
>> + its value should be a positive integer. This is a read-only control.
>> +
>> +``V4L2_CID_ZOOM_STATUS (bitmask)``
>> + The status of the zoom lens group. The possible flags are described in
>> + the table below. This is a read-only control.
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> + :header-rows: 0
>> + :stub-columns: 0
>> +
>> + * - ``V4L2_LENS_STATUS_IDLE``
>> + - Zoom lens group is at rest.
>> + * - ``V4L2_LENS_STATUS_BUSY``
>> + - Zoom lens group is moving.
>> + * - ``V4L2_LENS_STATUS_FAILED``
>> + - Zoom lens group has failed to reach its target position. The driver will
>> + not transition from this state until another action is performed by an
>> + application.
>> +
>> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
>> This control sets the camera's aperture to the specified value. The
>> unit is undefined. Larger values open the iris wider, smaller values
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 564fedee2c88..794ef3ab0c02 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1044,6 +1044,10 @@ const char *v4l2_ctrl_get_name(u32 id)
>> case V4L2_CID_CAMERA_ORIENTATION: return "Camera Orientation";
>> case V4L2_CID_CAMERA_SENSOR_ROTATION: return "Camera Sensor Rotation";
>> case V4L2_CID_HDR_SENSOR_MODE: return "HDR Sensor Mode";
>> + case V4L2_CID_FOCUS_CURRENT: return "Focus, Current";
>> + case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
>> + case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
>> + case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
>>
>> /* FM Radio Modulator controls */
>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1593,6 +1597,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>> *flags |= V4L2_CTRL_FLAG_WRITE_ONLY |
>> V4L2_CTRL_FLAG_EXECUTE_ON_WRITE;
>> break;
>> + case V4L2_CID_FOCUS_CURRENT:
>> + case V4L2_CID_FOCUS_STATUS:
>> + case V4L2_CID_ZOOM_CURRENT:
>> + case V4L2_CID_ZOOM_STATUS:
>> + *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
>> + break;
>> case V4L2_CID_FLASH_STROBE_STATUS:
>> case V4L2_CID_AUTO_FOCUS_STATUS:
>> case V4L2_CID_FLASH_READY:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 5e80daa4ffe0..793ee8c65e87 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -993,6 +993,15 @@ enum v4l2_auto_focus_range {
>>
>> #define V4L2_CID_HDR_SENSOR_MODE (V4L2_CID_CAMERA_CLASS_BASE+36)
>>
>> +#define V4L2_LENS_STATUS_IDLE (0 << 0)
>> +#define V4L2_LENS_STATUS_BUSY (1 << 0)
>> +#define V4L2_LENS_STATUS_FAILED (1 << 2)
>> +
>> +#define V4L2_CID_FOCUS_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+37)
>> +#define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
>> +#define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
>> +#define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
>> +
>> /* FM Modulator class control IDs */
>>
>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
>
Hi Laurent,
On 6/7/23 09:07, Laurent Pinchart wrote:
> Hi Michael,
>
> Thank you for the patch.
>
> On Tue, Apr 25, 2023 at 11:45:14AM +0200, Michael Riesch wrote:
>> Add the controls V4L2_CID_FOCUS_SPEED and V4L2_CID_ZOOM_SPEED that set
>> the speed of the zoom lens group and focus lens group, respectively.
>
> Ah, now the UVC relative focus and zoom controls could be implemented
> ;-) They still don't match the definition of the corresponding V4L2
> controls, so I'm still tempted to deprecate both.
There is also V4L2_CID_ZOOM_CONTINUOUS that could be mapped to the UVC
relative zoom, if I am not mistaken. I had a similar
V4L2_CID_FOCUS_CONTINUOUS control in mind briefly, but for our use case
this would not be general enough.
>> Signed-off-by: Michael Riesch <[email protected]>
>> ---
>> Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst | 10 ++++++++++
>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 2 ++
>> include/uapi/linux/v4l2-controls.h | 2 ++
>> 3 files changed, 14 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 3ea4175f9619..a17620ab03b9 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -174,6 +174,11 @@ enum v4l2_exposure_metering -
>> will not transition from this state until another action is performed
>> by an application.
>>
>> +``V4L2_CID_FOCUS_SPEED (integer)``
>> + Set the speed with which the focus lens group of the camera is moved
>> + (V4L2_CID_FOCUS_ABSOLUTE and V4L2_CID_FOCUS_RELATIVE). The unit is
>> + driver-specific. The value should be a positive integer.
>> +
>
> Could you explain your expected use cases for focus and zoom speed ?
As I outlined in the cover letter, the speed of the lenses can be
adjusted. This can be used e.g., in the autofocus algorithm, where a
coarse scan and a fine grained scan can be implemented using different
speeds.
The adjustable zoom speed control is simply there to give the user the
choice to zoom in/out fast or slow.
Should this go into the documentation?
Best regards,
Michael
>
>> ``V4L2_CID_FOCUS_AUTO (boolean)``
>> Enables continuous automatic focus adjustments. The effect of manual
>> focus adjustments while this feature is enabled is undefined,
>> @@ -287,6 +292,11 @@ enum v4l2_auto_focus_range -
>> not transition from this state until another action is performed by an
>> application.
>>
>> +``V4L2_CID_ZOOM_SPEED (integer)``
>> + Set the speed with which the zoom lens group of the camera is moved
>> + (V4L2_CID_ZOOM_ABSOLUTE and V4L2_CID_ZOOM_RELATIVE). The unit is
>> + driver-specific. The value should be a positive integer.
>> +
>> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
>> This control sets the camera's aperture to the specified value. The
>> unit is undefined. Larger values open the iris wider, smaller values
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index 794ef3ab0c02..3ef465ba73bd 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1048,6 +1048,8 @@ const char *v4l2_ctrl_get_name(u32 id)
>> case V4L2_CID_FOCUS_STATUS: return "Focus, Status";
>> case V4L2_CID_ZOOM_CURRENT: return "Zoom, Current";
>> case V4L2_CID_ZOOM_STATUS: return "Zoom, Status";
>> + case V4L2_CID_FOCUS_SPEED: return "Focus, Speed";
>> + case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
>>
>> /* FM Radio Modulator controls */
>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 793ee8c65e87..8d84508d4db8 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1001,6 +1001,8 @@ enum v4l2_auto_focus_range {
>> #define V4L2_CID_FOCUS_STATUS (V4L2_CID_CAMERA_CLASS_BASE+38)
>> #define V4L2_CID_ZOOM_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+39)
>> #define V4L2_CID_ZOOM_STATUS (V4L2_CID_CAMERA_CLASS_BASE+40)
>> +#define V4L2_CID_FOCUS_SPEED (V4L2_CID_CAMERA_CLASS_BASE+41)
>> +#define V4L2_CID_ZOOM_SPEED (V4L2_CID_CAMERA_CLASS_BASE+42)
>>
>> /* FM Modulator class control IDs */
>>
>
Hi Hans,
On 6/6/23 12:29, Hans Verkuil wrote:
> On 25/04/2023 11:45, Michael Riesch wrote:
>> A zoom lens group may consist of several lenses, and in a calibration
>> context it may be necessary to position the lenses individually. Add a
>> tuple of V4L2_CID_LENS_CALIB_ZOOMx_{ABSOLUTE,CURRENT,STATUS} controls
>> for each individual lens, where x = {1...5}.
>>
>> Signed-off-by: Michael Riesch <[email protected]>
>> ---
>> .../userspace-api/media/v4l/ext-ctrls-camera.rst | 30 ++++++++++++++++++++++
>> drivers/media/v4l2-core/v4l2-ctrls-defs.c | 25 ++++++++++++++++++
>> include/uapi/linux/v4l2-controls.h | 18 +++++++++++++
>> 3 files changed, 73 insertions(+)
>>
>> diff --git a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> index 8b54a0f3a617..21391f076971 100644
>> --- a/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> +++ b/Documentation/userspace-api/media/v4l/ext-ctrls-camera.rst
>> @@ -332,6 +332,36 @@ enum v4l2_auto_focus_range -
>> * - ``V4L2_LENS_CALIB_FAILED``
>> - Lens calibration procedure has failed.
>>
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_ABSOLUTE`` (integer)
>> + Set the absolute position of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The unit is
>> + driver-specific.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_CURRENT`` (integer)
>> + The current absolute position of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The unit is
>> + driver-specific. This is a read-only control.
>> +
>> +``V4L2_CID_LENS_CALIB_ZOOM{1...5}_STATUS`` (bitmask)
>> + The current status of the individual lens of the zoom lens group.
>> + Most likely, this is done in a calibration context. The possible flags are
>> + described in the table below. This is a read-only control.
>
> Wouldn't it be better to have this as an array control? That way the number of
> lenses is set by the driver and can be easily read by userspace.
Are you referring to the _STATUS controls exclusively or is this a
suggestion for _CURRENT and _ABSOLUTE as well?
This would mean that the user space can read out e.g., the status of all
lenses in one batch, right? This would make sense to me.
I am not sure about setting all target positions at the same time ->
I'll have to think about that one a bit.
Best regards,
Michael
>
>> +
>> +.. tabularcolumns:: |p{6.8cm}|p{10.7cm}|
>> +
>> +.. flat-table::
>> + :header-rows: 0
>> + :stub-columns: 0
>> +
>> + * - ``V4L2_LENS_STATUS_IDLE``
>> + - Zoom lens is at rest.
>> + * - ``V4L2_LENS_STATUS_BUSY``
>> + - Zoom lens is moving.
>> + * - ``V4L2_LENS_STATUS_FAILED``
>> + - Zoom lens has failed to reach its target position. The driver will
>> + not transition from this state until another action is performed by an
>> + application.
>> +
>> ``V4L2_CID_IRIS_ABSOLUTE (integer)``
>> This control sets the camera's aperture to the specified value. The
>> unit is undefined. Larger values open the iris wider, smaller values
>> diff --git a/drivers/media/v4l2-core/v4l2-ctrls-defs.c b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> index faddfecba6d9..8a78cffcd3e8 100644
>> --- a/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> +++ b/drivers/media/v4l2-core/v4l2-ctrls-defs.c
>> @@ -1052,6 +1052,21 @@ const char *v4l2_ctrl_get_name(u32 id)
>> case V4L2_CID_ZOOM_SPEED: return "Zoom, Speed";
>> case V4L2_CID_LENS_CALIB_CONTROL: return "Lens Calibration, Control";
>> case V4L2_CID_LENS_CALIB_STATUS: return "Lens Calibration, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE: return "Zoom1, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE: return "Zoom2, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE: return "Zoom3, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE: return "Zoom4, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE: return "Zoom5, Absolute";
>> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT: return "Zoom1, Current";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT: return "Zoom1, Current";
>
> You forget to update the number, it's all Zoom1 here.
>
>> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS: return "Zoom1, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS: return "Zoom2, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS: return "Zoom3, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS: return "Zoom4, Status";
>> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS: return "Zoom5, Status";
>>
>> /* FM Radio Modulator controls */
>> /* Keep the order of the 'case's the same as in v4l2-controls.h! */
>> @@ -1607,6 +1622,16 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>> case V4L2_CID_ZOOM_CURRENT:
>> case V4L2_CID_ZOOM_STATUS:
>> case V4L2_CID_LENS_CALIB_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM1_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM2_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM3_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM4_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM5_CURRENT:
>> + case V4L2_CID_LENS_CALIB_ZOOM1_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM2_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM3_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM4_STATUS:
>> + case V4L2_CID_LENS_CALIB_ZOOM5_STATUS:
>> *flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_VOLATILE;
>
> Same issue as for patch 3 w.r.t. VOLATILE.
>
>> break;
>> case V4L2_CID_FLASH_STROBE_STATUS:
>> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
>> index 24c0eb5f4d29..7c49c0ba23d4 100644
>> --- a/include/uapi/linux/v4l2-controls.h
>> +++ b/include/uapi/linux/v4l2-controls.h
>> @@ -1016,6 +1016,24 @@ enum v4l2_auto_focus_range {
>>
>> #define V4L2_CID_LENS_CALIB_STATUS (V4L2_CID_CAMERA_CLASS_BASE+44)
>>
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+45)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+46)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+47)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+48)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_ABSOLUTE (V4L2_CID_CAMERA_CLASS_BASE+49)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+50)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+51)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+52)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+53)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_CURRENT (V4L2_CID_CAMERA_CLASS_BASE+54)
>> +
>> +#define V4L2_CID_LENS_CALIB_ZOOM1_STATUS (V4L2_CID_CAMERA_CLASS_BASE+55)
>> +#define V4L2_CID_LENS_CALIB_ZOOM2_STATUS (V4L2_CID_CAMERA_CLASS_BASE+56)
>> +#define V4L2_CID_LENS_CALIB_ZOOM3_STATUS (V4L2_CID_CAMERA_CLASS_BASE+57)
>> +#define V4L2_CID_LENS_CALIB_ZOOM4_STATUS (V4L2_CID_CAMERA_CLASS_BASE+58)
>> +#define V4L2_CID_LENS_CALIB_ZOOM5_STATUS (V4L2_CID_CAMERA_CLASS_BASE+59)
>> +
>> /* FM Modulator class control IDs */
>>
>> #define V4L2_CID_FM_TX_CLASS_BASE (V4L2_CTRL_CLASS_FM_TX | 0x900)
>>
>
> Disclaimer: I do not have enough domain knowledge to comment on if this is the
> right solution or not. I can only comment on the control framework specifics.
>
> Regards,
>
> Hans