[PATCH] drm/xe: Refactor mmio setup for multi-tile
Lucas De Marchi
lucas.demarchi at intel.com
Tue Jul 16 22:03:42 UTC 2024
On Tue, Jul 16, 2024 at 11:51:05PM GMT, Michal Wajdeczko wrote:
>
>
>On 16.07.2024 21:54, Lucas De Marchi wrote:
>> Extract functions to setup the multi-tile mmio space and extension
>> space, while better documenting the final memory layout. No change in
>> behavior.
>>
>> Signed-off-by: Lucas De Marchi <lucas.demarchi at intel.com>
>> ---
>>
>> tested only on single-tile platforms for now. It shouldn't change the
>> behavior, but it would be good to get some extra testing.
>
>hmm, but CI likely wont help here as our PVC are running with:
>
><6>[ 149.783555] xe 0000:aa:00.0: [drm] tile_count: 2, reduced_tile_count 1
yes... I will have to do some manual testing and also asking for help
from original authors (Cc'ed) to check if it doesn't break their use
case.
>
>>
>> drivers/gpu/drm/xe/xe_mmio.c | 99 ++++++++++++++++++++++++++++--------
>> 1 file changed, 77 insertions(+), 22 deletions(-)
>>
>> diff --git a/drivers/gpu/drm/xe/xe_mmio.c b/drivers/gpu/drm/xe/xe_mmio.c
>> index ea3c37d3e13f..31f6703daec2 100644
>> --- a/drivers/gpu/drm/xe/xe_mmio.c
>> +++ b/drivers/gpu/drm/xe/xe_mmio.c
>> @@ -33,29 +33,56 @@ static void tiles_fini(void *arg)
>> tile->mmio.regs = NULL;
>> }
>>
>> -int xe_mmio_probe_tiles(struct xe_device *xe)
>> +/*
>> + * On multi-tile devices, partition the BAR space for MMIO on each tile,
>> + * possibly accounting for register override on the number of tiles available.
>> + * Resulting memory layout is like below:
>> + *
>> + * .----------------------. <- tile_count * tile_mmio_size
>> + * | .... |
>> + * |----------------------| <- 2 * tile_mmio_size
>> + * | tile1->mmio.regs |
>> + * |----------------------| <- 1 * tile_mmio_size
>> + * | tile0->mmio.regs |
>> + * '----------------------' <- 0MB
>> + */
>> +static void mmio_multi_tile_setup(struct xe_device *xe, size_t tile_mmio_size)
>> {
>> - size_t tile_mmio_size = SZ_16M, tile_mmio_ext_size = xe->info.tile_mmio_ext_size;
>> - u8 id, tile_count = xe->info.tile_count;
>> - struct xe_gt *gt = xe_root_mmio_gt(xe);
>> struct xe_tile *tile;
>> void __iomem *regs;
>> - u32 mtcfg;
>> + u8 id;
>>
>> - if (tile_count == 1)
>> - goto add_mmio_ext;
>> + /*
>> + * Nothing to be done as tile 0 has already been setup earlier with the
>> + * entire BAR mapped - see xe_mmio_init()
>> + */
>> + if (xe->info.tile_count == 1)
>> + return;
>>
>> + /* Possibly override number of tile based on configuration register */
>> if (!xe->info.skip_mtcfg) {
>> + struct xe_gt *gt = xe_root_mmio_gt(xe);
>> + u8 tile_count = xe->info.tile_count;
>> + u32 mtcfg;
>> +
>> + /*
>> + * Although the per-tile mmio regs are not yet initialized, this
>> + * is fine as it's going to the root gt, that's guaranteed to be
>> + * initialized earlier in xe_mmio_init()
>> + */
>> mtcfg = xe_mmio_read64_2x32(gt, XEHP_MTCFG_ADDR);
>> tile_count = REG_FIELD_GET(TILE_COUNT, mtcfg) + 1;
>> +
>> if (tile_count < xe->info.tile_count) {
>> drm_info(&xe->drm, "tile_count: %d, reduced_tile_count %d\n",
>> xe->info.tile_count, tile_count);
>> xe->info.tile_count = tile_count;
>>
>> /*
>> - * FIXME: Needs some work for standalone media, but should be impossible
>> - * with multi-tile for now.
>> + * FIXME: Needs some work for standalone media, but
>> + * should be impossible with multi-tile for now:
>> + * multi-tile platform with standalone media doesn't
>> + * exist
>> */
>> xe->info.gt_count = xe->info.tile_count;
>> }
>> @@ -67,23 +94,51 @@ int xe_mmio_probe_tiles(struct xe_device *xe)
>> tile->mmio.regs = regs;
>> regs += tile_mmio_size;
>> }
>> +}
>>
>> -add_mmio_ext:
>> - /*
>> - * By design, there's a contiguous multi-tile MMIO space (16MB hard coded per tile).
>> - * When supported, there could be an additional contiguous multi-tile MMIO extension
>> - * space ON TOP of it, and hence the necessity for distinguished MMIO spaces.
>> - */
>> - if (xe->info.has_mmio_ext) {
>> - regs = xe->mmio.regs + tile_mmio_size * tile_count;
>> +/*
>> + * 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;
>>
>> - for_each_tile(tile, xe, id) {
>> - tile->mmio_ext.size = tile_mmio_ext_size;
>> - tile->mmio_ext.regs = regs;
>> + if (!xe->info.has_mmio_ext)
>> + return;
>>
>> - regs += tile_mmio_ext_size;
>> - }
>> + regs = xe->mmio.regs + tile_mmio_size * xe->info.tile_count;
>> + for_each_tile(tile, xe, id) {
>> + tile->mmio_ext.size = tile_mmio_ext_size;
>> + tile->mmio_ext.regs = regs;
>> + regs += tile_mmio_ext_size;
>> }
>> +}
>> +
>
>as you are adding some documentation to static functions, maybe you can
>also add true kernel-doc for xe_mmio_probe_tiles() below ?
will do
>
>> +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);
>
>btw, I'm wondering if we really need this tiles_fini as it just reset
>some pointers, while in general we don't do such detailed cleanup any
>more, since actions are called in order and only care about resources
c7117419784f ("drm/xe: reset mmio mappings with devm") added this on
purpose and it seems like a good reasoning.
Lucas De Marchi
>
>> }
More information about the Intel-xe
mailing list