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

drm: rp1: rp1-dpi: Add "rgb_order" property (to match VC4 DPI) #6597

Merged
merged 1 commit into from
Jan 21, 2025

Conversation

njhollinghurst
Copy link
Contributor

As on VC4, the OF property overrides the order implied by media bus format. Only 4 of the 6 possible orders are supported. New add-on hardware designs should not rely on this "legacy" feature.

This is is an attempt to deal with #6505.

Not all combinations of DRM pixel format, bus format and rgb_order have been tested yet.

@njhollinghurst njhollinghurst requested a review from 6by9 January 10, 2025 10:14
@njhollinghurst
Copy link
Contributor Author

I think this is correct, my only reticence (and the reason it's still a draft) is code size.
There might well be a smaller or cleaner way to achieve the permutation of input (shift,mask,scaling).
Alternatively, if we drop GRB, BRG support for RGB565_1X16, RGB666_1X18 there might be a simpler version that permutes output shifts (though BGR666 will still need asymmetrical settings).

@@ -353,6 +354,20 @@ static int rp1dpi_platform_probe(struct platform_device *pdev)
if (ret)
goto done_err;

dpi->rgb_order_override = -1;
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My only comment is that this could potentially be defined as RP1DSPI_ORDER_UNCHANGED, and check for that by name instead of < 0 at line 423.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's copied from vc4_dpi.c (though in that instance the values were from a register field).

BTW I also toyed with encoding the lookup order more directly in the variable (values like 0x020100) to avoid the switch statement.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

vc4_dpi.c isn't always the best example, and there is no requirement to copy how it is implemented in the code, only the DT side.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Added RP1DSI_ORDER_UNCHANGED as an explicit value, and re-worked the lookup to use bit-shifts (which makes both object and source code slightly smaller. That last & 3 is superstitious since the values are internal).

@njhollinghurst njhollinghurst marked this pull request as ready for review January 17, 2025 16:47
@njhollinghurst
Copy link
Contributor Author

njhollinghurst commented Jan 17, 2025

Tested on RGB666_CPADHI (Mode 6) and RGB666 (Mode 5) HATs; kmstest with all supported 4CCs over all values of rgb_order. The colours seemed to be going to the right places as far as I could see.

Also tested RGB565 (Mode 2) and RGB888 (Mode 7) albeit without appropriate hardware, and the output looked plausible.

@othermod
Copy link

Tested on my hardware as well. Verified rgb888 with rgb-order of brg and grb work as expected.

As on VC4, the OF property overrides the order implied by media
bus format. Only 4 of the 6 possible orders are supported. New
add-on hardware designs should not rely on this "legacy" feature.

Signed-off-by: Nick Hollinghurst <nick.hollinghurst@raspberrypi.com>
@njhollinghurst njhollinghurst merged commit ba9883e into raspberrypi:rpi-6.6.y Jan 21, 2025
12 checks passed
@njhollinghurst njhollinghurst deleted the rp1-rgb-order branch January 21, 2025 14:48
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Jan 24, 2025
kernel: configs: add CONFIG_SND_SOC_WM8904=m
See: raspberrypi/linux#6623

kernel: drm: rp1: rp1-dpi: Add rgb_order property (to match VC4 DPI)
See: raspberrypi/linux#6597
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Jan 24, 2025
kernel: configs: add CONFIG_SND_SOC_WM8904=m
See: raspberrypi/linux#6623

kernel: drm: rp1: rp1-dpi: Add rgb_order property (to match VC4 DPI)
See: raspberrypi/linux#6597
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

Successfully merging this pull request may close these issues.

3 participants