[PATCH 0/4] media i.MX27 camera: fix buffer handling and videobuf2 support.

[PATCH 0/4] media i.MX27 camera: fix buffer handling and videobuf2 support.

[Javier Martin]

The way video buffer handling is programmed for i.MX27 leads
to buffers being written when they are not ready.

It can be easily checked enabling DEBUG features of the driver.

This series migrate the driver to videobuf2 and provide an   
additional discard queue to make sure all the events are handled
in the right order.

I've only tested the series with an i.MX27 device and so I've 
tried not to touch code scpecific for mx25. However, any mx25
tester would be more than welcome.

[PATCH 1/4] media i.MX27 camera: migrate driver to videobuf2
[PATCH 2/4] media i.MX27 camera: add start_stream and stop_stream callbacks.
[PATCH 3/4] media i.MX27 camera: improve discard buffer handling.
[PATCH 4/4] media i.MX27 camera: handle overflows properly.

[PATCH 1/4] media i.MX27 camera: migrate driver to videobuf2

[Javier Martin]

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |  277 ++++++++++++++++++--------------------
 1 files changed, 128 insertions(+), 149 deletions(-)

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 68038e7..290ac9d 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -3,6 +3,7 @@
  *
  * Copyright (C) 2008, Sascha Hauer, Pengutronix
  * Copyright (C) 2010, Baruch Siach, Orex Computed Radiography
+ * Copyright (C) 2012, Javier Martin, Vista Silicon S.L.
  *
  * This program is free software; you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -30,8 +31,8 @@
 
 #include <media/v4l2-common.h>
 #include <media/v4l2-dev.h>
-#include <media/videobuf-core.h>
-#include <media/videobuf-dma-contig.h>
+#include <media/videobuf2-core.h>
+#include <media/videobuf2-dma-contig.h>
 #include <media/soc_camera.h>
 #include <media/soc_mediabus.h>
 
@@ -256,13 +257,25 @@ struct mx2_camera_dev {
 	size_t			discard_size;
 	struct mx2_fmt_cfg	*emma_prp;
 	u32			frame_count;
+	struct vb2_alloc_ctx	*alloc_ctx;
+};
+
+enum mx2_buffer_state {
+	MX2_STATE_NEEDS_INIT = 0,
+	MX2_STATE_PREPARED   = 1,
+	MX2_STATE_QUEUED     = 2,
+	MX2_STATE_ACTIVE     = 3,
+	MX2_STATE_DONE       = 4,
+	MX2_STATE_ERROR      = 5,
+	MX2_STATE_IDLE       = 6,
 };
 
 /* buffer for one video frame */
 struct mx2_buffer {
 	/* common v4l buffer stuff -- must be first */
-	struct videobuf_buffer		vb;
-
+	struct vb2_buffer		vb;
+	struct list_head		queue;
+	enum mx2_buffer_state		state;
 	enum v4l2_mbus_pixelcode	code;
 
 	int bufnum;
@@ -407,7 +420,7 @@ static irqreturn_t mx27_camera_irq(int irq_csi, void *data)
 static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
 		int state)
 {
-	struct videobuf_buffer *vb;
+	struct vb2_buffer *vb;
 	struct mx2_buffer *buf;
 	struct mx2_buffer **fb_active = fb == 1 ? &pcdev->fb1_active :
 		&pcdev->fb2_active;
@@ -420,25 +433,24 @@ static void mx25_camera_frame_done(struct mx2_camera_dev *pcdev, int fb,
 		goto out;
 
 	vb = &(*fb_active)->vb;
-	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
+	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%p %lu\n", __func__,
+		vb, vb2_plane_vaddr(vb, 0), vb2_get_plane_payload(vb, 0));
 
-	vb->state = state;
-	do_gettimeofday(&vb->ts);
-	vb->field_count++;
-
-	wake_up(&vb->done);
+	do_gettimeofday(&vb->v4l2_buf.timestamp);
+	vb->v4l2_buf.sequence++;
+	vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 
 	if (list_empty(&pcdev->capture)) {
 		buf = NULL;
 		writel(0, pcdev->base_csi + fb_reg);
 	} else {
 		buf = list_entry(pcdev->capture.next, struct mx2_buffer,
-				vb.queue);
+				queue);
 		vb = &buf->vb;
-		list_del(&vb->queue);
-		vb->state = VIDEOBUF_ACTIVE;
-		writel(videobuf_to_dma_contig(vb), pcdev->base_csi + fb_reg);
+		list_del(&buf->queue);
+		buf->state = MX2_STATE_ACTIVE;
+		writel(vb2_dma_contig_plane_dma_addr(vb, 0),
+		       pcdev->base_csi + fb_reg);
 	}
 
 	*fb_active = buf;
@@ -453,9 +465,9 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 	u32 status = readl(pcdev->base_csi + CSISR);
 
 	if (status & CSISR_DMA_TSF_FB1_INT)
-		mx25_camera_frame_done(pcdev, 1, VIDEOBUF_DONE);
+		mx25_camera_frame_done(pcdev, 1, MX2_STATE_DONE);
 	else if (status & CSISR_DMA_TSF_FB2_INT)
-		mx25_camera_frame_done(pcdev, 2, VIDEOBUF_DONE);
+		mx25_camera_frame_done(pcdev, 2, MX2_STATE_DONE);
 
 	/* FIXME: handle CSISR_RFF_OR_INT */
 
@@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
 /*
  *  Videobuf operations
  */
-static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
-			      unsigned int *size)
+static int mx2_videobuf_setup(struct vb2_queue *vq,
+			const struct v4l2_format *fmt,
+			unsigned int *count, unsigned int *num_planes,
+			unsigned int sizes[], void *alloc_ctxs[])
 {
-	struct soc_camera_device *icd = vq->priv_data;
+	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
+	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
+	struct mx2_camera_dev *pcdev = ici->priv;
 	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
 			icd->current_fmt->host_fmt);
 
-	dev_dbg(icd->parent, "count=%d, size=%d\n", *count, *size);
+	dev_dbg(icd->parent, "count=%d, size=%d\n", *count, sizes[0]);
 
 	if (bytes_per_line < 0)
 		return bytes_per_line;
 
-	*size = bytes_per_line * icd->user_height;
+	alloc_ctxs[0] = pcdev->alloc_ctx;
+
+	sizes[0] = bytes_per_line * icd->user_height;
 
 	if (0 == *count)
 		*count = 32;
-	if (*size * *count > MAX_VIDEO_MEM * 1024 * 1024)
-		*count = (MAX_VIDEO_MEM * 1024 * 1024) / *size;
-
-	return 0;
-}
+	if (!*num_planes &&
+	    sizes[0] * *count > MAX_VIDEO_MEM * 1024 * 1024)
+		*count = (MAX_VIDEO_MEM * 1024 * 1024) / sizes[0];
 
-static void free_buffer(struct videobuf_queue *vq, struct mx2_buffer *buf)
-{
-	struct soc_camera_device *icd = vq->priv_data;
-	struct videobuf_buffer *vb = &buf->vb;
-
-	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
-
-	/*
-	 * This waits until this buffer is out of danger, i.e., until it is no
-	 * longer in state VIDEOBUF_QUEUED or VIDEOBUF_ACTIVE
-	 */
-	videobuf_waiton(vq, vb, 0, 0);
+	*num_planes = 1;
 
-	videobuf_dma_contig_free(vq, vb);
-	dev_dbg(icd->parent, "%s freed\n", __func__);
-
-	vb->state = VIDEOBUF_NEEDS_INIT;
+	return 0;
 }
 
-static int mx2_videobuf_prepare(struct videobuf_queue *vq,
-		struct videobuf_buffer *vb, enum v4l2_field field)
+static int mx2_videobuf_prepare(struct vb2_buffer *vb)
 {
-	struct soc_camera_device *icd = vq->priv_data;
+	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
 	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
 	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
 			icd->current_fmt->host_fmt);
 	int ret = 0;
 
-	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
+	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%p %lu\n", __func__,
+		vb, vb2_plane_vaddr(vb, 0), vb2_get_plane_payload(vb, 0));
 
 	if (bytes_per_line < 0)
 		return bytes_per_line;
@@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue *vq,
 	 * This can be useful if you want to see if we actually fill
 	 * the buffer with something
 	 */
-	memset((void *)vb->baddr, 0xaa, vb->bsize);
+	memset((void *)vb2_plane_vaddr(vb, 0),
+	       0xaa, vb2_get_plane_payload(vb, 0));
 #endif
 
-	if (buf->code	!= icd->current_fmt->code ||
-	    vb->width	!= icd->user_width ||
-	    vb->height	!= icd->user_height ||
-	    vb->field	!= field) {
+	if (buf->code	!= icd->current_fmt->code) {
 		buf->code	= icd->current_fmt->code;
-		vb->width	= icd->user_width;
-		vb->height	= icd->user_height;
-		vb->field	= field;
-		vb->state	= VIDEOBUF_NEEDS_INIT;
+		buf->state	= MX2_STATE_NEEDS_INIT;
 	}
 
-	vb->size = bytes_per_line * vb->height;
-	if (vb->baddr && vb->bsize < vb->size) {
+	vb2_set_plane_payload(vb, 0, bytes_per_line * icd->user_height);
+	if (vb2_plane_vaddr(vb, 0) &&
+	    vb2_get_plane_payload(vb, 0) < vb2_plane_size(vb, 0)) {
 		ret = -EINVAL;
 		goto out;
 	}
 
-	if (vb->state == VIDEOBUF_NEEDS_INIT) {
-		ret = videobuf_iolock(vq, vb, NULL);
-		if (ret)
-			goto fail;
-
-		vb->state = VIDEOBUF_PREPARED;
-	}
+	if (buf->state == MX2_STATE_NEEDS_INIT)
+		buf->state = MX2_STATE_PREPARED;
 
 	return 0;
 
-fail:
-	free_buffer(vq, buf);
 out:
 	return ret;
 }
 
-static void mx2_videobuf_queue(struct videobuf_queue *vq,
-			       struct videobuf_buffer *vb)
+static void mx2_videobuf_queue(struct vb2_buffer *vb)
 {
-	struct soc_camera_device *icd = vq->priv_data;
+	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->parent);
 	struct mx2_camera_dev *pcdev = ici->priv;
@@ -576,13 +564,13 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
 	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
 	unsigned long flags;
 
-	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
+	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%p %lu\n", __func__,
+		vb, vb2_plane_vaddr(vb, 0), vb2_get_plane_payload(vb, 0));
 
 	spin_lock_irqsave(&pcdev->lock, flags);
 
-	vb->state = VIDEOBUF_QUEUED;
-	list_add_tail(&vb->queue, &pcdev->capture);
+	buf->state = MX2_STATE_QUEUED;
+	list_add_tail(&buf->queue, &pcdev->capture);
 
 	if (mx27_camera_emma(pcdev)) {
 		if (prp->cfg.channel == 1) {
@@ -610,20 +598,20 @@ static void mx2_videobuf_queue(struct videobuf_queue *vq,
 		u32 csicr3, dma_inten = 0;
 
 		if (pcdev->fb1_active == NULL) {
-			writel(videobuf_to_dma_contig(vb),
+			writel(vb2_dma_contig_plane_dma_addr(vb, 0),
 					pcdev->base_csi + CSIDMASA_FB1);
 			pcdev->fb1_active = buf;
 			dma_inten = CSICR1_FB1_DMA_INTEN;
 		} else if (pcdev->fb2_active == NULL) {
-			writel(videobuf_to_dma_contig(vb),
+			writel(vb2_dma_contig_plane_dma_addr(vb, 0),
 					pcdev->base_csi + CSIDMASA_FB2);
 			pcdev->fb2_active = buf;
 			dma_inten = CSICR1_FB2_DMA_INTEN;
 		}
 
 		if (dma_inten) {
-			list_del(&vb->queue);
-			vb->state = VIDEOBUF_ACTIVE;
+			list_del(&buf->queue);
+			buf->state = MX2_STATE_ACTIVE;
 
 			csicr3 = readl(pcdev->base_csi + CSICR3);
 
@@ -646,32 +634,31 @@ out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 }
 
-static void mx2_videobuf_release(struct videobuf_queue *vq,
-				 struct videobuf_buffer *vb)
+static void mx2_videobuf_release(struct vb2_buffer *vb)
 {
-	struct soc_camera_device *icd = vq->priv_data;
+	struct soc_camera_device *icd = soc_camera_from_vb2q(vb->vb2_queue);
 	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
 	struct mx2_camera_dev *pcdev = ici->priv;
 	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
 	unsigned long flags;
 
 #ifdef DEBUG
-	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
+	dev_dbg(icd->parent, "%s (vb=0x%p) 0x%p %lu\n", __func__,
+		vb, vb2_plane_vaddr(vb, 0), vb2_get_plane_payload(vb, 0));
 
-	switch (vb->state) {
-	case VIDEOBUF_ACTIVE:
+	switch (buf->state) {
+	case MX2_STATE_ACTIVE:
 		dev_info(icd->parent, "%s (active)\n", __func__);
 		break;
-	case VIDEOBUF_QUEUED:
+	case MX2_STATE_QUEUED:
 		dev_info(icd->parent, "%s (queued)\n", __func__);
 		break;
-	case VIDEOBUF_PREPARED:
+	case MX2_STATE_PREPARED:
 		dev_info(icd->parent, "%s (prepared)\n", __func__);
 		break;
 	default:
 		dev_info(icd->parent, "%s (unknown) %d\n", __func__,
-				vb->state);
+				buf->state);
 		break;
 	}
 #endif
@@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
 	 * types.
 	 */
 	spin_lock_irqsave(&pcdev->lock, flags);
-	if (vb->state == VIDEOBUF_QUEUED) {
-		list_del(&vb->queue);
-		vb->state = VIDEOBUF_ERROR;
-	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
+	if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
+		list_del_init(&buf->queue);
+		buf->state = MX2_STATE_NEEDS_INIT;
+	} else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
 		if (pcdev->fb1_active == buf) {
 			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
 			writel(0, pcdev->base_csi + CSIDMASA_FB1);
@@ -700,30 +687,29 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
 			pcdev->fb2_active = NULL;
 		}
 		writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
-		vb->state = VIDEOBUF_ERROR;
+		buf->state = MX2_STATE_NEEDS_INIT;
 	}
 	spin_unlock_irqrestore(&pcdev->lock, flags);
-
-	free_buffer(vq, buf);
 }
 
-static struct videobuf_queue_ops mx2_videobuf_ops = {
-	.buf_setup      = mx2_videobuf_setup,
-	.buf_prepare    = mx2_videobuf_prepare,
-	.buf_queue      = mx2_videobuf_queue,
-	.buf_release    = mx2_videobuf_release,
+static struct vb2_ops mx2_videobuf_ops = {
+	.queue_setup	= mx2_videobuf_setup,
+	.buf_prepare	= mx2_videobuf_prepare,
+	.buf_queue	= mx2_videobuf_queue,
+	.buf_cleanup	= mx2_videobuf_release,
 };
 
-static void mx2_camera_init_videobuf(struct videobuf_queue *q,
+static int mx2_camera_init_videobuf(struct vb2_queue *q,
 			      struct soc_camera_device *icd)
 {
-	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
-	struct mx2_camera_dev *pcdev = ici->priv;
-
-	videobuf_queue_dma_contig_init(q, &mx2_videobuf_ops, pcdev->dev,
-			&pcdev->lock, V4L2_BUF_TYPE_VIDEO_CAPTURE,
-			V4L2_FIELD_NONE, sizeof(struct mx2_buffer),
-			icd, &icd->video_lock);
+	q->type = V4L2_BUF_TYPE_VIDEO_CAPTURE;
+	q->io_modes = VB2_MMAP | VB2_USERPTR;
+	q->drv_priv = icd;
+	q->ops = &mx2_videobuf_ops;
+	q->mem_ops = &vb2_dma_contig_memops;
+	q->buf_struct_size = sizeof(struct mx2_buffer);
+
+	return vb2_queue_init(q);
 }
 
 #define MX2_BUS_FLAGS	(V4L2_MBUS_MASTER | \
@@ -1137,25 +1123,10 @@ static int mx2_camera_querycap(struct soc_camera_host *ici,
 	return 0;
 }
 
-static int mx2_camera_reqbufs(struct soc_camera_device *icd,
-			      struct v4l2_requestbuffers *p)
-{
-	int i;
-
-	for (i = 0; i < p->count; i++) {
-		struct mx2_buffer *buf = container_of(icd->vb_vidq.bufs[i],
-						      struct mx2_buffer, vb);
-		INIT_LIST_HEAD(&buf->vb.queue);
-	}
-
-	return 0;
-}
-
 static unsigned int mx2_camera_poll(struct file *file, poll_table *pt)
 {
 	struct soc_camera_device *icd = file->private_data;
-
-	return videobuf_poll_stream(file, &icd->vb_vidq, pt);
+	return vb2_poll(&icd->vb2_vidq, file, pt);
 }
 
 static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
@@ -1166,8 +1137,7 @@ static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
 	.set_crop	= mx2_camera_set_crop,
 	.get_formats	= mx2_camera_get_formats,
 	.try_fmt	= mx2_camera_try_fmt,
-	.init_videobuf	= mx2_camera_init_videobuf,
-	.reqbufs	= mx2_camera_reqbufs,
+	.init_videobuf2	= mx2_camera_init_videobuf,
 	.poll		= mx2_camera_poll,
 	.querycap	= mx2_camera_querycap,
 	.set_bus_param	= mx2_camera_set_bus_param,
@@ -1179,18 +1149,18 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
 	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 	struct mx2_buffer *buf;
-	struct videobuf_buffer *vb;
+	struct vb2_buffer *vb;
 	unsigned long phys;
 
 	if (!list_empty(&pcdev->active_bufs)) {
 		buf = list_entry(pcdev->active_bufs.next,
-			struct mx2_buffer, vb.queue);
+			struct mx2_buffer, queue);
 
 		BUG_ON(buf->bufnum != bufnum);
 
 		vb = &buf->vb;
 #ifdef DEBUG
-		phys = videobuf_to_dma_contig(vb);
+		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
 		if (prp->cfg.channel == 1) {
 			if (readl(pcdev->base_emma + PRP_DEST_RGB1_PTR +
 				4 * bufnum) != phys) {
@@ -1209,15 +1179,15 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 			}
 		}
 #endif
-		dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, vb,
-				vb->baddr, vb->bsize);
-
-		list_del(&vb->queue);
-		vb->state = state;
-		do_gettimeofday(&vb->ts);
-		vb->field_count = pcdev->frame_count * 2;
-
-		wake_up(&vb->done);
+		dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%p %lu\n", __func__, vb,
+				vb2_plane_vaddr(vb, 0),
+				vb2_get_plane_payload(vb, 0));
+
+		list_del(&buf->queue);
+		buf->state = state;
+		do_gettimeofday(&vb->v4l2_buf.timestamp);
+		vb->v4l2_buf.sequence = pcdev->frame_count;
+		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 	}
 
 	if (list_empty(&pcdev->capture)) {
@@ -1243,16 +1213,16 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 	pcdev->frame_count++;
 
 	buf = list_entry(pcdev->capture.next,
-			struct mx2_buffer, vb.queue);
+			struct mx2_buffer, queue);
 
 	buf->bufnum = !bufnum;
 
 	list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
 
 	vb = &buf->vb;
-	vb->state = VIDEOBUF_ACTIVE;
+	buf->state = MX2_STATE_ACTIVE;
 
-	phys = videobuf_to_dma_contig(vb);
+	phys = vb2_dma_contig_plane_dma_addr(vb, 0);
 	if (prp->cfg.channel == 1) {
 		writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
 	} else {
@@ -1296,14 +1266,14 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
 		 * to first
 		 */
 		buf = list_entry(pcdev->active_bufs.next,
-			struct mx2_buffer, vb.queue);
-		mx27_camera_frame_done_emma(pcdev, buf->bufnum, VIDEOBUF_DONE);
+			struct mx2_buffer, queue);
+		mx27_camera_frame_done_emma(pcdev, buf->bufnum, MX2_STATE_DONE);
 		status &= ~(1 << (6 - buf->bufnum)); /* mark processed */
 	}
 	if ((status & (1 << 6)) || (status & (1 << 4)))
-		mx27_camera_frame_done_emma(pcdev, 0, VIDEOBUF_DONE);
+		mx27_camera_frame_done_emma(pcdev, 0, MX2_STATE_DONE);
 	if ((status & (1 << 5)) || (status & (1 << 3)))
-		mx27_camera_frame_done_emma(pcdev, 1, VIDEOBUF_DONE);
+		mx27_camera_frame_done_emma(pcdev, 1, MX2_STATE_DONE);
 
 	writel(status, pcdev->base_emma + PRP_INTRSTATUS);
 
@@ -1464,6 +1434,12 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
 	pcdev->soc_host.priv		= pcdev;
 	pcdev->soc_host.v4l2_dev.dev	= &pdev->dev;
 	pcdev->soc_host.nr		= pdev->id;
+
+	pcdev->alloc_ctx = vb2_dma_contig_init_ctx(&pdev->dev);
+	if (IS_ERR(pcdev->alloc_ctx)) {
+		err = PTR_ERR(pcdev->alloc_ctx);
+		goto eallocctx;
+	}
 	err = soc_camera_host_register(&pcdev->soc_host);
 	if (err)
 		goto exit_free_emma;
@@ -1472,8 +1448,9 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
 			clk_get_rate(pcdev->clk_csi));
 
 	return 0;
-
 exit_free_emma:
+	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
+eallocctx:
 	if (mx27_camera_emma(pcdev)) {
 		free_irq(pcdev->irq_emma, pcdev);
 		clk_disable(pcdev->clk_emma);
@@ -1508,6 +1485,8 @@ static int __devexit mx2_camera_remove(struct platform_device *pdev)
 
 	soc_camera_host_unregister(&pcdev->soc_host);
 
+	vb2_dma_contig_cleanup_ctx(pcdev->alloc_ctx);
+
 	iounmap(pcdev->base_csi);
 
 	if (mx27_camera_emma(pcdev)) {
-- 
1.7.0.4

[PATCH 2/4] media i.MX27 camera: add start_stream and stop_stream callbacks.

[Javier Martin]

Add "start_stream" and "stop_stream" callback in order to enable
and disable the eMMa-PrP properly and save CPU usage avoiding
IRQs when the device is not streaming.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |  107 +++++++++++++++++++++++++++-----------
 1 files changed, 77 insertions(+), 30 deletions(-)

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 290ac9d..4816da6 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -560,7 +560,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
 	struct soc_camera_host *ici =
 		to_soc_camera_host(icd->parent);
 	struct mx2_camera_dev *pcdev = ici->priv;
-	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
 	unsigned long flags;
 
@@ -572,29 +571,7 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
 	buf->state = MX2_STATE_QUEUED;
 	list_add_tail(&buf->queue, &pcdev->capture);
 
-	if (mx27_camera_emma(pcdev)) {
-		if (prp->cfg.channel == 1) {
-			writel(PRP_CNTL_CH1EN |
-				PRP_CNTL_CSIEN |
-				prp->cfg.in_fmt |
-				prp->cfg.out_fmt |
-				PRP_CNTL_CH1_LEN |
-				PRP_CNTL_CH1BYP |
-				PRP_CNTL_CH1_TSKIP(0) |
-				PRP_CNTL_IN_TSKIP(0),
-				pcdev->base_emma + PRP_CNTL);
-		} else {
-			writel(PRP_CNTL_CH2EN |
-				PRP_CNTL_CSIEN |
-				prp->cfg.in_fmt |
-				prp->cfg.out_fmt |
-				PRP_CNTL_CH2_LEN |
-				PRP_CNTL_CH2_TSKIP(0) |
-				PRP_CNTL_IN_TSKIP(0),
-				pcdev->base_emma + PRP_CNTL);
-		}
-		goto out;
-	} else { /* cpu_is_mx25() */
+	if (!mx27_camera_emma(pcdev)) { /* cpu_is_mx25() */
 		u32 csicr3, dma_inten = 0;
 
 		if (pcdev->fb1_active == NULL) {
@@ -629,8 +606,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
 			writel(csicr3, pcdev->base_csi + CSICR3);
 		}
 	}
-
-out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 }
 
@@ -692,11 +667,83 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 }
 
+static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
+{
+	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icd->parent);
+	struct mx2_camera_dev *pcdev = ici->priv;
+	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
+	unsigned long flags;
+	int ret = 0;
+
+	spin_lock_irqsave(&pcdev->lock, flags);
+	if (mx27_camera_emma(pcdev)) {
+		if (count < 2) {
+			ret = -EINVAL;
+			goto err;
+		}
+
+		if (prp->cfg.channel == 1) {
+			writel(PRP_CNTL_CH1EN |
+				PRP_CNTL_CSIEN |
+				prp->cfg.in_fmt |
+				prp->cfg.out_fmt |
+				PRP_CNTL_CH1_LEN |
+				PRP_CNTL_CH1BYP |
+				PRP_CNTL_CH1_TSKIP(0) |
+				PRP_CNTL_IN_TSKIP(0),
+				pcdev->base_emma + PRP_CNTL);
+		} else {
+			writel(PRP_CNTL_CH2EN |
+				PRP_CNTL_CSIEN |
+				prp->cfg.in_fmt |
+				prp->cfg.out_fmt |
+				PRP_CNTL_CH2_LEN |
+				PRP_CNTL_CH2_TSKIP(0) |
+				PRP_CNTL_IN_TSKIP(0),
+				pcdev->base_emma + PRP_CNTL);
+		}
+	}
+err:
+	spin_unlock_irqrestore(&pcdev->lock, flags);
+
+	return ret;
+}
+
+static int mx2_stop_streaming(struct vb2_queue *q)
+{
+	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
+	struct soc_camera_host *ici =
+		to_soc_camera_host(icd->parent);
+	struct mx2_camera_dev *pcdev = ici->priv;
+	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
+	unsigned long flags;
+	u32 cntl;
+
+	spin_lock_irqsave(&pcdev->lock, flags);
+	if (mx27_camera_emma(pcdev)) {
+		cntl = readl(pcdev->base_emma + PRP_CNTL);
+		if (prp->cfg.channel == 1) {
+			writel(cntl & ~PRP_CNTL_CH1EN,
+			       pcdev->base_emma + PRP_CNTL);
+		} else {
+			writel(cntl & ~PRP_CNTL_CH2EN,
+			       pcdev->base_emma + PRP_CNTL);
+		}
+	}
+	spin_unlock_irqrestore(&pcdev->lock, flags);
+
+	return 0;
+}
+
 static struct vb2_ops mx2_videobuf_ops = {
-	.queue_setup	= mx2_videobuf_setup,
-	.buf_prepare	= mx2_videobuf_prepare,
-	.buf_queue	= mx2_videobuf_queue,
-	.buf_cleanup	= mx2_videobuf_release,
+	.queue_setup	 = mx2_videobuf_setup,
+	.buf_prepare	 = mx2_videobuf_prepare,
+	.buf_queue	 = mx2_videobuf_queue,
+	.buf_cleanup	 = mx2_videobuf_release,
+	.start_streaming = mx2_start_streaming,
+	.stop_streaming  = mx2_stop_streaming,
 };
 
 static int mx2_camera_init_videobuf(struct vb2_queue *q,
-- 
1.7.0.4

[PATCH 3/4] media i.MX27 camera: improve discard buffer handling.

[Javier Martin]

The way discard buffer was previously handled lead
to possible races that made a buffer that was not
yet ready to be overwritten by new video data. This
is easily detected at 25fps just adding "#define DEBUG"
to enable the "memset" check and seeing how the image
is corrupted.

A new "discard" queue and two discard buffers have
been added to make them flow trough the pipeline
of queues and thus provide suitable event ordering.

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |  215 +++++++++++++++++++++-----------------
 1 files changed, 117 insertions(+), 98 deletions(-)

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index 4816da6..e0c5dd4 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
 	struct mx2_prp_cfg		cfg;
 };
 
+enum mx2_buffer_state {
+	MX2_STATE_NEEDS_INIT = 0,
+	MX2_STATE_PREPARED   = 1,
+	MX2_STATE_QUEUED     = 2,
+	MX2_STATE_ACTIVE     = 3,
+	MX2_STATE_DONE       = 4,
+	MX2_STATE_ERROR      = 5,
+	MX2_STATE_IDLE       = 6,
+};
+
+/* buffer for one video frame */
+struct mx2_buffer {
+	/* common v4l buffer stuff -- must be first */
+	struct vb2_buffer		vb;
+	struct list_head		queue;
+	enum mx2_buffer_state		state;
+	enum v4l2_mbus_pixelcode	code;
+
+	int				bufnum;
+	bool				discard;
+};
+
 struct mx2_camera_dev {
 	struct device		*dev;
 	struct soc_camera_host	soc_host;
@@ -240,6 +262,7 @@ struct mx2_camera_dev {
 
 	struct list_head	capture;
 	struct list_head	active_bufs;
+	struct list_head	discard;
 
 	spinlock_t		lock;
 
@@ -252,6 +275,7 @@ struct mx2_camera_dev {
 
 	u32			csicr1;
 
+	struct mx2_buffer	buf_discard[2];
 	void			*discard_buffer;
 	dma_addr_t		discard_buffer_dma;
 	size_t			discard_size;
@@ -260,27 +284,6 @@ struct mx2_camera_dev {
 	struct vb2_alloc_ctx	*alloc_ctx;
 };
 
-enum mx2_buffer_state {
-	MX2_STATE_NEEDS_INIT = 0,
-	MX2_STATE_PREPARED   = 1,
-	MX2_STATE_QUEUED     = 2,
-	MX2_STATE_ACTIVE     = 3,
-	MX2_STATE_DONE       = 4,
-	MX2_STATE_ERROR      = 5,
-	MX2_STATE_IDLE       = 6,
-};
-
-/* buffer for one video frame */
-struct mx2_buffer {
-	/* common v4l buffer stuff -- must be first */
-	struct vb2_buffer		vb;
-	struct list_head		queue;
-	enum mx2_buffer_state		state;
-	enum v4l2_mbus_pixelcode	code;
-
-	int bufnum;
-};
-
 static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
 	/*
 	 * This is a generic configuration which is valid for most
@@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
 	return &mx27_emma_prp_table[0];
 };
 
+static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
+				 unsigned long phys, int bufnum)
+{
+	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
+	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
+
+	if (prp->cfg.channel == 1) {
+		writel(phys, pcdev->base_emma +
+				PRP_DEST_RGB1_PTR + 4 * bufnum);
+	} else {
+		writel(phys, pcdev->base_emma +
+					PRP_DEST_Y_PTR -
+					0x14 * bufnum);
+		if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
+			writel(phys + imgsize,
+			pcdev->base_emma + PRP_DEST_CB_PTR -
+			0x14 * bufnum);
+			writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
+			PRP_DEST_CR_PTR - 0x14 * bufnum);
+		}
+	}
+}
+
 static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
 {
 	unsigned long flags;
@@ -382,7 +408,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
 	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
 
 	pcdev->icd = icd;
-	pcdev->frame_count = -1;
+	pcdev->frame_count = 0;
 
 	dev_info(icd->parent, "Camera driver attached to camera %d\n",
 		 icd->devnum);
@@ -648,10 +674,9 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
 	 * types.
 	 */
 	spin_lock_irqsave(&pcdev->lock, flags);
-	if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
-		list_del_init(&buf->queue);
-		buf->state = MX2_STATE_NEEDS_INIT;
-	} else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
+	INIT_LIST_HEAD(&buf->queue);
+	buf->state = MX2_STATE_NEEDS_INIT;
+	if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
 		if (pcdev->fb1_active == buf) {
 			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
 			writel(0, pcdev->base_csi + CSIDMASA_FB1);
@@ -674,7 +699,10 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 		to_soc_camera_host(icd->parent);
 	struct mx2_camera_dev *pcdev = ici->priv;
 	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
+	struct vb2_buffer *vb;
+	struct mx2_buffer *buf;
 	unsigned long flags;
+	unsigned long phys;
 	int ret = 0;
 
 	spin_lock_irqsave(&pcdev->lock, flags);
@@ -684,6 +712,26 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
 			goto err;
 		}
 
+		buf = list_entry(pcdev->capture.next,
+				 struct mx2_buffer, queue);
+		buf->bufnum = 0;
+		vb = &buf->vb;
+		buf->state = MX2_STATE_ACTIVE;
+
+		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
+		mx27_update_emma_buf(pcdev, phys, buf->bufnum);
+		list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
+
+		buf = list_entry(pcdev->capture.next,
+				 struct mx2_buffer, queue);
+		buf->bufnum = 1;
+		vb = &buf->vb;
+		buf->state = MX2_STATE_ACTIVE;
+
+		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
+		mx27_update_emma_buf(pcdev, phys, buf->bufnum);
+		list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
+
 		if (prp->cfg.channel == 1) {
 			writel(PRP_CNTL_CH1EN |
 				PRP_CNTL_CSIEN |
@@ -731,6 +779,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
 			writel(cntl & ~PRP_CNTL_CH2EN,
 			       pcdev->base_emma + PRP_CNTL);
 		}
+		INIT_LIST_HEAD(&pcdev->capture);
+		INIT_LIST_HEAD(&pcdev->active_bufs);
+		INIT_LIST_HEAD(&pcdev->discard);
 	}
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 
@@ -793,50 +844,21 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
 		to_soc_camera_host(icd->parent);
 	struct mx2_camera_dev *pcdev = ici->priv;
 	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
-	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
 
+	writel((icd->user_width << 16) | icd->user_height,
+	       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
+	writel(prp->cfg.src_pixel,
+	       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
 	if (prp->cfg.channel == 1) {
-		writel(pcdev->discard_buffer_dma,
-				pcdev->base_emma + PRP_DEST_RGB1_PTR);
-		writel(pcdev->discard_buffer_dma,
-				pcdev->base_emma + PRP_DEST_RGB2_PTR);
-
-		writel((icd->user_width << 16) | icd->user_height,
-			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
 		writel((icd->user_width << 16) | icd->user_height,
 			pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
 		writel(bytesperline,
 			pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
-		writel(prp->cfg.src_pixel,
-			pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
 		writel(prp->cfg.ch1_pixel,
 			pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
 	} else { /* channel 2 */
-		writel(pcdev->discard_buffer_dma,
-			pcdev->base_emma + PRP_DEST_Y_PTR);
-		writel(pcdev->discard_buffer_dma,
-			pcdev->base_emma + PRP_SOURCE_Y_PTR);
-
-		if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
-			writel(pcdev->discard_buffer_dma + imgsize,
-				pcdev->base_emma + PRP_DEST_CB_PTR);
-			writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
-				pcdev->base_emma + PRP_DEST_CR_PTR);
-			writel(pcdev->discard_buffer_dma + imgsize,
-				pcdev->base_emma + PRP_SOURCE_CB_PTR);
-			writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
-				pcdev->base_emma + PRP_SOURCE_CR_PTR);
-		}
-
-		writel((icd->user_width << 16) | icd->user_height,
-			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
-
 		writel((icd->user_width << 16) | icd->user_height,
 			pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
-
-		writel(prp->cfg.src_pixel,
-			pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
-
 	}
 
 	/* Enable interrupts */
@@ -927,11 +949,22 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
 		if (ret)
 			return ret;
 
-		if (pcdev->discard_buffer)
+		if (pcdev->discard_buffer) {
 			dma_free_coherent(ici->v4l2_dev.dev,
 				pcdev->discard_size, pcdev->discard_buffer,
 				pcdev->discard_buffer_dma);
 
+			pcdev->buf_discard[0].discard = true;
+			INIT_LIST_HEAD(&pcdev->buf_discard[0].queue);
+			list_add_tail(&pcdev->buf_discard[0].queue,
+				      &pcdev->discard);
+
+			pcdev->buf_discard[1].discard = true;
+			INIT_LIST_HEAD(&pcdev->buf_discard[1].queue);
+			list_add_tail(&pcdev->buf_discard[1].queue,
+				      &pcdev->discard);
+		}
+
 		/*
 		 * I didn't manage to properly enable/disable the prp
 		 * on a per frame basis during running transfers,
@@ -1193,18 +1226,24 @@ static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
 static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 		int bufnum, int state)
 {
-	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
-	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
 	struct mx2_buffer *buf;
 	struct vb2_buffer *vb;
 	unsigned long phys;
 
-	if (!list_empty(&pcdev->active_bufs)) {
-		buf = list_entry(pcdev->active_bufs.next,
-			struct mx2_buffer, queue);
+	BUG_ON(list_empty(&pcdev->active_bufs));
+
+	buf = list_entry(pcdev->active_bufs.next,
+			 struct mx2_buffer, queue);
 
-		BUG_ON(buf->bufnum != bufnum);
+	BUG_ON(buf->bufnum != bufnum);
 
+	if (buf->discard) {
+		/*
+		 * Discard buffer must not be returned to user space.
+		 * Just return it to the discard queue.
+		 */
+		list_move_tail(pcdev->active_bufs.next, &pcdev->discard);
+	} else {
 		vb = &buf->vb;
 #ifdef DEBUG
 		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
@@ -1226,6 +1265,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 			}
 		}
 #endif
+
 		dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%p %lu\n", __func__, vb,
 				vb2_plane_vaddr(vb, 0),
 				vb2_get_plane_payload(vb, 0));
@@ -1237,32 +1277,21 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 	}
 
-	if (list_empty(&pcdev->capture)) {
-		if (prp->cfg.channel == 1) {
-			writel(pcdev->discard_buffer_dma, pcdev->base_emma +
-					PRP_DEST_RGB1_PTR + 4 * bufnum);
-		} else {
-			writel(pcdev->discard_buffer_dma, pcdev->base_emma +
-						PRP_DEST_Y_PTR -
-						0x14 * bufnum);
-			if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
-				writel(pcdev->discard_buffer_dma + imgsize,
-				       pcdev->base_emma + PRP_DEST_CB_PTR -
-				       0x14 * bufnum);
-				writel(pcdev->discard_buffer_dma +
-				       ((5 * imgsize) / 4), pcdev->base_emma +
-				       PRP_DEST_CR_PTR - 0x14 * bufnum);
-			}
-		}
+	pcdev->frame_count++;
+
+	if (list_empty(&pcdev->capture) && !list_empty(&pcdev->discard)) {
+		buf = list_entry(pcdev->discard.next,
+			struct mx2_buffer, queue);
+		buf->bufnum = bufnum;
+		list_move_tail(pcdev->discard.next, &pcdev->active_bufs);
+		mx27_update_emma_buf(pcdev, pcdev->discard_buffer_dma, bufnum);
 		return;
 	}
 
-	pcdev->frame_count++;
-
 	buf = list_entry(pcdev->capture.next,
 			struct mx2_buffer, queue);
 
-	buf->bufnum = !bufnum;
+	buf->bufnum = bufnum;
 
 	list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
 
@@ -1270,18 +1299,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 	buf->state = MX2_STATE_ACTIVE;
 
 	phys = vb2_dma_contig_plane_dma_addr(vb, 0);
-	if (prp->cfg.channel == 1) {
-		writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
-	} else {
-		writel(phys, pcdev->base_emma +
-				PRP_DEST_Y_PTR - 0x14 * bufnum);
-		if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
-			writel(phys + imgsize, pcdev->base_emma +
-					PRP_DEST_CB_PTR - 0x14 * bufnum);
-			writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
-					PRP_DEST_CR_PTR - 0x14 * bufnum);
-		}
-	}
+	mx27_update_emma_buf(pcdev, phys, bufnum);
 }
 
 static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
@@ -1433,6 +1451,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
 
 	INIT_LIST_HEAD(&pcdev->capture);
 	INIT_LIST_HEAD(&pcdev->active_bufs);
+	INIT_LIST_HEAD(&pcdev->discard);
 	spin_lock_init(&pcdev->lock);
 
 	/*
-- 
1.7.0.4

[PATCH 4/4] media i.MX27 camera: handle overflows properly.

[Javier Martin]

Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
---
 drivers/media/video/mx2_camera.c |   23 +++++++++--------------
 1 files changed, 9 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
index e0c5dd4..cdc614f 100644
--- a/drivers/media/video/mx2_camera.c
+++ b/drivers/media/video/mx2_camera.c
@@ -1274,7 +1274,10 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
 		buf->state = state;
 		do_gettimeofday(&vb->v4l2_buf.timestamp);
 		vb->v4l2_buf.sequence = pcdev->frame_count;
-		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
+		if (state == MX2_STATE_ERROR)
+			vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
+		else
+			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
 	}
 
 	pcdev->frame_count++;
@@ -1309,19 +1312,11 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
 	struct mx2_buffer *buf;
 
 	if (status & (1 << 7)) { /* overflow */
-		u32 cntl;
-		/*
-		 * We only disable channel 1 here since this is the only
-		 * enabled channel
-		 *
-		 * FIXME: the correct DMA overflow handling should be resetting
-		 * the buffer, returning an error frame, and continuing with
-		 * the next one.
-		 */
-		cntl = readl(pcdev->base_emma + PRP_CNTL);
-		writel(cntl & ~(PRP_CNTL_CH1EN | PRP_CNTL_CH2EN),
-		       pcdev->base_emma + PRP_CNTL);
-		writel(cntl, pcdev->base_emma + PRP_CNTL);
+		buf = list_entry(pcdev->active_bufs.next,
+			struct mx2_buffer, queue);
+		mx27_camera_frame_done_emma(pcdev,
+					buf->bufnum, MX2_STATE_ERROR);
+		status &= ~(1 << 7);
 	}
 	if ((((status & (3 << 5)) == (3 << 5)) ||
 		((status & (3 << 3)) == (3 << 3)))
-- 
1.7.0.4

Re: [PATCH 1/4] media i.MX27 camera: migrate driver to videobuf2

[Guennadi Liakhovetski]

This patch seems incomplete to me? On the one hand you're saying, you only 
work with i.MX27, but you've left

static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
{
	struct videobuf_buffer *vb;

TBH, I don't understand how you've tested this patch: it doesn't compile 
for me for i.MX27. And to use EMMA CONFIG_MACH_MX27 has to be on too, 
right? Confused...

On Fri, 20 Jan 2012, Javier Martin wrote:

> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |  277 ++++++++++++++++++--------------------
>  1 files changed, 128 insertions(+), 149 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 68038e7..290ac9d 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c

[snip]

> @@ -256,13 +257,25 @@ struct mx2_camera_dev {
>  	size_t			discard_size;
>  	struct mx2_fmt_cfg	*emma_prp;
>  	u32			frame_count;
> +	struct vb2_alloc_ctx	*alloc_ctx;
> +};
> +
> +enum mx2_buffer_state {
> +	MX2_STATE_NEEDS_INIT = 0,
> +	MX2_STATE_PREPARED   = 1,
> +	MX2_STATE_QUEUED     = 2,
> +	MX2_STATE_ACTIVE     = 3,
> +	MX2_STATE_DONE       = 4,
> +	MX2_STATE_ERROR      = 5,
> +	MX2_STATE_IDLE       = 6,

Are the numerical values important? If not - please, drop. And actually, 
you don't need most of these states, I wouldn't be surprised, if you 
didn't need them at all. You might want to revise them in a future patch.

[snip]

> @@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
>  /*
>   *  Videobuf operations
>   */
> -static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
> -			      unsigned int *size)
> +static int mx2_videobuf_setup(struct vb2_queue *vq,
> +			const struct v4l2_format *fmt,
> +			unsigned int *count, unsigned int *num_planes,
> +			unsigned int sizes[], void *alloc_ctxs[])
>  {
> -	struct soc_camera_device *icd = vq->priv_data;
> +	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> +	struct mx2_camera_dev *pcdev = ici->priv;
>  	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>  			icd->current_fmt->host_fmt);

You choose not to support VIDIOC_CREATE_BUFS? You have to at least return 
an error if fmt != NULL. Or consider supporting it - look at mx3_camera.c 
or sh_mobile_ceu_camera.c (btw, atmel-isi.c has to be fixed with this 
respect too). If you decide to support it, you'll also have to drop 
.buf_prepare() (see, e.g., 07f92448045a23d27dbc3ece3abcb6bafc618d43)

[snip]

> @@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue *vq,
>  	 * This can be useful if you want to see if we actually fill
>  	 * the buffer with something
>  	 */
> -	memset((void *)vb->baddr, 0xaa, vb->bsize);
> +	memset((void *)vb2_plane_vaddr(vb, 0),
> +	       0xaa, vb2_get_plane_payload(vb, 0));
>  #endif
>  
> -	if (buf->code	!= icd->current_fmt->code ||
> -	    vb->width	!= icd->user_width ||
> -	    vb->height	!= icd->user_height ||
> -	    vb->field	!= field) {
> +	if (buf->code	!= icd->current_fmt->code) {
>  		buf->code	= icd->current_fmt->code;
> -		vb->width	= icd->user_width;
> -		vb->height	= icd->user_height;
> -		vb->field	= field;
> -		vb->state	= VIDEOBUF_NEEDS_INIT;
> +		buf->state	= MX2_STATE_NEEDS_INIT;

This looks broken or most likely redundant to me. The check for a changed 
code was there to reallocate the buffer, doesn't seem to make much sense 
now.

[snip]

> @@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
>  	 * types.
>  	 */
>  	spin_lock_irqsave(&pcdev->lock, flags);
> -	if (vb->state == VIDEOBUF_QUEUED) {
> -		list_del(&vb->queue);
> -		vb->state = VIDEOBUF_ERROR;
> -	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
> +	if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
> +		list_del_init(&buf->queue);
> +		buf->state = MX2_STATE_NEEDS_INIT;
> +	} else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {

This doesn't look right. You already have " || buf->state == 
MX2_STATE_ACTIVE" above, so, this latter condition is never entered?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 1/4] media i.MX27 camera: migrate driver to videobuf2

[Guennadi Liakhovetski]

On Wed, 25 Jan 2012, Guennadi Liakhovetski wrote:

> This patch seems incomplete to me? On the one hand you're saying, you only 
> work with i.MX27, but you've left
> 
> static void mx27_camera_frame_done(struct mx2_camera_dev *pcdev, int state)
> {
> 	struct videobuf_buffer *vb;
> 
> TBH, I don't understand how you've tested this patch: it doesn't compile 
> for me for i.MX27. And to use EMMA CONFIG_MACH_MX27 has to be on too, 
> right? Confused...

Ok, got it, I missed Sascha's patch, removing legacy i.MX27 DMA support. 
Will schedule it for the next window.

Thanks
Guennadi

> 
> On Fri, 20 Jan 2012, Javier Martin wrote:
> 
> > 
> > Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> > ---
> >  drivers/media/video/mx2_camera.c |  277 ++++++++++++++++++--------------------
> >  1 files changed, 128 insertions(+), 149 deletions(-)
> > 
> > diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> > index 68038e7..290ac9d 100644
> > --- a/drivers/media/video/mx2_camera.c
> > +++ b/drivers/media/video/mx2_camera.c
> 
> [snip]
> 
> > @@ -256,13 +257,25 @@ struct mx2_camera_dev {
> >  	size_t			discard_size;
> >  	struct mx2_fmt_cfg	*emma_prp;
> >  	u32			frame_count;
> > +	struct vb2_alloc_ctx	*alloc_ctx;
> > +};
> > +
> > +enum mx2_buffer_state {
> > +	MX2_STATE_NEEDS_INIT = 0,
> > +	MX2_STATE_PREPARED   = 1,
> > +	MX2_STATE_QUEUED     = 2,
> > +	MX2_STATE_ACTIVE     = 3,
> > +	MX2_STATE_DONE       = 4,
> > +	MX2_STATE_ERROR      = 5,
> > +	MX2_STATE_IDLE       = 6,
> 
> Are the numerical values important? If not - please, drop. And actually, 
> you don't need most of these states, I wouldn't be surprised, if you 
> didn't need them at all. You might want to revise them in a future patch.
> 
> [snip]
> 
> > @@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
> >  /*
> >   *  Videobuf operations
> >   */
> > -static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
> > -			      unsigned int *size)
> > +static int mx2_videobuf_setup(struct vb2_queue *vq,
> > +			const struct v4l2_format *fmt,
> > +			unsigned int *count, unsigned int *num_planes,
> > +			unsigned int sizes[], void *alloc_ctxs[])
> >  {
> > -	struct soc_camera_device *icd = vq->priv_data;
> > +	struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
> > +	struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
> > +	struct mx2_camera_dev *pcdev = ici->priv;
> >  	int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
> >  			icd->current_fmt->host_fmt);
> 
> You choose not to support VIDIOC_CREATE_BUFS? You have to at least return 
> an error if fmt != NULL. Or consider supporting it - look at mx3_camera.c 
> or sh_mobile_ceu_camera.c (btw, atmel-isi.c has to be fixed with this 
> respect too). If you decide to support it, you'll also have to drop 
> .buf_prepare() (see, e.g., 07f92448045a23d27dbc3ece3abcb6bafc618d43)
> 
> [snip]
> 
> > @@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue *vq,
> >  	 * This can be useful if you want to see if we actually fill
> >  	 * the buffer with something
> >  	 */
> > -	memset((void *)vb->baddr, 0xaa, vb->bsize);
> > +	memset((void *)vb2_plane_vaddr(vb, 0),
> > +	       0xaa, vb2_get_plane_payload(vb, 0));
> >  #endif
> >  
> > -	if (buf->code	!= icd->current_fmt->code ||
> > -	    vb->width	!= icd->user_width ||
> > -	    vb->height	!= icd->user_height ||
> > -	    vb->field	!= field) {
> > +	if (buf->code	!= icd->current_fmt->code) {
> >  		buf->code	= icd->current_fmt->code;
> > -		vb->width	= icd->user_width;
> > -		vb->height	= icd->user_height;
> > -		vb->field	= field;
> > -		vb->state	= VIDEOBUF_NEEDS_INIT;
> > +		buf->state	= MX2_STATE_NEEDS_INIT;
> 
> This looks broken or most likely redundant to me. The check for a changed 
> code was there to reallocate the buffer, doesn't seem to make much sense 
> now.
> 
> [snip]
> 
> > @@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
> >  	 * types.
> >  	 */
> >  	spin_lock_irqsave(&pcdev->lock, flags);
> > -	if (vb->state == VIDEOBUF_QUEUED) {
> > -		list_del(&vb->queue);
> > -		vb->state = VIDEOBUF_ERROR;
> > -	} else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
> > +	if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
> > +		list_del_init(&buf->queue);
> > +		buf->state = MX2_STATE_NEEDS_INIT;
> > +	} else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
> 
> This doesn't look right. You already have " || buf->state == 
> MX2_STATE_ACTIVE" above, so, this latter condition is never entered?
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski, Ph.D.
> Freelance Open-Source Software Developer
> http://www.open-technology.de/
> --
> To unsubscribe from this list: send the line "unsubscribe linux-media" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 2/4] media i.MX27 camera: add start_stream and stop_stream callbacks.

[Guennadi Liakhovetski]

As discussed before, please, merge this patch with

"media i.MX27 camera: properly detect frame loss."

One more cosmetic note:

On Fri, 20 Jan 2012, Javier Martin wrote:

> Add "start_stream" and "stop_stream" callback in order to enable
> and disable the eMMa-PrP properly and save CPU usage avoiding
> IRQs when the device is not streaming.
> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |  107 +++++++++++++++++++++++++++-----------
>  1 files changed, 77 insertions(+), 30 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 290ac9d..4816da6 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -560,7 +560,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>  	struct soc_camera_host *ici =
>  		to_soc_camera_host(icd->parent);
>  	struct mx2_camera_dev *pcdev = ici->priv;
> -	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>  	struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
>  	unsigned long flags;
>  
> @@ -572,29 +571,7 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>  	buf->state = MX2_STATE_QUEUED;
>  	list_add_tail(&buf->queue, &pcdev->capture);
>  
> -	if (mx27_camera_emma(pcdev)) {
> -		if (prp->cfg.channel == 1) {
> -			writel(PRP_CNTL_CH1EN |
> -				PRP_CNTL_CSIEN |
> -				prp->cfg.in_fmt |
> -				prp->cfg.out_fmt |
> -				PRP_CNTL_CH1_LEN |
> -				PRP_CNTL_CH1BYP |
> -				PRP_CNTL_CH1_TSKIP(0) |
> -				PRP_CNTL_IN_TSKIP(0),
> -				pcdev->base_emma + PRP_CNTL);
> -		} else {
> -			writel(PRP_CNTL_CH2EN |
> -				PRP_CNTL_CSIEN |
> -				prp->cfg.in_fmt |
> -				prp->cfg.out_fmt |
> -				PRP_CNTL_CH2_LEN |
> -				PRP_CNTL_CH2_TSKIP(0) |
> -				PRP_CNTL_IN_TSKIP(0),
> -				pcdev->base_emma + PRP_CNTL);
> -		}
> -		goto out;
> -	} else { /* cpu_is_mx25() */
> +	if (!mx27_camera_emma(pcdev)) { /* cpu_is_mx25() */
>  		u32 csicr3, dma_inten = 0;
>  
>  		if (pcdev->fb1_active == NULL) {
> @@ -629,8 +606,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>  			writel(csicr3, pcdev->base_csi + CSICR3);
>  		}
>  	}
> -
> -out:

To my taste you're a bit too aggressive on blank lines;-) This also holds 
for the previous patch. Unless you absolutely have to edit your sources in 
a 24-line terminal, keeping those empty lines would be appreciated:-)

Thanks
Guennadi

>  	spin_unlock_irqrestore(&pcdev->lock, flags);
>  }
>  
> @@ -692,11 +667,83 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
>  	spin_unlock_irqrestore(&pcdev->lock, flags);
>  }
>  
> +static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
> +{
> +	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
> +	struct soc_camera_host *ici =
> +		to_soc_camera_host(icd->parent);
> +	struct mx2_camera_dev *pcdev = ici->priv;
> +	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +	unsigned long flags;
> +	int ret = 0;
> +
> +	spin_lock_irqsave(&pcdev->lock, flags);
> +	if (mx27_camera_emma(pcdev)) {
> +		if (count < 2) {
> +			ret = -EINVAL;
> +			goto err;
> +		}
> +
> +		if (prp->cfg.channel == 1) {
> +			writel(PRP_CNTL_CH1EN |
> +				PRP_CNTL_CSIEN |
> +				prp->cfg.in_fmt |
> +				prp->cfg.out_fmt |
> +				PRP_CNTL_CH1_LEN |
> +				PRP_CNTL_CH1BYP |
> +				PRP_CNTL_CH1_TSKIP(0) |
> +				PRP_CNTL_IN_TSKIP(0),
> +				pcdev->base_emma + PRP_CNTL);
> +		} else {
> +			writel(PRP_CNTL_CH2EN |
> +				PRP_CNTL_CSIEN |
> +				prp->cfg.in_fmt |
> +				prp->cfg.out_fmt |
> +				PRP_CNTL_CH2_LEN |
> +				PRP_CNTL_CH2_TSKIP(0) |
> +				PRP_CNTL_IN_TSKIP(0),
> +				pcdev->base_emma + PRP_CNTL);
> +		}
> +	}
> +err:
> +	spin_unlock_irqrestore(&pcdev->lock, flags);
> +
> +	return ret;
> +}
> +
> +static int mx2_stop_streaming(struct vb2_queue *q)
> +{
> +	struct soc_camera_device *icd = soc_camera_from_vb2q(q);
> +	struct soc_camera_host *ici =
> +		to_soc_camera_host(icd->parent);
> +	struct mx2_camera_dev *pcdev = ici->priv;
> +	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +	unsigned long flags;
> +	u32 cntl;
> +
> +	spin_lock_irqsave(&pcdev->lock, flags);
> +	if (mx27_camera_emma(pcdev)) {
> +		cntl = readl(pcdev->base_emma + PRP_CNTL);
> +		if (prp->cfg.channel == 1) {
> +			writel(cntl & ~PRP_CNTL_CH1EN,
> +			       pcdev->base_emma + PRP_CNTL);
> +		} else {
> +			writel(cntl & ~PRP_CNTL_CH2EN,
> +			       pcdev->base_emma + PRP_CNTL);
> +		}
> +	}
> +	spin_unlock_irqrestore(&pcdev->lock, flags);
> +
> +	return 0;
> +}
> +
>  static struct vb2_ops mx2_videobuf_ops = {
> -	.queue_setup	= mx2_videobuf_setup,
> -	.buf_prepare	= mx2_videobuf_prepare,
> -	.buf_queue	= mx2_videobuf_queue,
> -	.buf_cleanup	= mx2_videobuf_release,
> +	.queue_setup	 = mx2_videobuf_setup,
> +	.buf_prepare	 = mx2_videobuf_prepare,
> +	.buf_queue	 = mx2_videobuf_queue,
> +	.buf_cleanup	 = mx2_videobuf_release,
> +	.start_streaming = mx2_start_streaming,
> +	.stop_streaming  = mx2_stop_streaming,
>  };
>  
>  static int mx2_camera_init_videobuf(struct vb2_queue *q,
> -- 
> 1.7.0.4
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.

[Guennadi Liakhovetski]

On Fri, 20 Jan 2012, Javier Martin wrote:

> The way discard buffer was previously handled lead
> to possible races that made a buffer that was not
> yet ready to be overwritten by new video data. This
> is easily detected at 25fps just adding "#define DEBUG"
> to enable the "memset" check and seeing how the image
> is corrupted.
> 
> A new "discard" queue and two discard buffers have
> been added to make them flow trough the pipeline
> of queues and thus provide suitable event ordering.

Hmm, do I understand it right, that the problem is, that while the first 
frame went to the discard buffer, the second one went already to a user 
buffer, while it wasn't ready yet? And you solve this by adding one more 
discard buffer? Wouldn't it be possible to either not start capture until 
.start_streaming() is issued, which should also be the case after your 
patch 2/4, or, at least, just reuse one discard buffer multiple times 
until user buffers are available?

If I understand right, you don't really introduce two discard buffers, 
there's still only one data buffer, but you add two discard data objects 
and a list to keep them on. TBH, this seems severely over-engineered to 
me. What's wrong with just keeping one DMA data buffer and using it as 
long, as needed, and checking in your ISR, whether a proper buffer is 
present, by looking for list_empty(active)?

I added a couple of comments below, but my biggest question really is - 
why are these two buffer objects needed? Please, consider getting rid of 
them. So, this is not a full review, if the objects get removed, most of 
this patch will change anyway.

> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |  215 +++++++++++++++++++++-----------------
>  1 files changed, 117 insertions(+), 98 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index 4816da6..e0c5dd4 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
>  	struct mx2_prp_cfg		cfg;
>  };
>  
> +enum mx2_buffer_state {
> +	MX2_STATE_NEEDS_INIT = 0,
> +	MX2_STATE_PREPARED   = 1,
> +	MX2_STATE_QUEUED     = 2,
> +	MX2_STATE_ACTIVE     = 3,
> +	MX2_STATE_DONE       = 4,
> +	MX2_STATE_ERROR      = 5,
> +	MX2_STATE_IDLE       = 6,
> +};
> +
> +/* buffer for one video frame */
> +struct mx2_buffer {
> +	/* common v4l buffer stuff -- must be first */
> +	struct vb2_buffer		vb;
> +	struct list_head		queue;
> +	enum mx2_buffer_state		state;
> +	enum v4l2_mbus_pixelcode	code;
> +
> +	int				bufnum;
> +	bool				discard;
> +};
> +

When submitting a patch series, it is usually good to avoid 
double-patching. E.g., in this case, your first patch adds these enum and 
struct and this patch moves them a couple of lines up. Please, place them 
at the correct location already with the first patch.

>  struct mx2_camera_dev {
>  	struct device		*dev;
>  	struct soc_camera_host	soc_host;
> @@ -240,6 +262,7 @@ struct mx2_camera_dev {
>  
>  	struct list_head	capture;
>  	struct list_head	active_bufs;
> +	struct list_head	discard;
>  
>  	spinlock_t		lock;
>  
> @@ -252,6 +275,7 @@ struct mx2_camera_dev {
>  
>  	u32			csicr1;
>  
> +	struct mx2_buffer	buf_discard[2];
>  	void			*discard_buffer;
>  	dma_addr_t		discard_buffer_dma;
>  	size_t			discard_size;
> @@ -260,27 +284,6 @@ struct mx2_camera_dev {
>  	struct vb2_alloc_ctx	*alloc_ctx;
>  };
>  
> -enum mx2_buffer_state {
> -	MX2_STATE_NEEDS_INIT = 0,
> -	MX2_STATE_PREPARED   = 1,
> -	MX2_STATE_QUEUED     = 2,
> -	MX2_STATE_ACTIVE     = 3,
> -	MX2_STATE_DONE       = 4,
> -	MX2_STATE_ERROR      = 5,
> -	MX2_STATE_IDLE       = 6,
> -};
> -
> -/* buffer for one video frame */
> -struct mx2_buffer {
> -	/* common v4l buffer stuff -- must be first */
> -	struct vb2_buffer		vb;
> -	struct list_head		queue;
> -	enum mx2_buffer_state		state;
> -	enum v4l2_mbus_pixelcode	code;
> -
> -	int bufnum;
> -};
> -
>  static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
>  	/*
>  	 * This is a generic configuration which is valid for most
> @@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
>  	return &mx27_emma_prp_table[0];
>  };
>  
> +static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
> +				 unsigned long phys, int bufnum)
> +{
> +	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;

Are only 1-byte-per-pixel formats supported? Ok, it is only used for 
YUV420, please, move this variable down in that "if."

> +	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +
> +	if (prp->cfg.channel == 1) {
> +		writel(phys, pcdev->base_emma +
> +				PRP_DEST_RGB1_PTR + 4 * bufnum);
> +	} else {
> +		writel(phys, pcdev->base_emma +
> +					PRP_DEST_Y_PTR -
> +					0x14 * bufnum);

Join the above two lines, please.

> +		if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
> +			writel(phys + imgsize,
> +			pcdev->base_emma + PRP_DEST_CB_PTR -
> +			0x14 * bufnum);
> +			writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
> +			PRP_DEST_CR_PTR - 0x14 * bufnum);
> +		}
> +	}
> +}
> +
>  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
>  {
>  	unsigned long flags;
> @@ -382,7 +408,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>  	writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
>  
>  	pcdev->icd = icd;
> -	pcdev->frame_count = -1;
> +	pcdev->frame_count = 0;
>  
>  	dev_info(icd->parent, "Camera driver attached to camera %d\n",
>  		 icd->devnum);
> @@ -648,10 +674,9 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
>  	 * types.
>  	 */
>  	spin_lock_irqsave(&pcdev->lock, flags);
> -	if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
> -		list_del_init(&buf->queue);
> -		buf->state = MX2_STATE_NEEDS_INIT;
> -	} else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
> +	INIT_LIST_HEAD(&buf->queue);

Wouldn't this leave the list, on which this buffer is, corrupted?

> +	buf->state = MX2_STATE_NEEDS_INIT;
> +	if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>  		if (pcdev->fb1_active == buf) {
>  			pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
>  			writel(0, pcdev->base_csi + CSIDMASA_FB1);
> @@ -674,7 +699,10 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>  		to_soc_camera_host(icd->parent);
>  	struct mx2_camera_dev *pcdev = ici->priv;
>  	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> +	struct vb2_buffer *vb;
> +	struct mx2_buffer *buf;
>  	unsigned long flags;
> +	unsigned long phys;
>  	int ret = 0;
>  
>  	spin_lock_irqsave(&pcdev->lock, flags);
> @@ -684,6 +712,26 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>  			goto err;
>  		}
>  
> +		buf = list_entry(pcdev->capture.next,
> +				 struct mx2_buffer, queue);
> +		buf->bufnum = 0;
> +		vb = &buf->vb;
> +		buf->state = MX2_STATE_ACTIVE;
> +
> +		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> +		mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> +		list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> +
> +		buf = list_entry(pcdev->capture.next,
> +				 struct mx2_buffer, queue);
> +		buf->bufnum = 1;
> +		vb = &buf->vb;
> +		buf->state = MX2_STATE_ACTIVE;
> +
> +		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> +		mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> +		list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> +
>  		if (prp->cfg.channel == 1) {
>  			writel(PRP_CNTL_CH1EN |
>  				PRP_CNTL_CSIEN |
> @@ -731,6 +779,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
>  			writel(cntl & ~PRP_CNTL_CH2EN,
>  			       pcdev->base_emma + PRP_CNTL);
>  		}
> +		INIT_LIST_HEAD(&pcdev->capture);
> +		INIT_LIST_HEAD(&pcdev->active_bufs);
> +		INIT_LIST_HEAD(&pcdev->discard);
>  	}
>  	spin_unlock_irqrestore(&pcdev->lock, flags);
>  
> @@ -793,50 +844,21 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
>  		to_soc_camera_host(icd->parent);
>  	struct mx2_camera_dev *pcdev = ici->priv;
>  	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> -	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
>  
> +	writel((icd->user_width << 16) | icd->user_height,
> +	       pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> +	writel(prp->cfg.src_pixel,
> +	       pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
>  	if (prp->cfg.channel == 1) {
> -		writel(pcdev->discard_buffer_dma,
> -				pcdev->base_emma + PRP_DEST_RGB1_PTR);
> -		writel(pcdev->discard_buffer_dma,
> -				pcdev->base_emma + PRP_DEST_RGB2_PTR);
> -
> -		writel((icd->user_width << 16) | icd->user_height,
> -			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>  		writel((icd->user_width << 16) | icd->user_height,
>  			pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
>  		writel(bytesperline,
>  			pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> -		writel(prp->cfg.src_pixel,
> -			pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
>  		writel(prp->cfg.ch1_pixel,
>  			pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
>  	} else { /* channel 2 */
> -		writel(pcdev->discard_buffer_dma,
> -			pcdev->base_emma + PRP_DEST_Y_PTR);
> -		writel(pcdev->discard_buffer_dma,
> -			pcdev->base_emma + PRP_SOURCE_Y_PTR);
> -
> -		if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
> -			writel(pcdev->discard_buffer_dma + imgsize,
> -				pcdev->base_emma + PRP_DEST_CB_PTR);
> -			writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> -				pcdev->base_emma + PRP_DEST_CR_PTR);
> -			writel(pcdev->discard_buffer_dma + imgsize,
> -				pcdev->base_emma + PRP_SOURCE_CB_PTR);
> -			writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> -				pcdev->base_emma + PRP_SOURCE_CR_PTR);
> -		}
> -
> -		writel((icd->user_width << 16) | icd->user_height,
> -			pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> -
>  		writel((icd->user_width << 16) | icd->user_height,
>  			pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
> -
> -		writel(prp->cfg.src_pixel,
> -			pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> -
>  	}
>  
>  	/* Enable interrupts */
> @@ -927,11 +949,22 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
>  		if (ret)
>  			return ret;
>  
> -		if (pcdev->discard_buffer)
> +		if (pcdev->discard_buffer) {
>  			dma_free_coherent(ici->v4l2_dev.dev,
>  				pcdev->discard_size, pcdev->discard_buffer,
>  				pcdev->discard_buffer_dma);
>  
> +			pcdev->buf_discard[0].discard = true;
> +			INIT_LIST_HEAD(&pcdev->buf_discard[0].queue);
> +			list_add_tail(&pcdev->buf_discard[0].queue,
> +				      &pcdev->discard);
> +
> +			pcdev->buf_discard[1].discard = true;
> +			INIT_LIST_HEAD(&pcdev->buf_discard[1].queue);
> +			list_add_tail(&pcdev->buf_discard[1].queue,
> +				      &pcdev->discard);
> +		}
> +

So, you want to do this every time someone does S_FMT?...

>  		/*
>  		 * I didn't manage to properly enable/disable the prp
>  		 * on a per frame basis during running transfers,
> @@ -1193,18 +1226,24 @@ static struct soc_camera_host_ops mx2_soc_camera_host_ops = {
>  static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  		int bufnum, int state)
>  {
> -	u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
> -	struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>  	struct mx2_buffer *buf;
>  	struct vb2_buffer *vb;
>  	unsigned long phys;
>  
> -	if (!list_empty(&pcdev->active_bufs)) {
> -		buf = list_entry(pcdev->active_bufs.next,
> -			struct mx2_buffer, queue);
> +	BUG_ON(list_empty(&pcdev->active_bufs));
> +
> +	buf = list_entry(pcdev->active_bufs.next,
> +			 struct mx2_buffer, queue);
>  
> -		BUG_ON(buf->bufnum != bufnum);
> +	BUG_ON(buf->bufnum != bufnum);
>  
> +	if (buf->discard) {
> +		/*
> +		 * Discard buffer must not be returned to user space.
> +		 * Just return it to the discard queue.
> +		 */
> +		list_move_tail(pcdev->active_bufs.next, &pcdev->discard);
> +	} else {
>  		vb = &buf->vb;
>  #ifdef DEBUG
>  		phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> @@ -1226,6 +1265,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  			}
>  		}
>  #endif
> +
>  		dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%p %lu\n", __func__, vb,
>  				vb2_plane_vaddr(vb, 0),
>  				vb2_get_plane_payload(vb, 0));
> @@ -1237,32 +1277,21 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>  	}
>  
> -	if (list_empty(&pcdev->capture)) {
> -		if (prp->cfg.channel == 1) {
> -			writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> -					PRP_DEST_RGB1_PTR + 4 * bufnum);
> -		} else {
> -			writel(pcdev->discard_buffer_dma, pcdev->base_emma +
> -						PRP_DEST_Y_PTR -
> -						0x14 * bufnum);
> -			if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
> -				writel(pcdev->discard_buffer_dma + imgsize,
> -				       pcdev->base_emma + PRP_DEST_CB_PTR -
> -				       0x14 * bufnum);
> -				writel(pcdev->discard_buffer_dma +
> -				       ((5 * imgsize) / 4), pcdev->base_emma +
> -				       PRP_DEST_CR_PTR - 0x14 * bufnum);
> -			}
> -		}
> +	pcdev->frame_count++;
> +
> +	if (list_empty(&pcdev->capture) && !list_empty(&pcdev->discard)) {
> +		buf = list_entry(pcdev->discard.next,
> +			struct mx2_buffer, queue);
> +		buf->bufnum = bufnum;
> +		list_move_tail(pcdev->discard.next, &pcdev->active_bufs);
> +		mx27_update_emma_buf(pcdev, pcdev->discard_buffer_dma, bufnum);
>  		return;
>  	}
>  
> -	pcdev->frame_count++;
> -
>  	buf = list_entry(pcdev->capture.next,
>  			struct mx2_buffer, queue);
>  
> -	buf->bufnum = !bufnum;
> +	buf->bufnum = bufnum;
>  
>  	list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
>  
> @@ -1270,18 +1299,7 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  	buf->state = MX2_STATE_ACTIVE;
>  
>  	phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> -	if (prp->cfg.channel == 1) {
> -		writel(phys, pcdev->base_emma + PRP_DEST_RGB1_PTR + 4 * bufnum);
> -	} else {
> -		writel(phys, pcdev->base_emma +
> -				PRP_DEST_Y_PTR - 0x14 * bufnum);
> -		if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
> -			writel(phys + imgsize, pcdev->base_emma +
> -					PRP_DEST_CB_PTR - 0x14 * bufnum);
> -			writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
> -					PRP_DEST_CR_PTR - 0x14 * bufnum);
> -		}
> -	}
> +	mx27_update_emma_buf(pcdev, phys, bufnum);
>  }
>  
>  static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
> @@ -1433,6 +1451,7 @@ static int __devinit mx2_camera_probe(struct platform_device *pdev)
>  
>  	INIT_LIST_HEAD(&pcdev->capture);
>  	INIT_LIST_HEAD(&pcdev->active_bufs);
> +	INIT_LIST_HEAD(&pcdev->discard);
>  	spin_lock_init(&pcdev->lock);
>  
>  	/*
> -- 
> 1.7.0.4
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 4/4] media i.MX27 camera: handle overflows properly.

[Guennadi Liakhovetski]

On Fri, 20 Jan 2012, Javier Martin wrote:

> 
> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> ---
>  drivers/media/video/mx2_camera.c |   23 +++++++++--------------
>  1 files changed, 9 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> index e0c5dd4..cdc614f 100644
> --- a/drivers/media/video/mx2_camera.c
> +++ b/drivers/media/video/mx2_camera.c
> @@ -1274,7 +1274,10 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>  		buf->state = state;
>  		do_gettimeofday(&vb->v4l2_buf.timestamp);
>  		vb->v4l2_buf.sequence = pcdev->frame_count;
> -		vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
> +		if (state == MX2_STATE_ERROR)
> +			vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
> +		else
> +			vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>  	}
>  
>  	pcdev->frame_count++;
> @@ -1309,19 +1312,11 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>  	struct mx2_buffer *buf;
>  
>  	if (status & (1 << 7)) { /* overflow */
> -		u32 cntl;
> -		/*
> -		 * We only disable channel 1 here since this is the only
> -		 * enabled channel
> -		 *
> -		 * FIXME: the correct DMA overflow handling should be resetting
> -		 * the buffer, returning an error frame, and continuing with
> -		 * the next one.
> -		 */
> -		cntl = readl(pcdev->base_emma + PRP_CNTL);
> -		writel(cntl & ~(PRP_CNTL_CH1EN | PRP_CNTL_CH2EN),
> -		       pcdev->base_emma + PRP_CNTL);
> -		writel(cntl, pcdev->base_emma + PRP_CNTL);
> +		buf = list_entry(pcdev->active_bufs.next,
> +			struct mx2_buffer, queue);
> +		mx27_camera_frame_done_emma(pcdev,
> +					buf->bufnum, MX2_STATE_ERROR);
> +		status &= ~(1 << 7);
>  	}
>  	if ((((status & (3 << 5)) == (3 << 5)) ||

Does it make sense continuing processing here, if an error occurred? To me 
all the four "if" statements in this function seem mutually-exclusive and 
should be handled by a

	if () {
	} else if () {
	...
chain.

>  		((status & (3 << 3)) == (3 << 3)))
> -- 
> 1.7.0.4
> 

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 1/4] media i.MX27 camera: migrate driver to videobuf2

[javier Martin]

Hi Guennadi,
thank you for your review.

>>       u32                     frame_count;
>> +     struct vb2_alloc_ctx    *alloc_ctx;
>> +};
>> +
>> +enum mx2_buffer_state {
>> +     MX2_STATE_NEEDS_INIT = 0,
>> +     MX2_STATE_PREPARED   = 1,
>> +     MX2_STATE_QUEUED     = 2,
>> +     MX2_STATE_ACTIVE     = 3,
>> +     MX2_STATE_DONE       = 4,
>> +     MX2_STATE_ERROR      = 5,
>> +     MX2_STATE_IDLE       = 6,
>
> Are the numerical values important? If not - please, drop. And actually,
> you don't need most of these states, I wouldn't be surprised, if you
> didn't need them at all. You might want to revise them in a future patch.

Yes, you are right, I might have been too cautious here. I will make
mx27 not to depend on these states and will try to reduce them.
However, those ones used by mx25 I can't eliminate since I don't have
the possibility to test it.

> [snip]
>
>> @@ -467,59 +479,47 @@ static irqreturn_t mx25_camera_irq(int irq_csi, void *data)
>>  /*
>>   *  Videobuf operations
>>   */
>> -static int mx2_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
>> -                           unsigned int *size)
>> +static int mx2_videobuf_setup(struct vb2_queue *vq,
>> +                     const struct v4l2_format *fmt,
>> +                     unsigned int *count, unsigned int *num_planes,
>> +                     unsigned int sizes[], void *alloc_ctxs[])
>>  {
>> -     struct soc_camera_device *icd = vq->priv_data;
>> +     struct soc_camera_device *icd = soc_camera_from_vb2q(vq);
>> +     struct soc_camera_host *ici = to_soc_camera_host(icd->parent);
>> +     struct mx2_camera_dev *pcdev = ici->priv;
>>       int bytes_per_line = soc_mbus_bytes_per_line(icd->user_width,
>>                       icd->current_fmt->host_fmt);
>
> You choose not to support VIDIOC_CREATE_BUFS? You have to at least return
> an error if fmt != NULL. Or consider supporting it - look at mx3_camera.c
> or sh_mobile_ceu_camera.c (btw, atmel-isi.c has to be fixed with this
> respect too). If you decide to support it, you'll also have to drop
> .buf_prepare() (see, e.g., 07f92448045a23d27dbc3ece3abcb6bafc618d43)

I'm a bit tight on schedule and would prefer just returning NULL by
now and add this in a further patch if you are so kind.

> [snip]
>
>> @@ -529,46 +529,34 @@ static int mx2_videobuf_prepare(struct videobuf_queue *vq,
>>        * This can be useful if you want to see if we actually fill
>>        * the buffer with something
>>        */
>> -     memset((void *)vb->baddr, 0xaa, vb->bsize);
>> +     memset((void *)vb2_plane_vaddr(vb, 0),
>> +            0xaa, vb2_get_plane_payload(vb, 0));
>>  #endif
>>
>> -     if (buf->code   != icd->current_fmt->code ||
>> -         vb->width   != icd->user_width ||
>> -         vb->height  != icd->user_height ||
>> -         vb->field   != field) {
>> +     if (buf->code   != icd->current_fmt->code) {
>>               buf->code       = icd->current_fmt->code;
>> -             vb->width       = icd->user_width;
>> -             vb->height      = icd->user_height;
>> -             vb->field       = field;
>> -             vb->state       = VIDEOBUF_NEEDS_INIT;
>> +             buf->state      = MX2_STATE_NEEDS_INIT;
>
> This looks broken or most likely redundant to me. The check for a changed
> code was there to reallocate the buffer, doesn't seem to make much sense
> now.

Yes, this will definitely go away and will take MX2_STATE_NEEDS_INIT with it.

> [snip]
>
>> @@ -686,10 +673,10 @@ static void mx2_videobuf_release(struct videobuf_queue *vq,
>>        * types.
>>        */
>>       spin_lock_irqsave(&pcdev->lock, flags);
>> -     if (vb->state == VIDEOBUF_QUEUED) {
>> -             list_del(&vb->queue);
>> -             vb->state = VIDEOBUF_ERROR;
>> -     } else if (cpu_is_mx25() && vb->state == VIDEOBUF_ACTIVE) {
>> +     if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
>> +             list_del_init(&buf->queue);
>> +             buf->state = MX2_STATE_NEEDS_INIT;
>> +     } else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>
> This doesn't look right. You already have " || buf->state ==
> MX2_STATE_ACTIVE" above, so, this latter condition is never entered?

Yeah, I'm probably breaking m25 support here. This will be fixed in
the following version of my patches.

Regards.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 2/4] media i.MX27 camera: add start_stream and stop_stream callbacks.

[javier Martin]

Hi Guennadi,

On 25 January 2012 11:26, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> As discussed before, please, merge this patch with
>
> "media i.MX27 camera: properly detect frame loss."

Yes. You can drop that patch already.

> One more cosmetic note:
>
> On Fri, 20 Jan 2012, Javier Martin wrote:
>
>> Add "start_stream" and "stop_stream" callback in order to enable
>> and disable the eMMa-PrP properly and save CPU usage avoiding
>> IRQs when the device is not streaming.
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>>  drivers/media/video/mx2_camera.c |  107 +++++++++++++++++++++++++++-----------
>>  1 files changed, 77 insertions(+), 30 deletions(-)
>>
>> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
>> index 290ac9d..4816da6 100644
>> --- a/drivers/media/video/mx2_camera.c
>> +++ b/drivers/media/video/mx2_camera.c
>> @@ -560,7 +560,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>>       struct soc_camera_host *ici =
>>               to_soc_camera_host(icd->parent);
>>       struct mx2_camera_dev *pcdev = ici->priv;
>> -     struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>>       struct mx2_buffer *buf = container_of(vb, struct mx2_buffer, vb);
>>       unsigned long flags;
>>
>> @@ -572,29 +571,7 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>>       buf->state = MX2_STATE_QUEUED;
>>       list_add_tail(&buf->queue, &pcdev->capture);
>>
>> -     if (mx27_camera_emma(pcdev)) {
>> -             if (prp->cfg.channel == 1) {
>> -                     writel(PRP_CNTL_CH1EN |
>> -                             PRP_CNTL_CSIEN |
>> -                             prp->cfg.in_fmt |
>> -                             prp->cfg.out_fmt |
>> -                             PRP_CNTL_CH1_LEN |
>> -                             PRP_CNTL_CH1BYP |
>> -                             PRP_CNTL_CH1_TSKIP(0) |
>> -                             PRP_CNTL_IN_TSKIP(0),
>> -                             pcdev->base_emma + PRP_CNTL);
>> -             } else {
>> -                     writel(PRP_CNTL_CH2EN |
>> -                             PRP_CNTL_CSIEN |
>> -                             prp->cfg.in_fmt |
>> -                             prp->cfg.out_fmt |
>> -                             PRP_CNTL_CH2_LEN |
>> -                             PRP_CNTL_CH2_TSKIP(0) |
>> -                             PRP_CNTL_IN_TSKIP(0),
>> -                             pcdev->base_emma + PRP_CNTL);
>> -             }
>> -             goto out;
>> -     } else { /* cpu_is_mx25() */
>> +     if (!mx27_camera_emma(pcdev)) { /* cpu_is_mx25() */
>>               u32 csicr3, dma_inten = 0;
>>
>>               if (pcdev->fb1_active == NULL) {
>> @@ -629,8 +606,6 @@ static void mx2_videobuf_queue(struct vb2_buffer *vb)
>>                       writel(csicr3, pcdev->base_csi + CSICR3);
>>               }
>>       }
>> -
>> -out:
>
> To my taste you're a bit too aggressive on blank lines;-) This also holds
> for the previous patch. Unless you absolutely have to edit your sources in
> a 24-line terminal, keeping those empty lines would be appreciated:-)

OK. I'll try to overcome my anger ^^

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.

[javier Martin]

Hi Guennadi,

On 25 January 2012 13:12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Fri, 20 Jan 2012, Javier Martin wrote:
>
>> The way discard buffer was previously handled lead
>> to possible races that made a buffer that was not
>> yet ready to be overwritten by new video data. This
>> is easily detected at 25fps just adding "#define DEBUG"
>> to enable the "memset" check and seeing how the image
>> is corrupted.
>>
>> A new "discard" queue and two discard buffers have
>> been added to make them flow trough the pipeline
>> of queues and thus provide suitable event ordering.
>
> Hmm, do I understand it right, that the problem is, that while the first
> frame went to the discard buffer, the second one went already to a user
> buffer, while it wasn't ready yet?

The problem is not only related to the discard buffer but also the way
valid buffers were handled in an unsafe basis.
For instance, the "buf->bufnum = !bufnum" issue. If you receive and
IRQ from bufnum = 0 you have to update buffer 0, not buffer 1.

>And you solve this by adding one more
> discard buffer? Wouldn't it be possible to either not start capture until
> .start_streaming() is issued, which should also be the case after your
> patch 2/4, or, at least, just reuse one discard buffer multiple times
> until user buffers are available?
>
> If I understand right, you don't really introduce two discard buffers,
> there's still only one data buffer, but you add two discard data objects
> and a list to keep them on. TBH, this seems severely over-engineered to
> me. What's wrong with just keeping one DMA data buffer and using it as
> long, as needed, and checking in your ISR, whether a proper buffer is
> present, by looking for list_empty(active)?
>
> I added a couple of comments below, but my biggest question really is -
> why are these two buffer objects needed? Please, consider getting rid of
> them. So, this is not a full review, if the objects get removed, most of
> this patch will change anyway.

1. Why a discard buffer is needed?

When I first took a look at the code it only supported CH1 of the PrP
(i.e. RGB formats) and a discard buffer was used. My first reaction
was trying to get rid of that trick. Both CH1 and CH2 of the PrP have
two pointers that the engine uses to write video frames in a ping-pong
basis. However, there is a big difference between both channels: if
you want to detect frame loss in CH1 you have to continually feed the
two pointers with valid memory areas because the engine is always
writing and you can't stop it, and this is where the discard buffer
function is required, but CH2 has a mechanism to stop capturing and
keep counting loss frames, thus a discard buffer wouldn't be needed.

To sum up: the driver would be much cleaner without this discard
buffer trick but we would have to drop support for CH1 (RGB format).
Being respectful to CH1 support I decided to add CH2 by extending the
discard buffer strategy to both channels, since the code is cleaner
this way and doesn't make sense to manage both channels differently.

2. Why two discard buffer objects are needed?

After enabling the DEBUG functionality that writes the buffers with
0xaa before they are filled with video data, I discovered some of them
were being corrupted. When I tried to find the reason I found that we
really have a pipeline here:

-------------------               -----------------------
  capture (n) | ------------>  active_bufs (2)|
-------------------              ------------------------

where
"capture" has buffers that are queued and ready to be written into
"active_bufs" represents those buffers that are assigned to a pointer
in the PrP and has a maximum of 2 since there are two pointers that
are written in a ping-pong fashion

However, with the previous approach, the discard buffer is kept
outside this pipeline as if it was a global variable which is usually
a dangerous practice and definitely it's wrong since the buffers are
corrupted.


On the other hand, if we add 2 discard buffer objects we will be able
to pass them through the pipeline as if they were normal buffers. We
chose 2 because this is the number of pointers of the PrP and thus,
the maximum of elements that can be in "active_bufs" queue (i.e. we
have to cover the case where both pointers are assigned discard
buffers). This has several advantages:
 - They can be treated in most cases as normal buffers which saves
code ("mx27_update_emma_buf" function).
 - The events are properly ordered: every time an IRQ comes you know
the first element in active_bufs queue is the buffer you want, if it
is not the case something has gone terribly wrong.
 - It's much easier to demonstrate mathematically that this will work
(I wasn't able with the previous approach).


>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>>  drivers/media/video/mx2_camera.c |  215 +++++++++++++++++++++-----------------
>>  1 files changed, 117 insertions(+), 98 deletions(-)
>>
>> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
>> index 4816da6..e0c5dd4 100644
>> --- a/drivers/media/video/mx2_camera.c
>> +++ b/drivers/media/video/mx2_camera.c
>> @@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
>>       struct mx2_prp_cfg              cfg;
>>  };
>>
>> +enum mx2_buffer_state {
>> +     MX2_STATE_NEEDS_INIT = 0,
>> +     MX2_STATE_PREPARED   = 1,
>> +     MX2_STATE_QUEUED     = 2,
>> +     MX2_STATE_ACTIVE     = 3,
>> +     MX2_STATE_DONE       = 4,
>> +     MX2_STATE_ERROR      = 5,
>> +     MX2_STATE_IDLE       = 6,
>> +};
>> +
>> +/* buffer for one video frame */
>> +struct mx2_buffer {
>> +     /* common v4l buffer stuff -- must be first */
>> +     struct vb2_buffer               vb;
>> +     struct list_head                queue;
>> +     enum mx2_buffer_state           state;
>> +     enum v4l2_mbus_pixelcode        code;
>> +
>> +     int                             bufnum;
>> +     bool                            discard;
>> +};
>> +
>
> When submitting a patch series, it is usually good to avoid
> double-patching. E.g., in this case, your first patch adds these enum and
> struct and this patch moves them a couple of lines up. Please, place them
> at the correct location already with the first patch.

OK, I'll fix it in the next version.

>>  struct mx2_camera_dev {
>>       struct device           *dev;
>>       struct soc_camera_host  soc_host;
>> @@ -240,6 +262,7 @@ struct mx2_camera_dev {
>>
>>       struct list_head        capture;
>>       struct list_head        active_bufs;
>> +     struct list_head        discard;
>>
>>       spinlock_t              lock;
>>
>> @@ -252,6 +275,7 @@ struct mx2_camera_dev {
>>
>>       u32                     csicr1;
>>
>> +     struct mx2_buffer       buf_discard[2];
>>       void                    *discard_buffer;
>>       dma_addr_t              discard_buffer_dma;
>>       size_t                  discard_size;
>> @@ -260,27 +284,6 @@ struct mx2_camera_dev {
>>       struct vb2_alloc_ctx    *alloc_ctx;
>>  };
>>
>> -enum mx2_buffer_state {
>> -     MX2_STATE_NEEDS_INIT = 0,
>> -     MX2_STATE_PREPARED   = 1,
>> -     MX2_STATE_QUEUED     = 2,
>> -     MX2_STATE_ACTIVE     = 3,
>> -     MX2_STATE_DONE       = 4,
>> -     MX2_STATE_ERROR      = 5,
>> -     MX2_STATE_IDLE       = 6,
>> -};
>> -
>> -/* buffer for one video frame */
>> -struct mx2_buffer {
>> -     /* common v4l buffer stuff -- must be first */
>> -     struct vb2_buffer               vb;
>> -     struct list_head                queue;
>> -     enum mx2_buffer_state           state;
>> -     enum v4l2_mbus_pixelcode        code;
>> -
>> -     int bufnum;
>> -};
>> -
>>  static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
>>       /*
>>        * This is a generic configuration which is valid for most
>> @@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
>>       return &mx27_emma_prp_table[0];
>>  };
>>
>> +static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
>> +                              unsigned long phys, int bufnum)
>> +{
>> +     u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
>
> Are only 1-byte-per-pixel formats supported? Ok, it is only used for
> YUV420, please, move this variable down in that "if".
>> +     struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>> +
>> +     if (prp->cfg.channel == 1) {
>> +             writel(phys, pcdev->base_emma +
>> +                             PRP_DEST_RGB1_PTR + 4 * bufnum);
>> +     } else {
>> +             writel(phys, pcdev->base_emma +
>> +                                     PRP_DEST_Y_PTR -
>> +                                     0x14 * bufnum);
>
> Join the above two lines, please.
>
>> +             if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
>> +                     writel(phys + imgsize,
>> +                     pcdev->base_emma + PRP_DEST_CB_PTR -
>> +                     0x14 * bufnum);
>> +                     writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
>> +                     PRP_DEST_CR_PTR - 0x14 * bufnum);
>> +             }
>> +     }
>> +}
>> +
>>  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
>>  {
>>       unsigned long flags;
>> @@ -382,7 +408,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
>>       writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
>>
>>       pcdev->icd = icd;
>> -     pcdev->frame_count = -1;
>> +     pcdev->frame_count = 0;
>>
>>       dev_info(icd->parent, "Camera driver attached to camera %d\n",
>>                icd->devnum);
>> @@ -648,10 +674,9 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
>>        * types.
>>        */
>>       spin_lock_irqsave(&pcdev->lock, flags);
>> -     if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
>> -             list_del_init(&buf->queue);
>> -             buf->state = MX2_STATE_NEEDS_INIT;
>> -     } else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>> +     INIT_LIST_HEAD(&buf->queue);
>
> Wouldn't this leave the list, on which this buffer is, corrupted?

By the time this is called, the queues have already been initialized
(stream_stop).

>> +     buf->state = MX2_STATE_NEEDS_INIT;
>> +     if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
>>               if (pcdev->fb1_active == buf) {
>>                       pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
>>                       writel(0, pcdev->base_csi + CSIDMASA_FB1);
>> @@ -674,7 +699,10 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>>               to_soc_camera_host(icd->parent);
>>       struct mx2_camera_dev *pcdev = ici->priv;
>>       struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>> +     struct vb2_buffer *vb;
>> +     struct mx2_buffer *buf;
>>       unsigned long flags;
>> +     unsigned long phys;
>>       int ret = 0;
>>
>>       spin_lock_irqsave(&pcdev->lock, flags);
>> @@ -684,6 +712,26 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
>>                       goto err;
>>               }
>>
>> +             buf = list_entry(pcdev->capture.next,
>> +                              struct mx2_buffer, queue);
>> +             buf->bufnum = 0;
>> +             vb = &buf->vb;
>> +             buf->state = MX2_STATE_ACTIVE;
>> +
>> +             phys = vb2_dma_contig_plane_dma_addr(vb, 0);
>> +             mx27_update_emma_buf(pcdev, phys, buf->bufnum);
>> +             list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
>> +
>> +             buf = list_entry(pcdev->capture.next,
>> +                              struct mx2_buffer, queue);
>> +             buf->bufnum = 1;
>> +             vb = &buf->vb;
>> +             buf->state = MX2_STATE_ACTIVE;
>> +
>> +             phys = vb2_dma_contig_plane_dma_addr(vb, 0);
>> +             mx27_update_emma_buf(pcdev, phys, buf->bufnum);
>> +             list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
>> +
>>               if (prp->cfg.channel == 1) {
>>                       writel(PRP_CNTL_CH1EN |
>>                               PRP_CNTL_CSIEN |
>> @@ -731,6 +779,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
>>                       writel(cntl & ~PRP_CNTL_CH2EN,
>>                              pcdev->base_emma + PRP_CNTL);
>>               }
>> +             INIT_LIST_HEAD(&pcdev->capture);
>> +             INIT_LIST_HEAD(&pcdev->active_bufs);
>> +             INIT_LIST_HEAD(&pcdev->discard);
>>       }
>>       spin_unlock_irqrestore(&pcdev->lock, flags);
>>
>> @@ -793,50 +844,21 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
>>               to_soc_camera_host(icd->parent);
>>       struct mx2_camera_dev *pcdev = ici->priv;
>>       struct mx2_fmt_cfg *prp = pcdev->emma_prp;
>> -     u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
>>
>> +     writel((icd->user_width << 16) | icd->user_height,
>> +            pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>> +     writel(prp->cfg.src_pixel,
>> +            pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
>>       if (prp->cfg.channel == 1) {
>> -             writel(pcdev->discard_buffer_dma,
>> -                             pcdev->base_emma + PRP_DEST_RGB1_PTR);
>> -             writel(pcdev->discard_buffer_dma,
>> -                             pcdev->base_emma + PRP_DEST_RGB2_PTR);
>> -
>> -             writel((icd->user_width << 16) | icd->user_height,
>> -                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>>               writel((icd->user_width << 16) | icd->user_height,
>>                       pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
>>               writel(bytesperline,
>>                       pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
>> -             writel(prp->cfg.src_pixel,
>> -                     pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
>>               writel(prp->cfg.ch1_pixel,
>>                       pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
>>       } else { /* channel 2 */
>> -             writel(pcdev->discard_buffer_dma,
>> -                     pcdev->base_emma + PRP_DEST_Y_PTR);
>> -             writel(pcdev->discard_buffer_dma,
>> -                     pcdev->base_emma + PRP_SOURCE_Y_PTR);
>> -
>> -             if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
>> -                     writel(pcdev->discard_buffer_dma + imgsize,
>> -                             pcdev->base_emma + PRP_DEST_CB_PTR);
>> -                     writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
>> -                             pcdev->base_emma + PRP_DEST_CR_PTR);
>> -                     writel(pcdev->discard_buffer_dma + imgsize,
>> -                             pcdev->base_emma + PRP_SOURCE_CB_PTR);
>> -                     writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
>> -                             pcdev->base_emma + PRP_SOURCE_CR_PTR);
>> -             }
>> -
>> -             writel((icd->user_width << 16) | icd->user_height,
>> -                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
>> -
>>               writel((icd->user_width << 16) | icd->user_height,
>>                       pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
>> -
>> -             writel(prp->cfg.src_pixel,
>> -                     pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
>> -
>>       }
>>
>>       /* Enable interrupts */
>> @@ -927,11 +949,22 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
>>               if (ret)
>>                       return ret;
>>
>> -             if (pcdev->discard_buffer)
>> +             if (pcdev->discard_buffer) {
>>                       dma_free_coherent(ici->v4l2_dev.dev,
>>                               pcdev->discard_size, pcdev->discard_buffer,
>>                               pcdev->discard_buffer_dma);
>>
>> +                     pcdev->buf_discard[0].discard = true;
>> +                     INIT_LIST_HEAD(&pcdev->buf_discard[0].queue);
>> +                     list_add_tail(&pcdev->buf_discard[0].queue,
>> +                                   &pcdev->discard);
>> +
>> +                     pcdev->buf_discard[1].discard = true;
>> +                     INIT_LIST_HEAD(&pcdev->buf_discard[1].queue);
>> +                     list_add_tail(&pcdev->buf_discard[1].queue,
>> +                                   &pcdev->discard);
>> +             }
>> +
>
> So, you want to do this every time someone does S_FMT?...

Not really, good you noticed, let me fix it for v2.

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 4/4] media i.MX27 camera: handle overflows properly.

[javier Martin]

Hi Guennadi,

On 25 January 2012 13:17, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> On Fri, 20 Jan 2012, Javier Martin wrote:
>
>>
>> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
>> ---
>>  drivers/media/video/mx2_camera.c |   23 +++++++++--------------
>>  1 files changed, 9 insertions(+), 14 deletions(-)
>>
>> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
>> index e0c5dd4..cdc614f 100644
>> --- a/drivers/media/video/mx2_camera.c
>> +++ b/drivers/media/video/mx2_camera.c
>> @@ -1274,7 +1274,10 @@ static void mx27_camera_frame_done_emma(struct mx2_camera_dev *pcdev,
>>               buf->state = state;
>>               do_gettimeofday(&vb->v4l2_buf.timestamp);
>>               vb->v4l2_buf.sequence = pcdev->frame_count;
>> -             vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>> +             if (state == MX2_STATE_ERROR)
>> +                     vb2_buffer_done(vb, VB2_BUF_STATE_ERROR);
>> +             else
>> +                     vb2_buffer_done(vb, VB2_BUF_STATE_DONE);
>>       }
>>
>>       pcdev->frame_count++;
>> @@ -1309,19 +1312,11 @@ static irqreturn_t mx27_camera_emma_irq(int irq_emma, void *data)
>>       struct mx2_buffer *buf;
>>
>>       if (status & (1 << 7)) { /* overflow */
>> -             u32 cntl;
>> -             /*
>> -              * We only disable channel 1 here since this is the only
>> -              * enabled channel
>> -              *
>> -              * FIXME: the correct DMA overflow handling should be resetting
>> -              * the buffer, returning an error frame, and continuing with
>> -              * the next one.
>> -              */
>> -             cntl = readl(pcdev->base_emma + PRP_CNTL);
>> -             writel(cntl & ~(PRP_CNTL_CH1EN | PRP_CNTL_CH2EN),
>> -                    pcdev->base_emma + PRP_CNTL);
>> -             writel(cntl, pcdev->base_emma + PRP_CNTL);
>> +             buf = list_entry(pcdev->active_bufs.next,
>> +                     struct mx2_buffer, queue);
>> +             mx27_camera_frame_done_emma(pcdev,
>> +                                     buf->bufnum, MX2_STATE_ERROR);
>> +             status &= ~(1 << 7);
>>       }
>>       if ((((status & (3 << 5)) == (3 << 5)) ||
>
> Does it make sense continuing processing here, if an error occurred? To me
> all the four "if" statements in this function seem mutually-exclusive and
> should be handled by a
>
>        if () {
>        } else if () {
>        ...
> chain.
>
>>               ((status & (3 << 3)) == (3 << 3)))

Yes, as you point out, everything is mutually exclusive.

Regards.
-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com

Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.

[Guennadi Liakhovetski]

(removing baruch@tkos.co.il - it bounces)

On Thu, 26 Jan 2012, javier Martin wrote:

> Hi Guennadi,
> 
> On 25 January 2012 13:12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> > On Fri, 20 Jan 2012, Javier Martin wrote:
> >
> >> The way discard buffer was previously handled lead
> >> to possible races that made a buffer that was not
> >> yet ready to be overwritten by new video data. This
> >> is easily detected at 25fps just adding "#define DEBUG"
> >> to enable the "memset" check and seeing how the image
> >> is corrupted.
> >>
> >> A new "discard" queue and two discard buffers have
> >> been added to make them flow trough the pipeline
> >> of queues and thus provide suitable event ordering.
> >
> > Hmm, do I understand it right, that the problem is, that while the first
> > frame went to the discard buffer, the second one went already to a user
> > buffer, while it wasn't ready yet?
> 
> The problem is not only related to the discard buffer but also the way
> valid buffers were handled in an unsafe basis.
> For instance, the "buf->bufnum = !bufnum" issue. If you receive and
> IRQ from bufnum = 0 you have to update buffer 0, not buffer 1.
> 
> >And you solve this by adding one more
> > discard buffer? Wouldn't it be possible to either not start capture until
> > .start_streaming() is issued, which should also be the case after your
> > patch 2/4, or, at least, just reuse one discard buffer multiple times
> > until user buffers are available?
> >
> > If I understand right, you don't really introduce two discard buffers,
> > there's still only one data buffer, but you add two discard data objects
> > and a list to keep them on. TBH, this seems severely over-engineered to
> > me. What's wrong with just keeping one DMA data buffer and using it as
> > long, as needed, and checking in your ISR, whether a proper buffer is
> > present, by looking for list_empty(active)?
> >
> > I added a couple of comments below, but my biggest question really is -
> > why are these two buffer objects needed? Please, consider getting rid of
> > them. So, this is not a full review, if the objects get removed, most of
> > this patch will change anyway.
> 
> 1. Why a discard buffer is needed?
> 
> When I first took a look at the code it only supported CH1 of the PrP
> (i.e. RGB formats) and a discard buffer was used. My first reaction
> was trying to get rid of that trick. Both CH1 and CH2 of the PrP have
> two pointers that the engine uses to write video frames in a ping-pong
> basis. However, there is a big difference between both channels: if
> you want to detect frame loss in CH1 you have to continually feed the
> two pointers with valid memory areas because the engine is always
> writing and you can't stop it, and this is where the discard buffer
> function is required, but CH2 has a mechanism to stop capturing and
> keep counting loss frames, thus a discard buffer wouldn't be needed.
> 
> To sum up: the driver would be much cleaner without this discard
> buffer trick but we would have to drop support for CH1 (RGB format).
> Being respectful to CH1 support I decided to add CH2 by extending the
> discard buffer strategy to both channels, since the code is cleaner
> this way and doesn't make sense to manage both channels differently.
> 
> 2. Why two discard buffer objects are needed?
> 
> After enabling the DEBUG functionality that writes the buffers with
> 0xaa before they are filled with video data, I discovered some of them
> were being corrupted. When I tried to find the reason I found that we
> really have a pipeline here:
> 
> -------------------               -----------------------
>   capture (n) | ------------>  active_bufs (2)|
> -------------------              ------------------------
> 
> where
> "capture" has buffers that are queued and ready to be written into
> "active_bufs" represents those buffers that are assigned to a pointer
> in the PrP and has a maximum of 2 since there are two pointers that
> are written in a ping-pong fashion

Ok, I understand what the discard memory is used for and why you need to 
write it twice to the hardware - to those two pointers. And I can kindof 
agree, that using them uniformly with user buffers on the active list 
simplifies handling. I just don't think it's a good idea to keep two 
struct vb2_buffer instances around with no use. Maybe you could use 
something like

struct mx2_buf_internal {
	struct list_head		queue;
	int				bufnum;
	bool				discard;
};

struct mx2_buffer {
	struct vb2_buffer		vb;
	enum mx2_buffer_state		state;
	struct mx2_buf_internal		internal;
};

and only use struct mx2_buf_internal for your discard buffers.

Thanks
Guennadi

> 
> However, with the previous approach, the discard buffer is kept
> outside this pipeline as if it was a global variable which is usually
> a dangerous practice and definitely it's wrong since the buffers are
> corrupted.
> 
> 
> On the other hand, if we add 2 discard buffer objects we will be able
> to pass them through the pipeline as if they were normal buffers. We
> chose 2 because this is the number of pointers of the PrP and thus,
> the maximum of elements that can be in "active_bufs" queue (i.e. we
> have to cover the case where both pointers are assigned discard
> buffers). This has several advantages:
>  - They can be treated in most cases as normal buffers which saves
> code ("mx27_update_emma_buf" function).
>  - The events are properly ordered: every time an IRQ comes you know
> the first element in active_bufs queue is the buffer you want, if it
> is not the case something has gone terribly wrong.
>  - It's much easier to demonstrate mathematically that this will work
> (I wasn't able with the previous approach).
> 
> 
> >>
> >> Signed-off-by: Javier Martin <javier.martin@vista-silicon.com>
> >> ---
> >>  drivers/media/video/mx2_camera.c |  215 +++++++++++++++++++++-----------------
> >>  1 files changed, 117 insertions(+), 98 deletions(-)
> >>
> >> diff --git a/drivers/media/video/mx2_camera.c b/drivers/media/video/mx2_camera.c
> >> index 4816da6..e0c5dd4 100644
> >> --- a/drivers/media/video/mx2_camera.c
> >> +++ b/drivers/media/video/mx2_camera.c
> >> @@ -224,6 +224,28 @@ struct mx2_fmt_cfg {
> >>       struct mx2_prp_cfg              cfg;
> >>  };
> >>
> >> +enum mx2_buffer_state {
> >> +     MX2_STATE_NEEDS_INIT = 0,
> >> +     MX2_STATE_PREPARED   = 1,
> >> +     MX2_STATE_QUEUED     = 2,
> >> +     MX2_STATE_ACTIVE     = 3,
> >> +     MX2_STATE_DONE       = 4,
> >> +     MX2_STATE_ERROR      = 5,
> >> +     MX2_STATE_IDLE       = 6,
> >> +};
> >> +
> >> +/* buffer for one video frame */
> >> +struct mx2_buffer {
> >> +     /* common v4l buffer stuff -- must be first */
> >> +     struct vb2_buffer               vb;
> >> +     struct list_head                queue;
> >> +     enum mx2_buffer_state           state;
> >> +     enum v4l2_mbus_pixelcode        code;
> >> +
> >> +     int                             bufnum;
> >> +     bool                            discard;
> >> +};
> >> +
> >
> > When submitting a patch series, it is usually good to avoid
> > double-patching. E.g., in this case, your first patch adds these enum and
> > struct and this patch moves them a couple of lines up. Please, place them
> > at the correct location already with the first patch.
> 
> OK, I'll fix it in the next version.
> 
> >>  struct mx2_camera_dev {
> >>       struct device           *dev;
> >>       struct soc_camera_host  soc_host;
> >> @@ -240,6 +262,7 @@ struct mx2_camera_dev {
> >>
> >>       struct list_head        capture;
> >>       struct list_head        active_bufs;
> >> +     struct list_head        discard;
> >>
> >>       spinlock_t              lock;
> >>
> >> @@ -252,6 +275,7 @@ struct mx2_camera_dev {
> >>
> >>       u32                     csicr1;
> >>
> >> +     struct mx2_buffer       buf_discard[2];
> >>       void                    *discard_buffer;
> >>       dma_addr_t              discard_buffer_dma;
> >>       size_t                  discard_size;
> >> @@ -260,27 +284,6 @@ struct mx2_camera_dev {
> >>       struct vb2_alloc_ctx    *alloc_ctx;
> >>  };
> >>
> >> -enum mx2_buffer_state {
> >> -     MX2_STATE_NEEDS_INIT = 0,
> >> -     MX2_STATE_PREPARED   = 1,
> >> -     MX2_STATE_QUEUED     = 2,
> >> -     MX2_STATE_ACTIVE     = 3,
> >> -     MX2_STATE_DONE       = 4,
> >> -     MX2_STATE_ERROR      = 5,
> >> -     MX2_STATE_IDLE       = 6,
> >> -};
> >> -
> >> -/* buffer for one video frame */
> >> -struct mx2_buffer {
> >> -     /* common v4l buffer stuff -- must be first */
> >> -     struct vb2_buffer               vb;
> >> -     struct list_head                queue;
> >> -     enum mx2_buffer_state           state;
> >> -     enum v4l2_mbus_pixelcode        code;
> >> -
> >> -     int bufnum;
> >> -};
> >> -
> >>  static struct mx2_fmt_cfg mx27_emma_prp_table[] = {
> >>       /*
> >>        * This is a generic configuration which is valid for most
> >> @@ -334,6 +337,29 @@ static struct mx2_fmt_cfg *mx27_emma_prp_get_format(
> >>       return &mx27_emma_prp_table[0];
> >>  };
> >>
> >> +static void mx27_update_emma_buf(struct mx2_camera_dev *pcdev,
> >> +                              unsigned long phys, int bufnum)
> >> +{
> >> +     u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
> >
> > Are only 1-byte-per-pixel formats supported? Ok, it is only used for
> > YUV420, please, move this variable down in that "if".
> >> +     struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> >> +
> >> +     if (prp->cfg.channel == 1) {
> >> +             writel(phys, pcdev->base_emma +
> >> +                             PRP_DEST_RGB1_PTR + 4 * bufnum);
> >> +     } else {
> >> +             writel(phys, pcdev->base_emma +
> >> +                                     PRP_DEST_Y_PTR -
> >> +                                     0x14 * bufnum);
> >
> > Join the above two lines, please.
> >
> >> +             if (prp->out_fmt == V4L2_PIX_FMT_YUV420) {
> >> +                     writel(phys + imgsize,
> >> +                     pcdev->base_emma + PRP_DEST_CB_PTR -
> >> +                     0x14 * bufnum);
> >> +                     writel(phys + ((5 * imgsize) / 4), pcdev->base_emma +
> >> +                     PRP_DEST_CR_PTR - 0x14 * bufnum);
> >> +             }
> >> +     }
> >> +}
> >> +
> >>  static void mx2_camera_deactivate(struct mx2_camera_dev *pcdev)
> >>  {
> >>       unsigned long flags;
> >> @@ -382,7 +408,7 @@ static int mx2_camera_add_device(struct soc_camera_device *icd)
> >>       writel(pcdev->csicr1, pcdev->base_csi + CSICR1);
> >>
> >>       pcdev->icd = icd;
> >> -     pcdev->frame_count = -1;
> >> +     pcdev->frame_count = 0;
> >>
> >>       dev_info(icd->parent, "Camera driver attached to camera %d\n",
> >>                icd->devnum);
> >> @@ -648,10 +674,9 @@ static void mx2_videobuf_release(struct vb2_buffer *vb)
> >>        * types.
> >>        */
> >>       spin_lock_irqsave(&pcdev->lock, flags);
> >> -     if (buf->state == MX2_STATE_QUEUED || buf->state == MX2_STATE_ACTIVE) {
> >> -             list_del_init(&buf->queue);
> >> -             buf->state = MX2_STATE_NEEDS_INIT;
> >> -     } else if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
> >> +     INIT_LIST_HEAD(&buf->queue);
> >
> > Wouldn't this leave the list, on which this buffer is, corrupted?
> 
> By the time this is called, the queues have already been initialized
> (stream_stop).
> 
> >> +     buf->state = MX2_STATE_NEEDS_INIT;
> >> +     if (cpu_is_mx25() && buf->state == MX2_STATE_ACTIVE) {
> >>               if (pcdev->fb1_active == buf) {
> >>                       pcdev->csicr1 &= ~CSICR1_FB1_DMA_INTEN;
> >>                       writel(0, pcdev->base_csi + CSIDMASA_FB1);
> >> @@ -674,7 +699,10 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
> >>               to_soc_camera_host(icd->parent);
> >>       struct mx2_camera_dev *pcdev = ici->priv;
> >>       struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> >> +     struct vb2_buffer *vb;
> >> +     struct mx2_buffer *buf;
> >>       unsigned long flags;
> >> +     unsigned long phys;
> >>       int ret = 0;
> >>
> >>       spin_lock_irqsave(&pcdev->lock, flags);
> >> @@ -684,6 +712,26 @@ static int mx2_start_streaming(struct vb2_queue *q, unsigned int count)
> >>                       goto err;
> >>               }
> >>
> >> +             buf = list_entry(pcdev->capture.next,
> >> +                              struct mx2_buffer, queue);
> >> +             buf->bufnum = 0;
> >> +             vb = &buf->vb;
> >> +             buf->state = MX2_STATE_ACTIVE;
> >> +
> >> +             phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> >> +             mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> >> +             list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> >> +
> >> +             buf = list_entry(pcdev->capture.next,
> >> +                              struct mx2_buffer, queue);
> >> +             buf->bufnum = 1;
> >> +             vb = &buf->vb;
> >> +             buf->state = MX2_STATE_ACTIVE;
> >> +
> >> +             phys = vb2_dma_contig_plane_dma_addr(vb, 0);
> >> +             mx27_update_emma_buf(pcdev, phys, buf->bufnum);
> >> +             list_move_tail(pcdev->capture.next, &pcdev->active_bufs);
> >> +
> >>               if (prp->cfg.channel == 1) {
> >>                       writel(PRP_CNTL_CH1EN |
> >>                               PRP_CNTL_CSIEN |
> >> @@ -731,6 +779,9 @@ static int mx2_stop_streaming(struct vb2_queue *q)
> >>                       writel(cntl & ~PRP_CNTL_CH2EN,
> >>                              pcdev->base_emma + PRP_CNTL);
> >>               }
> >> +             INIT_LIST_HEAD(&pcdev->capture);
> >> +             INIT_LIST_HEAD(&pcdev->active_bufs);
> >> +             INIT_LIST_HEAD(&pcdev->discard);
> >>       }
> >>       spin_unlock_irqrestore(&pcdev->lock, flags);
> >>
> >> @@ -793,50 +844,21 @@ static void mx27_camera_emma_buf_init(struct soc_camera_device *icd,
> >>               to_soc_camera_host(icd->parent);
> >>       struct mx2_camera_dev *pcdev = ici->priv;
> >>       struct mx2_fmt_cfg *prp = pcdev->emma_prp;
> >> -     u32 imgsize = pcdev->icd->user_height * pcdev->icd->user_width;
> >>
> >> +     writel((icd->user_width << 16) | icd->user_height,
> >> +            pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> >> +     writel(prp->cfg.src_pixel,
> >> +            pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> >>       if (prp->cfg.channel == 1) {
> >> -             writel(pcdev->discard_buffer_dma,
> >> -                             pcdev->base_emma + PRP_DEST_RGB1_PTR);
> >> -             writel(pcdev->discard_buffer_dma,
> >> -                             pcdev->base_emma + PRP_DEST_RGB2_PTR);
> >> -
> >> -             writel((icd->user_width << 16) | icd->user_height,
> >> -                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> >>               writel((icd->user_width << 16) | icd->user_height,
> >>                       pcdev->base_emma + PRP_CH1_OUT_IMAGE_SIZE);
> >>               writel(bytesperline,
> >>                       pcdev->base_emma + PRP_DEST_CH1_LINE_STRIDE);
> >> -             writel(prp->cfg.src_pixel,
> >> -                     pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> >>               writel(prp->cfg.ch1_pixel,
> >>                       pcdev->base_emma + PRP_CH1_PIXEL_FORMAT_CNTL);
> >>       } else { /* channel 2 */
> >> -             writel(pcdev->discard_buffer_dma,
> >> -                     pcdev->base_emma + PRP_DEST_Y_PTR);
> >> -             writel(pcdev->discard_buffer_dma,
> >> -                     pcdev->base_emma + PRP_SOURCE_Y_PTR);
> >> -
> >> -             if (prp->cfg.out_fmt == PRP_CNTL_CH2_OUT_YUV420) {
> >> -                     writel(pcdev->discard_buffer_dma + imgsize,
> >> -                             pcdev->base_emma + PRP_DEST_CB_PTR);
> >> -                     writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> >> -                             pcdev->base_emma + PRP_DEST_CR_PTR);
> >> -                     writel(pcdev->discard_buffer_dma + imgsize,
> >> -                             pcdev->base_emma + PRP_SOURCE_CB_PTR);
> >> -                     writel(pcdev->discard_buffer_dma + ((5 * imgsize) / 4),
> >> -                             pcdev->base_emma + PRP_SOURCE_CR_PTR);
> >> -             }
> >> -
> >> -             writel((icd->user_width << 16) | icd->user_height,
> >> -                     pcdev->base_emma + PRP_SRC_FRAME_SIZE);
> >> -
> >>               writel((icd->user_width << 16) | icd->user_height,
> >>                       pcdev->base_emma + PRP_CH2_OUT_IMAGE_SIZE);
> >> -
> >> -             writel(prp->cfg.src_pixel,
> >> -                     pcdev->base_emma + PRP_SRC_PIXEL_FORMAT_CNTL);
> >> -
> >>       }
> >>
> >>       /* Enable interrupts */
> >> @@ -927,11 +949,22 @@ static int mx2_camera_set_bus_param(struct soc_camera_device *icd,
> >>               if (ret)
> >>                       return ret;
> >>
> >> -             if (pcdev->discard_buffer)
> >> +             if (pcdev->discard_buffer) {
> >>                       dma_free_coherent(ici->v4l2_dev.dev,
> >>                               pcdev->discard_size, pcdev->discard_buffer,
> >>                               pcdev->discard_buffer_dma);
> >>
> >> +                     pcdev->buf_discard[0].discard = true;
> >> +                     INIT_LIST_HEAD(&pcdev->buf_discard[0].queue);
> >> +                     list_add_tail(&pcdev->buf_discard[0].queue,
> >> +                                   &pcdev->discard);
> >> +
> >> +                     pcdev->buf_discard[1].discard = true;
> >> +                     INIT_LIST_HEAD(&pcdev->buf_discard[1].queue);
> >> +                     list_add_tail(&pcdev->buf_discard[1].queue,
> >> +                                   &pcdev->discard);
> >> +             }
> >> +
> >
> > So, you want to do this every time someone does S_FMT?...
> 
> Not really, good you noticed, let me fix it for v2.
> 
> Regards.
> -- 
> Javier Martin
> Vista Silicon S.L.
> CDTUC - FASE C - Oficina S-345
> Avda de los Castros s/n
> 39005- Santander. Cantabria. Spain
> +34 942 25 32 60
> www.vista-silicon.com
> 

---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: [PATCH 3/4] media i.MX27 camera: improve discard buffer handling.

[javier Martin]

Hi Guennadi,

On 27 January 2012 19:02, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
> (removing baruch@tkos.co.il - it bounces)
>
> On Thu, 26 Jan 2012, javier Martin wrote:
>
>> Hi Guennadi,
>>
>> On 25 January 2012 13:12, Guennadi Liakhovetski <g.liakhovetski@gmx.de> wrote:
>> > On Fri, 20 Jan 2012, Javier Martin wrote:
>> >
>> >> The way discard buffer was previously handled lead
>> >> to possible races that made a buffer that was not
>> >> yet ready to be overwritten by new video data. This
>> >> is easily detected at 25fps just adding "#define DEBUG"
>> >> to enable the "memset" check and seeing how the image
>> >> is corrupted.
>> >>
>> >> A new "discard" queue and two discard buffers have
>> >> been added to make them flow trough the pipeline
>> >> of queues and thus provide suitable event ordering.
>> >
>> > Hmm, do I understand it right, that the problem is, that while the first
>> > frame went to the discard buffer, the second one went already to a user
>> > buffer, while it wasn't ready yet?
>>
>> The problem is not only related to the discard buffer but also the way
>> valid buffers were handled in an unsafe basis.
>> For instance, the "buf->bufnum = !bufnum" issue. If you receive and
>> IRQ from bufnum = 0 you have to update buffer 0, not buffer 1.
>>
>> >And you solve this by adding one more
>> > discard buffer? Wouldn't it be possible to either not start capture until
>> > .start_streaming() is issued, which should also be the case after your
>> > patch 2/4, or, at least, just reuse one discard buffer multiple times
>> > until user buffers are available?
>> >
>> > If I understand right, you don't really introduce two discard buffers,
>> > there's still only one data buffer, but you add two discard data objects
>> > and a list to keep them on. TBH, this seems severely over-engineered to
>> > me. What's wrong with just keeping one DMA data buffer and using it as
>> > long, as needed, and checking in your ISR, whether a proper buffer is
>> > present, by looking for list_empty(active)?
>> >
>> > I added a couple of comments below, but my biggest question really is -
>> > why are these two buffer objects needed? Please, consider getting rid of
>> > them. So, this is not a full review, if the objects get removed, most of
>> > this patch will change anyway.
>>
>> 1. Why a discard buffer is needed?
>>
>> When I first took a look at the code it only supported CH1 of the PrP
>> (i.e. RGB formats) and a discard buffer was used. My first reaction
>> was trying to get rid of that trick. Both CH1 and CH2 of the PrP have
>> two pointers that the engine uses to write video frames in a ping-pong
>> basis. However, there is a big difference between both channels: if
>> you want to detect frame loss in CH1 you have to continually feed the
>> two pointers with valid memory areas because the engine is always
>> writing and you can't stop it, and this is where the discard buffer
>> function is required, but CH2 has a mechanism to stop capturing and
>> keep counting loss frames, thus a discard buffer wouldn't be needed.
>>
>> To sum up: the driver would be much cleaner without this discard
>> buffer trick but we would have to drop support for CH1 (RGB format).
>> Being respectful to CH1 support I decided to add CH2 by extending the
>> discard buffer strategy to both channels, since the code is cleaner
>> this way and doesn't make sense to manage both channels differently.
>>
>> 2. Why two discard buffer objects are needed?
>>
>> After enabling the DEBUG functionality that writes the buffers with
>> 0xaa before they are filled with video data, I discovered some of them
>> were being corrupted. When I tried to find the reason I found that we
>> really have a pipeline here:
>>
>> -------------------               -----------------------
>>   capture (n) | ------------>  active_bufs (2)|
>> -------------------              ------------------------
>>
>> where
>> "capture" has buffers that are queued and ready to be written into
>> "active_bufs" represents those buffers that are assigned to a pointer
>> in the PrP and has a maximum of 2 since there are two pointers that
>> are written in a ping-pong fashion
>
> Ok, I understand what the discard memory is used for and why you need to
> write it twice to the hardware - to those two pointers. And I can kindof
> agree, that using them uniformly with user buffers on the active list
> simplifies handling. I just don't think it's a good idea to keep two
> struct vb2_buffer instances around with no use. Maybe you could use
> something like
>
> struct mx2_buf_internal {
>        struct list_head                queue;
>        int                             bufnum;
>        bool                            discard;
> };
>
> struct mx2_buffer {
>        struct vb2_buffer               vb;
>        enum mx2_buffer_state           state;
>        struct mx2_buf_internal         internal;
> };
>
> and only use struct mx2_buf_internal for your discard buffers.

You are right, the approach you suggest is more efficient.
What I purpose is that you accept my following v3 patch series and
allow me to send a further cleanup series with the following changes:

1. Remove "goto out" from "mx2_videobuf_queue".
2. Use "list_first_entry" macro wherever possible.
3. Use new structure for internal buffers.

This would makes things a lot of easier for me and I add issues 1 and
2, which you'll probably appreciate.

-- 
Javier Martin
Vista Silicon S.L.
CDTUC - FASE C - Oficina S-345
Avda de los Castros s/n
39005- Santander. Cantabria. Spain
+34 942 25 32 60
www.vista-silicon.com