Re: [PATCH v3 1/8] drm/amd/display: Introduce KUnit tests for fixed31_32 library

From: Harry Wentland
Date: Fri Sep 30 2022 - 10:14:26 EST

Next message: Rob Herring: "Re: [PATCH 1/2] dt-bindings: dp-connector: Fix the property name for dp pwr"
Previous message: Serge Semin: "Re: [PATCH v5 2/2] PCI: dwc: Add support for 64-bit MSI target address"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

On 9/12/22 11:59, Maíra Canal wrote:
> From: Tales Aparecida <tales.aparecida@xxxxxxxxx>
>
> 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/
>
> Reviewed-by: David Gow <davidgow@xxxxxxxxxx>
> Signed-off-by: Tales Aparecida <tales.aparecida@xxxxxxxxx>
> Signed-off-by: Maíra Canal <mairacanal@xxxxxxxxxx>
> ---
> drivers/gpu/drm/amd/display/Kconfig | 13 +
> drivers/gpu/drm/amd/display/Makefile | 2 +-
> .../gpu/drm/amd/display/tests/.kunitconfig | 6 +
> drivers/gpu/drm/amd/display/tests/Makefile | 12 +
> .../display/tests/dc/basics/fixpt31_32_test.c | 232 ++++++++++++++++++
> 5 files changed, 264 insertions(+), 1 deletion(-)
> create mode 100644 drivers/gpu/drm/amd/display/tests/.kunitconfig
> create mode 100644 drivers/gpu/drm/amd/display/tests/Makefile
> create mode 100644 drivers/gpu/drm/amd/display/tests/dc/basics/fixpt31_32_test.c
>
> diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig
> index 96cbc87f7b6b..cc44cfe88607 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

I would prefer if we kept the same prefix as for other
configs in the file: "DRM_AMD_DC". Maybe name all the
KUNIT configs here "DRM_AMD_DC_KUNIT_XYZ".

> + bool "Enable KUnit tests for the 'basics' sub-component of DAL" if !KUNIT_ALL_TESTS
> + depends on DRM_AMD_DC && KUNIT
> + default KUNIT_ALL_TESTS
> + help
> + Enables unit tests for the Display Core. Only useful for kernel
> + devs running KUnit.
> +
> + 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/Makefile b/drivers/gpu/drm/amd/display/Makefile
> index 2633de77de5e..0f329aab13f0 100644
> --- a/drivers/gpu/drm/amd/display/Makefile
> +++ b/drivers/gpu/drm/amd/display/Makefile
> @@ -43,7 +43,7 @@ endif
> #TODO: remove when Timing Sync feature is complete
> subdir-ccflags-y += -DBUILD_FEATURE_TIMING_SYNC=0
>
> -DAL_LIBS = amdgpu_dm dc modules/freesync modules/color modules/info_packet modules/power dmub/src
> +DAL_LIBS = amdgpu_dm dc modules/freesync modules/color modules/info_packet modules/power dmub/src tests

Can we put these tests into a 'kunit' directory instead of 'tests'?
We use the codebase on other platforms and 'tests' might be
confusing to some AMD-internal developers, whereas 'kunit' is
explicit and will ensure people understand where these are coming
from and how to use them.

Other than the CONFIG and directory naming these tests look really
nice. Thanks for your contribution.

Harry

>
> ifdef CONFIG_DRM_AMD_DC_HDCP
> DAL_LIBS += modules/hdcp
> 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/Makefile b/drivers/gpu/drm/amd/display/tests/Makefile
> new file mode 100644
> index 000000000000..ef16497318e8
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/tests/Makefile
> @@ -0,0 +1,12 @@
> +# SPDX-License-Identifier: MIT
> +#
> +# Makefile for the KUnit Tests for DC
> +#
> +
> +ifdef CONFIG_AMD_DC_BASICS_KUNIT_TEST
> + DC_TESTS += dc/basics/fixpt31_32_test.o
> +endif
> +
> +AMD_DAL_DC_TESTS = $(addprefix $(AMDDALPATH)/tests/,$(DC_TESTS))
> +
> +AMD_DISPLAY_FILES += $(AMD_DAL_DC_TESTS)
> diff --git a/drivers/gpu/drm/amd/display/tests/dc/basics/fixpt31_32_test.c b/drivers/gpu/drm/amd/display/tests/dc/basics/fixpt31_32_test.c
> new file mode 100644
> index 000000000000..2fc489203499
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/tests/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@xxxxxxxxx>
> + */
> +
> +#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 */
> +}
> +
> +/**
> + * 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);
> +

Next message: Rob Herring: "Re: [PATCH 1/2] dt-bindings: dp-connector: Fix the property name for dp pwr"
Previous message: Serge Semin: "Re: [PATCH v5 2/2] PCI: dwc: Add support for 64-bit MSI target address"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]