[PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
Fimp_jpeg used in Exynos7 is a revised version. Some register
configurations are slightly different from Jpeg in Exynos4.
Added one more variant SJPEG_EXYNOS7 to handle these differences.
Signed-off-by: Tony K Nadackal
---
This patch is created and tested on top of linux-next-20141210.
It can be cleanly applied on media-next and kgene/for-next.
.../bindings/media/exynos-jpeg-codec.txt | 2 +-
drivers/media/platform/s5p-jpeg/jpeg-core.c| 69 +++---
drivers/media/platform/s5p-jpeg/jpeg-core.h| 1 +
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 33 ++-
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h | 8 ++-
drivers/media/platform/s5p-jpeg/jpeg-regs.h| 17 --
6 files changed, 98 insertions(+), 32 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
index bf52ed4..cd19417 100644
--- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
+++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
@@ -4,7 +4,7 @@ Required properties:
- compatible : should be one of:
"samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
- "samsung,exynos3250-jpeg";
+ "samsung,exynos3250-jpeg", "samsung,exynos7-jpeg";
- reg : address and length of the JPEG codec IP register set;
- interrupts : specifies the JPEG codec IP interrupt;
- clock-names : should contain:
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 54fa5d9..ad42a4e 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1225,8 +1225,9 @@ static int s5p_jpeg_try_fmt_vid_cap(struct file *file,
void *priv,
return -EINVAL;
}
- if ((ctx->jpeg->variant->version != SJPEG_EXYNOS4) ||
- (ctx->mode != S5P_JPEG_DECODE))
+ if (((ctx->jpeg->variant->version != SJPEG_EXYNOS4) &&
+ (ctx->jpeg->variant->version != SJPEG_EXYNOS7)) ||
+ (ctx->mode != S5P_JPEG_DECODE))
goto exit;
/*
@@ -1349,7 +1350,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct
v4l2_format *f)
* the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
* page fault calculate proper buffer size in such a case.
*/
- if (ct->jpeg->variant->version == SJPEG_EXYNOS4 &&
+ if (((ct->jpeg->variant->version == SJPEG_EXYNOS4) ||
+ (ct->jpeg->variant->version == SJPEG_EXYNOS7)) &&
f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
f,
@@ -1901,7 +1903,6 @@ static void exynos4_jpeg_device_run(void *priv)
if (ctx->mode == S5P_JPEG_ENCODE) {
exynos4_jpeg_sw_reset(jpeg->regs);
- exynos4_jpeg_set_interrupt(jpeg->regs);
exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
exynos4_jpeg_set_huff_tbl(jpeg->regs);
@@ -1918,20 +1919,60 @@ static void exynos4_jpeg_device_run(void *priv)
exynos4_jpeg_set_stream_size(jpeg->regs, ctx->cap_q.w,
ctx->cap_q.h);
- exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling);
- exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc);
+ 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);
+ }
+
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);
Re: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
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_S5P1
#define SJPEG_EXYNOS3250 2
#define SJPEG_EXYNOS43
+#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 *b
RE: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
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);
> >>
>
Re: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
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
RE: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
Hi Kukjin,
On Wednesday, January 07, 2015 6:09 PM, Jacek Anaszewski wrote,
> 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 str
Re: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
Hi Tony,
Thanks for the patches.
Please process them with scripts/checkpatch.pl as you will be submitting
the next version - they contain many coding style related issues.
My remaining comments below.
On 12/17/2014 08:27 AM, Tony K Nadackal wrote:
Fimp_jpeg used in Exynos7 is a revised version. Some register
configurations are slightly different from Jpeg in Exynos4.
Added one more variant SJPEG_EXYNOS7 to handle these differences.
Signed-off-by: Tony K Nadackal
---
This patch is created and tested on top of linux-next-20141210.
It can be cleanly applied on media-next and kgene/for-next.
.../bindings/media/exynos-jpeg-codec.txt | 2 +-
drivers/media/platform/s5p-jpeg/jpeg-core.c| 69 +++---
drivers/media/platform/s5p-jpeg/jpeg-core.h| 1 +
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c | 33 ++-
drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h | 8 ++-
drivers/media/platform/s5p-jpeg/jpeg-regs.h| 17 --
6 files changed, 98 insertions(+), 32 deletions(-)
diff --git a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
index bf52ed4..cd19417 100644
--- a/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
+++ b/Documentation/devicetree/bindings/media/exynos-jpeg-codec.txt
@@ -4,7 +4,7 @@ Required properties:
- compatible : should be one of:
"samsung,s5pv210-jpeg", "samsung,exynos4210-jpeg",
- "samsung,exynos3250-jpeg";
+ "samsung,exynos3250-jpeg", "samsung,exynos7-jpeg";
- reg : address and length of the JPEG codec IP register set;
- interrupts : specifies the JPEG codec IP interrupt;
- clock-names : should contain:
diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.c
b/drivers/media/platform/s5p-jpeg/jpeg-core.c
index 54fa5d9..ad42a4e 100644
--- a/drivers/media/platform/s5p-jpeg/jpeg-core.c
+++ b/drivers/media/platform/s5p-jpeg/jpeg-core.c
@@ -1225,8 +1225,9 @@ static int s5p_jpeg_try_fmt_vid_cap(struct file *file,
void *priv,
return -EINVAL;
}
- if ((ctx->jpeg->variant->version != SJPEG_EXYNOS4) ||
- (ctx->mode != S5P_JPEG_DECODE))
+ if (((ctx->jpeg->variant->version != SJPEG_EXYNOS4) &&
+ (ctx->jpeg->variant->version != SJPEG_EXYNOS7)) ||
+ (ctx->mode != S5P_JPEG_DECODE))
goto exit;
/*
@@ -1349,7 +1350,8 @@ static int s5p_jpeg_s_fmt(struct s5p_jpeg_ctx *ct, struct
v4l2_format *f)
* the JPEG_IMAGE_SIZE register. In order to avoid sysmmu
* page fault calculate proper buffer size in such a case.
*/
- if (ct->jpeg->variant->version == SJPEG_EXYNOS4 &&
+ if (((ct->jpeg->variant->version == SJPEG_EXYNOS4) ||
+ (ct->jpeg->variant->version == SJPEG_EXYNOS7)) &&
f_type == FMT_TYPE_OUTPUT && ct->mode == S5P_JPEG_ENCODE)
q_data->size = exynos4_jpeg_get_output_buffer_size(ct,
f,
@@ -1901,7 +1903,6 @@ static void exynos4_jpeg_device_run(void *priv)
if (ctx->mode == S5P_JPEG_ENCODE) {
exynos4_jpeg_sw_reset(jpeg->regs);
- exynos4_jpeg_set_interrupt(jpeg->regs);
exynos4_jpeg_set_huf_table_enable(jpeg->regs, 1);
exynos4_jpeg_set_huff_tbl(jpeg->regs);
@@ -1918,20 +1919,60 @@ static void exynos4_jpeg_device_run(void *priv)
exynos4_jpeg_set_stream_size(jpeg->regs, ctx->cap_q.w,
ctx->cap_q.h);
- exynos4_jpeg_set_enc_out_fmt(jpeg->regs, ctx->subsampling);
- exynos4_jpeg_set_img_fmt(jpeg->regs, ctx->out_q.fmt->fourcc);
+ 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,
RE: [PATCH] [media] s5p-jpeg: Adding Exynos7 Jpeg variant support
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?
> 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.
> > + 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.
[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.
[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
