TC358743 produces BGR instead of RGB

[mdevaev]

Describe the bug

I'm trying to use RGB24 format with TC358743 instead of UYVY, and noticed that it produces BGR sequence in the raw data instead of RGB. I used the blue fullscreen image as an HDMI source and made a stream dump that confirms this, getting something like [255, 0, 0] for first 3 bytes.

If you have a top secret datasheet under Toshiba NDA, you can check pages 36/261 and 37/261. This confirms that UYVY is implemented correctly, but RGB has the reverse sequence. Apparently, this bug went unnoticed because everyone was using UYVY. I need RGB to output the image in DRM and encode in M2M at the same time.

Steps to reproduce the behaviour

Use any capture software with TC358743 in RGB24 format and dump the raw image.

Device (s)

Raspberry Pi CM4, Raspberry Pi CM4 Lite

System

• Arch Linux ARM, but it doesn't matter
• version c14213886ae1e31951022b74ce776ce7ee7023ee (clean) (release) (start)
• Linux pikvm 6.6.21-4-rpi #1 SMP Wed Mar 13 20:36:42 UTC 2024 armv7l GNU/Linux
• Apparently, the problem is relevant for any version of kernel.

Logs

No response

Additional context

I found this bug when encoding JPEG in M2M and using libjpeg. It's the same with M2M H.264. Interestingly, because of the endianness, DRM_FORMAT_RGB888 is actually BGR, so the rendering of the TC* chip output in DRM was correct colors, while the encoding was not.

I don't know if it is possible to fix this with little blood without loss of performance, given that it looks like you need to use the reverse order to put bytes in memory after receiving from CSI. This is not a big problem and I can make a userspace workaround for this, but it would probably be better to fix it upstream if the fix is not complex or fatal to performance.

[6by9]

Life with component ordering is always complicated.

tc358743 is an upstream driver with relatively minimal changes. It was written by Cisco for some of their video conferencing kit. IIRC it was against an ST SoC, but it may have been TI.

They've defined that the media bus code is MEDIA_BUS_FMT_RGB888_1X24

Checking the definition for MEDIA_BUS_FMT_RGB888_1X24, it is putting B in the LSBs of the bus. CSI-2 does send the data LSB and blue first, so would make sense as a mapping,

As a cause for further confusion, the CSI-2 spec (I'm looking at v1.01.00 r0.04 2-Apr-2009) does include 12.2 RGB888 Data Reception

The RGB888 data format byte to 32-bit memory word mapping follows the generic CSI-2 rule.

Figure 110 that follows writes B0 into the LSB of a 32bit word.

Section 12.7 YUV422 8-bit Data Reception and figure 115 that follows says that format does NOT follow the generic CSI-2 rule, and whilst U1 is sent first on the wire, it is written into the MSB of a 32bit word.

However the whole of section 12 is marked as informative.

I can't see anything in the Unicam docs that imply it does implement the generic CSI-2 rule. I don't see why it would ever be implemented on a big endian system such as ARM, as it means that the byte order is totally messed if wanting to access individual components as bytes (memory order would be B2 R1 G1 B1 G3 B3 R2 G2!)

Looking at how MEDIA_BUS_FMT_RGB888_1X24 should map to a V4L2_PIX_FMT_xx define, comparing against V4L2_PIX_FMT_SRGGB16 and MEDIA_BUS_FMT_SRGGB16_1X16, I can see an argument that MEDIA_BUS_FMT_RGB888_1X24 should be V4L2_PIX_FMT_BGR24.
TC358743 is the only vaguely supported device on the Pi using the format (we don't support OV5640, ADV7511, ADV7605, or ADV7842), so swapping them would be of limited risk, but needs a chunk more studying. I don't see an equivalent in any other driver which would have given me more confidence in the change.

[6by9]

And it wasn't unnoticed - there are various forum threads where GStreamer's capssetter is being used to swap bgr to rgb when using tc358743.

[mdevaev]

Thanks for researching this. It looks like total hell. I believe CISCO can use byte swapping in its userspace (or a format overriding, whatever), and it looks like this is the path I should follow right now to avoid headache.