Re: [PATCH V10 19/46] drm/tests: Add a few tests around drm_fixed.h

["Wentland, Harry" <Harry.Wentland@amd.com>]

On 2025-06-17 07:04, Maíra Canal wrote:
> Hi Alex,
> 
> On 17/06/25 01:17, Alex Hung wrote:
>> From: Harry Wentland <harry.wentland@amd.com>
>>
>> While working on the CTM implementation of VKMS I had to ascertain
>> myself of a few assumptions. One of those is whether drm_fixed.h
>> treats its numbers using signed-magnitude or twos-complement. It is
>> twos-complement.
>>
>> In order to make someone else's day easier I am adding the
>> drm_test_int2fixp test that validates the above assumption.
>>
>> I am also adding a test for the new sm2fixp function that converts
>> from a signed-magnitude fixed point to the twos-complement fixed
>> point.
>>
>> Reviewed-by: Louis Chauvet <louis.chauvet@bootlin.com>
>> Signed-off-by: Alex Hung <alex.hung@amd.com>
>> Signed-off-by: Harry Wentland <harry.wentland@amd.com>
>> Reviewed-by: Daniel Stone <daniels@collabora.com>
>> ---
>> v7:
>>   - Fix checkpatch warnings (Louis Chauvet)
>>
>> v6:
>>   - add missing MODULE_DESCRIPTION (Jeff Johnson)
>>   - fix buffer overflow
>>
>>
>>   drivers/gpu/drm/tests/Makefile        |  3 +-
>>   drivers/gpu/drm/tests/drm_fixp_test.c | 71 +++++++++++++++++++++++++++
>>   2 files changed, 73 insertions(+), 1 deletion(-)
>>   create mode 100644 drivers/gpu/drm/tests/drm_fixp_test.c
>>
>> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/ 
>> Makefile
>> index 3afd6587df08..91437cf34d92 100644
>> --- a/drivers/gpu/drm/tests/Makefile
>> +++ b/drivers/gpu/drm/tests/Makefile
>> @@ -23,6 +23,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>>       drm_modes_test.o \
>>       drm_plane_helper_test.o \
>>       drm_probe_helper_test.o \
>> -    drm_rect_test.o
>> +    drm_rect_test.o    \
>> +    drm_fixp_test.o
>>   CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
>> diff --git a/drivers/gpu/drm/tests/drm_fixp_test.c b/drivers/gpu/drm/ 
>> tests/drm_fixp_test.c
>> new file mode 100644
>> index 000000000000..de91177af213
>> --- /dev/null
>> +++ b/drivers/gpu/drm/tests/drm_fixp_test.c
>> @@ -0,0 +1,71 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright 2022 Advanced Micro Devices, Inc.
>> + */
>> +
>> +#include <kunit/test.h>
>> +#include <drm/drm_fixed.h>
>> +
>> +static void drm_test_sm2fixp(struct kunit *test)
>> +{
>> +    KUNIT_EXPECT_EQ(test, 0x7fffffffffffffffll, ((1ull << 63) - 1));
>> +
>> +    /* 1 */
>> +    KUNIT_EXPECT_EQ(test, drm_int2fixp(1), drm_sm2fixp(1ull << 
>> DRM_FIXED_POINT));
>> +
>> +    /* -1 */
>> +    KUNIT_EXPECT_EQ(test, drm_int2fixp(-1),
>> +            drm_sm2fixp((1ull << 63) | (1ull << DRM_FIXED_POINT)));
>> +
>> +    /* 0.5 */
>> +    KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(1, 2),
>> +            drm_sm2fixp(1ull << (DRM_FIXED_POINT - 1)));
>> +
>> +    /* -0.5 */
>> +    KUNIT_EXPECT_EQ(test, drm_fixp_from_fraction(-1, 2),
>> +            drm_sm2fixp((1ull << 63) | (1ull << (DRM_FIXED_POINT - 
>> 1))));
>> +}
>> +
>> +static void drm_test_int2fixp(struct kunit *test)
>> +{
>> +    /* 1 */
>> +    KUNIT_EXPECT_EQ(test, 1ll << 32, drm_int2fixp(1));
>> +
>> +    /* -1 */
>> +    KUNIT_EXPECT_EQ(test, -(1ll << 32), drm_int2fixp(-1));
>> +
>> +    /* 1 + (-1) = 0 */
>> +    KUNIT_EXPECT_EQ(test, 0, drm_int2fixp(1) + drm_int2fixp(-1));
>> +
>> +    /* 1 / 2 */
>> +    KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(1, 2));
>> +
>> +    /* -0.5 */
>> +    KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(-1, 2));
>> +
>> +    /* (1 / 2) + (-1) = 0.5 */
>> +    KUNIT_EXPECT_EQ(test, 1ll << 31, drm_fixp_from_fraction(-1, 2) + 
>> drm_int2fixp(1));
>> +
>> +    /* (1 / 2) - 1) = 0.5 */
>> +    KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2) 
>> + drm_int2fixp(-1));
>> +
>> +    /* (1 / 2) - 1) = 0.5 */
>> +    KUNIT_EXPECT_EQ(test, -(1ll << 31), drm_fixp_from_fraction(1, 2) 
>> - drm_int2fixp(1));
>> +}
>> +
>> +static struct kunit_case drm_fixp_tests[] = {
>> +    KUNIT_CASE(drm_test_int2fixp),
>> +    KUNIT_CASE(drm_test_sm2fixp),
>> +    { }
>> +};
>> +
>> +static struct kunit_suite drm_rect_test_suite = {
> 
> Instead of `drm_rect_test_suite`, how about `drm_fixp_test_suite`?
> 

Of course. I forgot to rename it when copy-pasting the
example code.

Thanks.

Harry

>> +    .name = "drm_fixp",
>> +    .test_cases = drm_fixp_tests,
>> +};
>> +
>> +kunit_test_suite(drm_rect_test_suite);
>> +
>> +MODULE_AUTHOR("AMD");
>> +MODULE_LICENSE("GPL and additional rights");
> 
>  From the kernel docs, "GPL and additional rights" shouldn't be used in
> new code [1].
> 
> [1] https://docs.kernel.org/process/license-rules.html#id1
> 
> Best Regards,
> - Maíra
> 
>> +MODULE_DESCRIPTION("Unit tests for drm_fixed.h");
>