* [PATCH i-g-t] tools/intel_reg: add possibility to select device
@ 2024-03-14 22:58 Ashutosh Dixit
2024-03-14 23:01 ` Dixit, Ashutosh
` (3 more replies)
0 siblings, 4 replies; 8+ messages in thread
From: Ashutosh Dixit @ 2024-03-14 22:58 UTC (permalink / raw)
To: igt-dev; +Cc: Kamil Konieczny, Zbigniew Kempczynski
From: Łukasz Łaguna <lukasz.laguna@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"
Signed-off-by: Łukasz Łaguna <lukasz.laguna@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 {
+ 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",
+ .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);
+}
+
+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) {
+ fprintf(stderr, "opt_filter: %s\n",
+ strerror(errno));
+ return EXIT_FAILURE;
+ }
+ break;
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;
+ }
+
+ 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;
}
--
2.41.0
^ permalink raw reply related [flat|nested] 8+ messages in thread* Re: [PATCH i-g-t] tools/intel_reg: add possibility to select device 2024-03-14 22:58 [PATCH i-g-t] tools/intel_reg: add possibility to select device Ashutosh Dixit @ 2024-03-14 23:01 ` Dixit, Ashutosh 2024-03-15 8:44 ` Ville Syrjälä 2024-03-15 1:14 ` ✓ CI.xeBAT: success for " Patchwork ` (2 subsequent siblings) 3 siblings, 1 reply; 8+ messages in thread From: Dixit, Ashutosh @ 2024-03-14 23:01 UTC (permalink / raw) To: igt-dev; +Cc: Kamil Konieczny, Zbigniew Kempczynski, lukasz.laguna On Thu, 14 Mar 2024 15:58:39 -0700, Ashutosh Dixit wrote: > > From: Łukasz Łaguna <lukasz.laguna@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" > > Signed-off-by: Łukasz Łaguna <lukasz.laguna@intel.com> Is it possible to merge this old patch upstream? Thanks. ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH i-g-t] tools/intel_reg: add possibility to select device 2024-03-14 23:01 ` Dixit, Ashutosh @ 2024-03-15 8:44 ` Ville Syrjälä 0 siblings, 0 replies; 8+ messages in thread From: Ville Syrjälä @ 2024-03-15 8:44 UTC (permalink / raw) To: Dixit, Ashutosh Cc: igt-dev, Kamil Konieczny, Zbigniew Kempczynski, lukasz.laguna On Thu, Mar 14, 2024 at 04:01:55PM -0700, Dixit, Ashutosh wrote: > On Thu, 14 Mar 2024 15:58:39 -0700, Ashutosh Dixit wrote: > > > > From: Łukasz Łaguna <lukasz.laguna@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" > > > > Signed-off-by: Łukasz Łaguna <lukasz.laguna@intel.com> > > Is it possible to merge this old patch upstream? Thanks. Looks like two patches squashed to one. Someone should split it up properly. -- Ville Syrjälä Intel ^ permalink raw reply [flat|nested] 8+ messages in thread
* ✓ CI.xeBAT: success for tools/intel_reg: add possibility to select device 2024-03-14 22:58 [PATCH i-g-t] tools/intel_reg: add possibility to select device Ashutosh Dixit 2024-03-14 23:01 ` Dixit, Ashutosh @ 2024-03-15 1:14 ` Patchwork 2024-03-15 1:35 ` ✗ Fi.CI.BAT: failure " Patchwork 2024-03-15 9:13 ` [PATCH i-g-t] " Jani Nikula 3 siblings, 0 replies; 8+ messages in thread From: Patchwork @ 2024-03-15 1:14 UTC (permalink / raw) To: Dixit, Ashutosh; +Cc: igt-dev [-- Attachment #1: Type: text/plain, Size: 1641 bytes --] == Series Details == Series: tools/intel_reg: add possibility to select device URL : https://patchwork.freedesktop.org/series/131155/ State : success == Summary == CI Bug Log - changes from XEIGT_7766_BAT -> XEIGTPW_10839_BAT ==================================================== Summary ------- **SUCCESS** No regressions found. Participating hosts (4 -> 4) ------------------------------ No changes in participating hosts Known issues ------------ Here are the changes found in XEIGTPW_10839_BAT that come from known issues: ### IGT changes ### #### Issues hit #### * igt@xe_exec_threads@threads-mixed-shared-vm-basic: - bat-pvc-2: [PASS][1] -> [DMESG-WARN][2] ([Intel XE#1238]) [1]: https://intel-gfx-ci.01.org/tree/intel-xe/IGT_7766/bat-pvc-2/igt@xe_exec_threads@threads-mixed-shared-vm-basic.html [2]: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10839/bat-pvc-2/igt@xe_exec_threads@threads-mixed-shared-vm-basic.html [Intel XE#1238]: https://gitlab.freedesktop.org/drm/xe/kernel/issues/1238 Build changes ------------- * IGT: IGT_7766 -> IGTPW_10839 * Linux: xe-943-c44e29a7b62293355744fd857aad93cbf43e5126 -> xe-946-b7ead5c90db25002638773b1a9289220e6a36b4d IGTPW_10839: 10839 IGT_7766: 08cf1b2fa6b2f422f417ea74f41b12b93e91156f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git xe-943-c44e29a7b62293355744fd857aad93cbf43e5126: c44e29a7b62293355744fd857aad93cbf43e5126 xe-946-b7ead5c90db25002638773b1a9289220e6a36b4d: b7ead5c90db25002638773b1a9289220e6a36b4d == Logs == For more details see: https://intel-gfx-ci.01.org/tree/intel-xe/IGTPW_10839/index.html [-- Attachment #2: Type: text/html, Size: 2217 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* ✗ Fi.CI.BAT: failure for tools/intel_reg: add possibility to select device 2024-03-14 22:58 [PATCH i-g-t] tools/intel_reg: add possibility to select device Ashutosh Dixit 2024-03-14 23:01 ` Dixit, Ashutosh 2024-03-15 1:14 ` ✓ CI.xeBAT: success for " Patchwork @ 2024-03-15 1:35 ` Patchwork 2024-03-15 9:13 ` [PATCH i-g-t] " Jani Nikula 3 siblings, 0 replies; 8+ messages in thread From: Patchwork @ 2024-03-15 1:35 UTC (permalink / raw) To: Ashutosh Dixit; +Cc: igt-dev [-- Attachment #1: Type: text/plain, Size: 3961 bytes --] == Series Details == Series: tools/intel_reg: add possibility to select device URL : https://patchwork.freedesktop.org/series/131155/ State : failure == Summary == CI Bug Log - changes from CI_DRM_14437 -> IGTPW_10839 ==================================================== Summary ------- **FAILURE** Serious unknown changes coming with IGTPW_10839 absolutely need to be verified manually. If you think the reported changes have nothing to do with the changes introduced in IGTPW_10839, please notify your bug team (I915-ci-infra@lists.freedesktop.org) to allow them to document this new failure mode, which will reduce false positives in CI. External URL: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/index.html Participating hosts (35 -> 33) ------------------------------ Missing (2): bat-jsl-1 fi-snb-2520m Possible new issues ------------------- Here are the unknown changes that may have been introduced in IGTPW_10839: ### IGT changes ### #### Possible regressions #### * igt@dmabuf@all-tests@dma_fence_chain: - fi-kbl-8809g: [PASS][1] -> [INCOMPLETE][2] [1]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14437/fi-kbl-8809g/igt@dmabuf@all-tests@dma_fence_chain.html [2]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/fi-kbl-8809g/igt@dmabuf@all-tests@dma_fence_chain.html Known issues ------------ Here are the changes found in IGTPW_10839 that come from known issues: ### IGT changes ### #### Issues hit #### * igt@gem_huc_copy@huc-copy: - fi-cfl-8109u: NOTRUN -> [SKIP][3] ([i915#2190]) [3]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/fi-cfl-8109u/igt@gem_huc_copy@huc-copy.html * igt@gem_lmem_swapping@verify-random: - fi-cfl-8109u: NOTRUN -> [SKIP][4] ([i915#4613]) +3 other tests skip [4]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/fi-cfl-8109u/igt@gem_lmem_swapping@verify-random.html * igt@i915_selftest@live@gt_lrc: - bat-dg2-9: [PASS][5] -> [ABORT][6] ([i915#10366]) [5]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14437/bat-dg2-9/igt@i915_selftest@live@gt_lrc.html [6]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/bat-dg2-9/igt@i915_selftest@live@gt_lrc.html * igt@i915_selftest@live@hangcheck: - bat-rpls-3: [PASS][7] -> [DMESG-WARN][8] ([i915#5591]) [7]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14437/bat-rpls-3/igt@i915_selftest@live@hangcheck.html [8]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/bat-rpls-3/igt@i915_selftest@live@hangcheck.html * igt@kms_pm_backlight@basic-brightness: - fi-cfl-8109u: NOTRUN -> [SKIP][9] +11 other tests skip [9]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/fi-cfl-8109u/igt@kms_pm_backlight@basic-brightness.html #### Possible fixes #### * igt@gem_lmem_swapping@basic@lmem0: - bat-dg2-14: [FAIL][10] ([i915#10378]) -> [PASS][11] [10]: https://intel-gfx-ci.01.org/tree/drm-tip/CI_DRM_14437/bat-dg2-14/igt@gem_lmem_swapping@basic@lmem0.html [11]: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/bat-dg2-14/igt@gem_lmem_swapping@basic@lmem0.html [i915#10366]: https://gitlab.freedesktop.org/drm/intel/issues/10366 [i915#10378]: https://gitlab.freedesktop.org/drm/intel/issues/10378 [i915#2190]: https://gitlab.freedesktop.org/drm/intel/issues/2190 [i915#4613]: https://gitlab.freedesktop.org/drm/intel/issues/4613 [i915#5591]: https://gitlab.freedesktop.org/drm/intel/issues/5591 Build changes ------------- * CI: CI-20190529 -> None * IGT: IGT_7766 -> IGTPW_10839 CI-20190529: 20190529 CI_DRM_14437: b7ead5c90db25002638773b1a9289220e6a36b4d @ git://anongit.freedesktop.org/gfx-ci/linux IGTPW_10839: 10839 IGT_7766: 08cf1b2fa6b2f422f417ea74f41b12b93e91156f @ https://gitlab.freedesktop.org/drm/igt-gpu-tools.git == Logs == For more details see: https://intel-gfx-ci.01.org/tree/drm-tip/IGTPW_10839/index.html [-- Attachment #2: Type: text/html, Size: 4734 bytes --] ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH i-g-t] tools/intel_reg: add possibility to select device 2024-03-14 22:58 [PATCH i-g-t] tools/intel_reg: add possibility to select device Ashutosh Dixit ` (2 preceding siblings ...) 2024-03-15 1:35 ` ✗ Fi.CI.BAT: failure " Patchwork @ 2024-03-15 9:13 ` Jani Nikula 2024-08-22 12:24 ` Kamil Konieczny 3 siblings, 1 reply; 8+ messages in thread From: Jani Nikula @ 2024-03-15 9:13 UTC (permalink / raw) To: Ashutosh Dixit, igt-dev; +Cc: Kamil Konieczny, Zbigniew Kempczynski On Thu, 14 Mar 2024, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > From: Łukasz Łaguna <lukasz.laguna@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. > Signed-off-by: Łukasz Łaguna <lukasz.laguna@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH i-g-t] tools/intel_reg: add possibility to select device 2024-03-15 9:13 ` [PATCH i-g-t] " Jani Nikula @ 2024-08-22 12:24 ` Kamil Konieczny 2024-08-22 12:45 ` Jani Nikula 0 siblings, 1 reply; 8+ messages in thread From: Kamil Konieczny @ 2024-08-22 12:24 UTC (permalink / raw) To: igt-dev Cc: Jani Nikula, Ashutosh Dixit, Zbigniew Kempczynski, Joonas Lahtinen, Łukasz Łaguna Hi Jani, On 2024-03-15 at 11:13:37 +0200, Jani Nikula wrote: > On Thu, 14 Mar 2024, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: > > From: Łukasz Łaguna <lukasz.laguna@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@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 ^ permalink raw reply [flat|nested] 8+ messages in thread
* Re: [PATCH i-g-t] tools/intel_reg: add possibility to select device 2024-08-22 12:24 ` Kamil Konieczny @ 2024-08-22 12:45 ` Jani Nikula 0 siblings, 0 replies; 8+ messages in thread From: Jani Nikula @ 2024-08-22 12:45 UTC (permalink / raw) To: Kamil Konieczny, igt-dev Cc: Ashutosh Dixit, Zbigniew Kempczynski, Joonas Lahtinen, Łukasz Łaguna On Thu, 22 Aug 2024, Kamil Konieczny <kamil.konieczny@linux.intel.com> wrote: > Hi Jani, > On 2024-03-15 at 11:13:37 +0200, Jani Nikula wrote: >> On Thu, 14 Mar 2024, Ashutosh Dixit <ashutosh.dixit@intel.com> wrote: >> > From: Łukasz Łaguna <lukasz.laguna@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. It's been a while, but I think I meant (but clearly didn't actually say), please try to follow the lspci style for -s and -d options, but not necessarily the names of the options. I'm always in favor of long options over short options anyway. Perhaps --pci-slot and --pci-device and have their values match the -s and -d options for lspci, respectively? --pci-slot [[[[<domain>]:]<bus>]:][<device>][.[<func>]] --pci-device [<vendor>]:[<device>][:<class>[:<prog-if>]] And maybe start with a compatible subset of the optional [] stuff first, and expand from there. BR, Jani. > > Regards, > Kamil > >> > Signed-off-by: Łukasz Łaguna <lukasz.laguna@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 -- Jani Nikula, Intel ^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2024-08-22 12:45 UTC | newest] Thread overview: 8+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 2024-03-14 22:58 [PATCH i-g-t] tools/intel_reg: add possibility to select device Ashutosh Dixit 2024-03-14 23:01 ` Dixit, Ashutosh 2024-03-15 8:44 ` Ville Syrjälä 2024-03-15 1:14 ` ✓ CI.xeBAT: success for " Patchwork 2024-03-15 1:35 ` ✗ Fi.CI.BAT: failure " Patchwork 2024-03-15 9:13 ` [PATCH i-g-t] " Jani Nikula 2024-08-22 12:24 ` Kamil Konieczny 2024-08-22 12:45 ` Jani Nikula
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox