[PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support

[PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support

[Laurent Pinchart]

Hello,

This patch set adds support for exposing the OMAP4 ISS IPIPE RGB2RGB blending
matrix through V4L2 controls, using the compound controls support.

Patches 1 to 4 add new signed compound control types and simplify the control
type init operation. Patches 5 then fixes an issue with the omap4iss driver,
and patch 6 finally adds RGB2RGB blending matrix support.

Please see individual patches for changes since v1.

Laurent Pinchart (6):
  v4l2-ctrls: Add new S8, S16 and S32 compound control types
  v4l2-ctrls: Don't initialize array tail when setting a control
  v4l2-ctrls: Make the control type init op initialize the whole control
  v4l2-ctrls: Export the standard control type operations
  staging: media: omap4iss: Cleanup media entities after unregistration
  staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix

 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       |  21 ++++
 .../DocBook/media/v4l/vidioc-queryctrl.xml         |  30 +++++
 drivers/media/v4l2-core/v4l2-ctrls.c               | 103 ++++++++++++----
 drivers/staging/media/omap4iss/iss_ipipe.c         | 135 ++++++++++++++++++++-
 drivers/staging/media/omap4iss/iss_ipipe.h         |  17 +++
 drivers/staging/media/omap4iss/iss_ipipeif.c       |   6 +-
 drivers/staging/media/omap4iss/iss_resizer.c       |   6 +-
 include/media/v4l2-ctrls.h                         |  16 ++-
 include/uapi/linux/videodev2.h                     |   6 +
 9 files changed, 301 insertions(+), 39 deletions(-)

-- 
Regards,

Laurent Pinchart

[PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types

[Laurent Pinchart]

Only unsigned compound types are implemented so far, add the
corresponding signes types.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 .../DocBook/media/v4l/vidioc-g-ext-ctrls.xml       | 21 +++++++++++++++
 .../DocBook/media/v4l/vidioc-queryctrl.xml         | 30 ++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c               | 30 ++++++++++++++++++++++
 include/media/v4l2-ctrls.h                         |  4 +++
 include/uapi/linux/videodev2.h                     |  6 +++++
 5 files changed, 91 insertions(+)

diff --git a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
index c5bdbfc..845087e 100644
--- a/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-g-ext-ctrls.xml
@@ -186,6 +186,27 @@ type <constant>V4L2_CTRL_TYPE_STRING</constant>.</entry>
 	  </row>
 	  <row>
 	    <entry></entry>
+	    <entry>__s8 *</entry>
+	    <entry><structfield>p_s8</structfield></entry>
+	    <entry>A pointer to a matrix control of signed 8-bit values.
+Valid if this control is of type <constant>V4L2_CTRL_TYPE_S8</constant>.</entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__s16 *</entry>
+	    <entry><structfield>p_s16</structfield></entry>
+	    <entry>A pointer to a matrix control of signed 16-bit values.
+Valid if this control is of type <constant>V4L2_CTRL_TYPE_S16</constant>.</entry>
+	  </row>
+	  <row>
+	    <entry></entry>
+	    <entry>__s32 *</entry>
+	    <entry><structfield>p_s32</structfield></entry>
+	    <entry>A pointer to a matrix control of signed 32-bit values.
+Valid if this control is of type <constant>V4L2_CTRL_TYPE_S32</constant>.</entry>
+	  </row>
+	  <row>
+	    <entry></entry>
 	    <entry>__u8 *</entry>
 	    <entry><structfield>p_u8</structfield></entry>
 	    <entry>A pointer to a matrix control of unsigned 8-bit values.
diff --git a/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml b/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml
index 2bd98fd..293e225 100644
--- a/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml
+++ b/Documentation/DocBook/media/v4l/vidioc-queryctrl.xml
@@ -512,6 +512,36 @@ Older drivers which do not support this feature return an
 &EINVAL;.</entry>
 	  </row>
 	  <row>
+	    <entry><constant>V4L2_CTRL_TYPE_S8</constant></entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>A signed 8-bit valued control ranging from minimum to
+maximum inclusive. The step value indicates the increment between
+values which are actually different on the hardware.
+</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_CTRL_TYPE_S16</constant></entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>A signed 16-bit valued control ranging from minimum to
+maximum inclusive. The step value indicates the increment between
+values which are actually different on the hardware.
+</entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_CTRL_TYPE_S32</constant></entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>any</entry>
+	    <entry>A signed 32-bit valued control ranging from minimum to
+maximum inclusive. The step value indicates the increment between
+values which are actually different on the hardware.
+</entry>
+	  </row>
+	  <row>
 	    <entry><constant>V4L2_CTRL_TYPE_U8</constant></entry>
 	    <entry>any</entry>
 	    <entry>any</entry>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 45c5b47..301abb7 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1247,10 +1247,13 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_INTEGER64:
 		return ptr1.p_s64[idx] == ptr2.p_s64[idx];
 	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_S8:
 		return ptr1.p_u8[idx] == ptr2.p_u8[idx];
 	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_S16:
 		return ptr1.p_u16[idx] == ptr2.p_u16[idx];
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S32:
 		return ptr1.p_u32[idx] == ptr2.p_u32[idx];
 	default:
 		if (ctrl->is_int)
@@ -1280,12 +1283,15 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
 		ptr.p_s32[idx] = ctrl->default_value;
 		break;
 	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_S8:
 		ptr.p_u8[idx] = ctrl->default_value;
 		break;
 	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_S16:
 		ptr.p_u16[idx] = ctrl->default_value;
 		break;
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S32:
 		ptr.p_u32[idx] = ctrl->default_value;
 		break;
 	default:
@@ -1338,6 +1344,15 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 	case V4L2_CTRL_TYPE_U32:
 		pr_cont("%u", (unsigned)*ptr.p_u32);
 		break;
+	case V4L2_CTRL_TYPE_S8:
+		pr_cont("%d", (int)*ptr.p_s8);
+		break;
+	case V4L2_CTRL_TYPE_S16:
+		pr_cont("%d", (int)*ptr.p_s16);
+		break;
+	case V4L2_CTRL_TYPE_S32:
+		pr_cont("%d", (int)*ptr.p_s32);
+		break;
 	default:
 		pr_cont("unknown type %d", ctrl->type);
 		break;
@@ -1397,6 +1412,12 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 		return ROUND_TO_RANGE(ptr.p_u16[idx], u16, ctrl);
 	case V4L2_CTRL_TYPE_U32:
 		return ROUND_TO_RANGE(ptr.p_u32[idx], u32, ctrl);
+	case V4L2_CTRL_TYPE_S8:
+		return ROUND_TO_RANGE(ptr.p_s8[idx], s8, ctrl);
+	case V4L2_CTRL_TYPE_S16:
+		return ROUND_TO_RANGE(ptr.p_s16[idx], s16, ctrl);
+	case V4L2_CTRL_TYPE_S32:
+		return ROUND_TO_RANGE(ptr.p_s32[idx], s32, ctrl);
 
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		ptr.p_s32[idx] = !!ptr.p_s32[idx];
@@ -1630,6 +1651,9 @@ static int check_range(enum v4l2_ctrl_type type,
 	case V4L2_CTRL_TYPE_U8:
 	case V4L2_CTRL_TYPE_U16:
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S8:
+	case V4L2_CTRL_TYPE_S16:
+	case V4L2_CTRL_TYPE_S32:
 	case V4L2_CTRL_TYPE_INTEGER:
 	case V4L2_CTRL_TYPE_INTEGER64:
 		if (step == 0 || min > max || def < min || def > max)
@@ -1933,12 +1957,15 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 		elem_size = max + 1;
 		break;
 	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_S8:
 		elem_size = sizeof(u8);
 		break;
 	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_S16:
 		elem_size = sizeof(u16);
 		break;
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S32:
 		elem_size = sizeof(u32);
 		break;
 	default:
@@ -3312,6 +3339,9 @@ int __v4l2_ctrl_modify_range(struct v4l2_ctrl *ctrl,
 	case V4L2_CTRL_TYPE_U8:
 	case V4L2_CTRL_TYPE_U16:
 	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S8:
+	case V4L2_CTRL_TYPE_S16:
+	case V4L2_CTRL_TYPE_S32:
 		if (ctrl->is_array)
 			return -EINVAL;
 		ret = check_range(ctrl->type, min, max, step, def);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index 911f3e5..e1cfb8f 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -37,6 +37,8 @@ struct v4l2_fh;
 struct poll_table_struct;
 
 /** union v4l2_ctrl_ptr - A pointer to a control value.
+ * @p_s8:	Pointer to a 8-bit signed value.
+ * @p_s16:	Pointer to a 16-bit signed value.
  * @p_s32:	Pointer to a 32-bit signed value.
  * @p_s64:	Pointer to a 64-bit signed value.
  * @p_u8:	Pointer to a 8-bit unsigned value.
@@ -46,6 +48,8 @@ struct poll_table_struct;
  * @p:		Pointer to a compound value.
  */
 union v4l2_ctrl_ptr {
+	s8 *p_s8;
+	s16 *p_s16;
 	s32 *p_s32;
 	s64 *p_s64;
 	u8 *p_u8;
diff --git a/include/uapi/linux/videodev2.h b/include/uapi/linux/videodev2.h
index fbdc360..9f51535 100644
--- a/include/uapi/linux/videodev2.h
+++ b/include/uapi/linux/videodev2.h
@@ -1370,6 +1370,9 @@ struct v4l2_ext_control {
 		__u8 __user *p_u8;
 		__u16 __user *p_u16;
 		__u32 __user *p_u32;
+		__s8 __user *p_s8;
+		__s16 __user *p_s16;
+		__s32 __user *p_s32;
 		void __user *ptr;
 	};
 } __attribute__ ((packed));
@@ -1403,6 +1406,9 @@ enum v4l2_ctrl_type {
 	V4L2_CTRL_TYPE_U8	     = 0x0100,
 	V4L2_CTRL_TYPE_U16	     = 0x0101,
 	V4L2_CTRL_TYPE_U32	     = 0x0102,
+	V4L2_CTRL_TYPE_S8	     = 0x0103,
+	V4L2_CTRL_TYPE_S16	     = 0x0104,
+	V4L2_CTRL_TYPE_S32	     = 0x0105,
 };
 
 /*  Used in the VIDIOC_QUERYCTRL ioctl for querying controls */
-- 
2.0.5

[PATCH v2 2/6] v4l2-ctrls: Don't initialize array tail when setting a control

[Laurent Pinchart]

Setting an array control subset isn't allowed by the control framework,
which returns an error in prepare_ext_ctrls() if the control size
specified by userspace is smaller than the total size. There is thus no
need to initialize the array tail to its default value, as the tail will
always be empty.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 4 ----
 1 file changed, 4 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 301abb7..adac93e 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1518,13 +1518,9 @@ static int user_to_ptr(struct v4l2_ext_control *c,
 
 	ctrl->is_new = 1;
 	if (ctrl->is_ptr && !ctrl->is_string) {
-		unsigned idx;
-
 		ret = copy_from_user(ptr.p, c->ptr, c->size) ? -EFAULT : 0;
 		if (ret || !ctrl->is_array)
 			return ret;
-		for (idx = c->size / ctrl->elem_size; idx < ctrl->elems; idx++)
-			ctrl->type_ops->init(ctrl, idx, ptr);
 		return 0;
 	}
 
-- 
2.0.5

[PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control

[Laurent Pinchart]

The control type init operation is called in a loop to initialize all
elements of a control. Not only is this inefficient for control types
that could use a memset(), it also complicates the implementation of
custom control types, for instance when a matrix needs to be initialized
with different values for its elements.

Make the init operation initialize the whole control instead, and use
memset() when possible.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 46 ++++++++++++++++++++++++++----------
 include/media/v4l2-ctrls.h           |  3 +--
 2 files changed, 34 insertions(+), 15 deletions(-)

Changes since v1:

- Remove support for V4L2_CTRL_TYPE_U8 and V4L2_CTRL_TYPE_S8 from
  std_init_one(), as those cases are now handled by std_init()

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index adac93e..81b8e66 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1263,8 +1263,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
 	}
 }
 
-static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
-		     union v4l2_ctrl_ptr ptr)
+static void std_init_one(const struct v4l2_ctrl *ctrl, u32 idx,
+			 union v4l2_ctrl_ptr ptr)
 {
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_STRING:
@@ -1282,10 +1282,6 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
 	case V4L2_CTRL_TYPE_BOOLEAN:
 		ptr.p_s32[idx] = ctrl->default_value;
 		break;
-	case V4L2_CTRL_TYPE_U8:
-	case V4L2_CTRL_TYPE_S8:
-		ptr.p_u8[idx] = ctrl->default_value;
-		break;
 	case V4L2_CTRL_TYPE_U16:
 	case V4L2_CTRL_TYPE_S16:
 		ptr.p_u16[idx] = ctrl->default_value;
@@ -1295,8 +1291,35 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
 		ptr.p_u32[idx] = ctrl->default_value;
 		break;
 	default:
-		idx *= ctrl->elem_size;
-		memset(ptr.p + idx, 0, ctrl->elem_size);
+		break;
+	}
+}
+
+static void std_init(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr)
+{
+	u32 idx;
+
+	switch (ctrl->type) {
+	case V4L2_CTRL_TYPE_STRING:
+	case V4L2_CTRL_TYPE_INTEGER64:
+	case V4L2_CTRL_TYPE_INTEGER:
+	case V4L2_CTRL_TYPE_INTEGER_MENU:
+	case V4L2_CTRL_TYPE_MENU:
+	case V4L2_CTRL_TYPE_BITMASK:
+	case V4L2_CTRL_TYPE_BOOLEAN:
+	case V4L2_CTRL_TYPE_U16:
+	case V4L2_CTRL_TYPE_S16:
+	case V4L2_CTRL_TYPE_U32:
+	case V4L2_CTRL_TYPE_S32:
+		for (idx = 0; idx < ctrl->elems; idx++)
+			std_init_one(ctrl, idx, ptr);
+		break;
+	case V4L2_CTRL_TYPE_U8:
+	case V4L2_CTRL_TYPE_S8:
+		memset(ptr.p_u8, ctrl->default_value, ctrl->elems);
+		break;
+	default:
+		memset(ptr.p, 0, ctrl->elems * ctrl->elem_size);
 		break;
 	}
 }
@@ -1929,7 +1952,6 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 	unsigned elems = 1;
 	bool is_array;
 	unsigned tot_ctrl_size;
-	unsigned idx;
 	void *data;
 	int err;
 
@@ -2049,10 +2071,8 @@ 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;
 	}
-	for (idx = 0; idx < elems; idx++) {
-		ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
-		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
-	}
+	ctrl->type_ops->init(ctrl, ctrl->p_cur);
+	ctrl->type_ops->init(ctrl, ctrl->p_new);
 
 	if (handler_new_ref(hdl, ctrl)) {
 		kfree(ctrl);
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index e1cfb8f..a7280e9 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -87,8 +87,7 @@ struct v4l2_ctrl_type_ops {
 	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, u32 idx,
-		     union v4l2_ctrl_ptr ptr);
+	void (*init)(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr);
 	void (*log)(const struct v4l2_ctrl *ctrl);
 	int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
 			union v4l2_ctrl_ptr ptr);
-- 
2.0.5

[PATCH v2 4/6] v4l2-ctrls: Export the standard control type operations

[Laurent Pinchart]

Drivers that implement custom control types need to implement the equal,
init, log and validate operations. Depending on the control type some of
those operations can use the standard control type implementation
provided by the v4l2 control framework. Export them to enable their
reuse.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
Acked-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/v4l2-core/v4l2-ctrls.c | 27 ++++++++++++++++-----------
 include/media/v4l2-ctrls.h           |  9 +++++++++
 2 files changed, 25 insertions(+), 11 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index 81b8e66..cbc83b6 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -1233,9 +1233,9 @@ 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, u32 idx,
-		      union v4l2_ctrl_ptr ptr1,
-		      union v4l2_ctrl_ptr ptr2)
+bool v4l2_ctrl_type_std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
+			      union v4l2_ctrl_ptr ptr1,
+			      union v4l2_ctrl_ptr ptr2)
 {
 	switch (ctrl->type) {
 	case V4L2_CTRL_TYPE_BUTTON:
@@ -1262,6 +1262,7 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
 		return !memcmp(ptr1.p + idx, ptr2.p + idx, ctrl->elem_size);
 	}
 }
+EXPORT_SYMBOL_GPL(v4l2_ctrl_type_std_equal);
 
 static void std_init_one(const struct v4l2_ctrl *ctrl, u32 idx,
 			 union v4l2_ctrl_ptr ptr)
@@ -1295,7 +1296,8 @@ static void std_init_one(const struct v4l2_ctrl *ctrl, u32 idx,
 	}
 }
 
-static void std_init(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr)
+void v4l2_ctrl_type_std_init(const struct v4l2_ctrl *ctrl,
+			     union v4l2_ctrl_ptr ptr)
 {
 	u32 idx;
 
@@ -1323,8 +1325,9 @@ static void std_init(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr)
 		break;
 	}
 }
+EXPORT_SYMBOL_GPL(v4l2_ctrl_type_std_init);
 
-static void std_log(const struct v4l2_ctrl *ctrl)
+void v4l2_ctrl_type_std_log(const struct v4l2_ctrl *ctrl)
 {
 	union v4l2_ctrl_ptr ptr = ctrl->p_cur;
 
@@ -1381,6 +1384,7 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 		break;
 	}
 }
+EXPORT_SYMBOL_GPL(v4l2_ctrl_type_std_log);
 
 /*
  * Round towards the closest legal value. Be careful when we are
@@ -1404,8 +1408,8 @@ static void std_log(const struct v4l2_ctrl *ctrl)
 })
 
 /* Validate a new control */
-static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
-			union v4l2_ctrl_ptr ptr)
+int v4l2_ctrl_type_std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
+				union v4l2_ctrl_ptr ptr)
 {
 	size_t len;
 	u64 offset;
@@ -1479,12 +1483,13 @@ static int std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
 		return -EINVAL;
 	}
 }
+EXPORT_SYMBOL_GPL(v4l2_ctrl_type_std_validate);
 
 static const struct v4l2_ctrl_type_ops std_type_ops = {
-	.equal = std_equal,
-	.init = std_init,
-	.log = std_log,
-	.validate = std_validate,
+	.equal = v4l2_ctrl_type_std_equal,
+	.init = v4l2_ctrl_type_std_init,
+	.log = v4l2_ctrl_type_std_log,
+	.validate = v4l2_ctrl_type_std_validate,
 };
 
 /* Helper function: copy the given control value back to the caller */
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index a7280e9..71067fb 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -93,6 +93,15 @@ struct v4l2_ctrl_type_ops {
 			union v4l2_ctrl_ptr ptr);
 };
 
+bool v4l2_ctrl_type_std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
+			      union v4l2_ctrl_ptr ptr1,
+			      union v4l2_ctrl_ptr ptr2);
+void v4l2_ctrl_type_std_init(const struct v4l2_ctrl *ctrl,
+			     union v4l2_ctrl_ptr ptr);
+void v4l2_ctrl_type_std_log(const struct v4l2_ctrl *ctrl);
+int v4l2_ctrl_type_std_validate(const struct v4l2_ctrl *ctrl, u32 idx,
+				union v4l2_ctrl_ptr ptr);
+
 typedef void (*v4l2_ctrl_notify_fnc)(struct v4l2_ctrl *ctrl, void *priv);
 
 /** struct v4l2_ctrl - The control structure.
-- 
2.0.5

[PATCH v2 5/6] staging: media: omap4iss: Cleanup media entities after unregistration

[Laurent Pinchart]

The ipipeif, ipipe and resizer media entities are cleaned up before
unregistering the media device, creating a race condition. Fix it by
cleaning them up at cleanup time.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/omap4iss/iss_ipipe.c   | 6 +++---
 drivers/staging/media/omap4iss/iss_ipipeif.c | 6 +++---
 drivers/staging/media/omap4iss/iss_resizer.c | 6 +++---
 3 files changed, 9 insertions(+), 9 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss_ipipe.c b/drivers/staging/media/omap4iss/iss_ipipe.c
index a1a46ef..73b165e 100644
--- a/drivers/staging/media/omap4iss/iss_ipipe.c
+++ b/drivers/staging/media/omap4iss/iss_ipipe.c
@@ -516,8 +516,6 @@ static int ipipe_init_entities(struct iss_ipipe_device *ipipe)
 
 void omap4iss_ipipe_unregister_entities(struct iss_ipipe_device *ipipe)
 {
-	media_entity_cleanup(&ipipe->subdev.entity);
-
 	v4l2_device_unregister_subdev(&ipipe->subdev);
 }
 
@@ -566,5 +564,7 @@ int omap4iss_ipipe_init(struct iss_device *iss)
  */
 void omap4iss_ipipe_cleanup(struct iss_device *iss)
 {
-	/* FIXME: are you sure there's nothing to do? */
+	struct iss_ipipe_device *ipipe = &iss->ipipe;
+
+	media_entity_cleanup(&ipipe->subdev.entity);
 }
diff --git a/drivers/staging/media/omap4iss/iss_ipipeif.c b/drivers/staging/media/omap4iss/iss_ipipeif.c
index 3943fae..1a905e1 100644
--- a/drivers/staging/media/omap4iss/iss_ipipeif.c
+++ b/drivers/staging/media/omap4iss/iss_ipipeif.c
@@ -773,8 +773,6 @@ static int ipipeif_init_entities(struct iss_ipipeif_device *ipipeif)
 
 void omap4iss_ipipeif_unregister_entities(struct iss_ipipeif_device *ipipeif)
 {
-	media_entity_cleanup(&ipipeif->subdev.entity);
-
 	v4l2_device_unregister_subdev(&ipipeif->subdev);
 	omap4iss_video_unregister(&ipipeif->video_out);
 }
@@ -828,5 +826,7 @@ int omap4iss_ipipeif_init(struct iss_device *iss)
  */
 void omap4iss_ipipeif_cleanup(struct iss_device *iss)
 {
-	/* FIXME: are you sure there's nothing to do? */
+	struct iss_ipipeif_device *ipipeif = &iss->ipipeif;
+
+	media_entity_cleanup(&ipipeif->subdev.entity);
 }
diff --git a/drivers/staging/media/omap4iss/iss_resizer.c b/drivers/staging/media/omap4iss/iss_resizer.c
index 3ab9728..d4834b6 100644
--- a/drivers/staging/media/omap4iss/iss_resizer.c
+++ b/drivers/staging/media/omap4iss/iss_resizer.c
@@ -817,8 +817,6 @@ static int resizer_init_entities(struct iss_resizer_device *resizer)
 
 void omap4iss_resizer_unregister_entities(struct iss_resizer_device *resizer)
 {
-	media_entity_cleanup(&resizer->subdev.entity);
-
 	v4l2_device_unregister_subdev(&resizer->subdev);
 	omap4iss_video_unregister(&resizer->video_out);
 }
@@ -872,5 +870,7 @@ int omap4iss_resizer_init(struct iss_device *iss)
  */
 void omap4iss_resizer_cleanup(struct iss_device *iss)
 {
-	/* FIXME: are you sure there's nothing to do? */
+	struct iss_resizer_device *resizer = &iss->resizer;
+
+	media_entity_cleanup(&resizer->subdev.entity);
 }
-- 
2.0.5

[PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix

[Laurent Pinchart]

Expose the module as two controls, one for the 3x3 multiplier matrix and
one for the 3x1 offset vector.

Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
 drivers/staging/media/omap4iss/iss_ipipe.c | 129 ++++++++++++++++++++++++++++-
 drivers/staging/media/omap4iss/iss_ipipe.h |  17 ++++
 2 files changed, 144 insertions(+), 2 deletions(-)

diff --git a/drivers/staging/media/omap4iss/iss_ipipe.c b/drivers/staging/media/omap4iss/iss_ipipe.c
index 73b165e..624c5d2 100644
--- a/drivers/staging/media/omap4iss/iss_ipipe.c
+++ b/drivers/staging/media/omap4iss/iss_ipipe.c
@@ -119,6 +119,105 @@ static void ipipe_configure(struct iss_ipipe_device *ipipe)
 }
 
 /* -----------------------------------------------------------------------------
+ * V4L2 controls
+ */
+
+#define OMAP4ISS_IPIPE_CID_BASE			(V4L2_CID_USER_BASE | 0xf000)
+#define OMAP4ISS_IPIPE_CID_RGB2RGB_MULT		(OMAP4ISS_IPIPE_CID_BASE + 0)
+#define OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET	(OMAP4ISS_IPIPE_CID_BASE + 1)
+
+/*
+ * ipipe_s_ctrl - Handle set control subdev method
+ * @ctrl: pointer to v4l2 control structure
+ */
+static int ipipe_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct iss_ipipe_device *ipipe =
+		container_of(ctrl->handler, struct iss_ipipe_device, ctrls);
+	struct iss_device *iss = to_iss_device(ipipe);
+	unsigned int i;
+
+	mutex_lock(&ipipe->lock);
+
+	if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED)
+		goto done;
+
+	switch (ctrl->id) {
+	case OMAP4ISS_IPIPE_CID_RGB2RGB_MULT:
+	case OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET:
+		ctrl = ipipe->rgb2rgb_mult;
+		for (i = 0; i < ctrl->elems; ++i)
+			iss_reg_write(iss, OMAP4_ISS_MEM_ISP_IPIPE,
+				      IPIPE_RGB1_MUL_RR + 4 * i,
+				      ctrl->p_new.p_s16[i]);
+
+		ctrl = ipipe->rgb2rgb_offset;
+		for (i = 0; i < ctrl->elems; ++i)
+			iss_reg_write(iss, OMAP4_ISS_MEM_ISP_IPIPE,
+				      IPIPE_RGB1_OFT_OR + 4 * i,
+				      ctrl->p_new.p_s16[i]);
+		break;
+	}
+
+done:
+	mutex_unlock(&ipipe->lock);
+	return 0;
+}
+
+static const struct v4l2_ctrl_ops ipipe_ctrl_ops = {
+	.s_ctrl = ipipe_s_ctrl,
+};
+
+static void ipipe_ctrl_type_init(const struct v4l2_ctrl *ctrl,
+				 union v4l2_ctrl_ptr ptr)
+{
+	unsigned int i;
+
+	switch (ctrl->id) {
+	case OMAP4ISS_IPIPE_CID_RGB2RGB_MULT:
+		/*
+		 * Initialize the diagonal to 1.0 and all other elements to
+		 * 0.0.
+		 */
+		for (i = 0; i < ctrl->elems; ++i)
+			ptr.p_s16[i] = (i % 4) ? 0 : 256;
+		break;
+	}
+}
+
+static const struct v4l2_ctrl_type_ops ipipe_ctrl_type_ops = {
+	.equal = v4l2_ctrl_type_std_equal,
+	.init = ipipe_ctrl_type_init,
+	.log = v4l2_ctrl_type_std_log,
+	.validate = v4l2_ctrl_type_std_validate,
+};
+
+static const struct v4l2_ctrl_config ipipe_ctrls[] = {
+	{
+		.ops = &ipipe_ctrl_ops,
+		.type_ops = &ipipe_ctrl_type_ops,
+		.id = OMAP4ISS_IPIPE_CID_RGB2RGB_MULT,
+		.name = "RGB2RGB Multiplier",
+		.type = V4L2_CTRL_TYPE_S16,
+		.def = 0,
+		.min = -2048,
+		.max = 2047,
+		.step = 1,
+		.dims = { 3, 3 },
+	}, {
+		.ops = &ipipe_ctrl_ops,
+		.id = OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET,
+		.name = "RGB2RGB Offset",
+		.type = V4L2_CTRL_TYPE_S16,
+		.def = 0,
+		.min = -4096,
+		.max = 4095,
+		.step = 1,
+		.dims = { 3 },
+	},
+};
+
+/* -----------------------------------------------------------------------------
  * V4L2 subdev operations
  */
 
@@ -133,9 +232,11 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
 	struct iss_device *iss = to_iss_device(ipipe);
 	int ret = 0;
 
+	mutex_lock(&ipipe->lock);
+
 	if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED) {
 		if (enable == ISS_PIPELINE_STREAM_STOPPED)
-			return 0;
+			goto done;
 
 		omap4iss_isp_subclk_enable(iss, OMAP4_ISS_ISP_SUBCLK_IPIPE);
 
@@ -161,7 +262,7 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
 
 	case ISS_PIPELINE_STREAM_STOPPED:
 		if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED)
-			return 0;
+			goto done;
 		if (omap4iss_module_sync_idle(&sd->entity, &ipipe->wait,
 					      &ipipe->stopping))
 			ret = -ETIMEDOUT;
@@ -172,6 +273,13 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
 	}
 
 	ipipe->state = enable;
+
+done:
+	mutex_unlock(&ipipe->lock);
+
+	if (enable == ISS_PIPELINE_STREAM_CONTINUOUS)
+		v4l2_ctrl_handler_setup(ipipe->subdev.ctrl_handler);
+
 	return ret;
 }
 
@@ -501,6 +609,20 @@ static int ipipe_init_entities(struct iss_ipipe_device *ipipe)
 	v4l2_set_subdevdata(sd, ipipe);
 	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
 
+	v4l2_ctrl_handler_init(&ipipe->ctrls, 2);
+
+	ipipe->rgb2rgb_mult = v4l2_ctrl_new_custom(&ipipe->ctrls,
+						   &ipipe_ctrls[0], NULL);
+	ipipe->rgb2rgb_offset = v4l2_ctrl_new_custom(&ipipe->ctrls,
+						     &ipipe_ctrls[1], NULL);
+
+	if (ipipe->ctrls.error)
+		return ipipe->ctrls.error;
+
+	v4l2_ctrl_cluster(2, &ipipe->rgb2rgb_mult);
+
+	sd->ctrl_handler = &ipipe->ctrls;
+
 	pads[IPIPE_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
 	pads[IPIPE_PAD_SOURCE_VP].flags = MEDIA_PAD_FL_SOURCE;
 
@@ -554,6 +676,7 @@ int omap4iss_ipipe_init(struct iss_device *iss)
 
 	ipipe->state = ISS_PIPELINE_STREAM_STOPPED;
 	init_waitqueue_head(&ipipe->wait);
+	mutex_init(&ipipe->lock);
 
 	return ipipe_init_entities(ipipe);
 }
@@ -566,5 +689,7 @@ void omap4iss_ipipe_cleanup(struct iss_device *iss)
 {
 	struct iss_ipipe_device *ipipe = &iss->ipipe;
 
+	v4l2_ctrl_handler_free(&ipipe->ctrls);
 	media_entity_cleanup(&ipipe->subdev.entity);
+	mutex_destroy(&ipipe->lock);
 }
diff --git a/drivers/staging/media/omap4iss/iss_ipipe.h b/drivers/staging/media/omap4iss/iss_ipipe.h
index c22d904..7684271 100644
--- a/drivers/staging/media/omap4iss/iss_ipipe.h
+++ b/drivers/staging/media/omap4iss/iss_ipipe.h
@@ -14,6 +14,10 @@
 #ifndef OMAP4_ISS_IPIPE_H
 #define OMAP4_ISS_IPIPE_H
 
+#include <linux/mutex.h>
+
+#include <media/v4l2-ctrls.h>
+
 #include "iss_video.h"
 
 enum ipipe_input_entity {
@@ -34,9 +38,13 @@ enum ipipe_input_entity {
  * @subdev: V4L2 subdevice
  * @pads: Sink and source media entity pads
  * @formats: Active video formats
+ * @ctrls: Control handler
+ * @rgb2rgb_mult: RGB to RGB matrix multiplier control
+ * @rgb2rgb_offset: RGB to RGB matrix offset control
  * @input: Active input
  * @output: Active outputs
  * @error: A hardware error occurred during capture
+ * @lock: Protects the state field
  * @state: Streaming state
  * @wait: Wait queue used to stop the module
  * @stopping: Stopping state
@@ -46,10 +54,19 @@ struct iss_ipipe_device {
 	struct media_pad pads[IPIPE_PADS_NUM];
 	struct v4l2_mbus_framefmt formats[IPIPE_PADS_NUM];
 
+	struct v4l2_ctrl_handler ctrls;
+	struct {
+		/* RGB2RGB cluster */
+		struct v4l2_ctrl *rgb2rgb_mult;
+		struct v4l2_ctrl *rgb2rgb_offset;
+	};
+
 	enum ipipe_input_entity input;
 	unsigned int output;
 	unsigned int error;
 
+	struct mutex lock;
+	bool streaming;
 	enum iss_pipeline_stream_state state;
 	wait_queue_head_t wait;
 	atomic_t stopping;
-- 
2.0.5

Re: [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types

[Hans Verkuil]

On 01/28/15 10:17, Laurent Pinchart wrote:
> Only unsigned compound types are implemented so far, add the
> corresponding signes types.

Nitpick: signes -> signed

Regards,

	Hans

> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Re: [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control

[Hans Verkuil]

On 01/28/15 10:17, Laurent Pinchart wrote:
> The control type init operation is called in a loop to initialize all
> elements of a control. Not only is this inefficient for control types
> that could use a memset(), it also complicates the implementation of
> custom control types, for instance when a matrix needs to be initialized
> with different values for its elements.
> 
> Make the init operation initialize the whole control instead, and use
> memset() when possible.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

Acked-by: Hans Verkuil <hans.verkuil@cisco.com>

Thanks!

	Hans

> ---
>  drivers/media/v4l2-core/v4l2-ctrls.c | 46 ++++++++++++++++++++++++++----------
>  include/media/v4l2-ctrls.h           |  3 +--
>  2 files changed, 34 insertions(+), 15 deletions(-)
> 
> Changes since v1:
> 
> - Remove support for V4L2_CTRL_TYPE_U8 and V4L2_CTRL_TYPE_S8 from
>   std_init_one(), as those cases are now handled by std_init()
> 
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index adac93e..81b8e66 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -1263,8 +1263,8 @@ static bool std_equal(const struct v4l2_ctrl *ctrl, u32 idx,
>  	}
>  }
>  
> -static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
> -		     union v4l2_ctrl_ptr ptr)
> +static void std_init_one(const struct v4l2_ctrl *ctrl, u32 idx,
> +			 union v4l2_ctrl_ptr ptr)
>  {
>  	switch (ctrl->type) {
>  	case V4L2_CTRL_TYPE_STRING:
> @@ -1282,10 +1282,6 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
>  	case V4L2_CTRL_TYPE_BOOLEAN:
>  		ptr.p_s32[idx] = ctrl->default_value;
>  		break;
> -	case V4L2_CTRL_TYPE_U8:
> -	case V4L2_CTRL_TYPE_S8:
> -		ptr.p_u8[idx] = ctrl->default_value;
> -		break;
>  	case V4L2_CTRL_TYPE_U16:
>  	case V4L2_CTRL_TYPE_S16:
>  		ptr.p_u16[idx] = ctrl->default_value;
> @@ -1295,8 +1291,35 @@ static void std_init(const struct v4l2_ctrl *ctrl, u32 idx,
>  		ptr.p_u32[idx] = ctrl->default_value;
>  		break;
>  	default:
> -		idx *= ctrl->elem_size;
> -		memset(ptr.p + idx, 0, ctrl->elem_size);
> +		break;
> +	}
> +}
> +
> +static void std_init(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr)
> +{
> +	u32 idx;
> +
> +	switch (ctrl->type) {
> +	case V4L2_CTRL_TYPE_STRING:
> +	case V4L2_CTRL_TYPE_INTEGER64:
> +	case V4L2_CTRL_TYPE_INTEGER:
> +	case V4L2_CTRL_TYPE_INTEGER_MENU:
> +	case V4L2_CTRL_TYPE_MENU:
> +	case V4L2_CTRL_TYPE_BITMASK:
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +	case V4L2_CTRL_TYPE_U16:
> +	case V4L2_CTRL_TYPE_S16:
> +	case V4L2_CTRL_TYPE_U32:
> +	case V4L2_CTRL_TYPE_S32:
> +		for (idx = 0; idx < ctrl->elems; idx++)
> +			std_init_one(ctrl, idx, ptr);
> +		break;
> +	case V4L2_CTRL_TYPE_U8:
> +	case V4L2_CTRL_TYPE_S8:
> +		memset(ptr.p_u8, ctrl->default_value, ctrl->elems);
> +		break;
> +	default:
> +		memset(ptr.p, 0, ctrl->elems * ctrl->elem_size);
>  		break;
>  	}
>  }
> @@ -1929,7 +1952,6 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  	unsigned elems = 1;
>  	bool is_array;
>  	unsigned tot_ctrl_size;
> -	unsigned idx;
>  	void *data;
>  	int err;
>  
> @@ -2049,10 +2071,8 @@ 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;
>  	}
> -	for (idx = 0; idx < elems; idx++) {
> -		ctrl->type_ops->init(ctrl, idx, ctrl->p_cur);
> -		ctrl->type_ops->init(ctrl, idx, ctrl->p_new);
> -	}
> +	ctrl->type_ops->init(ctrl, ctrl->p_cur);
> +	ctrl->type_ops->init(ctrl, ctrl->p_new);
>  
>  	if (handler_new_ref(hdl, ctrl)) {
>  		kfree(ctrl);
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index e1cfb8f..a7280e9 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -87,8 +87,7 @@ struct v4l2_ctrl_type_ops {
>  	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, u32 idx,
> -		     union v4l2_ctrl_ptr ptr);
> +	void (*init)(const struct v4l2_ctrl *ctrl, union v4l2_ctrl_ptr ptr);
>  	void (*log)(const struct v4l2_ctrl *ctrl);
>  	int (*validate)(const struct v4l2_ctrl *ctrl, u32 idx,
>  			union v4l2_ctrl_ptr ptr);
> 

Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix

[Hans Verkuil]

On 01/28/15 10:17, Laurent Pinchart wrote:
> Expose the module as two controls, one for the 3x3 multiplier matrix and
> one for the 3x1 offset vector.
> 
> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> ---
>  drivers/staging/media/omap4iss/iss_ipipe.c | 129 ++++++++++++++++++++++++++++-
>  drivers/staging/media/omap4iss/iss_ipipe.h |  17 ++++
>  2 files changed, 144 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/staging/media/omap4iss/iss_ipipe.c b/drivers/staging/media/omap4iss/iss_ipipe.c
> index 73b165e..624c5d2 100644
> --- a/drivers/staging/media/omap4iss/iss_ipipe.c
> +++ b/drivers/staging/media/omap4iss/iss_ipipe.c
> @@ -119,6 +119,105 @@ static void ipipe_configure(struct iss_ipipe_device *ipipe)
>  }
>  
>  /* -----------------------------------------------------------------------------
> + * V4L2 controls
> + */
> +
> +#define OMAP4ISS_IPIPE_CID_BASE			(V4L2_CID_USER_BASE | 0xf000)

Private control ranges should be reserved in uapi/linux/v4l2-controls.h.

See e.g. V4L2_CID_USER_SAA7134_BASE.

> +#define OMAP4ISS_IPIPE_CID_RGB2RGB_MULT		(OMAP4ISS_IPIPE_CID_BASE + 0)
> +#define OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET	(OMAP4ISS_IPIPE_CID_BASE + 1)

Can you give some information how the values are interpreted? That should
be documented anyway, but I would like to see how this compares to the
adv drivers. This is something that we might want to make available as
standard controls. I will have to think about that a bit more.

Regards,

	Hans

> +
> +/*
> + * ipipe_s_ctrl - Handle set control subdev method
> + * @ctrl: pointer to v4l2 control structure
> + */
> +static int ipipe_s_ctrl(struct v4l2_ctrl *ctrl)
> +{
> +	struct iss_ipipe_device *ipipe =
> +		container_of(ctrl->handler, struct iss_ipipe_device, ctrls);
> +	struct iss_device *iss = to_iss_device(ipipe);
> +	unsigned int i;
> +
> +	mutex_lock(&ipipe->lock);
> +
> +	if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED)
> +		goto done;
> +
> +	switch (ctrl->id) {
> +	case OMAP4ISS_IPIPE_CID_RGB2RGB_MULT:
> +	case OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET:
> +		ctrl = ipipe->rgb2rgb_mult;
> +		for (i = 0; i < ctrl->elems; ++i)
> +			iss_reg_write(iss, OMAP4_ISS_MEM_ISP_IPIPE,
> +				      IPIPE_RGB1_MUL_RR + 4 * i,
> +				      ctrl->p_new.p_s16[i]);
> +
> +		ctrl = ipipe->rgb2rgb_offset;
> +		for (i = 0; i < ctrl->elems; ++i)
> +			iss_reg_write(iss, OMAP4_ISS_MEM_ISP_IPIPE,
> +				      IPIPE_RGB1_OFT_OR + 4 * i,
> +				      ctrl->p_new.p_s16[i]);
> +		break;
> +	}
> +
> +done:
> +	mutex_unlock(&ipipe->lock);
> +	return 0;
> +}
> +
> +static const struct v4l2_ctrl_ops ipipe_ctrl_ops = {
> +	.s_ctrl = ipipe_s_ctrl,
> +};
> +
> +static void ipipe_ctrl_type_init(const struct v4l2_ctrl *ctrl,
> +				 union v4l2_ctrl_ptr ptr)
> +{
> +	unsigned int i;
> +
> +	switch (ctrl->id) {
> +	case OMAP4ISS_IPIPE_CID_RGB2RGB_MULT:
> +		/*
> +		 * Initialize the diagonal to 1.0 and all other elements to
> +		 * 0.0.
> +		 */
> +		for (i = 0; i < ctrl->elems; ++i)
> +			ptr.p_s16[i] = (i % 4) ? 0 : 256;
> +		break;
> +	}
> +}
> +
> +static const struct v4l2_ctrl_type_ops ipipe_ctrl_type_ops = {
> +	.equal = v4l2_ctrl_type_std_equal,
> +	.init = ipipe_ctrl_type_init,
> +	.log = v4l2_ctrl_type_std_log,
> +	.validate = v4l2_ctrl_type_std_validate,
> +};
> +
> +static const struct v4l2_ctrl_config ipipe_ctrls[] = {
> +	{
> +		.ops = &ipipe_ctrl_ops,
> +		.type_ops = &ipipe_ctrl_type_ops,
> +		.id = OMAP4ISS_IPIPE_CID_RGB2RGB_MULT,
> +		.name = "RGB2RGB Multiplier",
> +		.type = V4L2_CTRL_TYPE_S16,
> +		.def = 0,
> +		.min = -2048,
> +		.max = 2047,
> +		.step = 1,
> +		.dims = { 3, 3 },
> +	}, {
> +		.ops = &ipipe_ctrl_ops,
> +		.id = OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET,
> +		.name = "RGB2RGB Offset",
> +		.type = V4L2_CTRL_TYPE_S16,
> +		.def = 0,
> +		.min = -4096,
> +		.max = 4095,
> +		.step = 1,
> +		.dims = { 3 },
> +	},
> +};
> +
> +/* -----------------------------------------------------------------------------
>   * V4L2 subdev operations
>   */
>  
> @@ -133,9 +232,11 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
>  	struct iss_device *iss = to_iss_device(ipipe);
>  	int ret = 0;
>  
> +	mutex_lock(&ipipe->lock);
> +
>  	if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED) {
>  		if (enable == ISS_PIPELINE_STREAM_STOPPED)
> -			return 0;
> +			goto done;
>  
>  		omap4iss_isp_subclk_enable(iss, OMAP4_ISS_ISP_SUBCLK_IPIPE);
>  
> @@ -161,7 +262,7 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
>  
>  	case ISS_PIPELINE_STREAM_STOPPED:
>  		if (ipipe->state == ISS_PIPELINE_STREAM_STOPPED)
> -			return 0;
> +			goto done;
>  		if (omap4iss_module_sync_idle(&sd->entity, &ipipe->wait,
>  					      &ipipe->stopping))
>  			ret = -ETIMEDOUT;
> @@ -172,6 +273,13 @@ static int ipipe_set_stream(struct v4l2_subdev *sd, int enable)
>  	}
>  
>  	ipipe->state = enable;
> +
> +done:
> +	mutex_unlock(&ipipe->lock);
> +
> +	if (enable == ISS_PIPELINE_STREAM_CONTINUOUS)
> +		v4l2_ctrl_handler_setup(ipipe->subdev.ctrl_handler);
> +
>  	return ret;
>  }
>  
> @@ -501,6 +609,20 @@ static int ipipe_init_entities(struct iss_ipipe_device *ipipe)
>  	v4l2_set_subdevdata(sd, ipipe);
>  	sd->flags |= V4L2_SUBDEV_FL_HAS_DEVNODE;
>  
> +	v4l2_ctrl_handler_init(&ipipe->ctrls, 2);
> +
> +	ipipe->rgb2rgb_mult = v4l2_ctrl_new_custom(&ipipe->ctrls,
> +						   &ipipe_ctrls[0], NULL);
> +	ipipe->rgb2rgb_offset = v4l2_ctrl_new_custom(&ipipe->ctrls,
> +						     &ipipe_ctrls[1], NULL);
> +
> +	if (ipipe->ctrls.error)
> +		return ipipe->ctrls.error;
> +
> +	v4l2_ctrl_cluster(2, &ipipe->rgb2rgb_mult);
> +
> +	sd->ctrl_handler = &ipipe->ctrls;
> +
>  	pads[IPIPE_PAD_SINK].flags = MEDIA_PAD_FL_SINK;
>  	pads[IPIPE_PAD_SOURCE_VP].flags = MEDIA_PAD_FL_SOURCE;
>  
> @@ -554,6 +676,7 @@ int omap4iss_ipipe_init(struct iss_device *iss)
>  
>  	ipipe->state = ISS_PIPELINE_STREAM_STOPPED;
>  	init_waitqueue_head(&ipipe->wait);
> +	mutex_init(&ipipe->lock);
>  
>  	return ipipe_init_entities(ipipe);
>  }
> @@ -566,5 +689,7 @@ void omap4iss_ipipe_cleanup(struct iss_device *iss)
>  {
>  	struct iss_ipipe_device *ipipe = &iss->ipipe;
>  
> +	v4l2_ctrl_handler_free(&ipipe->ctrls);
>  	media_entity_cleanup(&ipipe->subdev.entity);
> +	mutex_destroy(&ipipe->lock);
>  }
> diff --git a/drivers/staging/media/omap4iss/iss_ipipe.h b/drivers/staging/media/omap4iss/iss_ipipe.h
> index c22d904..7684271 100644
> --- a/drivers/staging/media/omap4iss/iss_ipipe.h
> +++ b/drivers/staging/media/omap4iss/iss_ipipe.h
> @@ -14,6 +14,10 @@
>  #ifndef OMAP4_ISS_IPIPE_H
>  #define OMAP4_ISS_IPIPE_H
>  
> +#include <linux/mutex.h>
> +
> +#include <media/v4l2-ctrls.h>
> +
>  #include "iss_video.h"
>  
>  enum ipipe_input_entity {
> @@ -34,9 +38,13 @@ enum ipipe_input_entity {
>   * @subdev: V4L2 subdevice
>   * @pads: Sink and source media entity pads
>   * @formats: Active video formats
> + * @ctrls: Control handler
> + * @rgb2rgb_mult: RGB to RGB matrix multiplier control
> + * @rgb2rgb_offset: RGB to RGB matrix offset control
>   * @input: Active input
>   * @output: Active outputs
>   * @error: A hardware error occurred during capture
> + * @lock: Protects the state field
>   * @state: Streaming state
>   * @wait: Wait queue used to stop the module
>   * @stopping: Stopping state
> @@ -46,10 +54,19 @@ struct iss_ipipe_device {
>  	struct media_pad pads[IPIPE_PADS_NUM];
>  	struct v4l2_mbus_framefmt formats[IPIPE_PADS_NUM];
>  
> +	struct v4l2_ctrl_handler ctrls;
> +	struct {
> +		/* RGB2RGB cluster */
> +		struct v4l2_ctrl *rgb2rgb_mult;
> +		struct v4l2_ctrl *rgb2rgb_offset;
> +	};
> +
>  	enum ipipe_input_entity input;
>  	unsigned int output;
>  	unsigned int error;
>  
> +	struct mutex lock;
> +	bool streaming;
>  	enum iss_pipeline_stream_state state;
>  	wait_queue_head_t wait;
>  	atomic_t stopping;
> 

Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix

[Laurent Pinchart]

Hi Hans,

Thank you for the review.

On Wednesday 28 January 2015 11:27:02 Hans Verkuil wrote:
> On 01/28/15 10:17, Laurent Pinchart wrote:
> > Expose the module as two controls, one for the 3x3 multiplier matrix and
> > one for the 3x1 offset vector.
> > 
> > Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> > ---
> > 
> >  drivers/staging/media/omap4iss/iss_ipipe.c | 129 +++++++++++++++++++++++-
> >  drivers/staging/media/omap4iss/iss_ipipe.h |  17 ++++
> >  2 files changed, 144 insertions(+), 2 deletions(-)
> > 
> > diff --git a/drivers/staging/media/omap4iss/iss_ipipe.c
> > b/drivers/staging/media/omap4iss/iss_ipipe.c index 73b165e..624c5d2
> > 100644
> > --- a/drivers/staging/media/omap4iss/iss_ipipe.c
> > +++ b/drivers/staging/media/omap4iss/iss_ipipe.c
> > @@ -119,6 +119,105 @@ static void ipipe_configure(struct iss_ipipe_device
> > *ipipe)> 
> >  }
> >  
> >  /* ---------------------------------------------------------------------- 
> > + * V4L2 controls
> > + */
> > +
> > +#define OMAP4ISS_IPIPE_CID_BASE			(V4L2_CID_USER_BASE | 0xf000)
> 
> Private control ranges should be reserved in uapi/linux/v4l2-controls.h.
> 
> See e.g. V4L2_CID_USER_SAA7134_BASE.

My bad, I'll fix that.

> > +#define OMAP4ISS_IPIPE_CID_RGB2RGB_MULT		(OMAP4ISS_IPIPE_CID_BASE + 
0)
> > +#define OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET	(OMAP4ISS_IPIPE_CID_BASE + 
1)
> 
> Can you give some information how the values are interpreted? That should
> be documented anyway, but I would like to see how this compares to the
> adv drivers. This is something that we might want to make available as
> standard controls. I will have to think about that a bit more.

Sure.

http://www.ti.com/lit/pdf/swpu235, section 8.3.3.4.6, page 1863.

/       \   /                         \   /      \   /          \
| R_out |   | gain_RR gain_GR gain_BR |   | R_in |   | offset_R |
| G_out | = | gain_RG gain_GG gain_BG | x | G_in | + | offset_G |
| B_out |   | gain_RB gain_GB gain_BB |   | B_in |   | offset_B |
\       /   \                         /   \      /   \          /

The two controls correspond to the multiplication matrix and offset vector. 
Coefficients are stored in 16 bits each and expressed as S3.8 (-4 to +3.996) 
for the gains and S11 (-1024 to 1023) for the offsets.

Note that the ISS IPIPE has two RGB to RGB blending matrices as shown on 
figure 8-132, page 1859. This patch implements support for the first one only. 
We should probably consider how to expose the second one as well.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix

[Laurent Pinchart]

Hi Hans,

On Wednesday 28 January 2015 14:18:20 Laurent Pinchart wrote:
> On Wednesday 28 January 2015 11:27:02 Hans Verkuil wrote:
> > On 01/28/15 10:17, Laurent Pinchart wrote:
> >> Expose the module as two controls, one for the 3x3 multiplier matrix and
> >> one for the 3x1 offset vector.
> >> 
> >> Signed-off-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
> >> ---
> >> 
> >>  drivers/staging/media/omap4iss/iss_ipipe.c | 129 +++++++++++++++++++++-
> >>  drivers/staging/media/omap4iss/iss_ipipe.h |  17 ++++
> >>  2 files changed, 144 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/drivers/staging/media/omap4iss/iss_ipipe.c
> >> b/drivers/staging/media/omap4iss/iss_ipipe.c index 73b165e..624c5d2
> >> 100644
> >> --- a/drivers/staging/media/omap4iss/iss_ipipe.c
> >> +++ b/drivers/staging/media/omap4iss/iss_ipipe.c
> >> @@ -119,6 +119,105 @@ static void ipipe_configure(struct
> >> iss_ipipe_device *ipipe)
> >>  }
> >>  
> >>  /* --------------------------------------------------------------------
> >> + * V4L2 controls
> >> + */
> >> +
> >> +#define OMAP4ISS_IPIPE_CID_BASE			(V4L2_CID_USER_BASE | 0xf000)
> > 
> > Private control ranges should be reserved in uapi/linux/v4l2-controls.h.
> > 
> > See e.g. V4L2_CID_USER_SAA7134_BASE.
> 
> My bad, I'll fix that.
> 
> >> +#define OMAP4ISS_IPIPE_CID_RGB2RGB_MULT		(OMAP4ISS_IPIPE_CID_BASE
> >> + 0)
> >> +#define OMAP4ISS_IPIPE_CID_RGB2RGB_OFFSET	(OMAP4ISS_IPIPE_CID_BASE
> >> + 1)
> 
> > Can you give some information how the values are interpreted? That should
> > be documented anyway, but I would like to see how this compares to the
> > adv drivers. This is something that we might want to make available as
> > standard controls. I will have to think about that a bit more.
> 
> Sure.
> 
> http://www.ti.com/lit/pdf/swpu235, section 8.3.3.4.6, page 1863.
> 
> /       \   /                         \   /      \   /          \
> | R_out |   | gain_RR gain_GR gain_BR |   | R_in |   | offset_R |
> | G_out | = | gain_RG gain_GG gain_BG | x | G_in | + | offset_G |
> | B_out |   | gain_RB gain_GB gain_BB |   | B_in |   | offset_B |
> \       /   \                         /   \      /   \          /
> 
> The two controls correspond to the multiplication matrix and offset vector.
> Coefficients are stored in 16 bits each and expressed as S3.8 (-4 to +3.996)
> for the gains and S11 (-1024 to 1023) for the offsets.
> 
> Note that the ISS IPIPE has two RGB to RGB blending matrices as shown on
> figure 8-132, page 1859. This patch implements support for the first one
> only. We should probably consider how to expose the second one as well.

Have you had a chance to compare this to the ADV* drivers ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types

[Laurent Pinchart]

Hi Sadegh,

On Friday 10 July 2015 15:45:23 sadegh abbasi wrote:
> Hi Hans / Laurent,
> Just wondering what has happened to these patches. I used them in my driver
> and can not find them in 4.1 release. Have they been rejected?

Not exactly. The changes to v4l2-ctrls were considered to be fine, but we have 
a policy not to merge core changes without at least one driver using them. As 
the OMAP4 ISS part of the series still needs work, nothing got merged.

Hans, you mentioned you wanted to look at the RGB2RGB controls in a wider 
context (including the adv drivers for instance). Do you have anything to 
report ?

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types

[Hans Verkuil]

On 07/13/2015 12:18 AM, Laurent Pinchart wrote:
> Hi Sadegh,
> 
> On Friday 10 July 2015 15:45:23 sadegh abbasi wrote:
>> Hi Hans / Laurent,
>> Just wondering what has happened to these patches. I used them in my driver
>> and can not find them in 4.1 release. Have they been rejected?
> 
> Not exactly. The changes to v4l2-ctrls were considered to be fine, but we have 
> a policy not to merge core changes without at least one driver using them. As 
> the OMAP4 ISS part of the series still needs work, nothing got merged.
> 
> Hans, you mentioned you wanted to look at the RGB2RGB controls in a wider 
> context (including the adv drivers for instance). Do you have anything to 
> report ?

Yes and no :-)

It's part of my work to support colorspace conversion hardware. Now for 4.2 I merged
a lot of patches that pave the way for the remaining code to go in. But that remaining
code still needs to be cleaned up. My code can be found here:

http://git.linuxtv.org/cgit.cgi/hverkuil/media_tree.git/log/?h=csc

Unfortunately it's on hold because the CEC framework has priority for me (and Cisco).

In addition, for drm Intel is working on color manager code as well (see the patch
series titled "Color Manager Implementation" at dri-devel). I would like to be able
to share the low-level matrix/vector calculation code with them. So I am waiting to
see how that turns out.

So quite some work has been done, but it's not ready for merging.

Regards,

	Hans