RE: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support

[Tony K Nadackal <tony.kn@samsung.com>]

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.

> 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."

> 
> >>> +			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.

> 
> > [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

Thanks and Regards,
Tony