[PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series

[PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series

[Arthur Grillo]

These are some patches that add some fixes/features to the series by
Louis Chauvet[1], it was based on top of the patches from v4.

Patches #2 and #3 should be amended to "[PATCH v4 11/14] drm/vkms: Add
YUV support". To make patch #3 work, we need patch #1. So, please, add
it before the patch that #2 and #3 amend to.

Patches #4 to #6 should be amended to "[PATCH v4 14/14] drm/vkms: Create
KUnit tests for YUV conversions". While doing the additions, I found
some compilation issues, so I fixed them (patch #4). That's when I
thought that it would be good to add some documentation on how to run
them (patch #7), this patch should be added to the series as new patch.

[1]: https://lore.kernel.org/r/20240304-yuv-v4-0-76beac8e9793@bootlin.com

To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
To: Melissa Wen <melissa.srw@gmail.com>
To: Maíra Canal <mairacanal@riseup.net>
To: Haneen Mohammed <hamohammed.sa@gmail.com>
To: Daniel Vetter <daniel@ffwll.ch>
To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
To: Maxime Ripard <mripard@kernel.org>
To: Thomas Zimmermann <tzimmermann@suse.de>
To: David Airlie <airlied@gmail.com>
To: arthurgrillo@riseup.net
To: Jonathan Corbet <corbet@lwn.net>
To: pekka.paalanen@haloniitty.fi
To: Louis Chauvet <louis.chauvet@bootlin.com>
Cc: dri-devel@lists.freedesktop.org
Cc: linux-kernel@vger.kernel.org
Cc: jeremie.dautheribes@bootlin.com
Cc: miquel.raynal@bootlin.com
Cc: thomas.petazzoni@bootlin.com
Cc: seanpaul@google.com
Cc: marcheu@google.com
Cc: nicolejadeyee@google.com

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
Arthur Grillo (7):
      drm: Fix drm_fixp2int_round() making it add 0.5
      drm/vkms: Add comments
      drm/vkmm: Use drm_fixed api
      drm/vkms: Fix compilation issues
      drm/vkms: Add comments to format tests
      drm/vkms: Change the gray RGB representation
      drm/vkms: Add how to run the Kunit tests

 Documentation/gpu/vkms.rst                    |  11 +++
 drivers/gpu/drm/vkms/tests/vkms_format_test.c |  81 +++++++++++++++++++--
 drivers/gpu/drm/vkms/vkms_drv.h               |   4 +
 drivers/gpu/drm/vkms/vkms_formats.c           | 101 +++++++++++++++++++-------
 include/drm/drm_fixed.h                       |   2 +-
 5 files changed, 165 insertions(+), 34 deletions(-)
---
base-commit: 9658aba38ae9f3f3068506c9c8e93e85b500fcb4
change-id: 20240306-louis-vkms-conv-61362ff12ab8

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

[PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Arthur Grillo]

As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
To round a number, you need to add 0.5 to the number and floor that,
drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.

[1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/

Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 include/drm/drm_fixed.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
index 0c9f917a4d4b..de3a79909ac9 100644
--- a/include/drm/drm_fixed.h
+++ b/include/drm/drm_fixed.h
@@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
 
 static inline int drm_fixp2int_round(s64 a)
 {
-	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
+	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
 }
 
 static inline int drm_fixp2int_ceil(s64 a)

-- 
2.43.0

[PATCH 2/7] drm/vkms: Add comments

[Arthur Grillo]

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

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 44d9b9b3bdc3..55ed3f598bd7 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -577,6 +577,18 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.y_offset = 0,
 	};
+
+	/*
+	 * Those matrixies were generated using the colour python framework
+	 *
+	 * Below are the function calls used to generate eac 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 struct conversion_matrix yuv_bt601_full = {
 		.matrix = {
 			{ 4294967296, 0,           6021544149 },
@@ -585,6 +597,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.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 struct conversion_matrix yuv_bt601_limited = {
 		.matrix = {
 			{ 5020601039, 0,           6881764740 },
@@ -593,6 +611,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.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 struct conversion_matrix yuv_bt709_full = {
 		.matrix = {
 			{ 4294967296, 0,          6763714498 },
@@ -601,6 +625,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.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 struct conversion_matrix yuv_bt709_limited = {
 		.matrix = {
 			{ 5020601039, 0,          7729959424 },
@@ -609,6 +639,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.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 struct conversion_matrix yuv_bt2020_full = {
 		.matrix = {
 			{ 4294967296, 0,          6333358775 },
@@ -617,6 +653,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.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 struct conversion_matrix yuv_bt2020_limited = {
 		.matrix = {
 			{ 5020601039, 0,          7238124312 },
@@ -625,6 +667,11 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
 		},
 		.y_offset = 16,
 	};
+
+	/*
+	 * The next matrices are just the previous ones, but with the first and
+	 * second columns swapped
+	 */
 	static struct conversion_matrix yvu_bt601_full = {
 		.matrix = {
 			{ 4294967296, 6021544149,  0 },

-- 
2.43.0

[PATCH 3/7] drm/vkmm: Use drm_fixed api

[Arthur Grillo]

With the new 32.32 values it makes more sense to use drm_fixed functions
than trying to recreate the wheel.

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

diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
index 55ed3f598bd7..adde53cdea26 100644
--- a/drivers/gpu/drm/vkms/vkms_formats.c
+++ b/drivers/gpu/drm/vkms/vkms_formats.c
@@ -191,32 +191,34 @@ VISIBLE_IF_KUNIT struct pixel_argb_u16 argb_u16_from_yuv888(u8 y, u8 cb, u8 cr,
 						  struct conversion_matrix *matrix)
 {
 	u8 r, g, b;
-	s64 y_16, cb_16, cr_16;
-	s64 r_16, g_16, b_16;
-
-	y_16 = y - matrix->y_offset;
-	cb_16 = cb - 128;
-	cr_16 = cr - 128;
-
-	r_16 = matrix->matrix[0][0] * y_16 + matrix->matrix[0][1] * cb_16 +
-	       matrix->matrix[0][2] * cr_16;
-	g_16 = matrix->matrix[1][0] * y_16 + matrix->matrix[1][1] * cb_16 +
-	       matrix->matrix[1][2] * cr_16;
-	b_16 = matrix->matrix[2][0] * y_16 + matrix->matrix[2][1] * cb_16 +
-	       matrix->matrix[2][2] * cr_16;
-
-	// rounding the values
-	r_16 = r_16 + (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH - 4));
-	g_16 = g_16 + (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH - 4));
-	b_16 = b_16 + (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH - 4));
-
-	r_16 = clamp(r_16, 0, (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH + 8)) - 1);
-	g_16 = clamp(g_16, 0, (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH + 8)) - 1);
-	b_16 = clamp(b_16, 0, (1LL << (CONVERSION_MATRIX_FLOAT_DEPTH + 8)) - 1);
-
-	r = r_16 >> CONVERSION_MATRIX_FLOAT_DEPTH;
-	g = g_16 >> CONVERSION_MATRIX_FLOAT_DEPTH;
-	b = b_16 >> CONVERSION_MATRIX_FLOAT_DEPTH;
+	s64 fp_y, fp_cb, fp_cr;
+	s64 fp_r, fp_g, fp_b;
+
+	fp_y = y - matrix->y_offset;
+	fp_cb = cb - 128;
+	fp_cr = cr - 128;
+
+	fp_y = drm_int2fixp(fp_y);
+	fp_cb = drm_int2fixp(fp_cb);
+	fp_cr = drm_int2fixp(fp_cr);
+
+	fp_r = drm_fixp_mul(matrix->matrix[0][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[0][1], fp_cb) +
+	       drm_fixp_mul(matrix->matrix[0][2], fp_cr);
+	fp_g = drm_fixp_mul(matrix->matrix[1][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[1][1], fp_cb) +
+	       drm_fixp_mul(matrix->matrix[1][2], fp_cr);
+	fp_b = drm_fixp_mul(matrix->matrix[2][0], fp_y) +
+	       drm_fixp_mul(matrix->matrix[2][1], fp_cb) +
+	       drm_fixp_mul(matrix->matrix[2][2], fp_cr);
+
+	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, 0xff);
+	g = clamp(fp_g, 0, 0xff);
+	b = clamp(fp_b, 0, 0xff);
 
 	return argb_u16_from_u8888(255, r, g, b);
 }

-- 
2.43.0

[PATCH 4/7] drm/vkms: Fix compilation issues

[Arthur Grillo]

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/tests/vkms_format_test.c | 2 +-
 drivers/gpu/drm/vkms/vkms_drv.h               | 4 ++++
 2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
index 4636b013602f..3522ecee960f 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -113,7 +113,7 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
 	for (size_t i = 0; i < param->n_colors; i++) {
 		const struct format_pair *color = &param->colors[i];
 
-		const struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
+		struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
 			(DRM_FORMAT_NV12, param->encoding, param->range);
 
 		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, matrix);
diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
index 393b76e7c694..3d62578499ab 100644
--- a/drivers/gpu/drm/vkms/vkms_drv.h
+++ b/drivers/gpu/drm/vkms/vkms_drv.h
@@ -47,6 +47,10 @@ struct pixel_argb_u16 {
 	u16 a, r, g, b;
 };
 
+struct pixel_yuv_u8 {
+	u8 y, u, v;
+};
+
 struct line_buffer {
 	size_t n_pixels;
 	struct pixel_argb_u16 *pixels;

-- 
2.43.0

[PATCH 5/7] drm/vkms: Add comments to format tests

[Arthur Grillo]

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/tests/vkms_format_test.c | 67 +++++++++++++++++++++++++++
 1 file changed, 67 insertions(+)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
index 3522ecee960f..66cdd83c3d25 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -24,7 +24,24 @@ struct yuv_u8_to_argb_u16_case {
 	} 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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT601,
 		.range = DRM_COLOR_YCBCR_FULL_RANGE,
@@ -38,6 +55,16 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 			{"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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT601,
 		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
@@ -51,6 +78,16 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 			{"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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT709,
 		.range = DRM_COLOR_YCBCR_FULL_RANGE,
@@ -64,6 +101,16 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 			{"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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT709,
 		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,
@@ -77,6 +124,16 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 			{"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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT2020,
 		.range = DRM_COLOR_YCBCR_FULL_RANGE,
@@ -90,6 +147,16 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 			{"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)
+	 */
 	{
 		.encoding = DRM_COLOR_YCBCR_BT2020,
 		.range = DRM_COLOR_YCBCR_LIMITED_RANGE,

-- 
2.43.0

[PATCH 6/7] drm/vkms: Change the gray RGB representation

[Arthur Grillo]

Using the drm_fixed calls, this needs to be changed. Which in fact is
more correct, colour.YCbCr_to_RGB() gives 0x8080 for same the yuv
parameters.

Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/tests/vkms_format_test.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
index 66cdd83c3d25..49125cf76eb5 100644
--- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
+++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
@@ -48,7 +48,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 6,
 		.colors = {
 			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},
@@ -71,7 +71,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 6,
 		.colors = {
 			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},
@@ -94,7 +94,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 4,
 		.colors = {
 			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},
@@ -117,7 +117,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 4,
 		.colors = {
 			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},
@@ -140,7 +140,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 4,
 		.colors = {
 			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},
@@ -163,7 +163,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
 		.n_colors = 4,
 		.colors = {
 			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
-			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
+			{"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}},

-- 
2.43.0

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

[Arthur Grillo]

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

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

diff --git a/Documentation/gpu/vkms.rst b/Documentation/gpu/vkms.rst
index 13b866c3617c..5ef5ef2e6a21 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.43.0

Re: [PATCH 4/7] drm/vkms: Fix compilation issues

[Louis Chauvet]

Le 06/03/24 - 17:03, Arthur Grillo a écrit :
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 2 +-
>  drivers/gpu/drm/vkms/vkms_drv.h               | 4 ++++
>  2 files changed, 5 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> index 4636b013602f..3522ecee960f 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> @@ -113,7 +113,7 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
>  	for (size_t i = 0; i < param->n_colors; i++) {
>  		const struct format_pair *color = &param->colors[i];
>  
> -		const struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
> +		struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
>  			(DRM_FORMAT_NV12, param->encoding, param->range);
>  
>  		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, matrix);
> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
> index 393b76e7c694..3d62578499ab 100644
> --- a/drivers/gpu/drm/vkms/vkms_drv.h
> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
> @@ -47,6 +47,10 @@ struct pixel_argb_u16 {
>  	u16 a, r, g, b;
>  };
>  
> +struct pixel_yuv_u8 {
> +	u8 y, u, v;
> +};

Can I move this structure in the test itself? As discussed with Pekka, I 
think it's not a good idea to have such specific `pixel_{fmt}_{size}` for 
each variant. Leaving it in vkms_drv.h give the idea of "for each new kind 
of format we have to create a new structure".

> +
>  struct line_buffer {
>  	size_t n_pixels;
>  	struct pixel_argb_u16 *pixels;
> 
> -- 
> 2.43.0
> 

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

Re: [PATCH 4/7] drm/vkms: Fix compilation issues

[Arthur Grillo]

On 06/03/24 21:03, Louis Chauvet wrote:
> Le 06/03/24 - 17:03, Arthur Grillo a écrit :
>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 2 +-
>>  drivers/gpu/drm/vkms/vkms_drv.h               | 4 ++++
>>  2 files changed, 5 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> index 4636b013602f..3522ecee960f 100644
>> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
>> @@ -113,7 +113,7 @@ static void vkms_format_test_yuv_u8_to_argb_u16(struct kunit *test)
>>  	for (size_t i = 0; i < param->n_colors; i++) {
>>  		const struct format_pair *color = &param->colors[i];
>>  
>> -		const struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
>> +		struct conversion_matrix *matrix = get_conversion_matrix_to_argb_u16
>>  			(DRM_FORMAT_NV12, param->encoding, param->range);
>>  
>>  		argb = argb_u16_from_yuv888(color->yuv.y, color->yuv.u, color->yuv.v, matrix);
>> diff --git a/drivers/gpu/drm/vkms/vkms_drv.h b/drivers/gpu/drm/vkms/vkms_drv.h
>> index 393b76e7c694..3d62578499ab 100644
>> --- a/drivers/gpu/drm/vkms/vkms_drv.h
>> +++ b/drivers/gpu/drm/vkms/vkms_drv.h
>> @@ -47,6 +47,10 @@ struct pixel_argb_u16 {
>>  	u16 a, r, g, b;
>>  };
>>  
>> +struct pixel_yuv_u8 {
>> +	u8 y, u, v;
>> +};
> 
> Can I move this structure in the test itself? As discussed with Pekka, I 
> think it's not a good idea to have such specific `pixel_{fmt}_{size}` for 
> each variant. Leaving it in vkms_drv.h give the idea of "for each new kind 
> of format we have to create a new structure".

Sure, it makes more sense.

Best Regards,
~Arthur Grillo

> 
>> +
>>  struct line_buffer {
>>  	size_t n_pixels;
>>  	struct pixel_argb_u16 *pixels;
>>
>> -- 
>> 2.43.0
>>
> 

Re: [PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series

[Maíra Canal]

Hi Arthur,

Would it be possible for you to coordinate with Louis and create a
single series with all the modification?

I don't see a reason to submit fixes to a series that it is still
on review.

Best Regards,
- Maíra

On 3/6/24 17:03, Arthur Grillo wrote:
> These are some patches that add some fixes/features to the series by
> Louis Chauvet[1], it was based on top of the patches from v4.
> 
> Patches #2 and #3 should be amended to "[PATCH v4 11/14] drm/vkms: Add
> YUV support". To make patch #3 work, we need patch #1. So, please, add
> it before the patch that #2 and #3 amend to.
> 
> Patches #4 to #6 should be amended to "[PATCH v4 14/14] drm/vkms: Create
> KUnit tests for YUV conversions". While doing the additions, I found
> some compilation issues, so I fixed them (patch #4). That's when I
> thought that it would be good to add some documentation on how to run
> them (patch #7), this patch should be added to the series as new patch.
> 
> [1]: https://lore.kernel.org/r/20240304-yuv-v4-0-76beac8e9793@bootlin.com
> 
> To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> To: Melissa Wen <melissa.srw@gmail.com>
> To: Maíra Canal <mairacanal@riseup.net>
> To: Haneen Mohammed <hamohammed.sa@gmail.com>
> To: Daniel Vetter <daniel@ffwll.ch>
> To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> To: Maxime Ripard <mripard@kernel.org>
> To: Thomas Zimmermann <tzimmermann@suse.de>
> To: David Airlie <airlied@gmail.com>
> To: arthurgrillo@riseup.net
> To: Jonathan Corbet <corbet@lwn.net>
> To: pekka.paalanen@haloniitty.fi
> To: Louis Chauvet <louis.chauvet@bootlin.com>
> Cc: dri-devel@lists.freedesktop.org
> Cc: linux-kernel@vger.kernel.org
> Cc: jeremie.dautheribes@bootlin.com
> Cc: miquel.raynal@bootlin.com
> Cc: thomas.petazzoni@bootlin.com
> Cc: seanpaul@google.com
> Cc: marcheu@google.com
> Cc: nicolejadeyee@google.com
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
> Arthur Grillo (7):
>        drm: Fix drm_fixp2int_round() making it add 0.5
>        drm/vkms: Add comments
>        drm/vkmm: Use drm_fixed api
>        drm/vkms: Fix compilation issues
>        drm/vkms: Add comments to format tests
>        drm/vkms: Change the gray RGB representation
>        drm/vkms: Add how to run the Kunit tests
> 
>   Documentation/gpu/vkms.rst                    |  11 +++
>   drivers/gpu/drm/vkms/tests/vkms_format_test.c |  81 +++++++++++++++++++--
>   drivers/gpu/drm/vkms/vkms_drv.h               |   4 +
>   drivers/gpu/drm/vkms/vkms_formats.c           | 101 +++++++++++++++++++-------
>   include/drm/drm_fixed.h                       |   2 +-
>   5 files changed, 165 insertions(+), 34 deletions(-)
> ---
> base-commit: 9658aba38ae9f3f3068506c9c8e93e85b500fcb4
> change-id: 20240306-louis-vkms-conv-61362ff12ab8
> 
> Best regards,

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Harry Wentland]

On 2024-03-06 15:03, Arthur Grillo wrote:
> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
> To round a number, you need to add 0.5 to the number and floor that,
> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
> 
> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
> 
> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>

Reviewed-by: Harry Wentland <harry.wentland@amd.com>

I had a different jab at this [1], but your patch is cleaner.

https://patchwork.freedesktop.org/patch/579978/?series=123446&rev=4

Harry

> ---
>   include/drm/drm_fixed.h | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 0c9f917a4d4b..de3a79909ac9 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>   
>   static inline int drm_fixp2int_round(s64 a)
>   {
> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
>   }
>   
>   static inline int drm_fixp2int_ceil(s64 a)
> 

Re: [PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series

[Louis Chauvet]

Le 08/03/24 - 17:38, Maíra Canal a écrit :
> Hi Arthur,

Hi Maíra,

> Would it be possible for you to coordinate with Louis and create a
> single series with all the modification?

This is already the case, [1] contains all our work. But as there were a 
lot of things to change in the YUV part, it was easier for Arthur to send 
a "real" series over [1]. I've already merged everything, and it'll all be 
in v5 (probably Monday or Tuesday).

Kind regards,
Louis Chauvet

> I don't see a reason to submit fixes to a series that it is still
> on review.
>
> Best Regards,
> - Maíra
> 
> On 3/6/24 17:03, Arthur Grillo wrote:
> > These are some patches that add some fixes/features to the series by
> > Louis Chauvet[1], it was based on top of the patches from v4.
> > 
> > Patches #2 and #3 should be amended to "[PATCH v4 11/14] drm/vkms: Add
> > YUV support". To make patch #3 work, we need patch #1. So, please, add
> > it before the patch that #2 and #3 amend to.
> > 
> > Patches #4 to #6 should be amended to "[PATCH v4 14/14] drm/vkms: Create
> > KUnit tests for YUV conversions". While doing the additions, I found
> > some compilation issues, so I fixed them (patch #4). That's when I
> > thought that it would be good to add some documentation on how to run
> > them (patch #7), this patch should be added to the series as new patch.
> > 
> > [1]: https://lore.kernel.org/r/20240304-yuv-v4-0-76beac8e9793@bootlin.com
> > 
> > To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > To: Melissa Wen <melissa.srw@gmail.com>
> > To: Maíra Canal <mairacanal@riseup.net>
> > To: Haneen Mohammed <hamohammed.sa@gmail.com>
> > To: Daniel Vetter <daniel@ffwll.ch>
> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > To: Maxime Ripard <mripard@kernel.org>
> > To: Thomas Zimmermann <tzimmermann@suse.de>
> > To: David Airlie <airlied@gmail.com>
> > To: arthurgrillo@riseup.net
> > To: Jonathan Corbet <corbet@lwn.net>
> > To: pekka.paalanen@haloniitty.fi
> > To: Louis Chauvet <louis.chauvet@bootlin.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: jeremie.dautheribes@bootlin.com
> > Cc: miquel.raynal@bootlin.com
> > Cc: thomas.petazzoni@bootlin.com
> > Cc: seanpaul@google.com
> > Cc: marcheu@google.com
> > Cc: nicolejadeyee@google.com
> > 
> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > ---
> > Arthur Grillo (7):
> >        drm: Fix drm_fixp2int_round() making it add 0.5
> >        drm/vkms: Add comments
> >        drm/vkmm: Use drm_fixed api
> >        drm/vkms: Fix compilation issues
> >        drm/vkms: Add comments to format tests
> >        drm/vkms: Change the gray RGB representation
> >        drm/vkms: Add how to run the Kunit tests
> > 
> >   Documentation/gpu/vkms.rst                    |  11 +++
> >   drivers/gpu/drm/vkms/tests/vkms_format_test.c |  81 +++++++++++++++++++--
> >   drivers/gpu/drm/vkms/vkms_drv.h               |   4 +
> >   drivers/gpu/drm/vkms/vkms_formats.c           | 101 +++++++++++++++++++-------
> >   include/drm/drm_fixed.h                       |   2 +-
> >   5 files changed, 165 insertions(+), 34 deletions(-)
> > ---
> > base-commit: 9658aba38ae9f3f3068506c9c8e93e85b500fcb4
> > change-id: 20240306-louis-vkms-conv-61362ff12ab8
> > 
> > Best regards,

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

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Melissa Wen]

On 03/06, Arthur Grillo wrote:
> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
> To round a number, you need to add 0.5 to the number and floor that,
> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
> 
> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
> 
Hi Arthur,

thanks for addressing this issue.

Please, add a fix tag to the commit that you are fixing, so we can
easily backport. Might be this commit:
https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  include/drm/drm_fixed.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> index 0c9f917a4d4b..de3a79909ac9 100644
> --- a/include/drm/drm_fixed.h
> +++ b/include/drm/drm_fixed.h
> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>  
>  static inline int drm_fixp2int_round(s64 a)
>  {
> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
remove it as it won't be used anymore?

> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
Would this division be equivalent to just shifting 1ULL by 31 instead of
32 as done in DRM_FIXED_ONE?

Melissa

>  }
>  
>  static inline int drm_fixp2int_ceil(s64 a)
> 
> -- 
> 2.43.0
> 

Re: [PATCH 6/7] drm/vkms: Change the gray RGB representation

[Melissa Wen]

On 03/06, Arthur Grillo wrote:
> Using the drm_fixed calls, this needs to be changed. Which in fact is
> more correct, colour.YCbCr_to_RGB() gives 0x8080 for same the yuv
> parameters.

Hi Arthur,

For consistency, shouldn't this change be together with the previous
patch that uses the drm_fixed api? I mean, a single patch that changes
to drm_fixed calls and adjust the color values accordingly, avoiding
room for mismatches?

Melissa
> 
> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/tests/vkms_format_test.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/gpu/drm/vkms/tests/vkms_format_test.c b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> index 66cdd83c3d25..49125cf76eb5 100644
> --- a/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> +++ b/drivers/gpu/drm/vkms/tests/vkms_format_test.c
> @@ -48,7 +48,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 6,
>  		.colors = {
>  			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> @@ -71,7 +71,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 6,
>  		.colors = {
>  			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> @@ -94,7 +94,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 4,
>  		.colors = {
>  			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> @@ -117,7 +117,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 4,
>  		.colors = {
>  			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> @@ -140,7 +140,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 4,
>  		.colors = {
>  			{"white", {0xff, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x80, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> @@ -163,7 +163,7 @@ static struct yuv_u8_to_argb_u16_case yuv_u8_to_argb_u16_cases[] = {
>  		.n_colors = 4,
>  		.colors = {
>  			{"white", {0xeb, 0x80, 0x80}, {0xffff, 0xffff, 0xffff, 0xffff}},
> -			{"gray",  {0x7e, 0x80, 0x80}, {0xffff, 0x8000, 0x8000, 0x8000}},
> +			{"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}},
> 
> -- 
> 2.43.0
> 

Re: [PATCH 2/7] drm/vkms: Add comments

[Melissa Wen]

On 03/06, Arthur Grillo wrote:
Hi,

can you describe better the scope of your changes? Limiting the scope
in the commit message and also describing in the body a summary of what
you are documenting and reasons.

Thanks,

Melissa

> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/vkms_formats.c | 47 +++++++++++++++++++++++++++++++++++++
>  1 file changed, 47 insertions(+)
> 
> diff --git a/drivers/gpu/drm/vkms/vkms_formats.c b/drivers/gpu/drm/vkms/vkms_formats.c
> index 44d9b9b3bdc3..55ed3f598bd7 100644
> --- a/drivers/gpu/drm/vkms/vkms_formats.c
> +++ b/drivers/gpu/drm/vkms/vkms_formats.c
> @@ -577,6 +577,18 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.y_offset = 0,
>  	};
> +
> +	/*
> +	 * Those matrixies were generated using the colour python framework
> +	 *
> +	 * Below are the function calls used to generate eac 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 struct conversion_matrix yuv_bt601_full = {
>  		.matrix = {
>  			{ 4294967296, 0,           6021544149 },
> @@ -585,6 +597,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.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 struct conversion_matrix yuv_bt601_limited = {
>  		.matrix = {
>  			{ 5020601039, 0,           6881764740 },
> @@ -593,6 +611,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.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 struct conversion_matrix yuv_bt709_full = {
>  		.matrix = {
>  			{ 4294967296, 0,          6763714498 },
> @@ -601,6 +625,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.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 struct conversion_matrix yuv_bt709_limited = {
>  		.matrix = {
>  			{ 5020601039, 0,          7729959424 },
> @@ -609,6 +639,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.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 struct conversion_matrix yuv_bt2020_full = {
>  		.matrix = {
>  			{ 4294967296, 0,          6333358775 },
> @@ -617,6 +653,12 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.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 struct conversion_matrix yuv_bt2020_limited = {
>  		.matrix = {
>  			{ 5020601039, 0,          7238124312 },
> @@ -625,6 +667,11 @@ get_conversion_matrix_to_argb_u16(u32 format, enum drm_color_encoding encoding,
>  		},
>  		.y_offset = 16,
>  	};
> +
> +	/*
> +	 * The next matrices are just the previous ones, but with the first and
> +	 * second columns swapped
> +	 */
>  	static struct conversion_matrix yvu_bt601_full = {
>  		.matrix = {
>  			{ 4294967296, 6021544149,  0 },
> 
> -- 
> 2.43.0
> 

Re: [PATCH 0/7] Additions to "Reimplement line-per-line pixel conversion for plane reading" series

[Melissa Wen]

On 03/08, Maíra Canal wrote:
> Hi Arthur,
> 
> Would it be possible for you to coordinate with Louis and create a
> single series with all the modification?
> 
> I don't see a reason to submit fixes to a series that it is still
> on review.

I agree. Moreover, the fix for drm_fixp2int_round() should go in a
single patch detached from these multiple work branches, since it
is addressing an issue already upstream.

Thanks,

Melissa

> 
> Best Regards,
> - Maíra
> 
> On 3/6/24 17:03, Arthur Grillo wrote:
> > These are some patches that add some fixes/features to the series by
> > Louis Chauvet[1], it was based on top of the patches from v4.
> > 
> > Patches #2 and #3 should be amended to "[PATCH v4 11/14] drm/vkms: Add
> > YUV support". To make patch #3 work, we need patch #1. So, please, add
> > it before the patch that #2 and #3 amend to.
> > 
> > Patches #4 to #6 should be amended to "[PATCH v4 14/14] drm/vkms: Create
> > KUnit tests for YUV conversions". While doing the additions, I found
> > some compilation issues, so I fixed them (patch #4). That's when I
> > thought that it would be good to add some documentation on how to run
> > them (patch #7), this patch should be added to the series as new patch.
> > 
> > [1]: https://lore.kernel.org/r/20240304-yuv-v4-0-76beac8e9793@bootlin.com
> > 
> > To: Rodrigo Siqueira <rodrigosiqueiramelo@gmail.com>
> > To: Melissa Wen <melissa.srw@gmail.com>
> > To: Maíra Canal <mairacanal@riseup.net>
> > To: Haneen Mohammed <hamohammed.sa@gmail.com>
> > To: Daniel Vetter <daniel@ffwll.ch>
> > To: Maarten Lankhorst <maarten.lankhorst@linux.intel.com>
> > To: Maxime Ripard <mripard@kernel.org>
> > To: Thomas Zimmermann <tzimmermann@suse.de>
> > To: David Airlie <airlied@gmail.com>
> > To: arthurgrillo@riseup.net
> > To: Jonathan Corbet <corbet@lwn.net>
> > To: pekka.paalanen@haloniitty.fi
> > To: Louis Chauvet <louis.chauvet@bootlin.com>
> > Cc: dri-devel@lists.freedesktop.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: jeremie.dautheribes@bootlin.com
> > Cc: miquel.raynal@bootlin.com
> > Cc: thomas.petazzoni@bootlin.com
> > Cc: seanpaul@google.com
> > Cc: marcheu@google.com
> > Cc: nicolejadeyee@google.com
> > 
> > Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > ---
> > Arthur Grillo (7):
> >        drm: Fix drm_fixp2int_round() making it add 0.5
> >        drm/vkms: Add comments
> >        drm/vkmm: Use drm_fixed api
> >        drm/vkms: Fix compilation issues
> >        drm/vkms: Add comments to format tests
> >        drm/vkms: Change the gray RGB representation
> >        drm/vkms: Add how to run the Kunit tests
> > 
> >   Documentation/gpu/vkms.rst                    |  11 +++
> >   drivers/gpu/drm/vkms/tests/vkms_format_test.c |  81 +++++++++++++++++++--
> >   drivers/gpu/drm/vkms/vkms_drv.h               |   4 +
> >   drivers/gpu/drm/vkms/vkms_formats.c           | 101 +++++++++++++++++++-------
> >   include/drm/drm_fixed.h                       |   2 +-
> >   5 files changed, 165 insertions(+), 34 deletions(-)
> > ---
> > base-commit: 9658aba38ae9f3f3068506c9c8e93e85b500fcb4
> > change-id: 20240306-louis-vkms-conv-61362ff12ab8
> > 
> > Best regards,

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Arthur Grillo]

On 12/03/24 15:27, Melissa Wen wrote:
> On 03/06, Arthur Grillo wrote:
>> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
>> To round a number, you need to add 0.5 to the number and floor that,
>> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
>>
>> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
>>
> Hi Arthur,
> 
> thanks for addressing this issue.
> 
> Please, add a fix tag to the commit that you are fixing, so we can
> easily backport. Might be this commit:
> https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
>> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>>  include/drm/drm_fixed.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
>> index 0c9f917a4d4b..de3a79909ac9 100644
>> --- a/include/drm/drm_fixed.h
>> +++ b/include/drm/drm_fixed.h
>> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>>  
>>  static inline int drm_fixp2int_round(s64 a)
>>  {
>> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
> remove it as it won't be used anymore?
> 
>> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
> Would this division be equivalent to just shifting 1ULL by 31 instead of
> 32 as done in DRM_FIXED_ONE?

Yes, but I think the division makes it easier to understand what is
going on.

Best Regards,
~Arthur Grillo

> 
> Melissa
> 
>>  }
>>  
>>  static inline int drm_fixp2int_ceil(s64 a)
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Melissa Wen]

On 03/13, Arthur Grillo wrote:
> 
> 
> On 12/03/24 15:27, Melissa Wen wrote:
> > On 03/06, Arthur Grillo wrote:
> >> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
> >> To round a number, you need to add 0.5 to the number and floor that,
> >> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
> >>
> >> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
> >>
> > Hi Arthur,
> > 
> > thanks for addressing this issue.
> > 
> > Please, add a fix tag to the commit that you are fixing, so we can
> > easily backport. Might be this commit:
> > https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
> >> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> >> ---
> >>  include/drm/drm_fixed.h | 2 +-
> >>  1 file changed, 1 insertion(+), 1 deletion(-)
> >>
> >> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> >> index 0c9f917a4d4b..de3a79909ac9 100644
> >> --- a/include/drm/drm_fixed.h
> >> +++ b/include/drm/drm_fixed.h
> >> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
> >>  
> >>  static inline int drm_fixp2int_round(s64 a)
> >>  {
> >> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> > Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
> > remove it as it won't be used anymore?
> > 
> >> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
> > Would this division be equivalent to just shifting 1ULL by 31 instead of
> > 32 as done in DRM_FIXED_ONE?
> 
> Yes, but I think the division makes it easier to understand what is
> going on.

Right. I was thinking about slightly better performance, but I don't
have any data. We can go with this since you consider more readable,
anyway.

Can you send another version addressing the other comments? Then I can
cherry-pick and already apply the fix.

Thanks,

Melissa

> 
> Best Regards,
> ~Arthur Grillo
> 
> > 
> > Melissa
> > 
> >>  }
> >>  
> >>  static inline int drm_fixp2int_ceil(s64 a)
> >>
> >> -- 
> >> 2.43.0
> >>

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Pekka Paalanen]

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

On Thu, 14 Mar 2024 09:59:39 -0300
Melissa Wen <mwen@igalia.com> wrote:

> On 03/13, Arthur Grillo wrote:
> > 
> > 
> > On 12/03/24 15:27, Melissa Wen wrote:  
> > > On 03/06, Arthur Grillo wrote:  
> > >> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
> > >> To round a number, you need to add 0.5 to the number and floor that,
> > >> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
> > >>
> > >> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
> > >>  
> > > Hi Arthur,
> > > 
> > > thanks for addressing this issue.
> > > 
> > > Please, add a fix tag to the commit that you are fixing, so we can
> > > easily backport. Might be this commit:
> > > https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665  
> > >> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > >> ---
> > >>  include/drm/drm_fixed.h | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> > >> index 0c9f917a4d4b..de3a79909ac9 100644
> > >> --- a/include/drm/drm_fixed.h
> > >> +++ b/include/drm/drm_fixed.h
> > >> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
> > >>  
> > >>  static inline int drm_fixp2int_round(s64 a)
> > >>  {
> > >> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));  
> > > Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
> > > remove it as it won't be used anymore?
> > >   
> > >> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);  
> > > Would this division be equivalent to just shifting 1ULL by 31 instead of
> > > 32 as done in DRM_FIXED_ONE?  
> > 
> > Yes, but I think the division makes it easier to understand what is
> > going on.  
> 
> Right. I was thinking about slightly better performance, but I don't
> have any data. We can go with this since you consider more readable,
> anyway.

Those are constants, the compiler should compute it at build time in
both styles.

I like the DRM_FIXED_ONE / 2 style, it's semantically obvious. Bit
shift is less obvious.


Thanks,
pq

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

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Melissa Wen]

On 03/14, Melissa Wen wrote:
> On 03/13, Arthur Grillo wrote:
> > 
> > 
> > On 12/03/24 15:27, Melissa Wen wrote:
> > > On 03/06, Arthur Grillo wrote:
> > >> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
> > >> To round a number, you need to add 0.5 to the number and floor that,
> > >> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
> > >>
> > >> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
> > >>
> > > Hi Arthur,
> > > 
> > > thanks for addressing this issue.
> > > 
> > > Please, add a fix tag to the commit that you are fixing, so we can
> > > easily backport. Might be this commit:
> > > https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
> > >> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
> > >> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
> > >> ---
> > >>  include/drm/drm_fixed.h | 2 +-
> > >>  1 file changed, 1 insertion(+), 1 deletion(-)
> > >>
> > >> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
> > >> index 0c9f917a4d4b..de3a79909ac9 100644
> > >> --- a/include/drm/drm_fixed.h
> > >> +++ b/include/drm/drm_fixed.h
> > >> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
> > >>  
> > >>  static inline int drm_fixp2int_round(s64 a)
> > >>  {
> > >> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> > > Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
> > > remove it as it won't be used anymore?
> > > 
> > >> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
> > > Would this division be equivalent to just shifting 1ULL by 31 instead of
> > > 32 as done in DRM_FIXED_ONE?
> > 
> > Yes, but I think the division makes it easier to understand what is
> > going on.
> 
> Right. I was thinking about slightly better performance, but I don't
> have any data. We can go with this since you consider more readable,
> anyway.

Just checked that Harry proposed in another patch[1] this:
`#define DRM_FIXED_HALF		0x80000000ll` for the 0.5 const

Doesn't it sounds better?

[1] https://lore.kernel.org/dri-devel/20240226211100.100108-4-harry.wentland@amd.com/
> 
> Can you send another version addressing the other comments? Then I can
> cherry-pick and already apply the fix.
> 
> Thanks,
> 
> Melissa
> 
> > 
> > Best Regards,
> > ~Arthur Grillo
> > 
> > > 
> > > Melissa
> > > 
> > >>  }
> > >>  
> > >>  static inline int drm_fixp2int_ceil(s64 a)
> > >>
> > >> -- 
> > >> 2.43.0
> > >>

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Harry Wentland]

On 2024-03-14 09:31, Melissa Wen wrote:
> On 03/14, Melissa Wen wrote:
>> On 03/13, Arthur Grillo wrote:
>>>
>>>
>>> On 12/03/24 15:27, Melissa Wen wrote:
>>>> On 03/06, Arthur Grillo wrote:
>>>>> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
>>>>> To round a number, you need to add 0.5 to the number and floor that,
>>>>> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
>>>>>
>>>>> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
>>>>>
>>>> Hi Arthur,
>>>>
>>>> thanks for addressing this issue.
>>>>
>>>> Please, add a fix tag to the commit that you are fixing, so we can
>>>> easily backport. Might be this commit:
>>>> https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
>>>>> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>>>>> ---
>>>>>  include/drm/drm_fixed.h | 2 +-
>>>>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
>>>>> index 0c9f917a4d4b..de3a79909ac9 100644
>>>>> --- a/include/drm/drm_fixed.h
>>>>> +++ b/include/drm/drm_fixed.h
>>>>> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>>>>>  
>>>>>  static inline int drm_fixp2int_round(s64 a)
>>>>>  {
>>>>> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
>>>> Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
>>>> remove it as it won't be used anymore?
>>>>
>>>>> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
>>>> Would this division be equivalent to just shifting 1ULL by 31 instead of
>>>> 32 as done in DRM_FIXED_ONE?
>>>
>>> Yes, but I think the division makes it easier to understand what is
>>> going on.
>>
>> Right. I was thinking about slightly better performance, but I don't
>> have any data. We can go with this since you consider more readable,
>> anyway.
> 
> Just checked that Harry proposed in another patch[1] this:
> `#define DRM_FIXED_HALF		0x80000000ll` for the 0.5 const
> 
> Doesn't it sounds better?
> 

I tend to agree with Arthur and Pekka. DRM_FIXED_ONE / 2 makes it
really clear what's happening here. And I'd imagine compilers would
optimize it out anyways.

Obviously not opposed to a define but if it's only used in one place
I don't think it matters much.

Harry

> [1] https://lore.kernel.org/dri-devel/20240226211100.100108-4-harry.wentland@amd.com/
>>
>> Can you send another version addressing the other comments? Then I can
>> cherry-pick and already apply the fix.
>>
>> Thanks,
>>
>> Melissa
>>
>>>
>>> Best Regards,
>>> ~Arthur Grillo
>>>
>>>>
>>>> Melissa
>>>>
>>>>>  }
>>>>>  
>>>>>  static inline int drm_fixp2int_ceil(s64 a)
>>>>>
>>>>> -- 
>>>>> 2.43.0
>>>>>

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Arthur Grillo]

On 12/03/24 15:27, Melissa Wen wrote:
> On 03/06, Arthur Grillo wrote:
>> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
>> To round a number, you need to add 0.5 to the number and floor that,
>> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
>>
>> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
>>
> Hi Arthur,
> 
> thanks for addressing this issue.
> 
> Please, add a fix tag to the commit that you are fixing, so we can
> easily backport. Might be this commit:
> https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665

Wouldn't be this commit instead?
https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8b25320887d7feac98875546ea0f521628b745bb

Best Regards,
~Arthur Grillo


>> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>>  include/drm/drm_fixed.h | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
>> index 0c9f917a4d4b..de3a79909ac9 100644
>> --- a/include/drm/drm_fixed.h
>> +++ b/include/drm/drm_fixed.h
>> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>>  
>>  static inline int drm_fixp2int_round(s64 a)
>>  {
>> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
> Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
> remove it as it won't be used anymore?
> 
>> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
> Would this division be equivalent to just shifting 1ULL by 31 instead of
> 32 as done in DRM_FIXED_ONE?
> 
> Melissa
> 
>>  }
>>  
>>  static inline int drm_fixp2int_ceil(s64 a)
>>
>> -- 
>> 2.43.0
>>

Re: [PATCH 1/7] drm: Fix drm_fixp2int_round() making it add 0.5

[Melissa Wen]

On 16/03/2024 08:59, Arthur Grillo wrote:
>
> On 12/03/24 15:27, Melissa Wen wrote:
>> On 03/06, Arthur Grillo wrote:
>>> As well noted by Pekka[1], the rounding of drm_fixp2int_round is wrong.
>>> To round a number, you need to add 0.5 to the number and floor that,
>>> drm_fixp2int_round() is adding 0.0000076. Make it add 0.5.
>>>
>>> [1]: https://lore.kernel.org/all/20240301135327.22efe0dd.pekka.paalanen@collabora.com/
>>>
>> Hi Arthur,
>>
>> thanks for addressing this issue.
>>
>> Please, add a fix tag to the commit that you are fixing, so we can
>> easily backport. Might be this commit:
>> https://cgit.freedesktop.org/drm/drm-misc/commit/drivers/gpu/drm/vkms?id=ab87f558dcfb2562c3497e89600dec798a446665
> Wouldn't be this commit instead?
> https://cgit.freedesktop.org/drm/drm-misc/commit/?id=8b25320887d7feac98875546ea0f521628b745bb
Yes, you're right!

Melissa
>
> Best Regards,
> ~Arthur Grillo
>
>
>>> Suggested-by: Pekka Paalanen <pekka.paalanen@collabora.com>
>>> Signed-off-by: Arthur Grillo <arthurgrillo@riseup.net>
>>> ---
>>>   include/drm/drm_fixed.h | 2 +-
>>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>>
>>> diff --git a/include/drm/drm_fixed.h b/include/drm/drm_fixed.h
>>> index 0c9f917a4d4b..de3a79909ac9 100644
>>> --- a/include/drm/drm_fixed.h
>>> +++ b/include/drm/drm_fixed.h
>>> @@ -90,7 +90,7 @@ static inline int drm_fixp2int(s64 a)
>>>   
>>>   static inline int drm_fixp2int_round(s64 a)
>>>   {
>>> -	return drm_fixp2int(a + (1 << (DRM_FIXED_POINT_HALF - 1)));
>> Also, this is the only usage of DRM_FIXED_POINT_HALF. Can you also
>> remove it as it won't be used anymore?
>>
>>> +	return drm_fixp2int(a + DRM_FIXED_ONE / 2);
>> Would this division be equivalent to just shifting 1ULL by 31 instead of
>> 32 as done in DRM_FIXED_ONE?
>>
>> Melissa
>>
>>>   }
>>>   
>>>   static inline int drm_fixp2int_ceil(s64 a)
>>>
>>> -- 
>>> 2.43.0
>>>