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

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 22:56:36)
> On Fri, 17 May 2019 23:03:56 +0200, Chris Wilson  
> <chris at chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 21:53:33)
> >> On Fri, 17 May 2019 22:44:53 +0200, Chris Wilson
> >> <chris at 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 at intel.com>
> >> >> Cc: Joonas Lahtinen <joonas.lahtinen at linux.intel.com>
> >> >> Cc: Martin Peres <martin.peres at linux.intel.com>
> >> >> Cc: Chris Wilson <chris at chris-wilson.co.uk>
> >> >> Cc: Tony Ye <tony.ye at 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.
> 
> which, as you said earlier, may not exist

In which case the user is left to their devices to divine what happened,
presumably looking at dmesg. Still looking at why the kernel didn't do
something is much better than guessing.
-Chris