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

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

Piotr Piórkowski <piotr.piorkowski@intel.com> wrote on śro [2025-paź-29 09:47:07 +0100]:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on wto [2025-paź-28 18:58:17 +0100]:
> > In upcoming patches we will want to configure VF's execution
> > quantum (EQ) on all GTs under single lock to avoid potential
> > races in 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 | 58 +++++++++++++++++-----
> >  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h |  4 ++
> >  2 files changed, 49 insertions(+), 13 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 c0c0215c0703..717f81e76b8c 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
> > @@ -1732,29 +1732,65 @@ static int pf_get_exec_quantum(struct xe_gt *gt, unsigned int vfid)
> >  }
> >  
> >  /**
> > - * xe_gt_sriov_pf_config_set_exec_quantum - Configure execution quantum for the VF.
> > + * xe_gt_sriov_pf_config_set_exec_quantum_locked() - Configure execution quantum of the VF.
> >   * @gt: the &xe_gt
> >   * @vfid: the VF identifier
> >   * @exec_quantum: requested execution quantum in milliseconds (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_exec_quantum(struct xe_gt *gt, unsigned int vfid,
> > -					   u32 exec_quantum)
> > +int xe_gt_sriov_pf_config_set_exec_quantum_locked(struct xe_gt *gt, unsigned int vfid,
> > +						  u32 exec_quantum)
> >  {
> >  	int err;
> >  
> > -	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
> > +	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
> > +
> >  	err = pf_provision_exec_quantum(gt, vfid, exec_quantum);
> > -	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
> >  
> >  	return pf_config_set_u32_done(gt, vfid, exec_quantum,
> > -				      xe_gt_sriov_pf_config_get_exec_quantum(gt, vfid),
> > +				      pf_get_exec_quantum(gt, vfid),
> >  				      "execution quantum", exec_quantum_unit, err);
> >  }
> >  
> > +/**
> > + * xe_gt_sriov_pf_config_set_exec_quantum - Configure execution quantum for the VF.
> > + * @gt: the &xe_gt
> > + * @vfid: the VF identifier
> > + * @exec_quantum: requested execution quantum in milliseconds (0 is infinity)
> > + *
> > + * This function can only be called on PF.
> > + * It will log the provisioned value or na error in case of the failure.
> 
> typo: na -> an
> > + *
> > + * Return: 0 on success or a negative error code on failure.
> > + */
> > +int xe_gt_sriov_pf_config_set_exec_quantum(struct xe_gt *gt, unsigned int vfid,
> > +					   u32 exec_quantum)
> > +{
> > +	guard(mutex)(xe_gt_sriov_pf_master_mutex(gt));
> > +
> > +	return xe_gt_sriov_pf_config_set_exec_quantum_locked(gt, vfid, exec_quantum);
> > +}
> > +
> > +/**
> > + * xe_gt_sriov_pf_config_get_exec_quantum_locked() - Get VF's execution quantum.
> > + * @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) execution quantum in milliseconds.
> > + */
> > +u32 xe_gt_sriov_pf_config_get_exec_quantum_locked(struct xe_gt *gt, unsigned int vfid)
> > +{
> > +	lockdep_assert_held(xe_gt_sriov_pf_master_mutex(gt));
> > +
> > +	return pf_get_exec_quantum(gt, vfid);
> 
> NIT: Perhaps it would have been better to be consistent and call it the locked version here.
> 
> > +}
> > +
> >  /**
> >   * xe_gt_sriov_pf_config_get_exec_quantum - Get VF's execution quantum.
> >   * @gt: the &xe_gt
> > @@ -1766,13 +1802,9 @@ int xe_gt_sriov_pf_config_set_exec_quantum(struct xe_gt *gt, unsigned int vfid,
> >   */
> >  u32 xe_gt_sriov_pf_config_get_exec_quantum(struct xe_gt *gt, unsigned int vfid)
> >  {
> > -	u32 exec_quantum;
> > +	guard(mutex)(xe_gt_sriov_pf_master_mutex(gt));
> >  
> > -	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
> > -	exec_quantum = pf_get_exec_quantum(gt, vfid);
> > -	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
> > -
> > -	return exec_quantum;
> > +	return pf_get_exec_quantum(gt, vfid);
> >  }
> >  
> >  static const char *preempt_timeout_unit(u32 preempt_timeout)
> > 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 513e6512a575..b4beb5a97031 100644
> > --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
> > +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
> > @@ -40,6 +40,10 @@ int xe_gt_sriov_pf_config_bulk_set_lmem(struct xe_gt *gt, unsigned int vfid, uns
> >  u32 xe_gt_sriov_pf_config_get_exec_quantum(struct xe_gt *gt, unsigned int vfid);
> >  int xe_gt_sriov_pf_config_set_exec_quantum(struct xe_gt *gt, unsigned int vfid, u32 exec_quantum);
> >  
> > +u32 xe_gt_sriov_pf_config_get_exec_quantum_locked(struct xe_gt *gt, unsigned int vfid);
> > +int xe_gt_sriov_pf_config_set_exec_quantum_locked(struct xe_gt *gt, unsigned int vfid,
> > +						  u32 exec_quantum);
> > +
> >  u32 xe_gt_sriov_pf_config_get_preempt_timeout(struct xe_gt *gt, unsigned int vfid);
> >  int xe_gt_sriov_pf_config_set_preempt_timeout(struct xe_gt *gt, unsigned int vfid,
> >  					      u32 preempt_timeout);
> 
> but anyway it looks good:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

one more comment about kernel-doc:
You are inconsistent when writing function names in kernel-doc. Sometimes you use
parentheses and sometimes you don't:

+ * xe_gt_sriov_pf_config_set_exec_quantum - Configure execution quantum for the VF.
vs
+ * xe_gt_sriov_pf_config_get_exec_quantum_locked() - Get VF's execution quantum.

Based on https://docs.kernel.org/doc-guide/kernel-doc.html#function-documentation
I assume that the version with parentheses is correct.

Thanks,
Piotr

> 
> > -- 
> > 2.47.1
> > 
> 
> -- 

--