[PATCH v2 00/10] media: Report camera sensor properties

[PATCH v2 00/10] media: Report camera sensor properties

[Jacopo Mondi]

Hello,
   this second version of this series includes both the previously sent
"media: v4l2-ctrls: Add camera sensor location"
https://patchwork.kernel.org/project/linux-media/list/?series=160901
"media: i2c: ov5670: Report native size and crop bounds"
https://patchwork.kernel.org/project/linux-media/list/?series=161169
which could now be marked as superseded.

I decided to merge them together as the final intent of both series is to expose
informations on the camera sensor which are currently not available.

Retrieving the following camera static information is a requirement for the
implementation of the Android-compatiblity layer of libcamera, but I'm sure
it might prove useful for other user-space applications and libraries as well.

The series reports the following camera static informations
- Camera sensor location: by adding a new read-only control and a new DTS
  property to report it
- Camera sensor rotation: by adding a new read-only control and parsing the
  existing 'rotation' property
- Camera sensor pixel array size and active area size by using the V4L2
  subdevice selection API for two sensor of interest (these are just sensor
  patches without any core changes, but I've included them in this series
  anyway)

Compared to v1:
- Add V4L2_CID_CAMERA_SENSOR_ROTATION read-only control
- Parse the 'rotation' device property in the newly introduced
  v4l2_fwnode_register_controls() helper

As an example, a final not-to-merge patch to the coreboot ACPI tables
for the Soraka device used for development is provided as a simple usage
example of the 'rotation' and 'location' properties.
The patch is against the https://review.coreboot.org/coreboot.git master branch.

Thanks
   j

Jacopo Mondi (10):
  media: dt-bindings: Document 'location' property
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION
  media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION
  media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_LOCATION
  media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_ROTATION
  media: v4l2-fwnode: Add helper to register controls from fw
  media: i2c: ov5670: Register controls from firmware
  media: i2c: ov13858: Register controls from firmware
  media: i2c: ov5670: Report native size and crop bounds
  media: i2c: ov13858: Report native size and crop bounds

 .../bindings/media/video-interfaces.txt       | 10 ++++
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 43 ++++++++++++++
 drivers/media/i2c/ov13858.c                   | 31 ++++++++++
 drivers/media/i2c/ov5670.c                    | 27 +++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c          |  4 ++
 drivers/media/v4l2-core/v4l2-fwnode.c         | 57 +++++++++++++++++++
 include/media/v4l2-fwnode.h                   | 30 ++++++++++
 include/uapi/linux/v4l2-controls.h            |  7 +++
 8 files changed, 209 insertions(+)

--
2.23.0

[PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

Add the 'location' device property, used to specify the camera device
mounting position. The property is particularly meaningful for mobile
devices with a well defined usage orientation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index f884ada0bffc..865f4142f432 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -89,6 +89,16 @@ Optional properties
   but a number of degrees counter clockwise. Typical values are 0 and 180
   (upside down).

+- location: The camera sensor mounting location, expressed as a position
+  relative to the usage orientation of the device the sensor is installed on.
+  Possible values are:
+  0 - Front. The image sensor is mounted on the front facing side of the device.
+  For mobile devices such as smartphones, tablets and laptops the front side is
+  the user facing side of the device.
+  1 - Back. The image sensor is mounted on the back side of the device, which is
+  defined as the opposite side of the front facing one.
+  2 - External. The image sensor is connected to the device by extension cables,
+  and can be freely moved, regardless of the device position.

 Optional endpoint properties
 ----------------------------
--
2.23.0

[PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

Add the 'location' device property, used to specify the camera device
mounting position. The property is particularly meaningful for mobile
devices with a well defined usage orientation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
index f884ada0bffc..865f4142f432 100644
--- a/Documentation/devicetree/bindings/media/video-interfaces.txt
+++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
@@ -89,6 +89,16 @@ Optional properties
   but a number of degrees counter clockwise. Typical values are 0 and 180
   (upside down).

+- location: The camera sensor mounting location, expressed as a position
+  relative to the usage orientation of the device the sensor is installed on.
+  Possible values are:
+  0 - Front. The image sensor is mounted on the front facing side of the device.
+  For mobile devices such as smartphones, tablets and laptops the front side is
+  the user facing side of the device.
+  1 - Back. The image sensor is mounted on the back side of the device, which is
+  defined as the opposite side of the front facing one.
+  2 - External. The image sensor is connected to the device by extension cables,
+  and can be freely moved, regardless of the device position.

 Optional endpoint properties
 ----------------------------
--
2.23.0

[PATCH v2 02/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
control. The newly added read-only control reports the camera device
mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../media/uapi/v4l/ext-ctrls-camera.rst       | 34 +++++++++++++++++++
 1 file changed, 34 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index 51c1d5c9eb00..ecf151f3f0f4 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -510,6 +510,40 @@ enum v4l2_scene_mode -
     value down. A value of zero stops the motion if one is in progress
     and has no effect otherwise.
 
+``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
+    This read-only control describes the camera sensor location by
+    reporting its mounting position on the device where the camera is
+    installed. The control value is constant and not modifiable by software
+    and its value is retrieved from the firmware interface by parsing the
+    'location' property. This control is particularly meaningful for
+    devices which have a well defined orientation, such as phones, laptops
+    and portable devices as the camera location is expressed as a position
+    relative to the device intended usage orientation. In example, a camera
+    sensor installed on the user-facing side of a phone, a tablet or a
+    laptop device is said to be installed in the ``V4L2_LOCATION_FRONT``
+    location while camera sensors installed on the side opposed to the
+    front one are said to be installed in the ``V4L2_LOCATION_BACK``
+    location. Camera sensors connected to the device by extension cables
+    which are freely movable regardless of the device orientation, such as
+    webcams and digital cameras, are said to be have
+    ``V4L2_LOCATION_EXTERNAL`` location.
+
+
+
+.. flat-table::
+    :header-rows:  0
+    :stub-columns: 0
+
+    * - ``V4L2_LOCATION_FRONT``
+      - The camera sensor is located on the front side of the device.
+    * - ``V4L2_LOCATION_BACK``
+      - The camera sensor is located on the back side of the device.
+    * - ``V4L2_LOCATION_EXTERNAL``
+      - The camera sensor is connected by extension cables to the device and
+        it's freely movable.
+
+
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.23.0

[PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
control. The newly added read-only control reports the camera device
mounting rotation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
index ecf151f3f0f4..03d1c23d18f7 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
@@ -544,6 +544,15 @@ enum v4l2_scene_mode -
 
 
 
+``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
+    This read-only control describes the camera sensor orientation by
+    reporting its mounting rotation in respect to the device intended usage
+    orientation, expressed in counter clockwise degrees. The control value is
+    constant and not modifiable by software and its value is retrieved from the
+    firmware interface by parsing the 'rotation' property. Typical values are 0
+    degrees for upright mounted sensors and 180 degrees for sensors mounted
+    upside down.
+
 .. [#f1]
    This control may be changed to a menu control in the future, if more
    options are required.
-- 
2.23.0

[PATCH v2 04/10] media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
read-only control used to report the camera device mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
 include/uapi/linux/v4l2-controls.h   | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..db7124494530 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -994,6 +994,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1318,6 +1319,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..387c2c8553cb 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -912,6 +912,11 @@ enum v4l2_auto_focus_range {
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
 
+#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_LOCATION_FRONT			0
+#define V4L2_LOCATION_BACK			1
+#define V4L2_LOCATION_EXTERNAL			2
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.23.0

[PATCH v2 04/10] media: v4l2-ctrls: Add V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
read-only control used to report the camera device mounting position.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
 include/uapi/linux/v4l2-controls.h   | 5 +++++
 2 files changed, 7 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..db7124494530 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -994,6 +994,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1318,6 +1319,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		break;
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
+	case V4L2_CID_CAMERA_SENSOR_LOCATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..387c2c8553cb 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -912,6 +912,11 @@ enum v4l2_auto_focus_range {
 #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
 #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
 
+#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
+#define V4L2_LOCATION_FRONT			0
+#define V4L2_LOCATION_BACK			1
+#define V4L2_LOCATION_EXTERNAL			2
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.23.0

[PATCH v2 05/10] media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_ROTATION
read-only control used to report the camera device mounting rotation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
 include/uapi/linux/v4l2-controls.h   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index db7124494530..b7af47a25125 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -995,6 +995,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
 	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1320,6 +1321,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 	case V4L2_CID_CAMERA_SENSOR_LOCATION:
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 387c2c8553cb..f2be7a99818e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -917,6 +917,8 @@ enum v4l2_auto_focus_range {
 #define V4L2_LOCATION_BACK			1
 #define V4L2_LOCATION_EXTERNAL			2
 
+#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.23.0

[PATCH v2 05/10] media: v4l2-ctrls: Add V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

Add support for the newly defined V4L2_CID_CAMERA_SENSOR_ROTATION
read-only control used to report the camera device mounting rotation.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
 include/uapi/linux/v4l2-controls.h   | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index db7124494530..b7af47a25125 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -995,6 +995,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
 	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
 	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1320,6 +1321,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
 	case V4L2_CID_CAMERA_SENSOR_LOCATION:
+	case V4L2_CID_CAMERA_SENSOR_ROTATION:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
 		break;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 387c2c8553cb..f2be7a99818e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -917,6 +917,8 @@ enum v4l2_auto_focus_range {
 #define V4L2_LOCATION_BACK			1
 #define V4L2_LOCATION_EXTERNAL			2
 
+#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
+
 /* FM Modulator class control IDs */
 
 #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
-- 
2.23.0

[PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Jacopo Mondi]

Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
function parses the device node and endpoint firmware properties to
which a v4l2 control is associated to and registers the control with the
provided handler.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
 include/media/v4l2-fwnode.h           | 30 ++++++++++++++
 2 files changed, 87 insertions(+)

diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
index 3bd1888787eb..669801fceb64 100644
--- a/drivers/media/v4l2-core/v4l2-fwnode.c
+++ b/drivers/media/v4l2-core/v4l2-fwnode.c
@@ -25,6 +25,7 @@
 #include <linux/types.h>
 
 #include <media/v4l2-async.h>
+#include <media/v4l2-ctrls.h>
 #include <media/v4l2-fwnode.h>
 #include <media/v4l2-subdev.h>
 
@@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
 }
 EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
 
+int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
+				  struct v4l2_ctrl_handler *hdl,
+				  const struct v4l2_ctrl_ops *ctrl_ops)
+{
+	u32 val;
+	int ret;
+
+	ret = fwnode_property_read_u32(fwnode, "location", &val);
+	if (!ret) {
+		switch (val) {
+		case V4L2_LOCATION_FRONT:
+		case V4L2_LOCATION_BACK:
+		case V4L2_LOCATION_EXTERNAL:
+			break;
+		default:
+			pr_warn("Unsupported location: %u\n", val);
+			return -EINVAL;
+		}
+
+		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
+			pr_debug("Skip control '%s': already registered",
+				 v4l2_ctrl_get_name(
+					 V4L2_CID_CAMERA_SENSOR_LOCATION));
+		else
+			v4l2_ctrl_new_std(hdl, ctrl_ops,
+					  V4L2_CID_CAMERA_SENSOR_LOCATION,
+					  val, val, 1, val);
+	}
+
+	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
+	if (!ret) {
+		if (val > 360) {
+			pr_warn("Unsupported rotation: %u\n", val);
+			return -EINVAL;
+		}
+
+		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
+			pr_debug("Skip control '%s': already registered",
+				 v4l2_ctrl_get_name(
+					 V4L2_CID_CAMERA_SENSOR_ROTATION));
+		else
+			v4l2_ctrl_new_std(hdl, ctrl_ops,
+					  V4L2_CID_CAMERA_SENSOR_ROTATION,
+					  val, val, 1, val);
+	}
+
+	if (hdl->error) {
+		pr_warn("Failed to register controls from firmware: %d\n",
+			hdl->error);
+		return hdl->error;
+	}
+
+	return 0;
+}
+EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
+
 static int
 v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
 					  struct v4l2_async_notifier *notifier,
diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
index f6a7bcd13197..0dad6968bde9 100644
--- a/include/media/v4l2-fwnode.h
+++ b/include/media/v4l2-fwnode.h
@@ -25,6 +25,8 @@
 struct fwnode_handle;
 struct v4l2_async_notifier;
 struct v4l2_async_subdev;
+struct v4l2_ctrl_handler;
+struct v4l2_ctrl_ops;
 
 #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
 
@@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
  */
 void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
 
+/**
+ * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
+ *				     properties and register a v4l2 control
+ *				     for each of them
+ * @fwnode: pointer to the device fwnode handle
+ * @hdl: pointer to the v4l2 control handler to register controls with
+ * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
+ *
+ * Parse the @fwnode device and endpoint properties to which a v4l2 control
+ * is associated and register them with the provided handler @hdl.
+ * Currently the following v4l2 controls are parsed and registered:
+ * - V4L2_CID_CAMERA_SENSOR_LOCATION;
+ * - V4L2_CID_CAMERA_SENSOR_ROTATION;
+ *
+ * Controls already registered by the caller with the @hdl control handler are
+ * not overwritten. Callers should register the controls they want to handle
+ * themselves before calling this function.
+ *
+ * NOTE: This function locks the @hdl control handler mutex, the caller shall
+ * not hold the lock when calling this function.
+ *
+ * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
+ * specified.
+ */
+int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
+				  struct v4l2_ctrl_handler *hdl,
+				  const struct v4l2_ctrl_ops *ctrl_ops);
+
 /**
  * typedef parse_endpoint_func - Driver's callback function to be called on
  *	each V4L2 fwnode endpoint.
-- 
2.23.0

[PATCH v2 07/10] media: i2c: ov5670: Register controls from firmware

[Jacopo Mondi]

Use the newly introduced helper to parse properties from firmware and
register the associated v4l2 control.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 7 +++++++
 1 file changed, 7 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 041fcbb4eebd..2bc57e85f721 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV5670_REG_CHIP_ID		0x300a
 #define OV5670_CHIP_ID			0x005670
@@ -2059,6 +2060,7 @@ static const struct v4l2_ctrl_ops ov5670_ctrl_ops = {
 /* Initialize control handlers */
 static int ov5670_init_controls(struct ov5670 *ov5670)
 {
+	struct i2c_client *client = v4l2_get_subdevdata(&ov5670->sd);
 	struct v4l2_ctrl_handler *ctrl_hdlr;
 	s64 vblank_max;
 	s64 vblank_def;
@@ -2129,6 +2131,11 @@ static int ov5670_init_controls(struct ov5670 *ov5670)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_register_controls(dev_fwnode(&client->dev),
+					    ctrl_hdlr, &ov5670_ctrl_ops);
+	if (ret)
+		goto error;
+
 	ov5670->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0

[PATCH v2 08/10] media: i2c: ov13858: Register controls from firmware

[Jacopo Mondi]

Use the newly introduced helper to parse properties from firmware and
register the associated v4l2 control.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov13858.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index aac6f77afa0f..d06be9b1e8d5 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -7,6 +7,7 @@
 #include <linux/pm_runtime.h>
 #include <media/v4l2-ctrls.h>
 #include <media/v4l2-device.h>
+#include <media/v4l2-fwnode.h>
 
 #define OV13858_REG_VALUE_08BIT		1
 #define OV13858_REG_VALUE_16BIT		2
@@ -1666,6 +1667,11 @@ static int ov13858_init_controls(struct ov13858 *ov13858)
 		goto error;
 	}
 
+	ret = v4l2_fwnode_register_controls(dev_fwnode(&client->dev),
+					    ctrl_hdlr, &ov13858_ctrl_ops);
+	if (ret)
+		goto error;
+
 	ov13858->sd.ctrl_handler = ctrl_hdlr;
 
 	return 0;
-- 
2.23.0

[PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

Report the native pixel array size and the crop bounds for the ov5670
sensor driver.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
 1 file changed, 20 insertions(+)

diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
index 2bc57e85f721..3e22fe9ccad1 100644
--- a/drivers/media/i2c/ov5670.c
+++ b/drivers/media/i2c/ov5670.c
@@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov5670_get_selection(struct v4l2_subdev *sd,
+				struct v4l2_subdev_pad_config *cfg,
+				struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = 2592;
+		sel->r.height = 1944;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
 {
 	*frames = OV5670_NUM_OF_SKIP_FRAMES;
@@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
 	.enum_mbus_code = ov5670_enum_mbus_code,
 	.get_fmt = ov5670_get_pad_format,
 	.set_fmt = ov5670_set_pad_format,
+	.get_selection = ov5670_get_selection,
 	.enum_frame_size = ov5670_enum_frame_size,
 };
 
-- 
2.23.0

[PATCH v2 10/10] media: i2c: ov13858: Report native size and crop bounds

[Jacopo Mondi]

Report the native pixel array size and the crop bounds for the ov13858
sensor driver.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 drivers/media/i2c/ov13858.c | 25 +++++++++++++++++++++++++
 1 file changed, 25 insertions(+)

diff --git a/drivers/media/i2c/ov13858.c b/drivers/media/i2c/ov13858.c
index d06be9b1e8d5..81ebb38e6567 100644
--- a/drivers/media/i2c/ov13858.c
+++ b/drivers/media/i2c/ov13858.c
@@ -1402,6 +1402,30 @@ ov13858_set_pad_format(struct v4l2_subdev *sd,
 	return 0;
 }
 
+static int ov13858_get_selection(struct v4l2_subdev *sd,
+				 struct v4l2_subdev_pad_config *cfg,
+				 struct v4l2_subdev_selection *sel)
+{
+	switch (sel->target) {
+	case V4L2_SEL_TGT_CROP_BOUNDS:
+		sel->r.left = 16;
+		sel->r.top = 16;
+		sel->r.width = 4224;
+		sel->r.height = 3136;
+		break;
+	case V4L2_SEL_TGT_NATIVE_SIZE:
+		sel->r.left = 0;
+		sel->r.top = 0;
+		sel->r.width = 4256;
+		sel->r.height = 3168;
+		break;
+	default:
+		return -EINVAL;
+	}
+
+	return 0;
+}
+
 static int ov13858_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
 {
 	*frames = OV13858_NUM_OF_SKIP_FRAMES;
@@ -1565,6 +1589,7 @@ static const struct v4l2_subdev_pad_ops ov13858_pad_ops = {
 	.enum_mbus_code = ov13858_enum_mbus_code,
 	.get_fmt = ov13858_get_pad_format,
 	.set_fmt = ov13858_set_pad_format,
+	.get_selection = ov13858_get_selection,
 	.enum_frame_size = ov13858_enum_frame_size,
 };
 
-- 
2.23.0

[DO NOT MERGE] mb/google/poppy/variant/soraka: Add camera properties

[Jacopo Mondi]

Add the 'rotation' and 'location' properties to the cameras .asl files.

Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
---
 .../poppy/variants/baseboard/include/baseboard/acpi/cam0.asl    | 2 ++
 .../poppy/variants/baseboard/include/baseboard/acpi/cam1.asl    | 2 ++
 2 files changed, 4 insertions(+)

diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl
index 5bb8df5b75..07a1bf3ff7 100644
--- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl
+++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam0.asl
@@ -47,6 +47,8 @@ Scope (\_SB.PCI0.I2C2)
 			},
 			ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 			Package () {
+				Package () { "location", 0 },
+				Package () { "rotation", 0 },
 				Package () { "clock-frequency", 19200000 },
 				Package () { "lens-focus",
 					Package () { \_SB.PCI0.I2C2.VCM0 }
diff --git a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl
index d7640b249d..1814b422a2 100644
--- a/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl
+++ b/src/mainboard/google/poppy/variants/baseboard/include/baseboard/acpi/cam1.asl
@@ -47,6 +47,8 @@ Scope (\_SB.PCI0.I2C4)
 			},
 			ToUUID ("daffd814-6eba-4d8c-8a91-bc9bbf4aa301"),
 			Package () {
+				Package () { "location", 1 },
+				Package () { "rotation", 0 },
 				Package () { "clock-frequency", 19200000 },
 			}
 		})
-- 
2.23.0

Re: [PATCH v2 04/10] media: v4l2-ctrls: Add V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

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

Ups, please ignore this, I changed the commit message and forgot to
wipe the directory where I have formatted out the patches..

On Tue, Aug 27, 2019 at 11:23:31AM +0200, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
> read-only control used to report the camera device mounting position.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
>  include/uapi/linux/v4l2-controls.h   | 5 +++++
>  2 files changed, 7 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..db7124494530 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -994,6 +994,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> +	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
>
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1318,6 +1319,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		break;
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
> +	case V4L2_CID_CAMERA_SENSOR_LOCATION:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  		break;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..387c2c8553cb 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -912,6 +912,11 @@ enum v4l2_auto_focus_range {
>  #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
>  #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
>
> +#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_LOCATION_FRONT			0
> +#define V4L2_LOCATION_BACK			1
> +#define V4L2_LOCATION_EXTERNAL			2
> +
>  /* FM Modulator class control IDs */
>
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> --
> 2.23.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 05/10] media: v4l2-ctrls: Add V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

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

Ups, please ignore this, I changed the commit message and forgot to
wipe the directory where I have formatted out the patches..

On Tue, Aug 27, 2019 at 11:23:33AM +0200, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_CAMERA_SENSOR_ROTATION
> read-only control used to report the camera device mounting rotation.
>
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
>  include/uapi/linux/v4l2-controls.h   | 2 ++
>  2 files changed, 4 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index db7124494530..b7af47a25125 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -995,6 +995,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
>  	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
> +	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1320,6 +1321,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
>  	case V4L2_CID_CAMERA_SENSOR_LOCATION:
> +	case V4L2_CID_CAMERA_SENSOR_ROTATION:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  		break;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 387c2c8553cb..f2be7a99818e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -917,6 +917,8 @@ enum v4l2_auto_focus_range {
>  #define V4L2_LOCATION_BACK			1
>  #define V4L2_LOCATION_EXTERNAL			2
>
> +#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
> +
>  /* FM Modulator class control IDs */
>
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)
> --
> 2.23.0
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify the camera device
> mounting position. The property is particularly meaningful for mobile
> devices with a well defined usage orientation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..865f4142f432 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,16 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
> 
> +- location: The camera sensor mounting location, expressed as a position
> +  relative to the usage orientation of the device the sensor is installed on.

DT bindings being ABIs, we need to be precise and thorough there. One
particular point that bothers me is that the property is named location,
and its description refers to camera sensor mounting location. 

I see two options to fix this. One of them is to rename the property to
camera-location, but that would limit its future usage for other types
of devices. The other one is to document the property as applying to a
"device" instead of a "camera sensor", and add one sentence stating that
this property is valid for camera sensors only.

This will require finding another name for the device that the device is
mounted on though, as using device twice would be very confusing.

> +  Possible values are:
> +  0 - Front. The image sensor is mounted on the front facing side of the device.
> +  For mobile devices such as smartphones, tablets and laptops the front side is
> +  the user facing side of the device.
> +  1 - Back. The image sensor is mounted on the back side of the device, which is
> +  defined as the opposite side of the front facing one.
> +  2 - External. The image sensor is connected to the device by extension cables,
> +  and can be freely moved, regardless of the device position.

It could be something else than cables (wireless possibly ?). I would
phrase this along the lines of "The device is not attached directly to
the [device], or is attached in a way that allows it to move to
different locations."

> 
>  Optional endpoint properties
>  ----------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> Add the 'location' device property, used to specify the camera device
> mounting position. The property is particularly meaningful for mobile
> devices with a well defined usage orientation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
>  1 file changed, 10 insertions(+)
> 
> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> index f884ada0bffc..865f4142f432 100644
> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> @@ -89,6 +89,16 @@ Optional properties
>    but a number of degrees counter clockwise. Typical values are 0 and 180
>    (upside down).
> 
> +- location: The camera sensor mounting location, expressed as a position
> +  relative to the usage orientation of the device the sensor is installed on.

DT bindings being ABIs, we need to be precise and thorough there. One
particular point that bothers me is that the property is named location,
and its description refers to camera sensor mounting location. 

I see two options to fix this. One of them is to rename the property to
camera-location, but that would limit its future usage for other types
of devices. The other one is to document the property as applying to a
"device" instead of a "camera sensor", and add one sentence stating that
this property is valid for camera sensors only.

This will require finding another name for the device that the device is
mounted on though, as using device twice would be very confusing.

> +  Possible values are:
> +  0 - Front. The image sensor is mounted on the front facing side of the device.
> +  For mobile devices such as smartphones, tablets and laptops the front side is
> +  the user facing side of the device.
> +  1 - Back. The image sensor is mounted on the back side of the device, which is
> +  defined as the opposite side of the front facing one.
> +  2 - External. The image sensor is connected to the device by extension cables,
> +  and can be freely moved, regardless of the device position.

It could be something else than cables (wireless possibly ?). I would
phrase this along the lines of "The device is not attached directly to
the [device], or is attached in a way that allows it to move to
different locations."

> 
>  Optional endpoint properties
>  ----------------------------

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 02/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:28AM +0200, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> control. The newly added read-only control reports the camera device
> mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  .../media/uapi/v4l/ext-ctrls-camera.rst       | 34 +++++++++++++++++++
>  1 file changed, 34 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index 51c1d5c9eb00..ecf151f3f0f4 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -510,6 +510,40 @@ enum v4l2_scene_mode -
>      value down. A value of zero stops the motion if one is in progress
>      and has no effect otherwise.
>  
> +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> +    This read-only control describes the camera sensor location by
> +    reporting its mounting position on the device where the camera is
> +    installed. The control value is constant and not modifiable by software
> +    and its value is retrieved from the firmware interface by parsing the
> +    'location' property.

How the value is known to the driver is irrelevant in this context, I
would drop "and its value ...".

> This control is particularly meaningful for
> +    devices which have a well defined orientation, such as phones, laptops
> +    and portable devices as the camera location is expressed as a position
> +    relative to the device intended usage orientation. In example, a camera

s/In example/For example/

> +    sensor installed on the user-facing side of a phone, a tablet or a
> +    laptop device is said to be installed in the ``V4L2_LOCATION_FRONT``
> +    location while camera sensors installed on the side opposed to the

s/opposed to the front one/opposite the front/

> +    front one are said to be installed in the ``V4L2_LOCATION_BACK``
> +    location. Camera sensors connected to the device by extension cables
> +    which are freely movable regardless of the device orientation, such as
> +    webcams and digital cameras, are said to be have
> +    ``V4L2_LOCATION_EXTERNAL`` location.

Same comment as for the DT bindings regarding cables.

> +
> +
> +
> +.. flat-table::
> +    :header-rows:  0
> +    :stub-columns: 0
> +
> +    * - ``V4L2_LOCATION_FRONT``
> +      - The camera sensor is located on the front side of the device.
> +    * - ``V4L2_LOCATION_BACK``
> +      - The camera sensor is located on the back side of the device.
> +    * - ``V4L2_LOCATION_EXTERNAL``
> +      - The camera sensor is connected by extension cables to the device and
> +        it's freely movable.

s/it's/is/

> +
> +
> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> control. The newly added read-only control reports the camera device
> mounting rotation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
>  1 file changed, 9 insertions(+)
> 
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> index ecf151f3f0f4..03d1c23d18f7 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
>  
>  
>  
> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> +    This read-only control describes the camera sensor orientation by
> +    reporting its mounting rotation in respect to the device intended usage
> +    orientation, expressed in counter clockwise degrees. The control value is

Counter clockwise relative to what ? :-)

> +    constant and not modifiable by software and its value is retrieved from the
> +    firmware interface by parsing the 'rotation' property. Typical values are 0
> +    degrees for upright mounted sensors and 180 degrees for sensors mounted
> +    upside down.

Same comment as for 02/10 regarding how the value is known to the
driver.

> +
>  .. [#f1]
>     This control may be changed to a menu control in the future, if more
>     options are required.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 04/10] media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_LOCATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:30AM +0200, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_CAMERA_SENSOR_LOCATION
> read-only control used to report the camera device mounting position.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
>  include/uapi/linux/v4l2-controls.h   | 5 +++++
>  2 files changed, 7 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..db7124494530 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -994,6 +994,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_AUTO_FOCUS_RANGE:		return "Auto Focus, Range";
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
> +	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1318,6 +1319,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  		break;
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
> +	case V4L2_CID_CAMERA_SENSOR_LOCATION:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  		break;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..387c2c8553cb 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -912,6 +912,11 @@ enum v4l2_auto_focus_range {
>  #define V4L2_CID_PAN_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+32)
>  #define V4L2_CID_TILT_SPEED			(V4L2_CID_CAMERA_CLASS_BASE+33)
>  
> +#define V4L2_CID_CAMERA_SENSOR_LOCATION		(V4L2_CID_CAMERA_CLASS_BASE+34)
> +#define V4L2_LOCATION_FRONT			0
> +#define V4L2_LOCATION_BACK			1
> +#define V4L2_LOCATION_EXTERNAL			2
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 05/10] media: v4l2-ctrl: Add V4L2_CID_CAMERA_SENSOR_ROTATION

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:32AM +0200, Jacopo Mondi wrote:
> Add support for the newly defined V4L2_CID_CAMERA_SENSOR_ROTATION
> read-only control used to report the camera device mounting rotation.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>

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

(although I would squash it with the previous patch)

> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 2 ++
>  include/uapi/linux/v4l2-controls.h   | 2 ++
>  2 files changed, 4 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index db7124494530..b7af47a25125 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -995,6 +995,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_PAN_SPEED:		return "Pan, Speed";
>  	case V4L2_CID_TILT_SPEED:		return "Tilt, Speed";
>  	case V4L2_CID_CAMERA_SENSOR_LOCATION:	return "Camera Sensor Location";
> +	case V4L2_CID_CAMERA_SENSOR_ROTATION:	return "Camera Sensor Rotation";
>  
>  	/* FM Radio Modulator controls */
>  	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
> @@ -1320,6 +1321,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:
>  	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:
>  	case V4L2_CID_CAMERA_SENSOR_LOCATION:
> +	case V4L2_CID_CAMERA_SENSOR_ROTATION:
>  		*type = V4L2_CTRL_TYPE_INTEGER;
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
>  		break;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 387c2c8553cb..f2be7a99818e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -917,6 +917,8 @@ enum v4l2_auto_focus_range {
>  #define V4L2_LOCATION_BACK			1
>  #define V4L2_LOCATION_EXTERNAL			2
>  
> +#define V4L2_CID_CAMERA_SENSOR_ROTATION		(V4L2_CID_CAMERA_CLASS_BASE+35)
> +
>  /* FM Modulator class control IDs */
>  
>  #define V4L2_CID_FM_TX_CLASS_BASE		(V4L2_CTRL_CLASS_FM_TX | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Laurent Pinchart]

Hi Jacopo,

Thank you for the patch.

On Tue, Aug 27, 2019 at 11:23:34AM +0200, Jacopo Mondi wrote:
> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> function parses the device node and endpoint firmware properties to
> which a v4l2 control is associated to and registers the control with the
> provided handler.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3bd1888787eb..669801fceb64 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -25,6 +25,7 @@
>  #include <linux/types.h>
>  
>  #include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> +				  struct v4l2_ctrl_handler *hdl,
> +				  const struct v4l2_ctrl_ops *ctrl_ops)

Passing the ctrl_ops is a bit annoying. Would there be a way to get the
V4L2 control framework to accept NULL ops for read-only controls ?

> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> +	if (!ret) {
> +		switch (val) {
> +		case V4L2_LOCATION_FRONT:
> +		case V4L2_LOCATION_BACK:
> +		case V4L2_LOCATION_EXTERNAL:
> +			break;
> +		default:
> +			pr_warn("Unsupported location: %u\n", val);

dev_warn() would be nicer. Is there a way we could either pass the
struct device pointer, or maybe a subdev that would be populated with
the device, fwnode and hdl pointers ?

> +			return -EINVAL;
> +		}
> +
> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> +			pr_debug("Skip control '%s': already registered",
> +				 v4l2_ctrl_get_name(
> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> +		else
> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> +					  val, val, 1, val);
> +	}
> +
> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> +	if (!ret) {
> +		if (val > 360) {
> +			pr_warn("Unsupported rotation: %u\n", val);
> +			return -EINVAL;
> +		}

We need to define the range of allowed values in the control
documentation. 360 doesn't seem very useful as it's equivalent to 0.
A few possible options are [0, 360[, [-180, +180[ or ]-180, +180].

> +
> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> +			pr_debug("Skip control '%s': already registered",
> +				 v4l2_ctrl_get_name(
> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> +		else
> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> +					  val, val, 1, val);
> +	}
> +
> +	if (hdl->error) {
> +		pr_warn("Failed to register controls from firmware: %d\n",
> +			hdl->error);
> +		return hdl->error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> +
>  static int
>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>  					  struct v4l2_async_notifier *notifier,
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index f6a7bcd13197..0dad6968bde9 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -25,6 +25,8 @@
>  struct fwnode_handle;
>  struct v4l2_async_notifier;
>  struct v4l2_async_subdev;
> +struct v4l2_ctrl_handler;
> +struct v4l2_ctrl_ops;
>  
>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
>  
> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>  
> +/**
> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> + *				     properties and register a v4l2 control
> + *				     for each of them

I don't think that description is accurate.

> + * @fwnode: pointer to the device fwnode handle
> + * @hdl: pointer to the v4l2 control handler to register controls with
> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> + *
> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> + * is associated and register them with the provided handler @hdl.
> + * Currently the following v4l2 controls are parsed and registered:
> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> + *
> + * Controls already registered by the caller with the @hdl control handler are
> + * not overwritten. Callers should register the controls they want to handle
> + * themselves before calling this function.
> + *
> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> + * not hold the lock when calling this function.
> + *
> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> + * specified.
> + */
> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> +				  struct v4l2_ctrl_handler *hdl,
> +				  const struct v4l2_ctrl_ops *ctrl_ops);
> +
>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Hans Verkuil]

On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> Report the native pixel array size and the crop bounds for the ov5670
> sensor driver.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> index 2bc57e85f721..3e22fe9ccad1 100644
> --- a/drivers/media/i2c/ov5670.c
> +++ b/drivers/media/i2c/ov5670.c
> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>  	return 0;
>  }
>  
> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> +				struct v4l2_subdev_pad_config *cfg,
> +				struct v4l2_subdev_selection *sel)
> +{
> +	switch (sel->target) {
> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> +		sel->r.left = 0;
> +		sel->r.top = 0;
> +		sel->r.width = 2592;
> +		sel->r.height = 1944;

Why do you need this?

Since the format can change for this and the next driver I think CROP_BOUNDS
at least should match the current format.

I don't think this patch and the next have anything to do with the location/rotate
support. I would split it off from this series.

Regards,

	Hans

> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +
> +	return 0;
> +}
> +
>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>  {
>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
>  	.enum_mbus_code = ov5670_enum_mbus_code,
>  	.get_fmt = ov5670_get_pad_format,
>  	.set_fmt = ov5670_set_pad_format,
> +	.get_selection = ov5670_get_selection,
>  	.enum_frame_size = ov5670_enum_frame_size,
>  };
>  
> 

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Hans Verkuil]

On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> function parses the device node and endpoint firmware properties to
> which a v4l2 control is associated to and registers the control with the
> provided handler.
> 
> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> ---
>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
>  2 files changed, 87 insertions(+)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> index 3bd1888787eb..669801fceb64 100644
> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> @@ -25,6 +25,7 @@
>  #include <linux/types.h>
>  
>  #include <media/v4l2-async.h>
> +#include <media/v4l2-ctrls.h>
>  #include <media/v4l2-fwnode.h>
>  #include <media/v4l2-subdev.h>
>  
> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>  }
>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>  
> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> +				  struct v4l2_ctrl_handler *hdl,
> +				  const struct v4l2_ctrl_ops *ctrl_ops)

I'm not convinced that this helper is a good idea.

A helper that parses and validates this information makes sense,
but combining that with creating the controls feels wrong to me.

You're mixing two very different things in one function.

I think something like this would work better in a driver:

	if (!v4l2_fwnode_parse_location(&val))
		v4l2_ctrl_new_std(hdl, ctrl_ops,
				  V4L2_CID_CAMERA_SENSOR_LOCATION,
				  val, val, 1, val);
	if (!v4l2_fwnode_parse_rotation(&val))
		v4l2_ctrl_new_std(hdl, ctrl_ops,
				  V4L2_CID_CAMERA_SENSOR_ROTATION,
				  val, val, 1, val);

Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
function prototypes!)

> +{
> +	u32 val;
> +	int ret;
> +
> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> +	if (!ret) {
> +		switch (val) {
> +		case V4L2_LOCATION_FRONT:
> +		case V4L2_LOCATION_BACK:
> +		case V4L2_LOCATION_EXTERNAL:
> +			break;
> +		default:
> +			pr_warn("Unsupported location: %u\n", val);
> +			return -EINVAL;
> +		}
> +
> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> +			pr_debug("Skip control '%s': already registered",
> +				 v4l2_ctrl_get_name(
> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> +		else
> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> +					  val, val, 1, val);
> +	}
> +
> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> +	if (!ret) {
> +		if (val > 360) {

I'd add '|| val % 90' to this condition.

Regards,

	Hans

> +			pr_warn("Unsupported rotation: %u\n", val);
> +			return -EINVAL;
> +		}
> +
> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> +			pr_debug("Skip control '%s': already registered",
> +				 v4l2_ctrl_get_name(
> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> +		else
> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> +					  val, val, 1, val);
> +	}
> +
> +	if (hdl->error) {
> +		pr_warn("Failed to register controls from firmware: %d\n",
> +			hdl->error);
> +		return hdl->error;
> +	}
> +
> +	return 0;
> +}
> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> +
>  static int
>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>  					  struct v4l2_async_notifier *notifier,
> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> index f6a7bcd13197..0dad6968bde9 100644
> --- a/include/media/v4l2-fwnode.h
> +++ b/include/media/v4l2-fwnode.h
> @@ -25,6 +25,8 @@
>  struct fwnode_handle;
>  struct v4l2_async_notifier;
>  struct v4l2_async_subdev;
> +struct v4l2_ctrl_handler;
> +struct v4l2_ctrl_ops;
>  
>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
>  
> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>   */
>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>  
> +/**
> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> + *				     properties and register a v4l2 control
> + *				     for each of them
> + * @fwnode: pointer to the device fwnode handle
> + * @hdl: pointer to the v4l2 control handler to register controls with
> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> + *
> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> + * is associated and register them with the provided handler @hdl.
> + * Currently the following v4l2 controls are parsed and registered:
> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> + *
> + * Controls already registered by the caller with the @hdl control handler are
> + * not overwritten. Callers should register the controls they want to handle
> + * themselves before calling this function.
> + *
> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> + * not hold the lock when calling this function.
> + *
> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> + * specified.
> + */
> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> +				  struct v4l2_ctrl_handler *hdl,
> +				  const struct v4l2_ctrl_ops *ctrl_ops);
> +
>  /**
>   * typedef parse_endpoint_func - Driver's callback function to be called on
>   *	each V4L2 fwnode endpoint.
> 

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

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

HI Hans,

On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> > Report the native pixel array size and the crop bounds for the ov5670
> > sensor driver.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> >  1 file changed, 20 insertions(+)
> >
> > diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > index 2bc57e85f721..3e22fe9ccad1 100644
> > --- a/drivers/media/i2c/ov5670.c
> > +++ b/drivers/media/i2c/ov5670.c
> > @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> >  	return 0;
> >  }
> >
> > +static int ov5670_get_selection(struct v4l2_subdev *sd,
> > +				struct v4l2_subdev_pad_config *cfg,
> > +				struct v4l2_subdev_selection *sel)
> > +{
> > +	switch (sel->target) {
> > +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > +		sel->r.left = 0;
> > +		sel->r.top = 0;
> > +		sel->r.width = 2592;
> > +		sel->r.height = 1944;
>
> Why do you need this?
>

I need to expose the pixel array size and the active pixel area size,
and I interpreted the two above targets as the right place where to do
so.

At least for NATIVE_SIZE the documentation seems to match my
understanding:

"The native size of the device, e.g. a sensor’s pixel array. left and top
fields are zero for this target."


> Since the format can change for this and the next driver I think CROP_BOUNDS
> at least should match the current format.
>

Ah, does it? It was not clear to me from the documentation, as it
suggested to me that target actually identifies the active pixel area

"Bounds of the crop rectangle. All valid crop rectangles fit inside the
crop bounds rectangle."

It does not mention format, should this be updated?

How would you report the two information I need?

> I don't think this patch and the next have anything to do with the location/rotate
> support. I would split it off from this series.
>

Agreed, they were split in v1, maybe it has not been a wise move to
make a single series out of them. I'll split again.

Thanks
   j

> Regards,
>
> 	Hans
>
> > +		break;
> > +	default:
> > +		return -EINVAL;
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> >  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> >  {
> >  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
> > @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> >  	.enum_mbus_code = ov5670_enum_mbus_code,
> >  	.get_fmt = ov5670_get_pad_format,
> >  	.set_fmt = ov5670_set_pad_format,
> > +	.get_selection = ov5670_get_selection,
> >  	.enum_frame_size = ov5670_enum_frame_size,
> >  };
> >
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> > Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> > function parses the device node and endpoint firmware properties to
> > which a v4l2 control is associated to and registers the control with the
> > provided handler.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 3bd1888787eb..669801fceb64 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/types.h>
> >
> >  #include <media/v4l2-async.h>
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> > @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> > +				  struct v4l2_ctrl_handler *hdl,
> > +				  const struct v4l2_ctrl_ops *ctrl_ops)
>
> I'm not convinced that this helper is a good idea.
>
> A helper that parses and validates this information makes sense,
> but combining that with creating the controls feels wrong to me.
>
> You're mixing two very different things in one function.
>
> I think something like this would work better in a driver:
>
> 	if (!v4l2_fwnode_parse_location(&val))
> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
> 				  val, val, 1, val);
> 	if (!v4l2_fwnode_parse_rotation(&val))
> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
> 				  val, val, 1, val);
>
> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
> function prototypes!)
>

Could the control registration being conditional on the presence of
the *hdl parameter otherwise, or would you split the two operations
(property parsing and control registration) nonetheless ?

An helper was suggested in the v1 review, Laurent, Sakari, what do you
think here?

> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> > +	if (!ret) {
> > +		switch (val) {
> > +		case V4L2_LOCATION_FRONT:
> > +		case V4L2_LOCATION_BACK:
> > +		case V4L2_LOCATION_EXTERNAL:
> > +			break;
> > +		default:
> > +			pr_warn("Unsupported location: %u\n", val);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> > +			pr_debug("Skip control '%s': already registered",
> > +				 v4l2_ctrl_get_name(
> > +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> > +		else
> > +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> > +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> > +					  val, val, 1, val);
> > +	}
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> > +	if (!ret) {
> > +		if (val > 360) {
>
> I'd add '|| val % 90' to this condition.
>

Do we want to enforce this? I can't imagine any use case, but why a
camera cannot be rotated of an arbitrary number of degrees ?

Thanks
   j
> Regards,
>
> 	Hans
>
> > +			pr_warn("Unsupported rotation: %u\n", val);
> > +			return -EINVAL;
> > +		}
> > +
> > +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> > +			pr_debug("Skip control '%s': already registered",
> > +				 v4l2_ctrl_get_name(
> > +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> > +		else
> > +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> > +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> > +					  val, val, 1, val);
> > +	}
> > +
> > +	if (hdl->error) {
> > +		pr_warn("Failed to register controls from firmware: %d\n",
> > +			hdl->error);
> > +		return hdl->error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> > +
> >  static int
> >  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >  					  struct v4l2_async_notifier *notifier,
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index f6a7bcd13197..0dad6968bde9 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> >  struct fwnode_handle;
> >  struct v4l2_async_notifier;
> >  struct v4l2_async_subdev;
> > +struct v4l2_ctrl_handler;
> > +struct v4l2_ctrl_ops;
> >
> >  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
> >
> > @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> > + *				     properties and register a v4l2 control
> > + *				     for each of them
> > + * @fwnode: pointer to the device fwnode handle
> > + * @hdl: pointer to the v4l2 control handler to register controls with
> > + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> > + *
> > + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> > + * is associated and register them with the provided handler @hdl.
> > + * Currently the following v4l2 controls are parsed and registered:
> > + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> > + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> > + *
> > + * Controls already registered by the caller with the @hdl control handler are
> > + * not overwritten. Callers should register the controls they want to handle
> > + * themselves before calling this function.
> > + *
> > + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> > + * not hold the lock when calling this function.
> > + *
> > + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> > + * specified.
> > + */
> > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> > +				  struct v4l2_ctrl_handler *hdl,
> > +				  const struct v4l2_ctrl_ops *ctrl_ops);
> > +
> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Laurent,

On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify the camera device
> > mounting position. The property is particularly meaningful for mobile
> > devices with a well defined usage orientation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..865f4142f432 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,16 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> >
> > +- location: The camera sensor mounting location, expressed as a position
> > +  relative to the usage orientation of the device the sensor is installed on.
>
> DT bindings being ABIs, we need to be precise and thorough there. One
> particular point that bothers me is that the property is named location,
> and its description refers to camera sensor mounting location.
>
> I see two options to fix this. One of them is to rename the property to
> camera-location, but that would limit its future usage for other types
> of devices. The other one is to document the property as applying to a
> "device" instead of a "camera sensor", and add one sentence stating that
> this property is valid for camera sensors only.
>
> This will require finding another name for the device that the device is
> mounted on though, as using device twice would be very confusing.

I had the same concern, but I cannot find another term to convey
this... suggestions?

>
> > +  Possible values are:
> > +  0 - Front. The image sensor is mounted on the front facing side of the device.
> > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > +  the user facing side of the device.
> > +  1 - Back. The image sensor is mounted on the back side of the device, which is
> > +  defined as the opposite side of the front facing one.
> > +  2 - External. The image sensor is connected to the device by extension cables,
> > +  and can be freely moved, regardless of the device position.
>
> It could be something else than cables (wireless possibly ?). I would
> phrase this along the lines of "The device is not attached directly to
> the [device], or is attached in a way that allows it to move to
> different locations."

Agreed, once we find a good term for [device] :)

>
> >
> >  Optional endpoint properties
> >  ----------------------------
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Laurent,

On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify the camera device
> > mounting position. The property is particularly meaningful for mobile
> > devices with a well defined usage orientation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> >
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..865f4142f432 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,16 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> >
> > +- location: The camera sensor mounting location, expressed as a position
> > +  relative to the usage orientation of the device the sensor is installed on.
>
> DT bindings being ABIs, we need to be precise and thorough there. One
> particular point that bothers me is that the property is named location,
> and its description refers to camera sensor mounting location.
>
> I see two options to fix this. One of them is to rename the property to
> camera-location, but that would limit its future usage for other types
> of devices. The other one is to document the property as applying to a
> "device" instead of a "camera sensor", and add one sentence stating that
> this property is valid for camera sensors only.
>
> This will require finding another name for the device that the device is
> mounted on though, as using device twice would be very confusing.

I had the same concern, but I cannot find another term to convey
this... suggestions?

>
> > +  Possible values are:
> > +  0 - Front. The image sensor is mounted on the front facing side of the device.
> > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > +  the user facing side of the device.
> > +  1 - Back. The image sensor is mounted on the back side of the device, which is
> > +  defined as the opposite side of the front facing one.
> > +  2 - External. The image sensor is connected to the device by extension cables,
> > +  and can be freely moved, regardless of the device position.
>
> It could be something else than cables (wireless possibly ?). I would
> phrase this along the lines of "The device is not attached directly to
> the [device], or is attached in a way that allows it to move to
> different locations."

Agreed, once we find a good term for [device] :)

>
> >
> >  Optional endpoint properties
> >  ----------------------------
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 02/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_LOCATION

[Jacopo Mondi]

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

Hi Laurent,

On Tue, Aug 27, 2019 at 03:50:08PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2019 at 11:23:28AM +0200, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_CAMERA_SENSOR_LOCATION camera
> > control. The newly added read-only control reports the camera device
> > mounting position.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../media/uapi/v4l/ext-ctrls-camera.rst       | 34 +++++++++++++++++++
> >  1 file changed, 34 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index 51c1d5c9eb00..ecf151f3f0f4 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -510,6 +510,40 @@ enum v4l2_scene_mode -
> >      value down. A value of zero stops the motion if one is in progress
> >      and has no effect otherwise.
> >
> > +``V4L2_CID_CAMERA_SENSOR_LOCATION (integer)``
> > +    This read-only control describes the camera sensor location by
> > +    reporting its mounting position on the device where the camera is
> > +    installed. The control value is constant and not modifiable by software
> > +    and its value is retrieved from the firmware interface by parsing the
> > +    'location' property.
>
> How the value is known to the driver is irrelevant in this context, I
> would drop "and its value ...".
>

I recall Sakari pointed out it was not mentioned the control value
comes from firmware in review of v1. Are you both ok in removing this?

> > This control is particularly meaningful for
> > +    devices which have a well defined orientation, such as phones, laptops
> > +    and portable devices as the camera location is expressed as a position
> > +    relative to the device intended usage orientation. In example, a camera
>
> s/In example/For example/
>
> > +    sensor installed on the user-facing side of a phone, a tablet or a
> > +    laptop device is said to be installed in the ``V4L2_LOCATION_FRONT``
> > +    location while camera sensors installed on the side opposed to the
>
> s/opposed to the front one/opposite the front/
>
> > +    front one are said to be installed in the ``V4L2_LOCATION_BACK``
> > +    location. Camera sensors connected to the device by extension cables
> > +    which are freely movable regardless of the device orientation, such as
> > +    webcams and digital cameras, are said to be have
> > +    ``V4L2_LOCATION_EXTERNAL`` location.
>
> Same comment as for the DT bindings regarding cables.
>
> > +
> > +
> > +
> > +.. flat-table::
> > +    :header-rows:  0
> > +    :stub-columns: 0
> > +
> > +    * - ``V4L2_LOCATION_FRONT``
> > +      - The camera sensor is located on the front side of the device.
> > +    * - ``V4L2_LOCATION_BACK``
> > +      - The camera sensor is located on the back side of the device.
> > +    * - ``V4L2_LOCATION_EXTERNAL``
> > +      - The camera sensor is connected by extension cables to the device and
> > +        it's freely movable.
>
> s/it's/is/
>
> > +
> > +
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Hans Verkuil]

On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> HI Hans,
> 
> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
>>> Report the native pixel array size and the crop bounds for the ov5670
>>> sensor driver.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
>>>  1 file changed, 20 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>>> index 2bc57e85f721..3e22fe9ccad1 100644
>>> --- a/drivers/media/i2c/ov5670.c
>>> +++ b/drivers/media/i2c/ov5670.c
>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>>>  	return 0;
>>>  }
>>>
>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
>>> +				struct v4l2_subdev_pad_config *cfg,
>>> +				struct v4l2_subdev_selection *sel)
>>> +{
>>> +	switch (sel->target) {
>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
>>> +		sel->r.left = 0;
>>> +		sel->r.top = 0;
>>> +		sel->r.width = 2592;
>>> +		sel->r.height = 1944;
>>
>> Why do you need this?
>>
> 
> I need to expose the pixel array size and the active pixel area size,
> and I interpreted the two above targets as the right place where to do
> so.

That didn't answer my question :-)

Why do you need to expose this? Who uses it for what purpose?

> 
> At least for NATIVE_SIZE the documentation seems to match my
> understanding:
> 
> "The native size of the device, e.g. a sensor’s pixel array. left and top
> fields are zero for this target."

Correct.

> 
> 
>> Since the format can change for this and the next driver I think CROP_BOUNDS
>> at least should match the current format.
>>
> 
> Ah, does it? It was not clear to me from the documentation, as it
> suggested to me that target actually identifies the active pixel area
> 
> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> crop bounds rectangle."
> 
> It does not mention format, should this be updated?

The problem is that for sensors it is indeed not clear.

In principle the crop bounds matches the resolution that the sensor uses
pre-scaling. So yes, that means that it is equal to the native size.

But many sensors have a discrete list of supported formats and it is not
clear how they map each format to the actual sensor. If it is clearly just
done through binning or averaging, then all is fine.

If the formats have different aspect ratios, then it becomes a bit more
difficult how to relate the crop bounds with the format since you may not
know to which sensor area the current format corresponds.

I have no clear answer for you, to be honest, but it can get tricky.

> 
> How would you report the two information I need?

It depends on my original question: what do you need this information for?

Regards,

	Hans

> 
>> I don't think this patch and the next have anything to do with the location/rotate
>> support. I would split it off from this series.
>>
> 
> Agreed, they were split in v1, maybe it has not been a wise move to
> make a single series out of them. I'll split again.
> 
> Thanks
>    j
> 
>> Regards,
>>
>> 	Hans
>>
>>> +		break;
>>> +	default:
>>> +		return -EINVAL;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +
>>>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>>>  {
>>>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
>>> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
>>>  	.enum_mbus_code = ov5670_enum_mbus_code,
>>>  	.get_fmt = ov5670_get_pad_format,
>>>  	.set_fmt = ov5670_set_pad_format,
>>> +	.get_selection = ov5670_get_selection,
>>>  	.enum_frame_size = ov5670_enum_frame_size,
>>>  };
>>>
>>>
>>

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Jacopo Mondi]

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

Hi Laurent,

On Tue, Aug 27, 2019 at 04:16:20PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2019 at 11:23:34AM +0200, Jacopo Mondi wrote:
> > Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> > function parses the device node and endpoint firmware properties to
> > which a v4l2 control is associated to and registers the control with the
> > provided handler.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
> >  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
> >  2 files changed, 87 insertions(+)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> > index 3bd1888787eb..669801fceb64 100644
> > --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> > +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> > @@ -25,6 +25,7 @@
> >  #include <linux/types.h>
> >
> >  #include <media/v4l2-async.h>
> > +#include <media/v4l2-ctrls.h>
> >  #include <media/v4l2-fwnode.h>
> >  #include <media/v4l2-subdev.h>
> >
> > @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >  }
> >  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >
> > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> > +				  struct v4l2_ctrl_handler *hdl,
> > +				  const struct v4l2_ctrl_ops *ctrl_ops)
>
> Passing the ctrl_ops is a bit annoying. Would there be a way to get the
> V4L2 control framework to accept NULL ops for read-only controls ?
>

That would require a core change that is imho outside the scope of
this patch

> > +{
> > +	u32 val;
> > +	int ret;
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> > +	if (!ret) {
> > +		switch (val) {
> > +		case V4L2_LOCATION_FRONT:
> > +		case V4L2_LOCATION_BACK:
> > +		case V4L2_LOCATION_EXTERNAL:
> > +			break;
> > +		default:
> > +			pr_warn("Unsupported location: %u\n", val);
>
> dev_warn() would be nicer. Is there a way we could either pass the
> struct device pointer, or maybe a subdev that would be populated with
> the device, fwnode and hdl pointers ?
>

I went for pr_ as in all other functions of this file it is used when
a fwnode_handle is passed as parameter. I could pass the device node,
but I'm not sure passing an already populated subdevice is a good
idea, as the subdevice ctrl_handler is usually populated after having
made sure all controls have been registered without errors.

> > +			return -EINVAL;
> > +		}
> > +
> > +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> > +			pr_debug("Skip control '%s': already registered",
> > +				 v4l2_ctrl_get_name(
> > +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> > +		else
> > +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> > +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> > +					  val, val, 1, val);
> > +	}
> > +
> > +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> > +	if (!ret) {
> > +		if (val > 360) {
> > +			pr_warn("Unsupported rotation: %u\n", val);
> > +			return -EINVAL;
> > +		}
>
> We need to define the range of allowed values in the control
> documentation. 360 doesn't seem very useful as it's equivalent to 0.
> A few possible options are [0, 360[, [-180, +180[ or ]-180, +180].
>

As asked to Hans, why should we restrict this? I don't see a use case
for a 73 degrees rotated camera, but why prevent it?

> > +
> > +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> > +			pr_debug("Skip control '%s': already registered",
> > +				 v4l2_ctrl_get_name(
> > +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> > +		else
> > +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> > +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> > +					  val, val, 1, val);
> > +	}
> > +
> > +	if (hdl->error) {
> > +		pr_warn("Failed to register controls from firmware: %d\n",
> > +			hdl->error);
> > +		return hdl->error;
> > +	}
> > +
> > +	return 0;
> > +}
> > +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> > +
> >  static int
> >  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >  					  struct v4l2_async_notifier *notifier,
> > diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> > index f6a7bcd13197..0dad6968bde9 100644
> > --- a/include/media/v4l2-fwnode.h
> > +++ b/include/media/v4l2-fwnode.h
> > @@ -25,6 +25,8 @@
> >  struct fwnode_handle;
> >  struct v4l2_async_notifier;
> >  struct v4l2_async_subdev;
> > +struct v4l2_ctrl_handler;
> > +struct v4l2_ctrl_ops;
> >
> >  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
> >
> > @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >   */
> >  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >
> > +/**
> > + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> > + *				     properties and register a v4l2 control
> > + *				     for each of them
>
> I don't think that description is accurate.
>

How so? The function parses the device (not yet endpoint, right)
properties and register a control for each of them.

How would you phrase this?

Thanks
  j

> > + * @fwnode: pointer to the device fwnode handle
> > + * @hdl: pointer to the v4l2 control handler to register controls with
> > + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> > + *
> > + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> > + * is associated and register them with the provided handler @hdl.
> > + * Currently the following v4l2 controls are parsed and registered:
> > + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> > + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> > + *
> > + * Controls already registered by the caller with the @hdl control handler are
> > + * not overwritten. Callers should register the controls they want to handle
> > + * themselves before calling this function.
> > + *
> > + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> > + * not hold the lock when calling this function.
> > + *
> > + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> > + * specified.
> > + */
> > +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> > +				  struct v4l2_ctrl_handler *hdl,
> > +				  const struct v4l2_ctrl_ops *ctrl_ops);
> > +
> >  /**
> >   * typedef parse_endpoint_func - Driver's callback function to be called on
> >   *	each V4L2 fwnode endpoint.
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Hans Verkuil]

On 8/29/19 2:45 PM, Jacopo Mondi wrote:
> Hi Hans,
> 
> On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
>>> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
>>> function parses the device node and endpoint firmware properties to
>>> which a v4l2 control is associated to and registers the control with the
>>> provided handler.
>>>
>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>> ---
>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
>>>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
>>>  2 files changed, 87 insertions(+)
>>>
>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> index 3bd1888787eb..669801fceb64 100644
>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>> @@ -25,6 +25,7 @@
>>>  #include <linux/types.h>
>>>
>>>  #include <media/v4l2-async.h>
>>> +#include <media/v4l2-ctrls.h>
>>>  #include <media/v4l2-fwnode.h>
>>>  #include <media/v4l2-subdev.h>
>>>
>>> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>>>  }
>>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>>>
>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
>>> +				  struct v4l2_ctrl_handler *hdl,
>>> +				  const struct v4l2_ctrl_ops *ctrl_ops)
>>
>> I'm not convinced that this helper is a good idea.
>>
>> A helper that parses and validates this information makes sense,
>> but combining that with creating the controls feels wrong to me.
>>
>> You're mixing two very different things in one function.
>>
>> I think something like this would work better in a driver:
>>
>> 	if (!v4l2_fwnode_parse_location(&val))
>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
>> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
>> 				  val, val, 1, val);
>> 	if (!v4l2_fwnode_parse_rotation(&val))
>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
>> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
>> 				  val, val, 1, val);
>>
>> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
>> function prototypes!)
>>
> 
> Could the control registration being conditional on the presence of
> the *hdl parameter otherwise, or would you split the two operations
> (property parsing and control registration) nonetheless ?

Split it. My main problem with this helper is that it is mixing two
frameworks. Most of Laurent's comments on this patch just go away if
you leave the control creation to the driver.

It really isn't much code, and it is much easier to review a driver
if all the controls are created in the same place instead of some
controls being magically created in a helper function.

> 
> An helper was suggested in the v1 review, Laurent, Sakari, what do you
> think here?
> 
>>> +{
>>> +	u32 val;
>>> +	int ret;
>>> +
>>> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
>>> +	if (!ret) {
>>> +		switch (val) {
>>> +		case V4L2_LOCATION_FRONT:
>>> +		case V4L2_LOCATION_BACK:
>>> +		case V4L2_LOCATION_EXTERNAL:
>>> +			break;
>>> +		default:
>>> +			pr_warn("Unsupported location: %u\n", val);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
>>> +			pr_debug("Skip control '%s': already registered",
>>> +				 v4l2_ctrl_get_name(
>>> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
>>> +		else
>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
>>> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
>>> +					  val, val, 1, val);
>>> +	}
>>> +
>>> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
>>> +	if (!ret) {
>>> +		if (val > 360) {
>>
>> I'd add '|| val % 90' to this condition.
>>
> 
> Do we want to enforce this? I can't imagine any use case, but why a
> camera cannot be rotated of an arbitrary number of degrees ?

I would start out by enforcing this until someone comes up with a
realistic use-case.

As long as it is a multiple of 90 degree, then there is a clear interaction
with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing
and I'd rather avoid that for now.

Regards,

	Hans

> 
> Thanks
>    j
>> Regards,
>>
>> 	Hans
>>
>>> +			pr_warn("Unsupported rotation: %u\n", val);
>>> +			return -EINVAL;
>>> +		}
>>> +
>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
>>> +			pr_debug("Skip control '%s': already registered",
>>> +				 v4l2_ctrl_get_name(
>>> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
>>> +		else
>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
>>> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
>>> +					  val, val, 1, val);
>>> +	}
>>> +
>>> +	if (hdl->error) {
>>> +		pr_warn("Failed to register controls from firmware: %d\n",
>>> +			hdl->error);
>>> +		return hdl->error;
>>> +	}
>>> +
>>> +	return 0;
>>> +}
>>> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
>>> +
>>>  static int
>>>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>>>  					  struct v4l2_async_notifier *notifier,
>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>> index f6a7bcd13197..0dad6968bde9 100644
>>> --- a/include/media/v4l2-fwnode.h
>>> +++ b/include/media/v4l2-fwnode.h
>>> @@ -25,6 +25,8 @@
>>>  struct fwnode_handle;
>>>  struct v4l2_async_notifier;
>>>  struct v4l2_async_subdev;
>>> +struct v4l2_ctrl_handler;
>>> +struct v4l2_ctrl_ops;
>>>
>>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
>>>
>>> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>>>   */
>>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>>>
>>> +/**
>>> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
>>> + *				     properties and register a v4l2 control
>>> + *				     for each of them
>>> + * @fwnode: pointer to the device fwnode handle
>>> + * @hdl: pointer to the v4l2 control handler to register controls with
>>> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
>>> + *
>>> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
>>> + * is associated and register them with the provided handler @hdl.
>>> + * Currently the following v4l2 controls are parsed and registered:
>>> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
>>> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
>>> + *
>>> + * Controls already registered by the caller with the @hdl control handler are
>>> + * not overwritten. Callers should register the controls they want to handle
>>> + * themselves before calling this function.
>>> + *
>>> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
>>> + * not hold the lock when calling this function.
>>> + *
>>> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
>>> + * specified.
>>> + */
>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
>>> +				  struct v4l2_ctrl_handler *hdl,
>>> +				  const struct v4l2_ctrl_ops *ctrl_ops);
>>> +
>>>  /**
>>>   * typedef parse_endpoint_func - Driver's callback function to be called on
>>>   *	each V4L2 fwnode endpoint.
>>>
>>

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

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

Hi Hans

On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> > HI Hans,
> >
> > On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> >> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>> Report the native pixel array size and the crop bounds for the ov5670
> >>> sensor driver.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> >>>  1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> >>> index 2bc57e85f721..3e22fe9ccad1 100644
> >>> --- a/drivers/media/i2c/ov5670.c
> >>> +++ b/drivers/media/i2c/ov5670.c
> >>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> >>> +				struct v4l2_subdev_pad_config *cfg,
> >>> +				struct v4l2_subdev_selection *sel)
> >>> +{
> >>> +	switch (sel->target) {
> >>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> >>> +		sel->r.left = 0;
> >>> +		sel->r.top = 0;
> >>> +		sel->r.width = 2592;
> >>> +		sel->r.height = 1944;
> >>
> >> Why do you need this?
> >>
> >
> > I need to expose the pixel array size and the active pixel area size,
> > and I interpreted the two above targets as the right place where to do
> > so.
>
> That didn't answer my question :-)
>
> Why do you need to expose this? Who uses it for what purpose?
>

Ah, ok :)

Quoting the cover letter of the series:

Retrieving the following camera static information is a requirement for the
implementation of the Android-compatiblity layer of libcamera, but I'm sure
it might prove useful for other user-space applications and libraries as well.

In other words, in order to report to the android camera stack those
two information (among -many- others) they should be somehow retrieved
from the kernel interface, and after considering adding two more
read-only controls to expose them as I did for rotation and location,
I went for using the selection API.

> >
> > At least for NATIVE_SIZE the documentation seems to match my
> > understanding:
> >
> > "The native size of the device, e.g. a sensor’s pixel array. left and top
> > fields are zero for this target."
>
> Correct.
>
> >
> >
> >> Since the format can change for this and the next driver I think CROP_BOUNDS
> >> at least should match the current format.
> >>
> >
> > Ah, does it? It was not clear to me from the documentation, as it
> > suggested to me that target actually identifies the active pixel area
> >
> > "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> > crop bounds rectangle."
> >
> > It does not mention format, should this be updated?
>
> The problem is that for sensors it is indeed not clear.
>
> In principle the crop bounds matches the resolution that the sensor uses
> pre-scaling. So yes, that means that it is equal to the native size.

native size != active areas size.
Did you mean the latter here?

>
> But many sensors have a discrete list of supported formats and it is not
> clear how they map each format to the actual sensor. If it is clearly just
> done through binning or averaging, then all is fine.
>
> If the formats have different aspect ratios, then it becomes a bit more
> difficult how to relate the crop bounds with the format since you may not
> know to which sensor area the current format corresponds.

I see. We've had the same discussion in the libcamera list, as indeed
the crop bounds might depend on the aspect ratio as you said.

Maybe the crop_bound target is not the right place to report the
active area, or maybe the concept of active area is ill-defined if
the sensor is cross-shaped:

    /--------------------/ -> Pixel array size
    ----------------------
    |x    |        |--- x|------> 4:3 crop bounds
    |x------------------x|
    |x|   |        |   |x|
    |x|   |        |   |x|------> 21:9 crop bounds
    |x|   |        |   |x|
    |x------------------x|
    |x    |________|    x|
    ----------------------
      |                 |
      ---------------------------> Black pixels colums

>
> I have no clear answer for you, to be honest, but it can get tricky.
>

Indeed, but assuming a simpler square-shaped sensor, is the crop bound
target the right one to report the active pixel area ?

> >
> > How would you report the two information I need?
>
> It depends on my original question: what do you need this information for?
>

Please note that it's for the android camera stack in this case, but
it's an information that userspace might want to know for several
different reasons. Calibration and FOV calculation come to mind. Does
this makes sense to you?

Thanks
   j

> Regards,
>
> 	Hans
>
> >
> >> I don't think this patch and the next have anything to do with the location/rotate
> >> support. I would split it off from this series.
> >>
> >
> > Agreed, they were split in v1, maybe it has not been a wise move to
> > make a single series out of them. I'll split again.
> >
> > Thanks
> >    j
> >
> >> Regards,
> >>
> >> 	Hans
> >>
> >>> +		break;
> >>> +	default:
> >>> +		return -EINVAL;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +
> >>>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> >>>  {
> >>>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
> >>> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> >>>  	.enum_mbus_code = ov5670_enum_mbus_code,
> >>>  	.get_fmt = ov5670_get_pad_format,
> >>>  	.set_fmt = ov5670_set_pad_format,
> >>> +	.get_selection = ov5670_get_selection,
> >>>  	.enum_frame_size = ov5670_enum_frame_size,
> >>>  };
> >>>
> >>>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Hans Verkuil]

On 8/29/19 3:18 PM, Jacopo Mondi wrote:
> Hi Hans
> 
> On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
>> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
>>> HI Hans,
>>>
>>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
>>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
>>>>> Report the native pixel array size and the crop bounds for the ov5670
>>>>> sensor driver.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
>>>>>  1 file changed, 20 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>>>>> index 2bc57e85f721..3e22fe9ccad1 100644
>>>>> --- a/drivers/media/i2c/ov5670.c
>>>>> +++ b/drivers/media/i2c/ov5670.c
>>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>>>>>  	return 0;
>>>>>  }
>>>>>
>>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
>>>>> +				struct v4l2_subdev_pad_config *cfg,
>>>>> +				struct v4l2_subdev_selection *sel)
>>>>> +{
>>>>> +	switch (sel->target) {
>>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
>>>>> +		sel->r.left = 0;
>>>>> +		sel->r.top = 0;
>>>>> +		sel->r.width = 2592;
>>>>> +		sel->r.height = 1944;
>>>>
>>>> Why do you need this?
>>>>
>>>
>>> I need to expose the pixel array size and the active pixel area size,
>>> and I interpreted the two above targets as the right place where to do
>>> so.
>>
>> That didn't answer my question :-)
>>
>> Why do you need to expose this? Who uses it for what purpose?
>>
> 
> Ah, ok :)
> 
> Quoting the cover letter of the series:
> 
> Retrieving the following camera static information is a requirement for the
> implementation of the Android-compatiblity layer of libcamera, but I'm sure
> it might prove useful for other user-space applications and libraries as well.
> 
> In other words, in order to report to the android camera stack those
> two information (among -many- others) they should be somehow retrieved
> from the kernel interface, and after considering adding two more
> read-only controls to expose them as I did for rotation and location,
> I went for using the selection API.
> 
>>>
>>> At least for NATIVE_SIZE the documentation seems to match my
>>> understanding:
>>>
>>> "The native size of the device, e.g. a sensor’s pixel array. left and top
>>> fields are zero for this target."
>>
>> Correct.
>>
>>>
>>>
>>>> Since the format can change for this and the next driver I think CROP_BOUNDS
>>>> at least should match the current format.
>>>>
>>>
>>> Ah, does it? It was not clear to me from the documentation, as it
>>> suggested to me that target actually identifies the active pixel area
>>>
>>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
>>> crop bounds rectangle."
>>>
>>> It does not mention format, should this be updated?
>>
>> The problem is that for sensors it is indeed not clear.
>>
>> In principle the crop bounds matches the resolution that the sensor uses
>> pre-scaling. So yes, that means that it is equal to the native size.
> 
> native size != active areas size.
> Did you mean the latter here?

I mean the latter, but I have to say that the spec doesn't make it clear
if TGT_NATIVE_SIZE equals the active area or also includes inactive pixels.

What exactly does Android expect?

> 
>>
>> But many sensors have a discrete list of supported formats and it is not
>> clear how they map each format to the actual sensor. If it is clearly just
>> done through binning or averaging, then all is fine.
>>
>> If the formats have different aspect ratios, then it becomes a bit more
>> difficult how to relate the crop bounds with the format since you may not
>> know to which sensor area the current format corresponds.
> 
> I see. We've had the same discussion in the libcamera list, as indeed
> the crop bounds might depend on the aspect ratio as you said.
> 
> Maybe the crop_bound target is not the right place to report the
> active area, or maybe the concept of active area is ill-defined if
> the sensor is cross-shaped:
> 
>     /--------------------/ -> Pixel array size
>     ----------------------
>     |x    |        |--- x|------> 4:3 crop bounds
>     |x------------------x|
>     |x|   |        |   |x|
>     |x|   |        |   |x|------> 21:9 crop bounds
>     |x|   |        |   |x|
>     |x------------------x|
>     |x    |________|    x|
>     ----------------------
>       |                 |
>       ---------------------------> Black pixels colums
> 
>>
>> I have no clear answer for you, to be honest, but it can get tricky.
>>
> 
> Indeed, but assuming a simpler square-shaped sensor, is the crop bound
> target the right one to report the active pixel area ?

I don't think so.

The crop bounds defines the outer bounds of the area where you can put
your crop rectangle. This can very well include areas where the pixels
are covered and so always return black (sometimes used to obtain black
levels, I believe).

The default crop rectangle would be the one that reports the active
area. The native size rectangle would be the full pixel array.

So CROP_DEFAULT <= CROP_BOUNDS <= NATIVE_SIZE.

For a cross-shaped sensor I would expect that the CROP_BOUNDS/DEFAULT
depends on the format (aspect ratio).

I think this makes sense.

The specification definitely needs to be improved, patches are welcome...

> 
>>>
>>> How would you report the two information I need?
>>
>> It depends on my original question: what do you need this information for?
>>
> 
> Please note that it's for the android camera stack in this case, but
> it's an information that userspace might want to know for several
> different reasons. Calibration and FOV calculation come to mind. Does
> this makes sense to you?

Ah, so that's what it is used for :-)

Which of the three targets above would match with what Android needs?

Regards,

	Hans

> 
> Thanks
>    j
> 
>> Regards,
>>
>> 	Hans
>>
>>>
>>>> I don't think this patch and the next have anything to do with the location/rotate
>>>> support. I would split it off from this series.
>>>>
>>>
>>> Agreed, they were split in v1, maybe it has not been a wise move to
>>> make a single series out of them. I'll split again.
>>>
>>> Thanks
>>>    j
>>>
>>>> Regards,
>>>>
>>>> 	Hans
>>>>
>>>>> +		break;
>>>>> +	default:
>>>>> +		return -EINVAL;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +
>>>>>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
>>>>>  {
>>>>>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
>>>>> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
>>>>>  	.enum_mbus_code = ov5670_enum_mbus_code,
>>>>>  	.get_fmt = ov5670_get_pad_format,
>>>>>  	.set_fmt = ov5670_set_pad_format,
>>>>> +	.get_selection = ov5670_get_selection,
>>>>>  	.enum_frame_size = ov5670_enum_frame_size,
>>>>>  };
>>>>>
>>>>>
>>>>
>>

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Laurent Pinchart]

Hi Hans,

On Thu, Aug 29, 2019 at 03:04:42PM +0200, Hans Verkuil wrote:
> On 8/29/19 2:45 PM, Jacopo Mondi wrote:
> > On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
> >> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> >>> function parses the device node and endpoint firmware properties to
> >>> which a v4l2 control is associated to and registers the control with the
> >>> provided handler.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
> >>>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
> >>>  2 files changed, 87 insertions(+)
> >>>
> >>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> index 3bd1888787eb..669801fceb64 100644
> >>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>> @@ -25,6 +25,7 @@
> >>>  #include <linux/types.h>
> >>>
> >>>  #include <media/v4l2-async.h>
> >>> +#include <media/v4l2-ctrls.h>
> >>>  #include <media/v4l2-fwnode.h>
> >>>  #include <media/v4l2-subdev.h>
> >>>
> >>> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >>>  }
> >>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >>>
> >>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>> +				  struct v4l2_ctrl_handler *hdl,
> >>> +				  const struct v4l2_ctrl_ops *ctrl_ops)
> >>
> >> I'm not convinced that this helper is a good idea.
> >>
> >> A helper that parses and validates this information makes sense,
> >> but combining that with creating the controls feels wrong to me.
> >>
> >> You're mixing two very different things in one function.
> >>
> >> I think something like this would work better in a driver:
> >>
> >> 	if (!v4l2_fwnode_parse_location(&val))
> >> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >> 				  val, val, 1, val);
> >> 	if (!v4l2_fwnode_parse_rotation(&val))
> >> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >> 				  val, val, 1, val);
> >>
> >> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
> >> function prototypes!)
> >>
> > 
> > Could the control registration being conditional on the presence of
> > the *hdl parameter otherwise, or would you split the two operations
> > (property parsing and control registration) nonetheless ?
> 
> Split it. My main problem with this helper is that it is mixing two
> frameworks. Most of Laurent's comments on this patch just go away if
> you leave the control creation to the driver.
> 
> It really isn't much code, and it is much easier to review a driver
> if all the controls are created in the same place instead of some
> controls being magically created in a helper function.

But this would require copying the above code in every single camera
sensor driver. Furthermore, the helper proposed by Jacopo would make
addition of new firmware properties much simpler, as we wouldn't need to
modify all sensor drivers.

V4L2 requires lots of boilerplate code for sensor drivers, and I think
more helper would be useful. There's really not point in doing the same
thing slightly differently in dozens of drivers. Maybe we could
experiment with a v4l2_camera_subdev structure ?

> > An helper was suggested in the v1 review, Laurent, Sakari, what do you
> > think here?
> > 
> >>> +{
> >>> +	u32 val;
> >>> +	int ret;
> >>> +
> >>> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> >>> +	if (!ret) {
> >>> +		switch (val) {
> >>> +		case V4L2_LOCATION_FRONT:
> >>> +		case V4L2_LOCATION_BACK:
> >>> +		case V4L2_LOCATION_EXTERNAL:
> >>> +			break;
> >>> +		default:
> >>> +			pr_warn("Unsupported location: %u\n", val);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> >>> +			pr_debug("Skip control '%s': already registered",
> >>> +				 v4l2_ctrl_get_name(
> >>> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> >>> +		else
> >>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>> +					  val, val, 1, val);
> >>> +	}
> >>> +
> >>> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> >>> +	if (!ret) {
> >>> +		if (val > 360) {
> >>
> >> I'd add '|| val % 90' to this condition.
> > 
> > Do we want to enforce this? I can't imagine any use case, but why a
> > camera cannot be rotated of an arbitrary number of degrees ?
> 
> I would start out by enforcing this until someone comes up with a
> realistic use-case.
> 
> As long as it is a multiple of 90 degree, then there is a clear interaction
> with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing
> and I'd rather avoid that for now.

If we enfore this, then let's update the DT bindings accordingly. yaml
would help with validation ;-)

> >>> +			pr_warn("Unsupported rotation: %u\n", val);
> >>> +			return -EINVAL;
> >>> +		}
> >>> +
> >>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> >>> +			pr_debug("Skip control '%s': already registered",
> >>> +				 v4l2_ctrl_get_name(
> >>> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> >>> +		else
> >>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>> +					  val, val, 1, val);
> >>> +	}
> >>> +
> >>> +	if (hdl->error) {
> >>> +		pr_warn("Failed to register controls from firmware: %d\n",
> >>> +			hdl->error);
> >>> +		return hdl->error;
> >>> +	}
> >>> +
> >>> +	return 0;
> >>> +}
> >>> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> >>> +
> >>>  static int
> >>>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >>>  					  struct v4l2_async_notifier *notifier,
> >>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> >>> index f6a7bcd13197..0dad6968bde9 100644
> >>> --- a/include/media/v4l2-fwnode.h
> >>> +++ b/include/media/v4l2-fwnode.h
> >>> @@ -25,6 +25,8 @@
> >>>  struct fwnode_handle;
> >>>  struct v4l2_async_notifier;
> >>>  struct v4l2_async_subdev;
> >>> +struct v4l2_ctrl_handler;
> >>> +struct v4l2_ctrl_ops;
> >>>
> >>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
> >>>
> >>> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >>>   */
> >>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >>>
> >>> +/**
> >>> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> >>> + *				     properties and register a v4l2 control
> >>> + *				     for each of them
> >>> + * @fwnode: pointer to the device fwnode handle
> >>> + * @hdl: pointer to the v4l2 control handler to register controls with
> >>> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> >>> + *
> >>> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> >>> + * is associated and register them with the provided handler @hdl.
> >>> + * Currently the following v4l2 controls are parsed and registered:
> >>> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> >>> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> >>> + *
> >>> + * Controls already registered by the caller with the @hdl control handler are
> >>> + * not overwritten. Callers should register the controls they want to handle
> >>> + * themselves before calling this function.
> >>> + *
> >>> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> >>> + * not hold the lock when calling this function.
> >>> + *
> >>> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> >>> + * specified.
> >>> + */
> >>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>> +				  struct v4l2_ctrl_handler *hdl,
> >>> +				  const struct v4l2_ctrl_ops *ctrl_ops);
> >>> +
> >>>  /**
> >>>   * typedef parse_endpoint_func - Driver's callback function to be called on
> >>>   *	each V4L2 fwnode endpoint.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Hans Verkuil]

On 8/29/19 5:05 PM, Laurent Pinchart wrote:
> Hi Hans,
> 
> On Thu, Aug 29, 2019 at 03:04:42PM +0200, Hans Verkuil wrote:
>> On 8/29/19 2:45 PM, Jacopo Mondi wrote:
>>> On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
>>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
>>>>> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
>>>>> function parses the device node and endpoint firmware properties to
>>>>> which a v4l2 control is associated to and registers the control with the
>>>>> provided handler.
>>>>>
>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>> ---
>>>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
>>>>>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
>>>>>  2 files changed, 87 insertions(+)
>>>>>
>>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>> index 3bd1888787eb..669801fceb64 100644
>>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
>>>>> @@ -25,6 +25,7 @@
>>>>>  #include <linux/types.h>
>>>>>
>>>>>  #include <media/v4l2-async.h>
>>>>> +#include <media/v4l2-ctrls.h>
>>>>>  #include <media/v4l2-fwnode.h>
>>>>>  #include <media/v4l2-subdev.h>
>>>>>
>>>>> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
>>>>>  }
>>>>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
>>>>>
>>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
>>>>> +				  struct v4l2_ctrl_handler *hdl,
>>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops)
>>>>
>>>> I'm not convinced that this helper is a good idea.
>>>>
>>>> A helper that parses and validates this information makes sense,
>>>> but combining that with creating the controls feels wrong to me.
>>>>
>>>> You're mixing two very different things in one function.
>>>>
>>>> I think something like this would work better in a driver:
>>>>
>>>> 	if (!v4l2_fwnode_parse_location(&val))
>>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
>>>> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
>>>> 				  val, val, 1, val);
>>>> 	if (!v4l2_fwnode_parse_rotation(&val))
>>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
>>>> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
>>>> 				  val, val, 1, val);
>>>>
>>>> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
>>>> function prototypes!)
>>>>
>>>
>>> Could the control registration being conditional on the presence of
>>> the *hdl parameter otherwise, or would you split the two operations
>>> (property parsing and control registration) nonetheless ?
>>
>> Split it. My main problem with this helper is that it is mixing two
>> frameworks. Most of Laurent's comments on this patch just go away if
>> you leave the control creation to the driver.
>>
>> It really isn't much code, and it is much easier to review a driver
>> if all the controls are created in the same place instead of some
>> controls being magically created in a helper function.
> 
> But this would require copying the above code in every single camera
> sensor driver. Furthermore, the helper proposed by Jacopo would make
> addition of new firmware properties much simpler, as we wouldn't need to
> modify all sensor drivers.
> 
> V4L2 requires lots of boilerplate code for sensor drivers, and I think
> more helper would be useful. There's really not point in doing the same
> thing slightly differently in dozens of drivers. Maybe we could
> experiment with a v4l2_camera_subdev structure ?

You can make one v4l2_fwnode helper that parses all the sensor properties,
and another helper in v4l2-ctrl.c or v4l2-common.c that uses the parsed
result to create the controls. That way if the driver needs to do something
unusual with the controls it can just create them manually.

But don't mix the two in one helper.

Note that creating some more advanced framework for sensors wouldn't hurt
since it's a bit mess at the moment IMHO.

Regards,

	Hans

> 
>>> An helper was suggested in the v1 review, Laurent, Sakari, what do you
>>> think here?
>>>
>>>>> +{
>>>>> +	u32 val;
>>>>> +	int ret;
>>>>> +
>>>>> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
>>>>> +	if (!ret) {
>>>>> +		switch (val) {
>>>>> +		case V4L2_LOCATION_FRONT:
>>>>> +		case V4L2_LOCATION_BACK:
>>>>> +		case V4L2_LOCATION_EXTERNAL:
>>>>> +			break;
>>>>> +		default:
>>>>> +			pr_warn("Unsupported location: %u\n", val);
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
>>>>> +			pr_debug("Skip control '%s': already registered",
>>>>> +				 v4l2_ctrl_get_name(
>>>>> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
>>>>> +		else
>>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
>>>>> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
>>>>> +					  val, val, 1, val);
>>>>> +	}
>>>>> +
>>>>> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
>>>>> +	if (!ret) {
>>>>> +		if (val > 360) {
>>>>
>>>> I'd add '|| val % 90' to this condition.
>>>
>>> Do we want to enforce this? I can't imagine any use case, but why a
>>> camera cannot be rotated of an arbitrary number of degrees ?
>>
>> I would start out by enforcing this until someone comes up with a
>> realistic use-case.
>>
>> As long as it is a multiple of 90 degree, then there is a clear interaction
>> with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing
>> and I'd rather avoid that for now.
> 
> If we enfore this, then let's update the DT bindings accordingly. yaml
> would help with validation ;-)
> 
>>>>> +			pr_warn("Unsupported rotation: %u\n", val);
>>>>> +			return -EINVAL;
>>>>> +		}
>>>>> +
>>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
>>>>> +			pr_debug("Skip control '%s': already registered",
>>>>> +				 v4l2_ctrl_get_name(
>>>>> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
>>>>> +		else
>>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
>>>>> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
>>>>> +					  val, val, 1, val);
>>>>> +	}
>>>>> +
>>>>> +	if (hdl->error) {
>>>>> +		pr_warn("Failed to register controls from firmware: %d\n",
>>>>> +			hdl->error);
>>>>> +		return hdl->error;
>>>>> +	}
>>>>> +
>>>>> +	return 0;
>>>>> +}
>>>>> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
>>>>> +
>>>>>  static int
>>>>>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
>>>>>  					  struct v4l2_async_notifier *notifier,
>>>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
>>>>> index f6a7bcd13197..0dad6968bde9 100644
>>>>> --- a/include/media/v4l2-fwnode.h
>>>>> +++ b/include/media/v4l2-fwnode.h
>>>>> @@ -25,6 +25,8 @@
>>>>>  struct fwnode_handle;
>>>>>  struct v4l2_async_notifier;
>>>>>  struct v4l2_async_subdev;
>>>>> +struct v4l2_ctrl_handler;
>>>>> +struct v4l2_ctrl_ops;
>>>>>
>>>>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
>>>>>
>>>>> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
>>>>>   */
>>>>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
>>>>>
>>>>> +/**
>>>>> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
>>>>> + *				     properties and register a v4l2 control
>>>>> + *				     for each of them
>>>>> + * @fwnode: pointer to the device fwnode handle
>>>>> + * @hdl: pointer to the v4l2 control handler to register controls with
>>>>> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
>>>>> + *
>>>>> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
>>>>> + * is associated and register them with the provided handler @hdl.
>>>>> + * Currently the following v4l2 controls are parsed and registered:
>>>>> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
>>>>> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
>>>>> + *
>>>>> + * Controls already registered by the caller with the @hdl control handler are
>>>>> + * not overwritten. Callers should register the controls they want to handle
>>>>> + * themselves before calling this function.
>>>>> + *
>>>>> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
>>>>> + * not hold the lock when calling this function.
>>>>> + *
>>>>> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
>>>>> + * specified.
>>>>> + */
>>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
>>>>> +				  struct v4l2_ctrl_handler *hdl,
>>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops);
>>>>> +
>>>>>  /**
>>>>>   * typedef parse_endpoint_func - Driver's callback function to be called on
>>>>>   *	each V4L2 fwnode endpoint.
> 

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

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

Hi Hans,

On Thu, Aug 29, 2019 at 03:39:43PM +0200, Hans Verkuil wrote:
> On 8/29/19 3:18 PM, Jacopo Mondi wrote:
> > Hi Hans
> >
> > On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> >> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> >>> HI Hans,
> >>>
> >>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> >>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>>>> Report the native pixel array size and the crop bounds for the ov5670
> >>>>> sensor driver.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> >>>>>  1 file changed, 20 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> >>>>> index 2bc57e85f721..3e22fe9ccad1 100644
> >>>>> --- a/drivers/media/i2c/ov5670.c
> >>>>> +++ b/drivers/media/i2c/ov5670.c
> >>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> >>>>>  	return 0;
> >>>>>  }
> >>>>>
> >>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> >>>>> +				struct v4l2_subdev_pad_config *cfg,
> >>>>> +				struct v4l2_subdev_selection *sel)
> >>>>> +{
> >>>>> +	switch (sel->target) {
> >>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >>>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> >>>>> +		sel->r.left = 0;
> >>>>> +		sel->r.top = 0;
> >>>>> +		sel->r.width = 2592;
> >>>>> +		sel->r.height = 1944;
> >>>>
> >>>> Why do you need this?
> >>>>
> >>>
> >>> I need to expose the pixel array size and the active pixel area size,
> >>> and I interpreted the two above targets as the right place where to do
> >>> so.
> >>
> >> That didn't answer my question :-)
> >>
> >> Why do you need to expose this? Who uses it for what purpose?
> >>
> >
> > Ah, ok :)
> >
> > Quoting the cover letter of the series:
> >
> > Retrieving the following camera static information is a requirement for the
> > implementation of the Android-compatiblity layer of libcamera, but I'm sure
> > it might prove useful for other user-space applications and libraries as well.
> >
> > In other words, in order to report to the android camera stack those
> > two information (among -many- others) they should be somehow retrieved
> > from the kernel interface, and after considering adding two more
> > read-only controls to expose them as I did for rotation and location,
> > I went for using the selection API.
> >
> >>>
> >>> At least for NATIVE_SIZE the documentation seems to match my
> >>> understanding:
> >>>
> >>> "The native size of the device, e.g. a sensor’s pixel array. left and top
> >>> fields are zero for this target."
> >>
> >> Correct.
> >>
> >>>
> >>>
> >>>> Since the format can change for this and the next driver I think CROP_BOUNDS
> >>>> at least should match the current format.
> >>>>
> >>>
> >>> Ah, does it? It was not clear to me from the documentation, as it
> >>> suggested to me that target actually identifies the active pixel area
> >>>
> >>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> >>> crop bounds rectangle."
> >>>
> >>> It does not mention format, should this be updated?
> >>
> >> The problem is that for sensors it is indeed not clear.
> >>
> >> In principle the crop bounds matches the resolution that the sensor uses
> >> pre-scaling. So yes, that means that it is equal to the native size.
> >
> > native size != active areas size.
> > Did you mean the latter here?
>
> I mean the latter, but I have to say that the spec doesn't make it clear
> if TGT_NATIVE_SIZE equals the active area or also includes inactive pixels.
>
> What exactly does Android expect?
>
Ah! You asked for this, so be prepared...

I started by trying to retrieve from the kernel interface the
following parameters:

The pixel array size
https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.pixelArraySize
The physical dimensions of the sensor (TODO)
https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.physicalSize
The active area size:
https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.activeArraySize
Sensor orientation:
https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.orientation
Location:
https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.facing

For very simple cameras there are not many more informations about
sensor/lens to be retrieved, but if you look at the end of the page you can
see a list of other informations Android requires for more advanced use case
(not so advanced actually, those are expected to be there for any
recent modern phone device I guess)

It's a long list, with many details about the sensor, lens and the
capabilities of the camera.

Just to provide an example list:
android.lens.info.availableApertures (static)
android.lens.info.availableFilterDensities (static)
android.lens.info.availableFocalLengths (static)
android.lens.info.availableOpticalStabilization (static)
android.lens.info.minimumFocusDistance (static)
android.lens.info.shadingMapSize (static)
android.lens.info.focusDistanceCalibration (static)
android.sensor.info.sensitivityRange (static)
android.sensor.info.exposureTimeRange (static)
android.sensor.info.maxFrameDuration (static)
android.sensor.info.physicalSize (static)
android.sensor.info.timestampSource (static)
android.sensor.maxAnalogSensitivity (static)

And they grow up to the point of describing the color transformation
matrices and other low level details, if your device claim to support
raw capture and manipulation:
android.sensor.info.colorFilterArrangement (static)
android.sensor.info.whiteLevel (static)
android.sensor.info.preCorrectionActiveArraySize (static)
android.sensor.referenceIlluminant1 (static)
android.sensor.referenceIlluminant2 (static)
android.sensor.calibrationTransform1 (static)
android.sensor.calibrationTransform2 (static)
android.sensor.colorTransform1 (static)
android.sensor.colorTransform2 (static)
android.sensor.forwardMatrix1 (static)
android.sensor.forwardMatrix2 (static)
android.sensor.blackLevelPattern (static)
android.sensor.profileHueSatMapDimensions (static)

So, going forward I cannot tell how much of this could actually come
from V4L, ideally all of this, but as of today it's not possible, so
vendors keep adding custom solutions to support this with out-of-tree
drivers or find solutions to generate these information from
configuration files in user space.

For libcamera this is not possible, it aims to build on top of
standard v4l2 interfaces, so I've started the process of adding small
bits to v4l2 to at least retrieve the most basic information.

> >
> >>
> >> But many sensors have a discrete list of supported formats and it is not
> >> clear how they map each format to the actual sensor. If it is clearly just
> >> done through binning or averaging, then all is fine.
> >>
> >> If the formats have different aspect ratios, then it becomes a bit more
> >> difficult how to relate the crop bounds with the format since you may not
> >> know to which sensor area the current format corresponds.
> >
> > I see. We've had the same discussion in the libcamera list, as indeed
> > the crop bounds might depend on the aspect ratio as you said.
> >
> > Maybe the crop_bound target is not the right place to report the
> > active area, or maybe the concept of active area is ill-defined if
> > the sensor is cross-shaped:
> >
> >     /--------------------/ -> Pixel array size
> >     ----------------------
> >     |x    |        |--- x|------> 4:3 crop bounds
> >     |x------------------x|
> >     |x|   |        |   |x|
> >     |x|   |        |   |x|------> 21:9 crop bounds
> >     |x|   |        |   |x|
> >     |x------------------x|
> >     |x    |________|    x|
> >     ----------------------
> >       |                 |
> >       ---------------------------> Black pixels colums
> >
> >>
> >> I have no clear answer for you, to be honest, but it can get tricky.
> >>
> >
> > Indeed, but assuming a simpler square-shaped sensor, is the crop bound
> > target the right one to report the active pixel area ?
>
> I don't think so.
>
> The crop bounds defines the outer bounds of the area where you can put
> your crop rectangle. This can very well include areas where the pixels
> are covered and so always return black (sometimes used to obtain black
> levels, I believe).

Ah, I didn't considered black pixels as part of a possible crop
selection..

>
> The default crop rectangle would be the one that reports the active
> area. The native size rectangle would be the full pixel array.
>
> So CROP_DEFAULT <= CROP_BOUNDS <= NATIVE_SIZE.
>

I see thanks for clarifying this

> For a cross-shaped sensor I would expect that the CROP_BOUNDS/DEFAULT
> depends on the format (aspect ratio).

Currently the selection API do not support to retrieve that
information depending on the aspect-ratio though
>
> I think this makes sense.
>
> The specification definitely needs to be improved, patches are welcome...
>
> >
> >>>
> >>> How would you report the two information I need?
> >>
> >> It depends on my original question: what do you need this information for?
> >>
> >
> > Please note that it's for the android camera stack in this case, but
> > it's an information that userspace might want to know for several
> > different reasons. Calibration and FOV calculation come to mind. Does
> > this makes sense to you?
>
> Ah, so that's what it is used for :-)
>
> Which of the three targets above would match with what Android needs?
>

the full pixel array size would be reported by the NATIVE_SIZE target
the active area size by the CROP_DEFAULT rectangle (regardless of the
aspect ratio)

It's still not clear to me what the CROP_BOUNDS rectangle would then
be used for, unless it is made to be dependent on an aspect ratio but
that would require userspace to provide a populated rectangle to
G_SELECTION and drivers to inspect it and return an opportune
CROP_BOUNDS one. Would this be acceptable ?

Thanks
  j

> Regards,
>
> 	Hans
>
> >
> > Thanks
> >    j
> >
> >> Regards,
> >>
> >> 	Hans
> >>
> >>>
> >>>> I don't think this patch and the next have anything to do with the location/rotate
> >>>> support. I would split it off from this series.
> >>>>
> >>>
> >>> Agreed, they were split in v1, maybe it has not been a wise move to
> >>> make a single series out of them. I'll split again.
> >>>
> >>> Thanks
> >>>    j
> >>>
> >>>> Regards,
> >>>>
> >>>> 	Hans
> >>>>
> >>>>> +		break;
> >>>>> +	default:
> >>>>> +		return -EINVAL;
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +
> >>>>>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> >>>>>  {
> >>>>>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
> >>>>> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> >>>>>  	.enum_mbus_code = ov5670_enum_mbus_code,
> >>>>>  	.get_fmt = ov5670_get_pad_format,
> >>>>>  	.set_fmt = ov5670_set_pad_format,
> >>>>> +	.get_selection = ov5670_get_selection,
> >>>>>  	.enum_frame_size = ov5670_enum_frame_size,
> >>>>>  };
> >>>>>
> >>>>>
> >>>>
> >>
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Jacopo Mondi]

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

Hi Hans, Laurent,

On Thu, Aug 29, 2019 at 05:32:52PM +0200, Hans Verkuil wrote:
> On 8/29/19 5:05 PM, Laurent Pinchart wrote:
> > Hi Hans,
> >
> > On Thu, Aug 29, 2019 at 03:04:42PM +0200, Hans Verkuil wrote:
> >> On 8/29/19 2:45 PM, Jacopo Mondi wrote:
> >>> On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
> >>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>>>> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> >>>>> function parses the device node and endpoint firmware properties to
> >>>>> which a v4l2 control is associated to and registers the control with the
> >>>>> provided handler.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
> >>>>>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
> >>>>>  2 files changed, 87 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> index 3bd1888787eb..669801fceb64 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> @@ -25,6 +25,7 @@
> >>>>>  #include <linux/types.h>
> >>>>>
> >>>>>  #include <media/v4l2-async.h>
> >>>>> +#include <media/v4l2-ctrls.h>
> >>>>>  #include <media/v4l2-fwnode.h>
> >>>>>  #include <media/v4l2-subdev.h>
> >>>>>
> >>>>> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >>>>>
> >>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>>>> +				  struct v4l2_ctrl_handler *hdl,
> >>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops)
> >>>>
> >>>> I'm not convinced that this helper is a good idea.
> >>>>
> >>>> A helper that parses and validates this information makes sense,
> >>>> but combining that with creating the controls feels wrong to me.
> >>>>
> >>>> You're mixing two very different things in one function.
> >>>>
> >>>> I think something like this would work better in a driver:
> >>>>
> >>>> 	if (!v4l2_fwnode_parse_location(&val))
> >>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>>> 				  val, val, 1, val);
> >>>> 	if (!v4l2_fwnode_parse_rotation(&val))
> >>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>>> 				  val, val, 1, val);
> >>>>
> >>>> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
> >>>> function prototypes!)
> >>>>
> >>>
> >>> Could the control registration being conditional on the presence of
> >>> the *hdl parameter otherwise, or would you split the two operations
> >>> (property parsing and control registration) nonetheless ?
> >>
> >> Split it. My main problem with this helper is that it is mixing two
> >> frameworks. Most of Laurent's comments on this patch just go away if
> >> you leave the control creation to the driver.
> >>
> >> It really isn't much code, and it is much easier to review a driver
> >> if all the controls are created in the same place instead of some
> >> controls being magically created in a helper function.
> >
> > But this would require copying the above code in every single camera
> > sensor driver. Furthermore, the helper proposed by Jacopo would make
> > addition of new firmware properties much simpler, as we wouldn't need to
> > modify all sensor drivers.
> >
> > V4L2 requires lots of boilerplate code for sensor drivers, and I think
> > more helper would be useful. There's really not point in doing the same
> > thing slightly differently in dozens of drivers. Maybe we could
> > experiment with a v4l2_camera_subdev structure ?
>
> You can make one v4l2_fwnode helper that parses all the sensor properties,
> and another helper in v4l2-ctrl.c or v4l2-common.c that uses the parsed
> result to create the controls. That way if the driver needs to do something
> unusual with the controls it can just create them manually.
>
> But don't mix the two in one helper.
>

I see, I like this better than a single helper per property, as this
is not going to scale well if the number of static properties
increases.

> Note that creating some more advanced framework for sensors wouldn't hurt
> since it's a bit mess at the moment IMHO.
>

I would start by defining a v4l2_sensor_properties structure, filled
by parsing the fwnode properties and then used by to register controls
with the informations there collected.

Thanks
   j
> Regards,
>
> 	Hans
>
> >
> >>> An helper was suggested in the v1 review, Laurent, Sakari, what do you
> >>> think here?
> >>>
> >>>>> +{
> >>>>> +	u32 val;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> >>>>> +	if (!ret) {
> >>>>> +		switch (val) {
> >>>>> +		case V4L2_LOCATION_FRONT:
> >>>>> +		case V4L2_LOCATION_BACK:
> >>>>> +		case V4L2_LOCATION_EXTERNAL:
> >>>>> +			break;
> >>>>> +		default:
> >>>>> +			pr_warn("Unsupported location: %u\n", val);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> >>>>> +			pr_debug("Skip control '%s': already registered",
> >>>>> +				 v4l2_ctrl_get_name(
> >>>>> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> >>>>> +		else
> >>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>>> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>>>> +					  val, val, 1, val);
> >>>>> +	}
> >>>>> +
> >>>>> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> >>>>> +	if (!ret) {
> >>>>> +		if (val > 360) {
> >>>>
> >>>> I'd add '|| val % 90' to this condition.
> >>>
> >>> Do we want to enforce this? I can't imagine any use case, but why a
> >>> camera cannot be rotated of an arbitrary number of degrees ?
> >>
> >> I would start out by enforcing this until someone comes up with a
> >> realistic use-case.
> >>
> >> As long as it is a multiple of 90 degree, then there is a clear interaction
> >> with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing
> >> and I'd rather avoid that for now.
> >
> > If we enfore this, then let's update the DT bindings accordingly. yaml
> > would help with validation ;-)
> >
> >>>>> +			pr_warn("Unsupported rotation: %u\n", val);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> >>>>> +			pr_debug("Skip control '%s': already registered",
> >>>>> +				 v4l2_ctrl_get_name(
> >>>>> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> >>>>> +		else
> >>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>>> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>>>> +					  val, val, 1, val);
> >>>>> +	}
> >>>>> +
> >>>>> +	if (hdl->error) {
> >>>>> +		pr_warn("Failed to register controls from firmware: %d\n",
> >>>>> +			hdl->error);
> >>>>> +		return hdl->error;
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> >>>>> +
> >>>>>  static int
> >>>>>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >>>>>  					  struct v4l2_async_notifier *notifier,
> >>>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> >>>>> index f6a7bcd13197..0dad6968bde9 100644
> >>>>> --- a/include/media/v4l2-fwnode.h
> >>>>> +++ b/include/media/v4l2-fwnode.h
> >>>>> @@ -25,6 +25,8 @@
> >>>>>  struct fwnode_handle;
> >>>>>  struct v4l2_async_notifier;
> >>>>>  struct v4l2_async_subdev;
> >>>>> +struct v4l2_ctrl_handler;
> >>>>> +struct v4l2_ctrl_ops;
> >>>>>
> >>>>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
> >>>>>
> >>>>> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >>>>>   */
> >>>>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >>>>>
> >>>>> +/**
> >>>>> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> >>>>> + *				     properties and register a v4l2 control
> >>>>> + *				     for each of them
> >>>>> + * @fwnode: pointer to the device fwnode handle
> >>>>> + * @hdl: pointer to the v4l2 control handler to register controls with
> >>>>> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> >>>>> + *
> >>>>> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> >>>>> + * is associated and register them with the provided handler @hdl.
> >>>>> + * Currently the following v4l2 controls are parsed and registered:
> >>>>> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> >>>>> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> >>>>> + *
> >>>>> + * Controls already registered by the caller with the @hdl control handler are
> >>>>> + * not overwritten. Callers should register the controls they want to handle
> >>>>> + * themselves before calling this function.
> >>>>> + *
> >>>>> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> >>>>> + * not hold the lock when calling this function.
> >>>>> + *
> >>>>> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> >>>>> + * specified.
> >>>>> + */
> >>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>>>> +				  struct v4l2_ctrl_handler *hdl,
> >>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops);
> >>>>> +
> >>>>>  /**
> >>>>>   * typedef parse_endpoint_func - Driver's callback function to be called on
> >>>>>   *	each V4L2 fwnode endpoint.
> >
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 06/10] media: v4l2-fwnode: Add helper to register controls from fw

[Laurent Pinchart]

Hi Hans,

On Thu, Aug 29, 2019 at 05:32:52PM +0200, Hans Verkuil wrote:
> On 8/29/19 5:05 PM, Laurent Pinchart wrote:
> > On Thu, Aug 29, 2019 at 03:04:42PM +0200, Hans Verkuil wrote:
> >> On 8/29/19 2:45 PM, Jacopo Mondi wrote:
> >>> On Thu, Aug 29, 2019 at 12:31:37PM +0200, Hans Verkuil wrote:
> >>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>>>> Add the 'v4l2_fwnode_register_controls()' helper to v4l2-fwnode. The
> >>>>> function parses the device node and endpoint firmware properties to
> >>>>> which a v4l2 control is associated to and registers the control with the
> >>>>> provided handler.
> >>>>>
> >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>> ---
> >>>>>  drivers/media/v4l2-core/v4l2-fwnode.c | 57 +++++++++++++++++++++++++++
> >>>>>  include/media/v4l2-fwnode.h           | 30 ++++++++++++++
> >>>>>  2 files changed, 87 insertions(+)
> >>>>>
> >>>>> diff --git a/drivers/media/v4l2-core/v4l2-fwnode.c b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> index 3bd1888787eb..669801fceb64 100644
> >>>>> --- a/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> +++ b/drivers/media/v4l2-core/v4l2-fwnode.c
> >>>>> @@ -25,6 +25,7 @@
> >>>>>  #include <linux/types.h>
> >>>>>
> >>>>>  #include <media/v4l2-async.h>
> >>>>> +#include <media/v4l2-ctrls.h>
> >>>>>  #include <media/v4l2-fwnode.h>
> >>>>>  #include <media/v4l2-subdev.h>
> >>>>>
> >>>>> @@ -595,6 +596,62 @@ void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link)
> >>>>>  }
> >>>>>  EXPORT_SYMBOL_GPL(v4l2_fwnode_put_link);
> >>>>>
> >>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>>>> +				  struct v4l2_ctrl_handler *hdl,
> >>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops)
> >>>>
> >>>> I'm not convinced that this helper is a good idea.
> >>>>
> >>>> A helper that parses and validates this information makes sense,
> >>>> but combining that with creating the controls feels wrong to me.
> >>>>
> >>>> You're mixing two very different things in one function.
> >>>>
> >>>> I think something like this would work better in a driver:
> >>>>
> >>>> 	if (!v4l2_fwnode_parse_location(&val))
> >>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>> 				  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>>> 				  val, val, 1, val);
> >>>> 	if (!v4l2_fwnode_parse_rotation(&val))
> >>>> 		v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>> 				  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>>> 				  val, val, 1, val);
> >>>>
> >>>> Much cleaner IMHO. (Just a brainstorm, so don't get stuck on these
> >>>> function prototypes!)
> >>>>
> >>>
> >>> Could the control registration being conditional on the presence of
> >>> the *hdl parameter otherwise, or would you split the two operations
> >>> (property parsing and control registration) nonetheless ?
> >>
> >> Split it. My main problem with this helper is that it is mixing two
> >> frameworks. Most of Laurent's comments on this patch just go away if
> >> you leave the control creation to the driver.
> >>
> >> It really isn't much code, and it is much easier to review a driver
> >> if all the controls are created in the same place instead of some
> >> controls being magically created in a helper function.
> > 
> > But this would require copying the above code in every single camera
> > sensor driver. Furthermore, the helper proposed by Jacopo would make
> > addition of new firmware properties much simpler, as we wouldn't need to
> > modify all sensor drivers.
> > 
> > V4L2 requires lots of boilerplate code for sensor drivers, and I think
> > more helper would be useful. There's really not point in doing the same
> > thing slightly differently in dozens of drivers. Maybe we could
> > experiment with a v4l2_camera_subdev structure ?
> 
> You can make one v4l2_fwnode helper that parses all the sensor properties,
> and another helper in v4l2-ctrl.c or v4l2-common.c that uses the parsed
> result to create the controls. That way if the driver needs to do something
> unusual with the controls it can just create them manually.

Looks good to me.

> But don't mix the two in one helper.
> 
> Note that creating some more advanced framework for sensors wouldn't hurt
> since it's a bit mess at the moment IMHO.

It's a bit too much as a prerequisite for this patch series, but I'd
like to see this happening sooner than later, yes.

> >>> An helper was suggested in the v1 review, Laurent, Sakari, what do you
> >>> think here?
> >>>
> >>>>> +{
> >>>>> +	u32 val;
> >>>>> +	int ret;
> >>>>> +
> >>>>> +	ret = fwnode_property_read_u32(fwnode, "location", &val);
> >>>>> +	if (!ret) {
> >>>>> +		switch (val) {
> >>>>> +		case V4L2_LOCATION_FRONT:
> >>>>> +		case V4L2_LOCATION_BACK:
> >>>>> +		case V4L2_LOCATION_EXTERNAL:
> >>>>> +			break;
> >>>>> +		default:
> >>>>> +			pr_warn("Unsupported location: %u\n", val);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_LOCATION))
> >>>>> +			pr_debug("Skip control '%s': already registered",
> >>>>> +				 v4l2_ctrl_get_name(
> >>>>> +					 V4L2_CID_CAMERA_SENSOR_LOCATION));
> >>>>> +		else
> >>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>>> +					  V4L2_CID_CAMERA_SENSOR_LOCATION,
> >>>>> +					  val, val, 1, val);
> >>>>> +	}
> >>>>> +
> >>>>> +	ret = fwnode_property_read_u32(fwnode, "rotation", &val);
> >>>>> +	if (!ret) {
> >>>>> +		if (val > 360) {
> >>>>
> >>>> I'd add '|| val % 90' to this condition.
> >>>
> >>> Do we want to enforce this? I can't imagine any use case, but why a
> >>> camera cannot be rotated of an arbitrary number of degrees ?
> >>
> >> I would start out by enforcing this until someone comes up with a
> >> realistic use-case.
> >>
> >> As long as it is a multiple of 90 degree, then there is a clear interaction
> >> with the ROTATE/HFLIP/VFLIP controls. For other angles that gets more confusing
> >> and I'd rather avoid that for now.
> > 
> > If we enfore this, then let's update the DT bindings accordingly. yaml
> > would help with validation ;-)
> > 
> >>>>> +			pr_warn("Unsupported rotation: %u\n", val);
> >>>>> +			return -EINVAL;
> >>>>> +		}
> >>>>> +
> >>>>> +		if (v4l2_ctrl_find(hdl, V4L2_CID_CAMERA_SENSOR_ROTATION))
> >>>>> +			pr_debug("Skip control '%s': already registered",
> >>>>> +				 v4l2_ctrl_get_name(
> >>>>> +					 V4L2_CID_CAMERA_SENSOR_ROTATION));
> >>>>> +		else
> >>>>> +			v4l2_ctrl_new_std(hdl, ctrl_ops,
> >>>>> +					  V4L2_CID_CAMERA_SENSOR_ROTATION,
> >>>>> +					  val, val, 1, val);
> >>>>> +	}
> >>>>> +
> >>>>> +	if (hdl->error) {
> >>>>> +		pr_warn("Failed to register controls from firmware: %d\n",
> >>>>> +			hdl->error);
> >>>>> +		return hdl->error;
> >>>>> +	}
> >>>>> +
> >>>>> +	return 0;
> >>>>> +}
> >>>>> +EXPORT_SYMBOL_GPL(v4l2_fwnode_register_controls);
> >>>>> +
> >>>>>  static int
> >>>>>  v4l2_async_notifier_fwnode_parse_endpoint(struct device *dev,
> >>>>>  					  struct v4l2_async_notifier *notifier,
> >>>>> diff --git a/include/media/v4l2-fwnode.h b/include/media/v4l2-fwnode.h
> >>>>> index f6a7bcd13197..0dad6968bde9 100644
> >>>>> --- a/include/media/v4l2-fwnode.h
> >>>>> +++ b/include/media/v4l2-fwnode.h
> >>>>> @@ -25,6 +25,8 @@
> >>>>>  struct fwnode_handle;
> >>>>>  struct v4l2_async_notifier;
> >>>>>  struct v4l2_async_subdev;
> >>>>> +struct v4l2_ctrl_handler;
> >>>>> +struct v4l2_ctrl_ops;
> >>>>>
> >>>>>  #define V4L2_FWNODE_CSI2_MAX_DATA_LANES	4
> >>>>>
> >>>>> @@ -233,6 +235,34 @@ int v4l2_fwnode_parse_link(struct fwnode_handle *fwnode,
> >>>>>   */
> >>>>>  void v4l2_fwnode_put_link(struct v4l2_fwnode_link *link);
> >>>>>
> >>>>> +/**
> >>>>> + * v4l2_fwnode_register_controls() - parse device and endpoint fwnode
> >>>>> + *				     properties and register a v4l2 control
> >>>>> + *				     for each of them
> >>>>> + * @fwnode: pointer to the device fwnode handle
> >>>>> + * @hdl: pointer to the v4l2 control handler to register controls with
> >>>>> + * @ctrl_ops: pointer to the v4l2 control operations to register with the handler
> >>>>> + *
> >>>>> + * Parse the @fwnode device and endpoint properties to which a v4l2 control
> >>>>> + * is associated and register them with the provided handler @hdl.
> >>>>> + * Currently the following v4l2 controls are parsed and registered:
> >>>>> + * - V4L2_CID_CAMERA_SENSOR_LOCATION;
> >>>>> + * - V4L2_CID_CAMERA_SENSOR_ROTATION;
> >>>>> + *
> >>>>> + * Controls already registered by the caller with the @hdl control handler are
> >>>>> + * not overwritten. Callers should register the controls they want to handle
> >>>>> + * themselves before calling this function.
> >>>>> + *
> >>>>> + * NOTE: This function locks the @hdl control handler mutex, the caller shall
> >>>>> + * not hold the lock when calling this function.
> >>>>> + *
> >>>>> + * Return: 0 on success, -EINVAL if the fwnode properties are not correctly
> >>>>> + * specified.
> >>>>> + */
> >>>>> +int v4l2_fwnode_register_controls(struct fwnode_handle *fwnode,
> >>>>> +				  struct v4l2_ctrl_handler *hdl,
> >>>>> +				  const struct v4l2_ctrl_ops *ctrl_ops);
> >>>>> +
> >>>>>  /**
> >>>>>   * typedef parse_endpoint_func - Driver's callback function to be called on
> >>>>>   *	each V4L2 fwnode endpoint.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Laurent Pinchart]

Hi Jacopo,

On Thu, Aug 29, 2019 at 06:43:30PM +0200, Jacopo Mondi wrote:
> On Thu, Aug 29, 2019 at 03:39:43PM +0200, Hans Verkuil wrote:
> > On 8/29/19 3:18 PM, Jacopo Mondi wrote:
> > > On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> > >> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> > >>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> > >>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> > >>>>> Report the native pixel array size and the crop bounds for the ov5670
> > >>>>> sensor driver.
> > >>>>>
> > >>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>>> ---
> > >>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> > >>>>>  1 file changed, 20 insertions(+)
> > >>>>>
> > >>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > >>>>> index 2bc57e85f721..3e22fe9ccad1 100644
> > >>>>> --- a/drivers/media/i2c/ov5670.c
> > >>>>> +++ b/drivers/media/i2c/ov5670.c
> > >>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> > >>>>>  	return 0;
> > >>>>>  }
> > >>>>>
> > >>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> > >>>>> +				struct v4l2_subdev_pad_config *cfg,
> > >>>>> +				struct v4l2_subdev_selection *sel)
> > >>>>> +{
> > >>>>> +	switch (sel->target) {
> > >>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > >>>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > >>>>> +		sel->r.left = 0;
> > >>>>> +		sel->r.top = 0;
> > >>>>> +		sel->r.width = 2592;
> > >>>>> +		sel->r.height = 1944;
> > >>>>
> > >>>> Why do you need this?
> > >>>>
> > >>>
> > >>> I need to expose the pixel array size and the active pixel area size,
> > >>> and I interpreted the two above targets as the right place where to do
> > >>> so.
> > >>
> > >> That didn't answer my question :-)
> > >>
> > >> Why do you need to expose this? Who uses it for what purpose?
> > >>
> > >
> > > Ah, ok :)
> > >
> > > Quoting the cover letter of the series:
> > >
> > > Retrieving the following camera static information is a requirement for the
> > > implementation of the Android-compatiblity layer of libcamera, but I'm sure
> > > it might prove useful for other user-space applications and libraries as well.
> > >
> > > In other words, in order to report to the android camera stack those
> > > two information (among -many- others) they should be somehow retrieved
> > > from the kernel interface, and after considering adding two more
> > > read-only controls to expose them as I did for rotation and location,
> > > I went for using the selection API.
> > >
> > >>>
> > >>> At least for NATIVE_SIZE the documentation seems to match my
> > >>> understanding:
> > >>>
> > >>> "The native size of the device, e.g. a sensor’s pixel array. left and top
> > >>> fields are zero for this target."
> > >>
> > >> Correct.
> > >>
> > >>>
> > >>>
> > >>>> Since the format can change for this and the next driver I think CROP_BOUNDS
> > >>>> at least should match the current format.
> > >>>>
> > >>>
> > >>> Ah, does it? It was not clear to me from the documentation, as it
> > >>> suggested to me that target actually identifies the active pixel area
> > >>>
> > >>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> > >>> crop bounds rectangle."
> > >>>
> > >>> It does not mention format, should this be updated?
> > >>
> > >> The problem is that for sensors it is indeed not clear.
> > >>
> > >> In principle the crop bounds matches the resolution that the sensor uses
> > >> pre-scaling. So yes, that means that it is equal to the native size.
> > >
> > > native size != active areas size.
> > > Did you mean the latter here?
> >
> > I mean the latter, but I have to say that the spec doesn't make it clear
> > if TGT_NATIVE_SIZE equals the active area or also includes inactive pixels.
> >
> > What exactly does Android expect?
> >
> Ah! You asked for this, so be prepared...
> 
> I started by trying to retrieve from the kernel interface the
> following parameters:
> 
> The pixel array size
> https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.pixelArraySize
> The physical dimensions of the sensor (TODO)
> https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.physicalSize
> The active area size:
> https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.info.activeArraySize
> Sensor orientation:
> https://jmondi.org/android_metadata_tags/docs.html#static_android.sensor.orientation
> Location:
> https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.facing
> 
> For very simple cameras there are not many more informations about
> sensor/lens to be retrieved, but if you look at the end of the page you can
> see a list of other informations Android requires for more advanced use case
> (not so advanced actually, those are expected to be there for any
> recent modern phone device I guess)
> 
> It's a long list, with many details about the sensor, lens and the
> capabilities of the camera.
> 
> Just to provide an example list:
> android.lens.info.availableApertures (static)
> android.lens.info.availableFilterDensities (static)
> android.lens.info.availableFocalLengths (static)
> android.lens.info.availableOpticalStabilization (static)
> android.lens.info.minimumFocusDistance (static)
> android.lens.info.shadingMapSize (static)
> android.lens.info.focusDistanceCalibration (static)
> android.sensor.info.sensitivityRange (static)
> android.sensor.info.exposureTimeRange (static)
> android.sensor.info.maxFrameDuration (static)
> android.sensor.info.physicalSize (static)
> android.sensor.info.timestampSource (static)
> android.sensor.maxAnalogSensitivity (static)
> 
> And they grow up to the point of describing the color transformation
> matrices and other low level details, if your device claim to support
> raw capture and manipulation:
> android.sensor.info.colorFilterArrangement (static)
> android.sensor.info.whiteLevel (static)
> android.sensor.info.preCorrectionActiveArraySize (static)
> android.sensor.referenceIlluminant1 (static)
> android.sensor.referenceIlluminant2 (static)
> android.sensor.calibrationTransform1 (static)
> android.sensor.calibrationTransform2 (static)
> android.sensor.colorTransform1 (static)
> android.sensor.colorTransform2 (static)
> android.sensor.forwardMatrix1 (static)
> android.sensor.forwardMatrix2 (static)
> android.sensor.blackLevelPattern (static)
> android.sensor.profileHueSatMapDimensions (static)
> 
> So, going forward I cannot tell how much of this could actually come
> from V4L, ideally all of this, but as of today it's not possible, so
> vendors keep adding custom solutions to support this with out-of-tree
> drivers or find solutions to generate these information from
> configuration files in user space.
> 
> For libcamera this is not possible, it aims to build on top of
> standard v4l2 interfaces, so I've started the process of adding small
> bits to v4l2 to at least retrieve the most basic information.

Note that for properties that only depend on the sensor model, we could
keep a database of sensors in userspace instead of pushing all that to
the kernel. We could in theory do the same for the pixel array size and
other related sizes, but as V4L2 already expose those, I would prefer
using the V4L2 API to query them.

> > >> But many sensors have a discrete list of supported formats and it is not
> > >> clear how they map each format to the actual sensor. If it is clearly just
> > >> done through binning or averaging, then all is fine.
> > >>
> > >> If the formats have different aspect ratios, then it becomes a bit more
> > >> difficult how to relate the crop bounds with the format since you may not
> > >> know to which sensor area the current format corresponds.
> > >
> > > I see. We've had the same discussion in the libcamera list, as indeed
> > > the crop bounds might depend on the aspect ratio as you said.
> > >
> > > Maybe the crop_bound target is not the right place to report the
> > > active area, or maybe the concept of active area is ill-defined if
> > > the sensor is cross-shaped:
> > >
> > >     /--------------------/ -> Pixel array size
> > >     ----------------------
> > >     |x    |        |--- x|------> 4:3 crop bounds
> > >     |x------------------x|
> > >     |x|   |        |   |x|
> > >     |x|   |        |   |x|------> 21:9 crop bounds
> > >     |x|   |        |   |x|
> > >     |x------------------x|
> > >     |x    |________|    x|
> > >     ----------------------
> > >       |                 |
> > >       ---------------------------> Black pixels colums
> > >
> > >>
> > >> I have no clear answer for you, to be honest, but it can get tricky.
> > >>
> > >
> > > Indeed, but assuming a simpler square-shaped sensor, is the crop bound
> > > target the right one to report the active pixel area ?
> >
> > I don't think so.
> >
> > The crop bounds defines the outer bounds of the area where you can put
> > your crop rectangle. This can very well include areas where the pixels
> > are covered and so always return black (sometimes used to obtain black
> > levels, I believe).
> 
> Ah, I didn't considered black pixels as part of a possible crop
> selection..

The whole point of black pixels is that they can be captured (or at
least part of them), for the ISP to calculate an average black level and
subtract it from the active pixels. Let's thus make sure we clearly
define every selection rectangle in the specification.

> > The default crop rectangle would be the one that reports the active
> > area. The native size rectangle would be the full pixel array.
> >
> > So CROP_DEFAULT <= CROP_BOUNDS <= NATIVE_SIZE.
> 
> I see thanks for clarifying this
> 
> > For a cross-shaped sensor I would expect that the CROP_BOUNDS/DEFAULT
> > depends on the format (aspect ratio).
> 
> Currently the selection API do not support to retrieve that
> information depending on the aspect-ratio though
>
> > I think this makes sense.
> >
> > The specification definitely needs to be improved, patches are welcome...
> >
> > >>>
> > >>> How would you report the two information I need?
> > >>
> > >> It depends on my original question: what do you need this information for?
> > >
> > > Please note that it's for the android camera stack in this case, but
> > > it's an information that userspace might want to know for several
> > > different reasons. Calibration and FOV calculation come to mind. Does
> > > this makes sense to you?
> >
> > Ah, so that's what it is used for :-)
> >
> > Which of the three targets above would match with what Android needs?
> 
> the full pixel array size would be reported by the NATIVE_SIZE target
> the active area size by the CROP_DEFAULT rectangle (regardless of the
> aspect ratio)
> 
> It's still not clear to me what the CROP_BOUNDS rectangle would then
> be used for, unless it is made to be dependent on an aspect ratio but
> that would require userspace to provide a populated rectangle to
> G_SELECTION and drivers to inspect it and return an opportune
> CROP_BOUNDS one. Would this be acceptable ?
> 
> > >>>> I don't think this patch and the next have anything to do with the location/rotate
> > >>>> support. I would split it off from this series.
> > >>>>
> > >>>
> > >>> Agreed, they were split in v1, maybe it has not been a wise move to
> > >>> make a single series out of them. I'll split again.
> > >>>
> > >>>>> +		break;
> > >>>>> +	default:
> > >>>>> +		return -EINVAL;
> > >>>>> +	}
> > >>>>> +
> > >>>>> +	return 0;
> > >>>>> +}
> > >>>>> +
> > >>>>>  static int ov5670_get_skip_frames(struct v4l2_subdev *sd, u32 *frames)
> > >>>>>  {
> > >>>>>  	*frames = OV5670_NUM_OF_SKIP_FRAMES;
> > >>>>> @@ -2425,6 +2444,7 @@ static const struct v4l2_subdev_pad_ops ov5670_pad_ops = {
> > >>>>>  	.enum_mbus_code = ov5670_enum_mbus_code,
> > >>>>>  	.get_fmt = ov5670_get_pad_format,
> > >>>>>  	.set_fmt = ov5670_set_pad_format,
> > >>>>> +	.get_selection = ov5670_get_selection,
> > >>>>>  	.enum_frame_size = ov5670_enum_frame_size,
> > >>>>>  };
> > >>>>>

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

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

HI Laurent,
On Tue, Aug 27, 2019 at 03:51:32PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> Thank you for the patch.
>
> On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> > Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> > control. The newly added read-only control reports the camera device
> > mounting rotation.
> >
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
> >  1 file changed, 9 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > index ecf151f3f0f4..03d1c23d18f7 100644
> > --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
> >
> >
> >
> > +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> > +    This read-only control describes the camera sensor orientation by
> > +    reporting its mounting rotation in respect to the device intended usage
> > +    orientation, expressed in counter clockwise degrees. The control value is
>
> Counter clockwise relative to what ? :-)

I think it is expressed in the previous line:
"mounting rotation in respect to the device intended usage
orientation, expressed in counter clockwise degrees"

Would you prefer to have this re-phrased as:

This read-only control describes the camera sensor orientation
expressed as rotation in counter clockwise degrees in respect to
the device intended usage orientation.

?

>
> > +    constant and not modifiable by software and its value is retrieved from the
> > +    firmware interface by parsing the 'rotation' property. Typical values are 0
> > +    degrees for upright mounted sensors and 180 degrees for sensors mounted
> > +    upside down.
>
> Same comment as for 02/10 regarding how the value is known to the
> driver.
>
> > +
> >  .. [#f1]
> >     This control may be changed to a menu control in the future, if more
> >     options are required.
>
> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Rob Herring]

On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > > Add the 'location' device property, used to specify the camera device
> > > mounting position. The property is particularly meaningful for mobile
> > > devices with a well defined usage orientation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index f884ada0bffc..865f4142f432 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,16 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > >
> > > +- location: The camera sensor mounting location, expressed as a position
> > > +  relative to the usage orientation of the device the sensor is installed on.
> >
> > DT bindings being ABIs, we need to be precise and thorough there. One
> > particular point that bothers me is that the property is named location,
> > and its description refers to camera sensor mounting location.
> >
> > I see two options to fix this. One of them is to rename the property to
> > camera-location, but that would limit its future usage for other types
> > of devices. The other one is to document the property as applying to a
> > "device" instead of a "camera sensor", and add one sentence stating that
> > this property is valid for camera sensors only.
> >
> > This will require finding another name for the device that the device is
> > mounted on though, as using device twice would be very confusing.
> 
> I had the same concern, but I cannot find another term to convey
> this... suggestions?

For accelerometers and/or gyroscopes, we already have 'mount-matrix'. 
That would replace 'rotation'. Maybe we can do something similar here?

Rob

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Rob Herring]

On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> Hi Laurent,
> 
> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > > Add the 'location' device property, used to specify the camera device
> > > mounting position. The property is particularly meaningful for mobile
> > > devices with a well defined usage orientation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index f884ada0bffc..865f4142f432 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,16 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > >
> > > +- location: The camera sensor mounting location, expressed as a position
> > > +  relative to the usage orientation of the device the sensor is installed on.
> >
> > DT bindings being ABIs, we need to be precise and thorough there. One
> > particular point that bothers me is that the property is named location,
> > and its description refers to camera sensor mounting location.
> >
> > I see two options to fix this. One of them is to rename the property to
> > camera-location, but that would limit its future usage for other types
> > of devices. The other one is to document the property as applying to a
> > "device" instead of a "camera sensor", and add one sentence stating that
> > this property is valid for camera sensors only.
> >
> > This will require finding another name for the device that the device is
> > mounted on though, as using device twice would be very confusing.
> 
> I had the same concern, but I cannot find another term to convey
> this... suggestions?

For accelerometers and/or gyroscopes, we already have 'mount-matrix'. 
That would replace 'rotation'. Maybe we can do something similar here?

Rob

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Rob,

On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > > > Add the 'location' device property, used to specify the camera device
> > > > mounting position. The property is particularly meaningful for mobile
> > > > devices with a well defined usage orientation.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > index f884ada0bffc..865f4142f432 100644
> > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > @@ -89,6 +89,16 @@ Optional properties
> > > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > > >    (upside down).
> > > >
> > > > +- location: The camera sensor mounting location, expressed as a position
> > > > +  relative to the usage orientation of the device the sensor is installed on.
> > >
> > > DT bindings being ABIs, we need to be precise and thorough there. One
> > > particular point that bothers me is that the property is named location,
> > > and its description refers to camera sensor mounting location.
> > >
> > > I see two options to fix this. One of them is to rename the property to
> > > camera-location, but that would limit its future usage for other types
> > > of devices. The other one is to document the property as applying to a
> > > "device" instead of a "camera sensor", and add one sentence stating that
> > > this property is valid for camera sensors only.
> > >
> > > This will require finding another name for the device that the device is
> > > mounted on though, as using device twice would be very confusing.
> >
> > I had the same concern, but I cannot find another term to convey
> > this... suggestions?
>
> For accelerometers and/or gyroscopes, we already have 'mount-matrix'.

Pardon my ignorance, but I could not find it documented. Some binding
files refers to an iio/mount-matrix.txt file which I cannot find. Has
it been removed? Anyway, some individual bindings report examples of
mount matrices (ie
Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)

> That would replace 'rotation'. Maybe we can do something similar here?

I think 'rotation' simply expressed as degrees is fine here, our
problem was to find a term that would make it possible to specify that
rotation/location are applied to a 'device' mounted on a 'device'.
Sakari suggested on irc to use 'system' in place of the second
'device' and that seems to work well to me.

Or were you suggesting to use a construct similar to mount-matrix for
a purpose I didn't get :) ?

Thanks
   j
>
> Rob
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Rob,

On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> > Hi Laurent,
> >
> > On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > > Hi Jacopo,
> > >
> > > Thank you for the patch.
> > >
> > > On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > > > Add the 'location' device property, used to specify the camera device
> > > > mounting position. The property is particularly meaningful for mobile
> > > > devices with a well defined usage orientation.
> > > >
> > > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > > ---
> > > >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > > >  1 file changed, 10 insertions(+)
> > > >
> > > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > index f884ada0bffc..865f4142f432 100644
> > > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > > @@ -89,6 +89,16 @@ Optional properties
> > > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > > >    (upside down).
> > > >
> > > > +- location: The camera sensor mounting location, expressed as a position
> > > > +  relative to the usage orientation of the device the sensor is installed on.
> > >
> > > DT bindings being ABIs, we need to be precise and thorough there. One
> > > particular point that bothers me is that the property is named location,
> > > and its description refers to camera sensor mounting location.
> > >
> > > I see two options to fix this. One of them is to rename the property to
> > > camera-location, but that would limit its future usage for other types
> > > of devices. The other one is to document the property as applying to a
> > > "device" instead of a "camera sensor", and add one sentence stating that
> > > this property is valid for camera sensors only.
> > >
> > > This will require finding another name for the device that the device is
> > > mounted on though, as using device twice would be very confusing.
> >
> > I had the same concern, but I cannot find another term to convey
> > this... suggestions?
>
> For accelerometers and/or gyroscopes, we already have 'mount-matrix'.

Pardon my ignorance, but I could not find it documented. Some binding
files refers to an iio/mount-matrix.txt file which I cannot find. Has
it been removed? Anyway, some individual bindings report examples of
mount matrices (ie
Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)

> That would replace 'rotation'. Maybe we can do something similar here?

I think 'rotation' simply expressed as degrees is fine here, our
problem was to find a term that would make it possible to specify that
rotation/location are applied to a 'device' mounted on a 'device'.
Sakari suggested on irc to use 'system' in place of the second
'device' and that seems to work well to me.

Or were you suggesting to use a construct similar to mount-matrix for
a purpose I didn't get :) ?

Thanks
   j
>
> Rob
>

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 02, 2019 at 01:20:49PM +0200, Jacopo Mondi wrote:
> On Tue, Aug 27, 2019 at 03:51:32PM +0300, Laurent Pinchart wrote:
> > On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> >> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> >> control. The newly added read-only control reports the camera device
> >> mounting rotation.
> >>
> >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >> ---
> >>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
> >>  1 file changed, 9 insertions(+)
> >>
> >> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >> index ecf151f3f0f4..03d1c23d18f7 100644
> >> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >> @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
> >>
> >>
> >>
> >> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> >> +    This read-only control describes the camera sensor orientation by
> >> +    reporting its mounting rotation in respect to the device intended usage
> >> +    orientation, expressed in counter clockwise degrees. The control value is
> >
> > Counter clockwise relative to what ? :-)
> 
> I think it is expressed in the previous line:
> "mounting rotation in respect to the device intended usage
> orientation, expressed in counter clockwise degrees"
> 
> Would you prefer to have this re-phrased as:
> 
> This read-only control describes the camera sensor orientation
> expressed as rotation in counter clockwise degrees in respect to
> the device intended usage orientation.
> 
> ?

Rotation is expressed in degrees around an axis, which is itself
expressed as a vector. It's pretty intuitive that the rotation vector
should be perpendicular to the plane of the sensor (which itself should
be parallel to the plane of the device side on which it is mounted, as
expressed by the location property), but there are still two possible
directions for the vector, facing in the same direction as the sensor or
the opposite direction. Unless there's a good reason to do so, I would
use the same direction as the one defined by Android. I don't know what
direction that is though :-)

> >> +    constant and not modifiable by software and its value is retrieved from the
> >> +    firmware interface by parsing the 'rotation' property. Typical values are 0
> >> +    degrees for upright mounted sensors and 180 degrees for sensors mounted
> >> +    upside down.
> >
> > Same comment as for 02/10 regarding how the value is known to the
> > driver.
> >
> >> +
> >>  .. [#f1]
> >>     This control may be changed to a menu control in the future, if more
> >>     options are required.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> > On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> >> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> >>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> >>>> Add the 'location' device property, used to specify the camera device
> >>>> mounting position. The property is particularly meaningful for mobile
> >>>> devices with a well defined usage orientation.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> index f884ada0bffc..865f4142f432 100644
> >>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> @@ -89,6 +89,16 @@ Optional properties
> >>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> >>>>    (upside down).
> >>>>
> >>>> +- location: The camera sensor mounting location, expressed as a position
> >>>> +  relative to the usage orientation of the device the sensor is installed on.
> >>>
> >>> DT bindings being ABIs, we need to be precise and thorough there. One
> >>> particular point that bothers me is that the property is named location,
> >>> and its description refers to camera sensor mounting location.
> >>>
> >>> I see two options to fix this. One of them is to rename the property to
> >>> camera-location, but that would limit its future usage for other types
> >>> of devices. The other one is to document the property as applying to a
> >>> "device" instead of a "camera sensor", and add one sentence stating that
> >>> this property is valid for camera sensors only.
> >>>
> >>> This will require finding another name for the device that the device is
> >>> mounted on though, as using device twice would be very confusing.
> >>
> >> I had the same concern, but I cannot find another term to convey
> >> this... suggestions?
> >
> > For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> 
> Pardon my ignorance, but I could not find it documented. Some binding
> files refers to an iio/mount-matrix.txt file which I cannot find. Has
> it been removed? Anyway, some individual bindings report examples of
> mount matrices (ie
> Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> 
> > That would replace 'rotation'. Maybe we can do something similar here?
> 
> I think 'rotation' simply expressed as degrees is fine here, our
> problem was to find a term that would make it possible to specify that
> rotation/location are applied to a 'device' mounted on a 'device'.
> Sakari suggested on irc to use 'system' in place of the second
> 'device' and that seems to work well to me.
> 
> Or were you suggesting to use a construct similar to mount-matrix for
> a purpose I didn't get :) ?

mount-matrix would allow exposing the rotation in a more generic way,
and if there are standard DT bindings, I think this would make sense,
especially given that we could have device with camera sensors not
mounted parallel to the device side.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> > On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> >> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> >>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> >>>> Add the 'location' device property, used to specify the camera device
> >>>> mounting position. The property is particularly meaningful for mobile
> >>>> devices with a well defined usage orientation.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >>>>  1 file changed, 10 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> index f884ada0bffc..865f4142f432 100644
> >>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>> @@ -89,6 +89,16 @@ Optional properties
> >>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> >>>>    (upside down).
> >>>>
> >>>> +- location: The camera sensor mounting location, expressed as a position
> >>>> +  relative to the usage orientation of the device the sensor is installed on.
> >>>
> >>> DT bindings being ABIs, we need to be precise and thorough there. One
> >>> particular point that bothers me is that the property is named location,
> >>> and its description refers to camera sensor mounting location.
> >>>
> >>> I see two options to fix this. One of them is to rename the property to
> >>> camera-location, but that would limit its future usage for other types
> >>> of devices. The other one is to document the property as applying to a
> >>> "device" instead of a "camera sensor", and add one sentence stating that
> >>> this property is valid for camera sensors only.
> >>>
> >>> This will require finding another name for the device that the device is
> >>> mounted on though, as using device twice would be very confusing.
> >>
> >> I had the same concern, but I cannot find another term to convey
> >> this... suggestions?
> >
> > For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> 
> Pardon my ignorance, but I could not find it documented. Some binding
> files refers to an iio/mount-matrix.txt file which I cannot find. Has
> it been removed? Anyway, some individual bindings report examples of
> mount matrices (ie
> Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> 
> > That would replace 'rotation'. Maybe we can do something similar here?
> 
> I think 'rotation' simply expressed as degrees is fine here, our
> problem was to find a term that would make it possible to specify that
> rotation/location are applied to a 'device' mounted on a 'device'.
> Sakari suggested on irc to use 'system' in place of the second
> 'device' and that seems to work well to me.
> 
> Or were you suggesting to use a construct similar to mount-matrix for
> a purpose I didn't get :) ?

mount-matrix would allow exposing the rotation in a more generic way,
and if there are standard DT bindings, I think this would make sense,
especially given that we could have device with camera sensors not
mounted parallel to the device side.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi again,

On Mon, Sep 02, 2019 at 07:49:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> > On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> > > On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> > >> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > >>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > >>>> Add the 'location' device property, used to specify the camera device
> > >>>> mounting position. The property is particularly meaningful for mobile
> > >>>> devices with a well defined usage orientation.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>> ---
> > >>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > >>>>  1 file changed, 10 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> index f884ada0bffc..865f4142f432 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> @@ -89,6 +89,16 @@ Optional properties
> > >>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >>>>    (upside down).
> > >>>>
> > >>>> +- location: The camera sensor mounting location, expressed as a position
> > >>>> +  relative to the usage orientation of the device the sensor is installed on.
> > >>>
> > >>> DT bindings being ABIs, we need to be precise and thorough there. One
> > >>> particular point that bothers me is that the property is named location,
> > >>> and its description refers to camera sensor mounting location.
> > >>>
> > >>> I see two options to fix this. One of them is to rename the property to
> > >>> camera-location, but that would limit its future usage for other types
> > >>> of devices. The other one is to document the property as applying to a
> > >>> "device" instead of a "camera sensor", and add one sentence stating that
> > >>> this property is valid for camera sensors only.
> > >>>
> > >>> This will require finding another name for the device that the device is
> > >>> mounted on though, as using device twice would be very confusing.
> > >>
> > >> I had the same concern, but I cannot find another term to convey
> > >> this... suggestions?
> > >
> > > For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> >
> > Pardon my ignorance, but I could not find it documented. Some binding
> > files refers to an iio/mount-matrix.txt file which I cannot find. Has
> > it been removed? Anyway, some individual bindings report examples of
> > mount matrices (ie
> > Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> >
> > > That would replace 'rotation'. Maybe we can do something similar here?
> >
> > I think 'rotation' simply expressed as degrees is fine here, our
> > problem was to find a term that would make it possible to specify that
> > rotation/location are applied to a 'device' mounted on a 'device'.
> > Sakari suggested on irc to use 'system' in place of the second
> > 'device' and that seems to work well to me.
> >
> > Or were you suggesting to use a construct similar to mount-matrix for
> > a purpose I didn't get :) ?
>
> mount-matrix would allow exposing the rotation in a more generic way,
> and if there are standard DT bindings, I think this would make sense,
> especially given that we could have device with camera sensors not
> mounted parallel to the device side.
>

After reading this entry you pointed me to
https://lore.kernel.org/patchwork/patch/1044786/
I understand why Rob suggested that, but it seems a bit far fetched to
me... The devices that could make use of 'mount-matrix', such as the
ones IIO deal with, sample data in a 3D space, and to compensate their
mounting displacement a rotation matrix is required.

Do you see use cases for compensating mounting displacement in a 3-d
space for a camera ? I'm not saying it's not possible, but seems -a
bit- far fetched.

If we consider cameras as always mounted parallel to the device used
to take picturea, the correction you could apply to the possible mounting
displacements are just rotations along the origin (or the z axis you want
to consider a 3 dimensional space) and requiring to supply a 2-dimensions
rotation matrix (with only a few combinations of 0, 1 and -1, if we only
accept multiples of 90 degrees as we agreed) it seems a bit an overkill.

True, we could also handle image flipping, not just rotation with a rotation
matrix but an image sensor mounted flipped upside down kind of defeat its
purpose, doesn't it ? (it would point to the inside of the device :)

When we'll have devices with movable sensors like the one Pavel
linked, rotation matrices could be used to report the current camera
position maybe, but to represent a mounting displacement compensation,
for devices working in a 2 dimensional space and with fixed rotations
of 90 degrees multiples it seems an overkill to ask to developers to
me.

That said, there are devices out there which do things I cannot even
imagine, and maybe could actually compensate rotations in the 3D
space, sample 3D point maps or other advanced things. Indeed they
could use a rotation matrix if they need to, but the purpose of
'rotation' is much more humble and even if it could be represented
with 'mount-matrix' it seems to me a non justifiable effort.

Thanks
   j

> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi again,

On Mon, Sep 02, 2019 at 07:49:37PM +0300, Laurent Pinchart wrote:
> Hi Jacopo,
>
> On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> > On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> > > On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> > >> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> > >>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > >>>> Add the 'location' device property, used to specify the camera device
> > >>>> mounting position. The property is particularly meaningful for mobile
> > >>>> devices with a well defined usage orientation.
> > >>>>
> > >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>> ---
> > >>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > >>>>  1 file changed, 10 insertions(+)
> > >>>>
> > >>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> index f884ada0bffc..865f4142f432 100644
> > >>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > >>>> @@ -89,6 +89,16 @@ Optional properties
> > >>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >>>>    (upside down).
> > >>>>
> > >>>> +- location: The camera sensor mounting location, expressed as a position
> > >>>> +  relative to the usage orientation of the device the sensor is installed on.
> > >>>
> > >>> DT bindings being ABIs, we need to be precise and thorough there. One
> > >>> particular point that bothers me is that the property is named location,
> > >>> and its description refers to camera sensor mounting location.
> > >>>
> > >>> I see two options to fix this. One of them is to rename the property to
> > >>> camera-location, but that would limit its future usage for other types
> > >>> of devices. The other one is to document the property as applying to a
> > >>> "device" instead of a "camera sensor", and add one sentence stating that
> > >>> this property is valid for camera sensors only.
> > >>>
> > >>> This will require finding another name for the device that the device is
> > >>> mounted on though, as using device twice would be very confusing.
> > >>
> > >> I had the same concern, but I cannot find another term to convey
> > >> this... suggestions?
> > >
> > > For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> >
> > Pardon my ignorance, but I could not find it documented. Some binding
> > files refers to an iio/mount-matrix.txt file which I cannot find. Has
> > it been removed? Anyway, some individual bindings report examples of
> > mount matrices (ie
> > Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> >
> > > That would replace 'rotation'. Maybe we can do something similar here?
> >
> > I think 'rotation' simply expressed as degrees is fine here, our
> > problem was to find a term that would make it possible to specify that
> > rotation/location are applied to a 'device' mounted on a 'device'.
> > Sakari suggested on irc to use 'system' in place of the second
> > 'device' and that seems to work well to me.
> >
> > Or were you suggesting to use a construct similar to mount-matrix for
> > a purpose I didn't get :) ?
>
> mount-matrix would allow exposing the rotation in a more generic way,
> and if there are standard DT bindings, I think this would make sense,
> especially given that we could have device with camera sensors not
> mounted parallel to the device side.
>

After reading this entry you pointed me to
https://lore.kernel.org/patchwork/patch/1044786/
I understand why Rob suggested that, but it seems a bit far fetched to
me... The devices that could make use of 'mount-matrix', such as the
ones IIO deal with, sample data in a 3D space, and to compensate their
mounting displacement a rotation matrix is required.

Do you see use cases for compensating mounting displacement in a 3-d
space for a camera ? I'm not saying it's not possible, but seems -a
bit- far fetched.

If we consider cameras as always mounted parallel to the device used
to take picturea, the correction you could apply to the possible mounting
displacements are just rotations along the origin (or the z axis you want
to consider a 3 dimensional space) and requiring to supply a 2-dimensions
rotation matrix (with only a few combinations of 0, 1 and -1, if we only
accept multiples of 90 degrees as we agreed) it seems a bit an overkill.

True, we could also handle image flipping, not just rotation with a rotation
matrix but an image sensor mounted flipped upside down kind of defeat its
purpose, doesn't it ? (it would point to the inside of the device :)

When we'll have devices with movable sensors like the one Pavel
linked, rotation matrices could be used to report the current camera
position maybe, but to represent a mounting displacement compensation,
for devices working in a 2 dimensional space and with fixed rotations
of 90 degrees multiples it seems an overkill to ask to developers to
me.

That said, there are devices out there which do things I cannot even
imagine, and maybe could actually compensate rotations in the 3D
space, sample 3D point maps or other advanced things. Indeed they
could use a rotation matrix if they need to, but the purpose of
'rotation' is much more humble and even if it could be represented
with 'mount-matrix' it seems to me a non justifiable effort.

Thanks
   j

> --
> Regards,
>
> Laurent Pinchart

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Tomasz Figa]

On Tue, Sep 3, 2019 at 1:43 AM Laurent Pinchart
<laurent.pinchart@ideasonboard.com> wrote:
>
> Hi Jacopo,
>
> On Mon, Sep 02, 2019 at 01:20:49PM +0200, Jacopo Mondi wrote:
> > On Tue, Aug 27, 2019 at 03:51:32PM +0300, Laurent Pinchart wrote:
> > > On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> > >> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> > >> control. The newly added read-only control reports the camera device
> > >> mounting rotation.
> > >>
> > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >> ---
> > >>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
> > >>  1 file changed, 9 insertions(+)
> > >>
> > >> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > >> index ecf151f3f0f4..03d1c23d18f7 100644
> > >> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > >> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > >> @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
> > >>
> > >>
> > >>
> > >> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> > >> +    This read-only control describes the camera sensor orientation by
> > >> +    reporting its mounting rotation in respect to the device intended usage
> > >> +    orientation, expressed in counter clockwise degrees. The control value is
> > >
> > > Counter clockwise relative to what ? :-)
> >
> > I think it is expressed in the previous line:
> > "mounting rotation in respect to the device intended usage
> > orientation, expressed in counter clockwise degrees"
> >
> > Would you prefer to have this re-phrased as:
> >
> > This read-only control describes the camera sensor orientation
> > expressed as rotation in counter clockwise degrees in respect to
> > the device intended usage orientation.
> >
> > ?
>
> Rotation is expressed in degrees around an axis, which is itself
> expressed as a vector. It's pretty intuitive that the rotation vector
> should be perpendicular to the plane of the sensor (which itself should
> be parallel to the plane of the device side on which it is mounted, as
> expressed by the location property), but there are still two possible
> directions for the vector, facing in the same direction as the sensor or
> the opposite direction. Unless there's a good reason to do so, I would
> use the same direction as the one defined by Android. I don't know what
> direction that is though :-)

I don't think Android exposes camera rotation to the layers above the
HAL. The stream is expected to be pre-rotated by the HAL, taking into
account the desired target rotation of the stream itself [1].

[1] https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#1428

That said, Android seems to use "degrees counterclockwise" for rotations.

Best regards,
Tomasz

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Sakari Ailus]

Hi Hans, Jacopo,

On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> > HI Hans,
> > 
> > On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> >> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>> Report the native pixel array size and the crop bounds for the ov5670
> >>> sensor driver.
> >>>
> >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>> ---
> >>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> >>>  1 file changed, 20 insertions(+)
> >>>
> >>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> >>> index 2bc57e85f721..3e22fe9ccad1 100644
> >>> --- a/drivers/media/i2c/ov5670.c
> >>> +++ b/drivers/media/i2c/ov5670.c
> >>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> >>>  	return 0;
> >>>  }
> >>>
> >>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> >>> +				struct v4l2_subdev_pad_config *cfg,
> >>> +				struct v4l2_subdev_selection *sel)
> >>> +{
> >>> +	switch (sel->target) {
> >>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> >>> +		sel->r.left = 0;
> >>> +		sel->r.top = 0;
> >>> +		sel->r.width = 2592;
> >>> +		sel->r.height = 1944;
> >>
> >> Why do you need this?
> >>
> > 
> > I need to expose the pixel array size and the active pixel area size,
> > and I interpreted the two above targets as the right place where to do
> > so.
> 
> That didn't answer my question :-)
> 
> Why do you need to expose this? Who uses it for what purpose?
> 
> > 
> > At least for NATIVE_SIZE the documentation seems to match my
> > understanding:
> > 
> > "The native size of the device, e.g. a sensor’s pixel array. left and top
> > fields are zero for this target."
> 
> Correct.
> 
> > 
> > 
> >> Since the format can change for this and the next driver I think CROP_BOUNDS
> >> at least should match the current format.
> >>
> > 
> > Ah, does it? It was not clear to me from the documentation, as it
> > suggested to me that target actually identifies the active pixel area
> > 
> > "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> > crop bounds rectangle."
> > 
> > It does not mention format, should this be updated?
> 
> The problem is that for sensors it is indeed not clear.
> 
> In principle the crop bounds matches the resolution that the sensor uses
> pre-scaling. So yes, that means that it is equal to the native size.
> 
> But many sensors have a discrete list of supported formats and it is not
> clear how they map each format to the actual sensor. If it is clearly just
> done through binning or averaging, then all is fine.

Sensor drivers do; sensors themselves support much, much more than most
drivers allow. But this is due to the nature of information available to
the sensor driver developers, not really something that is trivial to
change.

> 
> If the formats have different aspect ratios, then it becomes a bit more
> difficult how to relate the crop bounds with the format since you may not
> know to which sensor area the current format corresponds.
> 
> I have no clear answer for you, to be honest, but it can get tricky.

I've suggested earlier that the crop and compose selection targets to be
used to convey the cropping and binning (or scaling) that is done on the
sensor, in that order. In reality, there are usually more than one
(sometimes three) inside a sensor to crop, and often more than one place to
scale as well. So the driver would need to accommodate this.

The modes come with both cropping and scaling configuration, and V4L2 only
allows specifying one at a time. In principle an output size may be
achieved by scaling and cropping by different amounts, and as most drivers
use only the output format (size) in mode selection, the result could be
ambiguous. In practice this hasn't been an actual issue.

Better sensor drivers (such as smiapp) do not have this problem as the
configurations (cropping in three different places as well as binning and
scaling) can be all independently configured. So with some luck this
problem could disappear in time with newer hardware and better hardware
documentation.

I have no objections to providing the cropping and scaling information to
the user space using the selection rectangles, albeit it's somewhat against
the semantics currently. This approach would also require using compose
rectangles on the source pads which is not supported (documentation-wise)
at the moment, but it's better that way: it can be added now. There are
other, older, drivers such as omap3isp that configure scaling based on the
source format configuration only.

Perhaps a new selection flag telling the selection is read-only?

-- 
Regards,

Sakari Ailus
sakari.ailus@linux.intel.com

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Laurent Pinchart]

Hi Tomasz,

On Tue, Sep 03, 2019 at 01:16:21PM +0900, Tomasz Figa wrote:
> On Tue, Sep 3, 2019 at 1:43 AM Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 01:20:49PM +0200, Jacopo Mondi wrote:
> >> On Tue, Aug 27, 2019 at 03:51:32PM +0300, Laurent Pinchart wrote:
> >>> On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> >>>> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> >>>> control. The newly added read-only control reports the camera device
> >>>> mounting rotation.
> >>>>
> >>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>> ---
> >>>>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
> >>>>  1 file changed, 9 insertions(+)
> >>>>
> >>>> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>> index ecf151f3f0f4..03d1c23d18f7 100644
> >>>> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> >>>> @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
> >>>>
> >>>>
> >>>>
> >>>> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> >>>> +    This read-only control describes the camera sensor orientation by
> >>>> +    reporting its mounting rotation in respect to the device intended usage
> >>>> +    orientation, expressed in counter clockwise degrees. The control value is
> >>>
> >>> Counter clockwise relative to what ? :-)
> >>
> >> I think it is expressed in the previous line:
> >> "mounting rotation in respect to the device intended usage
> >> orientation, expressed in counter clockwise degrees"
> >>
> >> Would you prefer to have this re-phrased as:
> >>
> >> This read-only control describes the camera sensor orientation
> >> expressed as rotation in counter clockwise degrees in respect to
> >> the device intended usage orientation.
> >>
> >> ?
> >
> > Rotation is expressed in degrees around an axis, which is itself
> > expressed as a vector. It's pretty intuitive that the rotation vector
> > should be perpendicular to the plane of the sensor (which itself should
> > be parallel to the plane of the device side on which it is mounted, as
> > expressed by the location property), but there are still two possible
> > directions for the vector, facing in the same direction as the sensor or
> > the opposite direction. Unless there's a good reason to do so, I would
> > use the same direction as the one defined by Android. I don't know what
> > direction that is though :-)
> 
> I don't think Android exposes camera rotation to the layers above the
> HAL. The stream is expected to be pre-rotated by the HAL, taking into
> account the desired target rotation of the stream itself [1].
> 
> [1] https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#1428
> 
> That said, Android seems to use "degrees counterclockwise" for rotations.

Counterclockwise seems better to me as that's coherent with
trigonometry. We still have to define the direction of the rotation axis
though :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 02, 2019 at 09:48:30PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 02, 2019 at 07:49:37PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> >> On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> >>> On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> >>>> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> >>>>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> >>>>>> Add the 'location' device property, used to specify the camera device
> >>>>>> mounting position. The property is particularly meaningful for mobile
> >>>>>> devices with a well defined usage orientation.
> >>>>>>
> >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >>>>>>  1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> index f884ada0bffc..865f4142f432 100644
> >>>>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> @@ -89,6 +89,16 @@ Optional properties
> >>>>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> >>>>>>    (upside down).
> >>>>>>
> >>>>>> +- location: The camera sensor mounting location, expressed as a position
> >>>>>> +  relative to the usage orientation of the device the sensor is installed on.
> >>>>>
> >>>>> DT bindings being ABIs, we need to be precise and thorough there. One
> >>>>> particular point that bothers me is that the property is named location,
> >>>>> and its description refers to camera sensor mounting location.
> >>>>>
> >>>>> I see two options to fix this. One of them is to rename the property to
> >>>>> camera-location, but that would limit its future usage for other types
> >>>>> of devices. The other one is to document the property as applying to a
> >>>>> "device" instead of a "camera sensor", and add one sentence stating that
> >>>>> this property is valid for camera sensors only.
> >>>>>
> >>>>> This will require finding another name for the device that the device is
> >>>>> mounted on though, as using device twice would be very confusing.
> >>>>
> >>>> I had the same concern, but I cannot find another term to convey
> >>>> this... suggestions?
> >>>
> >>> For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> >>
> >> Pardon my ignorance, but I could not find it documented. Some binding
> >> files refers to an iio/mount-matrix.txt file which I cannot find. Has
> >> it been removed? Anyway, some individual bindings report examples of
> >> mount matrices (ie
> >> Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> >>
> >>> That would replace 'rotation'. Maybe we can do something similar here?
> >>
> >> I think 'rotation' simply expressed as degrees is fine here, our
> >> problem was to find a term that would make it possible to specify that
> >> rotation/location are applied to a 'device' mounted on a 'device'.
> >> Sakari suggested on irc to use 'system' in place of the second
> >> 'device' and that seems to work well to me.
> >>
> >> Or were you suggesting to use a construct similar to mount-matrix for
> >> a purpose I didn't get :) ?
> >
> > mount-matrix would allow exposing the rotation in a more generic way,
> > and if there are standard DT bindings, I think this would make sense,
> > especially given that we could have device with camera sensors not
> > mounted parallel to the device side.
> 
> After reading this entry you pointed me to
> https://lore.kernel.org/patchwork/patch/1044786/
> I understand why Rob suggested that, but it seems a bit far fetched to
> me... The devices that could make use of 'mount-matrix', such as the
> ones IIO deal with, sample data in a 3D space, and to compensate their
> mounting displacement a rotation matrix is required.
> 
> Do you see use cases for compensating mounting displacement in a 3-d
> space for a camera ? I'm not saying it's not possible, but seems -a
> bit- far fetched.

360° cameras come to mind, they can use multiple sensors mounted with
different angles.

> If we consider cameras as always mounted parallel to the device used
> to take picturea, the correction you could apply to the possible mounting
> displacements are just rotations along the origin (or the z axis you want
> to consider a 3 dimensional space) and requiring to supply a 2-dimensions
> rotation matrix (with only a few combinations of 0, 1 and -1, if we only
> accept multiples of 90 degrees as we agreed) it seems a bit an overkill.
> 
> True, we could also handle image flipping, not just rotation with a rotation
> matrix but an image sensor mounted flipped upside down kind of defeat its
> purpose, doesn't it ? (it would point to the inside of the device :)
> 
> When we'll have devices with movable sensors like the one Pavel
> linked, rotation matrices could be used to report the current camera
> position maybe, but to represent a mounting displacement compensation,
> for devices working in a 2 dimensional space and with fixed rotations
> of 90 degrees multiples it seems an overkill to ask to developers to
> me.
> 
> That said, there are devices out there which do things I cannot even
> imagine, and maybe could actually compensate rotations in the 3D
> space, sample 3D point maps or other advanced things. Indeed they
> could use a rotation matrix if they need to, but the purpose of
> 'rotation' is much more humble and even if it could be represented
> with 'mount-matrix' it seems to me a non justifiable effort.

Most devices will only need to report a 0, 90, 180 or 270 rotation angle
along an axis perpendicular to the device. For those, mount-matrix is
indeed a bit overkill, and a rotation property would be enough. However,
when we will need to express more than those simple configurations,
mount-matrix will become needed, and all of a sudden we will have
multiple ways to express the same information. That's what I would like
to avoid by going for mount-matrix already.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Laurent Pinchart]

Hi Jacopo,

On Mon, Sep 02, 2019 at 09:48:30PM +0200, Jacopo Mondi wrote:
> On Mon, Sep 02, 2019 at 07:49:37PM +0300, Laurent Pinchart wrote:
> > On Mon, Sep 02, 2019 at 06:40:31PM +0200, Jacopo Mondi wrote:
> >> On Mon, Sep 02, 2019 at 02:38:40PM +0100, Rob Herring wrote:
> >>> On Thu, Aug 29, 2019 at 02:46:40PM +0200, Jacopo Mondi wrote:
> >>>> On Tue, Aug 27, 2019 at 03:21:26PM +0300, Laurent Pinchart wrote:
> >>>>> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> >>>>>> Add the 'location' device property, used to specify the camera device
> >>>>>> mounting position. The property is particularly meaningful for mobile
> >>>>>> devices with a well defined usage orientation.
> >>>>>>
> >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>> ---
> >>>>>>  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >>>>>>  1 file changed, 10 insertions(+)
> >>>>>>
> >>>>>> diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> index f884ada0bffc..865f4142f432 100644
> >>>>>> --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> >>>>>> @@ -89,6 +89,16 @@ Optional properties
> >>>>>>    but a number of degrees counter clockwise. Typical values are 0 and 180
> >>>>>>    (upside down).
> >>>>>>
> >>>>>> +- location: The camera sensor mounting location, expressed as a position
> >>>>>> +  relative to the usage orientation of the device the sensor is installed on.
> >>>>>
> >>>>> DT bindings being ABIs, we need to be precise and thorough there. One
> >>>>> particular point that bothers me is that the property is named location,
> >>>>> and its description refers to camera sensor mounting location.
> >>>>>
> >>>>> I see two options to fix this. One of them is to rename the property to
> >>>>> camera-location, but that would limit its future usage for other types
> >>>>> of devices. The other one is to document the property as applying to a
> >>>>> "device" instead of a "camera sensor", and add one sentence stating that
> >>>>> this property is valid for camera sensors only.
> >>>>>
> >>>>> This will require finding another name for the device that the device is
> >>>>> mounted on though, as using device twice would be very confusing.
> >>>>
> >>>> I had the same concern, but I cannot find another term to convey
> >>>> this... suggestions?
> >>>
> >>> For accelerometers and/or gyroscopes, we already have 'mount-matrix'.
> >>
> >> Pardon my ignorance, but I could not find it documented. Some binding
> >> files refers to an iio/mount-matrix.txt file which I cannot find. Has
> >> it been removed? Anyway, some individual bindings report examples of
> >> mount matrices (ie
> >> Documentation/devicetree/bindings/iio/imu/inv_mpu6050.txt)
> >>
> >>> That would replace 'rotation'. Maybe we can do something similar here?
> >>
> >> I think 'rotation' simply expressed as degrees is fine here, our
> >> problem was to find a term that would make it possible to specify that
> >> rotation/location are applied to a 'device' mounted on a 'device'.
> >> Sakari suggested on irc to use 'system' in place of the second
> >> 'device' and that seems to work well to me.
> >>
> >> Or were you suggesting to use a construct similar to mount-matrix for
> >> a purpose I didn't get :) ?
> >
> > mount-matrix would allow exposing the rotation in a more generic way,
> > and if there are standard DT bindings, I think this would make sense,
> > especially given that we could have device with camera sensors not
> > mounted parallel to the device side.
> 
> After reading this entry you pointed me to
> https://lore.kernel.org/patchwork/patch/1044786/
> I understand why Rob suggested that, but it seems a bit far fetched to
> me... The devices that could make use of 'mount-matrix', such as the
> ones IIO deal with, sample data in a 3D space, and to compensate their
> mounting displacement a rotation matrix is required.
> 
> Do you see use cases for compensating mounting displacement in a 3-d
> space for a camera ? I'm not saying it's not possible, but seems -a
> bit- far fetched.

360° cameras come to mind, they can use multiple sensors mounted with
different angles.

> If we consider cameras as always mounted parallel to the device used
> to take picturea, the correction you could apply to the possible mounting
> displacements are just rotations along the origin (or the z axis you want
> to consider a 3 dimensional space) and requiring to supply a 2-dimensions
> rotation matrix (with only a few combinations of 0, 1 and -1, if we only
> accept multiples of 90 degrees as we agreed) it seems a bit an overkill.
> 
> True, we could also handle image flipping, not just rotation with a rotation
> matrix but an image sensor mounted flipped upside down kind of defeat its
> purpose, doesn't it ? (it would point to the inside of the device :)
> 
> When we'll have devices with movable sensors like the one Pavel
> linked, rotation matrices could be used to report the current camera
> position maybe, but to represent a mounting displacement compensation,
> for devices working in a 2 dimensional space and with fixed rotations
> of 90 degrees multiples it seems an overkill to ask to developers to
> me.
> 
> That said, there are devices out there which do things I cannot even
> imagine, and maybe could actually compensate rotations in the 3D
> space, sample 3D point maps or other advanced things. Indeed they
> could use a rotation matrix if they need to, but the purpose of
> 'rotation' is much more humble and even if it could be represented
> with 'mount-matrix' it seems to me a non justifiable effort.

Most devices will only need to report a 0, 90, 180 or 270 rotation angle
along an axis perpendicular to the device. For those, mount-matrix is
indeed a bit overkill, and a rotation property would be enough. However,
when we will need to express more than those simple configurations,
mount-matrix will become needed, and all of a sudden we will have
multiple ways to express the same information. That's what I would like
to avoid by going for mount-matrix already.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 03/10] media: v4l2-ctrl: Document V4L2_CID_CAMERA_SENSOR_ROTATION

[Jacopo Mondi]

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

Hi Tomasz, Laurent

On Tue, Sep 03, 2019 at 01:16:21PM +0900, Tomasz Figa wrote:
> On Tue, Sep 3, 2019 at 1:43 AM Laurent Pinchart
> <laurent.pinchart@ideasonboard.com> wrote:
> >
> > Hi Jacopo,
> >
> > On Mon, Sep 02, 2019 at 01:20:49PM +0200, Jacopo Mondi wrote:
> > > On Tue, Aug 27, 2019 at 03:51:32PM +0300, Laurent Pinchart wrote:
> > > > On Tue, Aug 27, 2019 at 11:23:29AM +0200, Jacopo Mondi wrote:
> > > >> Add documentation for the V4L2_CID_CAMERA_SENSOR_ROTATION camera
> > > >> control. The newly added read-only control reports the camera device
> > > >> mounting rotation.
> > > >>
> > > >> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > >> ---
> > > >>  Documentation/media/uapi/v4l/ext-ctrls-camera.rst | 9 +++++++++
> > > >>  1 file changed, 9 insertions(+)
> > > >>
> > > >> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > >> index ecf151f3f0f4..03d1c23d18f7 100644
> > > >> --- a/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > >> +++ b/Documentation/media/uapi/v4l/ext-ctrls-camera.rst
> > > >> @@ -544,6 +544,15 @@ enum v4l2_scene_mode -
> > > >>
> > > >>
> > > >>
> > > >> +``V4L2_CID_CAMERA_SENSOR_ROTATION (integer)``
> > > >> +    This read-only control describes the camera sensor orientation by
> > > >> +    reporting its mounting rotation in respect to the device intended usage
> > > >> +    orientation, expressed in counter clockwise degrees. The control value is
> > > >
> > > > Counter clockwise relative to what ? :-)
> > >
> > > I think it is expressed in the previous line:
> > > "mounting rotation in respect to the device intended usage
> > > orientation, expressed in counter clockwise degrees"
> > >
> > > Would you prefer to have this re-phrased as:
> > >
> > > This read-only control describes the camera sensor orientation
> > > expressed as rotation in counter clockwise degrees in respect to
> > > the device intended usage orientation.
> > >
> > > ?
> >
> > Rotation is expressed in degrees around an axis, which is itself
> > expressed as a vector. It's pretty intuitive that the rotation vector
> > should be perpendicular to the plane of the sensor (which itself should
> > be parallel to the plane of the device side on which it is mounted, as
> > expressed by the location property), but there are still two possible
> > directions for the vector, facing in the same direction as the sensor or
> > the opposite direction. Unless there's a good reason to do so, I would
> > use the same direction as the one defined by Android. I don't know what
> > direction that is though :-)

What about:
   This read-only control describes the sensor orientation
   expressed as rotation in counterclockwise degrees along the axis
   perpendicular to the device mounting reference plane, assuming a
   canonical right-handed orientation with the y axis being positive
   towards the system's top and negative towards the bottom, and the
   axis directed pointing away from the sensor lens. Possible values
   for the control are 90, 180 and 270 degrees. To compensate the
   device mounting rotation on the captured images, a rotation of
   the same amount of degrees, in the same counterclockwise rotation
   direction should be applied along the axis directed from the
   observer to the captured image when displayed on a screen

This is for the control, while the 'rotation' DT property is simply
documented as:

- rotation: The device, typically an image sensor, is not mounted upright,
  but a number of degrees counter clockwise. Typical values are 0 and 180
  (upside down).

I would expand it with a description of the effects of rotation on
captured images and how that should be compensated.

Let me have some ascii-art fun:
-------------------------------------------------------------------------------

  To understand the effect of the sensor rotation on the acquired
  images, it is helpful to consider a fictional scan-out sequence of
  the sensor's pixels.

  Assuming the pixel array having its top-left pixel at position (0,
  0) with x increasing towards the right direction, and y increasing
  towards the bottom direction, the effect of sensor rotation could be
  easily visualized considering the sequence of captured pixels.

  Assuming the following scene has to be captured

                o
               -|-
               / \

  An upright mounted sensor has its pixel array displaced as follows

                                 x
      (0,0)---------------------->
        ! 0,0 0,1 0,2 ... 0,line-len
        ! 1,0 1,1 1,2 ...
        ! ...
        ! ...
        ! (num-col,0)...  (num-col,line-len)
      y V


  Assuming pixels are scanned out from (0,0) to (num-col,line-len)
  progressively.

    (0,0) ------------------> (0,line-len)---!
      !------------------------------------<-!
      V
    (1,0) ------------------> (1,line-len)---!
      !------------------------------------<-!
      V
    (...) .-----------------> ( ,,,, )    ---!
      !------------------------------------<-!
      V
    (num-col,0)------------->(num-col,line-len)

  If a rotation of 90 degrees counterclockwise along the axis
  perpendicular to the sensor's lens and directed towards the scene
  to be captured is applied to the sensor, the pixel array would then
  be rotated as follows

      x ^  0,line-len,,,(num-col,line-len
        !  ....
        !  0,2 1,2      ...
        !  0,1 1,1      ...
        !  0,0 1,0 ... num-col,0
       (0,0)------------------------>
                                   y

   And the pixel scan-out sequence would then proceed as follows

   (0,line-len)            (num-cols,line-len)
        ^\    ^\    ^\    ^\    ^
        ! \   ! \   ! \   ! \   !
        !  \  !  \  !  \  !  \  !
        !   \ !   \ !   \ !   \ !
        !    \!    \!    \!    \!
      (0,0)  (1,0) ....      (num-cols,0)

   Which when applied on the capture scene gives

   (0,line-len)            (num-cols,line-len)
        ^\    ^\    ^\    ^\    ^
        ! \   ! \   0 \   ! \   !
        !  \  !  \ -|- \  !  \  !
        !   \ !    / \  \ !   \ !
        !    \!    \!    \!    \!
      (0,0)  (1,0) ....      (num-cols,0)

   Producing the following image once captured to memory and
   displayed to the user

         \ !
           --0
         / !

   which (ascii-art shortcomings apart in the drawing) has a rotation
   of the  same amount of degrees applied on the opposite rotation direction
   along the axis that goes from the observer to the displayed image.

   To summarize, to compensate the sensor mounting rotation, when expressed
   as counterclockwise rotation along the axis directed from the sensor
   the captured scene, a rotation of the same amount of degrees in the
   same counterclockwise rotation direction applied along the axis
   directed from the observer to the captured image, has to be applied.

 -------                     -------                        -------
 |   o  |  90deg sensor      | \ !  |  90degrees image      |   o  |
 |  -|- |  counterclockwise  |  --0 | counterclockwise      |  -|- |
 |  / \ |  mounting rotation | / !  | compensation rotation |  / \ |
 -------                     -------                        -------

------------------End of ascii-art fun -----------------------------------------

As it took me some off-line discussions to get this right (thanks :)
do you think something along these lines should be anywhere in the
control or the property documentation ?

I know you're still in favour of going from something like the
'mount-matrix' property Rob suggested, but I still think it's an
unnecessary complication for such a 'trivial' concept. Asking developers
to provide a 3D rotation matrix where they could only express
rotations of 90 degrees multiples on 2 axes out of tree is calling for
them simply ignoring this complication and going for custom methods to
report the mounting rotation.

>
> I don't think Android exposes camera rotation to the layers above the
> HAL. The stream is expected to be pre-rotated by the HAL, taking into
> account the desired target rotation of the stream itself [1].
>
> [1] https://android.googlesource.com/platform/hardware/libhardware/+/master/include/hardware/camera3.h#1428
>
> That said, Android seems to use "degrees counterclockwise" for rotations.

https://jmondi.org/android_metadata_tags/docs.html#static_android.lens.facing
I see the property description reporting

"Clockwise angle through which the output image needs to be rotated to be
upright on the device screen in its native orientation."

Who knows which direction of the rotation axis do they take into
account :)

Thanks
  j

>
> Best regards,
> Tomasz

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

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

Hi Sakari, Hans,

On Tue, Sep 03, 2019 at 04:06:26PM +0300, Sakari Ailus wrote:
> Hi Hans, Jacopo,
>
> On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> > On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> > > HI Hans,
> > >
> > > On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> > >> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> > >>> Report the native pixel array size and the crop bounds for the ov5670
> > >>> sensor driver.
> > >>>
> > >>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>> ---
> > >>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> > >>>  1 file changed, 20 insertions(+)
> > >>>
> > >>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > >>> index 2bc57e85f721..3e22fe9ccad1 100644
> > >>> --- a/drivers/media/i2c/ov5670.c
> > >>> +++ b/drivers/media/i2c/ov5670.c
> > >>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> > >>>  	return 0;
> > >>>  }
> > >>>
> > >>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> > >>> +				struct v4l2_subdev_pad_config *cfg,
> > >>> +				struct v4l2_subdev_selection *sel)
> > >>> +{
> > >>> +	switch (sel->target) {
> > >>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> > >>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> > >>> +		sel->r.left = 0;
> > >>> +		sel->r.top = 0;
> > >>> +		sel->r.width = 2592;
> > >>> +		sel->r.height = 1944;
> > >>
> > >> Why do you need this?
> > >>
> > >
> > > I need to expose the pixel array size and the active pixel area size,
> > > and I interpreted the two above targets as the right place where to do
> > > so.
> >
> > That didn't answer my question :-)
> >
> > Why do you need to expose this? Who uses it for what purpose?
> >
> > >
> > > At least for NATIVE_SIZE the documentation seems to match my
> > > understanding:
> > >
> > > "The native size of the device, e.g. a sensor’s pixel array. left and top
> > > fields are zero for this target."
> >
> > Correct.
> >
> > >
> > >
> > >> Since the format can change for this and the next driver I think CROP_BOUNDS
> > >> at least should match the current format.
> > >>
> > >
> > > Ah, does it? It was not clear to me from the documentation, as it
> > > suggested to me that target actually identifies the active pixel area
> > >
> > > "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> > > crop bounds rectangle."
> > >
> > > It does not mention format, should this be updated?
> >
> > The problem is that for sensors it is indeed not clear.
> >
> > In principle the crop bounds matches the resolution that the sensor uses
> > pre-scaling. So yes, that means that it is equal to the native size.
> >
> > But many sensors have a discrete list of supported formats and it is not
> > clear how they map each format to the actual sensor. If it is clearly just
> > done through binning or averaging, then all is fine.
>
> Sensor drivers do; sensors themselves support much, much more than most
> drivers allow. But this is due to the nature of information available to
> the sensor driver developers, not really something that is trivial to
> change.
>
> >
> > If the formats have different aspect ratios, then it becomes a bit more
> > difficult how to relate the crop bounds with the format since you may not
> > know to which sensor area the current format corresponds.
> >
> > I have no clear answer for you, to be honest, but it can get tricky.
>
> I've suggested earlier that the crop and compose selection targets to be
> used to convey the cropping and binning (or scaling) that is done on the
> sensor, in that order. In reality, there are usually more than one
> (sometimes three) inside a sensor to crop, and often more than one place to
> scale as well. So the driver would need to accommodate this.
>
> The modes come with both cropping and scaling configuration, and V4L2 only
> allows specifying one at a time. In principle an output size may be
> achieved by scaling and cropping by different amounts, and as most drivers
> use only the output format (size) in mode selection, the result could be
> ambiguous. In practice this hasn't been an actual issue.
>
> Better sensor drivers (such as smiapp) do not have this problem as the
> configurations (cropping in three different places as well as binning and
> scaling) can be all independently configured. So with some luck this
> problem could disappear in time with newer hardware and better hardware
> documentation.
>
> I have no objections to providing the cropping and scaling information to
> the user space using the selection rectangles, albeit it's somewhat against
> the semantics currently. This approach would also require using compose
> rectangles on the source pads which is not supported (documentation-wise)
> at the moment, but it's better that way: it can be added now. There are
> other, older, drivers such as omap3isp that configure scaling based on the
> source format configuration only.

Thanks for all information here, but I think we've gone a bit far
from my original point. The cropping and scaling informations you
mention are, in my understanding, the portion of the pixel array which
is fed to the ISP before scaling, and the result of the ISP
binning/averaging respectively. Those information indeed depends on the
desired capture resolution, the driver implementation, and the sensor
capabilities.

What I was interested in was just reporting to userspace the physical
size of the active pixel array area, which should reasonably be
defined as a sub-portion of the native pixel array, excluding the back
calibration pixels.

In one of the previous emails Hans suggested to use CROP_DEFAULT for
that purpose, but in the documentation it is reported not to apply to
subdevice :(

Do we need a new target, similar to V4L2_SEL_TGT_NATIVE_SIZE that
could maybe report multiple rectangles to accommodate cross-shaped
sensors?

Are the selection API the right place to report these information?
With a control, it might be easier to report such a static
information...

Thanks
   j

>
> Perhaps a new selection flag telling the selection is read-only?
>
> --
> Regards,
>
> Sakari Ailus
> sakari.ailus@linux.intel.com

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Mauro Carvalho Chehab]

Em Tue, 27 Aug 2019 15:21:26 +0300
Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:

> Hi Jacopo,
> 
> Thank you for the patch.
> 
> On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > Add the 'location' device property, used to specify the camera device
> > mounting position. The property is particularly meaningful for mobile
> > devices with a well defined usage orientation.
> > 
> > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > ---
> >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> >  1 file changed, 10 insertions(+)
> > 
> > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > index f884ada0bffc..865f4142f432 100644
> > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > @@ -89,6 +89,16 @@ Optional properties
> >    but a number of degrees counter clockwise. Typical values are 0 and 180
> >    (upside down).
> > 
> > +- location: The camera sensor mounting location, expressed as a position
> > +  relative to the usage orientation of the device the sensor is installed on.  
> 
> DT bindings being ABIs, we need to be precise and thorough there. One
> particular point that bothers me is that the property is named location,
> and its description refers to camera sensor mounting location.

Yeah, "location" doesn't sound a good name for me neither.
 
> 
> I see two options to fix this. One of them is to rename the property to
> camera-location, but that would limit its future usage for other types
> of devices. The other one is to document the property as applying to a
> "device" instead of a "camera sensor", and add one sentence stating that
> this property is valid for camera sensors only.
> 
> This will require finding another name for the device that the device is
> mounted on though, as using device twice would be very confusing.
> 
> > +  Possible values are:
> > +  0 - Front. The image sensor is mounted on the front facing side of the device.
> > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > +  the user facing side of the device.
> > +  1 - Back. The image sensor is mounted on the back side of the device, which is
> > +  defined as the opposite side of the front facing one.
> > +  2 - External. The image sensor is connected to the device by extension cables,
> > +  and can be freely moved, regardless of the device position. 

Hmm...

Besides the point that Laurent and Rob already commented, just those 3 options 
doesn't seem good enough. I was reading a public article yesterday about a new
device (Samsung Galaxy Fold[1]) with comes with 6 cameras, being 3 at back, 
1 at front, when the device is opened, and 1 camera that could be either at the
back or at the front, depending if the device is opened or not. 

Btw, on a device with multiple cameras at the same side, it would
make sense to also be able to uniquely identify the location of each
sensor somehow.

There are also some other new devices with a front motorized slider camera
that sits hidden inside the phone, until when someone uses it.

Thanks,
Mauro

Re: [PATCH v2 01/10] media: dt-bindings: Document 'location' property

[Jacopo Mondi]

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

Hi Mauro,

On Thu, Sep 12, 2019 at 09:51:47AM -0300, Mauro Carvalho Chehab wrote:
> Em Tue, 27 Aug 2019 15:21:26 +0300
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> escreveu:
>
> > Hi Jacopo,
> >
> > Thank you for the patch.
> >
> > On Tue, Aug 27, 2019 at 11:23:27AM +0200, Jacopo Mondi wrote:
> > > Add the 'location' device property, used to specify the camera device
> > > mounting position. The property is particularly meaningful for mobile
> > > devices with a well defined usage orientation.
> > >
> > > Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > > ---
> > >  .../devicetree/bindings/media/video-interfaces.txt     | 10 ++++++++++
> > >  1 file changed, 10 insertions(+)
> > >
> > > diff --git a/Documentation/devicetree/bindings/media/video-interfaces.txt b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > index f884ada0bffc..865f4142f432 100644
> > > --- a/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > +++ b/Documentation/devicetree/bindings/media/video-interfaces.txt
> > > @@ -89,6 +89,16 @@ Optional properties
> > >    but a number of degrees counter clockwise. Typical values are 0 and 180
> > >    (upside down).
> > >
> > > +- location: The camera sensor mounting location, expressed as a position
> > > +  relative to the usage orientation of the device the sensor is installed on.
> >
> > DT bindings being ABIs, we need to be precise and thorough there. One
> > particular point that bothers me is that the property is named location,
> > and its description refers to camera sensor mounting location.
>
> Yeah, "location" doesn't sound a good name for me neither.
>

I think Laurent referred to the fact that as we changed the property
name to 'location' it is not a good idea to refer to 'camera sensor'
as it could refer to flash leds too (and potentially other devices).

In v3 I have
s/camera sensor/device
s/device/system

How would you name the property?

> >
> > I see two options to fix this. One of them is to rename the property to
> > camera-location, but that would limit its future usage for other types
> > of devices. The other one is to document the property as applying to a
> > "device" instead of a "camera sensor", and add one sentence stating that
> > this property is valid for camera sensors only.
> >
> > This will require finding another name for the device that the device is
> > mounted on though, as using device twice would be very confusing.
> >
> > > +  Possible values are:
> > > +  0 - Front. The image sensor is mounted on the front facing side of the device.
> > > +  For mobile devices such as smartphones, tablets and laptops the front side is
> > > +  the user facing side of the device.
> > > +  1 - Back. The image sensor is mounted on the back side of the device, which is
> > > +  defined as the opposite side of the front facing one.
> > > +  2 - External. The image sensor is connected to the device by extension cables,
> > > +  and can be freely moved, regardless of the device position.
>
> Hmm...
>
> Besides the point that Laurent and Rob already commented, just those 3 options
> doesn't seem good enough. I was reading a public article yesterday about a new
> device (Samsung Galaxy Fold[1]) with comes with 6 cameras, being 3 at back,
> 1 at front, when the device is opened, and 1 camera that could be either at the
> back or at the front, depending if the device is opened or not.
>

Pavel linked a similar device in a previous reply:
https://www.samsung.com/global/galaxy/galaxy-a80/

I understand the above options might not get enough in future, but
right now they cover the 99,9% of devices in the market, and those
phones are far from being mainline supported I presume.

> Btw, on a device with multiple cameras at the same side, it would
> make sense to also be able to uniquely identify the location of each
> sensor somehow.
>

I think the location property should not identify devices, but just
report where they are installed. Does having two cameras reporting the
same mounting location bother you?


> There are also some other new devices with a front motorized slider camera
> that sits hidden inside the phone, until when someone uses it.
>

So it's an HIDDEN_FRONT_CAMERA ? :)

Thanks
   j


> Thanks,
> Mauro

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Hans Verkuil]

On 9/3/19 6:49 PM, Jacopo Mondi wrote:
> Hi Sakari, Hans,
> 
> On Tue, Sep 03, 2019 at 04:06:26PM +0300, Sakari Ailus wrote:
>> Hi Hans, Jacopo,
>>
>> On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
>>> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
>>>> HI Hans,
>>>>
>>>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
>>>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
>>>>>> Report the native pixel array size and the crop bounds for the ov5670
>>>>>> sensor driver.
>>>>>>
>>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
>>>>>> ---
>>>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
>>>>>>  1 file changed, 20 insertions(+)
>>>>>>
>>>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
>>>>>> index 2bc57e85f721..3e22fe9ccad1 100644
>>>>>> --- a/drivers/media/i2c/ov5670.c
>>>>>> +++ b/drivers/media/i2c/ov5670.c
>>>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
>>>>>>  	return 0;
>>>>>>  }
>>>>>>
>>>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
>>>>>> +				struct v4l2_subdev_pad_config *cfg,
>>>>>> +				struct v4l2_subdev_selection *sel)
>>>>>> +{
>>>>>> +	switch (sel->target) {
>>>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
>>>>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
>>>>>> +		sel->r.left = 0;
>>>>>> +		sel->r.top = 0;
>>>>>> +		sel->r.width = 2592;
>>>>>> +		sel->r.height = 1944;
>>>>>
>>>>> Why do you need this?
>>>>>
>>>>
>>>> I need to expose the pixel array size and the active pixel area size,
>>>> and I interpreted the two above targets as the right place where to do
>>>> so.
>>>
>>> That didn't answer my question :-)
>>>
>>> Why do you need to expose this? Who uses it for what purpose?
>>>
>>>>
>>>> At least for NATIVE_SIZE the documentation seems to match my
>>>> understanding:
>>>>
>>>> "The native size of the device, e.g. a sensor’s pixel array. left and top
>>>> fields are zero for this target."
>>>
>>> Correct.
>>>
>>>>
>>>>
>>>>> Since the format can change for this and the next driver I think CROP_BOUNDS
>>>>> at least should match the current format.
>>>>>
>>>>
>>>> Ah, does it? It was not clear to me from the documentation, as it
>>>> suggested to me that target actually identifies the active pixel area
>>>>
>>>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
>>>> crop bounds rectangle."
>>>>
>>>> It does not mention format, should this be updated?
>>>
>>> The problem is that for sensors it is indeed not clear.
>>>
>>> In principle the crop bounds matches the resolution that the sensor uses
>>> pre-scaling. So yes, that means that it is equal to the native size.
>>>
>>> But many sensors have a discrete list of supported formats and it is not
>>> clear how they map each format to the actual sensor. If it is clearly just
>>> done through binning or averaging, then all is fine.
>>
>> Sensor drivers do; sensors themselves support much, much more than most
>> drivers allow. But this is due to the nature of information available to
>> the sensor driver developers, not really something that is trivial to
>> change.
>>
>>>
>>> If the formats have different aspect ratios, then it becomes a bit more
>>> difficult how to relate the crop bounds with the format since you may not
>>> know to which sensor area the current format corresponds.
>>>
>>> I have no clear answer for you, to be honest, but it can get tricky.
>>
>> I've suggested earlier that the crop and compose selection targets to be
>> used to convey the cropping and binning (or scaling) that is done on the
>> sensor, in that order. In reality, there are usually more than one
>> (sometimes three) inside a sensor to crop, and often more than one place to
>> scale as well. So the driver would need to accommodate this.
>>
>> The modes come with both cropping and scaling configuration, and V4L2 only
>> allows specifying one at a time. In principle an output size may be
>> achieved by scaling and cropping by different amounts, and as most drivers
>> use only the output format (size) in mode selection, the result could be
>> ambiguous. In practice this hasn't been an actual issue.
>>
>> Better sensor drivers (such as smiapp) do not have this problem as the
>> configurations (cropping in three different places as well as binning and
>> scaling) can be all independently configured. So with some luck this
>> problem could disappear in time with newer hardware and better hardware
>> documentation.
>>
>> I have no objections to providing the cropping and scaling information to
>> the user space using the selection rectangles, albeit it's somewhat against
>> the semantics currently. This approach would also require using compose
>> rectangles on the source pads which is not supported (documentation-wise)
>> at the moment, but it's better that way: it can be added now. There are
>> other, older, drivers such as omap3isp that configure scaling based on the
>> source format configuration only.
> 
> Thanks for all information here, but I think we've gone a bit far
> from my original point. The cropping and scaling informations you
> mention are, in my understanding, the portion of the pixel array which
> is fed to the ISP before scaling, and the result of the ISP
> binning/averaging respectively. Those information indeed depends on the
> desired capture resolution, the driver implementation, and the sensor
> capabilities.
> 
> What I was interested in was just reporting to userspace the physical
> size of the active pixel array area, which should reasonably be
> defined as a sub-portion of the native pixel array, excluding the back
> calibration pixels.
> 
> In one of the previous emails Hans suggested to use CROP_DEFAULT for
> that purpose, but in the documentation it is reported not to apply to
> subdevice :(
> 
> Do we need a new target, similar to V4L2_SEL_TGT_NATIVE_SIZE that
> could maybe report multiple rectangles to accommodate cross-shaped
> sensors?
> 
> Are the selection API the right place to report these information?
> With a control, it might be easier to report such a static
> information...

Sorry for the late follow-up.

I am uncomfortable with hacking something here. I strongly feel that we
are missing a proper API to configure a sensor.

If I look at video receivers (SDTV or HDTV), then in both cases you set up
the incoming video resolution with S_STD or S_DV_TIMINGS. This explicitly
defines the incoming image size and all crop/compose/scale operations
operate on that image size.

In my opinion we are missing an equivalent to that for sensors. I think the
original thinking was that sensors have a fixed pixel array, so there is
nothing to configure. But with cross-shaped sensors, binning/skipping,
sensors that just hide all the inner details and just report a set of
discrete framesizes, this is not actually trivial anymore.

And the odd thing is that our API does have discovery (at least to some
extent) of the various options through ENUM_FRAMESIZES, but does not let
you choose a specific variant. We abuse S_FMT for this, which is really
not the right ioctl to use since it defines what you want to receive in
memory after cropping/composing/scaling, not the initial image size before
all these operations take place.

Regarding binning and skipping: I never understood why this wasn't
configured using controls. Is there a reason why it was never implemented
like that?

If we had a Set Source Size operation (either a control, new ioctl or selection
target) then you can use that to select a specific source size based on what
ENUM_FRAMESIZES reports. Sensor drivers can choose to either enumerate
variants based on the cross-shape and binning/skipping, or just enumerate variants
based on the cross-shape and add binning/skipping controls to explicitly set
this up.

In any case, since you now know exactly which image size the sensor produces,
you can set up all the selection rectangles associated with that correctly.

Since I am not a sensor expert I no doubt missed things, but in my view we
really need to see a sensor not as a fixed array, but as something more akin
to video receiver, i.e. you need to explicitly configure the video source
before cropping/composing/scaling takes place.

So in the example of a cross-shaped sensor ENUM_FRAMESIZES would report
two sizes (landscape/portrait), binning/cropping controls (optional, this
might be implicit in the enumerated framesizes), and after configuring all
this using the new API 'Set Source Size' (or whatever it will be called) and
possible binning/skipping controls, the user can read various selection
targets to get all the needed information.

Any cropping/composing/scaling will be done based on the selected image size
+ binning/skipping.

Re binning/skipping: one other option is to add a flags field to v4l2_frmsizeenum
that reports if the reported size was achieved via binning and/or skipping.

You wouldn't need controls in that case.

Regards,

	Hans

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Jacopo Mondi]

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

Hi Hans, thanks for the lenghty reply

On Thu, Sep 26, 2019 at 10:11:51AM +0200, Hans Verkuil wrote:
> On 9/3/19 6:49 PM, Jacopo Mondi wrote:
> > Hi Sakari, Hans,
> >
> > On Tue, Sep 03, 2019 at 04:06:26PM +0300, Sakari Ailus wrote:
> >> Hi Hans, Jacopo,
> >>
> >> On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> >>> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> >>>> HI Hans,
> >>>>
> >>>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> >>>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> >>>>>> Report the native pixel array size and the crop bounds for the ov5670
> >>>>>> sensor driver.
> >>>>>>
> >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> >>>>>> ---
> >>>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> >>>>>>  1 file changed, 20 insertions(+)
> >>>>>>
> >>>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> >>>>>> index 2bc57e85f721..3e22fe9ccad1 100644
> >>>>>> --- a/drivers/media/i2c/ov5670.c
> >>>>>> +++ b/drivers/media/i2c/ov5670.c
> >>>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> >>>>>>  	return 0;
> >>>>>>  }
> >>>>>>
> >>>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> >>>>>> +				struct v4l2_subdev_pad_config *cfg,
> >>>>>> +				struct v4l2_subdev_selection *sel)
> >>>>>> +{
> >>>>>> +	switch (sel->target) {
> >>>>>> +	case V4L2_SEL_TGT_CROP_BOUNDS:
> >>>>>> +	case V4L2_SEL_TGT_NATIVE_SIZE:
> >>>>>> +		sel->r.left = 0;
> >>>>>> +		sel->r.top = 0;
> >>>>>> +		sel->r.width = 2592;
> >>>>>> +		sel->r.height = 1944;
> >>>>>
> >>>>> Why do you need this?
> >>>>>
> >>>>
> >>>> I need to expose the pixel array size and the active pixel area size,
> >>>> and I interpreted the two above targets as the right place where to do
> >>>> so.
> >>>
> >>> That didn't answer my question :-)
> >>>
> >>> Why do you need to expose this? Who uses it for what purpose?
> >>>
> >>>>
> >>>> At least for NATIVE_SIZE the documentation seems to match my
> >>>> understanding:
> >>>>
> >>>> "The native size of the device, e.g. a sensor’s pixel array. left and top
> >>>> fields are zero for this target."
> >>>
> >>> Correct.
> >>>
> >>>>
> >>>>
> >>>>> Since the format can change for this and the next driver I think CROP_BOUNDS
> >>>>> at least should match the current format.
> >>>>>
> >>>>
> >>>> Ah, does it? It was not clear to me from the documentation, as it
> >>>> suggested to me that target actually identifies the active pixel area
> >>>>
> >>>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> >>>> crop bounds rectangle."
> >>>>
> >>>> It does not mention format, should this be updated?
> >>>
> >>> The problem is that for sensors it is indeed not clear.
> >>>
> >>> In principle the crop bounds matches the resolution that the sensor uses
> >>> pre-scaling. So yes, that means that it is equal to the native size.
> >>>
> >>> But many sensors have a discrete list of supported formats and it is not
> >>> clear how they map each format to the actual sensor. If it is clearly just
> >>> done through binning or averaging, then all is fine.
> >>
> >> Sensor drivers do; sensors themselves support much, much more than most
> >> drivers allow. But this is due to the nature of information available to
> >> the sensor driver developers, not really something that is trivial to
> >> change.
> >>
> >>>
> >>> If the formats have different aspect ratios, then it becomes a bit more
> >>> difficult how to relate the crop bounds with the format since you may not
> >>> know to which sensor area the current format corresponds.
> >>>
> >>> I have no clear answer for you, to be honest, but it can get tricky.
> >>
> >> I've suggested earlier that the crop and compose selection targets to be
> >> used to convey the cropping and binning (or scaling) that is done on the
> >> sensor, in that order. In reality, there are usually more than one
> >> (sometimes three) inside a sensor to crop, and often more than one place to
> >> scale as well. So the driver would need to accommodate this.
> >>
> >> The modes come with both cropping and scaling configuration, and V4L2 only
> >> allows specifying one at a time. In principle an output size may be
> >> achieved by scaling and cropping by different amounts, and as most drivers
> >> use only the output format (size) in mode selection, the result could be
> >> ambiguous. In practice this hasn't been an actual issue.
> >>
> >> Better sensor drivers (such as smiapp) do not have this problem as the
> >> configurations (cropping in three different places as well as binning and
> >> scaling) can be all independently configured. So with some luck this
> >> problem could disappear in time with newer hardware and better hardware
> >> documentation.
> >>
> >> I have no objections to providing the cropping and scaling information to
> >> the user space using the selection rectangles, albeit it's somewhat against
> >> the semantics currently. This approach would also require using compose
> >> rectangles on the source pads which is not supported (documentation-wise)
> >> at the moment, but it's better that way: it can be added now. There are
> >> other, older, drivers such as omap3isp that configure scaling based on the
> >> source format configuration only.
> >
> > Thanks for all information here, but I think we've gone a bit far
> > from my original point. The cropping and scaling informations you
> > mention are, in my understanding, the portion of the pixel array which
> > is fed to the ISP before scaling, and the result of the ISP
> > binning/averaging respectively. Those information indeed depends on the
> > desired capture resolution, the driver implementation, and the sensor
> > capabilities.
> >
> > What I was interested in was just reporting to userspace the physical
> > size of the active pixel array area, which should reasonably be
> > defined as a sub-portion of the native pixel array, excluding the back
> > calibration pixels.
> >
> > In one of the previous emails Hans suggested to use CROP_DEFAULT for
> > that purpose, but in the documentation it is reported not to apply to
> > subdevice :(
> >
> > Do we need a new target, similar to V4L2_SEL_TGT_NATIVE_SIZE that
> > could maybe report multiple rectangles to accommodate cross-shaped
> > sensors?
> >
> > Are the selection API the right place to report these information?
> > With a control, it might be easier to report such a static
> > information...
>
> Sorry for the late follow-up.
>
> I am uncomfortable with hacking something here. I strongly feel that we
> are missing a proper API to configure a sensor.
>
> If I look at video receivers (SDTV or HDTV), then in both cases you set up
> the incoming video resolution with S_STD or S_DV_TIMINGS. This explicitly
> defines the incoming image size and all crop/compose/scale operations
> operate on that image size.
>
> In my opinion we are missing an equivalent to that for sensors. I think the
> original thinking was that sensors have a fixed pixel array, so there is
> nothing to configure. But with cross-shaped sensors, binning/skipping,
> sensors that just hide all the inner details and just report a set of
> discrete framesizes, this is not actually trivial anymore.
>
> And the odd thing is that our API does have discovery (at least to some
> extent) of the various options through ENUM_FRAMESIZES, but does not let
> you choose a specific variant. We abuse S_FMT for this, which is really
> not the right ioctl to use since it defines what you want to receive in
> memory after cropping/composing/scaling, not the initial image size before
> all these operations take place.
>
> Regarding binning and skipping: I never understood why this wasn't
> configured using controls. Is there a reason why it was never implemented
> like that?
>
> If we had a Set Source Size operation (either a control, new ioctl or selection
> target) then you can use that to select a specific source size based on what
> ENUM_FRAMESIZES reports. Sensor drivers can choose to either enumerate
> variants based on the cross-shape and binning/skipping, or just enumerate variants
> based on the cross-shape and add binning/skipping controls to explicitly set
> this up.
>
> In any case, since you now know exactly which image size the sensor produces,
> you can set up all the selection rectangles associated with that correctly.
>
> Since I am not a sensor expert I no doubt missed things, but in my view we
> really need to see a sensor not as a fixed array, but as something more akin
> to video receiver, i.e. you need to explicitly configure the video source
> before cropping/composing/scaling takes place.
>
> So in the example of a cross-shaped sensor ENUM_FRAMESIZES would report
> two sizes (landscape/portrait), binning/cropping controls (optional, this
> might be implicit in the enumerated framesizes), and after configuring all
> this using the new API 'Set Source Size' (or whatever it will be called) and
> possible binning/skipping controls, the user can read various selection
> targets to get all the needed information.
>
> Any cropping/composing/scaling will be done based on the selected image size
> + binning/skipping.

If I'm not mistaken smiapp sensor driver models this by exposing
multiple subdevices, one for the pixel array and additional ones for
the scaler (and the CSI-2 TX port, iirc from my multiplexed stream
support adventures). Sakari knows better for sure here...

Anyway, going towards a model where sensor expose several subdeves
would allow to have one for each configurable part of the sensor
processing pipeline, such as one for the raw pixel array (where one could
support cross-shaped sensors as you have suggested), one for ISP input
where to select the area of the pixel array to feed to the ISP, and one
to model the final processing stage where to to get to the final
image from, obtained through binning/skipping/averaging whatever and
possibly by applying a composition rectangle or selecting the
desired output format, if the sensor supports so.

Am I over-simplifying things here? I fear this is mostly frowned upon by
the constant lack of decent documentation from sensor manufacturers,
as most drivers still relies on magic blobs 'guaranteed to work' and
often produced by some 'certified' tuning applications..

And yes, more burden on userspace, as right now most sensors are a single
entity that gives you images in one format/resolution and that's it,
but this clearly is showing limits. On this side, libcamera could be a
great help and sensor-specific configurations will be offloaded there.

One quick consideration from my limited experience with sensors: the
method used to obtain the final image from the portion of the pixel
array fed to the ISP is not something often freely selectable. From
the sensor I've seen the manuals of, to obtain a certain resolution
you use averaging/binning, for another one skipping or direct crop of
the full pixel array etc.. So I'm not sure a control would fit well here.

Again, I might be really simplifying things :)

>
> Re binning/skipping: one other option is to add a flags field to v4l2_frmsizeenum
> that reports if the reported size was achieved via binning and/or skipping.
>

Yes, but in this case that would be a read-only information, not sure
what you would use it for

Thanks
  j

> You wouldn't need controls in that case.
>
> Regards,
>
> 	Hans

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v2 09/10] media: i2c: ov5670: Report native size and crop bounds

[Tomasz Figa]

On Fri, Sep 27, 2019 at 3:55 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Hans, thanks for the lenghty reply
>
> On Thu, Sep 26, 2019 at 10:11:51AM +0200, Hans Verkuil wrote:
> > On 9/3/19 6:49 PM, Jacopo Mondi wrote:
> > > Hi Sakari, Hans,
> > >
> > > On Tue, Sep 03, 2019 at 04:06:26PM +0300, Sakari Ailus wrote:
> > >> Hi Hans, Jacopo,
> > >>
> > >> On Thu, Aug 29, 2019 at 02:55:30PM +0200, Hans Verkuil wrote:
> > >>> On 8/29/19 2:40 PM, Jacopo Mondi wrote:
> > >>>> HI Hans,
> > >>>>
> > >>>> On Thu, Aug 29, 2019 at 12:20:18PM +0200, Hans Verkuil wrote:
> > >>>>> On 8/27/19 11:23 AM, Jacopo Mondi wrote:
> > >>>>>> Report the native pixel array size and the crop bounds for the ov5670
> > >>>>>> sensor driver.
> > >>>>>>
> > >>>>>> Signed-off-by: Jacopo Mondi <jacopo@jmondi.org>
> > >>>>>> ---
> > >>>>>>  drivers/media/i2c/ov5670.c | 20 ++++++++++++++++++++
> > >>>>>>  1 file changed, 20 insertions(+)
> > >>>>>>
> > >>>>>> diff --git a/drivers/media/i2c/ov5670.c b/drivers/media/i2c/ov5670.c
> > >>>>>> index 2bc57e85f721..3e22fe9ccad1 100644
> > >>>>>> --- a/drivers/media/i2c/ov5670.c
> > >>>>>> +++ b/drivers/media/i2c/ov5670.c
> > >>>>>> @@ -2258,6 +2258,25 @@ static int ov5670_set_pad_format(struct v4l2_subdev *sd,
> > >>>>>>        return 0;
> > >>>>>>  }
> > >>>>>>
> > >>>>>> +static int ov5670_get_selection(struct v4l2_subdev *sd,
> > >>>>>> +                              struct v4l2_subdev_pad_config *cfg,
> > >>>>>> +                              struct v4l2_subdev_selection *sel)
> > >>>>>> +{
> > >>>>>> +      switch (sel->target) {
> > >>>>>> +      case V4L2_SEL_TGT_CROP_BOUNDS:
> > >>>>>> +      case V4L2_SEL_TGT_NATIVE_SIZE:
> > >>>>>> +              sel->r.left = 0;
> > >>>>>> +              sel->r.top = 0;
> > >>>>>> +              sel->r.width = 2592;
> > >>>>>> +              sel->r.height = 1944;
> > >>>>>
> > >>>>> Why do you need this?
> > >>>>>
> > >>>>
> > >>>> I need to expose the pixel array size and the active pixel area size,
> > >>>> and I interpreted the two above targets as the right place where to do
> > >>>> so.
> > >>>
> > >>> That didn't answer my question :-)
> > >>>
> > >>> Why do you need to expose this? Who uses it for what purpose?
> > >>>
> > >>>>
> > >>>> At least for NATIVE_SIZE the documentation seems to match my
> > >>>> understanding:
> > >>>>
> > >>>> "The native size of the device, e.g. a sensor’s pixel array. left and top
> > >>>> fields are zero for this target."
> > >>>
> > >>> Correct.
> > >>>
> > >>>>
> > >>>>
> > >>>>> Since the format can change for this and the next driver I think CROP_BOUNDS
> > >>>>> at least should match the current format.
> > >>>>>
> > >>>>
> > >>>> Ah, does it? It was not clear to me from the documentation, as it
> > >>>> suggested to me that target actually identifies the active pixel area
> > >>>>
> > >>>> "Bounds of the crop rectangle. All valid crop rectangles fit inside the
> > >>>> crop bounds rectangle."
> > >>>>
> > >>>> It does not mention format, should this be updated?
> > >>>
> > >>> The problem is that for sensors it is indeed not clear.
> > >>>
> > >>> In principle the crop bounds matches the resolution that the sensor uses
> > >>> pre-scaling. So yes, that means that it is equal to the native size.
> > >>>
> > >>> But many sensors have a discrete list of supported formats and it is not
> > >>> clear how they map each format to the actual sensor. If it is clearly just
> > >>> done through binning or averaging, then all is fine.
> > >>
> > >> Sensor drivers do; sensors themselves support much, much more than most
> > >> drivers allow. But this is due to the nature of information available to
> > >> the sensor driver developers, not really something that is trivial to
> > >> change.
> > >>
> > >>>
> > >>> If the formats have different aspect ratios, then it becomes a bit more
> > >>> difficult how to relate the crop bounds with the format since you may not
> > >>> know to which sensor area the current format corresponds.
> > >>>
> > >>> I have no clear answer for you, to be honest, but it can get tricky.
> > >>
> > >> I've suggested earlier that the crop and compose selection targets to be
> > >> used to convey the cropping and binning (or scaling) that is done on the
> > >> sensor, in that order. In reality, there are usually more than one
> > >> (sometimes three) inside a sensor to crop, and often more than one place to
> > >> scale as well. So the driver would need to accommodate this.
> > >>
> > >> The modes come with both cropping and scaling configuration, and V4L2 only
> > >> allows specifying one at a time. In principle an output size may be
> > >> achieved by scaling and cropping by different amounts, and as most drivers
> > >> use only the output format (size) in mode selection, the result could be
> > >> ambiguous. In practice this hasn't been an actual issue.
> > >>
> > >> Better sensor drivers (such as smiapp) do not have this problem as the
> > >> configurations (cropping in three different places as well as binning and
> > >> scaling) can be all independently configured. So with some luck this
> > >> problem could disappear in time with newer hardware and better hardware
> > >> documentation.
> > >>
> > >> I have no objections to providing the cropping and scaling information to
> > >> the user space using the selection rectangles, albeit it's somewhat against
> > >> the semantics currently. This approach would also require using compose
> > >> rectangles on the source pads which is not supported (documentation-wise)
> > >> at the moment, but it's better that way: it can be added now. There are
> > >> other, older, drivers such as omap3isp that configure scaling based on the
> > >> source format configuration only.
> > >
> > > Thanks for all information here, but I think we've gone a bit far
> > > from my original point. The cropping and scaling informations you
> > > mention are, in my understanding, the portion of the pixel array which
> > > is fed to the ISP before scaling, and the result of the ISP
> > > binning/averaging respectively. Those information indeed depends on the
> > > desired capture resolution, the driver implementation, and the sensor
> > > capabilities.
> > >
> > > What I was interested in was just reporting to userspace the physical
> > > size of the active pixel array area, which should reasonably be
> > > defined as a sub-portion of the native pixel array, excluding the back
> > > calibration pixels.
> > >
> > > In one of the previous emails Hans suggested to use CROP_DEFAULT for
> > > that purpose, but in the documentation it is reported not to apply to
> > > subdevice :(
> > >
> > > Do we need a new target, similar to V4L2_SEL_TGT_NATIVE_SIZE that
> > > could maybe report multiple rectangles to accommodate cross-shaped
> > > sensors?
> > >
> > > Are the selection API the right place to report these information?
> > > With a control, it might be easier to report such a static
> > > information...
> >
> > Sorry for the late follow-up.
> >
> > I am uncomfortable with hacking something here. I strongly feel that we
> > are missing a proper API to configure a sensor.
> >
> > If I look at video receivers (SDTV or HDTV), then in both cases you set up
> > the incoming video resolution with S_STD or S_DV_TIMINGS. This explicitly
> > defines the incoming image size and all crop/compose/scale operations
> > operate on that image size.
> >
> > In my opinion we are missing an equivalent to that for sensors. I think the
> > original thinking was that sensors have a fixed pixel array, so there is
> > nothing to configure. But with cross-shaped sensors, binning/skipping,
> > sensors that just hide all the inner details and just report a set of
> > discrete framesizes, this is not actually trivial anymore.
> >
> > And the odd thing is that our API does have discovery (at least to some
> > extent) of the various options through ENUM_FRAMESIZES, but does not let
> > you choose a specific variant. We abuse S_FMT for this, which is really
> > not the right ioctl to use since it defines what you want to receive in
> > memory after cropping/composing/scaling, not the initial image size before
> > all these operations take place.
> >
> > Regarding binning and skipping: I never understood why this wasn't
> > configured using controls. Is there a reason why it was never implemented
> > like that?
> >
> > If we had a Set Source Size operation (either a control, new ioctl or selection
> > target) then you can use that to select a specific source size based on what
> > ENUM_FRAMESIZES reports. Sensor drivers can choose to either enumerate
> > variants based on the cross-shape and binning/skipping, or just enumerate variants
> > based on the cross-shape and add binning/skipping controls to explicitly set
> > this up.
> >
> > In any case, since you now know exactly which image size the sensor produces,
> > you can set up all the selection rectangles associated with that correctly.
> >
> > Since I am not a sensor expert I no doubt missed things, but in my view we
> > really need to see a sensor not as a fixed array, but as something more akin
> > to video receiver, i.e. you need to explicitly configure the video source
> > before cropping/composing/scaling takes place.
> >
> > So in the example of a cross-shaped sensor ENUM_FRAMESIZES would report
> > two sizes (landscape/portrait), binning/cropping controls (optional, this
> > might be implicit in the enumerated framesizes), and after configuring all
> > this using the new API 'Set Source Size' (or whatever it will be called) and
> > possible binning/skipping controls, the user can read various selection
> > targets to get all the needed information.
> >
> > Any cropping/composing/scaling will be done based on the selected image size
> > + binning/skipping.
>
> If I'm not mistaken smiapp sensor driver models this by exposing
> multiple subdevices, one for the pixel array and additional ones for
> the scaler (and the CSI-2 TX port, iirc from my multiplexed stream
> support adventures). Sakari knows better for sure here...
>
> Anyway, going towards a model where sensor expose several subdeves
> would allow to have one for each configurable part of the sensor
> processing pipeline, such as one for the raw pixel array (where one could
> support cross-shaped sensors as you have suggested), one for ISP input
> where to select the area of the pixel array to feed to the ISP, and one
> to model the final processing stage where to to get to the final
> image from, obtained through binning/skipping/averaging whatever and
> possibly by applying a composition rectangle or selecting the
> desired output format, if the sensor supports so.
>
> Am I over-simplifying things here? I fear this is mostly frowned upon by
> the constant lack of decent documentation from sensor manufacturers,
> as most drivers still relies on magic blobs 'guaranteed to work' and
> often produced by some 'certified' tuning applications..
>

I can only add that even though we usually have the right
documentation, there are still specific blessed (certified?) register
combinations that sensor and SoC vendor support and if we use anything
else, we're on our own.

> And yes, more burden on userspace, as right now most sensors are a single
> entity that gives you images in one format/resolution and that's it,
> but this clearly is showing limits. On this side, libcamera could be a
> great help and sensor-specific configurations will be offloaded there.
>
> One quick consideration from my limited experience with sensors: the
> method used to obtain the final image from the portion of the pixel
> array fed to the ISP is not something often freely selectable. From
> the sensor I've seen the manuals of, to obtain a certain resolution
> you use averaging/binning, for another one skipping or direct crop of
> the full pixel array etc.. So I'm not sure a control would fit well here.
>
> Again, I might be really simplifying things :)
>

Modelling various processing bits with a proper topology surely sounds
reasonable. However I think we may need to lay some educational
groundwork first to switch the mindsets from register array-based
pseudo-drivers to proper drivers.

Best regards,
Tomasz

> >
> > Re binning/skipping: one other option is to add a flags field to v4l2_frmsizeenum
> > that reports if the reported size was achieved via binning and/or skipping.
> >
>
> Yes, but in this case that would be a read-only information, not sure
> what you would use it for
>
> Thanks
>   j
>
> > You wouldn't need controls in that case.
> >
> > Regards,
> >
> >       Hans