[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Pavel Grunt
pgrunt at redhat.com
Mon Oct 10 14:09:11 UTC 2016
Hi Marc-André,
On Mon, 2016-10-10 at 08:37 -0400, Marc-André Lureau wrote:
> 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).
The configuration messages are sent only when agent is running.
It is happening all the time that the configuration message was
technically not successful (linux guest supports only width of
multiple of 8) and in fact it is the reason of this flickering bug.
The problem is that client has no info that the message was
unsuccessful.
>
> Imho, it should be fine for the client to send the same monitor
> config
How should client know that it should resend the message ? Would it
require a new message ?
> , the server should however not notify of changes if none happened.
(the change has happened - the primary surface was destroyed)
iow some component should just ignore the size request - it can be
client, server or driver (eg. Windows driver ignores that, QXL +Xorg
driver ignores that).
Imho server does a correct job - it gets info from the driver that the
primary surface was destroyed and it forwards the message to the
client.
I think this patch is just a workaround of the real root cause. It is
only GNOME on Wayland having the issue (iirc QXL was blacklisted by
GNOME - maybe it is a part of the problem).
The other option is to stop using QXL for kms and the patch can be
dropped.
Jonathon and You are right that there is a risk of introducing a
regression - we don't have tests for this area
Pavel
>
> > v2:
> > https://lists.freedesktop.org/archives/spice-devel/2015-October/02
> > 2784.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