[PATCH] pxa_camera: Add support for YUV modes

[PATCH] pxa_camera: Add support for YUV modes

[Mike Rapoport]

This patch adds support for YUV packed and planar capture for pxa_camera.

-- 
Sincerely yours,
Mike.

Signed-off-by: Mike Rapoport <mike@compulab.co.il>

diff -r 0776e4801991 linux/drivers/media/video/pxa_camera.c
--- a/linux/drivers/media/video/pxa_camera.c	Fri Mar 28 14:52:44 2008 -0300
+++ b/linux/drivers/media/video/pxa_camera.c	Tue Apr 01 13:53:47 2008 +0300
@@ -67,9 +67,24 @@

 static DEFINE_MUTEX(camera_lock);

+#define BUFFER_TIMEOUT     msecs_to_jiffies(1000)  /* 0.5 seconds */
+
 /*
  * Structures
  */
+enum pxa_camera_active_dma {
+	DMA_Y = 0x1,
+	DMA_U = 0x2,
+	DMA_V = 0x4,
+};
+
+/* descriptor needed for the PXA DMA engine */
+struct pxa_cam_dma {
+	dma_addr_t		sg_dma;
+	struct pxa_dma_desc	*sg_cpu;
+	size_t			sg_size;
+	int			sglen;
+};

 /* buffer for one video frame */
 struct pxa_buffer {
@@ -78,11 +93,12 @@ struct pxa_buffer {

 	const struct soc_camera_data_format        *fmt;

-	/* our descriptor list needed for the PXA DMA engine */
-	dma_addr_t		sg_dma;
-	struct pxa_dma_desc	*sg_cpu;
-	size_t			sg_size;
+	/* our descriptor lists for Y, U and V channels */
+	struct pxa_cam_dma dmas[3];
+
 	int			inwork;
+
+	enum pxa_camera_active_dma active_dma;
 };

 struct pxa_framebuffer_queue {
@@ -100,7 +116,8 @@ struct pxa_camera_dev {

 	unsigned int		irq;
 	void __iomem		*base;
-	unsigned int		dma_chan_y;
+
+	unsigned int		dma_chans[3];

 	struct pxacamera_platform_data *pdata;
 	struct resource		*res;
@@ -108,6 +125,8 @@ struct pxa_camera_dev {
 	unsigned long		platform_mclk_10khz;

 	struct list_head	capture;
+
+	struct timer_list	timeout;

 	spinlock_t		lock;

@@ -128,7 +147,15 @@ static int pxa_videobuf_setup(struct vid

 	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);

-	*size = icd->width * icd->height * ((icd->current_fmt->depth + 7) >> 3);
+	/* planar capture requires Y, U and V buffers to be page aligned */
+	if (icd->current_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+		*size = PAGE_ALIGN(icd->width * icd->height); /* Y pages */
+		*size += PAGE_ALIGN(icd->width * icd->height / 2); /* U pages */
+		*size += PAGE_ALIGN(icd->width * icd->height / 2); /* V pages */
+	} else {
+		*size = icd->width * icd->height *
+			((icd->current_fmt->depth + 7) >> 3);
+	}

 	if (0 == *count)
 		*count = 32;
@@ -145,6 +172,7 @@ static void free_buffer(struct videobuf_
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb);
+	int i;

 	BUG_ON(in_interrupt());

@@ -157,12 +185,60 @@ static void free_buffer(struct videobuf_
 	videobuf_dma_unmap(vq, dma);
 	videobuf_dma_free(dma);

-	if (buf->sg_cpu)
-		dma_free_coherent(pcdev->dev, buf->sg_size, buf->sg_cpu,
-				  buf->sg_dma);
-	buf->sg_cpu = NULL;
+	for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) {
+		if (buf->dmas[i].sg_cpu)
+			dma_free_coherent(pcdev->dev, buf->dmas[i].sg_size,
+					  buf->dmas[i].sg_cpu,
+					  buf->dmas[i].sg_dma);
+		buf->dmas[i].sg_cpu = NULL;
+	}

 	buf->vb.state = VIDEOBUF_NEEDS_INIT;
+}
+
+static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
+				struct pxa_buffer *buf,
+				struct videobuf_dmabuf *dma, int channel,
+				int sglen, int sg_start, int cibr, int size)
+{
+	struct pxa_cam_dma *pxa_dma = &buf->dmas[channel];
+	int i;
+
+	if (pxa_dma->sg_cpu)
+		dma_free_coherent(pcdev->dev, pxa_dma->sg_size,
+				  pxa_dma->sg_cpu, pxa_dma->sg_dma);
+
+	pxa_dma->sg_size = (sglen + 1) * sizeof(struct pxa_dma_desc);
+	pxa_dma->sg_cpu = dma_alloc_coherent(pcdev->dev, pxa_dma->sg_size,
+					     &pxa_dma->sg_dma, GFP_KERNEL);
+	if (!pxa_dma->sg_cpu)
+		return -ENOMEM;
+
+	pxa_dma->sglen = sglen;
+
+	/* DMA for Y buffers */
+	for (i = 0; i < sglen; i++) {
+		int sg_i = sg_start + i;
+		struct scatterlist *sg = dma->sglist;
+		unsigned int dma_len = sg_dma_len(&sg[sg_i]), xfer_len;
+
+		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
+		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(&sg[sg_i]);
+
+		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
+		xfer_len = (min(dma_len, size) + 7) & ~7;
+
+		pxa_dma->sg_cpu[i].dcmd =
+			DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len;
+		size -= dma_len;
+		pxa_dma->sg_cpu[i].ddadr =
+			pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc);
+	}
+	/* end of Y pages */
+	pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
+	pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
+
+	return 0;
 }

 static int pxa_videobuf_prepare(struct videobuf_queue *vq,
@@ -173,7 +249,8 @@ static int pxa_videobuf_prepare(struct v
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
-	int i, ret;
+	int ret;
+	int sglen_y, sglen_u, sglen_v, sglen_yu, size_y, size_uv;

 	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
 		vb, vb->baddr, vb->bsize);
@@ -218,49 +295,61 @@ static int pxa_videobuf_prepare(struct v
 		if (ret)
 			goto fail;

-		if (buf->sg_cpu)
-			dma_free_coherent(pcdev->dev, buf->sg_size, buf->sg_cpu,
-					  buf->sg_dma);
-
-		buf->sg_size = (dma->sglen + 1) * sizeof(struct pxa_dma_desc);
-		buf->sg_cpu = dma_alloc_coherent(pcdev->dev, buf->sg_size,
-						 &buf->sg_dma, GFP_KERNEL);
-		if (!buf->sg_cpu) {
-			ret = -ENOMEM;
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			/* FIXME the calculations should be more precise */
+			sglen_y = dma->sglen / 2;
+			sglen_u = sglen_v = dma->sglen / 4 + 1;
+			sglen_yu = sglen_y + dma->sglen / 4 + 1;
+			size_y = size / 2;
+			size_uv = size / 4;
+		} else {
+			sglen_y = dma->sglen;
+			size_y = size;
+		}
+
+		ret = pxa_init_dma_channel(pcdev, buf, dma, 0, sglen_y,
+					   0, 0x28, size_y);
+
+		if (ret) {
+			dev_err(pcdev->dev,
+				"DMA initialization for Y/RGB failed\n");
 			goto fail;
 		}

-		dev_dbg(&icd->dev, "nents=%d size: %d sg=0x%p\n",
-			dma->sglen, size, dma->sglist);
-		for (i = 0; i < dma->sglen; i++) {
-			struct scatterlist *sg = dma->sglist;
-			unsigned int dma_len = sg_dma_len(&sg[i]), xfer_len;
-
-			/* CIBR0 */
-			buf->sg_cpu[i].dsadr = pcdev->res->start + 0x28;
-			buf->sg_cpu[i].dtadr = sg_dma_address(&sg[i]);
-			/* PXA270 Developer's Manual 27.4.4.1:
-			 * round up to 8 bytes */
-			xfer_len = (min(dma_len, size) + 7) & ~7;
-			if (xfer_len & 7)
-				dev_err(&icd->dev, "Unaligned buffer: "
-					"dma_len %u, size %u\n", dma_len, size);
-			buf->sg_cpu[i].dcmd = DCMD_FLOWSRC | DCMD_BURST8 |
-				DCMD_INCTRGADDR | xfer_len;
-			size -= dma_len;
-			buf->sg_cpu[i].ddadr = buf->sg_dma + (i + 1) *
-					sizeof(struct pxa_dma_desc);
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, sglen_y,
+						   sglen_y, 0x30, size_uv);
+			if (ret) {
+				dev_err(pcdev->dev,
+					"DMA initialization for U failed\n");
+				goto fail_u;
+			}
+
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, sglen_y,
+						   sglen_yu, 0x38, size_uv);
+			if (ret) {
+				dev_err(pcdev->dev,
+					"DMA initialization for V failed\n");
+				goto fail_v;
+			}
 		}
-		buf->sg_cpu[dma->sglen - 1].ddadr = DDADR_STOP;
-		buf->sg_cpu[dma->sglen - 1].dcmd |= DCMD_ENDIRQEN;

 		vb->state = VIDEOBUF_PREPARED;
 	}

 	buf->inwork = 0;
+	buf->active_dma = DMA_Y;
+	if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P)
+		buf->active_dma |= DMA_U | DMA_V;

 	return 0;

+fail_v:
+	dma_free_coherent(pcdev->dev, buf->dmas[1].sg_size,
+			  buf->dmas[1].sg_cpu, buf->dmas[1].sg_dma);
+fail_u:
+	dma_free_coherent(pcdev->dev, buf->dmas[0].sg_size,
+			  buf->dmas[0].sg_cpu, buf->dmas[0].sg_dma);
 fail:
 	free_buffer(vq, buf);
 out:
@@ -277,8 +366,6 @@ static void pxa_videobuf_queue(struct vi
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
 	struct pxa_buffer *active;
-	struct videobuf_dmabuf *dma = videobuf_to_dma(vb);
-	int nents = dma->sglen;
 	unsigned long flags;

 	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
@@ -292,59 +379,90 @@ static void pxa_videobuf_queue(struct vi

 	if (!active) {
 		CIFR |= CIFR_RESET_F;
-		DDADR(pcdev->dma_chan_y) = buf->sg_dma;
-		DCSR(pcdev->dma_chan_y) = DCSR_RUN;
+		DDADR(pcdev->dma_chans[0]) = buf->dmas[0].sg_dma;
+		DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
+
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			DDADR(pcdev->dma_chans[1]) = buf->dmas[1].sg_dma;
+			DCSR(pcdev->dma_chans[1]) = DCSR_RUN;
+
+			DDADR(pcdev->dma_chans[2]) = buf->dmas[2].sg_dma;
+			DCSR(pcdev->dma_chans[2]) = DCSR_RUN;
+		}
+
 		pcdev->active = buf;
 		CICR0 |= CICR0_ENB;
+		mod_timer(&pcdev->timeout, jiffies + BUFFER_TIMEOUT);
 	} else {
-		struct videobuf_dmabuf *active_dma =
-			videobuf_to_dma(&active->vb);
-		/* Stop DMA engine */
-		DCSR(pcdev->dma_chan_y) = 0;
-
-		/* Add the descriptors we just initialized to the currently
-		 * running chain
-		 */
-		active->sg_cpu[active_dma->sglen - 1].ddadr = buf->sg_dma;
-
-		/* Setup a dummy descriptor with the DMA engines current
-		 * state
-		 */
-		/* CIBR0 */
-		buf->sg_cpu[nents].dsadr = pcdev->res->start + 0x28;
-		buf->sg_cpu[nents].dtadr = DTADR(pcdev->dma_chan_y);
-		buf->sg_cpu[nents].dcmd = DCMD(pcdev->dma_chan_y);
-
-		if (DDADR(pcdev->dma_chan_y) == DDADR_STOP) {
-			/* The DMA engine is on the last descriptor, set the
-			 * next descriptors address to the descriptors
-			 * we just initialized
+		struct pxa_cam_dma *buf_dma;
+		struct pxa_cam_dma *act_dma;
+		int channels = 1;
+		int nents;
+		int i;
+
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P)
+			channels = 3;
+
+		for (i = 0; i < channels; i++) {
+			buf_dma = &buf->dmas[i];
+			act_dma = &active->dmas[0];
+			nents = buf_dma->sglen;
+
+			/* Stop DMA engine */
+			DCSR(pcdev->dma_chans[i]) = 0;
+
+			/* Add the descriptors we just initialized to
+			   the currently running chain */
+			act_dma->sg_cpu[act_dma->sglen - 1].ddadr =
+				buf_dma->sg_dma;
+
+			/* Setup a dummy descriptor with the DMA engines current
+			 * state
 			 */
-			buf->sg_cpu[nents].ddadr = buf->sg_dma;
-		} else {
-			buf->sg_cpu[nents].ddadr = DDADR(pcdev->dma_chan_y);
+			buf_dma->sg_cpu[nents].dsadr =
+				pcdev->res->start + 0x28 + i*8; /* CIBRx */
+			buf_dma->sg_cpu[nents].dtadr =
+				DTADR(pcdev->dma_chans[i]);
+			buf_dma->sg_cpu[nents].dcmd =
+				DCMD(pcdev->dma_chans[i]);
+
+			if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) {
+				/* The DMA engine is on the last
+				   descriptor, set the next descriptors
+				   address to the descriptors we just
+				   initialized */
+				buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma;
+			} else {
+				buf_dma->sg_cpu[nents].ddadr =
+					DDADR(pcdev->dma_chans[i]);
+			}
+
+			/* The next descriptor is the dummy descriptor */
+			DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents *
+				sizeof(struct pxa_dma_desc);
+
+			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
+#ifdef DEBUG
+			if (CISR & CISR_IFO_0) {
+				dev_warn(pcdev->dev, "FIFO overrun\n");
+				for (i = 0; i < channels; i++)
+					DDADR(pcdev->dma_chans[i]) =
+						pcdev->active->dmas[i].sg_dma;
+
+				CICR0 &= ~CICR0_ENB;
+				CIFR |= CIFR_RESET_F;
+				for (i = 0; i < channels; i++)
+					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
+				CICR0 |= CICR0_ENB;
+			} else {
+				for (i = 0; i < channels; i++)
+					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
+			}
+#endif
 		}
-
-		/* The next descriptor is the dummy descriptor */
-		DDADR(pcdev->dma_chan_y) = buf->sg_dma + nents *
-			sizeof(struct pxa_dma_desc);
-
-#ifdef DEBUG
-		if (CISR & CISR_IFO_0) {
-			dev_warn(pcdev->dev, "FIFO overrun\n");
-			DDADR(pcdev->dma_chan_y) = pcdev->active->sg_dma;
-
-			CICR0 &= ~CICR0_ENB;
-			CIFR |= CIFR_RESET_F;
-			DCSR(pcdev->dma_chan_y) = DCSR_RUN;
-			CICR0 |= CICR0_ENB;
-		} else
-#endif
-			DCSR(pcdev->dma_chan_y) = DCSR_RUN;
 	}

 	spin_unlock_irqrestore(&pcdev->lock, flags);
-
 }

 static void pxa_videobuf_release(struct videobuf_queue *vq,
@@ -376,41 +494,10 @@ static void pxa_videobuf_release(struct
 	free_buffer(vq, buf);
 }

-static void pxa_camera_dma_irq_y(int channel, void *data)
-{
-	struct pxa_camera_dev *pcdev = data;
-	struct pxa_buffer *buf;
-	unsigned long flags;
-	unsigned int status;
-	struct videobuf_buffer *vb;
-
-	spin_lock_irqsave(&pcdev->lock, flags);
-
-	status = DCSR(pcdev->dma_chan_y);
-	DCSR(pcdev->dma_chan_y) = status;
-
-	if (status & DCSR_BUSERR) {
-		dev_err(pcdev->dev, "DMA Bus Error IRQ!\n");
-		goto out;
-	}
-
-	if (!(status & DCSR_ENDINTR)) {
-		dev_err(pcdev->dev, "Unknown DMA IRQ source, "
-			"status: 0x%08x\n", status);
-		goto out;
-	}
-
-	if (!pcdev->active) {
-		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
-		goto out;
-	}
-
-	vb = &pcdev->active->vb;
-	buf = container_of(vb, struct pxa_buffer, vb);
-	WARN_ON(buf->inwork || list_empty(&vb->queue));
-	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
-		vb, vb->baddr, vb->bsize);
-
+static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
+			      struct videobuf_buffer *vb,
+			      struct pxa_buffer *buf)
+{
 	/* _init is used to debug races, see comment in pxa_camera_reqbufs() */
 	list_del_init(&vb->queue);
 	vb->state = VIDEOBUF_DONE;
@@ -420,16 +507,76 @@ static void pxa_camera_dma_irq_y(int cha

 	if (list_empty(&pcdev->capture)) {
 		pcdev->active = NULL;
-		DCSR(pcdev->dma_chan_y) = 0;
+		DCSR(pcdev->dma_chans[0]) = 0;
+		DCSR(pcdev->dma_chans[1]) = 0;
+		DCSR(pcdev->dma_chans[2]) = 0;
 		CICR0 &= ~CICR0_ENB;
+		return;
+	}
+
+	pcdev->active = list_entry(pcdev->capture.next,
+				   struct pxa_buffer, vb.queue);
+}
+
+static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
+			       enum pxa_camera_active_dma act_dma)
+{
+	struct pxa_buffer *buf;
+	unsigned long flags;
+	unsigned int status;
+	struct videobuf_buffer *vb;
+
+	spin_lock_irqsave(&pcdev->lock, flags);
+
+	status = DCSR(channel);
+	DCSR(channel) |= DCSR_ENDINTR;
+
+	if (status & DCSR_BUSERR) {
+		dev_err(pcdev->dev, "DMA Bus Error IRQ!\n");
 		goto out;
 	}

-	pcdev->active = list_entry(pcdev->capture.next, struct pxa_buffer,
-				   vb.queue);
+	if (!(status & DCSR_ENDINTR)) {
+		dev_err(pcdev->dev, "Unknown DMA IRQ source, "
+			"status: 0x%08x\n", status);
+		goto out;
+	}
+
+	if (!pcdev->active) {
+		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
+		goto out;
+	}
+
+	vb = &pcdev->active->vb;
+	buf = container_of(vb, struct pxa_buffer, vb);
+	WARN_ON(buf->inwork || list_empty(&vb->queue));
+	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
+		vb, vb->baddr, vb->bsize);
+
+	buf->active_dma &= ~act_dma;
+	if (!buf->active_dma)
+		pxa_camera_wakeup(pcdev, vb, buf);

 out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
+}
+
+static void pxa_camera_dma_irq_y(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_Y);
+}
+
+static void pxa_camera_dma_irq_u(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_U);
+}
+
+static void pxa_camera_dma_irq_v(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_V);
 }

 static struct videobuf_queue_ops pxa_videobuf_ops = {
@@ -525,8 +672,39 @@ static irqreturn_t pxa_camera_irq(int ir
 	dev_dbg(pcdev->dev, "Camera interrupt status 0x%x\n", status);

 	CISR = status;
-
 	return IRQ_HANDLED;
+}
+
+static void pxa_camera_timeout(unsigned long data)
+{
+	struct pxa_camera_dev *pcdev = (struct pxa_camera_dev *)data;
+	struct pxa_buffer *buf;
+	unsigned long flags;
+
+	spin_lock_irqsave(&pcdev->lock, flags);
+	if (pcdev->active) {
+		buf = pcdev->active;
+		list_del_init(&buf->vb.queue);
+		buf->vb.state = VIDEOBUF_ERROR;
+		wake_up(&buf->vb.done);
+		pcdev->active = NULL;
+	}
+
+	while (!list_empty(&pcdev->capture)) {
+		buf = list_entry(pcdev->capture.next,
+				 struct pxa_buffer, vb.queue);
+		list_del_init(&buf->vb.queue);
+		buf->vb.state = VIDEOBUF_ERROR;
+		wake_up(&buf->vb.done);
+		printk("[%p/%d] timeout\n", buf, buf->vb.i);
+	}
+
+	DCSR(pcdev->dma_chans[0]) = 0;
+	DCSR(pcdev->dma_chans[1]) = 0;
+	DCSR(pcdev->dma_chans[2]) = 0;
+	CICR0 &= ~CICR0_ENB;
+
+	spin_unlock_irqrestore(&pcdev->lock, flags);
 }

 /* The following two functions absolutely depend on the fact, that
@@ -572,7 +750,7 @@ static void pxa_camera_remove_device(str
 	/* disable capture, disable interrupts */
 	CICR0 = 0x3ff;
 	/* Stop DMA engine */
-	DCSR(pcdev->dma_chan_y) = 0;
+	DCSR(pcdev->dma_chans[0]) = 0;

 	icd->ops->release(icd);

@@ -625,7 +803,7 @@ static int pxa_camera_set_bus_param(stru
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long dw, bpp, bus_flags, camera_flags, common_flags;
-	u32 cicr0, cicr4 = 0;
+	u32 cicr0, cicr1, cicr4 = 0;
 	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);

 	if (ret < 0)
@@ -702,7 +880,25 @@ static int pxa_camera_set_bus_param(stru
 	cicr0 = CICR0;
 	if (cicr0 & CICR0_ENB)
 		CICR0 = cicr0 & ~CICR0_ENB;
-	CICR1 = CICR1_PPL_VAL(icd->width - 1) | bpp | dw;
+
+	cicr1 = CICR1_PPL_VAL(icd->width - 1) | bpp | dw;
+
+	switch (pixfmt) {
+	case V4L2_PIX_FMT_YUV422P:
+		cicr1 |= CICR1_YCBCR_F;
+	case V4L2_PIX_FMT_YUYV:
+		cicr1 |= CICR1_COLOR_SP_VAL(2);
+		break;
+	case V4L2_PIX_FMT_RGB555:
+		cicr1 |= CICR1_RGB_BPP_VAL(1) | CICR1_RGBT_CONV_VAL(2) |
+			CICR1_TBIT | CICR1_COLOR_SP_VAL(1);
+		break;
+	case V4L2_PIX_FMT_RGB565:
+		cicr1 |= CICR1_COLOR_SP_VAL(1) | CICR1_RGB_BPP_VAL(2);
+		break;
+	}
+
+	CICR1 = cicr1;
 	CICR2 = 0;
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
@@ -871,6 +1067,10 @@ static int pxa_camera_probe(struct platf
 	INIT_LIST_HEAD(&pcdev->capture);
 	spin_lock_init(&pcdev->lock);

+	pcdev->timeout.function = pxa_camera_timeout;
+	pcdev->timeout.data = (unsigned long)pcdev;
+	init_timer(&pcdev->timeout);
+
 	/*
 	 * Request the regions.
 	 */
@@ -890,16 +1090,36 @@ static int pxa_camera_probe(struct platf
 	pcdev->dev = &pdev->dev;

 	/* request dma */
-	pcdev->dma_chan_y = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
-					    pxa_camera_dma_irq_y, pcdev);
-	if (pcdev->dma_chan_y < 0) {
+	pcdev->dma_chans[0] = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_y, pcdev);
+	if (pcdev->dma_chans[0] < 0) {
 		dev_err(pcdev->dev, "Can't request DMA for Y\n");
 		err = -ENOMEM;
 		goto exit_iounmap;
 	}
-	dev_dbg(pcdev->dev, "got DMA channel %d\n", pcdev->dma_chan_y);
-
-	DRCMR68 = pcdev->dma_chan_y  | DRCMR_MAPVLD;
+	dev_dbg(pcdev->dev, "got DMA channel %d\n", pcdev->dma_chans[0]);
+
+	pcdev->dma_chans[1] = pxa_request_dma("CI_U", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_u, pcdev);
+	if (pcdev->dma_chans[1] < 0) {
+		dev_err(pcdev->dev, "Can't request DMA for U\n");
+		err = -ENOMEM;
+		goto exit_free_dma_y;
+	}
+	dev_dbg(pcdev->dev, "got DMA channel (U) %d\n", pcdev->dma_chans[1]);
+
+	pcdev->dma_chans[2] = pxa_request_dma("CI_V", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_v, pcdev);
+	if (pcdev->dma_chans[0] < 0) {
+		dev_err(pcdev->dev, "Can't request DMA for V\n");
+		err = -ENOMEM;
+		goto exit_free_dma_u;
+	}
+	dev_dbg(pcdev->dev, "got DMA channel (V) %d\n", pcdev->dma_chans[2]);
+
+	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
+	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
+	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;

 	/* request irq */
 	err = request_irq(pcdev->irq, pxa_camera_irq, 0, PXA_CAM_DRV_NAME,
@@ -921,7 +1141,11 @@ exit_free_irq:
 exit_free_irq:
 	free_irq(pcdev->irq, pcdev);
 exit_free_dma:
-	pxa_free_dma(pcdev->dma_chan_y);
+	pxa_free_dma(pcdev->dma_chans[2]);
+exit_free_dma_u:
+	pxa_free_dma(pcdev->dma_chans[1]);
+exit_free_dma_y:
+	pxa_free_dma(pcdev->dma_chans[0]);
 exit_iounmap:
 	iounmap(base);
 exit_release:
@@ -941,7 +1165,9 @@ static int __devexit pxa_camera_remove(s

 	clk_put(pcdev->clk);

-	pxa_free_dma(pcdev->dma_chan_y);
+	pxa_free_dma(pcdev->dma_chans[0]);
+	pxa_free_dma(pcdev->dma_chans[1]);
+	pxa_free_dma(pcdev->dma_chans[2]);
 	free_irq(pcdev->irq, pcdev);

 	soc_camera_host_unregister(&pxa_soc_camera_host);

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Guennadi Liakhovetski]

On Tue, 1 Apr 2008, Mike Rapoport wrote:

> This patch adds support for YUV packed and planar capture for pxa_camera.

Great, thanks for the patch! I'll try to look at it this weekend... Have 
you been able to test both AUV and raw modes with your patch or only YUV? 
Are you also planning to submit the camera driver(s) and the platform code 
(for ARMCore I presume)?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Mike Rapoport]

Guennadi Liakhovetski wrote:
> On Tue, 1 Apr 2008, Mike Rapoport wrote:
> 
>> This patch adds support for YUV packed and planar capture for pxa_camera.
> 
> Great, thanks for the patch! I'll try to look at it this weekend... Have 
> you been able to test both AUV and raw modes with your patch or only YUV? 

Only YUV.

> Are you also planning to submit the camera driver(s) and the platform code 
> (for ARMCore I presume)?

The camera driver is currently not in the state it can be submitted. It'll take
some time to make it generic enough.

> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 

-- 
Sincerely yours,
Mike.


--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Guennadi Liakhovetski]

Looks good overall!

First static analysis:

On Tue, 1 Apr 2008, Mike Rapoport wrote:

> @@ -67,9 +67,24 @@
> 
>  static DEFINE_MUTEX(camera_lock);
> 
> +#define BUFFER_TIMEOUT     msecs_to_jiffies(1000)  /* 0.5 seconds */

This doesn't look like 0.5 second to me:-) AFAIU, this timeout is supposed 
to trigger when an application failed to queue new buffers quickly enough. 
I see an init_timer, a mod_timer, but no del_timer(_sync). Don't you have 
to delete the timer at least on rmmod to avoid an Oops? Also on streamoff, 
which ATM wouldn't be possible with the current soc-camera API, because 
streamoff is not passed to the camera-host driver. A good place could be 
in the interrupt handler where you check

	if (list_empty(&pcdev->capture)) {
	}

I would prefer to first leave this timer aside and concentrate on the YUV 
functionality, could we do that? If I am right in that the YUV support 
doesn't require this timer, please, separate it into an additional patch.

> +static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
> +				struct pxa_buffer *buf,
> +				struct videobuf_dmabuf *dma, int channel,
> +				int sglen, int sg_start, int cibr, int size)
> +{
> +	struct pxa_cam_dma *pxa_dma = &buf->dmas[channel];
> +	int i;
> +
> +	if (pxa_dma->sg_cpu)
> +		dma_free_coherent(pcdev->dev, pxa_dma->sg_size,
> +				  pxa_dma->sg_cpu, pxa_dma->sg_dma);
> +
> +	pxa_dma->sg_size = (sglen + 1) * sizeof(struct pxa_dma_desc);
> +	pxa_dma->sg_cpu = dma_alloc_coherent(pcdev->dev, pxa_dma->sg_size,
> +					     &pxa_dma->sg_dma, GFP_KERNEL);
> +	if (!pxa_dma->sg_cpu)
> +		return -ENOMEM;
> +
> +	pxa_dma->sglen = sglen;
> +
> +	/* DMA for Y buffers */

I presume, this comment doesn't hold?

> +	for (i = 0; i < sglen; i++) {
> +		int sg_i = sg_start + i;
> +		struct scatterlist *sg = dma->sglist;
> +		unsigned int dma_len = sg_dma_len(&sg[sg_i]), xfer_len;
> +
> +		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
> +		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(&sg[sg_i]);
> +
> +		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
> +		xfer_len = (min(dma_len, size) + 7) & ~7;
> +
> +		pxa_dma->sg_cpu[i].dcmd =
> +			DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len;
> +		size -= dma_len;
> +		pxa_dma->sg_cpu[i].ddadr =
> +			pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc);
> +	}
> +	/* end of Y pages */

This one too.

> +	pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
> +	pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
> +
> +	return 0;
>  }
> 
>  static int pxa_videobuf_prepare(struct videobuf_queue *vq,
> @@ -173,7 +249,8 @@ static int pxa_videobuf_prepare(struct v
>  		to_soc_camera_host(icd->dev.parent);
>  	struct pxa_camera_dev *pcdev = ici->priv;
>  	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
> -	int i, ret;
> +	int ret;
> +	int sglen_y, sglen_u, sglen_v, sglen_yu, size_y, size_uv;
> 
>  	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
>  		vb, vb->baddr, vb->bsize);
> @@ -218,49 +295,61 @@ static int pxa_videobuf_prepare(struct v
>  		if (ret)
>  			goto fail;
> 
> -		if (buf->sg_cpu)
> -			dma_free_coherent(pcdev->dev, buf->sg_size, buf->sg_cpu,
> -					  buf->sg_dma);
> -
> -		buf->sg_size = (dma->sglen + 1) * sizeof(struct pxa_dma_desc);
> -		buf->sg_cpu = dma_alloc_coherent(pcdev->dev, buf->sg_size,
> -						 &buf->sg_dma, GFP_KERNEL);
> -		if (!buf->sg_cpu) {
> -			ret = -ENOMEM;
> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> +			/* FIXME the calculations should be more precise */
> +			sglen_y = dma->sglen / 2;
> +			sglen_u = sglen_v = dma->sglen / 4 + 1;
> +			sglen_yu = sglen_y + dma->sglen / 4 + 1;

IMHO better readable would be

> +			sglen_yu = sglen_y + sglen_u;


> +			size_y = size / 2;
> +			size_uv = size / 4;

Following your above notation more consistent would be

> +			size_u = size_v = size / 4;

and then use them for the two pxa_init_dma_channel() calls respectively? 
It is confusing because sglen_yu above means (sglen_y + sglen_u).

> +		} else {
> +			sglen_y = dma->sglen;
> +			size_y = size;
> +		}
> +
> +		ret = pxa_init_dma_channel(pcdev, buf, dma, 0, sglen_y,
> +					   0, 0x28, size_y);
> +
> +		if (ret) {
> +			dev_err(pcdev->dev,
> +				"DMA initialization for Y/RGB failed\n");
>  			goto fail;
>  		}
> 
> -		dev_dbg(&icd->dev, "nents=%d size: %d sg=0x%p\n",
> -			dma->sglen, size, dma->sglist);
> -		for (i = 0; i < dma->sglen; i++) {
> -			struct scatterlist *sg = dma->sglist;
> -			unsigned int dma_len = sg_dma_len(&sg[i]), xfer_len;
> -
> -			/* CIBR0 */
> -			buf->sg_cpu[i].dsadr = pcdev->res->start + 0x28;
> -			buf->sg_cpu[i].dtadr = sg_dma_address(&sg[i]);
> -			/* PXA270 Developer's Manual 27.4.4.1:
> -			 * round up to 8 bytes */
> -			xfer_len = (min(dma_len, size) + 7) & ~7;
> -			if (xfer_len & 7)
> -				dev_err(&icd->dev, "Unaligned buffer: "
> -					"dma_len %u, size %u\n", dma_len, size);
> -			buf->sg_cpu[i].dcmd = DCMD_FLOWSRC | DCMD_BURST8 |
> -				DCMD_INCTRGADDR | xfer_len;
> -			size -= dma_len;
> -			buf->sg_cpu[i].ddadr = buf->sg_dma + (i + 1) *
> -					sizeof(struct pxa_dma_desc);
> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, sglen_y,

This should be sglen_u							^^^^^

> +						   sglen_y, 0x30, size_uv);
> +			if (ret) {
> +				dev_err(pcdev->dev,
> +					"DMA initialization for U failed\n");
> +				goto fail_u;
> +			}
> +
> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, sglen_y,

This should be sglen_v							^^^^^

> @@ -292,59 +379,90 @@ static void pxa_videobuf_queue(struct vi
> 
>  	if (!active) {
>  		CIFR |= CIFR_RESET_F;
> -		DDADR(pcdev->dma_chan_y) = buf->sg_dma;
> -		DCSR(pcdev->dma_chan_y) = DCSR_RUN;
> +		DDADR(pcdev->dma_chans[0]) = buf->dmas[0].sg_dma;
> +		DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
> +
> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> +			DDADR(pcdev->dma_chans[1]) = buf->dmas[1].sg_dma;
> +			DCSR(pcdev->dma_chans[1]) = DCSR_RUN;
> +
> +			DDADR(pcdev->dma_chans[2]) = buf->dmas[2].sg_dma;
> +			DCSR(pcdev->dma_chans[2]) = DCSR_RUN;
> +		}
> +
>  		pcdev->active = buf;
>  		CICR0 |= CICR0_ENB;
> +		mod_timer(&pcdev->timeout, jiffies + BUFFER_TIMEOUT);
>  	} else {
> -		struct videobuf_dmabuf *active_dma =
> -			videobuf_to_dma(&active->vb);
> -		/* Stop DMA engine */
> -		DCSR(pcdev->dma_chan_y) = 0;
> -
> -		/* Add the descriptors we just initialized to the currently
> -		 * running chain
> -		 */
> -		active->sg_cpu[active_dma->sglen - 1].ddadr = buf->sg_dma;
> -
> -		/* Setup a dummy descriptor with the DMA engines current
> -		 * state
> -		 */
> -		/* CIBR0 */
> -		buf->sg_cpu[nents].dsadr = pcdev->res->start + 0x28;
> -		buf->sg_cpu[nents].dtadr = DTADR(pcdev->dma_chan_y);
> -		buf->sg_cpu[nents].dcmd = DCMD(pcdev->dma_chan_y);
> -
> -		if (DDADR(pcdev->dma_chan_y) == DDADR_STOP) {
> -			/* The DMA engine is on the last descriptor, set the
> -			 * next descriptors address to the descriptors
> -			 * we just initialized
> +		struct pxa_cam_dma *buf_dma;
> +		struct pxa_cam_dma *act_dma;
> +		int channels = 1;
> +		int nents;
> +		int i;
> +
> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P)
> +			channels = 3;
> +
> +		for (i = 0; i < channels; i++) {
> +			buf_dma = &buf->dmas[i];
> +			act_dma = &active->dmas[0];
> +			nents = buf_dma->sglen;
> +
> +			/* Stop DMA engine */
> +			DCSR(pcdev->dma_chans[i]) = 0;
> +
> +			/* Add the descriptors we just initialized to
> +			   the currently running chain */
> +			act_dma->sg_cpu[act_dma->sglen - 1].ddadr =
> +				buf_dma->sg_dma;
> +
> +			/* Setup a dummy descriptor with the DMA engines current
> +			 * state
>  			 */
> -			buf->sg_cpu[nents].ddadr = buf->sg_dma;
> -		} else {
> -			buf->sg_cpu[nents].ddadr = DDADR(pcdev->dma_chan_y);
> +			buf_dma->sg_cpu[nents].dsadr =
> +				pcdev->res->start + 0x28 + i*8; /* CIBRx */
> +			buf_dma->sg_cpu[nents].dtadr =
> +				DTADR(pcdev->dma_chans[i]);
> +			buf_dma->sg_cpu[nents].dcmd =
> +				DCMD(pcdev->dma_chans[i]);
> +
> +			if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) {
> +				/* The DMA engine is on the last
> +				   descriptor, set the next descriptors
> +				   address to the descriptors we just
> +				   initialized */
> +				buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma;
> +			} else {
> +				buf_dma->sg_cpu[nents].ddadr =
> +					DDADR(pcdev->dma_chans[i]);
> +			}
> +
> +			/* The next descriptor is the dummy descriptor */
> +			DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents *
> +				sizeof(struct pxa_dma_desc);
> +
> +			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
> +#ifdef DEBUG
> +			if (CISR & CISR_IFO_0) {
> +				dev_warn(pcdev->dev, "FIFO overrun\n");
> +				for (i = 0; i < channels; i++)
> +					DDADR(pcdev->dma_chans[i]) =
> +						pcdev->active->dmas[i].sg_dma;
> +
> +				CICR0 &= ~CICR0_ENB;
> +				CIFR |= CIFR_RESET_F;
> +				for (i = 0; i < channels; i++)
> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
> +				CICR0 |= CICR0_ENB;
> +			} else {
> +				for (i = 0; i < channels; i++)
> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
> +			}

These three loops don't look right. At least because they use the same 
index i. And you're iterating over channels inside a loop over channels, 
and you have dma_chans[0] instead of [i]. Please fix.

> +static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
> +			       enum pxa_camera_active_dma act_dma)
> +{
> +	struct pxa_buffer *buf;
> +	unsigned long flags;
> +	unsigned int status;
> +	struct videobuf_buffer *vb;
> +
> +	spin_lock_irqsave(&pcdev->lock, flags);
> +
> +	status = DCSR(channel);
> +	DCSR(channel) |= DCSR_ENDINTR;

better

> +	DCSR(channel) = status | DCSR_ENDINTR;

to avoid clearing new interrupts.

> @@ -572,7 +750,7 @@ static void pxa_camera_remove_device(str
>  	/* disable capture, disable interrupts */
>  	CICR0 = 0x3ff;
>  	/* Stop DMA engine */
> -	DCSR(pcdev->dma_chan_y) = 0;
> +	DCSR(pcdev->dma_chans[0]) = 0;

Disable the other two channels too?

Ok, now compile-time, applied on the top of v4l-dvb/develop:

drivers/media/video/pxa_camera.c: In function 'pxa_init_dma_channel':
drivers/media/video/pxa_camera.c:229: warning: comparison of distinct pointer types lacks a cast
drivers/media/video/pxa_camera.c: In function 'pxa_camera_set_bus_param':
drivers/media/video/pxa_camera.c:890: error: implicit declaration of function 'CICR1_COLOR_SP_VAL'
drivers/media/video/pxa_camera.c:893: error: implicit declaration of function 'CICR1_RGB_BPP_VAL'
drivers/media/video/pxa_camera.c:893: error: implicit declaration of function 'CICR1_RGBT_CONV_VAL'

I converted parameters of pxa_init_dma_channel() to unsigned int and added 
the missing defines. Please, do the same in your next version. Then I 
still got

drivers/media/video/pxa_camera.c: In function 'pxa_videobuf_prepare':
drivers/media/video/pxa_camera.c:257: warning: 'size_uv' may be used uninitialized in this function
drivers/media/video/pxa_camera.c:257: warning: 'sglen_yu' may be used uninitialized in this function

which also have to be fixed.

Run-time: positive. My 8 bit raw modes still worked. Stopping mplayer and 
immediately rmmod-ing pxa-camera locked up the system, as expected.

So, please fix these issues and we'll try to get it in!

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Mike Rapoport]

Guennadi Liakhovetski wrote:
> Looks good overall!
> 
> First static analysis:
> 
> On Tue, 1 Apr 2008, Mike Rapoport wrote:
> 
> I would prefer to first leave this timer aside and concentrate on the YUV 
> functionality, could we do that? If I am right in that the YUV support 
> doesn't require this timer, please, separate it into an additional patch.

I've removed the timer for now.

>> +	pxa_dma->sglen = sglen;
>> +
>> +	/* DMA for Y buffers */
> 
> I presume, this comment doesn't hold?

Fixed.

>> +	}
>> +	/* end of Y pages */
> 
> This one too.

Fixed.

>> +			sglen_y = dma->sglen / 2;
>> +			sglen_u = sglen_v = dma->sglen / 4 + 1;
>> +			sglen_yu = sglen_y + dma->sglen / 4 + 1;
> 
> IMHO better readable would be
> 
>> +			sglen_yu = sglen_y + sglen_u;
> 
> 
>> +			size_y = size / 2;
>> +			size_uv = size / 4;
> 
> Following your above notation more consistent would be
> 
>> +			size_u = size_v = size / 4;
> 
> and then use them for the two pxa_init_dma_channel() calls respectively? 
> It is confusing because sglen_yu above means (sglen_y + sglen_u).

Completely agree.

>> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
>> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, sglen_y,
> 
> This should be sglen_u							^^^^^

Right. No wonder I had colors distorted :)

> 
>> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, sglen_y,
> 
> This should be sglen_v							^^^^^

Fixed.

>> +			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
>> +#ifdef DEBUG
>> +			if (CISR & CISR_IFO_0) {
>> +				dev_warn(pcdev->dev, "FIFO overrun\n");
>> +				for (i = 0; i < channels; i++)
>> +					DDADR(pcdev->dma_chans[i]) =
>> +						pcdev->active->dmas[i].sg_dma;
>> +
>> +				CICR0 &= ~CICR0_ENB;
>> +				CIFR |= CIFR_RESET_F;
>> +				for (i = 0; i < channels; i++)
>> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
>> +				CICR0 |= CICR0_ENB;
>> +			} else {
>> +				for (i = 0; i < channels; i++)
>> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
>> +			}
> 
> These three loops don't look right. At least because they use the same 
> index i. And you're iterating over channels inside a loop over channels, 
> and you have dma_chans[0] instead of [i]. Please fix.

Here I'm not quite sure what exactly should be done as I never got overruns.
For now I move this code out of the loop and in case of overrun re-enable
all three DMAs. BTW, the 'else' here is completely redundant, so I just removed it.

>> +	status = DCSR(channel);
>> +	DCSR(channel) |= DCSR_ENDINTR;
> 
> better

Fixed.

>> +	DCSR(channel) = status | DCSR_ENDINTR;
> 
> to avoid clearing new interrupts.
> 
>> @@ -572,7 +750,7 @@ static void pxa_camera_remove_device(str
>>  	/* disable capture, disable interrupts */
>>  	CICR0 = 0x3ff;
>>  	/* Stop DMA engine */
>> -	DCSR(pcdev->dma_chan_y) = 0;
>> +	DCSR(pcdev->dma_chans[0]) = 0;
> 
> Disable the other two channels too?

Fixed.

> Ok, now compile-time, applied on the top of v4l-dvb/develop:
> 
> drivers/media/video/pxa_camera.c: In function 'pxa_init_dma_channel':
> drivers/media/video/pxa_camera.c:229: warning: comparison of distinct pointer types lacks a cast
> drivers/media/video/pxa_camera.c: In function 'pxa_camera_set_bus_param':
> drivers/media/video/pxa_camera.c:890: error: implicit declaration of function 'CICR1_COLOR_SP_VAL'
> drivers/media/video/pxa_camera.c:893: error: implicit declaration of function 'CICR1_RGB_BPP_VAL'
> drivers/media/video/pxa_camera.c:893: error: implicit declaration of function 'CICR1_RGBT_CONV_VAL'
> 
> I converted parameters of pxa_init_dma_channel() to unsigned int and added 
> the missing defines. Please, do the same in your next version. Then I 
> still got
> 
> drivers/media/video/pxa_camera.c: In function 'pxa_videobuf_prepare':
> drivers/media/video/pxa_camera.c:257: warning: 'size_uv' may be used uninitialized in this function
> drivers/media/video/pxa_camera.c:257: warning: 'sglen_yu' may be used uninitialized in this function
> 
> which also have to be fixed.

Both are fixed.

> Run-time: positive. My 8 bit raw modes still worked. Stopping mplayer and 
> immediately rmmod-ing pxa-camera locked up the system, as expected.
> 
> So, please fix these issues and we'll try to get it in!
> 
> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 

-- 
Sincerely yours,
Mike.


Signed-off-by: Mike Rapoport <mike@compulab.co.il>

diff --git a/linux/drivers/media/video/pxa_camera.c b/linux/drivers/media/video/pxa_camera.c
--- a/linux/drivers/media/video/pxa_camera.c
+++ b/linux/drivers/media/video/pxa_camera.c
@@ -49,6 +49,9 @@

 #define CICR1_DW_VAL(x)   ((x) & CICR1_DW)	    /* Data bus width */
 #define CICR1_PPL_VAL(x)  (((x) << 15) & CICR1_PPL) /* Pixels per line */
+#define CICR1_COLOR_SP_VAL(x)	(((x) << 3) & CICR1_COLOR_SP)	/* color space */
+#define CICR1_RGB_BPP_VAL(x)	(((x) << 7) & CICR1_RGB_BPP)	/* bpp for rgb */
+#define CICR1_RGBT_CONV_VAL(x)	(((x) << 29) & CICR1_RGBT_CONV)	/* rgbt conv */

 #define CICR2_BLW_VAL(x)  (((x) << 24) & CICR2_BLW) /* Beginning-of-line pixel clock wait count */
 #define CICR2_ELW_VAL(x)  (((x) << 16) & CICR2_ELW) /* End-of-line pixel clock wait count */
@@ -70,6 +73,19 @@ static DEFINE_MUTEX(camera_lock);
 /*
  * Structures
  */
+enum pxa_camera_active_dma {
+	DMA_Y = 0x1,
+	DMA_U = 0x2,
+	DMA_V = 0x4,
+};
+
+/* descriptor needed for the PXA DMA engine */
+struct pxa_cam_dma {
+	dma_addr_t		sg_dma;
+	struct pxa_dma_desc	*sg_cpu;
+	size_t			sg_size;
+	int			sglen;
+};

 /* buffer for one video frame */
 struct pxa_buffer {
@@ -78,11 +94,12 @@ struct pxa_buffer {

 	const struct soc_camera_data_format        *fmt;

-	/* our descriptor list needed for the PXA DMA engine */
-	dma_addr_t		sg_dma;
-	struct pxa_dma_desc	*sg_cpu;
-	size_t			sg_size;
+	/* our descriptor lists for Y, U and V channels */
+	struct pxa_cam_dma dmas[3];
+
 	int			inwork;
+
+	enum pxa_camera_active_dma active_dma;
 };

 struct pxa_framebuffer_queue {
@@ -100,7 +117,8 @@ struct pxa_camera_dev {

 	unsigned int		irq;
 	void __iomem		*base;
-	unsigned int		dma_chan_y;
+
+	unsigned int		dma_chans[3];

 	struct pxacamera_platform_data *pdata;
 	struct resource		*res;
@@ -128,7 +146,15 @@ static int pxa_videobuf_setup(struct vid

 	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);

-	*size = icd->width * icd->height * ((icd->current_fmt->depth + 7) >> 3);
+	/* planar capture requires Y, U and V buffers to be page aligned */
+	if (icd->current_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+		*size = PAGE_ALIGN(icd->width * icd->height); /* Y pages */
+		*size += PAGE_ALIGN(icd->width * icd->height / 2); /* U pages */
+		*size += PAGE_ALIGN(icd->width * icd->height / 2); /* V pages */
+	} else {
+		*size = icd->width * icd->height *
+			((icd->current_fmt->depth + 7) >> 3);
+	}

 	if (0 == *count)
 		*count = 32;
@@ -145,6 +171,7 @@ static void free_buffer(struct videobuf_
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb);
+	int i;

 	BUG_ON(in_interrupt());

@@ -157,12 +184,60 @@ static void free_buffer(struct videobuf_
 	videobuf_dma_unmap(vq, dma);
 	videobuf_dma_free(dma);

-	if (buf->sg_cpu)
-		dma_free_coherent(pcdev->dev, buf->sg_size, buf->sg_cpu,
-				  buf->sg_dma);
-	buf->sg_cpu = NULL;
+	for (i = 0; i < ARRAY_SIZE(buf->dmas); i++) {
+		if (buf->dmas[i].sg_cpu)
+			dma_free_coherent(pcdev->dev, buf->dmas[i].sg_size,
+					  buf->dmas[i].sg_cpu,
+					  buf->dmas[i].sg_dma);
+		buf->dmas[i].sg_cpu = NULL;
+	}

 	buf->vb.state = VIDEOBUF_NEEDS_INIT;
+}
+
+static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
+				struct pxa_buffer *buf,
+				struct videobuf_dmabuf *dma, int channel,
+				int sglen, int sg_start, int cibr,
+				unsigned int size)
+{
+	struct pxa_cam_dma *pxa_dma = &buf->dmas[channel];
+	int i;
+
+	if (pxa_dma->sg_cpu)
+		dma_free_coherent(pcdev->dev, pxa_dma->sg_size,
+				  pxa_dma->sg_cpu, pxa_dma->sg_dma);
+
+	pxa_dma->sg_size = (sglen + 1) * sizeof(struct pxa_dma_desc);
+	pxa_dma->sg_cpu = dma_alloc_coherent(pcdev->dev, pxa_dma->sg_size,
+					     &pxa_dma->sg_dma, GFP_KERNEL);
+	if (!pxa_dma->sg_cpu)
+		return -ENOMEM;
+
+	pxa_dma->sglen = sglen;
+
+	for (i = 0; i < sglen; i++) {
+		int sg_i = sg_start + i;
+		struct scatterlist *sg = dma->sglist;
+		unsigned int dma_len = sg_dma_len(&sg[sg_i]), xfer_len;
+
+		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
+		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(&sg[sg_i]);
+
+		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
+		xfer_len = (min(dma_len, size) + 7) & ~7;
+
+		pxa_dma->sg_cpu[i].dcmd =
+			DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len;
+		size -= dma_len;
+		pxa_dma->sg_cpu[i].ddadr =
+			pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc);
+	}
+
+	pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
+	pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
+
+	return 0;
 }

 static int pxa_videobuf_prepare(struct videobuf_queue *vq,
@@ -173,7 +248,9 @@ static int pxa_videobuf_prepare(struct v
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
-	int i, ret;
+	int ret;
+	int sglen_y,  sglen_yu = 0, sglen_u = 0, sglen_v = 0;
+	int size_y, size_u = 0, size_v = 0;

 	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
 		vb, vb->baddr, vb->bsize);
@@ -218,49 +295,64 @@ static int pxa_videobuf_prepare(struct v
 		if (ret)
 			goto fail;

-		if (buf->sg_cpu)
-			dma_free_coherent(pcdev->dev, buf->sg_size, buf->sg_cpu,
-					  buf->sg_dma);
-
-		buf->sg_size = (dma->sglen + 1) * sizeof(struct pxa_dma_desc);
-		buf->sg_cpu = dma_alloc_coherent(pcdev->dev, buf->sg_size,
-						 &buf->sg_dma, GFP_KERNEL);
-		if (!buf->sg_cpu) {
-			ret = -ENOMEM;
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			/* FIXME the calculations should be more precise */
+			sglen_y = dma->sglen / 2;
+			sglen_u = sglen_v = dma->sglen / 4 + 1;
+			sglen_yu = sglen_y + sglen_u;
+			size_y = size / 2;
+			size_u = size_v = size / 4;
+		} else {
+			sglen_y = dma->sglen;
+			size_y = size;
+		}
+
+		/* init DMA for Y channel */
+		ret = pxa_init_dma_channel(pcdev, buf, dma, 0, sglen_y,
+					   0, 0x28, size_y);
+
+		if (ret) {
+			dev_err(pcdev->dev,
+				"DMA initialization for Y/RGB failed\n");
 			goto fail;
 		}

-		dev_dbg(&icd->dev, "nents=%d size: %d sg=0x%p\n",
-			dma->sglen, size, dma->sglist);
-		for (i = 0; i < dma->sglen; i++) {
-			struct scatterlist *sg = dma->sglist;
-			unsigned int dma_len = sg_dma_len(&sg[i]), xfer_len;
-
-			/* CIBR0 */
-			buf->sg_cpu[i].dsadr = pcdev->res->start + 0x28;
-			buf->sg_cpu[i].dtadr = sg_dma_address(&sg[i]);
-			/* PXA270 Developer's Manual 27.4.4.1:
-			 * round up to 8 bytes */
-			xfer_len = (min(dma_len, size) + 7) & ~7;
-			if (xfer_len & 7)
-				dev_err(&icd->dev, "Unaligned buffer: "
-					"dma_len %u, size %u\n", dma_len, size);
-			buf->sg_cpu[i].dcmd = DCMD_FLOWSRC | DCMD_BURST8 |
-				DCMD_INCTRGADDR | xfer_len;
-			size -= dma_len;
-			buf->sg_cpu[i].ddadr = buf->sg_dma + (i + 1) *
-					sizeof(struct pxa_dma_desc);
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			/* init DMA for U channel */
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, sglen_u,
+						   sglen_y, 0x30, size_u);
+			if (ret) {
+				dev_err(pcdev->dev,
+					"DMA initialization for U failed\n");
+				goto fail_u;
+			}
+
+			/* init DMA for V channel */
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, sglen_v,
+						   sglen_yu, 0x38, size_v);
+			if (ret) {
+				dev_err(pcdev->dev,
+					"DMA initialization for V failed\n");
+				goto fail_v;
+			}
 		}
-		buf->sg_cpu[dma->sglen - 1].ddadr = DDADR_STOP;
-		buf->sg_cpu[dma->sglen - 1].dcmd |= DCMD_ENDIRQEN;

 		vb->state = VIDEOBUF_PREPARED;
 	}

 	buf->inwork = 0;
+	buf->active_dma = DMA_Y;
+	if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P)
+		buf->active_dma |= DMA_U | DMA_V;

 	return 0;

+fail_v:
+	dma_free_coherent(pcdev->dev, buf->dmas[1].sg_size,
+			  buf->dmas[1].sg_cpu, buf->dmas[1].sg_dma);
+fail_u:
+	dma_free_coherent(pcdev->dev, buf->dmas[0].sg_size,
+			  buf->dmas[0].sg_cpu, buf->dmas[0].sg_dma);
 fail:
 	free_buffer(vq, buf);
 out:
@@ -277,8 +369,6 @@ static void pxa_videobuf_queue(struct vi
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
 	struct pxa_buffer *active;
-	struct videobuf_dmabuf *dma = videobuf_to_dma(vb);
-	int nents = dma->sglen;
 	unsigned long flags;

 	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
@@ -292,59 +382,86 @@ static void pxa_videobuf_queue(struct vi

 	if (!active) {
 		CIFR |= CIFR_RESET_F;
-		DDADR(pcdev->dma_chan_y) = buf->sg_dma;
-		DCSR(pcdev->dma_chan_y) = DCSR_RUN;
+		DDADR(pcdev->dma_chans[0]) = buf->dmas[0].sg_dma;
+		DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
+
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+			DDADR(pcdev->dma_chans[1]) = buf->dmas[1].sg_dma;
+			DCSR(pcdev->dma_chans[1]) = DCSR_RUN;
+
+			DDADR(pcdev->dma_chans[2]) = buf->dmas[2].sg_dma;
+			DCSR(pcdev->dma_chans[2]) = DCSR_RUN;
+		}
+
 		pcdev->active = buf;
 		CICR0 |= CICR0_ENB;
 	} else {
-		struct videobuf_dmabuf *active_dma =
-			videobuf_to_dma(&active->vb);
-		/* Stop DMA engine */
-		DCSR(pcdev->dma_chan_y) = 0;
-
-		/* Add the descriptors we just initialized to the currently
-		 * running chain
-		 */
-		active->sg_cpu[active_dma->sglen - 1].ddadr = buf->sg_dma;
-
-		/* Setup a dummy descriptor with the DMA engines current
-		 * state
-		 */
-		/* CIBR0 */
-		buf->sg_cpu[nents].dsadr = pcdev->res->start + 0x28;
-		buf->sg_cpu[nents].dtadr = DTADR(pcdev->dma_chan_y);
-		buf->sg_cpu[nents].dcmd = DCMD(pcdev->dma_chan_y);
-
-		if (DDADR(pcdev->dma_chan_y) == DDADR_STOP) {
-			/* The DMA engine is on the last descriptor, set the
-			 * next descriptors address to the descriptors
-			 * we just initialized
+		struct pxa_cam_dma *buf_dma;
+		struct pxa_cam_dma *act_dma;
+		int channels = 1;
+		int nents;
+		int i;
+
+		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P)
+			channels = 3;
+
+		for (i = 0; i < channels; i++) {
+			buf_dma = &buf->dmas[i];
+			act_dma = &active->dmas[0];
+			nents = buf_dma->sglen;
+
+			/* Stop DMA engine */
+			DCSR(pcdev->dma_chans[i]) = 0;
+
+			/* Add the descriptors we just initialized to
+			   the currently running chain */
+			act_dma->sg_cpu[act_dma->sglen - 1].ddadr =
+				buf_dma->sg_dma;
+
+			/* Setup a dummy descriptor with the DMA engines current
+			 * state
 			 */
-			buf->sg_cpu[nents].ddadr = buf->sg_dma;
-		} else {
-			buf->sg_cpu[nents].ddadr = DDADR(pcdev->dma_chan_y);
+			buf_dma->sg_cpu[nents].dsadr =
+				pcdev->res->start + 0x28 + i*8; /* CIBRx */
+			buf_dma->sg_cpu[nents].dtadr =
+				DTADR(pcdev->dma_chans[i]);
+			buf_dma->sg_cpu[nents].dcmd =
+				DCMD(pcdev->dma_chans[i]);
+
+			if (DDADR(pcdev->dma_chans[i]) == DDADR_STOP) {
+				/* The DMA engine is on the last
+				   descriptor, set the next descriptors
+				   address to the descriptors we just
+				   initialized */
+				buf_dma->sg_cpu[nents].ddadr = buf_dma->sg_dma;
+			} else {
+				buf_dma->sg_cpu[nents].ddadr =
+					DDADR(pcdev->dma_chans[i]);
+			}
+
+			/* The next descriptor is the dummy descriptor */
+			DDADR(pcdev->dma_chans[i]) = buf_dma->sg_dma + nents *
+				sizeof(struct pxa_dma_desc);
+
+			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
 		}
-
-		/* The next descriptor is the dummy descriptor */
-		DDADR(pcdev->dma_chan_y) = buf->sg_dma + nents *
-			sizeof(struct pxa_dma_desc);
-
 #ifdef DEBUG
-		if (CISR & CISR_IFO_0) {
+		if (CISR & (CISR_IFO_0 | CISR_IFO_1 | CISR_IFO_2)) {
 			dev_warn(pcdev->dev, "FIFO overrun\n");
-			DDADR(pcdev->dma_chan_y) = pcdev->active->sg_dma;
+			for (i = 0; i < channels; i++)
+				DDADR(pcdev->dma_chans[i]) =
+					pcdev->active->dmas[i].sg_dma;

 			CICR0 &= ~CICR0_ENB;
 			CIFR |= CIFR_RESET_F;
-			DCSR(pcdev->dma_chan_y) = DCSR_RUN;
+			for (i = 0; i < channels; i++)
+				DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
 			CICR0 |= CICR0_ENB;
-		} else
+		}
 #endif
-			DCSR(pcdev->dma_chan_y) = DCSR_RUN;
 	}

 	spin_unlock_irqrestore(&pcdev->lock, flags);
-
 }

 static void pxa_videobuf_release(struct videobuf_queue *vq,
@@ -376,41 +493,10 @@ static void pxa_videobuf_release(struct
 	free_buffer(vq, buf);
 }

-static void pxa_camera_dma_irq_y(int channel, void *data)
-{
-	struct pxa_camera_dev *pcdev = data;
-	struct pxa_buffer *buf;
-	unsigned long flags;
-	unsigned int status;
-	struct videobuf_buffer *vb;
-
-	spin_lock_irqsave(&pcdev->lock, flags);
-
-	status = DCSR(pcdev->dma_chan_y);
-	DCSR(pcdev->dma_chan_y) = status;
-
-	if (status & DCSR_BUSERR) {
-		dev_err(pcdev->dev, "DMA Bus Error IRQ!\n");
-		goto out;
-	}
-
-	if (!(status & DCSR_ENDINTR)) {
-		dev_err(pcdev->dev, "Unknown DMA IRQ source, "
-			"status: 0x%08x\n", status);
-		goto out;
-	}
-
-	if (!pcdev->active) {
-		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
-		goto out;
-	}
-
-	vb = &pcdev->active->vb;
-	buf = container_of(vb, struct pxa_buffer, vb);
-	WARN_ON(buf->inwork || list_empty(&vb->queue));
-	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
-		vb, vb->baddr, vb->bsize);
-
+static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
+			      struct videobuf_buffer *vb,
+			      struct pxa_buffer *buf)
+{
 	/* _init is used to debug races, see comment in pxa_camera_reqbufs() */
 	list_del_init(&vb->queue);
 	vb->state = VIDEOBUF_DONE;
@@ -420,16 +506,76 @@ static void pxa_camera_dma_irq_y(int cha

 	if (list_empty(&pcdev->capture)) {
 		pcdev->active = NULL;
-		DCSR(pcdev->dma_chan_y) = 0;
+		DCSR(pcdev->dma_chans[0]) = 0;
+		DCSR(pcdev->dma_chans[1]) = 0;
+		DCSR(pcdev->dma_chans[2]) = 0;
 		CICR0 &= ~CICR0_ENB;
+		return;
+	}
+
+	pcdev->active = list_entry(pcdev->capture.next,
+				   struct pxa_buffer, vb.queue);
+}
+
+static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
+			       enum pxa_camera_active_dma act_dma)
+{
+	struct pxa_buffer *buf;
+	unsigned long flags;
+	unsigned int status;
+	struct videobuf_buffer *vb;
+
+	spin_lock_irqsave(&pcdev->lock, flags);
+
+	status = DCSR(channel);
+	DCSR(channel) = status | DCSR_ENDINTR;
+
+	if (status & DCSR_BUSERR) {
+		dev_err(pcdev->dev, "DMA Bus Error IRQ!\n");
 		goto out;
 	}

-	pcdev->active = list_entry(pcdev->capture.next, struct pxa_buffer,
-				   vb.queue);
+	if (!(status & DCSR_ENDINTR)) {
+		dev_err(pcdev->dev, "Unknown DMA IRQ source, "
+			"status: 0x%08x\n", status);
+		goto out;
+	}
+
+	if (!pcdev->active) {
+		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
+		goto out;
+	}
+
+	vb = &pcdev->active->vb;
+	buf = container_of(vb, struct pxa_buffer, vb);
+	WARN_ON(buf->inwork || list_empty(&vb->queue));
+	dev_dbg(pcdev->dev, "%s (vb=0x%p) 0x%08lx %d\n", __FUNCTION__,
+		vb, vb->baddr, vb->bsize);
+
+	buf->active_dma &= ~act_dma;
+	if (!buf->active_dma)
+		pxa_camera_wakeup(pcdev, vb, buf);

 out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
+}
+
+static void pxa_camera_dma_irq_y(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_Y);
+}
+
+static void pxa_camera_dma_irq_u(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_U);
+}
+
+static void pxa_camera_dma_irq_v(int channel, void *data)
+{
+	struct pxa_camera_dev *pcdev = data;
+	pxa_camera_dma_irq(channel, pcdev, DMA_V);
 }

 static struct videobuf_queue_ops pxa_videobuf_ops = {
@@ -525,7 +671,6 @@ static irqreturn_t pxa_camera_irq(int ir
 	dev_dbg(pcdev->dev, "Camera interrupt status 0x%x\n", status);

 	CISR = status;
-
 	return IRQ_HANDLED;
 }

@@ -571,8 +716,11 @@ static void pxa_camera_remove_device(str

 	/* disable capture, disable interrupts */
 	CICR0 = 0x3ff;
+
 	/* Stop DMA engine */
-	DCSR(pcdev->dma_chan_y) = 0;
+	DCSR(pcdev->dma_chans[0]) = 0;
+	DCSR(pcdev->dma_chans[1]) = 0;
+	DCSR(pcdev->dma_chans[2]) = 0;

 	icd->ops->release(icd);

@@ -625,7 +773,7 @@ static int pxa_camera_set_bus_param(stru
 		to_soc_camera_host(icd->dev.parent);
 	struct pxa_camera_dev *pcdev = ici->priv;
 	unsigned long dw, bpp, bus_flags, camera_flags, common_flags;
-	u32 cicr0, cicr4 = 0;
+	u32 cicr0, cicr1, cicr4 = 0;
 	int ret = test_platform_param(pcdev, icd->buswidth, &bus_flags);

 	if (ret < 0)
@@ -702,7 +850,25 @@ static int pxa_camera_set_bus_param(stru
 	cicr0 = CICR0;
 	if (cicr0 & CICR0_ENB)
 		CICR0 = cicr0 & ~CICR0_ENB;
-	CICR1 = CICR1_PPL_VAL(icd->width - 1) | bpp | dw;
+
+	cicr1 = CICR1_PPL_VAL(icd->width - 1) | bpp | dw;
+
+	switch (pixfmt) {
+	case V4L2_PIX_FMT_YUV422P:
+		cicr1 |= CICR1_YCBCR_F;
+	case V4L2_PIX_FMT_YUYV:
+		cicr1 |= CICR1_COLOR_SP_VAL(2);
+		break;
+	case V4L2_PIX_FMT_RGB555:
+		cicr1 |= CICR1_RGB_BPP_VAL(1) | CICR1_RGBT_CONV_VAL(2) |
+			CICR1_TBIT | CICR1_COLOR_SP_VAL(1);
+		break;
+	case V4L2_PIX_FMT_RGB565:
+		cicr1 |= CICR1_COLOR_SP_VAL(1) | CICR1_RGB_BPP_VAL(2);
+		break;
+	}
+
+	CICR1 = cicr1;
 	CICR2 = 0;
 	CICR3 = CICR3_LPF_VAL(icd->height - 1) |
 		CICR3_BFW_VAL(min((unsigned short)255, icd->y_skip_top));
@@ -890,16 +1056,36 @@ static int pxa_camera_probe(struct platf
 	pcdev->dev = &pdev->dev;

 	/* request dma */
-	pcdev->dma_chan_y = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
-					    pxa_camera_dma_irq_y, pcdev);
-	if (pcdev->dma_chan_y < 0) {
+	pcdev->dma_chans[0] = pxa_request_dma("CI_Y", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_y, pcdev);
+	if (pcdev->dma_chans[0] < 0) {
 		dev_err(pcdev->dev, "Can't request DMA for Y\n");
 		err = -ENOMEM;
 		goto exit_iounmap;
 	}
-	dev_dbg(pcdev->dev, "got DMA channel %d\n", pcdev->dma_chan_y);
-
-	DRCMR68 = pcdev->dma_chan_y  | DRCMR_MAPVLD;
+	dev_dbg(pcdev->dev, "got DMA channel %d\n", pcdev->dma_chans[0]);
+
+	pcdev->dma_chans[1] = pxa_request_dma("CI_U", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_u, pcdev);
+	if (pcdev->dma_chans[1] < 0) {
+		dev_err(pcdev->dev, "Can't request DMA for U\n");
+		err = -ENOMEM;
+		goto exit_free_dma_y;
+	}
+	dev_dbg(pcdev->dev, "got DMA channel (U) %d\n", pcdev->dma_chans[1]);
+
+	pcdev->dma_chans[2] = pxa_request_dma("CI_V", DMA_PRIO_HIGH,
+					      pxa_camera_dma_irq_v, pcdev);
+	if (pcdev->dma_chans[0] < 0) {
+		dev_err(pcdev->dev, "Can't request DMA for V\n");
+		err = -ENOMEM;
+		goto exit_free_dma_u;
+	}
+	dev_dbg(pcdev->dev, "got DMA channel (V) %d\n", pcdev->dma_chans[2]);
+
+	DRCMR68 = pcdev->dma_chans[0] | DRCMR_MAPVLD;
+	DRCMR69 = pcdev->dma_chans[1] | DRCMR_MAPVLD;
+	DRCMR70 = pcdev->dma_chans[2] | DRCMR_MAPVLD;

 	/* request irq */
 	err = request_irq(pcdev->irq, pxa_camera_irq, 0, PXA_CAM_DRV_NAME,
@@ -921,7 +1107,11 @@ exit_free_irq:
 exit_free_irq:
 	free_irq(pcdev->irq, pcdev);
 exit_free_dma:
-	pxa_free_dma(pcdev->dma_chan_y);
+	pxa_free_dma(pcdev->dma_chans[2]);
+exit_free_dma_u:
+	pxa_free_dma(pcdev->dma_chans[1]);
+exit_free_dma_y:
+	pxa_free_dma(pcdev->dma_chans[0]);
 exit_iounmap:
 	iounmap(base);
 exit_release:
@@ -941,7 +1131,9 @@ static int __devexit pxa_camera_remove(s

 	clk_put(pcdev->clk);

-	pxa_free_dma(pcdev->dma_chan_y);
+	pxa_free_dma(pcdev->dma_chans[0]);
+	pxa_free_dma(pcdev->dma_chans[1]);
+	pxa_free_dma(pcdev->dma_chans[2]);
 	free_irq(pcdev->irq, pcdev);

 	soc_camera_host_unregister(&pxa_soc_camera_host);

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Guennadi Liakhovetski]

On Sun, 6 Apr 2008, Mike Rapoport wrote:

> >> +		if (buf->fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> >> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, sglen_y,
> > 
> > This should be sglen_u							^^^^^
> 
> Right. No wonder I had colors distorted :)

Now you get them right?

The patch looks good to me now, just please regenerate it in the "-p1" 
format. Also, I'm not quite sure why you don't have blanks in unchenged 
lines like, normally one sees (space replaced with "*")

<hunk>
@@ -145,6 +171,7 @@ static void free_buffer(struct videobuf_
*		to_soc_camera_host(icd->dev.parent);
*	struct pxa_camera_dev *pcdev = ici->priv;
*	struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb);
+	int i;
*
*	BUG_ON(in_interrupt());
*
</hunk>

whereas you have everywhere just blank lines:

<hunk>
@@ -145,6 +171,7 @@ static void free_buffer(struct videobuf_
*		to_soc_camera_host(icd->dev.parent);
*	struct pxa_camera_dev *pcdev = ici->priv;
*	struct videobuf_dmabuf *dma = videobuf_to_dma(&buf->vb);
+	int i;

*	BUG_ON(in_interrupt());

</hunk>

It wasn't a problem for my version of GNU patch, don't know if this can be 
a problem for git or whatever else the person applying your patch (Mauro?) 
will be using. Maybe would be better to fix this as you resend in "-p1" 
too. Hm, I think, on V4L they do use "-p2" for mercurial, so, this might 
not be a problem then.

In any case here's my

> Signed-off-by: Mike Rapoport <mike@compulab.co.il>
Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@pengutronix.de>

Just one more request:

> diff --git a/linux/drivers/media/video/pxa_camera.c b/linux/drivers/media/video/pxa_camera.c
> --- a/linux/drivers/media/video/pxa_camera.c
> +++ b/linux/drivers/media/video/pxa_camera.c
> @@ -49,6 +49,9 @@
> 
>  #define CICR1_DW_VAL(x)   ((x) & CICR1_DW)	    /* Data bus width */
>  #define CICR1_PPL_VAL(x)  (((x) << 15) & CICR1_PPL) /* Pixels per line */
> +#define CICR1_COLOR_SP_VAL(x)	(((x) << 3) & CICR1_COLOR_SP)	/* color space */
> +#define CICR1_RGB_BPP_VAL(x)	(((x) << 7) & CICR1_RGB_BPP)	/* bpp for rgb */
> +#define CICR1_RGBT_CONV_VAL(x)	(((x) << 29) & CICR1_RGBT_CONV)	/* rgbt conv */

There's a typo in include/asm-arm/arch-pxa/pxa-regs.h:

#define CICR1_RGBT_CONV	(0x3 << 30)	/* RGBT conversion mask */

30 should be 29, you have it right. Could you please submit a patch to ARM 
kernel to fix this?

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Guennadi Liakhovetski]

On Sun, 6 Apr 2008, Mike Rapoport wrote:

> >> +			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
> >> +#ifdef DEBUG
> >> +			if (CISR & CISR_IFO_0) {
> >> +				dev_warn(pcdev->dev, "FIFO overrun\n");
> >> +				for (i = 0; i < channels; i++)
> >> +					DDADR(pcdev->dma_chans[i]) =
> >> +						pcdev->active->dmas[i].sg_dma;
> >> +
> >> +				CICR0 &= ~CICR0_ENB;
> >> +				CIFR |= CIFR_RESET_F;
> >> +				for (i = 0; i < channels; i++)
> >> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
> >> +				CICR0 |= CICR0_ENB;
> >> +			} else {
> >> +				for (i = 0; i < channels; i++)
> >> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
> >> +			}
> > 
> > These three loops don't look right. At least because they use the same 
> > index i. And you're iterating over channels inside a loop over channels, 
> > and you have dma_chans[0] instead of [i]. Please fix.
> 
> Here I'm not quite sure what exactly should be done as I never got overruns.
> For now I move this code out of the loop and in case of overrun re-enable
> all three DMAs. BTW, the 'else' here is completely redundant, so I just removed it.

Mike, you probably saw the recent thread on this list, where the overrun 
problem did come up: http://marc.info/?t=120732439600006&r=1&w=2 and my 
last post in this thread with a patch. Could you, please, test it with 
your YCbCr setup? Would be great, if you could test it with the 
application, that fengxin quoted in his mail 
http://marc.info/?l=linux-video&m=120762092820785&w=2, see if you too get 
overruns with it, and see if my patch fixes them.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Mike Rapoport]

Guennadi Liakhovetski wrote:
> On Sun, 6 Apr 2008, Mike Rapoport wrote:
> 
>>>> +			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
>>>> +#ifdef DEBUG
>>>> +			if (CISR & CISR_IFO_0) {
>>>> +				dev_warn(pcdev->dev, "FIFO overrun\n");
>>>> +				for (i = 0; i < channels; i++)
>>>> +					DDADR(pcdev->dma_chans[i]) =
>>>> +						pcdev->active->dmas[i].sg_dma;
>>>> +
>>>> +				CICR0 &= ~CICR0_ENB;
>>>> +				CIFR |= CIFR_RESET_F;
>>>> +				for (i = 0; i < channels; i++)
>>>> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
>>>> +				CICR0 |= CICR0_ENB;
>>>> +			} else {
>>>> +				for (i = 0; i < channels; i++)
>>>> +					DCSR(pcdev->dma_chans[0]) = DCSR_RUN;
>>>> +			}
>>> These three loops don't look right. At least because they use the same 
>>> index i. And you're iterating over channels inside a loop over channels, 
>>> and you have dma_chans[0] instead of [i]. Please fix.
>> Here I'm not quite sure what exactly should be done as I never got overruns.
>> For now I move this code out of the loop and in case of overrun re-enable
>> all three DMAs. BTW, the 'else' here is completely redundant, so I just removed it.
> 
> Mike, you probably saw the recent thread on this list, where the overrun 
> problem did come up: http://marc.info/?t=120732439600006&r=1&w=2 and my 
> last post in this thread with a patch. Could you, please, test it with 
> your YCbCr setup? Would be great, if you could test it with the 
> application, that fengxin quoted in his mail 
> http://marc.info/?l=linux-video&m=120762092820785&w=2, see if you too get 
> overruns with it, and see if my patch fixes them.

Sorry for the delay, and as far as I can see you've already have a fix. :)
If you'd like I can test it with YUV setup. I'll apreciate if you can send me
the entire updated pxa_camera.c, to save time on merge conflicts.

> Thanks
> Guennadi
> ---
> Guennadi Liakhovetski
> 

-- 
Sincerely yours,
Mike.

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

Re: [PATCH] pxa_camera: Add support for YUV modes

[Guennadi Liakhovetski]

On Thu, 17 Apr 2008, Mike Rapoport wrote:

> Sorry for the delay, and as far as I can see you've already have a fix. :)

I hope so:-)

> If you'd like I can test it with YUV setup. I'll apreciate if you can send me
> the entire updated pxa_camera.c, to save time on merge conflicts.

Well, let's try it this way: I've uploaded the current patch set on

http://home.arcor.de/g.liakhovetski/v4l/2.6.25/

where patches under v4l-dvb:devel/ are already in the current 
v4l-dvb/devel. The patch under gpio/ is needed for mt9*.c, but you won't 
probably need it either. Let me know if you get any problems.

Thanks
Guennadi
---
Guennadi Liakhovetski

--
video4linux-list mailing list
Unsubscribe mailto:video4linux-list-request@redhat.com?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list