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

[Michal Wajdeczko]

On Mon, 20 May 2019 11:41:12 +0200, Chris Wilson  
<chris at chris-wilson.co.uk> wrote:

> Quoting Michal Wajdeczko (2019-05-19 21:16:34)
>> Add simple test to check that HuC firmware is available.
>> Use existing I915_GETPARAM and debugfs entry.
>>
>> v2: make it even simpler
>> v3: dump with IGT_LOG_INFO, decode status
>>
>> 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>
>> Reviewed-by: Chris Wilson <chris at chris-wilson.co.uk> #v2
>> ---
>>  tests/Makefile.sources |  3 +++
>>  tests/i915/i915_huc.c  | 53 ++++++++++++++++++++++++++++++++++++++++++
>>  tests/meson.build      |  1 +
>>  3 files changed, 57 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..2b4bc0d9
>> --- /dev/null
>> +++ b/tests/i915/i915_huc.c
>> @@ -0,0 +1,53 @@
>> +/* 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("Test HuC firmware");
>> +
>> +static int get_huc_status(int fd)
>> +{
>> +       int status = 0;
>> +       drm_i915_getparam_t gp = {
>> +               .param = I915_PARAM_HUC_STATUS,
>> +               .value = &status,
>> +       };
>> +
>> +       if (igt_ioctl(fd, DRM_IOCTL_I915_GETPARAM, &gp))
>> +               return -errno;
>> +
>> +       errno = 0;
>> +       return status;
>> +}
>> +
>> +igt_main
>> +{
>> +       int fd;
>> +
>> +       igt_fixture {
>> +               int status;
>> +
>> +               fd = drm_open_driver(DRIVER_INTEL);
>> +               igt_require_intel(fd);
>> +               __igt_debugfs_dump(fd, "i915_huc_load_status",  
>> IGT_LOG_INFO);
>> +
>> +               status = get_huc_status(fd);
>> +               igt_skip_on_f(status == -ENODEV,
>> +                             "HuC is not available on this  
>> platform!\n");
>> +               igt_skip_on_f(status < 0, "HuC firmware error: %i,  
>> %s\n",
>> +                             -status, strerror(-status));
>> +               igt_skip_on_f(status == 0, "HuC firmware is not  
>> loaded!\n");

After [1] this could be changed to "HuC firmware is not enabled" and
we can also add:

	igt_skip_on_f(status == -ENOPKG, "HuC firmware is not installed")

[1] https://patchwork.freedesktop.org/patch/305806/?series=60807&rev=2

>
> Yup, that I think is for the best; intelligible error messages even I
> can digest.
>
> (I was thinking a switch, e.g.
> static bool has_addfb2_iface(int fd)
> {
>         struct local_drm_mode_fb_cmd2 f = {};
>         int err;
>
>         err = 0;
>         if (drmIoctl(fd, DRM_IOCTL_MODE_ADDFB2, &f))
>                 err = -errno;
>         switch (err) {
>         case -ENOTTY: /* ioctl unrecognised (kernel too old) */
>         case -ENOTSUP: /* driver doesn't support KMS */
>                 return false;
>
>                 /*
>                  * The only other valid response is -EINVAL, but we leave
>                  * that for the actual tests themselves to discover for
>                  * more accurate reporting.
>                  */
>         default:
>                 return true;
>         }
> }
> igt_require(has_adddb2_iface(fd))
>
> But with sensible error messages, you win ;)
>
> Just need to hold off until we agree on the best error codes.
> -Chris