mt9m111 swap_rgb_red_blue

mt9m111 swap_rgb_red_blue

[Sascha Hauer]

Hi,

The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is
hardcoded to 1. This results in, well the name says it, red and blue being
swapped in my picture.
Is this value needed on some boards or is it just a leftover from
development?

Sascha

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: mt9m111 swap_rgb_red_blue

[Sascha Hauer]

On Wed, May 26, 2010 at 10:19:21PM +0200, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > Hi,
> >
> > The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is
> > hardcoded to 1. This results in, well the name says it, red and blue being
> > swapped in my picture.
> > Is this value needed on some boards or is it just a leftover from
> > development?
> 
> Hi Sascha,
> 
> It's not a development leftover, it's something that the sensor and the host
> have to agree upon (ie. agree upon the output the sensor has to deliver to the
> host).
> 
> By now, only the Marvell PXA27x CPU was used as the host of this sensor, and the
> PXA expects the inverted Red/Blue order (ie. have BGR format).
> 
> Now, for the solution to your problem, we could :
>  - enhance the mt9m111, and add the V4L2_MBUS_FMT_BGR565_2X8_LE format
>    This format would have swap_rgb_red_blue = 1
>  - fix the mt9m111, and add for the V4L2_MBUS_FMT_BGR565_2X8_LE format
>    swap_rgb_red_blue = 0

You mean V4L2_MBUS_FMT_RGB565_2X8_LE => swap_rgb_red_blue = 0 ?


>  - fix the pxa_camera, and convert the RGB format asked for by userland into the
>  V4L2_MBUS_FMT_BGR565_2X8_LE
> 
> What is your opinion here, Guennadi ?
> 
> --
> Robert
> 
> PS: As for now, the RGB565 format is transfered as follows from the sensor, for
> one pixel, over a 8 bit bus (D7-D0):
> 
>        D7 D6 D5 D4 D3 D2 D1 D0
>        =======================
> Byte1: G4 G3 G2 R7 R6 R5 R4 R3
> Byte2: B7 B6 B5 B4 B3 G7 G6 G5
> 
> This is BGR565, with byte1 and byte2 inverted.
> 
> 
> PPS: This is what Sascha is expecting, if I understood correctly:
> 
>        D7 D6 D5 D4 D3 D2 D1 D0
>        =======================
> Byte1: G4 G3 G2 B7 B6 B5 B4 B3
> Byte2: R7 R6 R5 R4 R3 G7 G6 G5
> 
> This is RGB565, with byte1 and byte2 inverted.

Yes, that's what I need

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: mt9m111 swap_rgb_red_blue

[Guennadi Liakhovetski]

On Wed, 26 May 2010, Robert Jarzmik wrote:

> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > Hi,
> >
> > The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is
> > hardcoded to 1. This results in, well the name says it, red and blue being
> > swapped in my picture.
> > Is this value needed on some boards or is it just a leftover from
> > development?
> 
> Hi Sascha,
> 
> It's not a development leftover, it's something that the sensor and the host
> have to agree upon (ie. agree upon the output the sensor has to deliver to the
> host).
> 
> By now, only the Marvell PXA27x CPU was used as the host of this sensor, and the
> PXA expects the inverted Red/Blue order (ie. have BGR format).
> 
> Now, for the solution to your problem, we could :
>  - enhance the mt9m111, and add the V4L2_MBUS_FMT_BGR565_2X8_LE format
>    This format would have swap_rgb_red_blue = 1
>  - fix the mt9m111, and add for the V4L2_MBUS_FMT_BGR565_2X8_LE format
>    swap_rgb_red_blue = 0
>  - fix the pxa_camera, and convert the RGB format asked for by userland into the
>  V4L2_MBUS_FMT_BGR565_2X8_LE
> 
> What is your opinion here, Guennadi ?
> 
> --
> Robert
> 
> PS: As for now, the RGB565 format is transfered as follows from the sensor, for
> one pixel, over a 8 bit bus (D7-D0):
> 
>        D7 D6 D5 D4 D3 D2 D1 D0
>        =======================
> Byte1: G4 G3 G2 R7 R6 R5 R4 R3
> Byte2: B7 B6 B5 B4 B3 G7 G6 G5
> 
> This is BGR565, with byte1 and byte2 inverted.

"inverted" has to be explained here, I think. So, an BGR565 is a 16-bit 
word like (using your notation)

High byte                  | Low byte
bit15                      |                      bit0
   b7 b6 b5 b4 b3 g7 g6 g5 | g4 g3 g2 r7 r6 r5 r4 r3

on a LE machine this will be stored in RAM as

address 0 | address 1
Low byte  | High byte

So, if we take a "natural pass-through" packing as

Byte1 -> address 0
Byte2 -> address 1

Then your table above is a V4L2_MBUS_FMT_BGR565_2X8_LE format. Agree? So, 
that's what you get with "swap_rgb_red_blue = 1." Now, this flag actually 
swaps the colour components, not the bytes, right? With "swap_rgb_red_blue 
= 0" you'd get V4L2_MBUS_FMT_BGR565_2X8_BE. So, yes, I agree, that 
you have to extend the mt9m111 driver to support both these formats by 
toggling that bit, and yes, you have to replace *RGB* formats with *BGR* 
analogs in both mt9m111 and pxa drivers, because that's what we actually 
have, right? And, while at it, we should extend mt9m111 to handle the 
swap_rgb_red_blue flag to also provide *RGB* formats.

> PPS: This is what Sascha is expecting, if I understood correctly:
> 
>        D7 D6 D5 D4 D3 D2 D1 D0
>        =======================
> Byte1: G4 G3 G2 B7 B6 B5 B4 B3
> Byte2: R7 R6 R5 R4 R3 G7 G6 G5
> 
> This is RGB565, with byte1 and byte2 inverted.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: mt9m111 swap_rgb_red_blue

[Sascha Hauer]

On Thu, May 27, 2010 at 11:45:25AM +0200, Guennadi Liakhovetski wrote:
> On Wed, 26 May 2010, Robert Jarzmik wrote:
> 
> > Sascha Hauer <s.hauer@pengutronix.de> writes:
> > 
> > > Hi,
> > >
> > > The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is
> > > hardcoded to 1. This results in, well the name says it, red and blue being
> > > swapped in my picture.
> > > Is this value needed on some boards or is it just a leftover from
> > > development?
> > 
> > Hi Sascha,
> > 
> > It's not a development leftover, it's something that the sensor and the host
> > have to agree upon (ie. agree upon the output the sensor has to deliver to the
> > host).
> > 
> > By now, only the Marvell PXA27x CPU was used as the host of this sensor, and the
> > PXA expects the inverted Red/Blue order (ie. have BGR format).
> > 
> > Now, for the solution to your problem, we could :
> >  - enhance the mt9m111, and add the V4L2_MBUS_FMT_BGR565_2X8_LE format
> >    This format would have swap_rgb_red_blue = 1
> >  - fix the mt9m111, and add for the V4L2_MBUS_FMT_BGR565_2X8_LE format
> >    swap_rgb_red_blue = 0
> >  - fix the pxa_camera, and convert the RGB format asked for by userland into the
> >  V4L2_MBUS_FMT_BGR565_2X8_LE
> > 
> > What is your opinion here, Guennadi ?
> > 
> > --
> > Robert
> > 
> > PS: As for now, the RGB565 format is transfered as follows from the sensor, for
> > one pixel, over a 8 bit bus (D7-D0):
> > 
> >        D7 D6 D5 D4 D3 D2 D1 D0
> >        =======================
> > Byte1: G4 G3 G2 R7 R6 R5 R4 R3
> > Byte2: B7 B6 B5 B4 B3 G7 G6 G5
> > 
> > This is BGR565, with byte1 and byte2 inverted.
> 
> "inverted" has to be explained here, I think. So, an BGR565 is a 16-bit 
> word like (using your notation)
> 
> High byte                  | Low byte
> bit15                      |                      bit0
>    b7 b6 b5 b4 b3 g7 g6 g5 | g4 g3 g2 r7 r6 r5 r4 r3
> 
> on a LE machine this will be stored in RAM as
> 
> address 0 | address 1
> Low byte  | High byte
> 
> So, if we take a "natural pass-through" packing as
> 
> Byte1 -> address 0
> Byte2 -> address 1
> 
> Then your table above is a V4L2_MBUS_FMT_BGR565_2X8_LE format. Agree? So, 
> that's what you get with "swap_rgb_red_blue = 1." Now, this flag actually 
> swaps the colour components, not the bytes, right? With "swap_rgb_red_blue 
> = 0" you'd get V4L2_MBUS_FMT_BGR565_2X8_BE. So, yes, I agree, that 
> you have to extend the mt9m111 driver to support both these formats by 
> toggling that bit, and yes, you have to replace *RGB* formats with *BGR* 
> analogs in both mt9m111 and pxa drivers, because that's what we actually 
> have, right? And, while at it, we should extend mt9m111 to handle the 
> swap_rgb_red_blue flag to also provide *RGB* formats.

Ok then, I'll create patches for this.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: mt9m111 swap_rgb_red_blue

[Sascha Hauer]

Hi Robert,

On Wed, May 26, 2010 at 10:19:21PM +0200, Robert Jarzmik wrote:
> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > Hi,
> >
> > The mt9m111 soc-camera driver has a swap_rgb_red_blue variable which is
> > hardcoded to 1. This results in, well the name says it, red and blue being
> > swapped in my picture.
> > Is this value needed on some boards or is it just a leftover from
> > development?
> 
> Hi Sascha,
> 
> It's not a development leftover, it's something that the sensor and the host
> have to agree upon (ie. agree upon the output the sensor has to deliver to the
> host).
> 
> By now, only the Marvell PXA27x CPU was used as the host of this sensor, and the
> PXA expects the inverted Red/Blue order (ie. have BGR format).

I have digged around in the Datasheet and if I understand it correctly
the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv
(which should be a pass through) we should be able to support rgb on PXA
aswell. Robert, can you confirm that with the following patch applied
you still get an image but with red/blue swapped?

Sascha



>From c7b7d94eca2ed3c17121c558b4cbd31eaadb9dc0 Mon Sep 17 00:00:00 2001
From: Sascha Hauer <s.hauer@pengutronix.de>
Date: Fri, 28 May 2010 08:23:20 +0200
Subject: [PATCH] pxa_camera: Allow real rgb565 format

Signed-off-by: Sascha Hauer <s.hauer@pengutronix.de>
---
 drivers/media/video/pxa_camera.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/drivers/media/video/pxa_camera.c b/drivers/media/video/pxa_camera.c
index 7fe70e7..f635ad2 100644
--- a/drivers/media/video/pxa_camera.c
+++ b/drivers/media/video/pxa_camera.c
@@ -1129,7 +1129,7 @@ static void pxa_camera_setup_cicr(struct soc_camera_device *icd,
 			CICR1_TBIT | CICR1_COLOR_SP_VAL(1);
 		break;
 	case V4L2_PIX_FMT_RGB565:
-		cicr1 |= CICR1_COLOR_SP_VAL(1) | CICR1_RGB_BPP_VAL(2);
+		cicr1 |= CICR1_COLOR_SP_VAL(2);
 		break;
 	}
 
-- 
1.7.1

-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |

Re: mt9m111 swap_rgb_red_blue

[Robert Jarzmik]

Sascha Hauer <s.hauer@pengutronix.de> writes:

> Hi Robert,
>
> I have digged around in the Datasheet and if I understand it correctly
> the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv
> (which should be a pass through) we should be able to support rgb on PXA
> aswell. Robert, can you confirm that with the following patch applied
> you still get an image but with red/blue swapped?
I can confirm the color swap.
If you want to follow that path, I would suggest instead :
   cicr1 |= CICR1_COLOR_SP_VAL(0);

There is no difference from a processing point of view, it's just that
CICR1_COLOR_SP_VAL(0) is "raw colorspace", with means "pass through", and that
seems to be your goal here.

Note that the patch would have to be completed with the BGR565 into RGB565
conversion, if the sensor was to provide only BGR565. But that could very well
be for another patch.

Cheers.

--
Robert

Re: mt9m111 swap_rgb_red_blue

[Guennadi Liakhovetski]

On Mon, 31 May 2010, Robert Jarzmik wrote:

> Sascha Hauer <s.hauer@pengutronix.de> writes:
> 
> > Hi Robert,
> >
> > I have digged around in the Datasheet and if I understand it correctly
> > the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv
> > (which should be a pass through) we should be able to support rgb on PXA
> > aswell. Robert, can you confirm that with the following patch applied
> > you still get an image but with red/blue swapped?
> I can confirm the color swap.
> If you want to follow that path, I would suggest instead :
>    cicr1 |= CICR1_COLOR_SP_VAL(0);
> 
> There is no difference from a processing point of view, it's just that
> CICR1_COLOR_SP_VAL(0) is "raw colorspace", with means "pass through", and that
> seems to be your goal here.

That would be the default case in that switch, but raw only supports 8, 9, 
or 10 bpp, so, you'd have to use 8bpp but then fake the pixels-per-line 
field. But that would be the cleanest way, yes. Would that work like that?

> Note that the patch would have to be completed with the BGR565 into RGB565
> conversion, if the sensor was to provide only BGR565. But that could very well
> be for another patch.

Thanks
Guennadi
---
Guennadi Liakhovetski, Ph.D.
Freelance Open-Source Software Developer
http://www.open-technology.de/

Re: mt9m111 swap_rgb_red_blue

[Sascha Hauer]

On Mon, May 31, 2010 at 08:46:00AM +0200, Guennadi Liakhovetski wrote:
> On Mon, 31 May 2010, Robert Jarzmik wrote:
> 
> > Sascha Hauer <s.hauer@pengutronix.de> writes:
> > 
> > > Hi Robert,
> > >
> > > I have digged around in the Datasheet and if I understand it correctly
> > > the PXA swaps red/blue in RGB mode. So if we do not use rgb mode but yuv
> > > (which should be a pass through) we should be able to support rgb on PXA
> > > aswell. Robert, can you confirm that with the following patch applied
> > > you still get an image but with red/blue swapped?
> > I can confirm the color swap.
> > If you want to follow that path, I would suggest instead :
> >    cicr1 |= CICR1_COLOR_SP_VAL(0);
> > 
> > There is no difference from a processing point of view, it's just that
> > CICR1_COLOR_SP_VAL(0) is "raw colorspace", with means "pass through", and that
> > seems to be your goal here.
> 
> That would be the default case in that switch, but raw only supports 8, 9, 
> or 10 bpp, so, you'd have to use 8bpp but then fake the pixels-per-line 
> field.

That's why I suggested yuv. I could leave a big comment why this is
done, but I would implement it using raw mode aswell if that's prefered.

> But that would be the cleanest way, yes. Would that work like that?
> 
> > Note that the patch would have to be completed with the BGR565 into RGB565
> > conversion, if the sensor was to provide only BGR565. But that could very well
> > be for another patch.

Will do, I just wanted to see if this works at all.

Sascha


-- 
Pengutronix e.K.                           |                             |
Industrial Linux Solutions                 | http://www.pengutronix.de/  |
Peiner Str. 6-8, 31137 Hildesheim, Germany | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686           | Fax:   +49-5121-206917-5555 |