* [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 10:19 ` Hans Verkuil
2015-01-28 9:17 ` [PATCH v2 2/6] v4l2-ctrls: Don't initialize array tail when setting a control Laurent Pinchart
` (4 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 2/6] v4l2-ctrls: Don't initialize array tail when setting a control
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control Laurent Pinchart
` (3 subsequent siblings)
5 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 1/6] v4l2-ctrls: Add new S8, S16 and S32 compound control types Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 2/6] v4l2-ctrls: Don't initialize array tail when setting a control Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 10:20 ` Hans Verkuil
2015-01-28 9:17 ` [PATCH v2 4/6] v4l2-ctrls: Export the standard control type operations Laurent Pinchart
` (2 subsequent siblings)
5 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control
2015-01-28 9:17 ` [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control Laurent Pinchart
@ 2015-01-28 10:20 ` Hans Verkuil
0 siblings, 0 replies; 14+ messages in thread
From: Hans Verkuil @ 2015-01-28 10:20 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: sadegh abbasi
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);
>
^ permalink raw reply [flat|nested] 14+ messages in thread
* [PATCH v2 4/6] v4l2-ctrls: Export the standard control type operations
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
` (2 preceding siblings ...)
2015-01-28 9:17 ` [PATCH v2 3/6] v4l2-ctrls: Make the control type init op initialize the whole control Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 5/6] staging: media: omap4iss: Cleanup media entities after unregistration Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix Laurent Pinchart
5 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 5/6] staging: media: omap4iss: Cleanup media entities after unregistration
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
` (3 preceding siblings ...)
2015-01-28 9:17 ` [PATCH v2 4/6] v4l2-ctrls: Export the standard control type operations Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 9:17 ` [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix Laurent Pinchart
5 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix
2015-01-28 9:17 [PATCH v2 0/6] omap4iss: Add RGB2RGB blending matrix support Laurent Pinchart
` (4 preceding siblings ...)
2015-01-28 9:17 ` [PATCH v2 5/6] staging: media: omap4iss: Cleanup media entities after unregistration Laurent Pinchart
@ 2015-01-28 9:17 ` Laurent Pinchart
2015-01-28 10:27 ` Hans Verkuil
5 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 9:17 UTC (permalink / raw)
To: linux-media; +Cc: Hans Verkuil, sadegh abbasi
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
^ permalink raw reply related [flat|nested] 14+ messages in thread* Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix
2015-01-28 9:17 ` [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix Laurent Pinchart
@ 2015-01-28 10:27 ` Hans Verkuil
2015-01-28 12:18 ` Laurent Pinchart
0 siblings, 1 reply; 14+ messages in thread
From: Hans Verkuil @ 2015-01-28 10:27 UTC (permalink / raw)
To: Laurent Pinchart, linux-media; +Cc: sadegh abbasi
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;
>
^ permalink raw reply [flat|nested] 14+ messages in thread* Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix
2015-01-28 10:27 ` Hans Verkuil
@ 2015-01-28 12:18 ` Laurent Pinchart
2015-06-14 10:30 ` Laurent Pinchart
0 siblings, 1 reply; 14+ messages in thread
From: Laurent Pinchart @ 2015-01-28 12:18 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, sadegh abbasi
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
^ permalink raw reply [flat|nested] 14+ messages in thread
* Re: [PATCH v2 6/6] staging: media: omap4iss: ipipe: Expose the RGB2RGB blending matrix
2015-01-28 12:18 ` Laurent Pinchart
@ 2015-06-14 10:30 ` Laurent Pinchart
0 siblings, 0 replies; 14+ messages in thread
From: Laurent Pinchart @ 2015-06-14 10:30 UTC (permalink / raw)
To: Hans Verkuil; +Cc: linux-media, sadegh abbasi
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
^ permalink raw reply [flat|nested] 14+ messages in thread