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

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

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
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev