[Spice-devel] [RFC linux vdagent] Add POC for getting xrandr output from monitor ID
Jonathon Jongsma
jjongsma at redhat.com
Wed Nov 7 17:20:43 UTC 2018
On Wed, 2018-11-07 at 05:51 -0500, Frediano Ziglio wrote:
> >
> > This is just a proof of concept utility that takes a PCI device
> > address
> > and a monitor ID and finds the xrandr output associated with that
> > monitor id.
>
> In title and comment "monitor ID" -> "device display ID".
> I would add some explanation on the comment like how this is supposed
> to
> be used.
> I did some changes, not much functional beside some QXL detection and
> removal of
> some issue, see
> https://cgit.freedesktop.org/~fziglio/vd_agent_linux/log/?h=jj,
> you probably can squash them all.
>
Thanks, they look like good changes. Note that I am mostly interested
in whether the general logic and approach will do what we want it to
do, however. I'm slightly less interested in the minor code-correctness
issues at the moment.
> > ---
> >
> > Changes in v2:
> > - used different format for specifying the PCI address
> > (pci/$domain/$dev.$fn)
> > - used err()/errx() to report errors (from err.h)
> > - Added some debug output (export DEBUG=1)
> > - read PCI address from sysfs by getting the link for
> > /sys/class/drm/card0 instead of /sys/class/drm/card0/device
> > - handle the full PCI heirarchy (including bridges)
> > - handle different vendor/device types by customizing the expected
> > xrandr names
> > - Query X outputs outside the loop so they don't get looked up for
> > every different device
> > - handle Nvidia devices that don't provide outputs via the DRM
> > subsystem by assuming that it's the only device being used by X
> > and
> > simply looking up the Nth Xrandr output.
> > - added tests
> > - various other fixes
> >
> > Makefile.am | 18 +
> > configure.ac | 1 +
> > src/vdagent/get-xrandr-output.c | 806
> > ++++++++++++++++++++++++++++++++
> > 3 files changed, 825 insertions(+)
> > create mode 100644 src/vdagent/get-xrandr-output.c
> >
> > diff --git a/Makefile.am b/Makefile.am
> > index 3e405bc..b159650 100644
> > --- a/Makefile.am
> > +++ b/Makefile.am
> > @@ -3,6 +3,7 @@ NULL =
> >
> > bin_PROGRAMS = src/spice-vdagent
> > sbin_PROGRAMS = src/spice-vdagentd
> > +noinst_PROGRAMS = src/get-xrandr-output
> >
> > common_sources = \
> > src/udscs.c \
> > @@ -77,6 +78,23 @@ src_spice_vdagentd_SOURCES =
> > \
> > src/vdagentd/virtio-port.h \
> > $(NULL)
> >
> > +src_get_xrandr_output_SOURCES = \
> > + src/vdagent/get-xrandr-output.c \
> > + $(NULL)
> > +
> > +src_get_xrandr_output_CFLAGS = \
> > + $(AM_CPPFLAGS) \
> > + $(DRM_CFLAGS) \
> > + $(X_CFLAGS) \
> > + $(GLIB2_CFLAGS) \
> > + $(NULL)
> > +
> > +src_get_xrandr_output_LDADD = \
> > + $(DRM_LIBS) \
> > + $(X_LIBS) \
> > + $(GLIB2_LIBS) \
> > + $(NULL)
> > +
> > if HAVE_CONSOLE_KIT
> > src_spice_vdagentd_SOURCES += src/vdagentd/console-kit.c
> > else
> > 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/get-xrandr-output.c
> > b/src/vdagent/get-xrandr-output.c
> > new file mode 100644
> > index 0000000..c2116d2
> > --- /dev/null
> > +++ b/src/vdagent/get-xrandr-output.c
> > @@ -0,0 +1,806 @@
> > +/* get-xrandr-output.c proof of concept for converting PCI
> > address and
> > device
> > + * display id to an xrandr output in the guest.
> > + *
> > + * 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 <err.h>
> > +#include <errno.h>
> > +#include <fcntl.h>
> > +#include <limits.h>
> > +#include <string.h>
> > +#include <stdbool.h>
> > +#include <stdio.h>
> > +#include <stdlib.h>
> > +#include <sys/stat.h>
> > +#include <xf86drm.h>
> > +#include <xf86drmMode.h>
> > +#include <unistd.h>
> > +#include <X11/extensions/Xrandr.h>
> > +#include <glib.h>
> > +
> > +static bool debug = false;
> > +#define DEBUG(...) \
> > + if (G_UNLIKELY(debug)) printf(__VA_ARGS__);
> > +
> > +typedef struct PciDevice {
> > + int domain;
> > + uint8_t bus;
> > + uint8_t slot;
> > + uint8_t function;
> > +} PciDevice;
> > +
> > +typedef struct PciAddress {
> > + int domain;
> > + GList *devices; /* PciDevice */
> > +} PciAddress;
> > +
> > +PciAddress* pci_address_new()
> > +{
> > + return g_new0(PciAddress, 1);
> > +}
> > +
> > +void pci_address_free(PciAddress *addr)
> > +{
> > + g_list_free_full(addr->devices, g_free);
> > + g_free(addr);
> > +}
> > +
> > +
> > +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);
>
> I don't think a big issue but this will accept numbers like " -
> 0x123a" or
> "21287531273521751834213421312214215425328513725" (overflow), but
> should
> fail later so not a big issue if somebody wants to pass garbage.
>
> > +
> > + // 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
> > +bool parse_pci_device(const char *bdf, const char *end, PciDevice
> > *device)
> > +{
> > + if (!device)
> > + return false;
>
> there is a mix in the code of returning failure, ignore or asserting
> for NULL
> pointers.
Yeah, I suppose it's not the most consistent code in the world. When we
integrate this into the agent (rather than a throw-away POC commandline
utility) I'll definitely be more consistent
>
> > +
> > + char *pos;
> > + device->domain = read_next_hex_number(bdf, ':', &pos);
> > + if (!pos)
> > + return false;
> > +
> > + device->bus = read_next_hex_number(pos + 1, ':', &pos);
> > + if (!pos)
> > + return false;
> > +
> > + device->slot = read_next_hex_number(pos + 1, '.', &pos);
> > + if (!pos)
> > + return false;
> > +
> > + device->function = strtol(pos + 1, &pos, 16);
> > + if (!pos || (end != NULL && end != pos))
> > + return false;
> > +
> > + 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/virt
> > io2/drm/card0
> > +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.
> > + 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
> > + for (int n = 0; ; n++) {
> > + 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;
> > + if (!pos)
> > + break;
> > + }
> > + return address;
> > +}
> > +
> > +// format should be something like pci/$domain/$slot.$fn/$slot.$fn
> > +PciAddress* parse_pci_address_from_spice(char *input)
> > +{
> > + const char * const 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;
> > +}
> > +
> > +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
> > +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" ,
> > +};
> > +
> > +
> > +void drm_conn_name_full(drmModeConnector *conn, const char * const
> > *names,
> > int nnames, char *dest, size_t dlen)
> > +{
> > + const char *type;
> > +
> > + if (conn->connector_type_id < nnames &&
> > + names[conn->connector_type]) {
> > + type = names[conn->connector_type];
> > + } else {
> > + type = "unknown";
> > + }
> > + snprintf(dest, dlen, "%s-%d", type, conn->connector_type_id);
> > +}
> > +
> > +void drm_conn_name_qxl(drmModeConnector *conn, char *dest, size_t
> > dlen)
> > +{
> > + return drm_conn_name_full(conn, qxl_output_names,
> > +
> > sizeof(qxl_output_names)/sizeof(qxl_output_names[0]),
> > + dest, dlen);
> > +}
> > +
> > +// 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 (?)
> > +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);
> > +}
> > +
> > +static const char *connections[] = {"", "*CONNECTED*",
> > "disconnected",
> > "unknown connection"};
> > +
> > +// verify that we parse the BDF notation correctly
> > +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));
> > +}
> > +
> > +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, 0, 0));
> > + assert(!test_bdf("-0001:00:02.1", 0, 0, 2, 1));
> > + assert(!test_bdf("0000.00.02.0", 0, 0, 0, 0));
> > + assert(!test_bdf("000f:00:02", 0, 0, 0, 0));
> > + assert(!test_bdf("000f:00", 0, 0, 0, 0));
> > + assert(!test_bdf("000f", 0, 0, 0, 0));
> > + assert(!test_bdf("random string", 0, 0, 0, 0));
> > + assert(!test_bdf("12345", 0, 0, 0, 0));
> > +}
> > +
> > +#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_); \
> > +}
> > +
> > +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);
> > +}
> > +
> > +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);
> > +}
> > +
> > +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};I did not attempt to
> > + 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));
> > + }
> > +}
> > +
> > +void run_tests()
> > +{
> > + test_bdf_parsing();
> > + test_sysfs_parsing();
> > + test_spice_parsing();
> > + test_compare_addresses();
> > +}
> > +
> > +#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;
> > +
> > + int fd = open(path, O_RDONLY);
> > + char buf[255];
> > + bool result = false;
> > + if (fd < 1)
> > + return false;
> > +
> > + size_t nread = 0, n = 0;
> > + while (nread < sizeof(buf)
> > + && (n = read(fd, buf + nread, sizeof(buf) - nread))) {
> > + nread += n;
> > + }
> > + if (n < 0) {
> > + goto cleanup;
> > + }
> > + buf[nread] = '\0';
> > + char *endptr = NULL;
> > + *value = strtol(buf, &endptr, 16);
> > + // if we didn't read the entire string up to a newline,
> > something went
> > wrong.
> > + if (endptr == buf || *endptr != '\n') {
> > + goto cleanup;
> > + }
> > + result = true;
> > +
> > +cleanup:
> > + //closing the fd here causes an abort...
> > + //close(fd);
>
> I think this is due to the overflow in the main function.
Oops, I had intended to debug this but forgot about it before sending
the code for revew... ;)
>
> > + return result;
> > +}
> > +
> > +int main(int argc, char* argv[])
> > +{
> > + if (argc < 3) {
> > + if (argc == 2 && (strcmp(argv[1], "test") == 0)) {
> > + run_tests();
> > + printf("All tests succeeded\n");
> > + return EXIT_SUCCESS;
> > + }
> > + errx(EXIT_FAILURE, "Usage: %s PCIADDR DEVICEDISPLAYID",
> > argv[0]);
> > + }
> > + debug = (getenv("DEBUG") != NULL);
> > +
> > + // PCI address should be in the following format:
> > + // pci/$domain/$slot.$fn/$slot.$fn
> > + PciAddress *user_pci_addr =
> > parse_pci_address_from_spice(argv[1]);
> > + if (!user_pci_addr) {
> > + errx(EXIT_FAILURE, "Couldn't parse PCI address '%s'.
> > Address should
> > be the form 'pci/$domain/$slot.$fn/$slot.fn...", argv[1]);
> > + }
> > + int device_display_id = atoi(argv[2]);
> > +
> > + // look up xrandr outputs
> > + Display *xdisplay = XOpenDisplay(NULL);
> > + if (!xdisplay) {
> > + errx(EXIT_FAILURE, "Failed to open X dislay");
> > + }
> > +
> > + int rr_event_base, rr_error_base;
> > + if (!XRRQueryExtension(xdisplay, &rr_event_base,
> > &rr_error_base)) {
> > + errx(EXIT_FAILURE, "Failed to initialize XRandr
> > extension");
> > + }
> > +
> > + XRRScreenResources *xres =
> > XRRGetScreenResourcesCurrent(xdisplay,
> > DefaultRootWindow(xdisplay));
> > + if (!xres) {
> > + errx(EXIT_FAILURE, "Unable to get Xorg screen resources");
> > + }
> > +
> > + // Look for a device that matches the PCI address parsed
> > above. Loop
> > + // through the list of cards reported by the DRM subsytem
> > + bool found_device = false;
> > + 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
> > + 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);
> > + 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) {
> > + err(EXIT_FAILURE, "Failed to get the real path of %s",
> > sys_path);
> > + }
> > + 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) {
> > + DEBUG("Can't determine pci address from '%s'",
> > device_link);
> > + continue;
> > + }
> > +
> > + if (compare_addresses(user_pci_addr, drm_pci_addr)) {
> > + bool found_output = false;
> > + char vendor_id_path[150], device_id_path[150];
> > + snprintf(vendor_id_path, sizeof(vendor_id_path),
> > "%s/device/vendor", sys_path);
> > + int vendor_id = 0;
> > + if (!read_hex_value_from_file(vendor_id_path,
> > &vendor_id)) {
> > + DEBUG("Unable to read vendor ID of card\n");
> > + } else {
> > + DEBUG("Vendor id of this card is 0x%x\n",
> > vendor_id);
> > + }
> > + snprintf(device_id_path, sizeof(device_id_path),
> > "%s/device/device", sys_path);
> > + int device_id = 0;
> > + if (!read_hex_value_from_file(device_id_path,
> > &device_id)) {
> > + DEBUG("Unable to read device ID of card\n");
> > + } else {
> > + DEBUG("Device id of this card is 0x%x\n",
> > device_id);
> > + }
> > +
> > + int fd = open(dev_path, O_RDWR);
> > + if (fd < 0) {
> > + err(EXIT_FAILURE, "Unable to open file %s",
> > dev_path);
> > + }
> > +
> > + drmModeResPtr res = drmModeGetResources(fd);
> > + if (res) {
> > + // find the drm output that is equal to
> > device_display_id
> > + if (device_display_id >= res->count_connectors) {
> > + errx(EXIT_FAILURE, "Specified display id %i is
> > higher
> > than the maximum display id provided by this device (%i)",
> > + device_display_id, res->count_connectors
> > - 1);
> > + }
> > +
> > + drmModeConnectorPtr conn = drmModeGetConnector(fd,
> > res->connectors[device_display_id]);
> > + drmModeFreeResources(res);
> > + res = NULL;
> > +
> > + bool increment_xrandr_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 output
> > name will
> > need
> > + // to be incremented before comparing to the
> > expected
> > drm
> > + // connector 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
> > + if (i == 0) {
> > + // check the first output to see if it
> > ends with
> > '-0'
> > + char *postfix = oinfo->name +
> > (strlen(oinfo->name) - 2);
> > + if (strcmp(postfix, "-0") == 0) {
> > + DEBUG("Need to increment xrandr
> > name...\n");
> > + increment_xrandr_name = true;
> > + 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));
> > + } 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);
> > + char xname[100];
> > + // NOTE: this will increment the counter for
> > every
> > XRandr
> > + // output, not only the ones associated with
> > the current
> > + // device. This should only matter in the
> > unlikely
> > scenario
> > + // where X is configured with multiple
> > devices and the
> > + // incremented name of an output on another
> > device
> > + // conflicts with the name of the output we're
> > looking
> > for,
> > + if (increment_xrandr_name) {
> > + DEBUG("Original xrandr name is %s\n",
> > oinfo->name);
> > + char *id_pos = strrchr(oinfo->name, '-') +
> > 1;
> > + size_t prefix_len = id_pos - oinfo->name;
> > + memcpy(xname, oinfo->name, prefix_len);
> > + int newid = atoi(id_pos) + 1;
> > + DEBUG("New id suffix should be -%i\n",
> > newid);
> > + int sz = snprintf(xname + prefix_len,
> > sizeof(xname -
> > prefix_len), "%i", newid);
> > + if (sz < 0) {
> > + errx(EXIT_FAILURE, "Unable to
> > increment name");
> > + }
> > + xname[prefix_len + sz] = '\0';
> > + } else {
> > + strncpy(xname, oinfo->name, sizeof(xname)
> > - 1);
> > + xname[sizeof(xname)] = '\0';
> > + }
> > +
> > + DEBUG("Checking whether xrandr output %i (%s)
> > matches
> > expected name %s\n", i, xname, expected_name);
> > + if (strcmp(xname, expected_name) == 0) {
> > + 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;
> > + } else {
> > + if (vendor_id == PCI_VENDOR_ID_NVIDIA) {
>
> Why only Nvidia ?
Good question. I guess it's because I initially had a special-case for
NVidia (where I didn't even try to get the DRM resources if I detected
that the vendor was Nvidia). Then I realized that it's valid to use an
Nvidia device with an open-source driver that *does* support DRM, so I
moved the special-case to this 'else' block so that it was only done
when DRM resources were unavailable.
But as long as we are falling back to this behavior for NVidia, it's
probably fair to do the same for other non-Nvidia drivers that don't
support DRM.
It's important to note that this fallback only works if the given
device is the only one configured under X. Right now we just move
forward with the assumption that this is true. But I just realized that
perhaps we could guarantee this by looking up the number of "providers"
from xrandr (which requires version 1.4 of xrandr) and returning a
failure if there is more than one provider? This would need testing of
course, since I'm not sure whether a single device can have multiple
"providers" or not. Is this worth investigating?
>
> > + DEBUG("This device is an Nvidia device that
> > doesn't
> > provide information about drm connectors. We'll simply look up the
> > XRandr
> > output with index %i\n", device_display_id);
> > + // the proprietary nvidia driver 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_display_id >= xres->noutput) {
> > + errx(EXIT_FAILURE, "The device display id
> > %i does
> > not exist", device_display_id);
> > + }
> > + int oid = xres->outputs[device_display_id];
> > + XRROutputInfo *oinfo =
> > XRRGetOutputInfo(xdisplay, xres,
> > oid);
> > + printf(" Found matching X Output: name=%s
> > id=%i
> > (%s)\n",
> > + oinfo->name,
> > + (int)oid,
> > + connections[oinfo->connection + 1]);
> > + XRRFreeOutputInfo(oinfo);
> > + found_output = true;
> > + } else {
> > + errx(EXIT_FAILURE, "Unable to get DRM
> > resources for
> > card");
> > + }
> > + }
> > +
> > +
> > + if (!found_output) {
> > + errx(EXIT_FAILURE, "Couldn't find an XRandr output
> > for the
> > specified device");
> > + }
> > +
> > + // we found the right card, abort the loop
> > + found_device = true;
> > + break;
> > + } else {
> > + DEBUG("card addr '%s' does not match requested addr
> > '%s'\n",
> > device_link, argv[1]);
> > + }
> > + }
> > + XRRFreeScreenResources(xres);
> > + xres = NULL;
> > +
> > + if (!found_device) {
> > + errx(EXIT_FAILURE, "Couldn't find a card with the given
> > PCI
> > address");
> > + }
> > + return EXIT_SUCCESS;
> > +}
>
> Frediano
More information about the Spice-devel
mailing list