| Message ID | 1236986240-24115-2-git-send-email-robert.jarzmik@free.fr (mailing list archive) |
|---|---|
| State | RFC |
| Headers | show |
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Guennadi Liakhovetski <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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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. -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 -- To unsubscribe from this list: send the line "unsubscribe linux-media" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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;
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(-)