Skip to contentSkip to conversation

media: add V4L2_CID_UNIT_CELL_SIZE control

PATCH In review Ricardo Ribalda Delgado 4 participants 23 messages Aug 23, 2019 – Sep 12, 2019

Subsystems (4)
lore
Share & tools
lore

Ricardo Ribalda Delgado

author maintainer
#0 lore raw

PATCH v3 1/7 media: add V4L2_CID_UNIT_CELL_SIZE control

This control returns the unit cell size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.

---
v3:
-Put together all actions on ctrl_fill
-Move the control to IMAGE_SOURCE

 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 include/uapi/linux/v4l2-controls.h   |  1 +
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 4 files changed, 25 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +11 -0
index 1d8f38824631..b3bf458df7f7 100644
@@ -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_UNIT_CELL_SIZE:		return "Unit Cell Size";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1375,6 +1376,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
+	case V4L2_CID_UNIT_CELL_SIZE:
+		*type = V4L2_CTRL_TYPE_AREA;
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
@@ -1723,6 +1728,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_FWHT_PARAMS:
 		break;
 
+	case V4L2_CTRL_TYPE_AREA:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
@@ -2421,6 +2429,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
+	case V4L2_CTRL_TYPE_AREA:
+		elem_size = sizeof(struct v4l2_area);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
include/media/v4l2-ctrls.h +2 -0
index 570ff4b0205a..9a3d11350e67 100644
@@ -50,6 +50,7 @@ struct poll_table_struct;
  * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
  * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
  * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
+ * @p_area:			Pointer to an area.
  * @p:				Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_area *p_area;
 	void *p;
 };
 
include/uapi/linux/v4l2-controls.h +1 -0
index a2669b79b294..5a7bedee2b0e 100644
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
 #define V4L2_CID_TEST_PATTERN_BLUE		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
 #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
 
 
 /* Image processing controls */
include/uapi/linux/videodev2.h +11 -0
index 530638dffd93..05cfc69d7ed6 100644
@@ -422,6 +422,11 @@ struct v4l2_fract {
 	__u32   denominator;
 };
 
+struct v4l2_area {
+	__u32   width;
+	__u32   height;
+};
+
 /**
   * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
   *
@@ -1720,6 +1725,12 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	/*
+	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
+	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
+	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
+	 */
+	V4L2_CTRL_TYPE_AREA    = 0x0106,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
Signature · 1 line
2.23.0.rc1
author maintainer
#1 lore raw

PATCH v3 2/7 Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE

New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection.

---
 Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +8 -0
index 2c3ab5796d76..6e728accf67b 100644
@@ -55,3 +55,11 @@ Image Source Control IDs
 
 ``V4L2_CID_TEST_PATTERN_GREENB (integer)``
     Test pattern green (next to blue) colour component.
+
+``V4L2_CID_UNIT_CELL_SIZE (struct)``
+    This control returns the unit cell size in nanometres. The struct provides
+    the width and the height in separated fields to take into consideration
+    asymmetric pixels and/or hardware binning.
+    The unit cell consists of the whole area of the pixel, sensitive and
+    non-sensitive.
+    This control is required for automatic calibration sensors/cameras.
Signature · 1 line
2.23.0.rc1
#2 lore raw

Re: [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE

Quoted 14 lines · Ricardo Ribalda Delgado

New control to pass to userspace the width/height of a pixel. Which is needed for calibration and lens selection.

---
 Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 8 ++++++++
 1 file changed, 8 insertions(+)
Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +0 -0
index 2c3ab5796d76..6e728accf67b 100644
@@ -55,3 +55,11 @@ Image Source Control IDs
 

Documentation/media/uapi/v4l/ext-ctrls-image-source.rst Image Source Control IDs · line 65

Quoted 10 lines
Documentation/media/uapi/v4l/ext-ctrls-image-source.rst +8 -0
 ``V4L2_CID_TEST_PATTERN_GREENB (integer)``
     Test pattern green (next to blue) colour component.
+
+``V4L2_CID_UNIT_CELL_SIZE (struct)``
+    This control returns the unit cell size in nanometres. The struct provides
+    the width and the height in separated fields to take into consideration
+    asymmetric pixels and/or hardware binning.
+    The unit cell consists of the whole area of the pixel, sensitive and
+    non-sensitive.
+    This control is required for automatic calibration sensors/cameras.

Can we have a link from here to the struct v4l2_area documentation?

regards
Philipp

author maintainer
#3 lore raw

PATCH v3 3/7 Documentation: media: Document V4L2_CTRL_TYPE_AREA

In reply to Ricardo Ribalda Delgado

A struct v4l2_area containing the width and the height of a rectangular area.

---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +6 -0
index a3d56ffbf4cc..c09d06ef2b08 100644
@@ -443,6 +443,12 @@ See also the examples in :ref:`control`.
       - n/a
       - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
 	quantization matrices for stateless video decoders.
+    * - ``V4L2_CTRL_TYPE_AREA``
+      - n/a
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_area`, containing the width and the height
+        of a rectangular area.
     * - ``V4L2_CTRL_TYPE_H264_SPS``
       - n/a
       - n/a
Signature · 1 line
2.23.0.rc1
#4 lore raw

Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

Quoted 14 lines · Ricardo Ribalda Delgado

A struct v4l2_area containing the width and the height of a rectangular area.

---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +0 -0
index a3d56ffbf4cc..c09d06ef2b08 100644

Documentation/media/uapi/v4l/vidioc-queryctrl.rst See also the examples in :ref:`control`. · line 451

Quoted 9 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +6 -0
@@ -443,6 +443,12 @@ See also the examples in :ref:`control`.
       - n/a
       - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
 	quantization matrices for stateless video decoders.
+    * - ``V4L2_CTRL_TYPE_AREA``
+      - n/a
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_area`, containing the width and the height
+        of a rectangular area.

Maybe explicitly mention that units depend on the use case?

regards
Philipp

#5 lore raw

Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

In reply to Ricardo Ribalda Delgado

Hi Ricardo,

Quoted 14 lines · Ricardo Ribalda Delgado

A struct v4l2_area containing the width and the height of a rectangular area.

---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
 1 file changed, 6 insertions(+)
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +0 -0
index a3d56ffbf4cc..c09d06ef2b08 100644

Documentation/media/uapi/v4l/vidioc-queryctrl.rst See also the examples in :ref:`control`. · line 447

Quoted 5 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +2 -0
@@ -443,6 +443,12 @@ See also the examples in :ref:`control`.
       - n/a
       - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
 	quantization matrices for stateless video decoders.
+    * - ``V4L2_CTRL_TYPE_AREA``
+      - n/a

Can an area be negative ?
I would set these fields to ">= 0" ">= 1" and ">= 0" respectively.

Thanks
j

Quoted 10 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +4 -1
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_area`, containing the width and the height
+        of a rectangular area.
     * - ``V4L2_CTRL_TYPE_H264_SPS``
       - n/a
       - n/a
--

2.23.0.rc1

author maintainer
#6 lore raw

Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

HI Jacopo

(Sorry for the late reply, I have been in holidays plus with plenty of family matters)

Quoted 13 lines · Jacopo Mondi

Hi Ricardo,

Quoted 9 lines · Ricardo Ribalda Delgado

A struct v4l2_area containing the width and the height of a rectangular area.

---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
 1 file changed, 6 insertions(+)
Quoted 14 lines

Documentation/media/uapi/v4l/vidioc-queryctrl.rst See also the examples in :ref:`control`. · line 447

Quoted 10 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +2 -0
index a3d56ffbf4cc..c09d06ef2b08 100644
@@ -443,6 +443,12 @@ See also the examples in :ref:`control`.
       - n/a
       - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
      quantization matrices for stateless video decoders.
+    * - ``V4L2_CTRL_TYPE_AREA``
+      - n/a

Can an area be negative ?
I would set these fields to ">= 0" ">= 1" and ">= 0" respectively.

Dont min, max and step only make sense for integer controls?

Quoted 13 lines

Thanks
j

Quoted 10 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +4 -1
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_area`, containing the width and the height
+        of a rectangular area.
     * - ``V4L2_CTRL_TYPE_H264_SPS``
       - n/a
       - n/a
--

2.23.0.rc1

#7 lore raw

Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

Quoted 34 lines · Ricardo Ribalda Delgado

HI Jacopo

(Sorry for the late reply, I have been in holidays plus with plenty of family matters)

Quoted 13 lines · Jacopo Mondi

Hi Ricardo,

Quoted 9 lines · Ricardo Ribalda Delgado

A struct v4l2_area containing the width and the height of a rectangular area.

---
 Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
 1 file changed, 6 insertions(+)
Quoted 14 lines

Documentation/media/uapi/v4l/vidioc-queryctrl.rst See also the examples in :ref:`control`. · line 447

Quoted 10 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +2 -0
index a3d56ffbf4cc..c09d06ef2b08 100644
@@ -443,6 +443,12 @@ See also the examples in :ref:`control`.
       - n/a
       - A struct :c:type:`v4l2_ctrl_mpeg2_quantization`, containing MPEG-2
      quantization matrices for stateless video decoders.
+    * - ``V4L2_CTRL_TYPE_AREA``
+      - n/a

Can an area be negative ?
I would set these fields to ">= 0" ">= 1" and ">= 0" respectively.

Quoted 1 line

Dont min, max and step only make sense for integer controls?

It's up to us to define this :-)

I think it would make sense here as well where the range applies to both width and height values. Obviously, the range will be a superset of the range of each field separately, but it does help validation and prevent negative values.

Regards,

Hans

Quoted 14 lines
Quoted 14 lines

Thanks
j

Quoted 10 lines
Documentation/media/uapi/v4l/vidioc-queryctrl.rst +4 -1
+      - n/a
+      - n/a
+      - A struct :c:type:`v4l2_area`, containing the width and the height
+        of a rectangular area.
     * - ``V4L2_CTRL_TYPE_H264_SPS``
       - n/a
       - n/a
--

2.23.0.rc1

author maintainer
#8 lore raw

PATCH v3 4/7 media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE

In reply to Ricardo Ribalda Delgado

According to the product brief, the unit cell size is 1120 nanometers^2.

https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf

---
 drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
 1 file changed, 23 insertions(+)
drivers/media/i2c/imx214.c +23 -0
index 159a3a604f0e..cc0a013ba7da 100644
@@ -47,6 +47,7 @@ struct imx214 {
 	struct v4l2_ctrl *pixel_rate;
 	struct v4l2_ctrl *link_freq;
 	struct v4l2_ctrl *exposure;
+	struct v4l2_ctrl *unit_size;
 
 	struct regulator_bulk_data	supplies[IMX214_NUM_SUPPLIES];
 
@@ -941,6 +942,26 @@ static int __maybe_unused imx214_resume(struct device *dev)
 	return ret;
 }
 
+static void unit_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
+		     union v4l2_ctrl_ptr ptr)
+{
+	ptr.p_area->width = 1120;
+	ptr.p_area->height = 1120;
+}
+
+static const struct v4l2_ctrl_type_ops unit_size_ops = {
+	.init = unit_size_init,
+};
+
+static struct v4l2_ctrl *new_unit_size_ctrl(struct v4l2_ctrl_handler *handler)
+{
+	static struct v4l2_ctrl_config ctrl = {
+		.id = V4L2_CID_UNIT_CELL_SIZE,
+		.type_ops = &unit_size_ops,
+	};
+
+	return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
+}
 static int imx214_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -1029,6 +1050,8 @@ static int imx214_probe(struct i2c_client *client)
 					     V4L2_CID_EXPOSURE,
 					     0, 3184, 1, 0x0c70);
 
+	imx214->unit_size = new_unit_size_ctrl(&imx214->ctrls);
+
 	ret = imx214->ctrls.error;
 	if (ret) {
 		dev_err(&client->dev, "%s control init failed (%d)\n",
Signature · 1 line
2.23.0.rc1
#9 lore raw

Re: [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE

Quoted 5 lines · Ricardo Ribalda Delgado

According to the product brief, the unit cell size is 1120 nanometers^2.

https://www.sony-semicon.co.jp/products_en/IS/sensor1/img/products/ProductBrief_IMX214_20150428.pdf

If the v4l2_ctrl_new_area helper is accepted, I'd reorder this afterwards and squash it together with patch 7.

regards
Philipp

author maintainer
#10 lore raw

PATCH v3 5/7 media: v4l2-core: Add new helper for area controls

In reply to Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +24 -1
index b3bf458df7f7..33e48f0aec1a 100644
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+		union v4l2_ctrl_ptr ptr)
+{
+	memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+	.init = area_init,
+};
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+				     const struct v4l2_ctrl_ops *ops,
+				     u32 id, const struct v4l2_area *area)
+{
+	static struct v4l2_ctrl_config ctrl = {
+		.id = V4L2_CID_UNIT_CELL_SIZE,
+		.type_ops = &area_ops,
+	};
+
+	return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
include/media/v4l2-ctrls.h +16 -0
index 9a3d11350e67..36f0712ea6dd 100644
@@ -669,6 +669,22 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 					 u32 id, u8 max, u8 def,
 					 const s64 *qmenu_int);
 
+/**
+ * v4l2_ctrl_new_area() - Allocate and initialize a V4L2_CTRL_TYPE_AREA control.
+ *
+ * @hdl:	The control handler.
+ * @ops:	The control ops.
+ * @id:	The control ID.
+ * @area:	The width and height of the area in a struct v4l2_area.
+ *
+ * If the &v4l2_ctrl struct could not be allocated then NULL is returned
+ * and @hdl->error is set to the error code (if it wasn't set already).
+ */
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+				     const struct v4l2_ctrl_ops *ops,
+				     u32 id, const struct v4l2_area *area);
+
 /**
  * typedef v4l2_ctrl_filter - Typedef to define the filter function to be
  *	used when adding a control handler.
Signature · 1 line
2.23.0.rc1
#11 lore raw

Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls

Quoted 15 lines · Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
index b3bf458df7f7..33e48f0aec1a 100644

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, · line 2663

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -1
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */

Why move this ...

Quoted 21 lines
drivers/media/v4l2-core/v4l2-ctrls.c +14 -0
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+		union v4l2_ctrl_ptr ptr)
+{
+	memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+	.init = area_init,
+};
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+				     const struct v4l2_ctrl_ops *ops,
+				     u32 id, const struct v4l2_area *area)
+{

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2709

Quoted 10 lines
drivers/media/v4l2-core/v4l2-ctrls.c +10 -0
+	static struct v4l2_ctrl_config ctrl = {
+		.id = V4L2_CID_UNIT_CELL_SIZE,
+		.type_ops = &area_ops,
+	};
+
+	return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */

... here?

Looks to me like this comment should stay attached to v4l2_ctrl_new_int_menu.

regards
Philipp

author maintainer
#12 lore raw

Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls

Quoted 49 lines · Philipp Zabel
Quoted 15 lines · Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
index b3bf458df7f7..33e48f0aec1a 100644

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, · line 2663

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -1
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */

Why move this ...

Quoted 25 lines
drivers/media/v4l2-core/v4l2-ctrls.c +18 -0
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
                      const struct v4l2_ctrl_ops *ops,
                      u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+             union v4l2_ctrl_ptr ptr)
+{
+     memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+     .init = area_init,
+};
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+                                  const struct v4l2_ctrl_ops *ops,
+                                  u32 id, const struct v4l2_area *area)
+{
+     static struct v4l2_ctrl_config ctrl = {
+             .id = V4L2_CID_UNIT_CELL_SIZE,
+             .type_ops = &area_ops,
+     };
Quoted 8 lines

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2709

Quoted 6 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6 -0
+
+     return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */

... here?

Because I screwed up :). Let me fix that sorry.

I will push all your changes to:

https://github.com/ribalda/linux/tree/unit-size-v4

plus any other comment and then I will wait 2-3 days for resend

Quoted 6 lines

Looks to me like this comment should stay attached to v4l2_ctrl_new_int_menu.

regards
Philipp

Reviewed-by
#13 lore raw

Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls

Quoted 59 lines · Ricardo Ribalda Delgado
Quoted 49 lines · Philipp Zabel
Quoted 15 lines · Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
index b3bf458df7f7..33e48f0aec1a 100644

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, · line 2663

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -1
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */

Why move this ...

Quoted 25 lines
drivers/media/v4l2-core/v4l2-ctrls.c +18 -0
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
                      const struct v4l2_ctrl_ops *ops,
                      u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+             union v4l2_ctrl_ptr ptr)
+{
+     memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+     .init = area_init,
+};
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+                                  const struct v4l2_ctrl_ops *ops,
+                                  u32 id, const struct v4l2_area *area)
+{
+     static struct v4l2_ctrl_config ctrl = {
+             .id = V4L2_CID_UNIT_CELL_SIZE,
+             .type_ops = &area_ops,
+     };
Quoted 8 lines

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2709

Quoted 6 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6 -0
+
+     return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */

... here?

Quoted 7 lines

Because I screwed up :). Let me fix that sorry.

I will push all your changes to:

https://github.com/ribalda/linux/tree/unit-size-v4

plus any other comment and then I will wait 2-3 days for resend

Awesome, thanks! Feel free to add

regards
Philipp

Reviewed-by
#14 lore raw

Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls

Hi Ricardo,

Quoted 64 lines · Philipp Zabel
Quoted 63 lines · Ricardo Ribalda Delgado
Quoted 49 lines · Philipp Zabel
Quoted 15 lines · Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
index b3bf458df7f7..33e48f0aec1a 100644

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, · line 2663

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -1
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */

Why move this ...

Quoted 25 lines
drivers/media/v4l2-core/v4l2-ctrls.c +18 -0
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
                      const struct v4l2_ctrl_ops *ops,
                      u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+             union v4l2_ctrl_ptr ptr)
+{
+     memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+     .init = area_init,
+};
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+                                  const struct v4l2_ctrl_ops *ops,
+                                  u32 id, const struct v4l2_area *area)
+{
+     static struct v4l2_ctrl_config ctrl = {
+             .id = V4L2_CID_UNIT_CELL_SIZE,
+             .type_ops = &area_ops,
+     };
Quoted 8 lines

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2709

Quoted 6 lines
drivers/media/v4l2-core/v4l2-ctrls.c +6 -0
+
+     return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */

... here?

Because I screwed up :). Let me fix that sorry.

I will push all your changes to:

Quoted 7 lines
Quoted 3 lines

https://github.com/ribalda/linux/tree/unit-size-v4

plus any other comment and then I will wait 2-3 days for resend

Awesome, thanks! Feel free to add

With the issue pointed out by Philipp addressed

Thanks
j

Quoted 3 lines

regards
Philipp

#15 lore raw

Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls

In reply to Ricardo Ribalda Delgado
Quoted 15 lines · Ricardo Ribalda Delgado

Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate, try to minimize it by adding a new helper.

---
 drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
 include/media/v4l2-ctrls.h           | 16 ++++++++++++++++
 2 files changed, 40 insertions(+), 1 deletion(-)
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
index b3bf458df7f7..33e48f0aec1a 100644

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl, · line 2663

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -1
@@ -2660,7 +2660,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
 
-/* Helper function for standard integer menu controls */

Huh?

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2689

Quoted 11 lines
drivers/media/v4l2-core/v4l2-ctrls.c +4 -0
 struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, u8 _max, u8 _def, const s64 *qmenu_int)
@@ -2684,6 +2683,30 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
 
+static void area_init(const struct v4l2_ctrl *ctrl, u32 idx,
+		union v4l2_ctrl_ptr ptr)
+{
+	memcpy(ptr.p_area, ctrl->priv, sizeof(*ptr.p_area));

This can be used in an array, so you have to honor the idx field.

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2694

Quoted 5 lines
drivers/media/v4l2-core/v4l2-ctrls.c +5 -0
+}
+
+static const struct v4l2_ctrl_type_ops area_ops = {
+	.init = area_init,
+};

This is a standard control type, so just add support for it the std_type_ops functions.

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2701

Quoted 7 lines
drivers/media/v4l2-core/v4l2-ctrls.c +7 -0
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+				     const struct v4l2_ctrl_ops *ops,
+				     u32 id, const struct v4l2_area *area)
+{
+	static struct v4l2_ctrl_config ctrl = {
+		.id = V4L2_CID_UNIT_CELL_SIZE,

This should be set to the passed id, not hardcoded.

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2702

Quoted 1 line
drivers/media/v4l2-core/v4l2-ctrls.c +1 -0
+		.type_ops = &area_ops,

And just drop this line.

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2705

Quoted 3 lines
drivers/media/v4l2-core/v4l2-ctrls.c +3 -0
+	};
+
+	return v4l2_ctrl_new_custom(hdl, &ctrl, (void *)area);

Don't pass area as a priv pointer. Instead the core will just initialize the area to some default value (1x1), and then you can call set_ctrl() after creating the control to set it to the proper value.

The READ_ONLY flag applies to the public API, but the kernel API still has to be able to change it.

drivers/media/v4l2-core/v4l2-ctrls.c struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 2709

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +4 -0
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_area);
+
+/* Helper function for standard integer menu controls */

This comment doesn't belong here.

Quoted 8 lines
drivers/media/v4l2-core/v4l2-ctrls.c +0 -0
 /* Add the controls from another handler to our own. */
 int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
 			  struct v4l2_ctrl_handler *add,
include/media/v4l2-ctrls.h +0 -0
index 9a3d11350e67..36f0712ea6dd 100644

include/media/v4l2-ctrls.h struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 678

Quoted 10 lines
include/media/v4l2-ctrls.h +7 -0
@@ -669,6 +669,22 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 					 u32 id, u8 max, u8 def,
 					 const s64 *qmenu_int);
 
+/**
+ * v4l2_ctrl_new_area() - Allocate and initialize a V4L2_CTRL_TYPE_AREA control.
+ *
+ * @hdl:	The control handler.
+ * @ops:	The control ops.
+ * @id:	The control ID.
+ * @area:	The width and height of the area in a struct v4l2_area.

Specifically, this is the initial width and height.

include/media/v4l2-ctrls.h struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl, · line 690

Quoted 13 lines
include/media/v4l2-ctrls.h +9 -0
+ *
+ * If the &v4l2_ctrl struct could not be allocated then NULL is returned
+ * and @hdl->error is set to the error code (if it wasn't set already).
+ */
+
+struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+				     const struct v4l2_ctrl_ops *ops,
+				     u32 id, const struct v4l2_area *area);
+
 /**
  * typedef v4l2_ctrl_filter - Typedef to define the filter function to be
  *	used when adding a control handler.

Regards,

Hans

author maintainer
#16 lore raw

PATCH v3 6/7 Documentation: Document v4l2_ctrl_new area

In reply to Ricardo Ribalda Delgado

Helper for creating area controls.

---
 Documentation/media/kapi/v4l2-controls.rst | 9 +++++++++
 1 file changed, 9 insertions(+)
Documentation/media/kapi/v4l2-controls.rst +9 -0
index ebe2a55908be..656e9428f6a6 100644
@@ -149,6 +149,15 @@ Integer menu controls with a driver specific menu can be added by calling
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 max, s32 def, const s64 *qmenu_int);
 
+Area controls can be added by calling
+:c:func:`v4l2_ctrl_new_area`:
+
+.. code-block:: c
+
+	struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+			const struct v4l2_ctrl_ops *ops,
+			u32 id, const struct v4l2_area *area);
+
 These functions are typically called right after the
 :c:func:`v4l2_ctrl_handler_init`:
 
Signature · 1 line
2.23.0.rc1
Reviewed-by on v3 6/7
#17 lore raw

Re: [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area

Hi Ricardo,

Quoted 3 lines · Ricardo Ribalda Delgado

Helper for creating area controls.

With this squashed on 5/7 or separated if we want documentation changes to get in separately

Thanks
j

Quoted 27 lines
---
 Documentation/media/kapi/v4l2-controls.rst | 9 +++++++++
 1 file changed, 9 insertions(+)
Documentation/media/kapi/v4l2-controls.rst +9 -1
index ebe2a55908be..656e9428f6a6 100644
@@ -149,6 +149,15 @@ Integer menu controls with a driver specific menu can be added by calling
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 max, s32 def, const s64 *qmenu_int);
 
+Area controls can be added by calling
+:c:func:`v4l2_ctrl_new_area`:
+
+.. code-block:: c
+
+	struct v4l2_ctrl *v4l2_ctrl_new_area(struct v4l2_ctrl_handler *hdl,
+			const struct v4l2_ctrl_ops *ops,
+			u32 id, const struct v4l2_area *area);
+
 These functions are typically called right after the
 :c:func:`v4l2_ctrl_handler_init`:
 
--

2.23.0.rc1

author maintainer
#18 lore raw

PATCH v3 7/7 imx214: Use v4l2_ctrl_new_area helper

In reply to Ricardo Ribalda Delgado

Instead of creating manually the V4L2_CID_UNIT_CELL_SIZE control, lets use the helper.

---
 drivers/media/i2c/imx214.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)
drivers/media/i2c/imx214.c +8 -21
index cc0a013ba7da..625617d4c81a 100644
@@ -942,26 +942,6 @@ static int __maybe_unused imx214_resume(struct device *dev)
 	return ret;
 }
 
-static void unit_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
-		     union v4l2_ctrl_ptr ptr)
-{
-	ptr.p_area->width = 1120;
-	ptr.p_area->height = 1120;
-}
-
-static const struct v4l2_ctrl_type_ops unit_size_ops = {
-	.init = unit_size_init,
-};
-
-static struct v4l2_ctrl *new_unit_size_ctrl(struct v4l2_ctrl_handler *handler)
-{
-	static struct v4l2_ctrl_config ctrl = {
-		.id = V4L2_CID_UNIT_CELL_SIZE,
-		.type_ops = &unit_size_ops,
-	};
-
-	return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
-}
 static int imx214_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -969,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
 	static const s64 link_freq[] = {
 		IMX214_DEFAULT_LINK_FREQ,
 	};
+	struct v4l2_area unit_size = {
+		.width = 1120,
+		.height = 1120,
+	};
 	int ret;
 
 	ret = imx214_parse_fwnode(dev);
@@ -1050,7 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
 					     V4L2_CID_EXPOSURE,
 					     0, 3184, 1, 0x0c70);
 
-	imx214->unit_size = new_unit_size_ctrl(&imx214->ctrls);
+	imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
+					       &imx214_ctrl_ops,
+					       V4L2_CID_UNIT_CELL_SIZE,
+					       &unit_size);
 
 	ret = imx214->ctrls.error;
 	if (ret) {
Signature · 1 line
2.23.0.rc1
#19 lore raw

Re: [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper

Hi Ricardo,

Quoted 3 lines · Ricardo Ribalda Delgado

Instead of creating manually the V4L2_CID_UNIT_CELL_SIZE control, lets use the helper.

I think you should drop 4/7 and use directly the new helper here.

Thanks
j

Quoted 62 lines
---
 drivers/media/i2c/imx214.c | 29 ++++++++---------------------
 1 file changed, 8 insertions(+), 21 deletions(-)
drivers/media/i2c/imx214.c +8 -22
index cc0a013ba7da..625617d4c81a 100644
@@ -942,26 +942,6 @@ static int __maybe_unused imx214_resume(struct device *dev)
 	return ret;
 }
 
-static void unit_size_init(const struct v4l2_ctrl *ctrl, u32 idx,
-		     union v4l2_ctrl_ptr ptr)
-{
-	ptr.p_area->width = 1120;
-	ptr.p_area->height = 1120;
-}
-
-static const struct v4l2_ctrl_type_ops unit_size_ops = {
-	.init = unit_size_init,
-};
-
-static struct v4l2_ctrl *new_unit_size_ctrl(struct v4l2_ctrl_handler *handler)
-{
-	static struct v4l2_ctrl_config ctrl = {
-		.id = V4L2_CID_UNIT_CELL_SIZE,
-		.type_ops = &unit_size_ops,
-	};
-
-	return v4l2_ctrl_new_custom(handler, &ctrl, NULL);
-}
 static int imx214_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -969,6 +949,10 @@ static int imx214_probe(struct i2c_client *client)
 	static const s64 link_freq[] = {
 		IMX214_DEFAULT_LINK_FREQ,
 	};
+	struct v4l2_area unit_size = {
+		.width = 1120,
+		.height = 1120,
+	};
 	int ret;
 
 	ret = imx214_parse_fwnode(dev);
@@ -1050,7 +1034,10 @@ static int imx214_probe(struct i2c_client *client)
 					     V4L2_CID_EXPOSURE,
 					     0, 3184, 1, 0x0c70);
 
-	imx214->unit_size = new_unit_size_ctrl(&imx214->ctrls);
+	imx214->unit_size = v4l2_ctrl_new_area(&imx214->ctrls,
+					       &imx214_ctrl_ops,
+					       V4L2_CID_UNIT_CELL_SIZE,
+					       &unit_size);
 
 	ret = imx214->ctrls.error;
 	if (ret) {
--

2.23.0.rc1

Reviewed-by on v3 1/7
#20 lore raw
In reply to Ricardo Ribalda Delgado

Hi Ricardo,

Quoted 109 lines · Ricardo Ribalda Delgado

This control returns the unit cell size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.

---
v3:

(quoted diff) +0 -2
-Put together all actions on ctrl_fill
-Move the control to IMAGE_SOURCE
 
 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 include/uapi/linux/v4l2-controls.h   |  1 +
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 4 files changed, 25 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +11 -0
index 1d8f38824631..b3bf458df7f7 100644
@@ -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_UNIT_CELL_SIZE:		return "Unit Cell Size";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1375,6 +1376,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
+	case V4L2_CID_UNIT_CELL_SIZE:
+		*type = V4L2_CTRL_TYPE_AREA;
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
@@ -1723,6 +1728,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_FWHT_PARAMS:
 		break;
 
+	case V4L2_CTRL_TYPE_AREA:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
@@ -2421,6 +2429,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
+	case V4L2_CTRL_TYPE_AREA:
+		elem_size = sizeof(struct v4l2_area);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
include/media/v4l2-ctrls.h +2 -0
index 570ff4b0205a..9a3d11350e67 100644
@@ -50,6 +50,7 @@ struct poll_table_struct;
  * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
  * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
  * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
+ * @p_area:			Pointer to an area.
  * @p:				Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_area *p_area;
 	void *p;
 };
 
include/uapi/linux/v4l2-controls.h +1 -0
index a2669b79b294..5a7bedee2b0e 100644
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
 #define V4L2_CID_TEST_PATTERN_BLUE		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
 #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
 
 
 /* Image processing controls */
include/uapi/linux/videodev2.h +5 -0
index 530638dffd93..05cfc69d7ed6 100644
@@ -422,6 +422,11 @@ struct v4l2_fract {
 	__u32   denominator;
 };
 
+struct v4l2_area {
+	__u32   width;
+	__u32   height;
+};
+
 /**
   * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
   *

include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1732

Quoted 8 lines
include/uapi/linux/videodev2.h +5 -0
@@ -1720,6 +1725,12 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	/*
+	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
+	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
+	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
+	 */

With a confirmation from Hans and others this comments fits here

Thanks
j

Quoted 7 lines
include/uapi/linux/videodev2.h +1 -1
+	V4L2_CTRL_TYPE_AREA    = 0x0106,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
--

2.23.0.rc1

#21 lore raw
In reply to Ricardo Ribalda Delgado

Hi Ricardo,

Quoted 65 lines · Ricardo Ribalda Delgado

This control returns the unit cell size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.

---
v3:

(quoted diff) +0 -2
-Put together all actions on ctrl_fill
-Move the control to IMAGE_SOURCE
 
 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 include/uapi/linux/v4l2-controls.h   |  1 +
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 4 files changed, 25 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +11 -0
index 1d8f38824631..b3bf458df7f7 100644
@@ -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_UNIT_CELL_SIZE:		return "Unit Cell Size";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1375,6 +1376,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
+	case V4L2_CID_UNIT_CELL_SIZE:
+		*type = V4L2_CTRL_TYPE_AREA;
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;
@@ -1723,6 +1728,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_FWHT_PARAMS:
 		break;
 
+	case V4L2_CTRL_TYPE_AREA:
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
@@ -2421,6 +2429,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
+	case V4L2_CTRL_TYPE_AREA:
+		elem_size = sizeof(struct v4l2_area);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
include/media/v4l2-ctrls.h +0 -0
index 570ff4b0205a..9a3d11350e67 100644

include/media/v4l2-ctrls.h struct poll_table_struct; · line 53

Quoted 4 lines
include/media/v4l2-ctrls.h +1 -0
@@ -50,6 +50,7 @@ struct poll_table_struct;
  * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
  * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
  * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
+ * @p_area:			Pointer to an area.

Actually:
"Pointer to a struct v4l2_area" ?

Also,, I'm not sure which patch introduces this but I see a new
warning when building documentation:
.../Documentation/output/videodev2.h.rst:6: WARNING: undefined label: v4l2-ctrl-type-area (if the link has no caption the label must precede a section header)

Thanks
j

Quoted 55 lines
include/media/v4l2-ctrls.h +1 -0
  * @p:				Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_area *p_area;
 	void *p;
 };
 
include/uapi/linux/v4l2-controls.h +1 -0
index a2669b79b294..5a7bedee2b0e 100644
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
 #define V4L2_CID_TEST_PATTERN_BLUE		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
 #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
 
 
 /* Image processing controls */
include/uapi/linux/videodev2.h +11 -1
index 530638dffd93..05cfc69d7ed6 100644
@@ -422,6 +422,11 @@ struct v4l2_fract {
 	__u32   denominator;
 };
 
+struct v4l2_area {
+	__u32   width;
+	__u32   height;
+};
+
 /**
   * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
   *
@@ -1720,6 +1725,12 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	/*
+	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
+	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
+	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
+	 */
+	V4L2_CTRL_TYPE_AREA    = 0x0106,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
--

2.23.0.rc1

#22 lore raw
In reply to Ricardo Ribalda Delgado
Quoted 41 lines · Ricardo Ribalda Delgado

This control returns the unit cell size in nanometres. The struct provides the width and the height in separated fields to take into consideration asymmetric pixels and/or hardware binning.
This control is required for automatic calibration of sensors/cameras.

---
v3:

(quoted diff) +0 -2
-Put together all actions on ctrl_fill
-Move the control to IMAGE_SOURCE
 
 drivers/media/v4l2-core/v4l2-ctrls.c | 11 +++++++++++
 include/media/v4l2-ctrls.h           |  2 ++
 include/uapi/linux/v4l2-controls.h   |  1 +
 include/uapi/linux/videodev2.h       | 11 +++++++++++
 4 files changed, 25 insertions(+)
drivers/media/v4l2-core/v4l2-ctrls.c +5 -0
index 1d8f38824631..b3bf458df7f7 100644
@@ -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_UNIT_CELL_SIZE:		return "Unit Cell Size";
 
 	/* FM Radio Modulator controls */
 	/* Keep the order of the 'case's the same as in v4l2-controls.h! */
@@ -1375,6 +1376,10 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_MPEG_VIDEO_VP8_FRAME_HEADER:
 		*type = V4L2_CTRL_TYPE_VP8_FRAME_HEADER;
 		break;
+	case V4L2_CID_UNIT_CELL_SIZE:
+		*type = V4L2_CTRL_TYPE_AREA;
+		*flags |= V4L2_CTRL_FLAG_READ_ONLY;
+		break;
 	default:
 		*type = V4L2_CTRL_TYPE_INTEGER;
 		break;

drivers/media/v4l2-core/v4l2-ctrls.c static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx, · line 1731

Quoted 4 lines
drivers/media/v4l2-core/v4l2-ctrls.c +1 -0
@@ -1723,6 +1728,9 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_FWHT_PARAMS:
 		break;
 
+	case V4L2_CTRL_TYPE_AREA:

You need to check that the width and height are not 0 (that would make no sense).

Quoted 64 lines
drivers/media/v4l2-core/v4l2-ctrls.c +5 -0
+		break;
+
 	case V4L2_CTRL_TYPE_H264_SPS:
 	case V4L2_CTRL_TYPE_H264_PPS:
 	case V4L2_CTRL_TYPE_H264_SCALING_MATRIX:
@@ -2421,6 +2429,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	case V4L2_CTRL_TYPE_VP8_FRAME_HEADER:
 		elem_size = sizeof(struct v4l2_ctrl_vp8_frame_header);
 		break;
+	case V4L2_CTRL_TYPE_AREA:
+		elem_size = sizeof(struct v4l2_area);
+		break;
 	default:
 		if (type < V4L2_CTRL_COMPOUND_TYPES)
 			elem_size = sizeof(s32);
include/media/v4l2-ctrls.h +2 -0
index 570ff4b0205a..9a3d11350e67 100644
@@ -50,6 +50,7 @@ struct poll_table_struct;
  * @p_h264_slice_params:	Pointer to a struct v4l2_ctrl_h264_slice_params.
  * @p_h264_decode_params:	Pointer to a struct v4l2_ctrl_h264_decode_params.
  * @p_vp8_frame_header:		Pointer to a VP8 frame header structure.
+ * @p_area:			Pointer to an area.
  * @p:				Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
@@ -68,6 +69,7 @@ union v4l2_ctrl_ptr {
 	struct v4l2_ctrl_h264_slice_params *p_h264_slice_params;
 	struct v4l2_ctrl_h264_decode_params *p_h264_decode_params;
 	struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
+	struct v4l2_area *p_area;
 	void *p;
 };
 
include/uapi/linux/v4l2-controls.h +1 -0
index a2669b79b294..5a7bedee2b0e 100644
@@ -1034,6 +1034,7 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_TEST_PATTERN_GREENR		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 5)
 #define V4L2_CID_TEST_PATTERN_BLUE		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 6)
 #define V4L2_CID_TEST_PATTERN_GREENB		(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 7)
+#define V4L2_CID_UNIT_CELL_SIZE			(V4L2_CID_IMAGE_SOURCE_CLASS_BASE + 8)
 
 
 /* Image processing controls */
include/uapi/linux/videodev2.h +5 -0
index 530638dffd93..05cfc69d7ed6 100644
@@ -422,6 +422,11 @@ struct v4l2_fract {
 	__u32   denominator;
 };
 
+struct v4l2_area {
+	__u32   width;
+	__u32   height;
+};
+
 /**
   * struct v4l2_capability - Describes V4L2 device caps returned by VIDIOC_QUERYCAP
   *

include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1732

Quoted 8 lines
include/uapi/linux/videodev2.h +5 -0
@@ -1720,6 +1725,12 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	/*
+	 * V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS = 0x0103,
+	 * V4L2_CTRL_TYPE_MPEG2_QUANTIZATION = 0x0104,
+	 * V4L2_CTRL_TYPE_FWHT_PARAMS = 0x0105,
+	 */

I'd drop this comment. Not because it is wrong as such, but it is incomplete (there are more of these types today) and I think these types (most of them in staging) need a bit of a cleanup. Something I want to look at for the next kernel cycle.

include/uapi/linux/videodev2.h enum v4l2_ctrl_type { · line 1736

Quoted 5 lines
include/uapi/linux/videodev2.h +1 -0
+	V4L2_CTRL_TYPE_AREA    = 0x0106,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */

Regards,

Hans