From: j.anaszewski@samsung•com (Jacek Anaszewski)
To: linux-arm-kernel@lists•infradead.org
Subject: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
Date: Wed, 07 Jan 2015 10:45:23 +0100 [thread overview]
Message-ID: <54AD0033.1020103@samsung.com> (raw)
In-Reply-To: <001801d01b3d$2cb181d0$86148570$@samsung.com>
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
next prev parent reply other threads:[~2015-01-07 9:45 UTC|newest]
Thread overview: 7+ messages / expand[flat|nested] mbox.gz Atom feed top
2014-12-17 7:27 [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support Tony K Nadackal
2014-12-17 14:15 ` Jacek Anaszewski
2014-12-19 3:37 ` Tony K Nadackal
2015-01-07 9:45 ` Jacek Anaszewski [this message]
2015-01-07 12:08 ` Tony K Nadackal
2015-01-07 12:39 ` Jacek Anaszewski
2015-01-12 11:27 ` Tony K Nadackal
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=54AD0033.1020103@samsung.com \
--to=j.anaszewski@samsung$(echo .)com \
--cc=linux-arm-kernel@lists$(echo .)infradead.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox