[PATCH 0/3] drm: Endianness fixes

[PATCH 0/3] drm: Endianness fixes

[Geert Uytterhoeven]

	Hi all,

This patch series contains several endianness fixes for the DRM
subsystem.

This has been tested on ARAnyM using a work-in-progress Atari DRM
driver, which supports (a.o.) big-endian RGB565 (more info and related
patches can be found in [1]).

Thanks for your comments!

[1] "[PATCH v3 00/10] drm: Add support for low-color frame buffer formats"
    https://lore.kernel.org/r/cover.1657294931.git.geert@linux-m68k.org

Geert Uytterhoeven (3):
  drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats
  drm/format-helper: Fix endianness in drm_fb_*_to_*() conversion
    helpers
  drm/gud: Fix endianness in gud_xrgb8888_to_color() helper

 drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++------------
 drivers/gpu/drm/drm_fourcc.c        |  4 ++
 drivers/gpu/drm/gud/gud_pipe.c      | 14 ++---
 3 files changed, 58 insertions(+), 40 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 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Geert Uytterhoeven]

As of commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken
bigendian drivers"), drivers must set the
quirk_addfb_prefer_host_byte_order quirk to make the drm_mode_addfb()
compat code work correctly on big-endian machines.

While that works fine for big-endian XRGB8888 and ARGB8888, which are
mapped to the existing little-endian BGRX8888 and BGRA8888 formats, it
does not work for big-endian XRGB1555 and RGB565, as the latter are not
listed in the format database.

Fix this by adding the missing formats.  Limit this to big-endian
platforms, as there is currently no need to support these formats on
little-endian platforms.

Fixes: 6960e6da9cec3f66 ("drm: fix drm_mode_addfb() on big endian machines.")
Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
in cirrus_fb_create, so you cannot get a graphical text console.

Do we need these definitions on little-endian platforms, too?
Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
formats[]?

Alternative, callers could filter out the DRM_FORMAT_BIG_ENDIAN flag
when calling {,__}drm_format_info().

Advantages:
  - No need to have separate entries with DRM_FORMAT_BIG_ENDIAN set.

Disadvantages:
  - {,__}drm_format_info() returns a pointer to a const object,
    whose .format field won't have the DRM_FORMAT_BIG_ENDIAN flag set,
    complicating callers,
  - All callers need to be updated,
  - It is difficult to know which big-endian formats are really
    supported, especially as only a few are needed.
---
 drivers/gpu/drm/drm_fourcc.c | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/drivers/gpu/drm/drm_fourcc.c b/drivers/gpu/drm/drm_fourcc.c
index e09331bb3bc73f21..16a854c9e07fa4b0 100644
--- a/drivers/gpu/drm/drm_fourcc.c
+++ b/drivers/gpu/drm/drm_fourcc.c
@@ -190,6 +190,10 @@ const struct drm_format_info *__drm_format_info(u32 format)
 		{ .format = DRM_FORMAT_BGRA5551,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1, .has_alpha = true },
 		{ .format = DRM_FORMAT_RGB565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_BGR565,		.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+#ifdef __BIG_ENDIAN
+		{ .format = DRM_FORMAT_HOST_XRGB1555,	.depth = 15, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+		{ .format = DRM_FORMAT_HOST_RGB565,	.depth = 16, .num_planes = 1, .cpp = { 2, 0, 0 }, .hsub = 1, .vsub = 1 },
+#endif
 		{ .format = DRM_FORMAT_RGB888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_BGR888,		.depth = 24, .num_planes = 1, .cpp = { 3, 0, 0 }, .hsub = 1, .vsub = 1 },
 		{ .format = DRM_FORMAT_XRGB8888,	.depth = 24, .num_planes = 1, .cpp = { 4, 0, 0 }, .hsub = 1, .vsub = 1 },
-- 
2.25.1

[PATCH 2/3] drm/format-helper: Fix endianness in drm_fb_*_to_*() conversion helpers

[Geert Uytterhoeven]

DRM formats are defined to be little-endian, unless the
DRM_FORMAT_BIG_ENDIAN flag is set.  Hence when converting from one
format to another, multi-byte pixel values loaded from memory must be
converted from little-endian to host-endian.  Conversely, multi-byte
pixel values written to memory must be converted from host-endian to
little-endian.  Currently only drm_fb_xrgb8888_to_rgb332_line() includes
endianness handling.

Fix this by adding endianness handling to all conversion functions that
process multi-byte pixel values.

Note that the conversion to RGB565 is special, as there are two
versions: with and without byteswapping of the RGB565 pixel data.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Tested with WIP atari_drm, which supports native big-endian RGB565.

Notes:
  - Most of these are used only by GUD or simpledrm (the latter through
    drm_fb_blit_toio()).
    Interestingly, drm_fb_xrgb8888_to_rgb332() was introduced for GUD,
    and always had correct endiannes handling...
  - drm_fb_xrgb8888_to_gray8() is also used by st7586.c.
---
 drivers/gpu/drm/drm_format_helper.c | 80 +++++++++++++++++------------
 1 file changed, 46 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index a3ccd8bc966fd816..c6182b5de78b0bd8 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -279,14 +279,16 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb332);
 static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u16 *dbuf16 = dbuf;
-	const u32 *sbuf32 = sbuf;
+	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 	u16 val16;
+	u32 pix;
 
 	for (x = 0; x < pixels; x++) {
-		val16 = ((sbuf32[x] & 0x00F80000) >> 8) |
-			((sbuf32[x] & 0x0000FC00) >> 5) |
-			((sbuf32[x] & 0x000000F8) >> 3);
+		pix = le32_to_cpu(sbuf32[x]);
+		val16 = ((pix & 0x00F80000) >> 8) |
+			((pix & 0x0000FC00) >> 5) |
+			((pix & 0x000000F8) >> 3);
 		dbuf16[x] = val16;
 	}
 }
@@ -295,14 +297,16 @@ static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
 						unsigned int pixels)
 {
 	u16 *dbuf16 = dbuf;
-	const u32 *sbuf32 = sbuf;
+	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 	u16 val16;
+	u32 pix;
 
 	for (x = 0; x < pixels; x++) {
-		val16 = ((sbuf32[x] & 0x00F80000) >> 8) |
-			((sbuf32[x] & 0x0000FC00) >> 5) |
-			((sbuf32[x] & 0x000000F8) >> 3);
+		pix = le32_to_cpu(sbuf32[x]);
+		val16 = ((pix & 0x00F80000) >> 8) |
+			((pix & 0x0000FC00) >> 5) |
+			((pix & 0x000000F8) >> 3);
 		dbuf16[x] = swab16(val16);
 	}
 }
@@ -360,13 +364,15 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565_toio);
 static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
-	const u32 *sbuf32 = sbuf;
+	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
+	u32 pix;
 
 	for (x = 0; x < pixels; x++) {
-		*dbuf8++ = (sbuf32[x] & 0x000000FF) >>  0;
-		*dbuf8++ = (sbuf32[x] & 0x0000FF00) >>  8;
-		*dbuf8++ = (sbuf32[x] & 0x00FF0000) >> 16;
+		pix = le32_to_cpu(sbuf32[x]);
+		*dbuf8++ = (pix & 0x000000FF) >>  0;
+		*dbuf8++ = (pix & 0x0000FF00) >>  8;
+		*dbuf8++ = (pix & 0x00FF0000) >> 16;
 	}
 }
 
@@ -410,17 +416,19 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888_toio);
 
 static void drm_fb_rgb565_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
-	u32 *dbuf32 = dbuf;
-	const u16 *sbuf16 = sbuf;
+	__le32 *dbuf32 = dbuf;
+	const __le16 *sbuf16 = sbuf;
 	unsigned int x;
 
-	for (x = 0; x < pixels; x++, ++sbuf16, ++dbuf32) {
-		u32 val32 = ((*sbuf16 & 0xf800) << 8) |
-			    ((*sbuf16 & 0x07e0) << 5) |
-			    ((*sbuf16 & 0x001f) << 3);
-		*dbuf32 = 0xff000000 | val32 |
-			  ((val32 >> 3) & 0x00070007) |
-			  ((val32 >> 2) & 0x00000300);
+	for (x = 0; x < pixels; x++) {
+		u16 val16 = le16_to_cpu(sbuf16[x]);
+		u32 val32 = ((val16 & 0xf800) << 8) |
+			    ((val16 & 0x07e0) << 5) |
+			    ((val16 & 0x001f) << 3);
+		val32 = 0xff000000 | val32 |
+			((val32 >> 3) & 0x00070007) |
+			((val32 >> 2) & 0x00000300);
+		dbuf32[x] = cpu_to_le32(val32);
 	}
 }
 
@@ -434,7 +442,7 @@ static void drm_fb_rgb565_to_xrgb8888_toio(void __iomem *dst, unsigned int dst_p
 
 static void drm_fb_rgb888_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
-	u32 *dbuf32 = dbuf;
+	__le32 *dbuf32 = dbuf;
 	const u8 *sbuf8 = sbuf;
 	unsigned int x;
 
@@ -442,7 +450,8 @@ static void drm_fb_rgb888_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigne
 		u8 r = *sbuf8++;
 		u8 g = *sbuf8++;
 		u8 b = *sbuf8++;
-		*dbuf32++ = 0xff000000 | (r << 16) | (g << 8) | b;
+		u32 pix = 0xff000000 | (r << 16) | (g << 8) | b;
+		dbuf32[x] = cpu_to_le32(pix);
 	}
 }
 
@@ -456,16 +465,19 @@ static void drm_fb_rgb888_to_xrgb8888_toio(void __iomem *dst, unsigned int dst_p
 
 static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
-	u32 *dbuf32 = dbuf;
-	const u32 *sbuf32 = sbuf;
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 	u32 val32;
+	u32 pix;
 
 	for (x = 0; x < pixels; x++) {
-		val32 = ((sbuf32[x] & 0x000000FF) << 2) |
-			((sbuf32[x] & 0x0000FF00) << 4) |
-			((sbuf32[x] & 0x00FF0000) << 6);
-		*dbuf32++ = val32 | ((val32 >> 8) & 0x00300C03);
+		pix = le32_to_cpu(sbuf32[x]);
+		val32 = ((pix & 0x000000FF) << 2) |
+			((pix & 0x0000FF00) << 4) |
+			((pix & 0x00FF0000) << 6);
+		pix = val32 | ((val32 >> 8) & 0x00300C03);
+		*dbuf32++ = cpu_to_le32(pix);
 	}
 }
 
@@ -494,17 +506,17 @@ EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010_toio);
 static void drm_fb_xrgb8888_to_gray8_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
-	const u32 *sbuf32 = sbuf;
+	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 
 	for (x = 0; x < pixels; x++) {
-		u8 r = (*sbuf32 & 0x00ff0000) >> 16;
-		u8 g = (*sbuf32 & 0x0000ff00) >> 8;
-		u8 b =  *sbuf32 & 0x000000ff;
+		u32 pix = le32_to_cpu(sbuf32[x]);
+		u8 r = (pix & 0x00ff0000) >> 16;
+		u8 g = (pix & 0x0000ff00) >> 8;
+		u8 b =  pix & 0x000000ff;
 
 		/* ITU BT.601: Y = 0.299 R + 0.587 G + 0.114 B */
 		*dbuf8++ = (3 * r + 6 * g + b) / 10;
-		sbuf32++;
 	}
 }
 
-- 
2.25.1

[PATCH 3/3] drm/gud: Fix endianness in gud_xrgb8888_to_color() helper

[Geert Uytterhoeven]

DRM formats are defined to be little-endian, unless the
DRM_FORMAT_BIG_ENDIAN flag is set.  Hence when converting from one
format to another, multi-byte pixel values loaded from memory must be
converted from little-endian to host-endian.  Conversely, multi-byte
pixel values written to memory must be converted from host-endian to
little-endian.  Currently only drm_fb_xrgb8888_to_rgb332_line() includes
endianness handling.

Fix gud_xrgb8888_to_color() on big-endian platforms by adding the
missing endianness handling.

Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
---
Compile-tested only.

Interestingly, drm_fb_xrgb8888_to_rgb332() was introduced for GUD,
and always had correct endiannes handling...
---
 drivers/gpu/drm/gud/gud_pipe.c | 14 ++++++++------
 1 file changed, 8 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
index 4873f9799f412e04..d42592f6daab8b2a 100644
--- a/drivers/gpu/drm/gud/gud_pipe.c
+++ b/drivers/gpu/drm/gud/gud_pipe.c
@@ -105,7 +105,8 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
 	unsigned int bits_per_pixel = 8 / block_width;
 	u8 r, g, b, pix, *block = dst; /* Assign to silence compiler warning */
 	unsigned int x, y, width;
-	u32 *pix32;
+	__le32 *sbuf32;
+	u32 pix32;
 	size_t len;
 
 	/* Start on a byte boundary */
@@ -114,8 +115,8 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
 	len = drm_format_info_min_pitch(format, 0, width) * drm_rect_height(rect);
 
 	for (y = rect->y1; y < rect->y2; y++) {
-		pix32 = src + (y * fb->pitches[0]);
-		pix32 += rect->x1;
+		sbuf32 = src + (y * fb->pitches[0]);
+		sbuf32 += rect->x1;
 
 		for (x = 0; x < width; x++) {
 			unsigned int pixpos = x % block_width; /* within byte from the left */
@@ -126,9 +127,10 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
 				*block = 0;
 			}
 
-			r = *pix32 >> 16;
-			g = *pix32 >> 8;
-			b = *pix32++;
+			pix32 = le32_to_cpu(*sbuf32++);
+			r = pix32 >> 16;
+			g = pix32 >> 8;
+			b = pix32;
 
 			switch (format->format) {
 			case GUD_DRM_FORMAT_XRGB1111:
-- 
2.25.1

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Michel Dänzer]

On 2022-07-08 20:21, Geert Uytterhoeven wrote:
> As of commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken
> bigendian drivers"), drivers must set the
> quirk_addfb_prefer_host_byte_order quirk to make the drm_mode_addfb()
> compat code work correctly on big-endian machines.
> 
> While that works fine for big-endian XRGB8888 and ARGB8888, which are
> mapped to the existing little-endian BGRX8888 and BGRA8888 formats, it
> does not work for big-endian XRGB1555 and RGB565, as the latter are not
> listed in the format database.
> 
> Fix this by adding the missing formats.  Limit this to big-endian
> platforms, as there is currently no need to support these formats on
> little-endian platforms.
> 
> Fixes: 6960e6da9cec3f66 ("drm: fix drm_mode_addfb() on big endian machines.")
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
> and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
> Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
> in cirrus_fb_create, so you cannot get a graphical text console.
> 
> Do we need these definitions on little-endian platforms, too?
> Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
> DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
> formats[]?

The intention of DRM_FORMAT_HOST_* is that they are macros in include/drm/drm_fourcc.h which just map to little endian formats defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible for big endian hosts for XRGB1555 or RGB565 (or any other format with non-8-bit components), this isn't applicable here.

It's also doubtful that Cirrus hardware would access these formats as big endian (drivers/gpu/drm/tiny/cirrus.c has no endianness references at all, and the hardware was surely designed for x86 first and foremost).

Instead, fbcon (and user space) needs to convert to little endian when using DRM_FORMAT_HOST_{XRGB1555,RGB565} with the cirrus driver on big endian hosts.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Geert Uytterhoeven]

Hi Michel,

On Mon, Jul 11, 2022 at 5:23 PM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
> On 2022-07-08 20:21, Geert Uytterhoeven wrote:
> > As of commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken
> > bigendian drivers"), drivers must set the
> > quirk_addfb_prefer_host_byte_order quirk to make the drm_mode_addfb()
> > compat code work correctly on big-endian machines.
> >
> > While that works fine for big-endian XRGB8888 and ARGB8888, which are
> > mapped to the existing little-endian BGRX8888 and BGRA8888 formats, it
> > does not work for big-endian XRGB1555 and RGB565, as the latter are not
> > listed in the format database.
> >
> > Fix this by adding the missing formats.  Limit this to big-endian
> > platforms, as there is currently no need to support these formats on
> > little-endian platforms.
> >
> > Fixes: 6960e6da9cec3f66 ("drm: fix drm_mode_addfb() on big endian machines.")
> > Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> > ---
> > Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
> > and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
> > Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
> > in cirrus_fb_create, so you cannot get a graphical text console.
> >
> > Do we need these definitions on little-endian platforms, too?
> > Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
> > DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
> > formats[]?
>
> The intention of DRM_FORMAT_HOST_* is that they are macros in include/drm/drm_fourcc.h which just map to little endian formats defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible for big endian hosts for XRGB1555 or RGB565 (or any other format with non-8-bit components), this isn't applicable here.

I read that as that you prefer to write
"DRM_FORMAT_{XRGB1555,RGB565} | DRM_FORMAT_BIG_ENDIAN" in formats[]?

> It's also doubtful that Cirrus hardware would access these formats as big endian (drivers/gpu/drm/tiny/cirrus.c has no endianness references at all, and the hardware was surely designed for x86 first and foremost).
>
> Instead, fbcon (and user space) needs to convert to little endian when using DRM_FORMAT_HOST_{XRGB1555,RGB565} with the cirrus driver on big endian hosts.

Yeah, probably the cirrus driver can use some fixes...

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 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Michel Dänzer]

On 2022-07-11 17:30, Geert Uytterhoeven wrote:
> Hi Michel,
> 
> On Mon, Jul 11, 2022 at 5:23 PM Michel Dänzer
> <michel.daenzer@mailbox.org> wrote:
>> On 2022-07-08 20:21, Geert Uytterhoeven wrote:
>>> As of commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken
>>> bigendian drivers"), drivers must set the
>>> quirk_addfb_prefer_host_byte_order quirk to make the drm_mode_addfb()
>>> compat code work correctly on big-endian machines.
>>>
>>> While that works fine for big-endian XRGB8888 and ARGB8888, which are
>>> mapped to the existing little-endian BGRX8888 and BGRA8888 formats, it
>>> does not work for big-endian XRGB1555 and RGB565, as the latter are not
>>> listed in the format database.
>>>
>>> Fix this by adding the missing formats.  Limit this to big-endian
>>> platforms, as there is currently no need to support these formats on
>>> little-endian platforms.
>>>
>>> Fixes: 6960e6da9cec3f66 ("drm: fix drm_mode_addfb() on big endian machines.")
>>> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
>>> ---
>>> Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
>>> and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
>>> Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
>>> in cirrus_fb_create, so you cannot get a graphical text console.
>>>
>>> Do we need these definitions on little-endian platforms, too?
>>> Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
>>> DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
>>> formats[]?
>>
>> The intention of DRM_FORMAT_HOST_* is that they are macros in include/drm/drm_fourcc.h which just map to little endian formats defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible for big endian hosts for XRGB1555 or RGB565 (or any other format with non-8-bit components), this isn't applicable here.
> 
> I read that as that you prefer to write
> "DRM_FORMAT_{XRGB1555,RGB565} | DRM_FORMAT_BIG_ENDIAN" in formats[]?

In other drivers for hardware which can access these formats as big endian, yes.

Note that AFAIK little if any user-space code uses DRM_FORMAT_BIG_ENDIAN yet though.


>> It's also doubtful that Cirrus hardware would access these formats as big endian (drivers/gpu/drm/tiny/cirrus.c has no endianness references at all, and the hardware was surely designed for x86 first and foremost).
>>
>> Instead, fbcon (and user space) needs to convert to little endian when using DRM_FORMAT_HOST_{XRGB1555,RGB565} with the cirrus driver on big endian hosts.
> 
> Yeah, probably the cirrus driver can use some fixes...

I suspect the fix here would rather need to be in the DRM glue code for fbcon than in the driver. Or maybe some kind of byte-swapping helper(s) which can be used by drivers.


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Gerd Hoffmann]

On Mon, Jul 11, 2022 at 05:30:30PM +0200, Geert Uytterhoeven wrote:
> Hi Michel,
> 
> > > Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
> > > and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
> > > Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
> > > in cirrus_fb_create, so you cannot get a graphical text console.
> > >
> > > Do we need these definitions on little-endian platforms, too?
> > > Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
> > > DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
> > > formats[]?
> >
> > The intention of DRM_FORMAT_HOST_* is that they are macros in
> > include/drm/drm_fourcc.h which just map to little endian formats
> > defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible
> > for big endian hosts for XRGB1555 or RGB565 (or any other format
> > with non-8-bit components), this isn't applicable here.

It IMHO is not applicable to any physical hardware.  It's used by
virtio-gpu where the supported format depends on the byte order
(it is argb8888 in native byte order).  Only virtual hardware can
have that kind of behavior.

And we can probably drop the DRM_FORMAT_HOST_* variants for 1555 and
565, they are not used anywhere.

> I read that as that you prefer to write "DRM_FORMAT_{XRGB1555,RGB565}
> | DRM_FORMAT_BIG_ENDIAN" in formats[]?

Agree.

> > It's also doubtful that Cirrus hardware would access these formats
> > as big endian (drivers/gpu/drm/tiny/cirrus.c has no endianness
> > references at all, and the hardware was surely designed for x86
> > first and foremost).

Yes.  qemu mimics physical cirrus hardware which uses little endian.

> > Instead, fbcon (and user space) needs to convert to little endian
> > when using DRM_FORMAT_HOST_{XRGB1555,RGB565} with the cirrus driver
> > on big endian hosts.

Well, the cirrus driver uses shadow framebuffers anyway (the only
workable approach given it has 4M vram only), and it also supports
converting formats on-the-fly when copying from shadow to vram.

So adding support for bigendian formats to the driver shouldn't be
much of a problem.  The vram will continue to run in little endian
RGB565, the shadow will be big endian RGB565, and the driver must
byteswap when copying.

> Yeah, probably the cirrus driver can use some fixes...

I'd call it improvements.  It's not like the cirrus driver is broken.

take care,
  Gerd

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Geert Uytterhoeven]

Hi Gerd,

On Tue, Jul 12, 2022 at 9:47 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Mon, Jul 11, 2022 at 05:30:30PM +0200, Geert Uytterhoeven wrote:
> > > > Cirrus is the only driver setting quirk_addfb_prefer_host_byte_order
> > > > and supporting RGB565 or XRGB1555, but no one tried that on big-endian?
> > > > Cirrus does not support DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN
> > > > in cirrus_fb_create, so you cannot get a graphical text console.
> > > >
> > > > Do we need these definitions on little-endian platforms, too?
> > > > Would it be better to use "DRM_FORMAT_{XRGB1555,RGB565} |
> > > > DRM_FORMAT_BIG_ENDIAN" instead of "DRM_FORMAT_HOST_{XRGB1555,RGB565}" in
> > > > formats[]?
> > >
> > > The intention of DRM_FORMAT_HOST_* is that they are macros in
> > > include/drm/drm_fourcc.h which just map to little endian formats
> > > defined in drivers/gpu/drm/drm_fourcc.c. Since this is not possible
> > > for big endian hosts for XRGB1555 or RGB565 (or any other format
> > > with non-8-bit components), this isn't applicable here.
>
> It IMHO is not applicable to any physical hardware.  It's used by
> virtio-gpu where the supported format depends on the byte order
> (it is argb8888 in native byte order).  Only virtual hardware can
> have that kind of behavior.
>
> And we can probably drop the DRM_FORMAT_HOST_* variants for 1555 and
> 565, they are not used anywhere.

Atari DRM supports (big-endian) RGB565, so it uses
DRM_FORMAT_HOST_RGB565.

The alternative is to drop the quirk_addfb_prefer_host_byte_order
requirement on big-endian, and always use a little-endian RGB565
shadow frame buffer, at the expense of never being able to get rid
of the copying and byteswapping.

[Cirrus discussion removed]

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 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Gerd Hoffmann]

  Hi,

> So adding support for bigendian formats to the driver shouldn't be
> much of a problem.  The vram will continue to run in little endian
> RGB565, the shadow will be big endian RGB565, and the driver must
> byteswap when copying.

For completeness: The other obvious option (for fbcon) would be to
handle the byteswapping in the generic drm fbdev emulation, which
would have the advantage that it would be more generic and would
not depend on the drm driver supporting the bigendian rgb565
formats ...

take care,
  Gerd

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Gerd Hoffmann]

On Tue, Jul 12, 2022 at 10:01:15AM +0200, Geert Uytterhoeven wrote:
> Hi Gerd,
> 
> > It IMHO is not applicable to any physical hardware.  It's used by
> > virtio-gpu where the supported format depends on the byte order
> > (it is argb8888 in native byte order).  Only virtual hardware can
> > have that kind of behavior.
> >
> > And we can probably drop the DRM_FORMAT_HOST_* variants for 1555 and
> > 565, they are not used anywhere.
> 
> Atari DRM supports (big-endian) RGB565, so it uses
> DRM_FORMAT_HOST_RGB565.

Fixed big endian should use 'DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN'.

As described above DRM_FORMAT_HOST_RGB565 means bigendian on bigendian
hosts and little endian on little endian hosts.  Which is not correct
when your hardware does big endian no matter what.

take care,
  Gerd

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Geert Uytterhoeven]

Hi Gerd,

On Tue, Jul 12, 2022 at 10:39 AM Gerd Hoffmann <kraxel@redhat.com> wrote:
> On Tue, Jul 12, 2022 at 10:01:15AM +0200, Geert Uytterhoeven wrote:
> > > It IMHO is not applicable to any physical hardware.  It's used by
> > > virtio-gpu where the supported format depends on the byte order
> > > (it is argb8888 in native byte order).  Only virtual hardware can
> > > have that kind of behavior.
> > >
> > > And we can probably drop the DRM_FORMAT_HOST_* variants for 1555 and
> > > 565, they are not used anywhere.
> >
> > Atari DRM supports (big-endian) RGB565, so it uses
> > DRM_FORMAT_HOST_RGB565.
>
> Fixed big endian should use 'DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN'.

True.

> As described above DRM_FORMAT_HOST_RGB565 means bigendian on bigendian
> hosts and little endian on little endian hosts.  Which is not correct
> when your hardware does big endian no matter what.

But (a) drm_driver_legacy_fb_format() uses DRM_FORMAT_HOST_RGB565
if quirk_addfb_prefer_host_byte_order is set, and (b)
quirk_addfb_prefer_host_byte_order must be set on big-endian as
per commit eae06120f1974e1a ("drm: refuse ADDFB2 ioctl for broken
bigendian drivers").

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 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Gerd Hoffmann]

> > As described above DRM_FORMAT_HOST_RGB565 means bigendian on bigendian
> > hosts and little endian on little endian hosts.  Which is not correct
> > when your hardware does big endian no matter what.
> 
> But (a) drm_driver_legacy_fb_format() uses DRM_FORMAT_HOST_RGB565
> if quirk_addfb_prefer_host_byte_order is set,

Ah, right.  Missed that in 'git grep' output.  Given that traditional
fbdev behavior is to expect native byte order using
DRM_FORMAT_HOST_RGB565 there makes sense indeed.

Scratch my comment about it being unused then ;)

thanks,
  Gerd

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Michel Dänzer]

On 2022-07-12 11:03, Gerd Hoffmann wrote:
>>> As described above DRM_FORMAT_HOST_RGB565 means bigendian on bigendian
>>> hosts and little endian on little endian hosts.  Which is not correct
>>> when your hardware does big endian no matter what.
>>
>> But (a) drm_driver_legacy_fb_format() uses DRM_FORMAT_HOST_RGB565
>> if quirk_addfb_prefer_host_byte_order is set,
> 
> Ah, right.  Missed that in 'git grep' output.  Given that traditional
> fbdev behavior is to expect native byte order using
> DRM_FORMAT_HOST_RGB565 there makes sense indeed.
> 
> Scratch my comment about it being unused then ;)

DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN is still what the driver should use conceptually, and should match DRM_FORMAT_HOST_RGB565 in drm_driver_legacy_fb_format on a big endian host (which is presumably always the case for the atari driver).


-- 
Earthling Michel Dänzer            |                  https://redhat.com
Libre software enthusiast          |         Mesa and Xwayland developer

Re: [PATCH 1/3] drm/fourcc: Add missing big-endian XRGB1555 and RGB565 formats

[Geert Uytterhoeven]

Hi Michel,

On Tue, Jul 12, 2022 at 11:09 AM Michel Dänzer
<michel.daenzer@mailbox.org> wrote:
> On 2022-07-12 11:03, Gerd Hoffmann wrote:
> >>> As described above DRM_FORMAT_HOST_RGB565 means bigendian on bigendian
> >>> hosts and little endian on little endian hosts.  Which is not correct
> >>> when your hardware does big endian no matter what.
> >>
> >> But (a) drm_driver_legacy_fb_format() uses DRM_FORMAT_HOST_RGB565
> >> if quirk_addfb_prefer_host_byte_order is set,
> >
> > Ah, right.  Missed that in 'git grep' output.  Given that traditional
> > fbdev behavior is to expect native byte order using
> > DRM_FORMAT_HOST_RGB565 there makes sense indeed.
> >
> > Scratch my comment about it being unused then ;)
>
> DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN is still what the driver should use conceptually, and should match DRM_FORMAT_HOST_RGB565 in drm_driver_legacy_fb_format on a big endian host (which is presumably always the case for the atari driver).

Sure, I'll update the patch accordingly.

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 3/3] drm/gud: Fix endianness in gud_xrgb8888_to_color() helper

[Noralf Trønnes]

Den 08.07.2022 20.21, skrev Geert Uytterhoeven:
> DRM formats are defined to be little-endian, unless the
> DRM_FORMAT_BIG_ENDIAN flag is set.  Hence when converting from one
> format to another, multi-byte pixel values loaded from memory must be
> converted from little-endian to host-endian.  Conversely, multi-byte
> pixel values written to memory must be converted from host-endian to
> little-endian.  Currently only drm_fb_xrgb8888_to_rgb332_line() includes
> endianness handling.
> 
> Fix gud_xrgb8888_to_color() on big-endian platforms by adding the
> missing endianness handling.
> 
> Signed-off-by: Geert Uytterhoeven <geert@linux-m68k.org>
> ---
> Compile-tested only.
> 
> Interestingly, drm_fb_xrgb8888_to_rgb332() was introduced for GUD,
> and always had correct endiannes handling...

RGB332 support was added later and by that time I had understood that
the framebuffer was little endian and not host endian as I first assumed
(there's a fixme comment in gud_pipe.c that BE is probably broken but I
haven't got any hw to test on so I haven't tried to fix it).

Thanks for fixing this, pathces 2 and 3 tested on drm/gud and applied to
drm-misc-next.

Noralf.

> ---
>  drivers/gpu/drm/gud/gud_pipe.c | 14 ++++++++------
>  1 file changed, 8 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/gud/gud_pipe.c b/drivers/gpu/drm/gud/gud_pipe.c
> index 4873f9799f412e04..d42592f6daab8b2a 100644
> --- a/drivers/gpu/drm/gud/gud_pipe.c
> +++ b/drivers/gpu/drm/gud/gud_pipe.c
> @@ -105,7 +105,8 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
>  	unsigned int bits_per_pixel = 8 / block_width;
>  	u8 r, g, b, pix, *block = dst; /* Assign to silence compiler warning */
>  	unsigned int x, y, width;
> -	u32 *pix32;
> +	__le32 *sbuf32;
> +	u32 pix32;
>  	size_t len;
>  
>  	/* Start on a byte boundary */
> @@ -114,8 +115,8 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
>  	len = drm_format_info_min_pitch(format, 0, width) * drm_rect_height(rect);
>  
>  	for (y = rect->y1; y < rect->y2; y++) {
> -		pix32 = src + (y * fb->pitches[0]);
> -		pix32 += rect->x1;
> +		sbuf32 = src + (y * fb->pitches[0]);
> +		sbuf32 += rect->x1;
>  
>  		for (x = 0; x < width; x++) {
>  			unsigned int pixpos = x % block_width; /* within byte from the left */
> @@ -126,9 +127,10 @@ static size_t gud_xrgb8888_to_color(u8 *dst, const struct drm_format_info *forma
>  				*block = 0;
>  			}
>  
> -			r = *pix32 >> 16;
> -			g = *pix32 >> 8;
> -			b = *pix32++;
> +			pix32 = le32_to_cpu(*sbuf32++);
> +			r = pix32 >> 16;
> +			g = pix32 >> 8;
> +			b = pix32;
>  
>  			switch (format->format) {
>  			case GUD_DRM_FORMAT_XRGB1111: