Re: [PATCH i-g-t 04/39] lib/vkms: Add minimal VKMS library and test device default files

[Louis Chauvet <louis.chauvet@bootlin.com>]

Le 18/02/2025 à 17:49, José Expósito a écrit :
> Create a library containing helpers for creating VKMS devices and
> configuring them dynamically using configfs.
> For the moment, add the minimal number of helpers to be able to start
> testing VKMS's configfs support: Create device, destroy device and
> destroy all devices.
> 
> Also, include the simplest possible test using those helpers (checking
> the device's default files) and the scaffolding required to generate
> the documentation.
> 
> Co-developed-by: Jim Shargo <jshargo@chromium.org>
> Signed-off-by: Jim Shargo <jshargo@chromium.org>
> Co-developed-by: Marius Vlad <marius.vlad@collabora.com>
> Signed-off-by: Marius Vlad <marius.vlad@collabora.com>
> Signed-off-by: José Expósito <jose.exposito89@gmail.com>
> ---
>   docs/testplan/meson.build        |   7 +-
>   lib/igt_vkms.c                   | 191 +++++++++++++++++++++++++++++++
>   lib/igt_vkms.h                   |  27 +++++
>   lib/meson.build                  |   1 +
>   meson.build                      |   8 ++
>   tests/meson.build                |   2 +
>   tests/vkms/meson.build           |  13 +++
>   tests/vkms/vkms_configfs.c       | 122 ++++++++++++++++++++
>   tests/vkms/vkms_test_config.json |  72 ++++++++++++
>   9 files changed, 441 insertions(+), 2 deletions(-)
>   create mode 100644 lib/igt_vkms.c
>   create mode 100644 lib/igt_vkms.h
>   create mode 100644 tests/vkms/meson.build
>   create mode 100644 tests/vkms/vkms_configfs.c
>   create mode 100644 tests/vkms/vkms_test_config.json
> 
> diff --git a/docs/testplan/meson.build b/docs/testplan/meson.build
> index 5560347f1..9e22f4c7d 100644
> --- a/docs/testplan/meson.build
> +++ b/docs/testplan/meson.build
> @@ -11,6 +11,7 @@ stylesheet = join_paths(meson.current_source_dir(), 'testplan.css')
>   xe_test_config = join_paths(source_root, 'tests', 'intel', 'xe_test_config.json')
>   kms_test_config = join_paths(source_root, 'tests', 'intel', 'kms_test_config.json')
>   i915_test_config = join_paths(source_root, 'tests', 'intel', 'i915_test_config.json')
> +vkms_test_config = join_paths(source_root, 'tests', 'vkms', 'vkms_test_config.json')
>   
>   check_testlist = []
>   kms_check_testlist = []
> @@ -37,12 +38,14 @@ if build_xe
>   	test_dict = {
>   		'i915_tests': { 'input': i915_test_config, 'extra_args': check_testlist },
>   		'kms_tests': { 'input': kms_test_config, 'extra_args': kms_check_testlist },
> -		'xe_tests': { 'input': xe_test_config, 'extra_args': check_testlist }
> +		'xe_tests': { 'input': xe_test_config, 'extra_args': check_testlist },
> +		'vkms_tests': { 'input': vkms_test_config, 'extra_args': check_testlist }
>   	    }
>   else
>   	test_dict = {
>   	      'i915_tests': { 'input': i915_test_config, 'extra_args': check_testlist },
> -	      'kms_tests': { 'input': kms_test_config, 'extra_args': kms_check_testlist }
> +	      'kms_tests': { 'input': kms_test_config, 'extra_args': kms_check_testlist },
> +	      'vkms_tests': { 'input': vkms_test_config, 'extra_args': check_testlist }
>   	    }
>   endif
>   
> diff --git a/lib/igt_vkms.c b/lib/igt_vkms.c
> new file mode 100644
> index 000000000..8cea94901
> --- /dev/null
> +++ b/lib/igt_vkms.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Google LLC.
> + * Copyright © 2023 Collabora, Ltd.
> + * Copyright © 2024 Red Hat, Inc.
> + */
> +
> +#include <dirent.h>
> +#include <errno.h>
> +#include <string.h>
> +
> +#include <ftw.h>
> +#include <linux/limits.h>
> +#include <sys/stat.h>
> +
> +#include "igt.h"
> +#include "igt_vkms.h"
> +
> +#define VKMS_ROOT_DIR_NAME		"vkms"
> +
> +/**
> + * SECTION:igt_vkms
> + * @short_description: Helpers to create and configure VKMS devices
> + * @title: VKMS
> + * @include: igt_vkms.h
> + *
> + * Helpers for creating VKMS devices and configuring them dynamically.
> + *
> + * First, create a VKMS device, next, add pipeline items (planes, CRTCs,
> + * encoders, CRTCs and connectors) compose the pipeline by attaching each item
> + * using the _attach_ functions and finally, enable the VKMS device.
> + */
> +
> +static const char *mount_vkms_configfs(void)
> +{
> +	static char vkms_root_path[PATH_MAX];
> +	const char *configfs_path;
> +
> +	configfs_path = igt_configfs_mount();
> +	igt_assert_f(configfs_path, "Error mounting configfs");
> +
> +	snprintf(vkms_root_path, sizeof(vkms_root_path), "%s/%s", configfs_path,
> +		 VKMS_ROOT_DIR_NAME);

As in PATCH 02/39, maybe you can add a check:

	igt_assert_lt(snprintf(...), sizeof(vkms_root_path));

> +
> +	return vkms_root_path;
> +}
> +
> +/**
> + * igt_require_vkms_configfs:
> + *
> + * Require that VKMS supports configfs configuration.
> + */
> +void igt_require_vkms_configfs(void)
> +{
> +	const char *vkms_root_path;
> +	DIR *dir;
> +
> +	vkms_root_path = mount_vkms_configfs();
> +
> +	dir = opendir(vkms_root_path);
> +	igt_require(dir);
> +	if (dir)
> +		closedir(dir);
> +}
> +
> +/**
> + * igt_vkms_device_create:
> + * @name: VKMS device name
> + *
> + * Create a directory in the ConfigFS VKMS root directory, where the entire
> + * pipeline will be configured.
> + */
> +igt_vkms_t *igt_vkms_device_create(const char *name)
> +{
> +	igt_vkms_t *dev;
> +	const char *vkms_root_path;
> +	size_t path_len;
> +	DIR *dir;
> +	int ret;
> +
> +	dev = calloc(1, sizeof(*dev));
> +
> +	vkms_root_path = mount_vkms_configfs();
> +
> +	path_len = strlen(vkms_root_path) + strlen(name) + 2;
> +	dev->path = malloc(path_len);
> +	snprintf(dev->path, path_len, "%s/%s", vkms_root_path, name);
> +
> +	dir = opendir(dev->path);
> +	if (dir) {

Can we add some debug logging here? I think it is important to quickly 
know if a device is reused or not.

> +		closedir(dir);
> +	} else {
> +		ret = mkdir(dev->path, 0777);
> +		if (ret != 0) {
> +			free(dev->path);
> +			free(dev);
> +			dev = NULL;
> +		}
> +	}
> +
> +	return dev;
> +}
> +
> +static int detach_pipeline_items(const char *path, const struct stat *info,
> +				 const int typeflag, struct FTW *pathinfo)
> +{
> +	/* Level 4 are the links in the possible_* directories */

Can you add an example path to clarify this comment? Same for other 
usage of pathinfo->level

	/*
	 * Level 4 are the links in the possible_* directories
	 * i.e: vkms/<DEV>/<OBJECT>/<POSSIBLE_*>/*
	 */

Can we also log the unlinked files?

> +	if (pathinfo->level == 4 && typeflag == FTW_SL)
> +		return unlink(path);
> +
> +	/* Ignore the other files, they are removed by remove_pipeline_items */
> +	return 0;
> +}
> +
> +static int remove_pipeline_items(const char *path, const struct stat *info,
> +				 const int typeflag, struct FTW *pathinfo)
> +{
> +	/* Level 0 is the device root directory */
> +	if (pathinfo->level == 0)
> +		return rmdir(path);
> +
> +	/* Level 2 directories are the pipeline items */
> +	if (pathinfo->level == 2 && typeflag == FTW_DP)
> +		return rmdir(path);

Can we log the deleted files? I think it can be useful because 
igt_vkms_device_destroy only prints the root directory, not which file 
we fail to remove.

> +
> +	/* Ignore the other files, they are removed by VKMS */
> +	return 0;
> +}
> +
> +static int remove_device_dir(igt_vkms_t *dev)
> +{
> +	int ret;
> +
> +	ret = nftw(dev->path, detach_pipeline_items, 64, FTW_DEPTH | FTW_PHYS);
> +	if (ret)
> +		return ret;
> +
> +	ret = nftw(dev->path, remove_pipeline_items, 64, FTW_DEPTH | FTW_PHYS);
> +	return ret;
> +}
> +
> +/**
> + * igt_vkms_device_destroy:
> + * @dev: Device to destroy
> + *
> + * Remove and free the VKMS device.
> + */
> +void igt_vkms_device_destroy(igt_vkms_t *dev)
> +{
> +	int ret;
> +
> +	igt_assert(dev);
> +
> +	ret = remove_device_dir(dev);
> +	igt_assert_f(ret == 0,
> +		     "Unable to rmdir device directory '%s'. Got errno=%d (%s)\n",
> +		     dev->path, errno, strerror(errno));
> +
> +	free(dev->path);
> +	free(dev);
> +}
> +
> +/**
> + * igt_vkms_destroy_all_devices:
> + *
> + * Remove all VKMS devices created via configfs.
> + */
> +void igt_vkms_destroy_all_devices(void)
> +{
> +	igt_vkms_t *dev;
> +	const char *vkms_root_path;
> +	DIR *dir;
> +	struct dirent *ent;
> +
> +	vkms_root_path = mount_vkms_configfs();
> +	dir = opendir(vkms_root_path);
> +	igt_assert_f(dir, "VKMS configfs directory not available at '%s'. "
> +		     "Got errno=%d (%s)\n", vkms_root_path, errno,
> +		     strerror(errno));
> +
> +	while ((ent = readdir(dir)) != NULL) {
> +		if (strcmp(ent->d_name, ".") == 0 ||
> +		    strcmp(ent->d_name, "..") == 0)
> +			continue;
> +
> +		dev = igt_vkms_device_create(ent->d_name);
> +		igt_vkms_device_destroy(dev);
> +	}
> +
> +	closedir(dir);
> +}
> diff --git a/lib/igt_vkms.h b/lib/igt_vkms.h
> new file mode 100644
> index 000000000..48c48f296
> --- /dev/null
> +++ b/lib/igt_vkms.h
> @@ -0,0 +1,27 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright © 2023 Google LLC.
> + * Copyright © 2023 Collabora, Ltd.
> + * Copyright © 2024 Red Hat, Inc.
> + */
> +
> +#ifndef __IGT_VKMS_H__
> +#define __IGT_VKMS_H__
> +
> +/**
> + * igt_vkms_t:
> + * @path: VKMS root directory inside configfs mounted directory
> + *
> + * A struct representing a VKMS device.
> + */
> +typedef struct igt_vkms {
> +	char *path;
> +} igt_vkms_t;
> +
> +void igt_require_vkms_configfs(void);
> +
> +igt_vkms_t *igt_vkms_device_create(const char *name);
> +void igt_vkms_device_destroy(igt_vkms_t *dev);
> +void igt_vkms_destroy_all_devices(void);
> +
> +#endif /* __IGT_VKMS_H__ */
> diff --git a/lib/meson.build b/lib/meson.build
> index 201b1fbb4..8e0b4b7e3 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -50,6 +50,7 @@ lib_sources = [
>   	'igt_types.c',
>   	'igt_vec.c',
>   	'igt_vgem.c',
> +	'igt_vkms.c',
>   	'igt_x86.c',
>   	'instdone.c',
>   	'intel_allocator.c',
> diff --git a/meson.build b/meson.build
> index 2f663dc03..aec49152b 100644
> --- a/meson.build
> +++ b/meson.build
> @@ -287,6 +287,7 @@ msmdir = join_paths(libexecdir, 'msm')
>   panfrostdir = join_paths(libexecdir, 'panfrost')
>   v3ddir = join_paths(libexecdir, 'v3d')
>   vc4dir = join_paths(libexecdir, 'vc4')
> +vkmsdir = join_paths(libexecdir, 'vkms')
>   vmwgfxdir = join_paths(libexecdir, 'vmwgfx')
>   mandir = get_option('mandir')
>   pkgconfigdir = join_paths(libdir, 'pkgconfig')
> @@ -348,6 +349,12 @@ if get_option('use_rpath')
>   	endforeach
>   	vc4_rpathdir = join_paths(vc4_rpathdir, libdir)
>   
> +	vkms_rpathdir = '$ORIGIN'
> +	foreach p : vkmsdir.split('/')
> +		vkms_rpathdir = join_paths(vkms_rpathdir, '..')
> +	endforeach
> +	vkms_rpathdir = join_paths(vkms_rpathdir, libdir)
> +
>   	vmwgfx_rpathdir = '$ORIGIN'
>   	foreach p : vmwgfxdir.split('/')
>   		vmwgfx_rpathdir = join_paths(vmwgfx_rpathdir, '..')
> @@ -361,6 +368,7 @@ else
>   	panfrost_rpathdir = ''
>   	v3d_rpathdir = ''
>   	vc4_rpathdir = ''
> +	vkms_rpathdir = ''
>   	vmwgfx_rpathdir = ''
>   endif
>   
> diff --git a/tests/meson.build b/tests/meson.build
> index f8a0ab836..b2ec1361d 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -474,6 +474,8 @@ subdir('v3d')
>   
>   subdir('vc4')
>   
> +subdir('vkms')
> +
>   subdir('vmwgfx')
>   
>   gen_testlist = find_program('generate_testlist.sh')
> diff --git a/tests/vkms/meson.build b/tests/vkms/meson.build
> new file mode 100644
> index 000000000..e55ba32ba
> --- /dev/null
> +++ b/tests/vkms/meson.build
> @@ -0,0 +1,13 @@
> +vkms_progs = [
> +	'vkms_configfs',
> +]
> +vkms_deps = test_deps
> +
> +foreach prog : vkms_progs
> +	test_executables += executable(prog, prog + '.c',
> +				       dependencies : vkms_deps,
> +				       install_dir : vkmsdir,
> +				       install_rpath : vkms_rpathdir,
> +				       install : true)
> +	test_list += join_paths('vkms', prog)
> +endforeach
> diff --git a/tests/vkms/vkms_configfs.c b/tests/vkms/vkms_configfs.c
> new file mode 100644
> index 000000000..441c39a47
> --- /dev/null
> +++ b/tests/vkms/vkms_configfs.c
> @@ -0,0 +1,122 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2023 Google LLC.
> + * Copyright © 2023 Collabora, Ltd.
> + * Copyright © 2024 Red Hat, Inc.
> + */
> +
> +/**
> + * TEST: Tests for VKMS configfs support.
> + * Category: Display
> + * Mega feature: General Display Features
> + * Sub-category: uapi
> + * Functionality: vkms,configfs
> + * Test category: functionality test
> + */
> +
> +#include <dirent.h>
> +#include <string.h>
> +
> +#include <linux/limits.h>
> +#include <sys/stat.h>
> +
> +#include "drmtest.h"
> +#include "igt.h"
> +#include "igt_vkms.h"
> +
> +static void assert_default_files(const char *path,
> +				 const char **files, size_t n_files,
> +				 const char **dirs, size_t n_dirs)
> +{
> +	DIR *dir;
> +	struct dirent *ent;
> +	int total = 0;
> +
> +	/* Check that the number of files/directories matches the expected.
> +	 * Plus 2 because of "." and "..".
> +	 */
> +	dir = opendir(path);
> +	igt_assert(dir);
> +	while ((ent = readdir(dir)) != NULL)
> +		total++;

Can we simply skip "." and ".." as you did in 
igt_vkms_destroy_all_devices? This way we don't have this "magic" +2

> +	igt_assert_eq(total, n_dirs + n_files + 2);
> +	closedir(dir);
> +
> +	/* Check that the files/directories are present */
> +	for (int i = 0; i < n_files; i++) {
> +		char file_path[PATH_MAX];
> +		struct stat buf;
> +
> +		snprintf(file_path, sizeof(file_path), "%s/%s", path, files[i]);
> +
> +		igt_assert_eq(stat(file_path, &buf), 0);

Can you transform this igt_assert_eq into:

	igt_assert_f(stat(...) == 0, "File %s does not exists\n", file_path)

So it is easier to know exactly the fail reason

> +	}
> +
> +	for (int i = 0; i < n_dirs; i++) {
> +		char dir_path[PATH_MAX];
> +
> +		snprintf(dir_path, sizeof(dir_path), "%s/%s", path, dirs[i]);

Ditto

Thanks,
Louis Chauvet

> +		dir = opendir(dir_path);
> +		igt_assert(dir);
> +		closedir(dir);
> +	}
> +}
> +
> +/**
> + * SUBTEST: device-default-files
> + * Description: Test that creating a VKMS device creates the default files and
> + *              directories.
> + */
> +
> +static void test_device_default_files(void)
> +{
> +	igt_vkms_t *dev;
> +
> +	const char *files[] = {
> +		"enabled",
> +	};
> +
> +	const char *dirs[] = {
> +		"planes",
> +		"crtcs",
> +		"encoders",
> +		"connectors",
> +	};
> +
> +	dev = igt_vkms_device_create(__func__);
> +	igt_assert(dev);
> +
> +	assert_default_files(dev->path,
> +			     files, ARRAY_SIZE(files),
> +			     dirs, ARRAY_SIZE(dirs));
> +
> +	igt_vkms_device_destroy(dev);
> +}
> +
> +igt_main
> +{
> +	struct {
> +		const char *name;
> +		void (*fn)(void);
> +	} tests[] = {
> +		{ "device-default-files", test_device_default_files },
> +	};
> +
> +	igt_fixture {
> +		igt_require_vkms();
> +		igt_require_vkms_configfs();
> +		igt_vkms_destroy_all_devices();
> +	}
> +
> +	for (int i = 0; i < ARRAY_SIZE(tests); i++) {
> +		igt_subtest(tests[i].name)
> +			tests[i].fn();
> +	}
> +
> +	igt_fixture {
> +		igt_require_vkms();
> +		igt_require_vkms_configfs();
> +		igt_vkms_destroy_all_devices();
> +	}
> +}
> diff --git a/tests/vkms/vkms_test_config.json b/tests/vkms/vkms_test_config.json
> new file mode 100644
> index 000000000..4e84e184a
> --- /dev/null
> +++ b/tests/vkms/vkms_test_config.json
> @@ -0,0 +1,72 @@
> +{
> +    "description": "JSON file to be used to parse VKMS documentation",
> +    "name": "Tests for VKMS Driver",
> +    "drivers": [ "vkms" ],
> +    "files": [ "*.c" ],
> +    "fields": {
> +        "Run type": {
> +            "_properties_": {
> +                "mandatory": true
> +            },
> +            "Category": {
> +                "_properties_": {
> +                    "description": "Contains the major group for the tested functionality 'Display'"
> +                },
> +                "Mega feature": {
> +                    "_properties_": {
> +                        "mandatory": true,
> +                        "description": "Contains the mega feature for end to end use case"
> +                    },
> +                    "Sub-category": {
> +                        "_properties_": {
> +                            "mandatory": true,
> +                            "description": "Contains the technical feature/functionality"
> +                        },
> +                        "Functionality": {
> +                            "_properties_": {
> +                                "mandatory": true,
> +                                "description": "Groups page table tests on buckets containing more detailed functionality"
> +                            },
> +                            "Feature": {
> +                                "_properties_": {
> +                                    "description": "Describes the lowest level feature bucket"
> +                                }
> +                            }
> +                        }
> +                    }
> +                }
> +            }
> +        },
> +        "Test category": {
> +            "_properties_": {
> +                "description": "Defines the test category. Usually used at subtest level."
> +            }
> +        },
> +        "Test requirement": {
> +            "_properties_": {
> +                "description": "Defines Kernel parameters required for the test to run"
> +            }
> +        },
> +        "Issue": {
> +            "_properties_": {
> +                "description": "If the test is used to solve an issue, point to the URL containing the issue."
> +            }
> +        },
> +        "Depends on": {
> +            "_properties_": {
> +                "description": "List other subtests that are required to not be skipped before calling this one."
> +            }
> +        },
> +        "TODO": {
> +            "_properties_": {
> +                "description": "Point to known missing features at the test or subtest."
> +            }
> +        },
> +        "Description": {
> +            "_properties_": {
> +                "mandatory": true,
> +                "description": "Provides a description for the test/subtest."
> +            }
> +        }
> +    }
> +}


-- 
Louis Chauvet, Bootlin
Embedded Linux and Kernel engineering
https://bootlin.com