[igt-dev] [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test
Iddamsetty, Aravind
aravind.iddamsetty at intel.com
Fri Nov 25 05:13:18 UTC 2022
> -----Original Message-----
> From: Gupta, Anshuman <anshuman.gupta at intel.com>
> Sent: Friday, November 25, 2022 10:24 AM
> To: Iddamsetty, Aravind <aravind.iddamsetty 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>; Nilawar, Badal
> <badal.nilawar at intel.com>
> Subject: RE: [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset IGT test
>
>
>
> > -----Original Message-----
> > From: Iddamsetty, Aravind <aravind.iddamsetty at intel.com>
> > Sent: Friday, November 25, 2022 9:57 AM
> > 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>; Nilawar, Badal <badal.nilawar at intel.com>
> > Subject: Re: [PATCH i-g-t v3 5/5] tests/device_reset: Add cold reset
> > IGT test
> >
> >
> >
> > On 23-11-2022 14:24, 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]
> > >
> > > 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 | 153
> > > ++++++++++++++++++++++++++++++++++++++++---
> > > 3 files changed, 184 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..4c624ce7d3 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,45 @@ static int open_driver_sysfs_dir(int fd)
> > > return __open_sysfs_dir(fd, "device/driver"); }
> > >
> > > +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];
> > > +
> > > + 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 +166,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 +186,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 +224,60 @@ 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;
> > > +}
> > > +
> >
> > why do not you combine the below two checks into
> > is_sysfs_cold_reset_supported as the check will be done only if power
> > sysfs is present.
> Thanks for review after this comment, I am pondering that it would be better
> to check the
> is_slot_power_ctrl_present() even before checking for slot number to save
> the CI time.
> As without power controller capability it is in vain to check for slot number.
> igt_skip_on_f(!is_slot_power_ctrl_present(dev.fds.dev), "Gfx Card root port
> does not support power ctrl"); 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_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
That's true.
Regards,
Aravind.
>
> BR,
> Anshuman Gupta.
> >
> > Thanks,
> > Aravind.
> > > +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); }
> > > +
> > > /* Unbind the driver from the device */ static void
> > > driver_unbind(struct device_fds *dev) { @@ -231,8 +330,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 +415,46 @@ 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_skip_on(!is_slot_power_ctrl_present(dev.fds.dev));
> > > + }
> > > +
> > > + 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