[Qemu-devel] [RFC PATCH spice/qemu 0/2] QXL interface to set monitor ID

[Qemu-devel] [RFC PATCH spice/qemu 0/2] QXL interface to set monitor ID

[Lukáš Hrázký]

Hello,

this is a preliminary RFC on the new QXL interface to identify the
graphics device monitors in the guest. This interface adds two functions
that allow to set the device path (e.g. a PCI path) and the IDs of the
device's displays from QEMU to SPICE server. The server will forward
this information to the guest agent, which can use it to identify the
displays in the guest context.

The mechanism, as well as the actual format of the pci path is in
accordance to what Gerd described in an earlier email [1].

Cheers,
Lukas

[1] https://lists.freedesktop.org/archives/spice-devel/2018-August/045465.html


-- 
2.19.1

[Qemu-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

Adds two functions to let QEMU provide information to identify graphics
devices and their monitors in the guest:

* device path - The path identifying the device on the system (e.g. PCI
  path):
  spice_qxl_device_set_path(...)

* device display ID - The index of the monitor on the graphics device:
  spice_qxl_device_monitor_add_display_id(...)

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 server/red-qxl.c         | 67 ++++++++++++++++++++++++++++++++++++++++
 server/spice-qxl.h       |  3 ++
 server/spice-server.syms |  6 ++++
 3 files changed, 76 insertions(+)

diff --git a/server/red-qxl.c b/server/red-qxl.c
index 97940611..143228ca 100644
--- a/server/red-qxl.c
+++ b/server/red-qxl.c
@@ -41,6 +41,9 @@
 #include "red-qxl.h"
 
 
+#define MAX_PATH_LEN 256
+#define MAX_MONITORS_COUNT 16
+
 struct QXLState {
     QXLWorker qxl_worker;
     QXLInstance *qxl;
@@ -53,6 +56,9 @@ struct QXLState {
     unsigned int max_monitors;
     RedsState *reds;
     RedWorker *worker;
+    char device_path[MAX_PATH_LEN];
+    uint32_t device_display_ids[MAX_MONITORS_COUNT];
+    size_t monitors_count;  // length of ^^^
 
     pthread_mutex_t scanout_mutex;
     SpiceMsgDisplayGlScanoutUnix scanout;
@@ -846,6 +852,67 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
     red_qxl_async_complete(qxl, cookie);
 }
 
+/**
+ * spice_qxl_device_set_path:
+ * @instance the QXL instance to set the path to
+ * @device_path the path of the device this QXL instance belongs to
+ *
+ * Sets the hardware (e.g. PCI) path of the graphics device represented by this QXL interface instance.
+ *
+ */
+SPICE_GNUC_VISIBLE
+void spice_qxl_device_set_path(QXLInstance *instance, const char *device_path)
+{
+    g_return_if_fail(device_path != NULL);
+
+    size_t dp_len = strnlen(device_path, MAX_PATH_LEN);
+    if (dp_len >= MAX_PATH_LEN) {
+        spice_error("PCI device path too long: %lu > %u", dp_len, MAX_PATH_LEN);
+        return;
+    }
+
+    strncpy(instance->st->device_path, device_path, MAX_PATH_LEN);
+
+    g_debug("QXL Instance %d setting device PCI path \"%s\"", instance->id, device_path);
+}
+
+/**
+ * spice_qxl_device_monitor_add_display_id:
+ * @instance the QXL instance to add the display ID to
+ * @device_display_id the display ID to add to the QXL instance
+ *
+ * Adds a #device_display_id to this QXL interface instance. The
+ * #device_display_id is an index (a sequence starting from 0) into the list of
+ * all the displays on the graphics device. This function should be called in
+ * sequence for all possible displays of the device. The ID of the first call
+ * will belong to #monitor_id 0, and so forth in the sequence.
+ *
+ * Since for some graphics devices (e.g. virtio-gpu) a QXL interface instance
+ * is created per each display of a single device, you can get e.g. the single
+ * following call on the third QXL interface instance for this device:
+ *
+ * spice_qxl_device_monitor_add_display_id(instance, 2);
+ *
+ * This tells you that the single monitor of this QXL interface actually
+ * belongs to the 3rd display of the underlying device.
+ *
+ * Returns: The actual SPICE server monitor_id associated with the display
+ */
+SPICE_GNUC_VISIBLE
+int spice_qxl_device_monitor_add_display_id(QXLInstance *instance, uint32_t device_display_id)
+{
+    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
+        spice_error("Cannot register more than %u monitors per QXL interface", MAX_MONITORS_COUNT);
+        return -1;
+    }
+
+    instance->st->device_display_ids[instance->st->monitors_count] = device_display_id;
+
+    g_debug("QXL Instance %d adding device display ID %u", instance->id, device_display_id);
+
+    return instance->st->monitors_count++;
+}
+
 void red_qxl_init(RedsState *reds, QXLInstance *qxl)
 {
     QXLState *qxl_state;
diff --git a/server/spice-qxl.h b/server/spice-qxl.h
index 0c4e75fc..ae2d5975 100644
--- a/server/spice-qxl.h
+++ b/server/spice-qxl.h
@@ -114,6 +114,9 @@ void spice_qxl_gl_draw_async(QXLInstance *instance,
                              uint32_t x, uint32_t y,
                              uint32_t w, uint32_t h,
                              uint64_t cookie);
+/* since spice 0.14.2 */
+void spice_qxl_device_set_path(QXLInstance *instance, const char *device_path);
+int spice_qxl_device_monitor_add_display_id(QXLInstance *instance, uint32_t device_display_id);
 
 typedef struct QXLDevInitInfo {
     uint32_t num_memslots_groups;
diff --git a/server/spice-server.syms b/server/spice-server.syms
index edf04a42..fdeafd23 100644
--- a/server/spice-server.syms
+++ b/server/spice-server.syms
@@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
 global:
     spice_server_set_video_codecs;
 } SPICE_SERVER_0.13.1;
+
+SPICE_SERVER_0.14.2 {
+global:
+    spice_qxl_device_set_path;
+    spice_qxl_device_monitor_add_display_id;
+} SPICE_SERVER_0.13.2;
-- 
2.19.1

[Qemu-devel] [RFC PATCH qemu 2/2] spice: set PCI path and device display ID in QXL interface

[Lukáš Hrázký]

Calls new SPICE QXL interface functions to set:

* The hardware path of the graphics device represented by the QXL
  interface (e.g. a PCI path):
  spice_qxl_device_set_path(...)

* The device display IDs (the IDs of the device's monitors that belong
  to this QXL interface):
  spice_qxl_device_monitor_add_display_id(...)

Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
---
 hw/display/qxl.c   |  8 ++++++++
 ui/spice-core.c    | 45 ++++++++++++++++++++++++++++++++++++++++++++-
 ui/spice-display.c |  5 +++++
 3 files changed, 57 insertions(+), 1 deletion(-)

diff --git a/hw/display/qxl.c b/hw/display/qxl.c
index e628cf1286..cdce678046 100644
--- a/hw/display/qxl.c
+++ b/hw/display/qxl.c
@@ -2184,6 +2184,14 @@ static void qxl_realize_common(PCIQXLDevice *qxl, Error **errp)
                    SPICE_INTERFACE_QXL_MAJOR, SPICE_INTERFACE_QXL_MINOR);
         return;
     }
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    // register all possible device display IDs to SPICE server
+    for (int i = 0; i < qxl->max_outputs; ++i) {
+        spice_qxl_device_monitor_add_display_id(&qxl->ssd.qxl, i);
+    }
+#endif
+
     qemu_add_vm_change_state_handler(qxl_vm_change_state_handler, qxl);
 
     qxl->update_irq = qemu_bh_new(qxl_update_irq_bh, qxl);
diff --git a/ui/spice-core.c b/ui/spice-core.c
index a4fbbc3898..eb1fb17174 100644
--- a/ui/spice-core.c
+++ b/ui/spice-core.c
@@ -35,6 +35,8 @@
 #include "qemu/option.h"
 #include "migration/misc.h"
 #include "hw/hw.h"
+#include "hw/pci/pci.h"
+#include "hw/pci/pci_bus.h"
 #include "ui/spice-display.h"
 
 /* core bits */
@@ -871,6 +873,26 @@ bool qemu_spice_have_display_interface(QemuConsole *con)
     return false;
 }
 
+/*
+ * Recursively (in reverse order) appends addresses of PCI devices as it moves
+ * up in the PCI hierarchy.
+ *
+ * @returns true on success, false when the buffer wasn't large enough
+ */
+static bool append_pci_address_recursive(char *buf, size_t buf_size, const PCIDevice *pci)
+{
+    PCIBus *bus = pci_get_bus(pci);
+    if (!pci_bus_is_root(bus)) {
+        append_pci_address_recursive(buf, buf_size, bus->parent_dev);
+    }
+
+    size_t len = strlen(buf);
+    size_t written = snprintf(buf + len, buf_size - len, "/%02x.%x",
+        PCI_SLOT(pci->devfn), PCI_FUNC(pci->devfn));
+
+    return written > 0 && written < buf_size - len;
+}
+
 int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
 {
     if (g_slist_find(spice_consoles, con)) {
@@ -878,7 +900,28 @@ int qemu_spice_add_display_interface(QXLInstance *qxlin, QemuConsole *con)
     }
     qxlin->id = qemu_console_get_index(con);
     spice_consoles = g_slist_append(spice_consoles, con);
-    return qemu_spice_add_interface(&qxlin->base);
+    int res = qemu_spice_add_interface(&qxlin->base);
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    DeviceState *dev = DEVICE(object_property_get_link(OBJECT(con), "device", &error_abort));
+    PCIDevice *pci = (PCIDevice *) object_dynamic_cast(OBJECT(dev), TYPE_PCI_DEVICE);
+
+    if (pci == NULL) {
+        warn_report("Setting PCI path of a display device to SPICE: Not a PCI device.");
+        return res;
+    }
+
+    char device_path[128] = "pci/0000";  // hardcoded PCI domain 0000
+    if (!append_pci_address_recursive(device_path, sizeof(device_path), pci)) {
+        warn_report("Setting PCI path of a display device to SPICE: "
+            "Too many PCI devices in the chain.");
+        return res;
+    }
+
+    spice_qxl_device_set_path(qxlin, device_path);
+#endif
+
+    return res;
 }
 
 static int qemu_spice_set_ticket(bool fail_if_conn, bool disconnect_if_conn)
diff --git a/ui/spice-display.c b/ui/spice-display.c
index 2f8adb6b9f..9c0d1b3fd2 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -1129,6 +1129,11 @@ static void qemu_spice_display_init_one(QemuConsole *con)
 
     ssd->qxl.base.sif = &dpy_interface.base;
     qemu_spice_add_display_interface(&ssd->qxl, con);
+
+#if SPICE_SERVER_VERSION >= 0x000e02 /* release 0.14.2 */
+    spice_qxl_device_monitor_add_display_id(&ssd->qxl, qemu_console_get_head(con));
+#endif
+
     qemu_spice_create_host_memslot(ssd);
 
     register_displaychangelistener(&ssd->dcl);
-- 
2.19.1

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Jonathon Jongsma]

On Tue, 2018-10-09 at 15:10 +0200, Lukáš Hrázký wrote:
> Adds two functions to let QEMU provide information to identify
> graphics
> devices and their monitors in the guest:
> 
> * device path - The path identifying the device on the system (e.g.
> PCI
>   path):
>   spice_qxl_device_set_path(...)
> 
> * device display ID - The index of the monitor on the graphics
> device:
>   spice_qxl_device_monitor_add_display_id(...)
> 
> Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> ---
>  server/red-qxl.c         | 67
> ++++++++++++++++++++++++++++++++++++++++
>  server/spice-qxl.h       |  3 ++
>  server/spice-server.syms |  6 ++++
>  3 files changed, 76 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..143228ca 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
>  #include "red-qxl.h"
>  
>  
> +#define MAX_PATH_LEN 256
> +#define MAX_MONITORS_COUNT 16
> +
>  struct QXLState {
>      QXLWorker qxl_worker;
>      QXLInstance *qxl;
> @@ -53,6 +56,9 @@ struct QXLState {
>      unsigned int max_monitors;
>      RedsState *reds;
>      RedWorker *worker;
> +    char device_path[MAX_PATH_LEN];
> +    uint32_t device_display_ids[MAX_MONITORS_COUNT];
> +    size_t monitors_count;  // length of ^^^
>  
>      pthread_mutex_t scanout_mutex;
>      SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -846,6 +852,67 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> *qxl)
>      red_qxl_async_complete(qxl, cookie);
>  }
>  
> +/**
> + * spice_qxl_device_set_path:

It seems to me that _set_device_path() might be a better name than
_device_set_path(). And maybe _address is better than _path? 

> + * @instance the QXL instance to set the path to
> + * @device_path the path of the device this QXL instance belongs to

It seems to me that if this is public API, the format of the
@device_path should be documented a bit better?

> + *
> + * Sets the hardware (e.g. PCI) path of the graphics device
> represented by this QXL interface instance.

So, this comment suggests that the caller might be able to provide a
path that is not a PCI path. But the implementation below (mostly the
debug output, I suppose...) assumes a PCI path. Do we need to support
non-PCI paths?

> + *
> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_device_set_path(QXLInstance *instance, const char
> *device_path)
> +{
> +    g_return_if_fail(device_path != NULL);
> +
> +    size_t dp_len = strnlen(device_path, MAX_PATH_LEN);
> +    if (dp_len >= MAX_PATH_LEN) {
> +        spice_error("PCI device path too long: %lu > %u", dp_len,
> MAX_PATH_LEN);
> +        return;
> +    }
> +
> +    strncpy(instance->st->device_path, device_path, MAX_PATH_LEN);
> +
> +    g_debug("QXL Instance %d setting device PCI path \"%s\"",
> instance->id, device_path);
> +}
> +
> +/**
> + * spice_qxl_device_monitor_add_display_id:

I'm not sure that the word 'device' in this function adds much here. It
is essentially operating on the QxlInstance, which may represent a
single device, or may only represent part of a device (e.g. virtio-
gpu). So I think including 'device' in the name actually makes things
less clear. I'm also unclear why the word 'monitor' is here. As
written, it could be interpreted as refering to a "device monitor",
whatever that is. But it means that we have the word 'monitor' and
'display' within the same function name, which adds more confusion. 

I would suggest a name like spice_qxl_add_device_display_id().

But part of me also doesn't like the verb 'add' here either. Because
we're not really adding anything by calling this function. The caller
is simply informing the spice server that this qxl instance provides
the specified display. So I might even consider a name like
spice_qxl_provides_device_display_id()... Or maybe
_register_display_id(). Maybe there are better options, though.

> + * @instance the QXL instance to add the display ID to
> + * @device_display_id the display ID to add to the QXL instance
> + *
> + * Adds a #device_display_id to this QXL interface instance. The
> + * #device_display_id is an index (a sequence starting from 0) into
> the list of
> + * all the displays on the graphics device. This function should be
> called in
> + * sequence for all possible displays of the device. The ID of the
> first call
> + * will belong to #monitor_id 0, and so forth in the sequence.
> + *
> + * Since for some graphics devices (e.g. virtio-gpu) a QXL interface
> instance
> + * is created per each display of a single device, you can get e.g.
> the single
> + * following call on the third QXL interface instance for this
> device:
> + *
> + * spice_qxl_device_monitor_add_display_id(instance, 2);
> + *
> + * This tells you that the single monitor of this QXL interface
> actually
> + * belongs to the 3rd display of the underlying device.
> + *
> + * Returns: The actual SPICE server monitor_id associated with the
> display

So, if I remember correctly, Gerd recommended returning this value from
the function. But I think it needs more explanation here. What exactly
is a "monitor_id" supposed to represent? It is not used in your follow-
up qemu patch so it's hard to tell.

> + */
> +SPICE_GNUC_VISIBLE
> +int spice_qxl_device_monitor_add_display_id(QXLInstance *instance,
> uint32_t device_display_id)
> +{
> +    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
> +        spice_error("Cannot register more than %u monitors per QXL
> interface", MAX_MONITORS_COUNT);
> +        return -1;
> +    }
> +
> +    instance->st->device_display_ids[instance->st->monitors_count] =
> device_display_id;
> +
> +    g_debug("QXL Instance %d adding device display ID %u", instance-
> >id, device_display_id);
> +
> +    return instance->st->monitors_count++;
> +}

So, as far as I can tell, when you have e.g. a virtio-gpu device with
four monitors, you'll call this function 4 times, with the following
arguments:

spice_qxl_device_monitor_add_display_id(qxl1, 0);
spice_qxl_device_monitor_add_display_id(qxl2, 1);
spice_qxl_device_monitor_add_display_id(qxl3, 2);
spice_qxl_device_monitor_add_display_id(qxl4, 3);

And each of these calls would return a monitor_id of 0. Is that what
you expect? Or am I misreading this?

The other thing about this API, however, is that it seems pretty easy
to misuse. In general, I think that one of the marks of a good API is
that it's easy to use and difficult to misuse. At the moment, it seems
to require a lot of documentation explaining exactly how and when you
should call this function in order to get the right behavior.

If the caller (for whatever reason) executes the same call twice, what
happens?

spice_qxl_device_monitor_add_display_id(qxl, 0);
spice_qxl_device_monitor_add_display_id(qxl, 0);

Or if it calls them in the reverse order:

spice_qxl_device_monitor_add_display_id(qxl, 3);
spice_qxl_device_monitor_add_display_id(qxl, 2);
spice_qxl_device_monitor_add_display_id(qxl, 1);
spice_qxl_device_monitor_add_display_id(qxl, 0);

Granted, some of this could probably be solved by better internal error
handling and validation, but it makes me think that there may be a
better interface.

Maybe that better interface is something similar to the one suggested
by Gerd. Something like:

spice_qxl_register_display_ids(QxlInstance *qxl, uint32_t first_id,
uint32_t n_ids);

Or maybe not. I'm still thinking it over.

Jonathon


> +
>  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
>  {
>      QXLState *qxl_state;
> diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> index 0c4e75fc..ae2d5975 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -114,6 +114,9 @@ void spice_qxl_gl_draw_async(QXLInstance
> *instance,
>                               uint32_t x, uint32_t y,
>                               uint32_t w, uint32_t h,
>                               uint64_t cookie);
> +/* since spice 0.14.2 */
> +void spice_qxl_device_set_path(QXLInstance *instance, const char
> *device_path);
> +int spice_qxl_device_monitor_add_display_id(QXLInstance *instance,
> uint32_t device_display_id);
>  
>  typedef struct QXLDevInitInfo {
>      uint32_t num_memslots_groups;
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index edf04a42..fdeafd23 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
>  global:
>      spice_server_set_video_codecs;
>  } SPICE_SERVER_0.13.1;
> +
> +SPICE_SERVER_0.14.2 {
> +global:
> +    spice_qxl_device_set_path;
> +    spice_qxl_device_monitor_add_display_id;
> +} SPICE_SERVER_0.13.2;

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

  Hi,

> > + * Sets the hardware (e.g. PCI) path of the graphics device
> > represented by this QXL interface instance.
> 
> So, this comment suggests that the caller might be able to provide a
> path that is not a PCI path. But the implementation below (mostly the
> debug output, I suppose...) assumes a PCI path. Do we need to support
> non-PCI paths?

Certainly not for the initial revision, maybe never.

But thanks to the "pci/" prefix we should be able to add support for
other paths later in case it turns out we need it.

> > + * Returns: The actual SPICE server monitor_id associated with the
> > display
> 
> So, if I remember correctly, Gerd recommended returning this value from
> the function. But I think it needs more explanation here. What exactly
> is a "monitor_id" supposed to represent? It is not used in your follow-
> up qemu patch so it's hard to tell.

IIRC the plan was to ditch the global monior_id idea and use the
(channel_id, display_id) tuple everywhere ...

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Tue, 2018-10-09 at 14:33 -0500, Jonathon Jongsma wrote:
> On Tue, 2018-10-09 at 15:10 +0200, Lukáš Hrázký wrote:
> > Adds two functions to let QEMU provide information to identify
> > graphics
> > devices and their monitors in the guest:
> > 
> > * device path - The path identifying the device on the system (e.g.
> > PCI
> >   path):
> >   spice_qxl_device_set_path(...)
> > 
> > * device display ID - The index of the monitor on the graphics
> > device:
> >   spice_qxl_device_monitor_add_display_id(...)
> > 
> > Signed-off-by: Lukáš Hrázký <lhrazky@redhat.com>
> > ---
> >  server/red-qxl.c         | 67
> > ++++++++++++++++++++++++++++++++++++++++
> >  server/spice-qxl.h       |  3 ++
> >  server/spice-server.syms |  6 ++++
> >  3 files changed, 76 insertions(+)
> > 
> > diff --git a/server/red-qxl.c b/server/red-qxl.c
> > index 97940611..143228ca 100644
> > --- a/server/red-qxl.c
> > +++ b/server/red-qxl.c
> > @@ -41,6 +41,9 @@
> >  #include "red-qxl.h"
> >  
> >  
> > +#define MAX_PATH_LEN 256
> > +#define MAX_MONITORS_COUNT 16
> > +
> >  struct QXLState {
> >      QXLWorker qxl_worker;
> >      QXLInstance *qxl;
> > @@ -53,6 +56,9 @@ struct QXLState {
> >      unsigned int max_monitors;
> >      RedsState *reds;
> >      RedWorker *worker;
> > +    char device_path[MAX_PATH_LEN];
> > +    uint32_t device_display_ids[MAX_MONITORS_COUNT];
> > +    size_t monitors_count;  // length of ^^^
> >  
> >      pthread_mutex_t scanout_mutex;
> >      SpiceMsgDisplayGlScanoutUnix scanout;
> > @@ -846,6 +852,67 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> > *qxl)
> >      red_qxl_async_complete(qxl, cookie);
> >  }
> >  
> > +/**
> > + * spice_qxl_device_set_path:
> 
> It seems to me that _set_device_path() might be a better name than
> _device_set_path().

I've tried to keep the prefix notation that is usually used in C to
denote 'object' hierarchy, so that it's roughly equivalent to
spice.qxl.device.set_path(). It seemed to me the way to go to keep the
naming consistent. But there is really no device 'object' here, so
maybe your way reads better.

> And maybe _address is better than _path? 

Yeah, maybe. The contents of the string in it's current form is not
really much of an address, but path may be too generic and misleading.

> > + * @instance the QXL instance to set the path to
> > + * @device_path the path of the device this QXL instance belongs to
> 
> It seems to me that if this is public API, the format of the
> @device_path should be documented a bit better?

You're right, I should add that. Lets first agree on the contents and
I'll document it better.

> > + *
> > + * Sets the hardware (e.g. PCI) path of the graphics device
> > represented by this QXL interface instance.
> 
> So, this comment suggests that the caller might be able to provide a
> path that is not a PCI path. But the implementation below (mostly the
> debug output, I suppose...) assumes a PCI path. Do we need to support
> non-PCI paths?

I've missed the debug message below. At first I had the implementation
explicitly stating PCI, but then I realized there may eventually be
another interface, and the actual content of the variable starts with
"pci/", so that it allows to use something else in the future. So even
though now we don't have anything but PCI, we don't need and should not
limit the implementation to it.

> > + *
> > + */
> > +SPICE_GNUC_VISIBLE
> > +void spice_qxl_device_set_path(QXLInstance *instance, const char
> > *device_path)
> > +{
> > +    g_return_if_fail(device_path != NULL);
> > +
> > +    size_t dp_len = strnlen(device_path, MAX_PATH_LEN);
> > +    if (dp_len >= MAX_PATH_LEN) {
> > +        spice_error("PCI device path too long: %lu > %u", dp_len,
> > MAX_PATH_LEN);
> > +        return;
> > +    }
> > +
> > +    strncpy(instance->st->device_path, device_path, MAX_PATH_LEN);
> > +
> > +    g_debug("QXL Instance %d setting device PCI path \"%s\"",
> > instance->id, device_path);
> > +}
> > +
> > +/**
> > + * spice_qxl_device_monitor_add_display_id:
> 
> I'm not sure that the word 'device' in this function adds much here. It
> is essentially operating on the QxlInstance, which may represent a
> single device, or may only represent part of a device (e.g. virtio-
> gpu). So I think including 'device' in the name actually makes things
> less clear. I'm also unclear why the word 'monitor' is here. As
> written, it could be interpreted as refering to a "device monitor",
> whatever that is. But it means that we have the word 'monitor' and
> 'display' within the same function name, which adds more confusion. 

As above, I've tried to keep the prefix notation based on the object
hierarchy, although here it has gotten quite convoluted :) The word
"monitor" is there because it adds a display_id that belongs to a
certain monitor_id in the SPICE server. The function does have
problems, as you stated below.

> I would suggest a name like spice_qxl_add_device_display_id().
> 
> But part of me also doesn't like the verb 'add' here either. Because
> we're not really adding anything by calling this function. The caller
> is simply informing the spice server that this qxl instance provides
> the specified display. So I might even consider a name like
> spice_qxl_provides_device_display_id()... Or maybe
> _register_display_id(). Maybe there are better options, though.

I was thinking about using "register" too.

> > + * @instance the QXL instance to add the display ID to
> > + * @device_display_id the display ID to add to the QXL instance
> > + *
> > + * Adds a #device_display_id to this QXL interface instance. The
> > + * #device_display_id is an index (a sequence starting from 0) into
> > the list of
> > + * all the displays on the graphics device. This function should be
> > called in
> > + * sequence for all possible displays of the device. The ID of the
> > first call
> > + * will belong to #monitor_id 0, and so forth in the sequence.
> > + *
> > + * Since for some graphics devices (e.g. virtio-gpu) a QXL interface
> > instance
> > + * is created per each display of a single device, you can get e.g.
> > the single
> > + * following call on the third QXL interface instance for this
> > device:
> > + *
> > + * spice_qxl_device_monitor_add_display_id(instance, 2);
> > + *
> > + * This tells you that the single monitor of this QXL interface
> > actually
> > + * belongs to the 3rd display of the underlying device.
> > + *
> > + * Returns: The actual SPICE server monitor_id associated with the
> > display
> 
> So, if I remember correctly, Gerd recommended returning this value from
> the function. But I think it needs more explanation here. What exactly
> is a "monitor_id" supposed to represent? It is not used in your follow-
> up qemu patch so it's hard to tell.

It's supposed to be the actual monitor_id that we use in SPICE to
identify the monitors. I've just spent quite some time looking up where
the monitor_id actually comes from, and it's actually set all the way
down in the driver (either xf86-video-qxl or the KMS driver in the
kernel) and passed through the monitors_config functions to SPICE
server.

And the ID is actually the internal head ID from the driver or
something like that (I don't really understand the code well enough).

Interstingly enough, that seems to be the ID we want to have in the
device_display_id attribute. I expect (still need to look that up, I'm
out of time right now) that for virtio-gpu the ID is a bit different,
because if it was the internal head ID, it would not always start at 0
and that would actually break the display_id = channel_id + monitor_id
mechanism, ironically enough.

And yeah, I didn't use the id in the QEMU patches, as I didn't know
how, I expect Gerd to have some grand plans for it :)

> > + */
> > +SPICE_GNUC_VISIBLE
> > +int spice_qxl_device_monitor_add_display_id(QXLInstance *instance,
> > uint32_t device_display_id)
> > +{
> > +    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
> > +        spice_error("Cannot register more than %u monitors per QXL
> > interface", MAX_MONITORS_COUNT);
> > +        return -1;
> > +    }
> > +
> > +    instance->st->device_display_ids[instance->st->monitors_count] =
> > device_display_id;
> > +
> > +    g_debug("QXL Instance %d adding device display ID %u", instance-
> > > id, device_display_id);
> > 
> > +
> > +    return instance->st->monitors_count++;
> > +}
> 
> So, as far as I can tell, when you have e.g. a virtio-gpu device with
> four monitors, you'll call this function 4 times, with the following
> arguments:
> 
> spice_qxl_device_monitor_add_display_id(qxl1, 0);
> spice_qxl_device_monitor_add_display_id(qxl2, 1);
> spice_qxl_device_monitor_add_display_id(qxl3, 2);
> spice_qxl_device_monitor_add_display_id(qxl4, 3);
> 
> And each of these calls would return a monitor_id of 0. Is that what
> you expect? Or am I misreading this?

Yes, that's right.

> The other thing about this API, however, is that it seems pretty easy
> to misuse. In general, I think that one of the marks of a good API is
> that it's easy to use and difficult to misuse. At the moment, it seems
> to require a lot of documentation explaining exactly how and when you
> should call this function in order to get the right behavior.

I couldn't agree with you more, but the current API is already falling
quite short in this regard. The problem is a monitor and its monitor_id
is not really a thing until a monitors_config is sent from QEMU to
spice server where it suddenly appears. Before that, you have no notion
of monitors and them having any IDs. Therefore you can't really
associate any information with them explicitly.

> If the caller (for whatever reason) executes the same call twice, what
> happens?
> 
> spice_qxl_device_monitor_add_display_id(qxl, 0);
> spice_qxl_device_monitor_add_display_id(qxl, 0);
> 
> Or if it calls them in the reverse order:
> 
> spice_qxl_device_monitor_add_display_id(qxl, 3);
> spice_qxl_device_monitor_add_display_id(qxl, 2);
> spice_qxl_device_monitor_add_display_id(qxl, 1);
> spice_qxl_device_monitor_add_display_id(qxl, 0);
> 
> Granted, some of this could probably be solved by better internal error
> handling and validation, but it makes me think that there may be a
> better interface.
> 
> Maybe that better interface is something similar to the one suggested
> by Gerd. Something like:
> 
> spice_qxl_register_display_ids(QxlInstance *qxl, uint32_t first_id,
> uint32_t n_ids);

Yeah, maybe. I don't find either version a good API overall...

Thanks for the good comments,
Lukas

> Or maybe not. I'm still thinking it over.
> 
> Jonathon
> 
> 
> > +
> >  void red_qxl_init(RedsState *reds, QXLInstance *qxl)
> >  {
> >      QXLState *qxl_state;
> > diff --git a/server/spice-qxl.h b/server/spice-qxl.h
> > index 0c4e75fc..ae2d5975 100644
> > --- a/server/spice-qxl.h
> > +++ b/server/spice-qxl.h
> > @@ -114,6 +114,9 @@ void spice_qxl_gl_draw_async(QXLInstance
> > *instance,
> >                               uint32_t x, uint32_t y,
> >                               uint32_t w, uint32_t h,
> >                               uint64_t cookie);
> > +/* since spice 0.14.2 */
> > +void spice_qxl_device_set_path(QXLInstance *instance, const char
> > *device_path);
> > +int spice_qxl_device_monitor_add_display_id(QXLInstance *instance,
> > uint32_t device_display_id);
> >  
> >  typedef struct QXLDevInitInfo {
> >      uint32_t num_memslots_groups;
> > diff --git a/server/spice-server.syms b/server/spice-server.syms
> > index edf04a42..fdeafd23 100644
> > --- a/server/spice-server.syms
> > +++ b/server/spice-server.syms
> > @@ -173,3 +173,9 @@ SPICE_SERVER_0.13.2 {
> >  global:
> >      spice_server_set_video_codecs;
> >  } SPICE_SERVER_0.13.1;
> > +
> > +SPICE_SERVER_0.14.2 {
> > +global:
> > +    spice_qxl_device_set_path;
> > +    spice_qxl_device_monitor_add_display_id;
> > +} SPICE_SERVER_0.13.2;

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> > So, if I remember correctly, Gerd recommended returning this value from
> > the function. But I think it needs more explanation here. What exactly
> > is a "monitor_id" supposed to represent? It is not used in your follow-
> > up qemu patch so it's hard to tell.
> 
> It's supposed to be the actual monitor_id that we use in SPICE to
> identify the monitors. I've just spent quite some time looking up where
> the monitor_id actually comes from, and it's actually set all the way
> down in the driver (either xf86-video-qxl or the KMS driver in the
> kernel) and passed through the monitors_config functions to SPICE
> server.

How does all this monitors_config work currently in case multiple
display channels are present?

There is the QXLInterface->client_monitors_config() callback.

There is the spice_qxl_monitors_config_async() function.

Both are linked to a display channel.  The server/client messages go
through the main channel though ...

So what happens if a message from the client arrives?  Is that
broadcasted as-is to all display channels?  The current qemu code
(interface_client_monitors_config in spice-display.c, which is used with
virtio-gpu) simply uses the head as index into the array, so it looks
like spice-server doesn't do any filtering here (like only filling in
the monitors which belong to the display channel).

spice_qxl_monitors_config_async() is never called with virtio-gpu,
except when opengl is enabled.  In that case qemu simply sets
QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
(see qemu_spice_gl_monitor_config in spice-display.c).  Which would be
ok in case spice-server merges the configs from all channels before
sending it off to the client.  Not sure this actually happens ...

> Interstingly enough, that seems to be the ID we want to have in the
> device_display_id attribute.  I expect (still need to look that up, I'm
> out of time right now) that for virtio-gpu the ID is a bit different,

Keep in mind that multiple display devices don't really work right now,
and possibly we need to fix not only spice but qemu too.

> And yeah, I didn't use the id in the QEMU patches, as I didn't know
> how, I expect Gerd to have some grand plans for it :)

IIRC the latest plan was to just keep things as is, plan with one
channel per monitor for all future things, and just not support
one-qxl-device-multihead in combination with multiple display channels.

Is that correct?

I don't think we need the monitors_id in qemu then, qemu can simply use
the channel_id (except for the legacy qxl case).

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

Hi Gerd,

On Wed, 2018-10-10 at 12:37 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > + * Sets the hardware (e.g. PCI) path of the graphics device
> > > represented by this QXL interface instance.
> > 
> > So, this comment suggests that the caller might be able to provide a
> > path that is not a PCI path. But the implementation below (mostly the
> > debug output, I suppose...) assumes a PCI path. Do we need to support
> > non-PCI paths?
> 
> Certainly not for the initial revision, maybe never.
> 
> But thanks to the "pci/" prefix we should be able to add support for
> other paths later in case it turns out we need it.
> 
> > > + * Returns: The actual SPICE server monitor_id associated with the
> > > display
> > 
> > So, if I remember correctly, Gerd recommended returning this value from
> > the function. But I think it needs more explanation here. What exactly
> > is a "monitor_id" supposed to represent? It is not used in your follow-
> > up qemu patch so it's hard to tell.
> 
> IIRC the plan was to ditch the global monior_id idea and use the
> (channel_id, display_id) tuple everywhere ...

Not sure what exactly you mean here by "global monitor_id". Not sure
about "use the (channel_id, display_id)" either, it doesn't seem quite
correct.

The plan was (and still is) to limit the use cases to the following
two:

* Legacy QXL on linux with multiple monitors per display channel, but
only this single display channel. Multiple display channels are not
supported in this case, so no streaming etc.

* Limit the number of monitors per display channel to one for all other
cases.

With these limitations, the display_id = channel_id + monitor_id
formula that is used on the client remains unique. With one more
condition, that I think I should add, and that is that monitor_id is
always 0 for the multiple display channel case. It seems it may come up
that the monitor_id could be non-zero, e.g. for the virtio-gpu case...

So the IDs used are:

monitors_config server -> client:
(channel_id, monitor_id)

monitors_config client -> server and possibly server -> vd_agent:
display_id

I hope it's clear like this :)

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> 
> > > So, if I remember correctly, Gerd recommended returning this value from
> > > the function. But I think it needs more explanation here. What exactly
> > > is a "monitor_id" supposed to represent? It is not used in your follow-
> > > up qemu patch so it's hard to tell.
> > 
> > It's supposed to be the actual monitor_id that we use in SPICE to
> > identify the monitors. I've just spent quite some time looking up where
> > the monitor_id actually comes from, and it's actually set all the way
> > down in the driver (either xf86-video-qxl or the KMS driver in the
> > kernel) and passed through the monitors_config functions to SPICE
> > server.
> 
> How does all this monitors_config work currently in case multiple
> display channels are present?
> 
> There is the QXLInterface->client_monitors_config() callback.
> 
> There is the spice_qxl_monitors_config_async() function.
> 
> Both are linked to a display channel.  The server/client messages go
> through the main channel though ...
> 
> So what happens if a message from the client arrives?  Is that
> broadcasted as-is to all display channels?  The current qemu code
> (interface_client_monitors_config in spice-display.c, which is used with
> virtio-gpu) simply uses the head as index into the array, so it looks
> like spice-server doesn't do any filtering here (like only filling in
> the monitors which belong to the display channel).
> 

Yes, it is a broadcast but is a bug that is working only because the
message is handled only by Linux driver and in Linux configuration is
expected that there is only a QXL card supporting multiple monitor
(in this case the broadcast is equivalent to send only the monitor
for the given QXL card).

> spice_qxl_monitors_config_async() is never called with virtio-gpu,
> except when opengl is enabled.  In that case qemu simply sets
> QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
> (see qemu_spice_gl_monitor_config in spice-display.c).  Which would be
> ok in case spice-server merges the configs from all channels before
> sending it off to the client.  Not sure this actually happens ...
> 

Yes, server send all the monitors together (or at least it should).
So client sends and receives all the monitor sizes at once.

> > Interstingly enough, that seems to be the ID we want to have in the
> > device_display_id attribute.  I expect (still need to look that up, I'm
> > out of time right now) that for virtio-gpu the ID is a bit different,
> 
> Keep in mind that multiple display devices don't really work right now,
> and possibly we need to fix not only spice but qemu too.
> 

What does not work at the moment? On Windows is normal to have multiple
QXL cards and is working for instance. We use QXL card and vGPU and is
not working that bad (beside the problem we are fixing here).

> > And yeah, I didn't use the id in the QEMU patches, as I didn't know
> > how, I expect Gerd to have some grand plans for it :)
> 
> IIRC the latest plan was to just keep things as is, plan with one
> channel per monitor for all future things, and just not support
> one-qxl-device-multihead in combination with multiple display channels.
> 
> Is that correct?
> 

Well, I think depends form the definition of "things". As we are
changing the code we are not "just keep things as is". But yes, for
the future we plan to support only one monitor per channel so
monitor_id == 0 (and so will be display_id == channel_id).
I suppose by "things" in "just keep things as is" you mean the
SPICE (client <-> server) protocol.

> I don't think we need the monitors_id in qemu then, qemu can simply use
> the channel_id (except for the legacy qxl case).
> 

Yes, and the channel_id is provided by Qemu to spice-server (as id of
the QXL interface) so there's no reason to ask to spice-server to provide
back channel_id.

> cheers,
>   Gerd
> 

Frediano

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Thu, 2018-10-11 at 14:27 +0200, Gerd Hoffmann wrote:
> > > So, if I remember correctly, Gerd recommended returning this value from
> > > the function. But I think it needs more explanation here. What exactly
> > > is a "monitor_id" supposed to represent? It is not used in your follow-
> > > up qemu patch so it's hard to tell.
> > 
> > It's supposed to be the actual monitor_id that we use in SPICE to
> > identify the monitors. I've just spent quite some time looking up where
> > the monitor_id actually comes from, and it's actually set all the way
> > down in the driver (either xf86-video-qxl or the KMS driver in the
> > kernel) and passed through the monitors_config functions to SPICE
> > server.
> 
> How does all this monitors_config work currently in case multiple
> display channels are present?
> 
> There is the QXLInterface->client_monitors_config() callback.
> 
> There is the spice_qxl_monitors_config_async() function.
> 
> Both are linked to a display channel.  The server/client messages go
> through the main channel though ...

Not really, it actually is like this:

       display channel
server --------------> client


        main channel
client --------------> server

So the monitors_configs go each on its own display channel to the
client, the client puts it all together and sends back an aggregated
list on the main channel.

> So what happens if a message from the client arrives?  Is that
> broadcasted as-is to all display channels?  The current qemu code
> (interface_client_monitors_config in spice-display.c, which is used with
> virtio-gpu) simply uses the head as index into the array, so it looks
> like spice-server doesn't do any filtering here (like only filling in
> the monitors which belong to the display channel).

Correct, the spice server doesn't do any filtering and sends the whole
monitors_config to all the interfaces...

> spice_qxl_monitors_config_async() is never called with virtio-gpu,
> except when opengl is enabled.  In that case qemu simply sets
> QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
> (see qemu_spice_gl_monitor_config in spice-display.c).

As a side note, I have no idea about opengl. I got a slight impression
from some earlier conversation that this is actually not used, but I
don't really know.

My wild guess at this moment is, that if you don't call
spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't
send any monitors config, a simple one containing a single monitor for
the surface is created somewhere along the way.

> Which would be
> ok in case spice-server merges the configs from all channels before
> sending it off to the client.  Not sure this actually happens ...

A bit differently, as I said, but the configs are merged on the client,
which should be an equivalent outcome.

> > Interstingly enough, that seems to be the ID we want to have in the
> > device_display_id attribute.  I expect (still need to look that up, I'm
> > out of time right now) that for virtio-gpu the ID is a bit different,
> 
> Keep in mind that multiple display devices don't really work right now,
> and possibly we need to fix not only spice but qemu too.

What exactly do you mean by multiple devices not working?

> > And yeah, I didn't use the id in the QEMU patches, as I didn't know
> > how, I expect Gerd to have some grand plans for it :)
> 
> IIRC the latest plan was to just keep things as is, plan with one
> channel per monitor for all future things, and just not support
> one-qxl-device-multihead in combination with multiple display channels.
> 
> Is that correct?

Correct.

> I don't think we need the monitors_id in qemu then, qemu can simply use
> the channel_id (except for the legacy qxl case).

Ok. Given the fact that the monitor_ids actually come from the driver,
QEMU should actually know them already, right? No need to pass them
back anyway? (except for the virtio-gpu case, which doesn't send
monitors_config and so a single monitor_id = 0 is deduced in spice
server)

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> The plan was (and still is) to limit the use cases to the following
> two:
> 
> * Legacy QXL on linux with multiple monitors per display channel, but
> only this single display channel. Multiple display channels are not
> supported in this case, so no streaming etc.
> 
> * Limit the number of monitors per display channel to one for all other
> cases.

Right, I mis-remembered.

> With these limitations, the display_id = channel_id + monitor_id
> formula that is used on the client remains unique.

And in case no qxl card is in use we even have display_id == channel_id.

So, do we need this add display_id function at all?

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Thu, 2018-10-11 at 15:20 +0200, Gerd Hoffmann wrote:
> > The plan was (and still is) to limit the use cases to the following
> > two:
> > 
> > * Legacy QXL on linux with multiple monitors per display channel, but
> > only this single display channel. Multiple display channels are not
> > supported in this case, so no streaming etc.
> > 
> > * Limit the number of monitors per display channel to one for all other
> > cases.
> 
> Right, I mis-remembered.
> 
> > With these limitations, the display_id = channel_id + monitor_id
> > formula that is used on the client remains unique.
> 
> And in case no qxl card is in use we even have display_id == channel_id.
> 
> So, do we need this add display_id function at all?

You mean the addition of IDs in display_id = channel_id + monitor_id?

That's what is in the clients now, at least in remote_viewer and spicy,
although in remote_viewer it's display_id = channel_id ? channel_id :
monitor_id, which is equivalent if either of the numbers (or both) is
always zero.

We also still need to handle the "legacy" (it's still the majority in
usage on linux though...) QXL case, and this formula works for both
cases, so I'm not sure it's worth trying to replace it with something.
Although we _should_ try to clean this mess up eventually...

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

  Hi,

> > How does all this monitors_config work currently in case multiple
> > display channels are present?
> > 
> > There is the QXLInterface->client_monitors_config() callback.
> > 
> > There is the spice_qxl_monitors_config_async() function.
> > 
> > Both are linked to a display channel.  The server/client messages go
> > through the main channel though ...
> 
> Not really, it actually is like this:
> 
>        display channel
> server --------------> client
> 
> 
>         main channel
> client --------------> server
> 
> So the monitors_configs go each on its own display channel to the
> client, the client puts it all together and sends back an aggregated
> list on the main channel.

Ok.

> > So what happens if a message from the client arrives?  Is that
> > broadcasted as-is to all display channels?  The current qemu code
> > (interface_client_monitors_config in spice-display.c, which is used with
> > virtio-gpu) simply uses the head as index into the array, so it looks
> > like spice-server doesn't do any filtering here (like only filling in
> > the monitors which belong to the display channel).
> 
> Correct, the spice server doesn't do any filtering and sends the whole
> monitors_config to all the interfaces...

Ok.  We probably should fix interface_client_monitors_config() to use
the channel_id instead of qemu_console_get_head() then.

> > spice_qxl_monitors_config_async() is never called with virtio-gpu,
> > except when opengl is enabled.  In that case qemu simply sets
> > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
> > (see qemu_spice_gl_monitor_config in spice-display.c).
> 
> As a side note, I have no idea about opengl. I got a slight impression
> from some earlier conversation that this is actually not used, but I
> don't really know.

Not by default, but when virgl is enabled (-spice gl=on -vga virtio).

> My wild guess at this moment is, that if you don't call
> spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't
> send any monitors config, a simple one containing a single monitor for
> the surface is created somewhere along the way.

Probably.

> > Which would be
> > ok in case spice-server merges the configs from all channels before
> > sending it off to the client.  Not sure this actually happens ...
> 
> A bit differently, as I said, but the configs are merged on the client,
> which should be an equivalent outcome.

Indeed.  So the qemu_spice_gl_monitor_config() function is correct then.

> > > Interstingly enough, that seems to be the ID we want to have in the
> > > device_display_id attribute.  I expect (still need to look that up, I'm
> > > out of time right now) that for virtio-gpu the ID is a bit different,
> > 
> > Keep in mind that multiple display devices don't really work right now,
> > and possibly we need to fix not only spice but qemu too.
> 
> What exactly do you mean by multiple devices not working?

qxl + vgpu (which is why we are discussing this).
qxl + virtio-gpu will have simliar issues I think.

> > I don't think we need the monitors_id in qemu then, qemu can simply use
> > the channel_id (except for the legacy qxl case).
> 
> Ok. Given the fact that the monitor_ids actually come from the driver,
> QEMU should actually know them already, right?

Yes.

> No need to pass them back anyway? (except for the virtio-gpu case,
> which doesn't send monitors_config and so a single monitor_id = 0 is
> deduced in spice server)

When virtio-gpu sends monitors_config monitor_id will be zero too,
because we have one channel per monitor.


Another story is linking display channels to device heads, i.e.
virtio-gpu registers one display channel per head, each channel
registers the same device path of course, and now you need to
figure in the guest agent which xrandr output is which head.

Simplest way would be to require display channels being registered in
order, so the channel with the smallest channel_id is head 0, ...

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> > So, do we need this add display_id function at all?
> 
> You mean the addition of IDs in display_id = channel_id + monitor_id?

No, I mean spice_qxl_set_path() is enough, the other function is not
needed.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Thu, 2018-10-11 at 15:43 +0200, Gerd Hoffmann wrote:
>   Hi,
> 
> > > How does all this monitors_config work currently in case multiple
> > > display channels are present?
> > > 
> > > There is the QXLInterface->client_monitors_config() callback.
> > > 
> > > There is the spice_qxl_monitors_config_async() function.
> > > 
> > > Both are linked to a display channel.  The server/client messages go
> > > through the main channel though ...
> > 
> > Not really, it actually is like this:
> > 
> >        display channel
> > server --------------> client
> > 
> > 
> >         main channel
> > client --------------> server
> > 
> > So the monitors_configs go each on its own display channel to the
> > client, the client puts it all together and sends back an aggregated
> > list on the main channel.
> 
> Ok.
> 
> > > So what happens if a message from the client arrives?  Is that
> > > broadcasted as-is to all display channels?  The current qemu code
> > > (interface_client_monitors_config in spice-display.c, which is used with
> > > virtio-gpu) simply uses the head as index into the array, so it looks
> > > like spice-server doesn't do any filtering here (like only filling in
> > > the monitors which belong to the display channel).
> > 
> > Correct, the spice server doesn't do any filtering and sends the whole
> > monitors_config to all the interfaces...
> 
> Ok.  We probably should fix interface_client_monitors_config() to use
> the channel_id instead of qemu_console_get_head() then.

It's not that simple. This would break the QXL with multiple monitors
per channel case.

I think we should fix spice server to actually do the filtering and
only send monitors_config that belongs to the device to the QXL
interface. As Frediano mentioned, it looks more like a bug.

> > > spice_qxl_monitors_config_async() is never called with virtio-gpu,
> > > except when opengl is enabled.  In that case qemu simply sets
> > > QXLMonitorsConfig->count to 1 and fills in QXLMonitorsConfig->head[0]
> > > (see qemu_spice_gl_monitor_config in spice-display.c).
> > 
> > As a side note, I have no idea about opengl. I got a slight impression
> > from some earlier conversation that this is actually not used, but I
> > don't really know.
> 
> Not by default, but when virgl is enabled (-spice gl=on -vga virtio).
> 
> > My wild guess at this moment is, that if you don't call
> > spice_qxl_monitors_config_async() with virtio-gpu, meaning you don't
> > send any monitors config, a simple one containing a single monitor for
> > the surface is created somewhere along the way.
> 
> Probably.
> 
> > > Which would be
> > > ok in case spice-server merges the configs from all channels before
> > > sending it off to the client.  Not sure this actually happens ...
> > 
> > A bit differently, as I said, but the configs are merged on the client,
> > which should be an equivalent outcome.
> 
> Indeed.  So the qemu_spice_gl_monitor_config() function is correct then.

I don't follow you reasoning for this conclusion... Is it correct
because it sends a single monitor in the monitors_config? Why is this
function needed for the opengl case and not for regular virtio-gpu
case?

> > > > Interstingly enough, that seems to be the ID we want to have in the
> > > > device_display_id attribute.  I expect (still need to look that up, I'm
> > > > out of time right now) that for virtio-gpu the ID is a bit different,
> > > 
> > > Keep in mind that multiple display devices don't really work right now,
> > > and possibly we need to fix not only spice but qemu too.
> > 
> > What exactly do you mean by multiple devices not working?
> 
> qxl + vgpu (which is why we are discussing this).
> qxl + virtio-gpu will have simliar issues I think.

Ok, I see.

> > > I don't think we need the monitors_id in qemu then, qemu can simply use
> > > the channel_id (except for the legacy qxl case).
> > 
> > Ok. Given the fact that the monitor_ids actually come from the driver,
> > QEMU should actually know them already, right?
> 
> Yes.
> 
> > No need to pass them back anyway? (except for the virtio-gpu case,
> > which doesn't send monitors_config and so a single monitor_id = 0 is
> > deduced in spice server)
> 
> When virtio-gpu sends monitors_config monitor_id will be zero too,
> because we have one channel per monitor.

Right.

> Another story is linking display channels to device heads, i.e.
> virtio-gpu registers one display channel per head, each channel
> registers the same device path of course, and now you need to
> figure in the guest agent which xrandr output is which head.

This is actually the reason for the
spice_qxl_device_monitor_add_display_id(qxl, device_display_id)
function (using this opportunity to merge the other email thread
discussion into one).

So for example if you have 3 heads on a virtio-gpu device, and you call
it on qxl instance id (i.e. channel_id) = 2, the device_display_id
argument will be 2 (as it is the head ID) and in the current state
(which I agree with Jonathon is unintuitive and we should probably
change it) spice will associate the first call of this function with
monitor_id 0, so it will know that monitor_id 0 on this qxl instance
has the device_display_id 2.

> Simplest way would be to require display channels being registered in
> order, so the channel with the smallest channel_id is head 0, ...

I don't like this, you once again rely on implicit information derived
from the order of registration of the channels. We should make this
explicit, so that it doesn't cause trouble in the future...

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

> > Ok.  We probably should fix interface_client_monitors_config() to use
> > the channel_id instead of qemu_console_get_head() then.
> 
> It's not that simple. This would break the QXL with multiple monitors
> per channel case.

It is that simple.

qxl doesn't use that code path and has its own version of the callback
(in qxl.c).  Fixing it there is a bit more tricky.

> I think we should fix spice server to actually do the filtering and
> only send monitors_config that belongs to the device to the QXL
> interface. As Frediano mentioned, it looks more like a bug.

Only problem is changing the callback semantics changes the library api.
We could add a second version of the callback which gets called with a
filtered list (if present).  Not sure this is worth the effort though.

> > > A bit differently, as I said, but the configs are merged on the client,
> > > which should be an equivalent outcome.
> > 
> > Indeed.  So the qemu_spice_gl_monitor_config() function is correct then.
> 
> I don't follow you reasoning for this conclusion... Is it correct
> because it sends a single monitor in the monitors_config?

Yes (again, qxl doesn't run this code path so it'll only see the
one-monitor-per-channel cases).

> Why is this
> function needed for the opengl case and not for regular virtio-gpu
> case?

Not fully sure why opengl needs this.  Maybe because the texture can be
larger than the actual scanout (for padding reasons) so we need to
communicate the scanout rectangle.

> > Another story is linking display channels to device heads, i.e.
> > virtio-gpu registers one display channel per head, each channel
> > registers the same device path of course, and now you need to
> > figure in the guest agent which xrandr output is which head.
> 
> This is actually the reason for the
> spice_qxl_device_monitor_add_display_id(qxl, device_display_id)
> function (using this opportunity to merge the other email thread
> discussion into one).

Ah, ok.  We should *not* call this thing display_id then, that'll be
confusing.  Or at very least rename the function to something like ...

spice_qxl_monitor_add_device_display_id()
                      ^^^^^^^^^^^^^^^^^   don't split this

... to make more clear this is something else than the display_id.

> > Simplest way would be to require display channels being registered in
> > order, so the channel with the smallest channel_id is head 0, ...
> 
> I don't like this, you once again rely on implicit information derived
> from the order of registration of the channels. We should make this
> explicit, so that it doesn't cause trouble in the future...

Fine with me too.

I'd suggest to split the patches, one for the path and one for the
device_display_id thing (or whatever else we call this to make it less
likely being confused with display_id, even though I don't have a good
suggestion offhand).

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote:
> > > Ok.  We probably should fix interface_client_monitors_config() to use
> > > the channel_id instead of qemu_console_get_head() then.
> > 
> > It's not that simple. This would break the QXL with multiple monitors
> > per channel case.
> 
> It is that simple.
> 
> qxl doesn't use that code path and has its own version of the callback
> (in qxl.c).  Fixing it there is a bit more tricky.

Ok.. and what's actually the problem using qemu_console_get_head()? It
just doesn't feel right to me using channel_id as an index into this
array. It is not the right index strictly speaking.

> > I think we should fix spice server to actually do the filtering and
> > only send monitors_config that belongs to the device to the QXL
> > interface. As Frediano mentioned, it looks more like a bug.
> 
> Only problem is changing the callback semantics changes the library api.
> We could add a second version of the callback which gets called with a
> filtered list (if present).  Not sure this is worth the effort though.

That's right. But if we don't actually break any currently supported
use cases, it might be fine? The only thing we would be breaking is the
virtio-gpu, I think? Is that already something we don't want to break?

> > > > A bit differently, as I said, but the configs are merged on the client,
> > > > which should be an equivalent outcome.
> > > 
> > > Indeed.  So the qemu_spice_gl_monitor_config() function is correct then.
> > 
> > I don't follow you reasoning for this conclusion... Is it correct
> > because it sends a single monitor in the monitors_config?
> 
> Yes (again, qxl doesn't run this code path so it'll only see the
> one-monitor-per-channel cases).
> 
> > Why is this
> > function needed for the opengl case and not for regular virtio-gpu
> > case?
> 
> Not fully sure why opengl needs this.  Maybe because the texture can be
> larger than the actual scanout (for padding reasons) so we need to
> communicate the scanout rectangle.
> 
> > > Another story is linking display channels to device heads, i.e.
> > > virtio-gpu registers one display channel per head, each channel
> > > registers the same device path of course, and now you need to
> > > figure in the guest agent which xrandr output is which head.
> > 
> > This is actually the reason for the
> > spice_qxl_device_monitor_add_display_id(qxl, device_display_id)
> > function (using this opportunity to merge the other email thread
> > discussion into one).
> 
> Ah, ok.  We should *not* call this thing display_id then, that'll be
> confusing.  Or at very least rename the function to something like ...
> 
> spice_qxl_monitor_add_device_display_id()
>                       ^^^^^^^^^^^^^^^^^   don't split this
> 
> ... to make more clear this is something else than the display_id.

Ok.

> > > Simplest way would be to require display channels being registered in
> > > order, so the channel with the smallest channel_id is head 0, ...
> > 
> > I don't like this, you once again rely on implicit information derived
> > from the order of registration of the channels. We should make this
> > explicit, so that it doesn't cause trouble in the future...
> 
> Fine with me too.
> 
> I'd suggest to split the patches, one for the path and one for the
> device_display_id thing (or whatever else we call this to make it less
> likely being confused with display_id, even though I don't have a good
> suggestion offhand).

Sure, no problem.

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote:
> On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote:
> > > > Ok.  We probably should fix interface_client_monitors_config() to use
> > > > the channel_id instead of qemu_console_get_head() then.
> > > 
> > > It's not that simple. This would break the QXL with multiple monitors
> > > per channel case.
> > 
> > It is that simple.
> > 
> > qxl doesn't use that code path and has its own version of the callback
> > (in qxl.c).  Fixing it there is a bit more tricky.
> 
> Ok.. and what's actually the problem using qemu_console_get_head()? It
> just doesn't feel right to me using channel_id as an index into this
> array. It is not the right index strictly speaking.

Assume you have one qxl and one virtio-gpu device (one head each), for
example:

   00:02.0   qxl          channel 0
   00:03.0   virtio-gpu   channel 1

So the client will assign display 0 to qxl and display 1 to virtio-gpu.
In interface_client_monitors_config() we have to pick the correct array
entry.

When using the channel_id it works correctly.

When using qemu_console_get_head() it doesn't work correctly, it would
use the qxl card's data.  It would work if spice-server would filter the
list to only include the entries for the given display channel before
calling the ->client_monitors_config() callback.  But it doesn't, we get
the complete set.

> > > I think we should fix spice server to actually do the filtering and
> > > only send monitors_config that belongs to the device to the QXL
> > > interface. As Frediano mentioned, it looks more like a bug.
> > 
> > Only problem is changing the callback semantics changes the library api.
> > We could add a second version of the callback which gets called with a
> > filtered list (if present).  Not sure this is worth the effort though.
> 
> That's right. But if we don't actually break any currently supported
> use cases, it might be fine? The only thing we would be breaking is
> the virtio-gpu, I think?  Is that already something we don't want to
> break?

It would break any multihead configuration which uses multiple display
channels.  Yes, virtio-gpu is one case.  Breaking that would not be very
nice, but maybe acceptable.  The other case is multiple qxl devices
(i.e.  windows guest style multihead).  Breaking that is out of
question.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Lukáš Hrázký]

On Fri, 2018-10-12 at 11:27 +0200, Gerd Hoffmann wrote:
> On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote:
> > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote:
> > > > > Ok.  We probably should fix interface_client_monitors_config() to use
> > > > > the channel_id instead of qemu_console_get_head() then.
> > > > 
> > > > It's not that simple. This would break the QXL with multiple monitors
> > > > per channel case.
> > > 
> > > It is that simple.
> > > 
> > > qxl doesn't use that code path and has its own version of the callback
> > > (in qxl.c).  Fixing it there is a bit more tricky.
> > 
> > Ok.. and what's actually the problem using qemu_console_get_head()? It
> > just doesn't feel right to me using channel_id as an index into this
> > array. It is not the right index strictly speaking.
> 
> Assume you have one qxl and one virtio-gpu device (one head each), for
> example:
> 
>    00:02.0   qxl          channel 0
>    00:03.0   virtio-gpu   channel 1
> 
> So the client will assign display 0 to qxl and display 1 to virtio-gpu.
> In interface_client_monitors_config() we have to pick the correct array
> entry.
> 
> When using the channel_id it works correctly.
> 
> When using qemu_console_get_head() it doesn't work correctly, it would
> use the qxl card's data.  It would work if spice-server would filter the
> list to only include the entries for the given display channel before
> calling the ->client_monitors_config() callback.  But it doesn't, we get
> the complete set.
> 
> > > > I think we should fix spice server to actually do the filtering and
> > > > only send monitors_config that belongs to the device to the QXL
> > > > interface. As Frediano mentioned, it looks more like a bug.
> > > 
> > > Only problem is changing the callback semantics changes the library api.
> > > We could add a second version of the callback which gets called with a
> > > filtered list (if present).  Not sure this is worth the effort though.
> > 
> > That's right. But if we don't actually break any currently supported
> > use cases, it might be fine? The only thing we would be breaking is
> > the virtio-gpu, I think?  Is that already something we don't want to
> > break?
> 
> It would break any multihead configuration which uses multiple display
> channels.  Yes, virtio-gpu is one case.  Breaking that would not be very
> nice, but maybe acceptable.  The other case is multiple qxl devices
> (i.e.  windows guest style multihead).  Breaking that is out of
> question.

The windows QXL driver doesn't support the monitors_config callback, at
least that's what Frediano said here:

https://lists.freedesktop.org/archives/spice-devel/2018-October/045965.html

So I don't think this is a problem and if breaking virtio-gpu would be
acceptable, we should probably consider fixing the interface...

Cheers,
Lukas

> cheers,
>   Gerd
> 

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> On Thu, Oct 11, 2018 at 05:37:46PM +0200, Lukáš Hrázký wrote:
> > On Thu, 2018-10-11 at 17:09 +0200, Gerd Hoffmann wrote:
> > > > > Ok.  We probably should fix interface_client_monitors_config() to use
> > > > > the channel_id instead of qemu_console_get_head() then.
> > > > 
> > > > It's not that simple. This would break the QXL with multiple monitors
> > > > per channel case.
> > > 
> > > It is that simple.
> > > 
> > > qxl doesn't use that code path and has its own version of the callback
> > > (in qxl.c).  Fixing it there is a bit more tricky.
> > 
> > Ok.. and what's actually the problem using qemu_console_get_head()? It
> > just doesn't feel right to me using channel_id as an index into this
> > array. It is not the right index strictly speaking.
> 
> Assume you have one qxl and one virtio-gpu device (one head each), for
> example:
> 
>    00:02.0   qxl          channel 0
>    00:03.0   virtio-gpu   channel 1
> 
> So the client will assign display 0 to qxl and display 1 to virtio-gpu.
> In interface_client_monitors_config() we have to pick the correct array
> entry.
> 
> When using the channel_id it works correctly.
> 
> When using qemu_console_get_head() it doesn't work correctly, it would
> use the qxl card's data.  It would work if spice-server would filter the
> list to only include the entries for the given display channel before
> calling the ->client_monitors_config() callback.  But it doesn't, we get
> the complete set.
> 

That's why I said is a bug, IMHO it should.

> > > > I think we should fix spice server to actually do the filtering and
> > > > only send monitors_config that belongs to the device to the QXL
> > > > interface. As Frediano mentioned, it looks more like a bug.
> > > 
> > > Only problem is changing the callback semantics changes the library api.
> > > We could add a second version of the callback which gets called with a
> > > filtered list (if present).  Not sure this is worth the effort though.
> > 
> > That's right. But if we don't actually break any currently supported
> > use cases, it might be fine? The only thing we would be breaking is
> > the virtio-gpu, I think?  Is that already something we don't want to
> > break?
> 
> It would break any multihead configuration which uses multiple display
> channels.  Yes, virtio-gpu is one case.  Breaking that would not be very
> nice, but maybe acceptable.  The other case is multiple qxl devices
> (i.e.  windows guest style multihead).  Breaking that is out of
> question.
> 

Windows is not a problem as it ignores the data passed to client_monitors_config.

> cheers,
>   Gerd
> 

Frediano

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

  Hi,

> > When using qemu_console_get_head() it doesn't work correctly, it would
> > use the qxl card's data.  It would work if spice-server would filter the
> > list to only include the entries for the given display channel before
> > calling the ->client_monitors_config() callback.  But it doesn't, we get
> > the complete set.
> 
> That's why I said is a bug, IMHO it should.

Hmm, this should be easily detectable on qemu side I think.  If
num_of_monitors (for a non-qxl card) is > 1, then we have an old,
non-filtering spice-server.  If num_of_monitors == 1, then it is a new,
filtering spice-server.  Or a single-head channel configuration, in
which case it doesn't matter whenever it filters or not.

So this should be fixable without causing too much compatibility pain.

cheers,
  Gerd

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Frediano Ziglio]

> 
> Hi,
> 
> > > When using qemu_console_get_head() it doesn't work correctly, it would
> > > use the qxl card's data.  It would work if spice-server would filter the
> > > list to only include the entries for the given display channel before
> > > calling the ->client_monitors_config() callback.  But it doesn't, we get
> > > the complete set.
> > 
> > That's why I said is a bug, IMHO it should.
> 
> Hmm, this should be easily detectable on qemu side I think.  If
> num_of_monitors (for a non-qxl card) is > 1, then we have an old,
> non-filtering spice-server.  If num_of_monitors == 1, then it is a new,
> filtering spice-server.  Or a single-head channel configuration, in
> which case it doesn't matter whenever it filters or not.
> 
> So this should be fixable without causing too much compatibility pain.
> 
> cheers,
>   Gerd
> 

Sorry, why fixing in Qemu is the bug is in spice-server?
I really don't follow your reasoning.
For me Qemu should not change at all and spice-server should
do the filtering. Is this not fixing everything?

Frediano

Re: [Qemu-devel] [Spice-devel] [RFC PATCH spice 1/2] QXL interface: add functions to identify monitors in the guest

[Gerd Hoffmann]

On Fri, Oct 12, 2018 at 06:46:37AM -0400, Frediano Ziglio wrote:
> > 
> > Hi,
> > 
> > > > When using qemu_console_get_head() it doesn't work correctly, it would
> > > > use the qxl card's data.  It would work if spice-server would filter the
> > > > list to only include the entries for the given display channel before
> > > > calling the ->client_monitors_config() callback.  But it doesn't, we get
> > > > the complete set.
> > > 
> > > That's why I said is a bug, IMHO it should.
> > 
> > Hmm, this should be easily detectable on qemu side I think.  If
> > num_of_monitors (for a non-qxl card) is > 1, then we have an old,
> > non-filtering spice-server.  If num_of_monitors == 1, then it is a new,
> > filtering spice-server.  Or a single-head channel configuration, in
> > which case it doesn't matter whenever it filters or not.
> > 
> > So this should be fixable without causing too much compatibility pain.
> > 
> > cheers,
> >   Gerd
> > 
> 
> Sorry, why fixing in Qemu is the bug is in spice-server?
> I really don't follow your reasoning.

I had a thinko in one of the previous messages.
qemu_console_get_head() is never correct, so qemu is buggy too.

It should be either qemu_console_get_index() (equals channel id), for
the non-filtering case, or just 0, for the filtering case, because every
virtio-gpu head has its own display channel.  Patch below.

> For me Qemu should not change at all and spice-server should
> do the filtering. Is this not fixing everything?

Well, fixed qemu being able to deal with all spice-server versions
(fixed and unifixed) is certainly useful to reduce the damage caused
by this incompatible change.

The qxl version of that callback can stay as-is, the spice-server side
fix should make sure the current code continues to work when we start
supporting new configurations (qxl and non-qxl devices mixed).

cheers,
  Gerd

diff --git a/ui/spice-display.c b/ui/spice-display.c
index 2f8adb6b9f..52f8cb5ae1 100644
--- a/ui/spice-display.c
+++ b/ui/spice-display.c
@@ -674,10 +674,28 @@ static int
interface_client_monitors_config(QXLInstance *sin,
 
     memset(&info, 0, sizeof(info));
 
-    head = qemu_console_get_head(ssd->dcl.con);
-    if (mc->num_of_monitors > head) {
-        info.width  = mc->monitors[head].width;
-        info.height = mc->monitors[head].height;
+    if (mc->num_of_monitors == 1) {
+        /*
+         * New spice-server version which filters the list of monitors
+         * to only include those that belong to our display channel.
+         *
+         * single-head configuration (where filtering doesn't matter)
+         * takes this code path too.
+         */
+        info.width  = mc->monitors[0].width;
+        info.height = mc->monitors[0].height;
+    } else {
+        /*
+         * Old spice-server which gives us all monitors, so we have to
+         * figure ourself which entry we need.  Array index is the
+         * channel_id, which is the qemu console index, see
+         * qemu_spice_add_display_interface().
+         */
+        head = qemu_console_get_index(ssd->dcl.con);
+        if (mc->num_of_monitors > head) {
+            info.width  = mc->monitors[head].width;
+            info.height = mc->monitors[head].height;
+        }
     }
 
     trace_qemu_spice_ui_info(ssd->qxl.id, info.width, info.height);