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

["Levi, Ilia" <ilia.levi@intel.com>]

On 30/01/2025 21:54, Michal Wajdeczko wrote:
>
> 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 ?


The addr field of struct xe_reg is currently 22 bits, i. e. the biggest offset
that can be used within xe_mmio is up to 8MiB. This means that there's
no sense in instantiating xe_mmio bigger than 8MiB, as xe_mmio_read &
xe_mmio_write functions will not be able to access a register placed at
an offset after 8MiB. Hence, I validate that xe_mmio is no larger than 8MiB
(~ small enough). This patch is in continuation of the discussion I had with Matt in
https://lore.kernel.org/all/55e647fd-08b0-4395-8fb0-ad53ac3f9be7@intel.com
(specifically my 2nd comment there).

- Ilia

>
>> Signed-off-by: Ilia Levi <ilia.levi@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

See my explanation above, I check the other direction - that xe_mmio is not too big to be
addressed with 22 bits. You're suggesting a check that the provided register offset, while
still being 22 bits, is smaller than xe_mmio size. It sounds like a good check to add, but
different from the concern that I had. I'll implement it in an additional patch.

- Ilia

>
>> +
>> +	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);