On 2025-02-25 04:51, Louis Chauvet wrote:
>
>
> Le 20/12/2024 à 05:33, Alex Hung a écrit :
>> From: Harry Wentland <harry.wentland@xxxxxxx>
>>
>> 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@xxxxxxxxxxx>
>> Signed-off-by: Alex Hung <alex.hung@xxxxxxx>
>> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
>> Cc: Arthur Grillo <arthurgrillo@xxxxxxxxxx>
>
> Hi,
>
> I would like to take this patch too. Can I take it with the modifications below:
>
>> ---
>> 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/Kconfig | 15 ++
>> drivers/gpu/drm/vkms/Makefile | 1 +
>> drivers/gpu/drm/vkms/tests/.kunitconfig | 4 +
>> drivers/gpu/drm/vkms/tests/Makefile | 3 +
>> drivers/gpu/drm/vkms/tests/vkms_color_test.c | 172 +++++++++++++++++++
>> drivers/gpu/drm/vkms/vkms_composer.c | 8 +-
>> drivers/gpu/drm/vkms/vkms_composer.h | 13 ++
>> 7 files changed, 214 insertions(+), 2 deletions(-)
>> create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>> create mode 100644 drivers/gpu/drm/vkms/tests/Makefile
>> 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/Kconfig b/drivers/gpu/drm/vkms/Kconfig
>> index b9ecdebecb0b..59c4a32adb9d 100644
>> --- a/drivers/gpu/drm/vkms/Kconfig
>> +++ b/drivers/gpu/drm/vkms/Kconfig
>> @@ -13,3 +13,18 @@ config DRM_VKMS
>> a VKMS.
>> If M is selected the module will be called vkms.
>> +
>> +config DRM_VKMS_KUNIT_TESTS
>
> Can I change to:
>
> config DRM_VKMS_KUNIT_TEST
>
>> + tristate "KUnit tests for VKMS." if !KUNIT_ALL_TESTS
>
> Can I change to:
>
> tristate "KUnit tests for VKMS" if !KUNIT_ALL_TESTS
>
>> + depends on DRM_VKMS=y && KUNIT
>
> Can I change to:
>
> depends on DRM_VKMS && KUNIT
>
>> + default KUNIT_ALL_TESTS
>> + help
>> + This builds unit tests for VKMS. This option is not useful for
>> + distributions or general kernels, but only for kernel
>> + developers working on VKMS.
>> +
>> + For more information on KUnit and unit tests in general,
>> + please refer to the KUnit documentation in
>> + Documentation/dev-tools/kunit/.
>> +
>> + If in doubt, say "N".
>> diff --git a/drivers/gpu/drm/vkms/Makefile b/drivers/gpu/drm/vkms/Makefile
>> index 1b28a6a32948..8d3e46dde635 100644
>> --- a/drivers/gpu/drm/vkms/Makefile
>> +++ b/drivers/gpu/drm/vkms/Makefile
>> @@ -9,3 +9,4 @@ vkms-y := \
>> vkms_writeback.o
>> obj-$(CONFIG_DRM_VKMS) += vkms.o
>> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += tests/
>> diff --git a/drivers/gpu/drm/vkms/tests/.kunitconfig b/drivers/gpu/drm/vkms/tests/.kunitconfig
>> new file mode 100644
>> index 000000000000..70e378228cbd
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/.kunitconfig
>> @@ -0,0 +1,4 @@
>> +CONFIG_KUNIT=y
>> +CONFIG_DRM=y
>> +CONFIG_DRM_VKMS=y
>> +CONFIG_DRM_VKMS_KUNIT_TESTS=y
>> diff --git a/drivers/gpu/drm/vkms/tests/Makefile b/drivers/gpu/drm/vkms/tests/Makefile
>> new file mode 100644
>> index 000000000000..7876ca7a3c42
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/Makefile
>> @@ -0,0 +1,3 @@
>> +# SPDX-License-Identifier: GPL-2.0-only
>> +
>> +obj-$(CONFIG_DRM_VKMS_KUNIT_TESTS) += vkms_color_test.o
>> \ No newline at end of file
>> 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..b53beaac2703
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_test.c
>> @@ -0,0 +1,172 @@
>> +// SPDX-License-Identifier: GPL-2.0+
>> +
>> +#include <kunit/test.h>
>> +
>> +#include <drm/drm_fixed.h>
>> +#include <drm/drm_mode.h>
>> +#include "../vkms_drv.h"
>> +#include "../vkms_composer.h"
>> +
>> +#define TEST_LUT_SIZE 16
>> +
>> +MODULE_IMPORT_NS(EXPORTED_FOR_KUNIT_TESTING);
>
> Needs to be changed to: (Can I do it?)
>
> MODULE_IMPORT_NS("EXPORTED_FOR_KUNIT_TESTING");
>
Sure, no objections to any of the changes. Feel free to take it
whenever you like.
Harry
> Thanks,
> Louis Chauvet
>
>> +
>> +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 },
>> +};
>> +
>> +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)
>> +{
>> + /*** 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);
>> +}
>> +
>> +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 3d6785d081f2..d15ceac6c56f 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"
>> static u16 pre_mul_blend_channel(u16 src, u16 dst, u16 alpha)
>> {
>> @@ -91,7 +93,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);
>> @@ -100,13 +102,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..9316a053e7d7
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.h
>> @@ -0,0 +1,13 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +
>> +#ifndef _VKMS_COMPOSER_H_
>> +#define _VKMS_COMPOSER_H_
>> +
>> +#include <kunit/visibility.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_ */
>
