Hi Shaik,

A few more remarks...

On 07/05/2012 12:27 PM, Shaik Ameer Basha wrote:
> diff --git a/drivers/media/video/exynos/gsc/gsc-core.c 
> b/drivers/media/video/exynos/gsc/gsc-core.c
> new file mode 100644
> index 0000000..06bd24f
> --- /dev/null
> +++ b/drivers/media/video/exynos/gsc/gsc-core.c
> @@ -0,0 +1,1304 @@
> +/* linux/drivers/media/video/exynos/gsc/gsc-core.c
> + *
> + * Copyright (c) 2011 - 2012 Samsung Electronics Co., Ltd.
> + *           http://www.samsung.com
> + *
> + * Samsung EXYNOS5 SoC series G-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/module.h>
> +#include<linux/kernel.h>
> +#include<linux/version.h>
> +#include<linux/types.h>
> +#include<linux/errno.h>
> +#include<linux/bug.h>
> +#include<linux/interrupt.h>
> +#include<linux/workqueue.h>
> +#include<linux/device.h>
> +#include<linux/platform_device.h>
> +#include<linux/list.h>
> +#include<linux/io.h>
> +#include<linux/slab.h>
> +#include<linux/clk.h>
> +#include<media/v4l2-ioctl.h>
> +#include<linux/of.h>
> +#include "gsc-core.h"
> +
> +#define GSC_CLOCK_GATE_NAME  "gscl"
> +
> +int gsc_dbg;
> +module_param(gsc_dbg, int, 0644);
> +
> +
> +static struct gsc_fmt gsc_formats[] = {

static const

> +     {
> +             .name           = "RGB565",
> +             .pixelformat    = V4L2_PIX_FMT_RGB565X,
> +             .depth          = { 16 },
> +             .color          = GSC_RGB,
> +             .num_planes     = 1,
> +             .num_comp       = 1,
> +     }, {
...
> +struct gsc_fmt *find_fmt(u32 *pixelformat, u32 *mbus_code, int index)
> +{
> +     struct gsc_fmt *fmt, *def_fmt = NULL;
> +     unsigned int i;
> +
> +     if (index>= ARRAY_SIZE(gsc_formats))

        if (index >= (int)ARRAY_SIZE(gsc_formats))

(otherwise negative indexes won't work)

> +             return NULL;
> +
> +     for (i = 0; i<  ARRAY_SIZE(gsc_formats); ++i) {
> +             fmt = get_format(i);
> +             if (pixelformat&&  fmt->pixelformat == *pixelformat)
> +                     return fmt;
> +             if (mbus_code&&  fmt->mbus_code == *mbus_code)
> +                     return fmt;
> +             if (index == i)
> +                     def_fmt = fmt;
> +     }
> +     return def_fmt;
> +
> +}
...
> +int gsc_enum_fmt_mplane(struct v4l2_fmtdesc *f)
> +{
> +     struct gsc_fmt *fmt;
> +
> +     fmt = find_fmt(NULL, NULL, f->index);
> +     if (!fmt)
> +             return -EINVAL;
> +
> +     strncpy(f->description, fmt->name, sizeof(f->description) - 1);

        strlcpy(f->description, fmt->name, sizeof(f->description));

> +     f->pixelformat = fmt->pixelformat;
> +
> +     return 0;
> +}
> +
> +u32 get_plane_size(struct gsc_frame *frame, unsigned int plane)
> +{
> +     if (!frame || plane>= frame->fmt->num_planes) {
> +             gsc_err("Invalid argument");
> +             return 0;
> +     }
> +
> +     return frame->payload[plane];
> +}
> +
> +u32 get_plane_info(struct gsc_frame frm, u32 addr, u32 *index)

Why don't you just pass a pointer to struct gsc_frame ?
It is so inefficient this way...

> +{
> +     if (frm.addr.y == addr) {
> +             *index = 0;
> +             return frm.addr.y;
> +     } else if (frm.addr.cb == addr) {
> +             *index = 1;
> +             return frm.addr.cb;
> +     } else if (frm.addr.cr == addr) {
> +             *index = 2;
> +             return frm.addr.cr;
> +     } else {
> +             gsc_err("Plane address is wrong");
> +             return -EINVAL;
> +     }
> +}
> +
> +void gsc_set_prefbuf(struct gsc_dev *gsc, struct gsc_frame frm)

Ditto.

> +{
> +     u32 f_chk_addr, f_chk_len, s_chk_addr, s_chk_len;
> +     f_chk_addr = f_chk_len = s_chk_addr = s_chk_len = 0;
> +
> +     f_chk_addr = frm.addr.y;
> +     f_chk_len = frm.payload[0];
> +     if (frm.fmt->num_planes == 2) {
> +             s_chk_addr = frm.addr.cb;
> +             s_chk_len = frm.payload[1];
> +     } else if (frm.fmt->num_planes == 3) {
> +             u32 low_addr, low_plane, mid_addr, mid_plane;
> +             u32 high_addr, high_plane;
> +             u32 t_min, t_max;
> +
> +             t_min = min3(frm.addr.y, frm.addr.cb, frm.addr.cr);
> +             low_addr = get_plane_info(frm, t_min,&low_plane);
> +             t_max = max3(frm.addr.y, frm.addr.cb, frm.addr.cr);
> +             high_addr = get_plane_info(frm, t_max,&high_plane);
> +
> +             mid_plane = 3 - (low_plane + high_plane);
> +             if (mid_plane == 0)
> +                     mid_addr = frm.addr.y;
> +             else if (mid_plane == 1)
> +                     mid_addr = frm.addr.cb;
> +             else if (mid_plane == 2)
> +                     mid_addr = frm.addr.cr;
> +             else
> +                     return;
> +
> +             f_chk_addr = low_addr;
> +             if (mid_addr + frm.payload[mid_plane] - low_addr>
> +                 high_addr + frm.payload[high_plane] - mid_addr) {
> +                     f_chk_len = frm.payload[low_plane];
> +                     s_chk_addr = mid_addr;
> +                     s_chk_len = high_addr +
> +                                     frm.payload[high_plane] - mid_addr;
> +             } else {
> +                     f_chk_len = mid_addr +
> +                                     frm.payload[mid_plane] - low_addr;
> +                     s_chk_addr = high_addr;
> +                     s_chk_len = frm.payload[high_plane];
> +             }
> +     }
> +     gsc_dbg("f_addr = 0x%08x, f_len = %d, s_addr = 0x%08x, s_len = %d\n",
> +             f_chk_addr, f_chk_len, s_chk_addr, s_chk_len);
> +}
> +
...
> +static int gsc_m2m_querycap(struct file *file, void *fh,
> +                        struct v4l2_capability *cap)
> +{
> +     struct gsc_ctx *ctx = fh_to_ctx(fh);
> +     struct gsc_dev *gsc = ctx->gsc_dev;
> +
> +     strncpy(cap->driver, gsc->pdev->name, sizeof(cap->driver) - 1);

strlcpy(cap->driver, gsc->pdev->name, sizeof(cap->driver));

> +     strncpy(cap->card, gsc->pdev->name, sizeof(cap->card) - 1);

strlcpy(cap->card, gsc->pdev->name, sizeof(cap->card));

> +     cap->bus_info[0] = 0;

strlcpy(cap->bus_info, "platform", sizeof(cap->bus_info));

> +     cap->capabilities = V4L2_CAP_STREAMING |
> +             V4L2_CAP_VIDEO_CAPTURE_MPLANE | V4L2_CAP_VIDEO_OUTPUT_MPLANE;

Need to set device_caps as well:

 cap->device_caps = V4L2_CAP_STREAMING | 
                    V4L2_CAP_VIDEO_CAPTURE_MPLANE | 
                    V4L2_CAP_VIDEO_OUTPUT_MPLANE;

 cap->capabilities = cap->device_caps | V4L2_CAP_DEVICE_CAPS;


Howewer this will probably need to be 

 cap->device_caps = V4L2_CAP_STREAMING | V4L2_CAP_VIDEO_M2M_MPLANE;

as per http://www.mail-archive.com/linux-media@vger.kernel.org/msg48498.html
by the time this patch is to be merged (v3.7 ?).

> +
> +     return 0;
> +}
> +
[...]
> +static int gsc_m2m_g_selection(struct file *file, void *fh,
> +                     struct v4l2_selection *s)
> +{
> +     struct gsc_frame *frame;
> +     struct gsc_ctx *ctx = fh_to_ctx(fh);
> +
> +     if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)&&
> +         (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +             return -EINVAL;
> +
> +     frame = ctx_get_frame(ctx, s->type);
> +     if (IS_ERR(frame))
> +             return PTR_ERR(frame);
> +
> +     switch (s->target) {
> +     case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +     case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +     case V4L2_SEL_TGT_CROP_BOUNDS:
> +     case V4L2_SEL_TGT_CROP_DEFAULT:
> +             s->r.left = 0;
> +             s->r.top = 0;
> +             s->r.width = frame->f_width;
> +             s->r.height = frame->f_height;
> +             return 0;
> +
> +     case V4L2_SEL_TGT_COMPOSE_ACTIVE:
> +     case V4L2_SEL_TGT_CROP_ACTIVE:

Please use V4L2_SEL_TGT_CROP/COMPOSE instead of V4L2_SEL_TGT_CROP/COMPOSE_ACTIVE
which is being phased out.

http://git.linuxtv.org/media_tree.git/commit/c133482300113b3b71fa4a1fd2118531e765b36a

> +             s->r.left = frame->crop.left;
> +             s->r.top = frame->crop.top;
> +             s->r.width = frame->crop.width;
> +             s->r.height = frame->crop.height;
> +             return 0;
> +     }
> +
> +     return -EINVAL;
> +}
> +
> +static int gsc_m2m_s_selection(struct file *file, void *fh,
> +                     struct v4l2_selection *s)
> +{
> +     struct gsc_frame *frame;
> +     struct gsc_ctx *ctx = fh_to_ctx(fh);
> +     struct v4l2_crop cr;
> +     struct gsc_variant *variant = ctx->gsc_dev->variant;
> +     int ret;
> +
> +     cr.type = s->type;
> +     cr.c = s->r;
> +
> +     if ((s->type != V4L2_BUF_TYPE_VIDEO_CAPTURE_MPLANE)&&
> +         (s->type != V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE))
> +             return -EINVAL;
> +
> +     ret = gsc_try_crop(ctx,&cr);
> +     if (ret)
> +             return ret;
> +
> +     if (s->flags&  V4L2_SEL_FLAG_LE&&
> +         !is_rectangle_enclosed(&cr.c,&s->r))
> +             return -ERANGE;
> +
> +     if (s->flags&  V4L2_SEL_FLAG_GE&&
> +         !is_rectangle_enclosed(&s->r,&cr.c))
> +             return -ERANGE;
> +
> +     s->r = cr.c;
> +
> +     switch (s->target) {
> +     case V4L2_SEL_TGT_COMPOSE_BOUNDS:
> +     case V4L2_SEL_TGT_COMPOSE_DEFAULT:
> +     case V4L2_SEL_TGT_COMPOSE_ACTIVE:

V4L2_SEL_TGT_COMPOSE_ACTIVE -> V4L2_SEL_TGT_COMPOSE

> +             frame =&ctx->s_frame;
> +             break;
> +
> +     case V4L2_SEL_TGT_CROP_BOUNDS:
> +     case V4L2_SEL_TGT_CROP_ACTIVE:

V4L2_SEL_TGT_CROP_ACTIVE -> V4L2_SEL_TGT_CROP

> +     case V4L2_SEL_TGT_CROP_DEFAULT:
> +             frame =&ctx->d_frame;
> +             break;
> +
> +     default:
> +             return -EINVAL;
> +     }
> +
> +     /* Check to see if scaling ratio is within supported range */
> +     if (gsc_ctx_state_is_set(GSC_DST_FMT | GSC_SRC_FMT, ctx)) {
> +             if (s->type == V4L2_BUF_TYPE_VIDEO_OUTPUT_MPLANE) {
> +                     ret = gsc_check_scaler_ratio(variant, cr.c.width,
> +                             cr.c.height, ctx->d_frame.crop.width,
> +                             ctx->d_frame.crop.height,
> +                             ctx->gsc_ctrls.rotate->val, ctx->out_path);
> +             } else {
> +                     ret = gsc_check_scaler_ratio(variant,
> +                             ctx->s_frame.crop.width,
> +                             ctx->s_frame.crop.height, cr.c.width,
> +                             cr.c.height, ctx->gsc_ctrls.rotate->val,
> +                             ctx->out_path);
> +             }
> +
> +             if (ret) {
> +                     gsc_err("Out of scaler range");
> +                     return -EINVAL;
> +             }
> +     }
> +
> +     frame->crop = cr.c;
> +
> +     gsc_ctx_state_lock_set(GSC_PARAMS, ctx);
> +     return 0;
> +}
[...]
> +static int gsc_m2m_open(struct file *file)
> +{
> +     struct gsc_dev *gsc = video_drvdata(file);
> +     struct gsc_ctx *ctx = NULL;
> +     int ret;
> +
> +     gsc_dbg("pid: %d, state: 0x%lx", task_pid_nr(current), gsc->state);
> +
> +     ctx = kzalloc(sizeof *ctx, GFP_KERNEL);
> +     if (!ctx)
> +             return -ENOMEM;
> +
> +     v4l2_fh_init(&ctx->fh, gsc->m2m.vfd);
> +     ret = gsc_ctrls_create(ctx);
> +     if (ret)
> +             goto error_fh;
> +
> +     /* Use separate control handler per file handle */
> +     ctx->fh.ctrl_handler =&ctx->ctrl_handler;
> +     file->private_data =&ctx->fh;
> +     v4l2_fh_add(&ctx->fh);
> +
> +     ctx->gsc_dev = gsc;
> +     /* Default color format */
> +     ctx->s_frame.fmt = get_format(0);
> +     ctx->d_frame.fmt = get_format(0);
> +     /* Setup the device context for mem2mem mode. */
> +     ctx->state |= GSC_CTX_M2M;
> +     ctx->flags = 0;
> +     ctx->in_path = GSC_DMA;
> +     ctx->out_path = GSC_DMA;
> +     spin_lock_init(&ctx->slock);
> +
> +     ctx->m2m_ctx = v4l2_m2m_ctx_init(gsc->m2m.m2m_dev, ctx, queue_init);
> +     if (IS_ERR(ctx->m2m_ctx)) {
> +             gsc_err("Failed to initialize m2m context");
> +             ret = PTR_ERR(ctx->m2m_ctx);
> +             goto error_fh;
> +     }
> +
> +     if (gsc->m2m.refcnt++ == 0)
> +             set_bit(ST_M2M_OPEN,&gsc->state);
> +
> +     gsc_dbg("gsc m2m driver is opened, ctx(0x%p)", ctx);
> +     return 0;
> +
> +error_fh:
> +     v4l2_fh_del(&ctx->fh);
> +     v4l2_fh_exit(&ctx->fh);
> +     kfree(ctx);
> +     return ret;
> +}
> +
> +static int gsc_m2m_release(struct file *file)
> +{
> +     struct gsc_ctx *ctx = fh_to_ctx(file->private_data);
> +     struct gsc_dev *gsc = ctx->gsc_dev;
> +
> +     gsc_dbg("pid: %d, state: 0x%lx, refcnt= %d",
> +             task_pid_nr(current), gsc->state, gsc->m2m.refcnt);
> +
> +     v4l2_m2m_ctx_release(ctx->m2m_ctx);
> +     gsc_ctrls_delete(ctx);
> +     v4l2_fh_del(&ctx->fh);
> +     v4l2_fh_exit(&ctx->fh);
> +
> +     if (--gsc->m2m.refcnt<= 0)
> +             clear_bit(ST_M2M_OPEN,&gsc->state);
> +     kfree(ctx);
> +     return 0;
> +}
> +
> +static unsigned int gsc_m2m_poll(struct file *file,
> +                                  struct poll_table_struct *wait)
> +{
> +     struct gsc_ctx *ctx = fh_to_ctx(file->private_data);
> +
> +     return v4l2_m2m_poll(file, ctx->m2m_ctx, wait);
> +}
> +
> +static int gsc_m2m_mmap(struct file *file, struct vm_area_struct *vma)
> +{
> +     struct gsc_ctx *ctx = fh_to_ctx(file->private_data);
> +
> +     return v4l2_m2m_mmap(file, ctx->m2m_ctx, vma);
> +}
> +static const struct v4l2_file_operations gsc_m2m_fops = {
> +     .owner          = THIS_MODULE,
> +     .open           = gsc_m2m_open,
> +     .release        = gsc_m2m_release,
> +     .poll           = gsc_m2m_poll,
> +     .unlocked_ioctl = video_ioctl2,
> +     .mmap           = gsc_m2m_mmap,

There was a change recently in the v4l2 core modules and only 
.unlocked_ioctl is serialized with the core video device mutex.
All other file operations must be serialized explicitly in drivers.

For more details see:
http://git.infradead.org/users/kmpark/linux-samsung/commit/66ba3641dbc122c9c82f03aed9320f087730dd8f

[...]
> diff --git a/drivers/media/video/exynos/gsc/gsc-regs.c 
> b/drivers/media/video/exynos/gsc/gsc-regs.c
> new file mode 100644
> index 0000000..7c5cce2
> --- /dev/null
> +++ b/drivers/media/video/exynos/gsc/gsc-regs.c
> @@ -0,0 +1,579 @@
> +/* linux/drivers/media/video/exynos/gsc/gsc-regs.c
> + *
> + * Copyright (c) 2011 Samsung Electronics Co., Ltd.
> + *           http://www.samsung.com
> + *
> + * Samsung EXYNOS5 SoC series G-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/io.h>
> +#include<linux/delay.h>
> +#include<mach/map.h>
> +#include "gsc-core.h"
> +
> +void gsc_hw_set_sw_reset(struct gsc_dev *dev)
> +{
> +     u32 cfg = 0;
> +
> +     cfg |= GSC_SW_RESET_SRESET;
> +     writel(cfg, dev->regs + GSC_SW_RESET);
> +}

Probably worth to make it an inline function and move it to some 
header, e.g.

+static inline void gsc_hw_set_sw_reset(struct gsc_dev *dev)
+{
+       writel(GSC_SW_RESET_SRESET, dev->regs + GSC_SW_RESET);
+}

--

Thanks,
Sylwester
--
To unsubscribe from this list: send the line "unsubscribe linux-media" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to