Re: [igt-dev] [PATCH i-g-t v2] tests/i915: Add simple test for HuC

Chris Wilson
2019-05-17T21:04:10Z

Quoting Michal Wajdeczko (2019-05-17 21:53:33)
> On Fri, 17 May 2019 22:44:53 +0200, Chris Wilson
> <chris@chris-wilson.co.uk> wrote:
>
> > Quoting Michal Wajdeczko (2019-05-17 21:21:54)
> >> Add simple test to check that HuC firmware is available.
> >> Use existing I915_GETPARAM and debugfs entry.
> >>
> >> v2: make it even simpler
> >>
> >> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> >> Cc: Joonas Lahtinen <joonas.lahtinen@linux.intel.com>
> >> Cc: Martin Peres <martin.peres@linux.intel.com>
> >> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> >> Cc: Tony Ye <tony.ye@intel.com>
> >> ---
> >> tests/Makefile.sources | 3 +++
> >> tests/i915/i915_huc.c | 59 ++++++++++++++++++++++++++++++++++++++++++
> >> tests/meson.build | 1 +
> >> 3 files changed, 63 insertions(+)
> >> create mode 100644 tests/i915/i915_huc.c
> >>
> >> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> >> index 7f921f6c..dfa3fcd3 100644
> >> --- a/tests/Makefile.sources
> >> +++ b/tests/Makefile.sources
> >> @@ -475,6 +475,9 @@ i915_getparams_basic_SOURCES =
> >> i915/i915_getparams_basic.c
> >> TESTS_progs += i915_hangman
> >> i915_hangman_SOURCES = i915/i915_hangman.c
> >>
> >> +TESTS_progs += i915_huc
> >> +i915_huc_SOURCES = i915/i915_huc.c
> >> +
> >> TESTS_progs += i915_module_load
> >> i915_module_load_SOURCES = i915/i915_module_load.c
> >>
> >> diff --git a/tests/i915/i915_huc.c b/tests/i915/i915_huc.c
> >> new file mode 100644
> >> index 00000000..e4e6c532
> >> --- /dev/null
> >> +++ b/tests/i915/i915_huc.c
> >> @@ -0,0 +1,59 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright (c) 2019 Intel Corporation
> >> + */
> >> +
> >> +#include "igt.h"
> >> +#include <fcntl.h>
> >> +#include <i915_drm.h>
> >> +#include <sys/ioctl.h>
> >> +
> >> +IGT_TEST_DESCRIPTION("Check that HuC firmware is available.");
> >> +
> >> +static int huc_status(int fd)
> >> +{
> >> + int loaded = 0;
> >> + drm_i915_getparam_t gp = {
> >> + .param = I915_PARAM_HUC_STATUS,
> >> + .value = &loaded,
> >> + };
> >> +
> >> + if (ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
> >> + return -errno;
> >
> > (mutters about errno upsetting igt_assert and making a mess)
>
> maybe better to return -1 ?
>
> >
> >> +
> >> + return loaded;
> >> +}
> >> +
> >> +igt_main
> >> +{
> >> + int fd;
> >> +
> >> + igt_fixture {
> >> + fd = drm_open_driver(DRIVER_INTEL);
> >> + igt_require_intel(fd);
> >> + igt_require(has_huc(fd));
> >
> > This is now redundant...
>
> not quite, as diagnostic message will be different, compare:
>
> Test requirement not met
> Test requirement: has_huc(fd)
>
> means lack of HuC hardware, vs

But why does this test dictate on what HW we expect HuC? Did not cnl
ship a similar uc, just no fw released?

I do not accept this igt as being the better source of truth than the
kernel.

> Test requirement not met
> Test requirement: huc_status(fd) > 0
>
> means HuC firmware was not loaded

Without us guessing why, as the explanation should be in the debugfs.

> >> + igt_debugfs_dump(fd, "i915_huc_load_status");
> >> + igt_require(huc_status(fd) > 0);
> >
> > As we get the debugfs and HUC_STATUS.
>
> debugfs is mainly for fw version which may be changing,
> above requirements are likely easier to add as filters to CI

Fwiw, make that __igt_debugfs_dump(..., IGT_INFO).
-Chris