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

[Chris Wilson]

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.

> +}
> +
> +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
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.

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...

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.

As it stands this test fails on all my boxen, which afaict are
functioning perfectly fine.
-Chris