Re: [PATCH v7 06/12] drm/xe: Improve xe_gt_sriov_pf_config GGTT handling

[Matthew Brost <matthew.brost@intel.com>]

On Wed, Oct 15, 2025 at 09:47:15AM +0200, Maarten Lankhorst wrote:
> Do not directly dereference xe_ggtt_node, and add
> a member to store the GGTT size.
> 
> Signed-off-by: Maarten Lankhorst <dev@lankhorst.se>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c       | 15 ++++++++-------
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config_types.h |  8 ++++++--
>  2 files changed, 14 insertions(+), 9 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> index 2289756761636..c0dfffd5c553b 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -264,7 +264,7 @@ static u32 encode_config_ggtt(u32 *cfg, const struct xe_gt_sriov_config *config,
>  	if (!xe_ggtt_node_allocated(node))
>  		return 0;
>  
> -	return encode_ggtt(cfg, node->base.start, node->base.size, details);
> +	return encode_ggtt(cfg, xe_ggtt_node_addr(node), config->ggtt_size, details);
>  }
>  
>  /* Return: number of configuration dwords written */
> @@ -495,13 +495,14 @@ static int pf_provision_vf_ggtt(struct xe_gt *gt, unsigned int vfid, u64 size)
>  
>  	xe_ggtt_assign(node, vfid);
>  	xe_gt_sriov_dbg_verbose(gt, "VF%u assigned GGTT %llx-%llx\n",
> -				vfid, node->base.start, node->base.start + node->base.size - 1);
> +				vfid, xe_ggtt_node_addr(node), xe_ggtt_node_addr(node) + size - 1);
>  
> -	err = pf_distribute_config_ggtt(gt->tile, vfid, node->base.start, node->base.size);
> +	err = pf_distribute_config_ggtt(gt->tile, vfid, xe_ggtt_node_addr(node), size);
>  	if (unlikely(err))
>  		goto err;
>  
>  	config->ggtt_region = node;
> +	config->ggtt_size = size;
>  	return 0;
>  err:
>  	pf_release_ggtt(tile, node);
> @@ -514,7 +515,7 @@ static u64 pf_get_vf_config_ggtt(struct xe_gt *gt, unsigned int vfid)
>  	struct xe_ggtt_node *node = config->ggtt_region;
>  
>  	xe_gt_assert(gt, xe_gt_is_main_type(gt));
> -	return xe_ggtt_node_allocated(node) ? node->base.size : 0;
> +	return xe_ggtt_node_allocated(node) ? config->ggtt_size : 0;
>  }
>  
>  /**
> @@ -2516,11 +2517,11 @@ int xe_gt_sriov_pf_config_print_ggtt(struct xe_gt *gt, struct drm_printer *p)
>  		if (!xe_ggtt_node_allocated(config->ggtt_region))
>  			continue;
>  
> -		string_get_size(config->ggtt_region->base.size, 1, STRING_UNITS_2,
> +		string_get_size(config->ggtt_size, 1, STRING_UNITS_2,
>  				buf, sizeof(buf));
>  		drm_printf(p, "VF%u:\t%#0llx-%#llx\t(%s)\n",
> -			   n, config->ggtt_region->base.start,
> -			   config->ggtt_region->base.start + config->ggtt_region->base.size - 1,
> +			   n, xe_ggtt_node_addr(config->ggtt_region),
> +			   xe_ggtt_node_addr(config->ggtt_region) + config->ggtt_size - 1,
>  			   buf);
>  	}
>  
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config_types.h b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config_types.h
> index 686c7b3b6d7a5..9a8e66c8b539f 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config_types.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config_types.h
> @@ -17,10 +17,14 @@ struct xe_bo;
>   * Used by the PF driver to maintain per-VF provisioning data.
>   */
>  struct xe_gt_sriov_config {
> -	/** @ggtt_region: GGTT region assigned to the VF. */
> -	struct xe_ggtt_node *ggtt_region;
>  	/** @lmem_obj: LMEM allocation for use by the VF. */
>  	struct xe_bo *lmem_obj;
> +
> +	/** @ggtt_region: GGTT region assigned to the VF. */
> +	struct xe_ggtt_node *ggtt_region;
> +	/** @ggtt_size: Size of GGTT region */
> +	u64 ggtt_size;

Nit: Couldn't we have a helper to dervive the size from the
xe_ggtt_node to avoid storing the ggtt_size explicitly?

> +

Longterm we should likely move the PF GGTT config to a per-tile
structure like we did for the VF GGTT config. IMO we can do that in
follow up. Maybe ping Michal for his thoughts here.

Anyways, this looks like a good cleanup for the existing code structure.

With that:
Reviewed-by: <Matthew.brost@intel.com> 

>  	/** @num_ctxs: number of GuC contexts IDs.  */
>  	u16 num_ctxs;
>  	/** @begin_ctx: start index of GuC context ID range. */
> -- 
> 2.51.0
>