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 23:03:56 +0200, Chris Wilson  
<chris@chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-17 21:53:33)
>> 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
>
> But why does this test dictate on what HW we expect HuC? Did not cnl
> ship a similar uc, just no fw released?
>
> I do not accept this igt as being the better source of truth than the
> kernel.
>
>>         Test requirement not met
>>         Test requirement: huc_status(fd) > 0
>>
>> means HuC firmware was not loaded
>
> Without us guessing why, as the explanation should be in the debugfs.

which, as you said earlier, may not exist

>
>> >> +               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
>
> Fwiw, make that __igt_debugfs_dump(..., IGT_INFO).
> -Chris
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev