s5p-jpeg: Adding Exynos7 Jpeg variant support

PATCH New Tony K Nadackal 2 participants 7 messages Dec 17, 2014 – Jan 12, 2015

Subsystems (3)

Device tree bindings Media (V4L/DVB) Arm Samsung Exynos

lore

Share & tools

lore

Tony K Nadackal

author

2014-12-17 07:27 UTC #0 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Fimp_jpeg used in Exynos7 is a revised version. Some register configurations are slightly different from Jpeg in Exynos4. Added one more variant SJPEG_EXYNOS7 to handle these differences.

Signed-off-by Tony K Nadackal <[email protected]>

---
This patch is created and tested on top of linux-next-20141210. It can be cleanly applied on media-next and kgene/for-next.

 .../bindings/media/exynos-jpeg-codec.txt           |  2 +-
 drivers/media/platform/s5p-jpeg/jpeg-core.c        | 69 +++++++++++++++++++---
 drivers/media/platform/s5p-jpeg/jpeg-core.h        |  1 +
 drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c  | 33 ++++++-----
 drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h  |  8 ++-
 drivers/media/platform/s5p-jpeg/jpeg-regs.h        | 17 ++++--
 6 files changed, 98 insertions(+), 32 deletions(-)

Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt +1 -1

index bf52ed4..cd19417 100644
@@ -4,7 +4,7 @@ Required properties:
 
 - compatible	: should be one of:
 		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
-		  "samsung,exynos3250-jpeg";
+		  "samsung,exynos3250-jpeg", "samsung,exynos7-jpeg";
 - reg		: address and length of the JPEG codec IP register set;
 - interrupts	: specifies the JPEG codec IP interrupt;
 - clock-names   : should contain:

drivers/media/platform/s5p-jpeg/jpeg-core.c +60 -9

114 lines not on this page · raw message

drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0

index 764b32d..734710a 100644
@@ -71,6 +71,7 @@
 #define SJPEG_S5P		1
 #define SJPEG_EXYNOS3250	2
 #define SJPEG_EXYNOS4		3
+#define SJPEG_EXYNOS7		4
 
 enum exynos4_jpeg_result {
 	OK_ENC_OR_DEC,

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c +19 ~~-14~~

index e53f13a..2611259 100644
@@ -49,7 +49,8 @@ void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode)
 	}
 }
 
-void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
+void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt,
+					unsigned int shift)
 {
 	unsigned int reg;

@@ -71,48 +72,48 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
 	case V4L2_PIX_FMT_NV24:
 		reg = reg | EXYNOS4_ENC_YUV_444_IMG |
 				EXYNOS4_YUV_444_IP_YUV_444_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
 		break;
 	case V4L2_PIX_FMT_NV42:
 		reg = reg | EXYNOS4_ENC_YUV_444_IMG |
 				EXYNOS4_YUV_444_IP_YUV_444_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
 		break;
 	case V4L2_PIX_FMT_YUYV:
 		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
 				EXYNOS4_YUV_422_IP_YUV_422_1P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
 		break;
 
 	case V4L2_PIX_FMT_YVYU:
 		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
 				EXYNOS4_YUV_422_IP_YUV_422_1P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
 		break;
 	case V4L2_PIX_FMT_NV16:
 		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
 				EXYNOS4_YUV_422_IP_YUV_422_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
 		break;
 	case V4L2_PIX_FMT_NV61:
 		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
 				EXYNOS4_YUV_422_IP_YUV_422_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
 		break;
 	case V4L2_PIX_FMT_NV12:
 		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
 				EXYNOS4_YUV_420_IP_YUV_420_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
 		break;
 	case V4L2_PIX_FMT_NV21:
 		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
 				EXYNOS4_YUV_420_IP_YUV_420_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
 		break;
 	case V4L2_PIX_FMT_YUV420:
 		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
 				EXYNOS4_YUV_420_IP_YUV_420_3P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
 		break;
 	default:
 		break;

@@ -122,12 +123,13 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
 	writel(reg, base + EXYNOS4_IMG_FMT_REG);
 }
 
-void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt)
+void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt,
+					unsigned int mask)
 {
 	unsigned int reg;
 
 	reg = readl(base + EXYNOS4_IMG_FMT_REG) &
-			~EXYNOS4_ENC_FMT_MASK; /* clear enc format */
+			~EXYNOS_ENC_FMT_MASK(mask); /* clear enc format */
 
 	switch (out_fmt) {
 	case V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY:

@@ -153,9 +155,12 @@ void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt)
 	writel(reg, base + EXYNOS4_IMG_FMT_REG);
 }
 
-void exynos4_jpeg_set_interrupt(void __iomem *base)
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int version)
 {
-	writel(EXYNOS4_INT_EN_ALL, base + EXYNOS4_INT_EN_REG);
+	unsigned int reg;
+
+	reg = readl(base + EXYNOS4_INT_EN_REG) & ~EXYNOS4_INT_EN_MASK(version);
+	writel(EXYNOS4_INT_EN_ALL(version), base + EXYNOS4_INT_EN_REG);
 }
 
 unsigned int exynos4_jpeg_get_int_status(void __iomem *base)

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h +5 -3

index c228d28..b425199 100644
@@ -15,10 +15,12 @@
 
 void exynos4_jpeg_sw_reset(void __iomem *base);
 void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode);
-void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt);
-void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt);
+void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt,
+					unsigned int shift);
+void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt,
+							unsigned int mask);
 void exynos4_jpeg_set_enc_tbl(void __iomem *base);
-void exynos4_jpeg_set_interrupt(void __iomem *base);
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int variant);
 unsigned int exynos4_jpeg_get_int_status(void __iomem *base);
 void exynos4_jpeg_set_huf_table_enable(void __iomem *base, int value);
 void exynos4_jpeg_set_sys_int_enable(void __iomem *base, int value);

drivers/media/platform/s5p-jpeg/jpeg-regs.h +12 -5

index 050fc44..08bef4e 100644
@@ -230,13 +230,15 @@
 #define EXYNOS4_SOFT_RESET_HI		(1 << 29)
 
 /* JPEG INT Register bit */
-#define EXYNOS4_INT_EN_MASK		(0x1f << 0)
+#define EXYNOS4_INT_EN_MASK(version)	(((version) == SJPEG_EXYNOS7) \
+						? (0x1ff << 0) : (0x1f << 0))
 #define EXYNOS4_PROT_ERR_INT_EN		(1 << 0)
 #define EXYNOS4_IMG_COMPLETION_INT_EN	(1 << 1)
 #define EXYNOS4_DEC_INVALID_FORMAT_EN	(1 << 2)
 #define EXYNOS4_MULTI_SCAN_ERROR_EN	(1 << 3)
 #define EXYNOS4_FRAME_ERR_EN		(1 << 4)
-#define EXYNOS4_INT_EN_ALL		(0x1f << 0)
+#define EXYNOS4_INT_EN_ALL(version)    (((version) == SJPEG_EXYNOS7) \
+						? (0x1b6 << 0) : (0x1f << 0))
 
 #define EXYNOS4_MOD_REG_PROC_ENC	(0 << 3)
 #define EXYNOS4_MOD_REG_PROC_DEC	(1 << 3)

@@ -294,8 +296,11 @@
 #define EXYNOS4_YUV_420_IP_YUV_420_2P_IMG	(4 << EXYNOS4_YUV_420_IP_SHIFT)
 #define EXYNOS4_YUV_420_IP_YUV_420_3P_IMG	(5 << EXYNOS4_YUV_420_IP_SHIFT)
 
+#define EXYNOS4_ENC_FMT_MASK			3
+#define EXYNOS7_ENC_FMT_MASK			7
 #define EXYNOS4_ENC_FMT_SHIFT			24
-#define EXYNOS4_ENC_FMT_MASK			(3 << EXYNOS4_ENC_FMT_SHIFT)
+#define EXYNOS_ENC_FMT_MASK(mask)		((mask) \
+						<< EXYNOS4_ENC_FMT_SHIFT)
 #define EXYNOS4_ENC_FMT_GRAY			(0 << EXYNOS4_ENC_FMT_SHIFT)
 #define EXYNOS4_ENC_FMT_YUV_444			(1 << EXYNOS4_ENC_FMT_SHIFT)
 #define EXYNOS4_ENC_FMT_YUV_422			(2 << EXYNOS4_ENC_FMT_SHIFT)

@@ -303,8 +308,10 @@
 
 #define EXYNOS4_JPEG_DECODED_IMG_FMT_MASK	0x03
 
-#define EXYNOS4_SWAP_CHROMA_CRCB		(1 << 26)
-#define EXYNOS4_SWAP_CHROMA_CBCR		(0 << 26)
+#define EXYNOS7_SWAP_CHROMA_SHIFT		27
+#define EXYNOS4_SWAP_CHROMA_SHIFT		26
+#define EXYNOS_SWAP_CHROMA_CRCB(shift)		(1 << (shift))
+#define EXYNOS_SWAP_CHROMA_CBCR(shift)		(0 << (shift))
 
 /* JPEG HUFF count Register bit */
 #define EXYNOS4_HUFF_COUNT_MASK			0xffff

Signature · 1 line

2.2.0

Jacek Anaszewski

2014-12-17 14:15 UTC #1 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Hi Tony,

Thanks for the patches.

Please process them with scripts/checkpatch.pl as you will be submitting the next version - they contain many coding style related issues.

My remaining comments below.

Quoted 76 lines · Tony K Nadackal

Fimp_jpeg used in Exynos7 is a revised version. Some register configurations are slightly different from Jpeg in Exynos4. Added one more variant SJPEG_EXYNOS7 to handle these differences.

Signed-off-by Tony K Nadackal <[email protected]>

---
This patch is created and tested on top of linux-next-20141210. It can be cleanly applied on media-next and kgene/for-next.

  .../bindings/media/exynos-jpeg-codec.txt           |  2 +-
  drivers/media/platform/s5p-jpeg/jpeg-core.c        | 69 +++++++++++++++++++---
  drivers/media/platform/s5p-jpeg/jpeg-core.h        |  1 +
  drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c  | 33 ++++++-----
  drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h  |  8 ++-
  drivers/media/platform/s5p-jpeg/jpeg-regs.h        | 17 ++++--
  6 files changed, 98 insertions(+), 32 deletions(-)

Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt +1 -1

index bf52ed4..cd19417 100644
@@ -4,7 +4,7 @@ Required properties:
 
  - compatible	: should be one of:
  		  "samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
-		  "samsung,exynos3250-jpeg";
+		  "samsung,exynos3250-jpeg", "samsung,exynos7-jpeg";
  - reg		: address and length of the JPEG codec IP register set;
  - interrupts	: specifies the JPEG codec IP interrupt;
  - clock-names   : should contain:

drivers/media/platform/s5p-jpeg/jpeg-core.c +11 -6

index 54fa5d9..ad42a4e 100644
@@ -1225,8 +1225,9 @@ static int s5p_jpeg_try_fmt_vid_cap(struct file *file, void *priv,
  		return -EINVAL;
  	}
 
-	if ((ctx->jpeg->variant->version != SJPEG_EXYNOS4) ||
-	    (ctx->mode != S5P_JPEG_DECODE))
+	if (((ctx->jpeg->variant->version != SJPEG_EXYNOS4) &&
+		(ctx->jpeg->variant->version != SJPEG_EXYNOS7)) ||
+		(ctx->mode != S5P_JPEG_DECODE))
  		goto exit;
 
  	/*

@@ -1349,7 +1350,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct v4l2_format *f)
  		 * the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
  		 * page fault calculate proper buffer size in such a case.
  		 */
-		if (ct->jpeg->variant->version == SJPEG_EXYNOS4 &&
+		if (((ct->jpeg->variant->version == SJPEG_EXYNOS4) ||
+			(ct->jpeg->variant->version == SJPEG_EXYNOS7)) &&
  		    f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
  			q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
  							f,

@@ -1901,7 +1903,6 @@ static void exynos4_jpeg_device_run(void *priv)
 
  	if (ctx->mode == S5P_JPEG_ENCODE) {
  		exynos4_jpeg_sw_reset(jpeg->regs);
-		exynos4_jpeg_set_interrupt(jpeg->regs);
  		exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
 
  		exynos4_jpeg_set_huff_tbl(jpeg->regs);

@@ -1918,20 +1919,60 @@ static void exynos4_jpeg_device_run(void *priv)
  		exynos4_jpeg_set_stream_size(jpeg->regs, ctx->cap_q.w,
  							ctx->cap_q.h);
 
-		exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS7);
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling, EXYNOS7_ENC_FMT_MASK);
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937

Quoted 10 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +10 -0

+					EXYNOS7_SWAP_CHROMA_SHIFT);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS4);
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling, EXYNOS4_ENC_FMT_MASK);
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+		}
+

I'd implement it this way:

exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling,

			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);

Quoted 15 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +3 -3

  		exynos4_jpeg_set_img_addr(ctx);
  		exynos4_jpeg_set_jpeg_addr(ctx);
  		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,
  							ctx->out_q.fmt->fourcc);
  	} else {
  		exynos4_jpeg_sw_reset(jpeg->regs);
-		exynos4_jpeg_set_interrupt(jpeg->regs);
  		exynos4_jpeg_set_img_addr(ctx);
  		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt->fourcc);
 
-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS7);
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1959

Quoted 10 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +10 -0

+			exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for chroma
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx->compr_quality);
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx->compr_quality);

Is it really required to setup quantization tables for encoding?

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962

Quoted 2 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx->cap_q.w,
+					ctx->cap_q.h);

For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of Exynos7?

Quoted 45 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +23 -0

+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling, EXYNOS7_ENC_FMT_MASK);
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->cap_q.fmt->fourcc,
+					EXYNOS7_SWAP_CHROMA_SHIFT);
+			bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 16);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS4);
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->cap_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+			bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		}
 
  		exynos4_jpeg_set_dec_bitstream_size(jpeg->regs, bitstream_size);
  	}

@@ -2729,6 +2770,13 @@ static struct s5p_jpeg_variant exynos4_jpeg_drvdata = {
  	.fmt_ver_flag	= SJPEG_FMT_FLAG_EXYNOS4,
  };
 
+static struct s5p_jpeg_variant exynos7_jpeg_drvdata = {
+	.version	= SJPEG_EXYNOS7,
+	.jpeg_irq	= exynos4_jpeg_irq,
+	.m2m_ops	= &exynos4_jpeg_m2m_ops,
+	.fmt_ver_flag	= SJPEG_FMT_FLAG_EXYNOS4,
+};
+
  static const struct of_device_id samsung_jpeg_match[] = {
  	{
  		.compatible = "samsung,s5pv210-jpeg",

@@ -2742,6 +2790,9 @@ static const struct of_device_id samsung_jpeg_match[] = {
  	}, {
  		.compatible = "samsung,exynos4212-jpeg",
  		.data = &exynos4_jpeg_drvdata,
+	}, {
+		.compatible = "samsung,exynos7-jpeg",
+		.data = &exynos7_jpeg_drvdata,
  	},
  	{},
  };

drivers/media/platform/s5p-jpeg/jpeg-core.h +0 -0

index 764b32d..734710a 100644

drivers/media/platform/s5p-jpeg/jpeg-core.h line 74

Quoted 4 lines

drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0

@@ -71,6 +71,7 @@
  #define SJPEG_S5P		1
  #define SJPEG_EXYNOS3250	2
  #define SJPEG_EXYNOS4		3
+#define SJPEG_EXYNOS7		4

As you adding a new variant I propose to turn these macros into enum.

Quoted 93 lines

drivers/media/platform/s5p-jpeg/jpeg-core.h +0 -0

  enum exynos4_jpeg_result {
  	OK_ENC_OR_DEC,

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c +14 ~~-12~~

index e53f13a..2611259 100644
@@ -49,7 +49,8 @@ void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode)
  	}
  }
 
-void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
+void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt,
+					unsigned int shift)
  {
  	unsigned int reg;

@@ -71,48 +72,48 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
  	case V4L2_PIX_FMT_NV24:
  		reg = reg | EXYNOS4_ENC_YUV_444_IMG |
  				EXYNOS4_YUV_444_IP_YUV_444_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
  		break;
  	case V4L2_PIX_FMT_NV42:
  		reg = reg | EXYNOS4_ENC_YUV_444_IMG |
  				EXYNOS4_YUV_444_IP_YUV_444_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
  		break;
  	case V4L2_PIX_FMT_YUYV:
  		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
  				EXYNOS4_YUV_422_IP_YUV_422_1P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
  		break;
 
  	case V4L2_PIX_FMT_YVYU:
  		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
  				EXYNOS4_YUV_422_IP_YUV_422_1P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
  		break;
  	case V4L2_PIX_FMT_NV16:
  		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
  				EXYNOS4_YUV_422_IP_YUV_422_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
  		break;
  	case V4L2_PIX_FMT_NV61:
  		reg = reg | EXYNOS4_DEC_YUV_422_IMG |
  				EXYNOS4_YUV_422_IP_YUV_422_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
  		break;
  	case V4L2_PIX_FMT_NV12:
  		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
  				EXYNOS4_YUV_420_IP_YUV_420_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
  		break;
  	case V4L2_PIX_FMT_NV21:
  		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
  				EXYNOS4_YUV_420_IP_YUV_420_2P_IMG |
-				EXYNOS4_SWAP_CHROMA_CRCB;
+				EXYNOS_SWAP_CHROMA_CRCB(shift);
  		break;
  	case V4L2_PIX_FMT_YUV420:
  		reg = reg | EXYNOS4_DEC_YUV_420_IMG |
  				EXYNOS4_YUV_420_IP_YUV_420_3P_IMG |
-				EXYNOS4_SWAP_CHROMA_CBCR;
+				EXYNOS_SWAP_CHROMA_CBCR(shift);
  		break;
  	default:
  		break;

@@ -122,12 +123,13 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt)
  	writel(reg, base + EXYNOS4_IMG_FMT_REG);
  }
 
-void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt)
+void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt,
+					unsigned int mask)
  {
  	unsigned int reg;
 
  	reg = readl(base + EXYNOS4_IMG_FMT_REG) &
-			~EXYNOS4_ENC_FMT_MASK; /* clear enc format */
+			~EXYNOS_ENC_FMT_MASK(mask); /* clear enc format */
 
  	switch (out_fmt) {
  	case V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY:

@@ -153,9 +155,12 @@ void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt)
  	writel(reg, base + EXYNOS4_IMG_FMT_REG);
  }

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt) · line 164

Quoted 10 lines

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c +5 -2

 
-void exynos4_jpeg_set_interrupt(void __iomem *base)
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int version)
  {
-	writel(EXYNOS4_INT_EN_ALL, base + EXYNOS4_INT_EN_REG);
+	unsigned int reg;
+
+	reg = readl(base + EXYNOS4_INT_EN_REG) & ~EXYNOS4_INT_EN_MASK(version);
+	writel(EXYNOS4_INT_EN_ALL(version), base + EXYNOS4_INT_EN_REG);
  }

I believe that adding readl is a fix. I'd enclose it into separate patch and explain its merit.

Quoted 70 lines

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c +0 -0

  unsigned int exynos4_jpeg_get_int_status(void __iomem *base)

drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h +5 -3

index c228d28..b425199 100644
@@ -15,10 +15,12 @@
 
  void exynos4_jpeg_sw_reset(void __iomem *base);
  void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode);
-void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt);
-void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt);
+void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt,
+					unsigned int shift);
+void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt,
+							unsigned int mask);
  void exynos4_jpeg_set_enc_tbl(void __iomem *base);
-void exynos4_jpeg_set_interrupt(void __iomem *base);
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int variant);
  unsigned int exynos4_jpeg_get_int_status(void __iomem *base);
  void exynos4_jpeg_set_huf_table_enable(void __iomem *base, int value);
  void exynos4_jpeg_set_sys_int_enable(void __iomem *base, int value);

drivers/media/platform/s5p-jpeg/jpeg-regs.h +12 -5

index 050fc44..08bef4e 100644
@@ -230,13 +230,15 @@
  #define EXYNOS4_SOFT_RESET_HI		(1 << 29)
 
  /* JPEG INT Register bit */
-#define EXYNOS4_INT_EN_MASK		(0x1f << 0)
+#define EXYNOS4_INT_EN_MASK(version)	(((version) == SJPEG_EXYNOS7) \
+						? (0x1ff << 0) : (0x1f << 0))
  #define EXYNOS4_PROT_ERR_INT_EN		(1 << 0)
  #define EXYNOS4_IMG_COMPLETION_INT_EN	(1 << 1)
  #define EXYNOS4_DEC_INVALID_FORMAT_EN	(1 << 2)
  #define EXYNOS4_MULTI_SCAN_ERROR_EN	(1 << 3)
  #define EXYNOS4_FRAME_ERR_EN		(1 << 4)
-#define EXYNOS4_INT_EN_ALL		(0x1f << 0)
+#define EXYNOS4_INT_EN_ALL(version)    (((version) == SJPEG_EXYNOS7) \
+						? (0x1b6 << 0) : (0x1f << 0))
 
  #define EXYNOS4_MOD_REG_PROC_ENC	(0 << 3)
  #define EXYNOS4_MOD_REG_PROC_DEC	(1 << 3)

@@ -294,8 +296,11 @@
  #define EXYNOS4_YUV_420_IP_YUV_420_2P_IMG	(4 << EXYNOS4_YUV_420_IP_SHIFT)
  #define EXYNOS4_YUV_420_IP_YUV_420_3P_IMG	(5 << EXYNOS4_YUV_420_IP_SHIFT)
 
+#define EXYNOS4_ENC_FMT_MASK			3
+#define EXYNOS7_ENC_FMT_MASK			7
  #define EXYNOS4_ENC_FMT_SHIFT			24
-#define EXYNOS4_ENC_FMT_MASK			(3 << EXYNOS4_ENC_FMT_SHIFT)
+#define EXYNOS_ENC_FMT_MASK(mask)		((mask) \
+						<< EXYNOS4_ENC_FMT_SHIFT)
  #define EXYNOS4_ENC_FMT_GRAY			(0 << EXYNOS4_ENC_FMT_SHIFT)
  #define EXYNOS4_ENC_FMT_YUV_444			(1 << EXYNOS4_ENC_FMT_SHIFT)
  #define EXYNOS4_ENC_FMT_YUV_422			(2 << EXYNOS4_ENC_FMT_SHIFT)

@@ -303,8 +308,10 @@
 
  #define EXYNOS4_JPEG_DECODED_IMG_FMT_MASK	0x03
 
-#define EXYNOS4_SWAP_CHROMA_CRCB		(1 << 26)
-#define EXYNOS4_SWAP_CHROMA_CBCR		(0 << 26)
+#define EXYNOS7_SWAP_CHROMA_SHIFT		27
+#define EXYNOS4_SWAP_CHROMA_SHIFT		26
+#define EXYNOS_SWAP_CHROMA_CRCB(shift)		(1 << (shift))
+#define EXYNOS_SWAP_CHROMA_CBCR(shift)		(0 << (shift))
 
  /* JPEG HUFF count Register bit */
  #define EXYNOS4_HUFF_COUNT_MASK			0xffff

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Tony K Nadackal

author

2014-12-19 03:36 UTC #2 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Hi Jacek,

On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,

Quoted 4 lines

Hi Tony,
Thanks for the patches.

Thanks for the review.

Quoted 1 line

Please process them with scripts/checkpatch.pl as you will be submitting the

Quoted 2 lines

version - they contain many coding style related issues.

I ran checkpatch before posting. Do you find any checkpatch related issues in the patch?

Quoted 2 lines

My remaining comments below.

[snip]

Quoted 25 lines

Review
Quoted 2 lines
(quoted diff) +2 -0
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS7_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS7_SWAP_CHROMA_SHIFT);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS4);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS4_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+		}
+
I'd implement it this way:
exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling,

Quoted 8 lines

			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);

OK. Looks goods to me. Thanks for the suggestion.

Quoted 16 lines

Quoted 12 lines

  		exynos4_jpeg_set_img_addr(ctx);
  		exynos4_jpeg_set_jpeg_addr(ctx);
  		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,
  							ctx->out_q.fmt->fourcc);
  	} else {
  		exynos4_jpeg_sw_reset(jpeg->regs);

drivers/media/platform/s5p-jpeg/jpeg-core.c +0 -2

-		exynos4_jpeg_set_interrupt(jpeg->regs);
  		exynos4_jpeg_set_img_addr(ctx);
  		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt-

fourcc);

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)

Quoted 3 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -1

-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,

SJPEG_EXYNOS7);

Quoted 14 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1959
Quoted 11 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +11 -0
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);
+			exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for chroma
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx->compr_quality);
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx->compr_quality);
Is it really required to setup quantization tables for encoding?

Without setting up the quantization tables, encoder is working fine. But, as per Exynos7 User Manual setting up the quantization tables are required for encoding also.

Quoted 8 lines

drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962
Quoted 3 lines
(quoted diff) +1 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx-
cap_q.w,
drivers/media/platform/s5p-jpeg/jpeg-core.c +1 -0
+					ctx->cap_q.h);
For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of Exynos7?

Yes. In case of Exynos7 Encoding, This step is required.

[snip]

Quoted 10 lines

drivers/media/platform/s5p-jpeg/jpeg-core.h line 74
Quoted 7 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
@@ -71,6 +71,7 @@
  #define SJPEG_S5P		1
  #define SJPEG_EXYNOS3250	2
  #define SJPEG_EXYNOS4		3
+#define SJPEG_EXYNOS7		4
As you adding a new variant I propose to turn these macros into enum.

Ok. I will make this change in my next version.

[snip]

Quoted 9 lines

drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +5 -2
-void exynos4_jpeg_set_interrupt(void __iomem *base)
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int
+version)
  {
-	writel(EXYNOS4_INT_EN_ALL, base + EXYNOS4_INT_EN_REG);
+	unsigned int reg;
+
+	reg = readl(base + EXYNOS4_INT_EN_REG) &
~EXYNOS4_INT_EN_MASK(version);

Quoted 4 lines

drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 2 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
+	writel(EXYNOS4_INT_EN_ALL(version), base + EXYNOS4_INT_EN_REG);
  }
I believe that adding readl is a fix. I'd enclose it into separate patch and

explain its

Quoted 2 lines

merit.

Thanks for the suggestion. I will make a separate patch for this bug fix.

[snip]

Quoted 4 lines

--
Best Regards,
Jacek Anaszewski

Thanks,
Tony K Nadackal

Jacek Anaszewski

2015-01-07 09:45 UTC #3 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Hi Tony,

Sorry for late response, just got back from vacation.

Quoted 10 lines · Tony K Nadackal

Hi Jacek,
On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,
Quoted 4 lines
Hi Tony,
Thanks for the patches.
Thanks for the review.

Quoted 7 lines

Quoted 1 line
Please process them with scripts/checkpatch.pl as you will be submitting the
next
Quoted 2 lines
version - they contain many coding style related issues.
I ran checkpatch before posting. Do you find any checkpatch related issues in the patch?

There was a problem on my side, sorry for making confusion.

Quoted 72 lines

Quoted 2 lines
My remaining comments below.
[snip]
Quoted 25 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS7_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS7_SWAP_CHROMA_SHIFT);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS4);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS4_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+		}
+
I'd implement it this way:
exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling,
Quoted 8 lines
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);
OK. Looks goods to me. Thanks for the suggestion.
Quoted 16 lines
Quoted 12 lines
   		exynos4_jpeg_set_img_addr(ctx);
   		exynos4_jpeg_set_jpeg_addr(ctx);
   		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,
   							ctx->out_q.fmt->fourcc);
   	} else {
   		exynos4_jpeg_sw_reset(jpeg->regs);
drivers/media/platform/s5p-jpeg/jpeg-core.c +0 -2
-		exynos4_jpeg_set_interrupt(jpeg->regs);
   		exynos4_jpeg_set_img_addr(ctx);
   		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt-
fourcc);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 3 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -1
-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Quoted 14 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1959
Quoted 11 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +11 -0
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);
+			exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for chroma
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx->compr_quality);
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx->compr_quality);
Is it really required to setup quantization tables for encoding?

Quoted 3 lines

Without setting up the quantization tables, encoder is working fine. But, as per Exynos7 User Manual setting up the quantization tables are required for encoding also.

Actually I intended to ask if setting the quantization tables is required for *decoding*, as you set it also in decoding path, whereas for Exynos4 it is not required. I looks strange for me as quantization tables are usually required only for encoding raw images. The same is related to huffman tables.

Quoted 10 lines

Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962
Quoted 3 lines
(quoted diff) +1 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx-
cap_q.w,
drivers/media/platform/s5p-jpeg/jpeg-core.c +1 -0
+					ctx->cap_q.h);
For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of Exynos7?
Yes. In case of Exynos7 Encoding, This step is required.

Ack.

Quoted 13 lines

[snip]
Quoted 10 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h line 74
Quoted 7 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
@@ -71,6 +71,7 @@
   #define SJPEG_S5P		1
   #define SJPEG_EXYNOS3250	2
   #define SJPEG_EXYNOS4		3
+#define SJPEG_EXYNOS7		4
As you adding a new variant I propose to turn these macros into enum.

Quoted 1 line

Ok. I will make this change in my next version.

Thanks.

Quoted 15 lines

[snip]
Quoted 9 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +5 -2
-void exynos4_jpeg_set_interrupt(void __iomem *base)
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int
+version)
   {
-	writel(EXYNOS4_INT_EN_ALL, base + EXYNOS4_INT_EN_REG);
+	unsigned int reg;
+
+	reg = readl(base + EXYNOS4_INT_EN_REG) &
~EXYNOS4_INT_EN_MASK(version);
Quoted 4 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 2 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
+	writel(EXYNOS4_INT_EN_ALL(version), base + EXYNOS4_INT_EN_REG);
   }
I believe that adding readl is a fix. I'd enclose it into separate patch and

Quoted 5 lines

explain its
Quoted 2 lines
merit.
Thanks for the suggestion. I will make a separate patch for this bug fix.

Great.

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Tony K Nadackal

author

2015-01-07 12:08 UTC #4 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Dear Jacek,

On Wednesday, January 07, 2015 3:15 PM Jacek Anaszewski wrote,

Quoted 17 lines

Hi Tony,
Sorry for late response, just got back from vacation.
Quoted 12 lines · Tony K Nadackal
Hi Jacek,
On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,
Quoted 4 lines
Hi Tony,
Thanks for the patches.
Thanks for the review.
Quoted 2 lines
Please process them with scripts/checkpatch.pl as you will be submitting the

Quoted 54 lines

Quoted 6 lines
next
Quoted 2 lines
version - they contain many coding style related issues.
I ran checkpatch before posting. Do you find any checkpatch related issues in the patch?
There was a problem on my side, sorry for making confusion.
Quoted 45 lines
Quoted 2 lines
My remaining comments below.
[snip]
Quoted 25 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS7_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS7_SWAP_CHROMA_SHIFT);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS4);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS4_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+		}
+
I'd implement it this way:
exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling,
Quoted 8 lines
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);
OK. Looks goods to me. Thanks for the suggestion.
Quoted 4 lines
Quoted 4 lines
   		exynos4_jpeg_set_img_addr(ctx);
   		exynos4_jpeg_set_jpeg_addr(ctx);
   		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,

ctx->out_q.fmt->fourcc);

Quoted 27 lines

Quoted 27 lines
Quoted 26 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 11 lines
   	} else {
   		exynos4_jpeg_sw_reset(jpeg->regs);
drivers/media/platform/s5p-jpeg/jpeg-core.c +0 -2
-		exynos4_jpeg_set_interrupt(jpeg->regs);
   		exynos4_jpeg_set_img_addr(ctx);
   		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt-
fourcc);
drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -1
-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1959
Quoted 11 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +11 -0
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);
+			exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for chroma
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx->compr_quality);
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx->compr_quality);
Is it really required to setup quantization tables for encoding?

Quoted 4 lines

Quoted 3 lines
Without setting up the quantization tables, encoder is working fine. But, as per Exynos7 User Manual setting up the quantization tables are required for encoding also.

Sorry I also got it mixed up.
*Decoder* works fine without setting up the quantization tables. But this step is mentioned in User Manual.

Quoted 3 lines

Actually I intended to ask if setting the quantization tables is required for *decoding*, as you set it also in decoding path, whereas for Exynos4 it is not required. I looks strange for me as quantization tables are usually required

only

Quoted 2 lines

for encoding raw images.
The same is related to huffman tables.

Huffman table is required for Exynos7 decoding.
User Manual says about Update_Huf_Tbl [bit 19 of PEG_CNTL], "User/Host should mandatory program this Bit as "1" for every decoder operation. SFR "HUFF_TBL_ENT" and SFR "HUFF_CNT" should be programmed accordingly for every encoder/decoder operation."

Quoted 8 lines

Quoted 8 lines
Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962
Quoted 4 lines
(quoted diff) +1 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx-
cap_q.w,
drivers/media/platform/s5p-jpeg/jpeg-core.c +1 -0
+					ctx->cap_q.h);
For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of

Exynos7?

Quoted 5 lines

Quoted 3 lines
Quoted 1 line
Yes. In case of Exynos7 Encoding, This step is required.
Ack.

I will request Kukjin or any Samsung colleagues who has access to Exynos7 Manual to give ack or tested by.

Quoted 38 lines

Quoted 14 lines
[snip]
Quoted 10 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h line 74
Quoted 7 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
@@ -71,6 +71,7 @@
   #define SJPEG_S5P		1
   #define SJPEG_EXYNOS3250	2
   #define SJPEG_EXYNOS4		3
+#define SJPEG_EXYNOS7		4
As you adding a new variant I propose to turn these macros into enum.
Quoted 1 line
Ok. I will make this change in my next version.
Thanks.
Quoted 20 lines
[snip]
Quoted 9 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +5 -2
-void exynos4_jpeg_set_interrupt(void __iomem *base)
+void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int
+version)
   {
-	writel(EXYNOS4_INT_EN_ALL, base + EXYNOS4_INT_EN_REG);
+	unsigned int reg;
+
+	reg = readl(base + EXYNOS4_INT_EN_REG) &
~EXYNOS4_INT_EN_MASK(version);
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h
Quoted 2 lines
drivers/media/platform/s5p-jpeg/jpeg-core.h +1 -0
+	writel(EXYNOS4_INT_EN_ALL(version), base + EXYNOS4_INT_EN_REG);
   }
I believe that adding readl is a fix. I'd enclose it into separate patch and
explain its
Quoted 2 lines
merit.

Quoted 7 lines

Quoted 1 line
Thanks for the suggestion. I will make a separate patch for this bug fix.
Great.
--
Best Regards,
Jacek Anaszewski

Thanks and Regards,
Tony

Jacek Anaszewski

2015-01-07 12:39 UTC #5 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Hi Tony,

Quoted 103 lines · Tony K Nadackal

Dear Jacek,
On Wednesday, January 07, 2015 3:15 PM Jacek Anaszewski wrote,
Quoted 17 lines
Hi Tony,
Sorry for late response, just got back from vacation.
Quoted 12 lines · Tony K Nadackal
Hi Jacek,
On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,
Quoted 4 lines
Hi Tony,
Thanks for the patches.
Thanks for the review.
Quoted 2 lines
Please process them with scripts/checkpatch.pl as you will be submitting the
Quoted 54 lines
Quoted 6 lines
next
Quoted 2 lines
version - they contain many coding style related issues.
I ran checkpatch before posting. Do you find any checkpatch related issues in the patch?
There was a problem on my side, sorry for making confusion.
Quoted 45 lines
Quoted 2 lines
My remaining comments below.
[snip]
Quoted 25 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS7_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS7_SWAP_CHROMA_SHIFT);
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS4);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS4_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 5 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +5 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+					EXYNOS4_SWAP_CHROMA_SHIFT);
+		}
+
I'd implement it this way:
exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling,
Quoted 8 lines
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);
OK. Looks goods to me. Thanks for the suggestion.
Quoted 4 lines
Quoted 4 lines
    		exynos4_jpeg_set_img_addr(ctx);
    		exynos4_jpeg_set_jpeg_addr(ctx);
    		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,
ctx->out_q.fmt->fourcc);
Quoted 27 lines
Quoted 27 lines
Quoted 26 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 11 lines
    	} else {
    		exynos4_jpeg_sw_reset(jpeg->regs);
drivers/media/platform/s5p-jpeg/jpeg-core.c +0 -2
-		exynos4_jpeg_set_interrupt(jpeg->regs);
    		exynos4_jpeg_set_img_addr(ctx);
    		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt-
fourcc);
drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -1
-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1959
Quoted 11 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +11 -0
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);
+			exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for chroma
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx->compr_quality);
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx->compr_quality);
Is it really required to setup quantization tables for encoding?

Quoted 8 lines

Quoted 4 lines
Quoted 3 lines
Without setting up the quantization tables, encoder is working fine. But, as per Exynos7 User Manual setting up the quantization tables are required for encoding also.
Sorry I also got it mixed up.
*Decoder* works fine without setting up the quantization tables. But this step is mentioned in User Manual.

I'm ok with it provided that you will get an ack from Samsung SOCs maintainer.

Quoted 4 lines

Quoted 3 lines
Actually I intended to ask if setting the quantization tables is required for *decoding*, as you set it also in decoding path, whereas for Exynos4 it is not required. I looks strange for me as quantization tables are usually required
only

Quoted 8 lines

Quoted 2 lines
for encoding raw images.
The same is related to huffman tables.
Huffman table is required for Exynos7 decoding.
User Manual says about Update_Huf_Tbl [bit 19 of PEG_CNTL], "User/Host should mandatory program this Bit as "1" for every decoder operation. SFR "HUFF_TBL_ENT" and SFR "HUFF_CNT" should be programmed accordingly for every encoder/decoder operation."

Same situation as above.

Quoted 11 lines

Quoted 8 lines
Quoted 8 lines
Quoted 8 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962
Quoted 4 lines
(quoted diff) +1 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx-
cap_q.w,
drivers/media/platform/s5p-jpeg/jpeg-core.c +1 -0
+					ctx->cap_q.h);
For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of
Exynos7?
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

Quoted 6 lines

Quoted 3 lines
Quoted 1 line
Yes. In case of Exynos7 Encoding, This step is required.
Ack.
I will request Kukjin or any Samsung colleagues who has access to Exynos7 Manual to give ack or tested by.

This is a good idea.

Signature · 2 lines

Best Regards,
Jacek Anaszewski

Tony K Nadackal

author

2015-01-12 11:26 UTC #6 lore raw

PATCH 1/1 s5p-jpeg: Adding Exynos7 Jpeg variant support

Hi Kukjin,

On Wednesday, January 07, 2015 6:09 PM, Jacek Anaszewski wrote,

Quoted 94 lines

Hi Tony,
Quoted 38 lines · Tony K Nadackal
Dear Jacek,
On Wednesday, January 07, 2015 3:15 PM Jacek Anaszewski wrote,
Quoted 34 lines
Hi Tony,
Sorry for late response, just got back from vacation.
Quoted 10 lines · Tony K Nadackal
Hi Jacek,
On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,
Quoted 4 lines
Hi Tony,
Thanks for the patches.
Thanks for the review.
Quoted 8 lines
Quoted 2 lines
Please process them with scripts/checkpatch.pl as you will be submitting the
next
Quoted 2 lines
version - they contain many coding style related issues.
I ran checkpatch before posting. Do you find any checkpatch related issues in the patch?
There was a problem on my side, sorry for making confusion.
Quoted 8 lines
Quoted 2 lines
My remaining comments below.
[snip]
Quoted 3 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Quoted 6 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS7_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 3 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +3 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+
	EXYNOS7_SWAP_CHROMA_SHIFT);
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		} else {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS4);
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
+					ctx->subsampling,
EXYNOS4_ENC_FMT_MASK);
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1937
Quoted 3 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +3 -0
+			exynos4_jpeg_set_img_fmt(jpeg->regs,
+					ctx->out_q.fmt->fourcc,
+
	EXYNOS4_SWAP_CHROMA_SHIFT);
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Quoted 7 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+		}
+
I'd implement it this way:
exynos4_jpeg_set_interrupt(jpeg->regs, ctx->jpeg->variant->version); exynos4_jpeg_set_enc_out_fmt(jpeg->regs,
ctx->subsampling,
Quoted 28 lines
Quoted 9 lines
Quoted 9 lines
Quoted 8 lines
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_ENC_FMT_MASK :
				EXYNOS7_ENC_FMT_MASK);
exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc,
			(ctx->jpeg->variant->version == SJPEG_EXYNOS4) ?
				EXYNOS4_SWAP_CHROMA_SHIFT :
				EXYNOS7_SWAP_CHROMA_SHIFT);
Quoted 6 lines
Quoted 6 lines
OK. Looks goods to me. Thanks for the suggestion.
Quoted 4 lines
Quoted 4 lines
    		exynos4_jpeg_set_img_addr(ctx);
    		exynos4_jpeg_set_jpeg_addr(ctx);
    		exynos4_jpeg_set_encode_hoff_cnt(jpeg->regs,
ctx->out_q.fmt->fourcc);
Quoted 12 lines
Quoted 12 lines
Quoted 12 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 11 lines
    	} else {
    		exynos4_jpeg_sw_reset(jpeg->regs);
drivers/media/platform/s5p-jpeg/jpeg-core.c +0 -2
-		exynos4_jpeg_set_interrupt(jpeg->regs);
    		exynos4_jpeg_set_img_addr(ctx);
    		exynos4_jpeg_set_jpeg_addr(ctx);
-		exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->cap_q.fmt-
fourcc);
drivers/media/platform/s5p-jpeg/jpeg-core.c +2 -1
-		bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32);
+		if (ctx->jpeg->variant->version == SJPEG_EXYNOS7) {
+			exynos4_jpeg_set_interrupt(jpeg->regs,
SJPEG_EXYNOS7);

Quoted 7 lines

Quoted 7 lines
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_huff_tbl(jpeg->regs);
+			exynos4_jpeg_set_huf_table_enable(jpeg-
regs, 1);
Quoted 4 lines
Quoted 4 lines
Quoted 4 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 4 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +4 -0
+
+			/*
+			 * JPEG IP allows storing 4 quantization tables
+			 * We fill table 0 for luma and table 1 for

chroma

Quoted 50 lines

Quoted 11 lines
Quoted 3 lines
Quoted 3 lines
Quoted 3 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv)
Quoted 3 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c +3 -0
+			 */
+			exynos4_jpeg_set_qtbl_lum(jpeg->regs,
+							ctx-
compr_quality);
Quoted 2 lines
Quoted 2 lines
Quoted 2 lines
Review
Quoted 2 lines
(quoted diff) +2 -0
+			exynos4_jpeg_set_qtbl_chr(jpeg->regs,
+							ctx-
compr_quality);
Quoted 4 lines
Quoted 4 lines
Quoted 3 lines
Is it really required to setup quantization tables for encoding?
Quoted 8 lines
Quoted 4 lines
Quoted 3 lines
Without setting up the quantization tables, encoder is working fine. But, as per Exynos7 User Manual setting up the quantization tables are required for encoding also.
Sorry I also got it mixed up.
*Decoder* works fine without setting up the quantization tables. But this step is mentioned in User Manual.
I'm ok with it provided that you will get an ack from Samsung SOCs maintainer.
Quoted 5 lines
Quoted 4 lines
Actually I intended to ask if setting the quantization tables is required for *decoding*, as you set it also in decoding path, whereas for Exynos4 it is not required. I looks strange for me as quantization tables are usually required
only
Quoted 8 lines
Quoted 2 lines
for encoding raw images.
The same is related to huffman tables.
Huffman table is required for Exynos7 decoding.
User Manual says about Update_Huf_Tbl [bit 19 of PEG_CNTL], "User/Host should mandatory program this Bit as "1" for every decoder operation. SFR "HUFF_TBL_ENT" and SFR "HUFF_CNT" should be programmed accordingly for every encoder/decoder operation."
Same situation as above.
Quoted 12 lines
Quoted 9 lines
Quoted 9 lines
Quoted 9 lines
drivers/media/platform/s5p-jpeg/jpeg-core.c static void exynos4_jpeg_device_run(void *priv) · line 1962
Quoted 4 lines
(quoted diff) +1 -0
+			exynos4_jpeg_set_stream_size(jpeg->regs, ctx-
cap_q.w,
drivers/media/platform/s5p-jpeg/jpeg-core.c +1 -0
+					ctx->cap_q.h);
For exynos4 this function writes the number of samples per line and number lines of the resulting JPEG image and is used only during encoding. Is the semantics of the related register different in case of
Exynos7?
Quoted 2 lines
Quoted 2 lines
Quoted 1 line

Quoted 8 lines

Quoted 6 lines
Quoted 3 lines
Quoted 1 line
Yes. In case of Exynos7 Encoding, This step is required.
Ack.
I will request Kukjin or any Samsung colleagues who has access to Exynos7 Manual to give ack or tested by.
This is a good idea.

Will you please take care of this..?

Quoted 4 lines

--
Best Regards,
Jacek Anaszewski

Thanks and Regards
Tony