[PATCH 1/8] drm/amd/display: Introduce KUnit tests for fixed31_32 library
David Gow
davidgow at google.com
Sat Aug 13 05:56:52 UTC 2022
On Thu, Aug 11, 2022 at 8:40 AM Tales Aparecida
<tales.aparecida at gmail.com> wrote:
>
> The fixed31_32 library performs a lot of the mathematical operations
> involving fixed-point arithmetic and the conversion of integers to
> fixed-point representation.
>
> This unit tests intend to assure the proper functioning of the basic
> mathematical operations of fixed-point arithmetic, such as
> multiplication, conversion from fractional to fixed-point number,
> and more. Use kunit_tool to run:
>
> $ ./tools/testing/kunit/kunit.py run --arch=x86_64 \
> --kunitconfig=drivers/gpu/drm/amd/display/tests/
>
> Signed-off-by: Tales Aparecida <tales.aparecida at gmail.com>
> ---
On the whole, I really like these tests: this sort-of fixed-point
library code is such an excellent example of the sorts of thing KUnit
is really well suited for.
The only thing I'll comment on is that these _could_ be parameterised
tests (given most of them are just testing the same function over and
over with different inputs). That being said, it's a matter of taste
as much as anything.
Otherwise, the remaining comments I'd have apply to most of the
patches in the series, particularly around where they live and how
they're compiled. I think what you have now is okay, but does have
some drawbacks.
In particular, while the #include method is a good
lowest-common-denominator (in that it opens up testing static
functions), it isn't as flexible and broadly compatible with different
testing setups (particularly some with KUNIT=m) as having a separate
test module. I tried making this a separate
It's up to you (and the DRM/AMDGPU maintainers) as to whether having
consistency between tests which _do_ need to test static functions is
worth having those that don't be less accessible.
I've left some comments with more detail below, but regardless, these
tests look fine to me:
Reviewed-by: David Gow <davidgow at google.com>
Cheers,
-- David
> drivers/gpu/drm/amd/display/Kconfig | 13 +
> .../drm/amd/display/dc/basics/fixpt31_32.c | 5 +
> .../gpu/drm/amd/display/tests/.kunitconfig | 6 +
> .../dc/basics/dc_basics_fixpt31_32_test.c | 232 ++++++++++++++++++
> 4 files changed, 256 insertions(+)
> create mode 100644 drivers/gpu/drm/amd/display/tests/.kunitconfig
> create mode 100644 drivers/gpu/drm/amd/display/tests/dc/basics/dc_basics_fixpt31_32_test.c
>
> diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig
> index 96cbc87f7b6b..27981ccb7032 100644
> --- a/drivers/gpu/drm/amd/display/Kconfig
> +++ b/drivers/gpu/drm/amd/display/Kconfig
> @@ -55,4 +55,17 @@ config DRM_AMD_SECURE_DISPLAY
> Cooperate with specific DMCU FW.
>
>
> +config AMD_DC_BASICS_KUNIT_TEST
> + bool "Enable unit tests for the 'utils' sub-component of DAL" if !KUNIT_ALL_TESTS
> + depends on DRM_AMD_DC && KUNIT
If these tests get compiled into the amdgpu kernel module, and
CONFIG_KUNIT=m, this could break loading the driver in some setups.
In particular, see the discussion here:
https://lore.kernel.org/all/20220810234056.2494993-1-npache@redhat.com/T/#u
Given this doesn't affect the "supported" cases, I think you can leave
it as-is, but if you want to handle his "build with KUNIT=m, but
kunit.ko not installed" case, you'll probably want to either split the
tests out into a separate module, or depend on KUNIT=y.
> + default KUNIT_ALL_TESTS
> + help
> + Enables unit tests for the Display Core. Only useful for kernel
> + devs running KUnit.
Given the above, maybe it's worth explicitly noting that these tests
get compiled into the amdgpu module.
> +
> + For more information on KUnit and unit tests in general please refer to
> + the KUnit documentation in Documentation/dev-tools/kunit/.
> +
> + If unsure, say N.
> +
> endmenu
> diff --git a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> index 1726bdf89bae..82747d8ab95a 100644
> --- a/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> +++ b/drivers/gpu/drm/amd/display/dc/basics/fixpt31_32.c
> @@ -490,3 +490,8 @@ int dc_fixpt_s4d19(struct fixed31_32 arg)
> else
> return ux_dy(arg.value, 4, 19);
> }
> +
> +#if IS_ENABLED(CONFIG_AMD_DC_BASICS_KUNIT_TEST)
> +#include "../../tests/dc/basics/dc_basics_fixpt31_32_test.c"
> +#endif
> +
> diff --git a/drivers/gpu/drm/amd/display/tests/.kunitconfig b/drivers/gpu/drm/amd/display/tests/.kunitconfig
> new file mode 100644
> index 000000000000..60f2ff8158f5
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/tests/.kunitconfig
> @@ -0,0 +1,6 @@
> +CONFIG_KUNIT=y
> +CONFIG_PCI=y
> +CONFIG_DRM=y
> +CONFIG_DRM_AMDGPU=y
> +CONFIG_DRM_AMD_DC=y
> +CONFIG_AMD_DC_BASICS_KUNIT_TEST=y
> \ No newline at end of file
> diff --git a/drivers/gpu/drm/amd/display/tests/dc/basics/dc_basics_fixpt31_32_test.c b/drivers/gpu/drm/amd/display/tests/dc/basics/dc_basics_fixpt31_32_test.c
> new file mode 100644
> index 000000000000..2fc489203499
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/tests/dc/basics/dc_basics_fixpt31_32_test.c
> @@ -0,0 +1,232 @@
> +// SPDX-License-Identifier: MIT
> +/* Unit tests for display/include/fixed31_32.h and dc/basics/fixpt31_32.c
> + *
> + * Copyright (C) 2022, Tales Aparecida <tales.aparecida at gmail.com>
> + */
> +
> +#include <kunit/test.h>
> +#include "os_types.h"
> +#include "fixed31_32.h"
> +
> +static const struct fixed31_32 dc_fixpt_minus_one = { -0x100000000LL };
> +
> +/**
> + * dc_fixpt_from_int_test - KUnit test for dc_fixpt_from_int
> + * @test: represents a running instance of a test.
> + */
> +static void dc_fixpt_from_int_test(struct kunit *test)
> +{
> + struct fixed31_32 res;
> +
> + res = dc_fixpt_from_int(0);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_zero.value);
> +
> + res = dc_fixpt_from_int(1);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_int(-1);
> + KUNIT_EXPECT_EQ(test, res.value, -dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_int(INT_MAX);
> + KUNIT_EXPECT_EQ(test, res.value, 0x7FFFFFFF00000000LL);
> +
> + res = dc_fixpt_from_int(INT_MIN);
> + KUNIT_EXPECT_EQ(test, res.value,
> + 0x8000000000000000LL); /* implicit negative signal */
> +}
These could be done as a parameterised test if you prefer, though it's
fine either way.
> +
> +/**
> + * dc_fixpt_from_fraction_test - KUnit test for dc_fixpt_from_fraction
> + * @test: represents a running instance of a test.
> + */
> +static void dc_fixpt_from_fraction_test(struct kunit *test)
> +{
> + struct fixed31_32 res;
> +
> + /* Assert signal works as expected */
> + res = dc_fixpt_from_fraction(1LL, 1LL);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_fraction(-1LL, 1LL);
> + KUNIT_EXPECT_EQ(test, res.value, -dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_fraction(1LL, -1LL);
> + KUNIT_EXPECT_EQ(test, res.value, -dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_fraction(-1LL, -1LL);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + /* Assert that the greatest parameter values works as expected */
> + res = dc_fixpt_from_fraction(LLONG_MAX, LLONG_MAX);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_fraction(LLONG_MIN, LLONG_MIN);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + /* Edge case using the smallest fraction possible without LSB rounding */
> + res = dc_fixpt_from_fraction(1, 1LL << (FIXED31_32_BITS_PER_FRACTIONAL_PART));
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_epsilon.value);
> +
> + /* Edge case using the smallest fraction possible with LSB rounding */
> + res = dc_fixpt_from_fraction(1, 1LL << (FIXED31_32_BITS_PER_FRACTIONAL_PART + 1));
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_epsilon.value);
> +
> + /* Assert an nil numerator is a valid input */
> + res = dc_fixpt_from_fraction(0LL, LLONG_MAX);
> + KUNIT_EXPECT_EQ(test, res.value, 0LL);
> +
> + /* Edge case using every bit of the decimal part without rounding */
> + res = dc_fixpt_from_fraction(8589934590LL, 8589934592LL);
> + KUNIT_EXPECT_EQ(test, res.value, 0x0FFFFFFFFLL);
> +
> + res = dc_fixpt_from_fraction(-8589934590LL, 8589934592LL);
> + KUNIT_EXPECT_EQ(test, res.value, -0x0FFFFFFFFLL);
> +
> + /* Edge case using every bit of the decimal part then rounding LSB */
> + res = dc_fixpt_from_fraction(8589934591LL, 8589934592LL);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + res = dc_fixpt_from_fraction(-8589934591LL, 8589934592LL);
> + KUNIT_EXPECT_EQ(test, res.value, -dc_fixpt_one.value);
> + /* A repeating decimal in binary representation that doesn't round up the LSB */
> + res = dc_fixpt_from_fraction(4, 3);
> + KUNIT_EXPECT_EQ(test, res.value, 0x0000000155555555LL);
> +
> + res = dc_fixpt_from_fraction(-4, 3);
> + KUNIT_EXPECT_EQ(test, res.value, -0x0000000155555555LL);
> +
> + /* A repeating decimal in binary representation that rounds up the LSB */
> + res = dc_fixpt_from_fraction(5, 3);
> + KUNIT_EXPECT_EQ(test, res.value, 0x00000001AAAAAAABLL);
> +
> + res = dc_fixpt_from_fraction(-5, 3);
> + KUNIT_EXPECT_EQ(test, res.value, -0x00000001AAAAAAABLL);
> +}
> +
> +/**
> + * dc_fixpt_mul_test - KUnit test for dc_fixpt_mul
> + * @test: represents a running instance of a test.
> + */
> +static void dc_fixpt_mul_test(struct kunit *test)
> +{
> + struct fixed31_32 res;
> + struct fixed31_32 arg;
> +
> + /* Assert signal works as expected */
> + res = dc_fixpt_mul(dc_fixpt_one, dc_fixpt_one);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + res = dc_fixpt_mul(dc_fixpt_minus_one, dc_fixpt_one);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_minus_one.value);
> +
> + res = dc_fixpt_mul(dc_fixpt_one, dc_fixpt_minus_one);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_minus_one.value);
> +
> + res = dc_fixpt_mul(dc_fixpt_minus_one, dc_fixpt_minus_one);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + /* Assert that the greatest parameter values works as expected */
> + arg.value = LONG_MAX;
> + res = dc_fixpt_mul(arg, dc_fixpt_one);
> + KUNIT_EXPECT_EQ(test, res.value, arg.value);
> +
> + arg.value = LONG_MIN;
> + res = dc_fixpt_mul(arg, dc_fixpt_one);
> + KUNIT_EXPECT_EQ(test, res.value, arg.value);
> +
> + arg.value = LONG_MAX;
> + res = dc_fixpt_mul(dc_fixpt_one, arg);
> + KUNIT_EXPECT_EQ(test, res.value, arg.value);
> +
> + arg.value = LONG_MIN;
> + res = dc_fixpt_mul(dc_fixpt_one, arg);
> + KUNIT_EXPECT_EQ(test, res.value, arg.value);
> +
> + /* Assert it doesn't round LSB as expected */
> + arg.value = 0x7FFFFFFF7fffffffLL;
> + res = dc_fixpt_mul(arg, dc_fixpt_epsilon);
> + KUNIT_EXPECT_EQ(test, res.value, 0x000000007FFFFFFF);
> +
> + /* Assert it rounds LSB as expected */
> + arg.value = 0x7FFFFFFF80000000LL;
> + res = dc_fixpt_mul(arg, dc_fixpt_epsilon);
> + KUNIT_EXPECT_EQ(test, res.value, 0x0000000080000000);
> +}
> +
> +/**
> + * dc_fixpt_sqr_test - KUnit test for dc_fixpt_sqr
> + * @test: represents a running instance of a test.
> + */
> +static void dc_fixpt_sqr_test(struct kunit *test)
> +{
> + struct fixed31_32 res;
> + struct fixed31_32 arg;
> +
> + arg.value = dc_fixpt_one.value;
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + arg.value = dc_fixpt_minus_one.value;
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + arg.value = 0;
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value, 0);
> +
> + /* Test some recognizable values */
> + arg = dc_fixpt_from_int(100);
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_from_int(10000).value);
> +
> + arg = dc_fixpt_from_fraction(1, 100);
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value,
> + dc_fixpt_from_fraction(1, 10000).value);
> +
> + /* LSB rounding */
> + arg = dc_fixpt_from_fraction(3, 2);
> + res = dc_fixpt_sqr(arg);
> + KUNIT_EXPECT_EQ(test, res.value,
> + dc_fixpt_from_fraction(9, 4).value + 1LL);
> +}
> +
> +/**
> + * dc_fixpt_recip_test - KUnit test for dc_fixpt_recip
> + * @test: represents a running instance of a test.
> + */
> +static void dc_fixpt_recip_test(struct kunit *test)
> +{
> + struct fixed31_32 res;
> + struct fixed31_32 arg;
> +
> + /* Assert 1/1 works as expected */
> + res = dc_fixpt_recip(dc_fixpt_one);
> + KUNIT_EXPECT_EQ(test, res.value, dc_fixpt_one.value);
> +
> + /* Assert smallest parameters work as expected. */
> + arg.value = 3LL;
> + res = dc_fixpt_recip(arg);
> + KUNIT_EXPECT_EQ(test, res.value, 0x5555555555555555LL);
> +
> + arg.value = -3LL;
> + res = dc_fixpt_recip(arg);
> + KUNIT_EXPECT_EQ(test, res.value, -0x5555555555555555LL);
> +}
> +
> +static struct kunit_case dc_basics_fixpt31_32_test_cases[] = {
> + KUNIT_CASE(dc_fixpt_from_int_test),
> + KUNIT_CASE(dc_fixpt_from_fraction_test),
> + KUNIT_CASE(dc_fixpt_mul_test),
> + KUNIT_CASE(dc_fixpt_sqr_test),
> + KUNIT_CASE(dc_fixpt_recip_test),
> + {}
> +};
> +
> +static struct kunit_suite dc_basics_fixpt31_32_test_suite = {
> + .name = "dc_basics_fixpt31_32",
> + .test_cases = dc_basics_fixpt31_32_test_cases,
> +};
> +
> +kunit_test_suites(&dc_basics_fixpt31_32_test_suite);
> +
> --
> 2.37.0
>
> --
> You received this message because you are subscribed to the Google Groups "KUnit Development" group.
> To unsubscribe from this group and stop receiving emails from it, send an email to kunit-dev+unsubscribe at googlegroups.com.
> To view this discussion on the web visit https://groups.google.com/d/msgid/kunit-dev/20220811004010.61299-2-tales.aparecida%40gmail.com.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: smime.p7s
Type: application/pkcs7-signature
Size: 4003 bytes
Desc: S/MIME Cryptographic Signature
URL: <https://lists.freedesktop.org/archives/amd-gfx/attachments/20220813/34b45fac/attachment.bin>
More information about the amd-gfx
mailing list