Re: [PATCH 6/6] drm/xe/pf: Add support to configure SR-IOV VFs

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

On 15.04.2024 16:29, Piotr Piórkowski wrote:
> Michal Wajdeczko <michal.wajdeczko@intel.com> wrote on nie [2024-kwi-14 21:01:37 +0200]:

...

>> +static int pf_push_full_vf_config(struct xe_gt *gt, unsigned int vfid)
>> +{
>> +	struct xe_gt_sriov_config *config = pf_pick_vf_config(gt, vfid);
>> +	u32 max_cfg_dwords = SZ_4K / sizeof(u32);
>> +	u32 num_dwords;
>> +	int num_klvs;
>> +	u32 *cfg;
>> +	int err;
>> +
>> +	cfg = kcalloc(max_cfg_dwords, sizeof(u32), GFP_KERNEL);
>> +	if (!cfg)
>> +		return -ENOMEM;
>> +
>> +	num_dwords = encode_config(cfg, config);
>> +	xe_gt_assert(gt, num_dwords <= max_cfg_dwords);
>> +
> 
> NIT: I would add some commentary as to why in the case of media GT we do GGTT provisioning
> with values from primary GT.
> At first glance, it looks like we are adding KLV twice for GGTT - at least I caught myself
> doing that.

sure, will add small comment there (maybe also together with an assert
to enforce that there shall be no GGTT config on the media-GT that could
have been emitted by the encode_config()

...

>> +int xe_gt_sriov_pf_config_bulk_set_ggtt(struct xe_gt *gt, unsigned int vfid,
>> +					unsigned int num_vfs, u64 size)
>> +{
>> +	unsigned int n;
>> +	int err = 0;
>> +
>> +	xe_gt_assert(gt, vfid);
>> +	xe_gt_assert(gt, !xe_gt_is_media_type(gt));
>> +
> 
> NIT: Maybe you should add an assert here to check if vfid + num_vfs <= total_vfs

no really needed as there is an existing assert in pf_pick_vf_config()
helper which is called by most of the functions including the
pf_provision_vf_ggtt() from the loop below

> 
>> +	if (!num_vfs)
>> +		return 0;
>> +
>> +	mutex_lock(xe_gt_sriov_pf_master_mutex(gt));
>> +	for (n = vfid; n < vfid + num_vfs; n++) {
>> +		err = pf_provision_vf_ggtt(gt, n, size);
>> +		if (err)
>> +			break;
>> +	}
>> +	mutex_unlock(xe_gt_sriov_pf_master_mutex(gt));
>> +
>> +	return pf_config_bulk_set_u64_done(gt, vfid, num_vfs, size,
>> +					   xe_gt_sriov_pf_config_get_ggtt,
>> +					   "GGTT", n, err);
>> +}

...

>> +static u64 pf_estimate_fair_ggtt(struct xe_gt *gt, unsigned int num_vfs)
>> +{
>> +	u64 available = pf_get_max_ggtt(gt);
>> +	u64 alignment = pf_get_ggtt_alignment(gt);
>> +	u64 fair;
>> +
>> +	fair = div_u64(available, num_vfs);
>> +	fair = ALIGN_DOWN(fair, alignment);
>> +	xe_gt_sriov_dbg_verbose(gt, "GGTT available(%lluK) fair(%u x %lluK)\n",
>> +				available / SZ_1K, num_vfs, fair / SZ_1K);
>> +	return fair;
>> +}
> NIT: This is not the most optimal solution in any case.
> Perhaps it is worth noting here in the comment that it is a conscious decision

well, it's still the best solution for 1 VF

note that 'fair' allocations used by auto-provisioning are best-effort,
as we aim to expose some kind of uabi that will allow to setup specific
predefined configuration that could provide some SLA

anyway, added some comment in v2

> 
>> +
>> +/**
>> + * xe_gt_sriov_pf_config_set_fair_ggtt - Provision many VFs with fair GGTT.
>> + * @gt: the &xe_gt (can't be media)
>> + * @vfid: starting VF identifier (can't be 0)
>> + * @num_vfs: number of VFs to provision
>> + *
>> + * 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_fair_ggtt(struct xe_gt *gt,
>> +					unsigned int vfid, unsigned int num_vfs)
> 
> NIT: In my opinion strangely divided into a new line these parameters
> 

fixed in v2

...

>> +static const char *exec_quantum_unit(u32 exec_quantum)
>> +{
>> +	return exec_quantum ? "ms" : "(inifinity)";
> 
> typo

oops

...

> 
> Several NITs, 2 typo (in the same word).
> But the functionality seems to be ok.
> After the necessary fixes (I mean mainly typo, but also pls consider my comments):
> 
> Reviewed-by: Piotr Piórkowski <piotr.piorkowski@intel.com>

thanks!

Michal