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

["Michal Wajdeczko" <michal.wajdeczko@intel.com>]

On Fri, 17 May 2019 19:48:55 +0200, Chris Wilson  
<chris@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@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  | 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

(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 ;)

> 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();

>
> 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?)

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

>
> As it stands this test fails on all my boxen, which afaict are
> functioning perfectly fine.
> -Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev