[Spice-devel] [PATCH spice-gtk v3] main: Send monitor config only when it changes
Pavel Grunt
pgrunt at redhat.com
Tue Oct 11 14:56:42 UTC 2016
On Tue, 2016-10-11 at 10:36 -0400, Marc-André Lureau wrote:
>
> ----- Original Message -----
> > Hi,
> >
> > 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.
> >
> > sorry, I overlooked your question
> >
> > I did more investigation and following is happening from clients
> > perspective:
> > 1. let window width be 401
> > 2. client sends request to resize
> > 3. client receives PRIMARY_DESTROY
> > 4. client marks the display as disabled (config in the main
> > channel)
> > 5. client receives PRIMARY_CREATE + dimensions of display - width
> > is
> > 400
> > 6. client sends a new resize request since resize-guest is on, and
> > display width != window width
> >
> > Flickering is PRIMARY_DESTROY & PRIMARY_CREATE
>
> Is there a way to prevent DESTROY & CREATE of same size on
> server/guest side? why not?
The problem is that it is not the same size, since client requests 401
and driver creates 400.
Maybe it would be possible to delay the DESTROY to the moment the new
surface is created and send it, as you said, only if there is a
change.
Pavel
> >
> > > > 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-Octobe
> > > > r/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_MONI
> > > > TORS
> > > > _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_MONI
> > > > TORS
> > > > _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
> > > >
> >
> > _______________________________________________
> > 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