Re: [i-g-t,v2,2/2] lib/xe_mmio: Introduce Xe MMIO lib

["Laguna, Lukasz" <lukasz.laguna@intel.com>]

On 3/26/2024 11:09, Piórkowski, Piotr wrote:
> From: Piotr Piórkowski <piotr.piorkowski@intel.com>
>
> Currently in IGT we have a library intel_mmio for simple MMIO operations
> on intel GPU devices, but it is limited only to accessing registers,
> has a lot of legacy code related to the i915 and offers no support for
> multi tile.
> Let's reuse the memory mapping from the previous library and add separate
> helpers, dedicated to Xe, for registers and GGTT access that support multi
> tile.
>
> v2:
>   - add missing std ifndef/define in header lib/xe_mmio.h
>
> Signed-off-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> Cc: Kamil Konieczny <kamil.konieczny@linux.intel.com>
> ---
>   lib/meson.build   |   1 +
>   lib/xe/xe_mmio.c  | 207 ++++++++++++++++++++++++++++++++++++++++++++++
>   lib/xe/xe_mmio.h  |  42 ++++++++++
>   lib/xe/xe_query.c |  19 +++++
>   lib/xe/xe_query.h |   1 +
>   5 files changed, 270 insertions(+)
>   create mode 100644 lib/xe/xe_mmio.c
>   create mode 100644 lib/xe/xe_mmio.h
>
> diff --git a/lib/meson.build b/lib/meson.build
> index a5651571b..e2f740c11 100644
> --- a/lib/meson.build
> +++ b/lib/meson.build
> @@ -111,6 +111,7 @@ lib_sources = [
>   	'igt_dsc.c',
>   	'xe/xe_gt.c',
>   	'xe/xe_ioctl.c',
> +	'xe/xe_mmio.c',
>   	'xe/xe_query.c',
>   	'xe/xe_spin.c',
>   	'xe/xe_util.c',
> diff --git a/lib/xe/xe_mmio.c b/lib/xe/xe_mmio.c
> new file mode 100644
> index 000000000..bd4227f25
> --- /dev/null
> +++ b/lib/xe/xe_mmio.c
> @@ -0,0 +1,207 @@
> +// SPDX-License-Identifier: MIT
> +/*
> + * Copyright(c) 2024 Intel Corporation. All rights reserved.
> + */
> +
> +#include "igt_device.h"
> +
> +#include "xe/xe_mmio.h"
> +#include "xe/xe_query.h"
> +
> +/**
> + * xe_mmio_vf_access_init:
> + * @pf_fd: xe device file descriptor
> + * @vf_id: PCI virtual function number (0 if native or PF itself)
> + * @mmio: xe mmio structure for IO operations
> + *
> + * This initializes the xe mmio structure, and maps the MMIO BAR owned by
> + * the specified virtual function associated with @pf_fd.
> + */
> +void xe_mmio_vf_access_init(int pf_fd, int vf_id, struct xe_mmio *mmio)
> +{
> +	struct pci_device *pci_dev = __igt_device_get_pci_device(pf_fd, vf_id);
> +
> +	igt_assert_f(pci_dev, "No PCI device found for VF%u\n", vf_id);
> +
> +	intel_mmio_use_pci_bar(&mmio->intel_mmio, pci_dev);
> +
> +	igt_assert(!mmio->intel_mmio.igt_mmio);
> +
> +	mmio->fd = pf_fd;


no mmio->vf_id = ... ?


> +	mmio->intel_mmio.safe = false;
> +	mmio->intel_mmio.pci_device_id = pci_dev->device_id;
> +}
> +
> +/**
> + * xe_mmio_access_init:
> + * @pf_fd: xe device file descriptor
> + * @mmio: xe mmio structure for IO operations
> + *
> + * This initializes the xe mmio structure, and maps MMIO BAR for @pf_fd device.
> + */
> +void xe_mmio_access_init(int pf_fd, struct xe_mmio *mmio)
> +{
> +	xe_mmio_vf_access_init(pf_fd, 0, mmio);
> +}
> +
> +/**
> + * xe_mmio_access_fini:
> + * @mmio: xe mmio structure for IO operations
> + *
> + * Clean up the mmio access helper initialized with
> + * xe_mmio_access_init()/xe_mmio_vf_access_init().
> + */
> +void xe_mmio_access_fini(struct xe_mmio *mmio)
> +{
> +	mmio->intel_mmio.pci_device_id = 0;


Hmm... maybe better to remove warn from intel_mmio_unmap_pci_bar() ? Is 
it really needed there?


> +	intel_mmio_unmap_pci_bar(&mmio->intel_mmio);
> +	igt_pci_system_cleanup();
> +}
> +
> +/**
> + * xe_mmio_read32:
> + * @mmio: xe mmio structure for IO operations
> + * @reg: mmio register offset
> + *
> + * 32-bit read of the register at @offset.


@reg / @offset inconsistency
s/@offset/@reg ?


> + *
> + * Returns:
> + * The value read from the register.
> + */
> +uint32_t xe_mmio_read32(struct xe_mmio *mmio, uint32_t reg)
> +{
> +	return ioread32(mmio->intel_mmio.igt_mmio, reg);
> +}
> +
> +/**
> + * xe_mmio_read64:
> + * @mmio: xe mmio structure for IO operations
> + * @reg: mmio register offset
> + *
> + * 64-bit read of the register at @offset.
> + *
> + * Returns:
> + * The value read from the register.
> + */
> +uint64_t xe_mmio_read64(struct xe_mmio *mmio, uint32_t reg)
> +{
> +	return ioread64(mmio->intel_mmio.igt_mmio, reg);
> +}
> +
> +/**
> + * xe_mmio_write32:
> + * @mmio: xe mmio structure for IO operations
> + * @reg: mmio register offset
> + * @val: value to write
> + *
> + * 32-bit write to the register at @offset.
> + */
> +void xe_mmio_write32(struct xe_mmio *mmio, uint32_t reg, uint32_t val)
> +{
> +	return iowrite32(mmio->intel_mmio.igt_mmio, reg, val);
> +}
> +
> +/**
> + * xe_mmio_write64:
> + * @mmio: xe mmio structure for IO operations
> + * @reg: mmio register offset
> + * @val: value to write
> + *
> + * 64-bit write to the register at @offset.
> + */
> +void xe_mmio_write64(struct xe_mmio *mmio, uint32_t reg, uint64_t val)
> +{
> +	return iowrite64(mmio->intel_mmio.igt_mmio, reg, val);
> +}
> +
> +/**
> + * xe_mmio_gt_read32:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @reg: mmio register offset in tile to which @gt belongs
> + *
> + * 32-bit read of the register at @offset in tile to which @gt belongs.
> + *
> + * Returns:
> + * The value read from the register.
> + */
> +uint32_t xe_mmio_gt_read32(struct xe_mmio *mmio, int gt, uint32_t reg)
> +{
> +	return xe_mmio_read32(mmio, reg + (TILE_MMIO_SIZE * xe_gt_get_tile_id(mmio->fd, gt)));
> +}
> +
> +/**
> + * xe_mmio_gt_read64:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @reg: mmio register offset in tile to which @gt belongs
> + *
> + * 64-bit read of the register at @offset in tile to which @gt belongs.
> + *
> + * Returns:
> + * The value read from the register.
> + */
> +uint64_t xe_mmio_gt_read64(struct xe_mmio *mmio, int gt, uint32_t reg)
> +{
> +	return xe_mmio_read64(mmio, reg + (TILE_MMIO_SIZE * xe_gt_get_tile_id(mmio->fd, gt)));
> +}
> +
> +/**
> + * xe_mmio_gt_write32:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @reg: mmio register offset
> + * @val: value to write
> + *
> + * 32-bit write to the register at @offset in tile to which @gt belongs.
> + */
> +void xe_mmio_gt_write32(struct xe_mmio *mmio, int gt, uint32_t reg, uint32_t val)
> +{
> +	return xe_mmio_write32(mmio, reg + (TILE_MMIO_SIZE * xe_gt_get_tile_id(mmio->fd, gt)),
> +			       val);
> +}
> +
> +/**
> + * xe_mmio_gt_write64:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @reg: mmio register offset
> + * @val: value to write
> + *
> + * 64-bit write to the register at @offset in tile to which @gt belongs.
> + */
> +void xe_mmio_gt_write64(struct xe_mmio *mmio, int gt, uint32_t reg, uint64_t val)
> +{
> +	return xe_mmio_write64(mmio, reg + (TILE_MMIO_SIZE * xe_gt_get_tile_id(mmio->fd, gt)),
> +			       val);
> +}
> +
> +/**
> + * xe_mmio_ggtt_read:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @offset: PTE offset from the beginning of GGTT, in tile to which @gt belongs
> + *
> + * Read of GGTT PTE at GGTT @offset in tile to which @gt belongs.
> + *
> + * Returns:
> + * The value read from the register.
> + */
> +xe_ggtt_pte_t xe_mmio_ggtt_read(struct xe_mmio *mmio, int gt, uint32_t offset)
> +{
> +	return xe_mmio_gt_read64(mmio, gt, offset + GGTT_OFFSET_IN_TILE);
> +}
> +
> +/**
> + * xe_mmio_ggtt_write:
> + * @mmio: xe mmio structure for IO operations
> + * @gt: gt id
> + * @offset: PTE offset from the beginning of GGTT, in tile to which @gt belongs
> + * @pte: PTE value to write
> + *
> + * Write PTE value at GGTT @offset in tile to which @gt belongs.
> + */
> +void xe_mmio_ggtt_write(struct xe_mmio *mmio, int gt, uint32_t offset, xe_ggtt_pte_t pte)
> +{
> +	return xe_mmio_gt_write64(mmio, gt, offset + GGTT_OFFSET_IN_TILE, pte);
> +}
> diff --git a/lib/xe/xe_mmio.h b/lib/xe/xe_mmio.h
> new file mode 100644
> index 000000000..96bba6e16
> --- /dev/null
> +++ b/lib/xe/xe_mmio.h
> @@ -0,0 +1,42 @@
> +/* SPDX-License-Identifier: MIT */
> +/*
> + * Copyright(c) 2024 Intel Corporation. All rights reserved.
> + */
> +
> +#include "lib/intel_io.h"
> +#include "lib/igt_sizes.h"
> +
> +#ifndef XE_MMIO_H
> +#define XE_MMIO_H
> +
> +#define TILE_MMIO_SIZE		SZ_16M
> +#define GGTT_OFFSET_IN_TILE	SZ_8M
> +
> +typedef uint64_t xe_ggtt_pte_t;
> +
> +struct xe_mmio {
> +	int fd;
> +	unsigned int vf_id;
> +	struct intel_mmio_data intel_mmio;
> +};
> +
> +void xe_mmio_vf_access_init(int pf_fd, int vf_id, struct xe_mmio *mmio);
> +void xe_mmio_access_init(int pf_fd, struct xe_mmio *mmio);
> +void xe_mmio_access_fini(struct xe_mmio *mmio);
> +
> +uint32_t xe_mmio_read32(struct xe_mmio *mmio, uint32_t reg);
> +uint64_t xe_mmio_read64(struct xe_mmio *mmio, uint32_t reg);
> +
> +void xe_mmio_write32(struct xe_mmio *mmio, uint32_t reg, uint32_t val);
> +void xe_mmio_write64(struct xe_mmio *mmio, uint32_t reg, uint64_t val);
> +
> +uint32_t xe_mmio_gt_read32(struct xe_mmio *mmio, int gt, uint32_t reg);
> +uint64_t xe_mmio_gt_read64(struct xe_mmio *mmio, int gt, uint32_t reg);
> +
> +void xe_mmio_gt_write32(struct xe_mmio *mmio, int gt, uint32_t reg, uint32_t val);
> +void xe_mmio_gt_write64(struct xe_mmio *mmio, int gt, uint32_t reg, uint64_t val);
> +
> +xe_ggtt_pte_t xe_mmio_ggtt_read(struct xe_mmio *mmio, int gt, uint32_t pte_offset);
> +void xe_mmio_ggtt_write(struct xe_mmio *mmio, int gt, uint32_t pte_offset, xe_ggtt_pte_t pte);
> +
> +#endif	/* XE_MMIO_H */
> diff --git a/lib/xe/xe_query.c b/lib/xe/xe_query.c
> index 53a2b4386..c885e3a79 100644
> --- a/lib/xe/xe_query.c
> +++ b/lib/xe/xe_query.c
> @@ -731,6 +731,25 @@ bool xe_is_media_gt(int fd, int gt)
>   	return false;
>   }
>   
> +/**
> + * xe_gt_to_tile_id:
> + * @fd: xe device fd
> + * @gt: gt id
> + *
> + * Returns tile id for given @gt.
> + */
> +int xe_gt_get_tile_id(int fd, int gt)


uint16_t xe_gt_get_tile_id(...)


> +{
> +	struct xe_device *xe_dev;
> +
> +	xe_dev = find_in_cache(fd);
> +
> +	igt_assert(xe_dev);
> +	igt_assert(gt < xe_number_gt(fd));
> +
> +	return xe_dev->gt_list->gt_list[gt].tile_id;
> +}
> +
>   igt_constructor
>   {
>   	xe_device_cache_init();
> diff --git a/lib/xe/xe_query.h b/lib/xe/xe_query.h
> index 82af2706d..b1b3a989e 100644
> --- a/lib/xe/xe_query.h
> +++ b/lib/xe/xe_query.h
> @@ -99,6 +99,7 @@ const char *xe_engine_class_string(uint32_t engine_class);
>   bool xe_has_engine_class(int fd, uint16_t engine_class);
>   bool xe_has_media_gt(int fd);
>   bool xe_is_media_gt(int fd, int gt);
> +int xe_gt_get_tile_id(int fd, int gt);
>   
>   struct xe_device *xe_device_get(int fd);
>   void xe_device_put(int fd);