* [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
* ✓ 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 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
* 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