* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole [not found] <1402002204.2045281236327939805.JavaMail.root@zimbra20-e3.priv.proxad.net> @ 2009-03-06 8:26 ` robert.jarzmik 2009-03-06 9:56 ` Trent Piepho 2009-03-06 18:55 ` Guennadi Liakhovetski 0 siblings, 2 replies; 16+ messages in thread From: robert.jarzmik @ 2009-03-06 8:26 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: mike, Linux Media Mailing List, Trent Piepho ----- Mail Original ----- De: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de> À: "Trent Piepho" <xyzzy@speakeasy.org> Cc: "Robert Jarzmik" <robert.jarzmik@free.fr>, mike@compulab.co.il, "Linux Media Mailing List" <linux-media@vger.kernel.org> Envoyé: Jeudi 5 Mars 2009 23h15:03 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne Objet: Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole > Yes, adjusting both is also what I was suggesting in my original review. > How about aligning the bigger of the two to 4 bytes and the smaller to 2? Yes, sounds good. I remade my calculations : - if (width x height % 8 == 0) : => frame size = width x height x 2 => U plane size = frame size / 4 = width x height /2 => U plane size is a multiple of 4 As the last DMA load from QIF fifo will return 8 bytes (and not 4 as we would expect, cf. PXA Developer's Manual, 27.4.4.1), this is not good. This implies that even if DMA is 8 bytes aligned, width x height should be a multiple of 16, not 8 as I stated in the first git comment. So that would align : - width on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to width) - and height on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to height) Do we have an agreement on that specification, so that I can amend the code accordingly ? Cheers. -- Robert ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-06 8:26 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole robert.jarzmik @ 2009-03-06 9:56 ` Trent Piepho 2009-03-06 18:55 ` Guennadi Liakhovetski 1 sibling, 0 replies; 16+ messages in thread From: Trent Piepho @ 2009-03-06 9:56 UTC (permalink / raw) To: robert.jarzmik; +Cc: Guennadi Liakhovetski, mike, Linux Media Mailing List [-- Warning: decoded text below may be mangled, UTF-8 assumed --] [-- Attachment #1: Type: TEXT/PLAIN; charset=X-UNKNOWN, Size: 1220 bytes --] On Fri, 6 Mar 2009 robert.jarzmik@free.fr wrote: > ----- Mail Original ----- > De: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de> > À: "Trent Piepho" <xyzzy@speakeasy.org> > Cc: "Robert Jarzmik" <robert.jarzmik@free.fr>, mike@compulab.co.il, "Linux Media Mailing List" <linux-media@vger.kernel.org> > Envoyé: Jeudi 5 Mars 2009 23h15:03 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne > Objet: Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole > > > Yes, adjusting both is also what I was suggesting in my original review. > > How about aligning the bigger of the two to 4 bytes and the smaller to 2? > > Yes, sounds good. > > I remade my calculations : > - if (width x height % 8 == 0) : > => frame size = width x height x 2 > => U plane size = frame size / 4 = width x height /2 > => U plane size is a multiple of 4 > As the last DMA load from QIF fifo will return 8 bytes (and not 4 as > we would expect, cf. PXA Developer's Manual, 27.4.4.1), this is not > good. So it's only a requirement that the total size of all three planes put together is a multiple of 8? Or is it a requirement that each plane is a multiple of 8? And are you using 4:2:2 or 4:2:0? ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-06 8:26 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole robert.jarzmik 2009-03-06 9:56 ` Trent Piepho @ 2009-03-06 18:55 ` Guennadi Liakhovetski 2009-03-06 23:12 ` Trent Piepho 2009-03-06 23:17 ` Robert Jarzmik 1 sibling, 2 replies; 16+ messages in thread From: Guennadi Liakhovetski @ 2009-03-06 18:55 UTC (permalink / raw) To: robert.jarzmik; +Cc: mike, Linux Media Mailing List, Trent Piepho On Fri, 6 Mar 2009, robert.jarzmik@free.fr wrote: > ----- Mail Original ----- > De: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de> (hm, there's something strange about this and following emails in this thread, I didn't get them directly, only over the mailing list... strange) > Ã: "Trent Piepho" <xyzzy@speakeasy.org> > Cc: "Robert Jarzmik" <robert.jarzmik@free.fr>, mike@compulab.co.il, "Linux Media Mailing List" <linux-media@vger.kernel.org> > Envoyé: Jeudi 5 Mars 2009 23h15:03 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne > Objet: Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole > > > Yes, adjusting both is also what I was suggesting in my original review. > > How about aligning the bigger of the two to 4 bytes and the smaller to 2? > > Yes, sounds good. > > I remade my calculations : > - if (width x height % 8 == 0) : > => frame size = width x height x 2 > => U plane size = frame size / 4 = width x height /2 > => U plane size is a multiple of 4 > As the last DMA load from QIF fifo will return 8 bytes (and not 4 as > we would expect, cf. PXA Developer's Manual, 27.4.4.1), this is not > good. > > This implies that even if DMA is 8 bytes aligned, width x height should > be a multiple of 16, not 8 as I stated in the first git comment. So that > would align : > - width on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to width) > - and height on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to height) > > Do we have an agreement on that specification, so that I can amend the code accordingly ? Yep, looks good to me. Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-06 18:55 ` Guennadi Liakhovetski @ 2009-03-06 23:12 ` Trent Piepho 2009-03-06 23:30 ` Robert Jarzmik 2009-03-06 23:17 ` Robert Jarzmik 1 sibling, 1 reply; 16+ messages in thread From: Trent Piepho @ 2009-03-06 23:12 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: robert.jarzmik, mike, Linux Media Mailing List On Fri, 6 Mar 2009, Guennadi Liakhovetski wrote: > On Fri, 6 Mar 2009, robert.jarzmik@free.fr wrote: > > > > This implies that even if DMA is 8 bytes aligned, width x height should > > be a multiple of 16, not 8 as I stated in the first git comment. So that > > would align : > > - width on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to width) > > - and height on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to height) > > > > Do we have an agreement on that specification, so that I can amend the code accordingly ? > > Yep, looks good to me. I like the algorithm I posted, after another small improvement, better. For instance, if width is aligned by 8 and height by 2, then you have already have 16 byte alignment and there is no need to align height by 4. E.g., 168x202 will be kept as 168x202 with my method but the rounding down method changes it to 168x200. Another example, take 159x243. My algorithm produces 160x243, which seems much better than 156x240, what one gets by rounding each dimention down to a multiple of four. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-06 23:12 ` Trent Piepho @ 2009-03-06 23:30 ` Robert Jarzmik 0 siblings, 0 replies; 16+ messages in thread From: Robert Jarzmik @ 2009-03-06 23:30 UTC (permalink / raw) To: Trent Piepho; +Cc: Guennadi Liakhovetski, mike, Linux Media Mailing List Trent Piepho <xyzzy@speakeasy.org> writes: > I like the algorithm I posted, after another small improvement, better. So push it toward v4l2, to have wider audience. If I were you, I'd have a peek at include/linux/kernel.h, which brings you beautiful functions like ALIGN(), IS_ALIGNED(), and so on ... That could make your next review easier. > For instance, if width is aligned by 8 and height by 2, then you have > already have 16 byte alignment and there is no need to align height by 4. > E.g., 168x202 will be kept as 168x202 with my method but the rounding down > method changes it to 168x200. In the algorithm I posted, I keep 168x202 as well. > Another example, take 159x243. My algorithm produces 160x243, which seems > much better than 156x240, what one gets by rounding each dimention down to > a multiple of four. By better you mean "nearer" ? Well, why not. If your patch mades it through v4l2 stack, I'll push an update to use it, deal ? -- Robert ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-06 18:55 ` Guennadi Liakhovetski 2009-03-06 23:12 ` Trent Piepho @ 2009-03-06 23:17 ` Robert Jarzmik 1 sibling, 0 replies; 16+ messages in thread From: Robert Jarzmik @ 2009-03-06 23:17 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: mike, Linux Media Mailing List, Trent Piepho Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> This implies that even if DMA is 8 bytes aligned, width x height should >> be a multiple of 16, not 8 as I stated in the first git comment. So that >> would align : >> - width on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to width) >> - and height on 4 bytes (aligning meaning the lowest multiple of 4 below or equal to height) >> >> Do we have an agreement on that specification, so that I can amend the code accordingly ? > > Yep, looks good to me. All right, this should amend patch 1/4. I'll wait for the complete review to resend the patch as a whole. Cheers -- Robert commit 4d3bd5219dd3ef27f11c7061adf10f8249d2ba26 Author: Robert Jarzmik <robert.jarzmik@free.fr> Date: Fri Mar 6 22:39:41 2009 +0100 pxa_camera: Enforce YUV422P frame sizes to be 16 multiples 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 | 19 +++++++++++++++---- 1 files changed, 15 insertions(+), 4 deletions(-) diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c index 54df071..f736f6b 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,11 +243,8 @@ 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 = roundup(icd->width * icd->height, 8) /* Y pages */ - + roundup(icd->width * icd->height / 2, 8) /* U pages */ - + roundup(icd->width * icd->height / 2, 8); /* V pages */ + *size = icd->width * icd->height * 2; else *size = roundup(icd->width * icd->height * ((icd->current_fmt->depth + 7) >> 3), 8); @@ -1297,6 +1296,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 ((pix->width * pix->height) & PIX_YUV422P_ALIGN) + pix->height = ALIGN(pix->height, PIX_YUV422P_ALIGN / 2); + if ((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; ^ permalink raw reply related [flat|nested] 16+ messages in thread
[parent not found: <421785551.2131221236346602590.JavaMail.root@zimbra20-e3.priv.proxad.net>]
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole [not found] <421785551.2131221236346602590.JavaMail.root@zimbra20-e3.priv.proxad.net> @ 2009-03-06 13:39 ` robert.jarzmik 0 siblings, 0 replies; 16+ messages in thread From: robert.jarzmik @ 2009-03-06 13:39 UTC (permalink / raw) To: Trent Piepho; +Cc: Guennadi Liakhovetski, mike, Linux Media Mailing List ----- Mail Original ----- De: "Trent Piepho" <xyzzy@speakeasy.org> À: "robert jarzmik" <robert.jarzmik@free.fr> Cc: "Guennadi Liakhovetski" <g.liakhovetski@gmx.de>, mike@compulab.co.il, "Linux Media Mailing List" <linux-media@vger.kernel.org> Envoyé: Vendredi 6 Mars 2009 10h56:39 GMT +01:00 Amsterdam / Berlin / Berne / Rome / Stockholm / Vienne Objet: Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole > I remade my calculations : > - if (width x height % 8 == 0) : > => frame size = width x height x 2 > => U plane size = frame size / 4 = width x height /2 > => U plane size is a multiple of 4 > As the last DMA load from QIF fifo will return 8 bytes (and not 4 as > we would expect, cf. PXA Developer's Manual, 27.4.4.1), this is not > good. So it's only a requirement that the total size of all three planes put together is a multiple of 8? Or is it a requirement that each plane is a multiple of 8? And are you using 4:2:2 or 4:2:0? The requirement is for each plane, as each plane has a QIF fifo associated. If the requirement is granted for plane U, then it is for plane V (same size), and plane Y as well (twice the size of plane U). And the file format is planar 4:2:2 YCbCr, which gives (assuming 32 bit wide representation, with most significant bit on the left of the schema and least on the right) for example : 31 23 15 7 0 +------+------+------+------+ | Yn+3 | Yn+2 | Yn+1 | Yn | | ..........................| + YN | YN-1 | YN-1 | 0 | +------+------+------+------+ | Cr+3 | Cr+2 | Cr+1 | Cr | | ..........................| | CrN | CrN-1| 0 | 0 | +------+------+------+------+ | Cr+3 | Cr+2 | Cr+1 | Cr | | ..........................| | CrN | CrN-1| 0 | 0 | +------+------+------+------+ Everywhere where I put a "0", de QIF interface sends a 0 byte even if it doesn't represent a pixel (padding by 0s). The same is true for RGB formats, but instead of 0, it is 0 or "transparency bit". For the height/width calculation, I didn't find your function in kernel tree. As it is very generic, I have no way to put it into pxa_camera, it should go ... elsewhere. So I think I'll use a dumb "ALIGN(x, 8)" ... Cheers. -- Robert ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 0/4] pxa_camera: Redesign DMA handling @ 2009-03-05 19:45 Robert Jarzmik 2009-03-05 19:45 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole Robert Jarzmik 0 siblings, 1 reply; 16+ messages in thread From: Robert Jarzmik @ 2009-03-05 19:45 UTC (permalink / raw) To: g.liakhovetski, mike; +Cc: linux-media, Robert Jarzmik This patchset, formerly known as "pxa_camera: Redesign DMA handling", attempts so simplify the code for all DMA related parts of pxa_camera host driver. As asked for by Guennadi and Mike, the original patch was split up into 4 patches : - one to address the YUV planar format hole (page alignment) - one to redesign the DMA - one for code style change - one for lately discovered overrun issue A decision about enforcing a size for pxa_camera_set_fmt() to be a multiple of 8 was not done yet. Meanwhile, the patchset doesn't make any hypothesis about the image size, and even a weird size like 223 x 111 will work. If such a decision was to be taken, patch 1 would have to amended. Powermanagment with suspend to RAM, then resume in the middle of a capture does work. As Mike noticed, YUV planar format overlay was not tested after these changes. Robert Jarzmik (4): pxa_camera: remove YUV planar formats hole pxa_camera: Redesign DMA handling pxa_camera: Coding style sweeping pxa_camera: Fix overrun condition on last buffer drivers/media/video/pxa_camera.c | 474 ++++++++++++++++++++++---------------- 1 files changed, 277 insertions(+), 197 deletions(-) ^ permalink raw reply [flat|nested] 16+ messages in thread
* [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 19:45 [PATCH 0/4] pxa_camera: Redesign DMA handling Robert Jarzmik @ 2009-03-05 19:45 ` Robert Jarzmik 2009-03-05 20:29 ` Guennadi Liakhovetski 2009-03-09 10:45 ` Guennadi Liakhovetski 0 siblings, 2 replies; 16+ messages in thread From: Robert Jarzmik @ 2009-03-05 19:45 UTC (permalink / raw) To: g.liakhovetski, mike; +Cc: linux-media, 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 | 165 ++++++++++++++++++++++++++------------ 1 files changed, 114 insertions(+), 51 deletions(-) diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c index e3e6b29..54df071 100644 --- a/drivers/media/video/pxa_camera.c +++ b/drivers/media/video/pxa_camera.c @@ -242,14 +242,13 @@ 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 = roundup(icd->width * icd->height, 8) /* Y pages */ + + roundup(icd->width * icd->height / 2, 8) /* U pages */ + + roundup(icd->width * icd->height / 2, 8); /* V pages */ + else + *size = roundup(icd->width * icd->height * + ((icd->current_fmt->depth + 7) >> 3), 8); if (0 == *count) *count = 32; @@ -289,19 +288,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); @@ -309,27 +352,51 @@ 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 : + * - if we finish the DMA transfer in the last 7 bytes of a RAM page + * then we return the sg element pointing on the next page + */ + if (*sg_first_ofs >= dma_len) { + *sg_first_ofs -= dma_len; + *sg_first = sg_next(sg); + } else { + *sg_first = sg; + } return 0; } @@ -342,9 +409,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; - + int size_y = 0, size_u = 0, size_v = 0; dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, vb, vb->baddr, vb->bsize); @@ -381,53 +446,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 ^ permalink raw reply related [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 19:45 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole Robert Jarzmik @ 2009-03-05 20:29 ` Guennadi Liakhovetski 2009-03-05 21:10 ` Robert Jarzmik 2009-03-09 10:45 ` Guennadi Liakhovetski 1 sibling, 1 reply; 16+ messages in thread From: Guennadi Liakhovetski @ 2009-03-05 20:29 UTC (permalink / raw) To: Robert Jarzmik; +Cc: mike, Linux Media Mailing List On Thu, 5 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). This is not a review yet - just an explanation why I was suggesting to adjust height and width - you say yourself, that YUV422P (I think, this is wat you meant, not just YUV422) requires planes to immediately follow one another. But you have to align them on 8 byte boundary for DMA, so, you violate the standard, right? If so, I would rather suggest to adjust width and height for planar formats to comply to the standard. Or have I misunderstood you? Thanks Guennadi > > 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 | 165 ++++++++++++++++++++++++++------------ > 1 files changed, 114 insertions(+), 51 deletions(-) > > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > index e3e6b29..54df071 100644 > --- a/drivers/media/video/pxa_camera.c > +++ b/drivers/media/video/pxa_camera.c > @@ -242,14 +242,13 @@ 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 = roundup(icd->width * icd->height, 8) /* Y pages */ > + + roundup(icd->width * icd->height / 2, 8) /* U pages */ > + + roundup(icd->width * icd->height / 2, 8); /* V pages */ > + else > + *size = roundup(icd->width * icd->height * > + ((icd->current_fmt->depth + 7) >> 3), 8); > > if (0 == *count) > *count = 32; > @@ -289,19 +288,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); > @@ -309,27 +352,51 @@ 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 : > + * - if we finish the DMA transfer in the last 7 bytes of a RAM page > + * then we return the sg element pointing on the next page > + */ > + if (*sg_first_ofs >= dma_len) { > + *sg_first_ofs -= dma_len; > + *sg_first = sg_next(sg); > + } else { > + *sg_first = sg; > + } > > return 0; > } > @@ -342,9 +409,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; > - > + int size_y = 0, size_u = 0, size_v = 0; > dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > vb, vb->baddr, vb->bsize); > > @@ -381,53 +446,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 > --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 20:29 ` Guennadi Liakhovetski @ 2009-03-05 21:10 ` Robert Jarzmik 2009-03-05 21:22 ` Trent Piepho 0 siblings, 1 reply; 16+ messages in thread From: Robert Jarzmik @ 2009-03-05 21:10 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: mike, Linux Media Mailing List Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > This is not a review yet - just an explanation why I was suggesting to > adjust height and width - you say yourself, that YUV422P (I think, this is > wat you meant, not just YUV422) requires planes to immediately follow one > another. But you have to align them on 8 byte boundary for DMA, so, you > violate the standard, right? If so, I would rather suggest to adjust width > and height for planar formats to comply to the standard. Or have I > misunderstood you? No, you understand perfectly. And now, what do we do : - adjust height ? - adjust height ? - adjust both ? I couldn't decide which one, any hint ? -- Robert ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 21:10 ` Robert Jarzmik @ 2009-03-05 21:22 ` Trent Piepho 2009-03-05 22:15 ` Guennadi Liakhovetski 0 siblings, 1 reply; 16+ messages in thread From: Trent Piepho @ 2009-03-05 21:22 UTC (permalink / raw) To: Robert Jarzmik; +Cc: Guennadi Liakhovetski, mike, Linux Media Mailing List On Thu, 5 Mar 2009, Robert Jarzmik wrote: > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > This is not a review yet - just an explanation why I was suggesting to > > adjust height and width - you say yourself, that YUV422P (I think, this is > > wat you meant, not just YUV422) requires planes to immediately follow one > > another. But you have to align them on 8 byte boundary for DMA, so, you > > violate the standard, right? If so, I would rather suggest to adjust width > > and height for planar formats to comply to the standard. Or have I > > misunderstood you? > No, you understand perfectly. > > And now, what do we do : > - adjust height ? > - adjust height ? > - adjust both ? > > I couldn't decide which one, any hint ? Shame the planes have to be contiguous. Software like ffmpeg doesn't require this and could handle planes with gaps between them without trouble. Plans aligned on 8 bytes boundaries would probably be faster in fact. Be better if v4l2_buffer gave us offsets for each plane. If you must adjust, probably better to adjust both. ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 21:22 ` Trent Piepho @ 2009-03-05 22:15 ` Guennadi Liakhovetski 2009-03-06 9:30 ` Trent Piepho 0 siblings, 1 reply; 16+ messages in thread From: Guennadi Liakhovetski @ 2009-03-05 22:15 UTC (permalink / raw) To: Trent Piepho; +Cc: Robert Jarzmik, mike, Linux Media Mailing List On Thu, 5 Mar 2009, Trent Piepho wrote: > On Thu, 5 Mar 2009, Robert Jarzmik wrote: > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > > > This is not a review yet - just an explanation why I was suggesting to > > > adjust height and width - you say yourself, that YUV422P (I think, this is > > > wat you meant, not just YUV422) requires planes to immediately follow one > > > another. But you have to align them on 8 byte boundary for DMA, so, you > > > violate the standard, right? If so, I would rather suggest to adjust width > > > and height for planar formats to comply to the standard. Or have I > > > misunderstood you? > > No, you understand perfectly. > > > > And now, what do we do : > > - adjust height ? > > - adjust height ? > > - adjust both ? > > > > I couldn't decide which one, any hint ? > > Shame the planes have to be contiguous. Software like ffmpeg doesn't > require this and could handle planes with gaps between them without > trouble. Plans aligned on 8 bytes boundaries would probably be faster in > fact. Be better if v4l2_buffer gave us offsets for each plane. > > If you must adjust, probably better to adjust both. Yes, adjusting both is also what I was suggesting in my original review. How about aligning the bigger of the two to 4 bytes and the smaller to 2? Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 22:15 ` Guennadi Liakhovetski @ 2009-03-06 9:30 ` Trent Piepho 0 siblings, 0 replies; 16+ messages in thread From: Trent Piepho @ 2009-03-06 9:30 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: Robert Jarzmik, mike, Linux Media Mailing List On Thu, 5 Mar 2009, Guennadi Liakhovetski wrote: > On Thu, 5 Mar 2009, Trent Piepho wrote: > > On Thu, 5 Mar 2009, Robert Jarzmik wrote: > > > Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: > > > > This is not a review yet - just an explanation why I was suggesting to > > > > adjust height and width - you say yourself, that YUV422P (I think, this is > > > > wat you meant, not just YUV422) requires planes to immediately follow one > > > > another. But you have to align them on 8 byte boundary for DMA, so, you > > > > violate the standard, right? If so, I would rather suggest to adjust width > > > > and height for planar formats to comply to the standard. Or have I > > > > misunderstood you? > > > No, you understand perfectly. > > > > > > And now, what do we do : > > > - adjust height ? > > > - adjust height ? > > > - adjust both ? > > > > > > I couldn't decide which one, any hint ? > > > > Shame the planes have to be contiguous. Software like ffmpeg doesn't > > require this and could handle planes with gaps between them without > > trouble. Plans aligned on 8 bytes boundaries would probably be faster in > > fact. Be better if v4l2_buffer gave us offsets for each plane. > > > > If you must adjust, probably better to adjust both. > > Yes, adjusting both is also what I was suggesting in my original review. > How about aligning the bigger of the two to 4 bytes and the smaller to 2? In order to 8 byte align the end of the first chroma plane, doesn't the size need to be a multiple of 32, to take into account the chroma decimation, assuming yuv 4:2:0? Here is some code I could put this code into v4l that solves this. In this case one could say: v4l_bound_align_image(*width, 48, 640, 2, *height, 32, 480, 0, 5); That will give you width between 48 and 640 that's a multiple of 4, a height between 32 and 480, and the image size will be a multiple of 32. It will try to adjust the image size as little as possible to get it to work. For example, give it 175x241 and it comes back with 176x242. As opposed to something like 192x241 or 172x240, which are also valid but more different than the requested size. /* Clamp x to be between min and max, aligned to a multiple of 2^align. min * and max don't have to be aligned, but there must be at least one valid * value. E.g., min=17,max=31,align=4 is not allowed as there are no multiples * of 16 between 17 and 31. */ static unsigned int clamp_align(unsigned int x, unsigned int min, unsigned int max, unsigned int align) { /* Bits that must be zero to be aligned */ unsigned int mask = (1 << align) - 1; /* Round to nearest aligned value */ if (x & (1 << (align - 1))) x += mask; /* make x&=~mask round up */ x &= ~mask; /* Clamp to aligned value of min and max */ if (x < min) x = (min + mask) & ~mask; else if (x > max) x = max & ~mask; return x; } /* Bound an image to have a width between wmin and wmax, and height between * hmin and hmax, inclusive. Additionally, the width will be a multiple of * 2^walign, the height will be a multiple of 2^halign, and the overall size * (width*height) will be a multiple of 2^salign. May shrink or enlarge the * image to fit the alignment constraints. The width or height maximum must * not be smaller than the corresponding minimum. The alignments must not be * so high there are no possible image sizes within the allowed bounds. */ void v4l_bound_align_image(unsigned int *width, unsigned int wmin, unsigned int wmax, unsigned int walign, unsigned int *height, unsigned int hmin, unsigned int hmax, unsigned int halign, unsigned int salign) { *width = clamp_align(*width, wmin, wmax, walign); *height = clamp_align(*height, hmin, hmax, halign); /* How much alignment do we have? */ walign = __ffs(*width); halign = __ffs(*height); /* Enough to satisfy the image alignment? */ if (walign + halign < salign) { /* Max walign where there is still a valid width */ unsigned int wmaxa = __fls(wmax ^ (wmin - 1)); salign -= walign + halign; /* up the smaller alignment until we have enough */ do { if (walign <= halign && walign < wmaxa) walign++; else halign++; } while(--salign); *width = clamp_align(*width, wmin, wmax, walign); *height = clamp_align(*height, hmin, hmax, halign); } } ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-05 19:45 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole Robert Jarzmik 2009-03-05 20:29 ` Guennadi Liakhovetski @ 2009-03-09 10:45 ` Guennadi Liakhovetski 2009-03-09 19:13 ` Robert Jarzmik 1 sibling, 1 reply; 16+ messages in thread From: Guennadi Liakhovetski @ 2009-03-09 10:45 UTC (permalink / raw) To: Robert Jarzmik; +Cc: mike, Linux Media Mailing List On Thu, 5 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 | 165 ++++++++++++++++++++++++++------------ > 1 files changed, 114 insertions(+), 51 deletions(-) > > diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c > index e3e6b29..54df071 100644 > --- a/drivers/media/video/pxa_camera.c > +++ b/drivers/media/video/pxa_camera.c > @@ -242,14 +242,13 @@ 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 = roundup(icd->width * icd->height, 8) /* Y pages */ > + + roundup(icd->width * icd->height / 2, 8) /* U pages */ > + + roundup(icd->width * icd->height / 2, 8); /* V pages */ > + else > + *size = roundup(icd->width * icd->height * > + ((icd->current_fmt->depth + 7) >> 3), 8); > > if (0 == *count) > *count = 32; Ok, this one will change I presume - new alignment calculations and line-breaking. In fact, if you adjust width and height earlier in set_fmt, maybe you'll just remove any rounding here completely. > @@ -289,19 +288,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); Ok, let's see. dma_len is PAGE_SIZE. offset is for Y-plane 0, for further planes it will be aligned after we recalculate width and height. size will be aligned too, so, roundup will disappear, right? You might want to just just add a test for these. The calculation itself gives size >= xfer_len > + > + size = max(0, size - xfer_len); So, max is useless here, just "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); > @@ -309,27 +352,51 @@ 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); Same here for roundup() and max(). > + > + 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; Why are you now always using the n+1'th element? Even if it is right, it rather belongs to the patch "2/4," not "1/4," right? In your earlier email you wrote: > - in the former pxa_videobuf_queue(), when a buffer was queued while another > was already active, a dummy descriptor was added, and then the new buffer was > chained with the actively running buffer. See code below : > > - } 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); > > The fix is in the code refactoring, as now the buffer is always added at the > tail of the queue through pxa_dma_add_tail_buf(). I don't understand, what this is fixing. It would make a nice simplification, if it worked, but see my review to patch "2/4." > + > + *sg_first_ofs = xfer_len; > + /* > + * Handle 1 special case : > + * - if we finish the DMA transfer in the last 7 bytes of a RAM page > + * then we return the sg element pointing on the next page > + */ > + if (*sg_first_ofs >= dma_len) { > + *sg_first_ofs -= dma_len; > + *sg_first = sg_next(sg); > + } else { > + *sg_first = sg; > + } As we will not be rounding up any more, this special case shouldn't be needed either, right? > > return 0; > } > @@ -342,9 +409,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; > - > + int size_y = 0, size_u = 0, size_v = 0; Isn't size_y always initialised? > dev_dbg(&icd->dev, "%s (vb=0x%p) 0x%08lx %d\n", __func__, > vb, vb->baddr, vb->bsize); > > @@ -381,53 +446,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 > Thanks Guennadi --- Guennadi Liakhovetski, Ph.D. Freelance Open-Source Software Developer ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-09 10:45 ` Guennadi Liakhovetski @ 2009-03-09 19:13 ` Robert Jarzmik 2009-03-10 18:33 ` Trent Piepho 0 siblings, 1 reply; 16+ messages in thread From: Robert Jarzmik @ 2009-03-09 19:13 UTC (permalink / raw) To: Guennadi Liakhovetski; +Cc: mike, Linux Media Mailing List Guennadi Liakhovetski <g.liakhovetski@gmx.de> writes: >> diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c >> index e3e6b29..54df071 100644 >> --- a/drivers/media/video/pxa_camera.c >> +++ b/drivers/media/video/pxa_camera.c >> @@ -242,14 +242,13 @@ 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 = roundup(icd->width * icd->height, 8) /* Y pages */ >> + + roundup(icd->width * icd->height / 2, 8) /* U pages */ >> + + roundup(icd->width * icd->height / 2, 8); /* V pages */ >> + else >> + *size = roundup(icd->width * icd->height * >> + ((icd->current_fmt->depth + 7) >> 3), 8); >> >> if (0 == *count) >> *count = 32; > > Ok, this one will change I presume - new alignment calculations and > line-breaking. In fact, if you adjust width and height earlier in set_fmt, > maybe you'll just remove any rounding here completely. Helas, not fully. The problem is with passthrough and rgb formats, where I don't enforce width/height. In the newest form of the patch I have this : if (pcdev->channels == 3) *size = icd->width * icd->height * 2; else *size = roundup(icd->width * icd->height * ((icd->current_fmt->depth + 7) >> 3), 8); > >> @@ -289,19 +288,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); > > Ok, let's see. dma_len is PAGE_SIZE. offset is for Y-plane 0, for further > planes it will be aligned after we recalculate width and height. size will > be aligned too, so, roundup will disappear, right? No, I don't think so. Consider the case of a RGB565 image which size is 223*33 = 7359 bytes. This makes a transfer of 4096 bytes and another of 3263 bytes. But the QIF fifo will give 4096 + 3264 bytes (the last one beeing 0), and this last byte has to be read from the fifo. As I understand it, the QIF fifo works with 8 bytes permutations, and that why it's giving always a multiple of 8 bytes. Please, cross-check PXA developper manual, paragraph 27.4.4.1 to see if you understand the same as I did. So the roundup() is to be kept :( > You might want to just > just add a test for these. The calculation itself gives size >= xfer_len > >> + >> + size = max(0, size - xfer_len); > > So, max is useless here, just "size -= xfer_len." And so the max() still hold I think. >> + size = max(0, size - xfer_len); > > Same here for roundup() and max(). Yes, same discussion. >> >> - 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; > > Why are you now always using the n+1'th element? Even if it is right, it > rather belongs to the patch "2/4," not "1/4," right? In your earlier email > you wrote: > >> - in the former pxa_videobuf_queue(), when a buffer was queued while another >> was already active, a dummy descriptor was added, and then the new buffer was >> chained with the actively running buffer. See code below : >> >> - } 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); >> >> The fix is in the code refactoring, as now the buffer is always added at the >> tail of the queue through pxa_dma_add_tail_buf(). > > I don't understand, what this is fixing. It would make a nice > simplification, if it worked, but see my review to patch "2/4." Let's discuss it in patch 2/4 review, yes. >> + >> + *sg_first_ofs = xfer_len; >> + /* >> + * Handle 1 special case : >> + * - if we finish the DMA transfer in the last 7 bytes of a RAM page >> + * then we return the sg element pointing on the next page >> + */ >> + if (*sg_first_ofs >= dma_len) { >> + *sg_first_ofs -= dma_len; >> + *sg_first = sg_next(sg); >> + } else { >> + *sg_first = sg; >> + } > > As we will not be rounding up any more, this special case shouldn't be > needed either, right? Same discussion as before. But here, as sg_first and sf_first_ofs only make sense for 3 planes output, and the rounding takes care of the corner cases, this part will be simplified, yes. > >> >> return 0; >> } >> @@ -342,9 +409,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; >> - >> + int size_y = 0, size_u = 0, size_v = 0; > > Isn't size_y always initialised? Yes, I'll remove that. Thanks for the review, let's keep that going on :) Cheers. -- Robert ^ permalink raw reply [flat|nested] 16+ messages in thread
* Re: [PATCH 1/4] pxa_camera: Remove YUV planar formats hole 2009-03-09 19:13 ` Robert Jarzmik @ 2009-03-10 18:33 ` Trent Piepho 0 siblings, 0 replies; 16+ messages in thread From: Trent Piepho @ 2009-03-10 18:33 UTC (permalink / raw) To: Robert Jarzmik; +Cc: Guennadi Liakhovetski, mike, Linux Media Mailing List On Mon, 9 Mar 2009, Robert Jarzmik wrote: > > Ok, this one will change I presume - new alignment calculations and > > line-breaking. In fact, if you adjust width and height earlier in set_fmt, > > maybe you'll just remove any rounding here completely. > Helas, not fully. > The problem is with passthrough and rgb formats, where I don't enforce > width/height. In the newest form of the patch I have this : > 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 icd->current_fmt->depth could be set to 16 for planar formats, then you could get rid of the special case here. ^ permalink raw reply [flat|nested] 16+ messages in thread
end of thread, other threads:[~2009-03-10 18:33 UTC | newest] Thread overview: 16+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <1402002204.2045281236327939805.JavaMail.root@zimbra20-e3.priv.proxad.net> 2009-03-06 8:26 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole robert.jarzmik 2009-03-06 9:56 ` Trent Piepho 2009-03-06 18:55 ` Guennadi Liakhovetski 2009-03-06 23:12 ` Trent Piepho 2009-03-06 23:30 ` Robert Jarzmik 2009-03-06 23:17 ` Robert Jarzmik [not found] <421785551.2131221236346602590.JavaMail.root@zimbra20-e3.priv.proxad.net> 2009-03-06 13:39 ` robert.jarzmik 2009-03-05 19:45 [PATCH 0/4] pxa_camera: Redesign DMA handling Robert Jarzmik 2009-03-05 19:45 ` [PATCH 1/4] pxa_camera: Remove YUV planar formats hole Robert Jarzmik 2009-03-05 20:29 ` Guennadi Liakhovetski 2009-03-05 21:10 ` Robert Jarzmik 2009-03-05 21:22 ` Trent Piepho 2009-03-05 22:15 ` Guennadi Liakhovetski 2009-03-06 9:30 ` Trent Piepho 2009-03-09 10:45 ` Guennadi Liakhovetski 2009-03-09 19:13 ` Robert Jarzmik 2009-03-10 18:33 ` Trent Piepho
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.