Re: [PATCH v3 1/2] drm/xe/vf: Split GGTT info query and fixup into separate helpers

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

On Tue, Nov 18, 2025 at 12:47:40PM +0000, Satyanarayana K V P wrote:
> The current vf_get_ggtt_info() function handles both querying GGTT
> information and applying fixups. Refactor this into two distinct
> functions: one for querying and another for performing fixups.
> 
> Signed-off-by: Satyanarayana K V P <satyanarayana.k.v.p@intel.com>
> Cc: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Michal Winiarski <michal.winiarski@intel.com>
> Cc: Matthew Brost <matthew.brost@intel.com>
> Cc: Tomasz Lis <tomasz.lis@intel.com>
> 
> ---
> V2 -> V3:
> - New commit
> 
> V1 -> V2:
> - None.
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_vf.c | 45 ++++++++++++++++++++---------
>  1 file changed, 31 insertions(+), 14 deletions(-)
> 
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> index 4c73a077d314..be7986537aad 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_vf.c
> @@ -438,41 +438,50 @@ u32 xe_gt_sriov_vf_gmdid(struct xe_gt *gt)
>  	return value;
>  }
>  
> -static int vf_get_ggtt_info(struct xe_gt *gt)
> +static int vf_query_ggtt_info(struct xe_gt *gt, u64 *start, u64 *size, s64 *shift)
>  {
>  	struct xe_tile *tile = gt_to_tile(gt);
> -	struct xe_ggtt *ggtt = tile->mem.ggtt;
>  	struct xe_guc *guc = &gt->uc.guc;
> -	u64 start, size, ggtt_size;
> -	s64 shift;
> +	u64 ggtt_size;
>  	int err;
>  
>  	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
>  
> -	guard(mutex)(&ggtt->lock);
> -
> -	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_START_KEY, &start);
> +	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_START_KEY, start);
>  	if (unlikely(err))
>  		return err;
>  
> -	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_SIZE_KEY, &size);
> +	err = guc_action_query_single_klv64(guc, GUC_KLV_VF_CFG_GGTT_SIZE_KEY, size);
>  	if (unlikely(err))
>  		return err;
>  
> -	if (!size)
> +	if (!*size)
>  		return -ENODATA;
>  
>  	ggtt_size = xe_tile_sriov_vf_ggtt(tile);
> -	if (ggtt_size && ggtt_size != size) {
> +	if (ggtt_size && ggtt_size != *size) {
>  		xe_gt_sriov_err(gt, "Unexpected GGTT reassignment: %lluK != %lluK\n",
> -				size / SZ_1K, ggtt_size / SZ_1K);
> +				*size / SZ_1K, ggtt_size / SZ_1K);
>  		return -EREMCHG;
>  	}
>  
>  	xe_gt_sriov_dbg_verbose(gt, "GGTT %#llx-%#llx = %lluK\n",
> -				start, start + size - 1, size / SZ_1K);
> +				*start, *start + *size - 1, *size / SZ_1K);
> +
> +	*shift = *start - (s64)xe_tile_sriov_vf_ggtt_base(tile);

I don't think this split works. The shift calculation is the critical
path between 2 GTs running in parallel doing a VF restore. So I believe
vf_query_ggtt_info should just return start, size and shift calculation
should be done in vf_fixup_ggtt_nodes under the ggtt->lock. Without that
it would be possible to shift the GGTT twice on a VF restore which would
break everything.

Matt

> +
> +	return 0;
> +}
> +
> +static int vf_fixup_ggtt_nodes(struct xe_gt *gt, u64 start, u64 size, s64 shift)
> +{
> +	struct xe_tile *tile = gt_to_tile(gt);
> +	struct xe_ggtt *ggtt = tile->mem.ggtt;
> +
> +	xe_gt_assert(gt, IS_SRIOV_VF(gt_to_xe(gt)));
> +
> +	guard(mutex)(&ggtt->lock);
>  
> -	shift = start - (s64)xe_tile_sriov_vf_ggtt_base(tile);
>  	xe_tile_sriov_vf_ggtt_base_store(tile, start);
>  	xe_tile_sriov_vf_ggtt_store(tile, size);
>  
> @@ -577,9 +586,17 @@ static void vf_cache_gmdid(struct xe_gt *gt)
>  int xe_gt_sriov_vf_query_config(struct xe_gt *gt)
>  {
>  	struct xe_device *xe = gt_to_xe(gt);
> +	u64 start, size;
> +	s64 shift;
>  	int err;
>  
> -	err = vf_get_ggtt_info(gt);
> +	err = vf_query_ggtt_info(gt, &start, &size, &shift);
> +	if (unlikely(err)) {
> +		xe_gt_sriov_err(gt, "Error in querying ggtt info (%d)\n", err);
> +		return err;
> +	}
> +
> +	err = vf_fixup_ggtt_nodes(gt, start, size, shift);
>  	if (unlikely(err))
>  		return err;
>  
> -- 
> 2.51.0
>