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

[Chris Wilson]

Quoting Michal Wajdeczko (2019-05-17 19:47:42)
> On Fri, 17 May 2019 19:48:55 +0200, Chris Wilson  
> <chris at chris-wilson.co.uk> wrote:
> 
> > Quoting Michal Wajdeczko (2019-05-17 18:37:53)
> >> Add simple test to check that HuC firmware is available.
> >> Use existing I915_GETPARAM and debugfs entry.
> >>
> >> 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  | 74 ++++++++++++++++++++++++++++++++++++++++++
> >>  tests/meson.build      |  1 +
> >>  3 files changed, 78 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..10cd5d6f
> >> --- /dev/null
> >> +++ b/tests/i915/i915_huc.c
> >> @@ -0,0 +1,74 @@
> >> +/* SPDX-License-Identifier: MIT */
> >> +/*
> >> + * Copyright © 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 bool has_guc(int fd)
> >> +{
> >> +       uint32_t devid = intel_get_drm_devid(fd);
> >> +
> >> +       return IS_SKYLAKE(devid) || IS_BROXTON(devid) ||
> >> +              IS_KABYLAKE(devid) || IS_COFFEELAKE(devid) ||
> >> +              IS_GEMINILAKE(devid) || IS_ICELAKE(devid);
> >> +}
> >> +
> >> +static bool has_huc(int fd)
> >> +{
> >> +       return has_guc(fd);
> >> +}
> >> +
> >> +static void check_huc_status(int device)
> >> +{
> >> +       int loaded = 0;
> >> +       drm_i915_getparam_t gp = {
> >> +               .param = I915_PARAM_HUC_STATUS,
> >> +               .value = &loaded,
> >> +       };
> >> +
> >> +       igt_require(ioctl(device, DRM_IOCTL_I915_GETPARAM, &gp) == 0);
> >> +       igt_assert_eq(loaded, 1);
> >> +}
> >> +
> >> +#define HUC_STATUS_DEBUGFS_FILE "i915_huc_load_status"
> >> +
> >> +static void check_huc_info(int device)
> >> +{
> >> +       int fd = igt_debugfs_open(device, HUC_STATUS_DEBUGFS_FILE,  
> >> O_RDONLY);
> >> +       igt_assert_f(fd >= 0, "'%s' debugfs entry not found\n",
> >> +                    HUC_STATUS_DEBUGFS_FILE);
> >
> > Debugfs may not exist...
> >
> >> +       close(fd);
> >> +
> >> +       igt_assert(igt_debugfs_search(device, HUC_STATUS_DEBUGFS_FILE,
> >> +                  "status: fetch SUCCESS"));
> >> +       igt_assert(igt_debugfs_search(device, HUC_STATUS_DEBUGFS_FILE,
> >> +                  "load SUCCESS"));
> >
> > and doesn't constitute ABI, so asserts are little ott.
> 
> btw, IGT code seems to be little inconsistent on that

igt relies on debugfs for sure, (often too much, and we use debugfs as a
crutch to avoid a real API with stable ABI, imo)

But in terms of an igt determining the expected behaviour of the kernel
for userspace and defining the ABI, this subtest paints the picture to
me that HUC_STATUS_DEBUGFS_FILE is defacto ABI. That I want to avoid.

It will certainly be useful to dump the debugfs file at some point, and
just dumping it into IGT_INFO may be nice as part of the fixture.
 
> (and based on below discussion, I inclined to drop this subtest)
> 
> >
> >> +}
> >> +
> >> +igt_main
> >> +{
> >> +       int fd;
> >> +
> >> +       igt_fixture {
> >> +               fd = drm_open_driver(DRIVER_INTEL);
> >> +               igt_require_intel(fd);
> >> +               igt_skip_on_f(!has_huc(fd),
> >> +                             "HuC is not available on this  
> >> platform\n");
> >
> > Hmm, HuC is optional as it may both be disabled by [default] parameter
> 
> afaik, this is about to change soon ;)

Apparently they have some blocking bugs to resolve first :-p

> > and lack of fw [a plague on us who use builtin modules]. I think this
> > should be igt_require(huc_status(fd)) -- skips will be highlighted if we
> > regress.
> 
> I'm not igt expert, so to confirm, maybe subtest should be empty and
> we should put all require stuff into fixture ?
> 
> igt_fixture {
>         igt_require_intel(fd);
>         igt_require(has_huc(fd));
>         igt_require(huc_status(fd));
> }
> 
> igt_subtest("basic")
>         igt_success();

Yup. That covers the simplest test we can do, SKIP <->

> > And for the main test we would much rather have something behaviour that
> > only the HuC can provide. Is there not any challenge-response we can
> > send to the gpu for a simple ping? I have no idea what function the HuC
> > serves...
> 
> The goal if this patch was to provide 'simple' test that covers what we
> expose today (at least GETPARAM is ABI, right?)

Yup. That appears to be used.

> > There's https://bugs.freedesktop.org/show_bug.cgi?id=110617 is we need
> > an example where loading the HuC breaks previously working video
> > playback.
> 
> For above bug, maybe HuC fw binary was corrupted? anyone tried  
> enable_guc=3?

I thought the fw were signed? Wouldn't that prevent execution of a
corrupted binary? (And if they are not signed, what's stopping us from
uploading our own...)

I was hoping maybe it would be an insight as to something we could use
to determine the HuC exists.
-Chris