On Tue, Aug 20, 2013 at 2:13 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: > > >> -----Original Message----- >> From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- >> owner@xxxxxxxxxxxxxxx] On Behalf Of Shaik Ameer Basha >> Sent: Tuesday, August 20, 2013 5:07 PM >> To: Inki Dae >> Cc: Shaik Ameer Basha; LMML; linux-samsung-soc@xxxxxxxxxxxxxxx; >> cpgs@xxxxxxxxxxx; Sylwester Nawrocki; posciak@xxxxxxxxxx; Arun Kumar K >> Subject: Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M- >> Scaler >> >> Hi Inki Dae, >> >> Thanks for the review. >> >> >> On Mon, Aug 19, 2013 at 6:18 PM, Inki Dae <inki.dae@xxxxxxxxxxx> wrote: >> > Just quick review. >> > >> >> -----Original Message----- >> >> From: linux-media-owner@xxxxxxxxxxxxxxx [mailto:linux-media- >> >> owner@xxxxxxxxxxxxxxx] On Behalf Of Shaik Ameer Basha >> >> Sent: Monday, August 19, 2013 7:59 PM >> >> To: linux-media@xxxxxxxxxxxxxxx; linux-samsung-soc@xxxxxxxxxxxxxxx >> >> Cc: s.nawrocki@xxxxxxxxxxx; posciak@xxxxxxxxxx; arun.kk@xxxxxxxxxxx; >> >> shaik.ameer@xxxxxxxxxxx >> >> Subject: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M- >> Scaler >> >> >> >> This patch adds support for M-Scaler (M2M Scaler) device which is a >> >> new device for scaling, blending, color fill and color space >> >> conversion on EXYNOS5 SoCs. >> >> >> >> This device supports the followings as key feature. >> >> input image format >> >> - YCbCr420 2P(UV/VU), 3P >> >> - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P >> >> - YCbCr444 2P(UV,VU), 3P >> >> - RGB565, ARGB1555, ARGB4444, ARGB8888, RGBA8888 >> >> - Pre-multiplexed ARGB8888, L8A8 and L8 >> >> output image format >> >> - YCbCr420 2P(UV/VU), 3P >> >> - YCbCr422 1P(YUYV/UYVY/YVYU), 2P(UV,VU), 3P >> >> - YCbCr444 2P(UV,VU), 3P >> >> - RGB565, ARGB1555, ARGB4444, ARGB8888, RGBA8888 >> >> - Pre-multiplexed ARGB8888 >> >> input rotation >> >> - 0/90/180/270 degree, X/Y/XY Flip >> >> scale ratio >> >> - 1/4 scale down to 16 scale up >> >> color space conversion >> >> - RGB to YUV / YUV to RGB >> >> Size >> >> - Input : 16x16 to 8192x8192 >> >> - Output: 4x4 to 8192x8192 >> >> alpha blending, color fill >> >> >> >> Signed-off-by: Shaik Ameer Basha <shaik.ameer@xxxxxxxxxxx> >> >> --- >> >> drivers/media/platform/exynos-mscl/mscl-regs.c | 318 >> >> ++++++++++++++++++++++++ >> >> drivers/media/platform/exynos-mscl/mscl-regs.h | 282 >> >> +++++++++++++++++++++ >> >> 2 files changed, 600 insertions(+) >> >> create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.c >> >> create mode 100644 drivers/media/platform/exynos-mscl/mscl-regs.h >> >> >> >> diff --git a/drivers/media/platform/exynos-mscl/mscl-regs.c >> >> b/drivers/media/platform/exynos-mscl/mscl-regs.c >> >> new file mode 100644 >> >> index 0000000..9354afc >> >> --- /dev/null >> >> +++ b/drivers/media/platform/exynos-mscl/mscl-regs.c >> >> @@ -0,0 +1,318 @@ >> >> +/* >> >> + * Copyright (c) 2013 - 2014 Samsung Electronics Co., Ltd. >> >> + * http://www.samsung.com >> >> + * >> >> + * Samsung EXYNOS5 SoC series M-Scaler driver >> >> + * >> >> + * This program is free software; you can redistribute it and/or >> modify >> >> + * it under the terms of the GNU General Public License as published >> >> + * by the Free Software Foundation, either version 2 of the License, >> >> + * or (at your option) any later version. >> >> + */ >> >> + >> >> +#include <linux/delay.h> >> >> +#include <linux/platform_device.h> >> >> + >> >> +#include "mscl-core.h" >> >> + >> >> +void mscl_hw_set_sw_reset(struct mscl_dev *dev) >> >> +{ >> >> + u32 cfg; >> >> + >> >> + cfg = readl(dev->regs + MSCL_CFG); >> >> + cfg |= MSCL_CFG_SOFT_RESET; >> >> + >> >> + writel(cfg, dev->regs + MSCL_CFG); >> >> +} >> >> + >> >> +int mscl_wait_reset(struct mscl_dev *dev) >> >> +{ >> >> + unsigned long end = jiffies + msecs_to_jiffies(50); >> > >> > What does 50 mean? >> > >> >> + u32 cfg, reset_done = 0; >> >> + >> > >> > Please describe why the below codes are needed. >> >> >> As per the Documentation, >> >> " SOFT RESET: Writing "1" to this bit generates software reset. To >> check the completion of the reset, wait until this >> field becomes zero, then wrie an arbitrary value to any of RW >> registers and read it. If the read data matches the written data, >> it means SW reset succeeded. Otherwise, repeat write & read until >> matched." >> >> >> Thie below code tries to do the same (as per user manual). and in the >> above msec_to_jiffies(50), 50 is the 50msec wait. before >> checking the SOFT RESET is really done. >> >> Is it good to ignore this checks? >> > > No, I mean that someone may want to understand your codes so leave comments enough for them. Ok. thanks. I will add more comments. :) Regards, Shaik Ameer Basha > > Thanks, > Inki Dae > >> >> >> > >> >> + while (time_before(jiffies, end)) { >> >> + cfg = readl(dev->regs + MSCL_CFG); >> >> + if (!(cfg & MSCL_CFG_SOFT_RESET)) { >> >> + reset_done = 1; >> >> + break; >> >> + } >> >> + usleep_range(10, 20); >> >> + } >> >> + >> >> + /* write any value to r/w reg and read it back */ >> >> + while (reset_done) { >> >> + >> >> + /* [TBD] need to define number of tries before returning >> >> + * -EBUSY to the caller >> >> + */ >> >> + >> >> + writel(MSCL_CFG_SOFT_RESET_CHECK_VAL, >> >> + dev->regs + MSCL_CFG_SOFT_RESET_CHECK_REG); >> >> + if (MSCL_CFG_SOFT_RESET_CHECK_VAL == >> >> + readl(dev->regs + MSCL_CFG_SOFT_RESET_CHECK_REG)) >> >> + return 0; >> >> + } >> >> + >> >> + return -EBUSY; >> >> +} >> >> + >> >> +void mscl_hw_set_irq_mask(struct mscl_dev *dev, int interrupt, bool >> mask) >> >> +{ >> >> + u32 cfg; >> >> + >> >> + switch (interrupt) { >> >> + case MSCL_INT_TIMEOUT: >> >> + case MSCL_INT_ILLEGAL_BLEND: >> >> + case MSCL_INT_ILLEGAL_RATIO: >> >> + case MSCL_INT_ILLEGAL_DST_HEIGHT: >> >> + case MSCL_INT_ILLEGAL_DST_WIDTH: >> >> + case MSCL_INT_ILLEGAL_DST_V_POS: >> >> + case MSCL_INT_ILLEGAL_DST_H_POS: >> >> + case MSCL_INT_ILLEGAL_DST_C_SPAN: >> >> + case MSCL_INT_ILLEGAL_DST_Y_SPAN: >> >> + case MSCL_INT_ILLEGAL_DST_CR_BASE: >> >> + case MSCL_INT_ILLEGAL_DST_CB_BASE: >> >> + case MSCL_INT_ILLEGAL_DST_Y_BASE: >> >> + case MSCL_INT_ILLEGAL_DST_COLOR: >> >> + case MSCL_INT_ILLEGAL_SRC_HEIGHT: >> >> + case MSCL_INT_ILLEGAL_SRC_WIDTH: >> >> + case MSCL_INT_ILLEGAL_SRC_CV_POS: >> >> + case MSCL_INT_ILLEGAL_SRC_CH_POS: >> >> + case MSCL_INT_ILLEGAL_SRC_YV_POS: >> >> + case MSCL_INT_ILLEGAL_SRC_YH_POS: >> >> + case MSCL_INT_ILLEGAL_SRC_C_SPAN: >> >> + case MSCL_INT_ILLEGAL_SRC_Y_SPAN: >> >> + case MSCL_INT_ILLEGAL_SRC_CR_BASE: >> >> + case MSCL_INT_ILLEGAL_SRC_CB_BASE: >> >> + case MSCL_INT_ILLEGAL_SRC_Y_BASE: >> >> + case MSCL_INT_ILLEGAL_SRC_COLOR: >> >> + case MSCL_INT_FRAME_END: >> >> + break; >> >> + default: >> >> + return; >> >> + } >> > >> > It seems that the above codes could be more simple, >> >> >> ok. will change this. >> >> >> > >> > >> >> + cfg = readl(dev->regs + MSCL_INT_EN); >> >> + if (mask) >> >> + cfg |= interrupt; >> >> + else >> >> + cfg &= ~interrupt; >> >> + writel(cfg, dev->regs + MSCL_INT_EN); >> >> +} >> >> + >> >> +void mscl_hw_set_input_addr(struct mscl_dev *dev, struct mscl_addr >> *addr) >> >> +{ >> >> + dev_dbg(&dev->pdev->dev, "src_buf: 0x%X, cb: 0x%X, cr: 0x%X", >> >> + addr->y, addr->cb, addr->cr); >> >> + writel(addr->y, dev->regs + MSCL_SRC_Y_BASE); >> >> + writel(addr->cb, dev->regs + MSCL_SRC_CB_BASE); >> >> + writel(addr->cr, dev->regs + MSCL_SRC_CR_BASE); >> >> +} >> >> + >> >> +void mscl_hw_set_output_addr(struct mscl_dev *dev, >> >> + struct mscl_addr *addr) >> >> +{ >> >> + dev_dbg(&dev->pdev->dev, "dst_buf: 0x%X, cb: 0x%X, cr: 0x%X", >> >> + addr->y, addr->cb, addr->cr); >> >> + writel(addr->y, dev->regs + MSCL_DST_Y_BASE); >> >> + writel(addr->cb, dev->regs + MSCL_DST_CB_BASE); >> >> + writel(addr->cr, dev->regs + MSCL_DST_CR_BASE); >> >> +} >> >> + >> >> +void mscl_hw_set_in_size(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + struct mscl_frame *frame = &ctx->s_frame; >> >> + u32 cfg; >> >> + >> >> + /* set input pixel offset */ >> >> + cfg = MSCL_SRC_YH_POS(frame->crop.left); >> >> + cfg |= MSCL_SRC_YV_POS(frame->crop.top); >> > >> > Where are the limitations to left and top checked?. >> >> >> >> mscl_try_crop() does this checking. >> >> >> >> > >> >> + writel(cfg, dev->regs + MSCL_SRC_Y_POS); >> >> + >> >> + /* [TBD] calculate 'C' plane h/v offset using 'Y' plane h/v offset >> >> */ >> >> + >> >> + /* set input span */ >> >> + cfg = MSCL_SRC_Y_SPAN(frame->f_width); >> >> + if (is_yuv420_2p(frame->fmt)) >> >> + cfg |= MSCL_SRC_C_SPAN(frame->f_width); >> >> + else >> >> + cfg |= MSCL_SRC_C_SPAN(frame->f_width); /* [TBD] Verify */ >> >> + >> >> + writel(cfg, dev->regs + MSCL_SRC_SPAN); >> >> + >> >> + /* Set input cropped size */ >> >> + cfg = MSCL_SRC_WIDTH(frame->crop.width); >> >> + cfg |= MSCL_SRC_HEIGHT(frame->crop.height); >> >> + writel(cfg, dev->regs + MSCL_SRC_WH); >> >> + >> >> + dev_dbg(&dev->pdev->dev, >> >> + "src: posx: %d, posY: %d, spanY: %d, spanC: %d, " >> >> + "cropX: %d, cropY: %d\n", >> >> + frame->crop.left, frame->crop.top, frame->f_width, >> >> + frame->f_width, frame->crop.width, frame->crop.height); >> >> +} >> >> + >> >> +void mscl_hw_set_in_image_format(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + struct mscl_frame *frame = &ctx->s_frame; >> >> + u32 cfg; >> >> + >> >> + cfg = readl(dev->regs + MSCL_SRC_CFG); >> >> + cfg &= ~MSCL_SRC_COLOR_FORMAT_MASK; >> >> + cfg |= MSCL_SRC_COLOR_FORMAT(frame->fmt->mscl_color); >> >> + >> >> + /* setting tile/linear format */ >> >> + if (frame->fmt->is_tiled) >> >> + cfg |= MSCL_SRC_TILE_EN; >> >> + else >> >> + cfg &= ~MSCL_SRC_TILE_EN; >> >> + >> >> + writel(cfg, dev->regs + MSCL_SRC_CFG); >> >> +} >> >> + >> >> +void mscl_hw_set_out_size(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + struct mscl_frame *frame = &ctx->d_frame; >> >> + u32 cfg; >> >> + >> >> + /* set output pixel offset */ >> >> + cfg = MSCL_DST_H_POS(frame->crop.left); >> >> + cfg |= MSCL_DST_V_POS(frame->crop.top); >> > >> > Ditto. >> > >> >> + writel(cfg, dev->regs + MSCL_DST_POS); >> >> + >> >> + /* set output span */ >> >> + cfg = MSCL_DST_Y_SPAN(frame->f_width); >> >> + if (is_yuv420_2p(frame->fmt)) >> >> + cfg |= MSCL_DST_C_SPAN(frame->f_width/2); >> >> + else >> >> + cfg |= MSCL_DST_C_SPAN(frame->f_width); >> >> + writel(cfg, dev->regs + MSCL_DST_SPAN); >> >> + >> >> + /* set output scaled size */ >> >> + cfg = MSCL_DST_WIDTH(frame->crop.width); >> >> + cfg |= MSCL_DST_HEIGHT(frame->crop.height); >> >> + writel(cfg, dev->regs + MSCL_DST_WH); >> >> + >> >> + dev_dbg(&dev->pdev->dev, >> >> + "dst: posx: %d, posY: %d, spanY: %d, spanC: %d, " >> >> + "cropX: %d, cropY: %d\n", >> >> + frame->crop.left, frame->crop.top, frame->f_width, >> >> + frame->f_width, frame->crop.width, frame->crop.height); >> >> +} >> >> + >> >> +void mscl_hw_set_out_image_format(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + struct mscl_frame *frame = &ctx->d_frame; >> >> + u32 cfg; >> >> + >> >> + cfg = readl(dev->regs + MSCL_DST_CFG); >> >> + cfg &= ~MSCL_DST_COLOR_FORMAT_MASK; >> >> + cfg |= MSCL_DST_COLOR_FORMAT(frame->fmt->mscl_color); >> >> + >> >> + writel(cfg, dev->regs + MSCL_DST_CFG); >> >> +} >> >> + >> >> +void mscl_hw_set_scaler_ratio(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + struct mscl_scaler *sc = &ctx->scaler; >> >> + u32 cfg; >> >> + >> >> + cfg = MSCL_H_RATIO_VALUE(sc->hratio); >> >> + writel(cfg, dev->regs + MSCL_H_RATIO); >> >> + >> >> + cfg = MSCL_V_RATIO_VALUE(sc->vratio); >> >> + writel(cfg, dev->regs + MSCL_V_RATIO); >> >> +} >> >> + >> >> +void mscl_hw_set_rotation(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + u32 cfg = 0; >> >> + >> >> + cfg = MSCL_ROTMODE(ctx->ctrls_mscl.rotate->val/90); >> >> + >> >> + if (ctx->ctrls_mscl.hflip->val) >> >> + cfg |= MSCL_FLIP_X_EN; >> >> + >> >> + if (ctx->ctrls_mscl.vflip->val) >> >> + cfg |= MSCL_FLIP_Y_EN; >> >> + >> >> + writel(cfg, dev->regs + MSCL_ROT_CFG); >> >> +} >> >> + >> >> +void mscl_hw_address_queue_reset(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + >> >> + writel(MSCL_ADDR_QUEUE_RST, dev->regs + MSCL_ADDR_QUEUE_CONFIG); >> >> +} >> >> + >> >> +void mscl_hw_set_csc_coeff(struct mscl_ctx *ctx) >> >> +{ >> >> + struct mscl_dev *dev = ctx->mscl_dev; >> >> + enum mscl_csc_coeff type; >> >> + u32 cfg = 0; >> >> + int i, j; >> >> + static const u32 csc_coeff[MSCL_CSC_COEFF_MAX][3][3] = { >> >> + { /* YCbCr to RGB */ >> >> + {0x200, 0x000, 0x2be}, >> >> + {0x200, 0xeac, 0x165}, >> >> + {0x200, 0x377, 0x000} >> >> + }, >> >> + { /* YCbCr to RGB with -16 offset */ >> >> + {0x254, 0x000, 0x331}, >> >> + {0x254, 0xec8, 0xFA0}, >> >> + {0x254, 0x409, 0x000} >> >> + }, >> >> + { /* RGB to YCbCr */ >> >> + {0x099, 0x12d, 0x03a}, >> >> + {0xe58, 0xeae, 0x106}, >> >> + {0x106, 0xedb, 0xe2a} >> >> + }, >> >> + { /* RGB to YCbCr with -16 offset */ >> >> + {0x084, 0x102, 0x032}, >> >> + {0xe4c, 0xe95, 0x0e1}, >> >> + {0x0e1, 0xebc, 0xe24} >> >> + } }; >> >> + >> >> + if (is_rgb(ctx->s_frame.fmt) == is_rgb(ctx->d_frame.fmt)) >> >> + type = MSCL_CSC_COEFF_NONE; >> >> + else if (is_rgb(ctx->d_frame.fmt)) >> >> + type = MSCL_CSC_COEFF_YCBCR_TO_RGB_OFF16; >> >> + else >> >> + type = MSCL_CSC_COEFF_RGB_TO_YCBCR_OFF16; >> >> + >> >> + if ((type == ctx->mscl_dev->coeff_type) || (type >= >> >> MSCL_CSC_COEFF_MAX)) >> >> + return; >> >> + >> >> + for (i = 0; i < 3; i++) { >> >> + for (j = 0; j < 3; j++) { >> >> + cfg = csc_coeff[type][i][j]; >> >> + writel(cfg, dev->regs + MSCL_CSC_COEF(i, j)); >> >> + } >> >> + } >> >> + >> >> + switch (type) { >> >> + case MSCL_CSC_COEFF_YCBCR_TO_RGB: >> > >> > Is there this case? >> > >> >> + mscl_hw_src_y_offset_en(ctx->mscl_dev, false); >> > >> > And this switch-case could be removed if you move the above line to the >> > above if-sentence. >> > >> > >> >> + break; >> >> + case MSCL_CSC_COEFF_YCBCR_TO_RGB_OFF16: >> >> + mscl_hw_src_y_offset_en(ctx->mscl_dev, true); >> > >> > Ditto. >> > >> >> + break; >> >> + case MSCL_CSC_COEFF_RGB_TO_YCBCR: >> > >> > Seems no case. >> > >> >> + mscl_hw_src_y_offset_en(ctx->mscl_dev, false); >> > >> > Could be moved to the above if-sentence. >> >> >> >> I think MSCL_CSC_COEFF_YCBCR_TO_RGB_OFF16, MSCL_CSC_COEFF_YCBCR_TO_RGB >> belongs to different color spaces. >> Anyways, will remove the unused cases and will reorganize the code as >> per your comments. >> >> Regards, >> Shaik Ameer Basha >> >> >> >> > >> >> + break; >> >> + case MSCL_CSC_COEFF_RGB_TO_YCBCR_OFF16: >> >> + mscl_hw_src_y_offset_en(ctx->mscl_dev, true); >> > >> > Ditto. >> > >> >> + break; >> >> + default: >> >> + return; >> >> + } >> >> + >> >> + ctx->mscl_dev->coeff_type = type; >> >> + return; >> >> +} >> >> [snip] >> -- >> To unsubscribe from this list: send the line "unsubscribe linux-media" in >> the body of a message to majordomo@xxxxxxxxxxxxxxx >> More majordomo info at http://vger.kernel.org/majordomo-info.html > -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
