From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mgamail.intel.com (mgamail.intel.com [198.175.65.9]) by gabe.freedesktop.org (Postfix) with ESMTPS id BF4E010E454 for ; Mon, 6 Nov 2023 22:46:21 +0000 (UTC) Message-ID: <0a0e6099-9fab-4906-ba18-81c615867ce1@intel.com> Date: Mon, 6 Nov 2023 23:46:17 +0100 MIME-Version: 1.0 Content-Language: en-US To: Lukasz Laguna , igt-dev@lists.freedesktop.org References: <20231106195947.14640-1-lukasz.laguna@intel.com> <20231106195947.14640-6-lukasz.laguna@intel.com> From: Michal Wajdeczko In-Reply-To: <20231106195947.14640-6-lukasz.laguna@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit Subject: Re: [igt-dev] [PATCH i-g-t 5/8] tests/sriov_basic: add basic tests for enabling SR-IOV VFs List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: igt-dev-bounces@lists.freedesktop.org Sender: "igt-dev" List-ID: On 06.11.2023 20:59, Lukasz Laguna wrote: > From: Katarzyna Dec > > Add subtests that validate SR-IOV VFs enabling in two variants: with > autoprobe disabled and enabled. > > Signed-off-by: Katarzyna Dec > Reviewed-by: Lukasz Laguna > Signed-off-by: Lukasz Laguna > Reviewed-by: Marcin Bernatowicz > --- > 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); > + } > +}