[REVIEWv3,14/35] v4l2-ctrls: prepare for matrix support.

Commit Message

Hans Verkuil Feb. 17, 2014, 9:57 a.m. UTC

From: Hans Verkuil <[e-mail]>

Add core support for matrices.

Signed-off-by: Hans Verkuil <[e-mail]>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 54 +++++++++++++++++++++++-------------
 include/media/v4l2-ctrls.h           |  8 ++++--
 2 files changed, 39 insertions(+), 23 deletions(-)

Comments

Mauro Carvalho Chehab March 12, 2014, 10:42 a.m. UTC | #1

Em Mon, 17 Feb 2014 10:57:29 +0100
Hans Verkuil <[e-mail]> escreveu:

> From: Hans Verkuil <[e-mail]>
> 
> Add core support for matrices.

Again, this patch has negative values for array index.

I'll stop analyzing here, as it is hard to keep the mind in a
sane state seeing those crazy things ;)

> 
> Signed-off-by: Hans Verkuil <[e-mail]>
> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 54 +++++++++++++++++++++++-------------
>  include/media/v4l2-ctrls.h           |  8 ++++--
>  2 files changed, 39 insertions(+), 23 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index 49ce52e..f76716e 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1132,7 +1132,7 @@ static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
>  			v4l2_event_queue_fh(sev->fh, &ev);
>  }
>  
> -static bool std_equal(const struct v4l2_ctrl *ctrl,
> +static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>  		      union v4l2_ctrl_ptr ptr1,
>  		      union v4l2_ctrl_ptr ptr2)
>  {
> @@ -1151,7 +1151,7 @@ static bool std_equal(const struct v4l2_ctrl *ctrl,
>  	}
>  }
>  
> -static void std_init(const struct v4l2_ctrl *ctrl,
> +static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
>  		     union v4l2_ctrl_ptr ptr)
>  {
>  	switch (ctrl->type) {
> @@ -1178,6 +1178,9 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>  {
>  	union v4l2_ctrl_ptr ptr = ctrl->stores[0];
>  
> +	if (ctrl->is_matrix)
> +		pr_cont("[%u][%u] ", ctrl->rows, ctrl->cols);
> +
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_INTEGER:
>  		pr_cont("%d", *ptr.p_s32);
> @@ -1220,7 +1223,7 @@ static void std_log(const struct v4l2_ctrl *ctrl)
>  })
>  
>  /* Validate a new control */
> -static int std_validate(const struct v4l2_ctrl *ctrl,
> +static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
>  			union v4l2_ctrl_ptr ptr)
>  {
>  	size_t len;
> @@ -1444,7 +1447,7 @@ static int cluster_changed(struct v4l2_ctrl *master)
>  
>  		if (ctrl == NULL)
>  			continue;
> -		ctrl->has_changed = !ctrl->type_ops->equal(ctrl,
> +		ctrl->has_changed = !ctrl->type_ops->equal(ctrl, 0,
>  						ctrl->stores[0], ctrl->new);
>  		changed |= ctrl->has_changed;
>  	}
> @@ -1502,15 +1505,15 @@ static int validate_new(const struct v4l2_ctrl *ctrl,
>  	case V4L2_CTRL_TYPE_BUTTON:
>  	case V4L2_CTRL_TYPE_CTRL_CLASS:
>  		ptr.p_s32 = &c->value;
> -		return ctrl->type_ops->validate(ctrl, ptr);
> +		return ctrl->type_ops->validate(ctrl, 0, ptr);
>  
>  	case V4L2_CTRL_TYPE_INTEGER64:
>  		ptr.p_s64 = &c->value64;
> -		return ctrl->type_ops->validate(ctrl, ptr);
> +		return ctrl->type_ops->validate(ctrl, 0, ptr);
>  
>  	default:
>  		ptr.p = c->p;
> -		return ctrl->type_ops->validate(ctrl, ptr);
> +		return ctrl->type_ops->validate(ctrl, 0, ptr);
>  	}
>  }
>  
> @@ -1736,7 +1739,8 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  			const s64 *qmenu_int, void *priv)
>  {
>  	struct v4l2_ctrl *ctrl;
> -	unsigned sz_extra;
> +	bool is_matrix;
> +	unsigned sz_extra, tot_ctrl_size;
>  	void *data;
>  	int err;
>  	int s;
> @@ -1748,6 +1752,7 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  		cols = 1;
>  	if (rows == 0)
>  		rows = 1;
> +	is_matrix = cols > 1 || rows > 1;
>  
>  	if (type == V4L2_CTRL_TYPE_INTEGER64)
>  		elem_size = sizeof(s64);
> @@ -1755,17 +1760,18 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  		elem_size = max + 1;
>  	else if (type < V4L2_CTRL_COMPLEX_TYPES)
>  		elem_size = sizeof(s32);
> +	tot_ctrl_size = elem_size * cols * rows;
>  
>  	/* Sanity checks */
> -	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
> -	    elem_size == 0 ||
> +	if (id == 0 || name == NULL || !elem_size ||
> +	    id >= V4L2_CID_PRIVATE_BASE ||
>  	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
>  	    (type == V4L2_CTRL_TYPE_INTEGER_MENU && qmenu_int == NULL)) {
>  		handler_set_err(hdl, -ERANGE);
>  		return NULL;
>  	}
>  	/* Complex controls are always hidden */
> -	if (type >= V4L2_CTRL_COMPLEX_TYPES)
> +	if (is_matrix || type >= V4L2_CTRL_COMPLEX_TYPES)
>  		flags |= V4L2_CTRL_FLAG_HIDDEN;
>  	err = check_range(type, min, max, step, def);
>  	if (err) {
> @@ -1776,14 +1782,21 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  		handler_set_err(hdl, -ERANGE);
>  		return NULL;
>  	}
> +	if (is_matrix &&
> +	    (type == V4L2_CTRL_TYPE_BUTTON ||
> +	     type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
> +		handler_set_err(hdl, -EINVAL);
> +		return NULL;
> +	}
>  
> -	sz_extra = elem_size;
> +	sz_extra = tot_ctrl_size;
>  	if (type == V4L2_CTRL_TYPE_BUTTON)
>  		flags |= V4L2_CTRL_FLAG_WRITE_ONLY;
>  	else if (type == V4L2_CTRL_TYPE_CTRL_CLASS)
>  		flags |= V4L2_CTRL_FLAG_READ_ONLY;
> -	else if (type == V4L2_CTRL_TYPE_STRING || type >= V4L2_CTRL_COMPLEX_TYPES)
> -		sz_extra += elem_size;
> +	else if (type == V4L2_CTRL_TYPE_STRING ||
> +		 type >= V4L2_CTRL_COMPLEX_TYPES || is_matrix)
> +		sz_extra += tot_ctrl_size;
>  
>  	ctrl = kzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
>  	if (ctrl == NULL) {
> @@ -1805,9 +1818,10 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	ctrl->maximum = max;
>  	ctrl->step = step;
>  	ctrl->default_value = def;
> -	ctrl->is_string = type == V4L2_CTRL_TYPE_STRING;
> -	ctrl->is_ptr = type >= V4L2_CTRL_COMPLEX_TYPES || ctrl->is_string;
> +	ctrl->is_string = !is_matrix && type == V4L2_CTRL_TYPE_STRING;
> +	ctrl->is_ptr = is_matrix || type >= V4L2_CTRL_COMPLEX_TYPES || ctrl->is_string;
>  	ctrl->is_int = !ctrl->is_ptr && type != V4L2_CTRL_TYPE_INTEGER64;
> +	ctrl->is_matrix = is_matrix;
>  	ctrl->cols = cols;
>  	ctrl->rows = rows;
>  	ctrl->elem_size = elem_size;
> @@ -1821,13 +1835,13 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  
>  	if (ctrl->is_ptr) {
>  		for (s = -1; s <= 0; s++)
> -			ctrl->stores[s].p = data + (s + 1) * elem_size;
> +			ctrl->stores[s].p = data + (s + 1) * tot_ctrl_size;
>  	} else {
>  		ctrl->new.p = &ctrl->val;
>  		ctrl->stores[0].p = data;
>  	}
>  	for (s = -1; s <= 0; s++)
> -		ctrl->type_ops->init(ctrl, ctrl->stores[s]);
> +		ctrl->type_ops->init(ctrl, 0, ctrl->stores[s]);
>  
>  	if (handler_new_ref(hdl, ctrl)) {
>  		kfree(ctrl);
> @@ -2734,7 +2748,7 @@ s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
>  	struct v4l2_ext_control c;
>  
>  	/* It's a driver bug if this happens. */
> -	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
> +	WARN_ON(ctrl->is_ptr || ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
>  	c.value = 0;
>  	get_ctrl(ctrl, &c);
>  	return c.value;
> @@ -3044,7 +3058,7 @@ int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
>  	struct v4l2_ext_control c;
>  
>  	/* It's a driver bug if this happens. */
> -	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
> +	WARN_ON(ctrl->is_ptr || ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
>  	c.value64 = val;
>  	return set_ctrl_lock(NULL, ctrl, &c);
>  }
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index 1b06930..7d72328 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -74,13 +74,13 @@ struct v4l2_ctrl_ops {
>    * @validate: validate the value. Return 0 on success and a negative value otherwise.
>    */
>  struct v4l2_ctrl_type_ops {
> -	bool (*equal)(const struct v4l2_ctrl *ctrl,
> +	bool (*equal)(const struct v4l2_ctrl *ctrl, u32 idx,
>  		      union v4l2_ctrl_ptr ptr1,
>  		      union v4l2_ctrl_ptr ptr2);
> -	void (*init)(const struct v4l2_ctrl *ctrl,
> +	void (*init)(const struct v4l2_ctrl *ctrl, u32 idx,
>  		     union v4l2_ctrl_ptr ptr);
>  	void (*log)(const struct v4l2_ctrl *ctrl);
> -	int (*validate)(const struct v4l2_ctrl *ctrl,
> +	int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
>  			union v4l2_ctrl_ptr ptr);
>  };
>  
> @@ -111,6 +111,7 @@ typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
>    * @is_ptr:	If set, then this control is a matrix and/or has type >= V4L2_CTRL_COMPLEX_TYPES
>    *		and/or has type V4L2_CTRL_TYPE_STRING. In other words, struct
>    *		v4l2_ext_control uses field p to point to the data.
> +  * @is_matrix: If set, then this control contains a matrix.
>    * @has_volatiles: If set, then one or more members of the cluster are volatile.
>    *		Drivers should never touch this flag.
>    * @call_notify: If set, then call the handler's notify function whenever the
> @@ -169,6 +170,7 @@ struct v4l2_ctrl {
>  	unsigned int is_int:1;
>  	unsigned int is_string:1;
>  	unsigned int is_ptr:1;
> +	unsigned int is_matrix:1;
>  	unsigned int has_volatiles:1;
>  	unsigned int call_notify:1;
>  	unsigned int manual_mode_value:8;

Hans Verkuil March 12, 2014, 12:21 p.m. UTC | #2

On 03/12/14 11:42, Mauro Carvalho Chehab wrote:
> Em Mon, 17 Feb 2014 10:57:29 +0100
> Hans Verkuil <[e-mail]> escreveu:
> 
>> From: Hans Verkuil <[e-mail]>
>>
>> Add core support for matrices.
> 
> Again, this patch has negative values for array index.
> 
> I'll stop analyzing here, as it is hard to keep the mind in a
> sane state seeing those crazy things ;)

Rather than getting bogged down in these details can you please give
your opinion on the public API aspects. There is no point for me to
spend time on this and then get it NACKed because you don't like the
API itself and want something completely different.

Internal things I can change, but I'm not going to spend a second on
that unless I know the concept stands. Otherwise it is wasted time.

This is something we need to improve on with regards to our
processes: when it comes to API enhancements you really need to be
involved earlier or it's going to be a huge waste of everyones time
it is gets NACked. Not to mention demotivating and frustrating for
all concerned.

Regards,

	Hans
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Mauro Carvalho Chehab March 12, 2014, 1 p.m. UTC | #3

Em Wed, 12 Mar 2014 13:21:41 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 03/12/14 11:42, Mauro Carvalho Chehab wrote:
> > Em Mon, 17 Feb 2014 10:57:29 +0100
> > Hans Verkuil <[e-mail]> escreveu:
> > 
> >> From: Hans Verkuil <[e-mail]>
> >>
> >> Add core support for matrices.
> > 
> > Again, this patch has negative values for array index.
> > 
> > I'll stop analyzing here, as it is hard to keep the mind in a
> > sane state seeing those crazy things ;)
> 
> Rather than getting bogged down in these details can you please give
> your opinion on the public API aspects. There is no point for me to
> spend time on this and then get it NACKed because you don't like the
> API itself and want something completely different.
> 
> Internal things I can change, but I'm not going to spend a second on
> that unless I know the concept stands. Otherwise it is wasted time.

Ok, what patches after 16/35 contains the API bits?

The changes I saw so far seem ok, with the adjustments I pointed.

> This is something we need to improve on with regards to our
> processes: when it comes to API enhancements you really need to be
> involved earlier or it's going to be a huge waste of everyones time
> it is gets NACked. Not to mention demotivating and frustrating for
> all concerned.

As I commented before: those complex API changes should ideally
be discussed during our mini-summits, as it allows us to better
understand the hole proposal.

Mauro Carvalho Chehab March 12, 2014, 1 p.m. UTC | #4

Em Wed, 12 Mar 2014 13:21:41 +0100
Hans Verkuil <[e-mail]> escreveu:

> On 03/12/14 11:42, Mauro Carvalho Chehab wrote:
> > Em Mon, 17 Feb 2014 10:57:29 +0100
> > Hans Verkuil <[e-mail]> escreveu:
> > 
> >> From: Hans Verkuil <[e-mail]>
> >>
> >> Add core support for matrices.
> > 
> > Again, this patch has negative values for array index.
> > 
> > I'll stop analyzing here, as it is hard to keep the mind in a
> > sane state seeing those crazy things ;)
> 
> Rather than getting bogged down in these details can you please give
> your opinion on the public API aspects. There is no point for me to
> spend time on this and then get it NACKed because you don't like the
> API itself and want something completely different.
> 
> Internal things I can change, but I'm not going to spend a second on
> that unless I know the concept stands. Otherwise it is wasted time.

Ok, what patches after 16/35 contains the API bits?

The changes I saw so far seem ok, with the adjustments I pointed.

> This is something we need to improve on with regards to our
> processes: when it comes to API enhancements you really need to be
> involved earlier or it's going to be a huge waste of everyones time
> it is gets NACked. Not to mention demotivating and frustrating for
> all concerned.

As I commented before: those complex API changes should ideally
be discussed during our mini-summits, as it allows us to better
understand the hole proposal and the taken approach.

Hans Verkuil March 12, 2014, 1:41 p.m. UTC | #5

On 03/12/2014 02:00 PM, Mauro Carvalho Chehab wrote:
> Em Wed, 12 Mar 2014 13:21:41 +0100
> Hans Verkuil <[e-mail]> escreveu:
> 
>> On 03/12/14 11:42, Mauro Carvalho Chehab wrote:
>>> Em Mon, 17 Feb 2014 10:57:29 +0100
>>> Hans Verkuil <[e-mail]> escreveu:
>>>
>>>> From: Hans Verkuil <[e-mail]>
>>>>
>>>> Add core support for matrices.
>>>
>>> Again, this patch has negative values for array index.
>>>
>>> I'll stop analyzing here, as it is hard to keep the mind in a
>>> sane state seeing those crazy things ;)
>>
>> Rather than getting bogged down in these details can you please give
>> your opinion on the public API aspects. There is no point for me to
>> spend time on this and then get it NACKed because you don't like the
>> API itself and want something completely different.
>>
>> Internal things I can change, but I'm not going to spend a second on
>> that unless I know the concept stands. Otherwise it is wasted time.
> 
> Ok, what patches after 16/35 contains the API bits?

Core control functionality: 4, 5, 19-22.

Adding support for the motion detection matrices: 24, 25, 27, 28.

Adding support for the motion detection event: 29, 30.

Sorry for sounding so irritated in my email: today is one of those
frustrating days where nothing works out the way you want it and
where I should have stayed in bed in the morning, and that spilled
over in my mail.

Regards,

	Hans

> 
> The changes I saw so far seem ok, with the adjustments I pointed.
> 
>> This is something we need to improve on with regards to our
>> processes: when it comes to API enhancements you really need to be
>> involved earlier or it's going to be a huge waste of everyones time
>> it is gets NACked. Not to mention demotivating and frustrating for
>> all concerned.
> 
> As I commented before: those complex API changes should ideally
> be discussed during our mini-summits, as it allows us to better
> understand the hole proposal and the taken approach.
> 

--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Sylwester Nawrocki/Kernel \(PLT\) /SRPOL/Staff Engineer/Samsung Electronics March 12, 2014, 1:44 p.m. UTC | #6

Hi Mauro,

On 12/03/14 14:00, Mauro Carvalho Chehab wrote:
[...]
> As I commented before: those complex API changes should ideally
> be discussed during our mini-summits, as it allows us to better
> understand the hole proposal and the taken approach.

We discussed this in a dedicated brainstorming meeting in Edinburgh,
with Sakari, Laurent, Andrzej. IIRC you didn't take part in that
discussions for some reason.
After we initially agreed on the API Hans started working on the
actual implementation. He put much effort and did a pretty good
job IMO. I guess we just need to refine any controversial aspects
that might be there and move forward with the API. :)

--
Regards,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to [e-mail]
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Patch

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 49ce52e..f76716e 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1132,7 +1132,7 @@  static void send_event(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 changes)
 			v4l2_event_queue_fh(sev->fh, &ev);
 }
 
-static bool std_equal(const struct v4l2_ctrl *ctrl,
+static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
 		      union v4l2_ctrl_ptr ptr1,
 		      union v4l2_ctrl_ptr ptr2)
 {
@@ -1151,7 +1151,7 @@  static bool std_equal(const struct v4l2_ctrl *ctrl,
 	}
 }
 
-static void std_init(const struct v4l2_ctrl *ctrl,
+static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
 		     union v4l2_ctrl_ptr ptr)
 {
 	switch (ctrl->type) {
@@ -1178,6 +1178,9 @@  static void std_log(const struct v4l2_ctrl *ctrl)
 {
 	union v4l2_ctrl_ptr ptr = ctrl->stores[0];
 
+	if (ctrl->is_matrix)
+		pr_cont("[%u][%u] ", ctrl->rows, ctrl->cols);
+
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_INTEGER:
 		pr_cont("%d", *ptr.p_s32);
@@ -1220,7 +1223,7 @@  static void std_log(const struct v4l2_ctrl *ctrl)
 })
 
 /* Validate a new control */
-static int std_validate(const struct v4l2_ctrl *ctrl,
+static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 			union v4l2_ctrl_ptr ptr)
 {
 	size_t len;
@@ -1444,7 +1447,7 @@  static int cluster_changed(struct v4l2_ctrl *master)
 
 		if (ctrl == NULL)
 			continue;
-		ctrl->has_changed = !ctrl->type_ops->equal(ctrl,
+		ctrl->has_changed = !ctrl->type_ops->equal(ctrl, 0,
 						ctrl->stores[0], ctrl->new);
 		changed |= ctrl->has_changed;
 	}
@@ -1502,15 +1505,15 @@  static int validate_new(const struct v4l2_ctrl *ctrl,
 	case V4L2_CTRL_TYPE_BUTTON:
 	case V4L2_CTRL_TYPE_CTRL_CLASS:
 		ptr.p_s32 = &c->value;
-		return ctrl->type_ops->validate(ctrl, ptr);
+		return ctrl->type_ops->validate(ctrl, 0, ptr);
 
 	case V4L2_CTRL_TYPE_INTEGER64:
 		ptr.p_s64 = &c->value64;
-		return ctrl->type_ops->validate(ctrl, ptr);
+		return ctrl->type_ops->validate(ctrl, 0, ptr);
 
 	default:
 		ptr.p = c->p;
-		return ctrl->type_ops->validate(ctrl, ptr);
+		return ctrl->type_ops->validate(ctrl, 0, ptr);
 	}
 }
 
@@ -1736,7 +1739,8 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			const s64 *qmenu_int, void *priv)
 {
 	struct v4l2_ctrl *ctrl;
-	unsigned sz_extra;
+	bool is_matrix;
+	unsigned sz_extra, tot_ctrl_size;
 	void *data;
 	int err;
 	int s;
@@ -1748,6 +1752,7 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		cols = 1;
 	if (rows == 0)
 		rows = 1;
+	is_matrix = cols > 1 || rows > 1;
 
 	if (type == V4L2_CTRL_TYPE_INTEGER64)
 		elem_size = sizeof(s64);
@@ -1755,17 +1760,18 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		elem_size = max + 1;
 	else if (type < V4L2_CTRL_COMPLEX_TYPES)
 		elem_size = sizeof(s32);
+	tot_ctrl_size = elem_size * cols * rows;
 
 	/* Sanity checks */
-	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
-	    elem_size == 0 ||
+	if (id == 0 || name == NULL || !elem_size ||
+	    id >= V4L2_CID_PRIVATE_BASE ||
 	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
 	    (type == V4L2_CTRL_TYPE_INTEGER_MENU && qmenu_int == NULL)) {
 		handler_set_err(hdl, -ERANGE);
 		return NULL;
 	}
 	/* Complex controls are always hidden */
-	if (type >= V4L2_CTRL_COMPLEX_TYPES)
+	if (is_matrix || type >= V4L2_CTRL_COMPLEX_TYPES)
 		flags |= V4L2_CTRL_FLAG_HIDDEN;
 	err = check_range(type, min, max, step, def);
 	if (err) {
@@ -1776,14 +1782,21 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		handler_set_err(hdl, -ERANGE);
 		return NULL;
 	}
+	if (is_matrix &&
+	    (type == V4L2_CTRL_TYPE_BUTTON ||
+	     type == V4L2_CTRL_TYPE_CTRL_CLASS)) {
+		handler_set_err(hdl, -EINVAL);
+		return NULL;
+	}
 
-	sz_extra = elem_size;
+	sz_extra = tot_ctrl_size;
 	if (type == V4L2_CTRL_TYPE_BUTTON)
 		flags |= V4L2_CTRL_FLAG_WRITE_ONLY;
 	else if (type == V4L2_CTRL_TYPE_CTRL_CLASS)
 		flags |= V4L2_CTRL_FLAG_READ_ONLY;
-	else if (type == V4L2_CTRL_TYPE_STRING || type >= V4L2_CTRL_COMPLEX_TYPES)
-		sz_extra += elem_size;
+	else if (type == V4L2_CTRL_TYPE_STRING ||
+		 type >= V4L2_CTRL_COMPLEX_TYPES || is_matrix)
+		sz_extra += tot_ctrl_size;
 
 	ctrl = kzalloc(sizeof(*ctrl) + sz_extra, GFP_KERNEL);
 	if (ctrl == NULL) {
@@ -1805,9 +1818,10 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	ctrl->maximum = max;
 	ctrl->step = step;
 	ctrl->default_value = def;
-	ctrl->is_string = type == V4L2_CTRL_TYPE_STRING;
-	ctrl->is_ptr = type >= V4L2_CTRL_COMPLEX_TYPES || ctrl->is_string;
+	ctrl->is_string = !is_matrix && type == V4L2_CTRL_TYPE_STRING;
+	ctrl->is_ptr = is_matrix || type >= V4L2_CTRL_COMPLEX_TYPES || ctrl->is_string;
 	ctrl->is_int = !ctrl->is_ptr && type != V4L2_CTRL_TYPE_INTEGER64;
+	ctrl->is_matrix = is_matrix;
 	ctrl->cols = cols;
 	ctrl->rows = rows;
 	ctrl->elem_size = elem_size;
@@ -1821,13 +1835,13 @@  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 
 	if (ctrl->is_ptr) {
 		for (s = -1; s <= 0; s++)
-			ctrl->stores[s].p = data + (s + 1) * elem_size;
+			ctrl->stores[s].p = data + (s + 1) * tot_ctrl_size;
 	} else {
 		ctrl->new.p = &ctrl->val;
 		ctrl->stores[0].p = data;
 	}
 	for (s = -1; s <= 0; s++)
-		ctrl->type_ops->init(ctrl, ctrl->stores[s]);
+		ctrl->type_ops->init(ctrl, 0, ctrl->stores[s]);
 
 	if (handler_new_ref(hdl, ctrl)) {
 		kfree(ctrl);
@@ -2734,7 +2748,7 @@  s64 v4l2_ctrl_g_ctrl_int64(struct v4l2_ctrl *ctrl)
 	struct v4l2_ext_control c;
 
 	/* It's a driver bug if this happens. */
-	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	WARN_ON(ctrl->is_ptr || ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
 	c.value = 0;
 	get_ctrl(ctrl, &c);
 	return c.value;
@@ -3044,7 +3058,7 @@  int v4l2_ctrl_s_ctrl_int64(struct v4l2_ctrl *ctrl, s64 val)
 	struct v4l2_ext_control c;
 
 	/* It's a driver bug if this happens. */
-	WARN_ON(ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
+	WARN_ON(ctrl->is_ptr || ctrl->type != V4L2_CTRL_TYPE_INTEGER64);
 	c.value64 = val;
 	return set_ctrl_lock(NULL, ctrl, &c);
 }
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 1b06930..7d72328 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -74,13 +74,13 @@  struct v4l2_ctrl_ops {
   * @validate: validate the value. Return 0 on success and a negative value otherwise.
   */
 struct v4l2_ctrl_type_ops {
-	bool (*equal)(const struct v4l2_ctrl *ctrl,
+	bool (*equal)(const struct v4l2_ctrl *ctrl, u32 idx,
 		      union v4l2_ctrl_ptr ptr1,
 		      union v4l2_ctrl_ptr ptr2);
-	void (*init)(const struct v4l2_ctrl *ctrl,
+	void (*init)(const struct v4l2_ctrl *ctrl, u32 idx,
 		     union v4l2_ctrl_ptr ptr);
 	void (*log)(const struct v4l2_ctrl *ctrl);
-	int (*validate)(const struct v4l2_ctrl *ctrl,
+	int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
 			union v4l2_ctrl_ptr ptr);
 };
 
@@ -111,6 +111,7 @@  typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
   * @is_ptr:	If set, then this control is a matrix and/or has type >= V4L2_CTRL_COMPLEX_TYPES
   *		and/or has type V4L2_CTRL_TYPE_STRING. In other words, struct
   *		v4l2_ext_control uses field p to point to the data.
+  * @is_matrix: If set, then this control contains a matrix.
   * @has_volatiles: If set, then one or more members of the cluster are volatile.
   *		Drivers should never touch this flag.
   * @call_notify: If set, then call the handler's notify function whenever the
@@ -169,6 +170,7 @@  struct v4l2_ctrl {
 	unsigned int is_int:1;
 	unsigned int is_string:1;
 	unsigned int is_ptr:1;
+	unsigned int is_matrix:1;
 	unsigned int has_volatiles:1;
 	unsigned int call_notify:1;
 	unsigned int manual_mode_value:8;