[PATCH 0/5] s5p-fimc: Misc fixes

[PATCH 0/5] s5p-fimc: Misc fixes

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 586 bytes --]

This patch series fixes several bugs found in the s5p-fimc driver. These
bugs relate to bad parameters in the formats definition and short size
of image buffers.

Nicolas Dufresne (5):
  s5p-fimc: Reuse calculated sizes
  s5p-fimc: Iterate for each memory plane
  s5p-fimc: Align imagesize to row size for tiled formats
  s5p-fimc: Fix YUV422P depth
  s5p-fimc: Changed RGB32 to BGR32

 drivers/media/platform/exynos4-is/fimc-core.c | 21 +++++++++++++++------
 drivers/media/platform/exynos4-is/fimc-m2m.c  |  6 +++---
 2 files changed, 18 insertions(+), 9 deletions(-)


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/5] s5p-fimc: Fix YUV422P depth

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 745 bytes --]

All YUV 422 has 16bit per pixels.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c
b/drivers/media/platform/exynos4-is/fimc-core.c
index bfb80fb..2e70fab 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -122,7 +122,7 @@ static struct fimc_fmt fimc_formats[] = {
 	}, {
 		.name		= "YUV 4:2:2 planar, Y/Cb/Cr",
 		.fourcc		= V4L2_PIX_FMT_YUV422P,
-		.depth		= { 12 },
+		.depth		= { 16 },
 		.color		= FIMC_FMT_YCBYCR422,
 		.memplanes	= 1,
 		.colplanes	= 3,
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/5] s5p-fimc: Align imagesize to row size for tiled formats

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1765 bytes --]

For tiled format, we need to allocated a multiple of the row size. A
good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
the previous code would only have enough space to fit half of the last
row.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index 2e70fab..0e94412 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 	for (i = 0; i < pix->num_planes; ++i) {
 		struct v4l2_plane_pix_format *plane_fmt = &pix->plane_fmt[i];
 		u32 bpl = plane_fmt->bytesperline;
+		u32 sizeimage;
 
 		if (fmt->colplanes > 1 && (bpl == 0 || bpl < pix->width))
 			bpl = pix->width; /* Planar */
@@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 			bytesperline /= 2;
 
 		plane_fmt->bytesperline = bytesperline;
-		plane_fmt->sizeimage = max((pix->width * pix->height *
-				   fmt->depth[i]) / 8, plane_fmt->sizeimage);
+		sizeimage = pix->width * pix->height * fmt->depth[i] / 8;
+
+		/* Ensure full last row for tiled formats */
+		if (tiled_fmt(fmt)) {
+			/* 64 * 32 * plane_fmt->bytesperline / 64 */
+			u32 row_size = plane_fmt->bytesperline * 32;
+			sizeimage = ALIGN(sizeimage, row_size);
+		}
+
+		plane_fmt->sizeimage = max(sizeimage, plane_fmt->sizeimage);
 	}
 }
 
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 4/5] s5p-fimc: Iterate for each memory plane

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1934 bytes --]

Depth and payload is defined per memory plane. It's better to iterate using
number of memory planes. This was not causing much issue since the rest
of the arrays involved where intialized to zero.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 drivers/media/platform/exynos4-is/fimc-m2m.c  | 4 ++--
 2 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index 0e94412..ab9450b 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -450,7 +450,7 @@ void fimc_prepare_dma_offset(struct fimc_ctx *ctx, struct fimc_frame *f)
 	bool pix_hoff = ctx->fimc_dev->drv_data->dma_pix_hoff;
 	u32 i, depth = 0;
 
-	for (i = 0; i < f->fmt->colplanes; i++)
+	for (i = 0; i < f->fmt->memplanes; i++)
 		depth += f->fmt->depth[i];
 
 	f->dma_offset.y_h = f->offs_h;
diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 36971d9..07b8f97 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -342,7 +342,7 @@ static void __set_frame_format(struct fimc_frame *frame, struct fimc_fmt *fmt,
 {
 	int i;
 
-	for (i = 0; i < fmt->colplanes; i++) {
+	for (i = 0; i < fmt->memplanes; i++) {
 		frame->bytesperline[i] = pixm->plane_fmt[i].bytesperline;
 		frame->payload[i] = pixm->plane_fmt[i].sizeimage;
 	}
@@ -461,7 +461,7 @@ static int fimc_m2m_try_crop(struct fimc_ctx *ctx, struct v4l2_crop *cr)
 	else
 		halign = ffs(fimc->variant->min_vsize_align) - 1;
 
-	for (i = 0; i < f->fmt->colplanes; i++)
+	for (i = 0; i < f->fmt->memplanes; i++)
 		depth += f->fmt->depth[i];
 
 	v4l_bound_align_image(&cr->c.width, min_size, f->o_width,
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 5/5] s5p-fimc: Reuse calculated sizes

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 958 bytes --]

This formula did not take into account the required tiled alignement for
NV12MT format. As this was already computed an stored in payload array
initially, reuse that value.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-m2m.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-m2m.c b/drivers/media/platform/exynos4-is/fimc-m2m.c
index 07b8f97..c648e5f 100644
--- a/drivers/media/platform/exynos4-is/fimc-m2m.c
+++ b/drivers/media/platform/exynos4-is/fimc-m2m.c
@@ -197,7 +197,7 @@ static int fimc_queue_setup(struct vb2_queue *vq, const struct v4l2_format *fmt,
 
 	*num_planes = f->fmt->memplanes;
 	for (i = 0; i < f->fmt->memplanes; i++) {
-		sizes[i] = (f->f_width * f->f_height * f->fmt->depth[i]) / 8;
+		sizes[i] = f->payload[i];
 		allocators[i] = ctx->fimc_dev->alloc_ctx;
 	}
 	return 0;
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 935 bytes --]

Testing showed that HW produces BGR32 rather then RGB32 as exposed
in the driver. The documentation seems to state the pixels are stored
in little endian order.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index da2fc86..bfb80fb 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -56,8 +56,8 @@ static struct fimc_fmt fimc_formats[] = {
 		.colplanes	= 1,
 		.flags		= FMT_FLAGS_M2M,
 	}, {
-		.name		= "ARGB8888, 32 bpp",
-		.fourcc		= V4L2_PIX_FMT_RGB32,
+		.name		= "BGRB888, 32 bpp",
+		.fourcc		= V4L2_PIX_FMT_BGR32,
 		.depth		= { 32 },
 		.color		= FIMC_FMT_RGB888,
 		.memplanes	= 1,
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 2/5] s5p-fimc: Fix YUV422P depth

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 744 bytes --]

All YUV 422 has 16bit per pixels.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index bfb80fb..2e70fab 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -122,7 +122,7 @@ static struct fimc_fmt fimc_formats[] = {
 	}, {
 		.name		= "YUV 4:2:2 planar, Y/Cb/Cr",
 		.fourcc		= V4L2_PIX_FMT_YUV422P,
-		.depth		= { 12 },
+		.depth		= { 16 },
 		.color		= FIMC_FMT_YCBYCR422,
 		.memplanes	= 1,
 		.colplanes	= 3,
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 3/5] s5p-fimc: Align imagesize to row size for tiled formats

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 2176 bytes --]

Le mardi 25 mars 2014 à 16:51 -0400, Nicolas Dufresne a écrit :
> For tiled format, we need to allocated a multiple of the row size. A
> good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
> this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
> the previous code would only have enough space to fit half of the last
> row.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/media/platform/exynos4-is/fimc-core.c | 13 +++++++++++--
>  1 file changed, 11 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
> index 2e70fab..0e94412 100644
> --- a/drivers/media/platform/exynos4-is/fimc-core.c
> +++ b/drivers/media/platform/exynos4-is/fimc-core.c
> @@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
>  	for (i = 0; i < pix->num_planes; ++i) {
>  		struct v4l2_plane_pix_format *plane_fmt = &pix->plane_fmt[i];
>  		u32 bpl = plane_fmt->bytesperline;
> +		u32 sizeimage;
>  
>  		if (fmt->colplanes > 1 && (bpl == 0 || bpl < pix->width))
>  			bpl = pix->width; /* Planar */
> @@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
>  			bytesperline /= 2;
>  
>  		plane_fmt->bytesperline = bytesperline;
> -		plane_fmt->sizeimage = max((pix->width * pix->height *
> -				   fmt->depth[i]) / 8, plane_fmt->sizeimage);
> +		sizeimage = pix->width * pix->height * fmt->depth[i] / 8;
> +
> +		/* Ensure full last row for tiled formats */
> +		if (tiled_fmt(fmt)) {
> +			/* 64 * 32 * plane_fmt->bytesperline / 64 */
> +			u32 row_size = plane_fmt->bytesperline * 32;
> +			sizeimage = ALIGN(sizeimage, row_size);

I made a mistake here, and it seems I've badly tested it too (was
setting the size from the test application). ALIGN won't work as
row_size is not a power of two. Sorry for that, will send an update.
roundup(sizeimage, row_size) would be the way to go. Will resubmit.

> +		}
> +
> +		plane_fmt->sizeimage = max(sizeimage, plane_fmt->sizeimage);
>  	}
>  }
>  


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 3/5] s5p-fimc: Roundup imagesize to row size for tiled formats

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 1763 bytes --]

For tiled format, we need to allocated a multiple of the row size. A
good example is for 1280x720, wich get adjusted to 1280x736. In tiles,
this mean Y plane is 20x23 and UV plane 20x12. Because of the rounding,
the previous code would only have enough space to fit half of the last
row.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 13 +++++++++++--
 1 file changed, 11 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
index 2e70fab..7ea15ef 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -736,6 +736,7 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 	for (i = 0; i < pix->num_planes; ++i) {
 		struct v4l2_plane_pix_format *plane_fmt = &pix->plane_fmt[i];
 		u32 bpl = plane_fmt->bytesperline;
+		u32 sizeimage;
 
 		if (fmt->colplanes > 1 && (bpl == 0 || bpl < pix->width))
 			bpl = pix->width; /* Planar */
@@ -755,8 +756,16 @@ void fimc_adjust_mplane_format(struct fimc_fmt *fmt, u32 width, u32 height,
 			bytesperline /= 2;
 
 		plane_fmt->bytesperline = bytesperline;
-		plane_fmt->sizeimage = max((pix->width * pix->height *
-				   fmt->depth[i]) / 8, plane_fmt->sizeimage);
+		sizeimage = pix->width * pix->height * fmt->depth[i] / 8;
+
+		/* Ensure full last row for tiled formats */
+		if (tiled_fmt(fmt)) {
+			/* 64 * 32 * plane_fmt->bytesperline / 64 */
+			u32 row_size = plane_fmt->bytesperline * 32;
+			sizeimage = roundup(sizeimage, row_size);
+		}
+
+		plane_fmt->sizeimage = max(sizeimage, plane_fmt->sizeimage);
 	}
 }
 
-- 
1.8.5.3

[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] s5p-fimc: Misc fixes

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 708 bytes --]

Any comment/input ?

Le mardi 25 mars 2014 à 16:45 -0400, Nicolas Dufresne a écrit :
> This patch series fixes several bugs found in the s5p-fimc driver. These
> bugs relate to bad parameters in the formats definition and short size
> of image buffers.
> 
> Nicolas Dufresne (5):
>   s5p-fimc: Reuse calculated sizes
>   s5p-fimc: Iterate for each memory plane
>   s5p-fimc: Align imagesize to row size for tiled formats
>   s5p-fimc: Fix YUV422P depth
>   s5p-fimc: Changed RGB32 to BGR32
> 
>  drivers/media/platform/exynos4-is/fimc-core.c | 21 +++++++++++++++------
>  drivers/media/platform/exynos4-is/fimc-m2m.c  |  6 +++---
>  2 files changed, 18 insertions(+), 9 deletions(-)
> 


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

Re: [PATCH 0/5] s5p-fimc: Misc fixes

[Sylwester Nawrocki]

Hi Nicolas,

On 01/04/14 16:13, Nicolas Dufresne wrote:
> Any comment/input ?

My apologies for the delay. The patches look good to me, I'm going
to apply them to my tree for 3.16, as the media tree is already closed
for 3.15. Thanks a lot for these fixes!

> Le mardi 25 mars 2014 à 16:45 -0400, Nicolas Dufresne a écrit :
>> This patch series fixes several bugs found in the s5p-fimc driver. These
>> bugs relate to bad parameters in the formats definition and short size
>> of image buffers.
>>
>> Nicolas Dufresne (5):
>>   s5p-fimc: Reuse calculated sizes
>>   s5p-fimc: Iterate for each memory plane
>>   s5p-fimc: Align imagesize to row size for tiled formats
>>   s5p-fimc: Fix YUV422P depth
>>   s5p-fimc: Changed RGB32 to BGR32
>>
>>  drivers/media/platform/exynos4-is/fimc-core.c | 21 +++++++++++++++------
>>  drivers/media/platform/exynos4-is/fimc-m2m.c  |  6 +++---
>>  2 files changed, 18 insertions(+), 9 deletions(-)


Regards,
Sylwester

Re: [PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Sylwester Nawrocki]

On 25/03/14 21:55, Nicolas Dufresne wrote:
> Testing showed that HW produces BGR32 rather then RGB32 as exposed
> in the driver. The documentation seems to state the pixels are stored
> in little endian order.
> 
> Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
> ---
>  drivers/media/platform/exynos4-is/fimc-core.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/platform/exynos4-is/fimc-core.c b/drivers/media/platform/exynos4-is/fimc-core.c
> index da2fc86..bfb80fb 100644
> --- a/drivers/media/platform/exynos4-is/fimc-core.c
> +++ b/drivers/media/platform/exynos4-is/fimc-core.c
> @@ -56,8 +56,8 @@ static struct fimc_fmt fimc_formats[] = {
>  		.colplanes	= 1,
>  		.flags		= FMT_FLAGS_M2M,
>  	}, {
> -		.name		= "ARGB8888, 32 bpp",
> -		.fourcc		= V4L2_PIX_FMT_RGB32,
> +		.name		= "BGRB888, 32 bpp",

It should be "BGRA8888, 32 bpp", I can fix it when applying, if
you won't send next version of this patch until then.

> +		.fourcc		= V4L2_PIX_FMT_BGR32,
>  		.depth		= { 32 },
>  		.color		= FIMC_FMT_RGB888,
>  		.memplanes	= 1,

--
Thanks!
Sylwester

Re: [PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 263 bytes --]

Le mardi 01 avril 2014 à 16:34 +0200, Sylwester Nawrocki a écrit :
> It should be "BGRA8888, 32 bpp", I can fix it when applying, if
> you won't send next version of this patch until then.

Good catch, I'll let you fix when applying.

thanks,
Nicolas


[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]

[PATCH 1/5] s5p-fimc: Changed RGB32 to BGR32

[Nicolas Dufresne]

[-- Attachment #1: Type: text/plain, Size: 936 bytes --]

Testing showed that HW produces BGR32 rather then RGB32 as exposed
in the driver. The documentation seems to state the pixels are stored
in little endian order.

Signed-off-by: Nicolas Dufresne <nicolas.dufresne@collabora.com>
---
 drivers/media/platform/exynos4-is/fimc-core.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/drivers/media/platform/exynos4-is/fimc-core.c
b/drivers/media/platform/exynos4-is/fimc-core.c
index da2fc86..bfb80fb 100644
--- a/drivers/media/platform/exynos4-is/fimc-core.c
+++ b/drivers/media/platform/exynos4-is/fimc-core.c
@@ -56,8 +56,8 @@ static struct fimc_fmt fimc_formats[] = {
 		.colplanes	= 1,
 		.flags		= FMT_FLAGS_M2M,
 	}, {
-		.name		= "ARGB8888, 32 bpp",
-		.fourcc		= V4L2_PIX_FMT_RGB32,
+		.name		= "BGRB888, 32 bpp",
+		.fourcc		= V4L2_PIX_FMT_BGR32,
 		.depth		= { 32 },
 		.color		= FIMC_FMT_RGB888,
 		.memplanes	= 1,
-- 
1.8.5.3



[-- Attachment #2: This is a digitally signed message part --]
[-- Type: application/pgp-signature, Size: 198 bytes --]