[PATCH libdrm 01/10] xf86drm: drmGetDevice2: error out if the fd has unknown subsys

[PATCH libdrm 01/10] xf86drm: drmGetDevice2: error out if the fd has unknown subsys

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Currently one can open() any /dev node. If it's unknown
drmParseSubsystemType() will return an error.

Track that and bail as needed.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/xf86drm.c b/xf86drm.c
index 87c216cf..e1bbbe99 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3814,6 +3814,8 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
         return -EINVAL;
 
     subsystem_type = drmParseSubsystemType(maj, min);
+    if (subsystem_type < 0)
+        return subsystem_type;
 
     local_devices = calloc(max_count, sizeof(drmDevicePtr));
     if (local_devices == NULL)
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 02/10] xf86drm: introduce drm_device_has_rdev() helper

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Currently we match the opened drmDevice fd with each drmDevice we
process.

Move that after all the devices are processed and folded, via the
drm_device_has_rdev(). This makes the code easier to follow and allows
us to unify the massive process loop across drmGetDevice2 and
drmGetDevices2. That in itself is coming with a later commit.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 34 +++++++++++++++++++++++++---------
 1 file changed, 25 insertions(+), 9 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index e1bbbe99..cbc0a408 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3705,6 +3705,21 @@ drm_device_validate_flags(uint32_t flags)
         return (flags & ~DRM_DEVICE_GET_PCI_REVISION);
 }
 
+static bool
+drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
+{
+    struct stat sbuf;
+
+    for (int i = 0; i < DRM_NODE_MAX; i++) {
+        if (device->available_nodes & 1 << i) {
+            if (stat(device->nodes[i], &sbuf) == 0 &&
+                sbuf.st_rdev == find_rdev)
+                return true;
+        }
+    }
+    return false;
+}
+
 /**
  * Get information about the opened drm device
  *
@@ -3889,21 +3904,22 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
             local_devices = temp;
         }
 
-        /* store target at local_devices[0] for ease to use below */
-        if (find_rdev == sbuf.st_rdev && i) {
-            local_devices[i] = local_devices[0];
-            local_devices[0] = d;
-        }
-        else
-            local_devices[i] = d;
+        local_devices[i] = d;
         i++;
     }
     node_count = i;
 
     drmFoldDuplicatedDevices(local_devices, node_count);
 
-    *device = local_devices[0];
-    drmFreeDevices(&local_devices[1], node_count - 1);
+    for (i = 0; i < node_count; i++) {
+        if (!local_devices[i])
+            continue;
+
+        if (drm_device_has_rdev(local_devices[i], find_rdev))
+            *device = local_devices[i];
+        else
+            drmFreeDevice(&local_devices[i]);
+    }
 
     closedir(sysdir);
     free(local_devices);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 03/10] xf86drm: Fold drmDevice processing into process_device() helper

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Don't the duplicate (nearly) identical code across the two call sites.
It improves legibility and the diff stat seems nice.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 159 ++++++++++++++++++------------------------------------
 1 file changed, 51 insertions(+), 108 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index cbc0a408..114cf855 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3676,6 +3676,52 @@ free_device:
     return ret;
 }
 
+static int
+process_device(drmDevicePtr *device, const char *d_name,
+               int req_subsystem_type,
+               bool fetch_deviceinfo, uint32_t flags)
+{
+    struct stat sbuf;
+    char node[PATH_MAX + 1];
+    int node_type, subsystem_type;
+    unsigned int maj, min;
+
+    node_type = drmGetNodeType(d_name);
+    if (node_type < 0)
+        return -1;
+
+    snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, d_name);
+    if (stat(node, &sbuf))
+        return -1;
+
+    maj = major(sbuf.st_rdev);
+    min = minor(sbuf.st_rdev);
+
+    if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
+        return -1;
+
+    subsystem_type = drmParseSubsystemType(maj, min);
+    if (req_subsystem_type != -1 && req_subsystem_type != subsystem_type)
+        return -1;
+
+    switch (subsystem_type) {
+    case DRM_BUS_PCI:
+        return drmProcessPciDevice(device, node, node_type, maj, min,
+                                   fetch_deviceinfo, flags);
+    case DRM_BUS_USB:
+        return drmProcessUsbDevice(device, node, node_type, maj, min,
+                                   fetch_deviceinfo, flags);
+    case DRM_BUS_PLATFORM:
+        return drmProcessPlatformDevice(device, node, node_type, maj, min,
+                                        fetch_deviceinfo, flags);
+    case DRM_BUS_HOST1X:
+        return drmProcessHost1xDevice(device, node, node_type, maj, min,
+                                      fetch_deviceinfo, flags);
+    default:
+        return -1;
+   }
+}
+
 /* Consider devices located on the same bus as duplicate and fold the respective
  * entries into a single one.
  *
@@ -3805,8 +3851,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     DIR *sysdir;
     struct dirent *dent;
     struct stat sbuf;
-    char node[PATH_MAX + 1];
-    int node_type, subsystem_type;
+    int subsystem_type;
     int maj, min;
     int ret, i, node_count;
     int max_count = 16;
@@ -3844,55 +3889,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 
     i = 0;
     while ((dent = readdir(sysdir))) {
-        node_type = drmGetNodeType(dent->d_name);
-        if (node_type < 0)
-            continue;
-
-        snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
-        if (stat(node, &sbuf))
-            continue;
-
-        maj = major(sbuf.st_rdev);
-        min = minor(sbuf.st_rdev);
-
-        if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
-            continue;
-
-        if (drmParseSubsystemType(maj, min) != subsystem_type)
-            continue;
-
-        switch (subsystem_type) {
-        case DRM_BUS_PCI:
-            ret = drmProcessPciDevice(&d, node, node_type, maj, min, true, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_USB:
-            ret = drmProcessUsbDevice(&d, node, node_type, maj, min, true, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_PLATFORM:
-            ret = drmProcessPlatformDevice(&d, node, node_type, maj, min, true, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_HOST1X:
-            ret = drmProcessHost1xDevice(&d, node, node_type, maj, min, true, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        default:
+        ret = process_device(&d, dent->d_name, subsystem_type, true, flags);
+        if (ret)
             continue;
-        }
 
         if (i >= max_count) {
             drmDevicePtr *temp;
@@ -3973,10 +3972,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
     drmDevicePtr device;
     DIR *sysdir;
     struct dirent *dent;
-    struct stat sbuf;
-    char node[PATH_MAX + 1];
-    int node_type, subsystem_type;
-    int maj, min;
     int ret, i, node_count, device_count;
     int max_count = 16;
 
@@ -3995,61 +3990,9 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 
     i = 0;
     while ((dent = readdir(sysdir))) {
-        node_type = drmGetNodeType(dent->d_name);
-        if (node_type < 0)
-            continue;
-
-        snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
-        if (stat(node, &sbuf))
-            continue;
-
-        maj = major(sbuf.st_rdev);
-        min = minor(sbuf.st_rdev);
-
-        if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
-            continue;
-
-        subsystem_type = drmParseSubsystemType(maj, min);
-
-        if (subsystem_type < 0)
-            continue;
-
-        switch (subsystem_type) {
-        case DRM_BUS_PCI:
-            ret = drmProcessPciDevice(&device, node, node_type,
-                                      maj, min, devices != NULL, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_USB:
-            ret = drmProcessUsbDevice(&device, node, node_type, maj, min,
-                                      devices != NULL, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_PLATFORM:
-            ret = drmProcessPlatformDevice(&device, node, node_type, maj, min,
-                                           devices != NULL, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        case DRM_BUS_HOST1X:
-            ret = drmProcessHost1xDevice(&device, node, node_type, maj, min,
-                                         devices != NULL, flags);
-            if (ret)
-                continue;
-
-            break;
-
-        default:
+        ret = process_device(&device, dent->d_name, -1, devices != NULL, flags);
+        if (ret)
             continue;
-        }
 
         if (i >= max_count) {
             drmDevicePtr *temp;
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Currently we dynamically allocate 16 pointers and reallocate more as
needed.

Instead, allocate the maximum number (256) on stack - the number is
small enough and is unlikely to change in the foreseeable future.

This allows us to simplify the error handling and even shed a few bytes
off the final binary.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 64 ++++++-------------------------------------------------
 1 file changed, 6 insertions(+), 58 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 114cf855..d4810740 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 
     return 0;
 #else
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[256];
     drmDevicePtr d;
     DIR *sysdir;
     struct dirent *dent;
@@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     int subsystem_type;
     int maj, min;
     int ret, i, node_count;
-    int max_count = 16;
     dev_t find_rdev;
 
     if (drm_device_validate_flags(flags))
@@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     if (subsystem_type < 0)
         return subsystem_type;
 
-    local_devices = calloc(max_count, sizeof(drmDevicePtr));
-    if (local_devices == NULL)
-        return -ENOMEM;
-
     sysdir = opendir(DRM_DIR_NAME);
-    if (!sysdir) {
-        ret = -errno;
-        goto free_locals;
-    }
+    if (!sysdir)
+        return -errno;
 
     i = 0;
     while ((dent = readdir(sysdir))) {
@@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
         if (ret)
             continue;
 
-        if (i >= max_count) {
-            drmDevicePtr *temp;
-
-            max_count += 16;
-            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
-            if (!temp)
-                goto free_devices;
-            local_devices = temp;
-        }
-
         local_devices[i] = d;
         i++;
     }
@@ -3921,18 +3904,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     }
 
     closedir(sysdir);
-    free(local_devices);
     if (*device == NULL)
         return -ENODEV;
     return 0;
-
-free_devices:
-    drmFreeDevices(local_devices, i);
-    closedir(sysdir);
-
-free_locals:
-    free(local_devices);
-    return ret;
 #endif
 }
 
@@ -3968,25 +3942,18 @@ int drmGetDevice(int fd, drmDevicePtr *device)
  */
 int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 {
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[256];
     drmDevicePtr device;
     DIR *sysdir;
     struct dirent *dent;
     int ret, i, node_count, device_count;
-    int max_count = 16;
 
     if (drm_device_validate_flags(flags))
         return -EINVAL;
 
-    local_devices = calloc(max_count, sizeof(drmDevicePtr));
-    if (local_devices == NULL)
-        return -ENOMEM;
-
     sysdir = opendir(DRM_DIR_NAME);
-    if (!sysdir) {
-        ret = -errno;
-        goto free_locals;
-    }
+    if (!sysdir)
+        return -errno;
 
     i = 0;
     while ((dent = readdir(sysdir))) {
@@ -3994,16 +3961,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
         if (ret)
             continue;
 
-        if (i >= max_count) {
-            drmDevicePtr *temp;
-
-            max_count += 16;
-            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
-            if (!temp)
-                goto free_devices;
-            local_devices = temp;
-        }
-
         local_devices[i] = device;
         i++;
     }
@@ -4025,16 +3982,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
     }
 
     closedir(sysdir);
-    free(local_devices);
     return device_count;
-
-free_devices:
-    drmFreeDevices(local_devices, i);
-    closedir(sysdir);
-
-free_locals:
-    free(local_devices);
-    return ret;
 }
 
 /**
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 05/10] xf86drm: introduce a get_real_pci_path() helper

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Introduce a helper which gets the real sysfs path for the given pci
device.

In other words, instead opening the /sys/dev/char/*/device symlink, we
opt for the actual /sys/devices/pci*/*/

It folds three (nearly identical) snprintf's and paves the way of adding
extra devices (see next patch) a piece of pie.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++-----------
 1 file changed, 50 insertions(+), 12 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index d4810740..8ccd528f 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min)
 #endif
 }
 
+static char *
+get_real_pci_path(int maj, int min)
+{
+    char path[PATH_MAX + 1];
+    char *real_path = malloc(PATH_MAX);
+
+    if (!real_path)
+        return NULL;
+
+    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
+    if (!realpath(path, real_path)) {
+        free(real_path);
+        return NULL;
+    }
+
+    return real_path;
+}
+
 static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
 {
 #ifdef __linux__
     unsigned int domain, bus, dev, func;
-    char path[PATH_MAX + 1], *value;
+    char *real_path, *value;
     int num;
 
-    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
+    real_path = get_real_pci_path(maj, min);
+    if (!real_path)
+        return -ENOENT;
 
-    value = sysfs_uevent_get(path, "PCI_SLOT_NAME");
+    value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
+    free(real_path);
     if (!value)
         return -ENOENT;
 
@@ -3114,24 +3135,32 @@ static int parse_separate_sysfs_files(int maj, int min,
       "subsystem_vendor",
       "subsystem_device",
     };
-    char path[PATH_MAX + 1];
+    char path[PATH_MAX + 1], *real_path;
     unsigned int data[ARRAY_SIZE(attrs)];
     FILE *fp;
     int ret;
 
+    real_path = get_real_pci_path(maj, min);
+    if (!real_path)
+        return -ENOENT;
+
     for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {
-        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
-                 attrs[i]);
+        snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
         fp = fopen(path, "r");
-        if (!fp)
+        if (!fp) {
+            free(real_path);
             return -errno;
+        }
 
         ret = fscanf(fp, "%x", &data[i]);
         fclose(fp);
-        if (ret != 1)
+        if (ret != 1) {
+            free(real_path);
             return -errno;
+        }
 
     }
+    free(real_path);
 
     device->revision_id = ignore_revision ? 0xff : data[0] & 0xff;
     device->vendor_id = data[1] & 0xffff;
@@ -3145,19 +3174,28 @@ static int parse_separate_sysfs_files(int maj, int min,
 static int parse_config_sysfs_file(int maj, int min,
                                    drmPciDeviceInfoPtr device)
 {
-    char path[PATH_MAX + 1];
+    char path[PATH_MAX + 1], *real_path;
     unsigned char config[64];
     int fd, ret;
 
-    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);
+    real_path = get_real_pci_path(maj, min);
+    if (!real_path)
+        return -ENOENT;
+
+    snprintf(path, PATH_MAX, "%s/config", real_path);
     fd = open(path, O_RDONLY);
-    if (fd < 0)
+    if (fd < 0) {
+        free(real_path);
         return -errno;
+    }
 
     ret = read(fd, config, sizeof(config));
     close(fd);
-    if (ret < 0)
+    if (ret < 0) {
+        free(real_path);
         return -errno;
+    }
+    free(real_path);
 
     device->vendor_id = config[0] | (config[1] << 8);
     device->device_id = config[2] | (config[3] << 8);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 06/10] xf86drm: Add drmDevice support for virtio_gpu

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

The GPU almost exclusively lives on the PCI bus, so we expose it as a
normal PCI one.

This allows any existing drmDevice users to work without any changes.

One could wonder why a separate typeset is not introduced, alike say
host1x. Unlike host1x the PCI/platform distinction for virtio provides
no extra information. Plus needed we can add the separate set at a later
stage.

Here are a couple of 'features' that virtio seems to be missing:
 - provides extra information on top the plaform devices
 - supports a range of GPU devices
 - is considered hardware description (DT)

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 xf86drm.c | 13 ++++++++++++-
 1 file changed, 12 insertions(+), 1 deletion(-)

diff --git a/xf86drm.c b/xf86drm.c
index 8ccd528f..b847ea26 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2954,6 +2954,9 @@ sysfs_uevent_get(const char *path, const char *fmt, ...)
 }
 #endif
 
+/* Little white lie to avoid major rework of the existing code */
+#define DRM_BUS_VIRTIO 0x10
+
 static int drmParseSubsystemType(int maj, int min)
 {
 #ifdef __linux__
@@ -2983,6 +2986,9 @@ static int drmParseSubsystemType(int maj, int min)
     if (strncmp(name, "/host1x", 7) == 0)
         return DRM_BUS_HOST1X;
 
+    if (strncmp(name, "/virtio", 7) == 0)
+        return DRM_BUS_VIRTIO;
+
     return -EINVAL;
 #elif defined(__OpenBSD__)
     return DRM_BUS_PCI;
@@ -2996,7 +3002,7 @@ static char *
 get_real_pci_path(int maj, int min)
 {
     char path[PATH_MAX + 1];
-    char *real_path = malloc(PATH_MAX);
+    char *term, *real_path = malloc(PATH_MAX);
 
     if (!real_path)
         return NULL;
@@ -3007,6 +3013,10 @@ get_real_pci_path(int maj, int min)
         return NULL;
     }
 
+    term = strrchr(real_path, '/');
+    if (term && strncmp(term, "/virtio", 7) == 0)
+        *term = 0;
+
     return real_path;
 }
 
@@ -3744,6 +3754,7 @@ process_device(drmDevicePtr *device, const char *d_name,
 
     switch (subsystem_type) {
     case DRM_BUS_PCI:
+    case DRM_BUS_VIRTIO:
         return drmProcessPciDevice(device, node, node_type, maj, min,
                                    fetch_deviceinfo, flags);
     case DRM_BUS_USB:
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 07/10] tests/drmdevices: install alongside other utilities

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

It's mildly useful program, to ship it when the user wants the "tests"
installed. Obviously the "tests" in the name is a misnomer.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/Makefile.am | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/tests/Makefile.am b/tests/Makefile.am
index 0355a925..b72c24f9 100644
--- a/tests/Makefile.am
+++ b/tests/Makefile.am
@@ -43,5 +43,10 @@ TESTS = \
 	random
 
 check_PROGRAMS = \
-	$(TESTS) \
-	drmdevice
+	$(TESTS)
+
+if HAVE_INSTALL_TESTS
+bin_PROGRAMS = drmdevice
+else
+check_PROGRAMS += drmdevice
+endif
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 08/10] tests/drmdevice: add a couple of printf headers

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Add a few printf statements, which should make the output easier to
parse.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/drmdevice.c | 7 ++++++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c
index 9dd5098a..0d75836f 100644
--- a/tests/drmdevice.c
+++ b/tests/drmdevice.c
@@ -112,12 +112,15 @@ main(void)
     drmDevicePtr device;
     int fd, ret, max_devices;
 
+    printf("--- Checking the number of DRM device available ---\n");
     max_devices = drmGetDevices2(0, NULL, 0);
 
     if (max_devices <= 0) {
         printf("drmGetDevices2() has returned %d\n", max_devices);
         return -1;
     }
+    printf("--- Devices reported %d ---\n", max_devices);
+
 
     devices = calloc(max_devices, sizeof(drmDevicePtr));
     if (devices == NULL) {
@@ -125,6 +128,7 @@ main(void)
         return -1;
     }
 
+    printf("--- Retrieving devices information (PCI device revision is ignored) ---\n");
     ret = drmGetDevices2(0, devices, max_devices);
     if (ret < 0) {
         printf("drmGetDevices2() returned an error %d\n", ret);
@@ -137,13 +141,14 @@ main(void)
 
         for (int j = 0; j < DRM_NODE_MAX; j++) {
             if (devices[i]->available_nodes & 1 << j) {
-                printf("Opening device %d node %s\n", i, devices[i]->nodes[j]);
+                printf("--- Opening device node %s ---\n", devices[i]->nodes[j]);
                 fd = open(devices[i]->nodes[j], O_RDONLY | O_CLOEXEC, 0);
                 if (fd < 0) {
                     printf("Failed - %s (%d)\n", strerror(errno), errno);
                     continue;
                 }
 
+                printf("--- Retrieving device info, for node %s ---\n", devices[i]->nodes[j]);
                 if (drmGetDevice2(fd, DRM_DEVICE_GET_PCI_REVISION, &device) == 0) {
                     print_device_info(device, i, true);
                     drmFreeDevice(&device);
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 09/10] drmdevice: convert the tabbed output into a tree

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Making the output a little bit easier to parse by human beings.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/drmdevice.c | 78 +++++++++++++++++++++++------------------------
 1 file changed, 39 insertions(+), 39 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c
index 0d75836f..e9e9d7f1 100644
--- a/tests/drmdevice.c
+++ b/tests/drmdevice.c
@@ -36,67 +36,67 @@ static void
 print_device_info(drmDevicePtr device, int i, bool print_revision)
 {
     printf("device[%i]\n", i);
-    printf("\tavailable_nodes %04x\n", device->available_nodes);
-    printf("\tnodes\n");
+    printf("+->available_nodes %#04x\n", device->available_nodes);
+    printf("+->nodes\n");
     for (int j = 0; j < DRM_NODE_MAX; j++)
         if (device->available_nodes & 1 << j)
-            printf("\t\tnodes[%d] %s\n", j, device->nodes[j]);
+            printf("|  +->nodes[%d] %s\n", j, device->nodes[j]);
 
-    printf("\tbustype %04x\n", device->bustype);
-    printf("\tbusinfo\n");
+    printf("+->bustype %04x\n", device->bustype);
+    printf("+->businfo\n");
     if (device->bustype == DRM_BUS_PCI) {
-        printf("\t\tpci\n");
-        printf("\t\t\tdomain\t%04x\n",device->businfo.pci->domain);
-        printf("\t\t\tbus\t%02x\n", device->businfo.pci->bus);
-        printf("\t\t\tdev\t%02x\n", device->businfo.pci->dev);
-        printf("\t\t\tfunc\t%1u\n", device->businfo.pci->func);
-
-        printf("\tdeviceinfo\n");
-        printf("\t\tpci\n");
-        printf("\t\t\tvendor_id\t%04x\n", device->deviceinfo.pci->vendor_id);
-        printf("\t\t\tdevice_id\t%04x\n", device->deviceinfo.pci->device_id);
-        printf("\t\t\tsubvendor_id\t%04x\n", device->deviceinfo.pci->subvendor_id);
-        printf("\t\t\tsubdevice_id\t%04x\n", device->deviceinfo.pci->subdevice_id);
+        printf("|  +->pci\n");
+        printf("|     +->domain %04x\n",device->businfo.pci->domain);
+        printf("|     +->bus    %02x\n", device->businfo.pci->bus);
+        printf("|     +->dev    %02x\n", device->businfo.pci->dev);
+        printf("|     +->func   %1u\n", device->businfo.pci->func);
+
+        printf("+->deviceinfo\n");
+        printf("    +->pci\n");
+        printf("       +->vendor_id     %04x\n", device->deviceinfo.pci->vendor_id);
+        printf("       +->device_id     %04x\n", device->deviceinfo.pci->device_id);
+        printf("       +->subvendor_id  %04x\n", device->deviceinfo.pci->subvendor_id);
+        printf("       +->subdevice_id  %04x\n", device->deviceinfo.pci->subdevice_id);
         if (print_revision)
-            printf("\t\t\trevision_id\t%02x\n", device->deviceinfo.pci->revision_id);
+            printf("       +->revision_id   %02x\n", device->deviceinfo.pci->revision_id);
         else
-            printf("\t\t\trevision_id\tIGNORED\n");
+            printf("       +->revision_id   IGNORED\n");
 
     } else if (device->bustype == DRM_BUS_USB) {
-        printf("\t\tusb\n");
-        printf("\t\t\tbus\t%03u\n", device->businfo.usb->bus);
-        printf("\t\t\tdev\t%03u\n", device->businfo.usb->dev);
-
-        printf("\tdeviceinfo\n");
-        printf("\t\tusb\n");
-        printf("\t\t\tvendor\t%04x\n", device->deviceinfo.usb->vendor);
-        printf("\t\t\tproduct\t%04x\n", device->deviceinfo.usb->product);
+        printf("|  +->usb\n");
+        printf("|     +->bus %03u\n", device->businfo.usb->bus);
+        printf("|     +->dev %03u\n", device->businfo.usb->dev);
+
+        printf("+->deviceinfo\n");
+        printf("   +->usb\n");
+        printf("      +->vendor  %04x\n", device->deviceinfo.usb->vendor);
+        printf("      +->product %04x\n", device->deviceinfo.usb->product);
     } else if (device->bustype == DRM_BUS_PLATFORM) {
         char **compatible = device->deviceinfo.platform->compatible;
 
-        printf("\t\tplatform\n");
-        printf("\t\t\tfullname\t%s\n", device->businfo.platform->fullname);
+        printf("|  +->platform\n");
+        printf("|     +->fullname\t%s\n", device->businfo.platform->fullname);
 
-        printf("\tdeviceinfo\n");
-        printf("\t\tplatform\n");
-        printf("\t\t\tcompatible\n");
+        printf("+->deviceinfo\n");
+        printf("   +->platform\n");
+        printf("      +->compatible\n");
 
         while (*compatible) {
-            printf("\t\t\t\t%s\n", *compatible);
+            printf("                    %s\n", *compatible);
             compatible++;
         }
     } else if (device->bustype == DRM_BUS_HOST1X) {
         char **compatible = device->deviceinfo.platform->compatible;
 
-        printf("\t\thost1x\n");
-        printf("\t\t\tfullname\t%s\n", device->businfo.host1x->fullname);
+        printf("|  +->host1x\n");
+        printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
 
-        printf("\tdeviceinfo\n");
-        printf("\t\tplatform\n");
-        printf("\t\t\tcompatible\n");
+        printf("+->deviceinfo\n");
+        printf("   +->platform\n");
+        printf("      +->compatible\n");
 
         while (*compatible) {
-            printf("\t\t\t\t%s\n", *compatible);
+            printf("                    %s\n", *compatible);
             compatible++;
         }
     } else {
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm 10/10] drmdevice: print the correct host1x information

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

While fairly close, the host1x and platform are two separate things.

Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
---
 tests/drmdevice.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c
index e9e9d7f1..97ce8ff9 100644
--- a/tests/drmdevice.c
+++ b/tests/drmdevice.c
@@ -86,13 +86,13 @@ print_device_info(drmDevicePtr device, int i, bool print_revision)
             compatible++;
         }
     } else if (device->bustype == DRM_BUS_HOST1X) {
-        char **compatible = device->deviceinfo.platform->compatible;
+        char **compatible = device->deviceinfo.host1x->compatible;
 
         printf("|  +->host1x\n");
         printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
 
         printf("+->deviceinfo\n");
-        printf("   +->platform\n");
+        printf("   +->host1x\n");
         printf("      +->compatible\n");
 
         while (*compatible) {
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 01/10] xf86drm: drmGetDevice2: error out if the fd has unknown subsys

[Robert Foss]

This series has been:
Tested-by: Robert Foss <robert.foss@collabora.com>

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently one can open() any /dev node. If it's unknown
> drmParseSubsystemType() will return an error.
> 
> Track that and bail as needed.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 87c216cf..e1bbbe99 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3814,6 +3814,8 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>           return -EINVAL;
>   
>       subsystem_type = drmParseSubsystemType(maj, min);
> +    if (subsystem_type < 0)
> +        return subsystem_type;
>   
>       local_devices = calloc(max_count, sizeof(drmDevicePtr));
>       if (local_devices == NULL)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 01/10] xf86drm: drmGetDevice2: error out if the fd has unknown subsys

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently one can open() any /dev node. If it's unknown
> drmParseSubsystemType() will return an error.
> 
> Track that and bail as needed.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 2 ++
>   1 file changed, 2 insertions(+)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 87c216cf..e1bbbe99 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3814,6 +3814,8 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>           return -EINVAL;
>   
>       subsystem_type = drmParseSubsystemType(maj, min);
> +    if (subsystem_type < 0)
> +        return subsystem_type;
>   
>       local_devices = calloc(max_count, sizeof(drmDevicePtr));
>       if (local_devices == NULL)
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 02/10] xf86drm: introduce drm_device_has_rdev() helper

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently we match the opened drmDevice fd with each drmDevice we
> process.
> 
> Move that after all the devices are processed and folded, via the
> drm_device_has_rdev(). This makes the code easier to follow and allows
> us to unify the massive process loop across drmGetDevice2 and
> drmGetDevices2. That in itself is coming with a later commit.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 34 +++++++++++++++++++++++++---------
>   1 file changed, 25 insertions(+), 9 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index e1bbbe99..cbc0a408 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3705,6 +3705,21 @@ drm_device_validate_flags(uint32_t flags)
>           return (flags & ~DRM_DEVICE_GET_PCI_REVISION);
>   }
>   
> +static bool
> +drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
> +{
> +    struct stat sbuf;
> +
> +    for (int i = 0; i < DRM_NODE_MAX; i++) {
> +        if (device->available_nodes & 1 << i) {
> +            if (stat(device->nodes[i], &sbuf) == 0 &&
> +                sbuf.st_rdev == find_rdev)
> +                return true;
> +        }
> +    }
> +    return false;
> +}
> +
>   /**
>    * Get information about the opened drm device
>    *
> @@ -3889,21 +3904,22 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>               local_devices = temp;
>           }
>   
> -        /* store target at local_devices[0] for ease to use below */
> -        if (find_rdev == sbuf.st_rdev && i) {
> -            local_devices[i] = local_devices[0];
> -            local_devices[0] = d;
> -        }
> -        else
> -            local_devices[i] = d;
> +        local_devices[i] = d;
>           i++;
>       }
>       node_count = i;
>   
>       drmFoldDuplicatedDevices(local_devices, node_count);
>   
> -    *device = local_devices[0];
> -    drmFreeDevices(&local_devices[1], node_count - 1);
> +    for (i = 0; i < node_count; i++) {
> +        if (!local_devices[i])
> +            continue;
> +
> +        if (drm_device_has_rdev(local_devices[i], find_rdev))
> +            *device = local_devices[i];
> +        else
> +            drmFreeDevice(&local_devices[i]);
> +    }
>   
>       closedir(sysdir);
>       free(local_devices);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 09/10] drmdevice: convert the tabbed output into a tree

[Eric Engestrom]

On Monday, 2018-06-25 17:40:02 +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Making the output a little bit easier to parse by human beings.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  tests/drmdevice.c | 78 +++++++++++++++++++++++------------------------
>  1 file changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/tests/drmdevice.c b/tests/drmdevice.c
> index 0d75836f..e9e9d7f1 100644
> --- a/tests/drmdevice.c
> +++ b/tests/drmdevice.c
> @@ -36,67 +36,67 @@ static void
>  print_device_info(drmDevicePtr device, int i, bool print_revision)
>  {
>      printf("device[%i]\n", i);
> -    printf("\tavailable_nodes %04x\n", device->available_nodes);
> -    printf("\tnodes\n");
> +    printf("+->available_nodes %#04x\n", device->available_nodes);
> +    printf("+->nodes\n");

Nit: I'd put a space between `>` and the text, for readability

>      for (int j = 0; j < DRM_NODE_MAX; j++)
>          if (device->available_nodes & 1 << j)
> -            printf("\t\tnodes[%d] %s\n", j, device->nodes[j]);
> +            printf("|  +->nodes[%d] %s\n", j, device->nodes[j]);
>  
> -    printf("\tbustype %04x\n", device->bustype);
> -    printf("\tbusinfo\n");
> +    printf("+->bustype %04x\n", device->bustype);
> +    printf("+->businfo\n");
>      if (device->bustype == DRM_BUS_PCI) {
> -        printf("\t\tpci\n");
> -        printf("\t\t\tdomain\t%04x\n",device->businfo.pci->domain);
> -        printf("\t\t\tbus\t%02x\n", device->businfo.pci->bus);
> -        printf("\t\t\tdev\t%02x\n", device->businfo.pci->dev);
> -        printf("\t\t\tfunc\t%1u\n", device->businfo.pci->func);
> -
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tpci\n");
> -        printf("\t\t\tvendor_id\t%04x\n", device->deviceinfo.pci->vendor_id);
> -        printf("\t\t\tdevice_id\t%04x\n", device->deviceinfo.pci->device_id);
> -        printf("\t\t\tsubvendor_id\t%04x\n", device->deviceinfo.pci->subvendor_id);
> -        printf("\t\t\tsubdevice_id\t%04x\n", device->deviceinfo.pci->subdevice_id);
> +        printf("|  +->pci\n");
> +        printf("|     +->domain %04x\n",device->businfo.pci->domain);
> +        printf("|     +->bus    %02x\n", device->businfo.pci->bus);
> +        printf("|     +->dev    %02x\n", device->businfo.pci->dev);
> +        printf("|     +->func   %1u\n", device->businfo.pci->func);
> +
> +        printf("+->deviceinfo\n");
> +        printf("    +->pci\n");
> +        printf("       +->vendor_id     %04x\n", device->deviceinfo.pci->vendor_id);
> +        printf("       +->device_id     %04x\n", device->deviceinfo.pci->device_id);
> +        printf("       +->subvendor_id  %04x\n", device->deviceinfo.pci->subvendor_id);
> +        printf("       +->subdevice_id  %04x\n", device->deviceinfo.pci->subdevice_id);
>          if (print_revision)
> -            printf("\t\t\trevision_id\t%02x\n", device->deviceinfo.pci->revision_id);
> +            printf("       +->revision_id   %02x\n", device->deviceinfo.pci->revision_id);
>          else
> -            printf("\t\t\trevision_id\tIGNORED\n");
> +            printf("       +->revision_id   IGNORED\n");
>  
>      } else if (device->bustype == DRM_BUS_USB) {
> -        printf("\t\tusb\n");
> -        printf("\t\t\tbus\t%03u\n", device->businfo.usb->bus);
> -        printf("\t\t\tdev\t%03u\n", device->businfo.usb->dev);
> -
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tusb\n");
> -        printf("\t\t\tvendor\t%04x\n", device->deviceinfo.usb->vendor);
> -        printf("\t\t\tproduct\t%04x\n", device->deviceinfo.usb->product);
> +        printf("|  +->usb\n");
> +        printf("|     +->bus %03u\n", device->businfo.usb->bus);
> +        printf("|     +->dev %03u\n", device->businfo.usb->dev);
> +
> +        printf("+->deviceinfo\n");
> +        printf("   +->usb\n");
> +        printf("      +->vendor  %04x\n", device->deviceinfo.usb->vendor);
> +        printf("      +->product %04x\n", device->deviceinfo.usb->product);
>      } else if (device->bustype == DRM_BUS_PLATFORM) {
>          char **compatible = device->deviceinfo.platform->compatible;
>  
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tfullname\t%s\n", device->businfo.platform->fullname);
> +        printf("|  +->platform\n");
> +        printf("|     +->fullname\t%s\n", device->businfo.platform->fullname);
>  
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tcompatible\n");
> +        printf("+->deviceinfo\n");
> +        printf("   +->platform\n");
> +        printf("      +->compatible\n");
>  
>          while (*compatible) {
> -            printf("\t\t\t\t%s\n", *compatible);
> +            printf("                    %s\n", *compatible);
>              compatible++;
>          }
>      } else if (device->bustype == DRM_BUS_HOST1X) {
>          char **compatible = device->deviceinfo.platform->compatible;
>  
> -        printf("\t\thost1x\n");
> -        printf("\t\t\tfullname\t%s\n", device->businfo.host1x->fullname);
> +        printf("|  +->host1x\n");
> +        printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
>  
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tcompatible\n");
> +        printf("+->deviceinfo\n");
> +        printf("   +->platform\n");
> +        printf("      +->compatible\n");
>  
>          while (*compatible) {
> -            printf("\t\t\t\t%s\n", *compatible);
> +            printf("                    %s\n", *compatible);
>              compatible++;
>          }
>      } else {
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 05/10] xf86drm: introduce a get_real_pci_path() helper

[Eric Engestrom]

On Monday, 2018-06-25 17:39:14 +0000, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Introduce a helper which gets the real sysfs path for the given pci
> device.
> 
> In other words, instead opening the /sys/dev/char/*/device symlink, we
> opt for the actual /sys/devices/pci*/*/
> 
> It folds three (nearly identical) snprintf's and paves the way of adding
> extra devices (see next patch) a piece of pie.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>  xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++-----------
>  1 file changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index d4810740..8ccd528f 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min)
>  #endif
>  }
>  
> +static char *
> +get_real_pci_path(int maj, int min)
> +{
> +    char path[PATH_MAX + 1];
> +    char *real_path = malloc(PATH_MAX);

Why not allocate this on the stack and pass it in?
It would avoid the error handling you had to add in this patch :)

> +
> +    if (!real_path)
> +        return NULL;
> +
> +    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> +    if (!realpath(path, real_path)) {
> +        free(real_path);
> +        return NULL;
> +    }
> +
> +    return real_path;
> +}
> +
>  static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
>  {
>  #ifdef __linux__
>      unsigned int domain, bus, dev, func;
> -    char path[PATH_MAX + 1], *value;
> +    char *real_path, *value;
>      int num;
>  
> -    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
>  
> -    value = sysfs_uevent_get(path, "PCI_SLOT_NAME");
> +    value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
> +    free(real_path);
>      if (!value)
>          return -ENOENT;
>  
> @@ -3114,24 +3135,32 @@ static int parse_separate_sysfs_files(int maj, int min,
>        "subsystem_vendor",
>        "subsystem_device",
>      };
> -    char path[PATH_MAX + 1];
> +    char path[PATH_MAX + 1], *real_path;
>      unsigned int data[ARRAY_SIZE(attrs)];
>      FILE *fp;
>      int ret;
>  
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
> +
>      for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {
> -        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
> -                 attrs[i]);
> +        snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
>          fp = fopen(path, "r");
> -        if (!fp)
> +        if (!fp) {
> +            free(real_path);
>              return -errno;
> +        }
>  
>          ret = fscanf(fp, "%x", &data[i]);
>          fclose(fp);
> -        if (ret != 1)
> +        if (ret != 1) {
> +            free(real_path);
>              return -errno;
> +        }
>  
>      }
> +    free(real_path);
>  
>      device->revision_id = ignore_revision ? 0xff : data[0] & 0xff;
>      device->vendor_id = data[1] & 0xffff;
> @@ -3145,19 +3174,28 @@ static int parse_separate_sysfs_files(int maj, int min,
>  static int parse_config_sysfs_file(int maj, int min,
>                                     drmPciDeviceInfoPtr device)
>  {
> -    char path[PATH_MAX + 1];
> +    char path[PATH_MAX + 1], *real_path;
>      unsigned char config[64];
>      int fd, ret;
>  
> -    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
> +
> +    snprintf(path, PATH_MAX, "%s/config", real_path);
>      fd = open(path, O_RDONLY);
> -    if (fd < 0)
> +    if (fd < 0) {
> +        free(real_path);
>          return -errno;
> +    }
>  
>      ret = read(fd, config, sizeof(config));
>      close(fd);
> -    if (ret < 0)
> +    if (ret < 0) {
> +        free(real_path);
>          return -errno;
> +    }
> +    free(real_path);
>  
>      device->vendor_id = config[0] | (config[1] << 8);
>      device->device_id = config[2] | (config[3] << 8);
> -- 
> 2.18.0
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 05/10] xf86drm: introduce a get_real_pci_path() helper

[Eric Engestrom]

On Thursday, 2018-06-28 11:21:26 +0100, Eric Engestrom wrote:
> On Monday, 2018-06-25 17:39:14 +0000, Emil Velikov wrote:
> > From: Emil Velikov <emil.velikov@collabora.com>
> > 
> > Introduce a helper which gets the real sysfs path for the given pci
> > device.
> > 
> > In other words, instead opening the /sys/dev/char/*/device symlink, we
> > opt for the actual /sys/devices/pci*/*/
> > 
> > It folds three (nearly identical) snprintf's and paves the way of adding
> > extra devices (see next patch) a piece of pie.
> > 
> > Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> > ---
> >  xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++-----------
> >  1 file changed, 50 insertions(+), 12 deletions(-)
> > 
> > diff --git a/xf86drm.c b/xf86drm.c
> > index d4810740..8ccd528f 100644
> > --- a/xf86drm.c
> > +++ b/xf86drm.c
> > @@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min)
> >  #endif
> >  }
> >  
> > +static char *
> > +get_real_pci_path(int maj, int min)
> > +{
> > +    char path[PATH_MAX + 1];
> > +    char *real_path = malloc(PATH_MAX);
> 
> Why not allocate this on the stack and pass it in?
> It would avoid the error handling you had to add in this patch :)

Sorry, I forgot to add: with that amend, the series is
Reviewed-by: Eric Engestrom <eric@engestrom.ch>

> 
> > +
> > +    if (!real_path)
> > +        return NULL;
> > +
> > +    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> > +    if (!realpath(path, real_path)) {
> > +        free(real_path);
> > +        return NULL;
> > +    }
> > +
> > +    return real_path;
> > +}
> > +
> >  static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
> >  {
> >  #ifdef __linux__
> >      unsigned int domain, bus, dev, func;
> > -    char path[PATH_MAX + 1], *value;
> > +    char *real_path, *value;
> >      int num;
> >  
> > -    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> > +    real_path = get_real_pci_path(maj, min);
> > +    if (!real_path)
> > +        return -ENOENT;
> >  
> > -    value = sysfs_uevent_get(path, "PCI_SLOT_NAME");
> > +    value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
> > +    free(real_path);
> >      if (!value)
> >          return -ENOENT;
> >  
> > @@ -3114,24 +3135,32 @@ static int parse_separate_sysfs_files(int maj, int min,
> >        "subsystem_vendor",
> >        "subsystem_device",
> >      };
> > -    char path[PATH_MAX + 1];
> > +    char path[PATH_MAX + 1], *real_path;
> >      unsigned int data[ARRAY_SIZE(attrs)];
> >      FILE *fp;
> >      int ret;
> >  
> > +    real_path = get_real_pci_path(maj, min);
> > +    if (!real_path)
> > +        return -ENOENT;
> > +
> >      for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {
> > -        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
> > -                 attrs[i]);
> > +        snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
> >          fp = fopen(path, "r");
> > -        if (!fp)
> > +        if (!fp) {
> > +            free(real_path);
> >              return -errno;
> > +        }
> >  
> >          ret = fscanf(fp, "%x", &data[i]);
> >          fclose(fp);
> > -        if (ret != 1)
> > +        if (ret != 1) {
> > +            free(real_path);
> >              return -errno;
> > +        }
> >  
> >      }
> > +    free(real_path);
> >  
> >      device->revision_id = ignore_revision ? 0xff : data[0] & 0xff;
> >      device->vendor_id = data[1] & 0xffff;
> > @@ -3145,19 +3174,28 @@ static int parse_separate_sysfs_files(int maj, int min,
> >  static int parse_config_sysfs_file(int maj, int min,
> >                                     drmPciDeviceInfoPtr device)
> >  {
> > -    char path[PATH_MAX + 1];
> > +    char path[PATH_MAX + 1], *real_path;
> >      unsigned char config[64];
> >      int fd, ret;
> >  
> > -    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);
> > +    real_path = get_real_pci_path(maj, min);
> > +    if (!real_path)
> > +        return -ENOENT;
> > +
> > +    snprintf(path, PATH_MAX, "%s/config", real_path);
> >      fd = open(path, O_RDONLY);
> > -    if (fd < 0)
> > +    if (fd < 0) {
> > +        free(real_path);
> >          return -errno;
> > +    }
> >  
> >      ret = read(fd, config, sizeof(config));
> >      close(fd);
> > -    if (ret < 0)
> > +    if (ret < 0) {
> > +        free(real_path);
> >          return -errno;
> > +    }
> > +    free(real_path);
> >  
> >      device->vendor_id = config[0] | (config[1] << 8);
> >      device->device_id = config[2] | (config[3] << 8);
> > -- 
> > 2.18.0
> > 
> > _______________________________________________
> > dri-devel mailing list
> > dri-devel@lists.freedesktop.org
> > https://lists.freedesktop.org/mailman/listinfo/dri-devel
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 03/10] xf86drm: Fold drmDevice processing into process_device() helper

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Don't the duplicate (nearly) identical code across the two call sites.
> It improves legibility and the diff stat seems nice.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 159 ++++++++++++++++++------------------------------------
>   1 file changed, 51 insertions(+), 108 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index cbc0a408..114cf855 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3676,6 +3676,52 @@ free_device:
>       return ret;
>   }
>   
> +static int
> +process_device(drmDevicePtr *device, const char *d_name,
> +               int req_subsystem_type,
> +               bool fetch_deviceinfo, uint32_t flags)
> +{
> +    struct stat sbuf;
> +    char node[PATH_MAX + 1];
> +    int node_type, subsystem_type;
> +    unsigned int maj, min;
> +
> +    node_type = drmGetNodeType(d_name);
> +    if (node_type < 0)
> +        return -1;
> +
> +    snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, d_name);
> +    if (stat(node, &sbuf))
> +        return -1;
> +
> +    maj = major(sbuf.st_rdev);
> +    min = minor(sbuf.st_rdev);
> +
> +    if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
> +        return -1;
> +
> +    subsystem_type = drmParseSubsystemType(maj, min);
> +    if (req_subsystem_type != -1 && req_subsystem_type != subsystem_type)
> +        return -1;
> +
> +    switch (subsystem_type) {
> +    case DRM_BUS_PCI:
> +        return drmProcessPciDevice(device, node, node_type, maj, min,
> +                                   fetch_deviceinfo, flags);
> +    case DRM_BUS_USB:
> +        return drmProcessUsbDevice(device, node, node_type, maj, min,
> +                                   fetch_deviceinfo, flags);
> +    case DRM_BUS_PLATFORM:
> +        return drmProcessPlatformDevice(device, node, node_type, maj, min,
> +                                        fetch_deviceinfo, flags);
> +    case DRM_BUS_HOST1X:
> +        return drmProcessHost1xDevice(device, node, node_type, maj, min,
> +                                      fetch_deviceinfo, flags);
> +    default:
> +        return -1;
> +   }
> +}
> +
>   /* Consider devices located on the same bus as duplicate and fold the respective
>    * entries into a single one.
>    *
> @@ -3805,8 +3851,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       DIR *sysdir;
>       struct dirent *dent;
>       struct stat sbuf;
> -    char node[PATH_MAX + 1];
> -    int node_type, subsystem_type;
> +    int subsystem_type;
>       int maj, min;
>       int ret, i, node_count;
>       int max_count = 16;
> @@ -3844,55 +3889,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> -        node_type = drmGetNodeType(dent->d_name);
> -        if (node_type < 0)
> -            continue;
> -
> -        snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
> -        if (stat(node, &sbuf))
> -            continue;
> -
> -        maj = major(sbuf.st_rdev);
> -        min = minor(sbuf.st_rdev);
> -
> -        if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
> -            continue;
> -
> -        if (drmParseSubsystemType(maj, min) != subsystem_type)
> -            continue;
> -
> -        switch (subsystem_type) {
> -        case DRM_BUS_PCI:
> -            ret = drmProcessPciDevice(&d, node, node_type, maj, min, true, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_USB:
> -            ret = drmProcessUsbDevice(&d, node, node_type, maj, min, true, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_PLATFORM:
> -            ret = drmProcessPlatformDevice(&d, node, node_type, maj, min, true, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_HOST1X:
> -            ret = drmProcessHost1xDevice(&d, node, node_type, maj, min, true, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        default:
> +        ret = process_device(&d, dent->d_name, subsystem_type, true, flags);
> +        if (ret)
>               continue;
> -        }
>   
>           if (i >= max_count) {
>               drmDevicePtr *temp;
> @@ -3973,10 +3972,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>       drmDevicePtr device;
>       DIR *sysdir;
>       struct dirent *dent;
> -    struct stat sbuf;
> -    char node[PATH_MAX + 1];
> -    int node_type, subsystem_type;
> -    int maj, min;
>       int ret, i, node_count, device_count;
>       int max_count = 16;
>   
> @@ -3995,61 +3990,9 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> -        node_type = drmGetNodeType(dent->d_name);
> -        if (node_type < 0)
> -            continue;
> -
> -        snprintf(node, PATH_MAX, "%s/%s", DRM_DIR_NAME, dent->d_name);
> -        if (stat(node, &sbuf))
> -            continue;
> -
> -        maj = major(sbuf.st_rdev);
> -        min = minor(sbuf.st_rdev);
> -
> -        if (maj != DRM_MAJOR || !S_ISCHR(sbuf.st_mode))
> -            continue;
> -
> -        subsystem_type = drmParseSubsystemType(maj, min);
> -
> -        if (subsystem_type < 0)
> -            continue;
> -
> -        switch (subsystem_type) {
> -        case DRM_BUS_PCI:
> -            ret = drmProcessPciDevice(&device, node, node_type,
> -                                      maj, min, devices != NULL, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_USB:
> -            ret = drmProcessUsbDevice(&device, node, node_type, maj, min,
> -                                      devices != NULL, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_PLATFORM:
> -            ret = drmProcessPlatformDevice(&device, node, node_type, maj, min,
> -                                           devices != NULL, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        case DRM_BUS_HOST1X:
> -            ret = drmProcessHost1xDevice(&device, node, node_type, maj, min,
> -                                         devices != NULL, flags);
> -            if (ret)
> -                continue;
> -
> -            break;
> -
> -        default:
> +        ret = process_device(&device, dent->d_name, -1, devices != NULL, flags);
> +        if (ret)
>               continue;
> -        }
>   
>           if (i >= max_count) {
>               drmDevicePtr *temp;
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Robert Foss]

Hey Emil,

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently we dynamically allocate 16 pointers and reallocate more as
> needed.
> 
> Instead, allocate the maximum number (256) on stack - the number is
> small enough and is unlikely to change in the foreseeable future.
> 
> This allows us to simplify the error handling and even shed a few bytes
> off the final binary.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 64 ++++++-------------------------------------------------
>   1 file changed, 6 insertions(+), 58 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 114cf855..d4810740 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>   
>       return 0;
>   #else
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[256];

This number is seen later on in this patch, maybe it should be broken out into a
define, since it's reused later on too at [1].

>       drmDevicePtr d;
>       DIR *sysdir;
>       struct dirent *dent;
> @@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       int subsystem_type;
>       int maj, min;
>       int ret, i, node_count;
> -    int max_count = 16;
>       dev_t find_rdev;
>   
>       if (drm_device_validate_flags(flags))
> @@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       if (subsystem_type < 0)
>           return subsystem_type;
>   
> -    local_devices = calloc(max_count, sizeof(drmDevicePtr));
> -    if (local_devices == NULL)
> -        return -ENOMEM;
> -
>       sysdir = opendir(DRM_DIR_NAME);
> -    if (!sysdir) {
> -        ret = -errno;
> -        goto free_locals;
> -    }
> +    if (!sysdir)
> +        return -errno;
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> @@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>           if (ret)
>               continue;
>   
> -        if (i >= max_count) {

Is this check really not needded what exactly is it that defines 256 as the maximum?

 From what I understand readdir(sysdir) is the call that defines how many 
devices will be looked through, and as far as I understand it can return an 
arbitrary number of files.

> -            drmDevicePtr *temp;
> -
> -            max_count += 16;
> -            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
> -            if (!temp)
> -                goto free_devices;
> -            local_devices = temp;
> -        }
> -
>           local_devices[i] = d;
>           i++;
>       }
> @@ -3921,18 +3904,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       }
>   
>       closedir(sysdir);
> -    free(local_devices);
>       if (*device == NULL)
>           return -ENODEV;
>       return 0;
> -
> -free_devices:
> -    drmFreeDevices(local_devices, i);
> -    closedir(sysdir);
> -
> -free_locals:
> -    free(local_devices);
> -    return ret;
>   #endif
>   }
>   
> @@ -3968,25 +3942,18 @@ int drmGetDevice(int fd, drmDevicePtr *device)
>    */
>   int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>   {
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[256];'

[1]

>       drmDevicePtr device;
>       DIR *sysdir;
>       struct dirent *dent;
>       int ret, i, node_count, device_count;
> -    int max_count = 16;
>   
>       if (drm_device_validate_flags(flags))
>           return -EINVAL;
>   
> -    local_devices = calloc(max_count, sizeof(drmDevicePtr));
> -    if (local_devices == NULL)
> -        return -ENOMEM;
> -
>       sysdir = opendir(DRM_DIR_NAME);
> -    if (!sysdir) {
> -        ret = -errno;
> -        goto free_locals;
> -    }
> +    if (!sysdir)
> +        return -errno;
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> @@ -3994,16 +3961,6 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>           if (ret)
>               continue;
>   
> -        if (i >= max_count) {

This out of bounds check should be dealt with too, if the above bounds check 
should be.

> -            drmDevicePtr *temp;
> -
> -            max_count += 16;
> -            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
> -            if (!temp)
> -                goto free_devices;
> -            local_devices = temp;
> -        }
> -
>           local_devices[i] = device;
>           i++;
>       }
> @@ -4025,16 +3982,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>       }
>   
>       closedir(sysdir);
> -    free(local_devices);
>       return device_count;
> -
> -free_devices:
> -    drmFreeDevices(local_devices, i);
> -    closedir(sysdir);
> -
> -free_locals:
> -    free(local_devices);
> -    return ret;
>   }
>   
>   /**
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 05/10] xf86drm: introduce a get_real_pci_path() helper

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Introduce a helper which gets the real sysfs path for the given pci
> device.
> 
> In other words, instead opening the /sys/dev/char/*/device symlink, we
> opt for the actual /sys/devices/pci*/*/
> 
> It folds three (nearly identical) snprintf's and paves the way of adding
> extra devices (see next patch) a piece of pie.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 62 ++++++++++++++++++++++++++++++++++++++++++++-----------
>   1 file changed, 50 insertions(+), 12 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index d4810740..8ccd528f 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min)
>   #endif
>   }
>   
> +static char *
> +get_real_pci_path(int maj, int min)
> +{
> +    char path[PATH_MAX + 1];
> +    char *real_path = malloc(PATH_MAX);
> +
> +    if (!real_path)
> +        return NULL;
> +
> +    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> +    if (!realpath(path, real_path)) {
> +        free(real_path);
> +        return NULL;
> +    }
> +
> +    return real_path;
> +}
> +
>   static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
>   {
>   #ifdef __linux__
>       unsigned int domain, bus, dev, func;
> -    char path[PATH_MAX + 1], *value;
> +    char *real_path, *value;
>       int num;
>   
> -    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
>   
> -    value = sysfs_uevent_get(path, "PCI_SLOT_NAME");
> +    value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
> +    free(real_path);
>       if (!value)
>           return -ENOENT;
>   
> @@ -3114,24 +3135,32 @@ static int parse_separate_sysfs_files(int maj, int min,
>         "subsystem_vendor",
>         "subsystem_device",
>       };
> -    char path[PATH_MAX + 1];
> +    char path[PATH_MAX + 1], *real_path;
>       unsigned int data[ARRAY_SIZE(attrs)];
>       FILE *fp;
>       int ret;
>   
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
> +
>       for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {
> -        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
> -                 attrs[i]);
> +        snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
>           fp = fopen(path, "r");
> -        if (!fp)
> +        if (!fp) {
> +            free(real_path);
>               return -errno;
> +        }
>   
>           ret = fscanf(fp, "%x", &data[i]);
>           fclose(fp);
> -        if (ret != 1)
> +        if (ret != 1) {
> +            free(real_path);
>               return -errno;
> +        }
>   
>       }
> +    free(real_path);
>   
>       device->revision_id = ignore_revision ? 0xff : data[0] & 0xff;
>       device->vendor_id = data[1] & 0xffff;
> @@ -3145,19 +3174,28 @@ static int parse_separate_sysfs_files(int maj, int min,
>   static int parse_config_sysfs_file(int maj, int min,
>                                      drmPciDeviceInfoPtr device)
>   {
> -    char path[PATH_MAX + 1];
> +    char path[PATH_MAX + 1], *real_path;
>       unsigned char config[64];
>       int fd, ret;
>   
> -    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);
> +    real_path = get_real_pci_path(maj, min);
> +    if (!real_path)
> +        return -ENOENT;
> +
> +    snprintf(path, PATH_MAX, "%s/config", real_path);
>       fd = open(path, O_RDONLY);
> -    if (fd < 0)
> +    if (fd < 0) {
> +        free(real_path);
>           return -errno;
> +    }
>   
>       ret = read(fd, config, sizeof(config));
>       close(fd);
> -    if (ret < 0)
> +    if (ret < 0) {
> +        free(real_path);
>           return -errno;
> +    }
> +    free(real_path);
>   
>       device->vendor_id = config[0] | (config[1] << 8);
>       device->device_id = config[2] | (config[3] << 8);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 06/10] xf86drm: Add drmDevice support for virtio_gpu

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> The GPU almost exclusively lives on the PCI bus, so we expose it as a
> normal PCI one.
> 
> This allows any existing drmDevice users to work without any changes.
> 
> One could wonder why a separate typeset is not introduced, alike say
> host1x. Unlike host1x the PCI/platform distinction for virtio provides
> no extra information. Plus needed we can add the separate set at a later
> stage.
> 
> Here are a couple of 'features' that virtio seems to be missing:
>   - provides extra information on top the plaform devices
>   - supports a range of GPU devices
>   - is considered hardware description (DT)
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   xf86drm.c | 13 ++++++++++++-
>   1 file changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 8ccd528f..b847ea26 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -2954,6 +2954,9 @@ sysfs_uevent_get(const char *path, const char *fmt, ...)
>   }
>   #endif
>   
> +/* Little white lie to avoid major rework of the existing code */
> +#define DRM_BUS_VIRTIO 0x10
> +
>   static int drmParseSubsystemType(int maj, int min)
>   {
>   #ifdef __linux__
> @@ -2983,6 +2986,9 @@ static int drmParseSubsystemType(int maj, int min)
>       if (strncmp(name, "/host1x", 7) == 0)
>           return DRM_BUS_HOST1X;
>   
> +    if (strncmp(name, "/virtio", 7) == 0)
> +        return DRM_BUS_VIRTIO;
> +
>       return -EINVAL;
>   #elif defined(__OpenBSD__)
>       return DRM_BUS_PCI;
> @@ -2996,7 +3002,7 @@ static char *
>   get_real_pci_path(int maj, int min)
>   {
>       char path[PATH_MAX + 1];
> -    char *real_path = malloc(PATH_MAX);
> +    char *term, *real_path = malloc(PATH_MAX);
>   
>       if (!real_path)
>           return NULL;
> @@ -3007,6 +3013,10 @@ get_real_pci_path(int maj, int min)
>           return NULL;
>       }
>   
> +    term = strrchr(real_path, '/');
> +    if (term && strncmp(term, "/virtio", 7) == 0)
> +        *term = 0;
> +
>       return real_path;
>   }
>   
> @@ -3744,6 +3754,7 @@ process_device(drmDevicePtr *device, const char *d_name,
>   
>       switch (subsystem_type) {
>       case DRM_BUS_PCI:
> +    case DRM_BUS_VIRTIO:
>           return drmProcessPciDevice(device, node, node_type, maj, min,
>                                      fetch_deviceinfo, flags);
>       case DRM_BUS_USB:
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 07/10] tests/drmdevices: install alongside other utilities

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> It's mildly useful program, to ship it when the user wants the "tests"
> installed. Obviously the "tests" in the name is a misnomer.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   tests/Makefile.am | 9 +++++++--
>   1 file changed, 7 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/Makefile.am b/tests/Makefile.am
> index 0355a925..b72c24f9 100644
> --- a/tests/Makefile.am
> +++ b/tests/Makefile.am
> @@ -43,5 +43,10 @@ TESTS = \
>   	random
>   
>   check_PROGRAMS = \
> -	$(TESTS) \
> -	drmdevice
> +	$(TESTS)
> +
> +if HAVE_INSTALL_TESTS
> +bin_PROGRAMS = drmdevice
> +else
> +check_PROGRAMS += drmdevice
> +endif
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 08/10] tests/drmdevice: add a couple of printf headers

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Add a few printf statements, which should make the output easier to
> parse.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   tests/drmdevice.c | 7 ++++++-
>   1 file changed, 6 insertions(+), 1 deletion(-)
> 
> diff --git a/tests/drmdevice.c b/tests/drmdevice.c
> index 9dd5098a..0d75836f 100644
> --- a/tests/drmdevice.c
> +++ b/tests/drmdevice.c
> @@ -112,12 +112,15 @@ main(void)
>       drmDevicePtr device;
>       int fd, ret, max_devices;
>   
> +    printf("--- Checking the number of DRM device available ---\n");
>       max_devices = drmGetDevices2(0, NULL, 0);
>   
>       if (max_devices <= 0) {
>           printf("drmGetDevices2() has returned %d\n", max_devices);
>           return -1;
>       }
> +    printf("--- Devices reported %d ---\n", max_devices);
> +
>   
>       devices = calloc(max_devices, sizeof(drmDevicePtr));
>       if (devices == NULL) {
> @@ -125,6 +128,7 @@ main(void)
>           return -1;
>       }
>   
> +    printf("--- Retrieving devices information (PCI device revision is ignored) ---\n");
>       ret = drmGetDevices2(0, devices, max_devices);
>       if (ret < 0) {
>           printf("drmGetDevices2() returned an error %d\n", ret);
> @@ -137,13 +141,14 @@ main(void)
>   
>           for (int j = 0; j < DRM_NODE_MAX; j++) {
>               if (devices[i]->available_nodes & 1 << j) {
> -                printf("Opening device %d node %s\n", i, devices[i]->nodes[j]);
> +                printf("--- Opening device node %s ---\n", devices[i]->nodes[j]);
>                   fd = open(devices[i]->nodes[j], O_RDONLY | O_CLOEXEC, 0);
>                   if (fd < 0) {
>                       printf("Failed - %s (%d)\n", strerror(errno), errno);
>                       continue;
>                   }
>   
> +                printf("--- Retrieving device info, for node %s ---\n", devices[i]->nodes[j]);
>                   if (drmGetDevice2(fd, DRM_DEVICE_GET_PCI_REVISION, &device) == 0) {
>                       print_device_info(device, i, true);
>                       drmFreeDevice(&device);
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 09/10] drmdevice: convert the tabbed output into a tree

[Robert Foss]

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Making the output a little bit easier to parse by human beings.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   tests/drmdevice.c | 78 +++++++++++++++++++++++------------------------
>   1 file changed, 39 insertions(+), 39 deletions(-)
> 
> diff --git a/tests/drmdevice.c b/tests/drmdevice.c
> index 0d75836f..e9e9d7f1 100644
> --- a/tests/drmdevice.c
> +++ b/tests/drmdevice.c
> @@ -36,67 +36,67 @@ static void
>   print_device_info(drmDevicePtr device, int i, bool print_revision)
>   {
>       printf("device[%i]\n", i);
> -    printf("\tavailable_nodes %04x\n", device->available_nodes);
> -    printf("\tnodes\n");
> +    printf("+->available_nodes %#04x\n", device->available_nodes);
> +    printf("+->nodes\n");
>       for (int j = 0; j < DRM_NODE_MAX; j++)
>           if (device->available_nodes & 1 << j)
> -            printf("\t\tnodes[%d] %s\n", j, device->nodes[j]);
> +            printf("|  +->nodes[%d] %s\n", j, device->nodes[j]);
>   
> -    printf("\tbustype %04x\n", device->bustype);
> -    printf("\tbusinfo\n");
> +    printf("+->bustype %04x\n", device->bustype);
> +    printf("+->businfo\n");
>       if (device->bustype == DRM_BUS_PCI) {
> -        printf("\t\tpci\n");
> -        printf("\t\t\tdomain\t%04x\n",device->businfo.pci->domain);
> -        printf("\t\t\tbus\t%02x\n", device->businfo.pci->bus);
> -        printf("\t\t\tdev\t%02x\n", device->businfo.pci->dev);
> -        printf("\t\t\tfunc\t%1u\n", device->businfo.pci->func);
> -
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tpci\n");
> -        printf("\t\t\tvendor_id\t%04x\n", device->deviceinfo.pci->vendor_id);
> -        printf("\t\t\tdevice_id\t%04x\n", device->deviceinfo.pci->device_id);
> -        printf("\t\t\tsubvendor_id\t%04x\n", device->deviceinfo.pci->subvendor_id);
> -        printf("\t\t\tsubdevice_id\t%04x\n", device->deviceinfo.pci->subdevice_id);
> +        printf("|  +->pci\n");
> +        printf("|     +->domain %04x\n",device->businfo.pci->domain);
> +        printf("|     +->bus    %02x\n", device->businfo.pci->bus);
> +        printf("|     +->dev    %02x\n", device->businfo.pci->dev);
> +        printf("|     +->func   %1u\n", device->businfo.pci->func);
> +
> +        printf("+->deviceinfo\n");
> +        printf("    +->pci\n");
> +        printf("       +->vendor_id     %04x\n", device->deviceinfo.pci->vendor_id);
> +        printf("       +->device_id     %04x\n", device->deviceinfo.pci->device_id);
> +        printf("       +->subvendor_id  %04x\n", device->deviceinfo.pci->subvendor_id);
> +        printf("       +->subdevice_id  %04x\n", device->deviceinfo.pci->subdevice_id);
>           if (print_revision)
> -            printf("\t\t\trevision_id\t%02x\n", device->deviceinfo.pci->revision_id);
> +            printf("       +->revision_id   %02x\n", device->deviceinfo.pci->revision_id);
>           else
> -            printf("\t\t\trevision_id\tIGNORED\n");
> +            printf("       +->revision_id   IGNORED\n");
>   
>       } else if (device->bustype == DRM_BUS_USB) {
> -        printf("\t\tusb\n");
> -        printf("\t\t\tbus\t%03u\n", device->businfo.usb->bus);
> -        printf("\t\t\tdev\t%03u\n", device->businfo.usb->dev);
> -
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tusb\n");
> -        printf("\t\t\tvendor\t%04x\n", device->deviceinfo.usb->vendor);
> -        printf("\t\t\tproduct\t%04x\n", device->deviceinfo.usb->product);
> +        printf("|  +->usb\n");
> +        printf("|     +->bus %03u\n", device->businfo.usb->bus);
> +        printf("|     +->dev %03u\n", device->businfo.usb->dev);
> +
> +        printf("+->deviceinfo\n");
> +        printf("   +->usb\n");
> +        printf("      +->vendor  %04x\n", device->deviceinfo.usb->vendor);
> +        printf("      +->product %04x\n", device->deviceinfo.usb->product);
>       } else if (device->bustype == DRM_BUS_PLATFORM) {
>           char **compatible = device->deviceinfo.platform->compatible;
>   
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tfullname\t%s\n", device->businfo.platform->fullname);
> +        printf("|  +->platform\n");
> +        printf("|     +->fullname\t%s\n", device->businfo.platform->fullname);
>   
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tcompatible\n");
> +        printf("+->deviceinfo\n");
> +        printf("   +->platform\n");
> +        printf("      +->compatible\n");
>   
>           while (*compatible) {
> -            printf("\t\t\t\t%s\n", *compatible);
> +            printf("                    %s\n", *compatible);
>               compatible++;
>           }
>       } else if (device->bustype == DRM_BUS_HOST1X) {
>           char **compatible = device->deviceinfo.platform->compatible;
>   
> -        printf("\t\thost1x\n");
> -        printf("\t\t\tfullname\t%s\n", device->businfo.host1x->fullname);
> +        printf("|  +->host1x\n");
> +        printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
>   
> -        printf("\tdeviceinfo\n");
> -        printf("\t\tplatform\n");
> -        printf("\t\t\tcompatible\n");
> +        printf("+->deviceinfo\n");
> +        printf("   +->platform\n");
> +        printf("      +->compatible\n");
>   
>           while (*compatible) {
> -            printf("\t\t\t\t%s\n", *compatible);
> +            printf("                    %s\n", *compatible);
>               compatible++;
>           }
>       } else {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 10/10] drmdevice: print the correct host1x information

[Robert Foss]

Nice catch!

Feel free to add my r-b to this patch.

On 2018-06-25 19:36, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> While fairly close, the host1x and platform are two separate things.
> 
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> ---
>   tests/drmdevice.c | 4 ++--
>   1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/tests/drmdevice.c b/tests/drmdevice.c
> index e9e9d7f1..97ce8ff9 100644
> --- a/tests/drmdevice.c
> +++ b/tests/drmdevice.c
> @@ -86,13 +86,13 @@ print_device_info(drmDevicePtr device, int i, bool print_revision)
>               compatible++;
>           }
>       } else if (device->bustype == DRM_BUS_HOST1X) {
> -        char **compatible = device->deviceinfo.platform->compatible;
> +        char **compatible = device->deviceinfo.host1x->compatible;
>   
>           printf("|  +->host1x\n");
>           printf("|     +->fullname\t%s\n", device->businfo.host1x->fullname);
>   
>           printf("+->deviceinfo\n");
> -        printf("   +->platform\n");
> +        printf("   +->host1x\n");
>           printf("      +->compatible\n");
>   
>           while (*compatible) {
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 05/10] xf86drm: introduce a get_real_pci_path() helper

[Emil Velikov]

Hi Eric,

On 28 June 2018 at 11:21, Eric Engestrom <eric@engestrom.ch> wrote:

>> @@ -2992,16 +2992,37 @@ static int drmParseSubsystemType(int maj, int min)
>>  #endif
>>  }
>>
>> +static char *
>> +get_real_pci_path(int maj, int min)
>> +{
>> +    char path[PATH_MAX + 1];
>> +    char *real_path = malloc(PATH_MAX);
>
> Why not allocate this on the stack and pass it in?
> It would avoid the error handling you had to add in this patch :)
>
As always, thanks for having a look.

I was mostly hesitant since some callers already have a char foo[PATH_MAX +1].
Thinking about it - the default stack size should be enough to
accommodate an extra ~4k bytes.

Will fix.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 09/10] drmdevice: convert the tabbed output into a tree

[Emil Velikov]

On 28 June 2018 at 11:19, Eric Engestrom <eric@engestrom.ch> wrote:
> On Monday, 2018-06-25 17:40:02 +0000, Emil Velikov wrote:
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Making the output a little bit easier to parse by human beings.
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>>  tests/drmdevice.c | 78 +++++++++++++++++++++++------------------------
>>  1 file changed, 39 insertions(+), 39 deletions(-)
>>
>> diff --git a/tests/drmdevice.c b/tests/drmdevice.c
>> index 0d75836f..e9e9d7f1 100644
>> --- a/tests/drmdevice.c
>> +++ b/tests/drmdevice.c
>> @@ -36,67 +36,67 @@ static void
>>  print_device_info(drmDevicePtr device, int i, bool print_revision)
>>  {
>>      printf("device[%i]\n", i);
>> -    printf("\tavailable_nodes %04x\n", device->available_nodes);
>> -    printf("\tnodes\n");
>> +    printf("+->available_nodes %#04x\n", device->available_nodes);
>> +    printf("+->nodes\n");
>
> Nit: I'd put a space between `>` and the text, for readability
>
Ack, will do.

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Emil Velikov]

Hi Rob,

On 28 June 2018 at 13:52, Robert Foss <robert.foss@collabora.com> wrote:
> Hey Emil,
>
> On 2018-06-25 19:36, Emil Velikov wrote:
>>
>> From: Emil Velikov <emil.velikov@collabora.com>
>>
>> Currently we dynamically allocate 16 pointers and reallocate more as
>> needed.
>>
>> Instead, allocate the maximum number (256) on stack - the number is
>> small enough and is unlikely to change in the foreseeable future.
>>
>> This allows us to simplify the error handling and even shed a few bytes
>> off the final binary.
>>
>> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
>> ---
>>   xf86drm.c | 64 ++++++-------------------------------------------------
>>   1 file changed, 6 insertions(+), 58 deletions(-)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 114cf855..d4810740 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -3846,7 +3846,7 @@ int drmGetDevice2(int fd, uint32_t flags,
>> drmDevicePtr *device)
>>         return 0;
>>   #else
>> -    drmDevicePtr *local_devices;
>> +    drmDevicePtr local_devices[256];
>
>
> This number is seen later on in this patch, maybe it should be broken out
> into a
> define, since it's reused later on too at [1].
>
Sure thing. The lame MAX_DRM_NODES comes to mind, so alternatives are welcome.

>
>>       drmDevicePtr d;
>>       DIR *sysdir;
>>       struct dirent *dent;
>> @@ -3854,7 +3854,6 @@ int drmGetDevice2(int fd, uint32_t flags,
>> drmDevicePtr *device)
>>       int subsystem_type;
>>       int maj, min;
>>       int ret, i, node_count;
>> -    int max_count = 16;
>>       dev_t find_rdev;
>>         if (drm_device_validate_flags(flags))
>> @@ -3877,15 +3876,9 @@ int drmGetDevice2(int fd, uint32_t flags,
>> drmDevicePtr *device)
>>       if (subsystem_type < 0)
>>           return subsystem_type;
>>   -    local_devices = calloc(max_count, sizeof(drmDevicePtr));
>> -    if (local_devices == NULL)
>> -        return -ENOMEM;
>> -
>>       sysdir = opendir(DRM_DIR_NAME);
>> -    if (!sysdir) {
>> -        ret = -errno;
>> -        goto free_locals;
>> -    }
>> +    if (!sysdir)
>> +        return -errno;
>>         i = 0;
>>       while ((dent = readdir(sysdir))) {
>> @@ -3893,16 +3886,6 @@ int drmGetDevice2(int fd, uint32_t flags,
>> drmDevicePtr *device)
>>           if (ret)
>>               continue;
>>   -        if (i >= max_count) {
>
>
> Is this check really not needded what exactly is it that defines 256 as the
> maximum?
>
> From what I understand readdir(sysdir) is the call that defines how many
> devices will be looked through, and as far as I understand it can return an
> arbitrary number of files.
>
The kernel drm core has a number of places that assume maximum of 3x64
devices nodes.
That's 64 for each of primary, control and render nodes. I've rounded
it up to 256 for simplicity.

I assume we'll be able to see if anyone changes the kernel, although
bailing out just in case is a good idea.
The is one exciting error message - let me know if anything else comes to mind.

if (i >= MAX_DRM_NODES) {
    fprintf(stderr, "More than %d drm nodes detected. Please report a
bug - that should not happen.\nSkipping extra nodes\n",
MAX_DRM_NODES);
    break;
}

I'll send out v2 for the patches that need work some time
tomorrow/over the weekend..

Thanks
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm v2 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Currently we dynamically allocate 16 pointers and reallocate more as
needed.

Instead, allocate the maximum number (256) on stack - the number is
small enough and is unlikely to change in the foreseeable future.

This allows us to simplify the error handling and even shed a few bytes
off the final binary.

v2:
 - add a define & description behind the magic 256 (Rob)
 - report error to strerr and skip when over 256 device nodes (Rob)

Cc: Robert Foss <robert.foss@collabora.com>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
---
 xf86drm.c | 79 ++++++++++++++++---------------------------------------
 1 file changed, 23 insertions(+), 56 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 114cf855..02da3e1f 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3766,6 +3766,13 @@ drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
     return false;
 }
 
+/*
+ * The kernel drm core has a number of places that assume maximum of
+ * 3x64 devices nodes. That's 64 for each of primary, control and
+ * render nodes. Rounded it up to 256 for simplicity.
+ */
+#define MAX_DRM_NODES 256
+
 /**
  * Get information about the opened drm device
  *
@@ -3846,7 +3853,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 
     return 0;
 #else
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[MAX_DRM_NODES];
     drmDevicePtr d;
     DIR *sysdir;
     struct dirent *dent;
@@ -3854,7 +3861,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     int subsystem_type;
     int maj, min;
     int ret, i, node_count;
-    int max_count = 16;
     dev_t find_rdev;
 
     if (drm_device_validate_flags(flags))
@@ -3877,15 +3883,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     if (subsystem_type < 0)
         return subsystem_type;
 
-    local_devices = calloc(max_count, sizeof(drmDevicePtr));
-    if (local_devices == NULL)
-        return -ENOMEM;
-
     sysdir = opendir(DRM_DIR_NAME);
-    if (!sysdir) {
-        ret = -errno;
-        goto free_locals;
-    }
+    if (!sysdir)
+        return -errno;
 
     i = 0;
     while ((dent = readdir(sysdir))) {
@@ -3893,16 +3893,12 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
         if (ret)
             continue;
 
-        if (i >= max_count) {
-            drmDevicePtr *temp;
-
-            max_count += 16;
-            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
-            if (!temp)
-                goto free_devices;
-            local_devices = temp;
+        if (i >= MAX_DRM_NODES) {
+            fprintf(stderr, "More than %d drm nodes detected. "
+                    "Please report a bug - that should not happen.\n"
+                    "Skipping extra nodes\n", MAX_DRM_NODES);
+            break;
         }
-
         local_devices[i] = d;
         i++;
     }
@@ -3921,18 +3917,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
     }
 
     closedir(sysdir);
-    free(local_devices);
     if (*device == NULL)
         return -ENODEV;
     return 0;
-
-free_devices:
-    drmFreeDevices(local_devices, i);
-    closedir(sysdir);
-
-free_locals:
-    free(local_devices);
-    return ret;
 #endif
 }
 
@@ -3968,25 +3955,18 @@ int drmGetDevice(int fd, drmDevicePtr *device)
  */
 int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 {
-    drmDevicePtr *local_devices;
+    drmDevicePtr local_devices[MAX_DRM_NODES];
     drmDevicePtr device;
     DIR *sysdir;
     struct dirent *dent;
     int ret, i, node_count, device_count;
-    int max_count = 16;
 
     if (drm_device_validate_flags(flags))
         return -EINVAL;
 
-    local_devices = calloc(max_count, sizeof(drmDevicePtr));
-    if (local_devices == NULL)
-        return -ENOMEM;
-
     sysdir = opendir(DRM_DIR_NAME);
-    if (!sysdir) {
-        ret = -errno;
-        goto free_locals;
-    }
+    if (!sysdir)
+        return -errno;
 
     i = 0;
     while ((dent = readdir(sysdir))) {
@@ -3994,16 +3974,12 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
         if (ret)
             continue;
 
-        if (i >= max_count) {
-            drmDevicePtr *temp;
-
-            max_count += 16;
-            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
-            if (!temp)
-                goto free_devices;
-            local_devices = temp;
+        if (i >= MAX_DRM_NODES) {
+            fprintf(stderr, "More than %d drm nodes detected. "
+                    "Please report a bug - that should not happen.\n"
+                    "Skipping extra nodes\n", MAX_DRM_NODES);
+            break;
         }
-
         local_devices[i] = device;
         i++;
     }
@@ -4025,16 +4001,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
     }
 
     closedir(sysdir);
-    free(local_devices);
     return device_count;
-
-free_devices:
-    drmFreeDevices(local_devices, i);
-    closedir(sysdir);
-
-free_locals:
-    free(local_devices);
-    return ret;
 }
 
 /**
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm v2 05/10] xf86drm: introduce a get_real_pci_path() helper

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Introduce a helper which gets the real sysfs path for the given pci
device.

In other words, instead opening the /sys/dev/char/*/device symlink, we
opt for the actual /sys/devices/pci*/*/

It folds three (nearly identical) snprintf's and paves the way of adding
extra devices (see next patch) a piece of pie.

v2: use a caller (on stack) provided real_path (Eric)

Cc: Eric Engestrom <eric@engestrom.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
---
Eric, I couldn't quite remove all the error handling since on realpath()
failure the output buffer contents are undefined :-\

 xf86drm.c | 34 ++++++++++++++++++++++++++--------
 1 file changed, 26 insertions(+), 8 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index 02da3e1f..51e00d23 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2992,16 +2992,29 @@ static int drmParseSubsystemType(int maj, int min)
 #endif
 }
 
+static char *
+get_real_pci_path(int maj, int min, char *real_path)
+{
+    char path[PATH_MAX + 1];
+
+    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
+    if (!realpath(path, real_path))
+        return NULL;
+
+    return real_path;
+}
+
 static int drmParsePciBusInfo(int maj, int min, drmPciBusInfoPtr info)
 {
 #ifdef __linux__
     unsigned int domain, bus, dev, func;
-    char path[PATH_MAX + 1], *value;
+    char real_path[PATH_MAX + 1], *value;
     int num;
 
-    snprintf(path, sizeof(path), "/sys/dev/char/%d:%d/device", maj, min);
+    if (get_real_pci_path(maj, min, real_path) == NULL)
+        return -ENOENT;
 
-    value = sysfs_uevent_get(path, "PCI_SLOT_NAME");
+    value = sysfs_uevent_get(real_path, "PCI_SLOT_NAME");
     if (!value)
         return -ENOENT;
 
@@ -3114,14 +3127,16 @@ static int parse_separate_sysfs_files(int maj, int min,
       "subsystem_vendor",
       "subsystem_device",
     };
-    char path[PATH_MAX + 1];
+    char path[PATH_MAX + 1], real_path[PATH_MAX + 1];
     unsigned int data[ARRAY_SIZE(attrs)];
     FILE *fp;
     int ret;
 
+    if (get_real_pci_path(maj, min, real_path) == NULL)
+        return -ENOENT;
+
     for (unsigned i = ignore_revision ? 1 : 0; i < ARRAY_SIZE(attrs); i++) {
-        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/%s", maj, min,
-                 attrs[i]);
+        snprintf(path, PATH_MAX, "%s/%s", real_path, attrs[i]);
         fp = fopen(path, "r");
         if (!fp)
             return -errno;
@@ -3145,11 +3160,14 @@ static int parse_separate_sysfs_files(int maj, int min,
 static int parse_config_sysfs_file(int maj, int min,
                                    drmPciDeviceInfoPtr device)
 {
-    char path[PATH_MAX + 1];
+    char path[PATH_MAX + 1], real_path[PATH_MAX + 1];
     unsigned char config[64];
     int fd, ret;
 
-    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/config", maj, min);
+    if (get_real_pci_path(maj, min, real_path) == NULL)
+        return -ENOENT;
+
+    snprintf(path, PATH_MAX, "%s/config", real_path);
     fd = open(path, O_RDONLY);
     if (fd < 0)
         return -errno;
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH libdrm v2 09/10] drmdevice: convert the tabbed output into a tree

[Emil Velikov]

From: Emil Velikov <emil.velikov@collabora.com>

Making the output a little bit easier to parse by human beings.

v2: Add extra whitespace (Eric)

Cc: Eric Engestrom <eric@engestrom.ch>
Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
---
 tests/drmdevice.c | 77 +++++++++++++++++++++++------------------------
 1 file changed, 38 insertions(+), 39 deletions(-)

diff --git a/tests/drmdevice.c b/tests/drmdevice.c
index 0d75836f..049df3cf 100644
--- a/tests/drmdevice.c
+++ b/tests/drmdevice.c
@@ -36,67 +36,66 @@ static void
 print_device_info(drmDevicePtr device, int i, bool print_revision)
 {
     printf("device[%i]\n", i);
-    printf("\tavailable_nodes %04x\n", device->available_nodes);
-    printf("\tnodes\n");
+    printf("+-> available_nodes %#04x\n", device->available_nodes);
+    printf("+-> nodes\n");
     for (int j = 0; j < DRM_NODE_MAX; j++)
         if (device->available_nodes & 1 << j)
-            printf("\t\tnodes[%d] %s\n", j, device->nodes[j]);
+            printf("|   +-> nodes[%d] %s\n", j, device->nodes[j]);
 
-    printf("\tbustype %04x\n", device->bustype);
-    printf("\tbusinfo\n");
+    printf("+-> bustype %04x\n", device->bustype);
     if (device->bustype == DRM_BUS_PCI) {
-        printf("\t\tpci\n");
-        printf("\t\t\tdomain\t%04x\n",device->businfo.pci->domain);
-        printf("\t\t\tbus\t%02x\n", device->businfo.pci->bus);
-        printf("\t\t\tdev\t%02x\n", device->businfo.pci->dev);
-        printf("\t\t\tfunc\t%1u\n", device->businfo.pci->func);
-
-        printf("\tdeviceinfo\n");
-        printf("\t\tpci\n");
-        printf("\t\t\tvendor_id\t%04x\n", device->deviceinfo.pci->vendor_id);
-        printf("\t\t\tdevice_id\t%04x\n", device->deviceinfo.pci->device_id);
-        printf("\t\t\tsubvendor_id\t%04x\n", device->deviceinfo.pci->subvendor_id);
-        printf("\t\t\tsubdevice_id\t%04x\n", device->deviceinfo.pci->subdevice_id);
+        printf("|   +-> pci\n");
+        printf("|       +-> domain %04x\n",device->businfo.pci->domain);
+        printf("|       +-> bus    %02x\n", device->businfo.pci->bus);
+        printf("|       +-> dev    %02x\n", device->businfo.pci->dev);
+        printf("|       +-> func   %1u\n", device->businfo.pci->func);
+
+        printf("+-> deviceinfo\n");
+        printf("    +-> pci\n");
+        printf("        +-> vendor_id     %04x\n", device->deviceinfo.pci->vendor_id);
+        printf("        +-> device_id     %04x\n", device->deviceinfo.pci->device_id);
+        printf("        +-> subvendor_id  %04x\n", device->deviceinfo.pci->subvendor_id);
+        printf("        +-> subdevice_id  %04x\n", device->deviceinfo.pci->subdevice_id);
         if (print_revision)
-            printf("\t\t\trevision_id\t%02x\n", device->deviceinfo.pci->revision_id);
+            printf("        +-> revision_id   %02x\n", device->deviceinfo.pci->revision_id);
         else
-            printf("\t\t\trevision_id\tIGNORED\n");
+            printf("        +-> revision_id   IGNORED\n");
 
     } else if (device->bustype == DRM_BUS_USB) {
-        printf("\t\tusb\n");
-        printf("\t\t\tbus\t%03u\n", device->businfo.usb->bus);
-        printf("\t\t\tdev\t%03u\n", device->businfo.usb->dev);
-
-        printf("\tdeviceinfo\n");
-        printf("\t\tusb\n");
-        printf("\t\t\tvendor\t%04x\n", device->deviceinfo.usb->vendor);
-        printf("\t\t\tproduct\t%04x\n", device->deviceinfo.usb->product);
+        printf("|   +-> usb\n");
+        printf("|       +-> bus %03u\n", device->businfo.usb->bus);
+        printf("|       +-> dev %03u\n", device->businfo.usb->dev);
+
+        printf("+-> deviceinfo\n");
+        printf("    +-> usb\n");
+        printf("        +-> vendor  %04x\n", device->deviceinfo.usb->vendor);
+        printf("        +-> product %04x\n", device->deviceinfo.usb->product);
     } else if (device->bustype == DRM_BUS_PLATFORM) {
         char **compatible = device->deviceinfo.platform->compatible;
 
-        printf("\t\tplatform\n");
-        printf("\t\t\tfullname\t%s\n", device->businfo.platform->fullname);
+        printf("|   +-> platform\n");
+        printf("|       +-> fullname\t%s\n", device->businfo.platform->fullname);
 
-        printf("\tdeviceinfo\n");
-        printf("\t\tplatform\n");
-        printf("\t\t\tcompatible\n");
+        printf("+-> deviceinfo\n");
+        printf("    +-> platform\n");
+        printf("        +-> compatible\n");
 
         while (*compatible) {
-            printf("\t\t\t\t%s\n", *compatible);
+            printf("                    %s\n", *compatible);
             compatible++;
         }
     } else if (device->bustype == DRM_BUS_HOST1X) {
         char **compatible = device->deviceinfo.platform->compatible;
 
-        printf("\t\thost1x\n");
-        printf("\t\t\tfullname\t%s\n", device->businfo.host1x->fullname);
+        printf("|   +-> host1x\n");
+        printf("|       +-> fullname\t%s\n", device->businfo.host1x->fullname);
 
-        printf("\tdeviceinfo\n");
-        printf("\t\tplatform\n");
-        printf("\t\t\tcompatible\n");
+        printf("+-> deviceinfo\n");
+        printf("    +-> platform\n");
+        printf("        +-> compatible\n");
 
         while (*compatible) {
-            printf("\t\t\t\t%s\n", *compatible);
+            printf("                    %s\n", *compatible);
             compatible++;
         }
     } else {
-- 
2.18.0

_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH libdrm v2 04/10] xf86drm: Allocate drmDevicePtr's on stack

[Robert Foss]

LGTM

On 2018-06-29 17:20, Emil Velikov wrote:
> From: Emil Velikov <emil.velikov@collabora.com>
> 
> Currently we dynamically allocate 16 pointers and reallocate more as
> needed.
> 
> Instead, allocate the maximum number (256) on stack - the number is
> small enough and is unlikely to change in the foreseeable future.
> 
> This allows us to simplify the error handling and even shed a few bytes
> off the final binary.
> 
> v2:
>   - add a define & description behind the magic 256 (Rob)
>   - report error to strerr and skip when over 256 device nodes (Rob)
> 
> Cc: Robert Foss <robert.foss@collabora.com>
> Signed-off-by: Emil Velikov <emil.velikov@collabora.com>
> Tested-by: Robert Foss <robert.foss@collabora.com> (v1)
> Reviewed-by: Robert Foss <robert.foss@collabora.com> (v1)
> Reviewed-by: Eric Engestrom <eric@engestrom.ch> (v1)
> ---
>   xf86drm.c | 79 ++++++++++++++++---------------------------------------
>   1 file changed, 23 insertions(+), 56 deletions(-)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 114cf855..02da3e1f 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -3766,6 +3766,13 @@ drm_device_has_rdev(drmDevicePtr device, dev_t find_rdev)
>       return false;
>   }
>   
> +/*
> + * The kernel drm core has a number of places that assume maximum of
> + * 3x64 devices nodes. That's 64 for each of primary, control and
> + * render nodes. Rounded it up to 256 for simplicity.
> + */
> +#define MAX_DRM_NODES 256
> +
>   /**
>    * Get information about the opened drm device
>    *
> @@ -3846,7 +3853,7 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>   
>       return 0;
>   #else
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[MAX_DRM_NODES];
>       drmDevicePtr d;
>       DIR *sysdir;
>       struct dirent *dent;
> @@ -3854,7 +3861,6 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       int subsystem_type;
>       int maj, min;
>       int ret, i, node_count;
> -    int max_count = 16;
>       dev_t find_rdev;
>   
>       if (drm_device_validate_flags(flags))
> @@ -3877,15 +3883,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       if (subsystem_type < 0)
>           return subsystem_type;
>   
> -    local_devices = calloc(max_count, sizeof(drmDevicePtr));
> -    if (local_devices == NULL)
> -        return -ENOMEM;
> -
>       sysdir = opendir(DRM_DIR_NAME);
> -    if (!sysdir) {
> -        ret = -errno;
> -        goto free_locals;
> -    }
> +    if (!sysdir)
> +        return -errno;
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> @@ -3893,16 +3893,12 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>           if (ret)
>               continue;
>   
> -        if (i >= max_count) {
> -            drmDevicePtr *temp;
> -
> -            max_count += 16;
> -            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
> -            if (!temp)
> -                goto free_devices;
> -            local_devices = temp;
> +        if (i >= MAX_DRM_NODES) {
> +            fprintf(stderr, "More than %d drm nodes detected. "
> +                    "Please report a bug - that should not happen.\n"
> +                    "Skipping extra nodes\n", MAX_DRM_NODES);
> +            break;
>           }
> -
>           local_devices[i] = d;
>           i++;
>       }
> @@ -3921,18 +3917,9 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
>       }
>   
>       closedir(sysdir);
> -    free(local_devices);
>       if (*device == NULL)
>           return -ENODEV;
>       return 0;
> -
> -free_devices:
> -    drmFreeDevices(local_devices, i);
> -    closedir(sysdir);
> -
> -free_locals:
> -    free(local_devices);
> -    return ret;
>   #endif
>   }
>   
> @@ -3968,25 +3955,18 @@ int drmGetDevice(int fd, drmDevicePtr *device)
>    */
>   int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>   {
> -    drmDevicePtr *local_devices;
> +    drmDevicePtr local_devices[MAX_DRM_NODES];
>       drmDevicePtr device;
>       DIR *sysdir;
>       struct dirent *dent;
>       int ret, i, node_count, device_count;
> -    int max_count = 16;
>   
>       if (drm_device_validate_flags(flags))
>           return -EINVAL;
>   
> -    local_devices = calloc(max_count, sizeof(drmDevicePtr));
> -    if (local_devices == NULL)
> -        return -ENOMEM;
> -
>       sysdir = opendir(DRM_DIR_NAME);
> -    if (!sysdir) {
> -        ret = -errno;
> -        goto free_locals;
> -    }
> +    if (!sysdir)
> +        return -errno;
>   
>       i = 0;
>       while ((dent = readdir(sysdir))) {
> @@ -3994,16 +3974,12 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>           if (ret)
>               continue;
>   
> -        if (i >= max_count) {
> -            drmDevicePtr *temp;
> -
> -            max_count += 16;
> -            temp = realloc(local_devices, max_count * sizeof(drmDevicePtr));
> -            if (!temp)
> -                goto free_devices;
> -            local_devices = temp;
> +        if (i >= MAX_DRM_NODES) {
> +            fprintf(stderr, "More than %d drm nodes detected. "
> +                    "Please report a bug - that should not happen.\n"
> +                    "Skipping extra nodes\n", MAX_DRM_NODES);
> +            break;
>           }
> -
>           local_devices[i] = device;
>           i++;
>       }
> @@ -4025,16 +4001,7 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
>       }
>   
>       closedir(sysdir);
> -    free(local_devices);
>       return device_count;
> -
> -free_devices:
> -    drmFreeDevices(local_devices, i);
> -    closedir(sysdir);
> -
> -free_locals:
> -    free(local_devices);
> -    return ret;
>   }
>   
>   /**
> 
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/dri-devel