[PATCH i-g-t] tools/intel_reg: Add --pci-slot option
Kamil Konieczny
kamil.konieczny at linux.intel.com
Tue Feb 18 17:03:47 UTC 2025
Hi Adam,
On 2025-02-12 at 08:54:07 +0100, Adam Miszczak wrote:
> Hi Kamil,
>
> On 31.01.2025 22:37, Kamil Konieczny wrote:
> > From: Łukasz Łaguna <lukasz.laguna at intel.com>
> >
> > Add device selection by PCI slot with new option
> >
> > --pci-slot <domain>:<bus>:<device>[.<func>]
> >
> > Example:
> >
> > intel_reg dump --pci-slot 0000:01:00.0
> > intel_reg dump --pci-slot 0000:8c:00
> >
> > This should help in multi-GPU scenario when someone uses two or
> > more discrete GPUs.
> >
> > v1: address review comments from Jani (Kamil)
> >
> > Cc: Ashutosh Dixit <ashutosh.dixit at intel.com>
> > Cc: Jani Nikula <jani.nikula at intel.com>
> > Cc: Zbigniew Kempczyński <zbigniew.kempczynski at intel.com>
> > Signed-off-by: Łukasz Łaguna <lukasz.laguna at intel.com>
> > Signed-off-by: Kamil Konieczny <kamil.konieczny at linux.intel.com>
> > ---
> > This is a re-work of Lukasz Laguna patch from
> > https://patchwork.freedesktop.org/series/131155/
> > ("tools/intel_reg: add possibility to select device")
> >
> > Example usage on two dGPU:
> > sudo ./intel_reg --pci-slot 0000:8c:00.0 read 0x00138108
> > (0x00138108): 0xf4e57b26
> > sudo ./intel_reg --pci-slot 0000:87:00.0 read 0x00138108
> > (0x00138108): 0xf4162825
> >
> > man/intel_reg.rst | 5 ++-
> > tools/intel_reg.c | 106 +++++++++++++++++++++++++++++++++++++++++++---
> > 2 files changed, 103 insertions(+), 8 deletions(-)
> >
> > diff --git a/man/intel_reg.rst b/man/intel_reg.rst
> > index 059d8834a..64e4fa57f 100644
> > --- a/man/intel_reg.rst
> > +++ b/man/intel_reg.rst
> > @@ -9,7 +9,7 @@ Intel graphics register multitool
> > :Author: Jani Nikula <jani.nikula at intel.com>
> > :Date: 2016-03-01
> > :Version: |PACKAGE_STRING|
> > -:Copyright: 2015-2016 Intel Corporation
> > +:Copyright: 2015-2025 Intel Corporation
> > :Manual section: |MANUAL_SECTION|
> > :Manual group: |MANUAL_GROUP|
> > @@ -56,6 +56,9 @@ Some options are global, and some specific to commands.
> > Pretend to be PCI ID DEVID. Useful with MMIO bar snapshots from other
> > machines.
> > +--pci-slot <domain>:<bus>:<device>[.<func>]
> > + Find Intel GPU by PCI slot. Useful with multi-GPU hardware.
> > +
> > --spec=PATH
> > Read register spec from directory or file specified by PATH; see REGISTER
> > SPEC DEFINITIONS below for details. This option implies --decode.
> > diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> > index bb1ab2889..45f8b3a94 100644
> > --- a/tools/intel_reg.c
> > +++ b/tools/intel_reg.c
> > @@ -39,6 +39,7 @@
> > #include "intel_chipset.h"
> > #include "intel_reg_spec.h"
> > +#include "igt_device_scan.h"
> > #ifdef HAVE_SYS_IO_H
> > @@ -89,6 +90,13 @@ struct config {
> > int verbosity;
> > };
> > +struct igt_pci_slot {
> > + int domain;
> > + int bus;
> > + int dev;
> > + int func;
> > +};
> > +
> > /* port desc must have been set */
> > static int set_reg_by_addr(struct config *config, struct reg *reg,
> > uint32_t addr)
> > @@ -1019,6 +1027,11 @@ static const struct command commands[] = {
> > .description = "list all known register names",
> > .decode = true,
> > },
> > + {
> > + .name = "help",
> > + .function = intel_reg_help,
> > + .description = "show this help",
> > + },
> > };
> > static int intel_reg_help(struct config *config, int argc, char *argv[])
> > @@ -1055,6 +1068,8 @@ static int intel_reg_help(struct config *config, int argc, char *argv[])
> > printf(" --devid=DEVID Specify PCI device ID for --mmio=FILE\n");
> > printf(" --decode Decode registers. Implied by commands that require it\n");
> > printf(" --all Decode registers for all known platforms. Implies --decode\n");
> > + printf(" --pci-slot Decode registers for platform described by PCI slot\n"
> > + " <domain>:<bus>:<device>[.<func>]\n");
>
> Nit: as the new '--pci-slot' option requires a parameter, the help message
> could clearly state it
> (as for --mmio/--devid options), e.g.:
> --pci-slot=BDF
I will add it, thx for spotting.
>
> > printf(" --binary Binary dump registers\n");
> > printf(" --verbose Increase verbosity\n");
> > printf(" --quiet Reduce verbosity\n");
> > @@ -1164,6 +1179,62 @@ builtin:
> > return config->regcount;
> > }
> > +static int parse_pci_slot_name(struct igt_pci_slot *st, const char *slot_name)
> > +{
> > + int i;
> > +
> > + st->domain = 0;
> > + st->bus = 0;
> > + st->dev = 0;
> > + st->func = 0;
> > + i = sscanf(slot_name, "%x:%x:%x.%x", &st->domain, &st->bus, &st->dev, &st->func);
> > +
> > + return i;
> > +}
> > +
> > +static bool is_graphics_card_valid(struct pci_device *pci_dev)
> > +{
> > + if (!pci_dev) {
> > + fprintf(stderr, "Graphics card not found\n");
> > + return false;
> > + }
> > + if (pci_device_probe(pci_dev) != 0) {
> > + fprintf(stderr, "Couldn't probe graphics card\n");
> > + return false;
> > + }
> > + if (pci_dev->vendor_id != 0x8086) {
> > + fprintf(stderr, "Graphics card is non-intel\n");
> > + return false;
> > + }
> > + return true;
> > +}
> > +
> > +static bool find_dev_from_slot(struct pci_device **pci_dev, char *opt_slot)
> > +{
> > + struct igt_pci_slot bdf;
> > + bool ret;
> > +
> > + if (parse_pci_slot_name(&bdf, opt_slot) < 3) {
> > + fprintf(stderr, "Cannot decode PCI slot from '%s'\n", opt_slot);
> > + return false;
> > + }
> > +
> > + if (pci_system_init() != 0) {
> > + fprintf(stderr, "Couldn't initialize PCI system\n");
> > + return false;
> > + }
> > +
> > + igt_devices_scan();
> > + *pci_dev = pci_device_find_by_slot(bdf.domain, bdf.bus, bdf.dev, bdf.func);
> > + ret = is_graphics_card_valid(*pci_dev);
> > + igt_devices_free();
> > +
> > + if (!ret)
> > + fprintf(stderr, "Cannot find PCI card given by slot '%s'\n", opt_slot);
> > +
> > + return ret;
> > +}
> > +
> > enum opt {
> > OPT_UNKNOWN = '?',
> > OPT_END = -1,
> > @@ -1173,6 +1244,7 @@ enum opt {
> > OPT_POST,
> > OPT_DECODE,
> > OPT_ALL,
> > + OPT_SLOT,
> > OPT_BINARY,
> > OPT_SPEC,
> > OPT_VERBOSE,
> > @@ -1182,8 +1254,9 @@ enum opt {
> > int main(int argc, char *argv[])
> > {
> > - int ret, i, index;
> > + int i, index;
> > char *endp;
> > + char *opt_slot = NULL;
> > enum opt opt;
> > const struct command *command = NULL;
> > struct config config = {
> > @@ -1191,6 +1264,7 @@ int main(int argc, char *argv[])
> > .fd = -1,
> > };
> > bool help = false;
> > + int ret = EXIT_FAILURE;
> > static struct option options[] = {
> > /* global options */
> > @@ -1208,6 +1282,7 @@ int main(int argc, char *argv[])
> > /* options specific to read, dump and decode */
> > { "decode", no_argument, NULL, OPT_DECODE },
> > { "all", no_argument, NULL, OPT_ALL },
> > + { "pci-slot", required_argument, NULL, OPT_SLOT },
> > { "binary", no_argument, NULL, OPT_BINARY },
> > { 0 }
> > };
> > @@ -1257,6 +1332,14 @@ int main(int argc, char *argv[])
> > case OPT_DECODE:
> > config.decode = true;
> > break;
> > + case OPT_SLOT:
> > + opt_slot = strdup(optarg);
> > + if (!opt_slot) {
> > + fprintf(stderr, "strdup: %s\n",
> > + strerror(errno));
> > + return EXIT_FAILURE;
> > + }
> > + break;
> > case OPT_BINARY:
> > config.binary = true;
> > break;
> > @@ -1298,7 +1381,14 @@ int main(int argc, char *argv[])
> > fprintf(stderr, "--devid without --mmio\n");
> > return EXIT_FAILURE;
> > }
> > - config.pci_dev = intel_get_pci_device();
> > +
> > + if (opt_slot) {
> > + if (!find_dev_from_slot(&config.pci_dev, opt_slot))
> > + return EXIT_FAILURE;
> > + } else {
> > + config.pci_dev = intel_get_pci_device();
> > + }
> > +
> > config.devid = config.pci_dev->device_id;
> > }
> > @@ -1311,21 +1401,23 @@ int main(int argc, char *argv[])
> > if (!command) {
> > fprintf(stderr, "'%s' is not an intel-reg command\n", argv[0]);
> > - return EXIT_FAILURE;
> > + goto exit;
> > }
> > if (command->decode)
> > config.decode = true;
> > - if (read_reg_spec(&config) < 0)
> > - return EXIT_FAILURE;
> > -
> > - ret = command->function(&config, argc, argv);
> > + if (read_reg_spec(&config) >= 0)
> > + ret = command->function(&config, argc, argv);
> > +exit:
> > free(config.mmiofile);
> > if (config.fd >= 0)
> > close(config.fd);
> > + if (opt_slot)
> > + free(opt_slot);
> > +
> > return ret;
> > }
>
> Verified the new option on a multi-GPU platform - reads registers for a
> device with a given PCI BDF as expected.
> Suggest a small update to the help message, but overall LGTM:
>
> Reviewed-by: Adam Miszczak <adam.miszczak at linux.intel.com>
>
Thank you for review,
Regards,
Kamil
More information about the igt-dev
mailing list