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

[Daniel Latypov <dlatypov@google.com>]

On Tue, Jun 7, 2022 at 6:09 PM Maíra Canal <maira.canal@usp.br> wrote:
>
> KUnit unifies the test structure and provides helper tools that simplify
> the development. Basic use case allows running tests as regular

Thanks for sending this out!
I've added some comments on the KUnit side of things.

Wording nit: was this meant to be "the development of tests." ?

> processes, which makes easier to run unit tests on a development machine
> and to integrate the tests in a CI system.
>
> This commit introduce a basic unit test to one part of the Display Mode

Also very minor typo: "introduces"

> Library: display_mode_lib, in order to introduce the basic structure of the
> tests on the DML.
>
> Signed-off-by: Maíra Canal <maira.canal@usp.br>
> ---
>  drivers/gpu/drm/amd/display/Kconfig           |  1 +
>  .../gpu/drm/amd/display/amdgpu_dm/Makefile    |  5 ++
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c |  3 +
>  .../gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h |  3 +
>  .../drm/amd/display/amdgpu_dm/tests/Kconfig   | 29 +++++++
>  .../drm/amd/display/amdgpu_dm/tests/Makefile  | 14 ++++
>  .../amdgpu_dm/tests/display_mode_lib_test.c   | 83 +++++++++++++++++++
>  .../amd/display/amdgpu_dm/tests/dml_test.c    | 23 +++++
>  .../amd/display/amdgpu_dm/tests/dml_test.h    | 13 +++
>  9 files changed, 174 insertions(+)
>  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
>  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
>  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/display_mode_lib_test.c
>  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
>  create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
>
> diff --git a/drivers/gpu/drm/amd/display/Kconfig b/drivers/gpu/drm/amd/display/Kconfig
> index 127667e549c1..83042e2e4d22 100644
> --- a/drivers/gpu/drm/amd/display/Kconfig
> +++ b/drivers/gpu/drm/amd/display/Kconfig
> @@ -53,5 +53,6 @@ config DRM_AMD_SECURE_DISPLAY
>              of crc of specific region via debugfs.
>              Cooperate with specific DMCU FW.
>
> +source "drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig"
>
>  endmenu
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> index 718e123a3230..d25b63566640 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/Makefile
> @@ -24,6 +24,11 @@
>  # It provides the control and status of dm blocks.
>
>
> +AMDGPU_DM_LIBS = tests
> +
> +AMD_DM = $(addsuffix /Makefile, $(addprefix $(FULL_AMD_DISPLAY_PATH)/amdgpu_dm/,$(AMDGPU_DM_LIBS)))
> +
> +include $(AMD_DM)
>
>  AMDGPUDM = amdgpu_dm.o amdgpu_dm_irq.o amdgpu_dm_mst_types.o amdgpu_dm_color.o
>
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> index cb1e9bb60db2..f73da1e0088f 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.c
> @@ -1655,6 +1655,7 @@ static int amdgpu_dm_init(struct amdgpu_device *adev)
>                 goto error;
>         }
>
> +       amdgpu_dml_test_init();
>
>         DRM_DEBUG_DRIVER("KMS initialized.\n");
>
> @@ -1678,6 +1679,8 @@ static void amdgpu_dm_fini(struct amdgpu_device *adev)
>  {
>         int i;
>
> +       amdgpu_dml_test_exit();
> +
>         if (adev->dm.vblank_control_workqueue) {
>                 destroy_workqueue(adev->dm.vblank_control_workqueue);
>                 adev->dm.vblank_control_workqueue = NULL;
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> index 3cc5c15303e6..e586d3a3d2f0 100644
> --- a/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/amdgpu_dm.h
> @@ -749,4 +749,7 @@ int dm_atomic_get_state(struct drm_atomic_state *state,
>  struct amdgpu_dm_connector *
>  amdgpu_dm_find_first_crtc_matching_connector(struct drm_atomic_state *state,
>                                              struct drm_crtc *crtc);
> +
> +int amdgpu_dml_test_init(void);
> +void amdgpu_dml_test_exit(void);
>  #endif /* __AMDGPU_DM_H__ */
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
> new file mode 100644
> index 000000000000..bd1d971d4452
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
> @@ -0,0 +1,29 @@
> +# SPDX-License-Identifier: MIT
> +menu "DML Unit Tests"
> +       depends on DRM_AMD_DC && KUNIT=m
> +
> +config DISPLAY_MODE_LIB_KUNIT_TEST
> +       bool "Enable unit tests for dml/display_mode_lib" if !DML_KUNIT_TEST
> +       default y if DML_KUNIT_TEST
> +       help
> +               Enables unit tests for the dml/display_mode_lib. 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 DML_KUNIT_TEST
> +       bool "Run all unit tests for DML" if !KUNIT_ALL_TESTS
> +       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.
> +
> +endmenu
> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
> new file mode 100644
> index 000000000000..53b38e340564
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
> @@ -0,0 +1,14 @@
> +# SPDX-License-Identifier: GPL-2.0-only
> +#
> +# Makefile for the KUnit Tests for DML
> +#
> +
> +DML_TESTS = dml_test.o
> +
> +ifdef CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST
> +DML_TESTS += display_mode_lib_test.o
> +endif
> +
> +AMD_DAL_DML_TESTS = $(addprefix $(AMDDALPATH)/amdgpu_dm/tests/,$(DML_TESTS))
> +
> +AMD_DISPLAY_FILES += $(AMD_DAL_DML_TESTS)
> 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@riseup.net>
> + */
> +
> +#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.

> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_VALIDATION_OK), "Validation OK");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_SCALE_RATIO_TAP), "Scale ratio/tap");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_SOURCE_PIXEL_FORMAT), "Source pixel format");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_VIEWPORT_SIZE), "Viewport size");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_TOTAL_V_ACTIVE_BW), "Total vertical active bandwidth");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_DIO_SUPPORT), "DIO support");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_NOT_ENOUGH_DSC), "Not enough DSC Units");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_DSC_CLK_REQUIRED), "DSC clock required");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_URGENT_LATENCY), "Urgent latency");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_REORDERING_BUFFER), "Re-ordering buffer");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_DISPCLK_DPPCLK), "Dispclk and Dppclk");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_TOTAL_AVAILABLE_PIPES), "Total available pipes");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_NUM_OTG), "Number of OTG");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_WRITEBACK_MODE), "Writeback mode");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_WRITEBACK_LATENCY), "Writeback latency");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_WRITEBACK_SCALE_RATIO_TAP), "Writeback scale ratio/tap");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_CURSOR_SUPPORT), "Cursor support");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_PITCH_SUPPORT), "Pitch support");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_PTE_BUFFER_SIZE), "PTE buffer size");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_DSC_INPUT_BPC), "DSC input bpc");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_PREFETCH_SUPPORT), "Prefetch support");
> +       KUNIT_EXPECT_STREQ(test, dml_get_status_message(DML_FAIL_V_RATIO_PREFETCH), "Vertical ratio prefetch");

Hmm, perhaps we could add a test like checking that
dml_get_status_message(-1) gives the expected value ("Unknown
Status")?
Checking values that are too small and too big is generally a nice
thing to check as some of these enum->str funcs are implemented as
array lookups.

> +}
> +
> +static struct kunit_case display_mode_library_test_cases[] = {
> +       KUNIT_CASE(dml_get_status_message_test),
> +       {  }
> +};
> +
> +static struct kunit_suite display_mode_lib_test_suite = {
> +       .name = "dml-display-mode-lib",

Perhaps "display_mode_lib"?

It sounds like DML stands for that, so having both might not be necessary.
Also, it seems like the agreed upon convention is to use "_" instead
of "-" per https://www.kernel.org/doc/html/latest/dev-tools/kunit/style.html#suites.

> +       .test_cases = display_mode_library_test_cases,
> +};
> +
> +static struct kunit_suite *display_mode_lib_test_suites[] = { &display_mode_lib_test_suite, NULL };
> +
> +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@google.com/

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.

> +
> +       return __kunit_test_suites_init(display_mode_lib_test_suites);
> +}
> +
> +void display_mode_lib_test_exit(void)
> +{
> +       return __kunit_test_suites_exit(display_mode_lib_test_suites);
> +}

Other tests have used `return` here, but it's void, so it's not really
necessary.

> diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
> new file mode 100644
> index 000000000000..9a5d47597c10
> --- /dev/null
> +++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
> @@ -0,0 +1,23 @@
> +// SPDX-License-Identifier: GPL-2.0-only
> +
> +#include "dml_test.h"
> +
> +/**
> + * amdgpu_dml_test_init() - Initialise KUnit Tests for DML
> + *
> + * It aggregates all KUnit Tests related to the Display Mode Library (DML).
> + * The DML contains multiple modules, and to assure the modularity of the
> + * tests, the KUnit Tests for a DML module are also gathered in a separated
> + * module. Each KUnit Tests module is initiated in this function.
> + *
> + */
> +int amdgpu_dml_test_init(void)
> +{
> +       display_mode_lib_test_init();
> +       return 0;

Optional: we can make this func void.
It looks like we're never looking at the return value of this func and
we don't need it to make a certain signature (since we're not using it
for module_init).

Daniel