[PATCH v3 00/13] drm: Fix color-format selection in fbdev emulation

[PATCH v3 00/13] drm: Fix color-format selection in fbdev emulation

[Thomas Zimmermann]

Fix the selection of the fbdev emulation's color format and make
XRGB8888 the only emulated color format. Resolves the blank screen
in cases where video= specifies an unsupported color format. Also
resolves the issues around current format-conversion helpers.

Version 2 of the patchset fixes the format-helper test cases on
big-endian platforms. This involves some changes to existing tests
as well.

DRM drivers usually pick a default format for their fbdev emulation.
Via the kernel's video= parameter, users can specify a different
format. If the given format is unsupported by the driver, the fbdev
console screen remains dark. As the console is essential to many
systems, not displaying anything is to be avoided.

Patch 1 fixes the detection of the firmware's native color format.
The meaning of several color parameters is inconsistent among Linux
and various standards. Take this into account.

Patches 2 to 5 fix the existing conversion helpers and test cases
for big-endian platforms. These patches are new in version 2 of the
patchset.

As drivers are supposed to provide XRGB8888 as a default fallback
format, provide XRGB8888 conversion helpers in patches 6 to 9. The
new helpers handle cases where the client uses a XRGB8888 framebuffer
and the display scanout buffer uses a different format. All scanout
formats of the simplefb infrastructure should now be covered. The
patchset also extend the Kunit tests for the new formats.

With format conversion in place, patches 10 and 11 fix the single-probe
function's format selection. The helper now goes over the given video=
parameters until it finds a compatible format. If none is found, the
uses driver's default format.

Patches 12 and 13 clean up DRM code in drivers and helpers.

Tested on x86-64 with EFI output and x86 with various VESA color
modes. Also tested on ppc64 with OF output.

v3:
	* use endian-specific types in format helpers (Jose, LKP bot)
v2:
	* fix problems with big-endian platforms

Thomas Zimmermann (13):
  firmware/sysfb: Fix EFI/VESA format selection
  drm/format-helper: Comment on RGB888 byte order
  drm/format-helper: Fix test-input format conversion
  drm/format-helper: Store RGB565 in little-endian order
  drm/format-helper: Type fixes in format-helper tests
  drm/format-helper: Flip src/dst-format branches in blit helper
  drm/format-helper: Add conversion from XRGB8888 to ARGB8888
  drm/format-helper: Add conversion from XRGB8888 to ARGB2101010
  drm/format-helper: Add conversion from XRGB8888 to 15-bit RGB555
    formats
  drm/fh-helper: Split fbdev single-probe helper
  drm/fb-helper: Fix single-probe color-format selection
  drm/format-helper: Simplify drm_fb_build_fourcc_list()
  drm/format-helper: Remove unnecessary conversion helpers

 drivers/firmware/sysfb_simplefb.c             |  43 +-
 drivers/gpu/drm/drm_fb_helper.c               | 252 ++++++----
 drivers/gpu/drm/drm_format_helper.c           | 462 +++++++++++++-----
 .../gpu/drm/tests/drm_format_helper_test.c    | 386 ++++++++++++++-
 drivers/gpu/drm/tiny/ofdrm.c                  |  20 -
 drivers/gpu/drm/tiny/simpledrm.c              |  21 -
 include/drm/drm_format_helper.h               |  16 +-
 7 files changed, 897 insertions(+), 303 deletions(-)

-- 
2.39.0

[PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Thomas Zimmermann]

Select color format for EFI/VESA firmware scanout buffer from the
number of bits per pixel and the position of the individual color
components. Fixes the selected format for the buffer in several odd
cases. For example, XRGB1555 has been reported as ARGB1555 because
of the different use of depth and transparency in VESA and Linux.

Bits-per-pixel is always the pixel's raw number of bits; including
alpha and filler bits. It is preferred over color depth, which has a
different meaning among various components and standards.

Also do not compare reserved bits and transparency bits to each other.
These values have different meanings, as reserved bits include filler
bits while transparency does not.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb_simplefb.c | 43 ++++++++++++++++++++++++++-----
 1 file changed, 37 insertions(+), 6 deletions(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index a353e27f83f5..ce9c007ed66f 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -27,25 +27,56 @@ static const struct simplefb_format formats[] = SIMPLEFB_FORMATS;
 __init bool sysfb_parse_mode(const struct screen_info *si,
 			     struct simplefb_platform_data *mode)
 {
-	const struct simplefb_format *f;
 	__u8 type;
+	u32 bits_per_pixel;
 	unsigned int i;
 
 	type = si->orig_video_isVGA;
 	if (type != VIDEO_TYPE_VLFB && type != VIDEO_TYPE_EFI)
 		return false;
 
+	/*
+	 * The meaning of depth and bpp for direct-color formats is
+	 * inconsistent:
+	 *
+	 *  - DRM format info specifies depth as the number of color
+	 *    bits; including alpha, but not including filler bits.
+	 *  - Linux' EFI platform code computes lfb_depth from the
+	 *    individual color channels, including the reserved bits.
+	 *  - VBE 1.1 defines lfb_depth for XRGB1555 as 16, but later
+	 *    versions use 15.
+	 *  - On the kernel command line, 'bpp' of 32 is usually
+	 *    XRGB8888 including the filler bits, but 15 is XRGB1555
+	 *    not including the filler bit.
+	 *
+	 * It's not easily possible to fix this in struct screen_info,
+	 * as this could break UAPI. The best solution is to compute
+	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
+	 * ignore simplefb formats with alpha bits, as EFI and VESA
+	 * don't specify alpha channels.
+	 */
+	if (si->lfb_depth > 8) {
+		bits_per_pixel = max(max3(si->red_size + si->red_pos,
+					  si->green_size + si->green_pos,
+					  si->blue_size + si->blue_pos),
+				     si->rsvd_size + si->rsvd_pos);
+	} else {
+		bits_per_pixel = si->lfb_depth;
+	}
+
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
-		f = &formats[i];
-		if (si->lfb_depth == f->bits_per_pixel &&
+		const struct simplefb_format *f = &formats[i];
+
+		if (f->transp.length)
+			continue; /* transparent formats are unsupported by VESA/EFI */
+
+		if (bits_per_pixel == f->bits_per_pixel &&
 		    si->red_size == f->red.length &&
 		    si->red_pos == f->red.offset &&
 		    si->green_size == f->green.length &&
 		    si->green_pos == f->green.offset &&
 		    si->blue_size == f->blue.length &&
-		    si->blue_pos == f->blue.offset &&
-		    si->rsvd_size == f->transp.length &&
-		    si->rsvd_pos == f->transp.offset) {
+		    si->blue_pos == f->blue.offset) {
 			mode->format = f->name;
 			mode->width = si->lfb_width;
 			mode->height = si->lfb_height;
-- 
2.39.0

[PATCH v3 02/13] drm/format-helper: Comment on RGB888 byte order

[Thomas Zimmermann]

RGB888 is different than the other formats as most of its pixels are
unaligned and therefore helper functions do not use endianness conversion
helpers. Comment on this in the source code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c            | 1 +
 drivers/gpu/drm/tests/drm_format_helper_test.c | 4 ++++
 2 files changed, 5 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 74ff33c2ddaa..b98bd7c5caee 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -404,6 +404,7 @@ static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigne
 
 	for (x = 0; x < pixels; x++) {
 		pix = le32_to_cpu(sbuf32[x]);
+		/* write blue-green-red to output in little endianness */
 		*dbuf8++ = (pix & 0x000000FF) >>  0;
 		*dbuf8++ = (pix & 0x0000FF00) >>  8;
 		*dbuf8++ = (pix & 0x00FF0000) >> 16;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 2191e57f2297..cd1d7da3483c 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -407,6 +407,10 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
+	/*
+	 * RGB888 expected results are already in little-endian
+	 * order, so there's no need to convert the test output.
+	 */
 	drm_fb_xrgb8888_to_rgb888(&dst, &result->dst_pitch, &src, &fb, &params->clip);
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
 }
-- 
2.39.0

[PATCH v3 03/13] drm/format-helper: Fix test-input format conversion

[Thomas Zimmermann]

Convert test input for format helpers from host byte order to
little-endian order. The current code does it the other way around,
but there's no effective difference to the result.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 .../gpu/drm/tests/drm_format_helper_test.c    | 35 +++++++++++++------
 1 file changed, 25 insertions(+), 10 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index cd1d7da3483c..e7c49e6d3f6d 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -279,6 +279,21 @@ static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
 	return dst;
 }
 
+static __le32 *cpubuf_to_le32(struct kunit *test, const u32 *buf, size_t buf_size)
+{
+	__le32 *dst = NULL;
+	int n;
+
+	dst = kunit_kzalloc(test, sizeof(*dst) * buf_size, GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	for (n = 0; n < buf_size; n++)
+		dst[n] = cpu_to_le32(buf[n]);
+
+	return dst;
+}
+
 static void convert_xrgb8888_case_desc(struct convert_xrgb8888_case *t,
 				       char *desc)
 {
@@ -294,7 +309,7 @@ static void drm_test_fb_xrgb8888_to_gray8(struct kunit *test)
 	const struct convert_to_gray8_result *result = &params->gray8_result;
 	size_t dst_size;
 	__u8 *buf = NULL;
-	__u32 *xrgb8888 = NULL;
+	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
 	struct drm_framebuffer fb = {
@@ -310,7 +325,7 @@ static void drm_test_fb_xrgb8888_to_gray8(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
 	iosys_map_set_vaddr(&dst, buf);
 
-	xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
@@ -324,7 +339,7 @@ static void drm_test_fb_xrgb8888_to_rgb332(struct kunit *test)
 	const struct convert_to_rgb332_result *result = &params->rgb332_result;
 	size_t dst_size;
 	__u8 *buf = NULL;
-	__u32 *xrgb8888 = NULL;
+	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
 	struct drm_framebuffer fb = {
@@ -340,7 +355,7 @@ static void drm_test_fb_xrgb8888_to_rgb332(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
 	iosys_map_set_vaddr(&dst, buf);
 
-	xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
@@ -354,7 +369,7 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	const struct convert_to_rgb565_result *result = &params->rgb565_result;
 	size_t dst_size;
 	__u16 *buf = NULL;
-	__u32 *xrgb8888 = NULL;
+	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
 	struct drm_framebuffer fb = {
@@ -370,7 +385,7 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
 	iosys_map_set_vaddr(&dst, buf);
 
-	xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
@@ -387,7 +402,7 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	const struct convert_to_rgb888_result *result = &params->rgb888_result;
 	size_t dst_size;
 	__u8 *buf = NULL;
-	__u32 *xrgb8888 = NULL;
+	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
 	struct drm_framebuffer fb = {
@@ -403,7 +418,7 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
 	iosys_map_set_vaddr(&dst, buf);
 
-	xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
@@ -421,7 +436,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	const struct convert_to_xrgb2101010_result *result = &params->xrgb2101010_result;
 	size_t dst_size;
 	__u32 *buf = NULL;
-	__u32 *xrgb8888 = NULL;
+	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
 	struct drm_framebuffer fb = {
@@ -437,7 +452,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
 	iosys_map_set_vaddr(&dst, buf);
 
-	xrgb8888 = le32buf_to_cpu(test, params->xrgb8888, TEST_BUF_SIZE);
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
 	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
 	iosys_map_set_vaddr(&src, xrgb8888);
 
-- 
2.39.0

[PATCH v3 04/13] drm/format-helper: Store RGB565 in little-endian order

[Thomas Zimmermann]

Fix to-RGB565 conversion helpers to store the result in little-
endian byte order. Update test cases as well.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c           |  9 +++++----
 .../gpu/drm/tests/drm_format_helper_test.c    | 20 ++++++++++++++++++-
 2 files changed, 24 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index b98bd7c5caee..f3f3b3809a3e 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -322,7 +322,7 @@ 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;
+	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 	u16 val16;
@@ -333,14 +333,15 @@ static void drm_fb_xrgb8888_to_rgb565_line(void *dbuf, const void *sbuf, unsigne
 		val16 = ((pix & 0x00F80000) >> 8) |
 			((pix & 0x0000FC00) >> 5) |
 			((pix & 0x000000F8) >> 3);
-		dbuf16[x] = val16;
+		dbuf16[x] = cpu_to_le16(val16);
 	}
 }
 
+/* TODO: implement this helper as conversion to RGB565|BIG_ENDIAN */
 static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
 						unsigned int pixels)
 {
-	u16 *dbuf16 = dbuf;
+	__le16 *dbuf16 = dbuf;
 	const __le32 *sbuf32 = sbuf;
 	unsigned int x;
 	u16 val16;
@@ -351,7 +352,7 @@ static void drm_fb_xrgb8888_to_rgb565_swab_line(void *dbuf, const void *sbuf,
 		val16 = ((pix & 0x00F80000) >> 8) |
 			((pix & 0x0000FC00) >> 5) |
 			((pix & 0x000000F8) >> 3);
-		dbuf16[x] = swab16(val16);
+		dbuf16[x] = cpu_to_le16(swab16(val16));
 	}
 }
 
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index e7c49e6d3f6d..04fe373c9d97 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -264,6 +264,21 @@ static size_t conversion_buf_size(u32 dst_format, unsigned int dst_pitch,
 	return dst_pitch * drm_rect_height(clip);
 }
 
+static u16 *le16buf_to_cpu(struct kunit *test, const __le16 *buf, size_t buf_size)
+{
+	u16 *dst = NULL;
+	int n;
+
+	dst = kunit_kzalloc(test, sizeof(*dst) * buf_size, GFP_KERNEL);
+	if (!dst)
+		return NULL;
+
+	for (n = 0; n < buf_size; n++)
+		dst[n] = le16_to_cpu(buf[n]);
+
+	return dst;
+}
+
 static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
 {
 	u32 *dst = NULL;
@@ -368,7 +383,7 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	const struct convert_xrgb8888_case *params = test->param_value;
 	const struct convert_to_rgb565_result *result = &params->rgb565_result;
 	size_t dst_size;
-	__u16 *buf = NULL;
+	u16 *buf = NULL;
 	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
@@ -390,9 +405,12 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	iosys_map_set_vaddr(&src, xrgb8888);
 
 	drm_fb_xrgb8888_to_rgb565(&dst, &result->dst_pitch, &src, &fb, &params->clip, false);
+	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
 
+	buf = dst.vaddr; /* restore original value of buf */
 	drm_fb_xrgb8888_to_rgb565(&dst, &result->dst_pitch, &src, &fb, &params->clip, true);
+	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected_swab, dst_size), 0);
 }
 
-- 
2.39.0

[PATCH v3 05/13] drm/format-helper: Type fixes in format-helper tests

[Thomas Zimmermann]

Change the source-buffer type of le32buf_to_cpu() to __le32* to
reflect endianness. Result buffers are converted to local endianness,
so instantiate them from regular u8 or u32 types.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Maíra Canal <mcanal@igalia.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/tests/drm_format_helper_test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index 04fe373c9d97..c2411ec808a1 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -279,7 +279,7 @@ static u16 *le16buf_to_cpu(struct kunit *test, const __le16 *buf, size_t buf_siz
 	return dst;
 }
 
-static u32 *le32buf_to_cpu(struct kunit *test, const u32 *buf, size_t buf_size)
+static u32 *le32buf_to_cpu(struct kunit *test, const __le32 *buf, size_t buf_size)
 {
 	u32 *dst = NULL;
 	int n;
@@ -323,7 +323,7 @@ static void drm_test_fb_xrgb8888_to_gray8(struct kunit *test)
 	const struct convert_xrgb8888_case *params = test->param_value;
 	const struct convert_to_gray8_result *result = &params->gray8_result;
 	size_t dst_size;
-	__u8 *buf = NULL;
+	u8 *buf = NULL;
 	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
@@ -353,7 +353,7 @@ static void drm_test_fb_xrgb8888_to_rgb332(struct kunit *test)
 	const struct convert_xrgb8888_case *params = test->param_value;
 	const struct convert_to_rgb332_result *result = &params->rgb332_result;
 	size_t dst_size;
-	__u8 *buf = NULL;
+	u8 *buf = NULL;
 	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
@@ -419,7 +419,7 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	const struct convert_xrgb8888_case *params = test->param_value;
 	const struct convert_to_rgb888_result *result = &params->rgb888_result;
 	size_t dst_size;
-	__u8 *buf = NULL;
+	u8 *buf = NULL;
 	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
@@ -453,7 +453,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	const struct convert_xrgb8888_case *params = test->param_value;
 	const struct convert_to_xrgb2101010_result *result = &params->xrgb2101010_result;
 	size_t dst_size;
-	__u32 *buf = NULL;
+	u32 *buf = NULL;
 	__le32 *xrgb8888 = NULL;
 	struct iosys_map dst, src;
 
@@ -475,7 +475,7 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	iosys_map_set_vaddr(&src, xrgb8888);
 
 	drm_fb_xrgb8888_to_xrgb2101010(&dst, &result->dst_pitch, &src, &fb, &params->clip);
-	buf = le32buf_to_cpu(test, buf, dst_size / sizeof(u32));
+	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
 }
 
-- 
2.39.0

[PATCH v3 06/13] drm/format-helper: Flip src/dst-format branches in blit helper

[Thomas Zimmermann]

Upcoming changes to the format conversion will mostly blit from
XRGB8888 to some other format. So put the source format in blit's
outer branches to make the code more readable. For cases where
a format only changes its endianness, such as XRGB565, introduce
dedicated branches that handle this for all formats.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_format_helper.c | 44 +++++++++++++----------------
 1 file changed, 20 insertions(+), 24 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index f3f3b3809a3e..36d2ca9d0f01 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -653,41 +653,37 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 	if (dst_format == DRM_FORMAT_ARGB2101010)
 		dst_format = DRM_FORMAT_XRGB2101010;
 
-	if (dst_format == fb_format) {
+	if (fb_format == dst_format) {
 		drm_fb_memcpy(dst, dst_pitch, src, fb, clip);
 		return 0;
-
-	} else if (dst_format == DRM_FORMAT_RGB565) {
-		if (fb_format == DRM_FORMAT_XRGB8888) {
+	} else if (fb_format == (dst_format | DRM_FORMAT_BIG_ENDIAN)) {
+		drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+		return 0;
+	} else if (fb_format == (dst_format & ~DRM_FORMAT_BIG_ENDIAN)) {
+		drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+		return 0;
+	} else if (fb_format == DRM_FORMAT_XRGB8888) {
+		if (dst_format == DRM_FORMAT_RGB565) {
 			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
 			return 0;
-		}
-	} else if (dst_format == (DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN)) {
-		if (fb_format == DRM_FORMAT_RGB565) {
-			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
-			return 0;
-		}
-	} else if (dst_format == DRM_FORMAT_RGB888) {
-		if (fb_format == DRM_FORMAT_XRGB8888) {
+		} else if (dst_format == DRM_FORMAT_RGB888) {
 			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
 			return 0;
-		}
-	} else if (dst_format == DRM_FORMAT_XRGB8888) {
-		if (fb_format == DRM_FORMAT_RGB888) {
-			drm_fb_rgb888_to_xrgb8888(dst, dst_pitch, src, fb, clip);
+		} else if (dst_format == DRM_FORMAT_XRGB2101010) {
+			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
 			return 0;
-		} else if (fb_format == DRM_FORMAT_RGB565) {
-			drm_fb_rgb565_to_xrgb8888(dst, dst_pitch, src, fb, clip);
+		} else if (dst_format == DRM_FORMAT_BGRX8888) {
+			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
 			return 0;
 		}
-	} else if (dst_format == DRM_FORMAT_XRGB2101010) {
-		if (fb_format == DRM_FORMAT_XRGB8888) {
-			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
+	} else if (fb_format == DRM_FORMAT_RGB888) {
+		if (dst_format == DRM_FORMAT_XRGB8888) {
+			drm_fb_rgb888_to_xrgb8888(dst, dst_pitch, src, fb, clip);
 			return 0;
 		}
-	} else if (dst_format == DRM_FORMAT_BGRX8888) {
-		if (fb_format == DRM_FORMAT_XRGB8888) {
-			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
+	} else if (fb_format == DRM_FORMAT_RGB565) {
+		if (dst_format == DRM_FORMAT_XRGB8888) {
+			drm_fb_rgb565_to_xrgb8888(dst, dst_pitch, src, fb, clip);
 			return 0;
 		}
 	}
-- 
2.39.0

[PATCH v3 07/13] drm/format-helper: Add conversion from XRGB8888 to ARGB8888

[Thomas Zimmermann]

Add dedicated helper to convert from XRGB8888 to ARGB8888. Sets
all alpha bits to make pixels fully opaque.

v3:
	* use __le32 for destination buffer (Jose, kernel test robot)
v2:
	* use cpubuf_to_le32()
	* type fixes

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c           | 53 +++++++++++++++-
 .../gpu/drm/tests/drm_format_helper_test.c    | 63 +++++++++++++++++++
 include/drm/drm_format_helper.h               |  3 +
 3 files changed, 117 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 36d2ca9d0f01..718e29341773 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -446,6 +446,54 @@ void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pi
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb888);
 
+static void drm_fb_xrgb8888_to_argb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		pix |= GENMASK(31, 24); /* fill alpha bits */
+		dbuf32[x] = cpu_to_le32(pix);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_argb8888 - Convert XRGB8888 to ARGB8888 clip buffer
+ * @dst: Array of ARGB8888 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory and converts the
+ * color format during the process. The parameters @dst, @dst_pitch and @src refer
+ * to arrays. Each array must have at least as many entries as there are planes in
+ * @fb's format. Each entry stores the value for the format's respective color plane
+ * at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for ARGB8888 devices that don't support XRGB8888
+ * natively. It sets an opaque alpha channel as part of the conversion.
+ */
+void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		4,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_argb8888_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb8888);
+
 static void drm_fb_rgb565_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
@@ -646,8 +694,6 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 	/* treat alpha channel like filler bits */
 	if (fb_format == DRM_FORMAT_ARGB8888)
 		fb_format = DRM_FORMAT_XRGB8888;
-	if (dst_format == DRM_FORMAT_ARGB8888)
-		dst_format = DRM_FORMAT_XRGB8888;
 	if (fb_format == DRM_FORMAT_ARGB2101010)
 		fb_format = DRM_FORMAT_XRGB2101010;
 	if (dst_format == DRM_FORMAT_ARGB2101010)
@@ -669,6 +715,9 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		} else if (dst_format == DRM_FORMAT_RGB888) {
 			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
 			return 0;
+		} else if (dst_format == DRM_FORMAT_ARGB8888) {
+			drm_fb_xrgb8888_to_argb8888(dst, dst_pitch, src, fb, clip);
+			return 0;
 		} else if (dst_format == DRM_FORMAT_XRGB2101010) {
 			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
 			return 0;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index c2411ec808a1..bc969413a412 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -37,6 +37,11 @@ struct convert_to_rgb888_result {
 	const u8 expected[TEST_BUF_SIZE];
 };
 
+struct convert_to_argb8888_result {
+	unsigned int dst_pitch;
+	const u32 expected[TEST_BUF_SIZE];
+};
+
 struct convert_to_xrgb2101010_result {
 	unsigned int dst_pitch;
 	const u32 expected[TEST_BUF_SIZE];
@@ -51,6 +56,7 @@ struct convert_xrgb8888_case {
 	struct convert_to_rgb332_result rgb332_result;
 	struct convert_to_rgb565_result rgb565_result;
 	struct convert_to_rgb888_result rgb888_result;
+	struct convert_to_argb8888_result argb8888_result;
 	struct convert_to_xrgb2101010_result xrgb2101010_result;
 };
 
@@ -77,6 +83,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = 0,
 			.expected = { 0x00, 0x00, 0xFF },
 		},
+		.argb8888_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFFFF0000 },
+		},
 		.xrgb2101010_result = {
 			.dst_pitch = 0,
 			.expected = { 0x3FF00000 },
@@ -107,6 +117,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = 0,
 			.expected = { 0x00, 0x00, 0xFF },
 		},
+		.argb8888_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFFFF0000 },
+		},
 		.xrgb2101010_result = {
 			.dst_pitch = 0,
 			.expected = { 0x3FF00000 },
@@ -169,6 +183,15 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x00, 0xFF, 0xFF, 0xFF, 0xFF, 0x00,
 			},
 		},
+		.argb8888_result = {
+			.dst_pitch = 0,
+			.expected = {
+				0xFFFFFFFF, 0xFF000000,
+				0xFFFF0000, 0xFF00FF00,
+				0xFF0000FF, 0xFFFF00FF,
+				0xFFFFFF00, 0xFF00FFFF,
+			},
+		},
 		.xrgb2101010_result = {
 			.dst_pitch = 0,
 			.expected = {
@@ -229,6 +252,14 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x00, 0x00, 0x00, 0x00, 0x00, 0x00,
 			},
 		},
+		.argb8888_result = {
+			.dst_pitch = 20,
+			.expected = {
+				0xFF0E449C, 0xFF114D05, 0xFFA80303, 0x00000000, 0x00000000,
+				0xFF6C7073, 0xFF0E449C, 0xFF114D05, 0x00000000, 0x00000000,
+				0xFFA80303, 0xFF6C7073, 0xFF0E449C, 0x00000000, 0x00000000,
+			},
+		},
 		.xrgb2101010_result = {
 			.dst_pitch = 20,
 			.expected = {
@@ -448,6 +479,37 @@ static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
 }
 
+static void drm_test_fb_xrgb8888_to_argb8888(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_argb8888_result *result = &params->argb8888_result;
+	size_t dst_size;
+	u32 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_ARGB8888,
+				       result->dst_pitch, &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	drm_fb_xrgb8888_to_argb8888(&dst, &result->dst_pitch, &src, &fb, &params->clip);
+	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
+	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
+}
+
 static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 {
 	const struct convert_xrgb8888_case *params = test->param_value;
@@ -484,6 +546,7 @@ static struct kunit_case drm_format_helper_test_cases[] = {
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb332, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb565, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
 	{}
 };
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index eb5c98cf82b8..3ce8129dfe43 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -33,6 +33,9 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip);
 void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
 				    const struct drm_rect *clip);
-- 
2.39.0

[PATCH v3 08/13] drm/format-helper: Add conversion from XRGB8888 to ARGB2101010

[Thomas Zimmermann]

Add dedicated helper to convert from XRGB8888 to ARGB2101010. Sets
all alpha bits to make pixels fully opaque.

v2:
	* set correct format in struct drm_framebuffer (Javier)
	* use cpubuf_to_le32()
	* type fixes

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c           | 58 ++++++++++++++++-
 .../gpu/drm/tests/drm_format_helper_test.c    | 63 +++++++++++++++++++
 include/drm/drm_format_helper.h               |  3 +
 3 files changed, 122 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 718e29341773..33780c9183bb 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -605,6 +605,59 @@ void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *d
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb2101010);
 
+static void drm_fb_xrgb8888_to_argb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le32 *dbuf32 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u32 val32;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		val32 = ((pix & 0x000000ff) << 2) |
+			((pix & 0x0000ff00) << 4) |
+			((pix & 0x00ff0000) << 6);
+		pix = GENMASK(31, 30) | /* set alpha bits */
+		      val32 | ((val32 >> 8) & 0x00300c03);
+		*dbuf32++ = cpu_to_le32(pix);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_argb2101010 - Convert XRGB8888 to ARGB2101010 clip buffer
+ * @dst: Array of ARGB2101010 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffers
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory and converts
+ * the color format during the process. The parameters @dst, @dst_pitch and
+ * @src refer to arrays. Each array must have at least as many entries as
+ * there are planes in @fb's format. Each entry stores the value for the
+ * format's respective color plane at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for ARGB2101010 devices that don't support XRGB8888
+ * natively.
+ */
+void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
+				    const struct iosys_map *src, const struct drm_framebuffer *fb,
+				    const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		4,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_argb2101010_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb2101010);
+
 static void drm_fb_xrgb8888_to_gray8_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
@@ -696,8 +749,6 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		fb_format = DRM_FORMAT_XRGB8888;
 	if (fb_format == DRM_FORMAT_ARGB2101010)
 		fb_format = DRM_FORMAT_XRGB2101010;
-	if (dst_format == DRM_FORMAT_ARGB2101010)
-		dst_format = DRM_FORMAT_XRGB2101010;
 
 	if (fb_format == dst_format) {
 		drm_fb_memcpy(dst, dst_pitch, src, fb, clip);
@@ -721,6 +772,9 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		} else if (dst_format == DRM_FORMAT_XRGB2101010) {
 			drm_fb_xrgb8888_to_xrgb2101010(dst, dst_pitch, src, fb, clip);
 			return 0;
+		} else if (dst_format == DRM_FORMAT_ARGB2101010) {
+			drm_fb_xrgb8888_to_argb2101010(dst, dst_pitch, src, fb, clip);
+			return 0;
 		} else if (dst_format == DRM_FORMAT_BGRX8888) {
 			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
 			return 0;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index bc969413a412..de3aa252e8eb 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -47,6 +47,11 @@ struct convert_to_xrgb2101010_result {
 	const u32 expected[TEST_BUF_SIZE];
 };
 
+struct convert_to_argb2101010_result {
+	unsigned int dst_pitch;
+	const u32 expected[TEST_BUF_SIZE];
+};
+
 struct convert_xrgb8888_case {
 	const char *name;
 	unsigned int pitch;
@@ -58,6 +63,7 @@ struct convert_xrgb8888_case {
 	struct convert_to_rgb888_result rgb888_result;
 	struct convert_to_argb8888_result argb8888_result;
 	struct convert_to_xrgb2101010_result xrgb2101010_result;
+	struct convert_to_argb2101010_result argb2101010_result;
 };
 
 static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
@@ -91,6 +97,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = 0,
 			.expected = { 0x3FF00000 },
 		},
+		.argb2101010_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFFF00000 },
+		},
 	},
 	{
 		.name = "single_pixel_clip_rectangle",
@@ -125,6 +135,10 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.dst_pitch = 0,
 			.expected = { 0x3FF00000 },
 		},
+		.argb2101010_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFFF00000 },
+		},
 	},
 	{
 		/* Well known colors: White, black, red, green, blue, magenta,
@@ -201,6 +215,15 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x3FFFFC00, 0x000FFFFF,
 			},
 		},
+		.argb2101010_result = {
+			.dst_pitch = 0,
+			.expected = {
+				0xFFFFFFFF, 0xC0000000,
+				0xFFF00000, 0xC00FFC00,
+				0xC00003FF, 0xFFF003FF,
+				0xFFFFFC00, 0xC00FFFFF,
+			},
+		},
 	},
 	{
 		/* Randomly picked colors. Full buffer within the clip area. */
@@ -268,6 +291,14 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x2A20300C, 0x1B1705CD, 0x03844672, 0x00000000, 0x00000000,
 			},
 		},
+		.argb2101010_result = {
+			.dst_pitch = 20,
+			.expected = {
+				0xC3844672, 0xC444D414, 0xEA20300C, 0x00000000, 0x00000000,
+				0xDB1705CD, 0xC3844672, 0xC444D414, 0x00000000, 0x00000000,
+				0xEA20300C, 0xDB1705CD, 0xC3844672, 0x00000000, 0x00000000,
+			},
+		},
 	},
 };
 
@@ -541,6 +572,37 @@ static void drm_test_fb_xrgb8888_to_xrgb2101010(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
 }
 
+static void drm_test_fb_xrgb8888_to_argb2101010(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_argb2101010_result *result = &params->argb2101010_result;
+	size_t dst_size;
+	u32 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_ARGB2101010,
+				       result->dst_pitch, &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	drm_fb_xrgb8888_to_argb2101010(&dst, &result->dst_pitch, &src, &fb, &params->clip);
+	buf = le32buf_to_cpu(test, (__force const __le32 *)buf, dst_size / sizeof(u32));
+	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
+}
+
 static struct kunit_case drm_format_helper_test_cases[] = {
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_gray8, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb332, convert_xrgb8888_gen_params),
@@ -548,6 +610,7 @@ static struct kunit_case drm_format_helper_test_cases[] = {
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb2101010, convert_xrgb8888_gen_params),
 	{}
 };
 
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 3ce8129dfe43..10b2d19cdb66 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -39,6 +39,9 @@ void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_
 void drm_fb_xrgb8888_to_xrgb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
 				    const struct iosys_map *src, const struct drm_framebuffer *fb,
 				    const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_argb2101010(struct iosys_map *dst, const unsigned int *dst_pitch,
+				    const struct iosys_map *src, const struct drm_framebuffer *fb,
+				    const struct drm_rect *clip);
 void drm_fb_xrgb8888_to_gray8(struct iosys_map *dst, const unsigned int *dst_pitch,
 			      const struct iosys_map *src, const struct drm_framebuffer *fb,
 			      const struct drm_rect *clip);
-- 
2.39.0

[PATCH v3 09/13] drm/format-helper: Add conversion from XRGB8888 to 15-bit RGB555 formats

[Thomas Zimmermann]

Add conversion from XRGB8888 to XRGB1555, ARGB1555 and RGBA5551, which
are the formats currently supported by the simplefb infrastructure. The
new helpers allow the output of XRGB8888 framebuffers to firmware
scanout buffers in one of the 15-bit formats.

v3:
	* use __le* for destination buffers (Jose, kernel test robot)
v2:
	* test 15-bit results with local endianness (Jose)

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
Reviewed-by: José Expósito <jose.exposito89@gmail.com>
---
 drivers/gpu/drm/drm_format_helper.c           | 164 +++++++++++++++
 .../gpu/drm/tests/drm_format_helper_test.c    | 189 ++++++++++++++++++
 include/drm/drm_format_helper.h               |   9 +
 3 files changed, 362 insertions(+)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 33780c9183bb..d631627e6c39 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -396,6 +396,161 @@ void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pi
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgb565);
 
+static void drm_fb_xrgb8888_to_xrgb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le16 *dbuf16 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u16 val16;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		val16 = ((pix & 0x00f80000) >> 9) |
+			((pix & 0x0000f800) >> 6) |
+			((pix & 0x000000f8) >> 3);
+		dbuf16[x] = cpu_to_le16(val16);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_xrgb1555 - Convert XRGB8888 to XRGB1555 clip buffer
+ * @dst: Array of XRGB1555 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory and converts
+ * the color format during the process. The parameters @dst, @dst_pitch and
+ * @src refer to arrays. Each array must have at least as many entries as
+ * there are planes in @fb's format. Each entry stores the value for the
+ * format's respective color plane at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for XRGB1555 devices that don't support
+ * XRGB8888 natively.
+ */
+void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		2,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_xrgb1555_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_xrgb1555);
+
+static void drm_fb_xrgb8888_to_argb1555_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le16 *dbuf16 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u16 val16;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		val16 = BIT(15) | /* set alpha bit */
+			((pix & 0x00f80000) >> 9) |
+			((pix & 0x0000f800) >> 6) |
+			((pix & 0x000000f8) >> 3);
+		dbuf16[x] = cpu_to_le16(val16);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_argb1555 - Convert XRGB8888 to ARGB1555 clip buffer
+ * @dst: Array of ARGB1555 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory and converts
+ * the color format during the process. The parameters @dst, @dst_pitch and
+ * @src refer to arrays. Each array must have at least as many entries as
+ * there are planes in @fb's format. Each entry stores the value for the
+ * format's respective color plane at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for ARGB1555 devices that don't support
+ * XRGB8888 natively. It sets an opaque alpha channel as part of the conversion.
+ */
+void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		2,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_argb1555_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb1555);
+
+static void drm_fb_xrgb8888_to_rgba5551_line(void *dbuf, const void *sbuf, unsigned int pixels)
+{
+	__le16 *dbuf16 = dbuf;
+	const __le32 *sbuf32 = sbuf;
+	unsigned int x;
+	u16 val16;
+	u32 pix;
+
+	for (x = 0; x < pixels; x++) {
+		pix = le32_to_cpu(sbuf32[x]);
+		val16 = ((pix & 0x00f80000) >> 8) |
+			((pix & 0x0000f800) >> 5) |
+			((pix & 0x000000f8) >> 2) |
+			BIT(0); /* set alpha bit */
+		dbuf16[x] = cpu_to_le16(val16);
+	}
+}
+
+/**
+ * drm_fb_xrgb8888_to_rgba5551 - Convert XRGB8888 to RGBA5551 clip buffer
+ * @dst: Array of RGBA5551 destination buffers
+ * @dst_pitch: Array of numbers of bytes between the start of two consecutive scanlines
+ *             within @dst; can be NULL if scanlines are stored next to each other.
+ * @src: Array of XRGB8888 source buffer
+ * @fb: DRM framebuffer
+ * @clip: Clip rectangle area to copy
+ *
+ * This function copies parts of a framebuffer to display memory and converts
+ * the color format during the process. The parameters @dst, @dst_pitch and
+ * @src refer to arrays. Each array must have at least as many entries as
+ * there are planes in @fb's format. Each entry stores the value for the
+ * format's respective color plane at the same index.
+ *
+ * This function does not apply clipping on @dst (i.e. the destination is at the
+ * top-left corner).
+ *
+ * Drivers can use this function for RGBA5551 devices that don't support
+ * XRGB8888 natively. It sets an opaque alpha channel as part of the conversion.
+ */
+void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip)
+{
+	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
+		2,
+	};
+
+	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
+		    drm_fb_xrgb8888_to_rgba5551_line);
+}
+EXPORT_SYMBOL(drm_fb_xrgb8888_to_rgba5551);
+
 static void drm_fb_xrgb8888_to_rgb888_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	u8 *dbuf8 = dbuf;
@@ -763,6 +918,15 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 		if (dst_format == DRM_FORMAT_RGB565) {
 			drm_fb_xrgb8888_to_rgb565(dst, dst_pitch, src, fb, clip, false);
 			return 0;
+		} else if (dst_format == DRM_FORMAT_XRGB1555) {
+			drm_fb_xrgb8888_to_xrgb1555(dst, dst_pitch, src, fb, clip);
+			return 0;
+		} else if (dst_format == DRM_FORMAT_ARGB1555) {
+			drm_fb_xrgb8888_to_argb1555(dst, dst_pitch, src, fb, clip);
+			return 0;
+		} else if (dst_format == DRM_FORMAT_RGBA5551) {
+			drm_fb_xrgb8888_to_rgba5551(dst, dst_pitch, src, fb, clip);
+			return 0;
 		} else if (dst_format == DRM_FORMAT_RGB888) {
 			drm_fb_xrgb8888_to_rgb888(dst, dst_pitch, src, fb, clip);
 			return 0;
diff --git a/drivers/gpu/drm/tests/drm_format_helper_test.c b/drivers/gpu/drm/tests/drm_format_helper_test.c
index de3aa252e8eb..f71dc0fe08a1 100644
--- a/drivers/gpu/drm/tests/drm_format_helper_test.c
+++ b/drivers/gpu/drm/tests/drm_format_helper_test.c
@@ -32,6 +32,21 @@ struct convert_to_rgb565_result {
 	const u16 expected_swab[TEST_BUF_SIZE];
 };
 
+struct convert_to_xrgb1555_result {
+	unsigned int dst_pitch;
+	const u16 expected[TEST_BUF_SIZE];
+};
+
+struct convert_to_argb1555_result {
+	unsigned int dst_pitch;
+	const u16 expected[TEST_BUF_SIZE];
+};
+
+struct convert_to_rgba5551_result {
+	unsigned int dst_pitch;
+	const u16 expected[TEST_BUF_SIZE];
+};
+
 struct convert_to_rgb888_result {
 	unsigned int dst_pitch;
 	const u8 expected[TEST_BUF_SIZE];
@@ -60,6 +75,9 @@ struct convert_xrgb8888_case {
 	struct convert_to_gray8_result gray8_result;
 	struct convert_to_rgb332_result rgb332_result;
 	struct convert_to_rgb565_result rgb565_result;
+	struct convert_to_xrgb1555_result xrgb1555_result;
+	struct convert_to_argb1555_result argb1555_result;
+	struct convert_to_rgba5551_result rgba5551_result;
 	struct convert_to_rgb888_result rgb888_result;
 	struct convert_to_argb8888_result argb8888_result;
 	struct convert_to_xrgb2101010_result xrgb2101010_result;
@@ -85,6 +103,18 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.expected = { 0xF800 },
 			.expected_swab = { 0x00F8 },
 		},
+		.xrgb1555_result = {
+			.dst_pitch = 0,
+			.expected = { 0x7C00 },
+		},
+		.argb1555_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFC00 },
+		},
+		.rgba5551_result = {
+			.dst_pitch = 0,
+			.expected = { 0xF801 },
+		},
 		.rgb888_result = {
 			.dst_pitch = 0,
 			.expected = { 0x00, 0x00, 0xFF },
@@ -123,6 +153,18 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 			.expected = { 0xF800 },
 			.expected_swab = { 0x00F8 },
 		},
+		.xrgb1555_result = {
+			.dst_pitch = 0,
+			.expected = { 0x7C00 },
+		},
+		.argb1555_result = {
+			.dst_pitch = 0,
+			.expected = { 0xFC00 },
+		},
+		.rgba5551_result = {
+			.dst_pitch = 0,
+			.expected = { 0xF801 },
+		},
 		.rgb888_result = {
 			.dst_pitch = 0,
 			.expected = { 0x00, 0x00, 0xFF },
@@ -188,6 +230,33 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0xE0FF, 0xFF07,
 			},
 		},
+		.xrgb1555_result = {
+			.dst_pitch = 0,
+			.expected = {
+				0x7FFF, 0x0000,
+				0x7C00, 0x03E0,
+				0x001F, 0x7C1F,
+				0x7FE0, 0x03FF,
+			},
+		},
+		.argb1555_result = {
+			.dst_pitch = 0,
+			.expected = {
+				0xFFFF, 0x8000,
+				0xFC00, 0x83E0,
+				0x801F, 0xFC1F,
+				0xFFE0, 0x83FF,
+			},
+		},
+		.rgba5551_result = {
+			.dst_pitch = 0,
+			.expected = {
+				0xFFFF, 0x0001,
+				0xF801, 0x07C1,
+				0x003F, 0xF83F,
+				0xFFC1, 0x07FF,
+			},
+		},
 		.rgb888_result = {
 			.dst_pitch = 0,
 			.expected = {
@@ -264,6 +333,30 @@ static struct convert_xrgb8888_case convert_xrgb8888_cases[] = {
 				0x00A8, 0x8E6B, 0x330A, 0x0000, 0x0000,
 			},
 		},
+		.xrgb1555_result = {
+			.dst_pitch = 10,
+			.expected = {
+				0x0513, 0x0920, 0x5400, 0x0000, 0x0000,
+				0x35CE, 0x0513, 0x0920, 0x0000, 0x0000,
+				0x5400, 0x35CE, 0x0513, 0x0000, 0x0000,
+			},
+		},
+		.argb1555_result = {
+			.dst_pitch = 10,
+			.expected = {
+				0x8513, 0x8920, 0xD400, 0x0000, 0x0000,
+				0xB5CE, 0x8513, 0x8920, 0x0000, 0x0000,
+				0xD400, 0xB5CE, 0x8513, 0x0000, 0x0000,
+			},
+		},
+		.rgba5551_result = {
+			.dst_pitch = 10,
+			.expected = {
+				0x0A27, 0x1241, 0xA801, 0x0000, 0x0000,
+				0x6B9D, 0x0A27, 0x1241, 0x0000, 0x0000,
+				0xA801, 0x6B9D, 0x0A27, 0x0000, 0x0000,
+			},
+		},
 		.rgb888_result = {
 			.dst_pitch = 15,
 			.expected = {
@@ -476,6 +569,99 @@ static void drm_test_fb_xrgb8888_to_rgb565(struct kunit *test)
 	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected_swab, dst_size), 0);
 }
 
+static void drm_test_fb_xrgb8888_to_xrgb1555(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_xrgb1555_result *result = &params->xrgb1555_result;
+	size_t dst_size;
+	u16 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_XRGB1555, result->dst_pitch,
+				       &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	drm_fb_xrgb8888_to_xrgb1555(&dst, &result->dst_pitch, &src, &fb, &params->clip);
+	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
+	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
+}
+
+static void drm_test_fb_xrgb8888_to_argb1555(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_argb1555_result *result = &params->argb1555_result;
+	size_t dst_size;
+	u16 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_ARGB1555, result->dst_pitch,
+				       &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	drm_fb_xrgb8888_to_argb1555(&dst, &result->dst_pitch, &src, &fb, &params->clip);
+	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
+	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
+}
+
+static void drm_test_fb_xrgb8888_to_rgba5551(struct kunit *test)
+{
+	const struct convert_xrgb8888_case *params = test->param_value;
+	const struct convert_to_rgba5551_result *result = &params->rgba5551_result;
+	size_t dst_size;
+	u16 *buf = NULL;
+	__le32 *xrgb8888 = NULL;
+	struct iosys_map dst, src;
+
+	struct drm_framebuffer fb = {
+		.format = drm_format_info(DRM_FORMAT_XRGB8888),
+		.pitches = { params->pitch, 0, 0 },
+	};
+
+	dst_size = conversion_buf_size(DRM_FORMAT_RGBA5551, result->dst_pitch,
+				       &params->clip);
+	KUNIT_ASSERT_GT(test, dst_size, 0);
+
+	buf = kunit_kzalloc(test, dst_size, GFP_KERNEL);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, buf);
+	iosys_map_set_vaddr(&dst, buf);
+
+	xrgb8888 = cpubuf_to_le32(test, params->xrgb8888, TEST_BUF_SIZE);
+	KUNIT_ASSERT_NOT_ERR_OR_NULL(test, xrgb8888);
+	iosys_map_set_vaddr(&src, xrgb8888);
+
+	drm_fb_xrgb8888_to_rgba5551(&dst, &result->dst_pitch, &src, &fb, &params->clip);
+	buf = le16buf_to_cpu(test, (__force const __le16 *)buf, dst_size / sizeof(__le16));
+	KUNIT_EXPECT_EQ(test, memcmp(buf, result->expected, dst_size), 0);
+}
+
 static void drm_test_fb_xrgb8888_to_rgb888(struct kunit *test)
 {
 	const struct convert_xrgb8888_case *params = test->param_value;
@@ -607,6 +793,9 @@ static struct kunit_case drm_format_helper_test_cases[] = {
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_gray8, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb332, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb565, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb1555, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb1555, convert_xrgb8888_gen_params),
+	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgba5551, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_rgb888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_argb8888, convert_xrgb8888_gen_params),
 	KUNIT_CASE_PARAM(drm_test_fb_xrgb8888_to_xrgb2101010, convert_xrgb8888_gen_params),
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 10b2d19cdb66..2d04f5863722 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -30,6 +30,15 @@ void drm_fb_xrgb8888_to_rgb332(struct iosys_map *dst, const unsigned int *dst_pi
 void drm_fb_xrgb8888_to_rgb565(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip, bool swab);
+void drm_fb_xrgb8888_to_xrgb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_argb1555(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip);
+void drm_fb_xrgb8888_to_rgba5551(struct iosys_map *dst, const unsigned int *dst_pitch,
+				 const struct iosys_map *src, const struct drm_framebuffer *fb,
+				 const struct drm_rect *clip);
 void drm_fb_xrgb8888_to_rgb888(struct iosys_map *dst, const unsigned int *dst_pitch,
 			       const struct iosys_map *src, const struct drm_framebuffer *fb,
 			       const struct drm_rect *clip);
-- 
2.39.0

[PATCH v3 10/13] drm/fh-helper: Split fbdev single-probe helper

[Thomas Zimmermann]

Split the single-probe helper's implementation into multiple
functions and get locking and overallocation out of the way of
the surface setup. Simplifies later changes to the setup code.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 116 ++++++++++++++++++++------------
 1 file changed, 73 insertions(+), 43 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index b3a731b9170a..af1495394d38 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1726,36 +1726,30 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
-/*
- * Allocates the backing storage and sets up the fbdev info structure through
- * the ->fb_probe callback.
- */
-static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
-					 int preferred_bpp)
+
+static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int preferred_bpp,
+				      struct drm_fb_helper_surface_size *sizes)
 {
 	struct drm_client_dev *client = &fb_helper->client;
 	struct drm_device *dev = fb_helper->dev;
-	struct drm_mode_config *config = &dev->mode_config;
-	int ret = 0;
 	int crtc_count = 0;
 	struct drm_connector_list_iter conn_iter;
-	struct drm_fb_helper_surface_size sizes;
 	struct drm_connector *connector;
 	struct drm_mode_set *mode_set;
 	int best_depth = 0;
 
-	memset(&sizes, 0, sizeof(struct drm_fb_helper_surface_size));
-	sizes.surface_depth = 24;
-	sizes.surface_bpp = 32;
-	sizes.fb_width = (u32)-1;
-	sizes.fb_height = (u32)-1;
+	memset(sizes, 0, sizeof(struct drm_fb_helper_surface_size));
+	sizes->surface_depth = 24;
+	sizes->surface_bpp = 32;
+	sizes->fb_width = (u32)-1;
+	sizes->fb_height = (u32)-1;
 
 	/*
 	 * If driver picks 8 or 16 by default use that for both depth/bpp
 	 * to begin with
 	 */
-	if (preferred_bpp != sizes.surface_bpp)
-		sizes.surface_depth = sizes.surface_bpp = preferred_bpp;
+	if (preferred_bpp != sizes->surface_bpp)
+		sizes->surface_depth = sizes->surface_bpp = preferred_bpp;
 
 	drm_connector_list_iter_begin(fb_helper->dev, &conn_iter);
 	drm_client_for_each_connector_iter(connector, &conn_iter) {
@@ -1766,21 +1760,21 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		if (cmdline_mode->bpp_specified) {
 			switch (cmdline_mode->bpp) {
 			case 8:
-				sizes.surface_depth = sizes.surface_bpp = 8;
+				sizes->surface_depth = sizes->surface_bpp = 8;
 				break;
 			case 15:
-				sizes.surface_depth = 15;
-				sizes.surface_bpp = 16;
+				sizes->surface_depth = 15;
+				sizes->surface_bpp = 16;
 				break;
 			case 16:
-				sizes.surface_depth = sizes.surface_bpp = 16;
+				sizes->surface_depth = sizes->surface_bpp = 16;
 				break;
 			case 24:
-				sizes.surface_depth = sizes.surface_bpp = 24;
+				sizes->surface_depth = sizes->surface_bpp = 24;
 				break;
 			case 32:
-				sizes.surface_depth = 24;
-				sizes.surface_bpp = 32;
+				sizes->surface_depth = 24;
+				sizes->surface_bpp = 32;
 				break;
 			}
 			break;
@@ -1793,7 +1787,6 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 	 * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth
 	 * 16) we need to scale down the depth of the sizes we request.
 	 */
-	mutex_lock(&client->modeset_mutex);
 	drm_client_for_each_modeset(mode_set, client) {
 		struct drm_crtc *crtc = mode_set->crtc;
 		struct drm_plane *plane = crtc->primary;
@@ -1817,13 +1810,13 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 				continue;
 
 			/* We found a perfect fit, great */
-			if (fmt->depth == sizes.surface_depth) {
+			if (fmt->depth == sizes->surface_depth) {
 				best_depth = fmt->depth;
 				break;
 			}
 
 			/* Skip depths above what we're looking for */
-			if (fmt->depth > sizes.surface_depth)
+			if (fmt->depth > sizes->surface_depth)
 				continue;
 
 			/* Best depth found so far */
@@ -1831,10 +1824,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 				best_depth = fmt->depth;
 		}
 	}
-	if (sizes.surface_depth != best_depth && best_depth) {
+	if (sizes->surface_depth != best_depth && best_depth) {
 		drm_info(dev, "requested bpp %d, scaled depth down to %d",
-			 sizes.surface_bpp, best_depth);
-		sizes.surface_depth = best_depth;
+			 sizes->surface_bpp, best_depth);
+		sizes->surface_depth = best_depth;
 	}
 
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
@@ -1858,8 +1851,10 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		x = mode_set->x;
 		y = mode_set->y;
 
-		sizes.surface_width  = max_t(u32, desired_mode->hdisplay + x, sizes.surface_width);
-		sizes.surface_height = max_t(u32, desired_mode->vdisplay + y, sizes.surface_height);
+		sizes->surface_width  =
+			max_t(u32, desired_mode->hdisplay + x, sizes->surface_width);
+		sizes->surface_height =
+			max_t(u32, desired_mode->vdisplay + y, sizes->surface_height);
 
 		for (j = 0; j < mode_set->num_connectors; j++) {
 			struct drm_connector *connector = mode_set->connectors[j];
@@ -1875,28 +1870,63 @@ static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
 		}
 
 		if (lasth)
-			sizes.fb_width  = min_t(u32, desired_mode->hdisplay + x, sizes.fb_width);
+			sizes->fb_width  = min_t(u32, desired_mode->hdisplay + x, sizes->fb_width);
 		if (lastv)
-			sizes.fb_height = min_t(u32, desired_mode->vdisplay + y, sizes.fb_height);
+			sizes->fb_height = min_t(u32, desired_mode->vdisplay + y, sizes->fb_height);
 	}
-	mutex_unlock(&client->modeset_mutex);
 
-	if (crtc_count == 0 || sizes.fb_width == -1 || sizes.fb_height == -1) {
+	if (crtc_count == 0 || sizes->fb_width == -1 || sizes->fb_height == -1) {
 		drm_info(dev, "Cannot find any crtc or sizes\n");
-
-		/* First time: disable all crtc's.. */
-		if (!fb_helper->deferred_setup)
-			drm_client_modeset_commit(client);
 		return -EAGAIN;
 	}
 
+	return 0;
+}
+
+static int drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int preferred_bpp,
+				    struct drm_fb_helper_surface_size *sizes)
+{
+	struct drm_client_dev *client = &fb_helper->client;
+	struct drm_device *dev = fb_helper->dev;
+	struct drm_mode_config *config = &dev->mode_config;
+	int ret;
+
+	mutex_lock(&client->modeset_mutex);
+	ret = __drm_fb_helper_find_sizes(fb_helper, preferred_bpp, sizes);
+	mutex_unlock(&client->modeset_mutex);
+
+	if (ret)
+		return ret;
+
 	/* Handle our overallocation */
-	sizes.surface_height *= drm_fbdev_overalloc;
-	sizes.surface_height /= 100;
-	if (sizes.surface_height > config->max_height) {
+	sizes->surface_height *= drm_fbdev_overalloc;
+	sizes->surface_height /= 100;
+	if (sizes->surface_height > config->max_height) {
 		drm_dbg_kms(dev, "Fbdev over-allocation too large; clamping height to %d\n",
 			    config->max_height);
-		sizes.surface_height = config->max_height;
+		sizes->surface_height = config->max_height;
+	}
+
+	return 0;
+}
+
+/*
+ * Allocates the backing storage and sets up the fbdev info structure through
+ * the ->fb_probe callback.
+ */
+static int drm_fb_helper_single_fb_probe(struct drm_fb_helper *fb_helper,
+					 int preferred_bpp)
+{
+	struct drm_client_dev *client = &fb_helper->client;
+	struct drm_fb_helper_surface_size sizes;
+	int ret;
+
+	ret = drm_fb_helper_find_sizes(fb_helper, preferred_bpp, &sizes);
+	if (ret) {
+		/* First time: disable all crtc's.. */
+		if (!fb_helper->deferred_setup)
+			drm_client_modeset_commit(client);
+		return ret;
 	}
 
 #if IS_ENABLED(CONFIG_DRM_FBDEV_LEAK_PHYS_SMEM)
-- 
2.39.0

[PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

Fix the color-format selection of the single-probe helper. Go
through all user-specified values and test each for compatibility
with the driver. If none is supported, use the driver-provided
default. This guarantees that the console is always available in
any color format at least.

Until now, the format selection of the single-probe helper tried
to either use a user-specified format or a 32-bit default format.
If the user-specified format was not supported by the driver, the
selection failed and the display remained blank.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_fb_helper.c | 172 +++++++++++++++++---------------
 1 file changed, 94 insertions(+), 78 deletions(-)

diff --git a/drivers/gpu/drm/drm_fb_helper.c b/drivers/gpu/drm/drm_fb_helper.c
index af1495394d38..1369ca4ae39b 100644
--- a/drivers/gpu/drm/drm_fb_helper.c
+++ b/drivers/gpu/drm/drm_fb_helper.c
@@ -1726,6 +1726,70 @@ int drm_fb_helper_pan_display(struct fb_var_screeninfo *var,
 }
 EXPORT_SYMBOL(drm_fb_helper_pan_display);
 
+static uint32_t drm_fb_helper_find_format(struct drm_fb_helper *fb_helper, const uint32_t *formats,
+					  size_t format_count, uint32_t bpp, uint32_t depth)
+{
+	struct drm_device *dev = fb_helper->dev;
+	uint32_t format;
+	size_t i;
+
+	/*
+	 * Do not consider YUV or other complicated formats
+	 * for framebuffers. This means only legacy formats
+	 * are supported (fmt->depth is a legacy field), but
+	 * the framebuffer emulation can only deal with such
+	 * formats, specifically RGB/BGA formats.
+	 */
+	format = drm_mode_legacy_fb_format(bpp, depth);
+	if (!format)
+		goto err;
+
+	for (i = 0; i < format_count; ++i) {
+		if (formats[i] == format)
+			return format;
+	}
+
+err:
+	/* We found nothing. */
+	drm_warn(dev, "bpp/depth value of %u/%u not supported\n", bpp, depth);
+
+	return DRM_FORMAT_INVALID;
+}
+
+static uint32_t drm_fb_helper_find_cmdline_format(struct drm_fb_helper *fb_helper,
+						  const uint32_t *formats, size_t format_count,
+						  const struct drm_cmdline_mode *cmdline_mode)
+{
+	struct drm_device *dev = fb_helper->dev;
+	uint32_t bpp, depth;
+
+	if (!cmdline_mode->bpp_specified)
+		return DRM_FORMAT_INVALID;
+
+	switch (cmdline_mode->bpp) {
+	case 1:
+	case 2:
+	case 4:
+	case 8:
+	case 16:
+	case 24:
+		bpp = depth = cmdline_mode->bpp;
+		break;
+	case 15:
+		bpp = 16;
+		depth = 15;
+		break;
+	case 32:
+		bpp = 32;
+		depth = 24;
+		break;
+	default:
+		drm_info(dev, "unsupported bpp value of %d\n", cmdline_mode->bpp);
+		return DRM_FORMAT_INVALID;
+	}
+
+	return drm_fb_helper_find_format(fb_helper, formats, format_count, bpp, depth);
+}
 
 static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int preferred_bpp,
 				      struct drm_fb_helper_surface_size *sizes)
@@ -1736,100 +1800,52 @@ static int __drm_fb_helper_find_sizes(struct drm_fb_helper *fb_helper, int prefe
 	struct drm_connector_list_iter conn_iter;
 	struct drm_connector *connector;
 	struct drm_mode_set *mode_set;
-	int best_depth = 0;
+	uint32_t surface_format = DRM_FORMAT_INVALID;
+	const struct drm_format_info *info;
 
-	memset(sizes, 0, sizeof(struct drm_fb_helper_surface_size));
-	sizes->surface_depth = 24;
-	sizes->surface_bpp = 32;
+	memset(sizes, 0, sizeof(*sizes));
 	sizes->fb_width = (u32)-1;
 	sizes->fb_height = (u32)-1;
 
-	/*
-	 * If driver picks 8 or 16 by default use that for both depth/bpp
-	 * to begin with
-	 */
-	if (preferred_bpp != sizes->surface_bpp)
-		sizes->surface_depth = sizes->surface_bpp = preferred_bpp;
-
-	drm_connector_list_iter_begin(fb_helper->dev, &conn_iter);
-	drm_client_for_each_connector_iter(connector, &conn_iter) {
-		struct drm_cmdline_mode *cmdline_mode;
-
-		cmdline_mode = &connector->cmdline_mode;
-
-		if (cmdline_mode->bpp_specified) {
-			switch (cmdline_mode->bpp) {
-			case 8:
-				sizes->surface_depth = sizes->surface_bpp = 8;
-				break;
-			case 15:
-				sizes->surface_depth = 15;
-				sizes->surface_bpp = 16;
-				break;
-			case 16:
-				sizes->surface_depth = sizes->surface_bpp = 16;
-				break;
-			case 24:
-				sizes->surface_depth = sizes->surface_bpp = 24;
-				break;
-			case 32:
-				sizes->surface_depth = 24;
-				sizes->surface_bpp = 32;
-				break;
-			}
-			break;
-		}
-	}
-	drm_connector_list_iter_end(&conn_iter);
-
-	/*
-	 * If we run into a situation where, for example, the primary plane
-	 * supports RGBA5551 (16 bpp, depth 15) but not RGB565 (16 bpp, depth
-	 * 16) we need to scale down the depth of the sizes we request.
-	 */
 	drm_client_for_each_modeset(mode_set, client) {
 		struct drm_crtc *crtc = mode_set->crtc;
 		struct drm_plane *plane = crtc->primary;
-		int j;
 
 		drm_dbg_kms(dev, "test CRTC %u primary plane\n", drm_crtc_index(crtc));
 
-		for (j = 0; j < plane->format_count; j++) {
-			const struct drm_format_info *fmt;
-
-			fmt = drm_format_info(plane->format_types[j]);
+		drm_connector_list_iter_begin(fb_helper->dev, &conn_iter);
+		drm_client_for_each_connector_iter(connector, &conn_iter) {
+			struct drm_cmdline_mode *cmdline_mode = &connector->cmdline_mode;
 
-			/*
-			 * Do not consider YUV or other complicated formats
-			 * for framebuffers. This means only legacy formats
-			 * are supported (fmt->depth is a legacy field) but
-			 * the framebuffer emulation can only deal with such
-			 * formats, specifically RGB/BGA formats.
-			 */
-			if (fmt->depth == 0)
-				continue;
-
-			/* We found a perfect fit, great */
-			if (fmt->depth == sizes->surface_depth) {
-				best_depth = fmt->depth;
-				break;
-			}
+			surface_format = drm_fb_helper_find_cmdline_format(fb_helper,
+									   plane->format_types,
+									   plane->format_count,
+									   cmdline_mode);
+			if (surface_format != DRM_FORMAT_INVALID)
+				break; /* found supported format */
+		}
+		drm_connector_list_iter_end(&conn_iter);
 
-			/* Skip depths above what we're looking for */
-			if (fmt->depth > sizes->surface_depth)
-				continue;
+		if (surface_format != DRM_FORMAT_INVALID)
+			break; /* found supported format */
 
-			/* Best depth found so far */
-			if (fmt->depth > best_depth)
-				best_depth = fmt->depth;
-		}
+		/* try preferred bpp/depth */
+		surface_format = drm_fb_helper_find_format(fb_helper, plane->format_types,
+							   plane->format_count, preferred_bpp,
+							   dev->mode_config.preferred_depth);
+		if (surface_format != DRM_FORMAT_INVALID)
+			break; /* found supported format */
 	}
-	if (sizes->surface_depth != best_depth && best_depth) {
-		drm_info(dev, "requested bpp %d, scaled depth down to %d",
-			 sizes->surface_bpp, best_depth);
-		sizes->surface_depth = best_depth;
+
+	if (surface_format == DRM_FORMAT_INVALID) {
+		drm_warn(dev, "No compatible format found\n");
+		return -EAGAIN;
 	}
 
+	info = drm_format_info(surface_format);
+	sizes->surface_bpp = drm_format_info_bpp(info, 0);
+	sizes->surface_depth = info->depth;
+
 	/* first up get a count of crtcs now in use and new min/maxes width/heights */
 	crtc_count = 0;
 	drm_client_for_each_modeset(mode_set, client) {
-- 
2.39.0

[PATCH v3 12/13] drm/format-helper: Simplify drm_fb_build_fourcc_list()

[Thomas Zimmermann]

The DRM helper drm_fb_build_fourcc_list() creates a list of color
formats for primary planes of the generic drivers. Simplify the helper:

 - It used to mix and filter native and emulated formats as provided
   by the driver. Now the only emulated format is XRGB8888, which is
   required as fallback by legacy software. Drop support for emulating
   any other formats.
 - Also convert alpha formats to their non-alpha counterparts. Generic
   drivers don't support primary planes with alpha formats and some
   DTs incorrectly advertise alpha channels for non-alpha hardware. So
   only export non-alpha formats for primary planes.

With the simplified helper, scrap format lists of the affected generic
drivers. All they need is the firmware buffer's native format, from which
the helper creates the list of color formats.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_format_helper.c | 112 ++++++++++++++--------------
 drivers/gpu/drm/tiny/ofdrm.c        |  20 -----
 drivers/gpu/drm/tiny/simpledrm.c    |  21 ------
 include/drm/drm_format_helper.h     |   1 -
 4 files changed, 57 insertions(+), 97 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index d631627e6c39..0523f81e2445 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -1070,6 +1070,39 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_mono);
 
+static uint32_t drm_fb_nonalpha_fourcc(uint32_t fourcc)
+{
+	/* only handle formats with depth != 0 and alpha channel */
+	switch (fourcc) {
+	case DRM_FORMAT_ARGB1555:
+		return DRM_FORMAT_XRGB1555;
+	case DRM_FORMAT_ABGR1555:
+		return DRM_FORMAT_XBGR1555;
+	case DRM_FORMAT_RGBA5551:
+		return DRM_FORMAT_RGBX5551;
+	case DRM_FORMAT_BGRA5551:
+		return DRM_FORMAT_BGRX5551;
+	case DRM_FORMAT_ARGB8888:
+		return DRM_FORMAT_XRGB8888;
+	case DRM_FORMAT_ABGR8888:
+		return DRM_FORMAT_XBGR8888;
+	case DRM_FORMAT_RGBA8888:
+		return DRM_FORMAT_RGBX8888;
+	case DRM_FORMAT_BGRA8888:
+		return DRM_FORMAT_BGRX8888;
+	case DRM_FORMAT_ARGB2101010:
+		return DRM_FORMAT_XRGB2101010;
+	case DRM_FORMAT_ABGR2101010:
+		return DRM_FORMAT_XBGR2101010;
+	case DRM_FORMAT_RGBA1010102:
+		return DRM_FORMAT_RGBX1010102;
+	case DRM_FORMAT_BGRA1010102:
+		return DRM_FORMAT_BGRX1010102;
+	}
+
+	return fourcc;
+}
+
 static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t fourcc)
 {
 	const uint32_t *fourccs_end = fourccs + nfourccs;
@@ -1082,73 +1115,48 @@ static bool is_listed_fourcc(const uint32_t *fourccs, size_t nfourccs, uint32_t
 	return false;
 }
 
-static const uint32_t conv_from_xrgb8888[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_XRGB2101010,
-	DRM_FORMAT_ARGB2101010,
-	DRM_FORMAT_RGB565,
-	DRM_FORMAT_RGB888,
-};
-
-static const uint32_t conv_from_rgb565_888[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-};
-
-static bool is_conversion_supported(uint32_t from, uint32_t to)
-{
-	switch (from) {
-	case DRM_FORMAT_XRGB8888:
-	case DRM_FORMAT_ARGB8888:
-		return is_listed_fourcc(conv_from_xrgb8888, ARRAY_SIZE(conv_from_xrgb8888), to);
-	case DRM_FORMAT_RGB565:
-	case DRM_FORMAT_RGB888:
-		return is_listed_fourcc(conv_from_rgb565_888, ARRAY_SIZE(conv_from_rgb565_888), to);
-	case DRM_FORMAT_XRGB2101010:
-		return to == DRM_FORMAT_ARGB2101010;
-	case DRM_FORMAT_ARGB2101010:
-		return to == DRM_FORMAT_XRGB2101010;
-	default:
-		return false;
-	}
-}
-
 /**
  * drm_fb_build_fourcc_list - Filters a list of supported color formats against
  *                            the device's native formats
  * @dev: DRM device
  * @native_fourccs: 4CC codes of natively supported color formats
  * @native_nfourccs: The number of entries in @native_fourccs
- * @driver_fourccs: 4CC codes of all driver-supported color formats
- * @driver_nfourccs: The number of entries in @driver_fourccs
  * @fourccs_out: Returns 4CC codes of supported color formats
  * @nfourccs_out: The number of available entries in @fourccs_out
  *
  * This function create a list of supported color format from natively
- * supported formats and the emulated formats.
+ * supported formats and additional emulated formats.
  * At a minimum, most userspace programs expect at least support for
  * XRGB8888 on the primary plane. Devices that have to emulate the
  * format, and possibly others, can use drm_fb_build_fourcc_list() to
  * create a list of supported color formats. The returned list can
  * be handed over to drm_universal_plane_init() et al. Native formats
- * will go before emulated formats. Other heuristics might be applied
+ * will go before emulated formats. Native formats with alpha channel
+ * will be replaced by such without, as primary planes usually don't
+ * support alpha. Other heuristics might be applied
  * to optimize the order. Formats near the beginning of the list are
- * usually preferred over formats near the end of the list. Formats
- * without conversion helpers will be skipped. New drivers should only
- * pass in XRGB8888 and avoid exposing additional emulated formats.
+ * usually preferred over formats near the end of the list.
  *
  * Returns:
  * The number of color-formats 4CC codes returned in @fourccs_out.
  */
 size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 				const u32 *native_fourccs, size_t native_nfourccs,
-				const u32 *driver_fourccs, size_t driver_nfourccs,
 				u32 *fourccs_out, size_t nfourccs_out)
 {
+	/*
+	 * XRGB8888 is the default fallback format for most of userspace
+	 * and it's currently the only format that should be emulated for
+	 * the primary plane. Only if there's ever another default fallback,
+	 * it should be added here.
+	 */
+	static const uint32_t extra_fourccs[] = {
+		DRM_FORMAT_XRGB8888,
+	};
+	static const size_t extra_nfourccs = ARRAY_SIZE(extra_fourccs);
+
 	u32 *fourccs = fourccs_out;
 	const u32 *fourccs_end = fourccs_out + nfourccs_out;
-	uint32_t native_format = 0;
 	size_t i;
 
 	/*
@@ -1156,7 +1164,12 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 	 */
 
 	for (i = 0; i < native_nfourccs; ++i) {
-		u32 fourcc = native_fourccs[i];
+		/*
+		 * Several DTs, boot loaders and firmware report native
+		 * alpha formats that are non-alpha formats instead. So
+		 * replace alpha formats by non-alpha formats.
+		 */
+		u32 fourcc = drm_fb_nonalpha_fourcc(native_fourccs[i]);
 
 		if (is_listed_fourcc(fourccs_out, fourccs - fourccs_out, fourcc)) {
 			continue; /* skip duplicate entries */
@@ -1167,14 +1180,6 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 
 		drm_dbg_kms(dev, "adding native format %p4cc\n", &fourcc);
 
-		/*
-		 * There should only be one native format with the current API.
-		 * This API needs to be refactored to correctly support arbitrary
-		 * sets of native formats, since it needs to report which native
-		 * format to use for each emulated format.
-		 */
-		if (!native_format)
-			native_format = fourcc;
 		*fourccs = fourcc;
 		++fourccs;
 	}
@@ -1183,17 +1188,14 @@ size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 	 * The extra formats, emulated by the driver, go second.
 	 */
 
-	for (i = 0; (i < driver_nfourccs) && (fourccs < fourccs_end); ++i) {
-		u32 fourcc = driver_fourccs[i];
+	for (i = 0; (i < extra_nfourccs) && (fourccs < fourccs_end); ++i) {
+		u32 fourcc = extra_fourccs[i];
 
 		if (is_listed_fourcc(fourccs_out, fourccs - fourccs_out, fourcc)) {
 			continue; /* skip duplicate and native entries */
 		} else if (fourccs == fourccs_end) {
 			drm_warn(dev, "Ignoring emulated format %p4cc\n", &fourcc);
 			continue; /* end of available output buffer */
-		} else if (!is_conversion_supported(fourcc, native_format)) {
-			drm_dbg_kms(dev, "Unsupported emulated format %p4cc\n", &fourcc);
-			continue; /* format is not supported for conversion */
 		}
 
 		drm_dbg_kms(dev, "adding emulated format %p4cc\n", &fourcc);
diff --git a/drivers/gpu/drm/tiny/ofdrm.c b/drivers/gpu/drm/tiny/ofdrm.c
index 33eefeba437c..39c5fd463fec 100644
--- a/drivers/gpu/drm/tiny/ofdrm.c
+++ b/drivers/gpu/drm/tiny/ofdrm.c
@@ -754,24 +754,6 @@ static void ofdrm_crtc_state_destroy(struct ofdrm_crtc_state *ofdrm_crtc_state)
 	kfree(ofdrm_crtc_state);
 }
 
-/*
- * Support all formats of OF display and maybe more; in order
- * of preference. The display's update function will do any
- * conversion necessary.
- *
- * TODO: Add blit helpers for remaining formats and uncomment
- *       constants.
- */
-static const uint32_t ofdrm_primary_plane_formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_RGB565,
-	//DRM_FORMAT_XRGB1555,
-	//DRM_FORMAT_C8,
-	/* Big-endian formats below */
-	DRM_FORMAT_BGRX8888,
-	DRM_FORMAT_RGB565 | DRM_FORMAT_BIG_ENDIAN,
-};
-
 static const uint64_t ofdrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
@@ -1290,8 +1272,6 @@ static struct ofdrm_device *ofdrm_device_create(struct drm_driver *drv,
 	/* Primary plane */
 
 	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
-					    ofdrm_primary_plane_formats,
-					    ARRAY_SIZE(ofdrm_primary_plane_formats),
 					    odev->formats, ARRAY_SIZE(odev->formats));
 
 	primary_plane = &odev->primary_plane;
diff --git a/drivers/gpu/drm/tiny/simpledrm.c b/drivers/gpu/drm/tiny/simpledrm.c
index 30e928d627e8..7355617f38d3 100644
--- a/drivers/gpu/drm/tiny/simpledrm.c
+++ b/drivers/gpu/drm/tiny/simpledrm.c
@@ -446,25 +446,6 @@ static int simpledrm_device_init_regulators(struct simpledrm_device *sdev)
  * Modesetting
  */
 
-/*
- * Support all formats of simplefb and maybe more; in order
- * of preference. The display's update function will do any
- * conversion necessary.
- *
- * TODO: Add blit helpers for remaining formats and uncomment
- *       constants.
- */
-static const uint32_t simpledrm_primary_plane_formats[] = {
-	DRM_FORMAT_XRGB8888,
-	DRM_FORMAT_ARGB8888,
-	DRM_FORMAT_RGB565,
-	//DRM_FORMAT_XRGB1555,
-	//DRM_FORMAT_ARGB1555,
-	DRM_FORMAT_RGB888,
-	DRM_FORMAT_XRGB2101010,
-	DRM_FORMAT_ARGB2101010,
-};
-
 static const uint64_t simpledrm_primary_plane_format_modifiers[] = {
 	DRM_FORMAT_MOD_LINEAR,
 	DRM_FORMAT_MOD_INVALID
@@ -745,8 +726,6 @@ static struct simpledrm_device *simpledrm_device_create(struct drm_driver *drv,
 	/* Primary plane */
 
 	nformats = drm_fb_build_fourcc_list(dev, &format->format, 1,
-					    simpledrm_primary_plane_formats,
-					    ARRAY_SIZE(simpledrm_primary_plane_formats),
 					    sdev->formats, ARRAY_SIZE(sdev->formats));
 
 	primary_plane = &sdev->primary_plane;
diff --git a/include/drm/drm_format_helper.h b/include/drm/drm_format_helper.h
index 2d04f5863722..291deb09475b 100644
--- a/include/drm/drm_format_helper.h
+++ b/include/drm/drm_format_helper.h
@@ -65,7 +65,6 @@ void drm_fb_xrgb8888_to_mono(struct iosys_map *dst, const unsigned int *dst_pitc
 
 size_t drm_fb_build_fourcc_list(struct drm_device *dev,
 				const u32 *native_fourccs, size_t native_nfourccs,
-				const u32 *extra_fourccs, size_t extra_nfourccs,
 				u32 *fourccs_out, size_t nfourccs_out);
 
 #endif /* __LINUX_DRM_FORMAT_HELPER_H */
-- 
2.39.0

[PATCH v3 13/13] drm/format-helper: Remove unnecessary conversion helpers

[Thomas Zimmermann]

Drivers only emulate XRGB8888 framebuffers. Remove all conversion
helpers that do not use XRGB8888 as their source format. Also remove
some special cases for alpha formats in the blit helper.

Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/gpu/drm/drm_format_helper.c | 75 -----------------------------
 1 file changed, 75 deletions(-)

diff --git a/drivers/gpu/drm/drm_format_helper.c b/drivers/gpu/drm/drm_format_helper.c
index 0523f81e2445..994f8fb71f45 100644
--- a/drivers/gpu/drm/drm_format_helper.c
+++ b/drivers/gpu/drm/drm_format_helper.c
@@ -649,65 +649,6 @@ void drm_fb_xrgb8888_to_argb8888(struct iosys_map *dst, const unsigned int *dst_
 }
 EXPORT_SYMBOL(drm_fb_xrgb8888_to_argb8888);
 
-static void drm_fb_rgb565_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
-{
-	__le32 *dbuf32 = dbuf;
-	const __le16 *sbuf16 = sbuf;
-	unsigned int x;
-
-	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);
-	}
-}
-
-static void drm_fb_rgb565_to_xrgb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
-				      const struct iosys_map *src,
-				      const struct drm_framebuffer *fb,
-				      const struct drm_rect *clip)
-{
-	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
-		4,
-	};
-
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
-		    drm_fb_rgb565_to_xrgb8888_line);
-}
-
-static void drm_fb_rgb888_to_xrgb8888_line(void *dbuf, const void *sbuf, unsigned int pixels)
-{
-	__le32 *dbuf32 = dbuf;
-	const u8 *sbuf8 = sbuf;
-	unsigned int x;
-
-	for (x = 0; x < pixels; x++) {
-		u8 r = *sbuf8++;
-		u8 g = *sbuf8++;
-		u8 b = *sbuf8++;
-		u32 pix = 0xff000000 | (r << 16) | (g << 8) | b;
-		dbuf32[x] = cpu_to_le32(pix);
-	}
-}
-
-static void drm_fb_rgb888_to_xrgb8888(struct iosys_map *dst, const unsigned int *dst_pitch,
-				      const struct iosys_map *src,
-				      const struct drm_framebuffer *fb,
-				      const struct drm_rect *clip)
-{
-	static const u8 dst_pixsize[DRM_FORMAT_MAX_PLANES] = {
-		4,
-	};
-
-	drm_fb_xfrm(dst, dst_pitch, dst_pixsize, src, fb, clip, false,
-		    drm_fb_rgb888_to_xrgb8888_line);
-}
-
 static void drm_fb_xrgb8888_to_xrgb2101010_line(void *dbuf, const void *sbuf, unsigned int pixels)
 {
 	__le32 *dbuf32 = dbuf;
@@ -899,12 +840,6 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 {
 	uint32_t fb_format = fb->format->format;
 
-	/* treat alpha channel like filler bits */
-	if (fb_format == DRM_FORMAT_ARGB8888)
-		fb_format = DRM_FORMAT_XRGB8888;
-	if (fb_format == DRM_FORMAT_ARGB2101010)
-		fb_format = DRM_FORMAT_XRGB2101010;
-
 	if (fb_format == dst_format) {
 		drm_fb_memcpy(dst, dst_pitch, src, fb, clip);
 		return 0;
@@ -943,16 +878,6 @@ int drm_fb_blit(struct iosys_map *dst, const unsigned int *dst_pitch, uint32_t d
 			drm_fb_swab(dst, dst_pitch, src, fb, clip, false);
 			return 0;
 		}
-	} else if (fb_format == DRM_FORMAT_RGB888) {
-		if (dst_format == DRM_FORMAT_XRGB8888) {
-			drm_fb_rgb888_to_xrgb8888(dst, dst_pitch, src, fb, clip);
-			return 0;
-		}
-	} else if (fb_format == DRM_FORMAT_RGB565) {
-		if (dst_format == DRM_FORMAT_XRGB8888) {
-			drm_fb_rgb565_to_xrgb8888(dst, dst_pitch, src, fb, clip);
-			return 0;
-		}
 	}
 
 	drm_warn_once(fb->dev, "No conversion helper from %p4cc to %p4cc found.\n",
-- 
2.39.0

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Maíra Canal]

Hi Thomas,

On 1/2/23 08:29, Thomas Zimmermann wrote:
> Fix the color-format selection of the single-probe helper. Go
> through all user-specified values and test each for compatibility
> with the driver. If none is supported, use the driver-provided
> default. This guarantees that the console is always available in
> any color format at least.
> 
> Until now, the format selection of the single-probe helper tried
> to either use a user-specified format or a 32-bit default format.
> If the user-specified format was not supported by the driver, the
> selection failed and the display remained blank.
> 
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---

I started to get the following warning on the Raspberry Pi 4 Model B
(arm64/defconfig) using drm-misc-next:

[    4.376317] [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0
[    4.433587] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
[    4.433617] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
[    4.433629] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
[    4.433640] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
[    4.433650] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
[    4.433658] vc4-drm gpu: [drm] No compatible format found
[    4.433854] ------------[ cut here ]------------
[    4.433861] WARNING: CPU: 2 PID: 66 at drivers/gpu/drm/drm_atomic.c:1604 __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
[    4.434172] Modules linked in: btbcm(+) crct10dif_ce reset_raspberrypi clk_raspberrypi raspberrypi_hwmon bluetooth ecdh_generic ecc
pwm_bcm2835 broadcom rfkill iproc_rng200 bcm_phy_lib i2c_bcm2835 vc4 rng_core snd_soc_hdmi_codec bcm2711_thermal cec drm_display_helper
v3d pcie_brcmstb drm_dma_helper gpu_sched genet drm_shmem_helper nvmem_rmem mdio_bcm_unimac drm_kms_helper drm fuse ip_tables x_tables
ipv6
[    4.434322] CPU: 2 PID: 66 Comm: kworker/u8:2 Not tainted 6.1.0-rc6-00011-g37c90d589dc0 #29
[    4.434337] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
[    4.434345] Workqueue: events_unbound deferred_probe_work_func
[    4.434376] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
[    4.434390] pc : __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
[    4.434668] lr : __drm_atomic_helper_set_config+0x64/0x314 [drm]
[    4.434943] sp : ffff8000082c3840
[    4.434949] x29: ffff8000082c3850 x28: ffff2d6d448e12c0 x27: 0000000000000001
[    4.434972] x26: 0000000000000038 x25: ffff2d6d448e12c0 x24: ffff2d6d401a0690
[    4.434991] x23: ffff2d6d41f74080 x22: ffff2d6d40d8a400 x21: ffff2d6d433fcc00
[    4.435009] x20: ffff2d6d401a0690 x19: ffff2d6d44b8e180 x18: 0000000000000020
[    4.435027] x17: 0000000000000010 x16: ffffa6757bee52d0 x15: 0000000000000000
[    4.435044] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
[    4.435063] x11: 0000000000000000 x10: ffff2d6d43240800 x9 : ffff2d6d44b8e200
[    4.435081] x8 : 0000000000000000 x7 : ffff2d6d44b8e180 x6 : ffff2d6d40d8a400
[    4.435099] x5 : ffff2d6d45e7ca80 x4 : 0000000000000050 x3 : ffffa675498c3bad
[    4.435116] x2 : 0000000000000004 x1 : ffff2d6d4323f080 x0 : ffff2d6d433fcc00
[    4.435136] Call trace:
[    4.435143]  __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
[    4.435440]  drm_client_modeset_commit_atomic+0x140/0x244 [drm]
[    4.435723]  drm_client_modeset_commit_locked+0x50/0x168 [drm]
[    4.436001]  drm_client_modeset_commit+0x2c/0x54 [drm]
[    4.436273]  __drm_fb_helper_initial_config_and_unlock+0x548/0x5a0 [drm_kms_helper]
[    4.436407]  drm_fb_helper_initial_config+0x38/0x50 [drm_kms_helper]
[    4.436528]  drm_fbdev_client_hotplug+0xa8/0x120 [drm_kms_helper]
[    4.436648]  drm_fbdev_generic_setup+0x80/0x150 [drm_kms_helper]
[    4.436768]  vc4_drm_bind+0x1f0/0x22c [vc4]
[    4.436928]  try_to_bring_up_aggregate_device+0x168/0x1b4
[    4.436958]  __component_add+0xbc/0x15c
[    4.436974]  component_add+0x14/0x20
[    4.436990]  vc4_hdmi_dev_probe+0x1c/0x28 [vc4]
[    4.437146]  platform_probe+0xa8/0xd0
[    4.437158]  really_probe+0x130/0x2f4
[    4.437174]  __driver_probe_device+0xb4/0xe0
[    4.437189]  driver_probe_device+0x3c/0x1f8
[    4.437202]  __device_attach_driver+0x118/0x140
[    4.437217]  bus_for_each_drv+0x84/0xd0
[    4.437229]  __device_attach+0xd0/0x19c
[    4.437243]  device_initial_probe+0x14/0x20
[    4.437256]  bus_probe_device+0x34/0x98
[    4.437268]  deferred_probe_work_func+0x88/0xc4
[    4.437282]  process_one_work+0x1cc/0x2c8
[    4.437295]  worker_thread+0x248/0x458
[    4.437304]  kthread+0xec/0x198
[    4.437319]  ret_from_fork+0x10/0x20
[    4.437333] ---[ end trace 0000000000000000 ]---

After bisecting the problem, I was able to detect that the warning started to
appear on the commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format
selection").

Do you have any idea on what might be causing this warning?

Best Regards,
- Maíra Canal

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 6116 bytes --]

Hi

Thanks for reporting the problem.

Am 03.01.23 um 22:18 schrieb Maíra Canal:
> Hi Thomas,
> 
> On 1/2/23 08:29, Thomas Zimmermann wrote:
>> Fix the color-format selection of the single-probe helper. Go
>> through all user-specified values and test each for compatibility
>> with the driver. If none is supported, use the driver-provided
>> default. This guarantees that the console is always available in
>> any color format at least.
>>
>> Until now, the format selection of the single-probe helper tried
>> to either use a user-specified format or a 32-bit default format.
>> If the user-specified format was not supported by the driver, the
>> selection failed and the display remained blank.
>>
>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>> ---
> 
> I started to get the following warning on the Raspberry Pi 4 Model B
> (arm64/defconfig) using drm-misc-next:
> 
> [    4.376317] [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0
> [    4.433587] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
> [    4.433617] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
> [    4.433629] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
> [    4.433640] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
> [    4.433650] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
> [    4.433658] vc4-drm gpu: [drm] No compatible format found
> [    4.433854] ------------[ cut here ]------------
> [    4.433861] WARNING: CPU: 2 PID: 66 at 
> drivers/gpu/drm/drm_atomic.c:1604 
> __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
> [    4.434172] Modules linked in: btbcm(+) crct10dif_ce 
> reset_raspberrypi clk_raspberrypi raspberrypi_hwmon bluetooth 
> ecdh_generic ecc
> pwm_bcm2835 broadcom rfkill iproc_rng200 bcm_phy_lib i2c_bcm2835 vc4 
> rng_core snd_soc_hdmi_codec bcm2711_thermal cec drm_display_helper
> v3d pcie_brcmstb drm_dma_helper gpu_sched genet drm_shmem_helper 
> nvmem_rmem mdio_bcm_unimac drm_kms_helper drm fuse ip_tables x_tables
> ipv6
> [    4.434322] CPU: 2 PID: 66 Comm: kworker/u8:2 Not tainted 
> 6.1.0-rc6-00011-g37c90d589dc0 #29
> [    4.434337] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
> [    4.434345] Workqueue: events_unbound deferred_probe_work_func
> [    4.434376] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS 
> BTYPE=--)
> [    4.434390] pc : __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
> [    4.434668] lr : __drm_atomic_helper_set_config+0x64/0x314 [drm]
> [    4.434943] sp : ffff8000082c3840
> [    4.434949] x29: ffff8000082c3850 x28: ffff2d6d448e12c0 x27: 
> 0000000000000001
> [    4.434972] x26: 0000000000000038 x25: ffff2d6d448e12c0 x24: 
> ffff2d6d401a0690
> [    4.434991] x23: ffff2d6d41f74080 x22: ffff2d6d40d8a400 x21: 
> ffff2d6d433fcc00
> [    4.435009] x20: ffff2d6d401a0690 x19: ffff2d6d44b8e180 x18: 
> 0000000000000020
> [    4.435027] x17: 0000000000000010 x16: ffffa6757bee52d0 x15: 
> 0000000000000000
> [    4.435044] x14: 0000000000000000 x13: 0000000000000000 x12: 
> 0000000000000000
> [    4.435063] x11: 0000000000000000 x10: ffff2d6d43240800 x9 : 
> ffff2d6d44b8e200
> [    4.435081] x8 : 0000000000000000 x7 : ffff2d6d44b8e180 x6 : 
> ffff2d6d40d8a400
> [    4.435099] x5 : ffff2d6d45e7ca80 x4 : 0000000000000050 x3 : 
> ffffa675498c3bad
> [    4.435116] x2 : 0000000000000004 x1 : ffff2d6d4323f080 x0 : 
> ffff2d6d433fcc00
> [    4.435136] Call trace:
> [    4.435143]  __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
> [    4.435440]  drm_client_modeset_commit_atomic+0x140/0x244 [drm]
> [    4.435723]  drm_client_modeset_commit_locked+0x50/0x168 [drm]
> [    4.436001]  drm_client_modeset_commit+0x2c/0x54 [drm]
> [    4.436273]  __drm_fb_helper_initial_config_and_unlock+0x548/0x5a0 
> [drm_kms_helper]
> [    4.436407]  drm_fb_helper_initial_config+0x38/0x50 [drm_kms_helper]
> [    4.436528]  drm_fbdev_client_hotplug+0xa8/0x120 [drm_kms_helper]
> [    4.436648]  drm_fbdev_generic_setup+0x80/0x150 [drm_kms_helper]
> [    4.436768]  vc4_drm_bind+0x1f0/0x22c [vc4]
> [    4.436928]  try_to_bring_up_aggregate_device+0x168/0x1b4
> [    4.436958]  __component_add+0xbc/0x15c
> [    4.436974]  component_add+0x14/0x20
> [    4.436990]  vc4_hdmi_dev_probe+0x1c/0x28 [vc4]
> [    4.437146]  platform_probe+0xa8/0xd0
> [    4.437158]  really_probe+0x130/0x2f4
> [    4.437174]  __driver_probe_device+0xb4/0xe0
> [    4.437189]  driver_probe_device+0x3c/0x1f8
> [    4.437202]  __device_attach_driver+0x118/0x140
> [    4.437217]  bus_for_each_drv+0x84/0xd0
> [    4.437229]  __device_attach+0xd0/0x19c
> [    4.437243]  device_initial_probe+0x14/0x20
> [    4.437256]  bus_probe_device+0x34/0x98
> [    4.437268]  deferred_probe_work_func+0x88/0xc4
> [    4.437282]  process_one_work+0x1cc/0x2c8
> [    4.437295]  worker_thread+0x248/0x458
> [    4.437304]  kthread+0xec/0x198
> [    4.437319]  ret_from_fork+0x10/0x20
> [    4.437333] ---[ end trace 0000000000000000 ]---
> 
> After bisecting the problem, I was able to detect that the warning 
> started to
> appear on the commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe 
> color-format
> selection").
> 
> Do you have any idea on what might be causing this warning?

vc4 wants 16 bits per pixel and 24-bit color depth at the same time. 
That makes no sense.

Does it work if you call drm_fbdev_generic_setup() with 32 bpp at [1]? 
Like this:

   drm_fbdev_generic_setup(drm, 32)

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.2-rc2/source/drivers/gpu/drm/vc4/vc4_drv.c#L391

> 
> Best Regards,
> - Maíra Canal

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Maíra Canal]

On 1/4/23 05:14, Thomas Zimmermann wrote:
> Hi
> 
> Thanks for reporting the problem.
> 
> Am 03.01.23 um 22:18 schrieb Maíra Canal:
>> Hi Thomas,
>>
>> On 1/2/23 08:29, Thomas Zimmermann wrote:
>>> Fix the color-format selection of the single-probe helper. Go
>>> through all user-specified values and test each for compatibility
>>> with the driver. If none is supported, use the driver-provided
>>> default. This guarantees that the console is always available in
>>> any color format at least.
>>>
>>> Until now, the format selection of the single-probe helper tried
>>> to either use a user-specified format or a 32-bit default format.
>>> If the user-specified format was not supported by the driver, the
>>> selection failed and the display remained blank.
>>>
>>> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
>>> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
>>> ---
>>
>> I started to get the following warning on the Raspberry Pi 4 Model B
>> (arm64/defconfig) using drm-misc-next:
>>
>> [    4.376317] [drm] Initialized vc4 0.0.0 20140616 for gpu on minor 0
>> [    4.433587] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
>> [    4.433617] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
>> [    4.433629] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
>> [    4.433640] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
>> [    4.433650] vc4-drm gpu: [drm] bpp/depth value of 16/24 not supported
>> [    4.433658] vc4-drm gpu: [drm] No compatible format found
>> [    4.433854] ------------[ cut here ]------------
>> [    4.433861] WARNING: CPU: 2 PID: 66 at drivers/gpu/drm/drm_atomic.c:1604 __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
>> [    4.434172] Modules linked in: btbcm(+) crct10dif_ce reset_raspberrypi clk_raspberrypi raspberrypi_hwmon bluetooth ecdh_generic ecc
>> pwm_bcm2835 broadcom rfkill iproc_rng200 bcm_phy_lib i2c_bcm2835 vc4 rng_core snd_soc_hdmi_codec bcm2711_thermal cec drm_display_helper
>> v3d pcie_brcmstb drm_dma_helper gpu_sched genet drm_shmem_helper nvmem_rmem mdio_bcm_unimac drm_kms_helper drm fuse ip_tables x_tables
>> ipv6
>> [    4.434322] CPU: 2 PID: 66 Comm: kworker/u8:2 Not tainted 6.1.0-rc6-00011-g37c90d589dc0 #29
>> [    4.434337] Hardware name: Raspberry Pi 4 Model B Rev 1.4 (DT)
>> [    4.434345] Workqueue: events_unbound deferred_probe_work_func
>> [    4.434376] pstate: 80000005 (Nzcv daif -PAN -UAO -TCO -DIT -SSBS BTYPE=--)
>> [    4.434390] pc : __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
>> [    4.434668] lr : __drm_atomic_helper_set_config+0x64/0x314 [drm]
>> [    4.434943] sp : ffff8000082c3840
>> [    4.434949] x29: ffff8000082c3850 x28: ffff2d6d448e12c0 x27: 0000000000000001
>> [    4.434972] x26: 0000000000000038 x25: ffff2d6d448e12c0 x24: ffff2d6d401a0690
>> [    4.434991] x23: ffff2d6d41f74080 x22: ffff2d6d40d8a400 x21: ffff2d6d433fcc00
>> [    4.435009] x20: ffff2d6d401a0690 x19: ffff2d6d44b8e180 x18: 0000000000000020
>> [    4.435027] x17: 0000000000000010 x16: ffffa6757bee52d0 x15: 0000000000000000
>> [    4.435044] x14: 0000000000000000 x13: 0000000000000000 x12: 0000000000000000
>> [    4.435063] x11: 0000000000000000 x10: ffff2d6d43240800 x9 : ffff2d6d44b8e200
>> [    4.435081] x8 : 0000000000000000 x7 : ffff2d6d44b8e180 x6 : ffff2d6d40d8a400
>> [    4.435099] x5 : ffff2d6d45e7ca80 x4 : 0000000000000050 x3 : ffffa675498c3bad
>> [    4.435116] x2 : 0000000000000004 x1 : ffff2d6d4323f080 x0 : ffff2d6d433fcc00
>> [    4.435136] Call trace:
>> [    4.435143]  __drm_atomic_helper_set_config+0x2e8/0x314 [drm]
>> [    4.435440]  drm_client_modeset_commit_atomic+0x140/0x244 [drm]
>> [    4.435723]  drm_client_modeset_commit_locked+0x50/0x168 [drm]
>> [    4.436001]  drm_client_modeset_commit+0x2c/0x54 [drm]
>> [    4.436273]  __drm_fb_helper_initial_config_and_unlock+0x548/0x5a0 [drm_kms_helper]
>> [    4.436407]  drm_fb_helper_initial_config+0x38/0x50 [drm_kms_helper]
>> [    4.436528]  drm_fbdev_client_hotplug+0xa8/0x120 [drm_kms_helper]
>> [    4.436648]  drm_fbdev_generic_setup+0x80/0x150 [drm_kms_helper]
>> [    4.436768]  vc4_drm_bind+0x1f0/0x22c [vc4]
>> [    4.436928]  try_to_bring_up_aggregate_device+0x168/0x1b4
>> [    4.436958]  __component_add+0xbc/0x15c
>> [    4.436974]  component_add+0x14/0x20
>> [    4.436990]  vc4_hdmi_dev_probe+0x1c/0x28 [vc4]
>> [    4.437146]  platform_probe+0xa8/0xd0
>> [    4.437158]  really_probe+0x130/0x2f4
>> [    4.437174]  __driver_probe_device+0xb4/0xe0
>> [    4.437189]  driver_probe_device+0x3c/0x1f8
>> [    4.437202]  __device_attach_driver+0x118/0x140
>> [    4.437217]  bus_for_each_drv+0x84/0xd0
>> [    4.437229]  __device_attach+0xd0/0x19c
>> [    4.437243]  device_initial_probe+0x14/0x20
>> [    4.437256]  bus_probe_device+0x34/0x98
>> [    4.437268]  deferred_probe_work_func+0x88/0xc4
>> [    4.437282]  process_one_work+0x1cc/0x2c8
>> [    4.437295]  worker_thread+0x248/0x458
>> [    4.437304]  kthread+0xec/0x198
>> [    4.437319]  ret_from_fork+0x10/0x20
>> [    4.437333] ---[ end trace 0000000000000000 ]---
>>
>> After bisecting the problem, I was able to detect that the warning started to
>> appear on the commit 37c90d589dc0 ("drm/fb-helper: Fix single-probe color-format
>> selection").
>>
>> Do you have any idea on what might be causing this warning?
> 
> vc4 wants 16 bits per pixel and 24-bit color depth at the same time. That makes no sense.
> 
> Does it work if you call drm_fbdev_generic_setup() with 32 bpp at [1]? Like this:
> 
>    drm_fbdev_generic_setup(drm, 32)

I wasn't able to reproduce the problem again with 32 bpp. Maybe you could send a patch
fixing it, so that Maxime could that a look and check if there is any problem with using
32 bpp.

Thanks for figuring out the problem!

Best Regards,
- Maíra Canal

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.2-rc2/source/drivers/gpu/drm/vc4/vc4_drv.c#L391
> 
>>
>> Best Regards,
>> - Maíra Canal
> 

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Pierre Asselin]

Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Select color format for EFI/VESA firmware scanout buffer from the
> number of bits per pixel and the position of the individual color
> components. Fixes the selected format for the buffer in several odd
> cases. For example, XRGB1555 has been reported as ARGB1555 because
> of the different use of depth and transparency in VESA and Linux.
>
> Bits-per-pixel is always the pixel's raw number of bits; including
> alpha and filler bits. It is preferred over color depth, which has a
> different meaning among various components and standards.
>
> Also do not compare reserved bits and transparency bits to each other.
> These values have different meanings, as reserved bits include filler
> bits while transparency does not.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>  drivers/firmware/sysfb_simplefb.c | 43 ++++++++++++++++++++++++++-----
>  1 file changed, 37 insertions(+), 6 deletions(-)
>
[patch elided]

Starting at linux-6.3-rc1 my simplefb picks the wrong mode and garbles
the display This is on a 16-year old i686 laptop.  I can post lshw or
dmidecode output if it helps.

The issue is still present in 6.3-rc5.

Screenshot: https://www.panix.com/~pa/linux-6.3-simplefb/bug-simplefb-bad.jpg
Compare: https://www.panix.com/~pa/linux-6.3-simplefb/bug-simplefb-good.jpg

This happens during early boot.  Grub picks a 1024x786 mode and leaves
it for the kernel.  The screenshots are from a rescueshell in early
userspace.  The dmesg excerpts at the bottom might give some clues.

I bisected it to f35cd3fa77293c2cd03e94b6a6151e1a7d9309cf
    firmware/sysfb: Fix EFI/VESA format selection
which is this patch.

It's not the end of the world:
1) I have a workaround, booting with vga=0x318;
2) the screen is usable even without the workaround;
3) the final fbcon driver takes over after the switch_root.
Nevertheless, it would be nice to get this fixed this before 6.3.

I may be the only one with this problem.  Who else runs -rc kernels on
such old hardware ?  I'll answer questions as best I can and test any
patches thrown at me.

--pa

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 06.04.23 17:45, Pierre Asselin wrote:
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> [...] 
> Starting at linux-6.3-rc1 my simplefb picks the wrong mode and garbles
> the display This is on a 16-year old i686 laptop.  I can post lshw or
> dmidecode output if it helps.
> [...] 
> I bisected it to f35cd3fa77293c2cd03e94b6a6151e1a7d9309cf
>     firmware/sysfb: Fix EFI/VESA format selection

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced f35cd3fa77293c2cd03e
#regzbot title firmware/sysfb: wrong mode and display garbled on 16-year
old i686 laptop
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 06.04.23 17:45, Pierre Asselin wrote:
> Thomas Zimmermann <tzimmermann@suse.de> wrote:
> [...] 
> Starting at linux-6.3-rc1 my simplefb picks the wrong mode and garbles
> the display This is on a 16-year old i686 laptop.  I can post lshw or
> dmidecode output if it helps.
> [...] 
> I bisected it to f35cd3fa77293c2cd03e94b6a6151e1a7d9309cf
>     firmware/sysfb: Fix EFI/VESA format selection

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced f35cd3fa77293c2cd03e
#regzbot title firmware/sysfb: wrong mode and display garbled on 16-year
old i686 laptop
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Pierre Asselin]

Oh oh, I just reproduced the problem on a desktop tower, a Dell Precision
T3610, probably 2019 vintage.  The only thing in common with the old
laptop, that I can think of, is that both use the legacy BIOS.  The Dell
has EFI but the graphics card may not support that; there is no integrated
graphics, if I switch to EFI and lose the video I may not be able to switch
back.

Recommend some testing on boxes with legacy BIOS.

--PA

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Javier Martinez Canillas]

"Pierre Asselin" <pa@panix.com> writes:

Hello Pierre,

> Oh oh, I just reproduced the problem on a desktop tower, a Dell Precision
> T3610, probably 2019 vintage.  The only thing in common with the old
> laptop, that I can think of, is that both use the legacy BIOS.  The Dell
> has EFI but the graphics card may not support that; there is no integrated
> graphics, if I switch to EFI and lose the video I may not be able to switch
> back.
>

Can you please share you grub config file? It seems that is set to
GRUB_GFXMODE=1024x768x32 but the actual mode is set to 1024x768x24 ?

That is, it fails when the picked format is DRM_FORMAT_RGB8888 but
works when is DRM_FORMAT_XRGB888. I can't spot any error in Thomas'
patch so I wonder if the problem is with what grub is passing to the
kernel.

The mentioned vga=0x318 workaround that you mentioned makes the mode
passed to match the selected DRM_FORMAT_RGB888 which I guess is why
that worked for you.

-- 
Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Pierre Asselin]

> Can you please share you grub config file? It seems that is set to
> GRUB_GFXMODE=1024x768x32 but the actual mode is set to 1024x768x24 ?

Okay, but you'll be sorry...  The gfxmode is set to "keep" in all the
entries.  https://www.panix.com/~pa/linux-6.3-simplefb/grub.cfg .

The "TEST" entry was used to bisect.  The "PRE-TEST" was to set things
up, to receive the bzImages compiled on a faster machine. Now I boot
the "Linux 6.3.0-rc5-x86".


> That is, it fails when the picked format is DRM_FORMAT_RGB8888 but
> works when is DRM_FORMAT_XRGB888. I can't spot any error in Thomas'
> patch so I wonder if the problem is with what grub is passing to the
> kernel.
>
> The mentioned vga=0x318 workaround that you mentioned makes the mode
> passed to match the selected DRM_FORMAT_RGB888 which I guess is why
> that worked for you.

All right, I did a series of reboots, editing the grub command line
to change the "gfxpayload=" grub option or the "vga=" kernel option.
In each case I captured the output of
  "dmesg | egrep -i 'fbcon|console|fb0|frameb|simple|vga|vesa'

https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes

(D'oh.  My script printed "vga=vga=" twice when that option was set.
Good enough as is.)

Note the difference in linelength= between the bad and good r8g8b8.
Does it mean anything ?
 (bad)> format=r8g8b8, mode=1024x768x24, linelength=4096
(good)> format=r8g8b8, mode=1024x768x24, linelength=3072

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Javier Martinez Canillas]

"Pierre Asselin" <pa@panix.com> writes:

>> Can you please share you grub config file? It seems that is set to
>> GRUB_GFXMODE=1024x768x32 but the actual mode is set to 1024x768x24 ?
>
> Okay, but you'll be sorry...  The gfxmode is set to "keep" in all the
> entries.  https://www.panix.com/~pa/linux-6.3-simplefb/grub.cfg .
>
> The "TEST" entry was used to bisect.  The "PRE-TEST" was to set things
> up, to receive the bzImages compiled on a faster machine. Now I boot
> the "Linux 6.3.0-rc5-x86".
>
>
>> That is, it fails when the picked format is DRM_FORMAT_RGB8888 but
>> works when is DRM_FORMAT_XRGB888. I can't spot any error in Thomas'
>> patch so I wonder if the problem is with what grub is passing to the
>> kernel.
>>
>> The mentioned vga=0x318 workaround that you mentioned makes the mode
>> passed to match the selected DRM_FORMAT_RGB888 which I guess is why
>> that worked for you.
>
> All right, I did a series of reboots, editing the grub command line
> to change the "gfxpayload=" grub option or the "vga=" kernel option.
> In each case I captured the output of
>   "dmesg | egrep -i 'fbcon|console|fb0|frameb|simple|vga|vesa'
>
> https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes
>
> (D'oh.  My script printed "vga=vga=" twice when that option was set.
> Good enough as is.)
>
> Note the difference in linelength= between the bad and good r8g8b8.
> Does it mean anything ?
>  (bad)> format=r8g8b8, mode=1024x768x24, linelength=4096
> (good)> format=r8g8b8, mode=1024x768x24, linelength=3072
>

Ah! That's a good data point and I believe that found a possible issue in
the sysfb format selection logic. Can you please try the following patch?

From 55b5375c528b4128350dfa2126277049f8821349 Mon Sep 17 00:00:00 2001
From: Javier Martinez Canillas <javierm@redhat.com>
Date: Wed, 12 Apr 2023 13:20:48 +0200
Subject: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
 calculated

The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
fixed format selection by calculating the bits-per-pixel instead of just
using the reported color depth.

But unfortunately this broke some modes because the stride is always set
to the reported line length (in bytes), which could not match the actual
stride if the calculated bits-per-pixel doesn't match the reported depth.

Fixes: commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
Reported-by: Pierre Asselin <pa@panix.com>
Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
---
 drivers/firmware/sysfb_simplefb.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
index 82c64cb9f531..5dc23e57089f 100644
--- a/drivers/firmware/sysfb_simplefb.c
+++ b/drivers/firmware/sysfb_simplefb.c
@@ -28,7 +28,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 			     struct simplefb_platform_data *mode)
 {
 	__u8 type;
-	u32 bits_per_pixel;
+	u32 bits_per_pixel, stride;
 	unsigned int i;
 
 	type = si->orig_video_isVGA;
@@ -54,14 +54,19 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
 	 * ignore simplefb formats with alpha bits, as EFI and VESA
 	 * don't specify alpha channels.
+	 *
+	 * If a calculated bits_per_pixel is used instead of lfb_depth,
+	 * then also ignore lfb_linelength and calculate the stride.
 	 */
 	if (si->lfb_depth > 8) {
 		bits_per_pixel = max(max3(si->red_size + si->red_pos,
 					  si->green_size + si->green_pos,
 					  si->blue_size + si->blue_pos),
 				     si->rsvd_size + si->rsvd_pos);
+		stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8);
 	} else {
 		bits_per_pixel = si->lfb_depth;
+		stride = si->lfb_linelength;
 	}
 
 	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
@@ -80,7 +85,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
 			mode->format = f->name;
 			mode->width = si->lfb_width;
 			mode->height = si->lfb_height;
-			mode->stride = si->lfb_linelength;
+			mode->stride = stride;
 			return true;
 		}
 	}

base-commit: fd35174e13f98f9232c4aa66689816731d34ca28
-- 

Best regards,

Javier Martinez Canillas
Core Platforms
Red Hat

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Link: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]

On 08.04.23 13:26, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> 
> On 06.04.23 17:45, Pierre Asselin wrote:
>> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> [...] 
>> Starting at linux-6.3-rc1 my simplefb picks the wrong mode and garbles
>> the display This is on a 16-year old i686 laptop.  I can post lshw or
>> dmidecode output if it helps.
>> [...] 
>> I bisected it to f35cd3fa77293c2cd03e94b6a6151e1a7d9309cf
>>     firmware/sysfb: Fix EFI/VESA format selection
> 
> Thanks for the report. To be sure the issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced f35cd3fa77293c2cd03e
> #regzbot title firmware/sysfb: wrong mode and display garbled on 16-year
> old i686 laptop
> #regzbot ignore-activity

#regzbot monitor:
https://lore.kernel.org/lkml/20230412150225.3757223-1-javierm@redhat.com/
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Link: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]

On 08.04.23 13:26, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> 
> On 06.04.23 17:45, Pierre Asselin wrote:
>> Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> [...] 
>> Starting at linux-6.3-rc1 my simplefb picks the wrong mode and garbles
>> the display This is on a 16-year old i686 laptop.  I can post lshw or
>> dmidecode output if it helps.
>> [...] 
>> I bisected it to f35cd3fa77293c2cd03e94b6a6151e1a7d9309cf
>>     firmware/sysfb: Fix EFI/VESA format selection
> 
> Thanks for the report. To be sure the issue doesn't fall through the
> cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
> tracking bot:
> 
> #regzbot ^introduced f35cd3fa77293c2cd03e
> #regzbot title firmware/sysfb: wrong mode and display garbled on 16-year
> old i686 laptop
> #regzbot ignore-activity

#regzbot monitor:
https://lore.kernel.org/lkml/20230412150225.3757223-1-javierm@redhat.com/
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 01/13] firmware/sysfb: Fix EFI/VESA format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4715 bytes --]

ping

Am 12.04.23 um 13:22 schrieb Javier Martinez Canillas:
> "Pierre Asselin" <pa@panix.com> writes:
> 
>>> Can you please share you grub config file? It seems that is set to
>>> GRUB_GFXMODE=1024x768x32 but the actual mode is set to 1024x768x24 ?
>>
>> Okay, but you'll be sorry...  The gfxmode is set to "keep" in all the
>> entries.  https://www.panix.com/~pa/linux-6.3-simplefb/grub.cfg .
>>
>> The "TEST" entry was used to bisect.  The "PRE-TEST" was to set things
>> up, to receive the bzImages compiled on a faster machine. Now I boot
>> the "Linux 6.3.0-rc5-x86".
>>
>>
>>> That is, it fails when the picked format is DRM_FORMAT_RGB8888 but
>>> works when is DRM_FORMAT_XRGB888. I can't spot any error in Thomas'
>>> patch so I wonder if the problem is with what grub is passing to the
>>> kernel.
>>>
>>> The mentioned vga=0x318 workaround that you mentioned makes the mode
>>> passed to match the selected DRM_FORMAT_RGB888 which I guess is why
>>> that worked for you.
>>
>> All right, I did a series of reboots, editing the grub command line
>> to change the "gfxpayload=" grub option or the "vga=" kernel option.
>> In each case I captured the output of
>>    "dmesg | egrep -i 'fbcon|console|fb0|frameb|simple|vga|vesa'
>>
>> https://www.panix.com/~pa/linux-6.3-simplefb/selected-modes
>>
>> (D'oh.  My script printed "vga=vga=" twice when that option was set.
>> Good enough as is.)
>>
>> Note the difference in linelength= between the bad and good r8g8b8.
>> Does it mean anything ?
>>   (bad)> format=r8g8b8, mode=1024x768x24, linelength=4096
>> (good)> format=r8g8b8, mode=1024x768x24, linelength=3072
>>
> 
> Ah! That's a good data point and I believe that found a possible issue in
> the sysfb format selection logic. Can you please try the following patch?
> 
>  From 55b5375c528b4128350dfa2126277049f8821349 Mon Sep 17 00:00:00 2001
> From: Javier Martinez Canillas <javierm@redhat.com>
> Date: Wed, 12 Apr 2023 13:20:48 +0200
> Subject: [PATCH] firmware/sysfb: Fix wrong stride when bits-per-pixel is
>   calculated
> 
> The commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> fixed format selection by calculating the bits-per-pixel instead of just
> using the reported color depth.
> 
> But unfortunately this broke some modes because the stride is always set
> to the reported line length (in bytes), which could not match the actual
> stride if the calculated bits-per-pixel doesn't match the reported depth.
> 
> Fixes: commit f35cd3fa7729 ("firmware/sysfb: Fix EFI/VESA format selection")
> Reported-by: Pierre Asselin <pa@panix.com>
> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> ---
>   drivers/firmware/sysfb_simplefb.c | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/firmware/sysfb_simplefb.c b/drivers/firmware/sysfb_simplefb.c
> index 82c64cb9f531..5dc23e57089f 100644
> --- a/drivers/firmware/sysfb_simplefb.c
> +++ b/drivers/firmware/sysfb_simplefb.c
> @@ -28,7 +28,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   			     struct simplefb_platform_data *mode)
>   {
>   	__u8 type;
> -	u32 bits_per_pixel;
> +	u32 bits_per_pixel, stride;
>   	unsigned int i;
>   
>   	type = si->orig_video_isVGA;
> @@ -54,14 +54,19 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   	 * bits_per_pixel here and ignore lfb_depth. In the loop below,
>   	 * ignore simplefb formats with alpha bits, as EFI and VESA
>   	 * don't specify alpha channels.
> +	 *
> +	 * If a calculated bits_per_pixel is used instead of lfb_depth,
> +	 * then also ignore lfb_linelength and calculate the stride.
>   	 */
>   	if (si->lfb_depth > 8) {
>   		bits_per_pixel = max(max3(si->red_size + si->red_pos,
>   					  si->green_size + si->green_pos,
>   					  si->blue_size + si->blue_pos),
>   				     si->rsvd_size + si->rsvd_pos);
> +		stride = DIV_ROUND_UP(si->lfb_width * bits_per_pixel, 8);
>   	} else {
>   		bits_per_pixel = si->lfb_depth;
> +		stride = si->lfb_linelength;
>   	}
>   
>   	for (i = 0; i < ARRAY_SIZE(formats); ++i) {
> @@ -80,7 +85,7 @@ __init bool sysfb_parse_mode(const struct screen_info *si,
>   			mode->format = f->name;
>   			mode->width = si->lfb_width;
>   			mode->height = si->lfb_height;
> -			mode->stride = si->lfb_linelength;
> +			mode->stride = stride;
>   			return true;
>   		}
>   	}
> 
> base-commit: fd35174e13f98f9232c4aa66689816731d34ca28

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Maxfeldstr. 5, 90409 Nürnberg, Germany
(HRB 36809, AG Nürnberg)
Geschäftsführer: Ivo Totev

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linus Walleij]

Sorry for late regression detection but this patch regresses
the Integrator AB IMPD-1 graphics, I bisected down to this
patch.

On Mon, Jan 2, 2023 at 12:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:

> Fix the color-format selection of the single-probe helper. Go
> through all user-specified values and test each for compatibility
> with the driver. If none is supported, use the driver-provided
> default. This guarantees that the console is always available in
> any color format at least.
>
> Until now, the format selection of the single-probe helper tried
> to either use a user-specified format or a 32-bit default format.
> If the user-specified format was not supported by the driver, the
> selection failed and the display remained blank.
>
> Signed-off-by: Thomas Zimmermann <tzimmermann@suse.de>
> Reviewed-by: Javier Martinez Canillas <javierm@redhat.com>

Before this patch:

[drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
drm-clcd-pl111 c1000000.display: [drm] requested bpp 16, scaled depth down to 15
drm-clcd-pl111 c1000000.display: enable IM-PD1 CLCD connectors
Console: switching to colour frame buffer device 80x30
drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device

After this patch:

[drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
drm-clcd-pl111 c1000000.display: [drm] bpp/depth value of 16/16 not supported
drm-clcd-pl111 c1000000.display: [drm] No compatible format found
drm-clcd-pl111 c1000000.display: [drm] *ERROR* fbdev: Failed to setup
generic emulation (ret=-12)

It seems the bpp downscaling stopped to work?

This is the driver:
drivers/gpu/drm/pl111/pl111_versatile.c
with the pl110_impd1 variant, so these are the supported modes:

/* PL110 pixel formats for Integrator, vanilla PL110 */
static const u32 pl110_integrator_pixel_formats[] = {
        DRM_FORMAT_ABGR8888,
        DRM_FORMAT_XBGR8888,
        DRM_FORMAT_ARGB8888,
        DRM_FORMAT_XRGB8888,
        DRM_FORMAT_ABGR1555,
        DRM_FORMAT_XBGR1555,
        DRM_FORMAT_ARGB1555,
        DRM_FORMAT_XRGB1555,
};
(...)
/*
 * The IM-PD1 variant is a PL110 with a bunch of broken, or not
 * yet implemented features
 */
static const struct pl111_variant_data pl110_impd1 = {
        .name = "PL110 IM-PD1",
        .is_pl110 = true,
        .broken_clockdivider = true,
        .broken_vblank = true,
        .formats = pl110_integrator_pixel_formats,
        .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
        .fb_bpp = 16,
};

Notice the absence of RGB565!
Then we initialized the frambuffer like this:

        drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);

And as you see priv->variant->fb_bpp will be 16, so we want some
16bpp mode however the only supported depth is 15 (the 1555 modes)
so it would use that by scaling back depth to 15.

However after this patch that doesn't work anymore.

Any hints on how we can fix this?

Yours,
Linus Walleij

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2681 bytes --]

Hi

Am 12.05.23 um 15:20 schrieb Linus Walleij:
> Sorry for late regression detection but this patch regresses
> the Integrator AB IMPD-1 graphics, I bisected down to this
> patch.
[...]
> This is the driver:
> drivers/gpu/drm/pl111/pl111_versatile.c
> with the pl110_impd1 variant, so these are the supported modes:
> 
> /* PL110 pixel formats for Integrator, vanilla PL110 */
> static const u32 pl110_integrator_pixel_formats[] = {
>          DRM_FORMAT_ABGR8888,
>          DRM_FORMAT_XBGR8888,
>          DRM_FORMAT_ARGB8888,
>          DRM_FORMAT_XRGB8888,
>          DRM_FORMAT_ABGR1555,
>          DRM_FORMAT_XBGR1555,
>          DRM_FORMAT_ARGB1555,
>          DRM_FORMAT_XRGB1555,
> };
> (...)
> /*
>   * The IM-PD1 variant is a PL110 with a bunch of broken, or not
>   * yet implemented features
>   */
> static const struct pl111_variant_data pl110_impd1 = {
>          .name = "PL110 IM-PD1",
>          .is_pl110 = true,
>          .broken_clockdivider = true,
>          .broken_vblank = true,
>          .formats = pl110_integrator_pixel_formats,
>          .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
>          .fb_bpp = 16,
> };
> 
> Notice the absence of RGB565!
> Then we initialized the frambuffer like this:
> 
>          drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);
> 
> And as you see priv->variant->fb_bpp will be 16, so we want some
> 16bpp mode however the only supported depth is 15 (the 1555 modes)
> so it would use that by scaling back depth to 15.
> 
> However after this patch that doesn't work anymore.
> 
> Any hints on how we can fix this?

According to a quick grep for fb_bpp, it's only used for the call to 
drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for 
the models without rgb565. The switch at [1] should then pick the 
correct values.

The preferred_bpp parameter had a change in semantics. It used to be the 
number of bits per pixel. But that makes it hard to select between 
RGB1555 and RGB565.  So it's now a special color-mode value that works 
like the kernel's video= parameter. Values of 15 and 32 are different 
from the rest. That switch at [1] explains it. Maybe you should rename 
fb_bpp to color_mode for clarity.

Let me know if this helps.

Best regards
Thomas

[1] 
https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L1827

> 
> Yours,
> Linus Walleij

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 12.05.23 15:20, Linus Walleij wrote:
> Sorry for late regression detection but this patch regresses
> the Integrator AB IMPD-1 graphics, I bisected down to this
> patch.
> 
> On Mon, Jan 2, 2023 at 12:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> [...]
> Before this patch:
> 
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] requested bpp 16, scaled depth down to 15
> drm-clcd-pl111 c1000000.display: enable IM-PD1 CLCD connectors
> Console: switching to colour frame buffer device 80x30
> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> 
> After this patch:
> 
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] bpp/depth value of 16/16 not supported
> drm-clcd-pl111 c1000000.display: [drm] No compatible format found
> drm-clcd-pl111 c1000000.display: [drm] *ERROR* fbdev: Failed to setup
> generic emulation (ret=-12)
> 
> It seems the bpp downscaling stopped to work? [...]

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 37c90d589dc
#regzbot title drm/fb-helper: downscaling apparently stopped to work
with pl110_impd1
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linux regression tracking #adding (Thorsten Leemhuis)]

[CCing the regression list, as it should be in the loop for regressions:
https://docs.kernel.org/admin-guide/reporting-regressions.html]

[TLDR: I'm adding this report to the list of tracked Linux kernel
regressions; the text you find below is based on a few templates
paragraphs you might have encountered already in similar form.
See link in footer if these mails annoy you.]

On 12.05.23 15:20, Linus Walleij wrote:
> Sorry for late regression detection but this patch regresses
> the Integrator AB IMPD-1 graphics, I bisected down to this
> patch.
> 
> On Mon, Jan 2, 2023 at 12:30 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> [...]
> Before this patch:
> 
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] requested bpp 16, scaled depth down to 15
> drm-clcd-pl111 c1000000.display: enable IM-PD1 CLCD connectors
> Console: switching to colour frame buffer device 80x30
> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> 
> After this patch:
> 
> [drm] Initialized pl111 1.0.0 20170317 for c1000000.display on minor 0
> drm-clcd-pl111 c1000000.display: [drm] bpp/depth value of 16/16 not supported
> drm-clcd-pl111 c1000000.display: [drm] No compatible format found
> drm-clcd-pl111 c1000000.display: [drm] *ERROR* fbdev: Failed to setup
> generic emulation (ret=-12)
> 
> It seems the bpp downscaling stopped to work? [...]

Thanks for the report. To be sure the issue doesn't fall through the
cracks unnoticed, I'm adding it to regzbot, the Linux kernel regression
tracking bot:

#regzbot ^introduced 37c90d589dc
#regzbot title drm/fb-helper: downscaling apparently stopped to work
with pl110_impd1
#regzbot ignore-activity

This isn't a regression? This issue or a fix for it are already
discussed somewhere else? It was fixed already? You want to clarify when
the regression started to happen? Or point out I got the title or
something else totally wrong? Then just reply and tell me -- ideally
while also telling regzbot about it, as explained by the page listed in
the footer of this mail.

Developers: When fixing the issue, remember to add 'Link:' tags pointing
to the report (the parent of this mail). See page linked in footer for
details.

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linus Walleij]

On Fri, May 12, 2023 at 4:11 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:

> > Any hints on how we can fix this?
>
> According to a quick grep for fb_bpp, it's only used for the call to
> drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for
> the models without rgb565. The switch at [1] should then pick the
> correct values.

I tried this. It does work better, the selected format becomes

0x35315258 = LE '5', '1', 'R', 'X" = 'X', 'R', '1', '5', =
DRM_FORMAT_XRGB1555.

But the display is flickering like crazy so the updating frequency is
totally off, which is because it does not scale down the resolution,
the print used to be:

Console: switching to colour frame buffer device 80x30
drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device

It is now:

Console: switching to colour frame buffer device 100x37
drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device

100x37! (i.e. 800x296), this display can only do 640x240.
Any idea what else is going wrong here? Or is this another regression
on top of the first regression ... I was under the impression that
your change was only about formats not resolutions.

Yours,
Linus Walleij

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Daniel Vetter]

On Fri, May 12, 2023 at 04:11:48PM +0200, Thomas Zimmermann wrote:
> Hi
> 
> Am 12.05.23 um 15:20 schrieb Linus Walleij:
> > Sorry for late regression detection but this patch regresses
> > the Integrator AB IMPD-1 graphics, I bisected down to this
> > patch.
> [...]
> > This is the driver:
> > drivers/gpu/drm/pl111/pl111_versatile.c
> > with the pl110_impd1 variant, so these are the supported modes:
> > 
> > /* PL110 pixel formats for Integrator, vanilla PL110 */
> > static const u32 pl110_integrator_pixel_formats[] = {
> >          DRM_FORMAT_ABGR8888,
> >          DRM_FORMAT_XBGR8888,
> >          DRM_FORMAT_ARGB8888,
> >          DRM_FORMAT_XRGB8888,
> >          DRM_FORMAT_ABGR1555,
> >          DRM_FORMAT_XBGR1555,
> >          DRM_FORMAT_ARGB1555,
> >          DRM_FORMAT_XRGB1555,
> > };
> > (...)
> > /*
> >   * The IM-PD1 variant is a PL110 with a bunch of broken, or not
> >   * yet implemented features
> >   */
> > static const struct pl111_variant_data pl110_impd1 = {
> >          .name = "PL110 IM-PD1",
> >          .is_pl110 = true,
> >          .broken_clockdivider = true,
> >          .broken_vblank = true,
> >          .formats = pl110_integrator_pixel_formats,
> >          .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
> >          .fb_bpp = 16,
> > };
> > 
> > Notice the absence of RGB565!
> > Then we initialized the frambuffer like this:
> > 
> >          drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);
> > 
> > And as you see priv->variant->fb_bpp will be 16, so we want some
> > 16bpp mode however the only supported depth is 15 (the 1555 modes)
> > so it would use that by scaling back depth to 15.
> > 
> > However after this patch that doesn't work anymore.
> > 
> > Any hints on how we can fix this?
> 
> According to a quick grep for fb_bpp, it's only used for the call to
> drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for the
> models without rgb565. The switch at [1] should then pick the correct
> values.
> 
> The preferred_bpp parameter had a change in semantics. It used to be the
> number of bits per pixel. But that makes it hard to select between RGB1555
> and RGB565.  So it's now a special color-mode value that works like the
> kernel's video= parameter. Values of 15 and 32 are different from the rest.
> That switch at [1] explains it. Maybe you should rename fb_bpp to color_mode
> for clarity.
> 
> Let me know if this helps.

Shouldn't the helpers try to do this automatically? I think they kinda did
that in the past in some limited circumstances like this ...
-Daniel

> 
> Best regards
> Thomas
> 
> [1] https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L1827
> 
> > 
> > Yours,
> > Linus Walleij
> 
> -- 
> Thomas Zimmermann
> Graphics Driver Developer
> SUSE Software Solutions Germany GmbH
> Frankenstrasse 146, 90461 Nuernberg, Germany
> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
> HRB 36809 (AG Nuernberg)




-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 2255 bytes --]

Hi

Am 15.05.23 um 10:01 schrieb Linus Walleij:
> On Fri, May 12, 2023 at 4:11 PM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> 
>>> Any hints on how we can fix this?
>>
>> According to a quick grep for fb_bpp, it's only used for the call to
>> drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for
>> the models without rgb565. The switch at [1] should then pick the
>> correct values.
> 
> I tried this. It does work better, the selected format becomes
> 
> 0x35315258 = LE '5', '1', 'R', 'X" = 'X', 'R', '1', '5', =
> DRM_FORMAT_XRGB1555.

That's good news.

> 
> But the display is flickering like crazy so the updating frequency is
> totally off, which is because it does not scale down the resolution,
> the print used to be:
> 
> Console: switching to colour frame buffer device 80x30
> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> 
> It is now:
> 
> Console: switching to colour frame buffer device 100x37
> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> 
> 100x37! (i.e. 800x296), this display can only do 640x240.
> Any idea what else is going wrong here? Or is this another regression
> on top of the first regression ... I was under the impression that
> your change was only about formats not resolutions.

If your display only supports 640x240, you should filter out all the 
other modes in the driver, if necessary. To me, that seems fix-worthy in 
any case.

It's important that the kernel's console is always available. We had 
reports where the display remained dark, which is not acceptable for the 
console. Therefore our fbdev helpers now try harder to a find a working 
display setup. It could be that this interferes with your driver somehow.

As you say, this patch is only about selecting a color format. There's 
probably another regression elsewhere.

Can you try to bisect again with your current fix on top?

Best regards
Thomas

> 
> Yours,
> Linus Walleij

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 4339 bytes --]

Hi

Am 15.05.23 um 10:16 schrieb Daniel Vetter:
> On Fri, May 12, 2023 at 04:11:48PM +0200, Thomas Zimmermann wrote:
>> Hi
>>
>> Am 12.05.23 um 15:20 schrieb Linus Walleij:
>>> Sorry for late regression detection but this patch regresses
>>> the Integrator AB IMPD-1 graphics, I bisected down to this
>>> patch.
>> [...]
>>> This is the driver:
>>> drivers/gpu/drm/pl111/pl111_versatile.c
>>> with the pl110_impd1 variant, so these are the supported modes:
>>>
>>> /* PL110 pixel formats for Integrator, vanilla PL110 */
>>> static const u32 pl110_integrator_pixel_formats[] = {
>>>           DRM_FORMAT_ABGR8888,
>>>           DRM_FORMAT_XBGR8888,
>>>           DRM_FORMAT_ARGB8888,
>>>           DRM_FORMAT_XRGB8888,
>>>           DRM_FORMAT_ABGR1555,
>>>           DRM_FORMAT_XBGR1555,
>>>           DRM_FORMAT_ARGB1555,
>>>           DRM_FORMAT_XRGB1555,
>>> };
>>> (...)
>>> /*
>>>    * The IM-PD1 variant is a PL110 with a bunch of broken, or not
>>>    * yet implemented features
>>>    */
>>> static const struct pl111_variant_data pl110_impd1 = {
>>>           .name = "PL110 IM-PD1",
>>>           .is_pl110 = true,
>>>           .broken_clockdivider = true,
>>>           .broken_vblank = true,
>>>           .formats = pl110_integrator_pixel_formats,
>>>           .nformats = ARRAY_SIZE(pl110_integrator_pixel_formats),
>>>           .fb_bpp = 16,
>>> };
>>>
>>> Notice the absence of RGB565!
>>> Then we initialized the frambuffer like this:
>>>
>>>           drm_fbdev_dma_setup(drm, priv->variant->fb_bpp);
>>>
>>> And as you see priv->variant->fb_bpp will be 16, so we want some
>>> 16bpp mode however the only supported depth is 15 (the 1555 modes)
>>> so it would use that by scaling back depth to 15.
>>>
>>> However after this patch that doesn't work anymore.
>>>
>>> Any hints on how we can fix this?
>>
>> According to a quick grep for fb_bpp, it's only used for the call to
>> drm_fbdev_dma_setup(), right? In this case, you should set it to 15 for the
>> models without rgb565. The switch at [1] should then pick the correct
>> values.
>>
>> The preferred_bpp parameter had a change in semantics. It used to be the
>> number of bits per pixel. But that makes it hard to select between RGB1555
>> and RGB565.  So it's now a special color-mode value that works like the
>> kernel's video= parameter. Values of 15 and 32 are different from the rest.
>> That switch at [1] explains it. Maybe you should rename fb_bpp to color_mode
>> for clarity.
>>
>> Let me know if this helps.
> 
> Shouldn't the helpers try to do this automatically? I think they kinda did
> that in the past in some limited circumstances like this ...

That was the intention, but it never really worked at all. IIRC the 
color-format selection mixed up depth and bpp values freely. Factor in 
the command-line override (video=@bpp) and some odd case has always been 
broken.

So fbdev emulation now mostly uses the color-mode value that works as on 
the command line. The current semantics is:

  * select the user-given color mode, if non-zero
  * select the driver-given color mode, if non-zero
  * otherwise select a color mode of 32 by default (that's XRGB8888)

And later during fb_probe:

  * try the selected color mode
  * otherwise try to auto-detect if the selected color mode doesn't work,
  * otherwise use XRGB8888 as a last resort

That nicely splits the code into color-mode selection and color-format 
selection. And all the public interfaces (command line, 
drm_fbdev_{}_setup(), etc) use the same semantics, which is the color mode.

Best regards
Thomas

> -Daniel
> 
>>
>> Best regards
>> Thomas
>>
>> [1] https://elixir.bootlin.com/linux/v6.4-rc1/source/drivers/gpu/drm/drm_fb_helper.c#L1827
>>
>>>
>>> Yours,
>>> Linus Walleij
>>
>> -- 
>> Thomas Zimmermann
>> Graphics Driver Developer
>> SUSE Software Solutions Germany GmbH
>> Frankenstrasse 146, 90461 Nuernberg, Germany
>> GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
>> HRB 36809 (AG Nuernberg)
> 
> 
> 
> 

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linus Walleij]

On Mon, May 15, 2023 at 10:17 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
> Am 15.05.23 um 10:01 schrieb Linus Walleij:

> > But the display is flickering like crazy so the updating frequency is
> > totally off, which is because it does not scale down the resolution,
> > the print used to be:
> >
> > Console: switching to colour frame buffer device 80x30
> > drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> >
> > It is now:
> >
> > Console: switching to colour frame buffer device 100x37
> > drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
> >
> > 100x37! (i.e. 800x296), this display can only do 640x240.
> > Any idea what else is going wrong here? Or is this another regression
> > on top of the first regression ... I was under the impression that
> > your change was only about formats not resolutions.
>
> If your display only supports 640x240, you should filter out all the
> other modes in the driver, if necessary. To me, that seems fix-worthy in
> any case.

I think I found this, the bandwidth limit calculation in
drivers/gpu/drm/pl111/pl111_display.c was using the bpp from
the config and this was decreased from 16 to 15 and as it determined
cpp by dividing bpp/8 this decreased from 2 bytes to 1 byte.

Testing with DIV_ROUND_UP() in combination with the previous
fix!

Yours,
Linus Walleij

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Thomas Zimmermann]

[-- Attachment #1.1: Type: text/plain, Size: 1998 bytes --]

Hi

Am 15.05.23 um 10:59 schrieb Linus Walleij:
> On Mon, May 15, 2023 at 10:17 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:
>> Am 15.05.23 um 10:01 schrieb Linus Walleij:
> 
>>> But the display is flickering like crazy so the updating frequency is
>>> totally off, which is because it does not scale down the resolution,
>>> the print used to be:
>>>
>>> Console: switching to colour frame buffer device 80x30
>>> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
>>>
>>> It is now:
>>>
>>> Console: switching to colour frame buffer device 100x37
>>> drm-clcd-pl111 c1000000.display: [drm] fb0: pl111drmfb frame buffer device
>>>
>>> 100x37! (i.e. 800x296), this display can only do 640x240.
>>> Any idea what else is going wrong here? Or is this another regression
>>> on top of the first regression ... I was under the impression that
>>> your change was only about formats not resolutions.
>>
>> If your display only supports 640x240, you should filter out all the
>> other modes in the driver, if necessary. To me, that seems fix-worthy in
>> any case.
> 
> I think I found this, the bandwidth limit calculation in
> drivers/gpu/drm/pl111/pl111_display.c was using the bpp from
> the config and this was decreased from 16 to 15 and as it determined
> cpp by dividing bpp/8 this decreased from 2 bytes to 1 byte.
> 
> Testing with DIV_ROUND_UP() in combination with the previous
> fix!

Great. It's the code in mode_valid, right? That fix should be good 
enough for now. In the long term, we could make some of the internal 
fb-helper code available to drivers. You'd then be able to use it to get 
the depth/bpp values for the color mode.

Best regards
Thomas

> 
> Yours,
> Linus Walleij

-- 
Thomas Zimmermann
Graphics Driver Developer
SUSE Software Solutions Germany GmbH
Frankenstrasse 146, 90461 Nuernberg, Germany
GF: Ivo Totev, Andrew Myers, Andrew McDonald, Boudien Moerman
HRB 36809 (AG Nuernberg)

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

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linus Walleij]

On Mon, May 15, 2023 at 11:26 AM Thomas Zimmermann <tzimmermann@suse.de> wrote:

> > I think I found this, the bandwidth limit calculation in
> > drivers/gpu/drm/pl111/pl111_display.c was using the bpp from
> > the config and this was decreased from 16 to 15 and as it determined
> > cpp by dividing bpp/8 this decreased from 2 bytes to 1 byte.
> >
> > Testing with DIV_ROUND_UP() in combination with the previous
> > fix!
>
> Great. It's the code in mode_valid, right? That fix should be good
> enough for now. In the long term, we could make some of the internal
> fb-helper code available to drivers. You'd then be able to use it to get
> the depth/bpp values for the color mode.

Yeah I sent a patch, check it out!

Yours,
Linus Walleij

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Link: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]

On 14.05.23 14:10, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 12.05.23 15:20, Linus Walleij wrote:
>> Sorry for late regression detection but this patch regresses
>> the Integrator AB IMPD-1 graphics, I bisected down to this
>> patch.
> 
> #regzbot ^introduced 37c90d589dc
> #regzbot title drm/fb-helper: downscaling apparently stopped to work
> with pl110_impd1
> #regzbot ignore-activity

#regzbot monitor:
https://lore.kernel.org/all/20230515092943.1401558-1-linus.walleij@linaro.org/
#regzbot fix: drm/pl111: Fix FB depth on IMPD-1 framebuffer
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.

Re: [PATCH v3 11/13] drm/fb-helper: Fix single-probe color-format selection

[Linux regression tracking #update (Thorsten Leemhuis)]

[TLDR: This mail in primarily relevant for Linux regression tracking. A
change or fix related to the regression discussed in this thread was
posted or applied, but it did not use a Link: tag to point to the
report, as Linus and the documentation call for. Things happen, no
worries -- but now the regression tracking bot needs to be told manually
about the fix. See link in footer if these mails annoy you.]

On 14.05.23 14:10, Linux regression tracking #adding (Thorsten Leemhuis)
wrote:
> On 12.05.23 15:20, Linus Walleij wrote:
>> Sorry for late regression detection but this patch regresses
>> the Integrator AB IMPD-1 graphics, I bisected down to this
>> patch.
> 
> #regzbot ^introduced 37c90d589dc
> #regzbot title drm/fb-helper: downscaling apparently stopped to work
> with pl110_impd1
> #regzbot ignore-activity

#regzbot monitor:
https://lore.kernel.org/all/20230515092943.1401558-1-linus.walleij@linaro.org/
#regzbot fix: drm/pl111: Fix FB depth on IMPD-1 framebuffer
#regzbot ignore-activity

Ciao, Thorsten (wearing his 'the Linux kernel's regression tracker' hat)
--
Everything you wanna know about Linux kernel regression tracking:
https://linux-regtracking.leemhuis.info/about/#tldr
That page also explains what to do if mails like this annoy you.