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

["Maíra Canal" <mcanal@igalia.com>]

Hi Alex,

On 17/06/25 01:16, Alex Hung wrote:
> 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>
> ---
> 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 | 172 +++++++++++++++++++
>   drivers/gpu/drm/vkms/vkms_composer.c         |   8 +-
>   drivers/gpu/drm/vkms/vkms_composer.h         |  13 ++
>   4 files changed, 192 insertions(+), 3 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 9ded37b67a46..e92f7143e540 100644
> --- a/drivers/gpu/drm/vkms/tests/Makefile
> +++ b/drivers/gpu/drm/vkms/tests/Makefile
> @@ -1,3 +1,3 @@
>   # SPDX-License-Identifier: GPL-2.0-only
>   
> -obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms_config_test.o
> +obj-$(CONFIG_DRM_VKMS_KUNIT_TEST) += vkms_config_test.o vkms_color_test.o

I believe you might need to rebase this patch on top of drm-misc-next
due to 60ba94338047 ("drm/vkms: Compile all tests with
CONFIG_DRM_VKMS_KUNIT_TEST").

> 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..affca56cac7b
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c

[...]

> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> +	/*** half-way round down ***/
> +	s64 t = 0x80000000 - 1;
> +
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x8);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x8);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0x8);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x10);
> +
> +	/*** half-way round up ***/
> +	t = 0x80000000;
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x8);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x9);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0x8);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x11);
> +
> +	/*** t = 0.0 ***/
> +	t = 0x0;
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x0);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x1);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0x1);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x10);
> +
> +	/*** t = 1.0 ***/
> +	t = 0x100000000;
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x10);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x10);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0xf);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x11);
> +
> +	/*** t = 0.0 + 1 ***/
> +	t = 0x0 + 1;
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x0);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x1);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0x1);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x10);
> +
> +	/*** t = 1.0 - 1 ***/
> +	t = 0x100000000 - 1;
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, t), 0x10);
> +
> +	/* odd a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0x10, t), 0x10);
> +
> +	/* odd b */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x1, 0xf, t), 0xf);
> +
> +	/* b = a */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x10, t), 0x10);
> +
> +	/* b = a + 1 */
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x10, 0x11, t), 0x11);
> +
> +	/*** t chosen to verify the flipping point of result a (or b) to a+1 (or b-1) ***/
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x1, 0x80000000 - 1), 0x0);
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x1, 0x80000000), 0x1);
> +}

This test case seems to be a perfect use-case for parametized tests [1].

[1] https://docs.kernel.org/dev-tools/kunit/usage.html#parameterized-testing

> +
> +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..b0dc95f971d8 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -12,6 +12,8 @@
>   #include <linux/minmax.h>
>   
>   #include "vkms_drv.h"
> +#include <kunit/visibility.h>
> +#include "vkms_composer.h"

Nit: IIRC, it's common to sort the includes entries.

Best Regards,
- Maíra