> -----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. 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-samsung-soc" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html
 |