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/[email protected]
To: Rodrigo Siqueira <[email protected]>
To: Melissa Wen <[email protected]>
To: Maíra Canal <[email protected]>
To: Haneen Mohammed <[email protected]>
To: Daniel Vetter <[email protected]>
To: Maarten Lankhorst <[email protected]>
To: Maxime Ripard <[email protected]>
To: Thomas Zimmermann <[email protected]>
To: David Airlie <[email protected]>
To: [email protected]
To: Jonathan Corbet <[email protected]>
To: [email protected]
To: Louis Chauvet <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Cc: [email protected]
Signed-off-by: Arthur Grillo <[email protected]>
---
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 <[email protected]>
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/[email protected]/
Suggested-by: Pekka Paalanen <[email protected]>
Signed-off-by: Arthur Grillo <[email protected]>
---
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
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 <[email protected]>
---
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
Signed-off-by: Arthur Grillo <[email protected]>
---
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
Signed-off-by: Arthur Grillo <[email protected]>
---
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 = ¶m->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
Signed-off-by: Arthur Grillo <[email protected]>
---
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
Now that we have KUnit tests, add instructions on how to run them.
Signed-off-by: Arthur Grillo <[email protected]>
---
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
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 <[email protected]>
---
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
Le 06/03/24 - 17:03, Arthur Grillo a ?crit :
> Signed-off-by: Arthur Grillo <[email protected]>
> ---
> 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 = ¶m->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
On 06/03/24 21:03, Louis Chauvet wrote:
> Le 06/03/24 - 17:03, Arthur Grillo a écrit :
>> Signed-off-by: Arthur Grillo <[email protected]>
>> ---
>> 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 = ¶m->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
>>
>
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/[email protected]
>
> To: Rodrigo Siqueira <[email protected]>
> To: Melissa Wen <[email protected]>
> To: Maíra Canal <[email protected]>
> To: Haneen Mohammed <[email protected]>
> To: Daniel Vetter <[email protected]>
> To: Maarten Lankhorst <[email protected]>
> To: Maxime Ripard <[email protected]>
> To: Thomas Zimmermann <[email protected]>
> To: David Airlie <[email protected]>
> To: [email protected]
> To: Jonathan Corbet <[email protected]>
> To: [email protected]
> To: Louis Chauvet <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected]
>
> Signed-off-by: Arthur Grillo <[email protected]>
> ---
> 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,
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/[email protected]/
>
> Suggested-by: Pekka Paalanen <[email protected]>
> Signed-off-by: Arthur Grillo <[email protected]>
Reviewed-by: Harry Wentland <[email protected]>
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)
>
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/[email protected]
> >
> > To: Rodrigo Siqueira <[email protected]>
> > To: Melissa Wen <[email protected]>
> > To: Ma?ra Canal <[email protected]>
> > To: Haneen Mohammed <[email protected]>
> > To: Daniel Vetter <[email protected]>
> > To: Maarten Lankhorst <[email protected]>
> > To: Maxime Ripard <[email protected]>
> > To: Thomas Zimmermann <[email protected]>
> > To: David Airlie <[email protected]>
> > To: [email protected]
> > To: Jonathan Corbet <[email protected]>
> > To: [email protected]
> > To: Louis Chauvet <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> >
> > Signed-off-by: Arthur Grillo <[email protected]>
> > ---
> > 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
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/[email protected]/
>
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 <[email protected]>
> Signed-off-by: Arthur Grillo <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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 <[email protected]>
> ---
> 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
>
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/[email protected]
> >
> > To: Rodrigo Siqueira <[email protected]>
> > To: Melissa Wen <[email protected]>
> > To: Ma?ra Canal <[email protected]>
> > To: Haneen Mohammed <[email protected]>
> > To: Daniel Vetter <[email protected]>
> > To: Maarten Lankhorst <[email protected]>
> > To: Maxime Ripard <[email protected]>
> > To: Thomas Zimmermann <[email protected]>
> > To: David Airlie <[email protected]>
> > To: [email protected]
> > To: Jonathan Corbet <[email protected]>
> > To: [email protected]
> > To: Louis Chauvet <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected]
> >
> > Signed-off-by: Arthur Grillo <[email protected]>
> > ---
> > 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,
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/[email protected]/
>>
> 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 <[email protected]>
>> Signed-off-by: Arthur Grillo <[email protected]>
>> ---
>> 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
>>
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/[email protected]/
> >>
> > 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 <[email protected]>
> >> Signed-off-by: Arthur Grillo <[email protected]>
> >> ---
> >> 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
> >>
On Thu, 14 Mar 2024 09:59:39 -0300
Melissa Wen <[email protected]> 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/[email protected]/
> > >>
> > > 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 <[email protected]>
> > >> Signed-off-by: Arthur Grillo <[email protected]>
> > >> ---
> > >> 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
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/[email protected]/
> > >>
> > > 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 <[email protected]>
> > >> Signed-off-by: Arthur Grillo <[email protected]>
> > >> ---
> > >> 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/[email protected]/
>
> 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
> > >>
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/[email protected]/
>>>>>
>>>> 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 <[email protected]>
>>>>> Signed-off-by: Arthur Grillo <[email protected]>
>>>>> ---
>>>>> 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/[email protected]/
>>
>> 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
>>>>>
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/[email protected]/
>>
> 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 <[email protected]>
>> Signed-off-by: Arthur Grillo <[email protected]>
>> ---
>> 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
>>
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/[email protected]/
>>>
>> 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 <[email protected]>
>>> Signed-off-by: Arthur Grillo <[email protected]>
>>> ---
>>> 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
>>>