Re: [PATCH linux vdagent 01/10] Add lookup_xrand_output_for_device_info()

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi,

sorry about piecemealing this.

On Thu, 2018-12-13 at 16:46 -0600, Jonathon Jongsma wrote:
> Add a function to look up an xrandr output for a given device display
> id. This uses sysfs and the drm subsystem to lookup information about a
> graphics device output. It then compares the drm output name to xrandr
> output names to try to match that device output to an xrandr output.
> This is necesary for guests that have multiple graphics devices.
> ---
>  Makefile.am               |  14 ++
>  configure.ac              |   1 +
>  src/vdagent/device-info.c | 516 ++++++++++++++++++++++++++++++++++++++
>  src/vdagent/device-info.h |  26 ++
>  tests/test-device-info.c  | 262 +++++++++++++++++++
>  5 files changed, 819 insertions(+)
>  create mode 100644 src/vdagent/device-info.c
>  create mode 100644 src/vdagent/device-info.h
>  create mode 100644 tests/test-device-info.c
> 
> diff --git a/Makefile.am b/Makefile.am
> index 3e405bc..d2f2258 100644
> --- a/Makefile.am
> +++ b/Makefile.am
> @@ -12,6 +12,7 @@ common_sources =				\
>  	$(NULL)
>  
>  src_spice_vdagent_CFLAGS =			\
> +	$(DRM_CFLAGS)				\
>  	$(X_CFLAGS)				\
>  	$(SPICE_CFLAGS)				\
>  	$(GLIB2_CFLAGS)				\
> @@ -22,6 +23,7 @@ src_spice_vdagent_CFLAGS =			\
>  	$(NULL)
>  
>  src_spice_vdagent_LDADD =			\
> +	$(DRM_LIBS)				\
>  	$(X_LIBS)				\
>  	$(SPICE_LIBS)				\
>  	$(GLIB2_LIBS)				\
> @@ -35,6 +37,8 @@ src_spice_vdagent_SOURCES =			\
>  	src/vdagent/audio.h			\
>  	src/vdagent/clipboard.c			\
>  	src/vdagent/clipboard.h			\
> +	src/vdagent/device-info.c		\
> +	src/vdagent/device-info.h		\
>  	src/vdagent/file-xfers.c		\
>  	src/vdagent/file-xfers.h		\
>  	src/vdagent/x11-priv.h			\
> @@ -137,3 +141,13 @@ EXTRA_DIST =					\
>  DISTCHECK_CONFIGURE_FLAGS =			\
>  	--with-init-script=redhat		\
>  	$(NULL)
> +
> +tests_test_device_info_LDADD = $(src_spice_vdagent_LDADD)
> +tests_test_device_info_CFLAGS = $(src_spice_vdagent_CFLAGS)
> +
> +check_PROGRAMS = 					\
> +	tests/test-device-info			\
> +	$(NULL)
> +
> +TESTS = $(check_PROGRAMS)
> +
> diff --git a/configure.ac b/configure.ac
> index 7cb44db..55b031e 100644
> --- a/configure.ac
> +++ b/configure.ac
> @@ -105,6 +105,7 @@ PKG_CHECK_MODULES(X, [xfixes xrandr >= 1.3 xinerama x11])
>  PKG_CHECK_MODULES(SPICE, [spice-protocol >= 0.12.13])
>  PKG_CHECK_MODULES(ALSA, [alsa >= 1.0.22])
>  PKG_CHECK_MODULES([DBUS], [dbus-1])
> +PKG_CHECK_MODULES([DRM], [libdrm])
>  
>  if test "$with_session_info" = "auto" || test "$with_session_info" = "systemd"; then
>      PKG_CHECK_MODULES([LIBSYSTEMD_LOGIN],
> diff --git a/src/vdagent/device-info.c b/src/vdagent/device-info.c
> new file mode 100644
> index 0000000..c519741
> --- /dev/null
> +++ b/src/vdagent/device-info.c
> @@ -0,0 +1,516 @@
> +/*  device-info.c utility function for looking up the xrandr output id for a
> + *  given device address and display id
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * Red Hat Authors:
> + * Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +#include <assert.h>
> +#include <fcntl.h>
> +#include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
> +#include <sys/stat.h>
> +#include <syslog.h>
> +#include <xf86drm.h>
> +#include <xf86drmMode.h>
> +#include <unistd.h>
> +#include <X11/extensions/Xrandr.h>
> +#include <glib.h>
> +
> +#include "device-info.h"
> +
> +typedef struct PciDevice {
> +    int domain;
> +    uint8_t bus;
> +    uint8_t slot;
> +    uint8_t function;
> +} PciDevice;
> +
> +typedef struct PciAddress {
> +    int domain;
> +    GList *devices; /* PciDevice */
> +} PciAddress;
> +
> +static PciAddress* pci_address_new()
> +{
> +    return g_new0(PciAddress, 1);
> +}
> +
> +static void pci_address_free(PciAddress *addr)
> +{
> +    g_list_free_full(addr->devices, g_free);
> +    g_free(addr);
> +}
> +
> +
> +static int read_next_hex_number(const char *input, char delim, char **endptr)
> +{
> +    assert(input != NULL);
> +    assert(endptr != NULL);
> +
> +    const char *pos = strchr(input, delim);
> +    int n;
> +    if (!pos) {
> +        *endptr = NULL;
> +        return 0;
> +    }
> +
> +    char *endpos;
> +    n = strtol(input, &endpos, 16);
> +
> +    // check if we read all characters until the delimiter
> +    if (endpos != pos)
> +        endpos = NULL;
> +
> +    *endptr = endpos;
> +    return n;
> +}
> +
> +// the device should be specified in BDF notation (e.g. 0000:00:02.0)
> +// see https://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
> +static bool parse_pci_device(const char *bdf, const char *end, PciDevice *device)
> +{
> +    if (!end) end = strchr(bdf, 0);
> +
> +    int endpos = -1;
> +    int domain, bus, slot, function;
> +    sscanf(bdf, "%x:%x:%x.%x%n", &domain, &bus, &slot, &function, &endpos);
> +    if (!device || endpos < 0 || bdf + endpos != end)
> +        return false;
> +    if (domain < 0 || bus < 0 || slot < 0 || function < 0)
> +        return false;
> +
> +    device->domain = domain;
> +    device->bus = bus;
> +    device->slot = slot;
> +    device->function = function;
> +    return true;
> +}
> +
> +// We need to extract the pci address of the device from the sysfs entry for the device like so:
> +// $ readlink /sys/class/drm/card0
> +// This should give you a path such as this for cards on the root bus:
> +// /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> +// or something like this if there is a pci bridge:
> +// /sys/devices/pci0000:00/0000:00:03.0/0000:01:01.0/0000:02:03.0/virtio2/drm/card0
> +static PciAddress* parse_pci_address_from_sysfs_path(const char* addr)
> +{
> +    char *pos = strstr(addr, "/pci");
> +    if (!pos)
> +        return NULL;
> +
> +    // advance to the numbers in pci0000:00
> +    pos += 4;
> +    int domain = read_next_hex_number(pos, ':', &pos);
> +    if (!pos) {
> +        return NULL;
> +    }
> +
> +    // not used right now.
> +    G_GNUC_UNUSED uint8_t bus = read_next_hex_number(pos + 1, '/', &pos);
> +    if (!pos) {
> +        return NULL;
> +    }
> +
> +    PciAddress *address = pci_address_new();
> +    address->domain = domain;
> +    // now read all of the devices
> +    while (pos) {
> +        PciDevice *dev = g_new0(PciDevice, 1);
> +        char *next = strchr(pos + 1, '/');
> +        if (!parse_pci_device(pos + 1, next, dev)) {
> +            g_free(dev);
> +            break;
> +        }
> +        address->devices = g_list_append(address->devices, dev);
> +        pos = next;
> +    }
> +    return address;
> +}
> +
> +// format should be something like pci/$domain/$slot.$fn/$slot.$fn
> +static PciAddress* parse_pci_address_from_spice(char *input)
> +{
> +    static const char prefix[] = "pci/";
> +    if (strncmp(input, prefix, strlen(prefix)) != 0)
> +        return NULL;
> +
> +    char *pos = input + strlen(prefix);
> +    int domain = read_next_hex_number(pos, '/', &pos);
> +    if (!pos) {
> +        return NULL;
> +    }
> +
> +    PciAddress *address = pci_address_new();
> +    address->domain = domain;
> +    // now read all of the devices
> +    for (int n = 0; ; n++) {
> +        PciDevice *dev = g_new0(PciDevice, 1);
> +        char *next = strchr(pos + 1, '/');
> +
> +        dev->slot = read_next_hex_number(pos + 1, '.', &pos);
> +        if (!pos) {
> +            g_free(dev);
> +            break;
> +        }
> +
> +        dev->function = strtol(pos + 1, &pos, 16);
> +        if (!pos || (next != NULL && next != pos)) {
> +            g_free(dev);
> +            break;
> +        }
> +
> +        address->devices = g_list_append(address->devices, dev);
> +        pos = next;
> +        if (!pos)
> +            break;
> +    }
> +    return address;
> +}
> +
> +static bool compare_addresses(PciAddress *a, PciAddress *b)
> +{
> +    // only check domain, slot, and function
> +    if (!(a->domain == b->domain
> +        && g_list_length(a->devices) == g_list_length(b->devices))) {
> +        return false;
> +    }
> +
> +    for (GList *la = a->devices, *lb = b->devices;
> +         la != NULL;
> +         la = la->next, lb = lb->next) {
> +        PciDevice *deva = la->data;
> +        PciDevice *devb = lb->data;
> +
> +        if (deva->slot != devb->slot
> +            || deva->function != devb->function) {
> +            return false;
> +        }
> +    }
> +    return true;
> +}
> +
> +// Connector type names from xorg modesetting driver
> +static const char * const modesetting_output_names[] = {
> +    [DRM_MODE_CONNECTOR_Unknown] = "None" ,
> +    [DRM_MODE_CONNECTOR_VGA] = "VGA" ,
> +    [DRM_MODE_CONNECTOR_DVII] = "DVI-I" ,
> +    [DRM_MODE_CONNECTOR_DVID] = "DVI-D" ,
> +    [DRM_MODE_CONNECTOR_DVIA] = "DVI-A" ,
> +    [DRM_MODE_CONNECTOR_Composite] = "Composite" ,
> +    [DRM_MODE_CONNECTOR_SVIDEO] = "SVIDEO" ,
> +    [DRM_MODE_CONNECTOR_LVDS] = "LVDS" ,
> +    [DRM_MODE_CONNECTOR_Component] = "Component" ,
> +    [DRM_MODE_CONNECTOR_9PinDIN] = "DIN" ,
> +    [DRM_MODE_CONNECTOR_DisplayPort] = "DP" ,
> +    [DRM_MODE_CONNECTOR_HDMIA] = "HDMI" ,
> +    [DRM_MODE_CONNECTOR_HDMIB] = "HDMI-B" ,
> +    [DRM_MODE_CONNECTOR_TV] = "TV" ,
> +    [DRM_MODE_CONNECTOR_eDP] = "eDP" ,
> +    [DRM_MODE_CONNECTOR_VIRTUAL] = "Virtual" ,
> +    [DRM_MODE_CONNECTOR_DSI] = "DSI" ,
> +    [DRM_MODE_CONNECTOR_DPI] = "DPI" ,
> +};
> +// Connector type names from qxl driver
> +static const char * const qxl_output_names[] = {
> +    [DRM_MODE_CONNECTOR_Unknown] = "None" ,
> +    [DRM_MODE_CONNECTOR_VGA] = "VGA" ,
> +    [DRM_MODE_CONNECTOR_DVII] = "DVI" ,
> +    [DRM_MODE_CONNECTOR_DVID] = "DVI" ,
> +    [DRM_MODE_CONNECTOR_DVIA] = "DVI" ,
> +    [DRM_MODE_CONNECTOR_Composite] = "Composite" ,
> +    [DRM_MODE_CONNECTOR_SVIDEO] = "S-video" ,
> +    [DRM_MODE_CONNECTOR_LVDS] = "LVDS" ,
> +    [DRM_MODE_CONNECTOR_Component] = "CTV" ,
> +    [DRM_MODE_CONNECTOR_9PinDIN] = "DIN" ,
> +    [DRM_MODE_CONNECTOR_DisplayPort] = "DisplayPort" ,
> +    [DRM_MODE_CONNECTOR_HDMIA] = "HDMI" ,
> +    [DRM_MODE_CONNECTOR_HDMIB] = "HDMI" ,
> +    [DRM_MODE_CONNECTOR_TV] = "TV" ,
> +    [DRM_MODE_CONNECTOR_eDP] = "eDP" ,
> +    [DRM_MODE_CONNECTOR_VIRTUAL] = "Virtual" ,
> +};
> +
> +
> +static void drm_conn_name_full(drmModeConnector *conn, const char * const *names, int nnames, char *dest, size_t dlen, bool decrement_id)
> +{
> +    const char *type;
> +
> +    if (conn->connector_type_id < nnames &&

I think this part of the condition should read:
if (conn->connector_type < nnames &&

> +        names[conn->connector_type]) {
> +        type = names[conn->connector_type];
> +    } else {
> +        type = "unknown";
> +    }
> +
> +    uint8_t id = conn->connector_type_id;
> +    if (decrement_id) {
> +        id--;
> +    }
> +    snprintf(dest, dlen, "%s-%d", type, id);
> +}
> +
> +static void drm_conn_name_qxl(drmModeConnector *conn, char *dest, size_t dlen, bool decrement_id)
> +{
> +    return drm_conn_name_full(conn, qxl_output_names,
> +                              sizeof(qxl_output_names)/sizeof(qxl_output_names[0]),
> +                              dest, dlen, decrement_id);
> +}
> +
> +// FIXME: there are some cases (for example, in my Lenovo T460p laptop with
> +// intel graphics) where the modesetting driver uses a name such as DP-3-1
> +// instead of DP-4. These outputs are not likely to be common in virtual
> +// machines, so it may not matter much (?)
> +static void drm_conn_name_modesetting(drmModeConnector *conn, char *dest, size_t dlen)
> +{
> +    return drm_conn_name_full(conn, modesetting_output_names,
> +                              sizeof(modesetting_output_names)/sizeof(modesetting_output_names[0]),
> +                              dest, dlen, false);
> +}
> +
> +static const char *const connections[] = {"", "*CONNECTED*", "disconnected", "unknown connection"};
> +
> +#define PCI_VENDOR_ID_REDHAT 0x1b36
> +#define PCI_VENDOR_ID_REDHAT_QUMRANET 0x1af4 // virtio-gpu
> +#define PCI_VENDOR_ID_INTEL 0x8086
> +#define PCI_VENDOR_ID_NVIDIA 0x10de
> +
> +#define PCI_DEVICE_ID_QXL 0x0100
> +#define PCI_DEVICE_ID_VIRTIO_GPU 0x1050
> +
> +static bool read_hex_value_from_file(const char *path, int* value)
> +{
> +    if (value == NULL || path == NULL)
> +        return false;
> +
> +    FILE *f = fopen(path, "r");
> +    if (f == NULL)
> +        return false;
> +
> +    int endpos = -1;
> +    bool result = (fscanf(f, "%x\n%n", value, &endpos) > 0 && endpos >= 0);
> +
> +    fclose(f);
> +    return result;
> +}
> +
> +    // PCI address should be in the following format:
> +    //   pci/$domain/$slot.$fn/$slot.$fn
> +bool lookup_xrandr_output_for_device_info(VDAgentGraphicsDeviceInfo *device_info, RROutput *output_id)
> +{

It would actually be nice to further split the content of this function
into smaller ones, e.g. translating the pci address, doing the drm
stuff etc.

> +    bool found_device = false;
> +    XRRScreenResources *xres = NULL;
> +    Display *xdisplay = NULL;
> +    PciAddress *user_pci_addr = parse_pci_address_from_spice((char*)device_info->device_address);
> +    if (!user_pci_addr) {
> +        syslog(LOG_WARNING, "Couldn't parse PCI address '%s'. Address should be the form 'pci/$domain/$slot.$fn/$slot.fn...", device_info->device_address);
> +        goto cleanup;
> +    }
> +    // look up xrandr outputs
> +    xdisplay = XOpenDisplay(NULL);
> +    if (!xdisplay) {
> +        syslog(LOG_WARNING, "Failed to open X dislay");
> +        goto cleanup;
> +    }
> +
> +    int rr_event_base, rr_error_base;
> +    if (!XRRQueryExtension(xdisplay, &rr_event_base, &rr_error_base)) {
> +        syslog(LOG_WARNING, "Failed to initialize XRandr extension");
> +        goto cleanup;
> +    }
> +
> +    xres = XRRGetScreenResourcesCurrent(xdisplay, DefaultRootWindow(xdisplay));
> +    if (!xres) {
> +        syslog(LOG_WARNING, "Unable to get Xorg screen resources");
> +        goto cleanup;
> +    }
> +
> +    // Look for a device that matches the PCI address parsed above. Loop
> +    // through the list of cards reported by the DRM subsytem
> +    for (int i = 0; i < 10; ++i) {
> +        char dev_path[64];
> +        struct stat buf;
> +
> +        // device node for the card is needed to access libdrm functionality
> +        snprintf(dev_path, sizeof(dev_path), DRM_DEV_NAME, DRM_DIR_NAME, i);
> +        if (stat(dev_path, &buf) != 0) {
> +            // no card exists, exit loop
> +            syslog(LOG_DEBUG, "No card %i exists\n", i);
> +            break;
> +        }
> +
> +        // the sysfs directory for the card will allow us to determine the
> +        // pci address for the device
> +        char sys_path[64];
> +        snprintf(sys_path, sizeof(sys_path), "/sys/class/drm/card%d", i);
> +        syslog(LOG_DEBUG, "sys path: %s\n", sys_path);
> +
> +        // the file /sys/class/drm/card0 is a symlink to a file that
> +        // specifies the device's address. It usually points to something
> +        // like /sys/devices/pci0000:00/0000:00:02.0/drm/card0
> +        char device_link[PATH_MAX];
> +        if (realpath(sys_path, device_link) == NULL) {
> +            syslog(LOG_WARNING, "Failed to get the real path of %s", sys_path);
> +            goto cleanup;
> +        }
> +        syslog(LOG_DEBUG, "Device %s is at %s\n", dev_path, device_link);
> +
> +        PciAddress *drm_pci_addr = parse_pci_address_from_sysfs_path(device_link);
> +        if (!drm_pci_addr) {
> +            syslog(LOG_DEBUG, "Can't determine pci address from '%s'", device_link);
> +            continue;
> +        }
> +
> +        if (!compare_addresses(user_pci_addr, drm_pci_addr)) {
> +            pci_address_free(drm_pci_addr);
> +            syslog(LOG_DEBUG, "card addr '%s' does not match requested addr '%s'\n", device_link, device_info->device_address);
> +            continue;
> +        }
> +        pci_address_free(drm_pci_addr);
> +
> +        bool found_output = false;
> +        char id_path[150];
> +        snprintf(id_path, sizeof(id_path), "%s/device/vendor", sys_path);
> +        int vendor_id = 0;
> +        if (!read_hex_value_from_file(id_path, &vendor_id)) {
> +            syslog(LOG_WARNING, "Unable to read vendor ID of card\n");
> +        } else {
> +            syslog(LOG_DEBUG, "Vendor id of this card is %#x\n", vendor_id);
> +        }
> +        snprintf(id_path, sizeof(id_path), "%s/device/device", sys_path);
> +        int device_id = 0;
> +        if (!read_hex_value_from_file(id_path, &device_id)) {
> +            syslog(LOG_WARNING, "Unable to read device ID of card\n");
> +        } else {
> +            syslog(LOG_DEBUG, "Device id of this card is %#x\n", device_id);
> +        }
> +
> +        int drm_fd = open(dev_path, O_RDWR);
> +        if (drm_fd < 0) {
> +            syslog(LOG_WARNING, "Unable to open file %s", dev_path);
> +            goto cleanup;
> +        }
> +
> +        drmModeResPtr res = drmModeGetResources(drm_fd);
> +        if (res) {
> +            // find the drm output that is equal to device_display_id
> +            if (device_info->device_display_id >= res->count_connectors) {
> +                syslog(LOG_WARNING, "Specified display id %i is higher than the maximum display id provided by this device (%i)",
> +                     device_info->device_display_id, res->count_connectors - 1);
> +                goto cleanup;
> +            }
> +
> +            drmModeConnectorPtr conn = drmModeGetConnector(drm_fd, res->connectors[device_info->device_display_id]);

drmModeGetConnector can return NULL in case of an error, better check
it.

> +            drmModeFreeResources(res);
> +            res = NULL;
> +
> +            bool decrement_name = false;
> +            if (vendor_id == PCI_VENDOR_ID_REDHAT && device_id == PCI_DEVICE_ID_QXL) {
> +                // Older QXL drivers numbered their outputs starting with
> +                // 0. This contrasts with most drivers who start numbering
> +                // outputs with 1.  In this case, the expected drm connector
> +                // name will need to be decremented before comparing to the
> +                // xrandr output name
> +                for (int i = 0; i < xres->noutput; ++i) {
> +                    XRROutputInfo *oinfo = XRRGetOutputInfo(xdisplay, xres, xres->outputs[i]);
> +                    // the QXL device doesn't use the drm names directly, but
> +                    // starts numbering from 0 instead of 1, so we need to
> +                    // detect this condition and add 1 to all output names in
> +                    // this scenario before comparing
> +                    // check if one output is named "Virtual-0"
> +                    if (strcmp(oinfo->name, "Virtual-0") == 0) {

In case of Virtual-X output names on a different card than the one used
with X, this check may fail. Though maybe that's an unsupported use
case? Maybe a more reliable way would be to check for a file matching
/sys/class/drm/card*-Virtual-0?

> +                        syslog(LOG_DEBUG, "Need to decrement connector name...\n");
> +                        decrement_name = true;
> +                        XRRFreeOutputInfo(oinfo);
> +                        break;
> +                    }
> +                    XRRFreeOutputInfo(oinfo);
> +                }
> +            }
> +            // Compare the name of the xrandr output against what we would
> +            // expect based on the drm connection type. The xrandr names
> +            // are driver-specific, so we need to special-case some
> +            // drivers.  Most hardware these days uses the 'modesetting'
> +            // driver, but the QXL device uses its own driver which has
> +            // different naming conventions
> +            char expected_name[100];
> +            if (vendor_id == PCI_VENDOR_ID_REDHAT && device_id == PCI_DEVICE_ID_QXL) {
> +                drm_conn_name_qxl(conn, expected_name, sizeof(expected_name), decrement_name);
> +            } else {
> +                drm_conn_name_modesetting(conn, expected_name, sizeof(expected_name));
> +            }
> +
> +            // Loop through xrandr outputs and check whether the xrandr
> +            // output name matches the drm connector name
> +            for (int i = 0; i < xres->noutput; ++i) {
> +                int oid = xres->outputs[i];
> +                XRROutputInfo *oinfo = XRRGetOutputInfo(xdisplay, xres, oid);
> +
> +                syslog(LOG_DEBUG, "Checking whether xrandr output %i (%s) matches expected name %s\n", i, oinfo->name, expected_name);
> +                if (strcmp(oinfo->name, expected_name) == 0) {
> +                    *output_id = oid;
> +                    found_output = true;
> +                    printf("    Found matching X Output:	name=%s id=%i (%s)\n",
> +                           oinfo->name,
> +                           (int)oid,
> +                           connections[oinfo->connection + 1]);
> +                    XRRFreeOutputInfo(oinfo);
> +                    break;
> +                }
> +                XRRFreeOutputInfo(oinfo);
> +            }
> +            drmModeFreeConnector(conn);
> +            conn = NULL;

No need to NULL the pointer.

Cheers,
Lukas

> +        } else {
> +            syslog(LOG_WARNING, "Unable to get DRM resources for card");
> +            // This is probably a proprietary driver (e.g. Nvidia) that does
> +            // not provide outputs via drm, so the only thing we can do is just
> +            // assume that it is the only device assigned to X, and use the
> +            // xrandr output order to determine the proper display.
> +            if (device_info->device_display_id >= xres->noutput) {
> +                syslog(LOG_WARNING, "The device display id %i does not exist", device_info->device_display_id);
> +                goto cleanup;
> +            }
> +            *output_id = xres->outputs[device_info->device_display_id];
> +            found_output = true;
> +        }
> +        close(drm_fd);
> +
> +        if (!found_output) {
> +            syslog(LOG_WARNING, "Couldn't find an XRandr output for the specified device");
> +            goto cleanup;
> +        }
> +
> +        // we found the right card, abort the loop
> +        found_device = true;
> +        break;
> +    }
> +
> +cleanup:
> +    if (xres) {
> +        XRRFreeScreenResources(xres);
> +    }
> +    if (xdisplay) {
> +        XCloseDisplay(xdisplay);
> +    }
> +    if (user_pci_addr) {
> +        pci_address_free(user_pci_addr);
> +    }
> +    return found_device;
> +}
> diff --git a/src/vdagent/device-info.h b/src/vdagent/device-info.h
> new file mode 100644
> index 0000000..ec3ca32
> --- /dev/null
> +++ b/src/vdagent/device-info.h
> @@ -0,0 +1,26 @@
> +/*  device-info.c utility function for looking up the xrandr output id for a
> + *  given device address and display id
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * Red Hat Authors:
> + * Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +#include <spice/vd_agent.h>
> +#include <X11/extensions/Xrandr.h>
> +#include <stdbool.h>
> +
> +bool lookup_xrandr_output_for_device_info(VDAgentGraphicsDeviceInfo *device_info, RROutput *output_id);
> diff --git a/tests/test-device-info.c b/tests/test-device-info.c
> new file mode 100644
> index 0000000..5964313
> --- /dev/null
> +++ b/tests/test-device-info.c
> @@ -0,0 +1,262 @@
> +/*  test-device-info.c tests to see whether the functionality for converting a
> + *  device address to a xrandr output are working properly
> + *
> + * Copyright 2018 Red Hat, Inc.
> + *
> + * Red Hat Authors:
> + * Jonathon Jongsma <jjongsma@xxxxxxxxxx>
> + *
> + * This program is free software: you can redistribute it and/or modify
> + * it under the terms of the GNU General Public License as published by
> + * the Free Software Foundation, either version 3 of the License, or
> + * (at your option) any later version.
> + *
> + * This program is distributed in the hope that it will be useful,
> + * but WITHOUT ANY WARRANTY; without even the implied warranty of
> + * MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> + * GNU General Public License for more details.
> + *
> + * You should have received a copy of the GNU General Public License
> + * along with this program.  If not, see <http://www.gnu.org/licenses/>.
> + */
> +
> +// just include the source file for testing
> +#include "vdagent/device-info.c"
> +
> +#define assert_device(dev, domain_, bus_, slot_, function_) \
> +{ \
> +    PciDevice* dev_ = (dev); \
> +    assert(dev_ != NULL); \
> +    assert(dev_->domain == domain_); \
> +    assert(dev_->bus == bus_); \
> +    assert(dev_->slot == slot_); \
> +    assert(dev_->function == function_); \
> +}
> +
> +static void test_compare_addresses()
> +{
> +    {
> +        PciDevice da1 = {1, 0, 3, 0};
> +        PciDevice da2 = {1, 1, 1, 0};
> +        PciDevice da3 = {1, 2, 3, 0};
> +        PciAddress a1 = {1, NULL};
> +        a1.domain = 0;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +        a1.devices = g_list_append(a1.devices, &da2);
> +        a1.devices = g_list_append(a1.devices, &da3);
> +
> +        PciDevice db1 = {1, 0, 3, 0};
> +        PciDevice db2 = {1, 1, 1, 0};
> +        PciDevice db3 = {1, 2, 3, 0};
> +        PciAddress a2 = {1, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +        a2.devices = g_list_append(a2.devices, &db2);
> +        a2.devices = g_list_append(a2.devices, &db3);
> +
> +        assert(compare_addresses(&a1, &a2));
> +    }
> +    {
> +        PciDevice da1 = {1, 0, 3, 0};
> +        PciDevice da2 = {1, 1, 1, 0};
> +        PciDevice da3 = {1, 2, 3, 0};
> +        PciAddress a1 = {1, NULL};
> +        a1.domain = 0;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +        a1.devices = g_list_append(a1.devices, &da2);
> +        a1.devices = g_list_append(a1.devices, &da3);
> +
> +        // a 'spice' format PCI address will not provide domain or bus for each
> +        // device, only slot and function. So first two numbers for each device
> +        // will always be set to 0
> +        PciDevice db1 = {0, 0, 3, 0};
> +        PciDevice db2 = {0, 0, 1, 0};
> +        PciDevice db3 = {0, 0, 3, 0};
> +        PciAddress a2 = {1, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +        a2.devices = g_list_append(a2.devices, &db2);
> +        a2.devices = g_list_append(a2.devices, &db3);
> +
> +        assert(compare_addresses(&a1, &a2));
> +    }
> +    // different number of devices
> +    {
> +        PciDevice da1 = {0, 0, 3, 0};
> +        PciDevice da2 = {0, 1, 1, 0};
> +        PciDevice da3 = {0, 2, 3, 0};
> +        PciAddress a1 = {0, NULL};
> +        a1.domain = 0;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +        a1.devices = g_list_append(a1.devices, &da2);
> +        a1.devices = g_list_append(a1.devices, &da3);
> +
> +        PciDevice db1 = {0, 0, 3, 0};
> +        PciDevice db2 = {0, 1, 1, 0};
> +        PciAddress a2 = {0, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +        a2.devices = g_list_append(a2.devices, &db2);
> +
> +        assert(!compare_addresses(&a1, &a2));
> +    }
> +    // mismatched function
> +    {
> +        PciDevice da1 = {0, 0, 2, 0};
> +        PciAddress a1 = {0, NULL};
> +        a1.domain = 0;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +
> +        PciDevice db1 = {0, 0, 2, 1};
> +        PciAddress a2 = {0, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +
> +        assert(!compare_addresses(&a1, &a2));
> +    }
> +    // mismatched slot
> +    {
> +        PciDevice da1 = {0, 0, 2, 0};
> +        PciAddress a1 = {0, NULL};
> +        a1.domain = 0;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +
> +        PciDevice db1 = {0, 0, 1, 0};
> +        PciAddress a2 = {0, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +
> +        assert(!compare_addresses(&a1, &a2));
> +    }
> +    // mismatched domain
> +    {
> +        PciDevice da1 = {0, 0, 2, 0};
> +        PciAddress a1 = {0, NULL};
> +        a1.domain = 1;
> +        a1.devices = g_list_append(a1.devices, &da1);
> +
> +        PciDevice db1 = {0, 0, 2, 0};
> +        PciAddress a2 = {0, NULL};
> +        a2.domain = 0;
> +        a2.devices = g_list_append(a2.devices, &db1);
> +
> +        assert(!compare_addresses(&a1, &a2));
> +    }
> +}
> +
> +static void test_spice_parsing()
> +{
> +    PciAddress *addr = parse_pci_address_from_spice("pci/0000/02.0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 0);
> +    assert(g_list_length(addr->devices) == 1);
> +    assert_device(addr->devices->data, 0, 0, 2, 0);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_spice("pci/ffff/ff.f");
> +    assert(addr != NULL);
> +    assert(addr->domain == 65535);
> +    assert(g_list_length(addr->devices) == 1);
> +    assert_device(addr->devices->data, 0, 0, 255, 15);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_spice("pci/0000/02.1/03.0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 0);
> +    assert(g_list_length(addr->devices) == 2);
> +    assert_device(addr->devices->data, 0, 0, 2, 1);
> +    assert_device(addr->devices->next->data, 0, 0, 3, 0);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_spice("pci/000a/01.0/02.1/03.0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 10);
> +    assert(g_list_length(addr->devices) == 3);
> +    assert_device(addr->devices->data, 0, 0, 1, 0);
> +    assert_device(addr->devices->next->data, 0, 0, 2, 1);
> +    assert_device(addr->devices->next->next->data, 0, 0, 3, 0);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_spice("pcx/0000/02.1/03.0");
> +    assert(addr == NULL);
> +
> +    addr = parse_pci_address_from_spice("0000/02.0");
> +    assert(addr == NULL);
> +
> +    addr = parse_pci_address_from_spice("0000/02.1/03.0");
> +    assert(addr == NULL);
> +}
> +
> +static void test_sysfs_parsing()
> +{
> +    PciAddress *addr = parse_pci_address_from_sysfs_path("../../devices/pci0000:00/0000:00:02.0/drm/card0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 0);
> +    assert(g_list_length(addr->devices) == 1);
> +    assert_device(addr->devices->data, 0, 0, 2, 0);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_sysfs_path("../../devices/pciffff:ff/ffff:ff:ff.f/drm/card0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 65535);
> +    assert(g_list_length(addr->devices) == 1);
> +    assert_device(addr->devices->data, 65535, 255, 255, 15);
> +    pci_address_free(addr);
> +
> +    addr = parse_pci_address_from_sysfs_path("../../devices/pci0000:00/0000:00:03.0/0000:01:01.0/0000:02:03.0/virtio2/drm/card0");
> +    assert(addr != NULL);
> +    assert(addr->domain == 0);
> +    assert(g_list_length(addr->devices) == 3);
> +    assert_device(addr->devices->data, 0, 0, 3, 0);
> +    assert_device(addr->devices->next->data, 0, 1, 1, 0);
> +    assert_device(addr->devices->next->next->data, 0, 2, 3, 0);
> +    pci_address_free(addr);
> +}
> +
> +// verify that we parse the BDF notation correctly
> +static bool test_bdf(const char* string, int domain, uint8_t bus, uint8_t slot, uint8_t function)
> +{
> +    PciDevice pci_dev;
> +    return (parse_pci_device(string, NULL, &pci_dev)
> +            && (pci_dev.domain == domain)
> +            && (pci_dev.bus == bus)
> +            && (pci_dev.slot == slot)
> +            && (pci_dev.function == function));
> +}
> +
> +static void test_bdf_parsing()
> +{
> +    // valid input
> +    assert(test_bdf("0000:00:02.1", 0, 0, 2, 1));
> +    assert(test_bdf("00:00:02.1", 0, 0, 2, 1));
> +    assert(test_bdf("0000:00:03.0", 0, 0, 3, 0));
> +    assert(test_bdf("0000:00:1d.1", 0, 0, 29, 1));
> +    assert(test_bdf("0000:09:02.1", 0, 9, 2, 1));
> +    assert(test_bdf("0000:1d:02.1", 0, 29, 2, 1));
> +    assert(test_bdf("0000:00:02.d", 0, 0, 2, 13));
> +    assert(test_bdf("000f:00:02.d", 15, 0, 2, 13));
> +    assert(test_bdf("000f:00:02.d", 15, 0, 2, 13));
> +    assert(test_bdf("000f:00:02.d", 15, 0, 2, 13));
> +    assert(test_bdf("000f:00:02.d", 15, 0, 2, 13));
> +    assert(test_bdf("ffff:ff:ff.f", 65535, 255, 255, 15));
> +    assert(test_bdf("0:0:2.1", 0, 0, 2, 1));
> +
> +    // invalid input
> +    assert(!test_bdf("0000:00:02:0", 0, 0, 2, 0));
> +    assert(!test_bdf("-0001:00:02.1", -1, 0, 2, 1));
> +    assert(!test_bdf("0000.00.02.0", 0, 0, 2, 0));
> +    assert(!test_bdf("000f:00:02", 15, 0, 2, 0));
> +    assert(!test_bdf("000f:00", 15, 0, 0, 0));
> +    assert(!test_bdf("000f", 15, 0, 0, 0));
> +    assert(!test_bdf("random string", 0, 0, 0, 0));
> +    assert(!test_bdf("12345", 12345, 0, 0, 0));
> +}
> +
> +int main(int argc, char **argv)
> +{
> +    test_bdf_parsing();
> +    test_sysfs_parsing();
> +    test_spice_parsing();
> +    test_compare_addresses();
> +}
> +
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel




[Index of Archives]     [Linux Virtualization]     [Linux Virtualization]     [Linux ARM Kernel]     [Linux ARM]     [Linux Omap]     [Fedora ARM]     [IETF Annouce]     [Security]     [Bugtraq]     [Linux OMAP]     [Linux MIPS]     [ECOS]     [Asterisk Internet PBX]     [Linux API]     [Monitors]