[PATCH libdrm 0/3] Add support for low-color frame buffer formats

[PATCH libdrm 0/3] Add support for low-color frame buffer formats

[Geert Uytterhoeven]

	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(-)

-- 
2.25.1

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

[PATCH libdrm 1/3] util: Optimize C8 SMPTE color LUT

[Geert Uytterhoeven]

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);
 
-- 
2.25.1

[PATCH libdrm 2/3] drm_fourcc: Add DRM_FORMAT_C[124]

[Geert Uytterhoeven]

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) },
-- 
2.25.1

Re: [PATCH libdrm 2/3] drm_fourcc: Add DRM_FORMAT_C[124]

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 2951 bytes --]

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) },


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Geert Uytterhoeven]

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:
-- 
2.25.1

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Ilia Mirkin]

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.

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Geert Uytterhoeven]

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);
> > +       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.

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

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Geert Uytterhoeven]

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);
> > > +       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.
>
> 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.1646683502.git.geert@linux-m68k.org/

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

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Ilia Mirkin]

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);
> > > > +       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.
> >
> > 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

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Geert Uytterhoeven]

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);
> > > > > +       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.
> > >
> > > 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

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Ilia Mirkin]

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);
> > > > > > +       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.
> > > >
> > > > 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.

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

Re: [PATCH libdrm 3/3] util: Add SMPTE pattern support for C4 format

[Geert Uytterhoeven]

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:
> >   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.
>
> 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.

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