UNIT_CELL_SIZE is a control that represents the size of a cell (pixel).
We required a bit of boilerplate to add this control :)
- New way to init compount controls
- New control type
Thanks to Hans, Jacopo and Philipp for your help.
You might want to see the series at my github repository if needed.
https://github.com/ribalda/linux/tree/unit-size-v6
v4, v5 of this patchset never reached the mailing list.
Ricardo Ribalda Delgado (7):
media: v4l2-core: Implement v4l2_ctrl_new_std_compound
Documentation: v4l2_ctrl_new_std_compound
media: add V4L2_CTRL_TYPE_AREA control type
Documentation: media: Document V4L2_CTRL_TYPE_AREA
media: add V4L2_CID_UNIT_CELL_SIZE control
Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE
media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE
Documentation/media/kapi/v4l2-controls.rst | 9 +++
.../media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++
.../media/uapi/v4l/vidioc-queryctrl.rst | 6 ++
drivers/media/i2c/imx214.c | 12 +++
drivers/media/v4l2-core/v4l2-ctrls.c | 76 +++++++++++++++++--
include/media/v4l2-ctrls.h | 63 ++++++++++++++-
include/uapi/linux/v4l2-controls.h | 1 +
include/uapi/linux/videodev2.h | 6 ++
8 files changed, 174 insertions(+), 8 deletions(-)
--
2.23.0
Function for initializing compound controls with a default value.
Suggested-by: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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..b20800cae3f2 100644
--- a/Documentation/media/kapi/v4l2-controls.rst
+++ b/Documentation/media/kapi/v4l2-controls.rst
@@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling
const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
s32 skip_mask, s32 def, const char * const *qmenu);
+Standard compound controls can be added by calling
+:c:func:`v4l2_ctrl_new_std_compound`:
+
+.. code-block:: c
+
+ struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+ const struct v4l2_ctrl_ops *ops, u32 id,
+ const union v4l2_ctrl_ptr p_def);
+
Integer menu controls with a driver specific menu can be added by calling
:c:func:`v4l2_ctrl_new_int_menu`:
--
2.23.0
Currently compound controls do not have a simple way of initializing its
values. This results in ofuscated code with type_ops init.
This patch introduces a new field on the control with the default value
for the compound control that can be set with the brand new
v4l2_ctrl_new_std_compound function
Suggested-by: Hans Verkuil <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 50 ++++++++++++++++++++++++----
include/media/v4l2-ctrls.h | 22 +++++++++++-
2 files changed, 64 insertions(+), 8 deletions(-)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 1d8f38824631..219d8aeefa20 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -29,6 +29,8 @@
#define call_op(master, op) \
(has_op(master, op) ? master->ops->op(master) : 0)
+static const union v4l2_ctrl_ptr ptr_null;
+
/* Internal temporary helper struct, one for each v4l2_ext_control */
struct v4l2_ctrl_helper {
/* Pointer to the control reference of the master control */
@@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
void *p = ptr.p + idx * ctrl->elem_size;
- memset(p, 0, ctrl->elem_size);
+ if (ctrl->p_def.p)
+ memcpy(p, ctrl->p_def.p, ctrl->elem_size);
+ else
+ memset(p, 0, ctrl->elem_size);
/*
* The cast is needed to get rid of a gcc warning complaining that
@@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
s64 min, s64 max, u64 step, s64 def,
const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
u32 flags, const char * const *qmenu,
- const s64 *qmenu_int, void *priv)
+ const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
+ void *priv)
{
struct v4l2_ctrl *ctrl;
unsigned sz_extra;
@@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
is_array)
sz_extra += 2 * tot_ctrl_size;
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
+ sz_extra += elem_size;
+
ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
if (ctrl == NULL) {
handler_set_err(hdl, -ENOMEM);
@@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
ctrl->p_new.p = &ctrl->val;
ctrl->p_cur.p = &ctrl->cur.val;
}
+
+ if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
+ ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
+ memcpy(ctrl->p_def.p, p_def.p, elem_size);
+ }
+
for (idx = 0; idx < elems; idx++) {
ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
@@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
type, min, max,
is_menu ? cfg->menu_skip_mask : step, def,
cfg->dims, cfg->elem_size,
- flags, qmenu, qmenu_int, priv);
+ flags, qmenu, qmenu_int, ptr_null, priv);
if (ctrl)
ctrl->is_private = cfg->is_private;
return ctrl;
@@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
- flags, NULL, NULL, NULL);
+ flags, NULL, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std);
@@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
- flags, qmenu, qmenu_int, NULL);
+ flags, qmenu, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
@@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, mask, def, NULL, 0,
- flags, qmenu, NULL, NULL);
+ flags, qmenu, NULL, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
+/* Helper function for standard compound controls */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+ const struct v4l2_ctrl_ops *ops, u32 id,
+ const union v4l2_ctrl_ptr p_def)
+{
+ const char *name;
+ enum v4l2_ctrl_type type;
+ u32 flags;
+ s64 min, max, step, def;
+
+ v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
+ if (type < V4L2_CTRL_COMPOUND_TYPES) {
+ handler_set_err(hdl, -EINVAL);
+ return NULL;
+ }
+ return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
+ min, max, step, def, NULL, 0,
+ flags, NULL, NULL, p_def, NULL);
+}
+EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
+
/* 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,
@@ -2669,7 +2705,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
}
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
0, max, 0, def, NULL, 0,
- flags, NULL, qmenu_int, NULL);
+ flags, NULL, qmenu_int, ptr_null, NULL);
}
EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 570ff4b0205a..4b356df850a1 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -200,6 +200,9 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
* not freed when the control is deleted. Should this be needed
* then a new internal bitfield can be added to tell the framework
* to free this pointer.
+ * @p_def: The control's default value represented via a union which
+ * provides a standard way of accessing control types
+ * through a pointer (for compound controls only).
* @p_cur: The control's current value represented via a union which
* provides a standard way of accessing control types
* through a pointer.
@@ -254,6 +257,7 @@ struct v4l2_ctrl {
s32 val;
} cur;
+ union v4l2_ctrl_ptr p_def;
union v4l2_ctrl_ptr p_new;
union v4l2_ctrl_ptr p_cur;
};
@@ -618,7 +622,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
const struct v4l2_ctrl_ops *ops,
u32 id, u8 max, u64 mask, u8 def);
-
/**
* v4l2_ctrl_new_std_menu_items() - Create a new standard V4L2 menu control
* with driver specific menu.
@@ -646,6 +649,23 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
u64 mask, u8 def,
const char * const *qmenu);
+/**
+ * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
+ * compound control.
+ *
+ * @hdl: The control handler.
+ * @ops: The control ops.
+ * @id: The control ID.
+ * @p_def: The control's p_def value.
+ *
+ * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks
+ * to the @p_def field.
+ *
+ */
+struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
+ const struct v4l2_ctrl_ops *ops, u32 id,
+ const union v4l2_ctrl_ptr p_def);
+
/**
* v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
*
--
2.23.0
From: Ricardo Ribalda Delgado <[email protected]>
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 <[email protected]>
---
drivers/media/i2c/imx214.c | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 159a3a604f0e..57562e20c4ca 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];
@@ -948,6 +949,13 @@ 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,
+ };
+ union v4l2_ctrl_ptr p_def = {
+ .p_area = &unit_size,
+ };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
V4L2_CID_EXPOSURE,
0, 3184, 1, 0x0c70);
+ imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
+ NULL,
+ V4L2_CID_UNIT_CELL_SIZE,
+ p_def);
ret = imx214->ctrls.error;
if (ret) {
dev_err(&client->dev, "%s control init failed (%d)\n",
--
2.23.0
From: Ricardo Ribalda Delgado <[email protected]>
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.
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 5 +++++
include/uapi/linux/v4l2-controls.h | 1 +
2 files changed, 6 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index b9a46f536406..f626f9983408 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -996,6 +996,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! */
@@ -1377,6 +1378,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;
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 */
--
2.23.0
From: Ricardo Ribalda Delgado <[email protected]>
A struct v4l2_area containing the width and the height of a rectangular
area.
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
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..33aff21b7d11 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. Units depend on the use case.
* - ``V4L2_CTRL_TYPE_H264_SPS``
- n/a
- n/a
--
2.23.0
From: Ricardo Ribalda Delgado <[email protected]>
New control to pass to userspace the width/height of a pixel. Which is
needed for calibration and lens selection.
Reviewed-by: Philipp Zabel <[email protected]>
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
Documentation/media/uapi/v4l/ext-ctrls-image-source.rst | 9 +++++++++
1 file changed, 9 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..033672dcb43d 100644
--- a/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
+++ b/Documentation/media/uapi/v4l/ext-ctrls-image-source.rst
@@ -55,3 +55,12 @@ 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
+ :c:type:`v4l2_area` 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
This type contains the width and the height of a rectangular area.
Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
---
drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++++++++++++++
include/media/v4l2-ctrls.h | 41 ++++++++++++++++++++++++++++
include/uapi/linux/videodev2.h | 6 ++++
3 files changed, 68 insertions(+)
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 219d8aeefa20..b9a46f536406 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1678,6 +1678,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
void *p = ptr.p + idx * ctrl->elem_size;
+ struct v4l2_area *area;
switch ((u32)ctrl->type) {
case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
@@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
zero_padding(p_vp8_frame_header->entropy_header);
zero_padding(p_vp8_frame_header->coder_state);
break;
+ case V4L2_CTRL_TYPE_AREA:
+ area = p;
+ if (!area->width || !area->height)
+ return -EINVAL;
+ break;
default:
return -EINVAL;
}
@@ -2427,6 +2433,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);
@@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
}
EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ lockdep_assert_held(ctrl->handler->lock);
+
+ /* It's a driver bug if this happens. */
+ WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
+ memcpy(ctrl->p_new.p_area, area, sizeof(*area));
+ return set_ctrl(NULL, ctrl, 0);
+}
+EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
+
void v4l2_ctrl_request_complete(struct media_request *req,
struct v4l2_ctrl_handler *main_hdl)
{
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 4b356df850a1..746969559ef3 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;
};
@@ -1085,6 +1087,45 @@ static inline int v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
return rval;
}
+/**
+ * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
+ *
+ * @ctrl: The control.
+ * @area: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function assumes the control's handler is already locked,
+ * allowing it to be used from within the &v4l2_ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area);
+
+/**
+ * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
+ * from within a driver.
+ *
+ * @ctrl: The control.
+ * @s: The new area.
+ *
+ * This sets the control's new area safely by going through the control
+ * framework. This function will lock the control's handler, so it cannot be
+ * used from within the &v4l2_ctrl_ops functions.
+ *
+ * This function is for area type controls only.
+ */
+static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
+ const struct v4l2_area *area)
+{
+ int rval;
+
+ v4l2_ctrl_lock(ctrl);
+ rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
+ v4l2_ctrl_unlock(ctrl);
+
+ return rval;
+}
/* Internal helper functions that deal with control events. */
extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index 530638dffd93..b3c0961b62a0 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,7 @@ enum v4l2_ctrl_type {
V4L2_CTRL_TYPE_U8 = 0x0100,
V4L2_CTRL_TYPE_U16 = 0x0101,
V4L2_CTRL_TYPE_U32 = 0x0102,
+ V4L2_CTRL_TYPE_AREA = 0x0106,
};
/* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
--
2.23.0
Hi Ricardo,
On Fri, Sep 20, 2019 at 03:51:31PM +0200, Ricardo Ribalda Delgado wrote:
> Currently compound controls do not have a simple way of initializing its
> values. This results in ofuscated code with type_ops init.
>
> This patch introduces a new field on the control with the default value
> for the compound control that can be set with the brand new
> v4l2_ctrl_new_std_compound function
>
> Suggested-by: Hans Verkuil <[email protected]>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 50 ++++++++++++++++++++++++----
> include/media/v4l2-ctrls.h | 22 +++++++++++-
> 2 files changed, 64 insertions(+), 8 deletions(-)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 1d8f38824631..219d8aeefa20 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -29,6 +29,8 @@
> #define call_op(master, op) \
> (has_op(master, op) ? master->ops->op(master) : 0)
>
> +static const union v4l2_ctrl_ptr ptr_null;
> +
> /* Internal temporary helper struct, one for each v4l2_ext_control */
> struct v4l2_ctrl_helper {
> /* Pointer to the control reference of the master control */
> @@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> void *p = ptr.p + idx * ctrl->elem_size;
>
> - memset(p, 0, ctrl->elem_size);
> + if (ctrl->p_def.p)
> + memcpy(p, ctrl->p_def.p, ctrl->elem_size);
> + else
> + memset(p, 0, ctrl->elem_size);
>
> /*
> * The cast is needed to get rid of a gcc warning complaining that
> @@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> s64 min, s64 max, u64 step, s64 def,
> const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> u32 flags, const char * const *qmenu,
> - const s64 *qmenu_int, void *priv)
> + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
> + void *priv)
> {
> struct v4l2_ctrl *ctrl;
> unsigned sz_extra;
> @@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> is_array)
> sz_extra += 2 * tot_ctrl_size;
>
> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
> + sz_extra += elem_size;
> +
I'm not sure I get how sz_extra is used in this function and what's
its purpose, just be aware that the previous if() condition already
sz_extra += 2 * tot_ctrl_size
for compound controls.
Are you just making space for the new p_def elements ? Should't you
then use tot_cltr_size ? In patch 3 you add support for AREA which has
a single element, but could p_def in future transport multiple values?
> ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
> if (ctrl == NULL) {
> handler_set_err(hdl, -ENOMEM);
> @@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> ctrl->p_new.p = &ctrl->val;
> ctrl->p_cur.p = &ctrl->cur.val;
> }
> +
> + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
> + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
> + memcpy(ctrl->p_def.p, p_def.p, elem_size);
> + }
> +
> for (idx = 0; idx < elems; idx++) {
> ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
> ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> @@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> type, min, max,
> is_menu ? cfg->menu_skip_mask : step, def,
> cfg->dims, cfg->elem_size,
> - flags, qmenu, qmenu_int, priv);
> + flags, qmenu, qmenu_int, ptr_null, priv);
> if (ctrl)
> ctrl->is_private = cfg->is_private;
> return ctrl;
> @@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> }
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> min, max, step, def, NULL, 0,
> - flags, NULL, NULL, NULL);
> + flags, NULL, NULL, ptr_null, NULL);
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_std);
>
> @@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> }
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> 0, max, mask, def, NULL, 0,
> - flags, qmenu, qmenu_int, NULL);
> + flags, qmenu, qmenu_int, ptr_null, NULL);
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
>
> @@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> }
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> 0, max, mask, def, NULL, 0,
> - flags, qmenu, NULL, NULL);
> + flags, qmenu, NULL, ptr_null, NULL);
>
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
>
> +/* Helper function for standard compound controls */
> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> + const struct v4l2_ctrl_ops *ops, u32 id,
> + const union v4l2_ctrl_ptr p_def)
> +{
> + const char *name;
> + enum v4l2_ctrl_type type;
> + u32 flags;
> + s64 min, max, step, def;
> +
> + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> + if (type < V4L2_CTRL_COMPOUND_TYPES) {
> + handler_set_err(hdl, -EINVAL);
> + return NULL;
> + }
> + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> + min, max, step, def, NULL, 0,
> + flags, NULL, NULL, p_def, NULL);
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> +
> /* 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,
> @@ -2669,7 +2705,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
> }
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> 0, max, 0, def, NULL, 0,
> - flags, NULL, qmenu_int, NULL);
> + flags, NULL, qmenu_int, ptr_null, NULL);
> }
> EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
>
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 570ff4b0205a..4b356df850a1 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -200,6 +200,9 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> * not freed when the control is deleted. Should this be needed
> * then a new internal bitfield can be added to tell the framework
> * to free this pointer.
> + * @p_def: The control's default value represented via a union which
> + * provides a standard way of accessing control types
> + * through a pointer (for compound controls only).
> * @p_cur: The control's current value represented via a union which
> * provides a standard way of accessing control types
> * through a pointer.
> @@ -254,6 +257,7 @@ struct v4l2_ctrl {
> s32 val;
> } cur;
>
> + union v4l2_ctrl_ptr p_def;
> union v4l2_ctrl_ptr p_new;
> union v4l2_ctrl_ptr p_cur;
> };
> @@ -618,7 +622,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> const struct v4l2_ctrl_ops *ops,
> u32 id, u8 max, u64 mask, u8 def);
> -
This seems unrelated
> /**
> * v4l2_ctrl_new_std_menu_items() - Create a new standard V4L2 menu control
> * with driver specific menu.
> @@ -646,6 +649,23 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> u64 mask, u8 def,
> const char * const *qmenu);
>
> +/**
> + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
> + * compound control.
> + *
> + * @hdl: The control handler.
> + * @ops: The control ops.
> + * @id: The control ID.
> + * @p_def: The control's p_def value.
Nit:
s/p_def value/default value/ ?
Thanks
j
> + *
> + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks
> + * to the @p_def field.
> + *
> + */
> +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> + const struct v4l2_ctrl_ops *ops, u32 id,
> + const union v4l2_ctrl_ptr p_def);
> +
> /**
> * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
> *
> --
> 2.23.0
>
Hi Ricardo,
On Fri, Sep 20, 2019 at 03:51:33PM +0200, Ricardo Ribalda Delgado wrote:
> This type contains the width and the height of a rectangular area.
>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> drivers/media/v4l2-core/v4l2-ctrls.c | 21 ++++++++++++++
> include/media/v4l2-ctrls.h | 41 ++++++++++++++++++++++++++++
> include/uapi/linux/videodev2.h | 6 ++++
> 3 files changed, 68 insertions(+)
>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 219d8aeefa20..b9a46f536406 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1678,6 +1678,7 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> struct v4l2_ctrl_vp8_frame_header *p_vp8_frame_header;
> void *p = ptr.p + idx * ctrl->elem_size;
> + struct v4l2_area *area;
>
> switch ((u32)ctrl->type) {
> case V4L2_CTRL_TYPE_MPEG2_SLICE_PARAMS:
> @@ -1753,6 +1754,11 @@ static int std_validate_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> zero_padding(p_vp8_frame_header->entropy_header);
> zero_padding(p_vp8_frame_header->coder_state);
> break;
> + case V4L2_CTRL_TYPE_AREA:
> + area = p;
> + if (!area->width || !area->height)
> + return -EINVAL;
> + break;
> default:
> return -EINVAL;
> }
> @@ -2427,6 +2433,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);
> @@ -4116,6 +4125,18 @@ int __v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
> }
> EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_string);
>
> +int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
> + const struct v4l2_area *area)
> +{
> + lockdep_assert_held(ctrl->handler->lock);
> +
> + /* It's a driver bug if this happens. */
> + WARN_ON(ctrl->type != V4L2_CTRL_TYPE_AREA);
> + memcpy(ctrl->p_new.p_area, area, sizeof(*area));
> + return set_ctrl(NULL, ctrl, 0);
> +}
> +EXPORT_SYMBOL(__v4l2_ctrl_s_ctrl_area);
> +
> void v4l2_ctrl_request_complete(struct media_request *req,
> struct v4l2_ctrl_handler *main_hdl)
> {
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 4b356df850a1..746969559ef3 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;
> };
>
> @@ -1085,6 +1087,45 @@ static inline int v4l2_ctrl_s_ctrl_string(struct v4l2_ctrl *ctrl, const char *s)
> return rval;
> }
>
> +/**
> + * __v4l2_ctrl_s_ctrl_area() - Unlocked variant of v4l2_ctrl_s_ctrl_area().
> + *
> + * @ctrl: The control.
> + * @area: The new area.
> + *
> + * This sets the control's new area safely by going through the control
> + * framework. This function assumes the control's handler is already locked,
> + * allowing it to be used from within the &v4l2_ctrl_ops functions.
> + *
> + * This function is for area type controls only.
> + */
> +int __v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
> + const struct v4l2_area *area);
> +
> +/**
> + * v4l2_ctrl_s_ctrl_area() - Helper function to set a control's area value
> + * from within a driver.
> + *
> + * @ctrl: The control.
> + * @s: The new area.
> + *
> + * This sets the control's new area safely by going through the control
> + * framework. This function will lock the control's handler, so it cannot be
> + * used from within the &v4l2_ctrl_ops functions.
> + *
> + * This function is for area type controls only.
> + */
> +static inline int v4l2_ctrl_s_ctrl_area(struct v4l2_ctrl *ctrl,
> + const struct v4l2_area *area)
> +{
> + int rval;
> +
> + v4l2_ctrl_lock(ctrl);
> + rval = __v4l2_ctrl_s_ctrl_area(ctrl, area);
> + v4l2_ctrl_unlock(ctrl);
> +
> + return rval;
> +}
Newline maybe?
> /* Internal helper functions that deal with control events. */
> extern const struct v4l2_subscribed_event_ops v4l2_ctrl_sub_ev_ops;
>
> diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
> index 530638dffd93..b3c0961b62a0 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,7 @@ enum v4l2_ctrl_type {
> V4L2_CTRL_TYPE_U8 = 0x0100,
> V4L2_CTRL_TYPE_U16 = 0x0101,
> V4L2_CTRL_TYPE_U32 = 0x0102,
> + V4L2_CTRL_TYPE_AREA = 0x0106,
I'll let Hans comment on these two bits as I don't know if that's
adding a new control type is right or not.
Have my R-b for the rest though:
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> };
>
> /* Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> --
> 2.23.0
>
Hi Ricardo,
On Fri, Sep 20, 2019 at 03:51:34PM +0200, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <[email protected]>
>
> A struct v4l2_area containing the width and the height of a rectangular
> area.
>
> Reviewed-by: Philipp Zabel <[email protected]>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> 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..33aff21b7d11 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. Units depend on the use case.
I recall Hans too was in favour of having min, max and step defined
(and applied to both width and height).
Really a minor issue from my side, feel free to keep it the way it is
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> * - ``V4L2_CTRL_TYPE_H264_SPS``
> - n/a
> - n/a
> --
> 2.23.0
>
Hi Ricardo,
On Fri, Sep 20, 2019 at 03:51:32PM +0200, Ricardo Ribalda Delgado wrote:
> Function for initializing compound controls with a default value.
>
> Suggested-by: Hans Verkuil <[email protected]>
> Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> ---
> 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..b20800cae3f2 100644
> --- a/Documentation/media/kapi/v4l2-controls.rst
> +++ b/Documentation/media/kapi/v4l2-controls.rst
> @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling
> const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
> s32 skip_mask, s32 def, const char * const *qmenu);
>
> +Standard compound controls can be added by calling
Standard compound controls with a driver provided default value can be..
Or is it un-necessary to re-state it?
In any case:
Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
j
> +:c:func:`v4l2_ctrl_new_std_compound`:
> +
> +.. code-block:: c
> +
> + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> + const struct v4l2_ctrl_ops *ops, u32 id,
> + const union v4l2_ctrl_ptr p_def);
> +
> Integer menu controls with a driver specific menu can be added by calling
> :c:func:`v4l2_ctrl_new_int_menu`:
>
> --
> 2.23.0
>
Hi Ricardo,
On Fri, Sep 20, 2019 at 03:51:37PM +0200, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 159a3a604f0e..57562e20c4ca 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];
>
> @@ -948,6 +949,13 @@ 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,
> + };
> + union v4l2_ctrl_ptr p_def = {
> + .p_area = &unit_size,
> + };
> int ret;
>
> ret = imx214_parse_fwnode(dev);
> @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> V4L2_CID_EXPOSURE,
> 0, 3184, 1, 0x0c70);
>
> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> + NULL,
> + V4L2_CID_UNIT_CELL_SIZE,
> + p_def);
> ret = imx214->ctrls.error;
> if (ret) {
> dev_err(&client->dev, "%s control init failed (%d)\n",
Would something like this scale? I'm a bit bothered by the need of
declaring v4l2_ctrl_ptr structure just to set a field there in
drivers.
diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
index 57562e20c4ca..a00d8fa481c2 100644
--- a/drivers/media/i2c/imx214.c
+++ b/drivers/media/i2c/imx214.c
@@ -953,9 +953,6 @@ static int imx214_probe(struct i2c_client *client)
.width = 1120,
.height = 1120,
};
- union v4l2_ctrl_ptr p_def = {
- .p_area = &unit_size,
- };
int ret;
ret = imx214_parse_fwnode(dev);
@@ -1040,7 +1037,7 @@ static int imx214_probe(struct i2c_client *client)
imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
NULL,
V4L2_CID_UNIT_CELL_SIZE,
- p_def);
+ &unit_size);
ret = imx214->ctrls.error;
if (ret) {
dev_err(&client->dev, "%s control init failed (%d)\n",
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index f626f9983408..4a2648bee6f5 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -2681,18 +2681,26 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
/* Helper function for standard compound controls */
struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
const struct v4l2_ctrl_ops *ops, u32 id,
- const union v4l2_ctrl_ptr p_def)
+ void *defval)
{
const char *name;
enum v4l2_ctrl_type type;
u32 flags;
s64 min, max, step, def;
+ union v4l2_ctrl_ptr p_def;
v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
if (type < V4L2_CTRL_COMPOUND_TYPES) {
handler_set_err(hdl, -EINVAL);
return NULL;
}
+
+ switch ((u32)type) {
+ case V4L2_CTRL_TYPE_AREA:
+ p_def.p_area = defval;
+ break;
+ }
+
return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
min, max, step, def, NULL, 0,
flags, NULL, NULL, p_def, NULL);
However, due to my limitied understanding of the control framework, I
cannot tell how many cases the newly introduced switch should
handle...
> --
> 2.23.0
>
Hi Jacopo
On Wed, Sep 25, 2019 at 10:20 AM Jacopo Mondi <[email protected]> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:32PM +0200, Ricardo Ribalda Delgado wrote:
> > Function for initializing compound controls with a default value.
> >
> > Suggested-by: Hans Verkuil <[email protected]>
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > 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..b20800cae3f2 100644
> > --- a/Documentation/media/kapi/v4l2-controls.rst
> > +++ b/Documentation/media/kapi/v4l2-controls.rst
> > @@ -140,6 +140,15 @@ Menu controls with a driver specific menu are added by calling
> > const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
> > s32 skip_mask, s32 def, const char * const *qmenu);
> >
> > +Standard compound controls can be added by calling
>
> Standard compound controls with a driver provided default value can be..
>
> Or is it un-necessary to re-state it?
>
I think is a bit redundant, nothing stops you from not defining def,
if ctrl_fill has the default value for it.
> In any case:
> Reviewed-by: Jacopo Mondi <[email protected]>
Thanks
>
> Thanks
> j
> > +:c:func:`v4l2_ctrl_new_std_compound`:
> > +
> > +.. code-block:: c
> > +
> > + struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> > + const struct v4l2_ctrl_ops *ops, u32 id,
> > + const union v4l2_ctrl_ptr p_def);
> > +
> > Integer menu controls with a driver specific menu can be added by calling
> > :c:func:`v4l2_ctrl_new_int_menu`:
> >
> > --
> > 2.23.0
> >
--
Ricardo Ribalda
Hi Jacopo
On Wed, Sep 25, 2019 at 10:32 AM Jacopo Mondi <[email protected]> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:34PM +0200, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <[email protected]>
> >
> > A struct v4l2_area containing the width and the height of a rectangular
> > area.
> >
> > Reviewed-by: Philipp Zabel <[email protected]>
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > 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..33aff21b7d11 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. Units depend on the use case.
>
> I recall Hans too was in favour of having min, max and step defined
> (and applied to both width and height).
>
He changed his mind :)
https://www.mail-archive.com/[email protected]/msg150103.html
> Really a minor issue from my side, feel free to keep it the way it is
> Reviewed-by: Jacopo Mondi <[email protected]>
>
> Thanks
> j
> > * - ``V4L2_CTRL_TYPE_H264_SPS``
> > - n/a
> > - n/a
> > --
> > 2.23.0
> >
--
Ricardo Ribalda
Hi Jacopo
Thanks for your review!
On Wed, Sep 25, 2019 at 10:18 AM Jacopo Mondi <[email protected]> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:31PM +0200, Ricardo Ribalda Delgado wrote:
> > Currently compound controls do not have a simple way of initializing its
> > values. This results in ofuscated code with type_ops init.
> >
> > This patch introduces a new field on the control with the default value
> > for the compound control that can be set with the brand new
> > v4l2_ctrl_new_std_compound function
> >
> > Suggested-by: Hans Verkuil <[email protected]>
> > Signed-off-by: Ricardo Ribalda Delgado <[email protected]>
> > ---
> > drivers/media/v4l2-core/v4l2-ctrls.c | 50 ++++++++++++++++++++++++----
> > include/media/v4l2-ctrls.h | 22 +++++++++++-
> > 2 files changed, 64 insertions(+), 8 deletions(-)
> >
> > diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> > index 1d8f38824631..219d8aeefa20 100644
> > --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> > +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> > @@ -29,6 +29,8 @@
> > #define call_op(master, op) \
> > (has_op(master, op) ? master->ops->op(master) : 0)
> >
> > +static const union v4l2_ctrl_ptr ptr_null;
> > +
> > /* Internal temporary helper struct, one for each v4l2_ext_control */
> > struct v4l2_ctrl_helper {
> > /* Pointer to the control reference of the master control */
> > @@ -1530,7 +1532,10 @@ static void std_init_compound(const struct v4l2_ctrl *ctrl, u32 idx,
> > struct v4l2_ctrl_mpeg2_slice_params *p_mpeg2_slice_params;
> > void *p = ptr.p + idx * ctrl->elem_size;
> >
> > - memset(p, 0, ctrl->elem_size);
> > + if (ctrl->p_def.p)
> > + memcpy(p, ctrl->p_def.p, ctrl->elem_size);
> > + else
> > + memset(p, 0, ctrl->elem_size);
> >
> > /*
> > * The cast is needed to get rid of a gcc warning complaining that
> > @@ -2354,7 +2359,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > s64 min, s64 max, u64 step, s64 def,
> > const u32 dims[V4L2_CTRL_MAX_DIMS], u32 elem_size,
> > u32 flags, const char * const *qmenu,
> > - const s64 *qmenu_int, void *priv)
> > + const s64 *qmenu_int, const union v4l2_ctrl_ptr p_def,
> > + void *priv)
> > {
> > struct v4l2_ctrl *ctrl;
> > unsigned sz_extra;
> > @@ -2460,6 +2466,9 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > is_array)
> > sz_extra += 2 * tot_ctrl_size;
> >
> > + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p)
> > + sz_extra += elem_size;
> > +
>
> I'm not sure I get how sz_extra is used in this function and what's
> its purpose, just be aware that the previous if() condition already
>
> sz_extra += 2 * tot_ctrl_size
>
> for compound controls.
>
> Are you just making space for the new p_def elements ? Should't you
> then use tot_cltr_size ? In patch 3 you add support for AREA which has
> a single element, but could p_def in future transport multiple values?
>
I am making space for p_def. I only want to make space for one
element, because that value will be used for all of them if there is
an array.
In case the user wants to provide a different value per element of the
array he has to provide a function initializer, just like with the not
compo\
> > ctrl = kvzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
> > if (ctrl == NULL) {
> > handler_set_err(hdl, -ENOMEM);
> > @@ -2503,6 +2512,12 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
> > ctrl->p_new.p = &ctrl->val;
> > ctrl->p_cur.p = &ctrl->cur.val;
> > }
> > +
> > + if (type >= V4L2_CTRL_COMPOUND_TYPES && p_def.p) {
> > + ctrl->p_def.p = ctrl->p_cur.p + tot_ctrl_size;
> > + memcpy(ctrl->p_def.p, p_def.p, elem_size);
> > + }
> > +
> > for (idx = 0; idx < elems; idx++) {
> > ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
> > ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> > @@ -2554,7 +2569,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_custom(struct v4l2_ctrl_handler *hdl,
> > type, min, max,
> > is_menu ? cfg->menu_skip_mask : step, def,
> > cfg->dims, cfg->elem_size,
> > - flags, qmenu, qmenu_int, priv);
> > + flags, qmenu, qmenu_int, ptr_null, priv);
> > if (ctrl)
> > ctrl->is_private = cfg->is_private;
> > return ctrl;
> > @@ -2579,7 +2594,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> > }
> > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> > min, max, step, def, NULL, 0,
> > - flags, NULL, NULL, NULL);
> > + flags, NULL, NULL, ptr_null, NULL);
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_new_std);
> >
> > @@ -2612,7 +2627,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> > }
> > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> > 0, max, mask, def, NULL, 0,
> > - flags, qmenu, qmenu_int, NULL);
> > + flags, qmenu, qmenu_int, ptr_null, NULL);
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
> >
> > @@ -2644,11 +2659,32 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> > }
> > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> > 0, max, mask, def, NULL, 0,
> > - flags, qmenu, NULL, NULL);
> > + flags, qmenu, NULL, ptr_null, NULL);
> >
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
> >
> > +/* Helper function for standard compound controls */
> > +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> > + const struct v4l2_ctrl_ops *ops, u32 id,
> > + const union v4l2_ctrl_ptr p_def)
> > +{
> > + const char *name;
> > + enum v4l2_ctrl_type type;
> > + u32 flags;
> > + s64 min, max, step, def;
> > +
> > + v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> > + if (type < V4L2_CTRL_COMPOUND_TYPES) {
> > + handler_set_err(hdl, -EINVAL);
> > + return NULL;
> > + }
> > + return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> > + min, max, step, def, NULL, 0,
> > + flags, NULL, NULL, p_def, NULL);
> > +}
> > +EXPORT_SYMBOL(v4l2_ctrl_new_std_compound);
> > +
> > /* 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,
> > @@ -2669,7 +2705,7 @@ struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
> > }
> > return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> > 0, max, 0, def, NULL, 0,
> > - flags, NULL, qmenu_int, NULL);
> > + flags, NULL, qmenu_int, ptr_null, NULL);
> > }
> > EXPORT_SYMBOL(v4l2_ctrl_new_int_menu);
> >
> > diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> > index 570ff4b0205a..4b356df850a1 100644
> > --- a/include/media/v4l2-ctrls.h
> > +++ b/include/media/v4l2-ctrls.h
> > @@ -200,6 +200,9 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
> > * not freed when the control is deleted. Should this be needed
> > * then a new internal bitfield can be added to tell the framework
> > * to free this pointer.
> > + * @p_def: The control's default value represented via a union which
> > + * provides a standard way of accessing control types
> > + * through a pointer (for compound controls only).
> > * @p_cur: The control's current value represented via a union which
> > * provides a standard way of accessing control types
> > * through a pointer.
> > @@ -254,6 +257,7 @@ struct v4l2_ctrl {
> > s32 val;
> > } cur;
> >
> > + union v4l2_ctrl_ptr p_def;
> > union v4l2_ctrl_ptr p_new;
> > union v4l2_ctrl_ptr p_cur;
> > };
> > @@ -618,7 +622,6 @@ struct v4l2_ctrl *v4l2_ctrl_new_std(struct v4l2_ctrl_handler *hdl,
> > struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
> > const struct v4l2_ctrl_ops *ops,
> > u32 id, u8 max, u64 mask, u8 def);
> > -
>
> This seems unrelated
Good catch!
>
> > /**
> > * v4l2_ctrl_new_std_menu_items() - Create a new standard V4L2 menu control
> > * with driver specific menu.
> > @@ -646,6 +649,23 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(struct v4l2_ctrl_handler *hdl,
> > u64 mask, u8 def,
> > const char * const *qmenu);
> >
> > +/**
> > + * v4l2_ctrl_new_std_compound() - Allocate and initialize a new standard V4L2
> > + * compound control.
> > + *
> > + * @hdl: The control handler.
> > + * @ops: The control ops.
> > + * @id: The control ID.
> > + * @p_def: The control's p_def value.
>
> Nit:
> s/p_def value/default value/ ?
>
>
> Thanks
> j
Fixing all and uploading to my github waiting for more reviews :)
>
> > + *
> > + * Sames as v4l2_ctrl_new_std(), but with support to compound controls, thanks
> > + * to the @p_def field.
> > + *
> > + */
> > +struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> > + const struct v4l2_ctrl_ops *ops, u32 id,
> > + const union v4l2_ctrl_ptr p_def);
> > +
> > /**
> > * v4l2_ctrl_new_int_menu() - Create a new standard V4L2 integer menu control.
> > *
> > --
> > 2.23.0
> >
--
Ricardo Ribalda
Hi Jacopo
Thanks for the review
On Wed, Sep 25, 2019 at 11:24 AM Jacopo Mondi <[email protected]> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:37PM +0200, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/media/i2c/imx214.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..57562e20c4ca 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];
> >
> > @@ -948,6 +949,13 @@ 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,
> > + };
> > + union v4l2_ctrl_ptr p_def = {
> > + .p_area = &unit_size,
> > + };
> > int ret;
> >
> > ret = imx214_parse_fwnode(dev);
> > @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> > V4L2_CID_EXPOSURE,
> > 0, 3184, 1, 0x0c70);
> >
> > + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> > + NULL,
> > + V4L2_CID_UNIT_CELL_SIZE,
> > + p_def);
> > ret = imx214->ctrls.error;
> > if (ret) {
> > dev_err(&client->dev, "%s control init failed (%d)\n",
>
> Would something like this scale? I'm a bit bothered by the need of
> declaring v4l2_ctrl_ptr structure just to set a field there in
> drivers.
I have implemented the interface that Hans proposed on
https://www.mail-archive.com/[email protected]/msg149901.html
I thik Hans prefers this way to avoid castings, which can end in a lot
of "here be dragoons" :)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 57562e20c4ca..a00d8fa481c2 100644
> --- a/drivers/media/i2c/imx214.c
> +++ b/drivers/media/i2c/imx214.c
> @@ -953,9 +953,6 @@ static int imx214_probe(struct i2c_client *client)
> .width = 1120,
> .height = 1120,
> };
> - union v4l2_ctrl_ptr p_def = {
> - .p_area = &unit_size,
> - };
> int ret;
>
> ret = imx214_parse_fwnode(dev);
> @@ -1040,7 +1037,7 @@ static int imx214_probe(struct i2c_client *client)
> imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> NULL,
> V4L2_CID_UNIT_CELL_SIZE,
> - p_def);
> + &unit_size);
> ret = imx214->ctrls.error;
> if (ret) {
> dev_err(&client->dev, "%s control init failed (%d)\n",
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index f626f9983408..4a2648bee6f5 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -2681,18 +2681,26 @@ EXPORT_SYMBOL(v4l2_ctrl_new_std_menu_items);
> /* Helper function for standard compound controls */
> struct v4l2_ctrl *v4l2_ctrl_new_std_compound(struct v4l2_ctrl_handler *hdl,
> const struct v4l2_ctrl_ops *ops, u32 id,
> - const union v4l2_ctrl_ptr p_def)
> + void *defval)
> {
> const char *name;
> enum v4l2_ctrl_type type;
> u32 flags;
> s64 min, max, step, def;
> + union v4l2_ctrl_ptr p_def;
>
> v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
> if (type < V4L2_CTRL_COMPOUND_TYPES) {
> handler_set_err(hdl, -EINVAL);
> return NULL;
> }
> +
> + switch ((u32)type) {
> + case V4L2_CTRL_TYPE_AREA:
> + p_def.p_area = defval;
> + break;
> + }
> +
> return v4l2_ctrl_new(hdl, ops, NULL, id, name, type,
> min, max, step, def, NULL, 0,
> flags, NULL, NULL, p_def, NULL);
>
> However, due to my limitied understanding of the control framework, I
> cannot tell how many cases the newly introduced switch should
> handle...
>
> > --
> > 2.23.0
> >
--
Ricardo Ribalda
On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <[email protected]>
>
> 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 <[email protected]>
> ---
> drivers/media/i2c/imx214.c | 12 ++++++++++++
> 1 file changed, 12 insertions(+)
>
> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> index 159a3a604f0e..57562e20c4ca 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];
>
> @@ -948,6 +949,13 @@ 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,
> + };
> + union v4l2_ctrl_ptr p_def = {
> + .p_area = &unit_size,
> + };
Use static const for both.
I think you should add a small static inline helper function to v4l2-ctrls.h that
takes a void pointer and returns a union v4l2_ctrl_ptr.
Then you don't need to make a union v4l2_ctrl_ptr just to pass the unit_size pointer.
Regards,
Hans
> int ret;
>
> ret = imx214_parse_fwnode(dev);
> @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> V4L2_CID_EXPOSURE,
> 0, 3184, 1, 0x0c70);
>
> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> + NULL,
> + V4L2_CID_UNIT_CELL_SIZE,
> + p_def);
> ret = imx214->ctrls.error;
> if (ret) {
> dev_err(&client->dev, "%s control init failed (%d)\n",
>
Hi Hans
On Fri, 27 Sep 2019, 09:14 Hans Verkuil, <[email protected]> wrote:
>
> On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <[email protected]>
> >
> > 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 <[email protected]>
> > ---
> > drivers/media/i2c/imx214.c | 12 ++++++++++++
> > 1 file changed, 12 insertions(+)
> >
> > diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
> > index 159a3a604f0e..57562e20c4ca 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];
> >
> > @@ -948,6 +949,13 @@ 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,
> > + };
> > + union v4l2_ctrl_ptr p_def = {
> > + .p_area = &unit_size,
> > + };
>
> Use static const for both.
>
> I think you should add a small static inline helper function to v4l2-ctrls.h that
> takes a void pointer and returns a union v4l2_ctrl_ptr.
>
> Then you don't need to make a union v4l2_ctrl_ptr just to pass the unit_size pointer.
>
That sounds useful, but can we warantee for all the arches that
sizeof(v4l2_ctrl_ptr) <= sizeof (void *)
Of course, it sounds logic, that a union of pointers is the same size
than a pointer... but you never know.
No matter what I will make the helper and resend. with all the changes
from Jacopo
Thanks!
> Regards,
>
> Hans
>
> > int ret;
> >
> > ret = imx214_parse_fwnode(dev);
> > @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
> > V4L2_CID_EXPOSURE,
> > 0, 3184, 1, 0x0c70);
> >
> > + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
> > + NULL,
> > + V4L2_CID_UNIT_CELL_SIZE,
> > + p_def);
> > ret = imx214->ctrls.error;
> > if (ret) {
> > dev_err(&client->dev, "%s control init failed (%d)\n",
> >
>
On 9/27/19 9:33 AM, Ricardo Ribalda Delgado wrote:
> Hi Hans
>
> On Fri, 27 Sep 2019, 09:14 Hans Verkuil, <[email protected]> wrote:
>>
>> On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda Delgado <[email protected]>
>>>
>>> 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 <[email protected]>
>>> ---
>>> drivers/media/i2c/imx214.c | 12 ++++++++++++
>>> 1 file changed, 12 insertions(+)
>>>
>>> diff --git a/drivers/media/i2c/imx214.c b/drivers/media/i2c/imx214.c
>>> index 159a3a604f0e..57562e20c4ca 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];
>>>
>>> @@ -948,6 +949,13 @@ 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,
>>> + };
>>> + union v4l2_ctrl_ptr p_def = {
>>> + .p_area = &unit_size,
>>> + };
>>
>> Use static const for both.
>>
>> I think you should add a small static inline helper function to v4l2-ctrls.h that
>> takes a void pointer and returns a union v4l2_ctrl_ptr.
>>
>> Then you don't need to make a union v4l2_ctrl_ptr just to pass the unit_size pointer.
>>
>
> That sounds useful, but can we warantee for all the arches that
> sizeof(v4l2_ctrl_ptr) <= sizeof (void *)
Yes. Everything in the union is a pointer, so sizeof(v4l2_ctrl_ptr) == sizeof (void *)
Regards,
Hans
>
> Of course, it sounds logic, that a union of pointers is the same size
> than a pointer... but you never know.
>
> No matter what I will make the helper and resend. with all the changes
> from Jacopo
>
> Thanks!
>
>> Regards,
>
>
>
>>
>> Hans
>>
>>> int ret;
>>>
>>> ret = imx214_parse_fwnode(dev);
>>> @@ -1029,6 +1037,10 @@ static int imx214_probe(struct i2c_client *client)
>>> V4L2_CID_EXPOSURE,
>>> 0, 3184, 1, 0x0c70);
>>>
>>> + imx214->unit_size = v4l2_ctrl_new_std_compound(&imx214->ctrls,
>>> + NULL,
>>> + V4L2_CID_UNIT_CELL_SIZE,
>>> + p_def);
>>> ret = imx214->ctrls.error;
>>> if (ret) {
>>> dev_err(&client->dev, "%s control init failed (%d)\n",
>>>
>>