[Spice-devel] [PATCH linux vdagent 01/10] Add lookup_xrand_output_for_device_info()
Lukáš Hrázký
lhrazky at redhat.com
Mon Dec 17 16:57:45 UTC 2018
* Hello,
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.
All in all, the logging here seems a bit too verbose... I tried to make
some suggestions below. I would also change the lines representing
errors from LOG_WARNING to LOG_ERR.
> ---
> 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 at redhat.com>
> + *
> + * 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 &&
> + 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 (?)
I'm not really sure what to do here, there need to be some
specification to how the names are composed, we should probably look it
up (unless we want to just assume it won't be happening inside a
VM...).
> +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
I'm kind of used to having the #defines and declarations (typedefs,
static variables) at the beginning of the file followed by definitions
of the functions. Subjective, I see you are putting the declarations
before the functions in which they are used, but a bit unexpected for
me.
> +
> +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)
> +{
> + 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;
> + }
Getting the X display and querying the extension is already done in
x11-randr initialization, any reason to duplicate it here?
> + 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);
Slightly confusing, not sure it's useful to log this. If you want it to
stay, how about something like "card%i not found while listing DRM
cards, qitting."?
> + break;
"break" and "goto cleanup" amount to the same thing inside this for
loop, maybe unify the usage?
> + }
> +
> + // 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);
Not sure if this log line is useful, but it certainly lacks more
context information. I'd probably just leave it out.
> + // 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);
This should be at least LOG_WARNING, though I'd consinder logging the
"error-like" log lines like this one at LOG_ERR.
> + 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);
Instead of logging what does not match, I'd log the match once you got
it. It's much less verbose and more readable in case of multiple items
being compared, albeit that won't usually be the case here.
> + 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");
strerror() would be very useful here, in the similar log line above and
anywhere else where you're logging an error resulting from a syscall or
library call.
> + } else {
> + syslog(LOG_DEBUG, "Device id of this card is %#x\n", device_id);
I'd combine this with the vendor ID logline above, you can also put it
on a line saying you found a card, like "Found card %s matching PCI
address %s vendor ID %#x device ID %#x".
> + }
> +
> + 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;
Looks like this will leak the drm_fd here.
> + }
> +
> + drmModeConnectorPtr conn = drmModeGetConnector(drm_fd, res->connectors[device_info->device_display_id]);
> + 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
This comment seems like it needs to be updated since you are now
decrementing the connector name instead of incrementing the output
names. Also seems a bit reduntant with the comment above it.
> + // check if one output is named "Virtual-0"
> + if (strcmp(oinfo->name, "Virtual-0") == 0) {
> + syslog(LOG_DEBUG, "Need to decrement connector name...\n");
I'd leave this logline out as well, but if you want to keep it, how
about "Decrementing legacy QXL connector name by 1" or something along
those lines?
> + 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);
I'd leave out this one too, found (i.e. the printf below) / not found
(should be added) would be better?
> + 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]);
printf().
Also, what about the "connections" array? Is it useful for anything?
And why shift the contents by putting "" as the first item and doing a
+1 here?
> + XRRFreeOutputInfo(oinfo);
> + break;
> + }
> + XRRFreeOutputInfo(oinfo);
> + }
In case we don't find the output name, should we like print a warning
and fall back to output indexing too? (like in the "else" branch just
below)
> + drmModeFreeConnector(conn);
> + conn = NULL;
> + } else {
> + syslog(LOG_WARNING, "Unable to get DRM resources for card");
I'd mention the PCI address here and also that we're falling back to
simple xrandr output list index approach.
Cheers,
Lukas
> + // 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 at redhat.com>
> + *
> + * 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 at redhat.com>
> + *
> + * 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();
> +}
> +
More information about the Spice-devel
mailing list