Re: [PATCH 2/8] drm/amd/display: Introduce KUnit tests to the bw_fixed library

2022-08-11 Thread Tales Lelo da Aparecida

On 11/08/2022 04:34, David Gow wrote:

On Thu, Aug 11, 2022 at 8:40 AM Tales Aparecida
 wrote:


From: Maíra Canal 

KUnit unifies the test structure and provides helper tools that simplify
the development of tests. Basic use case allows running tests as regular
processes, which makes easier to run unit tests on a development machine
and to integrate the tests in a CI system.

This commit introduces a unit test to the bw_fixed library, which
performs a lot of the mathematical operations involving fixed-point
arithmetic and the conversion of integers to fixed-point representation
inside the Display Mode Library.

As fixed-point representation is the base foundation of the DML calcs
operations, 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.  You can run it with: ./tools/testing/kunit/kunit.py run \
 --arch=x86_64 \
 --kunitconfig=drivers/gpu/drm/amd/display/tests/

Signed-off-by: Maíra Canal 
Co-developed-by: Magali Lemes 
Signed-off-by: Magali Lemes 
Co-developed-by: Tales Aparecida 
Signed-off-by: Tales Aparecida 
---


Not directly related to this patch, but I get a whole stack of
warnings about the definition of MIN_I64 causing integer overflow:
../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/calcs/../../../tests/dc/dml/calcs/bw_fixed_test.c:214:31:
note: in expansion of macro ‘MIN_I64’
  214 | KUNIT_EXPECT_EQ(test, MIN_I64 + 1, res.value);
  |   ^~~
../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/calcs/bw_fixed.c:30:19:
warning: integer overflow in expression ‘-9223372036854775808’ of type
‘long long int’ results in ‘-9223372036854775808’ [-Woverflow]
   30 | (int64_t)(-(1LL << 63))
  |   ^

This seems to fix it (I'll re-send it out as a separate patch so gmail
doesn't mangle it once I'm a bit more convinced it's the best
implementation):


Thanks!

We were aware of this warnings, in fact there was a patch fixing this 
that got dropped in the last minute to expedite its review as a 
standalone patch, I'm sorry I didn't mention it in the cover letter.


To make amends I've sent your approach to the mailing list, hope you 
don't mind!


https://lore.kernel.org/all/20220811204327.411709-1-tales.aparec...@gmail.com/



--- 8< ---
 From 84e84664873dc9e98dff5ee9f74d95872e6cd423 Mon Sep 17 00:00:00 2001
From: David Gow 
Date: Thu, 11 Aug 2022 15:21:02 +0800
Subject: [PATCH] drm/amd/display: MIN_I64 definition causes overflow
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

The definition of MIN_I64 in bw_fixed.c can cause gcc to whinge about
integer overflow, because it is treated as a positive value, which is
then negated. The temporary postive value is not necessarily
representable.

This causes the following warning:
../drivers/gpu/drm/amd/amdgpu/../display/dc/dml/calcs/bw_fixed.c:30:19:
warning: integer overflow in expression ‘-9223372036854775808’ of type
‘long long int’ results in ‘-9223372036854775808’ [-Woverflow]
   30 | (int64_t)(-(1LL << 63))
  |   ^

Writing out (INT_MIN - 1) - 1 works instead.

Signed-off-by: David Gow 
---
drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
index fbe8d0661396..3850f7f0f679 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
@@ -26,12 +26,12 @@
#include "bw_fixed.h"


-#define MIN_I64 \
-   (int64_t)(-(1LL << 63))
-
#define MAX_I64 \
(int64_t)((1ULL << 63) - 1)

+#define MIN_I64 \
+   (-MAX_I64 - 1)
+
#define FRACTIONAL_PART_MASK \
((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)

--
2.37.1.595.g718a3a8f04-goog
--- 8< ---

Otherwise, this test seems to okay. I'll review it (and the series)
more properly over then next few days.

Cheers,
-- David


Thanks for reviewing,
Tales


[PATCH 2/8] drm/amd/display: Introduce KUnit tests to the bw_fixed library

2022-08-10 Thread Tales Aparecida
From: Maíra Canal 

KUnit unifies the test structure and provides helper tools that simplify
the development of tests. Basic use case allows running tests as regular
processes, which makes easier to run unit tests on a development machine
and to integrate the tests in a CI system.

This commit introduces a unit test to the bw_fixed library, which
performs a lot of the mathematical operations involving fixed-point
arithmetic and the conversion of integers to fixed-point representation
inside the Display Mode Library.

As fixed-point representation is the base foundation of the DML calcs
operations, 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.  You can run it with: ./tools/testing/kunit/kunit.py run \
--arch=x86_64 \
--kunitconfig=drivers/gpu/drm/amd/display/tests/

Signed-off-by: Maíra Canal 
Co-developed-by: Magali Lemes 
Signed-off-by: Magali Lemes 
Co-developed-by: Tales Aparecida 
Signed-off-by: Tales Aparecida 
---
 drivers/gpu/drm/amd/display/Kconfig   |  12 +
 .../drm/amd/display/dc/dml/calcs/bw_fixed.c   |   3 +
 .../gpu/drm/amd/display/tests/.kunitconfig|   3 +-
 .../tests/dc/dml/calcs/bw_fixed_test.c| 323 ++
 4 files changed, 340 insertions(+), 1 deletion(-)
 create mode 100644 
drivers/gpu/drm/amd/display/tests/dc/dml/calcs/bw_fixed_test.c

diff --git a/drivers/gpu/drm/amd/display/Kconfig 
b/drivers/gpu/drm/amd/display/Kconfig
index 27981ccb7032..72e7a63da79f 100644
--- a/drivers/gpu/drm/amd/display/Kconfig
+++ b/drivers/gpu/drm/amd/display/Kconfig
@@ -54,6 +54,18 @@ config DRM_AMD_SECURE_DISPLAY
 of crc of specific region via debugfs.
 Cooperate with specific DMCU FW.
 
+config DML_KUNIT_TEST
+   bool "Run all unit tests for DML" if !KUNIT_ALL_TESTS
+   depends on DRM_AMD_DC && KUNIT
+   default KUNIT_ALL_TESTS
+   help
+   Enables unit tests for the Display Mode Library. 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.
 
 config AMD_DC_BASICS_KUNIT_TEST
bool "Enable unit tests for the 'utils' sub-component of DAL" if 
!KUNIT_ALL_TESTS
diff --git a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c 
b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
index 6ca288fb5fb9..fbe8d0661396 100644
--- a/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
+++ b/drivers/gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c
@@ -189,3 +189,6 @@ struct bw_fixed bw_mul(const struct bw_fixed arg1, const 
struct bw_fixed arg2)
return res;
 }
 
+#if IS_ENABLED(CONFIG_DML_KUNIT_TEST)
+#include "../../../tests/dc/dml/calcs/bw_fixed_test.c"
+#endif
diff --git a/drivers/gpu/drm/amd/display/tests/.kunitconfig 
b/drivers/gpu/drm/amd/display/tests/.kunitconfig
index 60f2ff8158f5..dd52cae852d2 100644
--- a/drivers/gpu/drm/amd/display/tests/.kunitconfig
+++ b/drivers/gpu/drm/amd/display/tests/.kunitconfig
@@ -3,4 +3,5 @@ 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
+CONFIG_AMD_DC_BASICS_KUNIT_TEST=y
+CONFIG_DML_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/amd/display/tests/dc/dml/calcs/bw_fixed_test.c 
b/drivers/gpu/drm/amd/display/tests/dc/dml/calcs/bw_fixed_test.c
new file mode 100644
index ..1369da49f444
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/tests/dc/dml/calcs/bw_fixed_test.c
@@ -0,0 +1,323 @@
+// SPDX-License-Identifier: MIT
+/*
+ * KUnit tests for dml/calcs/bw_fixed.h
+ *
+ * Copyright (C) 2022, Magali Lemes 
+ * Copyright (C) 2022, Maíra Canal 
+ * Copyright (C) 2022, Tales Aparecida 
+ */
+
+#include 
+#include 
+#include "bw_fixed.h"
+
+/**
+ * DOC: Unit tests for AMDGPU DML calcs/bw_fixed.h
+ *
+ * bw_fixed.h performs a lot of the mathematical operations involving
+ * fixed-point arithmetic and the conversion of integers to fixed-point
+ * representation.
+ *
+ * As fixed-point representation is the base foundation of the DML calcs
+ * operations, these 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.
+ *
+ */
+
+/**
+ * abs_i64_test - KUnit test for abs_i64
+ * @test: represents a running instance of a test.
+ */
+static void abs_i64_test(struct kunit *test)
+{
+   KUNIT_EXPECT_EQ(test, 0ULL, abs_i64(0LL));
+
+   /* Argument type limits */
+   KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64, abs_i64(MAX_I64));
+   KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64 + 1, abs_i64(MIN_I64));
+}
+
+/**
+ * bw_int_to_fixed_nonconst_test - KUnit test for bw_int_to_fixed_nonconst
+ * @test: