Re: [PATCH v2 1/5] [media] exynos-mscl: Add new driver for M-Scaler

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



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




[Index of Archives]     [Linux SoC Development]     [Linux Rockchip Development]     [Linux USB Development]     [Video for Linux]     [Linux Audio Users]     [Linux SCSI]     [Yosemite News]

  Powered by Linux