Re: [PATCH i-g-t v5 4/7] lib/xe: Introduce dmem driver and implement Xe support

On 23/07/2026 20:43, Thadeu Lima de Souza Cascardo wrote:
> In order to be reuse the same dmem tests with multiple drivers, we need to
> abstract a few operations. That includes getting the region name, and
> allocating and releasing VRAM. As there is some initialization also when
> multiple allocations are done, also provide init and deinit functions.
> 
> The Xe implementation was based on the original operations from
> xe_cgroups.c written by Thomas Hellström. However, instead of doing a
> deferred backing, followed by a bind, it does a simple non-deferred GEM
> object creation on the VRAM region.
> 
> Signed-off-by: Thadeu Lima de Souza Cascardo <[email protected]>
> ---
>   lib/igt_dmem_driver.h |  24 ++++++++
>   lib/meson.build       |   1 +
>   lib/xe/xe_dmem.c      | 139 ++++++++++++++++++++++++++++++++++++++++++
>   3 files changed, 164 insertions(+)
>   create mode 100644 lib/igt_dmem_driver.h
>   create mode 100644 lib/xe/xe_dmem.c
> 
> diff --git a/lib/igt_dmem_driver.h b/lib/igt_dmem_driver.h
> new file mode 100644
> index 000000000000..2f4a4673ab16
> --- /dev/null
> +++ b/lib/igt_dmem_driver.h
> @@ -0,0 +1,24 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2026 Valve Corporation
> + * Authors:
> + *  Thadeu Lima de Souza Cascardo <[email protected]>
> + */
> +
> +#ifndef __IGT_DMEM_DRIVER_H__
> +#define __IGT_DMEM_DRIVER_H__
> +
> +#include <stdlib.h>
> +
> +struct igt_dmem_driver {
> +	const char *name;
> +	char * (*get_region_name)(int fd);
> +	int (*init)(void **ctx, int fd, int max_bo);
> +	void (*deinit)(void *ctx);
> +	int (*allocate_vram)(void *ctx, int n_bo, size_t len);
> +	void (*free_vram)(void *ctx, int n_bo);

I expect IGT maintainers might ask for kerneldoc for these.

> +};
> +
> +extern const struct igt_dmem_driver xe_dmem_driver;
> +
> +#endif
> diff --git a/lib/meson.build b/lib/meson.build
> index 868220408f8c..012802475de0 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -126,6 +126,7 @@ lib_sources = [
>   	'igt_msm.c',
>   	'igt_dsc.c',
>   	'igt_hook.c',
> +	'xe/xe_dmem.c',
>   	'xe/xe_gt.c',
>   	'xe/xe_ioctl.c',
>   	'xe/xe_legacy.c',
> diff --git a/lib/xe/xe_dmem.c b/lib/xe/xe_dmem.c
> new file mode 100644
> index 000000000000..1dec8cf8b0a1
> --- /dev/null
> +++ b/lib/xe/xe_dmem.c
> @@ -0,0 +1,139 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright 2026 Valve Corporation
> + * Authors:
> + *  Thadeu Lima de Souza Cascardo <[email protected]>
> + */
> +
> +#include <errno.h>
> +
> +#include "igt.h"
> +#include "igt_cgroup.h"
> +#include "igt_dmem_driver.h"
> +#include "xe_drm.h"
> +#include "xe/xe_ioctl.h"
> +#include "xe/xe_query.h"
> +
> +static char * xe_dmem_get_region_name(int fd)
> +{
> +	uint64_t vram_region = 0;
> +	uint64_t region;
> +	char *cg_region;
> +
> +	/* Find first VRAM region */
> +	xe_for_each_mem_region(fd, all_memory_regions(fd), region) {
> +		if (xe_region_class(fd, region) == DRM_XE_MEM_REGION_CLASS_VRAM) {
> +			vram_region = region;
> +			break;
> +		}
> +	}

FWIW it looks like that from here..

> +	if (!vram_region)
> +		return NULL;
> +
> +	cg_region = xe_cgroup_region_name(fd, vram_region);
> +
> +	return cg_region;

.. to here could be simplified as return xe_cgroup_region_name(...), 
given the helper returns NULL for non-VRAM regions. Or even return 
straight from the above loop instead of the break. Up to you which of 
the three options you prefer, just thinking out loud.

> +}
> +
> +struct xe_bo {
> +	uint32_t handle;
> +	size_t len;
> +};
> +
> +struct xe_dmem_ctx {
> +	int fd;
> +	struct xe_bo *bos;
> +	uint64_t vram_region;
> +	int max_bo;
> +};
> +
> +static int xe_dmem_init(void **ctx, int fd, int max_bo)
> +{
> +	struct xe_dmem_ctx *xe_ctx;
> +	uint64_t region;
> +
> +	xe_ctx = malloc(sizeof(*xe_ctx));
> +	if (!xe_ctx)
> +		return -ENOMEM;
> +
> +	xe_ctx->bos = calloc(max_bo, sizeof(xe_ctx->bos[0]));
> +	if (!xe_ctx->bos)
> +		goto out;
> +	memset(xe_ctx->bos, 0, max_bo * sizeof(xe_ctx->bos[0]));

calloc already zeroes the memory.

> +	xe_ctx->max_bo = max_bo;

Could the need for max_bo input parameter be avoided if the arrays were 
allocated in chunks and grown? More complex implementation but a simpler 
API.

> +
> +	xe_ctx->vram_region = 0;
> +	/* Find first VRAM region */
> +	xe_for_each_mem_region(fd, all_memory_regions(fd), region) {
> +		if (xe_region_class(fd, region) == DRM_XE_MEM_REGION_CLASS_VRAM) {
> +			xe_ctx->vram_region = region;
> +			break;
> +		}
> +	}
> +	if (!xe_ctx->vram_region)
> +		goto out;
> +
> +	xe_ctx->fd = fd;
> +
> +	*ctx = xe_ctx;
> +
> +	return 0;
> +
> +out:
> +	if (xe_ctx->bos)
> +		free(xe_ctx->bos);
> +	free(xe_ctx);
> +
> +	return -ENOMEM;
> +}
> +
> +static void xe_dmem_deinit(void *ctx)
> +{
> +	struct xe_dmem_ctx *xe_ctx = ctx;
> +
> +	free(xe_ctx->bos);
> +	free(xe_ctx);

Handle cleanup is not desirable?

> +}
> +
> +static int xe_dmem_allocate_vram(void *ctx, int n_bo, size_t len)

Is the last patch in the series the only user of this API? I am thinking 
whether the n_bo input parameter is required and it wouldn't be simpler 
to just return a handle (or null) and grow the internal count. Ie. is 
there a use case for callers to precisely need to control "allocate/free 
at this index"?

> +{
> +	struct xe_dmem_ctx *xe_ctx = ctx;
> +	uint32_t handle;
> +	int err;
> +
> +	if (n_bo >= xe_ctx->max_bo)
> +		return -ENOMEM;

If the max_bo approach remains, I'd be tempted to igt_assert here since 
it is a test writer error to hit it.

Also, return error if a valid handle is already at the requested index?

> +
> +	err = __xe_bo_create(xe_ctx->fd, 0, len, xe_ctx->vram_region, 0,
> +			     NULL, &handle);
> +	if (err)
> +		goto out;
> +
> +	xe_ctx->bos[n_bo].handle = handle;
> +	xe_ctx->bos[n_bo].len = len;
> +
> +out:
> +	return err;
> +}
> +
> +static void xe_dmem_free_vram(void *ctx, int n_bo)
> +{
> +	struct xe_dmem_ctx *xe_ctx = ctx;
> +	size_t len;
> +	if (n_bo >= xe_ctx->max_bo)
> +		return;

Same assert or not comment as above.

> +	len = xe_ctx->bos[n_bo].len;
> +	if (len) {
> +		gem_close(xe_ctx->fd, xe_ctx->bos[n_bo].handle);
> +		xe_ctx->bos[n_bo].len = 0;
> +	}

It looks like the len local is superflous and handle also needs clearing.

> +}
> +
> +const struct igt_dmem_driver xe_dmem_driver = {
> +	.name = "xe",
> +	.get_region_name = xe_dmem_get_region_name,
> +	.init = xe_dmem_init,
> +	.deinit = xe_dmem_deinit,
> +	.allocate_vram = xe_dmem_allocate_vram,
> +	.free_vram = xe_dmem_free_vram,
> +};

Regards,

Tvrtko