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 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@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 ?
> 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);
>>> +    }
>>> +}