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

[Michal Wajdeczko <michal.wajdeczko@intel.com>]

On 10/16/2025 12:04 AM, Matthew Brost wrote:
> 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?

+1 (as already commented)

> 
>> +
> 
> 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.

I have almost finished series with GGTT/LMEM changes, but switched to
higher priority task (provisioning over sysfs), hopefully will be able
to return to this topic soon

> 
> 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
>>