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 3:46 PM, Maarten Lankhorst wrote:
> Hey,
> 
> Den 2025-10-16 kl. 10:50, skrev Michal Wajdeczko:
>>
>>
>> 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> 
> 
> All other users of xe_ggtt_node are not using the size member, would it

because they don't interact with GGTT directly, and they can get the size
from the xe_bo itself

while in case of the PF, there is no BOs for those "allocations"
that's why PF directly uses xe_ggtt_node_insert() that doesn't take xe_bo*

> be too terrible to maintain it in a separate member for SRIOV PF
> provisioning, instead of exposing it too?

but why forcing the PF to cache something that the node already has (for free)

> 
> Seems to me that the size of the ggtt_node is a detail, that we just
> happened to expose and use in xe_sriov_pf_config. If necessary I can
> keep maintaining that API, but it feels cleaner not to here.

exposing oneline function shouldn't be a huge maintenance cost IMO

> 
> Kind regards,
> ~Maarten Lankhorst