Re: [igt-dev] [PATCH i-g-t v5] tests/i915/gem_huc_copy: Enable a HuC copy test

["Argenziano, Antonio" <antonio.argenziano@intel.com>]

On 4/14/2020 2:24 PM, Robert M. Fosha wrote:
> From: Sally Qi <feng.qi@intel.com>
>
> This test case loads the HuC copy firmware to copy the content of
> the source buffer to the destination buffer.
>
> v2: (Tony Ye)
>   * Restructured some functions and files.
>   * Defined the copy buffer size as 4K explicitly as the HuC Copy kernel
>     always copy 4K bytes from src buffer to dst buffer.
>
> v3: (Feng Qi, Antonio Argenziano, Tony Ye)
>   * Restructured some functions as igt requested, exclude libdrm function call.
>   * Remove huc function wrappers
>   * Random initialize source input buffer
>
> v4: (Robert Fosha)
>   * Fix autotools build failure.
>
> v5: (Feng Qi, Tony Ye)
>   * Released all bo buffer after huc copying.
>   * Restructured huc_copy() function.
>
> v6: (Feng Qi)
>   * Fixed the function of huc enabling and status check
>   * Added huc_copy to fast feedback testlist
>
> v7: (Tony Ye, Feng Qi, Robert Fosha, Chris Wilson, Michal Wajdeczko)
>   * Check error with HUC_STATUS ioctl instead of debugfs
>
> Signed-off-by: Feng Qi <feng.qi@intel.com>
> Signed-off-by: Tony Ye <tony.ye@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Chris Wilson <chris@chris-wilson.co.uk>
> Acked-by: Antonio Argenziano <antonio.argenziano@intel.com>
> ---
>   lib/Makefile.sources                  |   2 +
>   lib/huc_copy.c                        | 107 +++++++++++++++++++++
>   lib/huc_copy.h                        |  50 ++++++++++
>   lib/intel_batchbuffer.c               |  20 ++++
>   lib/intel_batchbuffer.h               |  20 ++++
>   lib/meson.build                       |   1 +
>   tests/Makefile.sources                |   3 +
>   tests/i915/gem_huc_copy.c             | 130 ++++++++++++++++++++++++++
>   tests/intel-ci/fast-feedback.testlist |   1 +
>   tests/meson.build                     |   1 +
>   10 files changed, 335 insertions(+)
>   create mode 100644 lib/huc_copy.c
>   create mode 100644 lib/huc_copy.h
>   create mode 100644 tests/i915/gem_huc_copy.c
>
> diff --git a/lib/Makefile.sources b/lib/Makefile.sources
> index 1e2c88ae..a882b0e7 100644
> --- a/lib/Makefile.sources
> +++ b/lib/Makefile.sources
> @@ -84,6 +84,8 @@ lib_source_list =	 	\
>   	ioctl_wrappers.h	\
>   	media_fill.c		\
>   	media_fill.h            \
> +	huc_copy.c		\
> +	huc_copy.h		\
>   	media_spin.h		\
>   	media_spin.c		\
>   	gpgpu_fill.h		\
> diff --git a/lib/huc_copy.c b/lib/huc_copy.c
> new file mode 100644
> index 00000000..c3dece9f
> --- /dev/null
> +++ b/lib/huc_copy.c
> @@ -0,0 +1,107 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#include <i915_drm.h>
> +#include "huc_copy.h"
> +
> +static void
> +gen9_emit_huc_virtual_addr_state(struct drm_i915_gem_exec_object2 *src,
> +		struct drm_i915_gem_exec_object2 *dst,
> +		struct drm_i915_gem_relocation_entry *reloc_src,
> +		struct drm_i915_gem_relocation_entry *reloc_dst,
> +		uint32_t *buf,
> +		int *i)
> +{
> +	buf[(*i)++] = HUC_VIRTUAL_ADDR_STATE;
> +
> +	for (int j = 0; j < HUC_VIRTUAL_ADDR_REGION_NUM; j++) {
> +		if (j == HUC_VIRTUAL_ADDR_REGION_SRC) {
> +			buf[(*i)++] = src->offset;
> +
> +			reloc_src->target_handle = src->handle;
> +			reloc_src->delta = 0;
> +			reloc_src->offset = (*i - 1) * sizeof(buf[0]);
> +			reloc_src->read_domains = 0;
> +			reloc_src->write_domain = 0;
> +		} else if (j == HUC_VIRTUAL_ADDR_REGION_DST) {
> +			buf[(*i)++] = dst->offset;
> +
> +			reloc_dst->target_handle = dst->handle;
> +			reloc_dst->delta = 0;
> +			reloc_dst->offset = (*i - 1) * sizeof(buf[0]);
> +			reloc_dst->read_domains = 0;
> +			reloc_dst->write_domain = I915_GEM_DOMAIN_RENDER;
> +		} else {
> +			buf[(*i)++] = 0;
> +		}
> +		buf[(*i)++] = 0;
> +		buf[(*i)++] = 0;
> +	}
> +}
> +
> +void
> +gen9_huc_copyfunc(int fd,
> +		struct drm_i915_gem_exec_object2 *obj,
> +		int n_bo)


Not sure you really need 'n_bo', it is always assumed to be 3 no matter 
what. If you really want to keep it I think you should check it == 3.


> +{
> +	struct drm_i915_gem_relocation_entry reloc[2];
> +	struct drm_i915_gem_execbuffer2 execbuf;
> +	int i = 0;
> +	uint32_t buf[63];
> +
> +	/* load huc kernel */
> +	buf[i++] = HUC_IMEM_STATE;
> +	buf[i++] = 0;
> +	buf[i++] = 0;
> +	buf[i++] = 0;
> +	buf[i++] = 0x3;
> +
> +	buf[i++] = MFX_WAIT;
> +	buf[i++] = MFX_WAIT;
> +
> +	buf[i++] = HUC_PIPE_MODE_SELECT;
> +	buf[i++] = 0;
> +	buf[i++] = 0;
> +
> +	buf[i++] = MFX_WAIT;
> +
> +	memset(reloc, 0, sizeof(reloc));
> +	gen9_emit_huc_virtual_addr_state(&obj[0], &obj[1], &reloc[0], &reloc[1], buf, &i);
> +
> +	buf[i++] = HUC_START;
> +	buf[i++] = 1;
> +
> +	buf[i++] = MI_BATCH_BUFFER_END;
> +
> +	gem_write(fd, obj[2].handle, 0, buf, sizeof(buf));
> +	obj[2].relocation_count = 2;
> +	obj[2].relocs_ptr = to_user_pointer(reloc);
> +
> +	memset(&execbuf, 0, sizeof(execbuf));
> +	execbuf.buffers_ptr = to_user_pointer(obj);
> +	execbuf.buffer_count = n_bo;
> +	execbuf.flags = I915_EXEC_BSD;
> +
> +	gem_execbuf(fd, &execbuf);
> +}
> diff --git a/lib/huc_copy.h b/lib/huc_copy.h
> new file mode 100644
> index 00000000..088f7954
> --- /dev/null
> +++ b/lib/huc_copy.h
> @@ -0,0 +1,50 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + *
> + */
> +
> +#ifndef HUC_COPY_H
> +#define HUC_COPY_H
> +
> +#include <stdint.h>
> +#include <string.h>
> +#include "ioctl_wrappers.h"
> +#include "intel_reg.h"
> +
> +#define PARALLEL_VIDEO_PIPE		(0x3<<29)
> +#define MFX_WAIT			(PARALLEL_VIDEO_PIPE|(0x1<<27)|(0x1<<8))
> +
> +#define HUC_IMEM_STATE			(PARALLEL_VIDEO_PIPE|(0x2<<27)|(0xb<<23)|(0x1<<16)|0x3)
> +#define HUC_PIPE_MODE_SELECT		(PARALLEL_VIDEO_PIPE|(0x2<<27)|(0xb<<23)|0x1)
> +#define HUC_START			(PARALLEL_VIDEO_PIPE|(0x2<<27)|(0xb<<23)|(0x21<<16))
> +#define HUC_VIRTUAL_ADDR_STATE		(PARALLEL_VIDEO_PIPE|(0x2<<27)|(0xb<<23)|(0x4<<16)|0x2f)
> +
> +#define HUC_VIRTUAL_ADDR_REGION_NUM	16
> +#define HUC_VIRTUAL_ADDR_REGION_SRC	0
> +#define HUC_VIRTUAL_ADDR_REGION_DST	14
> +
> +void
> +gen9_huc_copyfunc(int fd,
> +		struct drm_i915_gem_exec_object2 *obj,
> +		int n_bo);
> +
> +#endif /* HUC_COPY_H */
> diff --git a/lib/intel_batchbuffer.c b/lib/intel_batchbuffer.c
> index f1a45b47..93ac4546 100644
> --- a/lib/intel_batchbuffer.c
> +++ b/lib/intel_batchbuffer.c
> @@ -45,6 +45,7 @@
>   #include "gpgpu_fill.h"
>   #include "igt_aux.h"
>   #include "i830_reg.h"
> +#include "huc_copy.h"
>   
>   #include <i915_drm.h>
>   
> @@ -1171,3 +1172,22 @@ igt_media_spinfunc_t igt_get_media_spinfunc(int devid)
>   
>   	return spin;
>   }
> +
> +/**
> + * igt_get_huc_copyfunc:
> + * @devid: pci device id
> + *
> + * Returns:
> + *
> + * The platform-specific huc copy function pointer for the device specified
> + * with @devid. Will return NULL when no media spin function is implemented.
> + */
> +igt_huc_copyfunc_t igt_get_huc_copyfunc(int devid)
> +{
> +	igt_huc_copyfunc_t copy = NULL;
> +
> +	if (IS_GEN12(devid) || IS_GEN11(devid) || IS_GEN9(devid))
> +		copy = gen9_huc_copyfunc;
> +
> +	return copy;
> +}
> diff --git a/lib/intel_batchbuffer.h b/lib/intel_batchbuffer.h
> index 442f3a18..a3d4ec36 100644
> --- a/lib/intel_batchbuffer.h
> +++ b/lib/intel_batchbuffer.h
> @@ -422,4 +422,24 @@ typedef void (*igt_media_spinfunc_t)(struct intel_batchbuffer *batch,
>   
>   igt_media_spinfunc_t igt_get_media_spinfunc(int devid);
>   
> +/**
> + * igt_huc_copyfunc_t:
> + * @fd: drm fd
> + * @obj: drm_i915_gem_exec_object2 buffer array
> + *       obj[0] is source buffer
> + *       obj[1] is destination buffer
> + *       obj[2] is execution buffer
> + * @n_bo: the size of @obj array
> + *
> + * This is the type of the per-platform huc copy functions.
> + *
> + * The huc copy function emits a batchbuffer to the VDBOX engine to
> + * invoke the HuC Copy kernel to copy 4K bytes from the source buffer
> + * to the destination buffer.
> + */
> +typedef void (*igt_huc_copyfunc_t)(int fd,
> +		struct drm_i915_gem_exec_object2 *obj,
> +		int n_bo);
> +
> +igt_huc_copyfunc_t	igt_get_huc_copyfunc(int devid);
>   #endif
> diff --git a/lib/meson.build b/lib/meson.build
> index e2060430..3f8f42f4 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -1,5 +1,6 @@
>   lib_sources = [
>   	'drmtest.c',
> +	'huc_copy.c',
>   	'i915/gem_context.c',
>   	'i915/gem_engine_topology.c',
>   	'i915/gem_scheduler.c',
> diff --git a/tests/Makefile.sources b/tests/Makefile.sources
> index 4e44c98c..ec94d25b 100644
> --- a/tests/Makefile.sources
> +++ b/tests/Makefile.sources
> @@ -314,6 +314,9 @@ gem_media_fill_SOURCES = i915/gem_media_fill.c
>   TESTS_progs += gem_media_vme
>   gem_media_vme_SOURCES = i915/gem_media_vme.c
>   
> +TESTS_progs += gem_huc_copy
> +gem_huc_copy_SOURCES = i915/gem_huc_copy.c
> +
>   TESTS_progs += gem_mmap
>   gem_mmap_SOURCES = i915/gem_mmap.c
>   
> diff --git a/tests/i915/gem_huc_copy.c b/tests/i915/gem_huc_copy.c
> new file mode 100644
> index 00000000..06a9129b
> --- /dev/null
> +++ b/tests/i915/gem_huc_copy.c
> @@ -0,0 +1,130 @@
> +/*
> + * Copyright © 2019 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +#include "igt.h"
> +#include <stdbool.h>
> +#include <unistd.h>
> +#include <stdlib.h>
> +#include <sys/ioctl.h>
> +#include <stdio.h>
> +#include <string.h>
> +#include <fcntl.h>
> +#include <inttypes.h>
> +#include <errno.h>
> +#include <sys/stat.h>
> +#include <sys/time.h>
> +#include "drm.h"
> +#include "igt_sysfs.c"
> +
> +IGT_TEST_DESCRIPTION("A very simple workload for the HuC.");
> +
> +#define HUC_COPY_DATA_BUF_SIZE	4096
> +
> +static void
> +compare_huc_copy_result(int drm_fd, uint32_t src_handle, uint32_t dst_handle)
> +{
> +	char src_output[HUC_COPY_DATA_BUF_SIZE];
> +	char dst_output[HUC_COPY_DATA_BUF_SIZE];
> +
> +	gem_read(drm_fd, src_handle, 0, src_output, HUC_COPY_DATA_BUF_SIZE);
> +	gem_read(drm_fd, dst_handle, 0, dst_output, HUC_COPY_DATA_BUF_SIZE);
> +
> +	for (int i = 0; i < HUC_COPY_DATA_BUF_SIZE; i++)
> +		igt_assert_f(src_output[i] == dst_output[i],
> +			     "Exepected %c, found %c at %4d.\n",
> +			     src_output[i], dst_output[i], i);
> +}
> +
> +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 drm_fd = -1;
> +	uint32_t devid;
> +	igt_huc_copyfunc_t huc_copy;
> +
> +	igt_fixture {
> +		int status;
> +
> +		drm_fd = drm_open_driver(DRIVER_INTEL);
> +		igt_require_gem(drm_fd);
> +		devid = intel_get_drm_devid(drm_fd);
> +		huc_copy = igt_get_huc_copyfunc(devid);
> +
> +		igt_require_f(huc_copy, "no huc_copy function\n");
> +		status = get_huc_status(drm_fd);
> +		igt_skip_on_f(status == -ENODEV,
> +			      "HuC is not present on this platform!\n");
> +		igt_skip_on_f(status == -EOPNOTSUPP,
> +			      "HuC firmware is disabled!\n");
> +		igt_fail_on_f(status < 0, "HuC firmware loading error: %i, %s\n",
> +			      -status, strerror(-status));
> +		igt_fail_on_f(status == 0, "HuC firmware is not running!\n");


Couple of things here:

1. Not convinced that failing in the fixture is ever a good idea, it 
will cause a lot of failures but possibly be linked to one single failure.

2. Is the value of status an error condition? Why is that? I would 
expect a HuC loading test to tell me in what conditions it expects the 
firmware to be loaded and running and when it expect it not to be loaded.


Antonio


> +	}
> +
> +	igt_describe("Make sure that Huc firmware works"
> +		     "by copying a char array using Huc"
> +		     "and verifying the copied result");
> +
> +	igt_subtest("huc-copy") {
> +		char inputs[HUC_COPY_DATA_BUF_SIZE];
> +		struct drm_i915_gem_exec_object2 obj[3];
> +
> +		/* Initialize src buffer randomly */
> +		srand(time(NULL));
> +		for (int i = 0; i < HUC_COPY_DATA_BUF_SIZE; i++)
> +			inputs[i] = (char) (rand() % 256);
> +
> +		memset(obj, 0, sizeof(obj));
> +		/* source buffer object for storing input */
> +		obj[0].handle = gem_create(drm_fd, HUC_COPY_DATA_BUF_SIZE);
> +		/* destination buffer object to receive input */
> +		obj[1].handle = gem_create(drm_fd, HUC_COPY_DATA_BUF_SIZE);
> +		/* execution buffer object */
> +		obj[2].handle = gem_create(drm_fd, 4096);
> +
> +		gem_write(drm_fd, obj[0].handle, 0, inputs, HUC_COPY_DATA_BUF_SIZE);
> +
> +		huc_copy(drm_fd, obj, 3);
> +		compare_huc_copy_result(drm_fd, obj[0].handle, obj[1].handle);
> +
> +		gem_close(drm_fd, obj[0].handle);
> +		gem_close(drm_fd, obj[1].handle);
> +		gem_close(drm_fd, obj[2].handle);
> +	}
> +
> +	igt_fixture
> +		close(drm_fd);
> +}
> diff --git a/tests/intel-ci/fast-feedback.testlist b/tests/intel-ci/fast-feedback.testlist
> index 2ccad438..f332d8ec 100644
> --- a/tests/intel-ci/fast-feedback.testlist
> +++ b/tests/intel-ci/fast-feedback.testlist
> @@ -30,6 +30,7 @@ igt@gem_flink_basic@bad-open
>   igt@gem_flink_basic@basic
>   igt@gem_flink_basic@double-flink
>   igt@gem_flink_basic@flink-lifetime
> +igt@gem_huc_copy@huc_copy
>   igt@gem_linear_blits@basic
>   igt@gem_mmap@basic
>   igt@gem_mmap_gtt@basic
> diff --git a/tests/meson.build b/tests/meson.build
> index e882f4dc..a4934a85 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -163,6 +163,7 @@ i915_progs = [
>   	'gem_gtt_cpu_tlb',
>   	'gem_gtt_hog',
>   	'gem_gtt_speed',
> +	'gem_huc_copy',
>   	'gem_linear_blits',
>   	'gem_lut_handle',
>   	'gem_madvise',
_______________________________________________
igt-dev mailing list
igt-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/igt-dev