[PATCH 0/5] some drm and amdgpu patches

[PATCH 0/5] some drm and amdgpu patches

[Jammy Zhou]

This series is a set of patches in my side pending for merge. And I rebased it
with the drm_private series from Emil.

Emil Velikov (1):
  drm: add interface to get drm devices on the system v2

Jammy Zhou (4):
  amdgpu: improve amdgpu_vamgr_init
  amdgpu: add flag to support 32bit VA address v3
  amdgpu: fix one warning from previous commit
  amdgpu: make vamgr per device v2

 amdgpu/amdgpu.h          |   5 +
 amdgpu/amdgpu_device.c   |  33 ++++-
 amdgpu/amdgpu_internal.h |  10 +-
 amdgpu/amdgpu_vamgr.c    |  61 ++++----
 xf86drm.c                | 351 +++++++++++++++++++++++++++++++++++++++++++++++
 xf86drm.h                |  34 +++++
 6 files changed, 458 insertions(+), 36 deletions(-)

-- 
1.9.1

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

[PATCH 1/5] drm: add interface to get drm devices on the system v2

[Jammy Zhou]

From: Emil Velikov <emil.l.velikov@gmail.com>

For mutiple GPU support, the devices on the system should be enumerated
to get necessary information about each device, and the drmGetDevices
interface is added for this. Currently only PCI devices are supported for
the enumeration.

Typical usage:
int count;
drmDevicePtr *foo;
count = drmGetDevices(NULL, 0);
foo = calloc(count, sizeof(drmDevicePtr));
count = drmGetDevices(foo, count);
/* find proper device, open correct device node, etc */
drmFreeDevices(foo, count);
free(foo);

v2: change according to feedback from Emil

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
---
 xf86drm.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 xf86drm.h |  34 ++++++
 2 files changed, 385 insertions(+)

diff --git a/xf86drm.c b/xf86drm.c
index 5e02969..237663b 100644
--- a/xf86drm.c
+++ b/xf86drm.c
@@ -55,6 +55,7 @@
 #ifdef HAVE_SYS_MKDEV_H
 # include <sys/mkdev.h> /* defines major(), minor(), and makedev() on Solaris */
 #endif
+#include <math.h>
 
 /* Not all systems have MAP_FAILED defined */
 #ifndef MAP_FAILED
@@ -2829,3 +2830,353 @@ char *drmGetRenderDeviceNameFromFd(int fd)
 {
 	return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
 }
+
+#ifdef __linux__
+static int drmParseSubsystemType(const char *str)
+{
+    char link[PATH_MAX + 1] = "";
+    char *name;
+
+    if (readlink(str, link, PATH_MAX) < 0)
+        return -EINVAL;
+
+    name = strrchr(link, '/');
+    if (!name)
+        return -EINVAL;
+
+    name++;
+
+    if (strncmp(name, "pci", 3) == 0)
+        return DRM_BUS_PCI;
+
+    return -EINVAL;
+}
+
+static int drmParsePciBusInfo(const char *str, drmPciBusInfoPtr info)
+{
+    int domain, bus, dev, func;
+    char *value;
+
+    if (str == NULL)
+        return -EINVAL;
+
+    value = strstr(str, "PCI_SLOT_NAME=");
+    if (value == NULL)
+        return -EINVAL;
+
+    value += strlen("PCI_SLOT_NAME=");
+
+    if (sscanf(value, "%04x:%02x:%02x.%1u",
+               &domain, &bus, &dev, &func) != 4)
+        return -EINVAL;
+
+    info->domain = domain;
+    info->bus = bus;
+    info->dev = dev;
+    info->func = func;
+
+    return 0;
+}
+
+static int drmSameDevice(drmDevicePtr a, drmDevicePtr b)
+{
+    if (a->bustype != b->bustype)
+        return 0;
+
+    switch (a->bustype) {
+    case DRM_BUS_PCI:
+        if (memcmp(a->businfo.pci, b->businfo.pci, sizeof(drmPciBusInfo)) == 0)
+            return 1;
+    default:
+        break;
+    }
+
+    return 0;
+}
+
+static int drmGetNodeType(const char *name)
+{
+    if (strncmp(name, DRM_PRIMARY_MINOR_NAME,
+        sizeof(DRM_PRIMARY_MINOR_NAME) - 1) == 0)
+        return DRM_NODE_PRIMARY;
+
+    if (strncmp(name, DRM_CONTROL_MINOR_NAME,
+        sizeof(DRM_CONTROL_MINOR_NAME ) - 1) == 0)
+        return DRM_NODE_CONTROL;
+
+    if (strncmp(name, DRM_RENDER_MINOR_NAME,
+        sizeof(DRM_RENDER_MINOR_NAME) - 1) == 0)
+        return DRM_NODE_RENDER;
+
+    return -EINVAL;
+}
+
+static int drmParsePciDeviceInfo(const char *config,
+                                 drmPciDeviceInfoPtr device)
+{
+    if (config == NULL)
+        return -EINVAL;
+
+    device->vendor_id = config[0] | (config[1] << 8);
+    device->device_id = config[2] | (config[3] << 8);
+    device->revision_id = config[8];
+    device->subvendor_id = config[44] | (config[45] << 8);
+    device->subdevice_id = config[46] | (config[47] << 8);
+
+    return 0;
+}
+
+static void drmFreeDevice(drmDevicePtr device)
+{
+    int i;
+
+    if (device == NULL)
+        return;
+
+    if (device->nodes != NULL)
+        for (i = 0; i < DRM_NODE_MAX; i++)
+            free(device->nodes[i]);
+
+    free(device->nodes);
+    free(device->businfo.pci);
+    free(device->deviceinfo.pci);
+}
+
+void drmFreeDevices(drmDevicePtr devices[], int count)
+{
+    int i;
+
+    if (devices == NULL)
+        return;
+
+    for (i = 0; i < count; i++) {
+        drmFreeDevice(devices[i]);
+        free(devices[i]);
+        devices[i] = NULL;
+    }
+}
+
+/**
+ * Get drm devices on the system
+ *
+ * \param devices the array of devices with drmDevicePtr elements
+ *                can be NULL to get the device number first
+ * \param max_devices the maximum number of devices for the array
+ *
+ * \return on error - negative error code,
+ *         if devices is NULL - total number of devices available on the system,
+ *         alternatively the number of devices stored in devices[], which is
+ *         capped by the max_devices.
+ */
+int drmGetDevices(drmDevicePtr devices[], int max_devices)
+{
+    drmDevicePtr devs = NULL;
+    drmPciBusInfoPtr pcibus = NULL;
+    drmPciDeviceInfoPtr pcidevice = NULL;
+    DIR *sysdir = NULL;
+    struct dirent *dent = NULL;
+    struct stat sbuf = {0};
+    char node[PATH_MAX + 1] = "";
+    char path[PATH_MAX + 1] = "";
+    char data[128] = "";
+    char config[64] = "";
+    int node_type, subsystem_type;
+    int maj, min;
+    int fd;
+    int ret, i = 0, j, node_count, device_count = 0;
+    int max_count = 16;
+    int *duplicated = NULL;
+
+    devs = calloc(max_count, sizeof(*devs));
+    if (devs == NULL)
+        return -ENOMEM;
+
+    sysdir = opendir(DRM_DIR_NAME);
+    if (!sysdir) {
+        ret = -errno;
+        goto free_locals;
+    }
+
+    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;
+
+        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/subsystem",
+                 maj, min);
+        subsystem_type = drmParseSubsystemType(path);
+
+        if (subsystem_type < 0)
+            continue;
+
+        switch (subsystem_type) {
+        case DRM_BUS_PCI:
+            pcibus = calloc(1, sizeof(*pcibus));
+            if (pcibus == NULL) {
+                ret = -ENOMEM;
+                goto free_locals;
+            }
+
+            snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/uevent",
+                     maj, min);
+            fd = open(path, O_RDONLY);
+            if (fd < 0) {
+                ret = -errno;
+                goto free_locals;
+            }
+            ret = read(fd, data, sizeof(data));
+            if (ret < 0) {
+                ret = -errno;
+                close(fd);
+                goto free_locals;
+            }
+
+            ret = drmParsePciBusInfo(data, pcibus);
+            close(fd);
+            if (ret)
+                goto free_locals;
+
+            if (i >= max_count) {
+                max_count += 16;
+                devs = realloc(devs, max_count * sizeof(*devs));
+            }
+
+            devs[i].businfo.pci = pcibus;
+            devs[i].bustype = subsystem_type;
+            devs[i].nodes = calloc(DRM_NODE_MAX, sizeof(char *));
+            if (devs[i].nodes == NULL) {
+                ret = -ENOMEM;
+                goto free_locals;
+            }
+            devs[i].nodes[node_type] = strdup(node);
+            if (devs[i].nodes[node_type] == NULL) {
+                ret = -ENOMEM;
+                goto free_locals;
+            }
+            devs[i].available_nodes = 1 << node_type;
+
+            if (devices != NULL) {
+                snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config",
+                         dent->d_name);
+                fd = open(path, O_RDONLY);
+                if (fd < 0) {
+                     ret = -errno;
+                     goto free_locals;
+                }
+                ret = read(fd, config, 64);
+                if (ret < 0) {
+                    ret = -errno;
+                    close(fd);
+                    goto free_locals;
+                }
+
+                pcidevice = calloc(1, sizeof(*pcidevice));
+                if (pcidevice == NULL) {
+                    ret = -ENOMEM;
+                    goto free_locals;
+                }
+
+                ret = drmParsePciDeviceInfo(config, pcidevice);
+                if (ret)
+                    goto free_locals;
+
+                devs[i].deviceinfo.pci = pcidevice;
+                close(fd);
+            }
+            break;
+        default:
+            fprintf(stderr, "The subsystem type is not supported yet\n");
+            break;
+        }
+        i++;
+    }
+
+    node_count = i;
+
+    /* merge duplicated devices with same domain/bus/device/func IDs */
+    duplicated = calloc(node_count, sizeof(*duplicated));
+    if (duplicated == NULL) {
+        ret = -ENOMEM;
+        goto free_locals;
+    }
+
+    for (i = 0; i < node_count; i++) {
+        for (j = i+1; j < node_count; j++) {
+            if (duplicated[i] || duplicated[j])
+                continue;
+            if (drmSameDevice(&devs[i], &devs[j])) {
+                duplicated[j] = 1;
+                devs[i].available_nodes |= devs[j].available_nodes;
+                node_type = log2(devs[j].available_nodes);
+                devs[i].nodes[node_type] = devs[j].nodes[node_type];
+                free(devs[j].nodes);
+                free(devs[j].businfo.pci);
+                free(devs[j].deviceinfo.pci);
+            }
+        }
+    }
+
+    for (i = 0; i < node_count; i++) {
+        if(duplicated[i] == 0) {
+            if ((devices != NULL) && (device_count < max_devices)) {
+                devices[device_count] = calloc(1, sizeof(drmDevice));
+                if (devices[device_count] == NULL) {
+                    ret = -ENOMEM;
+                    break;
+                }
+                memcpy(devices[device_count], &devs[i], sizeof(drmDevice));
+            } else
+                drmFreeDevice(&devs[i]);
+            device_count++;
+        }
+    }
+
+    if (i < node_count) {
+        drmFreeDevices(devices, device_count);
+        for ( ; i < node_count; i++)
+            if(duplicated[i] == 0)
+                drmFreeDevice(&devs[i]);
+    } else
+        ret = device_count;
+
+    free(duplicated);
+    free(devs);
+    closedir(sysdir);
+    return ret;
+
+free_locals:
+    for (j = 0; j < i; j++)
+        drmFreeDevice(&devs[j]);
+    free(pcidevice);
+    free(pcibus);
+    free(devs);
+    closedir(sysdir);
+    return ret;
+}
+#else
+void drmFreeDevices(drmDevicePtr devices[], int count)
+{
+    (void)devices;
+    (void)count;
+}
+
+int drmGetDevices(drmDevicePtr devices[], int max_devices)
+{
+    (void)devices;
+    (void)max_devices;
+    return -EINVAL;
+}
+
+#warning "Missing implementation of drmGetDevices/drmFreeDevices"
+
+#endif
diff --git a/xf86drm.h b/xf86drm.h
index 360e04a..e82ca84 100644
--- a/xf86drm.h
+++ b/xf86drm.h
@@ -563,6 +563,8 @@ extern int           drmOpen(const char *name, const char *busid);
 #define DRM_NODE_PRIMARY 0
 #define DRM_NODE_CONTROL 1
 #define DRM_NODE_RENDER  2
+#define DRM_NODE_MAX     3
+
 extern int           drmOpenWithType(const char *name, const char *busid,
                                      int type);
 
@@ -759,6 +761,38 @@ 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
+
+typedef struct _drmPciBusInfo {
+    uint16_t domain;
+    uint8_t bus;
+    uint8_t dev;
+    uint8_t func;
+} drmPciBusInfo, *drmPciBusInfoPtr;
+
+typedef struct _drmPciDeviceInfo {
+    uint16_t vendor_id;
+    uint16_t device_id;
+    uint16_t subvendor_id;
+    uint16_t subdevice_id;
+    uint8_t revision_id;
+} drmPciDeviceInfo, *drmPciDeviceInfoPtr;
+
+typedef struct _drmDevice {
+    char **nodes; /* DRM_NODE_MAX sized array */
+    int available_nodes; /* DRM_NODE_* bitmask */
+    int bustype;
+    union {
+        drmPciBusInfoPtr pci;
+    } businfo;
+    union {
+        drmPciDeviceInfoPtr pci;
+    } deviceinfo;
+} drmDevice, *drmDevicePtr;
+
+extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
+extern void drmFreeDevices(drmDevicePtr devices[], int count);
+
 #if defined(__cplusplus)
 }
 #endif
-- 
1.9.1

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 13 August 2015 at 04:33, Jammy Zhou <Jammy.Zhou@amd.com> wrote:
> From: Emil Velikov <emil.l.velikov@gmail.com>
>
> For mutiple GPU support, the devices on the system should be enumerated
> to get necessary information about each device, and the drmGetDevices
> interface is added for this. Currently only PCI devices are supported for
> the enumeration.
>
If there are any other devices they will still be counted when
drmGetDevices(NULL, 0)... Is that intentional ?


> +static int drmParsePciDeviceInfo(const char *config,
> +                                 drmPciDeviceInfoPtr device)
> +{
> +    if (config == NULL)
> +        return -EINVAL;
> +
> +    device->vendor_id = config[0] | (config[1] << 8);
> +    device->device_id = config[2] | (config[3] << 8);
> +    device->revision_id = config[8];
> +    device->subvendor_id = config[44] | (config[45] << 8);
> +    device->subdevice_id = config[46] | (config[47] << 8);
> +
Something funny is happening here - on my intel system vendor_id is
reported as 0xff86, instead of 0x8086. Subvendor/device are also
messed up - ffaa and ffda instead of 17aa + 21da.

One could bikeshed on the de-duplication error path(s), but
considering that things work and there are no leaks we can leave that
for some other day.

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

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

Hi Emil,

> If there are any other devices they will still be counted when drmGetDevices(NULL, 0)... Is that intentional ?
Yes, I think so, so that this interface can support different kinds of devices in the future. For example, we have some ARM platforms supporting PCIE, in which case we can connect one PCIE graphics card, then there will be one GPU with the platform bus (integrated GPU in the ARM SOC), and one discrete GPU on the PCIE bus. 

> Something funny is happening here - on my intel system vendor_id is reported as 0xff86, instead of 0x8086. Subvendor/device are also messed up - ffaa and ffda instead of 17aa + 21da.
That's really interesting. Did you try to update the system BIOS?

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Thursday, August 13, 2015 9:58 PM
To: Zhou, Jammy
Cc: ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 13 August 2015 at 04:33, Jammy Zhou <Jammy.Zhou@amd.com> wrote:
> From: Emil Velikov <emil.l.velikov@gmail.com>
>
> For mutiple GPU support, the devices on the system should be 
> enumerated to get necessary information about each device, and the 
> drmGetDevices interface is added for this. Currently only PCI devices 
> are supported for the enumeration.
>
If there are any other devices they will still be counted when drmGetDevices(NULL, 0)... Is that intentional ?


> +static int drmParsePciDeviceInfo(const char *config,
> +                                 drmPciDeviceInfoPtr device) {
> +    if (config == NULL)
> +        return -EINVAL;
> +
> +    device->vendor_id = config[0] | (config[1] << 8);
> +    device->device_id = config[2] | (config[3] << 8);
> +    device->revision_id = config[8];
> +    device->subvendor_id = config[44] | (config[45] << 8);
> +    device->subdevice_id = config[46] | (config[47] << 8);
> +
Something funny is happening here - on my intel system vendor_id is reported as 0xff86, instead of 0x8086. Subvendor/device are also messed up - ffaa and ffda instead of 17aa + 21da.

One could bikeshed on the de-duplication error path(s), but considering that things work and there are no leaks we can leave that for some other day.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14 August 2015 at 06:53, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Hi Emil,
>
>> If there are any other devices they will still be counted when drmGetDevices(NULL, 0)... Is that intentional ?
> Yes, I think so, so that this interface can support different kinds of devices in the future. For example, we have some ARM platforms supporting PCIE, in which case we can connect one PCIE graphics card, then there will be one GPU with the platform bus (integrated GPU in the ARM SOC), and one discrete GPU on the PCIE bus.
>
What is the point in claiming that you have X+Y devices, if the API
does not provide any information about Y of them ? It seems very
misleading imho.

>> Something funny is happening here - on my intel system vendor_id is reported as 0xff86, instead of 0x8086. Subvendor/device are also messed up - ffaa and ffda instead of 17aa + 21da.
> That's really interesting. Did you try to update the system BIOS?
>
Seems like a C Programming 101 issue to me rather than a BIOS one.The
(signed) char 0x86 gets extended/promoted to 0xff86 and then all hell
breaks loose. Adding typecast(s) should fix it. That does not excuse
me from writing is so weird from the start :)

Thanks for tweaking/ironing the bugs out.
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

> What is the point in claiming that you have X+Y devices, if the API does not provide any information about Y of them ? It seems very misleading imho.

I'm not sure if I understand your question correctly. Do you mean if the Y devices will be enumerated with current implementation? If so, I think the answer should be 'NO', since other bus types (i.e, platform, USB) are not supported yet.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, August 14, 2015 4:35 PM
To: Zhou, Jammy
Cc: ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 14 August 2015 at 06:53, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Hi Emil,
>
>> If there are any other devices they will still be counted when drmGetDevices(NULL, 0)... Is that intentional ?
> Yes, I think so, so that this interface can support different kinds of devices in the future. For example, we have some ARM platforms supporting PCIE, in which case we can connect one PCIE graphics card, then there will be one GPU with the platform bus (integrated GPU in the ARM SOC), and one discrete GPU on the PCIE bus.
>
What is the point in claiming that you have X+Y devices, if the API does not provide any information about Y of them ? It seems very misleading imho.

>> Something funny is happening here - on my intel system vendor_id is reported as 0xff86, instead of 0x8086. Subvendor/device are also messed up - ffaa and ffda instead of 17aa + 21da.
> That's really interesting. Did you try to update the system BIOS?
>
Seems like a C Programming 101 issue to me rather than a BIOS one.The
(signed) char 0x86 gets extended/promoted to 0xff86 and then all hell breaks loose. Adding typecast(s) should fix it. That does not excuse me from writing is so weird from the start :)

Thanks for tweaking/ironing the bugs out.
Emil
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14 August 2015 at 10:41, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
>> What is the point in claiming that you have X+Y devices, if the API does not provide any information about Y of them ? It seems very misleading imho.
>
> I'm not sure if I understand your question correctly.
Easy - replace X with "pci" and Y with "platform/usb" :)

Or in other words:
user: "hey libdrm, how many devices do we have"
libdrm: "hey user, there are 10 devices here."
user: "great, tell me all about them"
libdrm: "sure... well I cannot tell you anything about 3 of them, but
the rest are fine"
user: "why did you stay that they are 10, if there is no info for 3 of them ?"

Fwiw it can be argued either way but I'd suspect that the current
behaviour is not too welcoming.

If people feel for the current behaviour I'kk be ok with it.

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

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

Okay, I got it. Actually with current implementation, only the number of PCI devices on the system is returned for drmGetDevices(NULL, 0). I extracted related code below. I hope it can address your concern :-)

+static int drmParseSubsystemType(const char *str) {
+    char link[PATH_MAX + 1] = "";
+    char *name;
+
+    if (readlink(str, link, PATH_MAX) < 0)
+        return -EINVAL;
+
+    name = strrchr(link, '/');
+    if (!name)
+        return -EINVAL;
+
+    name++;
+
+    if (strncmp(name, "pci", 3) == 0)
+        return DRM_BUS_PCI;
+
+    return -EINVAL;
+}
...
+        subsystem_type = drmParseSubsystemType(path);
+
+        if (subsystem_type < 0)
+            continue;

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, August 14, 2015 6:05 PM
To: Zhou, Jammy
Cc: ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 14 August 2015 at 10:41, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
>> What is the point in claiming that you have X+Y devices, if the API does not provide any information about Y of them ? It seems very misleading imho.
>
> I'm not sure if I understand your question correctly.
Easy - replace X with "pci" and Y with "platform/usb" :)

Or in other words:
user: "hey libdrm, how many devices do we have"
libdrm: "hey user, there are 10 devices here."
user: "great, tell me all about them"
libdrm: "sure... well I cannot tell you anything about 3 of them, but the rest are fine"
user: "why did you stay that they are 10, if there is no info for 3 of them ?"

Fwiw it can be argued either way but I'd suspect that the current behaviour is not too welcoming.

If people feel for the current behaviour I'kk be ok with it.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14 August 2015 at 13:15, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Okay, I got it. Actually with current implementation, only the number of PCI devices on the system is returned for drmGetDevices(NULL, 0). I extracted related code below. I hope it can address your concern :-)
>
Had a blond moment and got confused with the subsystem_type default
statement. Please ignore my earlier rambling.

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

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

That's okay. Shall we get this patch merged now if no other objections?

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, August 14, 2015 8:29 PM
To: Zhou, Jammy
Cc: ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 14 August 2015 at 13:15, Zhou, Jammy <Jammy.Zhou@amd.com> wrote:
> Okay, I got it. Actually with current implementation, only the number 
> of PCI devices on the system is returned for drmGetDevices(NULL, 0). I 
> extracted related code below. I hope it can address your concern :-)
>
Had a blond moment and got confused with the subsystem_type default statement. Please ignore my earlier rambling.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14/08/15 13:45, Zhou, Jammy wrote:
> That's okay. Shall we get this patch merged now if no other objections?
> 
First we should fix the funny vendor/device/etc id issue. Otherwise the
API is bugged badly.

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

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

Oh, I really missed that. I will get it resolved next week.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, August 14, 2015 9:19 PM
To: Zhou, Jammy
Cc: emil.l.velikov@gmail.com; ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 14/08/15 13:45, Zhou, Jammy wrote:
> That's okay. Shall we get this patch merged now if no other objections?
> 
First we should fix the funny vendor/device/etc id issue. Otherwise the API is bugged badly.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Daniel Vetter]

On Thu, Aug 13, 2015 at 11:33:41AM +0800, Jammy Zhou wrote:
> From: Emil Velikov <emil.l.velikov@gmail.com>
> 
> For mutiple GPU support, the devices on the system should be enumerated
> to get necessary information about each device, and the drmGetDevices
> interface is added for this. Currently only PCI devices are supported for
> the enumeration.
> 
> Typical usage:
> int count;
> drmDevicePtr *foo;
> count = drmGetDevices(NULL, 0);
> foo = calloc(count, sizeof(drmDevicePtr));
> count = drmGetDevices(foo, count);
> /* find proper device, open correct device node, etc */
> drmFreeDevices(foo, count);
> free(foo);
> 
> v2: change according to feedback from Emil
> 
> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
> ---
>  xf86drm.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  xf86drm.h |  34 ++++++
>  2 files changed, 385 insertions(+)
> 
> diff --git a/xf86drm.c b/xf86drm.c
> index 5e02969..237663b 100644
> --- a/xf86drm.c
> +++ b/xf86drm.c
> @@ -55,6 +55,7 @@
>  #ifdef HAVE_SYS_MKDEV_H
>  # include <sys/mkdev.h> /* defines major(), minor(), and makedev() on Solaris */
>  #endif
> +#include <math.h>
>  
>  /* Not all systems have MAP_FAILED defined */
>  #ifndef MAP_FAILED
> @@ -2829,3 +2830,353 @@ char *drmGetRenderDeviceNameFromFd(int fd)
>  {
>  	return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
>  }
> +
> +#ifdef __linux__
> +static int drmParseSubsystemType(const char *str)
> +{
> +    char link[PATH_MAX + 1] = "";
> +    char *name;
> +
> +    if (readlink(str, link, PATH_MAX) < 0)
> +        return -EINVAL;
> +
> +    name = strrchr(link, '/');
> +    if (!name)
> +        return -EINVAL;
> +
> +    name++;
> +
> +    if (strncmp(name, "pci", 3) == 0)
> +        return DRM_BUS_PCI;
> +
> +    return -EINVAL;
> +}
> +
> +static int drmParsePciBusInfo(const char *str, drmPciBusInfoPtr info)
> +{
> +    int domain, bus, dev, func;
> +    char *value;
> +
> +    if (str == NULL)
> +        return -EINVAL;
> +
> +    value = strstr(str, "PCI_SLOT_NAME=");
> +    if (value == NULL)
> +        return -EINVAL;
> +
> +    value += strlen("PCI_SLOT_NAME=");
> +
> +    if (sscanf(value, "%04x:%02x:%02x.%1u",
> +               &domain, &bus, &dev, &func) != 4)
> +        return -EINVAL;
> +
> +    info->domain = domain;
> +    info->bus = bus;
> +    info->dev = dev;
> +    info->func = func;
> +
> +    return 0;
> +}
> +
> +static int drmSameDevice(drmDevicePtr a, drmDevicePtr b)
> +{
> +    if (a->bustype != b->bustype)
> +        return 0;
> +
> +    switch (a->bustype) {
> +    case DRM_BUS_PCI:
> +        if (memcmp(a->businfo.pci, b->businfo.pci, sizeof(drmPciBusInfo)) == 0)
> +            return 1;
> +    default:
> +        break;
> +    }
> +
> +    return 0;
> +}
> +
> +static int drmGetNodeType(const char *name)
> +{
> +    if (strncmp(name, DRM_PRIMARY_MINOR_NAME,
> +        sizeof(DRM_PRIMARY_MINOR_NAME) - 1) == 0)
> +        return DRM_NODE_PRIMARY;
> +
> +    if (strncmp(name, DRM_CONTROL_MINOR_NAME,
> +        sizeof(DRM_CONTROL_MINOR_NAME ) - 1) == 0)
> +        return DRM_NODE_CONTROL;
> +
> +    if (strncmp(name, DRM_RENDER_MINOR_NAME,
> +        sizeof(DRM_RENDER_MINOR_NAME) - 1) == 0)
> +        return DRM_NODE_RENDER;
> +
> +    return -EINVAL;
> +}
> +
> +static int drmParsePciDeviceInfo(const char *config,
> +                                 drmPciDeviceInfoPtr device)
> +{
> +    if (config == NULL)
> +        return -EINVAL;
> +
> +    device->vendor_id = config[0] | (config[1] << 8);
> +    device->device_id = config[2] | (config[3] << 8);
> +    device->revision_id = config[8];
> +    device->subvendor_id = config[44] | (config[45] << 8);
> +    device->subdevice_id = config[46] | (config[47] << 8);

Why not reuse libpciaccess?
-Daniel

> +
> +    return 0;
> +}
> +
> +static void drmFreeDevice(drmDevicePtr device)
> +{
> +    int i;
> +
> +    if (device == NULL)
> +        return;
> +
> +    if (device->nodes != NULL)
> +        for (i = 0; i < DRM_NODE_MAX; i++)
> +            free(device->nodes[i]);
> +
> +    free(device->nodes);
> +    free(device->businfo.pci);
> +    free(device->deviceinfo.pci);
> +}
> +
> +void drmFreeDevices(drmDevicePtr devices[], int count)
> +{
> +    int i;
> +
> +    if (devices == NULL)
> +        return;
> +
> +    for (i = 0; i < count; i++) {
> +        drmFreeDevice(devices[i]);
> +        free(devices[i]);
> +        devices[i] = NULL;
> +    }
> +}
> +
> +/**
> + * Get drm devices on the system
> + *
> + * \param devices the array of devices with drmDevicePtr elements
> + *                can be NULL to get the device number first
> + * \param max_devices the maximum number of devices for the array
> + *
> + * \return on error - negative error code,
> + *         if devices is NULL - total number of devices available on the system,
> + *         alternatively the number of devices stored in devices[], which is
> + *         capped by the max_devices.
> + */
> +int drmGetDevices(drmDevicePtr devices[], int max_devices)
> +{
> +    drmDevicePtr devs = NULL;
> +    drmPciBusInfoPtr pcibus = NULL;
> +    drmPciDeviceInfoPtr pcidevice = NULL;
> +    DIR *sysdir = NULL;
> +    struct dirent *dent = NULL;
> +    struct stat sbuf = {0};
> +    char node[PATH_MAX + 1] = "";
> +    char path[PATH_MAX + 1] = "";
> +    char data[128] = "";
> +    char config[64] = "";
> +    int node_type, subsystem_type;
> +    int maj, min;
> +    int fd;
> +    int ret, i = 0, j, node_count, device_count = 0;
> +    int max_count = 16;
> +    int *duplicated = NULL;
> +
> +    devs = calloc(max_count, sizeof(*devs));
> +    if (devs == NULL)
> +        return -ENOMEM;
> +
> +    sysdir = opendir(DRM_DIR_NAME);
> +    if (!sysdir) {
> +        ret = -errno;
> +        goto free_locals;
> +    }
> +
> +    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;
> +
> +        snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/subsystem",
> +                 maj, min);
> +        subsystem_type = drmParseSubsystemType(path);
> +
> +        if (subsystem_type < 0)
> +            continue;
> +
> +        switch (subsystem_type) {
> +        case DRM_BUS_PCI:
> +            pcibus = calloc(1, sizeof(*pcibus));
> +            if (pcibus == NULL) {
> +                ret = -ENOMEM;
> +                goto free_locals;
> +            }
> +
> +            snprintf(path, PATH_MAX, "/sys/dev/char/%d:%d/device/uevent",
> +                     maj, min);
> +            fd = open(path, O_RDONLY);
> +            if (fd < 0) {
> +                ret = -errno;
> +                goto free_locals;
> +            }
> +            ret = read(fd, data, sizeof(data));
> +            if (ret < 0) {
> +                ret = -errno;
> +                close(fd);
> +                goto free_locals;
> +            }
> +
> +            ret = drmParsePciBusInfo(data, pcibus);
> +            close(fd);
> +            if (ret)
> +                goto free_locals;
> +
> +            if (i >= max_count) {
> +                max_count += 16;
> +                devs = realloc(devs, max_count * sizeof(*devs));
> +            }
> +
> +            devs[i].businfo.pci = pcibus;
> +            devs[i].bustype = subsystem_type;
> +            devs[i].nodes = calloc(DRM_NODE_MAX, sizeof(char *));
> +            if (devs[i].nodes == NULL) {
> +                ret = -ENOMEM;
> +                goto free_locals;
> +            }
> +            devs[i].nodes[node_type] = strdup(node);
> +            if (devs[i].nodes[node_type] == NULL) {
> +                ret = -ENOMEM;
> +                goto free_locals;
> +            }
> +            devs[i].available_nodes = 1 << node_type;
> +
> +            if (devices != NULL) {
> +                snprintf(path, PATH_MAX, "/sys/class/drm/%s/device/config",
> +                         dent->d_name);
> +                fd = open(path, O_RDONLY);
> +                if (fd < 0) {
> +                     ret = -errno;
> +                     goto free_locals;
> +                }
> +                ret = read(fd, config, 64);
> +                if (ret < 0) {
> +                    ret = -errno;
> +                    close(fd);
> +                    goto free_locals;
> +                }
> +
> +                pcidevice = calloc(1, sizeof(*pcidevice));
> +                if (pcidevice == NULL) {
> +                    ret = -ENOMEM;
> +                    goto free_locals;
> +                }
> +
> +                ret = drmParsePciDeviceInfo(config, pcidevice);
> +                if (ret)
> +                    goto free_locals;
> +
> +                devs[i].deviceinfo.pci = pcidevice;
> +                close(fd);
> +            }
> +            break;
> +        default:
> +            fprintf(stderr, "The subsystem type is not supported yet\n");
> +            break;
> +        }
> +        i++;
> +    }
> +
> +    node_count = i;
> +
> +    /* merge duplicated devices with same domain/bus/device/func IDs */
> +    duplicated = calloc(node_count, sizeof(*duplicated));
> +    if (duplicated == NULL) {
> +        ret = -ENOMEM;
> +        goto free_locals;
> +    }
> +
> +    for (i = 0; i < node_count; i++) {
> +        for (j = i+1; j < node_count; j++) {
> +            if (duplicated[i] || duplicated[j])
> +                continue;
> +            if (drmSameDevice(&devs[i], &devs[j])) {
> +                duplicated[j] = 1;
> +                devs[i].available_nodes |= devs[j].available_nodes;
> +                node_type = log2(devs[j].available_nodes);
> +                devs[i].nodes[node_type] = devs[j].nodes[node_type];
> +                free(devs[j].nodes);
> +                free(devs[j].businfo.pci);
> +                free(devs[j].deviceinfo.pci);
> +            }
> +        }
> +    }
> +
> +    for (i = 0; i < node_count; i++) {
> +        if(duplicated[i] == 0) {
> +            if ((devices != NULL) && (device_count < max_devices)) {
> +                devices[device_count] = calloc(1, sizeof(drmDevice));
> +                if (devices[device_count] == NULL) {
> +                    ret = -ENOMEM;
> +                    break;
> +                }
> +                memcpy(devices[device_count], &devs[i], sizeof(drmDevice));
> +            } else
> +                drmFreeDevice(&devs[i]);
> +            device_count++;
> +        }
> +    }
> +
> +    if (i < node_count) {
> +        drmFreeDevices(devices, device_count);
> +        for ( ; i < node_count; i++)
> +            if(duplicated[i] == 0)
> +                drmFreeDevice(&devs[i]);
> +    } else
> +        ret = device_count;
> +
> +    free(duplicated);
> +    free(devs);
> +    closedir(sysdir);
> +    return ret;
> +
> +free_locals:
> +    for (j = 0; j < i; j++)
> +        drmFreeDevice(&devs[j]);
> +    free(pcidevice);
> +    free(pcibus);
> +    free(devs);
> +    closedir(sysdir);
> +    return ret;
> +}
> +#else
> +void drmFreeDevices(drmDevicePtr devices[], int count)
> +{
> +    (void)devices;
> +    (void)count;
> +}
> +
> +int drmGetDevices(drmDevicePtr devices[], int max_devices)
> +{
> +    (void)devices;
> +    (void)max_devices;
> +    return -EINVAL;
> +}
> +
> +#warning "Missing implementation of drmGetDevices/drmFreeDevices"
> +
> +#endif
> diff --git a/xf86drm.h b/xf86drm.h
> index 360e04a..e82ca84 100644
> --- a/xf86drm.h
> +++ b/xf86drm.h
> @@ -563,6 +563,8 @@ extern int           drmOpen(const char *name, const char *busid);
>  #define DRM_NODE_PRIMARY 0
>  #define DRM_NODE_CONTROL 1
>  #define DRM_NODE_RENDER  2
> +#define DRM_NODE_MAX     3
> +
>  extern int           drmOpenWithType(const char *name, const char *busid,
>                                       int type);
>  
> @@ -759,6 +761,38 @@ 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
> +
> +typedef struct _drmPciBusInfo {
> +    uint16_t domain;
> +    uint8_t bus;
> +    uint8_t dev;
> +    uint8_t func;
> +} drmPciBusInfo, *drmPciBusInfoPtr;
> +
> +typedef struct _drmPciDeviceInfo {
> +    uint16_t vendor_id;
> +    uint16_t device_id;
> +    uint16_t subvendor_id;
> +    uint16_t subdevice_id;
> +    uint8_t revision_id;
> +} drmPciDeviceInfo, *drmPciDeviceInfoPtr;
> +
> +typedef struct _drmDevice {
> +    char **nodes; /* DRM_NODE_MAX sized array */
> +    int available_nodes; /* DRM_NODE_* bitmask */
> +    int bustype;
> +    union {
> +        drmPciBusInfoPtr pci;
> +    } businfo;
> +    union {
> +        drmPciDeviceInfoPtr pci;
> +    } deviceinfo;
> +} drmDevice, *drmDevicePtr;
> +
> +extern int drmGetDevices(drmDevicePtr devices[], int max_devices);
> +extern void drmFreeDevices(drmDevicePtr devices[], int count);
> +
>  #if defined(__cplusplus)
>  }
>  #endif
> -- 
> 1.9.1
> 
> _______________________________________________
> dri-devel mailing list
> dri-devel@lists.freedesktop.org
> http://lists.freedesktop.org/mailman/listinfo/dri-devel

-- 
Daniel Vetter
Software Engineer, Intel Corporation
http://blog.ffwll.ch
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 13 August 2015 at 16:07, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 13, 2015 at 11:33:41AM +0800, Jammy Zhou wrote:
>> From: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> For mutiple GPU support, the devices on the system should be enumerated
>> to get necessary information about each device, and the drmGetDevices
>> interface is added for this. Currently only PCI devices are supported for
>> the enumeration.
>>
>> Typical usage:
>> int count;
>> drmDevicePtr *foo;
>> count = drmGetDevices(NULL, 0);
>> foo = calloc(count, sizeof(drmDevicePtr));
>> count = drmGetDevices(foo, count);
>> /* find proper device, open correct device node, etc */
>> drmFreeDevices(foo, count);
>> free(foo);
>>
>> v2: change according to feedback from Emil
>>
>> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
>> ---
>>  xf86drm.c | 351 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  xf86drm.h |  34 ++++++
>>  2 files changed, 385 insertions(+)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 5e02969..237663b 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -55,6 +55,7 @@
>>  #ifdef HAVE_SYS_MKDEV_H
>>  # include <sys/mkdev.h> /* defines major(), minor(), and makedev() on Solaris */
>>  #endif
>> +#include <math.h>
>>
>>  /* Not all systems have MAP_FAILED defined */
>>  #ifndef MAP_FAILED
>> @@ -2829,3 +2830,353 @@ char *drmGetRenderDeviceNameFromFd(int fd)
>>  {
>>       return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);
>>  }
>> +
>> +#ifdef __linux__
>> +static int drmParseSubsystemType(const char *str)
>> +{
>> +    char link[PATH_MAX + 1] = "";
>> +    char *name;
>> +
>> +    if (readlink(str, link, PATH_MAX) < 0)
>> +        return -EINVAL;
>> +
>> +    name = strrchr(link, '/');
>> +    if (!name)
>> +        return -EINVAL;
>> +
>> +    name++;
>> +
>> +    if (strncmp(name, "pci", 3) == 0)
>> +        return DRM_BUS_PCI;
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int drmParsePciBusInfo(const char *str, drmPciBusInfoPtr info)
>> +{
>> +    int domain, bus, dev, func;
>> +    char *value;
>> +
>> +    if (str == NULL)
>> +        return -EINVAL;
>> +
>> +    value = strstr(str, "PCI_SLOT_NAME=");
>> +    if (value == NULL)
>> +        return -EINVAL;
>> +
>> +    value += strlen("PCI_SLOT_NAME=");
>> +
>> +    if (sscanf(value, "%04x:%02x:%02x.%1u",
>> +               &domain, &bus, &dev, &func) != 4)
>> +        return -EINVAL;
>> +
>> +    info->domain = domain;
>> +    info->bus = bus;
>> +    info->dev = dev;
>> +    info->func = func;
>> +
>> +    return 0;
>> +}
>> +
>> +static int drmSameDevice(drmDevicePtr a, drmDevicePtr b)
>> +{
>> +    if (a->bustype != b->bustype)
>> +        return 0;
>> +
>> +    switch (a->bustype) {
>> +    case DRM_BUS_PCI:
>> +        if (memcmp(a->businfo.pci, b->businfo.pci, sizeof(drmPciBusInfo)) == 0)
>> +            return 1;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int drmGetNodeType(const char *name)
>> +{
>> +    if (strncmp(name, DRM_PRIMARY_MINOR_NAME,
>> +        sizeof(DRM_PRIMARY_MINOR_NAME) - 1) == 0)
>> +        return DRM_NODE_PRIMARY;
>> +
>> +    if (strncmp(name, DRM_CONTROL_MINOR_NAME,
>> +        sizeof(DRM_CONTROL_MINOR_NAME ) - 1) == 0)
>> +        return DRM_NODE_CONTROL;
>> +
>> +    if (strncmp(name, DRM_RENDER_MINOR_NAME,
>> +        sizeof(DRM_RENDER_MINOR_NAME) - 1) == 0)
>> +        return DRM_NODE_RENDER;
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int drmParsePciDeviceInfo(const char *config,
>> +                                 drmPciDeviceInfoPtr device)
>> +{
>> +    if (config == NULL)
>> +        return -EINVAL;
>> +
>> +    device->vendor_id = config[0] | (config[1] << 8);
>> +    device->device_id = config[2] | (config[3] << 8);
>> +    device->revision_id = config[8];
>> +    device->subvendor_id = config[44] | (config[45] << 8);
>> +    device->subdevice_id = config[46] | (config[47] << 8);
>
> Why not reuse libpciaccess?
I fully agree that the implementation is not pretty, although...

Adding dependencies for optional new features doesn't seem too
appealing, either. It will also save us some headaches when someone
starts shipping libpciaccess.so with their binary game/program (just
like libudev is today).

Would be great if we can use libudev but... just not yet.

If you have any other ideas/suggestions please fire away.

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

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Thursday, August 13, 2015 11:27 PM
To: Daniel Vetter
Cc: Zhou, Jammy; ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 13 August 2015 at 16:07, Daniel Vetter <daniel@ffwll.ch> wrote:
> On Thu, Aug 13, 2015 at 11:33:41AM +0800, Jammy Zhou wrote:
>> From: Emil Velikov <emil.l.velikov@gmail.com>
>>
>> For mutiple GPU support, the devices on the system should be 
>> enumerated to get necessary information about each device, and the 
>> drmGetDevices interface is added for this. Currently only PCI devices 
>> are supported for the enumeration.
>>
>> Typical usage:
>> int count;
>> drmDevicePtr *foo;
>> count = drmGetDevices(NULL, 0);
>> foo = calloc(count, sizeof(drmDevicePtr)); count = drmGetDevices(foo, 
>> count);
>> /* find proper device, open correct device node, etc */ 
>> drmFreeDevices(foo, count); free(foo);
>>
>> v2: change according to feedback from Emil
>>
>> Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
>> ---
>>  xf86drm.c | 351 
>> ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  xf86drm.h |  34 ++++++
>>  2 files changed, 385 insertions(+)
>>
>> diff --git a/xf86drm.c b/xf86drm.c
>> index 5e02969..237663b 100644
>> --- a/xf86drm.c
>> +++ b/xf86drm.c
>> @@ -55,6 +55,7 @@
>>  #ifdef HAVE_SYS_MKDEV_H
>>  # include <sys/mkdev.h> /* defines major(), minor(), and makedev() 
>> on Solaris */  #endif
>> +#include <math.h>
>>
>>  /* Not all systems have MAP_FAILED defined */  #ifndef MAP_FAILED @@ 
>> -2829,3 +2830,353 @@ char *drmGetRenderDeviceNameFromFd(int fd)  {
>>       return drmGetMinorNameForFD(fd, DRM_NODE_RENDER);  }
>> +
>> +#ifdef __linux__
>> +static int drmParseSubsystemType(const char *str) {
>> +    char link[PATH_MAX + 1] = "";
>> +    char *name;
>> +
>> +    if (readlink(str, link, PATH_MAX) < 0)
>> +        return -EINVAL;
>> +
>> +    name = strrchr(link, '/');
>> +    if (!name)
>> +        return -EINVAL;
>> +
>> +    name++;
>> +
>> +    if (strncmp(name, "pci", 3) == 0)
>> +        return DRM_BUS_PCI;
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int drmParsePciBusInfo(const char *str, drmPciBusInfoPtr 
>> +info) {
>> +    int domain, bus, dev, func;
>> +    char *value;
>> +
>> +    if (str == NULL)
>> +        return -EINVAL;
>> +
>> +    value = strstr(str, "PCI_SLOT_NAME=");
>> +    if (value == NULL)
>> +        return -EINVAL;
>> +
>> +    value += strlen("PCI_SLOT_NAME=");
>> +
>> +    if (sscanf(value, "%04x:%02x:%02x.%1u",
>> +               &domain, &bus, &dev, &func) != 4)
>> +        return -EINVAL;
>> +
>> +    info->domain = domain;
>> +    info->bus = bus;
>> +    info->dev = dev;
>> +    info->func = func;
>> +
>> +    return 0;
>> +}
>> +
>> +static int drmSameDevice(drmDevicePtr a, drmDevicePtr b) {
>> +    if (a->bustype != b->bustype)
>> +        return 0;
>> +
>> +    switch (a->bustype) {
>> +    case DRM_BUS_PCI:
>> +        if (memcmp(a->businfo.pci, b->businfo.pci, sizeof(drmPciBusInfo)) == 0)
>> +            return 1;
>> +    default:
>> +        break;
>> +    }
>> +
>> +    return 0;
>> +}
>> +
>> +static int drmGetNodeType(const char *name) {
>> +    if (strncmp(name, DRM_PRIMARY_MINOR_NAME,
>> +        sizeof(DRM_PRIMARY_MINOR_NAME) - 1) == 0)
>> +        return DRM_NODE_PRIMARY;
>> +
>> +    if (strncmp(name, DRM_CONTROL_MINOR_NAME,
>> +        sizeof(DRM_CONTROL_MINOR_NAME ) - 1) == 0)
>> +        return DRM_NODE_CONTROL;
>> +
>> +    if (strncmp(name, DRM_RENDER_MINOR_NAME,
>> +        sizeof(DRM_RENDER_MINOR_NAME) - 1) == 0)
>> +        return DRM_NODE_RENDER;
>> +
>> +    return -EINVAL;
>> +}
>> +
>> +static int drmParsePciDeviceInfo(const char *config,
>> +                                 drmPciDeviceInfoPtr device) {
>> +    if (config == NULL)
>> +        return -EINVAL;
>> +
>> +    device->vendor_id = config[0] | (config[1] << 8);
>> +    device->device_id = config[2] | (config[3] << 8);
>> +    device->revision_id = config[8];
>> +    device->subvendor_id = config[44] | (config[45] << 8);
>> +    device->subdevice_id = config[46] | (config[47] << 8);
>
> Why not reuse libpciaccess?
I fully agree that the implementation is not pretty, although...

Adding dependencies for optional new features doesn't seem too appealing, either. It will also save us some headaches when someone starts shipping libpciaccess.so with their binary game/program (just like libudev is today).

Would be great if we can use libudev but... just not yet.

If you have any other ideas/suggestions please fire away.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Kai Wasserbäch]

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

Zhou, Jammy wrote on 14.08.2015 07:59:
> We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.

The reason sounds wrong. There was a similar discussion over at Mesa. I think
you (as in hardware/driver vendors like AMD/Intel/Nvidia) need to push Valve (or
the game devs through Valve or directly) to fix their setup. Steam runtime is
fine and all, but please only pre-load it, if needed (ie. library foo is missing
on the system and can't be installed through the package manager). IIRC the
VMWare guys said in the Mesa discussion, they have a script in place for their
virtualisation products, that checks whether a library needs to be loaded from
their "baseline directory" or from the system.

Working around a bug/design flaw in Steam's Linux version doesn't sound like a
supportable solution in the long run. As long as you let them get away with
that, you will face this problem over and over with different libraries. (For me
it's usually libstdc++ (needed by LLVM), libncurses and a few X(CB) libraries I
need to remove from Steam, before anything works. Though I do have script for
that, that I can run after every upgrade, this is not a solution for everyone.)

Cheers,
Kai


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

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

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14 August 2015 at 08:59, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
> Zhou, Jammy wrote on 14.08.2015 07:59:
>> We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.
>
> The reason sounds wrong. There was a similar discussion over at Mesa. I think
> you (as in hardware/driver vendors like AMD/Intel/Nvidia) need to push Valve (or
> the game devs through Valve or directly) to fix their setup. Steam runtime is
> fine and all, but please only pre-load it, if needed (ie. library foo is missing
> on the system and can't be installed through the package manager). IIRC the
> VMWare guys said in the Mesa discussion, they have a script in place for their
> virtualisation products, that checks whether a library needs to be loaded from
> their "baseline directory" or from the system.
>
> Working around a bug/design flaw in Steam's Linux version doesn't sound like a
> supportable solution in the long run. As long as you let them get away with
> that, you will face this problem over and over with different libraries. (For me
> it's usually libstdc++ (needed by LLVM), libncurses and a few X(CB) libraries I
> need to remove from Steam, before anything works. Though I do have script for
> that, that I can run after every upgrade, this is not a solution for everyone.)
>
Helping and applying pressure to resolve the issue is the way to go.
But until that is resolved it's great to have a solution that does not
lead to a crash. It feels rude towards you and other users to
deliberately use the problematic combo and expect from you to remove
libfoo.so.

When things get sorted out, we can easily replace this (a tad ugly
implementation) with libudev.

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Kai Wasserbäch]

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

Emil Velikov wrote on 14.08.2015 10:17:
> On 14 August 2015 at 08:59, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
>> Zhou, Jammy wrote on 14.08.2015 07:59:
>>> We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.
>>
>> The reason sounds wrong. There was a similar discussion over at Mesa. I think
>> you (as in hardware/driver vendors like AMD/Intel/Nvidia) need to push Valve (or
>> the game devs through Valve or directly) to fix their setup. Steam runtime is
>> fine and all, but please only pre-load it, if needed (ie. library foo is missing
>> on the system and can't be installed through the package manager). IIRC the
>> VMWare guys said in the Mesa discussion, they have a script in place for their
>> virtualisation products, that checks whether a library needs to be loaded from
>> their "baseline directory" or from the system.
>>
>> Working around a bug/design flaw in Steam's Linux version doesn't sound like a
>> supportable solution in the long run. As long as you let them get away with
>> that, you will face this problem over and over with different libraries. (For me
>> it's usually libstdc++ (needed by LLVM), libncurses and a few X(CB) libraries I
>> need to remove from Steam, before anything works. Though I do have script for
>> that, that I can run after every upgrade, this is not a solution for everyone.)
>>
> Helping and applying pressure to resolve the issue is the way to go.
> But until that is resolved it's great to have a solution that does not
> lead to a crash. It feels rude towards you and other users to
> deliberately use the problematic combo and expect from you to remove
> libfoo.so.

Well, I'd rather remove stuff from Steam's runtime than burden you and other
developers with maintaing code that is unnecessrily ugly. (Though that's
obviously just my opinion.)

> When things get sorted out, we can easily replace this (a tad ugly
> implementation) with libudev.

As long as you allow this behaviour by working around it, I don't see Valve/game
developers "invest" in a real solution (because it works now). Businesses
usually only move from a position, when there's outside pressure and a clear
advantage to do so (here: no bug reports about crashing games).

Anyway, this was just my two cents and you can obviously decide in any way you
deem to be the best.


[-- Attachment #1.2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 630 bytes --]

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

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

Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Emil Velikov]

On 14 August 2015 at 09:26, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
> Emil Velikov wrote on 14.08.2015 10:17:
>> On 14 August 2015 at 08:59, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
>>> Zhou, Jammy wrote on 14.08.2015 07:59:
>>>> We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.
>>>
>>> The reason sounds wrong. There was a similar discussion over at Mesa. I think
>>> you (as in hardware/driver vendors like AMD/Intel/Nvidia) need to push Valve (or
>>> the game devs through Valve or directly) to fix their setup. Steam runtime is
>>> fine and all, but please only pre-load it, if needed (ie. library foo is missing
>>> on the system and can't be installed through the package manager). IIRC the
>>> VMWare guys said in the Mesa discussion, they have a script in place for their
>>> virtualisation products, that checks whether a library needs to be loaded from
>>> their "baseline directory" or from the system.
>>>
>>> Working around a bug/design flaw in Steam's Linux version doesn't sound like a
>>> supportable solution in the long run. As long as you let them get away with
>>> that, you will face this problem over and over with different libraries. (For me
>>> it's usually libstdc++ (needed by LLVM), libncurses and a few X(CB) libraries I
>>> need to remove from Steam, before anything works. Though I do have script for
>>> that, that I can run after every upgrade, this is not a solution for everyone.)
>>>
>> Helping and applying pressure to resolve the issue is the way to go.
>> But until that is resolved it's great to have a solution that does not
>> lead to a crash. It feels rude towards you and other users to
>> deliberately use the problematic combo and expect from you to remove
>> libfoo.so.
>
> Well, I'd rather remove stuff from Steam's runtime than burden you and other
> developers with maintaing code that is unnecessrily ugly. (Though that's
> obviously just my opinion.)
>
>> When things get sorted out, we can easily replace this (a tad ugly
>> implementation) with libudev.
>
> As long as you allow this behaviour by working around it,
There is a saying (roughly translated to) "The wiser man always steps
back". Or we could/should be like Linus - "F* you $company"

> I don't see Valve/game
> developers "invest" in a real solution (because it works now). Businesses
> usually only move from a position, when there's outside pressure and a clear
> advantage to do so (here: no bug reports about crashing games).
>
There have been plenty of reports opened wrt
libudev/libgcc_s/libstdc++ on their trackers and seemingly limited
interest to fix things.

This is a catch 20/20 afaics. "FOSS drivers do not work thus they are
s**t" is how a sizeable hunk of people think. They rarely consider
what the actual issue might be, because "I installed the nvidia/amd
proprietary drivers and things work now".

> Anyway, this was just my two cents and you can obviously decide in any way you
> deem to be the best.
>
Personally, I'd love if there was no "options" and we can just use
libfoo. Who knows Valve devs might get a wake up call and fix the
problem ?


-Emil
P.S. Fun fact: Valve's annual "TI" Dota2 tournament managed to
accumulate some 66 million USD gross income, over 100 days.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 1/5] drm: add interface to get drm devices on the system v2

[Zhou, Jammy]

> There have been plenty of reports opened wrt libudev/libgcc_s/libstdc++ on their trackers and seemingly limited interest to fix things.

Yes, there are a bunch of issues reported for this already. But it looks like Valve has no plan to fix these issues. For example,
https://github.com/ValveSoftware/steam-runtime/issues/13

IMHO, we can probably use sysfs first, and when the issue is solved by Valve, we can switch to the udev solution later.

Regards,
Jammy

-----Original Message-----
From: Emil Velikov [mailto:emil.l.velikov@gmail.com] 
Sent: Friday, August 14, 2015 5:08 PM
To: Kai Wasserbäch
Cc: Zhou, Jammy; Daniel Vetter; ML dri-devel
Subject: Re: [PATCH 1/5] drm: add interface to get drm devices on the system v2

On 14 August 2015 at 09:26, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
> Emil Velikov wrote on 14.08.2015 10:17:
>> On 14 August 2015 at 08:59, Kai Wasserbäch <kai@dev.carbon-project.org> wrote:
>>> Zhou, Jammy wrote on 14.08.2015 07:59:
>>>> We tried several different ways already for the enumeration interface (libpciaccess, libudev, etc). But we ran into some problems with these options for example when run Steam games which ships 32bit libraries (including libudev) in the steam runtime, so finally we decided to use sysfs directly to avoid introducing some additional dependencies into libdrm.
>>>
>>> The reason sounds wrong. There was a similar discussion over at 
>>> Mesa. I think you (as in hardware/driver vendors like 
>>> AMD/Intel/Nvidia) need to push Valve (or the game devs through Valve 
>>> or directly) to fix their setup. Steam runtime is fine and all, but 
>>> please only pre-load it, if needed (ie. library foo is missing on 
>>> the system and can't be installed through the package manager). IIRC 
>>> the VMWare guys said in the Mesa discussion, they have a script in 
>>> place for their virtualisation products, that checks whether a library needs to be loaded from their "baseline directory" or from the system.
>>>
>>> Working around a bug/design flaw in Steam's Linux version doesn't 
>>> sound like a supportable solution in the long run. As long as you 
>>> let them get away with that, you will face this problem over and 
>>> over with different libraries. (For me it's usually libstdc++ 
>>> (needed by LLVM), libncurses and a few X(CB) libraries I need to 
>>> remove from Steam, before anything works. Though I do have script 
>>> for that, that I can run after every upgrade, this is not a solution 
>>> for everyone.)
>>>
>> Helping and applying pressure to resolve the issue is the way to go.
>> But until that is resolved it's great to have a solution that does 
>> not lead to a crash. It feels rude towards you and other users to 
>> deliberately use the problematic combo and expect from you to remove 
>> libfoo.so.
>
> Well, I'd rather remove stuff from Steam's runtime than burden you and 
> other developers with maintaing code that is unnecessrily ugly. 
> (Though that's obviously just my opinion.)
>
>> When things get sorted out, we can easily replace this (a tad ugly
>> implementation) with libudev.
>
> As long as you allow this behaviour by working around it,
There is a saying (roughly translated to) "The wiser man always steps back". Or we could/should be like Linus - "F* you $company"

> I don't see Valve/game
> developers "invest" in a real solution (because it works now). 
> Businesses usually only move from a position, when there's outside 
> pressure and a clear advantage to do so (here: no bug reports about crashing games).
>
There have been plenty of reports opened wrt libudev/libgcc_s/libstdc++ on their trackers and seemingly limited interest to fix things.

This is a catch 20/20 afaics. "FOSS drivers do not work thus they are s**t" is how a sizeable hunk of people think. They rarely consider what the actual issue might be, because "I installed the nvidia/amd proprietary drivers and things work now".

> Anyway, this was just my two cents and you can obviously decide in any 
> way you deem to be the best.
>
Personally, I'd love if there was no "options" and we can just use libfoo. Who knows Valve devs might get a wake up call and fix the problem ?


-Emil
P.S. Fun fact: Valve's annual "TI" Dota2 tournament managed to accumulate some 66 million USD gross income, over 100 days.
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

[PATCH 2/5] amdgpu: improve amdgpu_vamgr_init

[Jammy Zhou]

Make it a generic function independent of the device info.

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 amdgpu/amdgpu_vamgr.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index cc4a1c4..71fd2b1 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -46,11 +46,12 @@ int amdgpu_va_range_query(amdgpu_device_handle dev,
 	return -EINVAL;
 }
 
-static void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, struct amdgpu_device *dev)
+static void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
+			      uint64_t max, uint64_t alignment)
 {
-	mgr->va_offset = dev->dev_info.virtual_address_offset;
-	mgr->va_max = dev->dev_info.virtual_address_max;
-	mgr->va_alignment = dev->dev_info.virtual_address_alignment;
+	mgr->va_offset = start;
+	mgr->va_max = max;
+	mgr->va_alignment = alignment;
 
 	list_inithead(&mgr->va_holes);
 	pthread_mutex_init(&mgr->bo_va_mutex, NULL);
@@ -72,7 +73,9 @@ drm_private struct amdgpu_bo_va_mgr * amdgpu_vamgr_get_global(struct amdgpu_devi
 	ref = atomic_inc_return(&vamgr.refcount);
 
 	if (ref == 1)
-		amdgpu_vamgr_init(&vamgr, dev);
+		amdgpu_vamgr_init(&vamgr, dev->dev_info.virtual_address_offset,
+				  dev->dev_info.virtual_address_max,
+				  dev->dev_info.virtual_address_alignment);
 	return &vamgr;
 }
 
-- 
1.9.1

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

[PATCH 3/5] amdgpu: add flag to support 32bit VA address v3

[Jammy Zhou]

The AMDGPU_VA_RANGE_32_BIT flag is added to request VA range in the
32bit address space for amdgpu_va_range_alloc.

The 32bit address space is reserved at initialization time, and managed
with a separate VAMGR as part of the global VAMGR. And if no enough VA
space available in range above 4GB, this reserved range can be used as
fallback.

v2: add comment for AMDGPU_VA_RANGE_32_BIT, and add vamgr to va_range
v3: rebase to Emil's drm_private series

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 amdgpu/amdgpu.h          |  5 +++++
 amdgpu/amdgpu_device.c   | 20 ++++++++++++++++++++
 amdgpu/amdgpu_internal.h |  9 +++++++++
 amdgpu/amdgpu_vamgr.c    | 34 +++++++++++++++++++++++++++-------
 4 files changed, 61 insertions(+), 7 deletions(-)

diff --git a/amdgpu/amdgpu.h b/amdgpu/amdgpu.h
index a90c1ac..1e633e3 100644
--- a/amdgpu/amdgpu.h
+++ b/amdgpu/amdgpu.h
@@ -1075,6 +1075,11 @@ int amdgpu_read_mm_registers(amdgpu_device_handle dev, unsigned dword_offset,
 			     uint32_t *values);
 
 /**
+ * Flag to request VA address range in the 32bit address space
+*/
+#define AMDGPU_VA_RANGE_32_BIT		0x1
+
+/**
  * Allocate virtual address range
  *
  * \param dev - [in] Device handle. See #amdgpu_device_initialize()
diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index b977847..0ef1d31 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -44,6 +44,7 @@
 #include "amdgpu_drm.h"
 #include "amdgpu_internal.h"
 #include "util_hash_table.h"
+#include "util_math.h"
 
 #define PTR_TO_UINT(x) ((unsigned)((intptr_t)(x)))
 #define UINT_TO_PTR(x) ((void *)((intptr_t)(x)))
@@ -174,6 +175,7 @@ int amdgpu_device_initialize(int fd,
 	int flag_auth = 0;
 	int flag_authexist=0;
 	uint32_t accel_working = 0;
+	uint64_t start, max;
 
 	*device_handle = NULL;
 
@@ -252,6 +254,19 @@ int amdgpu_device_initialize(int fd,
 
 	dev->vamgr = amdgpu_vamgr_get_global(dev);
 
+	max = MIN2(dev->dev_info.virtual_address_max, 0xffffffff);
+	start = amdgpu_vamgr_find_va(dev->vamgr,
+				     max - dev->dev_info.virtual_address_offset,
+				     dev->dev_info.virtual_address_alignment, 0);
+	if (start > 0xffffffff)
+		goto free_va; /* shouldn't get here */
+
+	dev->vamgr_32 =  calloc(1, sizeof(struct amdgpu_bo_va_mgr));
+	if (dev->vamgr_32 == NULL)
+		goto free_va;
+	amdgpu_vamgr_init(dev->vamgr_32, start, max,
+			  dev->dev_info.virtual_address_alignment);
+
 	*major_version = dev->major_version;
 	*minor_version = dev->minor_version;
 	*device_handle = dev;
@@ -260,6 +275,11 @@ int amdgpu_device_initialize(int fd,
 
 	return 0;
 
+free_va:
+	r = -ENOMEM;
+	amdgpu_vamgr_free_va(dev->vamgr, start,
+			     max - dev->dev_info.virtual_address_offset);
+
 cleanup:
 	if (dev->fd >= 0)
 		close(dev->fd);
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index 92eb5ec..ca155be 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -65,6 +65,7 @@ struct amdgpu_va {
 	uint64_t address;
 	uint64_t size;
 	enum amdgpu_gpu_va_range range;
+	struct amdgpu_bo_va_mgr *vamgr;
 };
 
 struct amdgpu_device {
@@ -82,7 +83,10 @@ struct amdgpu_device {
 	pthread_mutex_t bo_table_mutex;
 	struct drm_amdgpu_info_device dev_info;
 	struct amdgpu_gpu_info info;
+	/** The global VA manager for the whole virtual address space */
 	struct amdgpu_bo_va_mgr *vamgr;
+	/** The VA manager for the 32bit address space */
+	struct amdgpu_bo_va_mgr *vamgr_32;
 };
 
 struct amdgpu_bo {
@@ -124,6 +128,11 @@ drm_private struct amdgpu_bo_va_mgr* amdgpu_vamgr_get_global(struct amdgpu_devic
 
 drm_private void amdgpu_vamgr_reference(struct amdgpu_bo_va_mgr **dst, struct amdgpu_bo_va_mgr *src);
 
+drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
+		       uint64_t max, uint64_t alignment);
+
+drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr);
+
 drm_private uint64_t amdgpu_vamgr_find_va(struct amdgpu_bo_va_mgr *mgr, uint64_t size,
 				uint64_t alignment, uint64_t base_required);
 
diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 71fd2b1..91aad4e 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -46,7 +46,7 @@ int amdgpu_va_range_query(amdgpu_device_handle dev,
 	return -EINVAL;
 }
 
-static void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
+drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
 			      uint64_t max, uint64_t alignment)
 {
 	mgr->va_offset = start;
@@ -57,7 +57,7 @@ static void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
 	pthread_mutex_init(&mgr->bo_va_mutex, NULL);
 }
 
-static void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr)
+drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr)
 {
 	struct amdgpu_bo_va_hole *hole;
 	LIST_FOR_EACH_ENTRY(hole, &mgr->va_holes, list) {
@@ -252,23 +252,39 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
 			  amdgpu_va_handle *va_range_handle,
 			  uint64_t flags)
 {
-	va_base_alignment = MAX2(va_base_alignment, dev->vamgr->va_alignment);
-	size = ALIGN(size, vamgr.va_alignment);
+	struct amdgpu_bo_va_mgr *vamgr;
 
-	*va_base_allocated = amdgpu_vamgr_find_va(dev->vamgr, size,
+	if (flags & AMDGPU_VA_RANGE_32_BIT)
+		vamgr = dev->vamgr_32;
+	else
+		vamgr = dev->vamgr;
+
+	va_base_alignment = MAX2(va_base_alignment, vamgr->va_alignment);
+	size = ALIGN(size, vamgr->va_alignment);
+
+	*va_base_allocated = amdgpu_vamgr_find_va(vamgr, size,
 					va_base_alignment, va_base_required);
 
+	if (!(flags & AMDGPU_VA_RANGE_32_BIT) &&
+	    (*va_base_allocated == AMDGPU_INVALID_VA_ADDRESS)) {
+		/* fallback to 32bit address */
+		vamgr = dev->vamgr_32;
+		*va_base_allocated = amdgpu_vamgr_find_va(vamgr, size,
+					va_base_alignment, va_base_required);
+	}
+
 	if (*va_base_allocated != AMDGPU_INVALID_VA_ADDRESS) {
 		struct amdgpu_va* va;
 		va = calloc(1, sizeof(struct amdgpu_va));
 		if(!va){
-			amdgpu_vamgr_free_va(dev->vamgr, *va_base_allocated, size);
+			amdgpu_vamgr_free_va(vamgr, *va_base_allocated, size);
 			return -ENOMEM;
 		}
 		va->dev = dev;
 		va->address = *va_base_allocated;
 		va->size = size;
 		va->range = va_range_type;
+		va->vamgr = vamgr;
 		*va_range_handle = va;
 	} else {
 		return -EINVAL;
@@ -279,9 +295,13 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
 
 int amdgpu_va_range_free(amdgpu_va_handle va_range_handle)
 {
+	struct amdgpu_bo_va_mgr *vamgr;
+
 	if(!va_range_handle || !va_range_handle->address)
 		return 0;
-	amdgpu_vamgr_free_va(va_range_handle->dev->vamgr, va_range_handle->address,
+
+	amdgpu_vamgr_free_va(va_range_handle->vamgr,
+			va_range_handle->address,
 			va_range_handle->size);
 	free(va_range_handle);
 	return 0;
-- 
1.9.1

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

[PATCH 4/5] amdgpu: fix one warning from previous commit

[Jammy Zhou]

The local variable 'vamgr' is unused in amdgpu_va_range_free.

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
Reviewed-by: Alex Deucher <alexander.deucher@amd.com>
---
 amdgpu/amdgpu_vamgr.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 91aad4e..698826d 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -295,8 +295,6 @@ int amdgpu_va_range_alloc(amdgpu_device_handle dev,
 
 int amdgpu_va_range_free(amdgpu_va_handle va_range_handle)
 {
-	struct amdgpu_bo_va_mgr *vamgr;
-
 	if(!va_range_handle || !va_range_handle->address)
 		return 0;
 
-- 
1.9.1

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

[PATCH 5/5] amdgpu: make vamgr per device v2

[Jammy Zhou]

Each device can have its own vamgr, so make it per device now.
This can fix the failure with multiple GPUs used in one single
process.

v2: rebase

Signed-off-by: Jammy Zhou <Jammy.Zhou@amd.com>
Reviewed-by: Christian König <christian.koenig@amd.com>
---
 amdgpu/amdgpu_device.c   | 13 +++++++++++--
 amdgpu/amdgpu_internal.h |  5 -----
 amdgpu/amdgpu_vamgr.c    | 24 +-----------------------
 3 files changed, 12 insertions(+), 30 deletions(-)

diff --git a/amdgpu/amdgpu_device.c b/amdgpu/amdgpu_device.c
index 0ef1d31..d5f65e5 100644
--- a/amdgpu/amdgpu_device.c
+++ b/amdgpu/amdgpu_device.c
@@ -131,7 +131,8 @@ static int amdgpu_get_auth(int fd, int *auth)
 
 static void amdgpu_device_free_internal(amdgpu_device_handle dev)
 {
-	amdgpu_vamgr_reference(&dev->vamgr, NULL);
+	amdgpu_vamgr_deinit(dev->vamgr);
+	free(dev->vamgr);
 	util_hash_table_destroy(dev->bo_flink_names);
 	util_hash_table_destroy(dev->bo_handles);
 	pthread_mutex_destroy(&dev->bo_table_mutex);
@@ -252,7 +253,13 @@ int amdgpu_device_initialize(int fd,
 	if (r)
 		goto cleanup;
 
-	dev->vamgr = amdgpu_vamgr_get_global(dev);
+	dev->vamgr = calloc(1, sizeof(struct amdgpu_bo_va_mgr));
+	if (dev->vamgr == NULL)
+		goto cleanup;
+
+	amdgpu_vamgr_init(dev->vamgr, dev->dev_info.virtual_address_offset,
+			  dev->dev_info.virtual_address_max,
+			  dev->dev_info.virtual_address_alignment);
 
 	max = MIN2(dev->dev_info.virtual_address_max, 0xffffffff);
 	start = amdgpu_vamgr_find_va(dev->vamgr,
@@ -279,6 +286,8 @@ free_va:
 	r = -ENOMEM;
 	amdgpu_vamgr_free_va(dev->vamgr, start,
 			     max - dev->dev_info.virtual_address_offset);
+	amdgpu_vamgr_deinit(dev->vamgr);
+	free(dev->vamgr);
 
 cleanup:
 	if (dev->fd >= 0)
diff --git a/amdgpu/amdgpu_internal.h b/amdgpu/amdgpu_internal.h
index ca155be..cb06dbf 100644
--- a/amdgpu/amdgpu_internal.h
+++ b/amdgpu/amdgpu_internal.h
@@ -51,7 +51,6 @@ struct amdgpu_bo_va_hole {
 };
 
 struct amdgpu_bo_va_mgr {
-	atomic_t refcount;
 	/* the start virtual address */
 	uint64_t va_offset;
 	uint64_t va_max;
@@ -124,10 +123,6 @@ struct amdgpu_context {
 
 drm_private void amdgpu_bo_free_internal(amdgpu_bo_handle bo);
 
-drm_private struct amdgpu_bo_va_mgr* amdgpu_vamgr_get_global(struct amdgpu_device *dev);
-
-drm_private void amdgpu_vamgr_reference(struct amdgpu_bo_va_mgr **dst, struct amdgpu_bo_va_mgr *src);
-
 drm_private void amdgpu_vamgr_init(struct amdgpu_bo_va_mgr *mgr, uint64_t start,
 		       uint64_t max, uint64_t alignment);
 
diff --git a/amdgpu/amdgpu_vamgr.c b/amdgpu/amdgpu_vamgr.c
index 698826d..659e6c8 100644
--- a/amdgpu/amdgpu_vamgr.c
+++ b/amdgpu/amdgpu_vamgr.c
@@ -33,8 +33,6 @@
 #include "amdgpu_internal.h"
 #include "util_math.h"
 
-static struct amdgpu_bo_va_mgr vamgr = {{0}};
-
 int amdgpu_va_range_query(amdgpu_device_handle dev,
 			  enum amdgpu_gpu_va_range type, uint64_t *start, uint64_t *end)
 {
@@ -67,26 +65,6 @@ drm_private void amdgpu_vamgr_deinit(struct amdgpu_bo_va_mgr *mgr)
 	pthread_mutex_destroy(&mgr->bo_va_mutex);
 }
 
-drm_private struct amdgpu_bo_va_mgr * amdgpu_vamgr_get_global(struct amdgpu_device *dev)
-{
-	int ref;
-	ref = atomic_inc_return(&vamgr.refcount);
-
-	if (ref == 1)
-		amdgpu_vamgr_init(&vamgr, dev->dev_info.virtual_address_offset,
-				  dev->dev_info.virtual_address_max,
-				  dev->dev_info.virtual_address_alignment);
-	return &vamgr;
-}
-
-drm_private void amdgpu_vamgr_reference(struct amdgpu_bo_va_mgr **dst,
-				struct amdgpu_bo_va_mgr *src)
-{
-	if (update_references(&(*dst)->refcount, NULL))
-		amdgpu_vamgr_deinit(*dst);
-	*dst = src;
-}
-
 drm_private uint64_t amdgpu_vamgr_find_va(struct amdgpu_bo_va_mgr *mgr, uint64_t size,
 				uint64_t alignment, uint64_t base_required)
 {
@@ -102,7 +80,7 @@ drm_private uint64_t amdgpu_vamgr_find_va(struct amdgpu_bo_va_mgr *mgr, uint64_t
 	pthread_mutex_lock(&mgr->bo_va_mutex);
 	/* TODO: using more appropriate way to track the holes */
 	/* first look for a hole */
-	LIST_FOR_EACH_ENTRY_SAFE(hole, n, &vamgr.va_holes, list) {
+	LIST_FOR_EACH_ENTRY_SAFE(hole, n, &mgr->va_holes, list) {
 		if (base_required) {
 			if(hole->offset > base_required ||
 				(hole->offset + hole->size) < (base_required + size))
-- 
1.9.1

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

Re: [PATCH 0/5] some drm and amdgpu patches

[Michel Dänzer]

Hi Jammy,


On 13.08.2015 12:33, Jammy Zhou wrote:
> This series is a set of patches in my side pending for merge. And I rebased it
> with the drm_private series from Emil.
> 
> Emil Velikov (1):
>   drm: add interface to get drm devices on the system v2
> 
> Jammy Zhou (4):
>   amdgpu: improve amdgpu_vamgr_init
>   amdgpu: add flag to support 32bit VA address v3
>   amdgpu: fix one warning from previous commit
>   amdgpu: make vamgr per device v2

Please squash patch 4 (and maybe patch 5 as well?) into patch 3.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel

RE: [PATCH 0/5] some drm and amdgpu patches

[Zhou, Jammy]

+Alex

Sure, we can squash patch #4 into #3 when push the changes.

Regards,
Jammy

-----Original Message-----
From: Michel Dänzer [mailto:michel@daenzer.net] 
Sent: Thursday, August 13, 2015 2:20 PM
To: Zhou, Jammy; dri-devel@lists.freedesktop.org
Cc: Emil Velikov
Subject: Re: [PATCH 0/5] some drm and amdgpu patches


Hi Jammy,


On 13.08.2015 12:33, Jammy Zhou wrote:
> This series is a set of patches in my side pending for merge. And I 
> rebased it with the drm_private series from Emil.
> 
> Emil Velikov (1):
>   drm: add interface to get drm devices on the system v2
> 
> Jammy Zhou (4):
>   amdgpu: improve amdgpu_vamgr_init
>   amdgpu: add flag to support 32bit VA address v3
>   amdgpu: fix one warning from previous commit
>   amdgpu: make vamgr per device v2

Please squash patch 4 (and maybe patch 5 as well?) into patch 3.


-- 
Earthling Michel Dänzer               |               http://www.amd.com
Libre software enthusiast             |             Mesa and X developer
_______________________________________________
dri-devel mailing list
dri-devel@lists.freedesktop.org
http://lists.freedesktop.org/mailman/listinfo/dri-devel