Re: [PATCH 2/3] drm/xe: Add proper detection of the SR-IOV PF mode

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

On 09.04.2024 19:47, Rodrigo Vivi wrote:
> On Thu, Apr 04, 2024 at 05:44:30PM +0200, Michal Wajdeczko wrote:
>> SR-IOV PF mode detection is based on PCI capability as reported by
>> the PCI dev_is_pf() function and additionally on 'max_vfs' module
>> parameter which could be also used to disable PF capability even
>> if SR-IOV PF capability is reported by the hardware.
>>
>> Signed-off-by: Michal Wajdeczko <michal.wajdeczko@intel.com>
>> ---
>>  drivers/gpu/drm/xe/Makefile          |  3 +-
>>  drivers/gpu/drm/xe/xe_device_types.h |  4 ++
>>  drivers/gpu/drm/xe/xe_sriov.c        |  3 +
>>  drivers/gpu/drm/xe/xe_sriov_pf.c     | 89 ++++++++++++++++++++++++++++
>>  drivers/gpu/drm/xe/xe_sriov_pf.h     | 24 ++++++++
>>  drivers/gpu/drm/xe/xe_sriov_types.h  | 15 +++++
>>  6 files changed, 137 insertions(+), 1 deletion(-)
>>  create mode 100644 drivers/gpu/drm/xe/xe_sriov_pf.c
>>  create mode 100644 drivers/gpu/drm/xe/xe_sriov_pf.h
>>
>> diff --git a/drivers/gpu/drm/xe/Makefile b/drivers/gpu/drm/xe/Makefile
>> index e5b1715f721e..8d79df05b84f 100644
>> --- a/drivers/gpu/drm/xe/Makefile
>> +++ b/drivers/gpu/drm/xe/Makefile
>> @@ -159,7 +159,8 @@ xe-$(CONFIG_PCI_IOV) += \
>>  	xe_gt_sriov_pf_control.o \
>>  	xe_lmtt.o \
>>  	xe_lmtt_2l.o \
>> -	xe_lmtt_ml.o
>> +	xe_lmtt_ml.o \
>> +	xe_sriov_pf.o
>>  
>>  # include helpers for tests even when XE is built-in
>>  ifdef CONFIG_DRM_XE_KUNIT_TEST
>> diff --git a/drivers/gpu/drm/xe/xe_device_types.h b/drivers/gpu/drm/xe/xe_device_types.h
>> index c710cec835a7..8244b177a6a3 100644
>> --- a/drivers/gpu/drm/xe/xe_device_types.h
>> +++ b/drivers/gpu/drm/xe/xe_device_types.h
>> @@ -321,6 +321,10 @@ struct xe_device {
>>  	struct {
>>  		/** @sriov.__mode: SR-IOV mode (Don't access directly!) */
>>  		enum xe_sriov_mode __mode;
>> +
>> +		/** @sriov.pf: PF specific data */
>> +		struct xe_device_pf pf;
>> +
>>  		/** @sriov.wq: workqueue used by the virtualization workers */
>>  		struct workqueue_struct *wq;
>>  	} sriov;
>> diff --git a/drivers/gpu/drm/xe/xe_sriov.c b/drivers/gpu/drm/xe/xe_sriov.c
>> index 3e103edf7174..94fa98d8206e 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov.c
>> +++ b/drivers/gpu/drm/xe/xe_sriov.c
>> @@ -11,6 +11,7 @@
>>  #include "xe_device.h"
>>  #include "xe_mmio.h"
>>  #include "xe_sriov.h"
>> +#include "xe_sriov_pf.h"
>>  
>>  /**
>>   * xe_sriov_mode_to_string - Convert enum value to string.
>> @@ -58,6 +59,8 @@ void xe_sriov_probe_early(struct xe_device *xe)
>>  	if (has_sriov) {
>>  		if (test_is_vf(xe))
>>  			mode = XE_SRIOV_MODE_VF;
>> +		else if (xe_sriov_pf_readiness(xe))
>> +			mode = XE_SRIOV_MODE_PF;
>>  	}
>>  
>>  	xe_assert(xe, !xe->sriov.__mode);
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_pf.c b/drivers/gpu/drm/xe/xe_sriov_pf.c
>> new file mode 100644
>> index 000000000000..030c2b69ecc4
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_sriov_pf.c
>> @@ -0,0 +1,89 @@
>> +// SPDX-License-Identifier: MIT
>> +/*
>> + * Copyright © 2023-2024 Intel Corporation
>> + */
>> +
>> +#include "xe_assert.h"
>> +#include "xe_device.h"
>> +#include "xe_module.h"
>> +#include "xe_sriov.h"
>> +#include "xe_sriov_pf.h"
>> +#include "xe_sriov_printk.h"
>> +
>> +static unsigned int wanted_max_vfs(struct xe_device *xe)
>> +{
>> +	return xe_modparam.max_vfs;
>> +}
>> +
>> +static int pf_reduce_totalvfs(struct xe_device *xe, int limit)
>> +{
>> +	struct device *dev = xe->drm.dev;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	int err;
>> +
>> +	err = pci_sriov_set_totalvfs(pdev, limit);
>> +	if (err)
>> +		xe_sriov_notice(xe, "Failed to set number of VFs to %d (%pe)\n",
>> +				limit, ERR_PTR(err));
>> +	return err;
>> +}
>> +
>> +static bool pf_continue_as_native(struct xe_device *xe, const char *why)
>> +{
>> +	xe_sriov_dbg(xe, "%s, continuing as native\n", why);
>> +	pf_reduce_totalvfs(xe, 0);
>> +	return false;
>> +}
>> +
>> +/**
>> + * xe_sriov_pf_readiness - Check if PF functionality can be enabled.
>> + * @xe: the &xe_device to check
>> + *
>> + * This function is called as part of the SR-IOV probe to validate if all
>> + * PF prerequisites are satisfied and we can continue with enabling PF mode.
>> + *
>> + * Return: true if the PF mode can be turned on.
>> + */
>> +bool xe_sriov_pf_readiness(struct xe_device *xe)
>> +{
>> +	struct device *dev = xe->drm.dev;
>> +	struct pci_dev *pdev = to_pci_dev(dev);
>> +	int totalvfs = pci_sriov_get_totalvfs(pdev);
>> +	int newlimit = min_t(u16, wanted_max_vfs(xe), totalvfs);
>> +
>> +	xe_assert(xe, totalvfs <= U16_MAX);
> 
> perhaps we should do this check before the min_t?

this assert is just to express/confirm SLA with the PCI function

> 
> should we also check what we are getting from the params?

we don't care, param could be any number up to U32_MAX, as we will just
use it to limit what PCI function (and thus our HW) reports

> 
>> +
>> +	if (!dev_is_pf(dev))
>> +		return false;
>> +
>> +	if (!xe_device_uc_enabled(xe))
>> +		return pf_continue_as_native(xe, "Guc submission disabled");
>> +
>> +	if (!newlimit)
>> +		return pf_continue_as_native(xe, "all VFs disabled");
>> +
>> +	pf_reduce_totalvfs(xe, newlimit);
> 
> do we really need to always call this or only when newlimit != totalvfs?

it's harmless to call it even if there is no change as this just updates
internal data in PCI core, does not touch any HW, so one "if" spared

> 
>> +
>> +	xe->sriov.pf.device_total_vfs = totalvfs;
>> +	xe->sriov.pf.driver_max_vfs = newlimit;
>> +
>> +	return true;
>> +}
>> +
>> +/**
>> + * xe_sriov_pf_print_vfs_summary - Print SR-IOV PF information.
>> + * @xe: the &xe_device to print info from
>> + * @p: the &drm_printer
>> + *
>> + * Print SR-IOV PF related information into provided DRM printer.
>> + */
>> +void xe_sriov_pf_print_vfs_summary(struct xe_device *xe, struct drm_printer *p)
>> +{
>> +	struct pci_dev *pdev = to_pci_dev(xe->drm.dev);
>> +
>> +	xe_assert(xe, IS_SRIOV_PF(xe));
>> +
>> +	drm_printf(p, "total: %u\n", xe->sriov.pf.device_total_vfs);
>> +	drm_printf(p, "supported: %u\n", xe->sriov.pf.driver_max_vfs);
>> +	drm_printf(p, "enabled: %u\n", pci_num_vf(pdev));
>> +}
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_pf.h b/drivers/gpu/drm/xe/xe_sriov_pf.h
>> new file mode 100644
>> index 000000000000..ebef2e01838a
>> --- /dev/null
>> +++ b/drivers/gpu/drm/xe/xe_sriov_pf.h
>> @@ -0,0 +1,24 @@
>> +/* SPDX-License-Identifier: MIT */
>> +/*
>> + * Copyright © 2023-2024 Intel Corporation
>> + */
>> +
>> +#ifndef _XE_SRIOV_PF_H_
>> +#define _XE_SRIOV_PF_H_
>> +
>> +#include <linux/types.h>
>> +
>> +struct drm_printer;
>> +struct xe_device;
>> +
>> +#ifdef CONFIG_PCI_IOV
>> +bool xe_sriov_pf_readiness(struct xe_device *xe);
>> +void xe_sriov_pf_print_vfs_summary(struct xe_device *xe, struct drm_printer *p);
>> +#else
>> +static inline bool xe_sriov_pf_readiness(struct xe_device *xe)
>> +{
>> +	return false;
>> +}
> 
> missing the vfs_summary function in this block?!

only xe_sriov_pf_readiness() function will be used regardless of the
CONFIG_PCI_IOV, the other xe_sriov_pf_print_vfs_summary() function and
few more functions (coming soon) will be used only for CONFIG_PCI_IOV=y
and thus will not require empty stubs

> 
>> +#endif
>> +
>> +#endif
>> diff --git a/drivers/gpu/drm/xe/xe_sriov_types.h b/drivers/gpu/drm/xe/xe_sriov_types.h
>> index 1a138108d139..fa583e8fa0c2 100644
>> --- a/drivers/gpu/drm/xe/xe_sriov_types.h
>> +++ b/drivers/gpu/drm/xe/xe_sriov_types.h
>> @@ -7,6 +7,7 @@
>>  #define _XE_SRIOV_TYPES_H_
>>  
>>  #include <linux/build_bug.h>
>> +#include <linux/types.h>
>>  
>>  /**
>>   * VFID - Virtual Function Identifier
>> @@ -37,4 +38,18 @@ enum xe_sriov_mode {
>>  };
>>  static_assert(XE_SRIOV_MODE_NONE);
>>  
>> +/**
>> + * struct xe_device_pf - Xe PF related data
>> + *
>> + * The data in this structure is valid only if driver is running in the
>> + * @XE_SRIOV_MODE_PF mode.
>> + */
>> +struct xe_device_pf {
>> +	/** @device_total_vfs: Maximum number of VFs supported by the device. */
>> +	u16 device_total_vfs;
>> +
>> +	/** @driver_max_vfs: Maximum number of VFs supported by the driver. */
>> +	u16 driver_max_vfs;
>> +};
>> +
>>  #endif
>> -- 
>> 2.43.0
>>