[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Marc-André Lureau
mlureau at redhat.com
Mon Oct 10 12:37:04 UTC 2016
Hi
----- Original Message -----
> When the guest receives the monitor configuration message, it replies
> (through spice-server) by destroying the primary surface, which makes
> the SpiceDisplay disabled if its "resize-guest" property is used.
> This change of the display state (disabled/enabled) leads to sending
> a new monitor config message (containing the disabled display) after
> a second. Then spice-server sends the monitor configuration message,
> spice-gtk replies back and we may end up with a loop of monitor
> configuration messages even if no real change of monitor configuration
> happens.
It's hard follow, could you explain step by step what you mean.
> To avoid the loop, the new monitor configuration messages are only sent
> if they are different from the current monitor configuration.
>
> All the issues are visible only with Wayland & QXL driver on the guest
>
> Fixes:
> https://bugzilla.redhat.com/show_bug.cgi?id=1266484
> https://bugs.freedesktop.org/show_bug.cgi?id=94950
> ---
> v3: rebased, fixed typos
In v2, Jonathon raised a valid concern: "However, it's theoretically possible that the last sent configuration may not have been successful." For ex, vdagent wasn't yet started (whatever handles the monitor config change).
Imho, it should be fine for the client to send the same monitor config, the server should however not notify of changes if none happened.
> v2:
> https://lists.freedesktop.org/archives/spice-devel/2015-October/022784.html
> ---
> src/channel-main.c | 59
> +++++++++++++++++++++++++++++++++++-------------------
> 1 file changed, 38 insertions(+), 21 deletions(-)
>
> diff --git a/src/channel-main.c b/src/channel-main.c
> index 990a06a..1a46819 100644
> --- a/src/channel-main.c
> +++ b/src/channel-main.c
> @@ -105,7 +105,8 @@ struct _SpiceMainChannelPrivate {
> guint agent_msg_pos;
> uint8_t agent_msg_size;
> uint32_t agent_caps[VD_AGENT_CAPS_SIZE];
> - SpiceDisplayConfig display[MAX_DISPLAY];
> + SpiceDisplayConfig display_current[MAX_DISPLAY]; /* current
> configuration of spice-widgets */
> + SpiceDisplayConfig display_new[MAX_DISPLAY]; /* new requested
> configuration */
> gint timer_id;
> GQueue *agent_msg_queue;
> GHashTable *file_xfer_tasks;
> @@ -1098,11 +1099,11 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>
> if (spice_main_agent_test_capability(channel,
> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG)) {
> - monitors = SPICE_N_ELEMENTS(c->display);
> + monitors = SPICE_N_ELEMENTS(c->display_new);
> } else {
> monitors = 0;
> - for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> - if (c->display[i].display_state == DISPLAY_ENABLED)
> + for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> + if (c->display_new[i].display_state == DISPLAY_ENABLED)
> monitors += 1;
> }
> }
> @@ -1117,24 +1118,25 @@ gboolean
> spice_main_send_monitor_config(SpiceMainChannel *channel)
>
> CHANNEL_DEBUG(channel, "sending new monitors config to guest");
> j = 0;
> - for (i = 0; i < SPICE_N_ELEMENTS(c->display); i++) {
> - if (c->display[i].display_state != DISPLAY_ENABLED) {
> + for (i = 0; i < SPICE_N_ELEMENTS(c->display_new); i++) {
> + if (c->display_new[i].display_state != DISPLAY_ENABLED) {
> if (spice_main_agent_test_capability(channel,
> VD_AGENT_CAP_SPARSE_MONITORS_CONFIG))
> j++;
> continue;
> }
> mon->monitors[j].depth = c->display_color_depth ?
> c->display_color_depth : 32;
> - mon->monitors[j].width = c->display[i].width;
> - mon->monitors[j].height = c->display[i].height;
> - mon->monitors[j].x = c->display[i].x;
> - mon->monitors[j].y = c->display[i].y;
> + mon->monitors[j].width = c->display_new[i].width;
> + mon->monitors[j].height = c->display_new[i].height;
> + mon->monitors[j].x = c->display_new[i].x;
> + mon->monitors[j].y = c->display_new[i].y;
> CHANNEL_DEBUG(channel, "monitor #%d: %ux%u+%d+%d @ %u bpp", j,
> mon->monitors[j].width, mon->monitors[j].height,
> mon->monitors[j].x, mon->monitors[j].y,
> mon->monitors[j].depth);
> j++;
> }
> + memcpy(c->display_current, c->display_new, MAX_DISPLAY *
> sizeof(SpiceDisplayConfig));
>
> if (c->disable_display_align == FALSE)
> monitors_align(mon->monitors, mon->num_of_monitors);
> @@ -1469,13 +1471,23 @@ static gboolean
> any_display_has_dimensions(SpiceMainChannel *channel)
> c = channel->priv;
>
> for (i = 0; i < MAX_DISPLAY; i++) {
> - if (c->display[i].width > 0 && c->display[i].height > 0)
> + if (c->display_new[i].width > 0 && c->display_new[i].height > 0)
> return TRUE;
> }
>
> return FALSE;
> }
>
> +static gboolean spice_monitor_configuration_changed(SpiceMainChannel
> *channel)
> +{
> + SpiceMainChannelPrivate *c;
> +
> + g_return_val_if_fail(SPICE_IS_MAIN_CHANNEL(channel), FALSE);
> + c = channel->priv;
> +
> + return memcmp(c->display_current, c->display_new, MAX_DISPLAY *
> sizeof(SpiceDisplayConfig)) != 0;
> +}
> +
> /* main context*/
> static gboolean timer_set_display(gpointer data)
> {
> @@ -1488,6 +1500,11 @@ static gboolean timer_set_display(gpointer data)
> if (!c->agent_connected)
> return FALSE;
>
> + if (!spice_monitor_configuration_changed(channel)) {
> + SPICE_DEBUG("Not sending monitors config, configuration has not
> changed");
> + return FALSE;
> + }
> +
> if (!any_display_has_dimensions(channel)) {
> SPICE_DEBUG("Not sending monitors config, at least one monitor must
> have dimensions");
> return FALSE;
> @@ -1499,7 +1516,7 @@ static gboolean timer_set_display(gpointer data)
> /* ensure we have an explicit monitor configuration at least for
> number of display channels */
> for (i = 0; i < spice_session_get_n_display_channels(session); i++)
> - if (c->display[i].display_state == DISPLAY_UNDEFINED) {
> + if (c->display_new[i].display_state == DISPLAY_UNDEFINED) {
> SPICE_DEBUG("Not sending monitors config, missing
> monitors");
> return FALSE;
> }
> @@ -2573,17 +2590,17 @@ void spice_main_update_display(SpiceMainChannel
> *channel, int id,
>
> c = SPICE_MAIN_CHANNEL(channel)->priv;
>
> - g_return_if_fail(id < SPICE_N_ELEMENTS(c->display));
> + g_return_if_fail(id < SPICE_N_ELEMENTS(c->display_new));
>
> SpiceDisplayConfig display = {
> .x = x, .y = y, .width = width, .height = height,
> - .display_state = c->display[id].display_state
> + .display_state = c->display_new[id].display_state
> };
>
> - if (memcmp(&display, &c->display[id], sizeof(SpiceDisplayConfig)) == 0)
> + if (memcmp(&display, &c->display_new[id], sizeof(SpiceDisplayConfig)) ==
> 0)
> return;
>
> - c->display[id] = display;
> + c->display_new[id] = display;
>
> if (update)
> update_display_timer(channel, 1);
> @@ -2785,14 +2802,14 @@ void
> spice_main_update_display_enabled(SpiceMainChannel *channel, int id, gboole
>
> if (id == -1) {
> gint i;
> - for (i = 0; i < G_N_ELEMENTS(c->display); i++) {
> - c->display[i].display_state = display_state;
> + for (i = 0; i < G_N_ELEMENTS(c->display_new); i++) {
> + c->display_new[i].display_state = display_state;
> }
> } else {
> - g_return_if_fail(id < G_N_ELEMENTS(c->display));
> - if (c->display[id].display_state == display_state)
> + g_return_if_fail(id < G_N_ELEMENTS(c->display_new));
> + if (c->display_new[id].display_state == display_state)
> return;
> - c->display[id].display_state = display_state;
> + c->display_new[id].display_state = display_state;
> }
>
> if (update)
> --
> 2.10.1
>
> _______________________________________________
> Spice-devel mailing list
> Spice-devel at lists.freedesktop.org
> https://lists.freedesktop.org/mailman/listinfo/spice-devel
>
More information about the Spice-devel
mailing list