Hi Tony,
On 01/07/2015 01:08 PM, Tony K Nadackal wrote:
Dear Jacek,
On Wednesday, January 07, 2015 3:15 PM Jacek Anaszewski wrote,
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.
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.
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.
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.
+ 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.
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.
--
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
