Re: [PATCH] Add the viafb video capture driver
Hi Jonathan,
On Tuesday 08 June 2010 04:31:58 Jonathan Corbet wrote:
> On Tue, 8 Jun 2010 03:03:14 +0200 Laurent Pinchart wrote:
[snip]
> > Don't define device structures as static object. You must kmalloc the
> > via_camera structure in probe and set the pointer as driver private data
> > to access it later in V4L2 operations and device core callbacks.
> > Otherwise Bad Things (TM) will happen if the device is removed while the
> > video device node is opened.
>
> I understand the comment...but this device is blasted onto the system's
> base silicon. It's not going to be removed in a way which leaves a
> functioning computer. Still, dynamic allocation is easy enough to do.
I think it would still be better. The hardware device can't be removed (unless
you seriously damage the system, but you'll have other problems then :-)) but
the platform device could easily be unregistered. It's a good practice not to
store instance-specific data in static variables anyway.
[snip]
> > > + /*
> > > + * 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);
> >
> > Ouch that's going to hurt performances !
> >
> > What are the hardware restrictions regarding the memory it can capture
> > images to ? Does it just have to be physically contiguous, or does the
> > memory need to come from a specific memory area ? In the first case you
> > could use videobuf_dma_contig and avoid the memcpy completely. In the
> > second case you should still mmap the memory to userspace when using
> > kernel-allocated buffers instead of memcpying the data. If you really
> > need a memcpy, you should then probably use videobuf_vmalloc instead of
> > videobuf_dma_sg.
>
> It's a DMA copy, so performance is actually not a problem.
>
> The video capture engine grabs frames into a three-buffer ring stored in
> viafb framebuffer memory.
OK.
> I *could* let user space map that memory directly, but it would be an
> eternal race with the engine and would not end well. We really do have to
> do the copy. In a sense, the framebuffer memory is just part of the capture
> device;
Just to make sure I understand things correctly, the hardware captures to a
dedicated ring buffer continuously, and generates IRQs when the next frame is
available. No software action is needed to feed the engine with new buffers.
Is that correct ?
> the DMA operation is how we make data available to the rest of the system.
Is that why you're using a threaded IRQ handler, to wait for the DMA
completion ? Isn't there a risk of racing with the engine, where the same
buffer could be overwritten with a new image while DMA is ongoing ? The
threaded IRQ handler would have to have been delayed quite a lot for that to
happen though, and I suppose there's not much you can do about it anyway.
> [Incidentally, the biggest cost here, I think, is setting up 150 DMA
> descriptors for each transfer. That's an artifact of the page-at-a-time
> memory allocation used by videobuf_dma_sg. I have a branch with an SG
> variant which tries to allocate the largest contiguous buffers possible
> without going over; it reduces the number of descriptors to about five. It
> didn't change my life a whole lot, so I back-burnered it, but I might
> send that patch out one of these days.]
[snip]
> > > + 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);
> >
> > Why don't you initialize the queue on probe ?
>
> ...because every example I saw does it at open time? Looking at the code
> now, it doesn't look like it needs to be done at open time.
That's one of the problems with videobuf. Drivers often use it in sub-optimal
ways, or even completely abuse the API, giving a hard time to developers when
they're looking for examples.
[snip]
> > > +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;
> >
> > According to the V4L2 spec the version number should be formatted using
> > KERNEL_VERSION().
>
> Interesting, I'd missed that. I've just optimized a call to
> KERNEL_VERSION(0, 0, 1) :)
You've saved the compiler a few cycles. I wonder how many times people will
need to compile the driver to notice a different in energy consumption and
green house effect ;-)
[snip]
> > > +static struct video_device viacam_v4l_template = {
> > > + .name = "via-camera",
> > > + .minor = -1,
> > > + .tvnorms= V4L2_STD_NTSC_M,
> > > + .current_norm = V4L2_STD_NTSC_M,
> >
> > It's a webcam, norms don't make sense.
>
> I agree they don't make se
Re: [PATCH] Add the viafb video capture driver
Em 07-06-2010 23:31, Jonathan Corbet escreveu: > On Tue, 8 Jun 2010 03:03:14 +0200 > Laurent Pinchart wrote: > >> If it's not too late for review, here are some comments. I've reviewed the >> code from bottom to top, so comments might be a bit inconsistent sometimes. > > Never too late to make the code better. These are good comments, thanks. > Mauro, I guess I've got another version coming...:) Ok, I'll be waiting for it ;) Cheers, Mauro -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Add the viafb video capture driver
On Tue, 8 Jun 2010 03:03:14 +0200
Laurent Pinchart wrote:
> If it's not too late for review, here are some comments. I've reviewed the
> code from bottom to top, so comments might be a bit inconsistent sometimes.
Never too late to make the code better. These are good comments, thanks.
Mauro, I guess I've got another version coming...:) It will take me a bit,
I've got another ocean to cross tomorrow.
Specific responses below. I've snipped out a fair number of comments; that
means "you're right, I'll fix it."
> Don't define device structures as static object. You must kmalloc the
> via_camera structure in probe and set the pointer as driver private data to
> access it later in V4L2 operations and device core callbacks. Otherwise Bad
> Things (TM) will happen if the device is removed while the video device node
> is opened.
I understand the comment...but this device is blasted onto the system's
base silicon. It's not going to be removed in a way which leaves a
functioning computer. Still, dynamic allocation is easy enough to do.
> > +/*
> > + * 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...
>
> What if the user sets vflip using VIDIOC_S_CTRL directly before setting the
> format ?
All is well; we remember the setting and set the flip properly afterward.
> > + /*
> > +* 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);
>
> Ouch that's going to hurt performances !
>
> What are the hardware restrictions regarding the memory it can capture images
> to ? Does it just have to be physically contiguous, or does the memory need
> to
> come from a specific memory area ? In the first case you could use
> videobuf_dma_contig and avoid the memcpy completely. In the second case you
> should still mmap the memory to userspace when using kernel-allocated buffers
> instead of memcpying the data. If you really need a memcpy, you should then
> probably use videobuf_vmalloc instead of videobuf_dma_sg.
It's a DMA copy, so performance is actually not a problem.
The video capture engine grabs frames into a three-buffer ring stored in
viafb framebuffer memory. I *could* let user space map that memory
directly, but it would be an eternal race with the engine and would not end
well. We really do have to do the copy. In a sense, the framebuffer
memory is just part of the capture device; the DMA operation is how we make
data available to the rest of the system.
[Incidentally, the biggest cost here, I think, is setting up 150 DMA
descriptors for each transfer. That's an artifact of the page-at-a-time
memory allocation used by videobuf_dma_sg. I have a branch with an SG
variant which tries to allocate the largest contiguous buffers possible
without going over; it reduces the number of descriptors to about five. It
didn't change my life a whole lot, so I back-burnered it, but I might
send that patch out one of these days.]
> > + viacam_write_reg(cam, VCR_CAPINTC, ~VCR_CI_ENABLE);
> > + viacam_write_reg(cam, VCR_CAPINTC, ~(VCR_CI_ENABLE|VCR_CI_CLKEN));
>
> I don't know how the VCR_CAPINTC register works, but did you really mean to
> write all bits to 1 except VCR_CI_ENABLE and VCR_CI_CLKEN ?
Ouch, no, I don't; that's meant to be a mask operation.
> > + /*
> > +* Disable a bunch of stuff.
> > +*/
> > + viacam_write_reg(cam, VCR_HORRANGE, 0x06200120);
> > + viacam_write_reg(cam, VCR_VERTRANGE, 0x01de);
>
> Any idea what that bunch of stuff is ? Replacing the magic numbers by
> #define'd constants would be nice.
It's 640x480, modulo weird VIA magic. I got it straight from them. I can
add a comment, though.
> > + (void) viacam_read_reg(cam, VCR_CAPINTC); /* Force post */
>
> Why a (void) cast ?
It's my way of saying that I meant to ignore the return value of a function
whose purpose is to return a value.
> > +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;
>
> Shouldn't the limit should be computed from the available fb memory ?
That would always be three, but user space might well want more buffering
than that. I don't quite see why the two need to be tied.
> > +static void viacam_vb_buf_queue(struct videobuf_queue *q,
> > + s
Re: [PATCH] Add the viafb video capture driver
Hi Jonathan,
On Tuesday 08 June 2010 01:26:15 Jonathan Corbet wrote:
> Hi, Mauro,
>
> Linus has quietly ignored a couple of pull requests for this driver; I
> guess he's gotten tired of me this time around or something. There's
> little profit in pushing the issue, so, can you just send it up through
> your tree (for 2.6.36 at this point) instead?
If it's not too late for review, here are some comments. I've reviewed the
code from bottom to top, so comments might be a bit inconsistent sometimes.
[snip]
> diff --git a/drivers/media/video/via-camera.c
> b/drivers/media/video/via-camera.c new file mode 100644
> index 000..7b1ff0c
> --- /dev/null
> +++ b/drivers/media/video/via-camera.c
[snip]
> +/*
> + * Basic window sizes.
> + */
> +#define VGA_WIDTH640
> +#define VGA_HEIGHT 480
That's already defined in include/linux/via-core.h (ugly defines though). It
would be better to define constants such as VIA_SENSOR_WIDTH,
VIA_xxx_MIN_WIDTH, VIA_xxx_MAX_WIDTH, ...
> +#define QCIF_WIDTH 176
> +#define QCIF_HEIGHT 144
> +
> +/*
> + * The structure describing our camera.
> + */
> +enum viacam_opstate { S_IDLE = 0, S_RUNNING = 1 };
> +
> +static 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;
> + /*
> + * 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;
> +} via_cam_info;
Don't define device structures as static object. You must kmalloc the
via_camera structure in probe and set the pointer as driver private data to
access it later in V4L2 operations and device core callbacks. Otherwise Bad
Things (TM) will happen if the device is removed while the video device node
is opened.
[snip]
> +/*
> + * 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...
What if the user sets vflip using VIDIOC_S_CTRL directly before setting the
format ?
> + */
> + if (ret == 0)
> + ret = viacam_set_flip(cam);
> + return ret;
> +}
[snip]
> +/*
> + * 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);
Ouch that's going to hurt performances !
What are the hardware restrictions regarding the memory it can capture images
to ? Does it just have to be physically conti
[PATCH] Add the viafb video capture driver
Hi, Mauro,
Linus has quietly ignored a couple of pull requests for this driver; I
guess he's gotten tired of me this time around or something. There's
little profit in pushing the issue, so, can you just send it up through
your tree (for 2.6.36 at this point) instead?
Thanks,
jon
---
Add the viafb video capture driver
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.
Signed-off-by: Jonathan Corbet
---
drivers/media/video/Kconfig | 10 +
drivers/media/video/Makefile |2 +
drivers/media/video/via-camera.c | 1368 ++
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, 1495 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 bdbc9d3..a26ded1 100644
--- a/drivers/media/video/Kconfig
+++ b/drivers/media/video/Kconfig
@@ -853,6 +853,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 cc93859..47fa0c0 100644
--- a/drivers/media/video/Makefile
+++ b/drivers/media/video/Makefile
@@ -126,6 +126,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_OV511) += ov511.o
obj-$(CONFIG_USB_SE401) += se401.o
diff --git a/drivers/media/video/via-camera.c b/drivers/media/video/via-camera.c
new file mode 100644
index 000..7b1ff0c
--- /dev/null
+++ b/drivers/media/video/via-camera.c
@@ -0,0 +1,1368 @@
+/*
+ * Driver for the VIA Chrome integrated camera controller.
+ *
+ * Copyright 2009,2010 Jonathan Corbet
+ * Distributable under the terms of the GNU General Public License, version 2
+ *
+ * This work was supported by the One Laptop Per Child project
+ */
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+#include
+
+#include "via-camera.h"
+
+MODULE_AUTHOR("Jonathan Corbet ");
+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
+#defineQCIF_HEIGHT 144
+
+/*
+ * The structure describing our camera.
+ */
+enum viacam_opstate { S_IDLE = 0, S_RUNNING = 1 };
+
+static 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;
+ /*
+* 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 me
