| Message ID | 1418801229-7532-1-git-send-email-tony.kn@samsung.com |
|---|---|
| State | Needs Review / ACK, archived |
| Headers | show |
| Context | Check | Description |
|---|---|---|
| robh/checkpatch | warning | total: 1 errors, 0 warnings, 0 lines checked |
| robh/patch-applied | success |
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 <tony.kn@samsung.com> > --- > 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, (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); > 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? > + 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? > + exynos4_jpeg_set_enc_out_fmt(jpeg->regs, > + ctx->subsampling, EXYNOS7_ENC_FMT_MASK); > + exynos4_jpeg_set_img_fmt(jpeg->regs, > + ctx->cap_q.fmt->fourcc, > + EXYNOS7_SWAP_CHROMA_SHIFT); > + bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 16); > + } else { > + exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS4); > + exynos4_jpeg_set_img_fmt(jpeg->regs, > + ctx->cap_q.fmt->fourcc, > + EXYNOS4_SWAP_CHROMA_SHIFT); > + bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32); > + } > > exynos4_jpeg_set_dec_bitstream_size(jpeg->regs, bitstream_size); > } > @@ -2729,6 +2770,13 @@ static struct s5p_jpeg_variant exynos4_jpeg_drvdata = { > .fmt_ver_flag = SJPEG_FMT_FLAG_EXYNOS4, > }; > > +static struct s5p_jpeg_variant exynos7_jpeg_drvdata = { > + .version = SJPEG_EXYNOS7, > + .jpeg_irq = exynos4_jpeg_irq, > + .m2m_ops = &exynos4_jpeg_m2m_ops, > + .fmt_ver_flag = SJPEG_FMT_FLAG_EXYNOS4, > +}; > + > static const struct of_device_id samsung_jpeg_match[] = { > { > .compatible = "samsung,s5pv210-jpeg", > @@ -2742,6 +2790,9 @@ static const struct of_device_id samsung_jpeg_match[] = { > }, { > .compatible = "samsung,exynos4212-jpeg", > .data = &exynos4_jpeg_drvdata, > + }, { > + .compatible = "samsung,exynos7-jpeg", > + .data = &exynos7_jpeg_drvdata, > }, > {}, > }; > diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h > index 764b32d..734710a 100644 > --- 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. > enum exynos4_jpeg_result { > OK_ENC_OR_DEC, > diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c > index e53f13a..2611259 100644 > --- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c > +++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c > @@ -49,7 +49,8 @@ void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode) > } > } > > -void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) > +void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt, > + unsigned int shift) > { > unsigned int reg; > > @@ -71,48 +72,48 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) > case V4L2_PIX_FMT_NV24: > reg = reg | EXYNOS4_ENC_YUV_444_IMG | > EXYNOS4_YUV_444_IP_YUV_444_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CBCR; > + EXYNOS_SWAP_CHROMA_CBCR(shift); > break; > case V4L2_PIX_FMT_NV42: > reg = reg | EXYNOS4_ENC_YUV_444_IMG | > EXYNOS4_YUV_444_IP_YUV_444_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CRCB; > + EXYNOS_SWAP_CHROMA_CRCB(shift); > break; > case V4L2_PIX_FMT_YUYV: > reg = reg | EXYNOS4_DEC_YUV_422_IMG | > EXYNOS4_YUV_422_IP_YUV_422_1P_IMG | > - EXYNOS4_SWAP_CHROMA_CBCR; > + EXYNOS_SWAP_CHROMA_CBCR(shift); > break; > > case V4L2_PIX_FMT_YVYU: > reg = reg | EXYNOS4_DEC_YUV_422_IMG | > EXYNOS4_YUV_422_IP_YUV_422_1P_IMG | > - EXYNOS4_SWAP_CHROMA_CRCB; > + EXYNOS_SWAP_CHROMA_CRCB(shift); > break; > case V4L2_PIX_FMT_NV16: > reg = reg | EXYNOS4_DEC_YUV_422_IMG | > EXYNOS4_YUV_422_IP_YUV_422_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CBCR; > + EXYNOS_SWAP_CHROMA_CBCR(shift); > break; > case V4L2_PIX_FMT_NV61: > reg = reg | EXYNOS4_DEC_YUV_422_IMG | > EXYNOS4_YUV_422_IP_YUV_422_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CRCB; > + EXYNOS_SWAP_CHROMA_CRCB(shift); > break; > case V4L2_PIX_FMT_NV12: > reg = reg | EXYNOS4_DEC_YUV_420_IMG | > EXYNOS4_YUV_420_IP_YUV_420_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CBCR; > + EXYNOS_SWAP_CHROMA_CBCR(shift); > break; > case V4L2_PIX_FMT_NV21: > reg = reg | EXYNOS4_DEC_YUV_420_IMG | > EXYNOS4_YUV_420_IP_YUV_420_2P_IMG | > - EXYNOS4_SWAP_CHROMA_CRCB; > + EXYNOS_SWAP_CHROMA_CRCB(shift); > break; > case V4L2_PIX_FMT_YUV420: > reg = reg | EXYNOS4_DEC_YUV_420_IMG | > EXYNOS4_YUV_420_IP_YUV_420_3P_IMG | > - EXYNOS4_SWAP_CHROMA_CBCR; > + EXYNOS_SWAP_CHROMA_CBCR(shift); > break; > default: > break; > @@ -122,12 +123,13 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) > writel(reg, base + EXYNOS4_IMG_FMT_REG); > } > > -void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt) > +void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt, > + unsigned int mask) > { > unsigned int reg; > > reg = readl(base + EXYNOS4_IMG_FMT_REG) & > - ~EXYNOS4_ENC_FMT_MASK; /* clear enc format */ > + ~EXYNOS_ENC_FMT_MASK(mask); /* clear enc format */ > > switch (out_fmt) { > case V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY: > @@ -153,9 +155,12 @@ void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt) > writel(reg, base + EXYNOS4_IMG_FMT_REG); > } > > -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. > unsigned int exynos4_jpeg_get_int_status(void __iomem *base) > diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h > index c228d28..b425199 100644 > --- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h > +++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h > @@ -15,10 +15,12 @@ > > void exynos4_jpeg_sw_reset(void __iomem *base); > void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode); > -void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt); > -void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt); > +void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt, > + unsigned int shift); > +void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt, > + unsigned int mask); > void exynos4_jpeg_set_enc_tbl(void __iomem *base); > -void exynos4_jpeg_set_interrupt(void __iomem *base); > +void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int variant); > unsigned int exynos4_jpeg_get_int_status(void __iomem *base); > void exynos4_jpeg_set_huf_table_enable(void __iomem *base, int value); > void exynos4_jpeg_set_sys_int_enable(void __iomem *base, int value); > diff --git a/drivers/media/platform/s5p-jpeg/jpeg-regs.h b/drivers/media/platform/s5p-jpeg/jpeg-regs.h > index 050fc44..08bef4e 100644 > --- a/drivers/media/platform/s5p-jpeg/jpeg-regs.h > +++ b/drivers/media/platform/s5p-jpeg/jpeg-regs.h > @@ -230,13 +230,15 @@ > #define EXYNOS4_SOFT_RESET_HI (1 << 29) > > /* JPEG INT Register bit */ > -#define EXYNOS4_INT_EN_MASK (0x1f << 0) > +#define EXYNOS4_INT_EN_MASK(version) (((version) == SJPEG_EXYNOS7) \ > + ? (0x1ff << 0) : (0x1f << 0)) > #define EXYNOS4_PROT_ERR_INT_EN (1 << 0) > #define EXYNOS4_IMG_COMPLETION_INT_EN (1 << 1) > #define EXYNOS4_DEC_INVALID_FORMAT_EN (1 << 2) > #define EXYNOS4_MULTI_SCAN_ERROR_EN (1 << 3) > #define EXYNOS4_FRAME_ERR_EN (1 << 4) > -#define EXYNOS4_INT_EN_ALL (0x1f << 0) > +#define EXYNOS4_INT_EN_ALL(version) (((version) == SJPEG_EXYNOS7) \ > + ? (0x1b6 << 0) : (0x1f << 0)) > > #define EXYNOS4_MOD_REG_PROC_ENC (0 << 3) > #define EXYNOS4_MOD_REG_PROC_DEC (1 << 3) > @@ -294,8 +296,11 @@ > #define EXYNOS4_YUV_420_IP_YUV_420_2P_IMG (4 << EXYNOS4_YUV_420_IP_SHIFT) > #define EXYNOS4_YUV_420_IP_YUV_420_3P_IMG (5 << EXYNOS4_YUV_420_IP_SHIFT) > > +#define EXYNOS4_ENC_FMT_MASK 3 > +#define EXYNOS7_ENC_FMT_MASK 7 > #define EXYNOS4_ENC_FMT_SHIFT 24 > -#define EXYNOS4_ENC_FMT_MASK (3 << EXYNOS4_ENC_FMT_SHIFT) > +#define EXYNOS_ENC_FMT_MASK(mask) ((mask) \ > + << EXYNOS4_ENC_FMT_SHIFT) > #define EXYNOS4_ENC_FMT_GRAY (0 << EXYNOS4_ENC_FMT_SHIFT) > #define EXYNOS4_ENC_FMT_YUV_444 (1 << EXYNOS4_ENC_FMT_SHIFT) > #define EXYNOS4_ENC_FMT_YUV_422 (2 << EXYNOS4_ENC_FMT_SHIFT) > @@ -303,8 +308,10 @@ > > #define EXYNOS4_JPEG_DECODED_IMG_FMT_MASK 0x03 > > -#define EXYNOS4_SWAP_CHROMA_CRCB (1 << 26) > -#define EXYNOS4_SWAP_CHROMA_CBCR (0 << 26) > +#define EXYNOS7_SWAP_CHROMA_SHIFT 27 > +#define EXYNOS4_SWAP_CHROMA_SHIFT 26 > +#define EXYNOS_SWAP_CHROMA_CRCB(shift) (1 << (shift)) > +#define EXYNOS_SWAP_CHROMA_CBCR(shift) (0 << (shift)) > > /* JPEG HUFF count Register bit */ > #define EXYNOS4_HUFF_COUNT_MASK 0xffff >
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 patch and explain its > merit. > Thanks for the suggestion. I will make a separate patch for this bug fix. [snip] > > -- > Best Regards, > Jacek Anaszewski Thanks, Tony K Nadackal -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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.
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 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. Will you please take care of this..? > > -- > Best Regards, > Jacek Anaszewski Thanks and Regards Tony -- To unsubscribe from this list: send the line "unsubscribe devicetree" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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); 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); + + 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, EXYNOS7_ENC_FMT_MASK); + exynos4_jpeg_set_img_fmt(jpeg->regs, + ctx->cap_q.fmt->fourcc, + EXYNOS7_SWAP_CHROMA_SHIFT); + bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 16); + } else { + exynos4_jpeg_set_interrupt(jpeg->regs, SJPEG_EXYNOS4); + exynos4_jpeg_set_img_fmt(jpeg->regs, + ctx->cap_q.fmt->fourcc, + EXYNOS4_SWAP_CHROMA_SHIFT); + bitstream_size = DIV_ROUND_UP(ctx->out_q.size, 32); + } exynos4_jpeg_set_dec_bitstream_size(jpeg->regs, bitstream_size); } @@ -2729,6 +2770,13 @@ static struct s5p_jpeg_variant exynos4_jpeg_drvdata = { .fmt_ver_flag = SJPEG_FMT_FLAG_EXYNOS4, }; +static struct s5p_jpeg_variant exynos7_jpeg_drvdata = { + .version = SJPEG_EXYNOS7, + .jpeg_irq = exynos4_jpeg_irq, + .m2m_ops = &exynos4_jpeg_m2m_ops, + .fmt_ver_flag = SJPEG_FMT_FLAG_EXYNOS4, +}; + static const struct of_device_id samsung_jpeg_match[] = { { .compatible = "samsung,s5pv210-jpeg", @@ -2742,6 +2790,9 @@ static const struct of_device_id samsung_jpeg_match[] = { }, { .compatible = "samsung,exynos4212-jpeg", .data = &exynos4_jpeg_drvdata, + }, { + .compatible = "samsung,exynos7-jpeg", + .data = &exynos7_jpeg_drvdata, }, {}, }; diff --git a/drivers/media/platform/s5p-jpeg/jpeg-core.h b/drivers/media/platform/s5p-jpeg/jpeg-core.h index 764b32d..734710a 100644 --- 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 enum exynos4_jpeg_result { OK_ENC_OR_DEC, diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c index e53f13a..2611259 100644 --- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c +++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.c @@ -49,7 +49,8 @@ void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode) } } -void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) +void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt, + unsigned int shift) { unsigned int reg; @@ -71,48 +72,48 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) case V4L2_PIX_FMT_NV24: reg = reg | EXYNOS4_ENC_YUV_444_IMG | EXYNOS4_YUV_444_IP_YUV_444_2P_IMG | - EXYNOS4_SWAP_CHROMA_CBCR; + EXYNOS_SWAP_CHROMA_CBCR(shift); break; case V4L2_PIX_FMT_NV42: reg = reg | EXYNOS4_ENC_YUV_444_IMG | EXYNOS4_YUV_444_IP_YUV_444_2P_IMG | - EXYNOS4_SWAP_CHROMA_CRCB; + EXYNOS_SWAP_CHROMA_CRCB(shift); break; case V4L2_PIX_FMT_YUYV: reg = reg | EXYNOS4_DEC_YUV_422_IMG | EXYNOS4_YUV_422_IP_YUV_422_1P_IMG | - EXYNOS4_SWAP_CHROMA_CBCR; + EXYNOS_SWAP_CHROMA_CBCR(shift); break; case V4L2_PIX_FMT_YVYU: reg = reg | EXYNOS4_DEC_YUV_422_IMG | EXYNOS4_YUV_422_IP_YUV_422_1P_IMG | - EXYNOS4_SWAP_CHROMA_CRCB; + EXYNOS_SWAP_CHROMA_CRCB(shift); break; case V4L2_PIX_FMT_NV16: reg = reg | EXYNOS4_DEC_YUV_422_IMG | EXYNOS4_YUV_422_IP_YUV_422_2P_IMG | - EXYNOS4_SWAP_CHROMA_CBCR; + EXYNOS_SWAP_CHROMA_CBCR(shift); break; case V4L2_PIX_FMT_NV61: reg = reg | EXYNOS4_DEC_YUV_422_IMG | EXYNOS4_YUV_422_IP_YUV_422_2P_IMG | - EXYNOS4_SWAP_CHROMA_CRCB; + EXYNOS_SWAP_CHROMA_CRCB(shift); break; case V4L2_PIX_FMT_NV12: reg = reg | EXYNOS4_DEC_YUV_420_IMG | EXYNOS4_YUV_420_IP_YUV_420_2P_IMG | - EXYNOS4_SWAP_CHROMA_CBCR; + EXYNOS_SWAP_CHROMA_CBCR(shift); break; case V4L2_PIX_FMT_NV21: reg = reg | EXYNOS4_DEC_YUV_420_IMG | EXYNOS4_YUV_420_IP_YUV_420_2P_IMG | - EXYNOS4_SWAP_CHROMA_CRCB; + EXYNOS_SWAP_CHROMA_CRCB(shift); break; case V4L2_PIX_FMT_YUV420: reg = reg | EXYNOS4_DEC_YUV_420_IMG | EXYNOS4_YUV_420_IP_YUV_420_3P_IMG | - EXYNOS4_SWAP_CHROMA_CBCR; + EXYNOS_SWAP_CHROMA_CBCR(shift); break; default: break; @@ -122,12 +123,13 @@ void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt) writel(reg, base + EXYNOS4_IMG_FMT_REG); } -void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt) +void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt, + unsigned int mask) { unsigned int reg; reg = readl(base + EXYNOS4_IMG_FMT_REG) & - ~EXYNOS4_ENC_FMT_MASK; /* clear enc format */ + ~EXYNOS_ENC_FMT_MASK(mask); /* clear enc format */ switch (out_fmt) { case V4L2_JPEG_CHROMA_SUBSAMPLING_GRAY: @@ -153,9 +155,12 @@ void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt) writel(reg, base + EXYNOS4_IMG_FMT_REG); } -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); } unsigned int exynos4_jpeg_get_int_status(void __iomem *base) diff --git a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h index c228d28..b425199 100644 --- a/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h +++ b/drivers/media/platform/s5p-jpeg/jpeg-hw-exynos4.h @@ -15,10 +15,12 @@ void exynos4_jpeg_sw_reset(void __iomem *base); void exynos4_jpeg_set_enc_dec_mode(void __iomem *base, unsigned int mode); -void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt); -void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt); +void exynos4_jpeg_set_img_fmt(void __iomem *base, unsigned int img_fmt, + unsigned int shift); +void exynos4_jpeg_set_enc_out_fmt(void __iomem *base, unsigned int out_fmt, + unsigned int mask); void exynos4_jpeg_set_enc_tbl(void __iomem *base); -void exynos4_jpeg_set_interrupt(void __iomem *base); +void exynos4_jpeg_set_interrupt(void __iomem *base, unsigned int variant); unsigned int exynos4_jpeg_get_int_status(void __iomem *base); void exynos4_jpeg_set_huf_table_enable(void __iomem *base, int value); void exynos4_jpeg_set_sys_int_enable(void __iomem *base, int value); diff --git a/drivers/media/platform/s5p-jpeg/jpeg-regs.h b/drivers/media/platform/s5p-jpeg/jpeg-regs.h index 050fc44..08bef4e 100644 --- a/drivers/media/platform/s5p-jpeg/jpeg-regs.h +++ b/drivers/media/platform/s5p-jpeg/jpeg-regs.h @@ -230,13 +230,15 @@ #define EXYNOS4_SOFT_RESET_HI (1 << 29) /* JPEG INT Register bit */ -#define EXYNOS4_INT_EN_MASK (0x1f << 0) +#define EXYNOS4_INT_EN_MASK(version) (((version) == SJPEG_EXYNOS7) \ + ? (0x1ff << 0) : (0x1f << 0)) #define EXYNOS4_PROT_ERR_INT_EN (1 << 0) #define EXYNOS4_IMG_COMPLETION_INT_EN (1 << 1) #define EXYNOS4_DEC_INVALID_FORMAT_EN (1 << 2) #define EXYNOS4_MULTI_SCAN_ERROR_EN (1 << 3) #define EXYNOS4_FRAME_ERR_EN (1 << 4) -#define EXYNOS4_INT_EN_ALL (0x1f << 0) +#define EXYNOS4_INT_EN_ALL(version) (((version) == SJPEG_EXYNOS7) \ + ? (0x1b6 << 0) : (0x1f << 0)) #define EXYNOS4_MOD_REG_PROC_ENC (0 << 3) #define EXYNOS4_MOD_REG_PROC_DEC (1 << 3) @@ -294,8 +296,11 @@ #define EXYNOS4_YUV_420_IP_YUV_420_2P_IMG (4 << EXYNOS4_YUV_420_IP_SHIFT) #define EXYNOS4_YUV_420_IP_YUV_420_3P_IMG (5 << EXYNOS4_YUV_420_IP_SHIFT) +#define EXYNOS4_ENC_FMT_MASK 3 +#define EXYNOS7_ENC_FMT_MASK 7 #define EXYNOS4_ENC_FMT_SHIFT 24 -#define EXYNOS4_ENC_FMT_MASK (3 << EXYNOS4_ENC_FMT_SHIFT) +#define EXYNOS_ENC_FMT_MASK(mask) ((mask) \ + << EXYNOS4_ENC_FMT_SHIFT) #define EXYNOS4_ENC_FMT_GRAY (0 << EXYNOS4_ENC_FMT_SHIFT) #define EXYNOS4_ENC_FMT_YUV_444 (1 << EXYNOS4_ENC_FMT_SHIFT) #define EXYNOS4_ENC_FMT_YUV_422 (2 << EXYNOS4_ENC_FMT_SHIFT) @@ -303,8 +308,10 @@ #define EXYNOS4_JPEG_DECODED_IMG_FMT_MASK 0x03 -#define EXYNOS4_SWAP_CHROMA_CRCB (1 << 26) -#define EXYNOS4_SWAP_CHROMA_CBCR (0 << 26) +#define EXYNOS7_SWAP_CHROMA_SHIFT 27 +#define EXYNOS4_SWAP_CHROMA_SHIFT 26 +#define EXYNOS_SWAP_CHROMA_CRCB(shift) (1 << (shift)) +#define EXYNOS_SWAP_CHROMA_CBCR(shift) (0 << (shift)) /* JPEG HUFF count Register bit */ #define EXYNOS4_HUFF_COUNT_MASK 0xffff
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 <tony.kn@samsung.com> --- 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(-)