soc_camera: endianness between camera and its host

Modern camera chips provide ways to invert their data output, as well in colors swap as in byte order. To be more precise, the one I know (mt9m111) enables :

- swapping Red and Blue in RGB formats

- swapping first and second byte in RGB formats

- swapping Cb and Cr in YUV formats

- swapping chrominance and luminance in YUV formats

This swap is necessary to adapt the camera chip output to the capture interface. For example, pxa_camera on pxa27x CPUs expects RGB and YUV formats in a very specific order. This order is not the default one on mt9m111 chip, so the mt9m111 driver has to have a way to know how to order its output.

The question I have is where to put such information, so that board specific code (arch/arm/mach-pxa/xxx.c) can setup this for a dedicated camera chip ?

One easy way would be to put it in soc_camera_link, but is it the right place ?

Quoted 2 lines · Robert Jarzmik

Modern camera chips provide ways to invert their data output, as well in colors swap as in byte order. To be more precise, the one I know (mt9m111) enables :

To me these look like just different pixel formats:

Quoted 1 line

- swapping first and second byte in RGB formats

hm, no idea about this one

So, I don't see at first any relation to host's endianness. You just define respective formats in cameras array of struct soc_camera_data_format.

Isn't using the existing pixel format negotiation procedure eough?

Quoted 6 lines · Guennadi Liakhovetski

Quoted 2 lines · Robert Jarzmik

Modern camera chips provide ways to invert their data output, as well in colors swap as in byte order. To be more precise, the one I know (mt9m111) enables :

To me these look like just different pixel formats:

Ah, they look like, but there aren't.

Let me explain the subtle part of it by an example on mio a701 phone :
- mt9m111 is connected to a pxa272 cpu through an 8bit bus
- when I select RGB565 as a pixel format, the pxa cpu expects the bits in a very precise order :
- have a look at PXA Developper Manual, chapter 27.4.5.2.1, table 27-10. The order the bytes are comming on the bus is important, because of the "interpretation" the PXA does, to reorder and store them in memory. => the chip must send the bytes in that order
- if you pay attention closely, you'll notice the pxa doesn't expect RGB but inverted BGR.
- have a look at Micron MT9M111 specification, table 5, page 14. You'll see what they consider as RGB565.

Sorry for a late reply. Looking at your mt9m111 patch, I realised we didn't finish this discussion:-)

Ok, I looked at them, I really did:-) Still, doesn't the following describe the situation:

mt9m111 supported formats:
rgb 565 (as specified in mt9m111 manual)
rgb 565 swapped (ditto swapped to match pxa270)
...

pxa270 supports formats
rgb 565 swapped (pxa manual doesn't call it swapped)
...

So, when a user enumerates supported formats, we should report rgb 565 swapped, but not report rgb 565. If you connect a mt9m111 to another host, maybe the non-swapped rgb 565 will be supported. So, mt9m111 should report both. The problem currently is, soc-camera doesn't ask the host controller whether it supports a specific pixel format. It only has a chance to fail an attempted VIDIOC_S_FMT, which is a bit too late. So, would adding pixel format negotiation with the camera host driver sufficiently fix the problem for you? One of us could try to cook a patch then.

Selon Guennadi Liakhovetski <[email protected]>:

Quoted 8 lines

So, when a user enumerates supported formats, we should report rgb 565 swapped, but not report rgb 565. If you connect a mt9m111 to another host, maybe the non-swapped rgb 565 will be supported. So, mt9m111 should report both. The problem currently is, soc-camera doesn't ask the host controller whether it supports a specific pixel format. It only has a chance to fail an attempted VIDIOC_S_FMT, which is a bit too late. So, would adding pixel format negotiation with the camera host driver sufficiently fix the problem for you? One of us could try to cook a patch then.

Yes, pixel format negotiation is the key, that's the clean solution. It will have impacts on existing camera drivers, like mt9m001, ..., and camera hosts, but you must already be aware of it and ready to pay the price :)

Now, let's talk schedule. Until the end of the week, I'll be a bit busy. If I don't see a patch you submitted by then, I'll cook one up. I only need to know at which point you wish the format negociation should be performed, and on which ground.

[RFC]
Would that be something like :
- all begins which the binding of both a camera driver and host driver
- soc_camera asks host controller which format it provides
- soc_camera asks camera driver which format it supports
- soc_camera make a table of possible pixel formats (which would be the common
subset of host and camera pixel formats)
- soc_camera uses that table for format enumeration
- soc_camera uses that table for preliminary check on VIDIOC_S_FMT

Hm, I would just do this during format-enumeration... I'll try to sketch something maybe today.

Quoted 1 line

[RFC]

I think, we might manage to get it a bit simpler.

As for your mt9m111 patch - unfortunately, during my first quick review I missed a few more minor formatting issues, so, because it is kinda my fault, my plan is to apply your patch as it is, and then I can just post for your ack a clean-up patch. And then, as we get format negotiation in place, you can extend it with further supported formats. What do you think?

Selon Guennadi Liakhovetski <[email protected]>:

Quoted 6 lines

As for your mt9m111 patch - unfortunately, during my first quick review I missed a few more minor formatting issues, so, because it is kinda my fault, my plan is to apply your patch as it is, and then I can just post for your ack a clean-up patch. And then, as we get format negotiation in place, you can extend it with further supported formats. What do you think?

Very good idea. Let's do it that way, it's perfect for me.

Fix a typo in Kconfig, simplify error checking, further minor cleanup.

Signed-off-by Guennadi Liakhovetski <[email protected]>

drivers/media/video/mt9m111.c +1 -1

@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret) {
 		mt9m111_reset(icd);

Quoted 3 lines · Guennadi Liakhovetski

Fix a typo in Kconfig, simplify error checking, further minor cleanup.

Signed-off-by Guennadi Liakhovetski <[email protected]>

Tested-by Robert Jarzmik <[email protected]>

But ...

Quoted 11 lines

drivers/media/video/mt9m111.c +1 -3

@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret) {
 		mt9m111_reset(icd);
-	if (ret >= 0)
 		mt9m111_set_context(icd, mt9m111->context);
-	if (ret >= 0)
 		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);

drivers/media/video/mt9m111.c static int mt9m111_init(struct soc_camera_device *icd) · line 786

Quoted 6 lines

drivers/media/video/mt9m111.c +2 -2

-	if (ret < 0)
+	} else
 		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
-	return ret ? -EIO : 0;
+	return ret;
 }

You changed the fault path here : you don't check if every call succeeds. I don't think it's very important though ... I certainly don't care that much.

Quoted 11 lines

Quoted 11 lines

drivers/media/video/mt9m111.c +1 -3

@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret) {
 		mt9m111_reset(icd);
-	if (ret >= 0)
 		mt9m111_set_context(icd, mt9m111->context);
-	if (ret >= 0)
 		mt9m111_set_autoexposure(icd, mt9m111->autoexposure);

Quoted 8 lines

drivers/media/video/mt9m111.c static int mt9m111_init(struct soc_camera_device *icd) · line 786

Quoted 6 lines

drivers/media/video/mt9m111.c +2 -2

-	if (ret < 0)
+	} else
 		dev_err(&icd->dev, "mt9m111 init failed: %d\n", ret);
-	return ret ? -EIO : 0;
+	return ret;
 }

You changed the fault path here : you don't check if every call succeeds. I don't think it's very important though ... I certainly don't care that much.

Sorry, don't understand, you don't set "ret" in the above calls, so, I don't think I changed anything.

The correct behaviour would be something like :

drivers/media/video/mt9m111.c +2 -2

@@ -778,15 +776,13 @@ static int mt9m111_init(struct soc_camera_device *icd)
 
 	mt9m111->context = HIGHPOWER;
 	ret = mt9m111_enable(icd);
-	if (ret >= 0)
+	if (!ret)
 		ret = mt9m111_reset(icd);
-	if (ret >= 0)
+	if (!ret)

And in mt9m111_resume()? Do you want to check return codes of each function there too?