* [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-08 1:07 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: amd-gfx, dri-devel, linux-kernel, kunit-dev, Maíra Canal
This RFC is a preview of the work being developed by Isabella Basso [1],
Maíra Canal [2], and Tales Lelo [3], as part of their Google Summer of Code
projects [4], and Magali Lemes [5], as part of her capstone project.
Our main goal is to bring unit testing to the AMDPGU driver; in particular,
we'll focus on the Display Mode Library (DML) for DCN2.0 and some of the DCE
functions. The modern AMD Linux kernel graphics driver is the single largest
driver in the mainline Linux codebase [6]. As AMD releases new GPU models,
the size of AMDGPU drivers is only becoming even larger.
Assuring the drivers' quality and reliability becomes a complex task without
systematic testing, especially for graphic drivers - which usually involve
tons of complex calculations. Also, keeping bugs away becomes an increasingly
hard task with the introduction of new code. Moreover, developers might want
to refactor old code without fear of the introduction of new issues.
In that sense, it is possible to argue for the benefits of implementing unit
testing at the AMDGPU drivers. This implementation will help developers to
recognize bugs before they are merged into the mainline and also makes it
possible for future code refactors of the AMDGPU driver.
When analyzing the AMDGPU driver, a particular part of the driver highlights
itself as a good candidate for the implementation of unit tests: the Display
Mode Library (DML), as it is focused on mathematical operations.
For the implementation of the tests, we decided to go with the Kernel Unit
Testing Framework (KUnit). KUnit makes it possible to run test suites on
kernel boot or load the tests as a module. It reports all test case results
through a TAP (Test Anything Protocol) in the kernel log.
Moreover, KUnit unifies the test structure and provides tools to simplify the
testing for developers and CI systems.
That said, we developed a little snippet on what we intend to develop in our
summer. We planned the basic structure on how the tests will be introduced
into the codebase and, on the concern of the CI systems, developed a structure
where the unit tests can be introduced as modules and run on IGT (the IGT patch
will be introduced soon).
The way the modules are implemented might seem a little unusual for KUnit
developers. We need to call the KUnit init function inside the AMDGPU stack,
otherwise, the test won't compile as a module. So, the solution to this
problem was based on the unit tests for the Thunderbolt driver, which uses
KUnit and also tests a physical driver.
As kunit_test_suites() defines itself as an init_module(), it conflicts with
the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
be able to compile the tests as modules and, therefore, won't be able to use
IGT to run the tests. This problem with kunit_test_suites() was already
discussed in the KUnit mailing list, as can be seen in [7].
The first patch configures the basic structure of the KUnit Tests, setting the
proper Makefile, Kconfig, and init function. It also contains a simple test
involving DML logging, which is the pretext for building the testing structure.
The second patch adds KUnit tests to bw_fixed functions. This patch represents
what we intend to do on the rest of the DML modules: systematic testing of the
public functions of the DML, especially mathematically complicated functions.
Also, it shows how simple it is to add new tests to the DML with the structure
we built.
Any feedback or ideas for the project are welcome!
[1] https://crosscat.me
[2] https://mairacanal.github.io
[3] https://tales-aparecida.github.io/
[4] https://summerofcode.withgoogle.com/programs/2022/organizations/xorg-foundation
[5] https://magalilemes.github.io/
[6] https://www.phoronix.com/scan.php?page=news_item&px=AMDGPU-Closing-4-Million
[7] https://groups.google.com/g/kunit-dev/c/hbJbh8L37FU/m/EmszZE9qBAAJ
- Isabella Basso, Magali Lemes, Maíra Canal, and Tales Lelo
Magali Lemes (1):
drm/amd/display: Introduce KUnit tests to the bw_fixed library
Maíra Canal (2):
drm/amd/display: Introduce KUnit to DML
drm/amd/display: Move bw_fixed macros to header file
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 | 41 +++
.../drm/amd/display/amdgpu_dm/tests/Makefile | 18 +
.../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
.../amdgpu_dm/tests/display_mode_lib_test.c | 83 +++++
.../amd/display/amdgpu_dm/tests/dml_test.c | 26 ++
.../amd/display/amdgpu_dm/tests/dml_test.h | 21 ++
.../drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-
drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 +
12 files changed, 538 insertions(+), 13 deletions(-)
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/calcs/bw_fixed_test.c
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
--
2.36.1
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 1/3] drm/amd/display: Introduce KUnit to DML
2022-06-08 1:07 ` Maíra Canal
@ 2022-06-08 1:07 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: Maíra Canal, kunit-dev, dri-devel, amd-gfx, linux-kernel
KUnit unifies the test structure and provides helper tools that simplify
the development. 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 introduce a basic unit test to one part of the Display Mode
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)
+{
+ 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");
+}
+
+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",
+ .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("**********************************************************");
+
+ 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);
+}
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;
+}
+
+void amdgpu_dml_test_exit(void)
+{
+ display_mode_lib_test_exit();
+}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
new file mode 100644
index 000000000000..2786db9d0e87
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef DML_TEST_H_
+#define DML_TEST_H_
+
+#if defined (CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST)
+int display_mode_lib_test_init(void);
+void display_mode_lib_test_exit(void);
+#else
+static inline int display_mode_lib_test_init(void) { return 0; }
+static inline void display_mode_lib_test_exit(void) { }
+#endif
+
+#endif
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [RFC 1/3] drm/amd/display: Introduce KUnit to DML
@ 2022-06-08 1:07 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: amd-gfx, dri-devel, linux-kernel, kunit-dev, Maíra Canal
KUnit unifies the test structure and provides helper tools that simplify
the development. 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 introduce a basic unit test to one part of the Display Mode
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)
+{
+ 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");
+}
+
+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",
+ .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("**********************************************************");
+
+ 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);
+}
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;
+}
+
+void amdgpu_dml_test_exit(void)
+{
+ display_mode_lib_test_exit();
+}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
new file mode 100644
index 000000000000..2786db9d0e87
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
@@ -0,0 +1,13 @@
+/* SPDX-License-Identifier: GPL-2.0 */
+#ifndef DML_TEST_H_
+#define DML_TEST_H_
+
+#if defined (CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST)
+int display_mode_lib_test_init(void);
+void display_mode_lib_test_exit(void);
+#else
+static inline int display_mode_lib_test_init(void) { return 0; }
+static inline void display_mode_lib_test_exit(void) { }
+#endif
+
+#endif
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
2022-06-08 1:07 ` Maíra Canal
(?)
@ 2022-06-08 2:36 ` Daniel Latypov
-1 siblings, 0 replies; 32+ messages in thread
From: Daniel Latypov @ 2022-06-08 2:36 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, brendanhiggins, dri-devel, Isabella Basso,
andrealmeid, Jun Lei, magalilemes00, Rodrigo Siqueira,
Javier Martinez Canillas, amd-gfx, Leo Li, mwen, Sean Paul,
David Gow, kunit-dev, Mark Yacoub, linux-kernel,
Dmytro Laktyushkin, Nicholas Choi, tales.aparecida, Alex Deucher,
christian.koenig
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
@ 2022-06-08 2:36 ` Daniel Latypov
0 siblings, 0 replies; 32+ messages in thread
From: Daniel Latypov @ 2022-06-08 2:36 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, brendanhiggins, dri-devel, Isabella Basso,
andrealmeid, Jun Lei, magalilemes00, Rodrigo Siqueira,
Javier Martinez Canillas, amd-gfx, Harry Wentland, Leo Li, mwen,
Sean Paul, David Gow, kunit-dev, Mark Yacoub, linux-kernel,
Dmytro Laktyushkin, Nicholas Choi, Daniel Vetter,
tales.aparecida, Alex Deucher, christian.koenig
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
@ 2022-06-08 2:36 ` Daniel Latypov
0 siblings, 0 replies; 32+ messages in thread
From: Daniel Latypov @ 2022-06-08 2:36 UTC (permalink / raw)
To: Maíra Canal
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, brendanhiggins,
amd-gfx, dri-devel, linux-kernel, kunit-dev
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
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
2022-06-08 2:36 ` Daniel Latypov
(?)
@ 2022-06-15 15:05 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-15 15:05 UTC (permalink / raw)
To: Daniel Latypov
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, brendanhiggins,
amd-gfx, dri-devel, linux-kernel, kunit-dev
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@usp.br> 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@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.
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@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.
This is going to be great! We will keep an eye on this proposal.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
@ 2022-06-15 15:05 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-15 15:05 UTC (permalink / raw)
To: Daniel Latypov
Cc: Harrison Chiu, brendanhiggins, dri-devel, Isabella Basso,
andrealmeid, Jun Lei, magalilemes00, Rodrigo Siqueira,
Javier Martinez Canillas, amd-gfx, Harry Wentland, Leo Li, mwen,
Sean Paul, David Gow, kunit-dev, Mark Yacoub, linux-kernel,
Dmytro Laktyushkin, Nicholas Choi, Daniel Vetter,
tales.aparecida, Alex Deucher, christian.koenig
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@usp.br> 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@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.
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@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.
This is going to be great! We will keep an eye on this proposal.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 1/3] drm/amd/display: Introduce KUnit to DML
@ 2022-06-15 15:05 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-15 15:05 UTC (permalink / raw)
To: Daniel Latypov
Cc: Harrison Chiu, brendanhiggins, dri-devel, Isabella Basso,
andrealmeid, Jun Lei, magalilemes00, Rodrigo Siqueira,
Javier Martinez Canillas, amd-gfx, Leo Li, mwen, Sean Paul,
David Gow, kunit-dev, Mark Yacoub, linux-kernel,
Dmytro Laktyushkin, Nicholas Choi, tales.aparecida, Alex Deucher,
christian.koenig
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@usp.br> 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@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.
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@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.
This is going to be great! We will keep an eye on this proposal.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread
* [RFC 2/3] drm/amd/display: Move bw_fixed macros to header file
2022-06-08 1:07 ` Maíra Canal
@ 2022-06-08 1:07 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: Maíra Canal, kunit-dev, dri-devel, amd-gfx, linux-kernel
The macros defined at bw_fixed are important mathematical definitions,
specifying masks to get the fractional part and the maximum and minimum
values of I64. In order to enable unit tests for bw_fixed, it is
relevant to have access to those macros. This commit moves the macros to
the header file, making it accessible to future unit tests.
Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
.../gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-------------
drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 ++++++++++++++
2 files changed, 15 insertions(+), 13 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 6ca288fb5fb9..d944570e5695 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,19 +26,7 @@
#include "bw_fixed.h"
-#define MIN_I64 \
- (int64_t)(-(1LL << 63))
-
-#define MAX_I64 \
- (int64_t)((1ULL << 63) - 1)
-
-#define FRACTIONAL_PART_MASK \
- ((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)
-
-#define GET_FRACTIONAL_PART(x) \
- (FRACTIONAL_PART_MASK & (x))
-
-static uint64_t abs_i64(int64_t arg)
+uint64_t abs_i64(int64_t arg)
{
if (arg >= 0)
return (uint64_t)(arg);
diff --git a/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h b/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
index d1656c9d50df..064839425b96 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
@@ -33,12 +33,26 @@ struct bw_fixed {
int64_t value;
};
+#define MIN_I64 \
+ (int64_t)(-(1ULL << 63))
+
+#define MAX_I64 \
+ (int64_t)((1ULL << 63) - 1)
+
+#define FRACTIONAL_PART_MASK \
+ ((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)
+
+#define GET_FRACTIONAL_PART(x) \
+ (FRACTIONAL_PART_MASK & (x))
+
#define BW_FIXED_MIN_I32 \
(int64_t)(-(1LL << (63 - BW_FIXED_BITS_PER_FRACTIONAL_PART)))
#define BW_FIXED_MAX_I32 \
(int64_t)((1ULL << (63 - BW_FIXED_BITS_PER_FRACTIONAL_PART)) - 1)
+uint64_t abs_i64(int64_t arg);
+
static inline struct bw_fixed bw_min2(const struct bw_fixed arg1,
const struct bw_fixed arg2)
{
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [RFC 2/3] drm/amd/display: Move bw_fixed macros to header file
@ 2022-06-08 1:07 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: amd-gfx, dri-devel, linux-kernel, kunit-dev, Maíra Canal
The macros defined at bw_fixed are important mathematical definitions,
specifying masks to get the fractional part and the maximum and minimum
values of I64. In order to enable unit tests for bw_fixed, it is
relevant to have access to those macros. This commit moves the macros to
the header file, making it accessible to future unit tests.
Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
.../gpu/drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-------------
drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 ++++++++++++++
2 files changed, 15 insertions(+), 13 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 6ca288fb5fb9..d944570e5695 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,19 +26,7 @@
#include "bw_fixed.h"
-#define MIN_I64 \
- (int64_t)(-(1LL << 63))
-
-#define MAX_I64 \
- (int64_t)((1ULL << 63) - 1)
-
-#define FRACTIONAL_PART_MASK \
- ((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)
-
-#define GET_FRACTIONAL_PART(x) \
- (FRACTIONAL_PART_MASK & (x))
-
-static uint64_t abs_i64(int64_t arg)
+uint64_t abs_i64(int64_t arg)
{
if (arg >= 0)
return (uint64_t)(arg);
diff --git a/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h b/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
index d1656c9d50df..064839425b96 100644
--- a/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
+++ b/drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h
@@ -33,12 +33,26 @@ struct bw_fixed {
int64_t value;
};
+#define MIN_I64 \
+ (int64_t)(-(1ULL << 63))
+
+#define MAX_I64 \
+ (int64_t)((1ULL << 63) - 1)
+
+#define FRACTIONAL_PART_MASK \
+ ((1ULL << BW_FIXED_BITS_PER_FRACTIONAL_PART) - 1)
+
+#define GET_FRACTIONAL_PART(x) \
+ (FRACTIONAL_PART_MASK & (x))
+
#define BW_FIXED_MIN_I32 \
(int64_t)(-(1LL << (63 - BW_FIXED_BITS_PER_FRACTIONAL_PART)))
#define BW_FIXED_MAX_I32 \
(int64_t)((1ULL << (63 - BW_FIXED_BITS_PER_FRACTIONAL_PART)) - 1)
+uint64_t abs_i64(int64_t arg);
+
static inline struct bw_fixed bw_min2(const struct bw_fixed arg1,
const struct bw_fixed arg2)
{
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* [RFC 3/3] drm/amd/display: Introduce KUnit tests to the bw_fixed library
2022-06-08 1:07 ` Maíra Canal
@ 2022-06-08 1:07 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: Maíra Canal, kunit-dev, dri-devel, amd-gfx, linux-kernel
From: Magali Lemes <magalilemes00@gmail.com>
The bw_fixed library performs a lot of the mathematical operations
involving fixed-point arithmetic and the conversion of integers to
fixed-point representation.
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.
Co-developed-by: Tales Aparecida <tales.aparecida@gmail.com>
Signed-off-by: Tales Aparecida <tales.aparecida@gmail.com>
Signed-off-by: Magali Lemes <magalilemes00@gmail.com>
Co-developed-by: Maíra Canal <maira.canal@usp.br>
Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
.../drm/amd/display/amdgpu_dm/tests/Kconfig | 12 +
.../drm/amd/display/amdgpu_dm/tests/Makefile | 4 +
.../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
.../amd/display/amdgpu_dm/tests/dml_test.c | 3 +
.../amd/display/amdgpu_dm/tests/dml_test.h | 8 +
5 files changed, 349 insertions(+)
create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
index bd1d971d4452..540b2f79f971 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
@@ -2,6 +2,18 @@
menu "DML Unit Tests"
depends on DRM_AMD_DC && KUNIT=m
+config BW_FIXED_KUNIT_TEST
+ bool "Enable unit tests for dml/calcs/bw_fixed" if !DML_KUNIT_TEST
+ default y if DML_KUNIT_TEST
+ help
+ Enables unit tests for the dml/calcs/bw_fixed. 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 DISPLAY_MODE_LIB_KUNIT_TEST
bool "Enable unit tests for dml/display_mode_lib" if !DML_KUNIT_TEST
default y if DML_KUNIT_TEST
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
index 53b38e340564..23109e51cf32 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
@@ -5,6 +5,10 @@
DML_TESTS = dml_test.o
+ifdef CONFIG_BW_FIXED_KUNIT_TEST
+DML_TESTS += calcs/bw_fixed_test.o
+endif
+
ifdef CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST
DML_TESTS += display_mode_lib_test.o
endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
new file mode 100644
index 000000000000..344c1517745e
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: MIT
+/*
+ * KUnit tests for dml/calcs/bw_fixed.h
+ *
+ * Copyright (C) 2022, Magali Lemes <magalilemes00@gmail.com>
+ * Copyright (C) 2022, Maíra Canal <mairacanal@riseup.net>
+ * Copyright (C) 2022, Tales Aparecida <tales.aparecida@gmail.com>
+ */
+
+#include <kunit/test.h>
+#include "../../../dc/inc/bw_fixed.h"
+#include "../dml_test.h"
+
+/**
+ * DOC: Unit tests for AMDGPU DML calcs/bw_fixed.h
+ *
+ * bw_fixed.h performs a lot of the mathematical operations involving
+ * fixed-point arithmetic and the conversion of integers to fixed-point
+ * representation.
+ *
+ * As fixed-point representation is the base foundation of the DML calcs
+ * operations, these 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.
+ *
+ */
+
+static void abs_i64_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 0ULL, abs_i64(0LL));
+
+ /* Argument type limits */
+ KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64, abs_i64(MAX_I64));
+ KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64 + 1, abs_i64(MIN_I64));
+}
+
+static void bw_int_to_fixed_nonconst_test(struct kunit *test)
+{
+ struct bw_fixed res;
+
+ /* Add BW_FIXED_BITS_PER_FRACTIONAL_PART trailing 0s to binary number */
+ res = bw_int_to_fixed_nonconst(1000); /* 0x3E8 */
+ KUNIT_EXPECT_EQ(test, 16777216000, res.value); /* 0x3E8000000 */
+
+ res = bw_int_to_fixed_nonconst(-1000); /* -0x3E8 */
+ KUNIT_EXPECT_EQ(test, -16777216000, res.value); /* -0x3E8000000 */
+
+ res = bw_int_to_fixed_nonconst(0LL);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ /**
+ * Test corner cases, as the function's argument has to be an int64_t
+ * between BW_FIXED_MIN_I32 and BW_FIXED_MAX_I32.
+ */
+ res = bw_int_to_fixed_nonconst(BW_FIXED_MAX_I32 - 1); /* 0x7FFFFFFFFE */
+ KUNIT_EXPECT_EQ(test, 9223372036821221376, res.value); /* 0x7FFFFFFFFE000000 */
+
+ res = bw_int_to_fixed_nonconst(BW_FIXED_MIN_I32 + 1); /* -0x7FFFFFFFFF */
+ KUNIT_EXPECT_EQ(test, -9223372036837998592, res.value); /* -0x7FFFFFFFFF000000 */
+}
+
+static void bw_frc_to_fixed_test(struct kunit *test)
+{
+ struct bw_fixed res;
+
+ /* Extreme scenarios */
+
+ /* A fraction of N/N should result in "1.0" */
+ res = bw_frc_to_fixed(MAX_I64, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 1LL << BW_FIXED_BITS_PER_FRACTIONAL_PART, res.value);
+
+ res = bw_frc_to_fixed(1, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 0LL, res.value);
+
+ res = bw_frc_to_fixed(0, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 0LL, res.value);
+
+ /* Turn a repeating decimal to the fixed-point representation */
+
+ /* A repeating decimal that doesn't round up the LSB */
+ res = bw_frc_to_fixed(4, 3);
+ KUNIT_EXPECT_EQ(test, 22369621LL, res.value); /* 0x1555555 */
+
+ res = bw_frc_to_fixed(-4, 3);
+ KUNIT_EXPECT_EQ(test, -22369621LL, res.value); /* -0x1555555 */
+
+ res = bw_frc_to_fixed(99999997, 100000000);
+ KUNIT_EXPECT_EQ(test, 16777215LL, res.value); /* 0x0FFFFFF */
+
+ /* A repeating decimal that rounds up the MSB */
+ res = bw_frc_to_fixed(5, 3);
+ KUNIT_EXPECT_EQ(test, 27962027LL, res.value); /* 0x1AAAAAB */
+
+ res = bw_frc_to_fixed(-5, 3);
+ KUNIT_EXPECT_EQ(test, -27962027LL, res.value); /* -0x1AAAAAB */
+
+ res = bw_frc_to_fixed(99999998, 100000000);
+ KUNIT_EXPECT_EQ(test, 1LL << BW_FIXED_BITS_PER_FRACTIONAL_PART, res.value);
+
+ /* Turn a terminating decimal to the fixed-point representation */
+ res = bw_frc_to_fixed(62609, 100);
+ KUNIT_EXPECT_EQ(test, 10504047165LL, res.value); /* 0X272170A3D */
+
+ res = bw_frc_to_fixed(-62609, 100);
+ KUNIT_EXPECT_EQ(test, -10504047165LL, res.value); /* -0X272170A3D */
+}
+
+static void bw_floor2_test(struct kunit *test)
+{
+ struct bw_fixed arg;
+ struct bw_fixed significance;
+ struct bw_fixed res;
+
+ /* Round 10 down to the nearest multiple of 3 */
+ arg.value = 10;
+ significance.value = 3;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 9, res.value);
+
+ /* Round 10 down to the nearest multiple of 5 */
+ arg.value = 10;
+ significance.value = 5;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 10, res.value);
+
+ /* Round 100 down to the nearest multiple of 7 */
+ arg.value = 100;
+ significance.value = 7;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 98, res.value);
+
+ /* Round an integer down to its nearest multiple should return itself */
+ arg.value = MAX_I64;
+ significance.value = MAX_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ arg.value = MIN_I64;
+ significance.value = MIN_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MIN_I64, res.value);
+
+ /* Value is a multiple of significance, result should be value */
+ arg.value = MAX_I64;
+ significance.value = MIN_I64 + 1;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ /* Round 0 down to the nearest multiple of any number should return 0 */
+ arg.value = 0;
+ significance.value = MAX_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg.value = 0;
+ significance.value = MIN_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+}
+
+static void bw_ceil2_test(struct kunit *test)
+{
+ struct bw_fixed arg;
+ struct bw_fixed significance;
+ struct bw_fixed res;
+
+ /* Round 10 up to the nearest multiple of 3 */
+ arg.value = 10;
+ significance.value = 3;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 12, res.value);
+
+ /* Round 10 up to the nearest multiple of 5 */
+ arg.value = 10;
+ significance.value = 5;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 10, res.value);
+
+ /* Round 100 up to the nearest multiple of 7 */
+ arg.value = 100;
+ significance.value = 7;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 105, res.value);
+
+ /* Round an integer up to its nearest multiple should return itself */
+ arg.value = MAX_I64;
+ significance.value = MAX_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ arg.value = MIN_I64 + 1;
+ significance.value = MIN_I64 + 1;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MIN_I64 + 1, res.value);
+
+ /* Value is a multiple of significance, result should be value */
+ arg.value = MAX_I64;
+ significance.value = MIN_I64 + 1;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ /* Round 0 up to the nearest multiple of any number should return 0 */
+ arg.value = 0;
+ significance.value = MAX_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg.value = 0;
+ significance.value = MIN_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+}
+
+static void bw_mul_test(struct kunit *test)
+{
+ struct bw_fixed arg1;
+ struct bw_fixed arg2;
+ struct bw_fixed res;
+ struct bw_fixed ans;
+
+ /* Extreme scenario */
+ arg1.value = MAX_I64;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MAX_I32 + 1, res.value);
+
+ /* Testing multiplication property: x * 1 = x */
+ arg1.value = 1;
+ arg2.value = MAX_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MAX_I32 + 1, res.value);
+
+ arg1.value = 1;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MIN_I32, res.value);
+
+ /* Testing multiplication property: x * 0 = 0 */
+ arg1.value = 0;
+ arg2.value = 0;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg1.value = 0;
+ arg2.value = MAX_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg1.value = 0;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ /* Testing multiplication between integers */
+ res = bw_mul(bw_int_to_fixed(8), bw_int_to_fixed(10));
+ KUNIT_EXPECT_EQ(test, 1342177280LL, res.value); /* 0x50000000 */
+
+ res = bw_mul(bw_int_to_fixed(10), bw_int_to_fixed(5));
+ KUNIT_EXPECT_EQ(test, 838860800LL, res.value); /* 0x32000000 */
+
+ res = bw_mul(bw_int_to_fixed(-10), bw_int_to_fixed(7));
+ KUNIT_EXPECT_EQ(test, -1174405120LL, res.value); /* -0x46000000 */
+
+ /* Testing multiplication between fractions and integers */
+ res = bw_mul(bw_frc_to_fixed(4, 3), bw_int_to_fixed(3));
+ ans = bw_int_to_fixed(4);
+
+ /**
+ * As bw_frc_to_fixed(4, 3) didn't round up the fixed-point representation,
+ * the ans must be subtrated by 1.
+ */
+ KUNIT_EXPECT_EQ(test, ans.value - 1, res.value);
+
+ res = bw_mul(bw_frc_to_fixed(5, 3), bw_int_to_fixed(3));
+ ans = bw_int_to_fixed(5);
+
+ /**
+ * As bw_frc_to_fixed(5, 3) rounds up the fixed-point representation,
+ * the ans must be added by 1.
+ */
+ KUNIT_EXPECT_EQ(test, ans.value + 1, res.value);
+}
+
+static struct kunit_case bw_fixed_test_cases[] = {
+ KUNIT_CASE(abs_i64_test),
+ KUNIT_CASE(bw_int_to_fixed_nonconst_test),
+ KUNIT_CASE(bw_frc_to_fixed_test),
+ KUNIT_CASE(bw_floor2_test),
+ KUNIT_CASE(bw_ceil2_test),
+ KUNIT_CASE(bw_mul_test),
+ { }
+};
+
+static struct kunit_suite bw_fixed_test_suite = {
+ .name = "dml-calcs-bw-fixed",
+ .test_cases = bw_fixed_test_cases,
+};
+
+static struct kunit_suite *bw_fixed_test_suites[] = { &bw_fixed_test_suite, NULL };
+
+int bw_fixed_test_init(void)
+{
+ pr_info("===> Running calcs/bw_fixed KUnit Tests");
+ pr_info("**********************************************************");
+ pr_info("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **");
+ pr_info("** **");
+ pr_info("** calcs/bw_fixed KUnit Tests are being run. This means **");
+ pr_info("** that this is a TEST kernel and should not be used **");
+ pr_info("** 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("**********************************************************");
+
+ return __kunit_test_suites_init(bw_fixed_test_suites);
+}
+
+void bw_fixed_test_exit(void)
+{
+ return __kunit_test_suites_exit(bw_fixed_test_suites);
+}
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
index 9a5d47597c10..98ae4e8cd952 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
@@ -13,11 +13,14 @@
*/
int amdgpu_dml_test_init(void)
{
+ bw_fixed_test_init();
display_mode_lib_test_init();
+
return 0;
}
void amdgpu_dml_test_exit(void)
{
display_mode_lib_test_exit();
+ bw_fixed_test_exit();
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
index 2786db9d0e87..d8fe38abd9bc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
@@ -2,6 +2,14 @@
#ifndef DML_TEST_H_
#define DML_TEST_H_
+#if defined (CONFIG_BW_FIXED_KUNIT_TEST)
+int bw_fixed_test_init(void);
+void bw_fixed_test_exit(void);
+#else
+static inline int bw_fixed_test_init(void) { return 0; }
+static inline void bw_fixed_test_exit(void) { }
+#endif
+
#if defined (CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST)
int display_mode_lib_test_init(void);
void display_mode_lib_test_exit(void);
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread* [RFC 3/3] drm/amd/display: Introduce KUnit tests to the bw_fixed library
@ 2022-06-08 1:07 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-08 1:07 UTC (permalink / raw)
To: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
christian.koenig, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, David Gow, Daniel Latypov,
brendanhiggins
Cc: amd-gfx, dri-devel, linux-kernel, kunit-dev, Maíra Canal
From: Magali Lemes <magalilemes00@gmail.com>
The bw_fixed library performs a lot of the mathematical operations
involving fixed-point arithmetic and the conversion of integers to
fixed-point representation.
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.
Co-developed-by: Tales Aparecida <tales.aparecida@gmail.com>
Signed-off-by: Tales Aparecida <tales.aparecida@gmail.com>
Signed-off-by: Magali Lemes <magalilemes00@gmail.com>
Co-developed-by: Maíra Canal <maira.canal@usp.br>
Signed-off-by: Maíra Canal <maira.canal@usp.br>
---
.../drm/amd/display/amdgpu_dm/tests/Kconfig | 12 +
.../drm/amd/display/amdgpu_dm/tests/Makefile | 4 +
.../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
.../amd/display/amdgpu_dm/tests/dml_test.c | 3 +
.../amd/display/amdgpu_dm/tests/dml_test.h | 8 +
5 files changed, 349 insertions(+)
create mode 100644 drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
index bd1d971d4452..540b2f79f971 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Kconfig
@@ -2,6 +2,18 @@
menu "DML Unit Tests"
depends on DRM_AMD_DC && KUNIT=m
+config BW_FIXED_KUNIT_TEST
+ bool "Enable unit tests for dml/calcs/bw_fixed" if !DML_KUNIT_TEST
+ default y if DML_KUNIT_TEST
+ help
+ Enables unit tests for the dml/calcs/bw_fixed. 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 DISPLAY_MODE_LIB_KUNIT_TEST
bool "Enable unit tests for dml/display_mode_lib" if !DML_KUNIT_TEST
default y if DML_KUNIT_TEST
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
index 53b38e340564..23109e51cf32 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/Makefile
@@ -5,6 +5,10 @@
DML_TESTS = dml_test.o
+ifdef CONFIG_BW_FIXED_KUNIT_TEST
+DML_TESTS += calcs/bw_fixed_test.o
+endif
+
ifdef CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST
DML_TESTS += display_mode_lib_test.o
endif
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
new file mode 100644
index 000000000000..344c1517745e
--- /dev/null
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/calcs/bw_fixed_test.c
@@ -0,0 +1,322 @@
+// SPDX-License-Identifier: MIT
+/*
+ * KUnit tests for dml/calcs/bw_fixed.h
+ *
+ * Copyright (C) 2022, Magali Lemes <magalilemes00@gmail.com>
+ * Copyright (C) 2022, Maíra Canal <mairacanal@riseup.net>
+ * Copyright (C) 2022, Tales Aparecida <tales.aparecida@gmail.com>
+ */
+
+#include <kunit/test.h>
+#include "../../../dc/inc/bw_fixed.h"
+#include "../dml_test.h"
+
+/**
+ * DOC: Unit tests for AMDGPU DML calcs/bw_fixed.h
+ *
+ * bw_fixed.h performs a lot of the mathematical operations involving
+ * fixed-point arithmetic and the conversion of integers to fixed-point
+ * representation.
+ *
+ * As fixed-point representation is the base foundation of the DML calcs
+ * operations, these 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.
+ *
+ */
+
+static void abs_i64_test(struct kunit *test)
+{
+ KUNIT_EXPECT_EQ(test, 0ULL, abs_i64(0LL));
+
+ /* Argument type limits */
+ KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64, abs_i64(MAX_I64));
+ KUNIT_EXPECT_EQ(test, (uint64_t)MAX_I64 + 1, abs_i64(MIN_I64));
+}
+
+static void bw_int_to_fixed_nonconst_test(struct kunit *test)
+{
+ struct bw_fixed res;
+
+ /* Add BW_FIXED_BITS_PER_FRACTIONAL_PART trailing 0s to binary number */
+ res = bw_int_to_fixed_nonconst(1000); /* 0x3E8 */
+ KUNIT_EXPECT_EQ(test, 16777216000, res.value); /* 0x3E8000000 */
+
+ res = bw_int_to_fixed_nonconst(-1000); /* -0x3E8 */
+ KUNIT_EXPECT_EQ(test, -16777216000, res.value); /* -0x3E8000000 */
+
+ res = bw_int_to_fixed_nonconst(0LL);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ /**
+ * Test corner cases, as the function's argument has to be an int64_t
+ * between BW_FIXED_MIN_I32 and BW_FIXED_MAX_I32.
+ */
+ res = bw_int_to_fixed_nonconst(BW_FIXED_MAX_I32 - 1); /* 0x7FFFFFFFFE */
+ KUNIT_EXPECT_EQ(test, 9223372036821221376, res.value); /* 0x7FFFFFFFFE000000 */
+
+ res = bw_int_to_fixed_nonconst(BW_FIXED_MIN_I32 + 1); /* -0x7FFFFFFFFF */
+ KUNIT_EXPECT_EQ(test, -9223372036837998592, res.value); /* -0x7FFFFFFFFF000000 */
+}
+
+static void bw_frc_to_fixed_test(struct kunit *test)
+{
+ struct bw_fixed res;
+
+ /* Extreme scenarios */
+
+ /* A fraction of N/N should result in "1.0" */
+ res = bw_frc_to_fixed(MAX_I64, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 1LL << BW_FIXED_BITS_PER_FRACTIONAL_PART, res.value);
+
+ res = bw_frc_to_fixed(1, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 0LL, res.value);
+
+ res = bw_frc_to_fixed(0, MAX_I64);
+ KUNIT_EXPECT_EQ(test, 0LL, res.value);
+
+ /* Turn a repeating decimal to the fixed-point representation */
+
+ /* A repeating decimal that doesn't round up the LSB */
+ res = bw_frc_to_fixed(4, 3);
+ KUNIT_EXPECT_EQ(test, 22369621LL, res.value); /* 0x1555555 */
+
+ res = bw_frc_to_fixed(-4, 3);
+ KUNIT_EXPECT_EQ(test, -22369621LL, res.value); /* -0x1555555 */
+
+ res = bw_frc_to_fixed(99999997, 100000000);
+ KUNIT_EXPECT_EQ(test, 16777215LL, res.value); /* 0x0FFFFFF */
+
+ /* A repeating decimal that rounds up the MSB */
+ res = bw_frc_to_fixed(5, 3);
+ KUNIT_EXPECT_EQ(test, 27962027LL, res.value); /* 0x1AAAAAB */
+
+ res = bw_frc_to_fixed(-5, 3);
+ KUNIT_EXPECT_EQ(test, -27962027LL, res.value); /* -0x1AAAAAB */
+
+ res = bw_frc_to_fixed(99999998, 100000000);
+ KUNIT_EXPECT_EQ(test, 1LL << BW_FIXED_BITS_PER_FRACTIONAL_PART, res.value);
+
+ /* Turn a terminating decimal to the fixed-point representation */
+ res = bw_frc_to_fixed(62609, 100);
+ KUNIT_EXPECT_EQ(test, 10504047165LL, res.value); /* 0X272170A3D */
+
+ res = bw_frc_to_fixed(-62609, 100);
+ KUNIT_EXPECT_EQ(test, -10504047165LL, res.value); /* -0X272170A3D */
+}
+
+static void bw_floor2_test(struct kunit *test)
+{
+ struct bw_fixed arg;
+ struct bw_fixed significance;
+ struct bw_fixed res;
+
+ /* Round 10 down to the nearest multiple of 3 */
+ arg.value = 10;
+ significance.value = 3;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 9, res.value);
+
+ /* Round 10 down to the nearest multiple of 5 */
+ arg.value = 10;
+ significance.value = 5;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 10, res.value);
+
+ /* Round 100 down to the nearest multiple of 7 */
+ arg.value = 100;
+ significance.value = 7;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 98, res.value);
+
+ /* Round an integer down to its nearest multiple should return itself */
+ arg.value = MAX_I64;
+ significance.value = MAX_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ arg.value = MIN_I64;
+ significance.value = MIN_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MIN_I64, res.value);
+
+ /* Value is a multiple of significance, result should be value */
+ arg.value = MAX_I64;
+ significance.value = MIN_I64 + 1;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ /* Round 0 down to the nearest multiple of any number should return 0 */
+ arg.value = 0;
+ significance.value = MAX_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg.value = 0;
+ significance.value = MIN_I64;
+ res = bw_floor2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+}
+
+static void bw_ceil2_test(struct kunit *test)
+{
+ struct bw_fixed arg;
+ struct bw_fixed significance;
+ struct bw_fixed res;
+
+ /* Round 10 up to the nearest multiple of 3 */
+ arg.value = 10;
+ significance.value = 3;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 12, res.value);
+
+ /* Round 10 up to the nearest multiple of 5 */
+ arg.value = 10;
+ significance.value = 5;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 10, res.value);
+
+ /* Round 100 up to the nearest multiple of 7 */
+ arg.value = 100;
+ significance.value = 7;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 105, res.value);
+
+ /* Round an integer up to its nearest multiple should return itself */
+ arg.value = MAX_I64;
+ significance.value = MAX_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ arg.value = MIN_I64 + 1;
+ significance.value = MIN_I64 + 1;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MIN_I64 + 1, res.value);
+
+ /* Value is a multiple of significance, result should be value */
+ arg.value = MAX_I64;
+ significance.value = MIN_I64 + 1;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, MAX_I64, res.value);
+
+ /* Round 0 up to the nearest multiple of any number should return 0 */
+ arg.value = 0;
+ significance.value = MAX_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg.value = 0;
+ significance.value = MIN_I64;
+ res = bw_ceil2(arg, significance);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+}
+
+static void bw_mul_test(struct kunit *test)
+{
+ struct bw_fixed arg1;
+ struct bw_fixed arg2;
+ struct bw_fixed res;
+ struct bw_fixed ans;
+
+ /* Extreme scenario */
+ arg1.value = MAX_I64;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MAX_I32 + 1, res.value);
+
+ /* Testing multiplication property: x * 1 = x */
+ arg1.value = 1;
+ arg2.value = MAX_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MAX_I32 + 1, res.value);
+
+ arg1.value = 1;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, BW_FIXED_MIN_I32, res.value);
+
+ /* Testing multiplication property: x * 0 = 0 */
+ arg1.value = 0;
+ arg2.value = 0;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg1.value = 0;
+ arg2.value = MAX_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ arg1.value = 0;
+ arg2.value = MIN_I64;
+ res = bw_mul(arg1, arg2);
+ KUNIT_EXPECT_EQ(test, 0, res.value);
+
+ /* Testing multiplication between integers */
+ res = bw_mul(bw_int_to_fixed(8), bw_int_to_fixed(10));
+ KUNIT_EXPECT_EQ(test, 1342177280LL, res.value); /* 0x50000000 */
+
+ res = bw_mul(bw_int_to_fixed(10), bw_int_to_fixed(5));
+ KUNIT_EXPECT_EQ(test, 838860800LL, res.value); /* 0x32000000 */
+
+ res = bw_mul(bw_int_to_fixed(-10), bw_int_to_fixed(7));
+ KUNIT_EXPECT_EQ(test, -1174405120LL, res.value); /* -0x46000000 */
+
+ /* Testing multiplication between fractions and integers */
+ res = bw_mul(bw_frc_to_fixed(4, 3), bw_int_to_fixed(3));
+ ans = bw_int_to_fixed(4);
+
+ /**
+ * As bw_frc_to_fixed(4, 3) didn't round up the fixed-point representation,
+ * the ans must be subtrated by 1.
+ */
+ KUNIT_EXPECT_EQ(test, ans.value - 1, res.value);
+
+ res = bw_mul(bw_frc_to_fixed(5, 3), bw_int_to_fixed(3));
+ ans = bw_int_to_fixed(5);
+
+ /**
+ * As bw_frc_to_fixed(5, 3) rounds up the fixed-point representation,
+ * the ans must be added by 1.
+ */
+ KUNIT_EXPECT_EQ(test, ans.value + 1, res.value);
+}
+
+static struct kunit_case bw_fixed_test_cases[] = {
+ KUNIT_CASE(abs_i64_test),
+ KUNIT_CASE(bw_int_to_fixed_nonconst_test),
+ KUNIT_CASE(bw_frc_to_fixed_test),
+ KUNIT_CASE(bw_floor2_test),
+ KUNIT_CASE(bw_ceil2_test),
+ KUNIT_CASE(bw_mul_test),
+ { }
+};
+
+static struct kunit_suite bw_fixed_test_suite = {
+ .name = "dml-calcs-bw-fixed",
+ .test_cases = bw_fixed_test_cases,
+};
+
+static struct kunit_suite *bw_fixed_test_suites[] = { &bw_fixed_test_suite, NULL };
+
+int bw_fixed_test_init(void)
+{
+ pr_info("===> Running calcs/bw_fixed KUnit Tests");
+ pr_info("**********************************************************");
+ pr_info("** NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE NOTICE **");
+ pr_info("** **");
+ pr_info("** calcs/bw_fixed KUnit Tests are being run. This means **");
+ pr_info("** that this is a TEST kernel and should not be used **");
+ pr_info("** 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("**********************************************************");
+
+ return __kunit_test_suites_init(bw_fixed_test_suites);
+}
+
+void bw_fixed_test_exit(void)
+{
+ return __kunit_test_suites_exit(bw_fixed_test_suites);
+}
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
index 9a5d47597c10..98ae4e8cd952 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.c
@@ -13,11 +13,14 @@
*/
int amdgpu_dml_test_init(void)
{
+ bw_fixed_test_init();
display_mode_lib_test_init();
+
return 0;
}
void amdgpu_dml_test_exit(void)
{
display_mode_lib_test_exit();
+ bw_fixed_test_exit();
}
diff --git a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
index 2786db9d0e87..d8fe38abd9bc 100644
--- a/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
+++ b/drivers/gpu/drm/amd/display/amdgpu_dm/tests/dml_test.h
@@ -2,6 +2,14 @@
#ifndef DML_TEST_H_
#define DML_TEST_H_
+#if defined (CONFIG_BW_FIXED_KUNIT_TEST)
+int bw_fixed_test_init(void);
+void bw_fixed_test_exit(void);
+#else
+static inline int bw_fixed_test_init(void) { return 0; }
+static inline void bw_fixed_test_exit(void) { }
+#endif
+
#if defined (CONFIG_DISPLAY_MODE_LIB_KUNIT_TEST)
int display_mode_lib_test_init(void);
void display_mode_lib_test_exit(void);
--
2.36.1
^ permalink raw reply related [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-08 1:07 ` Maíra Canal
(?)
@ 2022-06-16 14:39 ` David Gow
-1 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-16 14:39 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx, Leo Li,
mwen, Sean Paul, KUnit Development, Mark Yacoub,
Linux Kernel Mailing List, Dmytro Laktyushkin, Nicholas Choi,
tales.aparecida, Alex Deucher, Christian König
On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> This RFC is a preview of the work being developed by Isabella Basso [1],
> Maíra Canal [2], and Tales Lelo [3], as part of their Google Summer of Code
> projects [4], and Magali Lemes [5], as part of her capstone project.
>
> Our main goal is to bring unit testing to the AMDPGU driver; in particular,
> we'll focus on the Display Mode Library (DML) for DCN2.0 and some of the DCE
> functions. The modern AMD Linux kernel graphics driver is the single largest
> driver in the mainline Linux codebase [6]. As AMD releases new GPU models,
> the size of AMDGPU drivers is only becoming even larger.
>
> Assuring the drivers' quality and reliability becomes a complex task without
> systematic testing, especially for graphic drivers - which usually involve
> tons of complex calculations. Also, keeping bugs away becomes an increasingly
> hard task with the introduction of new code. Moreover, developers might want
> to refactor old code without fear of the introduction of new issues.
>
> In that sense, it is possible to argue for the benefits of implementing unit
> testing at the AMDGPU drivers. This implementation will help developers to
> recognize bugs before they are merged into the mainline and also makes it
> possible for future code refactors of the AMDGPU driver.
>
> When analyzing the AMDGPU driver, a particular part of the driver highlights
> itself as a good candidate for the implementation of unit tests: the Display
> Mode Library (DML), as it is focused on mathematical operations.
>
> For the implementation of the tests, we decided to go with the Kernel Unit
> Testing Framework (KUnit). KUnit makes it possible to run test suites on
> kernel boot or load the tests as a module. It reports all test case results
> through a TAP (Test Anything Protocol) in the kernel log.
>
> Moreover, KUnit unifies the test structure and provides tools to simplify the
> testing for developers and CI systems.
>
> That said, we developed a little snippet on what we intend to develop in our
> summer. We planned the basic structure on how the tests will be introduced
> into the codebase and, on the concern of the CI systems, developed a structure
> where the unit tests can be introduced as modules and run on IGT (the IGT patch
> will be introduced soon).
It's awesome to see this! It's definitely one of the more ambitious
KUnit test projects out there, and the DML code does seem particularly
well suited to unit-testings.
> The way the modules are implemented might seem a little unusual for KUnit
> developers. We need to call the KUnit init function inside the AMDGPU stack,
> otherwise, the test won't compile as a module. So, the solution to this
> problem was based on the unit tests for the Thunderbolt driver, which uses
> KUnit and also tests a physical driver.
>
> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> be able to compile the tests as modules and, therefore, won't be able to use
> IGT to run the tests. This problem with kunit_test_suites() was already
> discussed in the KUnit mailing list, as can be seen in [7].
I'm not sure I fully understand why these tests need to be part of the
amdgpu module, though admittedly I've not played with IGT much. Would
it be possible to compile these tests as separate modules, which could
depend on amdgpu (or maybe include the DML stuff directly), and
therefore not have this conflict? I definitely was able to get these
tests working under kunit_tool (albeit as built-ins) by using
kunit_test_suites(). If each suite were built as a separate module (or
indeed, even if all the tests were in one module, with one list of
suites), then it should be possible to avoid the init_module()
conflict. That'd also make it possible to run these tests without
actually needing the driver to initialise, which seems like it might
require actual hardware(?)
There are two other reasons the 'thunderbolt'-style technique is one
we want to avoid:
1. It makes it much more difficult to run tests using kunit_tool and
KUnit-based CI tools: these tests would not run automatically, and if
they were built-in as-is, they'd need to be
2. We're planning to improve module support to replace the
init_module()-based implementation of kunit_test_suites() with one
which won't have these conflicts, so the need for this should be
short-lived.
If you're curious, an early version of the improved module support can
be found here, though it's out-of-date enough it won't apply or work
as-is:
https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
Now, that's unlikely to be ready very soon, but I'd be hesitant to
implement too extensive a system for avoiding kunit_test_suites()
given at some point it should work and we'll need to migrate back to
it.
At the very least, having the dependency on KUNIT=m is a very bad
idea: it should be possible to have tests built as modules, even if
KUnit itself isn't, and ideally (even if this sort-of implementation
is required), it _should_ be possible to have these tests be built-in
if all their dependencies (KUnit, amdgpu) are, which would make it
possible to run the tests without a userland.
That being said, I've got basically no knowledge of amdgpu (or even
drm in general), so there could be something I'm missing.
>
> The first patch configures the basic structure of the KUnit Tests, setting the
> proper Makefile, Kconfig, and init function. It also contains a simple test
> involving DML logging, which is the pretext for building the testing structure.
>
> The second patch adds KUnit tests to bw_fixed functions. This patch represents
> what we intend to do on the rest of the DML modules: systematic testing of the
> public functions of the DML, especially mathematically complicated functions.
> Also, it shows how simple it is to add new tests to the DML with the structure
> we built.
>
> Any feedback or ideas for the project are welcome!
>
Looks great to me so far: I'll try to get a more detailed review in soon.
Cheers,
-- David
> [1] https://crosscat.me
> [2] https://mairacanal.github.io
> [3] https://tales-aparecida.github.io/
> [4] https://summerofcode.withgoogle.com/programs/2022/organizations/xorg-foundation
> [5] https://magalilemes.github.io/
> [6] https://www.phoronix.com/scan.php?page=news_item&px=AMDGPU-Closing-4-Million
> [7] https://groups.google.com/g/kunit-dev/c/hbJbh8L37FU/m/EmszZE9qBAAJ
>
> - Isabella Basso, Magali Lemes, Maíra Canal, and Tales Lelo
>
> Magali Lemes (1):
> drm/amd/display: Introduce KUnit tests to the bw_fixed library
>
> Maíra Canal (2):
> drm/amd/display: Introduce KUnit to DML
> drm/amd/display: Move bw_fixed macros to header file
>
> 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 | 41 +++
> .../drm/amd/display/amdgpu_dm/tests/Makefile | 18 +
> .../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
> .../amdgpu_dm/tests/display_mode_lib_test.c | 83 +++++
> .../amd/display/amdgpu_dm/tests/dml_test.c | 26 ++
> .../amd/display/amdgpu_dm/tests/dml_test.h | 21 ++
> .../drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-
> drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 +
> 12 files changed, 538 insertions(+), 13 deletions(-)
> 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/calcs/bw_fixed_test.c
> 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
>
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-16 14:39 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-16 14:39 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx,
Harry Wentland, Leo Li, mwen, Sean Paul, KUnit Development,
Mark Yacoub, Linux Kernel Mailing List, Dmytro Laktyushkin,
Nicholas Choi, Daniel Vetter, tales.aparecida, Alex Deucher,
Christian König
On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> This RFC is a preview of the work being developed by Isabella Basso [1],
> Maíra Canal [2], and Tales Lelo [3], as part of their Google Summer of Code
> projects [4], and Magali Lemes [5], as part of her capstone project.
>
> Our main goal is to bring unit testing to the AMDPGU driver; in particular,
> we'll focus on the Display Mode Library (DML) for DCN2.0 and some of the DCE
> functions. The modern AMD Linux kernel graphics driver is the single largest
> driver in the mainline Linux codebase [6]. As AMD releases new GPU models,
> the size of AMDGPU drivers is only becoming even larger.
>
> Assuring the drivers' quality and reliability becomes a complex task without
> systematic testing, especially for graphic drivers - which usually involve
> tons of complex calculations. Also, keeping bugs away becomes an increasingly
> hard task with the introduction of new code. Moreover, developers might want
> to refactor old code without fear of the introduction of new issues.
>
> In that sense, it is possible to argue for the benefits of implementing unit
> testing at the AMDGPU drivers. This implementation will help developers to
> recognize bugs before they are merged into the mainline and also makes it
> possible for future code refactors of the AMDGPU driver.
>
> When analyzing the AMDGPU driver, a particular part of the driver highlights
> itself as a good candidate for the implementation of unit tests: the Display
> Mode Library (DML), as it is focused on mathematical operations.
>
> For the implementation of the tests, we decided to go with the Kernel Unit
> Testing Framework (KUnit). KUnit makes it possible to run test suites on
> kernel boot or load the tests as a module. It reports all test case results
> through a TAP (Test Anything Protocol) in the kernel log.
>
> Moreover, KUnit unifies the test structure and provides tools to simplify the
> testing for developers and CI systems.
>
> That said, we developed a little snippet on what we intend to develop in our
> summer. We planned the basic structure on how the tests will be introduced
> into the codebase and, on the concern of the CI systems, developed a structure
> where the unit tests can be introduced as modules and run on IGT (the IGT patch
> will be introduced soon).
It's awesome to see this! It's definitely one of the more ambitious
KUnit test projects out there, and the DML code does seem particularly
well suited to unit-testings.
> The way the modules are implemented might seem a little unusual for KUnit
> developers. We need to call the KUnit init function inside the AMDGPU stack,
> otherwise, the test won't compile as a module. So, the solution to this
> problem was based on the unit tests for the Thunderbolt driver, which uses
> KUnit and also tests a physical driver.
>
> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> be able to compile the tests as modules and, therefore, won't be able to use
> IGT to run the tests. This problem with kunit_test_suites() was already
> discussed in the KUnit mailing list, as can be seen in [7].
I'm not sure I fully understand why these tests need to be part of the
amdgpu module, though admittedly I've not played with IGT much. Would
it be possible to compile these tests as separate modules, which could
depend on amdgpu (or maybe include the DML stuff directly), and
therefore not have this conflict? I definitely was able to get these
tests working under kunit_tool (albeit as built-ins) by using
kunit_test_suites(). If each suite were built as a separate module (or
indeed, even if all the tests were in one module, with one list of
suites), then it should be possible to avoid the init_module()
conflict. That'd also make it possible to run these tests without
actually needing the driver to initialise, which seems like it might
require actual hardware(?)
There are two other reasons the 'thunderbolt'-style technique is one
we want to avoid:
1. It makes it much more difficult to run tests using kunit_tool and
KUnit-based CI tools: these tests would not run automatically, and if
they were built-in as-is, they'd need to be
2. We're planning to improve module support to replace the
init_module()-based implementation of kunit_test_suites() with one
which won't have these conflicts, so the need for this should be
short-lived.
If you're curious, an early version of the improved module support can
be found here, though it's out-of-date enough it won't apply or work
as-is:
https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
Now, that's unlikely to be ready very soon, but I'd be hesitant to
implement too extensive a system for avoiding kunit_test_suites()
given at some point it should work and we'll need to migrate back to
it.
At the very least, having the dependency on KUNIT=m is a very bad
idea: it should be possible to have tests built as modules, even if
KUnit itself isn't, and ideally (even if this sort-of implementation
is required), it _should_ be possible to have these tests be built-in
if all their dependencies (KUnit, amdgpu) are, which would make it
possible to run the tests without a userland.
That being said, I've got basically no knowledge of amdgpu (or even
drm in general), so there could be something I'm missing.
>
> The first patch configures the basic structure of the KUnit Tests, setting the
> proper Makefile, Kconfig, and init function. It also contains a simple test
> involving DML logging, which is the pretext for building the testing structure.
>
> The second patch adds KUnit tests to bw_fixed functions. This patch represents
> what we intend to do on the rest of the DML modules: systematic testing of the
> public functions of the DML, especially mathematically complicated functions.
> Also, it shows how simple it is to add new tests to the DML with the structure
> we built.
>
> Any feedback or ideas for the project are welcome!
>
Looks great to me so far: I'll try to get a more detailed review in soon.
Cheers,
-- David
> [1] https://crosscat.me
> [2] https://mairacanal.github.io
> [3] https://tales-aparecida.github.io/
> [4] https://summerofcode.withgoogle.com/programs/2022/organizations/xorg-foundation
> [5] https://magalilemes.github.io/
> [6] https://www.phoronix.com/scan.php?page=news_item&px=AMDGPU-Closing-4-Million
> [7] https://groups.google.com/g/kunit-dev/c/hbJbh8L37FU/m/EmszZE9qBAAJ
>
> - Isabella Basso, Magali Lemes, Maíra Canal, and Tales Lelo
>
> Magali Lemes (1):
> drm/amd/display: Introduce KUnit tests to the bw_fixed library
>
> Maíra Canal (2):
> drm/amd/display: Introduce KUnit to DML
> drm/amd/display: Move bw_fixed macros to header file
>
> 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 | 41 +++
> .../drm/amd/display/amdgpu_dm/tests/Makefile | 18 +
> .../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
> .../amdgpu_dm/tests/display_mode_lib_test.c | 83 +++++
> .../amd/display/amdgpu_dm/tests/dml_test.c | 26 ++
> .../amd/display/amdgpu_dm/tests/dml_test.h | 21 ++
> .../drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-
> drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 +
> 12 files changed, 538 insertions(+), 13 deletions(-)
> 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/calcs/bw_fixed_test.c
> 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
>
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-16 14:39 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-16 14:39 UTC (permalink / raw)
To: Maíra Canal
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, Daniel Latypov,
Brendan Higgins, amd-gfx, dri-devel, Linux Kernel Mailing List,
KUnit Development
On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> This RFC is a preview of the work being developed by Isabella Basso [1],
> Maíra Canal [2], and Tales Lelo [3], as part of their Google Summer of Code
> projects [4], and Magali Lemes [5], as part of her capstone project.
>
> Our main goal is to bring unit testing to the AMDPGU driver; in particular,
> we'll focus on the Display Mode Library (DML) for DCN2.0 and some of the DCE
> functions. The modern AMD Linux kernel graphics driver is the single largest
> driver in the mainline Linux codebase [6]. As AMD releases new GPU models,
> the size of AMDGPU drivers is only becoming even larger.
>
> Assuring the drivers' quality and reliability becomes a complex task without
> systematic testing, especially for graphic drivers - which usually involve
> tons of complex calculations. Also, keeping bugs away becomes an increasingly
> hard task with the introduction of new code. Moreover, developers might want
> to refactor old code without fear of the introduction of new issues.
>
> In that sense, it is possible to argue for the benefits of implementing unit
> testing at the AMDGPU drivers. This implementation will help developers to
> recognize bugs before they are merged into the mainline and also makes it
> possible for future code refactors of the AMDGPU driver.
>
> When analyzing the AMDGPU driver, a particular part of the driver highlights
> itself as a good candidate for the implementation of unit tests: the Display
> Mode Library (DML), as it is focused on mathematical operations.
>
> For the implementation of the tests, we decided to go with the Kernel Unit
> Testing Framework (KUnit). KUnit makes it possible to run test suites on
> kernel boot or load the tests as a module. It reports all test case results
> through a TAP (Test Anything Protocol) in the kernel log.
>
> Moreover, KUnit unifies the test structure and provides tools to simplify the
> testing for developers and CI systems.
>
> That said, we developed a little snippet on what we intend to develop in our
> summer. We planned the basic structure on how the tests will be introduced
> into the codebase and, on the concern of the CI systems, developed a structure
> where the unit tests can be introduced as modules and run on IGT (the IGT patch
> will be introduced soon).
It's awesome to see this! It's definitely one of the more ambitious
KUnit test projects out there, and the DML code does seem particularly
well suited to unit-testings.
> The way the modules are implemented might seem a little unusual for KUnit
> developers. We need to call the KUnit init function inside the AMDGPU stack,
> otherwise, the test won't compile as a module. So, the solution to this
> problem was based on the unit tests for the Thunderbolt driver, which uses
> KUnit and also tests a physical driver.
>
> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> be able to compile the tests as modules and, therefore, won't be able to use
> IGT to run the tests. This problem with kunit_test_suites() was already
> discussed in the KUnit mailing list, as can be seen in [7].
I'm not sure I fully understand why these tests need to be part of the
amdgpu module, though admittedly I've not played with IGT much. Would
it be possible to compile these tests as separate modules, which could
depend on amdgpu (or maybe include the DML stuff directly), and
therefore not have this conflict? I definitely was able to get these
tests working under kunit_tool (albeit as built-ins) by using
kunit_test_suites(). If each suite were built as a separate module (or
indeed, even if all the tests were in one module, with one list of
suites), then it should be possible to avoid the init_module()
conflict. That'd also make it possible to run these tests without
actually needing the driver to initialise, which seems like it might
require actual hardware(?)
There are two other reasons the 'thunderbolt'-style technique is one
we want to avoid:
1. It makes it much more difficult to run tests using kunit_tool and
KUnit-based CI tools: these tests would not run automatically, and if
they were built-in as-is, they'd need to be
2. We're planning to improve module support to replace the
init_module()-based implementation of kunit_test_suites() with one
which won't have these conflicts, so the need for this should be
short-lived.
If you're curious, an early version of the improved module support can
be found here, though it's out-of-date enough it won't apply or work
as-is:
https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
Now, that's unlikely to be ready very soon, but I'd be hesitant to
implement too extensive a system for avoiding kunit_test_suites()
given at some point it should work and we'll need to migrate back to
it.
At the very least, having the dependency on KUNIT=m is a very bad
idea: it should be possible to have tests built as modules, even if
KUnit itself isn't, and ideally (even if this sort-of implementation
is required), it _should_ be possible to have these tests be built-in
if all their dependencies (KUnit, amdgpu) are, which would make it
possible to run the tests without a userland.
That being said, I've got basically no knowledge of amdgpu (or even
drm in general), so there could be something I'm missing.
>
> The first patch configures the basic structure of the KUnit Tests, setting the
> proper Makefile, Kconfig, and init function. It also contains a simple test
> involving DML logging, which is the pretext for building the testing structure.
>
> The second patch adds KUnit tests to bw_fixed functions. This patch represents
> what we intend to do on the rest of the DML modules: systematic testing of the
> public functions of the DML, especially mathematically complicated functions.
> Also, it shows how simple it is to add new tests to the DML with the structure
> we built.
>
> Any feedback or ideas for the project are welcome!
>
Looks great to me so far: I'll try to get a more detailed review in soon.
Cheers,
-- David
> [1] https://crosscat.me
> [2] https://mairacanal.github.io
> [3] https://tales-aparecida.github.io/
> [4] https://summerofcode.withgoogle.com/programs/2022/organizations/xorg-foundation
> [5] https://magalilemes.github.io/
> [6] https://www.phoronix.com/scan.php?page=news_item&px=AMDGPU-Closing-4-Million
> [7] https://groups.google.com/g/kunit-dev/c/hbJbh8L37FU/m/EmszZE9qBAAJ
>
> - Isabella Basso, Magali Lemes, Maíra Canal, and Tales Lelo
>
> Magali Lemes (1):
> drm/amd/display: Introduce KUnit tests to the bw_fixed library
>
> Maíra Canal (2):
> drm/amd/display: Introduce KUnit to DML
> drm/amd/display: Move bw_fixed macros to header file
>
> 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 | 41 +++
> .../drm/amd/display/amdgpu_dm/tests/Makefile | 18 +
> .../amdgpu_dm/tests/calcs/bw_fixed_test.c | 322 ++++++++++++++++++
> .../amdgpu_dm/tests/display_mode_lib_test.c | 83 +++++
> .../amd/display/amdgpu_dm/tests/dml_test.c | 26 ++
> .../amd/display/amdgpu_dm/tests/dml_test.h | 21 ++
> .../drm/amd/display/dc/dml/calcs/bw_fixed.c | 14 +-
> drivers/gpu/drm/amd/display/dc/inc/bw_fixed.h | 14 +
> 12 files changed, 538 insertions(+), 13 deletions(-)
> 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/calcs/bw_fixed_test.c
> 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
>
> --
> 2.36.1
>
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-16 14:39 ` David Gow
(?)
@ 2022-06-16 22:41 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-16 22:41 UTC (permalink / raw)
To: David Gow
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, Daniel Latypov,
Brendan Higgins, amd-gfx, dri-devel, Linux Kernel Mailing List,
KUnit Development
Hi David,
Thank you for your feedback!
On 6/16/22 11:39, David Gow wrote:
> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>> be able to compile the tests as modules and, therefore, won't be able to use
>> IGT to run the tests. This problem with kunit_test_suites() was already
>> discussed in the KUnit mailing list, as can be seen in [7].
>
> I'm not sure I fully understand why these tests need to be part of the
> amdgpu module, though admittedly I've not played with IGT much. Would
> it be possible to compile these tests as separate modules, which could
> depend on amdgpu (or maybe include the DML stuff directly), and
> therefore not have this conflict? I definitely was able to get these
> tests working under kunit_tool (albeit as built-ins) by using
> kunit_test_suites(). If each suite were built as a separate module (or
> indeed, even if all the tests were in one module, with one list of
> suites), then it should be possible to avoid the init_module()
> conflict. That'd also make it possible to run these tests without
> actually needing the driver to initialise, which seems like it might
> require actual hardware(?)
Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
At this point, we thought about a couple of options to resolve this problem:
- Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
- Take the Thunderbolt path and add the tests to the driver stack.
We end up taking the Thunderbolt path as it would be more maintainable.
Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> There are two other reasons the 'thunderbolt'-style technique is one
> we want to avoid:
> 1. It makes it much more difficult to run tests using kunit_tool and
> KUnit-based CI tools: these tests would not run automatically, and if
> they were built-in as-is, they'd need to be
> 2. We're planning to improve module support to replace the
> init_module()-based implementation of kunit_test_suites() with one
> which won't have these conflicts, so the need for this should be
> short-lived.
>
> If you're curious, an early version of the improved module support can
> be found here, though it's out-of-date enough it won't apply or work
> as-is:
> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>
> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> implement too extensive a system for avoiding kunit_test_suites()
> given at some point it should work and we'll need to migrate back to
> it.
We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
Could you explain more about what is missing to make this improved module support come upstream?
>
> At the very least, having the dependency on KUNIT=m is a very bad
> idea: it should be possible to have tests built as modules, even if
> KUnit itself isn't, and ideally (even if this sort-of implementation
> is required), it _should_ be possible to have these tests be built-in
> if all their dependencies (KUnit, amdgpu) are, which would make it
> possible to run the tests without a userland.
>
Thank you for the suggestion! We will change the KUNIT dependency.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-16 22:41 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-16 22:41 UTC (permalink / raw)
To: David Gow
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx,
Harry Wentland, Leo Li, mwen, Sean Paul, KUnit Development,
Mark Yacoub, Linux Kernel Mailing List, Dmytro Laktyushkin,
Nicholas Choi, Daniel Vetter, tales.aparecida, Alex Deucher,
Christian König
Hi David,
Thank you for your feedback!
On 6/16/22 11:39, David Gow wrote:
> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>> be able to compile the tests as modules and, therefore, won't be able to use
>> IGT to run the tests. This problem with kunit_test_suites() was already
>> discussed in the KUnit mailing list, as can be seen in [7].
>
> I'm not sure I fully understand why these tests need to be part of the
> amdgpu module, though admittedly I've not played with IGT much. Would
> it be possible to compile these tests as separate modules, which could
> depend on amdgpu (or maybe include the DML stuff directly), and
> therefore not have this conflict? I definitely was able to get these
> tests working under kunit_tool (albeit as built-ins) by using
> kunit_test_suites(). If each suite were built as a separate module (or
> indeed, even if all the tests were in one module, with one list of
> suites), then it should be possible to avoid the init_module()
> conflict. That'd also make it possible to run these tests without
> actually needing the driver to initialise, which seems like it might
> require actual hardware(?)
Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
At this point, we thought about a couple of options to resolve this problem:
- Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
- Take the Thunderbolt path and add the tests to the driver stack.
We end up taking the Thunderbolt path as it would be more maintainable.
Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> There are two other reasons the 'thunderbolt'-style technique is one
> we want to avoid:
> 1. It makes it much more difficult to run tests using kunit_tool and
> KUnit-based CI tools: these tests would not run automatically, and if
> they were built-in as-is, they'd need to be
> 2. We're planning to improve module support to replace the
> init_module()-based implementation of kunit_test_suites() with one
> which won't have these conflicts, so the need for this should be
> short-lived.
>
> If you're curious, an early version of the improved module support can
> be found here, though it's out-of-date enough it won't apply or work
> as-is:
> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>
> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> implement too extensive a system for avoiding kunit_test_suites()
> given at some point it should work and we'll need to migrate back to
> it.
We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
Could you explain more about what is missing to make this improved module support come upstream?
>
> At the very least, having the dependency on KUNIT=m is a very bad
> idea: it should be possible to have tests built as modules, even if
> KUnit itself isn't, and ideally (even if this sort-of implementation
> is required), it _should_ be possible to have these tests be built-in
> if all their dependencies (KUnit, amdgpu) are, which would make it
> possible to run the tests without a userland.
>
Thank you for the suggestion! We will change the KUNIT dependency.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-16 22:41 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-16 22:41 UTC (permalink / raw)
To: David Gow
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx, Leo Li,
mwen, Sean Paul, KUnit Development, Mark Yacoub,
Linux Kernel Mailing List, Dmytro Laktyushkin, Nicholas Choi,
tales.aparecida, Alex Deucher, Christian König
Hi David,
Thank you for your feedback!
On 6/16/22 11:39, David Gow wrote:
> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>> be able to compile the tests as modules and, therefore, won't be able to use
>> IGT to run the tests. This problem with kunit_test_suites() was already
>> discussed in the KUnit mailing list, as can be seen in [7].
>
> I'm not sure I fully understand why these tests need to be part of the
> amdgpu module, though admittedly I've not played with IGT much. Would
> it be possible to compile these tests as separate modules, which could
> depend on amdgpu (or maybe include the DML stuff directly), and
> therefore not have this conflict? I definitely was able to get these
> tests working under kunit_tool (albeit as built-ins) by using
> kunit_test_suites(). If each suite were built as a separate module (or
> indeed, even if all the tests were in one module, with one list of
> suites), then it should be possible to avoid the init_module()
> conflict. That'd also make it possible to run these tests without
> actually needing the driver to initialise, which seems like it might
> require actual hardware(?)
Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
At this point, we thought about a couple of options to resolve this problem:
- Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
- Take the Thunderbolt path and add the tests to the driver stack.
We end up taking the Thunderbolt path as it would be more maintainable.
Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> There are two other reasons the 'thunderbolt'-style technique is one
> we want to avoid:
> 1. It makes it much more difficult to run tests using kunit_tool and
> KUnit-based CI tools: these tests would not run automatically, and if
> they were built-in as-is, they'd need to be
> 2. We're planning to improve module support to replace the
> init_module()-based implementation of kunit_test_suites() with one
> which won't have these conflicts, so the need for this should be
> short-lived.
>
> If you're curious, an early version of the improved module support can
> be found here, though it's out-of-date enough it won't apply or work
> as-is:
> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>
> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> implement too extensive a system for avoiding kunit_test_suites()
> given at some point it should work and we'll need to migrate back to
> it.
We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
Could you explain more about what is missing to make this improved module support come upstream?
>
> At the very least, having the dependency on KUNIT=m is a very bad
> idea: it should be possible to have tests built as modules, even if
> KUnit itself isn't, and ideally (even if this sort-of implementation
> is required), it _should_ be possible to have these tests be built-in
> if all their dependencies (KUnit, amdgpu) are, which would make it
> possible to run the tests without a userland.
>
Thank you for the suggestion! We will change the KUNIT dependency.
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread
* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-16 22:41 ` Maíra Canal
(?)
@ 2022-06-17 7:55 ` David Gow
-1 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-17 7:55 UTC (permalink / raw)
To: Maíra Canal
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, Daniel Latypov,
Brendan Higgins, amd-gfx, dri-devel, Linux Kernel Mailing List,
KUnit Development
[-- Attachment #1: Type: text/plain, Size: 6514 bytes --]
On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> Hi David,
>
> Thank you for your feedback!
>
> On 6/16/22 11:39, David Gow wrote:
> > On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> >>
> >> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >> be able to compile the tests as modules and, therefore, won't be able to use
> >> IGT to run the tests. This problem with kunit_test_suites() was already
> >> discussed in the KUnit mailing list, as can be seen in [7].
> >
> > I'm not sure I fully understand why these tests need to be part of the
> > amdgpu module, though admittedly I've not played with IGT much. Would
> > it be possible to compile these tests as separate modules, which could
> > depend on amdgpu (or maybe include the DML stuff directly), and
> > therefore not have this conflict? I definitely was able to get these
> > tests working under kunit_tool (albeit as built-ins) by using
> > kunit_test_suites(). If each suite were built as a separate module (or
> > indeed, even if all the tests were in one module, with one list of
> > suites), then it should be possible to avoid the init_module()
> > conflict. That'd also make it possible to run these tests without
> > actually needing the driver to initialise, which seems like it might
> > require actual hardware(?)
>
> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>
> At this point, we thought about a couple of options to resolve this problem:
> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> - Take the Thunderbolt path and add the tests to the driver stack.
>
> We end up taking the Thunderbolt path as it would be more maintainable.
>
> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>
> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
As you point out, there are really two separate problems with
splitting the tests out totally:
- It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
everywhere.
- It's impossible to have multiple init_module() "calls" in the same module.
The first of these is, I think, the harder to solve generally. (There
are some ways to mitigate the namespace pollution part of it by either
hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
similar, or by using symbol namespaces:
https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
-- or both -- but they don't solve the issue entirely.)
That being said, it's as much a matter of taste as anything, so if
keeping things in the amdgpu module works well, don't let me stop you.
Either way should work, and have their own advantages and
disadvantages.
The latter is just a quirk of the current KUnit implementation of
kunit_test_suites(). This multiple-definition issue will go away in
the not-too-distant future.
So my suggestion here would be to make sure any changes you make to
work around the issue with multiple init_module definitions are easy
to remove. I suspect you could probably significantly simplify the
whole dml_test.{c,h} bit to just directly export the kunit_suites and
maybe throw them all in one array to pass to
__kunit_test_suites_init(). Then, when the improved modules work
lands, they could be deleted entirely and replaced with one or more
calls to kunit_test_suite().
> >
> > There are two other reasons the 'thunderbolt'-style technique is one
> > we want to avoid:
> > 1. It makes it much more difficult to run tests using kunit_tool and
> > KUnit-based CI tools: these tests would not run automatically, and if
> > they were built-in as-is, they'd need to be
> > 2. We're planning to improve module support to replace the
> > init_module()-based implementation of kunit_test_suites() with one
> > which won't have these conflicts, so the need for this should be
> > short-lived.
> >
> > If you're curious, an early version of the improved module support can
> > be found here, though it's out-of-date enough it won't apply or work
> > as-is:
> > https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >
> > Now, that's unlikely to be ready very soon, but I'd be hesitant to
> > implement too extensive a system for avoiding kunit_test_suites()
> > given at some point it should work and we'll need to migrate back to
> > it.
>
> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>
> Could you explain more about what is missing to make this improved module support come upstream?
>
Mostly just time and some other priorities. We've taken another look
at it over the last couple of days, and will try to accelerate getting
it in within the next couple of kernel releases. (Hopefully sooner
rather than later.)
> >
> > At the very least, having the dependency on KUNIT=m is a very bad
> > idea: it should be possible to have tests built as modules, even if
> > KUnit itself isn't, and ideally (even if this sort-of implementation
> > is required), it _should_ be possible to have these tests be built-in
> > if all their dependencies (KUnit, amdgpu) are, which would make it
> > possible to run the tests without a userland.
> >
>
> Thank you for the suggestion! We will change the KUNIT dependency.
Thanks -- with that gone, the tests do build for me under kunit_tool,
though I can't seem to get them to run as-is. (Moving the call to
amdgpu_dml_test_init() into the amdgpu_init() function in amdgpu_drv.c
does work, though I'm not sure if that's a sufficiently correct /
viable solution.)
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-17 7:55 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-17 7:55 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx,
Harry Wentland, Leo Li, mwen, Sean Paul, KUnit Development,
Mark Yacoub, Linux Kernel Mailing List, Dmytro Laktyushkin,
Nicholas Choi, Daniel Vetter, tales.aparecida, Alex Deucher,
Christian König
[-- Attachment #1: Type: text/plain, Size: 6514 bytes --]
On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> Hi David,
>
> Thank you for your feedback!
>
> On 6/16/22 11:39, David Gow wrote:
> > On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> >>
> >> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >> be able to compile the tests as modules and, therefore, won't be able to use
> >> IGT to run the tests. This problem with kunit_test_suites() was already
> >> discussed in the KUnit mailing list, as can be seen in [7].
> >
> > I'm not sure I fully understand why these tests need to be part of the
> > amdgpu module, though admittedly I've not played with IGT much. Would
> > it be possible to compile these tests as separate modules, which could
> > depend on amdgpu (or maybe include the DML stuff directly), and
> > therefore not have this conflict? I definitely was able to get these
> > tests working under kunit_tool (albeit as built-ins) by using
> > kunit_test_suites(). If each suite were built as a separate module (or
> > indeed, even if all the tests were in one module, with one list of
> > suites), then it should be possible to avoid the init_module()
> > conflict. That'd also make it possible to run these tests without
> > actually needing the driver to initialise, which seems like it might
> > require actual hardware(?)
>
> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>
> At this point, we thought about a couple of options to resolve this problem:
> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> - Take the Thunderbolt path and add the tests to the driver stack.
>
> We end up taking the Thunderbolt path as it would be more maintainable.
>
> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>
> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
As you point out, there are really two separate problems with
splitting the tests out totally:
- It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
everywhere.
- It's impossible to have multiple init_module() "calls" in the same module.
The first of these is, I think, the harder to solve generally. (There
are some ways to mitigate the namespace pollution part of it by either
hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
similar, or by using symbol namespaces:
https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
-- or both -- but they don't solve the issue entirely.)
That being said, it's as much a matter of taste as anything, so if
keeping things in the amdgpu module works well, don't let me stop you.
Either way should work, and have their own advantages and
disadvantages.
The latter is just a quirk of the current KUnit implementation of
kunit_test_suites(). This multiple-definition issue will go away in
the not-too-distant future.
So my suggestion here would be to make sure any changes you make to
work around the issue with multiple init_module definitions are easy
to remove. I suspect you could probably significantly simplify the
whole dml_test.{c,h} bit to just directly export the kunit_suites and
maybe throw them all in one array to pass to
__kunit_test_suites_init(). Then, when the improved modules work
lands, they could be deleted entirely and replaced with one or more
calls to kunit_test_suite().
> >
> > There are two other reasons the 'thunderbolt'-style technique is one
> > we want to avoid:
> > 1. It makes it much more difficult to run tests using kunit_tool and
> > KUnit-based CI tools: these tests would not run automatically, and if
> > they were built-in as-is, they'd need to be
> > 2. We're planning to improve module support to replace the
> > init_module()-based implementation of kunit_test_suites() with one
> > which won't have these conflicts, so the need for this should be
> > short-lived.
> >
> > If you're curious, an early version of the improved module support can
> > be found here, though it's out-of-date enough it won't apply or work
> > as-is:
> > https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >
> > Now, that's unlikely to be ready very soon, but I'd be hesitant to
> > implement too extensive a system for avoiding kunit_test_suites()
> > given at some point it should work and we'll need to migrate back to
> > it.
>
> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>
> Could you explain more about what is missing to make this improved module support come upstream?
>
Mostly just time and some other priorities. We've taken another look
at it over the last couple of days, and will try to accelerate getting
it in within the next couple of kernel releases. (Hopefully sooner
rather than later.)
> >
> > At the very least, having the dependency on KUNIT=m is a very bad
> > idea: it should be possible to have tests built as modules, even if
> > KUnit itself isn't, and ideally (even if this sort-of implementation
> > is required), it _should_ be possible to have these tests be built-in
> > if all their dependencies (KUnit, amdgpu) are, which would make it
> > possible to run the tests without a userland.
> >
>
> Thank you for the suggestion! We will change the KUNIT dependency.
Thanks -- with that gone, the tests do build for me under kunit_tool,
though I can't seem to get them to run as-is. (Moving the call to
amdgpu_dml_test_init() into the amdgpu_init() function in amdgpu_drv.c
does work, though I'm not sure if that's a sufficiently correct /
viable solution.)
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-17 7:55 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-17 7:55 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx, Leo Li,
mwen, Sean Paul, KUnit Development, Mark Yacoub,
Linux Kernel Mailing List, Dmytro Laktyushkin, Nicholas Choi,
tales.aparecida, Alex Deucher, Christian König
[-- Attachment #1: Type: text/plain, Size: 6514 bytes --]
On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> Hi David,
>
> Thank you for your feedback!
>
> On 6/16/22 11:39, David Gow wrote:
> > On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> >>
> >> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >> be able to compile the tests as modules and, therefore, won't be able to use
> >> IGT to run the tests. This problem with kunit_test_suites() was already
> >> discussed in the KUnit mailing list, as can be seen in [7].
> >
> > I'm not sure I fully understand why these tests need to be part of the
> > amdgpu module, though admittedly I've not played with IGT much. Would
> > it be possible to compile these tests as separate modules, which could
> > depend on amdgpu (or maybe include the DML stuff directly), and
> > therefore not have this conflict? I definitely was able to get these
> > tests working under kunit_tool (albeit as built-ins) by using
> > kunit_test_suites(). If each suite were built as a separate module (or
> > indeed, even if all the tests were in one module, with one list of
> > suites), then it should be possible to avoid the init_module()
> > conflict. That'd also make it possible to run these tests without
> > actually needing the driver to initialise, which seems like it might
> > require actual hardware(?)
>
> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>
> At this point, we thought about a couple of options to resolve this problem:
> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> - Take the Thunderbolt path and add the tests to the driver stack.
>
> We end up taking the Thunderbolt path as it would be more maintainable.
>
> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>
> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
As you point out, there are really two separate problems with
splitting the tests out totally:
- It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
everywhere.
- It's impossible to have multiple init_module() "calls" in the same module.
The first of these is, I think, the harder to solve generally. (There
are some ways to mitigate the namespace pollution part of it by either
hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
similar, or by using symbol namespaces:
https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
-- or both -- but they don't solve the issue entirely.)
That being said, it's as much a matter of taste as anything, so if
keeping things in the amdgpu module works well, don't let me stop you.
Either way should work, and have their own advantages and
disadvantages.
The latter is just a quirk of the current KUnit implementation of
kunit_test_suites(). This multiple-definition issue will go away in
the not-too-distant future.
So my suggestion here would be to make sure any changes you make to
work around the issue with multiple init_module definitions are easy
to remove. I suspect you could probably significantly simplify the
whole dml_test.{c,h} bit to just directly export the kunit_suites and
maybe throw them all in one array to pass to
__kunit_test_suites_init(). Then, when the improved modules work
lands, they could be deleted entirely and replaced with one or more
calls to kunit_test_suite().
> >
> > There are two other reasons the 'thunderbolt'-style technique is one
> > we want to avoid:
> > 1. It makes it much more difficult to run tests using kunit_tool and
> > KUnit-based CI tools: these tests would not run automatically, and if
> > they were built-in as-is, they'd need to be
> > 2. We're planning to improve module support to replace the
> > init_module()-based implementation of kunit_test_suites() with one
> > which won't have these conflicts, so the need for this should be
> > short-lived.
> >
> > If you're curious, an early version of the improved module support can
> > be found here, though it's out-of-date enough it won't apply or work
> > as-is:
> > https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >
> > Now, that's unlikely to be ready very soon, but I'd be hesitant to
> > implement too extensive a system for avoiding kunit_test_suites()
> > given at some point it should work and we'll need to migrate back to
> > it.
>
> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>
> Could you explain more about what is missing to make this improved module support come upstream?
>
Mostly just time and some other priorities. We've taken another look
at it over the last couple of days, and will try to accelerate getting
it in within the next couple of kernel releases. (Hopefully sooner
rather than later.)
> >
> > At the very least, having the dependency on KUNIT=m is a very bad
> > idea: it should be possible to have tests built as modules, even if
> > KUnit itself isn't, and ideally (even if this sort-of implementation
> > is required), it _should_ be possible to have these tests be built-in
> > if all their dependencies (KUnit, amdgpu) are, which would make it
> > possible to run the tests without a userland.
> >
>
> Thank you for the suggestion! We will change the KUNIT dependency.
Thanks -- with that gone, the tests do build for me under kunit_tool,
though I can't seem to get them to run as-is. (Moving the call to
amdgpu_dml_test_init() into the amdgpu_init() function in amdgpu_drv.c
does work, though I'm not sure if that's a sufficiently correct /
viable solution.)
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-17 7:55 ` David Gow
(?)
@ 2022-06-17 20:24 ` Maíra Canal
-1 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-17 20:24 UTC (permalink / raw)
To: David Gow
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, Daniel Latypov,
Brendan Higgins, amd-gfx, dri-devel, Linux Kernel Mailing List,
KUnit Development
On 6/17/22 04:55, David Gow wrote:
> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> Hi David,
>>
>> Thank you for your feedback!
>>
>> On 6/16/22 11:39, David Gow wrote:
>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>>>>
>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>
>>> I'm not sure I fully understand why these tests need to be part of the
>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>> it be possible to compile these tests as separate modules, which could
>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>> therefore not have this conflict? I definitely was able to get these
>>> tests working under kunit_tool (albeit as built-ins) by using
>>> kunit_test_suites(). If each suite were built as a separate module (or
>>> indeed, even if all the tests were in one module, with one list of
>>> suites), then it should be possible to avoid the init_module()
>>> conflict. That'd also make it possible to run these tests without
>>> actually needing the driver to initialise, which seems like it might
>>> require actual hardware(?)
>>
>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>
>> At this point, we thought about a couple of options to resolve this problem:
>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>> - Take the Thunderbolt path and add the tests to the driver stack.
>>
>> We end up taking the Thunderbolt path as it would be more maintainable.
>>
>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>
>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> As you point out, there are really two separate problems with
> splitting the tests out totally:
> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> everywhere.
> - It's impossible to have multiple init_module() "calls" in the same module.
>
> The first of these is, I think, the harder to solve generally. (There
> are some ways to mitigate the namespace pollution part of it by either
> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> similar, or by using symbol namespaces:
> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> -- or both -- but they don't solve the issue entirely.)
>
> That being said, it's as much a matter of taste as anything, so if
> keeping things in the amdgpu module works well, don't let me stop you.
> Either way should work, and have their own advantages and
> disadvantages.
>
> The latter is just a quirk of the current KUnit implementation of
> kunit_test_suites(). This multiple-definition issue will go away in
> the not-too-distant future.
>
> So my suggestion here would be to make sure any changes you make to
> work around the issue with multiple init_module definitions are easy
> to remove. I suspect you could probably significantly simplify the
> whole dml_test.{c,h} bit to just directly export the kunit_suites and
> maybe throw them all in one array to pass to
> __kunit_test_suites_init(). Then, when the improved modules work
> lands, they could be deleted entirely and replaced with one or more
> calls to kunit_test_suite().
>
>>>
>>> There are two other reasons the 'thunderbolt'-style technique is one
>>> we want to avoid:
>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>> KUnit-based CI tools: these tests would not run automatically, and if
>>> they were built-in as-is, they'd need to be
>>> 2. We're planning to improve module support to replace the
>>> init_module()-based implementation of kunit_test_suites() with one
>>> which won't have these conflicts, so the need for this should be
>>> short-lived.
>>>
>>> If you're curious, an early version of the improved module support can
>>> be found here, though it's out-of-date enough it won't apply or work
>>> as-is:
>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>
>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>> implement too extensive a system for avoiding kunit_test_suites()
>>> given at some point it should work and we'll need to migrate back to
>>> it.
>>
>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>
>> Could you explain more about what is missing to make this improved module support come upstream?
>>
>
> Mostly just time and some other priorities. We've taken another look
> at it over the last couple of days, and will try to accelerate getting
> it in within the next couple of kernel releases. (Hopefully sooner
> rather than later.)
Is there anything we can do to make this move faster? As it is our great
interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
can start work on this feature. We don´t have much knowledge of the
inner workings of KUnit, but if you point out a path, we can try to work
on this task.
Maybe, could we work in the same way as Jeremy?
>
> Cheers,
> -- David
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-17 20:24 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-17 20:24 UTC (permalink / raw)
To: David Gow
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx,
Harry Wentland, Leo Li, mwen, Sean Paul, KUnit Development,
Mark Yacoub, Linux Kernel Mailing List, Dmytro Laktyushkin,
Nicholas Choi, Daniel Vetter, tales.aparecida, Alex Deucher,
Christian König
On 6/17/22 04:55, David Gow wrote:
> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> Hi David,
>>
>> Thank you for your feedback!
>>
>> On 6/16/22 11:39, David Gow wrote:
>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>>>>
>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>
>>> I'm not sure I fully understand why these tests need to be part of the
>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>> it be possible to compile these tests as separate modules, which could
>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>> therefore not have this conflict? I definitely was able to get these
>>> tests working under kunit_tool (albeit as built-ins) by using
>>> kunit_test_suites(). If each suite were built as a separate module (or
>>> indeed, even if all the tests were in one module, with one list of
>>> suites), then it should be possible to avoid the init_module()
>>> conflict. That'd also make it possible to run these tests without
>>> actually needing the driver to initialise, which seems like it might
>>> require actual hardware(?)
>>
>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>
>> At this point, we thought about a couple of options to resolve this problem:
>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>> - Take the Thunderbolt path and add the tests to the driver stack.
>>
>> We end up taking the Thunderbolt path as it would be more maintainable.
>>
>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>
>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> As you point out, there are really two separate problems with
> splitting the tests out totally:
> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> everywhere.
> - It's impossible to have multiple init_module() "calls" in the same module.
>
> The first of these is, I think, the harder to solve generally. (There
> are some ways to mitigate the namespace pollution part of it by either
> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> similar, or by using symbol namespaces:
> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> -- or both -- but they don't solve the issue entirely.)
>
> That being said, it's as much a matter of taste as anything, so if
> keeping things in the amdgpu module works well, don't let me stop you.
> Either way should work, and have their own advantages and
> disadvantages.
>
> The latter is just a quirk of the current KUnit implementation of
> kunit_test_suites(). This multiple-definition issue will go away in
> the not-too-distant future.
>
> So my suggestion here would be to make sure any changes you make to
> work around the issue with multiple init_module definitions are easy
> to remove. I suspect you could probably significantly simplify the
> whole dml_test.{c,h} bit to just directly export the kunit_suites and
> maybe throw them all in one array to pass to
> __kunit_test_suites_init(). Then, when the improved modules work
> lands, they could be deleted entirely and replaced with one or more
> calls to kunit_test_suite().
>
>>>
>>> There are two other reasons the 'thunderbolt'-style technique is one
>>> we want to avoid:
>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>> KUnit-based CI tools: these tests would not run automatically, and if
>>> they were built-in as-is, they'd need to be
>>> 2. We're planning to improve module support to replace the
>>> init_module()-based implementation of kunit_test_suites() with one
>>> which won't have these conflicts, so the need for this should be
>>> short-lived.
>>>
>>> If you're curious, an early version of the improved module support can
>>> be found here, though it's out-of-date enough it won't apply or work
>>> as-is:
>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>
>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>> implement too extensive a system for avoiding kunit_test_suites()
>>> given at some point it should work and we'll need to migrate back to
>>> it.
>>
>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>
>> Could you explain more about what is missing to make this improved module support come upstream?
>>
>
> Mostly just time and some other priorities. We've taken another look
> at it over the last couple of days, and will try to accelerate getting
> it in within the next couple of kernel releases. (Hopefully sooner
> rather than later.)
Is there anything we can do to make this move faster? As it is our great
interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
can start work on this feature. We don´t have much knowledge of the
inner workings of KUnit, but if you point out a path, we can try to work
on this task.
Maybe, could we work in the same way as Jeremy?
>
> Cheers,
> -- David
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-17 20:24 ` Maíra Canal
0 siblings, 0 replies; 32+ messages in thread
From: Maíra Canal @ 2022-06-17 20:24 UTC (permalink / raw)
To: David Gow
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx, Leo Li,
mwen, Sean Paul, KUnit Development, Mark Yacoub,
Linux Kernel Mailing List, Dmytro Laktyushkin, Nicholas Choi,
tales.aparecida, Alex Deucher, Christian König
On 6/17/22 04:55, David Gow wrote:
> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> Hi David,
>>
>> Thank you for your feedback!
>>
>> On 6/16/22 11:39, David Gow wrote:
>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>>>>
>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>
>>> I'm not sure I fully understand why these tests need to be part of the
>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>> it be possible to compile these tests as separate modules, which could
>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>> therefore not have this conflict? I definitely was able to get these
>>> tests working under kunit_tool (albeit as built-ins) by using
>>> kunit_test_suites(). If each suite were built as a separate module (or
>>> indeed, even if all the tests were in one module, with one list of
>>> suites), then it should be possible to avoid the init_module()
>>> conflict. That'd also make it possible to run these tests without
>>> actually needing the driver to initialise, which seems like it might
>>> require actual hardware(?)
>>
>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>
>> At this point, we thought about a couple of options to resolve this problem:
>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>> - Take the Thunderbolt path and add the tests to the driver stack.
>>
>> We end up taking the Thunderbolt path as it would be more maintainable.
>>
>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>
>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>
> As you point out, there are really two separate problems with
> splitting the tests out totally:
> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> everywhere.
> - It's impossible to have multiple init_module() "calls" in the same module.
>
> The first of these is, I think, the harder to solve generally. (There
> are some ways to mitigate the namespace pollution part of it by either
> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> similar, or by using symbol namespaces:
> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> -- or both -- but they don't solve the issue entirely.)
>
> That being said, it's as much a matter of taste as anything, so if
> keeping things in the amdgpu module works well, don't let me stop you.
> Either way should work, and have their own advantages and
> disadvantages.
>
> The latter is just a quirk of the current KUnit implementation of
> kunit_test_suites(). This multiple-definition issue will go away in
> the not-too-distant future.
>
> So my suggestion here would be to make sure any changes you make to
> work around the issue with multiple init_module definitions are easy
> to remove. I suspect you could probably significantly simplify the
> whole dml_test.{c,h} bit to just directly export the kunit_suites and
> maybe throw them all in one array to pass to
> __kunit_test_suites_init(). Then, when the improved modules work
> lands, they could be deleted entirely and replaced with one or more
> calls to kunit_test_suite().
>
>>>
>>> There are two other reasons the 'thunderbolt'-style technique is one
>>> we want to avoid:
>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>> KUnit-based CI tools: these tests would not run automatically, and if
>>> they were built-in as-is, they'd need to be
>>> 2. We're planning to improve module support to replace the
>>> init_module()-based implementation of kunit_test_suites() with one
>>> which won't have these conflicts, so the need for this should be
>>> short-lived.
>>>
>>> If you're curious, an early version of the improved module support can
>>> be found here, though it's out-of-date enough it won't apply or work
>>> as-is:
>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>
>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>> implement too extensive a system for avoiding kunit_test_suites()
>>> given at some point it should work and we'll need to migrate back to
>>> it.
>>
>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>
>> Could you explain more about what is missing to make this improved module support come upstream?
>>
>
> Mostly just time and some other priorities. We've taken another look
> at it over the last couple of days, and will try to accelerate getting
> it in within the next couple of kernel releases. (Hopefully sooner
> rather than later.)
Is there anything we can do to make this move faster? As it is our great
interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
can start work on this feature. We don´t have much knowledge of the
inner workings of KUnit, but if you point out a path, we can try to work
on this task.
Maybe, could we work in the same way as Jeremy?
>
> Cheers,
> -- David
- Maíra Canal
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-17 20:24 ` Maíra Canal
(?)
@ 2022-06-18 9:08 ` David Gow
-1 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-18 9:08 UTC (permalink / raw)
To: Maíra Canal
Cc: Harry Wentland, Leo Li, Rodrigo Siqueira, Alex Deucher,
Christian König, Dmytro Laktyushkin, Jun Lei, Nicholas Choi,
Harrison Chiu, Mark Yacoub, Sean Paul, Daniel Vetter,
Javier Martinez Canillas, Isabella Basso, magalilemes00,
tales.aparecida, mwen, andrealmeid, Daniel Latypov,
Brendan Higgins, amd-gfx, dri-devel, Linux Kernel Mailing List,
KUnit Development
[-- Attachment #1: Type: text/plain, Size: 6962 bytes --]
On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> On 6/17/22 04:55, David Gow wrote:
> > On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >> Hi David,
> >>
> >> Thank you for your feedback!
> >>
> >> On 6/16/22 11:39, David Gow wrote:
> >>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >>>>
> >>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >>>> be able to compile the tests as modules and, therefore, won't be able to use
> >>>> IGT to run the tests. This problem with kunit_test_suites() was already
> >>>> discussed in the KUnit mailing list, as can be seen in [7].
> >>>
> >>> I'm not sure I fully understand why these tests need to be part of the
> >>> amdgpu module, though admittedly I've not played with IGT much. Would
> >>> it be possible to compile these tests as separate modules, which could
> >>> depend on amdgpu (or maybe include the DML stuff directly), and
> >>> therefore not have this conflict? I definitely was able to get these
> >>> tests working under kunit_tool (albeit as built-ins) by using
> >>> kunit_test_suites(). If each suite were built as a separate module (or
> >>> indeed, even if all the tests were in one module, with one list of
> >>> suites), then it should be possible to avoid the init_module()
> >>> conflict. That'd also make it possible to run these tests without
> >>> actually needing the driver to initialise, which seems like it might
> >>> require actual hardware(?)
> >>
> >> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
> >>
> >> At this point, we thought about a couple of options to resolve this problem:
> >> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> >> - Take the Thunderbolt path and add the tests to the driver stack.
> >>
> >> We end up taking the Thunderbolt path as it would be more maintainable.
> >>
> >> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
> >>
> >> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
> >
> > As you point out, there are really two separate problems with
> > splitting the tests out totally:
> > - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> > everywhere.
> > - It's impossible to have multiple init_module() "calls" in the same module.
> >
> > The first of these is, I think, the harder to solve generally. (There
> > are some ways to mitigate the namespace pollution part of it by either
> > hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> > similar, or by using symbol namespaces:
> > https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> > -- or both -- but they don't solve the issue entirely.)
> >
> > That being said, it's as much a matter of taste as anything, so if
> > keeping things in the amdgpu module works well, don't let me stop you.
> > Either way should work, and have their own advantages and
> > disadvantages.
> >
> > The latter is just a quirk of the current KUnit implementation of
> > kunit_test_suites(). This multiple-definition issue will go away in
> > the not-too-distant future.
> >
> > So my suggestion here would be to make sure any changes you make to
> > work around the issue with multiple init_module definitions are easy
> > to remove. I suspect you could probably significantly simplify the
> > whole dml_test.{c,h} bit to just directly export the kunit_suites and
> > maybe throw them all in one array to pass to
> > __kunit_test_suites_init(). Then, when the improved modules work
> > lands, they could be deleted entirely and replaced with one or more
> > calls to kunit_test_suite().
> >
> >>>
> >>> There are two other reasons the 'thunderbolt'-style technique is one
> >>> we want to avoid:
> >>> 1. It makes it much more difficult to run tests using kunit_tool and
> >>> KUnit-based CI tools: these tests would not run automatically, and if
> >>> they were built-in as-is, they'd need to be
> >>> 2. We're planning to improve module support to replace the
> >>> init_module()-based implementation of kunit_test_suites() with one
> >>> which won't have these conflicts, so the need for this should be
> >>> short-lived.
> >>>
> >>> If you're curious, an early version of the improved module support can
> >>> be found here, though it's out-of-date enough it won't apply or work
> >>> as-is:
> >>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >>>
> >>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> >>> implement too extensive a system for avoiding kunit_test_suites()
> >>> given at some point it should work and we'll need to migrate back to
> >>> it.
> >>
> >> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
> >>
> >> Could you explain more about what is missing to make this improved module support come upstream?
> >>
> >
> > Mostly just time and some other priorities. We've taken another look
> > at it over the last couple of days, and will try to accelerate getting
> > it in within the next couple of kernel releases. (Hopefully sooner
> > rather than later.)
> Is there anything we can do to make this move faster? As it is our great
> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
> can start work on this feature. We don´t have much knowledge of the
> inner workings of KUnit, but if you point out a path, we can try to work
> on this task.
>
> Maybe, could we work in the same way as Jeremy?
Daniel and I have quickly tidied up and finished the various
in-progress bits of this and sent it out here:
https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
You should be able to apply that series and then just use
kunit_test_suites(), which will no-longer conflict with module_init
functions.
The most useful thing you could do is to test and/or review it --
there's almost certainly something I'll have missed.
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-18 9:08 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-18 9:08 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx,
Harry Wentland, Leo Li, mwen, Sean Paul, KUnit Development,
Mark Yacoub, Linux Kernel Mailing List, Dmytro Laktyushkin,
Nicholas Choi, Daniel Vetter, tales.aparecida, Alex Deucher,
Christian König
[-- Attachment #1: Type: text/plain, Size: 6962 bytes --]
On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> On 6/17/22 04:55, David Gow wrote:
> > On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >> Hi David,
> >>
> >> Thank you for your feedback!
> >>
> >> On 6/16/22 11:39, David Gow wrote:
> >>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >>>>
> >>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >>>> be able to compile the tests as modules and, therefore, won't be able to use
> >>>> IGT to run the tests. This problem with kunit_test_suites() was already
> >>>> discussed in the KUnit mailing list, as can be seen in [7].
> >>>
> >>> I'm not sure I fully understand why these tests need to be part of the
> >>> amdgpu module, though admittedly I've not played with IGT much. Would
> >>> it be possible to compile these tests as separate modules, which could
> >>> depend on amdgpu (or maybe include the DML stuff directly), and
> >>> therefore not have this conflict? I definitely was able to get these
> >>> tests working under kunit_tool (albeit as built-ins) by using
> >>> kunit_test_suites(). If each suite were built as a separate module (or
> >>> indeed, even if all the tests were in one module, with one list of
> >>> suites), then it should be possible to avoid the init_module()
> >>> conflict. That'd also make it possible to run these tests without
> >>> actually needing the driver to initialise, which seems like it might
> >>> require actual hardware(?)
> >>
> >> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
> >>
> >> At this point, we thought about a couple of options to resolve this problem:
> >> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> >> - Take the Thunderbolt path and add the tests to the driver stack.
> >>
> >> We end up taking the Thunderbolt path as it would be more maintainable.
> >>
> >> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
> >>
> >> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
> >
> > As you point out, there are really two separate problems with
> > splitting the tests out totally:
> > - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> > everywhere.
> > - It's impossible to have multiple init_module() "calls" in the same module.
> >
> > The first of these is, I think, the harder to solve generally. (There
> > are some ways to mitigate the namespace pollution part of it by either
> > hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> > similar, or by using symbol namespaces:
> > https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> > -- or both -- but they don't solve the issue entirely.)
> >
> > That being said, it's as much a matter of taste as anything, so if
> > keeping things in the amdgpu module works well, don't let me stop you.
> > Either way should work, and have their own advantages and
> > disadvantages.
> >
> > The latter is just a quirk of the current KUnit implementation of
> > kunit_test_suites(). This multiple-definition issue will go away in
> > the not-too-distant future.
> >
> > So my suggestion here would be to make sure any changes you make to
> > work around the issue with multiple init_module definitions are easy
> > to remove. I suspect you could probably significantly simplify the
> > whole dml_test.{c,h} bit to just directly export the kunit_suites and
> > maybe throw them all in one array to pass to
> > __kunit_test_suites_init(). Then, when the improved modules work
> > lands, they could be deleted entirely and replaced with one or more
> > calls to kunit_test_suite().
> >
> >>>
> >>> There are two other reasons the 'thunderbolt'-style technique is one
> >>> we want to avoid:
> >>> 1. It makes it much more difficult to run tests using kunit_tool and
> >>> KUnit-based CI tools: these tests would not run automatically, and if
> >>> they were built-in as-is, they'd need to be
> >>> 2. We're planning to improve module support to replace the
> >>> init_module()-based implementation of kunit_test_suites() with one
> >>> which won't have these conflicts, so the need for this should be
> >>> short-lived.
> >>>
> >>> If you're curious, an early version of the improved module support can
> >>> be found here, though it's out-of-date enough it won't apply or work
> >>> as-is:
> >>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >>>
> >>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> >>> implement too extensive a system for avoiding kunit_test_suites()
> >>> given at some point it should work and we'll need to migrate back to
> >>> it.
> >>
> >> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
> >>
> >> Could you explain more about what is missing to make this improved module support come upstream?
> >>
> >
> > Mostly just time and some other priorities. We've taken another look
> > at it over the last couple of days, and will try to accelerate getting
> > it in within the next couple of kernel releases. (Hopefully sooner
> > rather than later.)
> Is there anything we can do to make this move faster? As it is our great
> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
> can start work on this feature. We don´t have much knowledge of the
> inner workings of KUnit, but if you point out a path, we can try to work
> on this task.
>
> Maybe, could we work in the same way as Jeremy?
Daniel and I have quickly tidied up and finished the various
in-progress bits of this and sent it out here:
https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
You should be able to apply that series and then just use
kunit_test_suites(), which will no-longer conflict with module_init
functions.
The most useful thing you could do is to test and/or review it --
there's almost certainly something I'll have missed.
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-18 9:08 ` David Gow
0 siblings, 0 replies; 32+ messages in thread
From: David Gow @ 2022-06-18 9:08 UTC (permalink / raw)
To: Maíra Canal
Cc: Harrison Chiu, Daniel Latypov, Brendan Higgins, dri-devel,
Isabella Basso, andrealmeid, Jun Lei, magalilemes00,
Rodrigo Siqueira, Javier Martinez Canillas, amd-gfx, Leo Li,
mwen, Sean Paul, KUnit Development, Mark Yacoub,
Linux Kernel Mailing List, Dmytro Laktyushkin, Nicholas Choi,
tales.aparecida, Alex Deucher, Christian König
[-- Attachment #1: Type: text/plain, Size: 6962 bytes --]
On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>
> On 6/17/22 04:55, David Gow wrote:
> > On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >> Hi David,
> >>
> >> Thank you for your feedback!
> >>
> >> On 6/16/22 11:39, David Gow wrote:
> >>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
> >>
> >>>>
> >>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
> >>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
> >>>> be able to compile the tests as modules and, therefore, won't be able to use
> >>>> IGT to run the tests. This problem with kunit_test_suites() was already
> >>>> discussed in the KUnit mailing list, as can be seen in [7].
> >>>
> >>> I'm not sure I fully understand why these tests need to be part of the
> >>> amdgpu module, though admittedly I've not played with IGT much. Would
> >>> it be possible to compile these tests as separate modules, which could
> >>> depend on amdgpu (or maybe include the DML stuff directly), and
> >>> therefore not have this conflict? I definitely was able to get these
> >>> tests working under kunit_tool (albeit as built-ins) by using
> >>> kunit_test_suites(). If each suite were built as a separate module (or
> >>> indeed, even if all the tests were in one module, with one list of
> >>> suites), then it should be possible to avoid the init_module()
> >>> conflict. That'd also make it possible to run these tests without
> >>> actually needing the driver to initialise, which seems like it might
> >>> require actual hardware(?)
> >>
> >> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
> >>
> >> At this point, we thought about a couple of options to resolve this problem:
> >> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
> >> - Take the Thunderbolt path and add the tests to the driver stack.
> >>
> >> We end up taking the Thunderbolt path as it would be more maintainable.
> >>
> >> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
> >>
> >> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
> >
> > As you point out, there are really two separate problems with
> > splitting the tests out totally:
> > - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
> > everywhere.
> > - It's impossible to have multiple init_module() "calls" in the same module.
> >
> > The first of these is, I think, the harder to solve generally. (There
> > are some ways to mitigate the namespace pollution part of it by either
> > hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
> > similar, or by using symbol namespaces:
> > https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
> > -- or both -- but they don't solve the issue entirely.)
> >
> > That being said, it's as much a matter of taste as anything, so if
> > keeping things in the amdgpu module works well, don't let me stop you.
> > Either way should work, and have their own advantages and
> > disadvantages.
> >
> > The latter is just a quirk of the current KUnit implementation of
> > kunit_test_suites(). This multiple-definition issue will go away in
> > the not-too-distant future.
> >
> > So my suggestion here would be to make sure any changes you make to
> > work around the issue with multiple init_module definitions are easy
> > to remove. I suspect you could probably significantly simplify the
> > whole dml_test.{c,h} bit to just directly export the kunit_suites and
> > maybe throw them all in one array to pass to
> > __kunit_test_suites_init(). Then, when the improved modules work
> > lands, they could be deleted entirely and replaced with one or more
> > calls to kunit_test_suite().
> >
> >>>
> >>> There are two other reasons the 'thunderbolt'-style technique is one
> >>> we want to avoid:
> >>> 1. It makes it much more difficult to run tests using kunit_tool and
> >>> KUnit-based CI tools: these tests would not run automatically, and if
> >>> they were built-in as-is, they'd need to be
> >>> 2. We're planning to improve module support to replace the
> >>> init_module()-based implementation of kunit_test_suites() with one
> >>> which won't have these conflicts, so the need for this should be
> >>> short-lived.
> >>>
> >>> If you're curious, an early version of the improved module support can
> >>> be found here, though it's out-of-date enough it won't apply or work
> >>> as-is:
> >>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
> >>>
> >>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
> >>> implement too extensive a system for avoiding kunit_test_suites()
> >>> given at some point it should work and we'll need to migrate back to
> >>> it.
> >>
> >> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
> >>
> >> Could you explain more about what is missing to make this improved module support come upstream?
> >>
> >
> > Mostly just time and some other priorities. We've taken another look
> > at it over the last couple of days, and will try to accelerate getting
> > it in within the next couple of kernel releases. (Hopefully sooner
> > rather than later.)
> Is there anything we can do to make this move faster? As it is our great
> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
> can start work on this feature. We don´t have much knowledge of the
> inner workings of KUnit, but if you point out a path, we can try to work
> on this task.
>
> Maybe, could we work in the same way as Jeremy?
Daniel and I have quickly tidied up and finished the various
in-progress bits of this and sent it out here:
https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
You should be able to apply that series and then just use
kunit_test_suites(), which will no-longer conflict with module_init
functions.
The most useful thing you could do is to test and/or review it --
there's almost certainly something I'll have missed.
Cheers,
-- David
[-- Attachment #2: S/MIME Cryptographic Signature --]
[-- Type: application/pkcs7-signature, Size: 4003 bytes --]
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
2022-06-18 9:08 ` David Gow
(?)
@ 2022-06-22 22:55 ` Rodrigo Siqueira Jordao
-1 siblings, 0 replies; 32+ messages in thread
From: Rodrigo Siqueira Jordao @ 2022-06-22 22:55 UTC (permalink / raw)
To: David Gow, Maíra Canal, Daniel Latypov, tales.aparecida,
andrealmeid, mwen, magalilemes00, Isabella Basso, Harry Wentland
Cc: Leo Li, Alex Deucher, Christian König, Dmytro Laktyushkin,
Jun Lei, Nicholas Choi, Harrison Chiu, Mark Yacoub, Sean Paul,
Daniel Vetter, Javier Martinez Canillas, Brendan Higgins,
amd-gfx, dri-devel, Linux Kernel Mailing List, KUnit Development
Hi,
First of all, thanks a lot for exploring the introduction of kunit
inside amdgpu.
See my inline comments
On 2022-06-18 05:08, David Gow wrote:
> On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> On 6/17/22 04:55, David Gow wrote:
>>> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>> Hi David,
>>>>
>>>> Thank you for your feedback!
>>>>
>>>> On 6/16/22 11:39, David Gow wrote:
>>>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>>>>
>>>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>>>
>>>>> I'm not sure I fully understand why these tests need to be part of the
>>>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>>>> it be possible to compile these tests as separate modules, which could
>>>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>>>> therefore not have this conflict? I definitely was able to get these
>>>>> tests working under kunit_tool (albeit as built-ins) by using
>>>>> kunit_test_suites(). If each suite were built as a separate module (or
>>>>> indeed, even if all the tests were in one module, with one list of
>>>>> suites), then it should be possible to avoid the init_module()
>>>>> conflict. That'd also make it possible to run these tests without
>>>>> actually needing the driver to initialise, which seems like it might
>>>>> require actual hardware(?)
In the Display code for amdgpu, we heavily rely on IGT for automated
tests. We have some internal CI where we run a large set of IGT tests
per patch, and I'm sure many other DRM developers also use IGT. In this
sense, if we can have an interface inside IGT that can easily run those
kunit tests, we can enable kunit tests in our CI pipeline almost for free :)
We already have a prototype for this sort of integration at:
https://patchwork.freedesktop.org/series/105294/
>>>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>>>
>>>> At this point, we thought about a couple of options to resolve this problem:
>>>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>>>> - Take the Thunderbolt path and add the tests to the driver stack.
>>>>
>>>> We end up taking the Thunderbolt path as it would be more maintainable.
>>>>
>>>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>>>
>>>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>>>
>>> As you point out, there are really two separate problems with
>>> splitting the tests out totally:
>>> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
>>> everywhere.
>>> - It's impossible to have multiple init_module() "calls" in the same module.
>>>
>>> The first of these is, I think, the harder to solve generally. (There
>>> are some ways to mitigate the namespace pollution part of it by either
>>> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
>>> similar, or by using symbol namespaces:
>>> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
>>> -- or both -- but they don't solve the issue entirely.)
>>>
>>> That being said, it's as much a matter of taste as anything, so if
>>> keeping things in the amdgpu module works well, don't let me stop you.
>>> Either way should work, and have their own advantages and
>>> disadvantages.
I want to avoid making changes inside the dc code [1] (or keep it
minimal) for enabling kunit. Aligned with the IGT comment, I'm more
inclined to a solution where we treat the kunit tests for DML as a
module. However, I'm not sure yet if it is possible to have something
like that...
Does it make things easier if we have a single module that handles the
dml-kunit interface, and we can control which test to invoke via kernel
parameter?
1.
https://gitlab.freedesktop.org/agd5f/linux/-/tree/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc
>>> The latter is just a quirk of the current KUnit implementation of
>>> kunit_test_suites(). This multiple-definition issue will go away in
>>> the not-too-distant future.
>>>
>>> So my suggestion here would be to make sure any changes you make to
>>> work around the issue with multiple init_module definitions are easy
>>> to remove. I suspect you could probably significantly simplify the
>>> whole dml_test.{c,h} bit to just directly export the kunit_suites and
>>> maybe throw them all in one array to pass to
>>> __kunit_test_suites_init(). Then, when the improved modules work
>>> lands, they could be deleted entirely and replaced with one or more
>>> calls to kunit_test_suite().
>>>
>>>>>
>>>>> There are two other reasons the 'thunderbolt'-style technique is one
>>>>> we want to avoid:
>>>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>>>> KUnit-based CI tools: these tests would not run automatically, and if
>>>>> they were built-in as-is, they'd need to be
>>>>> 2. We're planning to improve module support to replace the
>>>>> init_module()-based implementation of kunit_test_suites() with one
>>>>> which won't have these conflicts, so the need for this should be
>>>>> short-lived.
>>>>>
>>>>> If you're curious, an early version of the improved module support can
>>>>> be found here, though it's out-of-date enough it won't apply or work
>>>>> as-is:
>>>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>>>
>>>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>>>> implement too extensive a system for avoiding kunit_test_suites()
>>>>> given at some point it should work and we'll need to migrate back to
>>>>> it.
>>>>
>>>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>>>
>>>> Could you explain more about what is missing to make this improved module support come upstream?
>>>>
>>>
>>> Mostly just time and some other priorities. We've taken another look
>>> at it over the last couple of days, and will try to accelerate getting
>>> it in within the next couple of kernel releases. (Hopefully sooner
>>> rather than later.)
>> Is there anything we can do to make this move faster? As it is our great
>> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
>> can start work on this feature. We don´t have much knowledge of the
>> inner workings of KUnit, but if you point out a path, we can try to work
>> on this task.
>>
>> Maybe, could we work in the same way as Jeremy?
>
> Daniel and I have quickly tidied up and finished the various
> in-progress bits of this and sent it out here:
> https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
>
> You should be able to apply that series and then just use
> kunit_test_suites(), which will no-longer conflict with module_init
> functions.
>
> The most useful thing you could do is to test and/or review it --
> there's almost certainly something I'll have missed.
Nice!
Maira/Magali/Isabela/Tales,
Maybe for the next version, we can use the above patches even if they
are not in amd-staging-drm-next yet.
Thanks
Siqueira
> Cheers,
> -- David
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-22 22:55 ` Rodrigo Siqueira Jordao
0 siblings, 0 replies; 32+ messages in thread
From: Rodrigo Siqueira Jordao @ 2022-06-22 22:55 UTC (permalink / raw)
To: David Gow, Maíra Canal, Daniel Latypov, tales.aparecida,
andrealmeid, mwen, magalilemes00, Isabella Basso, Harry Wentland
Cc: Harrison Chiu, Leo Li, Mark Yacoub, Brendan Higgins,
Javier Martinez Canillas, amd-gfx, Linux Kernel Mailing List,
Dmytro Laktyushkin, Nicholas Choi, dri-devel, Daniel Vetter,
Alex Deucher, Sean Paul, Jun Lei, Christian König,
KUnit Development
Hi,
First of all, thanks a lot for exploring the introduction of kunit
inside amdgpu.
See my inline comments
On 2022-06-18 05:08, David Gow wrote:
> On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> On 6/17/22 04:55, David Gow wrote:
>>> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>> Hi David,
>>>>
>>>> Thank you for your feedback!
>>>>
>>>> On 6/16/22 11:39, David Gow wrote:
>>>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>>>>
>>>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>>>
>>>>> I'm not sure I fully understand why these tests need to be part of the
>>>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>>>> it be possible to compile these tests as separate modules, which could
>>>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>>>> therefore not have this conflict? I definitely was able to get these
>>>>> tests working under kunit_tool (albeit as built-ins) by using
>>>>> kunit_test_suites(). If each suite were built as a separate module (or
>>>>> indeed, even if all the tests were in one module, with one list of
>>>>> suites), then it should be possible to avoid the init_module()
>>>>> conflict. That'd also make it possible to run these tests without
>>>>> actually needing the driver to initialise, which seems like it might
>>>>> require actual hardware(?)
In the Display code for amdgpu, we heavily rely on IGT for automated
tests. We have some internal CI where we run a large set of IGT tests
per patch, and I'm sure many other DRM developers also use IGT. In this
sense, if we can have an interface inside IGT that can easily run those
kunit tests, we can enable kunit tests in our CI pipeline almost for free :)
We already have a prototype for this sort of integration at:
https://patchwork.freedesktop.org/series/105294/
>>>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>>>
>>>> At this point, we thought about a couple of options to resolve this problem:
>>>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>>>> - Take the Thunderbolt path and add the tests to the driver stack.
>>>>
>>>> We end up taking the Thunderbolt path as it would be more maintainable.
>>>>
>>>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>>>
>>>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>>>
>>> As you point out, there are really two separate problems with
>>> splitting the tests out totally:
>>> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
>>> everywhere.
>>> - It's impossible to have multiple init_module() "calls" in the same module.
>>>
>>> The first of these is, I think, the harder to solve generally. (There
>>> are some ways to mitigate the namespace pollution part of it by either
>>> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
>>> similar, or by using symbol namespaces:
>>> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
>>> -- or both -- but they don't solve the issue entirely.)
>>>
>>> That being said, it's as much a matter of taste as anything, so if
>>> keeping things in the amdgpu module works well, don't let me stop you.
>>> Either way should work, and have their own advantages and
>>> disadvantages.
I want to avoid making changes inside the dc code [1] (or keep it
minimal) for enabling kunit. Aligned with the IGT comment, I'm more
inclined to a solution where we treat the kunit tests for DML as a
module. However, I'm not sure yet if it is possible to have something
like that...
Does it make things easier if we have a single module that handles the
dml-kunit interface, and we can control which test to invoke via kernel
parameter?
1.
https://gitlab.freedesktop.org/agd5f/linux/-/tree/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc
>>> The latter is just a quirk of the current KUnit implementation of
>>> kunit_test_suites(). This multiple-definition issue will go away in
>>> the not-too-distant future.
>>>
>>> So my suggestion here would be to make sure any changes you make to
>>> work around the issue with multiple init_module definitions are easy
>>> to remove. I suspect you could probably significantly simplify the
>>> whole dml_test.{c,h} bit to just directly export the kunit_suites and
>>> maybe throw them all in one array to pass to
>>> __kunit_test_suites_init(). Then, when the improved modules work
>>> lands, they could be deleted entirely and replaced with one or more
>>> calls to kunit_test_suite().
>>>
>>>>>
>>>>> There are two other reasons the 'thunderbolt'-style technique is one
>>>>> we want to avoid:
>>>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>>>> KUnit-based CI tools: these tests would not run automatically, and if
>>>>> they were built-in as-is, they'd need to be
>>>>> 2. We're planning to improve module support to replace the
>>>>> init_module()-based implementation of kunit_test_suites() with one
>>>>> which won't have these conflicts, so the need for this should be
>>>>> short-lived.
>>>>>
>>>>> If you're curious, an early version of the improved module support can
>>>>> be found here, though it's out-of-date enough it won't apply or work
>>>>> as-is:
>>>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>>>
>>>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>>>> implement too extensive a system for avoiding kunit_test_suites()
>>>>> given at some point it should work and we'll need to migrate back to
>>>>> it.
>>>>
>>>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>>>
>>>> Could you explain more about what is missing to make this improved module support come upstream?
>>>>
>>>
>>> Mostly just time and some other priorities. We've taken another look
>>> at it over the last couple of days, and will try to accelerate getting
>>> it in within the next couple of kernel releases. (Hopefully sooner
>>> rather than later.)
>> Is there anything we can do to make this move faster? As it is our great
>> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
>> can start work on this feature. We don´t have much knowledge of the
>> inner workings of KUnit, but if you point out a path, we can try to work
>> on this task.
>>
>> Maybe, could we work in the same way as Jeremy?
>
> Daniel and I have quickly tidied up and finished the various
> in-progress bits of this and sent it out here:
> https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
>
> You should be able to apply that series and then just use
> kunit_test_suites(), which will no-longer conflict with module_init
> functions.
>
> The most useful thing you could do is to test and/or review it --
> there's almost certainly something I'll have missed.
Nice!
Maira/Magali/Isabela/Tales,
Maybe for the next version, we can use the above patches even if they
are not in amd-staging-drm-next yet.
Thanks
Siqueira
> Cheers,
> -- David
^ permalink raw reply [flat|nested] 32+ messages in thread* Re: [RFC 0/3] drm/amd/display: Introduce KUnit to Display Mode Library
@ 2022-06-22 22:55 ` Rodrigo Siqueira Jordao
0 siblings, 0 replies; 32+ messages in thread
From: Rodrigo Siqueira Jordao @ 2022-06-22 22:55 UTC (permalink / raw)
To: David Gow, Maíra Canal, Daniel Latypov, tales.aparecida,
andrealmeid, mwen, magalilemes00, Isabella Basso, Harry Wentland
Cc: Harrison Chiu, Leo Li, Mark Yacoub, Brendan Higgins,
Javier Martinez Canillas, amd-gfx, Linux Kernel Mailing List,
Dmytro Laktyushkin, Nicholas Choi, dri-devel, Alex Deucher,
Sean Paul, Jun Lei, Christian König, KUnit Development
Hi,
First of all, thanks a lot for exploring the introduction of kunit
inside amdgpu.
See my inline comments
On 2022-06-18 05:08, David Gow wrote:
> On Sat, Jun 18, 2022 at 4:24 AM Maíra Canal <maira.canal@usp.br> wrote:
>>
>> On 6/17/22 04:55, David Gow wrote:
>>> On Fri, Jun 17, 2022 at 6:41 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>> Hi David,
>>>>
>>>> Thank you for your feedback!
>>>>
>>>> On 6/16/22 11:39, David Gow wrote:
>>>>> On Wed, Jun 8, 2022 at 9:08 AM Maíra Canal <maira.canal@usp.br> wrote:
>>>>
>>>>>>
>>>>>> As kunit_test_suites() defines itself as an init_module(), it conflicts with
>>>>>> the existing one at amdgpu_drv. So, if we use kunit_test_suites(), we won't
>>>>>> be able to compile the tests as modules and, therefore, won't be able to use
>>>>>> IGT to run the tests. This problem with kunit_test_suites() was already
>>>>>> discussed in the KUnit mailing list, as can be seen in [7].
>>>>>
>>>>> I'm not sure I fully understand why these tests need to be part of the
>>>>> amdgpu module, though admittedly I've not played with IGT much. Would
>>>>> it be possible to compile these tests as separate modules, which could
>>>>> depend on amdgpu (or maybe include the DML stuff directly), and
>>>>> therefore not have this conflict? I definitely was able to get these
>>>>> tests working under kunit_tool (albeit as built-ins) by using
>>>>> kunit_test_suites(). If each suite were built as a separate module (or
>>>>> indeed, even if all the tests were in one module, with one list of
>>>>> suites), then it should be possible to avoid the init_module()
>>>>> conflict. That'd also make it possible to run these tests without
>>>>> actually needing the driver to initialise, which seems like it might
>>>>> require actual hardware(?)
In the Display code for amdgpu, we heavily rely on IGT for automated
tests. We have some internal CI where we run a large set of IGT tests
per patch, and I'm sure many other DRM developers also use IGT. In this
sense, if we can have an interface inside IGT that can easily run those
kunit tests, we can enable kunit tests in our CI pipeline almost for free :)
We already have a prototype for this sort of integration at:
https://patchwork.freedesktop.org/series/105294/
>>>> Initially, we tried the kunit_test_suites() approach. And it did work pretty well for the kunit_tool (although we didn't test any hardware-specific unit test). But when compiling the test as a module, we would get a linking error, pointing out multiple definitions of 'init_module'/'cleanup_module' at kunit_test_suites().
>>>>
>>>> At this point, we thought about a couple of options to resolve this problem:
>>>> - Add EXPORT_SYMBOL to the functions we would test. But, this doesn't scale pretty well, because it would pollute AMDGPU code as the tests expand.
>>>> - Take the Thunderbolt path and add the tests to the driver stack.
>>>>
>>>> We end up taking the Thunderbolt path as it would be more maintainable.
>>>>
>>>> Compiling the tests as a module is essential to make the tests run at IGT, as IGT essentially loads the module, runs it, and parses the output (a very very simplified explanation of what IGT does). IGT is a very known tool for DRI developers, so we believe that IGT support is crucial for this project.
>>>>
>>>> If you have any other options on how to make the module compilation viable without using the 'thunderbolt'-style, we would be glad to hear your suggestions.
>>>
>>> As you point out, there are really two separate problems with
>>> splitting the tests out totally:
>>> - It's ugly and pollutes the symbol namespace to have EXPORT_SYMBOL()
>>> everywhere.
>>> - It's impossible to have multiple init_module() "calls" in the same module.
>>>
>>> The first of these is, I think, the harder to solve generally. (There
>>> are some ways to mitigate the namespace pollution part of it by either
>>> hiding the EXPORT_SYMBOL() directives behind #ifdef CONFIG_KUNIT or
>>> similar, or by using symbol namespaces:
>>> https://www.kernel.org/doc/html/latest/core-api/symbol-namespaces.html
>>> -- or both -- but they don't solve the issue entirely.)
>>>
>>> That being said, it's as much a matter of taste as anything, so if
>>> keeping things in the amdgpu module works well, don't let me stop you.
>>> Either way should work, and have their own advantages and
>>> disadvantages.
I want to avoid making changes inside the dc code [1] (or keep it
minimal) for enabling kunit. Aligned with the IGT comment, I'm more
inclined to a solution where we treat the kunit tests for DML as a
module. However, I'm not sure yet if it is possible to have something
like that...
Does it make things easier if we have a single module that handles the
dml-kunit interface, and we can control which test to invoke via kernel
parameter?
1.
https://gitlab.freedesktop.org/agd5f/linux/-/tree/amd-staging-drm-next/drivers/gpu/drm/amd/display/dc
>>> The latter is just a quirk of the current KUnit implementation of
>>> kunit_test_suites(). This multiple-definition issue will go away in
>>> the not-too-distant future.
>>>
>>> So my suggestion here would be to make sure any changes you make to
>>> work around the issue with multiple init_module definitions are easy
>>> to remove. I suspect you could probably significantly simplify the
>>> whole dml_test.{c,h} bit to just directly export the kunit_suites and
>>> maybe throw them all in one array to pass to
>>> __kunit_test_suites_init(). Then, when the improved modules work
>>> lands, they could be deleted entirely and replaced with one or more
>>> calls to kunit_test_suite().
>>>
>>>>>
>>>>> There are two other reasons the 'thunderbolt'-style technique is one
>>>>> we want to avoid:
>>>>> 1. It makes it much more difficult to run tests using kunit_tool and
>>>>> KUnit-based CI tools: these tests would not run automatically, and if
>>>>> they were built-in as-is, they'd need to be
>>>>> 2. We're planning to improve module support to replace the
>>>>> init_module()-based implementation of kunit_test_suites() with one
>>>>> which won't have these conflicts, so the need for this should be
>>>>> short-lived.
>>>>>
>>>>> If you're curious, an early version of the improved module support can
>>>>> be found here, though it's out-of-date enough it won't apply or work
>>>>> as-is:
>>>>> https://lore.kernel.org/all/101d12fc9250b7a445ff50a9e7a25cd74d0e16eb.camel@codeconstruct.com.au/
>>>>>
>>>>> Now, that's unlikely to be ready very soon, but I'd be hesitant to
>>>>> implement too extensive a system for avoiding kunit_test_suites()
>>>>> given at some point it should work and we'll need to migrate back to
>>>>> it.
>>>>
>>>> We hope to see in the near future the improved module support from KUnit as it would make the addition of tests much more simple and clean.
>>>>
>>>> Could you explain more about what is missing to make this improved module support come upstream?
>>>>
>>>
>>> Mostly just time and some other priorities. We've taken another look
>>> at it over the last couple of days, and will try to accelerate getting
>>> it in within the next couple of kernel releases. (Hopefully sooner
>>> rather than later.)
>> Is there anything we can do to make this move faster? As it is our great
>> interest to make this work in KUnit, maybe I, Isabella, Tales, or Magali
>> can start work on this feature. We don´t have much knowledge of the
>> inner workings of KUnit, but if you point out a path, we can try to work
>> on this task.
>>
>> Maybe, could we work in the same way as Jeremy?
>
> Daniel and I have quickly tidied up and finished the various
> in-progress bits of this and sent it out here:
> https://lore.kernel.org/linux-kselftest/20220618090310.1174932-1-davidgow@google.com/T/
>
> You should be able to apply that series and then just use
> kunit_test_suites(), which will no-longer conflict with module_init
> functions.
>
> The most useful thing you could do is to test and/or review it --
> there's almost certainly something I'll have missed.
Nice!
Maira/Magali/Isabela/Tales,
Maybe for the next version, we can use the above patches even if they
are not in amd-staging-drm-next yet.
Thanks
Siqueira
> Cheers,
> -- David
^ permalink raw reply [flat|nested] 32+ messages in thread