[igt-dev] [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test
Gupta, Anshuman
anshuman.gupta at intel.com
Wed Dec 7 08:38:17 UTC 2022
> -----Original Message-----
> From: Nilawar, Badal <badal.nilawar at intel.com>
> Sent: Wednesday, November 30, 2022 6:55 PM
> To: Gupta, Anshuman <anshuman.gupta at intel.com>; igt-
> dev at lists.freedesktop.org
> Cc: Vivi, Rodrigo <rodrigo.vivi at intel.com>; kamil.konieczny at linux.intel.com;
> Tangudu, Tilak <tilak.tangudu at intel.com>; Iddamsetty, Aravind
> <aravind.iddamsetty at intel.com>; Dixit, Ashutosh
> <ashutosh.dixit at intel.com>
> Subject: Re: [PATCH i-g-t v4 5/5] tests/device_reset: Add cold reset IGT test
>
> Hi Anshuman,
>
> On 28-11-2022 15:50, Anshuman Gupta wrote:
> > Add cold-reset IGT subtest, IGT subtest will use
> > /sys/bus/pci/slots/$SUN/power sysfs in order to initiate a cold reset
> > sequence. $SUN value will be retrieved from PCIe device ACPI firmware
> > node.
> >
> > v2:
> > - %s/igt_require(fd > 0)/igt_assert(errno >0)
> > v3:
> > - Added Slot Power Controller capability check. [Aravind]
> > - Check whether slot is powered down before powering up. [Aravind]
> > v4:
> > - Check power ctrl presence before slot presence to save CI
> > execution time.
> >
> > Signed-off-by: Anshuman Gupta <anshuman.gupta at intel.com>
> > Reviewed-by: Badal Nilawar <badal.nilawar at intel.com>
> > ---
> > lib/igt_pm.c | 40 +++++++++++
> > lib/igt_pm.h | 1 +
> > tests/device_reset.c | 156
> ++++++++++++++++++++++++++++++++++++++++---
> > 3 files changed, 187 insertions(+), 10 deletions(-)
> >
> > diff --git a/lib/igt_pm.c b/lib/igt_pm.c index 4f0cfbdd10..9b7ed4be64
> > 100644
> > --- a/lib/igt_pm.c
> > +++ b/lib/igt_pm.c
> > @@ -877,6 +877,46 @@ static int igt_pm_open_pci_firmware_node(struct
> pci_device *pci_dev)
> > return dir;
> > }
> >
> > +/**
> > + * igt_pm_get_pcie_acpihp_slot:
> > + * @pci_dev: pci bridge device.
> > + * Get pci bridge acpi hotplug slot number, if bridge's ACPI
> > +firmware_node
> > + * handle supports _SUN method.
> > + *
> > + * Returns:
> > + * PCIe bridge Slot number.
> > + * Returns -ENOENT number in case firmware_node/sun is not supported
> > +by the
> > + * bridge.
> > + */
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev) {
> > + int firmware_node_fd, fd, n_read;
> > + char sun[8];
> > +
> > + firmware_node_fd = igt_pm_open_pci_firmware_node(pci_dev);
> > +
> > + if (firmware_node_fd < 0 && errno == ENOENT)
> > + return -ENOENT;
> > +
> > + igt_require(firmware_node_fd > 0);
> > +
> > + fd = openat(firmware_node_fd, "sun", O_RDONLY);
> > + if (fd < 0 && errno == ENOENT) {
> > + close(firmware_node_fd);
> > + return -ENOENT;
> > + }
> > +
> > + igt_assert(errno > 0);
> > +
> > + n_read = read(fd, sun, sizeof(sun));
> > + igt_assert(n_read > 0);
> > +
> > + close(firmware_node_fd);
> > + close(fd);
> > +
> > + return strtol(sun, NULL, 10);
> > +}
> > +
> > /**
> > * igt_pm_acpi_d3cold_supported:
> > * @pci_dev: root port pci_dev.
> > diff --git a/lib/igt_pm.h b/lib/igt_pm.h index e81fceb897..f65b960c38
> > 100644
> > --- a/lib/igt_pm.h
> > +++ b/lib/igt_pm.h
> > @@ -73,6 +73,7 @@ bool igt_wait_for_pm_status(enum
> igt_runtime_pm_status status);
> > bool igt_pm_dmc_loaded(int debugfs);
> > bool igt_pm_pc8_plus_residencies_enabled(int msr_fd);
> > bool i915_output_is_lpsp_capable(int drm_fd, igt_output_t *output);
> > +int igt_pm_get_pcie_acpihp_slot(struct pci_device *pci_dev);
> > bool igt_pm_acpi_d3cold_supported(struct pci_device *pci_dev);
> > enum igt_acpi_d_state
> > igt_pm_get_acpi_real_d_state(struct pci_device *pci_dev); diff --git
> > a/tests/device_reset.c b/tests/device_reset.c index
> > 0c477a02c0..b65395fbb9 100644
> > --- a/tests/device_reset.c
> > +++ b/tests/device_reset.c
> > @@ -9,7 +9,9 @@
> >
> > #include "i915/gem.h"
> > #include "igt.h"
> > +#include "igt_device.h"
> > #include "igt_device_scan.h"
> > +#include "igt_pci.h"
> > #include "igt_sysfs.h"
> > #include "igt_kmod.h"
> >
> > @@ -33,6 +35,7 @@ struct device_fds {
> > int dev;
> > int dev_dir;
> > int drv_dir;
> > + int slot_dir; /* pci hotplug slots fd */
> > } fds;
> > char dev_bus_addr[DEV_BUS_ADDR_LEN];
> > bool snd_unload;
> > @@ -62,6 +65,74 @@ static int open_driver_sysfs_dir(int fd)
> > return __open_sysfs_dir(fd, "device/driver");
> > }
> >
> > +static bool is_pci_power_ctrl_present(struct pci_device *dev) {
> > + int offset;
> > + uint32_t slot_cap;
> > +
> > + offset = find_pci_cap_offset(dev, PCI_EXPRESS_CAP_ID);
> > + igt_require_f(offset > 0, "PCI Express Capability not found\n");
> > + igt_assert(!pci_device_cfg_read_u32(dev, &slot_cap, offset +
> PCI_SLOT_CAP_OFFSET));
> > + igt_debug("slot cap register 0x%x\n", slot_cap);
> > +
> > + return slot_cap & PCI_SLOT_PWR_CTRL_PRESENT; }
> > +
> > +static bool is_slot_power_ctrl_present(int fd) {
> > + struct pci_device *root;
> > +
> > + /*
> > + * Card root port Slot Capabilities Register
> > + * determines Power Controller Presence.
> > + */
> > + root = igt_device_get_pci_root_port(fd);
> > + return is_pci_power_ctrl_present(root);
> Why power controller capability of root port is checked here?
> Shouldn't we check the power controller capability of downstream
> switch/bridge on which slot is found?
This is to switch on/off the power of card and as per PCI specs the slot capability register
Are only applicable to downstream port or root port. Not for upstream switch port.
Therefore we only requires to check for root port because we are operating on card here.
BR,
Anshuman Gupta.
>
> Regards,
> Badal
> > +}
> > +
> > +static int open_slot_sysfs_dir(int fd) {
> > + struct pci_device *pci_dev = NULL;
> > + int slot_fd = -1, slot;
> > + char slot_fd_path[PATH_MAX];
> > +
> > + /* Don't search for slot if root port doesn't support power ctrl */
> > + if (!is_slot_power_ctrl_present(fd))
> > + return slot_fd;
> > +
> > + pci_dev = igt_device_get_pci_device(fd);
> > + igt_require(pci_dev);
> > +
> > + while ((pci_dev = pci_device_get_parent_bridge(pci_dev))) {
> > + slot = igt_pm_get_pcie_acpihp_slot(pci_dev);
> > + if (slot == -ENOENT) {
> > + igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> does not support acpihp slot\n",
> > + pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func);
> > + continue;
> > + }
> > +
> > + /*
> > + * Upon getting the valid acpihp slot number break the loop.
> > + * It is the desired acpihp slot for gfx card.
> > + */
> > + if (slot > 0) {
> > + igt_debug("Bridge PCI device %04x:%02x:%02x.%01x
> associated acpihp slot %d\n",
> > + pci_dev->domain, pci_dev->bus, pci_dev-
> >dev, pci_dev->func, slot);
> > + break;
> > + }
> > + }
> > +
> > + if (!pci_dev)
> > + return slot_fd;
> > +
> > + snprintf(slot_fd_path, PATH_MAX, "/sys/bus/pci/slots/%d", slot);
> > + slot_fd = open(slot_fd_path, O_RDONLY);
> > + if (slot_fd < 0)
> > + return -errno;
> > +
> > + return slot_fd;
> > +}
> > +
> > /**
> > * device_sysfs_path:
> > * @fd: opened device file descriptor @@ -124,6 +195,8 @@ static
> > void init_device_fds(struct device_fds *dev)
> >
> > dev->fds.drv_dir = open_driver_sysfs_dir(dev->fds.dev);
> > igt_assert_fd(dev->fds.drv_dir);
> > +
> > + dev->fds.slot_dir = open_slot_sysfs_dir(dev->fds.dev);
> > }
> >
> > static int close_if_opened(int *fd)
> > @@ -142,6 +215,7 @@ static void cleanup_device_fds(struct device_fds
> *dev)
> > igt_ignore_warn(close_if_opened(&dev->fds.dev));
> > igt_ignore_warn(close_if_opened(&dev->fds.dev_dir));
> > igt_ignore_warn(close_if_opened(&dev->fds.drv_dir));
> > + igt_ignore_warn(close_if_opened(&dev->fds.slot_dir));
> > }
> >
> > /**
> > @@ -179,6 +253,35 @@ static bool is_sysfs_reset_supported(int fd)
> > return true;
> > }
> >
> > +/**
> > + * is_sysfs_cold_reset_supported:
> > + * @fd: opened device file descriptor
> > + *
> > + * Check if device supports cold reset based on sysfs file presence.
> > + *
> > + * Returns:
> > + * True if device supports reset, false otherwise.
> > + */
> > +static bool is_sysfs_cold_reset_supported(int slot_fd) {
> > + struct stat st;
> > + int rc;
> > + int cold_reset_fd = -1;
> > +
> > + cold_reset_fd = openat(slot_fd, "power", O_WRONLY);
> > +
> > + if (cold_reset_fd < 0)
> > + return false;
> > +
> > + rc = fstat(cold_reset_fd, &st);
> > + close(cold_reset_fd);
> > +
> > + if (rc || !S_ISREG(st.st_mode))
> > + return false;
> > +
> > + return true;
> > +}
> > +
> > /* Unbind the driver from the device */
> > static void driver_unbind(struct device_fds *dev)
> > {
> > @@ -231,8 +334,13 @@ static void initiate_device_reset(struct device_fds
> *dev, enum reset type)
> > {
> > igt_debug("reset device\n");
> >
> > - if (type == FLR_RESET)
> > + if (type == FLR_RESET) {
> > igt_assert(igt_sysfs_set(dev->fds.dev_dir, "reset", "1"));
> > + } else if (type == COLD_RESET) {
> > + igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "0"));
> > + igt_assert(!igt_sysfs_get_boolean(dev->fds.slot_dir,
> "power"));
> > + igt_assert(igt_sysfs_set(dev->fds.slot_dir, "power", "1"));
> > + }
> >
> > }
> >
> > @@ -311,17 +419,45 @@ igt_main
> > igt_skip_on(!is_sysfs_reset_supported(dev.fds.dev));
> > }
> >
> > - igt_describe("Unbinds driver from device, initiates reset"
> > - " then rebinds driver to device");
> > - igt_subtest("unbind-reset-rebind") {
> > - unbind_reset_rebind(&dev, FLR_RESET);
> > - healthcheck(&dev);
> > + igt_subtest_group {
> > + igt_describe("Unbinds driver from device, initiates reset"
> > + " then rebinds driver to device");
> > + igt_subtest("unbind-reset-rebind") {
> > + unbind_reset_rebind(&dev, FLR_RESET);
> > + healthcheck(&dev);
> > + }
> > +
> > + igt_describe("Resets device with bound driver");
> > + igt_subtest("reset-bound") {
> > + initiate_device_reset(&dev, FLR_RESET);
> > + /*
> > + * Cold reset will initiate card boot sequence again,
> > + * therefore let healthcheck() re-epen the dev fd.
> > + */
> > + dev.fds.dev = -1;
> > + healthcheck(&dev);
> > + }
> > }
> >
> > - igt_describe("Resets device with bound driver");
> > - igt_subtest("reset-bound") {
> > - initiate_device_reset(&dev, FLR_RESET);
> > - healthcheck(&dev);
> > + igt_subtest_group {
> > + igt_fixture {
> > + igt_skip_on_f(dev.fds.slot_dir < 0, "Gfx Card does
> not support any "
> > + "pcie slot for cold reset\n");
> > +
> igt_skip_on(!is_sysfs_cold_reset_supported(dev.fds.slot_dir));
> > + }
> > +
> > + igt_describe("Unbinds driver from device, initiates cold
> reset"
> > + " then rebinds driver to device");
> > + igt_subtest("unbind-cold-reset-rebind") {
> > + unbind_reset_rebind(&dev, COLD_RESET);
> > + healthcheck(&dev);
> > + }
> > +
> > + igt_describe("Cold Resets device with bound driver");
> > + igt_subtest("cold-reset-bound") {
> > + initiate_device_reset(&dev, COLD_RESET);
> > + healthcheck(&dev);
> > + }
> > }
> >
> > igt_fixture {
More information about the igt-dev
mailing list