RE: [PATCH 5/6 v5] V4L/DVB: s5p-fimc: Add camera capture support
Latest your reply is easy to understand.
And I send you another parts review comments.
Tuesday, October 12, 2010 2:27, Sylwester Nawrocki wrote :
>
> Add a video device driver per each FIMC entity to support
> the camera capture input mode. Video capture node is registered
> only if CCD sensor data is provided through driver's platfrom data
> and board setup code.
>
> Signed-off-by: Sylwester Nawrocki <s.nawrocki@xxxxxxxxxxx>
> Signed-off-by: Kyungmin Park <kyungmin.park@xxxxxxxxxxx>
> Reviewed-by: Marek Szyprowski <m.szyprowski@xxxxxxxxxxx>
> ---
> drivers/media/video/s5p-fimc/Makefile | 2 +-
> drivers/media/video/s5p-fimc/fimc-capture.c | 819
> +++++++++++++++++++++++++++
> drivers/media/video/s5p-fimc/fimc-core.c | 563 +++++++++++++------
> drivers/media/video/s5p-fimc/fimc-core.h | 205 +++++++-
> drivers/media/video/s5p-fimc/fimc-reg.c | 173 +++++-
> include/media/s3c_fimc.h | 60 ++
> 6 files changed, 1630 insertions(+), 192 deletions(-)
> create mode 100644 drivers/media/video/s5p-fimc/fimc-capture.c
> create mode 100644 include/media/s3c_fimc.h
>
> diff --git a/drivers/media/video/s5p-fimc/Makefile
> b/drivers/media/video/s5p-fimc/Makefile
> index 0d9d541..7ea1b14 100644
> --- a/drivers/media/video/s5p-fimc/Makefile
> +++ b/drivers/media/video/s5p-fimc/Makefile
> @@ -1,3 +1,3 @@
>
> obj-$(CONFIG_VIDEO_SAMSUNG_S5P_FIMC) := s5p-fimc.o
> -s5p-fimc-y := fimc-core.o fimc-reg.o
> +s5p-fimc-y := fimc-core.o fimc-reg.o fimc-capture.o
> diff --git a/drivers/media/video/s5p-fimc/fimc-capture.c
> b/drivers/media/video/s5p-fimc/fimc-capture.c
> new file mode 100644
> index 0000000..e8f13d3
> --- /dev/null
> +++ b/drivers/media/video/s5p-fimc/fimc-capture.c
> @@ -0,0 +1,819 @@
> +/*
> + * Samsung S5P SoC series camera interface (camera capture) driver
> + *
> + * Copyright (c) 2010 Samsung Electronics Co., Ltd
> + * Author: Sylwester Nawrocki, <s.nawrocki@xxxxxxxxxxx>
> + *
> + * This program is free software; you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License version 2 as
> + * published by the Free Software Foundation.
> + */
> +
> +#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/device.h>
> +#include <linux/platform_device.h>
> +#include <linux/list.h>
> +#include <linux/slab.h>
> +#include <linux/clk.h>
> +#include <linux/i2c.h>
> +
> +#include <linux/videodev2.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-mem2mem.h>
> +#include <media/videobuf-core.h>
> +#include <media/videobuf-dma-contig.h>
> +
> +#include "fimc-core.h"
> +
> +static struct v4l2_subdev *fimc_subdev_register(struct fimc_dev *fimc,
> + struct s3c_fimc_isp_info *isp_info)
> +{
> + struct i2c_adapter *i2c_adap;
> + struct fimc_vid_cap *vid_cap = &fimc->vid_cap;
> + struct v4l2_subdev *sd = NULL;
> +
> + i2c_adap = i2c_get_adapter(isp_info->i2c_bus_num);
> + if (!i2c_adap)
> + return ERR_PTR(-ENOMEM);
> +
> + sd = v4l2_i2c_new_subdev_board(&vid_cap->v4l2_dev, i2c_adap,
> + MODULE_NAME, isp_info->board_info, NULL);
> + if (!sd) {
> + v4l2_err(&vid_cap->v4l2_dev, "failed to acquire subdev\n");
> + return NULL;
> + }
> +
> + v4l2_info(&vid_cap->v4l2_dev, "subdevice %s registered
> successfuly\n",
> + isp_info->board_info->type);
> +
> + return sd;
> +}
> +
> +static void fimc_subdev_unregister(struct fimc_dev *fimc)
> +{
> + struct fimc_vid_cap *vid_cap = &fimc->vid_cap;
> + struct i2c_client *client;
> +
> + if (vid_cap->input_index < 0)
> + return; /* Subdevice already released or not registered.
> */
> +
> + if (vid_cap->sd) {
> + v4l2_device_unregister_subdev(vid_cap->sd);
> + client = v4l2_get_subdevdata(vid_cap->sd);
> + i2c_unregister_device(client);
> + i2c_put_adapter(client->adapter);
> + vid_cap->sd = NULL;
> + }
> +
> + vid_cap->input_index = -1;
> +}
(snip)
> +static int fimc_cap_s_fmt(struct file *file, void *priv,
> + struct v4l2_format *f)
> +{
> + struct fimc_ctx *ctx = priv;
> + struct fimc_dev *fimc = ctx->fimc_dev;
> + struct fimc_frame *frame;
> + struct v4l2_pix_format *pix;
> + int ret;
> +
> + if (f->type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> +
> + ret = fimc_vidioc_try_fmt(file, priv, f);
> + if (ret)
> + return ret;
> +
> + if (mutex_lock_interruptible(&fimc->lock))
> + return -ERESTARTSYS;
> +
> + if (fimc_capture_active(fimc)) {
> + ret = -EBUSY;
> + goto sf_unlock;
> + }
I suggest to use vb_lock on here.
You already use vb_lock "fimc_m2m_s_fmt" function in fimc-core.c code
-- sample --
struct fimc_capture_device *cap = &ctx->fimc_dev->vid_cap;
mutex_lock(&cap->vbq->vb->lock);
> +
> + frame = &ctx->d_frame;
> +
> + pix = &f->fmt.pix;
> + frame->fmt = find_format(f, FMT_FLAGS_M2M | FMT_FLAGS_CAM);
> + if (!frame->fmt) {
> + err("fimc target format not found\n");
> + ret = -EINVAL;
> + goto sf_unlock;
> + }
> +
> + /* Output DMA frame pixel size and offsets. */
> + frame->f_width = pix->bytesperline * 8 / frame->fmt->depth;
> + frame->f_height = pix->height;
> + frame->width = pix->width;
> + frame->height = pix->height;
> + frame->o_width = pix->width;
> + frame->o_height = pix->height;
> + frame->size = (pix->width * pix->height * frame->fmt->depth) >> 3;
> + frame->offs_h = 0;
> + frame->offs_v = 0;
> +
> + ret = sync_capture_fmt(ctx);
> +
> + ctx->state |= (FIMC_PARAMS | FIMC_DST_FMT);
> +
> +sf_unlock:
> + mutex_unlock(&fimc->lock);
> + return ret;
> +}
(snip)
> -static int fimc_prepare_config(struct fimc_ctx *ctx, u32 flags)
> +int fimc_prepare_config(struct fimc_ctx *ctx, u32 flags)
> {
> struct fimc_frame *s_frame, *d_frame;
> struct fimc_vid_buffer *buf = NULL;
> @@ -513,9 +585,9 @@ static void fimc_dma_run(void *priv)
> if (ctx->state & FIMC_PARAMS)
> fimc_hw_set_out_dma(ctx);
>
> - ctx->state = 0;
> fimc_activate_capture(ctx);
>
> + ctx->state &= (FIMC_CTX_M2M | FIMC_CTX_CAP);
> fimc_hw_activate_input_dma(fimc, true);
>
> dma_unlock:
> @@ -598,10 +670,31 @@ static void fimc_buf_queue(struct videobuf_queue *vq,
> struct videobuf_buffer *vb)
> {
> struct fimc_ctx *ctx = vq->priv_data;
> - v4l2_m2m_buf_queue(ctx->m2m_ctx, vq, vb);
> + struct fimc_dev *fimc = ctx->fimc_dev;
> + struct fimc_vid_cap *cap = &fimc->vid_cap;
> + unsigned long flags;
> +
> + dbg("ctx: %p, ctx->state: 0x%x", ctx, ctx->state);
> +
> + if ((ctx->state & FIMC_CTX_M2M) && ctx->m2m_ctx) {
> + v4l2_m2m_buf_queue(ctx->m2m_ctx, vq, vb);
> + } else if (ctx->state & FIMC_CTX_CAP) {
> + spin_lock_irqsave(&fimc->slock, flags);
> + fimc_vid_cap_buf_queue(fimc, (struct fimc_vid_buffer *)vb);
> +
> + dbg("fimc->cap.active_buf_cnt: %d",
> + fimc->vid_cap.active_buf_cnt);
> +
> + if (cap->active_buf_cnt >= cap->reqbufs_count ||
> + cap->active_buf_cnt >= FIMC_MAX_OUT_BUFS) {
1. purpose of queues
In my understand through your code,
The qbuf() which call before streamon() is pushed done_list in videobuf
framework to use dqbuf().
During streamon(), Number of FIMC h/w output DMA allocated buffers have pushed
in active_queue(normally 4) and rest allocated buffers have pushed to pending_queue.
It means that, active_queue is connection to FIMC h/w for output DMA.
(maximum 4 in s5pc110, 32 in s5pc210)
pending_queue is available buffer list or returned buffer list from user.
Please let me know if I have misunderstanding.
2. Which case of condition, active_buf_cnt greater than reqbufs_count?
In preview mode normally, reqbufs_count are 4 or more.
Likewise, which case of condition, active_buf_cnt greater than FIMC_MAX_OUT_BUFS(4)?
> + if (!test_and_set_bit(ST_CAPT_STREAM, &fimc->state))
> + fimc_activate_capture(ctx);
> + }
Condition which fimc_deactivate_capture() function run is
"cap->active_buf_cnt <= 1".
Then I think condition which fimc_activate_capture() function run is
"cap->active_buf_cnt >= 2".
But, your code wait 4 or more.
> + spin_unlock_irqrestore(&fimc->slock, flags);
> + }
> }
>
> -static struct videobuf_queue_ops fimc_qops = {
> +struct videobuf_queue_ops fimc_qops = {
> .buf_setup = fimc_buf_setup,
> .buf_prepare = fimc_buf_prepare,
> .buf_queue = fimc_buf_queue,
> @@ -624,7 +717,7 @@ static int fimc_m2m_querycap(struct file *file, void
> *priv,
> return 0;
> }
>
(snip)
> +
> +
> +#define FIMC_MAX_CAMIF_CLIENTS 2
> +
> +/**
> + * struct s3c_platform_fimc - camera host interface platform data
> + *
> + * @isp_info: properties of camera sensor required for host interface
> setup
> + */
> +struct s3c_platform_fimc {
> + struct s3c_fimc_isp_info *isp_info[FIMC_MAX_CAMIF_CLIENTS];
> +};
> +#endif /* S3C_FIMC_H_ */
> --
> 1.7.3.1
>
> --
> 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
[Linux Input]
[Video for Linux]
[Mplayer Users]
[Linux USB Devel]
[Linux Audio Users]
[Photos]
[Yosemite Photos]
[Linux Kernel]
[Linux SCSI]
[XFree86]
[Devices]
[Yosemite Backpacking]
[Linux Input]
