All of lore.kernel.org
 help / color / mirror / Atom feed
* [RFC] Second marvell-cam patch series
@ 2011-06-20 19:14 Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 1/5] marvell-cam: convert to videobuf2 Jonathan Corbet
                   ` (4 more replies)
  0 siblings, 5 replies; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee

OK, here's my second series of marvell-cam patches for comment; the main
thing here is (finally) the addition of videobuf2 vmalloc and dma-contig
support.  Anybody who would just rather look at the final product can grab:

	git://git.lwn.net/linux-2.6.git mmp-linuxtv

There is one real mystery here; ideas would be welcome.  When I switch to
contiguous DMA mode, mplayer gets faster as one might expect - copying all
those frames in the kernel hurts.  But a basic gstreamer pipeline:

  gst-launch v4l2src ! ffmpegcolorspace ! videoscale ! ximagesink

slows down by a factor of two.  Somehow the gst-launch binary finds a way
to use twice as much time processing frames.  Given that the difference
should not really even be visible to user space, I'm at a total loss here.

In this series:

Jonathan Corbet (5):
      marvell-cam: convert to videobuf2
      marvell-cam: include file cleanup
      marvell-cam: no need to initialize the DMA buffers
      marvell-cam: Don't spam the logs on frame loss
      marvell-cam: implement contiguous DMA operation

 Kconfig       |    4 
 cafe-driver.c |    6 
 mcam-core.c   |  785 +++++++++++++++++++++++++---------------------------------
 mcam-core.h   |   47 ++-
 mmp-driver.c  |    1 
 5 files changed, 387 insertions(+), 456 deletions(-)

Todo items at this point:

 - Scatter/gather DMA support (probably)
 - Eliminate ov7670 assumptions
 - Userptr support (should Just Work in DMA-contiguous mode)

Comments?

Thanks,

jon



^ permalink raw reply	[flat|nested] 10+ messages in thread
* [PATCH 1/5] marvell-cam: convert to videobuf2
  2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
@ 2011-06-20 19:14 ` Jonathan Corbet
  2011-06-22 13:59   ` Marek Szyprowski
  2011-06-20 19:14 ` [PATCH 2/5] marvell-cam: include file cleanup Jonathan Corbet
                   ` (3 subsequent siblings)
  4 siblings, 1 reply; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee, Jonathan Corbet

This is a basic, naive conversion to the videobuf2 infrastructure, removing
a lot of code in the process.  For now, we're using vmalloc, which is
suboptimal, but it does match what the cafe driver did before.  In the cafe
case, it may have to stay that way just because memory is too tight to do
direct streaming; mmp-camera will be able to do better.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/video/marvell-ccic/Kconfig     |    2 +
 drivers/media/video/marvell-ccic/mcam-core.c |  579 ++++++++------------------
 drivers/media/video/marvell-ccic/mcam-core.h |   26 +-
 3 files changed, 196 insertions(+), 411 deletions(-)

diff --git a/drivers/media/video/marvell-ccic/Kconfig b/drivers/media/video/marvell-ccic/Kconfig
index b4f7260..eb535b1 100644
--- a/drivers/media/video/marvell-ccic/Kconfig
+++ b/drivers/media/video/marvell-ccic/Kconfig
@@ -2,6 +2,7 @@ config VIDEO_CAFE_CCIC
 	tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
 	depends on PCI && I2C && VIDEO_V4L2
 	select VIDEO_OV7670
+	select VIDEOBUF2_VMALLOC
 	---help---
 	  This is a video4linux2 driver for the Marvell 88ALP01 integrated
 	  CMOS camera controller.  This is the controller found on first-
@@ -12,6 +13,7 @@ config VIDEO_MMP_CAMERA
 	depends on ARCH_MMP && I2C && VIDEO_V4L2
 	select VIDEO_OV7670
 	select I2C_GPIO
+	select VIDEOBUF2_VMALLOC
 	---help---
 	  This is a Video4Linux2 driver for the integrated camera
 	  controller found on Marvell Armada 610 application
diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
index 3e6a5e8..055d843 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -17,6 +17,7 @@
 #include <media/v4l2-ioctl.h>
 #include <media/v4l2-chip-ident.h>
 #include <media/ov7670.h>
+#include <media/videobuf2-vmalloc.h>
 #include <linux/device.h>
 #include <linux/wait.h>
 #include <linux/list.h>
@@ -149,7 +150,6 @@ static void mcam_reset_buffers(struct mcam_camera *cam)
 	cam->next_buf = -1;
 	for (i = 0; i < cam->nbufs; i++)
 		clear_bit(i, &cam->flags);
-	cam->specframes = 0;
 }
 
 static inline int mcam_needs_config(struct mcam_camera *cam)
@@ -165,6 +165,21 @@ static void mcam_set_config_needed(struct mcam_camera *cam, int needed)
 		clear_bit(CF_CONFIG_NEEDED, &cam->flags);
 }
 
+/*
+ * Our buffer type for working with videobuf2.  Note that the vb2
+ * developers have decreed that struct vb2_buffer must be at the
+ * beginning of this structure.
+ */
+struct mcam_vb_buffer {
+	struct vb2_buffer vb_buf;
+	struct list_head queue;
+};
+
+static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
+{
+	return container_of(vb, struct mcam_vb_buffer, vb_buf);
+}
+
 
 /*
  * Debugging and related.
@@ -339,9 +354,7 @@ static void mcam_ctlr_stop_dma(struct mcam_camera *cam)
 	spin_lock_irqsave(&cam->dev_lock, flags);
 	mcam_ctlr_stop(cam);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
-	mdelay(1);
-	wait_event_timeout(cam->iowait,
-			!test_bit(CF_DMA_ACTIVE, &cam->flags), HZ);
+	msleep(10);
 	if (test_bit(CF_DMA_ACTIVE, &cam->flags))
 		cam_err(cam, "Timeout waiting for DMA to end\n");
 		/* This would be bad news - what now? */
@@ -524,44 +537,11 @@ static void mcam_free_dma_bufs(struct mcam_camera *cam)
 
 
 
-
-
 /* ----------------------------------------------------------------------- */
 /*
  * Here starts the V4L2 interface code.
  */
 
-/*
- * Read an image from the device.
- */
-static ssize_t mcam_deliver_buffer(struct mcam_camera *cam,
-		char __user *buffer, size_t len, loff_t *pos)
-{
-	int bufno;
-	unsigned long flags;
-
-	spin_lock_irqsave(&cam->dev_lock, flags);
-	if (cam->next_buf < 0) {
-		cam_err(cam, "deliver_buffer: No next buffer\n");
-		spin_unlock_irqrestore(&cam->dev_lock, flags);
-		return -EIO;
-	}
-	bufno = cam->next_buf;
-	clear_bit(bufno, &cam->flags);
-	if (++(cam->next_buf) >= cam->nbufs)
-		cam->next_buf = 0;
-	if (!test_bit(cam->next_buf, &cam->flags))
-		cam->next_buf = -1;
-	cam->specframes = 0;
-	spin_unlock_irqrestore(&cam->dev_lock, flags);
-
-	if (len > cam->pix_format.sizeimage)
-		len = cam->pix_format.sizeimage;
-	if (copy_to_user(buffer, cam->dma_bufs[bufno], len))
-		return -EFAULT;
-	(*pos) += len;
-	return len;
-}
 
 /*
  * Get everything ready, and start grabbing frames.
@@ -598,75 +578,138 @@ static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
 	return 0;
 }
 
+/* ----------------------------------------------------------------------- */
+/*
+ * Videobuf2 interface code.
+ */
 
-static ssize_t mcam_v4l_read(struct file *filp,
-		char __user *buffer, size_t len, loff_t *pos)
+static int mcam_vb_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
+		unsigned int *num_planes, unsigned long sizes[],
+		void *alloc_ctxs[])
 {
-	struct mcam_camera *cam = filp->private_data;
-	int ret = 0;
+	struct mcam_camera *cam = vb2_get_drv_priv(vq);
+
+	sizes[0] = cam->pix_format.sizeimage;
+	*num_planes = 1; /* Someday we have to support planar formats... */
+	if (*nbufs < 2 || *nbufs > 32)
+		*nbufs = 6;  /* semi-arbitrary numbers */
+	return 0;
+}
+
+static int mcam_vb_buf_init(struct vb2_buffer *vb)
+{
+	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
+
+	INIT_LIST_HEAD(&mvb->queue);
+	return 0;
+}
+
+static void mcam_vb_buf_queue(struct vb2_buffer *vb)
+{
+	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
+	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
+	unsigned long flags;
+
+	spin_lock_irqsave(&cam->dev_lock, flags);
+	list_add(&cam->buffers, &mvb->queue);
+	spin_unlock_irqrestore(&cam->dev_lock, flags);
+}
+
+/*
+ * vb2 uses these to release the mutex when waiting in dqbuf.  I'm
+ * not actually sure we need to do this (I'm not sure that vb2_dqbuf() needs
+ * to be called with the mutex held), but better safe than sorry.
+ */
+static void mcam_vb_wait_prepare(struct vb2_queue *vq)
+{
+	struct mcam_camera *cam = vb2_get_drv_priv(vq);
+
+	mutex_unlock(&cam->s_mutex);
+}
+
+static void mcam_vb_wait_finish(struct vb2_queue *vq)
+{
+	struct mcam_camera *cam = vb2_get_drv_priv(vq);
 
-	/*
-	 * Perhaps we're in speculative read mode and already
-	 * have data?
-	 */
 	mutex_lock(&cam->s_mutex);
-	if (cam->state == S_SPECREAD) {
-		if (cam->next_buf >= 0) {
-			ret = mcam_deliver_buffer(cam, buffer, len, pos);
-			if (ret != 0)
-				goto out_unlock;
-		}
-	} else if (cam->state == S_FLAKED || cam->state == S_NOTREADY) {
-		ret = -EIO;
-		goto out_unlock;
-	} else if (cam->state != S_IDLE) {
-		ret = -EBUSY;
-		goto out_unlock;
-	}
+}
 
-	/*
-	 * v4l2: multiple processes can open the device, but only
-	 * one gets to grab data from it.
-	 */
-	if (cam->owner && cam->owner != filp) {
-		ret = -EBUSY;
-		goto out_unlock;
-	}
-	cam->owner = filp;
+/*
+ * These need to be called with the mutex held from vb2
+ */
+static int mcam_vb_start_streaming(struct vb2_queue *vq)
+{
+	struct mcam_camera *cam = vb2_get_drv_priv(vq);
+	int ret = -EINVAL;
 
-	/*
-	 * Do setup if need be.
-	 */
-	if (cam->state != S_SPECREAD) {
-		ret = mcam_read_setup(cam, S_SINGLEREAD);
-		if (ret)
-			goto out_unlock;
-	}
-	/*
-	 * Wait for something to happen.  This should probably
-	 * be interruptible (FIXME).
-	 */
-	wait_event_timeout(cam->iowait, cam->next_buf >= 0, HZ);
-	if (cam->next_buf < 0) {
-		cam_err(cam, "read() operation timed out\n");
-		mcam_ctlr_stop_dma(cam);
-		ret = -EIO;
-		goto out_unlock;
+	if (cam->state == S_IDLE) {
+		cam->sequence = 0;
+		ret = mcam_read_setup(cam, S_STREAMING);
 	}
+	return ret;
+}
+
+static int mcam_vb_stop_streaming(struct vb2_queue *vq)
+{
+	struct mcam_camera *cam = vb2_get_drv_priv(vq);
+	unsigned long flags;
+
+	if (cam->state != S_STREAMING)
+		return -EINVAL;
+	mcam_ctlr_stop_dma(cam);
 	/*
-	 * Give them their data and we should be done.
+	 * VB2 reclaims the buffers, so we need to forget
+	 * about them.
 	 */
-	ret = mcam_deliver_buffer(cam, buffer, len, pos);
-
-out_unlock:
-	mutex_unlock(&cam->s_mutex);
-	return ret;
+	spin_lock_irqsave(&cam->dev_lock, flags);
+	INIT_LIST_HEAD(&cam->buffers);
+	spin_unlock_irqrestore(&cam->dev_lock, flags);
+	return 0;
 }
 
 
+static const struct vb2_ops mcam_vb2_ops = {
+	.queue_setup		= mcam_vb_queue_setup,
+	.buf_init		= mcam_vb_buf_init,
+	.buf_queue		= mcam_vb_buf_queue,
+	.start_streaming	= mcam_vb_start_streaming,
+	.stop_streaming		= mcam_vb_stop_streaming,
+	.wait_prepare		= mcam_vb_wait_prepare,
+	.wait_finish		= mcam_vb_wait_finish,
+};
 
+static int mcam_setup_vb2(struct mcam_camera *cam)
+{
+	struct vb2_queue *vq = &cam->vb_queue;
 
+	memset(vq, 0, sizeof(*vq));
+	vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	vq->io_modes = VB2_MMAP;  /* Add userptr */
+	vq->drv_priv = cam;
+	vq->ops = &mcam_vb2_ops;
+	vq->mem_ops = &vb2_vmalloc_memops;
+	vq->buf_struct_size = sizeof(struct mcam_vb_buffer);
 
+	return vb2_queue_init(vq);
+}
+
+static void mcam_cleanup_vb2(struct mcam_camera *cam)
+{
+	vb2_queue_release(&cam->vb_queue);
+}
+
+static ssize_t mcam_v4l_read(struct file *filp,
+		char __user *buffer, size_t len, loff_t *pos)
+{
+	struct mcam_camera *cam = filp->private_data;
+	int ret;
+
+	mutex_lock(&cam->s_mutex);
+	ret = vb2_read(&cam->vb_queue, buffer, len, pos,
+			filp->f_flags & O_NONBLOCK);
+	mutex_unlock(&cam->s_mutex);
+	return ret;
+}
 
 
 
@@ -674,26 +717,15 @@ out_unlock:
  * Streaming I/O support.
  */
 
-
-
 static int mcam_vidioc_streamon(struct file *filp, void *priv,
 		enum v4l2_buf_type type)
 {
 	struct mcam_camera *cam = filp->private_data;
-	int ret = -EINVAL;
+	int ret;
 
-	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		goto out;
 	mutex_lock(&cam->s_mutex);
-	if (cam->state != S_IDLE || cam->n_sbufs == 0)
-		goto out_unlock;
-
-	cam->sequence = 0;
-	ret = mcam_read_setup(cam, S_STREAMING);
-
-out_unlock:
+	ret = vb2_streamon(&cam->vb_queue, type);
 	mutex_unlock(&cam->s_mutex);
-out:
 	return ret;
 }
 
@@ -702,137 +734,23 @@ static int mcam_vidioc_streamoff(struct file *filp, void *priv,
 		enum v4l2_buf_type type)
 {
 	struct mcam_camera *cam = filp->private_data;
-	int ret = -EINVAL;
+	int ret;
 
-	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
-		goto out;
 	mutex_lock(&cam->s_mutex);
-	if (cam->state != S_STREAMING)
-		goto out_unlock;
-
-	mcam_ctlr_stop_dma(cam);
-	ret = 0;
-
-out_unlock:
+	ret = vb2_streamoff(&cam->vb_queue, type);
 	mutex_unlock(&cam->s_mutex);
-out:
 	return ret;
 }
 
 
-
-static int mcam_setup_siobuf(struct mcam_camera *cam, int index)
-{
-	struct mcam_sio_buffer *buf = cam->sb_bufs + index;
-
-	INIT_LIST_HEAD(&buf->list);
-	buf->v4lbuf.length = PAGE_ALIGN(cam->pix_format.sizeimage);
-	buf->buffer = vmalloc_user(buf->v4lbuf.length);
-	if (buf->buffer == NULL)
-		return -ENOMEM;
-	buf->mapcount = 0;
-	buf->cam = cam;
-
-	buf->v4lbuf.index = index;
-	buf->v4lbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
-	buf->v4lbuf.field = V4L2_FIELD_NONE;
-	buf->v4lbuf.memory = V4L2_MEMORY_MMAP;
-	/*
-	 * Offset: must be 32-bit even on a 64-bit system.  videobuf-dma-sg
-	 * just uses the length times the index, but the spec warns
-	 * against doing just that - vma merging problems.  So we
-	 * leave a gap between each pair of buffers.
-	 */
-	buf->v4lbuf.m.offset = 2*index*buf->v4lbuf.length;
-	return 0;
-}
-
-static int mcam_free_sio_buffers(struct mcam_camera *cam)
-{
-	int i;
-
-	/*
-	 * If any buffers are mapped, we cannot free them at all.
-	 */
-	for (i = 0; i < cam->n_sbufs; i++)
-		if (cam->sb_bufs[i].mapcount > 0)
-			return -EBUSY;
-	/*
-	 * OK, let's do it.
-	 */
-	for (i = 0; i < cam->n_sbufs; i++)
-		vfree(cam->sb_bufs[i].buffer);
-	cam->n_sbufs = 0;
-	kfree(cam->sb_bufs);
-	cam->sb_bufs = NULL;
-	INIT_LIST_HEAD(&cam->sb_avail);
-	INIT_LIST_HEAD(&cam->sb_full);
-	return 0;
-}
-
-
-
 static int mcam_vidioc_reqbufs(struct file *filp, void *priv,
 		struct v4l2_requestbuffers *req)
 {
 	struct mcam_camera *cam = filp->private_data;
-	int ret = 0;  /* Silence warning */
+	int ret;
 
-	/*
-	 * Make sure it's something we can do.  User pointers could be
-	 * implemented without great pain, but that's not been done yet.
-	 */
-	if (req->memory != V4L2_MEMORY_MMAP)
-		return -EINVAL;
-	/*
-	 * If they ask for zero buffers, they really want us to stop streaming
-	 * (if it's happening) and free everything.  Should we check owner?
-	 */
 	mutex_lock(&cam->s_mutex);
-	if (req->count == 0) {
-		if (cam->state == S_STREAMING)
-			mcam_ctlr_stop_dma(cam);
-		ret = mcam_free_sio_buffers(cam);
-		goto out;
-	}
-	/*
-	 * Device needs to be idle and working.  We *could* try to do the
-	 * right thing in S_SPECREAD by shutting things down, but it
-	 * probably doesn't matter.
-	 */
-	if (cam->state != S_IDLE || (cam->owner && cam->owner != filp)) {
-		ret = -EBUSY;
-		goto out;
-	}
-	cam->owner = filp;
-
-	if (req->count < min_buffers)
-		req->count = min_buffers;
-	else if (req->count > max_buffers)
-		req->count = max_buffers;
-	if (cam->n_sbufs > 0) {
-		ret = mcam_free_sio_buffers(cam);
-		if (ret)
-			goto out;
-	}
-
-	cam->sb_bufs = kzalloc(req->count*sizeof(struct mcam_sio_buffer),
-			GFP_KERNEL);
-	if (cam->sb_bufs == NULL) {
-		ret = -ENOMEM;
-		goto out;
-	}
-	for (cam->n_sbufs = 0; cam->n_sbufs < req->count; (cam->n_sbufs++)) {
-		ret = mcam_setup_siobuf(cam, cam->n_sbufs);
-		if (ret)
-			break;
-	}
-
-	if (cam->n_sbufs == 0)  /* no luck at all - ret already set */
-		kfree(cam->sb_bufs);
-	req->count = cam->n_sbufs;  /* In case of partial success */
-
-out:
+	ret = vb2_reqbufs(&cam->vb_queue, req);
 	mutex_unlock(&cam->s_mutex);
 	return ret;
 }
@@ -842,14 +760,10 @@ static int mcam_vidioc_querybuf(struct file *filp, void *priv,
 		struct v4l2_buffer *buf)
 {
 	struct mcam_camera *cam = filp->private_data;
-	int ret = -EINVAL;
+	int ret;
 
 	mutex_lock(&cam->s_mutex);
-	if (buf->index >= cam->n_sbufs)
-		goto out;
-	*buf = cam->sb_bufs[buf->index].v4lbuf;
-	ret = 0;
-out:
+	ret = vb2_querybuf(&cam->vb_queue, buf);
 	mutex_unlock(&cam->s_mutex);
 	return ret;
 }
@@ -858,29 +772,10 @@ static int mcam_vidioc_qbuf(struct file *filp, void *priv,
 		struct v4l2_buffer *buf)
 {
 	struct mcam_camera *cam = filp->private_data;
-	struct mcam_sio_buffer *sbuf;
-	int ret = -EINVAL;
-	unsigned long flags;
+	int ret;
 
 	mutex_lock(&cam->s_mutex);
-	if (buf->index >= cam->n_sbufs)
-		goto out;
-	sbuf = cam->sb_bufs + buf->index;
-	if (sbuf->v4lbuf.flags & V4L2_BUF_FLAG_QUEUED) {
-		ret = 0; /* Already queued?? */
-		goto out;
-	}
-	if (sbuf->v4lbuf.flags & V4L2_BUF_FLAG_DONE) {
-		/* Spec doesn't say anything, seems appropriate tho */
-		ret = -EBUSY;
-		goto out;
-	}
-	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_QUEUED;
-	spin_lock_irqsave(&cam->dev_lock, flags);
-	list_add(&sbuf->list, &cam->sb_avail);
-	spin_unlock_irqrestore(&cam->dev_lock, flags);
-	ret = 0;
-out:
+	ret = vb2_qbuf(&cam->vb_queue, buf);
 	mutex_unlock(&cam->s_mutex);
 	return ret;
 }
@@ -889,111 +784,22 @@ static int mcam_vidioc_dqbuf(struct file *filp, void *priv,
 		struct v4l2_buffer *buf)
 {
 	struct mcam_camera *cam = filp->private_data;
-	struct mcam_sio_buffer *sbuf;
-	int ret = -EINVAL;
-	unsigned long flags;
+	int ret;
 
 	mutex_lock(&cam->s_mutex);
-	if (cam->state != S_STREAMING)
-		goto out_unlock;
-	if (list_empty(&cam->sb_full) && filp->f_flags & O_NONBLOCK) {
-		ret = -EAGAIN;
-		goto out_unlock;
-	}
-
-	while (list_empty(&cam->sb_full) && cam->state == S_STREAMING) {
-		mutex_unlock(&cam->s_mutex);
-		if (wait_event_interruptible(cam->iowait,
-						!list_empty(&cam->sb_full))) {
-			ret = -ERESTARTSYS;
-			goto out;
-		}
-		mutex_lock(&cam->s_mutex);
-	}
-
-	if (cam->state != S_STREAMING)
-		ret = -EINTR;
-	else {
-		spin_lock_irqsave(&cam->dev_lock, flags);
-		/* Should probably recheck !list_empty() here */
-		sbuf = list_entry(cam->sb_full.next,
-				struct mcam_sio_buffer, list);
-		list_del_init(&sbuf->list);
-		spin_unlock_irqrestore(&cam->dev_lock, flags);
-		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_DONE;
-		*buf = sbuf->v4lbuf;
-		ret = 0;
-	}
-
-out_unlock:
+	ret = vb2_dqbuf(&cam->vb_queue, buf, filp->f_flags & O_NONBLOCK);
 	mutex_unlock(&cam->s_mutex);
-out:
 	return ret;
 }
 
 
-
-static void mcam_v4l_vm_open(struct vm_area_struct *vma)
-{
-	struct mcam_sio_buffer *sbuf = vma->vm_private_data;
-	/*
-	 * Locking: done under mmap_sem, so we don't need to
-	 * go back to the camera lock here.
-	 */
-	sbuf->mapcount++;
-}
-
-
-static void mcam_v4l_vm_close(struct vm_area_struct *vma)
-{
-	struct mcam_sio_buffer *sbuf = vma->vm_private_data;
-
-	mutex_lock(&sbuf->cam->s_mutex);
-	sbuf->mapcount--;
-	/* Docs say we should stop I/O too... */
-	if (sbuf->mapcount == 0)
-		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_MAPPED;
-	mutex_unlock(&sbuf->cam->s_mutex);
-}
-
-static const struct vm_operations_struct mcam_v4l_vm_ops = {
-	.open = mcam_v4l_vm_open,
-	.close = mcam_v4l_vm_close
-};
-
-
 static int mcam_v4l_mmap(struct file *filp, struct vm_area_struct *vma)
 {
 	struct mcam_camera *cam = filp->private_data;
-	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
-	int ret = -EINVAL;
-	int i;
-	struct mcam_sio_buffer *sbuf = NULL;
+	int ret;
 
-	if (!(vma->vm_flags & VM_WRITE) || !(vma->vm_flags & VM_SHARED))
-		return -EINVAL;
-	/*
-	 * Find the buffer they are looking for.
-	 */
 	mutex_lock(&cam->s_mutex);
-	for (i = 0; i < cam->n_sbufs; i++)
-		if (cam->sb_bufs[i].v4lbuf.m.offset == offset) {
-			sbuf = cam->sb_bufs + i;
-			break;
-		}
-	if (sbuf == NULL)
-		goto out;
-
-	ret = remap_vmalloc_range(vma, sbuf->buffer, 0);
-	if (ret)
-		goto out;
-	vma->vm_flags |= VM_DONTEXPAND;
-	vma->vm_private_data = sbuf;
-	vma->vm_ops = &mcam_v4l_vm_ops;
-	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_MAPPED;
-	mcam_v4l_vm_open(vma);
-	ret = 0;
-out:
+	ret = vb2_mmap(&cam->vb_queue, vma);
 	mutex_unlock(&cam->s_mutex);
 	return ret;
 }
@@ -1003,19 +809,23 @@ out:
 static int mcam_v4l_open(struct file *filp)
 {
 	struct mcam_camera *cam = video_drvdata(filp);
+	int ret = 0;
 
 	filp->private_data = cam;
 
 	mutex_lock(&cam->s_mutex);
 	if (cam->users == 0) {
+		ret = mcam_setup_vb2(cam);
+		if (ret)
+			goto out;
 		mcam_ctlr_power_up(cam);
 		__mcam_cam_reset(cam);
 		mcam_set_config_needed(cam, 1);
-	/* FIXME make sure this is complete */
 	}
 	(cam->users)++;
+out:
 	mutex_unlock(&cam->s_mutex);
-	return 0;
+	return ret;
 }
 
 
@@ -1027,10 +837,10 @@ static int mcam_v4l_release(struct file *filp)
 	(cam->users)--;
 	if (filp == cam->owner) {
 		mcam_ctlr_stop_dma(cam);
-		mcam_free_sio_buffers(cam);
 		cam->owner = NULL;
 	}
 	if (cam->users == 0) {
+		mcam_cleanup_vb2(cam);
 		mcam_ctlr_power_down(cam);
 		if (alloc_bufs_at_read)
 			mcam_free_dma_bufs(cam);
@@ -1045,11 +855,12 @@ static unsigned int mcam_v4l_poll(struct file *filp,
 		struct poll_table_struct *pt)
 {
 	struct mcam_camera *cam = filp->private_data;
+	int ret;
 
-	poll_wait(filp, &cam->iowait, pt);
-	if (cam->next_buf >= 0)
-		return POLLIN | POLLRDNORM;
-	return 0;
+	mutex_lock(&cam->s_mutex);
+	ret = vb2_poll(&cam->vb_queue, filp, pt);
+	mutex_unlock(&cam->s_mutex);
+	return ret;
 }
 
 
@@ -1093,9 +904,6 @@ static int mcam_vidioc_s_ctrl(struct file *filp, void *priv,
 }
 
 
-
-
-
 static int mcam_vidioc_querycap(struct file *file, void *priv,
 		struct v4l2_capability *cap)
 {
@@ -1166,7 +974,7 @@ static int mcam_vidioc_s_fmt_vid_cap(struct file *filp, void *priv,
 	 * Can't do anything if the device is not idle
 	 * Also can't if there are streaming buffers in place.
 	 */
-	if (cam->state != S_IDLE || cam->n_sbufs > 0)
+	if (cam->state != S_IDLE || cam->vb_queue.num_buffers > 0)
 		return -EBUSY;
 
 	f = mcam_find_format(fmt->fmt.pix.pixelformat);
@@ -1416,39 +1224,39 @@ static void mcam_frame_tasklet(unsigned long data)
 	struct mcam_camera *cam = (struct mcam_camera *) data;
 	int i;
 	unsigned long flags;
-	struct mcam_sio_buffer *sbuf;
+	struct mcam_vb_buffer *buf;
 
 	spin_lock_irqsave(&cam->dev_lock, flags);
 	for (i = 0; i < cam->nbufs; i++) {
 		int bufno = cam->next_buf;
-		if (bufno < 0) {  /* "will never happen" */
-			cam_err(cam, "No valid bufs in tasklet!\n");
-			break;
-		}
+
+		if (cam->state != S_STREAMING || bufno < 0)
+			break;  /* I/O got stopped */
 		if (++(cam->next_buf) >= cam->nbufs)
 			cam->next_buf = 0;
 		if (!test_bit(bufno, &cam->flags))
 			continue;
-		if (list_empty(&cam->sb_avail))
+		if (list_empty(&cam->buffers))
 			break;  /* Leave it valid, hope for better later */
 		clear_bit(bufno, &cam->flags);
-		sbuf = list_entry(cam->sb_avail.next,
-				struct mcam_sio_buffer, list);
+		buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
+				queue);
+		list_del_init(&buf->queue);
 		/*
 		 * Drop the lock during the big copy.  This *should* be safe...
 		 */
 		spin_unlock_irqrestore(&cam->dev_lock, flags);
-		memcpy(sbuf->buffer, cam->dma_bufs[bufno],
+		memcpy(vb2_plane_vaddr(&buf->vb_buf, 0), cam->dma_bufs[bufno],
 				cam->pix_format.sizeimage);
-		sbuf->v4lbuf.bytesused = cam->pix_format.sizeimage;
-		sbuf->v4lbuf.sequence = cam->buf_seq[bufno];
-		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;
-		sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE;
+		buf->vb_buf.v4l2_buf.bytesused = cam->pix_format.sizeimage;
+		buf->vb_buf.v4l2_buf.sequence = cam->buf_seq[bufno];
+		buf->vb_buf.v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
+		buf->vb_buf.v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
+		vb2_set_plane_payload(&buf->vb_buf, 0,
+				cam->pix_format.sizeimage);
+		vb2_buffer_done(&buf->vb_buf, VB2_BUF_STATE_DONE);
 		spin_lock_irqsave(&cam->dev_lock, flags);
-		list_move_tail(&sbuf->list, &cam->sb_full);
 	}
-	if (!list_empty(&cam->sb_full))
-		wake_up(&cam->iowait);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
 }
 
@@ -1469,27 +1277,6 @@ static void mcam_frame_complete(struct mcam_camera *cam, int frame)
 
 	switch (cam->state) {
 	/*
-	 * If in single read mode, try going speculative.
-	 */
-	case S_SINGLEREAD:
-		cam->state = S_SPECREAD;
-		cam->specframes = 0;
-		wake_up(&cam->iowait);
-		break;
-
-	/*
-	 * If we are already doing speculative reads, and nobody is
-	 * reading them, just stop.
-	 */
-	case S_SPECREAD:
-		if (++(cam->specframes) >= cam->nbufs) {
-			mcam_ctlr_stop(cam);
-			mcam_ctlr_irq_disable(cam);
-			cam->state = S_IDLE;
-		}
-		wake_up(&cam->iowait);
-		break;
-	/*
 	 * For the streaming case, we defer the real work to the
 	 * camera tasklet.
 	 *
@@ -1570,12 +1357,10 @@ int mccic_register(struct mcam_camera *cam)
 	mutex_init(&cam->s_mutex);
 	cam->state = S_NOTREADY;
 	mcam_set_config_needed(cam, 1);
-	init_waitqueue_head(&cam->iowait);
 	cam->pix_format = mcam_def_pix_format;
 	cam->mbus_code = mcam_def_mbus_code;
 	INIT_LIST_HEAD(&cam->dev_list);
-	INIT_LIST_HEAD(&cam->sb_avail);
-	INIT_LIST_HEAD(&cam->sb_full);
+	INIT_LIST_HEAD(&cam->buffers);
 	tasklet_init(&cam->s_tasklet, mcam_frame_tasklet, (unsigned long) cam);
 
 	mcam_ctlr_init(cam);
@@ -1638,10 +1423,8 @@ void mccic_shutdown(struct mcam_camera *cam)
 		cam_warn(cam, "Removing a device with users!\n");
 		mcam_ctlr_power_down(cam);
 	}
+	vb2_queue_release(&cam->vb_queue);
 	mcam_free_dma_bufs(cam);
-	if (cam->n_sbufs > 0)
-		/* What if they are still mapped?  Shouldn't be, but... */
-		mcam_free_sio_buffers(cam);
 	video_unregister_device(&cam->vdev);
 	v4l2_device_unregister(&cam->v4l2_dev);
 }
@@ -1674,9 +1457,7 @@ int mccic_resume(struct mcam_camera *cam)
 	mutex_unlock(&cam->s_mutex);
 
 	set_bit(CF_CONFIG_NEEDED, &cam->flags);
-	if (cam->state == S_SPECREAD)
-		cam->state = S_IDLE;  /* Don't bother restarting */
-	else if (cam->state == S_SINGLEREAD || cam->state == S_STREAMING)
+	if (cam->state == S_STREAMING)
 		ret = mcam_read_setup(cam, cam->state);
 	return ret;
 }
diff --git a/drivers/media/video/marvell-ccic/mcam-core.h b/drivers/media/video/marvell-ccic/mcam-core.h
index 5effa82..f40450c 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.h
+++ b/drivers/media/video/marvell-ccic/mcam-core.h
@@ -3,6 +3,13 @@
  *
  * Copyright 2011 Jonathan Corbet corbet@lwn.net
  */
+#ifndef _MCAM_CORE_H
+#define _MCAM_CORE_H
+
+#include <linux/list.h>
+#include <media/v4l2-common.h>
+#include <media/v4l2-dev.h>
+#include <media/videobuf2-core.h>
 
 /*
  * Tracking of streaming I/O buffers.
@@ -20,8 +27,6 @@ enum mcam_state {
 	S_NOTREADY,	/* Not yet initialized */
 	S_IDLE,		/* Just hanging around */
 	S_FLAKED,	/* Some sort of problem */
-	S_SINGLEREAD,	/* In read() */
-	S_SPECREAD,	/* Speculative read (for future read()) */
 	S_STREAMING	/* Streaming data */
 };
 #define MAX_DMA_BUFS 3
@@ -70,21 +75,19 @@ struct mcam_camera {
 
 	struct list_head dev_list;	/* link to other devices */
 
+	/* Videobuf2 stuff */
+	struct vb2_queue vb_queue;
+	struct list_head buffers;	/* Available frames */
+
 	/* DMA buffers */
 	unsigned int nbufs;		/* How many are alloc'd */
 	int next_buf;			/* Next to consume (dev_lock) */
 	unsigned int dma_buf_size;	/* allocated size */
 	void *dma_bufs[MAX_DMA_BUFS];	/* Internal buffer addresses */
 	dma_addr_t dma_handles[MAX_DMA_BUFS]; /* Buffer bus addresses */
-	unsigned int specframes;	/* Unconsumed spec frames (dev_lock) */
 	unsigned int sequence;		/* Frame sequence number */
-	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual buffers */
+	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual bufs */
 
-	/* Streaming buffers */
-	unsigned int n_sbufs;		/* How many we have */
-	struct mcam_sio_buffer *sb_bufs; /* The array of housekeeping structs */
-	struct list_head sb_avail;	/* Available for data (we own) (dev_lock) */
-	struct list_head sb_full;	/* With data (user space owns) (dev_lock) */
 	struct tasklet_struct s_tasklet;
 
 	/* Current operating parameters */
@@ -94,9 +97,6 @@ struct mcam_camera {
 
 	/* Locks */
 	struct mutex s_mutex; /* Access to this structure */
-
-	/* Misc */
-	wait_queue_head_t iowait;	/* Waiting on frame data */
 };
 
 
@@ -257,3 +257,5 @@ int mccic_resume(struct mcam_camera *cam);
  */
 #define VGA_WIDTH	640
 #define VGA_HEIGHT	480
+
+#endif /* _MCAM_CORE_H */
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 2/5] marvell-cam: include file cleanup
  2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 1/5] marvell-cam: convert to videobuf2 Jonathan Corbet
@ 2011-06-20 19:14 ` Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 3/5] marvell-cam: no need to initialize the DMA buffers Jonathan Corbet
                   ` (2 subsequent siblings)
  4 siblings, 0 replies; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee, Jonathan Corbet

Put the includes into a slightly more readable ordering and get rid of a
few unneeded ones.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/video/marvell-ccic/mcam-core.c |   14 ++++++--------
 1 files changed, 6 insertions(+), 8 deletions(-)

diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
index 055d843..65d9c0f 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -11,22 +11,20 @@
 #include <linux/i2c.h>
 #include <linux/interrupt.h>
 #include <linux/spinlock.h>
-#include <linux/videodev2.h>
 #include <linux/slab.h>
-#include <media/v4l2-device.h>
-#include <media/v4l2-ioctl.h>
-#include <media/v4l2-chip-ident.h>
-#include <media/ov7670.h>
-#include <media/videobuf2-vmalloc.h>
 #include <linux/device.h>
 #include <linux/wait.h>
 #include <linux/list.h>
 #include <linux/dma-mapping.h>
 #include <linux/delay.h>
-#include <linux/jiffies.h>
 #include <linux/vmalloc.h>
-#include <linux/uaccess.h>
 #include <linux/io.h>
+#include <linux/videodev2.h>
+#include <media/v4l2-device.h>
+#include <media/v4l2-ioctl.h>
+#include <media/v4l2-chip-ident.h>
+#include <media/ov7670.h>
+#include <media/videobuf2-vmalloc.h>
 
 #include "mcam-core.h"
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 3/5] marvell-cam: no need to initialize the DMA buffers
  2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 1/5] marvell-cam: convert to videobuf2 Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 2/5] marvell-cam: include file cleanup Jonathan Corbet
@ 2011-06-20 19:14 ` Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 4/5] marvell-cam: Don't spam the logs on frame loss Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 5/5] marvell-cam: implement contiguous DMA operation Jonathan Corbet
  4 siblings, 0 replies; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee, Jonathan Corbet

This was an old debugging thing from years ago.  It's only done at
initialization time, but it's still unnecessary; take it out.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/video/marvell-ccic/mcam-core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
index 65d9c0f..da7ec2f 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -499,8 +499,6 @@ static int mcam_alloc_dma_bufs(struct mcam_camera *cam, int loadtime)
 			cam_warn(cam, "Failed to allocate DMA buffer\n");
 			break;
 		}
-		/* For debug, remove eventually */
-		memset(cam->dma_bufs[i], 0xcc, cam->dma_buf_size);
 		(cam->nbufs)++;
 	}
 
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 4/5] marvell-cam: Don't spam the logs on frame loss
  2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
                   ` (2 preceding siblings ...)
  2011-06-20 19:14 ` [PATCH 3/5] marvell-cam: no need to initialize the DMA buffers Jonathan Corbet
@ 2011-06-20 19:14 ` Jonathan Corbet
  2011-06-20 19:14 ` [PATCH 5/5] marvell-cam: implement contiguous DMA operation Jonathan Corbet
  4 siblings, 0 replies; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee, Jonathan Corbet

The sequence numbers already give that information if user space cares;
this is a frequent occurrence on slower machines, alas.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/video/marvell-ccic/mcam-core.c |    2 --
 1 files changed, 0 insertions(+), 2 deletions(-)

diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
index da7ec2f..ca3c56f 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -1263,8 +1263,6 @@ static void mcam_frame_complete(struct mcam_camera *cam, int frame)
 	/*
 	 * Basic frame housekeeping.
 	 */
-	if (test_bit(frame, &cam->flags) && printk_ratelimit())
-		cam_err(cam, "Frame overrun on %d, frames lost\n", frame);
 	set_bit(frame, &cam->flags);
 	clear_bit(CF_DMA_ACTIVE, &cam->flags);
 	if (cam->next_buf < 0)
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* [PATCH 5/5] marvell-cam: implement contiguous DMA operation
  2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
                   ` (3 preceding siblings ...)
  2011-06-20 19:14 ` [PATCH 4/5] marvell-cam: Don't spam the logs on frame loss Jonathan Corbet
@ 2011-06-20 19:14 ` Jonathan Corbet
  2011-06-21 20:26   ` Mauro Carvalho Chehab
  4 siblings, 1 reply; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-20 19:14 UTC (permalink / raw)
  To: linux-media; +Cc: g.liakhovetski, Kassey Lee, Jonathan Corbet

The core driver can now operate in either vmalloc or dma-contig modes;
obviously the latter is preferable when it is supported.  Default is
currently vmalloc on all platforms; load the module with buffer_mode=1 for
contiguous DMA mode.

Signed-off-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/video/marvell-ccic/Kconfig       |    1 +
 drivers/media/video/marvell-ccic/cafe-driver.c |    6 +
 drivers/media/video/marvell-ccic/mcam-core.c   |  230 ++++++++++++++++++------
 drivers/media/video/marvell-ccic/mcam-core.h   |   21 ++-
 drivers/media/video/marvell-ccic/mmp-driver.c  |    1 +
 5 files changed, 205 insertions(+), 54 deletions(-)

diff --git a/drivers/media/video/marvell-ccic/Kconfig b/drivers/media/video/marvell-ccic/Kconfig
index eb535b1..22314a0 100644
--- a/drivers/media/video/marvell-ccic/Kconfig
+++ b/drivers/media/video/marvell-ccic/Kconfig
@@ -14,6 +14,7 @@ config VIDEO_MMP_CAMERA
 	select VIDEO_OV7670
 	select I2C_GPIO
 	select VIDEOBUF2_VMALLOC
+	select VIDEOBUF2_DMA_CONTIG
 	---help---
 	  This is a Video4Linux2 driver for the integrated camera
 	  controller found on Marvell Armada 610 application
diff --git a/drivers/media/video/marvell-ccic/cafe-driver.c b/drivers/media/video/marvell-ccic/cafe-driver.c
index 6a29cc1..d030f9b 100644
--- a/drivers/media/video/marvell-ccic/cafe-driver.c
+++ b/drivers/media/video/marvell-ccic/cafe-driver.c
@@ -482,6 +482,12 @@ static int cafe_pci_probe(struct pci_dev *pdev,
 	mcam->clock_speed = 45;
 	mcam->use_smbus = 1;
 	/*
+	 * Vmalloc mode for buffers is traditional with this driver.
+	 * We *might* be able to run DMA_contig, especially on a system
+	 * with CMA in it.
+	 */
+	mcam->buffer_mode = B_vmalloc;
+	/*
 	 * Get set up on the PCI bus.
 	 */
 	ret = pci_enable_device(pdev);
diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
index ca3c56f..419b4e5 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.c
+++ b/drivers/media/video/marvell-ccic/mcam-core.c
@@ -25,9 +25,16 @@
 #include <media/v4l2-chip-ident.h>
 #include <media/ov7670.h>
 #include <media/videobuf2-vmalloc.h>
+#include <media/videobuf2-dma-contig.h>
 
 #include "mcam-core.h"
 
+/*
+ * Basic frame stats - to be deleted shortly
+ */
+static int frames;
+static int singles;
+static int delivered;
 
 /*
  * Internal DMA buffer management.  Since the controller cannot do S/G I/O,
@@ -48,7 +55,8 @@ MODULE_PARM_DESC(alloc_bufs_at_read,
 		"Non-zero value causes DMA buffers to be allocated when the "
 		"video capture device is read, rather than at module load "
 		"time.  This saves memory, but decreases the chances of "
-		"successfully getting those buffers.");
+		"successfully getting those buffers.  This parameter is "
+		"only used in the vmalloc buffer mode");
 
 static int n_dma_bufs = 3;
 module_param(n_dma_bufs, uint, 0644);
@@ -82,6 +90,13 @@ MODULE_PARM_DESC(flip,
 		"If set, the sensor will be instructed to flip the image "
 		"vertically.");
 
+static int buffer_mode = -1;
+module_param(buffer_mode, int, 0444);
+MODULE_PARM_DESC(buffer_mode,
+		"Set the buffer mode to be used; default is to go with what "
+		"the platform driver asks for.  Set to 0 for vmalloc, 1 for "
+		"DMA contiguous.");
+
 /*
  * Status flags.  Always manipulated with bit operations.
  */
@@ -90,6 +105,7 @@ MODULE_PARM_DESC(flip,
 #define CF_BUF2_VALID	 2
 #define CF_DMA_ACTIVE	 3	/* A frame is incoming */
 #define CF_CONFIG_NEEDED 4	/* Must configure hardware */
+#define CF_SINGLE_BUFFER 5	/* Running with a single buffer */
 
 #define sensor_call(cam, o, f, args...) \
 	v4l2_subdev_call(cam->sensor, o, f, ##args)
@@ -197,10 +213,9 @@ static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
  */
 
 /*
- * Do everything we think we need to have the interface operating
- * according to the desired format.
+ * Set up DMA buffers when operating in vmalloc mode
  */
-static void mcam_ctlr_dma(struct mcam_camera *cam)
+static void mcam_ctlr_dma_vmalloc(struct mcam_camera *cam)
 {
 	/*
 	 * Store the first two Y buffers (we aren't supporting
@@ -219,6 +234,57 @@ static void mcam_ctlr_dma(struct mcam_camera *cam)
 		mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only */
 }
 
+/*
+ * Set up a contiguous buffer for the given frame.  Here also is where
+ * the underrun strategy is set: if there is no buffer available, reuse
+ * the buffer from the other BAR and set the CF_SINGLE_BUFFER flag to
+ * keep the interrupt handler from giving that buffer back to user
+ * space.  In this way, we always have a buffer to DMA to and don't
+ * have to try to play games stopping and restarting the controller.
+ */
+static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
+{
+	struct mcam_vb_buffer *buf;
+	/*
+	 * If there are no available buffers, go into single mode
+	 */
+	if (list_empty(&cam->buffers)) {
+		buf = cam->vb_bufs[frame ^ 0x1];
+		cam->vb_bufs[frame] = buf;
+		mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
+				vb2_dma_contig_plane_paddr(&buf->vb_buf, 0));
+		set_bit(CF_SINGLE_BUFFER, &cam->flags);
+		singles++;
+		return;
+	}
+	/*
+	 * OK, we have a buffer we can use.
+	 */
+	buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer, queue);
+	list_del_init(&buf->queue);
+	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
+			vb2_dma_contig_plane_paddr(&buf->vb_buf, 0));
+	cam->vb_bufs[frame] = buf;
+	clear_bit(CF_SINGLE_BUFFER, &cam->flags);
+}
+
+static void mcam_ctlr_dma_contig(struct mcam_camera *cam)
+{
+	mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
+	cam->nbufs = 2;
+	mcam_set_contig_buffer(cam, 0);
+	mcam_set_contig_buffer(cam, 1);
+}
+
+
+static void mcam_ctlr_dma(struct mcam_camera *cam)
+{
+	if (cam->buffer_mode == B_DMA_contig)
+		mcam_ctlr_dma_contig(cam);
+	else
+		mcam_ctlr_dma_vmalloc(cam);
+}
+
 static void mcam_ctlr_image(struct mcam_camera *cam)
 {
 	int imgsz;
@@ -542,7 +608,7 @@ static void mcam_free_dma_bufs(struct mcam_camera *cam)
 /*
  * Get everything ready, and start grabbing frames.
  */
-static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
+static int mcam_read_setup(struct mcam_camera *cam)
 {
 	int ret;
 	unsigned long flags;
@@ -551,9 +617,9 @@ static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
 	 * Configuration.  If we still don't have DMA buffers,
 	 * make one last, desperate attempt.
 	 */
-	if (cam->nbufs == 0)
-		if (mcam_alloc_dma_bufs(cam, 0))
-			return -ENOMEM;
+	if (cam->buffer_mode == B_vmalloc && cam->nbufs == 0 &&
+			mcam_alloc_dma_bufs(cam, 0))
+		return -ENOMEM;
 
 	if (mcam_needs_config(cam)) {
 		mcam_cam_configure(cam);
@@ -568,7 +634,7 @@ static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
 	spin_lock_irqsave(&cam->dev_lock, flags);
 	mcam_reset_buffers(cam);
 	mcam_ctlr_irq_enable(cam);
-	cam->state = state;
+	cam->state = S_STREAMING;
 	mcam_ctlr_start(cam);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
 	return 0;
@@ -587,8 +653,10 @@ static int mcam_vb_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
 
 	sizes[0] = cam->pix_format.sizeimage;
 	*num_planes = 1; /* Someday we have to support planar formats... */
-	if (*nbufs < 2 || *nbufs > 32)
-		*nbufs = 6;  /* semi-arbitrary numbers */
+	if (*nbufs < 3 || *nbufs > 32)
+		*nbufs = 3;  /* semi-arbitrary numbers */
+	if (cam->buffer_mode == B_DMA_contig)
+		alloc_ctxs[0] = cam->vb_alloc_ctx;
 	return 0;
 }
 
@@ -605,10 +673,14 @@ static void mcam_vb_buf_queue(struct vb2_buffer *vb)
 	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
 	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
 	unsigned long flags;
+	int start;
 
 	spin_lock_irqsave(&cam->dev_lock, flags);
-	list_add(&cam->buffers, &mvb->queue);
+	start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
+	list_add(&mvb->queue, &cam->buffers);
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
+	if (start)
+		mcam_read_setup(cam);
 }
 
 /*
@@ -636,13 +708,22 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
 static int mcam_vb_start_streaming(struct vb2_queue *vq)
 {
 	struct mcam_camera *cam = vb2_get_drv_priv(vq);
-	int ret = -EINVAL;
 
-	if (cam->state == S_IDLE) {
-		cam->sequence = 0;
-		ret = mcam_read_setup(cam, S_STREAMING);
+	if (cam->state != S_IDLE)
+		return -EINVAL;
+	cam->sequence = 0;
+	/*
+	 * Videobuf2 sneakily hoards all the buffers and won't
+	 * give them to us until *after* streaming starts.  But
+	 * we can't actually start streaming until we have a
+	 * destination.  So go into a wait state and hope they
+	 * give us buffers soon.
+	 */
+	if (cam->buffer_mode != B_vmalloc && list_empty(&cam->buffers)) {
+		cam->state = S_BUFWAIT;
+		return 0;
 	}
-	return ret;
+	return mcam_read_setup(cam);
 }
 
 static int mcam_vb_stop_streaming(struct vb2_queue *vq)
@@ -650,6 +731,11 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
 	struct mcam_camera *cam = vb2_get_drv_priv(vq);
 	unsigned long flags;
 
+	if (cam->state == S_BUFWAIT) {
+		/* They never gave us buffers */
+		cam->state = S_IDLE;
+		return 0;
+	}
 	if (cam->state != S_STREAMING)
 		return -EINVAL;
 	mcam_ctlr_stop_dma(cam);
@@ -683,7 +769,11 @@ static int mcam_setup_vb2(struct mcam_camera *cam)
 	vq->io_modes = VB2_MMAP;  /* Add userptr */
 	vq->drv_priv = cam;
 	vq->ops = &mcam_vb2_ops;
-	vq->mem_ops = &vb2_vmalloc_memops;
+	if (cam->buffer_mode == B_DMA_contig) {
+		vq->mem_ops = &vb2_dma_contig_memops;
+		cam->vb_alloc_ctx = vb2_dma_contig_init_ctx(cam->dev);
+	} else
+		vq->mem_ops = &vb2_vmalloc_memops;
 	vq->buf_struct_size = sizeof(struct mcam_vb_buffer);
 
 	return vb2_queue_init(vq);
@@ -692,6 +782,8 @@ static int mcam_setup_vb2(struct mcam_camera *cam)
 static void mcam_cleanup_vb2(struct mcam_camera *cam)
 {
 	vb2_queue_release(&cam->vb_queue);
+	if (cam->buffer_mode == B_DMA_contig)
+		vb2_dma_contig_cleanup_ctx(cam->vb_alloc_ctx);
 }
 
 static ssize_t mcam_v4l_read(struct file *filp,
@@ -809,6 +901,7 @@ static int mcam_v4l_open(struct file *filp)
 
 	filp->private_data = cam;
 
+	frames = singles = delivered = 0;
 	mutex_lock(&cam->s_mutex);
 	if (cam->users == 0) {
 		ret = mcam_setup_vb2(cam);
@@ -829,6 +922,8 @@ static int mcam_v4l_release(struct file *filp)
 {
 	struct mcam_camera *cam = filp->private_data;
 
+	cam_err(cam, "Release, %d frames, %d singles, %d delivered\n", frames,
+			singles, delivered);
 	mutex_lock(&cam->s_mutex);
 	(cam->users)--;
 	if (filp == cam->owner) {
@@ -838,7 +933,7 @@ static int mcam_v4l_release(struct file *filp)
 	if (cam->users == 0) {
 		mcam_cleanup_vb2(cam);
 		mcam_ctlr_power_down(cam);
-		if (alloc_bufs_at_read)
+		if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read)
 			mcam_free_dma_bufs(cam);
 	}
 	mutex_unlock(&cam->s_mutex);
@@ -993,18 +1088,17 @@ static int mcam_vidioc_s_fmt_vid_cap(struct file *filp, void *priv,
 	 * Make sure we have appropriate DMA buffers.
 	 */
 	ret = -ENOMEM;
-	if (cam->nbufs > 0 && cam->dma_buf_size < cam->pix_format.sizeimage)
-		mcam_free_dma_bufs(cam);
-	if (cam->nbufs == 0) {
-		if (mcam_alloc_dma_bufs(cam, 0))
-			goto out;
+	if (cam->buffer_mode == B_vmalloc) {
+		if (cam->nbufs > 0 &&
+				cam->dma_buf_size < cam->pix_format.sizeimage)
+			mcam_free_dma_bufs(cam);
+		if (cam->nbufs == 0) {
+			if (mcam_alloc_dma_bufs(cam, 0))
+				goto out;
+		}
 	}
-	/*
-	 * It looks like this might work, so let's program the sensor.
-	 */
-	ret = mcam_cam_configure(cam);
-	if (!ret)
-		ret = mcam_ctlr_configure(cam);
+	mcam_set_config_needed(cam, 1);
+	ret = 0;
 out:
 	mutex_unlock(&cam->s_mutex);
 	return ret;
@@ -1214,7 +1308,20 @@ static struct video_device mcam_v4l_template = {
  */
 
 
+static void mcam_buffer_done(struct mcam_camera *cam, int frame,
+		struct vb2_buffer *vbuf)
+{
+	vbuf->v4l2_buf.bytesused = cam->pix_format.sizeimage;
+	vbuf->v4l2_buf.sequence = cam->buf_seq[frame];
+	vbuf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
+	vbuf->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
+	vb2_set_plane_payload(vbuf, 0, cam->pix_format.sizeimage);
+	vb2_buffer_done(vbuf, VB2_BUF_STATE_DONE);
+}
 
+/*
+ * Copy data out to user space in the vmalloc case
+ */
 static void mcam_frame_tasklet(unsigned long data)
 {
 	struct mcam_camera *cam = (struct mcam_camera *) data;
@@ -1232,8 +1339,11 @@ static void mcam_frame_tasklet(unsigned long data)
 			cam->next_buf = 0;
 		if (!test_bit(bufno, &cam->flags))
 			continue;
-		if (list_empty(&cam->buffers))
+		if (list_empty(&cam->buffers)) {
+			singles++;
 			break;  /* Leave it valid, hope for better later */
+		}
+		delivered++;
 		clear_bit(bufno, &cam->flags);
 		buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
 				queue);
@@ -1244,18 +1354,25 @@ static void mcam_frame_tasklet(unsigned long data)
 		spin_unlock_irqrestore(&cam->dev_lock, flags);
 		memcpy(vb2_plane_vaddr(&buf->vb_buf, 0), cam->dma_bufs[bufno],
 				cam->pix_format.sizeimage);
-		buf->vb_buf.v4l2_buf.bytesused = cam->pix_format.sizeimage;
-		buf->vb_buf.v4l2_buf.sequence = cam->buf_seq[bufno];
-		buf->vb_buf.v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
-		buf->vb_buf.v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
-		vb2_set_plane_payload(&buf->vb_buf, 0,
-				cam->pix_format.sizeimage);
-		vb2_buffer_done(&buf->vb_buf, VB2_BUF_STATE_DONE);
+		mcam_buffer_done(cam, bufno, &buf->vb_buf);
 		spin_lock_irqsave(&cam->dev_lock, flags);
 	}
 	spin_unlock_irqrestore(&cam->dev_lock, flags);
 }
 
+/*
+ * For direct DMA, mark the buffer ready and set up another one.
+ */
+static void mcam_dma_complete(struct mcam_camera *cam, int frame)
+{
+	struct mcam_vb_buffer *buf = cam->vb_bufs[frame];
+
+	if (!test_bit(CF_SINGLE_BUFFER, &cam->flags)) {
+		delivered++;
+		mcam_buffer_done(cam, frame, &buf->vb_buf);
+	}
+	mcam_set_contig_buffer(cam, frame);
+}
 
 
 static void mcam_frame_complete(struct mcam_camera *cam, int frame)
@@ -1265,21 +1382,20 @@ static void mcam_frame_complete(struct mcam_camera *cam, int frame)
 	 */
 	set_bit(frame, &cam->flags);
 	clear_bit(CF_DMA_ACTIVE, &cam->flags);
-	if (cam->next_buf < 0)
-		cam->next_buf = frame;
+	cam->next_buf = frame;
 	cam->buf_seq[frame] = ++(cam->sequence);
+	cam->last_delivered = frame;
 
+	frames++;
 	switch (cam->state) {
 	/*
-	 * For the streaming case, we defer the real work to the
-	 * camera tasklet.
-	 *
-	 * FIXME: if the application is not consuming the buffers,
-	 * we should eventually put things on hold and restart in
-	 * vidioc_dqbuf().
+	 * We're streaming and have a ready frame, hand it back
 	 */
 	case S_STREAMING:
-		tasklet_schedule(&cam->s_tasklet);
+		if (cam->buffer_mode == B_vmalloc)
+			tasklet_schedule(&cam->s_tasklet);
+		else
+			mcam_dma_complete(cam, frame);
 		break;
 
 	default:
@@ -1356,7 +1472,18 @@ int mccic_register(struct mcam_camera *cam)
 	INIT_LIST_HEAD(&cam->dev_list);
 	INIT_LIST_HEAD(&cam->buffers);
 	tasklet_init(&cam->s_tasklet, mcam_frame_tasklet, (unsigned long) cam);
-
+	/*
+	 * User space may want to override the asked-for buffer mode;
+	 * here's hoping they know what they're doing.
+	 */
+	if (buffer_mode == 0)
+		cam->buffer_mode = B_vmalloc;
+	else if (buffer_mode == 1)
+		cam->buffer_mode = B_DMA_contig;
+	else if (buffer_mode != -1)
+		printk(KERN_ERR "marvel-cam: "
+				"Strange module buffer mode %d - ignoring\n",
+				buffer_mode);
 	mcam_ctlr_init(cam);
 
 	/*
@@ -1390,7 +1517,7 @@ int mccic_register(struct mcam_camera *cam)
 	/*
 	 * If so requested, try to get our DMA buffers now.
 	 */
-	if (!alloc_bufs_at_read) {
+	if (cam->buffer_mode == B_vmalloc && !alloc_bufs_at_read) {
 		if (mcam_alloc_dma_bufs(cam, 1))
 			cam_warn(cam, "Unable to alloc DMA buffers at load"
 					" will try again later.");
@@ -1418,7 +1545,8 @@ void mccic_shutdown(struct mcam_camera *cam)
 		mcam_ctlr_power_down(cam);
 	}
 	vb2_queue_release(&cam->vb_queue);
-	mcam_free_dma_bufs(cam);
+	if (cam->buffer_mode == B_vmalloc)
+		mcam_free_dma_bufs(cam);
 	video_unregister_device(&cam->vdev);
 	v4l2_device_unregister(&cam->v4l2_dev);
 }
@@ -1452,7 +1580,7 @@ int mccic_resume(struct mcam_camera *cam)
 
 	set_bit(CF_CONFIG_NEEDED, &cam->flags);
 	if (cam->state == S_STREAMING)
-		ret = mcam_read_setup(cam, cam->state);
+		ret = mcam_read_setup(cam);
 	return ret;
 }
 #endif /* CONFIG_PM */
diff --git a/drivers/media/video/marvell-ccic/mcam-core.h b/drivers/media/video/marvell-ccic/mcam-core.h
index f40450c..2e667a0 100644
--- a/drivers/media/video/marvell-ccic/mcam-core.h
+++ b/drivers/media/video/marvell-ccic/mcam-core.h
@@ -27,11 +27,21 @@ enum mcam_state {
 	S_NOTREADY,	/* Not yet initialized */
 	S_IDLE,		/* Just hanging around */
 	S_FLAKED,	/* Some sort of problem */
-	S_STREAMING	/* Streaming data */
+	S_STREAMING,	/* Streaming data */
+	S_BUFWAIT	/* streaming requested but no buffers yet */
 };
 #define MAX_DMA_BUFS 3
 
 /*
+ * Different platforms work best with different buffer modes, so we
+ * let the platform pick.
+ */
+enum mcam_buffer_mode {
+	B_vmalloc = 0,
+	B_DMA_contig
+};
+
+/*
  * A description of one of our devices.
  * Locking: controlled by s_mutex.  Certain fields, however, require
  *          the dev_lock spinlock; they are marked as such by comments.
@@ -49,7 +59,7 @@ struct mcam_camera {
 	unsigned int chip_id;
 	short int clock_speed;	/* Sensor clock speed, default 30 */
 	short int use_smbus;	/* SMBUS or straight I2c? */
-
+	enum mcam_buffer_mode buffer_mode;
 	/*
 	 * Callbacks from the core to the platform code.
 	 */
@@ -79,7 +89,7 @@ struct mcam_camera {
 	struct vb2_queue vb_queue;
 	struct list_head buffers;	/* Available frames */
 
-	/* DMA buffers */
+	/* DMA buffers - vmalloc mode */
 	unsigned int nbufs;		/* How many are alloc'd */
 	int next_buf;			/* Next to consume (dev_lock) */
 	unsigned int dma_buf_size;	/* allocated size */
@@ -88,6 +98,11 @@ struct mcam_camera {
 	unsigned int sequence;		/* Frame sequence number */
 	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual bufs */
 
+	/* DMA buffers - contiguous DMA mode */
+	struct mcam_vb_buffer *vb_bufs[MAX_DMA_BUFS];
+	struct vb2_alloc_ctx *vb_alloc_ctx;
+	unsigned short last_delivered;
+
 	struct tasklet_struct s_tasklet;
 
 	/* Current operating parameters */
diff --git a/drivers/media/video/marvell-ccic/mmp-driver.c b/drivers/media/video/marvell-ccic/mmp-driver.c
index ac9976f..7b9c48c 100644
--- a/drivers/media/video/marvell-ccic/mmp-driver.c
+++ b/drivers/media/video/marvell-ccic/mmp-driver.c
@@ -180,6 +180,7 @@ static int mmpcam_probe(struct platform_device *pdev)
 	mcam->dev = &pdev->dev;
 	mcam->use_smbus = 0;
 	mcam->chip_id = V4L2_IDENT_ARMADA610;
+	mcam->buffer_mode = B_vmalloc;  /* Switch to dma */
 	spin_lock_init(&mcam->dev_lock);
 	/*
 	 * Get our I/O memory.
-- 
1.7.5.4


^ permalink raw reply related	[flat|nested] 10+ messages in thread
* Re: [PATCH 5/5] marvell-cam: implement contiguous DMA operation
  2011-06-20 19:14 ` [PATCH 5/5] marvell-cam: implement contiguous DMA operation Jonathan Corbet
@ 2011-06-21 20:26   ` Mauro Carvalho Chehab
  2011-06-22 14:22     ` Marek Szyprowski
  0 siblings, 1 reply; 10+ messages in thread
From: Mauro Carvalho Chehab @ 2011-06-21 20:26 UTC (permalink / raw)
  To: Jonathan Corbet; +Cc: linux-media, g.liakhovetski, Kassey Lee

Em 20-06-2011 16:14, Jonathan Corbet escreveu:
> The core driver can now operate in either vmalloc or dma-contig modes;
> obviously the latter is preferable when it is supported.  Default is
> currently vmalloc on all platforms; load the module with buffer_mode=1 for
> contiguous DMA mode.

Patch looks correct.

A side note for vb2 maintainers:

IMO, vb2 core should take the responsibility to allow to switch between DMA
scatter/gather and continuous (and, eventually, vmalloc), where the bridge driver
support more than one option.

Otherwise, we'll end by having codes similar to that on all drivers that can be used
on different architectures.

> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
>  drivers/media/video/marvell-ccic/Kconfig       |    1 +
>  drivers/media/video/marvell-ccic/cafe-driver.c |    6 +
>  drivers/media/video/marvell-ccic/mcam-core.c   |  230 ++++++++++++++++++------
>  drivers/media/video/marvell-ccic/mcam-core.h   |   21 ++-
>  drivers/media/video/marvell-ccic/mmp-driver.c  |    1 +
>  5 files changed, 205 insertions(+), 54 deletions(-)
>
> diff --git a/drivers/media/video/marvell-ccic/Kconfig b/drivers/media/video/marvell-ccic/Kconfig
> index eb535b1..22314a0 100644
> --- a/drivers/media/video/marvell-ccic/Kconfig
> +++ b/drivers/media/video/marvell-ccic/Kconfig
> @@ -14,6 +14,7 @@ config VIDEO_MMP_CAMERA
>  	select VIDEO_OV7670
>  	select I2C_GPIO
>  	select VIDEOBUF2_VMALLOC
> +	select VIDEOBUF2_DMA_CONTIG
>  	---help---
>  	  This is a Video4Linux2 driver for the integrated camera
>  	  controller found on Marvell Armada 610 application
> diff --git a/drivers/media/video/marvell-ccic/cafe-driver.c b/drivers/media/video/marvell-ccic/cafe-driver.c
> index 6a29cc1..d030f9b 100644
> --- a/drivers/media/video/marvell-ccic/cafe-driver.c
> +++ b/drivers/media/video/marvell-ccic/cafe-driver.c
> @@ -482,6 +482,12 @@ static int cafe_pci_probe(struct pci_dev *pdev,
>  	mcam->clock_speed = 45;
>  	mcam->use_smbus = 1;
>  	/*
> +	 * Vmalloc mode for buffers is traditional with this driver.
> +	 * We *might* be able to run DMA_contig, especially on a system
> +	 * with CMA in it.
> +	 */
> +	mcam->buffer_mode = B_vmalloc;
> +	/*
>  	 * Get set up on the PCI bus.
>  	 */
>  	ret = pci_enable_device(pdev);
> diff --git a/drivers/media/video/marvell-ccic/mcam-core.c b/drivers/media/video/marvell-ccic/mcam-core.c
> index ca3c56f..419b4e5 100644
> --- a/drivers/media/video/marvell-ccic/mcam-core.c
> +++ b/drivers/media/video/marvell-ccic/mcam-core.c
> @@ -25,9 +25,16 @@
>  #include <media/v4l2-chip-ident.h>
>  #include <media/ov7670.h>
>  #include <media/videobuf2-vmalloc.h>
> +#include <media/videobuf2-dma-contig.h>
>  
>  #include "mcam-core.h"
>  
> +/*
> + * Basic frame stats - to be deleted shortly
> + */
> +static int frames;
> +static int singles;
> +static int delivered;
>  
>  /*
>   * Internal DMA buffer management.  Since the controller cannot do S/G I/O,
> @@ -48,7 +55,8 @@ MODULE_PARM_DESC(alloc_bufs_at_read,
>  		"Non-zero value causes DMA buffers to be allocated when the "
>  		"video capture device is read, rather than at module load "
>  		"time.  This saves memory, but decreases the chances of "
> -		"successfully getting those buffers.");
> +		"successfully getting those buffers.  This parameter is "
> +		"only used in the vmalloc buffer mode");
>  
>  static int n_dma_bufs = 3;
>  module_param(n_dma_bufs, uint, 0644);
> @@ -82,6 +90,13 @@ MODULE_PARM_DESC(flip,
>  		"If set, the sensor will be instructed to flip the image "
>  		"vertically.");
>  
> +static int buffer_mode = -1;
> +module_param(buffer_mode, int, 0444);
> +MODULE_PARM_DESC(buffer_mode,
> +		"Set the buffer mode to be used; default is to go with what "
> +		"the platform driver asks for.  Set to 0 for vmalloc, 1 for "
> +		"DMA contiguous.");
> +
>  /*
>   * Status flags.  Always manipulated with bit operations.
>   */
> @@ -90,6 +105,7 @@ MODULE_PARM_DESC(flip,
>  #define CF_BUF2_VALID	 2
>  #define CF_DMA_ACTIVE	 3	/* A frame is incoming */
>  #define CF_CONFIG_NEEDED 4	/* Must configure hardware */
> +#define CF_SINGLE_BUFFER 5	/* Running with a single buffer */
>  
>  #define sensor_call(cam, o, f, args...) \
>  	v4l2_subdev_call(cam->sensor, o, f, ##args)
> @@ -197,10 +213,9 @@ static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
>   */
>  
>  /*
> - * Do everything we think we need to have the interface operating
> - * according to the desired format.
> + * Set up DMA buffers when operating in vmalloc mode
>   */
> -static void mcam_ctlr_dma(struct mcam_camera *cam)
> +static void mcam_ctlr_dma_vmalloc(struct mcam_camera *cam)
>  {
>  	/*
>  	 * Store the first two Y buffers (we aren't supporting
> @@ -219,6 +234,57 @@ static void mcam_ctlr_dma(struct mcam_camera *cam)
>  		mcam_reg_write(cam, REG_UBAR, 0); /* 32 bits only */
>  }
>  
> +/*
> + * Set up a contiguous buffer for the given frame.  Here also is where
> + * the underrun strategy is set: if there is no buffer available, reuse
> + * the buffer from the other BAR and set the CF_SINGLE_BUFFER flag to
> + * keep the interrupt handler from giving that buffer back to user
> + * space.  In this way, we always have a buffer to DMA to and don't
> + * have to try to play games stopping and restarting the controller.
> + */
> +static void mcam_set_contig_buffer(struct mcam_camera *cam, int frame)
> +{
> +	struct mcam_vb_buffer *buf;
> +	/*
> +	 * If there are no available buffers, go into single mode
> +	 */
> +	if (list_empty(&cam->buffers)) {
> +		buf = cam->vb_bufs[frame ^ 0x1];
> +		cam->vb_bufs[frame] = buf;
> +		mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
> +				vb2_dma_contig_plane_paddr(&buf->vb_buf, 0));
> +		set_bit(CF_SINGLE_BUFFER, &cam->flags);
> +		singles++;
> +		return;
> +	}
> +	/*
> +	 * OK, we have a buffer we can use.
> +	 */
> +	buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer, queue);
> +	list_del_init(&buf->queue);
> +	mcam_reg_write(cam, frame == 0 ? REG_Y0BAR : REG_Y1BAR,
> +			vb2_dma_contig_plane_paddr(&buf->vb_buf, 0));
> +	cam->vb_bufs[frame] = buf;
> +	clear_bit(CF_SINGLE_BUFFER, &cam->flags);
> +}
> +
> +static void mcam_ctlr_dma_contig(struct mcam_camera *cam)
> +{
> +	mcam_reg_set_bit(cam, REG_CTRL1, C1_TWOBUFS);
> +	cam->nbufs = 2;
> +	mcam_set_contig_buffer(cam, 0);
> +	mcam_set_contig_buffer(cam, 1);
> +}
> +
> +
> +static void mcam_ctlr_dma(struct mcam_camera *cam)
> +{
> +	if (cam->buffer_mode == B_DMA_contig)
> +		mcam_ctlr_dma_contig(cam);
> +	else
> +		mcam_ctlr_dma_vmalloc(cam);
> +}
> +
>  static void mcam_ctlr_image(struct mcam_camera *cam)
>  {
>  	int imgsz;
> @@ -542,7 +608,7 @@ static void mcam_free_dma_bufs(struct mcam_camera *cam)
>  /*
>   * Get everything ready, and start grabbing frames.
>   */
> -static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
> +static int mcam_read_setup(struct mcam_camera *cam)
>  {
>  	int ret;
>  	unsigned long flags;
> @@ -551,9 +617,9 @@ static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
>  	 * Configuration.  If we still don't have DMA buffers,
>  	 * make one last, desperate attempt.
>  	 */
> -	if (cam->nbufs == 0)
> -		if (mcam_alloc_dma_bufs(cam, 0))
> -			return -ENOMEM;
> +	if (cam->buffer_mode == B_vmalloc && cam->nbufs == 0 &&
> +			mcam_alloc_dma_bufs(cam, 0))
> +		return -ENOMEM;
>  
>  	if (mcam_needs_config(cam)) {
>  		mcam_cam_configure(cam);
> @@ -568,7 +634,7 @@ static int mcam_read_setup(struct mcam_camera *cam, enum mcam_state state)
>  	spin_lock_irqsave(&cam->dev_lock, flags);
>  	mcam_reset_buffers(cam);
>  	mcam_ctlr_irq_enable(cam);
> -	cam->state = state;
> +	cam->state = S_STREAMING;
>  	mcam_ctlr_start(cam);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
>  	return 0;
> @@ -587,8 +653,10 @@ static int mcam_vb_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
>  
>  	sizes[0] = cam->pix_format.sizeimage;
>  	*num_planes = 1; /* Someday we have to support planar formats... */
> -	if (*nbufs < 2 || *nbufs > 32)
> -		*nbufs = 6;  /* semi-arbitrary numbers */
> +	if (*nbufs < 3 || *nbufs > 32)
> +		*nbufs = 3;  /* semi-arbitrary numbers */
> +	if (cam->buffer_mode == B_DMA_contig)
> +		alloc_ctxs[0] = cam->vb_alloc_ctx;
>  	return 0;
>  }
>  
> @@ -605,10 +673,14 @@ static void mcam_vb_buf_queue(struct vb2_buffer *vb)
>  	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
>  	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
>  	unsigned long flags;
> +	int start;
>  
>  	spin_lock_irqsave(&cam->dev_lock, flags);
> -	list_add(&cam->buffers, &mvb->queue);
> +	start = (cam->state == S_BUFWAIT) && !list_empty(&cam->buffers);
> +	list_add(&mvb->queue, &cam->buffers);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
> +	if (start)
> +		mcam_read_setup(cam);
>  }
>  
>  /*
> @@ -636,13 +708,22 @@ static void mcam_vb_wait_finish(struct vb2_queue *vq)
>  static int mcam_vb_start_streaming(struct vb2_queue *vq)
>  {
>  	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> -	int ret = -EINVAL;
>  
> -	if (cam->state == S_IDLE) {
> -		cam->sequence = 0;
> -		ret = mcam_read_setup(cam, S_STREAMING);
> +	if (cam->state != S_IDLE)
> +		return -EINVAL;
> +	cam->sequence = 0;
> +	/*
> +	 * Videobuf2 sneakily hoards all the buffers and won't
> +	 * give them to us until *after* streaming starts.  But
> +	 * we can't actually start streaming until we have a
> +	 * destination.  So go into a wait state and hope they
> +	 * give us buffers soon.
> +	 */
> +	if (cam->buffer_mode != B_vmalloc && list_empty(&cam->buffers)) {
> +		cam->state = S_BUFWAIT;
> +		return 0;
>  	}
> -	return ret;
> +	return mcam_read_setup(cam);
>  }
>  
>  static int mcam_vb_stop_streaming(struct vb2_queue *vq)
> @@ -650,6 +731,11 @@ static int mcam_vb_stop_streaming(struct vb2_queue *vq)
>  	struct mcam_camera *cam = vb2_get_drv_priv(vq);
>  	unsigned long flags;
>  
> +	if (cam->state == S_BUFWAIT) {
> +		/* They never gave us buffers */
> +		cam->state = S_IDLE;
> +		return 0;
> +	}
>  	if (cam->state != S_STREAMING)
>  		return -EINVAL;
>  	mcam_ctlr_stop_dma(cam);
> @@ -683,7 +769,11 @@ static int mcam_setup_vb2(struct mcam_camera *cam)
>  	vq->io_modes = VB2_MMAP;  /* Add userptr */
>  	vq->drv_priv = cam;
>  	vq->ops = &mcam_vb2_ops;
> -	vq->mem_ops = &vb2_vmalloc_memops;
> +	if (cam->buffer_mode == B_DMA_contig) {
> +		vq->mem_ops = &vb2_dma_contig_memops;
> +		cam->vb_alloc_ctx = vb2_dma_contig_init_ctx(cam->dev);
> +	} else
> +		vq->mem_ops = &vb2_vmalloc_memops;
>  	vq->buf_struct_size = sizeof(struct mcam_vb_buffer);
>  
>  	return vb2_queue_init(vq);
> @@ -692,6 +782,8 @@ static int mcam_setup_vb2(struct mcam_camera *cam)
>  static void mcam_cleanup_vb2(struct mcam_camera *cam)
>  {
>  	vb2_queue_release(&cam->vb_queue);
> +	if (cam->buffer_mode == B_DMA_contig)
> +		vb2_dma_contig_cleanup_ctx(cam->vb_alloc_ctx);
>  }
>  
>  static ssize_t mcam_v4l_read(struct file *filp,
> @@ -809,6 +901,7 @@ static int mcam_v4l_open(struct file *filp)
>  
>  	filp->private_data = cam;
>  
> +	frames = singles = delivered = 0;
>  	mutex_lock(&cam->s_mutex);
>  	if (cam->users == 0) {
>  		ret = mcam_setup_vb2(cam);
> @@ -829,6 +922,8 @@ static int mcam_v4l_release(struct file *filp)
>  {
>  	struct mcam_camera *cam = filp->private_data;
>  
> +	cam_err(cam, "Release, %d frames, %d singles, %d delivered\n", frames,
> +			singles, delivered);
>  	mutex_lock(&cam->s_mutex);
>  	(cam->users)--;
>  	if (filp == cam->owner) {
> @@ -838,7 +933,7 @@ static int mcam_v4l_release(struct file *filp)
>  	if (cam->users == 0) {
>  		mcam_cleanup_vb2(cam);
>  		mcam_ctlr_power_down(cam);
> -		if (alloc_bufs_at_read)
> +		if (cam->buffer_mode == B_vmalloc && alloc_bufs_at_read)
>  			mcam_free_dma_bufs(cam);
>  	}
>  	mutex_unlock(&cam->s_mutex);
> @@ -993,18 +1088,17 @@ static int mcam_vidioc_s_fmt_vid_cap(struct file *filp, void *priv,
>  	 * Make sure we have appropriate DMA buffers.
>  	 */
>  	ret = -ENOMEM;
> -	if (cam->nbufs > 0 && cam->dma_buf_size < cam->pix_format.sizeimage)
> -		mcam_free_dma_bufs(cam);
> -	if (cam->nbufs == 0) {
> -		if (mcam_alloc_dma_bufs(cam, 0))
> -			goto out;
> +	if (cam->buffer_mode == B_vmalloc) {
> +		if (cam->nbufs > 0 &&
> +				cam->dma_buf_size < cam->pix_format.sizeimage)
> +			mcam_free_dma_bufs(cam);
> +		if (cam->nbufs == 0) {
> +			if (mcam_alloc_dma_bufs(cam, 0))
> +				goto out;
> +		}
>  	}
> -	/*
> -	 * It looks like this might work, so let's program the sensor.
> -	 */
> -	ret = mcam_cam_configure(cam);
> -	if (!ret)
> -		ret = mcam_ctlr_configure(cam);
> +	mcam_set_config_needed(cam, 1);
> +	ret = 0;
>  out:
>  	mutex_unlock(&cam->s_mutex);
>  	return ret;
> @@ -1214,7 +1308,20 @@ static struct video_device mcam_v4l_template = {
>   */
>  
>  
> +static void mcam_buffer_done(struct mcam_camera *cam, int frame,
> +		struct vb2_buffer *vbuf)
> +{
> +	vbuf->v4l2_buf.bytesused = cam->pix_format.sizeimage;
> +	vbuf->v4l2_buf.sequence = cam->buf_seq[frame];
> +	vbuf->v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> +	vbuf->v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
> +	vb2_set_plane_payload(vbuf, 0, cam->pix_format.sizeimage);
> +	vb2_buffer_done(vbuf, VB2_BUF_STATE_DONE);
> +}
>  
> +/*
> + * Copy data out to user space in the vmalloc case
> + */
>  static void mcam_frame_tasklet(unsigned long data)
>  {
>  	struct mcam_camera *cam = (struct mcam_camera *) data;
> @@ -1232,8 +1339,11 @@ static void mcam_frame_tasklet(unsigned long data)
>  			cam->next_buf = 0;
>  		if (!test_bit(bufno, &cam->flags))
>  			continue;
> -		if (list_empty(&cam->buffers))
> +		if (list_empty(&cam->buffers)) {
> +			singles++;
>  			break;  /* Leave it valid, hope for better later */
> +		}
> +		delivered++;
>  		clear_bit(bufno, &cam->flags);
>  		buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
>  				queue);
> @@ -1244,18 +1354,25 @@ static void mcam_frame_tasklet(unsigned long data)
>  		spin_unlock_irqrestore(&cam->dev_lock, flags);
>  		memcpy(vb2_plane_vaddr(&buf->vb_buf, 0), cam->dma_bufs[bufno],
>  				cam->pix_format.sizeimage);
> -		buf->vb_buf.v4l2_buf.bytesused = cam->pix_format.sizeimage;
> -		buf->vb_buf.v4l2_buf.sequence = cam->buf_seq[bufno];
> -		buf->vb_buf.v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> -		buf->vb_buf.v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;
> -		vb2_set_plane_payload(&buf->vb_buf, 0,
> -				cam->pix_format.sizeimage);
> -		vb2_buffer_done(&buf->vb_buf, VB2_BUF_STATE_DONE);
> +		mcam_buffer_done(cam, bufno, &buf->vb_buf);
>  		spin_lock_irqsave(&cam->dev_lock, flags);
>  	}
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
>  }
>  
> +/*
> + * For direct DMA, mark the buffer ready and set up another one.
> + */
> +static void mcam_dma_complete(struct mcam_camera *cam, int frame)
> +{
> +	struct mcam_vb_buffer *buf = cam->vb_bufs[frame];
> +
> +	if (!test_bit(CF_SINGLE_BUFFER, &cam->flags)) {
> +		delivered++;
> +		mcam_buffer_done(cam, frame, &buf->vb_buf);
> +	}
> +	mcam_set_contig_buffer(cam, frame);
> +}
>  
>  
>  static void mcam_frame_complete(struct mcam_camera *cam, int frame)
> @@ -1265,21 +1382,20 @@ static void mcam_frame_complete(struct mcam_camera *cam, int frame)
>  	 */
>  	set_bit(frame, &cam->flags);
>  	clear_bit(CF_DMA_ACTIVE, &cam->flags);
> -	if (cam->next_buf < 0)
> -		cam->next_buf = frame;
> +	cam->next_buf = frame;
>  	cam->buf_seq[frame] = ++(cam->sequence);
> +	cam->last_delivered = frame;
>  
> +	frames++;
>  	switch (cam->state) {
>  	/*
> -	 * For the streaming case, we defer the real work to the
> -	 * camera tasklet.
> -	 *
> -	 * FIXME: if the application is not consuming the buffers,
> -	 * we should eventually put things on hold and restart in
> -	 * vidioc_dqbuf().
> +	 * We're streaming and have a ready frame, hand it back
>  	 */
>  	case S_STREAMING:
> -		tasklet_schedule(&cam->s_tasklet);
> +		if (cam->buffer_mode == B_vmalloc)
> +			tasklet_schedule(&cam->s_tasklet);
> +		else
> +			mcam_dma_complete(cam, frame);
>  		break;
>  
>  	default:
> @@ -1356,7 +1472,18 @@ int mccic_register(struct mcam_camera *cam)
>  	INIT_LIST_HEAD(&cam->dev_list);
>  	INIT_LIST_HEAD(&cam->buffers);
>  	tasklet_init(&cam->s_tasklet, mcam_frame_tasklet, (unsigned long) cam);
> -
> +	/*
> +	 * User space may want to override the asked-for buffer mode;
> +	 * here's hoping they know what they're doing.
> +	 */
> +	if (buffer_mode == 0)
> +		cam->buffer_mode = B_vmalloc;
> +	else if (buffer_mode == 1)
> +		cam->buffer_mode = B_DMA_contig;
> +	else if (buffer_mode != -1)
> +		printk(KERN_ERR "marvel-cam: "
> +				"Strange module buffer mode %d - ignoring\n",
> +				buffer_mode);
>  	mcam_ctlr_init(cam);
>  
>  	/*
> @@ -1390,7 +1517,7 @@ int mccic_register(struct mcam_camera *cam)
>  	/*
>  	 * If so requested, try to get our DMA buffers now.
>  	 */
> -	if (!alloc_bufs_at_read) {
> +	if (cam->buffer_mode == B_vmalloc && !alloc_bufs_at_read) {
>  		if (mcam_alloc_dma_bufs(cam, 1))
>  			cam_warn(cam, "Unable to alloc DMA buffers at load"
>  					" will try again later.");
> @@ -1418,7 +1545,8 @@ void mccic_shutdown(struct mcam_camera *cam)
>  		mcam_ctlr_power_down(cam);
>  	}
>  	vb2_queue_release(&cam->vb_queue);
> -	mcam_free_dma_bufs(cam);
> +	if (cam->buffer_mode == B_vmalloc)
> +		mcam_free_dma_bufs(cam);
>  	video_unregister_device(&cam->vdev);
>  	v4l2_device_unregister(&cam->v4l2_dev);
>  }
> @@ -1452,7 +1580,7 @@ int mccic_resume(struct mcam_camera *cam)
>  
>  	set_bit(CF_CONFIG_NEEDED, &cam->flags);
>  	if (cam->state == S_STREAMING)
> -		ret = mcam_read_setup(cam, cam->state);
> +		ret = mcam_read_setup(cam);
>  	return ret;
>  }
>  #endif /* CONFIG_PM */
> diff --git a/drivers/media/video/marvell-ccic/mcam-core.h b/drivers/media/video/marvell-ccic/mcam-core.h
> index f40450c..2e667a0 100644
> --- a/drivers/media/video/marvell-ccic/mcam-core.h
> +++ b/drivers/media/video/marvell-ccic/mcam-core.h
> @@ -27,11 +27,21 @@ enum mcam_state {
>  	S_NOTREADY,	/* Not yet initialized */
>  	S_IDLE,		/* Just hanging around */
>  	S_FLAKED,	/* Some sort of problem */
> -	S_STREAMING	/* Streaming data */
> +	S_STREAMING,	/* Streaming data */
> +	S_BUFWAIT	/* streaming requested but no buffers yet */
>  };
>  #define MAX_DMA_BUFS 3
>  
>  /*
> + * Different platforms work best with different buffer modes, so we
> + * let the platform pick.
> + */
> +enum mcam_buffer_mode {
> +	B_vmalloc = 0,
> +	B_DMA_contig
> +};
> +
> +/*
>   * A description of one of our devices.
>   * Locking: controlled by s_mutex.  Certain fields, however, require
>   *          the dev_lock spinlock; they are marked as such by comments.
> @@ -49,7 +59,7 @@ struct mcam_camera {
>  	unsigned int chip_id;
>  	short int clock_speed;	/* Sensor clock speed, default 30 */
>  	short int use_smbus;	/* SMBUS or straight I2c? */
> -
> +	enum mcam_buffer_mode buffer_mode;
>  	/*
>  	 * Callbacks from the core to the platform code.
>  	 */
> @@ -79,7 +89,7 @@ struct mcam_camera {
>  	struct vb2_queue vb_queue;
>  	struct list_head buffers;	/* Available frames */
>  
> -	/* DMA buffers */
> +	/* DMA buffers - vmalloc mode */
>  	unsigned int nbufs;		/* How many are alloc'd */
>  	int next_buf;			/* Next to consume (dev_lock) */
>  	unsigned int dma_buf_size;	/* allocated size */
> @@ -88,6 +98,11 @@ struct mcam_camera {
>  	unsigned int sequence;		/* Frame sequence number */
>  	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual bufs */
>  
> +	/* DMA buffers - contiguous DMA mode */
> +	struct mcam_vb_buffer *vb_bufs[MAX_DMA_BUFS];
> +	struct vb2_alloc_ctx *vb_alloc_ctx;
> +	unsigned short last_delivered;
> +
>  	struct tasklet_struct s_tasklet;
>  
>  	/* Current operating parameters */
> diff --git a/drivers/media/video/marvell-ccic/mmp-driver.c b/drivers/media/video/marvell-ccic/mmp-driver.c
> index ac9976f..7b9c48c 100644
> --- a/drivers/media/video/marvell-ccic/mmp-driver.c
> +++ b/drivers/media/video/marvell-ccic/mmp-driver.c
> @@ -180,6 +180,7 @@ static int mmpcam_probe(struct platform_device *pdev)
>  	mcam->dev = &pdev->dev;
>  	mcam->use_smbus = 0;
>  	mcam->chip_id = V4L2_IDENT_ARMADA610;
> +	mcam->buffer_mode = B_vmalloc;  /* Switch to dma */
>  	spin_lock_init(&mcam->dev_lock);
>  	/*
>  	 * Get our I/O memory.


^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [PATCH 1/5] marvell-cam: convert to videobuf2
  2011-06-20 19:14 ` [PATCH 1/5] marvell-cam: convert to videobuf2 Jonathan Corbet
@ 2011-06-22 13:59   ` Marek Szyprowski
  2011-06-22 14:11     ` Jonathan Corbet
  0 siblings, 1 reply; 10+ messages in thread
From: Marek Szyprowski @ 2011-06-22 13:59 UTC (permalink / raw)
  To: 'Jonathan Corbet', linux-media
  Cc: g.liakhovetski, 'Kassey Lee', 'Pawel Osciak'

Hello Jonathan,

On Monday, June 20, 2011 9:15 PM Jonathan Corbet wrote:

> This is a basic, naive conversion to the videobuf2 infrastructure, removing
> a lot of code in the process.  For now, we're using vmalloc, which is
> suboptimal, but it does match what the cafe driver did before.

Could you elaborate a bit why vmalloc is suboptimal for your case?

> In the cafe
> case, it may have to stay that way just because memory is too tight to do
> direct streaming; mmp-camera will be able to do better.
> 
> Signed-off-by: Jonathan Corbet <corbet@lwn.net>
> ---
>  drivers/media/video/marvell-ccic/Kconfig     |    2 +
>  drivers/media/video/marvell-ccic/mcam-core.c |  579 ++++++++--------------
> ----
>  drivers/media/video/marvell-ccic/mcam-core.h |   26 +-
>  3 files changed, 196 insertions(+), 411 deletions(-)
> 
> diff --git a/drivers/media/video/marvell-ccic/Kconfig
> b/drivers/media/video/marvell-ccic/Kconfig
> index b4f7260..eb535b1 100644
> --- a/drivers/media/video/marvell-ccic/Kconfig
> +++ b/drivers/media/video/marvell-ccic/Kconfig
> @@ -2,6 +2,7 @@ config VIDEO_CAFE_CCIC
>  	tristate "Marvell 88ALP01 (Cafe) CMOS Camera Controller support"
>  	depends on PCI && I2C && VIDEO_V4L2
>  	select VIDEO_OV7670
> +	select VIDEOBUF2_VMALLOC
>  	---help---
>  	  This is a video4linux2 driver for the Marvell 88ALP01 integrated
>  	  CMOS camera controller.  This is the controller found on first-
> @@ -12,6 +13,7 @@ config VIDEO_MMP_CAMERA
>  	depends on ARCH_MMP && I2C && VIDEO_V4L2
>  	select VIDEO_OV7670
>  	select I2C_GPIO
> +	select VIDEOBUF2_VMALLOC
>  	---help---
>  	  This is a Video4Linux2 driver for the integrated camera
>  	  controller found on Marvell Armada 610 application
> diff --git a/drivers/media/video/marvell-ccic/mcam-core.c
> b/drivers/media/video/marvell-ccic/mcam-core.c
> index 3e6a5e8..055d843 100644
> --- a/drivers/media/video/marvell-ccic/mcam-core.c
> +++ b/drivers/media/video/marvell-ccic/mcam-core.c
> @@ -17,6 +17,7 @@
>  #include <media/v4l2-ioctl.h>
>  #include <media/v4l2-chip-ident.h>
>  #include <media/ov7670.h>
> +#include <media/videobuf2-vmalloc.h>
>  #include <linux/device.h>
>  #include <linux/wait.h>
>  #include <linux/list.h>
> @@ -149,7 +150,6 @@ static void mcam_reset_buffers(struct mcam_camera *cam)
>  	cam->next_buf = -1;
>  	for (i = 0; i < cam->nbufs; i++)
>  		clear_bit(i, &cam->flags);
> -	cam->specframes = 0;
>  }
> 
>  static inline int mcam_needs_config(struct mcam_camera *cam)
> @@ -165,6 +165,21 @@ static void mcam_set_config_needed(struct mcam_camera
> *cam, int needed)
>  		clear_bit(CF_CONFIG_NEEDED, &cam->flags);
>  }
> 
> +/*
> + * Our buffer type for working with videobuf2.  Note that the vb2
> + * developers have decreed that struct vb2_buffer must be at the
> + * beginning of this structure.
> + */
> +struct mcam_vb_buffer {
> +	struct vb2_buffer vb_buf;
> +	struct list_head queue;
> +};
> +
> +static inline struct mcam_vb_buffer *vb_to_mvb(struct vb2_buffer *vb)
> +{
> +	return container_of(vb, struct mcam_vb_buffer, vb_buf);
> +}
> +
> 
>  /*
>   * Debugging and related.
> @@ -339,9 +354,7 @@ static void mcam_ctlr_stop_dma(struct mcam_camera *cam)
>  	spin_lock_irqsave(&cam->dev_lock, flags);
>  	mcam_ctlr_stop(cam);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
> -	mdelay(1);
> -	wait_event_timeout(cam->iowait,
> -			!test_bit(CF_DMA_ACTIVE, &cam->flags), HZ);
> +	msleep(10);
>  	if (test_bit(CF_DMA_ACTIVE, &cam->flags))
>  		cam_err(cam, "Timeout waiting for DMA to end\n");
>  		/* This would be bad news - what now? */
> @@ -524,44 +537,11 @@ static void mcam_free_dma_bufs(struct mcam_camera
> *cam)
> 
> 
> 
> -
> -
>  /* -----------------------------------------------------------------------
> */
>  /*
>   * Here starts the V4L2 interface code.
>   */
> 
> -/*
> - * Read an image from the device.
> - */
> -static ssize_t mcam_deliver_buffer(struct mcam_camera *cam,
> -		char __user *buffer, size_t len, loff_t *pos)
> -{
> -	int bufno;
> -	unsigned long flags;
> -
> -	spin_lock_irqsave(&cam->dev_lock, flags);
> -	if (cam->next_buf < 0) {
> -		cam_err(cam, "deliver_buffer: No next buffer\n");
> -		spin_unlock_irqrestore(&cam->dev_lock, flags);
> -		return -EIO;
> -	}
> -	bufno = cam->next_buf;
> -	clear_bit(bufno, &cam->flags);
> -	if (++(cam->next_buf) >= cam->nbufs)
> -		cam->next_buf = 0;
> -	if (!test_bit(cam->next_buf, &cam->flags))
> -		cam->next_buf = -1;
> -	cam->specframes = 0;
> -	spin_unlock_irqrestore(&cam->dev_lock, flags);
> -
> -	if (len > cam->pix_format.sizeimage)
> -		len = cam->pix_format.sizeimage;
> -	if (copy_to_user(buffer, cam->dma_bufs[bufno], len))
> -		return -EFAULT;
> -	(*pos) += len;
> -	return len;
> -}
> 
>  /*
>   * Get everything ready, and start grabbing frames.
> @@ -598,75 +578,138 @@ static int mcam_read_setup(struct mcam_camera *cam,
> enum mcam_state state)
>  	return 0;
>  }
> 
> +/* -----------------------------------------------------------------------
> */
> +/*
> + * Videobuf2 interface code.
> + */
> 
> -static ssize_t mcam_v4l_read(struct file *filp,
> -		char __user *buffer, size_t len, loff_t *pos)
> +static int mcam_vb_queue_setup(struct vb2_queue *vq, unsigned int *nbufs,
> +		unsigned int *num_planes, unsigned long sizes[],
> +		void *alloc_ctxs[])
>  {
> -	struct mcam_camera *cam = filp->private_data;
> -	int ret = 0;
> +	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> +
> +	sizes[0] = cam->pix_format.sizeimage;
> +	*num_planes = 1; /* Someday we have to support planar formats... */
> +	if (*nbufs < 2 || *nbufs > 32)
> +		*nbufs = 6;  /* semi-arbitrary numbers */
> +	return 0;
> +}
> +
> +static int mcam_vb_buf_init(struct vb2_buffer *vb)
> +{
> +	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
> +
> +	INIT_LIST_HEAD(&mvb->queue);

This operation is not needed. mvb->queue is used only by list_add() which
overwrites the values set here.

> +	return 0;
> +}

Taking the above, the whole mcam_bv_buf_init() callback is not needed.

> +
> +static void mcam_vb_buf_queue(struct vb2_buffer *vb)
> +{
> +	struct mcam_vb_buffer *mvb = vb_to_mvb(vb);
> +	struct mcam_camera *cam = vb2_get_drv_priv(vb->vb2_queue);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&cam->dev_lock, flags);
> +	list_add(&cam->buffers, &mvb->queue);
> +	spin_unlock_irqrestore(&cam->dev_lock, flags);
> +}
> +
> +/*
> + * vb2 uses these to release the mutex when waiting in dqbuf.  I'm
> + * not actually sure we need to do this (I'm not sure that vb2_dqbuf()
> needs
> + * to be called with the mutex held), but better safe than sorry.
> + */
> +static void mcam_vb_wait_prepare(struct vb2_queue *vq)
> +{
> +	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> +
> +	mutex_unlock(&cam->s_mutex);
> +}
> +
> +static void mcam_vb_wait_finish(struct vb2_queue *vq)
> +{
> +	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> 
> -	/*
> -	 * Perhaps we're in speculative read mode and already
> -	 * have data?
> -	 */
>  	mutex_lock(&cam->s_mutex);
> -	if (cam->state == S_SPECREAD) {
> -		if (cam->next_buf >= 0) {
> -			ret = mcam_deliver_buffer(cam, buffer, len, pos);
> -			if (ret != 0)
> -				goto out_unlock;
> -		}
> -	} else if (cam->state == S_FLAKED || cam->state == S_NOTREADY) {
> -		ret = -EIO;
> -		goto out_unlock;
> -	} else if (cam->state != S_IDLE) {
> -		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> +}
> 
> -	/*
> -	 * v4l2: multiple processes can open the device, but only
> -	 * one gets to grab data from it.
> -	 */
> -	if (cam->owner && cam->owner != filp) {
> -		ret = -EBUSY;
> -		goto out_unlock;
> -	}
> -	cam->owner = filp;
> +/*
> + * These need to be called with the mutex held from vb2
> + */
> +static int mcam_vb_start_streaming(struct vb2_queue *vq)
> +{
> +	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> +	int ret = -EINVAL;
> 
> -	/*
> -	 * Do setup if need be.
> -	 */
> -	if (cam->state != S_SPECREAD) {
> -		ret = mcam_read_setup(cam, S_SINGLEREAD);
> -		if (ret)
> -			goto out_unlock;
> -	}
> -	/*
> -	 * Wait for something to happen.  This should probably
> -	 * be interruptible (FIXME).
> -	 */
> -	wait_event_timeout(cam->iowait, cam->next_buf >= 0, HZ);
> -	if (cam->next_buf < 0) {
> -		cam_err(cam, "read() operation timed out\n");
> -		mcam_ctlr_stop_dma(cam);
> -		ret = -EIO;
> -		goto out_unlock;
> +	if (cam->state == S_IDLE) {
> +		cam->sequence = 0;
> +		ret = mcam_read_setup(cam, S_STREAMING);
>  	}
> +	return ret;
> +}
> +
> +static int mcam_vb_stop_streaming(struct vb2_queue *vq)
> +{
> +	struct mcam_camera *cam = vb2_get_drv_priv(vq);
> +	unsigned long flags;
> +
> +	if (cam->state != S_STREAMING)
> +		return -EINVAL;
> +	mcam_ctlr_stop_dma(cam);
>  	/*
> -	 * Give them their data and we should be done.
> +	 * VB2 reclaims the buffers, so we need to forget
> +	 * about them.
>  	 */
> -	ret = mcam_deliver_buffer(cam, buffer, len, pos);
> -
> -out_unlock:
> -	mutex_unlock(&cam->s_mutex);
> -	return ret;
> +	spin_lock_irqsave(&cam->dev_lock, flags);
> +	INIT_LIST_HEAD(&cam->buffers);
> +	spin_unlock_irqrestore(&cam->dev_lock, flags);
> +	return 0;
>  }
> 
> 
> +static const struct vb2_ops mcam_vb2_ops = {
> +	.queue_setup		= mcam_vb_queue_setup,
> +	.buf_init		= mcam_vb_buf_init,
> +	.buf_queue		= mcam_vb_buf_queue,
> +	.start_streaming	= mcam_vb_start_streaming,
> +	.stop_streaming		= mcam_vb_stop_streaming,
> +	.wait_prepare		= mcam_vb_wait_prepare,
> +	.wait_finish		= mcam_vb_wait_finish,
> +};
> 
> +static int mcam_setup_vb2(struct mcam_camera *cam)
> +{
> +	struct vb2_queue *vq = &cam->vb_queue;
> 
> +	memset(vq, 0, sizeof(*vq));
> +	vq->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> +	vq->io_modes = VB2_MMAP;  /* Add userptr */
> +	vq->drv_priv = cam;
> +	vq->ops = &mcam_vb2_ops;
> +	vq->mem_ops = &vb2_vmalloc_memops;
> +	vq->buf_struct_size = sizeof(struct mcam_vb_buffer);
> 
> +	return vb2_queue_init(vq);
> +}
> +
> +static void mcam_cleanup_vb2(struct mcam_camera *cam)
> +{
> +	vb2_queue_release(&cam->vb_queue);
> +}
> +
> +static ssize_t mcam_v4l_read(struct file *filp,
> +		char __user *buffer, size_t len, loff_t *pos)
> +{
> +	struct mcam_camera *cam = filp->private_data;
> +	int ret;
> +
> +	mutex_lock(&cam->s_mutex);
> +	ret = vb2_read(&cam->vb_queue, buffer, len, pos,
> +			filp->f_flags & O_NONBLOCK);
> +	mutex_unlock(&cam->s_mutex);
> +	return ret;
> +}
> 
> 
> 
> @@ -674,26 +717,15 @@ out_unlock:
>   * Streaming I/O support.
>   */
> 
> -
> -
>  static int mcam_vidioc_streamon(struct file *filp, void *priv,
>  		enum v4l2_buf_type type)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	int ret = -EINVAL;
> +	int ret;
> 
> -	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> -		goto out;
>  	mutex_lock(&cam->s_mutex);
> -	if (cam->state != S_IDLE || cam->n_sbufs == 0)
> -		goto out_unlock;
> -
> -	cam->sequence = 0;
> -	ret = mcam_read_setup(cam, S_STREAMING);
> -
> -out_unlock:
> +	ret = vb2_streamon(&cam->vb_queue, type);
>  	mutex_unlock(&cam->s_mutex);
> -out:
>  	return ret;
>  }
> 
> @@ -702,137 +734,23 @@ static int mcam_vidioc_streamoff(struct file *filp,
> void *priv,
>  		enum v4l2_buf_type type)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	int ret = -EINVAL;
> +	int ret;
> 
> -	if (type != V4L2_BUF_TYPE_VIDEO_CAPTURE)
> -		goto out;
>  	mutex_lock(&cam->s_mutex);
> -	if (cam->state != S_STREAMING)
> -		goto out_unlock;
> -
> -	mcam_ctlr_stop_dma(cam);
> -	ret = 0;
> -
> -out_unlock:
> +	ret = vb2_streamoff(&cam->vb_queue, type);
>  	mutex_unlock(&cam->s_mutex);
> -out:
>  	return ret;
>  }
> 
> 
> -
> -static int mcam_setup_siobuf(struct mcam_camera *cam, int index)
> -{
> -	struct mcam_sio_buffer *buf = cam->sb_bufs + index;
> -
> -	INIT_LIST_HEAD(&buf->list);
> -	buf->v4lbuf.length = PAGE_ALIGN(cam->pix_format.sizeimage);
> -	buf->buffer = vmalloc_user(buf->v4lbuf.length);
> -	if (buf->buffer == NULL)
> -		return -ENOMEM;
> -	buf->mapcount = 0;
> -	buf->cam = cam;
> -
> -	buf->v4lbuf.index = index;
> -	buf->v4lbuf.type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
> -	buf->v4lbuf.field = V4L2_FIELD_NONE;
> -	buf->v4lbuf.memory = V4L2_MEMORY_MMAP;
> -	/*
> -	 * Offset: must be 32-bit even on a 64-bit system.  videobuf-dma-sg
> -	 * just uses the length times the index, but the spec warns
> -	 * against doing just that - vma merging problems.  So we
> -	 * leave a gap between each pair of buffers.
> -	 */
> -	buf->v4lbuf.m.offset = 2*index*buf->v4lbuf.length;
> -	return 0;
> -}
> -
> -static int mcam_free_sio_buffers(struct mcam_camera *cam)
> -{
> -	int i;
> -
> -	/*
> -	 * If any buffers are mapped, we cannot free them at all.
> -	 */
> -	for (i = 0; i < cam->n_sbufs; i++)
> -		if (cam->sb_bufs[i].mapcount > 0)
> -			return -EBUSY;
> -	/*
> -	 * OK, let's do it.
> -	 */
> -	for (i = 0; i < cam->n_sbufs; i++)
> -		vfree(cam->sb_bufs[i].buffer);
> -	cam->n_sbufs = 0;
> -	kfree(cam->sb_bufs);
> -	cam->sb_bufs = NULL;
> -	INIT_LIST_HEAD(&cam->sb_avail);
> -	INIT_LIST_HEAD(&cam->sb_full);
> -	return 0;
> -}
> -
> -
> -
>  static int mcam_vidioc_reqbufs(struct file *filp, void *priv,
>  		struct v4l2_requestbuffers *req)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	int ret = 0;  /* Silence warning */
> +	int ret;
> 
> -	/*
> -	 * Make sure it's something we can do.  User pointers could be
> -	 * implemented without great pain, but that's not been done yet.
> -	 */
> -	if (req->memory != V4L2_MEMORY_MMAP)
> -		return -EINVAL;
> -	/*
> -	 * If they ask for zero buffers, they really want us to stop
> streaming
> -	 * (if it's happening) and free everything.  Should we check owner?
> -	 */
>  	mutex_lock(&cam->s_mutex);
> -	if (req->count == 0) {
> -		if (cam->state == S_STREAMING)
> -			mcam_ctlr_stop_dma(cam);
> -		ret = mcam_free_sio_buffers(cam);
> -		goto out;
> -	}
> -	/*
> -	 * Device needs to be idle and working.  We *could* try to do the
> -	 * right thing in S_SPECREAD by shutting things down, but it
> -	 * probably doesn't matter.
> -	 */
> -	if (cam->state != S_IDLE || (cam->owner && cam->owner != filp)) {
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -	cam->owner = filp;
> -
> -	if (req->count < min_buffers)
> -		req->count = min_buffers;
> -	else if (req->count > max_buffers)
> -		req->count = max_buffers;
> -	if (cam->n_sbufs > 0) {
> -		ret = mcam_free_sio_buffers(cam);
> -		if (ret)
> -			goto out;
> -	}
> -
> -	cam->sb_bufs = kzalloc(req->count*sizeof(struct mcam_sio_buffer),
> -			GFP_KERNEL);
> -	if (cam->sb_bufs == NULL) {
> -		ret = -ENOMEM;
> -		goto out;
> -	}
> -	for (cam->n_sbufs = 0; cam->n_sbufs < req->count; (cam->n_sbufs++)) {
> -		ret = mcam_setup_siobuf(cam, cam->n_sbufs);
> -		if (ret)
> -			break;
> -	}
> -
> -	if (cam->n_sbufs == 0)  /* no luck at all - ret already set */
> -		kfree(cam->sb_bufs);
> -	req->count = cam->n_sbufs;  /* In case of partial success */
> -
> -out:
> +	ret = vb2_reqbufs(&cam->vb_queue, req);
>  	mutex_unlock(&cam->s_mutex);
>  	return ret;
>  }
> @@ -842,14 +760,10 @@ static int mcam_vidioc_querybuf(struct file *filp,
> void *priv,
>  		struct v4l2_buffer *buf)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	int ret = -EINVAL;
> +	int ret;
> 
>  	mutex_lock(&cam->s_mutex);
> -	if (buf->index >= cam->n_sbufs)
> -		goto out;
> -	*buf = cam->sb_bufs[buf->index].v4lbuf;
> -	ret = 0;
> -out:
> +	ret = vb2_querybuf(&cam->vb_queue, buf);
>  	mutex_unlock(&cam->s_mutex);
>  	return ret;
>  }
> @@ -858,29 +772,10 @@ static int mcam_vidioc_qbuf(struct file *filp, void
> *priv,
>  		struct v4l2_buffer *buf)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	struct mcam_sio_buffer *sbuf;
> -	int ret = -EINVAL;
> -	unsigned long flags;
> +	int ret;
> 
>  	mutex_lock(&cam->s_mutex);
> -	if (buf->index >= cam->n_sbufs)
> -		goto out;
> -	sbuf = cam->sb_bufs + buf->index;
> -	if (sbuf->v4lbuf.flags & V4L2_BUF_FLAG_QUEUED) {
> -		ret = 0; /* Already queued?? */
> -		goto out;
> -	}
> -	if (sbuf->v4lbuf.flags & V4L2_BUF_FLAG_DONE) {
> -		/* Spec doesn't say anything, seems appropriate tho */
> -		ret = -EBUSY;
> -		goto out;
> -	}
> -	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_QUEUED;
> -	spin_lock_irqsave(&cam->dev_lock, flags);
> -	list_add(&sbuf->list, &cam->sb_avail);
> -	spin_unlock_irqrestore(&cam->dev_lock, flags);
> -	ret = 0;
> -out:
> +	ret = vb2_qbuf(&cam->vb_queue, buf);
>  	mutex_unlock(&cam->s_mutex);
>  	return ret;
>  }
> @@ -889,111 +784,22 @@ static int mcam_vidioc_dqbuf(struct file *filp, void
> *priv,
>  		struct v4l2_buffer *buf)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	struct mcam_sio_buffer *sbuf;
> -	int ret = -EINVAL;
> -	unsigned long flags;
> +	int ret;
> 
>  	mutex_lock(&cam->s_mutex);
> -	if (cam->state != S_STREAMING)
> -		goto out_unlock;
> -	if (list_empty(&cam->sb_full) && filp->f_flags & O_NONBLOCK) {
> -		ret = -EAGAIN;
> -		goto out_unlock;
> -	}
> -
> -	while (list_empty(&cam->sb_full) && cam->state == S_STREAMING) {
> -		mutex_unlock(&cam->s_mutex);
> -		if (wait_event_interruptible(cam->iowait,
> -						!list_empty(&cam->sb_full))) {
> -			ret = -ERESTARTSYS;
> -			goto out;
> -		}
> -		mutex_lock(&cam->s_mutex);
> -	}
> -
> -	if (cam->state != S_STREAMING)
> -		ret = -EINTR;
> -	else {
> -		spin_lock_irqsave(&cam->dev_lock, flags);
> -		/* Should probably recheck !list_empty() here */
> -		sbuf = list_entry(cam->sb_full.next,
> -				struct mcam_sio_buffer, list);
> -		list_del_init(&sbuf->list);
> -		spin_unlock_irqrestore(&cam->dev_lock, flags);
> -		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_DONE;
> -		*buf = sbuf->v4lbuf;
> -		ret = 0;
> -	}
> -
> -out_unlock:
> +	ret = vb2_dqbuf(&cam->vb_queue, buf, filp->f_flags & O_NONBLOCK);
>  	mutex_unlock(&cam->s_mutex);
> -out:
>  	return ret;
>  }
> 
> 
> -
> -static void mcam_v4l_vm_open(struct vm_area_struct *vma)
> -{
> -	struct mcam_sio_buffer *sbuf = vma->vm_private_data;
> -	/*
> -	 * Locking: done under mmap_sem, so we don't need to
> -	 * go back to the camera lock here.
> -	 */
> -	sbuf->mapcount++;
> -}
> -
> -
> -static void mcam_v4l_vm_close(struct vm_area_struct *vma)
> -{
> -	struct mcam_sio_buffer *sbuf = vma->vm_private_data;
> -
> -	mutex_lock(&sbuf->cam->s_mutex);
> -	sbuf->mapcount--;
> -	/* Docs say we should stop I/O too... */
> -	if (sbuf->mapcount == 0)
> -		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_MAPPED;
> -	mutex_unlock(&sbuf->cam->s_mutex);
> -}
> -
> -static const struct vm_operations_struct mcam_v4l_vm_ops = {
> -	.open = mcam_v4l_vm_open,
> -	.close = mcam_v4l_vm_close
> -};
> -
> -
>  static int mcam_v4l_mmap(struct file *filp, struct vm_area_struct *vma)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> -	unsigned long offset = vma->vm_pgoff << PAGE_SHIFT;
> -	int ret = -EINVAL;
> -	int i;
> -	struct mcam_sio_buffer *sbuf = NULL;
> +	int ret;
> 
> -	if (!(vma->vm_flags & VM_WRITE) || !(vma->vm_flags & VM_SHARED))
> -		return -EINVAL;
> -	/*
> -	 * Find the buffer they are looking for.
> -	 */
>  	mutex_lock(&cam->s_mutex);
> -	for (i = 0; i < cam->n_sbufs; i++)
> -		if (cam->sb_bufs[i].v4lbuf.m.offset == offset) {
> -			sbuf = cam->sb_bufs + i;
> -			break;
> -		}
> -	if (sbuf == NULL)
> -		goto out;
> -
> -	ret = remap_vmalloc_range(vma, sbuf->buffer, 0);
> -	if (ret)
> -		goto out;
> -	vma->vm_flags |= VM_DONTEXPAND;
> -	vma->vm_private_data = sbuf;
> -	vma->vm_ops = &mcam_v4l_vm_ops;
> -	sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_MAPPED;
> -	mcam_v4l_vm_open(vma);
> -	ret = 0;
> -out:
> +	ret = vb2_mmap(&cam->vb_queue, vma);
>  	mutex_unlock(&cam->s_mutex);
>  	return ret;
>  }
> @@ -1003,19 +809,23 @@ out:
>  static int mcam_v4l_open(struct file *filp)
>  {
>  	struct mcam_camera *cam = video_drvdata(filp);
> +	int ret = 0;
> 
>  	filp->private_data = cam;
> 
>  	mutex_lock(&cam->s_mutex);
>  	if (cam->users == 0) {
> +		ret = mcam_setup_vb2(cam);
> +		if (ret)
> +			goto out;
>  		mcam_ctlr_power_up(cam);
>  		__mcam_cam_reset(cam);
>  		mcam_set_config_needed(cam, 1);
> -	/* FIXME make sure this is complete */
>  	}
>  	(cam->users)++;
> +out:
>  	mutex_unlock(&cam->s_mutex);
> -	return 0;
> +	return ret;
>  }
> 
> 
> @@ -1027,10 +837,10 @@ static int mcam_v4l_release(struct file *filp)
>  	(cam->users)--;
>  	if (filp == cam->owner) {
>  		mcam_ctlr_stop_dma(cam);
> -		mcam_free_sio_buffers(cam);
>  		cam->owner = NULL;
>  	}
>  	if (cam->users == 0) {
> +		mcam_cleanup_vb2(cam);
>  		mcam_ctlr_power_down(cam);
>  		if (alloc_bufs_at_read)
>  			mcam_free_dma_bufs(cam);
> @@ -1045,11 +855,12 @@ static unsigned int mcam_v4l_poll(struct file *filp,
>  		struct poll_table_struct *pt)
>  {
>  	struct mcam_camera *cam = filp->private_data;
> +	int ret;
> 
> -	poll_wait(filp, &cam->iowait, pt);
> -	if (cam->next_buf >= 0)
> -		return POLLIN | POLLRDNORM;
> -	return 0;
> +	mutex_lock(&cam->s_mutex);
> +	ret = vb2_poll(&cam->vb_queue, filp, pt);
> +	mutex_unlock(&cam->s_mutex);
> +	return ret;
>  }
> 
> 
> @@ -1093,9 +904,6 @@ static int mcam_vidioc_s_ctrl(struct file *filp, void
> *priv,
>  }
> 
> 
> -
> -
> -
>  static int mcam_vidioc_querycap(struct file *file, void *priv,
>  		struct v4l2_capability *cap)
>  {
> @@ -1166,7 +974,7 @@ static int mcam_vidioc_s_fmt_vid_cap(struct file *filp,
> void *priv,
>  	 * Can't do anything if the device is not idle
>  	 * Also can't if there are streaming buffers in place.
>  	 */
> -	if (cam->state != S_IDLE || cam->n_sbufs > 0)
> +	if (cam->state != S_IDLE || cam->vb_queue.num_buffers > 0)
>  		return -EBUSY;
> 
>  	f = mcam_find_format(fmt->fmt.pix.pixelformat);
> @@ -1416,39 +1224,39 @@ static void mcam_frame_tasklet(unsigned long data)
>  	struct mcam_camera *cam = (struct mcam_camera *) data;
>  	int i;
>  	unsigned long flags;
> -	struct mcam_sio_buffer *sbuf;
> +	struct mcam_vb_buffer *buf;
> 
>  	spin_lock_irqsave(&cam->dev_lock, flags);
>  	for (i = 0; i < cam->nbufs; i++) {
>  		int bufno = cam->next_buf;
> -		if (bufno < 0) {  /* "will never happen" */
> -			cam_err(cam, "No valid bufs in tasklet!\n");
> -			break;
> -		}
> +
> +		if (cam->state != S_STREAMING || bufno < 0)
> +			break;  /* I/O got stopped */
>  		if (++(cam->next_buf) >= cam->nbufs)
>  			cam->next_buf = 0;
>  		if (!test_bit(bufno, &cam->flags))
>  			continue;
> -		if (list_empty(&cam->sb_avail))
> +		if (list_empty(&cam->buffers))
>  			break;  /* Leave it valid, hope for better later */
>  		clear_bit(bufno, &cam->flags);
> -		sbuf = list_entry(cam->sb_avail.next,
> -				struct mcam_sio_buffer, list);
> +		buf = list_first_entry(&cam->buffers, struct mcam_vb_buffer,
> +				queue);
> +		list_del_init(&buf->queue);
>  		/*
>  		 * Drop the lock during the big copy.  This *should* be safe...
>  		 */
>  		spin_unlock_irqrestore(&cam->dev_lock, flags);
> -		memcpy(sbuf->buffer, cam->dma_bufs[bufno],
> +		memcpy(vb2_plane_vaddr(&buf->vb_buf, 0), cam->dma_bufs[bufno],
>  				cam->pix_format.sizeimage);
> -		sbuf->v4lbuf.bytesused = cam->pix_format.sizeimage;
> -		sbuf->v4lbuf.sequence = cam->buf_seq[bufno];
> -		sbuf->v4lbuf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> -		sbuf->v4lbuf.flags |= V4L2_BUF_FLAG_DONE;
> +		buf->vb_buf.v4l2_buf.bytesused = cam->pix_format.sizeimage;
> +		buf->vb_buf.v4l2_buf.sequence = cam->buf_seq[bufno];

> +		buf->vb_buf.v4l2_buf.flags &= ~V4L2_BUF_FLAG_QUEUED;
> +		buf->vb_buf.v4l2_buf.flags |= V4L2_BUF_FLAG_DONE;

These 2 lines should be dropped. vb2 takes care about V4L2 status flags
internally.

> +		vb2_set_plane_payload(&buf->vb_buf, 0,
> +				cam->pix_format.sizeimage);
> +		vb2_buffer_done(&buf->vb_buf, VB2_BUF_STATE_DONE);
>  		spin_lock_irqsave(&cam->dev_lock, flags);
> -		list_move_tail(&sbuf->list, &cam->sb_full);
>  	}
> -	if (!list_empty(&cam->sb_full))
> -		wake_up(&cam->iowait);
>  	spin_unlock_irqrestore(&cam->dev_lock, flags);
>  }
> 
> @@ -1469,27 +1277,6 @@ static void mcam_frame_complete(struct mcam_camera
> *cam, int frame)
> 
>  	switch (cam->state) {
>  	/*
> -	 * If in single read mode, try going speculative.
> -	 */
> -	case S_SINGLEREAD:
> -		cam->state = S_SPECREAD;
> -		cam->specframes = 0;
> -		wake_up(&cam->iowait);
> -		break;
> -
> -	/*
> -	 * If we are already doing speculative reads, and nobody is
> -	 * reading them, just stop.
> -	 */
> -	case S_SPECREAD:
> -		if (++(cam->specframes) >= cam->nbufs) {
> -			mcam_ctlr_stop(cam);
> -			mcam_ctlr_irq_disable(cam);
> -			cam->state = S_IDLE;
> -		}
> -		wake_up(&cam->iowait);
> -		break;
> -	/*
>  	 * For the streaming case, we defer the real work to the
>  	 * camera tasklet.
>  	 *
> @@ -1570,12 +1357,10 @@ int mccic_register(struct mcam_camera *cam)
>  	mutex_init(&cam->s_mutex);
>  	cam->state = S_NOTREADY;
>  	mcam_set_config_needed(cam, 1);
> -	init_waitqueue_head(&cam->iowait);
>  	cam->pix_format = mcam_def_pix_format;
>  	cam->mbus_code = mcam_def_mbus_code;
>  	INIT_LIST_HEAD(&cam->dev_list);
> -	INIT_LIST_HEAD(&cam->sb_avail);
> -	INIT_LIST_HEAD(&cam->sb_full);
> +	INIT_LIST_HEAD(&cam->buffers);
>  	tasklet_init(&cam->s_tasklet, mcam_frame_tasklet, (unsigned long)
> cam);
> 
>  	mcam_ctlr_init(cam);
> @@ -1638,10 +1423,8 @@ void mccic_shutdown(struct mcam_camera *cam)
>  		cam_warn(cam, "Removing a device with users!\n");
>  		mcam_ctlr_power_down(cam);
>  	}
> +	vb2_queue_release(&cam->vb_queue);
>  	mcam_free_dma_bufs(cam);
> -	if (cam->n_sbufs > 0)
> -		/* What if they are still mapped?  Shouldn't be, but... */
> -		mcam_free_sio_buffers(cam);
>  	video_unregister_device(&cam->vdev);
>  	v4l2_device_unregister(&cam->v4l2_dev);
>  }
> @@ -1674,9 +1457,7 @@ int mccic_resume(struct mcam_camera *cam)
>  	mutex_unlock(&cam->s_mutex);
> 
>  	set_bit(CF_CONFIG_NEEDED, &cam->flags);
> -	if (cam->state == S_SPECREAD)
> -		cam->state = S_IDLE;  /* Don't bother restarting */
> -	else if (cam->state == S_SINGLEREAD || cam->state == S_STREAMING)
> +	if (cam->state == S_STREAMING)
>  		ret = mcam_read_setup(cam, cam->state);
>  	return ret;
>  }
> diff --git a/drivers/media/video/marvell-ccic/mcam-core.h
> b/drivers/media/video/marvell-ccic/mcam-core.h
> index 5effa82..f40450c 100644
> --- a/drivers/media/video/marvell-ccic/mcam-core.h
> +++ b/drivers/media/video/marvell-ccic/mcam-core.h
> @@ -3,6 +3,13 @@
>   *
>   * Copyright 2011 Jonathan Corbet corbet@lwn.net
>   */
> +#ifndef _MCAM_CORE_H
> +#define _MCAM_CORE_H
> +
> +#include <linux/list.h>
> +#include <media/v4l2-common.h>
> +#include <media/v4l2-dev.h>
> +#include <media/videobuf2-core.h>
> 
>  /*
>   * Tracking of streaming I/O buffers.
> @@ -20,8 +27,6 @@ enum mcam_state {
>  	S_NOTREADY,	/* Not yet initialized */
>  	S_IDLE,		/* Just hanging around */
>  	S_FLAKED,	/* Some sort of problem */
> -	S_SINGLEREAD,	/* In read() */
> -	S_SPECREAD,	/* Speculative read (for future read()) */
>  	S_STREAMING	/* Streaming data */
>  };
>  #define MAX_DMA_BUFS 3
> @@ -70,21 +75,19 @@ struct mcam_camera {
> 
>  	struct list_head dev_list;	/* link to other devices */
> 
> +	/* Videobuf2 stuff */
> +	struct vb2_queue vb_queue;
> +	struct list_head buffers;	/* Available frames */
> +
>  	/* DMA buffers */
>  	unsigned int nbufs;		/* How many are alloc'd */
>  	int next_buf;			/* Next to consume (dev_lock) */
>  	unsigned int dma_buf_size;	/* allocated size */
>  	void *dma_bufs[MAX_DMA_BUFS];	/* Internal buffer addresses */
>  	dma_addr_t dma_handles[MAX_DMA_BUFS]; /* Buffer bus addresses */
> -	unsigned int specframes;	/* Unconsumed spec frames (dev_lock) */
>  	unsigned int sequence;		/* Frame sequence number */
> -	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual
> buffers */
> +	unsigned int buf_seq[MAX_DMA_BUFS]; /* Sequence for individual bufs
> */
> 
> -	/* Streaming buffers */
> -	unsigned int n_sbufs;		/* How many we have */
> -	struct mcam_sio_buffer *sb_bufs; /* The array of housekeeping structs
> */
> -	struct list_head sb_avail;	/* Available for data (we own)
> (dev_lock) */
> -	struct list_head sb_full;	/* With data (user space owns)
> (dev_lock) */
>  	struct tasklet_struct s_tasklet;
> 
>  	/* Current operating parameters */
> @@ -94,9 +97,6 @@ struct mcam_camera {
> 
>  	/* Locks */
>  	struct mutex s_mutex; /* Access to this structure */
> -
> -	/* Misc */
> -	wait_queue_head_t iowait;	/* Waiting on frame data */
>  };
> 
> 
> @@ -257,3 +257,5 @@ int mccic_resume(struct mcam_camera *cam);
>   */
>  #define VGA_WIDTH	640
>  #define VGA_HEIGHT	480
> +
> +#endif /* _MCAM_CORE_H */
> --

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center




^ permalink raw reply	[flat|nested] 10+ messages in thread
* Re: [PATCH 1/5] marvell-cam: convert to videobuf2
  2011-06-22 13:59   ` Marek Szyprowski
@ 2011-06-22 14:11     ` Jonathan Corbet
  0 siblings, 0 replies; 10+ messages in thread
From: Jonathan Corbet @ 2011-06-22 14:11 UTC (permalink / raw)
  To: Marek Szyprowski
  Cc: linux-media, g.liakhovetski, 'Kassey Lee',
	'Pawel Osciak'

On Wed, 22 Jun 2011 15:59:04 +0200
Marek Szyprowski <m.szyprowski@samsung.com> wrote:

> > This is a basic, naive conversion to the videobuf2 infrastructure, removing
> > a lot of code in the process.  For now, we're using vmalloc, which is
> > suboptimal, but it does match what the cafe driver did before.  
> 
> Could you elaborate a bit why vmalloc is suboptimal for your case?

Because it requires copying every frame in kernel space.  That's not a
problem with the vb2 vmalloc implementation, obviously, it's just inherent
in that approach.  Systems with the old Cafe controller are probably stuck
with it (though CMA might just make contiguous DMA operation possible); I
hope to not see it used on anything newer.

(Other comments noted, thanks.)

jon

^ permalink raw reply	[flat|nested] 10+ messages in thread
* RE: [PATCH 5/5] marvell-cam: implement contiguous DMA operation
  2011-06-21 20:26   ` Mauro Carvalho Chehab
@ 2011-06-22 14:22     ` Marek Szyprowski
  0 siblings, 0 replies; 10+ messages in thread
From: Marek Szyprowski @ 2011-06-22 14:22 UTC (permalink / raw)
  To: 'Mauro Carvalho Chehab', 'Jonathan Corbet'
  Cc: linux-media, g.liakhovetski, 'Kassey Lee',
	'Pawel Osciak'

Hello,

On Tuesday, June 21, 2011 10:26 PM Mauro Carvalho Chehab wrote:

> Em 20-06-2011 16:14, Jonathan Corbet escreveu:
> > The core driver can now operate in either vmalloc or dma-contig modes;
> > obviously the latter is preferable when it is supported.  Default is
> > currently vmalloc on all platforms; load the module with buffer_mode=1
> for contiguous DMA mode.
> 
> Patch looks correct.
> 
> A side note for vb2 maintainers:
> 
> IMO, vb2 core should take the responsibility to allow to switch between DMA
> scatter/gather and continuous (and, eventually, vmalloc), where the bridge
> driver support more than one option.

> Otherwise, we'll end by having codes similar to that on all drivers that
> can be used on different architectures.

Could you elaborate a bit more on this issue? Depending on driver needs, on
just sets queue->mem_ops to vb2_vmalloc_memops, vb2_dma-contig_memops or 
vb2_dma-sg_memops. There is no dependencies between the core and memory
allocators/handlers.

Best regards
-- 
Marek Szyprowski
Samsung Poland R&D Center



^ permalink raw reply	[flat|nested] 10+ messages in thread
end of thread, other threads:[~2011-06-22 14:22 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2011-06-20 19:14 [RFC] Second marvell-cam patch series Jonathan Corbet
2011-06-20 19:14 ` [PATCH 1/5] marvell-cam: convert to videobuf2 Jonathan Corbet
2011-06-22 13:59   ` Marek Szyprowski
2011-06-22 14:11     ` Jonathan Corbet
2011-06-20 19:14 ` [PATCH 2/5] marvell-cam: include file cleanup Jonathan Corbet
2011-06-20 19:14 ` [PATCH 3/5] marvell-cam: no need to initialize the DMA buffers Jonathan Corbet
2011-06-20 19:14 ` [PATCH 4/5] marvell-cam: Don't spam the logs on frame loss Jonathan Corbet
2011-06-20 19:14 ` [PATCH 5/5] marvell-cam: implement contiguous DMA operation Jonathan Corbet
2011-06-21 20:26   ` Mauro Carvalho Chehab
2011-06-22 14:22     ` Marek Szyprowski

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.