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

[Tales Lelo da Aparecida]

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

> 
> --- 8< ---
>  From 84e84664873dc9e98dff5ee9f74d95872e6cd423 Mon Sep 17 00:00:00 2001
> From: David Gow <davidgow at 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 <davidgow at 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