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

[Jacek Anaszewski <j.anaszewski-Sze3O3UU22JBDgjK7y7TUQ@public.gmane.org>]

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-u79uwXL29TY76Z2rM5mHXA@public.gmane.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html