[igt-dev] [PATCH i-g-t 5/8] tests/sriov_basic: add basic tests for enabling SR-IOV VFs
Michal Wajdeczko
michal.wajdeczko at intel.com
Fri Nov 10 19:37:07 UTC 2023
On 09.11.2023 08:04, Laguna, Lukasz wrote:
>
> On 11/6/2023 23:46, Michal Wajdeczko wrote:
>>
>> On 06.11.2023 20:59, Lukasz Laguna wrote:
>>> From: Katarzyna Dec <katarzyna.dec at 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 at intel.com>
>>> Reviewed-by: Lukasz Laguna <lukasz.laguna at intel.com>
>>> Signed-off-by: Lukasz Laguna <lukasz.laguna at intel.com>
>>> Reviewed-by: Marcin Bernatowicz <marcin.bernatowicz at 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 ?
> Done
>>
>>> +{
>>> + 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 ?
> It's not the same. First fixtureis not executed between dynamic subtests.
hmm, I'm not an igt expert, but this seems to be little broken
>>
>>> + 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
> Done
>>
>>> + 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
> VFs will be disabled in exit fixture. VFs disabling in subtest is needed
> between dynamic subtests.
>>
>>> + igt_assert(igt_sriov_disable_vfs(pf_fd));
>> maybe assert here that enabled_vfs == num_vfs ?
> Some time ago we've got a sugesstion that we should have seperate test
> for VFs disabling. We can check
> igt_assert_eq(0, igt_sriov_get_enabled_vfs(pf_fd));
> there, when implemented.
>>
>>> +}
>>> +
>>> +/**
>>> + * 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
> Done
>>
>>> +{
>>> + 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 ?
> I can add some sleep before VFs disabling. Do you have some specific
> value we should use in mind? 2s?
dunno
but I still doubt that enabling all VFs in autoprobe mode is a good test
for "basic" scenario (the only argument for being 'basic' is that is is
1-liner from test point-of-view, but definitely it is not 'basic' from
the system and driver POV)
in basic tests we should just try enable 1 VF at the time, unload it,
then try with next one
"autoprobe all" shouldn't be "Run type: BAT"
>> stress loop with probe/unload could be different test case
> Yeah, it's in another patch from this series
>>
>>> + 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 ?
> Probably it could be, but no one has implemented it yet. There are many
> other tests that initializes seed in fixture.
but why follow bad design/pattern ?
>>> + }
>>> +
>>> + 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), "");
> It's for case when helper e.g. igt_sriov_disable_vfsdoesn't return
> error, but VFs are still enabled.
but do we care here ?
I'm not sure that we should add test code to test other test code, as
then you will just write code and miss what was the original goal of the
test.
>>> + 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);
>>> + }
>>> +}
More information about the igt-dev
mailing list