Please see my comments inline below.
Regards,
Zhanjun Dong
On 2025-04-24 1:36 p.m., Peter Senna Tschudin wrote:
> There were two similar implementations of test_legacy_mode(), located in
> tests/intel/xe_exec_capture and tests/intel/xe_exec_reset. This patch
> consolidates them by moving the more complete version from xe_exec_reset
> to lib/xe/xe_legacy, and updates call sites on both tests to use the
> shared function.
>
> The version from xe_exec_reset.c was chosen because it is more
> feature-complete and flexible, offering the following advantages:
> - Supports CLOSE_FD
> - Supports GT reset
> - Waits for spinner start
> - Checks batch buffer result
> - Conditional bind call using xe_vm_bind_async() instead of
> __xe_vm_bind_assert()
> - Allows early return
>
> To cover for a difference between the two function definitions, the
> shared function was extended with the use_capture_mode parameter to
> control whether to use __xe_vm_bind_assert (capture mode) or
> xe_vm_bind_async (legacy mode).
>
> Cc: zhanjun.dong at intel.com
> Signed-off-by: Peter Senna Tschudin <peter.senna at linux.intel.com>
> ---
> lib/meson.build | 1 +
> lib/xe/xe_legacy.c | 167 ++++++++++++++++++++++++++++++++++
> lib/xe/xe_legacy.h | 15 +++
> tests/intel/xe_exec_capture.c | 109 +---------------------
> tests/intel/xe_exec_reset.c | 153 +++----------------------------
> 5 files changed, 201 insertions(+), 244 deletions(-)
> create mode 100644 lib/xe/xe_legacy.c
> create mode 100644 lib/xe/xe_legacy.h
>
> diff --git a/lib/meson.build b/lib/meson.build
> index 8517cd540..454dcd244 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -114,6 +114,7 @@ lib_sources = [
> 'igt_hook.c',
> 'xe/xe_gt.c',
> 'xe/xe_ioctl.c',
> + 'xe/xe_legacy.c',
> 'xe/xe_mmio.c',
> 'xe/xe_query.c',
> 'xe/xe_spin.c',
> diff --git a/lib/xe/xe_legacy.c b/lib/xe/xe_legacy.c
> new file mode 100644
> index 000000000..81a9f3bd6
> --- /dev/null
> +++ b/lib/xe/xe_legacy.c
> @@ -0,0 +1,167 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + */
> +
> +#include "lib/igt_syncobj.h"
> +#include "linux_scaffold.h"
> +#include "xe/xe_ioctl.h"
> +#include "xe/xe_legacy.h"
> +#include "xe/xe_spin.h"
> +
> +/* Batch buffer element count, in number of dwords(u32) */
> +#define BATCH_DW_COUNT 16
> +#define CAT_ERROR (0x1 << 5)
> +#define CLOSE_EXEC_QUEUES (0x1 << 2)
> +#define CLOSE_FD (0x1 << 1)
> +/* Batch buffer element count, in number of dwords(u32) */
> +#define GT_RESET (0x1 << 0)
> +#define MAX_N_EXECQUEUES 16
> +
> +/**
> + * xe_legacy_test_mode:
> + * @fd: file descriptor
> + * @eci: engine class instance
> + * @n_exec_queues: number of exec queues
> + * @n_execs: number of execs
> + * @flags: flags for the test
> + * @addr: address for the test
> + * @use_capture_mode: use capture mode or not
> + *
> + * Returns: void
> + */
> +void
> +xe_legacy_test_mode(int fd, struct drm_xe_engine_class_instance *eci,
> + int n_exec_queues, int n_execs, unsigned int flags,
> + u64 addr, bool use_capture_mode)
> +{
> + u32 vm;
> + struct drm_xe_sync sync[2] = {
> + { .type = DRM_XE_SYNC_TYPE_SYNCOBJ, .flags = DRM_XE_SYNC_FLAG_SIGNAL, },
> + { .type = DRM_XE_SYNC_TYPE_SYNCOBJ, .flags = DRM_XE_SYNC_FLAG_SIGNAL, },
> + };
> + struct drm_xe_exec exec = {
> + .num_batch_buffer = 1,
> + .num_syncs = 2,
> + .syncs = to_user_pointer(sync),
> + };
> + u32 exec_queues[MAX_N_EXECQUEUES];
> + u32 syncobjs[MAX_N_EXECQUEUES];
> + size_t bo_size;
> + u32 bo = 0;
> + struct {
> + struct xe_spin spin;
> + u32 batch[BATCH_DW_COUNT];
> + u64 pad;
> + u32 data;
> + } *data;
> + struct xe_spin_opts spin_opts = { .preempt = false };
> + int i, b;
> +
> + igt_assert_lte(n_exec_queues, MAX_N_EXECQUEUES);
> +
> + if (flags & CLOSE_FD)
> + fd = drm_open_driver(DRIVER_XE);
> +
> + vm = xe_vm_create(fd, 0, 0);
> + bo_size = sizeof(*data) * n_execs;
> + bo_size = xe_bb_size(fd, bo_size);
> +
> + bo = xe_bo_create(fd, vm, bo_size,
> + vram_if_possible(fd, eci->gt_id),
> + DRM_XE_GEM_CREATE_FLAG_NEEDS_VISIBLE_VRAM);
> + data = xe_bo_map(fd, bo, bo_size);
> +
> + for (i = 0; i < n_exec_queues; i++) {
> + exec_queues[i] = xe_exec_queue_create(fd, vm, eci, 0);
> + syncobjs[i] = syncobj_create(fd, 0);
> + }
> +
> + sync[0].handle = syncobj_create(fd, 0);
> +
> + /* Binding mechanism based on use_capture_mode */
> + if (use_capture_mode) {
> + __xe_vm_bind_assert(fd, vm, 0, bo, 0, addr, bo_size,
> + DRM_XE_VM_BIND_OP_MAP, flags, sync, 1, 0, 0);
> + } else {
> + xe_vm_bind_async(fd, vm, 0, bo, 0, addr, bo_size, sync, 1);
> + }
> +
> + for (i = 0; i < n_execs; i++) {
> + u64 base_addr = (use_capture_mode || !(flags & CAT_ERROR)) ? addr
> + : addr + bo_size * 128;
Reference form xe_exec_capture.c:
for (i = 0; i < n_execs; i++) {
u64 base_addr = addr;
u64 batch_offset = (char *)&data[i].batch - (char *)data;
Reference form xe_exec_reset.c:
for (i = 0; i < n_execs; i++) {
uint64_t base_addr = flags & CAT_ERROR && !i ?
addr + bo_size * 128 : addr;
uint64_t batch_offset = (char *)&data[i].batch - (char *)data;
Is "&& !i" check lost here?
> + u64 batch_offset = (char *)&data[i].batch - (char *)data;
> + u64 batch_addr = base_addr + batch_offset;
> + u64 spin_offset = (char *)&data[i].spin - (char *)data;
> + u64 sdi_offset = (char *)&data[i].data - (char *)data;
> + u64 sdi_addr = base_addr + sdi_offset;
> + u64 exec_addr;
> + int e = i % n_exec_queues;
...