[PATCH v2 2/2] drm/xe: Add xe_mmio_init() initialization function
Levi, Ilia
ilia.levi at intel.com
Sun Feb 9 13:34:01 UTC 2025
On 05/02/2025 17:56, Michal Wajdeczko wrote:
>
> On 02.02.2025 12:00, Ilia Levi wrote:
>> Add a convenience function for minimal initialization of struct xe_mmio.
>> This function also validates that the entirety of the provided mmio region
>> is usable with struct xe_reg.
>>
>> v2: Modify commit message, add kernel doc, refactor assert (Michal)
>>
>> Signed-off-by: Ilia Levi <ilia.levi at intel.com>
> Reviewed-by: Michal Wajdeczko <michal.wajdeczko at intel.com>
>
> with one nit below
>
>> ---
>> 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 | 33 ++++++++++++++++++---------
>> drivers/gpu/drm/xe/xe_mmio.h | 2 ++
>> 4 files changed, 30 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
> maybe we should also add a comment for this definition that we aim to
> support at most 8MB MMIO regions and define this as:
>
> #define XE_REG_ADDR_SPACE SZ_8M
>
> so we can use this definition as-is in the assert below and then for the
> xe_reg.addr bit width use const_ilog2(XE_REG_ADDR_SPACE) ?
It is a good idea, but the limitation comes from the fact that we use only
22 bits to represent the address, not because we want to support up to a
specific size. The limitation is imposed by the specific implementation -
if we used a u64 for the address, we wouldn't have any limitation at all.
Thus, I think reversing the computation would convey the wrong idea.
However, I've added and documented XE_MMIO_SIZE_MAX macro in v3. Let me know
if it makes more sense now.
- Ilia
>> +
>> /**
>> * 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(>->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..8ba314de02ce 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,30 @@ 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);
>> }
>>
>> +/**
>> + * xe_mmio_init() - Initialize an MMIO instance
>> + * @mmio: Pointer to the MMIO instance to initialize
>> + * @tile: The tile to which the MMIO region belongs
>> + * @ptr: Pointer to the start of the MMIO region
>> + * @size: The size of the MMIO region in bytes
>> + *
>> + * This is a convenience function for minimal initialization of struct xe_mmio.
>> + */
>> +void xe_mmio_init(struct xe_mmio *mmio, struct xe_tile *tile, void __iomem *ptr, u32 size)
>> +{
>> + /* Validate that the entire MMIO region is usable with struct xe_reg */
>> + xe_tile_assert(tile, size <= BIT(XE_REG_ADDR_WIDTH + 1));
>> +
>> + 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);
More information about the Intel-xe
mailing list