Hi Tony,
Sorry for late response, just got back from vacation.
On 12/19/2014 04:37 AM, Tony K Nadackal wrote:
Hi Jacek,
On Wednesday, December 17, 2014 7:46 PM Jacek Anaszewski wrote,
Hi Tony,
Thanks for the patches.
Thanks for the review.
Please process them with scripts/checkpatch.pl as you will be submitting the
next
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.
My remaining comments below.
[snip]
+ 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,
+ 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);
OK. Looks goods to me. Thanks for the suggestion.
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);
+ 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.
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.
+ 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?
Yes. In case of Exynos7 Encoding, This step is required.
Ack.
[snip]
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.h
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.h
@@ -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.
Thanks.
[snip]
-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.
Thanks for the suggestion. I will make a separate patch for this bug fix.
Great.
--
Best Regards,
Jacek Anaszewski
--
To unsubscribe from this list: send the line "unsubscribe devicetree" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html
