On 11/08/2022 04:34, David Gow wrote:
On Thu, Aug 11, 2022 at 8:40 AM Tales Aparecida
<tales.aparec...@gmail.com> wrote:
From: Maíra Canal <maira.ca...@usp.br>
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 <maira.ca...@usp.br>
Co-developed-by: Magali Lemes <magalileme...@gmail.com>
Signed-off-by: Magali Lemes <magalileme...@gmail.com>
Co-developed-by: Tales Aparecida <tales.aparec...@gmail.com>
Signed-off-by: Tales Aparecida <tales.aparec...@gmail.com>
---
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 <david...@google.com>
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 <david...@google.com>
---
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
