diff mbox series

[RFC,v3,04/23] drm/vkms: Add kunit tests for VKMS LUT handling

Message ID 20231108163647.106853-5-harry.wentland@amd.com (mailing list archive)
State New, archived
Headers show
Series Color Pipeline API w/ VKMS | expand

Commit Message

Harry Wentland Nov. 8, 2023, 4:36 p.m. UTC 
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

v3:
 - Use include way of testing static functions (Arthur)

Signed-off-by: Harry Wentland <harry.wentland@amd.com>
Cc: Arthur Grillo <arthurgrillo@riseup.net>
---
 drivers/gpu/drm/vkms/Kconfig                  |  5 ++
 drivers/gpu/drm/vkms/tests/.kunitconfig       |  4 ++
 drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++++++++++++++++++
 drivers/gpu/drm/vkms/vkms_composer.c          |  8 ++-
 4 files changed, 77 insertions(+), 2 deletions(-)
 create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
 create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c

Comments

Arthur Grillo Nov. 9, 2023, 10:05 p.m. UTC | #1 
On 08/11/23 13:36, Harry Wentland wrote:
> 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
> 
> v3:
>  - Use include way of testing static functions (Arthur)
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/Kconfig                  |  5 ++
>  drivers/gpu/drm/vkms/tests/.kunitconfig       |  4 ++
>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_composer.c          |  8 ++-
>  4 files changed, 77 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> 
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..c1f8b343ff0e 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -13,3 +13,8 @@ config DRM_VKMS
>  	  a VKMS.
>  
>  	  If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> +	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
> +	depends on DRM_VKMS && KUNIT
> +	default KUNIT_ALL_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/vkms_color_tests.c b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> new file mode 100644
> index 000000000000..b995114cf6b8
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_fixed.h>
> +
> +#define TEST_LUT_SIZE 16
> +
> +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 },
> +};
> +
> +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)
> +{
> +	int i;
> +
> +	KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, test_linear_array[0].red)), 0);
> +
> +	for (i = 0; i < TEST_LUT_SIZE; i++)
> +		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), i);
> +}
> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x80000000), 0x8);
> +}
> +
> +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_LICENSE("GPL");
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 3c99fb8b54e2..6f942896036e 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -91,7 +91,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)
> +u16 lerp_u16(u16 a, u16 b, s64 t)

Now you don't need to remove the static keyword.

>  {
>  	s64 a_fp = drm_int2fixp(a);
>  	s64 b_fp = drm_int2fixp(b);
> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>  	return drm_fixp2int(a_fp + delta);
>  }
>  
> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)

DITTO

Best Regards,
~Arthur Grillo

>  {
>  	s64 color_channel_fp = drm_int2fixp(channel_value);
>  
> @@ -429,3 +429,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>  
>  	return ret;
>  }
> +
> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
> +#include "tests/vkms_color_tests.c"
> +#endif
Arthur Grillo Nov. 10, 2023, 8:24 p.m. UTC | #2 
On 09/11/23 19:05, Arthur Grillo wrote:
> 
> 
> On 08/11/23 13:36, Harry Wentland wrote:
>> 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
>>
>> v3:
>>  - Use include way of testing static functions (Arthur)
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>>  drivers/gpu/drm/vkms/Kconfig                  |  5 ++
>>  drivers/gpu/drm/vkms/tests/.kunitconfig       |  4 ++
>>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++++++++++++++++++

Also, s/tests/test/ to follow the naming of other test files (like on
drivers/gpu/drm/tests/)

Best Regards,
~Arthur Grillo

>>  drivers/gpu/drm/vkms/vkms_composer.c          |  8 ++-
>>  4 files changed, 77 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>>  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>>
>> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
>> index b9ecdebecb0b..c1f8b343ff0e 100644
>> --- a/drivers/gpu/drm/vkms/Kconfig
>> +++ b/drivers/gpu/drm/vkms/Kconfig
>> @@ -13,3 +13,8 @@ config DRM_VKMS
>>  	  a VKMS.
>>  
>>  	  If M is selected the module will be called vkms.
>> +
>> +config DRM_VKMS_KUNIT_TESTS
>> +	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
>> +	depends on DRM_VKMS && KUNIT
>> +	default KUNIT_ALL_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/vkms_color_tests.c b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>> new file mode 100644
>> index 000000000000..b995114cf6b8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +
>> +#include <kunit/test.h>
>> +
>> +#include <drm/drm_fixed.h>
>> +
>> +#define TEST_LUT_SIZE 16
>> +
>> +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 },
>> +};
>> +
>> +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)
>> +{
>> +	int i;
>> +
>> +	KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, test_linear_array[0].red)), 0);
>> +
>> +	for (i = 0; i < TEST_LUT_SIZE; i++)
>> +		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), i);
>> +}
>> +
>> +static void vkms_color_test_lerp(struct kunit *test)
>> +{
>> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x80000000), 0x8);
>> +}
>> +
>> +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_LICENSE("GPL");
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index 3c99fb8b54e2..6f942896036e 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -91,7 +91,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)
>> +u16 lerp_u16(u16 a, u16 b, s64 t)
> 
> Now you don't need to remove the static keyword.
> 
>>  {
>>  	s64 a_fp = drm_int2fixp(a);
>>  	s64 b_fp = drm_int2fixp(b);
>> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>>  	return drm_fixp2int(a_fp + delta);
>>  }
>>  
>> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
>> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> 
> DITTO
> 
> Best Regards,
> ~Arthur Grillo
> 
>>  {
>>  	s64 color_channel_fp = drm_int2fixp(channel_value);
>>  
>> @@ -429,3 +429,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>  
>>  	return ret;
>>  }
>> +
>> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
>> +#include "tests/vkms_color_tests.c"
>> +#endif
Pekka Paalanen Dec. 7, 2023, 2:30 p.m. UTC | #3 
On Wed, 8 Nov 2023 11:36:23 -0500
Harry Wentland <harry.wentland@amd.com> wrote:

> 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
> 
> v3:
>  - Use include way of testing static functions (Arthur)
> 
> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
> Cc: Arthur Grillo <arthurgrillo@riseup.net>
> ---
>  drivers/gpu/drm/vkms/Kconfig                  |  5 ++
>  drivers/gpu/drm/vkms/tests/.kunitconfig       |  4 ++
>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++++++++++++++++++
>  drivers/gpu/drm/vkms/vkms_composer.c          |  8 ++-
>  4 files changed, 77 insertions(+), 2 deletions(-)
>  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> 
> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
> index b9ecdebecb0b..c1f8b343ff0e 100644
> --- a/drivers/gpu/drm/vkms/Kconfig
> +++ b/drivers/gpu/drm/vkms/Kconfig
> @@ -13,3 +13,8 @@ config DRM_VKMS
>  	  a VKMS.
>  
>  	  If M is selected the module will be called vkms.
> +
> +config DRM_VKMS_KUNIT_TESTS
> +	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
> +	depends on DRM_VKMS && KUNIT
> +	default KUNIT_ALL_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/vkms_color_tests.c b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> new file mode 100644
> index 000000000000..b995114cf6b8
> --- /dev/null
> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
> @@ -0,0 +1,62 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +
> +#include <kunit/test.h>
> +
> +#include <drm/drm_fixed.h>
> +
> +#define TEST_LUT_SIZE 16
> +
> +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 },
> +};
> +
> +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)
> +{
> +	int i;
> +
> +	KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, test_linear_array[0].red)), 0);
> +
> +	for (i = 0; i < TEST_LUT_SIZE; i++)
> +		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), i);

Hi,

what about testing with values not directly hitting a LUT element?

> +}
> +
> +static void vkms_color_test_lerp(struct kunit *test)
> +{
> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x80000000), 0x8);

It would raise much more confidence in lerp_u16() if there were more cases:
- odd a
- odd b
- odd b-a
- b = a
- b = a + 1

For each of the above:
- t = 0.0
- t = 1.0
- t = 0.0 + 1
- t = 1.0 - 1
- t chosen so that result must round/truncate
- t chosen to verify the flipping point of result a (or b) to a+1 (or b-1)

I think those are the fragile points in a lerp implementation.


Thanks,
pq

> +}
> +
> +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_LICENSE("GPL");
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
> index 3c99fb8b54e2..6f942896036e 100644
> --- a/drivers/gpu/drm/vkms/vkms_composer.c
> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
> @@ -91,7 +91,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)
> +u16 lerp_u16(u16 a, u16 b, s64 t)
>  {
>  	s64 a_fp = drm_int2fixp(a);
>  	s64 b_fp = drm_int2fixp(b);
> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>  	return drm_fixp2int(a_fp + delta);
>  }
>  
> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
>  {
>  	s64 color_channel_fp = drm_int2fixp(channel_value);
>  
> @@ -429,3 +429,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>  
>  	return ret;
>  }
> +
> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
> +#include "tests/vkms_color_tests.c"
> +#endif
Harry Wentland Dec. 19, 2023, 7:36 p.m. UTC | #4 
On 2023-12-07 09:30, Pekka Paalanen wrote:
> On Wed, 8 Nov 2023 11:36:23 -0500
> Harry Wentland <harry.wentland@amd.com> wrote:
> 
>> 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
>>
>> v3:
>>  - Use include way of testing static functions (Arthur)
>>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Cc: Arthur Grillo <arthurgrillo@riseup.net>
>> ---
>>  drivers/gpu/drm/vkms/Kconfig                  |  5 ++
>>  drivers/gpu/drm/vkms/tests/.kunitconfig       |  4 ++
>>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 62 +++++++++++++++++++
>>  drivers/gpu/drm/vkms/vkms_composer.c          |  8 ++-
>>  4 files changed, 77 insertions(+), 2 deletions(-)
>>  create mode 100644 drivers/gpu/drm/vkms/tests/.kunitconfig
>>  create mode 100644 drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>>
>> diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
>> index b9ecdebecb0b..c1f8b343ff0e 100644
>> --- a/drivers/gpu/drm/vkms/Kconfig
>> +++ b/drivers/gpu/drm/vkms/Kconfig
>> @@ -13,3 +13,8 @@ config DRM_VKMS
>>  	  a VKMS.
>>  
>>  	  If M is selected the module will be called vkms.
>> +
>> +config DRM_VKMS_KUNIT_TESTS
>> +	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
>> +	depends on DRM_VKMS && KUNIT
>> +	default KUNIT_ALL_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/vkms_color_tests.c b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>> new file mode 100644
>> index 000000000000..b995114cf6b8
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>> @@ -0,0 +1,62 @@
>> +/* SPDX-License-Identifier: GPL-2.0+ */
>> +
>> +#include <kunit/test.h>
>> +
>> +#include <drm/drm_fixed.h>
>> +
>> +#define TEST_LUT_SIZE 16
>> +
>> +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 },
>> +};
>> +
>> +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)
>> +{
>> +	int i;
>> +
>> +	KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, test_linear_array[0].red)), 0);
>> +
>> +	for (i = 0; i < TEST_LUT_SIZE; i++)
>> +		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), i);
> 
> Hi,
> 
> what about testing with values not directly hitting a LUT element?
> 
>> +}
>> +
>> +static void vkms_color_test_lerp(struct kunit *test)
>> +{
>> +	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x80000000), 0x8);
> 
> It would raise much more confidence in lerp_u16() if there were more cases:
> - odd a
> - odd b
> - odd b-a
> - b = a
> - b = a + 1
> 
> For each of the above:
> - t = 0.0
> - t = 1.0
> - t = 0.0 + 1
> - t = 1.0 - 1
> - t chosen so that result must round/truncate
> - t chosen to verify the flipping point of result a (or b) to a+1 (or b-1)
> 
> I think those are the fragile points in a lerp implementation.
> 

Good point. Adding the "t = 1.0 - 1" test actually showed a problem
with the lerp_u16 function. I have a fix for it.

Harry

> 
> Thanks,
> pq
> 
>> +}
>> +
>> +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_LICENSE("GPL");
>> \ No newline at end of file
>> diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
>> index 3c99fb8b54e2..6f942896036e 100644
>> --- a/drivers/gpu/drm/vkms/vkms_composer.c
>> +++ b/drivers/gpu/drm/vkms/vkms_composer.c
>> @@ -91,7 +91,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)
>> +u16 lerp_u16(u16 a, u16 b, s64 t)
>>  {
>>  	s64 a_fp = drm_int2fixp(a);
>>  	s64 b_fp = drm_int2fixp(b);
>> @@ -101,7 +101,7 @@ static u16 lerp_u16(u16 a, u16 b, s64 t)
>>  	return drm_fixp2int(a_fp + delta);
>>  }
>>  
>> -static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
>> +s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
>>  {
>>  	s64 color_channel_fp = drm_int2fixp(channel_value);
>>  
>> @@ -429,3 +429,7 @@ int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
>>  
>>  	return ret;
>>  }
>> +
>> +#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
>> +#include "tests/vkms_color_tests.c"
>> +#endif
>
diff mbox series

Patch

diff --git a/drivers/gpu/drm/vkms/Kconfig b/drivers/gpu/drm/vkms/Kconfig
index b9ecdebecb0b..c1f8b343ff0e 100644
--- a/drivers/gpu/drm/vkms/Kconfig
+++ b/drivers/gpu/drm/vkms/Kconfig
@@ -13,3 +13,8 @@  config DRM_VKMS
 	  a VKMS.
 
 	  If M is selected the module will be called vkms.
+
+config DRM_VKMS_KUNIT_TESTS
+	tristate "Tests for VKMS" if !KUNIT_ALL_TESTS
+	depends on DRM_VKMS && KUNIT
+	default KUNIT_ALL_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/vkms_color_tests.c b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
new file mode 100644
index 000000000000..b995114cf6b8
--- /dev/null
+++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
@@ -0,0 +1,62 @@ 
+/* SPDX-License-Identifier: GPL-2.0+ */
+
+#include <kunit/test.h>
+
+#include <drm/drm_fixed.h>
+
+#define TEST_LUT_SIZE 16
+
+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 },
+};
+
+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)
+{
+	int i;
+
+	KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, test_linear_array[0].red)), 0);
+
+	for (i = 0; i < TEST_LUT_SIZE; i++)
+		KUNIT_EXPECT_EQ(test, drm_fixp2int_ceil(get_lut_index(&test_linear_lut, test_linear_array[i].red)), i);
+}
+
+static void vkms_color_test_lerp(struct kunit *test)
+{
+	KUNIT_EXPECT_EQ(test, lerp_u16(0x0, 0x10, 0x80000000), 0x8);
+}
+
+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_LICENSE("GPL");
\ No newline at end of file
diff --git a/drivers/gpu/drm/vkms/vkms_composer.c b/drivers/gpu/drm/vkms/vkms_composer.c
index 3c99fb8b54e2..6f942896036e 100644
--- a/drivers/gpu/drm/vkms/vkms_composer.c
+++ b/drivers/gpu/drm/vkms/vkms_composer.c
@@ -91,7 +91,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)
+u16 lerp_u16(u16 a, u16 b, s64 t)
 {
 	s64 a_fp = drm_int2fixp(a);
 	s64 b_fp = drm_int2fixp(b);
@@ -101,7 +101,7 @@  static u16 lerp_u16(u16 a, u16 b, s64 t)
 	return drm_fixp2int(a_fp + delta);
 }
 
-static s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
+s64 get_lut_index(const struct vkms_color_lut *lut, u16 channel_value)
 {
 	s64 color_channel_fp = drm_int2fixp(channel_value);
 
@@ -429,3 +429,7 @@  int vkms_set_crc_source(struct drm_crtc *crtc, const char *src_name)
 
 	return ret;
 }
+
+#ifdef CONFIG_DRM_VKMS_KUNIT_TESTS
+#include "tests/vkms_color_tests.c"
+#endif