[PATCH] drm/xe: Remove unused "mmio_ext" code
Levi, Ilia
ilia.levi at intel.com
Wed Jan 15 09:46:10 UTC 2025
On 07/01/2025 1:43, Matt Roper wrote:
> The "mmio_ext" and 'REG_EXT" code is currently unused on any existing
> platform. Going forward, this also isn't the design we want to use for
> any future platforms/features either, so we should just go ahead and
> remove the dead code to avoid confusion.
>
> mmio_ext was originally added in an attempt to hack around the early
> (mis)design of the Xe driver, which used xe_gt as the target for all
> register MMIO access, even those completely unrelated to the GT subunit
> of the hardware. With the introduction of commit 34953ee349dd ("drm/xe:
> Create dedicated xe_mmio structure") and its follow-up patches, that
> misdesign has been corrected and access to register MMIO regions
> specific to hardware units is now done through xe_mmio structures which
> encapsulate an iomap, region size, and some other metadata.
>
> Although all of the registers used by the driver today happen to fall
> within one specific PCI BAR region, and thus re-use a single device-wide
> iomap, there's no requirement that this stay true for future platforms
> or features. I.e., if a future platform adds a new 'foo' hardware unit
> that exists at a different area in the BAR, or even in a completely
> different BAR, then that would be handled by doing a separate iomap of
> that unit's register region and wrapping it in its own 'struct xe_mmio
> foo_regs' structure. The pointer to the new 'foo_regs' could be placed
> within the xe_device, xe_tile, xe_gt, etc., according to where the new
> hardware unit falls within the current hardware hierarchy.
So this approach suggests that each instance of struct xe_mmio should be
initialized with its own iomap() call? If so, I think there's more work
to be done here, something like:
1. get rid of xe->mmio
2. refactor mmio_multi_tile_setup (each tile has its own xe_mmio)
- Ilia
>
> This effectively reverts the following commits, although parts of these
> commits had already vanished or changed with the earlier xe_mmio
> refactor work:
>
> - commit 399a13323f0d ("drm/xe: add 28-bit address support in struct
> xe_reg")
> - commit fdef72e02e20 ("drm/xe: add a flag to bypass multi-tile config
> from MTCFG reg")
> - commit 866b2b176434 ("drm/xe: add MMIO extension support flags")
> - commit ef29b390c734 ("drm/xe: map MMIO BAR according to the num of
> tiles in device desc")
> - commit a4e2f3a299ea ("drm/xe: refactor xe_mmio_probe_tiles to support
> MMIO extension")
>
> Cc: Lucas De Marchi <lucas.demarchi at intel.com>
> Cc: Rodrigo Vivi <rodrigo.vivi at intel.com>
> Cc: Koby Elbaz <kelbaz at habana.ai>
> Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
> ---
> drivers/gpu/drm/xe/regs/xe_reg_defs.h | 16 +----------
> drivers/gpu/drm/xe/xe_device_types.h | 11 --------
> drivers/gpu/drm/xe/xe_mmio.c | 39 ---------------------------
> drivers/gpu/drm/xe/xe_pci.c | 3 ---
> drivers/gpu/drm/xe/xe_pci_types.h | 2 --
> 5 files changed, 1 insertion(+), 70 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> index 51fd40ffafcb..0234d1e84139 100644
> --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> @@ -21,7 +21,7 @@ struct xe_reg {
> union {
> struct {
> /** @addr: address */
> - u32 addr:28;
> + u32 addr:22;
This will limit the xe_mmio read/write operations up to 8MiB offset across
all xe_mmio instances. It is enough for now, but we might want to leave it
as is for future platforms. Also, this means that setting regs_size in
xe_mmio struct to anything bigger is meaningless and probably a bug.
Maybe it's worth to add an initializer to xe_mmio struct that asserts this?
- Ilia
> /**
> * @masked: register is "masked", with upper 16bits used
> * to identify the bits that are updated on the lower
> @@ -41,10 +41,6 @@ struct xe_reg {
> * @vf: register is accessible from the Virtual Function.
> */
> u32 vf:1;
> - /**
> - * @ext: access MMIO extension space for current register.
> - */
> - u32 ext:1;
> };
> /** @raw: Raw value with both address and options */
> u32 raw;
> @@ -111,16 +107,6 @@ struct xe_reg_mcr {
> */
> #define XE_REG(r_, ...) ((const struct xe_reg)XE_REG_INITIALIZER(r_, ##__VA_ARGS__))
>
> -/**
> - * XE_REG_EXT - Create a struct xe_reg from extension offset and additional
> - * flags
> - * @r_: Register extension offset
> - * @...: Additional options like access mode. See struct xe_reg for available
> - * options.
> - */
> -#define XE_REG_EXT(r_, ...) \
> - ((const struct xe_reg)XE_REG_INITIALIZER(r_, ##__VA_ARGS__, .ext = 1))
> -
> /**
> * XE_REG_MCR - Create a struct xe_reg_mcr from offset and additional flags
> * @r_: Register offset
> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
> index 8a7b15972413..16ebb2859877 100644
> --- a/drivers/gpu/drm/xe/xe_device_types.h
> +++ b/drivers/gpu/drm/xe/xe_device_types.h
> @@ -186,13 +186,6 @@ struct xe_tile {
> */
> struct xe_mmio mmio;
>
> - /**
> - * @mmio_ext: MMIO-extension info for a tile.
> - *
> - * Each tile has its own additional 256MB (28-bit) MMIO-extension space.
> - */
> - struct xe_mmio mmio_ext;
> -
> /** @mem: memory management info for tile */
> struct {
> /**
> @@ -263,8 +256,6 @@ struct xe_device {
> const char *graphics_name;
> /** @info.media_name: media IP name */
> const char *media_name;
> - /** @info.tile_mmio_ext_size: size of MMIO extension space, per-tile */
> - u32 tile_mmio_ext_size;
> /** @info.graphics_verx100: graphics IP version */
> u32 graphics_verx100;
> /** @info.media_verx100: media IP version */
> @@ -314,8 +305,6 @@ struct xe_device {
> u8 has_heci_gscfi:1;
> /** @info.has_llc: Device has a shared CPU+GPU last level cache */
> u8 has_llc:1;
> - /** @info.has_mmio_ext: Device has extra MMIO address range */
> - u8 has_mmio_ext:1;
> /** @info.has_range_tlb_invalidation: Has range based TLB invalidations */
> u8 has_range_tlb_invalidation:1;
> /** @info.has_sriov: Supports SR-IOV */
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index a48f239cad1c..d321a21aacf0 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -103,50 +103,11 @@ static void mmio_multi_tile_setup(struct xe_device *xe, size_t tile_mmio_size)
> }
> }
>
> -/*
> - * On top of all the multi-tile MMIO space there can be a platform-dependent
> - * extension for each tile, resulting in a layout like below:
> - *
> - * .----------------------. <- ext_base + tile_count * tile_mmio_ext_size
> - * | .... |
> - * |----------------------| <- ext_base + 2 * tile_mmio_ext_size
> - * | tile1->mmio_ext.regs |
> - * |----------------------| <- ext_base + 1 * tile_mmio_ext_size
> - * | tile0->mmio_ext.regs |
> - * |======================| <- ext_base = tile_count * tile_mmio_size
> - * | |
> - * | mmio.regs |
> - * | |
> - * '----------------------' <- 0MB
> - *
> - * Set up the tile[]->mmio_ext pointers/sizes.
> - */
> -static void mmio_extension_setup(struct xe_device *xe, size_t tile_mmio_size,
> - size_t tile_mmio_ext_size)
> -{
> - struct xe_tile *tile;
> - void __iomem *regs;
> - u8 id;
> -
> - if (!xe->info.has_mmio_ext)
> - return;
> -
> - regs = xe->mmio.regs + tile_mmio_size * xe->info.tile_count;
> - for_each_tile(tile, xe, id) {
> - tile->mmio_ext.regs_size = tile_mmio_ext_size;
> - tile->mmio_ext.regs = regs;
> - tile->mmio_ext.tile = tile;
> - regs += tile_mmio_ext_size;
> - }
> -}
> -
> int xe_mmio_probe_tiles(struct xe_device *xe)
> {
> size_t tile_mmio_size = SZ_16M;
> - size_t tile_mmio_ext_size = xe->info.tile_mmio_ext_size;
>
> mmio_multi_tile_setup(xe, tile_mmio_size);
> - mmio_extension_setup(xe, tile_mmio_size, tile_mmio_ext_size);
>
> return devm_add_action_or_reset(xe->drm.dev, tiles_fini, xe);
> }
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 7d146e3e8e21..90fcfafd362f 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -61,7 +61,6 @@ struct xe_device_desc {
> u8 has_heci_gscfi:1;
> u8 has_heci_cscfi:1;
> u8 has_llc:1;
> - u8 has_mmio_ext:1;
> u8 has_sriov:1;
> u8 skip_guc_pc:1;
> u8 skip_mtcfg:1;
> @@ -622,7 +621,6 @@ static int xe_info_init_early(struct xe_device *xe,
> xe->info.has_heci_gscfi = desc->has_heci_gscfi;
> xe->info.has_heci_cscfi = desc->has_heci_cscfi;
> xe->info.has_llc = desc->has_llc;
> - xe->info.has_mmio_ext = desc->has_mmio_ext;
> xe->info.has_sriov = desc->has_sriov;
> xe->info.skip_guc_pc = desc->skip_guc_pc;
> xe->info.skip_mtcfg = desc->skip_mtcfg;
> @@ -682,7 +680,6 @@ static int xe_info_init(struct xe_device *xe,
>
> xe->info.graphics_name = graphics_desc->name;
> xe->info.media_name = media_desc ? media_desc->name : "none";
> - xe->info.tile_mmio_ext_size = graphics_desc->tile_mmio_ext_size;
>
> xe->info.dma_mask_size = graphics_desc->dma_mask_size;
> xe->info.vram_flags = graphics_desc->vram_flags;
> diff --git a/drivers/gpu/drm/xe/xe_pci_types.h b/drivers/gpu/drm/xe/xe_pci_types.h
> index 79b0f80376a4..873efec5cdee 100644
> --- a/drivers/gpu/drm/xe/xe_pci_types.h
> +++ b/drivers/gpu/drm/xe/xe_pci_types.h
> @@ -20,8 +20,6 @@ struct xe_graphics_desc {
>
> u64 hw_engine_mask; /* hardware engines provided by graphics IP */
>
> - u32 tile_mmio_ext_size; /* size of MMIO extension space, per-tile */
> -
> u8 max_remote_tiles:2;
>
> u8 has_asid:1;
More information about the Intel-xe
mailing list