[PATCH libdrm 0/3] xf86drm: Add USB, platform and host1x bus support

[PATCH libdrm 0/3] xf86drm: Add USB, platform and host1x bus support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Hi,

This series enables support for USB, platform and host1x busses in the
drmDevice infrastructure. The goal is to make use of these in Mesa for
PRIME support (via the DRI_PRIME environment variable) for devices on
one of these busses.

I'll be sending out Mesa patches for this shortly.

Thierry

Thierry Reding (3):
  xf86drm: Factor out drmDeviceAlloc()
  xf86drm: Add USB support
  xf86drm: Add platform and host1x bus support

 xf86drm.c | 385 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-----
 xf86drm.h |  35 +++++-
 2 files changed, 392 insertions(+), 28 deletions(-)

-- 
2.11.0

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

[PATCH libdrm 1/3] xf86drm: Factor out drmDeviceAlloc()

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

Subsequent patches will add support for other bus types to drmDevice and
they will duplicate a lot of the code to allocate a drmDevice. Factor
out the common code so it can be reused.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
 xf86drm.c | 78 +++++++++++++++++++++++++++++++++++++++++----------------------
 1 file changed, 51 insertions(+), 27 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index ac59bf513d6c..6a271000af82 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -3153,57 +3153,81 @@ void drmFreeDevices(drmDevicePtr devices[], int count)
             drmFreeDevice(&devices[i]);
 }
 
+static drmDevicePtr drmDeviceAlloc(unsigned int type, const char *node,
+                                   size_t bus_size, size_t device_size,
+                                   char **ptrp)
+{
+    size_t max_node_length, extra, size;
+    drmDevicePtr device;
+    unsigned int i;
+    char *ptr;
+
+    max_node_length = ALIGN(drmGetMaxNodeName(), sizeof(void *));
+    extra = DRM_NODE_MAX * (sizeof(void *) + max_node_length);
+
+    size = sizeof(*device) + extra + bus_size + device_size;
+
+    device = calloc(1, size);
+    if (!device)
+        return NULL;
+
+    device->available_nodes = 1 << type;
+
+    ptr = (char *)device + sizeof(*device);
+    device->nodes = (char **)ptr;
+
+    ptr += DRM_NODE_MAX * sizeof(void *);
+
+    for (i = 0; i < DRM_NODE_MAX; i++) {
+        device->nodes[i] = ptr;
+        ptr += max_node_length;
+    }
+
+    memcpy(device->nodes[type], node, max_node_length);
+
+    *ptrp = ptr;
+
+    return device;
+}
+
 static int drmProcessPciDevice(drmDevicePtr *device,
                                const char *node, int node_type,
                                int maj, int min, bool fetch_deviceinfo,
                                uint32_t flags)
 {
-    const int max_node_str = ALIGN(drmGetMaxNodeName(), sizeof(void *));
-    int ret, i;
+    drmDevicePtr dev;
     char *addr;
+    int ret;
 
-    *device = calloc(1, sizeof(drmDevice) +
-                     (DRM_NODE_MAX * (sizeof(void *) + max_node_str)) +
-                     sizeof(drmPciBusInfo) +
-                     sizeof(drmPciDeviceInfo));
-    if (!*device)
+    dev = drmDeviceAlloc(node_type, node, sizeof(drmPciBusInfo),
+                         sizeof(drmPciDeviceInfo), &addr);
+    if (!dev)
         return -ENOMEM;
 
-    addr = (char*)*device;
+    dev->bustype = DRM_BUS_PCI;
 
-    (*device)->bustype = DRM_BUS_PCI;
-    (*device)->available_nodes = 1 << node_type;
+    dev->businfo.pci = (drmPciBusInfoPtr)addr;
 
-    addr += sizeof(drmDevice);
-    (*device)->nodes = (char**)addr;
-
-    addr += DRM_NODE_MAX * sizeof(void *);
-    for (i = 0; i < DRM_NODE_MAX; i++) {
-        (*device)->nodes[i] = addr;
-        addr += max_node_str;
-    }
-    memcpy((*device)->nodes[node_type], node, max_node_str);
-
-    (*device)->businfo.pci = (drmPciBusInfoPtr)addr;
-
-    ret = drmParsePciBusInfo(maj, min, (*device)->businfo.pci);
+    ret = drmParsePciBusInfo(maj, min, dev->businfo.pci);
     if (ret)
         goto free_device;
 
     // Fetch the device info if the user has requested it
     if (fetch_deviceinfo) {
         addr += sizeof(drmPciBusInfo);
-        (*device)->deviceinfo.pci = (drmPciDeviceInfoPtr)addr;
+        dev->deviceinfo.pci = (drmPciDeviceInfoPtr)addr;
 
-        ret = drmParsePciDeviceInfo(maj, min, (*device)->deviceinfo.pci, flags);
+        ret = drmParsePciDeviceInfo(maj, min, dev->deviceinfo.pci, flags);
         if (ret)
             goto free_device;
     }
+
+    *device = dev;
+
     return 0;
 
 free_device:
-    free(*device);
-    *device = NULL;
+    free(dev);
     return ret;
 }
 
-- 
2.11.0

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

[PATCH libdrm 2/3] xf86drm: Add USB support

[Thierry Reding]

Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
infrastructure.

Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
---
Note that this is completely untested because I don't have a UDL device
for testing. I'm fairly confident that this will work, though, and it'd
be nice to include it before the new platform and host1x busses because
support for it existed in the kernel longer than for platform devices.

 xf86drm.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xf86drm.h |  13 ++++++
 2 files changed, 156 insertions(+)

diff --git a/xf86drm.c b/xf86drm.c
index 6a271000af82..f0171c34c958 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2902,6 +2902,9 @@ static int drmParseSubsystemType(int maj, int min)
     if (strncmp(name, "/pci", 4) == 0)
         return DRM_BUS_PCI;
 
+    if (strncmp(name, "/usb", 4) == 0)
+        return DRM_BUS_USB;
+
     return -EINVAL;
 #elif defined(__OpenBSD__)
     return DRM_BUS_PCI;
@@ -2988,6 +2991,10 @@ static int drmCompareBusInfo(drmDevicePtr a, drmDevicePtr b)
     switch (a->bustype) {
     case DRM_BUS_PCI:
         return memcmp(a->businfo.pci, b->businfo.pci, sizeof(drmPciBusInfo));
+
+    case DRM_BUS_USB:
+        return memcmp(a->businfo.usb, b->businfo.usb, sizeof(drmUsbBusInfo));
+
     default:
         break;
     }
@@ -3231,6 +3238,125 @@ free_device:
     return ret;
 }
 
+static char *sysfs_uevent_get(const char *path, const char *key)
+{
+    char filename[PATH_MAX], *line = NULL, *value = NULL;
+    size_t size = 0, len = strlen(key);
+    ssize_t num;
+    FILE *fp;
+
+    snprintf(filename, PATH_MAX, "%s/uevent", path);
+
+    fp = fopen(filename, "r");
+    if (!fp)
+        return NULL;
+
+    while ((num = getline(&line, &size, fp)) >= 0) {
+        if ((strncmp(line, key, len) == 0) && (line[len] == '=')) {
+            char *start = line + len + 1, *end = line + num - 1;
+
+            if (*end != '\n')
+                end++;
+
+            value = strndup(start, end - start);
+            break;
+        }
+    }
+
+    free(line);
+    fclose(fp);
+
+    return value;
+}
+
+static int drmParseUsbBusInfo(int maj, int min, drmUsbBusInfoPtr info)
+{
+    char path[PATH_MAX + 1], *value;
+    unsigned int bus, dev;
+    int ret;
+
+    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
+
+    value = sysfs_uevent_get(path, "BUSNUM");
+    ret = sscanf(value, "%03u", &bus);
+    free(value);
+
+    if (ret <= 0)
+        return -errno;
+
+    value = sysfs_uevent_get(path, "DEVNUM");
+    ret = sscanf(value, "%03u", &dev);
+    free(value);
+
+    if (ret <= 0)
+        return -errno;
+
+    info->bus = bus;
+    info->dev = dev;
+
+    return 0;
+}
+
+static int drmParseUsbDeviceInfo(int maj, int min, drmUsbDeviceInfoPtr info)
+{
+    char path[PATH_MAX + 1], *value;
+    unsigned int vendor, product;
+    int ret;
+
+    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
+
+    value = sysfs_uevent_get(path, "PRODUCT");
+    ret = sscanf(value, "%x/%x", &vendor, &product);
+    free(value);
+
+    if (ret <= 0)
+        return -errno;
+
+    info->vendor = vendor;
+    info->product = product;
+
+    return 0;
+}
+
+static int drmProcessUsbDevice(drmDevicePtr *device, const char *node,
+                               int node_type, int maj, int min,
+                               bool fetch_deviceinfo, uint32_t flags)
+{
+    drmDevicePtr dev;
+    char *ptr;
+    int ret;
+
+    dev = drmDeviceAlloc(node_type, node, sizeof(drmUsbBusInfo),
+                         sizeof(drmUsbDeviceInfo), &ptr);
+    if (!dev)
+        return -ENOMEM;
+
+    dev->bustype = DRM_BUS_USB;
+
+    dev->businfo.usb = (drmUsbBusInfoPtr)ptr;
+
+    ret = drmParseUsbBusInfo(maj, min, dev->businfo.usb);
+    if (ret < 0)
+        goto free_device;
+
+    if (fetch_deviceinfo) {
+        ptr += sizeof(drmUsbBusInfo);
+        dev->deviceinfo.usb = (drmUsbDeviceInfoPtr)ptr;
+
+        ret = drmParseUsbDeviceInfo(maj, min, dev->deviceinfo.usb);
+        if (ret < 0)
+            goto free_device;
+    }
+
+    *device = dev;
+
+    return 0;
+
+free_device:
+    free(dev);
+    return ret;
+}
+
 /* Consider devices located on the same bus as duplicate and fold the respective
  * entries into a single one.
  *
@@ -3402,6 +3528,14 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
                 goto free_devices;
 
             break;
+
+        case DRM_BUS_USB:
+            ret = drmProcessUsbDevice(&d, node, node_type, maj, min, true, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
         default:
             continue;
         }
@@ -3533,6 +3667,15 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
                 goto free_devices;
 
             break;
+
+        case DRM_BUS_USB:
+            ret = drmProcessUsbDevice(&device, node, node_type, maj, min,
+                                      devices != NULL, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
         default:
             continue;
         }
diff --git a/xf86drm.h b/xf86drm.h
index b340fc46cd44..65d5321950fc 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -767,6 +767,7 @@ extern char *drmGetPrimaryDeviceNameFromFd(int fd);
 extern char *drmGetRenderDeviceNameFromFd(int fd);
 
 #define DRM_BUS_PCI   0
+#define DRM_BUS_USB   1
 
 typedef struct _drmPciBusInfo {
     uint16_t domain;
@@ -783,15 +784,27 @@ typedef struct _drmPciDeviceInfo {
     uint8_t revision_id;
 } drmPciDeviceInfo, *drmPciDeviceInfoPtr;
 
+typedef struct _drmUsbBusInfo {
+    uint8_t bus;
+    uint8_t dev;
+} drmUsbBusInfo, *drmUsbBusInfoPtr;
+
+typedef struct _drmUsbDeviceInfo {
+    uint16_t vendor;
+    uint16_t product;
+} drmUsbDeviceInfo, *drmUsbDeviceInfoPtr;
+
 typedef struct _drmDevice {
     char **nodes; /* DRM_NODE_MAX sized array */
     int available_nodes; /* DRM_NODE_* bitmask */
     int bustype;
     union {
         drmPciBusInfoPtr pci;
+        drmUsbBusInfoPtr usb;
     } businfo;
     union {
         drmPciDeviceInfoPtr pci;
+        drmUsbDeviceInfoPtr usb;
     } deviceinfo;
 } drmDevice, *drmDevicePtr;
 
-- 
2.11.0

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

[PATCH libdrm 3/3] xf86drm: Add platform and host1x bus support

[Thierry Reding]

From: Thierry Reding <treding@nvidia.com>

ARM SoCs usually have their DRM/KMS devices on the platform bus, so add
support for that to enable these devices to be used with the drmDevice
infrastructure.

NVIDIA Tegra SoCs have an additional level in the hierarchy and DRM/KMS
devices can also be on the host1x bus. This is mostly equivalent to the
platform bus.

Signed-off-by: Thierry Reding <treding@nvidia.com>
---
Note that we could probably get away with treating host1x as platform
bus. However, they are technically two different busses in the kernel
and hence we may want to make use of that differentiation later on.

 xf86drm.c | 164 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xf86drm.h |  24 ++++++++-
 2 files changed, 186 insertions(+), 2 deletions(-)

diff --git a/xf86drm.c b/xf86drm.c
index f0171c34c958..69887accb3cb 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -2905,6 +2905,12 @@ static int drmParseSubsystemType(int maj, int min)
     if (strncmp(name, "/usb", 4) == 0)
         return DRM_BUS_USB;
 
+    if (strncmp(name, "/platform", 9) == 0)
+        return DRM_BUS_PLATFORM;
+
+    if (strncmp(name, "/host1x", 7) == 0)
+        return DRM_BUS_HOST1X;
+
     return -EINVAL;
 #elif defined(__OpenBSD__)
     return DRM_BUS_PCI;
@@ -2995,6 +3001,12 @@ static int drmCompareBusInfo(drmDevicePtr a, drmDevicePtr b)
     case DRM_BUS_USB:
         return memcmp(a->businfo.usb, b->businfo.usb, sizeof(drmUsbBusInfo));
 
+    case DRM_BUS_PLATFORM:
+        return memcmp(a->businfo.platform, b->businfo.platform, sizeof(drmPlatformBusInfo));
+
+    case DRM_BUS_HOST1X:
+        return memcmp(a->businfo.host1x, b->businfo.host1x, sizeof(drmHost1xBusInfo));
+
     default:
         break;
     }
@@ -3357,6 +3369,128 @@ free_device:
     return ret;
 }
 
+static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
+{
+    char path[PATH_MAX + 1], *name;
+
+    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
+
+    name = sysfs_uevent_get(path, "OF_FULLNAME");
+    strcpy(info->fullname, name);
+    free(name);
+
+    return 0;
+}
+
+static int drmParsePlatformDeviceInfo(int maj, int min,
+                                      drmPlatformDeviceInfoPtr info)
+{
+    /* XXX fill in platform device info */
+
+    return 0;
+}
+
+static int drmProcessPlatformDevice(drmDevicePtr *device,
+                                    const char *node, int node_type,
+                                    int maj, int min, bool fetch_deviceinfo,
+                                    uint32_t flags)
+{
+    drmDevicePtr dev;
+    char *ptr;
+    int ret;
+
+    dev = drmDeviceAlloc(node_type, node, sizeof(drmPlatformBusInfo),
+                         sizeof(drmPlatformDeviceInfo), &ptr);
+    if (!dev)
+        return -ENOMEM;
+
+    dev->bustype = DRM_BUS_PLATFORM;
+
+    dev->businfo.platform = (drmPlatformBusInfoPtr)ptr;
+
+    ret = drmParsePlatformBusInfo(maj, min, dev->businfo.platform);
+    if (ret < 0)
+        goto free_device;
+
+    if (fetch_deviceinfo) {
+        ptr += sizeof(drmPlatformBusInfo);
+        dev->deviceinfo.platform = (drmPlatformDeviceInfoPtr)ptr;
+
+        ret = drmParsePlatformDeviceInfo(maj, min, dev->deviceinfo.platform);
+        if (ret < 0)
+            goto free_device;
+    }
+
+    *device = dev;
+
+    return 0;
+
+free_device:
+    free(dev);
+    return ret;
+}
+
+static int drmParseHost1xBusInfo(int maj, int min, drmHost1xBusInfoPtr info)
+{
+    char path[PATH_MAX + 1], *name;
+
+    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
+
+    name = sysfs_uevent_get(path, "OF_FULLNAME");
+    strcpy(info->fullname, name);
+    free(name);
+
+    return 0;
+}
+
+static int drmParseHost1xDeviceInfo(int maj, int min,
+                                    drmHost1xDeviceInfoPtr info)
+{
+    /* XXX fill in host1x device info */
+
+    return 0;
+}
+
+static int drmProcessHost1xDevice(drmDevicePtr *device,
+                                  const char *node, int node_type,
+                                  int maj, int min, bool fetch_deviceinfo,
+                                  uint32_t flags)
+{
+    drmDevicePtr dev;
+    char *ptr;
+    int ret;
+
+    dev = drmDeviceAlloc(node_type, node, sizeof(drmHost1xBusInfo),
+                         sizeof(drmHost1xDeviceInfo), &ptr);
+    if (!dev)
+        return -ENOMEM;
+
+    dev->bustype = DRM_BUS_HOST1X;
+
+    dev->businfo.host1x = (drmHost1xBusInfoPtr)ptr;
+
+    ret = drmParseHost1xBusInfo(maj, min, dev->businfo.host1x);
+    if (ret < 0)
+        goto free_device;
+
+    if (fetch_deviceinfo) {
+        ptr += sizeof(drmHost1xBusInfo);
+        dev->deviceinfo.host1x = (drmHost1xDeviceInfoPtr)ptr;
+
+        ret = drmParseHost1xDeviceInfo(maj, min, dev->deviceinfo.host1x);
+        if (ret < 0)
+            goto free_device;
+    }
+
+    *device = dev;
+
+    return 0;
+
+free_device:
+    free(dev);
+    return ret;
+}
+
 /* Consider devices located on the same bus as duplicate and fold the respective
  * entries into a single one.
  *
@@ -3536,6 +3670,20 @@ int drmGetDevice2(int fd, uint32_t flags, drmDevicePtr *device)
 
             break;
 
+        case DRM_BUS_PLATFORM:
+            ret = drmProcessPlatformDevice(&d, node, node_type, maj, min, true, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
+        case DRM_BUS_HOST1X:
+            ret = drmProcessHost1xDevice(&d, node, node_type, maj, min, true, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
         default:
             continue;
         }
@@ -3676,6 +3824,22 @@ int drmGetDevices2(uint32_t flags, drmDevicePtr devices[], int max_devices)
 
             break;
 
+        case DRM_BUS_PLATFORM:
+            ret = drmProcessPlatformDevice(&device, node, node_type, maj, min,
+                                           devices != NULL, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
+        case DRM_BUS_HOST1X:
+            ret = drmProcessHost1xDevice(&device, node, node_type, maj, min,
+                                         devices != NULL, flags);
+            if (ret)
+                goto free_devices;
+
+            break;
+
         default:
             continue;
         }
diff --git a/xf86drm.h b/xf86drm.h
index 65d5321950fc..eeb19e5e187e 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -766,8 +766,10 @@ extern int drmPrimeFDToHandle(int fd, int prime_fd, uint32_t *handle);
 extern char *drmGetPrimaryDeviceNameFromFd(int fd);
 extern char *drmGetRenderDeviceNameFromFd(int fd);
 
-#define DRM_BUS_PCI   0
-#define DRM_BUS_USB   1
+#define DRM_BUS_PCI       0
+#define DRM_BUS_USB       1
+#define DRM_BUS_PLATFORM  2
+#define DRM_BUS_HOST1X    3
 
 typedef struct _drmPciBusInfo {
     uint16_t domain;
@@ -794,6 +796,20 @@ typedef struct _drmUsbDeviceInfo {
     uint16_t product;
 } drmUsbDeviceInfo, *drmUsbDeviceInfoPtr;
 
+typedef struct _drmPlatformBusInfo {
+    char fullname[512];
+} drmPlatformBusInfo, *drmPlatformBusInfoPtr;
+
+typedef struct _drmPlatformDeviceInfo {
+} drmPlatformDeviceInfo, *drmPlatformDeviceInfoPtr;
+
+typedef struct _drmHost1xBusInfo {
+    char fullname[512];
+} drmHost1xBusInfo, *drmHost1xBusInfoPtr;
+
+typedef struct _drmHost1xDeviceInfo {
+} drmHost1xDeviceInfo, *drmHost1xDeviceInfoPtr;
+
 typedef struct _drmDevice {
     char **nodes; /* DRM_NODE_MAX sized array */
     int available_nodes; /* DRM_NODE_* bitmask */
@@ -801,10 +817,14 @@ typedef struct _drmDevice {
     union {
         drmPciBusInfoPtr pci;
         drmUsbBusInfoPtr usb;
+        drmPlatformBusInfoPtr platform;
+        drmHost1xBusInfoPtr host1x;
     } businfo;
     union {
         drmPciDeviceInfoPtr pci;
         drmUsbDeviceInfoPtr usb;
+        drmPlatformDeviceInfoPtr platform;
+        drmHost1xDeviceInfoPtr host1x;
     } deviceinfo;
 } drmDevice, *drmDevicePtr;
 
-- 
2.11.0

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

Re: [PATCH libdrm 2/3] xf86drm: Add USB support

[Emil Velikov]

Hi Thierry,

On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
> Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
> infrastructure.
>
> Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> ---
> Note that this is completely untested because I don't have a UDL device
> for testing. I'm fairly confident that this will work, though, and it'd
> be nice to include it before the new platform and host1x busses because
> support for it existed in the kernel longer than for platform devices.
>
Functionality looks spot on, but I'm a bit hesitant to get this in
without any testing.
Can we please extend tests/drmdevice.c (separate patch?) as poke
someone on dri-devel/xorg-devel to give it a quick run ?

> +static int drmParseUsbDeviceInfo(int maj, int min, drmUsbDeviceInfoPtr info)
> +{
> +    char path[PATH_MAX + 1], *value;
> +    unsigned int vendor, product;
> +    int ret;
> +
> +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
> +
> +    value = sysfs_uevent_get(path, "PRODUCT");
> +    ret = sscanf(value, "%x/%x", &vendor, &product);
> +    free(value);
> +
> +    if (ret <= 0)
> +        return -errno;
> +
> +    info->vendor = vendor;
> +    info->product = product;
> +
Worth fetching bcdDevice as well ?

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

Re: [PATCH libdrm 3/3] xf86drm: Add platform and host1x bus support

[Emil Velikov]

On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
> From: Thierry Reding <treding@nvidia.com>
>
> ARM SoCs usually have their DRM/KMS devices on the platform bus, so add
> support for that to enable these devices to be used with the drmDevice
> infrastructure.
>
> NVIDIA Tegra SoCs have an additional level in the hierarchy and DRM/KMS
> devices can also be on the host1x bus. This is mostly equivalent to the
> platform bus.
>
> Signed-off-by: Thierry Reding <treding@nvidia.com>
> ---
> Note that we could probably get away with treating host1x as platform
> bus. However, they are technically two different busses in the kernel
> and hence we may want to make use of that differentiation later on.
>
Admittedly there's no clear cut either way, but I'm inclined to have
host1x as platform.
I'm leaning towards that since there is no /sys/bus/host1x (afaict),
plus otherwise devices (like imx-vpu/vc4) will end with their own bus
type.


> +static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
> +{
> +    char path[PATH_MAX + 1], *name;
> +
> +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
> +
> +    name = sysfs_uevent_get(path, "OF_FULLNAME");
> +    strcpy(info->fullname, name);
> +    free(name);
> +
Let's be extra careful and ensure that we don't overrun (and properly
terminate) the fixed size fullname[].


> +    return 0;
> +}
> +
> +static int drmParsePlatformDeviceInfo(int maj, int min,
> +                                      drmPlatformDeviceInfoPtr info)
> +{
> +    /* XXX fill in platform device info */
> +
Not 100% sure what exactly we should consider bus and what device
info, either way an empty struct is not going to go well.
As a food for thought, here is some info for imx/etna and vc4.

cat /sys/dev/char/226\:*/device/uevent

DRIVER=etnaviv
OF_NAME=gpu-subsystem
OF_FULLNAME=/gpu-subsystem
OF_COMPATIBLE_0=fsl,imx-gpu-subsystem
OF_COMPATIBLE_N=1
MODALIAS=of:Ngpu-subsystemT<NULL>Cfsl,imx-gpu-subsystem
DRIVER=imx-drm
OF_NAME=display-subsystem
OF_FULLNAME=/display-subsystem
OF_COMPATIBLE_0=fsl,imx-display-subsystem
OF_COMPATIBLE_N=1

DRIVER=vc4-drm
OF_NAME=gpu
OF_FULLNAME=/soc/gpu
OF_COMPATIBLE_0=brcm,bcm2835-vc4
OF_COMPATIBLE_N=1
MODALIAS=of:NgpuT<NULL>Cbrcm,bcm2835-vc4


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

Re: [PATCH libdrm 2/3] xf86drm: Add USB support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 2625 bytes --]

On Sat, Dec 24, 2016 at 04:38:04PM +0000, Emil Velikov wrote:
> Hi Thierry,
> 
> On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
> > Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
> > infrastructure.
> >
> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
> > ---
> > Note that this is completely untested because I don't have a UDL device
> > for testing. I'm fairly confident that this will work, though, and it'd
> > be nice to include it before the new platform and host1x busses because
> > support for it existed in the kernel longer than for platform devices.
> >
> Functionality looks spot on, but I'm a bit hesitant to get this in
> without any testing.
> Can we please extend tests/drmdevice.c (separate patch?) as poke
> someone on dri-devel/xorg-devel to give it a quick run ?

I can do that.

> > +static int drmParseUsbDeviceInfo(int maj, int min, drmUsbDeviceInfoPtr info)
> > +{
> > +    char path[PATH_MAX + 1], *value;
> > +    unsigned int vendor, product;
> > +    int ret;
> > +
> > +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
> > +
> > +    value = sysfs_uevent_get(path, "PRODUCT");
> > +    ret = sscanf(value, "%x/%x", &vendor, &product);
> > +    free(value);
> > +
> > +    if (ret <= 0)
> > +        return -errno;
> > +
> > +    info->vendor = vendor;
> > +    info->product = product;
> > +
> Worth fetching bcdDevice as well ?

This is currently only parsing the uevent file, which doesn't have
bcdDevice. The only data that isn't currently being parsed is TYPE
(bDeviceClass/bdeviceSubClass/bDeviceProtocol), not sure if we'd
want that.

I could of course read bcdDevice from the bcdDevice file.

One thing that I realized the other day, and it's relevant to all of
drmDevice, not just USB, is that we don't have a good way of preserving
ABI compatibility. With the USB and platform/host1x bus patches we're
not running into problems yet, but consider what would happen if we
added more bus or device info. The drmDevice.businfo and
drmDevice.deviceinfo unions could be growing beyond what they are now,
causing applications written against old versions to potentially
allocate too little memory.

I wonder if that's something we need to care about. As long as we keep
the bus and device info fixed after they've been added, we should be
safe because the bus type will be unknown to earlier versions and hence
cause the code to error out early. Technically we wouldn't be able to
ever extend one type of bus or device info, though.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH libdrm 3/3] xf86drm: Add platform and host1x bus support

[Thierry Reding]

[-- Attachment #1.1: Type: text/plain, Size: 4321 bytes --]

On Sat, Dec 24, 2016 at 05:00:27PM +0000, Emil Velikov wrote:
> On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
> > From: Thierry Reding <treding@nvidia.com>
> >
> > ARM SoCs usually have their DRM/KMS devices on the platform bus, so add
> > support for that to enable these devices to be used with the drmDevice
> > infrastructure.
> >
> > NVIDIA Tegra SoCs have an additional level in the hierarchy and DRM/KMS
> > devices can also be on the host1x bus. This is mostly equivalent to the
> > platform bus.
> >
> > Signed-off-by: Thierry Reding <treding@nvidia.com>
> > ---
> > Note that we could probably get away with treating host1x as platform
> > bus. However, they are technically two different busses in the kernel
> > and hence we may want to make use of that differentiation later on.
> >
> Admittedly there's no clear cut either way, but I'm inclined to have
> host1x as platform.
> I'm leaning towards that since there is no /sys/bus/host1x (afaict),

Actually there is:

	# find /sys/bus/host1x
	/sys/bus/host1x
	/sys/bus/host1x/drivers_probe
	/sys/bus/host1x/devices
	/sys/bus/host1x/devices/drm
	/sys/bus/host1x/uevent
	/sys/bus/host1x/drivers
	/sys/bus/host1x/drivers/drm
	/sys/bus/host1x/drivers/drm/bind
	/sys/bus/host1x/drivers/drm/unbind
	/sys/bus/host1x/drivers/drm/module
	/sys/bus/host1x/drivers/drm/uevent
	/sys/bus/host1x/drivers/drm/drm
	/sys/bus/host1x/drivers_autoprobe

> plus otherwise devices (like imx-vpu/vc4) will end with their own bus
> type.

I don't think that would happen. Tegra is somewhat special in this case
because of the host1x device that is a parent for both display and
capture devices. The host1x bus is architected to allow binding each of
the DRM/KMS and V4L2 devices (the V4L2 driver hasn't been merged yet)
to the host1x device.

The reason for this is mostly historical. Back when Tegra support was
added there was a strict rule against adding dummy device nodes to the
device tree. For Tegra this wasn't much of a problem because the host1x
device is the logical choice for a parent, so I just had to write some
glue (the host1x bus) to instantiate a dummy device for each composite
driver.

Later on the component/master infrastructure was added and the rules
regarding dummy device nodes were relaxed, so as far as I know all other
devices end up anchored to a dummy device on the platform bus.

> > +static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
> > +{
> > +    char path[PATH_MAX + 1], *name;
> > +
> > +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
> > +
> > +    name = sysfs_uevent_get(path, "OF_FULLNAME");
> > +    strcpy(info->fullname, name);
> > +    free(name);
> > +
> Let's be extra careful and ensure that we don't overrun (and properly
> terminate) the fixed size fullname[].

Okay, will do.

> > +    return 0;
> > +}
> > +
> > +static int drmParsePlatformDeviceInfo(int maj, int min,
> > +                                      drmPlatformDeviceInfoPtr info)
> > +{
> > +    /* XXX fill in platform device info */
> > +
> Not 100% sure what exactly we should consider bus and what device
> info, either way an empty struct is not going to go well.
> As a food for thought, here is some info for imx/etna and vc4.
> 
> cat /sys/dev/char/226\:*/device/uevent
> 
> DRIVER=etnaviv
> OF_NAME=gpu-subsystem
> OF_FULLNAME=/gpu-subsystem
> OF_COMPATIBLE_0=fsl,imx-gpu-subsystem
> OF_COMPATIBLE_N=1
> MODALIAS=of:Ngpu-subsystemT<NULL>Cfsl,imx-gpu-subsystem
> DRIVER=imx-drm
> OF_NAME=display-subsystem
> OF_FULLNAME=/display-subsystem
> OF_COMPATIBLE_0=fsl,imx-display-subsystem
> OF_COMPATIBLE_N=1
> 
> DRIVER=vc4-drm
> OF_NAME=gpu
> OF_FULLNAME=/soc/gpu
> OF_COMPATIBLE_0=brcm,bcm2835-vc4
> OF_COMPATIBLE_N=1
> MODALIAS=of:NgpuT<NULL>Cbrcm,bcm2835-vc4

I think the full name is appropriate for bus info because it's about the
closest thing we have to address the device (the equivalent to domain,
bus, device and function numbers).

The list of compatible strings might be suitable for device information
and might be useful for drivers to determine capabilities if the kernel
doesn't have another means of communicating that to userspace.

Thierry

[-- Attachment #1.2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[-- Attachment #2: Type: text/plain, Size: 160 bytes --]

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

Re: [PATCH libdrm 2/3] xf86drm: Add USB support

[Emil Velikov]

On 2 January 2017 at 13:26, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Sat, Dec 24, 2016 at 04:38:04PM +0000, Emil Velikov wrote:
>> Hi Thierry,
>>
>> On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > Allow DRM/KMS devices hosted on USB to be detected by the drmDevice
>> > infrastructure.
>> >
>> > Signed-off-by: Thierry Reding <thierry.reding@gmail.com>
>> > ---
>> > Note that this is completely untested because I don't have a UDL device
>> > for testing. I'm fairly confident that this will work, though, and it'd
>> > be nice to include it before the new platform and host1x busses because
>> > support for it existed in the kernel longer than for platform devices.
>> >
>> Functionality looks spot on, but I'm a bit hesitant to get this in
>> without any testing.
>> Can we please extend tests/drmdevice.c (separate patch?) as poke
>> someone on dri-devel/xorg-devel to give it a quick run ?
>
> I can do that.
>
>> > +static int drmParseUsbDeviceInfo(int maj, int min, drmUsbDeviceInfoPtr info)
>> > +{
>> > +    char path[PATH_MAX + 1], *value;
>> > +    unsigned int vendor, product;
>> > +    int ret;
>> > +
>> > +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
>> > +
>> > +    value = sysfs_uevent_get(path, "PRODUCT");
>> > +    ret = sscanf(value, "%x/%x", &vendor, &product);
>> > +    free(value);
>> > +
>> > +    if (ret <= 0)
>> > +        return -errno;
>> > +
>> > +    info->vendor = vendor;
>> > +    info->product = product;
>> > +
>> Worth fetching bcdDevice as well ?
>
> This is currently only parsing the uevent file, which doesn't have
> bcdDevice. The only data that isn't currently being parsed is TYPE
> (bDeviceClass/bdeviceSubClass/bDeviceProtocol), not sure if we'd
> want that.
>
> I could of course read bcdDevice from the bcdDevice file.
>
It's up-to you really. The above was meant as food for thought.

> One thing that I realized the other day, and it's relevant to all of
> drmDevice, not just USB, is that we don't have a good way of preserving
> ABI compatibility. With the USB and platform/host1x bus patches we're
> not running into problems yet, but consider what would happen if we
> added more bus or device info. The drmDevice.businfo and
> drmDevice.deviceinfo unions could be growing beyond what they are now,
> causing applications written against old versions to potentially
> allocate too little memory.
>
> I wonder if that's something we need to care about. As long as we keep
> the bus and device info fixed after they've been added, we should be
> safe because the bus type will be unknown to earlier versions and hence
> cause the code to error out early. Technically we wouldn't be able to
> ever extend one type of bus or device info, though.
>
Afaict extending structs will be an issue, as one builds mesa/other
against new libdrm and then uses older at runtime.
Yes, things are not ideal on the topic, but it's something that's been
around for a while.

These days distros are pretty good at keeping the runtime version same
as (or greater than) the build-time one, so we should be safe.

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

Re: [PATCH libdrm 3/3] xf86drm: Add platform and host1x bus support

[Emil Velikov]

On 2 January 2017 at 13:53, Thierry Reding <thierry.reding@gmail.com> wrote:
> On Sat, Dec 24, 2016 at 05:00:27PM +0000, Emil Velikov wrote:
>> On 23 December 2016 at 17:49, Thierry Reding <thierry.reding@gmail.com> wrote:
>> > From: Thierry Reding <treding@nvidia.com>
>> >
>> > ARM SoCs usually have their DRM/KMS devices on the platform bus, so add
>> > support for that to enable these devices to be used with the drmDevice
>> > infrastructure.
>> >
>> > NVIDIA Tegra SoCs have an additional level in the hierarchy and DRM/KMS
>> > devices can also be on the host1x bus. This is mostly equivalent to the
>> > platform bus.
>> >
>> > Signed-off-by: Thierry Reding <treding@nvidia.com>
>> > ---
>> > Note that we could probably get away with treating host1x as platform
>> > bus. However, they are technically two different busses in the kernel
>> > and hence we may want to make use of that differentiation later on.
>> >
>> Admittedly there's no clear cut either way, but I'm inclined to have
>> host1x as platform.
>> I'm leaning towards that since there is no /sys/bus/host1x (afaict),
>
> Actually there is:
>
>         # find /sys/bus/host1x
>         /sys/bus/host1x
>         /sys/bus/host1x/drivers_probe
>         /sys/bus/host1x/devices
>         /sys/bus/host1x/devices/drm
>         /sys/bus/host1x/uevent
>         /sys/bus/host1x/drivers
>         /sys/bus/host1x/drivers/drm
>         /sys/bus/host1x/drivers/drm/bind
>         /sys/bus/host1x/drivers/drm/unbind
>         /sys/bus/host1x/drivers/drm/module
>         /sys/bus/host1x/drivers/drm/uevent
>         /sys/bus/host1x/drivers/drm/drm
>         /sys/bus/host1x/drivers_autoprobe
>
>> plus otherwise devices (like imx-vpu/vc4) will end with their own bus
>> type.
>
> I don't think that would happen. Tegra is somewhat special in this case
> because of the host1x device that is a parent for both display and
> capture devices. The host1x bus is architected to allow binding each of
> the DRM/KMS and V4L2 devices (the V4L2 driver hasn't been merged yet)
> to the host1x device.
>
> The reason for this is mostly historical. Back when Tegra support was
> added there was a strict rule against adding dummy device nodes to the
> device tree. For Tegra this wasn't much of a problem because the host1x
> device is the logical choice for a parent, so I just had to write some
> glue (the host1x bus) to instantiate a dummy device for each composite
> driver.
>
> Later on the component/master infrastructure was added and the rules
> regarding dummy device nodes were relaxed, so as far as I know all other
> devices end up anchored to a dummy device on the platform bus.
>
I see. Thanks for the correction and the educational comment.

>> > +static int drmParsePlatformBusInfo(int maj, int min, drmPlatformBusInfoPtr info)
>> > +{
>> > +    char path[PATH_MAX + 1], *name;
>> > +
>> > +    snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device", maj, min);
>> > +
>> > +    name = sysfs_uevent_get(path, "OF_FULLNAME");
>> > +    strcpy(info->fullname, name);
>> > +    free(name);
>> > +
>> Let's be extra careful and ensure that we don't overrun (and properly
>> terminate) the fixed size fullname[].
>
> Okay, will do.
>
>> > +    return 0;
>> > +}
>> > +
>> > +static int drmParsePlatformDeviceInfo(int maj, int min,
>> > +                                      drmPlatformDeviceInfoPtr info)
>> > +{
>> > +    /* XXX fill in platform device info */
>> > +
>> Not 100% sure what exactly we should consider bus and what device
>> info, either way an empty struct is not going to go well.
>> As a food for thought, here is some info for imx/etna and vc4.
>>
>> cat /sys/dev/char/226\:*/device/uevent
>>
>> DRIVER=etnaviv
>> OF_NAME=gpu-subsystem
>> OF_FULLNAME=/gpu-subsystem
>> OF_COMPATIBLE_0=fsl,imx-gpu-subsystem
>> OF_COMPATIBLE_N=1
>> MODALIAS=of:Ngpu-subsystemT<NULL>Cfsl,imx-gpu-subsystem
>> DRIVER=imx-drm
>> OF_NAME=display-subsystem
>> OF_FULLNAME=/display-subsystem
>> OF_COMPATIBLE_0=fsl,imx-display-subsystem
>> OF_COMPATIBLE_N=1
>>
>> DRIVER=vc4-drm
>> OF_NAME=gpu
>> OF_FULLNAME=/soc/gpu
>> OF_COMPATIBLE_0=brcm,bcm2835-vc4
>> OF_COMPATIBLE_N=1
>> MODALIAS=of:NgpuT<NULL>Cbrcm,bcm2835-vc4
>
> I think the full name is appropriate for bus info because it's about the
> closest thing we have to address the device (the equivalent to domain,
> bus, device and function numbers).
>
> The list of compatible strings might be suitable for device information
> and might be useful for drivers to determine capabilities if the kernel
> doesn't have another means of communicating that to userspace.
>
Agreed. With your thorough explanation in mind I think your
patch/proposal is spot on.

So with the overflow check, this is safe to land.

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