[PATCH 03/43] drm/xe: Clarify size of MMIO region

[Lucas De Marchi]

On Tue, Sep 03, 2024 at 05:21:04PM GMT, Matt Roper wrote:
>xe_mmio currently has a size parameter that is assigned but never used
>anywhere.  The current values assigned appear to be the size of the BAR
>region assigned for the tile (both for registers and other purposes such
>as the GGTT).  Since the current field isn't being used for anything,
>change the assignments to 4MB (the size of the register region on all
>current platform) and rename the field to 'regs_length' to more clearly
>describe what it represents.  We can use this value in later patches to
>help ensure no register accesses accidentally go past the end of the
>desired register space (which might not be caught easily if they still
>fall within the iomap).
>
>Signed-off-by: Matt Roper <matthew.d.roper at intel.com>
>---
> drivers/gpu/drm/xe/xe_device_types.h | 10 ++++++++--
> drivers/gpu/drm/xe/xe_mmio.c         | 10 ++++++++--
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
>diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>index a02e5dfcc6a7..7427f40086e8 100644
>--- a/drivers/gpu/drm/xe/xe_device_types.h
>+++ b/drivers/gpu/drm/xe/xe_device_types.h
>@@ -119,8 +119,14 @@ struct xe_mmio {
> 	/** @regs: Map used to access registers. */
> 	void __iomem *regs;
>
>-	/** @map_size: Size of the map. */
>-	size_t size;
>+	/**
>+	 * @map_size: Length of the register region within the map.

see review on previous patch

>+	 *
>+	 * The size of the map itself it generally larger than the iomap; the

... is generally?

>+	 * map itself is generally shared with other regions, and also includes
>+	 * non-register regions such as the GGTT PTEs.

There's some repeated sentences above... Maybe consider a reword like:

"The size of the iomap set in @regs is generally larger than the register
mmio space since it includes non-register regions such as the GGTT PTEs"

I also wonder if we shouldn't break the map into smaller regions so we
don't risk having bugs in a lower region overwritting things above it.

>+	 */
>+	size_t regs_length;

why not regs_size? It seems more natural than "length".


Reviewed-by: Lucas De Marchi <lucas.demarchi at intel.com>

Lucas De Marchi

> };
>
> /**
>diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>index 3fd462fda625..ad6a7992b00e 100644
>--- a/drivers/gpu/drm/xe/xe_mmio.c
>+++ b/drivers/gpu/drm/xe/xe_mmio.c
>@@ -36,13 +36,19 @@ static void tiles_fini(void *arg)
> /*
>  * On multi-tile devices, partition the BAR space for MMIO on each tile,
>  * possibly accounting for register override on the number of tiles available.
>+ * tile_mmio_size contains both the tile's 4MB register space, as well as
>+ * additional space for the GTT and other (possibly unused) regions).
>  * Resulting memory layout is like below:
>  *
>  * .----------------------. <- tile_count * tile_mmio_size
>  * |         ....         |
>  * |----------------------| <- 2 * tile_mmio_size
>+ * |   tile1 GTT + other  |
>+ * |----------------------| <- 1 * tile_mmio_size + 4MB
>  * |   tile1->mmio.regs   |
>  * |----------------------| <- 1 * tile_mmio_size
>+ * |   tile0 GTT + other  |
>+ * |----------------------| <- 4MB
>  * |   tile0->mmio.regs   |
>  * '----------------------' <- 0MB
>  */
>@@ -90,7 +96,7 @@ static void mmio_multi_tile_setup(struct xe_device *xe, size_t tile_mmio_size)
>
> 	regs = xe->mmio.regs;
> 	for_each_tile(tile, xe, id) {
>-		tile->mmio.size = tile_mmio_size;
>+		tile->mmio.regs_length = SZ_4M;
> 		tile->mmio.regs = regs;
> 		regs += tile_mmio_size;
> 	}
>@@ -172,7 +178,7 @@ int xe_mmio_init(struct xe_device *xe)
> 	}
>
> 	/* Setup first tile; other tiles (if present) will be setup later. */
>-	root_tile->mmio.size = SZ_16M;
>+	root_tile->mmio.regs_length = SZ_4M;
> 	root_tile->mmio.regs = xe->mmio.regs;
>
> 	return devm_add_action_or_reset(xe->drm.dev, mmio_fini, xe);
>-- 
>2.45.2
>