Hi all,
A long outstanding issue with the DRM subsystem has been the lack of support for low-color displays, as used typically on older desktop systems, and on small embedded displays.
This patch series adds support for color-indexed frame buffer formats with 2, 4, and 16 colors. It has been tested on ARAnyM using a work-in-progress Atari DRM driver supporting 2, 4, 16, and 256 colors.
Overview: - Patch 1 optimizes the C8 SMPTE color LUT, for reuse with formats providing less colors, - Patch 2 introduces the new C[124] formats, - Patch 3 adds SMPTE pattern support for the C4 format.
Note that so far there is no specific pattern support for the C1 and C2 formats.
Please refer to [1] for more information.
Thanks for your comments!
[1] "[PATCH v2 00/10] drm: Add support for low-color frame buffer formats" https://lore.kernel.org/r/cover.1646683502.git.geert@linux-m68k.org
Geert Uytterhoeven (3): util: Optimize C8 SMPTE color LUT drm_fourcc: Add DRM_FORMAT_C[124] util: Add SMPTE pattern support for C4 format
include/drm/drm_fourcc.h | 5 ++- tests/modetest/buffers.c | 15 +++++++ tests/modetest/modetest.c | 13 ++++-- tests/util/format.c | 3 ++ tests/util/pattern.c | 93 +++++++++++++++++++++++++++++---------- tests/util/pattern.h | 2 +- 6 files changed, 101 insertions(+), 30 deletions(-)
The color LUT for the SMPTE pattern in indexed mode contains 22 entries, although only 13 are non-unique.
Reduce the size of the color LUT by dropping duplicate entries, so it can be reused for formats supporting e.g. 16 colors. Rename util_smpte_c8_gamma() to util_smpte_index_gamma() accordingly.
Signed-off-by: Geert Uytterhoeven geert@linux-m68k.org --- tests/modetest/modetest.c | 2 +- tests/util/pattern.c | 51 +++++++++++++++++++++------------------ tests/util/pattern.h | 2 +- 3 files changed, 29 insertions(+), 26 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index 5fd22f79d71332cc..e369044fb7a484c1 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -1124,7 +1124,7 @@ static void set_gamma(struct device *dev, unsigned crtc_id, unsigned fourcc)
if (fourcc == DRM_FORMAT_C8) { /* TODO: Add C8 support for more patterns */ - util_smpte_c8_gamma(256, gamma_lut); + util_smpte_index_gamma(256, gamma_lut); drmModeCreatePropertyBlob(dev->fd, gamma_lut, sizeof(gamma_lut), &blob_id); } else { for (i = 0; i < 256; i++) { diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 158c0b160a2ee870..953bf95492ee150c 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -605,6 +605,9 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, } }
+static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; +static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 }; + static void fill_smpte_c8(void *mem, unsigned int width, unsigned int height, unsigned int stride) { @@ -619,28 +622,28 @@ static void fill_smpte_c8(void *mem, unsigned int width, unsigned int height,
for (; y < height * 7 / 9; ++y) { for (x = 0; x < width; ++x) - ((uint8_t *)mem)[x] = 7 + (x * 7 / width); + ((uint8_t *)mem)[x] = smpte_middle[x * 7 / width]; mem += stride; }
for (; y < height; ++y) { for (x = 0; x < width * 5 / 7; ++x) ((uint8_t *)mem)[x] = - 14 + (x * 4 / (width * 5 / 7)); + smpte_bottom[x * 4 / (width * 5 / 7)]; for (; x < width * 6 / 7; ++x) ((uint8_t *)mem)[x] = - 14 + ((x - width * 5 / 7) * 3 - / (width / 7) + 4); + smpte_bottom[(x - width * 5 / 7) * 3 + / (width / 7) + 4]; for (; x < width; ++x) - ((uint8_t *)mem)[x] = 14 + 7; + ((uint8_t *)mem)[x] = smpte_bottom[7]; mem += stride; } }
-void util_smpte_c8_gamma(unsigned size, struct drm_color_lut *lut) +void util_smpte_index_gamma(unsigned size, struct drm_color_lut *lut) { - if (size < 7 + 7 + 8) { - printf("Error: gamma too small: %d < %d\n", size, 7 + 7 + 8); + if (size < 13) { + printf("Error: gamma too small: %d < %d\n", size, 13); return; } memset(lut, 0, size * sizeof(struct drm_color_lut)); @@ -658,22 +661,22 @@ void util_smpte_c8_gamma(unsigned size, struct drm_color_lut *lut) FILL_COLOR( 5, 192, 0, 0 ); /* red */ FILL_COLOR( 6, 0, 0, 192); /* blue */
- FILL_COLOR( 7, 0, 0, 192); /* blue */ - FILL_COLOR( 8, 19, 19, 19 ); /* black */ - FILL_COLOR( 9, 192, 0, 192); /* magenta */ - FILL_COLOR(10, 19, 19, 19 ); /* black */ - FILL_COLOR(11, 0, 192, 192); /* cyan */ - FILL_COLOR(12, 19, 19, 19 ); /* black */ - FILL_COLOR(13, 192, 192, 192); /* grey */ - - FILL_COLOR(14, 0, 33, 76); /* in-phase */ - FILL_COLOR(15, 255, 255, 255); /* super white */ - FILL_COLOR(16, 50, 0, 106); /* quadrature */ - FILL_COLOR(17, 19, 19, 19); /* black */ - FILL_COLOR(18, 9, 9, 9); /* 3.5% */ - FILL_COLOR(19, 19, 19, 19); /* 7.5% */ - FILL_COLOR(20, 29, 29, 29); /* 11.5% */ - FILL_COLOR(21, 19, 19, 19); /* black */ + /* duplicate of 6 */ /* blue */ + FILL_COLOR( 7, 19, 19, 19 ); /* black */ + /* duplicate of 4 */ /* magenta */ + /* duplicate of 7 */ /* black */ + /* duplicate of 2 */ /* cyan */ + /* duplicate of 7 */ /* black */ + /* duplicate of 0 */ /* grey */ + + FILL_COLOR( 8, 0, 33, 76); /* in-phase */ + FILL_COLOR( 9, 255, 255, 255); /* super white */ + FILL_COLOR(10, 50, 0, 106); /* quadrature */ + /* duplicate of 7 */ /* black */ + FILL_COLOR(11, 9, 9, 9); /* 3.5% */ + /* duplicate of 7 */ /* 7.5% */ + FILL_COLOR(12, 29, 29, 29); /* 11.5% */ + /* duplicate of 7 */ /* black */
#undef FILL_COLOR } diff --git a/tests/util/pattern.h b/tests/util/pattern.h index ea38cafdcf27d811..d178bca69b227751 100644 --- a/tests/util/pattern.h +++ b/tests/util/pattern.h @@ -39,7 +39,7 @@ void util_fill_pattern(uint32_t format, enum util_fill_pattern pattern, void *planes[3], unsigned int width, unsigned int height, unsigned int stride);
-void util_smpte_c8_gamma(unsigned size, struct drm_color_lut *lut); +void util_smpte_index_gamma(unsigned size, struct drm_color_lut *lut);
enum util_fill_pattern util_pattern_enum(const char *name);
Add fourcc codes for color-indexed frame buffer formats with two, four, and sixteen colors. Add support for creating buffers using these formats.
Signed-off-by: Geert Uytterhoeven geert@linux-m68k.org --- include/drm/drm_fourcc.h | 5 ++++- tests/modetest/buffers.c | 15 +++++++++++++++ tests/util/format.c | 3 +++ 3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 957c7be29239c0a1..f8b18d28a71dabff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -99,7 +99,10 @@ extern "C" { #define DRM_FORMAT_INVALID 0
/* color index */ -#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */ +#define DRM_FORMAT_C1 fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */ +#define DRM_FORMAT_C2 fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */ +#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */ +#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
/* 8 bpp Red */ #define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ diff --git a/tests/modetest/buffers.c b/tests/modetest/buffers.c index 8a8d9e0143474378..af7f60b4fb4d09ad 100644 --- a/tests/modetest/buffers.c +++ b/tests/modetest/buffers.c @@ -135,6 +135,18 @@ bo_create(int fd, unsigned int format, int ret;
switch (format) { + case DRM_FORMAT_C1: + bpp = 1; + break; + + case DRM_FORMAT_C2: + bpp = 2; + break; + + case DRM_FORMAT_C4: + bpp = 4; + break; + case DRM_FORMAT_C8: case DRM_FORMAT_NV12: case DRM_FORMAT_NV21: @@ -283,6 +295,9 @@ bo_create(int fd, unsigned int format, planes[2] = virtual + offsets[2]; break;
+ case DRM_FORMAT_C1: + case DRM_FORMAT_C2: + case DRM_FORMAT_C4: case DRM_FORMAT_C8: case DRM_FORMAT_ARGB4444: case DRM_FORMAT_XRGB4444: diff --git a/tests/util/format.c b/tests/util/format.c index 1ca1b82ce947b2f4..4b984af9bce8ac6f 100644 --- a/tests/util/format.c +++ b/tests/util/format.c @@ -40,6 +40,9 @@
static const struct util_format_info format_info[] = { /* Indexed */ + { DRM_FORMAT_C1, "C1" }, + { DRM_FORMAT_C2, "C2" }, + { DRM_FORMAT_C4, "C4" }, { DRM_FORMAT_C8, "C8" }, /* YUV packed */ { DRM_FORMAT_UYVY, "UYVY", MAKE_YUV_INFO(YUV_YCbCr | YUV_CY, 2, 2, 2) },
On Mon, 7 Mar 2022 21:53:17 +0100 Geert Uytterhoeven geert@linux-m68k.org wrote:
Add fourcc codes for color-indexed frame buffer formats with two, four, and sixteen colors. Add support for creating buffers using these formats.
Signed-off-by: Geert Uytterhoeven geert@linux-m68k.org
include/drm/drm_fourcc.h | 5 ++++- tests/modetest/buffers.c | 15 +++++++++++++++ tests/util/format.c | 3 +++ 3 files changed, 22 insertions(+), 1 deletion(-)
diff --git a/include/drm/drm_fourcc.h b/include/drm/drm_fourcc.h index 957c7be29239c0a1..f8b18d28a71dabff 100644 --- a/include/drm/drm_fourcc.h +++ b/include/drm/drm_fourcc.h @@ -99,7 +99,10 @@ extern "C" { #define DRM_FORMAT_INVALID 0
/* color index */ -#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C */ +#define DRM_FORMAT_C1 fourcc_code('C', '1', ' ', ' ') /* [7:0] C0:C1:C2:C3:C4:C5:C6:C7 1:1:1:1:1:1:1:1 eight pixels/byte */ +#define DRM_FORMAT_C2 fourcc_code('C', '2', ' ', ' ') /* [7:0] C0:C1:C2:C3 2:2:2:2 four pixels/byte */ +#define DRM_FORMAT_C4 fourcc_code('C', '4', ' ', ' ') /* [7:0] C0:C1 4:4 two pixels/byte */ +#define DRM_FORMAT_C8 fourcc_code('C', '8', ' ', ' ') /* [7:0] C 8 one pixel/byte */
Hi Geert,
I believe updates to drm_fourcc.h in libdrm must be done with the specific process, please see
https://gitlab.freedesktop.org/mesa/drm/-/blob/main/include/drm/README
section "When and how to update these files".
Thanks, pq
/* 8 bpp Red */ #define DRM_FORMAT_R8 fourcc_code('R', '8', ' ', ' ') /* [7:0] R */ diff --git a/tests/modetest/buffers.c b/tests/modetest/buffers.c index 8a8d9e0143474378..af7f60b4fb4d09ad 100644 --- a/tests/modetest/buffers.c +++ b/tests/modetest/buffers.c @@ -135,6 +135,18 @@ bo_create(int fd, unsigned int format, int ret;
switch (format) {
- case DRM_FORMAT_C1:
bpp = 1;break;- case DRM_FORMAT_C2:
bpp = 2;break;- case DRM_FORMAT_C4:
bpp = 4;break;- case DRM_FORMAT_C8: case DRM_FORMAT_NV12: case DRM_FORMAT_NV21:
@@ -283,6 +295,9 @@ bo_create(int fd, unsigned int format, planes[2] = virtual + offsets[2]; break;
- case DRM_FORMAT_C1:
- case DRM_FORMAT_C2:
- case DRM_FORMAT_C4: case DRM_FORMAT_C8: case DRM_FORMAT_ARGB4444: case DRM_FORMAT_XRGB4444:
diff --git a/tests/util/format.c b/tests/util/format.c index 1ca1b82ce947b2f4..4b984af9bce8ac6f 100644 --- a/tests/util/format.c +++ b/tests/util/format.c @@ -40,6 +40,9 @@
static const struct util_format_info format_info[] = { /* Indexed */
- { DRM_FORMAT_C1, "C1" },
- { DRM_FORMAT_C2, "C2" },
- { DRM_FORMAT_C4, "C4" }, { DRM_FORMAT_C8, "C8" }, /* YUV packed */ { DRM_FORMAT_UYVY, "UYVY", MAKE_YUV_INFO(YUV_YCbCr | YUV_CY, 2, 2, 2) },
Add support for drawing the SMPTE pattern in a buffer using the C4 indexed format.
Note that this still uses 256 instead of 16 as the CLUT size, as DRM_IOCTL_MODE_SETGAMMA enforces that the size matches against the (fixed) gamma size, while the CLUT size depends on the format.
Signed-off-by: Geert Uytterhoeven geert@linux-m68k.org --- The linuxdoc comments say userspace can query the gamma size:
* drm_mode_gamma_set_ioctl - set the gamma table * * Set the gamma table of a CRTC to the one passed in by the user. Userspace can * inquire the required gamma table size through drm_mode_gamma_get_ioctl.
* drm_mode_gamma_get_ioctl - get the gamma table * * Copy the current gamma table into the storage provided. This also provides * the gamma table size the driver expects, which can be used to size the * allocated storage.
but the code doesn't seem to support that in an easy way (like setting red/green/blue to NULL on input, retrieving gamma_size on output), only by providing big enough buffers for red/green/blue, and looping over gamma_size until -EINVAL is no longer returned. --- tests/modetest/modetest.c | 11 +++++++--- tests/util/pattern.c | 42 +++++++++++++++++++++++++++++++++++++++ 2 files changed, 50 insertions(+), 3 deletions(-)
diff --git a/tests/modetest/modetest.c b/tests/modetest/modetest.c index e369044fb7a484c1..205b7093b0045e01 100644 --- a/tests/modetest/modetest.c +++ b/tests/modetest/modetest.c @@ -1122,16 +1122,21 @@ static void set_gamma(struct device *dev, unsigned crtc_id, unsigned fourcc) struct drm_color_lut gamma_lut[256]; int i, ret;
- if (fourcc == DRM_FORMAT_C8) { - /* TODO: Add C8 support for more patterns */ + switch (fourcc) { + case DRM_FORMAT_C4: + case DRM_FORMAT_C8: + /* TODO: Add index support for more patterns */ util_smpte_index_gamma(256, gamma_lut); drmModeCreatePropertyBlob(dev->fd, gamma_lut, sizeof(gamma_lut), &blob_id); - } else { + break; + + default: for (i = 0; i < 256; i++) { gamma_lut[i].red = gamma_lut[i].green = gamma_lut[i].blue = i << 8; } + break; }
add_property_optional(dev, crtc_id, "DEGAMMA_LUT", 0); diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{ + if (x & 1) + mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f); + else + mem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4); +} + +static void fill_smpte_c4(void *mem, unsigned int width, unsigned int height, + unsigned int stride) +{ + unsigned int x; + unsigned int y; + + for (y = 0; y < height * 6 / 9; ++y) { + for (x = 0; x < width; ++x) + write_pixel_4(mem, x, x * 7 / width); + mem += stride; + } + + for (; y < height * 7 / 9; ++y) { + for (x = 0; x < width; ++x) + write_pixel_4(mem, x, smpte_middle[x * 7 / width]); + mem += stride; + } + + for (; y < height; ++y) { + for (x = 0; x < width * 5 / 7; ++x) + write_pixel_4(mem, x, + smpte_bottom[x * 4 / (width * 5 / 7)]); + for (; x < width * 6 / 7; ++x) + write_pixel_4(mem, x, + smpte_bottom[(x - width * 5 / 7) * 3 / + (width / 7) + 4]); + for (; x < width; ++x) + write_pixel_4(mem, x, smpte_bottom[7]); + mem += stride; + } +} + static void fill_smpte_c8(void *mem, unsigned int width, unsigned int height, unsigned int stride) { @@ -688,6 +728,8 @@ static void fill_smpte(const struct util_format_info *info, void *planes[3], unsigned char *u, *v;
switch (info->format) { + case DRM_FORMAT_C4: + return fill_smpte_c4(planes[0], width, height, stride); case DRM_FORMAT_C8: return fill_smpte_c8(planes[0], width, height, stride); case DRM_FORMAT_UYVY:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Hi Ilia,
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Exactly. All register documentation I've ever seen shows the MSB on the left, i.e. for bytes:
MSB LSB +---+---+---+---+---+---+---+---+ | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +---+---+---+---+---+---+---+---+
IBM used to count bits in the reverse order, but still had MSB left:
MSB LSB +---+---+---+---+---+---+---+---+ | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | +---+---+---+---+---+---+---+---+
If the reverse ordering of pixels is ever needed, a new fourcc code can be introduced. Note that the fbdev API has support for both orderings (see fb_bitfield.msb_right), but no driver ever sets msb_right = 1, hence the fbdev core doesn't support it yet.
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
Hi Ilia,
On Tue, Mar 8, 2022 at 8:57 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Exactly. All register documentation I've ever seen shows the MSB on the left, i.e. for bytes:
MSB LSB +---+---+---+---+---+---+---+---+ | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +---+---+---+---+---+---+---+---+IBM used to count bits in the reverse order, but still had MSB left:
MSB LSB +---+---+---+---+---+---+---+---+ | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | +---+---+---+---+---+---+---+---+If the reverse ordering of pixels is ever needed, a new fourcc code can be introduced. Note that the fbdev API has support for both orderings (see fb_bitfield.msb_right), but no driver ever sets msb_right = 1, hence the fbdev core doesn't support it yet.
Turns out I was wrong: fbdev ordering follows native ordering, and there's also FBINFO_FOREIGN_ENDIAN :-(
I'll reply to the thread that introduced the format. https://lore.kernel.org/all/8d3c0cc370b0214244b01a64c588e5e506531716.1646683...
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Mar 14, 2022 at 9:07 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Ilia,
On Tue, Mar 8, 2022 at 8:57 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Exactly. All register documentation I've ever seen shows the MSB on the left, i.e. for bytes:
MSB LSB +---+---+---+---+---+---+---+---+ | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +---+---+---+---+---+---+---+---+IBM used to count bits in the reverse order, but still had MSB left:
MSB LSB +---+---+---+---+---+---+---+---+ | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | +---+---+---+---+---+---+---+---+If the reverse ordering of pixels is ever needed, a new fourcc code can be introduced. Note that the fbdev API has support for both orderings (see fb_bitfield.msb_right), but no driver ever sets msb_right = 1, hence the fbdev core doesn't support it yet.
Turns out I was wrong: fbdev ordering follows native ordering, and there's also FBINFO_FOREIGN_ENDIAN :-(
I haven't double-checked the meaning in fbdev, but ENDIAN-ness generally refers to the layout of *bytes*, not *bits*. Although one could also argue that it's the layout of "elements", and so in that way, upper/lower values could be considered flipped. I've never gone that far though.
Cheers,
-ilia
Hi Ilia,
On Mon, Mar 14, 2022 at 2:44 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 14, 2022 at 9:07 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Tue, Mar 8, 2022 at 8:57 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Exactly. All register documentation I've ever seen shows the MSB on the left, i.e. for bytes:
MSB LSB +---+---+---+---+---+---+---+---+ | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +---+---+---+---+---+---+---+---+IBM used to count bits in the reverse order, but still had MSB left:
MSB LSB +---+---+---+---+---+---+---+---+ | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | +---+---+---+---+---+---+---+---+If the reverse ordering of pixels is ever needed, a new fourcc code can be introduced. Note that the fbdev API has support for both orderings (see fb_bitfield.msb_right), but no driver ever sets msb_right = 1, hence the fbdev core doesn't support it yet.
Turns out I was wrong: fbdev ordering follows native ordering, and there's also FBINFO_FOREIGN_ENDIAN :-(
I haven't double-checked the meaning in fbdev, but ENDIAN-ness generally refers to the layout of *bytes*, not *bits*. Although one could also argue that it's the layout of "elements", and so in that way, upper/lower values could be considered flipped. I've never gone that far though.
Yes, usually it refers to the ordering of bytes in a word. Here, it's about the ordering of sub-byte pixels in a byte. Note that with C2 and C4, there's a third ordering that comes into play. So we have: 1. Ordering of bytes in a word, for bpp > 8, 2. Ordering of pixels in a byte, for bpp < 8, 3. Ordering of bits in a pixel, for bpp > 1.
1. Is handled by DRM_FORMAT_BIG_ENDIAN. 2. Is what we need to handle here. As bpp cannot be both < 8 and > 8, I think reusing DRM_FORMAT_BIG_ENDIAN is OK. 3. Is handled by fb_bitfield.msb_right, and always false (i.e. no driver ever set it).
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
On Mon, Mar 14, 2022 at 10:06 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
Hi Ilia,
On Mon, Mar 14, 2022 at 2:44 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 14, 2022 at 9:07 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Tue, Mar 8, 2022 at 8:57 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote:
diff --git a/tests/util/pattern.c b/tests/util/pattern.c index 953bf95492ee150c..42d75d700700dc3d 100644 --- a/tests/util/pattern.c +++ b/tests/util/pattern.c @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 };
+static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) +{
if (x & 1)mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f);elsemem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4);+}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Exactly. All register documentation I've ever seen shows the MSB on the left, i.e. for bytes:
MSB LSB +---+---+---+---+---+---+---+---+ | 7 | 6 | 5 | 4 | 3 | 2 | 1 | 0 | +---+---+---+---+---+---+---+---+IBM used to count bits in the reverse order, but still had MSB left:
MSB LSB +---+---+---+---+---+---+---+---+ | 0 | 1 | 2 | 3 | 4 | 5 | 6 | 7 | +---+---+---+---+---+---+---+---+If the reverse ordering of pixels is ever needed, a new fourcc code can be introduced. Note that the fbdev API has support for both orderings (see fb_bitfield.msb_right), but no driver ever sets msb_right = 1, hence the fbdev core doesn't support it yet.
Turns out I was wrong: fbdev ordering follows native ordering, and there's also FBINFO_FOREIGN_ENDIAN :-(
I haven't double-checked the meaning in fbdev, but ENDIAN-ness generally refers to the layout of *bytes*, not *bits*. Although one could also argue that it's the layout of "elements", and so in that way, upper/lower values could be considered flipped. I've never gone that far though.
Yes, usually it refers to the ordering of bytes in a word. Here, it's about the ordering of sub-byte pixels in a byte. Note that with C2 and C4, there's a third ordering that comes into play. So we have:
Ordering of bytes in a word, for bpp > 8,
Ordering of pixels in a byte, for bpp < 8,
Ordering of bits in a pixel, for bpp > 1.
Is handled by DRM_FORMAT_BIG_ENDIAN.
OK. Note that DRM_FORMAT_BIG_ENDIAN flag for formats other than RGBX8888 and very similar formats is basically broken in drm. So ... watch out. There are two setups supported for big-endian currently:
1. Legacy: radeon/nouveau, ignore the "little endian" comment about formats and only supports AddFB, not AddFB2. The former only has depth/bpp, not the actual format, anyways. This matches what current user-space expects too. (quirk_addfb_prefer_host_byte_order = 1) 2. AddFB2 support with proper formats. Only used for vmwgfx and virgl in practice for BE, IIRC. Only supports 32-bit 8bpc formats, and uses some helpers to just flip around DRM_FORMAT_BIG_ENDIAN bit to an equivalent format in the frontend api handling. This obviously won't work for other formats, but none of the helpers are ready to receive the BIG_ENDIAN bit.
Cheers,
-ilia
Hi Ilia,
On Mon, Mar 14, 2022 at 3:39 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 14, 2022 at 10:06 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 14, 2022 at 2:44 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 14, 2022 at 9:07 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Tue, Mar 8, 2022 at 8:57 AM Geert Uytterhoeven geert@linux-m68k.org wrote:
On Mon, Mar 7, 2022 at 10:23 PM Ilia Mirkin imirkin@alum.mit.edu wrote:
On Mon, Mar 7, 2022 at 3:53 PM Geert Uytterhoeven geert@linux-m68k.org wrote: > diff --git a/tests/util/pattern.c b/tests/util/pattern.c > index 953bf95492ee150c..42d75d700700dc3d 100644 > --- a/tests/util/pattern.c > +++ b/tests/util/pattern.c > @@ -608,6 +608,46 @@ static void fill_smpte_rgb16fp(const struct util_rgb_info *rgb, void *mem, > static unsigned int smpte_middle[7] = { 6, 7, 4, 7, 2, 7, 0 }; > static unsigned int smpte_bottom[8] = { 8, 9, 10, 7, 11, 7, 12, 7 }; > > +static void write_pixel_4(uint8_t *mem, unsigned int x, unsigned int pixel) > +{ > + if (x & 1) > + mem[x / 2] = (mem[x / 2] & 0xf0) | (pixel & 0x0f); > + else > + mem[x / 2] = (mem[x / 2] & 0x0f) | (pixel << 4); > +}
The standard layout is MSB? i.e. first pixel goes in the upper bits of the first byte? It's been ages since I've dealt with C4 (or perhaps I never even touched it), but this seems a bit surprising.
Turns out I was wrong: fbdev ordering follows native ordering, and there's also FBINFO_FOREIGN_ENDIAN :-(
I haven't double-checked the meaning in fbdev, but ENDIAN-ness generally refers to the layout of *bytes*, not *bits*. Although one could also argue that it's the layout of "elements", and so in that way, upper/lower values could be considered flipped. I've never gone that far though.
Yes, usually it refers to the ordering of bytes in a word. Here, it's about the ordering of sub-byte pixels in a byte. Note that with C2 and C4, there's a third ordering that comes into play. So we have:
Ordering of bytes in a word, for bpp > 8,
Ordering of pixels in a byte, for bpp < 8,
Ordering of bits in a pixel, for bpp > 1.
Is handled by DRM_FORMAT_BIG_ENDIAN.
OK. Note that DRM_FORMAT_BIG_ENDIAN flag for formats other than RGBX8888 and very similar formats is basically broken in drm. So ... watch out. There are two setups supported for big-endian currently:
- Legacy: radeon/nouveau, ignore the "little endian" comment about
formats and only supports AddFB, not AddFB2. The former only has depth/bpp, not the actual format, anyways. This matches what current user-space expects too. (quirk_addfb_prefer_host_byte_order = 1) 2. AddFB2 support with proper formats. Only used for vmwgfx and virgl in practice for BE, IIRC. Only supports 32-bit 8bpc formats, and uses some helpers to just flip around DRM_FORMAT_BIG_ENDIAN bit to an equivalent format in the frontend api handling. This obviously won't work for other formats, but none of the helpers are ready to receive the BIG_ENDIAN bit.
I'm fully aware the DRM_FORMAT_BIG_ENDIAN flag is broken, and it's on my list of things to fix (for 16-bpp Atari).
Gr{oetje,eeting}s,
Geert
-- Geert Uytterhoeven -- There's lots of Linux beyond ia32 -- geert@linux-m68k.org
In personal conversations with technical people, I call myself a hacker. But when I'm talking to journalists I just say "programmer" or something like that. -- Linus Torvalds
dri-devel@lists.freedesktop.org
-
Geert Uytterhoeven -
Ilia Mirkin -
Pekka Paalanen