Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML

[Maíra Canal]

Hi Daniel

Thank you for your feedback! We are working on the comments you pointed out.

On 6/7/22 23:36, Daniel Latypov wrote:
> On Tue, Jun 7, 2022 at 6:09 PM Maíra Canal <maira.canal@xxxxxx> wrote:
>> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/display_mode_lib_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/display_mode_lib_test.c
>> new file mode 100644
>> index 000000000000..3ea0e7fb13e3
>> --- /dev/null
>> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/display_mode_lib_test.c
>> @@ -0,0 +1,83 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * KUnit tests for dml/display_mode_lib.h
>> + *
>> + * Copyright (C) 2022, Maíra Canal <mairacanal@xxxxxxxxxx>
>> + */
>> +
>> +#include <kunit/test.h>
>> +#include "../../dc/dml/display_mode_lib.h"
>> +#include "../../dc/dml/display_mode_enums.h"
>> +#include "dml_test.h"
>> +
>> +/**
>> + * DOC: Unit tests for AMDGPU DML display_mode_lib.h
>> + *
>> + * The display_mode_lib.h holds the functions responsible for the instantiation
>> + * and logging of the Display Mode Library (DML).
>> + *
>> + * These KUnit tests were implemented with the intention of assuring the proper
>> + * logging of the DML.
>> + *
>> + */
>> +
>> +static void dml_get_status_message_test(struct kunit *test)
>> +{
> 
> I think this is a case where an exhaustive test might not be warranted.
> The function under test is entirely just a switch statement with
> return statements, so the chances of things going wrong is minimal.
> But that's just my personal preference.

Maybe we left out some explanation on this unit test. This RFC was more of an introduction to our project. We wanted to show you the test structure we plan to develop the unit tests during this summer. Initially, we were thinking of using the typical kunit_test_suites structure, but we end up checking out that it wasn't possible, due to the need to insert the tests inside the AMDGPU stack.

We also agree with you that this test is trivial and it will probably be removed from the final version. We plan to have more functional tests that explore the inner workings of the DML and especially the corner cases as you said.

We apologize if we didn't make it clear in the Cover Letter that this RFC is more of an introduction to the project we pretend to develop in the summer.

If you have suggestions on how we can improve the use of KUnit, it is welcome.

>>
>> +int display_mode_lib_test_init(void)
>> +{
>> +       pr_info("===> Running display_mode_lib KUnit Tests");
>> +       pr_info("**********************************************************");
>> +       pr_info("**   NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE   **");
>> +       pr_info("**                                                      **");
>> +       pr_info("** display_mode_lib KUnit Tests are being run. This     **");
>> +       pr_info("** means that this is a TEST kernel and should not be   **");
>> +       pr_info("** used for production.                                 **");
>> +       pr_info("**                                                      **");
>> +       pr_info("** If you see this message and you are not debugging    **");
>> +       pr_info("** the kernel, report this immediately to your vendor!  **");
>> +       pr_info("**                                                      **");
>> +       pr_info("**********************************************************");
> 
> David Gow proposed tainting the kernel if we ever try to run a KUnit
> test suite here:
> https://lore.kernel.org/linux-kselftest/20220513083212.3537869-2-davidgow@xxxxxxxxxx/
> 
> If that goes in, then this logging might not be as necessary.
> I'm not sure what the status of that change is, but we're at least
> waiting on a v4, I think.

This is going to be great! We will keep an eye on this proposal.

- Maíra Canal