Skip to contentSkip to conversation

pxa_camera: Add support for YUV modes

PATCH Superseded Mike Rapoport 3 participants 9 messages Apr 1, 2008 – Apr 17, 2008 Media (V4L/DVB)

v1 of 3 Superseded Latest is v3 →

lore
Share & tools
lore

Mike Rapoport

author
#0 lore raw

PATCH 1/1 pxa_camera: Add support for YUV modes

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

Signature · 702 lines
Sincerely yours,
Mike.

Signed-off-by: Mike Rapoport <[email protected]>

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
#1 lore raw
Quoted 1 line · Mike Rapoport

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

author
#2 lore raw
Quoted 6 lines · Guennadi Liakhovetski
Quoted 1 line · Mike Rapoport

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.

Quoted 2 lines

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.

Quoted 5 lines

Thanks
Guennadi
---
Guennadi Liakhovetski

Signature · 8 lines
Sincerely yours,
Mike.


--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
#3 lore raw
In reply to Mike Rapoport

Looks good overall!

First static analysis:

Review line 70

Quoted 5 lines · Mike Rapoport
(quoted diff) +1 -0
@@ -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.

Quoted 11 lines
(quoted diff) +11 -0
+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);

Review

Quoted 10 lines
(quoted diff) +10 -0
+
+	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?

Quoted 8 lines
(quoted diff) +8 -0
+	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]);
+

Review

Quoted 10 lines
(quoted diff) +10 -0
+		/* 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.

Quoted 25 lines
(quoted diff) +6 -5
+	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);
-

Review static int pxa_videobuf_prepare(struct v · line 302

Quoted 10 lines
(quoted diff) +5 -5
-		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

Review static int pxa_videobuf_prepare(struct v · line 307

Quoted 5 lines
(quoted diff) +3 -0
+			sglen_yu = sglen_y + sglen_u;
 
 
+			size_y = size / 2;
+			size_uv = size / 4;

Following your above notation more consistent would be

Review static int pxa_videobuf_prepare(struct v · line 308

Quoted 1 line
(quoted diff) +1 -0
+			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).

Quoted 26 lines
(quoted diff) +11 -12
+		} 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;

Review static int pxa_videobuf_prepare(struct v · line 324

Quoted 10 lines
(quoted diff) +2 -8
-			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 ^^^^^

Review static int pxa_videobuf_prepare(struct v · line 332

Quoted 8 lines
(quoted diff) +8 -0
+						   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 ^^^^^

Quoted 100 lines
(quoted diff) +65 -27
@@ -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;

Review static void pxa_videobuf_queue(struct vi · line 460

Quoted 10 lines
(quoted diff) +10 -0
+
+				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.

Review static void pxa_videobuf_queue(struct vi

Quoted 12 lines
(quoted diff) +12 -0
+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

Review

Quoted 1 line
(quoted diff) +1 -0
+	DCSR(channel) = status | DCSR_ENDINTR;

to avoid clearing new interrupts.

Review static void pxa_camera_remove_device(str · line 753

Quoted 6 lines
(quoted diff) +1 -1
@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

author
#4 lore raw
Quoted 9 lines · Guennadi Liakhovetski

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.

Quoted 5 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+	pxa_dma->sglen = sglen;
+
+	/* DMA for Y buffers */

I presume, this comment doesn't hold?

Fixed.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	}
+	/* end of Y pages */

This one too.

Fixed.

Quoted 14 lines

Review

Quoted 3 lines
(quoted diff) +3 -0
+			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

Review

Quoted 5 lines
(quoted diff) +3 -0
+			sglen_yu = sglen_y + sglen_u;
 
 
+			size_y = size / 2;
+			size_uv = size / 4;

Following your above notation more consistent would be

Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+			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.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		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 :)

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +1 -0
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, sglen_y,

This should be sglen_v ^^^^^

Fixed.

Quoted 13 lines
Quoted 13 lines
(quoted diff) +13 -0
+			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;
Quoted 8 lines

Review

Quoted 4 lines
(quoted diff) +4 -0
+			} 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.

Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+	status = DCSR(channel);
+	DCSR(channel) |= DCSR_ENDINTR;

better

Fixed.

Quoted 4 lines

Review

Quoted 1 line
(quoted diff) +1 -0
+	DCSR(channel) = status | DCSR_ENDINTR;

to avoid clearing new interrupts.

Quoted 8 lines

Review static void pxa_camera_remove_device(str · line 753

Quoted 6 lines
(quoted diff) +1 -1
@@ -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.

Quoted 13 lines

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

Quoted 5 lines

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.

Quoted 10 lines

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

Signature · 5 lines
Sincerely yours,
Mike.


Signed-off-by: Mike Rapoport <[email protected]>
linux/drivers/media/video/pxa_camera.c +324 -132
@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#5 lore raw
Quoted 6 lines · Mike Rapoport
Quoted 4 lines

Review

Quoted 2 lines
(quoted diff) +2 -0
+		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

Quoted 1 line
Signed-off-by Mike Rapoport <[email protected]>
Signed-off-by Guennadi Liakhovetski <[email protected]>

Just one more request:

linux/drivers/media/video/pxa_camera.c line 54

Quoted 10 lines
linux/drivers/media/video/pxa_camera.c +3 -0
@@ -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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

#6 lore raw
In reply to Mike Rapoport
Quoted 22 lines · Mike Rapoport
Quoted 13 lines
Quoted 13 lines
(quoted diff) +13 -0
+			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;
Quoted 8 lines

linux/drivers/media/video/pxa_camera.c static void pxa_videobuf_queue(struct vi · line 460

Quoted 4 lines
linux/drivers/media/video/pxa_camera.c +4 -0
+			} 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.

Quoted 3 lines

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list

author
#7 lore raw
Quoted 26 lines · Guennadi Liakhovetski
Quoted 20 lines · Mike Rapoport
Quoted 12 lines
Quoted 12 lines
(quoted diff) +12 -0
+			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;
Quoted 8 lines

linux/drivers/media/video/pxa_camera.c static void pxa_videobuf_queue(struct vi · line 460

Quoted 5 lines
linux/drivers/media/video/pxa_camera.c +5 -0
+				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.

Quoted 3 lines

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.

Quoted 7 lines

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.

Quoted 5 lines

Thanks
Guennadi
---
Guennadi Liakhovetski

Signature · 7 lines
Sincerely yours,
Mike.

--
video4linux-list mailing list
Unsubscribe mailto:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list
#8 lore raw
Quoted 1 line · Mike Rapoport

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

I hope so:-)

Quoted 2 lines

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:[email protected]?subject=unsubscribe
https://www.redhat.com/mailman/listinfo/video4linux-list