Re: [PATCH 02/13] drm: add drm_exec selftests v2

[Maíra Canal <mcanal@xxxxxxxxxx>]

Hi Christian,

It would be nice if you use the KUnit macros, instead of pr_info.

On 5/4/23 08:51, Christian König wrote:
> Largely just the initial skeleton.
>
> v2: add array test as well
>
> Signed-off-by: Christian König <christian.koenig@xxxxxxx>
> ---
>   drivers/gpu/drm/Kconfig               |  1 +
>   drivers/gpu/drm/tests/Makefile        |  3 +-
>   drivers/gpu/drm/tests/drm_exec_test.c | 96 +++++++++++++++++++++++++++
>   3 files changed, 99 insertions(+), 1 deletion(-)
>   create mode 100644 drivers/gpu/drm/tests/drm_exec_test.c
>
> diff --git a/drivers/gpu/drm/Kconfig b/drivers/gpu/drm/Kconfig
> index 2dc81eb062eb..068e574e234e 100644
> --- a/drivers/gpu/drm/Kconfig
> +++ b/drivers/gpu/drm/Kconfig
> @@ -80,6 +80,7 @@ config DRM_KUNIT_TEST
>   	select DRM_BUDDY
>   	select DRM_EXPORT_FOR_TESTS if m
>   	select DRM_KUNIT_TEST_HELPERS
> +	select DRM_EXEC
>   	default KUNIT_ALL_TESTS
>   	help
>   	  This builds unit tests for DRM. This option is not useful for
> diff --git a/drivers/gpu/drm/tests/Makefile b/drivers/gpu/drm/tests/Makefile
> index bca726a8f483..ba7baa622675 100644
> --- a/drivers/gpu/drm/tests/Makefile
> +++ b/drivers/gpu/drm/tests/Makefile
> @@ -17,6 +17,7 @@ obj-$(CONFIG_DRM_KUNIT_TEST) += \
>   	drm_modes_test.o \
>   	drm_plane_helper_test.o \
>   	drm_probe_helper_test.o \
> -	drm_rect_test.o
> +	drm_rect_test.o	\
> +	drm_exec_test.o
>   
>   CFLAGS_drm_mm_test.o := $(DISABLE_STRUCTLEAK_PLUGIN)
> diff --git a/drivers/gpu/drm/tests/drm_exec_test.c b/drivers/gpu/drm/tests/drm_exec_test.c
> new file mode 100644
> index 000000000000..26aa13e62d22
> --- /dev/null
> +++ b/drivers/gpu/drm/tests/drm_exec_test.c
> @@ -0,0 +1,96 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2019 Intel Corporation
> + */
> +
> +#define pr_fmt(fmt) "drm_exec: " fmt
> +
> +#include <kunit/test.h>
> +
> +#include <linux/module.h>
> +#include <linux/prime_numbers.h>
> +
> +#include <drm/drm_exec.h>
> +#include <drm/drm_device.h>
> +#include <drm/drm_gem.h>
> +
> +#include "../lib/drm_random.h"
> +
> +static struct drm_device dev;
> +
> +static void drm_exec_sanitycheck(struct kunit *test)
> +{
> +	struct drm_exec exec;
> +
> +	drm_exec_init(&exec, true);
> +	drm_exec_fini(&exec);
> +	pr_info("%s - ok!\n", __func__);

Here you could use KUNIT_SUCCEED(test).

> +}
> +
> +static void drm_exec_lock1(struct kunit *test)

Is there a reason to call the function drm_exec_lock1 instead of
just drm_exec_lock?

> +{
> +	struct drm_gem_object gobj = { };
> +	struct drm_exec exec;
> +	int ret;
> +
> +	drm_gem_private_object_init(&dev, &gobj, PAGE_SIZE);
> +
> +	drm_exec_init(&exec, true);
> +	drm_exec_while_not_all_locked(&exec) {
> +		ret = drm_exec_prepare_obj(&exec, &gobj, 1);
> +		drm_exec_continue_on_contention(&exec);
> +		if (ret) {
> +			drm_exec_fini(&exec);
> +			pr_err("%s - err %d!\n", __func__, ret);

Here you could use KUNIT_FAIL. Same for the other function.

Actually, it would be better if you created a function `exit`
associated with the test suite, where you would call drm_exec_fini,
and checked the ret variable with KUNIT_EXPECT_EQ(test, ret, 0) in
the test.

> +			return;
> +		}
> +	}
> +	drm_exec_fini(&exec);
> +	pr_info("%s - ok!\n", __func__);
> +}
> +
> +static void drm_exec_lock_array(struct kunit *test)
> +{
> +	struct drm_gem_object gobj1 = { };
> +	struct drm_gem_object gobj2 = { };
> +	struct drm_gem_object *array[] = { &gobj1, &gobj2 };
> +	struct drm_exec exec;
> +	int ret;
> +
> +	drm_gem_private_object_init(&dev, &gobj1, PAGE_SIZE);
> +	drm_gem_private_object_init(&dev, &gobj2, PAGE_SIZE);
> +
> +	drm_exec_init(&exec, true);
> +	ret = drm_exec_prepare_array(&exec, array, ARRAY_SIZE(array), 0);
> +	if (ret) {
> +		drm_exec_fini(&exec);
> +		pr_err("%s - err %d!\n", __func__, ret);
> +		return;
> +	}
> +	drm_exec_fini(&exec)> +	pr_info("%s - ok!\n", __func__);
> +}
> +
> +static int drm_exec_suite_init(struct kunit_suite *suite)
> +{
> +	kunit_info(suite, "Testing DRM exec manager\n");

Isn't this already clear by the name of the test?

Best Regards,
- Maíra Canal

> +	return 0;
> +}
> +
> +static struct kunit_case drm_exec_tests[] = {
> +	KUNIT_CASE(drm_exec_sanitycheck),
> +	KUNIT_CASE(drm_exec_lock1),
> +	KUNIT_CASE(drm_exec_lock_array),
> +	{}
> +};
> +
> +static struct kunit_suite drm_exec_test_suite = {
> +	.name = "drm_exec",
> +	.suite_init = drm_exec_suite_init,
> +	.test_cases = drm_exec_tests,
> +};
> +
> +kunit_test_suite(drm_exec_test_suite);
> +
> +MODULE_AUTHOR("AMD");
> +MODULE_LICENSE("GPL and additional rights");