Re: [RFC v2 1/3] drm/ttm: Introduce KUnit tests

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Christian,

I'm taking a second look at this, and I wonder what would be the benefit of combining the initialization of device and ttm_device. (drm_)device can be initialized indepedently from the test params, so we can utilize .init and .exit callbacks offered by KUnit[1] to prepare and release the device indepedently of our setup. If we were to change it so ttm_device is also initialized there, we'd have to manually call init/fini in every single test. What's more, ttm_pool tests don't depend on ttm_device, and I can imagine that some structs can be tested in a similar way.

Would it be fine with you to rename ttm_kunit_helper_alloc_device(), but leave its definition as it is?

All the best,
Karolina
-------------------------------------------------------------------
[1] - https://kunit.dev/third_party/kernel/docs/api/test.html#c.kunit_suite

On 29.06.2023 12:05, Karolina Stolarek wrote:
Hi Christian,

Thanks a lot for taking a look at my patches.

On 29.06.2023 09:50, Christian König wrote:
Am 27.06.23 um 10:32 schrieb Karolina Stolarek:
Add the initial version of unit tests for ttm_device struct, together
with helper functions.

Signed-off-by: Karolina Stolarek <karolina.stolarek@xxxxxxxxx>
---
  drivers/gpu/drm/Kconfig                       | 15 +++++
  drivers/gpu/drm/ttm/Makefile                  |  1 +
  drivers/gpu/drm/ttm/tests/.kunitconfig        |  4 ++
  drivers/gpu/drm/ttm/tests/Makefile            |  5 ++
  drivers/gpu/drm/ttm/tests/ttm_device_test.c   | 54 +++++++++++++++++
  drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c | 59 +++++++++++++++++++
  drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h | 29 +++++++++
  7 files changed, 167 insertions(+)
  create mode 100644 drivers/gpu/drm/ttm/tests/.kunitconfig
  create mode 100644 drivers/gpu/drm/ttm/tests/Makefile
  create mode 100644 drivers/gpu/drm/ttm/tests/ttm_device_test.c
  create mode 100644 drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
  create mode 100644 drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h

diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
index afb3b2f5f425..53024e44a2d5 100644
--- a/drivers/gpu/drm/Kconfig
+++ b/drivers/gpu/drm/Kconfig
@@ -194,6 +194,21 @@ config DRM_TTM
        GPU memory types. Will be enabled automatically if a device driver
        uses it.
+config DRM_TTM_KUNIT_TEST
+        tristate "KUnit tests for TTM" if !KUNIT_ALL_TESTS
+        default n
+        depends on DRM && KUNIT
+        select DRM_TTM
+        select DRM_EXPORT_FOR_TESTS if m
+        select DRM_KUNIT_TEST_HELPERS
+        default KUNIT_ALL_TESTS
+        help
+          Enables unit tests for TTM, a GPU memory manager subsystem used +          to manage memory buffers. This option is mostly useful for kernel
+          developers.
+
+          If in doubt, say "N".
+
  config DRM_BUDDY
      tristate
      depends on DRM
diff --git a/drivers/gpu/drm/ttm/Makefile b/drivers/gpu/drm/ttm/Makefile
index f906b22959cf..dad298127226 100644
--- a/drivers/gpu/drm/ttm/Makefile
+++ b/drivers/gpu/drm/ttm/Makefile
@@ -8,3 +8,4 @@ ttm-y := ttm_tt.o ttm_bo.o ttm_bo_util.o ttm_bo_vm.o ttm_module.o \
  ttm-$(CONFIG_AGP) += ttm_agp_backend.o
  obj-$(CONFIG_DRM_TTM) += ttm.o
+obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += tests/
diff --git a/drivers/gpu/drm/ttm/tests/.kunitconfig b/drivers/gpu/drm/ttm/tests/.kunitconfig
new file mode 100644
index 000000000000..75fdce0cd98e
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/.kunitconfig
@@ -0,0 +1,4 @@
+CONFIG_KUNIT=y
+CONFIG_DRM=y
+CONFIG_DRM_KUNIT_TEST_HELPERS=y
+CONFIG_DRM_TTM_KUNIT_TEST=y
diff --git a/drivers/gpu/drm/ttm/tests/Makefile b/drivers/gpu/drm/ttm/tests/Makefile
new file mode 100644
index 000000000000..7917805f37af
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/Makefile
@@ -0,0 +1,5 @@
+# SPDX-License-Identifier: GPL-2.0 AND MIT
+
+obj-$(CONFIG_DRM_TTM_KUNIT_TEST) += \
+        ttm_device_test.o \
+        ttm_kunit_helpers.o
diff --git a/drivers/gpu/drm/ttm/tests/ttm_device_test.c b/drivers/gpu/drm/ttm/tests/ttm_device_test.c
new file mode 100644
index 000000000000..08d7314b1e35
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_device_test.c
@@ -0,0 +1,54 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include <drm/ttm/ttm_resource.h>
+#include <drm/ttm/ttm_device.h>
+#include <drm/ttm/ttm_placement.h>
+
+#include "ttm_kunit_helpers.h"
+
+static void ttm_device_init_basic(struct kunit *test)
+{
+    struct ttm_test_devices_priv *priv = test->priv;
+    struct ttm_device *ttm_dev;
+    struct ttm_resource_manager *ttm_sys_man;
+    int err;
+
+    ttm_dev = kunit_kzalloc(test, sizeof(*ttm_dev), GFP_KERNEL);
+    KUNIT_ASSERT_NOT_NULL(test, ttm_dev);
+
+    err = ttm_kunit_helper_alloc_device(test, ttm_dev, false, false);
+    KUNIT_ASSERT_EQ(test, err, 0);
+
+    KUNIT_EXPECT_PTR_EQ(test, ttm_dev->funcs, &ttm_dev_funcs);
+    KUNIT_ASSERT_NOT_NULL(test, ttm_dev->wq);
+    KUNIT_ASSERT_NOT_NULL(test, ttm_dev->man_drv[TTM_PL_SYSTEM]);
+
+    ttm_sys_man = &ttm_dev->sysman;
+    KUNIT_ASSERT_NOT_NULL(test, ttm_sys_man);
+    KUNIT_EXPECT_TRUE(test, ttm_sys_man->use_tt);
+    KUNIT_EXPECT_TRUE(test, ttm_sys_man->use_type);
+    KUNIT_ASSERT_NOT_NULL(test, ttm_sys_man->func);
+
+    KUNIT_EXPECT_PTR_EQ(test, ttm_dev->dev_mapping,
+                priv->drm->anon_inode->i_mapping);
+
+    ttm_device_fini(ttm_dev);
+}
+
+static struct kunit_case ttm_device_test_cases[] = {
+    KUNIT_CASE(ttm_device_init_basic),
+    {}
+};
+
+static struct kunit_suite ttm_device_test_suite = {
+    .name = "ttm_device",
+    .init = ttm_test_devices_init,
+    .exit = ttm_test_devices_fini,
+    .test_cases = ttm_device_test_cases,
+};
+
+kunit_test_suites(&ttm_device_test_suite);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
new file mode 100644
index 000000000000..d03db0405484
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.c
@@ -0,0 +1,59 @@
+// SPDX-License-Identifier: GPL-2.0 AND MIT
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#include "ttm_kunit_helpers.h"
+
+struct ttm_device_funcs ttm_dev_funcs = {
+};
+EXPORT_SYMBOL_GPL(ttm_dev_funcs);
+
+int ttm_kunit_helper_alloc_device(struct kunit *test,

Since this function is only initializing the ttm device I think we should name it ttm_kunit_helper_init_device().

On the other hand I don't see a good reason why it can't also allocate the device.

I believe that's a leftover from times when I thought I'll reuse DRM device between the tests. No problem, I can put them into one function.

All the best,
Karolina


Apart from that looks like a good start,
Christian.

+                  struct ttm_device *ttm,
+                  bool use_dma_alloc,
+                  bool use_dma32)
+{
+    struct ttm_test_devices_priv *priv = test->priv;
+    struct drm_device *drm = priv->drm;
+    int err;
+
+    err = ttm_device_init(ttm, &ttm_dev_funcs, drm->dev,
+                  drm->anon_inode->i_mapping,
+                  drm->vma_offset_manager,
+                  use_dma_alloc, use_dma32);
+
+    return err;
+}
+EXPORT_SYMBOL_GPL(ttm_kunit_helper_alloc_device);
+
+int ttm_test_devices_init(struct kunit *test)
+{
+    struct ttm_test_devices_priv *priv;
+
+    priv = kunit_kzalloc(test, sizeof(*priv), GFP_KERNEL);
+    KUNIT_ASSERT_NOT_NULL(test, priv);
+
+    test->priv = priv;
+
+    priv->dev = drm_kunit_helper_alloc_device(test);
+    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->dev);
+
+    priv->drm = __drm_kunit_helper_alloc_drm_device(test, priv->dev,
+                            sizeof(*priv->drm), 0,
+                            DRIVER_GEM);
+    KUNIT_ASSERT_NOT_ERR_OR_NULL(test, priv->drm);
+
+    return 0;
+}
+EXPORT_SYMBOL_GPL(ttm_test_devices_init);
+
+void ttm_test_devices_fini(struct kunit *test)
+{
+    struct ttm_test_devices_priv *priv = test->priv;
+
+    drm_kunit_helper_free_device(test, priv->dev);
+    drm_dev_put(priv->drm);
+}
+EXPORT_SYMBOL_GPL(ttm_test_devices_fini);
+
+MODULE_LICENSE("GPL");
diff --git a/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
new file mode 100644
index 000000000000..69fb03b9c4d2
--- /dev/null
+++ b/drivers/gpu/drm/ttm/tests/ttm_kunit_helpers.h
@@ -0,0 +1,29 @@
+/* SPDX-License-Identifier: GPL-2.0 AND MIT */
+/*
+ * Copyright © 2023 Intel Corporation
+ */
+#ifndef TTM_KUNIT_HELPERS_H
+#define TTM_KUNIT_HELPERS_H
+
+#include <drm/drm_drv.h>
+#include <drm/ttm/ttm_device.h>
+
+#include <drm/drm_kunit_helpers.h>
+#include <kunit/test.h>
+
+extern struct ttm_device_funcs ttm_dev_funcs;
+
+struct ttm_test_devices_priv {
+    struct drm_device *drm;
+    struct device *dev;
+};
+
+int ttm_kunit_helper_alloc_device(struct kunit *test,
+                  struct ttm_device *ttm,
+                  bool use_dma_alloc,
+                  bool use_dma32);
+
+int ttm_test_devices_init(struct kunit *test);
+void ttm_test_devices_fini(struct kunit *test);
+
+#endif // TTM_KUNIT_HELPERS_H




[Index of Archives]     [Linux DRI Users]     [Linux Intel Graphics]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite News]     [Linux Kernel]     [Linux SCSI]     [XFree86]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Linux Kernel]     [Linux SCSI]     [XFree86]
  Powered by Linux