>
> 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. It assumes that the list of drm connectors (outputs) are
> in a stable order and therefore we can use the monitor ID as an index
> into the array of connectors.
Looking at the code one question is "What's the monitor ID?"
Is obviously an ID to distinguish monitors in a single device but
what is not clear is where it comes from. This is pretty important
if this value has to have some meaning for spice-server instead
of just being a opaque data that spice-server stores for the guest.
Consider the "VNC console", in this case Qemu will provide a QXL
interface for a physical vGPU that will be passed to the server.
At some point spice-server will have to know that this QXL interface
will be replaced by a stream. How can the server know that? Is
this monitor ID useful or the PCI ID is enough? Having the card
multiple outputs (but unknown by Qemu!) seems an issue this
monitor ID should solve.
The POC is pretty long but mostly the issue seems that the Xorg names
are "inspired" and not derived from DRM. I said inspired because it
seems every Xorg driver is able to register whatever name they want,
is just that usually they use DRM information.
In the code there are manually adjustment for:
- Virtual-XX outputs starting from 0 instead of 1 (like kernel names);
- conversion from numeric to string format using fixed lookup tables
(prone to change).
In my laptop I have a HDMI-1 output name however the card has HDMI-A-1
so it seems this POC would fail on my laptop. Well, as I said matching
thing "inspired" and not derived could be hard to maintain. Not saying
I have a solution, just sharing some though.
> ---
>
> This isn't intended to be committed to the repository, but it gives an
> example for how we might do this. This will be necessary to handle vgpu
> streaming functionality properly.
>
> To test, you can simply run the utility and pass a PCI address and
> monitor ID and it should print out the associated Xrandr output id. For
> example, on my laptop:
>
> $ ./src/get-xrandr-output 0000:00:02.0 0
> Device /dev/dri/card0 is at ../../../0000:00:02.0
> DRM connector for monitor 0: name=eDP-1 id=71 (*CONNECTED*)
> Found matching X Output: name=eDP-1 id=114 (*CONNECTED*)
>
I though the format of the PCI ID was different (a long string with
all the path), are we going to have a function to extract this new
"PCI address" from the PCI ID we discussed?
>
> Makefile.am | 16 +++
> configure.ac | 1 +
> src/vdagent/get-xrandr-output.c | 209
> ++++++++++++++++++++++++++++++++++++++++
> 3 files changed, 226 insertions(+)
> create mode 100644 src/vdagent/get-xrandr-output.c
>
> diff --git a/Makefile.am b/Makefile.am
> index 3e405bc..cdadb87 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,21 @@ 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) \
> + $(NULL)
> +
> +src_get_xrandr_output_LDADD = \
> + $(DRM_LIBS) \
> + $(X_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..2127e66
> --- /dev/null
> +++ b/src/vdagent/get-xrandr-output.c
> @@ -0,0 +1,209 @@
> +/* proof of concept for converting PCI address and monitor id to an xrandr
> + * output in the guest.
> + *
> + * \copyright
> + * Copyright 2018 Red Hat Inc. All rights reserved.
> + */
> +
> +#include <errno.h>
> +#include <fcntl.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>
> +
> +#define ERROR(...) \
> + fprintf(stderr, __VA_ARGS__); \
> + return EXIT_FAILURE;
> +
OT: I found err (see err.h) functions really useful for this.
> +// connector type strings from kernel /drivers/gpu/drm/drm_connector.c
> +// these *should* match the drm Xrandr output names
> +static const char *conn_type[] = {
Minor: const, I don't suppose we want to change this on the flight.
> + [ DRM_MODE_CONNECTOR_Unknown] = "Unknown" ,
> + [ 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-A" ,
> + [ 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" ,
> +};
> +
> +// FIXME: LICENSE?
I suppose we can ask Gerd
> +// borrowed from Gerd's drminfo
> +void drm_conn_name(drmModeConnector *conn, char *dest, int dlen)
Minor why not size_t dlen instead of int? snprintf expects size_t.
> +{
> + const char *type;
> +
> + if (conn->connector_type_id < sizeof(conn_type)/sizeof(conn_type[0]) &&
> + conn_type[conn->connector_type]) {
> + type = conn_type[conn->connector_type];
> + } else {
> + type = "unknown";
> + }
> + snprintf(dest, dlen, "%s-%d", type, conn->connector_type_id);
> +}
> +
> +static const char *connections[] = {"", "*CONNECTED*", "disconnected",
> "unknown connection"};
> +
> +int main(int argc, char* argv[])
> +{
> + if (argc < 3) {
> + ERROR("Usage: %s PCIADDR MONITORID", argv[0]);
> + }
> +
> + // PCI address should be in extended BDF notation
> + // https://wiki.xen.org/wiki/Bus:Device.Function_(BDF)_Notation
> + // FIXME: validate?
I don't think an issue for the POC
> + char* user_addr = argv[1];
> + int monitor_id = atoi(argv[2]);
> +
> + bool found = 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
> + 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);
> +
Minor: I suppose you can avoid this variable and just use sys_path_device
> + // the file /sys/class/drm/card0/device is a symlink to a file that
> + // specifies the device's address. It usually points to something
> + // like ../../../0000:00:02.0
> + char sys_path_device[100], device_link[100];
> + snprintf(sys_path_device, sizeof(sys_path_device), "%s/device",
> sys_path);
> + ssize_t res = readlink(sys_path_device, device_link,
> sizeof(device_link) - 1);
> + if (res == -1) {
> + ERROR(strerror(errno));
> + }
> + // readlink does not guarantee that the string is null-terminated
> + device_link[res] = '\0';
> + printf("Device %s is at %s\n", dev_path, device_link);
> +
> + char *card_addr = device_link;
> + // strip possible initial relative path
I think this is getting the file/directory name
> + char *delim = strrchr(device_link, '/');
> + if (delim != NULL) {
> + card_addr = delim + 1;
> + }
> +
> + if (strcmp(card_addr, user_addr) == 0) {
> + int fd;
> + fd = open(dev_path, O_RDWR);
> + if (fd < 0) {
> + ERROR(strerror(errno));
> + }
> +
> + drmModeResPtr res = drmModeGetResources(fd);
> + if (!res) {
> + ERROR("Unable to get resources for card");
> + }
> +
> + // find the drm output that is equal to monitor_id
> + /* FIXME: verify that monitor id is 0-based */
I suppose if up to us to define the monitor ID range.
> + if (res->count_connectors < (monitor_id + 1)) {
> + ERROR("Unable to find output");
> + }
> +
> + drmModeConnectorPtr conn = drmModeGetConnector(fd,
> res->connectors[monitor_id]);
> + char cname[64];
> + drm_conn_name(conn, cname, sizeof(cname));
> + printf("DRM connector for monitor %i: name=%s id=%u (%s)\n",
> + monitor_id, cname, conn->connector_id,
> + connections[conn->connection]);
> +
> + drmModeFreeConnector(conn);
> + conn = NULL;
> + drmModeFreeResources(res);
> + res = NULL;
> +
> + // now look up all xrandr outputs
> + Display *xdisplay = XOpenDisplay(NULL);
This should be open once, not at each iteration.
> + if (!xdisplay) {
> + ERROR("Failed to open X dislay");
> + }
> +
> + int rr_event_base, rr_error_base;
> + if (!XRRQueryExtension(xdisplay, &rr_event_base,
> &rr_error_base)) {
> + ERROR("Failed to initialize XRandr extension");
> + }
> +
> + XRRScreenResources *xres =
> XRRGetScreenResourcesCurrent(xdisplay, DefaultRootWindow(xdisplay));
Also this can be computed out of the loop of cards.
> + if (!xres) {
> + ERROR("Unable to get screen resources");
> + }
> +
> + // Loop through xrandr outputs and check whether the xrandr
> + // output name matches the drm connector name
> + bool increment_xrandr_name = false;
> + for (int i = 0; i < xres->noutput; ++i) {
> + int oid = xres->outputs[i];
> + XRROutputInfo *oinfo = XRRGetOutputInfo(xdisplay, xres,
> oid);
> + // 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) {
This does not support different order of detecting card, the array should be scan
in advance to check if given name starts with 0 or 1, would be better to check
for every name, not just QXL. If Xorg is configured to see both QXL and vGPU and
vGPU is detected first (as an example) the first xrandr names won't be QXL.
> + // check the first output to see if it ends with '-0'
> + char *postfix = oinfo->name + (strlen(oinfo->name) - 2);
> + if (strcmp(postfix, "-0") == 0) {
> + increment_xrandr_name = true;
> + }
> + }
> + char xname[100];
> + if (increment_xrandr_name) {
> + char *id_pos = strrchr(oinfo->name, '-') + 1;
> + int newid = atoi(id_pos) + 1;
> + if (snprintf(xname, sizeof(xname), "%s%i", oinfo->name,
> newid) < 0) {
> + ERROR("Unable to increment name");
> + }
I think this is producing Virtual-0-1 instead of the wanted Virtual-1
> + } else {
> + strncpy(xname, oinfo->name, sizeof(xname) - 1);
Minor: strncpy sometimes does not terminate the string
> + }
> + if (strcmp(xname, cname) == 0) {
> + printf(" Found matching X Output: name=%s id=%i
> (%s)\n",
> + oinfo->name,
> + (int)oid,
> + connections[oinfo->connection + 1]);
> + XRRFreeOutputInfo(oinfo);
> + break;
> + }
> + XRRFreeOutputInfo(oinfo);
> + }
> +
> + XRRFreeScreenResources(xres);
> + xres = NULL;
> +
> + // we found the right card, abort the loop
> + found = true;
> + break;
> + }
> + }
> + if (!found) {
> + ERROR("Couldn't find a card with the given PCI address");
> + return EXIT_SUCCESS;
> + }
> +}
Frediano
_______________________________________________
Spice-devel mailing list
Spice-devel@xxxxxxxxxxxxxxxxxxxxx
https://lists.freedesktop.org/mailman/listinfo/spice-devel