[PATCH i-g-t] tools/intel_reg: add possibility to select device
Kamil Konieczny
kamil.konieczny at linux.intel.com
Thu Aug 22 12:24:04 UTC 2024
Hi Jani,
On 2024-03-15 at 11:13:37 +0200, Jani Nikula wrote:
> On Thu, 14 Mar 2024, Ashutosh Dixit <ashutosh.dixit at intel.com> wrote:
> > From: Łukasz Łaguna <lukasz.laguna at intel.com>
> >
> > Device can be selected by slot or using filter. Example:
> >
> > intel_reg dump --slot "0000:01:00.0"
> > intel_reg dump --filter "pci:vendor=8086"
>
> Really, if you want to add device filtering, the sensible thing to do is
> to mimic lspci -s or -d options instead of NIH'ing your own. Maybe add a
> subset of the functionality first, but please don't deviate from the
> format.
>
> Other comments inline.
>
It looks like NIH but there are devices which are not on pci bus,
see recent patch here:
https://patchwork.freedesktop.org/series/137520/
tests/device_reset: Require a PCI device
Submitted by Rob Clark on Aug. 20, 2024, 2:43 p.m.
Could we proceed with renamed options like --igt-filter and
--igt-slot ?
I chose that so it will not clash with '-s' which could be mistaken
as a shorter form of --slot but those options could have
another name, feel free to propose one.
And it will be possible to add libpci compatibile options.
Regards,
Kamil
> > Signed-off-by: Łukasz Łaguna <lukasz.laguna at intel.com>
> > ---
> > tools/intel_reg.c | 128 +++++++++++++++++++++++++++++++++++++++++++++-
> > 1 file changed, 126 insertions(+), 2 deletions(-)
> >
> > diff --git a/tools/intel_reg.c b/tools/intel_reg.c
> > index 6c37e14d12..6b903f3f87 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
> > @@ -86,6 +87,13 @@ struct config {
> > int verbosity;
> > };
> >
> > +struct pci_slot {
>
> The name has clash potential with pci libs.
>
> > + 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)
> > @@ -958,6 +966,12 @@ static int intel_reg_list(struct config *config, int argc, char *argv[])
> > return EXIT_SUCCESS;
> > }
> >
> > +static int intel_reg_list_filters(struct config *config, int argc, char *argv[])
> > +{
> > + igt_device_print_filter_types();
> > + return EXIT_SUCCESS;
> > +}
> > +
> > static int intel_reg_help(struct config *config, int argc, char *argv[]);
> >
> > struct command {
> > @@ -1001,6 +1015,11 @@ static const struct command commands[] = {
> > .function = intel_reg_list,
> > .description = "list all known register names",
> > },
> > + {
> > + .name = "list_filters",
>
> Please do not use underscore in subcommands. list-filters.
>
> When you update the command-line interface, a man page update is
> required as well: man/intel_reg.rst.
>
> > + .function = intel_reg_list_filters,
> > + .description = "list registered device filters types",
> > + },
> > {
> > .name = "help",
> > .function = intel_reg_help,
> > @@ -1041,6 +1060,8 @@ static int intel_reg_help(struct config *config, int argc, char *argv[])
> > printf(" --mmio=FILE Use an MMIO snapshot\n");
> > printf(" --devid=DEVID Specify PCI device ID for --mmio=FILE\n");
> > printf(" --all Decode registers for all known platforms\n");
> > + printf(" --filter Decode registers for platform described by filter\n");
> > + printf(" --slot Decode registers for platform described by slot\n");
> > printf(" --binary Binary dump registers\n");
> > printf(" --verbose Increase verbosity\n");
> > printf(" --quiet Reduce verbosity\n");
> > @@ -1147,6 +1168,52 @@ builtin:
> > return config->regcount;
> > }
> >
> > +static void parse_pci_slot_name(struct pci_slot *pci_slot, const char *pci_slot_name)
> > +{
> > + igt_assert_eq(sscanf(pci_slot_name, "%x:%x:%x.%x",
> > + &pci_slot->domain, &pci_slot->bus, &pci_slot->dev, &pci_slot->func), 4);
>
> This is a tool, not a test. Please do not use igt_assert or anything
> like that. Handle the errors and fprintf a message to stderr.
>
> > +}
> > +
> > +static struct pci_device *find_intel_graphics_card(void)
> > +{
> > + struct pci_device *pci_dev;
> > + struct pci_device_iterator *iter;
> > + struct pci_id_match match;
> > +
> > + match.vendor_id = 0x8086; /* Intel */
> > + match.device_id = PCI_MATCH_ANY;
> > + match.subvendor_id = PCI_MATCH_ANY;
> > + match.subdevice_id = PCI_MATCH_ANY;
> > +
> > + match.device_class = 0x3 << 16;
> > + match.device_class_mask = 0xff << 16;
> > +
> > + match.match_data = 0;
> > +
> > + iter = pci_id_match_iterator_create(&match);
> > + pci_dev = pci_device_next(iter);
> > + pci_iterator_destroy(iter);
> > +
> > + return pci_dev;
> > +}
> > +
> > +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;
> > +}
> > +
> > enum opt {
> > OPT_UNKNOWN = '?',
> > OPT_END = -1,
> > @@ -1155,6 +1222,8 @@ enum opt {
> > OPT_COUNT,
> > OPT_POST,
> > OPT_ALL,
> > + OPT_FILTER,
> > + OPT_SLOT,
> > OPT_BINARY,
> > OPT_SPEC,
> > OPT_VERBOSE,
> > @@ -1164,8 +1233,12 @@ enum opt {
> >
> > int main(int argc, char *argv[])
> > {
> > - int ret, i, index;
> > + int ret, i, index, len;
> > char *endp;
> > + char *opt_filter = NULL;
> > + struct pci_slot bdf;
> > + struct pci_device *pci_dev = NULL;
> > + struct igt_device_card card;
> > enum opt opt;
> > const struct command *command = NULL;
> > struct config config = {
> > @@ -1189,6 +1262,8 @@ int main(int argc, char *argv[])
> > { "post", no_argument, NULL, OPT_POST },
> > /* options specific to read, dump and decode */
> > { "all", no_argument, NULL, OPT_ALL },
> > + { "filter", required_argument, NULL, OPT_FILTER },
> > + { "slot", required_argument, NULL, OPT_SLOT },
> > { "binary", no_argument, NULL, OPT_BINARY },
> > { 0 }
> > };
> > @@ -1233,6 +1308,24 @@ int main(int argc, char *argv[])
> > case OPT_ALL:
> > config.all_platforms = true;
> > break;
> > + case OPT_FILTER:
> > + opt_filter = strdup(optarg);
> > + if (!opt_filter) {
> > + fprintf(stderr, "strdup: %s\n",
> > + strerror(errno));
> > + return EXIT_FAILURE;
> > + }
> > + break;
> > + case OPT_SLOT:
> > + len = strlen("pci:slot=") + strlen(optarg) + 1;
> > + opt_filter = malloc(len);
> > + snprintf(opt_filter, len, "%s%s", "pci:slot=", optarg);
> > + if (!opt_filter) {
>
> snprintf() would've crashed by now if !opt_filter.
>
> But maybe you're looking for asprintf() instead.
>
> > + fprintf(stderr, "opt_filter: %s\n",
> > + strerror(errno));
> > + return EXIT_FAILURE;
> > + }
> > + break;
>
> Adding both --filter and --slot options leaks opt_filter. It's benign
> for a small tool like this, but still wrong.
>
> > case OPT_BINARY:
> > config.binary = true;
> > break;
> > @@ -1258,6 +1351,9 @@ int main(int argc, char *argv[])
> > if (help || (argc > 0 && strcmp(argv[0], "help") == 0))
> > return intel_reg_help(&config, argc, argv);
> >
> > + if (argc > 0 && strcmp(argv[0], "list_filters") == 0)
> > + return intel_reg_list_filters(&config, argc, argv);
> > +
> > if (argc == 0) {
> > fprintf(stderr, "Command missing. Try intel_reg help.\n");
> > return EXIT_FAILURE;
> > @@ -1274,7 +1370,32 @@ int main(int argc, char *argv[])
> > fprintf(stderr, "--devid without --mmio\n");
> > return EXIT_FAILURE;
> > }
> > - config.pci_dev = intel_get_pci_device();
> > +
> > + if (pci_system_init() != 0) {
> > + fprintf(stderr, "Couldn't initialize PCI system\n");
> > + return EXIT_FAILURE;
> > + }
> > +
> > + igt_devices_scan(false);
> > +
> > + if (opt_filter) {
> > + if (!igt_device_card_match(opt_filter, &card)) {
> > + fprintf(stderr, "Requested device %s not found!\n", opt_filter);
> > + ret = EXIT_FAILURE;
> > + goto exit;
> > + }
> > + parse_pci_slot_name(&bdf, card.pci_slot_name);
> > + pci_dev = pci_device_find_by_slot(bdf.domain, bdf.bus, bdf.dev, bdf.func);
> > + } else {
> > + pci_dev = find_intel_graphics_card();
> > + }
> > +
> > + if (!is_graphics_card_valid(pci_dev)) {
> > + ret = EXIT_FAILURE;
> > + goto exit;
> > + }
> > +
>
> All of the above should be abstracted to a function instead of crammed
> into main().
>
> > + config.pci_dev = pci_dev;
> > config.devid = config.pci_dev->device_id;
> > }
> >
> > @@ -1301,5 +1422,8 @@ int main(int argc, char *argv[])
> > if (config.fd >= 0)
> > close(config.fd);
> >
> > +exit:
> > + igt_devices_free();
> > + free(opt_filter);
> > return ret;
> > }
>
> --
> Jani Nikula, Intel
More information about the igt-dev
mailing list