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

[Frediano Ziglio]

> 
> Adds two functions to let QEMU provide information to identify graphics
> devices and their monitors in the guest:
> 
> * device address - The path identifying the device on the system (e.g. PCI
>   path):
>   spice_qxl_set_device_address(...)
> 
> * device display ID - The index of the monitor on the graphics device:
>   spice_qxl_monitor_set_device_display_id(...)

This seems to indicate that this device is bound in some way to the previous
information but having 2 APIs make more fragile, potentially one could
call a function and not the other or in different order or mismatch them.

> 
> Signed-off-by: Lukáš Hrázký <address@hidden>
> ---
>  server/red-qxl.c         | 89 ++++++++++++++++++++++++++++++++++++++++
>  server/spice-qxl.h       |  5 +++
>  server/spice-server.syms |  6 +++
>  3 files changed, 100 insertions(+)
> 
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0b2043e1 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
>  #include "red-qxl.h"
>  
>  
> +#define MAX_PATH_LEN 256

Different OSes uses MAX_PATH/MAXPATH to specify filename limit so this sounds
confusing, maybe MAX_DEVICE_PATH_LEN would be better.

> +#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_address[MAX_PATH_LEN];
> +    uint32_t device_display_ids[MAX_MONITORS_COUNT];
> +    size_t monitors_count;  // length of ^^^

size_t look a bit too much for a number of item in a small array.

>  
>      pthread_mutex_t scanout_mutex;
>      SpiceMsgDisplayGlScanoutUnix scanout;
> @@ -846,6 +852,89 @@ void red_qxl_gl_draw_async_complete(QXLInstance *qxl)
>      red_qxl_async_complete(qxl, cookie);
>  }
>  
> +/**
> + * spice_qxl_set_device_address:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs to
> + *
> + * Sets the hardware address (e.g. PCI) of the graphics device represented
> by
> + * this QXL interface instance.
> + *
> + * The supported format is:
> + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> + *
> + * The "pci" identifies the rest of the string as a PCI adress. It is the
> only

typo: adress

> + * supported address at the moment, other identifiers can be introduced
> later.
> + * <DOMAIN> is the PCI domain, followed by <SLOT>.<FUNCTION> of any PCI
> bridges
> + * in the chain leading to the device. The last <SLOT>.<FUNCTION> is the
> + * graphics device.

Maybe better to specify also the encoding, like decimal/hexadecimal and number
of digits.

> + */

I would prefer documentation in the header so people don't have to open
the source to get it, also considering the the header is public.

> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_address(QXLInstance *instance, const char
> *device_address)
> +{
> +    g_return_if_fail(device_address != NULL);
> +
> +    size_t dp_len = strnlen(device_address, 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_address, device_address, MAX_PATH_LEN);
> +
> +    g_debug("QXL Instance %d setting device address \"%s\"", instance->id,
> device_address);
> +}
> +
> +/**
> + * spice_qxl_monitor_set_device_display_id:
> + * @instance the QXL instance to set the device display ID to
> + * @monitor_id the SPICE monitor ID to set the device display ID to
> + * @device_display_id the actual ID of the display (output) on the graphics
> device
> + *
> + * Sets the device display ID for a given monitor ID in a QXL instance. The
> + * monitor IDs are expected and required to be a consecutive sequence
> starting
> + * at 0. The function requires the calls to be made in the sequence to
> prevent
> + * holes.
> + *
> + * The requirement for the monitor ID to be a sequence starting from 0 comes
> + * from the mechanism of generating a single display_id from channel_id and
> + * monitor_id on the client:
> + *
> + * display_id = channel_id + monitor_id
> + *

No, the monitor_id sequence is defined as a sequence from 0, has nothing
to do with display_id. Also this comment seems to indicate that this new
interface is bad designed having the same limit, which is not.

> + * This is unambiguous either if there is only a single channel with
> multiple
> + * monitors ("legacy" QXL on linux case) or multiple channels with only a
> + * single monitor. Also both channel_id and monitor_id need to be a sequence
> + * starting from 0, otherwise there is still a possibility of collisions.

They are both defined (channel_id and monitor_id) as sequences starting from 0.
I don't see why need to be specified here.

> + */
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> +                                             uint32_t monitor_id,
> +                                             uint32_t device_display_id)

I still don't understand why, as suggested by Gerd, we need another function
instead of 2 additional parameters to the above API specifying start and
number, this API looks much more prone to errors.

Also there's no much documentation for this "device display ID" in the code,
potentially can be generated with something like:

uint32_t next_device_display_id(void)
{
   static unsigned id = 0;
   return (id++ + 0xf00) * 0xbeef;
}

which probably won't work for you.

> +{
> +    if (instance->st->monitors_count >= MAX_MONITORS_COUNT) {
> +        spice_error("Cannot register more than %u monitors per QXL
> interface", MAX_MONITORS_COUNT);
> +        return;
> +    }
> +
> +    if (monitor_id > instance->st->monitors_count) {
> +        spice_error("Monitor ID %u is not inside a consecutive sequence of
> monitor IDs "
> +                    "starting from zero. Needs to be lower than or equal to
> %lu.",
> +                    monitor_id, instance->st->monitors_count);
> +        return;
> +    }
> +
> +    instance->st->device_display_ids[monitor_id] = device_display_id;
> +
> +    g_debug("QXL Instance %d setting device display ID %u for monitor ID
> %u",
> +        instance->id, device_display_id, monitor_id);
> +
> +    // if we're adding a new ID (and not resetting an existing one),
> increment the array length
> +    if (monitor_id == instance->st->monitors_count) {
> +        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..c9ea6564 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -114,6 +114,11 @@ 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_set_device_address(QXLInstance *instance, const char
> *device_path);
> +void spice_qxl_monitor_set_device_display_id(QXLInstance *instance,
> +                                             uint32_t monitor_id,
> +                                             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..6e9ffa93 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_set_device_address;
> +    spice_qxl_monitor_set_device_display_id;
> +} SPICE_SERVER_0.13.2;

Frediano