* [PATCH] viafb camera controller driver
@ 2010-10-10 22:23 Jonathan Corbet
2010-10-11 12:09 ` Mauro Carvalho Chehab
` (2 more replies)
0 siblings, 3 replies; 20+ messages in thread
From: Jonathan Corbet @ 2010-10-10 22:23 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: linux-media, Florian Tobias Schandinat, Laurent Pinchart,
Daniel Drake
Howdy, all,
Well, that took a whole lot longer than I had hoped...but, attached, is a
new version of the viafb camera driver patch, done against 2.6.36-rc7.
I've tried to address most of Laurent's comments from back in June; in
particular, I have:
- Gotten rid of the static device structure
- Fixed some locking glitches
- Fixed a bit of device initialization silliness.
One thing I have *not* done is to push locking down into the ov7670
driver. That would be a good thing to do at some point, but playing with
that driver was beyond the scope of what I was trying to do here.
This driver will still need some OLPC bits to work properly, but Daniel is
working on that. This version of the driver does work on XO-1.5 systems,
modulo some 2.6.36 API changes.
Mauro, any chance of putting this in the queue for 2.6.37? Yes, I know
it's really late, my apologies for that.
Thanks,
jon
---
Add a driver for the video capture port on VIA integrated chipsets. This
version has a remaining OLPCism or two and expects to be talking to an
ov7670; those can be improved as the need arises.
This work was supported by the One Laptop Per Child project.
Thanks to Laurent Pinchart for a number of useful comments.
Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
drivers/media/video/Kconfig | 10 +
drivers/media/video/Makefile | 2 +
drivers/media/video/via-camera.c | 1406 ++++++++++++++++++++++++++++++++++++++
drivers/media/video/via-camera.h | 93 +++
drivers/video/via/accel.c | 2 +-
drivers/video/via/via-core.c | 16 +-
include/linux/via-core.h | 4 +-
include/media/v4l2-chip-ident.h | 4 +
8 files changed, 1533 insertions(+), 4 deletions(-)
create mode 100644 drivers/media/video/via-camera.c
create mode 100644 drivers/media/video/via-camera.h
diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
index f6e4d04..b62d6b0 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -774,6 +774,16 @@ config VIDEO_CAFE_CCIC
CMOS camera controller. This is the controller found on first-
generation OLPC systems.
+config VIDEO_VIA_CAMERA
+ tristate "VIAFB camera controller support"
+ depends on FB_VIA
+ select VIDEOBUF_DMA_SG
+ select VIDEO_OV7670
+ help
+ Driver support for the integrated camera controller in VIA
+ Chrome9 chipsets. Currently only tested on OLPC xo-1.5 systems
+ with ov7670 sensors.
+
config SOC_CAMERA
tristate "SoC camera support"
depends on VIDEO_V4L2 && HAS_DMA && I2C
diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
index 40f98fb..b592804 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -125,6 +125,8 @@ obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o
+obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
+
obj-$(CONFIG_USB_DABUSB) += dabusb.o
obj-$(CONFIG_USB_SE401) += se401.o
obj-$(CONFIG_USB_ZR364XX) += zr364xx.o
diff --git a/drivers/media/video/via-camera.c b/drivers/media/video/via-camera.c
new file mode 100644
index 0000000..8e970a7
--- /dev/null
+++ b/drivers/media/video/via-camera.c
@@ -0,0 +1,1406 @@
+/*
+ * Driver for the VIA Chrome integrated camera controller.
+ *
+ * Copyright 2009,2010 Jonathan Corbet <corbet@lwn.net>
+ * Distributable under the terms of the GNU General Public License, version 2
+ *
+ * This work was supported by the One Laptop Per Child project
+ */
+#include <linux/kernel.h>
+#include <linux/module.h>
+#include <linux/device.h>
+#include <linux/list.h>
+#include <linux/pci.h>
+#include <linux/gpio.h>
+#include <linux/interrupt.h>
+#include <linux/pci.h>
+#include <linux/platform_device.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/videobuf-dma-sg.h>
+#include <linux/device.h>
+#include <linux/delay.h>
+#include <linux/dma-mapping.h>
+#include <linux/pm_qos_params.h>
+#include <linux/via-core.h>
+#include <linux/via-gpio.h>
+#include <linux/via_i2c.h>
+
+#include "via-camera.h"
+
+MODULE_AUTHOR("Jonathan Corbet <corbet@lwn.net>");
+MODULE_DESCRIPTION("VIA framebuffer-based camera controller driver");
+MODULE_LICENSE("GPL");
+
+static int flip_image;
+module_param(flip_image, bool, 0444);
+MODULE_PARM_DESC(flip_image,
+ "If set, the sensor will be instructed to flip the image "
+ "vertically.");
+
+#ifdef CONFIG_OLPC_XO_1_5
+static int override_serial;
+module_param(override_serial, bool, 0444);
+MODULE_PARM_DESC(override_serial,
+ "The camera driver will normally refuse to load if "
+ "the XO 1.5 serial port is enabled. Set this option "
+ "to force the issue.");
+#endif
+
+/*
+ * Basic window sizes.
+ */
+#define VGA_WIDTH 640
+#define VGA_HEIGHT 480
+#define QCIF_WIDTH 176
+#define QCIF_HEIGHT 144
+
+/*
+ * The structure describing our camera.
+ */
+enum viacam_opstate { S_IDLE = 0, S_RUNNING = 1 };
+
+struct via_camera {
+ struct v4l2_device v4l2_dev;
+ struct video_device vdev;
+ struct v4l2_subdev *sensor;
+ struct platform_device *platdev;
+ struct viafb_dev *viadev;
+ struct mutex lock;
+ enum viacam_opstate opstate;
+ unsigned long flags;
+ struct pm_qos_request_list qos_request;
+ /*
+ * GPIO info for power/reset management
+ */
+ int power_gpio;
+ int reset_gpio;
+ /*
+ * I/O memory stuff.
+ */
+ void __iomem *mmio; /* Where the registers live */
+ void __iomem *fbmem; /* Frame buffer memory */
+ u32 fb_offset; /* Reserved memory offset (FB) */
+ /*
+ * Capture buffers and related. The controller supports
+ * up to three, so that's what we have here. These buffers
+ * live in frame buffer memory, so we don't call them "DMA".
+ */
+ unsigned int cb_offsets[3]; /* offsets into fb mem */
+ u8 *cb_addrs[3]; /* Kernel-space addresses */
+ int n_cap_bufs; /* How many are we using? */
+ int next_buf;
+ struct videobuf_queue vb_queue;
+ struct list_head buffer_queue; /* prot. by reg_lock */
+ /*
+ * User tracking.
+ */
+ int users;
+ struct file *owner;
+ /*
+ * Video format information. sensor_format is kept in a form
+ * that we can use to pass to the sensor. We always run the
+ * sensor in VGA resolution, though, and let the controller
+ * downscale things if need be. So we keep the "real*
+ * dimensions separately.
+ */
+ struct v4l2_pix_format sensor_format;
+ struct v4l2_pix_format user_format;
+};
+
+/*
+ * Yes, this is a hack, but there's only going to be one of these
+ * on any system we know of.
+ */
+static struct via_camera *via_cam_info;
+
+/*
+ * Flag values, manipulated with bitops
+ */
+#define CF_DMA_ACTIVE 0 /* A frame is incoming */
+#define CF_CONFIG_NEEDED 1 /* Must configure hardware */
+
+
+/*
+ * Nasty ugly v4l2 boilerplate.
+ */
+#define sensor_call(cam, optype, func, args...) \
+ v4l2_subdev_call(cam->sensor, optype, func, ##args)
+
+/*
+ * Debugging and related.
+ */
+#define cam_err(cam, fmt, arg...) \
+ dev_err(&(cam)->platdev->dev, fmt, ##arg);
+#define cam_warn(cam, fmt, arg...) \
+ dev_warn(&(cam)->platdev->dev, fmt, ##arg);
+#define cam_dbg(cam, fmt, arg...) \
+ dev_dbg(&(cam)->platdev->dev, fmt, ##arg);
+
+
+/*--------------------------------------------------------------------------*/
+/*
+ * Sensor power/reset management. This piece is OLPC-specific for
+ * sure; other configurations will have things connected differently.
+ */
+static int via_sensor_power_setup(struct via_camera *cam)
+{
+ int ret;
+
+ cam->power_gpio = viafb_gpio_lookup("VGPIO3");
+ cam->reset_gpio = viafb_gpio_lookup("VGPIO2");
+ if (cam->power_gpio < 0 || cam->reset_gpio < 0) {
+ dev_err(&cam->platdev->dev, "Unable to find GPIO lines\n");
+ return -EINVAL;
+ }
+ ret = gpio_request(cam->power_gpio, "viafb-camera");
+ if (ret) {
+ dev_err(&cam->platdev->dev, "Unable to request power GPIO\n");
+ return ret;
+ }
+ ret = gpio_request(cam->reset_gpio, "viafb-camera");
+ if (ret) {
+ dev_err(&cam->platdev->dev, "Unable to request reset GPIO\n");
+ gpio_free(cam->power_gpio);
+ return ret;
+ }
+ gpio_direction_output(cam->power_gpio, 0);
+ gpio_direction_output(cam->reset_gpio, 0);
+ return 0;
+}
+
+/*
+ * Power up the sensor and perform the reset dance.
+ */
+static void via_sensor_power_up(struct via_camera *cam)
+{
+ gpio_set_value(cam->power_gpio, 1);
+ gpio_set_value(cam->reset_gpio, 0);
+ msleep(20); /* Probably excessive */
+ gpio_set_value(cam->reset_gpio, 1);
+ msleep(20);
+}
+
+static void via_sensor_power_down(struct via_camera *cam)
+{
+ gpio_set_value(cam->power_gpio, 0);
+ gpio_set_value(cam->reset_gpio, 0);
+}
+
+
+static void via_sensor_power_release(struct via_camera *cam)
+{
+ via_sensor_power_down(cam);
+ gpio_free(cam->power_gpio);
+ gpio_free(cam->reset_gpio);
+}
+
+/* --------------------------------------------------------------------------*/
+/* Sensor ops */
+
+/*
+ * Manage the ov7670 "flip" bit, which needs special help.
+ */
+static int viacam_set_flip(struct via_camera *cam)
+{
+ struct v4l2_control ctrl;
+
+ memset(&ctrl, 0, sizeof(ctrl));
+ ctrl.id = V4L2_CID_VFLIP;
+ ctrl.value = flip_image;
+ return sensor_call(cam, core, s_ctrl, &ctrl);
+}
+
+/*
+ * Configure the sensor. It's up to the caller to ensure
+ * that the camera is in the correct operating state.
+ */
+static int viacam_configure_sensor(struct via_camera *cam)
+{
+ struct v4l2_format fmt;
+ int ret;
+
+ fmt.fmt.pix = cam->sensor_format;
+ ret = sensor_call(cam, core, init, 0);
+ if (ret == 0)
+ ret = sensor_call(cam, video, s_fmt, &fmt);
+ /*
+ * OV7670 does weird things if flip is set *before* format...
+ */
+ if (ret == 0)
+ ret = viacam_set_flip(cam);
+ return ret;
+}
+
+
+
+/* --------------------------------------------------------------------------*/
+/*
+ * Some simple register accessors; they assume that the lock is held.
+ *
+ * Should we want to support the second capture engine, we could
+ * hide the register difference by adding 0x1000 to registers in the
+ * 0x300-350 range.
+ */
+static inline void viacam_write_reg(struct via_camera *cam,
+ int reg, int value)
+{
+ iowrite32(value, cam->mmio + reg);
+}
+
+static inline int viacam_read_reg(struct via_camera *cam, int reg)
+{
+ return ioread32(cam->mmio + reg);
+}
+
+static inline void viacam_write_reg_mask(struct via_camera *cam,
+ int reg, int value, int mask)
+{
+ int tmp = viacam_read_reg(cam, reg);
+
+ tmp = (tmp & ~mask) | (value & mask);
+ viacam_write_reg(cam, reg, tmp);
+}
+
+
+/* --------------------------------------------------------------------------*/
+/* Interrupt management and handling */
+
+static irqreturn_t viacam_quick_irq(int irq, void *data)
+{
+ struct via_camera *cam = data;
+ irqreturn_t ret = IRQ_NONE;
+ int icv;
+
+ /*
+ * All we do here is to clear the interrupts and tell
+ * the handler thread to wake up.
+ */
+ spin_lock(&cam->viadev->reg_lock);
+ icv = viacam_read_reg(cam, VCR_INTCTRL);
+ if (icv & VCR_IC_EAV) {
+ icv |= VCR_IC_EAV|VCR_IC_EVBI|VCR_IC_FFULL;
+ viacam_write_reg(cam, VCR_INTCTRL, icv);
+ ret = IRQ_WAKE_THREAD;
+ }
+ spin_unlock(&cam->viadev->reg_lock);
+ return ret;
+}
+
+/*
+ * Find the next videobuf buffer which has somebody waiting on it.
+ */
+static struct videobuf_buffer *viacam_next_buffer(struct via_camera *cam)
+{
+ unsigned long flags;
+ struct videobuf_buffer *buf = NULL;
+
+ spin_lock_irqsave(&cam->viadev->reg_lock, flags);
+ if (cam->opstate != S_RUNNING)
+ goto out;
+ if (list_empty(&cam->buffer_queue))
+ goto out;
+ buf = list_entry(cam->buffer_queue.next, struct videobuf_buffer, queue);
+ if (!waitqueue_active(&buf->done)) {/* Nobody waiting */
+ buf = NULL;
+ goto out;
+ }
+ list_del(&buf->queue);
+ buf->state = VIDEOBUF_ACTIVE;
+out:
+ spin_unlock_irqrestore(&cam->viadev->reg_lock, flags);
+ return buf;
+}
+
+/*
+ * The threaded IRQ handler.
+ */
+static irqreturn_t viacam_irq(int irq, void *data)
+{
+ int bufn;
+ struct videobuf_buffer *vb;
+ struct via_camera *cam = data;
+ struct videobuf_dmabuf *vdma;
+
+ /*
+ * If there is no place to put the data frame, don't bother
+ * with anything else.
+ */
+ vb = viacam_next_buffer(cam);
+ if (vb == NULL)
+ goto done;
+ /*
+ * Figure out which buffer we just completed.
+ */
+ bufn = (viacam_read_reg(cam, VCR_INTCTRL) & VCR_IC_ACTBUF) >> 3;
+ bufn -= 1;
+ if (bufn < 0)
+ bufn = cam->n_cap_bufs - 1;
+ /*
+ * Copy over the data and let any waiters know.
+ */
+ vdma = videobuf_to_dma(vb);
+ viafb_dma_copy_out_sg(cam->cb_offsets[bufn], vdma->sglist, vdma->sglen);
+ vb->state = VIDEOBUF_DONE;
+ vb->size = cam->user_format.sizeimage;
+ wake_up(&vb->done);
+done:
+ return IRQ_HANDLED;
+}
+
+
+/*
+ * These functions must mess around with the general interrupt
+ * control register, which is relevant to much more than just the
+ * camera. Nothing else uses interrupts, though, as of this writing.
+ * Should that situation change, we'll have to improve support at
+ * the via-core level.
+ */
+static void viacam_int_enable(struct via_camera *cam)
+{
+ viacam_write_reg(cam, VCR_INTCTRL,
+ VCR_IC_INTEN|VCR_IC_EAV|VCR_IC_EVBI|VCR_IC_FFULL);
+ viafb_irq_enable(VDE_I_C0AVEN);
+}
+
+static void viacam_int_disable(struct via_camera *cam)
+{
+ viafb_irq_disable(VDE_I_C0AVEN);
+ viacam_write_reg(cam, VCR_INTCTRL, 0);
+}
+
+
+
+/* --------------------------------------------------------------------------*/
+/* Controller operations */
+
+/*
+ * Set up our capture buffers in framebuffer memory.
+ */
+static int viacam_ctlr_cbufs(struct via_camera *cam)
+{
+ int nbuf = cam->viadev->camera_fbmem_size/cam->sensor_format.sizeimage;
+ int i;
+ unsigned int offset;
+
+ /*
+ * See how many buffers we can work with.
+ */
+ if (nbuf >= 3) {
+ cam->n_cap_bufs = 3;
+ viacam_write_reg_mask(cam, VCR_CAPINTC, VCR_CI_3BUFS,
+ VCR_CI_3BUFS);
+ } else if (nbuf == 2) {
+ cam->n_cap_bufs = 2;
+ viacam_write_reg_mask(cam, VCR_CAPINTC, 0, VCR_CI_3BUFS);
+ } else {
+ cam_warn(cam, "Insufficient frame buffer memory\n");
+ return -ENOMEM;
+ }
+ /*
+ * Set them up.
+ */
+ offset = cam->fb_offset;
+ for (i = 0; i < cam->n_cap_bufs; i++) {
+ cam->cb_offsets[i] = offset;
+ cam->cb_addrs[i] = cam->fbmem + offset;
+ viacam_write_reg(cam, VCR_VBUF1 + i*4, offset & VCR_VBUF_MASK);
+ offset += cam->sensor_format.sizeimage;
+ }
+ return 0;
+}
+
+/*
+ * Set the scaling register for downscaling the image.
+ *
+ * This register works like this... Vertical scaling is enabled
+ * by bit 26; if that bit is set, downscaling is controlled by the
+ * value in bits 16:25. Those bits are divided by 1024 to get
+ * the scaling factor; setting just bit 25 thus cuts the height
+ * in half.
+ *
+ * Horizontal scaling works about the same, but it's enabled by
+ * bit 11, with bits 0:10 giving the numerator of a fraction
+ * (over 2048) for the scaling value.
+ *
+ * This function is naive in that, if the user departs from
+ * the 3x4 VGA scaling factor, the image will distort. We
+ * could work around that if it really seemed important.
+ */
+static void viacam_set_scale(struct via_camera *cam)
+{
+ unsigned int avscale;
+ int sf;
+
+ if (cam->user_format.width == VGA_WIDTH)
+ avscale = 0;
+ else {
+ sf = (cam->user_format.width*2048)/VGA_WIDTH;
+ avscale = VCR_AVS_HEN | sf;
+ }
+ if (cam->user_format.height < VGA_HEIGHT) {
+ sf = (1024*cam->user_format.height)/VGA_HEIGHT;
+ avscale |= VCR_AVS_VEN | (sf << 16);
+ }
+ viacam_write_reg(cam, VCR_AVSCALE, avscale);
+}
+
+
+/*
+ * Configure image-related information into the capture engine.
+ */
+static void viacam_ctlr_image(struct via_camera *cam)
+{
+ int cicreg;
+
+ /*
+ * Disable clock before messing with stuff - from the via
+ * sample driver.
+ */
+ viacam_write_reg(cam, VCR_CAPINTC, ~(VCR_CI_ENABLE|VCR_CI_CLKEN));
+ /*
+ * Set up the controller for VGA resolution, modulo magic
+ * offsets from the via sample driver.
+ */
+ viacam_write_reg(cam, VCR_HORRANGE, 0x06200120);
+ viacam_write_reg(cam, VCR_VERTRANGE, 0x01de0000);
+ viacam_set_scale(cam);
+ /*
+ * Image size info.
+ */
+ viacam_write_reg(cam, VCR_MAXDATA,
+ (cam->sensor_format.height << 16) |
+ (cam->sensor_format.bytesperline >> 3));
+ viacam_write_reg(cam, VCR_MAXVBI, 0);
+ viacam_write_reg(cam, VCR_VSTRIDE,
+ cam->user_format.bytesperline & VCR_VS_STRIDE);
+ /*
+ * Set up the capture interface control register,
+ * everything but the "go" bit.
+ *
+ * The FIFO threshold is a bit of a magic number; 8 is what
+ * VIA's sample code uses.
+ */
+ cicreg = VCR_CI_CLKEN |
+ 0x08000000 | /* FIFO threshold */
+ VCR_CI_FLDINV | /* OLPC-specific? */
+ VCR_CI_VREFINV | /* OLPC-specific? */
+ VCR_CI_DIBOTH | /* Capture both fields */
+ VCR_CI_CCIR601_8;
+ if (cam->n_cap_bufs == 3)
+ cicreg |= VCR_CI_3BUFS;
+ /*
+ * YUV formats need different byte swapping than RGB.
+ */
+ if (cam->user_format.pixelformat == V4L2_PIX_FMT_YUYV)
+ cicreg |= VCR_CI_YUYV;
+ else
+ cicreg |= VCR_CI_UYVY;
+ viacam_write_reg(cam, VCR_CAPINTC, cicreg);
+}
+
+
+static int viacam_config_controller(struct via_camera *cam)
+{
+ int ret;
+ unsigned long flags;
+
+ spin_lock_irqsave(&cam->viadev->reg_lock, flags);
+ ret = viacam_ctlr_cbufs(cam);
+ if (!ret)
+ viacam_ctlr_image(cam);
+ spin_unlock_irqrestore(&cam->viadev->reg_lock, flags);
+ clear_bit(CF_CONFIG_NEEDED, &cam->flags);
+ return ret;
+}
+
+/*
+ * Make it start grabbing data.
+ */
+static void viacam_start_engine(struct via_camera *cam)
+{
+ spin_lock_irq(&cam->viadev->reg_lock);
+ cam->next_buf = 0;
+ viacam_write_reg_mask(cam, VCR_CAPINTC, VCR_CI_ENABLE, VCR_CI_ENABLE);
+ viacam_int_enable(cam);
+ (void) viacam_read_reg(cam, VCR_CAPINTC); /* Force post */
+ cam->opstate = S_RUNNING;
+ spin_unlock_irq(&cam->viadev->reg_lock);
+}
+
+
+static void viacam_stop_engine(struct via_camera *cam)
+{
+ spin_lock_irq(&cam->viadev->reg_lock);
+ viacam_int_disable(cam);
+ viacam_write_reg_mask(cam, VCR_CAPINTC, 0, VCR_CI_ENABLE);
+ (void) viacam_read_reg(cam, VCR_CAPINTC); /* Force post */
+ cam->opstate = S_IDLE;
+ spin_unlock_irq(&cam->viadev->reg_lock);
+}
+
+
+/* --------------------------------------------------------------------------*/
+/* Videobuf callback ops */
+
+/*
+ * buffer_setup. The purpose of this one would appear to be to tell
+ * videobuf how big a single image is. It's also evidently up to us
+ * to put some sort of limit on the maximum number of buffers allowed.
+ */
+static int viacam_vb_buf_setup(struct videobuf_queue *q,
+ unsigned int *count, unsigned int *size)
+{
+ struct via_camera *cam = q->priv_data;
+
+ *size = cam->user_format.sizeimage;
+ if (*count == 0 || *count > 6) /* Arbitrary number */
+ *count = 6;
+ return 0;
+}
+
+/*
+ * Prepare a buffer.
+ */
+static int viacam_vb_buf_prepare(struct videobuf_queue *q,
+ struct videobuf_buffer *vb, enum v4l2_field field)
+{
+ struct via_camera *cam = q->priv_data;
+
+ vb->size = cam->user_format.sizeimage;
+ vb->width = cam->user_format.width; /* bytesperline???? */
+ vb->height = cam->user_format.height;
+ vb->field = field;
+ if (vb->state == VIDEOBUF_NEEDS_INIT) {
+ int ret = videobuf_iolock(q, vb, NULL);
+ if (ret)
+ return ret;
+ }
+ vb->state = VIDEOBUF_PREPARED;
+ return 0;
+}
+
+/*
+ * We've got a buffer to put data into.
+ *
+ * FIXME: check for a running engine and valid buffers?
+ */
+static void viacam_vb_buf_queue(struct videobuf_queue *q,
+ struct videobuf_buffer *vb)
+{
+ struct via_camera *cam = q->priv_data;
+
+ /*
+ * Note that videobuf holds the lock when it calls
+ * us, so we need not (indeed, cannot) take it here.
+ */
+ vb->state = VIDEOBUF_QUEUED;
+ list_add_tail(&vb->queue, &cam->buffer_queue);
+}
+
+/*
+ * Free a buffer.
+ */
+static void viacam_vb_buf_release(struct videobuf_queue *q,
+ struct videobuf_buffer *vb)
+{
+ struct via_camera *cam = q->priv_data;
+
+ videobuf_dma_unmap(&cam->platdev->dev, videobuf_to_dma(vb));
+ videobuf_dma_free(videobuf_to_dma(vb));
+ vb->state = VIDEOBUF_NEEDS_INIT;
+}
+
+static const struct videobuf_queue_ops viacam_vb_ops = {
+ .buf_setup = viacam_vb_buf_setup,
+ .buf_prepare = viacam_vb_buf_prepare,
+ .buf_queue = viacam_vb_buf_queue,
+ .buf_release = viacam_vb_buf_release,
+};
+
+/* --------------------------------------------------------------------------*/
+/* File operations */
+
+static int viacam_open(struct file *filp)
+{
+ struct via_camera *cam = video_drvdata(filp);
+
+ filp->private_data = cam;
+ /*
+ * Note the new user. If this is the first one, we'll also
+ * need to power up the sensor.
+ */
+ mutex_lock(&cam->lock);
+ if (cam->users == 0) {
+ int ret = viafb_request_dma();
+
+ if (ret) {
+ mutex_unlock(&cam->lock);
+ return ret;
+ }
+ via_sensor_power_up(cam);
+ set_bit(CF_CONFIG_NEEDED, &cam->flags);
+ /*
+ * Hook into videobuf. Evidently this cannot fail.
+ */
+ videobuf_queue_sg_init(&cam->vb_queue, &viacam_vb_ops,
+ &cam->platdev->dev, &cam->viadev->reg_lock,
+ V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE,
+ sizeof(struct videobuf_buffer), cam);
+ }
+ (cam->users)++;
+ mutex_unlock(&cam->lock);
+ return 0;
+}
+
+static int viacam_release(struct file *filp)
+{
+ struct via_camera *cam = video_drvdata(filp);
+
+ mutex_lock(&cam->lock);
+ (cam->users)--;
+ /*
+ * If the "owner" is closing, shut down any ongoing
+ * operations.
+ */
+ if (filp == cam->owner) {
+ videobuf_stop(&cam->vb_queue);
+ /*
+ * We don't hold the spinlock here, but, if release()
+ * is being called by the owner, nobody else will
+ * be changing the state. And an extra stop would
+ * not hurt anyway.
+ */
+ if (cam->opstate != S_IDLE)
+ viacam_stop_engine(cam);
+ cam->owner = NULL;
+ }
+ /*
+ * Last one out needs to turn out the lights.
+ */
+ if (cam->users == 0) {
+ videobuf_mmap_free(&cam->vb_queue);
+ via_sensor_power_down(cam);
+ viafb_release_dma();
+ }
+ mutex_unlock(&cam->lock);
+ return 0;
+}
+
+/*
+ * Read a frame from the device.
+ */
+static ssize_t viacam_read(struct file *filp, char __user *buffer,
+ size_t len, loff_t *pos)
+{
+ struct via_camera *cam = video_drvdata(filp);
+ int ret;
+
+ mutex_lock(&cam->lock);
+ /*
+ * Enforce the V4l2 "only one owner gets to read data" rule.
+ */
+ if (cam->owner && cam->owner != filp) {
+ ret = -EBUSY;
+ goto out_unlock;
+ }
+ cam->owner = filp;
+ /*
+ * Do we need to configure the hardware?
+ */
+ if (test_bit(CF_CONFIG_NEEDED, &cam->flags)) {
+ ret = viacam_configure_sensor(cam);
+ if (!ret)
+ ret = viacam_config_controller(cam);
+ if (ret)
+ goto out_unlock;
+ }
+ /*
+ * Fire up the capture engine, then have videobuf do
+ * the heavy lifting. Someday it would be good to avoid
+ * stopping and restarting the engine each time.
+ */
+ INIT_LIST_HEAD(&cam->buffer_queue);
+ viacam_start_engine(cam);
+ ret = videobuf_read_stream(&cam->vb_queue, buffer, len, pos, 0,
+ filp->f_flags & O_NONBLOCK);
+ viacam_stop_engine(cam);
+ /* videobuf_stop() ?? */
+
+out_unlock:
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+
+static unsigned int viacam_poll(struct file *filp, struct poll_table_struct *pt)
+{
+ struct via_camera *cam = video_drvdata(filp);
+
+ return videobuf_poll_stream(filp, &cam->vb_queue, pt);
+}
+
+
+static int viacam_mmap(struct file *filp, struct vm_area_struct *vma)
+{
+ struct via_camera *cam = video_drvdata(filp);
+
+ return videobuf_mmap_mapper(&cam->vb_queue, vma);
+}
+
+
+
+static const struct v4l2_file_operations viacam_fops = {
+ .owner = THIS_MODULE,
+ .open = viacam_open,
+ .release = viacam_release,
+ .read = viacam_read,
+ .poll = viacam_poll,
+ .mmap = viacam_mmap,
+ .ioctl = video_ioctl2,
+};
+
+/*----------------------------------------------------------------------------*/
+/*
+ * The long list of v4l2 ioctl ops
+ */
+
+static int viacam_g_chip_ident(struct file *file, void *priv,
+ struct v4l2_dbg_chip_ident *ident)
+{
+ struct via_camera *cam = priv;
+
+ ident->ident = V4L2_IDENT_NONE;
+ ident->revision = 0;
+ if (v4l2_chip_match_host(&ident->match)) {
+ ident->ident = V4L2_IDENT_VIA_VX855;
+ return 0;
+ }
+ return sensor_call(cam, core, g_chip_ident, ident);
+}
+
+/*
+ * Control ops are passed through to the sensor.
+ */
+static int viacam_queryctrl(struct file *filp, void *priv,
+ struct v4l2_queryctrl *qc)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, core, queryctrl, qc);
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+
+static int viacam_g_ctrl(struct file *filp, void *priv,
+ struct v4l2_control *ctrl)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, core, g_ctrl, ctrl);
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+
+static int viacam_s_ctrl(struct file *filp, void *priv,
+ struct v4l2_control *ctrl)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, core, s_ctrl, ctrl);
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+/*
+ * Only one input.
+ */
+static int viacam_enum_input(struct file *filp, void *priv,
+ struct v4l2_input *input)
+{
+ if (input->index != 0)
+ return -EINVAL;
+
+ input->type = V4L2_INPUT_TYPE_CAMERA;
+ input->std = V4L2_STD_ALL; /* Not sure what should go here */
+ strcpy(input->name, "Camera");
+ return 0;
+}
+
+static int viacam_g_input(struct file *filp, void *priv, unsigned int *i)
+{
+ *i = 0;
+ return 0;
+}
+
+static int viacam_s_input(struct file *filp, void *priv, unsigned int i)
+{
+ if (i != 0)
+ return -EINVAL;
+ return 0;
+}
+
+static int viacam_s_std(struct file *filp, void *priv, v4l2_std_id *std)
+{
+ return 0;
+}
+
+/*
+ * Video format stuff. Here is our default format until
+ * user space messes with things.
+ */
+static const struct v4l2_pix_format viacam_def_pix_format = {
+ .width = VGA_WIDTH,
+ .height = VGA_HEIGHT,
+ .pixelformat = V4L2_PIX_FMT_YUYV,
+ .field = V4L2_FIELD_NONE,
+ .bytesperline = VGA_WIDTH * 2,
+ .sizeimage = VGA_WIDTH * VGA_HEIGHT * 2,
+};
+
+static int viacam_enum_fmt_vid_cap(struct file *filp, void *priv,
+ struct v4l2_fmtdesc *fmt)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, video, enum_fmt, fmt);
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+/*
+ * Figure out proper image dimensions, but always force the
+ * sensor to VGA.
+ */
+static void viacam_fmt_pre(struct v4l2_pix_format *userfmt,
+ struct v4l2_pix_format *sensorfmt)
+{
+ *sensorfmt = *userfmt;
+ if (userfmt->width < QCIF_WIDTH || userfmt->height < QCIF_HEIGHT) {
+ userfmt->width = QCIF_WIDTH;
+ userfmt->height = QCIF_HEIGHT;
+ }
+ if (userfmt->width > VGA_WIDTH || userfmt->height > VGA_HEIGHT) {
+ userfmt->width = VGA_WIDTH;
+ userfmt->height = VGA_HEIGHT;
+ }
+ sensorfmt->width = VGA_WIDTH;
+ sensorfmt->height = VGA_HEIGHT;
+}
+
+static void viacam_fmt_post(struct v4l2_pix_format *userfmt,
+ struct v4l2_pix_format *sensorfmt)
+{
+ userfmt->pixelformat = sensorfmt->pixelformat;
+ userfmt->field = sensorfmt->field;
+ userfmt->bytesperline = 2 * userfmt->width;
+ userfmt->sizeimage = userfmt->bytesperline * userfmt->height;
+}
+
+static int viacam_try_fmt_vid_cap(struct file *filp, void *priv,
+ struct v4l2_format *fmt)
+{
+ struct via_camera *cam = priv;
+ int ret;
+ struct v4l2_format sfmt;
+
+ viacam_fmt_pre(&fmt->fmt.pix, &sfmt.fmt.pix);
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, video, try_fmt, &sfmt);
+ mutex_unlock(&cam->lock);
+ viacam_fmt_post(&fmt->fmt.pix, &sfmt.fmt.pix);
+ return ret;
+}
+
+static int viacam_g_fmt_vid_cap(struct file *filp, void *priv,
+ struct v4l2_format *fmt)
+{
+ struct via_camera *cam = priv;
+
+ mutex_lock(&cam->lock);
+ fmt->fmt.pix = cam->user_format;
+ mutex_unlock(&cam->lock);
+ return 0;
+}
+
+static int viacam_s_fmt_vid_cap(struct file *filp, void *priv,
+ struct v4l2_format *fmt)
+{
+ struct via_camera *cam = priv;
+ int ret;
+ struct v4l2_format sfmt;
+
+ /*
+ * Camera must be idle or we can't mess with the
+ * video setup.
+ */
+ mutex_lock(&cam->lock);
+ if (cam->opstate != S_IDLE) {
+ ret = -EBUSY;
+ goto out;
+ }
+ /*
+ * Let the sensor code look over and tweak the
+ * requested formatting.
+ */
+ viacam_fmt_pre(&fmt->fmt.pix, &sfmt.fmt.pix);
+ ret = sensor_call(cam, video, try_fmt, &sfmt);
+ if (ret)
+ goto out;
+ viacam_fmt_post(&fmt->fmt.pix, &sfmt.fmt.pix);
+ /*
+ * OK, let's commit to the new format.
+ */
+ cam->user_format = fmt->fmt.pix;
+ cam->sensor_format = sfmt.fmt.pix;
+ ret = viacam_configure_sensor(cam);
+ if (!ret)
+ ret = viacam_config_controller(cam);
+out:
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+static int viacam_querycap(struct file *filp, void *priv,
+ struct v4l2_capability *cap)
+{
+ strcpy(cap->driver, "via-camera");
+ strcpy(cap->card, "via-camera");
+ cap->version = 1;
+ cap->capabilities = V4L2_CAP_VIDEO_CAPTURE |
+ V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
+ return 0;
+}
+
+/*
+ * Streaming operations - pure videobuf stuff.
+ */
+static int viacam_reqbufs(struct file *filp, void *priv,
+ struct v4l2_requestbuffers *rb)
+{
+ struct via_camera *cam = priv;
+
+ return videobuf_reqbufs(&cam->vb_queue, rb);
+}
+
+static int viacam_querybuf(struct file *filp, void *priv,
+ struct v4l2_buffer *buf)
+{
+ struct via_camera *cam = priv;
+
+ return videobuf_querybuf(&cam->vb_queue, buf);
+}
+
+static int viacam_qbuf(struct file *filp, void *priv, struct v4l2_buffer *buf)
+{
+ struct via_camera *cam = priv;
+
+ return videobuf_qbuf(&cam->vb_queue, buf);
+}
+
+static int viacam_dqbuf(struct file *filp, void *priv, struct v4l2_buffer *buf)
+{
+ struct via_camera *cam = priv;
+
+ return videobuf_dqbuf(&cam->vb_queue, buf, filp->f_flags & O_NONBLOCK);
+}
+
+static int viacam_streamon(struct file *filp, void *priv, enum v4l2_buf_type t)
+{
+ struct via_camera *cam = priv;
+ int ret = 0;
+
+ if (t != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+
+ mutex_lock(&cam->lock);
+ if (cam->opstate != S_IDLE) {
+ ret = -EBUSY;
+ goto out;
+ }
+ /*
+ * Enforce the V4l2 "only one owner gets to read data" rule.
+ */
+ if (cam->owner && cam->owner != filp) {
+ ret = -EBUSY;
+ goto out;
+ }
+ cam->owner = filp;
+ /*
+ * Configure things if need be.
+ */
+ if (test_bit(CF_CONFIG_NEEDED, &cam->flags)) {
+ ret = viacam_configure_sensor(cam);
+ if (ret)
+ goto out;
+ ret = viacam_config_controller(cam);
+ if (ret)
+ goto out;
+ }
+ /*
+ * If the CPU goes into C3, the DMA transfer gets corrupted and
+ * users start filing unsightly bug reports. Put in a "latency"
+ * requirement which will keep the CPU out of the deeper sleep
+ * states.
+ */
+ pm_qos_add_request(&cam->qos_request, PM_QOS_CPU_DMA_LATENCY, 50);
+ /*
+ * Fire things up.
+ */
+ INIT_LIST_HEAD(&cam->buffer_queue);
+ ret = videobuf_streamon(&cam->vb_queue);
+ if (!ret)
+ viacam_start_engine(cam);
+out:
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+static int viacam_streamoff(struct file *filp, void *priv, enum v4l2_buf_type t)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ if (t != V4L2_BUF_TYPE_VIDEO_CAPTURE)
+ return -EINVAL;
+ mutex_lock(&cam->lock);
+ if (cam->opstate != S_RUNNING) {
+ ret = -EINVAL;
+ goto out;
+ }
+ pm_qos_remove_request(&cam->qos_request);
+ viacam_stop_engine(cam);
+ /*
+ * Videobuf will recycle all of the outstanding buffers, but
+ * we should be sure we don't retain any references to
+ * any of them.
+ */
+ ret = videobuf_streamoff(&cam->vb_queue);
+ INIT_LIST_HEAD(&cam->buffer_queue);
+out:
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+#ifdef CONFIG_VIDEO_V4L1_COMPAT
+static int viacam_vidiocgmbuf(struct file *filp, void *priv,
+ struct video_mbuf *mbuf)
+{
+ struct via_camera *cam = priv;
+
+ return videobuf_cgmbuf(&cam->vb_queue, mbuf, 6);
+}
+#endif
+
+/* G/S_PARM */
+
+static int viacam_g_parm(struct file *filp, void *priv,
+ struct v4l2_streamparm *parm)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, video, g_parm, parm);
+ mutex_unlock(&cam->lock);
+ parm->parm.capture.readbuffers = cam->n_cap_bufs;
+ return ret;
+}
+
+static int viacam_s_parm(struct file *filp, void *priv,
+ struct v4l2_streamparm *parm)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, video, s_parm, parm);
+ mutex_unlock(&cam->lock);
+ parm->parm.capture.readbuffers = cam->n_cap_bufs;
+ return ret;
+}
+
+static int viacam_enum_framesizes(struct file *filp, void *priv,
+ struct v4l2_frmsizeenum *sizes)
+{
+ if (sizes->index != 0)
+ return -EINVAL;
+ sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
+ sizes->stepwise.min_width = QCIF_WIDTH;
+ sizes->stepwise.min_height = QCIF_HEIGHT;
+ sizes->stepwise.max_width = VGA_WIDTH;
+ sizes->stepwise.max_height = VGA_HEIGHT;
+ sizes->stepwise.step_width = sizes->stepwise.step_height = 1;
+ return 0;
+}
+
+static int viacam_enum_frameintervals(struct file *filp, void *priv,
+ struct v4l2_frmivalenum *interval)
+{
+ struct via_camera *cam = priv;
+ int ret;
+
+ mutex_lock(&cam->lock);
+ ret = sensor_call(cam, video, enum_frameintervals, interval);
+ mutex_unlock(&cam->lock);
+ return ret;
+}
+
+
+
+static const struct v4l2_ioctl_ops viacam_ioctl_ops = {
+ .vidioc_g_chip_ident = viacam_g_chip_ident,
+ .vidioc_queryctrl = viacam_queryctrl,
+ .vidioc_g_ctrl = viacam_g_ctrl,
+ .vidioc_s_ctrl = viacam_s_ctrl,
+ .vidioc_enum_input = viacam_enum_input,
+ .vidioc_g_input = viacam_g_input,
+ .vidioc_s_input = viacam_s_input,
+ .vidioc_s_std = viacam_s_std,
+ .vidioc_enum_fmt_vid_cap = viacam_enum_fmt_vid_cap,
+ .vidioc_try_fmt_vid_cap = viacam_try_fmt_vid_cap,
+ .vidioc_g_fmt_vid_cap = viacam_g_fmt_vid_cap,
+ .vidioc_s_fmt_vid_cap = viacam_s_fmt_vid_cap,
+ .vidioc_querycap = viacam_querycap,
+ .vidioc_reqbufs = viacam_reqbufs,
+ .vidioc_querybuf = viacam_querybuf,
+ .vidioc_qbuf = viacam_qbuf,
+ .vidioc_dqbuf = viacam_dqbuf,
+ .vidioc_streamon = viacam_streamon,
+ .vidioc_streamoff = viacam_streamoff,
+ .vidioc_g_parm = viacam_g_parm,
+ .vidioc_s_parm = viacam_s_parm,
+ .vidioc_enum_framesizes = viacam_enum_framesizes,
+ .vidioc_enum_frameintervals = viacam_enum_frameintervals,
+#ifdef CONFIG_VIDEO_V4L1_COMPAT
+ .vidiocgmbuf = viacam_vidiocgmbuf,
+#endif
+};
+
+/*----------------------------------------------------------------------------*/
+
+/*
+ * Power management.
+ */
+
+/*
+ * Setup stuff.
+ */
+
+static struct video_device viacam_v4l_template = {
+ .name = "via-camera",
+ .minor = -1,
+ .tvnorms = V4L2_STD_NTSC_M,
+ .current_norm = V4L2_STD_NTSC_M,
+ .fops = &viacam_fops,
+ .ioctl_ops = &viacam_ioctl_ops,
+ .release = video_device_release_empty, /* Check this */
+};
+
+
+static __devinit int viacam_probe(struct platform_device *pdev)
+{
+ int ret;
+ struct i2c_adapter *sensor_adapter;
+ struct viafb_dev *viadev = pdev->dev.platform_data;
+
+ /*
+ * Note that there are actually two capture channels on
+ * the device. We only deal with one for now. That
+ * is encoded here; nothing else assumes it's dealing with
+ * a unique capture device.
+ */
+ struct via_camera *cam;
+
+ /*
+ * Ensure that frame buffer memory has been set aside for
+ * this purpose. As an arbitrary limit, refuse to work
+ * with less than two frames of VGA 16-bit data.
+ *
+ * If we ever support the second port, we'll need to set
+ * aside more memory.
+ */
+ if (viadev->camera_fbmem_size < (VGA_HEIGHT*VGA_WIDTH*4)) {
+ printk(KERN_ERR "viacam: insufficient FB memory reserved\n");
+ return -ENOMEM;
+ }
+ if (viadev->engine_mmio == NULL) {
+ printk(KERN_ERR "viacam: No I/O memory, so no pictures\n");
+ return -ENOMEM;
+ }
+ /*
+ * Basic structure initialization.
+ */
+ cam = kzalloc (sizeof(struct via_camera), GFP_KERNEL);
+ if (cam == NULL)
+ return -ENOMEM;
+ via_cam_info = cam;
+ cam->platdev = pdev;
+ cam->viadev = viadev;
+ cam->users = 0;
+ cam->owner = NULL;
+ cam->opstate = S_IDLE;
+ cam->user_format = cam->sensor_format = viacam_def_pix_format;
+ mutex_init(&cam->lock);
+ INIT_LIST_HEAD(&cam->buffer_queue);
+ cam->mmio = viadev->engine_mmio;
+ cam->fbmem = viadev->fbmem;
+ cam->fb_offset = viadev->camera_fbmem_offset;
+ cam->flags = 1 << CF_CONFIG_NEEDED;
+ /*
+ * Tell V4L that we exist.
+ */
+ ret = v4l2_device_register(&pdev->dev, &cam->v4l2_dev);
+ if (ret) {
+ dev_err(&pdev->dev, "Unable to register v4l2 device\n");
+ return ret;
+ }
+ /*
+ * Convince the system that we can do DMA.
+ */
+ pdev->dev.dma_mask = &viadev->pdev->dma_mask;
+ dma_set_mask(&pdev->dev, 0xffffffff);
+ /*
+ * Fire up the capture port. The write to 0x78 looks purely
+ * OLPCish; any system will need to tweak 0x1e.
+ */
+ via_write_reg_mask(VIASR, 0x78, 0, 0x80);
+ via_write_reg_mask(VIASR, 0x1e, 0xc0, 0xc0);
+ /*
+ * Get the sensor powered up.
+ */
+ ret = via_sensor_power_setup(cam);
+ if (ret)
+ goto out_unregister;
+ via_sensor_power_up(cam);
+
+ /*
+ * See if we can't find it on the bus. The VIA_PORT_31 assumption
+ * is OLPC-specific. 0x42 assumption is ov7670-specific.
+ */
+ sensor_adapter = viafb_find_i2c_adapter(VIA_PORT_31);
+ cam->sensor = v4l2_i2c_new_subdev(&cam->v4l2_dev, sensor_adapter,
+ "ov7670", "ov7670", 0x42 >> 1, NULL);
+ if (cam->sensor == NULL) {
+ dev_err(&pdev->dev, "Unable to find the sensor!\n");
+ ret = -ENODEV;
+ goto out_power_down;
+ }
+ /*
+ * Get the IRQ.
+ */
+ viacam_int_disable(cam);
+ ret = request_threaded_irq(viadev->pdev->irq, viacam_quick_irq,
+ viacam_irq, IRQF_SHARED, "via-camera", cam);
+ if (ret)
+ goto out_power_down;
+ /*
+ * Tell V4l2 that we exist.
+ */
+ cam->vdev = viacam_v4l_template;
+ cam->vdev.v4l2_dev = &cam->v4l2_dev;
+ ret = video_register_device(&cam->vdev, VFL_TYPE_GRABBER, -1);
+ if (ret)
+ goto out_irq;
+ video_set_drvdata(&cam->vdev, cam);
+
+ /* Power the sensor down until somebody opens the device */
+ via_sensor_power_down(cam);
+ return 0;
+
+out_irq:
+ free_irq(viadev->pdev->irq, cam);
+out_power_down:
+ via_sensor_power_release(cam);
+out_unregister:
+ v4l2_device_unregister(&cam->v4l2_dev);
+ return ret;
+}
+
+static __devexit int viacam_remove(struct platform_device *pdev)
+{
+ struct via_camera *cam = via_cam_info;
+ struct viafb_dev *viadev = pdev->dev.platform_data;
+
+ video_unregister_device(&cam->vdev);
+ v4l2_device_unregister(&cam->v4l2_dev);
+ free_irq(viadev->pdev->irq, cam);
+ via_sensor_power_release(cam);
+ via_cam_info = NULL;
+ return 0;
+}
+
+
+static struct platform_driver viacam_driver = {
+ .driver = {
+ .name = "viafb-camera",
+ },
+ .probe = viacam_probe,
+ .remove = viacam_remove,
+};
+
+
+#ifdef CONFIG_OLPC_XO_1_5
+/*
+ * The OLPC folks put the serial port on the same pin as
+ * the camera. They also get grumpy if we break the
+ * serial port and keep them from using it. So we have
+ * to check the serial enable bit and not step on it.
+ */
+#define VIACAM_SERIAL_DEVFN 0x88
+#define VIACAM_SERIAL_CREG 0x46
+#define VIACAM_SERIAL_BIT 0x40
+
+static __devinit int viacam_check_serial_port(void)
+{
+ struct pci_bus *pbus = pci_find_bus(0, 0);
+ u8 cbyte;
+
+ pci_bus_read_config_byte(pbus, VIACAM_SERIAL_DEVFN,
+ VIACAM_SERIAL_CREG, &cbyte);
+ if ((cbyte & VIACAM_SERIAL_BIT) == 0)
+ return 0; /* Not enabled */
+ if (override_serial == 0) {
+ printk(KERN_NOTICE "Via camera: serial port is enabled, " \
+ "refusing to load.\n");
+ printk(KERN_NOTICE "Specify override_serial=1 to force " \
+ "module loading.\n");
+ return -EBUSY;
+ }
+ printk(KERN_NOTICE "Via camera: overriding serial port\n");
+ pci_bus_write_config_byte(pbus, VIACAM_SERIAL_DEVFN,
+ VIACAM_SERIAL_CREG, cbyte & ~VIACAM_SERIAL_BIT);
+ return 0;
+}
+#endif
+
+
+
+
+static int viacam_init(void)
+{
+#ifdef CONFIG_OLPC_XO_1_5
+ if (viacam_check_serial_port())
+ return -EBUSY;
+#endif
+ return platform_driver_register(&viacam_driver);
+}
+module_init(viacam_init);
+
+static void viacam_exit(void)
+{
+ platform_driver_unregister(&viacam_driver);
+}
+module_exit(viacam_exit);
diff --git a/drivers/media/video/via-camera.h b/drivers/media/video/via-camera.h
new file mode 100644
index 0000000..b12a4b3
--- /dev/null
+++ b/drivers/media/video/via-camera.h
@@ -0,0 +1,93 @@
+/*
+ * VIA Camera register definitions.
+ */
+#define VCR_INTCTRL 0x300 /* Capture interrupt control */
+#define VCR_IC_EAV 0x0001 /* End of active video status */
+#define VCR_IC_EVBI 0x0002 /* End of VBI status */
+#define VCR_IC_FBOTFLD 0x0004 /* "flipping" Bottom field is active */
+#define VCR_IC_ACTBUF 0x0018 /* Active video buffer */
+#define VCR_IC_VSYNC 0x0020 /* 0 = VB, 1 = active video */
+#define VCR_IC_BOTFLD 0x0040 /* Bottom field is active */
+#define VCR_IC_FFULL 0x0080 /* FIFO full */
+#define VCR_IC_INTEN 0x0100 /* End of active video int. enable */
+#define VCR_IC_VBIINT 0x0200 /* End of VBI int enable */
+#define VCR_IC_VBIBUF 0x0400 /* Current VBI buffer */
+
+#define VCR_TSC 0x308 /* Transport stream control */
+#define VCR_TSC_ENABLE 0x000001 /* Transport stream input enable */
+#define VCR_TSC_DROPERR 0x000002 /* Drop error packets */
+#define VCR_TSC_METHOD 0x00000c /* DMA method (non-functional) */
+#define VCR_TSC_COUNT 0x07fff0 /* KByte or packet count */
+#define VCR_TSC_CBMODE 0x080000 /* Change buffer by byte count */
+#define VCR_TSC_PSSIG 0x100000 /* Packet starting signal disable */
+#define VCR_TSC_BE 0x200000 /* MSB first (serial mode) */
+#define VCR_TSC_SERIAL 0x400000 /* Serial input (0 = parallel) */
+
+#define VCR_CAPINTC 0x310 /* Capture interface control */
+#define VCR_CI_ENABLE 0x00000001 /* Capture enable */
+#define VCR_CI_BSS 0x00000002 /* WTF "bit stream selection" */
+#define VCR_CI_3BUFS 0x00000004 /* 1 = 3 buffers, 0 = 2 buffers */
+#define VCR_CI_VIPEN 0x00000008 /* VIP enable */
+#define VCR_CI_CCIR601_8 0 /* CCIR601 input stream, 8 bit */
+#define VCR_CI_CCIR656_8 0x00000010 /* ... CCIR656, 8 bit */
+#define VCR_CI_CCIR601_16 0x00000020 /* ... CCIR601, 16 bit */
+#define VCR_CI_CCIR656_16 0x00000030 /* ... CCIR656, 16 bit */
+#define VCR_CI_HDMODE 0x00000040 /* CCIR656-16 hdr decode mode; 1=16b */
+#define VCR_CI_BSWAP 0x00000080 /* Swap bytes (16-bit) */
+#define VCR_CI_YUYV 0 /* Byte order 0123 */
+#define VCR_CI_UYVY 0x00000100 /* Byte order 1032 */
+#define VCR_CI_YVYU 0x00000200 /* Byte order 0321 */
+#define VCR_CI_VYUY 0x00000300 /* Byte order 3012 */
+#define VCR_CI_VIPTYPE 0x00000400 /* VIP type */
+#define VCR_CI_IFSEN 0x00000800 /* Input field signal enable */
+#define VCR_CI_DIODD 0 /* De-interlace odd, 30fps */
+#define VCR_CI_DIEVEN 0x00001000 /* ...even field, 30fps */
+#define VCR_CI_DIBOTH 0x00002000 /* ...both fields, 60fps */
+#define VCR_CI_DIBOTH30 0x00003000 /* ...both fields, 30fps interlace */
+#define VCR_CI_CONVTYPE 0x00004000 /* 4:2:2 to 4:4:4; 1 = interpolate */
+#define VCR_CI_CFC 0x00008000 /* Capture flipping control */
+#define VCR_CI_FILTER 0x00070000 /* Horiz filter mode select
+ 000 = none
+ 001 = 2 tap
+ 010 = 3 tap
+ 011 = 4 tap
+ 100 = 5 tap */
+#define VCR_CI_CLKINV 0x00080000 /* Input CLK inverted */
+#define VCR_CI_VREFINV 0x00100000 /* VREF inverted */
+#define VCR_CI_HREFINV 0x00200000 /* HREF inverted */
+#define VCR_CI_FLDINV 0x00400000 /* Field inverted */
+#define VCR_CI_CLKPIN 0x00800000 /* Capture clock pin */
+#define VCR_CI_THRESH 0x0f000000 /* Capture fifo threshold */
+#define VCR_CI_HRLE 0x10000000 /* Positive edge of HREF */
+#define VCR_CI_VRLE 0x20000000 /* Positive edge of VREF */
+#define VCR_CI_OFLDINV 0x40000000 /* Field output inverted */
+#define VCR_CI_CLKEN 0x80000000 /* Capture clock enable */
+
+#define VCR_HORRANGE 0x314 /* Active video horizontal range */
+#define VCR_VERTRANGE 0x318 /* Active video vertical range */
+#define VCR_AVSCALE 0x31c /* Active video scaling control */
+#define VCR_AVS_HEN 0x00000800 /* Horizontal scale enable */
+#define VCR_AVS_VEN 0x04000000 /* Vertical enable */
+#define VCR_VBIHOR 0x320 /* VBI Data horizontal range */
+#define VCR_VBIVERT 0x324 /* VBI data vertical range */
+#define VCR_VBIBUF1 0x328 /* First VBI buffer */
+#define VCR_VBISTRIDE 0x32c /* VBI stride */
+#define VCR_ANCDATACNT 0x330 /* Ancillary data count setting */
+#define VCR_MAXDATA 0x334 /* Active data count of active video */
+#define VCR_MAXVBI 0x338 /* Maximum data count of VBI */
+#define VCR_CAPDATA 0x33c /* Capture data count */
+#define VCR_VBUF1 0x340 /* First video buffer */
+#define VCR_VBUF2 0x344 /* Second video buffer */
+#define VCR_VBUF3 0x348 /* Third video buffer */
+#define VCR_VBUF_MASK 0x1ffffff0 /* Bits 28:4 */
+#define VCR_VBIBUF2 0x34c /* Second VBI buffer */
+#define VCR_VSTRIDE 0x350 /* Stride of video + coring control */
+#define VCR_VS_STRIDE_SHIFT 4
+#define VCR_VS_STRIDE 0x00001ff0 /* Stride (8-byte units) */
+#define VCR_VS_CCD 0x007f0000 /* Coring compare data */
+#define VCR_VS_COREEN 0x00800000 /* Coring enable */
+#define VCR_TS0ERR 0x354 /* TS buffer 0 error indicator */
+#define VCR_TS1ERR 0x358 /* TS buffer 0 error indicator */
+#define VCR_TS2ERR 0x35c /* TS buffer 0 error indicator */
+
+/* Add 0x1000 for the second capture engine registers */
diff --git a/drivers/video/via/accel.c b/drivers/video/via/accel.c
index e44893e..04bec05 100644
--- a/drivers/video/via/accel.c
+++ b/drivers/video/via/accel.c
@@ -370,7 +370,7 @@ int viafb_init_engine(struct fb_info *info)
viapar->shared->vq_vram_addr = viapar->fbmem_free;
viapar->fbmem_used += VQ_SIZE;
-#if defined(CONFIG_FB_VIA_CAMERA) || defined(CONFIG_FB_VIA_CAMERA_MODULE)
+#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
/*
* Set aside a chunk of framebuffer memory for the camera
* driver. Someday this driver probably needs a proper allocator
diff --git a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c
index 66f4030..27d7260 100644
--- a/drivers/video/via/via-core.c
+++ b/drivers/video/via/via-core.c
@@ -95,6 +95,13 @@ EXPORT_SYMBOL_GPL(viafb_irq_disable);
/* ---------------------------------------------------------------------- */
/*
+ * Currently, the camera driver is the only user of the DMA code, so we
+ * only compile it in if the camera driver is being built. Chances are,
+ * most viafb systems will not need to have this extra code for a while.
+ * As soon as another user comes long, the ifdef can be removed.
+ */
+#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
+/*
* Access to the DMA engine. This currently provides what the camera
* driver needs (i.e. outgoing only) but is easily expandable if need
* be.
@@ -322,7 +329,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct scatterlist *sg, int nsg)
return 0;
}
EXPORT_SYMBOL_GPL(viafb_dma_copy_out_sg);
-
+#endif /* CONFIG_VIDEO_VIA_CAMERA */
/* ---------------------------------------------------------------------- */
/*
@@ -507,7 +514,12 @@ static struct viafb_subdev_info {
},
{
.name = "viafb-i2c",
- }
+ },
+#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
+ {
+ .name = "viafb-camera",
+ },
+#endif
};
#define N_SUBDEVS ARRAY_SIZE(viafb_subdevs)
diff --git a/include/linux/via-core.h b/include/linux/via-core.h
index 7ffb521..38bffd8 100644
--- a/include/linux/via-core.h
+++ b/include/linux/via-core.h
@@ -81,7 +81,7 @@ struct viafb_dev {
unsigned long fbmem_start;
long fbmem_len;
void __iomem *fbmem;
-#if defined(CONFIG_FB_VIA_CAMERA) || defined(CONFIG_FB_VIA_CAMERA_MODULE)
+#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
long camera_fbmem_offset;
long camera_fbmem_size;
#endif
@@ -138,6 +138,7 @@ void viafb_irq_disable(u32 mask);
#define VDE_I_LVDSSIEN 0x40000000 /* LVDS Sense enable */
#define VDE_I_ENABLE 0x80000000 /* Global interrupt enable */
+#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
/*
* DMA management.
*/
@@ -172,6 +173,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct scatterlist *sg, int nsg);
*/
#define VGA_WIDTH 640
#define VGA_HEIGHT 480
+#endif /* CONFIG_VIDEO_VIA_CAMERA */
/*
* Indexed port operations. Note that these are all multi-op
diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
index 21b4428..6400c7c 100644
--- a/include/media/v4l2-chip-ident.h
+++ b/include/media/v4l2-chip-ident.h
@@ -111,6 +111,10 @@ enum {
V4L2_IDENT_VPX3216B = 3216,
V4L2_IDENT_VPX3220A = 3220,
+ /* VX855 just ident 3409 */
+ /* Other via devs could use 3314, 3324, 3327, 3336, 3364, 3353 */
+ V4L2_IDENT_VIA_VX855 = 3409,
+
/* module tvp5150 */
V4L2_IDENT_TVP5150 = 5150,
--
1.7.3.1
^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-10 22:23 [PATCH] viafb camera controller driver Jonathan Corbet
@ 2010-10-11 12:09 ` Mauro Carvalho Chehab
2010-10-11 12:18 ` Laurent Pinchart
2010-10-16 13:44 ` Mauro Carvalho Chehab
2 siblings, 0 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-11 12:09 UTC (permalink / raw)
To: Jonathan Corbet
Cc: linux-media, Florian Tobias Schandinat, Laurent Pinchart,
Daniel Drake
Em 10-10-2010 19:23, Jonathan Corbet escreveu:
> Howdy, all,
>
> Well, that took a whole lot longer than I had hoped...but, attached, is a
> new version of the viafb camera driver patch, done against 2.6.36-rc7.
> I've tried to address most of Laurent's comments from back in June; in
> particular, I have:
>
> - Gotten rid of the static device structure
> - Fixed some locking glitches
> - Fixed a bit of device initialization silliness.
>
> One thing I have *not* done is to push locking down into the ov7670
> driver. That would be a good thing to do at some point, but playing with
> that driver was beyond the scope of what I was trying to do here.
>
> This driver will still need some OLPC bits to work properly, but Daniel is
> working on that. This version of the driver does work on XO-1.5 systems,
> modulo some 2.6.36 API changes.
>
> Mauro, any chance of putting this in the queue for 2.6.37? Yes, I know
> it's really late, my apologies for that.
Jon,
I'll work to handle it for 2.6.37.
>
> Thanks,
>
> jon
> ---
> Add a driver for the video capture port on VIA integrated chipsets. This
> version has a remaining OLPCism or two and expects to be talking to an
> ov7670; those can be improved as the need arises.
>
> This work was supported by the One Laptop Per Child project.
> Thanks to Laurent Pinchart for a number of useful comments.
>
> Cc: Florian Tobias Schandinat <FlorianSchandinat@gmx.de>
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
> drivers/media/video/Kconfig | 10 +
> drivers/media/video/Makefile | 2 +
> drivers/media/video/via-camera.c | 1406 ++++++++++++++++++++++++++++++++++++++
> drivers/media/video/via-camera.h | 93 +++
> drivers/video/via/accel.c | 2 +-
> drivers/video/via/via-core.c | 16 +-
> include/linux/via-core.h | 4 +-
> include/media/v4l2-chip-ident.h | 4 +
> 8 files changed, 1533 insertions(+), 4 deletions(-)
> create mode 100644 drivers/media/video/via-camera.c
> create mode 100644 drivers/media/video/via-camera.h
>
> diff --git a/drivers/media/video/Kconfig b/drivers/media/video/Kconfig
> index f6e4d04..b62d6b0 100644
> --- a/drivers/media/video/Kconfig
> +++ b/drivers/media/video/Kconfig
> @@ -774,6 +774,16 @@ config VIDEO_CAFE_CCIC
> CMOS camera controller. This is the controller found on first-
> generation OLPC systems.
>
> +config VIDEO_VIA_CAMERA
> + tristate "VIAFB camera controller support"
> + depends on FB_VIA
> + select VIDEOBUF_DMA_SG
> + select VIDEO_OV7670
> + help
> + Driver support for the integrated camera controller in VIA
> + Chrome9 chipsets. Currently only tested on OLPC xo-1.5 systems
> + with ov7670 sensors.
> +
> config SOC_CAMERA
> tristate "SoC camera support"
> depends on VIDEO_V4L2 && HAS_DMA && I2C
> diff --git a/drivers/media/video/Makefile b/drivers/media/video/Makefile
> index 40f98fb..b592804 100644
> --- a/drivers/media/video/Makefile
> +++ b/drivers/media/video/Makefile
> @@ -125,6 +125,8 @@ obj-$(CONFIG_VIDEO_CX2341X) += cx2341x.o
>
> obj-$(CONFIG_VIDEO_CAFE_CCIC) += cafe_ccic.o
>
> +obj-$(CONFIG_VIDEO_VIA_CAMERA) += via-camera.o
> +
> obj-$(CONFIG_USB_DABUSB) += dabusb.o
> obj-$(CONFIG_USB_SE401) += se401.o
> obj-$(CONFIG_USB_ZR364XX) += zr364xx.o
> diff --git a/drivers/media/video/via-camera.c b/drivers/media/video/via-camera.c
> new file mode 100644
> index 0000000..8e970a7
> --- /dev/null
> +++ b/drivers/media/video/via-camera.c
> @@ -0,0 +1,1406 @@
> +/*
> + * Driver for the VIA Chrome integrated camera controller.
> + *
> + * Copyright 2009,2010 Jonathan Corbet <corbet@lwn.net>
> + * Distributable under the terms of the GNU General Public License, version 2
> + *
> + * This work was supported by the One Laptop Per Child project
> + */
> +#include <linux/kernel.h>
> +#include <linux/module.h>
> +#include <linux/device.h>
> +#include <linux/list.h>
> +#include <linux/pci.h>
> +#include <linux/gpio.h>
> +#include <linux/interrupt.h>
> +#include <linux/pci.h>
> +#include <linux/platform_device.h>
> +#include <linux/videodev2.h>
> +#include <media/v4l2-device.h>
> +#include <media/v4l2-ioctl.h>
> +#include <media/v4l2-chip-ident.h>
> +#include <media/videobuf-dma-sg.h>
> +#include <linux/device.h>
> +#include <linux/delay.h>
> +#include <linux/dma-mapping.h>
> +#include <linux/pm_qos_params.h>
> +#include <linux/via-core.h>
> +#include <linux/via-gpio.h>
> +#include <linux/via_i2c.h>
> +
> +#include "via-camera.h"
> +
> +MODULE_AUTHOR("Jonathan Corbet <corbet@lwn.net>");
> +MODULE_DESCRIPTION("VIA framebuffer-based camera controller driver");
> +MODULE_LICENSE("GPL");
> +
> +static int flip_image;
> +module_param(flip_image, bool, 0444);
> +MODULE_PARM_DESC(flip_image,
> + "If set, the sensor will be instructed to flip the image "
> + "vertically.");
> +
> +#ifdef CONFIG_OLPC_XO_1_5
> +static int override_serial;
> +module_param(override_serial, bool, 0444);
> +MODULE_PARM_DESC(override_serial,
> + "The camera driver will normally refuse to load if "
> + "the XO 1.5 serial port is enabled. Set this option "
> + "to force the issue.");
> +#endif
> +
> +/*
> + * Basic window sizes.
> + */
> +#define VGA_WIDTH 640
> +#define VGA_HEIGHT 480
> +#define QCIF_WIDTH 176
> +#define QCIF_HEIGHT 144
> +
> +/*
> + * The structure describing our camera.
> + */
> +enum viacam_opstate { S_IDLE = 0, S_RUNNING = 1 };
> +
> +struct via_camera {
> + struct v4l2_device v4l2_dev;
> + struct video_device vdev;
> + struct v4l2_subdev *sensor;
> + struct platform_device *platdev;
> + struct viafb_dev *viadev;
> + struct mutex lock;
> + enum viacam_opstate opstate;
> + unsigned long flags;
> + struct pm_qos_request_list qos_request;
> + /*
> + * GPIO info for power/reset management
> + */
> + int power_gpio;
> + int reset_gpio;
> + /*
> + * I/O memory stuff.
> + */
> + void __iomem *mmio; /* Where the registers live */
> + void __iomem *fbmem; /* Frame buffer memory */
> + u32 fb_offset; /* Reserved memory offset (FB) */
> + /*
> + * Capture buffers and related. The controller supports
> + * up to three, so that's what we have here. These buffers
> + * live in frame buffer memory, so we don't call them "DMA".
> + */
> + unsigned int cb_offsets[3]; /* offsets into fb mem */
> + u8 *cb_addrs[3]; /* Kernel-space addresses */
> + int n_cap_bufs; /* How many are we using? */
> + int next_buf;
> + struct videobuf_queue vb_queue;
> + struct list_head buffer_queue; /* prot. by reg_lock */
> + /*
> + * User tracking.
> + */
> + int users;
> + struct file *owner;
> + /*
> + * Video format information. sensor_format is kept in a form
> + * that we can use to pass to the sensor. We always run the
> + * sensor in VGA resolution, though, and let the controller
> + * downscale things if need be. So we keep the "real*
> + * dimensions separately.
> + */
> + struct v4l2_pix_format sensor_format;
> + struct v4l2_pix_format user_format;
> +};
> +
> +/*
> + * Yes, this is a hack, but there's only going to be one of these
> + * on any system we know of.
> + */
> +static struct via_camera *via_cam_info;
> +
> +/*
> + * Flag values, manipulated with bitops
> + */
> +#define CF_DMA_ACTIVE 0 /* A frame is incoming */
> +#define CF_CONFIG_NEEDED 1 /* Must configure hardware */
> +
> +
> +/*
> + * Nasty ugly v4l2 boilerplate.
> + */
> +#define sensor_call(cam, optype, func, args...) \
> + v4l2_subdev_call(cam->sensor, optype, func, ##args)
> +
> +/*
> + * Debugging and related.
> + */
> +#define cam_err(cam, fmt, arg...) \
> + dev_err(&(cam)->platdev->dev, fmt, ##arg);
> +#define cam_warn(cam, fmt, arg...) \
> + dev_warn(&(cam)->platdev->dev, fmt, ##arg);
> +#define cam_dbg(cam, fmt, arg...) \
> + dev_dbg(&(cam)->platdev->dev, fmt, ##arg);
> +
> +
> +/*--------------------------------------------------------------------------*/
> +/*
> + * Sensor power/reset management. This piece is OLPC-specific for
> + * sure; other configurations will have things connected differently.
> + */
> +static int via_sensor_power_setup(struct via_camera *cam)
> +{
> + int ret;
> +
> + cam->power_gpio = viafb_gpio_lookup("VGPIO3");
> + cam->reset_gpio = viafb_gpio_lookup("VGPIO2");
> + if (cam->power_gpio < 0 || cam->reset_gpio < 0) {
> + dev_err(&cam->platdev->dev, "Unable to find GPIO lines\n");
> + return -EINVAL;
> + }
> + ret = gpio_request(cam->power_gpio, "viafb-camera");
> + if (ret) {
> + dev_err(&cam->platdev->dev, "Unable to request power GPIO\n");
> + return ret;
> + }
> + ret = gpio_request(cam->reset_gpio, "viafb-camera");
> + if (ret) {
> + dev_err(&cam->platdev->dev, "Unable to request reset GPIO\n");
> + gpio_free(cam->power_gpio);
> + return ret;
> + }
> + gpio_direction_output(cam->power_gpio, 0);
> + gpio_direction_output(cam->reset_gpio, 0);
> + return 0;
> +}
> +
> +/*
> + * Power up the sensor and perform the reset dance.
> + */
> +static void via_sensor_power_up(struct via_camera *cam)
> +{
> + gpio_set_value(cam->power_gpio, 1);
> + gpio_set_value(cam->reset_gpio, 0);
> + msleep(20); /* Probably excessive */
> + gpio_set_value(cam->reset_gpio, 1);
> + msleep(20);
> +}
> +
> +static void via_sensor_power_down(struct via_camera *cam)
> +{
> + gpio_set_value(cam->power_gpio, 0);
> + gpio_set_value(cam->reset_gpio, 0);
> +}
> +
> +
> +static void via_sensor_power_release(struct via_camera *cam)
> +{
> + via_sensor_power_down(cam);
> + gpio_free(cam->power_gpio);
> + gpio_free(cam->reset_gpio);
> +}
> +
> +/* --------------------------------------------------------------------------*/
> +/* Sensor ops */
> +
> +/*
> + * Manage the ov7670 "flip" bit, which needs special help.
> + */
> +static int viacam_set_flip(struct via_camera *cam)
> +{
> + struct v4l2_control ctrl;
> +
> + memset(&ctrl, 0, sizeof(ctrl));
> + ctrl.id = V4L2_CID_VFLIP;
> + ctrl.value = flip_image;
> + return sensor_call(cam, core, s_ctrl, &ctrl);
> +}
> +
> +/*
> + * Configure the sensor. It's up to the caller to ensure
> + * that the camera is in the correct operating state.
> + */
> +static int viacam_configure_sensor(struct via_camera *cam)
> +{
> + struct v4l2_format fmt;
> + int ret;
> +
> + fmt.fmt.pix = cam->sensor_format;
> + ret = sensor_call(cam, core, init, 0);
> + if (ret == 0)
> + ret = sensor_call(cam, video, s_fmt, &fmt);
> + /*
> + * OV7670 does weird things if flip is set *before* format...
> + */
> + if (ret == 0)
> + ret = viacam_set_flip(cam);
> + return ret;
> +}
> +
> +
> +
> +/* --------------------------------------------------------------------------*/
> +/*
> + * Some simple register accessors; they assume that the lock is held.
> + *
> + * Should we want to support the second capture engine, we could
> + * hide the register difference by adding 0x1000 to registers in the
> + * 0x300-350 range.
> + */
> +static inline void viacam_write_reg(struct via_camera *cam,
> + int reg, int value)
> +{
> + iowrite32(value, cam->mmio + reg);
> +}
> +
> +static inline int viacam_read_reg(struct via_camera *cam, int reg)
> +{
> + return ioread32(cam->mmio + reg);
> +}
> +
> +static inline void viacam_write_reg_mask(struct via_camera *cam,
> + int reg, int value, int mask)
> +{
> + int tmp = viacam_read_reg(cam, reg);
> +
> + tmp = (tmp & ~mask) | (value & mask);
> + viacam_write_reg(cam, reg, tmp);
> +}
> +
> +
> +/* --------------------------------------------------------------------------*/
> +/* Interrupt management and handling */
> +
> +static irqreturn_t viacam_quick_irq(int irq, void *data)
> +{
> + struct via_camera *cam = data;
> + irqreturn_t ret = IRQ_NONE;
> + int icv;
> +
> + /*
> + * All we do here is to clear the interrupts and tell
> + * the handler thread to wake up.
> + */
> + spin_lock(&cam->viadev->reg_lock);
> + icv = viacam_read_reg(cam, VCR_INTCTRL);
> + if (icv & VCR_IC_EAV) {
> + icv |= VCR_IC_EAV|VCR_IC_EVBI|VCR_IC_FFULL;
> + viacam_write_reg(cam, VCR_INTCTRL, icv);
> + ret = IRQ_WAKE_THREAD;
> + }
> + spin_unlock(&cam->viadev->reg_lock);
> + return ret;
> +}
> +
> +/*
> + * Find the next videobuf buffer which has somebody waiting on it.
> + */
> +static struct videobuf_buffer *viacam_next_buffer(struct via_camera *cam)
> +{
> + unsigned long flags;
> + struct videobuf_buffer *buf = NULL;
> +
> + spin_lock_irqsave(&cam->viadev->reg_lock, flags);
> + if (cam->opstate != S_RUNNING)
> + goto out;
> + if (list_empty(&cam->buffer_queue))
> + goto out;
> + buf = list_entry(cam->buffer_queue.next, struct videobuf_buffer, queue);
> + if (!waitqueue_active(&buf->done)) {/* Nobody waiting */
> + buf = NULL;
> + goto out;
> + }
> + list_del(&buf->queue);
> + buf->state = VIDEOBUF_ACTIVE;
> +out:
> + spin_unlock_irqrestore(&cam->viadev->reg_lock, flags);
> + return buf;
> +}
> +
> +/*
> + * The threaded IRQ handler.
> + */
> +static irqreturn_t viacam_irq(int irq, void *data)
> +{
> + int bufn;
> + struct videobuf_buffer *vb;
> + struct via_camera *cam = data;
> + struct videobuf_dmabuf *vdma;
> +
> + /*
> + * If there is no place to put the data frame, don't bother
> + * with anything else.
> + */
> + vb = viacam_next_buffer(cam);
> + if (vb == NULL)
> + goto done;
> + /*
> + * Figure out which buffer we just completed.
> + */
> + bufn = (viacam_read_reg(cam, VCR_INTCTRL) & VCR_IC_ACTBUF) >> 3;
> + bufn -= 1;
> + if (bufn < 0)
> + bufn = cam->n_cap_bufs - 1;
> + /*
> + * Copy over the data and let any waiters know.
> + */
> + vdma = videobuf_to_dma(vb);
> + viafb_dma_copy_out_sg(cam->cb_offsets[bufn], vdma->sglist, vdma->sglen);
> + vb->state = VIDEOBUF_DONE;
> + vb->size = cam->user_format.sizeimage;
> + wake_up(&vb->done);
> +done:
> + return IRQ_HANDLED;
> +}
> +
> +
> +/*
> + * These functions must mess around with the general interrupt
> + * control register, which is relevant to much more than just the
> + * camera. Nothing else uses interrupts, though, as of this writing.
> + * Should that situation change, we'll have to improve support at
> + * the via-core level.
> + */
> +static void viacam_int_enable(struct via_camera *cam)
> +{
> + viacam_write_reg(cam, VCR_INTCTRL,
> + VCR_IC_INTEN|VCR_IC_EAV|VCR_IC_EVBI|VCR_IC_FFULL);
> + viafb_irq_enable(VDE_I_C0AVEN);
> +}
> +
> +static void viacam_int_disable(struct via_camera *cam)
> +{
> + viafb_irq_disable(VDE_I_C0AVEN);
> + viacam_write_reg(cam, VCR_INTCTRL, 0);
> +}
> +
> +
> +
> +/* --------------------------------------------------------------------------*/
> +/* Controller operations */
> +
> +/*
> + * Set up our capture buffers in framebuffer memory.
> + */
> +static int viacam_ctlr_cbufs(struct via_camera *cam)
> +{
> + int nbuf = cam->viadev->camera_fbmem_size/cam->sensor_format.sizeimage;
> + int i;
> + unsigned int offset;
> +
> + /*
> + * See how many buffers we can work with.
> + */
> + if (nbuf >= 3) {
> + cam->n_cap_bufs = 3;
> + viacam_write_reg_mask(cam, VCR_CAPINTC, VCR_CI_3BUFS,
> + VCR_CI_3BUFS);
> + } else if (nbuf == 2) {
> + cam->n_cap_bufs = 2;
> + viacam_write_reg_mask(cam, VCR_CAPINTC, 0, VCR_CI_3BUFS);
> + } else {
> + cam_warn(cam, "Insufficient frame buffer memory\n");
> + return -ENOMEM;
> + }
> + /*
> + * Set them up.
> + */
> + offset = cam->fb_offset;
> + for (i = 0; i < cam->n_cap_bufs; i++) {
> + cam->cb_offsets[i] = offset;
> + cam->cb_addrs[i] = cam->fbmem + offset;
> + viacam_write_reg(cam, VCR_VBUF1 + i*4, offset & VCR_VBUF_MASK);
> + offset += cam->sensor_format.sizeimage;
> + }
> + return 0;
> +}
> +
> +/*
> + * Set the scaling register for downscaling the image.
> + *
> + * This register works like this... Vertical scaling is enabled
> + * by bit 26; if that bit is set, downscaling is controlled by the
> + * value in bits 16:25. Those bits are divided by 1024 to get
> + * the scaling factor; setting just bit 25 thus cuts the height
> + * in half.
> + *
> + * Horizontal scaling works about the same, but it's enabled by
> + * bit 11, with bits 0:10 giving the numerator of a fraction
> + * (over 2048) for the scaling value.
> + *
> + * This function is naive in that, if the user departs from
> + * the 3x4 VGA scaling factor, the image will distort. We
> + * could work around that if it really seemed important.
> + */
> +static void viacam_set_scale(struct via_camera *cam)
> +{
> + unsigned int avscale;
> + int sf;
> +
> + if (cam->user_format.width == VGA_WIDTH)
> + avscale = 0;
> + else {
> + sf = (cam->user_format.width*2048)/VGA_WIDTH;
> + avscale = VCR_AVS_HEN | sf;
> + }
> + if (cam->user_format.height < VGA_HEIGHT) {
> + sf = (1024*cam->user_format.height)/VGA_HEIGHT;
> + avscale |= VCR_AVS_VEN | (sf << 16);
> + }
> + viacam_write_reg(cam, VCR_AVSCALE, avscale);
> +}
> +
> +
> +/*
> + * Configure image-related information into the capture engine.
> + */
> +static void viacam_ctlr_image(struct via_camera *cam)
> +{
> + int cicreg;
> +
> + /*
> + * Disable clock before messing with stuff - from the via
> + * sample driver.
> + */
> + viacam_write_reg(cam, VCR_CAPINTC, ~(VCR_CI_ENABLE|VCR_CI_CLKEN));
> + /*
> + * Set up the controller for VGA resolution, modulo magic
> + * offsets from the via sample driver.
> + */
> + viacam_write_reg(cam, VCR_HORRANGE, 0x06200120);
> + viacam_write_reg(cam, VCR_VERTRANGE, 0x01de0000);
> + viacam_set_scale(cam);
> + /*
> + * Image size info.
> + */
> + viacam_write_reg(cam, VCR_MAXDATA,
> + (cam->sensor_format.height << 16) |
> + (cam->sensor_format.bytesperline >> 3));
> + viacam_write_reg(cam, VCR_MAXVBI, 0);
> + viacam_write_reg(cam, VCR_VSTRIDE,
> + cam->user_format.bytesperline & VCR_VS_STRIDE);
> + /*
> + * Set up the capture interface control register,
> + * everything but the "go" bit.
> + *
> + * The FIFO threshold is a bit of a magic number; 8 is what
> + * VIA's sample code uses.
> + */
> + cicreg = VCR_CI_CLKEN |
> + 0x08000000 | /* FIFO threshold */
> + VCR_CI_FLDINV | /* OLPC-specific? */
> + VCR_CI_VREFINV | /* OLPC-specific? */
> + VCR_CI_DIBOTH | /* Capture both fields */
> + VCR_CI_CCIR601_8;
> + if (cam->n_cap_bufs == 3)
> + cicreg |= VCR_CI_3BUFS;
> + /*
> + * YUV formats need different byte swapping than RGB.
> + */
> + if (cam->user_format.pixelformat == V4L2_PIX_FMT_YUYV)
> + cicreg |= VCR_CI_YUYV;
> + else
> + cicreg |= VCR_CI_UYVY;
> + viacam_write_reg(cam, VCR_CAPINTC, cicreg);
> +}
> +
> +
> +static int viacam_config_controller(struct via_camera *cam)
> +{
> + int ret;
> + unsigned long flags;
> +
> + spin_lock_irqsave(&cam->viadev->reg_lock, flags);
> + ret = viacam_ctlr_cbufs(cam);
> + if (!ret)
> + viacam_ctlr_image(cam);
> + spin_unlock_irqrestore(&cam->viadev->reg_lock, flags);
> + clear_bit(CF_CONFIG_NEEDED, &cam->flags);
> + return ret;
> +}
> +
> +/*
> + * Make it start grabbing data.
> + */
> +static void viacam_start_engine(struct via_camera *cam)
> +{
> + spin_lock_irq(&cam->viadev->reg_lock);
> + cam->next_buf = 0;
> + viacam_write_reg_mask(cam, VCR_CAPINTC, VCR_CI_ENABLE, VCR_CI_ENABLE);
> + viacam_int_enable(cam);
> + (void) viacam_read_reg(cam, VCR_CAPINTC); /* Force post */
> + cam->opstate = S_RUNNING;
> + spin_unlock_irq(&cam->viadev->reg_lock);
> +}
> +
> +
> +static void viacam_stop_engine(struct via_camera *cam)
> +{
> + spin_lock_irq(&cam->viadev->reg_lock);
> + viacam_int_disable(cam);
> + viacam_write_reg_mask(cam, VCR_CAPINTC, 0, VCR_CI_ENABLE);
> + (void) viacam_read_reg(cam, VCR_CAPINTC); /* Force post */
> + cam->opstate = S_IDLE;
> + spin_unlock_irq(&cam->viadev->reg_lock);
> +}
> +
> +
> +/* --------------------------------------------------------------------------*/
> +/* Videobuf callback ops */
> +
> +/*
> + * buffer_setup. The purpose of this one would appear to be to tell
> + * videobuf how big a single image is. It's also evidently up to us
> + * to put some sort of limit on the maximum number of buffers allowed.
> + */
> +static int viacam_vb_buf_setup(struct videobuf_queue *q,
> + unsigned int *count, unsigned int *size)
> +{
> + struct via_camera *cam = q->priv_data;
> +
> + *size = cam->user_format.sizeimage;
> + if (*count == 0 || *count > 6) /* Arbitrary number */
> + *count = 6;
> + return 0;
> +}
> +
> +/*
> + * Prepare a buffer.
> + */
> +static int viacam_vb_buf_prepare(struct videobuf_queue *q,
> + struct videobuf_buffer *vb, enum v4l2_field field)
> +{
> + struct via_camera *cam = q->priv_data;
> +
> + vb->size = cam->user_format.sizeimage;
> + vb->width = cam->user_format.width; /* bytesperline???? */
> + vb->height = cam->user_format.height;
> + vb->field = field;
> + if (vb->state == VIDEOBUF_NEEDS_INIT) {
> + int ret = videobuf_iolock(q, vb, NULL);
> + if (ret)
> + return ret;
> + }
> + vb->state = VIDEOBUF_PREPARED;
> + return 0;
> +}
> +
> +/*
> + * We've got a buffer to put data into.
> + *
> + * FIXME: check for a running engine and valid buffers?
> + */
> +static void viacam_vb_buf_queue(struct videobuf_queue *q,
> + struct videobuf_buffer *vb)
> +{
> + struct via_camera *cam = q->priv_data;
> +
> + /*
> + * Note that videobuf holds the lock when it calls
> + * us, so we need not (indeed, cannot) take it here.
> + */
> + vb->state = VIDEOBUF_QUEUED;
> + list_add_tail(&vb->queue, &cam->buffer_queue);
> +}
> +
> +/*
> + * Free a buffer.
> + */
> +static void viacam_vb_buf_release(struct videobuf_queue *q,
> + struct videobuf_buffer *vb)
> +{
> + struct via_camera *cam = q->priv_data;
> +
> + videobuf_dma_unmap(&cam->platdev->dev, videobuf_to_dma(vb));
> + videobuf_dma_free(videobuf_to_dma(vb));
> + vb->state = VIDEOBUF_NEEDS_INIT;
> +}
> +
> +static const struct videobuf_queue_ops viacam_vb_ops = {
> + .buf_setup = viacam_vb_buf_setup,
> + .buf_prepare = viacam_vb_buf_prepare,
> + .buf_queue = viacam_vb_buf_queue,
> + .buf_release = viacam_vb_buf_release,
> +};
> +
> +/* --------------------------------------------------------------------------*/
> +/* File operations */
> +
> +static int viacam_open(struct file *filp)
> +{
> + struct via_camera *cam = video_drvdata(filp);
> +
> + filp->private_data = cam;
> + /*
> + * Note the new user. If this is the first one, we'll also
> + * need to power up the sensor.
> + */
> + mutex_lock(&cam->lock);
> + if (cam->users == 0) {
> + int ret = viafb_request_dma();
> +
> + if (ret) {
> + mutex_unlock(&cam->lock);
> + return ret;
> + }
> + via_sensor_power_up(cam);
> + set_bit(CF_CONFIG_NEEDED, &cam->flags);
> + /*
> + * Hook into videobuf. Evidently this cannot fail.
> + */
> + videobuf_queue_sg_init(&cam->vb_queue, &viacam_vb_ops,
> + &cam->platdev->dev, &cam->viadev->reg_lock,
> + V4L2_BUF_TYPE_VIDEO_CAPTURE, V4L2_FIELD_NONE,
> + sizeof(struct videobuf_buffer), cam);
> + }
> + (cam->users)++;
> + mutex_unlock(&cam->lock);
> + return 0;
> +}
> +
> +static int viacam_release(struct file *filp)
> +{
> + struct via_camera *cam = video_drvdata(filp);
> +
> + mutex_lock(&cam->lock);
> + (cam->users)--;
> + /*
> + * If the "owner" is closing, shut down any ongoing
> + * operations.
> + */
> + if (filp == cam->owner) {
> + videobuf_stop(&cam->vb_queue);
> + /*
> + * We don't hold the spinlock here, but, if release()
> + * is being called by the owner, nobody else will
> + * be changing the state. And an extra stop would
> + * not hurt anyway.
> + */
> + if (cam->opstate != S_IDLE)
> + viacam_stop_engine(cam);
> + cam->owner = NULL;
> + }
> + /*
> + * Last one out needs to turn out the lights.
> + */
> + if (cam->users == 0) {
> + videobuf_mmap_free(&cam->vb_queue);
> + via_sensor_power_down(cam);
> + viafb_release_dma();
> + }
> + mutex_unlock(&cam->lock);
> + return 0;
> +}
> +
> +/*
> + * Read a frame from the device.
> + */
> +static ssize_t viacam_read(struct file *filp, char __user *buffer,
> + size_t len, loff_t *pos)
> +{
> + struct via_camera *cam = video_drvdata(filp);
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + /*
> + * Enforce the V4l2 "only one owner gets to read data" rule.
> + */
> + if (cam->owner && cam->owner != filp) {
> + ret = -EBUSY;
> + goto out_unlock;
> + }
> + cam->owner = filp;
> + /*
> + * Do we need to configure the hardware?
> + */
> + if (test_bit(CF_CONFIG_NEEDED, &cam->flags)) {
> + ret = viacam_configure_sensor(cam);
> + if (!ret)
> + ret = viacam_config_controller(cam);
> + if (ret)
> + goto out_unlock;
> + }
> + /*
> + * Fire up the capture engine, then have videobuf do
> + * the heavy lifting. Someday it would be good to avoid
> + * stopping and restarting the engine each time.
> + */
> + INIT_LIST_HEAD(&cam->buffer_queue);
> + viacam_start_engine(cam);
> + ret = videobuf_read_stream(&cam->vb_queue, buffer, len, pos, 0,
> + filp->f_flags & O_NONBLOCK);
> + viacam_stop_engine(cam);
> + /* videobuf_stop() ?? */
> +
> +out_unlock:
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +
> +static unsigned int viacam_poll(struct file *filp, struct poll_table_struct *pt)
> +{
> + struct via_camera *cam = video_drvdata(filp);
> +
> + return videobuf_poll_stream(filp, &cam->vb_queue, pt);
> +}
> +
> +
> +static int viacam_mmap(struct file *filp, struct vm_area_struct *vma)
> +{
> + struct via_camera *cam = video_drvdata(filp);
> +
> + return videobuf_mmap_mapper(&cam->vb_queue, vma);
> +}
> +
> +
> +
> +static const struct v4l2_file_operations viacam_fops = {
> + .owner = THIS_MODULE,
> + .open = viacam_open,
> + .release = viacam_release,
> + .read = viacam_read,
> + .poll = viacam_poll,
> + .mmap = viacam_mmap,
> + .ioctl = video_ioctl2,
> +};
> +
> +/*----------------------------------------------------------------------------*/
> +/*
> + * The long list of v4l2 ioctl ops
> + */
> +
> +static int viacam_g_chip_ident(struct file *file, void *priv,
> + struct v4l2_dbg_chip_ident *ident)
> +{
> + struct via_camera *cam = priv;
> +
> + ident->ident = V4L2_IDENT_NONE;
> + ident->revision = 0;
> + if (v4l2_chip_match_host(&ident->match)) {
> + ident->ident = V4L2_IDENT_VIA_VX855;
> + return 0;
> + }
> + return sensor_call(cam, core, g_chip_ident, ident);
> +}
> +
> +/*
> + * Control ops are passed through to the sensor.
> + */
> +static int viacam_queryctrl(struct file *filp, void *priv,
> + struct v4l2_queryctrl *qc)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, core, queryctrl, qc);
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +
> +static int viacam_g_ctrl(struct file *filp, void *priv,
> + struct v4l2_control *ctrl)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, core, g_ctrl, ctrl);
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +
> +static int viacam_s_ctrl(struct file *filp, void *priv,
> + struct v4l2_control *ctrl)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, core, s_ctrl, ctrl);
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +/*
> + * Only one input.
> + */
> +static int viacam_enum_input(struct file *filp, void *priv,
> + struct v4l2_input *input)
> +{
> + if (input->index != 0)
> + return -EINVAL;
> +
> + input->type = V4L2_INPUT_TYPE_CAMERA;
> + input->std = V4L2_STD_ALL; /* Not sure what should go here */
> + strcpy(input->name, "Camera");
> + return 0;
> +}
> +
> +static int viacam_g_input(struct file *filp, void *priv, unsigned int *i)
> +{
> + *i = 0;
> + return 0;
> +}
> +
> +static int viacam_s_input(struct file *filp, void *priv, unsigned int i)
> +{
> + if (i != 0)
> + return -EINVAL;
> + return 0;
> +}
> +
> +static int viacam_s_std(struct file *filp, void *priv, v4l2_std_id *std)
> +{
> + return 0;
> +}
> +
> +/*
> + * Video format stuff. Here is our default format until
> + * user space messes with things.
> + */
> +static const struct v4l2_pix_format viacam_def_pix_format = {
> + .width = VGA_WIDTH,
> + .height = VGA_HEIGHT,
> + .pixelformat = V4L2_PIX_FMT_YUYV,
> + .field = V4L2_FIELD_NONE,
> + .bytesperline = VGA_WIDTH * 2,
> + .sizeimage = VGA_WIDTH * VGA_HEIGHT * 2,
> +};
> +
> +static int viacam_enum_fmt_vid_cap(struct file *filp, void *priv,
> + struct v4l2_fmtdesc *fmt)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, video, enum_fmt, fmt);
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +/*
> + * Figure out proper image dimensions, but always force the
> + * sensor to VGA.
> + */
> +static void viacam_fmt_pre(struct v4l2_pix_format *userfmt,
> + struct v4l2_pix_format *sensorfmt)
> +{
> + *sensorfmt = *userfmt;
> + if (userfmt->width < QCIF_WIDTH || userfmt->height < QCIF_HEIGHT) {
> + userfmt->width = QCIF_WIDTH;
> + userfmt->height = QCIF_HEIGHT;
> + }
> + if (userfmt->width > VGA_WIDTH || userfmt->height > VGA_HEIGHT) {
> + userfmt->width = VGA_WIDTH;
> + userfmt->height = VGA_HEIGHT;
> + }
> + sensorfmt->width = VGA_WIDTH;
> + sensorfmt->height = VGA_HEIGHT;
> +}
> +
> +static void viacam_fmt_post(struct v4l2_pix_format *userfmt,
> + struct v4l2_pix_format *sensorfmt)
> +{
> + userfmt->pixelformat = sensorfmt->pixelformat;
> + userfmt->field = sensorfmt->field;
> + userfmt->bytesperline = 2 * userfmt->width;
> + userfmt->sizeimage = userfmt->bytesperline * userfmt->height;
> +}
> +
> +static int viacam_try_fmt_vid_cap(struct file *filp, void *priv,
> + struct v4l2_format *fmt)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> + struct v4l2_format sfmt;
> +
> + viacam_fmt_pre(&fmt->fmt.pix, &sfmt.fmt.pix);
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, video, try_fmt, &sfmt);
> + mutex_unlock(&cam->lock);
> + viacam_fmt_post(&fmt->fmt.pix, &sfmt.fmt.pix);
> + return ret;
> +}
> +
> +static int viacam_g_fmt_vid_cap(struct file *filp, void *priv,
> + struct v4l2_format *fmt)
> +{
> + struct via_camera *cam = priv;
> +
> + mutex_lock(&cam->lock);
> + fmt->fmt.pix = cam->user_format;
> + mutex_unlock(&cam->lock);
> + return 0;
> +}
> +
> +static int viacam_s_fmt_vid_cap(struct file *filp, void *priv,
> + struct v4l2_format *fmt)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> + struct v4l2_format sfmt;
> +
> + /*
> + * Camera must be idle or we can't mess with the
> + * video setup.
> + */
> + mutex_lock(&cam->lock);
> + if (cam->opstate != S_IDLE) {
> + ret = -EBUSY;
> + goto out;
> + }
> + /*
> + * Let the sensor code look over and tweak the
> + * requested formatting.
> + */
> + viacam_fmt_pre(&fmt->fmt.pix, &sfmt.fmt.pix);
> + ret = sensor_call(cam, video, try_fmt, &sfmt);
> + if (ret)
> + goto out;
> + viacam_fmt_post(&fmt->fmt.pix, &sfmt.fmt.pix);
> + /*
> + * OK, let's commit to the new format.
> + */
> + cam->user_format = fmt->fmt.pix;
> + cam->sensor_format = sfmt.fmt.pix;
> + ret = viacam_configure_sensor(cam);
> + if (!ret)
> + ret = viacam_config_controller(cam);
> +out:
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +static int viacam_querycap(struct file *filp, void *priv,
> + struct v4l2_capability *cap)
> +{
> + strcpy(cap->driver, "via-camera");
> + strcpy(cap->card, "via-camera");
> + cap->version = 1;
> + cap->capabilities = V4L2_CAP_VIDEO_CAPTURE |
> + V4L2_CAP_READWRITE | V4L2_CAP_STREAMING;
> + return 0;
> +}
> +
> +/*
> + * Streaming operations - pure videobuf stuff.
> + */
> +static int viacam_reqbufs(struct file *filp, void *priv,
> + struct v4l2_requestbuffers *rb)
> +{
> + struct via_camera *cam = priv;
> +
> + return videobuf_reqbufs(&cam->vb_queue, rb);
> +}
> +
> +static int viacam_querybuf(struct file *filp, void *priv,
> + struct v4l2_buffer *buf)
> +{
> + struct via_camera *cam = priv;
> +
> + return videobuf_querybuf(&cam->vb_queue, buf);
> +}
> +
> +static int viacam_qbuf(struct file *filp, void *priv, struct v4l2_buffer *buf)
> +{
> + struct via_camera *cam = priv;
> +
> + return videobuf_qbuf(&cam->vb_queue, buf);
> +}
> +
> +static int viacam_dqbuf(struct file *filp, void *priv, struct v4l2_buffer *buf)
> +{
> + struct via_camera *cam = priv;
> +
> + return videobuf_dqbuf(&cam->vb_queue, buf, filp->f_flags & O_NONBLOCK);
> +}
> +
> +static int viacam_streamon(struct file *filp, void *priv, enum v4l2_buf_type t)
> +{
> + struct via_camera *cam = priv;
> + int ret = 0;
> +
> + if (t != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> +
> + mutex_lock(&cam->lock);
> + if (cam->opstate != S_IDLE) {
> + ret = -EBUSY;
> + goto out;
> + }
> + /*
> + * Enforce the V4l2 "only one owner gets to read data" rule.
> + */
> + if (cam->owner && cam->owner != filp) {
> + ret = -EBUSY;
> + goto out;
> + }
> + cam->owner = filp;
> + /*
> + * Configure things if need be.
> + */
> + if (test_bit(CF_CONFIG_NEEDED, &cam->flags)) {
> + ret = viacam_configure_sensor(cam);
> + if (ret)
> + goto out;
> + ret = viacam_config_controller(cam);
> + if (ret)
> + goto out;
> + }
> + /*
> + * If the CPU goes into C3, the DMA transfer gets corrupted and
> + * users start filing unsightly bug reports. Put in a "latency"
> + * requirement which will keep the CPU out of the deeper sleep
> + * states.
> + */
> + pm_qos_add_request(&cam->qos_request, PM_QOS_CPU_DMA_LATENCY, 50);
> + /*
> + * Fire things up.
> + */
> + INIT_LIST_HEAD(&cam->buffer_queue);
> + ret = videobuf_streamon(&cam->vb_queue);
> + if (!ret)
> + viacam_start_engine(cam);
> +out:
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +static int viacam_streamoff(struct file *filp, void *priv, enum v4l2_buf_type t)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + if (t != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> + return -EINVAL;
> + mutex_lock(&cam->lock);
> + if (cam->opstate != S_RUNNING) {
> + ret = -EINVAL;
> + goto out;
> + }
> + pm_qos_remove_request(&cam->qos_request);
> + viacam_stop_engine(cam);
> + /*
> + * Videobuf will recycle all of the outstanding buffers, but
> + * we should be sure we don't retain any references to
> + * any of them.
> + */
> + ret = videobuf_streamoff(&cam->vb_queue);
> + INIT_LIST_HEAD(&cam->buffer_queue);
> +out:
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +#ifdef CONFIG_VIDEO_V4L1_COMPAT
> +static int viacam_vidiocgmbuf(struct file *filp, void *priv,
> + struct video_mbuf *mbuf)
> +{
> + struct via_camera *cam = priv;
> +
> + return videobuf_cgmbuf(&cam->vb_queue, mbuf, 6);
> +}
> +#endif
> +
> +/* G/S_PARM */
> +
> +static int viacam_g_parm(struct file *filp, void *priv,
> + struct v4l2_streamparm *parm)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, video, g_parm, parm);
> + mutex_unlock(&cam->lock);
> + parm->parm.capture.readbuffers = cam->n_cap_bufs;
> + return ret;
> +}
> +
> +static int viacam_s_parm(struct file *filp, void *priv,
> + struct v4l2_streamparm *parm)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, video, s_parm, parm);
> + mutex_unlock(&cam->lock);
> + parm->parm.capture.readbuffers = cam->n_cap_bufs;
> + return ret;
> +}
> +
> +static int viacam_enum_framesizes(struct file *filp, void *priv,
> + struct v4l2_frmsizeenum *sizes)
> +{
> + if (sizes->index != 0)
> + return -EINVAL;
> + sizes->type = V4L2_FRMSIZE_TYPE_CONTINUOUS;
> + sizes->stepwise.min_width = QCIF_WIDTH;
> + sizes->stepwise.min_height = QCIF_HEIGHT;
> + sizes->stepwise.max_width = VGA_WIDTH;
> + sizes->stepwise.max_height = VGA_HEIGHT;
> + sizes->stepwise.step_width = sizes->stepwise.step_height = 1;
> + return 0;
> +}
> +
> +static int viacam_enum_frameintervals(struct file *filp, void *priv,
> + struct v4l2_frmivalenum *interval)
> +{
> + struct via_camera *cam = priv;
> + int ret;
> +
> + mutex_lock(&cam->lock);
> + ret = sensor_call(cam, video, enum_frameintervals, interval);
> + mutex_unlock(&cam->lock);
> + return ret;
> +}
> +
> +
> +
> +static const struct v4l2_ioctl_ops viacam_ioctl_ops = {
> + .vidioc_g_chip_ident = viacam_g_chip_ident,
> + .vidioc_queryctrl = viacam_queryctrl,
> + .vidioc_g_ctrl = viacam_g_ctrl,
> + .vidioc_s_ctrl = viacam_s_ctrl,
> + .vidioc_enum_input = viacam_enum_input,
> + .vidioc_g_input = viacam_g_input,
> + .vidioc_s_input = viacam_s_input,
> + .vidioc_s_std = viacam_s_std,
> + .vidioc_enum_fmt_vid_cap = viacam_enum_fmt_vid_cap,
> + .vidioc_try_fmt_vid_cap = viacam_try_fmt_vid_cap,
> + .vidioc_g_fmt_vid_cap = viacam_g_fmt_vid_cap,
> + .vidioc_s_fmt_vid_cap = viacam_s_fmt_vid_cap,
> + .vidioc_querycap = viacam_querycap,
> + .vidioc_reqbufs = viacam_reqbufs,
> + .vidioc_querybuf = viacam_querybuf,
> + .vidioc_qbuf = viacam_qbuf,
> + .vidioc_dqbuf = viacam_dqbuf,
> + .vidioc_streamon = viacam_streamon,
> + .vidioc_streamoff = viacam_streamoff,
> + .vidioc_g_parm = viacam_g_parm,
> + .vidioc_s_parm = viacam_s_parm,
> + .vidioc_enum_framesizes = viacam_enum_framesizes,
> + .vidioc_enum_frameintervals = viacam_enum_frameintervals,
> +#ifdef CONFIG_VIDEO_V4L1_COMPAT
> + .vidiocgmbuf = viacam_vidiocgmbuf,
> +#endif
> +};
> +
> +/*----------------------------------------------------------------------------*/
> +
> +/*
> + * Power management.
> + */
> +
> +/*
> + * Setup stuff.
> + */
> +
> +static struct video_device viacam_v4l_template = {
> + .name = "via-camera",
> + .minor = -1,
> + .tvnorms = V4L2_STD_NTSC_M,
> + .current_norm = V4L2_STD_NTSC_M,
> + .fops = &viacam_fops,
> + .ioctl_ops = &viacam_ioctl_ops,
> + .release = video_device_release_empty, /* Check this */
> +};
> +
> +
> +static __devinit int viacam_probe(struct platform_device *pdev)
> +{
> + int ret;
> + struct i2c_adapter *sensor_adapter;
> + struct viafb_dev *viadev = pdev->dev.platform_data;
> +
> + /*
> + * Note that there are actually two capture channels on
> + * the device. We only deal with one for now. That
> + * is encoded here; nothing else assumes it's dealing with
> + * a unique capture device.
> + */
> + struct via_camera *cam;
> +
> + /*
> + * Ensure that frame buffer memory has been set aside for
> + * this purpose. As an arbitrary limit, refuse to work
> + * with less than two frames of VGA 16-bit data.
> + *
> + * If we ever support the second port, we'll need to set
> + * aside more memory.
> + */
> + if (viadev->camera_fbmem_size < (VGA_HEIGHT*VGA_WIDTH*4)) {
> + printk(KERN_ERR "viacam: insufficient FB memory reserved\n");
> + return -ENOMEM;
> + }
> + if (viadev->engine_mmio == NULL) {
> + printk(KERN_ERR "viacam: No I/O memory, so no pictures\n");
> + return -ENOMEM;
> + }
> + /*
> + * Basic structure initialization.
> + */
> + cam = kzalloc (sizeof(struct via_camera), GFP_KERNEL);
> + if (cam == NULL)
> + return -ENOMEM;
> + via_cam_info = cam;
> + cam->platdev = pdev;
> + cam->viadev = viadev;
> + cam->users = 0;
> + cam->owner = NULL;
> + cam->opstate = S_IDLE;
> + cam->user_format = cam->sensor_format = viacam_def_pix_format;
> + mutex_init(&cam->lock);
> + INIT_LIST_HEAD(&cam->buffer_queue);
> + cam->mmio = viadev->engine_mmio;
> + cam->fbmem = viadev->fbmem;
> + cam->fb_offset = viadev->camera_fbmem_offset;
> + cam->flags = 1 << CF_CONFIG_NEEDED;
> + /*
> + * Tell V4L that we exist.
> + */
> + ret = v4l2_device_register(&pdev->dev, &cam->v4l2_dev);
> + if (ret) {
> + dev_err(&pdev->dev, "Unable to register v4l2 device\n");
> + return ret;
> + }
> + /*
> + * Convince the system that we can do DMA.
> + */
> + pdev->dev.dma_mask = &viadev->pdev->dma_mask;
> + dma_set_mask(&pdev->dev, 0xffffffff);
> + /*
> + * Fire up the capture port. The write to 0x78 looks purely
> + * OLPCish; any system will need to tweak 0x1e.
> + */
> + via_write_reg_mask(VIASR, 0x78, 0, 0x80);
> + via_write_reg_mask(VIASR, 0x1e, 0xc0, 0xc0);
> + /*
> + * Get the sensor powered up.
> + */
> + ret = via_sensor_power_setup(cam);
> + if (ret)
> + goto out_unregister;
> + via_sensor_power_up(cam);
> +
> + /*
> + * See if we can't find it on the bus. The VIA_PORT_31 assumption
> + * is OLPC-specific. 0x42 assumption is ov7670-specific.
> + */
> + sensor_adapter = viafb_find_i2c_adapter(VIA_PORT_31);
> + cam->sensor = v4l2_i2c_new_subdev(&cam->v4l2_dev, sensor_adapter,
> + "ov7670", "ov7670", 0x42 >> 1, NULL);
> + if (cam->sensor == NULL) {
> + dev_err(&pdev->dev, "Unable to find the sensor!\n");
> + ret = -ENODEV;
> + goto out_power_down;
> + }
> + /*
> + * Get the IRQ.
> + */
> + viacam_int_disable(cam);
> + ret = request_threaded_irq(viadev->pdev->irq, viacam_quick_irq,
> + viacam_irq, IRQF_SHARED, "via-camera", cam);
> + if (ret)
> + goto out_power_down;
> + /*
> + * Tell V4l2 that we exist.
> + */
> + cam->vdev = viacam_v4l_template;
> + cam->vdev.v4l2_dev = &cam->v4l2_dev;
> + ret = video_register_device(&cam->vdev, VFL_TYPE_GRABBER, -1);
> + if (ret)
> + goto out_irq;
> + video_set_drvdata(&cam->vdev, cam);
> +
> + /* Power the sensor down until somebody opens the device */
> + via_sensor_power_down(cam);
> + return 0;
> +
> +out_irq:
> + free_irq(viadev->pdev->irq, cam);
> +out_power_down:
> + via_sensor_power_release(cam);
> +out_unregister:
> + v4l2_device_unregister(&cam->v4l2_dev);
> + return ret;
> +}
> +
> +static __devexit int viacam_remove(struct platform_device *pdev)
> +{
> + struct via_camera *cam = via_cam_info;
> + struct viafb_dev *viadev = pdev->dev.platform_data;
> +
> + video_unregister_device(&cam->vdev);
> + v4l2_device_unregister(&cam->v4l2_dev);
> + free_irq(viadev->pdev->irq, cam);
> + via_sensor_power_release(cam);
> + via_cam_info = NULL;
> + return 0;
> +}
> +
> +
> +static struct platform_driver viacam_driver = {
> + .driver = {
> + .name = "viafb-camera",
> + },
> + .probe = viacam_probe,
> + .remove = viacam_remove,
> +};
> +
> +
> +#ifdef CONFIG_OLPC_XO_1_5
> +/*
> + * The OLPC folks put the serial port on the same pin as
> + * the camera. They also get grumpy if we break the
> + * serial port and keep them from using it. So we have
> + * to check the serial enable bit and not step on it.
> + */
> +#define VIACAM_SERIAL_DEVFN 0x88
> +#define VIACAM_SERIAL_CREG 0x46
> +#define VIACAM_SERIAL_BIT 0x40
> +
> +static __devinit int viacam_check_serial_port(void)
> +{
> + struct pci_bus *pbus = pci_find_bus(0, 0);
> + u8 cbyte;
> +
> + pci_bus_read_config_byte(pbus, VIACAM_SERIAL_DEVFN,
> + VIACAM_SERIAL_CREG, &cbyte);
> + if ((cbyte & VIACAM_SERIAL_BIT) == 0)
> + return 0; /* Not enabled */
> + if (override_serial == 0) {
> + printk(KERN_NOTICE "Via camera: serial port is enabled, " \
> + "refusing to load.\n");
> + printk(KERN_NOTICE "Specify override_serial=1 to force " \
> + "module loading.\n");
> + return -EBUSY;
> + }
> + printk(KERN_NOTICE "Via camera: overriding serial port\n");
> + pci_bus_write_config_byte(pbus, VIACAM_SERIAL_DEVFN,
> + VIACAM_SERIAL_CREG, cbyte & ~VIACAM_SERIAL_BIT);
> + return 0;
> +}
> +#endif
> +
> +
> +
> +
> +static int viacam_init(void)
> +{
> +#ifdef CONFIG_OLPC_XO_1_5
> + if (viacam_check_serial_port())
> + return -EBUSY;
> +#endif
> + return platform_driver_register(&viacam_driver);
> +}
> +module_init(viacam_init);
> +
> +static void viacam_exit(void)
> +{
> + platform_driver_unregister(&viacam_driver);
> +}
> +module_exit(viacam_exit);
> diff --git a/drivers/media/video/via-camera.h b/drivers/media/video/via-camera.h
> new file mode 100644
> index 0000000..b12a4b3
> --- /dev/null
> +++ b/drivers/media/video/via-camera.h
> @@ -0,0 +1,93 @@
> +/*
> + * VIA Camera register definitions.
> + */
> +#define VCR_INTCTRL 0x300 /* Capture interrupt control */
> +#define VCR_IC_EAV 0x0001 /* End of active video status */
> +#define VCR_IC_EVBI 0x0002 /* End of VBI status */
> +#define VCR_IC_FBOTFLD 0x0004 /* "flipping" Bottom field is active */
> +#define VCR_IC_ACTBUF 0x0018 /* Active video buffer */
> +#define VCR_IC_VSYNC 0x0020 /* 0 = VB, 1 = active video */
> +#define VCR_IC_BOTFLD 0x0040 /* Bottom field is active */
> +#define VCR_IC_FFULL 0x0080 /* FIFO full */
> +#define VCR_IC_INTEN 0x0100 /* End of active video int. enable */
> +#define VCR_IC_VBIINT 0x0200 /* End of VBI int enable */
> +#define VCR_IC_VBIBUF 0x0400 /* Current VBI buffer */
> +
> +#define VCR_TSC 0x308 /* Transport stream control */
> +#define VCR_TSC_ENABLE 0x000001 /* Transport stream input enable */
> +#define VCR_TSC_DROPERR 0x000002 /* Drop error packets */
> +#define VCR_TSC_METHOD 0x00000c /* DMA method (non-functional) */
> +#define VCR_TSC_COUNT 0x07fff0 /* KByte or packet count */
> +#define VCR_TSC_CBMODE 0x080000 /* Change buffer by byte count */
> +#define VCR_TSC_PSSIG 0x100000 /* Packet starting signal disable */
> +#define VCR_TSC_BE 0x200000 /* MSB first (serial mode) */
> +#define VCR_TSC_SERIAL 0x400000 /* Serial input (0 = parallel) */
> +
> +#define VCR_CAPINTC 0x310 /* Capture interface control */
> +#define VCR_CI_ENABLE 0x00000001 /* Capture enable */
> +#define VCR_CI_BSS 0x00000002 /* WTF "bit stream selection" */
> +#define VCR_CI_3BUFS 0x00000004 /* 1 = 3 buffers, 0 = 2 buffers */
> +#define VCR_CI_VIPEN 0x00000008 /* VIP enable */
> +#define VCR_CI_CCIR601_8 0 /* CCIR601 input stream, 8 bit */
> +#define VCR_CI_CCIR656_8 0x00000010 /* ... CCIR656, 8 bit */
> +#define VCR_CI_CCIR601_16 0x00000020 /* ... CCIR601, 16 bit */
> +#define VCR_CI_CCIR656_16 0x00000030 /* ... CCIR656, 16 bit */
> +#define VCR_CI_HDMODE 0x00000040 /* CCIR656-16 hdr decode mode; 1=16b */
> +#define VCR_CI_BSWAP 0x00000080 /* Swap bytes (16-bit) */
> +#define VCR_CI_YUYV 0 /* Byte order 0123 */
> +#define VCR_CI_UYVY 0x00000100 /* Byte order 1032 */
> +#define VCR_CI_YVYU 0x00000200 /* Byte order 0321 */
> +#define VCR_CI_VYUY 0x00000300 /* Byte order 3012 */
> +#define VCR_CI_VIPTYPE 0x00000400 /* VIP type */
> +#define VCR_CI_IFSEN 0x00000800 /* Input field signal enable */
> +#define VCR_CI_DIODD 0 /* De-interlace odd, 30fps */
> +#define VCR_CI_DIEVEN 0x00001000 /* ...even field, 30fps */
> +#define VCR_CI_DIBOTH 0x00002000 /* ...both fields, 60fps */
> +#define VCR_CI_DIBOTH30 0x00003000 /* ...both fields, 30fps interlace */
> +#define VCR_CI_CONVTYPE 0x00004000 /* 4:2:2 to 4:4:4; 1 = interpolate */
> +#define VCR_CI_CFC 0x00008000 /* Capture flipping control */
> +#define VCR_CI_FILTER 0x00070000 /* Horiz filter mode select
> + 000 = none
> + 001 = 2 tap
> + 010 = 3 tap
> + 011 = 4 tap
> + 100 = 5 tap */
> +#define VCR_CI_CLKINV 0x00080000 /* Input CLK inverted */
> +#define VCR_CI_VREFINV 0x00100000 /* VREF inverted */
> +#define VCR_CI_HREFINV 0x00200000 /* HREF inverted */
> +#define VCR_CI_FLDINV 0x00400000 /* Field inverted */
> +#define VCR_CI_CLKPIN 0x00800000 /* Capture clock pin */
> +#define VCR_CI_THRESH 0x0f000000 /* Capture fifo threshold */
> +#define VCR_CI_HRLE 0x10000000 /* Positive edge of HREF */
> +#define VCR_CI_VRLE 0x20000000 /* Positive edge of VREF */
> +#define VCR_CI_OFLDINV 0x40000000 /* Field output inverted */
> +#define VCR_CI_CLKEN 0x80000000 /* Capture clock enable */
> +
> +#define VCR_HORRANGE 0x314 /* Active video horizontal range */
> +#define VCR_VERTRANGE 0x318 /* Active video vertical range */
> +#define VCR_AVSCALE 0x31c /* Active video scaling control */
> +#define VCR_AVS_HEN 0x00000800 /* Horizontal scale enable */
> +#define VCR_AVS_VEN 0x04000000 /* Vertical enable */
> +#define VCR_VBIHOR 0x320 /* VBI Data horizontal range */
> +#define VCR_VBIVERT 0x324 /* VBI data vertical range */
> +#define VCR_VBIBUF1 0x328 /* First VBI buffer */
> +#define VCR_VBISTRIDE 0x32c /* VBI stride */
> +#define VCR_ANCDATACNT 0x330 /* Ancillary data count setting */
> +#define VCR_MAXDATA 0x334 /* Active data count of active video */
> +#define VCR_MAXVBI 0x338 /* Maximum data count of VBI */
> +#define VCR_CAPDATA 0x33c /* Capture data count */
> +#define VCR_VBUF1 0x340 /* First video buffer */
> +#define VCR_VBUF2 0x344 /* Second video buffer */
> +#define VCR_VBUF3 0x348 /* Third video buffer */
> +#define VCR_VBUF_MASK 0x1ffffff0 /* Bits 28:4 */
> +#define VCR_VBIBUF2 0x34c /* Second VBI buffer */
> +#define VCR_VSTRIDE 0x350 /* Stride of video + coring control */
> +#define VCR_VS_STRIDE_SHIFT 4
> +#define VCR_VS_STRIDE 0x00001ff0 /* Stride (8-byte units) */
> +#define VCR_VS_CCD 0x007f0000 /* Coring compare data */
> +#define VCR_VS_COREEN 0x00800000 /* Coring enable */
> +#define VCR_TS0ERR 0x354 /* TS buffer 0 error indicator */
> +#define VCR_TS1ERR 0x358 /* TS buffer 0 error indicator */
> +#define VCR_TS2ERR 0x35c /* TS buffer 0 error indicator */
> +
> +/* Add 0x1000 for the second capture engine registers */
> diff --git a/drivers/video/via/accel.c b/drivers/video/via/accel.c
> index e44893e..04bec05 100644
> --- a/drivers/video/via/accel.c
> +++ b/drivers/video/via/accel.c
> @@ -370,7 +370,7 @@ int viafb_init_engine(struct fb_info *info)
> viapar->shared->vq_vram_addr = viapar->fbmem_free;
> viapar->fbmem_used += VQ_SIZE;
>
> -#if defined(CONFIG_FB_VIA_CAMERA) || defined(CONFIG_FB_VIA_CAMERA_MODULE)
> +#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
> /*
> * Set aside a chunk of framebuffer memory for the camera
> * driver. Someday this driver probably needs a proper allocator
> diff --git a/drivers/video/via/via-core.c b/drivers/video/via/via-core.c
> index 66f4030..27d7260 100644
> --- a/drivers/video/via/via-core.c
> +++ b/drivers/video/via/via-core.c
> @@ -95,6 +95,13 @@ EXPORT_SYMBOL_GPL(viafb_irq_disable);
>
> /* ---------------------------------------------------------------------- */
> /*
> + * Currently, the camera driver is the only user of the DMA code, so we
> + * only compile it in if the camera driver is being built. Chances are,
> + * most viafb systems will not need to have this extra code for a while.
> + * As soon as another user comes long, the ifdef can be removed.
> + */
> +#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
> +/*
> * Access to the DMA engine. This currently provides what the camera
> * driver needs (i.e. outgoing only) but is easily expandable if need
> * be.
> @@ -322,7 +329,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct scatterlist *sg, int nsg)
> return 0;
> }
> EXPORT_SYMBOL_GPL(viafb_dma_copy_out_sg);
> -
> +#endif /* CONFIG_VIDEO_VIA_CAMERA */
>
> /* ---------------------------------------------------------------------- */
> /*
> @@ -507,7 +514,12 @@ static struct viafb_subdev_info {
> },
> {
> .name = "viafb-i2c",
> - }
> + },
> +#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
> + {
> + .name = "viafb-camera",
> + },
> +#endif
> };
> #define N_SUBDEVS ARRAY_SIZE(viafb_subdevs)
>
> diff --git a/include/linux/via-core.h b/include/linux/via-core.h
> index 7ffb521..38bffd8 100644
> --- a/include/linux/via-core.h
> +++ b/include/linux/via-core.h
> @@ -81,7 +81,7 @@ struct viafb_dev {
> unsigned long fbmem_start;
> long fbmem_len;
> void __iomem *fbmem;
> -#if defined(CONFIG_FB_VIA_CAMERA) || defined(CONFIG_FB_VIA_CAMERA_MODULE)
> +#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
> long camera_fbmem_offset;
> long camera_fbmem_size;
> #endif
> @@ -138,6 +138,7 @@ void viafb_irq_disable(u32 mask);
> #define VDE_I_LVDSSIEN 0x40000000 /* LVDS Sense enable */
> #define VDE_I_ENABLE 0x80000000 /* Global interrupt enable */
>
> +#if defined(CONFIG_VIDEO_VIA_CAMERA) || defined(CONFIG_VIDEO_VIA_CAMERA_MODULE)
> /*
> * DMA management.
> */
> @@ -172,6 +173,7 @@ int viafb_dma_copy_out_sg(unsigned int offset, struct scatterlist *sg, int nsg);
> */
> #define VGA_WIDTH 640
> #define VGA_HEIGHT 480
> +#endif /* CONFIG_VIDEO_VIA_CAMERA */
>
> /*
> * Indexed port operations. Note that these are all multi-op
> diff --git a/include/media/v4l2-chip-ident.h b/include/media/v4l2-chip-ident.h
> index 21b4428..6400c7c 100644
> --- a/include/media/v4l2-chip-ident.h
> +++ b/include/media/v4l2-chip-ident.h
> @@ -111,6 +111,10 @@ enum {
> V4L2_IDENT_VPX3216B = 3216,
> V4L2_IDENT_VPX3220A = 3220,
>
> + /* VX855 just ident 3409 */
> + /* Other via devs could use 3314, 3324, 3327, 3336, 3364, 3353 */
> + V4L2_IDENT_VIA_VX855 = 3409,
> +
> /* module tvp5150 */
> V4L2_IDENT_TVP5150 = 5150,
>
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-10 22:23 [PATCH] viafb camera controller driver Jonathan Corbet
2010-10-11 12:09 ` Mauro Carvalho Chehab
@ 2010-10-11 12:18 ` Laurent Pinchart
2010-10-11 15:30 ` Jonathan Corbet
2010-10-16 13:44 ` Mauro Carvalho Chehab
2 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2010-10-11 12:18 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Daniel Drake
Hi Jonathan,
On Monday 11 October 2010 00:23:13 Jonathan Corbet wrote:
> Howdy, all,
>
> Well, that took a whole lot longer than I had hoped...but, attached, is a
> new version of the viafb camera driver patch, done against 2.6.36-rc7.
> I've tried to address most of Laurent's comments from back in June; in
> particular, I have:
>
> - Gotten rid of the static device structure
Thanks. There's one more left, and it's easy to remove, see below.
> - Fixed some locking glitches
> - Fixed a bit of device initialization silliness.
>
> One thing I have *not* done is to push locking down into the ov7670
> driver. That would be a good thing to do at some point, but playing with
> that driver was beyond the scope of what I was trying to do here.
>
> This driver will still need some OLPC bits to work properly, but Daniel is
> working on that. This version of the driver does work on XO-1.5 systems,
> modulo some 2.6.36 API changes.
[snip]
> +/*
> + * Yes, this is a hack, but there's only going to be one of these
> + * on any system we know of.
> + */
> +static struct via_camera *via_cam_info;
That's what I'm talking about.
[snip]
> +static __devinit int viacam_probe(struct platform_device *pdev)
> +{
[snip]
> + /*
> + * Basic structure initialization.
> + */
> + cam = kzalloc (sizeof(struct via_camera), GFP_KERNEL);
> + if (cam == NULL)
> + return -ENOMEM;
> + via_cam_info = cam;
You set it here.
[snip]
> +static __devexit int viacam_remove(struct platform_device *pdev)
> +{
> + struct via_camera *cam = via_cam_info;
And use it here.
Just call platform_set_drvdata(pdev, cam) in viacam_probe to store the struct
via_camera pointer in the platform device, and platform_get_drvdata(pdev) here
to retrieve it.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-11 12:18 ` Laurent Pinchart
@ 2010-10-11 15:30 ` Jonathan Corbet
2010-10-11 23:20 ` Laurent Pinchart
0 siblings, 1 reply; 20+ messages in thread
From: Jonathan Corbet @ 2010-10-11 15:30 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Daniel Drake
On Mon, 11 Oct 2010 14:18:55 +0200
Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > +static __devexit int viacam_remove(struct platform_device *pdev)
> > +{
> > + struct via_camera *cam = via_cam_info;
>
> And use it here.
>
> Just call platform_set_drvdata(pdev, cam) in viacam_probe to store the struct
> via_camera pointer in the platform device, and platform_get_drvdata(pdev) here
> to retrieve it.
Yes, I know...but the fix isn't quite that simple because the platform
data is already used elsewhere. What's needed is some sort of "viafb
subdevice instance" structure which can keep all of the pointers
together. My plan is to do that, but it will require via-core changes
and I just don't have time for that right now.
Can I get away with this (it will cause no real-world trouble) with a
promise of a fix in the next month or two? I have some other via-core
stuff (suspend/resume in particular) that I need to do anyway.
Thanks,
jon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-11 15:30 ` Jonathan Corbet
@ 2010-10-11 23:20 ` Laurent Pinchart
0 siblings, 0 replies; 20+ messages in thread
From: Laurent Pinchart @ 2010-10-11 23:20 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Daniel Drake
Hi Jonathan,
On Monday 11 October 2010 17:30:48 Jonathan Corbet wrote:
> On Mon, 11 Oct 2010 14:18:55 +0200
>
> Laurent Pinchart <laurent.pinchart@ideasonboard.com> wrote:
> > > +static __devexit int viacam_remove(struct platform_device *pdev)
> > > +{
> > > + struct via_camera *cam = via_cam_info;
> >
> > And use it here.
> >
> > Just call platform_set_drvdata(pdev, cam) in viacam_probe to store the
> > struct via_camera pointer in the platform device, and
> > platform_get_drvdata(pdev) here to retrieve it.
>
> Yes, I know...but the fix isn't quite that simple because the platform
> data is already used elsewhere.
Aren't you mistaking platform_data and platform device drvdata ?
platform_{gs}et_drvdata() access the
platform_device::device::device_private::driver_data, which is a private
pointer reserved for driver-specific information. pdev->dev.platform_data is a
private pointer that stores platform-specific data (often configuration data)
in the platform device.
> What's needed is some sort of "viafb subdevice instance" structure which can
> keep all of the pointers together. My plan is to do that, but it will
> require via-core changes and I just don't have time for that right now.
>
> Can I get away with this (it will cause no real-world trouble) with a
> promise of a fix in the next month or two? I have some other via-core
> stuff (suspend/resume in particular) that I need to do anyway.
If the platform device driver data is really used by something else, OK. But
don't dare not keeping your promise ;-)
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-10 22:23 [PATCH] viafb camera controller driver Jonathan Corbet
2010-10-11 12:09 ` Mauro Carvalho Chehab
2010-10-11 12:18 ` Laurent Pinchart
@ 2010-10-16 13:44 ` Mauro Carvalho Chehab
2010-10-18 13:25 ` Jonathan Corbet
2010-10-19 3:20 ` Jonathan Corbet
2 siblings, 2 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-16 13:44 UTC (permalink / raw)
To: Jonathan Corbet, Hans Verkuil
Cc: linux-media, Florian Tobias Schandinat, Laurent Pinchart,
Daniel Drake
Em 10-10-2010 19:23, Jonathan Corbet escreveu:
> Howdy, all,
>
> Well, that took a whole lot longer than I had hoped...but, attached, is a
> new version of the viafb camera driver patch, done against 2.6.36-rc7.
> I've tried to address most of Laurent's comments from back in June; in
> particular, I have:
>
> - Gotten rid of the static device structure
> - Fixed some locking glitches
> - Fixed a bit of device initialization silliness.
>
> One thing I have *not* done is to push locking down into the ov7670
> driver. That would be a good thing to do at some point, but playing with
> that driver was beyond the scope of what I was trying to do here.
>
> This driver will still need some OLPC bits to work properly, but Daniel is
> working on that. This version of the driver does work on XO-1.5 systems,
> modulo some 2.6.36 API changes.
>
> Mauro, any chance of putting this in the queue for 2.6.37? Yes, I know
> it's really late, my apologies for that.
>
> Thanks,
>
> jon
> +static const struct v4l2_file_operations viacam_fops = {
> + .owner = THIS_MODULE,
> + .open = viacam_open,
> + .release = viacam_release,
> + .read = viacam_read,
> + .poll = viacam_poll,
> + .mmap = viacam_mmap,
> + .ioctl = video_ioctl2,
Hmm... do you need BKL? Otherwise, you should be using, instead, .unlocked_ioctl.
Btw, the driver build is broken:
drivers/media/video/via-camera.c: In function ‘viacam_configure_sensor’:
drivers/media/video/via-camera.c:228: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘s_fmt’
drivers/media/video/via-camera.c:228: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘s_fmt’
drivers/media/video/via-camera.c: In function ‘viacam_open’:
drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
drivers/media/video/via-camera.c: In function ‘viacam_enum_fmt_vid_cap’:
drivers/media/video/via-camera.c:878: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘enum_fmt’
drivers/media/video/via-camera.c:878: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘enum_fmt’
drivers/media/video/via-camera.c: In function ‘viacam_try_fmt_vid_cap’:
drivers/media/video/via-camera.c:921: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘try_fmt’
drivers/media/video/via-camera.c:921: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘try_fmt’
drivers/media/video/via-camera.c: In function ‘viacam_s_fmt_vid_cap’:
drivers/media/video/via-camera.c:959: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘try_fmt’
drivers/media/video/via-camera.c:959: error: ‘const struct v4l2_subdev_video_ops’ has no member named ‘try_fmt’
The main responsible for this breakage is this commit: 383e6ad73194814cfc980c302e0173fac11f0ee0
You may take a look on those commits also:
2e2ae382d956aa221c7b4fd897346c1b39f1699d
8c26cfe8d5f05ec9f945ea825c7eb28f46d04348
all commits are at staging/v2.6.37 branch on my tree:
http://git.linuxtv.org/media_tree.git
They removed s_fmt/enum_fmt/try_fmt in favor of a different approach, as sometimes, the format between the
sensor and the bridge is not exactly the same as seen on userspace.
The fix for this one is trivial:
drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
Just add an extra NULL parameter to the function.
-
Hans,
You're not playing nice with the others by changing their drivers without their ack, and without even
c/c them. I'm sure that, if Jon was aware on the changes you did on cafe-ccic and ov7670, he would be
submitting this driver with the build issues addressed.
So, please help him to address this issue.
Mauro.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-16 13:44 ` Mauro Carvalho Chehab
@ 2010-10-18 13:25 ` Jonathan Corbet
2010-10-19 3:20 ` Jonathan Corbet
1 sibling, 0 replies; 20+ messages in thread
From: Jonathan Corbet @ 2010-10-18 13:25 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
On Sat, 16 Oct 2010 10:44:56 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> Hmm... do you need BKL? Otherwise, you should be using, instead, .unlocked_ioctl.
No, that was just silly.
> Btw, the driver build is broken:
And that, it seems, indicates that I'm making my patch against the
wrong tree. I'll try to fix that up in the next day or so, sorry for
the trouble.
Thanks,
jon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-16 13:44 ` Mauro Carvalho Chehab
2010-10-18 13:25 ` Jonathan Corbet
@ 2010-10-19 3:20 ` Jonathan Corbet
2010-10-19 6:54 ` Hans Verkuil
1 sibling, 1 reply; 20+ messages in thread
From: Jonathan Corbet @ 2010-10-19 3:20 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
On Sat, 16 Oct 2010 10:44:56 -0300
Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
> drivers/media/video/via-camera.c: In function ‘viacam_open’:
> drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
> The fix for this one is trivial:
> drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
>
> Just add an extra NULL parameter to the function.
So I'm looking into this stuff. The extra NULL parameter is a struct
mutex, which seems to be used in one place in videobuf_waiton():
is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
/* Release vdev lock to prevent this wait from blocking outside access to
the device. */
if (is_ext_locked)
mutex_unlock(q->ext_lock);
I'd be most curious to know what the reasoning behind this code is; to my
uneducated eye, it looks like a real hack. How does this function know who
locked ext_lock? Can it really just unlock it safely? It seems to me that
this is a sign of locking issues which should really be dealt with
elsewhere, but, as I said, I'm uneducated, and the changelogs don't help me
much. Can somebody educate me?
Thanks,
jon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 3:20 ` Jonathan Corbet
@ 2010-10-19 6:54 ` Hans Verkuil
2010-10-19 7:52 ` Laurent Pinchart
2010-10-20 19:23 ` ext_lock (was viafb camera controller driver) Jonathan Corbet
0 siblings, 2 replies; 20+ messages in thread
From: Hans Verkuil @ 2010-10-19 6:54 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
> On Sat, 16 Oct 2010 10:44:56 -0300
> Mauro Carvalho Chehab <mchehab@infradead.org> wrote:
>
> > drivers/media/video/via-camera.c: In function ‘viacam_open’:
> > drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
>
> > The fix for this one is trivial:
> > drivers/media/video/via-camera.c:651: error: too few arguments to function ‘videobuf_queue_sg_init’
> >
> > Just add an extra NULL parameter to the function.
>
> So I'm looking into this stuff. The extra NULL parameter is a struct
> mutex, which seems to be used in one place in videobuf_waiton():
>
> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
>
> /* Release vdev lock to prevent this wait from blocking outside access to
> the device. */
> if (is_ext_locked)
> mutex_unlock(q->ext_lock);
>
> I'd be most curious to know what the reasoning behind this code is; to my
> uneducated eye, it looks like a real hack. How does this function know who
> locked ext_lock? Can it really just unlock it safely? It seems to me that
> this is a sign of locking issues which should really be dealt with
> elsewhere, but, as I said, I'm uneducated, and the changelogs don't help me
> much. Can somebody educate me?
We are working on removing the BKL. As part of that effort it is now possible
for drivers to pass a serialization mutex to the v4l core (a mutex pointer was
added to struct video_device). If the core sees that mutex then the core will
serialize all open/ioctl/read/write/etc. file ops. So all file ops will in that
case be called with that mutex held. Which is fine, but if the driver has to do
a blocking wait, then you need to unlock the mutex first and lock it again
afterwards. And since videobuf does a blocking wait it needs to know about that
mutex.
Right now we are in the middle of the transition from BKL to using core locks
(and some drivers will do their own locking completely). During this transition
period we have drivers that provide an external lock and drivers still relying
on the BKL in which case videobuf needs to handle its own locking. Hopefully
in 1-2 kernel cycles we will have abolished the BKL and we can remove the
videobuf internal lock and use the external mutex only.
So yes, it is a bit of a hack but there is actually a plan behind it :-)
It's a common theme inside the v4l subsystem, I'm afraid. We are still in the
process of creating a fully functional v4l core framework that all drivers
can use. But for every new piece of core functionality it means a transition
period where drivers are one by one converted to use it. That can take a very
long time. Add to that mix all the new functionality that is being added to
support embedded video hardware and it can get complex indeed.
I have a very clear goal in mind, though, and I (and many others!) are steadily
moving closer to that goal. Every kernel release has one or more essential
building blocks in place.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 6:54 ` Hans Verkuil
@ 2010-10-19 7:52 ` Laurent Pinchart
2010-10-19 10:46 ` Mauro Carvalho Chehab
2010-10-20 19:23 ` ext_lock (was viafb camera controller driver) Jonathan Corbet
1 sibling, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2010-10-19 7:52 UTC (permalink / raw)
To: Hans Verkuil
Cc: Jonathan Corbet, Mauro Carvalho Chehab, linux-media,
Florian Tobias Schandinat, Daniel Drake
Hi Hans,
On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote:
> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
> > On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote:
> > > drivers/media/video/via-camera.c: In function ‘viacam_open’:
> > > drivers/media/video/via-camera.c:651: error: too few arguments to
> > > function ‘videobuf_queue_sg_init’
> > >
> > > The fix for this one is trivial:
> > > drivers/media/video/via-camera.c:651: error: too few arguments to
> > > function ‘videobuf_queue_sg_init’
> > >
> > > Just add an extra NULL parameter to the function.
> >
> > So I'm looking into this stuff. The extra NULL parameter is a struct
> >
> > mutex, which seems to be used in one place in videobuf_waiton():
> > is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
> >
> > /* Release vdev lock to prevent this wait from blocking outside access
> > to
> >
> > the device. */
> >
> > if (is_ext_locked)
> >
> > mutex_unlock(q->ext_lock);
> >
> > I'd be most curious to know what the reasoning behind this code is; to my
> > uneducated eye, it looks like a real hack. How does this function know
> > who locked ext_lock? Can it really just unlock it safely? It seems to
> > me that this is a sign of locking issues which should really be dealt
> > with elsewhere, but, as I said, I'm uneducated, and the changelogs don't
> > help me much. Can somebody educate me?
>
> We are working on removing the BKL. As part of that effort it is now
> possible for drivers to pass a serialization mutex to the v4l core (a
> mutex pointer was added to struct video_device). If the core sees that
> mutex then the core will serialize all open/ioctl/read/write/etc. file
> ops. So all file ops will in that case be called with that mutex held.
> Which is fine, but if the driver has to do a blocking wait, then you need
> to unlock the mutex first and lock it again afterwards. And since videobuf
> does a blocking wait it needs to know about that mutex.
>
> Right now we are in the middle of the transition from BKL to using core
> locks (and some drivers will do their own locking completely). During this
> transition period we have drivers that provide an external lock and
> drivers still relying on the BKL in which case videobuf needs to handle
> its own locking. Hopefully in 1-2 kernel cycles we will have abolished the
> BKL and we can remove the videobuf internal lock and use the external
> mutex only.
>
> So yes, it is a bit of a hack but there is actually a plan behind it :-)
I still believe drivers should be encouraged to handle locking on their own.
These new "big v4l lock" (one per device) should be used only to remove the
BKL in existing drivers. It's a hack that we should work on getting rid of.
> It's a common theme inside the v4l subsystem, I'm afraid. We are still in
> the process of creating a fully functional v4l core framework that all
> drivers can use. But for every new piece of core functionality it means a
> transition period where drivers are one by one converted to use it. That
> can take a very long time. Add to that mix all the new functionality that
> is being added to support embedded video hardware and it can get complex
> indeed.
>
> I have a very clear goal in mind, though, and I (and many others!) are
> steadily moving closer to that goal. Every kernel release has one or more
> essential building blocks in place.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 7:52 ` Laurent Pinchart
@ 2010-10-19 10:46 ` Mauro Carvalho Chehab
2010-10-19 13:05 ` Laurent Pinchart
0 siblings, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-19 10:46 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Em 19-10-2010 05:52, Laurent Pinchart escreveu:
> Hi Hans,
>
> On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote:
>> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
>>> On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote:
>>>> drivers/media/video/via-camera.c: In function ‘viacam_open’:
>>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>>>> function ‘videobuf_queue_sg_init’
>>>>
>>>> The fix for this one is trivial:
>>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>>>> function ‘videobuf_queue_sg_init’
>>>>
>>>> Just add an extra NULL parameter to the function.
>>>
>>> So I'm looking into this stuff. The extra NULL parameter is a struct
>>>
>>> mutex, which seems to be used in one place in videobuf_waiton():
>>> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
>>>
>>> /* Release vdev lock to prevent this wait from blocking outside access
>>> to
>>>
>>> the device. */
>>>
>>> if (is_ext_locked)
>>>
>>> mutex_unlock(q->ext_lock);
>>>
>>> I'd be most curious to know what the reasoning behind this code is; to my
>>> uneducated eye, it looks like a real hack. How does this function know
>>> who locked ext_lock? Can it really just unlock it safely? It seems to
>>> me that this is a sign of locking issues which should really be dealt
>>> with elsewhere, but, as I said, I'm uneducated, and the changelogs don't
>>> help me much. Can somebody educate me?
>>
>> We are working on removing the BKL. As part of that effort it is now
>> possible for drivers to pass a serialization mutex to the v4l core (a
>> mutex pointer was added to struct video_device). If the core sees that
>> mutex then the core will serialize all open/ioctl/read/write/etc. file
>> ops. So all file ops will in that case be called with that mutex held.
>> Which is fine, but if the driver has to do a blocking wait, then you need
>> to unlock the mutex first and lock it again afterwards. And since videobuf
>> does a blocking wait it needs to know about that mutex.
>>
>> Right now we are in the middle of the transition from BKL to using core
>> locks (and some drivers will do their own locking completely). During this
>> transition period we have drivers that provide an external lock and
>> drivers still relying on the BKL in which case videobuf needs to handle
>> its own locking. Hopefully in 1-2 kernel cycles we will have abolished the
>> BKL and we can remove the videobuf internal lock and use the external
>> mutex only.
>>
>> So yes, it is a bit of a hack but there is actually a plan behind it :-)
>
> I still believe drivers should be encouraged to handle locking on their own.
> These new "big v4l lock" (one per device) should be used only to remove the
> BKL in existing drivers. It's a hack that we should work on getting rid of.
It is not a "big lock": it doesn't stop other CPU's, doesn't affect other hardware,
not even another V4L device. Basically, what this new lock does is to serialize access
to the hardware and to the hardware-mirrored data. On several cases, if you serialize
open, close, ioctl, read, write and mmap, the hardware will be serialized.
Of course, this doesn't cover 100% of the cases where a lock is needed. So, if the
driver have more fun things like kthreads, alsa, dvb, IR polling, etc, the driver will
need to lock on other places as well.
A typical V4L driver has lots of functions that need locking: open, close, read, write,
mmap and almost all ioctl (depending on the driver, just a very few set of enum ioctl's
could eventually not need an ioctl). What we found is that:
1) several developers didn't do the right thing since the beginning;
2) as time goes by, locks got bit roted.
3) some drivers were needing to touch on several locks (videobuf, their internal
priv locks, etc), sometimes generating cases where a dead lock would be possible.
On the tests we did so far, the v4l-core assisted lock helped to solve some locking issues
on the very few drivers that were ported. Also, it caused a regression on a driver where
the lock were working ;)
There are basically several opinions about this new schema: some that think that this is the
right thing to do, others think that think that this is the wrong thing or that this is
acceptable only as a transition for BKL-free drivers.
IMO, I think that both ways are acceptable: a core-assisted "hardware-access lock" helps to
avoid having lots of lock/unlock code at the driver, making drivers cleaner and easier to review,
and reducing the risk of lock degradation with time. On the other hand, some drivers may require
more complex locking schemas, like, for example, devices that support several simultaneous
independent video streams may have some common parts used by all streams that need to be serialized,
and other parts that can (and should) not be serialized. So, a core-assisted locking
for some cases may cause unneeded long waits.
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 10:46 ` Mauro Carvalho Chehab
@ 2010-10-19 13:05 ` Laurent Pinchart
2010-10-19 14:04 ` Hans Verkuil
2010-10-19 14:49 ` Mauro Carvalho Chehab
0 siblings, 2 replies; 20+ messages in thread
From: Laurent Pinchart @ 2010-10-19 13:05 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Hi Mauro,
On Tuesday 19 October 2010 12:46:11 Mauro Carvalho Chehab wrote:
> Em 19-10-2010 05:52, Laurent Pinchart escreveu:
> > On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote:
> >> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
> >>> On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote:
> >>>> drivers/media/video/via-camera.c: In function ‘viacam_open’:
> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
> >>>> function ‘videobuf_queue_sg_init’
> >>>>
> >>>> The fix for this one is trivial:
> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
> >>>> function ‘videobuf_queue_sg_init’
> >>>>
> >>>> Just add an extra NULL parameter to the function.
> >>>
> >>> So I'm looking into this stuff. The extra NULL parameter is a struct
> >>>
> >>> mutex, which seems to be used in one place in videobuf_waiton():
> >>> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
> >>>
> >>> /* Release vdev lock to prevent this wait from blocking outside access
> >>> to
> >>>
> >>> the device. */
> >>>
> >>> if (is_ext_locked)
> >>>
> >>> mutex_unlock(q->ext_lock);
> >>>
> >>> I'd be most curious to know what the reasoning behind this code is; to
> >>> my uneducated eye, it looks like a real hack. How does this function
> >>> know who locked ext_lock? Can it really just unlock it safely? It
> >>> seems to me that this is a sign of locking issues which should really
> >>> be dealt with elsewhere, but, as I said, I'm uneducated, and the
> >>> changelogs don't help me much. Can somebody educate me?
> >>
> >> We are working on removing the BKL. As part of that effort it is now
> >> possible for drivers to pass a serialization mutex to the v4l core (a
> >> mutex pointer was added to struct video_device). If the core sees that
> >> mutex then the core will serialize all open/ioctl/read/write/etc. file
> >> ops. So all file ops will in that case be called with that mutex held.
> >> Which is fine, but if the driver has to do a blocking wait, then you
> >> need to unlock the mutex first and lock it again afterwards. And since
> >> videobuf does a blocking wait it needs to know about that mutex.
> >>
> >> Right now we are in the middle of the transition from BKL to using core
> >> locks (and some drivers will do their own locking completely). During
> >> this transition period we have drivers that provide an external lock
> >> and drivers still relying on the BKL in which case videobuf needs to
> >> handle its own locking. Hopefully in 1-2 kernel cycles we will have
> >> abolished the BKL and we can remove the videobuf internal lock and use
> >> the external mutex only.
> >>
> >> So yes, it is a bit of a hack but there is actually a plan behind it :-)
> >
> > I still believe drivers should be encouraged to handle locking on their
> > own. These new "big v4l lock" (one per device) should be used only to
> > remove the BKL in existing drivers. It's a hack that we should work on
> > getting rid of.
>
> It is not a "big lock": it doesn't stop other CPU's, doesn't affect other
> hardware, not even another V4L device. Basically, what this new lock does
> is to serialize access to the hardware and to the hardware-mirrored data.
The lock serializes all ioctls. That's much more than protecting access to
data (both in system memory and in the hardware).
> On several cases, if you serialize open, close, ioctl, read, write and
> mmap, the hardware will be serialized.
>
> Of course, this doesn't cover 100% of the cases where a lock is needed. So,
> if the driver have more fun things like kthreads, alsa, dvb, IR polling,
> etc, the driver will need to lock on other places as well.
>
> A typical V4L driver has lots of functions that need locking: open, close,
> read, write, mmap and almost all ioctl (depending on the driver, just a
> very few set of enum ioctl's could eventually not need an ioctl). What we
> found is that:
>
> 1) several developers didn't do the right thing since the beginning;
That's not a valid reason to push new drivers for a very coarse grain locking
scheme. Developers must not get told to be stupid and don't care about locks
just because other developers got it wrong in the past. If people don't get
locking right we need to educate them, not encourage them to understand even
less of it.
> 2) as time goes by, locks got bit roted.
> 3) some drivers were needing to touch on several locks (videobuf, their
> internal priv locks, etc), sometimes generating cases where a dead lock
> would be possible.
>
> On the tests we did so far, the v4l-core assisted lock helped to solve some
> locking issues on the very few drivers that were ported. Also, it caused a
> regression on a driver where the lock were working ;)
>
> There are basically several opinions about this new schema: some that think
> that this is the right thing to do, others think that think that this is
> the wrong thing or that this is acceptable only as a transition for
> BKL-free drivers.
Indeed, and I belong to the second group.
> IMO, I think that both ways are acceptable: a core-assisted
> "hardware-access lock" helps to avoid having lots of lock/unlock code at
> the driver, making drivers cleaner and easier to review, and reducing the
> risk of lock degradation with time. On the other hand, some drivers may
> require more complex locking schemas, like, for example, devices that
> support several simultaneous independent video streams may have some
> common parts used by all streams that need to be serialized, and other
> parts that can (and should) not be serialized. So, a core-assisted locking
> for some cases may cause unneeded long waits.
A coarse core lock is acceptable in a transition phase to get rid of the BKL
because we don't have the necessary resources to do it right and right now.
Our goal should be to get rid of it in the long term as well (although we will
probably never complete this task, as not all drivers have a wide users and
developers base). New drivers must thus implement proper locking.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 13:05 ` Laurent Pinchart
@ 2010-10-19 14:04 ` Hans Verkuil
2010-10-19 14:21 ` Hans Verkuil
2010-10-19 14:49 ` Mauro Carvalho Chehab
1 sibling, 1 reply; 20+ messages in thread
From: Hans Verkuil @ 2010-10-19 14:04 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Mauro Carvalho Chehab, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Hi Laurent,
> Hi Mauro,
>
> On Tuesday 19 October 2010 12:46:11 Mauro Carvalho Chehab wrote:
>> Em 19-10-2010 05:52, Laurent Pinchart escreveu:
>> > On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote:
>> >> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
>> >>> On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote:
>> >>>> drivers/media/video/via-camera.c: In function ‘viacam_open’:
>> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>> >>>> function ‘videobuf_queue_sg_init’
>> >>>>
>> >>>> The fix for this one is trivial:
>> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>> >>>> function ‘videobuf_queue_sg_init’
>> >>>>
>> >>>> Just add an extra NULL parameter to the function.
>> >>>
>> >>> So I'm looking into this stuff. The extra NULL parameter is a
>> struct
>> >>>
>> >>> mutex, which seems to be used in one place in videobuf_waiton():
>> >>> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
>> >>>
>> >>> /* Release vdev lock to prevent this wait from blocking outside
>> access
>> >>> to
>> >>>
>> >>> the device. */
>> >>>
>> >>> if (is_ext_locked)
>> >>>
>> >>> mutex_unlock(q->ext_lock);
>> >>>
>> >>> I'd be most curious to know what the reasoning behind this code is;
>> to
>> >>> my uneducated eye, it looks like a real hack. How does this
>> function
>> >>> know who locked ext_lock? Can it really just unlock it safely? It
>> >>> seems to me that this is a sign of locking issues which should
>> really
>> >>> be dealt with elsewhere, but, as I said, I'm uneducated, and the
>> >>> changelogs don't help me much. Can somebody educate me?
>> >>
>> >> We are working on removing the BKL. As part of that effort it is now
>> >> possible for drivers to pass a serialization mutex to the v4l core (a
>> >> mutex pointer was added to struct video_device). If the core sees
>> that
>> >> mutex then the core will serialize all open/ioctl/read/write/etc.
>> file
>> >> ops. So all file ops will in that case be called with that mutex
>> held.
>> >> Which is fine, but if the driver has to do a blocking wait, then you
>> >> need to unlock the mutex first and lock it again afterwards. And
>> since
>> >> videobuf does a blocking wait it needs to know about that mutex.
>> >>
>> >> Right now we are in the middle of the transition from BKL to using
>> core
>> >> locks (and some drivers will do their own locking completely). During
>> >> this transition period we have drivers that provide an external lock
>> >> and drivers still relying on the BKL in which case videobuf needs to
>> >> handle its own locking. Hopefully in 1-2 kernel cycles we will have
>> >> abolished the BKL and we can remove the videobuf internal lock and
>> use
>> >> the external mutex only.
>> >>
>> >> So yes, it is a bit of a hack but there is actually a plan behind it
>> :-)
>> >
>> > I still believe drivers should be encouraged to handle locking on
>> their
>> > own. These new "big v4l lock" (one per device) should be used only to
>> > remove the BKL in existing drivers. It's a hack that we should work on
>> > getting rid of.
>>
>> It is not a "big lock": it doesn't stop other CPU's, doesn't affect
>> other
>> hardware, not even another V4L device. Basically, what this new lock
>> does
>> is to serialize access to the hardware and to the hardware-mirrored
>> data.
>
> The lock serializes all ioctls. That's much more than protecting access to
> data (both in system memory and in the hardware).
Absolutely correct. That's exactly what this lock does. Serializing all
this makes it much easier to prove that a driver is correctly locking
everything.
For 90% of all our drivers this is all that is needed. Yes, it is at a
coarser level than the theoretical optimum, but basically, who cares? For
that 90% it is simply sufficient and without any performance penalties.
>> On several cases, if you serialize open, close, ioctl, read, write and
>> mmap, the hardware will be serialized.
>>
>> Of course, this doesn't cover 100% of the cases where a lock is needed.
>> So,
>> if the driver have more fun things like kthreads, alsa, dvb, IR polling,
>> etc, the driver will need to lock on other places as well.
>>
>> A typical V4L driver has lots of functions that need locking: open,
>> close,
>> read, write, mmap and almost all ioctl (depending on the driver, just a
>> very few set of enum ioctl's could eventually not need an ioctl). What
>> we
>> found is that:
>>
>> 1) several developers didn't do the right thing since the beginning;
>
> That's not a valid reason to push new drivers for a very coarse grain
> locking
> scheme. Developers must not get told to be stupid and don't care about
> locks
> just because other developers got it wrong in the past. If people don't
> get
> locking right we need to educate them, not encourage them to understand
> even
> less of it.
Locking is hard in complex drivers. Period.
>
>> 2) as time goes by, locks got bit roted.
>> 3) some drivers were needing to touch on several locks (videobuf, their
>> internal priv locks, etc), sometimes generating cases where a dead lock
>> would be possible.
>>
>> On the tests we did so far, the v4l-core assisted lock helped to solve
>> some
>> locking issues on the very few drivers that were ported. Also, it caused
>> a
>> regression on a driver where the lock were working ;)
>>
>> There are basically several opinions about this new schema: some that
>> think
>> that this is the right thing to do, others think that think that this is
>> the wrong thing or that this is acceptable only as a transition for
>> BKL-free drivers.
>
> Indeed, and I belong to the second group.
>
>> IMO, I think that both ways are acceptable: a core-assisted
>> "hardware-access lock" helps to avoid having lots of lock/unlock code at
>> the driver, making drivers cleaner and easier to review, and reducing
>> the
>> risk of lock degradation with time. On the other hand, some drivers may
>> require more complex locking schemas, like, for example, devices that
>> support several simultaneous independent video streams may have some
>> common parts used by all streams that need to be serialized, and other
>> parts that can (and should) not be serialized. So, a core-assisted
>> locking
>> for some cases may cause unneeded long waits.
>
> A coarse core lock is acceptable in a transition phase to get rid of the
> BKL
> because we don't have the necessary resources to do it right and right
> now.
> Our goal should be to get rid of it in the long term as well (although we
> will
> probably never complete this task, as not all drivers have a wide users
> and
> developers base). New drivers must thus implement proper locking.
>
> --
> Regards,
>
> Laurent Pinchart
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
>
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 14:04 ` Hans Verkuil
@ 2010-10-19 14:21 ` Hans Verkuil
0 siblings, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2010-10-19 14:21 UTC (permalink / raw)
To: Hans Verkuil
Cc: Laurent Pinchart, Mauro Carvalho Chehab, Jonathan Corbet,
linux-media, Florian Tobias Schandinat, Daniel Drake
> Hi Laurent,
>
>> Hi Mauro,
>>
>> On Tuesday 19 October 2010 12:46:11 Mauro Carvalho Chehab wrote:
>>> Em 19-10-2010 05:52, Laurent Pinchart escreveu:
>>> > On Tuesday 19 October 2010 08:54:40 Hans Verkuil wrote:
>>> >> On Tuesday, October 19, 2010 05:20:17 Jonathan Corbet wrote:
>>> >>> On Sat, 16 Oct 2010 10:44:56 -0300 Mauro Carvalho Chehab wrote:
>>> >>>> drivers/media/video/via-camera.c: In function ‘viacam_open’:
>>> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>>> >>>> function ‘videobuf_queue_sg_init’
>>> >>>>
>>> >>>> The fix for this one is trivial:
>>> >>>> drivers/media/video/via-camera.c:651: error: too few arguments to
>>> >>>> function ‘videobuf_queue_sg_init’
>>> >>>>
>>> >>>> Just add an extra NULL parameter to the function.
>>> >>>
>>> >>> So I'm looking into this stuff. The extra NULL parameter is a
>>> struct
>>> >>>
>>> >>> mutex, which seems to be used in one place in videobuf_waiton():
>>> >>> is_ext_locked = q->ext_lock && mutex_is_locked(q->ext_lock);
>>> >>>
>>> >>> /* Release vdev lock to prevent this wait from blocking outside
>>> access
>>> >>> to
>>> >>>
>>> >>> the device. */
>>> >>>
>>> >>> if (is_ext_locked)
>>> >>>
>>> >>> mutex_unlock(q->ext_lock);
>>> >>>
>>> >>> I'd be most curious to know what the reasoning behind this code is;
>>> to
>>> >>> my uneducated eye, it looks like a real hack. How does this
>>> function
>>> >>> know who locked ext_lock? Can it really just unlock it safely? It
>>> >>> seems to me that this is a sign of locking issues which should
>>> really
>>> >>> be dealt with elsewhere, but, as I said, I'm uneducated, and the
>>> >>> changelogs don't help me much. Can somebody educate me?
>>> >>
>>> >> We are working on removing the BKL. As part of that effort it is now
>>> >> possible for drivers to pass a serialization mutex to the v4l core
>>> (a
>>> >> mutex pointer was added to struct video_device). If the core sees
>>> that
>>> >> mutex then the core will serialize all open/ioctl/read/write/etc.
>>> file
>>> >> ops. So all file ops will in that case be called with that mutex
>>> held.
>>> >> Which is fine, but if the driver has to do a blocking wait, then you
>>> >> need to unlock the mutex first and lock it again afterwards. And
>>> since
>>> >> videobuf does a blocking wait it needs to know about that mutex.
>>> >>
>>> >> Right now we are in the middle of the transition from BKL to using
>>> core
>>> >> locks (and some drivers will do their own locking completely).
>>> During
>>> >> this transition period we have drivers that provide an external lock
>>> >> and drivers still relying on the BKL in which case videobuf needs to
>>> >> handle its own locking. Hopefully in 1-2 kernel cycles we will have
>>> >> abolished the BKL and we can remove the videobuf internal lock and
>>> use
>>> >> the external mutex only.
>>> >>
>>> >> So yes, it is a bit of a hack but there is actually a plan behind it
>>> :-)
>>> >
>>> > I still believe drivers should be encouraged to handle locking on
>>> their
>>> > own. These new "big v4l lock" (one per device) should be used only to
>>> > remove the BKL in existing drivers. It's a hack that we should work
>>> on
>>> > getting rid of.
>>>
>>> It is not a "big lock": it doesn't stop other CPU's, doesn't affect
>>> other
>>> hardware, not even another V4L device. Basically, what this new lock
>>> does
>>> is to serialize access to the hardware and to the hardware-mirrored
>>> data.
>>
>> The lock serializes all ioctls. That's much more than protecting access
>> to
>> data (both in system memory and in the hardware).
>
> Absolutely correct. That's exactly what this lock does. Serializing all
> this makes it much easier to prove that a driver is correctly locking
> everything.
>
> For 90% of all our drivers this is all that is needed. Yes, it is at a
> coarser level than the theoretical optimum, but basically, who cares? For
> that 90% it is simply sufficient and without any performance penalties.
>
>>> On several cases, if you serialize open, close, ioctl, read, write and
>>> mmap, the hardware will be serialized.
>>>
>>> Of course, this doesn't cover 100% of the cases where a lock is needed.
>>> So,
>>> if the driver have more fun things like kthreads, alsa, dvb, IR
>>> polling,
>>> etc, the driver will need to lock on other places as well.
>>>
>>> A typical V4L driver has lots of functions that need locking: open,
>>> close,
>>> read, write, mmap and almost all ioctl (depending on the driver, just a
>>> very few set of enum ioctl's could eventually not need an ioctl). What
>>> we
>>> found is that:
>>>
>>> 1) several developers didn't do the right thing since the beginning;
>>
>> That's not a valid reason to push new drivers for a very coarse grain
>> locking
>> scheme. Developers must not get told to be stupid and don't care about
>> locks
>> just because other developers got it wrong in the past. If people don't
>> get
>> locking right we need to educate them, not encourage them to understand
>> even
>> less of it.
>
> Locking is hard in complex drivers. Period.
Sorry, I hadn't finished this email. And of course I lost the rest of what
I wrote :-(
But basically I want to leave the choice of what to use to the driver
developer. If he wants to punish himself and do all the locking manually
(and prove that it is correct), then by all means, do so. If you want to
use the core locking support and so simplify your driver and allow your
brain to concentrate on getting the hardware to work, rather than trying
to get the locking right, then that's fine as well. As a code reviewer I'd
definitely prefer the latter approach as it makes my life much easier.
And I am not convinced that the argument that fine-grain locking improves
performance is valid. Definitely not for the 'normal' type of drivers.
Perhaps for complex SoCs it might be true, though. Although I'd love to
see some numbers first.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 13:05 ` Laurent Pinchart
2010-10-19 14:04 ` Hans Verkuil
@ 2010-10-19 14:49 ` Mauro Carvalho Chehab
2010-10-19 14:52 ` Laurent Pinchart
1 sibling, 1 reply; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-19 14:49 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Em 19-10-2010 11:05, Laurent Pinchart escreveu:
> Hi Mauro,
>
>> It is not a "big lock": it doesn't stop other CPU's, doesn't affect other
>> hardware, not even another V4L device. Basically, what this new lock does
>> is to serialize access to the hardware and to the hardware-mirrored data.
>
> The lock serializes all ioctls. That's much more than protecting access to
> data (both in system memory and in the hardware).
It is not much more. What ioctl's doesn't access the hardware directly nor access
some struct that caches the hardware data? None, at the most complex devices.
For simpler devices, there are very few VIDIOC*ENUM stuff that may just return
a fixed set of values, but the userspace applications that call them serializes
the access anyway (as they are the enum ioctls, used during the hardware detection
phase of the userspace software).
So, in practice, it makes no difference to serialize everything or to remove
the lock for the very few ioctls that just return a fixed set of info.
>> There are basically several opinions about this new schema: some that think
>> that this is the right thing to do, others think that think that this is
>> the wrong thing or that this is acceptable only as a transition for
>> BKL-free drivers.
>
> Indeed, and I belong to the second group.
And Hans belong to the first one.
>> IMO, I think that both ways are acceptable: a core-assisted
>> "hardware-access lock" helps to avoid having lots of lock/unlock code at
>> the driver, making drivers cleaner and easier to review, and reducing the
>> risk of lock degradation with time. On the other hand, some drivers may
>> require more complex locking schemas, like, for example, devices that
>> support several simultaneous independent video streams may have some
>> common parts used by all streams that need to be serialized, and other
>> parts that can (and should) not be serialized. So, a core-assisted locking
>> for some cases may cause unneeded long waits.
I am in a position between the first and the second group.
Reviewing locks is simpler with the new schema, and, if well implemented, it
will help to solve a big problem, but I don't believe that this schema is enough
to solve all cases, nor that drivers with lots of independent streams should
use it.
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 14:49 ` Mauro Carvalho Chehab
@ 2010-10-19 14:52 ` Laurent Pinchart
2010-10-19 15:42 ` Mauro Carvalho Chehab
0 siblings, 1 reply; 20+ messages in thread
From: Laurent Pinchart @ 2010-10-19 14:52 UTC (permalink / raw)
To: Mauro Carvalho Chehab
Cc: Hans Verkuil, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Hi Mauro,
On Tuesday 19 October 2010 16:49:25 Mauro Carvalho Chehab wrote:
> Em 19-10-2010 11:05, Laurent Pinchart escreveu:
> >> It is not a "big lock": it doesn't stop other CPU's, doesn't affect
> >> other hardware, not even another V4L device. Basically, what this new
> >> lock does is to serialize access to the hardware and to the
> >> hardware-mirrored data.
> >
> > The lock serializes all ioctls. That's much more than protecting access
> > to data (both in system memory and in the hardware).
>
> It is not much more. What ioctl's doesn't access the hardware directly nor
> access some struct that caches the hardware data? None, at the most
> complex devices.
>
> For simpler devices, there are very few VIDIOC*ENUM stuff that may just
> return a fixed set of values, but the userspace applications that call
> them serializes the access anyway (as they are the enum ioctls, used
> during the hardware detection phase of the userspace software).
>
> So, in practice, it makes no difference to serialize everything or to
> remove the lock for the very few ioctls that just return a fixed set of
> info.
That's not correct. There's a difference between taking a lock around
read/write operations and around the whole ioctl call stack.
> >> There are basically several opinions about this new schema: some that
> >> think that this is the right thing to do, others think that think that
> >> this is the wrong thing or that this is acceptable only as a transition
> >> for BKL-free drivers.
> >
> > Indeed, and I belong to the second group.
>
> And Hans belong to the first one.
So you're right, there are two groups :-)
> >> IMO, I think that both ways are acceptable: a core-assisted
> >> "hardware-access lock" helps to avoid having lots of lock/unlock code at
> >> the driver, making drivers cleaner and easier to review, and reducing
> >> the risk of lock degradation with time. On the other hand, some drivers
> >> may require more complex locking schemas, like, for example, devices
> >> that support several simultaneous independent video streams may have
> >> some common parts used by all streams that need to be serialized, and
> >> other parts that can (and should) not be serialized. So, a
> >> core-assisted locking for some cases may cause unneeded long waits.
>
> I am in a position between the first and the second group.
>
> Reviewing locks is simpler with the new schema, and, if well implemented,
> it will help to solve a big problem, but I don't believe that this schema
> is enough to solve all cases, nor that drivers with lots of independent
> streams should use it.
--
Regards,
Laurent Pinchart
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH] viafb camera controller driver
2010-10-19 14:52 ` Laurent Pinchart
@ 2010-10-19 15:42 ` Mauro Carvalho Chehab
0 siblings, 0 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-19 15:42 UTC (permalink / raw)
To: Laurent Pinchart
Cc: Hans Verkuil, Jonathan Corbet, linux-media,
Florian Tobias Schandinat, Daniel Drake
Em 19-10-2010 12:52, Laurent Pinchart escreveu:
> Hi Mauro,
>
> On Tuesday 19 October 2010 16:49:25 Mauro Carvalho Chehab wrote:
>> Em 19-10-2010 11:05, Laurent Pinchart escreveu:
>>>> It is not a "big lock": it doesn't stop other CPU's, doesn't affect
>>>> other hardware, not even another V4L device. Basically, what this new
>>>> lock does is to serialize access to the hardware and to the
>>>> hardware-mirrored data.
>>>
>>> The lock serializes all ioctls. That's much more than protecting access
>>> to data (both in system memory and in the hardware).
>>
>> It is not much more. What ioctl's doesn't access the hardware directly nor
>> access some struct that caches the hardware data? None, at the most
>> complex devices.
>>
>> For simpler devices, there are very few VIDIOC*ENUM stuff that may just
>> return a fixed set of values, but the userspace applications that call
>> them serializes the access anyway (as they are the enum ioctls, used
>> during the hardware detection phase of the userspace software).
>>
>> So, in practice, it makes no difference to serialize everything or to
>> remove the lock for the very few ioctls that just return a fixed set of
>> info.
>
> That's not correct. There's a difference between taking a lock around
> read/write operations and around the whole ioctl call stack.
Huh?
Even on simpler hardware, there are very few ioctls that don't access hardware.
Hardly, this would cause any performance impact.
What I said is that, if the userspace does:
open()
/* Serialized ioctls */
ioctl (...)
ioctl (...)
ioctl (...)
ioctl (...)
ioctl (...)
ioctl (...)
ioctl (...)
...
It doesn't matter if kernel is forcing serialization or not.
The difference only happens if userspace does things like:
fork() /* Or some thread creation function */
...
/* proccess/thread 1 */
ioctl()
...
/* proccess/thread 2 */
ioctl()
...
/* proccess/thread 3 */
ioctl()
...
However, it doesn't make sense to do parallel access to *ENUM ioctls, so I don't know
any application doing that during the "hardware discover phase", whe.
The usage of different process/threads may make sense, from userspace POV, while streaming.
There are a few reasons for that, like:
- users may want to adjust the bright/contrast (or other video/audio control)
while streaming;
- if the dqbuf logic is complex (for example, it may have some software processing
logic - like de interlacing);
- a separate process may be recording the video.
On all the above valid use-cases, the ioctl is really accessing the hardware and/or some
hardware-cached data. So, a lock is needed anyway.
Cheers,
Mauro
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ext_lock (was viafb camera controller driver)
2010-10-19 6:54 ` Hans Verkuil
2010-10-19 7:52 ` Laurent Pinchart
@ 2010-10-20 19:23 ` Jonathan Corbet
2010-10-21 2:57 ` Mauro Carvalho Chehab
2010-10-21 6:51 ` Hans Verkuil
1 sibling, 2 replies; 20+ messages in thread
From: Jonathan Corbet @ 2010-10-20 19:23 UTC (permalink / raw)
To: Hans Verkuil
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
On Tue, 19 Oct 2010 08:54:40 +0200
Hans Verkuil <hverkuil@xs4all.nl> wrote:
> We are working on removing the BKL. As part of that effort it is now possible
> for drivers to pass a serialization mutex to the v4l core (a mutex pointer was
> added to struct video_device). If the core sees that mutex then the core will
> serialize all open/ioctl/read/write/etc. file ops. So all file ops will in that
> case be called with that mutex held. Which is fine, but if the driver has to do
> a blocking wait, then you need to unlock the mutex first and lock it again
> afterwards. And since videobuf does a blocking wait it needs to know about that
> mutex.
So videobuf is expecting that you're passing this special device mutex
in particular? In that case, why not just use it directly? Having a
separate pointer to (what looks like) a distinct lock seems like a way
to cause fatal confusion. Given the tightness of the rules here (you
*must* know that this "ext_lock" has been grabbed by the v4l core in the
current call chain or you cannot possibly unlock it safely), I don't
get why you wouldn't use the lock directly.
I would be inclined to go even further and emit a warning if the mutex
is *not* locked. It seems that the rules would require it, no? If
mutex debugging is turned on, you could even check if the current task
is the locker, would would be even better.
In general, put me in the "leery of central locking" camp. If you
don't understand locking, you're going to mess things up somewhere
along the way. And, as soon as you get into hardware with interrupts,
it seems like you have to deal with your own locking for access to the
hardware regardless...
Thanks,
jon
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ext_lock (was viafb camera controller driver)
2010-10-20 19:23 ` ext_lock (was viafb camera controller driver) Jonathan Corbet
@ 2010-10-21 2:57 ` Mauro Carvalho Chehab
2010-10-21 6:51 ` Hans Verkuil
1 sibling, 0 replies; 20+ messages in thread
From: Mauro Carvalho Chehab @ 2010-10-21 2:57 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Hans Verkuil, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
Em 20-10-2010 17:23, Jonathan Corbet escreveu:
> On Tue, 19 Oct 2010 08:54:40 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
>> We are working on removing the BKL. As part of that effort it is now possible
>> for drivers to pass a serialization mutex to the v4l core (a mutex pointer was
>> added to struct video_device). If the core sees that mutex then the core will
>> serialize all open/ioctl/read/write/etc. file ops. So all file ops will in that
>> case be called with that mutex held. Which is fine, but if the driver has to do
>> a blocking wait, then you need to unlock the mutex first and lock it again
>> afterwards. And since videobuf does a blocking wait it needs to know about that
>> mutex.
>
> So videobuf is expecting that you're passing this special device mutex
> in particular? In that case, why not just use it directly? Having a
> separate pointer to (what looks like) a distinct lock seems like a way
> to cause fatal confusion. Given the tightness of the rules here (you
> *must* know that this "ext_lock" has been grabbed by the v4l core in the
> current call chain or you cannot possibly unlock it safely), I don't
> get why you wouldn't use the lock directly.
>
> I would be inclined to go even further and emit a warning if the mutex
> is *not* locked. It seems that the rules would require it, no? If
> mutex debugging is turned on, you could even check if the current task
> is the locker, would would be even better.
Yeah, a definitive solution would be to directly use the lock at videobuf. However,
people are re-writing videobuf (the new version is called videobuf2), in order
to fix some know issues on it.
Probably, it is better to wait for the new vb2, test it, and better integrate
its locking schema.
> In general, put me in the "leery of central locking" camp. If you
> don't understand locking, you're going to mess things up somewhere
> along the way. And, as soon as you get into hardware with interrupts,
> it seems like you have to deal with your own locking for access to the
> hardware regardless...
Yeah, if developers don't understand lock, a mess will happen, but this
doesn't matter if the developer is using a central or a driver-priv lock.
With a core-assisted lock, driver code is cleaner, and, hopefully, will be
easier to analyze the lock "exceptions", as the common case (hardware access
via file operations) will already be covered.
Cheers,
Mauro.
^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: ext_lock (was viafb camera controller driver)
2010-10-20 19:23 ` ext_lock (was viafb camera controller driver) Jonathan Corbet
2010-10-21 2:57 ` Mauro Carvalho Chehab
@ 2010-10-21 6:51 ` Hans Verkuil
1 sibling, 0 replies; 20+ messages in thread
From: Hans Verkuil @ 2010-10-21 6:51 UTC (permalink / raw)
To: Jonathan Corbet
Cc: Mauro Carvalho Chehab, linux-media, Florian Tobias Schandinat,
Laurent Pinchart, Daniel Drake
On Wednesday, October 20, 2010 21:23:42 Jonathan Corbet wrote:
> On Tue, 19 Oct 2010 08:54:40 +0200
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>
> > We are working on removing the BKL. As part of that effort it is now possible
> > for drivers to pass a serialization mutex to the v4l core (a mutex pointer was
> > added to struct video_device). If the core sees that mutex then the core will
> > serialize all open/ioctl/read/write/etc. file ops. So all file ops will in that
> > case be called with that mutex held. Which is fine, but if the driver has to do
> > a blocking wait, then you need to unlock the mutex first and lock it again
> > afterwards. And since videobuf does a blocking wait it needs to know about that
> > mutex.
>
> So videobuf is expecting that you're passing this special device mutex
> in particular? In that case, why not just use it directly?
The problem is that videobuf has to cater for two possible scenarios regarding
external locks: either it is the core lock which can be obtained through struct
video_device, or it is a driver-owned lock. In both cases videobuf has to unlock
before the wait.
So I can't just give it a pointer to video_device.
For the next kernel cycle we should start the work to convert drivers to
unlocked_ioctl and the core lock or a private lock. Hopefully that will give us
a better idea of how this can be improved.
> Having a
> separate pointer to (what looks like) a distinct lock seems like a way
> to cause fatal confusion. Given the tightness of the rules here (you
> *must* know that this "ext_lock" has been grabbed by the v4l core in the
> current call chain or you cannot possibly unlock it safely), I don't
> get why you wouldn't use the lock directly.
>
> I would be inclined to go even further and emit a warning if the mutex
> is *not* locked. It seems that the rules would require it, no? If
> mutex debugging is turned on, you could even check if the current task
> is the locker, would would be even better.
I agree with this. Mauro, I propose this change:
BUG_ON(q->ext_lock && !mutex_is_locked(q->ext_lock));
if (q->ext_lock)
mutex_unlock(q->ext_lock);
...
if (q->ext_lock)
mutex_lock(q->ext_lock);
It makes no sense to provide an external lock and not having it locked here.
> In general, put me in the "leery of central locking" camp. If you
> don't understand locking, you're going to mess things up somewhere
> along the way. And, as soon as you get into hardware with interrupts,
> it seems like you have to deal with your own locking for access to the
> hardware regardless...
Do you want to tackle the task of fixing all the old BKL drivers? That is
still the primary motivation for doing this, you know. I really don't
fancy putting manual locking in those drivers and auditing them all.
BTW, I want to add another reason why core-assisted locking is a good idea
for many drivers: the chances of hitting a race condition if the locking
isn't quite right are exceedingly low for v4l drivers. In general there is
just a single application that opens the device, streams and closes it again.
It is very rare for, say two apps to open the device at almost the same time,
or one closing it and another opening it. Or for two apps calling an ioctl at
the same time.
So things may seem to work just dandy, even though a driver contains more
races than you can shake a stick at.
Given the fact that a considerable amount of work went into verifying that the
v4l2 core does locking right (and I'm talking not just about the new core locking
feature, but also about the locks protecting internal core data structures), and
that it usually took several tries to get it right, I am not at all confident
that the same wouldn't happen on a much larger scale in all the drivers. Particularly
if it is a hot-pluggable driver.
Anyway, driver developers are completely free to choose either core-assisted
locking or manual locking. But for the BKL conversion core-assisted locking is
the only way if we want to have any hope of finishing that conversion in a
reasonable amount of time and with a reasonable degree of confidence in the
correctness of the locking.
Regards,
Hans
--
Hans Verkuil - video4linux developer - sponsored by TANDBERG, part of Cisco
^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2010-10-21 6:51 UTC | newest]
Thread overview: 20+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2010-10-10 22:23 [PATCH] viafb camera controller driver Jonathan Corbet
2010-10-11 12:09 ` Mauro Carvalho Chehab
2010-10-11 12:18 ` Laurent Pinchart
2010-10-11 15:30 ` Jonathan Corbet
2010-10-11 23:20 ` Laurent Pinchart
2010-10-16 13:44 ` Mauro Carvalho Chehab
2010-10-18 13:25 ` Jonathan Corbet
2010-10-19 3:20 ` Jonathan Corbet
2010-10-19 6:54 ` Hans Verkuil
2010-10-19 7:52 ` Laurent Pinchart
2010-10-19 10:46 ` Mauro Carvalho Chehab
2010-10-19 13:05 ` Laurent Pinchart
2010-10-19 14:04 ` Hans Verkuil
2010-10-19 14:21 ` Hans Verkuil
2010-10-19 14:49 ` Mauro Carvalho Chehab
2010-10-19 14:52 ` Laurent Pinchart
2010-10-19 15:42 ` Mauro Carvalho Chehab
2010-10-20 19:23 ` ext_lock (was viafb camera controller driver) Jonathan Corbet
2010-10-21 2:57 ` Mauro Carvalho Chehab
2010-10-21 6:51 ` Hans Verkuil
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox