[PATCH v2 0/3] drm/vkms: Reimplement line-per-line pixel conversion for writeback

[PATCH v2 0/3] drm/vkms: Reimplement line-per-line pixel conversion for writeback

[Louis Chauvet]

This series re-introduce the line-by-line algorithm. This is simpler than 
the read part because no rotation/translations are involved. 

PATCH 1/3 is the re-introduction itself
PATCH 2/3 is a proposition to avoid code repetition using a "big" macro.
PATCJ 3/3 is the usage of PATCH 2 to support a new format

This series depends on [1].

[1]: https://lore.kernel.org/all/20240814-b4-new-color-formats-v2-0-8b3499cfe90e@bootlin.com/

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v2:
- PATCH 2/3: Remove YUV from supported format list, it is not supported 
- Link to v1: https://lore.kernel.org/r/20240516-writeback_line_by_line-v1-0-7b2e3bf9f1c9@bootlin.com

---
Louis Chauvet (3):
      drm/vkms: Re-introduce line-by-line algorithm for writeback
      drm/vkms: Add a macro for write_line functions
      drm/vkms: Add support for XRGB2101010

 drivers/gpu/drm/vkms/vkms_composer.c  | 17 +++++++
 drivers/gpu/drm/vkms/vkms_drv.h       | 20 ++++----
 drivers/gpu/drm/vkms/vkms_formats.c   | 86 ++++++++++++++++++++++++-----------
 drivers/gpu/drm/vkms/vkms_formats.h   |  2 +-
 drivers/gpu/drm/vkms/vkms_writeback.c |  5 +-
 5 files changed, 93 insertions(+), 37 deletions(-)
---
base-commit: 219b45d023ed0902b05c5902a4f31c2c38bcf68c
change-id: 20240222-writeback_line_by_line-8475605b1d5c
prerequisite-message-id: <20240809-yuv-v10-0-1a7c764166f7@bootlin.com>
prerequisite-patch-id: ae2d8b2efbbaa9decce56632c498c87e708288b3
prerequisite-patch-id: c26b6d4867eaf6566195aa0002765357d4f69f8c
prerequisite-patch-id: 8791d34a6f3148dc518da5249453067e40d346e3
prerequisite-patch-id: 26ec7cd5a449004bcfd6ce483671f87655f8635c
prerequisite-patch-id: 2e855ba871f2e99d4b6b7d85da2ddac6bb32262e
prerequisite-patch-id: 82523a917646793deeec7cdcc7ff286bd924fd21
prerequisite-patch-id: 0e355e5316281f53ab5e97ab6e63b0a682f3eb9e
prerequisite-patch-id: 7a63d245a377d5f5283f48e8f52421b912811752
prerequisite-patch-id: dda6bf4692cd1795c489ff58e72c0841ea8ffbc4
prerequisite-patch-id: f70e535b6086cc587975fbfa75741f485f679a32
prerequisite-patch-id: 6c2aa2645c7d854951608aa4d15a02e076abe1fe
prerequisite-patch-id: dc61c6d3db73053fc36e115af561e0c42b467de2
prerequisite-patch-id: deda292af6d8bbf6762b0bf4d351ffd2225995d8
prerequisite-patch-id: 18554f49b53cbcfd4a8ca50dc83b17dd3cf96474
prerequisite-patch-id: 5633292e10132d29be2467812e6e2e824cfedb67
prerequisite-patch-id: 43f37e9c1bc041d491e41dfb59548ed258a1e071
prerequisite-message-id: <20240814-b4-new-color-formats-v2-0-8b3499cfe90e@bootlin.com>
prerequisite-patch-id: d10db4cb12a88de2e5f6440e9fcf5ddda191e3cd
prerequisite-patch-id: 16bac0ef1f1dc010a72ce2faae66631797d23d3f
prerequisite-patch-id: 8e0e5cc0727e8fd2d14ebafc5538fd987c2dd38e
prerequisite-patch-id: 32bad3bf3df46d042e9edd4c1259c2e2a3fb8975
prerequisite-patch-id: 4bd9e4cef308abd17b7b274a5575a3de73a1503b
prerequisite-patch-id: a98fac5a2c60fe23fbc6a455e9a4ab8b0f187ee8
prerequisite-patch-id: 62c8d109a22b9978f755255b67f13fe74fb7008d
prerequisite-patch-id: baa8da4871dd90b03a07c6d9ddb45e10929ee70a

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

[PATCH v2 1/3] drm/vkms: Re-introduce line-by-line algorithm for writeback

[Louis Chauvet]

Re-introduce a line-by-line writeback algorithm for each pixel format.
This allows more performance by not requiring an indirection per pixel
write.

Line-by-line writeback was introduced by [1] but rewritten back to
pixel-by-pixel algorithm in [2]. At this time, nobody noticed the impact
on performance, and it was merged.

This patch is almost a revert of [2], but with some effort to avoid code
duplication. Now only the loop is repeated, but it is required to have
good performances.

The performance gain is around 5 to 10%.

[1]: https://lore.kernel.org/all/20211005201637.58563-7-igormtorrente@gmail.com/
[2]: https://lore.kernel.org/all/20230515135204.115393-4-mcanal@igalia.com/

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_composer.c  |  17 +++++
 drivers/gpu/drm/vkms/vkms_drv.h       |  20 +++---
 drivers/gpu/drm/vkms/vkms_formats.c   | 117 +++++++++++++++++++++++++++-------
 drivers/gpu/drm/vkms/vkms_formats.h   |   2 +-
 drivers/gpu/drm/vkms/vkms_writeback.c |   2 +-
 5 files changed, 124 insertions(+), 34 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 76d4aa8a0ef6..f0cae142ac22 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -176,6 +176,23 @@ static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
 	return READ_LEFT_TO_RIGHT;
 }
 
+/**
+ * Write a line to the writeback buffer
+ *
+ * @wb: Job where to insert the final image
+ * @src_buffer: Line to write
+ * @y: Row to write in the writeback buffer
+ */
+static void vkms_writeback_row(struct vkms_writeback_job *wb,
+			       const struct line_buffer *src_buffer, size_t y_start)
+{
+	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
+	int x_start = frame_info->dst.x1;
+	int count = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
+
+	wb->pixel_write(wb, src_buffer->pixels, count, x_start, y_start);
+}
+
 /**
  * clamp_line_coordinates() - Compute and clamp the coordinate to read and write during the blend
  * process.
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 3870e825da81..526bf5207524 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -52,20 +52,25 @@ struct line_buffer {
 	struct pixel_argb_u16 *pixels;
 };
 
+struct vkms_writeback_job;
 /**
- * typedef pixel_write_t - These functions are used to read a pixel from a
- * &struct pixel_argb_u16, convert it in a specific format and write it in the @dst_pixels
- * buffer.
+ * typedef pixel_write_line_t - These functions are used to read a pixel line from a
+ * struct pixel_argb_u16 buffer, convert it and write it in the @wb job.
  *
- * @out_pixel: destination address to write the pixel
- * @in_pixel: pixel to write
+ * @wb: the writeback job to write the output of the conversion
+ * @in_pixels: Source buffer containing the line to convert
+ * @count: The width of a line
+ * @x_start: The x (width) coordinate in the destination plane
+ * @y_start: The y (height) coordinate in the destination plane
  */
-typedef void (*pixel_write_t)(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel);
+typedef void (*pixel_write_line_t)(struct vkms_writeback_job *wb,
+			      struct pixel_argb_u16 *in_pixels, int count, int x_start,
+			      int y_start);
 
 struct vkms_writeback_job {
 	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
 	struct vkms_frame_info wb_frame_info;
-	pixel_write_t pixel_write;
+	pixel_write_line_t pixel_write;
 };
 
 /**
@@ -232,7 +237,6 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
 /* Composer Support */
 void vkms_composer_worker(struct work_struct *work);
 void vkms_set_composer(struct vkms_output *out, bool enabled);
-void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
 
 /* Writeback */
 int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index d1abfb1c3e3c..d1309f6d307f 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -587,7 +587,7 @@ static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_sta
  * The following functions take one &struct pixel_argb_u16 and convert it to a specific format.
  * The result is stored in @out_pixel.
  *
- * They are used in vkms_writeback_row() to convert and store a pixel from the src_buffer to
+ * They are used in the `write_line` functions to convert and store a pixel from the src_buffer to
  * the writeback buffer.
  */
 static void argb_u16_to_ARGB8888(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel)
@@ -654,28 +654,97 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
 	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
 }
 
-/**
- * vkms_writeback_row() - Generic loop for all supported writeback format. It is executed just
- * after the blending to write a line in the writeback buffer.
+/*
+ * The following functions are write_line function for each pixel format supported by VKMS.
+ *
+ * They write a full line at index y. They must read data from the line src_pixels.
+ *
+ * The caller must ensure that count is not larger than the framebuffer and the src_pixels.
+ *
+ * Those function are very similar, but it is required for performance reason. In the past, some
+ * experiment were done, and with a generic loop the performance are very reduced [1].
  *
- * @wb: Job where to insert the final image
- * @src_buffer: Line to write
- * @y: Row to write in the writeback buffer
+ * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
  */
-void vkms_writeback_row(struct vkms_writeback_job *wb,
-			const struct line_buffer *src_buffer, int y)
+
+static void ARGB8888_write_line(struct vkms_writeback_job *wb,
+				struct pixel_argb_u16 *src_pixels, int count, int x_start,
+				int y_start)
 {
-	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
-	int x_dst = frame_info->dst.x1;
 	u8 *dst_pixels;
-	int rem_x, rem_y;
 
-	packed_pixels_addr(frame_info, x_dst, y, 0, &dst_pixels, &rem_x, &rem_y);
-	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
-	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
 
-	for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->fb->format->cpp[0])
-		wb->pixel_write(dst_pixels, &in_pixels[x]);
+	while (count) {
+		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
+		src_pixels += 1;
+		count--;
+	}
+}
+
+static void XRGB8888_write_line(struct vkms_writeback_job *wb,
+				struct pixel_argb_u16 *src_pixels, int count, int x_start,
+				int y_start)
+{
+	u8 *dst_pixels;
+
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
+
+	while (count) {
+		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
+		src_pixels += 1;
+		count--;
+	}
+}
+
+static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
+				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
+				    int y_start)
+{
+	u8 *dst_pixels;
+
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
+
+	while (count) {
+		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
+		src_pixels += 1;
+		count--;
+	}
+}
+
+static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
+				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
+				    int y_start)
+{
+	u8 *dst_pixels;
+
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
+
+	while (count) {
+		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
+		src_pixels += 1;
+		count--;
+	}
+}
+
+static void RGB565_write_line(struct vkms_writeback_job *wb,
+			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
+			      int y_start)
+{
+	u8 *dst_pixels;
+
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
+
+	while (count) {
+		argb_u16_to_RGB565(dst_pixels, src_pixels);
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
+		src_pixels += 1;
+		count--;
+	}
 }
 
 /**
@@ -936,25 +1005,25 @@ void get_conversion_matrix_to_argb_u16(u32 format,
 }
 
 /**
- * get_pixel_write_function() - Retrieve the correct write_pixel function for a specific format.
+ * get_pixel_write_function() - Retrieve the correct write_line function for a specific format.
  * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
  * pointer is valid before using it in a vkms_writeback_job.
  *
  * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
  */
-pixel_write_t get_pixel_write_function(u32 format)
+pixel_write_line_t get_pixel_write_line_function(u32 format)
 {
 	switch (format) {
 	case DRM_FORMAT_ARGB8888:
-		return &argb_u16_to_ARGB8888;
+		return &ARGB8888_write_line;
 	case DRM_FORMAT_XRGB8888:
-		return &argb_u16_to_XRGB8888;
+		return &XRGB8888_write_line;
 	case DRM_FORMAT_ARGB16161616:
-		return &argb_u16_to_ARGB16161616;
+		return &ARGB16161616_write_line;
 	case DRM_FORMAT_XRGB16161616:
-		return &argb_u16_to_XRGB16161616;
+		return &XRGB16161616_write_line;
 	case DRM_FORMAT_RGB565:
-		return &argb_u16_to_RGB565;
+		return &RGB565_write_line;
 	default:
 		/*
 		 * This is a bug in vkms_writeback_atomic_check. All the supported
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index eeb208cdd6b1..852ab9a4cee5 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -7,7 +7,7 @@
 
 pixel_read_line_t get_pixel_read_line_function(u32 format);
 
-pixel_write_t get_pixel_write_function(u32 format);
+pixel_write_line_t get_pixel_write_line_function(u32 format);
 
 void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 				       enum drm_color_range range,
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index c8582df1f739..f6ed3aa69af8 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -150,7 +150,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	crtc_state->wb_pending = true;
 	spin_unlock_irq(&output->composer_lock);
 	drm_writeback_queue_job(wb_conn, connector_state);
-	active_wb->pixel_write = get_pixel_write_function(wb_format);
+	active_wb->pixel_write = get_pixel_write_line_function(wb_format);
 	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
 	drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height);
 }

-- 
2.44.2

[PATCH v2 2/3] drm/vkms: Add a macro for write_line functions

[Louis Chauvet]

As stated in [2], the write_line functions are very similar and force code
duplication. This patch add a macro to avoid code repetition.

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

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index d1309f6d307f..a25cdf656d8a 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -654,6 +654,31 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
 	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
 }
 
+/**
+ * WRITE_LINE() - Generic generator for write_line functions
+ *
+ * This generator can only be used for format with only one plane and block_w == block_h == 1
+ *
+ * @function_name: Name to use for the generated function
+ * @conversion_function: Fonction to use for the conversion from argb_u16 to the required format.
+ */
+#define WRITE_LINE(function_name, conversion_function)					\
+static void function_name(struct vkms_writeback_job *wb,				\
+			  struct pixel_argb_u16 *src_pixels, int count, int x_start,	\
+			  int y_start)							\
+{											\
+	u8 *dst_pixels;									\
+											\
+	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);	\
+											\
+	while (count) {									\
+		(conversion_function)(dst_pixels, src_pixels);				\
+		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];		\
+		src_pixels += 1;							\
+		count--;								\
+	}										\
+}
+
 /*
  * The following functions are write_line function for each pixel format supported by VKMS.
  *
@@ -667,85 +692,13 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
  * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
  */
 
-static void ARGB8888_write_line(struct vkms_writeback_job *wb,
-				struct pixel_argb_u16 *src_pixels, int count, int x_start,
-				int y_start)
-{
-	u8 *dst_pixels;
+WRITE_LINE(ARGB8888_write_line, argb_u16_to_ARGB8888)
+WRITE_LINE(XRGB8888_write_line, argb_u16_to_XRGB8888)
 
-	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
+WRITE_LINE(ARGB16161616_write_line, argb_u16_to_ARGB16161616)
+WRITE_LINE(XRGB16161616_write_line, argb_u16_to_XRGB16161616)
 
-	while (count) {
-		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
-		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
-		src_pixels += 1;
-		count--;
-	}
-}
-
-static void XRGB8888_write_line(struct vkms_writeback_job *wb,
-				struct pixel_argb_u16 *src_pixels, int count, int x_start,
-				int y_start)
-{
-	u8 *dst_pixels;
-
-	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
-
-	while (count) {
-		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
-		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
-		src_pixels += 1;
-		count--;
-	}
-}
-
-static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
-				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
-				    int y_start)
-{
-	u8 *dst_pixels;
-
-	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
-
-	while (count) {
-		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
-		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
-		src_pixels += 1;
-		count--;
-	}
-}
-
-static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
-				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
-				    int y_start)
-{
-	u8 *dst_pixels;
-
-	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
-
-	while (count) {
-		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
-		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
-		src_pixels += 1;
-		count--;
-	}
-}
-
-static void RGB565_write_line(struct vkms_writeback_job *wb,
-			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
-			      int y_start)
-{
-	u8 *dst_pixels;
-
-	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
-
-	while (count) {
-		argb_u16_to_RGB565(dst_pixels, src_pixels);
-		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
-		src_pixels += 1;
-		count--;
-	}
-}
+WRITE_LINE(RGB565_write_line, argb_u16_to_RGB565)
 
 /**
  * get_pixel_read_function() - Retrieve the correct read_line function for a specific

-- 
2.44.2

[PATCH v2 3/3] drm/vkms: Add support for XRGB2101010

[Louis Chauvet]

Thanks to the WRITE_LINE macro, adding the format XRGB210101010 is trivial.

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

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index a25cdf656d8a..65fdd3999441 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -654,6 +654,14 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
 	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
 }
 
+static void argb_u16_to_XRGB2101010(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel)
+{
+	out_pixel[0] = (u8)(in_pixel->b & 0xFF);
+	out_pixel[1] = (u8)((in_pixel->b >> 8) & 0x03) | (u8)((in_pixel->g << 2) & 0xFC);
+	out_pixel[2] = (u8)((in_pixel->g >> 6) & 0x0F) | (u8)((in_pixel->r << 4) & 0xF0);
+	out_pixel[3] = (u8)((in_pixel->r >> 4) & 0x3F);
+}
+
 /**
  * WRITE_LINE() - Generic generator for write_line functions
  *
@@ -700,6 +708,8 @@ WRITE_LINE(XRGB16161616_write_line, argb_u16_to_XRGB16161616)
 
 WRITE_LINE(RGB565_write_line, argb_u16_to_RGB565)
 
+WRITE_LINE(XRGB2101010_write_line, argb_u16_to_XRGB2101010)
+
 /**
  * get_pixel_read_function() - Retrieve the correct read_line function for a specific
  * format. The returned pointer is NULL for unsupported pixel formats. The caller must ensure that
@@ -977,6 +987,8 @@ pixel_write_line_t get_pixel_write_line_function(u32 format)
 		return &XRGB16161616_write_line;
 	case DRM_FORMAT_RGB565:
 		return &RGB565_write_line;
+	case DRM_FORMAT_XRGB2101010:
+		return &XRGB2101010_write_line;
 	default:
 		/*
 		 * This is a bug in vkms_writeback_atomic_check. All the supported
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index f6ed3aa69af8..7e0302c0830c 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -19,7 +19,8 @@ static const u32 vkms_wb_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_XRGB16161616,
 	DRM_FORMAT_ARGB16161616,
-	DRM_FORMAT_RGB565
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_XRGB2101010,
 };
 
 static const struct drm_connector_funcs vkms_wb_connector_funcs = {

-- 
2.44.2

Re: [PATCH v2 1/3] drm/vkms: Re-introduce line-by-line algorithm for writeback

[Maíra Canal]

Hi Louis,

On 14/08/24 05:42, Louis Chauvet wrote:
> Re-introduce a line-by-line writeback algorithm for each pixel format.
> This allows more performance by not requiring an indirection per pixel
> write.
> 
> Line-by-line writeback was introduced by [1] but rewritten back to
> pixel-by-pixel algorithm in [2]. At this time, nobody noticed the impact
> on performance, and it was merged.
> 
> This patch is almost a revert of [2], but with some effort to avoid code
> duplication. Now only the loop is repeated, but it is required to have
> good performances.
> 
> The performance gain is around 5 to 10%.
> 
> [1]: https://lore.kernel.org/all/20211005201637.58563-7-igormtorrente@gmail.com/
> [2]: https://lore.kernel.org/all/20230515135204.115393-4-mcanal@igalia.com/
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_composer.c  |  17 +++++
>   drivers/gpu/drm/vkms/vkms_drv.h       |  20 +++---
>   drivers/gpu/drm/vkms/vkms_formats.c   | 117 +++++++++++++++++++++++++++-------
>   drivers/gpu/drm/vkms/vkms_formats.h   |   2 +-
>   drivers/gpu/drm/vkms/vkms_writeback.c |   2 +-
>   5 files changed, 124 insertions(+), 34 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 76d4aa8a0ef6..f0cae142ac22 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -176,6 +176,23 @@ static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
>   	return READ_LEFT_TO_RIGHT;
>   }
>   
> +/**
> + * Write a line to the writeback buffer > + *
> + * @wb: Job where to insert the final image
> + * @src_buffer: Line to write
> + * @y: Row to write in the writeback buffer
> + */

Please, review the documentation using the kernel-doc format.

> +static void vkms_writeback_row(struct vkms_writeback_job *wb,
> +			       const struct line_buffer *src_buffer, size_t y_start)
> +{
> +	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
> +	int x_start = frame_info->dst.x1;
> +	int count = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
> +
> +	wb->pixel_write(wb, src_buffer->pixels, count, x_start, y_start);
> +}
> +
>   /**
>    * clamp_line_coordinates() - Compute and clamp the coordinate to read and write during the blend
>    * process.
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 3870e825da81..526bf5207524 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -52,20 +52,25 @@ struct line_buffer {
>   	struct pixel_argb_u16 *pixels;
>   };
>   
> +struct vkms_writeback_job;
>   /**
> - * typedef pixel_write_t - These functions are used to read a pixel from a
> - * &struct pixel_argb_u16, convert it in a specific format and write it in the @dst_pixels
> - * buffer.
> + * typedef pixel_write_line_t - These functions are used to read a pixel line from a
> + * struct pixel_argb_u16 buffer, convert it and write it in the @wb job.
>    *
> - * @out_pixel: destination address to write the pixel
> - * @in_pixel: pixel to write
> + * @wb: the writeback job to write the output of the conversion
> + * @in_pixels: Source buffer containing the line to convert
> + * @count: The width of a line
> + * @x_start: The x (width) coordinate in the destination plane
> + * @y_start: The y (height) coordinate in the destination plane
>    */
> -typedef void (*pixel_write_t)(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel);
> +typedef void (*pixel_write_line_t)(struct vkms_writeback_job *wb,
> +			      struct pixel_argb_u16 *in_pixels, int count, int x_start,
> +			      int y_start);
>   
>   struct vkms_writeback_job {
>   	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
>   	struct vkms_frame_info wb_frame_info;
> -	pixel_write_t pixel_write;
> +	pixel_write_line_t pixel_write;
>   };
>   
>   /**
> @@ -232,7 +237,6 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
>   /* Composer Support */
>   void vkms_composer_worker(struct work_struct *work);
>   void vkms_set_composer(struct vkms_output *out, bool enabled);
> -void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
>   
>   /* Writeback */
>   int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index d1abfb1c3e3c..d1309f6d307f 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -587,7 +587,7 @@ static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_sta
>    * The following functions take one &struct pixel_argb_u16 and convert it to a specific format.
>    * The result is stored in @out_pixel.
>    *
> - * They are used in vkms_writeback_row() to convert and store a pixel from the src_buffer to
> + * They are used in the `write_line` functions to convert and store a pixel from the src_buffer to
>    * the writeback buffer.
>    */
>   static void argb_u16_to_ARGB8888(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel)
> @@ -654,28 +654,97 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
>   	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
>   }
>   
> -/**
> - * vkms_writeback_row() - Generic loop for all supported writeback format. It is executed just
> - * after the blending to write a line in the writeback buffer.
> +/*
> + * The following functions are write_line function for each pixel format supported by VKMS.
> + *
> + * They write a full line at index y. They must read data from the line src_pixels.
> + *
> + * The caller must ensure that count is not larger than the framebuffer and the src_pixels.
> + *
> + * Those function are very similar, but it is required for performance reason. In the past, some
> + * experiment were done, and with a generic loop the performance are very reduced [1].
>    *
> - * @wb: Job where to insert the final image
> - * @src_buffer: Line to write
> - * @y: Row to write in the writeback buffer
> + * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/

I'm not sure if a link is approriate here.

>    */
> -void vkms_writeback_row(struct vkms_writeback_job *wb,
> -			const struct line_buffer *src_buffer, int y)
> +
> +static void ARGB8888_write_line(struct vkms_writeback_job *wb,
> +				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> +				int y_start)
>   {
> -	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
> -	int x_dst = frame_info->dst.x1;
>   	u8 *dst_pixels;
> -	int rem_x, rem_y;
>   
> -	packed_pixels_addr(frame_info, x_dst, y, 0, &dst_pixels, &rem_x, &rem_y);
> -	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
> -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
>   
> -	for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->fb->format->cpp[0])
> -		wb->pixel_write(dst_pixels, &in_pixels[x]);
> +	while (count) {
> +		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> +		src_pixels += 1;
> +		count--;
> +	}
> +}
> +
> +static void XRGB8888_write_line(struct vkms_writeback_job *wb,
> +				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> +				int y_start)
> +{
> +	u8 *dst_pixels;
> +
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> +
> +	while (count) {
> +		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> +		src_pixels += 1;
> +		count--;
> +	}
> +}
> +
> +static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
> +				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> +				    int y_start)
> +{
> +	u8 *dst_pixels;
> +
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> +
> +	while (count) {
> +		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> +		src_pixels += 1;
> +		count--;
> +	}
> +}
> +
> +static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
> +				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> +				    int y_start)
> +{
> +	u8 *dst_pixels;
> +
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> +
> +	while (count) {
> +		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> +		src_pixels += 1;
> +		count--;
> +	}
> +}
> +
> +static void RGB565_write_line(struct vkms_writeback_job *wb,
> +			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
> +			      int y_start)
> +{
> +	u8 *dst_pixels;
> +
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> +
> +	while (count) {
> +		argb_u16_to_RGB565(dst_pixels, src_pixels);
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> +		src_pixels += 1;
> +		count--;
> +	}
>   }
>   
>   /**
> @@ -936,25 +1005,25 @@ void get_conversion_matrix_to_argb_u16(u32 format,
>   }
>   
>   /**
> - * get_pixel_write_function() - Retrieve the correct write_pixel function for a specific format.
> + * get_pixel_write_function() - Retrieve the correct write_line function for a specific format.

Correct the docs.

Best Regards,
- Maíra

>    * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
>    * pointer is valid before using it in a vkms_writeback_job.
>    *
>    * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
>    */
> -pixel_write_t get_pixel_write_function(u32 format)
> +pixel_write_line_t get_pixel_write_line_function(u32 format) >   {
>   	switch (format) {
>   	case DRM_FORMAT_ARGB8888:
> -		return &argb_u16_to_ARGB8888;
> +		return &ARGB8888_write_line;
>   	case DRM_FORMAT_XRGB8888:
> -		return &argb_u16_to_XRGB8888;
> +		return &XRGB8888_write_line;
>   	case DRM_FORMAT_ARGB16161616:
> -		return &argb_u16_to_ARGB16161616;
> +		return &ARGB16161616_write_line;
>   	case DRM_FORMAT_XRGB16161616:
> -		return &argb_u16_to_XRGB16161616;
> +		return &XRGB16161616_write_line;
>   	case DRM_FORMAT_RGB565:
> -		return &argb_u16_to_RGB565;
> +		return &RGB565_write_line;
>   	default:
>   		/*
>   		 * This is a bug in vkms_writeback_atomic_check. All the supported
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index eeb208cdd6b1..852ab9a4cee5 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -7,7 +7,7 @@
>   
>   pixel_read_line_t get_pixel_read_line_function(u32 format);
>   
> -pixel_write_t get_pixel_write_function(u32 format);
> +pixel_write_line_t get_pixel_write_line_function(u32 format);
>   
>   void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>   				       enum drm_color_range range,
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index c8582df1f739..f6ed3aa69af8 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -150,7 +150,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>   	crtc_state->wb_pending = true;
>   	spin_unlock_irq(&output->composer_lock);
>   	drm_writeback_queue_job(wb_conn, connector_state);
> -	active_wb->pixel_write = get_pixel_write_function(wb_format);
> +	active_wb->pixel_write = get_pixel_write_line_function(wb_format);
>   	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
>   	drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height);
>   }
> 

Re: [PATCH v2 2/3] drm/vkms: Add a macro for write_line functions

[Maíra Canal]

Hi Louis,

On 14/08/24 05:42, Louis Chauvet wrote:
> As stated in [2], the write_line functions are very similar and force code

Where is [2]?

> duplication. This patch add a macro to avoid code repetition.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>   drivers/gpu/drm/vkms/vkms_formats.c | 107 ++++++++++--------------------------
>   1 file changed, 30 insertions(+), 77 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index d1309f6d307f..a25cdf656d8a 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -654,6 +654,31 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
>   	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
>   }
>   
> +/**
> + * WRITE_LINE() - Generic generator for write_line functions
> + *
> + * This generator can only be used for format with only one plane and block_w == block_h == 1
> + *
> + * @function_name: Name to use for the generated function
> + * @conversion_function: Fonction to use for the conversion from argb_u16 to the required format.

s/Fonction/Function

> + */
> +#define WRITE_LINE(function_name, conversion_function)					\
> +static void function_name(struct vkms_writeback_job *wb,				\
> +			  struct pixel_argb_u16 *src_pixels, int count, int x_start,	\
> +			  int y_start)							\
> +{											\
> +	u8 *dst_pixels;									\
> +											\
> +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);	\
> +											\
> +	while (count) {									\
> +		(conversion_function)(dst_pixels, src_pixels);				\
> +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];		\
> +		src_pixels += 1;							\
> +		count--;								\

Just a nit: What do you think about this loop?

for (; count > 0; src_pixels++, count--)

It doesn't really matter what option you pick.

Best Regards,
- Maíra

> +	}										\
> +}
> +
>   /*
>    * The following functions are write_line function for each pixel format supported by VKMS.
>    *
> @@ -667,85 +692,13 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
>    * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
>    */
>   
> -static void ARGB8888_write_line(struct vkms_writeback_job *wb,
> -				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> -				int y_start)
> -{
> -	u8 *dst_pixels;
> +WRITE_LINE(ARGB8888_write_line, argb_u16_to_ARGB8888)
> +WRITE_LINE(XRGB8888_write_line, argb_u16_to_XRGB8888)
>   
> -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> +WRITE_LINE(ARGB16161616_write_line, argb_u16_to_ARGB16161616)
> +WRITE_LINE(XRGB16161616_write_line, argb_u16_to_XRGB16161616)
>   
> -	while (count) {
> -		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
> -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> -		src_pixels += 1;
> -		count--;
> -	}
> -}
> -
> -static void XRGB8888_write_line(struct vkms_writeback_job *wb,
> -				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> -				int y_start)
> -{
> -	u8 *dst_pixels;
> -
> -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> -
> -	while (count) {
> -		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
> -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> -		src_pixels += 1;
> -		count--;
> -	}
> -}
> -
> -static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
> -				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> -				    int y_start)
> -{
> -	u8 *dst_pixels;
> -
> -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> -
> -	while (count) {
> -		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
> -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> -		src_pixels += 1;
> -		count--;
> -	}
> -}
> -
> -static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
> -				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> -				    int y_start)
> -{
> -	u8 *dst_pixels;
> -
> -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> -
> -	while (count) {
> -		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
> -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> -		src_pixels += 1;
> -		count--;
> -	}
> -}
> -
> -static void RGB565_write_line(struct vkms_writeback_job *wb,
> -			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
> -			      int y_start)
> -{
> -	u8 *dst_pixels;
> -
> -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> -
> -	while (count) {
> -		argb_u16_to_RGB565(dst_pixels, src_pixels);
> -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> -		src_pixels += 1;
> -		count--;
> -	}
> -}
> +WRITE_LINE(RGB565_write_line, argb_u16_to_RGB565)
>   
>   /**
>    * get_pixel_read_function() - Retrieve the correct read_line function for a specific
> 

Re: [PATCH v2 2/3] drm/vkms: Add a macro for write_line functions

[Louis Chauvet]

On 26/10/24 - 12:08, Maíra Canal wrote:
> Hi Louis,
> 
> On 14/08/24 05:42, Louis Chauvet wrote:
> > As stated in [2], the write_line functions are very similar and force code
> 
> Where is [2]?
> 
> > duplication. This patch add a macro to avoid code repetition.
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_formats.c | 107 ++++++++++--------------------------
> >   1 file changed, 30 insertions(+), 77 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index d1309f6d307f..a25cdf656d8a 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -654,6 +654,31 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
> >   	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
> >   }
> > +/**
> > + * WRITE_LINE() - Generic generator for write_line functions
> > + *
> > + * This generator can only be used for format with only one plane and block_w == block_h == 1
> > + *
> > + * @function_name: Name to use for the generated function
> > + * @conversion_function: Fonction to use for the conversion from argb_u16 to the required format.
> 
> s/Fonction/Function
> 
> > + */
> > +#define WRITE_LINE(function_name, conversion_function)					\
> > +static void function_name(struct vkms_writeback_job *wb,				\
> > +			  struct pixel_argb_u16 *src_pixels, int count, int x_start,	\
> > +			  int y_start)							\
> > +{											\
> > +	u8 *dst_pixels;									\
> > +											\
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);	\
> > +											\
> > +	while (count) {									\
> > +		(conversion_function)(dst_pixels, src_pixels);				\
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];		\
> > +		src_pixels += 1;							\
> > +		count--;								\
> 
> Just a nit: What do you think about this loop?
> 
> for (; count > 0; src_pixels++, count--)
> 
> It doesn't really matter what option you pick.

I take this version, a bit shorter and not less explicit, thanks!

Thanks,
Louis Chauvet
 
> Best Regards,
> - Maíra
> 
> > +	}										\
> > +}
> > +
> >   /*
> >    * The following functions are write_line function for each pixel format supported by VKMS.
> >    *
> > @@ -667,85 +692,13 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
> >    * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
> >    */
> > -static void ARGB8888_write_line(struct vkms_writeback_job *wb,
> > -				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > -				int y_start)
> > -{
> > -	u8 *dst_pixels;
> > +WRITE_LINE(ARGB8888_write_line, argb_u16_to_ARGB8888)
> > +WRITE_LINE(XRGB8888_write_line, argb_u16_to_XRGB8888)
> > -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > +WRITE_LINE(ARGB16161616_write_line, argb_u16_to_ARGB16161616)
> > +WRITE_LINE(XRGB16161616_write_line, argb_u16_to_XRGB16161616)
> > -	while (count) {
> > -		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
> > -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > -		src_pixels += 1;
> > -		count--;
> > -	}
> > -}
> > -
> > -static void XRGB8888_write_line(struct vkms_writeback_job *wb,
> > -				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > -				int y_start)
> > -{
> > -	u8 *dst_pixels;
> > -
> > -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > -
> > -	while (count) {
> > -		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
> > -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > -		src_pixels += 1;
> > -		count--;
> > -	}
> > -}
> > -
> > -static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
> > -				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > -				    int y_start)
> > -{
> > -	u8 *dst_pixels;
> > -
> > -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > -
> > -	while (count) {
> > -		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
> > -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > -		src_pixels += 1;
> > -		count--;
> > -	}
> > -}
> > -
> > -static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
> > -				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > -				    int y_start)
> > -{
> > -	u8 *dst_pixels;
> > -
> > -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > -
> > -	while (count) {
> > -		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
> > -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > -		src_pixels += 1;
> > -		count--;
> > -	}
> > -}
> > -
> > -static void RGB565_write_line(struct vkms_writeback_job *wb,
> > -			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > -			      int y_start)
> > -{
> > -	u8 *dst_pixels;
> > -
> > -	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > -
> > -	while (count) {
> > -		argb_u16_to_RGB565(dst_pixels, src_pixels);
> > -		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > -		src_pixels += 1;
> > -		count--;
> > -	}
> > -}
> > +WRITE_LINE(RGB565_write_line, argb_u16_to_RGB565)
> >   /**
> >    * get_pixel_read_function() - Retrieve the correct read_line function for a specific
> > 

Re: [PATCH v2 1/3] drm/vkms: Re-introduce line-by-line algorithm for writeback

[Louis Chauvet]

On 26/10/24 - 12:05, Maíra Canal wrote:
> Hi Louis,
> 
> On 14/08/24 05:42, Louis Chauvet wrote:
> > Re-introduce a line-by-line writeback algorithm for each pixel format.
> > This allows more performance by not requiring an indirection per pixel
> > write.
> > 
> > Line-by-line writeback was introduced by [1] but rewritten back to
> > pixel-by-pixel algorithm in [2]. At this time, nobody noticed the impact
> > on performance, and it was merged.
> > 
> > This patch is almost a revert of [2], but with some effort to avoid code
> > duplication. Now only the loop is repeated, but it is required to have
> > good performances.
> > 
> > The performance gain is around 5 to 10%.
> > 
> > [1]: https://lore.kernel.org/all/20211005201637.58563-7-igormtorrente@gmail.com/
> > [2]: https://lore.kernel.org/all/20230515135204.115393-4-mcanal@igalia.com/
> > 
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >   drivers/gpu/drm/vkms/vkms_composer.c  |  17 +++++
> >   drivers/gpu/drm/vkms/vkms_drv.h       |  20 +++---
> >   drivers/gpu/drm/vkms/vkms_formats.c   | 117 +++++++++++++++++++++++++++-------
> >   drivers/gpu/drm/vkms/vkms_formats.h   |   2 +-
> >   drivers/gpu/drm/vkms/vkms_writeback.c |   2 +-
> >   5 files changed, 124 insertions(+), 34 deletions(-)
> > 
> > diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> > index 76d4aa8a0ef6..f0cae142ac22 100644
> > --- a/drivers/gpu/drm/vkms/vkms_composer.c
> > +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> > @@ -176,6 +176,23 @@ static enum pixel_read_direction direction_for_rotation(unsigned int rotation)
> >   	return READ_LEFT_TO_RIGHT;
> >   }
> > +/**
> > + * Write a line to the writeback buffer > + *
> > + * @wb: Job where to insert the final image
> > + * @src_buffer: Line to write
> > + * @y: Row to write in the writeback buffer
> > + */
> 
> Please, review the documentation using the kernel-doc format.
> 
> > +static void vkms_writeback_row(struct vkms_writeback_job *wb,
> > +			       const struct line_buffer *src_buffer, size_t y_start)
> > +{
> > +	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
> > +	int x_start = frame_info->dst.x1;
> > +	int count = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
> > +
> > +	wb->pixel_write(wb, src_buffer->pixels, count, x_start, y_start);
> > +}
> > +
> >   /**
> >    * clamp_line_coordinates() - Compute and clamp the coordinate to read and write during the blend
> >    * process.
> > diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> > index 3870e825da81..526bf5207524 100644
> > --- a/drivers/gpu/drm/vkms/vkms_drv.h
> > +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> > @@ -52,20 +52,25 @@ struct line_buffer {
> >   	struct pixel_argb_u16 *pixels;
> >   };
> > +struct vkms_writeback_job;
> >   /**
> > - * typedef pixel_write_t - These functions are used to read a pixel from a
> > - * &struct pixel_argb_u16, convert it in a specific format and write it in the @dst_pixels
> > - * buffer.
> > + * typedef pixel_write_line_t - These functions are used to read a pixel line from a
> > + * struct pixel_argb_u16 buffer, convert it and write it in the @wb job.
> >    *
> > - * @out_pixel: destination address to write the pixel
> > - * @in_pixel: pixel to write
> > + * @wb: the writeback job to write the output of the conversion
> > + * @in_pixels: Source buffer containing the line to convert
> > + * @count: The width of a line
> > + * @x_start: The x (width) coordinate in the destination plane
> > + * @y_start: The y (height) coordinate in the destination plane
> >    */
> > -typedef void (*pixel_write_t)(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel);
> > +typedef void (*pixel_write_line_t)(struct vkms_writeback_job *wb,
> > +			      struct pixel_argb_u16 *in_pixels, int count, int x_start,
> > +			      int y_start);
> >   struct vkms_writeback_job {
> >   	struct iosys_map data[DRM_FORMAT_MAX_PLANES];
> >   	struct vkms_frame_info wb_frame_info;
> > -	pixel_write_t pixel_write;
> > +	pixel_write_line_t pixel_write;
> >   };
> >   /**
> > @@ -232,7 +237,6 @@ int vkms_verify_crc_source(struct drm_crtc *crtc, const char *source_name,
> >   /* Composer Support */
> >   void vkms_composer_worker(struct work_struct *work);
> >   void vkms_set_composer(struct vkms_output *out, bool enabled);
> > -void vkms_writeback_row(struct vkms_writeback_job *wb, const struct line_buffer *src_buffer, int y);
> >   /* Writeback */
> >   int vkms_enable_writeback_connector(struct vkms_device *vkmsdev);
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> > index d1abfb1c3e3c..d1309f6d307f 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.c
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> > @@ -587,7 +587,7 @@ static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_sta
> >    * The following functions take one &struct pixel_argb_u16 and convert it to a specific format.
> >    * The result is stored in @out_pixel.
> >    *
> > - * They are used in vkms_writeback_row() to convert and store a pixel from the src_buffer to
> > + * They are used in the `write_line` functions to convert and store a pixel from the src_buffer to
> >    * the writeback buffer.
> >    */
> >   static void argb_u16_to_ARGB8888(u8 *out_pixel, const struct pixel_argb_u16 *in_pixel)
> > @@ -654,28 +654,97 @@ static void argb_u16_to_RGB565(u8 *out_pixel, const struct pixel_argb_u16 *in_pi
> >   	*pixel = cpu_to_le16(r << 11 | g << 5 | b);
> >   }
> > -/**
> > - * vkms_writeback_row() - Generic loop for all supported writeback format. It is executed just
> > - * after the blending to write a line in the writeback buffer.
> > +/*
> > + * The following functions are write_line function for each pixel format supported by VKMS.
> > + *
> > + * They write a full line at index y. They must read data from the line src_pixels.
> > + *
> > + * The caller must ensure that count is not larger than the framebuffer and the src_pixels.
> > + *
> > + * Those function are very similar, but it is required for performance reason. In the past, some
> > + * experiment were done, and with a generic loop the performance are very reduced [1].
> >    *
> > - * @wb: Job where to insert the final image
> > - * @src_buffer: Line to write
> > - * @y: Row to write in the writeback buffer
> > + * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
> 
> I'm not sure if a link is approriate here.

It was to have some context about the comment. If someone wants to modify 
this in the future (for example, factorise again the code :)), he will 
have some informations.
 
> >    */
> > -void vkms_writeback_row(struct vkms_writeback_job *wb,
> > -			const struct line_buffer *src_buffer, int y)
> > +
> > +static void ARGB8888_write_line(struct vkms_writeback_job *wb,
> > +				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > +				int y_start)
> >   {
> > -	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
> > -	int x_dst = frame_info->dst.x1;
> >   	u8 *dst_pixels;
> > -	int rem_x, rem_y;
> > -	packed_pixels_addr(frame_info, x_dst, y, 0, &dst_pixels, &rem_x, &rem_y);
> > -	struct pixel_argb_u16 *in_pixels = src_buffer->pixels;
> > -	int x_limit = min_t(size_t, drm_rect_width(&frame_info->dst), src_buffer->n_pixels);
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > -	for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->fb->format->cpp[0])
> > -		wb->pixel_write(dst_pixels, &in_pixels[x]);
> > +	while (count) {
> > +		argb_u16_to_ARGB8888(dst_pixels, src_pixels);
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > +		src_pixels += 1;
> > +		count--;
> > +	}
> > +}
> > +
> > +static void XRGB8888_write_line(struct vkms_writeback_job *wb,
> > +				struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > +				int y_start)
> > +{
> > +	u8 *dst_pixels;
> > +
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > +
> > +	while (count) {
> > +		argb_u16_to_XRGB8888(dst_pixels, src_pixels);
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > +		src_pixels += 1;
> > +		count--;
> > +	}
> > +}
> > +
> > +static void ARGB16161616_write_line(struct vkms_writeback_job *wb,
> > +				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > +				    int y_start)
> > +{
> > +	u8 *dst_pixels;
> > +
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > +
> > +	while (count) {
> > +		argb_u16_to_ARGB16161616(dst_pixels, src_pixels);
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > +		src_pixels += 1;
> > +		count--;
> > +	}
> > +}
> > +
> > +static void XRGB16161616_write_line(struct vkms_writeback_job *wb,
> > +				    struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > +				    int y_start)
> > +{
> > +	u8 *dst_pixels;
> > +
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > +
> > +	while (count) {
> > +		argb_u16_to_XRGB16161616(dst_pixels, src_pixels);
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > +		src_pixels += 1;
> > +		count--;
> > +	}
> > +}
> > +
> > +static void RGB565_write_line(struct vkms_writeback_job *wb,
> > +			      struct pixel_argb_u16 *src_pixels, int count, int x_start,
> > +			      int y_start)
> > +{
> > +	u8 *dst_pixels;
> > +
> > +	packed_pixels_addr_1x1(&wb->wb_frame_info, x_start, y_start, 0, &dst_pixels);
> > +
> > +	while (count) {
> > +		argb_u16_to_RGB565(dst_pixels, src_pixels);
> > +		dst_pixels += wb->wb_frame_info.fb->format->char_per_block[0];
> > +		src_pixels += 1;
> > +		count--;
> > +	}
> >   }
> >   /**
> > @@ -936,25 +1005,25 @@ void get_conversion_matrix_to_argb_u16(u32 format,
> >   }
> >   /**
> > - * get_pixel_write_function() - Retrieve the correct write_pixel function for a specific format.
> > + * get_pixel_write_function() - Retrieve the correct write_line function for a specific format.
> 
> Correct the docs.

Nice catch!

Thanks,
Louis Chauvet

> Best Regards,
> - Maíra
> 
> >    * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
> >    * pointer is valid before using it in a vkms_writeback_job.
> >    *
> >    * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
> >    */
> > -pixel_write_t get_pixel_write_function(u32 format)
> > +pixel_write_line_t get_pixel_write_line_function(u32 format) >   {
> >   	switch (format) {
> >   	case DRM_FORMAT_ARGB8888:
> > -		return &argb_u16_to_ARGB8888;
> > +		return &ARGB8888_write_line;
> >   	case DRM_FORMAT_XRGB8888:
> > -		return &argb_u16_to_XRGB8888;
> > +		return &XRGB8888_write_line;
> >   	case DRM_FORMAT_ARGB16161616:
> > -		return &argb_u16_to_ARGB16161616;
> > +		return &ARGB16161616_write_line;
> >   	case DRM_FORMAT_XRGB16161616:
> > -		return &argb_u16_to_XRGB16161616;
> > +		return &XRGB16161616_write_line;
> >   	case DRM_FORMAT_RGB565:
> > -		return &argb_u16_to_RGB565;
> > +		return &RGB565_write_line;
> >   	default:
> >   		/*
> >   		 * This is a bug in vkms_writeback_atomic_check. All the supported
> > diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> > index eeb208cdd6b1..852ab9a4cee5 100644
> > --- a/drivers/gpu/drm/vkms/vkms_formats.h
> > +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> > @@ -7,7 +7,7 @@
> >   pixel_read_line_t get_pixel_read_line_function(u32 format);
> > -pixel_write_t get_pixel_write_function(u32 format);
> > +pixel_write_line_t get_pixel_write_line_function(u32 format);
> >   void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
> >   				       enum drm_color_range range,
> > diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> > index c8582df1f739..f6ed3aa69af8 100644
> > --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> > +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> > @@ -150,7 +150,7 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
> >   	crtc_state->wb_pending = true;
> >   	spin_unlock_irq(&output->composer_lock);
> >   	drm_writeback_queue_job(wb_conn, connector_state);
> > -	active_wb->pixel_write = get_pixel_write_function(wb_format);
> > +	active_wb->pixel_write = get_pixel_write_line_function(wb_format);
> >   	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
> >   	drm_rect_init(&wb_frame_info->dst, 0, 0, crtc_width, crtc_height);
> >   }
> >