On Tue, 2019-01-08 at 16:26 +0100, Lukáš Hrázký wrote:
> Adds a function to let QEMU provide information to identify graphics
> devices and their monitors in the guest. The function
> (spice_qxl_set_device_info) sets the device address (e.g. a PCI path)
> and monitor ID -> device display ID mapping of displays exposed by
> given
> QXL interface.
In my previous review, I asked for a slightly more explicit explanation
of the phrase "monitor ID" in the commit log. In this case, "monitor
ID" is an ID specific to the QXL instance starting at 0 and represents
the displays that are associated with that particular QXL instance.
Perhaps it makes sense to rename this "Qxl monitor ID"?
>
> Signed-off-by: Lukáš Hrázký <lhra...@redhat.com>
> ---
> server/red-qxl.c | 44 ++++++++++++++++++++++++++++++++++++++
> server/spice-qxl.h | 46
> ++++++++++++++++++++++++++++++++++++++++
> server/spice-server.syms | 5 +++++
> 3 files changed, 95 insertions(+)
>
> diff --git a/server/red-qxl.c b/server/red-qxl.c
> index 97940611..0ea424cd 100644
> --- a/server/red-qxl.c
> +++ b/server/red-qxl.c
> @@ -41,6 +41,9 @@
> #include "red-qxl.h"
>
>
> +#define MAX_DEVICE_ADDRESS_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_address[MAX_DEVICE_ADDRESS_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,44 @@ void red_qxl_gl_draw_async_complete(QXLInstance
> *qxl)
> red_qxl_async_complete(qxl, cookie);
> }
>
> +SPICE_GNUC_VISIBLE
> +void spice_qxl_set_device_info(QXLInstance *instance,
> + const char *device_address,
> + uint32_t device_display_id_start,
> + uint32_t device_display_id_count)
> +{
> + g_return_if_fail(device_address != NULL);
Just a thought: what if qemu calls this function twice for the same
instance. In theory this shouldn't happen, but should we be defensive
and handle it somehow? Print a warning? Just overwrite the previous
mapping? Right now it looks like we would just overwrite.
> +
> + size_t da_len = strnlen(device_address, MAX_DEVICE_ADDRESS_LEN);
> + if (da_len >= MAX_DEVICE_ADDRESS_LEN) {
> + spice_error("Device address too long: %lu > %u", da_len,
> MAX_DEVICE_ADDRESS_LEN);
Technically, I think the format string for a size_t variable is %zu,
not %lu
> + return;
> + }
> +
> + if (device_display_id_count > MAX_MONITORS_COUNT) {
> + spice_error("Device display ID count (%u) is greater than
> limit %u",
> + device_display_id_count,
> + MAX_MONITORS_COUNT);
> + return;
> + }
> +
> + strncpy(instance->st->device_address, device_address,
> MAX_DEVICE_ADDRESS_LEN);
> +
> + g_debug("QXL Instance %d setting device address \"%s\" and
> monitor -> device display mapping:",
> + instance->id,
> + device_address);
> +
> + // store the mapping monitor_id -> device_display_id
> + for (uint32_t monitor_id = 0; monitor_id <
> device_display_id_count; ++monitor_id) {
> + uint32_t device_display_id = device_display_id_start +
> monitor_id;
> + instance->st->device_display_ids[monitor_id] =
> device_display_id;
> + g_debug(" monitor ID %u -> device display ID %u",
> + monitor_id, device_display_id);
> + }
> +
> + instance->st->monitors_count = device_display_id_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..547d3d93 100644
> --- a/server/spice-qxl.h
> +++ b/server/spice-qxl.h
> @@ -115,6 +115,52 @@ void spice_qxl_gl_draw_async(QXLInstance
> *instance,
> uint32_t w, uint32_t h,
> uint64_t cookie);
>
> +/* since spice 0.14.2 */
> +
> +/**
> + * spice_qxl_set_device_info:
> + * @instance the QXL instance to set the path to
> + * @device_address the path of the device this QXL instance belongs
> to
> + * @device_display_id_start the starting device display ID of this
> interface,
> + * i.e. the one monitor ID 0 maps to
perhaps reword this last line:
i.e. the device display ID of monitor ID 0
> + * @device_display_id_count the total number of device display IDs
> on this
> + * interface
> + *
> + * Sets the device information for this QXL interface, i.e. the
> hardware
> + * address (e.g. PCI) of the graphics device and the IDs of the
> displays of the
> + * graphics device that are exposed by this interface (device
> display IDs).
> + *
> + * The supported device address format is:
> + * "pci/<DOMAIN>/<SLOT>.<FUNCTION>/.../<SLOT>.<FUNCTION>"
> + *
> + * The "pci" identifies the rest of the string as a PCI address. It
> is the only
> + * 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. All of <DOMAIN>, <SLOT>, <FUNCTION> are
> hexadecimal numbers
> + * with the following number of digits:
> + * <DOMAIN>: 4
> + * <SLOT>: 2
> + * <FUNCTION>: 1
> + *
> + * The device_display_id_{start,count} denotes the sequence of
> device display
> + * IDs that map to the zero-based sequence of monitor IDs provided
> by monitors
> + * config on this interface. For example with
> device_display_id_start = 2 and
> + * device_display_id_count = 3 you get the following mapping:
> + * monitor_id -> device_display_id
> + * 0 -> 2
> + * 1 -> 3
> + * 2 -> 4
> + *
> + * Note this example is unsupported in practice. The only supported
> cases are
> + * either a single device display ID (count = 1) or multiple device
> display IDs
> + * in a sequence starting from 0.
> + */
Gerd previously objected to using an example of an unsupported
scenario. But it doesn't seem that you changed anything here. I don't
care too much, myself.
> +void spice_qxl_set_device_info(QXLInstance *instance,
> + const char *device_address,
> + uint32_t device_display_id_start,
> + uint32_t device_display_id_count);
> +
> typedef struct QXLDevInitInfo {
> uint32_t num_memslots_groups;
> uint32_t num_memslots;
> diff --git a/server/spice-server.syms b/server/spice-server.syms
> index edf04a42..ac4d9b14 100644
> --- a/server/spice-server.syms
> +++ b/server/spice-server.syms
> @@ -173,3 +173,8 @@ 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_info;
> +} SPICE_SERVER_0.13.2;
