Re: [PATCH v2 04/17] drm/xe/pf: Add _locked variants of the VF PT config functions

["Piotr Piórkowski" <piotr.piorkowski@intel.com>]

Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on wto [2025-paź-28 18:58:18 +0100]:
> In upcoming patches we will want to configure VF's preemption
> timeout (PT) on all GTs under single lock to avoid potential
> races due to parallel GT configuration attempts.
> 
> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
> Cc: Lucas De Marchi <lucas.demarchi@intel.com>
> ---
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c | 59 +++++++++++++++++-----
>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h |  4 ++
>  2 files changed, 49 insertions(+), 14 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 717f81e76b8c..e48457bd7d12 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> @@ -1835,31 +1835,66 @@ static int pf_get_preempt_timeout(struct xe_gt *gt, unsigned int vfid)
>  }
>  
>  /**
> - * xe_gt_sriov_pf_config_set_preempt_timeout - Configure preemption timeout for the VF.
> + * xe_gt_sriov_pf_config_set_preempt_timeout_locked() - Configure preemption timeout of the VF.
>   * @gt: the &xe_gt
>   * @vfid: the VF identifier
>   * @preempt_timeout: requested preemption timeout in microseconds (0 is infinity)
>   *
> - * This function can only be called on PF.
> + * This function can only be called on PF with the master mutex hold.
> + * It will log the provisioned value or an error in case of the failure.
>   *
>   * Return: 0 on success or a negative error code on failure.
>   */
> -int xe_gt_sriov_pf_config_set_preempt_timeout(struct xe_gt *gt, unsigned int vfid,
> -					      u32 preempt_timeout)
> +int xe_gt_sriov_pf_config_set_preempt_timeout_locked(struct xe_gt *gt, unsigned int vfid,
> +						     u32 preempt_timeout)
>  {
>  	int err;
>  
> -	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
> +	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
> +
>  	err = pf_provision_preempt_timeout(gt, vfid, preempt_timeout);
> -	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
>  
>  	return pf_config_set_u32_done(gt, vfid, preempt_timeout,
> -				      xe_gt_sriov_pf_config_get_preempt_timeout(gt, vfid),
> +				      pf_get_preempt_timeout(gt, vfid),
>  				      "preemption timeout", preempt_timeout_unit, err);
>  }
>  
>  /**
> - * xe_gt_sriov_pf_config_get_preempt_timeout - Get VF's preemption timeout.
> + * xe_gt_sriov_pf_config_set_preempt_timeout() - Configure preemption timeout of the VF.
> + * @gt: the &xe_gt
> + * @vfid: the VF identifier
> + * @preempt_timeout: requested preemption timeout in microseconds (0 is infinity)
> + *
> + * This function can only be called on PF.
> + *
> + * Return: 0 on success or a negative error code on failure.
> + */
> +int xe_gt_sriov_pf_config_set_preempt_timeout(struct xe_gt *gt, unsigned int vfid,
> +					      u32 preempt_timeout)
> +{
> +	guard(mutex)(xe_gt_sriov_pf_master_mutex(gt));
> +
> +	return xe_gt_sriov_pf_config_set_preempt_timeout_locked(gt, vfid, preempt_timeout);
> +}
> +
> +/**
> + * xe_gt_sriov_pf_config_get_preempt_timeout_locked() - Get VF's preemption timeout.

NIT: I'm wondering, since you write in the return section that this also applies to PF,
and here you write about "VF's preemption...", maybe it would be worth using some universal
phrase:
at first I'm I thought of “function,” but without mentioning that this is about “PCI function,”
it might confuse things even more.
I don't know, I'm just mentioning it so that you might consider changing
"VF's” to something else.
But you can just as well ignore it

> + * @gt: the &xe_gt
> + * @vfid: the VF identifier
> + *
> + * This function can only be called on PF with the master mutex hold.
> + *
> + * Return: VF's (or PF's) preemption timeout in microseconds.
> + */
> +u32 xe_gt_sriov_pf_config_get_preempt_timeout_locked(struct xe_gt *gt, unsigned int vfid)
> +{
> +	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
> +
> +	return pf_get_preempt_timeout(gt, vfid);
> +}
> +
> +/**
> + * xe_gt_sriov_pf_config_get_preempt_timeout() - Get VF's preemption timeout.
>   * @gt: the &xe_gt
>   * @vfid: the VF identifier
>   *
> @@ -1869,13 +1904,9 @@ int xe_gt_sriov_pf_config_set_preempt_timeout(struct xe_gt *gt, unsigned int vfi
>   */
>  u32 xe_gt_sriov_pf_config_get_preempt_timeout(struct xe_gt *gt, unsigned int vfid)
>  {
> -	u32 preempt_timeout;
> +	guard(mutex)(xe_gt_sriov_pf_master_mutex(gt));
>  
> -	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
> -	preempt_timeout = pf_get_preempt_timeout(gt, vfid);
> -	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
> -
> -	return preempt_timeout;
> +	return pf_get_preempt_timeout(gt, vfid);

NIT: I mentioned this in the review of previous patch:
     Perhaps it would have been better to be consistent and call it the locked version here.

>  }
>  
>  static const char *sched_priority_unit(u32 priority)
> diff --git a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
> index b4beb5a97031..6bab5ad6c849 100644
> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
> @@ -48,6 +48,10 @@ u32 xe_gt_sriov_pf_config_get_preempt_timeout(struct xe_gt *gt, unsigned int vfi
>  int xe_gt_sriov_pf_config_set_preempt_timeout(struct xe_gt *gt, unsigned int vfid,
>  					      u32 preempt_timeout);
>  
> +u32 xe_gt_sriov_pf_config_get_preempt_timeout_locked(struct xe_gt *gt, unsigned int vfid);
> +int xe_gt_sriov_pf_config_set_preempt_timeout_locked(struct xe_gt *gt, unsigned int vfid,
> +						     u32 preempt_timeout);
> +
>  u32 xe_gt_sriov_pf_config_get_sched_priority(struct xe_gt *gt, unsigned int vfid);
>  int xe_gt_sriov_pf_config_set_sched_priority(struct xe_gt *gt, unsigned int vfid, u32 priority);
>  

LGTM:
Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

> -- 
> 2.47.1
> 

--