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-dsi: Add DRM_FORMAT_ABGR8888 #6314

Merged
merged 3 commits into from
Aug 20, 2024

Conversation

JanKehren
Copy link
Contributor

Add support for DRM_FORMAT_ABGR8888.
This is required to use the driver in Android.

@6by9
Copy link
Contributor

6by9 commented Aug 16, 2024

Thanks for the patch, however this needs some discussion.

Stating that an alpha channel is supported on a primary plane makes little sense as there will be nothing underneath(*), so it seems a slightly daft thing for Android to be requiring.
The RP1 hardware certainly doesn't support any form of alpha, and indeed only has one plane.

There are a couple of formats that are defined as mandatory for DRM, but I can't find the reference for those at present. If Android is requiring this format, it'd be worth getting that list updated upstream.

If we do accept this then you should add DRM_FORMAT_ARGB8888 as well for symmetry.

(*) Patches for setting background colour have been mooted, but never merged

@JanKehren
Copy link
Contributor Author

Thanks for a quick review.

I added DRM_FORMAT_ARGB8888 for the symmetry.

I would also agree that Android behavior makes little sense but the default behavior of Android giving out stuff in HAL_PIXEL_FORMAT_RGBA_8888 which translates in minigbm to DRM_FORMAT_ABGR8888.

@6by9
Copy link
Contributor

6by9 commented Aug 19, 2024

Having worked on Android systems in previous jobs, I have no expectations of it doing anything sensible!

I'm of an opinion to accept this as it solves an issue, even if logically it makes little sense. I'll let others respond though

@njhollinghurst any thoughts?

@njhollinghurst
Copy link
Contributor

It seems reasonable if it solves a compatibility problem. (We have to assume of course that Alpha==0xFF or is safe to ignore.)

@njhollinghurst
Copy link
Contributor

njhollinghurst commented Aug 19, 2024

DPI and VEC as well?

@6by9
Copy link
Contributor

6by9 commented Aug 19, 2024

We have to assume of course that Alpha==0xFF or is safe to ignore.

We only have the one plane on the RP1 display devices, so there can never be a plane underneath.
Memory says that DRM does document that there is assumed to be an opaque black layer at the bottom of the stack should the lowest plane not cover the whole screen.
If the alpha isn't 0xff then we will do the wrong thing (the black should be reflected through), but the hardware can't do that.

VEC as well?

This change could be made for DPI and VEC as well as they'll have the same requirements under Android. I wasn't going to chase for it.

I was looking at other drivers to compare. Things like the tiny DRM drivers largely do not to support ABGR, with arcpgu being the exception. That programs the register with the same value for XRGB8888 and ARGB8888 (https://github.com/raspberrypi/linux/blob/rpi-6.6.y/drivers/gpu/drm/tiny/arcpgu.c#L133-L136). I guess that can be taken as prior art.

@JanKehren
Copy link
Contributor Author

I have added this for DPI and VEC driver.

Android requires this.
As the underlying hardware doesn't support alpha blending,
we ignore the alpha value.

Signed-off-by: Jan Kehren <[email protected]>
Android requires this.
As the underlying hardware doesn't support alpha blending,
we ignore the alpha value.

Signed-off-by: Jan Kehren <[email protected]>
Android requires this.
As the underlying hardware doesn't support alpha blending,
we ignore the alpha value.

Signed-off-by: Jan Kehren <[email protected]>
@njhollinghurst
Copy link
Contributor

Thank you

@njhollinghurst njhollinghurst merged commit 29f7f01 into raspberrypi:rpi-6.6.y Aug 20, 2024
12 checks passed
popcornmix added a commit to raspberrypi/firmware that referenced this pull request Aug 23, 2024
kernel: drm/vc4: Add a delay after disabling hdmi phy output
See: raspberrypi/linux#6309

kernel: Add overlay for a generic I2S clock-master DAC
See: raspberrypi/linux#6311

kernel: ASoC: DACplusADCPro - put ADC control definitions in header file
See: raspberrypi/linux#6313

kernel: drm: rp1: rp1-dsi: Add DRM_FORMAT_ABGR8888
See: raspberrypi/linux#6314

kernel: configs: Add I2C_HID_OF to Pi defconfigs
See: raspberrypi/linux#6316

kernel: ASoC: Add support for the HiFiBerry ADC sound card
See: raspberrypi/linux#6318
popcornmix added a commit to raspberrypi/rpi-firmware that referenced this pull request Aug 23, 2024
kernel: drm/vc4: Add a delay after disabling hdmi phy output
See: raspberrypi/linux#6309

kernel: Add overlay for a generic I2S clock-master DAC
See: raspberrypi/linux#6311

kernel: ASoC: DACplusADCPro - put ADC control definitions in header file
See: raspberrypi/linux#6313

kernel: drm: rp1: rp1-dsi: Add DRM_FORMAT_ABGR8888
See: raspberrypi/linux#6314

kernel: configs: Add I2C_HID_OF to Pi defconfigs
See: raspberrypi/linux#6316

kernel: ASoC: Add support for the HiFiBerry ADC sound card
See: raspberrypi/linux#6318
@JanKehren JanKehren deleted the rpi-6.6.y branch September 2, 2024 07:02
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