[PATCH v16 0/7] drm/vkms: Add support for YUV and DRM_FORMAT_R*

[PATCH v16 0/7] drm/vkms: Add support for YUV and DRM_FORMAT_R*

[Louis Chauvet]

This patchset is extracted from [1]. The goal is to introduce the YUV
support, thanks to Arthur's work.

- PATCH 1: Add the support of YUV formats
- PATCH 2: Add some drm properties to expose more YUV features
- PATCH 3: Cleanup the todo
- PATCH 4..6: Add some kunit tests
- PATCH 7: Add the support of DRM_FORMAT_R1/2/4/8

[1]: https://lore.kernel.org/r/20241007-yuv-v12-0-01c1ada6fec8@bootlin.com

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
Changes in v16:
- Rebased on drm-misc-next
- Updated comment and changed fail thresholds. 
- Link to v15: https://lore.kernel.org/r/20241231-yuv-v15-0-eda6bb3028e6@bootlin.com

Changes in v15:
- Export drm_get_color_encoding_name only for kunit tests
- Link to v14: https://lore.kernel.org/r/20241122-yuv-v14-0-e66d83d28d0c@bootlin.com

Changes in v14:
- Rebased on drm-misc-next
- Link to v13: https://lore.kernel.org/r/20241118-yuv-v13-0-ac0dd4129552@bootlin.com

Changes since previous series:
 - Fix build test as modules issue: https://lore.kernel.org/all/202410110407.EHvadSaF-lkp@intel.com/
 - Export required symbols in DRM core to use them in kunit
 - Update the kunit comments according to Maxime's feedback
 - Link to original series: https://lore.kernel.org/r/20241007-yuv-v12-0-01c1ada6fec8@bootlin.com

---
Arthur Grillo (5):
      drm/vkms: Add YUV support
      drm/vkms: Add range and encoding properties to the plane
      drm/vkms: Drop YUV formats TODO
      drm/vkms: Create KUnit tests for YUV conversions
      drm/vkms: Add how to run the Kunit tests

Louis Chauvet (2):
      drm: Export symbols to use in tests
      drm/vkms: Add support for DRM_FORMAT_R*

 Documentation/gpu/vkms.rst                    |  14 +-
 drivers/gpu/drm/drm_color_mgmt.c              |   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 | 271 +++++++++++++++
 drivers/gpu/drm/vkms/vkms_drv.h               |  18 +
 drivers/gpu/drm/vkms/vkms_formats.c           | 467 +++++++++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_formats.h           |   9 +
 drivers/gpu/drm/vkms/vkms_plane.c             |  29 +-
 11 files changed, 830 insertions(+), 4 deletions(-)
---
base-commit: 49a167c393b0ceb592b9d2e65cc4f46bcc707108
change-id: 20240201-yuv-1337d90d9576

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

[PATCH v16 1/7] drm/vkms: Add YUV support

[Louis Chauvet]

From: Arthur Grillo <arthurgrillo@riseup.net>

Add support to the YUV formats bellow:

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

The conversion from yuv to rgb is done with fixed-point arithmetic, using
32.32 fixed-point numbers and the drm_fixed helpers.

To do the conversion, a specific matrix must be used for each color range
(DRM_COLOR_*_RANGE) and encoding (DRM_COLOR_*). This matrix is stored in
the `conversion_matrix` struct, along with the specific y_offset needed.
This matrix is queried only once, in `vkms_plane_atomic_update` and
stored in a `vkms_plane_state`. Those conversion matrices of each
encoding and range were obtained by rounding the values of the original
conversion matrices multiplied by 2^32. This is done to avoid the use of
floating point operations.

The same reading function is used for YUV and YVU formats. As the only
difference between those two category of formats is the order of field, a
simple swap in conversion matrix columns allows using the same function.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
[Louis Chauvet:
- Adapted Arthur's work
- Implemented the read_line_t callbacks for yuv
- add struct conversion_matrix
- store the whole conversion_matrix in the plane state
- remove struct pixel_yuv_u8
- update the commit message
- Merge the modifications from Arthur]
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_drv.h     |  18 ++
 drivers/gpu/drm/vkms/vkms_formats.c | 354 ++++++++++++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_formats.h |   4 +
 drivers/gpu/drm/vkms/vkms_plane.c   |  16 +-
 4 files changed, 391 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index abbb652be2b5389f96cec78837117ceb9acef656..fd752c34e6c95606e1f421af24549d78d33cfa16 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -100,17 +100,35 @@ typedef void (*pixel_read_line_t)(const struct vkms_plane_state *plane, int x_st
 				  int y_start, enum pixel_read_direction direction, int count,
 				  struct pixel_argb_u16 out_pixel[]);
 
+/**
+ * struct conversion_matrix - Matrix to use for a specific encoding and range
+ *
+ * @matrix: Conversion matrix from yuv to rgb. The matrix is stored in a row-major manner and is
+ * used to compute rgb values from yuv values:
+ *     [[r],[g],[b]] = @matrix * [[y],[u],[v]]
+ *   OR for yvu formats:
+ *     [[r],[g],[b]] = @matrix * [[y],[v],[u]]
+ *  The values of the matrix are signed fixed-point values with 32 bits fractional part.
+ * @y_offset: Offset to apply on the y value.
+ */
+struct conversion_matrix {
+	s64 matrix[3][3];
+	int y_offset;
+};
+
 /**
  * struct vkms_plane_state - Driver specific plane state
  * @base: base plane state
  * @frame_info: data required for composing computation
  * @pixel_read_line: function to read a pixel line in this plane. The creator of a
  *		     struct vkms_plane_state must ensure that this pointer is valid
+ * @conversion_matrix: matrix used for yuv formats to convert to rgb
  */
 struct vkms_plane_state {
 	struct drm_shadow_plane_state base;
 	struct vkms_frame_info *frame_info;
 	pixel_read_line_t pixel_read_line;
+	struct conversion_matrix conversion_matrix;
 };
 
 struct vkms_plane {
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 39b1d7c97d454dfa1420990d35e455ed47e57c0c..1f3ce4f334be9560e62c9a7fd933fa0ed6640e8f 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -140,6 +140,51 @@ static void packed_pixels_addr_1x1(const struct vkms_frame_info *frame_info,
 	*addr = (u8 *)frame_info->map[0].vaddr + offset;
 }
 
+/**
+ * get_subsampling() - Get the subsampling divisor value on a specific direction
+ *
+ * @format: format to extarct the subsampling from
+ * @direction: direction of the subsampling requested
+ */
+static int get_subsampling(const struct drm_format_info *format,
+			   enum pixel_read_direction direction)
+{
+	switch (direction) {
+	case READ_BOTTOM_TO_TOP:
+	case READ_TOP_TO_BOTTOM:
+		return format->vsub;
+	case READ_RIGHT_TO_LEFT:
+	case READ_LEFT_TO_RIGHT:
+		return format->hsub;
+	}
+	WARN_ONCE(true, "Invalid direction for pixel reading: %d\n", direction);
+	return 1;
+}
+
+/**
+ * get_subsampling_offset() - An offset for keeping the chroma siting consistent regardless of
+ * x_start and y_start values
+ *
+ * @direction: direction of the reading to properly compute this offset
+ * @x_start: x coordinate of the starting point of the readed line
+ * @y_start: y coordinate of the starting point of the readed line
+ */
+static int get_subsampling_offset(enum pixel_read_direction direction, int x_start, int y_start)
+{
+	switch (direction) {
+	case READ_BOTTOM_TO_TOP:
+		return -y_start - 1;
+	case READ_TOP_TO_BOTTOM:
+		return y_start;
+	case READ_RIGHT_TO_LEFT:
+		return -x_start - 1;
+	case READ_LEFT_TO_RIGHT:
+		return x_start;
+	}
+	WARN_ONCE(true, "Invalid direction for pixel reading: %d\n", direction);
+	return 0;
+}
+
 /*
  * The following functions take pixel data (a, r, g, b, pixel, ...) and convert them to
  * &struct pixel_argb_u16
@@ -202,6 +247,38 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
 	return out_pixel;
 }
 
+static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
+						  const struct conversion_matrix *matrix)
+{
+	u16 r, g, b;
+	s64 fp_y, fp_channel_1, fp_channel_2;
+	s64 fp_r, fp_g, fp_b;
+
+	fp_y = drm_int2fixp(((int)y - matrix->y_offset) * 257);
+	fp_channel_1 = drm_int2fixp(((int)channel_1 - 128) * 257);
+	fp_channel_2 = drm_int2fixp(((int)channel_2 - 128) * 257);
+
+	fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[0][1], fp_channel_1) +
+	       drm_fixp_mul(matrix->matrix[0][2], fp_channel_2);
+	fp_g = drm_fixp_mul(matrix->matrix[1][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[1][1], fp_channel_1) +
+	       drm_fixp_mul(matrix->matrix[1][2], fp_channel_2);
+	fp_b = drm_fixp_mul(matrix->matrix[2][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[2][1], fp_channel_1) +
+	       drm_fixp_mul(matrix->matrix[2][2], fp_channel_2);
+
+	fp_r = drm_fixp2int_round(fp_r);
+	fp_g = drm_fixp2int_round(fp_g);
+	fp_b = drm_fixp2int_round(fp_b);
+
+	r = clamp(fp_r, 0, 0xffff);
+	g = clamp(fp_g, 0, 0xffff);
+	b = clamp(fp_b, 0, 0xffff);
+
+	return argb_u16_from_u16161616(0xffff, r, g, b);
+}
+
 /*
  * The following functions are read_line function for each pixel format supported by VKMS.
  *
@@ -311,6 +388,92 @@ static void RGB565_read_line(const struct vkms_plane_state *plane, int x_start,
 	}
 }
 
+/*
+ * This callback can be used for YUV formats where U and V values are
+ * stored in the same plane (often called semi-planar formats). It will
+ * correctly handle subsampling as described in the drm_format_info of the plane.
+ *
+ * The conversion matrix stored in the @plane is used to:
+ * - Apply the correct color range and encoding
+ * - Convert YUV and YVU with the same function (a column swap is needed when setting up
+ * plane->conversion_matrix)
+ */
+static void semi_planar_yuv_read_line(const struct vkms_plane_state *plane, int x_start,
+				      int y_start, enum pixel_read_direction direction, int count,
+				      struct pixel_argb_u16 out_pixel[])
+{
+	u8 *y_plane;
+	u8 *uv_plane;
+
+	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,
+			       &y_plane);
+	packed_pixels_addr_1x1(plane->frame_info,
+			       x_start / plane->frame_info->fb->format->hsub,
+			       y_start / plane->frame_info->fb->format->vsub, 1,
+			       &uv_plane);
+	int step_y = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+	int step_uv = get_block_step_bytes(plane->frame_info->fb, direction, 1);
+	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);
+	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);
+	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
+
+	for (int i = 0; i < count; i++) {
+		*out_pixel = argb_u16_from_yuv888(y_plane[0], uv_plane[0], uv_plane[1],
+						  conversion_matrix);
+		out_pixel += 1;
+		y_plane += step_y;
+		if ((i + subsampling_offset + 1) % subsampling == 0)
+			uv_plane += step_uv;
+	}
+}
+
+/*
+ * This callback can be used for YUV format where each color component is
+ * stored in a different plane (often called planar formats). It will
+ * correctly handle subsampling as described in the drm_format_info of the plane.
+ *
+ * The conversion matrix stored in the @plane is used to:
+ * - Apply the correct color range and encoding
+ * - Convert YUV and YVU with the same function (a column swap is needed when setting up
+ * plane->conversion_matrix)
+ */
+static void planar_yuv_read_line(const struct vkms_plane_state *plane, int x_start,
+				 int y_start, enum pixel_read_direction direction, int count,
+				 struct pixel_argb_u16 out_pixel[])
+{
+	u8 *y_plane;
+	u8 *channel_1_plane;
+	u8 *channel_2_plane;
+
+	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0,
+			       &y_plane);
+	packed_pixels_addr_1x1(plane->frame_info,
+			       x_start / plane->frame_info->fb->format->hsub,
+			       y_start / plane->frame_info->fb->format->vsub, 1,
+			       &channel_1_plane);
+	packed_pixels_addr_1x1(plane->frame_info,
+			       x_start / plane->frame_info->fb->format->hsub,
+			       y_start / plane->frame_info->fb->format->vsub, 2,
+			       &channel_2_plane);
+	int step_y = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+	int step_channel_1 = get_block_step_bytes(plane->frame_info->fb, direction, 1);
+	int step_channel_2 = get_block_step_bytes(plane->frame_info->fb, direction, 2);
+	int subsampling = get_subsampling(plane->frame_info->fb->format, direction);
+	int subsampling_offset = get_subsampling_offset(direction, x_start, y_start);
+	const struct conversion_matrix *conversion_matrix = &plane->conversion_matrix;
+
+	for (int i = 0; i < count; i++) {
+		*out_pixel = argb_u16_from_yuv888(*y_plane, *channel_1_plane, *channel_2_plane,
+						  conversion_matrix);
+		out_pixel += 1;
+		y_plane += step_y;
+		if ((i + subsampling_offset + 1) % subsampling == 0) {
+			channel_1_plane += step_channel_1;
+			channel_2_plane += step_channel_2;
+		}
+	}
+}
+
 /*
  * The following functions take one &struct pixel_argb_u16 and convert it to a specific format.
  * The result is stored in @out_pixel.
@@ -426,6 +589,20 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 		return &XRGB16161616_read_line;
 	case DRM_FORMAT_RGB565:
 		return &RGB565_read_line;
+	case DRM_FORMAT_NV12:
+	case DRM_FORMAT_NV16:
+	case DRM_FORMAT_NV24:
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV61:
+	case DRM_FORMAT_NV42:
+		return &semi_planar_yuv_read_line;
+	case DRM_FORMAT_YUV420:
+	case DRM_FORMAT_YUV422:
+	case DRM_FORMAT_YUV444:
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YVU444:
+		return &planar_yuv_read_line;
 	default:
 		/*
 		 * This is a bug in vkms_plane_atomic_check(). All the supported
@@ -439,6 +616,183 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 	}
 }
 
+/*
+ * Those matrices were generated using the colour python framework
+ *
+ * Below are the function calls used to generate each matrix, go to
+ * https://colour.readthedocs.io/en/develop/generated/colour.matrix_YCbCr.html
+ * for more info:
+ *
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
+ *                                  is_legal = False,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix no_operation = {
+	.matrix = {
+		{ 4294967296, 0,          0, },
+		{ 0,          4294967296, 0, },
+		{ 0,          0,          4294967296, },
+	},
+	.y_offset = 0,
+};
+
+static const struct conversion_matrix yuv_bt601_full = {
+	.matrix = {
+		{ 4294967296, 0,           6021544149 },
+		{ 4294967296, -1478054095, -3067191994 },
+		{ 4294967296, 7610682049,  0 },
+	},
+	.y_offset = 0,
+};
+
+/*
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
+ *                                  is_legal = True,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix yuv_bt601_limited = {
+	.matrix = {
+		{ 5020601039, 0,           6881764740 },
+		{ 5020601039, -1689204679, -3505362278 },
+		{ 5020601039, 8697922339,  0 },
+	},
+	.y_offset = 16,
+};
+
+/*
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
+ *                                  is_legal = False,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix yuv_bt709_full = {
+	.matrix = {
+		{ 4294967296, 0,          6763714498 },
+		{ 4294967296, -804551626, -2010578443 },
+		{ 4294967296, 7969741314, 0 },
+	},
+	.y_offset = 0,
+};
+
+/*
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
+ *                                  is_legal = True,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix yuv_bt709_limited = {
+	.matrix = {
+		{ 5020601039, 0,          7729959424 },
+		{ 5020601039, -919487572, -2297803934 },
+		{ 5020601039, 9108275786, 0 },
+	},
+	.y_offset = 16,
+};
+
+/*
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
+ *                                  is_legal = False,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix yuv_bt2020_full = {
+	.matrix = {
+		{ 4294967296, 0,          6333358775 },
+		{ 4294967296, -706750298, -2453942994 },
+		{ 4294967296, 8080551471, 0 },
+	},
+	.y_offset = 0,
+};
+
+/*
+ * numpy.around(colour.matrix_YCbCr(K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
+ *                                  is_legal = True,
+ *                                  bits = 8) * 2**32).astype(int)
+ */
+static const struct conversion_matrix yuv_bt2020_limited = {
+	.matrix = {
+		{ 5020601039, 0,          7238124312 },
+		{ 5020601039, -807714626, -2804506279 },
+		{ 5020601039, 9234915964, 0 },
+	},
+	.y_offset = 16,
+};
+
+/**
+ * swap_uv_columns() - Swap u and v column of a given matrix
+ *
+ * @matrix: Matrix in which column are swapped
+ */
+static void swap_uv_columns(struct conversion_matrix *matrix)
+{
+	swap(matrix->matrix[0][2], matrix->matrix[0][1]);
+	swap(matrix->matrix[1][2], matrix->matrix[1][1]);
+	swap(matrix->matrix[2][2], matrix->matrix[2][1]);
+}
+
+/**
+ * get_conversion_matrix_to_argb_u16() - Retrieve the correct yuv to rgb conversion matrix for a
+ * given encoding and range.
+ *
+ * @format: DRM_FORMAT_* value for which to obtain a conversion function (see [drm_fourcc.h])
+ * @encoding: DRM_COLOR_* value for which to obtain a conversion matrix
+ * @range: DRM_COLOR_*_RANGE value for which to obtain a conversion matrix
+ * @matrix: Pointer to store the value into
+ */
+void get_conversion_matrix_to_argb_u16(u32 format,
+				       enum drm_color_encoding encoding,
+				       enum drm_color_range range,
+				       struct conversion_matrix *matrix)
+{
+	const struct conversion_matrix *matrix_to_copy;
+	bool limited_range;
+
+	switch (range) {
+	case DRM_COLOR_YCBCR_LIMITED_RANGE:
+		limited_range = true;
+		break;
+	case DRM_COLOR_YCBCR_FULL_RANGE:
+		limited_range = false;
+		break;
+	case DRM_COLOR_RANGE_MAX:
+		limited_range = false;
+		WARN_ONCE(true, "The requested range is not supported.");
+		break;
+	}
+
+	switch (encoding) {
+	case DRM_COLOR_YCBCR_BT601:
+		matrix_to_copy = limited_range ? &yuv_bt601_limited :
+						 &yuv_bt601_full;
+		break;
+	case DRM_COLOR_YCBCR_BT709:
+		matrix_to_copy = limited_range ? &yuv_bt709_limited :
+						 &yuv_bt709_full;
+		break;
+	case DRM_COLOR_YCBCR_BT2020:
+		matrix_to_copy = limited_range ? &yuv_bt2020_limited :
+						 &yuv_bt2020_full;
+		break;
+	case DRM_COLOR_ENCODING_MAX:
+		matrix_to_copy = &no_operation;
+		WARN_ONCE(true, "The requested encoding is not supported.");
+		break;
+	}
+
+	memcpy(matrix, matrix_to_copy, sizeof(*matrix_to_copy));
+
+	switch (format) {
+	case DRM_FORMAT_YVU420:
+	case DRM_FORMAT_YVU422:
+	case DRM_FORMAT_YVU444:
+	case DRM_FORMAT_NV21:
+	case DRM_FORMAT_NV61:
+	case DRM_FORMAT_NV42:
+		swap_uv_columns(matrix);
+		break;
+	default:
+		break;
+	}
+}
+EXPORT_SYMBOL(get_conversion_matrix_to_argb_u16);
+
 /**
  * get_pixel_write_function() - Retrieve the correct write_pixel function for a specific format.
  * The returned pointer is NULL for unsupported pixel formats. The caller must ensure that the
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index 8d2bef95ff7974a5c852dbaf3bf3f45c3ac32047..d583855cb32027d16b73d2a5b5a0644b13191d08 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -9,4 +9,8 @@ pixel_read_line_t get_pixel_read_line_function(u32 format);
 
 pixel_write_t get_pixel_write_function(u32 format);
 
+void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
+				       enum drm_color_range range,
+				       struct conversion_matrix *matrix);
+
 #endif /* _VKMS_FORMATS_H_ */
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index e2fce471870f1899f2ccb66b339ce8c4332cc287..10e032e146f093d0fd99a82c7b58d62a0d987fd7 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 *
@@ -118,6 +130,8 @@ static void vkms_plane_atomic_update(struct drm_plane *plane,
 	frame_info->rotation = new_state->rotation;
 
 	vkms_plane_state->pixel_read_line = get_pixel_read_line_function(fmt);
+	get_conversion_matrix_to_argb_u16(fmt, new_state->color_encoding, new_state->color_range,
+					  &vkms_plane_state->conversion_matrix);
 }
 
 static int vkms_plane_atomic_check(struct drm_plane *plane,

-- 
2.47.1

[PATCH v16 2/7] drm/vkms: Add range and encoding properties to the plane

[Louis Chauvet]

From: Arthur Grillo <arthurgrillo@riseup.net>

Now that the driver internally handles these quantization ranges and YUV
encoding matrices, expose the UAPI for setting them.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
[Louis Chauvet: retained only relevant parts, updated the commit message]
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_plane.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index 10e032e146f093d0fd99a82c7b58d62a0d987fd7..e6ca8f2db32d92068ddba7eed92cfae0d28cd45f 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -218,5 +218,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.47.1

[PATCH v16 3/7] drm/vkms: Drop YUV formats TODO

[Louis Chauvet]

From: Arthur Grillo <arthurgrillo@riseup.net>

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

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 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 ba04ac7c2167a9d484c54c69a09a2fb8f2d9c0aa..13b866c3617cd44043406252d3caa912c931772f 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.47.1

[PATCH v16 4/7] drm: Export symbols to use in tests

[Louis Chauvet]

The functions drm_get_color_encoding_name and drm_get_color_range_name
are useful for clarifying test results. Therefore, export them so they
can be used in tests built as modules.

Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/drm_color_mgmt.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
index 3969dc548cff605cbdd3d56dceafb2ca00a5c886..b73a998352d175a26c69e0878da28a6288cfc8b7 100644
--- a/drivers/gpu/drm/drm_color_mgmt.c
+++ b/drivers/gpu/drm/drm_color_mgmt.c
@@ -28,6 +28,7 @@
 #include <drm/drm_device.h>
 #include <drm/drm_drv.h>
 #include <drm/drm_print.h>
+#include <kunit/visibility.h>
 
 #include "drm_crtc_internal.h"
 
@@ -494,6 +495,7 @@ const char *drm_get_color_encoding_name(enum drm_color_encoding encoding)
 
 	return color_encoding_name[encoding];
 }
+EXPORT_SYMBOL_IF_KUNIT(drm_get_color_encoding_name);
 
 /**
  * drm_get_color_range_name - return a string for color range
@@ -509,6 +511,7 @@ const char *drm_get_color_range_name(enum drm_color_range range)
 
 	return color_range_name[range];
 }
+EXPORT_SYMBOL_IF_KUNIT(drm_get_color_range_name);
 
 /**
  * drm_plane_create_color_properties - color encoding related plane properties

-- 
2.47.1

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

[Louis Chauvet]

From: Arthur Grillo <arthurgrillo@riseup.net>

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

The code used to compute the expected result can be found in comment.

[Louis Chauvet:
- fix minor formating issues (whitespace, double line)
- change expected alpha from 0x0000 to 0xffff
- adapt to the new get_conversion_matrix usage
- apply the changes from Arthur
- move struct pixel_yuv_u8 to the test itself]

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 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 | 271 ++++++++++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_formats.c           |   7 +-
 drivers/gpu/drm/vkms/vkms_formats.h           |   5 +
 7 files changed, 304 insertions(+), 2 deletions(-)

diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index 9def079f685bd30e1df3e4082e4818e402395391..d4665e913de7702fbd5c0f047876dad9715c690a 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -14,3 +14,18 @@ config DRM_VKMS
 	  a VKMS.
 
 	  If M is selected the module will be called vkms.
+
+config DRM_VKMS_KUNIT_TESTS
+	tristate "KUnit 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 1b28a6a32948b557867dda51d2ccfdea687a2b62..8d3e46dde6350558a0aab4254df0dfe863f9c6ce 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 0000000000000000000000000000000000000000..70e378228cbdaa025f01641f207a93a6c01f0853
--- /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 0000000000000000000000000000000000000000..2d1df668569e4f243ed9a06c1e16e595c131c4f6
--- /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 0000000000000000000000000000000000000000..aa81347ddce2288933abc2ff5b79f0ee11ff4271
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -0,0 +1,271 @@
+// SPDX-License-Identifier: GPL-2.0+
+
+#include <kunit/test.h>
+
+#include <drm/drm_fixed.h>
+#include <drm/drm_fourcc.h>
+
+#include "../../drm_crtc_internal.h"
+
+#include "../vkms_formats.h"
+
+#define TEST_BUFF_SIZE 50
+
+MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
+
+struct pixel_yuv_u8 {
+	u8 y, u, v;
+};
+
+/*
+ * struct yuv_u8_to_argb_u16_case - Reference values to test the color
+ * conversions in VKMS between YUV to ARGB
+ *
+ * @encoding: Encoding used to convert RGB to YUV
+ * @range: Range used to convert RGB to YUV
+ * @n_colors: Count of test colors in this case
+ * @format_pair.name: Name used for this color conversion, used to
+ *                    clarify the test results
+ * @format_pair.rgb: RGB color tested
+ * @format_pair.yuv: Same color as @format_pair.rgb, but converted to
+ *                   YUV using @encoding and @range.
+ */
+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];
+};
+
+/*
+ * The YUV color representation were acquired via the colour python framework.
+ * Below are the function calls used for generating each case.
+ *
+ * For more information got to the docs:
+ * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
+ */
+static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
+	 *                     in_bits = 16,
+	 *                     in_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = False,
+	 *                     out_int = True)
+	 *
+	 * Test cases for conversion between YUV BT601 full range and RGB
+	 * using the ITU-R BT.601 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT601,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 6,
+		.colors = {
+			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x4c, 0x55, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0x96, 0x2c, 0x15 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x1d, 0xff, 0x6b }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
+	 *                     in_bits = 16,
+	 *                     in_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = True,
+	 *                     out_int = True)
+	 * Test cases for conversion between YUV BT601 limited range and RGB
+	 * using the ITU-R BT.601 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT601,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 6,
+		.colors = {
+			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x51, 0x5a, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0x91, 0x36, 0x22 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x29, 0xf0, 0x6e }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
+	 *                     in_bits = 16,
+	 *                     in_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = False,
+	 *                     out_int = True)
+	 * Test cases for conversion between YUV BT709 full range and RGB
+	 * using the ITU-R BT.709 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT709,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x36, 0x63, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0xb6, 0x1e, 0x0c }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x12, 0xff, 0x74 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
+	 *                     in_bits = 16,
+	 *                     int_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = True,
+	 *                     out_int = True)
+	 * Test cases for conversion between YUV BT709 limited range and RGB
+	 * using the ITU-R BT.709 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT709,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x3f, 0x66, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0xad, 0x2a, 0x1a }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x20, 0xf0, 0x76 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
+	 *                     in_bits = 16,
+	 *                     in_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = False,
+	 *                     out_int = True)
+	 * Test cases for conversion between YUV BT2020 full range and RGB
+	 * using the ITU-R BT.2020 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT2020,
+		.range = DRM_COLOR_YCBCR_FULL_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x43, 0x5c, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0xad, 0x24, 0x0b }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x0f, 0xff, 0x76 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+	/*
+	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
+	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
+	 *                     in_bits = 16,
+	 *                     in_legal = False,
+	 *                     in_int = True,
+	 *                     out_bits = 8,
+	 *                     out_legal = True,
+	 *                     out_int = True)
+	 * Test cases for conversion between YUV BT2020 limited range and RGB
+	 * using the ITU-R BT.709 weights.
+	 */
+	{
+		.encoding = DRM_COLOR_YCBCR_BT2020,
+		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
+		.n_colors = 4,
+		.colors = {
+			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
+			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
+			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
+			{ "red",   { 0x4a, 0x61, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
+			{ "green", { 0xa4, 0x2f, 0x19 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
+			{ "blue",  { 0x1d, 0xf0, 0x77 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
+		},
+	},
+};
+
+/*
+ * vkms_format_test_yuv_u8_to_argb_u16 - Testing the conversion between YUV
+ * colors to ARGB colors in VKMS
+ *
+ * This test will use the functions get_conversion_matrix_to_argb_u16 and
+ * argb_u16_from_yuv888 to convert YUV colors (stored in
+ * yuv_u8_to_argb_u16_cases) into ARGB colors.
+ *
+ * The conversion between YUV and RGB is not totally reversible, so there may be
+ * some difference between the expected value and the result.
+ * In addition, there may be some rounding error as the input color is 8 bits
+ * and output color is 16 bits.
+ */
+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];
+		struct conversion_matrix matrix;
+
+		get_conversion_matrix_to_argb_u16
+			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
+
+		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);
+
+		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.a, color->argb.a), 0x1ff,
+				    "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), 0x1ff,
+				    "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), 0x1ff,
+				    "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), 0x1ff,
+				    "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");
+MODULE_DESCRIPTION("Kunit test for vkms format conversion");
diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 1f3ce4f334be9560e62c9a7fd933fa0ed6640e8f..0b867444999105262c855a24bf03bc66d9ebea1b 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"
 
 /**
@@ -247,8 +249,8 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
 	return out_pixel;
 }
 
-static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
-						  const struct conversion_matrix *matrix)
+VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
+							    const struct conversion_matrix *matrix)
 {
 	u16 r, g, b;
 	s64 fp_y, fp_channel_1, fp_channel_2;
@@ -278,6 +280,7 @@ static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel
 
 	return argb_u16_from_u16161616(0xffff, r, g, b);
 }
+EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
 
 /*
  * The following functions are read_line function for each pixel format supported by VKMS.
diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
index d583855cb32027d16b73d2a5b5a0644b13191d08..b4fe62ab9c65d465925d29911f26612193a80799 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.h
+++ b/drivers/gpu/drm/vkms/vkms_formats.h
@@ -13,4 +13,9 @@ void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encod
 				       enum drm_color_range range,
 				       struct conversion_matrix *matrix);
 
+#if IS_ENABLED(CONFIG_KUNIT)
+struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
+					   const struct conversion_matrix *matrix);
+#endif
+
 #endif /* _VKMS_FORMATS_H_ */

-- 
2.47.1

[PATCH v16 6/7] drm/vkms: Add how to run the Kunit tests

[Louis Chauvet]

From: Arthur Grillo <arthurgrillo@riseup.net>

Now that we have KUnit tests, add instructions on how to run them.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 Documentation/gpu/vkms.rst | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 13b866c3617cd44043406252d3caa912c931772f..5ef5ef2e6a210382a070eaf3552bbce75b04ff0c 100644
--- a/Documentation/gpu/vkms.rst
+++ b/Documentation/gpu/vkms.rst
@@ -89,6 +89,17 @@ You can also run subtests if you do not want to run the entire test::
   sudo ./build/tests/kms_flip --run-subtest basic-plain-flip --device "sys:/sys/devices/platform/vkms"
   sudo IGT_DEVICE="sys:/sys/devices/platform/vkms" ./build/tests/kms_flip --run-subtest basic-plain-flip
 
+Testing With KUnit
+==================
+
+KUnit (Kernel unit testing framework) provides a common framework for unit tests
+within the Linux kernel.
+More information in ../dev-tools/kunit/index.rst .
+
+To run the VKMS KUnit tests::
+
+  tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/vkms/tests
+
 TODO
 ====
 

-- 
2.47.1

[PATCH v16 7/7] drm/vkms: Add support for DRM_FORMAT_R*

[Louis Chauvet]

This add the support for:
- R1/R2/R4/R8

R1 format was tested with [1] and [2].

[1]: https://lore.kernel.org/r/20240313-new_rotation-v2-0-6230fd5cae59@bootlin.com
[2]: https://lore.kernel.org/igt-dev/20240306-b4-kms_tests-v1-0-8fe451efd2ac@bootlin.com/

Reviewed-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
---
 drivers/gpu/drm/vkms/vkms_formats.c | 110 +++++++++++++++++++++++++++++++++++-
 drivers/gpu/drm/vkms/vkms_plane.c   |   4 ++
 2 files changed, 113 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 0b867444999105262c855a24bf03bc66d9ebea1b..2edf1ceccd37ad3a2f9f6d2dcf2044c98d9e10f0 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -249,6 +249,16 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
 	return out_pixel;
 }
 
+static struct pixel_argb_u16 argb_u16_from_gray8(u8 gray)
+{
+	return argb_u16_from_u8888(255, gray, gray, gray);
+}
+
+static struct pixel_argb_u16 argb_u16_from_grayu16(u16 gray)
+{
+	return argb_u16_from_u16161616(0xFFFF, gray, gray, gray);
+}
+
 VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
 							    const struct conversion_matrix *matrix)
 {
@@ -286,7 +296,7 @@ EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
  * The following functions are read_line function for each pixel format supported by VKMS.
  *
  * They read a line starting at the point @x_start,@y_start following the @direction. The result
- * is stored in @out_pixel and in the format ARGB16161616.
+ * is stored in @out_pixel and in a 64 bits format, see struct pixel_argb_u16.
  *
  * These functions are very repetitive, but the innermost pixel loops must be kept inside these
  * functions for performance reasons. Some benchmarking was done in [1] where having the innermost
@@ -295,6 +305,96 @@ EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
  * [1]: https://lore.kernel.org/dri-devel/d258c8dc-78e9-4509-9037-a98f7f33b3a3@riseup.net/
  */
 
+static void Rx_read_line(const struct vkms_plane_state *plane, int x_start,
+			 int y_start, enum pixel_read_direction direction, int count,
+			 struct pixel_argb_u16 out_pixel[])
+{
+	struct pixel_argb_u16 *end = out_pixel + count;
+	int bits_per_pixel = drm_format_info_bpp(plane->frame_info->fb->format, 0);
+	u8 *src_pixels;
+	int rem_x, rem_y;
+
+	WARN_ONCE(drm_format_info_block_height(plane->frame_info->fb->format, 0) != 1,
+		  "%s() only support formats with block_h == 1", __func__);
+
+	packed_pixels_addr(plane->frame_info, x_start, y_start, 0, &src_pixels, &rem_x, &rem_y);
+	int bit_offset = (8 - bits_per_pixel) - rem_x * bits_per_pixel;
+	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+	int mask = (0x1 << bits_per_pixel) - 1;
+	int lum_per_level = 0xFFFF / mask;
+
+	if (direction == READ_LEFT_TO_RIGHT || direction == READ_RIGHT_TO_LEFT) {
+		int restart_bit_offset;
+		int step_bit_offset;
+
+		if (direction == READ_LEFT_TO_RIGHT) {
+			restart_bit_offset = 8 - bits_per_pixel;
+			step_bit_offset = -bits_per_pixel;
+		} else {
+			restart_bit_offset = 0;
+			step_bit_offset = bits_per_pixel;
+		}
+
+		while (out_pixel < end) {
+			u8 val = ((*src_pixels) >> bit_offset) & mask;
+
+			*out_pixel = argb_u16_from_grayu16((int)val * lum_per_level);
+
+			bit_offset += step_bit_offset;
+			if (bit_offset < 0 || 8 <= bit_offset) {
+				bit_offset = restart_bit_offset;
+				src_pixels += step;
+			}
+			out_pixel += 1;
+		}
+	} else if (direction == READ_TOP_TO_BOTTOM || direction == READ_BOTTOM_TO_TOP) {
+		while (out_pixel < end) {
+			u8 val = (*src_pixels >> bit_offset) & mask;
+			*out_pixel = argb_u16_from_grayu16((int)val * lum_per_level);
+			src_pixels += step;
+			out_pixel += 1;
+		}
+	}
+}
+
+static void R1_read_line(const struct vkms_plane_state *plane, int x_start,
+			 int y_start, enum pixel_read_direction direction, int count,
+			 struct pixel_argb_u16 out_pixel[])
+{
+	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
+}
+
+static void R2_read_line(const struct vkms_plane_state *plane, int x_start,
+			 int y_start, enum pixel_read_direction direction, int count,
+			 struct pixel_argb_u16 out_pixel[])
+{
+	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
+}
+
+static void R4_read_line(const struct vkms_plane_state *plane, int x_start,
+			 int y_start, enum pixel_read_direction direction, int count,
+			 struct pixel_argb_u16 out_pixel[])
+{
+	Rx_read_line(plane, x_start, y_start, direction, count, out_pixel);
+}
+
+static void R8_read_line(const struct vkms_plane_state *plane, int x_start,
+			 int y_start, enum pixel_read_direction direction, int count,
+			 struct pixel_argb_u16 out_pixel[])
+{
+	struct pixel_argb_u16 *end = out_pixel + count;
+	u8 *src_pixels;
+	int step = get_block_step_bytes(plane->frame_info->fb, direction, 0);
+
+	packed_pixels_addr_1x1(plane->frame_info, x_start, y_start, 0, &src_pixels);
+
+	while (out_pixel < end) {
+		*out_pixel = argb_u16_from_gray8(*src_pixels);
+		src_pixels += step;
+		out_pixel += 1;
+	}
+}
+
 static void ARGB8888_read_line(const struct vkms_plane_state *plane, int x_start, int y_start,
 			       enum pixel_read_direction direction, int count,
 			       struct pixel_argb_u16 out_pixel[])
@@ -606,6 +706,14 @@ pixel_read_line_t get_pixel_read_line_function(u32 format)
 	case DRM_FORMAT_YVU422:
 	case DRM_FORMAT_YVU444:
 		return &planar_yuv_read_line;
+	case DRM_FORMAT_R1:
+		return &R1_read_line;
+	case DRM_FORMAT_R2:
+		return &R2_read_line;
+	case DRM_FORMAT_R4:
+		return &R4_read_line;
+	case DRM_FORMAT_R8:
+		return &R8_read_line;
 	default:
 		/*
 		 * This is a bug in vkms_plane_atomic_check(). All the supported
diff --git a/drivers/gpu/drm/vkms/vkms_plane.c b/drivers/gpu/drm/vkms/vkms_plane.c
index e6ca8f2db32d92068ddba7eed92cfae0d28cd45f..81941914af87fcefb180dc393f2ec311f1a1d3fa 100644
--- a/drivers/gpu/drm/vkms/vkms_plane.c
+++ b/drivers/gpu/drm/vkms/vkms_plane.c
@@ -30,6 +30,10 @@ static const u32 vkms_formats[] = {
 	DRM_FORMAT_YVU420,
 	DRM_FORMAT_YVU422,
 	DRM_FORMAT_YVU444,
+	DRM_FORMAT_R1,
+	DRM_FORMAT_R2,
+	DRM_FORMAT_R4,
+	DRM_FORMAT_R8,
 };
 
 static struct drm_plane_state *

-- 
2.47.1

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 726 bytes --]

On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = False,
> +	 *                     out_int = True)
> +	 *
> +	 * Test cases for conversion between YUV BT601 full range and RGB
> +	 * using the ITU-R BT.601 weights.
> +	 */

What are the input and output formats?

Ditto for all the other tests.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Louis Chauvet]

On 26/01/25 - 18:06, Maxime Ripard wrote:
> On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > +	/*
> > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > +	 *                     in_bits = 16,
> > +	 *                     in_legal = False,
> > +	 *                     in_int = True,
> > +	 *                     out_bits = 8,
> > +	 *                     out_legal = False,
> > +	 *                     out_int = True)
> > +	 *
> > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > +	 * using the ITU-R BT.601 weights.
> > +	 */
> 
> What are the input and output formats?
> 
> Ditto for all the other tests.

There is no really "input" and "output" format, they are reference values 
for conversion, you should be able to use it in both direction. They are 
generated by RGB_to_YCbCr (RGB input, YUV output) just because it was 
easier to create the colors from RGB values.

If you think we should specify what is was used as input and output to 
generate those values, I can modify the comment to:

	Tests cases for color conversion generated by converting RGB 
	values to YUV BT601 full range using the ITU-R BT.601 weights.

Beside that modification, did you notice anything else on the series that 
require more work before adding your Ack-by/Reviewed-by on the other 
patches?

Thanks,
Louis Chauvet
 
> Maxime

[PATCH v16 3/7] drm/vkms: Drop YUV formats TODO

[José Expósito]

Hi Louis,

Thanks a lot for the patches.

I'm not well versed in YUV color formats, so I did my best reading the kernel
documentation before reviewing this series... But I'll most likely ask some
basic/dump questions.

> From: Arthur Grillo <arthurgrillo@riseup.net>
> 
> VKMS has support for YUV formats now. Remove the task from the TODO
> list.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  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 ba04ac7c2167a9d484c54c69a09a2fb8f2d9c0aa..13b866c3617cd44043406252d3caa912c931772f 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.

I see that you implemented support for 6 DRM_FORMAT_NV* formats, but
DRM_FORMAT_NV15, DRM_FORMAT_NV20 and DRM_FORMAT_NV30 are not implemented.

The same applies to DRM_FORMAT_Y210 or DRM_FORMAT_YUV410 among others.

Could it be useful to implement all of them in the future? If so, should we add
it to the ToDo list?
It might be a great task to get started in kernel development, as there are
already similar examples and tests.

>  
>  - Async updates (currently only possible on cursor plane using the legacy
>    cursor api).
> 

[PATCH v16 4/7] drm: Export symbols to use in tests

[José Expósito]

> The functions drm_get_color_encoding_name and drm_get_color_range_name
> are useful for clarifying test results. Therefore, export them so they
> can be used in tests built as modules.
> 
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

Nice and simple, in the next version:

Reviewed-by: José Expósito <jose.exposito89@gmail.com>

> ---
>  drivers/gpu/drm/drm_color_mgmt.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/drivers/gpu/drm/drm_color_mgmt.c b/drivers/gpu/drm/drm_color_mgmt.c
> index 3969dc548cff605cbdd3d56dceafb2ca00a5c886..b73a998352d175a26c69e0878da28a6288cfc8b7 100644
> --- a/drivers/gpu/drm/drm_color_mgmt.c
> +++ b/drivers/gpu/drm/drm_color_mgmt.c
> @@ -28,6 +28,7 @@
>  #include <drm/drm_device.h>
>  #include <drm/drm_drv.h>
>  #include <drm/drm_print.h>
> +#include <kunit/visibility.h>
>  
>  #include "drm_crtc_internal.h"
>  
> @@ -494,6 +495,7 @@ const char *drm_get_color_encoding_name(enum drm_color_encoding encoding)
>  
>  	return color_encoding_name[encoding];
>  }
> +EXPORT_SYMBOL_IF_KUNIT(drm_get_color_encoding_name);
>  
>  /**
>   * drm_get_color_range_name - return a string for color range
> @@ -509,6 +511,7 @@ const char *drm_get_color_range_name(enum drm_color_range range)
>  
>  	return color_range_name[range];
>  }
> +EXPORT_SYMBOL_IF_KUNIT(drm_get_color_range_name);
>  
>  /**
>   * drm_plane_create_color_properties - color encoding related plane properties
> 

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

[José Expósito]

Hi Louis,

> From: Arthur Grillo <arthurgrillo@riseup.net>
> 
> Create KUnit tests to test the conversion between YUV and RGB. Test each
> conversion and range combination with some common colors.
> 
> The code used to compute the expected result can be found in comment.
>
> [Louis Chauvet:
> - fix minor formating issues (whitespace, double line)
> - change expected alpha from 0x0000 to 0xffff
> - adapt to the new get_conversion_matrix usage
> - apply the changes from Arthur
> - move struct pixel_yuv_u8 to the test itself]
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> ---
>  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 | 271 ++++++++++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_formats.c           |   7 +-
>  drivers/gpu/drm/vkms/vkms_formats.h           |   5 +
>  7 files changed, 304 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index 9def079f685bd30e1df3e4082e4818e402395391..d4665e913de7702fbd5c0f047876dad9715c690a 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -14,3 +14,18 @@ config DRM_VKMS
>  	  a VKMS.
>  
>  	  If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> +	tristate "KUnit 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 1b28a6a32948b557867dda51d2ccfdea687a2b62..8d3e46dde6350558a0aab4254df0dfe863f9c6ce 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 0000000000000000000000000000000000000000..70e378228cbdaa025f01641f207a93a6c01f0853
> --- /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 0000000000000000000000000000000000000000..2d1df668569e4f243ed9a06c1e16e595c131c4f6
> --- /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 0000000000000000000000000000000000000000..aa81347ddce2288933abc2ff5b79f0ee11ff4271
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> @@ -0,0 +1,271 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_fixed.h>
> +#include <drm/drm_fourcc.h>
> +
> +#include "../../drm_crtc_internal.h"
> +
> +#include "../vkms_formats.h"
> +
> +#define TEST_BUFF_SIZE 50
> +
> +MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
> +
> +struct pixel_yuv_u8 {
> +	u8 y, u, v;
> +};
> +
> +/*
> + * struct yuv_u8_to_argb_u16_case - Reference values to test the color
> + * conversions in VKMS between YUV to ARGB
> + *
> + * @encoding: Encoding used to convert RGB to YUV
> + * @range: Range used to convert RGB to YUV
> + * @n_colors: Count of test colors in this case
> + * @format_pair.name: Name used for this color conversion, used to
> + *                    clarify the test results
> + * @format_pair.rgb: RGB color tested
> + * @format_pair.yuv: Same color as @format_pair.rgb, but converted to
> + *                   YUV using @encoding and @range.
> + */
> +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];
> +};
> +
> +/*
> + * The YUV color representation were acquired via the colour python framework.
> + * Below are the function calls used for generating each case.
> + *
> + * For more information got to the docs:
> + * https://colour.readthedocs.io/en/master/generated/colour.RGB_to_YCbCr.html
> + */
> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = False,
> +	 *                     out_int = True)
> +	 *
> +	 * Test cases for conversion between YUV BT601 full range and RGB
> +	 * using the ITU-R BT.601 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT601,
> +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> +		.n_colors = 6,
> +		.colors = {
> +			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x4c, 0x55, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0x96, 0x2c, 0x15 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x1d, 0xff, 0x6b }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = True,
> +	 *                     out_int = True)
> +	 * Test cases for conversion between YUV BT601 limited range and RGB
> +	 * using the ITU-R BT.601 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT601,
> +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +		.n_colors = 6,
> +		.colors = {
> +			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x51, 0x5a, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0x91, 0x36, 0x22 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x29, 0xf0, 0x6e }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = False,
> +	 *                     out_int = True)
> +	 * Test cases for conversion between YUV BT709 full range and RGB
> +	 * using the ITU-R BT.709 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT709,
> +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> +		.n_colors = 4,

If I understood correctly, "n_colors" here indicates the number of items in
"colors", but there is a mismatch between both lengths.

It also applies to the other test cases where "n_colors = 4".

> +		.colors = {
> +			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x36, 0x63, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0xb6, 0x1e, 0x0c }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x12, 0xff, 0x74 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
> +	 *                     in_bits = 16,
> +	 *                     int_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = True,
> +	 *                     out_int = True)
> +	 * Test cases for conversion between YUV BT709 limited range and RGB
> +	 * using the ITU-R BT.709 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT709,
> +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +		.n_colors = 4,
> +		.colors = {
> +			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x3f, 0x66, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0xad, 0x2a, 0x1a }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x20, 0xf0, 0x76 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = False,
> +	 *                     out_int = True)
> +	 * Test cases for conversion between YUV BT2020 full range and RGB
> +	 * using the ITU-R BT.2020 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT2020,
> +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> +		.n_colors = 4,
> +		.colors = {
> +			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x43, 0x5c, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0xad, 0x24, 0x0b }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x0f, 0xff, 0x76 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +	/*
> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.2020"],
> +	 *                     in_bits = 16,
> +	 *                     in_legal = False,
> +	 *                     in_int = True,
> +	 *                     out_bits = 8,
> +	 *                     out_legal = True,
> +	 *                     out_int = True)
> +	 * Test cases for conversion between YUV BT2020 limited range and RGB
> +	 * using the ITU-R BT.709 weights.
> +	 */
> +	{
> +		.encoding = DRM_COLOR_YCBCR_BT2020,
> +		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
> +		.n_colors = 4,
> +		.colors = {
> +			{ "white", { 0xeb, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> +			{ "gray",  { 0x7e, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> +			{ "black", { 0x10, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> +			{ "red",   { 0x4a, 0x61, 0xf0 }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> +			{ "green", { 0xa4, 0x2f, 0x19 }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> +			{ "blue",  { 0x1d, 0xf0, 0x77 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> +		},
> +	},
> +};
> +
> +/*
> + * vkms_format_test_yuv_u8_to_argb_u16 - Testing the conversion between YUV
> + * colors to ARGB colors in VKMS
> + *
> + * This test will use the functions get_conversion_matrix_to_argb_u16 and
> + * argb_u16_from_yuv888 to convert YUV colors (stored in
> + * yuv_u8_to_argb_u16_cases) into ARGB colors.
> + *
> + * The conversion between YUV and RGB is not totally reversible, so there may be
> + * some difference between the expected value and the result.
> + * In addition, there may be some rounding error as the input color is 8 bits
> + * and output color is 16 bits.
> + */
> +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];
> +		struct conversion_matrix matrix;
> +
> +		get_conversion_matrix_to_argb_u16
> +			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
> +
> +		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);

Running the test on ppc64 (big endian) doesn't fail. For reference:

  $ sudo dnf install powerpc64-linux-gnu-gcc
  $ sudo dnf install qemu-system-ppc64
  $ ./tools/testing/kunit/kunit.py run \
     --kunitconfig=drivers/gpu/drm/vkms/tests \
     --arch=powerpc --cross_compile=powerpc64-linux-gnu- \
     --make_options CF=-D__CHECK_ENDIAN__ \
     --kconfig_add CONFIG_KASAN=y \
     --kconfig_add CONFIG_KASAN_VMALLOC=y

However, I wonder if endianness is correctly handled. I always find endianness
difficult to reason about, but I'll try my best to explain it.

On a big endian architecture, color->yuv is stored in big endian. This might not
be an issue, because its components (y, u and v) are u8.
However, I think that the return value of argb_u16_from_yuv888(), which is the
result of argb_u16_from_u16161616(), is returned in big endian while it should
be little endian.

Since you are comparing argb.a (big endian) with color->argb.a (big endian) the
test succedess, but in this case it should fail because, if I remember
correctly, colors must be stored in little endian and therefore, the color
returned by argb_u16_from_yuv888() should be little endian.

If you replace this 4 KUNIT_EXPECT_LE_MSG() with KUNIT_EXPECT_MEMEQ(), all test
will fail, but you'll notice that the buffers printed in the error log are
different depending on the endianness (x86_64 vs ppc64).

What do you think? Did I overlook the conversion?

Have a look to the tests present in drm_format_helper_test.c. They use different
functions (cpubuf_to_le32, le32buf_to_cpu, etc) to make sure that colors are
represented in little endian and that comparing the expected and actual results
happens in the same endian.

> +
> +		KUNIT_EXPECT_LE_MSG(test, abs_diff(argb.a, color->argb.a), 0x1ff,
> +				    "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), 0x1ff,
> +				    "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), 0x1ff,
> +				    "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), 0x1ff,
> +				    "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");
> +MODULE_DESCRIPTION("Kunit test for vkms format conversion");
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 1f3ce4f334be9560e62c9a7fd933fa0ed6640e8f..0b867444999105262c855a24bf03bc66d9ebea1b 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"
>  
>  /**
> @@ -247,8 +249,8 @@ static struct pixel_argb_u16 argb_u16_from_RGB565(const __le16 *pixel)
>  	return out_pixel;
>  }
>  
> -static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> -						  const struct conversion_matrix *matrix)
> +VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> +							    const struct conversion_matrix *matrix)
>  {
>  	u16 r, g, b;
>  	s64 fp_y, fp_channel_1, fp_channel_2;
> @@ -278,6 +280,7 @@ static struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel
>  
>  	return argb_u16_from_u16161616(0xffff, r, g, b);
>  }
> +EXPORT_SYMBOL_IF_KUNIT(argb_u16_from_yuv888);
>  
>  /*
>   * The following functions are read_line function for each pixel format supported by VKMS.
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.h b/drivers/gpu/drm/vkms/vkms_formats.h
> index d583855cb32027d16b73d2a5b5a0644b13191d08..b4fe62ab9c65d465925d29911f26612193a80799 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.h
> +++ b/drivers/gpu/drm/vkms/vkms_formats.h
> @@ -13,4 +13,9 @@ void get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encod
>  				       enum drm_color_range range,
>  				       struct conversion_matrix *matrix);
>  
> +#if IS_ENABLED(CONFIG_KUNIT)
> +struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 channel_1, u8 channel_2,
> +					   const struct conversion_matrix *matrix);
> +#endif
> +
>  #endif /* _VKMS_FORMATS_H_ */
> 

[PATCH v16 6/7] drm/vkms: Add how to run the Kunit tests

[José Expósito]

> From: Arthur Grillo <arthurgrillo@riseup.net>
> 
> Now that we have KUnit tests, add instructions on how to run them.
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>

Oh nice, I missed this in [1].

Reviewed-by: José Expósito <jose.exposito89@gmail.com>

[1] https://patchwork.kernel.org/project/dri-devel/patch/20250129110059.12199-3-jose.exposito89@gmail.com/

> ---
>  Documentation/gpu/vkms.rst | 11 +++++++++++
>  1 file changed, 11 insertions(+)
> 
> diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
> index 13b866c3617cd44043406252d3caa912c931772f..5ef5ef2e6a210382a070eaf3552bbce75b04ff0c 100644
> --- a/Documentation/gpu/vkms.rst
> +++ b/Documentation/gpu/vkms.rst
> @@ -89,6 +89,17 @@ You can also run subtests if you do not want to run the entire test::
>    sudo ./build/tests/kms_flip --run-subtest basic-plain-flip --device "sys:/sys/devices/platform/vkms"
>    sudo IGT_DEVICE="sys:/sys/devices/platform/vkms" ./build/tests/kms_flip --run-subtest basic-plain-flip
>  
> +Testing With KUnit
> +==================
> +
> +KUnit (Kernel unit testing framework) provides a common framework for unit tests
> +within the Linux kernel.
> +More information in ../dev-tools/kunit/index.rst .
> +
> +To run the VKMS KUnit tests::
> +
> +  tools/testing/kunit/kunit.py run --kunitconfig=drivers/gpu/drm/vkms/tests
> +
>  TODO
>  ====
>  
> 

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

[Louis Chauvet]

On 31/01/25 - 09:41, José Expósito wrote:
> Hi Louis,
> 
> > From: Arthur Grillo <arthurgrillo@riseup.net>
> > 
> > Create KUnit tests to test the conversion between YUV and RGB. Test each
> > conversion and range combination with some common colors.
> > 
> > The code used to compute the expected result can be found in comment.
> >
> > [Louis Chauvet:
> > - fix minor formating issues (whitespace, double line)
> > - change expected alpha from 0x0000 to 0xffff
> > - adapt to the new get_conversion_matrix usage
> > - apply the changes from Arthur
> > - move struct pixel_yuv_u8 to the test itself]
> > 
> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---

[...]

> > +	/*
> > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
> > +	 *                     in_bits = 16,
> > +	 *                     in_legal = False,
> > +	 *                     in_int = True,
> > +	 *                     out_bits = 8,
> > +	 *                     out_legal = False,
> > +	 *                     out_int = True)
> > +	 * Test cases for conversion between YUV BT709 full range and RGB
> > +	 * using the ITU-R BT.709 weights.
> > +	 */
> > +	{
> > +		.encoding = DRM_COLOR_YCBCR_BT709,
> > +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> > +		.n_colors = 4,
> 
> If I understood correctly, "n_colors" here indicates the number of items in
> "colors", but there is a mismatch between both lengths.
> 
> It also applies to the other test cases where "n_colors = 4".

I don't know how I miss it, I am 100% sure I did the exact same comment to 
Arthur few mounth ago, thanks!
 
> > +		.colors = {
> > +			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> > +			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> > +			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> > +			{ "red",   { 0x36, 0x63, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> > +			{ "green", { 0xb6, 0x1e, 0x0c }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> > +			{ "blue",  { 0x12, 0xff, 0x74 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> > +		},
> > +	},
> > +	/*

[...]

> > +/*
> > + * vkms_format_test_yuv_u8_to_argb_u16 - Testing the conversion between YUV
> > + * colors to ARGB colors in VKMS
> > + *
> > + * This test will use the functions get_conversion_matrix_to_argb_u16 and
> > + * argb_u16_from_yuv888 to convert YUV colors (stored in
> > + * yuv_u8_to_argb_u16_cases) into ARGB colors.
> > + *
> > + * The conversion between YUV and RGB is not totally reversible, so there may be
> > + * some difference between the expected value and the result.
> > + * In addition, there may be some rounding error as the input color is 8 bits
> > + * and output color is 16 bits.
> > + */
> > +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];
> > +		struct conversion_matrix matrix;
> > +
> > +		get_conversion_matrix_to_argb_u16
> > +			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
> > +
> > +		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);
> 
> Running the test on ppc64 (big endian) doesn't fail. For reference:
> 
>   $ sudo dnf install powerpc64-linux-gnu-gcc
>   $ sudo dnf install qemu-system-ppc64
>   $ ./tools/testing/kunit/kunit.py run \
>      --kunitconfig=drivers/gpu/drm/vkms/tests \
>      --arch=powerpc --cross_compile=powerpc64-linux-gnu- \
>      --make_options CF=-D__CHECK_ENDIAN__ \
>      --kconfig_add CONFIG_KASAN=y \
>      --kconfig_add CONFIG_KASAN_VMALLOC=y
> 
> However, I wonder if endianness is correctly handled. I always find endianness
> difficult to reason about, but I'll try my best to explain it.
> 
> On a big endian architecture, color->yuv is stored in big endian. This might not
> be an issue, because its components (y, u and v) are u8.
> However, I think that the return value of argb_u16_from_yuv888(), which is the
> result of argb_u16_from_u16161616(), is returned in big endian while it should
> be little endian.

The goal of `struct argb_u16` is to hide machine-specific issues. We want 
to be able to do addition, multiplication... without 
`le_from_cpu`/`cpu_to_le` everywhere.

If you look at the rest of the vkms driver, we never do bit manipulation 
on `struct argb_u16`, only mathematical operations. 

> Since you are comparing argb.a (big endian) with color->argb.a (big endian) the
> test succedess, but in this case it should fail because, if I remember
> correctly, colors must be stored in little endian and therefore, the color
> returned by argb_u16_from_yuv888() should be little endian.

The colors are stored in a specific endian only in framebuffers, but in 
our case, this is not a framebuffer. For the `argb_u16_to_ARGB16161616`, 
you can see we use `cpu_to_le16` to store the data in the proper order.
 
> If you replace this 4 KUNIT_EXPECT_LE_MSG() with KUNIT_EXPECT_MEMEQ(), all test
> will fail, but you'll notice that the buffers printed in the error log are
> different depending on the endianness (x86_64 vs ppc64).
> 
> What do you think? Did I overlook the conversion?

I think yes, but thanks to make me think about it, I will steal your 
command line to test on powerPC :)

> Have a look to the tests present in drm_format_helper_test.c. They use different
> functions (cpubuf_to_le32, le32buf_to_cpu, etc) to make sure that colors are
> represented in little endian and that comparing the expected and actual results
> happens in the same endian.

Those tests are testing conversion "buffer to buffer", so yes, there is 
some endian-dependant issues.

[...]

Re: [PATCH v16 3/7] drm/vkms: Drop YUV formats TODO

[Louis Chauvet]

On 31/01/25 - 09:40, José Expósito wrote:
> Hi Louis,
> 
> Thanks a lot for the patches.
> 
> I'm not well versed in YUV color formats, so I did my best reading the kernel
> documentation before reviewing this series... But I'll most likely ask some
> basic/dump questions.
> 
> > From: Arthur Grillo <arthurgrillo@riseup.net>
> > 
> > VKMS has support for YUV formats now. Remove the task from the TODO
> > list.
> > 
> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > ---
> >  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 ba04ac7c2167a9d484c54c69a09a2fb8f2d9c0aa..13b866c3617cd44043406252d3caa912c931772f 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.
> 
> I see that you implemented support for 6 DRM_FORMAT_NV* formats, but
> DRM_FORMAT_NV15, DRM_FORMAT_NV20 and DRM_FORMAT_NV30 are not implemented.
> 
> The same applies to DRM_FORMAT_Y210 or DRM_FORMAT_YUV410 among others.
> 
> Could it be useful to implement all of them in the future? If so, should we add
> it to the ToDo list?

I don't think we need "all of them" (there are ≈100 + all the modifiers), 
but definitly all the commonly used ones (I have some of the "common" one 
ready here [1], I just wait for the YUV series to be accepted to avoid 
conflicts).

> It might be a great task to get started in kernel development, as there are
> already similar examples and tests.

I don't think we need to specify which format are missing, the point 
"Additionnal buffer formats. [...]" seems sufficient. If you think this is 
relevant, I can add "Easy task" so beginners will find it easier?
 
[1]:https://lore.kernel.org/all/20241122-b4-new-color-formats-v3-0-23f7776197c9@bootlin.com/

> >  
> >  - Async updates (currently only possible on cursor plane using the legacy
> >    cursor api).
> > 

Re: [PATCH v16 3/7] drm/vkms: Drop YUV formats TODO

[José Expósito]

On Fri, Jan 31, 2025 at 02:02:14PM +0100, Louis Chauvet wrote:
> On 31/01/25 - 09:40, José Expósito wrote:
> > Hi Louis,
> > 
> > Thanks a lot for the patches.
> > 
> > I'm not well versed in YUV color formats, so I did my best reading the kernel
> > documentation before reviewing this series... But I'll most likely ask some
> > basic/dump questions.
> > 
> > > From: Arthur Grillo <arthurgrillo@riseup.net>
> > > 
> > > VKMS has support for YUV formats now. Remove the task from the TODO
> > > list.
> > > 
> > > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> > >  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 ba04ac7c2167a9d484c54c69a09a2fb8f2d9c0aa..13b866c3617cd44043406252d3caa912c931772f 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.
> > 
> > I see that you implemented support for 6 DRM_FORMAT_NV* formats, but
> > DRM_FORMAT_NV15, DRM_FORMAT_NV20 and DRM_FORMAT_NV30 are not implemented.
> > 
> > The same applies to DRM_FORMAT_Y210 or DRM_FORMAT_YUV410 among others.
> > 
> > Could it be useful to implement all of them in the future? If so, should we add
> > it to the ToDo list?
> 
> I don't think we need "all of them" (there are ≈100 + all the modifiers), 
> but definitly all the commonly used ones (I have some of the "common" one 
> ready here [1], I just wait for the YUV series to be accepted to avoid 
> conflicts).

Good to know, thanks for the clarification. I think we are good with this
to-do item as it is. There's plenty of work that can be done :)

Jose

> > It might be a great task to get started in kernel development, as there are
> > already similar examples and tests.
> 
> I don't think we need to specify which format are missing, the point 
> "Additionnal buffer formats. [...]" seems sufficient. If you think this is 
> relevant, I can add "Easy task" so beginners will find it easier?
>  
> [1]:https://lore.kernel.org/all/20241122-b4-new-color-formats-v3-0-23f7776197c9@bootlin.com/
> 
> > >  
> > >  - Async updates (currently only possible on cursor plane using the legacy
> > >    cursor api).
> > > 

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

[José Expósito]

On Fri, Jan 31, 2025 at 02:02:14PM +0100, Louis Chauvet wrote:
> On 31/01/25 - 09:41, José Expósito wrote:
> > Hi Louis,
> > 
> > > From: Arthur Grillo <arthurgrillo@riseup.net>
> > > 
> > > Create KUnit tests to test the conversion between YUV and RGB. Test each
> > > conversion and range combination with some common colors.
> > > 
> > > The code used to compute the expected result can be found in comment.
> > >
> > > [Louis Chauvet:
> > > - fix minor formating issues (whitespace, double line)
> > > - change expected alpha from 0x0000 to 0xffff
> > > - adapt to the new get_conversion_matrix usage
> > > - apply the changes from Arthur
> > > - move struct pixel_yuv_u8 to the test itself]
> > > 
> > > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > > Acked-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > > Signed-off-by: Louis Chauvet <louis.chauvet@bootlin.com>
> > > ---
> 
> [...]
> 
> > > +	/*
> > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.709"],
> > > +	 *                     in_bits = 16,
> > > +	 *                     in_legal = False,
> > > +	 *                     in_int = True,
> > > +	 *                     out_bits = 8,
> > > +	 *                     out_legal = False,
> > > +	 *                     out_int = True)
> > > +	 * Test cases for conversion between YUV BT709 full range and RGB
> > > +	 * using the ITU-R BT.709 weights.
> > > +	 */
> > > +	{
> > > +		.encoding = DRM_COLOR_YCBCR_BT709,
> > > +		.range = DRM_COLOR_YCBCR_FULL_RANGE,
> > > +		.n_colors = 4,
> > 
> > If I understood correctly, "n_colors" here indicates the number of items in
> > "colors", but there is a mismatch between both lengths.
> > 
> > It also applies to the other test cases where "n_colors = 4".
> 
> I don't know how I miss it, I am 100% sure I did the exact same comment to 
> Arthur few mounth ago, thanks!
>  
> > > +		.colors = {
> > > +			{ "white", { 0xff, 0x80, 0x80 }, { 0xffff, 0xffff, 0xffff, 0xffff }},
> > > +			{ "gray",  { 0x80, 0x80, 0x80 }, { 0xffff, 0x8080, 0x8080, 0x8080 }},
> > > +			{ "black", { 0x00, 0x80, 0x80 }, { 0xffff, 0x0000, 0x0000, 0x0000 }},
> > > +			{ "red",   { 0x36, 0x63, 0xff }, { 0xffff, 0xffff, 0x0000, 0x0000 }},
> > > +			{ "green", { 0xb6, 0x1e, 0x0c }, { 0xffff, 0x0000, 0xffff, 0x0000 }},
> > > +			{ "blue",  { 0x12, 0xff, 0x74 }, { 0xffff, 0x0000, 0x0000, 0xffff }},
> > > +		},
> > > +	},
> > > +	/*
> 
> [...]
> 
> > > +/*
> > > + * vkms_format_test_yuv_u8_to_argb_u16 - Testing the conversion between YUV
> > > + * colors to ARGB colors in VKMS
> > > + *
> > > + * This test will use the functions get_conversion_matrix_to_argb_u16 and
> > > + * argb_u16_from_yuv888 to convert YUV colors (stored in
> > > + * yuv_u8_to_argb_u16_cases) into ARGB colors.
> > > + *
> > > + * The conversion between YUV and RGB is not totally reversible, so there may be
> > > + * some difference between the expected value and the result.
> > > + * In addition, there may be some rounding error as the input color is 8 bits
> > > + * and output color is 16 bits.
> > > + */
> > > +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];
> > > +		struct conversion_matrix matrix;
> > > +
> > > +		get_conversion_matrix_to_argb_u16
> > > +			(DRM_FORMAT_NV12, param->encoding, param->range, &matrix);
> > > +
> > > +		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, &matrix);
> > 
> > Running the test on ppc64 (big endian) doesn't fail. For reference:
> > 
> >   $ sudo dnf install powerpc64-linux-gnu-gcc
> >   $ sudo dnf install qemu-system-ppc64
> >   $ ./tools/testing/kunit/kunit.py run \
> >      --kunitconfig=drivers/gpu/drm/vkms/tests \
> >      --arch=powerpc --cross_compile=powerpc64-linux-gnu- \
> >      --make_options CF=-D__CHECK_ENDIAN__ \
> >      --kconfig_add CONFIG_KASAN=y \
> >      --kconfig_add CONFIG_KASAN_VMALLOC=y
> > 
> > However, I wonder if endianness is correctly handled. I always find endianness
> > difficult to reason about, but I'll try my best to explain it.
> > 
> > On a big endian architecture, color->yuv is stored in big endian. This might not
> > be an issue, because its components (y, u and v) are u8.
> > However, I think that the return value of argb_u16_from_yuv888(), which is the
> > result of argb_u16_from_u16161616(), is returned in big endian while it should
> > be little endian.
> 
> The goal of `struct argb_u16` is to hide machine-specific issues. We want 
> to be able to do addition, multiplication... without 
> `le_from_cpu`/`cpu_to_le` everywhere.
> 
> If you look at the rest of the vkms driver, we never do bit manipulation 
> on `struct argb_u16`, only mathematical operations. 
> 
> > Since you are comparing argb.a (big endian) with color->argb.a (big endian) the
> > test succedess, but in this case it should fail because, if I remember
> > correctly, colors must be stored in little endian and therefore, the color
> > returned by argb_u16_from_yuv888() should be little endian.
> 
> The colors are stored in a specific endian only in framebuffers, but in 
> our case, this is not a framebuffer. For the `argb_u16_to_ARGB16161616`, 
> you can see we use `cpu_to_le16` to store the data in the proper order.
>  
> > If you replace this 4 KUNIT_EXPECT_LE_MSG() with KUNIT_EXPECT_MEMEQ(), all test
> > will fail, but you'll notice that the buffers printed in the error log are
> > different depending on the endianness (x86_64 vs ppc64).
> > 
> > What do you think? Did I overlook the conversion?
> 
> I think yes, but thanks to make me think about it, I will steal your 
> command line to test on powerPC :)

Well, at least the command was useful :P Thanks for the explanation.
I have been looking with more detail into get_pixel_write_function()
and what you mention makes sense now.

Thanks!

> > Have a look to the tests present in drm_format_helper_test.c. They use different
> > functions (cpubuf_to_le32, le32buf_to_cpu, etc) to make sure that colors are
> > represented in little endian and that comparing the expected and actual results
> > happens in the same endian.
> 
> Those tests are testing conversion "buffer to buffer", so yes, there is 
> some endian-dependant issues.
> 
> [...]

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 2102 bytes --]

On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
> On 26/01/25 - 18:06, Maxime Ripard wrote:
> > On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > +	/*
> > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > +	 *                     in_bits = 16,
> > > +	 *                     in_legal = False,
> > > +	 *                     in_int = True,
> > > +	 *                     out_bits = 8,
> > > +	 *                     out_legal = False,
> > > +	 *                     out_int = True)
> > > +	 *
> > > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > > +	 * using the ITU-R BT.601 weights.
> > > +	 */
> > 
> > What are the input and output formats?
> > 
> > Ditto for all the other tests.
> 
> There is no really "input" and "output" format, they are reference values 
> for conversion, you should be able to use it in both direction. They are 
> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was 
> easier to create the colors from RGB values.

RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
NV12 is a format.

> If you think we should specify what is was used as input and output to 
> generate those values, I can modify the comment to:
> 
> 	Tests cases for color conversion generated by converting RGB 
> 	values to YUV BT601 full range using the ITU-R BT.601 weights.

My point is that those comments should provide a way to reimplement the
test from scratch, and compare to the actual implementation. It's useful
when you have a test failure and start to wonder if the implementation
or the test is at fault.

By saying only RGB and YUV, you can't possibly do that.

> Beside that modification, did you notice anything else on the series that 
> require more work before adding your Ack-by/Reviewed-by on the other 
> patches?

The rest looked good to me the last time I looked.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Louis Chauvet]

On 05/02/25 - 09:55, Maxime Ripard wrote:
> On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
> > On 26/01/25 - 18:06, Maxime Ripard wrote:
> > > On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > > +	/*
> > > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > > +	 *                     in_bits = 16,
> > > > +	 *                     in_legal = False,
> > > > +	 *                     in_int = True,
> > > > +	 *                     out_bits = 8,
> > > > +	 *                     out_legal = False,
> > > > +	 *                     out_int = True)
> > > > +	 *
> > > > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > > > +	 * using the ITU-R BT.601 weights.
> > > > +	 */
> > > 
> > > What are the input and output formats?
> > > 
> > > Ditto for all the other tests.
> > 
> > There is no really "input" and "output" format, they are reference values 
> > for conversion, you should be able to use it in both direction. They are 
> > generated by RGB_to_YCbCr (RGB input, YUV output) just because it was 
> > easier to create the colors from RGB values.
> 
> RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
> NV12 is a format.
>
> > If you think we should specify what is was used as input and output to 
> > generate those values, I can modify the comment to:
> > 
> > 	Tests cases for color conversion generated by converting RGB 
> > 	values to YUV BT601 full range using the ITU-R BT.601 weights.
> 
> My point is that those comments should provide a way to reimplement the
> test from scratch, and compare to the actual implementation. It's useful
> when you have a test failure and start to wonder if the implementation
> or the test is at fault.
> 
> By saying only RGB and YUV, you can't possibly do that.

I understand your concern, but I believe there might be a slight 
misunderstanding. The table in question stores reference values for 
specific color models, not formats. Therefore, it doesn't specify any 
particular format like XRGB8888 or NV12.

To clarify this, I can rename the format_pair struct to value_pair. This 
should make it clearer that we are dealing with color model values rather 
than formats.

If you want to test a specific format conversion, such as 
YUV420_to_argbu16, you would need to follow a process like this:

	// Recreate a YUV420 data
	plane_1[0] = test_case.yuv.y
	plane_2[0] = test_case.yuv.u
	plane_2[1] = test_case.yuv.v

	// convertion to test from YUV420 format to argb_u16
	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)

	// ensure the conversion is valid
	assert_eq(rgb_u16, test_case.rgb)

The current test is not performing this kind of format conversion. 
Instead, it verifies that for given (y, u, v) values, the correct (r, g, 
b, a) values are obtained. In other words, it tests color model 
conversion, not format conversion.

Do you think I need to change something in this test? If so, can you 
explain what kind of unit test you are expecting.

Thanks,
Louis Chauvet

> > Beside that modification, did you notice anything else on the series that 
> > require more work before adding your Ack-by/Reviewed-by on the other 
> > patches?
> 
> The rest looked good to me the last time I looked.
>
> Maxime

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 3984 bytes --]

On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:
> On 05/02/25 - 09:55, Maxime Ripard wrote:
> > On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
> > > On 26/01/25 - 18:06, Maxime Ripard wrote:
> > > > On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > > > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > > > +	/*
> > > > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > > > +	 *                     in_bits = 16,
> > > > > +	 *                     in_legal = False,
> > > > > +	 *                     in_int = True,
> > > > > +	 *                     out_bits = 8,
> > > > > +	 *                     out_legal = False,
> > > > > +	 *                     out_int = True)
> > > > > +	 *
> > > > > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > > > > +	 * using the ITU-R BT.601 weights.
> > > > > +	 */
> > > > 
> > > > What are the input and output formats?
> > > > 
> > > > Ditto for all the other tests.
> > > 
> > > There is no really "input" and "output" format, they are reference values 
> > > for conversion, you should be able to use it in both direction. They are 
> > > generated by RGB_to_YCbCr (RGB input, YUV output) just because it was 
> > > easier to create the colors from RGB values.
> > 
> > RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
> > NV12 is a format.
> >
> > > If you think we should specify what is was used as input and output to 
> > > generate those values, I can modify the comment to:
> > > 
> > > 	Tests cases for color conversion generated by converting RGB 
> > > 	values to YUV BT601 full range using the ITU-R BT.601 weights.
> > 
> > My point is that those comments should provide a way to reimplement the
> > test from scratch, and compare to the actual implementation. It's useful
> > when you have a test failure and start to wonder if the implementation
> > or the test is at fault.
> > 
> > By saying only RGB and YUV, you can't possibly do that.
> 
> I understand your concern, but I believe there might be a slight 
> misunderstanding. The table in question stores reference values for 
> specific color models, not formats. Therefore, it doesn't specify any 
> particular format like XRGB8888 or NV12.
> 
> To clarify this, I can rename the format_pair struct to value_pair. This 
> should make it clearer that we are dealing with color model values rather 
> than formats.
> 
> If you want to test a specific format conversion, such as 
> YUV420_to_argbu16, you would need to follow a process like this:
> 
> 	// Recreate a YUV420 data
> 	plane_1[0] = test_case.yuv.y
> 	plane_2[0] = test_case.yuv.u
> 	plane_2[1] = test_case.yuv.v
> 
> 	// convertion to test from YUV420 format to argb_u16
> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
> 
> 	// ensure the conversion is valid
> 	assert_eq(rgb_u16, test_case.rgb)
> 
> The current test is not performing this kind of format conversion. 
> Instead, it verifies that for given (y, u, v) values, the correct (r, g, 
> b, a) values are obtained.

You already stated that you check for the A, R, G, and B components. On
how many bits are the values you are comparing stored? The YUV values
you are comparing are stored on how many bits for each channel? With
subsampling?

If you want to compare values, you need to encode a given color into
bits, and the way that encoding is done is what the format is about.

You might not compare the memory layout but each component individually,
but it's still a format.

And then, you have the extra fun on top, like are you comparing
full-range or limited-range colors?

> In other words, it tests color model conversion, not format conversion.

No, you are testing color encoding, format and model conversions, all at
once.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 273 bytes --]

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

[Louis Chauvet]

Le 19/02/2025 à 11:15, Maxime Ripard a écrit :
> On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:
>> On 05/02/25 - 09:55, Maxime Ripard wrote:
>>> On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
>>>> On 26/01/25 - 18:06, Maxime Ripard wrote:
>>>>> On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
>>>>>> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>>>>>> +	/*
>>>>>> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
>>>>>> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
>>>>>> +	 *                     in_bits = 16,
>>>>>> +	 *                     in_legal = False,
>>>>>> +	 *                     in_int = True,
>>>>>> +	 *                     out_bits = 8,
>>>>>> +	 *                     out_legal = False,
>>>>>> +	 *                     out_int = True)
>>>>>> +	 *
>>>>>> +	 * Test cases for conversion between YUV BT601 full range and RGB
>>>>>> +	 * using the ITU-R BT.601 weights.
>>>>>> +	 */
>>>>>
>>>>> What are the input and output formats?
>>>>>
>>>>> Ditto for all the other tests.
>>>>
>>>> There is no really "input" and "output" format, they are reference values
>>>> for conversion, you should be able to use it in both direction. They are
>>>> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was
>>>> easier to create the colors from RGB values.
>>>
>>> RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
>>> NV12 is a format.
>>>
>>>> If you think we should specify what is was used as input and output to
>>>> generate those values, I can modify the comment to:
>>>>
>>>> 	Tests cases for color conversion generated by converting RGB
>>>> 	values to YUV BT601 full range using the ITU-R BT.601 weights.
>>>
>>> My point is that those comments should provide a way to reimplement the
>>> test from scratch, and compare to the actual implementation. It's useful
>>> when you have a test failure and start to wonder if the implementation
>>> or the test is at fault.
>>>
>>> By saying only RGB and YUV, you can't possibly do that.
>>
>> I understand your concern, but I believe there might be a slight
>> misunderstanding. The table in question stores reference values for
>> specific color models, not formats. Therefore, it doesn't specify any
>> particular format like XRGB8888 or NV12.
>>
>> To clarify this, I can rename the format_pair struct to value_pair. This
>> should make it clearer that we are dealing with color model values rather
>> than formats.
>>
>> If you want to test a specific format conversion, such as
>> YUV420_to_argbu16, you would need to follow a process like this:
>>
>> 	// Recreate a YUV420 data
>> 	plane_1[0] = test_case.yuv.y
>> 	plane_2[0] = test_case.yuv.u
>> 	plane_2[1] = test_case.yuv.v
>>
>> 	// convertion to test from YUV420 format to argb_u16
>> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
>>
>> 	// ensure the conversion is valid
>> 	assert_eq(rgb_u16, test_case.rgb)
>>
>> The current test is not performing this kind of format conversion.
>> Instead, it verifies that for given (y, u, v) values, the correct (r, g,
>> b, a) values are obtained.
> 
> You already stated that you check for the A, R, G, and B components. On
> how many bits are the values you are comparing stored? The YUV values
> you are comparing are stored on how many bits for each channel? With
> subsampling?
> 
> If you want to compare values, you need to encode a given color into
> bits, and the way that encoding is done is what the format is about.
> 
> You might not compare the memory layout but each component individually,
> but it's still a format.

Sorry, I think I misunderstood what a format really is. But even with 
this explanation, I don't understand well what you ask me to change. Is 
this better:

The values are computed by converting RGB values, with each component 
stored as u16, to YUV values, with each component stored as u8. The 
conversion is done from RGB full range to YUV BT601 full range using the 
ITU-R BT.601 weights.

TBH, I do not understand what you are asking for exactly. Can you please 
give the sentence you expect directly?

Thanks,
Louis Chauvet

> And then, you have the extra fun on top, like are you comparing
> full-range or limited-range colors?
> 
>> In other words, it tests color model conversion, not format conversion.
> 
> No, you are testing color encoding, format and model conversions, all at
> once.
> 
> Maxime

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 5538 bytes --]

On Wed, Feb 19, 2025 at 02:35:14PM +0100, Louis Chauvet wrote:
> 
> 
> Le 19/02/2025 à 11:15, Maxime Ripard a écrit :
> > On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:
> > > On 05/02/25 - 09:55, Maxime Ripard wrote:
> > > > On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
> > > > > On 26/01/25 - 18:06, Maxime Ripard wrote:
> > > > > > On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
> > > > > > > +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > > > > > > +	/*
> > > > > > > +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > > > > > > +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > > > > > > +	 *                     in_bits = 16,
> > > > > > > +	 *                     in_legal = False,
> > > > > > > +	 *                     in_int = True,
> > > > > > > +	 *                     out_bits = 8,
> > > > > > > +	 *                     out_legal = False,
> > > > > > > +	 *                     out_int = True)
> > > > > > > +	 *
> > > > > > > +	 * Test cases for conversion between YUV BT601 full range and RGB
> > > > > > > +	 * using the ITU-R BT.601 weights.
> > > > > > > +	 */
> > > > > > 
> > > > > > What are the input and output formats?
> > > > > > 
> > > > > > Ditto for all the other tests.
> > > > > 
> > > > > There is no really "input" and "output" format, they are reference values
> > > > > for conversion, you should be able to use it in both direction. They are
> > > > > generated by RGB_to_YCbCr (RGB input, YUV output) just because it was
> > > > > easier to create the colors from RGB values.
> > > > 
> > > > RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
> > > > NV12 is a format.
> > > > 
> > > > > If you think we should specify what is was used as input and output to
> > > > > generate those values, I can modify the comment to:
> > > > > 
> > > > > 	Tests cases for color conversion generated by converting RGB
> > > > > 	values to YUV BT601 full range using the ITU-R BT.601 weights.
> > > > 
> > > > My point is that those comments should provide a way to reimplement the
> > > > test from scratch, and compare to the actual implementation. It's useful
> > > > when you have a test failure and start to wonder if the implementation
> > > > or the test is at fault.
> > > > 
> > > > By saying only RGB and YUV, you can't possibly do that.
> > > 
> > > I understand your concern, but I believe there might be a slight
> > > misunderstanding. The table in question stores reference values for
> > > specific color models, not formats. Therefore, it doesn't specify any
> > > particular format like XRGB8888 or NV12.
> > > 
> > > To clarify this, I can rename the format_pair struct to value_pair. This
> > > should make it clearer that we are dealing with color model values rather
> > > than formats.
> > > 
> > > If you want to test a specific format conversion, such as
> > > YUV420_to_argbu16, you would need to follow a process like this:
> > > 
> > > 	// Recreate a YUV420 data
> > > 	plane_1[0] = test_case.yuv.y
> > > 	plane_2[0] = test_case.yuv.u
> > > 	plane_2[1] = test_case.yuv.v
> > > 
> > > 	// convertion to test from YUV420 format to argb_u16
> > > 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
> > > 
> > > 	// ensure the conversion is valid
> > > 	assert_eq(rgb_u16, test_case.rgb)
> > > 
> > > The current test is not performing this kind of format conversion.
> > > Instead, it verifies that for given (y, u, v) values, the correct (r, g,
> > > b, a) values are obtained.
> > 
> > You already stated that you check for the A, R, G, and B components. On
> > how many bits are the values you are comparing stored? The YUV values
> > you are comparing are stored on how many bits for each channel? With
> > subsampling?
> > 
> > If you want to compare values, you need to encode a given color into
> > bits, and the way that encoding is done is what the format is about.
> > 
> > You might not compare the memory layout but each component individually,
> > but it's still a format.
> 
> Sorry, I think I misunderstood what a format really is.

Ultimately, a format is how a given "color value" is stored. How many
bits will you use? If you have an unaligned number of bits, how many
bits of padding you'll use, where the padding is? If there's multiple
bytes, what's the endianness?

The answer to all these questions is "the format", and that's why
there's so many of them.

> But even with this explanation, I don't understand well what you ask
> me to change. Is this better:
> 
> The values are computed by converting RGB values, with each component stored
> as u16, to YUV values, with each component stored as u8. The conversion is
> done from RGB full range to YUV BT601 full range using the ITU-R BT.601
> weights.
> 
> TBH, I do not understand what you are asking for exactly. Can you please
> give the sentence you expect directly?

The fourcc[1] code for the input and output format would be nice. And if
you can't, an ad-hoc definition of the format, answering the questions I
mentionned earlier (and in the previous mail for YUV). I'm really
surprised about the RGB component values being stored on 16 bits though.
It's super unusual, to the point where it's almost useless for us to
test, and we should probably use 8 bits values.

Maxime

1: https://elixir.bootlin.com/linux/v6.13.5/source/include/uapi/drm/drm_fourcc.h#L486

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

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

[Louis Chauvet]

Le 07/03/2025 à 11:20, Maxime Ripard a écrit :
> On Wed, Feb 19, 2025 at 02:35:14PM +0100, Louis Chauvet wrote:
>>
>>
>> Le 19/02/2025 à 11:15, Maxime Ripard a écrit :
>>> On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:
>>>> On 05/02/25 - 09:55, Maxime Ripard wrote:
>>>>> On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:
>>>>>> On 26/01/25 - 18:06, Maxime Ripard wrote:
>>>>>>> On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:
>>>>>>>> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>>>>>>>> +	/*
>>>>>>>> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
>>>>>>>> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
>>>>>>>> +	 *                     in_bits = 16,
>>>>>>>> +	 *                     in_legal = False,
>>>>>>>> +	 *                     in_int = True,
>>>>>>>> +	 *                     out_bits = 8,
>>>>>>>> +	 *                     out_legal = False,
>>>>>>>> +	 *                     out_int = True)
>>>>>>>> +	 *
>>>>>>>> +	 * Test cases for conversion between YUV BT601 full range and RGB
>>>>>>>> +	 * using the ITU-R BT.601 weights.
>>>>>>>> +	 */
>>>>>>>
>>>>>>> What are the input and output formats?
>>>>>>>
>>>>>>> Ditto for all the other tests.
>>>>>>
>>>>>> There is no really "input" and "output" format, they are reference values
>>>>>> for conversion, you should be able to use it in both direction. They are
>>>>>> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was
>>>>>> easier to create the colors from RGB values.
>>>>>
>>>>> RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
>>>>> NV12 is a format.
>>>>>
>>>>>> If you think we should specify what is was used as input and output to
>>>>>> generate those values, I can modify the comment to:
>>>>>>
>>>>>> 	Tests cases for color conversion generated by converting RGB
>>>>>> 	values to YUV BT601 full range using the ITU-R BT.601 weights.
>>>>>
>>>>> My point is that those comments should provide a way to reimplement the
>>>>> test from scratch, and compare to the actual implementation. It's useful
>>>>> when you have a test failure and start to wonder if the implementation
>>>>> or the test is at fault.
>>>>>
>>>>> By saying only RGB and YUV, you can't possibly do that.
>>>>
>>>> I understand your concern, but I believe there might be a slight
>>>> misunderstanding. The table in question stores reference values for
>>>> specific color models, not formats. Therefore, it doesn't specify any
>>>> particular format like XRGB8888 or NV12.
>>>>
>>>> To clarify this, I can rename the format_pair struct to value_pair. This
>>>> should make it clearer that we are dealing with color model values rather
>>>> than formats.
>>>>
>>>> If you want to test a specific format conversion, such as
>>>> YUV420_to_argbu16, you would need to follow a process like this:
>>>>
>>>> 	// Recreate a YUV420 data
>>>> 	plane_1[0] = test_case.yuv.y
>>>> 	plane_2[0] = test_case.yuv.u
>>>> 	plane_2[1] = test_case.yuv.v
>>>>
>>>> 	// convertion to test from YUV420 format to argb_u16
>>>> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
>>>>
>>>> 	// ensure the conversion is valid
>>>> 	assert_eq(rgb_u16, test_case.rgb)
>>>>
>>>> The current test is not performing this kind of format conversion.
>>>> Instead, it verifies that for given (y, u, v) values, the correct (r, g,
>>>> b, a) values are obtained.
>>>
>>> You already stated that you check for the A, R, G, and B components. On
>>> how many bits are the values you are comparing stored? The YUV values
>>> you are comparing are stored on how many bits for each channel? With
>>> subsampling?
>>>
>>> If you want to compare values, you need to encode a given color into
>>> bits, and the way that encoding is done is what the format is about.
>>>
>>> You might not compare the memory layout but each component individually,
>>> but it's still a format.
>>
>> Sorry, I think I misunderstood what a format really is.
> 
> Ultimately, a format is how a given "color value" is stored. How many
> bits will you use? If you have an unaligned number of bits, how many
> bits of padding you'll use, where the padding is? If there's multiple
> bytes, what's the endianness?
> 
> The answer to all these questions is "the format", and that's why
> there's so many of them.

Thanks!

>> But even with this explanation, I don't understand well what you ask
>> me to change. Is this better:
>>
>> The values are computed by converting RGB values, with each component stored
>> as u16, to YUV values, with each component stored as u8. The conversion is
>> done from RGB full range to YUV BT601 full range using the ITU-R BT.601
>> weights.
>>
>> TBH, I do not understand what you are asking for exactly. Can you please
>> give the sentence you expect directly?
> 
> The fourcc[1] code for the input and output format would be nice. And if
> you can't, an ad-hoc definition of the format, answering the questions I
> mentionned earlier (and in the previous mail for YUV).

I don't think any fourcc code will apply in this case, the tests use 
internal VKMS structures pixel_argb_16 and pixel_yuv_u8. How do I 
describe them better? If I add this comment for the structures, is it 
enough?

/**
  * struct pixel_argb_u16 - Internal representation of a pixel color.
  * @r: Red component value, stored in 16 bits, without padding, using
  *     machine endianness
  * @b: [...]
  *
  * The goal of this structure is to keep enough precision to ensure
  * correct composition results in VKMS and simplifying color
  * manipulation by splitting each component into its own field.
  * Caution: the byte ordering of this structure is machine-dependent,
  * you can't cast it directly to AR48 or xR48.
  */
struct pixel_argb_u16 {
	u16 a, r, g, b;
};

(ditto for pixel_yuv_u8)

> I'm really
> surprised about the RGB component values being stored on 16 bits though.
> It's super unusual, to the point where it's almost useless for us to
> test, and we should probably use 8 bits values.

We need to have 16 bits because some of the writeback formats are 16 bits.

Louis Chauvet

> Maxime
> 
> 1: https://elixir.bootlin.com/linux/v6.13.5/source/include/uapi/drm/drm_fourcc.h#L486

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

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

[Pekka Paalanen]

[-- Attachment #1: Type: text/plain, Size: 8712 bytes --]

On Fri, 7 Mar 2025 15:50:41 +0100
Louis Chauvet <louis.chauvet@bootlin.com> wrote:

> Le 07/03/2025 à 11:20, Maxime Ripard a écrit :
> > On Wed, Feb 19, 2025 at 02:35:14PM +0100, Louis Chauvet wrote:  
> >>
> >>
> >> Le 19/02/2025 à 11:15, Maxime Ripard a écrit :  
> >>> On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:  
> >>>> On 05/02/25 - 09:55, Maxime Ripard wrote:  
> >>>>> On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:  
> >>>>>> On 26/01/25 - 18:06, Maxime Ripard wrote:  
> >>>>>>> On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:  
> >>>>>>>> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> >>>>>>>> +	/*
> >>>>>>>> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> >>>>>>>> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> >>>>>>>> +	 *                     in_bits = 16,
> >>>>>>>> +	 *                     in_legal = False,
> >>>>>>>> +	 *                     in_int = True,
> >>>>>>>> +	 *                     out_bits = 8,
> >>>>>>>> +	 *                     out_legal = False,
> >>>>>>>> +	 *                     out_int = True)
> >>>>>>>> +	 *
> >>>>>>>> +	 * Test cases for conversion between YUV BT601 full range and RGB
> >>>>>>>> +	 * using the ITU-R BT.601 weights.
> >>>>>>>> +	 */  
> >>>>>>>
> >>>>>>> What are the input and output formats?
> >>>>>>>
> >>>>>>> Ditto for all the other tests.  
> >>>>>>
> >>>>>> There is no really "input" and "output" format, they are reference values
> >>>>>> for conversion, you should be able to use it in both direction. They are
> >>>>>> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was
> >>>>>> easier to create the colors from RGB values.  
> >>>>>
> >>>>> RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
> >>>>> NV12 is a format.
> >>>>>  
> >>>>>> If you think we should specify what is was used as input and output to
> >>>>>> generate those values, I can modify the comment to:
> >>>>>>
> >>>>>> 	Tests cases for color conversion generated by converting RGB
> >>>>>> 	values to YUV BT601 full range using the ITU-R BT.601 weights.  
> >>>>>
> >>>>> My point is that those comments should provide a way to reimplement the
> >>>>> test from scratch, and compare to the actual implementation. It's useful
> >>>>> when you have a test failure and start to wonder if the implementation
> >>>>> or the test is at fault.
> >>>>>
> >>>>> By saying only RGB and YUV, you can't possibly do that.  
> >>>>
> >>>> I understand your concern, but I believe there might be a slight
> >>>> misunderstanding. The table in question stores reference values for
> >>>> specific color models, not formats. Therefore, it doesn't specify any
> >>>> particular format like XRGB8888 or NV12.
> >>>>
> >>>> To clarify this, I can rename the format_pair struct to value_pair. This
> >>>> should make it clearer that we are dealing with color model values rather
> >>>> than formats.
> >>>>
> >>>> If you want to test a specific format conversion, such as
> >>>> YUV420_to_argbu16, you would need to follow a process like this:
> >>>>
> >>>> 	// Recreate a YUV420 data
> >>>> 	plane_1[0] = test_case.yuv.y
> >>>> 	plane_2[0] = test_case.yuv.u
> >>>> 	plane_2[1] = test_case.yuv.v
> >>>>
> >>>> 	// convertion to test from YUV420 format to argb_u16
> >>>> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
> >>>>
> >>>> 	// ensure the conversion is valid
> >>>> 	assert_eq(rgb_u16, test_case.rgb)
> >>>>
> >>>> The current test is not performing this kind of format conversion.
> >>>> Instead, it verifies that for given (y, u, v) values, the correct (r, g,
> >>>> b, a) values are obtained.  
> >>>
> >>> You already stated that you check for the A, R, G, and B components. On
> >>> how many bits are the values you are comparing stored? The YUV values
> >>> you are comparing are stored on how many bits for each channel? With
> >>> subsampling?
> >>>
> >>> If you want to compare values, you need to encode a given color into
> >>> bits, and the way that encoding is done is what the format is about.
> >>>
> >>> You might not compare the memory layout but each component individually,
> >>> but it's still a format.  
> >>
> >> Sorry, I think I misunderstood what a format really is.  
> > 
> > Ultimately, a format is how a given "color value" is stored. How many
> > bits will you use? If you have an unaligned number of bits, how many
> > bits of padding you'll use, where the padding is? If there's multiple
> > bytes, what's the endianness?
> > 
> > The answer to all these questions is "the format", and that's why
> > there's so many of them.  
> 
> Thanks!
> 
> >> But even with this explanation, I don't understand well what you ask
> >> me to change. Is this better:
> >>
> >> The values are computed by converting RGB values, with each component stored
> >> as u16, to YUV values, with each component stored as u8. The conversion is
> >> done from RGB full range to YUV BT601 full range using the ITU-R BT.601
> >> weights.
> >>
> >> TBH, I do not understand what you are asking for exactly. Can you please
> >> give the sentence you expect directly?  
> > 
> > The fourcc[1] code for the input and output format would be nice. And if
> > you can't, an ad-hoc definition of the format, answering the questions I
> > mentionned earlier (and in the previous mail for YUV).  
> 
> I don't think any fourcc code will apply in this case, the tests use 
> internal VKMS structures pixel_argb_16 and pixel_yuv_u8. How do I 
> describe them better? If I add this comment for the structures, is it 
> enough?
> 
> /**
>   * struct pixel_argb_u16 - Internal representation of a pixel color.
>   * @r: Red component value, stored in 16 bits, without padding, using
>   *     machine endianness
>   * @b: [...]
>   *
>   * The goal of this structure is to keep enough precision to ensure
>   * correct composition results in VKMS and simplifying color
>   * manipulation by splitting each component into its own field.
>   * Caution: the byte ordering of this structure is machine-dependent,
>   * you can't cast it directly to AR48 or xR48.
>   */
> struct pixel_argb_u16 {
> 	u16 a, r, g, b;
> };
> 
> (ditto for pixel_yuv_u8)
> 
> > I'm really
> > surprised about the RGB component values being stored on 16 bits though.
> > It's super unusual, to the point where it's almost useless for us to
> > test, and we should probably use 8 bits values.  
> 
> We need to have 16 bits because some of the writeback formats are 16 bits.

Hi Maxime,

Louis' proposed comment is good and accurate. I can elaborate further on
it.

pixel_argb_u16 is an internal structure used only for temporary pixel
storage: the intermediate format. It's aim is to make computations on
pixel values easy: every input format is converted to it before
computations, and after computations it is converted to each output
format. This allows VKMS to implement computations, e.g. a matrix
operation, in simple code for only one cpu-endian "pixel format", the
intermediate format. (drm_fourcc.h has no cpu-endian formats at all,
and that is good.)

That VKMS never stores complete images in the intermediate format. To
strike a balance between temporary memory requirements and
computational overhead, VKMS processes images line-by-line. Only one
(or two) line's worth of pixels is needed to be kept in memory per
source or destination framebuffer at a time.

16-bit precision is required not just because some writeback and
framebuffer formats are 16-bit. We also need extra precision due to the
color value encoding. Transfer functions can convert pixel data between
the optical and electrical domains. Framebuffers usually contain
electrical domain data, because it takes less bits per pixel in order
to achieve a specific level of visual image quality (think of color
gradient banding). However, some computations, like color space
conversion with a matrix, must be done in the optical domain, which
requires more bits per pixel in order to not degrade the image quality.

In the future I would even expect needing 32-bit or even 64-bit per
channel precision in the intermediate format once higher-than-16 bits
per channel framebuffer formats require testing.

YUV can work with 8 bits per pixel for now, because in practice YUV is
always stored in electrical domain due its definition. YUV in optical
domain is simply never used. However, there are framebuffer formats
with more than 8 bits of YUV channels, so this may need extending too.


Thanks,
pq

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

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

[Maxime Ripard]

[-- Attachment #1: Type: text/plain, Size: 9391 bytes --]

Hi,

On Mon, Mar 10, 2025 at 11:12:59AM +0200, Pekka Paalanen wrote:
> On Fri, 7 Mar 2025 15:50:41 +0100
> Louis Chauvet <louis.chauvet@bootlin.com> wrote:
> 
> > Le 07/03/2025 à 11:20, Maxime Ripard a écrit :
> > > On Wed, Feb 19, 2025 at 02:35:14PM +0100, Louis Chauvet wrote:  
> > >>
> > >>
> > >> Le 19/02/2025 à 11:15, Maxime Ripard a écrit :  
> > >>> On Wed, Feb 05, 2025 at 04:32:07PM +0100, Louis Chauvet wrote:  
> > >>>> On 05/02/25 - 09:55, Maxime Ripard wrote:  
> > >>>>> On Mon, Jan 27, 2025 at 11:48:23AM +0100, Louis Chauvet wrote:  
> > >>>>>> On 26/01/25 - 18:06, Maxime Ripard wrote:  
> > >>>>>>> On Tue, Jan 21, 2025 at 11:48:06AM +0100, Louis Chauvet wrote:  
> > >>>>>>>> +static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
> > >>>>>>>> +	/*
> > >>>>>>>> +	 * colour.RGB_to_YCbCr(<rgb color in 16 bit form>,
> > >>>>>>>> +	 *                     K=colour.WEIGHTS_YCBCR["ITU-R BT.601"],
> > >>>>>>>> +	 *                     in_bits = 16,
> > >>>>>>>> +	 *                     in_legal = False,
> > >>>>>>>> +	 *                     in_int = True,
> > >>>>>>>> +	 *                     out_bits = 8,
> > >>>>>>>> +	 *                     out_legal = False,
> > >>>>>>>> +	 *                     out_int = True)
> > >>>>>>>> +	 *
> > >>>>>>>> +	 * Test cases for conversion between YUV BT601 full range and RGB
> > >>>>>>>> +	 * using the ITU-R BT.601 weights.
> > >>>>>>>> +	 */  
> > >>>>>>>
> > >>>>>>> What are the input and output formats?
> > >>>>>>>
> > >>>>>>> Ditto for all the other tests.  
> > >>>>>>
> > >>>>>> There is no really "input" and "output" format, they are reference values
> > >>>>>> for conversion, you should be able to use it in both direction. They are
> > >>>>>> generated by RGB_to_YCbCr (RGB input, YUV output) just because it was
> > >>>>>> easier to create the colors from RGB values.  
> > >>>>>
> > >>>>> RGB and YUV aren't formats, they are color models. XRGB8888 is a format.
> > >>>>> NV12 is a format.
> > >>>>>  
> > >>>>>> If you think we should specify what is was used as input and output to
> > >>>>>> generate those values, I can modify the comment to:
> > >>>>>>
> > >>>>>> 	Tests cases for color conversion generated by converting RGB
> > >>>>>> 	values to YUV BT601 full range using the ITU-R BT.601 weights.  
> > >>>>>
> > >>>>> My point is that those comments should provide a way to reimplement the
> > >>>>> test from scratch, and compare to the actual implementation. It's useful
> > >>>>> when you have a test failure and start to wonder if the implementation
> > >>>>> or the test is at fault.
> > >>>>>
> > >>>>> By saying only RGB and YUV, you can't possibly do that.  
> > >>>>
> > >>>> I understand your concern, but I believe there might be a slight
> > >>>> misunderstanding. The table in question stores reference values for
> > >>>> specific color models, not formats. Therefore, it doesn't specify any
> > >>>> particular format like XRGB8888 or NV12.
> > >>>>
> > >>>> To clarify this, I can rename the format_pair struct to value_pair. This
> > >>>> should make it clearer that we are dealing with color model values rather
> > >>>> than formats.
> > >>>>
> > >>>> If you want to test a specific format conversion, such as
> > >>>> YUV420_to_argbu16, you would need to follow a process like this:
> > >>>>
> > >>>> 	// Recreate a YUV420 data
> > >>>> 	plane_1[0] = test_case.yuv.y
> > >>>> 	plane_2[0] = test_case.yuv.u
> > >>>> 	plane_2[1] = test_case.yuv.v
> > >>>>
> > >>>> 	// convertion to test from YUV420 format to argb_u16
> > >>>> 	rgb_u16 = convert_YUV420_to_argbu16(plane_1, plane_2)
> > >>>>
> > >>>> 	// ensure the conversion is valid
> > >>>> 	assert_eq(rgb_u16, test_case.rgb)
> > >>>>
> > >>>> The current test is not performing this kind of format conversion.
> > >>>> Instead, it verifies that for given (y, u, v) values, the correct (r, g,
> > >>>> b, a) values are obtained.  
> > >>>
> > >>> You already stated that you check for the A, R, G, and B components. On
> > >>> how many bits are the values you are comparing stored? The YUV values
> > >>> you are comparing are stored on how many bits for each channel? With
> > >>> subsampling?
> > >>>
> > >>> If you want to compare values, you need to encode a given color into
> > >>> bits, and the way that encoding is done is what the format is about.
> > >>>
> > >>> You might not compare the memory layout but each component individually,
> > >>> but it's still a format.  
> > >>
> > >> Sorry, I think I misunderstood what a format really is.  
> > > 
> > > Ultimately, a format is how a given "color value" is stored. How many
> > > bits will you use? If you have an unaligned number of bits, how many
> > > bits of padding you'll use, where the padding is? If there's multiple
> > > bytes, what's the endianness?
> > > 
> > > The answer to all these questions is "the format", and that's why
> > > there's so many of them.  
> > 
> > Thanks!
> > 
> > >> But even with this explanation, I don't understand well what you ask
> > >> me to change. Is this better:
> > >>
> > >> The values are computed by converting RGB values, with each component stored
> > >> as u16, to YUV values, with each component stored as u8. The conversion is
> > >> done from RGB full range to YUV BT601 full range using the ITU-R BT.601
> > >> weights.
> > >>
> > >> TBH, I do not understand what you are asking for exactly. Can you please
> > >> give the sentence you expect directly?  
> > > 
> > > The fourcc[1] code for the input and output format would be nice. And if
> > > you can't, an ad-hoc definition of the format, answering the questions I
> > > mentionned earlier (and in the previous mail for YUV).  
> > 
> > I don't think any fourcc code will apply in this case, the tests use 
> > internal VKMS structures pixel_argb_16 and pixel_yuv_u8. How do I 
> > describe them better? If I add this comment for the structures, is it 
> > enough?
> > 
> > /**
> >   * struct pixel_argb_u16 - Internal representation of a pixel color.
> >   * @r: Red component value, stored in 16 bits, without padding, using
> >   *     machine endianness
> >   * @b: [...]
> >   *
> >   * The goal of this structure is to keep enough precision to ensure
> >   * correct composition results in VKMS and simplifying color
> >   * manipulation by splitting each component into its own field.
> >   * Caution: the byte ordering of this structure is machine-dependent,
> >   * you can't cast it directly to AR48 or xR48.
> >   */
> > struct pixel_argb_u16 {
> > 	u16 a, r, g, b;
> > };
> > 
> > (ditto for pixel_yuv_u8)
> > 
> > > I'm really
> > > surprised about the RGB component values being stored on 16 bits though.
> > > It's super unusual, to the point where it's almost useless for us to
> > > test, and we should probably use 8 bits values.  
> > 
> > We need to have 16 bits because some of the writeback formats are 16 bits.
> 
> Hi Maxime,
> 
> Louis' proposed comment is good and accurate. I can elaborate further on
> it.
> 
> pixel_argb_u16 is an internal structure used only for temporary pixel
> storage: the intermediate format. It's aim is to make computations on
> pixel values easy: every input format is converted to it before
> computations, and after computations it is converted to each output
> format. This allows VKMS to implement computations, e.g. a matrix
> operation, in simple code for only one cpu-endian "pixel format", the
> intermediate format. (drm_fourcc.h has no cpu-endian formats at all,
> and that is good.)
> 
> That VKMS never stores complete images in the intermediate format. To
> strike a balance between temporary memory requirements and
> computational overhead, VKMS processes images line-by-line. Only one
> (or two) line's worth of pixels is needed to be kept in memory per
> source or destination framebuffer at a time.
> 
> 16-bit precision is required not just because some writeback and
> framebuffer formats are 16-bit. We also need extra precision due to the
> color value encoding. Transfer functions can convert pixel data between
> the optical and electrical domains. Framebuffers usually contain
> electrical domain data, because it takes less bits per pixel in order
> to achieve a specific level of visual image quality (think of color
> gradient banding). However, some computations, like color space
> conversion with a matrix, must be done in the optical domain, which
> requires more bits per pixel in order to not degrade the image quality.
> 
> In the future I would even expect needing 32-bit or even 64-bit per
> channel precision in the intermediate format once higher-than-16 bits
> per channel framebuffer formats require testing.
> 
> YUV can work with 8 bits per pixel for now, because in practice YUV is
> always stored in electrical domain due its definition. YUV in optical
> domain is simply never used. However, there are framebuffer formats
> with more than 8 bits of YUV channels, so this may need extending too.

Thanks for your explanations, and yes Louis, I think it's in a much
better shape with your suggestion.

We'd still some additional info like whether you're testing limited vs
full range, but it's most likely going to be on a per-test basis.

Maxime

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]