[PATCH RESEND v2 0/8] drm/vkms: Add support for multiple plane formats

[PATCH RESEND v2 0/8] drm/vkms: Add support for multiple plane formats

[Louis Chauvet]

This series introduce a macro to generate a function to read simple
formats. It avoid duplication of the same logic for similar formats.

PATCH 1 is the introduction of the macro and adaptation of the existing
code to avoid duplication
PATCH 2-6 introduce new formats with the help of this macro.

This series must be applied on top of [1].

[1]: https://lore.kernel.org/all/20240809-yuv-v10-0-1a7c764166f7@bootlin.com/ 

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v2:
- Add proper casting/type to __le16 when needed to avoid warnings with 
  sparse
- Change the function argb_u16_from_yuv8888 to argb_u16_from_yuv161616 to 
  support 16 bits values.
- Add support for P010/P012/P016 format
- Link to v1: https://lore.kernel.org/r/20240516-b4-new-color-formats-v1-0-74cf9fe07317@bootlin.com

---
Louis Chauvet (8):
      drm/vkms: Create helpers macro to avoid code duplication in format  callbacks
      drm/vkms: Add support for ARGB8888 formats
      drm/vkms: Add support for ARGB16161616 formats
      drm/vkms: Add support for RGB565 formats
      drm/vkms: Add support for RGB888 formats
      drm/vkms: Change YUV helpers to support u16 inputs for conversion
      drm/vkms: Create helper macro for YUV formats
      drm/vkms: Add P01* formats

 drivers/gpu/drm/vkms/tests/vkms_format_test.c |   3 +-
 drivers/gpu/drm/vkms/vkms_formats.c           | 320 ++++++++++++++------------
 drivers/gpu/drm/vkms/vkms_formats.h           |   4 +-
 drivers/gpu/drm/vkms/vkms_plane.c             |  14 ++
 4 files changed, 195 insertions(+), 146 deletions(-)
---
base-commit: 82fe69e63d2b5a5e86ea94c7361c833d3848ab69
change-id: 20240312-b4-new-color-formats-1be9d688b21a
prerequisite-message-id: <20241007-yuv-v12-0-01c1ada6fec8@bootlin.com>
prerequisite-patch-id: ae2d8b2efbbaa9decce56632c498c87e708288b3
prerequisite-patch-id: d1e73379a15c5062924cf2dc8619676e13f35a13
prerequisite-patch-id: 2eed29b53617ba76169e1af303e4899d517a3a18
prerequisite-patch-id: 3f84c6e64b3a25510e929914e97ae2549451707c
prerequisite-patch-id: 82523a917646793deeec7cdcc7ff286bd924fd21
prerequisite-patch-id: dda6bf4692cd1795c489ff58e72c0841ea8ffbc4
prerequisite-patch-id: a0639eb773bf58c2ffe76f2567a8c74b6275092c
prerequisite-patch-id: 7a63d245a377d5f5283f48e8f52421b912811752
prerequisite-patch-id: deda292af6d8bbf6762b0bf4d351ffd2225995d8
prerequisite-patch-id: 6c2aa2645c7d854951608aa4d15a02e076abe1fe
prerequisite-patch-id: 11ae7be077ce7022f61101d41a9ba79b98efb273
prerequisite-patch-id: 18554f49b53cbcfd4a8ca50dc83b17dd3cf96474
prerequisite-patch-id: dc61c6d3db73053fc36e115af561e0c42b467de2
prerequisite-patch-id: 43f37e9c1bc041d491e41dfb59548ed258a1e071
prerequisite-patch-id: 5633292e10132d29be2467812e6e2e824cfedb67

Best regards,
-- 
Louis Chauvet <louis.chauvet@bootlin.com>

[PATCH RESEND v2 1/8] drm/vkms: Create helpers macro to avoid code duplication in format callbacks

[Louis Chauvet]

The callback functions for line conversion are almost identical for
some format. The generic READ_LINE macro generate all the required
boilerplate to process a line.

Two overrides of this macro have been added to avoid duplication of
the same arguments every time.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 163 +++++++++++++-----------------------
 1 file changed, 58 insertions(+), 105 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index f9841b8000c4..8f1bcca38148 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -292,6 +292,58 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
 }
 EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
 
+/**
+ * READ_LINE() - Generic generator for a read_line function which can be used for format with one
+ * plane and a block_h == block_w == 1.
+ *
+ * @function_name: Function name to generate
+ * @pixel_name: temporary pixel name used in the @__VA_ARGS__ parameters
+ * @pixel_type: Used to specify the type you want to cast the pixel pointer
+ * @callback: Callback to call for each pixels. This fonction should take @__VA_ARGS__ as parameter
+ *            and return a pixel_argb_u16
+ * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_name to access current
+ *  pixel.
+ */
+#define READ_LINE(function_name, pixel_name, pixel_type, callback, ...)				\
+static void function_name(const struct vkms_plane_state *plane, int x_start,			\
+			      int y_start, enum pixel_read_direction direction, int count,	\
+			      struct pixel_argb_u16 out_pixel[])				\
+{												\
+	struct pixel_argb_u16 *end = out_pixel + count;						\
+	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
+	u8 *src_pixels;										\
+												\
+	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);		\
+												\
+	while (out_pixel < end) {								\
+		pixel_type *(pixel_name) = (pixel_type *)src_pixels;				\
+		*out_pixel = (callback)(__VA_ARGS__);						\
+		out_pixel += 1;									\
+		src_pixels += step;								\
+	}											\
+}
+
+/**
+ * READ_LINE_ARGB8888() - Generic generator for ARGB8888 formats.
+ * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
+ *
+ * @function_name: Function name to generate
+ * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
+ * @a, @r, @g, @b: value of each channel
+ */
+#define READ_LINE_ARGB8888(function_name, pixel_name, a, r, g, b) \
+	READ_LINE(function_name, pixel_name, u8, argb_u16_from_u8888, a, r, g, b)
+/**
+ * READ_LINE_ARGB16161616() - Generic generator for ARGB16161616 formats.
+ * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
+ *
+ * @function_name: Function name to generate
+ * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
+ * @a, @r, @g, @b: value of each channel
+ */
+#define READ_LINE_16161616(function_name, pixel_name, a, r, g, b) \
+	READ_LINE(function_name, pixel_name, u16, argb_u16_from_u16161616, a, r, g, b)
+
 /*
  * The following functions are read_line function for each pixel format supported by VKMS.
  *
@@ -378,118 +430,19 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
 	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
 }
 
-static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
-			 int y_start, enum pixel_read_direction direction, int count,
-			 struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
-
-	while (out_pixel < end) {
-		*out_pixel = argb_u16_from_gray8(*src_pixels);
-		src_pixels += step;
-		out_pixel += 1;
-	}
-}
-
-static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
-			       enum pixel_read_direction direction, int count,
-			       struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
-
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
-
-	while (out_pixel < end) {
-		u8 *px = (u8 *)src_pixels;
-		*out_pixel = argb_u16_from_u8888(px[3], px[2], px[1], px[0]);
-		out_pixel += 1;
-		src_pixels += step;
-	}
-}
-
-static void XRGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
-			       enum pixel_read_direction direction, int count,
-			       struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
-
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
-
-	while (out_pixel < end) {
-		u8 *px = (u8 *)src_pixels;
-		*out_pixel = argb_u16_from_u8888(255, px[2], px[1], px[0]);
-		out_pixel += 1;
-		src_pixels += step;
-	}
-}
-
-static void ARGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
-				   int y_start, enum pixel_read_direction direction, int count,
-				   struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
-
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
-
-	while (out_pixel < end) {
-		u16 *px = (u16 *)src_pixels;
-		*out_pixel = argb_u16_from_u16161616(px[3], px[2], px[1], px[0]);
-		out_pixel += 1;
-		src_pixels += step;
-	}
-}
-
-static void XRGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
-				   int y_start, enum pixel_read_direction direction, int count,
-				   struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
 
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
 
-	while (out_pixel < end) {
-		__le16 *px = (__le16 *)src_pixels;
-		*out_pixel = argb_u16_from_le16161616(cpu_to_le16(0xFFFF), px[2], px[1], px[0]);
-		out_pixel += 1;
-		src_pixels += step;
-	}
-}
+READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
 
-static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
-			     int y_start, enum pixel_read_direction direction, int count,
-			     struct pixel_argb_u16 out_pixel[])
-{
-	struct pixel_argb_u16 *end = out_pixel + count;
-	u8 *src_pixels;
 
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
+READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
+READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
 
-	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
 
-	while (out_pixel < end) {
-		__le16 *px = (__le16 *)src_pixels;
+READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
 
-		*out_pixel = argb_u16_from_RGB565(px);
-		out_pixel += 1;
-		src_pixels += step;
-	}
-}
 
 /*
  * This callback can be used for YUV formats where U and V values are

-- 
2.46.2

[PATCH RESEND v2 2/8] drm/vkms: Add support for ARGB8888 formats

[Louis Chauvet]

The formats XRGB8888 and ARGB8888 were already supported.
Add the support for:
- XBGR8888
- RGBX8888
- BGRX8888
- ABGR8888
- RGBA8888
- BGRA8888

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 18 ++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_plane.c   |  6 ++++++
 2 files changed, 24 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 8f1bcca38148..b5a38f70c62b 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -432,8 +432,14 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
 
 
 READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
+READ_LINE_ARGB8888(XBGR8888_read_line, px, 255, px[0], px[1], px[2])
+READ_LINE_ARGB8888(RGBX8888_read_line, px, 255, px[3], px[2], px[1])
+READ_LINE_ARGB8888(BGRX8888_read_line, px, 255, px[1], px[2], px[3])
 
 READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
+READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
+READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
+READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
 
 
 READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
@@ -637,8 +643,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 	switch (format) {
 	case DRM_FORMAT_ARGB8888:
 		return &ARGB8888_read_line;
+	case DRM_FORMAT_ABGR8888:
+		return &ABGR8888_read_line;
+	case DRM_FORMAT_BGRA8888:
+		return &BGRA8888_read_line;
+	case DRM_FORMAT_RGBA8888:
+		return &RGBA8888_read_line;
 	case DRM_FORMAT_XRGB8888:
 		return &XRGB8888_read_line;
+	case DRM_FORMAT_XBGR8888:
+		return &XBGR8888_read_line;
+	case DRM_FORMAT_RGBX8888:
+		return &RGBX8888_read_line;
+	case DRM_FORMAT_BGRX8888:
+		return &BGRX8888_read_line;
 	case DRM_FORMAT_ARGB16161616:
 		return &ARGB16161616_read_line;
 	case DRM_FORMAT_XRGB16161616:
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 67f891e7ac58..941a6e92a040 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -14,7 +14,13 @@
 
 static const u32 vkms_formats[] = {
 	DRM_FORMAT_ARGB8888,
+	DRM_FORMAT_ABGR8888,
+	DRM_FORMAT_BGRA8888,
+	DRM_FORMAT_RGBA8888,
 	DRM_FORMAT_XRGB8888,
+	DRM_FORMAT_XBGR8888,
+	DRM_FORMAT_RGBX8888,
+	DRM_FORMAT_BGRX8888,
 	DRM_FORMAT_XRGB16161616,
 	DRM_FORMAT_ARGB16161616,
 	DRM_FORMAT_RGB565,

-- 
2.46.2

[PATCH RESEND v2 3/8] drm/vkms: Add support for ARGB16161616 formats

[Louis Chauvet]

The formats XRGB16161616 and ARGB16161616 were already supported.
Add the support for:
- ABGR16161616
- XBGR16161616

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 11 ++++++++---
 drivers/gpu/drm/vkms/vkms_plane.c   |  2 ++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index b5a38f70c62b..c03a481f5005 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -441,9 +441,10 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
 READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
 READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
 
-
-READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
-READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
+READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
+READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
+READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
+READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])
 
 READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
 
@@ -659,8 +660,12 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 		return &BGRX8888_read_line;
 	case DRM_FORMAT_ARGB16161616:
 		return &ARGB16161616_read_line;
+	case DRM_FORMAT_ABGR16161616:
+		return &ABGR16161616_read_line;
 	case DRM_FORMAT_XRGB16161616:
 		return &XRGB16161616_read_line;
+	case DRM_FORMAT_XBGR16161616:
+		return &XBGR16161616_read_line;
 	case DRM_FORMAT_RGB565:
 		return &RGB565_read_line;
 	case DRM_FORMAT_NV12:
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 941a6e92a040..1e971c7760d9 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -22,7 +22,9 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_RGBX8888,
 	DRM_FORMAT_BGRX8888,
 	DRM_FORMAT_XRGB16161616,
+	DRM_FORMAT_XBGR16161616,
 	DRM_FORMAT_ARGB16161616,
+	DRM_FORMAT_ABGR16161616,
 	DRM_FORMAT_RGB565,
 	DRM_FORMAT_NV12,
 	DRM_FORMAT_NV16,

-- 
2.46.2

[PATCH RESEND v2 4/8] drm/vkms: Add support for RGB565 formats

[Louis Chauvet]

The format RGB565 was already supported. Add the support for:
- BGR565

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 25 ++++++++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_plane.c   |  1 +
 2 files changed, 25 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index c03a481f5005..e34bea5da752 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -249,7 +249,7 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
 	return out_pixel;
 }
 
-static struct pixel_argb_u16 argb_u16_from_gray8(u16 gray)
+static struct pixel_argb_u16 argb_u16_from_gray8(u8 gray)
 {
 	return argb_u16_from_u8888(255, gray, gray, gray);
 }
@@ -259,6 +259,26 @@ static struct pixel_argb_u16 argb_u16_from_grayu16(u16 gray)
 	return argb_u16_from_u16161616(0xFFFF, gray, gray, gray);
 }
 
+static struct pixel_argb_u16 argb_u16_from_BGR565(const __le16 *pixel)
+{
+	struct pixel_argb_u16 out_pixel;
+
+	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
+	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
+
+	u16 rgb_565 = le16_to_cpu(*pixel);
+	s64 fp_b = drm_int2fixp((rgb_565 >> 11) & 0x1f);
+	s64 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
+	s64 fp_r = drm_int2fixp(rgb_565 & 0x1f);
+
+	out_pixel.a = (u16)0xffff;
+	out_pixel.b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
+	out_pixel.g = drm_fixp2int_round(drm_fixp_mul(fp_g, fp_g_ratio));
+	out_pixel.r = drm_fixp2int_round(drm_fixp_mul(fp_r, fp_rb_ratio));
+
+	return out_pixel;
+}
+
 VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
 							    const struct conversion_matrix *matrix)
 {
@@ -447,6 +467,7 @@ READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
 READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])
 
 READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
+READ_LINE(BGR565_read_line, px, __le16, argb_u16_from_BGR565, px)
 
 READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
 
@@ -668,6 +689,8 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 		return &XBGR16161616_read_line;
 	case DRM_FORMAT_RGB565:
 		return &RGB565_read_line;
+	case DRM_FORMAT_BGR565:
+		return &BGR565_read_line;
 	case DRM_FORMAT_NV12:
 	case DRM_FORMAT_NV16:
 	case DRM_FORMAT_NV24:
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 1e971c7760d9..a243a706459f 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -26,6 +26,7 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_ARGB16161616,
 	DRM_FORMAT_ABGR16161616,
 	DRM_FORMAT_RGB565,
+	DRM_FORMAT_BGR565,
 	DRM_FORMAT_NV12,
 	DRM_FORMAT_NV16,
 	DRM_FORMAT_NV24,

-- 
2.46.2

[PATCH RESEND v2 5/8] drm/vkms: Add support for RGB888 formats

[Louis Chauvet]

Add the support for:
- RGB888
- BGR888

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 7 +++++++
 drivers/gpu/drm/vkms/vkms_plane.c   | 2 ++
 2 files changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index e34bea5da752..2376ea8661ac 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -461,6 +461,9 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
 READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
 READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
 
+READ_LINE_ARGB8888(RGB888_read_line, px, 255, px[2], px[1], px[0])
+READ_LINE_ARGB8888(BGR888_read_line, px, 255, px[0], px[1], px[2])
+
 READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
 READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
 READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
@@ -679,6 +682,10 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 		return &RGBX8888_read_line;
 	case DRM_FORMAT_BGRX8888:
 		return &BGRX8888_read_line;
+	case DRM_FORMAT_RGB888:
+		return RGB888_read_line;
+	case DRM_FORMAT_BGR888:
+		return BGR888_read_line;
 	case DRM_FORMAT_ARGB16161616:
 		return &ARGB16161616_read_line;
 	case DRM_FORMAT_ABGR16161616:
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index a243a706459f..0fa589abc53a 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -21,6 +21,8 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XBGR8888,
 	DRM_FORMAT_RGBX8888,
 	DRM_FORMAT_BGRX8888,
+	DRM_FORMAT_RGB888,
+	DRM_FORMAT_BGR888,
 	DRM_FORMAT_XRGB16161616,
 	DRM_FORMAT_XBGR16161616,
 	DRM_FORMAT_ARGB16161616,

-- 
2.46.2

[PATCH RESEND v2 6/8] drm/vkms: Change YUV helpers to support u16 inputs for conversion

[Louis Chauvet]

Some YUV format uses 16 bit values, so change the helper function for
conversion to support those new formats.

Add support for the YUV format P010

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/tests/vkms_format_test.c |  3 ++-
 drivers/gpu/drm/vkms/vkms_formats.c           | 22 ++++++++++++----------
 drivers/gpu/drm/vkms/vkms_formats.h           |  4 ++--
 3 files changed, 16 insertions(+), 13 deletions(-)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
index 351409897ca3..e3a77954e6dc 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -188,7 +188,8 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
 		get_conversion_matrix_to_argb_u16
 			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
 
-		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);
+		argb = argb_u16_from_yuv161616(&matrix, color->yuv.y * 257, color->yuv.u * 257,
+					       color->yuv.v * 257);
 
 		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.a, color->argb.a), 257,
 				    "On the A channel of the color %s expected 0x%04x, got 0x%04x",
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 2376ea8661ac..95ff15051fb7 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -279,16 +279,17 @@ static struct pixel_argb_u16 argb_u16_from_BGR565(const __le16 *pixel)
 	return out_pixel;
 }
 
-VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
-							    const struct conversion_matrix *matrix)
+VISIBLE_IF_KUNIT
+struct pixel_argb_u16 argb_u16_from_yuv161616(const struct conversion_matrix *matrix,
+					      u16 y, u16 channel_1, u16 channel_2)
 {
 	u16 r, g, b;
 	s64 fp_y, fp_channel_1, fp_channel_2;
 	s64 fp_r, fp_g, fp_b;
 
-	fp_y = drm_int2fixp(((int)y - matrix->y_offset) * 257);
-	fp_channel_1 = drm_int2fixp(((int)channel_1 - 128) * 257);
-	fp_channel_2 = drm_int2fixp(((int)channel_2 - 128) * 257);
+	fp_y = drm_int2fixp((int)y - matrix->y_offset * 257);
+	fp_channel_1 = drm_int2fixp((int)channel_1 - 128 * 257);
+	fp_channel_2 = drm_int2fixp((int)channel_2 - 128 * 257);
 
 	fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
 	       drm_fixp_mul(matrix->matrix[0][1], fp_channel_1) +
@@ -310,7 +311,7 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
 
 	return argb_u16_from_u16161616(0xffff, r, g, b);
 }
-EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
+EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv161616);
 
 /**
  * READ_LINE() - Generic generator for a read_line function which can be used for format with one
@@ -505,8 +506,8 @@ static void semi_planar_yuv_read_line(const struct vkms_plane_state *plane, int
 	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
 
 	for (int i = 0; i < count; i++) {
-		*out_pixel = argb_u16_from_yuv888(y_plane[0], uv_plane[0], uv_plane[1],
-						  conversion_matrix);
+		*out_pixel = argb_u16_from_yuv161616(conversion_matrix, y_plane[0] * 257,
+						     uv_plane[0] * 257, uv_plane[1] * 257);
 		out_pixel += 1;
 		y_plane += step_y;
 		if ((i + subsampling_offset + 1) % subsampling == 0)
@@ -550,8 +551,9 @@ static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_sta
 	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
 
 	for (int i = 0; i < count; i++) {
-		*out_pixel = argb_u16_from_yuv888(*y_plane, *channel_1_plane, *channel_2_plane,
-						  conversion_matrix);
+		*out_pixel = argb_u16_from_yuv161616(conversion_matrix,
+						     *y_plane * 257, *channel_1_plane * 257,
+						     *channel_2_plane * 257);
 		out_pixel += 1;
 		y_plane += step_y;
 		if ((i + subsampling_offset + 1) % subsampling == 0) {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index b4fe62ab9c65..eeb208cdd6b1 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -14,8 +14,8 @@ void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encod
 				       struct conversion_matrix *matrix);
 
 #if IS_ENABLED(CONFIG_KUNIT)
-struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
-					   const struct conversion_matrix *matrix);
+struct pixel_argb_u16 argb_u16_from_yuv161616(const struct conversion_matrix *matrix,
+					      u16 y, u16 channel_1, u16 channel_2);
 #endif
 
 #endif /* _VKMS_FORMATS_H_ */

-- 
2.46.2

[PATCH RESEND v2 7/8] drm/vkms: Create helper macro for YUV formats

[Louis Chauvet]

The callback functions for line conversion are almost identical for
semi-planar formats. The generic READ_LINE_YUV_SEMIPLANAR macro
generate all the required boilerplate to process a line from a
semi-planar format.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 75 ++++++++++++++++++++++++-------------
 1 file changed, 48 insertions(+), 27 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 95ff15051fb7..1cc52320475d 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -486,35 +486,56 @@ READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
  * - Convert YUV and YVU with the same function (a column swap is needed when setting up
  * plane->conversion_matrix)
  */
-static void semi_planar_yuv_read_line(const struct vkms_plane_state *plane, int x_start,
-				      int y_start, enum pixel_read_direction direction, int count,
-				      struct pixel_argb_u16 out_pixel[])
-{
-	u8 *y_plane;
-	u8 *uv_plane;
-
-	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,
-			       &y_plane);
-	packed_pixels_addr_1x1(plane->frame_info,
-			       x_start / plane->frame_info->fb->format->hsub,
-			       y_start / plane->frame_info->fb->format->vsub, 1,
-			       &uv_plane);
-	int step_y = get_block_step_bytes(plane->frame_info->fb, direction, 0);
-	int step_uv = get_block_step_bytes(plane->frame_info->fb, direction, 1);
-	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);
-	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);
-	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
 
-	for (int i = 0; i < count; i++) {
-		*out_pixel = argb_u16_from_yuv161616(conversion_matrix, y_plane[0] * 257,
-						     uv_plane[0] * 257, uv_plane[1] * 257);
-		out_pixel += 1;
-		y_plane += step_y;
-		if ((i + subsampling_offset + 1) % subsampling == 0)
-			uv_plane += step_uv;
-	}
+/**
+ * READ_LINE_YUV_SEMIPLANAR() - Generic generator for a read_line function which can be used for yuv
+ * formats with two planes and block_w == block_h == 1.
+ *
+ * @function_name: Function name to generate
+ * @pixel_1_name: temporary pixel name for the first plane used in the @__VA_ARGS__ parameters
+ * @pixel_2_name: temporary pixel name for the second plane used in the @__VA_ARGS__ parameters
+ * @pixel_1_type: Used to specify the type you want to cast the pixel pointer on the plane 1
+ * @pixel_2_type: Used to specify the type you want to cast the pixel pointer on the plane 2
+ * @callback: Callback to call for each pixels. This function should take
+ *            (struct conversion_matrix*, @__VA_ARGS__) as parameter and return a pixel_argb_u16
+ * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_1_name and @pixel_2_name
+ *               to access current pixel values
+ */
+#define READ_LINE_YUV_SEMIPLANAR(function_name, pixel_1_name, pixel_2_name, pixel_1_type,	\
+				 pixel_2_type, callback, ...)					\
+static void function_name(const struct vkms_plane_state *plane, int x_start,			\
+		 int y_start, enum pixel_read_direction direction, int count,			\
+		 struct pixel_argb_u16 out_pixel[])						\
+{												\
+	u8 *plane_1;										\
+	u8 *plane_2;										\
+												\
+	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,				\
+			       &plane_1);							\
+	packed_pixels_addr_1x1(plane->frame_info,						\
+			       x_start / plane->frame_info->fb->format->hsub,			\
+			       y_start / plane->frame_info->fb->format->vsub, 1,		\
+			       &plane_2);							\
+	int step_1 = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
+	int step_2 = get_block_step_bytes(plane->frame_info->fb, direction, 1);			\
+	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);		\
+	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);		\
+	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;		\
+												\
+	for (int i = 0; i < count; i++) {							\
+		pixel_1_type *(pixel_1_name) = (pixel_1_type *)plane_1;				\
+		pixel_2_type *(pixel_2_name) = (pixel_2_type *)plane_2;				\
+		*out_pixel = (callback)(conversion_matrix, __VA_ARGS__);			\
+		out_pixel += 1;									\
+		plane_1 += step_1;								\
+		if ((i + subsampling_offset + 1) % subsampling == 0)				\
+			plane_2 += step_2;							\
+	}											\
 }
 
+READ_LINE_YUV_SEMIPLANAR(YUV888_semiplanar_read_line, y, uv, u8, u8, argb_u16_from_yuv161616,
+			 y[0] * 257, uv[0] * 257, uv[1] * 257)
+
 /*
  * This callback can be used for YUV format where each color component is
  * stored in a different plane (often called planar formats). It will
@@ -706,7 +727,7 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 	case DRM_FORMAT_NV21:
 	case DRM_FORMAT_NV61:
 	case DRM_FORMAT_NV42:
-		return &semi_planar_yuv_read_line;
+		return &YUV888_semiplanar_read_line;
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YUV422:
 	case DRM_FORMAT_YUV444:

-- 
2.46.2

[PATCH RESEND v2 8/8] drm/vkms: Add P01* formats

[Louis Chauvet]

The formats NV 12/16/24/21/61/42 were already supported.
Add support for:
- P010
- P012
- P016

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 7 ++++++-
 drivers/gpu/drm/vkms/vkms_plane.c   | 3 +++
 2 files changed, 9 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 1cc52320475d..d77718d8e01d 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -535,7 +535,8 @@ static void function_name(const struct vkms_plane_state *plane, int x_start,			\
 
 READ_LINE_YUV_SEMIPLANAR(YUV888_semiplanar_read_line, y, uv, u8, u8, argb_u16_from_yuv161616,
 			 y[0] * 257, uv[0] * 257, uv[1] * 257)
-
+READ_LINE_YUV_SEMIPLANAR(YUV161616_semiplanar_read_line, y, uv, u16, u16, argb_u16_from_yuv161616,
+			 y[0], uv[0], uv[1])
 /*
  * This callback can be used for YUV format where each color component is
  * stored in a different plane (often called planar formats). It will
@@ -728,6 +729,10 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 	case DRM_FORMAT_NV61:
 	case DRM_FORMAT_NV42:
 		return &YUV888_semiplanar_read_line;
+	case DRM_FORMAT_P010:
+	case DRM_FORMAT_P012:
+	case DRM_FORMAT_P016:
+		return &YUV161616_semiplanar_read_line;
 	case DRM_FORMAT_YUV420:
 	case DRM_FORMAT_YUV422:
 	case DRM_FORMAT_YUV444:
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 0fa589abc53a..03716616f819 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -41,6 +41,9 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_YVU420,
 	DRM_FORMAT_YVU422,
 	DRM_FORMAT_YVU444,
+	DRM_FORMAT_P010,
+	DRM_FORMAT_P012,
+	DRM_FORMAT_P016,
 	DRM_FORMAT_R1,
 	DRM_FORMAT_R2,
 	DRM_FORMAT_R4,

-- 
2.46.2

Re: [PATCH RESEND v2 2/8] drm/vkms: Add support for ARGB8888 formats

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> The formats XRGB8888 and ARGB8888 were already supported.
> Add the support for:
> - XBGR8888
> - RGBX8888
> - BGRX8888
> - ABGR8888
> - RGBA8888
> - BGRA8888
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 18 ++++++++++++++++++
>   drivers/gpu/drm/vkms/vkms_plane.c   |  6 ++++++
>   2 files changed, 24 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 8f1bcca38148..b5a38f70c62b 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -432,8 +432,14 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
>   
>   
>   READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> +READ_LINE_ARGB8888(XBGR8888_read_line, px, 255, px[0], px[1], px[2]) > +READ_LINE_ARGB8888(RGBX8888_read_line, px, 255, px[3], px[2], px[1])

I'm not expert in colors, but is this correct? From what I understand,
it should be:

READ_LINE_ARGB8888(RGBX8888_read_line, px, px[2], px[1], px[0], 255)
                                            ^R     ^G     ^B     ^X

> +READ_LINE_ARGB8888(BGRX8888_read_line, px, 255, px[1], px[2], px[3])

Again, is this correct?

Best Regards,
- Maíra

>   
>   READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
> +READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
> +READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
> +READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
>   
>   
>   READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> @@ -637,8 +643,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>   	switch (format) {
>   	case DRM_FORMAT_ARGB8888:
>   		return &ARGB8888_read_line;
> +	case DRM_FORMAT_ABGR8888:
> +		return &ABGR8888_read_line;
> +	case DRM_FORMAT_BGRA8888:
> +		return &BGRA8888_read_line;
> +	case DRM_FORMAT_RGBA8888:
> +		return &RGBA8888_read_line;
>   	case DRM_FORMAT_XRGB8888:
>   		return &XRGB8888_read_line;
> +	case DRM_FORMAT_XBGR8888:
> +		return &XBGR8888_read_line;
> +	case DRM_FORMAT_RGBX8888:
> +		return &RGBX8888_read_line;
> +	case DRM_FORMAT_BGRX8888:
> +		return &BGRX8888_read_line;
>   	case DRM_FORMAT_ARGB16161616:
>   		return &ARGB16161616_read_line;
>   	case DRM_FORMAT_XRGB16161616:
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 67f891e7ac58..941a6e92a040 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -14,7 +14,13 @@
>   
>   static const u32 vkms_formats[] = {
>   	DRM_FORMAT_ARGB8888,
> +	DRM_FORMAT_ABGR8888,
> +	DRM_FORMAT_BGRA8888,
> +	DRM_FORMAT_RGBA8888,
>   	DRM_FORMAT_XRGB8888,
> +	DRM_FORMAT_XBGR8888,
> +	DRM_FORMAT_RGBX8888,
> +	DRM_FORMAT_BGRX8888,
>   	DRM_FORMAT_XRGB16161616,
>   	DRM_FORMAT_ARGB16161616,
>   	DRM_FORMAT_RGB565,
> 

Re: [PATCH RESEND v2 3/8] drm/vkms: Add support for ARGB16161616 formats

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> The formats XRGB16161616 and ARGB16161616 were already supported.
> Add the support for:
> - ABGR16161616
> - XBGR16161616
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 11 ++++++++---
>   drivers/gpu/drm/vkms/vkms_plane.c   |  2 ++
>   2 files changed, 10 insertions(+), 3 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index b5a38f70c62b..c03a481f5005 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -441,9 +441,10 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
>   READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
>   READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
>   
> -
> -READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> -READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);

Please, correct the error in the patch that introduced. Don't fix it in
an unrelated patch.

> +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
> +READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
> +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])

Please, be consistent in the use of decimal numbers and hexadecimal
number. If you prefer to use hexadecimal, don't use 255, use 0xFF.

> +READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])

Are you using tests to check the new formats?

Best Regards,
- Maíra

>   
>   READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
>   
> @@ -659,8 +660,12 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>   		return &BGRX8888_read_line;
>   	case DRM_FORMAT_ARGB16161616:
>   		return &ARGB16161616_read_line;
> +	case DRM_FORMAT_ABGR16161616:
> +		return &ABGR16161616_read_line;
>   	case DRM_FORMAT_XRGB16161616:
>   		return &XRGB16161616_read_line;
> +	case DRM_FORMAT_XBGR16161616:
> +		return &XBGR16161616_read_line;
>   	case DRM_FORMAT_RGB565:
>   		return &RGB565_read_line;
>   	case DRM_FORMAT_NV12:
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 941a6e92a040..1e971c7760d9 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -22,7 +22,9 @@ static const u32 vkms_formats[] = {
>   	DRM_FORMAT_RGBX8888,
>   	DRM_FORMAT_BGRX8888,
>   	DRM_FORMAT_XRGB16161616,
> +	DRM_FORMAT_XBGR16161616,
>   	DRM_FORMAT_ARGB16161616,
> +	DRM_FORMAT_ABGR16161616,
>   	DRM_FORMAT_RGB565,
>   	DRM_FORMAT_NV12,
>   	DRM_FORMAT_NV16,
> 

Re: [PATCH RESEND v2 4/8] drm/vkms: Add support for RGB565 formats

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> The format RGB565 was already supported. Add the support for:
> - BGR565
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 25 ++++++++++++++++++++++++-
>   drivers/gpu/drm/vkms/vkms_plane.c   |  1 +
>   2 files changed, 25 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index c03a481f5005..e34bea5da752 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -249,7 +249,7 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
>   	return out_pixel;
>   }
>   
> -static struct pixel_argb_u16 argb_u16_from_gray8(u16 gray)
> +static struct pixel_argb_u16 argb_u16_from_gray8(u8 gray)

Again, fix the issue in the patch that introduce it.

Best Regards,
- Maíra

>   {
>   	return argb_u16_from_u8888(255, gray, gray, gray);
>   }
> @@ -259,6 +259,26 @@ static struct pixel_argb_u16 argb_u16_from_grayu16(u16 gray)
>   	return argb_u16_from_u16161616(0xFFFF, gray, gray, gray);
>   }
>   
> +static struct pixel_argb_u16 argb_u16_from_BGR565(const __le16 *pixel)
> +{
> +	struct pixel_argb_u16 out_pixel;
> +
> +	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> +	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
> +
> +	u16 rgb_565 = le16_to_cpu(*pixel);
> +	s64 fp_b = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> +	s64 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> +	s64 fp_r = drm_int2fixp(rgb_565 & 0x1f);
> +
> +	out_pixel.a = (u16)0xffff;
> +	out_pixel.b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
> +	out_pixel.g = drm_fixp2int_round(drm_fixp_mul(fp_g, fp_g_ratio));
> +	out_pixel.r = drm_fixp2int_round(drm_fixp_mul(fp_r, fp_rb_ratio));
> +
> +	return out_pixel;
> +}
> +
>   VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
>   							    const struct conversion_matrix *matrix)
>   {
> @@ -447,6 +467,7 @@ READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
>   READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])
>   
>   READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
> +READ_LINE(BGR565_read_line, px, __le16, argb_u16_from_BGR565, px)
>   
>   READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
>   
> @@ -668,6 +689,8 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>   		return &XBGR16161616_read_line;
>   	case DRM_FORMAT_RGB565:
>   		return &RGB565_read_line;
> +	case DRM_FORMAT_BGR565:
> +		return &BGR565_read_line;
>   	case DRM_FORMAT_NV12:
>   	case DRM_FORMAT_NV16:
>   	case DRM_FORMAT_NV24:
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 1e971c7760d9..a243a706459f 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -26,6 +26,7 @@ static const u32 vkms_formats[] = {
>   	DRM_FORMAT_ARGB16161616,
>   	DRM_FORMAT_ABGR16161616,
>   	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_BGR565,
>   	DRM_FORMAT_NV12,
>   	DRM_FORMAT_NV16,
>   	DRM_FORMAT_NV24,
> 

Re: [PATCH RESEND v2 1/8] drm/vkms: Create helpers macro to avoid code duplication in format callbacks

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> The callback functions for line conversion are almost identical for
> some format. The generic READ_LINE macro generate all the required
> boilerplate to process a line.
> 
> Two overrides of this macro have been added to avoid duplication of
> the same arguments every time.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 163 +++++++++++++-----------------------
>   1 file changed, 58 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index f9841b8000c4..8f1bcca38148 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -292,6 +292,58 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
>   }
>   EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
>   
> +/**
> + * READ_LINE() - Generic generator for a read_line function which can be used for format with one
> + * plane and a block_h == block_w == 1.
> + *
> + * @function_name: Function name to generate > + * @pixel_name: temporary pixel name used in the @__VA_ARGS__ parameters

s/temporary/Temporary

> + * @pixel_type: Used to specify the type you want to cast the pixel pointer
> + * @callback: Callback to call for each pixels. This fonction should take @__VA_ARGS__ as parameter
> + *            and return a pixel_argb_u16
> + * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_name to access current
> + *  pixel.
> + */
> +#define READ_LINE(function_name, pixel_name, pixel_type, callback, ...)				\
> +static void function_name(const struct vkms_plane_state *plane, int x_start,			\
> +			      int y_start, enum pixel_read_direction direction, int count,	\
> +			      struct pixel_argb_u16 out_pixel[])				\
> +{												\
> +	struct pixel_argb_u16 *end = out_pixel + count;						\
> +	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
> +	u8 *src_pixels;										\
> +												\
> +	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);		\
> +												\
> +	while (out_pixel < end) {								\
> +		pixel_type *(pixel_name) = (pixel_type *)src_pixels;				\
> +		*out_pixel = (callback)(__VA_ARGS__);						\
> +		out_pixel += 1;									\
> +		src_pixels += step;								\
> +	}											\
> +}
> +
> +/**
> + * READ_LINE_ARGB8888() - Generic generator for ARGB8888 formats.
> + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> + *
> + * @function_name: Function name to generate
> + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> + * @a, @r, @g, @b: value of each channel
> + */
> +#define READ_LINE_ARGB8888(function_name, pixel_name, a, r, g, b) \
> +	READ_LINE(function_name, pixel_name, u8, argb_u16_from_u8888, a, r, g, b)
> +/**
> + * READ_LINE_ARGB16161616() - Generic generator for ARGB16161616 formats.
> + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.

s/u8/u16

Best Regard,
- Maíra

> + *
> + * @function_name: Function name to generate
> + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> + * @a, @r, @g, @b: value of each channel
> + */
> +#define READ_LINE_16161616(function_name, pixel_name, a, r, g, b) \
> +	READ_LINE(function_name, pixel_name, u16, argb_u16_from_u16161616, a, r, g, b)
> +
>   /*
>    * The following functions are read_line function for each pixel format supported by VKMS.
>    *
> @@ -378,118 +430,19 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
>   	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
>   }
>   
> -static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
> -			 int y_start, enum pixel_read_direction direction, int count,
> -			 struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	while (out_pixel < end) {
> -		*out_pixel = argb_u16_from_gray8(*src_pixels);
> -		src_pixels += step;
> -		out_pixel += 1;
> -	}
> -}
> -
> -static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> -			       enum pixel_read_direction direction, int count,
> -			       struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u8 *px = (u8 *)src_pixels;
> -		*out_pixel = argb_u16_from_u8888(px[3], px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void XRGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> -			       enum pixel_read_direction direction, int count,
> -			       struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u8 *px = (u8 *)src_pixels;
> -		*out_pixel = argb_u16_from_u8888(255, px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void ARGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> -				   int y_start, enum pixel_read_direction direction, int count,
> -				   struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u16 *px = (u16 *)src_pixels;
> -		*out_pixel = argb_u16_from_u16161616(px[3], px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void XRGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> -				   int y_start, enum pixel_read_direction direction, int count,
> -				   struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
>   
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> +READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
>   
> -	while (out_pixel < end) {
> -		__le16 *px = (__le16 *)src_pixels;
> -		*out_pixel = argb_u16_from_le16161616(cpu_to_le16(0xFFFF), px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> +READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
>   
> -static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> -			     int y_start, enum pixel_read_direction direction, int count,
> -			     struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
>   
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
>   
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> +READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
>   
> -	while (out_pixel < end) {
> -		__le16 *px = (__le16 *)src_pixels;
> +READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
>   
> -		*out_pixel = argb_u16_from_RGB565(px);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
>   
>   /*
>    * This callback can be used for YUV formats where U and V values are
> 

Re: [PATCH RESEND v2 5/8] drm/vkms: Add support for RGB888 formats

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> Add the support for:
> - RGB888
> - BGR888
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 7 +++++++
>   drivers/gpu/drm/vkms/vkms_plane.c   | 2 ++
>   2 files changed, 9 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index e34bea5da752..2376ea8661ac 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -461,6 +461,9 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
>   READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
>   READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
>   
> +READ_LINE_ARGB8888(RGB888_read_line, px, 255, px[2], px[1], px[0])
> +READ_LINE_ARGB8888(BGR888_read_line, px, 255, px[0], px[1], px[2])
> +
>   READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
>   READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
>   READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
> @@ -679,6 +682,10 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>   		return &RGBX8888_read_line;
>   	case DRM_FORMAT_BGRX8888:
>   		return &BGRX8888_read_line;
> +	case DRM_FORMAT_RGB888:
> +		return RGB888_read_line;

Shouldn't it be &RGB888_read_line?

> +	case DRM_FORMAT_BGR888:
> +		return BGR888_read_line;

Same.

Best Regards,
- Maíra

>   	case DRM_FORMAT_ARGB16161616:
>   		return &ARGB16161616_read_line;
>   	case DRM_FORMAT_ABGR16161616:
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index a243a706459f..0fa589abc53a 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -21,6 +21,8 @@ static const u32 vkms_formats[] = {
>   	DRM_FORMAT_XBGR8888,
>   	DRM_FORMAT_RGBX8888,
>   	DRM_FORMAT_BGRX8888,
> +	DRM_FORMAT_RGB888,
> +	DRM_FORMAT_BGR888,
>   	DRM_FORMAT_XRGB16161616,
>   	DRM_FORMAT_XBGR16161616,
>   	DRM_FORMAT_ARGB16161616,
> 

Re: [PATCH RESEND v2 1/8] drm/vkms: Create helpers macro to avoid code duplication in format callbacks

[Maíra Canal]

Hi Louis,

On 07/10/24 13:46, Louis Chauvet wrote:
> The callback functions for line conversion are almost identical for
> some format. The generic READ_LINE macro generate all the required
> boilerplate to process a line.
> 
> Two overrides of this macro have been added to avoid duplication of
> the same arguments every time.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 163 +++++++++++++-----------------------
>   1 file changed, 58 insertions(+), 105 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index f9841b8000c4..8f1bcca38148 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -292,6 +292,58 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
>   }
>   EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
>   
> +/**
> + * READ_LINE() - Generic generator for a read_line function which can be used for format with one
> + * plane and a block_h == block_w == 1.
> + *
> + * @function_name: Function name to generate
> + * @pixel_name: temporary pixel name used in the @__VA_ARGS__ parameters
> + * @pixel_type: Used to specify the type you want to cast the pixel pointer
> + * @callback: Callback to call for each pixels. This fonction should take @__VA_ARGS__ as parameter
> + *            and return a pixel_argb_u16
> + * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_name to access current
> + *  pixel.
> + */
> +#define READ_LINE(function_name, pixel_name, pixel_type, callback, ...)				\
> +static void function_name(const struct vkms_plane_state *plane, int x_start,			\
> +			      int y_start, enum pixel_read_direction direction, int count,	\
> +			      struct pixel_argb_u16 out_pixel[])				\
> +{												\
> +	struct pixel_argb_u16 *end = out_pixel + count;						\
> +	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
> +	u8 *src_pixels;										\
> +												\
> +	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);		\
> +												\
> +	while (out_pixel < end) {								\
> +		pixel_type *(pixel_name) = (pixel_type *)src_pixels;				\
> +		*out_pixel = (callback)(__VA_ARGS__);						\
> +		out_pixel += 1;									\
> +		src_pixels += step;								\
> +	}											\
> +}
> +
> +/**
> + * READ_LINE_ARGB8888() - Generic generator for ARGB8888 formats.
> + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> + *
> + * @function_name: Function name to generate
> + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> + * @a, @r, @g, @b: value of each channel
> + */
> +#define READ_LINE_ARGB8888(function_name, pixel_name, a, r, g, b) \
> +	READ_LINE(function_name, pixel_name, u8, argb_u16_from_u8888, a, r, g, b)
> +/**
> + * READ_LINE_ARGB16161616() - Generic generator for ARGB16161616 formats.
> + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> + *
> + * @function_name: Function name to generate
> + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> + * @a, @r, @g, @b: value of each channel
> + */
> +#define READ_LINE_16161616(function_name, pixel_name, a, r, g, b) \
> +	READ_LINE(function_name, pixel_name, u16, argb_u16_from_u16161616, a, r, g, b)
> +
>   /*
>    * The following functions are read_line function for each pixel format supported by VKMS.
>    *
> @@ -378,118 +430,19 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
>   	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
>   }
>   
> -static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
> -			 int y_start, enum pixel_read_direction direction, int count,
> -			 struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	while (out_pixel < end) {
> -		*out_pixel = argb_u16_from_gray8(*src_pixels);
> -		src_pixels += step;
> -		out_pixel += 1;
> -	}
> -}
> -
> -static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> -			       enum pixel_read_direction direction, int count,
> -			       struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u8 *px = (u8 *)src_pixels;
> -		*out_pixel = argb_u16_from_u8888(px[3], px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void XRGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> -			       enum pixel_read_direction direction, int count,
> -			       struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u8 *px = (u8 *)src_pixels;
> -		*out_pixel = argb_u16_from_u8888(255, px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void ARGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> -				   int y_start, enum pixel_read_direction direction, int count,
> -				   struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> -
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> -
> -	while (out_pixel < end) {
> -		u16 *px = (u16 *)src_pixels;
> -		*out_pixel = argb_u16_from_u16161616(px[3], px[2], px[1], px[0]);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> -
> -static void XRGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> -				   int y_start, enum pixel_read_direction direction, int count,
> -				   struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
> -
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
>   
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> +READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
>   
> -	while (out_pixel < end) {
> -		__le16 *px = (__le16 *)src_pixels;
> -		*out_pixel = argb_u16_from_le16161616(cpu_to_le16(0xFFFF), px[2], px[1], px[0]);

I just compile the whole series and I saw the warning:

drivers/gpu/drm/vkms/vkms_formats.c:226:30: warning: unused function 
'argb_u16_from_le16161616' [-Wunused-function]
   226 | static struct pixel_argb_u16 argb_u16_from_le16161616(__le16 a, 
__le16 r, __le16 g, __le16 b)
       |                              ^~~~~~~~~~~~~~~~~~~~~~~~
1 warning generated.

Best Regards,
- Maíra

> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
> +READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
>   
> -static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> -			     int y_start, enum pixel_read_direction direction, int count,
> -			     struct pixel_argb_u16 out_pixel[])
> -{
> -	struct pixel_argb_u16 *end = out_pixel + count;
> -	u8 *src_pixels;
>   
> -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
>   
> -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> +READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
>   
> -	while (out_pixel < end) {
> -		__le16 *px = (__le16 *)src_pixels;
> +READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
>   
> -		*out_pixel = argb_u16_from_RGB565(px);
> -		out_pixel += 1;
> -		src_pixels += step;
> -	}
> -}
>   
>   /*
>    * This callback can be used for YUV formats where U and V values are
> 

Re: [PATCH RESEND v2 2/8] drm/vkms: Add support for ARGB8888 formats

[Louis Chauvet]

On 26/10/24 - 11:11, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > The formats XRGB8888 and ARGB8888 were already supported.
> > Add the support for:
> > - XBGR8888
> > - RGBX8888
> > - BGRX8888
> > - ABGR8888
> > - RGBA8888
> > - BGRA8888
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 18 ++++++++++++++++++
> >   drivers/gpu/drm/vkms/vkms_plane.c   |  6 ++++++
> >   2 files changed, 24 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index 8f1bcca38148..b5a38f70c62b 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -432,8 +432,14 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
> >   READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> > +READ_LINE_ARGB8888(XBGR8888_read_line, px, 255, px[0], px[1], px[2]) > +READ_LINE_ARGB8888(RGBX8888_read_line, px, 255, px[3], px[2], px[1])
> 
> I'm not expert in colors, but is this correct? From what I understand,
> it should be:

Yes, this is correct, READ_LINE_ARGB8888 take the parameters as A, R, G, 
B, so here 0xFF, px[2], px[1], px[0]
 
> READ_LINE_ARGB8888(RGBX8888_read_line, px, px[2], px[1], px[0], 255)
>                                            ^R     ^G     ^B     ^X
> 
> > +READ_LINE_ARGB8888(BGRX8888_read_line, px, 255, px[1], px[2], px[3])
> 
> Again, is this correct?
>
> Best Regards,
> - Maíra
> 
> >   READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
> > +READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
> > +READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
> > +READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
> >   READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> > @@ -637,8 +643,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> >   	switch (format) {
> >   	case DRM_FORMAT_ARGB8888:
> >   		return &ARGB8888_read_line;
> > +	case DRM_FORMAT_ABGR8888:
> > +		return &ABGR8888_read_line;
> > +	case DRM_FORMAT_BGRA8888:
> > +		return &BGRA8888_read_line;
> > +	case DRM_FORMAT_RGBA8888:
> > +		return &RGBA8888_read_line;
> >   	case DRM_FORMAT_XRGB8888:
> >   		return &XRGB8888_read_line;
> > +	case DRM_FORMAT_XBGR8888:
> > +		return &XBGR8888_read_line;
> > +	case DRM_FORMAT_RGBX8888:
> > +		return &RGBX8888_read_line;
> > +	case DRM_FORMAT_BGRX8888:
> > +		return &BGRX8888_read_line;
> >   	case DRM_FORMAT_ARGB16161616:
> >   		return &ARGB16161616_read_line;
> >   	case DRM_FORMAT_XRGB16161616:
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 67f891e7ac58..941a6e92a040 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -14,7 +14,13 @@
> >   static const u32 vkms_formats[] = {
> >   	DRM_FORMAT_ARGB8888,
> > +	DRM_FORMAT_ABGR8888,
> > +	DRM_FORMAT_BGRA8888,
> > +	DRM_FORMAT_RGBA8888,
> >   	DRM_FORMAT_XRGB8888,
> > +	DRM_FORMAT_XBGR8888,
> > +	DRM_FORMAT_RGBX8888,
> > +	DRM_FORMAT_BGRX8888,
> >   	DRM_FORMAT_XRGB16161616,
> >   	DRM_FORMAT_ARGB16161616,
> >   	DRM_FORMAT_RGB565,
> > 

Re: [PATCH RESEND v2 1/8] drm/vkms: Create helpers macro to avoid code duplication in format callbacks

[Louis Chauvet]

On 26/10/24 - 11:58, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > The callback functions for line conversion are almost identical for
> > some format. The generic READ_LINE macro generate all the required
> > boilerplate to process a line.
> > 
> > Two overrides of this macro have been added to avoid duplication of
> > the same arguments every time.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 163 +++++++++++++-----------------------
> >   1 file changed, 58 insertions(+), 105 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index f9841b8000c4..8f1bcca38148 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -292,6 +292,58 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
> >   }
> >   EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
> > +/**
> > + * READ_LINE() - Generic generator for a read_line function which can be used for format with one
> > + * plane and a block_h == block_w == 1.
> > + *
> > + * @function_name: Function name to generate
> > + * @pixel_name: temporary pixel name used in the @__VA_ARGS__ parameters
> > + * @pixel_type: Used to specify the type you want to cast the pixel pointer
> > + * @callback: Callback to call for each pixels. This fonction should take @__VA_ARGS__ as parameter
> > + *            and return a pixel_argb_u16
> > + * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_name to access current
> > + *  pixel.
> > + */
> > +#define READ_LINE(function_name, pixel_name, pixel_type, callback, ...)				\
> > +static void function_name(const struct vkms_plane_state *plane, int x_start,			\
> > +			      int y_start, enum pixel_read_direction direction, int count,	\
> > +			      struct pixel_argb_u16 out_pixel[])				\
> > +{												\
> > +	struct pixel_argb_u16 *end = out_pixel + count;						\
> > +	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
> > +	u8 *src_pixels;										\
> > +												\
> > +	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);		\
> > +												\
> > +	while (out_pixel < end) {								\
> > +		pixel_type *(pixel_name) = (pixel_type *)src_pixels;				\
> > +		*out_pixel = (callback)(__VA_ARGS__);						\
> > +		out_pixel += 1;									\
> > +		src_pixels += step;								\
> > +	}											\
> > +}
> > +
> > +/**
> > + * READ_LINE_ARGB8888() - Generic generator for ARGB8888 formats.
> > + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> > + *
> > + * @function_name: Function name to generate
> > + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> > + * @a, @r, @g, @b: value of each channel
> > + */
> > +#define READ_LINE_ARGB8888(function_name, pixel_name, a, r, g, b) \
> > +	READ_LINE(function_name, pixel_name, u8, argb_u16_from_u8888, a, r, g, b)
> > +/**
> > + * READ_LINE_ARGB16161616() - Generic generator for ARGB16161616 formats.
> > + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> > + *
> > + * @function_name: Function name to generate
> > + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> > + * @a, @r, @g, @b: value of each channel
> > + */
> > +#define READ_LINE_16161616(function_name, pixel_name, a, r, g, b) \
> > +	READ_LINE(function_name, pixel_name, u16, argb_u16_from_u16161616, a, r, g, b)
> > +
> >   /*
> >    * The following functions are read_line function for each pixel format supported by VKMS.
> >    *
> > @@ -378,118 +430,19 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
> >   	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
> >   }
> > -static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
> > -			 int y_start, enum pixel_read_direction direction, int count,
> > -			 struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	while (out_pixel < end) {
> > -		*out_pixel = argb_u16_from_gray8(*src_pixels);
> > -		src_pixels += step;
> > -		out_pixel += 1;
> > -	}
> > -}
> > -
> > -static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> > -			       enum pixel_read_direction direction, int count,
> > -			       struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u8 *px = (u8 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u8888(px[3], px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void XRGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> > -			       enum pixel_read_direction direction, int count,
> > -			       struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u8 *px = (u8 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u8888(255, px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void ARGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> > -				   int y_start, enum pixel_read_direction direction, int count,
> > -				   struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u16 *px = (u16 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u16161616(px[3], px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void XRGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> > -				   int y_start, enum pixel_read_direction direction, int count,
> > -				   struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > +READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> > -	while (out_pixel < end) {
> > -		__le16 *px = (__le16 *)src_pixels;
> > -		*out_pixel = argb_u16_from_le16161616(cpu_to_le16(0xFFFF), px[2], px[1], px[0]);
> 
> I just compile the whole series and I saw the warning:
> 
> drivers/gpu/drm/vkms/vkms_formats.c:226:30: warning: unused function
> 'argb_u16_from_le16161616' [-Wunused-function]
>   226 | static struct pixel_argb_u16 argb_u16_from_le16161616(__le16 a,
> __le16 r, __le16 g, __le16 b)
>       |                              ^~~~~~~~~~~~~~~~~~~~~~~~
> 1 warning generated.

Thanks, I will fix it for the v2!

In addition I will also fix the missing le/be managment in the macro, 
thanks!

Thanks,
Louis Chauvet
 
> Best Regards,
> - Maíra
> 
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > +READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
> > -static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> > -			     int y_start, enum pixel_read_direction direction, int count,
> > -			     struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> > +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > +READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
> > -	while (out_pixel < end) {
> > -		__le16 *px = (__le16 *)src_pixels;
> > +READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
> > -		*out_pixel = argb_u16_from_RGB565(px);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> >   /*
> >    * This callback can be used for YUV formats where U and V values are
> > 

Re: [PATCH RESEND v2 5/8] drm/vkms: Add support for RGB888 formats

[Louis Chauvet]

On 26/10/24 - 11:51, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > Add the support for:
> > - RGB888
> > - BGR888
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 7 +++++++
> >   drivers/gpu/drm/vkms/vkms_plane.c   | 2 ++
> >   2 files changed, 9 insertions(+)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index e34bea5da752..2376ea8661ac 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -461,6 +461,9 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
> >   READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
> >   READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
> > +READ_LINE_ARGB8888(RGB888_read_line, px, 255, px[2], px[1], px[0])
> > +READ_LINE_ARGB8888(BGR888_read_line, px, 255, px[0], px[1], px[2])
> > +
> >   READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
> >   READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
> >   READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
> > @@ -679,6 +682,10 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> >   		return &RGBX8888_read_line;
> >   	case DRM_FORMAT_BGRX8888:
> >   		return &BGRX8888_read_line;
> > +	case DRM_FORMAT_RGB888:
> > +		return RGB888_read_line;
> 
> Shouldn't it be &RGB888_read_line?

According to [1], &function, function, ***function are understood the 
same by gcc.

But this is ugly and I will change to use & everywhere, thanks!

[1]:https://stackoverflow.com/questions/6893285/why-do-function-pointer-definitions-work-with-any-number-of-ampersands-or-as

Thanks,
Louis Chauvet
 
> > +	case DRM_FORMAT_BGR888:
> > +		return BGR888_read_line;
> 
> Same.
> 
> Best Regards,
> - Maíra
> 
> >   	case DRM_FORMAT_ARGB16161616:
> >   		return &ARGB16161616_read_line;
> >   	case DRM_FORMAT_ABGR16161616:
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index a243a706459f..0fa589abc53a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -21,6 +21,8 @@ static const u32 vkms_formats[] = {
> >   	DRM_FORMAT_XBGR8888,
> >   	DRM_FORMAT_RGBX8888,
> >   	DRM_FORMAT_BGRX8888,
> > +	DRM_FORMAT_RGB888,
> > +	DRM_FORMAT_BGR888,
> >   	DRM_FORMAT_XRGB16161616,
> >   	DRM_FORMAT_XBGR16161616,
> >   	DRM_FORMAT_ARGB16161616,
> > 

Re: [PATCH RESEND v2 1/8] drm/vkms: Create helpers macro to avoid code duplication in format callbacks

[Louis Chauvet]

On 26/10/24 - 11:29, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > The callback functions for line conversion are almost identical for
> > some format. The generic READ_LINE macro generate all the required
> > boilerplate to process a line.
> > 
> > Two overrides of this macro have been added to avoid duplication of
> > the same arguments every time.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 163 +++++++++++++-----------------------
> >   1 file changed, 58 insertions(+), 105 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index f9841b8000c4..8f1bcca38148 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -292,6 +292,58 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1,
> >   }
> >   EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
> > +/**
> > + * READ_LINE() - Generic generator for a read_line function which can be used for format with one
> > + * plane and a block_h == block_w == 1.
> > + *
> > + * @function_name: Function name to generate > + * @pixel_name: temporary pixel name used in the @__VA_ARGS__ parameters
> 
> s/temporary/Temporary
> 
> > + * @pixel_type: Used to specify the type you want to cast the pixel pointer
> > + * @callback: Callback to call for each pixels. This fonction should take @__VA_ARGS__ as parameter
> > + *            and return a pixel_argb_u16
> > + * @__VA_ARGS__: Argument to pass inside the callback. You can use @pixel_name to access current
> > + *  pixel.
> > + */
> > +#define READ_LINE(function_name, pixel_name, pixel_type, callback, ...)				\
> > +static void function_name(const struct vkms_plane_state *plane, int x_start,			\
> > +			      int y_start, enum pixel_read_direction direction, int count,	\
> > +			      struct pixel_argb_u16 out_pixel[])				\
> > +{												\
> > +	struct pixel_argb_u16 *end = out_pixel + count;						\
> > +	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);			\
> > +	u8 *src_pixels;										\
> > +												\
> > +	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);		\
> > +												\
> > +	while (out_pixel < end) {								\
> > +		pixel_type *(pixel_name) = (pixel_type *)src_pixels;				\
> > +		*out_pixel = (callback)(__VA_ARGS__);						\
> > +		out_pixel += 1;									\
> > +		src_pixels += step;								\
> > +	}											\
> > +}
> > +
> > +/**
> > + * READ_LINE_ARGB8888() - Generic generator for ARGB8888 formats.
> > + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> > + *
> > + * @function_name: Function name to generate
> > + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> > + * @a, @r, @g, @b: value of each channel
> > + */
> > +#define READ_LINE_ARGB8888(function_name, pixel_name, a, r, g, b) \
> > +	READ_LINE(function_name, pixel_name, u8, argb_u16_from_u8888, a, r, g, b)
> > +/**
> > + * READ_LINE_ARGB16161616() - Generic generator for ARGB16161616 formats.
> > + * The pixel type used is u8, so pixel_name[0]..pixel_name[n] are the n components of the pixel.
> 
> s/u8/u16

Thanks,
Louis Chauvet
 
> Best Regard,
> - Maíra
> 
> > + *
> > + * @function_name: Function name to generate
> > + * @pixel_name: temporary pixel to use in @a, @r, @g and @b parameters
> > + * @a, @r, @g, @b: value of each channel
> > + */
> > +#define READ_LINE_16161616(function_name, pixel_name, a, r, g, b) \
> > +	READ_LINE(function_name, pixel_name, u16, argb_u16_from_u16161616, a, r, g, b)
> > +
> >   /*
> >    * The following functions are read_line function for each pixel format supported by VKMS.
> >    *
> > @@ -378,118 +430,19 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
> >   	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
> >   }
> > -static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
> > -			 int y_start, enum pixel_read_direction direction, int count,
> > -			 struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	while (out_pixel < end) {
> > -		*out_pixel = argb_u16_from_gray8(*src_pixels);
> > -		src_pixels += step;
> > -		out_pixel += 1;
> > -	}
> > -}
> > -
> > -static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> > -			       enum pixel_read_direction direction, int count,
> > -			       struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u8 *px = (u8 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u8888(px[3], px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void XRGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
> > -			       enum pixel_read_direction direction, int count,
> > -			       struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u8 *px = (u8 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u8888(255, px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void ARGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> > -				   int y_start, enum pixel_read_direction direction, int count,
> > -				   struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > -
> > -	while (out_pixel < end) {
> > -		u16 *px = (u16 *)src_pixels;
> > -		*out_pixel = argb_u16_from_u16161616(px[3], px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > -
> > -static void XRGB16161616_read_line(const struct vkms_plane_state *plane, int x_start,
> > -				   int y_start, enum pixel_read_direction direction, int count,
> > -				   struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > +READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> > -	while (out_pixel < end) {
> > -		__le16 *px = (__le16 *)src_pixels;
> > -		*out_pixel = argb_u16_from_le16161616(cpu_to_le16(0xFFFF), px[2], px[1], px[0]);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> > +READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
> > -static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
> > -			     int y_start, enum pixel_read_direction direction, int count,
> > -			     struct pixel_argb_u16 out_pixel[])
> > -{
> > -	struct pixel_argb_u16 *end = out_pixel + count;
> > -	u8 *src_pixels;
> > -	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
> > +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> > +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
> > -	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
> > +READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
> > -	while (out_pixel < end) {
> > -		__le16 *px = (__le16 *)src_pixels;
> > +READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
> > -		*out_pixel = argb_u16_from_RGB565(px);
> > -		out_pixel += 1;
> > -		src_pixels += step;
> > -	}
> > -}
> >   /*
> >    * This callback can be used for YUV formats where U and V values are
> > 

Re: [PATCH RESEND v2 3/8] drm/vkms: Add support for ARGB16161616 formats

[Louis Chauvet]

On 26/10/24 - 11:15, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > The formats XRGB16161616 and ARGB16161616 were already supported.
> > Add the support for:
> > - ABGR16161616
> > - XBGR16161616
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 11 ++++++++---
> >   drivers/gpu/drm/vkms/vkms_plane.c   |  2 ++
> >   2 files changed, 10 insertions(+), 3 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index b5a38f70c62b..c03a481f5005 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -441,9 +441,10 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
> >   READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
> >   READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
> > -
> > -READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> > -READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0]);
> 
> Please, correct the error in the patch that introduced. Don't fix it in
> an unrelated patch.

Thanks! 
 
> > +READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
> > +READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
> > +READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
> 
> Please, be consistent in the use of decimal numbers and hexadecimal
> number. If you prefer to use hexadecimal, don't use 255, use 0xFF.

I will change everything to hexadecimal for the v2.
 
> > +READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])
> 
> Are you using tests to check the new formats?

I need to check which ones, but I think yes.

Thanks,
Louis Chauvet

> Best Regards,
> - Maíra
> 
> >   READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
> > @@ -659,8 +660,12 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> >   		return &BGRX8888_read_line;
> >   	case DRM_FORMAT_ARGB16161616:
> >   		return &ARGB16161616_read_line;
> > +	case DRM_FORMAT_ABGR16161616:
> > +		return &ABGR16161616_read_line;
> >   	case DRM_FORMAT_XRGB16161616:
> >   		return &XRGB16161616_read_line;
> > +	case DRM_FORMAT_XBGR16161616:
> > +		return &XBGR16161616_read_line;
> >   	case DRM_FORMAT_RGB565:
> >   		return &RGB565_read_line;
> >   	case DRM_FORMAT_NV12:
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 941a6e92a040..1e971c7760d9 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -22,7 +22,9 @@ static const u32 vkms_formats[] = {
> >   	DRM_FORMAT_RGBX8888,
> >   	DRM_FORMAT_BGRX8888,
> >   	DRM_FORMAT_XRGB16161616,
> > +	DRM_FORMAT_XBGR16161616,
> >   	DRM_FORMAT_ARGB16161616,
> > +	DRM_FORMAT_ABGR16161616,
> >   	DRM_FORMAT_RGB565,
> >   	DRM_FORMAT_NV12,
> >   	DRM_FORMAT_NV16,
> > 

Re: [PATCH RESEND v2 4/8] drm/vkms: Add support for RGB565 formats

[Louis Chauvet]

On 26/10/24 - 11:17, Maíra Canal wrote:
> Hi Louis,
> 
> On 07/10/24 13:46, Louis Chauvet wrote:
> > The format RGB565 was already supported. Add the support for:
> > - BGR565
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 25 ++++++++++++++++++++++++-
> >   drivers/gpu/drm/vkms/vkms_plane.c   |  1 +
> >   2 files changed, 25 insertions(+), 1 deletion(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index c03a481f5005..e34bea5da752 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -249,7 +249,7 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
> >   	return out_pixel;
> >   }
> > -static struct pixel_argb_u16 argb_u16_from_gray8(u16 gray)
> > +static struct pixel_argb_u16 argb_u16_from_gray8(u8 gray)
> 
> Again, fix the issue in the patch that introduce it.

Will do for the v2!

Thanks,
Louis Chauvet
 
> Best Regards,
> - Maíra
> 
> >   {
> >   	return argb_u16_from_u8888(255, gray, gray, gray);
> >   }
> > @@ -259,6 +259,26 @@ static struct pixel_argb_u16 argb_u16_from_grayu16(u16 gray)
> >   	return argb_u16_from_u16161616(0xFFFF, gray, gray, gray);
> >   }
> > +static struct pixel_argb_u16 argb_u16_from_BGR565(const __le16 *pixel)
> > +{
> > +	struct pixel_argb_u16 out_pixel;
> > +
> > +	s64 fp_rb_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(31));
> > +	s64 fp_g_ratio = drm_fixp_div(drm_int2fixp(65535), drm_int2fixp(63));
> > +
> > +	u16 rgb_565 = le16_to_cpu(*pixel);
> > +	s64 fp_b = drm_int2fixp((rgb_565 >> 11) & 0x1f);
> > +	s64 fp_g = drm_int2fixp((rgb_565 >> 5) & 0x3f);
> > +	s64 fp_r = drm_int2fixp(rgb_565 & 0x1f);
> > +
> > +	out_pixel.a = (u16)0xffff;
> > +	out_pixel.b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
> > +	out_pixel.g = drm_fixp2int_round(drm_fixp_mul(fp_g, fp_g_ratio));
> > +	out_pixel.r = drm_fixp2int_round(drm_fixp_mul(fp_r, fp_rb_ratio));
> > +
> > +	return out_pixel;
> > +}
> > +
> >   VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> >   							    const struct conversion_matrix *matrix)
> >   {
> > @@ -447,6 +467,7 @@ READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
> >   READ_LINE_16161616(XBGR16161616_read_line, px, 0xFFFF, px[0], px[1], px[2])
> >   READ_LINE(RGB565_read_line, px, __le16, argb_u16_from_RGB565, px)
> > +READ_LINE(BGR565_read_line, px, __le16, argb_u16_from_BGR565, px)
> >   READ_LINE(R8_read_line, px, u8, argb_u16_from_gray8, *px)
> > @@ -668,6 +689,8 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> >   		return &XBGR16161616_read_line;
> >   	case DRM_FORMAT_RGB565:
> >   		return &RGB565_read_line;
> > +	case DRM_FORMAT_BGR565:
> > +		return &BGR565_read_line;
> >   	case DRM_FORMAT_NV12:
> >   	case DRM_FORMAT_NV16:
> >   	case DRM_FORMAT_NV24:
> > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > index 1e971c7760d9..a243a706459f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > @@ -26,6 +26,7 @@ static const u32 vkms_formats[] = {
> >   	DRM_FORMAT_ARGB16161616,
> >   	DRM_FORMAT_ABGR16161616,
> >   	DRM_FORMAT_RGB565,
> > +	DRM_FORMAT_BGR565,
> >   	DRM_FORMAT_NV12,
> >   	DRM_FORMAT_NV16,
> >   	DRM_FORMAT_NV24,
> > 

Re: [PATCH RESEND v2 2/8] drm/vkms: Add support for ARGB8888 formats

[Maíra Canal]

Hi Louis,

On 28/10/24 06:50, Louis Chauvet wrote:
> On 26/10/24 - 11:11, Maíra Canal wrote:
>> Hi Louis,
>>
>> On 07/10/24 13:46, Louis Chauvet wrote:
>>> The formats XRGB8888 and ARGB8888 were already supported.
>>> Add the support for:
>>> - XBGR8888
>>> - RGBX8888
>>> - BGRX8888
>>> - ABGR8888
>>> - RGBA8888
>>> - BGRA8888
>>>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> ---
>>>    drivers/gpu/drm/vkms/vkms_formats.c | 18 ++++++++++++++++++
>>>    drivers/gpu/drm/vkms/vkms_plane.c   |  6 ++++++
>>>    2 files changed, 24 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>>> index 8f1bcca38148..b5a38f70c62b 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>>> @@ -432,8 +432,14 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
>>>    READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
>>> +READ_LINE_ARGB8888(XBGR8888_read_line, px, 255, px[0], px[1], px[2]) > +READ_LINE_ARGB8888(RGBX8888_read_line, px, 255, px[3], px[2], px[1])
>>
>> I'm not expert in colors, but is this correct? From what I understand,
>> it should be:
> 
> Yes, this is correct, READ_LINE_ARGB8888 take the parameters as A, R, G,
> B, so here 0xFF, px[2], px[1], px[0]

Now I wonder if

READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])

is correct.

Best Regards,
- Maíra

>   
>> READ_LINE_ARGB8888(RGBX8888_read_line, px, px[2], px[1], px[0], 255)
>>                                             ^R     ^G     ^B     ^X
>>
>>> +READ_LINE_ARGB8888(BGRX8888_read_line, px, 255, px[1], px[2], px[3])
>>
>> Again, is this correct?
>>
>> Best Regards,
>> - Maíra
>>
>>>    READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
>>> +READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
>>> +READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
>>> +READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
>>>    READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
>>> @@ -637,8 +643,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>>>    	switch (format) {
>>>    	case DRM_FORMAT_ARGB8888:
>>>    		return &ARGB8888_read_line;
>>> +	case DRM_FORMAT_ABGR8888:
>>> +		return &ABGR8888_read_line;
>>> +	case DRM_FORMAT_BGRA8888:
>>> +		return &BGRA8888_read_line;
>>> +	case DRM_FORMAT_RGBA8888:
>>> +		return &RGBA8888_read_line;
>>>    	case DRM_FORMAT_XRGB8888:
>>>    		return &XRGB8888_read_line;
>>> +	case DRM_FORMAT_XBGR8888:
>>> +		return &XBGR8888_read_line;
>>> +	case DRM_FORMAT_RGBX8888:
>>> +		return &RGBX8888_read_line;
>>> +	case DRM_FORMAT_BGRX8888:
>>> +		return &BGRX8888_read_line;
>>>    	case DRM_FORMAT_ARGB16161616:
>>>    		return &ARGB16161616_read_line;
>>>    	case DRM_FORMAT_XRGB16161616:
>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>> index 67f891e7ac58..941a6e92a040 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>> @@ -14,7 +14,13 @@
>>>    static const u32 vkms_formats[] = {
>>>    	DRM_FORMAT_ARGB8888,
>>> +	DRM_FORMAT_ABGR8888,
>>> +	DRM_FORMAT_BGRA8888,
>>> +	DRM_FORMAT_RGBA8888,
>>>    	DRM_FORMAT_XRGB8888,
>>> +	DRM_FORMAT_XBGR8888,
>>> +	DRM_FORMAT_RGBX8888,
>>> +	DRM_FORMAT_BGRX8888,
>>>    	DRM_FORMAT_XRGB16161616,
>>>    	DRM_FORMAT_ARGB16161616,
>>>    	DRM_FORMAT_RGB565,
>>>

Re: [PATCH RESEND v2 5/8] drm/vkms: Add support for RGB888 formats

[Maíra Canal]

Hi Louis,

On 28/10/24 06:50, Louis Chauvet wrote:
> On 26/10/24 - 11:51, Maíra Canal wrote:
>> Hi Louis,
>>
>> On 07/10/24 13:46, Louis Chauvet wrote:
>>> Add the support for:
>>> - RGB888
>>> - BGR888
>>>
>>> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
>>> ---
>>>    drivers/gpu/drm/vkms/vkms_formats.c | 7 +++++++
>>>    drivers/gpu/drm/vkms/vkms_plane.c   | 2 ++
>>>    2 files changed, 9 insertions(+)
>>>
>>> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
>>> index e34bea5da752..2376ea8661ac 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_formats.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
>>> @@ -461,6 +461,9 @@ READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
>>>    READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
>>>    READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
>>> +READ_LINE_ARGB8888(RGB888_read_line, px, 255, px[2], px[1], px[0])
>>> +READ_LINE_ARGB8888(BGR888_read_line, px, 255, px[0], px[1], px[2])
>>> +
>>>    READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0])
>>>    READ_LINE_16161616(ABGR16161616_read_line, px, px[3], px[0], px[1], px[2])
>>>    READ_LINE_16161616(XRGB16161616_read_line, px, 0xFFFF, px[2], px[1], px[0])
>>> @@ -679,6 +682,10 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
>>>    		return &RGBX8888_read_line;
>>>    	case DRM_FORMAT_BGRX8888:
>>>    		return &BGRX8888_read_line;
>>> +	case DRM_FORMAT_RGB888:
>>> +		return RGB888_read_line;
>>
>> Shouldn't it be &RGB888_read_line?
> 
> According to [1], &function, function, ***function are understood the
> same by gcc.
> 
> But this is ugly and I will change to use & everywhere, thanks!

I didn't know that, that's interesting. I'd add & just for consistency.

Best Regards,
- Maíra

> 
> [1]:https://stackoverflow.com/questions/6893285/why-do-function-pointer-definitions-work-with-any-number-of-ampersands-or-as
> 
> Thanks,
> Louis Chauvet
>   
>>> +	case DRM_FORMAT_BGR888:
>>> +		return BGR888_read_line;
>>
>> Same.
>>
>> Best Regards,
>> - Maíra
>>
>>>    	case DRM_FORMAT_ARGB16161616:
>>>    		return &ARGB16161616_read_line;
>>>    	case DRM_FORMAT_ABGR16161616:
>>> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
>>> index a243a706459f..0fa589abc53a 100644
>>> --- a/drivers/gpu/drm/vkms/vkms_plane.c
>>> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
>>> @@ -21,6 +21,8 @@ static const u32 vkms_formats[] = {
>>>    	DRM_FORMAT_XBGR8888,
>>>    	DRM_FORMAT_RGBX8888,
>>>    	DRM_FORMAT_BGRX8888,
>>> +	DRM_FORMAT_RGB888,
>>> +	DRM_FORMAT_BGR888,
>>>    	DRM_FORMAT_XRGB16161616,
>>>    	DRM_FORMAT_XBGR16161616,
>>>    	DRM_FORMAT_ARGB16161616,
>>>

Re: [PATCH RESEND v2 2/8] drm/vkms: Add support for ARGB8888 formats

[Louis Chauvet]

On 28/10/24 - 07:20, Maíra Canal wrote:
> Hi Louis,
> 
> On 28/10/24 06:50, Louis Chauvet wrote:
> > On 26/10/24 - 11:11, Maíra Canal wrote:
> > > Hi Louis,
> > > 
> > > On 07/10/24 13:46, Louis Chauvet wrote:
> > > > The formats XRGB8888 and ARGB8888 were already supported.
> > > > Add the support for:
> > > > - XBGR8888
> > > > - RGBX8888
> > > > - BGRX8888
> > > > - ABGR8888
> > > > - RGBA8888
> > > > - BGRA8888
> > > > 
> > > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > > ---
> > > >    drivers/gpu/drm/vkms/vkms_formats.c | 18 ++++++++++++++++++
> > > >    drivers/gpu/drm/vkms/vkms_plane.c   |  6 ++++++
> > > >    2 files changed, 24 insertions(+)
> > > > 
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > > > index 8f1bcca38148..b5a38f70c62b 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > > > @@ -432,8 +432,14 @@ static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
> > > >    READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> > > > +READ_LINE_ARGB8888(XBGR8888_read_line, px, 255, px[0], px[1], px[2]) > +READ_LINE_ARGB8888(RGBX8888_read_line, px, 255, px[3], px[2], px[1])
> > > 
> > > I'm not expert in colors, but is this correct? From what I understand,
> > > it should be:
> > 
> > Yes, this is correct, READ_LINE_ARGB8888 take the parameters as A, R, G,
> > B, so here 0xFF, px[2], px[1], px[0]
> 
> Now I wonder if
> 
> READ_LINE_ARGB8888(XRGB8888_read_line, px, 255, px[2], px[1], px[0])
> 
> is correct.

Why? 

It creates the function `XRGB8888_read_line`, that reads the pixel line, 4 
bytes by 4 bytes, ignore the alpha value, uses px[2] as R, px[1] 
as G, px[0] as B.

I just tested with kms_plane (igt version 1.27.1-NO-GIT, fedora), and he 
seems happy on all the supported formats, including XRGB8888 (XR24):

Testing format XR24(0x34325258) / modifier linear(0x0) on A.0
Testing format AR24(0x34325241) / modifier linear(0x0) on A.0
Testing format AB24(0x34324241) / modifier linear(0x0) on A.0
Testing format XB24(0x34324258) / modifier linear(0x0) on A.0
Testing format RG24(0x34324752) / modifier linear(0x0) on A.0
Testing format BG24(0x34324742) / modifier linear(0x0) on A.0
Testing format XR48(0x38345258) / modifier linear(0x0) on A.0
Testing format XB48(0x38344258) / modifier linear(0x0) on A.0
Testing format AR48(0x38345241) / modifier linear(0x0) on A.0
Testing format AB48(0x38344241) / modifier linear(0x0) on A.0
Testing format RG16(0x36314752) / modifier linear(0x0) on A.0
Testing format BG16(0x36314742) / modifier linear(0x0) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format NV12(0x3231564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format NV16(0x3631564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format NV21(0x3132564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format NV61(0x3136564e) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format YU12(0x32315559) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format YU16(0x36315559) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format YV12(0x32315659) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format YV16(0x36315659) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format P010(0x30313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format P012(0x32313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr limited range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.601 YCbCr, YCbCr full range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr limited range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.709 YCbCr, YCbCr full range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr limited range) on A.0
Testing format P016(0x36313050) / modifier linear(0x0) (ITU-R BT.2020 YCbCr, YCbCr full range) on A.0


> Best Regards,
> - Maíra
> 
> > > READ_LINE_ARGB8888(RGBX8888_read_line, px, px[2], px[1], px[0], 255)
> > >                                             ^R     ^G     ^B     ^X
> > > 
> > > > +READ_LINE_ARGB8888(BGRX8888_read_line, px, 255, px[1], px[2], px[3])
> > > 
> > > Again, is this correct?
> > > 
> > > Best Regards,
> > > - Maíra
> > > 
> > > >    READ_LINE_ARGB8888(ARGB8888_read_line, px, px[3], px[2], px[1], px[0])
> > > > +READ_LINE_ARGB8888(ABGR8888_read_line, px, px[3], px[0], px[1], px[2])
> > > > +READ_LINE_ARGB8888(RGBA8888_read_line, px, px[0], px[3], px[2], px[1])
> > > > +READ_LINE_ARGB8888(BGRA8888_read_line, px, px[0], px[1], px[2], px[3])
> > > >    READ_LINE_16161616(ARGB16161616_read_line, px, px[3], px[2], px[1], px[0]);
> > > > @@ -637,8 +643,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
> > > >    	switch (format) {
> > > >    	case DRM_FORMAT_ARGB8888:
> > > >    		return &ARGB8888_read_line;
> > > > +	case DRM_FORMAT_ABGR8888:
> > > > +		return &ABGR8888_read_line;
> > > > +	case DRM_FORMAT_BGRA8888:
> > > > +		return &BGRA8888_read_line;
> > > > +	case DRM_FORMAT_RGBA8888:
> > > > +		return &RGBA8888_read_line;
> > > >    	case DRM_FORMAT_XRGB8888:
> > > >    		return &XRGB8888_read_line;
> > > > +	case DRM_FORMAT_XBGR8888:
> > > > +		return &XBGR8888_read_line;
> > > > +	case DRM_FORMAT_RGBX8888:
> > > > +		return &RGBX8888_read_line;
> > > > +	case DRM_FORMAT_BGRX8888:
> > > > +		return &BGRX8888_read_line;
> > > >    	case DRM_FORMAT_ARGB16161616:
> > > >    		return &ARGB16161616_read_line;
> > > >    	case DRM_FORMAT_XRGB16161616:
> > > > diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > index 67f891e7ac58..941a6e92a040 100644
> > > > --- a/drivers/gpu/drm/vkms/vkms_plane.c
> > > > +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> > > > @@ -14,7 +14,13 @@
> > > >    static const u32 vkms_formats[] = {
> > > >    	DRM_FORMAT_ARGB8888,
> > > > +	DRM_FORMAT_ABGR8888,
> > > > +	DRM_FORMAT_BGRA8888,
> > > > +	DRM_FORMAT_RGBA8888,
> > > >    	DRM_FORMAT_XRGB8888,
> > > > +	DRM_FORMAT_XBGR8888,
> > > > +	DRM_FORMAT_RGBX8888,
> > > > +	DRM_FORMAT_BGRX8888,
> > > >    	DRM_FORMAT_XRGB16161616,
> > > >    	DRM_FORMAT_ARGB16161616,
> > > >    	DRM_FORMAT_RGB565,
> > > > 
>