Message ID | 20240226211100.100108-7-harry.wentland@amd.com (mailing list archive) |
---|---|
State | New, archived |
Headers | show |
Series | Color Pipeline API w/ VKMS | expand |
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@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 | 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@riseup.net/ 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
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@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 | 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@riseup.net/ > 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
On Mon, 26 Feb 2024 16:10:20 -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 > > 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@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 | 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 > Hi Harry, since fixed point math is hard, please allow me to nitpick so maybe these patches could be landed ahead of time. > 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 Where does 0xf000f come from? Could it be computed from DRM_FIXED_ONE and ARRAY_LENGTH()? > +}; > + > + > +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); Why this instead of + for (i = 0; i < TEST_LUT_SIZE; i++) + KUNIT_EXPECT_EQ(test, get_lut_index(&test_linear_lut, test_linear_array[i].red), drm_int2fixp(i)); and + for (i = 0; i < 0xffff; i++) + KUNIT_EXPECT_EQ(test, drm_fixp2int(get_lut_index(&test_linear_lut, i)), i / 0x1111); ? I think your original form is leaving quite much room for error in precision. > +} > + > +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); What about b = 0xfffd also? The smaller number in b, and especially in b - a, the more precision error the rounding will forgive. > +} > + > +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) > { > 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); Given the past, I guess it wouldn't hurt if all drm fixp functions had a few tests of their own. Thanks, pq > } > > -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 --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) { 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) { 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
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@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 | 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