[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

Also note the kernel is already giving us the has_huc infomation in the
errno, so if you want to be informative, decode the error return.
-Chris