Skip to content

Conversation

@ngphibang
Copy link
Contributor

@ngphibang ngphibang commented Oct 17, 2024

There is a wrong assumption about the "byte swap" in Zephyr display that makes RGB565 and BGR565 formats are used interchangeably.

This mistake (perhaps) originally comes frome the display samples that leads to the confusion in display drivers, shields, video sample. This PR fixes it.

  • Fix display sample
  • Fix video sample
  • The NXP eLCDIF driver should not force BGR565 to RGB565.
  • The rkxxx panel overlays should declare RGB565, not BGR565.

@ngphibang
Copy link
Contributor Author

ngphibang commented Oct 17, 2024

@danieldegrasse : The display format is RGB565, not BGR565. I think there are two mistakes that makes it "works" in practice for NXP display, but make it confused for others, and that impacts the video capture where we forced RGB565 to BGR565 for display.

  • The rkxxx panel overlays should declare RGB565, not BGR565.
  • The NXP eLCDIF driver should not force BGR565 to RGB565.

josuah
josuah previously approved these changes Oct 17, 2024
@josuah
Copy link
Contributor

josuah commented Oct 17, 2024

Taking the example of RK055HDMIPI4MA01:
https://www.nxp.com/docs/en/data-sheet/RK055MHD091A0-CTG.pdf
scrot_20241017_153215_1070x310

I did not test on hardware though.

@ngphibang ngphibang changed the title Display: Fix rgb565 / bgr565 interchange issue NXP display: Fix rgb565 / bgr565 interchange issue Oct 17, 2024
Copy link
Contributor

@danieldegrasse danieldegrasse left a comment

Choose a reason for hiding this comment

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

Zephyr's definition of BGR_565 is unfortunately rather odd- BGR_565 corresponds to what most definitions consider RGB565, and RGB_565 corresponds to BGR_565, but byte swapped.

The only reason I can imagine we did this was that most SPI displays that use 16 bit color would use the RGB_565 format, since the byte swap would result on the data on the SPI MOSI line being in the correct order.

This comment by @zejiang0jason does a good job explaining what is going on here: #71406 (comment)

Another way to check this would be to run samples/drivers/display with this PR, you'll see that the box we expect to be gray is instead cycling through a set of incorrect colors.

@ngphibang
Copy link
Contributor Author

@danieldegrasse : I did check with both samples/drivers/display and samples/drivers/video/capture (where we used video_sw_generator to generate RGB565 color bar patttern) and see it works correctly.

@ngphibang
Copy link
Contributor Author

ngphibang commented Oct 18, 2024

I will look through Jason's explaination but honestly, I can't understand where and how Zephyr define a RGB565 to become BGR565. If it's an issue with only NXP's displays, it should not be enforced widely in Zephyr.

@ngphibang
Copy link
Contributor Author

ngphibang commented Oct 18, 2024

Oh, just retested. Indeed, with the display sample, the colors are wrong, from top-left to bottom-left the box colors are : R -> B, G -> R, B -> G and the grey box cycling through a set of incorrect colors as you said, sorry for that (I am not too familiar with the display sample, so I didn't check carefully).

But for the video capture sample, the color bar pattern are all correct how do we explain that ?
/* Black, Blue, Red, Purple, Green, Aqua, Yellow, White */

PXL_20241018_200555478

@ngphibang ngphibang force-pushed the fix_rgb565_bgr565_issue branch from 2ab5de2 to 5affa0b Compare July 22, 2025 11:37
@ngphibang ngphibang dismissed stale reviews from josuah and avolmat-st via 684e39d July 22, 2025 11:42
@ngphibang ngphibang force-pushed the fix_rgb565_bgr565_issue branch from 5affa0b to 684e39d Compare July 22, 2025 11:42
@aescolar aescolar dismissed their stale review July 22, 2025 11:50

Addressed

@ngphibang ngphibang force-pushed the fix_rgb565_bgr565_issue branch from 684e39d to e89e549 Compare July 28, 2025 08:47
Mention that the RGB565 and BGR565 interchange issue has been fixed
in the display sample.

Signed-off-by: Phi Bang Nguyen <[email protected]>
@ngphibang ngphibang force-pushed the fix_rgb565_bgr565_issue branch from e89e549 to 2eb7066 Compare July 28, 2025 09:10
@avolmat-st
Copy link

Would it make sense to add a comment into the display header to describe what is the expected orderring for each format ?
Something similar to what is already available on the video side.

/*
 * 5 red bits [15:11], 6 green bits [10:5], 5 blue bits [4:0].
 * This 16-bit integer is then packed in little endian format over two bytes:
 *
 * @code{.unparsed}
 *   7......0 15.....8
 * | gggBbbbb RrrrrGgg | ...
 * @endcode
 */
#define VIDEO_PIX_FMT_RGB565 VIDEO_FOURCC('R', 'G', 'B', 'P')

With this PR merged, the LTDC is now broken since we merged the PR #92483 in the v4.2.0 release cycle in order to have LTDC behaving properly in v4.2.0. So related swap done in the PR #92483 should now be reverted I believe to have it work again.
Could you also make the revert part of this PR or do you prefer I do it on top of this one ?

@sonarqubecloud
Copy link

@ngphibang
Copy link
Contributor Author

Would it make sense to add a comment into the display header to describe what is the expected orderring for each format ?

Yes, it's a good idea.

Could you also make the revert part of this PR or do you prefer I do it on top of this one ?

It should better be done in the same PR I think. I will do it.

Copy link
Contributor

@danieldegrasse danieldegrasse left a comment

Choose a reason for hiding this comment

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

Let's go ahead and get this moved forwards. @josuah I appreciate you creating #92343- I have an st7796s based display, which appears to need updating. I made the change here: danieldegrasse@0bce970. @ngphibang would you prefer I make a PR with that fix, or to just pull that commit into this PR?

@cfriedt cfriedt merged commit 1edc97c into zephyrproject-rtos:main Jul 28, 2025
26 checks passed
@avolmat-st
Copy link

@ngphibang since this PR got merged yesterday I will prepare and push a PR in order to tackle the two points I raised in my previous comment, that is, adding format description and fixing the LTDC.

@ngphibang
Copy link
Contributor Author

ngphibang commented Jul 29, 2025

@avolmat-st : Sorry for the delay. I am working on it now.
@danieldegrasse : Yes, I will pick your commit. Thanks !

@ngphibang
Copy link
Contributor Author

Here is the PR : #93805

josuah added a commit to josuah/zephyr that referenced this pull request Sep 16, 2025
Propagate zephyrproject-rtos#79996 where RGB_565 and BGR_565 interchange got fixed.
It is not necessary to enable the BGR_565 format for the ST7789V display
as RGB_565, the default, is correct.

Signed-off-by: Josuah Demangeon <[email protected]>
josuah added a commit to josuah/zephyr that referenced this pull request Sep 16, 2025
Propagate zephyrproject-rtos#79996 where RGB_565 and BGR_565 interchange got fixed.
It is not necessary to enable the BGR_565 format for the ST7789V display
as RGB_565, the default, is correct.

Signed-off-by: Josuah Demangeon <[email protected]>
josuah added a commit to josuah/zephyr that referenced this pull request Sep 16, 2025
Propagate zephyrproject-rtos#79996 where RGB_565 and BGR_565 interchange got fixed.
It is not necessary to enable the BGR_565 format for the ST7789V display
as RGB_565, the default, is correct.

Signed-off-by: Josuah Demangeon <[email protected]>
josuah added a commit to josuah/zephyr that referenced this pull request Sep 16, 2025
Propagate zephyrproject-rtos#79996 where RGB_565 and BGR_565 interchange got fixed.
It is not necessary to enable the BGR_565 format for the ST7789V display
as RGB_565, the default, is correct.

Signed-off-by: Josuah Demangeon <[email protected]>
fabiobaltieri pushed a commit that referenced this pull request Sep 18, 2025
Propagate #79996 where RGB_565 and BGR_565 interchange got fixed.
It is not necessary to enable the BGR_565 format for the ST7789V display
as RGB_565, the default, is correct.

Signed-off-by: Josuah Demangeon <[email protected]>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

area: Display area: Samples Samples area: Shields Shields (add-on boards) area: Video Video subsystem platform: NXP Drivers NXP Semiconductors, drivers Release Notes Required Release notes required for this change Release Notes To be mentioned in the release notes

Projects

None yet

Development

Successfully merging this pull request may close these issues.