Re: [igt-dev] [PATCH i-g-t 5/8] tests/sriov_basic: add basic tests for enabling SR-IOV VFs

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 06.11.2023 20:59, Lukasz Laguna wrote:
> From: Katarzyna Dec <katarzyna.dec@intel.com>
> 
> Add subtests that validate SR-IOV VFs enabling in two variants: with
> autoprobe disabled and enabled.
> 
> Signed-off-by: Katarzyna Dec <katarzyna.dec@intel.com>
> Reviewed-by: Lukasz Laguna <lukasz.laguna@intel.com>
> Signed-off-by: Lukasz Laguna <lukasz.laguna@intel.com>
> Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz@linux.intel.com>
> ---
>  tests/meson.build   |   1 +
>  tests/sriov_basic.c | 126 ++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 127 insertions(+)
>  create mode 100644 tests/sriov_basic.c
> 
> diff --git a/tests/meson.build b/tests/meson.build
> index 62721157d..7413d978c 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -71,6 +71,7 @@ test_progs = [
>  	'panfrost_submit',
>  	'prime_udl',
>  	'prime_vgem',
> +	'sriov_basic',
>  	'syncobj_basic',
>  	'syncobj_eventfd',
>  	'syncobj_wait',
> diff --git a/tests/sriov_basic.c b/tests/sriov_basic.c
> new file mode 100644
> index 000000000..fc0914962
> --- /dev/null
> +++ b/tests/sriov_basic.c
> @@ -0,0 +1,126 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2023 Intel Corporation. All rights reserved.
> + */
> +
> +#include "drmtest.h"
> +#include "igt_core.h"
> +#include "igt_sriov_device.h"
> +
> +IGT_TEST_DESCRIPTION("Basic tests for enabling SR-IOV Virtual Functions");
> +
> +/**
> + * TEST: sriov_basic
> + * Category: Software building block
> + * Mega feature: SR-IOV
> + * Sub-category: VFs enabling
> + * Run type: BAT
> + * Description: Validate SR-IOV VFs enabling
> + */
> +
> +/**
> + * SUBTEST: enable-vfs-autoprobe-off
> + * Description:
> + *   Verify VFs enabling without probing VF driver
> + */
> +static void enable_disable_vfs(int pf_fd, unsigned int num_vfs)

	"enable-vfs-autoprobe-off"
and
	"enable_disable_vfs"
are different
shouldn't they match somehow ?

> +{
> +	igt_debug("Using num_vfs=%u\n", num_vfs);
> +
> +	igt_require(igt_sriov_get_enabled_vfs(pf_fd) == 0);

this seems to duplicate first fixture, do we really need to repeat that
over and over ?

> +	igt_assert(igt_sriov_disable_driver_autoprobe(pf_fd));
> +	igt_assert(!igt_sriov_is_driver_autoprobe_enabled(pf_fd));

this seems crazy and unrelated to test scope - we are not checking here
the behavior of the "driver_autoprobe" attribute, we should just trust
that 'disable' above worked since it returned true and we already
asserted that

> +	igt_assert(igt_sriov_enable_vfs(pf_fd, num_vfs));
> +	igt_assert_eq(num_vfs, igt_sriov_get_enabled_vfs(pf_fd));

this should be "expect" type of check, as we still want to disable VFs

> +	igt_assert(igt_sriov_disable_vfs(pf_fd));

maybe assert here that enabled_vfs == num_vfs ?

> +}
> +
> +/**
> + * SUBTEST: enable-vfs-autoprobe-on
> + * Description:
> + *   Verify VFs enabling and auto-probing VF driver
> + */
> +static void probe_disable_vfs(int pf_fd, unsigned int num_vfs)

here is even more different

	"enable-vfs-autoprobe-on"
vs
	"probe_disable_vfs"

also "probe" here may clash with future test that will "probe" just
selected VF

> +{
> +	bool err = false;
> +
> +	igt_debug("Using num_vfs=%u\n", num_vfs);
> +
> +	igt_require(igt_sriov_get_enabled_vfs(pf_fd) == 0);

ditto

> +	igt_assert(igt_sriov_enable_driver_autoprobe(pf_fd));
> +	igt_assert(igt_sriov_is_driver_autoprobe_enabled(pf_fd));

ditto

> +	igt_assert(igt_sriov_enable_vfs(pf_fd, num_vfs));
> +	igt_assert_eq(num_vfs, igt_sriov_get_enabled_vfs(pf_fd));

ditto

> +	for (int vf_num = 1; vf_num <= num_vfs; ++vf_num) {
> +		if (!igt_sriov_is_vf_drm_driver_probed(pf_fd, vf_num)) {
> +			igt_debug("VF%u probe failed\n", vf_num);
> +			err = true;
> +		}
> +	}
> +	igt_assert(igt_sriov_disable_vfs(pf_fd));

disabling VFs immediately after enabling could be treated as a "stress"
test - shouldn't we have some grace period for a "basic" class test ?

stress loop with probe/unload could be different test case

> +	igt_assert(!err);
> +}
> +
> +igt_main
> +{
> +	int pf_fd;
> +	bool autoprobe;
> +
> +	igt_fixture {
> +		pf_fd = drm_open_driver(DRIVER_ANY);
> +		igt_require(igt_sriov_is_pf(pf_fd));
> +		igt_require(igt_sriov_get_enabled_vfs(pf_fd) == 0);
> +		autoprobe = igt_sriov_is_driver_autoprobe_enabled(pf_fd);
> +
> +		igt_srandom();

shouldn't this be part of the main() or something ?

> +	}
> +
> +	igt_describe("Verify VFs enabling without probing VF driver");
> +	igt_subtest_with_dynamic("enable-vfs-autoprobe-off") {
> +		for_each_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-%u", num_vfs) {
> +				enable_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +		for_random_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-random") {
> +				enable_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +		for_max_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-all") {
> +				enable_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +	}
> +
> +	igt_describe("Verify VFs enabling and auto-probing VF driver");
> +	igt_subtest_with_dynamic("enable-vfs-autoprobe-on") {
> +		for_each_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-%u", num_vfs) {
> +				probe_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +		for_random_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-random") {
> +				probe_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +		for_max_num_vfs(pf_fd, num_vfs) {
> +			igt_dynamic_f("numvfs-all") {
> +				probe_disable_vfs(pf_fd, num_vfs);
> +			}
> +		}
> +	}
> +
> +	igt_fixture {
> +		igt_sriov_disable_vfs(pf_fd);
> +		/* abort to avoid execution of next tests with enabled VFs */
> +		igt_abort_on_f(igt_sriov_get_enabled_vfs(pf_fd) > 0, "Failed to disable VF(s)");

can't this be just:

	igt_abort_on_f(!igt_sriov_disable_vfs(pf_fd), "");
	igt_abort_on_f(!igt_sriov_set_driver_autoprobe(autoprobe), "");

> +		autoprobe ? igt_sriov_enable_driver_autoprobe(pf_fd) :
> +			    igt_sriov_disable_driver_autoprobe(pf_fd);
> +		igt_abort_on_f(autoprobe != igt_sriov_is_driver_autoprobe_enabled(pf_fd),
> +			       "Failed to restore sriov_drivers_autoprobe value\n");
> +		close(pf_fd);
> +	}
> +}