Re: [PATCH i-g-t] lib/xe/xe_legacy: Move test_legacy_mode to lib/

["Dong, Zhanjun" <zhanjun.dong@intel.com>]

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@intel.com
> Signed-off-by: Peter Senna Tschudin <peter.senna@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;

...