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

[Rodrigo Vivi <rodrigo.vivi@intel.com>]

On Tue, Apr 09, 2024 at 08:39:36PM +0200, Michal Wajdeczko wrote:
> 
> 
> 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

Thanks for all the explanations.

Reviewed-by: Rodrigo Vivi <rodrigo.vivi@intel.com>

> 
> > 
> >> +#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
> >>