Re: [PATCH V12 02/49] drm/vkms: Add kunit tests for VKMS LUT handling

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 30/10/2025 à 04:42, Alex Hung a écrit :
> From: Harry Wentland <harry.wentland@amd.com>
> 
> Debugging LUT math is much easier when we can unit test
> it. Add kunit functionality to VKMS and add tests for
>   - get_lut_index
>   - lerp_u16
> 
> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
> Signed-off-by: Alex Hung <alex.hung@amd.com>
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Arthur Grillo <arthurgrillo@riseup.net>
> Reviewed-by: Daniel Stone <daniels@collabora.com>
> ---
> v12:
>   - Reorder header files (Louis Chauvet)
>   - Add header file "vkms_drv.h" to vkms_composer.h to avoid compile errors (kernel test robot)
> 
> v11:
>   - Refactor vkms_color_test_lerp() to parametized tests (Maíra Canal)
>   - Sort include file alphabetically (Maíra Canal)
> 
> v8:
>   - Update config names (Louis Chauvet)
> 
> v7:
>   - Fix checkpatch warnings and errors (Louis Chauvet)
>    - Change SPDX-License-Identifier: GPL-2.0+ from /* */ to //
>    - Fix checkpatch errors and warnings (new line at EOF, redundant spaces, and long lines)
>    - Add static to const struct vkms_color_lut test_linear_lut
>   - Add "MODULE_DESCRIPTION" (Jeff Johnson)
> 
> 
> v6:
>   - Eliminate need to include test as .c file (Louis Chauvet)
> 
> v5:
>   - Bring back static for lerp_u16 and get_lut_index (Arthur)
> 
> v4:
>   - Test the critical points of the lerp function (Pekka)
> 
> v3:
>   - Use include way of testing static functions (Arthur)
> 
>   drivers/gpu/drm/vkms/tests/Makefile          |   2 +-
>   drivers/gpu/drm/vkms/tests/vkms_color_test.c | 130 +++++++++++++++++++
>   drivers/gpu/drm/vkms/vkms_composer.c         |   9 +-
>   drivers/gpu/drm/vkms/vkms_composer.h         |  14 ++
>   4 files changed, 151 insertions(+), 4 deletions(-)
>   create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_test.c
>   create mode 100644 drivers/gpu/drm/vkms/vkms_composer.h
> 
> diff --git a/drivers/gpu/drm/vkms/tests/Makefile b/drivers/gpu/drm/vkms/tests/Makefile
> index 5750f0bd9d40..a834d182b022 100644
> --- a/drivers/gpu/drm/vkms/tests/Makefile
> +++ b/drivers/gpu/drm/vkms/tests/Makefile
> @@ -4,4 +4,4 @@ vkms-kunit-tests-y := \
>   	vkms_config_test.o \
>   	vkms_format_test.o
>   
> -obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms-kunit-tests.o
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms-kunit-tests.o vkms_config_test.o vkms_color_test.o

I need this to compile properly the test

diff --git a/drivers/gpu/drm/vkms/tests/Makefile 
b/drivers/gpu/drm/vkms/tests/Makefile
index a834d182b022..d4d9ba8d4c54 100644
--- a/drivers/gpu/drm/vkms/tests/Makefile
+++ b/drivers/gpu/drm/vkms/tests/Makefile
@@ -2,6 +2,7 @@

  vkms-kunit-tests-y := \
         vkms_config_test.o \
-       vkms_format_test.o
+       vkms_format_test.o \
+       vkms_color_test.o

-obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms-kunit-tests.o 
vkms_config_test.o vkms_color_test.o
+obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms-kunit-tests.o


> diff --git a/drivers/gpu/drm/vkms/tests/vkms_color_test.c b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> new file mode 100644
> index 000000000000..582fae1f3220
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
> @@ -0,0 +1,130 @@
> +// SPDX-License-Identifier: GPL-2.0+
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_fixed.h>
> +#include <drm/drm_mode.h>
> +#include "../vkms_composer.h"
> +#include "../vkms_drv.h"
> +
> +#define TEST_LUT_SIZE 16
> +
> +MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
> +
> +static struct drm_color_lut test_linear_array[TEST_LUT_SIZE] = {
> +	{ 0x0, 0x0, 0x0, 0 },
> +	{ 0x1111, 0x1111, 0x1111, 0 },
> +	{ 0x2222, 0x2222, 0x2222, 0 },
> +	{ 0x3333, 0x3333, 0x3333, 0 },
> +	{ 0x4444, 0x4444, 0x4444, 0 },
> +	{ 0x5555, 0x5555, 0x5555, 0 },
> +	{ 0x6666, 0x6666, 0x6666, 0 },
> +	{ 0x7777, 0x7777, 0x7777, 0 },
> +	{ 0x8888, 0x8888, 0x8888, 0 },
> +	{ 0x9999, 0x9999, 0x9999, 0 },
> +	{ 0xaaaa, 0xaaaa, 0xaaaa, 0 },
> +	{ 0xbbbb, 0xbbbb, 0xbbbb, 0 },
> +	{ 0xcccc, 0xcccc, 0xcccc, 0 },
> +	{ 0xdddd, 0xdddd, 0xdddd, 0 },
> +	{ 0xeeee, 0xeeee, 0xeeee, 0 },
> +	{ 0xffff, 0xffff, 0xffff, 0 },
> +};
> +
> +/* lerp test parameters */
> +struct vkms_color_test_lerp_params {
> +	s64 t;
> +	__u16 a;
> +	__u16 b;
> +	__u16 expected;
> +};
> +
> +/* lerp test cases */
> +static const struct vkms_color_test_lerp_params color_test_lerp_cases[] = {
> +	/* Half-way round down */
> +	{ 0x80000000 - 1, 0x0, 0x10, 0x8 },
> +	{ 0x80000000 - 1, 0x1, 0x10, 0x8 },	/* Odd a */
> +	{ 0x80000000 - 1, 0x1, 0xf, 0x8 },	/* Odd b */
> +	{ 0x80000000 - 1, 0x10, 0x10, 0x10 },	/* b = a */
> +	{ 0x80000000 - 1, 0x10, 0x11, 0x10 },	/* b = a + 1*/
> +	/* Half-way round up */
> +	{ 0x80000000, 0x0, 0x10, 0x8 },
> +	{ 0x80000000, 0x1, 0x10, 0x9 },		/* Odd a */
> +	{ 0x80000000, 0x1, 0xf, 0x8 },		/* Odd b */
> +	{ 0x80000000, 0x10, 0x10, 0x10 },	/* b = a */
> +	{ 0x80000000, 0x10, 0x11, 0x11 },	/* b = a + 1*/
> +	/*  t = 0.0 */
> +	{ 0x0, 0x0, 0x10, 0x0 },
> +	{ 0x0, 0x1, 0x10, 0x1 },		/* Odd a */
> +	{ 0x0, 0x1, 0xf, 0x1 },			/* Odd b */
> +	{ 0x0, 0x10, 0x10, 0x10 },		/* b = a */
> +	{ 0x0, 0x10, 0x11, 0x10 },		/* b = a + 1*/
> +	/*  t = 1.0 */
> +	{ 0x100000000, 0x0, 0x10, 0x10 },
> +	{ 0x100000000, 0x1, 0x10, 0x10 },	/* Odd a */
> +	{ 0x100000000, 0x1, 0xf, 0xf },		/* Odd b */
> +	{ 0x100000000, 0x10, 0x10, 0x10 },	/* b = a */
> +	{ 0x100000000, 0x10, 0x11, 0x11 },	/* b = a + 1*/
> +	/*  t = 0.0 + 1 */
> +	{ 0x0 + 1, 0x0, 0x10, 0x0 },
> +	{ 0x0 + 1, 0x1, 0x10, 0x1 },		/* Odd a */
> +	{ 0x0 + 1, 0x1, 0xf, 0x1 },		/* Odd b */
> +	{ 0x0 + 1, 0x10, 0x10, 0x10 },		/* b = a */
> +	{ 0x0 + 1, 0x10, 0x11, 0x10 },		/* b = a + 1*/
> +	/*  t = 1.0 - 1 */
> +	{ 0x100000000 - 1, 0x0, 0x10, 0x10 },
> +	{ 0x100000000 - 1, 0x1, 0x10, 0x10 },	/* Odd a */
> +	{ 0x100000000 - 1, 0x1, 0xf, 0xf },	/* Odd b */
> +	{ 0x100000000 - 1, 0x10, 0x10, 0x10 },	/* b = a */
> +	{ 0x100000000 - 1, 0x10, 0x11, 0x11 },	/* b = a + 1*/
> +	/*  t chosen to verify the flipping point of result a (or b) to a+1 (or b-1) */
> +	{ 0x80000000 - 1, 0x0, 0x1, 0x0 },
> +	{ 0x80000000, 0x0, 0x1, 0x1 },
> +};
> +
> +static const struct vkms_color_lut test_linear_lut = {
> +	.base = test_linear_array,
> +	.lut_length = TEST_LUT_SIZE,
> +	.channel_value2index_ratio = 0xf000fll
> +};
> +
> +
> +static void vkms_color_test_get_lut_index(struct kunit *test)
> +{
> +	s64 lut_index;
> +	int i;
> +
> +	lut_index = get_lut_index(&test_linear_lut, test_linear_array[0].red);
> +	KUNIT_EXPECT_EQ(test, drm_fixp2int(lut_index), 0);
> +
> +	for (i = 0; i < TEST_LUT_SIZE; i++) {
> +		lut_index = get_lut_index(&test_linear_lut, test_linear_array[i].red);
> +		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(lut_index), i);
> +	}
> +}
> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> +	int i;
> +
> +	for (i = 0; i < ARRAY_SIZE(color_test_lerp_cases); i++) {
> +		const struct vkms_color_test_lerp_params *params = &color_test_lerp_cases[i];
> +
> +		KUNIT_EXPECT_EQ(test, lerp_u16(params->a, params->b, params->t), params->expected);
> +	}
> +}
> +
> +static struct kunit_case vkms_color_test_cases[] = {
> +	KUNIT_CASE(vkms_color_test_get_lut_index),
> +	KUNIT_CASE(vkms_color_test_lerp),
> +	{}
> +};
> +
> +static struct kunit_suite vkms_color_test_suite = {
> +	.name = "vkms-color",
> +	.test_cases = vkms_color_test_cases,
> +};
> +
> +kunit_test_suite(vkms_color_test_suite);
> +
> +MODULE_DESCRIPTION("Kunit test for VKMS LUT handling");
> +MODULE_LICENSE("GPL");
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index fa269d279e25..a0ca28a253d2 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -10,8 +10,9 @@
>   #include <drm/drm_gem_framebuffer_helper.h>
>   #include <drm/drm_vblank.h>
>   #include <linux/minmax.h>
> +#include <kunit/visibility.h>
>   
> -#include "vkms_drv.h"
> +#include "vkms_composer.h"
>   
>   static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
>   {
> @@ -60,7 +61,7 @@ static void fill_background(const struct pixel_argb_u16 *background_color,
>   }
>   
>   // lerp(a, b, t) = a + (b - a) * t
> -static u16 lerp_u16(u16 a, u16 b, s64 t)
> +VISIBLE_IF_KUNIT u16 lerp_u16(u16 a, u16 b, s64 t)
>   {
>   	s64 a_fp = drm_int2fixp(a);
>   	s64 b_fp = drm_int2fixp(b);
> @@ -69,13 +70,15 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>   
>   	return drm_fixp2int_round(a_fp + delta);
>   }
> +EXPORT_SYMBOL_IF_KUNIT(lerp_u16);
>   
> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> +VISIBLE_IF_KUNIT s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
>   {
>   	s64 color_channel_fp = drm_int2fixp(channel_value);
>   
>   	return drm_fixp_mul(color_channel_fp, lut->channel_value2index_ratio);
>   }
> +EXPORT_SYMBOL_IF_KUNIT(get_lut_index);
>   
>   /*
>    * This enum is related to the positions of the variables inside
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.h b/drivers/gpu/drm/vkms/vkms_composer.h
> new file mode 100644
> index 000000000000..45f5e6a4986e
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/vkms_composer.h
> @@ -0,0 +1,14 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#ifndef _VKMS_COMPOSER_H_
> +#define _VKMS_COMPOSER_H_
> +
> +#include <kunit/visibility.h>
> +#include "vkms_drv.h"
> +
> +#if IS_ENABLED(CONFIG_KUNIT)
> +u16 lerp_u16(u16 a, u16 b, s64 t);
> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value);
> +#endif
> +
> +#endif /* _VKMS_COMPOSER_H_ */

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