[PATCH 0/6] s5p-mfc: Add support for MFC v7 firmware

[PATCH 0/6] s5p-mfc: Add support for MFC v7 firmware

[Arun Kumar K]

This patch series adds MFC v7 firmware support to the Exynos
MFC driver. MFC v7 is present in 5420 SoC which has support
for VP8 encoding and many other features.

Arun Kumar K (6):
  [media] s5p-mfc: Update v6 encoder buffer sizes
  [media] s5p-mfc: Add register definition file for MFC v7
  [media] s5p-mfc: Core support for MFC v7
  [media] s5p-mfc: Update driver for v7 firmware
  [media] V4L: Add VP8 encoder controls
  [media] s5p-mfc: Add support for VP8 encoder

 Documentation/DocBook/media/v4l/controls.xml    |  145 +++++++++++++++++++++++
 drivers/media/platform/s5p-mfc/regs-mfc-v6.h    |    4 +-
 drivers/media/platform/s5p-mfc/regs-mfc-v7.h    |   58 +++++++++
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |   32 +++++
 drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c |    3 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |   21 +++-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |  100 +++++++++++++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |  129 ++++++++++++++++++--
 drivers/media/v4l2-core/v4l2-ctrls.c            |   38 ++++++
 include/uapi/linux/v4l2-controls.h              |   30 ++++-
 10 files changed, 545 insertions(+), 15 deletions(-)
 create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v7.h

-- 
1.7.9.5

[PATCH 1/6] [media] s5p-mfc: Update v6 encoder buffer sizes

[Arun Kumar K]

The patch updates few encoder buffer sizes for MFC v6.5
as per the udpdated user manual. The same buffer sizes
holds good for v7 firmware also.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/regs-mfc-v6.h |    4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
index 363a97c..2398cdf 100644
--- a/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
+++ b/drivers/media/platform/s5p-mfc/regs-mfc-v6.h
@@ -374,9 +374,9 @@
 #define S5P_FIMV_NUM_PIXELS_IN_MB_COL_V6	16
 
 /* Buffer size requirements defined by hardware */
-#define S5P_FIMV_TMV_BUFFER_SIZE_V6(w, h)	(((w) + 1) * ((h) + 1) * 8)
+#define S5P_FIMV_TMV_BUFFER_SIZE_V6(w, h)	(((w) + 1) * ((h) + 3) * 8)
 #define S5P_FIMV_ME_BUFFER_SIZE_V6(imw, imh, mbw, mbh) \
-	((DIV_ROUND_UP(imw, 64) *  DIV_ROUND_UP(imh, 64) * 256) + \
+	(((((imw + 127) / 64) * 16) *  DIV_ROUND_UP(imh, 64) * 256) + \
 	 (DIV_ROUND_UP((mbw) * (mbh), 32) * 16))
 #define S5P_FIMV_SCRATCH_BUF_SIZE_H264_DEC_V6(w, h)	(((w) * 192) + 64)
 #define S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V6(w, h) \
-- 
1.7.9.5

[PATCH 2/6] [media] s5p-mfc: Add register definition file for MFC v7

[Arun Kumar K]

The patch adds the register definition file for new firmware
version v7 for MFC. New firmware supports VP8 encoding along with
many other features.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/regs-mfc-v7.h |   58 ++++++++++++++++++++++++++
 1 file changed, 58 insertions(+)
 create mode 100644 drivers/media/platform/s5p-mfc/regs-mfc-v7.h

diff --git a/drivers/media/platform/s5p-mfc/regs-mfc-v7.h b/drivers/media/platform/s5p-mfc/regs-mfc-v7.h
new file mode 100644
index 0000000..24dba69
--- /dev/null
+++ b/drivers/media/platform/s5p-mfc/regs-mfc-v7.h
@@ -0,0 +1,58 @@
+/*
+ * Register definition file for Samsung MFC V7.x Interface (FIMV) driver
+ *
+ * Copyright (c) 2013 Samsung Electronics Co., Ltd.
+ *		http://www.samsung.com/
+ *
+ * This program is free software; you can redistribute it and/or modify
+ * it under the terms of the GNU General Public License version 2 as
+ * published by the Free Software Foundation.
+ */
+
+#ifndef _REGS_MFC_V7_H
+#define _REGS_MFC_V7_H
+
+#include "regs-mfc-v6.h"
+
+/* Additional features of v7 */
+#define S5P_FIMV_CODEC_VP8_ENC_V7	25
+
+/* Additional registers for v7 */
+#define S5P_FIMV_D_INIT_BUFFER_OPTIONS_V7		0xf47c
+
+#define S5P_FIMV_E_SOURCE_FIRST_ADDR_V7			0xf9e0
+#define S5P_FIMV_E_SOURCE_SECOND_ADDR_V7		0xf9e4
+#define S5P_FIMV_E_SOURCE_THIRD_ADDR_V7			0xf9e8
+#define S5P_FIMV_E_SOURCE_FIRST_STRIDE_V7		0xf9ec
+#define S5P_FIMV_E_SOURCE_SECOND_STRIDE_V7		0xf9f0
+#define S5P_FIMV_E_SOURCE_THIRD_STRIDE_V7		0xf9f4
+
+#define S5P_FIMV_E_ENCODED_SOURCE_FIRST_ADDR_V7		0xfa70
+#define S5P_FIMV_E_ENCODED_SOURCE_SECOND_ADDR_V7	0xfa74
+
+#define S5P_FIMV_E_VP8_OPTIONS_V7			0xfdb0
+#define S5P_FIMV_E_VP8_FILTER_OPTIONS_V7		0xfdb4
+#define S5P_FIMV_E_VP8_GOLDEN_FRAME_OPTION_V7		0xfdb8
+#define S5P_FIMV_E_VP8_NUM_T_LAYER_V7			0xfdc4
+
+/* MFCv7 variant defines */
+#define MAX_FW_SIZE_V7			(SZ_1M)		/* 1MB */
+#define MAX_CPB_SIZE_V7			(3 * SZ_1M)	/* 3MB */
+#define MFC_VERSION_V7			0x72
+#define MFC_NUM_PORTS_V7		1
+
+/* MFCv7 Context buffer sizes */
+#define MFC_CTX_BUF_SIZE_V7		(30 * SZ_1K)	/*  30KB */
+#define MFC_H264_DEC_CTX_BUF_SIZE_V7	(2 * SZ_1M)	/*  2MB */
+#define MFC_OTHER_DEC_CTX_BUF_SIZE_V7	(20 * SZ_1K)	/*  20KB */
+#define MFC_H264_ENC_CTX_BUF_SIZE_V7	(100 * SZ_1K)	/* 100KB */
+#define MFC_OTHER_ENC_CTX_BUF_SIZE_V7	(10 * SZ_1K)	/*  10KB */
+
+/* Buffer size defines */
+#define S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V7(w, h) \
+			(SZ_1M + ((w) * 144) + (8192 * (h)) + 49216)
+
+#define S5P_FIMV_SCRATCH_BUF_SIZE_VP8_ENC_V7(w, h) \
+			(((w) * 48) + (((w) + 1) / 2 * 128) + 144 + 8192)
+
+#endif /*_REGS_MFC_V7_H*/
-- 
1.7.9.5

[PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Arun Kumar K]

Adds variant data and core support for the MFC v7 firmware

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc.c        |   32 +++++++++++++++++++++++
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    2 ++
 2 files changed, 34 insertions(+)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c b/drivers/media/platform/s5p-mfc/s5p_mfc.c
index d12faa6..d6be52f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
@@ -1391,6 +1391,32 @@ static struct s5p_mfc_variant mfc_drvdata_v6 = {
 	.fw_name        = "s5p-mfc-v6.fw",
 };
 
+struct s5p_mfc_buf_size_v6 mfc_buf_size_v7 = {
+	.dev_ctx	= MFC_CTX_BUF_SIZE_V7,
+	.h264_dec_ctx	= MFC_H264_DEC_CTX_BUF_SIZE_V7,
+	.other_dec_ctx	= MFC_OTHER_DEC_CTX_BUF_SIZE_V7,
+	.h264_enc_ctx	= MFC_H264_ENC_CTX_BUF_SIZE_V7,
+	.other_enc_ctx	= MFC_OTHER_ENC_CTX_BUF_SIZE_V7,
+};
+
+struct s5p_mfc_buf_size buf_size_v7 = {
+	.fw	= MAX_FW_SIZE_V7,
+	.cpb	= MAX_CPB_SIZE_V7,
+	.priv	= &mfc_buf_size_v7,
+};
+
+struct s5p_mfc_buf_align mfc_buf_align_v7 = {
+	.base = 0,
+};
+
+static struct s5p_mfc_variant mfc_drvdata_v7 = {
+	.version	= MFC_VERSION_V7,
+	.port_num	= MFC_NUM_PORTS_V7,
+	.buf_size	= &buf_size_v7,
+	.buf_align	= &mfc_buf_align_v7,
+	.fw_name        = "s5p-mfc-v7.fw",
+};
+
 static struct platform_device_id mfc_driver_ids[] = {
 	{
 		.name = "s5p-mfc",
@@ -1401,6 +1427,9 @@ static struct platform_device_id mfc_driver_ids[] = {
 	}, {
 		.name = "s5p-mfc-v6",
 		.driver_data = (unsigned long)&mfc_drvdata_v6,
+	}, {
+		.name = "s5p-mfc-v7",
+		.driver_data = (unsigned long)&mfc_drvdata_v7,
 	},
 	{},
 };
@@ -1413,6 +1442,9 @@ static const struct of_device_id exynos_mfc_match[] = {
 	}, {
 		.compatible = "samsung,mfc-v6",
 		.data = &mfc_drvdata_v6,
+	}, {
+		.compatible = "samsung,mfc-v7",
+		.data = &mfc_drvdata_v7,
 	},
 	{},
 };
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index ef4074c..7281de2 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -24,6 +24,7 @@
 #include <media/videobuf2-core.h>
 #include "regs-mfc.h"
 #include "regs-mfc-v6.h"
+#include "regs-mfc-v7.h"
 
 /* Definitions related to MFC memory */
 
@@ -684,5 +685,6 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx);
 				(dev->variant->port_num ? 1 : 0) : 0) : 0)
 #define IS_TWOPORT(dev)		(dev->variant->port_num == 2 ? 1 : 0)
 #define IS_MFCV6(dev)		(dev->variant->version >= 0x60 ? 1 : 0)
+#define IS_MFCV7(dev)		(dev->variant->version >= 0x70 ? 1 : 0)
 
 #endif /* S5P_MFC_COMMON_H_ */
-- 
1.7.9.5

[PATCH 4/6] [media] s5p-mfc: Update driver for v7 firmware

[Arun Kumar K]

Firmware version v7 is mostly similar to v6 in terms
of hardware specific controls and commands. So the hardware
specific opr_v6 and cmd_v6 are re-used for v7 also. This patch
updates the v6 files to handle v7 version also.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |   12 ++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   53 +++++++++++++++++++----
 2 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 2549967..13799a8 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -1662,8 +1662,16 @@ static int s5p_mfc_queue_setup(struct vb2_queue *vq,
 			*buf_count = 1;
 		if (*buf_count > MFC_MAX_BUFFERS)
 			*buf_count = MFC_MAX_BUFFERS;
-		psize[0] = ctx->luma_size;
-		psize[1] = ctx->chroma_size;
+
+		if (IS_MFCV7(dev)) {
+			/* MFCv7 needs pad bytes for input YUV */
+			psize[0] = ctx->luma_size + 256;
+			psize[1] = ctx->chroma_size + 128;
+		} else {
+			psize[0] = ctx->luma_size;
+			psize[1] = ctx->chroma_size;
+		}
+
 		if (IS_MFCV6(dev)) {
 			allocators[0] =
 				ctx->dev->alloc_ctx[MFC_BANK1_ALLOC_CTX];
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 7d4c5e1..7145ae5 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -80,6 +80,7 @@ static int s5p_mfc_alloc_codec_buffers_v6(struct s5p_mfc_ctx *ctx)
 		ctx->tmv_buffer_size = S5P_FIMV_NUM_TMV_BUFFERS_V6 *
 			ALIGN(S5P_FIMV_TMV_BUFFER_SIZE_V6(mb_width, mb_height),
 			S5P_FIMV_TMV_BUFFER_ALIGN_V6);
+
 		ctx->luma_dpb_size = ALIGN((mb_width * mb_height) *
 				S5P_FIMV_LUMA_MB_TO_PIXEL_V6,
 				S5P_FIMV_LUMA_DPB_BUFFER_ALIGN_V6);
@@ -112,10 +113,18 @@ static int s5p_mfc_alloc_codec_buffers_v6(struct s5p_mfc_ctx *ctx)
 			(ctx->mv_count * ctx->mv_size);
 		break;
 	case S5P_MFC_CODEC_MPEG4_DEC:
-		ctx->scratch_buf_size =
-			S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V6(
-					mb_width,
-					mb_height);
+		if (IS_MFCV7(dev)) {
+			ctx->scratch_buf_size =
+				S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V7(
+						mb_width,
+						mb_height);
+		} else {
+			ctx->scratch_buf_size =
+				S5P_FIMV_SCRATCH_BUF_SIZE_MPEG4_DEC_V6(
+						mb_width,
+						mb_height);
+		}
+
 		ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size,
 				S5P_FIMV_SCRATCH_BUFFER_ALIGN_V6);
 		ctx->bank1.size = ctx->scratch_buf_size;
@@ -453,8 +462,13 @@ static void s5p_mfc_set_enc_frame_buffer_v6(struct s5p_mfc_ctx *ctx,
 {
 	struct s5p_mfc_dev *dev = ctx->dev;
 
-	WRITEL(y_addr, S5P_FIMV_E_SOURCE_LUMA_ADDR_V6); /* 256B align */
-	WRITEL(c_addr, S5P_FIMV_E_SOURCE_CHROMA_ADDR_V6);
+	if (IS_MFCV7(dev)) {
+		WRITEL(y_addr, S5P_FIMV_E_SOURCE_FIRST_ADDR_V7);
+		WRITEL(c_addr, S5P_FIMV_E_SOURCE_SECOND_ADDR_V7);
+	} else {
+		WRITEL(y_addr, S5P_FIMV_E_SOURCE_LUMA_ADDR_V6);
+		WRITEL(c_addr, S5P_FIMV_E_SOURCE_CHROMA_ADDR_V6);
+	}
 
 	mfc_debug(2, "enc src y buf addr: 0x%08lx\n", y_addr);
 	mfc_debug(2, "enc src c buf addr: 0x%08lx\n", c_addr);
@@ -466,8 +480,13 @@ static void s5p_mfc_get_enc_frame_buffer_v6(struct s5p_mfc_ctx *ctx,
 	struct s5p_mfc_dev *dev = ctx->dev;
 	unsigned long enc_recon_y_addr, enc_recon_c_addr;
 
-	*y_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_LUMA_ADDR_V6);
-	*c_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_CHROMA_ADDR_V6);
+	if (IS_MFCV7(dev)) {
+		*y_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_FIRST_ADDR_V7);
+		*c_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_SECOND_ADDR_V7);
+	} else {
+		*y_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_LUMA_ADDR_V6);
+		*c_addr = READL(S5P_FIMV_E_ENCODED_SOURCE_CHROMA_ADDR_V6);
+	}
 
 	enc_recon_y_addr = READL(S5P_FIMV_E_RECON_LUMA_DPB_ADDR_V6);
 	enc_recon_c_addr = READL(S5P_FIMV_E_RECON_CHROMA_DPB_ADDR_V6);
@@ -1166,6 +1185,12 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 		reg |= (0x1 << S5P_FIMV_D_OPT_DDELAY_EN_SHIFT_V6);
 		WRITEL(ctx->display_delay, S5P_FIMV_D_DISPLAY_DELAY_V6);
 	}
+
+	if (IS_MFCV7(dev)) {
+		WRITEL(reg, S5P_FIMV_D_DEC_OPTIONS_V6);
+		reg = 0;
+	}
+
 	/* Setup loop filter, for decoding this is only valid for MPEG4 */
 	if (ctx->codec_mode == S5P_MFC_CODEC_MPEG4_DEC) {
 		mfc_debug(2, "Set loop filter to: %d\n",
@@ -1176,7 +1201,10 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 	if (ctx->dst_fmt->fourcc == V4L2_PIX_FMT_NV12MT_16X16)
 		reg |= (0x1 << S5P_FIMV_D_OPT_TILE_MODE_SHIFT_V6);
 
-	WRITEL(reg, S5P_FIMV_D_DEC_OPTIONS_V6);
+	if (IS_MFCV7(dev))
+		WRITEL(reg, S5P_FIMV_D_INIT_BUFFER_OPTIONS_V7);
+	else
+		WRITEL(reg, S5P_FIMV_D_DEC_OPTIONS_V6);
 
 	/* 0: NV12(CbCr), 1: NV21(CrCb) */
 	if (ctx->dst_fmt->fourcc == V4L2_PIX_FMT_NV21M)
@@ -1184,6 +1212,7 @@ static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 	else
 		WRITEL(0x0, S5P_FIMV_PIXEL_FORMAT_V6);
 
+
 	/* sei parse */
 	WRITEL(ctx->sei_fp_parse & 0x1, S5P_FIMV_D_SEI_ENABLE_V6);
 
@@ -1254,6 +1283,12 @@ static int s5p_mfc_init_encode_v6(struct s5p_mfc_ctx *ctx)
 		return -EINVAL;
 	}
 
+	/* Set stride lengths */
+	if (IS_MFCV7(dev)) {
+		WRITEL(ctx->img_width, S5P_FIMV_E_SOURCE_FIRST_STRIDE_V7);
+		WRITEL(ctx->img_width, S5P_FIMV_E_SOURCE_SECOND_STRIDE_V7);
+	}
+
 	WRITEL(ctx->inst_no, S5P_FIMV_INSTANCE_ID_V6);
 	s5p_mfc_hw_call(dev->mfc_cmds, cmd_host2risc, dev,
 			S5P_FIMV_CH_SEQ_HEADER_V6, NULL);
-- 
1.7.9.5

[PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

This patch adds new V4L controls for VP8 encoding.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
---
 Documentation/DocBook/media/v4l/controls.xml |  145 ++++++++++++++++++++++++++
 drivers/media/v4l2-core/v4l2-ctrls.c         |   38 +++++++
 include/uapi/linux/v4l2-controls.h           |   30 +++++-
 3 files changed, 212 insertions(+), 1 deletion(-)

diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
index 8d7a779..db614c7 100644
--- a/Documentation/DocBook/media/v4l/controls.xml
+++ b/Documentation/DocBook/media/v4l/controls.xml
@@ -4772,4 +4772,149 @@ defines possible values for de-emphasis. Here they are:</entry>
       </table>
 
       </section>
+
+    <section id="vpx-controls">
+      <title>VPX Control Reference</title>
+
+      <para>The VPX control class includes controls for encoding parameters
+      of VPx video codec.</para>
+
+      <table pgwide="1" frame="none" id="fm-rx-control-id">
+      <title>VPX Control IDs</title>
+
+      <tgroup cols="4">
+        <colspec colname="c1" colwidth="1*" />
+        <colspec colname="c2" colwidth="6*" />
+        <colspec colname="c3" colwidth="2*" />
+        <colspec colname="c4" colwidth="6*" />
+        <spanspec namest="c1" nameend="c2" spanname="id" />
+        <spanspec namest="c2" nameend="c4" spanname="descr" />
+        <thead>
+          <row>
+            <entry spanname="id" align="left">ID</entry>
+            <entry align="left">Type</entry>
+          </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
+          </row>
+        </thead>
+        <tbody valign="top">
+          <row><entry></entry></row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-num-partitions">
+		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_PARTITIONS</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_num_partitions</entry>
+	      </row>
+	      <row><entry spanname="descr">The number of token partitions to use in VP8 encoder.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_1_PARTITION</constant>&nbsp;</entry>
+		      <entry>1 coefficient partition</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_2_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>2 partitions</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_4_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>4 partitions</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_8_PARTITIONS</constant>&nbsp;</entry>
+		      <entry>8 partitions</entry>
+	            </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_IMD_DISABLE_4X4</constant>&nbsp;</entry>
+		<entry>boolean</entry>
+	      </row>
+	      <row><entry spanname="descr">Setting this prevents intra 4x4 mode in the intra mode decision.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-num-ref-frames">
+		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_REF_FRAMES</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_num_ref_frames</entry>
+	      </row>
+	      <row><entry spanname="descr">The number of reference pictures for encoding P frames.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_1_REF_FRAME</constant>&nbsp;</entry>
+		      <entry>Last encoded frame will be searched</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_2_REF_FRAME</constant>&nbsp;</entry>
+		      <entry>Last encoded frame and the Golden frame will be searched</entry>
+		    </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_LEVEL</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">Indicates the loop filter level. The adjustment of loop
+filter level is done via a delta value against a baseline loop filter value.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_SHARPNESS</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">This parameter affects the loop filter. Anything above
+zero weakens the deblocking effect on loop filter.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row>
+		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
+		<entry>integer</entry>
+	      </row>
+	      <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>
+	      </row>
+
+	      <row><entry></entry></row>
+	      <row id="v4l2-vpx-golden-frame-sel">
+		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
+		<entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
+	      </row>
+	      <row><entry spanname="descr">Selects the golden frame for encoding.
+Possible values are:</entry>
+	      </row>
+	      <row>
+		<entrytbl spanname="descr" cols="2">
+		  <tbody valign="top">
+		    <row>
+		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
+		      <entry>Use the previous second frame as a golden frame</entry>
+		    </row>
+		    <row>
+		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
+		      <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
+		    </row>
+                  </tbody>
+		</entrytbl>
+	      </row>
+
+          <row><entry></entry></row>
+        </tbody>
+      </tgroup>
+      </table>
+
+      </section>
+
 </section>
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index fccd08b..2cf17d4 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 		"RGB full range (0-255)",
 		NULL,
 	};
+	static const char * const vpx_num_partitions[] = {
+		"1 partition",
+		"2 partitions",
+		"4 partitions",
+		"8 partitions",
+		NULL,
+	};
+	static const char * const vpx_num_ref_frames[] = {
+		"1 reference frame",
+		"2 reference frame",
+		NULL,
+	};
+	static const char * const vpx_golden_frame_sel[] = {
+		"Use previous frame",
+		"Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
+		NULL,
+	};
 
 
 	switch (id) {
@@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 	case V4L2_CID_DV_TX_RGB_RANGE:
 	case V4L2_CID_DV_RX_RGB_RANGE:
 		return dv_rgb_range;
+	case V4L2_CID_VPX_NUM_PARTITIONS:
+		return vpx_num_partitions;
+	case V4L2_CID_VPX_NUM_REF_FRAMES:
+		return vpx_num_ref_frames;
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
+		return vpx_golden_frame_sel;
 
 	default:
 		return NULL;
@@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
 	case V4L2_CID_FM_RX_CLASS:		return "FM Radio Receiver Controls";
 	case V4L2_CID_TUNE_DEEMPHASIS:		return "De-Emphasis";
 	case V4L2_CID_RDS_RECEPTION:		return "RDS Reception";
+
+	/* VPX controls */
+	case V4L2_CID_VPX_CLASS:		return "VPX Encoder Controls";
+	case V4L2_CID_VPX_NUM_PARTITIONS:	return "VPX Number of partitions";
+	case V4L2_CID_VPX_IMD_DISABLE_4X4:	return "VPX Intra mode decision disable";
+	case V4L2_CID_VPX_NUM_REF_FRAMES:	return "VPX Number of reference pictures for P frames";
+	case V4L2_CID_VPX_FILTER_LEVEL:		return "VPX Loop filter level range";
+	case V4L2_CID_VPX_FILTER_SHARPNESS:	return "VPX Deblocking effect control";
+	case V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD:	return "VPX Golden frame refresh period";
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:	return "VPX Golden frame indicator";
+
 	default:
 		return NULL;
 	}
@@ -914,6 +948,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_DV_RX_RGB_RANGE:
 	case V4L2_CID_TEST_PATTERN:
 	case V4L2_CID_TUNE_DEEMPHASIS:
+	case V4L2_CID_VPX_NUM_PARTITIONS:
+	case V4L2_CID_VPX_NUM_REF_FRAMES:
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
 		*type = V4L2_CTRL_TYPE_MENU;
 		break;
 	case V4L2_CID_LINK_FREQ:
@@ -937,6 +974,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
 	case V4L2_CID_IMAGE_PROC_CLASS:
 	case V4L2_CID_DV_CLASS:
 	case V4L2_CID_FM_RX_CLASS:
+	case V4L2_CID_VPX_CLASS:
 		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
 		/* You can neither read not write these */
 		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
index 69bd5bb..3d6649c 100644
--- a/include/uapi/linux/v4l2-controls.h
+++ b/include/uapi/linux/v4l2-controls.h
@@ -60,6 +60,7 @@
 #define V4L2_CTRL_CLASS_IMAGE_PROC	0x009f0000	/* Image processing controls */
 #define V4L2_CTRL_CLASS_DV		0x00a00000	/* Digital Video controls */
 #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* Digital Video controls */
+#define V4L2_CTRL_CLASS_VPX		0x00a20000	/* VPX-compression controls */
 
 /* User-class control IDs */
 
@@ -818,7 +819,6 @@ enum v4l2_jpeg_chroma_subsampling {
 #define V4L2_CID_PIXEL_RATE			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 2)
 #define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
 
-
 /*  DV-class control IDs defined by V4L2 */
 #define V4L2_CID_DV_CLASS_BASE			(V4L2_CTRL_CLASS_DV | 0x900)
 #define V4L2_CID_DV_CLASS			(V4L2_CTRL_CLASS_DV | 1)
@@ -853,4 +853,32 @@ enum v4l2_deemphasis {
 
 #define V4L2_CID_RDS_RECEPTION			(V4L2_CID_FM_RX_CLASS_BASE + 2)
 
+/* VP-class control IDs */
+
+#define V4L2_CID_VPX_BASE			(V4L2_CTRL_CLASS_VPX | 0x900)
+#define V4L2_CID_VPX_CLASS			(V4L2_CTRL_CLASS_VPX | 1)
+
+/*  VPX streams, specific to multiplexed streams */
+#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
+enum v4l2_vp8_num_partitions {
+	V4L2_VPX_1_PARTITION	= 0,
+	V4L2_VPX_2_PARTITIONS	= (1 << 1),
+	V4L2_VPX_4_PARTITIONS	= (1 << 2),
+	V4L2_VPX_8_PARTITIONS	= (1 << 3),
+};
+#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
+#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
+enum v4l2_vp8_num_ref_frames {
+	V4L2_VPX_1_REF_FRAME	= 0,
+	V4L2_VPX_2_REF_FRAME	= 1,
+};
+#define V4L2_CID_VPX_FILTER_LEVEL		(V4L2_CID_VPX_BASE+3)
+#define V4L2_CID_VPX_FILTER_SHARPNESS		(V4L2_CID_VPX_BASE+4)
+#define V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD	(V4L2_CID_VPX_BASE+5)
+#define V4L2_CID_VPX_GOLDEN_FRAME_SEL		(V4L2_CID_VPX_BASE+6)
+enum v4l2_vp8_golden_frame_sel {
+	V4L2_VPX_GOLDEN_FRAME_USE_PREV		= 0,
+	V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD	= 1,
+};
+
 #endif
-- 
1.7.9.5

[PATCH 6/6] [media] s5p-mfc: Add support for VP8 encoder

[Arun Kumar K]

MFC v7 supports VP8 encoding and this patch adds support
for it in the driver.

Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
---
 drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c |    3 +
 drivers/media/platform/s5p-mfc/s5p_mfc_common.h |   19 ++++-
 drivers/media/platform/s5p-mfc/s5p_mfc_enc.c    |   88 +++++++++++++++++++++++
 drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c |   76 ++++++++++++++++++++
 4 files changed, 185 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c
index 5708fc3..db796c8 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_cmd_v6.c
@@ -108,6 +108,9 @@ static int s5p_mfc_open_inst_cmd_v6(struct s5p_mfc_ctx *ctx)
 	case S5P_MFC_CODEC_H263_ENC:
 		codec_type = S5P_FIMV_CODEC_H263_ENC_V6;
 		break;
+	case S5P_MFC_CODEC_VP8_ENC:
+		codec_type = S5P_FIMV_CODEC_VP8_ENC_V7;
+		break;
 	default:
 		codec_type = S5P_FIMV_CODEC_NONE_V6;
 	};
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
index 7281de2..a9d4593 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
@@ -65,7 +65,7 @@ static inline dma_addr_t s5p_mfc_mem_cookie(void *a, void *b)
 #define MFC_ENC_CAP_PLANE_COUNT	1
 #define MFC_ENC_OUT_PLANE_COUNT	2
 #define STUFF_BYTE		4
-#define MFC_MAX_CTRLS		70
+#define MFC_MAX_CTRLS		77
 
 #define S5P_MFC_CODEC_NONE		-1
 #define S5P_MFC_CODEC_H264_DEC		0
@@ -81,6 +81,7 @@ static inline dma_addr_t s5p_mfc_mem_cookie(void *a, void *b)
 #define S5P_MFC_CODEC_H264_MVC_ENC	21
 #define S5P_MFC_CODEC_MPEG4_ENC		22
 #define S5P_MFC_CODEC_H263_ENC		23
+#define S5P_MFC_CODEC_VP8_ENC		24
 
 #define S5P_MFC_R2H_CMD_EMPTY			0
 #define S5P_MFC_R2H_CMD_SYS_INIT_RET		1
@@ -409,6 +410,21 @@ struct s5p_mfc_mpeg4_enc_params {
 };
 
 /**
+ * struct s5p_mfc_vp8_enc_params - encoding parameters for vp8
+ */
+struct s5p_mfc_vp8_enc_params {
+	u8 imd_4x4;
+	enum v4l2_vp8_num_partitions num_partitions;
+	enum v4l2_vp8_num_ref_frames num_ref;
+	u8 filter_level;
+	u8 filter_sharpness;
+	u32 golden_frame_ref_period;
+	enum v4l2_vp8_golden_frame_sel golden_frame_sel;
+	u8 hier_layer;
+	u8 hier_layer_qp[3];
+};
+
+/**
  * struct s5p_mfc_enc_params - general encoding parameters
  */
 struct s5p_mfc_enc_params {
@@ -442,6 +458,7 @@ struct s5p_mfc_enc_params {
 	struct {
 		struct s5p_mfc_h264_enc_params h264;
 		struct s5p_mfc_mpeg4_enc_params mpeg4;
+		struct s5p_mfc_vp8_enc_params vp8;
 	} codec;
 
 };
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
index 13799a8..146462d 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_enc.c
@@ -84,6 +84,13 @@ static struct s5p_mfc_fmt formats[] = {
 		.type		= MFC_FMT_ENC,
 		.num_planes	= 1,
 	},
+	{
+		.name		= "VP8 Encoded Stream",
+		.fourcc		= V4L2_PIX_FMT_VP8,
+		.codec_mode	= S5P_MFC_CODEC_VP8_ENC,
+		.type		= MFC_FMT_ENC,
+		.num_planes	= 1,
+	},
 };
 
 #define NUM_FORMATS ARRAY_SIZE(formats)
@@ -557,6 +564,62 @@ static struct mfc_control controls[] = {
 		.step = 1,
 		.default_value = 0,
 	},
+	{
+		.id = V4L2_CID_VPX_NUM_PARTITIONS,
+		.type = V4L2_CTRL_TYPE_MENU,
+		.minimum = V4L2_VPX_1_PARTITION,
+		.maximum = V4L2_VPX_8_PARTITIONS,
+		.default_value = V4L2_VPX_1_PARTITION,
+		.menu_skip_mask = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_IMD_DISABLE_4X4,
+		.type = V4L2_CTRL_TYPE_BOOLEAN,
+		.minimum = 0,
+		.maximum = 1,
+		.step = 1,
+		.default_value = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_NUM_REF_FRAMES,
+		.type = V4L2_CTRL_TYPE_MENU,
+		.minimum = V4L2_VPX_1_REF_FRAME,
+		.maximum = V4L2_VPX_2_REF_FRAME,
+		.default_value = V4L2_VPX_1_REF_FRAME,
+		.menu_skip_mask = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_FILTER_LEVEL,
+		.type = V4L2_CTRL_TYPE_INTEGER,
+		.minimum = 0,
+		.maximum = 63,
+		.step = 1,
+		.default_value = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_FILTER_SHARPNESS,
+		.type = V4L2_CTRL_TYPE_INTEGER,
+		.minimum = 0,
+		.maximum = 7,
+		.step = 1,
+		.default_value = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD,
+		.type = V4L2_CTRL_TYPE_INTEGER,
+		.minimum = 0,
+		.maximum = (1 << 16) - 1,
+		.step = 1,
+		.default_value = 0,
+	},
+	{
+		.id = V4L2_CID_VPX_GOLDEN_FRAME_SEL,
+		.type = V4L2_CTRL_TYPE_MENU,
+		.minimum = V4L2_VPX_GOLDEN_FRAME_USE_PREV,
+		.maximum = V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD,
+		.default_value = V4L2_VPX_GOLDEN_FRAME_USE_PREV,
+		.menu_skip_mask = 0,
+	},
 };
 
 #define NUM_CTRLS ARRAY_SIZE(controls)
@@ -965,6 +1028,10 @@ static int vidioc_s_fmt(struct file *file, void *priv, struct v4l2_format *f)
 			mfc_err("failed to set capture format\n");
 			return -EINVAL;
 		}
+		if (!IS_MFCV7(dev) && (fmt->fourcc == V4L2_PIX_FMT_VP8)) {
+			mfc_err("VP8 is supported only in MFC v7\n");
+			return -EINVAL;
+		}
 		ctx->state = MFCINST_INIT;
 		ctx->dst_fmt = fmt;
 		ctx->codec_mode = ctx->dst_fmt->codec_mode;
@@ -1482,6 +1549,27 @@ static int s5p_mfc_enc_s_ctrl(struct v4l2_ctrl *ctrl)
 	case V4L2_CID_MPEG_VIDEO_MPEG4_QPEL:
 		p->codec.mpeg4.quarter_pixel = ctrl->val;
 		break;
+	case V4L2_CID_VPX_NUM_PARTITIONS:
+		p->codec.vp8.num_partitions = ctrl->val;
+		break;
+	case V4L2_CID_VPX_IMD_DISABLE_4X4:
+		p->codec.vp8.imd_4x4 = ctrl->val;
+		break;
+	case V4L2_CID_VPX_NUM_REF_FRAMES:
+		p->codec.vp8.num_ref = ctrl->val;
+		break;
+	case V4L2_CID_VPX_FILTER_LEVEL:
+		p->codec.vp8.filter_level = ctrl->val;
+		break;
+	case V4L2_CID_VPX_FILTER_SHARPNESS:
+		p->codec.vp8.filter_sharpness = ctrl->val;
+		break;
+	case V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD:
+		p->codec.vp8.golden_frame_ref_period = ctrl->val;
+		break;
+	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
+		p->codec.vp8.golden_frame_sel = ctrl->val;
+		break;
 	default:
 		v4l2_err(&dev->v4l2_dev, "Invalid control, id=%d, val=%d\n",
 							ctrl->id, ctrl->val);
diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
index 7145ae5..ed9f39f 100644
--- a/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
+++ b/drivers/media/platform/s5p-mfc/s5p_mfc_opr_v6.c
@@ -188,6 +188,19 @@ static int s5p_mfc_alloc_codec_buffers_v6(struct s5p_mfc_ctx *ctx)
 			ctx->chroma_dpb_size + ctx->me_buffer_size));
 		ctx->bank2.size = 0;
 		break;
+	case S5P_MFC_CODEC_VP8_ENC:
+		ctx->scratch_buf_size =
+			S5P_FIMV_SCRATCH_BUF_SIZE_VP8_ENC_V7(
+					mb_width,
+					mb_height);
+		ctx->scratch_buf_size = ALIGN(ctx->scratch_buf_size,
+				S5P_FIMV_SCRATCH_BUFFER_ALIGN_V6);
+		ctx->bank1.size =
+			ctx->scratch_buf_size + ctx->tmv_buffer_size +
+			(ctx->pb_count * (ctx->luma_dpb_size +
+			ctx->chroma_dpb_size + ctx->me_buffer_size));
+		ctx->bank2.size = 0;
+		break;
 	default:
 		break;
 	}
@@ -237,6 +250,7 @@ static int s5p_mfc_alloc_instance_buffer_v6(struct s5p_mfc_ctx *ctx)
 		break;
 	case S5P_MFC_CODEC_MPEG4_ENC:
 	case S5P_MFC_CODEC_H263_ENC:
+	case S5P_MFC_CODEC_VP8_ENC:
 		ctx->ctx.size = buf_size->other_enc_ctx;
 		break;
 	default:
@@ -1159,6 +1173,66 @@ static int s5p_mfc_set_enc_params_h263(struct s5p_mfc_ctx *ctx)
 	return 0;
 }
 
+static int s5p_mfc_set_enc_params_vp8(struct s5p_mfc_ctx *ctx)
+{
+	struct s5p_mfc_dev *dev = ctx->dev;
+	struct s5p_mfc_enc_params *p = &ctx->enc_params;
+	struct s5p_mfc_vp8_enc_params *p_vp8 = &p->codec.vp8;
+	unsigned int reg = 0;
+
+	mfc_debug_enter();
+
+	s5p_mfc_set_enc_params(ctx);
+
+	/* pictype : number of B */
+	reg = READL(S5P_FIMV_E_GOP_CONFIG_V6);
+	reg &= ~(0x3 << 16);
+	reg |= ((p->num_b_frame & 0x3) << 16);
+	WRITEL(reg, S5P_FIMV_E_GOP_CONFIG_V6);
+
+	/* profile & level */
+	reg = 0;
+	/** profile */
+	reg |= (0x1 << 4);
+	WRITEL(reg, S5P_FIMV_E_PICTURE_PROFILE_V6);
+
+	/* rate control config. */
+	reg = READL(S5P_FIMV_E_RC_CONFIG_V6);
+	/** macroblock level rate control */
+	reg &= ~(0x1 << 8);
+	reg |= ((p->rc_mb & 0x1) << 8);
+	WRITEL(reg, S5P_FIMV_E_RC_CONFIG_V6);
+
+	/* frame rate */
+	if (p->rc_frame && p->rc_framerate_num && p->rc_framerate_denom) {
+		reg = 0;
+		reg |= ((p->rc_framerate_num & 0xFFFF) << 16);
+		reg |= p->rc_framerate_denom & 0xFFFF;
+		WRITEL(reg, S5P_FIMV_E_RC_FRAME_RATE_V6);
+	}
+
+	/* vbv buffer size */
+	if (p->frame_skip_mode ==
+			V4L2_MPEG_MFC51_VIDEO_FRAME_SKIP_MODE_BUF_LIMIT) {
+		WRITEL(p->vbv_size & 0xFFFF, S5P_FIMV_E_VBV_BUFFER_SIZE_V6);
+
+		if (p->rc_frame)
+			WRITEL(p->vbv_delay, S5P_FIMV_E_VBV_INIT_DELAY_V6);
+	}
+
+	/* VP8 specific params */
+	/* Config */
+	reg = 0;
+	reg |= (p_vp8->imd_4x4 & 0x1) << 10;
+	reg |= (p_vp8->num_partitions & 0xF) << 3;
+	reg |= (p_vp8->num_ref & 0x2);
+	WRITEL(reg, S5P_FIMV_E_VP8_OPTIONS_V7);
+
+	mfc_debug_leave();
+
+	return 0;
+}
+
 /* Initialize decoding */
 static int s5p_mfc_init_decode_v6(struct s5p_mfc_ctx *ctx)
 {
@@ -1277,6 +1351,8 @@ static int s5p_mfc_init_encode_v6(struct s5p_mfc_ctx *ctx)
 		s5p_mfc_set_enc_params_mpeg4(ctx);
 	else if (ctx->codec_mode == S5P_MFC_CODEC_H263_ENC)
 		s5p_mfc_set_enc_params_h263(ctx);
+	else if (ctx->codec_mode == S5P_MFC_CODEC_VP8_ENC)
+		s5p_mfc_set_enc_params_vp8(ctx);
 	else {
 		mfc_err("Unknown codec for encoding (%x).\n",
 			ctx->codec_mode);
-- 
1.7.9.5

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Hans Verkuil]

Hi Arun,

Some review comments below...

On Mon June 10 2013 15:23:05 Arun Kumar K wrote:
> This patch adds new V4L controls for VP8 encoding.
> 
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> Signed-off-by: Kiran AVND <avnd.kiran@samsung.com>
> ---
>  Documentation/DocBook/media/v4l/controls.xml |  145 ++++++++++++++++++++++++++
>  drivers/media/v4l2-core/v4l2-ctrls.c         |   38 +++++++
>  include/uapi/linux/v4l2-controls.h           |   30 +++++-
>  3 files changed, 212 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/DocBook/media/v4l/controls.xml b/Documentation/DocBook/media/v4l/controls.xml
> index 8d7a779..db614c7 100644
> --- a/Documentation/DocBook/media/v4l/controls.xml
> +++ b/Documentation/DocBook/media/v4l/controls.xml
> @@ -4772,4 +4772,149 @@ defines possible values for de-emphasis. Here they are:</entry>
>        </table>
>  
>        </section>
> +
> +    <section id="vpx-controls">
> +      <title>VPX Control Reference</title>
> +
> +      <para>The VPX control class includes controls for encoding parameters
> +      of VPx video codec.</para>

Are these controls defined by the VPx standard, or are they specific to the
Samsung hardware?

I am not certain whether a separate class should be created for these. Adding
it to the MPEG control class might be a better approach (yes, I know MPEG is
a bit of a misnomer, but it already handles other compressions standards as
well).

> +
> +      <table pgwide="1" frame="none" id="fm-rx-control-id">
> +      <title>VPX Control IDs</title>
> +
> +      <tgroup cols="4">
> +        <colspec colname="c1" colwidth="1*" />
> +        <colspec colname="c2" colwidth="6*" />
> +        <colspec colname="c3" colwidth="2*" />
> +        <colspec colname="c4" colwidth="6*" />
> +        <spanspec namest="c1" nameend="c2" spanname="id" />
> +        <spanspec namest="c2" nameend="c4" spanname="descr" />
> +        <thead>
> +          <row>
> +            <entry spanname="id" align="left">ID</entry>
> +            <entry align="left">Type</entry>
> +          </row><row rowsep="1"><entry spanname="descr" align="left">Description</entry>
> +          </row>
> +        </thead>
> +        <tbody valign="top">
> +          <row><entry></entry></row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-num-partitions">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_PARTITIONS</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_num_partitions</entry>
> +	      </row>
> +	      <row><entry spanname="descr">The number of token partitions to use in VP8 encoder.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_1_PARTITION</constant>&nbsp;</entry>
> +		      <entry>1 coefficient partition</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_2_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>2 partitions</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_4_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>4 partitions</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_8_PARTITIONS</constant>&nbsp;</entry>
> +		      <entry>8 partitions</entry>
> +	            </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_IMD_DISABLE_4X4</constant>&nbsp;</entry>
> +		<entry>boolean</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Setting this prevents intra 4x4 mode in the intra mode decision.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-num-ref-frames">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_NUM_REF_FRAMES</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_num_ref_frames</entry>
> +	      </row>
> +	      <row><entry spanname="descr">The number of reference pictures for encoding P frames.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_1_REF_FRAME</constant>&nbsp;</entry>
> +		      <entry>Last encoded frame will be searched</entry>
> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_2_REF_FRAME</constant>&nbsp;</entry>
> +		      <entry>Last encoded frame and the Golden frame will be searched</entry>
> +		    </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_LEVEL</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Indicates the loop filter level. The adjustment of loop
> +filter level is done via a delta value against a baseline loop filter value.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_FILTER_SHARPNESS</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">This parameter affects the loop filter. Anything above
> +zero weakens the deblocking effect on loop filter.</entry>
> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row>
> +		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
> +		<entry>integer</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>

The period is in number of frames I assume? And is the golden frame taken at
the start or at the end of the period?

> +	      </row>
> +
> +	      <row><entry></entry></row>
> +	      <row id="v4l2-vpx-golden-frame-sel">
> +		<entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
> +		<entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
> +	      </row>
> +	      <row><entry spanname="descr">Selects the golden frame for encoding.
> +Possible values are:</entry>
> +	      </row>
> +	      <row>
> +		<entrytbl spanname="descr" cols="2">
> +		  <tbody valign="top">
> +		    <row>
> +		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
> +		      <entry>Use the previous second frame as a golden frame</entry>

Second frame of what? It's not clear to me how I should interpret this.

> +		    </row>
> +		    <row>
> +		      <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
> +		      <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
> +		    </row>
> +                  </tbody>
> +		</entrytbl>
> +	      </row>
> +
> +          <row><entry></entry></row>
> +        </tbody>
> +      </tgroup>
> +      </table>
> +
> +      </section>
> +
>  </section>
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index fccd08b..2cf17d4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"RGB full range (0-255)",
>  		NULL,
>  	};
> +	static const char * const vpx_num_partitions[] = {
> +		"1 partition",
> +		"2 partitions",
> +		"4 partitions",
> +		"8 partitions",

Please use a capital P for Partition.

> +		NULL,
> +	};
> +	static const char * const vpx_num_ref_frames[] = {
> +		"1 reference frame",
> +		"2 reference frame",

frame -> Frames

Capitalize these strings. Same for all the others.

> +		NULL,
> +	};
> +	static const char * const vpx_golden_frame_sel[] = {
> +		"Use previous frame",
> +		"Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
> +		NULL,
> +	};
>  
>  
>  	switch (id) {
> @@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  	case V4L2_CID_DV_TX_RGB_RANGE:
>  	case V4L2_CID_DV_RX_RGB_RANGE:
>  		return dv_rgb_range;
> +	case V4L2_CID_VPX_NUM_PARTITIONS:
> +		return vpx_num_partitions;
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:
> +		return vpx_num_ref_frames;
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
> +		return vpx_golden_frame_sel;
>  
>  	default:
>  		return NULL;
> @@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
>  	case V4L2_CID_FM_RX_CLASS:		return "FM Radio Receiver Controls";
>  	case V4L2_CID_TUNE_DEEMPHASIS:		return "De-Emphasis";
>  	case V4L2_CID_RDS_RECEPTION:		return "RDS Reception";
> +
> +	/* VPX controls */
> +	case V4L2_CID_VPX_CLASS:		return "VPX Encoder Controls";
> +	case V4L2_CID_VPX_NUM_PARTITIONS:	return "VPX Number of partitions";
> +	case V4L2_CID_VPX_IMD_DISABLE_4X4:	return "VPX Intra mode decision disable";
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:	return "VPX Number of reference pictures for P frames";

This string is too long: only 31 characters (excluding the trailing \0) can
be used.

> +	case V4L2_CID_VPX_FILTER_LEVEL:		return "VPX Loop filter level range";
> +	case V4L2_CID_VPX_FILTER_SHARPNESS:	return "VPX Deblocking effect control";
> +	case V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD:	return "VPX Golden frame refresh period";
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:	return "VPX Golden frame indicator";
> +
>  	default:
>  		return NULL;
>  	}
> @@ -914,6 +948,9 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_DV_RX_RGB_RANGE:
>  	case V4L2_CID_TEST_PATTERN:
>  	case V4L2_CID_TUNE_DEEMPHASIS:
> +	case V4L2_CID_VPX_NUM_PARTITIONS:
> +	case V4L2_CID_VPX_NUM_REF_FRAMES:
> +	case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
>  		*type = V4L2_CTRL_TYPE_MENU;
>  		break;
>  	case V4L2_CID_LINK_FREQ:
> @@ -937,6 +974,7 @@ void v4l2_ctrl_fill(u32 id, const char **name, enum v4l2_ctrl_type *type,
>  	case V4L2_CID_IMAGE_PROC_CLASS:
>  	case V4L2_CID_DV_CLASS:
>  	case V4L2_CID_FM_RX_CLASS:
> +	case V4L2_CID_VPX_CLASS:
>  		*type = V4L2_CTRL_TYPE_CTRL_CLASS;
>  		/* You can neither read not write these */
>  		*flags |= V4L2_CTRL_FLAG_READ_ONLY | V4L2_CTRL_FLAG_WRITE_ONLY;
> diff --git a/include/uapi/linux/v4l2-controls.h b/include/uapi/linux/v4l2-controls.h
> index 69bd5bb..3d6649c 100644
> --- a/include/uapi/linux/v4l2-controls.h
> +++ b/include/uapi/linux/v4l2-controls.h
> @@ -60,6 +60,7 @@
>  #define V4L2_CTRL_CLASS_IMAGE_PROC	0x009f0000	/* Image processing controls */
>  #define V4L2_CTRL_CLASS_DV		0x00a00000	/* Digital Video controls */
>  #define V4L2_CTRL_CLASS_FM_RX		0x00a10000	/* Digital Video controls */
> +#define V4L2_CTRL_CLASS_VPX		0x00a20000	/* VPX-compression controls */
>  
>  /* User-class control IDs */
>  
> @@ -818,7 +819,6 @@ enum v4l2_jpeg_chroma_subsampling {
>  #define V4L2_CID_PIXEL_RATE			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 2)
>  #define V4L2_CID_TEST_PATTERN			(V4L2_CID_IMAGE_PROC_CLASS_BASE + 3)
>  
> -
>  /*  DV-class control IDs defined by V4L2 */
>  #define V4L2_CID_DV_CLASS_BASE			(V4L2_CTRL_CLASS_DV | 0x900)
>  #define V4L2_CID_DV_CLASS			(V4L2_CTRL_CLASS_DV | 1)
> @@ -853,4 +853,32 @@ enum v4l2_deemphasis {
>  
>  #define V4L2_CID_RDS_RECEPTION			(V4L2_CID_FM_RX_CLASS_BASE + 2)
>  
> +/* VP-class control IDs */
> +
> +#define V4L2_CID_VPX_BASE			(V4L2_CTRL_CLASS_VPX | 0x900)
> +#define V4L2_CID_VPX_CLASS			(V4L2_CTRL_CLASS_VPX | 1)
> +
> +/*  VPX streams, specific to multiplexed streams */
> +#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
> +enum v4l2_vp8_num_partitions {
> +	V4L2_VPX_1_PARTITION	= 0,
> +	V4L2_VPX_2_PARTITIONS	= (1 << 1),
> +	V4L2_VPX_4_PARTITIONS	= (1 << 2),
> +	V4L2_VPX_8_PARTITIONS	= (1 << 3),
> +};
> +#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
> +#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
> +enum v4l2_vp8_num_ref_frames {
> +	V4L2_VPX_1_REF_FRAME	= 0,
> +	V4L2_VPX_2_REF_FRAME	= 1,
> +};
> +#define V4L2_CID_VPX_FILTER_LEVEL		(V4L2_CID_VPX_BASE+3)
> +#define V4L2_CID_VPX_FILTER_SHARPNESS		(V4L2_CID_VPX_BASE+4)
> +#define V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD	(V4L2_CID_VPX_BASE+5)
> +#define V4L2_CID_VPX_GOLDEN_FRAME_SEL		(V4L2_CID_VPX_BASE+6)
> +enum v4l2_vp8_golden_frame_sel {
> +	V4L2_VPX_GOLDEN_FRAME_USE_PREV		= 0,
> +	V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD	= 1,
> +};
> +
>  #endif
> 

Regards,

	Hans

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Sylwester Nawrocki]

Hi Arun,

On 06/10/2013 03:23 PM, Arun Kumar K wrote:
> diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
> index fccd08b..2cf17d4 100644
> --- a/drivers/media/v4l2-core/v4l2-ctrls.c
> +++ b/drivers/media/v4l2-core/v4l2-ctrls.c
> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>  		"RGB full range (0-255)",
>  		NULL,
>  	};
> +	static const char * const vpx_num_partitions[] = {
> +		"1 partition",
> +		"2 partitions",
> +		"4 partitions",
> +		"8 partitions",
> +		NULL,
> +	};
> +	static const char * const vpx_num_ref_frames[] = {
> +		"1 reference frame",
> +		"2 reference frame",
> +		NULL,
> +	};

Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
One example is V4L2_CID_ISO_SENSITIVITY control.

> +/*  VPX streams, specific to multiplexed streams */
> +#define V4L2_CID_VPX_NUM_PARTITIONS		(V4L2_CID_VPX_BASE+0)
> +enum v4l2_vp8_num_partitions {
> +	V4L2_VPX_1_PARTITION	= 0,
> +	V4L2_VPX_2_PARTITIONS	= (1 << 1),
> +	V4L2_VPX_4_PARTITIONS	= (1 << 2),
> +	V4L2_VPX_8_PARTITIONS	= (1 << 3),
> +};

I think we could still have such standard value definitions if needed,
but rather in form of:

#define V4L2_VPX_1_PARTITION	1
#define V4L2_VPX_2_PARTITIONS	2
#define V4L2_VPX_4_PARTITIONS	4
#define V4L2_VPX_8_PARTITIONS	8

> +#define V4L2_CID_VPX_IMD_DISABLE_4X4		(V4L2_CID_VPX_BASE+1)
> +#define V4L2_CID_VPX_NUM_REF_FRAMES		(V4L2_CID_VPX_BASE+2)
> +enum v4l2_vp8_num_ref_frames {
> +	V4L2_VPX_1_REF_FRAME	= 0,
> +	V4L2_VPX_2_REF_FRAME	= 1,
> +};

Regards,
Sylwester

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Hans,

Thank you for the review.

>> +
>> +    <section id="vpx-controls">
>> +      <title>VPX Control Reference</title>
>> +
>> +      <para>The VPX control class includes controls for encoding parameters
>> +      of VPx video codec.</para>
>
> Are these controls defined by the VPx standard, or are they specific to the
> Samsung hardware?

These controls are part of VP8 standard and even the reference VP8 encoder
supports these.

>
> I am not certain whether a separate class should be created for these. Adding
> it to the MPEG control class might be a better approach (yes, I know MPEG is
> a bit of a misnomer, but it already handles other compressions standards as
> well).
>

Ok. I added them as a separate control class as VP8 is not from MPEG.
I shall add it along with MPEG in the v2 version..


>> +           <row><entry></entry></row>
>> +           <row>
>> +             <entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD</constant>&nbsp;</entry>
>> +             <entry>integer</entry>
>> +           </row>
>> +           <row><entry spanname="descr">Sets the refresh period for golden frame.</entry>
>
> The period is in number of frames I assume? And is the golden frame taken at
> the start or at the end of the period?
>

Yes its in number of frames.
If we set refresh period as 5, then every 5th frame starting from 1st
frame is made as golden frame.
Will update to make it more clear.


>> +           <row><entry></entry></row>
>> +           <row id="v4l2-vpx-golden-frame-sel">
>> +             <entry spanname="id"><constant>V4L2_CID_VPX_GOLDEN_FRAME_SEL</constant>&nbsp;</entry>
>> +             <entry>enum&nbsp;v4l2_vp8_golden_frame_sel</entry>
>> +           </row>
>> +           <row><entry spanname="descr">Selects the golden frame for encoding.
>> +Possible values are:</entry>
>> +           </row>
>> +           <row>
>> +             <entrytbl spanname="descr" cols="2">
>> +               <tbody valign="top">
>> +                 <row>
>> +                   <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_PREV</constant>&nbsp;</entry>
>> +                   <entry>Use the previous second frame as a golden frame</entry>
>
> Second frame of what? It's not clear to me how I should interpret this.
>

This means the second last frame.
When user selects 2 reference frames for encoding, the last frame and
the golden frame is searched
for reference. With the V4L2_VPX_GOLDEN_FRAME_USE_PREV option, the
last to last frame is
selected as the golden frame. Will update the description for more clarity.

> +                 </row>
> +                 <row>
> +                   <entry><constant>V4L2_VPX_GOLDEN_FRAME_USE_REF_PERIOD</constant>&nbsp;</entry>
> +                   <entry>Use the previous specific frame indicated by V4L2_CID_VPX_GOLDEN_FRAME_REF_PERIOD as a golden frame</entry>
> +                 </row>
> +                  </tbody>
> +             </entrytbl>
> +           </row>

>> @@ -456,6 +456,23 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>               "RGB full range (0-255)",
>>               NULL,
>>       };
>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>
> Please use a capital P for Partition.
>

Ok.

>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>
> frame -> Frames
>
> Capitalize these strings. Same for all the others.

Ok.

>
>> +             NULL,
>> +     };
>> +     static const char * const vpx_golden_frame_sel[] = {
>> +             "Use previous frame",
>> +             "Use frame indicated by GOLDEN_FRAME_REF_PERIOD",
>> +             NULL,
>> +     };
>>
>>
>>       switch (id) {
>> @@ -545,6 +562,12 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
>>       case V4L2_CID_DV_TX_RGB_RANGE:
>>       case V4L2_CID_DV_RX_RGB_RANGE:
>>               return dv_rgb_range;
>> +     case V4L2_CID_VPX_NUM_PARTITIONS:
>> +             return vpx_num_partitions;
>> +     case V4L2_CID_VPX_NUM_REF_FRAMES:
>> +             return vpx_num_ref_frames;
>> +     case V4L2_CID_VPX_GOLDEN_FRAME_SEL:
>> +             return vpx_golden_frame_sel;
>>
>>       default:
>>               return NULL;
>> @@ -806,6 +829,17 @@ const char *v4l2_ctrl_get_name(u32 id)
>>       case V4L2_CID_FM_RX_CLASS:              return "FM Radio Receiver Controls";
>>       case V4L2_CID_TUNE_DEEMPHASIS:          return "De-Emphasis";
>>       case V4L2_CID_RDS_RECEPTION:            return "RDS Reception";
>> +
>> +     /* VPX controls */
>> +     case V4L2_CID_VPX_CLASS:                return "VPX Encoder Controls";
>> +     case V4L2_CID_VPX_NUM_PARTITIONS:       return "VPX Number of partitions";
>> +     case V4L2_CID_VPX_IMD_DISABLE_4X4:      return "VPX Intra mode decision disable";
>> +     case V4L2_CID_VPX_NUM_REF_FRAMES:       return "VPX Number of reference pictures for P frames";
>
> This string is too long: only 31 characters (excluding the trailing \0) can
> be used.

Ok will correct it.


Thanks and regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,

Thank you for the review.

>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>> +             NULL,
>> +     };
>
> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
> One example is V4L2_CID_ISO_SENSITIVITY control.

Ok will change it to V4L2_CTRL_TYPE_INTEGER_MENU.

>
>> +/*  VPX streams, specific to multiplexed streams */
>> +#define V4L2_CID_VPX_NUM_PARTITIONS          (V4L2_CID_VPX_BASE+0)
>> +enum v4l2_vp8_num_partitions {
>> +     V4L2_VPX_1_PARTITION    = 0,
>> +     V4L2_VPX_2_PARTITIONS   = (1 << 1),
>> +     V4L2_VPX_4_PARTITIONS   = (1 << 2),
>> +     V4L2_VPX_8_PARTITIONS   = (1 << 3),
>> +};
>
> I think we could still have such standard value definitions if needed,
> but rather in form of:
>
> #define V4L2_VPX_1_PARTITION    1
> #define V4L2_VPX_2_PARTITIONS   2
> #define V4L2_VPX_4_PARTITIONS   4
> #define V4L2_VPX_8_PARTITIONS   8
>

Ok will change.

Regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,

>> +     static const char * const vpx_num_partitions[] = {
>> +             "1 partition",
>> +             "2 partitions",
>> +             "4 partitions",
>> +             "8 partitions",
>> +             NULL,
>> +     };
>> +     static const char * const vpx_num_ref_frames[] = {
>> +             "1 reference frame",
>> +             "2 reference frame",
>> +             NULL,
>> +     };
>
> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
> One example is V4L2_CID_ISO_SENSITIVITY control.
>

If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
controls where
the driver / IP can support different values depending on its capabilities.
But here VP8 standard supports only 4 options for no. of partitions
that is 1, 2, 4 and 8.
Also for number of ref frames, the standard allows only the options 1,
2 and 3 which
cannot be extended more. So is it correct to use INTEGER_MENU control here and
let the driver define the values?

Regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Sylwester Nawrocki]

Hi Arun,

On 06/14/2013 11:26 AM, Arun Kumar K wrote:
> Hi Sylwester,
> 
>>> +     static const char * const vpx_num_partitions[] = {
>>> +             "1 partition",
>>> +             "2 partitions",
>>> +             "4 partitions",
>>> +             "8 partitions",
>>> +             NULL,
>>> +     };
>>> +     static const char * const vpx_num_ref_frames[] = {
>>> +             "1 reference frame",
>>> +             "2 reference frame",
>>> +             NULL,
>>> +     };
>>
>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>
> 
> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
> controls where
> the driver / IP can support different values depending on its capabilities.

No, not really, it just happens there is no INTEGER_MENU control with standard
values yet. I think there are some (minor) changes needed in the v4l2-ctrls
code to support INTEGER_MENU control with standard menu items.

> But here VP8 standard supports only 4 options for no. of partitions
> that is 1, 2, 4 and 8.

I think such a standard menu list should be defined in v4l2-ctrls.c then.

> Also for number of ref frames, the standard allows only the options 1,
> 2 and 3 which
> cannot be extended more. So is it correct to use INTEGER_MENU control here and
> let the driver define the values?

If this is standard then the core should define available menu items. But
it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
opinions though.

Regards,
Sylwester

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,

On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Hi Arun,
>
> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>> Hi Sylwester,
>>
>>>> +     static const char * const vpx_num_partitions[] = {
>>>> +             "1 partition",
>>>> +             "2 partitions",
>>>> +             "4 partitions",
>>>> +             "8 partitions",
>>>> +             NULL,
>>>> +     };
>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>> +             "1 reference frame",
>>>> +             "2 reference frame",
>>>> +             NULL,
>>>> +     };
>>>
>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>
>>
>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>> controls where
>> the driver / IP can support different values depending on its capabilities.
>
> No, not really, it just happens there is no INTEGER_MENU control with standard
> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
> code to support INTEGER_MENU control with standard menu items.
>
>> But here VP8 standard supports only 4 options for no. of partitions
>> that is 1, 2, 4 and 8.
>
> I think such a standard menu list should be defined in v4l2-ctrls.c then.

One more concern here is these integer values 1, 2, 4 and 8 may not hold
much significance while setting to the registers. Some IPs may need these
values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
values that are given by the user may not be directly applicable to the
register setting.

>
>> Also for number of ref frames, the standard allows only the options 1,
>> 2 and 3 which
>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>> let the driver define the values?
>
> If this is standard then the core should define available menu items. But
> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
> opinions though.


Here even though 1,2 and 3 are standard, the interpretation is
1 - 1 reference frame (previous frame)
2 - previous frame + golden frame
3 - previous frame + golden frame + altref frame.

Again the driver may need to set different registers based on these and the
integer values as such may not be used.


Regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Sylwester Nawrocki]

Hi Arun,

On 06/14/2013 03:21 PM, Arun Kumar K wrote:
> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>> +             "1 partition",
>>>>> +             "2 partitions",
>>>>> +             "4 partitions",
>>>>> +             "8 partitions",
>>>>> +             NULL,
>>>>> +     };
>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>> +             "1 reference frame",
>>>>> +             "2 reference frame",
>>>>> +             NULL,
>>>>> +     };
>>>>
>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>
>>>
>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>> controls where
>>> the driver / IP can support different values depending on its capabilities.
>>
>> No, not really, it just happens there is no INTEGER_MENU control with standard
>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>> code to support INTEGER_MENU control with standard menu items.
>>
>>> But here VP8 standard supports only 4 options for no. of partitions
>>> that is 1, 2, 4 and 8.
>>
>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
> 
> One more concern here is these integer values 1, 2, 4 and 8 may not hold
> much significance while setting to the registers. Some IPs may need these
> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
> values that are given by the user may not be directly applicable to the
> register setting.

The INTEGER_MENU control is not much different than MENU control from
driver POV. s_ctrl() op is called with similarly with the an index to
the menu option. In addition to standard menu applications can query
real value corresponding to a menu option, rather than a character
string only.

With each new control a nw strings are added, that cumulate in the
videodev module and make it bigger. Actually __s64 is not much smaller
than "1 partition" in your case. But it's a bit smaller. :)

That said I'm not either a codec expert or the v4l2 controls maintainer.
I think last words belongs to Hans. I just express my suggestions of
what I though we be more optimal (but not necessarily less work!). :)

>>> Also for number of ref frames, the standard allows only the options 1,
>>> 2 and 3 which
>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>> let the driver define the values?
>>
>> If this is standard then the core should define available menu items. But
>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>> opinions though.
> 
> Here even though 1,2 and 3 are standard, the interpretation is
> 1 - 1 reference frame (previous frame)
> 2 - previous frame + golden frame
> 3 - previous frame + golden frame + altref frame.

OK, then perhaps for this parameter a standard menu control would be better.
However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
You probably want to change the menu strings to reflect this more precisely,
but there might be not enough room for any creative names anyway. Maybe
something like:

static const char * const vpx_num_ref_frames[] = {
	"Previous Frame",
	"Previous + Golden Frame",
	"Prev + Golden + Altref Frame",
	NULL,
};

Anyway raw numbers might be better for the control and details could be
described in the documentation.

> Again the driver may need to set different registers based on these and the
> integer values as such may not be used.

This is really not relevant, the driver can map index of the menu to any
value that is needed by the hardware.

Regards,
Sylwester

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,

On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> Hi Arun,
>
> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>> <s.nawrocki@samsung.com> wrote:
>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>> +             "1 partition",
>>>>>> +             "2 partitions",
>>>>>> +             "4 partitions",
>>>>>> +             "8 partitions",
>>>>>> +             NULL,
>>>>>> +     };
>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>> +             "1 reference frame",
>>>>>> +             "2 reference frame",
>>>>>> +             NULL,
>>>>>> +     };
>>>>>
>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>
>>>>
>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>> controls where
>>>> the driver / IP can support different values depending on its capabilities.
>>>
>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>> code to support INTEGER_MENU control with standard menu items.
>>>
>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>> that is 1, 2, 4 and 8.
>>>
>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>
>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>> much significance while setting to the registers. Some IPs may need these
>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>> values that are given by the user may not be directly applicable to the
>> register setting.
>
> The INTEGER_MENU control is not much different than MENU control from
> driver POV. s_ctrl() op is called with similarly with the an index to
> the menu option. In addition to standard menu applications can query
> real value corresponding to a menu option, rather than a character
> string only.
>
> With each new control a nw strings are added, that cumulate in the
> videodev module and make it bigger. Actually __s64 is not much smaller
> than "1 partition" in your case. But it's a bit smaller. :)
>

Yes that makes sense. But there will be a few extra functions that
would be needed in the v4l2-ctrls.c like may be
v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
Will try to make this change.

> That said I'm not either a codec expert or the v4l2 controls maintainer.
> I think last words belongs to Hans. I just express my suggestions of
> what I though we be more optimal (but not necessarily less work!). :)
>
>>>> Also for number of ref frames, the standard allows only the options 1,
>>>> 2 and 3 which
>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>> let the driver define the values?
>>>
>>> If this is standard then the core should define available menu items. But
>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>> opinions though.
>>
>> Here even though 1,2 and 3 are standard, the interpretation is
>> 1 - 1 reference frame (previous frame)
>> 2 - previous frame + golden frame
>> 3 - previous frame + golden frame + altref frame.
>
> OK, then perhaps for this parameter a standard menu control would be better.
> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?

Thats because MFCv7 doesnt support the 3rd option yet. But still I would
add this in the control to make it generic.

> You probably want to change the menu strings to reflect this more precisely,
> but there might be not enough room for any creative names anyway. Maybe
> something like:
>
> static const char * const vpx_num_ref_frames[] = {
>         "Previous Frame",
>         "Previous + Golden Frame",
>         "Prev + Golden + Altref Frame",
>         NULL,
> };
>
> Anyway raw numbers might be better for the control and details could be
> described in the documentation.

Ok will do like this.

Regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,

>
>>>> Also for number of ref frames, the standard allows only the options 1,
>>>> 2 and 3 which
>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>> let the driver define the values?
>>>
>>> If this is standard then the core should define available menu items. But
>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>> opinions though.
>>
>> Here even though 1,2 and 3 are standard, the interpretation is
>> 1 - 1 reference frame (previous frame)
>> 2 - previous frame + golden frame
>> 3 - previous frame + golden frame + altref frame.
>
> OK, then perhaps for this parameter a standard menu control would be better.
> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
> You probably want to change the menu strings to reflect this more precisely,
> but there might be not enough room for any creative names anyway. Maybe
> something like:
>
> static const char * const vpx_num_ref_frames[] = {
>         "Previous Frame",
>         "Previous + Golden Frame",
>         "Prev + Golden + Altref Frame",
>         NULL,
> };
>

On a more detailed inspection, the standard says maximum of 3 reference
frames. So in case of 2, it can be any of the permutation combination
possible. So rather I will stick to integer menu items saying 1, 2 and 3 where
on setting value 2, the encoder can decide on which frames to refer based
on its implementation but keeping the searching limit to 2 frames only.

Regards
Arun

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Sylwester Nawrocki]

Hi Arun,

On 06/17/2013 06:25 AM, Arun Kumar K wrote:
> On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
> <s.nawrocki@samsung.com> wrote:
>> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>>> <s.nawrocki@samsung.com> wrote:
>>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>>> +             "1 partition",
>>>>>>> +             "2 partitions",
>>>>>>> +             "4 partitions",
>>>>>>> +             "8 partitions",
>>>>>>> +             NULL,
>>>>>>> +     };
>>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>>> +             "1 reference frame",
>>>>>>> +             "2 reference frame",
>>>>>>> +             NULL,
>>>>>>> +     };
>>>>>>
>>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>>
>>>>>
>>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>>> controls where
>>>>> the driver / IP can support different values depending on its capabilities.
>>>>
>>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>>> code to support INTEGER_MENU control with standard menu items.
>>>>
>>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>>> that is 1, 2, 4 and 8.
>>>>
>>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>>
>>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>>> much significance while setting to the registers. Some IPs may need these
>>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>>> values that are given by the user may not be directly applicable to the
>>> register setting.
>>
>> The INTEGER_MENU control is not much different than MENU control from
>> driver POV. s_ctrl() op is called with similarly with the an index to
>> the menu option. In addition to standard menu applications can query
>> real value corresponding to a menu option, rather than a character
>> string only.
>>
>> With each new control a nw strings are added, that cumulate in the
>> videodev module and make it bigger. Actually __s64 is not much smaller
>> than "1 partition" in your case. But it's a bit smaller. :)
> 
> Yes that makes sense. But there will be a few extra functions that
> would be needed in the v4l2-ctrls.c like may be
> v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
> Will try to make this change.

I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
could well handle such entirely standard control type. I looked into that
over the weekend, let me send you my work-in-progress patch. Maybe you find
it useful.

>> That said I'm not either a codec expert or the v4l2 controls maintainer.
>> I think last words belongs to Hans. I just express my suggestions of
>> what I though we be more optimal (but not necessarily less work!). :)
>>
>>>>> Also for number of ref frames, the standard allows only the options 1,
>>>>> 2 and 3 which
>>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>>> let the driver define the values?
>>>>
>>>> If this is standard then the core should define available menu items. But
>>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>>> opinions though.
>>>
>>> Here even though 1,2 and 3 are standard, the interpretation is
>>> 1 - 1 reference frame (previous frame)
>>> 2 - previous frame + golden frame
>>> 3 - previous frame + golden frame + altref frame.
>>
>> OK, then perhaps for this parameter a standard menu control would be better.
>> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
> 
> Thats because MFCv7 doesnt support the 3rd option yet. But still I would
> add this in the control to make it generic.

I see. Yes, I think it makes more sense to specify the control fully,
according to the standard.

>> You probably want to change the menu strings to reflect this more precisely,
>> but there might be not enough room for any creative names anyway. Maybe
>> something like:
>>
>> static const char * const vpx_num_ref_frames[] = {
>>         "Previous Frame",
>>         "Previous + Golden Frame",
>>         "Prev + Golden + Altref Frame",
>>         NULL,
>> };
>>
>> Anyway raw numbers might be better for the control and details could be
>> described in the documentation.
> 
> Ok will do like this.

Just one more note, I think I might have confused you with my comment
on the enum v4l2_vp8_num_partitions. Presumably we just need to define
contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
control. And the actual values would be defined only on the integer
menu values array, e.g.

copnst s64 qmenu_int_vpx_num_partitions[] [
	1, 2, 4, 8
};

and

#define V4L2_CID_VPX_NUM_PARTITIONS	(V4L2_CID_MPEG_BASE + ?)
enum v4l2_vp8_num_partitions {
	V4L2_VPX_1_PARTITION	= 0,
	V4L2_VPX_2_PARTITIONS	= 1,
	V4L2_VPX_4_PARTITIONS	= 2,
	V4L2_VPX_8_PARTITIONS	= 3,
};

or

#define V4L2_CID_VPX_NUM_PARTITIONS	(V4L2_CID_MPEG_BASE + ?)
#define V4L2_VPX_1_PARTITION	0
#define V4L2_VPX_2_PARTITIONS	1
#define V4L2_VPX_4_PARTITIONS	2
#define V4L2_VPX_8_PARTITIONS	3

Each driver could then map enum v4l2_vp8_num_partitions onto its
required H/W register values, without having to translate from
the MFC specific values. :)

Regards,
Sylwester

[PATCH] V4L: Add support for integer menu controls with standard menu items

[Sylwester Nawrocki]

Signed-off-by: Sylwester Nawrocki <s.nawrocki@samsung.com>
---
Hi Arun,

Perhaps you find this patch useful for your VP8 controls works.
It's not complete and I didn't test it, except ensuring it compiles.

Thanks,
Sylwester
---
 Documentation/video4linux/v4l2-controls.txt |   21 ++++++++++----------
 drivers/media/v4l2-core/v4l2-ctrls.c        |   28 ++++++++++++++++++++++++---
 2 files changed, 36 insertions(+), 13 deletions(-)

diff --git a/Documentation/video4linux/v4l2-controls.txt b/Documentation/video4linux/v4l2-controls.txt
index 676f873..fd11d0b 100644
--- a/Documentation/video4linux/v4l2-controls.txt
+++ b/Documentation/video4linux/v4l2-controls.txt
@@ -124,26 +124,27 @@ You add non-menu controls by calling v4l2_ctrl_new_std:
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 min, s32 max, u32 step, s32 def);
 
-Menu controls are added by calling v4l2_ctrl_new_std_menu:
+Menu and integer menu controls are added by calling v4l2_ctrl_new_std_menu:
 
 	struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 max, s32 skip_mask, s32 def);
 
-Or alternatively for integer menu controls, by calling v4l2_ctrl_new_int_menu:
+Menu controls with a driver specific menu are added by calling
+v4l2_ctrl_new_std_menu_items:
+
+	struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(
+			struct v4l2_ctrl_handler *hdl,
+			const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
+			s32 skip_mask, s32 def, const char * const *qmenu);
+
+Integer menu controls with driver specific menu can be added by calling
+v4l2_ctrl_new_int_menu:
 
 	struct v4l2_ctrl *v4l2_ctrl_new_int_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 max, s32 def, const s64 *qmenu_int);
 
-Standard menu controls with a driver specific menu are added by calling
-v4l2_ctrl_new_std_menu_items:
-
-	struct v4l2_ctrl *v4l2_ctrl_new_std_menu_items(
-		struct v4l2_ctrl_handler *hdl,
-		const struct v4l2_ctrl_ops *ops, u32 id, s32 max,
-		s32 skip_mask, s32 def, const char * const *qmenu);
-
 These functions are typically called right after the v4l2_ctrl_handler_init:
 
 	static const s64 exp_bias_qmenu[] = {
diff --git a/drivers/media/v4l2-core/v4l2-ctrls.c b/drivers/media/v4l2-core/v4l2-ctrls.c
index ebb8e48..c230b96 100644
--- a/drivers/media/v4l2-core/v4l2-ctrls.c
+++ b/drivers/media/v4l2-core/v4l2-ctrls.c
@@ -552,6 +552,20 @@ const char * const *v4l2_ctrl_get_menu(u32 id)
 }
 EXPORT_SYMBOL(v4l2_ctrl_get_menu);
 
+/*
+ * Returns NULL or an s64 type array containing the menu for given
+ * control ID. The total number of the menu items is returned in @len.
+ */
+const s64 const *v4l2_ctrl_get_int_menu(u32 id, u32 *len)
+{
+
+	switch (id) {
+	default:
+		return NULL;
+	}
+}
+EXPORT_SYMBOL(v4l2_ctrl_get_int_menu);
+
 /* Return the control name. */
 const char *v4l2_ctrl_get_name(u32 id)
 {
@@ -1712,20 +1726,28 @@ struct v4l2_ctrl *v4l2_ctrl_new_std_menu(struct v4l2_ctrl_handler *hdl,
 			const struct v4l2_ctrl_ops *ops,
 			u32 id, s32 max, s32 mask, s32 def)
 {
-	const char * const *qmenu = v4l2_ctrl_get_menu(id);
+	const char * const *qmenu = NULL;
+	const s64 *qmenu_int = NULL;
 	const char *name;
 	enum v4l2_ctrl_type type;
+	unsigned int qmenu_int_len;
 	s32 min;
 	s32 step;
 	u32 flags;
 
 	v4l2_ctrl_fill(id, &name, &type, &min, &max, &step, &def, &flags);
-	if (type != V4L2_CTRL_TYPE_MENU) {
+
+	if (type == V4L2_CTRL_TYPE_MENU)
+		qmenu = v4l2_ctrl_get_menu(id);
+	else if (type == V4L2_CTRL_TYPE_INTEGER_MENU)
+		qmenu_int = v4l2_ctrl_get_int_menu(id, &qmenu_int_len);
+
+	if ((!qmenu && !qmenu_int) || (qmenu_int && max > qmenu_int_len)) {
 		handler_set_err(hdl, -EINVAL);
 		return NULL;
 	}
 	return v4l2_ctrl_new(hdl, ops, id, name, type,
-			     0, max, mask, def, flags, qmenu, NULL, NULL);
+			     0, max, mask, def, flags, qmenu, qmenu_int, NULL);
 }
 EXPORT_SYMBOL(v4l2_ctrl_new_std_menu);
 
-- 
1.7.9.5

Re: [PATCH 5/6] [media] V4L: Add VP8 encoder controls

[Arun Kumar K]

Hi Sylwester,


On Mon, Jun 17, 2013 at 2:34 PM, Sylwester Nawrocki
<s.nawrocki@samsung.com> wrote:
> On 06/17/2013 06:25 AM, Arun Kumar K wrote:
>> On Sat, Jun 15, 2013 at 1:01 AM, Sylwester Nawrocki
>> <s.nawrocki@samsung.com> wrote:
>>> On 06/14/2013 03:21 PM, Arun Kumar K wrote:
>>>> On Fri, Jun 14, 2013 at 3:23 PM, Sylwester Nawrocki
>>>> <s.nawrocki@samsung.com> wrote:
>>>>> On 06/14/2013 11:26 AM, Arun Kumar K wrote:
>>>>>>>> +     static const char * const vpx_num_partitions[] = {
>>>>>>>> +             "1 partition",
>>>>>>>> +             "2 partitions",
>>>>>>>> +             "4 partitions",
>>>>>>>> +             "8 partitions",
>>>>>>>> +             NULL,
>>>>>>>> +     };
>>>>>>>> +     static const char * const vpx_num_ref_frames[] = {
>>>>>>>> +             "1 reference frame",
>>>>>>>> +             "2 reference frame",
>>>>>>>> +             NULL,
>>>>>>>> +     };
>>>>>>>
>>>>>>> Have you considered using V4L2_CTRL_TYPE_INTEGER_MENU control type for this ?
>>>>>>> One example is V4L2_CID_ISO_SENSITIVITY control.
>>>>>>>
>>>>>>
>>>>>> If I understand correctly, V4L2_CTRL_TYPE_INTEGER_MENU is used for
>>>>>> controls where
>>>>>> the driver / IP can support different values depending on its capabilities.
>>>>>
>>>>> No, not really, it just happens there is no INTEGER_MENU control with standard
>>>>> values yet. I think there are some (minor) changes needed in the v4l2-ctrls
>>>>> code to support INTEGER_MENU control with standard menu items.
>>>>>
>>>>>> But here VP8 standard supports only 4 options for no. of partitions
>>>>>> that is 1, 2, 4 and 8.
>>>>>
>>>>> I think such a standard menu list should be defined in v4l2-ctrls.c then.
>>>>
>>>> One more concern here is these integer values 1, 2, 4 and 8 may not hold
>>>> much significance while setting to the registers. Some IPs may need these
>>>> values to be set as 0, 1, 2 and 3. So unlike other settings like ISO, the
>>>> values that are given by the user may not be directly applicable to the
>>>> register setting.
>>>
>>> The INTEGER_MENU control is not much different than MENU control from
>>> driver POV. s_ctrl() op is called with similarly with the an index to
>>> the menu option. In addition to standard menu applications can query
>>> real value corresponding to a menu option, rather than a character
>>> string only.
>>>
>>> With each new control a nw strings are added, that cumulate in the
>>> videodev module and make it bigger. Actually __s64 is not much smaller
>>> than "1 partition" in your case. But it's a bit smaller. :)
>>
>> Yes that makes sense. But there will be a few extra functions that
>> would be needed in the v4l2-ctrls.c like may be
>> v4l2_ctrl_new_int_menu_fixed() which doesnt take qmenu arg from driver.
>> Will try to make this change.
>
> I wouldn't be adding another helper like this, IMHO v4l2_ctrl_new_menu()
> could well handle such entirely standard control type. I looked into that
> over the weekend, let me send you my work-in-progress patch. Maybe you find
> it useful.


Ok that would be really helpful. Will check that patch.

>
>>> That said I'm not either a codec expert or the v4l2 controls maintainer.
>>> I think last words belongs to Hans. I just express my suggestions of
>>> what I though we be more optimal (but not necessarily less work!). :)
>>>
>>>>>> Also for number of ref frames, the standard allows only the options 1,
>>>>>> 2 and 3 which
>>>>>> cannot be extended more. So is it correct to use INTEGER_MENU control here and
>>>>>> let the driver define the values?
>>>>>
>>>>> If this is standard then the core should define available menu items. But
>>>>> it seems more appropriate for me to use INTEGER_MENU. I'd like to hear other
>>>>> opinions though.
>>>>
>>>> Here even though 1,2 and 3 are standard, the interpretation is
>>>> 1 - 1 reference frame (previous frame)
>>>> 2 - previous frame + golden frame
>>>> 3 - previous frame + golden frame + altref frame.
>>>
>>> OK, then perhaps for this parameter a standard menu control would be better.
>>> However, why there are only 2 options in vpx_num_ref_frames[] arrays ?
>>
>> Thats because MFCv7 doesnt support the 3rd option yet. But still I would
>> add this in the control to make it generic.
>
> I see. Yes, I think it makes more sense to specify the control fully,
> according to the standard.
>
>>> You probably want to change the menu strings to reflect this more precisely,
>>> but there might be not enough room for any creative names anyway. Maybe
>>> something like:
>>>
>>> static const char * const vpx_num_ref_frames[] = {
>>>         "Previous Frame",
>>>         "Previous + Golden Frame",
>>>         "Prev + Golden + Altref Frame",
>>>         NULL,
>>> };
>>>
>>> Anyway raw numbers might be better for the control and details could be
>>> described in the documentation.
>>
>> Ok will do like this.
>
> Just one more note, I think I might have confused you with my comment
> on the enum v4l2_vp8_num_partitions. Presumably we just need to define
> contiguous enumeration for all options of V4L2_CID_VPX_NUM_PARTITIONS
> control. And the actual values would be defined only on the integer
> menu values array, e.g.
>
> copnst s64 qmenu_int_vpx_num_partitions[] [
>         1, 2, 4, 8
> };
>
> and
>
> #define V4L2_CID_VPX_NUM_PARTITIONS     (V4L2_CID_MPEG_BASE + ?)
> enum v4l2_vp8_num_partitions {
>         V4L2_VPX_1_PARTITION    = 0,
>         V4L2_VPX_2_PARTITIONS   = 1,
>         V4L2_VPX_4_PARTITIONS   = 2,
>         V4L2_VPX_8_PARTITIONS   = 3,
> };
>
> or
>
> #define V4L2_CID_VPX_NUM_PARTITIONS     (V4L2_CID_MPEG_BASE + ?)
> #define V4L2_VPX_1_PARTITION    0
> #define V4L2_VPX_2_PARTITIONS   1
> #define V4L2_VPX_4_PARTITIONS   2
> #define V4L2_VPX_8_PARTITIONS   3
>
> Each driver could then map enum v4l2_vp8_num_partitions onto its
> required H/W register values, without having to translate from
> the MFC specific values. :)


Ok got it now. Thanks for the wonderful explanation :)

Regards
Arun

RE: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Kamil Debski]

Hi Arun,

I have read your patches. They seem alright, I back comments made by Hans
and Sylwester. I have one question, which follows inline.

Best wishes,
-- 
Kamil Debski
Linux Kernel Developer
Samsung R&D Institute Poland

> -----Original Message-----
> From: Arun Kumar K [mailto:arun.kk@samsung.com]
> Sent: Monday, June 10, 2013 3:23 PM
> To: linux-media@vger.kernel.org
> Cc: k.debski@samsung.com; jtp.park@samsung.com; s.nawrocki@samsung.com;
> avnd.kiran@samsung.com; arunkk.samsung@gmail.com
> Subject: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7
> 
> Adds variant data and core support for the MFC v7 firmware
> 
> Signed-off-by: Arun Kumar K <arun.kk@samsung.com>
> ---
>  drivers/media/platform/s5p-mfc/s5p_mfc.c        |   32
> +++++++++++++++++++++++
>  drivers/media/platform/s5p-mfc/s5p_mfc_common.h |    2 ++
>  2 files changed, 34 insertions(+)
> 
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> index d12faa6..d6be52f 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc.c
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc.c
> @@ -1391,6 +1391,32 @@ static struct s5p_mfc_variant mfc_drvdata_v6 = {
>  	.fw_name        = "s5p-mfc-v6.fw",
>  };
> 
> +struct s5p_mfc_buf_size_v6 mfc_buf_size_v7 = {
> +	.dev_ctx	= MFC_CTX_BUF_SIZE_V7,
> +	.h264_dec_ctx	= MFC_H264_DEC_CTX_BUF_SIZE_V7,
> +	.other_dec_ctx	= MFC_OTHER_DEC_CTX_BUF_SIZE_V7,
> +	.h264_enc_ctx	= MFC_H264_ENC_CTX_BUF_SIZE_V7,
> +	.other_enc_ctx	= MFC_OTHER_ENC_CTX_BUF_SIZE_V7,
> +};
> +
> +struct s5p_mfc_buf_size buf_size_v7 = {
> +	.fw	= MAX_FW_SIZE_V7,
> +	.cpb	= MAX_CPB_SIZE_V7,
> +	.priv	= &mfc_buf_size_v7,
> +};
> +
> +struct s5p_mfc_buf_align mfc_buf_align_v7 = {
> +	.base = 0,
> +};
> +
> +static struct s5p_mfc_variant mfc_drvdata_v7 = {
> +	.version	= MFC_VERSION_V7,
> +	.port_num	= MFC_NUM_PORTS_V7,
> +	.buf_size	= &buf_size_v7,
> +	.buf_align	= &mfc_buf_align_v7,
> +	.fw_name        = "s5p-mfc-v7.fw",
> +};
> +
>  static struct platform_device_id mfc_driver_ids[] = {
>  	{
>  		.name = "s5p-mfc",
> @@ -1401,6 +1427,9 @@ static struct platform_device_id mfc_driver_ids[]
> = {
>  	}, {
>  		.name = "s5p-mfc-v6",
>  		.driver_data = (unsigned long)&mfc_drvdata_v6,
> +	}, {
> +		.name = "s5p-mfc-v7",
> +		.driver_data = (unsigned long)&mfc_drvdata_v7,
>  	},
>  	{},
>  };
> @@ -1413,6 +1442,9 @@ static const struct of_device_id
> exynos_mfc_match[] = {
>  	}, {
>  		.compatible = "samsung,mfc-v6",
>  		.data = &mfc_drvdata_v6,
> +	}, {
> +		.compatible = "samsung,mfc-v7",
> +		.data = &mfc_drvdata_v7,
>  	},
>  	{},
>  };
> diff --git a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> index ef4074c..7281de2 100644
> --- a/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> +++ b/drivers/media/platform/s5p-mfc/s5p_mfc_common.h
> @@ -24,6 +24,7 @@
>  #include <media/videobuf2-core.h>
>  #include "regs-mfc.h"
>  #include "regs-mfc-v6.h"
> +#include "regs-mfc-v7.h"
> 
>  /* Definitions related to MFC memory */
> 
> @@ -684,5 +685,6 @@ void set_work_bit_irqsave(struct s5p_mfc_ctx *ctx);
>  				(dev->variant->port_num ? 1 : 0) : 0) : 0)
>  #define IS_TWOPORT(dev)		(dev->variant->port_num == 2 ? 1 :
0)
>  #define IS_MFCV6(dev)		(dev->variant->version >= 0x60 ? 1 :
0)
> +#define IS_MFCV7(dev)		(dev->variant->version >= 0x70 ? 1 :
0)

According to this, MFC v7 is also detected as MFC v6. Was this intended?
I think that it would be much better to use this in code:
	if (IS_MFCV6(dev) || IS_MFCV7(dev))
And change the define to detect only single MFC revision:
	#define IS_MFCV6(dev)		(dev->variant->version >= 0x60 &&
dev->variant->version < 0x70)

Other possibility I see is to change the name of the check. Although
IS_MFCV6_OR_NEWER(dev) seems too long :)

> 
>  #endif /* S5P_MFC_COMMON_H_ */
> --
> 1.7.9.5

Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Arun Kumar K]

Hi Kamil,

Thank you for the review.


>>  #define IS_MFCV6(dev)                (dev->variant->version >= 0x60 ? 1 :
> 0)
>> +#define IS_MFCV7(dev)                (dev->variant->version >= 0x70 ? 1 :
> 0)
>
> According to this, MFC v7 is also detected as MFC v6. Was this intended?

Yes this was intentional as most of v7 will be reusing the v6 code and
only minor
changes are there w.r.t firmware interface.


> I think that it would be much better to use this in code:
>         if (IS_MFCV6(dev) || IS_MFCV7(dev))
> And change the define to detect only single MFC revision:
>         #define IS_MFCV6(dev)           (dev->variant->version >= 0x60 &&
> dev->variant->version < 0x70)
>

I kept it like that since the macro IS_MFCV6() is used quite frequently
in the driver. Also if MFCv8 comes which is again similar to v6 (not
sure about this),
then it will add another OR condition to this check.

> Other possibility I see is to change the name of the check. Although
> IS_MFCV6_OR_NEWER(dev) seems too long :)
>

How about making it IS_MFCV6_PLUS()?

Regards
Arun

Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Sachin Kamat]

On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
> Hi Kamil,
>
> Thank you for the review.
>
>
>>>  #define IS_MFCV6(dev)                (dev->variant->version >= 0x60 ? 1 :
>> 0)
>>> +#define IS_MFCV7(dev)                (dev->variant->version >= 0x70 ? 1 :
>> 0)
>>
>> According to this, MFC v7 is also detected as MFC v6. Was this intended?
>
> Yes this was intentional as most of v7 will be reusing the v6 code and
> only minor
> changes are there w.r.t firmware interface.
>
>
>> I think that it would be much better to use this in code:
>>         if (IS_MFCV6(dev) || IS_MFCV7(dev))
>> And change the define to detect only single MFC revision:
>>         #define IS_MFCV6(dev)           (dev->variant->version >= 0x60 &&
>> dev->variant->version < 0x70)
>>
>
> I kept it like that since the macro IS_MFCV6() is used quite frequently
> in the driver. Also if MFCv8 comes which is again similar to v6 (not
> sure about this),
> then it will add another OR condition to this check.
>
>> Other possibility I see is to change the name of the check. Although
>> IS_MFCV6_OR_NEWER(dev) seems too long :)
>>
>
> How about making it IS_MFCV6_PLUS()?

Technically
#define IS_MFCV6(dev)                (dev->variant->version >= 0x60...)
means all lower versions are also higher versions.
This may not cause much of a problem (other than the macro being a
misnomer) as all current higher versions are supersets of lower
versions.
But this is not guaranteed(?).

Hence changing the definition of the macro to (dev->variant->version
>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or
renaming it to
IS_MFCV6_PLUS() makes sense.

OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc?

If not we can make it just:
#define IS_MFCV6(dev)                (dev->variant->version == 0x60 ? 1 : 0)


-- 
With warm regards,
Sachin

Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Arun Kumar K]

Hi Sachin,


On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
>> Hi Kamil,
>>
>> Thank you for the review.
>>
>>
>>>>  #define IS_MFCV6(dev)                (dev->variant->version >= 0x60 ? 1 :
>>> 0)
>>>> +#define IS_MFCV7(dev)                (dev->variant->version >= 0x70 ? 1 :
>>> 0)
>>>
>>> According to this, MFC v7 is also detected as MFC v6. Was this intended?
>>
>> Yes this was intentional as most of v7 will be reusing the v6 code and
>> only minor
>> changes are there w.r.t firmware interface.
>>
>>
>>> I think that it would be much better to use this in code:
>>>         if (IS_MFCV6(dev) || IS_MFCV7(dev))
>>> And change the define to detect only single MFC revision:
>>>         #define IS_MFCV6(dev)           (dev->variant->version >= 0x60 &&
>>> dev->variant->version < 0x70)
>>>
>>
>> I kept it like that since the macro IS_MFCV6() is used quite frequently
>> in the driver. Also if MFCv8 comes which is again similar to v6 (not
>> sure about this),
>> then it will add another OR condition to this check.
>>
>>> Other possibility I see is to change the name of the check. Although
>>> IS_MFCV6_OR_NEWER(dev) seems too long :)
>>>
>>
>> How about making it IS_MFCV6_PLUS()?
>
> Technically
> #define IS_MFCV6(dev)                (dev->variant->version >= 0x60...)
> means all lower versions are also higher versions.
> This may not cause much of a problem (other than the macro being a
> misnomer) as all current higher versions are supersets of lower
> versions.
> But this is not guaranteed(?).
>

Till now we havent encountered otherwise and we can only hope that
it remains like this :)


> Hence changing the definition of the macro to (dev->variant->version
>>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or
> renaming it to
> IS_MFCV6_PLUS() makes sense.
>
> OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc?
>
> If not we can make it just:
> #define IS_MFCV6(dev)                (dev->variant->version == 0x60 ? 1 : 0)
>

The v6 version we use is actually v6.5 and v7 is v7.2.
In mainline we havent used any FW sub-versions yet.

Regards
Arun

Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Sachin Kamat]

Hi Arun,

On 18 June 2013 11:25, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
> Hi Sachin,
>
>
> On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
>>> Hi Kamil,
>>>
>>> Thank you for the review.
>>>
>>>
>>>>>  #define IS_MFCV6(dev)                (dev->variant->version >= 0x60 ? 1 :
>>>> 0)
>>>>> +#define IS_MFCV7(dev)                (dev->variant->version >= 0x70 ? 1 :
>>>> 0)
>>>>
>>>> According to this, MFC v7 is also detected as MFC v6. Was this intended?
>>>
>>> Yes this was intentional as most of v7 will be reusing the v6 code and
>>> only minor
>>> changes are there w.r.t firmware interface.
>>>
>>>
>>>> I think that it would be much better to use this in code:
>>>>         if (IS_MFCV6(dev) || IS_MFCV7(dev))
>>>> And change the define to detect only single MFC revision:
>>>>         #define IS_MFCV6(dev)           (dev->variant->version >= 0x60 &&
>>>> dev->variant->version < 0x70)
>>>>
>>>
>>> I kept it like that since the macro IS_MFCV6() is used quite frequently
>>> in the driver. Also if MFCv8 comes which is again similar to v6 (not
>>> sure about this),
>>> then it will add another OR condition to this check.
>>>
>>>> Other possibility I see is to change the name of the check. Although
>>>> IS_MFCV6_OR_NEWER(dev) seems too long :)
>>>>
>>>
>>> How about making it IS_MFCV6_PLUS()?
>>
>> Technically
>> #define IS_MFCV6(dev)                (dev->variant->version >= 0x60...)
>> means all lower versions are also higher versions.
>> This may not cause much of a problem (other than the macro being a
>> misnomer) as all current higher versions are supersets of lower
>> versions.
>> But this is not guaranteed(?).
>>
>
> Till now we havent encountered otherwise and we can only hope that
> it remains like this :)
>
>
>> Hence changing the definition of the macro to (dev->variant->version
>>>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or
>> renaming it to
>> IS_MFCV6_PLUS() makes sense.
>>
>> OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc?
>>
>> If not we can make it just:
>> #define IS_MFCV6(dev)                (dev->variant->version == 0x60 ? 1 : 0)
>>
>
> The v6 version we use is actually v6.5 and v7 is v7.2.
> In mainline we havent used any FW sub-versions yet.

OK. Do they co-exist or is there a possibility for that (to have v6.5
and say v6.7 or v7.2 and v7.4, etc). Just asking.


-- 
With warm regards,
Sachin

Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Arun Kumar K]

Hi Sachin,

On Tue, Jun 18, 2013 at 11:42 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
> Hi Arun,
>
> On 18 June 2013 11:25, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
>> Hi Sachin,
>>
>>
>> On Tue, Jun 18, 2013 at 10:56 AM, Sachin Kamat <sachin.kamat@linaro.org> wrote:
>>> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
>>>> Hi Kamil,
>>>>
>>>> Thank you for the review.
>>>>
>>>>
>>>>>>  #define IS_MFCV6(dev)                (dev->variant->version >= 0x60 ? 1 :
>>>>> 0)
>>>>>> +#define IS_MFCV7(dev)                (dev->variant->version >= 0x70 ? 1 :
>>>>> 0)
>>>>>
>>>>> According to this, MFC v7 is also detected as MFC v6. Was this intended?
>>>>
>>>> Yes this was intentional as most of v7 will be reusing the v6 code and
>>>> only minor
>>>> changes are there w.r.t firmware interface.
>>>>
>>>>
>>>>> I think that it would be much better to use this in code:
>>>>>         if (IS_MFCV6(dev) || IS_MFCV7(dev))
>>>>> And change the define to detect only single MFC revision:
>>>>>         #define IS_MFCV6(dev)           (dev->variant->version >= 0x60 &&
>>>>> dev->variant->version < 0x70)
>>>>>
>>>>
>>>> I kept it like that since the macro IS_MFCV6() is used quite frequently
>>>> in the driver. Also if MFCv8 comes which is again similar to v6 (not
>>>> sure about this),
>>>> then it will add another OR condition to this check.
>>>>
>>>>> Other possibility I see is to change the name of the check. Although
>>>>> IS_MFCV6_OR_NEWER(dev) seems too long :)
>>>>>
>>>>
>>>> How about making it IS_MFCV6_PLUS()?
>>>
>>> Technically
>>> #define IS_MFCV6(dev)                (dev->variant->version >= 0x60...)
>>> means all lower versions are also higher versions.
>>> This may not cause much of a problem (other than the macro being a
>>> misnomer) as all current higher versions are supersets of lower
>>> versions.
>>> But this is not guaranteed(?).
>>>
>>
>> Till now we havent encountered otherwise and we can only hope that
>> it remains like this :)
>>
>>
>>> Hence changing the definition of the macro to (dev->variant->version
>>>>= 0x60 && dev->variant->version < 0x70) as Kamil suggested or
>>> renaming it to
>>> IS_MFCV6_PLUS() makes sense.
>>>
>>> OTOH, do we really have intermediate version numbers? For e.g. 0x61, 0x72, etc?
>>>
>>> If not we can make it just:
>>> #define IS_MFCV6(dev)                (dev->variant->version == 0x60 ? 1 : 0)
>>>
>>
>> The v6 version we use is actually v6.5 and v7 is v7.2.
>> In mainline we havent used any FW sub-versions yet.
>
> OK. Do they co-exist or is there a possibility for that (to have v6.5
> and say v6.7 or v7.2 and v7.4, etc). Just asking.
>
>

For these sub-versions, the driver interface remains mostly the same
and only internal firmware implementations change (atleast that's what
I have seen till date). For mainline purpose, we choose one of the versions
and stick to that.

Regards
Arun

RE: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7

[Kamil Debski]

Hi Arun, Sachin,

> -----Original Message-----
> From: Sachin Kamat [mailto:sachin.kamat@linaro.org]
> Sent: Tuesday, June 18, 2013 7:27 AM
> To: Arun Kumar K
> Cc: Kamil Debski; Arun Kumar K; LMML; jtp.park@samsung.com; Sylwester
> Nawrocki; avnd.kiran@samsung.com
> Subject: Re: [PATCH 3/6] [media] s5p-mfc: Core support for MFC v7
> 
> On 18 June 2013 10:21, Arun Kumar K <arunkk.samsung@gmail.com> wrote:
> > Hi Kamil,
> >
> > Thank you for the review.
> >
> >
> >>>  #define IS_MFCV6(dev)                (dev->variant->version >=
> 0x60 ? 1 :
> >> 0)
> >>> +#define IS_MFCV7(dev)                (dev->variant->version >=
> 0x70 ? 1 :
> >> 0)
> >>
> >> According to this, MFC v7 is also detected as MFC v6. Was this
> intended?
> >
> > Yes this was intentional as most of v7 will be reusing the v6 code
> and
> > only minor changes are there w.r.t firmware interface.
> >
> >
> >> I think that it would be much better to use this in code:
> >>         if (IS_MFCV6(dev) || IS_MFCV7(dev)) And change the define to
> >> detect only single MFC revision:
> >>         #define IS_MFCV6(dev)           (dev->variant->version >=
> 0x60 &&
> >> dev->variant->version < 0x70)
> >>
> >
> > I kept it like that since the macro IS_MFCV6() is used quite
> > frequently in the driver. Also if MFCv8 comes which is again similar
> > to v6 (not sure about this), then it will add another OR condition to
> > this check.
> >
> >> Other possibility I see is to change the name of the check. Although
> >> IS_MFCV6_OR_NEWER(dev) seems too long :)
> >>
> >
> > How about making it IS_MFCV6_PLUS()?
> 
> Technically
> #define IS_MFCV6(dev)                (dev->variant->version >= 0x60...)
> means all lower versions are also higher versions.
> This may not cause much of a problem (other than the macro being a
> misnomer) as all current higher versions are supersets of lower
> versions.
> But this is not guaranteed(?).

MFC versions 5+ have very much in common. However there are two previous
MFC versions - 4 (s5pc100?) and 1 (s3c6410). These versions are much 
different if I remember correctly. Drivers for these version are not
present in mainline, but I know that there are community members that
provide support and keep adding new drivers for older SoCs. Maybe
some day they will be added.

> 
> Hence changing the definition of the macro to (dev->variant->version
> >= 0x60 && dev->variant->version < 0x70) as Kamil suggested or
> renaming it to
> IS_MFCV6_PLUS() makes sense.

I agree, this name will be easier to understand.

> 
> OTOH, do we really have intermediate version numbers? For e.g. 0x61,
> 0x72, etc?
> 
> If not we can make it just:
> #define IS_MFCV6(dev)                (dev->variant->version == 0x60 ?
> 1 : 0)
> 
> 

Best wishes,
-- 
Kamil Debski
Linux Kernel Developer
Samsung R&D Institute Poland