Re: [PATCH i-g-t] tests/intel/xe_multi_tile: Multi-Tile support in IGT

[Matt Roper <matthew.d.roper@intel.com>]

On Fri, Jan 31, 2025 at 02:27:12PM +0000, nishit.sharma@intel.com wrote:
> From: Nishit Sharma <nishit.sharma@intel.com>
> 
> Added functionality to get Tile ID, GT ID, GT belongs to which tile ID
> in multi-tile/single-tile platforms. Added functionality to check if any
> tile_ID information not provided by driver. E.g.tile available in platforms in order tile0,
> tile1...tileX in serial order, If tile1 tile2 tile4...tileX populated
> than will get Warning tile3 not available.
> 
> Signed-off-by: Nishit Sharma <nishit.sharma@intel.com>
> ---
>  lib/igt_sysfs.c             |   3 +-
>  tests/intel/xe_multi_tile.c | 191 ++++++++++++++++++++++++++++++++++++
>  tests/meson.build           |   1 +
>  3 files changed, 194 insertions(+), 1 deletion(-)
>  create mode 100644 tests/intel/xe_multi_tile.c
> 
> diff --git a/lib/igt_sysfs.c b/lib/igt_sysfs.c
> index 00d5822fd..37f1716e2 100644
> --- a/lib/igt_sysfs.c
> +++ b/lib/igt_sysfs.c
> @@ -234,7 +234,8 @@ char *xe_sysfs_gt_path(int xe_device, int gt, char *path, int pathlen)
>  
>  	if (IS_PONTEVECCHIO(intel_get_drm_devid(xe_device)))
>  		snprintf(path, pathlen, "/sys/dev/char/%d:%d/device/tile%d/gt%d",
> -			 major(st.st_rdev), minor(st.st_rdev), gt, gt);
> +			 major(st.st_rdev), minor(st.st_rdev),
> +			 xe_gt_get_tile_id(xe_device, gt), gt);
>  	else
>  		snprintf(path, pathlen, "/sys/dev/char/%d:%d/device/tile0/gt%d",
>  			 major(st.st_rdev), minor(st.st_rdev), gt);

The fix for the library code should probably be a separate commit from
the new test being added since this will presumably change/fix existing
code.

> diff --git a/tests/intel/xe_multi_tile.c b/tests/intel/xe_multi_tile.c
> new file mode 100644
> index 000000000..7ca40f1fe
> --- /dev/null
> +++ b/tests/intel/xe_multi_tile.c
> @@ -0,0 +1,191 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright © 2025 Intel Corporation
> + *
> + * Authors:
> + *      Nishit Sharma <nishit.sharma@intel.com>
> + */
> +
> +#include <dirent.h>
> +#include <fcntl.h>
> +#include <sys/types.h>
> +#include <sys/stat.h>
> +#include <unistd.h>
> +
> +#include "igt.h"
> +#include "igt_sysfs.h"
> +
> +#include "xe_drm.h"
> +#include "xe/xe_ioctl.h"
> +#include "xe/xe_query.h"
> +
> +#define MAX_TILES	8
> +#define MAX_SLICES	2
> +
> +struct xe_gt_info {
> +	/** @type: GT type: Main or Media */
> +	uint16_t type;
> +
> +	/** @tile_id: Tile ID where this GT lives (Information only) */
> +	uint16_t  tile_id;
> +
> +	/** @gt_id: Unique ID of this GT within the PCI Device */
> +	uint16_t gt_id;
> +};
> +
> +/** @info: tile-gt info */
> +struct xe_tile_info {
> +	uint8_t tile_count;
> +	uint8_t gt_count;
> +	uint8_t mis_tile:1;
> +	uint8_t mis_tile_id;
> +	struct xe_gt_info gt_info[];
> +};
> +
> +/**
> + * TEST: Test to get tile_id by iterating gt on xe
> + * Category: Core
> + * Mega feature: General Core features
> + * Sub-category: mapping tile/s with slices available
> + * Functionality: gt operation
> + */
> +
> +/**
> + * SUBTEST: show-tile-info
> + * SUBTEST: gt-configuration

These don't seem to match the name of the subtest in the code
("multi-tile-info").

> + * Description: Test iniitialize xe_tile_info structure with tile_id

This doesn't really seem like a meaningful description since
'xe_tile_info' is just an internal structure used inside this test.  

What is the actual goal/motivation here.  Are you trying to verify that
the Xe query interface returns data correctly?  Are you trying to
confirm that some kind of hardware behavior is sane?  Are you trying to
confirm that different interfaces give consistent information (e.g.,
query ioctl vs sysfs)?  It's hard to give feedback on the test when it
isn't clear what the primary goal is.

> + * Test category: functionality test
> + *
> + */
> +struct drm_xe_query_gt_list *xe_gt_list(int fd)
> +{
> +	struct drm_xe_query_gt_list *gt_list;
> +	struct drm_xe_device_query query = {
> +		.extensions = 0,
> +		.query = DRM_XE_DEVICE_QUERY_GT_LIST,
> +		.size = 0,
> +		.data = 0,
> +	};
> +
> +	igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
> +	igt_assert_neq(query.size, 0);
> +
> +	gt_list = malloc(query.size);
> +	igt_assert(gt_list);
> +
> +	query.data = to_user_pointer(gt_list);
> +	igt_assert_eq(igt_ioctl(fd, DRM_IOCTL_XE_DEVICE_QUERY, &query), 0);
> +
> +	return gt_list;
> +}

Isn't this function basically the same as xe_query_gt_list_new()?  Can't
we just call that instead?

> +
> +static void igt_get_gt_info(int fd, struct xe_tile_info *tile_info)
> +{
> +	uint8_t iteration = 0;
> +	uint8_t gt_id = 0;
> +
> +	igt_info("GT iteration\n");
> +
> +	for (int tile_id = 0; tile_id < tile_info->tile_count; tile_id++) {
> +		for (gt_id += iteration; gt_id < tile_info->gt_count; gt_id++) {
> +			if (tile_id == tile_info->gt_info[gt_id].tile_id) {
> +				igt_info("GT :gt%d belongs to :tile%d\n",
> +						tile_info->gt_info[gt_id].gt_id,
> +						tile_info->gt_info[gt_id].tile_id);
> +				iteration++;
> +			}
> +			else {
> +				igt_info("GT :gt%d belongs to :tile%d\n",
> +						tile_info->gt_info[gt_id].gt_id,
> +						tile_info->gt_info[gt_id].tile_id);
> +				continue;
> +			}
> +		}
> +	}
> +}

Was this function supposed to do/return something?  With a name like
this I expected it to return information to the caller, but it seems
like this just loops and prints some debug logging.  If we removed this
function completely it wouldn't actually affect the test in any way.

> +
> +static void igt_check_if_missingtile(int fd, struct xe_tile_info *tile_info)
> +{
> +	if (tile_info->mis_tile == true)
> +		igt_warn("Missing :tile%d\n", tile_info->mis_tile_id);
> +}
> +
> +static void igt_check_if_multitile(int fd, struct xe_tile_info *tile_info)
> +{
> +	igt_info("Check for Multi-tile support in Platform\n");
> +
> +	igt_skip_on_f(tile_info->tile_count == 0,
> +		      "No Tile :%d found in platform\n",
> +		      tile_info->tile_count);
> +
> +	if (tile_info->tile_count == 1)
> +		igt_info("Single tile :tile%d found in platform\n",
> +			 tile_info->tile_count);
> +	else {
> +		igt_info("Tile count :%d in Multi-Tile platform\n",
> +			 tile_info->tile_count);
> +
> +		for (int tile_id = 0; tile_id < tile_info->tile_count; tile_id++)
> +			igt_info("Tile :tile%d available in platform\n", tile_id);
> +	}
> +}

It seems like this is a lot of unnecessary debug logging; all we really
need is a skip at the callsite rather than a whole dedicated function to
do the same thing.

> +
> +static void igt_get_tile_info(int fd, struct drm_xe_query_gt_list *gt_list,
> +			 struct xe_tile_info *tile_info)
> +{
> +	uint8_t tile_mis_count = -1;
> +	int prev_tile = -1, tile_id;
> +
> +	tile_info->tile_count = 0;
> +	tile_info->mis_tile = false;
> +	for (int index = 0; index < gt_list->num_gt; index++) {
> +		tile_id = gt_list->gt_list[index].tile_id;
> +		if (prev_tile != tile_id) {
> +			if (++tile_mis_count != tile_id) {
> +				tile_info->mis_tile = true;
> +				tile_info->mis_tile_id = tile_mis_count;
> +				tile_mis_count = tile_id;
> +			}
> +			prev_tile = tile_id;
> +			tile_info->tile_count++;
> +			tile_info->gt_info[index].tile_id = prev_tile;
> +			tile_info->gt_info[index].gt_id = index;
> +		}
> +		else {
> +			tile_info->gt_info[index].tile_id = tile_id;
> +			tile_info->gt_info[index].gt_id = index;
> +		}
> +		tile_info->gt_info[index].type = xe_is_media_gt(fd, index);
> +	}
> +	tile_info->gt_count = gt_list->num_gt;
> +}
> +
> +igt_main
> +{
> +	int fd;
> +	struct xe_engine_list_entry *en;
> +	struct drm_xe_engine_class_instance *hwe;
> +	struct drm_xe_query_gt_list *gt_list;
> +	struct xe_tile_info *tile_info;
> +
> +	tile_info = malloc(sizeof(*tile_info));
> +	igt_assert(tile_info);
> +
> +	igt_fixture {
> +		fd = drm_open_driver(DRIVER_XE);
> +		xe_device_get(fd);
> +	}
> +
> +	igt_subtest("multi-tile-info") {

As noted above, this doesn't really seem to match what the test is
doing.  It appears the goal of this test is to make sure that all tiles
have consecutive IDs, at least as far as we can see from the query
interface.

> +		gt_list = xe_gt_list(fd);
> +
> +		igt_get_tile_info(fd, gt_list, tile_info);
> +		igt_check_if_multitile(fd, tile_info);
> +		igt_check_if_missingtile(fd, tile_info);
> +		igt_get_gt_info(fd, tile_info);

As noted above, this function doesn't do anything; we could just drop
it.

> +	}
> +

Depending on what your goal is, should there be other tests here as
well?  E.g., should we confirm that the query interface is consistent
with sysfs (no tile IDs reported in one that don't appear in the other)?
Is there anything else we should be checking?


Matt


> +	igt_fixture {
> +		drm_close_driver(fd);
> +	}
> +}
> diff --git a/tests/meson.build b/tests/meson.build
> index 2724c7a9a..2cc01aa0c 100644
> --- a/tests/meson.build
> +++ b/tests/meson.build
> @@ -292,6 +292,7 @@ intel_xe_progs = [
>  	'xe_exec_reset',
>  	'xe_exec_sip',
>  	'xe_exec_store',
> +	'xe_multi_tile',
>  	'xe_exec_threads',
>  	'xe_exercise_blt',
>  	'xe_fault_injection',
> -- 
> 2.34.1
> 

-- 
Matt Roper
Graphics Software Engineer
Linux GPU Platform Enablement
Intel Corporation