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

Michal Wajdeczko
2019-05-17T20:53:37Z

On Fri, 17 May 2019 22:44:53 +0200, Chris Wilson
<chris@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@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 | 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

Test requirement not met
Test requirement: huc_status(fd) > 0

means HuC firmware was not loaded

>
>> + igt_debugfs_dump(fd, "i915_huc_load_status");
>> + igt_require(huc_status(fd) > 0);
>
> As we get the debugfs and HUC_STATUS.

debugfs is mainly for fw version which may be changing,
above requirements are likely easier to add as filters to CI

>
>> + }
>> +
>> + igt_subtest("basic")
>> + igt_success();
>> +
>> + igt_fixture {
>> + close(fd);
>
> Worksforme as a starting point,
> Reviewed-by: Chris Wilson <chris@chris-wilson.co.uk>
> (preferably without the extra igt_require(has_huc))

please keep

>
> I trust that anyone who contemplates demanding that HuC be enabled by
> default follows up with an actual test to prove the HuC is functional
> and we don't accidentally break it.
> -Chris