display: fix rgb565 / bgr565 displays and add documentation #93805
Conversation
zephyrbot
added
platform: STM32
ngphibang
changed the title
zephyrbot
requested review from
cfriedt,
danieldegrasse,
djiatsaf-st,
erwango,
etienne-lms,
FRASTM,
gautierg-st,
GeorgeCGV,
JarmouniA,
jfischer-no,
jhedberg,
kartben,
marwaiehm-st,
mathieuchopstm and
VynDragon
|
Thanks @ngphibang !!! |
include/zephyr/drivers/display.h
Outdated
| * | ||
| * @code{.unparsed} | ||
| * 15.....8 7......0 | ||
| * | RrrrrGgg GggBbbbb | ... |
There was a problem hiding this comment.
Very minor typo: G bit7 should be g (small letter)
ngphibang
force-pushed
the
fix_rgb565_boards
branch
2 times, most recently
from
f8abbe5 to
90bdb0b
Compare
| * @code{.unparsed} | ||
| * 7......0 15.....8 | ||
| * | gggBbbbb RrrrrGgg | ... | ||
| * @endcode |
There was a problem hiding this comment.
I am wondering if description for both Big and Little endian is necessary. Most important here is the order of the component, hence R G B (from MSB to LSB) or B G R (from MSB to LSB). How this is stored into memory on little or big endian is not really display specific This is common way to store 16bit values in big or little endian machines.
There was a problem hiding this comment.
Yes, this is a bit redundant but I see that we (people) still have some confusion when discussing about both RGB565/BGR565 and LE/BE at the same time so it seems help to give the most clarity possible.
If you still see it is not necessary, I will remove it then.
There was a problem hiding this comment.
I removed the description for big-endian.
There was a problem hiding this comment.
Sounds great, mentioning this as an example for little-endian gives a hint about how it is stored in memory as well, so great.
Yes, done. |
Add more detailed documentation for RGB565 and BGR565 formats. Signed-off-by: Phi Bang Nguyen <[email protected]>
This reverts the workaround made in the commit: 772fbfe as the proper fix has now been merged upstream. Signed-off-by: Phi Bang Nguyen <[email protected]>
Zephyr is swapping the definitions of RGB565 and BGR565. Swap the return values provided by this driver for supported pixel formats to account for this. Signed-off-by: Daniel DeGrasse <[email protected]>
ngphibang
force-pushed
the
fix_rgb565_boards
branch
from
90bdb0b to
c80c77e
Compare
|
|
Sounds good to me, I will mark it as approved once tested on my board later today. Thanks @ngphibang. |
| /* | ||
| * Invert the pixel format for 8-bit 8080 Parallel Interface. | ||
| * | ||
| * Zephyr uses big endian byte order when the pixel format has | ||
| * Zephyr uses little endian byte order when the pixel format has | ||
| * multiple bytes. | ||
| * | ||
| * For RGB565, Red is placed in byte 1 and Blue in byte 0. | ||
| * For BGR565, Red is placed in byte 0 and Blue in byte 1. | ||
| * For BGR565, Red is placed in byte 1 and Blue in byte 0. | ||
| * For RGB565, Red is placed in byte 0 and Blue in byte 1. |
There was a problem hiding this comment.
@danieldegrasse It seems confused when we say "Zephyr uses little endian byte order" ? ... I think endianness is determined by the target hardware, and Zephyr follows the HW architecture's endianess when it is compiled for that HW, no ? And for LE
- RGB565:
* @code{.unparsed}
* 7......0 15.....8
* | gggBbbbb RrrrrGgg | ...
* @endcode
- BGR565:
* @code{.unparsed}
* 7......0 15.....8
* | gggRrrrr BbbbbGgg | ...
* @endcode
So, the old comment is correct, no need to change, I think ?
* For RGB565, Red is placed in byte 1 and Blue in byte 0.
* For BGR565, Red is placed in byte 0 and Blue in byte 1.
| if (((bool)(config->madctl & ST7796S_MADCTL_BGR)) != | ||
| config->rgb_is_inverted) { | ||
| return PIXEL_FORMAT_BGR_565; | ||
| } else { | ||
| return PIXEL_FORMAT_RGB_565; | ||
| } else { | ||
| return PIXEL_FORMAT_BGR_565; | ||
| } |
There was a problem hiding this comment.
Maybe we don't need ST7796S_MADCTL_BGR and rgb_is_inverted anymore ?
There was a problem hiding this comment.
I wasn't sure about that- I unfortunately don't have a board capable of 8 bit or 16 bit MIPI DBI (which is what this was needed for). @mmahadevan108 or @zejiang0jason, could you test this change and see if we need rgb_is_inverted?
After #79996 merged, this PR continues to