Re: [RFC PATCH v4 06/42] drm/vkms: Add kunit tests for VKMS LUT handling

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 




On 2024-02-27 07:14, Arthur Grillo wrote:
> 
> 
> On 26/02/24 18:10, 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
>>
>> v4:
>>  - Test the critical points of the lerp function (Pekka)
>>
>> v3:
>>  - Use include way of testing static functions (Arthur)
>>
>> Signed-off-by: Harry Wentland <harry.wentland@xxxxxxx>
>> Cc: Arthur Grillo <arthurgrillo@xxxxxxxxxx>
>> ---
>>  drivers/gpu/drm/vkms/Kconfig                  |   5 +
>>  drivers/gpu/drm/vkms/tests/.kunitconfig       |   4 +
>>  drivers/gpu/drm/vkms/tests/vkms_color_tests.c | 163 ++++++++++++++++++
>>  drivers/gpu/drm/vkms/vkms_composer.c          |   8 +-
>>  4 files changed, 178 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..fc73e48aa57c
>> --- /dev/null
>> +++ b/drivers/gpu/drm/vkms/tests/vkms_color_tests.c
>> @@ -0,0 +1,163 @@
>> +/* 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)
>> +{
>> +	/*** 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_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 e70cd473e3be..d178f2a400f6 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)
> 
> You don't need to do that, as already said on [1].
> 
>>  {
>>  	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_round(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)
> 
> Same here.
> 
> [1]: https://lore.kernel.org/dri-devel/4df0cb7c-e738-4927-89ed-51a0e8bcac77@xxxxxxxxxx/
> 

Oops, apologies. Somehow missed this when spinning the set.
static will be back in the next version.

Harry

> 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




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux