[Spice-devel] monitors_config rework: proposed protocol changes

[Lukáš Hrázký]

Hi all,

after posting the PoC patch series for the monitors_config rework and
the discussion there getting a bit hard to follow, I thought I'd try to
summarize the proposed protocol. There are also spice-gtk API changes
to consider, which I've decided to leave for a later email.

There are two variants for consideration right now:


Variant 1
=========

server -> client
----------------

struct SpiceMsgDisplayHead {
-   uint32_t id;
+   uint32_t monitor_id;
    uint32_t surface_id;
+   int32_t guest_output_id;
    uint32_t width;
    uint32_t height;
    uint32_t x;
    uint32_t y;
    uint32_t flags;
+   uint8_t reserved[?];
};

struct SpiceMsgDisplayMonitorsConfig {
    uint16_t count;
    uint16_t max_allowed;
    struct SpiceMsgDisplayHead heads[0];
};


client -> server -> vd_agent
----------------------------

struct VDAgentMonConfig {
+   uint32_t channel_id;
+   uint32_t monitor_id;
+   int32_t guest_output_id;
    uint32_t height;
    uint32_t width;
    uint32_t depth;
    int32_t x;
    int32_t y;
+   uint8_t reserved[?];
};

struct VDAgentMonitorsConfig {
    uint32_t num_of_monitors;
    uint32_t flags;
    struct VDAgentMonConfig monitors[];
};


The "SpiceMsgDisplayHead::id" is renamed to "monitor_id" for
consistency. It is defined in spice-common, so I believe we can do that
without breaking anything.

The name of the "guest_output_id" is subject to discussion, I'd prefer
to make it clear it is an ID originating from the guest.
"guest_display_id" is an option.

The value of the "guest_output_id" would be a zero-based sequence, an
index of an output in xrandr, with -1 meaning the value is unset. (as
opposed to the v1 of the patch series, where the sequence was one-
based, 0 meaning it's unset).

The "reserved" fields are added based on Christophe de Dinechin's
suggestion to prepare for possible features described in [1].


Variant 2
=========

The protocol changes are the same as in Variant 1, except we do not add
the guest_output_id to the messages (and only add channel_id and
monitor_id to VDAgentMonConfig). The guest_output_id can be kept in a
map on the server, to which the key would be the (channel_id,
monitor_id) pair.

The guest_output_id can then be fetched from the map when needed and
after that the mechanisms are identical in theory. In practice the
guest_output_id is needed mainly (and most probably only) in messages
to the vd_agent. As of now for example the VDAgentMonitorsConfig
message is forwarded to the vd_agent as-is on the server, which would
need bigger code changes and different messages for client -> server
and for server -> agent.

Variant 2 would also require to change the protocol of
SpiceMsgcMousePosition (the mouse position even sent from the client to
the server), which now sends a display_id, which is equivalent to
guest_output_id, but now would have to send (channel_id, monitor_id).
Doing this is not necessarily a bad idea even if we go with Variant 1.


I think the decision between the variants boils down to whether we want
all the monitors_config data go to the client (and therefore adding
anything means a protocol change), or keep the client <-> server
protocol to the bare minimum and store the rest on the server. I would
say, though, that majority of the data we would ever want to add to the
monitors_config message would be becase we need it on the client (the
amount of information Christophe wanted to add to the messages for the
client seems to be in accord with that). So in that case the map would
just be a mostly useless complication of the code?

Cheers,
Lukas


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