[PATCH 2/2] drm/xe: Add xe_mmio_init() initialization function

[Michal Wajdeczko]

On 30.01.2025 11:50, Ilia Levi wrote:
> Add a convenience function for minimal initialization of struct xe_mmio.
> This function also validates that the space referred by this xe_mmio is
> small enough to be accessed with struct xe_reg.

hmm, maybe is _large_ enough ?

> 
> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
> ---
>  drivers/gpu/drm/xe/regs/xe_reg_defs.h |  4 +++-
>  drivers/gpu/drm/xe/xe_gt.c            |  7 +++----
>  drivers/gpu/drm/xe/xe_mmio.c          | 24 +++++++++++++-----------
>  drivers/gpu/drm/xe/xe_mmio.h          |  2 ++
>  4 files changed, 21 insertions(+), 16 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/regs/xe_reg_defs.h b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> index 89716172fbb8..26d00d825454 100644
> --- a/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> +++ b/drivers/gpu/drm/xe/regs/xe_reg_defs.h
> @@ -10,6 +10,8 @@
>  
>  #include "compat-i915-headers/i915_reg_defs.h"
>  
> +#define XE_REG_ADDR_WIDTH	22
> +
>  /**
>   * struct xe_reg - Register definition
>   *
> @@ -21,7 +23,7 @@ struct xe_reg {
>  	union {
>  		struct {
>  			/** @addr: address */
> -			u32 addr:22;
> +			u32 addr:XE_REG_ADDR_WIDTH;
>  			/**
>  			 * @masked: register is "masked", with upper 16bits used
>  			 * to identify the bits that are updated on the lower
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index 01a4a852b8f4..f10c1f5fbbe1 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -637,10 +637,9 @@ int xe_gt_init(struct xe_gt *gt)
>  void xe_gt_mmio_init(struct xe_gt *gt)
>  {
>  	struct xe_tile *tile = gt_to_tile(gt);
> +	struct xe_device *xe = tile_to_xe(tile);
>  
> -	gt->mmio.regs = tile->mmio.regs;
> -	gt->mmio.regs_size = tile->mmio.regs_size;
> -	gt->mmio.tile = tile;
> +	xe_mmio_init(&gt->mmio, tile, tile->mmio.regs, tile->mmio.regs_size);
>  
>  	if (gt->info.type == XE_GT_TYPE_MEDIA) {
>  		gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
> @@ -650,7 +649,7 @@ void xe_gt_mmio_init(struct xe_gt *gt)
>  		gt->mmio.adj_limit = 0;
>  	}
>  
> -	if (IS_SRIOV_VF(gt_to_xe(gt)))
> +	if (IS_SRIOV_VF(xe))
>  		gt->mmio.sriov_vf_gt = gt;
>  }
>  
> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
> index 3aed849a128b..8cee986947bc 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.c
> +++ b/drivers/gpu/drm/xe/xe_mmio.c
> @@ -55,7 +55,6 @@ static void tiles_fini(void *arg)
>  static void mmio_multi_tile_setup(struct xe_device *xe, size_t tile_mmio_size)
>  {
>  	struct xe_tile *tile;
> -	void __iomem *regs;
>  	u8 id;
>  
>  	/*
> @@ -94,13 +93,8 @@ 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.regs_size = SZ_4M;
> -		tile->mmio.regs = regs;
> -		tile->mmio.tile = tile;
> -		regs += tile_mmio_size;
> -	}
> +	for_each_remote_tile(tile, xe, id)
> +		xe_mmio_init(&tile->mmio, tile, xe->mmio.regs + id * tile_mmio_size, SZ_4M);
>  }
>  
>  int xe_mmio_probe_tiles(struct xe_device *xe)
> @@ -140,13 +134,21 @@ int xe_mmio_probe_early(struct xe_device *xe)
>  	}
>  
>  	/* Setup first tile; other tiles (if present) will be setup later. */
> -	root_tile->mmio.regs_size = SZ_4M;
> -	root_tile->mmio.regs = xe->mmio.regs;
> -	root_tile->mmio.tile = root_tile;
> +	xe_mmio_init(&root_tile->mmio, root_tile, xe->mmio.regs, SZ_4M);
>  
>  	return devm_add_action_or_reset(xe->drm.dev, mmio_fini, xe);
>  }
>  

missing kernel-doc for this new public function

> +void xe_mmio_init(struct xe_mmio *mmio, struct xe_tile *tile, void __iomem *ptr, u32 size)
> +{
> +	/* Validate that we can use XE_REG macro with this xe_mmio later */
> +	xe_assert(tile_to_xe(tile), size <= (1 << (XE_REG_ADDR_WIDTH + 1)));

you can use xe_tile_assert(tile, ...) here and BIT() instead of shift

but I'm not sure I follow the logic here: if in the future we will
change xe_reg.addr to more than 22 bits to allow new extended register
definitions for new platforms, this assert will trigger when we would
initialize older platforms with smaller MMIO BAR, no?

maybe better place for such assert would be in xe_mmio_read|write():

	xe_tile_assert(mmio->tile, reg.addr < mmio->regs_size);

and likely this new assert/ADDR_WIDTH could be added in separate patch

> +
> +	mmio->regs = ptr;
> +	mmio->regs_size = size;
> +	mmio->tile = tile;
> +}
> +
>  static void mmio_flush_pending_writes(struct xe_mmio *mmio)
>  {
>  #define DUMMY_REG_OFFSET	0x130030
> diff --git a/drivers/gpu/drm/xe/xe_mmio.h b/drivers/gpu/drm/xe/xe_mmio.h
> index b32e7ee4b23e..c151ba569003 100644
> --- a/drivers/gpu/drm/xe/xe_mmio.h
> +++ b/drivers/gpu/drm/xe/xe_mmio.h
> @@ -14,6 +14,8 @@ struct xe_reg;
>  int xe_mmio_probe_early(struct xe_device *xe);
>  int xe_mmio_probe_tiles(struct xe_device *xe);
>  
> +void xe_mmio_init(struct xe_mmio *mmio, struct xe_tile *tile, void __iomem *ptr, u32 size);
> +
>  u8 xe_mmio_read8(struct xe_mmio *mmio, struct xe_reg reg);
>  u16 xe_mmio_read16(struct xe_mmio *mmio, struct xe_reg reg);
>  void xe_mmio_write32(struct xe_mmio *mmio, struct xe_reg reg, u32 val);