[PATCH v6 0/7] Implement UNIT_CELL_SIZE control

[PATCH v6 0/7] Implement UNIT_CELL_SIZE control

[Ricardo Ribalda Delgado]

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

[PATCH v6 1/7] media: v4l2-core: Implement v4l2_ctrl_new_std_compound

[Ricardo Ribalda Delgado]

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 <hverkuil@xs4all.nl>
Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
---
 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

Re: [PATCH v6 1/7] media: v4l2-core: Implement v4l2_ctrl_new_std_compound

[Jacopo Mondi]

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

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 <hverkuil@xs4all.nl>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
> ---
>  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
>

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

Re: [PATCH v6 1/7] media: v4l2-core: Implement v4l2_ctrl_new_std_compound

[Ricardo Ribalda Delgado]

Hi Jacopo

Thanks for your review!

On Wed, Sep 25, 2019 at 10:18 AM Jacopo Mondi <jacopo@jmondi.org> 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 <hverkuil@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
> > ---
> >  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

[PATCH v6 2/7] Documentation: v4l2_ctrl_new_std_compound

[Ricardo Ribalda Delgado]

Function for initializing compound controls with a default value.

Suggested-by: Hans Verkuil <hverkuil@xs4all.nl>
Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
---
 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

Re: [PATCH v6 2/7] Documentation: v4l2_ctrl_new_std_compound

[Jacopo Mondi]

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

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 <hverkuil@xs4all.nl>
> Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
> ---
>  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 <jacopo@jmondi.org>

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
>

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

Re: [PATCH v6 2/7] Documentation: v4l2_ctrl_new_std_compound

[Ricardo Ribalda Delgado]

Hi Jacopo
On Wed, Sep 25, 2019 at 10:20 AM Jacopo Mondi <jacopo@jmondi.org> 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 <hverkuil@xs4all.nl>
> > Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
> > ---
> >  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 <jacopo@jmondi.org>

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

[PATCH v6 3/7] media: add V4L2_CTRL_TYPE_AREA control type

[Ricardo Ribalda Delgado]

This type contains the width and the height of a rectangular area.

Signed-off-by: Ricardo Ribalda Delgado <ricardo@ribalda.com>
---
 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

Re: [PATCH v6 3/7] media: add V4L2_CTRL_TYPE_AREA control type

[Jacopo Mondi]

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

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 <ricardo@ribalda.com>
> ---
>  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 <jacopo@jmondi.org>

Thanks
   j


>  };
>
>  /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
> --
> 2.23.0
>

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

[PATCH v6 4/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

[Ricardo Ribalda Delgado]

From: Ricardo Ribalda Delgado <ribalda@kernel.org>

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

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 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

Re: [PATCH v6 4/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

[Jacopo Mondi]

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

Hi Ricardo,

On Fri, Sep 20, 2019 at 03:51:34PM +0200, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <ribalda@kernel.org>
>
> A struct v4l2_area containing the width and the height of a rectangular
> area.
>
> Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> ---
>  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 <jacopo@jmondi.org>

Thanks
   j
>      * - ``V4L2_CTRL_TYPE_H264_SPS``
>        - n/a
>        - n/a
> --
> 2.23.0
>

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

Re: [PATCH v6 4/7] Documentation: media: Document V4L2_CTRL_TYPE_AREA

[Ricardo Ribalda Delgado]

Hi Jacopo

On Wed, Sep 25, 2019 at 10:32 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:34PM +0200, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <ribalda@kernel.org>
> >
> > A struct v4l2_area containing the width and the height of a rectangular
> > area.
> >
> > Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
> > Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
> > ---
> >  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/linux-media@vger.kernel.org/msg150103.html


> Really a minor issue from my side, feel free to keep it the way it is
> Reviewed-by: Jacopo Mondi <jacopo@jmondi.org>
>
> Thanks
>    j
> >      * - ``V4L2_CTRL_TYPE_H264_SPS``
> >        - n/a
> >        - n/a
> > --
> > 2.23.0
> >



-- 
Ricardo Ribalda

[PATCH v6 5/7] media: add V4L2_CID_UNIT_CELL_SIZE control

[Ricardo Ribalda Delgado]

From: Ricardo Ribalda Delgado <ribalda@kernel.org>

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 <p.zabel@pengutronix.de>
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 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

[PATCH v6 6/7] Documentation: media: Describe V4L2_CID_UNIT_CELL_SIZE

[Ricardo Ribalda Delgado]

From: Ricardo Ribalda Delgado <ribalda@kernel.org>

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

Reviewed-by: Philipp Zabel <p.zabel@pengutronix.de>
Signed-off-by: Ricardo Ribalda Delgado <ribalda@kernel.org>
---
 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

[PATCH v6 7/7] media: imx214: Add new control with V4L2_CID_UNIT_CELL_SIZE

[Ricardo Ribalda Delgado]

From: Ricardo Ribalda Delgado <ribalda@kernel.org>

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 | 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

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

[Jacopo Mondi]

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

Hi Ricardo,

On Fri, Sep 20, 2019 at 03:51:37PM +0200, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <ribalda@kernel.org>
>
> 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 | 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
>

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

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

[Ricardo Ribalda Delgado]

Hi Jacopo

Thanks for the review

On Wed, Sep 25, 2019 at 11:24 AM Jacopo Mondi <jacopo@jmondi.org> wrote:
>
> Hi Ricardo,
>
> On Fri, Sep 20, 2019 at 03:51:37PM +0200, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <ribalda@kernel.org>
> >
> > 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 | 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/linux-media@vger.kernel.org/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

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

[Hans Verkuil]

On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
> From: Ricardo Ribalda Delgado <ribalda@kernel.org>
> 
> 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 | 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",
> 

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

[Ricardo Ribalda Delgado]

Hi Hans

On Fri, 27 Sep 2019, 09:14 Hans Verkuil, <hverkuil-cisco@xs4all.nl> wrote:
>
> On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
> > From: Ricardo Ribalda Delgado <ribalda@kernel.org>
> >
> > 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 | 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",
> >
>

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

[Hans Verkuil]

On 9/27/19 9:33 AM, Ricardo Ribalda Delgado wrote:
> Hi Hans
> 
> On Fri, 27 Sep 2019, 09:14 Hans Verkuil, <hverkuil-cisco@xs4all.nl> wrote:
>>
>> On 9/20/19 3:51 PM, Ricardo Ribalda Delgado wrote:
>>> From: Ricardo Ribalda Delgado <ribalda@kernel.org>
>>>
>>> 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 | 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",
>>>
>>