Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Open
othermod opened this issue Nov 30, 2024 · 8 comments
Open

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

othermod opened this issue Nov 30, 2024 · 8 comments

Comments

@othermod
Copy link

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

othermod added a commit to othermod/PSPi-Version-6 that referenced this issue Dec 2, 2024
Bookworm requires KMS, and it has its own implementation of DPI.
Doesn't work correctly on Pi5/CM5 yet raspberrypi/linux#6505
@6by9
Copy link
Contributor

6by9 commented Dec 2, 2024

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
Copy link
Author

othermod commented Dec 6, 2024

@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
Copy link
Contributor

6by9 commented Dec 6, 2024

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
Copy link
Author

othermod commented Dec 7, 2024

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
Copy link
Contributor

njhollinghurst commented Dec 9, 2024

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
Copy link
Contributor

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
Copy link
Author

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
Copy link
Contributor

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!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

No branches or pull requests

3 participants