[RFC v2 1/3] drm/ttm: Introduce KUnit tests
Karolina Stolarek
karolina.stolarek at intel.com
Fri Jun 30 11:09:47 UTC 2023
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 at intel.com>
>>> ---
>>> 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
>>
More information about the dri-devel
mailing list