Re: [PATCH 4/8] drm/xe/pf: Introduce functions to configure VF thresholds

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

On 14.05.2024 10:31, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on pon [2024-maj-06 15:38:10 +0200]:
>> The GuC firmware monitors VF's activity and notifies the PF driver
>> once any configured threshold related to such activity is exceeded.
>> Add functions to allow configuration of these thresholds per VF.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c    | 87 +++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h    |  6 ++
>>  .../gpu/drm/xe/xe_gt_sriov_pf_config_types.h  |  4 +
>>  3 files changed, 97 insertions(+)
>>
>> 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 7eac01e04cc5..fd35375ccd1e 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.c
>> @@ -25,6 +25,7 @@
>>  #include "xe_guc_fwif.h"
>>  #include "xe_guc_id_mgr.h"
>>  #include "xe_guc_klv_helpers.h"
>> +#include "xe_guc_klv_thresholds_set.h"
>>  #include "xe_guc_submit.h"
>>  #include "xe_lmtt.h"
>>  #include "xe_map.h"
>> @@ -208,6 +209,15 @@ static int pf_push_vf_cfg_lmem(struct xe_gt *gt, unsigned int vfid, u64 size)
>>  	return pf_push_vf_cfg_u64(gt, vfid, GUC_KLV_VF_CFG_LMEM_SIZE_KEY, size);
>>  }
>>  
>> +static int pf_push_vf_cfg_threshold(struct xe_gt *gt, unsigned int vfid,
>> +				    enum xe_guc_klv_threshold_index index, u32 value)
>> +{
>> +	u32 key = xe_guc_klv_threshold_index_to_key(index);
>> +
>> +	xe_gt_assert(gt, key);
>> +	return pf_push_vf_cfg_u32(gt, vfid, key, value);
>> +}
>> +
>>  static struct xe_gt_sriov_config *pf_pick_vf_config(struct xe_gt *gt, unsigned int vfid)
>>  {
>>  	xe_gt_assert(gt, IS_SRIOV_PF(gt_to_xe(gt)));
>> @@ -1748,6 +1758,83 @@ static void pf_reset_config_sched(struct xe_gt *gt, struct xe_gt_sriov_config *c
>>  	config->preempt_timeout = 0;
>>  }
>>  
>> +static int pf_provision_threshold(struct xe_gt *gt, unsigned int vfid,
>> +				  enum xe_guc_klv_threshold_index index, u32 value)
>> +{
>> +	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
>> +	int err;
>> +
>> +	err = pf_push_vf_cfg_threshold(gt, vfid, index, value);
>> +	if (unlikely(err))
>> +		return err;
>> +
>> +	config->thresholds[index] = value;
>> +
>> +	return 0;
>> +}
>> +
>> +static int pf_get_threshold(struct xe_gt *gt, unsigned int vfid,
>> +			    enum xe_guc_klv_threshold_index index)
>> +{
>> +	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
>> +
>> +	return config->thresholds[index];
>> +}
>> +
>> +static const char *threshold_unit(u32 threshold)
>> +{
>> +	return threshold ? "" : "(disabled)";
>> +}
>> +
>> +/**
>> + * xe_gt_sriov_pf_config_set_threshold - Configure threshold for the VF.
>> + * @gt: the &xe_gt
>> + * @vfid: the VF identifier
>> + * @threshold: the threshold index
>> + * @value: requested value (0 means disabled)
>> + *
>> + * 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_threshold(struct xe_gt *gt, unsigned int vfid,
>> +					enum xe_guc_klv_threshold_index index, u32 value)
>> +{
>> +	u32 key = xe_guc_klv_threshold_index_to_key(index);
>> +	const char *name = xe_guc_klv_key_to_string(key);
>> +	int err;
>> +
>> +	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
>> +	err = pf_provision_threshold(gt, vfid, index, value);
>> +	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
>> +
>> +	return pf_config_set_u32_done(gt, vfid, value,
>> +				      xe_gt_sriov_pf_config_get_threshold(gt, vfid, index),
>> +				      name, threshold_unit, err);
>> +}
>> +
>> +/**
>> + * xe_gt_sriov_pf_config_get_threshold - Get VF's preemption timeout.
> 
> Wrong description.

oops, copy-paste mistake

> 
>> + * @gt: the &xe_gt
>> + * @vfid: the VF identifier
>> + * @index: the threshold index
>> + *
>> + * This function can only be called on PF.
> 
> If these two functions (set/get) can be called only by PF, then maybe it is worth
> to give somewhere an assert that verifies it ?

there are already few indirect asserts here, first being inside
xe_gt_sriov_pf_master_mutex, and usually I only keep explicit assert
only when accessing some data from xe.sriov.pf or gt.sriov.pf

> 
>> + *
>> + * Return: value of VF's (or PF's) threshold.
>> + */
>> +u32 xe_gt_sriov_pf_config_get_threshold(struct xe_gt *gt, unsigned int vfid,
>> +					enum xe_guc_klv_threshold_index index)
>> +{
>> +	u32 value;
>> +
>> +	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
>> +	value = pf_get_threshold(gt, vfid, index);
>> +	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
>> +
>> +	return value;
>> +}
>> +
>>  static void pf_release_vf_config(struct xe_gt *gt, unsigned int vfid)
>>  {
>>  	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
>> 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 5e6b36f00b5b..e8238c1ad06a 100644
>> --- a/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
>> +++ b/drivers/gpu/drm/xe/xe_gt_sriov_pf_config.h
>> @@ -8,6 +8,7 @@
>>  
>>  #include <linux/types.h>
>>  
>> +enum xe_guc_klv_threshold_index;
>>  struct drm_printer;
>>  struct xe_gt;
>>  
>> @@ -43,6 +44,11 @@ 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_threshold(struct xe_gt *gt, unsigned int vfid,
>> +					enum xe_guc_klv_threshold_index index);
>> +int xe_gt_sriov_pf_config_set_threshold(struct xe_gt *gt, unsigned int vfid,
>> +					enum xe_guc_klv_threshold_index index, u32 value);
>> +
>>  int xe_gt_sriov_pf_config_set_fair(struct xe_gt *gt, unsigned int vfid, unsigned int num_vfs);
>>  int xe_gt_sriov_pf_config_release(struct xe_gt *gt, unsigned int vfid, bool force);
>>  int xe_gt_sriov_pf_config_push(struct xe_gt *gt, unsigned int vfid, bool refresh);
>> 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 d3745c355957..7bc66656fcc7 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
>> @@ -8,6 +8,8 @@
>>  
>>  #include <drm/drm_mm.h>
>>  
>> +#include "xe_guc_klv_thresholds_set_types.h"
>> +
>>  struct xe_bo;
>>  
>>  /**
>> @@ -32,6 +34,8 @@ struct xe_gt_sriov_config {
>>  	u32 exec_quantum;
>>  	/** @preempt_timeout: preemption timeout in microseconds. */
>>  	u32 preempt_timeout;
>> +	/** @thresholds: GuC thresholds for adverse events notifications. */
>> +	u32 thresholds[XE_GUC_KLV_NUM_THRESHOLDS];
>>  };
> 
> With fixes:
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>
> 
>>  
>>  /**
>> -- 
>> 2.43.0
>>
>