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

["Christian König" <christian.koenig@amd.com>]

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@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.

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