Re: [PATCH] drm/xe: Defer gt->mmio initialization until after multi-tile setup

["Pottumuttu, Sai Teja" <sai.teja.pottumuttu@intel.com>]

On 18-09-2024 03:46, Matt Roper wrote:
> With the recent xe_mmio redesign, tiles and GTs each have their own MMIO
> accessor, with the GT inheriting some of the information (such as the
> iomap pointer) from their containing tile.  Given that non-root tiles
> get initialized later than the root tile (and currently after the point
> at which GT MMIO is initialized for _all_ GTs), we wind up incorrectly
> inheriting uninitialized pointers for the initialization of GT MMIO for
> GTs that reside on non-root tiles.  This causes a driver crash on
> multi-tile PVC platforms.
>
> With the general xe_mmio redesign, it's now only necessary to do the
> GT-level MMIO setup before the point we start reading/writing GT
> registers.  Move initialization of gt->mmio out of xe_info_init (which
> runs before non-root tiles are initialized) and to the beginning of
> where we start actually accessing the GTs themselves.
>
> The high-level initialization flow now boils down to:
>   - General device init, software-only setup
>   - (no register access possible yet)
>   - Root tile initialization
>   - (access to device/tile0 registers possible via xe_root_tile_mmio())
>   - Initialization of non-root tiles
>   - (access to any tile's registers possible via tile->mmio)
>   - GT MMIO initialization, inheriting iomap from each GT's tile
>   - (access to any GT's registers possible via gt->mmio)
>
> Fixes: fa599b8c95a7 ("drm/xe: Populate GT's mmio iomap from tile during init")
> Reported-by: John Harrison <John.C.Harrison@Intel.com>
> Signed-off-by: Matt Roper <matthew.d.roper@intel.com>

Looks good to me,

Reviewed-by: Sai Teja Pottumuttu <sai.teja.pottumuttu@intel.com>

> ---
>   drivers/gpu/drm/xe/xe_device.c |  7 +++++++
>   drivers/gpu/drm/xe/xe_gt.c     | 24 ++++++++++++++++++++++++
>   drivers/gpu/drm/xe/xe_gt.h     |  1 +
>   drivers/gpu/drm/xe/xe_pci.c    | 12 ------------
>   4 files changed, 32 insertions(+), 12 deletions(-)
>
> diff --git a/drivers/gpu/drm/xe/xe_device.c b/drivers/gpu/drm/xe/xe_device.c
> index 4d3c794f134c..998165962cad 100644
> --- a/drivers/gpu/drm/xe/xe_device.c
> +++ b/drivers/gpu/drm/xe/xe_device.c
> @@ -656,6 +656,13 @@ int xe_device_probe(struct xe_device *xe)
>   		err = xe_gt_init_early(gt);
>   		if (err)
>   			return err;
> +
> +		/*
> +		 * Only after this point can GT-specific MMIO operations
> +		 * (including things like communication with the GuC)
> +		 * be performed.
> +		 */
> +		xe_gt_mmio_init(gt);
>   	}
>   
>   	for_each_tile(tile, xe, id) {
> diff --git a/drivers/gpu/drm/xe/xe_gt.c b/drivers/gpu/drm/xe/xe_gt.c
> index b32fb3fdf80d..9b0218109647 100644
> --- a/drivers/gpu/drm/xe/xe_gt.c
> +++ b/drivers/gpu/drm/xe/xe_gt.c
> @@ -624,6 +624,30 @@ int xe_gt_init(struct xe_gt *gt)
>   	return 0;
>   }
>   
> +/**
> + * xe_gt_mmio_init() - Initialize GT's MMIO access
> + * @gt: the GT object
> + *
> + * Initialize GT's MMIO accessor, which will be used to access registers inside
> + * this GT.
> + */
> +void xe_gt_mmio_init(struct xe_gt *gt)
> +{
> +	struct xe_tile *tile = gt_to_tile(gt);
> +
> +	gt->mmio.regs = tile->mmio.regs;
> +	gt->mmio.regs_size = tile->mmio.regs_size;
> +	gt->mmio.tile = tile;
> +
> +	if (gt->info.type == XE_GT_TYPE_MEDIA) {
> +		gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
> +		gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
> +	}
> +
> +	if (IS_SRIOV_VF(gt_to_xe(gt)))
> +		gt->mmio.sriov_vf_gt = gt;
> +}
> +
>   void xe_gt_record_user_engines(struct xe_gt *gt)
>   {
>   	struct xe_hw_engine *hwe;
> diff --git a/drivers/gpu/drm/xe/xe_gt.h b/drivers/gpu/drm/xe/xe_gt.h
> index 97def44afa4c..05c8351d0f39 100644
> --- a/drivers/gpu/drm/xe/xe_gt.h
> +++ b/drivers/gpu/drm/xe/xe_gt.h
> @@ -37,6 +37,7 @@ struct xe_gt *xe_gt_alloc(struct xe_tile *tile);
>   int xe_gt_init_hwconfig(struct xe_gt *gt);
>   int xe_gt_init_early(struct xe_gt *gt);
>   int xe_gt_init(struct xe_gt *gt);
> +void xe_gt_mmio_init(struct xe_gt *gt);
>   void xe_gt_declare_wedged(struct xe_gt *gt);
>   int xe_gt_record_default_lrcs(struct xe_gt *gt);
>   
> diff --git a/drivers/gpu/drm/xe/xe_pci.c b/drivers/gpu/drm/xe/xe_pci.c
> index 5ba4ec229494..2afa05799b8d 100644
> --- a/drivers/gpu/drm/xe/xe_pci.c
> +++ b/drivers/gpu/drm/xe/xe_pci.c
> @@ -721,11 +721,6 @@ static int xe_info_init(struct xe_device *xe,
>   		gt->info.type = XE_GT_TYPE_MAIN;
>   		gt->info.has_indirect_ring_state = graphics_desc->has_indirect_ring_state;
>   		gt->info.engine_mask = graphics_desc->hw_engine_mask;
> -		gt->mmio.regs = tile->mmio.regs;
> -		gt->mmio.regs_size = tile->mmio.regs_size;
> -		gt->mmio.tile = tile;
> -		if (IS_SRIOV_VF(xe))
> -			gt->mmio.sriov_vf_gt = gt;
>   
>   		if (MEDIA_VER(xe) < 13 && media_desc)
>   			gt->info.engine_mask |= media_desc->hw_engine_mask;
> @@ -745,13 +740,6 @@ static int xe_info_init(struct xe_device *xe,
>   		gt->info.type = XE_GT_TYPE_MEDIA;
>   		gt->info.has_indirect_ring_state = media_desc->has_indirect_ring_state;
>   		gt->info.engine_mask = media_desc->hw_engine_mask;
> -		gt->mmio.regs = tile->mmio.regs;
> -		gt->mmio.regs_size = tile->mmio.regs_size;
> -		gt->mmio.adj_offset = MEDIA_GT_GSI_OFFSET;
> -		gt->mmio.adj_limit = MEDIA_GT_GSI_LENGTH;
> -		gt->mmio.tile = tile;
> -		if (IS_SRIOV_VF(xe))
> -			gt->mmio.sriov_vf_gt = gt;
>   
>   		/*
>   		 * FIXME: At the moment multi-tile and standalone media are