* [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control
@ 2019-08-23 12:37 Ricardo Ribalda Delgado
2019-08-23 12:37 ` [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
` (8 more replies)
0 siblings, 9 replies; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: 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.
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
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(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..b3bf458df7f7 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_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);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 570ff4b0205a..9a3d11350e67 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -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;
};
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index a2669b79b294..5a7bedee2b0e 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -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 */
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..05cfc69d7ed6 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-23 12:37 ` [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA Ricardo Ribalda Delgado
` (7 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: Ricardo Ribalda Delgado
New control to pass to userspace the width/height of a pixel. Which is
needed for calibration and lens selection.
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 8 ++++++++
1 file changed, 8 insertions(+)
diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
index 2c3ab5796d76..6e728accf67b 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
@@ -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.
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
2019-08-23 12:37 ` [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-26 7:40 ` Jacopo Mondi
2019-08-23 12:37 ` [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
` (6 subsequent siblings)
8 siblings, 2 replies; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: Ricardo Ribalda Delgado
A struct v4l2_area containing the width and the height of a rectangular
area.
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
---
Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
1 file changed, 6 insertions(+)
diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
index a3d56ffbf4cc..c09d06ef2b08 100644
--- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
+++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
@@ -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
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
2019-08-23 12:37 ` [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
2019-08-23 12:37 ` [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-23 12:37 ` [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls Ricardo Ribalda Delgado
` (5 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: 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
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
drivers/media/i2c/imx214.c | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..cc0a013ba7da 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -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",
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (2 preceding siblings ...)
2019-08-23 12:37 ` [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-29 10:57 ` Hans Verkuil
2019-08-23 12:37 ` [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area Ricardo Ribalda Delgado
` (4 subsequent siblings)
8 siblings, 2 replies; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: 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.
Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
include/media/v4l2-ctrls.h | 16 ++++++++++++++++
2 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b3bf458df7f7..33e48f0aec1a 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -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,
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 9a3d11350e67..36f0712ea6dd 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -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.
--
2.23.0.rc1
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (3 preceding siblings ...)
2019-08-23 12:37 ` [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-26 7:46 ` Jacopo Mondi
2019-08-23 12:37 ` [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper Ricardo Ribalda Delgado
` (3 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: Ricardo Ribalda Delgado
Helper for creating area controls.
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
Documentation/media/kapi/v4l2-controls.rst | 9 +++++++++
1 file changed, 9 insertions(+)
diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst
index ebe2a55908be..656e9428f6a6 100644
--- a/Documentation/media/kapi/v4l2-controls.rst
+++ b/Documentation/media/kapi/v4l2-controls.rst
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (4 preceding siblings ...)
2019-08-23 12:37 ` [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area Ricardo Ribalda Delgado
@ 2019-08-23 12:37 ` Ricardo Ribalda Delgado
2019-08-26 7:54 ` Jacopo Mondi
2019-08-26 7:12 ` [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Jacopo Mondi
` (2 subsequent siblings)
8 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 12:37 UTC (permalink / raw)
To: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel, Jacopo Mondi
Cc: Ricardo Ribalda Delgado
Instead of creating manually the V4L2_CID_UNIT_CELL_SIZE control, lets
use the helper.
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
drivers/media/i2c/imx214.c | 29 ++++++++---------------------
1 file changed, 8 insertions(+), 21 deletions(-)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index cc0a013ba7da..625617d4c81a 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -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
^ permalink raw reply related [flat|nested] 23+ messages in thread
* Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 12:37 ` [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls Ricardo Ribalda Delgado
@ 2019-08-23 12:56 ` Philipp Zabel
2019-08-23 13:05 ` Ricardo Ribalda Delgado
2019-08-29 10:57 ` Hans Verkuil
1 sibling, 1 reply; 23+ messages in thread
From: Philipp Zabel @ 2019-08-23 12:56 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate,
> try to minimize it by adding a new helper.
>
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
> include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b3bf458df7f7..33e48f0aec1a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -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 ...
> 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 */
... here?
Looks to me like this comment should stay attached to
v4l2_ctrl_new_int_menu.
regards
Philipp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
2019-08-23 12:37 ` [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
@ 2019-08-23 12:56 ` Philipp Zabel
0 siblings, 0 replies; 23+ messages in thread
From: Philipp Zabel @ 2019-08-23 12:56 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> 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
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
If the v4l2_ctrl_new_area helper is accepted, I'd reorder this
afterwards and squash it together with patch 7.
regards
Philipp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA
2019-08-23 12:37 ` [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA Ricardo Ribalda Delgado
@ 2019-08-23 12:56 ` Philipp Zabel
2019-08-26 7:40 ` Jacopo Mondi
1 sibling, 0 replies; 23+ messages in thread
From: Philipp Zabel @ 2019-08-23 12:56 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> A struct v4l2_area containing the width and the height of a rectangular
> area.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index a3d56ffbf4cc..c09d06ef2b08 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
2019-08-23 12:37 ` [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
@ 2019-08-23 12:56 ` Philipp Zabel
0 siblings, 0 replies; 23+ messages in thread
From: Philipp Zabel @ 2019-08-23 12:56 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> New control to pass to userspace the width/height of a pixel. Which is
> needed for calibration and lens selection.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 8 ++++++++
> 1 file changed, 8 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
> index 2c3ab5796d76..6e728accf67b 100644
> --- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
> +++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
> @@ -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.
Can we have a link from here to the struct v4l2_area documentation?
regards
Philipp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 12:56 ` Philipp Zabel
@ 2019-08-23 13:05 ` Ricardo Ribalda Delgado
2019-08-23 13:13 ` Philipp Zabel
0 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-08-23 13:05 UTC (permalink / raw)
To: Philipp Zabel
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, linux-media,
LKML, Jacopo Mondi
On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
>
> On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate,
> > try to minimize it by adding a new helper.
> >
> > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
> > include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> > 2 files changed, 40 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index b3bf458df7f7..33e48f0aec1a 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -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 ...
>
> > 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 */
>
> ... 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
>
> Looks to me like this comment should stay attached to
> v4l2_ctrl_new_int_menu.
>
> regards
> Philipp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 13:05 ` Ricardo Ribalda Delgado
@ 2019-08-23 13:13 ` Philipp Zabel
2019-08-26 7:43 ` Jacopo Mondi
0 siblings, 1 reply; 23+ messages in thread
From: Philipp Zabel @ 2019-08-23 13:13 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus, linux-media,
LKML, Jacopo Mondi
On Fri, 2019-08-23 at 15:05 +0200, Ricardo Ribalda Delgado wrote:
> On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> >
> > On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> > > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate,
> > > try to minimize it by adding a new helper.
> > >
> > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > > ---
> > > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
> > > include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> > > 2 files changed, 40 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > index b3bf458df7f7..33e48f0aec1a 100644
> > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > @@ -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 ...
> >
> > > 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 */
> >
> > ... 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
Awesome, thanks! Feel free to add
Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
regards
Philipp
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (5 preceding siblings ...)
2019-08-23 12:37 ` [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper Ricardo Ribalda Delgado
@ 2019-08-26 7:12 ` Jacopo Mondi
2019-08-26 7:30 ` Jacopo Mondi
2019-08-29 10:39 ` Hans Verkuil
8 siblings, 0 replies; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:12 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 4951 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 02:37:31PM +0200, Ricardo Ribalda Delgado wrote:
> 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.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> 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(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..b3bf458df7f7 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_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);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..9a3d11350e67 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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;
> };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..5a7bedee2b0e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -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 */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 530638dffd93..05cfc69d7ed6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -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,
> + */
With a confirmation from Hans and others this comments fits here
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> + V4L2_CTRL_TYPE_AREA = 0x0106,
> };
>
> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> --
> 2.23.0.rc1
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (6 preceding siblings ...)
2019-08-26 7:12 ` [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Jacopo Mondi
@ 2019-08-26 7:30 ` Jacopo Mondi
2019-08-29 10:39 ` Hans Verkuil
8 siblings, 0 replies; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:30 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 5144 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 02:37:31PM +0200, Ricardo Ribalda Delgado wrote:
> 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.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> 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(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..b3bf458df7f7 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_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);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..9a3d11350e67 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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
> * @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;
> };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..5a7bedee2b0e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -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 */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 530638dffd93..05cfc69d7ed6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA
2019-08-23 12:37 ` [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
@ 2019-08-26 7:40 ` Jacopo Mondi
2019-09-12 10:51 ` Ricardo Ribalda Delgado
1 sibling, 1 reply; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:40 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1271 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 02:37:33PM +0200, Ricardo Ribalda Delgado wrote:
> A struct v4l2_area containing the width and the height of a rectangular
> area.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> ---
> Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
> 1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> index a3d56ffbf4cc..c09d06ef2b08 100644
> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> @@ -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
> + - 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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 13:13 ` Philipp Zabel
@ 2019-08-26 7:43 ` Jacopo Mondi
0 siblings, 0 replies; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:43 UTC (permalink / raw)
To: Philipp Zabel
Cc: Ricardo Ribalda Delgado, Mauro Carvalho Chehab, Hans Verkuil,
Sakari Ailus, linux-media, LKML
[-- Attachment #1: Type: text/plain, Size: 3147 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 03:13:36PM +0200, Philipp Zabel wrote:
> On Fri, 2019-08-23 at 15:05 +0200, Ricardo Ribalda Delgado wrote:
> > On Fri, Aug 23, 2019 at 2:56 PM Philipp Zabel <p.zabel@pengutronix.de> wrote:
> > >
> > > On Fri, 2019-08-23 at 14:37 +0200, Ricardo Ribalda Delgado wrote:
> > > > Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate,
> > > > try to minimize it by adding a new helper.
> > > >
> > > > Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> > > > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > > > ---
> > > > drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
> > > > include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> > > > 2 files changed, 40 insertions(+), 1 deletion(-)
> > > >
> > > > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > index b3bf458df7f7..33e48f0aec1a 100644
> > > > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > > > @@ -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 ...
> > >
> > > > 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 */
> > >
> > > ... 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
>
> Awesome, thanks! Feel free to add
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
With the issue pointed out by Philipp addressed
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
>
> regards
> Philipp
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area
2019-08-23 12:37 ` [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area Ricardo Ribalda Delgado
@ 2019-08-26 7:46 ` Jacopo Mondi
0 siblings, 0 replies; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:46 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 1262 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 02:37:36PM +0200, Ricardo Ribalda Delgado wrote:
> Helper for creating area controls.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
With this squashed on 5/7 or separated if we want documentation
changes to get in separately
Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
Thanks
j
> ---
> Documentation/media/kapi/v4l2-controls.rst | 9 +++++++++
> 1 file changed, 9 insertions(+)
>
> diff --git a/Documentation/media/kapi/v4l2-controls.rst b/Documentation/media/kapi/v4l2-controls.rst
> index ebe2a55908be..656e9428f6a6 100644
> --- a/Documentation/media/kapi/v4l2-controls.rst
> +++ b/Documentation/media/kapi/v4l2-controls.rst
> @@ -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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper
2019-08-23 12:37 ` [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper Ricardo Ribalda Delgado
@ 2019-08-26 7:54 ` Jacopo Mondi
0 siblings, 0 replies; 23+ messages in thread
From: Jacopo Mondi @ 2019-08-26 7:54 UTC (permalink / raw)
To: Ricardo Ribalda Delgado
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, linux-kernel
[-- Attachment #1: Type: text/plain, Size: 2096 bytes --]
Hi Ricardo,
On Fri, Aug 23, 2019 at 02:37:37PM +0200, Ricardo Ribalda Delgado wrote:
> 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
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> drivers/media/i2c/imx214.c | 29 ++++++++---------------------
> 1 file changed, 8 insertions(+), 21 deletions(-)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index cc0a013ba7da..625617d4c81a 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -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
>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
` (7 preceding siblings ...)
2019-08-26 7:30 ` Jacopo Mondi
@ 2019-08-29 10:39 ` Hans Verkuil
8 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2019-08-29 10:39 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Philipp Zabel, Mauro Carvalho Chehab,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On 8/23/19 2:37 PM, Ricardo Ribalda Delgado wrote:
> 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.
>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> 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(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..b3bf458df7f7 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_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:
You need to check that the width and height are not 0 (that would make no sense).
> + 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);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..9a3d11350e67 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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;
> };
>
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index a2669b79b294..5a7bedee2b0e 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -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 */
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 530638dffd93..05cfc69d7ed6 100644
> --- a/include/uapi/linux/videodev2.h
> +++ b/include/uapi/linux/videodev2.h
> @@ -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,
> + */
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.
> + V4L2_CTRL_TYPE_AREA = 0x0106,
> };
>
> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
>
Regards,
Hans
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls
2019-08-23 12:37 ` [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
@ 2019-08-29 10:57 ` Hans Verkuil
1 sibling, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2019-08-29 10:57 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Philipp Zabel, Mauro Carvalho Chehab,
Sakari Ailus, linux-media, linux-kernel, Jacopo Mondi
On 8/23/19 2:37 PM, Ricardo Ribalda Delgado wrote:
> Adding a V4L2_CID_UNIT_CELL_SIZE control requires a lot of boilerplate,
> try to minimize it by adding a new helper.
>
> Suggested-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 25 ++++++++++++++++++++++++-
> include/media/v4l2-ctrls.h | 16 ++++++++++++++++
> 2 files changed, 40 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index b3bf458df7f7..33e48f0aec1a 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -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?
> 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.
> +}
> +
> +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.
> +
> +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.
> + .type_ops = &area_ops,
And just drop this line.
> + };
> +
> + 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.
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_new_area);
> +
> +/* Helper function for standard integer menu controls */
This comment doesn't belong here.
> /* Add the controls from another handler to our own. */
> int v4l2_ctrl_add_handler(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ctrl_handler *add,
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 9a3d11350e67..36f0712ea6dd 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -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.
> + *
> + * 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
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA
2019-08-26 7:40 ` Jacopo Mondi
@ 2019-09-12 10:51 ` Ricardo Ribalda Delgado
2019-09-12 11:00 ` Hans Verkuil
0 siblings, 1 reply; 23+ messages in thread
From: Ricardo Ribalda Delgado @ 2019-09-12 10:51 UTC (permalink / raw)
To: Jacopo Mondi
Cc: Philipp Zabel, Mauro Carvalho Chehab, Hans Verkuil, Sakari Ailus,
linux-media, LKML
HI Jacopo
(Sorry for the late reply, I have been in holidays plus with plenty of
family matters)
On Mon, Aug 26, 2019 at 9:39 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Fri, Aug 23, 2019 at 02:37:33PM +0200, Ricardo Ribalda Delgado wrote:
> > A struct v4l2_area containing the width and the height of a rectangular
> > area.
> >
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> > ---
> > Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
> > 1 file changed, 6 insertions(+)
> >
> > diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > index a3d56ffbf4cc..c09d06ef2b08 100644
> > --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
> > @@ -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?
> Thanks
> j
>
> > + - 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
> >
^ permalink raw reply [flat|nested] 23+ messages in thread
* Re: [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA
2019-09-12 10:51 ` Ricardo Ribalda Delgado
@ 2019-09-12 11:00 ` Hans Verkuil
0 siblings, 0 replies; 23+ messages in thread
From: Hans Verkuil @ 2019-09-12 11:00 UTC (permalink / raw)
To: Ricardo Ribalda Delgado, Jacopo Mondi
Cc: Philipp Zabel, Mauro Carvalho Chehab, Sakari Ailus, linux-media, LKML
On 9/12/19 12:51 PM, Ricardo Ribalda Delgado wrote:
> HI Jacopo
>
> (Sorry for the late reply, I have been in holidays plus with plenty of
> family matters)
>
> On Mon, Aug 26, 2019 at 9:39 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>>
>> Hi Ricardo,
>>
>> On Fri, Aug 23, 2019 at 02:37:33PM +0200, Ricardo Ribalda Delgado wrote:
>>> A struct v4l2_area containing the width and the height of a rectangular
>>> area.
>>>
>>> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
>>> Suggested-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>> ---
>>> Documentation/media/uapi/v4l/vidioc-queryctrl.rst | 6 ++++++
>>> 1 file changed, 6 insertions(+)
>>>
>>> diff --git a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> index a3d56ffbf4cc..c09d06ef2b08 100644
>>> --- a/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> +++ b/Documentation/media/uapi/v4l/vidioc-queryctrl.rst
>>> @@ -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?
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
>
>> Thanks
>> j
>>
>>> + - 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
>>>
^ permalink raw reply [flat|nested] 23+ messages in thread
end of thread, other threads:[~2019-09-12 11:00 UTC | newest]
Thread overview: 23+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-08-23 12:37 [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Ricardo Ribalda Delgado
2019-08-23 12:37 ` [PATCH v3 2/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-23 12:37 ` [PATCH v3 3/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-26 7:40 ` Jacopo Mondi
2019-09-12 10:51 ` Ricardo Ribalda Delgado
2019-09-12 11:00 ` Hans Verkuil
2019-08-23 12:37 ` [PATCH v3 4/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-23 12:37 ` [PATCH v3 5/7] media: v4l2-core: Add new helper for area controls Ricardo Ribalda Delgado
2019-08-23 12:56 ` Philipp Zabel
2019-08-23 13:05 ` Ricardo Ribalda Delgado
2019-08-23 13:13 ` Philipp Zabel
2019-08-26 7:43 ` Jacopo Mondi
2019-08-29 10:57 ` Hans Verkuil
2019-08-23 12:37 ` [PATCH v3 6/7] Documentation: Document v4l2_ctrl_new area Ricardo Ribalda Delgado
2019-08-26 7:46 ` Jacopo Mondi
2019-08-23 12:37 ` [PATCH v3 7/7] imx214: Use v4l2_ctrl_new_area helper Ricardo Ribalda Delgado
2019-08-26 7:54 ` Jacopo Mondi
2019-08-26 7:12 ` [PATCH v3 1/7] media: add V4L2_CID_UNIT_CELL_SIZE control Jacopo Mondi
2019-08-26 7:30 ` Jacopo Mondi
2019-08-29 10:39 ` Hans Verkuil
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.