[PATCHv2 00/21] marvell-ccic: drop and fix formats

[PATCHv2 00/21] marvell-ccic: drop and fix formats

[Hans Verkuil]

This v2 patch series replaces patch 18 from the first series.

After some more testing I realized that the 422P format produced
wrong colors and I couldn't get it to work. Since it never worked and
nobody complained about it (and it is a fairly obscure format as well)
I've dropped it.

I also tested RGB444 format for the first time, and that had wrong colors
as well, but that was easy to fix. Finally there was a Bayer format
reported, but it was never implemented. So that too was dropped.

I double checked all remaining formats, and they all work fine.

Regards,

	Hans

[PATCHv2 18/21] marvell-ccic: fix Y'CbCr ordering

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

Various formats had their byte ordering implemented incorrectly, and
the V4L2_PIX_FMT_UYVY is actually impossible to create, instead you
get V4L2_PIX_FMT_YVYU.

This was working before commit ad6ac452227b7cb93ac79beec092850d178740b1
("add new formats support for marvell-ccic driver"). That commit broke
the original format support and the OLPC XO-1 laptop showed wrong
colors ever since (if you are crazy enough to attempt to run the latest
kernel on it, like I did).

The email addresses of the authors of that patch are no longer valid,
so without a way to reach them and ask them about their test setup
I am going with what I can test on the OLPC laptop.

If this breaks something for someone on their non-OLPC setup, then
contact the linux-media mailinglist. My suspicion however is that
that commit went in untested.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
Acked-by: Jonathan Corbet <corbet@lwn.net>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 14 +++++++-------
 drivers/media/platform/marvell-ccic/mcam-core.h |  8 ++++----
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 9343051..3f016fb 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -117,8 +117,8 @@ static struct mcam_format_struct {
 		.planar		= false,
 	},
 	{
-		.desc		= "UYVY 4:2:2",
-		.pixelformat	= V4L2_PIX_FMT_UYVY,
+		.desc		= "YVYU 4:2:2",
+		.pixelformat	= V4L2_PIX_FMT_YVYU,
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
 		.bpp		= 2,
 		.planar		= false,
@@ -751,7 +751,7 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 
 	switch (fmt->pixelformat) {
 	case V4L2_PIX_FMT_YUYV:
-	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_YVYU:
 		widthy = fmt->width * 2;
 		widthuv = 0;
 		break;
@@ -783,15 +783,15 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YVU420:
 		mcam_reg_write_mask(cam, REG_CTRL0,
-			C0_DF_YUV | C0_YUV_420PL | C0_YUVE_YVYU, C0_DF_MASK);
+			C0_DF_YUV | C0_YUV_420PL | C0_YUVE_VYUY, C0_DF_MASK);
 		break;
 	case V4L2_PIX_FMT_YUYV:
 		mcam_reg_write_mask(cam, REG_CTRL0,
-			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_UYVY, C0_DF_MASK);
+			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_NOSWAP, C0_DF_MASK);
 		break;
-	case V4L2_PIX_FMT_UYVY:
+	case V4L2_PIX_FMT_YVYU:
 		mcam_reg_write_mask(cam, REG_CTRL0,
-			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_YUYV, C0_DF_MASK);
+			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_SWAP24, C0_DF_MASK);
 		break;
 	case V4L2_PIX_FMT_RGB444:
 		mcam_reg_write_mask(cam, REG_CTRL0,
diff --git a/drivers/media/platform/marvell-ccic/mcam-core.h b/drivers/media/platform/marvell-ccic/mcam-core.h
index 2847e06..97167f6 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.h
+++ b/drivers/media/platform/marvell-ccic/mcam-core.h
@@ -331,10 +331,10 @@ int mccic_resume(struct mcam_camera *cam);
 #define	  C0_YUVE_YVYU	  0x00010000	/* Y1CrY0Cb		*/
 #define	  C0_YUVE_VYUY	  0x00020000	/* CrY1CbY0		*/
 #define	  C0_YUVE_UYVY	  0x00030000	/* CbY1CrY0		*/
-#define	  C0_YUVE_XYUV	  0x00000000	/* 420: .YUV		*/
-#define	  C0_YUVE_XYVU	  0x00010000	/* 420: .YVU		*/
-#define	  C0_YUVE_XUVY	  0x00020000	/* 420: .UVY		*/
-#define	  C0_YUVE_XVUY	  0x00030000	/* 420: .VUY		*/
+#define	  C0_YUVE_NOSWAP  0x00000000	/* no bytes swapping	*/
+#define	  C0_YUVE_SWAP13  0x00010000	/* swap byte 1 and 3	*/
+#define	  C0_YUVE_SWAP24  0x00020000	/* swap byte 2 and 4	*/
+#define	  C0_YUVE_SWAP1324 0x00030000	/* swap bytes 1&3 and 2&4 */
 /* Bayer bits 18,19 if needed */
 #define	  C0_EOF_VSYNC	  0x00400000	/* Generate EOF by VSYNC */
 #define	  C0_VEDGE_CTRL   0x00800000	/* Detect falling edge of VSYNC */
-- 
2.1.4

[PATCHv2 19/21] marvell-ccic: add XRGB444 and fix (X)RGB444 colors

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

After testing I discovered that the color ordering for the RGB444 format
was wrong. This is now fixed.

In addition support is added for the XRGB444 format, which is identical
to RGB444, but makes it explicit that there is no alpha, as is the
case here.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 3f016fb..17f5931 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -145,6 +145,13 @@ static struct mcam_format_struct {
 		.planar		= true,
 	},
 	{
+		.desc		= "XRGB 444",
+		.pixelformat	= V4L2_PIX_FMT_XRGB444,
+		.mbus_code	= MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE,
+		.bpp		= 2,
+		.planar		= false,
+	},
+	{
 		.desc		= "RGB 444",
 		.pixelformat	= V4L2_PIX_FMT_RGB444,
 		.mbus_code	= MEDIA_BUS_FMT_RGB444_2X8_PADHI_LE,
@@ -794,8 +801,9 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 			C0_DF_YUV | C0_YUV_PACKED | C0_YUVE_SWAP24, C0_DF_MASK);
 		break;
 	case V4L2_PIX_FMT_RGB444:
+	case V4L2_PIX_FMT_XRGB444:
 		mcam_reg_write_mask(cam, REG_CTRL0,
-			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XRGB, C0_DF_MASK);
+			C0_DF_RGB | C0_RGBF_444 | C0_RGB4_XBGR, C0_DF_MASK);
 		/* Alpha value? */
 		break;
 	case V4L2_PIX_FMT_RGB565:
-- 
2.1.4

[PATCHv2 20/21] marvell-ccic: drop bayer format

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

This format has clearly never been tested. This driver never programmed
the chip for this format and instead just reports that it is an unknown
format. Drop it from the list of formats.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 7 -------
 1 file changed, 7 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index 17f5931..c522c75 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -165,13 +165,6 @@ static struct mcam_format_struct {
 		.bpp		= 2,
 		.planar		= false,
 	},
-	{
-		.desc		= "Raw RGB Bayer",
-		.pixelformat	= V4L2_PIX_FMT_SBGGR8,
-		.mbus_code	= MEDIA_BUS_FMT_SBGGR8_1X8,
-		.bpp		= 1,
-		.planar		= false,
-	},
 };
 #define N_MCAM_FMTS ARRAY_SIZE(mcam_formats)
 
-- 
2.1.4

[PATCHv2 21/21] marvell-ccic: drop support for PIX_FMT_422P

[Hans Verkuil]

From: Hans Verkuil <hans.verkuil@cisco.com>

I cannot get this format to work, the colors keep coming out wrong.
Since this has never worked I just drop support for this.

Signed-off-by: Hans Verkuil <hans.verkuil@cisco.com>
---
 drivers/media/platform/marvell-ccic/mcam-core.c | 19 -------------------
 1 file changed, 19 deletions(-)

diff --git a/drivers/media/platform/marvell-ccic/mcam-core.c b/drivers/media/platform/marvell-ccic/mcam-core.c
index c522c75..d588445 100644
--- a/drivers/media/platform/marvell-ccic/mcam-core.c
+++ b/drivers/media/platform/marvell-ccic/mcam-core.c
@@ -124,13 +124,6 @@ static struct mcam_format_struct {
 		.planar		= false,
 	},
 	{
-		.desc		= "YUV 4:2:2 PLANAR",
-		.pixelformat	= V4L2_PIX_FMT_YUV422P,
-		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
-		.bpp		= 1,
-		.planar		= true,
-	},
-	{
 		.desc		= "YUV 4:2:0 PLANAR",
 		.pixelformat	= V4L2_PIX_FMT_YUV420,
 		.mbus_code	= MEDIA_BUS_FMT_YUYV8_2X8,
@@ -352,10 +345,6 @@ static void mcam_write_yuv_bases(struct mcam_camera *cam,
 	y = base;
 
 	switch (fmt->pixelformat) {
-	case V4L2_PIX_FMT_YUV422P:
-		u = y + pixel_count;
-		v = u + pixel_count / 2;
-		break;
 	case V4L2_PIX_FMT_YUV420:
 		u = y + pixel_count;
 		v = u + pixel_count / 4;
@@ -755,7 +744,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 		widthy = fmt->width * 2;
 		widthuv = 0;
 		break;
-	case V4L2_PIX_FMT_YUV422P:
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YVU420:
 		widthy = fmt->width;
@@ -776,10 +764,6 @@ static void mcam_ctlr_image(struct mcam_camera *cam)
 	 * Tell the controller about the image format we are using.
 	 */
 	switch (fmt->pixelformat) {
-	case V4L2_PIX_FMT_YUV422P:
-		mcam_reg_write_mask(cam, REG_CTRL0,
-			C0_DF_YUV | C0_YUV_PLANAR | C0_YUVE_YVYU, C0_DF_MASK);
-		break;
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YVU420:
 		mcam_reg_write_mask(cam, REG_CTRL0,
@@ -1374,9 +1358,6 @@ static int mcam_vidioc_try_fmt_vid_cap(struct file *filp, void *priv,
 	v4l2_fill_pix_format(pix, &mbus_fmt);
 	pix->bytesperline = pix->width * f->bpp;
 	switch (f->pixelformat) {
-	case V4L2_PIX_FMT_YUV422P:
-		pix->sizeimage = pix->height * pix->bytesperline * 2;
-		break;
 	case V4L2_PIX_FMT_YUV420:
 	case V4L2_PIX_FMT_YVU420:
 		pix->sizeimage = pix->height * pix->bytesperline * 3 / 2;
-- 
2.1.4

Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats

[Jonathan Corbet]

On Wed, 11 Mar 2015 09:10:24 +0100
Hans Verkuil <hverkuil@xs4all.nl> wrote:

> After some more testing I realized that the 422P format produced
> wrong colors and I couldn't get it to work. Since it never worked and
> nobody complained about it (and it is a fairly obscure format as well)
> I've dropped it.

I'm not sure how that format came in anymore; I didn't add it.  No
objections to its removal.

> I also tested RGB444 format for the first time, and that had wrong colors
> as well, but that was easy to fix. Finally there was a Bayer format
> reported, but it was never implemented. So that too was dropped.

The RGB444 change worries me somewhat; that was the default format on the
XO1 and worked for years.  I vaguely remember some discussions about the
ordering of the colors there, but that was a while ago.  Did you test it
with any of the Sugar apps?

In the end, correctness is probably the right way to go (it usually is!),
but I'd hate to get a regression report from somebody who is crazy enough
to put current kernels on those machines.  Fortunately, such people
should be rare.

Bayer sort-of worked once, honest.  I added it for some academic who
wanted to do stuff, and was never really able to close the loop on
getting it working correctly.  It might be worth removing the alleged
support from ov7670 as well.

In any case, for all of them:

Acked-by: Jonathan Corbet <corbet@lwn.net>

jon

Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats

[Hans Verkuil]

Hi Jon,

On 03/13/2015 10:28 PM, Jonathan Corbet wrote:
> On Wed, 11 Mar 2015 09:10:24 +0100
> Hans Verkuil <hverkuil@xs4all.nl> wrote:
> 
>> After some more testing I realized that the 422P format produced
>> wrong colors and I couldn't get it to work. Since it never worked and
>> nobody complained about it (and it is a fairly obscure format as well)
>> I've dropped it.
> 
> I'm not sure how that format came in anymore; I didn't add it.  No
> objections to its removal.

It came in with the patches from Marvell.

>> I also tested RGB444 format for the first time, and that had wrong colors
>> as well, but that was easy to fix. Finally there was a Bayer format
>> reported, but it was never implemented. So that too was dropped.
> 
> The RGB444 change worries me somewhat; that was the default format on the
> XO1 and worked for years.  I vaguely remember some discussions about the
> ordering of the colors there, but that was a while ago.  Did you test it
> with any of the Sugar apps?

I've tested with the 'Record' app, and that picks a YUV format, not RGB444.
Are there other apps that I can test with where you can select the capture
format?

> In the end, correctness is probably the right way to go (it usually is!),
> but I'd hate to get a regression report from somebody who is crazy enough
> to put current kernels on those machines.  Fortunately, such people
> should be rare.
> 
> Bayer sort-of worked once, honest.  I added it for some academic who
> wanted to do stuff, and was never really able to close the loop on
> getting it working correctly.  It might be worth removing the alleged
> support from ov7670 as well.

I might give it try to get it to work. I'm in the process of adding Bayer
support to the vivid driver, which makes it easier to test. I'll see if
I have some time this weekend.

> In any case, for all of them:
> 
> Acked-by: Jonathan Corbet <corbet@lwn.net>

Thanks!

	Hans

Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats

[Hans Verkuil]

On 03/13/2015 10:59 PM, Hans Verkuil wrote:
> Hi Jon,
> 
> On 03/13/2015 10:28 PM, Jonathan Corbet wrote:
>> On Wed, 11 Mar 2015 09:10:24 +0100
>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>
>>> After some more testing I realized that the 422P format produced
>>> wrong colors and I couldn't get it to work. Since it never worked and
>>> nobody complained about it (and it is a fairly obscure format as well)
>>> I've dropped it.
>>
>> I'm not sure how that format came in anymore; I didn't add it.  No
>> objections to its removal.
> 
> It came in with the patches from Marvell.
> 
>>> I also tested RGB444 format for the first time, and that had wrong colors
>>> as well, but that was easy to fix. Finally there was a Bayer format
>>> reported, but it was never implemented. So that too was dropped.
>>
>> The RGB444 change worries me somewhat; that was the default format on the
>> XO1 and worked for years.  I vaguely remember some discussions about the
>> ordering of the colors there, but that was a while ago.  Did you test it
>> with any of the Sugar apps?
> 
> I've tested with the 'Record' app, and that picks a YUV format, not RGB444.
> Are there other apps that I can test with where you can select the capture
> format?

Urgh. I did some more digging and this driver really supported a big endian
version of RGB444. So the description in the documentation of the RGB444
format and what this driver returns is different.

It looks like Michael Schimek's question regarding endianness went unanswered:
http://www.spinics.net/lists/vfl/msg28921.html

He probably assumed the same order as for RGB555/565 formats.

I have three options:

1) fix the driver as I did in my patch so RGB444 follows the documentation.
2) add a new RGB444X big endian pixel format and switch the driver to that.
   So RGB444 is no longer supported, instead RGB444X is now supported. Apps
   will have to change PIX_FMT_RGB444 to PIX_FMT_RGB444X.
3) add support for both RGB444 and RGB444X to the driver.

Note that it is not possible to change the RGB444 documentation since this
format is used in other drivers as well, and there it is in proper little
endian format.

I am actually favoring option 2, since that prevents current applications
using RGB444 from working with the new kernel, but it is easy to fix by
changing RGB444 to RGB444X. OLPC specific apps can even just assume that
RGB444 and RGB444X are the same, and so they will work with both the new
and old driver.

Let me know what you prefer.

Regards,

	Hans

Re: [PATCHv2 00/21] marvell-ccic: drop and fix formats

[Hans Verkuil]

On 03/14/2015 10:15 AM, Hans Verkuil wrote:
> On 03/13/2015 10:59 PM, Hans Verkuil wrote:
>> Hi Jon,
>>
>> On 03/13/2015 10:28 PM, Jonathan Corbet wrote:
>>> On Wed, 11 Mar 2015 09:10:24 +0100
>>> Hans Verkuil <hverkuil@xs4all.nl> wrote:
>>>
>>>> After some more testing I realized that the 422P format produced
>>>> wrong colors and I couldn't get it to work. Since it never worked and
>>>> nobody complained about it (and it is a fairly obscure format as well)
>>>> I've dropped it.
>>>
>>> I'm not sure how that format came in anymore; I didn't add it.  No
>>> objections to its removal.
>>
>> It came in with the patches from Marvell.
>>
>>>> I also tested RGB444 format for the first time, and that had wrong colors
>>>> as well, but that was easy to fix. Finally there was a Bayer format
>>>> reported, but it was never implemented. So that too was dropped.
>>>
>>> The RGB444 change worries me somewhat; that was the default format on the
>>> XO1 and worked for years.  I vaguely remember some discussions about the
>>> ordering of the colors there, but that was a while ago.  Did you test it
>>> with any of the Sugar apps?
>>
>> I've tested with the 'Record' app, and that picks a YUV format, not RGB444.
>> Are there other apps that I can test with where you can select the capture
>> format?
> 
> Urgh. I did some more digging and this driver really supported a big endian
> version of RGB444. So the description in the documentation of the RGB444
> format and what this driver returns is different.

OK, after some more research it turns out that it is not actually the big
endian format at all.

This is the actual format compared to RGB444 and big endian RGB444:

		Byte 0 in memory	Byte 1 in memory
RGB444		ggggbbbb		xxxxrrrr
RGB444 BE	xxxxrrrr		ggggbbbb
Marvell RGB444	ggggrrrr		xxxxbbbb

Basically RGB444 but with R and B reversed.

> 
> It looks like Michael Schimek's question regarding endianness went unanswered:
> http://www.spinics.net/lists/vfl/msg28921.html
> 
> He probably assumed the same order as for RGB555/565 formats.
> 
> I have three options:
> 
> 1) fix the driver as I did in my patch so RGB444 follows the documentation.
> 2) add a new RGB444X big endian pixel format and switch the driver to that.
>    So RGB444 is no longer supported, instead RGB444X is now supported. Apps
>    will have to change PIX_FMT_RGB444 to PIX_FMT_RGB444X.
> 3) add support for both RGB444 and RGB444X to the driver.

I stick with this proposal, except instead of an RGB444X (big endian) version
I create a BGR444 format.

BTW, I'm using this code written for the OLPC as a reference as to what apps
expect:

https://bitbucket.org/pygame/pygame/src/db67108d6a8e6064884549f471c22fab21bdbfa6/src/_camera.c?at=default

At line 491-495 it is clear what format is expected.

Regards,

	Hans

> 
> Note that it is not possible to change the RGB444 documentation since this
> format is used in other drivers as well, and there it is in proper little
> endian format.
> 
> I am actually favoring option 2, since that prevents current applications
> using RGB444 from working with the new kernel, but it is easy to fix by
> changing RGB444 to RGB444X. OLPC specific apps can even just assume that
> RGB444 and RGB444X are the same, and so they will work with both the new
> and old driver.
> 
> Let me know what you prefer.
> 
> Regards,
> 
> 	Hans
>