Some style/flow issues inline below.
On 22/06/17 04:36 PM, Jean-Francois Thibert wrote:
This allows selecting the GPU by its PCI device both with and
without kernel mode support. The instance is populated automatically
so that the proper corresponding debugfs files are used if present.
Signed-off-by: Jean-Francois Thibert <jfthib...@google.com>
---
src/app/main.c | 9 ++++++
src/lib/discover.c | 85 ++++++++++++++++++++++++++++++++++++++++++++++++++++--
src/umr.h | 6 +++-
3 files changed, 96 insertions(+), 4 deletions(-)
diff --git a/src/app/main.c b/src/app/main.c
index 1d9ef9e..aa630f5 100644
--- a/src/app/main.c
+++ b/src/app/main.c
@@ -174,6 +174,15 @@ int main(int argc, char **argv)
printf("--force requires a number/name\n");
return EXIT_FAILURE;
}
+ } else if (!strcmp(argv[i], "--pci")) {
+ if (i + 1 < argc && sscanf(argv[i+1],
"%04x:%02x:%02x.%01x",
+ &options.pci_domain, &options.pci_bus,
&options.pci_slot,
+ &options.pci_func ) >= 4) {
+ ++i;
+ } else {
+ printf("--pci requires
domain:bus:slot.function\n");
+ return EXIT_FAILURE;
+ }
Could use man/help text updates. I can do that though in a followup patch.
} else if (!strcmp(argv[i], "--print") || !strcmp(argv[i],
"-p")) {
options.print = 1;
options.need_scan = 1;
diff --git a/src/lib/discover.c b/src/lib/discover.c
index 9662d05..8ab321f 100644
--- a/src/lib/discover.c
+++ b/src/lib/discover.c
@@ -22,6 +22,9 @@
* Authors: Tom St Denis <tom.stde...@amd.com>
*
*/
+#include <dirent.h>
+#include <sys/types.h>
+
#include "umr.h"
static int is_did_match(struct umr_asic *asic, unsigned did)
@@ -44,6 +47,43 @@ static int is_did_match(struct umr_asic *asic, unsigned did)
return r;
}
+static int find_pci_instance(const char* pci_string) {
+ DIR* dir;
+ dir = opendir("/sys/kernel/debug/dri");
+ if (dir == NULL) {
+ perror("Couldn't open DRI under debugfs");
+ return -1;
+ }
+ struct dirent *dir_entry;
+ while ((dir_entry = readdir(dir)) != NULL) {
+ // ignore . and ..
+ if (strcmp(dir_entry->d_name, ".") == 0 ||
strcmp(dir_entry->d_name,
+ "..") == 0) {
+ continue;
+ }
+ char name[256];
I try not to use C99/C++ style declaration-in-scope (except at the start
of scope) variables. It makes the code easier to follow, e.g. "how big
is name, oh look at the start of the block).
Also I use kernel style {} rules where single statement blocks don't
receive {} unless they're paired with another related (e.g. if/else)
block which has multiple statements.
+ snprintf(name, sizeof(name) - 1,
"/sys/kernel/debug/dri/%s/name",
+ dir_entry->d_name);
+ FILE *f = fopen(name, "r");
+ if (!f) {
+ continue;
+ }
+ char device[256] = {};
+ int parsed_device = fscanf(f, "%*s %255s", device);
+ fclose(f);
+ if (parsed_device != 1)
+ continue;
+ // strip off dev= for kernels > 4.7
+ if (strstr(device, "dev="))
+ memmove(device, device+4, strlen(device)-3);
+ if (strcmp(pci_string, device) == 0) {
+ closedir(dir);
+ return atoi(dir_entry->d_name);
+ }
+ }
+ closedir(dir);
+ return -1;
+}
struct umr_asic *umr_discover_asic(struct umr_options *options)
{
@@ -53,6 +93,29 @@ struct umr_asic *umr_discover_asic(struct umr_options
*options)
struct umr_asic *asic;
long trydid = options->forcedid;
+ // Try to map to instance if we have a specific pci device
+ if (options->pci_domain || options->pci_bus ||
+ options->pci_slot || options->pci_func) {
+ char pci_string[16];
+ snprintf(pci_string, sizeof(pci_string) - 1,
"%04x:%02x:%02x.%x",
+ options->pci_domain, options->pci_bus,
options->pci_slot,
+ options->pci_func);
+ if (!options->no_kernel) {
+ options->instance = find_pci_instance(pci_string);
+ }
+ snprintf(driver, sizeof(driver)-1,
"/sys/bus/pci/devices/%s/device", pci_string);
+ f = fopen(driver, "r");
+ if (!f) {
+ if (!options->quiet) perror("Cannot open PCI device name
under sysfs (is a display attached?)");
+ return NULL;
+ }
+ int found_did = fscanf(f, "0x%04lx", &trydid);
+ fclose(f);
+ if (found_did != 1) {
+ if (!options->quiet) printf("Could not read device id");
+ return NULL;
+ }
Maybe set trydid here instead for consistency.
+ }
// try to scan via debugfs
asic = calloc(1, sizeof *asic);
if (asic) {
Not part of your patch but we should gate all of this behind "if
(!options->no_kernel)" since the debugfs entry cannot be used in that
mode. I can do that in a followup patch.
@@ -64,7 +127,6 @@ struct umr_asic *umr_discover_asic(struct umr_options
*options)
umr_free_asic(asic);
asic = NULL;
}
-
if (trydid < 0) {
snprintf(name, sizeof(name)-1, "/sys/kernel/debug/dri/%d/name",
options->instance);
f = fopen(name, "r");
@@ -86,8 +148,12 @@ struct umr_asic *umr_discover_asic(struct umr_options
*options)
}
return NULL;
}
- fscanf(f, "%s %s %s\n", driver, name, driver);
+ int parsed_pci_id = fscanf(f, "%*s %s", name);
fclose(f);
+ if (parsed_pci_id != 1) {
+ if (!options->quiet) printf("Cannot read pci device
id\n");
+ return NULL;
+ }
// strip off dev= for kernels > 4.7
if (strstr(name, "dev="))
@@ -99,8 +165,12 @@ struct umr_asic *umr_discover_asic(struct umr_options
*options)
if (!options->quiet) perror("Cannot open PCI device name
under sysfs (is a display attached?)");
return NULL;
}
- fscanf(f, "0x%04x", &did);
+ int parsed_did = fscanf(f, "0x%04x", &did);
fclose(f);
+ if (parsed_did != 1) {
+ if (!options->quiet) printf("Could not read device id");
+ return NULL;
+ }
asic = umr_discover_asic_by_did(options, did);
} else {
if (options->dev_name[0])
@@ -158,6 +228,15 @@ struct umr_asic *umr_discover_asic(struct umr_options
*options)
}
do {
asic->pci.pdevice = pci_device_next(pci_iter);
+ if (options->pci_domain || options->pci_bus ||
options->pci_slot || options->pci_func) {
+ while (asic->pci.pdevice && (
+ options->pci_domain !=
asic->pci.pdevice->domain ||
+ options->pci_bus !=
asic->pci.pdevice->bus ||
+ options->pci_slot !=
asic->pci.pdevice->dev ||
+ options->pci_func !=
asic->pci.pdevice->func)) {
+ asic->pci.pdevice =
pci_device_next(pci_iter);
+ }
+ }
} while (asic->pci.pdevice && !(asic->pci.pdevice->vendor_id ==
0x1002 && is_did_match(asic, asic->pci.pdevice->device_id)));
if (!asic->pci.pdevice) {
diff --git a/src/umr.h b/src/umr.h
index ccfac5d..391c5e7 100644
--- a/src/umr.h
+++ b/src/umr.h
@@ -173,7 +173,11 @@ struct umr_options {
read_smc,
quiet,
follow_ib,
- no_kernel;
+ no_kernel,
+ pci_domain,
+ pci_bus,
+ pci_slot,
+ pci_func;
As I commented in private maybe switch this to
struct pci {
int domain,
bus,
slot,
func;
};
Inside the options structure since it will make the code easier to read
and follow.
With the style issues fixed up: Reviewed-by: Tom St Denis
<tom.stde...@amd.com>.
Tom
_______________________________________________
amd-gfx mailing list
amd-gfx@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/amd-gfx
