[PATCH/RFC v2] Add new V4L2_CID_ALPHA_COMPONENT control

[PATCH/RFC v2] Add new V4L2_CID_ALPHA_COMPONENT control

[Sylwester Nawrocki]

Hello,

This changeset adds new V4L2_CID_ALPHA_COMPONENT control that allows to configure 
an alpha component of all pixels on the video capture device or on capture queue 
of a mem-to-mem device. This is meant for devices that allow to set a per-pixel
alpha at the pipeline output to a desired value and where the input alpha component 
doesn't influence the output alpha value.

The second patch adds the control to s5p-fimc video capture and mem-to-mem driver.

This changset also does a minor cleanup at the user controls DocBook chapter.

Changes since v2:
 - rename V4L2_CID_COLOR_ALPHA to V4L2_CID_ALPHA_COMPONENT,
 - the documentation improvements.


Sylwester Nawrocki (2):
  v4l: Add new alpha component control
  s5p-fimc: Add support for alpha component configuration

 Documentation/DocBook/media/v4l/compat.xml         |   11 ++++
 Documentation/DocBook/media/v4l/controls.xml       |   25 +++++++--
 .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |    7 ++-
 drivers/media/video/s5p-fimc/fimc-capture.c        |    4 ++
 drivers/media/video/s5p-fimc/fimc-core.c           |   49 ++++++++++++++++--
 drivers/media/video/s5p-fimc/fimc-core.h           |   13 ++++-
 drivers/media/video/s5p-fimc/fimc-reg.c            |   53 +++++++++++++++-----
 drivers/media/video/s5p-fimc/regs-fimc.h           |    5 ++
 drivers/media/video/v4l2-ctrls.c                   |    7 +++
 include/linux/videodev2.h                          |    6 +-
 10 files changed, 150 insertions(+), 30 deletions(-)

-- 
Regards,
 
Sylwester Nawrocki 
Samsung Poland R&D Center

[PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

This control is intended for the video capture or memory-to-memory devices
that are capable of setting up a per-pixel alpha component to some arbitrary
value. The V4L2_CID_ALPHA_COMPONENT control allows to set the alpha component
for all pixels to a value in range from 0 to 255.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
 Documentation/DocBook/media/v4l/controls.xml       |   25 +++++++++++++++----
 .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |    7 ++++-
 drivers/media/video/v4l2-ctrls.c                   |    7 +++++
 include/linux/videodev2.h                          |    6 ++--
 5 files changed, 45 insertions(+), 11 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index b68698f..0adda43 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2379,6 +2379,17 @@ that used it. It was originally scheduled for removal in 2.6.35.
       </orderedlist>
     </section>
 
+    <section>
+      <title>V4L2 in Linux 3.3</title>
+      <orderedlist>
+        <listitem>
+	  <para>Added <constant>V4L2_CID_ALPHA_COMPONENT</constant> control
+	    to the <link linkend="control">User controls class</link>.
+	  </para>
+        </listitem>
+      </orderedlist>
+    </section>
+
     <section id="other">
       <title>Relation of V4L2 to other Linux multimedia APIs</title>
 
diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 3bc5ee8..4fd83c0 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -324,12 +324,6 @@ minimum value disables backlight compensation.</entry>
 		(usually a microscope).</entry>
 	  </row>
 	  <row>
-	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
-	    <entry></entry>
-	    <entry>End of the predefined control IDs (currently
-<constant>V4L2_CID_ILLUMINATORS_2</constant> + 1).</entry>
-	  </row>
-	  <row>
 	    <entry><constant>V4L2_CID_MIN_BUFFERS_FOR_CAPTURE</constant></entry>
 	    <entry>integer</entry>
 	    <entry>This is a read-only control that can be read by the application
@@ -345,6 +339,25 @@ and used as a hint to determine the number of OUTPUT buffers to pass to REQBUFS.
 The value is the minimum number of OUTPUT buffers that is necessary for hardware
 to work.</entry>
 	  </row>
+	  <row id="v4l2-alpha-component">
+	    <entry><constant>V4L2_CID_ALPHA_COMPONENT</constant></entry>
+	    <entry>integer</entry>
+	    <entry> Sets the alpha color component on the capture device or on
+	    the capture buffer queue of a mem-to-mem device. When a mem-to-mem
+	    device produces frame format that includes an alpha component
+	    (e.g. <link linkend="rgb-formats">packed RGB image formats</link>)
+	    and the alpha value is not defined by the mem-to-mem input data
+	    this control lets you select the alpha component value of all
+	    pixels. It is applicable to any pixel format that contains an alpha
+	    component.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
+	    <entry></entry>
+	    <entry>End of the predefined control IDs (currently
+	      <constant>V4L2_CID_ALPHA_COMPONENT</constant> + 1).</entry>
+	  </row>
 	  <row>
 	    <entry><constant>V4L2_CID_PRIVATE_BASE</constant></entry>
 	    <entry></entry>
diff --git a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
index 4db272b..c13278b 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
@@ -428,8 +428,11 @@ colorspace <constant>V4L2_COLORSPACE_SRGB</constant>.</para>
     <para>Bit 7 is the most significant bit. The value of a = alpha
 bits is undefined when reading from the driver, ignored when writing
 to the driver, except when alpha blending has been negotiated for a
-<link linkend="overlay">Video Overlay</link> or <link
-linkend="osd">Video Output Overlay</link>.</para>
+<link linkend="overlay">Video Overlay</link> or <link linkend="osd">
+Video Output Overlay</link> or when alpha component has been configured
+for a <link linkend="capture">Video Capture</link> by means of <link
+linkend="v4l2-alpha-component"> <constant>V4L2_CID_ALPHA_COMPONENT
+</constant> </link> control.</para>
 
     <example>
       <title><constant>V4L2_PIX_FMT_BGR24</constant> 4 &times; 4 pixel
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 5552f81..882cc84 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -466,6 +466,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_ILLUMINATORS_2:		return "Illuminator 2";
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:	return "Minimum Number of Capture Buffers";
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Minimum Number of Output Buffers";
+	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
 
 	/* MPEG controls */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
@@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 		/* Max is calculated as RGB888 that is 2^24 */
 		*max = 0xFFFFFF;
 		break;
+	case V4L2_CID_ALPHA_COMPONENT:
+		*type = V4L2_CTRL_TYPE_INTEGER;
+		*step = 1;
+		*min = 0;
+		*max = 0xff;
+		break;
 	case V4L2_CID_FLASH_FAULT:
 		*type = V4L2_CTRL_TYPE_BITMASK;
 		break;
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 4b752d5..fdda200 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1204,10 +1204,10 @@ enum v4l2_colorfx {
 #define V4L2_CID_MIN_BUFFERS_FOR_CAPTURE	(V4L2_CID_BASE+39)
 #define V4L2_CID_MIN_BUFFERS_FOR_OUTPUT		(V4L2_CID_BASE+40)
 
-/* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+41)
+#define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
 
-/* Minimum number of buffer neede by the device */
+/* last CID + 1 */
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+42)
 
 /*  MPEG-class control IDs defined by V4L2 */
 #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)
-- 
1.7.7.2

[PATCH v2 2/2] s5p-fimc: Add support for alpha component configuration

[Sylwester Nawrocki]

On Exynos SoCs the FIMC IP allows to configure globally the alpha
component of all pixels for V4L2_PIX_FMT_RGB32, V4L2_PIX_FMT_RGB555
and V4L2_PIX_FMT_RGB444 image formats. This patch adds a v4l2 control
in order to let the applications control the alpha component value.

The alpha value range depends on the pixel format, for RGB32 it's
0..255 (8-bits), for RGB555 - 0..1 (1-bit) and for RGB444 - 0..15
(4-bits). The v4l2 control range is always 0..255 and the alpha
component data width is determined by currently set format on the
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE buffer queue. The applications
need to match the alpha channel data width and the pixel format
since the driver will ignore the alpha component bits that are not
applicable to the configured pixel format.

A new entry is added in the variant description data structure
so an additional control is created only where really supported
by the hardware.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |    4 ++
 drivers/media/video/s5p-fimc/fimc-core.c    |   49 ++++++++++++++++++++++---
 drivers/media/video/s5p-fimc/fimc-core.h    |   13 ++++++-
 drivers/media/video/s5p-fimc/fimc-reg.c     |   53 +++++++++++++++++++++------
 drivers/media/video/s5p-fimc/regs-fimc.h    |    5 +++
 5 files changed, 105 insertions(+), 19 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index 82d9ab6..70176e5 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -63,6 +63,8 @@ static int fimc_init_capture(struct fimc_dev *fimc)
 		fimc_hw_set_effect(ctx, false);
 		fimc_hw_set_output_path(ctx);
 		fimc_hw_set_out_dma(ctx);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
 		clear_bit(ST_CAPT_APPLY_CFG, &fimc->state);
 	}
 	spin_unlock_irqrestore(&fimc->slock, flags);
@@ -154,6 +156,8 @@ int fimc_capture_config_update(struct fimc_ctx *ctx)
 		fimc_hw_set_rotation(ctx);
 		fimc_prepare_dma_offset(ctx, &ctx->d_frame);
 		fimc_hw_set_out_dma(ctx);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
 		clear_bit(ST_CAPT_APPLY_CFG, &fimc->state);
 	}
 	spin_unlock(&ctx->slock);
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index 567e9ea..5fe9aeb 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -52,13 +52,29 @@ static struct fimc_fmt fimc_formats[] = {
 		.colplanes	= 1,
 		.flags		= FMT_FLAGS_M2M,
 	}, {
-		.name		= "XRGB-8-8-8-8, 32 bpp",
+		.name		= "ARGB8888, 32 bpp",
 		.fourcc		= V4L2_PIX_FMT_RGB32,
 		.depth		= { 32 },
 		.color		= S5P_FIMC_RGB888,
 		.memplanes	= 1,
 		.colplanes	= 1,
-		.flags		= FMT_FLAGS_M2M,
+		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
+	}, {
+		.name		= "ARGB1555",
+		.fourcc		= V4L2_PIX_FMT_RGB555,
+		.depth		= { 16 },
+		.color		= S5P_FIMC_RGB555,
+		.memplanes	= 1,
+		.colplanes	= 1,
+		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
+	}, {
+		.name		= "ARGB4444",
+		.fourcc		= V4L2_PIX_FMT_RGB444,
+		.depth		= { 16 },
+		.color		= S5P_FIMC_RGB444,
+		.memplanes	= 1,
+		.colplanes	= 1,
+		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
 	}, {
 		.name		= "YUV 4:2:2 packed, YCbYCr",
 		.fourcc		= V4L2_PIX_FMT_YUYV,
@@ -652,8 +668,11 @@ static void fimc_dma_run(void *priv)
 	if (ctx->state & (FIMC_DST_ADDR | FIMC_PARAMS))
 		fimc_hw_set_output_addr(fimc, &ctx->d_frame.paddr, -1);
 
-	if (ctx->state & FIMC_PARAMS)
+	if (ctx->state & FIMC_PARAMS) {
 		fimc_hw_set_out_dma(ctx);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
+	}
 
 	fimc_activate_capture(ctx);
 
@@ -790,6 +809,11 @@ static int fimc_s_ctrl(struct v4l2_ctrl *ctrl)
 		ctx->rotation = ctrl->val;
 		break;
 
+	case V4L2_CID_ALPHA_COMPONENT:
+		spin_lock_irqsave(&ctx->slock, flags);
+		ctx->d_frame.alpha = ctrl->val;
+		break;
+
 	default:
 		v4l2_err(fimc->v4l2_dev, "Invalid control: 0x%X\n", ctrl->id);
 		return -EINVAL;
@@ -806,9 +830,11 @@ static const struct v4l2_ctrl_ops fimc_ctrl_ops = {
 
 int fimc_ctrls_create(struct fimc_ctx *ctx)
 {
+	struct samsung_fimc_variant *variant = ctx->fimc_dev->variant;
+
 	if (ctx->ctrls_rdy)
 		return 0;
-	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
+	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 4);
 
 	ctx->ctrl_rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler, &fimc_ctrl_ops,
 				     V4L2_CID_HFLIP, 0, 1, 1, 0);
@@ -816,6 +842,14 @@ int fimc_ctrls_create(struct fimc_ctx *ctx)
 				    V4L2_CID_VFLIP, 0, 1, 1, 0);
 	ctx->ctrl_vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler, &fimc_ctrl_ops,
 				    V4L2_CID_ROTATE, 0, 270, 90, 0);
+
+	if (variant->has_alpha)
+		ctx->ctrl_alpha = v4l2_ctrl_new_std(&ctx->ctrl_handler,
+				    &fimc_ctrl_ops, V4L2_CID_ALPHA_COMPONENT,
+				    0, 0xff, 1, 0);
+	else
+		ctx->ctrl_alpha = NULL;
+
 	ctx->ctrls_rdy = ctx->ctrl_handler.error == 0;
 
 	return ctx->ctrl_handler.error;
@@ -838,6 +872,8 @@ void fimc_ctrls_activate(struct fimc_ctx *ctx, bool active)
 	v4l2_ctrl_activate(ctx->ctrl_rotate, active);
 	v4l2_ctrl_activate(ctx->ctrl_hflip, active);
 	v4l2_ctrl_activate(ctx->ctrl_vflip, active);
+	if (ctx->ctrl_alpha)
+		v4l2_ctrl_activate(ctx->ctrl_alpha, active);
 
 	if (active) {
 		ctx->rotation = ctx->ctrl_rotate->val;
@@ -1374,6 +1410,8 @@ static int fimc_m2m_open(struct file *file)
 	if (!ctx)
 		return -ENOMEM;
 	v4l2_fh_init(&ctx->fh, fimc->m2m.vfd);
+	ctx->fimc_dev = fimc;
+
 	ret = fimc_ctrls_create(ctx);
 	if (ret)
 		goto error_fh;
@@ -1383,7 +1421,6 @@ static int fimc_m2m_open(struct file *file)
 	file->private_data = &ctx->fh;
 	v4l2_fh_add(&ctx->fh);
 
-	ctx->fimc_dev = fimc;
 	/* Default color format */
 	ctx->s_frame.fmt = &fimc_formats[0];
 	ctx->d_frame.fmt = &fimc_formats[0];
@@ -1892,6 +1929,7 @@ static struct samsung_fimc_variant fimc0_variant_exynos4 = {
 	.has_cam_if	 = 1,
 	.has_cistatus2	 = 1,
 	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
 	.min_inp_pixsize = 16,
 	.min_out_pixsize = 16,
 	.hor_offs_align	 = 2,
@@ -1905,6 +1943,7 @@ static struct samsung_fimc_variant fimc3_variant_exynos4 = {
 	.has_cam_if	 = 1,
 	.has_cistatus2	 = 1,
 	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
 	.min_inp_pixsize = 16,
 	.min_out_pixsize = 16,
 	.hor_offs_align	 = 2,
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index c7f01c4..9d1f669 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -85,11 +85,14 @@ enum fimc_datapath {
 };
 
 enum fimc_color_fmt {
-	S5P_FIMC_RGB565 = 0x10,
+	S5P_FIMC_RGB444 = 0x10,
+	S5P_FIMC_RGB555,
+	S5P_FIMC_RGB565,
 	S5P_FIMC_RGB666,
 	S5P_FIMC_RGB888,
 	S5P_FIMC_RGB30_LOCAL,
 	S5P_FIMC_YCBCR420 = 0x20,
+	S5P_FIMC_YCBCR422,
 	S5P_FIMC_YCBYCR422,
 	S5P_FIMC_YCRYCB422,
 	S5P_FIMC_CBYCRY422,
@@ -162,6 +165,7 @@ struct fimc_fmt {
 	u16	flags;
 #define FMT_FLAGS_CAM	(1 << 0)
 #define FMT_FLAGS_M2M	(1 << 1)
+#define FMT_HAS_ALPHA	(1 << 2)
 };
 
 /**
@@ -283,6 +287,7 @@ struct fimc_frame {
 	struct fimc_addr	paddr;
 	struct fimc_dma_offset	dma_offset;
 	struct fimc_fmt		*fmt;
+	u8			alpha;
 };
 
 /**
@@ -387,6 +392,7 @@ struct samsung_fimc_variant {
 	unsigned int	has_cistatus2:1;
 	unsigned int	has_mainscaler_ext:1;
 	unsigned int	has_cam_if:1;
+	unsigned int	has_alpha:1;
 	struct fimc_pix_limit *pix_limit;
 	u16		min_inp_pixsize;
 	u16		min_out_pixsize;
@@ -482,7 +488,8 @@ struct fimc_dev {
  * @ctrl_handler:	v4l2 controls handler
  * @ctrl_rotate		image rotation control
  * @ctrl_hflip		horizontal flip control
- * @ctrl_vflip		vartical flip control
+ * @ctrl_vflip		vertical flip control
+ * @ctrl_alpha		RGB alpha control
  * @ctrls_rdy:		true if the control handler is initialized
  */
 struct fimc_ctx {
@@ -509,6 +516,7 @@ struct fimc_ctx {
 	struct v4l2_ctrl	*ctrl_rotate;
 	struct v4l2_ctrl	*ctrl_hflip;
 	struct v4l2_ctrl	*ctrl_vflip;
+	struct v4l2_ctrl	*ctrl_alpha;
 	bool			ctrls_rdy;
 };
 
@@ -674,6 +682,7 @@ void fimc_hw_set_prescaler(struct fimc_ctx *ctx);
 void fimc_hw_set_mainscaler(struct fimc_ctx *ctx);
 void fimc_hw_en_capture(struct fimc_ctx *ctx);
 void fimc_hw_set_effect(struct fimc_ctx *ctx, bool active);
+void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx);
 void fimc_hw_set_in_dma(struct fimc_ctx *ctx);
 void fimc_hw_set_input_path(struct fimc_ctx *ctx);
 void fimc_hw_set_output_path(struct fimc_ctx *ctx);
diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 44f5c2d..15466d0 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -117,7 +117,7 @@ void fimc_hw_set_target_format(struct fimc_ctx *ctx)
 		  S5P_CITRGFMT_VSIZE_MASK);
 
 	switch (frame->fmt->color) {
-	case S5P_FIMC_RGB565...S5P_FIMC_RGB888:
+	case S5P_FIMC_RGB444...S5P_FIMC_RGB888:
 		cfg |= S5P_CITRGFMT_RGB;
 		break;
 	case S5P_FIMC_YCBCR420:
@@ -175,6 +175,7 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
 	struct fimc_dev *dev = ctx->fimc_dev;
 	struct fimc_frame *frame = &ctx->d_frame;
 	struct fimc_dma_offset *offset = &frame->dma_offset;
+	struct fimc_fmt *fmt = frame->fmt;
 
 	/* Set the input dma offsets. */
 	cfg = 0;
@@ -198,15 +199,22 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
 	cfg = readl(dev->regs + S5P_CIOCTRL);
 
 	cfg &= ~(S5P_CIOCTRL_ORDER2P_MASK | S5P_CIOCTRL_ORDER422_MASK |
-		 S5P_CIOCTRL_YCBCR_PLANE_MASK);
+		 S5P_CIOCTRL_YCBCR_PLANE_MASK | S5P_CIOCTRL_RGB16FMT_MASK);
 
-	if (frame->fmt->colplanes == 1)
+	if (fmt->colplanes == 1)
 		cfg |= ctx->out_order_1p;
-	else if (frame->fmt->colplanes == 2)
+	else if (fmt->colplanes == 2)
 		cfg |= ctx->out_order_2p | S5P_CIOCTRL_YCBCR_2PLANE;
-	else if (frame->fmt->colplanes == 3)
+	else if (fmt->colplanes == 3)
 		cfg |= S5P_CIOCTRL_YCBCR_3PLANE;
 
+	if (fmt->color == S5P_FIMC_RGB565)
+		cfg |= S5P_CIOCTRL_RGB565;
+	else if (fmt->color == S5P_FIMC_RGB555)
+		cfg |= S5P_CIOCTRL_ARGB1555;
+	else if (fmt->color == S5P_FIMC_RGB444)
+		cfg |= S5P_CIOCTRL_ARGB4444;
+
 	writel(cfg, dev->regs + S5P_CIOCTRL);
 }
 
@@ -278,22 +286,28 @@ static void fimc_hw_set_scaler(struct fimc_ctx *ctx)
 	if (sc->copy_mode)
 		cfg |= S5P_CISCCTRL_ONE2ONE;
 
-
 	if (ctx->in_path == FIMC_DMA) {
-		if (src_frame->fmt->color == S5P_FIMC_RGB565)
+		switch (src_frame->fmt->color) {
+		case S5P_FIMC_RGB565:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB565;
-		else if (src_frame->fmt->color == S5P_FIMC_RGB666)
+			break;
+		case S5P_FIMC_RGB666:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB666;
-		else if (src_frame->fmt->color == S5P_FIMC_RGB888)
+			break;
+		case S5P_FIMC_RGB888:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB888;
+			break;
+		}
 	}
 
 	if (ctx->out_path == FIMC_DMA) {
-		if (dst_frame->fmt->color == S5P_FIMC_RGB565)
+		u32 color = dst_frame->fmt->color;
+
+		if (color >= S5P_FIMC_RGB444 && color <= S5P_FIMC_RGB565)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB565;
-		else if (dst_frame->fmt->color == S5P_FIMC_RGB666)
+		else if (color == S5P_FIMC_RGB666)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB666;
-		else if (dst_frame->fmt->color == S5P_FIMC_RGB888)
+		else if (color == S5P_FIMC_RGB888)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
 	} else {
 		cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
@@ -379,6 +393,21 @@ void fimc_hw_set_effect(struct fimc_ctx *ctx, bool active)
 	writel(cfg, dev->regs + S5P_CIIMGEFF);
 }
 
+void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx)
+{
+	struct fimc_dev *dev = ctx->fimc_dev;
+	struct fimc_frame *frame = &ctx->d_frame;
+	u32 cfg;
+
+	if (!(frame->fmt->flags & FMT_HAS_ALPHA))
+		return;
+
+	cfg = readl(dev->regs + S5P_CIOCTRL);
+	cfg &= ~S5P_CIOCTRL_ALPHA_OUT_MASK;
+	cfg |= (frame->alpha << 4);
+	writel(cfg, dev->regs + S5P_CIOCTRL);
+}
+
 static void fimc_hw_set_in_dma_size(struct fimc_ctx *ctx)
 {
 	struct fimc_dev *dev = ctx->fimc_dev;
diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h b/drivers/media/video/s5p-fimc/regs-fimc.h
index c8e3b94..c7a5bc5 100644
--- a/drivers/media/video/s5p-fimc/regs-fimc.h
+++ b/drivers/media/video/s5p-fimc/regs-fimc.h
@@ -107,6 +107,11 @@
 #define S5P_CIOCTRL_YCBCR_3PLANE	(0 << 3)
 #define S5P_CIOCTRL_YCBCR_2PLANE	(1 << 3)
 #define S5P_CIOCTRL_YCBCR_PLANE_MASK	(1 << 3)
+#define S5P_CIOCTRL_ALPHA_OUT_MASK	(0xff << 4)
+#define S5P_CIOCTRL_RGB16FMT_MASK	(3 << 16)
+#define S5P_CIOCTRL_RGB565		(0 << 16)
+#define S5P_CIOCTRL_ARGB1555		(1 << 16)
+#define S5P_CIOCTRL_ARGB4444		(2 << 16)
 #define S5P_CIOCTRL_ORDER2P_SHIFT	(24)
 #define S5P_CIOCTRL_ORDER2P_MASK	(3 << 24)
 #define S5P_CIOCTRL_ORDER422_2P_LSB_CRCB (0 << 24)
-- 
1.7.7.2

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Laurent Pinchart]

Hi Sylwester,

On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> This control is intended for the video capture or memory-to-memory devices
> that are capable of setting up a per-pixel alpha component to some
> arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to set the
> alpha component for all pixels to a value in range from 0 to 255.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>

Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>

> ---
>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
>  Documentation/DocBook/media/v4l/controls.xml       |   25
> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |  
>  7 ++++-
>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
>  include/linux/videodev2.h                          |    6 ++--
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/compat.xml
> b/Documentation/DocBook/media/v4l/compat.xml index b68698f..0adda43 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2379,6 +2379,17 @@ that used it. It was originally scheduled for
> removal in 2.6.35. </orderedlist>
>      </section>
> 
> +    <section>
> +      <title>V4L2 in Linux 3.3</title>
> +      <orderedlist>
> +        <listitem>
> +	  <para>Added <constant>V4L2_CID_ALPHA_COMPONENT</constant> control
> +	    to the <link linkend="control">User controls class</link>.
> +	  </para>
> +        </listitem>
> +      </orderedlist>
> +    </section>
> +
>      <section id="other">
>        <title>Relation of V4L2 to other Linux multimedia APIs</title>
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..4fd83c0
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -324,12 +324,6 @@ minimum value disables backlight compensation.</entry>
>  		(usually a microscope).</entry>
>  	  </row>
>  	  <row>
> -	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
> -	    <entry></entry>
> -	    <entry>End of the predefined control IDs (currently
> -<constant>V4L2_CID_ILLUMINATORS_2</constant> + 1).</entry>
> -	  </row>
> -	  <row>
>  	    <entry><constant>V4L2_CID_MIN_BUFFERS_FOR_CAPTURE</constant></entry>
>  	    <entry>integer</entry>
>  	    <entry>This is a read-only control that can be read by the
> application @@ -345,6 +339,25 @@ and used as a hint to determine the
> number of OUTPUT buffers to pass to REQBUFS. The value is the minimum
> number of OUTPUT buffers that is necessary for hardware to work.</entry>
>  	  </row>
> +	  <row id="v4l2-alpha-component">
> +	    <entry><constant>V4L2_CID_ALPHA_COMPONENT</constant></entry>
> +	    <entry>integer</entry>
> +	    <entry> Sets the alpha color component on the capture device or on
> +	    the capture buffer queue of a mem-to-mem device. When a mem-to-mem
> +	    device produces frame format that includes an alpha component
> +	    (e.g. <link linkend="rgb-formats">packed RGB image formats</link>)
> +	    and the alpha value is not defined by the mem-to-mem input data
> +	    this control lets you select the alpha component value of all
> +	    pixels. It is applicable to any pixel format that contains an alpha
> +	    component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
> +	    <entry></entry>
> +	    <entry>End of the predefined control IDs (currently
> +	      <constant>V4L2_CID_ALPHA_COMPONENT</constant> + 1).</entry>
> +	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_CID_PRIVATE_BASE</constant></entry>
>  	    <entry></entry>
> diff --git a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml index
> 4db272b..c13278b 100644
> --- a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> +++ b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> @@ -428,8 +428,11 @@ colorspace
> <constant>V4L2_COLORSPACE_SRGB</constant>.</para> <para>Bit 7 is the most
> significant bit. The value of a = alpha bits is undefined when reading
> from the driver, ignored when writing to the driver, except when alpha
> blending has been negotiated for a -<link linkend="overlay">Video
> Overlay</link> or <link
> -linkend="osd">Video Output Overlay</link>.</para>
> +<link linkend="overlay">Video Overlay</link> or <link linkend="osd">
> +Video Output Overlay</link> or when alpha component has been configured
> +for a <link linkend="capture">Video Capture</link> by means of <link
> +linkend="v4l2-alpha-component"> <constant>V4L2_CID_ALPHA_COMPONENT
> +</constant> </link> control.</para>
> 
>      <example>
>        <title><constant>V4L2_PIX_FMT_BGR24</constant> 4 &times; 4 pixel
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 5552f81..882cc84 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -466,6 +466,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_ILLUMINATORS_2:		return "Illuminator 2";
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:	return "Minimum Number of Capture
> Buffers"; case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Minimum Number of
> Output Buffers"; +	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha
> Component";
> 
>  	/* MPEG controls */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is 2^24 */
>  		*max = 0xFFFFFF;
>  		break;
> +	case V4L2_CID_ALPHA_COMPONENT:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*step = 1;
> +		*min = 0;
> +		*max = 0xff;
> +		break;
>  	case V4L2_CID_FLASH_FAULT:
>  		*type = V4L2_CTRL_TYPE_BITMASK;
>  		break;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 4b752d5..fdda200 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1204,10 +1204,10 @@ enum v4l2_colorfx {
>  #define V4L2_CID_MIN_BUFFERS_FOR_CAPTURE	(V4L2_CID_BASE+39)
>  #define V4L2_CID_MIN_BUFFERS_FOR_OUTPUT		(V4L2_CID_BASE+40)
> 
> -/* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+41)
> +#define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
> 
> -/* Minimum number of buffer neede by the device */
> +/* last CID + 1 */
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+42)
> 
>  /*  MPEG-class control IDs defined by V4L2 */
>  #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> This control is intended for the video capture or memory-to-memory devices
> that are capable of setting up a per-pixel alpha component to some
> arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to set the
> alpha component for all pixels to a value in range from 0 to 255.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
>  Documentation/DocBook/media/v4l/controls.xml       |   25
> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |  
>  7 ++++-
>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
>  include/linux/videodev2.h                          |    6 ++--
>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/compat.xml
> b/Documentation/DocBook/media/v4l/compat.xml index b68698f..0adda43 100644
> --- a/Documentation/DocBook/media/v4l/compat.xml
> +++ b/Documentation/DocBook/media/v4l/compat.xml
> @@ -2379,6 +2379,17 @@ that used it. It was originally scheduled for
> removal in 2.6.35. </orderedlist>
>      </section>
> 
> +    <section>
> +      <title>V4L2 in Linux 3.3</title>
> +      <orderedlist>
> +        <listitem>
> +	  <para>Added <constant>V4L2_CID_ALPHA_COMPONENT</constant> control
> +	    to the <link linkend="control">User controls class</link>.
> +	  </para>
> +        </listitem>
> +      </orderedlist>
> +    </section>
> +
>      <section id="other">
>        <title>Relation of V4L2 to other Linux multimedia APIs</title>
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml
> b/Documentation/DocBook/media/v4l/controls.xml index 3bc5ee8..4fd83c0
> 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -324,12 +324,6 @@ minimum value disables backlight compensation.</entry>
>  		(usually a microscope).</entry>
>  	  </row>
>  	  <row>
> -	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
> -	    <entry></entry>
> -	    <entry>End of the predefined control IDs (currently
> -<constant>V4L2_CID_ILLUMINATORS_2</constant> + 1).</entry>
> -	  </row>
> -	  <row>
>  	    
<entry><constant>V4L2_CID_MIN_BUFFERS_FOR_CAPTURE</constant></entry>
>  	    <entry>integer</entry>
>  	    <entry>This is a read-only control that can be read by the
> application @@ -345,6 +339,25 @@ and used as a hint to determine the
> number of OUTPUT buffers to pass to REQBUFS. The value is the minimum
> number of OUTPUT buffers that is necessary for hardware to work.</entry>
>  	  </row>
> +	  <row id="v4l2-alpha-component">
> +	    <entry><constant>V4L2_CID_ALPHA_COMPONENT</constant></entry>
> +	    <entry>integer</entry>
> +	    <entry> Sets the alpha color component on the capture device or on
> +	    the capture buffer queue of a mem-to-mem device. When a mem-to-mem
> +	    device produces frame format that includes an alpha component
> +	    (e.g. <link linkend="rgb-formats">packed RGB image formats</link>)
> +	    and the alpha value is not defined by the mem-to-mem input data
> +	    this control lets you select the alpha component value of all
> +	    pixels. It is applicable to any pixel format that contains an 
alpha
> +	    component.
> +	    </entry>
> +	  </row>
> +	  <row>
> +	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
> +	    <entry></entry>
> +	    <entry>End of the predefined control IDs (currently
> +	      <constant>V4L2_CID_ALPHA_COMPONENT</constant> + 1).</entry>
> +	  </row>
>  	  <row>
>  	    <entry><constant>V4L2_CID_PRIVATE_BASE</constant></entry>
>  	    <entry></entry>
> diff --git a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml index
> 4db272b..c13278b 100644
> --- a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> +++ b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
> @@ -428,8 +428,11 @@ colorspace
> <constant>V4L2_COLORSPACE_SRGB</constant>.</para> <para>Bit 7 is the most
> significant bit. The value of a = alpha bits is undefined when reading
> from the driver, ignored when writing to the driver, except when alpha
> blending has been negotiated for a -<link linkend="overlay">Video
> Overlay</link> or <link
> -linkend="osd">Video Output Overlay</link>.</para>
> +<link linkend="overlay">Video Overlay</link> or <link linkend="osd">
> +Video Output Overlay</link> or when alpha component has been configured
> +for a <link linkend="capture">Video Capture</link> by means of <link
> +linkend="v4l2-alpha-component"> <constant>V4L2_CID_ALPHA_COMPONENT
> +</constant> </link> control.</para>
> 
>      <example>
>        <title><constant>V4L2_PIX_FMT_BGR24</constant> 4 &times; 4 pixel
> diff --git a/drivers/media/video/v4l2-ctrls.c
> b/drivers/media/video/v4l2-ctrls.c index 5552f81..882cc84 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -466,6 +466,7 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_ILLUMINATORS_2:		return "Illuminator 2";
>  	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:	return "Minimum Number of 
Capture
> Buffers"; case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Minimum Number 
of
> Output Buffers"; +	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha
> Component";
> 
>  	/* MPEG controls */
>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is 2^24 */
>  		*max = 0xFFFFFF;
>  		break;
> +	case V4L2_CID_ALPHA_COMPONENT:
> +		*type = V4L2_CTRL_TYPE_INTEGER;
> +		*step = 1;
> +		*min = 0;
> +		*max = 0xff;
> +		break;

Hmm. Do we really want to fix the max value to 0xff? The bits assigned to the
alpha component will vary between 1 (V4L2_PIX_FMT_RGB555X), 4 
(V4L2_PIX_FMT_RGB444) or 8 (V4L2_PIX_FMT_RGB32). It wouldn't surprise me to
see larger sizes as well in the future (e.g. 16 bits).

I think the max value should be the largest alpha value the hardware can 
support. The application has to set it to the right value that corresponds
to the currently chosen pixel format. The driver just copies the first N bits 
into the alpha value where N depends on the pixel format.

what do you think?

	Hans

>  	case V4L2_CID_FLASH_FAULT:
>  		*type = V4L2_CTRL_TYPE_BITMASK;
>  		break;
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 4b752d5..fdda200 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -1204,10 +1204,10 @@ enum v4l2_colorfx {
>  #define V4L2_CID_MIN_BUFFERS_FOR_CAPTURE	(V4L2_CID_BASE+39)
>  #define V4L2_CID_MIN_BUFFERS_FOR_OUTPUT		(V4L2_CID_BASE+40)
> 
> -/* last CID + 1 */
> -#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+41)
> +#define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
> 
> -/* Minimum number of buffer neede by the device */
> +/* last CID + 1 */
> +#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+42)
> 
>  /*  MPEG-class control IDs defined by V4L2 */
>  #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)

Re: [PATCH v2 2/2] s5p-fimc: Add support for alpha component configuration

[Hans Verkuil]

On Friday 25 November 2011 16:39:32 Sylwester Nawrocki wrote:
> On Exynos SoCs the FIMC IP allows to configure globally the alpha
> component of all pixels for V4L2_PIX_FMT_RGB32, V4L2_PIX_FMT_RGB555
> and V4L2_PIX_FMT_RGB444 image formats. This patch adds a v4l2 control
> in order to let the applications control the alpha component value.
> 
> The alpha value range depends on the pixel format, for RGB32 it's
> 0..255 (8-bits), for RGB555 - 0..1 (1-bit) and for RGB444 - 0..15
> (4-bits). The v4l2 control range is always 0..255 and the alpha
> component data width is determined by currently set format on the
> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE buffer queue. The applications
> need to match the alpha channel data width and the pixel format
> since the driver will ignore the alpha component bits that are not
> applicable to the configured pixel format.

Will the driver ignore the least significant bits or the most significant 
bits?

Regards,

	Hans

> 
> A new entry is added in the variant description data structure
> so an additional control is created only where really supported
> by the hardware.
> 
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> ---
>  drivers/media/video/s5p-fimc/fimc-capture.c |    4 ++
>  drivers/media/video/s5p-fimc/fimc-core.c    |   49
> ++++++++++++++++++++++--- drivers/media/video/s5p-fimc/fimc-core.h    |  
> 13 ++++++-
>  drivers/media/video/s5p-fimc/fimc-reg.c     |   53
> +++++++++++++++++++++------ drivers/media/video/s5p-fimc/regs-fimc.h    | 
>   5 +++
>  5 files changed, 105 insertions(+), 19 deletions(-)
> 
> diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c
> b/drivers/media/video/s5p-fimc/fimc-capture.c index 82d9ab6..70176e5
> 100644
> --- a/drivers/media/video/s5p-fimc/fimc-capture.c
> +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
> @@ -63,6 +63,8 @@ static int fimc_init_capture(struct fimc_dev *fimc)
>  		fimc_hw_set_effect(ctx, false);
>  		fimc_hw_set_output_path(ctx);
>  		fimc_hw_set_out_dma(ctx);
> +		if (fimc->variant->has_alpha)
> +			fimc_hw_set_rgb_alpha(ctx);
>  		clear_bit(ST_CAPT_APPLY_CFG, &fimc->state);
>  	}
>  	spin_unlock_irqrestore(&fimc->slock, flags);
> @@ -154,6 +156,8 @@ int fimc_capture_config_update(struct fimc_ctx *ctx)
>  		fimc_hw_set_rotation(ctx);
>  		fimc_prepare_dma_offset(ctx, &ctx->d_frame);
>  		fimc_hw_set_out_dma(ctx);
> +		if (fimc->variant->has_alpha)
> +			fimc_hw_set_rgb_alpha(ctx);
>  		clear_bit(ST_CAPT_APPLY_CFG, &fimc->state);
>  	}
>  	spin_unlock(&ctx->slock);
> diff --git a/drivers/media/video/s5p-fimc/fimc-core.c
> b/drivers/media/video/s5p-fimc/fimc-core.c index 567e9ea..5fe9aeb 100644
> --- a/drivers/media/video/s5p-fimc/fimc-core.c
> +++ b/drivers/media/video/s5p-fimc/fimc-core.c
> @@ -52,13 +52,29 @@ static struct fimc_fmt fimc_formats[] = {
>  		.colplanes	= 1,
>  		.flags		= FMT_FLAGS_M2M,
>  	}, {
> -		.name		= "XRGB-8-8-8-8, 32 bpp",
> +		.name		= "ARGB8888, 32 bpp",
>  		.fourcc		= V4L2_PIX_FMT_RGB32,
>  		.depth		= { 32 },
>  		.color		= S5P_FIMC_RGB888,
>  		.memplanes	= 1,
>  		.colplanes	= 1,
> -		.flags		= FMT_FLAGS_M2M,
> +		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
> +	}, {
> +		.name		= "ARGB1555",
> +		.fourcc		= V4L2_PIX_FMT_RGB555,
> +		.depth		= { 16 },
> +		.color		= S5P_FIMC_RGB555,
> +		.memplanes	= 1,
> +		.colplanes	= 1,
> +		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
> +	}, {
> +		.name		= "ARGB4444",
> +		.fourcc		= V4L2_PIX_FMT_RGB444,
> +		.depth		= { 16 },
> +		.color		= S5P_FIMC_RGB444,
> +		.memplanes	= 1,
> +		.colplanes	= 1,
> +		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
>  	}, {
>  		.name		= "YUV 4:2:2 packed, YCbYCr",
>  		.fourcc		= V4L2_PIX_FMT_YUYV,
> @@ -652,8 +668,11 @@ static void fimc_dma_run(void *priv)
>  	if (ctx->state & (FIMC_DST_ADDR | FIMC_PARAMS))
>  		fimc_hw_set_output_addr(fimc, &ctx->d_frame.paddr, -1);
> 
> -	if (ctx->state & FIMC_PARAMS)
> +	if (ctx->state & FIMC_PARAMS) {
>  		fimc_hw_set_out_dma(ctx);
> +		if (fimc->variant->has_alpha)
> +			fimc_hw_set_rgb_alpha(ctx);
> +	}
> 
>  	fimc_activate_capture(ctx);
> 
> @@ -790,6 +809,11 @@ static int fimc_s_ctrl(struct v4l2_ctrl *ctrl)
>  		ctx->rotation = ctrl->val;
>  		break;
> 
> +	case V4L2_CID_ALPHA_COMPONENT:
> +		spin_lock_irqsave(&ctx->slock, flags);
> +		ctx->d_frame.alpha = ctrl->val;
> +		break;
> +
>  	default:
>  		v4l2_err(fimc->v4l2_dev, "Invalid control: 0x%X\n", ctrl->id);
>  		return -EINVAL;
> @@ -806,9 +830,11 @@ static const struct v4l2_ctrl_ops fimc_ctrl_ops = {
> 
>  int fimc_ctrls_create(struct fimc_ctx *ctx)
>  {
> +	struct samsung_fimc_variant *variant = ctx->fimc_dev->variant;
> +
>  	if (ctx->ctrls_rdy)
>  		return 0;
> -	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
> +	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 4);
> 
>  	ctx->ctrl_rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler, 
&fimc_ctrl_ops,
>  				     V4L2_CID_HFLIP, 0, 1, 1, 0);
> @@ -816,6 +842,14 @@ int fimc_ctrls_create(struct fimc_ctx *ctx)
>  				    V4L2_CID_VFLIP, 0, 1, 1, 0);
>  	ctx->ctrl_vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler, 
&fimc_ctrl_ops,
>  				    V4L2_CID_ROTATE, 0, 270, 90, 0);
> +
> +	if (variant->has_alpha)
> +		ctx->ctrl_alpha = v4l2_ctrl_new_std(&ctx->ctrl_handler,
> +				    &fimc_ctrl_ops, V4L2_CID_ALPHA_COMPONENT,
> +				    0, 0xff, 1, 0);
> +	else
> +		ctx->ctrl_alpha = NULL;
> +
>  	ctx->ctrls_rdy = ctx->ctrl_handler.error == 0;
> 
>  	return ctx->ctrl_handler.error;
> @@ -838,6 +872,8 @@ void fimc_ctrls_activate(struct fimc_ctx *ctx, bool
> active) v4l2_ctrl_activate(ctx->ctrl_rotate, active);
>  	v4l2_ctrl_activate(ctx->ctrl_hflip, active);
>  	v4l2_ctrl_activate(ctx->ctrl_vflip, active);
> +	if (ctx->ctrl_alpha)
> +		v4l2_ctrl_activate(ctx->ctrl_alpha, active);
> 
>  	if (active) {
>  		ctx->rotation = ctx->ctrl_rotate->val;
> @@ -1374,6 +1410,8 @@ static int fimc_m2m_open(struct file *file)
>  	if (!ctx)
>  		return -ENOMEM;
>  	v4l2_fh_init(&ctx->fh, fimc->m2m.vfd);
> +	ctx->fimc_dev = fimc;
> +
>  	ret = fimc_ctrls_create(ctx);
>  	if (ret)
>  		goto error_fh;
> @@ -1383,7 +1421,6 @@ static int fimc_m2m_open(struct file *file)
>  	file->private_data = &ctx->fh;
>  	v4l2_fh_add(&ctx->fh);
> 
> -	ctx->fimc_dev = fimc;
>  	/* Default color format */
>  	ctx->s_frame.fmt = &fimc_formats[0];
>  	ctx->d_frame.fmt = &fimc_formats[0];
> @@ -1892,6 +1929,7 @@ static struct samsung_fimc_variant
> fimc0_variant_exynos4 = { .has_cam_if	 = 1,
>  	.has_cistatus2	 = 1,
>  	.has_mainscaler_ext = 1,
> +	.has_alpha	 = 1,
>  	.min_inp_pixsize = 16,
>  	.min_out_pixsize = 16,
>  	.hor_offs_align	 = 2,
> @@ -1905,6 +1943,7 @@ static struct samsung_fimc_variant
> fimc3_variant_exynos4 = { .has_cam_if	 = 1,
>  	.has_cistatus2	 = 1,
>  	.has_mainscaler_ext = 1,
> +	.has_alpha	 = 1,
>  	.min_inp_pixsize = 16,
>  	.min_out_pixsize = 16,
>  	.hor_offs_align	 = 2,
> diff --git a/drivers/media/video/s5p-fimc/fimc-core.h
> b/drivers/media/video/s5p-fimc/fimc-core.h index c7f01c4..9d1f669 100644
> --- a/drivers/media/video/s5p-fimc/fimc-core.h
> +++ b/drivers/media/video/s5p-fimc/fimc-core.h
> @@ -85,11 +85,14 @@ enum fimc_datapath {
>  };
> 
>  enum fimc_color_fmt {
> -	S5P_FIMC_RGB565 = 0x10,
> +	S5P_FIMC_RGB444 = 0x10,
> +	S5P_FIMC_RGB555,
> +	S5P_FIMC_RGB565,
>  	S5P_FIMC_RGB666,
>  	S5P_FIMC_RGB888,
>  	S5P_FIMC_RGB30_LOCAL,
>  	S5P_FIMC_YCBCR420 = 0x20,
> +	S5P_FIMC_YCBCR422,
>  	S5P_FIMC_YCBYCR422,
>  	S5P_FIMC_YCRYCB422,
>  	S5P_FIMC_CBYCRY422,
> @@ -162,6 +165,7 @@ struct fimc_fmt {
>  	u16	flags;
>  #define FMT_FLAGS_CAM	(1 << 0)
>  #define FMT_FLAGS_M2M	(1 << 1)
> +#define FMT_HAS_ALPHA	(1 << 2)
>  };
> 
>  /**
> @@ -283,6 +287,7 @@ struct fimc_frame {
>  	struct fimc_addr	paddr;
>  	struct fimc_dma_offset	dma_offset;
>  	struct fimc_fmt		*fmt;
> +	u8			alpha;
>  };
> 
>  /**
> @@ -387,6 +392,7 @@ struct samsung_fimc_variant {
>  	unsigned int	has_cistatus2:1;
>  	unsigned int	has_mainscaler_ext:1;
>  	unsigned int	has_cam_if:1;
> +	unsigned int	has_alpha:1;
>  	struct fimc_pix_limit *pix_limit;
>  	u16		min_inp_pixsize;
>  	u16		min_out_pixsize;
> @@ -482,7 +488,8 @@ struct fimc_dev {
>   * @ctrl_handler:	v4l2 controls handler
>   * @ctrl_rotate		image rotation control
>   * @ctrl_hflip		horizontal flip control
> - * @ctrl_vflip		vartical flip control
> + * @ctrl_vflip		vertical flip control
> + * @ctrl_alpha		RGB alpha control
>   * @ctrls_rdy:		true if the control handler is initialized
>   */
>  struct fimc_ctx {
> @@ -509,6 +516,7 @@ struct fimc_ctx {
>  	struct v4l2_ctrl	*ctrl_rotate;
>  	struct v4l2_ctrl	*ctrl_hflip;
>  	struct v4l2_ctrl	*ctrl_vflip;
> +	struct v4l2_ctrl	*ctrl_alpha;
>  	bool			ctrls_rdy;
>  };
> 
> @@ -674,6 +682,7 @@ void fimc_hw_set_prescaler(struct fimc_ctx *ctx);
>  void fimc_hw_set_mainscaler(struct fimc_ctx *ctx);
>  void fimc_hw_en_capture(struct fimc_ctx *ctx);
>  void fimc_hw_set_effect(struct fimc_ctx *ctx, bool active);
> +void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx);
>  void fimc_hw_set_in_dma(struct fimc_ctx *ctx);
>  void fimc_hw_set_input_path(struct fimc_ctx *ctx);
>  void fimc_hw_set_output_path(struct fimc_ctx *ctx);
> diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c
> b/drivers/media/video/s5p-fimc/fimc-reg.c index 44f5c2d..15466d0 100644
> --- a/drivers/media/video/s5p-fimc/fimc-reg.c
> +++ b/drivers/media/video/s5p-fimc/fimc-reg.c
> @@ -117,7 +117,7 @@ void fimc_hw_set_target_format(struct fimc_ctx *ctx)
>  		  S5P_CITRGFMT_VSIZE_MASK);
> 
>  	switch (frame->fmt->color) {
> -	case S5P_FIMC_RGB565...S5P_FIMC_RGB888:
> +	case S5P_FIMC_RGB444...S5P_FIMC_RGB888:
>  		cfg |= S5P_CITRGFMT_RGB;
>  		break;
>  	case S5P_FIMC_YCBCR420:
> @@ -175,6 +175,7 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
>  	struct fimc_dev *dev = ctx->fimc_dev;
>  	struct fimc_frame *frame = &ctx->d_frame;
>  	struct fimc_dma_offset *offset = &frame->dma_offset;
> +	struct fimc_fmt *fmt = frame->fmt;
> 
>  	/* Set the input dma offsets. */
>  	cfg = 0;
> @@ -198,15 +199,22 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
>  	cfg = readl(dev->regs + S5P_CIOCTRL);
> 
>  	cfg &= ~(S5P_CIOCTRL_ORDER2P_MASK | S5P_CIOCTRL_ORDER422_MASK |
> -		 S5P_CIOCTRL_YCBCR_PLANE_MASK);
> +		 S5P_CIOCTRL_YCBCR_PLANE_MASK | S5P_CIOCTRL_RGB16FMT_MASK);
> 
> -	if (frame->fmt->colplanes == 1)
> +	if (fmt->colplanes == 1)
>  		cfg |= ctx->out_order_1p;
> -	else if (frame->fmt->colplanes == 2)
> +	else if (fmt->colplanes == 2)
>  		cfg |= ctx->out_order_2p | S5P_CIOCTRL_YCBCR_2PLANE;
> -	else if (frame->fmt->colplanes == 3)
> +	else if (fmt->colplanes == 3)
>  		cfg |= S5P_CIOCTRL_YCBCR_3PLANE;
> 
> +	if (fmt->color == S5P_FIMC_RGB565)
> +		cfg |= S5P_CIOCTRL_RGB565;
> +	else if (fmt->color == S5P_FIMC_RGB555)
> +		cfg |= S5P_CIOCTRL_ARGB1555;
> +	else if (fmt->color == S5P_FIMC_RGB444)
> +		cfg |= S5P_CIOCTRL_ARGB4444;
> +
>  	writel(cfg, dev->regs + S5P_CIOCTRL);
>  }
> 
> @@ -278,22 +286,28 @@ static void fimc_hw_set_scaler(struct fimc_ctx *ctx)
>  	if (sc->copy_mode)
>  		cfg |= S5P_CISCCTRL_ONE2ONE;
> 
> -
>  	if (ctx->in_path == FIMC_DMA) {
> -		if (src_frame->fmt->color == S5P_FIMC_RGB565)
> +		switch (src_frame->fmt->color) {
> +		case S5P_FIMC_RGB565:
>  			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB565;
> -		else if (src_frame->fmt->color == S5P_FIMC_RGB666)
> +			break;
> +		case S5P_FIMC_RGB666:
>  			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB666;
> -		else if (src_frame->fmt->color == S5P_FIMC_RGB888)
> +			break;
> +		case S5P_FIMC_RGB888:
>  			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB888;
> +			break;
> +		}
>  	}
> 
>  	if (ctx->out_path == FIMC_DMA) {
> -		if (dst_frame->fmt->color == S5P_FIMC_RGB565)
> +		u32 color = dst_frame->fmt->color;
> +
> +		if (color >= S5P_FIMC_RGB444 && color <= S5P_FIMC_RGB565)
>  			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB565;
> -		else if (dst_frame->fmt->color == S5P_FIMC_RGB666)
> +		else if (color == S5P_FIMC_RGB666)
>  			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB666;
> -		else if (dst_frame->fmt->color == S5P_FIMC_RGB888)
> +		else if (color == S5P_FIMC_RGB888)
>  			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
>  	} else {
>  		cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
> @@ -379,6 +393,21 @@ void fimc_hw_set_effect(struct fimc_ctx *ctx, bool
> active) writel(cfg, dev->regs + S5P_CIIMGEFF);
>  }
> 
> +void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx)
> +{
> +	struct fimc_dev *dev = ctx->fimc_dev;
> +	struct fimc_frame *frame = &ctx->d_frame;
> +	u32 cfg;
> +
> +	if (!(frame->fmt->flags & FMT_HAS_ALPHA))
> +		return;
> +
> +	cfg = readl(dev->regs + S5P_CIOCTRL);
> +	cfg &= ~S5P_CIOCTRL_ALPHA_OUT_MASK;
> +	cfg |= (frame->alpha << 4);
> +	writel(cfg, dev->regs + S5P_CIOCTRL);
> +}
> +
>  static void fimc_hw_set_in_dma_size(struct fimc_ctx *ctx)
>  {
>  	struct fimc_dev *dev = ctx->fimc_dev;
> diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h
> b/drivers/media/video/s5p-fimc/regs-fimc.h index c8e3b94..c7a5bc5 100644
> --- a/drivers/media/video/s5p-fimc/regs-fimc.h
> +++ b/drivers/media/video/s5p-fimc/regs-fimc.h
> @@ -107,6 +107,11 @@
>  #define S5P_CIOCTRL_YCBCR_3PLANE	(0 << 3)
>  #define S5P_CIOCTRL_YCBCR_2PLANE	(1 << 3)
>  #define S5P_CIOCTRL_YCBCR_PLANE_MASK	(1 << 3)
> +#define S5P_CIOCTRL_ALPHA_OUT_MASK	(0xff << 4)
> +#define S5P_CIOCTRL_RGB16FMT_MASK	(3 << 16)
> +#define S5P_CIOCTRL_RGB565		(0 << 16)
> +#define S5P_CIOCTRL_ARGB1555		(1 << 16)
> +#define S5P_CIOCTRL_ARGB4444		(2 << 16)
>  #define S5P_CIOCTRL_ORDER2P_SHIFT	(24)
>  #define S5P_CIOCTRL_ORDER2P_MASK	(3 << 24)
>  #define S5P_CIOCTRL_ORDER422_2P_LSB_CRCB (0 << 24)

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
>> This control is intended for the video capture or memory-to-memory devices
>> that are capable of setting up a per-pixel alpha component to some
>> arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to set the
>> alpha component for all pixels to a value in range from 0 to 255.
>>
>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>> ---
>>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
>>  Documentation/DocBook/media/v4l/controls.xml       |   25
>> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |  
>>  7 ++++-
>>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
>>  include/linux/videodev2.h                          |    6 ++--
>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
...
>>  	/* MPEG controls */
>>  	/* Keep the order of the 'case's the same as in videodev2.h! */
>> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
>> v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is 2^24 */
>>  		*max = 0xFFFFFF;
>>  		break;
>> +	case V4L2_CID_ALPHA_COMPONENT:
>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>> +		*step = 1;
>> +		*min = 0;
>> +		*max = 0xff;
>> +		break;
> 
> Hmm. Do we really want to fix the max value to 0xff? The bits assigned to the
> alpha component will vary between 1 (V4L2_PIX_FMT_RGB555X), 4 
> (V4L2_PIX_FMT_RGB444) or 8 (V4L2_PIX_FMT_RGB32). It wouldn't surprise me to
> see larger sizes as well in the future (e.g. 16 bits).
> 
> I think the max value should be the largest alpha value the hardware can 
> support. The application has to set it to the right value that corresponds
> to the currently chosen pixel format. The driver just copies the first N bits 
> into the alpha value where N depends on the pixel format.
> 
> what do you think?

Yes, ideally the maximum value of the alpha control should be changing depending
on the set colour format.
Currently the maximum value of the control equals maximum alpha value for the fourcc
of maximum colour depth (V4L2_PIX_FMT_RGB32).

What I found missing was a method for changing the control range dynamically, without 
deleting and re-initializing the control handler.
If we reinitalize whole control handler the previously set control values are lost.

And, AFAIU, single control cannot be currently removed and re-added to the control 
handler.

--

Regards,
Sylwester

Re: [PATCH v2 2/2] s5p-fimc: Add support for alpha component configuration

[Sylwester Nawrocki]

On 11/28/2011 12:42 PM, Hans Verkuil wrote:
> On Friday 25 November 2011 16:39:32 Sylwester Nawrocki wrote:
>> On Exynos SoCs the FIMC IP allows to configure globally the alpha
>> component of all pixels for V4L2_PIX_FMT_RGB32, V4L2_PIX_FMT_RGB555
>> and V4L2_PIX_FMT_RGB444 image formats. This patch adds a v4l2 control
>> in order to let the applications control the alpha component value.
>>
>> The alpha value range depends on the pixel format, for RGB32 it's
>> 0..255 (8-bits), for RGB555 - 0..1 (1-bit) and for RGB444 - 0..15
>> (4-bits). The v4l2 control range is always 0..255 and the alpha
>> component data width is determined by currently set format on the
>> V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE buffer queue. The applications
>> need to match the alpha channel data width and the pixel format
>> since the driver will ignore the alpha component bits that are not
>> applicable to the configured pixel format.
> 
> Will the driver ignore the least significant bits or the most significant 
> bits?

Most significant bits will be ignored, i.e. depending on fourcc the valid
alpha bits are:

V4L2_PIX_FMT_RGB555 - [0]
V4L2_PIX_FMT_RGB444 - [3:0]
V4L2_PIX_FMT_RGB32  - [7:0]

--

Regards,
Sylwester

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> > On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> >> This control is intended for the video capture or memory-to-memory
> >> devices that are capable of setting up a per-pixel alpha component to
> >> some arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to
> >> set the alpha component for all pixels to a value in range from 0 to
> >> 255.
> >> 
> >> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >> ---
> >> 
> >>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
> >>  Documentation/DocBook/media/v4l/controls.xml       |   25
> >> 
> >> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |
> >> 
> >>  7 ++++-
> >>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
> >>  include/linux/videodev2.h                          |    6 ++--
> >>  5 files changed, 45 insertions(+), 11 deletions(-)
> 
> ...
> 
> >>  	/* MPEG controls */
> >>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> >> 
> >> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
> >> v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is 2^24 */
> >> 
> >>  		*max = 0xFFFFFF;
> >>  		break;
> >> 
> >> +	case V4L2_CID_ALPHA_COMPONENT:
> >> +		*type = V4L2_CTRL_TYPE_INTEGER;
> >> +		*step = 1;
> >> +		*min = 0;
> >> +		*max = 0xff;
> >> +		break;
> > 
> > Hmm. Do we really want to fix the max value to 0xff? The bits assigned to
> > the alpha component will vary between 1 (V4L2_PIX_FMT_RGB555X), 4
> > (V4L2_PIX_FMT_RGB444) or 8 (V4L2_PIX_FMT_RGB32). It wouldn't surprise me
> > to see larger sizes as well in the future (e.g. 16 bits).
> > 
> > I think the max value should be the largest alpha value the hardware can
> > support. The application has to set it to the right value that
> > corresponds to the currently chosen pixel format. The driver just copies
> > the first N bits into the alpha value where N depends on the pixel
> > format.
> > 
> > what do you think?
> 
> Yes, ideally the maximum value of the alpha control should be changing
> depending on the set colour format.
> Currently the maximum value of the control equals maximum alpha value for
> the fourcc of maximum colour depth (V4L2_PIX_FMT_RGB32).
> 
> What I found missing was a method for changing the control range
> dynamically, without deleting and re-initializing the control handler.
> If we reinitalize whole control handler the previously set control values
> are lost.

You can just change the maximum field of struct v4l2_ctrl on the fly like 
this:

struct v4l2_ctrl *my_ctrl;

v4l2_ctrl_lock(my_ctrl);
my_ctrl->maximum = 10;
if (my_ctrl->cur.val > my_ctrl->maximum)
	my_ctrl->cur.val = my_ctrl->maximum;
v4l2_ctrl_unlock(my_ctrl);

Although this might warrant a v4l2_ctrl_update_range() function that does this
for you. Because after a change like this a V4L2_EVENT_CTRL should also be 
sent.

In any case, this functionality isn't hard to add. Just let me know if you 
need it and I can make a patch for this.

Regards,

	Hans

> 
> And, AFAIU, single control cannot be currently removed and re-added to the
> control handler.
> 
> --
> 
> Regards,
> Sylwester
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
>>>> This control is intended for the video capture or memory-to-memory
>>>> devices that are capable of setting up a per-pixel alpha component to
>>>> some arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to
>>>> set the alpha component for all pixels to a value in range from 0 to
>>>> 255.
>>>>
>>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
>>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
>>>> ---
>>>>
>>>>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
>>>>  Documentation/DocBook/media/v4l/controls.xml       |   25
>>>>
>>>> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |
>>>>
>>>>  7 ++++-
>>>>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
>>>>  include/linux/videodev2.h                          |    6 ++--
>>>>  5 files changed, 45 insertions(+), 11 deletions(-)
>>
>> ...
>>
>>>>  	/* MPEG controls */
>>>>  	/* Keep the order of the 'case's the same as in videodev2.h! */
>>>>
>>>> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum
>>>> v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is 2^24 */
>>>>
>>>>  		*max = 0xFFFFFF;
>>>>  		break;
>>>>
>>>> +	case V4L2_CID_ALPHA_COMPONENT:
>>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
>>>> +		*step = 1;
>>>> +		*min = 0;
>>>> +		*max = 0xff;
>>>> +		break;
>>>
>>> Hmm. Do we really want to fix the max value to 0xff? The bits assigned to
>>> the alpha component will vary between 1 (V4L2_PIX_FMT_RGB555X), 4
>>> (V4L2_PIX_FMT_RGB444) or 8 (V4L2_PIX_FMT_RGB32). It wouldn't surprise me
>>> to see larger sizes as well in the future (e.g. 16 bits).
>>>
>>> I think the max value should be the largest alpha value the hardware can
>>> support. The application has to set it to the right value that
>>> corresponds to the currently chosen pixel format. The driver just copies
>>> the first N bits into the alpha value where N depends on the pixel
>>> format.
>>>
>>> what do you think?
>>
>> Yes, ideally the maximum value of the alpha control should be changing
>> depending on the set colour format.
>> Currently the maximum value of the control equals maximum alpha value for
>> the fourcc of maximum colour depth (V4L2_PIX_FMT_RGB32).
>>
>> What I found missing was a method for changing the control range
>> dynamically, without deleting and re-initializing the control handler.
>> If we reinitalize whole control handler the previously set control values
>> are lost.
> 
> You can just change the maximum field of struct v4l2_ctrl on the fly like 
> this:
> 
> struct v4l2_ctrl *my_ctrl;
> 
> v4l2_ctrl_lock(my_ctrl);
> my_ctrl->maximum = 10;
> if (my_ctrl->cur.val > my_ctrl->maximum)
> 	my_ctrl->cur.val = my_ctrl->maximum;
> v4l2_ctrl_unlock(my_ctrl);
> 
> Although this might warrant a v4l2_ctrl_update_range() function that does this
> for you. Because after a change like this a V4L2_EVENT_CTRL should also be 
> sent.
> 
> In any case, this functionality isn't hard to add. Just let me know if you 
> need it and I can make a patch for this.

Yes, it would be great if you could prepare a patch for v4l2_ctrl_update_range().
Then I could use it in the next iteration of the patches, instead of hacking 
at the driver. IIRC it's not the first time we needed changing the control range
dynamically.

--

Thanks!
Sylwester

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> > On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> >> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> >>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> >>>> This control is intended for the video capture or memory-to-memory
> >>>> devices that are capable of setting up a per-pixel alpha component to
> >>>> some arbitrary value. The V4L2_CID_ALPHA_COMPONENT control allows to
> >>>> set the alpha component for all pixels to a value in range from 0 to
> >>>> 255.
> >>>> 
> >>>> Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
> >>>> Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
> >>>> ---
> >>>> 
> >>>>  Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
> >>>>  Documentation/DocBook/media/v4l/controls.xml       |   25
> >>>> 
> >>>> +++++++++++++++---- .../DocBook/media/v4l/pixfmt-packed-rgb.xml       
> >>>> |
> >>>> 
> >>>>  7 ++++-
> >>>>  drivers/media/video/v4l2-ctrls.c                   |    7 +++++
> >>>>  include/linux/videodev2.h                          |    6 ++--
> >>>>  5 files changed, 45 insertions(+), 11 deletions(-)
> >> 
> >> ...
> >> 
> >>>>  	/* MPEG controls */
> >>>>  	/* Keep the order of the 'case's the same as in videodev2.h! */
> >>>> 
> >>>> @@ -714,6 +715,12 @@ void v4l2_ctrl_fill(u32 id, const char **name,
> >>>> enum v4l2_ctrl_type *type, /* Max is calculated as RGB888 that is
> >>>> 2^24 */
> >>>> 
> >>>>  		*max = 0xFFFFFF;
> >>>>  		break;
> >>>> 
> >>>> +	case V4L2_CID_ALPHA_COMPONENT:
> >>>> +		*type = V4L2_CTRL_TYPE_INTEGER;
> >>>> +		*step = 1;
> >>>> +		*min = 0;
> >>>> +		*max = 0xff;
> >>>> +		break;
> >>> 
> >>> Hmm. Do we really want to fix the max value to 0xff? The bits assigned
> >>> to the alpha component will vary between 1 (V4L2_PIX_FMT_RGB555X), 4
> >>> (V4L2_PIX_FMT_RGB444) or 8 (V4L2_PIX_FMT_RGB32). It wouldn't surprise
> >>> me to see larger sizes as well in the future (e.g. 16 bits).
> >>> 
> >>> I think the max value should be the largest alpha value the hardware
> >>> can support. The application has to set it to the right value that
> >>> corresponds to the currently chosen pixel format. The driver just
> >>> copies the first N bits into the alpha value where N depends on the
> >>> pixel format.
> >>> 
> >>> what do you think?
> >> 
> >> Yes, ideally the maximum value of the alpha control should be changing
> >> depending on the set colour format.
> >> Currently the maximum value of the control equals maximum alpha value
> >> for the fourcc of maximum colour depth (V4L2_PIX_FMT_RGB32).
> >> 
> >> What I found missing was a method for changing the control range
> >> dynamically, without deleting and re-initializing the control handler.
> >> If we reinitalize whole control handler the previously set control
> >> values are lost.
> > 
> > You can just change the maximum field of struct v4l2_ctrl on the fly like
> > this:
> > diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
> > struct v4l2_ctrl *my_ctrl;
> > 
> > v4l2_ctrl_lock(my_ctrl);
> > my_ctrl->maximum = 10;
> > if (my_ctrl->cur.val > my_ctrl->maximum)
> > 
> > 	my_ctrl->cur.val = my_ctrl->maximum;
> > 
> > v4l2_ctrl_unlock(my_ctrl);
> > 
> > Although this might warrant a v4l2_ctrl_update_range() function that does
> > this for you. Because after a change like this a V4L2_EVENT_CTRL should
> > also be sent.
> > 
> > In any case, this functionality isn't hard to add. Just let me know if
> > you need it and I can make a patch for this.
> 
> Yes, it would be great if you could prepare a patch for
> v4l2_ctrl_update_range(). Then I could use it in the next iteration of the
> patches, instead of hacking at the driver. IIRC it's not the first time we
> needed changing the control range dynamically.

Here is a patch that updates the range. It also sends a control event telling
any listener that the range has changed. Tested with vivi and a modified
v4l2-ctl.

The only thing missing is a DocBook entry for that new event flag and perhaps
some more documentation in places.

Let me know how this works for you, and if it is really needed, then I can
add it to the control framework.

Regards,

	Hans

index 0f415da..d7ca646 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -913,8 +913,7 @@ static int new_to_user(struct v4l2_ext_control *c,
 }
 
 /* Copy the new value to the current value. */
-static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
-						bool update_inactive)
+static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
 {
 	bool changed = false;
 
@@ -938,8 +937,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 		ctrl->cur.val = ctrl->val;
 		break;
 	}
-	if (update_inactive) {
-		/* Note: update_inactive can only be true for auto clusters. */
+	if (ch_flags & V4L2_EVENT_CTRL_CH_FLAGS) {
+		/* Note: CH_FLAGS is only set for auto clusters. */
 		ctrl->flags &=
 			~(V4L2_CTRL_FLAG_INACTIVE | V4L2_CTRL_FLAG_VOLATILE);
 		if (!is_cur_manual(ctrl->cluster[0])) {
@@ -949,14 +948,13 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
 		}
 		fh = NULL;
 	}
-	if (changed || update_inactive) {
+	if (changed || ch_flags) {
 		/* If a control was changed that was not one of the controls
 		   modified by the application, then send the event to all. */
 		if (!ctrl->is_new)
 			fh = NULL;
 		send_event(fh, ctrl,
-			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
-			(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
+			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) | ch_flags);
 	}
 }
 
@@ -1290,6 +1288,40 @@ unlock:
 	return 0;
 }
 
+/* Control range checking */
+static int check_range(enum v4l2_ctrl_type type,
+		s32 min, s32 max, u32 step, s32 def)
+{
+	switch (type) {
+	case V4L2_CTRL_TYPE_BOOLEAN:
+		if (step != 1 || max > 1 || min < 0)
+			return -ERANGE;
+		/* fall through */
+	case V4L2_CTRL_TYPE_INTEGER:
+		if (step <= 0 || min > max || def < min || def > max)
+			return -ERANGE;
+		return 0;
+	case V4L2_CTRL_TYPE_BITMASK:
+		if (step || min || !max || (def & ~max))
+			return -ERANGE;
+		return 0;
+	case V4L2_CTRL_TYPE_MENU:
+		if (min > max || def < min || def > max)
+			return -ERANGE;
+		/* Note: step == menu_skip_mask for menu controls.
+		   So here we check if the default value is masked out. */
+		if (step && ((1 << def) & step))
+			return -EINVAL;
+		return 0;
+	case V4L2_CTRL_TYPE_STRING:
+		if (min > max || min < 0 || step < 1 || def)
+			return -ERANGE;
+		return 0;
+	default:
+		return 0;
+	}
+}
+
 /* Add a new control */
 static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
@@ -1299,32 +1331,20 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
 {
 	struct v4l2_ctrl *ctrl;
 	unsigned sz_extra = 0;
+	int err;
 
 	if (hdl->error)
 		return NULL;
 
 	/* Sanity checks */
 	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
-	    (type == V4L2_CTRL_TYPE_INTEGER && step == 0) ||
-	    (type == V4L2_CTRL_TYPE_BITMASK && max == 0) ||
-	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
-	    (type == V4L2_CTRL_TYPE_STRING && max == 0)) {
-		handler_set_err(hdl, -ERANGE);
-		return NULL;
-	}
-	if (type != V4L2_CTRL_TYPE_BITMASK && max < min) {
+	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL)) {
 		handler_set_err(hdl, -ERANGE);
 		return NULL;
 	}
-	if ((type == V4L2_CTRL_TYPE_INTEGER ||
-	     type == V4L2_CTRL_TYPE_MENU ||
-	     type == V4L2_CTRL_TYPE_BOOLEAN) &&
-	    (def < min || def > max)) {
-		handler_set_err(hdl, -ERANGE);
-		return NULL;
-	}
-	if (type == V4L2_CTRL_TYPE_BITMASK && ((def & ~max) || min || step)) {
-		handler_set_err(hdl, -ERANGE);
+	err = check_range(type, min, max, step, def);
+	if (err) {
+		handler_set_err(hdl, err);
 		return NULL;
 	}
 
@@ -2062,7 +2082,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
    copied to the current value on a set.
    Must be called with ctrl->handler->lock held. */
 static int try_or_set_cluster(struct v4l2_fh *fh,
-			      struct v4l2_ctrl *master, bool set)
+			      struct v4l2_ctrl *master, bool set, u32 ch_flags)
 {
 	bool update_flag;
 	int ret;
@@ -2100,7 +2120,8 @@ static int try_or_set_cluster(struct v4l2_fh *fh,
 	/* If OK, then make the new values permanent. */
 	update_flag = is_cur_manual(master) != is_new_manual(master);
 	for (i = 0; i < master->ncontrols; i++)
-		new_to_cur(fh, master->cluster[i], update_flag && i > 0);
+		new_to_cur(fh, master->cluster[i], ch_flags |
+			((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
 	return 0;
 }
 
@@ -2226,7 +2247,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 		} while (!ret && idx);
 
 		if (!ret)
-			ret = try_or_set_cluster(fh, master, set);
+			ret = try_or_set_cluster(fh, master, set, 0);
 
 		/* Copy the new values back to userspace. */
 		if (!ret) {
@@ -2271,18 +2292,12 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
 EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
 
 /* Helper function for VIDIOC_S_CTRL compatibility */
-static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
+static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
+		    s32 val, u32 ch_flags)
 {
 	struct v4l2_ctrl *master = ctrl->cluster[0];
-	int ret;
 	int i;
 
-	ret = validate_new_int(ctrl, val);
-	if (ret)
-		return ret;
-
-	v4l2_ctrl_lock(ctrl);
-
 	/* Reset the 'is_new' flags of the cluster */
 	for (i = 0; i < master->ncontrols; i++)
 		if (master->cluster[i])
@@ -2292,13 +2307,25 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
 	   manual mode we have to update the current volatile values since
 	   those will become the initial manual values after such a switch. */
 	if (master->is_auto && master->has_volatiles && ctrl == master &&
-	    !is_cur_manual(master) && *val == master->manual_mode_value)
+	    !is_cur_manual(master) && val == master->manual_mode_value)
 		update_from_auto_cluster(master);
-	ctrl->val = *val;
+	ctrl->val = val;
 	ctrl->is_new = 1;
-	ret = try_or_set_cluster(fh, master, true);
-	*val = ctrl->cur.val;
-	v4l2_ctrl_unlock(ctrl);
+	return try_or_set_cluster(fh, master, true, ch_flags);
+}
+
+/* Helper function for VIDIOC_S_CTRL compatibility */
+static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
+{
+	int ret = validate_new_int(ctrl, val);
+
+	if (!ret) {
+		v4l2_ctrl_lock(ctrl);
+		ret = set_ctrl(fh, ctrl, *val, 0);
+		if (!ret)
+			*val = ctrl->cur.val;
+		v4l2_ctrl_unlock(ctrl);
+	}
 	return ret;
 }
 
@@ -2313,7 +2340,7 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
 	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
 		return -EACCES;
 
-	return set_ctrl(fh, ctrl, &control->value);
+	return set_ctrl_lock(fh, ctrl, &control->value);
 }
 EXPORT_SYMBOL(v4l2_s_ctrl);
 
@@ -2327,10 +2354,44 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
 {
 	/* It's a driver bug if this happens. */
 	WARN_ON(!type_is_int(ctrl));
-	return set_ctrl(NULL, ctrl, &val);
+	return set_ctrl_lock(NULL, ctrl, &val);
 }
 EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
 
+int v4l2_ctrl_update_range(struct v4l2_ctrl *ctrl,
+			s32 min, s32 max, u32 step, s32 def)
+{
+	int ret = check_range(ctrl->type, min, max, step, def);
+	s32 val;
+
+	switch (ctrl->type) {
+	case V4L2_CTRL_TYPE_INTEGER:
+	case V4L2_CTRL_TYPE_BOOLEAN:
+	case V4L2_CTRL_TYPE_MENU:
+	case V4L2_CTRL_TYPE_BITMASK:
+		if (ret)
+			return ret;
+		break;
+	default:
+		return -EINVAL;
+	}
+	v4l2_ctrl_lock(ctrl);
+	ctrl->minimum = min;
+	ctrl->maximum = max;
+	ctrl->step = step;
+	ctrl->default_value = def;
+	val = ctrl->cur.val;
+	if (validate_new_int(ctrl, &val))
+		val = def;
+	if (val != ctrl->cur.val)
+		ret = set_ctrl(NULL, ctrl, val, V4L2_EVENT_CTRL_CH_RANGE);
+	else
+		send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
+	v4l2_ctrl_unlock(ctrl);
+	return ret;
+}
+EXPORT_SYMBOL(v4l2_ctrl_update_range);
+
 void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
 				struct v4l2_subscribed_event *sev)
 {
@@ -2339,7 +2400,7 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
 	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
 	    (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
 		struct v4l2_event ev;
-		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
+		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS | V4L2_EVENT_CTRL_CH_RANGE;
 
 		if (!(ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY))
 			changes |= V4L2_EVENT_CTRL_CH_VALUE;
diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
index 7d754fb..fd89106 100644
--- a/drivers/media/video/vivi.c
+++ b/drivers/media/video/vivi.c
@@ -190,6 +190,7 @@ struct vivi_dev {
 	unsigned 		   ms;
 	unsigned long              jiffies;
 	unsigned		   button_pressed;
+	bool			   toggle_range;
 
 	int			   mv_count;	/* Controls bars movement */
 
@@ -545,6 +546,17 @@ static void vivi_thread_tick(struct vivi_dev *dev)
 	vivi_fillbuff(dev, buf);
 	dprintk(dev, 1, "filled buffer %p\n", buf);
 
+	if (dev->toggle_range) {
+		static bool toggle;
+
+		dev->toggle_range = false;
+		if (toggle)
+			v4l2_ctrl_update_range(dev->contrast, 0, 255, 1, 16);
+		else
+			v4l2_ctrl_update_range(dev->contrast, 128, 255, 2, 150);
+		toggle = !toggle;
+	}
+
 	vb2_buffer_done(&buf->vb, VB2_BUF_STATE_DONE);
 	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
 }
@@ -1034,8 +1046,10 @@ static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
 {
 	struct vivi_dev *dev = container_of(ctrl->handler, struct vivi_dev, ctrl_handler);
 
-	if (ctrl == dev->button)
+	if (ctrl == dev->button) {
 		dev->button_pressed = 30;
+		dev->toggle_range = true;
+	}
 	return 0;
 }
 
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 4b752d5..22e632a 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -2063,6 +2063,7 @@ struct v4l2_event_vsync {
 /* Payload for V4L2_EVENT_CTRL */
 #define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
 #define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
+#define V4L2_EVENT_CTRL_CH_RANGE		(1 << 2)
 
 struct v4l2_event_ctrl {
 	__u32 changes;
diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
index eeb3df6..69ea0d5 100644
--- a/include/media/v4l2-ctrls.h
+++ b/include/media/v4l2-ctrls.h
@@ -445,6 +445,23 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
   */
 void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
 
+/** v4l2_ctrl_update_range() - Update the range of a control.
+  * @ctrl:	The control to update.
+  * @min:	The control's minimum value.
+  * @max:	The control's maximum value.
+  * @step:	The control's step value
+  * @def: 	The control's default value.
+  *
+  * Update the range of a control on the fly. This works for control types
+  * INTEGER, BOOLEAN, MENU and BITMASK. For menu controls the @step value
+  * is interpreted as a menu_skip_mask.
+  *
+  * An error is returned if one of the range arguments is invalid for this
+  * control type.
+  */
+int v4l2_ctrl_update_range(struct v4l2_ctrl *ctrl,
+			s32 min, s32 max, u32 step, s32 def);
+
 /** v4l2_ctrl_lock() - Helper function to lock the handler
   * associated with the control.
   * @ctrl:	The control to lock.

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

Hi Hans,

On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> 
> Here is a patch that updates the range. It also sends a control event telling
> any listener that the range has changed. Tested with vivi and a modified
> v4l2-ctl.
> 
> The only thing missing is a DocBook entry for that new event flag and perhaps
> some more documentation in places.
> 
> Let me know how this works for you, and if it is really needed, then I can
> add it to the control framework.

Thanks for your work, it's very appreciated.

I've tested the patch with s5p-fimc and it works well. I just didn't check
the event part yet.

I spoke to Kamil as in the past he considered the control range updating
at the codec driver. But since separate controls are used for different
encoding standards, this is not needed it any more.

Nevertheless I have at least two use cases, for the alpha control and
for the image sensor driver. In case of the camera sensor, different device
revisions may have different step and maximum value for some controls,
depending on firmware.
By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
start-up procedure just to find out properties of some controls.

It would be nice to have this enhancement in mainline.

> 
> Regards,
> 
> 	Hans
> 
> index 0f415da..d7ca646 100644
> --- a/drivers/media/video/v4l2-ctrls.c
> +++ b/drivers/media/video/v4l2-ctrls.c
> @@ -913,8 +913,7 @@ static int new_to_user(struct v4l2_ext_control *c,
>  }
>  
>  /* Copy the new value to the current value. */
> -static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> -						bool update_inactive)
> +static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, u32 ch_flags)
>  {
>  	bool changed = false;
>  
> @@ -938,8 +937,8 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>  		ctrl->cur.val = ctrl->val;
>  		break;
>  	}
> -	if (update_inactive) {
> -		/* Note: update_inactive can only be true for auto clusters. */
> +	if (ch_flags & V4L2_EVENT_CTRL_CH_FLAGS) {
> +		/* Note: CH_FLAGS is only set for auto clusters. */
>  		ctrl->flags &=
>  			~(V4L2_CTRL_FLAG_INACTIVE | V4L2_CTRL_FLAG_VOLATILE);
>  		if (!is_cur_manual(ctrl->cluster[0])) {
> @@ -949,14 +948,13 @@ static void new_to_cur(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
>  		}
>  		fh = NULL;
>  	}
> -	if (changed || update_inactive) {
> +	if (changed || ch_flags) {
>  		/* If a control was changed that was not one of the controls
>  		   modified by the application, then send the event to all. */
>  		if (!ctrl->is_new)
>  			fh = NULL;
>  		send_event(fh, ctrl,
> -			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) |
> -			(update_inactive ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
> +			(changed ? V4L2_EVENT_CTRL_CH_VALUE : 0) | ch_flags);
>  	}
>  }
>  
> @@ -1290,6 +1288,40 @@ unlock:
>  	return 0;
>  }
>  
> +/* Control range checking */
> +static int check_range(enum v4l2_ctrl_type type,
> +		s32 min, s32 max, u32 step, s32 def)
> +{
> +	switch (type) {
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +		if (step != 1 || max > 1 || min < 0)
> +			return -ERANGE;
> +		/* fall through */
> +	case V4L2_CTRL_TYPE_INTEGER:
> +		if (step <= 0 || min > max || def < min || def > max)
> +			return -ERANGE;
> +		return 0;
> +	case V4L2_CTRL_TYPE_BITMASK:
> +		if (step || min || !max || (def & ~max))
> +			return -ERANGE;
> +		return 0;
> +	case V4L2_CTRL_TYPE_MENU:
> +		if (min > max || def < min || def > max)
> +			return -ERANGE;
> +		/* Note: step == menu_skip_mask for menu controls.
> +		   So here we check if the default value is masked out. */
> +		if (step && ((1 << def) & step))
> +			return -EINVAL;
> +		return 0;
> +	case V4L2_CTRL_TYPE_STRING:
> +		if (min > max || min < 0 || step < 1 || def)
> +			return -ERANGE;
> +		return 0;
> +	default:
> +		return 0;
> +	}
> +}
> +
>  /* Add a new control */
>  static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  			const struct v4l2_ctrl_ops *ops,
> @@ -1299,32 +1331,20 @@ static struct v4l2_ctrl *v4l2_ctrl_new(struct v4l2_ctrl_handler *hdl,
>  {
>  	struct v4l2_ctrl *ctrl;
>  	unsigned sz_extra = 0;
> +	int err;
>  
>  	if (hdl->error)
>  		return NULL;
>  
>  	/* Sanity checks */
>  	if (id == 0 || name == NULL || id >= V4L2_CID_PRIVATE_BASE ||
> -	    (type == V4L2_CTRL_TYPE_INTEGER && step == 0) ||
> -	    (type == V4L2_CTRL_TYPE_BITMASK && max == 0) ||
> -	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL) ||
> -	    (type == V4L2_CTRL_TYPE_STRING && max == 0)) {
> -		handler_set_err(hdl, -ERANGE);
> -		return NULL;
> -	}
> -	if (type != V4L2_CTRL_TYPE_BITMASK && max < min) {
> +	    (type == V4L2_CTRL_TYPE_MENU && qmenu == NULL)) {
>  		handler_set_err(hdl, -ERANGE);
>  		return NULL;
>  	}
> -	if ((type == V4L2_CTRL_TYPE_INTEGER ||
> -	     type == V4L2_CTRL_TYPE_MENU ||
> -	     type == V4L2_CTRL_TYPE_BOOLEAN) &&
> -	    (def < min || def > max)) {
> -		handler_set_err(hdl, -ERANGE);
> -		return NULL;
> -	}
> -	if (type == V4L2_CTRL_TYPE_BITMASK && ((def & ~max) || min || step)) {
> -		handler_set_err(hdl, -ERANGE);
> +	err = check_range(type, min, max, step, def);
> +	if (err) {
> +		handler_set_err(hdl, err);
>  		return NULL;
>  	}
>  
> @@ -2062,7 +2082,7 @@ EXPORT_SYMBOL(v4l2_ctrl_g_ctrl);
>     copied to the current value on a set.
>     Must be called with ctrl->handler->lock held. */
>  static int try_or_set_cluster(struct v4l2_fh *fh,
> -			      struct v4l2_ctrl *master, bool set)
> +			      struct v4l2_ctrl *master, bool set, u32 ch_flags)
>  {
>  	bool update_flag;
>  	int ret;
> @@ -2100,7 +2120,8 @@ static int try_or_set_cluster(struct v4l2_fh *fh,
>  	/* If OK, then make the new values permanent. */
>  	update_flag = is_cur_manual(master) != is_new_manual(master);
>  	for (i = 0; i < master->ncontrols; i++)
> -		new_to_cur(fh, master->cluster[i], update_flag && i > 0);
> +		new_to_cur(fh, master->cluster[i], ch_flags |
> +			((update_flag && i > 0) ? V4L2_EVENT_CTRL_CH_FLAGS : 0));
>  	return 0;
>  }
>  
> @@ -2226,7 +2247,7 @@ static int try_set_ext_ctrls(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  		} while (!ret && idx);
>  
>  		if (!ret)
> -			ret = try_or_set_cluster(fh, master, set);
> +			ret = try_or_set_cluster(fh, master, set, 0);
>  
>  		/* Copy the new values back to userspace. */
>  		if (!ret) {
> @@ -2271,18 +2292,12 @@ int v4l2_subdev_s_ext_ctrls(struct v4l2_subdev *sd, struct v4l2_ext_controls *cs
>  EXPORT_SYMBOL(v4l2_subdev_s_ext_ctrls);
>  
>  /* Helper function for VIDIOC_S_CTRL compatibility */
> -static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
> +static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl,
> +		    s32 val, u32 ch_flags)
>  {
>  	struct v4l2_ctrl *master = ctrl->cluster[0];
> -	int ret;
>  	int i;
>  
> -	ret = validate_new_int(ctrl, val);
> -	if (ret)
> -		return ret;
> -
> -	v4l2_ctrl_lock(ctrl);
> -
>  	/* Reset the 'is_new' flags of the cluster */
>  	for (i = 0; i < master->ncontrols; i++)
>  		if (master->cluster[i])
> @@ -2292,13 +2307,25 @@ static int set_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
>  	   manual mode we have to update the current volatile values since
>  	   those will become the initial manual values after such a switch. */
>  	if (master->is_auto && master->has_volatiles && ctrl == master &&
> -	    !is_cur_manual(master) && *val == master->manual_mode_value)
> +	    !is_cur_manual(master) && val == master->manual_mode_value)
>  		update_from_auto_cluster(master);
> -	ctrl->val = *val;
> +	ctrl->val = val;
>  	ctrl->is_new = 1;
> -	ret = try_or_set_cluster(fh, master, true);
> -	*val = ctrl->cur.val;
> -	v4l2_ctrl_unlock(ctrl);
> +	return try_or_set_cluster(fh, master, true, ch_flags);
> +}
> +
> +/* Helper function for VIDIOC_S_CTRL compatibility */
> +static int set_ctrl_lock(struct v4l2_fh *fh, struct v4l2_ctrl *ctrl, s32 *val)
> +{
> +	int ret = validate_new_int(ctrl, val);
> +
> +	if (!ret) {
> +		v4l2_ctrl_lock(ctrl);
> +		ret = set_ctrl(fh, ctrl, *val, 0);
> +		if (!ret)
> +			*val = ctrl->cur.val;
> +		v4l2_ctrl_unlock(ctrl);
> +	}
>  	return ret;
>  }
>  
> @@ -2313,7 +2340,7 @@ int v4l2_s_ctrl(struct v4l2_fh *fh, struct v4l2_ctrl_handler *hdl,
>  	if (ctrl->flags & V4L2_CTRL_FLAG_READ_ONLY)
>  		return -EACCES;
>  
> -	return set_ctrl(fh, ctrl, &control->value);
> +	return set_ctrl_lock(fh, ctrl, &control->value);
>  }
>  EXPORT_SYMBOL(v4l2_s_ctrl);
>  
> @@ -2327,10 +2354,44 @@ int v4l2_ctrl_s_ctrl(struct v4l2_ctrl *ctrl, s32 val)
>  {
>  	/* It's a driver bug if this happens. */
>  	WARN_ON(!type_is_int(ctrl));
> -	return set_ctrl(NULL, ctrl, &val);
> +	return set_ctrl_lock(NULL, ctrl, &val);
>  }
>  EXPORT_SYMBOL(v4l2_ctrl_s_ctrl);
>  
> +int v4l2_ctrl_update_range(struct v4l2_ctrl *ctrl,
> +			s32 min, s32 max, u32 step, s32 def)
> +{
> +	int ret = check_range(ctrl->type, min, max, step, def);
> +	s32 val;
> +
> +	switch (ctrl->type) {
> +	case V4L2_CTRL_TYPE_INTEGER:
> +	case V4L2_CTRL_TYPE_BOOLEAN:
> +	case V4L2_CTRL_TYPE_MENU:
> +	case V4L2_CTRL_TYPE_BITMASK:
> +		if (ret)
> +			return ret;
> +		break;
> +	default:
> +		return -EINVAL;
> +	}
> +	v4l2_ctrl_lock(ctrl);
> +	ctrl->minimum = min;
> +	ctrl->maximum = max;
> +	ctrl->step = step;
> +	ctrl->default_value = def;
> +	val = ctrl->cur.val;
> +	if (validate_new_int(ctrl, &val))
> +		val = def;
> +	if (val != ctrl->cur.val)
> +		ret = set_ctrl(NULL, ctrl, val, V4L2_EVENT_CTRL_CH_RANGE);
> +	else
> +		send_event(NULL, ctrl, V4L2_EVENT_CTRL_CH_RANGE);
> +	v4l2_ctrl_unlock(ctrl);
> +	return ret;
> +}
> +EXPORT_SYMBOL(v4l2_ctrl_update_range);
> +
>  void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
>  				struct v4l2_subscribed_event *sev)
>  {
> @@ -2339,7 +2400,7 @@ void v4l2_ctrl_add_event(struct v4l2_ctrl *ctrl,
>  	if (ctrl->type != V4L2_CTRL_TYPE_CTRL_CLASS &&
>  	    (sev->flags & V4L2_EVENT_SUB_FL_SEND_INITIAL)) {
>  		struct v4l2_event ev;
> -		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS;
> +		u32 changes = V4L2_EVENT_CTRL_CH_FLAGS | V4L2_EVENT_CTRL_CH_RANGE;
>  
>  		if (!(ctrl->flags & V4L2_CTRL_FLAG_WRITE_ONLY))
>  			changes |= V4L2_EVENT_CTRL_CH_VALUE;
> diff --git a/drivers/media/video/vivi.c b/drivers/media/video/vivi.c
> index 7d754fb..fd89106 100644
> --- a/drivers/media/video/vivi.c
> +++ b/drivers/media/video/vivi.c
> @@ -190,6 +190,7 @@ struct vivi_dev {
>  	unsigned 		   ms;
>  	unsigned long              jiffies;
>  	unsigned		   button_pressed;
> +	bool			   toggle_range;
>  
>  	int			   mv_count;	/* Controls bars movement */
>  
> @@ -545,6 +546,17 @@ static void vivi_thread_tick(struct vivi_dev *dev)
>  	vivi_fillbuff(dev, buf);
>  	dprintk(dev, 1, "filled buffer %p\n", buf);
>  
> +	if (dev->toggle_range) {
> +		static bool toggle;
> +
> +		dev->toggle_range = false;
> +		if (toggle)
> +			v4l2_ctrl_update_range(dev->contrast, 0, 255, 1, 16);
> +		else
> +			v4l2_ctrl_update_range(dev->contrast, 128, 255, 2, 150);
> +		toggle = !toggle;
> +	}
> +
>  	vb2_buffer_done(&buf->vb, VB2_BUF_STATE_DONE);
>  	dprintk(dev, 2, "[%p/%d] done\n", buf, buf->vb.v4l2_buf.index);
>  }
> @@ -1034,8 +1046,10 @@ static int vivi_s_ctrl(struct v4l2_ctrl *ctrl)
>  {
>  	struct vivi_dev *dev = container_of(ctrl->handler, struct vivi_dev, ctrl_handler);
>  
> -	if (ctrl == dev->button)
> +	if (ctrl == dev->button) {
>  		dev->button_pressed = 30;
> +		dev->toggle_range = true;
> +	}
>  	return 0;
>  }
>  
> diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
> index 4b752d5..22e632a 100644
> --- a/include/linux/videodev2.h
> +++ b/include/linux/videodev2.h
> @@ -2063,6 +2063,7 @@ struct v4l2_event_vsync {
>  /* Payload for V4L2_EVENT_CTRL */
>  #define V4L2_EVENT_CTRL_CH_VALUE		(1 << 0)
>  #define V4L2_EVENT_CTRL_CH_FLAGS		(1 << 1)
> +#define V4L2_EVENT_CTRL_CH_RANGE		(1 << 2)
>  
>  struct v4l2_event_ctrl {
>  	__u32 changes;
> diff --git a/include/media/v4l2-ctrls.h b/include/media/v4l2-ctrls.h
> index eeb3df6..69ea0d5 100644
> --- a/include/media/v4l2-ctrls.h
> +++ b/include/media/v4l2-ctrls.h
> @@ -445,6 +445,23 @@ void v4l2_ctrl_activate(struct v4l2_ctrl *ctrl, bool active);
>    */
>  void v4l2_ctrl_grab(struct v4l2_ctrl *ctrl, bool grabbed);
>  
> +/** v4l2_ctrl_update_range() - Update the range of a control.
> +  * @ctrl:	The control to update.
> +  * @min:	The control's minimum value.
> +  * @max:	The control's maximum value.
> +  * @step:	The control's step value
> +  * @def: 	The control's default value.
> +  *
> +  * Update the range of a control on the fly. This works for control types
> +  * INTEGER, BOOLEAN, MENU and BITMASK. For menu controls the @step value
> +  * is interpreted as a menu_skip_mask.
> +  *
> +  * An error is returned if one of the range arguments is invalid for this
> +  * control type.
> +  */
> +int v4l2_ctrl_update_range(struct v4l2_ctrl *ctrl,
> +			s32 min, s32 max, u32 step, s32 def);
> +
>  /** v4l2_ctrl_lock() - Helper function to lock the handler
>    * associated with the control.
>    * @ctrl:	The control to lock.
> 

Regards
-- 
Sylwester Nawrocki
Samsung Poland R&D Center

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> > On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> >> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> >>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> >>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> >>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> > Here is a patch that updates the range. It also sends a control event
> > telling any listener that the range has changed. Tested with vivi and a
> > modified v4l2-ctl.
> > 
> > The only thing missing is a DocBook entry for that new event flag and
> > perhaps some more documentation in places.
> > 
> > Let me know how this works for you, and if it is really needed, then I
> > can add it to the control framework.
> 
> Thanks for your work, it's very appreciated.
> 
> I've tested the patch with s5p-fimc and it works well. I just didn't check
> the event part yet.
> 
> I spoke to Kamil as in the past he considered the control range updating
> at the codec driver. But since separate controls are used for different
> encoding standards, this is not needed it any more.
> 
> Nevertheless I have at least two use cases, for the alpha control and
> for the image sensor driver. In case of the camera sensor, different device
> revisions may have different step and maximum value for some controls,
> depending on firmware.
> By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
> start-up procedure just to find out properties of some controls.

Wouldn't it be confusing for applications to start with a range and have it 
updated at runtime ?

> It would be nice to have this enhancement in mainline.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Tuesday, November 29, 2011 19:10:39 Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> > On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> > > On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> > >> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> > >>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> > >>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> > >>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> > > Here is a patch that updates the range. It also sends a control event
> > > telling any listener that the range has changed. Tested with vivi and a
> > > modified v4l2-ctl.
> > > 
> > > The only thing missing is a DocBook entry for that new event flag and
> > > perhaps some more documentation in places.
> > > 
> > > Let me know how this works for you, and if it is really needed, then I
> > > can add it to the control framework.
> > 
> > Thanks for your work, it's very appreciated.
> > 
> > I've tested the patch with s5p-fimc and it works well. I just didn't check
> > the event part yet.
> > 
> > I spoke to Kamil as in the past he considered the control range updating
> > at the codec driver. But since separate controls are used for different
> > encoding standards, this is not needed it any more.
> > 
> > Nevertheless I have at least two use cases, for the alpha control and
> > for the image sensor driver. In case of the camera sensor, different device
> > revisions may have different step and maximum value for some controls,
> > depending on firmware.
> > By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
> > start-up procedure just to find out properties of some controls.
> 
> Wouldn't it be confusing for applications to start with a range and have it 
> updated at runtime ?

Good question. It was a nice exercise creating the range_update() function and
it works well, but it this something we want to do?

If we do, then we should mark such controls with a flag (_VOLATILE_RANGE or
something like that) so apps know that the range isn't fixed.

I think that when it comes to apps writing or reading such a control directly
it isn't a problem. But for applications that automatically generate control
panels (xawtv et al) it is rather complex to support such things.

Regards,

	Hans

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Laurent Pinchart]

Hi Hans,

On Tuesday 29 November 2011 19:30:25 Hans Verkuil wrote:
> On Tuesday, November 29, 2011 19:10:39 Laurent Pinchart wrote:
> > On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> > > On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> > > > On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> > > >> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> > > >>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> > > >>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> > > >>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> > > > Here is a patch that updates the range. It also sends a control event
> > > > telling any listener that the range has changed. Tested with vivi and
> > > > a modified v4l2-ctl.
> > > > 
> > > > The only thing missing is a DocBook entry for that new event flag and
> > > > perhaps some more documentation in places.
> > > > 
> > > > Let me know how this works for you, and if it is really needed, then
> > > > I can add it to the control framework.
> > > 
> > > Thanks for your work, it's very appreciated.
> > > 
> > > I've tested the patch with s5p-fimc and it works well. I just didn't
> > > check the event part yet.
> > > 
> > > I spoke to Kamil as in the past he considered the control range
> > > updating at the codec driver. But since separate controls are used for
> > > different encoding standards, this is not needed it any more.
> > > 
> > > Nevertheless I have at least two use cases, for the alpha control and
> > > for the image sensor driver. In case of the camera sensor, different
> > > device revisions may have different step and maximum value for some
> > > controls, depending on firmware.
> > > By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
> > > start-up procedure just to find out properties of some controls.
> > 
> > Wouldn't it be confusing for applications to start with a range and have
> > it updated at runtime ?
> 
> Good question. It was a nice exercise creating the range_update() function
> and it works well, but it this something we want to do?

I think that being able to modify the range is a very useful functionality. 
It's just that in this case the sensor would start with a default range and 
switch to another based on the model. It would be better if we could start 
with the right range from the start.

> If we do, then we should mark such controls with a flag (_VOLATILE_RANGE or
> something like that) so apps know that the range isn't fixed.
> 
> I think that when it comes to apps writing or reading such a control
> directly it isn't a problem. But for applications that automatically
> generate control panels (xawtv et al) it is rather complex to support such
> things.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

Hi Laurent!

On 11/29/2011 07:10 PM, Laurent Pinchart wrote:
> Hi Sylwester,
> 
> On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
>> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
>>> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
>>>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
>>>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
>>>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
>>>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:

>>
>> Nevertheless I have at least two use cases, for the alpha control and
>> for the image sensor driver. In case of the camera sensor, different device
>> revisions may have different step and maximum value for some controls,
>> depending on firmware.
>> By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
>> start-up procedure just to find out properties of some controls.
> 
> Wouldn't it be confusing for applications to start with a range and have it 
> updated at runtime ?
> 

Indeed, changing a control range like this is not the brightest idea ever.
I would not consider doing something like this commonly. However if the
applications are aware that the control range may change at any time and
they handle the events, there shouldn't be a problem. Of course life for
applications is getting harder. The complexity for applications is increasing
maybe a bit too much at this point already...

I guess you would agree that it's best to power up the sensor when sub-device
node is opened and do all necessary setup before any subdev file operation
is commenced. For that I'm just looking forward for the common struct clk
to be merged and all platforms to be converted to it. So we can use
a struct clk object to enable sensor clock from subdev drivers level.


--

Regards,
Sylwester

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Laurent Pinchart]

Hi Sylwester,

On Tuesday 29 November 2011 20:39:39 Sylwester Nawrocki wrote:
> On 11/29/2011 07:10 PM, Laurent Pinchart wrote:
> > On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> >> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> >>> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> >>>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> >>>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> >>>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> >>>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> >> Nevertheless I have at least two use cases, for the alpha control and
> >> for the image sensor driver. In case of the camera sensor, different
> >> device revisions may have different step and maximum value for some
> >> controls, depending on firmware.
> >> By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
> >> start-up procedure just to find out properties of some controls.
> > 
> > Wouldn't it be confusing for applications to start with a range and have
> > it updated at runtime ?
> 
> Indeed, changing a control range like this is not the brightest idea ever.
> I would not consider doing something like this commonly. However if the
> applications are aware that the control range may change at any time and
> they handle the events, there shouldn't be a problem. Of course life for
> applications is getting harder. The complexity for applications is
> increasing maybe a bit too much at this point already...
> 
> I guess you would agree that it's best to power up the sensor when
> sub-device node is opened and do all necessary setup before any subdev
> file operation is commenced.

And applications won't be able to query the control range without opening the 
subdev anyway.

> For that I'm just looking forward for the common struct clk to be merged and
> all platforms to be converted to it. So we can use a struct clk object to
> enable sensor clock from subdev drivers level.

I think we're all looking for that :-)

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

On 11/29/2011 07:58 PM, Laurent Pinchart wrote:
> On Tuesday 29 November 2011 19:30:25 Hans Verkuil wrote:
>> On Tuesday, November 29, 2011 19:10:39 Laurent Pinchart wrote:
>>> On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
>>>> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
>>>>> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
>>>>>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
>>>>>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
>>>>>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
>>>>>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
>>>>> Here is a patch that updates the range. It also sends a control event
>>>>> telling any listener that the range has changed. Tested with vivi and
>>>>> a modified v4l2-ctl.
>>>>>
>>>>> The only thing missing is a DocBook entry for that new event flag and
>>>>> perhaps some more documentation in places.
>>>>>
>>>>> Let me know how this works for you, and if it is really needed, then
>>>>> I can add it to the control framework.
>>>>
>>>> Thanks for your work, it's very appreciated.
>>>>
>>>> I've tested the patch with s5p-fimc and it works well. I just didn't
>>>> check the event part yet.
>>>>
>>>> I spoke to Kamil as in the past he considered the control range
>>>> updating at the codec driver. But since separate controls are used for
>>>> different encoding standards, this is not needed it any more.
>>>>
>>>> Nevertheless I have at least two use cases, for the alpha control and
>>>> for the image sensor driver. In case of the camera sensor, different
>>>> device revisions may have different step and maximum value for some
>>>> controls, depending on firmware.
>>>> By using v4l2_ctrl_range_update() I don't need to invoke lengthy sensor
>>>> start-up procedure just to find out properties of some controls.
>>>
>>> Wouldn't it be confusing for applications to start with a range and have
>>> it updated at runtime ?
>>
>> Good question. It was a nice exercise creating the range_update() function
>> and it works well, but it this something we want to do?
> 
> I think that being able to modify the range is a very useful functionality. 
> It's just that in this case the sensor would start with a default range and 
> switch to another based on the model. It would be better if we could start 
> with the right range from the start.
> 
>> If we do, then we should mark such controls with a flag (_VOLATILE_RANGE or
>> something like that) so apps know that the range isn't fixed.
>>
>> I think that when it comes to apps writing or reading such a control
>> directly it isn't a problem. But for applications that automatically
>> generate control panels (xawtv et al) it is rather complex to support such
>> things.

Hans,

are you going to carry on with the control range update patches ?
I'd like to push the alpha colour control for v3.3 but it depends
on the controls framework updates now.


Another use case for control range update would be with an auto-exposure
metering spot location controls. An available range for x and y coordinates
would depend on selected pixel resolution. If we would have created two
controls for (x, y) their range would depend on pixel (width, height)
respectively. So when a new format is set such controls would need to get
their range updated.

-- 

Thanks,
Sylwester

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Laurent Pinchart]

Hi Sylwester,

On Thursday 08 December 2011 10:30:58 Sylwester Nawrocki wrote:
> On 11/29/2011 07:58 PM, Laurent Pinchart wrote:
> > On Tuesday 29 November 2011 19:30:25 Hans Verkuil wrote:
> >> On Tuesday, November 29, 2011 19:10:39 Laurent Pinchart wrote:
> >>> On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> >>>> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> >>>>> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> >>>>>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> >>>>>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> >>>>>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> >>>>>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> >>>>> Here is a patch that updates the range. It also sends a control event
> >>>>> telling any listener that the range has changed. Tested with vivi and
> >>>>> a modified v4l2-ctl.
> >>>>> 
> >>>>> The only thing missing is a DocBook entry for that new event flag and
> >>>>> perhaps some more documentation in places.
> >>>>> 
> >>>>> Let me know how this works for you, and if it is really needed, then
> >>>>> I can add it to the control framework.
> >>>> 
> >>>> Thanks for your work, it's very appreciated.
> >>>> 
> >>>> I've tested the patch with s5p-fimc and it works well. I just didn't
> >>>> check the event part yet.
> >>>> 
> >>>> I spoke to Kamil as in the past he considered the control range
> >>>> updating at the codec driver. But since separate controls are used for
> >>>> different encoding standards, this is not needed it any more.
> >>>> 
> >>>> Nevertheless I have at least two use cases, for the alpha control and
> >>>> for the image sensor driver. In case of the camera sensor, different
> >>>> device revisions may have different step and maximum value for some
> >>>> controls, depending on firmware.
> >>>> By using v4l2_ctrl_range_update() I don't need to invoke lengthy
> >>>> sensor start-up procedure just to find out properties of some
> >>>> controls.
> >>> 
> >>> Wouldn't it be confusing for applications to start with a range and
> >>> have it updated at runtime ?
> >> 
> >> Good question. It was a nice exercise creating the range_update()
> >> function and it works well, but it this something we want to do?
> > 
> > I think that being able to modify the range is a very useful
> > functionality. It's just that in this case the sensor would start with a
> > default range and switch to another based on the model. It would be
> > better if we could start with the right range from the start.
> > 
> >> If we do, then we should mark such controls with a flag (_VOLATILE_RANGE
> >> or something like that) so apps know that the range isn't fixed.
> >> 
> >> I think that when it comes to apps writing or reading such a control
> >> directly it isn't a problem. But for applications that automatically
> >> generate control panels (xawtv et al) it is rather complex to support
> >> such things.
> 
> Hans,
> 
> are you going to carry on with the control range update patches ?
> I'd like to push the alpha colour control for v3.3 but it depends
> on the controls framework updates now.
> 
> 
> Another use case for control range update would be with an auto-exposure
> metering spot location controls. An available range for x and y coordinates
> would depend on selected pixel resolution. If we would have created two
> controls for (x, y) their range would depend on pixel (width, height)
> respectively. So when a new format is set such controls would need to get
> their range updated.

To be honest I'm not sure whether points, and especially rectangles, should be 
handled as controls. We have no structure-like control type at the moment, 
adding points might be possible, but rectangles would require either 2 point-
liek controls or 4 controls (left, top, width, height). I don't really like 
that. A new API (possibly based on the selection API ?) might be better.

-- 
Regards,

Laurent Pinchart

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

Hi Laurent,

On 12/08/2011 11:30 AM, Laurent Pinchart wrote:
>> Another use case for control range update would be with an auto-exposure
>> metering spot location controls. An available range for x and y coordinates
>> would depend on selected pixel resolution. If we would have created two
>> controls for (x, y) their range would depend on pixel (width, height)
>> respectively. So when a new format is set such controls would need to get
>> their range updated.
> 
> To be honest I'm not sure whether points, and especially rectangles, should be 
> handled as controls. We have no structure-like control type at the moment, 
> adding points might be possible, but rectangles would require either 2 point-
> liek controls or 4 controls (left, top, width, height). I don't really like 
> that. A new API (possibly based on the selection API ?) might be better.

Indeed, I don't like having 4 controls for specifying a rectangle as well, it
doesn't just sound right. I was concerned about specifying a point using
the selection API. But it could be just (left=x, top=y, width=1, height=1).


--
Regards,
Sylwester

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Thursday 08 December 2011 10:30:58 Sylwester Nawrocki wrote:
> On 11/29/2011 07:58 PM, Laurent Pinchart wrote:
> > On Tuesday 29 November 2011 19:30:25 Hans Verkuil wrote:
> >> On Tuesday, November 29, 2011 19:10:39 Laurent Pinchart wrote:
> >>> On Tuesday 29 November 2011 17:40:10 Sylwester Nawrocki wrote:
> >>>> On 11/29/2011 12:08 PM, Hans Verkuil wrote:
> >>>>> On Monday 28 November 2011 14:02:49 Sylwester Nawrocki wrote:
> >>>>>> On 11/28/2011 01:39 PM, Hans Verkuil wrote:
> >>>>>>> On Monday 28 November 2011 13:13:32 Sylwester Nawrocki wrote:
> >>>>>>>> On 11/28/2011 12:38 PM, Hans Verkuil wrote:
> >>>>>>>>> On Friday 25 November 2011 16:39:31 Sylwester Nawrocki wrote:
> >>>>> Here is a patch that updates the range. It also sends a control event
> >>>>> telling any listener that the range has changed. Tested with vivi and
> >>>>> a modified v4l2-ctl.
> >>>>> 
> >>>>> The only thing missing is a DocBook entry for that new event flag and
> >>>>> perhaps some more documentation in places.
> >>>>> 
> >>>>> Let me know how this works for you, and if it is really needed, then
> >>>>> I can add it to the control framework.
> >>>> 
> >>>> Thanks for your work, it's very appreciated.
> >>>> 
> >>>> I've tested the patch with s5p-fimc and it works well. I just didn't
> >>>> check the event part yet.
> >>>> 
> >>>> I spoke to Kamil as in the past he considered the control range
> >>>> updating at the codec driver. But since separate controls are used for
> >>>> different encoding standards, this is not needed it any more.
> >>>> 
> >>>> Nevertheless I have at least two use cases, for the alpha control and
> >>>> for the image sensor driver. In case of the camera sensor, different
> >>>> device revisions may have different step and maximum value for some
> >>>> controls, depending on firmware.
> >>>> By using v4l2_ctrl_range_update() I don't need to invoke lengthy
> >>>> sensor start-up procedure just to find out properties of some
> >>>> controls.
> >>> 
> >>> Wouldn't it be confusing for applications to start with a range and
> >>> have it updated at runtime ?
> >> 
> >> Good question. It was a nice exercise creating the range_update()
> >> function and it works well, but it this something we want to do?
> > 
> > I think that being able to modify the range is a very useful
> > functionality. It's just that in this case the sensor would start with a
> > default range and switch to another based on the model. It would be
> > better if we could start with the right range from the start.
> > 
> >> If we do, then we should mark such controls with a flag (_VOLATILE_RANGE
> >> or something like that) so apps know that the range isn't fixed.
> >> 
> >> I think that when it comes to apps writing or reading such a control
> >> directly it isn't a problem. But for applications that automatically
> >> generate control panels (xawtv et al) it is rather complex to support
> >> such things.
> 
> Hans,
> 
> are you going to carry on with the control range update patches ?
> I'd like to push the alpha colour control for v3.3 but it depends
> on the controls framework updates now.

Good question. I am not sure whether this is something we actually want. It 
would make applications much harder to write if the range of a control can 
suddenly change.

On the other hand, it might be a good solution for a harder problem which is 
as yet unsolved: if you have multiple inputs, and each input has a different 
set of controls (e.g. one input is a SDTV receiver, the other is a HDTV 
receiver), then you can have the situation where e.g. the contrast control is 
present for both inputs, but with a different range. Switching inputs would 
then generate a control event telling the app that the range changed.

But this may still be overkill...

In other words, I don't know. Not helpful, I agree.

Regards,

	Hans

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

Hi Hans,

On 12/13/2011 01:18 PM, Hans Verkuil wrote:
>> are you going to carry on with the control range update patches ?
>> I'd like to push the alpha colour control for v3.3 but it depends
>> on the controls framework updates now.
> 
> Good question. I am not sure whether this is something we actually want. It 
> would make applications much harder to write if the range of a control can 
> suddenly change.
> 
> On the other hand, it might be a good solution for a harder problem which is 
> as yet unsolved: if you have multiple inputs, and each input has a different 
> set of controls (e.g. one input is a SDTV receiver, the other is a HDTV 
> receiver), then you can have the situation where e.g. the contrast control is 
> present for both inputs, but with a different range. Switching inputs would 
> then generate a control event telling the app that the range changed.
> 
> But this may still be overkill...

Hmm, it doesn't look like an overkill to me. I'm certain there will be use
cases where control range update is needed. Maybe we could specify in
the API in what circumstances the control range update is allowed for drivers.
So not all applications need to handle the related events.

Nevertheless I won't be pushing on this, not to mess around in the whole
API because of some embedded systems requirements.
So I'm going to update the range for alpha control manually in the driver
for the time being.

> 
> In other words, I don't know. Not helpful, I agree.

That was helpful anyway :-) Thanks.

-- 

Regards,
Sylwester

[PATCH/RFC v4 0/2] Add new V4L2_CID_ALPHA_COMPONENT control

[Sylwester Nawrocki]

Hello,

This changeset adds new V4L2_CID_ALPHA_COMPONENT control allowing to configure 
an alpha component of all pixels on the video capture device or on capture queue 
of a mem-to-mem device. This is meant for devices that allow to set a per-pixel
alpha at the pipeline output to a desired value and where the input alpha component 
doesn't influence the output alpha value.

The second patch adds the control to s5p-fimc video capture and mem-to-mem driver.

This changset also does a minor cleanup at the user controls DocBook chapter.

Changes since v3:
  - update the alpha control maximum value manually in the driver rather than
    adding support for this in v4l core

Changes since v2:
  - removed limitation of maximum value for the V4L2_CID_ALPHA_COMPONENT control 
    to 0xff in v4l core
  - the driver now uses function v4l2_ctrl_update_range() for the control range 
    update according to selected colour format

Changes since v1:
 - rename V4L2_CID_COLOR_ALPHA to V4L2_CID_ALPHA_COMPONENT,
 - the documentation improvements.


Sylwester Nawrocki (2):
  v4l: Add new alpha component control
  s5p-fimc: Add support for alpha component configuration

 Documentation/DocBook/media/v4l/compat.xml         |   11 ++
 Documentation/DocBook/media/v4l/controls.xml       |   25 +++-
 .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |    7 +-
 drivers/media/video/s5p-fimc/fimc-capture.c        |   11 ++
 drivers/media/video/s5p-fimc/fimc-core.c           |  128 ++++++++++++++++----
 drivers/media/video/s5p-fimc/fimc-core.h           |   30 ++++-
 drivers/media/video/s5p-fimc/fimc-reg.c            |   53 ++++++--
 drivers/media/video/s5p-fimc/regs-fimc.h           |    5 +
 drivers/media/video/v4l2-ctrls.c                   |    1 +
 include/linux/videodev2.h                          |    6 +-
 10 files changed, 224 insertions(+), 53 deletions(-)

-- 
1.7.8

--
Regards,
Sylwester

[PATCH v4 1/2] v4l: Add new alpha component control

[Sylwester Nawrocki]

The V4L2_CID_ALPHA_COMPONENT control is intended for the video capture
or memory-to-memory devices that are capable of setting up the per-pixel
alpha component to some arbitrary value. It allows to set the alpha
component for all pixels to an arbitrary value.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
Acked-by: Laurent Pinchart <laurent.pinchart@ideasonboard.com>
---
There is no changes in this patch since v3.
---
 Documentation/DocBook/media/v4l/compat.xml         |   11 ++++++++
 Documentation/DocBook/media/v4l/controls.xml       |   25 +++++++++++++++----
 .../DocBook/media/v4l/pixfmt-packed-rgb.xml        |    7 ++++-
 drivers/media/video/v4l2-ctrls.c                   |    1 +
 include/linux/videodev2.h                          |    6 ++--
 5 files changed, 39 insertions(+), 11 deletions(-)

diff --git a/Documentation/DocBook/media/v4l/compat.xml b/Documentation/DocBook/media/v4l/compat.xml
index 8b44a43..12ba262 100644
--- a/Documentation/DocBook/media/v4l/compat.xml
+++ b/Documentation/DocBook/media/v4l/compat.xml
@@ -2379,6 +2379,17 @@ that used it. It was originally scheduled for removal in 2.6.35.
       </orderedlist>
     </section>
 
+    <section>
+      <title>V4L2 in Linux 3.3</title>
+      <orderedlist>
+        <listitem>
+	  <para>Added <constant>V4L2_CID_ALPHA_COMPONENT</constant> control
+	    to the <link linkend="control">User controls class</link>.
+	  </para>
+        </listitem>
+      </orderedlist>
+    </section>
+
     <section id="other">
       <title>Relation of V4L2 to other Linux multimedia APIs</title>
 
diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 9e72f07..60387d4 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -324,12 +324,6 @@ minimum value disables backlight compensation.</entry>
 		(usually a microscope).</entry>
 	  </row>
 	  <row>
-	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
-	    <entry></entry>
-	    <entry>End of the predefined control IDs (currently
-<constant>V4L2_CID_ILLUMINATORS_2</constant> + 1).</entry>
-	  </row>
-	  <row>
 	    <entry><constant>V4L2_CID_MIN_BUFFERS_FOR_CAPTURE</constant></entry>
 	    <entry>integer</entry>
 	    <entry>This is a read-only control that can be read by the application
@@ -345,6 +339,25 @@ and used as a hint to determine the number of OUTPUT buffers to pass to REQBUFS.
 The value is the minimum number of OUTPUT buffers that is necessary for hardware
 to work.</entry>
 	  </row>
+	  <row id="v4l2-alpha-component">
+	    <entry><constant>V4L2_CID_ALPHA_COMPONENT</constant></entry>
+	    <entry>integer</entry>
+	    <entry> Sets the alpha color component on the capture device or on
+	    the capture buffer queue of a mem-to-mem device. When a mem-to-mem
+	    device produces frame format that includes an alpha component
+	    (e.g. <link linkend="rgb-formats">packed RGB image formats</link>)
+	    and the alpha value is not defined by the mem-to-mem input data
+	    this control lets you select the alpha component value of all
+	    pixels. It is applicable to any pixel format that contains an alpha
+	    component.
+	    </entry>
+	  </row>
+	  <row>
+	    <entry><constant>V4L2_CID_LASTP1</constant></entry>
+	    <entry></entry>
+	    <entry>End of the predefined control IDs (currently
+	      <constant>V4L2_CID_ALPHA_COMPONENT</constant> + 1).</entry>
+	  </row>
 	  <row>
 	    <entry><constant>V4L2_CID_PRIVATE_BASE</constant></entry>
 	    <entry></entry>
diff --git a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
index ba56536..166c8d6 100644
--- a/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
+++ b/Documentation/DocBook/media/v4l/pixfmt-packed-rgb.xml
@@ -428,8 +428,11 @@ colorspace <constant>V4L2_COLORSPACE_SRGB</constant>.</para>
     <para>Bit 7 is the most significant bit. The value of a = alpha
 bits is undefined when reading from the driver, ignored when writing
 to the driver, except when alpha blending has been negotiated for a
-<link linkend="overlay">Video Overlay</link> or <link
-linkend="osd">Video Output Overlay</link>.</para>
+<link linkend="overlay">Video Overlay</link> or <link linkend="osd">
+Video Output Overlay</link> or when alpha component has been configured
+for a <link linkend="capture">Video Capture</link> by means of <link
+linkend="v4l2-alpha-component"> <constant>V4L2_CID_ALPHA_COMPONENT
+</constant> </link> control.</para>
 
     <example>
       <title><constant>V4L2_PIX_FMT_BGR24</constant> 4 &times; 4 pixel
diff --git a/drivers/media/video/v4l2-ctrls.c b/drivers/media/video/v4l2-ctrls.c
index 0f415da..3926615 100644
--- a/drivers/media/video/v4l2-ctrls.c
+++ b/drivers/media/video/v4l2-ctrls.c
@@ -467,6 +467,7 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_ILLUMINATORS_2:		return "Illuminator 2";
 	case V4L2_CID_MIN_BUFFERS_FOR_CAPTURE:	return "Minimum Number of Capture Buffers";
 	case V4L2_CID_MIN_BUFFERS_FOR_OUTPUT:	return "Minimum Number of Output Buffers";
+	case V4L2_CID_ALPHA_COMPONENT:		return "Alpha Component";
 
 	/* MPEG controls */
 	/* Keep the order of the 'case's the same as in videodev2.h! */
diff --git a/include/linux/videodev2.h b/include/linux/videodev2.h
index 4b752d5..fdda200 100644
--- a/include/linux/videodev2.h
+++ b/include/linux/videodev2.h
@@ -1204,10 +1204,10 @@ enum v4l2_colorfx {
 #define V4L2_CID_MIN_BUFFERS_FOR_CAPTURE	(V4L2_CID_BASE+39)
 #define V4L2_CID_MIN_BUFFERS_FOR_OUTPUT		(V4L2_CID_BASE+40)
 
-/* last CID + 1 */
-#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+41)
+#define V4L2_CID_ALPHA_COMPONENT		(V4L2_CID_BASE+41)
 
-/* Minimum number of buffer neede by the device */
+/* last CID + 1 */
+#define V4L2_CID_LASTP1                         (V4L2_CID_BASE+42)
 
 /*  MPEG-class control IDs defined by V4L2 */
 #define V4L2_CID_MPEG_BASE 			(V4L2_CTRL_CLASS_MPEG | 0x900)
-- 
1.7.8

[PATCH v4 2/2] s5p-fimc: Add support for alpha component configuration

[Sylwester Nawrocki]

On Exynos SoCs the FIMC IP allows to configure globally the alpha
component of all pixels for V4L2_PIX_FMT_RGB32, V4L2_PIX_FMT_RGB555
and V4L2_PIX_FMT_RGB444 image formats. This patch adds a v4l2 control
in order to let the applications control the alpha component value.

The alpha value range depends on pixel format, for RGB32 it's 0..255 
(8-bits), for RGB555 - 0..1 (1-bit) and for RGB444 - 0..15 (4-bits). 
The v4l2 control range is always 0..255 and the alpha component data 
width is determined by currently set format on the 
V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE buffer queue. The applications need 
to match the alpha channel value range and the pixel format since the 
driver will clamp the alpha component. Depending on fourcc valid alpha 
bits are:

 - V4L2_PIX_FMT_RGB555  [0]
 - V4L2_PIX_FMT_RGB444  [3:0]
 - V4L2_PIX_FMT_RGB32   [7:0]

When switching to a pixel format with smaller alpha component width
the currently set alpha value will be clamped to maximum value valid
for current format. When switching to a format with wider alpha the
alpha value remains unchanged.

The variant description data structure is extended with a new entry
so an additional control is created only where really supported by
the hardware.

V4L2_PIX_FMT_RGB555 and V4L2_PIX_FMT_RGB444 formats are only valid
for V4L2_BUF_TYPE_VIDEO_CAPTURE buffer queue.

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
Signed-off-by: Kyungmin Park <kyungmin.park@samsung.com>
---
 drivers/media/video/s5p-fimc/fimc-capture.c |   11 +++
 drivers/media/video/s5p-fimc/fimc-core.c    |  128 +++++++++++++++++++++------
 drivers/media/video/s5p-fimc/fimc-core.h    |   30 ++++++-
 drivers/media/video/s5p-fimc/fimc-reg.c     |   53 +++++++++---
 drivers/media/video/s5p-fimc/regs-fimc.h    |    5 +
 5 files changed, 185 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c b/drivers/media/video/s5p-fimc/fimc-capture.c
index c8d91b0..644cf04 100644
--- a/drivers/media/video/s5p-fimc/fimc-capture.c
+++ b/drivers/media/video/s5p-fimc/fimc-capture.c
@@ -63,6 +63,8 @@ static int fimc_init_capture(struct fimc_dev *fimc)
 		fimc_hw_set_effect(ctx, false);
 		fimc_hw_set_output_path(ctx);
 		fimc_hw_set_out_dma(ctx);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
 		clear_bit(ST_CAPT_APPLY_CFG, &fimc->state);
 	}
 	spin_unlock_irqrestore(&fimc->slock, flags);
@@ -151,6 +153,8 @@ int fimc_capture_config_update(struct fimc_ctx *ctx)
 		fimc_prepare_dma_offset(ctx, &ctx->d_frame);
 		fimc_hw_set_out_dma(ctx);
 		set_bit(ST_CAPT_APPLY_CFG, &fimc->state);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
 	}
 	spin_unlock(&ctx->slock);
 	return ret;
@@ -809,6 +813,10 @@ static int fimc_capture_set_format(struct fimc_dev *fimc, struct v4l2_format *f)
 					  FIMC_SD_PAD_SOURCE);
 	if (!ff->fmt)
 		return -EINVAL;
+
+	/* Update RGB Alpha control state and value range */
+	fimc_alpha_ctrl_update(ctx);
+
 	/* Try to match format at the host and the sensor */
 	if (!fimc->vid_cap.user_subdev_api) {
 		mf->code   = ff->fmt->mbus_code;
@@ -1232,6 +1240,9 @@ static int fimc_subdev_set_fmt(struct v4l2_subdev *sd,
 		*mf = fmt->format;
 		return 0;
 	}
+	/* Update RGB Alpha control state and value range */
+	fimc_alpha_ctrl_update(ctx);
+
 	fimc_capture_mark_jpeg_xfer(ctx, fimc_fmt_is_jpeg(ffmt->color));
 
 	ff = fmt->pad == FIMC_SD_PAD_SINK ?
diff --git a/drivers/media/video/s5p-fimc/fimc-core.c b/drivers/media/video/s5p-fimc/fimc-core.c
index 19ca6db..9411786 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.c
+++ b/drivers/media/video/s5p-fimc/fimc-core.c
@@ -52,13 +52,29 @@ static struct fimc_fmt fimc_formats[] = {
 		.colplanes	= 1,
 		.flags		= FMT_FLAGS_M2M,
 	}, {
-		.name		= "XRGB-8-8-8-8, 32 bpp",
+		.name		= "ARGB8888, 32 bpp",
 		.fourcc		= V4L2_PIX_FMT_RGB32,
 		.depth		= { 32 },
 		.color		= S5P_FIMC_RGB888,
 		.memplanes	= 1,
 		.colplanes	= 1,
-		.flags		= FMT_FLAGS_M2M,
+		.flags		= FMT_FLAGS_M2M | FMT_HAS_ALPHA,
+	}, {
+		.name		= "ARGB1555",
+		.fourcc		= V4L2_PIX_FMT_RGB555,
+		.depth		= { 16 },
+		.color		= S5P_FIMC_RGB555,
+		.memplanes	= 1,
+		.colplanes	= 1,
+		.flags		= FMT_FLAGS_M2M_OUT | FMT_HAS_ALPHA,
+	}, {
+		.name		= "ARGB4444",
+		.fourcc		= V4L2_PIX_FMT_RGB444,
+		.depth		= { 16 },
+		.color		= S5P_FIMC_RGB444,
+		.memplanes	= 1,
+		.colplanes	= 1,
+		.flags		= FMT_FLAGS_M2M_OUT | FMT_HAS_ALPHA,
 	}, {
 		.name		= "YUV 4:2:2 packed, YCbYCr",
 		.fourcc		= V4L2_PIX_FMT_YUYV,
@@ -171,6 +187,14 @@ static struct fimc_fmt fimc_formats[] = {
 	},
 };
 
+static unsigned int get_m2m_fmt_flags(unsigned int stream_type)
+{
+	if (stream_type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE)
+		return FMT_FLAGS_M2M_IN;
+	else
+		return FMT_FLAGS_M2M_OUT;
+}
+
 int fimc_check_scaler_ratio(struct fimc_ctx *ctx, int sw, int sh,
 			    int dw, int dh, int rotation)
 {
@@ -652,8 +676,11 @@ static void fimc_dma_run(void *priv)
 	if (ctx->state & (FIMC_DST_ADDR | FIMC_PARAMS))
 		fimc_hw_set_output_addr(fimc, &ctx->d_frame.paddr, -1);
 
-	if (ctx->state & FIMC_PARAMS)
+	if (ctx->state & FIMC_PARAMS) {
 		fimc_hw_set_out_dma(ctx);
+		if (fimc->variant->has_alpha)
+			fimc_hw_set_rgb_alpha(ctx);
+	}
 
 	fimc_activate_capture(ctx);
 
@@ -750,12 +777,11 @@ static struct vb2_ops fimc_qops = {
 #define ctrl_to_ctx(__ctrl) \
 	container_of((__ctrl)->handler, struct fimc_ctx, ctrl_handler)
 
-static int fimc_s_ctrl(struct v4l2_ctrl *ctrl)
+static int __fimc_s_ctrl(struct fimc_ctx *ctx, struct v4l2_ctrl *ctrl)
 {
-	struct fimc_ctx *ctx = ctrl_to_ctx(ctrl);
 	struct fimc_dev *fimc = ctx->fimc_dev;
 	struct samsung_fimc_variant *variant = fimc->variant;
-	unsigned long flags;
+	unsigned int flags = FIMC_DST_FMT | FIMC_SRC_FMT;
 	int ret = 0;
 
 	if (ctrl->flags & V4L2_CTRL_FLAG_INACTIVE)
@@ -763,52 +789,63 @@ static int fimc_s_ctrl(struct v4l2_ctrl *ctrl)
 
 	switch (ctrl->id) {
 	case V4L2_CID_HFLIP:
-		spin_lock_irqsave(&ctx->slock, flags);
 		ctx->hflip = ctrl->val;
 		break;
 
 	case V4L2_CID_VFLIP:
-		spin_lock_irqsave(&ctx->slock, flags);
 		ctx->vflip = ctrl->val;
 		break;
 
 	case V4L2_CID_ROTATE:
 		if (fimc_capture_pending(fimc) ||
-		    fimc_ctx_state_is_set(FIMC_DST_FMT | FIMC_SRC_FMT, ctx)) {
+		    (ctx->state & flags) == flags) {
 			ret = fimc_check_scaler_ratio(ctx, ctx->s_frame.width,
 					ctx->s_frame.height, ctx->d_frame.width,
 					ctx->d_frame.height, ctrl->val);
-		}
-		if (ret) {
-			v4l2_err(fimc->m2m.vfd, "Out of scaler range\n");
-			return -EINVAL;
+			if (ret)
+				return -EINVAL;
 		}
 		if ((ctrl->val == 90 || ctrl->val == 270) &&
 		    !variant->has_out_rot)
 			return -EINVAL;
-		spin_lock_irqsave(&ctx->slock, flags);
+
 		ctx->rotation = ctrl->val;
 		break;
 
-	default:
-		v4l2_err(fimc->v4l2_dev, "Invalid control: 0x%X\n", ctrl->id);
-		return -EINVAL;
+	case V4L2_CID_ALPHA_COMPONENT:
+		ctx->d_frame.alpha = ctrl->val;
+		break;
 	}
 	ctx->state |= FIMC_PARAMS;
 	set_bit(ST_CAPT_APPLY_CFG, &fimc->state);
-	spin_unlock_irqrestore(&ctx->slock, flags);
 	return 0;
 }
 
+static int fimc_s_ctrl(struct v4l2_ctrl *ctrl)
+{
+	struct fimc_ctx *ctx = ctrl_to_ctx(ctrl);
+	unsigned long flags;
+	int ret;
+
+	spin_lock_irqsave(&ctx->slock, flags);
+	ret = __fimc_s_ctrl(ctx, ctrl);
+	spin_unlock_irqrestore(&ctx->slock, flags);
+
+	return ret;
+}
+
 static const struct v4l2_ctrl_ops fimc_ctrl_ops = {
 	.s_ctrl = fimc_s_ctrl,
 };
 
 int fimc_ctrls_create(struct fimc_ctx *ctx)
 {
+	struct samsung_fimc_variant *variant = ctx->fimc_dev->variant;
+	unsigned int max_alpha = fimc_get_alpha_mask(ctx->d_frame.fmt);
+
 	if (ctx->ctrls_rdy)
 		return 0;
-	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 3);
+	v4l2_ctrl_handler_init(&ctx->ctrl_handler, 4);
 
 	ctx->ctrl_rotate = v4l2_ctrl_new_std(&ctx->ctrl_handler, &fimc_ctrl_ops,
 				     V4L2_CID_HFLIP, 0, 1, 1, 0);
@@ -816,6 +853,13 @@ int fimc_ctrls_create(struct fimc_ctx *ctx)
 				    V4L2_CID_VFLIP, 0, 1, 1, 0);
 	ctx->ctrl_vflip = v4l2_ctrl_new_std(&ctx->ctrl_handler, &fimc_ctrl_ops,
 				    V4L2_CID_ROTATE, 0, 270, 90, 0);
+	if (variant->has_alpha)
+		ctx->ctrl_alpha = v4l2_ctrl_new_std(&ctx->ctrl_handler,
+				    &fimc_ctrl_ops, V4L2_CID_ALPHA_COMPONENT,
+				    0, max_alpha, 1, 0);
+	else
+		ctx->ctrl_alpha = NULL;
+
 	ctx->ctrls_rdy = ctx->ctrl_handler.error == 0;
 
 	return ctx->ctrl_handler.error;
@@ -831,6 +875,8 @@ void fimc_ctrls_delete(struct fimc_ctx *ctx)
 
 void fimc_ctrls_activate(struct fimc_ctx *ctx, bool active)
 {
+	unsigned int has_alpha = ctx->d_frame.fmt->flags & FMT_HAS_ALPHA;
+
 	if (!ctx->ctrls_rdy)
 		return;
 
@@ -838,6 +884,8 @@ void fimc_ctrls_activate(struct fimc_ctx *ctx, bool active)
 	v4l2_ctrl_activate(ctx->ctrl_rotate, active);
 	v4l2_ctrl_activate(ctx->ctrl_hflip, active);
 	v4l2_ctrl_activate(ctx->ctrl_vflip, active);
+	if (ctx->ctrl_alpha)
+		v4l2_ctrl_activate(ctx->ctrl_alpha, active && has_alpha);
 
 	if (active) {
 		ctx->rotation = ctx->ctrl_rotate->val;
@@ -851,6 +899,24 @@ void fimc_ctrls_activate(struct fimc_ctx *ctx, bool active)
 	mutex_unlock(&ctx->ctrl_handler.lock);
 }
 
+/* Update maximum value of the alpha color control */
+void fimc_alpha_ctrl_update(struct fimc_ctx *ctx)
+{
+	struct fimc_dev *fimc = ctx->fimc_dev;
+	struct v4l2_ctrl *ctrl = ctx->ctrl_alpha;
+
+	if (ctrl == NULL || !fimc->variant->has_alpha)
+		return;
+
+	v4l2_ctrl_lock(ctrl);
+	ctrl->maximum = fimc_get_alpha_mask(ctx->d_frame.fmt);
+
+	if (ctrl->cur.val > ctrl->maximum)
+		ctrl->cur.val = ctrl->maximum;
+
+	v4l2_ctrl_unlock(ctrl);
+}
+
 /*
  * V4L2 ioctl handlers
  */
@@ -874,7 +940,8 @@ static int fimc_m2m_enum_fmt_mplane(struct file *file, void *priv,
 {
 	struct fimc_fmt *fmt;
 
-	fmt = fimc_find_format(NULL, NULL, FMT_FLAGS_M2M, f->index);
+	fmt = fimc_find_format(NULL, NULL, get_m2m_fmt_flags(f->type),
+			       f->index);
 	if (!fmt)
 		return -EINVAL;
 
@@ -938,6 +1005,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 	pix->colorspace	= V4L2_COLORSPACE_JPEG;
 	pix->field = V4L2_FIELD_NONE;
 	pix->num_planes = fmt->memplanes;
+	pix->pixelformat = fmt->fourcc;
 	pix->height = height;
 	pix->width = width;
 
@@ -1017,7 +1085,8 @@ static int fimc_try_fmt_mplane(struct fimc_ctx *ctx, struct v4l2_format *f)
 
 	dbg("w: %d, h: %d", pix->width, pix->height);
 
-	fmt = fimc_find_format(&pix->pixelformat, NULL, FMT_FLAGS_M2M, 0);
+	fmt = fimc_find_format(&pix->pixelformat, NULL,
+			       get_m2m_fmt_flags(f->type), 0);
 	if (WARN(fmt == NULL, "Pixel format lookup failed"))
 		return -EINVAL;
 
@@ -1088,10 +1157,13 @@ static int fimc_m2m_s_fmt_mplane(struct file *file, void *fh,
 
 	pix = &f->fmt.pix_mp;
 	frame->fmt = fimc_find_format(&pix->pixelformat, NULL,
-				      FMT_FLAGS_M2M, 0);
+				      get_m2m_fmt_flags(f->type), 0);
 	if (!frame->fmt)
 		return -EINVAL;
 
+	/* Update RGB Alpha control state and value range */
+	fimc_alpha_ctrl_update(ctx);
+
 	for (i = 0; i < frame->fmt->colplanes; i++) {
 		frame->payload[i] =
 			(pix->width * pix->height * frame->fmt->depth[i]) / 8;
@@ -1375,6 +1447,12 @@ static int fimc_m2m_open(struct file *file)
 	if (!ctx)
 		return -ENOMEM;
 	v4l2_fh_init(&ctx->fh, fimc->m2m.vfd);
+	ctx->fimc_dev = fimc;
+
+	/* Default color format */
+	ctx->s_frame.fmt = &fimc_formats[0];
+	ctx->d_frame.fmt = &fimc_formats[0];
+
 	ret = fimc_ctrls_create(ctx);
 	if (ret)
 		goto error_fh;
@@ -1384,10 +1462,6 @@ static int fimc_m2m_open(struct file *file)
 	file->private_data = &ctx->fh;
 	v4l2_fh_add(&ctx->fh);
 
-	ctx->fimc_dev = fimc;
-	/* Default color format */
-	ctx->s_frame.fmt = &fimc_formats[0];
-	ctx->d_frame.fmt = &fimc_formats[0];
 	/* Setup the device context for memory-to-memory mode */
 	ctx->state = FIMC_CTX_M2M;
 	ctx->flags = 0;
@@ -1895,6 +1969,7 @@ static struct samsung_fimc_variant fimc0_variant_exynos4 = {
 	.has_cam_if	 = 1,
 	.has_cistatus2	 = 1,
 	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
 	.min_inp_pixsize = 16,
 	.min_out_pixsize = 16,
 	.hor_offs_align	 = 2,
@@ -1907,6 +1982,7 @@ static struct samsung_fimc_variant fimc3_variant_exynos4 = {
 	.has_cam_if	 = 1,
 	.has_cistatus2	 = 1,
 	.has_mainscaler_ext = 1,
+	.has_alpha	 = 1,
 	.min_inp_pixsize = 16,
 	.min_out_pixsize = 16,
 	.hor_offs_align	 = 2,
diff --git a/drivers/media/video/s5p-fimc/fimc-core.h b/drivers/media/video/s5p-fimc/fimc-core.h
index a6936da..df7527d 100644
--- a/drivers/media/video/s5p-fimc/fimc-core.h
+++ b/drivers/media/video/s5p-fimc/fimc-core.h
@@ -85,7 +85,9 @@ enum fimc_datapath {
 };
 
 enum fimc_color_fmt {
-	S5P_FIMC_RGB565 = 0x10,
+	S5P_FIMC_RGB444 = 0x10,
+	S5P_FIMC_RGB555,
+	S5P_FIMC_RGB565,
 	S5P_FIMC_RGB666,
 	S5P_FIMC_RGB888,
 	S5P_FIMC_RGB30_LOCAL,
@@ -160,8 +162,11 @@ struct fimc_fmt {
 	u16	colplanes;
 	u8	depth[VIDEO_MAX_PLANES];
 	u16	flags;
-#define FMT_FLAGS_CAM	(1 << 0)
-#define FMT_FLAGS_M2M	(1 << 1)
+#define FMT_FLAGS_CAM		(1 << 0)
+#define FMT_FLAGS_M2M_IN	(1 << 1)
+#define FMT_FLAGS_M2M_OUT	(1 << 2)
+#define FMT_FLAGS_M2M		(1 << 1 | 1 << 2)
+#define FMT_HAS_ALPHA		(1 << 3)
 };
 
 /**
@@ -283,6 +288,7 @@ struct fimc_frame {
 	struct fimc_addr	paddr;
 	struct fimc_dma_offset	dma_offset;
 	struct fimc_fmt		*fmt;
+	u8			alpha;
 };
 
 /**
@@ -386,6 +392,7 @@ struct samsung_fimc_variant {
 	unsigned int	has_cistatus2:1;
 	unsigned int	has_mainscaler_ext:1;
 	unsigned int	has_cam_if:1;
+	unsigned int	has_alpha:1;
 	struct fimc_pix_limit *pix_limit;
 	u16		min_inp_pixsize;
 	u16		min_out_pixsize;
@@ -480,7 +487,8 @@ struct fimc_dev {
  * @ctrl_handler:	v4l2 controls handler
  * @ctrl_rotate		image rotation control
  * @ctrl_hflip		horizontal flip control
- * @ctrl_vflip		vartical flip control
+ * @ctrl_vflip		vertical flip control
+ * @ctrl_alpha		RGB alpha control
  * @ctrls_rdy:		true if the control handler is initialized
  */
 struct fimc_ctx {
@@ -507,6 +515,7 @@ struct fimc_ctx {
 	struct v4l2_ctrl	*ctrl_rotate;
 	struct v4l2_ctrl	*ctrl_hflip;
 	struct v4l2_ctrl	*ctrl_vflip;
+	struct v4l2_ctrl	*ctrl_alpha;
 	bool			ctrls_rdy;
 };
 
@@ -576,6 +585,17 @@ static inline int tiled_fmt(struct fimc_fmt *fmt)
 	return fmt->fourcc == V4L2_PIX_FMT_NV12MT;
 }
 
+/* Return the alpha component bit mask */
+static inline int fimc_get_alpha_mask(struct fimc_fmt *fmt)
+{
+	switch (fmt->color) {
+	case S5P_FIMC_RGB444:	return 0x0f;
+	case S5P_FIMC_RGB555:	return 0x01;
+	case S5P_FIMC_RGB888:	return 0xff;
+	default:		return 0;
+	};
+}
+
 static inline void fimc_hw_clear_irq(struct fimc_dev *dev)
 {
 	u32 cfg = readl(dev->regs + S5P_CIGCTRL);
@@ -672,6 +692,7 @@ void fimc_hw_set_prescaler(struct fimc_ctx *ctx);
 void fimc_hw_set_mainscaler(struct fimc_ctx *ctx);
 void fimc_hw_en_capture(struct fimc_ctx *ctx);
 void fimc_hw_set_effect(struct fimc_ctx *ctx, bool active);
+void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx);
 void fimc_hw_set_in_dma(struct fimc_ctx *ctx);
 void fimc_hw_set_input_path(struct fimc_ctx *ctx);
 void fimc_hw_set_output_path(struct fimc_ctx *ctx);
@@ -693,6 +714,7 @@ int fimc_vidioc_enum_fmt_mplane(struct file *file, void *priv,
 int fimc_ctrls_create(struct fimc_ctx *ctx);
 void fimc_ctrls_delete(struct fimc_ctx *ctx);
 void fimc_ctrls_activate(struct fimc_ctx *ctx, bool active);
+void fimc_alpha_ctrl_update(struct fimc_ctx *ctx);
 int fimc_fill_format(struct fimc_frame *frame, struct v4l2_format *f);
 void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 			       struct v4l2_pix_format_mplane *pix);
diff --git a/drivers/media/video/s5p-fimc/fimc-reg.c b/drivers/media/video/s5p-fimc/fimc-reg.c
index 20e664e..24f64c5 100644
--- a/drivers/media/video/s5p-fimc/fimc-reg.c
+++ b/drivers/media/video/s5p-fimc/fimc-reg.c
@@ -114,7 +114,7 @@ void fimc_hw_set_target_format(struct fimc_ctx *ctx)
 		  S5P_CITRGFMT_VSIZE_MASK);
 
 	switch (frame->fmt->color) {
-	case S5P_FIMC_RGB565...S5P_FIMC_RGB888:
+	case S5P_FIMC_RGB444...S5P_FIMC_RGB888:
 		cfg |= S5P_CITRGFMT_RGB;
 		break;
 	case S5P_FIMC_YCBCR420:
@@ -172,6 +172,7 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
 	struct fimc_dev *dev = ctx->fimc_dev;
 	struct fimc_frame *frame = &ctx->d_frame;
 	struct fimc_dma_offset *offset = &frame->dma_offset;
+	struct fimc_fmt *fmt = frame->fmt;
 
 	/* Set the input dma offsets. */
 	cfg = 0;
@@ -195,15 +196,22 @@ void fimc_hw_set_out_dma(struct fimc_ctx *ctx)
 	cfg = readl(dev->regs + S5P_CIOCTRL);
 
 	cfg &= ~(S5P_CIOCTRL_ORDER2P_MASK | S5P_CIOCTRL_ORDER422_MASK |
-		 S5P_CIOCTRL_YCBCR_PLANE_MASK);
+		 S5P_CIOCTRL_YCBCR_PLANE_MASK | S5P_CIOCTRL_RGB16FMT_MASK);
 
-	if (frame->fmt->colplanes == 1)
+	if (fmt->colplanes == 1)
 		cfg |= ctx->out_order_1p;
-	else if (frame->fmt->colplanes == 2)
+	else if (fmt->colplanes == 2)
 		cfg |= ctx->out_order_2p | S5P_CIOCTRL_YCBCR_2PLANE;
-	else if (frame->fmt->colplanes == 3)
+	else if (fmt->colplanes == 3)
 		cfg |= S5P_CIOCTRL_YCBCR_3PLANE;
 
+	if (fmt->color == S5P_FIMC_RGB565)
+		cfg |= S5P_CIOCTRL_RGB565;
+	else if (fmt->color == S5P_FIMC_RGB555)
+		cfg |= S5P_CIOCTRL_ARGB1555;
+	else if (fmt->color == S5P_FIMC_RGB444)
+		cfg |= S5P_CIOCTRL_ARGB4444;
+
 	writel(cfg, dev->regs + S5P_CIOCTRL);
 }
 
@@ -268,22 +276,28 @@ static void fimc_hw_set_scaler(struct fimc_ctx *ctx)
 	if (sc->copy_mode)
 		cfg |= S5P_CISCCTRL_ONE2ONE;
 
-
 	if (ctx->in_path == FIMC_DMA) {
-		if (src_frame->fmt->color == S5P_FIMC_RGB565)
+		switch (src_frame->fmt->color) {
+		case S5P_FIMC_RGB565:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB565;
-		else if (src_frame->fmt->color == S5P_FIMC_RGB666)
+			break;
+		case S5P_FIMC_RGB666:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB666;
-		else if (src_frame->fmt->color == S5P_FIMC_RGB888)
+			break;
+		case S5P_FIMC_RGB888:
 			cfg |= S5P_CISCCTRL_INRGB_FMT_RGB888;
+			break;
+		}
 	}
 
 	if (ctx->out_path == FIMC_DMA) {
-		if (dst_frame->fmt->color == S5P_FIMC_RGB565)
+		u32 color = dst_frame->fmt->color;
+
+		if (color >= S5P_FIMC_RGB444 && color <= S5P_FIMC_RGB565)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB565;
-		else if (dst_frame->fmt->color == S5P_FIMC_RGB666)
+		else if (color == S5P_FIMC_RGB666)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB666;
-		else if (dst_frame->fmt->color == S5P_FIMC_RGB888)
+		else if (color == S5P_FIMC_RGB888)
 			cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
 	} else {
 		cfg |= S5P_CISCCTRL_OUTRGB_FMT_RGB888;
@@ -370,6 +384,21 @@ void fimc_hw_set_effect(struct fimc_ctx *ctx, bool active)
 	writel(cfg, dev->regs + S5P_CIIMGEFF);
 }
 
+void fimc_hw_set_rgb_alpha(struct fimc_ctx *ctx)
+{
+	struct fimc_dev *dev = ctx->fimc_dev;
+	struct fimc_frame *frame = &ctx->d_frame;
+	u32 cfg;
+
+	if (!(frame->fmt->flags & FMT_HAS_ALPHA))
+		return;
+
+	cfg = readl(dev->regs + S5P_CIOCTRL);
+	cfg &= ~S5P_CIOCTRL_ALPHA_OUT_MASK;
+	cfg |= (frame->alpha << 4);
+	writel(cfg, dev->regs + S5P_CIOCTRL);
+}
+
 static void fimc_hw_set_in_dma_size(struct fimc_ctx *ctx)
 {
 	struct fimc_dev *dev = ctx->fimc_dev;
diff --git a/drivers/media/video/s5p-fimc/regs-fimc.h b/drivers/media/video/s5p-fimc/regs-fimc.h
index c8e3b94..c7a5bc5 100644
--- a/drivers/media/video/s5p-fimc/regs-fimc.h
+++ b/drivers/media/video/s5p-fimc/regs-fimc.h
@@ -107,6 +107,11 @@
 #define S5P_CIOCTRL_YCBCR_3PLANE	(0 << 3)
 #define S5P_CIOCTRL_YCBCR_2PLANE	(1 << 3)
 #define S5P_CIOCTRL_YCBCR_PLANE_MASK	(1 << 3)
+#define S5P_CIOCTRL_ALPHA_OUT_MASK	(0xff << 4)
+#define S5P_CIOCTRL_RGB16FMT_MASK	(3 << 16)
+#define S5P_CIOCTRL_RGB565		(0 << 16)
+#define S5P_CIOCTRL_ARGB1555		(1 << 16)
+#define S5P_CIOCTRL_ARGB4444		(2 << 16)
 #define S5P_CIOCTRL_ORDER2P_SHIFT	(24)
 #define S5P_CIOCTRL_ORDER2P_MASK	(3 << 24)
 #define S5P_CIOCTRL_ORDER422_2P_LSB_CRCB (0 << 24)
-- 
1.7.8

Re: [PATCH v2 1/2] v4l: Add new alpha component control

[Hans Verkuil]

On Wednesday, December 14, 2011 14:34:14 Sylwester Nawrocki wrote:
> Hi Hans,
> 
> On 12/13/2011 01:18 PM, Hans Verkuil wrote:
> >> are you going to carry on with the control range update patches ?
> >> I'd like to push the alpha colour control for v3.3 but it depends
> >> on the controls framework updates now.
> > 
> > Good question. I am not sure whether this is something we actually want. It 
> > would make applications much harder to write if the range of a control can 
> > suddenly change.
> > 
> > On the other hand, it might be a good solution for a harder problem which is 
> > as yet unsolved: if you have multiple inputs, and each input has a different 
> > set of controls (e.g. one input is a SDTV receiver, the other is a HDTV 
> > receiver), then you can have the situation where e.g. the contrast control is 
> > present for both inputs, but with a different range. Switching inputs would 
> > then generate a control event telling the app that the range changed.
> > 
> > But this may still be overkill...
> 
> Hmm, it doesn't look like an overkill to me. I'm certain there will be use
> cases where control range update is needed. Maybe we could specify in
> the API in what circumstances the control range update is allowed for drivers.
> So not all applications need to handle the related events.
> 
> Nevertheless I won't be pushing on this, not to mess around in the whole
> API because of some embedded systems requirements.
> So I'm going to update the range for alpha control manually in the driver
> for the time being.

I think I want to wait for other use cases before making these changes.
If it is clear that multiple drivers need this, then we can always add the
support (especially since all the hard work is already done in my patch).

Regards,

	Hans

> 
> > 
> > In other words, I don't know. Not helpful, I agree.
> 
> That was helpful anyway :-) Thanks.
> 
>