[PATCH v2 0/4] pxa_camera: DMA redesign

[PATCH v2 0/4] pxa_camera: DMA redesign

[Robert Jarzmik]

This is an update of the DMA redesign work for pxa_camera.

Guennadi, I hope I got all your comments within this serie. If I missed
something, I hope you won't be bothered. Just tell me and I'll fix it right
away.

To the last submit I added a little change to overrun :
 - handling for a corner case (test of pcdev->active in DMA irq).
 - case of YUV422P where one channel overrun while another completes

OK, now time for second iteration of this patchset.

Happy review.

Robert Jarzmik (4):
  pxa_camera: Enforce YUV422P frame sizes to be 16 multiples
  pxa_camera: remove YUV planar formats hole
  pxa_camera: Redesign DMA handling
  pxa_camera: Fix overrun condition on last buffer

 Documentation/video4linux/pxa_camera.txt |  125 +++++++
 drivers/media/video/pxa_camera.c         |  516 +++++++++++++++++++-----------
 2 files changed, 460 insertions(+), 181 deletions(-)
 create mode 100644 Documentation/video4linux/pxa_camera.txt

[PATCH v2 1/4] pxa_camera: Enforce YUV422P frame sizes to be 16 multiples

[Robert Jarzmik]

Due to DMA constraints, the DMA chain always transfers bytes
from the QIF fifos to memory in 8 bytes units. In planar
formats, that could mean 0 padding between Y and U plane
(and between U and V plane), which is against YUV422P
standard.

Therefore, a frame size is required to be a multiple of 16
(so U plane size is a multiple of 8). It is enforced in
try_fmt() and set_fmt() primitives, be aligning height then
width on 4 multiples as need be, to reach a 16 multiple.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |   28 +++++++++++++++++++---------
 1 files changed, 19 insertions(+), 9 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index e3e6b29..8e5611b 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -162,6 +162,8 @@
 			CICR0_PERRM | CICR0_QDM | CICR0_CDM | CICR0_SOFM | \
 			CICR0_EOFM | CICR0_FOM)
 
+#define PIX_YUV422P_ALIGN 16	/* YUV422P pix size should be a multiple of 16 */
+
 /*
  * Structures
  */
@@ -241,15 +243,11 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
 
 	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
 
-	/* planar capture requires Y, U and V buffers to be page aligned */
-	if (pcdev->channels == 3) {
-		*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 (pcdev->channels == 3)
+		*size = icd->width * icd->height * 2;
+	else
+		*size = roundup(icd->width * icd->height *
+				((icd->current_fmt->depth + 7) >> 3), 8);
 
 	if (0 == *count)
 		*count = 32;
@@ -1234,6 +1232,18 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
 		pix->width = 2048;
 	pix->width &= ~0x01;
 
+	/*
+	 * YUV422P planar format requires images size to be a 16 bytes
+	 * multiple. If not, zeros will be inserted between Y and U planes, and
+	 * U and V planes, and YUV422P standard would be violated.
+	 */
+	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
+		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
+			pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2);
+		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
+			pix->width = ALIGN(pix->width, PIX_YUV422P_ALIGN / 2);
+	}
+
 	pix->bytesperline = pix->width *
 		DIV_ROUND_UP(xlate->host_fmt->depth, 8);
 	pix->sizeimage = pix->height * pix->bytesperline;
-- 
1.5.6.5

[PATCH v2 2/4] pxa_camera: remove YUV planar formats hole

[Robert Jarzmik]

All planes were PAGE aligned (ie. 4096 bytes aligned). This
is not consistent with YUV422 format, which requires Y, U
and V planes glued together.  The new implementation forces
the alignement on 8 bytes (DMA requirement), which is almost
always the case (granted by width x height being a multiple
of 8).

The test cases include tests in both YUV422 and RGB565 :
 - a picture of size 111 x 111 (cross RAM pages example)
 - a picture of size 1023 x 4 in (under 1 RAM page)
 - a picture of size 1024 x 4 in (exactly 1 RAM page)
 - a picture of size 1025 x 4 in (over 1 RAM page)
 - a picture of size 1280 x 1024 (many RAM pages)

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |  150 +++++++++++++++++++++++++++-----------
 1 files changed, 108 insertions(+), 42 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 8e5611b..aca5374 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -287,19 +287,63 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf)
 	buf->vb.state = VIDEOBUF_NEEDS_INIT;
 }
 
+static int calculate_dma_sglen(struct scatterlist *sglist, int sglen,
+			       int sg_first_ofs, int size)
+{
+	int i, offset, dma_len, xfer_len;
+	struct scatterlist *sg;
+
+	offset = sg_first_ofs;
+	for_each_sg(sglist, sg, sglen, i) {
+		dma_len = sg_dma_len(sg);
+
+		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
+		xfer_len = roundup(min(dma_len - offset, size), 8);
+
+		size = max(0, size - xfer_len);
+		offset = 0;
+		if (size == 0)
+			break;
+	}
+
+	BUG_ON(size != 0);
+	return i + 1;
+}
+
+/**
+ * pxa_init_dma_channel - init dma descriptors
+ * @pcdev: pxa camera device
+ * @buf: pxa buffer to find pxa dma channel
+ * @dma: dma video buffer
+ * @channel: dma channel (0 => 'Y', 1 => 'U', 2 => 'V')
+ * @cibr: camera read fifo
+ * @size: bytes to transfer
+ * @sg_first: index of first element of sg_list
+ * @sg_first_ofs: offset in first element of sg_list
+ *
+ * Prepares the pxa dma descriptors to transfer one camera channel.
+ * Beware sg_first and sg_first_ofs are both input and output parameters.
+ *
+ * Returns 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,
-				unsigned int size)
+				int cibr, int size,
+				struct scatterlist **sg_first, int *sg_first_ofs)
 {
 	struct pxa_cam_dma *pxa_dma = &buf->dmas[channel];
-	int i;
+	struct scatterlist *sg;
+	int i, offset, sglen;
+	int dma_len = 0, xfer_len = 0;
 
 	if (pxa_dma->sg_cpu)
 		dma_free_coherent(pcdev->dev, pxa_dma->sg_size,
 				  pxa_dma->sg_cpu, pxa_dma->sg_dma);
 
+	sglen = calculate_dma_sglen(*sg_first, dma->sglen,
+				    *sg_first_ofs, size);
+
 	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);
@@ -307,27 +351,53 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
 		return -ENOMEM;
 
 	pxa_dma->sglen = sglen;
+	offset = *sg_first_ofs;
 
-	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;
+	dev_dbg(pcdev->dev, "DMA: sg_first=%p, sglen=%d, ofs=%d, dma.desc=%x\n",
+		*sg_first, sglen, *sg_first_ofs, pxa_dma->sg_dma);
 
-		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
-		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(&sg[sg_i]);
+
+	for_each_sg(*sg_first, sg, sglen, i) {
+		dma_len = sg_dma_len(sg);
 
 		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
-		xfer_len = (min(dma_len, size) + 7) & ~7;
+		xfer_len = roundup(min(dma_len - offset, size), 8);
 
+		size = max(0, size - xfer_len);
+
+		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
+		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset;
 		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);
+
+		dev_vdbg(pcdev->dev, "DMA: desc.%08x->@phys=0x%08x, len=%d\n",
+			 pxa_dma->sg_dma + i * sizeof(struct pxa_dma_desc),
+			 sg_dma_address(sg) + offset, xfer_len);
+		offset = 0;
+
+		if (size == 0)
+			break;
 	}
 
-	pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
-	pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
+	pxa_dma->sg_cpu[sglen].ddadr = DDADR_STOP;
+	pxa_dma->sg_cpu[sglen].dcmd  = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_ENDIRQEN;
+
+	*sg_first_ofs = xfer_len;
+	/*
+	 * Handle 1 special case :
+	 *  - in 3 planes (YUV422P format), we might finish with xfer_len
+	 *    equal to dma_len (end on PAGE boundary). In this case, the sg
+	 *    element for next plane should be the next of the last one
+	 *    used to store the last scatter gather RAM page
+	 */
+	if (*sg_first_ofs >= dma_len) {
+		*sg_first_ofs -= dma_len;
+		*sg_first = sg_next(sg);
+	} else {
+		*sg_first = sg;
+	}
 
 	return 0;
 }
@@ -340,9 +410,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 	struct pxa_camera_dev *pcdev = ici->priv;
 	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
 	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", __func__,
 		vb, vb->baddr, vb->bsize);
 
@@ -379,53 +447,51 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 	}
 
 	if (vb->state == VIDEOBUF_NEEDS_INIT) {
-		unsigned int size = vb->size;
+		int size = vb->size;
+		int next_ofs = 0;
 		struct videobuf_dmabuf *dma = videobuf_to_dma(vb);
+		struct scatterlist *sg;
 
 		ret = videobuf_iolock(vq, vb, NULL);
 		if (ret)
 			goto fail;
 
 		if (pcdev->channels == 3) {
-			/* 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);
+		sg = dma->sglist;
 
+		/* init DMA for Y channel */
+		ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, size_y,
+					   &sg, &next_ofs);
 		if (ret) {
 			dev_err(pcdev->dev,
 				"DMA initialization for Y/RGB failed\n");
 			goto fail;
 		}
 
-		if (pcdev->channels == 3) {
-			/* 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 U channel */
+		if (size_u)
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1,
+						   size_u, &sg, &next_ofs);
+		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;
-			}
+		/* init DMA for V channel */
+		if (size_v)
+			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2,
+						   size_u, &sg, &next_ofs);
+		if (ret) {
+			dev_err(pcdev->dev,
+				"DMA initialization for V failed\n");
+			goto fail_v;
 		}
 
 		vb->state = VIDEOBUF_PREPARED;
-- 
1.5.6.5

[PATCH v2 3/4] pxa_camera: Redesign DMA handling

[Robert Jarzmik]

The DMA transfers in pxa_camera showed some weaknesses in
multiple queued buffers context :
 - poll/select problem
   The bug shows up with capture_example tool from v4l2 hg
   tree. The process just "stalls" on a "select timeout".

 - multiple buffers DMA starting
   When multiple buffers were queued, the DMA channels were
   always started right away. This is not optimal, as a
   special case appears when the first EOF was not yet
   reached, and the DMA channels were prematurely started.

 - Maintainability
   DMA code was a bit obfuscated. Rationalize the code to be
   easily maintainable by anyone.

 - DMA hot chaining
   DMA is not stopped anymore to queue a buffer, the buffer
   is queued with DMA running. As a tribute, a corner case
   exists where chaining happens while DMA finishes the
   chain, and the capture is restarted to deal with the
   missed link buffer.

This patch attemps to address these issues / improvements.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 Documentation/video4linux/pxa_camera.txt |  125 ++++++++++++
 drivers/media/video/pxa_camera.c         |  317 ++++++++++++++++++------------
 2 files changed, 315 insertions(+), 127 deletions(-)
 create mode 100644 Documentation/video4linux/pxa_camera.txt

diff --git a/Documentation/video4linux/pxa_camera.txt b/Documentation/video4linux/pxa_camera.txt
new file mode 100644
index 0000000..2c68c1d
--- /dev/null
+++ b/Documentation/video4linux/pxa_camera.txt
@@ -0,0 +1,125 @@
+                              PXA-Camera Host Driver
+                              ======================
+
+Constraints
+-----------
+  a) Image size for YUV422P format
+     All YUV422P images are enforced to have width x height % 16 = 0.
+     This is due to DMA constraints, which transfers only planes of 8 byte
+     multiples.
+
+
+Global video workflow
+---------------------
+  a) QIF stopped
+     Initialy, the QIF interface is stopped.
+     When a buffer is queued (pxa_videobuf_ops->buf_queue), the QIF starts.
+
+  b) QIF started
+     More buffers can be queued while the QIF is started without halting the
+     capture.  The new buffers are "appended" at the tail of the DMA chain, and
+     smoothly captured one frame after the other.
+
+     Once a buffer is filled in the the QIF interface, it is marked as "DONE"
+     and removed from the active buffers list. It can be then requeud or
+     dequeued by userland application.
+
+     Once the last buffer is filled in, the QIF interface stops.
+
+
+DMA usage
+---------
+  a) DMA flow
+     - first buffer queued for capture
+       Once a first buffer is queued for capture, the QIF is started, but data
+       transfer is not started. On "End Of Frame" interrupt, the irq handler
+       starts the DMA chain.
+     - capture of one videobuffer
+       The DMA chain starts transfering data into videobuffer RAM pages.
+       When all pages are transfered, the DMA irq is raised on "ENDINTR" status
+     - finishing one videobuffer
+       The DMA irq handler marks the videobuffer as "done", and removes it from
+       the active running queue
+       Meanwhile, the next videobuffer (if there is one), is transfered by DMA
+     - finishing the last videobuffer
+       On the DMA irq of the last videobuffer, the QIF is stopped.
+
+  b) DMA prepared buffer will have this structure
+
+     +------------+-----+---------------+-----------------+
+     | desc-sg[0] | ... | desc-sg[last] | finisher/linker |
+     +------------+-----+---------------+-----------------+
+
+     This structure is pointed by dma->sg_cpu.
+     The descriptors are used as follows :
+      - desc-sg[i]: i-th descriptor, transfering the i-th sg
+        element to the video buffer scatter gather
+      - finisher: has ddadr=DADDR_STOP, dcmd=ENDIRQEN
+      - linker: has ddadr= desc-sg[0] of next video buffer, dcmd=0
+
+     For the next schema, let's assume d0=desc-sg[0] .. dN=desc-sg[N],
+     "f" stands for finisher and "l" for linker.
+     A typical running chain is :
+
+         Videobuffer 1         Videobuffer 2
+     +---------+----+---+  +----+----+----+---+
+     | d0 | .. | dN | l |  | d0 | .. | dN | f |
+     +---------+----+-|-+  ^----+----+----+---+
+                      |    |
+                      +----+
+
+     After the chaining is finished, the chain looks like :
+
+         Videobuffer 1         Videobuffer 2         Videobuffer 3
+     +---------+----+---+  +----+----+----+---+  +----+----+----+---+
+     | d0 | .. | dN | l |  | d0 | .. | dN | l |  | d0 | .. | dN | f |
+     +---------+----+-|-+  ^----+----+----+-|-+  ^----+----+----+---+
+                      |    |                |    |
+                      +----+                +----+
+                                           new_link
+
+  c) DMA hot chaining timeslice issue
+
+     As DMA chaining is done while DMA _is_ running, the linking may be done
+     while the DMA jumps from one Videobuffer to another. On the schema, that
+     would be a problem if the following sequence is encountered :
+
+      - DMA chain is Videobuffer1 + Videobuffer2
+      - pxa_videobuf_queue() is called to queue Videobuffer3
+      - DMA controller finishes Videobuffer2, and DMA stops
+      =>
+         Videobuffer 1         Videobuffer 2
+     +---------+----+---+  +----+----+----+---+
+     | d0 | .. | dN | l |  | d0 | .. | dN | f |
+     +---------+----+-|-+  ^----+----+----+-^-+
+                      |    |                |
+                      +----+                +-- DMA DDADR loads DDADR_STOP
+
+      - pxa_dma_add_tail_buf() is called, the Videobuffer2 "finisher" is
+        replaced by a "linker" to Videobuffer3 (creation of new_link)
+      - pxa_videobuf_queue() finishes
+      - the DMA irq handler is called, which terminates Videobuffer2
+      - Videobuffer3 capture is not scheduled on DMA chain (as it stopped !!!)
+
+         Videobuffer 1         Videobuffer 2         Videobuffer 3
+     +---------+----+---+  +----+----+----+---+  +----+----+----+---+
+     | d0 | .. | dN | l |  | d0 | .. | dN | l |  | d0 | .. | dN | f |
+     +---------+----+-|-+  ^----+----+----+-|-+  ^----+----+----+---+
+                      |    |                |    |
+                      +----+                +----+
+                                           new_link
+                                          DMA DDADR still is DDADR_STOP
+
+      - pxa_camera_check_link_miss() is called
+        This checks if the DMA is finished and a buffer is still on the
+        pcdev->capture list. If that's the case, the capture will be restarted,
+        and Videobuffer3 is scheduled on DMA chain.
+      - the DMA irq handler finishes
+
+     Note: if DMA stops just after pxa_camera_check_link_miss() reads DDADR()
+     value, we have the guarantee that the DMA irq handler will be called back
+     when the DMA will finish the buffer, and pxa_camer_check_link_miss() will
+     be called again, to reschedule Videobuffer3.
+
+--
+Author: Robert Jarzmik <robert.jarzmik@free.fr>
diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index aca5374..a0ca982 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -324,7 +324,7 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen,
  * Prepares the pxa dma descriptors to transfer one camera channel.
  * Beware sg_first and sg_first_ofs are both input and output parameters.
  *
- * Returns 0
+ * Returns 0 or -ENOMEM if no coherent memory is available
  */
 static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
 				struct pxa_buffer *buf,
@@ -369,6 +369,10 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
 		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset;
 		pxa_dma->sg_cpu[i].dcmd =
 			DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len;
+#ifdef DEBUG
+		if (!i)
+			pxa_dma->sg_cpu[i].dcmd |= DCMD_STARTIRQEN;
+#endif
 		pxa_dma->sg_cpu[i].ddadr =
 			pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc);
 
@@ -402,6 +406,20 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
 	return 0;
 }
 
+static void pxa_videobuf_set_actdma(struct pxa_camera_dev *pcdev,
+				    struct pxa_buffer *buf)
+{
+	buf->active_dma = DMA_Y;
+	if (pcdev->channels == 3)
+		buf->active_dma |= DMA_U | DMA_V;
+}
+
+/*
+ * Please check the DMA prepared buffer structure in :
+ *   Documentation/video4linux/pxa_camera.txt
+ * Please check also in pxa_camera_check_link_miss() to understand why DMA chain
+ * modification while DMA chain is running will work anyway.
+ */
 static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 		struct videobuf_buffer *vb, enum v4l2_field field)
 {
@@ -498,9 +516,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
 	}
 
 	buf->inwork = 0;
-	buf->active_dma = DMA_Y;
-	if (pcdev->channels == 3)
-		buf->active_dma |= DMA_U | DMA_V;
+	pxa_videobuf_set_actdma(pcdev, buf);
 
 	return 0;
 
@@ -517,6 +533,99 @@ out:
 	return ret;
 }
 
+/**
+ * pxa_dma_start_channels - start DMA channel for active buffer
+ * @pcdev: pxa camera device
+ *
+ * Initialize DMA channels to the beginning of the active video buffer, and
+ * start these channels.
+ */
+static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev)
+{
+	int i;
+	struct pxa_buffer *active;
+
+	active = pcdev->active;
+
+	for (i = 0; i < pcdev->channels; i++) {
+		dev_dbg(pcdev->dev, "%s (channel=%d) ddadr=%08x\n", __func__,
+			i, active->dmas[i].sg_dma);
+		DDADR(pcdev->dma_chans[i]) = active->dmas[i].sg_dma;
+		DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
+	}
+}
+
+static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev)
+{
+	int i;
+
+	for (i = 0; i < pcdev->channels; i++) {
+		dev_dbg(pcdev->dev, "%s (channel=%d)\n", __func__, i);
+		DCSR(pcdev->dma_chans[i]) = 0;
+	}
+}
+
+static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev,
+				   struct pxa_buffer *buf)
+{
+	int i;
+
+	for (i = 0; i < pcdev->channels; i++)
+		pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
+}
+
+static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev,
+				 struct pxa_buffer *buf)
+{
+	int i;
+	struct pxa_dma_desc *buf_last_desc;
+
+	for (i = 0; i < pcdev->channels; i++) {
+		buf_last_desc = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
+		buf_last_desc->ddadr = DDADR_STOP;
+
+		if (!pcdev->sg_tail[i])
+			continue;
+		pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma;
+	}
+
+	pxa_dma_update_sg_tail(pcdev, buf);
+}
+
+/**
+ * pxa_camera_start_capture - start video capturing
+ * @pcdev: camera device
+ *
+ * Launch capturing. DMA channels should not be active yet. They should get
+ * activated at the end of frame interrupt, to capture only whole frames, and
+ * never begin the capture of a partial frame.
+ */
+static void pxa_camera_start_capture(struct pxa_camera_dev *pcdev)
+{
+	unsigned long cicr0, cifr;
+
+	dev_dbg(pcdev->dev, "%s\n", __func__);
+	/* Reset the FIFOs */
+	cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F;
+	__raw_writel(cifr, pcdev->base + CIFR);
+	/* Enable End-Of-Frame Interrupt */
+	cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB;
+	cicr0 &= ~CICR0_EOFM;
+	__raw_writel(cicr0, pcdev->base + CICR0);
+}
+
+static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev)
+{
+	unsigned long cicr0;
+
+	pxa_dma_stop_channels(pcdev);
+
+	cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB;
+	__raw_writel(cicr0, pcdev->base + CICR0);
+
+	dev_dbg(pcdev->dev, "%s\n", __func__);
+}
+
 static void pxa_videobuf_queue(struct videobuf_queue *vq,
 			       struct videobuf_buffer *vb)
 {
@@ -524,81 +633,20 @@ static void pxa_videobuf_queue(struct videobuf_queue *vq,
 	struct soc_camera_host *ici = 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);
-	struct pxa_buffer *active;
 	unsigned long flags;
-	int i;
 
-	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__,
-		vb, vb->baddr, vb->bsize);
+	dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d active=%p\n", __func__,
+		vb, vb->baddr, vb->bsize, pcdev->active);
+
 	spin_lock_irqsave(&pcdev->lock, flags);
 
 	list_add_tail(&vb->queue, &pcdev->capture);
 
 	vb->state = VIDEOBUF_ACTIVE;
-	active = pcdev->active;
+	pxa_dma_add_tail_buf(pcdev, buf);
 
-	if (!active) {
-		unsigned long cifr, cicr0;
-
-		cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F;
-		__raw_writel(cifr, pcdev->base + CIFR);
-
-		for (i = 0; i < pcdev->channels; i++) {
-			DDADR(pcdev->dma_chans[i]) = buf->dmas[i].sg_dma;
-			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
-			pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen - 1;
-		}
-
-		pcdev->active = buf;
-
-		cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_ENB;
-		__raw_writel(cicr0, pcdev->base + CICR0);
-	} else {
-		struct pxa_cam_dma *buf_dma;
-		struct pxa_cam_dma *act_dma;
-		int nents;
-
-		for (i = 0; i < pcdev->channels; i++) {
-			buf_dma = &buf->dmas[i];
-			act_dma = &active->dmas[i];
-			nents = buf_dma->sglen;
-
-			/* Stop DMA engine */
-			DCSR(pcdev->dma_chans[i]) = 0;
-
-			/* Add the descriptors we just initialized to
-			   the currently running chain */
-			pcdev->sg_tail[i]->ddadr = buf_dma->sg_dma;
-			pcdev->sg_tail[i] = buf_dma->sg_cpu + buf_dma->sglen - 1;
-
-			/* Setup a dummy descriptor with the DMA engines current
-			 * state
-			 */
-			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;
-		}
-	}
+	if (!pcdev->active)
+		pxa_camera_start_capture(pcdev);
 
 	spin_unlock_irqrestore(&pcdev->lock, flags);
 }
@@ -636,7 +684,7 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
 			      struct videobuf_buffer *vb,
 			      struct pxa_buffer *buf)
 {
-	unsigned long cicr0;
+	int i;
 
 	/* _init is used to debug races, see comment in pxa_camera_reqbufs() */
 	list_del_init(&vb->queue);
@@ -644,15 +692,13 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
 	do_gettimeofday(&vb->ts);
 	vb->field_count++;
 	wake_up(&vb->done);
+	dev_dbg(pcdev->dev, "%s dequeud buffer (vb=0x%p)\n", __func__, vb);
 
 	if (list_empty(&pcdev->capture)) {
+		pxa_camera_stop_capture(pcdev);
 		pcdev->active = NULL;
-		DCSR(pcdev->dma_chans[0]) = 0;
-		DCSR(pcdev->dma_chans[1]) = 0;
-		DCSR(pcdev->dma_chans[2]) = 0;
-
-		cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB;
-		__raw_writel(cicr0, pcdev->base + CICR0);
+		for (i = 0; i < pcdev->channels; i++)
+			pcdev->sg_tail[i] = NULL;
 		return;
 	}
 
@@ -660,6 +706,35 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
 				   struct pxa_buffer, vb.queue);
 }
 
+/**
+ * pxa_camera_check_link_miss - check missed DMA linking
+ * @pcdev: camera device
+ *
+ * The DMA chaining is done with DMA running. This means a tiny temporal window
+ * remains, where a buffer is queued on the chain, while the chain is already
+ * stopped. This means the tailed buffer would never be transfered by DMA.
+ * This function restarts the capture for this corner case, where :
+ *  - DADR() == DADDR_STOP
+ *  - a videobuffer is queued on the pcdev->capture list
+ *
+ * Please check the "DMA hot chaining timeslice issue" in
+ *   Documentation/video4linux/pxa_camera.txt
+ *
+ * Context: should only be called within the dma irq handler
+ */
+static void pxa_camera_check_link_miss(struct pxa_camera_dev *pcdev)
+{
+	int i, is_dma_stopped = 1;
+
+	for (i = 0; i < pcdev->channels; i++)
+		if (DDADR(pcdev->dma_chans[i]) != DDADR_STOP)
+			is_dma_stopped = 0;
+	dev_dbg(pcdev->dev, "%s : top queued buffer=%p, dma_stopped=%d\n",
+		__func__, pcdev->active, is_dma_stopped);
+	if (pcdev->active && is_dma_stopped)
+		pxa_camera_start_capture(pcdev);
+}
+
 static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
 			       enum pxa_camera_active_dma act_dma)
 {
@@ -667,19 +742,23 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
 	unsigned long flags;
 	u32 status, camera_status, overrun;
 	struct videobuf_buffer *vb;
-	unsigned long cifr, cicr0;
 
 	spin_lock_irqsave(&pcdev->lock, flags);
 
 	status = DCSR(channel);
-	DCSR(channel) = status | DCSR_ENDINTR;
+	DCSR(channel) = status;
+
+	camera_status = __raw_readl(pcdev->base + CISR);
+	overrun = CISR_IFO_0;
+	if (pcdev->channels == 3)
+		overrun |= CISR_IFO_1 | CISR_IFO_2;
 
 	if (status & DCSR_BUSERR) {
 		dev_err(pcdev->dev, "DMA Bus Error IRQ!\n");
 		goto out;
 	}
 
-	if (!(status & DCSR_ENDINTR)) {
+	if (!(status & (DCSR_ENDINTR | DCSR_STARTINTR))) {
 		dev_err(pcdev->dev, "Unknown DMA IRQ source, "
 			"status: 0x%08x\n", status);
 		goto out;
@@ -690,38 +769,28 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
 		goto out;
 	}
 
-	camera_status = __raw_readl(pcdev->base + CISR);
-	overrun = CISR_IFO_0;
-	if (pcdev->channels == 3)
-		overrun |= CISR_IFO_1 | CISR_IFO_2;
-	if (camera_status & overrun) {
-		dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n", camera_status);
-		/* Stop the Capture Interface */
-		cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB;
-		__raw_writel(cicr0, pcdev->base + CICR0);
-
-		/* Stop DMA */
-		DCSR(channel) = 0;
-		/* Reset the FIFOs */
-		cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F;
-		__raw_writel(cifr, pcdev->base + CIFR);
-		/* Enable End-Of-Frame Interrupt */
-		cicr0 &= ~CICR0_EOFM;
-		__raw_writel(cicr0, pcdev->base + CICR0);
-		/* Restart the Capture Interface */
-		__raw_writel(cicr0 | CICR0_ENB, pcdev->base + CICR0);
-		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", __func__,
-		vb, vb->baddr, vb->bsize);
 
-	buf->active_dma &= ~act_dma;
-	if (!buf->active_dma)
-		pxa_camera_wakeup(pcdev, vb, buf);
+	dev_dbg(pcdev->dev, "%s channel=%d %s%s(vb=0x%p) dma.desc=%x\n",
+		__func__, channel, status & DCSR_STARTINTR ? "SOF " : "",
+		status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel));
+
+	if (status & DCSR_ENDINTR) {
+		if (camera_status & overrun) {
+			dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n",
+				camera_status);
+			pxa_camera_stop_capture(pcdev);
+			pxa_camera_start_capture(pcdev);
+			goto out;
+		}
+		buf->active_dma &= ~act_dma;
+		if (!buf->active_dma) {
+			pxa_camera_wakeup(pcdev, vb, buf);
+			pxa_camera_check_link_miss(pcdev);
+		}
+	}
 
 out:
 	spin_unlock_irqrestore(&pcdev->lock, flags);
@@ -850,6 +919,8 @@ static irqreturn_t pxa_camera_irq(int irq, void *data)
 {
 	struct pxa_camera_dev *pcdev = data;
 	unsigned long status, cicr0;
+	struct pxa_buffer *buf;
+	struct videobuf_buffer *vb;
 
 	status = __raw_readl(pcdev->base + CISR);
 	dev_dbg(pcdev->dev, "Camera interrupt status 0x%lx\n", status);
@@ -860,12 +931,14 @@ static irqreturn_t pxa_camera_irq(int irq, void *data)
 	__raw_writel(status, pcdev->base + CISR);
 
 	if (status & CISR_EOF) {
-		int i;
-		for (i = 0; i < pcdev->channels; i++) {
-			DDADR(pcdev->dma_chans[i]) =
-				pcdev->active->dmas[i].sg_dma;
-			DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
-		}
+		pcdev->active = list_first_entry(&pcdev->capture,
+					   struct pxa_buffer, vb.queue);
+		vb = &pcdev->active->vb;
+		buf = container_of(vb, struct pxa_buffer, vb);
+		pxa_videobuf_set_actdma(pcdev, buf);
+
+		pxa_dma_start_channels(pcdev);
+
 		cicr0 = __raw_readl(pcdev->base + CICR0) | CICR0_EOFM;
 		__raw_writel(cicr0, pcdev->base + CICR0);
 	}
@@ -1417,18 +1490,8 @@ static int pxa_camera_resume(struct soc_camera_device *icd)
 		ret = pcdev->icd->ops->resume(pcdev->icd);
 
 	/* Restart frame capture if active buffer exists */
-	if (!ret && pcdev->active) {
-		unsigned long cifr, cicr0;
-
-		/* Reset the FIFOs */
-		cifr = __raw_readl(pcdev->base + CIFR) | CIFR_RESET_F;
-		__raw_writel(cifr, pcdev->base + CIFR);
-
-		cicr0 = __raw_readl(pcdev->base + CICR0);
-		cicr0 &= ~CICR0_EOFM;	/* Enable End-Of-Frame Interrupt */
-		cicr0 |= CICR0_ENB;	/* Restart the Capture Interface */
-		__raw_writel(cicr0, pcdev->base + CICR0);
-	}
+	if (!ret && pcdev->active)
+		pxa_camera_start_capture(pcdev);
 
 	return ret;
 }
-- 
1.5.6.5

[PATCH v2 4/4] pxa_camera: Fix overrun condition on last buffer

[Robert Jarzmik]

The last buffer queued will often overrun, as the DMA chain
is finished, and the time the dma irq handler is activated,
the QIF fifos are filled by the sensor.

The fix is to ignore the overrun condition on the last
queued buffer, and restart the capture only on intermediate
buffers of the chain.

Moreover, a fix was added to the very unlikely condition
where in YUV422P mode, one channel overruns while another
completes at the very same time. The capture is restarted
after the overrun as before, but the other channel
completion is now ignored.

Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
---
 drivers/media/video/pxa_camera.c |   25 ++++++++++++++++++++-----
 1 files changed, 20 insertions(+), 5 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index a0ca982..35e54fc 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -623,6 +623,7 @@ static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev)
 	cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB;
 	__raw_writel(cicr0, pcdev->base + CICR0);
 
+	pcdev->active = NULL;
 	dev_dbg(pcdev->dev, "%s\n", __func__);
 }
 
@@ -696,7 +697,6 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
 
 	if (list_empty(&pcdev->capture)) {
 		pxa_camera_stop_capture(pcdev);
-		pcdev->active = NULL;
 		for (i = 0; i < pcdev->channels; i++)
 			pcdev->sg_tail[i] = NULL;
 		return;
@@ -764,10 +764,20 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
 		goto out;
 	}
 
-	if (!pcdev->active) {
-		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
+	/*
+	 * pcdev->active should not be NULL in DMA irq handler.
+	 *
+	 * But there is one corner case : if capture was stopped due to an
+	 * overrun of channel 1, and at that same channel 2 was completed.
+	 *
+	 * When handling the overrun in DMA irq for channel 1, we'll stop the
+	 * capture and restart it (and thus set pcdev->active to NULL). But the
+	 * DMA irq handler will already be pending for channel 2. So on entering
+	 * the DMA irq handler for channel 2 there will be no active buffer, yet
+	 * that is normal.
+	 */
+	if (!pcdev->active)
 		goto out;
-	}
 
 	vb = &pcdev->active->vb;
 	buf = container_of(vb, struct pxa_buffer, vb);
@@ -778,7 +788,12 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
 		status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel));
 
 	if (status & DCSR_ENDINTR) {
-		if (camera_status & overrun) {
+		/*
+		 * It's normal if the last frame creates an overrun, as there
+		 * are no more DMA descriptors to fetch from QIF fifos
+		 */
+		if (camera_status & overrun
+		    && !list_is_last(pcdev->capture.next, &pcdev->capture)) {
 			dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n",
 				camera_status);
 			pxa_camera_stop_capture(pcdev);
-- 
1.5.6.5

Re: [PATCH v2 4/4] pxa_camera: Fix overrun condition on last buffer

[Guennadi Liakhovetski]

On Sat, 14 Mar 2009, Robert Jarzmik wrote:

> The last buffer queued will often overrun, as the DMA chain
> is finished, and the time the dma irq handler is activated,
> the QIF fifos are filled by the sensor.
> 
> The fix is to ignore the overrun condition on the last
> queued buffer, and restart the capture only on intermediate
> buffers of the chain.
> 
> Moreover, a fix was added to the very unlikely condition
> where in YUV422P mode, one channel overruns while another
> completes at the very same time. The capture is restarted
> after the overrun as before, but the other channel
> completion is now ignored.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |   25 ++++++++++++++++++++-----
>  1 files changed, 20 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index a0ca982..35e54fc 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -623,6 +623,7 @@ static void pxa_camera_stop_capture(struct pxa_camera_dev *pcdev)
>  	cicr0 = __raw_readl(pcdev->base + CICR0) & ~CICR0_ENB;
>  	__raw_writel(cicr0, pcdev->base + CICR0);
>  
> +	pcdev->active = NULL;
>  	dev_dbg(pcdev->dev, "%s\n", __func__);
>  }
>  
> @@ -696,7 +697,6 @@ static void pxa_camera_wakeup(struct pxa_camera_dev *pcdev,
>  
>  	if (list_empty(&pcdev->capture)) {
>  		pxa_camera_stop_capture(pcdev);
> -		pcdev->active = NULL;
>  		for (i = 0; i < pcdev->channels; i++)
>  			pcdev->sg_tail[i] = NULL;
>  		return;
> @@ -764,10 +764,20 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
>  		goto out;
>  	}
>  
> -	if (!pcdev->active) {
> -		dev_err(pcdev->dev, "DMA End IRQ with no active buffer!\n");
> +	/*
> +	 * pcdev->active should not be NULL in DMA irq handler.
> +	 *
> +	 * But there is one corner case : if capture was stopped due to an
> +	 * overrun of channel 1, and at that same channel 2 was completed.
> +	 *
> +	 * When handling the overrun in DMA irq for channel 1, we'll stop the
> +	 * capture and restart it (and thus set pcdev->active to NULL). But the
> +	 * DMA irq handler will already be pending for channel 2. So on entering
> +	 * the DMA irq handler for channel 2 there will be no active buffer, yet
> +	 * that is normal.
> +	 */
> +	if (!pcdev->active)
>  		goto out;
> -	}
>  
>  	vb = &pcdev->active->vb;
>  	buf = container_of(vb, struct pxa_buffer, vb);
> @@ -778,7 +788,12 @@ static void pxa_camera_dma_irq(int channel, struct pxa_camera_dev *pcdev,
>  		status & DCSR_ENDINTR ? "EOF " : "", vb, DDADR(channel));
>  
>  	if (status & DCSR_ENDINTR) {
> -		if (camera_status & overrun) {
> +		/*
> +		 * It's normal if the last frame creates an overrun, as there
> +		 * are no more DMA descriptors to fetch from QIF fifos
> +		 */
> +		if (camera_status & overrun
> +		    && !list_is_last(pcdev->capture.next, &pcdev->capture)) {
>  			dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n",

Please, put "&&" on the first line:-)

>  				camera_status);
>  			pxa_camera_stop_capture(pcdev);
> -- 
> 1.5.6.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH v2 4/4] pxa_camera: Fix overrun condition on last buffer

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> +		if (camera_status & overrun
>> +		    && !list_is_last(pcdev->capture.next, &pcdev->capture)) {
>>  			dev_dbg(pcdev->dev, "FIFO overrun! CISR: %x\n",
>
> Please, put "&&" on the first line:-)
Dammit, I thought I had them all cornered :) Will do.

Cheers.

--
Robert

Re: [PATCH v2 3/4] pxa_camera: Redesign DMA handling

[Guennadi Liakhovetski]

On Sat, 14 Mar 2009, Robert Jarzmik wrote:

> The DMA transfers in pxa_camera showed some weaknesses in
> multiple queued buffers context :
>  - poll/select problem
>    The bug shows up with capture_example tool from v4l2 hg
>    tree. The process just "stalls" on a "select timeout".
> 
>  - multiple buffers DMA starting
>    When multiple buffers were queued, the DMA channels were
>    always started right away. This is not optimal, as a
>    special case appears when the first EOF was not yet
>    reached, and the DMA channels were prematurely started.
> 
>  - Maintainability
>    DMA code was a bit obfuscated. Rationalize the code to be
>    easily maintainable by anyone.
> 
>  - DMA hot chaining
>    DMA is not stopped anymore to queue a buffer, the buffer
>    is queued with DMA running. As a tribute, a corner case
>    exists where chaining happens while DMA finishes the
>    chain, and the capture is restarted to deal with the
>    missed link buffer.
> 
> This patch attemps to address these issues / improvements.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  Documentation/video4linux/pxa_camera.txt |  125 ++++++++++++
>  drivers/media/video/pxa_camera.c         |  317 ++++++++++++++++++------------
>  2 files changed, 315 insertions(+), 127 deletions(-)
>  create mode 100644 Documentation/video4linux/pxa_camera.txt
> 
> diff --git a/Documentation/video4linux/pxa_camera.txt b/Documentation/video4linux/pxa_camera.txt
> new file mode 100644
> index 0000000..2c68c1d
> --- /dev/null
> +++ b/Documentation/video4linux/pxa_camera.txt
> @@ -0,0 +1,125 @@
> +                              PXA-Camera Host Driver
> +                              ======================
> +
> +Constraints
> +-----------
> +  a) Image size for YUV422P format
> +     All YUV422P images are enforced to have width x height % 16 = 0.
> +     This is due to DMA constraints, which transfers only planes of 8 byte
> +     multiples.
> +
> +
> +Global video workflow
> +---------------------
> +  a) QIF stopped

What is QIF? Do you mean Quick Capture Interface - QCI? I also see CIF 
used in the datasheet, probably, for "Capture InterFace," but I don't see 
QIF anywhere. Also, please explain the first time you use the 
abbreviation. Also fix it in the commit message to patch 1/4.

> +     Initialy, the QIF interface is stopped.
> +     When a buffer is queued (pxa_videobuf_ops->buf_queue), the QIF starts.
> +
> +  b) QIF started
> +     More buffers can be queued while the QIF is started without halting the
> +     capture.  The new buffers are "appended" at the tail of the DMA chain, and
> +     smoothly captured one frame after the other.
> +
> +     Once a buffer is filled in the the QIF interface, it is marked as "DONE"

duplicate "the."

> +     and removed from the active buffers list. It can be then requeud or
> +     dequeued by userland application.
> +
> +     Once the last buffer is filled in, the QIF interface stops.
> +
> +
> +DMA usage
> +---------
> +  a) DMA flow
> +     - first buffer queued for capture
> +       Once a first buffer is queued for capture, the QIF is started, but data
> +       transfer is not started. On "End Of Frame" interrupt, the irq handler
> +       starts the DMA chain.
> +     - capture of one videobuffer
> +       The DMA chain starts transfering data into videobuffer RAM pages.
> +       When all pages are transfered, the DMA irq is raised on "ENDINTR" status
> +     - finishing one videobuffer
> +       The DMA irq handler marks the videobuffer as "done", and removes it from
> +       the active running queue
> +       Meanwhile, the next videobuffer (if there is one), is transfered by DMA
> +     - finishing the last videobuffer
> +       On the DMA irq of the last videobuffer, the QIF is stopped.
> +
> +  b) DMA prepared buffer will have this structure
> +
> +     +------------+-----+---------------+-----------------+
> +     | desc-sg[0] | ... | desc-sg[last] | finisher/linker |
> +     +------------+-----+---------------+-----------------+
> +
> +     This structure is pointed by dma->sg_cpu.
> +     The descriptors are used as follows :
> +      - desc-sg[i]: i-th descriptor, transfering the i-th sg
> +        element to the video buffer scatter gather
> +      - finisher: has ddadr=DADDR_STOP, dcmd=ENDIRQEN
> +      - linker: has ddadr= desc-sg[0] of next video buffer, dcmd=0
> +
> +     For the next schema, let's assume d0=desc-sg[0] .. dN=desc-sg[N],
> +     "f" stands for finisher and "l" for linker.
> +     A typical running chain is :
> +
> +         Videobuffer 1         Videobuffer 2
> +     +---------+----+---+  +----+----+----+---+
> +     | d0 | .. | dN | l |  | d0 | .. | dN | f |
> +     +---------+----+-|-+  ^----+----+----+---+
> +                      |    |
> +                      +----+
> +
> +     After the chaining is finished, the chain looks like :
> +
> +         Videobuffer 1         Videobuffer 2         Videobuffer 3
> +     +---------+----+---+  +----+----+----+---+  +----+----+----+---+
> +     | d0 | .. | dN | l |  | d0 | .. | dN | l |  | d0 | .. | dN | f |
> +     +---------+----+-|-+  ^----+----+----+-|-+  ^----+----+----+---+
> +                      |    |                |    |
> +                      +----+                +----+
> +                                           new_link
> +
> +  c) DMA hot chaining timeslice issue
> +
> +     As DMA chaining is done while DMA _is_ running, the linking may be done
> +     while the DMA jumps from one Videobuffer to another. On the schema, that
> +     would be a problem if the following sequence is encountered :
> +
> +      - DMA chain is Videobuffer1 + Videobuffer2
> +      - pxa_videobuf_queue() is called to queue Videobuffer3
> +      - DMA controller finishes Videobuffer2, and DMA stops
> +      =>
> +         Videobuffer 1         Videobuffer 2
> +     +---------+----+---+  +----+----+----+---+
> +     | d0 | .. | dN | l |  | d0 | .. | dN | f |
> +     +---------+----+-|-+  ^----+----+----+-^-+
> +                      |    |                |
> +                      +----+                +-- DMA DDADR loads DDADR_STOP
> +
> +      - pxa_dma_add_tail_buf() is called, the Videobuffer2 "finisher" is
> +        replaced by a "linker" to Videobuffer3 (creation of new_link)
> +      - pxa_videobuf_queue() finishes
> +      - the DMA irq handler is called, which terminates Videobuffer2
> +      - Videobuffer3 capture is not scheduled on DMA chain (as it stopped !!!)
> +
> +         Videobuffer 1         Videobuffer 2         Videobuffer 3
> +     +---------+----+---+  +----+----+----+---+  +----+----+----+---+
> +     | d0 | .. | dN | l |  | d0 | .. | dN | l |  | d0 | .. | dN | f |
> +     +---------+----+-|-+  ^----+----+----+-|-+  ^----+----+----+---+
> +                      |    |                |    |
> +                      +----+                +----+
> +                                           new_link
> +                                          DMA DDADR still is DDADR_STOP
> +
> +      - pxa_camera_check_link_miss() is called
> +        This checks if the DMA is finished and a buffer is still on the
> +        pcdev->capture list. If that's the case, the capture will be restarted,
> +        and Videobuffer3 is scheduled on DMA chain.
> +      - the DMA irq handler finishes
> +
> +     Note: if DMA stops just after pxa_camera_check_link_miss() reads DDADR()
> +     value, we have the guarantee that the DMA irq handler will be called back
> +     when the DMA will finish the buffer, and pxa_camer_check_link_miss() will

pxa_camerA_check_link_miss - an "a" is missing

> +     be called again, to reschedule Videobuffer3.
> +
> +--
> +Author: Robert Jarzmik <robert.jarzmik@free.fr>
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index aca5374..a0ca982 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -324,7 +324,7 @@ static int calculate_dma_sglen(struct scatterlist *sglist, int sglen,
>   * Prepares the pxa dma descriptors to transfer one camera channel.
>   * Beware sg_first and sg_first_ofs are both input and output parameters.
>   *
> - * Returns 0
> + * Returns 0 or -ENOMEM if no coherent memory is available

As mentioned before, put in the previous patch.

>   */
>  static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
>  				struct pxa_buffer *buf,
> @@ -369,6 +369,10 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
>  		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset;
>  		pxa_dma->sg_cpu[i].dcmd =
>  			DCMD_FLOWSRC | DCMD_BURST8 | DCMD_INCTRGADDR | xfer_len;
> +#ifdef DEBUG
> +		if (!i)
> +			pxa_dma->sg_cpu[i].dcmd |= DCMD_STARTIRQEN;
> +#endif
>  		pxa_dma->sg_cpu[i].ddadr =
>  			pxa_dma->sg_dma + (i + 1) * sizeof(struct pxa_dma_desc);
>  
> @@ -402,6 +406,20 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
>  	return 0;
>  }
>  
> +static void pxa_videobuf_set_actdma(struct pxa_camera_dev *pcdev,
> +				    struct pxa_buffer *buf)
> +{
> +	buf->active_dma = DMA_Y;
> +	if (pcdev->channels == 3)
> +		buf->active_dma |= DMA_U | DMA_V;
> +}
> +
> +/*
> + * Please check the DMA prepared buffer structure in :
> + *   Documentation/video4linux/pxa_camera.txt
> + * Please check also in pxa_camera_check_link_miss() to understand why DMA chain
> + * modification while DMA chain is running will work anyway.
> + */
>  static int pxa_videobuf_prepare(struct videobuf_queue *vq,
>  		struct videobuf_buffer *vb, enum v4l2_field field)
>  {
> @@ -498,9 +516,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
>  	}
>  
>  	buf->inwork = 0;
> -	buf->active_dma = DMA_Y;
> -	if (pcdev->channels == 3)
> -		buf->active_dma |= DMA_U | DMA_V;
> +	pxa_videobuf_set_actdma(pcdev, buf);
>  
>  	return 0;
>  
> @@ -517,6 +533,99 @@ out:
>  	return ret;
>  }
>  
> +/**
> + * pxa_dma_start_channels - start DMA channel for active buffer
> + * @pcdev: pxa camera device
> + *
> + * Initialize DMA channels to the beginning of the active video buffer, and
> + * start these channels.
> + */
> +static void pxa_dma_start_channels(struct pxa_camera_dev *pcdev)
> +{
> +	int i;
> +	struct pxa_buffer *active;
> +
> +	active = pcdev->active;
> +
> +	for (i = 0; i < pcdev->channels; i++) {
> +		dev_dbg(pcdev->dev, "%s (channel=%d) ddadr=%08x\n", __func__,
> +			i, active->dmas[i].sg_dma);
> +		DDADR(pcdev->dma_chans[i]) = active->dmas[i].sg_dma;
> +		DCSR(pcdev->dma_chans[i]) = DCSR_RUN;
> +	}
> +}
> +
> +static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev)
> +{
> +	int i;
> +
> +	for (i = 0; i < pcdev->channels; i++) {
> +		dev_dbg(pcdev->dev, "%s (channel=%d)\n", __func__, i);
> +		DCSR(pcdev->dma_chans[i]) = 0;
> +	}
> +}
> +
> +static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev,
> +				   struct pxa_buffer *buf)
> +{
> +	int i;
> +
> +	for (i = 0; i < pcdev->channels; i++)
> +		pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
> +}
> +
> +static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev,
> +				 struct pxa_buffer *buf)
> +{
> +	int i;
> +	struct pxa_dma_desc *buf_last_desc;
> +
> +	for (i = 0; i < pcdev->channels; i++) {
> +		buf_last_desc = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
> +		buf_last_desc->ddadr = DDADR_STOP;
> +
> +		if (!pcdev->sg_tail[i])
> +			continue;
> +		pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma;
> +	}
> +
> +	pxa_dma_update_sg_tail(pcdev, buf);

pxa_dma_update_sg_tail is called only here, why not inline it and also put 
inside one loop?

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH v2 3/4] pxa_camera: Redesign DMA handling

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> What is QIF? Do you mean Quick Capture Interface - QCI? I also see CIF 
> used in the datasheet, probably, for "Capture InterFace," but I don't see 
> QIF anywhere. Also, please explain the first time you use the 
> abbreviation. Also fix it in the commit message to patch 1/4.
OK, will replace with QCI, my bad.

And OK for all the other typos in pxa_camera.txt

>> +static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev,
>> +				   struct pxa_buffer *buf)
>> +{
>> +	int i;
>> +
>> +	for (i = 0; i < pcdev->channels; i++)
>> +		pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
>> +}
>> +
>> +static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev,
>> +				 struct pxa_buffer *buf)
>> +{
>> +	int i;
>> +	struct pxa_dma_desc *buf_last_desc;
>> +
>> +	for (i = 0; i < pcdev->channels; i++) {
>> +		buf_last_desc = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
>> +		buf_last_desc->ddadr = DDADR_STOP;
>> +
>> +		if (!pcdev->sg_tail[i])
>> +			continue;
>> +		pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma;
>> +	}
>> +
>> +	pxa_dma_update_sg_tail(pcdev, buf);
>
> pxa_dma_update_sg_tail is called only here, why not inline it and also put 
> inside one loop?
As for the inline, I'm pretty sure you know it is automatically done by gcc.

As for moving it inside the loop, that would certainly improve performance. Yet
I find it more readable/maintainable that way, and will leave it. But I won't be
bothered at all if you retransform it back to your view, that's up to you.

Cheers.

--
Robert

Re: [PATCH v2 3/4] pxa_camera: Redesign DMA handling

[Trent Piepho]

On Mon, 16 Mar 2009, Robert Jarzmik wrote:
> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> > What is QIF? Do you mean Quick Capture Interface - QCI? I also see CIF
> > used in the datasheet, probably, for "Capture InterFace," but I don't see
> > QIF anywhere. Also, please explain the first time you use the
> > abbreviation. Also fix it in the commit message to patch 1/4.
> OK, will replace with QCI, my bad.

In video capture, QIF and CIF usually refer to image size.  CIF would be
320x240 for NTSC and QIF or QCIF would be 160x120.

[PATCH] pxa-camera: simplify the .buf_queue path by merging two loops

[Guennadi Liakhovetski]

pxa_dma_update_sg_tail() is called only once, runs exactly the same loop as the
caller and has to recalculate the last element in an sg-list, that the caller
has already calculated. Eliminate redundancy by merging the two loops and
re-using the calculated pointer. This also saves a bit of performance which is
always good during video-capture.

Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
---

On Mon, 16 Mar 2009, Robert Jarzmik wrote:

> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
> 
> > pxa_dma_update_sg_tail is called only here, why not inline it and also put 
> > inside one loop?
> As for the inline, I'm pretty sure you know it is automatically done by gcc.
> 
> As for moving it inside the loop, that would certainly improve performance. Yet
> I find it more readable/maintainable that way, and will leave it. But I won't be
> bothered at all if you retransform it back to your view, that's up to you.

Robert, this is what I'm going to apply on top of your patch-series. 
Please, object:-)

 drivers/media/video/pxa_camera.c |   20 ++++++--------------
 1 files changed, 6 insertions(+), 14 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index cfa113c..c639845 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -566,15 +566,6 @@ static void pxa_dma_stop_channels(struct pxa_camera_dev *pcdev)
 	}
 }
 
-static void pxa_dma_update_sg_tail(struct pxa_camera_dev *pcdev,
-				   struct pxa_buffer *buf)
-{
-	int i;
-
-	for (i = 0; i < pcdev->channels; i++)
-		pcdev->sg_tail[i] = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
-}
-
 static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev,
 				 struct pxa_buffer *buf)
 {
@@ -585,12 +576,13 @@ static void pxa_dma_add_tail_buf(struct pxa_camera_dev *pcdev,
 		buf_last_desc = buf->dmas[i].sg_cpu + buf->dmas[i].sglen;
 		buf_last_desc->ddadr = DDADR_STOP;
 
-		if (!pcdev->sg_tail[i])
-			continue;
-		pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma;
-	}
+		if (pcdev->sg_tail[i])
+			/* Link the new buffer to the old tail */
+			pcdev->sg_tail[i]->ddadr = buf->dmas[i].sg_dma;
 
-	pxa_dma_update_sg_tail(pcdev, buf);
+		/* Update the channel tail */
+		pcdev->sg_tail[i] = buf_last_desc;
+	}
 }
 
 /**
-- 
1.5.4

Re: [PATCH] pxa-camera: simplify the .buf_queue path by merging two loops

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

> pxa_dma_update_sg_tail() is called only once, runs exactly the same loop as the
> caller and has to recalculate the last element in an sg-list, that the caller
> has already calculated. Eliminate redundancy by merging the two loops and
> re-using the calculated pointer. This also saves a bit of performance which is
> always good during video-capture.
>
> Signed-off-by: Guennadi Liakhovetski <g.liakhovetski@gmx.de>
> ---
>
> On Mon, 16 Mar 2009, Robert Jarzmik wrote:
>
>> Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
>> 
>> > pxa_dma_update_sg_tail is called only here, why not inline it and also put 
>> > inside one loop?
>> As for the inline, I'm pretty sure you know it is automatically done by gcc.
>> 
>> As for moving it inside the loop, that would certainly improve performance. Yet
>> I find it more readable/maintainable that way, and will leave it. But I won't be
>> bothered at all if you retransform it back to your view, that's up to you.
>
> Robert, this is what I'm going to apply on top of your patch-series. 
> Please, object:-)
Here you are : :)
Acked-by: Robert Jarzmik <robert.jarzmik@free.fr>

--
Robert

Re: [PATCH v2 2/4] pxa_camera: remove YUV planar formats hole

[Guennadi Liakhovetski]

On Sat, 14 Mar 2009, Robert Jarzmik wrote:

> All planes were PAGE aligned (ie. 4096 bytes aligned). This
> is not consistent with YUV422 format, which requires Y, U
> and V planes glued together.  The new implementation forces
> the alignement on 8 bytes (DMA requirement), which is almost
> always the case (granted by width x height being a multiple
> of 8).
> 
> The test cases include tests in both YUV422 and RGB565 :
>  - a picture of size 111 x 111 (cross RAM pages example)
>  - a picture of size 1023 x 4 in (under 1 RAM page)
>  - a picture of size 1024 x 4 in (exactly 1 RAM page)
>  - a picture of size 1025 x 4 in (over 1 RAM page)
>  - a picture of size 1280 x 1024 (many RAM pages)
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |  150 +++++++++++++++++++++++++++-----------
>  1 files changed, 108 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index 8e5611b..aca5374 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -287,19 +287,63 @@ static void free_buffer(struct videobuf_queue *vq, struct pxa_buffer *buf)
>  	buf->vb.state = VIDEOBUF_NEEDS_INIT;
>  }
>  
> +static int calculate_dma_sglen(struct scatterlist *sglist, int sglen,
> +			       int sg_first_ofs, int size)
> +{
> +	int i, offset, dma_len, xfer_len;
> +	struct scatterlist *sg;
> +
> +	offset = sg_first_ofs;
> +	for_each_sg(sglist, sg, sglen, i) {
> +		dma_len = sg_dma_len(sg);
> +
> +		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
> +		xfer_len = roundup(min(dma_len - offset, size), 8);
> +
> +		size = max(0, size - xfer_len);
> +		offset = 0;
> +		if (size == 0)
> +			break;
> +	}
> +
> +	BUG_ON(size != 0);
> +	return i + 1;
> +}
> +
> +/**
> + * pxa_init_dma_channel - init dma descriptors
> + * @pcdev: pxa camera device
> + * @buf: pxa buffer to find pxa dma channel
> + * @dma: dma video buffer
> + * @channel: dma channel (0 => 'Y', 1 => 'U', 2 => 'V')
> + * @cibr: camera read fifo

I would emphasise, that this is a handle to a register, and use the name 
from the datasheet, i.e., just write " camera Receive Buffer Register."

> + * @size: bytes to transfer
> + * @sg_first: index of first element of sg_list

This is not an index any more.

> + * @sg_first_ofs: offset in first element of sg_list
> + *
> + * Prepares the pxa dma descriptors to transfer one camera channel.
> + * Beware sg_first and sg_first_ofs are both input and output parameters.
> + *
> + * Returns 0

...or -errno... - do it right straight away instead of correcting in 3/4.

> + */
>  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)
> +				int cibr, int size,
> +				struct scatterlist **sg_first, int *sg_first_ofs)
>  {
>  	struct pxa_cam_dma *pxa_dma = &buf->dmas[channel];
> -	int i;
> +	struct scatterlist *sg;
> +	int i, offset, sglen;
> +	int dma_len = 0, xfer_len = 0;
>  
>  	if (pxa_dma->sg_cpu)
>  		dma_free_coherent(pcdev->dev, pxa_dma->sg_size,
>  				  pxa_dma->sg_cpu, pxa_dma->sg_dma);
>  
> +	sglen = calculate_dma_sglen(*sg_first, dma->sglen,
> +				    *sg_first_ofs, size);
> +
>  	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);
> @@ -307,27 +351,53 @@ static int pxa_init_dma_channel(struct pxa_camera_dev *pcdev,
>  		return -ENOMEM;
>  
>  	pxa_dma->sglen = sglen;
> +	offset = *sg_first_ofs;
>  
> -	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;
> +	dev_dbg(pcdev->dev, "DMA: sg_first=%p, sglen=%d, ofs=%d, dma.desc=%x\n",
> +		*sg_first, sglen, *sg_first_ofs, pxa_dma->sg_dma);
>  
> -		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
> -		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(&sg[sg_i]);
> +
> +	for_each_sg(*sg_first, sg, sglen, i) {
> +		dma_len = sg_dma_len(sg);
>  
>  		/* PXA27x Developer's Manual 27.4.4.1: round up to 8 bytes */
> -		xfer_len = (min(dma_len, size) + 7) & ~7;
> +		xfer_len = roundup(min(dma_len - offset, size), 8);
>  
> +		size = max(0, size - xfer_len);
> +
> +		pxa_dma->sg_cpu[i].dsadr = pcdev->res->start + cibr;
> +		pxa_dma->sg_cpu[i].dtadr = sg_dma_address(sg) + offset;
>  		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);
> +
> +		dev_vdbg(pcdev->dev, "DMA: desc.%08x->@phys=0x%08x, len=%d\n",
> +			 pxa_dma->sg_dma + i * sizeof(struct pxa_dma_desc),
> +			 sg_dma_address(sg) + offset, xfer_len);
> +		offset = 0;
> +
> +		if (size == 0)
> +			break;
>  	}
>  
> -	pxa_dma->sg_cpu[sglen - 1].ddadr = DDADR_STOP;
> -	pxa_dma->sg_cpu[sglen - 1].dcmd |= DCMD_ENDIRQEN;
> +	pxa_dma->sg_cpu[sglen].ddadr = DDADR_STOP;
> +	pxa_dma->sg_cpu[sglen].dcmd  = DCMD_FLOWSRC | DCMD_BURST8 | DCMD_ENDIRQEN;

I still do not understand why are you doing this in this patch - not in 
the next one.

> +
> +	*sg_first_ofs = xfer_len;
> +	/*
> +	 * Handle 1 special case :
> +	 *  - in 3 planes (YUV422P format), we might finish with xfer_len
> +	 *    equal to dma_len (end on PAGE boundary). In this case, the sg
> +	 *    element for next plane should be the next of the last one

better "next after the last?"

> +	 *    used to store the last scatter gather RAM page
> +	 */
> +	if (*sg_first_ofs >= dma_len) {
> +		*sg_first_ofs -= dma_len;
> +		*sg_first = sg_next(sg);
> +	} else {
> +		*sg_first = sg;
> +	}

Could do a bit simpler:

+	if (xfer_len >= dma_len) {
+		*sg_first_ofs = xfer_len - dma_len;
+		*sg_first = sg_next(sg);
+	} else {
+		*sg_first_ofs = xfer_len;
+		*sg_first = sg;
+	}

>  
>  	return 0;
>  }
> @@ -340,9 +410,7 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
>  	struct pxa_camera_dev *pcdev = ici->priv;
>  	struct pxa_buffer *buf = container_of(vb, struct pxa_buffer, vb);
>  	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", __func__,
>  		vb, vb->baddr, vb->bsize);

Please, keep this empty line.

>  
> @@ -379,53 +447,51 @@ static int pxa_videobuf_prepare(struct videobuf_queue *vq,
>  	}
>  
>  	if (vb->state == VIDEOBUF_NEEDS_INIT) {
> -		unsigned int size = vb->size;
> +		int size = vb->size;
> +		int next_ofs = 0;
>  		struct videobuf_dmabuf *dma = videobuf_to_dma(vb);
> +		struct scatterlist *sg;
>  
>  		ret = videobuf_iolock(vq, vb, NULL);
>  		if (ret)
>  			goto fail;
>  
>  		if (pcdev->channels == 3) {
> -			/* 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);
> +		sg = dma->sglist;
>  
> +		/* init DMA for Y channel */
> +		ret = pxa_init_dma_channel(pcdev, buf, dma, 0, CIBR0, size_y,
> +					   &sg, &next_ofs);
>  		if (ret) {
>  			dev_err(pcdev->dev,
>  				"DMA initialization for Y/RGB failed\n");
>  			goto fail;
>  		}
>  
> -		if (pcdev->channels == 3) {
> -			/* 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 U channel */
> +		if (size_u)
> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 1, CIBR1,
> +						   size_u, &sg, &next_ofs);
> +		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;
> -			}
> +		/* init DMA for V channel */
> +		if (size_v)
> +			ret = pxa_init_dma_channel(pcdev, buf, dma, 2, CIBR2,
> +						   size_u, &sg, &next_ofs);

You meant size_v

> +		if (ret) {
> +			dev_err(pcdev->dev,
> +				"DMA initialization for V failed\n");
> +			goto fail_v;
>  		}
>  
>  		vb->state = VIDEOBUF_PREPARED;
> -- 
> 1.5.6.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH v2 2/4] pxa_camera: remove YUV planar formats hole

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:
<snip>

Basically, yes to all comments.

Cheers.

--
Robert

Re: [PATCH v2 1/4] pxa_camera: Enforce YUV422P frame sizes to be 16 multiples

[Guennadi Liakhovetski]

On Sat, 14 Mar 2009, Robert Jarzmik wrote:

> Due to DMA constraints, the DMA chain always transfers bytes
> from the QIF fifos to memory in 8 bytes units. In planar
> formats, that could mean 0 padding between Y and U plane
> (and between U and V plane), which is against YUV422P
> standard.
> 
> Therefore, a frame size is required to be a multiple of 16
> (so U plane size is a multiple of 8). It is enforced in
> try_fmt() and set_fmt() primitives, be aligning height then
> width on 4 multiples as need be, to reach a 16 multiple.
> 
> Signed-off-by: Robert Jarzmik <robert.jarzmik@free.fr>
> ---
>  drivers/media/video/pxa_camera.c |   28 +++++++++++++++++++---------
>  1 files changed, 19 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
> index e3e6b29..8e5611b 100644
> --- a/drivers/media/video/pxa_camera.c
> +++ b/drivers/media/video/pxa_camera.c
> @@ -162,6 +162,8 @@
>  			CICR0_PERRM | CICR0_QDM | CICR0_CDM | CICR0_SOFM | \
>  			CICR0_EOFM | CICR0_FOM)
>  
> +#define PIX_YUV422P_ALIGN 16	/* YUV422P pix size should be a multiple of 16 */

What is a "pix size?" Did you mean "picture size?"

> +
>  /*
>   * Structures
>   */
> @@ -241,15 +243,11 @@ static int pxa_videobuf_setup(struct videobuf_queue *vq, unsigned int *count,
>  
>  	dev_dbg(&icd->dev, "count=%d, size=%d\n", *count, *size);
>  
> -	/* planar capture requires Y, U and V buffers to be page aligned */
> -	if (pcdev->channels == 3) {
> -		*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 (pcdev->channels == 3)
> +		*size = icd->width * icd->height * 2;

This is not very obvious, why "* 2". Maybe use

pxa_camera_formats[0].depth / 8 or at least add a comment?

> +	else
> +		*size = roundup(icd->width * icd->height *
> +				((icd->current_fmt->depth + 7) >> 3), 8);
>  
>  	if (0 == *count)
>  		*count = 32;
> @@ -1234,6 +1232,18 @@ static int pxa_camera_try_fmt(struct soc_camera_device *icd,
>  		pix->width = 2048;
>  	pix->width &= ~0x01;
>  
> +	/*
> +	 * YUV422P planar format requires images size to be a 16 bytes
> +	 * multiple. If not, zeros will be inserted between Y and U planes, and
> +	 * U and V planes, and YUV422P standard would be violated.
> +	 */
> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> +			pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2);
> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> +			pix->width = ALIGN(pix->width, PIX_YUV422P_ALIGN / 2);

Shouldn't this have been sqrt(PIX_YUV422P_ALIGN) (of course, not 
literally) instead of PIX_YUV422P_ALIGN / 2? At least above you say, 
height and width shall be 4 bytes aligned, not 8.

> +	}
> +
>  	pix->bytesperline = pix->width *
>  		DIV_ROUND_UP(xlate->host_fmt->depth, 8);
>  	pix->sizeimage = pix->height * pix->bytesperline;
> -- 
> 1.5.6.5

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer

Re: [PATCH v2 1/4] pxa_camera: Enforce YUV422P frame sizes to be 16 multiples

[Robert Jarzmik]

Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes:

>> @@ -162,6 +162,8 @@
>>  			CICR0_PERRM | CICR0_QDM | CICR0_CDM | CICR0_SOFM | \
>>  			CICR0_EOFM | CICR0_FOM)
>>  
>> +#define PIX_YUV422P_ALIGN 16	/* YUV422P pix size should be a multiple of 16 */
>
> What is a "pix size?" Did you mean "picture size?"
Yes. I'll change the comment from "pix size" into "picture size"

>> -	/* planar capture requires Y, U and V buffers to be page aligned */
>> -	if (pcdev->channels == 3) {
>> -		*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 (pcdev->channels == 3)
>> +		*size = icd->width * icd->height * 2;
>
> This is not very obvious, why "* 2". Maybe use
>
> pxa_camera_formats[0].depth / 8 or at least add a comment?

Yes.
I was wondering about simplifying the if (removing it actually), and changing :
>> +	if (pcdev->channels == 3)
>> +		*size = icd->width * icd->height * 2;
>> +	else
>> +		*size = roundup(icd->width * icd->height *
>> +				((icd->current_fmt->depth + 7) >> 3), 8);
into:
	*size = roundup(icd->width * icd->height *
			((icd->current_fmt->depth + 7) >> 3), 8);

>> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
>> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
>> +			pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2);
>> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
>> +			pix->width = ALIGN(pix->width, PIX_YUV422P_ALIGN / 2);
>
> Shouldn't this have been sqrt(PIX_YUV422P_ALIGN) (of course, not 
> literally) instead of PIX_YUV422P_ALIGN / 2? At least above you say, 
> height and width shall be 4 bytes aligned, not 8.
That's a very good catch.
Maybe 2 defines will fit better, as I'm not very please with log2 logic here ... :

/*
 * YUV422P picture size should be a multiple of 16, so the heuristic aligns
 * height, width on 4 byte boundaries to reach the 16 multiple for the size.
 */
#define YUV422P_X_Y_ALIGN 4
#define YUV422P_SIZE_ALIGN YUV422P_X_Y_ALIGN * YUV422P_X_Y_ALIGN

Cheers.

--
Robert

Re: [PATCH v2 1/4] pxa_camera: Enforce YUV422P frame sizes to be 16 multiples

[Trent Piepho]

On Mon, 16 Mar 2009, Robert Jarzmik wrote:
> >> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> >> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> >> +			pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2);
> >> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> >> +			pix->width = ALIGN(pix->width, PIX_YUV422P_ALIGN / 2);
> >
> > Shouldn't this have been sqrt(PIX_YUV422P_ALIGN) (of course, not
> > literally) instead of PIX_YUV422P_ALIGN / 2? At least above you say,
> > height and width shall be 4 bytes aligned, not 8.
> That's a very good catch.
> Maybe 2 defines will fit better, as I'm not very please with log2 logic here ... :
>
> /*
>  * YUV422P picture size should be a multiple of 16, so the heuristic aligns
>  * height, width on 4 byte boundaries to reach the 16 multiple for the size.
>  */
> #define YUV422P_X_Y_ALIGN 4
> #define YUV422P_SIZE_ALIGN YUV422P_X_Y_ALIGN * YUV422P_X_Y_ALIGN

Before you spend too much time on this, maybe I could offer a patch to use
the generic alignment function I posted before?  I beleive the method in
that code will produce better results I think there are multiple drivers
that could make use of it.

Re: [PATCH v2 1/4] pxa_camera: Enforce YUV422P frame sizes to be 16 multiples

[Guennadi Liakhovetski]

On Mon, 16 Mar 2009, Trent Piepho wrote:

> On Mon, 16 Mar 2009, Robert Jarzmik wrote:
> > >> +	if (xlate->host_fmt->fourcc == V4L2_PIX_FMT_YUV422P) {
> > >> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> > >> +			pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2);
> > >> +		if (!IS_ALIGNED(pix->width * pix->height, PIX_YUV422P_ALIGN))
> > >> +			pix->width = ALIGN(pix->width, PIX_YUV422P_ALIGN / 2);
> > >
> > > Shouldn't this have been sqrt(PIX_YUV422P_ALIGN) (of course, not
> > > literally) instead of PIX_YUV422P_ALIGN / 2? At least above you say,
> > > height and width shall be 4 bytes aligned, not 8.
> > That's a very good catch.
> > Maybe 2 defines will fit better, as I'm not very please with log2 logic here ... :
> >
> > /*
> >  * YUV422P picture size should be a multiple of 16, so the heuristic aligns
> >  * height, width on 4 byte boundaries to reach the 16 multiple for the size.
> >  */
> > #define YUV422P_X_Y_ALIGN 4
> > #define YUV422P_SIZE_ALIGN YUV422P_X_Y_ALIGN * YUV422P_X_Y_ALIGN
> 
> Before you spend too much time on this, maybe I could offer a patch to use
> the generic alignment function I posted before?  I beleive the method in
> that code will produce better results I think there are multiple drivers
> that could make use of it.

As Robert already replied to you: submit your patches, get them into 
mainline, then we'll gladly switch to them if they suit our purpose.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer