Missing Color Order in KMS DPI Overlay on Compute Module 5 and Pi 5

[othermod]

Describe the bug

The vc4-kms-dpi-generic-overlay.dts overlay is missing some capability that previously existed for changing the color order of DPI output. It gives only bgr888 and rgb888 as options. I developed a product using red instead of green in the middle group, and I'm currently unable to use the KMS DPI overlay.

The previous DPI method gave 4 color orders for RGB
rgb_order:
1: DPI_RGB_ORDER_RGB
2: DPI_RGB_ORDER_BGR
3: DPI_RGB_ORDER_GRB
4: DPI_RGB_ORDER_BRG

This is an identical issue to #6155, but is now needed for the Raspberry Pi 5 and CM5.

The previous fix was effective for the CM4, and I was able to compile an overlay that modifed the color order and enabled i2c, but this does not work on the CM5. Some discussion occurred about doing something similar for the RP1 #6156.

Steps to reproduce the behaviour

Use the KMS DPI overlay on the CM5 with a product (such as my PSPi 6) that makes use of the previously available DPI color orders that use red in the center group instead of green.

Device (s)

Raspberry Pi 5, Other

System

Raspberry Pi OS Bookworm

Logs

No response

Additional context

No response

[6by9]

Yes it would be possible to add something similar for RP1, but it hasn't happened yet.

It's probably a more complex version of set_output_format.
Currently RGB vs BGR handling is done by changing the input format configuration though, so fiddling the shifts there may be the better solution.

[othermod]

@6by9 I've got something functioning. Is this along the lines of what you are thinking?
rpi-6.6.y...othermod:linux:rpi-6.6.y

I'm also curious what you think about removing all the duplicate parameters (bgr666-padhi/rgb666-padhi/bgr888/rgb888) and just using rgb-order to change the order.

[6by9]

No to removing existing parameters. Firstly it breaks anyone already using them, and secondly the use of the media bus formats is the cleaner approach.

As for the implementation, I was envisaging reworking my_formats so that it had the bit positions for the channels rather than the precomputed register values. That then avoids having to pull it apart again if trying to handle alternate orders. Separating bus width and component order would probably also make it easier.

I think you've lost the handling of RGB666 vs BGR666 and RGB888 vs BGR888.

[othermod]

Would it be better if all formats are just explicity stated and don't use rgb_order at all for rp1 then? I don't think rgb_order is widely used. I've got a few thousand boards out there using it, but I'm having to update things anyway for the CM5.

I ask because there's overlap between them and it starts to make the code confusing. Handling a combo of format and rgb_order can make it tough to predict what the actual color order will be. For example, if I set BGR888 and the set the rgb_order to bgr as well, depending on the implementation it could shift things a couple times and give the opposite of what the user expects.

[njhollinghurst]

Tangentially, we shouldn't confuse the input RGB order (which matches the DRM pixel format and can change at run-time) with the output order (which matches the media bus format, or whatever overrides/supplements it). This is somewhat my fault as I used a hack (i^=1) to combine them in the RP1 driver (it was expedient at the time...)

There are some weird corners like an RGB565 framebuffer driving a GRB565 display, but perhaps we don't care about those.

I'm happy for a specified order to completely override the order implied by the media bus format, but we can't break the current use of media bus format (alone) to specify the output order.

[njhollinghurst]

I'm somewhat coming around to @othermod's approach of permuting the input shifts. We just need to take care to avoid double-swaps (or rather, make them consistent with VC4 behaviour) and deal with special cases like the 565 and 666 workarounds. Something might be forthcoming in the new year.

[othermod]

Things can get weird when dealing with 565. Either the middle group will always get 6 bits (which doesn't really help when moving colors since green is what really benefits) or the 6 would get moved to a different group and the pins get adjusted to 655 or 556. Maybe this is a situation that will never come up, so not sure how much effort should go into it. Maybe just do 666 and 888?

People can also still get 565 from the 666 option by just setting the unneeded pins manually in their own overlay. I do that on one of my boards to get 777 on an LCD using the 888 option, and use those 3 pins for other things.

I'm also a bit of a noob at some of this and just consuming info as fast as I can. I probably don't have the full context of why some decisions were made.

[njhollinghurst]

It's about legacy: we want to make Pi 5 match the behaviour of the previous models where practical, at the same time trying to match Linux conventions like media bus formats and expose the useful features of the new hardware.

I imagine (hope) that nobody would ever want to combine a non-standard "rgb_order" with 565...

There's a hardware issue that affects "packed" 555 and 565 (modes 2 and 5). Because of the way a 30-bit bus is converted to a 24-bit bus (by skipping some bits) inside RP1, colour components which cross "byte boundaries" (GPIOs 27:20, 19:12, 11:4) create difficulties. We partly work around it by messing with input formats, masks and scaling. That might explain some of the weird code!

[njhollinghurst]

How badly is this needed, and could the two new orders be restricted to modes 6 (RGB666_CPADHI) and 7 (RGB888) only?

A general implementation is proving unpleasant to implement. It interacts with the 565 and 666 workarounds which re-purpose the "input-scaling" mechanism to achieve certain output patterns, and need to adjust multiple registers depending which channel is where (even the 666 case is not symmetrical). This is nobody's fault but ours, as it's due to a bug in RP1.

[othermod]

@njhollinghurst 565 is not needed at all for me, and I'm not aware of anyone using it in combination with a color order change. 666 and 888 are the only ones needing the change, and I don't see a problem at the moment with using modes 6 and 7. I'll dig deeper to verify and update later.

[njhollinghurst]

Of course we can't/shouldn't stop people using an RGB565 framebuffer with those DPI modes, so the fix still has to affect more than input shifts. But if we can ignore both 565_1X16 (mode 2) and 666_1X18 (mode 5), it might become preferable to change output rather than input parameters (which obsoletes my change above!)

[othermod]

I may be the only person who made use of the color order change in this way. I did it to open up GPIO12 for PWM audio while maintaining green at 8 bits. Very little was gained from this, and its something I wouldn't have done if I knew the RP1 would be a problem.

Please lean in the direction of inconveniencing me rather than others/yourself. As long as I have something I can add to a custom overlay to fix this specific situation (on the few thousand boards I have in the wild), that will eventually make its way to Lakka/Recalbox, I'm good. Not sure whether that helps or not.

[othermod]

Just a quick follow-up.

I began the next steps of making the CM5 work on my board, and started working on PWM audio today (or whatever the RP1 hardware support is called). Did some digging, and learned that this isn't functioning at all. I know it's a bit of a detour in the conversation, but do you know whether there are concrete plans to make PWM audio work on the Pi5/CM5?
https://forums.raspberrypi.com/viewtopic.php?p=2154680#p2154680

I'm asking, because if there aren't plans, then that alone affects the possibility of using the CM5 on my board.

[njhollinghurst]

Should be a separate issue, really. I gather it's on the radar but not a top priority.

[njhollinghurst]

Does #6597 work on your hardware?

[othermod]

Cloning and compiling now. Will report back shortly.

[othermod]

@njhollinghurst
Yes, the color change works on my board.

[njhollinghurst]

Merged to rpi-6.6.y. This is likely to go into the next release, which is likely to be our last one based on 6.6.

[othermod]

@njhollinghurst
Is there a way to $ support the two of you as a thanks for the effort?

[njhollinghurst]

Thank you, but it is not necessary.

[pelwell]

Just tell your friends!