[PATCH v2 0/7] Add YUV formats to VKMS

[PATCH v2 0/7] Add YUV formats to VKMS

[Arthur Grillo]

This patchset aims to add support for additional buffer YUV formats.
More specifically, it adds support to:

Semi-planar formats:

- NV12
- NV16
- NV24
- NV21
- NV61
- NV42

Planar formats:

- YUV440
- YUV422
- YUV444
- YVU440
- YVU422
- YVU444

These formats have more than one plane, and most have chroma
subsampling. These properties don't have support on VKMS, so I had to
work on this before.

To ensure that the conversions from YUV to RGB are working, I wrote a
KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
not yet merged, I took the setup part (Kconfig entry and .kunitfile)
from it.

Furthermore, I couldn't find any sources with the conversion matrices,
so I had to work out the values myself based on the ITU papers[2][3][4].
So, I'm not 100% sure if the values are accurate. I'd appreciate some
input if anyone has more knowledge in this area.

Also, I used two IGT tests to check if the formats were having a correct
conversion (all with the --extended flag):

- kms_plane@pixel_format
- kms_plane@pixel_format_source_clamping.

The nonsubsampled formats don't have support on IGT, so I sent a patch
fixing this[5].

Currently, this patchset does not add those formats to the writeback, as
it would require a rewrite of how the conversions are done (similar to
what was done on a previous patch[6]). So, I would like to review this
patchset before I start the work on this other part.

[1]: https://lore.kernel.org/all/20231108163647.106853-5-harry.wentland@amd.com/
[2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
[3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
[4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
[5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
[6]: https://lore.kernel.org/dri-devel/20230414135151.75975-2-mcanal@igalia.com/

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
Changes in v2:
- Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
  file (Maxime)
- Link to v1: https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd3455e0@riseup.net

---
Arthur Grillo (7):
      drm/vkms: Use drm_frame directly
      drm/vkms: Add support for multy-planar framebuffers
      drm/vkms: Add range and encoding properties to pixel_read function
      drm/vkms: Add chroma subsampling
      drm/vkms: Add YUV support
      drm/vkms: Drop YUV formats TODO
      drm/vkms: Create KUnit tests for YUV conversions

 Documentation/gpu/vkms.rst                    |   3 +-
 drivers/gpu/drm/vkms/Kconfig                  |  15 ++
 drivers/gpu/drm/vkms/Makefile                 |   1 +
 drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
 drivers/gpu/drm/vkms/tests/Makefile           |   3 +
 drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h               |   6 +-
 drivers/gpu/drm/vkms/vkms_formats.c           | 247 ++++++++++++++++++++++----
 drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
 drivers/gpu/drm/vkms/vkms_plane.c             |  26 ++-
 drivers/gpu/drm/vkms/vkms_writeback.c         |   5 -
 11 files changed, 426 insertions(+), 49 deletions(-)
---
base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
change-id: 20231226-vkms-yuv-6f7859f32df8

Best regards,
-- 
Arthur Grillo <arthurgrillo@riseup.net>

[PATCH v2 1/7] drm/vkms: Use drm_frame directly

[Arthur Grillo]

Remove intermidiary variables and access the variables directly from
drm_frame. These changes should be noop.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_drv.h       |  3 ---
 drivers/gpu/drm/vkms/vkms_formats.c   | 12 +++++++-----
 drivers/gpu/drm/vkms/vkms_plane.c     |  3 ---
 drivers/gpu/drm/vkms/vkms_writeback.c |  5 -----
 4 files changed, 7 insertions(+), 16 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 8f5710debb1e..b4b357447292 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -31,9 +31,6 @@ struct vkms_frame_info {
 	struct drm_rect rotated;
 	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
 	unsigned int rotation;
-	unsigned int offset;
-	unsigned int pitch;
-	unsigned int cpp;
 };
 
 struct pixel_argb_u16 {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 36046b12f296..172830a3936a 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -11,8 +11,10 @@
 
 static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
 {
-	return frame_info->offset + (y * frame_info->pitch)
-				  + (x * frame_info->cpp);
+	struct drm_framebuffer *fb = frame_info->fb;
+
+	return fb->offsets[0] + (y * fb->pitches[0])
+			      + (x * fb->format->cpp[0]);
 }
 
 /*
@@ -131,12 +133,12 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	u8 *src_pixels = get_packed_src_addr(frame_info, y);
 	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
 
-	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
+	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
 		int x_pos = get_x_position(frame_info, limit, x);
 
 		if (drm_rotation_90_or_270(frame_info->rotation))
 			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
-				+ frame_info->cpp * y;
+				+ frame_info->fb->format->cpp[0] * y;
 
 		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
 	}
@@ -223,7 +225,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
 	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);
 
-	for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->cpp)
+	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]);
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index e5c625ab8e3e..8f2c6ea419a3 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -125,9 +125,6 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated),
 			drm_rect_height(&frame_info->rotated), frame_info->rotation);
 
-	frame_info->offset = fb->offsets[0];
-	frame_info->pitch = fb->pitches[0];
-	frame_info->cpp = fb->format->cpp[0];
 	vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
 }
 
diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
index bc724cbd5e3a..c8582df1f739 100644
--- a/drivers/gpu/drm/vkms/vkms_writeback.c
+++ b/drivers/gpu/drm/vkms/vkms_writeback.c
@@ -149,11 +149,6 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
 	crtc_state->active_writeback = active_wb;
 	crtc_state->wb_pending = true;
 	spin_unlock_irq(&output->composer_lock);
-
-	wb_frame_info->offset = fb->offsets[0];
-	wb_frame_info->pitch = fb->pitches[0];
-	wb_frame_info->cpp = fb->format->cpp[0];
-
 	drm_writeback_queue_job(wb_conn, connector_state);
 	active_wb->pixel_write = get_pixel_write_function(wb_format);
 	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);

-- 
2.43.0

[PATCH v2 2/7] drm/vkms: Add support for multy-planar framebuffers

[Arthur Grillo]

Add support to multy-planar formats by adding an index argument to the
framebuffer data access functions.

Also, give all the planes to the conversion functions. This, for now,
should be noop, as all the supported formats have only one plane.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_drv.h     |  2 +-
 drivers/gpu/drm/vkms/vkms_formats.c | 73 +++++++++++++++++++++----------------
 2 files changed, 42 insertions(+), 33 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index b4b357447292..c38590562e4b 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -56,7 +56,7 @@ struct vkms_writeback_job {
 struct vkms_plane_state {
 	struct drm_shadow_plane_state base;
 	struct vkms_frame_info *frame_info;
-	void (*pixel_read)(u8 *src_buffer, struct pixel_argb_u16 *out_pixel);
+	void (*pixel_read)(u8 **src_buffer, struct pixel_argb_u16 *out_pixel);
 };
 
 struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 172830a3936a..5566a7cd7bb4 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -9,12 +9,12 @@
 
 #include "vkms_formats.h"
 
-static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
+static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y, size_t index)
 {
 	struct drm_framebuffer *fb = frame_info->fb;
 
-	return fb->offsets[0] + (y * fb->pitches[0])
-			      + (x * fb->format->cpp[0]);
+	return fb->offsets[index] + (y * fb->pitches[index])
+				  + (x * fb->format->cpp[index]);
 }
 
 /*
@@ -23,27 +23,25 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
  * @frame_info: Buffer metadata
  * @x: The x(width) coordinate of the 2D buffer
  * @y: The y(Heigth) coordinate of the 2D buffer
+ * @index: The index of the plane on the 2D buffer
  *
  * Takes the information stored in the frame_info, a pair of coordinates, and
- * returns the address of the first color channel.
- * This function assumes the channels are packed together, i.e. a color channel
- * comes immediately after another in the memory. And therefore, this function
- * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21).
+ * returns the address of the first color channel on the desired index.
  */
 static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
-				int x, int y)
+				int x, int y, size_t index)
 {
-	size_t offset = pixel_offset(frame_info, x, y);
+	size_t offset = pixel_offset(frame_info, x, y, index);
 
 	return (u8 *)frame_info->map[0].vaddr + offset;
 }
 
-static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y)
+static void *get_packed_src_addr(const struct vkms_frame_info *frame_info, int y, size_t index)
 {
 	int x_src = frame_info->src.x1 >> 16;
 	int y_src = y - frame_info->rotated.y1 + (frame_info->src.y1 >> 16);
 
-	return packed_pixels_addr(frame_info, x_src, y_src);
+	return packed_pixels_addr(frame_info, x_src, y_src, index);
 }
 
 static int get_x_position(const struct vkms_frame_info *frame_info, int limit, int x)
@@ -53,7 +51,7 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i
 	return x;
 }
 
-static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
+static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
 {
 	/*
 	 * The 257 is the "conversion ratio". This number is obtained by the
@@ -61,23 +59,23 @@ static void ARGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixe
 	 * the best color value in a pixel format with more possibilities.
 	 * A similar idea applies to others RGB color conversions.
 	 */
-	out_pixel->a = (u16)src_pixels[3] * 257;
-	out_pixel->r = (u16)src_pixels[2] * 257;
-	out_pixel->g = (u16)src_pixels[1] * 257;
-	out_pixel->b = (u16)src_pixels[0] * 257;
+	out_pixel->a = (u16)src_pixels[0][3] * 257;
+	out_pixel->r = (u16)src_pixels[0][2] * 257;
+	out_pixel->g = (u16)src_pixels[0][1] * 257;
+	out_pixel->b = (u16)src_pixels[0][0] * 257;
 }
 
-static void XRGB8888_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
+static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
 {
 	out_pixel->a = (u16)0xffff;
-	out_pixel->r = (u16)src_pixels[2] * 257;
-	out_pixel->g = (u16)src_pixels[1] * 257;
-	out_pixel->b = (u16)src_pixels[0] * 257;
+	out_pixel->r = (u16)src_pixels[0][2] * 257;
+	out_pixel->g = (u16)src_pixels[0][1] * 257;
+	out_pixel->b = (u16)src_pixels[0][0] * 257;
 }
 
-static void ARGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
+static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	u16 *pixels = (u16 *)src_pixels;
+	u16 *pixels = (u16 *)src_pixels[0];
 
 	out_pixel->a = le16_to_cpu(pixels[3]);
 	out_pixel->r = le16_to_cpu(pixels[2]);
@@ -85,9 +83,9 @@ static void ARGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_
 	out_pixel->b = le16_to_cpu(pixels[0]);
 }
 
-static void XRGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
+static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	u16 *pixels = (u16 *)src_pixels;
+	u16 *pixels = (u16 *)src_pixels[0];
 
 	out_pixel->a = (u16)0xffff;
 	out_pixel->r = le16_to_cpu(pixels[2]);
@@ -95,9 +93,9 @@ static void XRGB16161616_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_
 	out_pixel->b = le16_to_cpu(pixels[0]);
 }
 
-static void RGB565_to_argb_u16(u8 *src_pixels, struct pixel_argb_u16 *out_pixel)
+static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
 {
-	u16 *pixels = (u16 *)src_pixels;
+	u16 *pixels = (u16 *)src_pixels[0];
 
 	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));
@@ -130,17 +128,28 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 {
 	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
 	struct vkms_frame_info *frame_info = plane->frame_info;
-	u8 *src_pixels = get_packed_src_addr(frame_info, y);
+	const struct drm_format_info *frame_format = frame_info->fb->format;
 	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
+	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
 
-	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
+	for (size_t i = 0; i < frame_format->num_planes; i++)
+		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
+
+	for (size_t x = 0; x < limit; x++) {
 		int x_pos = get_x_position(frame_info, limit, x);
 
-		if (drm_rotation_90_or_270(frame_info->rotation))
-			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
-				+ frame_info->fb->format->cpp[0] * y;
+		if (drm_rotation_90_or_270(frame_info->rotation)) {
+			for (size_t i = 0; i < frame_format->num_planes; i++) {
+				src_pixels[i] = get_packed_src_addr(frame_info,
+								    x + frame_info->rotated.y1, i);
+				src_pixels[i] += frame_format->cpp[i] * y;
+			}
+		}
 
 		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
+
+		for (size_t i = 0; i < frame_format->num_planes; i++)
+			src_pixels[i] += frame_format->cpp[i];
 	}
 }
 
@@ -221,7 +230,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
 {
 	struct vkms_frame_info *frame_info = &wb->wb_frame_info;
 	int x_dst = frame_info->dst.x1;
-	u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y);
+	u8 *dst_pixels = packed_pixels_addr(frame_info, x_dst, y, 0);
 	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);
 

-- 
2.43.0

[PATCH v2 3/7] drm/vkms: Add range and encoding properties to pixel_read function

[Arthur Grillo]

Create range and encoding properties. This should be noop, as none of
the conversion functions need those properties.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_drv.h     |  3 ++-
 drivers/gpu/drm/vkms/vkms_formats.c | 20 ++++++++++++++------
 drivers/gpu/drm/vkms/vkms_plane.c   |  9 +++++++++
 3 files changed, 25 insertions(+), 7 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index c38590562e4b..51349a0c32d8 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -56,7 +56,8 @@ struct vkms_writeback_job {
 struct vkms_plane_state {
 	struct drm_shadow_plane_state base;
 	struct vkms_frame_info *frame_info;
-	void (*pixel_read)(u8 **src_buffer, struct pixel_argb_u16 *out_pixel);
+	void (*pixel_read)(u8 **src_buffer, struct pixel_argb_u16 *out_pixel,
+			   enum drm_color_encoding enconding, enum drm_color_range range);
 };
 
 struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 5566a7cd7bb4..0156372aa1ef 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -51,7 +51,8 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i
 	return x;
 }
 
-static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
+static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				 enum drm_color_encoding encoding, enum drm_color_range range)
 {
 	/*
 	 * The 257 is the "conversion ratio". This number is obtained by the
@@ -65,7 +66,8 @@ static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pix
 	out_pixel->b = (u16)src_pixels[0][0] * 257;
 }
 
-static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
+static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				 enum drm_color_encoding encoding, enum drm_color_range range)
 {
 	out_pixel->a = (u16)0xffff;
 	out_pixel->r = (u16)src_pixels[0][2] * 257;
@@ -73,7 +75,8 @@ static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pix
 	out_pixel->b = (u16)src_pixels[0][0] * 257;
 }
 
-static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
+static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				     enum drm_color_encoding encoding, enum drm_color_range range)
 {
 	u16 *pixels = (u16 *)src_pixels[0];
 
@@ -83,7 +86,8 @@ static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out
 	out_pixel->b = le16_to_cpu(pixels[0]);
 }
 
-static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
+static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				     enum drm_color_encoding encoding, enum drm_color_range range)
 {
 	u16 *pixels = (u16 *)src_pixels[0];
 
@@ -93,7 +97,8 @@ static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out
 	out_pixel->b = le16_to_cpu(pixels[0]);
 }
 
-static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
+static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+			       enum drm_color_encoding encoding, enum drm_color_range range)
 {
 	u16 *pixels = (u16 *)src_pixels[0];
 
@@ -132,6 +137,9 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
 	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
 
+	enum drm_color_encoding encoding = plane->base.base.color_encoding;
+	enum drm_color_range range = plane->base.base.color_range;
+
 	for (size_t i = 0; i < frame_format->num_planes; i++)
 		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
 
@@ -146,7 +154,7 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 			}
 		}
 
-		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
+		plane->pixel_read(src_pixels, &out_pixels[x_pos], encoding, range);
 
 		for (size_t i = 0; i < frame_format->num_planes; i++)
 			src_pixels[i] += frame_format->cpp[i];
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 8f2c6ea419a3..e87c80575b7d 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -212,5 +212,14 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
 	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
 					   DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
 
+	drm_plane_create_color_properties(&plane->base,
+					  BIT(DRM_COLOR_YCBCR_BT601) |
+					  BIT(DRM_COLOR_YCBCR_BT709) |
+					  BIT(DRM_COLOR_YCBCR_BT2020),
+					  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
+					  BIT(DRM_COLOR_YCBCR_FULL_RANGE),
+					  DRM_COLOR_YCBCR_BT601,
+					  DRM_COLOR_YCBCR_FULL_RANGE);
+
 	return plane;
 }

-- 
2.43.0

[PATCH v2 4/7] drm/vkms: Add chroma subsampling

[Arthur Grillo]

Add support to chroma subsampling. This should be noop, as all supported
formats do not have chroma subsampling.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 0156372aa1ef..098ed16f2104 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -26,12 +26,15 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
  * @index: The index of the plane on the 2D buffer
  *
  * Takes the information stored in the frame_info, a pair of coordinates, and
- * returns the address of the first color channel on the desired index.
+ * returns the address of the first color channel on the desired index. The
+ * format's specific subsample is applied.
  */
 static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
 				int x, int y, size_t index)
 {
-	size_t offset = pixel_offset(frame_info, x, y, index);
+	int vsub = index == 0 ? 1 : frame_info->fb->format->vsub;
+	int hsub = index == 0 ? 1 : frame_info->fb->format->hsub;
+	size_t offset = pixel_offset(frame_info, x / hsub, y / vsub, index);
 
 	return (u8 *)frame_info->map[0].vaddr + offset;
 }
@@ -146,18 +149,23 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
 	for (size_t x = 0; x < limit; x++) {
 		int x_pos = get_x_position(frame_info, limit, x);
 
+		bool shoud_inc = !((x + 1) % frame_format->num_planes);
+
 		if (drm_rotation_90_or_270(frame_info->rotation)) {
 			for (size_t i = 0; i < frame_format->num_planes; i++) {
 				src_pixels[i] = get_packed_src_addr(frame_info,
 								    x + frame_info->rotated.y1, i);
-				src_pixels[i] += frame_format->cpp[i] * y;
+				if (!i || shoud_inc)
+					src_pixels[i] += frame_format->cpp[i] * y;
 			}
 		}
 
 		plane->pixel_read(src_pixels, &out_pixels[x_pos], encoding, range);
 
-		for (size_t i = 0; i < frame_format->num_planes; i++)
-			src_pixels[i] += frame_format->cpp[i];
+		for (size_t i = 0; i < frame_format->num_planes; i++) {
+			if (!i || shoud_inc)
+				src_pixels[i] += frame_format->cpp[i];
+		}
 	}
 }
 

-- 
2.43.0

[PATCH v2 5/7] drm/vkms: Add YUV support

[Arthur Grillo]

Add support to the YUV formats bellow:

- NV12
- NV16
- NV24
- NV21
- NV61
- NV42
- YUV420
- YUV422
- YUV444
- YVU420
- YVU422
- YVU444

The conversion matrices of each encoding and range were obtained by
rounding the values of the original conversion matrices multiplied by
2^8. This is done to avoid the use of fixed point operations.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 147 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_formats.h |   4 +
 drivers/gpu/drm/vkms/vkms_plane.c   |  14 +++-
 3 files changed, 164 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 098ed16f2104..7c1a0ca322d9 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -119,6 +119,137 @@ static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel
 	out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
 }
 
+static void ycbcr2rgb(const s16 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r, u8 *g, u8 *b)
+{
+	s32 y_16, cb_16, cr_16;
+	s32 r_16, g_16, b_16;
+
+	y_16 =  y - y_offset;
+	cb_16 = cb - 128;
+	cr_16 = cr - 128;
+
+	r_16 = m[0][0] * y_16 + m[0][1] * cb_16 + m[0][2] * cr_16;
+	g_16 = m[1][0] * y_16 + m[1][1] * cb_16 + m[1][2] * cr_16;
+	b_16 = m[2][0] * y_16 + m[2][1] * cb_16 + m[2][2] * cr_16;
+
+	*r = clamp(r_16, 0, 0xffff) >> 8;
+	*g = clamp(g_16, 0, 0xffff) >> 8;
+	*b = clamp(b_16, 0, 0xffff) >> 8;
+}
+
+static void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pixel_yuv_u8 *yuv_u8,
+			       enum drm_color_encoding encoding, enum drm_color_range range)
+{
+	static const s16 bt601_full[3][3] = {
+		{256,   0,  359},
+		{256, -88, -183},
+		{256, 454,    0},
+	};
+	static const s16 bt601[3][3] = {
+		{298,    0,  409},
+		{298, -100, -208},
+		{298,  516,    0},
+	};
+	static const s16 rec709_full[3][3] = {
+		{256,   0,  408},
+		{256, -48, -120},
+		{256, 476,   0 },
+	};
+	static const s16 rec709[3][3] = {
+		{298,   0,  459},
+		{298, -55, -136},
+		{298, 541,    0},
+	};
+	static const s16 bt2020_full[3][3] = {
+		{256,   0,  377},
+		{256, -42, -146},
+		{256, 482,    0},
+	};
+	static const s16 bt2020[3][3] = {
+		{298,   0,  430},
+		{298, -48, -167},
+		{298, 548,    0},
+	};
+
+	u8 r = 0;
+	u8 g = 0;
+	u8 b = 0;
+	bool full = range == DRM_COLOR_YCBCR_FULL_RANGE;
+	unsigned int y_offset = full ? 0 : 16;
+
+	switch (encoding) {
+	case DRM_COLOR_YCBCR_BT601:
+		ycbcr2rgb(full ? bt601_full : bt601,
+			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
+		break;
+	case DRM_COLOR_YCBCR_BT709:
+		ycbcr2rgb(full ? rec709_full : rec709,
+			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
+		break;
+	case DRM_COLOR_YCBCR_BT2020:
+		ycbcr2rgb(full ? bt2020_full : bt2020,
+			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
+		break;
+	default:
+		pr_warn_once("Not supported color encoding\n");
+		break;
+	}
+
+	argb_u16->r = r * 257;
+	argb_u16->g = g * 257;
+	argb_u16->b = b * 257;
+}
+
+static void semi_planar_yuv_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+					enum drm_color_encoding encoding,
+					enum drm_color_range range)
+{
+	struct pixel_yuv_u8 yuv_u8;
+
+	yuv_u8.y = src_pixels[0][0];
+	yuv_u8.u = src_pixels[1][0];
+	yuv_u8.v = src_pixels[1][1];
+
+	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
+}
+
+static void semi_planar_yvu_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+					enum drm_color_encoding encoding,
+					enum drm_color_range range)
+{
+	struct pixel_yuv_u8 yuv_u8;
+
+	yuv_u8.y = src_pixels[0][0];
+	yuv_u8.v = src_pixels[1][0];
+	yuv_u8.u = src_pixels[1][1];
+
+	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
+}
+
+static void planar_yuv_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				   enum drm_color_encoding encoding, enum drm_color_range range)
+{
+	struct pixel_yuv_u8 yuv_u8;
+
+	yuv_u8.y = src_pixels[0][0];
+	yuv_u8.u = src_pixels[1][0];
+	yuv_u8.v = src_pixels[2][0];
+
+	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
+}
+
+static void planar_yvu_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
+				   enum drm_color_encoding encoding, enum drm_color_range range)
+{
+	struct pixel_yuv_u8 yuv_u8;
+
+	yuv_u8.y = src_pixels[0][0];
+	yuv_u8.v = src_pixels[1][0];
+	yuv_u8.u = src_pixels[2][0];
+
+	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
+}
+
 /**
  * vkms_compose_row - compose a single row of a plane
  * @stage_buffer: output line with the composed pixels
@@ -267,6 +398,22 @@ void *get_pixel_conversion_function(u32 format)
 		return &XRGB16161616_to_argb_u16;
 	case DRM_FORMAT_RGB565:
 		return &RGB565_to_argb_u16;
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV24:
+		return &semi_planar_yuv_to_argb_u16;
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV61:
+	case DRM_FORMAT_NV42:
+		return &semi_planar_yvu_to_argb_u16;
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YUV444:
+		return &planar_yuv_to_argb_u16;
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YVU444:
+		return &planar_yvu_to_argb_u16;
 	default:
 		return NULL;
 	}
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index cf59c2ed8e9a..a8b2f92bdcb5 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -9,4 +9,8 @@ void *get_pixel_conversion_function(u32 format);
 
 void *get_pixel_write_function(u32 format);
 
+struct pixel_yuv_u8 {
+	u8 y, u, v;
+};
+
 #endif /* _VKMS_FORMATS_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index e87c80575b7d..932736fc3ee9 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -17,7 +17,19 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_XRGB8888,
 	DRM_FORMAT_XRGB16161616,
 	DRM_FORMAT_ARGB16161616,
-	DRM_FORMAT_RGB565
+	DRM_FORMAT_RGB565,
+	DRM_FORMAT_NV12,
+	DRM_FORMAT_NV16,
+	DRM_FORMAT_NV24,
+	DRM_FORMAT_NV21,
+	DRM_FORMAT_NV61,
+	DRM_FORMAT_NV42,
+	DRM_FORMAT_YUV420,
+	DRM_FORMAT_YUV422,
+	DRM_FORMAT_YUV444,
+	DRM_FORMAT_YVU420,
+	DRM_FORMAT_YVU422,
+	DRM_FORMAT_YVU444
 };
 
 static struct drm_plane_state *

-- 
2.43.0

[PATCH v2 6/7] drm/vkms: Drop YUV formats TODO

[Arthur Grillo]

VKMS has support for YUV formats now. Remove the task from the TODO
list.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 Documentation/gpu/vkms.rst | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index ba04ac7c2167..13b866c3617c 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -122,8 +122,7 @@ There's lots of plane features we could add support for:
 
 - Scaling.
 
-- Additional buffer formats, especially YUV formats for video like NV12.
-  Low/high bpp RGB formats would also be interesting.
+- Additional buffer formats. Low/high bpp RGB formats would be interesting.
 
 - Async updates (currently only possible on cursor plane using the legacy
   cursor api).

-- 
2.43.0

[PATCH v2 7/7] drm/vkms: Create KUnit tests for YUV conversions

[Arthur Grillo]

Create KUnit tests to test the conversion between YUV and RGB. Test each
conversion and range combination with some common colors.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/Kconfig                  |  15 +++
 drivers/gpu/drm/vkms/Makefile                 |   1 +
 drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
 drivers/gpu/drm/vkms/tests/Makefile           |   3 +
 drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_formats.c           |   9 +-
 drivers/gpu/drm/vkms/vkms_formats.h           |   5 +
 7 files changed, 191 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index b9ecdebecb0b..5b0094ad41eb 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -13,3 +13,18 @@ config DRM_VKMS
 	  a VKMS.
 
 	  If M is selected the module will be called vkms.
+
+config DRM_VKMS_KUNIT_TESTS
+	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
+	depends on DRM_VKMS && KUNIT
+	default KUNIT_ALL_TESTS
+	help
+	  This builds unit tests for VKMS. This option is not useful for
+	  distributions or general kernels, but only for kernel
+	  developers working on VKMS.
+
+	  For more information on KUnit and unit tests in general,
+	  please refer to the KUnit documentation in
+	  Documentation/dev-tools/kunit/.
+
+	  If in doubt, say "N".
diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
index 1b28a6a32948..8d3e46dde635 100644
--- a/drivers/gpu/drm/vkms/Makefile
+++ b/drivers/gpu/drm/vkms/Makefile
@@ -9,3 +9,4 @@ vkms-y := \
 	vkms_writeback.o
 
 obj-$(CONFIG_DRM_VKMS) += vkms.o
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig b/drivers/gpu/drm/vkms/tests/.kunitconfig
new file mode 100644
index 000000000000..70e378228cbd
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_VKMS=y
+CONFIG_DRM_VKMS_KUNIT_TESTS=y
diff --git a/drivers/gpu/drm/vkms/tests/Makefile b/drivers/gpu/drm/vkms/tests/Makefile
new file mode 100644
index 000000000000..2d1df668569e
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/Makefile
@@ -0,0 +1,3 @@
+# SPDX-License-Identifier: GPL-2.0-only
+
+obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_format_test.o
diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
new file mode 100644
index 000000000000..f04b8169d5a2
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -0,0 +1,156 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <kunit/test.h>
+
+#include <drm/drm_fixed.h>
+#include <drm/drm_fourcc.h>
+#include <drm/drm_print.h>
+
+#include "../../drm_crtc_internal.h"
+
+#include "../vkms_drv.h"
+#include "../vkms_formats.h"
+
+#define TEST_BUFF_SIZE 50
+
+struct yuv_u8_to_argb_u16_case {
+	enum drm_color_encoding encoding;
+	enum drm_color_range range;
+	size_t n_colors;
+	struct format_pair {
+		char *name;
+		struct pixel_yuv_u8 yuv;
+		struct pixel_argb_u16 argb;
+	} colors[TEST_BUFF_SIZE];
+};
+
+static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
+	{
+		.encoding = DRM_COLOR_YCBCR_BT601,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 6,
+		.colors = {
+			{"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x4c, 0x55, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0x96, 0x2c, 0x15}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x1d, 0xff, 0x6b}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+	{
+		.encoding = DRM_COLOR_YCBCR_BT601,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 6,
+		.colors = {
+			{"white", {0xeb, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x7e, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x10, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x51, 0x5a, 0xf0}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0x91, 0x36, 0x22}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x29, 0xf0, 0x6e}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+	{
+		.encoding = DRM_COLOR_YCBCR_BT709,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x35, 0x63, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0xb6, 0x1e, 0x0c}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x12, 0xff, 0x74}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+	{
+		.encoding = DRM_COLOR_YCBCR_BT709,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{"white", {0xeb, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x7e, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x10, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x3f, 0x66, 0xf0}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0xad, 0x2a, 0x1a}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x20, 0xf0, 0x76}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+	{
+		.encoding = DRM_COLOR_YCBCR_BT2020,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x43, 0x5c, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0xad, 0x24, 0x0b}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x0f, 0xff, 0x76}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+	{
+		.encoding = DRM_COLOR_YCBCR_BT2020,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{"white", {0xeb, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
+			{"gray",  {0x7e, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
+			{"black", {0x10, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
+			{"red",   {0x4a, 0x61, 0xf0}, {0x0000, 0xffff, 0x0000, 0x0000}},
+			{"green", {0xa4, 0x2f, 0x19}, {0x0000, 0x0000, 0xffff, 0x0000}},
+			{"blue",  {0x1d, 0xf0, 0x77}, {0x0000, 0x0000, 0x0000, 0xffff}},
+		},
+	},
+};
+
+static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
+{
+	const struct yuv_u8_to_argb_u16_case *param = test->param_value;
+	struct pixel_argb_u16 argb;
+
+	for (size_t i = 0; i < param->n_colors; i++) {
+		const struct format_pair *color = &param->colors[i];
+
+		yuv_u8_to_argb_u16(&argb, &color->yuv, param->encoding, param->range);
+
+		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",
+				    color->name, color->argb.a, argb.a);
+		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.r, color->argb.r), 257,
+				    "On the R channel of the color %s expected 0x%04x, got 0x%04x",
+				    color->name, color->argb.r, argb.r);
+		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.g, color->argb.g), 257,
+				    "On the G channel of the color %s expected 0x%04x, got 0x%04x",
+				    color->name, color->argb.g, argb.g);
+		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.b, color->argb.b), 257,
+				    "On the B channel of the color %s expected 0x%04x, got 0x%04x",
+				    color->name, color->argb.b, argb.b);
+	}
+}
+
+
+static void vkms_format_test_yuv_u8_to_argb_u16_case_desc(struct yuv_u8_to_argb_u16_case *t,
+							  char *desc)
+{
+	snprintf(desc, KUNIT_PARAM_DESC_SIZE, "%s - %s",
+		 drm_get_color_encoding_name(t->encoding), drm_get_color_range_name(t->range));
+}
+
+KUNIT_ARRAY_PARAM(yuv_u8_to_argb_u16, yuv_u8_to_argb_u16_cases,
+		  vkms_format_test_yuv_u8_to_argb_u16_case_desc);
+
+static struct kunit_case vkms_format_test_cases[] = {
+	KUNIT_CASE_PARAM(vkms_format_test_yuv_u8_to_argb_u16, yuv_u8_to_argb_u16_gen_params),
+	{}
+};
+
+static struct kunit_suite vkms_format_test_suite = {
+	.name = "vkms-format",
+	.test_cases = vkms_format_test_cases,
+};
+
+kunit_test_suite(vkms_format_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 7c1a0ca322d9..e06bbd7c0a67 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -7,6 +7,8 @@
 #include <drm/drm_rect.h>
 #include <drm/drm_fixed.h>
 
+#include <kunit/visibility.h>
+
 #include "vkms_formats.h"
 
 static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y, size_t index)
@@ -137,8 +139,10 @@ static void ycbcr2rgb(const s16 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r,
 	*b = clamp(b_16, 0, 0xffff) >> 8;
 }
 
-static void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pixel_yuv_u8 *yuv_u8,
-			       enum drm_color_encoding encoding, enum drm_color_range range)
+VISIBLE_IF_KUNIT void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16,
+					 const struct pixel_yuv_u8 *yuv_u8,
+					 enum drm_color_encoding encoding,
+					 enum drm_color_range range)
 {
 	static const s16 bt601_full[3][3] = {
 		{256,   0,  359},
@@ -199,6 +203,7 @@ static void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pix
 	argb_u16->g = g * 257;
 	argb_u16->b = b * 257;
 }
+EXPORT_SYMBOL_IF_KUNIT(yuv_u8_to_argb_u16);
 
 static void semi_planar_yuv_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
 					enum drm_color_encoding encoding,
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index a8b2f92bdcb5..0cf835292cec 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -13,4 +13,9 @@ struct pixel_yuv_u8 {
 	u8 y, u, v;
 };
 
+#if IS_ENABLED(CONFIG_KUNIT)
+void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pixel_yuv_u8 *yuv_u8,
+			       enum drm_color_encoding encoding, enum drm_color_range range);
+#endif
+
 #endif /* _VKMS_FORMATS_H_ */

-- 
2.43.0

Re: [PATCH v2 0/7] Add YUV formats to VKMS

[Sebastian Wick]

On Wed, Jan 10, 2024 at 02:44:00PM -0300, Arthur Grillo wrote:
> This patchset aims to add support for additional buffer YUV formats.
> More specifically, it adds support to:
> 
> Semi-planar formats:
> 
> - NV12
> - NV16
> - NV24
> - NV21
> - NV61
> - NV42
> 
> Planar formats:
> 
> - YUV440
> - YUV422
> - YUV444
> - YVU440
> - YVU422
> - YVU444
> 
> These formats have more than one plane, and most have chroma
> subsampling. These properties don't have support on VKMS, so I had to
> work on this before.
> 
> To ensure that the conversions from YUV to RGB are working, I wrote a
> KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
> not yet merged, I took the setup part (Kconfig entry and .kunitfile)
> from it.
> 
> Furthermore, I couldn't find any sources with the conversion matrices,
> so I had to work out the values myself based on the ITU papers[2][3][4].
> So, I'm not 100% sure if the values are accurate. I'd appreciate some
> input if anyone has more knowledge in this area.

H.273 CICP [1] has concise descriptions of the required values for most
widely used formats and the colour python framework [2] also can be used
to quickly get to the desired information most of the time.

[1]: https://www.itu.int/rec/T-REC-H.273
[2]: https://www.colour-science.org/

> Also, I used two IGT tests to check if the formats were having a correct
> conversion (all with the --extended flag):
> 
> - kms_plane@pixel_format
> - kms_plane@pixel_format_source_clamping.
> 
> The nonsubsampled formats don't have support on IGT, so I sent a patch
> fixing this[5].
> 
> Currently, this patchset does not add those formats to the writeback, as
> it would require a rewrite of how the conversions are done (similar to
> what was done on a previous patch[6]). So, I would like to review this
> patchset before I start the work on this other part.
> 
> [1]: https://lore.kernel.org/all/20231108163647.106853-5-harry.wentland@amd.com/
> [2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
> [3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
> [4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
> [5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
> [6]: https://lore.kernel.org/dri-devel/20230414135151.75975-2-mcanal@igalia.com/
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
> Changes in v2:
> - Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
>   file (Maxime)
> - Link to v1: https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd3455e0@riseup.net
> 
> ---
> Arthur Grillo (7):
>       drm/vkms: Use drm_frame directly
>       drm/vkms: Add support for multy-planar framebuffers
>       drm/vkms: Add range and encoding properties to pixel_read function
>       drm/vkms: Add chroma subsampling
>       drm/vkms: Add YUV support
>       drm/vkms: Drop YUV formats TODO
>       drm/vkms: Create KUnit tests for YUV conversions
> 
>  Documentation/gpu/vkms.rst                    |   3 +-
>  drivers/gpu/drm/vkms/Kconfig                  |  15 ++
>  drivers/gpu/drm/vkms/Makefile                 |   1 +
>  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
>  drivers/gpu/drm/vkms/tests/Makefile           |   3 +
>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_drv.h               |   6 +-
>  drivers/gpu/drm/vkms/vkms_formats.c           | 247 ++++++++++++++++++++++----
>  drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
>  drivers/gpu/drm/vkms/vkms_plane.c             |  26 ++-
>  drivers/gpu/drm/vkms/vkms_writeback.c         |   5 -
>  11 files changed, 426 insertions(+), 49 deletions(-)
> ---
> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
> change-id: 20231226-vkms-yuv-6f7859f32df8
> 
> Best regards,
> -- 
> Arthur Grillo <arthurgrillo@riseup.net>
> 

Re: [PATCH v2 2/7] drm/vkms: Add support for multy-planar framebuffers

[Louis Chauvet]

Hello,

I think there are some bugs in this implementation of multi-planar 
support (and not mylty-planar, there is a typo in the title), especially 
for the "new" drm_format_info description which uses block_w and block_h.

I will propose two patches [1] solving these issues and hopefully also 
simplifying a bit the composition.

TBH I'm not an expert, it's my first ever contribution in DRM, so please 
don't hesitate to correct me if you thin I missunderstood something, it 
actually took me a bit of time to fully understand the whole series and 
how it interacted with the rest of the vkms driver.

> -static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
> +static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y, size_t index)
>  {
>  	struct drm_framebuffer *fb = frame_info->fb;
>  
> -	return fb->offsets[0] + (y * fb->pitches[0])
> -			      + (x * fb->format->cpp[0]);
> +	return fb->offsets[index] + (y * fb->pitches[index])
> +				  + (x * fb->format->cpp[index]);
>  }
>  
>  /*
> @@ -23,27 +23,25 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
>   * @frame_info: Buffer metadata
>   * @x: The x(width) coordinate of the 2D buffer
>   * @y: The y(Heigth) coordinate of the 2D buffer
> + * @index: The index of the plane on the 2D buffer
>   *
>   * Takes the information stored in the frame_info, a pair of coordinates, and
> - * returns the address of the first color channel.
> - * This function assumes the channels are packed together, i.e. a color channel
> - * comes immediately after another in the memory. And therefore, this function
> - * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21).
> + * returns the address of the first color channel on the desired index.
>   */
>  static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
> -				int x, int y)
> +				int x, int y, size_t index)
>  {
> -	size_t offset = pixel_offset(frame_info, x, y);
> +	size_t offset = pixel_offset(frame_info, x, y, index);
>  
>  	return (u8 *)frame_info->map[0].vaddr + offset;
>  }

This implementation of packed_pixels_addr will only work with
block_w == block_h == 1. For packed or tiled formats we will need to use
x/y information to extract the correct address, and this address will not 
be a single pixel. See below my explanation.

[...]

> @@ -130,17 +128,28 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  {
>  	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
>  	struct vkms_frame_info *frame_info = plane->frame_info;
> -	u8 *src_pixels = get_packed_src_addr(frame_info, y);
> +	const struct drm_format_info *frame_format = frame_info->fb->format;
>  	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
> +	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
>  
> -	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
> +	for (size_t i = 0; i < frame_format->num_planes; i++)
> +		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
> +
> +	for (size_t x = 0; x < limit; x++) {
>  		int x_pos = get_x_position(frame_info, limit, x);
>  
> -		if (drm_rotation_90_or_270(frame_info->rotation))
> -			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
> -				+ frame_info->fb->format->cpp[0] * y;
> +		if (drm_rotation_90_or_270(frame_info->rotation)) {
> +			for (size_t i = 0; i < frame_format->num_planes; i++) {
> +				src_pixels[i] = get_packed_src_addr(frame_info,
> +								    x + frame_info->rotated.y1, i);
> +				src_pixels[i] += frame_format->cpp[i] * y;

I find the current rotation management a bit complex to understand. This 
is not related to your patch, but as I had to understand this to create my 
second patch, I think this could be significanlty simplified.

Please see the below comment about frame_format->cpp, it applies here too. 
I think the "easy" way here is simply to reuse the method 
get_packed_src_addr every time you need a new pixel.

> +			}
> +		}
>  
>		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
> +

The usage of cpp and pointer to specific pixel only work for non-packed 
and non-blocked pixels, but for example NV30 or Y0L0 need more 
informations about the exact location of the pixel to convert and write 
the correct pixel value (each pixel can't be referenced directly by a 
pointer). For example NV30 uses 5 bytes to store 3 pixels (10 bits each), 
so to access the "middle" one you need to read the 5 bytes and do a small 
computation to extract it's value.

I think a simple solution to handle most cases would be to profide two 
more parameters: the x and y positions of the pixel to copy, using 
"absolute coordinates" (i.e x=0,y=0 means the first byte of the src 
buffer, not the first pixel in the `drm_rect src`, this way the method 
`pixel_read` can extract the correct value).

This way it become easy to manage "complex" pixel representations in this 
loop: simply increment x/y and let the pixel_read method handle 
everything.

The second patch I will send is doing this. And as explained before, it 
will also simplify a lot the code related to rotation and translation (no 
more switch case everywhere to add offset to x/y, it simply use drm_rect_* 
helpers).

It's not optimal in term of performance (in some situation it will read 
the same block multiple time to generate different pixels), but I 
believe it still is an intersting trade-off.

In the future, if performance is actally critical, the whole composition 
loop will have to be specialized for each pixel formats: some can be 
treated line by line (as it's done today), but with blocks or packed 
pixels it's more complex.

> +		for (size_t i = 0; i < frame_format->num_planes; i++)
> +			src_pixels[i] += frame_format->cpp[i];

This is likely working with format with block_w != 1, see explanation 
above.

[...]

[1]: https://lore.kernel.org/dri-devel/20240201-yuv-v1-0-3ca376f27632@bootlin.com/T/#t

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 4/7] drm/vkms: Add chroma subsampling

[Louis Chauvet]

[...]

> @@ -146,18 +149,23 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  	for (size_t x = 0; x < limit; x++) {
>  		int x_pos = get_x_position(frame_info, limit, x);
>  
> +		bool shoud_inc = !((x + 1) % frame_format->num_planes);

I think this line will break if the subsampling is not the same as the 
plane count. For NV12 it works only because there are two planes and 
hsub=2/vsub=2, but I believe NV24 will not work because of plane 2, as 
we need to increment x at the same speed on all planes.

I have a proposal to solve this issue (see my patchset applying on top of 
yours). You probably at least need to use .hsub/vsub to 
increment/decrement properly src_pixels pointer.

Currently the tests pass for it because it only use "horizontal 
lines" and "full color" pictures. 

In the series [1] I proposed to change the pattern to detect this kind of 
issue.

[...]

[1]: https://lore.kernel.org/dri-devel/20240201-yuv-v1-0-3ca376f27632@bootlin.com/T/#t

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 7/7] drm/vkms: Create KUnit tests for YUV conversions

[Louis Chauvet]

[...]

> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT709,
> +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> +		.n_colors = 4,
> +		.colors = {
> +			{"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
> +			{"gray",  {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
> +			{"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
> +			{"red",   {0x35, 0x63, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
> +			{"green", {0xb6, 0x1e, 0x0c}, {0x0000, 0x0000, 0xffff, 0x0000}},
> +			{"blue",  {0x12, 0xff, 0x74}, {0x0000, 0x0000, 0x0000, 0xffff}},
> +		},
> +	},
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT709,
> +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +		.n_colors = 4,

I think there is a mistake in n_colors here, if I understand correctly it 
must be the size of .colors, so here it should probably be 6?

> +		.colors = {
> +			{"white", {0xeb, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
> +			{"gray",  {0x7e, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
> +			{"black", {0x10, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
> +			{"red",   {0x3f, 0x66, 0xf0}, {0x0000, 0xffff, 0x0000, 0x0000}},
> +			{"green", {0xad, 0x2a, 0x1a}, {0x0000, 0x0000, 0xffff, 0x0000}},
> +			{"blue",  {0x20, 0xf0, 0x76}, {0x0000, 0x0000, 0x0000, 0xffff}},
> +		},
> +	},
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT2020,
> +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> +		.n_colors = 4,

Same here.

> +		.colors = {
> +			{"white", {0xff, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
> +			{"gray",  {0x80, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
> +			{"black", {0x00, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
> +			{"red",   {0x43, 0x5c, 0xff}, {0x0000, 0xffff, 0x0000, 0x0000}},
> +			{"green", {0xad, 0x24, 0x0b}, {0x0000, 0x0000, 0xffff, 0x0000}},
> +			{"blue",  {0x0f, 0xff, 0x76}, {0x0000, 0x0000, 0x0000, 0xffff}},
> +		},
> +	},
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT2020,
> +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +		.n_colors = 4,

Same here.

> +		.colors = {
> +			{"white", {0xeb, 0x80, 0x80}, {0x0000, 0xffff, 0xffff, 0xffff}},
> +			{"gray",  {0x7e, 0x80, 0x80}, {0x0000, 0x8000, 0x8000, 0x8000}},
> +			{"black", {0x10, 0x80, 0x80}, {0x0000, 0x0000, 0x0000, 0x0000}},
> +			{"red",   {0x4a, 0x61, 0xf0}, {0x0000, 0xffff, 0x0000, 0x0000}},
> +			{"green", {0xa4, 0x2f, 0x19}, {0x0000, 0x0000, 0xffff, 0x0000}},
> +			{"blue",  {0x1d, 0xf0, 0x77}, {0x0000, 0x0000, 0x0000, 0xffff}},
> +		},
> +	},
> +};

[...]

-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com

Re: [PATCH v2 1/7] drm/vkms: Use drm_frame directly

[Louis Chauvet]

Le 10/01/24 - 14:44, Arthur Grillo a écrit :
> Remove intermidiary variables and access the variables directly from
> drm_frame. These changes should be noop.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.h       |  3 ---
>  drivers/gpu/drm/vkms/vkms_formats.c   | 12 +++++++-----
>  drivers/gpu/drm/vkms/vkms_plane.c     |  3 ---
>  drivers/gpu/drm/vkms/vkms_writeback.c |  5 -----
>  4 files changed, 7 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 8f5710debb1e..b4b357447292 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -31,9 +31,6 @@ struct vkms_frame_info {
>  	struct drm_rect rotated;
>  	struct iosys_map map[DRM_FORMAT_MAX_PLANES];
>  	unsigned int rotation;
> -	unsigned int offset;
> -	unsigned int pitch;
> -	unsigned int cpp;
>  };
>  
>  struct pixel_argb_u16 {
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 36046b12f296..172830a3936a 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -11,8 +11,10 @@
>  
>  static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int y)
>  {
> -	return frame_info->offset + (y * frame_info->pitch)
> -				  + (x * frame_info->cpp);
> +	struct drm_framebuffer *fb = frame_info->fb;
> +
> +	return fb->offsets[0] + (y * fb->pitches[0])
> +			      + (x * fb->format->cpp[0]);
>  }
>  
>  /*
> @@ -131,12 +133,12 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  	u8 *src_pixels = get_packed_src_addr(frame_info, y);
>  	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
>  
> -	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->cpp) {
> +	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
>  		int x_pos = get_x_position(frame_info, limit, x);
>  
>  		if (drm_rotation_90_or_270(frame_info->rotation))
>  			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
> -				+ frame_info->cpp * y;
> +				+ frame_info->fb->format->cpp[0] * y;
>  
>  		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
>  	}
> @@ -223,7 +225,7 @@ void vkms_writeback_row(struct vkms_writeback_job *wb,
>  	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);
>  
> -	for (size_t x = 0; x < x_limit; x++, dst_pixels += frame_info->cpp)
> +	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]);
>  }
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index e5c625ab8e3e..8f2c6ea419a3 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -125,9 +125,6 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
>  	drm_rect_rotate(&frame_info->rotated, drm_rect_width(&frame_info->rotated),
>  			drm_rect_height(&frame_info->rotated), frame_info->rotation);
>  
> -	frame_info->offset = fb->offsets[0];
> -	frame_info->pitch = fb->pitches[0];
> -	frame_info->cpp = fb->format->cpp[0];
>  	vkms_plane_state->pixel_read = get_pixel_conversion_function(fmt);
>  }
>  
> diff --git a/drivers/gpu/drm/vkms/vkms_writeback.c b/drivers/gpu/drm/vkms/vkms_writeback.c
> index bc724cbd5e3a..c8582df1f739 100644
> --- a/drivers/gpu/drm/vkms/vkms_writeback.c
> +++ b/drivers/gpu/drm/vkms/vkms_writeback.c
> @@ -149,11 +149,6 @@ static void vkms_wb_atomic_commit(struct drm_connector *conn,
>  	crtc_state->active_writeback = active_wb;
>  	crtc_state->wb_pending = true;
>  	spin_unlock_irq(&output->composer_lock);
> -
> -	wb_frame_info->offset = fb->offsets[0];
> -	wb_frame_info->pitch = fb->pitches[0];
> -	wb_frame_info->cpp = fb->format->cpp[0];
> -
>  	drm_writeback_queue_job(wb_conn, connector_state);
>  	active_wb->pixel_write = get_pixel_write_function(wb_format);
>  	drm_rect_init(&wb_frame_info->src, 0, 0, crtc_width, crtc_height);
> 
> -- 
> 2.43.0
> 

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

Re: [PATCH v2 3/7] drm/vkms: Add range and encoding properties to pixel_read function

[Louis Chauvet]

Le 10/01/24 - 14:44, Arthur Grillo a écrit :
> Create range and encoding properties. This should be noop, as none of
> the conversion functions need those properties.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/vkms_drv.h     |  3 ++-
>  drivers/gpu/drm/vkms/vkms_formats.c | 20 ++++++++++++++------
>  drivers/gpu/drm/vkms/vkms_plane.c   |  9 +++++++++
>  3 files changed, 25 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index c38590562e4b..51349a0c32d8 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -56,7 +56,8 @@ struct vkms_writeback_job {
>  struct vkms_plane_state {
>  	struct drm_shadow_plane_state base;
>  	struct vkms_frame_info *frame_info;
> -	void (*pixel_read)(u8 **src_buffer, struct pixel_argb_u16 *out_pixel);
> +	void (*pixel_read)(u8 **src_buffer, struct pixel_argb_u16 *out_pixel,
> +			   enum drm_color_encoding enconding, enum drm_color_range range);
>  };
>  
>  struct vkms_plane {
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 5566a7cd7bb4..0156372aa1ef 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -51,7 +51,8 @@ static int get_x_position(const struct vkms_frame_info *frame_info, int limit, i
>  	return x;
>  }
>  
> -static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				 enum drm_color_encoding encoding, enum drm_color_range range)
>  {
>  	/*
>  	 * The 257 is the "conversion ratio". This number is obtained by the
> @@ -65,7 +66,8 @@ static void ARGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pix
>  	out_pixel->b = (u16)src_pixels[0][0] * 257;
>  }
>  
> -static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				 enum drm_color_encoding encoding, enum drm_color_range range)
>  {
>  	out_pixel->a = (u16)0xffff;
>  	out_pixel->r = (u16)src_pixels[0][2] * 257;
> @@ -73,7 +75,8 @@ static void XRGB8888_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pix
>  	out_pixel->b = (u16)src_pixels[0][0] * 257;
>  }
>  
> -static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				     enum drm_color_encoding encoding, enum drm_color_range range)
>  {
>  	u16 *pixels = (u16 *)src_pixels[0];
>  
> @@ -83,7 +86,8 @@ static void ARGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out
>  	out_pixel->b = le16_to_cpu(pixels[0]);
>  }
>  
> -static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				     enum drm_color_encoding encoding, enum drm_color_range range)
>  {
>  	u16 *pixels = (u16 *)src_pixels[0];
>  
> @@ -93,7 +97,8 @@ static void XRGB16161616_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out
>  	out_pixel->b = le16_to_cpu(pixels[0]);
>  }
>  
> -static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel)
> +static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +			       enum drm_color_encoding encoding, enum drm_color_range range)
>  {
>  	u16 *pixels = (u16 *)src_pixels[0];
>  
> @@ -132,6 +137,9 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
>  	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
>  
> +	enum drm_color_encoding encoding = plane->base.base.color_encoding;
> +	enum drm_color_range range = plane->base.base.color_range;
> +
>  	for (size_t i = 0; i < frame_format->num_planes; i++)
>  		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
>  
> @@ -146,7 +154,7 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>  			}
>  		}
>  
> -		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
> +		plane->pixel_read(src_pixels, &out_pixels[x_pos], encoding, range);
>  
>  		for (size_t i = 0; i < frame_format->num_planes; i++)
>  			src_pixels[i] += frame_format->cpp[i];
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index 8f2c6ea419a3..e87c80575b7d 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -212,5 +212,14 @@ struct vkms_plane *vkms_plane_init(struct vkms_device *vkmsdev,
>  	drm_plane_create_rotation_property(&plane->base, DRM_MODE_ROTATE_0,
>  					   DRM_MODE_ROTATE_MASK | DRM_MODE_REFLECT_MASK);
>  
> +	drm_plane_create_color_properties(&plane->base,
> +					  BIT(DRM_COLOR_YCBCR_BT601) |
> +					  BIT(DRM_COLOR_YCBCR_BT709) |
> +					  BIT(DRM_COLOR_YCBCR_BT2020),
> +					  BIT(DRM_COLOR_YCBCR_LIMITED_RANGE) |
> +					  BIT(DRM_COLOR_YCBCR_FULL_RANGE),
> +					  DRM_COLOR_YCBCR_BT601,
> +					  DRM_COLOR_YCBCR_FULL_RANGE);
> +
>  	return plane;
>  }
> 
> -- 
> 2.43.0
> 

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

Re: [PATCH v2 5/7] drm/vkms: Add YUV support

[Louis Chauvet]

Le 10/01/24 - 14:44, Arthur Grillo a écrit :
> Add support to the YUV formats bellow:
> 
> - NV12
> - NV16
> - NV24
> - NV21
> - NV61
> - NV42
> - YUV420
> - YUV422
> - YUV444
> - YVU420
> - YVU422
> - YVU444
> 
> The conversion matrices of each encoding and range were obtained by
> rounding the values of the original conversion matrices multiplied by
> 2^8. This is done to avoid the use of fixed point operations.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/vkms_formats.c | 147 ++++++++++++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_formats.h |   4 +
>  drivers/gpu/drm/vkms/vkms_plane.c   |  14 +++-
>  3 files changed, 164 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 098ed16f2104..7c1a0ca322d9 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -119,6 +119,137 @@ static void RGB565_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel
>  	out_pixel->b = drm_fixp2int_round(drm_fixp_mul(fp_b, fp_rb_ratio));
>  }
>  
> +static void ycbcr2rgb(const s16 m[3][3], u8 y, u8 cb, u8 cr, u8 y_offset, u8 *r, u8 *g, u8 *b)
> +{
> +	s32 y_16, cb_16, cr_16;
> +	s32 r_16, g_16, b_16;
> +
> +	y_16 =  y - y_offset;
> +	cb_16 = cb - 128;
> +	cr_16 = cr - 128;
> +
> +	r_16 = m[0][0] * y_16 + m[0][1] * cb_16 + m[0][2] * cr_16;
> +	g_16 = m[1][0] * y_16 + m[1][1] * cb_16 + m[1][2] * cr_16;
> +	b_16 = m[2][0] * y_16 + m[2][1] * cb_16 + m[2][2] * cr_16;
> +
> +	*r = clamp(r_16, 0, 0xffff) >> 8;
> +	*g = clamp(g_16, 0, 0xffff) >> 8;
> +	*b = clamp(b_16, 0, 0xffff) >> 8;
> +}
> +
> +static void yuv_u8_to_argb_u16(struct pixel_argb_u16 *argb_u16, const struct pixel_yuv_u8 *yuv_u8,
> +			       enum drm_color_encoding encoding, enum drm_color_range range)
> +{
> +	static const s16 bt601_full[3][3] = {
> +		{256,   0,  359},
> +		{256, -88, -183},
> +		{256, 454,    0},
> +	};
> +	static const s16 bt601[3][3] = {
> +		{298,    0,  409},
> +		{298, -100, -208},
> +		{298,  516,    0},
> +	};
> +	static const s16 rec709_full[3][3] = {
> +		{256,   0,  408},
> +		{256, -48, -120},
> +		{256, 476,   0 },
> +	};
> +	static const s16 rec709[3][3] = {
> +		{298,   0,  459},
> +		{298, -55, -136},
> +		{298, 541,    0},
> +	};
> +	static const s16 bt2020_full[3][3] = {
> +		{256,   0,  377},
> +		{256, -42, -146},
> +		{256, 482,    0},
> +	};
> +	static const s16 bt2020[3][3] = {
> +		{298,   0,  430},
> +		{298, -48, -167},
> +		{298, 548,    0},
> +	};
> +
> +	u8 r = 0;
> +	u8 g = 0;
> +	u8 b = 0;
> +	bool full = range == DRM_COLOR_YCBCR_FULL_RANGE;
> +	unsigned int y_offset = full ? 0 : 16;
> +
> +	switch (encoding) {
> +	case DRM_COLOR_YCBCR_BT601:
> +		ycbcr2rgb(full ? bt601_full : bt601,
> +			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> +		break;
> +	case DRM_COLOR_YCBCR_BT709:
> +		ycbcr2rgb(full ? rec709_full : rec709,
> +			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> +		break;
> +	case DRM_COLOR_YCBCR_BT2020:
> +		ycbcr2rgb(full ? bt2020_full : bt2020,
> +			  yuv_u8->y, yuv_u8->u, yuv_u8->v, y_offset, &r, &g, &b);
> +		break;
> +	default:
> +		pr_warn_once("Not supported color encoding\n");
> +		break;
> +	}
> +
> +	argb_u16->r = r * 257;
> +	argb_u16->g = g * 257;
> +	argb_u16->b = b * 257;
> +}
> +
> +static void semi_planar_yuv_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +					enum drm_color_encoding encoding,
> +					enum drm_color_range range)
> +{
> +	struct pixel_yuv_u8 yuv_u8;
> +
> +	yuv_u8.y = src_pixels[0][0];
> +	yuv_u8.u = src_pixels[1][0];
> +	yuv_u8.v = src_pixels[1][1];
> +
> +	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
> +}
> +
> +static void semi_planar_yvu_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +					enum drm_color_encoding encoding,
> +					enum drm_color_range range)
> +{
> +	struct pixel_yuv_u8 yuv_u8;
> +
> +	yuv_u8.y = src_pixels[0][0];
> +	yuv_u8.v = src_pixels[1][0];
> +	yuv_u8.u = src_pixels[1][1];
> +
> +	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
> +}
> +
> +static void planar_yuv_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				   enum drm_color_encoding encoding, enum drm_color_range range)
> +{
> +	struct pixel_yuv_u8 yuv_u8;
> +
> +	yuv_u8.y = src_pixels[0][0];
> +	yuv_u8.u = src_pixels[1][0];
> +	yuv_u8.v = src_pixels[2][0];
> +
> +	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
> +}
> +
> +static void planar_yvu_to_argb_u16(u8 **src_pixels, struct pixel_argb_u16 *out_pixel,
> +				   enum drm_color_encoding encoding, enum drm_color_range range)
> +{
> +	struct pixel_yuv_u8 yuv_u8;
> +
> +	yuv_u8.y = src_pixels[0][0];
> +	yuv_u8.v = src_pixels[1][0];
> +	yuv_u8.u = src_pixels[2][0];
> +
> +	yuv_u8_to_argb_u16(out_pixel, &yuv_u8, encoding, range);
> +}
> +
>  /**
>   * vkms_compose_row - compose a single row of a plane
>   * @stage_buffer: output line with the composed pixels
> @@ -267,6 +398,22 @@ void *get_pixel_conversion_function(u32 format)
>  		return &XRGB16161616_to_argb_u16;
>  	case DRM_FORMAT_RGB565:
>  		return &RGB565_to_argb_u16;
> +	case DRM_FORMAT_NV12:
> +	case DRM_FORMAT_NV16:
> +	case DRM_FORMAT_NV24:
> +		return &semi_planar_yuv_to_argb_u16;
> +	case DRM_FORMAT_NV21:
> +	case DRM_FORMAT_NV61:
> +	case DRM_FORMAT_NV42:
> +		return &semi_planar_yvu_to_argb_u16;
> +	case DRM_FORMAT_YUV420:
> +	case DRM_FORMAT_YUV422:
> +	case DRM_FORMAT_YUV444:
> +		return &planar_yuv_to_argb_u16;
> +	case DRM_FORMAT_YVU420:
> +	case DRM_FORMAT_YVU422:
> +	case DRM_FORMAT_YVU444:
> +		return &planar_yvu_to_argb_u16;
>  	default:
>  		return NULL;
>  	}
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index cf59c2ed8e9a..a8b2f92bdcb5 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -9,4 +9,8 @@ void *get_pixel_conversion_function(u32 format);
>  
>  void *get_pixel_write_function(u32 format);
>  
> +struct pixel_yuv_u8 {
> +	u8 y, u, v;
> +};
> +
>  #endif /* _VKMS_FORMATS_H_ */
> diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
> index e87c80575b7d..932736fc3ee9 100644
> --- a/drivers/gpu/drm/vkms/vkms_plane.c
> +++ b/drivers/gpu/drm/vkms/vkms_plane.c
> @@ -17,7 +17,19 @@ static const u32 vkms_formats[] = {
>  	DRM_FORMAT_XRGB8888,
>  	DRM_FORMAT_XRGB16161616,
>  	DRM_FORMAT_ARGB16161616,
> -	DRM_FORMAT_RGB565
> +	DRM_FORMAT_RGB565,
> +	DRM_FORMAT_NV12,
> +	DRM_FORMAT_NV16,
> +	DRM_FORMAT_NV24,
> +	DRM_FORMAT_NV21,
> +	DRM_FORMAT_NV61,
> +	DRM_FORMAT_NV42,
> +	DRM_FORMAT_YUV420,
> +	DRM_FORMAT_YUV422,
> +	DRM_FORMAT_YUV444,
> +	DRM_FORMAT_YVU420,
> +	DRM_FORMAT_YVU422,
> +	DRM_FORMAT_YVU444
>  };
>  
>  static struct drm_plane_state *
> 
> -- 
> 2.43.0
> 

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

Re: [PATCH v2 6/7] drm/vkms: Drop YUV formats TODO

[Louis Chauvet]

Le 10/01/24 - 14:44, Arthur Grillo a écrit :
> VKMS has support for YUV formats now. Remove the task from the TODO
> list.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  Documentation/gpu/vkms.rst | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index ba04ac7c2167..13b866c3617c 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -122,8 +122,7 @@ There's lots of plane features we could add support for:
>  
>  - Scaling.
>  
> -- Additional buffer formats, especially YUV formats for video like NV12.
> -  Low/high bpp RGB formats would also be interesting.
> +- Additional buffer formats. Low/high bpp RGB formats would be interesting.
>  
>  - Async updates (currently only possible on cursor plane using the legacy
>    cursor api).
> 
> -- 
> 2.43.0
> 

(Sorry Arthur for the double mail, I miss the reply-all in the previous 
mail)

Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>

Re: [PATCH v2 2/7] drm/vkms: Add support for multy-planar framebuffers

[Arthur Grillo]

On 01/02/24 14:38, Louis Chauvet wrote:
>>  
>>  /*
>> @@ -23,27 +23,25 @@ static size_t pixel_offset(const struct vkms_frame_info *frame_info, int x, int
>>   * @frame_info: Buffer metadata
>>   * @x: The x(width) coordinate of the 2D buffer
>>   * @y: The y(Heigth) coordinate of the 2D buffer
>> + * @index: The index of the plane on the 2D buffer
>>   *
>>   * Takes the information stored in the frame_info, a pair of coordinates, and
>> - * returns the address of the first color channel.
>> - * This function assumes the channels are packed together, i.e. a color channel
>> - * comes immediately after another in the memory. And therefore, this function
>> - * doesn't work for YUV with chroma subsampling (e.g. YUV420 and NV21).
>> + * returns the address of the first color channel on the desired index.
>>   */
>>  static void *packed_pixels_addr(const struct vkms_frame_info *frame_info,
>> -				int x, int y)
>> +				int x, int y, size_t index)
>>  {
>> -	size_t offset = pixel_offset(frame_info, x, y);
>> +	size_t offset = pixel_offset(frame_info, x, y, index);
>>  
>>  	return (u8 *)frame_info->map[0].vaddr + offset;
>>  }
> 
> This implementation of packed_pixels_addr will only work with
> block_w == block_h == 1. For packed or tiled formats we will need to use
> x/y information to extract the correct address, and this address will not 
> be a single pixel. See below my explanation.

You're right, currently, VKMS only supports non-packed/tiled formats. As
all the formats I plan to add are too not packed or tiled, I haven't
added support to it. But if you want to add it, please do :).

>> @@ -130,17 +128,28 @@ void vkms_compose_row(struct line_buffer *stage_buffer, struct vkms_plane_state
>>  {
>>  	struct pixel_argb_u16 *out_pixels = stage_buffer->pixels;
>>  	struct vkms_frame_info *frame_info = plane->frame_info;
>> -	u8 *src_pixels = get_packed_src_addr(frame_info, y);
>> +	const struct drm_format_info *frame_format = frame_info->fb->format;
>>  	int limit = min_t(size_t, drm_rect_width(&frame_info->dst), stage_buffer->n_pixels);
>> +	u8 *src_pixels[DRM_FORMAT_MAX_PLANES];
>>  
>> -	for (size_t x = 0; x < limit; x++, src_pixels += frame_info->fb->format->cpp[0]) {
>> +	for (size_t i = 0; i < frame_format->num_planes; i++)
>> +		src_pixels[i] = get_packed_src_addr(frame_info, y, i);
>> +
>> +	for (size_t x = 0; x < limit; x++) {
>>  		int x_pos = get_x_position(frame_info, limit, x);
>>  
>> -		if (drm_rotation_90_or_270(frame_info->rotation))
>> -			src_pixels = get_packed_src_addr(frame_info, x + frame_info->rotated.y1)
>> -				+ frame_info->fb->format->cpp[0] * y;
>> +		if (drm_rotation_90_or_270(frame_info->rotation)) {
>> +			for (size_t i = 0; i < frame_format->num_planes; i++) {
>> +				src_pixels[i] = get_packed_src_addr(frame_info,
>> +								    x + frame_info->rotated.y1, i);
>> +				src_pixels[i] += frame_format->cpp[i] * y;
> 
> I find the current rotation management a bit complex to understand. This 
> is not related to your patch, but as I had to understand this to create my 
> second patch, I think this could be significanlty simplified.

I also found the rotation logic complex when implementing this. I would
appreciate it if it were simplified.

> 
> Please see the below comment about frame_format->cpp, it applies here too. 
> I think the "easy" way here is simply to reuse the method 
> get_packed_src_addr every time you need a new pixel.
> 
>> +			}
>> +		}
>>  
>> 		plane->pixel_read(src_pixels, &out_pixels[x_pos]);
>> +
> 
> The usage of cpp and pointer to specific pixel only work for non-packed 
> and non-blocked pixels, but for example NV30 or Y0L0 need more 
> informations about the exact location of the pixel to convert and write 
> the correct pixel value (each pixel can't be referenced directly by a 
> pointer). For example NV30 uses 5 bytes to store 3 pixels (10 bits each), 
> so to access the "middle" one you need to read the 5 bytes and do a small 
> computation to extract it's value.

Great explanation, I can see what is the problem here.

> 
> I think a simple solution to handle most cases would be to profide two 
> more parameters: the x and y positions of the pixel to copy, using 
> "absolute coordinates" (i.e x=0,y=0 means the first byte of the src 
> buffer, not the first pixel in the `drm_rect src`, this way the method 
> `pixel_read` can extract the correct value).
> 
> This way it become easy to manage "complex" pixel representations in this 
> loop: simply increment x/y and let the pixel_read method handle 
> everything.
> 
> The second patch I will send is doing this. And as explained before, it 
> will also simplify a lot the code related to rotation and translation (no 
> more switch case everywhere to add offset to x/y, it simply use drm_rect_* 
> helpers).

I like this, expect my review soon :).

> 
> It's not optimal in term of performance (in some situation it will read 
> the same block multiple time to generate different pixels), but I 
> believe it still is an intersting trade-off.
> 
> In the future, if performance is actally critical, the whole composition 
> loop will have to be specialized for each pixel formats: some can be 
> treated line by line (as it's done today), but with blocks or packed 
> pixels it's more complex.
> 
>> +		for (size_t i = 0; i < frame_format->num_planes; i++)
>> +			src_pixels[i] += frame_format->cpp[i];
> 
> This is likely working with format with block_w != 1, see explanation 
> above.

I think you meant that is _not_ working. Yeah, as I already explained,
it was never my plan to add support for packed or tiled formats.

Best Regards,
~Arthur Grillo

Re: [PATCH v2 0/7] Add YUV formats to VKMS

[Arthur Grillo]

On 15/01/24 12:06, Sebastian Wick wrote:
> On Wed, Jan 10, 2024 at 02:44:00PM -0300, Arthur Grillo wrote:
>> This patchset aims to add support for additional buffer YUV formats.
>> More specifically, it adds support to:
>>
>> Semi-planar formats:
>>
>> - NV12
>> - NV16
>> - NV24
>> - NV21
>> - NV61
>> - NV42
>>
>> Planar formats:
>>
>> - YUV440
>> - YUV422
>> - YUV444
>> - YVU440
>> - YVU422
>> - YVU444
>>
>> These formats have more than one plane, and most have chroma
>> subsampling. These properties don't have support on VKMS, so I had to
>> work on this before.
>>
>> To ensure that the conversions from YUV to RGB are working, I wrote a
>> KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
>> not yet merged, I took the setup part (Kconfig entry and .kunitfile)
>> from it.
>>
>> Furthermore, I couldn't find any sources with the conversion matrices,
>> so I had to work out the values myself based on the ITU papers[2][3][4].
>> So, I'm not 100% sure if the values are accurate. I'd appreciate some
>> input if anyone has more knowledge in this area.
> 
> H.273 CICP [1] has concise descriptions of the required values for most
> widely used formats and the colour python framework [2] also can be used
> to quickly get to the desired information most of the time.
> 
> [1]: https://www.itu.int/rec/T-REC-H.273
> [2]: https://www.colour-science.org/

I want to thank you for suggesting the python framework, it helped
immensely now that I'm changing the precision from 8-bit to 32-bit[1].

If I'd known about this framework while developing this patch, I
would've saved myself a few weeks of pain and suffering recreating a
part of this library XD.

[1]: https://lore.kernel.org/all/b23da076-0bfb-48b2-9386-383a6dec1868@riseup.net/

Best Regards,
~Arthur Grillo

> 
>> Also, I used two IGT tests to check if the formats were having a correct
>> conversion (all with the --extended flag):
>>
>> - kms_plane@pixel_format
>> - kms_plane@pixel_format_source_clamping.
>>
>> The nonsubsampled formats don't have support on IGT, so I sent a patch
>> fixing this[5].
>>
>> Currently, this patchset does not add those formats to the writeback, as
>> it would require a rewrite of how the conversions are done (similar to
>> what was done on a previous patch[6]). So, I would like to review this
>> patchset before I start the work on this other part.
>>
>> [1]: https://lore.kernel.org/all/20231108163647.106853-5-harry.wentland@amd.com/
>> [2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
>> [3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
>> [4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
>> [5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
>> [6]: https://lore.kernel.org/dri-devel/20230414135151.75975-2-mcanal@igalia.com/
>>
>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>> Changes in v2:
>> - Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
>>   file (Maxime)
>> - Link to v1: https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd3455e0@riseup.net
>>
>> ---
>> Arthur Grillo (7):
>>       drm/vkms: Use drm_frame directly
>>       drm/vkms: Add support for multy-planar framebuffers
>>       drm/vkms: Add range and encoding properties to pixel_read function
>>       drm/vkms: Add chroma subsampling
>>       drm/vkms: Add YUV support
>>       drm/vkms: Drop YUV formats TODO
>>       drm/vkms: Create KUnit tests for YUV conversions
>>
>>  Documentation/gpu/vkms.rst                    |   3 +-
>>  drivers/gpu/drm/vkms/Kconfig                  |  15 ++
>>  drivers/gpu/drm/vkms/Makefile                 |   1 +
>>  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
>>  drivers/gpu/drm/vkms/tests/Makefile           |   3 +
>>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++
>>  drivers/gpu/drm/vkms/vkms_drv.h               |   6 +-
>>  drivers/gpu/drm/vkms/vkms_formats.c           | 247 ++++++++++++++++++++++----
>>  drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
>>  drivers/gpu/drm/vkms/vkms_plane.c             |  26 ++-
>>  drivers/gpu/drm/vkms/vkms_writeback.c         |   5 -
>>  11 files changed, 426 insertions(+), 49 deletions(-)
>> ---
>> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
>> change-id: 20231226-vkms-yuv-6f7859f32df8
>>
>> Best regards,
>> -- 
>> Arthur Grillo <arthurgrillo@riseup.net>
>>
> 

Re: [PATCH v2 0/7] Add YUV formats to VKMS

[Sebastian Wick]

On Wed, Feb 28, 2024 at 08:42:41PM -0300, Arthur Grillo wrote:
> 
> 
> On 15/01/24 12:06, Sebastian Wick wrote:
> > On Wed, Jan 10, 2024 at 02:44:00PM -0300, Arthur Grillo wrote:
> >> This patchset aims to add support for additional buffer YUV formats.
> >> More specifically, it adds support to:
> >>
> >> Semi-planar formats:
> >>
> >> - NV12
> >> - NV16
> >> - NV24
> >> - NV21
> >> - NV61
> >> - NV42
> >>
> >> Planar formats:
> >>
> >> - YUV440
> >> - YUV422
> >> - YUV444
> >> - YVU440
> >> - YVU422
> >> - YVU444
> >>
> >> These formats have more than one plane, and most have chroma
> >> subsampling. These properties don't have support on VKMS, so I had to
> >> work on this before.
> >>
> >> To ensure that the conversions from YUV to RGB are working, I wrote a
> >> KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
> >> not yet merged, I took the setup part (Kconfig entry and .kunitfile)
> >> from it.
> >>
> >> Furthermore, I couldn't find any sources with the conversion matrices,
> >> so I had to work out the values myself based on the ITU papers[2][3][4].
> >> So, I'm not 100% sure if the values are accurate. I'd appreciate some
> >> input if anyone has more knowledge in this area.
> > 
> > H.273 CICP [1] has concise descriptions of the required values for most
> > widely used formats and the colour python framework [2] also can be used
> > to quickly get to the desired information most of the time.
> > 
> > [1]: https://www.itu.int/rec/T-REC-H.273
> > [2]: https://www.colour-science.org/
> 
> I want to thank you for suggesting the python framework, it helped
> immensely now that I'm changing the precision from 8-bit to 32-bit[1].
> 
> If I'd known about this framework while developing this patch, I
> would've saved myself a few weeks of pain and suffering recreating a
> part of this library XD.

I'm glad this is useful for you! We also used it for our color-and-hdr
project https://gitlab.freedesktop.org/pq/color-and-hdr/.

> [1]: https://lore.kernel.org/all/b23da076-0bfb-48b2-9386-383a6dec1868@riseup.net/
> 
> Best Regards,
> ~Arthur Grillo
> 
> > 
> >> Also, I used two IGT tests to check if the formats were having a correct
> >> conversion (all with the --extended flag):
> >>
> >> - kms_plane@pixel_format
> >> - kms_plane@pixel_format_source_clamping.
> >>
> >> The nonsubsampled formats don't have support on IGT, so I sent a patch
> >> fixing this[5].
> >>
> >> Currently, this patchset does not add those formats to the writeback, as
> >> it would require a rewrite of how the conversions are done (similar to
> >> what was done on a previous patch[6]). So, I would like to review this
> >> patchset before I start the work on this other part.
> >>
> >> [1]: https://lore.kernel.org/all/20231108163647.106853-5-harry.wentland@amd.com/
> >> [2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
> >> [3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
> >> [4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
> >> [5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
> >> [6]: https://lore.kernel.org/dri-devel/20230414135151.75975-2-mcanal@igalia.com/
> >>
> >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> >> ---
> >> Changes in v2:
> >> - Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
> >>   file (Maxime)
> >> - Link to v1: https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd3455e0@riseup.net
> >>
> >> ---
> >> Arthur Grillo (7):
> >>       drm/vkms: Use drm_frame directly
> >>       drm/vkms: Add support for multy-planar framebuffers
> >>       drm/vkms: Add range and encoding properties to pixel_read function
> >>       drm/vkms: Add chroma subsampling
> >>       drm/vkms: Add YUV support
> >>       drm/vkms: Drop YUV formats TODO
> >>       drm/vkms: Create KUnit tests for YUV conversions
> >>
> >>  Documentation/gpu/vkms.rst                    |   3 +-
> >>  drivers/gpu/drm/vkms/Kconfig                  |  15 ++
> >>  drivers/gpu/drm/vkms/Makefile                 |   1 +
> >>  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
> >>  drivers/gpu/drm/vkms/tests/Makefile           |   3 +
> >>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++
> >>  drivers/gpu/drm/vkms/vkms_drv.h               |   6 +-
> >>  drivers/gpu/drm/vkms/vkms_formats.c           | 247 ++++++++++++++++++++++----
> >>  drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
> >>  drivers/gpu/drm/vkms/vkms_plane.c             |  26 ++-
> >>  drivers/gpu/drm/vkms/vkms_writeback.c         |   5 -
> >>  11 files changed, 426 insertions(+), 49 deletions(-)
> >> ---
> >> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
> >> change-id: 20231226-vkms-yuv-6f7859f32df8
> >>
> >> Best regards,
> >> -- 
> >> Arthur Grillo <arthurgrillo@riseup.net>
> >>
> > 
> 

Re: [PATCH v2 0/7] Add YUV formats to VKMS

[Arthur Grillo]

On 29/02/24 14:52, Sebastian Wick wrote:
> On Wed, Feb 28, 2024 at 08:42:41PM -0300, Arthur Grillo wrote:
>>
>>
>> On 15/01/24 12:06, Sebastian Wick wrote:
>>> On Wed, Jan 10, 2024 at 02:44:00PM -0300, Arthur Grillo wrote:
>>>> This patchset aims to add support for additional buffer YUV formats.
>>>> More specifically, it adds support to:
>>>>
>>>> Semi-planar formats:
>>>>
>>>> - NV12
>>>> - NV16
>>>> - NV24
>>>> - NV21
>>>> - NV61
>>>> - NV42
>>>>
>>>> Planar formats:
>>>>
>>>> - YUV440
>>>> - YUV422
>>>> - YUV444
>>>> - YVU440
>>>> - YVU422
>>>> - YVU444
>>>>
>>>> These formats have more than one plane, and most have chroma
>>>> subsampling. These properties don't have support on VKMS, so I had to
>>>> work on this before.
>>>>
>>>> To ensure that the conversions from YUV to RGB are working, I wrote a
>>>> KUnit test. As the work from Harry on creating KUnit tests on VKMS[1] is
>>>> not yet merged, I took the setup part (Kconfig entry and .kunitfile)
>>>> from it.
>>>>
>>>> Furthermore, I couldn't find any sources with the conversion matrices,
>>>> so I had to work out the values myself based on the ITU papers[2][3][4].
>>>> So, I'm not 100% sure if the values are accurate. I'd appreciate some
>>>> input if anyone has more knowledge in this area.
>>>
>>> H.273 CICP [1] has concise descriptions of the required values for most
>>> widely used formats and the colour python framework [2] also can be used
>>> to quickly get to the desired information most of the time.
>>>
>>> [1]: https://www.itu.int/rec/T-REC-H.273
>>> [2]: https://www.colour-science.org/
>>
>> I want to thank you for suggesting the python framework, it helped
>> immensely now that I'm changing the precision from 8-bit to 32-bit[1].
>>
>> If I'd known about this framework while developing this patch, I
>> would've saved myself a few weeks of pain and suffering recreating a
>> part of this library XD.
> 
> I'm glad this is useful for you! We also used it for our color-and-hdr
> project https://gitlab.freedesktop.org/pq/color-and-hdr/.

Cool project! I'll be sure to give look!

Best Regards,
~Arthur Grillo

> 
>> [1]: https://lore.kernel.org/all/b23da076-0bfb-48b2-9386-383a6dec1868@riseup.net/
>>
>> Best Regards,
>> ~Arthur Grillo
>>
>>>
>>>> Also, I used two IGT tests to check if the formats were having a correct
>>>> conversion (all with the --extended flag):
>>>>
>>>> - kms_plane@pixel_format
>>>> - kms_plane@pixel_format_source_clamping.
>>>>
>>>> The nonsubsampled formats don't have support on IGT, so I sent a patch
>>>> fixing this[5].
>>>>
>>>> Currently, this patchset does not add those formats to the writeback, as
>>>> it would require a rewrite of how the conversions are done (similar to
>>>> what was done on a previous patch[6]). So, I would like to review this
>>>> patchset before I start the work on this other part.
>>>>
>>>> [1]: https://lore.kernel.org/all/20231108163647.106853-5-harry.wentland@amd.com/
>>>> [2]: https://www.itu.int/rec/R-REC-BT.601-7-201103-I/en
>>>> [3]: https://www.itu.int/rec/R-REC-BT.709-6-201506-I/en
>>>> [4]: https://www.itu.int/rec/R-REC-BT.2020-2-201510-I/en
>>>> [5]: https://lists.freedesktop.org/archives/igt-dev/2024-January/066937.html
>>>> [6]: https://lore.kernel.org/dri-devel/20230414135151.75975-2-mcanal@igalia.com/
>>>>
>>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>>>> ---
>>>> Changes in v2:
>>>> - Use EXPORT_SYMBOL_IF_KUNIT instead of including the .c test
>>>>   file (Maxime)
>>>> - Link to v1: https://lore.kernel.org/r/20240105-vkms-yuv-v1-0-34c4cd3455e0@riseup.net
>>>>
>>>> ---
>>>> Arthur Grillo (7):
>>>>       drm/vkms: Use drm_frame directly
>>>>       drm/vkms: Add support for multy-planar framebuffers
>>>>       drm/vkms: Add range and encoding properties to pixel_read function
>>>>       drm/vkms: Add chroma subsampling
>>>>       drm/vkms: Add YUV support
>>>>       drm/vkms: Drop YUV formats TODO
>>>>       drm/vkms: Create KUnit tests for YUV conversions
>>>>
>>>>  Documentation/gpu/vkms.rst                    |   3 +-
>>>>  drivers/gpu/drm/vkms/Kconfig                  |  15 ++
>>>>  drivers/gpu/drm/vkms/Makefile                 |   1 +
>>>>  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
>>>>  drivers/gpu/drm/vkms/tests/Makefile           |   3 +
>>>>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 156 ++++++++++++++++
>>>>  drivers/gpu/drm/vkms/vkms_drv.h               |   6 +-
>>>>  drivers/gpu/drm/vkms/vkms_formats.c           | 247 ++++++++++++++++++++++----
>>>>  drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
>>>>  drivers/gpu/drm/vkms/vkms_plane.c             |  26 ++-
>>>>  drivers/gpu/drm/vkms/vkms_writeback.c         |   5 -
>>>>  11 files changed, 426 insertions(+), 49 deletions(-)
>>>> ---
>>>> base-commit: eeb8e8d9f124f279e80ae679f4ba6e822ce4f95f
>>>> change-id: 20231226-vkms-yuv-6f7859f32df8
>>>>
>>>> Best regards,
>>>> -- 
>>>> Arthur Grillo <arthurgrillo@riseup.net>
>>>>
>>>
>>
>