Skip to contentSkip to conversation

mt9m111 swap_rgb_red_blue

DISCUSSION Sascha Hauer 3 participants 8 messages May 26, 2010 – May 31, 2010 1 message not in the archive Media (V4L/DVB)

lore
Share & tools
lore

Sascha Hauer

author
#0 lore raw

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

Signature · 4 lines
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 |

A message that is not in the archive (sent off-list or not yet received): [email protected]

author
#2 lore raw
Quoted 16 lines · Robert Jarzmik
Quoted 7 lines · 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?

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).

Quoted 8 lines

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 ?

Quoted 19 lines

- 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.

Quoted 8 lines

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

Signature · 4 lines
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 |
#3 lore raw
In reply to a message not in the archive
Quoted 35 lines · Robert Jarzmik
Quoted 7 lines · 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?

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):

Quoted 6 lines
       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.

Quoted 8 lines

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/

author
#4 lore raw
Quoted 61 lines · Guennadi Liakhovetski
Quoted 35 lines · Robert Jarzmik
Quoted 7 lines · 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?

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):

Quoted 6 lines
       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

Quoted 9 lines

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

Signature · 4 lines
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 |
author
#5 lore raw
In reply to a message not in the archive

Hi Robert,

Quoted 10 lines · Robert Jarzmik
Quoted 7 lines · 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?

Quoted 8 lines

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

Quoted 1 line

From c7b7d94eca2ed3c17121c558b4cbd31eaadb9dc0 Mon Sep 17 00:00:00 2001

From: Sascha Hauer <[email protected]>
Date: Fri, 28 May 2010 08:23:20 +0200
Subject: [PATCH] pxa_camera: Allow real rgb565 format

Signed-off-by Sascha Hauer <[email protected]>
---
 drivers/media/video/pxa_camera.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
drivers/media/video/pxa_camera.c +1 -1
index 7fe70e7..f635ad2 100644
@@ -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;
 	}
Signature · 7 lines
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 |
#6 lore raw
Quoted 7 lines · Sascha Hauer

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

#7 lore raw
Quoted 9 lines · Robert Jarzmik
Quoted 7 lines · Sascha Hauer

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?

Quoted 7 lines

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?

Quoted 3 lines

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/

author
#8 lore raw
Quoted 15 lines · Guennadi Liakhovetski
Quoted 13 lines · Robert Jarzmik
Quoted 7 lines · Sascha Hauer

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);
Quoted 7 lines
Quoted 3 lines

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.

Quoted 5 lines

But that would be the cleanest way, yes. Would that work like that?

Quoted 3 lines

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

Signature · 4 lines
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 |